spec: shadow-env-self-host

[yihanzhu]

Tracks #263

G2 spec for the merged intent (G1, PR #265). Frontmatter records intent-blob a606f5f2f67d247d4d7827684322e15ae10b4dc9 (main's current work/shadow-env-self-host/intent.md) and risk: high (the registry is an authorization list the driver enforces). Merging this PR accepts both. It does not close the intake issue.

This PR adds only work/shadow-env-self-host/spec.md.

[cloudflare-workers-and-pages]

Deploying ystack with    Cloudflare Pages

Latest commit:	298404f
Status:	✅  Deploy successful!
Preview URL:	https://b3d28e85.fabrica-6yx.pages.dev
Branch Preview URL:	https://ystack-spec-shadow-env-self.fabrica-6yx.pages.dev

View logs

[yihanzhu]

Codex reviewer (cross-vendor, read-only)

Reviewed-head: ea36aff
Reviewed-base: bfb1677
reviewer: gpt-5.5 @ high

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

The spec is internally inconsistent about the number of files in scope, which can make the accepted review-size/scope record wrong for the implementation PR.

Review comment:

• [P2] Align the review-size file count with the required edits — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.28BuCPsaU3/work/shadow-env-self-host/spec.md:77-77
  As written, this spec cannot be satisfied as stated: R1, R5, and R6 require changes to the registry, one test, and three docs, which is five files, but this review-size record says four files. Because the repo uses the accepted spec/plan to record review_size evidence (work/README.md:71-73), this should be corrected before G2 acceptance so the later plan/implementation is checked against a consistent scope.

[yihanzhu]

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 42033a0
Reviewed-base: bfb1677
reviewer: gpt-5.5 @ high

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

The spec leaves an important part of the registry contract untested, so the implementation proof could pass with authorization metadata that violates the stated requirements.

Review comment:

• [P2] Pin the complete registry contents in the test — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.fyFZgZJJ5T/work/shadow-env-self-host/spec.md:42-45
  For this registry change, R1/R2 make the new entry's description, exact key set, and surrounding registry fields part of the contract, but the proposed registry-contents assertion only checks ids, scopes, and proof states. A later implementation could ship the wrong description, an extra member, or a changed registry_version while still satisfying the listed proof, so the high-risk gate would not catch drift in this authorization file (AGENTS.md:102-106). Please make the assertion compare the full expected registry or at least include keys, descriptions, and header fields.

[yihanzhu]

Codex reviewer (cross-vendor, read-only)

Reviewed-head: ac7fae4
Reviewed-base: bfb1677
reviewer: gpt-5.5 @ high

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

The spec approves a registry-only implementation for a security-control change, but the current driver treats registry membership as a generic id allowlist. This would widen execution beyond the documented self-host ystack source scope.

Review comment:

• [P1] Bind the self-host entry to the target repo — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.2k6AVfow5a/work/shadow-env-self-host/spec.md:112-113
  When the implementation follows this and only edits the registry, a caller can run the driver against a non-ystack bare repository by supplying a satisfied sandbox claim whose caller-controlled id is env.local-macos-ystack-self: reproduce.sh only checks that the id appears in .body.environments and does not inspect evidence_scope, description, OS, or repository (shadow/v1/reproduce.sh:267-269). That means the self-host entry also authorizes the external-target/arbitrary-source scenario that lines 133-134 declare out of scope, so the spec needs a repo/environment binding instead of forbidding driver changes.

[yihanzhu]

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 424d223
Reviewed-base: bfb1677
reviewer: gpt-5.5 @ high

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

The change adds only the high-risk spec artifact, with a matching intent blob and clear bounded requirements for the later implementation. I did not find a discrete introduced defect that would break the artifact chain or direct the implementation toward an obviously incorrect result.

[yihanzhu]

@yihanzhu — G2 merge request G2-3 (merge-ready)

Codex review (gpt-5.5) is clean at exactly this head/base — Reviewed-head 424d223422b5de97444e6cae7377e4c3907d6b65, Reviewed-base bfb1677ffd4a688ebf7fd31e7420e5d8d3a0435f (current main) — and the required ci check passed. Four review rounds shaped this spec; the last one was a real P1: the registry was an id-only allowlist, so the spec now binds every entry to a target_repository_id and has the driver enforce it (mismatch falls into the existing environment.unlisted outcome; no consumer changes). Re-queried just now: head and base unchanged; intake #263 carries no ready/claimed/needs-human; this PR carries no claimed/needs-human.

Merging this PR is G2 for shadow-env-self-host: it accepts the spec AND risk: high (the registry is an authorization list the driver enforces). If you disagree with high, say so instead of merging. yshifu never merges; please squash-merge it yourself.

Next for this slug (high-risk path): a plan-only PR on ystack/plan/shadow-env-self-host, independent review + CI, your merge; then implementation.

[yihanzhu]

Codex reviewer (cross-vendor, read-only)

Reviewed-head: ce9ed7e
Reviewed-base: 5090b42
reviewer: gpt-5.5 @ high

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

The spec's central security binding still relies on caller-controlled incident data, so the planned implementation would not actually restrict the newly listed environment to ystack's source repository.

Review comment:

• [P1] Bind the environment to a verified repository source — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.6VUVKWqwIC/work/shadow-env-self-host/spec.md:58-60
  When the driver is invoked with caller-supplied incident/materialization inputs, .body.target_repository_id is not an independent fact: reproduce.sh reads it from the incident and passes the same value to the materializer as the source repository id. A caller can set those records to repo.ystack while pointing source_git_dir at another bare repository, so the proposed lookup still authorizes the self-host environment outside ystack. Please require an independently supplied or verified source identity, or a trusted mapping/path check, rather than binding only to the incident field.

[yihanzhu]

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 38cd172
Reviewed-base: 5090b42
reviewer: gpt-5.5 @ high

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

The spec adds a high-risk environment authorization path but its proposed root-check logic can ignore a failing git command if matching stdout was produced. That should be fixed before the plan is accepted and implemented.

Review comment:

• [P1] Check rev-list's status before trusting stdout — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.jbTgR5A7WZ/work/shadow-env-self-host/spec.md:88-89
  When the supplied source repo makes rev-list print the expected root and then exit nonzero, the command substitution still preserves that stdout and the script continues because there is no set -e; pipefail only makes the assignment's status nonzero if it is tested. That would let environment_listed=yes despite the spec requiring any git failure to stay on environment.unlisted, so the design should explicitly check the pipeline status before comparing the captured root.

[yihanzhu]

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 39a1081
Reviewed-base: 6526999
reviewer: gpt-5.5 @ high

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

The spec leaves a bypass in the proposed source-root binding: Git can satisfy the root check through alternate/common object stores before the later materializer guard rejects them. That undermines the main safety property this high-risk spec is meant to add.

Review comment:

• [P1] Reject alternate object stores before root check — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.tzJL6CQknE/work/shadow-env-self-host/spec.md:80-83
  When source_git_dir contains objects/info/alternates or a commondir pointing at an authorized object store, this rev-list can resolve $commit_id and the matching root from outside the supplied repository. The materializer refuses those impurities only later, so such a source reaches sandbox evaluation/materialization instead of the R8 environment.unlisted path with absent sections. Require the same git-dir impurity checks before this lookup, or otherwise make the root check ignore alternate/common object stores.

[yihanzhu]

Codex reviewer (cross-vendor, read-only)

Reviewed-head: b5229fc
Reviewed-base: 4965175
reviewer: gpt-5.5 @ high

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

The spec requires copying code that will make an existing required test fail, without specifying the needed test guard update. This should be fixed before the spec is accepted as the implementation contract.

Review comment:

• [P2] Update the Git-command guard for the copied config read — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.nEI8kAweFW/work/shadow-env-self-host/spec.md:110-116
  When R7 copies the materializer predicates into reproduce.sh, it brings in /usr/bin/git config --file ...; the existing shadow-slice guard rejects any git config in the reproducer (scripts/test/shadow-slice.test.sh:612-620), so R19's required all-pass proof cannot pass unless this spec also updates that guard/pass message to allow the read-only config snapshot.

[yihanzhu]

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 10b024a
Reviewed-base: 40582c2
reviewer: gpt-5.5 @ high

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

The spec directs an implementation that could run caller-controlled PATH commands before authorization, and it also contains a scope contradiction around the new registry fields. These should be corrected before accepting the artifact.

Full review comments:

• [P1] Sanitize PATH before copying materializer predicates — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.PUtWqRyciV/work/shadow-env-self-host/spec.md:110-117
  When this copy runs inside reproduce.sh, it will not be under the materializer's /usr/bin/env -i PATH=/usr/bin:/bin wrapper, and the copied spans include bare find calls. With a caller-controlled PATH and a listed environment claim, that would execute the caller's find before the root binding and sandbox evaluation, so the spec should require a fixed PATH around source_pure or the driver before running the verbatim copy.

• [P2] Include target_repository_id in scoped registry fields — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.PUtWqRyciV/work/shadow-env-self-host/spec.md:482-484
  This out-of-scope bullet conflicts with R1/R2/R6: the change adds both target_repository_id and source_root_commit, and the repository-id lookup cannot be implemented without the former. If followed literally, the implementation either omits the id binding or appears to violate the accepted scope, so this should name both allowed registry fields.

[yihanzhu]

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 9a819b8
Reviewed-base: 40582c2
reviewer: gpt-5.5 @ high

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

The spec directs a security-sensitive implementation but misses shell-function injection around copied bare commands and does not require the git identity command to succeed before accepting its output. These should be fixed before the spec is accepted as the basis for the high-risk plan.

Full review comments:

• [P1] Sanitize shell functions before copied find calls — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.SdPIzeCaBu/work/shadow-env-self-host/spec.md:160-172
  When reproduce.sh is invoked with an exported Bash function named find, PATH=/usr/bin:/bin does not force the copied bare find commands to /usr/bin/find; Bash resolves functions before PATH, and unlike the materializer's clean entry, the driver does not run privileged or unset imported functions. That lets the same bypass R14c is trying to prevent silence the worktrees/promisor/hooks checks, so the spec needs to carry over function cleansing or otherwise make those calls non-overridable, plus add a regression for function injection.

• [P2] Check rev-list status before accepting root — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.SdPIzeCaBu/work/shadow-env-self-host/spec.md:499-504
  For a crafted repository where git rev-list writes the expected root before later failing, the assignment captures matching stdout but the pseudocode ignores the non-zero pipeline status; set -uo pipefail without -e will not stop it. Since R8 says any git failure must leave the run environment.unlisted, capture the pipeline status and require it to be zero before setting environment_listed=yes.

[yihanzhu]

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 22e4b58
Reviewed-base: 40582c2
reviewer: gpt-5.5 @ high

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

The spec leaves a startup-environment gap in a security-sensitive direct invocation path and does not account for the PR's own size overrun under the repository rules.

Full review comments:

• [P1] Require a clean interpreter for direct bash runs — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.AM0O0je7i2/work/shadow-env-self-host/spec.md:131-136
  When the reproducer is launched as /bin/bash "$reproducer" (the invocation R15d requires), Bash processes $BASH_ENV before it reads line 1, so the scrub and re-exec described here run too late to stop that startup file from executing or installing traps that fire during the scrub before the clean process is reached. This leaves the direct-interpreter path claiming a clean entry while still allowing inherited shell startup code; make that path explicitly use bash -p/env -i or stop treating plain bash reproduce.sh as covered.

• [P2] Explain the current PR size overrun — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.AM0O0je7i2/work/shadow-env-self-host/spec.md:653-662
  For this spec PR, the actual diff is a single +957-line artifact, but this size section only argues that the later implementation will be ~300–320 lines and leaves review_size: standard; the repo rule says every PR has a ~300–400 net-line soft budget and that unexplained overruns block review (AGENTS.md:102-106). Either trim/split the spec or record an accepted one-concern size exception for this PR, otherwise the artifact stage cannot be accepted under the rule.

[yihanzhu]

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 298404f
Reviewed-base: 40582c2
reviewer: gpt-5.5 @ high

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

The change adds only the new high-risk spec artifact, and it is hash-linked to the existing intent. I found no discrete, actionable correctness issues in the proposed spec.

[yihanzhu]

Merging under operator decision OD-1 (#275): Codex review clean at Reviewed-head 298404fa7760f61742ee94738f6c6c2f0ef0ab7d / Reviewed-base 40582c2660a58d5fdf641d06d3a1aaeec7e0464d (current main) after twelve review rounds, ci green, carrier labels consistent. This merge is G2 for shadow-env-self-host: it accepts the spec and risk: high. Next: plan-only PR on ystack/plan/shadow-env-self-host.