Repository navigation
fix(#5182): watch a GSD-owned return receipt instead of the host transcript in the plan-phase stall watch - #5192
Conversation
… not argv, so Windows MSYS cannot strip the quote Windows conformance shard 1/3 on PR open-gsd#5192 failed the gsd_receipt_path unsafe-phase-dir row: the "ph'ase" argument reached bash as "phase" because Git Bash's MSYS layer re-splits and unescapes argv. The helper was right to accept "phase". Read the value from a file inside the script instead, the same transport hazard extractStallHelpersBash() documents.
trek-e
left a comment
There was a problem hiding this comment.
Review — fix(#5182): watch a GSD-owned return receipt instead of the host transcript in the plan-phase stall watch
Head reviewed: 47b8993 (fresh gh pr view this turn: mergeable=CONFLICTING, mergeStateStatus=DIRTY, reviewDecision=REVIEW_REQUIRED, no reviews/comments).
Summary
Replaces the host {outputFile} grep in gsd_stall_watch with a per-dispatch GSD-owned receipt (gsd_receipt_path, gsd_return_marker), wires all five watch sites, passes revision markers, and adds agent receipt rules. Substance is sound; the branch conflicts with origin/next and the new parser helper has no property test.
Classification & gate compliance
- Issue #5182: OPEN, labels
bug,area: workflow,confirmed-bug-> Bug-Fix track. Body hasFixes #5182; not draft; branchfix/5182-stall-watch-return-receipt; titlefix(#5182):. - Changeset
.changeset/lively-sloths-forage.md: type Fixed, pr: 5192, body format OK. No docs/ requirement (Fixed). - PR label is only
area: workflow(issue carries the gate label; fine). - No competing open PRs for #5182.
- Acceptance criteria (verbatim from trek-e brief): AC1 markers only in prompt/definition/transcript -> waiting (test row AC1); AC2 checker issues -> revision (row AC2); AC3 finished revision -> success within one poll (row AC3, revision site now passes
## REVISION COMPLETE/## REVISION_CONFLICT, plan-phase.md ~L1193); AC4 never-signals -> stalled (row AC4); AC5 identical on Claude Code vs no-output host (row AC5; receipt is host-independent, but see Finding 3 for the read-only checker host); AC6 fail-first (PR body reports 20 failing rows on untouched next; not executed here per brief).
Functional checklist / root-cause verdict
Root cause (one sentence): gsd_stall_watch grepped the whole host transcript, which already contains the prompt/definition quoting the markers, and the revision site passed no markers. Fix addresses the input to the watch (single owner gsd_return_marker used by watch and step 11), not the symptom. gsd_stall_should_recover unchanged. Verified in stall-detection-helpers.md (helpers ~L152-L195) and plan-phase.md steps 8/10/11/12.
Blast radius + risk tier
Shipped-prose + bash-in-markdown change; consumers are the five plan-phase watch sites, two agent definitions, agent-contracts registry. Tier: medium (runtime-loaded workflow/agent text, size-capped files). compact-content-benchmark-baseline.json refreshed for plan-phase growth.
Co-change completeness
No known co-change gap beyond the conflicting baseline fixture. agent-contracts.md updated for ## REVISION COMPLETE. CONTEXT.md has no gsd_stall_watch entry (grep: 0 hits), so no glossary gap is implied.
Memtrace Evidence
Memtrace is over quota (quota_exceeded) per orchestrator brief; get_impact, get_symbol_context, recall_decision, find_code_review_issues were NOT run (no result obtained; this is not a clean result). Evidence came from git diff/git merge-tree and direct reads of the PR head in the worktree. Co-change (get_cochange_context) not run. Changed symbols are bash helpers inside markdown (not graph-indexed), so graph impact would be limited regardless.
Findings
Blocker
- [judgment] Merge conflict with
origin/next(behind by 13):git merge-tree --write-tree --name-onlyexit 1, conflicting pathtests/fixtures/compact-content-benchmark-baseline.json(plan-phase offTokens/onTokens/aggregate at lines ~28-45).nextmovedplan-phase.mdafter the PR's measurement (e.g. 84b4dfe #5171, 7586af3 #5044). Fix:git fetch origin && git rebase origin/next, regenerate the baseline against the rebasedplan-phase.md, re-verify.
Major
2. [judgment] tests/plan-phase-stall-detection.test.cjs (new describe at ~L940-1200): gsd_return_marker is a new parser (line-start literal, word boundary, CR/BOM tolerance) and gsd_receipt_path a new sanitizer; the test file imports fast-check (L49) but the new rows use only example cases. RULESET.TESTS requires at least one discriminating fast-check property for parsers/bijective contracts. Add a property: for arbitrary lines/prefixes, a marker matches iff the line starts with it and the next char is not [A-Za-z0-9_]; and gsd_receipt_path output never escapes .gsd-returns/ for arbitrary spawn labels.
3. [judgment] AC5 / acceptance criterion partially unmet for the checker on read-only hosts: gsd-plan-checker writes its receipt via Bash printf; on a host that runs it read-only (Codex derives read-only from tools:) the write is refused and the checker watch ends stalled (agents/gsd-plan-checker.md ~L957-962; stall-detection-helpers.md "Plan-checker on a read-only host"). The PR discloses this and defers the Group B decision to the maintainer; it is a maintainer decision (must be raised in chat), but until decided the criterion "identical on every host" is not met for the checker.
Minor
4. [judgment] Boundary coverage: AC4 row tests elapsed 9 min and 11 min against a 10 min threshold but not the limit itself (10 min) — add limit case (limit-1/limit/limit+1). Rows derive times from Date.now() (nowSec() helper) rather than a clock seam; offsets are coarse so not flaky, but it is wall-clock-derived input.
Nit
5. [judgment] Several new rows assert prose via assert.match on workflow/agent markdown (e.g. "never counted as 0 issues", rm -f "{receipt}"); these are prose-contract checks, acceptable but brittle to rewording.
Verdict
Changes requested (blocker: merge conflict; major: no property test; maintainer decision pending on checker read-only host). CI: all 36 check-runs for the head are success/skipped, pending approval runs=0 — scoped to the CI result only; mergeStateStatus is DIRTY.
… not argv, so Windows MSYS cannot strip the quote Windows conformance shard 1/3 on PR open-gsd#5192 failed the gsd_receipt_path unsafe-phase-dir row: the "ph'ase" argument reached bash as "phase" because Git Bash's MSYS layer re-splits and unescapes argv. The helper was right to accept "phase". Read the value from a file inside the script instead, the same transport hazard extractStallHelpersBash() documents.
…and make the receipt helpers locale-proof Review follow-up on open-gsd#5192. - Rebased onto origin/next 47b22b8. next grew plan-phase.md to 98,051 LF bytes (open-gsd#5171), so the receipt binding is now stated once at step 7.99 (`{receipt}` = gsd_receipt_path "${PHASE_DIR}" <spawn>) and each watch site carries only its spawn label. Step 11 keeps its original marker_received bullets; the rule that the routing marker comes from gsd_return_marker over {receipt} moves into the helpers doc that 7.99 loads. plan-phase.md is 98,273 LF bytes (cap < 98,300). - gsd_receipt_path and gsd_return_marker spell out their ASCII word characters instead of bracket ranges, so no locale (bash 3.2 has no globasciiranges) can widen them. - gsd_return_marker drops its trailing-CR strip: a CR after the marker is already a non-word character, so the strip changed no result (a mutation of it survived the new property test). - The compact-content benchmark baseline is regenerated against the rebased plan-phase.md (`benchmark-compact-content.cjs --write`), not hand-merged.
…s, AC4 boundary rows and token-level prose asserts Review follow-up on open-gsd#5192. - Property: gsd_return_marker matches iff the line starts with the marker and the next character is not [A-Za-z0-9_], with a leading BOM, a trailing CR and a missing final newline tolerated. Property: gsd_receipt_path never leaves <phase>/.gsd-returns/ for any spawn label, and an unsafe phase dir (quote, $ or backtick) fails closed. Each runs one bash invocation per generated batch, with values passed through files, never argv. - AC4 rows now cover limit-1 / limit / limit+1 (599 / 600 / 601 s) against an injected clock (`date +%s` stubbed); no open-gsd#5182 row reads the wall clock. - Prose-contract asserts are reduced to command tokens (`rm -f "{receipt}"`, `gsd_return_marker "{receipt}"`, `<return_receipt>`, printf) plus one keyword, so rewording the surrounding prose does not break the suite.
47b8993 to
345d13e
Compare
|
Thanks for the review. Addressed at 1. Merge conflict (blocker). Rebased onto The rebase also put 2. Property tests (major). Added two
Each property run is one bash invocation over a whole generated batch. Since Red proof against deliberately broken helpers (each break reverted, file verified byte-identical afterwards):
A fifth break, removing the trailing-CR strip, survived. That was correct: a CR after the marker is already a non-word character, so the strip never changed a result. I removed it as dead code in The same commit spells out the ASCII word characters in both helpers instead of using bracket ranges, so no locale can widen them (bash 3.2 has no 3. Checker on a read-only host (your call). Two ways to close it:
I'd prefer the follow-up. Moving the checker to Group B widens its security posture: on Codex, 4. Threshold boundary (minor). The AC4 rows now cover 5. Prose-contract asserts (nit). Agreed. I removed the asserts that were pure prose ("last action", "only write", "never counted as 0 issues", "nothing persists across fences", the completion-result wording). The rest now anchor on command tokens the orchestrator or agent actually executes: CI on
|
… not argv, so Windows MSYS cannot strip the quote Windows conformance shard 1/3 on PR open-gsd#5192 failed the gsd_receipt_path unsafe-phase-dir row: the "ph'ase" argument reached bash as "phase" because Git Bash's MSYS layer re-splits and unescapes argv. The helper was right to accept "phase". Read the value from a file inside the script instead, the same transport hazard extractStallHelpersBash() documents.
…and make the receipt helpers locale-proof Review follow-up on open-gsd#5192. - Rebased onto origin/next 47b22b8. next grew plan-phase.md to 98,051 LF bytes (open-gsd#5171), so the receipt binding is now stated once at step 7.99 (`{receipt}` = gsd_receipt_path "${PHASE_DIR}" <spawn>) and each watch site carries only its spawn label. Step 11 keeps its original marker_received bullets; the rule that the routing marker comes from gsd_return_marker over {receipt} moves into the helpers doc that 7.99 loads. plan-phase.md is 98,273 LF bytes (cap < 98,300). - gsd_receipt_path and gsd_return_marker spell out their ASCII word characters instead of bracket ranges, so no locale (bash 3.2 has no globasciiranges) can widen them. - gsd_return_marker drops its trailing-CR strip: a CR after the marker is already a non-word character, so the strip changed no result (a mutation of it survived the new property test). - The compact-content benchmark baseline is regenerated against the rebased plan-phase.md (`benchmark-compact-content.cjs --write`), not hand-merged.
57aab6e to
08ddb32
Compare
…s, AC4 boundary rows and token-level prose asserts Review follow-up on open-gsd#5192. - Property: gsd_return_marker matches iff the line starts with the marker and the next character is not [A-Za-z0-9_], with a leading BOM, a trailing CR and a missing final newline tolerated. Property: gsd_receipt_path never leaves <phase>/.gsd-returns/ for any spawn label, and an unsafe phase dir (quote, $ or backtick) fails closed. Each runs one bash invocation per generated batch, with values passed through files, never argv. - AC4 rows now cover limit-1 / limit / limit+1 (599 / 600 / 601 s) against an injected clock (`date +%s` stubbed); no open-gsd#5182 row reads the wall clock. - Prose-contract asserts are reduced to command tokens (`rm -f "{receipt}"`, `gsd_return_marker "{receipt}"`, `<return_receipt>`, printf) plus one keyword, so rewording the surrounding prose does not break the suite.
|
Rebased onto Local on the new head: Finding 3 (move gsd-plan-checker to Group B here, or a follow-up issue) still needs your call; see my 10-03 reply.
|
trek-e
left a comment
There was a problem hiding this comment.
Review — fix(#5182): watch a GSD-owned return receipt instead of the host transcript in the plan-phase stall watch
Head reviewed: 08ddb32 (mergeable=MERGEABLE, mergeStateStatus=BEHIND by 1, reviewDecision=CHANGES_REQUESTED).
Summary
Replaces the host {outputFile} grep in gsd_stall_watch with a per-dispatch GSD-owned receipt (gsd_receipt_path, gsd_return_marker), wires all five watch sites, passes revision markers, and adds agent receipt rules. Prior findings 1, 2, 4 and 5 are fixed at this head. Finding 3 (checker on a read-only host) remains open pending a maintainer decision, and one new Minor test finding is added.
Classification & gate compliance
- #5182: OPEN, labels
bug,area: workflow,confirmed-bug-> bug-fix track.Fixes #5182present, not draft, conventional title, branchfix/5182-stall-watch-return-receipt. - Changeset
.changeset/lively-sloths-forage.md: type Fixed, pr 5192, format OK. Docs exempt (Fixed). CHANGELOG untouched. No new references/ or workflows/ files. Emitted-Drift-Ack-Growthtrailers present for plan-phase.md, gsd-planner.md, gsd-plan-checker.md.- No competing open PR for #5182.
Prior-finding resolution (trek-e, 2026-10-03)
| # | Finding | Status |
|---|---|---|
| 1 Blocker | baseline-fixture merge conflict | Resolved; merge of origin/next and head is clean |
| 2 Major | no fast-check property | Resolved; two oracle-backed properties |
| 3 Major | checker on read-only host / AC5 | OPEN (maintainer decision) |
| 4 Minor | boundary triple, wall clock | Resolved; 599/600/601 s with stubbed date +%s |
| 5 Nit | prose asserts | Resolved |
Functional checklist / root-cause verdict
Root cause: gsd_stall_watch grepped the whole host transcript, which already quotes the markers via prompt/definition, and the revision site passed no markers. The fix changes the watch's input to a GSD-owned file and uses one marker matcher shared with step 11 routing; gsd_stall_should_recover is untouched. Root-cause fix, not symptom handling.
AC1 pass, AC2 pass, AC3 pass, AC4 pass (limit-1/limit/limit+1), AC5 partial (Finding 1), AC6 per PR body (20 failing rows on next; not executed here).
Blast radius
Shipped prose and bash-in-markdown: five plan-phase watch sites, gsd-planner, gsd-plan-checker, agent-contracts registry. Argument 2 of gsd_stall_watch changes meaning (receipt, not host output file); only the five plan-phase sites call it. Risk: medium (runtime-loaded text, size-capped; growth acked by trailers).
Co-change completeness
agent-contracts.md updated for ## REVISION COMPLETE; benchmark baseline regenerated. No missing INVENTORY, CONTEXT or docs requirement identified.
CodeGraph Evidence
codegraph_impacttargetgsd_stall_watch(base index): 2 affected symbols, both in tests/plan-phase-stall-detection.test.cjs (extractStallHelpersBash). Shell helpers inside markdown are not graph-indexed, so this is a lower bound.codegraph_callerstargetgsd_return_marker: symbol not found (new in this PR, absent from base index).codegraph_nodetargetgsd_stall_should_recover: only the test wrapperrunShouldRecoveris indexed; the function body is unchanged by this PR.
Memtrace unavailable: monthly query quota exhausted (20972/20933, resets 2026-10-20)
Findings
Major
- [judgment, carried from prior review #3]
agents/gsd-plan-checker.md:962-966,gsd-core/workflows/plan-phase/steps/stall-detection-helpers.md("Plan-checker on a read-only host"): on a host that runs the checker read-only (Codex derivesread-onlyfromtools:) the receiptprintfis refused and the checker watch endsstalled, so "identical on every host" (AC5) is not met for the checker. The contributor asked twice for a decision (Group B move here vs follow-up issue); it needs a maintainer answer in chat before approval.
Minor
2. [judgment] tests/plan-phase-stall-detection.test.cjs, AC5 row (bind()): once the call site binds {receipt}, both "hosts" receive f.receipt, so the comparison is between identical inputs and cannot fail. Fix: supply a transcript file and a missing file as argument 2 and assert neither is consulted, or remove the row.
Nit
3. [judgment] fast-check numRuns of 10/8/6 is small; acceptable given per-run batching.
4. [judgment] stall-detection-helpers.md ("The runtime's completion result ends the wait"): only checker ## ISSUES FOUND has an explicit fail-closed route when the receipt arrives without a body; planner receipt-only ## CHECKPOINT REACHED / ## PHASE SPLIT RECOMMENDED have no stated step-9 behavior.
Verdict
Request changes (Finding 1 open, Finding 2 Minor). CI at head: 36 check-runs, 32 success / 4 skipped, 0 red, 0 pending-approval runs. Rebase is optional (behind by 1, docs-only chore, clean): git fetch origin next && git rebase origin/next.
… not argv, so Windows MSYS cannot strip the quote Windows conformance shard 1/3 on PR open-gsd#5192 failed the gsd_receipt_path unsafe-phase-dir row: the "ph'ase" argument reached bash as "phase" because Git Bash's MSYS layer re-splits and unescapes argv. The helper was right to accept "phase". Read the value from a file inside the script instead, the same transport hazard extractStallHelpersBash() documents.
…and make the receipt helpers locale-proof Review follow-up on open-gsd#5192. - Rebased onto origin/next 47b22b8. next grew plan-phase.md to 98,051 LF bytes (open-gsd#5171), so the receipt binding is now stated once at step 7.99 (`{receipt}` = gsd_receipt_path "${PHASE_DIR}" <spawn>) and each watch site carries only its spawn label. Step 11 keeps its original marker_received bullets; the rule that the routing marker comes from gsd_return_marker over {receipt} moves into the helpers doc that 7.99 loads. plan-phase.md is 98,273 LF bytes (cap < 98,300). - gsd_receipt_path and gsd_return_marker spell out their ASCII word characters instead of bracket ranges, so no locale (bash 3.2 has no globasciiranges) can widen them. - gsd_return_marker drops its trailing-CR strip: a CR after the marker is already a non-word character, so the strip changed no result (a mutation of it survived the new property test). - The compact-content benchmark baseline is regenerated against the rebased plan-phase.md (`benchmark-compact-content.cjs --write`), not hand-merged.
08ddb32 to
48ee141
Compare
…s, AC4 boundary rows and token-level prose asserts Review follow-up on open-gsd#5192. - Property: gsd_return_marker matches iff the line starts with the marker and the next character is not [A-Za-z0-9_], with a leading BOM, a trailing CR and a missing final newline tolerated. Property: gsd_receipt_path never leaves <phase>/.gsd-returns/ for any spawn label, and an unsafe phase dir (quote, $ or backtick) fails closed. Each runs one bash invocation per generated batch, with values passed through files, never argv. - AC4 rows now cover limit-1 / limit / limit+1 (599 / 600 / 601 s) against an injected clock (`date +%s` stubbed); no open-gsd#5182 row reads the wall clock. - Prose-contract asserts are reduced to command tokens (`rm -f "{receipt}"`, `gsd_return_marker "{receipt}"`, `<return_receipt>`, printf) plus one keyword, so rewording the surrounding prose does not break the suite.
|
@trek-e thanks. I rebased onto
Re-requested your review. Verification (local, head 48ee141)
|
|
Maintainer decision on the open question (review finding 3, AC5): the plan-checker return-receipt write on read-only hosts must be fixed in this PR. AC5 is a must-have acceptance criterion, so it is not deferred to a follow-up and the closing keyword stays. The Major finding remains open until the checker watch works on a read-only host; the Minor and Nits from the latest review also still stand. |
… not argv, so Windows MSYS cannot strip the quote Windows conformance shard 1/3 on PR open-gsd#5192 failed the gsd_receipt_path unsafe-phase-dir row: the "ph'ase" argument reached bash as "phase" because Git Bash's MSYS layer re-splits and unescapes argv. The helper was right to accept "phase". Read the value from a file inside the script instead, the same transport hazard extractStallHelpersBash() documents.
…ost transcript in the plan-phase stall watch
gsd_stall_watch grepped the whole host {outputFile} for the return markers.
On Claude Code that file is the subagent transcript, which already holds the
agent's prompt and definition snapshot (both quote the markers), so the
planner and checker waits reported marker_received at their first check. The
revision watch passed no markers, so it could only end stalled, and a host
with no output file could only ever reach stalled.
Each watched spawn (planner, checker, revision, chunked outline, chunked
per-plan) now gets a per-dispatch receipt from gsd_receipt_path, named in its
prompt as <return_receipt>; the agent writes its marker line there as its last
action. gsd_return_marker, a literal line-start match, is the one matcher used
by the watch and by step 11's checker routing. The revision watch passes
Return Markers and the agent-contracts registry). The checker writes its
receipt with one Bash printf and gains no tool; a refused write is non-fatal.
Emitted-Drift-Ack-Growth: plan-phase.md — the five-site receipt binding, the <return_receipt> prompt lines, the revision markers and receipt-based checker routing (open-gsd#5182); the bytes are the fix itself.
Emitted-Drift-Ack-Growth: gsd-planner.md — the return-receipt rule and the ## REVISION COMPLETE marker declaration (open-gsd#5182); the bytes are the fix itself.
Emitted-Drift-Ack-Growth: gsd-plan-checker.md — the return-receipt rule, written with the Write tool as its only write (open-gsd#5182); the bytes are the fix itself.
… not argv, so Windows MSYS cannot strip the quote Windows conformance shard 1/3 on PR open-gsd#5192 failed the gsd_receipt_path unsafe-phase-dir row: the "ph'ase" argument reached bash as "phase" because Git Bash's MSYS layer re-splits and unescapes argv. The helper was right to accept "phase". Read the value from a file inside the script instead, the same transport hazard extractStallHelpersBash() documents.
…and make the receipt helpers locale-proof Review follow-up on open-gsd#5192. - Rebased onto origin/next 47b22b8. next grew plan-phase.md to 98,051 LF bytes (open-gsd#5171), so the receipt binding is now stated once at step 7.99 (`{receipt}` = gsd_receipt_path "${PHASE_DIR}" <spawn>) and each watch site carries only its spawn label. Step 11 keeps its original marker_received bullets; the rule that the routing marker comes from gsd_return_marker over {receipt} moves into the helpers doc that 7.99 loads. plan-phase.md is 98,273 LF bytes (cap < 98,300). - gsd_receipt_path and gsd_return_marker spell out their ASCII word characters instead of bracket ranges, so no locale (bash 3.2 has no globasciiranges) can widen them. - gsd_return_marker drops its trailing-CR strip: a CR after the marker is already a non-word character, so the strip changed no result (a mutation of it survived the new property test). - The compact-content benchmark baseline is regenerated against the rebased plan-phase.md (`benchmark-compact-content.cjs --write`), not hand-merged.
…s, AC4 boundary rows and token-level prose asserts Review follow-up on open-gsd#5192. - Property: gsd_return_marker matches iff the line starts with the marker and the next character is not [A-Za-z0-9_], with a leading BOM, a trailing CR and a missing final newline tolerated. Property: gsd_receipt_path never leaves <phase>/.gsd-returns/ for any spawn label, and an unsafe phase dir (quote, $ or backtick) fails closed. Each runs one bash invocation per generated batch, with values passed through files, never argv. - AC4 rows now cover limit-1 / limit / limit+1 (599 / 600 / 601 s) against an injected clock (`date +%s` stubbed); no open-gsd#5182 row reads the wall clock. - Prose-contract asserts are reduced to command tokens (`rm -f "{receipt}"`, `gsd_return_marker "{receipt}"`, `<return_receipt>`, printf) plus one keyword, so rewording the surrounding prose does not break the suite.
… functions only, without the per-call Node preamble Every open-gsd#5182 row ran the whole helpers fence per bash call, including the runtime-locator preamble and two `gsd_run query config-get` Node spawns. On Windows CI shard 1/3 (run 37148401740) that put the open-gsd#5182 block at 169.5 s inside a concurrent chunk, and a sibling file (execute-phase-decimal-arithmetic) then hit `spawnSync bash ETIMEDOUT`. The open-gsd#5182 rows now slice the fence from gsd_stall_should_recover() on (extractStallFunctionsBash), so each call is one bare bash process running the same shipped functions. Config resolution stays covered by the open-gsd#2650 rows, which still run the full fence. Locally the block drops from about 16 s to 2.1 s. Both properties still fail against the four deliberate helper breaks.
… runs with identical inputs
Once every call site bound {receipt}, the row's bind() gave both "hosts"
the same receipt, so its comparison could not fail (review 5436473886,
Minor 2). The row now gives argument 2 each host's output, a transcript
that contains the checker marker and a missing file, and expects waiting
before the threshold, stalled past it, and marker_received only from the
receipt. The call-site binding stays with the "AC1 wiring" row. A watch
that ignores argument 2 is now caught by this row alone.
… body never arrives fails closed to 9a The receipt carries only the marker. A receipt-routed CHECKPOINT REACHED, PHASE SPLIT RECOMMENDED or Source Audit whose body has not arrived by the next cycle now goes to 9a, naming the marker received and offering Retry planner before Accept, instead of an empty checkpoint, 9b or 9c (review 5436473886, Nit 4). This matches the checker's ISSUES FOUND rule.
…its return receipt is written on read-only hosts
The checker wrote its stall-watch receipt with a Bash printf because it had
no Write tool. Codex derives a read-only sandbox from that tools: line and
refused the write, so a passing checker's watch ended stalled at the
threshold and AC5 ("identical on every host") did not hold for it.
The checker now declares Write, like the other report-writers (gsd-verifier,
gsd-doc-verifier, gsd-eval-auditor, gsd-ui-auditor): Claude Code denies it
only Edit and MultiEdit, Codex derives workspace-write, and its definition
states the receipt is its only write. Maintainer decision on review
5436473886 Finding 3 (issuecomment-6036744404): fixed in this PR.
The write limit is also stated unconditionally in <anti_patterns>, because
the quick, quick-batch and import spawns send no receipt. As a Write-capable
agent the checker now carries the commented hooks block and the
anti-heredoc instruction (tests/agent-frontmatter.test.cjs). The growth is
covered by this PR's existing Emitted-Drift-Ack-Growth key for
gsd-plan-checker.md (the receipt rule).
…e receipt read gsd_receipt_path ran mkdir -p on <phase>/.gsd-returns and wrote its .gitignore without checking for symlinks, so a checkout that commits either one as a link sent the directory creation and the '*' write outside the phase. It now fails closed (prints nothing, returns 1) when .gsd-returns or .gsd-returns/.gitignore is a symlink, checked before each write. New rows cover a linked directory, a linked .gitignore and a dangling .gitignore link; the link targets stay untouched. gsd_return_marker read the agent-written receipt with no limit. It now reads at most the first 64 lines through an in-loop counter (no extra process per call), keeping the BOM, missing-final-newline and first-match behavior. A new row shows a marker on line 1 or 64 is found and one only on line 65 is not. Answers review Minor 1 (symlinked .gsd-returns) and Nit 5 (unbounded receipt read).
…atch helpers kimi-code dispatches -planner to its plan built-in and -checker to explore, and neither can write files (runtime-aware-dispatch.md), so no return receipt is written there. The helpers doc now says so beside the plan-checker receipt rule: the wait ends on the runtime's completion result, or as stalled at the threshold. Answers review Finding 3 (kimi-code known limit).
…h and harden the receipt read gsd_receipt_path checked for a symlink only at .gsd-returns and its .gitignore, so a checkout that commits the phase dir, .planning or .planning/phases as a link sent mkdir -p and the '*' .gitignore write outside the project, and the printed receipt path told the planner and checker to Write there. It now walks from PHASE_DIR up to its nearest existing ancestor, refuses any walked component that is a link (tested before -e, so a dangling one counts), resolves that ancestor with cd -P/pwd -P and requires it to sit under the orchestrator's pwd -P, then re-verifies the created .gsd-returns physically after mkdir. Both sides go through pwd -P, so the macOS /var -> /private/var temp link and Git Bash's C:/ -> /c/ mapping compare equal. A refused receipt prints nothing and returns 1; the watch then ends on the runtime's completion result or as stalled. Only static, checked-in links are in scope (no check-then-act race guard). It also refuses a PHASE_DIR holding any control character (newline, tab, CR, ESC, DEL) or < or >, which would land in the <return_receipt> prompt line. gsd_return_marker now ignores a receipt that is a symlink, reads at most the first 4096 bytes (head -c) as well as the first 64 lines, so one huge line no longer costs tens of seconds per watch cycle (1,000,000 bytes took 35 s on bash 5.2 and 73-100 s on bash 3.2 before; 0 s now), and counts a marker only when it is followed by the end of the line, a space, tab, CR, VT, FF or a colon, so "## PLAN COMPLETE-x" is not "## PLAN COMPLETE". The colon is kept on purpose: the planner returns its Source Audit heading with ": Unplanned Items Found" after the marker the watch passes. gsd_stall_watch passed its artifact glob to find unquoted, so a space in PHASE_DIR split it and a fresh *-PLAN.md read as stalled. The directory part is now quoted and only the basename is globbed, under nullglob (restored after) with IFS empty; no match gives an empty list and artifact_fresh false. The call contract is unchanged. Tests: rows for a symlinked phase dir (relative, absolute, a child of it), a phase dir under a .planning linked outside the project, dangling .gsd-returns and phase-dir links, a symlinked receipt, the 4096-byte cap and a 1,000,000-byte line under the 15 s probe bound, the marker boundary (-x, ., a letter, space, tab, CR, colon) and a spaced phase dir with a fresh plan. The unsafe-dir row and property now read values with read -d '' and cover newline, tab, CR, ESC, DEL, < and >. The receipt-path property no longer exempts every non-ASCII label from the stem check (!ascii ||): ASCII labels must match exactly and non-ASCII labels match with runs of _ collapsed, since bash replaces per character in a UTF-8 locale and per byte in C. Looping rows assert their executed count. Answers review 5477738387 Major 1, Minor 1, Minor 2, Minor 4, Minor 6, Nit 1 and Nit 3.
…the no-receipt outcome
The helper doc said the orchestrator removes the receipt "after routing",
but no watch site carried the command, so stalled and Retry paths left
orphan .gsd-returns files. Each of the five watch rules (planner, checker,
revision, chunked outline, chunked per-plan) now ends with
`rm -f "{receipt}"` after routing any result, stalled and Retry included.
The helper doc now states the other half of Minor 3: a checker with no
receipt touches no *-PLAN.md, and neither does a planner before its first
PLAN.md, so that watch can only end on the completion result or as
stalled at the threshold. There is no heartbeat protocol.
Tests: a row per watch site asserting the cleanup clause follows its
gsd_stall_watch call (5 sites, count asserted), a doc row for the
no-receipt outcome, and a *-PLAN.md freshness row just inside and just
outside the interval. The two wiring rows fail on the previous docs.
Also: Write added to the plan-checker row of docs/ja-JP and docs/ko-KR
AGENTS.md (matching docs/AGENTS.md), the changeset states that no receipt
is created for a symlinked or control-character phase path, and the
compact-content baseline is refreshed.
Answers review 5477738387 Minor 3, Minor 5 (remaining rows), Nit 4 and
Nit 6.
The plan-phase.md growth is covered by this PR's existing
Emitted-Drift-Ack-Growth key for plan-phase.md (a second key with a
different reason would be an ambiguous duplicate).
…t is refused gsd_receipt_path now fails closed on an unsafe or symlinked phase dir, but the helpers doc did not say what the orchestrator does then. It now says: omit the <return_receipt> line and pass "" as the watch's second argument, so the wait ends on the runtime's completion result or as stalled. A test pins the doc line and that an empty receipt reads waiting before the threshold and stalled after it, never a match. The changeset's issue backlink moves to the end of the entry (review 5477738387 Nit 6).
…nd bind the receipt under either toggle
A `..` after a phase-dir component that does not exist yet passed the
nearest-existing-ancestor check (the project itself), so mkdir -p resolved
it and created `.gsd-returns` outside the project before the post-mkdir
check refused the path. gsd_receipt_path now refuses any `..` component
with the other unsafe-input checks, before touching the filesystem; a row
covers the relative, absolute and existing-ancestor forms and asserts
nothing is created outside.
The helpers doc bound `{receipt}` only "before each watched Agent() call"
while also saying the toggle-false prompt still carries the receipt line;
it now binds before every call that carries the line, under either toggle.
The ja-JP and ko-KR plan-checker tools rows gain Write, as the English row
did.
…ect link rows Every existing symlink row links outside the project, so the physical containment check alone refuses it; deleting the walk's `[ -L "$a" ]` test left the file at 77/77. A link that stays inside the project is refused only by that test. The new row covers a phase dir that is itself an in-project link and a walked link back to the project followed by a missing child, and asserts nothing is created through either. It also accepts an existing phase dir reached through an in-project parent link, which pins the scope of the claim. RED on the mutant, GREEN on the head.
…eceipt helper checks The changeset said no receipt is created when the phase directory or one of its parents is a symlink. A parent link that stays inside the project is accepted: the helper refuses a linked phase dir, a link on the walked missing components, and any path that resolves outside the project. The sentence now says that, and names the `..` refusal.
…l edit on OpenCode and Kilo
…at grant Write and Edit as one permission OpenCode, Kilo and Copilot map Write and Edit to the same `edit` permission, so the checker's one-file receipt grant became full edit there. The agent staging pipeline (ADR-1235 descriptor boundary) now removes Write from a receipt-only agent's tools on a host whose own map merges the two, before the user's agent_tools grants are appended. The checker keeps its read-only posture on those hosts and writes no receipt; the watch ends on the completion result or as stalled, as on kimi-code. Claude, Codex and the hosts with a separate write tool keep the receipt Write. Install rows pin edit denied for the checker on all three hosts, with gsd-verifier as the control that still holds edit, and a configured agent_tools Write grant kept. The host set is derived from the converters' maps and pinned against the capability registry.
…t, and name every refusal on stderr gsd_receipt_path took the orchestrator's `pwd -P` as the project root, so an absolute phase dir under the project but outside the cwd (a subdirectory cwd, a GSD_PROJECT workspace) silently lost its receipt. It now takes an optional third argument, init's `project_root`, which every bind site passes (plan-phase parses it as PROJECT_ROOT); with no argument it keeps the cwd. Each refusal prints one `gsd_receipt_path: no receipt: <reason>` line on stderr, never the phase dir itself. A `.` component is now refused like `..`. Tests: a row with an absolute phase dir under the root and a cwd outside it (accepted with the root, refused with a reason without it); a row that every refusal names a reason; a fast-check property over paths built from `a`, `.`, `..` and `link` segments with an oracle for the refusal set; symlinkOrSkip may skip only off CI, since the Windows CI shard that ran this file at 35bf07e created these links.
…tead of writing an empty conflict record Step 12 exports the conflict record's CONFLICT_* fields from the revision's return body, which the receipt does not carry. When the receipt arrives first, the conflict now joins the other body-needing routes in the fail-closed list: the revision watch's stalled menu, naming the marker, with Retry before Accept. A row pins all five body-needing markers in that list and that the revision watch passes the conflict marker.
… strip a BOM only on line 1 gsd_return_marker cut the receipt at 4096 bytes, so a longer word whose extra byte fell at 4097 (`## PLAN COMPLETEX`) was read as the marker. It now reads 4097 bytes: a marker must end within the first 4096 and its boundary must be seen, so a line cut at the read window never ends on a marker. The BOM strip ran on every line; it now runs on line 1 only, as the contract says. Rows: marker ending at byte 4095 / 4096 / 4097 -> found / found / not found, the same at end of file with no newline, a word cut at the cap (not found) with a real-boundary control, and a BOM on line 2 (not stripped) with a control. The cap row asserts what is read, not how long the read took.
… drop the asserts that pin wording The *-PLAN.md freshness row set mtimes from Date.now() and relied on find's real clock. A find shell function now logs its argv and reports a controlled age, so the rows pin what gsd_stall_watch hands find (the expanded plan path and -mmin -INTERVAL) and how it reads the answer: 4m59s active and 5m00s waiting at a 5-minute interval, 5m00s active at 7, and no find call when no plan matches. Real find's -mmin parsing stays covered by the open-gsd#2650 row. Prose asserts that pinned wording rather than behavior are gone (`the Write tool`, `only write`, `Group B report-writer`, `refuses that write`, `no heartbeat`, `After routing, the orchestrator runs`); the checker's per-host posture is pinned on installed artifacts instead.
…e after the PROJECT_ROOT bind plan-phase.md grew by the project-root argument at the receipt bind and the project_root parse; regenerated with node scripts/benchmark-compact-content.cjs --write.
… and checker receipt rules npm run gen:pi-package-skills after the rebase onto next, which added the generated pi/package-agents/ tree (ADR-5559). Pi maps Write to its own write tool, separate from edit, so the checker keeps write there as on Claude Code.
… each host's agent converter emits A host merges Write into edit iff its descriptor agent converter turns `Read, Write` and `Read, Edit` into the same output while differing from `Read` alone; the row now derives that set from every converter in the capability registry and requires the scoping table to equal it, so a host added later that merges the two cannot widen the checker unnoticed. The checker's no-Write-tool sentence no longer names hosts, so it cannot go stale against the table; Pi roles regenerated.
…installs too, with one install per host and scope The host row now runs local and global installs for OpenCode, Kilo and Copilot, reads only the host's own agents/ directory (a global install also ships the raw source agents under <config>/gsd-core/agents/, which no host loads as an agent), and reads the gsd-verifier control from the same install. The symlinkOrSkip comment now names the one Windows shard that ran this file.
4fa5ef7 to
aff15ba
Compare
|
@trek-e rebased onto
Finding status at aff15ba, and the Self-test numbers
Local runs on
|
Fix PR
Linked Issue
Fixes #5182
The linked issue carries
confirmed-bug. This PR follows @trek-e's diagnosis and Agent Brief on the issue (issuecomment-5963253983); its six acceptance criteria are mapped below. Out of scope, per the brief: #2699 (general subagent watchdog) and #3923 (threshold default).What was broken
With the default
planner.stall_detection_enabled: true,/gsd:plan-phasedecides that a background planner or plan-checker has finished by grepping the host's{outputFile}for the return markers. On Claude Code that file is the subagent transcript. The transcript already holds the agent's prompt and its definition snapshot, and both quote the markers, so the wait reportedmarker_receivedat its first check whether or not the agent had returned. The revision watch passed no markers, so every revision round could only end in the "Accept as revised / Retry / Stop" stall menu. A host that returns no output file could only ever reachstalled.What this fix does
Each watched spawn now gets a GSD-owned, per-dispatch return receipt, and the watch reads only that file:
gsd_receipt_path "${PHASE_DIR}" <spawn> "${PROJECT_ROOT}"prints a fresh absolute path,<phase>/.gsd-returns/<spawn>.<epoch>.XXXXXXXX. It creates the directory with a catch-all.gitignorebut not the file.PROJECT_ROOTis init'sproject_root; with no third argument the orchestrator'spwd -Pis used.pwd -P, under that root, and no walked component,.gsd-returnsor its.gitignoreis a symlink..or..component, and on a quote,$, a backtick,<,>or any control character in the phase path.gsd_receipt_path: no receipt: <reason>line on stderr.{receipt}in the spawn prompt (<return_receipt>{receipt}</return_receipt>) and in the watch call.gsd_return_marker FILE MARKER...is the single owner of "which marker did the agent return":gsd_stall_watchuses it formarker_found, and step 11 routes the checker on it, so the watch and the router cannot disagree. Prompt, definition and JSONL transcript text never match it.## REVISION COMPLETEand## REVISION_CONFLICT, so it has a success exit.## REVISION COMPLETEwas already emitted (planner-revision.mdStep 7). It is now declared in gsd-planner's Return Markers and in theagent-contracts.mdregistry.## ISSUES FOUNDgoes to 11a.## REVISION_CONFLICTgoes to the revision stalled menu, with Retry offered first.$TSlost between tool calls falls back to the epoch stamped into the receipt name.All five watch sites get the receipt, and each rule removes it with
rm -f "{receipt}"after routing any result, stalled and Retry included. Three are theplan-phase.mdsteps the brief names (8, 10, 12). The other two are the chunked outline and per-plan sites insteps/chunked-planning-mode.md.Root cause
gsd_stall_watchrangrep -qFover the whole host output file. On Claude Code that file contains the agent's inputs as well as its output. The revision call site passed no markers, so themarker_receivedexit ingsd_stall_should_recoverwas unreachable for it. Introduced by 067a4d1 (#3015).gsd_stall_should_recoveritself is unchanged on purpose.Checker posture (maintainer decision, issuecomment-6036744404; review 5563209291 Major 1)
gsd-plan-checker moves from #767 Group A to Group B. Its
tools:gainsWritefor the receipt, and its ClaudedisallowedToolsbecomesEdit, MultiEdit. Codex derivesworkspace-writefrom itstools:. The receipt rule and<anti_patterns>both say that the receipt is the checker's only write.Scoped per host. OpenCode, Kilo and Copilot grant Write and Edit as one
editpermission. There the installer drops the checker'sWritebefore conversion (scopeReceiptOnlyWrite, at the ADR-1235 descriptor boundary), so the checker keeps its read-only posture, edit stays denied, and no checker receipt is written. AWritethe user grants inagent_toolsis appended afterwards and kept. A test derives the set of merging hosts from every descriptor agent converter's real output and requires the table to equal it.Hosts that keep a separate write tool keep the checker's
Write: Claude Code, Antigravity (write_file), Pi (write), Hermes, ZCode, Qwen, and Kimi CLI (WriteFile). Augment, Cline, CodeBuddy, Cursor, Trae and Windsurf emit no tool restriction at all, so they are unchanged. On kimi-code the planner and checker map to built-ins that cannot write, so no receipt is written there.Security posture, stated plainly: on the hosts that keep
Write, the "only write" limit is prose, as it is for the other Group B agents. The checker already holdsBashon every host, so where there is no sandbox this grant does not change what it can reach. Codex's sandbox is the one real barrier, and the maintainer accepted it on 10-10.Testing
How I verified the fix
Rows in
tests/plan-phase-stall-detection.test.cjsrun the shipped bash withsleep,date +%sand (for the freshness window)findstubbed, and read each call site's markers fromplan-phase.md:AC1(+ wiring row: 5 sites bind{receipt}, none binds{outputFile})AC2(+ fail-closed rows for a body-less ISSUES FOUND and REVISION_CONFLICT)AC3AC4599/600/601 s + lost-$TSepoch rowAC5nextfirstPlus the following:
gsd_return_markerrows: byte cap at 4095/4096/4097, a word cut at the cap, BOM on line 2 only, word boundary, symlinked receipt.gsd_receipt_pathrows: project-root anchoring from a cwd outside the root, a stderr reason on every refusal, symlinked phase dir, ancestor,.gsd-returnsand.gitignore,./.., and unsafe characters.tests/agent-tools.install.test.cjs: per-host install rows (local and global) for the checker's edit posture, a gsd-verifier control, theagent_toolsgrant, and the converter-derived host set.Red first: run against the untouched
nextsource, the stall-detection test file failed its AC1 and AC5 rows withmarker_receivedwherewaitingwas expected. This is the issue's mechanism. The byte-cap straddle row and the line-2 BOM row each fail against the previous head's helper. Re-measured for the Self-test report below: with only this head's test file onnexte61dbb9 (also the merge base), 38 of the file's 102 tests fail, AC1 and AC5 among them; on aff15ba all 102 pass.Green on HEAD_SHA = aff15ba (
env -i, emptyHOME/GSD_HOME):nexte61dbb9 for a real conflict: the compact-content benchmark baseline fixture (tests/fixtures/compact-content-benchmark-baseline.json). The baseline is regenerated withnode scripts/benchmark-compact-content.cjs --writeat each commit that changed it, not hand-merged.git range-diffshows 23 of 27 commits identical; the other 4 differ only in that fixture's numbers. The stall-detection test file now has 102 tests.plan-phase-stall-detection,agent-descriptor-parity.install,agent-tools.install,codex-config-agents,codex-config,install,benchmark-compact-content,agent-size-budget,chunked-planning-parallel,copilot-install,pi-package,workflow-size-budget,workflow-sizeandopencode-permissions), withHOMEandGSD_HOMEempty andCODEX_HOMEandOPENCODE_CONFIG_DIRunset: tests 1349, pass 1349, fail 0. In this round's first attempt, without unsettingCODEX_HOME, the run failed atcopilot-install.test.cjs:159withactual: '/home/dave/.codex'; that is an environment leak, not a PR defect.GSD_EMITTED_BASE=origin/next node --testoveremitted-attribution,emitted-caps-gate,emitted-caps,emitted-sizesandemitted-ack-trailer: tests 229, pass 229, fail 0, againstorigin/nexte61dbb9.npm run lint:ci,lint-docs-requiredandchangeset/lintall exit 0.npm teston this head: 46559 passed, 0 failed, 108 skipped, 1 todo (29 chunks, exit 0), in a standalonegit clone --sharedcheckout withenv -i, emptyHOME/GSD_HOME, a real copy ofnode_modulesandorigin/nextat e61dbb9.Regression test added?
Platforms tested
Locally: Linux (WSL2), bash 5.x. CI runs the rows on macOS and Windows. At the previous head, the Windows shard that ran this file ran every symlink row with no skip, and the symlink rows now fail rather than skip on CI.
Runtimes tested
Self-test report
Bound to aff15ba. The base row runs this head's test file on
nexte61dbb9, which is also the merge base;git status --shortafter that checkout lists only the test file.$SCRATCHis a local scratch directory (the full-suite clone must sit outside the system tmpdir: helpers-cleanup refuses to run under it),$WORKTREEthe PR worktree at the head, and$DEPS_CHECKOUTa checkout with installednode_modules.{ "schema": 1, "head_sha": "aff15bafbeb67943a7f3639f1a5ed7c7a7c45e7c", "tool": "npm-test", "failing_first": { "test": "tests/plan-phase-stall-detection.test.cjs", "fails_on_base": true, "passes_on_head": true }, "commands": [ "git clone --shared --no-checkout \"$WORKTREE\" \"$SCRATCH/base-clone\" && cd \"$SCRATCH/base-clone\" && git checkout --detach e61dbb94817573fb13def0abc112441e498ef39a && ln -s \"$DEPS_CHECKOUT/node_modules\" node_modules && rm -f tsconfig.build.tsbuildinfo && npm run build:lib && git checkout aff15bafbeb67943a7f3639f1a5ed7c7a7c45e7c -- tests/plan-phase-stall-detection.test.cjs && git status --short # prints only: M tests/plan-phase-stall-detection.test.cjs", "H=$(mktemp -d -p \"$SCRATCH\"); env -i PATH=\"$PATH\" HOME=\"$H\" GSD_HOME=\"$H\" node --test tests/plan-phase-stall-detection.test.cjs # in base-clone", "cd \"$WORKTREE\" && rm -f tsconfig.build.tsbuildinfo && npm run build:lib && H=$(mktemp -d -p \"$SCRATCH\"); env -i PATH=\"$PATH\" HOME=\"$H\" GSD_HOME=\"$H\" node --test tests/plan-phase-stall-detection.test.cjs", "git clone --shared --no-checkout \"$WORKTREE\" \"$SCRATCH/selftest-clone\" && cd \"$SCRATCH/selftest-clone\" && git checkout --detach aff15bafbeb67943a7f3639f1a5ed7c7a7c45e7c && git fetch \"$WORKTREE\" +refs/remotes/origin/next:refs/remotes/origin/next # origin/next = e61dbb948", "cp -a \"$DEPS_CHECKOUT/node_modules\" . && rm -f tsconfig.build.tsbuildinfo && npm run build:lib", "H=$(mktemp -d -p \"$SCRATCH\"); G=$(mktemp -d -p \"$SCRATCH\"); env -i PATH=\"$PATH\" HOME=\"$H\" GSD_HOME=\"$G\" npm test" ], "full_suite": { "command": "H=$(mktemp -d -p \"$SCRATCH\"); G=$(mktemp -d -p \"$SCRATCH\"); env -i PATH=\"$PATH\" HOME=\"$H\" GSD_HOME=\"$G\" npm test", "result": "passed", "passed": 46559, "failed": 0 }, "environment": { "os": "Linux 6.18.54.1-microsoft-standard-WSL2 (WSL2, Ubuntu 24.04.4 LTS)", "node": "v24.13.0" }, "output_excerpt": "base: next e61dbb948 (also the merge base) plus only the head's tests/plan-phase-stall-detection.test.cjs; git status --short lists only that file. tests 102, pass 64, fail 38, exit 1. AC1 and AC5 fail (marker_received where waiting is expected)\nhead aff15bafb: tests 102, pass 102, fail 0, exit 0\nfull suite (standalone clone at head, origin/next = e61dbb948, env -i): 29 chunks, tests 46668, pass 46559, fail 0, cancelled 0, skipped 108, todo 1, exit 0" }Checklist
Fixes #5182confirmed-buglabelnpm test): locally 46559 passed, 0 failed on aff15ba in a clean clone (Self-test report above); CI re-runs on this head.changeset/fragment added (type: Fixed,pr: 5192)Breaking changes
There is no API or output-format change:
gsd_stall_watchstill prints one ofmarker_received | active | waiting | stalled. The behavior changes, all disclosed in the issue and this PR:gsd_stall_watchis now the receipt, and its only callers are the five plan-phase sites.gsd_receipt_pathtakes an optional third argument, the project root.ADRs consulted:
runtime === ...branch in workflow prose. The installer's host set is derived from the converters' own maps.disallowedToolsresolution.gsd_return_markeris the single marker matcher for the watch and the checker routing.writetool.plan-phase.mdis 97,131 bytes, under the 98,304-byte XL cap, and the agents stay under their tier caps.