spec: resolver-trusted-parent - #276
Conversation
Deploying ystack with
|
| Latest commit: |
8e40159
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3d2ef75a.fabrica-6yx.pages.dev |
| Branch Preview URL: | https://ystack-spec-resolver-trusted.fabrica-6yx.pages.dev |
Codex reviewer (cross-vendor, read-only)Reviewed-head: bd232b1 Posted verbatim by 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:
|
…table helper provenance)
Codex reviewer (cross-vendor, read-only)Reviewed-head: ebbef0f Posted verbatim by 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:
|
…g with stated residual; direct-parent jq test; narrowed read claim)
Codex reviewer (cross-vendor, read-only)Reviewed-head: 5a18ff3 Posted verbatim by 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:
|
…mporaries inside the run dir; cleanup proved after mktemp)
Codex reviewer (cross-vendor, read-only)Reviewed-head: 52e10fd Posted verbatim by 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:
|
…tion on signals; direct-parent coverage)
Codex reviewer (cross-vendor, read-only)Reviewed-head: eb96ef6 Posted verbatim by 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:
|
…sistently; race-free signal test via SIGSTOP)
Codex reviewer (cross-vendor, read-only)Reviewed-head: a69c86e Posted verbatim by 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:
|
… allowlist; reachable overlong case)
Codex reviewer (cross-vendor, read-only)Reviewed-head: a8a9374 Posted verbatim by 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:
|
…ry inside the caller's output root)
Codex reviewer (cross-vendor, read-only)Reviewed-head: b568fc0 Posted verbatim by 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:
|
… vars only via the entry; compile under env -i)
Codex reviewer (cross-vendor, read-only)Reviewed-head: d8da6ec Posted verbatim by 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:
|
… before pins; fd-relative sandbox writes)
Codex reviewer (cross-vendor, read-only)Reviewed-head: 0540b47 Posted verbatim by 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:
|
…dual stated; marker branch re-scrubs)
Codex reviewer (cross-vendor, read-only)Reviewed-head: 5619bfc Posted verbatim by 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:
|
…n shim in the shipped path)
…orded signals forwarded before each wait)
Codex reviewer (cross-vendor, read-only)Reviewed-head: 83a581c Posted verbatim by 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:
|
…e pid; full-pipe test fills with a blocking writer)
Codex reviewer (cross-vendor, read-only)Reviewed-head: 794611c Posted verbatim by 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:
|
Codex reviewer (cross-vendor, read-only)Reviewed-head: fce5309 Posted verbatim by 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:
|
… verified by the parent; /dev/fd enumeration; docs name the omission rule)
Codex reviewer (cross-vendor, read-only)Reviewed-head: 7bfc9b6 Posted verbatim by 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:
|
…parent pins all eight runtime files; entry diagnostic only to regular files)
Codex reviewer (cross-vendor, read-only)Reviewed-head: d362560 Posted verbatim by The spec contains a security-boundary overclaim that conflicts with its own later description of a pre-close fork inheriting caller descriptors. Review comment:
|
…ion; no child before the close loop)
Codex reviewer (cross-vendor, read-only)Reviewed-head: a558ffe Posted verbatim by 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:
|
…es; limit ladder explained)
Codex reviewer (cross-vendor, read-only)Reviewed-head: ec23df1 Posted verbatim by 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:
|
…dition and printf allowlist made consistent)
Codex reviewer (cross-vendor, read-only)Reviewed-head: 95cdf5b Posted verbatim by 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:
|
…wait signal is recorded; three-step platform check)
Codex reviewer (cross-vendor, read-only)Reviewed-head: b7a3f86 Posted verbatim by The spec has two actionable inconsistencies around the supported launch contract and the documented non-privileged refusal behavior. These should be fixed before the artifact is accepted because they would misdirect the implementation and docs for a security-boundary component. Full review comments:
|
… path made absolute; non-p refusal claim narrowed)
Codex reviewer (cross-vendor, read-only)Reviewed-head: 70779e6 Posted verbatim by The added spec contains several contradictions and missing requirements around supported Linux behavior, the parent security boundary, and the stated test coverage. These would likely lead to a broken or under-specified implementation if accepted as-is. Full review comments:
|
|
Full G2 content-review conclusion: no unresolved Important finding. Required CI and operator G2 acceptance remain pending. Reviewed tuple:
This conclusion combines substantive prior review with a fresh base review. I previously read the complete 10,003-line spec, relevant accepted resolver contracts, and the complete copied 702-line launcher. I subsequently reviewed the entire correction diff producing the current 10,025-line blob and its affected surrounding contracts. This turn verified that exact blob is unchanged, read the complete seven-file base delta introduced by CI sharding, and checked its implications for the spec. I did not substitute a bounded review for the full artifact review. Bugs: No unresolved Important finding. The seven earlier finding groups remain resolved. The new base changes CI execution and documentation; it does not change the resolver runtime, library, core contracts, copied launcher, or materializer source on which this review depends. Filename-based suite discovery remains compatible with the future focused test required by the spec. Security: No unresolved Important finding. The entry-only supported launch, direct-parent test boundary, pinning, descriptor and environment handling, signal/cleanup requirements, and explicitly documented residuals remain intact. This PR introduces no runtime implementation, credential access, installation, activation, or construction-mode authority. Compliance: No unresolved Important content finding. Verified:
The supplied API response matches the local head/base, reports PR OPEN with no labels, and shows CI still pending. My independent This is clean full-content review evidence for the exact tuple above, not a completed CI gate, G2 acceptance, or merge authorization. The manager must verify final required CI and unchanged remote head/base before the operator’s G2 decision. |
|
Final independent review: PASS — no unresolved Important findings; required CI is green. Operator G2 acceptance remains pending. Exact reviewed identity:
The complete substantive content review remains valid. It comprised the full original artifact read, complete correction-diff review producing the current 10,025-line blob, relevant resolver/source-contract review, and complete new-base delta review. This final turn refreshed identity and CI evidence; it does not present that narrow refresh as a replacement for the substantive review. All three passes remain clean:
CI run 34763567835 completed successfully on that exact head:
Remote observations are verified against the manager-supplied final API/run records and logs; my earlier direct GitHub API attempt was unavailable. Local identities independently match those records. This is final clean review evidence for the stated head/base. It accepts no implementation, activation, or live qualification and does not replace the operator’s G2 decision. |
|
Operator G2 acceptance and delegated merge The operator directly accepted PR #276 and PR #288 in the current Codex session, reply to question call_TL2Pvpizotz44mnffDKQzCLE/0: “接受这两份文件并授权代合并(推荐)”. The decision permits sequential delegated merges with unchanged artifact contents and renewed CI/independent review after any base change. For #276 this accepts spec blob b81fc2dcbf0358e3c796300b640170425bda511c and high risk at head 8e40159, reviewed base ec40e66. Full independent PASS and all required CI are recorded above. This is G2 acceptance for the separate planning stage, not implementation or live execution authority. |
|
Merge receipt PR #276 was squash-merged under the operator's direct G2 acceptance and delegated merge authorization. Main is cfc8eab, with parent ec40e66. Its full tree 5fec2ab647f3ed58d19ad38440c9816a66a5de15 exactly equals reviewed head 8e40159. The merge used a matching head constraint, required green CI and strict up-to-date protection; no bypass or history rewrite. G2 is accepted. The next stage is a separate high-risk plan, not implementation. |
Tracks #271
Specify the supported trusted parent for resolving a real profile outside the test launcher. The existing resolver behavior stays unchanged. This is a high-risk spec-only PR; implementation requires a separately accepted plan.
The spec binds the accepted DR-2 intent (blob eaa322c405502cc0ca7c453814ca0f005f11b48f). It defines the supported entry, pinned parent/runtime/tool identities, environment and descriptor handling, process supervision, cleanup, refusal behavior and complete tests. Direct parent invocation remains test-only; the operator launches the supported entry.
The current continuation preserves the original branch and inherited edits. Independent full-content review and follow-up review closed the identified issues: Linux awk symlinks, exact run-directory contents, jq hardlink directory binding, descriptor-read boundaries, self-path capture before environment scrubbing, interruption/reaping status, cleanup sequencing and parent refusal coverage. No construction-mode or live-execution authority is claimed.
Validation: intent hash link and spec-only scope verified; git diff --check passes; independent content review found no unresolved Important finding in spec blob b81fc2dcbf0358e3c796300b640170425bda511c. Publication, required CI and fresh exact-head/base review remain necessary before G2 acceptance. No runtime tests are claimed for this documentation-only revision.
The artifact's size exception and implementation size estimate are distinct and stated in the spec. This PR accepts no implementation or reduced testing merely because the artifact is large.