Skip to content

spec: resolver-trusted-parent - #276

Open
yihanzhu wants to merge 52 commits into
mainfrom
ystack/spec/resolver-trusted-parent
Open

spec: resolver-trusted-parent#276
yihanzhu wants to merge 52 commits into
mainfrom
ystack/spec/resolver-trusted-parent

Conversation

@yihanzhu

Copy link
Copy Markdown
Owner

Tracks #271

G2 spec for the merged intent (G1, PR #272). Frontmatter records intent-blob 0fd28feb8f1de6ce71ccb90e2fce69056ec2ab32 and risk: high (the launch boundary is the resolver's security boundary). Merging this PR accepts both. It does not close the intake issue.

This PR adds only work/resolver-trusted-parent/spec.md.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Sep 10, 2026

Copy link
Copy Markdown

Deploying ystack with  Cloudflare Pages  Cloudflare Pages

Latest commit: 95cdf5b
Status: ✅  Deploy successful!
Preview URL: https://82b6cf39.fabrica-6yx.pages.dev
Branch Preview URL: https://ystack-spec-resolver-trusted.fabrica-6yx.pages.dev

View logs

@yihanzhu

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

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

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

The spec currently contradicts the accepted no-network and human-merge constraints and contains an unimplementable parent-side helper provenance requirement. These issues should be fixed before the artifact is accepted as the basis for implementation.

Full review comments:

  • [P1] Keep jq resolution offline — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.VLvsfQnAaX/work/resolver-trusted-parent/spec.md:114-114
    When the pinned jq is missing or has a bad digest, this spec tells resolve-profile.sh to fetch it, but the accepted intent for this component explicitly says the shipped resolver path has “No network” (work/resolver-trusted-parent/intent.md:30-38). Following this would either fail locked-down step-7 runs or put network I/O ahead of the trusted launch boundary, so require a pre-provisioned caller-supplied jq/cache instead of downloading here.

  • [P1] Remove yshifu merge authorization — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.VLvsfQnAaX/work/resolver-trusted-parent/spec.md:142-145
    For this high-risk plan gate, the current repo state has no active construction overlay, so this sentence grants yshifu a merge path that the working rules do not allow; outside an active identity-matching overlay, “Opening a PR ends an agent's authority; the operator merges” (AGENTS.md:405-408, with config/construction-mode.json:95 currently retired). Leaving this in the accepted spec can cause the next plan PR to be merged by the wrong actor.

  • [P2] Define an attestable helper provenance check — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.VLvsfQnAaX/work/resolver-trusted-parent/spec.md:61-62
    If the parent is invoked with a swapped helper binary, it only receives a path to that binary, while changing resolver/v1/nofollow-snapshot.c is out of scope here; without a trusted build receipt, embedded source identity, or having the parent perform the build itself, it cannot prove the helper “was built from the committed source in this run.” As written, this R5 refusal is not implementable as a parent-side check and will force either a weak placeholder check or a test that proves entry-script behavior instead of the stated boundary.

@yihanzhu

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

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

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

The new high-risk spec contains contradictions and test gaps that would authorize a leaky or insufficiently bound trusted launcher. These should be corrected before accepting the artifact for planning.

Full review comments:

  • [P2] Preserve cleanup when replacing the shell — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.RuHl1MweZz/work/resolver-trusted-parent/spec.md:191-193
    When implemented with exec, the entry shell is replaced and its EXIT trap will not run, so the run directory promised earlier as “removed on exit” will leak the compiled parent/helper plus copied jq/awk after every successful invocation. Either avoid exec so the shell can clean up after the parent exits, or make the parent explicitly own removal of the run directory.

  • [P1] Bind the helper path the runtime actually executes — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.RuHl1MweZz/work/resolver-trusted-parent/spec.md:106-108
    In the same-uid swap case, checking an opened helper file in the parent does not bind what the runtime later executes, because scripts/lib/profile-resolution.sh invokes $YSTACK_RESOLVER_HELPER by path. As written, the accepted mode 0700 also leaves the helper owner-writable, so the file or directory entry can change after the parent's check; require a non-writable helper/run directory before exec or pass an executable descriptor that the runtime actually uses.

  • [P2] Exercise the parent's jq refusal directly — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.RuHl1MweZz/work/resolver-trusted-parent/spec.md:155-157
    With the shipped entry path, a wrong-digest jq is rejected by the entry before the parent runs, so this test requirement can pass while the parent's R5 jq digest/version check is missing or broken. Because R5 makes that a parent refusal, add a direct-parent or otherwise post-entry tamper case that proves the bound jq seen by the parent is rejected.

  • [P2] Narrow the read-boundary claim — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.RuHl1MweZz/work/resolver-trusted-parent/spec.md:128-133
    The “no read outside the repositories named in the map” claim is impossible for the shipped path described here: the entry and parent must read the supplied jq, committed C sources, runtime, compiler/system tools, and copied awk before the resolver reads mapped repositories. This should be narrowed to repository/content reads or explicitly list the trusted local inputs, otherwise the accepted spec overstates the security boundary.

…g with stated residual; direct-parent jq test; narrowed read claim)
@yihanzhu

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 5a18ff3
Reviewed-base: 40582c2
reviewer: gpt-5.5 @ high

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

