plan: shadow-input-assembler

[yihanzhu]

Tracks #262

High-risk plan for the merged spec (G2, PR #273, risk: high). Frontmatter records spec-blob ab212e82359ba3132fa6127194b61fa484bb1a85 (main's current work/shadow-input-assembler/spec.md). This PR changes only work/shadow-input-assembler/plan.md; merging it accepts the plan. Implementation follows on ystack/impl/shadow-input-assembler from updated main. It does not close the intake issue.

[cloudflare-workers-and-pages]

Deploying ystack with    Cloudflare Pages

Latest commit:	5e627ff
Status:	✅  Deploy successful!
Preview URL:	https://f18090a4.fabrica-6yx.pages.dev
Branch Preview URL:	https://ystack-plan-shadow-input-ass.fabrica-6yx.pages.dev

View logs

[yihanzhu]

Codex reviewer (cross-vendor, read-only)

Reviewed-head: fcb5712
Reviewed-base: 986531a
reviewer: gpt-5.5 @ high

Posted verbatim by codex-review.sh (codex exec review --json --base refs/codex-review/279-67367/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

The plan currently specifies an argument-check order that violates the spec's refusal taxonomy and a trap behavior that would remove successful outputs. These are blocking issues for a high-risk implementation plan.

Full review comments:

• [P1] Validate usage arguments before running jq — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.qpYxB0twXO/work/shadow-input-assembler/plan.md:240-242
  For a relative <jq-binary> or bad repository/commit argument, this order runs the pinned-jq and time_ok checks before the usage/path checks, so the script can execute or fail on that path and report E_RUNTIME instead of the required E_USAGE (work/shadow-input-assembler/spec.md:371-378). Move the cheap absolute-path/repository-id/commit-width checks ahead of any hashing or invocation of $jq_bin.

• [P1] Preserve committed outputs on successful exit — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.qpYxB0twXO/work/shadow-input-assembler/plan.md:268-270
  On a successful run the normal EXIT trap still fires after all seven staged files have been moved, but this plan says the trap removes every recorded commit destination, which would delete the successful outputs instead of leaving the required documents (work/shadow-input-assembler/spec.md:1159-1164). The plan should explicitly clear/disable destination cleanup, or only arm it for failure, before the normal exit path.

[yihanzhu]

Codex reviewer (cross-vendor, read-only)

Reviewed-head: fc8192f
Reviewed-base: 1b46de0
reviewer: gpt-5.5 @ high

Posted verbatim by codex-review.sh (codex exec review --json --base refs/codex-review/279-75280/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

The plan omits a required source-directory physicality check and leaves gaps/contradictions in the required proof. These issues would let the subsequent implementation pass the plan while violating the accepted spec.

Full review comments:

• [P2] Reject symlinked source repositories before the copy — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.5RZLz4Ky5h/work/shadow-input-assembler/plan.md:340-346
  For a <source-git-dir> that is a symlink to a bare repository, the copied spans here do not include the materializer's physical_dir "$source_git_dir" check at materialize.sh:118, so find, config reads, and git --git-dir can operate through the symlink and the assembler may accept a non-physical source. The accepted spec requires a physical bare repository and says existence/object checks are decided by the copy, but the physical-directory predicate is outside these spans (work/shadow-input-assembler/spec.md:106-126), so the plan needs an explicit E_TARGET physical-dir check before source_pure and a matching negative case.

• [P2] Cover every required refusal class in the proof — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.5RZLz4Ky5h/work/shadow-input-assembler/plan.md:514-520
  When the implementation follows this proof list, several requirement-12 refusal classes can be untested even though the plan says there is one case per refusal: there is no explicit non-JSON/BOM case for E_PARSE, oversized input/claim/output case for E_LIMIT, or inconsistent profile/finished-input relation case for E_RELATION (work/shadow-input-assembler/spec.md:371-424). That lets the assembler mis-map or skip those refusals while still satisfying the planned final proof, so add concrete negative cases for the omitted classes.

• [P2] Keep ShellCheck disables forbidden for the assembler — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.5RZLz4Ky5h/work/shadow-input-assembler/plan.md:570-574
  For an implementation that adds a file-level disable=SC2016, this proof would accept it, but the accepted spec says ShellCheck must pass on the assembler with no new shellcheck disable directive (work/shadow-input-assembler/spec.md:1052-1056). Since the repository pins ShellCheck as a hard local/CI check (AGENTS.md:90-94), allowing a new directive here weakens that contract; make the expected count 0 unless the spec is updated.

[yihanzhu]

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 00659a0
Reviewed-base: 1b46de0
reviewer: gpt-5.5 @ high

Posted verbatim by codex-review.sh (codex exec review --json --base refs/codex-review/279-81791/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

The plan contains contradictory and unimplementable proof instructions, including an output-limit fixture that will be rejected before the intended limit check. These should be corrected before the plan is accepted as the implementation contract.

Full review comments:

• [P2] Make the output-size limit case reachable — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.nRSqr68NCb/work/shadow-input-assembler/plan.md:194-199
  When implementing the output-size E_LIMIT case, padding skill_sources/tool_sources in the resolved profile will not reach the finished-output size check: the shipped default profile has no requested skills/tools, and profile_set_ok requires the resolved binding's skill/tool sources to match the pinned profile binding, so this fixture is refused earlier as a profile relation/shape error. This leaves requirement 12's finished-output limit proof unimplementable unless the plan names a valid way to grow input.json past 8 MiB.

• [P2] Exclude the copied shellcheck directive — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.nRSqr68NCb/work/shadow-input-assembler/plan.md:109-114
  If this step is followed literally, copying shadow-slice.test.sh:1-56 also copies its line 2 # shellcheck disable=SC2016, but this plan later requires zero new shellcheck disable directives and the proof greps for 0 in the new test. That makes the implementation instructions self-contradictory; the bootstrap copy should explicitly exclude the directive.

• [P3] Align the proof with the copied pass helper — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.nRSqr68NCb/work/shadow-input-assembler/plan.md:651-652
  The proof says the test prints the final summary line and nothing else, but step 0.1 tells the implementer to copy pass unchanged from shadow-slice.test.sh, whose helper prints an ok ... line for every check. If the test uses that helper, this proof condition is impossible even when all checks pass, so either the proof should allow the ok lines or the helper instruction should change.

[yihanzhu]

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 379f9c5
Reviewed-base: 307d85c
reviewer: gpt-5.5 @ high

Posted verbatim by codex-review.sh (codex exec review --json --base refs/codex-review/279-87843/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

The plan contains several concrete instructions that would either omit required source guards or make required tests/proofs exercise the wrong condition. These should be corrected before accepting the plan as implementation guidance.

Full review comments:

• [P2] Copy the full source-purity tail span — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.6yMSIayX7I/work/shadow-input-assembler/plan.md:489-489
  If the implementer follows this line, materialize.sh lines 348-354 are skipped, which drops the non-*.sample hook guard and the repository object-format comparison before the commit checks. That lets hook-bearing sources or commit-width/object-format mismatches avoid the intended E_TARGET coverage required by the spec; this line should match the surrounding 348-360 span.

• [P2] Size the padded claim past the actual 1 MiB cap — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.6yMSIayX7I/work/shadow-input-assembler/plan.md:208-210
  Using “a million x” does not necessarily exceed the 1,048,576-byte claim bound; the fixture claim is only a few KB, so that file can stay under the limit and fail to exercise E_LIMIT. Specify padding based on the measured file size, or use more than 1 MiB of padding, so the test reliably hits snapshot_bounded before parsing.

• [P2] Keep source path failures mapped to E_TARGET — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.6yMSIayX7I/work/shadow-input-assembler/plan.md:388-389
  This paragraph includes <source-git-dir> in “those paths” and then says missing or symlinked paths are deferred to the E_RUNTIME step, but the spec classifies a non-physical source Git directory as E_TARGET and this plan later requires the symlink source case to return E_TARGET. Exclude the source directory from this deferred E_RUNTIME bucket so an implementation does not refuse it with the wrong id before the explicit physical source check.

[yihanzhu]

Codex reviewer (cross-vendor, read-only)

Reviewed-head: cb09dcd
Reviewed-base: 307d85c
reviewer: gpt-5.5 @ high

Posted verbatim by codex-review.sh (codex exec review --json --base refs/codex-review/279-93493/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

The added plan contains proof/test instructions that are not reliable as written. These issues should be corrected before treating the plan as an implementation contract.

Full review comments:

• [P2] Use a real shell for the clean-path proof — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.5FKqfeZeiN/work/shadow-input-assembler/plan.md:836-837
  On Linux/GitHub runners this proof command relies on command being an external executable, but command is only a shell builtin there, so env -i ... command -v ... fails before checking any tools. Since the Proof section is meant to be pasted for the implementation PR, wrap this in /bin/bash -c (or another shell) so the environment check is runnable on CI-like hosts.

• [P2] Avoid padding nonexistent scope-ref paths — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.5FKqfeZeiN/work/shadow-input-assembler/plan.md:244-248
  For the E_LIMIT output-size case, this “better” padding target is not valid: selection_ref and repository_context_ref are scope_refs whose subjects are content refs, not Git object refs with location.value, and their content_ids are capped by id_ok. A test following this path would fail schema/profile validation before the finished-output size check, so it would not prove the intended limit.

[yihanzhu]

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 5e627ff
Reviewed-base: 307d85c
reviewer: gpt-5.5 @ high

Posted verbatim by codex-review.sh (codex exec review --json --base refs/codex-review/279-98332/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

The patch adds only the hash-linked plan document for the high-risk work, and I found no discrete actionable defects in the plan relative to the accepted spec or repository rules.