fix: tolerate login-shell process names in the ancestry walks - #3
Merged
Merged
Conversation
On macOS a login shell reports itself through `ps -o comm=` as "-zsh".
Passing that to basename makes it parse "-z -s -h" as option flags and
fail loudly on stderr, so every ancestry walk that reached a login shell
printed "basename: illegal option -- z" - noise where genuine warnings
must stand out, and an argument silently reinterpreted as flags.
Strip the directory with parameter expansion instead of basename in
fm_harness_ancestry_pid, fm_harness_pid_alive, and fm-harness.sh's
detect_own. Parameter expansion also avoids a subprocess on every hop of
a walk that runs up to sixteen times per check. Behavior is otherwise
unchanged: for every value `ps -o comm=` produces, "${comm##*/}" and
`basename --` agree.
Adds a regression case to tests/fm-secondmate-harness.test.sh, the
existing owner of both ancestry walks' process-identity coverage.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughUpdated harness and session-lock ancestry detection to extract executable names with Bash parameter expansion, plus tests for macOS-style login-shell names and controlled environment markers. ChangesHarness ancestry detection
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Test
participant fm-harness.sh
participant fm-session-lock-lib.sh
participant FakePS
Test->>FakePS: Report ancestry including -zsh
fm-harness.sh->>FakePS: Read process names
FakePS-->>fm-harness.sh: Return harness and login-shell names
fm-harness.sh-->>Test: Resolve harness
fm-session-lock-lib.sh->>FakePS: Read ancestry and liveness
FakePS-->>fm-session-lock-lib.sh: Return process information
fm-session-lock-lib.sh-->>Test: Resolve session-lock holder
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Jul 30, 2026
sanis
pushed a commit
that referenced
this pull request
Sep 13, 2026
…kunchenguid#3578) * fix(bin): let verified harness ancestry outrank retained markers (#3) * fix(bin): let a structural harness ancestor outrank a retained marker bin/fm-harness.sh treated a verified environment marker as unconditionally authoritative, so a Codex session started from an environment that had retained CLAUDECODE=1 detected as claude. Session start then emitted Claude's Stop-owned supervision protocol to a Codex primary, and every turn end was blocked for missing Claude recovery. The defect is the precedence boundary, not any one harness. codex, opencode, kimi, and muse publish no identity marker at all, so with markers winning outright any retained CLAUDECODE renamed them; the Cursor-before-Claude ordering was a point patch on the same class of problem, and the launch-time marker clearing only ever covered sessions fm-spawn started. Markers and ancestry are now separate evidence layers that detect_own arbitrates: - no ancestry match, or no marker: the single available layer answers, unchanged; - same harness family: the marker's finer verdict stands, so a launch-selected pi-signed is not flattened to pi by an ancestry walk that can only see the shared launcher name; - different harness with a structural (command-name) ancestor: ancestry wins, because only ancestry proves who owns the process tree; - different harness with only a bare-interpreter script-path match: the marker wins, since a harness-shaped path in some node process's arguments is weaker evidence than a harness publishing its own identity. The correction is symmetric: a retained CURSOR_AGENT no longer renames a claude worker nested under cursor either. Adds fm-harness.sh ancestry [<pid>], ancestry evidence with no marker layer, so a real harness process can be asked what the walk makes of it. tests/fm-harness-precedence.test.sh is the portable regression, built from real renamed processes with no harness installed. Every case drives the two layers apart and asserts each alone as well as the combination, so no case can pass vacuously; it also pins Codex's real two-process install topology, since the fix depends on the native binary being what a tool subprocess meets first. The opt-in drift guard gains the matching live half: each installed harness's real running process must still be identified by the ancestry walk, and it fails naming the harness and version when a release changes that name. Documentation follows the corrected contract in the script header, the harness-adapters detection section, the codex, opencode, kimi, and cursor references, and a dated verification record. * fix(tests): drop the unused argument pass-through in the shim-topology helper bin/fm-lint.sh refused the branch: run_shim declared a `[ancestry]` argument and forwarded "$@", but every call site that varies the environment or passes the ancestry subcommand invokes the shim entry point directly, so the helper is only ever called with no arguments (ShellCheck SC2120/SC2119). Behavior is unchanged: with no arguments "$@" expanded to nothing. * fix(bin): examine the top of the process chain instead of assuming init harness_ancestry stopped as soon as the next pid was 1, on the assumption that pid 1 is always init and can never be a harness. Inside a PID namespace that assumption inverts: the harness itself is pid 1, so the walk never examined the one process that proves who owns the tree, reported no ancestry at all, and handed the verdict straight back to a retained marker. A real Codex session under `codex sandbox`, holding CLAUDECODE=1 and CLAUDE_CODE_ENTRYPOINT=cli, is exactly that shape: it resolved claude and rendered Claude's Stop-owned supervision protocol even with the marker-vs-ancestry precedence boundary in place. The same probe now resolves codex and renders the Codex foreground checkpoint. A host's real pid 1 (init, systemd, launchd) matches no harness name, so examining it costs one ps call and can introduce no false positive; the walk still stops once that top process has been read, and a non-numeric or zero ppid still ends it. tests/fm-harness-precedence.test.sh pins the namespace shape with a fake ps that reports every process as bash with ppid 1 and pid 1 as the harness. The case asserts the marker still answers alone when pid 1 is host-shaped, so it cannot pass vacuously, and it fails against the previous stop condition. * docs(verification): record the real-Codex retained-marker evidence The existing record proved the precedence boundary with the portable regression and recorded each installed harness's process name behind the ancestry walk, but it had no evidence from a real Codex process actually holding a retained Claude marker, which is the failure the boundary exists for. Adds the dated before/after result from codex-cli 0.152.0 under `codex sandbox`, with the exact command and the decisive verdict and rendered protocol on each side, and records the second boundary that shape exposed: the walk must examine the top of the process chain, because inside a PID namespace the harness is pid 1. Refreshes the portable regression's observed output for the case it gained. * no-mistakes(review): blind ancestry in marker-pinned harness tests * no-mistakes(review): blind ancestry in the Pi guard-routing test * no-mistakes(review): classify precedence suite, dedupe ps stub, soften claims * no-mistakes(review): model the spawn-and-wait Codex shim topology * no-mistakes(document): correct stale muse marker-clearing detection claims * no-mistakes: apply CI fixes * fix(bin): examine the top of the chain in the lock and nudge walks too The pid-1 defect corrected in bin/fm-harness.sh survived unchanged in the two other harness-ancestry walks, on the exact topology the branch verified against a real Codex process. bin/fm-session-lock-lib.sh's fm_harness_ancestry_pids stopped as soon as the next pid was 1, so a firstmate whose harness is pid 1 of its own PID namespace could not find that harness at all and did not recognize its own session lock. bin/fm-sessionstart-nudge.sh carried the same stop plus a blanket rejection of a lock pid of 1, so the same session was told to run session start again on every turn. Both walks now compare the top process before stopping, matching the shape used in bin/fm-harness.sh. For the lock walk this is safe because fm_harness_process_matches rejects a host's real pid 1. For the nudge, `kill -0` still gates the lock pid, and on a host an unprivileged `kill -0 1` fails, so a lock file that wrongly names pid 1 leaves the hook silent rather than acting on init. Each walk gains one regression case. The lock case drives a deterministic process table whose pid 1 is the harness and asserts a host-shaped pid 1 still finds nothing, so it cannot pass vacuously. The nudge case needs a real PID namespace, because the builtin `kill -0` gate cannot be reached through a fake ps, and it first proves the same fixture nudges with no lock present; it skips explicitly where unprivileged namespaces are unavailable. * no-mistakes(review): assert comm-strength detection from subprocess vantage in drift guard * fix(bin): verify the live harness guard at the strength the guarantee needs The marker-versus-ancestry boundary this branch ships is a strength claim: detect_own hands an args-strength verdict straight back to a retained foreign marker, so a harness is only protected where the ancestry walk reaches it at comm strength. The installed-harness drift guard probed the pane process alone. Under an interpreter shim the pane process IS the shim, whose own script path is args strength, while the native binary that carries comm strength is its child. The guard therefore observed args for Codex, passed, and would have kept passing if a release stopped spawning that native child at all, while real sessions silently regressed to the original bug. fm-harness.sh gains `ancestry-subtree`, which asks the walk from the pane process and every descendant of it, the vantage a tool subprocess actually occupies. The guard now requires comm strength somewhere in that set and requires every vantage to name the same harness. This supersedes the preceding commit's in-guard leaf walk, which reached the same vantage but left the logic inside the test file, where CI could not pin it and nothing else could reuse it. A harness-dependent check needs both halves: `tests/fm-harness-precedence.test.sh` now carries a portable case proving the subtree probe reaches a strength the top-of-session probe cannot, mutation checked twice, once against the pre-change script and once by disabling descendant enumeration. The subtree walk also avoids depending on tty and process-group semantics that differ between Linux and macOS. Verified live: codex-cli 0.152.0 reports [args codex;comm codex] and Claude Code 2.1.257 reports [comm claude]. * no-mistakes(review): narrow drift guard to the upward vantage path * no-mistakes(review): judge only comm-strength vantages in drift guard * no-mistakes(document): drop duplicated rationale in detection precedence evidence * no-mistakes(review): fix pid-1 nudge case vacuity and descent no-arg expansion * no-mistakes(document): drop branch-relative phrasing in detection precedence evidence * no-mistakes(review): guard remaining empty positional expansions in fm-harness * no-mistakes(document): scope cursor marker-ordering claim to the marker layer * no-mistakes(review): Prefer comm-strength leaves in equal-depth descent ties * no-mistakes(document): Document comm-strength descent tie-break --------- * no-mistakes(review): Blind ancestry in stale gemini/rovo marker-precedence tests * no-mistakes(document): Add missing equal-depth-tie test line to precedence evidence transcript * no-mistakes(review): Fix stale/vacuous agy precedence test, add agy to precedence suite and docs * no-mistakes(document): Fix stale kimi.md marker doc missed by ancestry-precedence fix --------- Co-authored-by: NewAiCoder <170579485+NewAiCoder@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
On macOS a login shell reports itself through
ps -o comm=with a leading dash:The process-ancestry walks pass that name straight to
basename, which parses-zshas the option flags-z -s -hand errors. Every walk that reaches a loginshell printed
basename: illegal option -- zto stderr, on effectively everyturn-end check. The walk continued, so this was noise rather than corruption -
but it polluted output where genuine warnings must stand out, and an argument
silently reinterpreted as flags is a latent correctness trap.
Fix
Every
ps-derivedbasenamecall now strips the directory with shell parameterexpansion instead:
bin/fm-session-lock-lib.sh-fm_harness_ancestry_pidandfm_harness_pid_alivebin/fm-harness.sh-detect_ownbin/andbin/backends/were swept for everypsconsumer; those three werethe only ones passing a process name to
basename.bin/fm-backend.shandbin/backends/tmux.shalready pattern-match on the fullcomm(tmux even stripsthe leading dash itself).
basenamecalls on ordinary file paths are untouchedand out of scope.
Why parameter expansion rather than
basename --Both are correct.
${comm##*/}was chosen because it avoids a subprocess onevery hop of a walk that runs up to sixteen times per check, and it matches the
expansion style already used in the backends.
Behaviour is otherwise preserved exactly. For every value
ps -o comm=canproduce,
${comm##*/}andbasename --agree. These walks resolve session-lockownership and harness identity, so a behaviour change here would be a safety
change; there is none.
Tests
The regression case lives in
tests/fm-secondmate-harness.test.sh, the existingowner of process-identity coverage for both walks. It places a
-zshprocessmid-ancestry with a real harness above it and asserts both correct resolution and
empty stderr, across
detect_own,fm_harness_ancestry_pid, andfm_harness_pid_alive.harness detection wrote to stderr: basename: illegal option -- z.Test hermeticity (follow-up commits)
Review surfaced that the pre-existing sibling test
test_pi_signed_detection_and_session_lock_identityin the same file depended onambient harness markers: with
CLAUDECODEset it resolvedclaudeinstead ofpi, and with an ambientPI_CODING_AGENT,GROK_AGENT, orFM_PI_HARNESSitsassertions flipped to the branch they were written to rule out. It was passing
without validating what it claimed to.
Those invocations now scrub exactly the markers each assertion is proving the
absence of - matching the convention already used in
tests/fm-session-start.test.sh. No assertion, message, or structure changed.The file now passes with
CLAUDECODEset, so the marker was masking a hermeticityproblem, not a defect in harness detection.
Verification
bin/fm-lint.shclean.tests/fm-secondmate-harness.test.shpasses, including withCLAUDECODE=1set.fm-claude-stop-autoarm,fm-kimi-harness,fm-grok-harness, andfm-session-startall pass.Summary by CodeRabbit
Bug Fixes
Tests