The added spec has actionable issues in the accepted size bound and in requirements that would let the implementation or test proof miss declared boundary guarantees. These should be corrected before the spec is accepted as the basis for planning.

Full review comments:

  • [P2] Recalculate the review-size exception — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.ERK6p0B3Pg/work/resolver-trusted-parent/spec.md:17-18
    If the implementation follows R2 and copies the existing launcher's supervisor, limit, error-sanitizing, Darwin, and environment-construction code verbatim, the C file alone is much closer to the current launcher's ~600+ reusable lines than the stated ~260, so the later PR is likely to exceed the accepted 650-1000 range before tests/docs are counted. Because this repo requires review-size exceptions to have an evidence-based range and treats unexplained overruns as blocking (AGENTS.md:103-106), please revise the range before G2 accepts it.

  • [P2] Keep compiler temporaries inside the declared boundary — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.ERK6p0B3Pg/work/resolver-trusted-parent/spec.md:289-292
    When cc/clang creates intermediate files, this compile step only places the final binaries in the run directory; without binding the compiler's temp environment, those writes can land in ambient TMPDIR or /tmp, contradicting R7's no-writes-outside-output/run-dir claim and leaving cleanup blind. Require compiler temporaries to be inside the fresh run directory, or relax the write-boundary claim.

  • [P2] Exercise cleanup after the run directory exists — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.ERK6p0B3Pg/work/resolver-trusted-parent/spec.md:251-252
    The wrong-digest jq case fails during the pin check, before the design creates the run directory and installs the cleanup trap, so this refusal does not prove that post-mktemp failures are cleaned up. Use a refusal that happens after the run directory is created or after the parent is launched, such as a bad runtime mode or malformed request.

@yihanzhu

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 52e10fd
Reviewed-base: 40582c2
reviewer: gpt-5.5 @ high

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

The spec leaves gaps in the trusted-code provenance and signal handling for a high-risk launch boundary, and its required test does not actually cover the parent refusals it claims. These issues should be fixed before accepting the artifact.

Full review comments:

  • [P1] Pin the resolver code that the wrapper sources — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.uXN97CM1aF/work/resolver-trusted-parent/spec.md:194-195
    With a working tree where resolver/v1/profile-resolve-runtime.sh still matches the pinned blob but scripts/lib/profile-resolution.sh or resolver/v1/profile-resolution.jq is changed, this check still accepts and /bin/bash immediately sources/loads the changed resolver code before any trusted validation can run. Since this parent is the production provenance boundary, R5 should pin/check the sourced library and jq program, not only the thin wrapper.

  • [P1] Stop the resolver process group on entry signals — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.uXN97CM1aF/work/resolver-trusted-parent/spec.md:103-105
    When INT, TERM, or HUP reaches the entry after trusted-launch has forked the resolver, killing only the entry's direct child kills the C supervisor; the supervised resolver is in its own process group, so it can continue without the wall/process supervisor while the trap deletes its run directory. The signal path needs the parent to kill its process group, or another explicit group handoff, before cleanup is claimed.

  • [P2] Make R10 cover the R5 parent checks it claims — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.uXN97CM1aF/work/resolver-trusted-parent/spec.md:333-340
    Here R10 claims to prove each R5 refusal, but the named cases do not exercise several parent-owned R5 checks such as runtime blob mismatch, non-absolute/symlink request or map paths, overlong allowlist paths, and the compiled-parent mode check; conversely, malformed request is a runtime parse/input refusal and leaked caller variables are an R3 no-copy invariant. An implementation could omit those parent checks and still satisfy this test plan, so add direct-parent cases for the missing checks or narrow the claim.

@yihanzhu

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: eb96ef6
Reviewed-base: 1095ca4
reviewer: gpt-5.5 @ high

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

The spec contains contradictory parent pinning requirements and permits degraded signal-test proof for newly added security-boundary behavior. These should be corrected before accepting the artifact.

Full review comments:

  • [P2] Scope parent pinning to files it actually checks — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.wJiYnQ6KbI/work/resolver-trusted-parent/spec.md:246-251
    When trusted-launch is invoked directly, this says the parent rejects a mismatch in any runtime-loaded file, but the same R5 section later says the parent only re-pins files 1–3 and direct callers lose the module checks. That leaves the plan and tests with incompatible security requirements for the five jq modules, so please scope this parent refusal to the parent-pinned subset or move the missing checks into the parent.

  • [P2] Make the signal test exercise the signal path — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.wJiYnQ6KbI/work/resolver-trusted-parent/spec.md:563-568
    When the default-profile run finishes before SIGTERM lands, this allows the test to pass with a fallback that explicitly does not exercise the new parent handler or kill(-pgid, ...) path. In that timing, an implementation that leaves the resolver process group running could still pass CI, so the spec should require a deterministic long-running fixture or direct-parent child that makes the signal path mandatory.

…sistently; race-free signal test via SIGSTOP)
@yihanzhu

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: a69c86e
Reviewed-base: 1095ca4
reviewer: gpt-5.5 @ high

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

