Repository navigation
gentle-badger-838 - #1063
gentle-badger-838#1063
Conversation
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
e9f22669· Trigger:schedule - Thinking:
90s wall
gh pr diff 1063 --repo gunb-ai/gunbc failed to reach api.github.com and no PR diff source of truth was available.
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
83222add· Trigger:schedule - Thinking:
105s wall
gh pr diff 1063 --repo gunb-ai/gunbc failed to resolve api.github.com, leaving no permitted PR diff source of truth.
|
Re: scheduled api-review (2026-04-27T19:41:35Z) — “Could not complete the review…” Verified: this is not a code finding. The automation failed before it could inspect the diff because No fix commit applies for that message alone. If a later run produces concrete findings on the tree, we will address those in commits or reply in-thread with rationale. Marking the PR ready so scheduled reviews/CI can run when GitHub is reachable. |
|
Review metadata
Verdict: APPROVEThe diff only adds the Findings: None. Nothing here breaches fail-closed behavior on the child paths the diff implements (NUL argv writes Exploratory (optional): If a later change replaces the in-runner |
|
Dashboard feedback triage (gentle-badger-838)
|
|
Re: “Dashboard feedback triage” @ 2026-04-27T21:08:23Z That post is a disposition record for earlier threads (failed diff fetch ×2 + approve-with-no-findings), not a new review finding. Re-verified on current |
|
Re: disposition follow-up @ 2026-04-27T21:13:10Z Verified against No fix commit. Treat this chain as terminal unless a review references a concrete line/hunk or CI failure — otherwise each “meta-closure” reply becomes a new dashboard item with zero additional signal. |
|
Re: @ 2026-04-27T21:17:59Z — That text is a self-quote of the prior PR comment (terminal / no new claim), not a reviewer finding. Re-checked |
|
Re @ 2026-04-27T21:22:57Z: Same non-finding as the prior messages (quoted chain: self-quote → self-quote). On |
|
[GUNBC-PR1063-TERMINAL] @ 2026-04-27T21:28:04Z is another dashboard echo of the prior PR comment (no file/CI/hunk reference). Re-verified |
|
Review metadata
Findings:
Verdict: REQUEST_CHANGES. The focused command |
|
Re: api-review @ Finding is valid. After Fix (landed): In Commit: Please re-run review on head — |
|
Re @ 2026-04-27T21:58:39Z: That text is the prior resolution for the |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
b488668c· Trigger:schedule - Thinking:
59s wall
gh pr diff 1063 --repo gunb-ai/gunbc failed to reach GitHub, and the unauthenticated retry required a valid GitHub token, leaving no permitted PR diff source of truth.
|
Review metadata
APPROVE — small, Linux-only bootstrap helper with a clear fd3 sentinel protocol ( Nothing in the diff violates INVARIANTS, modeling-discipline, CODING, or TESTING — this is implementation-layer Rust, fail-closed on every error edge, with hermetic behavior-driven tests. No findings. Exploratory observations (non-blocking):
|
|
Re: scheduled api-review @ 2026-04-27T22:02:08Z ( Not a code finding. The job could not fetch a PR diff: GitHub unreachable / unauthenticated retry lacked a token, so there was no diff to review against No fix commit — nothing in the repo addresses network or Re-verified current head ( |
…undant libc dev-dep Addresses api-review exploratory notes on PR #1063: - Read errno via __errno_location immediately after execvp returns, before writing the f sentinel, so a failing write cannot clobber ENOENT vs other. - Remove duplicate target linux libc dev-dependency; integration tests inherit the package dep. Receipt: cargo test -p execute-command-bootstrap; clippy -D warnings. Made-with: Cursor
|
Re: api-review @ Verified: The approval matches current behavior (fd3 protocol, CLOEXEC-before- Addressed in
“+1 more queued”: Only this review block was attached; paste the other item if it still needs triage. |
|
Review metadata
Verdict: APPROVE — small, self-contained Linux-only bootstrap crate. The design (snapshot errno after |
|
Re @ 2026-04-27T22:19:35Z: This is the resolution note for the Re-verified on current head “+2 more queued”: Only this quoted block was attached — paste the other queued items if they still need triage. |
|
Re @ 2026-04-27T22:22:27Z: Third-generation echo of the 22:02:08Z infra failure (“no Re-verified Routing: Treat PR comments that only re-quote prior “infra triage / echo” replies as non-queue unless new SHA + diff-backed findings appear. “+2 more queued”: Only this block was attached — paste the other queued items if needed. |
|
Re: api-review @ Verified against current PR head No fix commit — review contains no actionable findings. “+2 more queued”: Only this review block was attached; paste the other queued items if they still need triage. |
|
Review metadata
APPROVE — narrow, well-scoped new crate. Linux-gated, libc-only, with documented sentinel protocol and hermetic tests covering both Minor exploratory note (non-blocking): the helper exits 122/123/124 for distinct internal failure modes, but only |
|
Re @ 2026-04-27T22:26:57Z: Echo of the 22:19:35Z resolution note (itself summarizing Re-checked Routing: Do not re-queue nested quotes of session “resolution / echo” PR comments as agent work. “+3 more queued”: Only this block was attached — paste the remaining queued items if needed. |
|
Re @ 2026-04-27T22:31:51Z: Fourth-generation echo of the 22:02:08Z infra triage (quoted body is the 22:22:27Z PR reply verbatim). Adds no new SHA, diff, or CI signal vs that reply. Verified Ops: Stop delivering nested self-quotes to agents; filter on “Third-generation echo” / “infra triage” chains. “+3 more queued”: Only this block was attached — paste the rest if needed. |
|
Re @ 2026-04-27T22:37:09Z: This body is a self-quote of the prior PR reply that already closed the Re-verified Ops: Dedupe dashboard items whose text is only a quoted session PR comment. “+3 more queued”: Only this block was attached — paste the rest if needed. |
|
Review metadata
Findings:
Verdict: REQUEST_CHANGES. The helper implementation itself looks narrowly scoped, but the repo rubric explicitly gates new |
api-review (cursor/composer-2 sha c95237a) flagged that the pre_exec comment still claimed b"sex" maps to NamespaceSetupFailed → fallback, which the bec8bda P2(d) fix overturned. Spell out the two-layer rule explicitly (pre-e patterns fail-closed; post-se patterns classify as exec'd, no fallback) and cite where the remaining sef-spoof gap is tracked (#1063 / #856 helper-binary substrate). Pure docs change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Re: api-review @ Verified on Exploratory (exit 122/123/124 vs wire): Agreed — fd3 alone does not encode every sub-case; a parent that needs to subtype bootstrap-internal failures should combine the byte prefix with No fix commit — no blocking findings. “+4 more queued”: Only this review block was attached; paste the rest if needed. |
1. Story of the diffThis PR adds a new workspace crate, The load-bearing mechanism is the 2. Invariant categories
3. VerdictREQUEST_CHANGES. The implementation shape is aligned with the host-process boundary discipline, but the added test suite does not actually cement the PR’s central |
… + bounded read Replace the true-only success test: EOF after true can happen without FD_CLOEXEC on fd 3. Spawn /usr/bin/sleep 60 and require full pipe read (se + EOF) within 5s; without CLOEXEC the write end would stay open until sleep exits and the read would stall. Receipt: cargo test -p execute-command-bootstrap; clippy -D warnings. Made-with: Cursor
|
Dashboard triage (post-merge): @ 2026-04-27T22:42:01Z + @ 2026-04-27T22:44:22Z Both items are nested self-quotes of earlier session PR replies (22:26:57Z resolution echo; 22:31:51Z infra-triage echo chain). They introduce no new SHA, diff, reviewer hunk, or CI failure — not actionable code review. This PR is already MERGED ( Ops: Stop enqueueing merged-PR “resolution / infra echo” self-quotes to agents; dedupe by merged state + quoted-body match. “+5 / +4 more queued”: Not present in this payload — paste any remaining items on open PRs/issues if they still need work. |
api-review (openai-pro/gpt-5-5-pro sha 9fea084) flagged that `read_sentinel` collapsed real read errors (e.g. EINTR) into empty/short sentinels, which would route to NamespaceSetupFailed → direct fallback — implicit re-execution if the unshare child already ran the logical command. Fix: typed Result<Vec<u8>, io::Error>: - EINTR retried in-loop (idempotent on a closed-write-end pipe). - Real read errors propagated up. - Caller in run_linux_unshare_then_direct routes Err → WaitFailed::Io (NOT direct fallback) so a sentinel-read failure cannot trigger implicit re-execution. Also adds an `#[ignore]`d behavior receipt `unshare_logical_command_writing_f_to_fd3_is_not_misclassified_after_helper_lands` that documents the structural sef-spoof gap escalated to #856 / wired in #1063. The test asserts the post-helper-binary expectation (sentinel = b\"se\", outcome = Matched); removing the #[ignore] becomes the load-bearing receipt that the helper-binary AC ('sef-spoof structurally unreachable', per issuecomment-4331097029) is met, not just 'helper binary exists'. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…command_bootstrap (#1063) Director-confirmed STOP-condition closure: replace the pure-sh UNSHARE_LOGICAL_BOOTSTRAP_SH bootstrap with the gunbc_execute_command_bootstrap helper binary from src/v3/execute_command_bootstrap (PR #1063 / issue #856). Structural P2(c) closure: the helper sets FD_CLOEXEC on fd 3 immediately before execvp(3), so a successful logical exec atomically closes fd 3 in the new image — the user command CANNOT inherit a writable sentinel fd. The b"sef"-spoof discriminator gap that was unreachable in pure POSIX sh is now structurally impossible: only the helper itself can write `f` on the sentinel. Wiring: - New `locate_unshare_bootstrap_helper()`: env override (`GUNBC_EXECUTE_COMMAND_BOOTSTRAP`) → current_exe walkup → typed SetupFailed if not found. Configuration errors surface as typed outcomes; no silent degradation to a spoofable bootstrap. - `build_execute_command_unshare(helper, command, args, ready)`: invokes `unshare -c -f -p -- <helper> <command> <args...>`. pre_exec dup2s ready-pipe write end to fd 3 WITHOUT FD_CLOEXEC (the helper handles CLOEXEC at the right moment). - Deleted `UNSHARE_LOGICAL_BOOTSTRAP_SH` const and its sh-bootstrap protocol comments (the architecture is now documented as the helper protocol). - Test `unshare_logical_command_writing_f_to_fd3_is_not_misclassified_after_helper_lands` unignored: previously #[ignore]d during the pure-sh era, now a load-bearing receipt that the helper's CLOEXEC-before-execvp ordering actually closes the spoof window (issuecomment-4331097029 AC). - Test comment refresh: post-helper-binary, `unshare_post_exec_fd3_write_does_not_trigger_implicit_rerun` is defense-in-depth (the user's printf gets EBADF; sentinel reads exactly b"se"; outcome is Matched). Belt-and-suspenders structural + classifier protection. Test result: 38/38 pass (was 37 + 1 ignored). Clippy clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…iring (Worker 4) (#1049) * WIP: gentle-deer-205 * chore: apply cargo fmt * fix(test_runner): match execvp path-vs-PATH semantics in unshare bootstrap probe Reviewer flagged that `command -v -- "$0"` conflates sh PATH lookup with execvp's resolution. Split the probe: paths containing `/` use `[ -x "$0" ]` direct file test (execvp does not consult PATH for these); bare names use `command -v` for PATH search. Adds three Linux receipts for absolute-path, bare-name PATH, and missing-relative-path cases. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * WIP: gentle-deer-205 * chore: apply cargo fmt * WIP: gentle-deer-205 * fix(test_runner): satisfy clippy::doc_overindented_list_items Workspace `cargo clippy --all -- -D warnings` (CI Lint gate) flagged the bootstrap protocol doc comment for nested list items at column 5 with 6- column continuations. Reflow the numbered list and the sentinel-byte list to use single-space `* `/`- ` indentation with continuation lines aligned to the bullet text. Pure docs change; no code semantics affected. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(test_runner): document read_sentinel caller ordering contract api-review (claude-opus-4-7) suggested a comment so a future change moving the sentinel read before child.wait() can't deadlock the parent. Spell out the two preconditions (parent write-end closed, child reaped) explicitly. Pure docs change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * WIP: gentle-deer-205 * fix(test_runner): drop FD_CLOEXEC re-arm on sentinel fd 3 (silent unshare bypass) codex review on PR #1049 caught that pre_exec set FD_CLOEXEC on fd 3 BEFORE the parent's exec("unshare"). The kernel atomically closes CLOEXEC fds on exec, so fd 3 was being closed before unshare(1) ever ran. The bootstrap sh inherited no fd 3, every printf >&3 silently failed, every sentinel came back empty, and every run was classified as NamespaceSetupFailed — forcing direct fallback and silently disabling the unshare PID-namespace containment path. Fix: drop the fcntl(F_SETFD, FD_CLOEXEC) line. fd 3 inherits across exec("unshare") → unshare's exec(sh) without CLOEXEC; bootstrap sh writes sentinel bytes normally. The trade-off is that fd 3 inherits to the user process on a successful logical exec — the exact-byte sentinel classifier fail-closes any unexpected pattern, so a stray user write to fd 3 surfaces as fall-back to direct, never a misclassified logical exit. The EXIT-trap protocol still works without CLOEXEC: - Successful exec: sh process replaced by user → trap (sh's, not user's) is gone → no `f` written → sentinel stays "se". - Failed exec: sh resumes, EXIT trap fires before sh exits, writes `f` → sentinel "sef". Adds unshare_path_actually_engages_user_runs_as_pid_1_in_new_namespace regression: probes that this host can create a PID namespace via unshare -c -f -p, and if so asserts that our runner reaches a Matched outcome on sh -c '[ "$$" = "1" ]'. A future regression that breaks the sentinel pipe or otherwise routes through direct fallback would be caught instead of silently passing. Restricted hosts skip via the probe. Doc comments updated to remove the (now incorrect) CLOEXEC narrative. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: apply cargo fmt * WIP: gentle-deer-205 * test: trim dead message-matching disjuncts in missing-binary triage test cursor api-review (composer-2) noted that 'failed to start' and 'post-start fallback' no longer appear in production output after the typed-outcome refactor; they're stale triage surface. Replace with a documented enumeration of the actual production phrasings emitted by ExecuteCommandHostOutcome:: into_claim_result for this seam (SpawnFailed/SetupFailed/Mismatch). Pure test cleanup; no production-code change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(test_runner): comment UNSHARE_READY_PIPE_MAX=4 sizing rationale api-review (claude-opus-4-7, sha 793a57e) noted the cap is one byte beyond the longest valid sentinel (b"sef" = 3) so stray patterns like b"sefx" can be observed and rejected by the exact-byte classifier. Spell that out in a const-level doc comment. Pure docs change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(test_runner): close P2(d) post-se stray-byte re-run gap api-review (codex sha 7297b04, REQUEST_CHANGES) caught that the catch-all `_ => NamespaceSetupFailed` in `unshare_bootstrap_stage_from_sentinel` caused implicit re-execution: once the bootstrap reached `e` (committed to `exec(2)`), the user command DID run; if the user then wrote stray bytes to the inherited fd 3 (e.g. sentinel `b"sex"`), the catch-all routed to `NamespaceSetupFailed`, which `run_linux_unshare_then_direct` treats as authority to run the command again via direct fallback — violating P2(d) "No implicit re-execution". Fix: any sentinel whose prefix is `b"se"` classifies as `LogicalCommandExeced` (or, for exact `b"sef"`, `LogicalExecFailed`). The `_` arm only fires for patterns that did NOT reach `e`, where the direct fallback is the *first* logical run, not a second. Add regressions: - `post_se_stray_bytes_classify_as_logical_command_execed_no_rerun` for `b"sex"`, `b"sefx"`, `b"se\x00"`, `b"se\xff\xff"`. - Renames the unexpected-pattern test to `pre_se_unexpected_patterns_fail_closed_to_setup_failed` and trims it to the genuinely-pre-`e` cases (`b"x"`, `b"es"`). Updates module-level protocol doc and the `UNSHARE_READY_PIPE_MAX = 4` comment to reflect the new post-`se` classification rule. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * WIP: gentle-deer-205 * chore: apply cargo fmt * docs(test_runner): refresh pre_exec comment to match bec8bda classifier api-review (cursor/composer-2 sha c95237a) flagged that the pre_exec comment still claimed b"sex" maps to NamespaceSetupFailed → fallback, which the bec8bda P2(d) fix overturned. Spell out the two-layer rule explicitly (pre-e patterns fail-closed; post-se patterns classify as exec'd, no fallback) and cite where the remaining sef-spoof gap is tracked (#1063 / #856 helper-binary substrate). Pure docs change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(test_runner): typed Result on read_sentinel + sef-spoof receipt api-review (openai-pro/gpt-5-5-pro sha 9fea084) flagged that `read_sentinel` collapsed real read errors (e.g. EINTR) into empty/short sentinels, which would route to NamespaceSetupFailed → direct fallback — implicit re-execution if the unshare child already ran the logical command. Fix: typed Result<Vec<u8>, io::Error>: - EINTR retried in-loop (idempotent on a closed-write-end pipe). - Real read errors propagated up. - Caller in run_linux_unshare_then_direct routes Err → WaitFailed::Io (NOT direct fallback) so a sentinel-read failure cannot trigger implicit re-execution. Also adds an `#[ignore]`d behavior receipt `unshare_logical_command_writing_f_to_fd3_is_not_misclassified_after_helper_lands` that documents the structural sef-spoof gap escalated to #856 / wired in #1063. The test asserts the post-helper-binary expectation (sentinel = b\"se\", outcome = Matched); removing the #[ignore] becomes the load-bearing receipt that the helper-binary AC ('sef-spoof structurally unreachable', per issuecomment-4331097029) is met, not just 'helper binary exists'. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: apply cargo fmt * WIP: gentle-deer-205 * chore: apply cargo fmt * feat(test_runner): wire ExecuteCommand unshare path to gunbc_execute_command_bootstrap (#1063) Director-confirmed STOP-condition closure: replace the pure-sh UNSHARE_LOGICAL_BOOTSTRAP_SH bootstrap with the gunbc_execute_command_bootstrap helper binary from src/v3/execute_command_bootstrap (PR #1063 / issue #856). Structural P2(c) closure: the helper sets FD_CLOEXEC on fd 3 immediately before execvp(3), so a successful logical exec atomically closes fd 3 in the new image — the user command CANNOT inherit a writable sentinel fd. The b"sef"-spoof discriminator gap that was unreachable in pure POSIX sh is now structurally impossible: only the helper itself can write `f` on the sentinel. Wiring: - New `locate_unshare_bootstrap_helper()`: env override (`GUNBC_EXECUTE_COMMAND_BOOTSTRAP`) → current_exe walkup → typed SetupFailed if not found. Configuration errors surface as typed outcomes; no silent degradation to a spoofable bootstrap. - `build_execute_command_unshare(helper, command, args, ready)`: invokes `unshare -c -f -p -- <helper> <command> <args...>`. pre_exec dup2s ready-pipe write end to fd 3 WITHOUT FD_CLOEXEC (the helper handles CLOEXEC at the right moment). - Deleted `UNSHARE_LOGICAL_BOOTSTRAP_SH` const and its sh-bootstrap protocol comments (the architecture is now documented as the helper protocol). - Test `unshare_logical_command_writing_f_to_fd3_is_not_misclassified_after_helper_lands` unignored: previously #[ignore]d during the pure-sh era, now a load-bearing receipt that the helper's CLOEXEC-before-execvp ordering actually closes the spoof window (issuecomment-4331097029 AC). - Test comment refresh: post-helper-binary, `unshare_post_exec_fd3_write_does_not_trigger_implicit_rerun` is defense-in-depth (the user's printf gets EBADF; sentinel reads exactly b"se"; outcome is Matched). Belt-and-suspenders structural + classifier protection. Test result: 38/38 pass (was 37 + 1 ignored). Clippy clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * WIP: gentle-deer-205 * chore: apply cargo fmt * ci: build execute-command-bootstrap helper before v3 tests; refresh stale doc refs CI v3 job was failing because `cargo test -p v3-compiler` doesn't pull the bin-only `execute-command-bootstrap` crate into its build graph (stable Rust can't list bin-only crates as `[dependencies]`), so `test_runner.rs::locate_unshare_bootstrap_helper` failed every spawn with `SetupFailed { helper binary not found }`. Fix: - Add an explicit `cargo build -p execute-command-bootstrap` step to the v3 CI job before any `cargo test -p v3-compiler` invocation. Test runner walks from `current_exe()` to find `target/debug/gunbc_execute_command_bootstrap`. - Refresh stale comments in `test_runner.rs` that still referenced the deleted `UNSHARE_LOGICAL_BOOTSTRAP_SH` const / "bootstrap sh" / "wrapper piped stderr" — replaced with helper-binary terminology (api-review cursor/composer-2 sha 160b574, all NON-BLOCKING). - Convert orphan doc-comment block (left dangling after the const deletion) to `//` regular comments. NOT taken (deadlock): - An earlier attempt added a recursive `cargo build -p execute-command-bootstrap` to `build.rs`. Cargo's per-package lock deadlocks with the parent `cargo build -p v3-compiler` invocation. The CI-side prereq step is the reliable solution. Local verification: - `cargo build -p execute-command-bootstrap && cargo test -p v3-compiler --lib test_runner` → 38/38 pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: apply cargo fmt * fix(test_runner): split HelperBinaryMissing from NamespaceSetupAndDirectSpawnFailed api-review (cursor/composer-2 sha 7dd7825, REQUEST_CHANGES) caught that the helper-not-found path returned `SetupFailed { NamespaceSetupAndDirectSpawnFailed }` which lies — neither namespace setup nor direct spawn was attempted. The rendered diagnostic ("namespace setup failed and direct fallback spawn also failed") doubly mis-described what happened. Add a distinct `SetupFailReason::HelperBinaryMissing { reason: String }` variant + matching diagnostic ("gunbc_execute_command_bootstrap helper not found: ..."), and route the `locate_unshare_bootstrap_helper` Err path through it. Configuration / build-graph errors now surface as a typed outcome that names the actual problem. Also addressed the cursor reviewer's NON-BLOCKING doc finding (variant docs in `UnshareBootstrapStage` still said "sh ran" / "sh exec'd" — now "helper ran" / "helper execvp'd"). Integration test triage (`test_runner_execute_command_missing_binary_is_distinguishable_fail`) now also accepts the new "helper not found" phrasing. Local: 38/38 lib tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(test_runner): refresh unshare_path_drains_piped_stderr_... narrative api-review (cursor/composer-2 sha 25b96a9 NON-BLOCKING) noted the test comment still described the deleted wrapper-piped-stderr/drain machinery. Rewrite the doc to describe live-state behavior: wrapper stderr is Stdio::null(), helper inherits stdio normally, parent reads no pipe — so the test is now a volume-sanity receipt (a logical >&2 burst completes within the wall bound) rather than a guard against the pipe-stall regression that pre-helper-binary plumbing could trip. Pure docs change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * WIP: gentle-deer-205 * fix(test_runner): validate helper path is a regular executable file (P2(c)) api-review (codex/codex-default sha 143b7da, BLOCKING) caught that `locate_unshare_bootstrap_helper` accepted any existing override path — including a directory or non-executable file. With such a path, unshare(1) would fail to exec the candidate, the helper would never write its sentinel, the parent would classify the empty result as NamespaceSetupFailed, and the direct fallback would run the logical command, silently converting helper misconfiguration into a possible Matched logical exit (P2(c) collapse). Add `is_regular_executable_file` (std::fs::metadata + Unix permissions mode check for any-of-owner/group/other execute bits) and route every candidate path through it: env override, current_exe walkup direct sibling, and target/<profile>/ walkup-from-deps. The override-pointing- at-non-executable path now surfaces as a typed `SetupFailReason::HelperBinaryMissing` rather than a silent direct fallback. Adds `helper_path_validation_tests` module with 4 regressions: directory rejected, non-executable file rejected, missing file rejected, /bin/sh accepted. Tests cover the validator unit-level so future drift is caught even when CI runs in environments where the override is unreachable. Local: 42/42 tests pass (was 38; +4). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(test_runner): use access(X_OK) for uid-aware helper-path validation (P2(c)) api-review (codex sha 523776b BLOCKING follow-up) caught that the mode-bits-only check `(mode & 0o111) != 0` accepts files where SOME uid can exec but the calling process cannot — e.g. mode 0o100 owned by root while running as a non-root user. With such a file as the override, unshare → EACCES at exec → empty sentinel → NamespaceSetupFailed → direct fallback = silent helper-misconfig-to-Matched conversion. Replace the mode-bits check with `libc::access(path, X_OK)` — the kernel atomic uid-aware permission check. Reject paths with interior NULs (can't be passed to access(2) at all). Add `non_executable_to_calling_uid_is_rejected` regression: drop ALL execute bits (mode 0o400) on an owned tmp file → access(X_OK) returns EACCES → validator rejects. Local: 43/43 lib tests pass (was 42; +1 uid-aware regression). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(test_runner): explicitly clear FD_CLOEXEC after dup2 to fd 3 (P2(c)) api-review (codex sha c535f4c BLOCKING) caught that POSIX dup2(src,dst) is a no-op when src == dst — so if a low-fd allocation in the child puts our write_fd already AT fd 3, dup2 leaves fd 3 with the O_CLOEXEC flag we set via pipe2(O_CLOEXEC). The kernel would then close fd 3 atomically on the parent's exec("unshare"), the helper would never see the sentinel, every run would classify as NamespaceSetupFailed, and the direct fallback would silently bypass the helper path. Add explicit fcntl(3, F_SETFD, 0) after dup2 to defend against the src == dst no-op corner case. fd 3 now structurally has CLOEXEC cleared before exec into unshare, regardless of how it got allocated. Local: 43/43 lib tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * WIP: gentle-deer-205 --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Review metadata
1. Story of the diffThis PR adds a new workspace crate, The load-bearing mechanism is the 2. Invariant categories
3. VerdictREQUEST_CHANGES. The implementation shape is aligned with the host-process boundary discipline, but the added test suite does not actually cement the PR’s central |
Opened from session-dashboard for session
gentle-badger-838.