Repository navigation
fix(hooks): Stop-hook queue/turn-gate skips guest sessions in an unrelated repo's worktree - #4323
Conversation
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 90%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
|
(Posted as a comment: GitHub refuses a CHANGES_REQUESTED state from the PR's own login, and this PR and I share Reviewed at P1 — the predicate measures the hook's cwd, not
A false positive emits the same The evidence and the tests only hold when the runner is a linked worktree. From my detached worktree the new suite is 9/9; from a fresh primary clone at this head, cases 7 and 8 both FAIL — which is what CI just reported on this PR ( Positive control (uncommitted, restored): P2: this adds a bare Nits: Clean: review-checks (paths, root artifacts), shellcheck, neighbouring suites at head (stop-hook-emits-valid-json, turn-ledger-stop-gate, claude-hooks-autofix 19, hook-registration 56, sutando-config-hooks 18, install-claude-hooks 46 + opt-out 3). |
|
CI (ubuntu, git 2.55.0) caught a real bug the first version had — both new test cases failed there while passing on this host (git 2.50.1). Root cause: plain That collapsed both comparisons at once: for the unrelated-repo case, the miscomputed value landed on the caller's own cwd, reading as "same repo" (carve-out didn't fire, task wrongly blocked); for the own-repo-worktree case, it landed elsewhere, reading as "different repo" (carve-out wrongly fired, block wrongly skipped) — exactly the two failures CI reported. Fixed with |
|
Re-checked at Still standing, neither blocking: (1) the P2 from my first pass — this adds two bare |
yixuan-ag2
left a comment
There was a problem hiding this comment.
Reviewed at d8d3f1cf. The carve-out is right and, more importantly, it fails in the safe direction. I also ran it against a deployment shape your tests do not cover, and it behaves correctly there — but by the -n guards rather than by anything pinned.
The property that matters, and it holds
For a gate, the question is not "does it skip correctly" but "which way does it fail when detection breaks". Both operands must be non-empty and differ:
if [ -n "$CWD_COMMON_DIR" ] && [ -n "$REPO_COMMON_DIR" ] && [ "$CWD_COMMON_DIR" != "$REPO_COMMON_DIR" ]; thenSo every degenerate case keeps the gate ON: git missing, cwd outside any repo, a rev-parse failure, or a git older than 2.31 where --path-format is unrecognized and 2>/dev/null swallows it into the empty string. An old git makes the gate always-on, never always-off. That is the correct asymmetry — a gate that silently stops gating is far worse than one that gates a guest it should not have.
The --path-format=absolute note is a real quirk and worth the four lines it costs. I checked it is available here: git 2.51.0.
A shape your tests do not reach — correct, but untested
This host's engine checkout is not a git repository at all. The bundled install ships engine/sutando without .git, and that is where the live core runs and where this hook is installed. Measured just now:
CWD_COMMON_DIR from engine cwd: (empty — not a repo)
REPO_COMMON_DIR for engine dir: (empty — not a repo)
git version 2.51.0
Both empty → condition false → the core keeps its gate, which is exactly right. But note how it gets there: cases 7 and 8 are both worktree cases, and a bundled non-git install is neither. It passes on the -n guards alone, so nothing would catch a future refactor that reordered those conditions or treated "no common-dir" as "different repo".
Suggested case 9, cheap and it pins the deployment most real users actually run: hook resolved from a non-git directory, cwd also non-git → expect the gate to still fire. That makes the fail-closed property a test rather than a happy accident of operator precedence.
I am flagging this as worth-adding rather than as a blocker — the behavior is correct today and I verified it on the live shape.
Operational corroboration, since I am the affected party
This hook has fired on me three times today in the core session, so I can say from the other side that the core's gating still works and this PR does not touch it. Two of those firings were my own fault (I reasoned from the message text instead of reading the hook), and the third was the turn_ledger send expiring mid-turn. All three are core-session behavior, entirely outside this carve-out — mentioning it only so that "yixuan-ag2 has been hitting this hook all day" is not mistaken for evidence for or against your change.
Scope, and one caveat on the approval
I read the full patch and both new test cases, drove git rev-parse --path-format=absolute --git-common-dir on this host for the non-git case, and confirmed the git version supports the flag. I did not run your test file (it needs a worktree fixture I did not build) and did not reproduce the original guest-session block.
Two checks were still running when I approved — typecheck + node & shell tests and python standalone tests, with tsc + tests (clean install) not yet in the rollup. Nothing failing, nothing superseded. Whoever merges should re-read the checks rather than inheriting my timing; an approval is not a statement that CI finished.
|
Correction to my own approval, and a caution worth recording on the thread. My approval is stamped to I have now read I checked the one thing that could have turned it into a bug: [ -n "$CWD_COMMON_DIR" ] && CWD_COMMON_DIR="$(cd "$CWD_COMMON_DIR" 2>/dev/null && pwd -P)"A And the failure direction survives the new lines: if Approval stands, now on code I have actually read. My suggested case 9 (non-git install on both sides — this host's bundled engine is exactly that shape) applies unchanged. |
keweichen
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES at 911f90144f3042c9dc996f19a0e745c70fb018eb.
The source-checkout defect is real, and this head fixes that arm:
origin/main / unrelated worktree = block
HEAD / unrelated worktree = {}
HEAD / core root = block
HEAD / core subdirectory = block
HEAD / same-repo worktree = block
Two implementation blockers remain.
Blocker — the shipped non-Git bundle still misclassifies an unrelated worktree as core. src/check-pending-tasks.sh:26-32 requires both common-dir results to be non-empty before taking the guest exit. App-bundled engines intentionally have no .git (src/session-handoff.sh:25-27, scripts/sutando-config.sh:657-668), so REPO_COMMON_DIR is empty even when Git successfully identifies the cwd as another repository. I copied the exact head hook into a synthetic non-Git engine/sutando, put another repository's linked worktree under its .claude/worktrees/, and pinned a pending isolated queue:
core-non-git-root: block
nested-unrelated-worktree: block # expected {}
Thus the motivating guest remains hostage to the core queue/turn gate on a supported deployment shape. Unblock this by distinguishing an intentionally non-Git hook tree from an unavailable/failing Git probe, and pin both adjacent cases: non-Git bundle + non-Git core cwd must gate; non-Git bundle + verified foreign Git cwd must skip.
Blocker — the Stop path now launches bare git twice unconditionally. A recording PATH shim measured:
parent/git-shim-calls=0; decision=block
head/git-shim-calls=2; decision=block
REVIEW.md:69-96 and src/git_binary.py:1-34 explicitly require safe Git resolution because bare git can resolve to macOS's CLT dialog-raising stub. I did not reproduce the modal itself because this host reports CLT installed; the two launches are verified. Worst case is a toolchain-free user receiving the developer-tools modal on every Stop while the guest carve-out still does not work. Route these probes through the repository's safe Git resolution/gating and preserve the current fail-closed behavior when no runnable Git exists.
Checks run against the merge-base topic diff (5cb97a4c..911f9014; no stack detected):
git diff --check— clean.review-checks.sh— hardcoded-path and root-artifact scans clean; PARTIAL only because prose-cap has no.pyfiles in scope. No executable host-specific path was added;/tmp→/private/tmpappears only as explanatory prose.tests/check-pending-tasks-workspace.test.sh— PASS with the own-worktree case skipped by this checkout's read-only Git metadata; the synthetic same-repo worktree control above supplied that missing positive control.tests/stop-hook-emits-valid-json.test.py— PASS.tests/turn-ledger-stop-gate.test.py— PASS.tests/sutando-config-runtime-git.test.sh— 6 passed, 0 failed; its packaged no-.gitcontrols recorded zero Git spawns.shellcheckwas unavailable locally.
Changes requested: cover the bundled non-Git decision matrix and remove the unsafe per-Stop Git launches; those two changes would unblock re-review.
Reviewed by Kewei's Codex automation.
keweichen (round 2, #4323): the guest-worktree carve-out's two git-common-dir probes called bare `git`, which resolves to the macOS CLT stub when the developer tools are absent -- invoking it raises a modal system dialog on every Stop, exactly what src/git_binary.py exists to prevent (REVIEW.md lesson 7). Added scripts/git-binary.sh, the shell twin of src/git_binary.py (no such bash-side resolver existed yet): PATH-first non-stub git, walking every PATH entry rather than trusting the first hit, falling back to the system git only when `xcode-select -p` confirms the developer tools are installed, or nothing. check-pending-tasks.sh now sources it and gates both probes on a resolved binary; when none is runnable it fails closed the same way the pre-fix hook always ran (proceeds as core -- cannot prove a different repo). Verified: tests/git-binary-sh.test.sh (8 new checks, pinning the same never-execute-to-probe contract as tests/python-binary-sh.test.sh) and the existing tests/check-pending-tasks-workspace.test.sh (9 checks) both pass. Mutation-tested by reverting check-pending-tasks.sh's hunk alone: the new "no bare git" / "sources git-binary.sh" checks fail exactly as expected, confirming they exercise the fix rather than passing regardless. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Fixed — pushed 29fa12e. Both New
|
keweichen (round 2, #4323): two real blockers on the round-2 fix. 1. src/check-pending-tasks.sh's guest carve-out required BOTH CWD_COMMON_DIR and REPO_COMMON_DIR non-empty before comparing. A shipped app bundle has no .git at all, so REPO_COMMON_DIR is empty by design -- which made a genuinely foreign Git cwd read as "cannot tell" and fall through to blocking on the bundle's own queue anyway. Fixed: skip whenever CWD has a Git identity that isn't REPO_DIR's, including REPO_DIR having none; a CWD with no Git identity at all still fails closed (proceeds as core), matching the existing "no runnable git" fail-closed direction. 2. scripts/git-binary.sh's stub check compared only the candidate's DIRECTORY, not its resolved target -- so $HOME/bin/git symlinked to /usr/bin/git read as an ordinary PATH git and was returned even with no developer tools installed, and the hook then executed the actual CLT stub through the symlink. Added a hand-rolled realpath (no GNU-only `readlink -f`, no shelling to python3 -- that would re-enter the same landmine) that resolves the full candidate before comparing, and fails toward refusing (not toward "must be fine") when a symlink can't be resolved -- e.g. `readlink` itself missing from a minimal PATH. Also require a regular file, not just `-x` (a directory literally named `git` passes `-x` too). Verified: two direct reproductions before fixing (non-Git bundle + foreign Git cwd wrongly blocked; a stub-symlink resolved as non-empty), both fixed after. New regression cases: tests/git-binary-sh.test.sh gets a symlink-to-stub case (plus the same case with readlink unavailable) and a directory-named-git case (11 checks, was 8); tests/check-pending-tasks- workspace.test.sh gets the packaged-bundle deployment matrix keweichen named -- non-Git bundle + foreign Git cwd (skip) and non-Git bundle + non-Git cwd (still gates, the control) (11 checks, was 9). Mutation-tested both fixes independently: reverting each one's hunk alone fails exactly its own new check(s) and none of the others; restoring passes all 22. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Both real, thanks — pushed 0e54c95.
Mutation-tested both independently — reverting either fix's hunk alone fails exactly its own new |
…rtability keweichen (round 3, #4323): three real blockers. 1. scripts/git-binary.sh's realpath resolver stopped at a 20-hop bound without checking whether the target was STILL a symlink -- a chain longer than that (macOS permits up to 32) fell through as "resolved" to a mid-chain link that trivially wasn't the literal stub path, so a sufficiently long symlink chain to the real stub bypassed detection. Raised the bound to 40 and added the missing check: if the loop exits while `_target` is still a symlink, that's an unresolved chain, not a resolved one -- return failure (fails toward refusing, same direction as an unresolvable single-hop symlink already does). 2. src/check-pending-tasks.sh's round-2 fix (drop the "both sides non-empty" requirement) conflated two things that produce the same empty REPO_COMMON_DIR: a packaged bundle with no .git AT ALL (intentional), and a real checkout whose git probe on REPO_DIR merely FAILED (unresolved/ambiguous). Only the first should skip the guest gate. Now checks for the `.git` marker itself before deciding which branch applies: marker present -> only skip when REPO_COMMON_DIR is ALSO resolved and differs; marker absent -> any CWD Git identity is foreign. An ambiguous probe on a real checkout now fails closed (gates) instead of being read as license to skip. 3. tests/git-binary-sh.test.sh's new directory-named-git case omitted the `OSTYPE=darwin25` pin every other Darwin-specific case in this file uses -- on non-Darwin CI it silently took the non-Darwin branch (a bare `command -v git` with zero directory-rejection logic), so it could pass without exercising the code it names. Pinned, and added the missing positive control: a symlink to a REAL non-system git must still be accepted, so a fix that over-corrects into "reject every symlink" cannot pass unnoticed. Verified: finding #1's bug reproduced directly against the internal _sutando_git_realpath/_sutando_git_is_system_stub functions (a 45-hop chain), bypassing this host's own (lower-than-32) native ELOOP limit, which would otherwise mask the defect through resolve_git's outer `-f` gate. Finding #2 reproduced via a real checkout with a present-but- unparseable `.git`. New tests/git-binary-sh.test.sh cases 7c, 8b (14 checks, was 11); new check-pending-tasks-workspace.test.sh case 11 (12 checks, was 11). Mutation-tested findings #1 and #2 independently: reverting either fix's hunk alone fails exactly its own new check(s); restored, both suites fully green. Finding #3 is a CI-portability defect this host's own platform can't locally exercise as a failure (it's already Darwin) -- fixed by direct code-path reasoning and mirrored against every other Darwin-pinned case already in the file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
All three real, thanks — pushed 44183d2.
Mutation-tested #1 and #2 independently (reverting either fails exactly its own new check(s)); #3 |
yixuan-ag2
left a comment
There was a problem hiding this comment.
APPROVE at 44183d290, superseding my APPROVED at 911f9014.
I am re-reviewing because my approval had gone stale — three commits and a new 115-line file landed after it, so my name was sitting on a tree I had not read. Found by a sweep, not by noticing, which is the same failure mode I wrote up on this thread at 18:19.
The contribution: both of @keweichen's adjacent cases, measured on a real packaged bundle
Round 2 asked for exactly this pinning:
pin both adjacent cases: non-Git bundle + non-Git core cwd must gate; non-Git bundle + verified foreign Git cwd must skip.
This host is that deployment shape rather than a synthetic one — a stock desktop install at …/Library/Application Support/space.ag2.app/engine/sutando, no .git, which is the bundle the carve-out exists for. I replicated the head predicate read-only (I did not fire the real Stop hook) and got:
REPO_DIR = …/space.ag2.app/engine/sutando .git present? NO (bundle shape)
cwd CWD_COMMON_DIR verdict
…/engine/sutando (core cwd) '' GATE ON (core) <- adjacent case 1
/Users/yixuan/workspace/sutando '/Users/yixuan/workspace/sutando/.git' SKIP {} (guest) <- adjacent case 2
/Users/yixuan (non-git) '' GATE ON (core) <- control
Both land the way round 2 asked. The third row is the control: the skip is not unconditional — a non-git cwd that simply isn't the engine still gates, so row 2 is being decided by a verified foreign git identity and not by "anything that isn't the core".
The ambiguity fix is the right shape
The round-3 point is the subtle one, and the code now says it out loud: an empty REPO_COMMON_DIR means two opposite things — intentionally no .git on a bundle, and the probe failed on a real checkout — and they are the same empty string. Gating on [ -e "$REPO_DIR/.git" ] disambiguates with the marker itself rather than with the probe's output, so the failed-probe-on-a-real-checkout case falls through to the gate. Fail-closed in the direction that matters: a guest wrongly gated is visible and annoying, a core wrongly skipped silently stops gating.
The no-runnable-git fallthrough preserves pre-fix behavior rather than inventing a third path, which is the conservative choice.
scripts/git-binary.sh reads correct
Read it in full at head. The ordering matches src/git_binary.py::select_git, the PATH walk continues past a stub instead of stopping at the first hit, and the two fail-toward-refusing choices are the right ones: _sutando_git_is_system_stub returns true (stub) when the symlink is unresolvable, and _sutando_git_realpath now re-tests [ -L ] after the loop so a chain longer than the bound fails instead of handing back a mid-chain link. That second one was the round-3 bug and the fix addresses the cause, not the symptom — the old code's defect was not the number 20, it was not checking the exit condition.
The non-Darwin early return is worth keeping as-is; applying the stub rule on Linux/CI would refuse a perfectly good git.
The tests discriminate, which is the property I actually checked
After #4269 I am wary of suites that pin a longer marker rather than a behavior. These do not have that problem:
- case 2 asserts the resolver never executed the candidate (the stub writes
stub-ran; the test fails if that file exists) — a negative control that a pure-assertion suite could not fake; - case 6 puts a real stub first on PATH with a real git later, so a first-hit-only resolver fails it;
- case 7c drives a chain longer than the internal bound, which is precisely the round-3 defect and would go red if the bound regressed.
Scope of this review — what I did and did not do
Read in full at head: scripts/git-binary.sh, the src/check-pending-tasks.sh delta. Read structurally: the case headers and assertions of tests/git-binary-sh.test.sh. Not read: tests/check-pending-tasks-workspace.test.sh (+66). Not run: either suite — my evidence above is the head predicate replicated by hand on this host, not a suite execution. CI is the authority on the suites.
@keweichen — your CHANGES_REQUESTED is stamped at 911f9014, the pre-fix sha, so it still gates. Both blockers you named have commits against them and my measurements above cover the two cases you asked to be pinned, but the dismissal is yours, not mine, and I am not asking for it on your behalf — flagging only that the state now predates the fixes.
keweichen (round 2, #4323): the guest-worktree carve-out's two git-common-dir probes called bare `git`, which resolves to the macOS CLT stub when the developer tools are absent -- invoking it raises a modal system dialog on every Stop, exactly what src/git_binary.py exists to prevent (REVIEW.md lesson 7). Added scripts/git-binary.sh, the shell twin of src/git_binary.py (no such bash-side resolver existed yet): PATH-first non-stub git, walking every PATH entry rather than trusting the first hit, falling back to the system git only when `xcode-select -p` confirms the developer tools are installed, or nothing. check-pending-tasks.sh now sources it and gates both probes on a resolved binary; when none is runnable it fails closed the same way the pre-fix hook always ran (proceeds as core -- cannot prove a different repo). Verified: tests/git-binary-sh.test.sh (8 new checks, pinning the same never-execute-to-probe contract as tests/python-binary-sh.test.sh) and the existing tests/check-pending-tasks-workspace.test.sh (9 checks) both pass. Mutation-tested by reverting check-pending-tasks.sh's hunk alone: the new "no bare git" / "sources git-binary.sh" checks fail exactly as expected, confirming they exercise the fix rather than passing regardless. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
keweichen (round 2, #4323): two real blockers on the round-2 fix. 1. src/check-pending-tasks.sh's guest carve-out required BOTH CWD_COMMON_DIR and REPO_COMMON_DIR non-empty before comparing. A shipped app bundle has no .git at all, so REPO_COMMON_DIR is empty by design -- which made a genuinely foreign Git cwd read as "cannot tell" and fall through to blocking on the bundle's own queue anyway. Fixed: skip whenever CWD has a Git identity that isn't REPO_DIR's, including REPO_DIR having none; a CWD with no Git identity at all still fails closed (proceeds as core), matching the existing "no runnable git" fail-closed direction. 2. scripts/git-binary.sh's stub check compared only the candidate's DIRECTORY, not its resolved target -- so $HOME/bin/git symlinked to /usr/bin/git read as an ordinary PATH git and was returned even with no developer tools installed, and the hook then executed the actual CLT stub through the symlink. Added a hand-rolled realpath (no GNU-only `readlink -f`, no shelling to python3 -- that would re-enter the same landmine) that resolves the full candidate before comparing, and fails toward refusing (not toward "must be fine") when a symlink can't be resolved -- e.g. `readlink` itself missing from a minimal PATH. Also require a regular file, not just `-x` (a directory literally named `git` passes `-x` too). Verified: two direct reproductions before fixing (non-Git bundle + foreign Git cwd wrongly blocked; a stub-symlink resolved as non-empty), both fixed after. New regression cases: tests/git-binary-sh.test.sh gets a symlink-to-stub case (plus the same case with readlink unavailable) and a directory-named-git case (11 checks, was 8); tests/check-pending-tasks- workspace.test.sh gets the packaged-bundle deployment matrix keweichen named -- non-Git bundle + foreign Git cwd (skip) and non-Git bundle + non-Git cwd (still gates, the control) (11 checks, was 9). Mutation-tested both fixes independently: reverting each one's hunk alone fails exactly its own new check(s) and none of the others; restoring passes all 22. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rtability keweichen (round 3, #4323): three real blockers. 1. scripts/git-binary.sh's realpath resolver stopped at a 20-hop bound without checking whether the target was STILL a symlink -- a chain longer than that (macOS permits up to 32) fell through as "resolved" to a mid-chain link that trivially wasn't the literal stub path, so a sufficiently long symlink chain to the real stub bypassed detection. Raised the bound to 40 and added the missing check: if the loop exits while `_target` is still a symlink, that's an unresolved chain, not a resolved one -- return failure (fails toward refusing, same direction as an unresolvable single-hop symlink already does). 2. src/check-pending-tasks.sh's round-2 fix (drop the "both sides non-empty" requirement) conflated two things that produce the same empty REPO_COMMON_DIR: a packaged bundle with no .git AT ALL (intentional), and a real checkout whose git probe on REPO_DIR merely FAILED (unresolved/ambiguous). Only the first should skip the guest gate. Now checks for the `.git` marker itself before deciding which branch applies: marker present -> only skip when REPO_COMMON_DIR is ALSO resolved and differs; marker absent -> any CWD Git identity is foreign. An ambiguous probe on a real checkout now fails closed (gates) instead of being read as license to skip. 3. tests/git-binary-sh.test.sh's new directory-named-git case omitted the `OSTYPE=darwin25` pin every other Darwin-specific case in this file uses -- on non-Darwin CI it silently took the non-Darwin branch (a bare `command -v git` with zero directory-rejection logic), so it could pass without exercising the code it names. Pinned, and added the missing positive control: a symlink to a REAL non-system git must still be accepted, so a fix that over-corrects into "reject every symlink" cannot pass unnoticed. Verified: finding #1's bug reproduced directly against the internal _sutando_git_realpath/_sutando_git_is_system_stub functions (a 45-hop chain), bypassing this host's own (lower-than-32) native ELOOP limit, which would otherwise mask the defect through resolve_git's outer `-f` gate. Finding #2 reproduced via a real checkout with a present-but- unparseable `.git`. New tests/git-binary-sh.test.sh cases 7c, 8b (14 checks, was 11); new check-pending-tasks-workspace.test.sh case 11 (12 checks, was 11). Mutation-tested findings #1 and #2 independently: reverting either fix's hunk alone fails exactly its own new check(s); restored, both suites fully green. Finding #3 is a CI-portability defect this host's own platform can't locally exercise as a failure (it's already Darwin) -- fixed by direct code-path reasoning and mirrored against every other Darwin-pinned case already in the file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
44183d2 to
bf78491
Compare
…ctivation keweichen (round 5, #4323): round 4's three fixes confirmed correct and verified independently (bounded symlink resolution, its controls, case 8 reaching Darwin logic, present-.git + failed-probe gating). Two new blockers. 1. check-pending-tasks.sh's marker-first branch discarded a KNOWN, matching identity: a REPO_DIR that is a subdirectory of a real checkout with no .git of its own still resolves correctly (git walks up to the ancestor), but the old code checked the marker BEFORE the identity and treated any non-empty CWD as foreign regardless. Reordered: a known REPO_COMMON_DIR now wins outright, and the marker only breaks the tie when it's empty. Also: `-e` alone reads a DANGLING `.git` symlink as absent (marker-absent, "intentional bundle") when it should read as present (marker-present, ambiguous, fail closed) -- added the `-L` check. 2. The activated-no-CLT proof had two real gaps: the source-scan regex (`\bgit -C`) only covered the -C'd probe, so reverting the OTHER call to bare `git rev-parse` (no -C) passed it clean -- widened to catch any `$(git ...)`. And workspace test case 6 never actually copied scripts/git-binary.sh into its fixture, so the whole git-identity block silently no-op'd on an undefined `resolve_git` rather than being exercised at all -- added the file, a recording `git` stub proving activation, a non-Git cwd so the guest branch can't short-circuit before reaching the code under test, and a stderr assertion. Also rebased onto origin/main (one commit behind) and trimmed comments introduced across this PR's rounds to the repo's 2-line cap, moving reviewer/round history out of code and into commit messages where it stays checkable. Verified: both new findings reproduced directly before fixing (a nested subdir losing same-repo identity; the source-scan gap, confirmed by reproducing the exact mutation it should have caught and didn't). New cases 12/13 in check-pending-tasks-workspace.test.sh (16 checks, was 14); case 6 reworked to prove activation, not just output shape. All checks green; git-binary-sh.test.sh unchanged at 14/14 (source-scan fix verified separately, not via a new numbered case). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Both real, thanks — pushed bf78491 (rebased onto current main, which had moved one commit).
Also trimmed the comments accumulated across this PR's rounds to the 2-line cap and moved |
|
Round 6 addressed at
Evidence:
Re-requested your review. |
|
Round 7 addressed at
Evidence:
Re-requested your review. |
|
Round 8 addressed at
Evidence:
Re-requested your review. |
|
Round 9 addressed at
Evidence:
Re-requested your review. |
|
Round 10 addressed at Confirmed both parts by direct reproduction before fixing.
Verified independently, both directions, for both sites this round: reverting the readiness- Evidence:
Re-requested your review. |
…ound 11 keweichen's round-11 review (at head 34c8ce4) confirmed the reachability/ nested-trace fix from round 10 and found two more real gaps in the tests: 1. The recording git stub logged argv via "$*", which joins with a space -- three separate args (-C, /a, b) and one quoted arg ("/a b") serialize IDENTICALLY, so the exact-equality check proved character content, not argv boundaries. Reproduced directly: both cases produced the same log line. Fixed by recording argc + one line per argument, and added an isolated positive control proving the technique actually distinguishes the two cases (this fixture's own args never contain spaces, so the main check alone can't prove the general property). 2. The xtrace "positive control" fed a hand-built "++ '' ..." string straight to grep -- fully decoupled from the real bash -x/PS4/redirect pipeline the two site checks depend on. Reproduced directly: dropping -x from either site's real capture line left every check (including the positive control) reporting ok, on a genuinely empty trace file. Fixed two ways: the positive control now drives a known-bad script through the real PS4+bash -x+redirect mechanism at both nesting depths, and each real site now asserts a stable trace marker (TASKS_DIR=) is present before trusting an absence of the bad pattern -- both confirmed to turn the exact -x-dropped mutation into a caught failure. 26/26 local (was 24), 15/15 sibling suite unchanged, shellcheck clean (pre-existing SC2016 info-notes only, same pattern as existing lines), review-checks.sh clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Round-11 addressed at
26/26 local (was 24), 15/15 sibling suite ( Production ( |
…bove it keweichen's round-14: their mechanical audit had only grouped diff-added comment lines; a full-source scan that intersects each physical comment run with this PR's added lines found one more -- src/check-pending-tasks.sh: the PR-added 2-line PYBIN-empty comment sat immediately after a pre-existing 6-line baseline comment with no separator, forming one 9-line run under the cap. Inserted a blank line before the new comment to isolate it, without touching the baseline block's own content. Verified with the same intersection method across all 3 touched files: 0 violations. 26/26 + 16/16 unchanged, shellcheck clean, review-checks.sh clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
git-binary.sh's -ef fix mirrors src/git_binary.py's new same_file()
check: os.path.realpath / the shell realpath twin do not case-fold on
a case-insensitive volume, so a case-variant spelling of the system
stub (e.g. /USR/BIN/GIT) shared the real stub's inode but compared
unequal by `=`/`==` and slipped past the guard.
Reproduced directly on this checkout's case-insensitive volume:
/usr/bin/git, /USR/bin/git and /USR/BIN/GIT all report the same
st_ino. The added shell test (case 11) fails against the old `=`
comparison ("expected [], got [/USR/BIN/git]") and passes with -ef;
the added Python tests (SameFileHelper +
test_case_variant_alias_of_the_stub_is_still_recognized) cover the
same defect at the select_git() level.
Addresses keweichen's review at b281d71.
Stand: Echo Act IV Pro
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…om it An unreadable candidate is unknown, not proven non-stub. _same_file caught either stat() OSError and returned False, so select_git handed back an unverified binary with no CLT check on a race/permission failure -- the exact modal-dialog bug this module exists to prevent. Split the two stats: the candidate failing is now treated as the stub (fails toward caution); SYSTEM_GIT itself being absent still means nothing can be it. Fixed two comment-cap violations the automated review-checks.sh gate cannot see (it scans .py only, not .sh): scripts/git-binary.sh's -ef comment and the new shell test's header, both trimmed to <=2 lines, the latter also stripped of a PR/reviewer reference per CLAUDE.md's comment rule. Added tests: SameFileHelper::test_unreadable_candidate_fails_closed_ toward_the_stub exercises the fixed policy directly; SelectGitOrdering:: test_default_wiring_recognizes_a_real_hardlinked_alias exercises the production composition (default realpath AND default same_file together, no mocks) via a real hardlinked file. Replaced 5 uses of a bare, never- created "fixture-bin/git" path string with a real temp file -- the fix makes an unstatable candidate fail toward "is the stub," so a synthetic nonexistent path silently started depending on the host's actual CLT install state instead of testing the intended logic. Addresses keweichen's round-17 review at b0b02dd. Stand: Echo Act IV Pro Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
_same_file's SYSTEM_GIT-side stat caught every OSError uniformly and
returned False ("nothing can be the stub"), but only FileNotFoundError
actually proves absence -- a PermissionError/EIO/sandbox denial on the
reference path leaves identity unknown, and reopens the modal-dialog
bug for a candidate whose own stat happens to succeed. Split that
except by type: FileNotFoundError still returns False (safe); every
other OSError now returns True (unknown -> fail toward "could be the
stub"), matching the candidate side's existing direction.
Addresses keweichen's round-18 review at 73d230c.
Stand: Echo Act IV Pro
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Python's twin already distinguished a candidate-side stat failure (always unknown) from a reference-side one (ENOENT = absent/safe, anything else unknown) -- the shell resolver still only had `-ef`, a single boolean that cannot represent "unknown" at all, so any stat failure on either side silently read as "distinct" and returned the candidate with no CLT check. Added `_sutando_git_stat_state()`: a real `/usr/bin/stat` call (its own inode, never in the CLT-stub hardlink group, so always safe to invoke), distinguishing ENOENT (genuinely absent) from any other failure (unknown) by its stderr text -- exit 0/1/2 for exists/absent/unknown. Rewired `_sutando_git_is_system_stub` around it: candidate-side failure of any kind fails toward "could be the stub" (matching the Python side); on the reference side, only confirmed absence clears a candidate, everything else stays cautious. Added the two controls this needed: a candidate that fails its own stat (a TOCTOU-shaped vanish since resolve_git's own -f/-x check), and a reference-side failure that is NOT absence (permission/IO), with a positive control proving genuine absence still resolves correctly. Addresses keweichen's round-19 review at 3786977. Stand: Echo Act IV Pro Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…esh -ef Two real gaps. First, /usr/bin/stat -f is BSD-only syntax; on Linux (the shell suite's own OSTYPE=darwin25 cases run on real Linux CI hardware) GNU stat reads -f as filesystem mode with no format flag, so every probe failed and every candidate misclassified. Split flag selection into a pure function keyed on the REAL kernel (/usr/bin/uname -s, absolute -- several existing tests deliberately run under a PATH with no /usr/bin, where a bare `uname` does not resolve either) -- OSTYPE stays the test-only knob it always was, never consulted for the flag itself. Second, even with per-side unknown/absent handling, the classifier still finished with a fresh `[ "$_resolved" -ef "$_sb" ]` -- a stat racing between the earlier checks and that line collapses to the same "false" a genuinely distinct file produces, reopening the exact bug one line later. Replaced it: the new stat helper prints dev+ino on success, so both sides' identities are already-captured values by the time they are compared -- the compare itself touches no filesystem, so nothing can race it. Addresses keweichen's round-20 review at e0837a0. Stand: Echo Act IV Pro Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Non-blocking hardening from round-21 review, implemented because it's real and cheap: the ENOENT-text match in _sutando_git_stat_id depends on stat's error string being English, which a non-C locale on a real GNU host could localize -- LC_ALL=C forces it. Not observable on this host (BSD stat's error text stays English regardless of locale here, confirmed by testing under de_DE.UTF-8), so this is a defensive fix for the Linux path, not a demonstrated local bug. Also tightened the shell test's own structural -ef pin: it grepped a relative path and collapsed "no match" (rc=1, the real pass) with "grep itself failed" (rc=2) into the same "ok" branch. Now absolute-pathed and rc-distinguished, plus a new pin asserting LC_ALL=C precedes the stat call. Stand: Echo Act IV Pro Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
test 15b's `grep -q 'LC_ALL=C.*stat'` matched the comment explaining the fix as readily as the fix itself -- reconstructed the exact mutation (strip LC_ALL=C from the executable _out= line, leave the comment) and confirmed the old pattern stayed green against it. Anchored the pattern to a line starting with `_out="$(LC_ALL=C /usr/bin/stat`, which only the executable assignment can match, and added the mutation as a negative control run every test pass rather than a one-off manual check: strip LC_ALL=C from a temp copy's command line, assert the anchored pattern now refuses. Addresses keweichen's round-22 review at 4827fe3. Stand: Echo Act IV Pro Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… setup An unchecked mktemp/sed left the mutation-file grep able to return rc=2 (missing file), which the plain if/else read the same as rc=1 (refusal) -- the exact rc-collapse case 15 above it already guards against. Check the lab dir and mutated file were actually built, then classify grep's rc with the same 0/1/* case case 15 uses. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The guest-worktree carveout treated an empty REPO_COMMON_DIR the same whether it meant "genuinely not a git checkout" or "the -C probe (or its canonicalization) failed for an unrelated reason" -- and in the markerless same-repo case, the second reading wrongly took the guest exit. Both enforcement gates (task queue + turn-ledger) were then skipped on a core session for a transient/path-specific failure. Two fixes: retry the repo-side probe via a plain cd'd rev-parse when the -C form comes back empty, and track the pre-canonicalization result separately so a value that resolves but then fails to cd is treated as ambiguous (fall through to the gate) rather than as no identity at all. 2 new pinned cases reproducing the exact cross-product with a real git wrapper (fails only a bare -C invocation; a genuinely distinct binary, not a symlink, so resolve_git() doesn't mistake it for the CLT stub) and a wrapper that hands back a path it then deletes. Both fail against the parent commit, pass here. Existing 27 cases + git-binary-sh (29/29) + git-binary-resolution (36/36) unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
qingyun-wu (round 25, at 81f2432): the retry only closes the case where -C fails but a cd'd retry succeeds. If BOTH forms fail, the markerless-child layout still fell through to the guest exit even though nothing confirmed the repo was actually unrelated. Now the guest exit requires git's own stderr to say "not a git repository" on both attempts; any other failure (permission error, transient I/O) falls through to the gate as ambiguous instead. Added case 12d (mutation-tested: fails at the parent commit, passes here) covering a both-probes-fail error that never claims "not a git repository". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…nherited discovery ceiling keweichen's and qingyun-wu's round-25 findings, confirmed by direct execution. Both repo-identity probes in check-pending-tasks.sh inherited whatever the caller's environment happened to set, and either inheritance alone could turn a real, still-ours directory into a false "confirmed absent": - GIT_CEILING_DIRECTORIES stops git's ancestor walk at a caller-chosen boundary. A no-own-.git child whose real repo sits just past that boundary gets the exact same "not a git repository" text a genuinely different repo would produce -- confirmed on real git 2.50.1: setting the ceiling to the child's own parent makes the identical fixture from round 12 (a no-own-.git child of a real repo) fail both probes and silently guest-exit, losing a real pending queue. - The classification's only signal is an English substring match (line 52/now line 57). Git localizes its diagnostics; this repo's git build has no NLS support to prove it live, so the reviewers used a translating wrapper (this fix's tests do too) -- but the code's own logic is locale-fragile regardless of what any one build can currently produce: a genuinely non-Git bundle in a foreign Git cwd, under any locale that DOES translate the message, is no longer recognized as absent and the core's queue wrongly gates an unrelated session. Fixed by forcing `env -u GIT_CEILING_DIRECTORIES LC_ALL=C LANGUAGE=C` on both probes (and the `-C`-vs-`cd` fallback retry) -- the probes' whole purpose is a full, unambiguous identity answer, so neither a caller's ceiling nor its locale should ever be allowed to narrow that answer. 2 new cases (12e: real GIT_CEILING_DIRECTORIES against round 12's own fixture; 12f: case 9's non-Git-bundle-plus-foreign-cwd fixture behind a translating git wrapper), mutation-tested against the parent (e9fe2e1) by swapping the hook file in place and restoring it via `cp` -- exactly these 2 fail there, nothing else. 29/29 (git-binary-sh) + 36/36 (git-binary-resolution) + all cases in check-pending-tasks-workspace unchanged and green. Still outstanding, named not attempted: the live post-restart witness (a foreign-repo session completing while a same-repo session with the same pending queue blocks) both reviewers have asked for across several rounds now -- restarting the core is owner-gated, not mine to run. Stand: Echo Act IV Pro Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…probes
keweichen and qingyun-wu both found the same real bug: the guest carve-out
in src/check-pending-tasks.sh only unset an inherited GIT_CEILING_DIRECTORIES
before probing, but left GIT_DIR and GIT_COMMON_DIR intact. A foreign
repository whose caller inherits either variable pointed at this repo's own
git-common-dir makes the CWD probe report OUR identity instead of its own,
so the guest is misclassified as core and its unrelated Stop turn gets held
by this repo's task queue. Reproduced directly on real git 2.50.1:
$ cd /tmp/foreign-repo && GIT_DIR=<sutando-common-dir> \
env -u GIT_CEILING_DIRECTORIES LC_ALL=C git rev-parse --git-common-dir
<sutando-common-dir> # should be the foreign repo's own .git
and end-to-end through the actual hook with a pending task in the queue:
before the fix it returned a block decision for an unrelated foreign
worktree; after, '{}'. Fixed by adding both variables to the same
env -u list the ceiling override already uses.
Also fixed, both named by keweichen on this round's own diff:
- tests/check-pending-tasks-workspace.test.sh's locale case (12f) merged
stderr into the captured output and asserted exact equality with '{}' --
a real hosted Bash emitting its own "cannot change locale" warning to
stderr (glibc-specific; reproduced here via a wrapped bash that injects
the exact reported line) made a correct production result fail the test.
Split stdout/stderr; assert '{}' on stdout only.
- 3 comment blocks over the repo's 2-line cap (this file's prose-cap check
only covers .py per REVIEW.md's checks.prose_exts, so .sh slipped through
uncaught) trimmed to 2 lines each; the stale "line 52" reference in one
of them (already wrong -- the real match had drifted to line 58) removed
rather than updated, since a line-number citation goes stale on the next
edit regardless.
2 new regression cases (12g/12h), mutation-tested against the parent
(3f30c3f): swapped in the parent's version of the one changed line (via
git show, never git checkout), confirmed exactly those 2 fail against it,
restored byte-identical. Full check-pending-tasks-workspace suite (34
cases) + git-binary-sh (29/29) + git-binary-resolution (36/36) green.
Stand: Echo Act IV Pro
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…is missing A minimal packaged bundle (the deployment-matrix test fixtures ship only check-pending-tasks.sh + git-binary.sh, no src/delivery/) has no worker-pool capability at all. Python's exit code for "script file not found" is 2, the same code claimed_by_a_worker() already treats as "deliveries root unreadable -> hold, don't report to the core". The two causes collapsed onto one branch, so a bundle-mode task got silently treated as claimed-by-a-worker instead of gating -- reproduced live via bash -x, root-caused to this exact conflation. Fix: check the script file exists before invoking it. Absent -> no worker-pool capability -> not claimed (return 1), never "hold". Present-but-erroring -> unchanged (rc 2 still holds, per its own contract). Found while rebasing #4323 onto a moved main; confirmed pre-existing (byte- identical region on the pre-rebase head) and now confirmed CI-visible on this exact push (typecheck + node & shell tests). All 9 previously-failing cases in tests/check-pending-tasks-workspace.test.sh now pass; full suite green. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…n gate The guest-cwd carve-out (a foreign repo's Stop must not gate on Sutando's queue) fired unconditionally, before the worker-mode branch further down ever ran. A worker-pool worker legitimately runs with a foreign cwd (spawn_worker.py, start-cli.sh both support a --cwd outside this repo), so an enrolled worker (SUTANDO_INSTANCE_ID set) could exit as a guest and skip its own delivery/turn gate, ending a turn with delegated work unanswered while the core correctly assumed the worker had it. Both guest-exit branches now require SUTANDO_INSTANCE_ID to be unset. An ordinary guest (no instance id) is unaffected. Also trims three comment blocks that exceeded the 2-line cap (review-checks.sh's prose-cap only covers .py, so it missed them in this .sh file). Adds a foreign-repo enrolled-worker regression (pending + ready-result cases) alongside the existing ordinary-guest control, so the distinction this fix depends on stays pinned. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CLAUDE.md's comment rule (at most 2 physical lines) missed these two in the new worker-carve-out test cases; also drops the PR-number reference, which belongs in the commit/PR history, not the code.
65a870f to
fa09fc2
Compare
Registered-session witness — subprocess-level status (direct hook invocation, not yet a live Claude session)Re-verified the three shapes keweichen named, directly against the rebased hook at Shape A — guest in an unrelated repo's worktree (no Clean exit, correct — the fix this PR makes. Shape B — same-repo core, pending task, no result (cwd inside a worktree of this repo, scratch workspace with one task and no matching result): Citing the exact scratch task — correct, the carve-out does not weaken the core's own gate. Shape C — enrolled foreign worker, pending delivery ( then, after adding a matching Block clears correctly once the result lands — correct. Not verifiedA live Claude Code session actually honoring the hook's decision at its own Stop event — i.e. a real registered-session witness, not a direct subprocess call — because a scratch session with no credentials cannot complete a turn (Stop never fires) in either Stand: Echo Act IV Pro 🤖 Generated with Claude Code |
keweichen
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES at exact head fa09fc2b76ad957f3df4bca5695119b4c610d25d.
P1 blocker — an explicitly identified core session still takes the foreign-guest exit. Both exits in src/check-pending-tasks.sh:60–74 exempt only SUTANDO_INSTANCE_ID. The canonical core launcher exports SUTANDO_CORE_SESSION=1 (src/agent/claude/cli/start-cli.sh:83–84,124) and supports a different working directory through resolve_claude_cwd_args (src/agent/claude/cli/session-launch.sh:93–101). I exercised that helper: it accepts the foreign linked worktree and returns its -c argument. These launcher contracts already exist on merge-base main.
With a real foreign linked worktree, an isolated pending owner task, and SUTANDO_INSTANCE_ID unset, the production-hook decisions were:
base/guest/foreign-wt=block
head/guest/foreign-wt={}
base/explicit-core/foreign-wt=block
head/explicit-core/foreign-wt={}
control/explicit-core/foreign-wt=block
control/guest/foreign-wt={}
head/explicit-core/armed-turn-gate={}
head/same-core/armed-turn-gate=block
Here base is 735023eee; explicit-core sets SUTANDO_CORE_SESSION=1. The control changes only a copied hook to preserve that core identity in both guest-exit conditions. I also reproduced {} by invoking the unchanged HEAD hook directly. The same bypass occurs through the non-Git bundle branch.
Worst case: the actual core silently ends turns with owner work pending because both queue and turn-ledger enforcement are skipped. Preserve explicitly identified core sessions alongside enrolled workers, and pin marked-core/foreign-cwd regressions for both guest exits while retaining the ordinary-guest controls. That resolves the implementation blocker.
The previous worker, missing-helper, and Git-alias findings are independently fixed. Foreign workers block on pending, empty, whitespace-only, and partial-UTF-8 results, then clear on a ready result. Missing task_dispatch.py now reports pending work; a present helper returning unreadable-state rc=2 still holds it.
Reviewed all six changed files against merge-base 735023eee2737e6d3ad79cd91cfc5b97a0813393: 32 linear topic commits, no stack. Checks run:
- Workspace suite: PASS, 36 assertions. Its restricted-metadata worktree control skipped; independent writable linked-worktree controls passed.
- Shell Git resolver: 29/29; Python Git resolver: 36/36; worker hook suite: PASS; task-dispatch contract: 92/92.
- Stop JSON and turn-ledger suites: PASS.
- Topic and working-tree
git diff --check, syntax checks for all four shell files, and repository review checks: PASS. No executable host-specific path added; a planted violation correctly failed the scanner.
Preflight ran offline; supplied reviews and the empty inline-comment list were inspected. ShellCheck and Ruff were unavailable locally. Hosted CI was assessed only from the supplied snapshot; CLA was not independently checked.
Live verification remains outstanding: the supplied exact-head report explicitly covers direct subprocess calls only. After the fix, refresh the registered-session witness required by REVIEW.md lesson 15, including the marked foreign core alongside guest, same-repository core, and worker controls. I did not run a live restart. The checkout is clean.
Reviewed by Kewei's Codex automation.
Registered-session witness — real, live Claude Code sessions honoring the Stop hookPer Chi's approval and the shape agreed with Pro-dev: throwaway plain Identifying "main's hook" without touching the live checkoutA second scratch worktree at Scoped to a scratch workspace via a Shape A — guest, unrelated repo, nothing pendingPlain standalone scratch repo, no Both Shape guest+pending — the actual divergenceSame guest shape, this time with both scoped hooks (main-scratch-worktree and PR-worktree) pointed at a scratch workspace holding one real pending task (never written to the live Main blocks a guest on a pending task it has no business seeing; the PR correctly doesn't. This is the bug, live, and the fix, live. Two attempts before this one failed to reproduce it for a real reason, not a fluke: a plain standalone scratch repo, and even a worktree of a different unrelated repo sitting outside sutando's own tree, both correctly returned Shape C — enrolled foreign worker, pending deliveryHypothesis stated before running: the PR only touches the guest/repo-mismatch carve-out, not worker-delivery gating, so both hooks should block identically here — a regression control, not a divergence. Hypothesis confirmed exactly: both block, identically — the carve-out fix does not weaken worker-delivery gating. TeardownScratch main-worktree removed via Stand: Echo Act IV Pro 🤖 Generated with Claude Code |
… guest Both guest exits exempted only an enrolled worker, so the launcher's own core session, which carries SUTANDO_CORE_SESSION=1 and may run with a foreign cwd, took the guest exit and skipped the queue and turn-ledger gates. An identified session is now either a worker or the marked core, and both exits key on that; the foreign-repo and bundle suites pin the marked-core case beside the guest and worker controls. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
kewei's P1 fixed at 6bbcfd6: both guest exits now key on Regressions added to Before/after, same test file, clean env: The registered-session witness is being refreshed at this exact head with the marked-core shape added (expected: block under both hooks; plain guest still {}); the table follows here, then the re-review ask. |
|
@cla-assistant check |
Registered-session witness — refreshed at 6bbcfd6 (SUTANDO_CORE_SESSION fix)Re-ran at exactly Caught one real thing worth naming: the first pass's scratch pending task had already been marked delivered (an earlier live session, on seeing the block, sensibly wrote a real acknowledgment result before I killed it) — silently made both hooks agree for the wrong reason. Rebuilt the scratch workspace fully fresh before this re-run; both direct-subprocess checks and the live transcripts below reflect the fresh state. Also: Guest + pending (re-confirmed at the new head)Unchanged from before the fix — main still (correctly) blocks a genuine guest, the PR still doesn't. Marked core (
|
john-the-dev
left a comment
There was a problem hiding this comment.
Approving at 6bbcfd63b. This resolves keweichen's P1 from fa09fc2b7.
Fix
Both guest exits (src/check-pending-tasks.sh around L64 for the foreign common-dir exit, and the confirmed-absent bundle exit around L73) now key on IDENTIFIED_SESSION. That flag is set when either SUTANDO_INSTANCE_ID or SUTANDO_CORE_SESSION is non-empty. git diff fa09fc2b7 6bbcfd63b shows no other hook change: +8/−4 in the hook, plus tests 8e and 9b.
Regressions fail at the parent
I swapped in git show fa09fc2b7:src/check-pending-tasks.sh and ran the suite with the new tests. Exactly the three new assertions FAIL:
- 8e: blocks
- 8e: payload names the task
- 9b: bundle blocks
The guest and worker controls stay ok. With the head hook restored, a clean env gives 40 ok / PASS.
Test-hygiene issue introduced by this head (non-blocking, worth a one-line fix)
tests/check-pending-tasks-workspace.test.sh inherits SUTANDO_CORE_SESSION from its caller. Every core session has it set: my session has SUTANDO_CORE_SESSION=1. So running the suite from a live core, which is how most reviewing agents run it, gives rc=1 with 6 false FAILs, all guest assertions:
- unrelated-repo worktree
- 8d ordinary guest
- 9 bundle guest
- translated not-a-repo
- inherited GIT_DIR
- inherited GIT_COMMON_DIR
The same run under env -u SUTANDO_CORE_SESSION -u SUTANDO_INSTANCE_ID passes. CI never sets the marker, so CI stays green. This is a false fail, not a false pass, but it will send the next reviewer chasing a regression that does not exist. The fix is an unset SUTANDO_CORE_SESSION SUTANDO_INSTANCE_ID near the top of the test, since 8b/8c/8e/9b already set their markers explicitly. It is the same class as #4652's clean_env().
Live evidence
The registered-session witness refreshed at exactly 6bbcfd63b is on the thread (sonichi, 05:56Z): guest+pending, where main blocks and the PR gives {}; and marked core, where both block. I read it but did not re-run it.
Not independently re-run: the resolver suites (git-binary-*). They are unchanged since keweichen's fa09fc2b7 pass (git diff --stat fa09fc2b7 HEAD touches only the hook and the workspace test).
keweichen
left a comment
There was a problem hiding this comment.
APPROVE at exact head 6bbcfd63bb5c7a4a0b2cc69d25a2e57669b774a1. The previous marked-core blocker is fixed; no implementation blocker remains.
src/check-pending-tasks.sh:18–21 recognizes both core and worker identity, and both guest exits at :64–78 preserve those sessions. I independently exercised the production hook with isolated queues and real linked worktrees:
merge-base/guest/foreign-worktree = block
HEAD/guest/foreign-worktree = {}
parent/marked-core/foreign-worktree = {}
HEAD/marked-core/foreign-worktree = block
HEAD/unmarked-core/same-repo-worktree = block
HEAD/worker/pending = block
HEAD/worker/empty/whitespace/partial-UTF8 = block
HEAD/worker/ready-result = {}
parent/marked-core/armed-turn-gate = {}
HEAD/marked-core/armed-turn-gate = block
The current workspace tests also discriminate against the parent:
HEAD: 40 passed, 0 failed
parent hook + new tests: 37 passed, 3 failed
All three failures are the new marked-core assertions, including the bundle branch. These runs used writable local copies inside this checkout, with a fixture-only Git discovery boundary preventing non-Git fixtures from discovering the outer checkout. The initial nested-fixture run had two ancestry-related failures; the isolated run covered all controls without skips.
Reviewed all six changed files against merge-base 735023eee2737e6d3ad79cd91cfc5b97a0813393, plus the latest commit separately: 33 linear topic commits, no stack. Current origin/main retains the merge-base hook blob. I also independently confirmed the earlier missing-helper and Git-alias fixes.
Checks run:
- Shell Git resolver: 29/29; Python Git resolver: 36/36.
- Worker hook, Stop JSON, Stop-reason, and turn-ledger suites: PASS; task-dispatch contract: 92/92.
- Topic and working-tree
git diff --check, Bash 3.2/5 syntax checks, and repository review checks: PASS. - Added-line path scan: no host-specific production path. A planted violation beside an allowed fixture path correctly failed the scanner.
Non-blocking: independently confirmed the reported test-isolation issue—an inherited SUTANDO_CORE_SESSION=1 produces six false guest-test failures. Clear ambient core/worker markers at suite entry; individual scenarios already set them explicitly. Also trim the three-line comment at tests/check-pending-tasks-workspace.test.sh:393–395 to the two-line cap.
Worst-case disruption is silently abandoning owner work, repeatedly blocking unrelated sessions, or triggering developer-tools dialogs. Explicit session identity, conservative handling of ambiguous probes, readiness checks, and safe Git resolution mitigate those risks in the exercised paths.
Preflight ran without GitHub access; supplied reviews and the empty inline-comment list were inspected. I read the supplied exact-head registered-session witness but did not rerun live sessions. Hosted CI/CLA success is from the supplied snapshot; ShellCheck and Ruff were unavailable locally. The checkout is clean. Ready from implementation review, subject to fresh required CI/CLA and the two-maintainer approval gate.
Reviewed by Kewei's Codex automation.
What
src/check-pending-tasks.sh(the Stop hook) is inherited by every Claude Code session that has this repo as an ancestor — including a session whose actual cwd is a worktree of a genuinely different repo (e.g. a skill-repo fix branch) that merely picked up this repo'sCLAUDE.md. Per CLAUDE.md's "Chat-path task tracking" guest carve-out, such a session is a guest of Sutando, not the core — Sutando's own task/result queue and its per-turn message discipline (turn_ledger.py's stop-gate) are the core's own operating rules, not a universal Claude Code requirement. A guest's Stop should not be held hostage to either.Fix
Compare
git rev-parse --git-common-dirfor the CWD against the same for this hook's own repo (resolved from$0, not cwd). Worktrees of the same repo share one common-dir, so a worktree of this repo is still the core and still gets the gate; only a genuinely different repo's common-dir exits early with{}, before both the task-queue loop and theturn_ledger.pystop-gate call.Evidence
New cases 7-8 in
tests/check-pending-tasks-workspace.test.sh:Mutation-tested (before/after, pasted from this session):
Full suite:
bash tests/check-pending-tasks-workspace.test.sh— all 9 checks pass at HEAD.Stand: Echo Act IV Pro
🤖 Generated with Claude Code