The spec contains internally inconsistent and infeasible test requirements for a high-risk security-boundary component. These should be corrected before accepting the artifact because the downstream plan and proof would otherwise be built on inaccurate requirements.

Full review comments:

  • [P2] Make the unsupported-platform refusal testable — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.WmCo81gjOE/work/resolver-trusted-parent/spec.md:490-495
    On normal supported CI hosts, the entry's platform check is described as using fixed /usr/bin/uname with no PATH/env override, so there is no input that can drive an unsupported platform through the shipped entry. Keeping this as an R10 refusal either leaves an untestable required case or forces adding a test-only platform override to the shipped security wrapper; please specify a testable seam or downgrade this to manual/code-review coverage.

  • [P2] Fix the exact read allowlist — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.WmCo81gjOE/work/resolver-trusted-parent/spec.md:454-458
    When the plan/tests treat R7 as an exact read claim, this “whole list” omits tools this same spec requires, including /usr/bin/uname for platform selection, /usr/bin/mktemp for the run directory, /bin/rm for cleanup, and the concrete compiler path. That makes the spec internally inconsistent: an implementation that performs those required steps violates the stated allowlist, while a test based on this list would miss real local inputs.

  • [P3] Use a reachable overlong-path check — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.WmCo81gjOE/work/resolver-trusted-parent/spec.md:521-523
    For a direct-parent test where the jq argument exceeds PATH_MAX, the copied launcher validates regular_absolute(argv[4], 1) before the cited strlen(argv[4]) guard, so such a path fails as not openable/regular before reaching portable-profile-resolution-launcher.c:662-665. This case therefore won't prove the allowlisted-value buffer refusal it claims; use a reachable overlong value/check or adjust the expected owner/error.

@yihanzhu

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: a8a9374
Reviewed-base: 1095ca4
reviewer: gpt-5.5 @ high

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

The spec adds a materially inaccurate read-boundary claim and widens the write surface beyond the accepted intent. These should be corrected before the artifact is accepted and used to drive implementation.

Full review comments:

  • [P2] Include process-table reads in R7’s read scope — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.M8nNnsywqd/work/resolver-trusted-parent/spec.md:460-474
    For implementations that copy the R4 supervisor, the parent also reads host process-table state: on Linux process_group_count opens /proc and /proc/<pid>/stat, and on Darwin the copied memory/process checks use proc_listallpids/proc_pidinfo. This R7 list says the entry and parent read only the listed files/directories, so the documented read boundary is false on every run that enforces process limits; add those host process-table reads to the scope or narrow the claim.

  • [P2] Keep scratch writes inside the accepted output scope — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.M8nNnsywqd/work/resolver-trusted-parent/spec.md:432-438
    When TMPDIR points outside the caller's output directory, this authorizes compiler outputs, copied tools, and intermediates in a second write root, but the accepted intent constrains the work to “no writes outside the caller's own output” (work/resolver-trusted-parent/intent.md:36). Either put this scratch/run directory under the caller-owned output scope or update the accepted intent before G2 accepts the wider write surface.

@yihanzhu

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: b568fc0
Reviewed-base: e9bea97
reviewer: gpt-5.5 @ high

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

The spec leaves security-boundary behavior under-specified in ways that would either write into an unvalidated output root or make the proposed environment tests/provenance claims unreliable. These should be fixed before accepting the artifact as the basis for implementation.

Full review comments:

  • [P1] Validate the output root before creating .run — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.pzI2u46g2t/work/resolver-trusted-parent/spec.md:246-249
    When <output> is a symlink, not caller-owned, or not mode 0700, the entry still creates .run and runs both compiles before the parent performs the R5 validation. That lets the first writes and compiler intermediates land in a directory that is not yet proven to be the caller's own trusted output root, violating the accepted intent's write-root constraint (work/resolver-trusted-parent/intent.md:36); move the owner/mode/non-symlink/emptiness validation before mkdir, or write only through an already-validated directory handle.

  • [P1] Do not test direct-parent cleanup with loader vars — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.pzI2u46g2t/work/resolver-trusted-parent/spec.md:838-840
    When the polluted direct-parent run includes LD_PRELOAD or DYLD_INSERT_LIBRARIES, those variables are consumed by the dynamic loader before trusted-launch reaches main, so the parent cannot prove it ignores them and the test can emit loader diagnostics or run injected code before the clean environment is built. This conflicts with the spec's own boundary note at work/resolver-trusted-parent/spec.md:1155-1158; drive loader-variable pollution through the entry after it scrubs them, or remove them from the direct-parent invariant.

  • [P2] Scrub the compiler environment before using /usr/bin/cc — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.pzI2u46g2t/work/resolver-trusted-parent/spec.md:1001-1004
    When the caller has compiler-affecting variables such as CPATH, C_INCLUDE_PATH, LIBRARY_PATH, SDKROOT, or DEVELOPER_DIR, these /usr/bin/cc invocations still use caller-selected headers or toolchain inputs even though $CC is ignored. That undermines the provenance claim that pinned sources plus a fixed compiler invocation define the parent/helper; run the pin checks and compiles under an explicit whitelisted environment, rather than clearing only loader/shell variables afterward.

@yihanzhu

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

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

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

The spec has blocking contradictions and security-boundary gaps that would either make the planned test fail or permit behavior the spec explicitly promises to prevent. These should be resolved before using this artifact as the implementation contract.

Full review comments:

  • [P1] Scrub loader variables before running external tools — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.5PdRu8g1Fj/work/resolver-trusted-parent/spec.md:405-407
    On Linux when the caller has LD_PRELOAD/LD_LIBRARY_PATH set, starting the parent via /usr/bin/env -i does not prevent injected code from running below the entry: the dynamic loader processes those variables for /usr/bin/env itself, and earlier absolute tools such as uname/stat run before this point as well. R10's marker assertion would therefore observe processes below the entry, and a malicious preload could tamper with pin checks, compilation, or launch before trusted-launch starts; the entry needs to scrub these variables with shell builtins before any external command or use a launch mechanism that does not execute an injected env process.

  • [P2] Create pin-check scratch directories before using them — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.5PdRu8g1Fj/work/resolver-trusted-parent/spec.md:1255-1263
    Following this sequence runs the pin checks under the R1 env -i line, which sets TMPDIR=<output>/.run/tmp and HOME=<output>/.run/home, before the next step creates .run, tmp, or home; R10 also says wrong-digest pin failures happen before any .run exists. As written, either the hash/version commands run with nonexistent scratch/home directories or the cleanup assertions are wrong, so the spec needs a pre-pin scratch location inside the output root or the mkdir/cleanup order updated consistently.

  • [P2] Use the checked output fd for later sandbox writes — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.5PdRu8g1Fj/work/resolver-trusted-parent/spec.md:491-500
    With a same-uid race after this descriptor check, the output path can be renamed and replaced with a symlink before the copied supervisor uses the sandbox string to create home, tmp, child.stdout, and child.stderr; the fstat on the old descriptor does not protect those later path-based writes. Since R7 promises one write root and this paragraph calls the parent pass authoritative, the parent should create/open sandbox entries relative to the checked directory fd, such as with mkdirat/openat, or explicitly record the residual.

@yihanzhu

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

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

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

The spec has security-boundary gaps around supported Darwin writes, same-uid replacement of the run directory path, and the entry script's clean-path handoff. These should be resolved before treating the artifact as an accepted basis for implementation.

Full review comments:

  • [P1] Keep xcrun cache inside the output root — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.lKu0VtvywA/work/resolver-trusted-parent/spec.md:498-501
    On supported Darwin runs, this assumption is what keeps R7's one-write-root promise, but xcrun does not keep its lookup cache under TMPDIR; /usr/bin/cc and often /usr/bin/git can create xcrun_db-* under the per-user temp directory or /tmp before the real tool runs. A Darwin invocation following this spec can therefore write outside <output> or fail in restricted temp dirs, so the plan needs to avoid the shim/cache or change the write-root claim.

  • [P1] Protect the .run path from parent-directory renames — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.lKu0VtvywA/work/resolver-trusted-parent/spec.md:520-524
    When another same-uid process can write the 0700 output directory, it can rename <output>/.run itself and replace it with a new .run after the parent checks it; that does not require chmoding the 0500 run dir. Since the runtime later resolves helper, jq, and PATH entries by the <output>/.run/... strings, it can execute the replacement even though the checked descriptor still points at the original directory, so either protect the parent directory/pass descriptors or state this broader residual instead of saying replacement needs chmod.

  • [P2] Reject forged clean-path invocations — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.lKu0VtvywA/work/resolver-trusted-parent/spec.md:320-323
    With this dispatch, a caller can invoke resolve-profile.sh __resolve_profile_clean ... with five args and land on the clean path directly. That skips the empty-environment re-exec for the main body, leaving validation and compile logic in the original bash process whose loader already consumed the caller's environment; make the clean handoff unforgeable or reject marker invocations that did not come from the re-exec.

@yihanzhu

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

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

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

The spec leaves a path re-resolution hole in the parent output handling and explicitly permits Darwin writes outside the output root despite the accepted intent forbidding them. These issues should be resolved before using the spec as the basis for the high-risk implementation.

Full review comments:

  • [P2] Keep supervisor reads fd-relative too — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.OYrtI0rvCZ/work/resolver-trusted-parent/spec.md:760-763
    When a same-uid process can rename or replace the output directory after the child writes, changing only creation to openat is not enough: the copied supervisor later calls empty_regular_file(stderr_path) and stream_file(stdout_path) by path, so the parent can still read a different child.stdout/child.stderr than the descriptors it gave the child. Since this paragraph claims there is no path left to re-resolve, the spec should also require the later stderr checks and stdout streaming to use the checked directory descriptor/open file descriptors.

  • [P2] Reconcile Darwin cache writes with the intent — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.OYrtI0rvCZ/work/resolver-trusted-parent/spec.md:985-996
    For the supported Darwin platforms, this permits /usr/bin/cc and /usr/bin/git to write xcrun_db under the per-user temp directory, but the accepted intent requires “No network, no credentials, no writes outside the caller's own output” (work/resolver-trusted-parent/intent.md:36) and R7 still frames the output path as the only write root. A plan built from this spec can therefore satisfy the spec while violating the G1 constraint; either remove/limit Darwin support, use a toolchain path that does not write outside the output root, or send the changed constraint back through the artifact chain.

@yihanzhu

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 9788015
Reviewed-base: 61336ff
reviewer: gpt-5.5 @ high

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

The spec has two actionable security-boundary issues in the signal forwarding loop and inherited file-descriptor handling. These could lead the implementation plan to preserve a pid-reuse signal hazard and fd leaks into pre-resolver helper processes.

Full review comments:

  • [P2] Avoid forwarding after the parent is already reaped — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.v46SRCA0i5/work/resolver-trusted-parent/spec.md:1513-1516
    If a signal is delivered after wait "$parent_pid" has already reaped the parent but before this branch runs, the trap sets wait_interrupted even though status is the parent's real status. This branch then sends kill to a PID the shell has already reaped and continues, which can lose the parent's status and, after pid reuse, signal an unrelated process — the same hazard the surrounding text says the loop avoids. Treat a non-interrupted wait result as reaped instead of forwarding.

  • [P2] Close inherited descriptors before any helper fork — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.v46SRCA0i5/work/resolver-trusted-parent/spec.md:2801-2802
    When the entry or documented direct-parent path inherits an fd above 2, closing it only in the resolver child before the final execve is too late. The parent forks the SHA tools and jq --version before launching the resolver, so those pre-resolver children can inherit caller fds such as credentials, sockets, or write handles outside the output root, undermining the no-credential/single-write-root boundary. Require closing inherited fds at parent startup before any fork.

…able says running; inherited fds closed at startup)
@yihanzhu

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 67bb175
Reviewed-base: 61336ff
reviewer: gpt-5.5 @ high

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

The spec leaves a signal window that can orphan resolver descendants, and its descriptor-close requirement is placed after earlier processes can already inherit caller fds. These are correctness/security issues in the accepted design before implementation starts.

Full review comments:

  • [P1] Keep pgid live through survivor cleanup — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.WfVD7MWa9k/work/resolver-trusted-parent/spec.md:2840-2842
    When the resolver process exits after spawning another process in its process group, the copied supervisor relies on the post-waitpid process_group_count(child) check to detect survivors and kill that group. This spec clears pgid before restoring the signal mask and before that survivor check runs, so an INT/TERM/HUP delivered during the process-table scan takes the handler's no-runtime branch and can leave the surviving resolver group running while the entry removes .run; keep pgid available, or keep signals blocked, until the survivor check and any group kill are complete.

  • [P2] Close inherited descriptors before the scrub/re-exec — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.WfVD7MWa9k/work/resolver-trusted-parent/spec.md:2165-2165
    For a supported entry invocation with an extra fd open, this close loop runs only after the copied materializer scrub and the /usr/bin/env re-exec path; the copied scrub uses process substitutions in adapters/local-git-materializer/v1/materialize.sh:4-13, so bash children, and then env/the second bash, can inherit fd >2 before it is closed. That leaves a credential/socket/write handle exposed before the point R10 observes, which conflicts with the intent's no-credentials/no-outside-writes boundary (work/resolver-trusted-parent/intent.md:36-44); move the close/CLOEXEC step ahead of any scrub subprocess or re-exec, or narrow the claim and test.

@yihanzhu

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 8c26952
Reviewed-base: e62b90b
reviewer: gpt-5.5 @ high

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

The spec leaves two security-boundary details wrong or underspecified: the signal handler can mutate the caller's stderr flags, and direct parent helper probes can inherit caller-controlled environment. These should be fixed before using the spec as implementation guidance.

Full review comments:

  • [P2] Restore stderr flags after signal diagnostics — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.Myqmc1xps2/work/resolver-trusted-parent/spec.md:3107-3110
    On the signal path where stderr is inherited from the entry/caller, setting O_NONBLOCK in the parent handler and relying on _exit leaves the shared open-file-description flag changed for the entry, because F_SETFL is not per-process. After the parent exits, the entry's EXIT trap still writes entry-signal: to that same stderr and the caller may continue using it; with a pipe this can turn later writes into EAGAIN/short writes and violates the diagnostic contract. Please restore the saved flags before _exit, or avoid mutating stderr's status flags in the handler.

  • [P2] Run parent helper probes under a fixed environment — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.Myqmc1xps2/work/resolver-trusted-parent/spec.md:3917-3923
    When trusted-launch is invoked directly, the SHA-1/SHA-256 tools and jq probe listed here run before the resolver's clean environment is constructed. As written, the spec never requires these execs to use an explicit empty environment, so caller variables such as PERL5OPT/PERL5LIB for Darwin /usr/bin/shasum can execute code or alter the digest probe and undermine the parent-pinned checks despite R7's no-caller-state claim. Please require a fixed envp for these parent helper execs or narrow the direct-parent contract.

@yihanzhu

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 9c1b9e3
Reviewed-base: e62b90b
reviewer: gpt-5.5 @ high

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

The spec leaves two signal-path stderr handling bugs that can break the stated cleanup/exit contracts under realistic pipe and signal timing scenarios.

Full review comments:

  • [P2] Keep signals blocked while toggling stderr flags — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.LStnyDSwgH/work/resolver-trusted-parent/spec.md:5688-5690
    If INT, TERM, or HUP arrives after runtime-pgid: sets O_NONBLOCK on stderr but before the saved flags are restored, the handler will observe the already-nonblocking flags, restore that value, and _exit, so the entry and caller inherit a nonblocking stderr despite the restore guarantee. Keep the three signals blocked through this fcntl/write/restore sequence, or use a diagnostic path that does not mutate the shared open file description.

  • [P2] Avoid blocking in the EXIT-trap diagnostic — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.LStnyDSwgH/work/resolver-trusted-parent/spec.md:5810-5812
    When the entry is interrupted and stderr is a pipe that is full or not being drained, this final printf can block forever; putting it after cleanup prevents a stale .run, but it still means the entry never reaches the promised 128 + signal exit and the caller remains hung. Make the entry-signal: diagnostic best-effort/nonblocking, or omit it when it cannot be written safely.

… block; entry diagnostic omitted where a write could block)
@yihanzhu

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 5a0b87c
Reviewed-base: e62b90b
reviewer: gpt-5.5 @ high

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

The spec would lead to a shell entry that can close Bash's own script descriptor, and it leaves unsafe signal-handler behavior underspecified. These are blocking issues for a high-risk launch boundary artifact.

Full review comments:

  • [P1] Preserve Bash's script descriptor when closing FDs — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.sDuIFqlMmm/work/resolver-trusted-parent/spec.md:2448-2451
    When the entry is invoked via its shebang or as /bin/bash -p <entry>, Bash commonly keeps the script open on an internal descriptor such as fd 255; this /dev/fd/* loop will match and close it because only 0/1/2 are skipped. For a long entry script, that can make Bash fail while reading the rest of the file before it reaches the re-exec or validation path. The close strategy needs to preserve shell-owned descriptors such as the script reader while still closing caller-inherited ones.

  • [P2] Require async-signal-safe handler waits — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.sDuIFqlMmm/work/resolver-trusted-parent/spec.md:3337-3340
    In the INT/TERM/HUP handler path, the spec requires a “brief wait” inside the actual signal handler but does not constrain how it is implemented. If the plan uses nanosleep, usleep, snprintf, or other non-async-signal-safe helpers there, the parent can deadlock or hit undefined behavior exactly during signal cleanup; the spec should state an async-signal-safe mechanism or move that work out of the handler.

…descriptor; handler calls only async-signal-safe functions)
@yihanzhu

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 1a4bb6d
Reviewed-base: e62b90b
reviewer: gpt-5.5 @ high

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

The spec leaves a security-boundary gap in its descriptor-closing design: a caller-supplied writable descriptor to the entry script can survive into child processes. This contradicts the stated no inherited write handles/no writes outside output-root behavior.

Review comment:

  • [P2] Do not assume skipped script descriptors are read-only — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.zH8t7qnpIg/work/resolver-trusted-parent/spec.md:2640-2643
    When the caller already has resolve-profile.sh open for writing on another fd, the -ef /dev/fd/3 skip preserves that descriptor and passes it to the re-exec and later children, even though this paragraph classifies the survivor as read-only and not a write handle outside the output root. That leaves an inherited writable fd outside the allowed write root; the close rule needs to distinguish bash's script fd from caller-supplied same-file fds or otherwise prevent those fds from reaching children.

… relocates its own input; ulimit headroom precondition)
@yihanzhu

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 2388ec6
Reviewed-base: e62b90b
reviewer: gpt-5.5 @ high

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

The spec's signal-handling requirements are internally inconsistent and leave a race where the entry may fail to forward or correctly report a signal after the parent starts.

Full review comments:

  • [P2] Set the wait flag in the signal traps — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.UR5CdmlrlW/work/resolver-trusted-parent/spec.md:1792-1794
    When a signal arrives while the entry is in wait "$parent_pid", traps that do only these assignments leave wait_interrupted empty; the loop below then treats wait's 128+signal return as the parent's real exit, breaks, and lets the EXIT trap remove .run while the parent/resolver may still be alive. Later prose says the traps also set wait_interrupted=1, so this normative “and nothing else” block needs to include that flag update.

  • [P2] Preserve pre-wait signals before clearing the flag — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.UR5CdmlrlW/work/resolver-trusted-parent/spec.md:2038-2039
    When INT/TERM/HUP arrives after parent_pid is set but before or just after the loop begins, the trap can set entry_signal/wait_interrupted before any wait has been interrupted. This loop then either clears the flag before blocking, or carries a stale flag into a wait that returns normally, so the parent is not reliably forwarded the signal and the entry can return 127 or wait for normal completion rather than 128+signal; handle an already-recorded signal before clearing/entering wait.

@yihanzhu

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 83a581c
Reviewed-base: e62b90b
reviewer: gpt-5.5 @ high

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

The spec contains two actionable correctness issues: one can lead the planned signal handler to signal a reaped/reused pid, and one makes a required regression test ineffective. These should be fixed before treating the artifact as accepted.

Full review comments:

  • [P2] Skip SIGKILL after the handler already reaped the target — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.jluVUlWlBX/work/resolver-trusted-parent/spec.md:4033-4035
    When the bounded waitpid(..., WNOHANG) succeeds during the grace wait, this instruction still sends SIGKILL and performs a final reap "whichever way the loop left". In the pre-resolver-helper branch, that means pre_child has already been reaped, so kill(pre_child, SIGKILL) can target a reused pid and reopens the stale-pid/stranger-kill class this signal design is trying to close; track whether the target was reaped and avoid signalling a reaped single-pid target.

  • [P2] Restore blocking mode before the full-pipe test run — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.jluVUlWlBX/work/resolver-trusted-parent/spec.md:6581-6584
    In this full-pipe case, setting O_NONBLOCK on a copy of the pipe write end leaves the shared open file description non-blocking for the entry unless the filler restores the original flags before launch. With stderr already non-blocking, an unconditional bash printf to the full pipe returns EAGAIN instead of hanging, so the test can pass even if the entry fails to omit the diagnostic.

…e pid; full-pipe test fills with a blocking writer)
@yihanzhu

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 794611c
Reviewed-base: e62b90b
reviewer: gpt-5.5 @ high

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

The spec is hash-linked and risk-classified, but it omits the required exact review_size record for this oversized artifact PR. That leaves the manual plan-gate compliance record incomplete.

Review comment:

  • [P2] Record this spec's review_size value — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.5lkVIhDbYR/work/resolver-trusted-parent/spec.md:803-803
    This waiver describes a size exception for the artifact PR, but it never records the exact review_size: accepted-exception value for this spec; the only exact review_size token in the file is explicitly for the future implementation PR. Since the plan gate requires review_size: standard|accepted-exception to be recorded with the one-concern range (work/README.md:71-73), future reviewers/checks can treat this 7,878-line spec PR as missing its own review-size record despite the prose waiver.

@yihanzhu

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: fce5309
Reviewed-base: e62b90b
reviewer: gpt-5.5 @ high

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

The spec leaves a direct-parent path able to execute an untrusted awk next to a valid jq, and its fd cleanup design can leak high-numbered caller descriptors to helper children. These are security-boundary issues that should be corrected before accepting the patch.

Full review comments:

  • [P1] Require bound jq/awk to stay in .run — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.fX0lobu0f3/work/resolver-trusted-parent/spec.md:4912-4920
    With direct trusted-launch invocations, checking only the helper and parent binary leaves the bound jq directory untrusted: the parent still sets PATH=<dir of the bound jq>, and the runtime derives and executes $tool_root/awk after only [ -x ] && [ ! -L ] (scripts/lib/profile-resolution.sh:673-676). A caller can pass a valid pinned jq from another directory containing their own executable awk, so the resolver can execute caller-controlled code despite the parent checks; bind jq/awk to the checked .run directory or validate them alongside the helper.

  • [P2] Do not cap fd cleanup at current soft limit — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.fX0lobu0f3/work/resolver-trusted-parent/spec.md:4724-4729
    If a direct caller opens a high-numbered fd and then lowers RLIMIT_NOFILE before execing the parent, that existing fd remains open even though its number is above rlim_cur. Capping the close loop at the smaller current limit leaves that descriptor inherited by the SHA/jq probe children, contradicting the “no caller state reaches the parent's children” claim; enumerate open fds or close across a range that covers already-open descriptors instead of using the current soft limit as a ceiling.

  • [P3] Document omitted entry-signal lines — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.fX0lobu0f3/work/resolver-trusted-parent/spec.md:5474-5477
    For callers whose stderr is a pipe, FIFO, or socket, R1 says the entry deliberately omits its entry-signal: line to avoid blocking, but this documentation requirement promises an interrupted launch prints that line beside the parent line. An implementation following R1 will make the docs false for piped stderr; mention the same best-effort/omission condition here too.

… verified by the parent; /dev/fd enumeration; docs name the omission rule)
@yihanzhu

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 7bfc9b6
Reviewed-base: e62b90b
reviewer: gpt-5.5 @ high

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

The spec currently authorizes a direct parent path that does not preserve the pinned dependency boundary, and it also specifies a signal diagnostic path that can still hang on blocking terminal/character stderr. These are blocking correctness issues for a high-risk launch-boundary spec.

Full review comments:

  • [P1] Do not support direct launches without entry pins — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.VgjueIZWFB/work/resolver-trusted-parent/spec.md:4812-4813
    When trusted-launch is invoked directly, which this spec says the operator and R10 do, it bypasses the entry's source and module pin checks: the direct caller gets only files 1-3 pinned, and the helper check is only owner/mode. A caller-owned 0500 helper or edited jq module can therefore pass these parent checks and then be executed/evaluated by the runtime, violating the pinned-dependency boundary for exactly the direct path named here; either make direct parent invocation test-only/unsupported or require equivalent pins before launch.

  • [P2] Do not treat TTY stderr as nonblocking — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.VgjueIZWFB/work/resolver-trusted-parent/spec.md:2905-2906
    When stderr is a TTY or other character device whose output is flow-controlled or whose pty master is not being read, this branch still runs bash printf, but that write can block. That breaks the signal-path guarantee that the entry always reaches cleanup and exits with 128 + signal; restrict the diagnostic to actually safe targets such as regular files, or use a nonblocking writer.

…parent pins all eight runtime files; entry diagnostic only to regular files)
@yihanzhu

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: d362560
Reviewed-base: e62b90b
reviewer: gpt-5.5 @ high

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

The spec contains a security-boundary overclaim that conflicts with its own later description of a pre-close fork inheriting caller descriptors.

Review comment:

  • [P2] Correct the descriptor-close guarantee — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.bIMhN5Jwc8/work/resolver-trusted-parent/spec.md:5547-5548
    In a supported entry invocation where the caller has fd 7 open to a credential/socket/write handle, this statement overclaims the boundary: R1 specifies nofile=$(ulimit -n) and says bash 3.2 forks one child before the close loop (work/resolver-trusted-parent/spec.md:3414-3420), so that child briefly inherits fd 7. R7 should carry that one-child residual or the precondition needs a fork-free shape; otherwise a plan can satisfy R1 while falsely claiming every child starts after descriptor cleanup.

@yihanzhu

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: a558ffe
Reviewed-base: e62b90b
reviewer: gpt-5.5 @ high

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

The added spec mandates a descriptor-closing sequence that can silently fail under a plausible inherited-fd state, undermining a stated security boundary. That should be corrected before accepting the artifact.

Review comment:

  • [P1] Close inherited fds before capping the fd table — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.jisCZKgUvU/work/resolver-trusted-parent/spec.md:3361-3362
    This sequence fails when the caller already occupies every descriptor below the normalized soft limit, such as fds 3-255 before the 256 cap: after lowering the limit, bash has no free fd to open /dev/fd, so the glob can expand only to the literal * and the loop closes nothing. That preserves exactly the credential/socket/write descriptors this requirement says must be removed before later child processes, breaking R7's one-write-root and no-leaked-descriptor claims.

@yihanzhu

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: ec23df1
Reviewed-base: e62b90b
reviewer: gpt-5.5 @ high

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

The added high-risk spec contains internal contradictions in security-relevant requirements. These should be corrected before it is used to drive implementation and tests.

Full review comments:

  • [P2] Update the parent fork count for all hash children — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.tOr5u27vED/work/resolver-trusted-parent/spec.md:4679-4683
    When a signal arrives during one of the five new module-hash forks, this rule can be read as protecting only the old three blob-id children, even though R5 now requires eight parent-pinned SHA-1 checks. Please update this every-fork mask rule to match the widened parent-pinned set so the implementation does not leave the module hash children outside the publish/reap protection.

  • [P2] Update stale soft-limit invariant for the 1024 rung — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.tOr5u27vED/work/resolver-trusted-parent/spec.md:3881-3884
    On callers whose hard limit allows the new 1024 rung, the entry should run downstream commands with a soft limit of 1024, but this normative paragraph still says the postcondition is only 256 or 64. That contradiction can lead the plan or tests to assert the old limit and miss the high-headroom path the preceding section adds.

  • [P2] Keep E_* printing out of the external command allowlist — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.tOr5u27vED/work/resolver-trusted-parent/spec.md:6191-6191
    For early E_RUNTIME paths before the scrub/re-exec, implementing this as /usr/bin/printf would run an external binary before the section’s own “first external command is /usr/bin/env” boundary; implementing those paths with the builtin printf instead makes this exact command allowlist/R10 grep stale. Please separate /usr/bin/printf uses for the blob/shim data writes from refusal-line printing.

…dition and printf allowlist made consistent)
@yihanzhu

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 95cdf5b
Reviewed-base: e62b90b
reviewer: gpt-5.5 @ high

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

The added spec contains signal-handling requirements that would direct the implementation toward incorrect status handling and contradict its own command-substitution rule. These should be corrected before accepting the artifact as the implementation contract.

Full review comments:

  • [P2] Preserve the parent status when wait reaps it — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.QxKBGrQzuB/work/resolver-trusted-parent/spec.md:2709-2714
    When a signal arrives after wait_interrupted='' but before wait "$parent_pid" actually blocks, the trap sets wait_interrupted=1; if the parent then exits normally before another signal interrupts wait, this loop discards the real status even though that wait reaped the parent, and the next iteration can report 127 or a synthetic signal status instead. This contradicts the surrounding R1 text that this window reports the parent's real status, so the implementation/test plan should distinguish a pre-wait recorded signal from a wait that was actually interrupted.

  • [P2] Avoid inline uname substitutions in the platform check — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.QxKBGrQzuB/work/resolver-trusted-parent/spec.md:6710-6711
    This prescribed platform-check shape runs both /usr/bin/uname calls inside the case word, but R1 explicitly says the uname substitutions must assign output, capture status, run checkpoint, and only then refuse. If a terminal signal interrupts one of these commands, an implementation following this shape can fall into the unsupported-platform refusal path instead of the promised signal path, so the spec should show the three-step form here too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant