* test(#2650): add failing-first regression for plan-phase stall detection Regression test for gsd_stall_should_recover / gsd_stall_watch and the planner.stall_* config keys, none of which exist yet — proves RED before the fix lands in the next commit. * fix(#2650): bound and auto-recover plan-phase planner/plan-checker stalls Mirrors the already-shipped executor.stall_* pattern (execute-phase.md, bug #3212) but with a dispatch change the executor's prose-only surveillance lacks: the standard planner spawn, chunked-outline planner spawn, chunked-per-plan planner spawn, plan-checker spawn, and revision-loop planner respawn now dispatch with run_in_background=true and are followed by a real, bounded bash poll (gsd_stall_watch) that returns control to the orchestrator on its own schedule instead of waiting indefinitely on a subagent that may never return. On stall, the existing accept-plans/retry/ stop recovery menu (9a/11a) is auto-surfaced instead of requiring a manual interrupt. New config keys planner.stall_detect_interval_minutes (default 5) / planner.stall_threshold_minutes (default 10) mirror executor.stall_*. The helper functions (gsd_stall_should_recover, gsd_stall_watch) live in a new lazily-loaded gsd-core/workflows/plan-phase/steps/stall-detection- helpers.md rather than inline, and per-site prose is kept minimal, because plan-phase.md is frozen under the ADR-857 Phase 6 PRE_PHASE6 gate (tests/phase6-capstone-conformance.test.cjs) with ~36 bytes of headroom at baseline; the net effect is plan-phase.md.md ships slightly SMALLER than before (the old unconditional-wait ORCHESTRATOR RULE sentences are gone at the five touched sites, superseded by the bounded watcher). Also fixes a stale doc comment in tests/workflow-size-budget.test.cjs that still described the per-file workflow-size-baseline.json guard removed by #2724 (ADR-2719 Phase 4) as if it were still the enforcement mechanism — discovered while verifying this fix's own byte budget. Researcher and pattern-mapper spawns are untouched (out of scope per the issue's Agent Brief). * fix(#2650): make gsd_stall_watch single-cycle; harden numeric config inputs Two review findings addressed on top of the prior commit: 1. gsd_stall_watch previously looped internally for the full threshold+interval duration inside ONE Bash tool call (up to 15 min at defaults) — a single call blocking that long risks the host tool's own timeout killing it before it ever prints a result, silently defeating the fix. Redesigned to a single sleep-and-check cycle per call, taking an explicit dispatch_ts so the orchestrator prose can repeat the (short, default 5 min) call until it resolves; the outer threshold is now enforced by dispatch_ts accumulating across calls, not by one call's duration. Documented the resulting trade-off (up to one interval of added latency on the success path) in the changeset and reference doc. 2. PLANNER_STALL_INTERVAL_MINUTES/THRESHOLD_MINUTES are config-controlled values that flow into bash arithmetic ($(( ))). A review flagged this as command injection; empirically verified against both macOS bash 3.2.57 and Docker bash:5 that this is NOT actually exploitable (bash hard-errors on a `$(cmd)`-shaped arithmetic operand rather than invoking it) — but an unvalidated malformed value WOULD abort the stall-watcher itself with that bash error, silently defeating the exact hang-recovery this issue ships. Added integer validation with safe-default fallback, both at the config-resolution point and defensively inside gsd_stall_should_recover. Also adds the previously-missing integration coverage for gsd_stall_watch's real execution (grep/find/date plumbing), not just the pure classifier. * fix(#2650): correct AC2 self-test — helpers doc may name teams-status in prose The AC2 regression test asserted the stall-detection-helpers.md step file never contains the substring "teams-status" at all, but the file's own prose explicitly documents its independence from that guard (containing the word by design). Narrowed the assertion to what actually matters: no second `query teams-status` call site and no gating on it, not a blanket absence of the word. * test(#2650): regenerate golden install-tree fixtures for the new step file gsd-core/workflows/plan-phase/steps/stall-detection-helpers.md is an emitted file (installed for every runtime), so adding it changes the install tree even though it is invisible to docs/INVENTORY.md and docs/INVENTORY-MANIFEST.json (both explicitly scope to non-recursive gsd-core/workflows/*.md — verified against the execute-phase #2930 and pre-existing plan-phase step-file precedent, which are equally absent from both inventory artifacts). The golden install tree snapshots the sorted list of emitted relative paths per runtime, so a file invisible to the inventory is still visible here. Regenerated via `npm run gen:install-tree` — one line added per runtime fixture (19 files), no other drift. * fix(#2650): restore 7 ORCHESTRATOR RULE labels; sync runtime-launcher preamble Two more consequences of extracting helper bodies out of plan-phase.md, both caught by verification (0017e1a78, 9 unique failures): 1. tests/plan-phase-drift-guard.test.cjs (#913) requires at least 7 "ORCHESTRATOR RULE — ALL RUNTIMES" labels in plan-phase.md itself, one per agent spawn site. Moving the full explanatory blocks to plan-phase/steps/stall-detection-helpers.md carried 5 of the 7 labels out with them (only the untouched researcher/pattern-mapper sites kept theirs). Restored a short label at each of the 5 stall-watch sites, trimmed a few more redundant words ("Per 7.99, " — already established by the adjacent step-7.99 pointer) to stay under the frozen PRE_PHASE6 cap (94497 bytes, 21 bytes headroom). 2. tests/runtime-launcher-parity.test.cjs (#373) requires exactly one canonical gsd_run preamble, byte-equal to gsd-core/workflows/_runtime-launcher.snippet.sh, before the first gsd_run call in any workflow .md that calls it (recursive scan under gsd-core/workflows/, unlike the non-recursive inventory/step-tag-balance checks). The new step file's config-get calls use gsd_run without one. Fixed via `node scripts/sync-runtime-launcher.cjs`, verified: exactly 1 preamble occurrence, before the first call, including the .claude/ and .codex/ home fallback arms. Also verified (no fix needed, evidence recorded): the generic `gsd-core-verbatim` identity rule in tests/helpers/emitted-provenance.cjs (roots: ['gsd-core'], pattern matching workflows/.+) self-attributes any new gsd-core/workflows/** path to itself, so the new step file needs no drift-ack entry — consistent with plan-phase.md's own net shrinkage requiring none either. * test(#2650): acknowledge plan-phase.md's +14 byte drift Restoring the 5 ORCHESTRATOR RULE — ALL RUNTIMES labels (#913) flipped plan-phase.md from -142 bytes (post-extraction) to +14 bytes net growth against baseline (94483 -> 94497), which the differential attribution size ratchet (tests/emitted-attribution.test.cjs) correctly flags as unacknowledged growth. Added tests/emitted-drift-acks/2650-plan-phase- stall-detection.json, keyed on the bare filename plan-phase.md per the existing fragment schema (see tests/emitted-drift-acks/2649-diagnose- execute-plan-base-check.json), explaining the growth as exactly the 5 restored labels — still verified under the PRE_PHASE6 cap (94497 < 94519) and satisfying #913's 7-label requirement. * fix(#2650): bind {outputFile} from the real Agent() return — was dead code Independent review blocker: PLANNER_OUTPUT_FILE/CHECKER_OUTPUT_FILE were read by every gsd_stall_watch call but never assigned anywhere in the diff. With the variable permanently empty, `[ -f "$output_file" ]` was always false, marker_found could never become true, and marker_received was unreachable — the marker-based detection path was permanently dead. Worse for the plan-checker spawn specifically: a checker that PASSES touches no *-PLAN.md files, so it had no working completion signal at all without the marker path. A healthy plan-checker finishing cleanly in two minutes would be declared stalled once planner.stall_threshold_minutes elapsed and the recovery menu would fire on an already-succeeded agent — worse than the original unbounded hang. Fixed by replacing the dead bash variable with the `{outputFile}` orchestrator-substitution token, the same convention docs-update.md:471 already uses for a real run_in_background=true Agent() return ("Read tool: file_path: `{outputFile from README agent result}`"). This is a net BYTE SAVING at each site (`"{outputFile}"` is shorter than `"$PLANNER_OUTPUT_FILE"`), which funded moving the full binding explanation — including why plan-checker's *-PLAN.md glob alone is not a working completion signal — into the lazily-loaded reference file to stay under the frozen PRE_PHASE6 cap (94496 bytes, 22 headroom; net +13 over baseline, acknowledged in tests/emitted-drift-acks/2650-plan-phase- stall-detection.json). Added a regression test asserting plan-phase.md itself binds {outputFile} at all 5 spawn sites and contains no dangling $PLANNER_OUTPUT_FILE / $CHECKER_OUTPUT_FILE reference — the previous test suite only exercised gsd_stall_watch's behavior when handed a valid argument, which is why the dead production wiring survived two rounds of review. Also fixed tests/fix-2650-plan-phase-stall-detection.test.cjs:170-195's raw try/finally to use t.after(), per CONTRIBUTING's test-cleanup convention. * chore(#2650): backfill changeset PR number to 3015 * fix: normalize CRLF at the read boundary in all .md-bash-extraction tests Maintainer-authorized scope expansion, folded into this PR rather than deferred: the Windows CI lane on this PR's own tests/fix-2650-plan-phase- stall-detection.test.cjs exposed DEFECT.TEST-SHELL-PIPELINE-NONPORTABLE (CONTEXT.md; recurring since #1700) as a repo-wide latent class, not a one-off. Ten test files parse a fenced ```bash block out of a workflow .md file and execute it via spawnSync/execFileSync; a Windows checkout can yield CRLF line endings despite .gitattributes eol=lf, and bash then treats the trailing \r on every extracted line as part of the token — "unexpected EOF while looking for matching `"'" or a bare syntax error, partway through the script. Added tests/helpers.cjs:readFileNormalized() — strips \r\n -> \n at the read boundary, before any fence-slicing or regex runs, so every downstream operation is correct by construction. Migrated all ten call sites to it: Previously broken (fs.readFileSync with no normalization anywhere between read and spawn): - tests/worktree-cleanup.test.cjs (extractCwdGuardBash) — also fixes a misleading comment claiming the fence regex alone was "CRLF-safe"; it protected only the fence delimiters, never the captured body. - tests/new-milestone-clear-phases.test.cjs (extractFenceBetween, extractFenceContaining) - tests/code-review-pipeline-regression.test.cjs (extractPostProcessingScript) - tests/drift-detection.test.cjs (readGate/bashBlock, plus the snippet-file comparison read in the same test) - tests/graphify-visualization.test.cjs (extractStep3Block) - tests/pause-work-improvements.test.cjs (extractCheckBlock) - tests/plan-review-convergence.test.cjs (extractReviewerFlagsParseBlock and the inline post-config-gate resolution-block slices) Already correct (split(/\r?\n/) then join('\n')), migrated to the shared helper for consistency rather than a fourth/fifth/sixth copy of the same fix: - tests/git-base-branch.test.cjs (extractHandleBranchingBash) - tests/quick-branching.test.cjs (extractStep25Bash) - tests/runtime-launcher-parity.test.cjs (extractResolverSnippet) Verified against a simulated Windows CRLF checkout (not assumed): for both the worktree-cleanup.test.cjs and new-milestone-clear-phases.test.cjs extraction shapes, confirmed the pre-fix code produces a real bash syntax error on CRLF input and the post-fix code does not. One eslint follow-up: local/no-crlf-fragile-split statically flags any bare `\n` inside a markdown-fence-shaped regex, regardless of whether the receiver was already normalized — it cannot see the readFileNormalized() data-flow. Kept `\r?\n` in extractCwdGuardBash's fence regex (redundant but harmless on pre-normalized input) rather than fight the rule. Scope note: this diff is broader than issue #2650's own change (plan- phase.md stall detection) because the Windows lane surfaced a genuine repo-wide defect class while verifying that fix, and the maintainer authorized fixing it here rather than filing it separately and shipping a known-broken pattern. Runtime impact: none — this is a test-harness-only defect. The live orchestrator (Claude Code or another runtime) does not do a byte-exact extract-and-pipe of .md content into a shell the way these tests do; it reads the instructions and generates its own bash invocation text, which does not reproduce a raw CRLF pass-through the same way. Not touched: tests/plan-review-convergence.test.cjs's separate, tracked spawnSync ETIMEDOUT flake under bench load (#3005, reproduced on unmodified next) — unrelated load-sensitivity, not a CRLF symptom. * fix(#2650): remove stale drift-ack fragment — plan-phase.md is self-explaining tests/emitted-drift-acks/2650-plan-phase-stall-detection.json acknowledged plan-phase.md's own emitted-path hash move, but plan-phase.md is directly edited in this diff. Per the emitted-attribution law (ADR-2719, tests/emitted-attribution.test.cjs), a workflow's emitted key equals its own source path (gsd-core-verbatim identity rule), so a direct edit to the source is self-explaining and auto-attributed — no ack was ever needed. Verified via the pre-merge lint (scripts/lint-emitted-drift-ack.cjs, run through npm run lint:ci with a fully cleared eslint cache): it passes clean with the fragment removed, confirming no contradiction between the lint and the runtime attribution gate — this was simply an unnecessary fragment. * fix(#2650): restore plan-phase.md drift-ack — size ratchet demands it against next tests/emitted-drift-acks/2650-plan-phase-stall-detection.json was deleted in the previous commit because, against an earlier verification base, it was inert: it explained a moved emitted hash that a direct edit to plan-phase.md already self-attributes. Against origin/next@f1af47766a the demand is different: plan-phase.md is 13 bytes larger than the base copy, which trips the emitted-attribution size ratchet — a job this same ack also performs. Recreated in the documented shape, keyed on the bare filename plan-phase.md (not the full path, and not restating the byte delta per review guidance), describing the actual change: the {outputFile} binding fix for the dead PLANNER_OUTPUT_FILE/CHECKER_OUTPUT_FILE variables and the 5 restored ORCHESTRATOR RULE labels required by #913, both at the stall-watch spawn sites, with explanatory bodies living in the lazily-loaded gsd-core/workflows/plan-phase/steps/stall-detection-helpers.md reference. Confirmed no other fragment (on this branch or on next) claims the bare key "plan-phase.md" before recreating — scripts/lint-emitted-drift-ack.cjs's duplicate check is an exact string match, and the only other mention of plan-phase.md in tests/emitted-drift-acks/ (2658-trae-instruction-file-path.json) uses the full path as its key, so there is no collision. * fix(#2650): real cause of Windows CI failure — bash -c argv-transport, not CRLF The CRLF diagnosis for PR #3015's Windows failure was wrong. Proven wrong, not assumed: .gitattributes' blanket `* text=auto eol=lf` means a Windows checkout never receives CRLF for stall-detection-helpers.md, and the extracted fence's line 64 is byte-identical and correctly balanced on every platform. The real cause: runShouldRecover() passed a 70+ line, quote-dense script as ONE argv element to `spawnSync('bash', ['-c', script, arg0, ...])` PLUS four more positional args. Windows has no execve — Node serializes that whole argv into a single CreateProcess command-line string, and Git Bash's MSYS layer re-splits and unescapes it with its own rules. The boundary between the script and the trailing args was not stable across that round trip (live evidence: one failure's stderr was prefixed `gsd_stall_should_recover_test:` — arg0 arrived — another `/usr/bin/bash:` — arg0 did not). Fixed by writing the script to a temp file and running `bash <file> <args>` instead — the four values are now normal, quote-free positional args, and the script itself never enters argv transport at all. Mirrors tests/quick-branching.test.cjs's extractStep25Bash/runStep, which already uses this exact shape and is green on Windows on `next`. tests/worktree-cleanup.test.cjs's extractCwdGuardBash/runGuard stays on `bash -c` but never appends extra positional args beyond the script itself, so it never hits the same boundary — checked both siblings per review, not assumed. Corrected the now-actively-misleading CRLF comment in extractStallHelpersBash(), and corrected the changeset's claim that the repo-wide CRLF-normalization fix (folded into this branch, maintainer- authorized) explains this PR's own Windows failure — it doesn't, though it remains defensible on its own merits as general test-portability hardening. Separately, while auditing the shipped (non-test) gsd_stall_watch for Windows portability per review request, found and fixed a second, real user-facing defect: the artifact-freshness check used GNU find's `-newermt "@<epoch>"` shorthand, which the BSD find(1) actually shipped on macOS does NOT understand ("Can't parse date/time: @<epoch>", verified live against /usr/bin/find on both a stale and a genuinely fresh file). With the adjacent `2>/dev/null`, that failed silently and permanently degraded artifact_fresh to false on every macOS run — a plan-checker or planner actively writing plan files could still be reported "stalled." Replaced with `find $glob -mmin -N` ("modified less than N minutes ago"), which needs no date-string parsing and is supported identically by GNU find and BSD find; verified live that the old shape fails and the new shape passes against the same real fresh file. Added a real-execution regression test (gsd_stall_watch with `sleep` stubbed to a no-op so the test doesn't actually wait, but the real `find ... -mmin` line still runs) proving the fix, replacing the prior "not integration-tested" note for that path. Note: the remote gsd-test runner is Linux-only, so it cannot itself confirm the Windows fix — only the actual windows-latest CI lane can. * fix(#2650): route the third bash -c call site through the same temp-file seam runWatch() and a `-mmin` regression test still passed their script via `bash -c <script>` after the previous commit only converted runShouldRecover() — live Windows CI on 4b86cc57f confirmed the mechanism: failures went 11 -> 4, and `full test (windows-latest, 22, shard 1/3)` and `shard 2/3` flipped from fail to pass, but the remaining 4 failures (all in this file, all still `bash: -c:`) were exactly the gsd_stall_watch describe block, which runWatch() serves. runWatch() passes NO extra positional args at all, so this also rules out the trailing-args theory from the prior commit: the ~73-line, quote-dense script itself is what does not survive Windows argv serialization when passed as a single `-c` element, regardless of how many (if any) further argv elements follow it. Extracted one shared runBashScript(script, args, opts) helper — write to a fs.mkdtempSync'd file, run `bash <file> [args...]`, clean up in `finally` — and routed all three bash-invoking call sites in this file through it (runShouldRecover, runWatch, and the -mmin freshness test that builds its own script inline for the `sleep` stub). One transport seam means a fourth call site in this file cannot silently reintroduce the bug in isolation, which is exactly what happened here with a second call site. Corrected extractStallHelpersBash()'s doc comment a second time to state the mechanism precisely (script content, not argv-element count) and cite the live evidence (11->4 failures, shards 1 and 2 flipping green) so the next reader does not have to rediscover it. Audited every other bash-invoking call site in files this branch touches, per review request: - tests/code-review-pipeline-regression.test.cjs (runPostProcessing), tests/graphify-visualization.test.cjs (runBlock), and tests/drift-detection.test.cjs (two execFileSync('bash', ['-c', ...]) sites, one of them carrying the same giant runtime-launcher preamble text) — all pre-existing, UNCHANGED by this branch (only touched for the readFileNormalized() CRLF swap), and already exercised on `next`'s last six Windows CI runs per the reviewer's own citation. Left as-is: no evidence of failure, and converting untested pre-existing code outside #2650's scope on an unverifiable guess would be its own risk. - tests/git-base-branch.test.cjs (runHandleBranchingStep) and tests/quick-branching.test.cjs (runStep) already use the same temp-file pattern. No action needed. - tests/runtime-launcher-parity.test.cjs (runResolver) uses `bash -c` but is explicitly `if (process.platform === 'win32') return '';` guarded off on Windows entirely, for an unrelated extension-less-PATH-stub reason — never reaches Windows argv transport at all. No action needed. - tests/worktree-cleanup.test.cjs (runGuard) confirmed by the reviewer as correct and verified; not touched, per instruction. Do not touch: the -mmin fix, the drift-ack fragment, the changeset — all three confirmed correct in prior rounds and left untouched here. Note: the remote gsd-test runner is Linux-only and cannot confirm this; only the windows-latest lanes on #3015 can. * fix(#2650): give runBashScript a default timeout runShouldRecover() was the only one of the three call sites through runBashScript() with no timeout — runWatch() and the -mmin test both pass timeout: 10000 explicitly. Not a regression (this path never had a bound before), but CONTEXT.md's unbounded-subprocess guidance applies directly, and runShouldRecover() is driven repeatedly by a fast-check property test: one pathological input that fails to terminate would hang CI indefinitely instead of failing. timeout: 10000 is now the helper's own default, with ...opts spread after it so the two existing explicit timeout: 10000 call sites are unchanged and any future caller inherits a bound automatically. * fix(#2650): build the -mmin freshness test's glob with forward slashes Windows CI on d6ddda6ea reported the last failure: the -mmin regression test expected 'active' but got 'waiting' — find matched nothing, the same silent-degradation shape as the macOS -newermt defect, but this time in the test's own fixture rather than the shipped bash. Traced what production actually passes: every gsd_stall_watch call site in plan-phase.md builds artifact_glob as `"${PHASE_DIR}"'/*-PLAN.md'` — PHASE_DIR is a POSIX-style .planning/phases/NN-slug value, and the whole thing runs under Git Bash regardless of host OS, so production's glob is always forward-slash. The test instead built it with `path.join(tmp, '*-PLAN.md')`, which on Windows yields a backslash path (C:\Users\RUNNER~1\...\*-PLAN.md). In bash pathname expansion a backslash escapes the next character, so that pattern can never match a real path — find silently returns empty under the existing 2>/dev/null, same shape as the macOS bug. Confirmed as a test artifact, not a production defect: production never constructs the glob this way, so no Windows user is affected. Fixed by forward-slashing the tmp dir before appending the glob suffix, matching production's own convention, with a comment recording why (so a future "simplify this back to path.join" edit doesn't silently reintroduce the failure). The shipped bash's unquoted $artifact_glob is untouched — quoting it would break the multi-file glob expansion it exists for. Note: the remote runner is Linux-only and already passed clean at d6ddda6ea (0/29,603, both node lanes); only the windows-latest lanes on #3015 can confirm this fix. * fix(#2650): forward-slash the three remaining runWatch globs (vacuous-pass CR) The :353 fix (833c11da9) only converted the -mmin freshness test's glob. Three sibling tests in the same describe block still built theirs with path.join(tmp, '*-PLAN.md'), which yields a backslash path on Windows. Two of those three were silently passing for the wrong reason: the '-> stalled' and '-> waiting' tests both expect the glob to match nothing, and on Windows a backslash path matches nothing regardless of whether the directory is actually empty (bash eats each backslash as an escape before the pattern is even evaluated). They would have passed identically with glob expansion completely broken, which is a vacuous pass — not exercising what they claim to. The third ('-> marker_received') is outcome-independent of the glob, so it was merely inconsistent rather than wrong. Converted all three to the same `${tmp.replace(/\\/g, '/')}/*-PLAN.md` construction already used at the -mmin test, so every glob in the file now matches production's own forward-slash `"${PHASE_DIR}"'/*-PLAN.md'` shape, and the two negative tests are meaningful on Windows instead of accidentally correct. Reworded the trailing comment on the 'stalled' test's glob line: it now describes the fixture (the tmp dir contains no *-PLAN.md files) rather than the pattern, since "matches nothing" read as a property of the glob syntax when it's a property of what's on disk. No assertion, the sleep stub, runBashScript, or the shipped bash changed. Smoke-tested all three updated tests manually before committing (not via node --test): marker_received / stalled / waiting, all correct. * fix(#2650): fix own regression tests for #2993's plan-phase.md relocation 531101843's merge with origin/next brought in #2993 (unrelated, epic #1671 Phase 6.2), which extracted plan-phase.md's whole "Chunked Planning Mode" section into gsd-core/workflows/plan-phase/steps/chunked-planning-mode.md, leaving a <!-- gsd:section --> pointer behind. tests/plan-phase-drift-guard. test.cjs (#913) was already updated to read the combined surface (host file + every steps/*.md) so its label count didn't go blind — my own #2650 regression tests were not, and searched plan-phase.md alone for the two chunked spawn sites' headings, which no longer exist there. Two tests failed outright (indexOf returning -1); a third ("standard planner spawn") was silently weakened to an unbounded slice-to-EOF by the same relocation, since its own end-boundary heading also moved — passing by accident rather than by testing what it claimed. Promoted the drift guard's local readPlanPhaseCombined() to a shared, exported tests/helpers.cjs readWorkflowCombined(workflowPath) (host file + sorted steps/*.md, CRLF-normalized at the read boundary) so a second, divergent implementation is never written — the drift guard now delegates to it via a same-named local wrapper, unchanged at every existing call site. Fixed the three affected tests in tests/fix-2650-plan-phase-stall-detection. test.cjs: - "standard planner spawn (step 8)": end boundary changed from the now-gone "## 8.5. Chunked Planning Mode" heading to "## 9. Handle Planner Return", which still exists in plan-phase.md. - "chunked outline spawn (8.5.1)" / "chunked per-plan spawn (8.5.2)": now read gsd-core/workflows/plan-phase/steps/chunked-planning-mode.md directly (not the generic multi-file combined blob, whose file-sort ordering would put unrelated step files between 8.5.2's slice and any downstream anchor) — the same heading-to-heading slicing as before still works because the file is small and self-contained. - Extended the "no unbound $PLANNER_OUTPUT_FILE/$CHECKER_OUTPUT_FILE" check to also scan chunked-planning-mode.md, since two of the five spawn sites now live there. - Added a new count-based test asserting exactly 5 (not "at least one") `gsd_stall_watch "$TS" "{outputFile}"` invocations across the combined surface, mirroring #913's own label-count guard, so every one of the five spawns stays provably bounded and a future relocation can't silently drop one without a test noticing. Also added a small positive test that plan-phase.md's <!-- gsd:section --> pointer to chunked-planning-mode.md exists (#2993 is unrelated to #2650 but its presence is now load-bearing for where 2 of the 5 spawn sites live). Audited every other test file in the repo for a stale reference to content #2993 relocated (searched for the moved headings/prose and for "chunked-planning-mode"/"CHUNKED_MODE" across all *.test.cjs): only this file and the drift guard needed changes. tests/issue-2762-plan-reviews-chunked.test.cjs already reads chunked-planning-mode.md directly (brought in correct by the same merge). gen-section-manifest.test.cjs, init.test.cjs, and workflow-fragments.test.cjs reference "chunked-planning-mode" only as a manifest/section-id fixture value for #2993 itself, not as a stale pointer to relocated content. Did not touch: the ported ORCHESTRATOR RULE lines, run_in_background=true, the glob constructions, runBashScript, the -mmin change, the timeout default, or the drift-ack fragment (confirmed correct against the stale local `next` ref two rounds ago and left alone). --------- Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
7
.changeset/eager-foxes-roam.md
Normal file
7
.changeset/eager-foxes-roam.md
Normal file
@@ -0,0 +1,7 @@
|
||||
---
|
||||
type: Fixed
|
||||
pr: 3015
|
||||
---
|
||||
**Plan-phase now auto-recovers from a stalled planner or plan-checker spawn instead of hanging indefinitely** — when a planner/plan-checker subagent produces no completion marker and no fresh on-disk plan activity for a configurable threshold (`planner.stall_threshold_minutes`, default 10 minutes, checked every `planner.stall_detect_interval_minutes`, default 5), plan-phase now automatically surfaces the existing accept-plans/retry/stop recovery choice instead of waiting for a manual interrupt. Trade-off: a planner/plan-checker that finishes quickly is no longer detected instantly — completion is observed at most one `stall_detect_interval_minutes` (default 5 min) after it happens, in exchange for eliminating the previously-indefinite hang. (#2650)
|
||||
|
||||
**Hardened a repo-wide test-portability pattern (maintainer-authorized scope expansion): ten test files that extract a fenced bash block from a workflow `.md` file and execute it via `spawnSync`/`execFileSync` now normalize CRLF to LF at the point of reading the file**, before any fence-slicing or regex runs. A raw `readFileSync` followed by a bare `\n`-based regex against markdown fences is fragile by construction — it silently assumes LF regardless of how the bytes actually arrived — and this normalization removes that assumption at a single shared `readFileNormalized()` helper in `tests/helpers.cjs`, used by all ten call sites, so the next `.md`-extraction test is correct by default instead of needing to rediscover the fix independently. (Correction: this was NOT the cause of this PR's own `windows-latest` CI failure — `.gitattributes`' blanket `* text=auto eol=lf` means a Windows checkout of this repo never receives CRLF in the first place. That failure was a separate `bash -c` argv-transport defect in the #2650 test file itself, fixed alongside this.)
|
||||
@@ -371,6 +371,8 @@ All workflow toggles follow the **absent = enabled** pattern. If a key is missin
|
||||
| `workflow.subagent_timeout` | number | `300000` | Timeout in milliseconds for parallel subagent tasks (e.g. codebase mapping). Increase for large codebases or slower models. Default: 300000 (5 minutes) |
|
||||
| `executor.stall_detect_interval_minutes` | number | `5` | Minutes between executor stall checks while an executor agent is active. The execute-phase orchestrator uses this cadence to inspect recent commits and avoid waiting forever on a silent agent. |
|
||||
| `executor.stall_threshold_minutes` | number | `10` | Minutes without executor completion or expected-branch commit activity before execute-phase offers recovery choices for a possible stalled executor. |
|
||||
| `planner.stall_detect_interval_minutes` | number | `5` | Minutes between planner/plan-checker stall checks while a planner or plan-checker agent is active. The plan-phase orchestrator uses this cadence to inspect on-disk `*-PLAN.md` activity and avoid waiting forever on a silent agent (#2650). |
|
||||
| `planner.stall_threshold_minutes` | number | `10` | Minutes without a completion marker or fresh on-disk plan activity before plan-phase automatically surfaces the accept-plans/retry/stop recovery choice for a possible stalled planner or plan-checker (#2650). |
|
||||
| `workflow.inline_plan_threshold` | number | `3` | Maximum number of tasks in a phase before the planner generates a separate PLAN.md file instead of inlining tasks in the prompt |
|
||||
| `workflow.drift_threshold` | number | `3` | Minimum number of new structural elements (new directories, barrel exports, migrations, route modules) before the codebase-drift gate takes action. The gate runs at two points: `plan:pre` (before `/gsd-plan-phase` plans — **non-blocking, warn-only**, so plans are authored against a fresh STRUCTURE.md) and `execute:wave:post` (after `/gsd-execute-phase` — honors `workflow.drift_action`). See [#2003](https://github.com/open-gsd/gsd-core/issues/2003). Added in v1.39 |
|
||||
| `workflow.drift_action` | string | `warn` | What to do when `workflow.drift_threshold` is exceeded **at `execute:wave:post`** (after `/gsd-execute-phase`). `warn` prints a message suggesting `/gsd-map-codebase --paths …`; `auto-remap` spawns `gsd-codebase-mapper` scoped to the affected paths. The `plan:pre` pre-check is always warn-only regardless of this setting — it never auto-spawns the mapper at plan entry. Added in v1.39 |
|
||||
|
||||
@@ -63,6 +63,8 @@
|
||||
"workflow.context_guard_mode",
|
||||
"executor.stall_detect_interval_minutes",
|
||||
"executor.stall_threshold_minutes",
|
||||
"planner.stall_detect_interval_minutes",
|
||||
"planner.stall_threshold_minutes",
|
||||
"workflow.inline_plan_threshold",
|
||||
"hooks.context_warnings",
|
||||
"hooks.workflow_guard",
|
||||
|
||||
@@ -667,6 +667,12 @@ after `$SPEC_FILE` (Step 7), before the gsd-planner spawn (Step 8).
|
||||
disabled or no requirement IDs; §A deterministic edge probe → `$COVERAGE` when `EDGE_ABSENT`; §B
|
||||
prohibition recall in the planner). Pass `$COVERAGE` and `$SPECLESS_FALLBACK_DISABLED` into Step 8.
|
||||
|
||||
## 7.99. Bounded Stall-Detection Helpers (#2650)
|
||||
|
||||
Read+execute `gsd-core/workflows/plan-phase/steps/stall-detection-helpers.md` (defines
|
||||
`gsd_stall_should_recover`/`gsd_stall_watch`, and how `{outputFile}` below is bound;
|
||||
independent of the teams-status guard above, AC2).
|
||||
|
||||
## 8. Spawn gsd-planner Agent
|
||||
|
||||
Display banner:
|
||||
@@ -830,11 +836,12 @@ Agent(
|
||||
prompt=filled_prompt,
|
||||
subagent_type="gsd-planner",
|
||||
model="{planner_model}",
|
||||
description="Plan Phase {phase}"
|
||||
description="Plan Phase {phase}",
|
||||
run_in_background=true
|
||||
)
|
||||
```
|
||||
|
||||
> **ORCHESTRATOR RULE — ALL RUNTIMES**: After calling Agent() above, stop working on this task immediately. Do not read more files, edit code, or run tests related to this task while the subagent is active. Wait for the subagent to return its result. This prevents duplicate work, conflicting edits, and wasted context. Only resume when the subagent result is available.
|
||||
**ORCHESTRATOR RULE — ALL RUNTIMES:** `TS=$(date +%s)`; repeat `PLANNER_STALL_RESULT=$(gsd_stall_watch "$TS" "{outputFile}" "${PHASE_DIR}"'/*-PLAN.md' "## PLANNING COMPLETE" "## PHASE SPLIT RECOMMENDED" "## ⚠ Source Audit" "## CHECKPOINT REACHED" "## PLANNING INCONCLUSIVE")` while waiting/active — `marker_received` -> step 9; `stalled` -> 9a.
|
||||
|
||||
**If `CHUNKED_MODE` is `true`:** Skip the Agent() call above — proceed to step 8.5 instead.
|
||||
|
||||
@@ -992,16 +999,18 @@ Agent(
|
||||
prompt=checker_prompt,
|
||||
subagent_type="gsd-plan-checker",
|
||||
model="{checker_model}",
|
||||
description="Verify Phase {phase} plans"
|
||||
description="Verify Phase {phase} plans",
|
||||
run_in_background=true
|
||||
)
|
||||
```
|
||||
|
||||
> **ORCHESTRATOR RULE — ALL RUNTIMES**: After calling Agent() above, stop working on this task immediately. Do not read more files, edit code, or run tests related to this task while the subagent is active. Wait for the subagent to return its result. This prevents duplicate work, conflicting edits, and wasted context. Only resume when the subagent result is available.
|
||||
**ORCHESTRATOR RULE — ALL RUNTIMES:** `TS=$(date +%s)`; repeat `CHECKER_STALL_RESULT=$(gsd_stall_watch "$TS" "{outputFile}" "${PHASE_DIR}"'/*-PLAN.md' "## VERIFICATION PASSED" "## ISSUES FOUND")` while waiting/active.
|
||||
|
||||
## 11. Handle Checker Return
|
||||
|
||||
- **`## VERIFICATION PASSED`:** Display confirmation, proceed to step 13.
|
||||
- **`## ISSUES FOUND`:** Display issues, check iteration count, proceed to step 12.
|
||||
- **`marker_received` + `## VERIFICATION PASSED`:** Display confirmation, proceed to step 13.
|
||||
- **`marker_received` + `## ISSUES FOUND`:** Display issues, check iteration count, proceed to step 12.
|
||||
- **`stalled`:** Automatically surface 11a's recovery choice (Accept verification / Retry checker / Stop) — no manual interrupt needed.
|
||||
- **Empty / truncated / no recognized marker:** → Filesystem fallback (step 11a).
|
||||
|
||||
**Thinking partner for architectural tradeoffs (conditional):**
|
||||
@@ -1107,11 +1116,12 @@ Agent(
|
||||
prompt=revision_prompt,
|
||||
subagent_type="gsd-planner",
|
||||
model="{planner_model}",
|
||||
description="Revise Phase {phase} plans"
|
||||
description="Revise Phase {phase} plans",
|
||||
run_in_background=true
|
||||
)
|
||||
```
|
||||
|
||||
> **ORCHESTRATOR RULE — ALL RUNTIMES**: After calling Agent() above, stop working on this task immediately. Do not read more files, edit code, or run tests related to this task while the subagent is active. Wait for the subagent to return its result. This prevents duplicate work, conflicting edits, and wasted context. Only resume when the subagent result is available.
|
||||
**ORCHESTRATOR RULE — ALL RUNTIMES:** (7.99; no marker, mtimes only) `TS=$(date +%s)`; repeat `PLANNER_STALL_RESULT=$(gsd_stall_watch "$TS" "{outputFile}" "${PHASE_DIR}"'/*-PLAN.md')` while waiting/active — `stalled` -> 1) Accept as revised, to step 13, 2) Retry, 3) Stop.
|
||||
|
||||
After planner returns -> spawn checker again (step 10), increment iteration_count.
|
||||
|
||||
|
||||
@@ -45,15 +45,16 @@ Agent(
|
||||
Return: ## OUTLINE COMPLETE with plan count.",
|
||||
subagent_type="gsd-planner",
|
||||
model="{planner_model}",
|
||||
description="Outline Phase {phase} (chunked)"
|
||||
description="Outline Phase {phase} (chunked)",
|
||||
run_in_background=true
|
||||
)
|
||||
```
|
||||
|
||||
> **ORCHESTRATOR RULE — ALL RUNTIMES**: After calling Agent() above, stop working on this task immediately. Do not read more files, edit code, or run tests related to this task while the subagent is active. Wait for the subagent to return its result. This prevents duplicate work, conflicting edits, and wasted context. Only resume when the subagent result is available.
|
||||
**ORCHESTRATOR RULE — ALL RUNTIMES:** `TS=$(date +%s)`; repeat `PLANNER_STALL_RESULT=$(gsd_stall_watch "$TS" "{outputFile}" "$OUTLINE_FILE" "## OUTLINE COMPLETE")` while waiting/active.
|
||||
|
||||
Handle return:
|
||||
- **`## OUTLINE COMPLETE`:** Read `PLAN-OUTLINE.md`, extract plan list. Continue to 8.5.2.
|
||||
- **Any other return or empty:** Display error. Offer: 1) Retry outline, 2) Stop.
|
||||
- **`marker_received`:** Read `PLAN-OUTLINE.md`, extract plan list. Continue to 8.5.2.
|
||||
- **`stalled` / any other return or empty:** Display error. Offer: 1) Retry outline, 2) Stop.
|
||||
|
||||
### 8.5.2 Per-Plan Tasks (single-plan mode, ~3-5 min each)
|
||||
|
||||
@@ -89,11 +90,12 @@ For each plan entry extracted from `PLAN-OUTLINE.md`:
|
||||
Return: ## PLAN COMPLETE with the plan ID.",
|
||||
subagent_type="gsd-planner",
|
||||
model="{planner_model}",
|
||||
description="Plan {plan_id} (chunked {k}/{N})"
|
||||
description="Plan {plan_id} (chunked {k}/{N})",
|
||||
run_in_background=true
|
||||
)
|
||||
```
|
||||
|
||||
> **ORCHESTRATOR RULE — ALL RUNTIMES**: After calling Agent() above, stop working on this task immediately. Do not read more files, edit code, or run tests related to this task while the subagent is active. Wait for the subagent to return its result. This prevents duplicate work, conflicting edits, and wasted context. Only resume when the subagent result is available.
|
||||
**ORCHESTRATOR RULE — ALL RUNTIMES:** `TS=$(date +%s)`; repeat `PLANNER_STALL_RESULT=$(gsd_stall_watch "$TS" "{outputFile}" "$PLAN_FILE" "## PLAN COMPLETE")` while waiting/active — `stalled` falls into step 4 (preserves prior committed chunks).
|
||||
|
||||
4. **Verify disk:** Check `${PHASE_DIR}/{plan_id}-PLAN.md` exists. If missing: offer 1) Retry, 2) Stop.
|
||||
|
||||
|
||||
149
gsd-core/workflows/plan-phase/steps/stall-detection-helpers.md
Normal file
149
gsd-core/workflows/plan-phase/steps/stall-detection-helpers.md
Normal file
@@ -0,0 +1,149 @@
|
||||
# Bounded Stall-Detection Helpers (#2650)
|
||||
|
||||
Every planner/plan-checker spawn in `plan-phase.md` dispatches with
|
||||
`run_in_background=true`, records `TS=$(date +%s)`, and then repeatedly
|
||||
calls `gsd_stall_watch` until it returns something other than
|
||||
`waiting`/`active`. This mirrors the already-shipped `executor.stall_*`
|
||||
pattern (`execute-phase.md`, bug #3212, commit `e7942c21b`) but — unlike
|
||||
that prose-only surveillance, which cannot run during a *blocking* `Agent()`
|
||||
call — each `gsd_stall_watch` call is a real, bounded bash subprocess wait
|
||||
issued as its own tool call, so it returns control to the orchestrator on
|
||||
its own schedule regardless of whether the backgrounded agent's own
|
||||
completion notification ever arrives.
|
||||
|
||||
**Binding `{outputFile}` (load-bearing, not optional):** every `gsd_stall_watch`
|
||||
call below takes `{outputFile}` as its second argument — a literal token the
|
||||
orchestrator must substitute with the REAL path from the immediately preceding
|
||||
`run_in_background=true` Agent() call's returned `async_launched` result,
|
||||
exactly as `docs-update.md:471` already does ("Read tool: file_path: `{outputFile
|
||||
from README agent result}`"). This is NOT a bash variable the snippet below
|
||||
assigns — there is nothing upstream that assigns one, so a bash variable
|
||||
reference here would silently stay empty forever. With `{outputFile}` correctly
|
||||
substituted, `[ -f "$output_file" ]` can find the real file and the
|
||||
`marker_received` path is reachable; left as a literal (or as an unbound bash
|
||||
variable), `marker_found` can never become `true` and every spawn silently
|
||||
falls back to the mtime-only path — for the plan-checker spawn specifically,
|
||||
that fallback is broken (see next paragraph), so binding this correctly there
|
||||
is not a nice-to-have.
|
||||
|
||||
**Plan-checker's artifact glob needs the marker, not just mtimes:** the
|
||||
plan-checker spawn watches `*-PLAN.md` for freshness, but a checker that
|
||||
PASSES touches none of those files — no fresh mtime, ever, on a clean run.
|
||||
Without `{outputFile}` correctly bound to the real completion output, a
|
||||
healthy plan-checker that returns `## VERIFICATION PASSED` in two minutes
|
||||
would still be declared `stalled` once `planner.stall_threshold_minutes`
|
||||
elapses — reporting a succeeded agent as hung, which is worse than the
|
||||
original unbounded wait. The marker path (via `{outputFile}`) is the ONLY
|
||||
working completion signal for that spawn; the artifact glob is secondary
|
||||
there.
|
||||
|
||||
**Single-cycle by design, not one long-lived loop:** `gsd_stall_watch` sleeps
|
||||
for exactly one `PLANNER_STALL_INTERVAL_MINUTES` and returns — it does NOT
|
||||
loop internally for the full `PLANNER_STALL_THRESHOLD_MINUTES`. A single Bash
|
||||
tool call blocking for `threshold + interval` minutes (up to 15 min at
|
||||
defaults) risks the *host tool's own* timeout killing the call before it ever
|
||||
prints a result — silently defeating the fix it exists to ship. Looping at
|
||||
the orchestrator-prose level instead means every cycle is a short (default 5
|
||||
min), real, bounded call that reliably hands control back — the outer
|
||||
threshold is enforced by `dispatch_ts` accumulating across calls, not by one
|
||||
call's own duration.
|
||||
|
||||
**Disclosed tradeoff:** the first cycle always sleeps a full
|
||||
`PLANNER_STALL_INTERVAL_MINUTES` before its first check, so a planner that
|
||||
completes in seconds is not observed by this path until that interval
|
||||
elapses (default 5 min) — slower than a plain blocking call's near-instant
|
||||
return on success. This is deliberate: it trades a bounded, at-most-one-
|
||||
interval delay on the (common) success path for eliminating the unbounded,
|
||||
possibly-indefinite hang on the (rare, previously unrecoverable) stall path
|
||||
this issue is about. `PLANNER_STALL_INTERVAL_MINUTES` is the knob for
|
||||
projects that want a tighter success-path latency at the cost of more
|
||||
config-get calls.
|
||||
|
||||
This block is independent of, and never gated behind, the `query
|
||||
teams-status` / `CLAUDE_CODE_EXPERIMENTAL_AGENT_TEAMS` guard used for the
|
||||
researcher spawn — the stall path applies on every runtime, teams-active or
|
||||
not (AC2).
|
||||
|
||||
```bash
|
||||
_GSD_SHIM_NAME="gsd-tools.cjs"; _GSD_RUNTIME_ROOT="${RUNTIME_DIR:-$(git rev-parse --show-toplevel 2>/dev/null || pwd)}"; GSD_TOOLS="${_GSD_RUNTIME_ROOT}/gsd-core/bin/${_GSD_SHIM_NAME}"; if [ -f "$GSD_TOOLS" ]; then gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${_GSD_RUNTIME_ROOT}/.claude/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${_GSD_RUNTIME_ROOT}/.claude/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${_GSD_RUNTIME_ROOT}/.codex/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${_GSD_RUNTIME_ROOT}/.codex/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif command -v gsd-tools >/dev/null 2>&1; then GSD_TOOLS="$(command -v gsd-tools)"; gsd_run() { "$GSD_TOOLS" "$@"; }; elif [ -f "${CLAUDE_CONFIG_DIR:-$HOME/.claude}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${CLAUDE_CONFIG_DIR:-$HOME/.claude}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${HERMES_HOME:-$HOME/.hermes}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${HERMES_HOME:-$HOME/.hermes}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${CURSOR_CONFIG_DIR:-$HOME/.cursor}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${CURSOR_CONFIG_DIR:-$HOME/.cursor}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${CODEX_HOME:-$HOME/.codex}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${CODEX_HOME:-$HOME/.codex}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${GEMINI_CONFIG_DIR:-$HOME/.gemini}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${GEMINI_CONFIG_DIR:-$HOME/.gemini}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${COPILOT_CONFIG_DIR:-$HOME/.copilot}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${COPILOT_CONFIG_DIR:-$HOME/.copilot}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${WINDSURF_CONFIG_DIR:-$HOME/.codeium/windsurf}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${WINDSURF_CONFIG_DIR:-$HOME/.codeium/windsurf}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${AUGMENT_CONFIG_DIR:-$HOME/.augment}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${AUGMENT_CONFIG_DIR:-$HOME/.augment}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${TRAE_CONFIG_DIR:-$HOME/.trae}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${TRAE_CONFIG_DIR:-$HOME/.trae}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${QWEN_CONFIG_DIR:-$HOME/.qwen}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${QWEN_CONFIG_DIR:-$HOME/.qwen}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${CODEBUDDY_CONFIG_DIR:-$HOME/.codebuddy}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${CODEBUDDY_CONFIG_DIR:-$HOME/.codebuddy}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${CLINE_CONFIG_DIR:-$HOME/.cline}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${CLINE_CONFIG_DIR:-$HOME/.cline}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${GROK_AGENTS_HOME:-$HOME/.agents}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${GROK_AGENTS_HOME:-$HOME/.agents}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${ANTIGRAVITY_CONFIG_DIR:-$HOME/.gemini/antigravity}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${ANTIGRAVITY_CONFIG_DIR:-$HOME/.gemini/antigravity}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${OPENCODE_CONFIG_DIR:-${XDG_CONFIG_HOME:-$HOME/.config}/opencode}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${OPENCODE_CONFIG_DIR:-${XDG_CONFIG_HOME:-$HOME/.config}/opencode}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${KILO_CONFIG_DIR:-${XDG_CONFIG_HOME:-$HOME/.config}/kilo}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${KILO_CONFIG_DIR:-${XDG_CONFIG_HOME:-$HOME/.config}/kilo}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; else echo "ERROR: gsd-tools.cjs not found at $GSD_TOOLS and gsd-tools is not on PATH. Run: npx -y @opengsd/gsd-core@latest --claude --local" >&2; exit 1; fi; if [ -n "${CLAUDE_ENV_FILE:-}" ] && [ -n "${GSD_TOOLS:-}" ]; then printf "export PATH='%s':\"\$PATH\"\n" "${GSD_TOOLS%/*}" >> "$CLAUDE_ENV_FILE" 2>/dev/null || true; fi
|
||||
PLANNER_STALL_INTERVAL_MINUTES=$(gsd_run query config-get planner.stall_detect_interval_minutes 2>/dev/null || echo "5")
|
||||
PLANNER_STALL_THRESHOLD_MINUTES=$(gsd_run query config-get planner.stall_threshold_minutes 2>/dev/null || echo "10")
|
||||
# Both values are config-controlled (.planning/config.json, editable by any repo
|
||||
# contributor) and both flow into `$(( ))` arithmetic below. A non-numeric
|
||||
# value there is NOT a code-execution risk (empirically verified: bash's
|
||||
# arithmetic evaluator hard-errors on a `$(cmd)`-shaped operand instead of
|
||||
# invoking it — "syntax error: operand expected", command never runs) but IS
|
||||
# a reliability risk this fix cannot afford: a malformed config value would
|
||||
# abort the stall-watcher itself with a bash syntax error, silently defeating
|
||||
# the exact hang-recovery this issue is about. Reject anything that is not a
|
||||
# bare non-negative integer before it is ever used, so a bad config value
|
||||
# degrades to the safe default instead of crashing the watcher.
|
||||
[[ "$PLANNER_STALL_INTERVAL_MINUTES" =~ ^[0-9]+$ ]] || PLANNER_STALL_INTERVAL_MINUTES=5
|
||||
[[ "$PLANNER_STALL_THRESHOLD_MINUTES" =~ ^[0-9]+$ ]] || PLANNER_STALL_THRESHOLD_MINUTES=10
|
||||
|
||||
# gsd_stall_should_recover — pure decision function, no IO, no sleeping. Given how
|
||||
# long the orchestrator has been waiting plus two liveness signals (a completion
|
||||
# marker found in the agent's output file, and fresh on-disk artifact activity),
|
||||
# decides whether to keep waiting, treat the wait as satisfied, or auto-surface the
|
||||
# existing accept/retry/stop recovery menu (9a/11a). Never kills or retries anything
|
||||
# itself — it only classifies. Re-validates both numeric args as bare non-negative
|
||||
# integers (defense in depth — safe to call with any input, not just the resolved
|
||||
# config globals above) before either ever reaches arithmetic expansion.
|
||||
gsd_stall_should_recover() {
|
||||
local elapsed_seconds="$1" threshold_minutes="$2" marker_found="$3" artifact_fresh="$4"
|
||||
[[ "$elapsed_seconds" =~ ^[0-9]+$ ]] || elapsed_seconds=0
|
||||
[[ "$threshold_minutes" =~ ^[0-9]+$ ]] || threshold_minutes=10
|
||||
local threshold_seconds=$(( threshold_minutes * 60 ))
|
||||
if [ "$marker_found" = "true" ]; then
|
||||
echo "marker_received"; return 0
|
||||
fi
|
||||
if [ "$artifact_fresh" = "true" ]; then
|
||||
echo "active"; return 0
|
||||
fi
|
||||
if [ "$elapsed_seconds" -ge "$threshold_seconds" ]; then
|
||||
echo "stalled"; return 0
|
||||
fi
|
||||
echo "waiting"; return 0
|
||||
}
|
||||
|
||||
# gsd_stall_watch — ONE bounded, real (non-LLM-side) sleep-and-check cycle, not
|
||||
# a long-lived loop (see "Single-cycle by design" above — a single Bash tool
|
||||
# call spanning the full threshold risks the host tool's own timeout killing
|
||||
# it first). Sleeps exactly one PLANNER_STALL_INTERVAL_MINUTES, then checks for
|
||||
# a completion marker in $2 (the outputFile returned by the run_in_background
|
||||
# Agent() call) or fresh mtime activity under $3 (an artifact glob), against
|
||||
# elapsed time since $1 (an epoch-seconds dispatch_ts the CALLER records once,
|
||||
# before the first call, and passes unchanged on every repeat). Remaining args
|
||||
# are completion markers. Prints exactly one of: marker_received | active |
|
||||
# waiting | stalled. The caller repeats the call while the result is
|
||||
# waiting/active; any other result ends the wait.
|
||||
gsd_stall_watch() {
|
||||
local dispatch_ts="$1" output_file="$2" artifact_glob="$3"; shift 3
|
||||
local markers=("$@")
|
||||
[[ "$dispatch_ts" =~ ^[0-9]+$ ]] || dispatch_ts=$(date +%s)
|
||||
sleep "$(( PLANNER_STALL_INTERVAL_MINUTES * 60 ))"
|
||||
local now elapsed marker_found artifact_fresh
|
||||
now=$(date +%s)
|
||||
elapsed=$(( now - dispatch_ts ))
|
||||
marker_found="false"
|
||||
if [ -f "$output_file" ]; then
|
||||
for m in "${markers[@]}"; do
|
||||
if grep -qF "$m" "$output_file" 2>/dev/null; then marker_found="true"; break; fi
|
||||
done
|
||||
fi
|
||||
# -mmin -N ("modified less than N minutes ago"), not -newermt "@<epoch>":
|
||||
# -newermt's "@<epoch>" shorthand is a GNU-date convenience the shipped
|
||||
# BSD find(1) on macOS does NOT understand ("Can't parse date/time:
|
||||
# @<epoch>", verified live) — with the 2>/dev/null below that failed
|
||||
# silently and permanently degraded artifact_fresh to false on every
|
||||
# macOS run. -mmin -N needs no epoch/date-string conversion at all and is
|
||||
# supported identically by GNU find (Linux, Git-for-Windows' bundled
|
||||
# findutils) and BSD find (macOS). $artifact_glob stays intentionally
|
||||
# unquoted — the shell, not find, expands it into the matching file list.
|
||||
artifact_fresh="false"
|
||||
if [ -n "$(find $artifact_glob -mmin "-${PLANNER_STALL_INTERVAL_MINUTES}" 2>/dev/null)" ]; then
|
||||
artifact_fresh="true"
|
||||
fi
|
||||
gsd_stall_should_recover "$elapsed" "$PLANNER_STALL_THRESHOLD_MINUTES" "$marker_found" "$artifact_fresh"
|
||||
}
|
||||
```
|
||||
@@ -88,6 +88,8 @@ const SCHEMA_DEFAULTS: Record<string, unknown> = {
|
||||
'context_window': 200000,
|
||||
'executor.stall_detect_interval_minutes': 5,
|
||||
'executor.stall_threshold_minutes': 10,
|
||||
'planner.stall_detect_interval_minutes': 5,
|
||||
'planner.stall_threshold_minutes': 10,
|
||||
'git.create_tag': true,
|
||||
// Derived from the defaults manifest rather than restated, so the manifest
|
||||
// stays the single source of truth for the smart-zone budget (#2630).
|
||||
|
||||
@@ -26,7 +26,7 @@ const assert = require('node:assert/strict');
|
||||
const fs = require('node:fs');
|
||||
const path = require('node:path');
|
||||
const { spawnSync } = require('node:child_process');
|
||||
const { createTempDir, cleanup } = require('./helpers.cjs');
|
||||
const { createTempDir, cleanup, readFileNormalized } = require('./helpers.cjs');
|
||||
|
||||
const ROOT = path.resolve(__dirname, '..');
|
||||
const WORKFLOW_PATH = path.join(ROOT, 'gsd-core', 'workflows', 'code-review.md');
|
||||
@@ -592,7 +592,11 @@ describe('Bug 4 (#2352) — compute_file_scope tilde-path expansion', () => {
|
||||
// here: it only matches relative planning-artifact paths and is orthogonal
|
||||
// to tilde expansion (see code-review.md step 2, "Apply exclusions").
|
||||
function extractPostProcessingScript() {
|
||||
const src = fs.readFileSync(WORKFLOW_PATH, 'utf8');
|
||||
// readFileNormalized() strips \r\n -> \n before either fence below is
|
||||
// sliced out and later spawned via spawnSync('bash', ...) in
|
||||
// runPostProcessing() — an un-normalized read on a Windows checkout would
|
||||
// break bash mid-script (DEFECT.TEST-SHELL-PIPELINE-NONPORTABLE, #2650).
|
||||
const src = readFileNormalized(WORKFLOW_PATH);
|
||||
const postProcessingIdx = src.indexOf('**Post-processing (all tiers):**');
|
||||
assert.ok(postProcessingIdx !== -1, 'code-review.md must have a "Post-processing (all tiers)" section');
|
||||
|
||||
|
||||
@@ -824,15 +824,19 @@ const fs = require('node:fs');
|
||||
const os = require('node:os');
|
||||
const path = require('node:path');
|
||||
const { execFileSync } = require('node:child_process');
|
||||
const { cleanup } = require('./helpers.cjs');
|
||||
const { cleanup, readFileNormalized } = require('./helpers.cjs');
|
||||
|
||||
const GATE_MD = path.join(
|
||||
__dirname, '..', 'gsd-core', 'workflows', 'execute-phase', 'steps', 'codebase-drift-gate.md',
|
||||
);
|
||||
const SNIPPET_FILE = path.join(__dirname, '..', 'gsd-core', 'workflows', '_runtime-launcher.snippet.sh');
|
||||
|
||||
// readFileNormalized() strips \r\n -> \n before bashBlock() slices a fence
|
||||
// out of the result and hands it to execFileSync('bash', ...) below — an
|
||||
// un-normalized read on a Windows checkout would break bash mid-script
|
||||
// (DEFECT.TEST-SHELL-PIPELINE-NONPORTABLE, #2650).
|
||||
function readGate() {
|
||||
return fs.readFileSync(GATE_MD, 'utf8');
|
||||
return readFileNormalized(GATE_MD);
|
||||
}
|
||||
|
||||
// Extract the Nth (0-based) ```bash fenced block body from the file.
|
||||
@@ -877,7 +881,7 @@ describe('bug #619 — codebase-drift-gate resolves gsd-tools via the runtime sh
|
||||
|
||||
test('exactly one canonical launcher preamble, in the drift-check block, before any launcher call (#619)', () => {
|
||||
const content = readGate();
|
||||
const snippet = fs.readFileSync(SNIPPET_FILE, 'utf8').replace(/\r?\n$/, '');
|
||||
const snippet = readFileNormalized(SNIPPET_FILE).replace(/\n$/, '');
|
||||
|
||||
// Count canonical preamble occurrences across the whole file (parity: exactly one).
|
||||
let count = 0;
|
||||
|
||||
@@ -0,0 +1,6 @@
|
||||
{
|
||||
"version": 1,
|
||||
"paths": {
|
||||
"plan-phase.md": "#2650: after merging origin/next's #2993 fragmentization (which extracted the whole 'Chunked Planning Mode' section behind a lazily-loaded steps/chunked-planning-mode.md pointer), plan-phase.md's growth against the new base is no longer about restoring labels at 5 sites in one file — it is the remaining #2650 diff itself. Three of the five stall-watch spawn sites (standard planner, plan-checker, revision-loop planner respawn) still live directly in plan-phase.md; the other two (chunked outline planner, chunked per-plan planner) now live in the extracted gsd-core/workflows/plan-phase/steps/chunked-planning-mode.md, where their ORCHESTRATOR RULE lines were ported during merge resolution so tests/plan-phase-drift-guard.test.cjs (#913), which now counts labels across plan-phase.md AND every file under plan-phase/steps/*.md via readPlanPhaseCombined(), still finds all 7 required labels (5 stall-watch + 2 pre-existing researcher/pattern-mapper labels). At the 3 sites remaining in plan-phase.md itself, converting the plain blocking-wait rule to the bounded gsd_stall_watch mechanism (plus restoring run_in_background=true and adding the step 7.99 pointer to stall-detection-helpers.md) is a net growth over origin/next's own copy of the file, which has the chunked-planning-mode extraction but not the #2650 stall-detection fix. Verified still well under the ADR-857 Phase 6 PRE_PHASE6 cap (94519 bytes) after the merge."
|
||||
}
|
||||
}
|
||||
597
tests/fix-2650-plan-phase-stall-detection.test.cjs
Normal file
597
tests/fix-2650-plan-phase-stall-detection.test.cjs
Normal file
@@ -0,0 +1,597 @@
|
||||
// allow-test-rule: source-text-is-the-product — see #2650
|
||||
// Workflow markdown is the installed orchestration contract.
|
||||
|
||||
'use strict';
|
||||
|
||||
/**
|
||||
* #2650 — plan-phase hangs after gsd-planner writes all plans; completion never
|
||||
* reaches orchestrator.
|
||||
*
|
||||
* plan-phase.md's five planner/plan-checker Agent() spawns (standard planner,
|
||||
* chunked outline planner, chunked per-plan planner, plan-checker, and the
|
||||
* revision-loop planner respawn) previously waited for a subagent's return
|
||||
* with no time bound, no periodic check, and no config-driven threshold — the
|
||||
* only recovery path (9a/11a "Filesystem Fallback") required Agent() to have
|
||||
* already returned, so it could never fire when the call never returned
|
||||
* control at all. This mirrors the already-shipped `executor.stall_*` fix for
|
||||
* execute-phase.md (bug #3212, commit e7942c21b).
|
||||
*
|
||||
* The fix extracts the decision logic into a pure, unit-testable bash
|
||||
* function (`gsd_stall_should_recover`) embedded in the lazily-loaded
|
||||
* `gsd-core/workflows/plan-phase/steps/stall-detection-helpers.md` (kept out
|
||||
* of plan-phase.md's own measured bytes — plan-phase.md is frozen under the
|
||||
* ADR-857 Phase 6 `PRE_PHASE6` gate, `tests/phase6-capstone-conformance.test.cjs`,
|
||||
* with ~36 bytes of headroom at baseline) and exercised here via the SAME
|
||||
* extraction pattern already used by tests/worktree-cleanup.test.cjs
|
||||
* (extractCwdGuardBash) and tests/quick-branching.test.cjs
|
||||
* (extractStep25Bash) — the test runs the exact shipped bash, not a
|
||||
* hand-copied duplicate (avoids the "Generative Fix Divergence" defect
|
||||
* class).
|
||||
*
|
||||
* Seam: gsd-core/workflows/plan-phase.md,
|
||||
* gsd-core/workflows/plan-phase/steps/stall-detection-helpers.md,
|
||||
* src/config.cts (SCHEMA_DEFAULTS),
|
||||
* gsd-core/bin/shared/config-schema.manifest.json, docs/CONFIGURATION.md
|
||||
*/
|
||||
|
||||
const { describe, test } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const { spawnSync } = require('node:child_process');
|
||||
const fs = require('node:fs');
|
||||
const os = require('node:os');
|
||||
const path = require('node:path');
|
||||
const fc = require('fast-check');
|
||||
const { cleanup, readFileNormalized, readWorkflowCombined } = require('./helpers.cjs');
|
||||
|
||||
const REPO_ROOT = path.join(__dirname, '..');
|
||||
const PLAN_PHASE_PATH = path.join(REPO_ROOT, 'gsd-core', 'workflows', 'plan-phase.md');
|
||||
const STALL_HELPERS_PATH = path.join(REPO_ROOT, 'gsd-core', 'workflows', 'plan-phase', 'steps', 'stall-detection-helpers.md');
|
||||
const CHUNKED_PLANNING_MODE_PATH = path.join(REPO_ROOT, 'gsd-core', 'workflows', 'plan-phase', 'steps', 'chunked-planning-mode.md');
|
||||
const CONFIG_SCHEMA_MANIFEST_PATH = path.join(REPO_ROOT, 'gsd-core', 'bin', 'shared', 'config-schema.manifest.json');
|
||||
const CONFIGURATION_DOCS_PATH = path.join(REPO_ROOT, 'docs', 'CONFIGURATION.md');
|
||||
|
||||
function readPlanPhase() {
|
||||
return readFileNormalized(PLAN_PHASE_PATH);
|
||||
}
|
||||
|
||||
// #2993 relocated plan-phase.md's chunked-planning-mode spawn sites into this
|
||||
// lazily-loaded step file. Read directly rather than via the generic
|
||||
// readWorkflowCombined() blob when a test needs to slice a SPECIFIC section by
|
||||
// heading-to-heading boundaries: chunked-planning-mode.md is small and
|
||||
// self-contained (8.5.1 immediately followed by 8.5.2, nothing else), so its
|
||||
// own heading boundaries stay precise, whereas the combined multi-file blob's
|
||||
// ordering (host file, then every steps/*.md sorted by filename) would put an
|
||||
// unrelated step file's content between "### 8.5.2 Per-Plan Tasks" and any
|
||||
// downstream anchor a slice tried to search for.
|
||||
function readChunkedPlanningMode() {
|
||||
return readFileNormalized(CHUNKED_PLANNING_MODE_PATH);
|
||||
}
|
||||
|
||||
function readStallHelpersDoc() {
|
||||
return readFileNormalized(STALL_HELPERS_PATH);
|
||||
}
|
||||
|
||||
/**
|
||||
* Extract the ```bash fence that defines gsd_stall_should_recover (and its
|
||||
* sibling gsd_stall_watch) from the lazily-loaded stall-detection-helpers.md
|
||||
* step file. Throws with a clear message if the anchor or fence cannot be
|
||||
* found — this is what makes row 1 of the test matrix a genuine failing-first
|
||||
* regression test (pre-fix, the function does not exist anywhere in the repo).
|
||||
*/
|
||||
function extractStallHelpersBash() {
|
||||
const content = readStallHelpersDoc();
|
||||
|
||||
const anchor = 'gsd_stall_should_recover';
|
||||
const anchorIdx = content.indexOf(anchor);
|
||||
if (anchorIdx === -1) {
|
||||
throw new Error(`extractStallHelpersBash: could not find "${anchor}" anywhere in ${STALL_HELPERS_PATH}`);
|
||||
}
|
||||
|
||||
// Walk backward to the start of the fenced ```bash block containing the anchor.
|
||||
const before = content.slice(0, anchorIdx);
|
||||
const fenceOpenRe = /```bash\r?\n/g;
|
||||
let lastOpen = -1;
|
||||
let m;
|
||||
while ((m = fenceOpenRe.exec(before)) !== null) {
|
||||
lastOpen = m.index + m[0].length;
|
||||
}
|
||||
if (lastOpen === -1) {
|
||||
throw new Error(`extractStallHelpersBash: "${anchor}" is not inside a \`\`\`bash fence in ${STALL_HELPERS_PATH}`);
|
||||
}
|
||||
|
||||
const after = content.slice(lastOpen);
|
||||
const closeIdx = after.indexOf('```');
|
||||
if (closeIdx === -1) {
|
||||
throw new Error('extractStallHelpersBash: unterminated ```bash fence');
|
||||
}
|
||||
|
||||
const body = after.slice(0, closeIdx);
|
||||
if (!body.includes('gsd_stall_watch')) {
|
||||
throw new Error('extractStallHelpersBash: sanity check failed — extracted block does not also define gsd_stall_watch');
|
||||
}
|
||||
// readStallHelpersDoc() reads through helpers.cjs's readFileNormalized(),
|
||||
// which strips \r\n -> \n at the read boundary before any slicing above
|
||||
// runs. That guards against the repo's general CRLF-in-extracted-source
|
||||
// defect class (#1700) and is worth keeping on its own merits (a bare \n
|
||||
// regex against readFileSync content is fragile either way), but it is
|
||||
// NOT what caused the #2650 Windows CI failure: .gitattributes forces
|
||||
// `eol=lf` on this file, so a Windows checkout never receives CRLF here
|
||||
// in the first place. The real cause, confirmed by evidence rather than
|
||||
// argument: passing this file's ~73-line, quote-dense script body as a
|
||||
// single `bash -c <script>` argv element does not survive Windows argv
|
||||
// serialization (Node has no execve there; CreateProcess flattens the
|
||||
// whole argv into one command-line string, and Git Bash's MSYS layer
|
||||
// re-splits and unescapes it with its own rules — the script itself gets
|
||||
// mangled in transit, not just the boundary around it). Proven by an
|
||||
// A/B on real CI: converting only runShouldRecover() to the temp-file
|
||||
// form below took Windows from 11 failures to 4, and flipped
|
||||
// `full test (windows-latest, 22, shard 1/3)` and `shard 2/3` from fail
|
||||
// to pass — while runWatch() (no extra positional args at all, values
|
||||
// embedded directly in the script text) still failed identically to
|
||||
// before, so the trailing-args theory is ruled out: it is script size
|
||||
// and quote density, not argv-element count. `runBashScript()` below
|
||||
// (used by every call site in this file) removes the script from `-c`
|
||||
// transport entirely by writing it to a file and running it by path.
|
||||
// tests/worktree-cleanup.test.cjs's extractCwdGuardBash/runGuard stays on
|
||||
// `bash -c` and is green on Windows only because its script is small
|
||||
// enough to round-trip that transport intact.
|
||||
return body;
|
||||
}
|
||||
|
||||
/**
|
||||
* Write `script` to a fresh temp file and run it as `bash <file> <args...>`
|
||||
* rather than `bash -c <script> <args...>` (#2650 Windows CI — see
|
||||
* extractStallHelpersBash()'s doc comment for the full evidence trail: a
|
||||
* quote-dense multi-line script does not survive Windows argv
|
||||
* serialization when passed as a `-c` argv element, regardless of how many
|
||||
* trailing positional args accompany it). Every bash-invoking call site in
|
||||
* this file routes through this one seam so a future call site cannot
|
||||
* silently reintroduce the transport bug in isolation. Cleans up the temp
|
||||
* dir in `finally` regardless of outcome.
|
||||
*
|
||||
* @param {string} script the full bash script body (helpers + a final call)
|
||||
* @param {string[]} [args] positional args passed to the script (become
|
||||
* $1, $2, ... inside it) — empty when the caller embeds values directly
|
||||
* into the script text instead (e.g. via JSON.stringify).
|
||||
* @param {object} [opts] extra spawnSync options (e.g. `{ timeout }`),
|
||||
* merged over the `{ encoding: 'utf-8' }` default.
|
||||
*/
|
||||
function runBashScript(script, args = [], opts = {}) {
|
||||
const scriptDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2650-sh-'));
|
||||
try {
|
||||
const scriptPath = path.join(scriptDir, 'script.sh');
|
||||
fs.writeFileSync(scriptPath, `#!/usr/bin/env bash\n${script}`, { mode: 0o755 });
|
||||
return spawnSync('bash', [scriptPath, ...args], { encoding: 'utf-8', timeout: 10000, ...opts });
|
||||
} finally {
|
||||
cleanup(scriptDir);
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Run gsd_stall_should_recover with the given args inside the extracted
|
||||
* script and return its stdout (trimmed). No real sleeping happens — the
|
||||
* function is pure and synchronous.
|
||||
*/
|
||||
function runShouldRecover(helpersBash, elapsedSeconds, thresholdMinutes, markerFound, artifactFresh) {
|
||||
const script = `${helpersBash}\ngsd_stall_should_recover "$1" "$2" "$3" "$4"\n`;
|
||||
const result = runBashScript(script,
|
||||
[String(elapsedSeconds), String(thresholdMinutes), String(markerFound), String(artifactFresh)]);
|
||||
assert.equal(result.status, 0, `gsd_stall_should_recover exited non-zero: ${result.stderr}`);
|
||||
return result.stdout.trim();
|
||||
}
|
||||
|
||||
describe('bug #2650 plan-phase stall detection — gsd_stall_should_recover (pure decision function)', () => {
|
||||
let helpersBash;
|
||||
|
||||
test('stall-detection-helpers.md defines gsd_stall_should_recover inside a ```bash fence', () => {
|
||||
helpersBash = extractStallHelpersBash();
|
||||
assert.ok(helpersBash.length > 0);
|
||||
});
|
||||
|
||||
test('boundary — one second under threshold keeps waiting (limit-1)', () => {
|
||||
const result = runShouldRecover(helpersBash, 599, 10, 'false', 'false'); // 10min = 600s
|
||||
assert.equal(result, 'waiting');
|
||||
});
|
||||
|
||||
test('boundary — exactly at threshold stalls (limit)', () => {
|
||||
const result = runShouldRecover(helpersBash, 600, 10, 'false', 'false');
|
||||
assert.equal(result, 'stalled');
|
||||
});
|
||||
|
||||
test('boundary — one second past threshold stalls (limit+1)', () => {
|
||||
const result = runShouldRecover(helpersBash, 601, 10, 'false', 'false');
|
||||
assert.equal(result, 'stalled');
|
||||
});
|
||||
|
||||
test('marker found short-circuits regardless of elapsed time', () => {
|
||||
assert.equal(runShouldRecover(helpersBash, 0, 10, 'true', 'false'), 'marker_received');
|
||||
assert.equal(runShouldRecover(helpersBash, 99999, 10, 'true', 'false'), 'marker_received');
|
||||
});
|
||||
|
||||
test('fresh artifact activity keeps waiting even past threshold (no false-fire while planner is actively writing)', () => {
|
||||
assert.equal(runShouldRecover(helpersBash, 99999, 10, 'false', 'true'), 'active');
|
||||
});
|
||||
|
||||
test('default threshold (10 min) does not false-fire on a normal 1-5 minute planner run (AC3)', () => {
|
||||
// A normal run returns (marker_found=true) well before 300s (5 min).
|
||||
assert.equal(runShouldRecover(helpersBash, 300, 10, 'true', 'false'), 'marker_received');
|
||||
// And absent a marker, 5 minutes of pure silence is still "waiting", not "stalled".
|
||||
assert.equal(runShouldRecover(helpersBash, 300, 10, 'false', 'false'), 'waiting');
|
||||
});
|
||||
|
||||
test('property — stalled iff elapsed seconds >= threshold minutes*60 (when no marker, no fresh activity)', () => {
|
||||
fc.assert(
|
||||
fc.property(
|
||||
fc.integer({ min: 0, max: 60 * 60 * 6 }),
|
||||
fc.integer({ min: 1, max: 120 }),
|
||||
(elapsedSeconds, thresholdMinutes) => {
|
||||
const result = runShouldRecover(helpersBash, elapsedSeconds, thresholdMinutes, 'false', 'false');
|
||||
const shouldStall = elapsedSeconds >= thresholdMinutes * 60;
|
||||
return shouldStall ? result === 'stalled' : result === 'waiting';
|
||||
},
|
||||
),
|
||||
{ numRuns: 25 },
|
||||
);
|
||||
});
|
||||
|
||||
test('a malformed threshold_minutes value degrades to the safe default instead of crashing the watcher', (t) => {
|
||||
// A security review initially flagged this as a command-injection path
|
||||
// (bash arithmetic recursively re-evaluating a `$(cmd)`-shaped string).
|
||||
// Empirically disproven: bash's arithmetic evaluator hard-errors on such
|
||||
// an operand ("syntax error: operand expected") rather than invoking it —
|
||||
// verified directly against both macOS bash 3.2.57 and Docker bash:5; the
|
||||
// payload command never runs on either. The REAL risk this guard closes
|
||||
// is reliability, not RCE: without validation, a malformed
|
||||
// `planner.stall_threshold_minutes` config value would abort the
|
||||
// stall-watcher itself with that bash syntax error, silently defeating
|
||||
// the exact hang-recovery this issue exists to ship. Prove the function
|
||||
// degrades to a safe default instead of erroring.
|
||||
const marker = `gsd-2650-untouched-${process.pid}-${Date.now()}`;
|
||||
const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2650-malformed-'));
|
||||
t.after(() => cleanup(tmp));
|
||||
const payload = `$(touch ${path.join(tmp, marker)})`;
|
||||
const result = runShouldRecover(helpersBash, 0, payload, 'false', 'false');
|
||||
// Must not error (proves the guard prevents the bash-abort), must not
|
||||
// have run the embedded command either way, and must fall back to the
|
||||
// safe default classification (threshold_minutes -> 10 -> elapsed 0 < 600 -> waiting).
|
||||
assert.equal(result, 'waiting');
|
||||
assert.equal(fs.existsSync(path.join(tmp, marker)), false, 'payload must not execute (also true without the guard — bash hard-errors on it instead)');
|
||||
});
|
||||
});
|
||||
|
||||
describe('bug #2650 plan-phase stall detection — gsd_stall_watch (real execution, not just the pure classifier)', () => {
|
||||
// Sourcing the extracted script without `gsd_run` defined naturally exercises
|
||||
// the `|| echo "<default>"` fallback already in the config-get lines (command
|
||||
// lookup fails -> non-zero exit -> the `||` branch fires), so
|
||||
// PLANNER_STALL_INTERVAL_MINUTES/PLANNER_STALL_THRESHOLD_MINUTES start at their
|
||||
// real defaults (5/10) here; each test overrides them afterward for speed.
|
||||
let helpersBash;
|
||||
let tmp;
|
||||
|
||||
test('loads helpers', () => {
|
||||
helpersBash = extractStallHelpersBash();
|
||||
assert.ok(helpersBash.includes('gsd_stall_watch()'));
|
||||
});
|
||||
|
||||
// Routed through the shared runBashScript() helper (#2650 Windows CI —
|
||||
// see extractStallHelpersBash()'s doc comment for the full evidence
|
||||
// trail). The call line is still built with JSON.stringify exactly as
|
||||
// before — that part was never the problem and correctly keeps Windows
|
||||
// paths and the injection-guard payload intact; only the transport of
|
||||
// the script itself changes.
|
||||
function runWatch(intervalMinutes, thresholdMinutes, dispatchTs, outputFile, artifactGlob, markers) {
|
||||
const overrides = `PLANNER_STALL_INTERVAL_MINUTES=${intervalMinutes}\nPLANNER_STALL_THRESHOLD_MINUTES=${thresholdMinutes}\n`;
|
||||
const call = `gsd_stall_watch ${JSON.stringify(String(dispatchTs))} ${JSON.stringify(outputFile)} ${JSON.stringify(artifactGlob)}` +
|
||||
markers.map((m) => ` ${JSON.stringify(m)}`).join('');
|
||||
const script = `${helpersBash}\n${overrides}${call}\n`;
|
||||
return runBashScript(script, [], { timeout: 10000 });
|
||||
}
|
||||
|
||||
test('marker present in the real output file (via real grep, interval=0 so sleep is instant) -> marker_received', (t) => {
|
||||
tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2650-watch-'));
|
||||
t.after(() => cleanup(tmp));
|
||||
const outputFile = path.join(tmp, 'agent-output.txt');
|
||||
fs.writeFileSync(outputFile, 'some agent output\n## PLANNING COMPLETE\nmore text\n');
|
||||
const glob = `${tmp.replace(/\\/g, '/')}/*-PLAN.md`;
|
||||
const now = Math.floor(Date.now() / 1000);
|
||||
const result = runWatch(0, 10, now, outputFile, glob, ['## PLANNING COMPLETE']);
|
||||
assert.equal(result.status, 0, result.stderr);
|
||||
assert.equal(result.stdout.trim(), 'marker_received');
|
||||
});
|
||||
|
||||
test('no marker, no output file, dispatch far in the past, threshold=0 (via real find/date, interval=0) -> stalled', (t) => {
|
||||
tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2650-watch-'));
|
||||
t.after(() => cleanup(tmp));
|
||||
const missingOutputFile = path.join(tmp, 'never-written.txt');
|
||||
const glob = `${tmp.replace(/\\/g, '/')}/*-PLAN.md`; // the tmp dir contains no *-PLAN.md files -> no fresh activity
|
||||
const longAgo = Math.floor(Date.now() / 1000) - 999999;
|
||||
const result = runWatch(0, 0, longAgo, missingOutputFile, glob, ['## PLANNING COMPLETE']);
|
||||
assert.equal(result.status, 0, result.stderr);
|
||||
assert.equal(result.stdout.trim(), 'stalled');
|
||||
});
|
||||
|
||||
test('marker absent, dispatch just now, non-zero threshold (via real find/date, interval=0) -> waiting', (t) => {
|
||||
tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2650-watch-'));
|
||||
t.after(() => cleanup(tmp));
|
||||
const missingOutputFile = path.join(tmp, 'never-written.txt');
|
||||
const glob = `${tmp.replace(/\\/g, '/')}/*-PLAN.md`;
|
||||
const now = Math.floor(Date.now() / 1000);
|
||||
const result = runWatch(0, 10, now, missingOutputFile, glob, ['## PLANNING COMPLETE']);
|
||||
assert.equal(result.status, 0, result.stderr);
|
||||
assert.equal(result.stdout.trim(), 'waiting');
|
||||
});
|
||||
|
||||
test('real `find ... -mmin` correctly detects a fresh artifact -> active (CR: BSD find -newermt "@epoch" is unparseable on macOS)', (t) => {
|
||||
// Regression for a production (not test-only) defect a review surfaced:
|
||||
// the shipped freshness check used to be `find $glob -newermt "@$(( ...
|
||||
// ))" ` — GNU find's "@<epoch>" shorthand for -newermt, which the
|
||||
// BSD find(1) actually shipped on macOS does NOT understand ("Can't
|
||||
// parse date/time: @<epoch>", verified live against /usr/bin/find). With
|
||||
// the `2>/dev/null` beside it, that failed silently and permanently
|
||||
// degraded artifact_fresh to "false" on every macOS run — a real
|
||||
// plan-checker or planner actively writing plan files could still be
|
||||
// reported "stalled". Fixed to `find $glob -mmin -N` ("modified less
|
||||
// than N minutes ago"), which needs no date-string parsing and is
|
||||
// supported identically by GNU find and BSD find.
|
||||
//
|
||||
// This runs the REAL shipped gsd_stall_watch (not a hand-copied
|
||||
// find invocation — see this file's header on Generative Fix
|
||||
// Divergence), with `sleep` shadowed to a no-op bash function so the
|
||||
// test does not actually wait a real PLANNER_STALL_INTERVAL_MINUTES;
|
||||
// the `find ... -mmin` line itself still executes for real. threshold
|
||||
// is set absurdly high so "stalled" cannot fire independently — the
|
||||
// ONLY path to "active" is a correctly-working freshness check.
|
||||
// Routed through runBashScript() (#2650 Windows CI) rather than a raw
|
||||
// `bash -c` call — this test builds its own script inline (the `sleep`
|
||||
// stub isn't something runWatch() supports), so it needs the same
|
||||
// transport seam explicitly rather than inheriting it for free.
|
||||
// Windows CR: production's own glob (plan-phase.md:895 et al.,
|
||||
// `"${PHASE_DIR}"'/*-PLAN.md'`) is always forward-slash — PHASE_DIR is a
|
||||
// POSIX-style `.planning/phases/NN-slug` value, never a native Windows
|
||||
// path, and this all runs under Git Bash regardless of host OS. This
|
||||
// test previously built the glob with `path.join(tmp, '*-PLAN.md')`,
|
||||
// which on Windows yields a backslash path
|
||||
// (`C:\Users\RUNNER~1\...\*-PLAN.md`). In bash pathname expansion a
|
||||
// backslash escapes the next character, so that pattern can never
|
||||
// match anything — `find` silently returned empty and the test failed
|
||||
// with 'waiting' instead of 'active'. Confirmed as a TEST artifact, not
|
||||
// a production defect: production never constructs the glob this way.
|
||||
// Fixed by forward-slashing the tmp dir before building the glob — the
|
||||
// same `.replace(/\\/g, '/')` idiom this repo already uses elsewhere —
|
||||
// so the test matches what production actually passes, while still
|
||||
// exercising the real shipped `find` line. Do not "simplify" this back
|
||||
// to a bare `path.join`; that silently reintroduces the failure.
|
||||
tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2650-fresh-'));
|
||||
t.after(() => cleanup(tmp));
|
||||
fs.writeFileSync(path.join(tmp, 'x-PLAN.md'), 'freshly written\n');
|
||||
const glob = `${tmp.replace(/\\/g, '/')}/*-PLAN.md`;
|
||||
const missingOutputFile = path.join(tmp, 'never-written.txt');
|
||||
const now = Math.floor(Date.now() / 1000);
|
||||
const overrides = 'sleep() { :; }\nPLANNER_STALL_INTERVAL_MINUTES=1\nPLANNER_STALL_THRESHOLD_MINUTES=99999\n';
|
||||
const call = `gsd_stall_watch ${JSON.stringify(String(now))} ${JSON.stringify(missingOutputFile)} ${JSON.stringify(glob)}` +
|
||||
` ${JSON.stringify('## PLANNING COMPLETE')}`;
|
||||
const script = `${helpersBash}\n${overrides}${call}\n`;
|
||||
const result = runBashScript(script, [], { timeout: 10000 });
|
||||
assert.equal(result.status, 0, result.stderr);
|
||||
assert.equal(result.stdout.trim(), 'active');
|
||||
});
|
||||
// Note: this platform's real find(1) is exercised by the test above via a
|
||||
// stubbed `sleep`, not a real ~60s wait. The mtime-based transition is
|
||||
// ALSO covered deterministically at the pure-function level above
|
||||
// ("fresh artifact activity keeps waiting...") for the classification
|
||||
// logic downstream of a given artifact_fresh value.
|
||||
});
|
||||
|
||||
describe('bug #2650 config schema — planner.stall_* keys mirror executor.stall_*', () => {
|
||||
test('config schemas register planner stall detector keys', () => {
|
||||
const { VALID_CONFIG_KEYS: cjsKeys } = require('../gsd-core/bin/lib/config-schema.cjs');
|
||||
const manifest = JSON.parse(fs.readFileSync(CONFIG_SCHEMA_MANIFEST_PATH, 'utf-8'));
|
||||
const manifestKeys = new Set(manifest.validKeys);
|
||||
|
||||
for (const key of ['planner.stall_detect_interval_minutes', 'planner.stall_threshold_minutes']) {
|
||||
assert.ok(cjsKeys.has(key), `CJS VALID_CONFIG_KEYS must include ${key}`);
|
||||
assert.ok(manifestKeys.has(key), `Manifest validKeys must include ${key} (SDK sources from manifest)`);
|
||||
}
|
||||
});
|
||||
|
||||
test('configuration docs describe planner stall detector defaults', () => {
|
||||
const docs = fs.readFileSync(CONFIGURATION_DOCS_PATH, 'utf-8');
|
||||
assert.match(docs, /`planner\.stall_detect_interval_minutes`\s*\|\s*number\s*\|\s*`5`/);
|
||||
assert.match(docs, /`planner\.stall_threshold_minutes`\s*\|\s*number\s*\|\s*`10`/);
|
||||
});
|
||||
|
||||
test('config-get returns schema defaults for planner stall detector keys', (t) => {
|
||||
const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2650-'));
|
||||
t.after(() => cleanup(tmp));
|
||||
fs.mkdirSync(path.join(tmp, '.planning'));
|
||||
fs.writeFileSync(path.join(tmp, '.planning/config.json'), '{}\n');
|
||||
|
||||
const toolsPath = path.join(REPO_ROOT, 'gsd-core', 'bin', 'gsd-tools.cjs');
|
||||
const interval = spawnSync(process.execPath, [toolsPath, 'config-get', 'planner.stall_detect_interval_minutes', '--raw'], { cwd: tmp, encoding: 'utf-8' });
|
||||
const threshold = spawnSync(process.execPath, [toolsPath, 'config-get', 'planner.stall_threshold_minutes', '--raw'], { cwd: tmp, encoding: 'utf-8' });
|
||||
|
||||
assert.equal(interval.status, 0, interval.stderr);
|
||||
assert.equal(interval.stdout.trim(), '5');
|
||||
assert.equal(threshold.status, 0, threshold.stderr);
|
||||
assert.equal(threshold.stdout.trim(), '10');
|
||||
});
|
||||
});
|
||||
|
||||
describe('bug #2650 plan-phase — all five planner/plan-checker spawns dispatch in the background with bounded stall surveillance', () => {
|
||||
let workflow;
|
||||
|
||||
test('loads', () => {
|
||||
workflow = readPlanPhase();
|
||||
assert.ok(workflow.length > 0);
|
||||
});
|
||||
|
||||
test('plan-phase.md points at the lazily-loaded stall-detection-helpers.md step file (step 7.99)', () => {
|
||||
assert.match(workflow, /gsd-core\/workflows\/plan-phase\/steps\/stall-detection-helpers\.md/);
|
||||
});
|
||||
|
||||
test('stall-detection-helpers.md resolves PLANNER_STALL_INTERVAL_MINUTES / PLANNER_STALL_THRESHOLD_MINUTES from config', () => {
|
||||
const helpersDoc = readStallHelpersDoc();
|
||||
assert.match(helpersDoc, /PLANNER_STALL_INTERVAL_MINUTES=.*planner\.stall_detect_interval_minutes/);
|
||||
assert.match(helpersDoc, /PLANNER_STALL_THRESHOLD_MINUTES=.*planner\.stall_threshold_minutes/);
|
||||
});
|
||||
|
||||
test('standard planner spawn (step 8) dispatches with run_in_background=true and calls gsd_stall_watch', () => {
|
||||
const idx = workflow.indexOf('## 8. Spawn gsd-planner Agent');
|
||||
assert.notEqual(idx, -1);
|
||||
// #2993 moved "## 8.5. Chunked Planning Mode" itself out of plan-phase.md
|
||||
// (now a <!-- gsd:section --> pointer to steps/chunked-planning-mode.md,
|
||||
// asserted separately below) — bound this slice at the next heading that
|
||||
// still actually exists in plan-phase.md instead.
|
||||
const nextSectionIdx = workflow.indexOf('## 9. Handle Planner Return', idx);
|
||||
const section = workflow.slice(idx, nextSectionIdx === -1 ? undefined : nextSectionIdx);
|
||||
assert.match(section, /run_in_background\s*=\s*true/, 'standard planner spawn must set run_in_background=true');
|
||||
assert.match(section, /gsd_stall_watch/, 'standard planner spawn must invoke the bounded stall watcher');
|
||||
assert.match(section, /gsd_stall_watch\s+"\$TS"\s+"\{outputFile\}"/, 'standard planner spawn must bind {outputFile} into the stall watcher call, not a dead bash variable');
|
||||
});
|
||||
|
||||
test('plan-phase.md points at the lazily-loaded chunked-planning-mode.md step file (8.5, #2993)', () => {
|
||||
// #2993 (epic #1671 Phase 6.2, unrelated to #2650) extracted the whole
|
||||
// "Chunked Planning Mode" section into gsd-core/workflows/plan-phase/steps/
|
||||
// chunked-planning-mode.md, leaving a <!-- gsd:section --> pointer behind.
|
||||
// The two stall-watch spawn sites that used to live inline (8.5.1 outline,
|
||||
// 8.5.2 per-plan) moved with it — asserted directly against that file below.
|
||||
assert.match(workflow, /gsd-core\/workflows\/plan-phase\/steps\/chunked-planning-mode\.md/);
|
||||
});
|
||||
|
||||
test('chunked outline spawn (8.5.1) dispatches with run_in_background=true and calls gsd_stall_watch', () => {
|
||||
// Lives in the extracted steps/chunked-planning-mode.md since #2993, not
|
||||
// in plan-phase.md itself — read that file directly (see
|
||||
// readChunkedPlanningMode()'s doc comment for why not the generic
|
||||
// combined-blob reader).
|
||||
const chunkedDoc = readChunkedPlanningMode();
|
||||
const idx = chunkedDoc.indexOf('### 8.5.1 Outline Phase');
|
||||
assert.notEqual(idx, -1);
|
||||
const nextSectionIdx = chunkedDoc.indexOf('### 8.5.2 Per-Plan Tasks', idx);
|
||||
const section = chunkedDoc.slice(idx, nextSectionIdx === -1 ? undefined : nextSectionIdx);
|
||||
assert.match(section, /run_in_background\s*=\s*true/, 'chunked outline spawn must set run_in_background=true');
|
||||
assert.match(section, /gsd_stall_watch/, 'chunked outline spawn must invoke the bounded stall watcher');
|
||||
assert.match(section, /gsd_stall_watch\s+"\$TS"\s+"\{outputFile\}"/, 'chunked outline spawn must bind {outputFile} into the stall watcher call, not a dead bash variable');
|
||||
});
|
||||
|
||||
test('chunked per-plan spawn (8.5.2) dispatches with run_in_background=true and calls gsd_stall_watch', () => {
|
||||
// Same relocation as the outline spawn above (#2993) — read
|
||||
// steps/chunked-planning-mode.md directly. 8.5.2 is the LAST section in
|
||||
// that file, so an unbounded slice to EOF is precise here (unlike slicing
|
||||
// the generic multi-file combined blob, which would run on into whatever
|
||||
// step file sorts next after this one).
|
||||
const chunkedDoc = readChunkedPlanningMode();
|
||||
const idx = chunkedDoc.indexOf('### 8.5.2 Per-Plan Tasks');
|
||||
assert.notEqual(idx, -1);
|
||||
const section = chunkedDoc.slice(idx);
|
||||
assert.match(section, /run_in_background\s*=\s*true/, 'chunked per-plan spawn must set run_in_background=true');
|
||||
assert.match(section, /gsd_stall_watch/, 'chunked per-plan spawn must invoke the bounded stall watcher');
|
||||
assert.match(section, /gsd_stall_watch\s+"\$TS"\s+"\{outputFile\}"/, 'chunked per-plan spawn must bind {outputFile} into the stall watcher call, not a dead bash variable');
|
||||
});
|
||||
|
||||
test('plan-checker spawn (step 10) dispatches with run_in_background=true and calls gsd_stall_watch', () => {
|
||||
const idx = workflow.indexOf('## 10. Spawn gsd-plan-checker Agent');
|
||||
assert.notEqual(idx, -1);
|
||||
const nextSectionIdx = workflow.indexOf('## 11. Handle Checker Return', idx);
|
||||
const section = workflow.slice(idx, nextSectionIdx === -1 ? undefined : nextSectionIdx);
|
||||
assert.match(section, /run_in_background\s*=\s*true/, 'plan-checker spawn must set run_in_background=true');
|
||||
assert.match(section, /gsd_stall_watch/, 'plan-checker spawn must invoke the bounded stall watcher');
|
||||
assert.match(section, /gsd_stall_watch\s+"\$TS"\s+"\{outputFile\}"/, 'plan-checker spawn must bind {outputFile} into the stall watcher call — this is the ONLY completion signal on a clean PASS, since a passing checker touches no *-PLAN.md files');
|
||||
});
|
||||
|
||||
test('revision-loop planner respawn (step 12) dispatches with run_in_background=true and calls gsd_stall_watch', () => {
|
||||
const idx = workflow.indexOf('## 12. Revision Loop');
|
||||
assert.notEqual(idx, -1);
|
||||
const nextSectionIdx = workflow.indexOf('## 12.5. Plan Bounce', idx);
|
||||
const section = workflow.slice(idx, nextSectionIdx === -1 ? undefined : nextSectionIdx);
|
||||
assert.match(section, /run_in_background\s*=\s*true/, 'revision-loop planner respawn must set run_in_background=true');
|
||||
assert.match(section, /gsd_stall_watch/, 'revision-loop planner respawn must invoke the bounded stall watcher');
|
||||
assert.match(section, /gsd_stall_watch\s+"\$TS"\s+"\{outputFile\}"/, 'revision-loop planner respawn must bind {outputFile} into the stall watcher call, not a dead bash variable');
|
||||
});
|
||||
|
||||
test('no spawn site references an unbound $PLANNER_OUTPUT_FILE / $CHECKER_OUTPUT_FILE bash variable', () => {
|
||||
// Regression for the blocker an independent review found: the original
|
||||
// design named PLANNER_OUTPUT_FILE/CHECKER_OUTPUT_FILE as bash variables
|
||||
// in the gsd_stall_watch calls, but nothing in plan-phase.md ever ASSIGNED
|
||||
// them — with the variable permanently empty, `[ -f "$output_file" ]` is
|
||||
// always false, marker_found can never become true, and marker_received is
|
||||
// unreachable. Worse for the plan-checker spawn specifically: a checker
|
||||
// that PASSES touches no *-PLAN.md files, so it has NO working completion
|
||||
// signal at all without the marker path — a healthy, already-succeeded
|
||||
// checker would be reported as stalled. The fix replaces the dead bash
|
||||
// variable with the `{outputFile}` orchestrator-substitution token (the
|
||||
// same convention docs-update.md:471 already uses for a real
|
||||
// run_in_background=true Agent() return). This test proves the dead
|
||||
// variable name is gone from every spawn site, not just that
|
||||
// gsd_stall_watch behaves correctly when handed a valid argument
|
||||
// (tests/fix-2650-plan-phase-stall-detection.test.cjs's gsd_stall_watch
|
||||
// describe block below already covers that half — this covers the
|
||||
// production wiring the previous tests never exercised).
|
||||
assert.doesNotMatch(workflow, /\$PLANNER_OUTPUT_FILE\b/, 'plan-phase.md must not reference an unassigned $PLANNER_OUTPUT_FILE bash variable');
|
||||
assert.doesNotMatch(workflow, /\$CHECKER_OUTPUT_FILE\b/, 'plan-phase.md must not reference an unassigned $CHECKER_OUTPUT_FILE bash variable');
|
||||
// #2993 moved two of the five spawn sites into steps/chunked-planning-mode.md
|
||||
// — check there too, not just plan-phase.md, now that it's a separate file.
|
||||
const chunkedDoc = readChunkedPlanningMode();
|
||||
assert.doesNotMatch(chunkedDoc, /\$PLANNER_OUTPUT_FILE\b/, 'chunked-planning-mode.md must not reference an unassigned $PLANNER_OUTPUT_FILE bash variable');
|
||||
assert.doesNotMatch(chunkedDoc, /\$CHECKER_OUTPUT_FILE\b/, 'chunked-planning-mode.md must not reference an unassigned $CHECKER_OUTPUT_FILE bash variable');
|
||||
});
|
||||
|
||||
test('exactly five gsd_stall_watch spawn-site invocations exist across plan-phase.md and its steps/*.md files', () => {
|
||||
// The whole point of #2650 is that EVERY planner/plan-checker spawn is
|
||||
// bounded — not "at least one". #2993 relocated two of the five call
|
||||
// sites (chunked outline, chunked per-plan) into
|
||||
// steps/chunked-planning-mode.md; this counts across the combined
|
||||
// surface so a future relocation can't silently drop a site without a
|
||||
// test noticing (mirrors tests/plan-phase-drift-guard.test.cjs's #913
|
||||
// ORCHESTRATOR RULE label count, which already does this).
|
||||
const combined = readWorkflowCombined(PLAN_PHASE_PATH);
|
||||
const callCount = (combined.match(/gsd_stall_watch\s+"\$TS"\s+"\{outputFile\}"/g) || []).length;
|
||||
assert.equal(callCount, 5,
|
||||
`expected exactly 5 gsd_stall_watch "$TS" "{outputFile}" spawn-site invocations across plan-phase.md + steps/*.md, found ${callCount}`);
|
||||
});
|
||||
|
||||
test('step 7.99 documents that {outputFile} must be bound from the real Agent() return (not passed literally)', () => {
|
||||
const idx = workflow.indexOf('## 7.99. Bounded Stall-Detection Helpers');
|
||||
assert.notEqual(idx, -1);
|
||||
const nextSectionIdx = workflow.indexOf('## 8. Spawn gsd-planner Agent', idx);
|
||||
const section = workflow.slice(idx, nextSectionIdx === -1 ? undefined : nextSectionIdx);
|
||||
assert.match(section, /\{outputFile\}/, 'step 7.99 must mention {outputFile} so a reader knows it is a binding token, not literal text');
|
||||
// The full binding contract (docs-update.md precedent, why a bash variable
|
||||
// does not work, and the plan-checker completion-signal implication) lives
|
||||
// in the lazily-loaded reference file to stay under the PRE_PHASE6 cap —
|
||||
// verify it is actually there, not just gestured at.
|
||||
const helpersDoc = readStallHelpersDoc();
|
||||
assert.match(helpersDoc, /\{outputFile\}/, 'stall-detection-helpers.md must explain the {outputFile} binding contract');
|
||||
assert.match(helpersDoc, /docs-update\.md/i, 'stall-detection-helpers.md must cite the docs-update.md precedent for {outputFile} substitution');
|
||||
assert.match(helpersDoc, /plan-checker/i, 'stall-detection-helpers.md must explain why binding {outputFile} is load-bearing for the plan-checker spawn specifically');
|
||||
});
|
||||
|
||||
test('stall surveillance is not gated behind the teams-status guard (AC2)', () => {
|
||||
// The only actual `query teams-status` CALL in plan-phase.md must stay
|
||||
// scoped to the researcher spawn banner (its pre-existing, unrelated
|
||||
// purpose) — the new stall blocks must not add a second call site or make
|
||||
// their own behavior conditional on it. The helpers doc is allowed (and
|
||||
// expected) to name "teams-status" in prose explaining that independence
|
||||
// (AC2 self-documentation) — what must never appear is a SECOND `query
|
||||
// teams-status` invocation, or any conditional gating on its result.
|
||||
const teamsStatusCallOccurrences = workflow.split('query teams-status').length - 1;
|
||||
assert.equal(teamsStatusCallOccurrences, 1, 'teams-status guard must remain scoped to its single pre-existing call site');
|
||||
assert.doesNotMatch(readStallHelpersDoc(), /query teams-status/, 'stall-detection helpers must not add their own teams-status call site');
|
||||
});
|
||||
|
||||
test('completion-marker contract is unchanged (AC4)', () => {
|
||||
for (const marker of ['## PLANNING COMPLETE', '## CHECKPOINT REACHED', '## VERIFICATION PASSED', '## ISSUES FOUND', '## PLANNING INCONCLUSIVE']) {
|
||||
assert.ok(workflow.includes(marker), `completion-marker contract must still include ${marker}`);
|
||||
}
|
||||
});
|
||||
|
||||
test('researcher (line ~404) and pattern-mapper (line ~681) spawns are untouched (out of scope)', () => {
|
||||
const researcherIdx = workflow.indexOf('### Spawn gsd-phase-researcher');
|
||||
const patternMapperIdx = workflow.indexOf('## 7.8. Spawn gsd-pattern-mapper Agent');
|
||||
assert.notEqual(researcherIdx, -1);
|
||||
assert.notEqual(patternMapperIdx, -1);
|
||||
const researcherSection = workflow.slice(researcherIdx, workflow.indexOf('### Handle Researcher Return'));
|
||||
const patternMapperSection = workflow.slice(patternMapperIdx, workflow.indexOf('## 7.9. Regenerate API-SURFACE.md'));
|
||||
assert.doesNotMatch(researcherSection, /gsd_stall_watch/, 'researcher spawn must remain a plain blocking call (out of scope per Agent Brief)');
|
||||
assert.doesNotMatch(patternMapperSection, /gsd_stall_watch/, 'pattern-mapper spawn must remain a plain blocking call (out of scope per Agent Brief)');
|
||||
});
|
||||
});
|
||||
1
tests/fixtures/install-tree/antigravity.json
vendored
1
tests/fixtures/install-tree/antigravity.json
vendored
@@ -299,6 +299,7 @@
|
||||
"gsd-core/workflows/plan-phase/steps/research-only-early-exit.md",
|
||||
"gsd-core/workflows/plan-phase/steps/research-only-modifiers.md",
|
||||
"gsd-core/workflows/plan-phase/steps/reviews-prerequisite.md",
|
||||
"gsd-core/workflows/plan-phase/steps/stall-detection-helpers.md",
|
||||
"gsd-core/workflows/plan-phase/steps/windows-troubleshooting.md",
|
||||
"gsd-core/workflows/plan-review-convergence.md",
|
||||
"gsd-core/workflows/plant-seed.md",
|
||||
|
||||
1
tests/fixtures/install-tree/augment.json
vendored
1
tests/fixtures/install-tree/augment.json
vendored
@@ -370,6 +370,7 @@
|
||||
"gsd-core/workflows/plan-phase/steps/research-only-early-exit.md",
|
||||
"gsd-core/workflows/plan-phase/steps/research-only-modifiers.md",
|
||||
"gsd-core/workflows/plan-phase/steps/reviews-prerequisite.md",
|
||||
"gsd-core/workflows/plan-phase/steps/stall-detection-helpers.md",
|
||||
"gsd-core/workflows/plan-phase/steps/windows-troubleshooting.md",
|
||||
"gsd-core/workflows/plan-review-convergence.md",
|
||||
"gsd-core/workflows/plant-seed.md",
|
||||
|
||||
@@ -369,6 +369,7 @@
|
||||
"gsd-core/workflows/plan-phase/steps/research-only-early-exit.md",
|
||||
"gsd-core/workflows/plan-phase/steps/research-only-modifiers.md",
|
||||
"gsd-core/workflows/plan-phase/steps/reviews-prerequisite.md",
|
||||
"gsd-core/workflows/plan-phase/steps/stall-detection-helpers.md",
|
||||
"gsd-core/workflows/plan-phase/steps/windows-troubleshooting.md",
|
||||
"gsd-core/workflows/plan-review-convergence.md",
|
||||
"gsd-core/workflows/plant-seed.md",
|
||||
|
||||
1
tests/fixtures/install-tree/claude.json
vendored
1
tests/fixtures/install-tree/claude.json
vendored
@@ -298,6 +298,7 @@
|
||||
"gsd-core/workflows/plan-phase/steps/research-only-early-exit.md",
|
||||
"gsd-core/workflows/plan-phase/steps/research-only-modifiers.md",
|
||||
"gsd-core/workflows/plan-phase/steps/reviews-prerequisite.md",
|
||||
"gsd-core/workflows/plan-phase/steps/stall-detection-helpers.md",
|
||||
"gsd-core/workflows/plan-phase/steps/windows-troubleshooting.md",
|
||||
"gsd-core/workflows/plan-review-convergence.md",
|
||||
"gsd-core/workflows/plant-seed.md",
|
||||
|
||||
1
tests/fixtures/install-tree/cline.json
vendored
1
tests/fixtures/install-tree/cline.json
vendored
@@ -302,6 +302,7 @@
|
||||
"gsd-core/workflows/plan-phase/steps/research-only-early-exit.md",
|
||||
"gsd-core/workflows/plan-phase/steps/research-only-modifiers.md",
|
||||
"gsd-core/workflows/plan-phase/steps/reviews-prerequisite.md",
|
||||
"gsd-core/workflows/plan-phase/steps/stall-detection-helpers.md",
|
||||
"gsd-core/workflows/plan-phase/steps/windows-troubleshooting.md",
|
||||
"gsd-core/workflows/plan-review-convergence.md",
|
||||
"gsd-core/workflows/plant-seed.md",
|
||||
|
||||
1
tests/fixtures/install-tree/codebuddy.json
vendored
1
tests/fixtures/install-tree/codebuddy.json
vendored
@@ -370,6 +370,7 @@
|
||||
"gsd-core/workflows/plan-phase/steps/research-only-early-exit.md",
|
||||
"gsd-core/workflows/plan-phase/steps/research-only-modifiers.md",
|
||||
"gsd-core/workflows/plan-phase/steps/reviews-prerequisite.md",
|
||||
"gsd-core/workflows/plan-phase/steps/stall-detection-helpers.md",
|
||||
"gsd-core/workflows/plan-phase/steps/windows-troubleshooting.md",
|
||||
"gsd-core/workflows/plan-review-convergence.md",
|
||||
"gsd-core/workflows/plant-seed.md",
|
||||
|
||||
1
tests/fixtures/install-tree/codex.json
vendored
1
tests/fixtures/install-tree/codex.json
vendored
@@ -405,6 +405,7 @@
|
||||
"gsd-core/workflows/plan-phase/steps/research-only-early-exit.md",
|
||||
"gsd-core/workflows/plan-phase/steps/research-only-modifiers.md",
|
||||
"gsd-core/workflows/plan-phase/steps/reviews-prerequisite.md",
|
||||
"gsd-core/workflows/plan-phase/steps/stall-detection-helpers.md",
|
||||
"gsd-core/workflows/plan-phase/steps/windows-troubleshooting.md",
|
||||
"gsd-core/workflows/plan-review-convergence.md",
|
||||
"gsd-core/workflows/plant-seed.md",
|
||||
|
||||
1
tests/fixtures/install-tree/copilot.json
vendored
1
tests/fixtures/install-tree/copilot.json
vendored
@@ -300,6 +300,7 @@
|
||||
"gsd-core/workflows/plan-phase/steps/research-only-early-exit.md",
|
||||
"gsd-core/workflows/plan-phase/steps/research-only-modifiers.md",
|
||||
"gsd-core/workflows/plan-phase/steps/reviews-prerequisite.md",
|
||||
"gsd-core/workflows/plan-phase/steps/stall-detection-helpers.md",
|
||||
"gsd-core/workflows/plan-phase/steps/windows-troubleshooting.md",
|
||||
"gsd-core/workflows/plan-review-convergence.md",
|
||||
"gsd-core/workflows/plant-seed.md",
|
||||
|
||||
1
tests/fixtures/install-tree/cursor.json
vendored
1
tests/fixtures/install-tree/cursor.json
vendored
@@ -370,6 +370,7 @@
|
||||
"gsd-core/workflows/plan-phase/steps/research-only-early-exit.md",
|
||||
"gsd-core/workflows/plan-phase/steps/research-only-modifiers.md",
|
||||
"gsd-core/workflows/plan-phase/steps/reviews-prerequisite.md",
|
||||
"gsd-core/workflows/plan-phase/steps/stall-detection-helpers.md",
|
||||
"gsd-core/workflows/plan-phase/steps/windows-troubleshooting.md",
|
||||
"gsd-core/workflows/plan-review-convergence.md",
|
||||
"gsd-core/workflows/plant-seed.md",
|
||||
|
||||
1
tests/fixtures/install-tree/hermes.json
vendored
1
tests/fixtures/install-tree/hermes.json
vendored
@@ -299,6 +299,7 @@
|
||||
"gsd-core/workflows/plan-phase/steps/research-only-early-exit.md",
|
||||
"gsd-core/workflows/plan-phase/steps/research-only-modifiers.md",
|
||||
"gsd-core/workflows/plan-phase/steps/reviews-prerequisite.md",
|
||||
"gsd-core/workflows/plan-phase/steps/stall-detection-helpers.md",
|
||||
"gsd-core/workflows/plan-phase/steps/windows-troubleshooting.md",
|
||||
"gsd-core/workflows/plan-review-convergence.md",
|
||||
"gsd-core/workflows/plant-seed.md",
|
||||
|
||||
1
tests/fixtures/install-tree/kilo.json
vendored
1
tests/fixtures/install-tree/kilo.json
vendored
@@ -370,6 +370,7 @@
|
||||
"gsd-core/workflows/plan-phase/steps/research-only-early-exit.md",
|
||||
"gsd-core/workflows/plan-phase/steps/research-only-modifiers.md",
|
||||
"gsd-core/workflows/plan-phase/steps/reviews-prerequisite.md",
|
||||
"gsd-core/workflows/plan-phase/steps/stall-detection-helpers.md",
|
||||
"gsd-core/workflows/plan-phase/steps/windows-troubleshooting.md",
|
||||
"gsd-core/workflows/plan-review-convergence.md",
|
||||
"gsd-core/workflows/plant-seed.md",
|
||||
|
||||
1
tests/fixtures/install-tree/kimi-code.json
vendored
1
tests/fixtures/install-tree/kimi-code.json
vendored
@@ -329,6 +329,7 @@
|
||||
"gsd-core/workflows/plan-phase/steps/research-only-early-exit.md",
|
||||
"gsd-core/workflows/plan-phase/steps/research-only-modifiers.md",
|
||||
"gsd-core/workflows/plan-phase/steps/reviews-prerequisite.md",
|
||||
"gsd-core/workflows/plan-phase/steps/stall-detection-helpers.md",
|
||||
"gsd-core/workflows/plan-phase/steps/windows-troubleshooting.md",
|
||||
"gsd-core/workflows/plan-review-convergence.md",
|
||||
"gsd-core/workflows/plant-seed.md",
|
||||
|
||||
1
tests/fixtures/install-tree/kimi.json
vendored
1
tests/fixtures/install-tree/kimi.json
vendored
@@ -365,6 +365,7 @@
|
||||
"gsd-core/workflows/plan-phase/steps/research-only-early-exit.md",
|
||||
"gsd-core/workflows/plan-phase/steps/research-only-modifiers.md",
|
||||
"gsd-core/workflows/plan-phase/steps/reviews-prerequisite.md",
|
||||
"gsd-core/workflows/plan-phase/steps/stall-detection-helpers.md",
|
||||
"gsd-core/workflows/plan-phase/steps/windows-troubleshooting.md",
|
||||
"gsd-core/workflows/plan-review-convergence.md",
|
||||
"gsd-core/workflows/plant-seed.md",
|
||||
|
||||
1
tests/fixtures/install-tree/opencode.json
vendored
1
tests/fixtures/install-tree/opencode.json
vendored
@@ -370,6 +370,7 @@
|
||||
"gsd-core/workflows/plan-phase/steps/research-only-early-exit.md",
|
||||
"gsd-core/workflows/plan-phase/steps/research-only-modifiers.md",
|
||||
"gsd-core/workflows/plan-phase/steps/reviews-prerequisite.md",
|
||||
"gsd-core/workflows/plan-phase/steps/stall-detection-helpers.md",
|
||||
"gsd-core/workflows/plan-phase/steps/windows-troubleshooting.md",
|
||||
"gsd-core/workflows/plan-review-convergence.md",
|
||||
"gsd-core/workflows/plant-seed.md",
|
||||
|
||||
1
tests/fixtures/install-tree/pi.json
vendored
1
tests/fixtures/install-tree/pi.json
vendored
@@ -267,6 +267,7 @@
|
||||
"gsd-core/workflows/plan-phase/steps/research-only-early-exit.md",
|
||||
"gsd-core/workflows/plan-phase/steps/research-only-modifiers.md",
|
||||
"gsd-core/workflows/plan-phase/steps/reviews-prerequisite.md",
|
||||
"gsd-core/workflows/plan-phase/steps/stall-detection-helpers.md",
|
||||
"gsd-core/workflows/plan-phase/steps/windows-troubleshooting.md",
|
||||
"gsd-core/workflows/plan-review-convergence.md",
|
||||
"gsd-core/workflows/plant-seed.md",
|
||||
|
||||
1
tests/fixtures/install-tree/qwen.json
vendored
1
tests/fixtures/install-tree/qwen.json
vendored
@@ -299,6 +299,7 @@
|
||||
"gsd-core/workflows/plan-phase/steps/research-only-early-exit.md",
|
||||
"gsd-core/workflows/plan-phase/steps/research-only-modifiers.md",
|
||||
"gsd-core/workflows/plan-phase/steps/reviews-prerequisite.md",
|
||||
"gsd-core/workflows/plan-phase/steps/stall-detection-helpers.md",
|
||||
"gsd-core/workflows/plan-phase/steps/windows-troubleshooting.md",
|
||||
"gsd-core/workflows/plan-review-convergence.md",
|
||||
"gsd-core/workflows/plant-seed.md",
|
||||
|
||||
1
tests/fixtures/install-tree/trae.json
vendored
1
tests/fixtures/install-tree/trae.json
vendored
@@ -299,6 +299,7 @@
|
||||
"gsd-core/workflows/plan-phase/steps/research-only-early-exit.md",
|
||||
"gsd-core/workflows/plan-phase/steps/research-only-modifiers.md",
|
||||
"gsd-core/workflows/plan-phase/steps/reviews-prerequisite.md",
|
||||
"gsd-core/workflows/plan-phase/steps/stall-detection-helpers.md",
|
||||
"gsd-core/workflows/plan-phase/steps/windows-troubleshooting.md",
|
||||
"gsd-core/workflows/plan-review-convergence.md",
|
||||
"gsd-core/workflows/plant-seed.md",
|
||||
|
||||
1
tests/fixtures/install-tree/windsurf.json
vendored
1
tests/fixtures/install-tree/windsurf.json
vendored
@@ -299,6 +299,7 @@
|
||||
"gsd-core/workflows/plan-phase/steps/research-only-early-exit.md",
|
||||
"gsd-core/workflows/plan-phase/steps/research-only-modifiers.md",
|
||||
"gsd-core/workflows/plan-phase/steps/reviews-prerequisite.md",
|
||||
"gsd-core/workflows/plan-phase/steps/stall-detection-helpers.md",
|
||||
"gsd-core/workflows/plan-phase/steps/windows-troubleshooting.md",
|
||||
"gsd-core/workflows/plan-review-convergence.md",
|
||||
"gsd-core/workflows/plant-seed.md",
|
||||
|
||||
1
tests/fixtures/install-tree/zcode.json
vendored
1
tests/fixtures/install-tree/zcode.json
vendored
@@ -370,6 +370,7 @@
|
||||
"gsd-core/workflows/plan-phase/steps/research-only-early-exit.md",
|
||||
"gsd-core/workflows/plan-phase/steps/research-only-modifiers.md",
|
||||
"gsd-core/workflows/plan-phase/steps/reviews-prerequisite.md",
|
||||
"gsd-core/workflows/plan-phase/steps/stall-detection-helpers.md",
|
||||
"gsd-core/workflows/plan-phase/steps/windows-troubleshooting.md",
|
||||
"gsd-core/workflows/plan-review-convergence.md",
|
||||
"gsd-core/workflows/plant-seed.md",
|
||||
|
||||
@@ -29,7 +29,7 @@ const os = require('node:os');
|
||||
const path = require('node:path');
|
||||
const { execSync } = require('node:child_process');
|
||||
|
||||
const { runGsdTools, cleanup } = require('./helpers.cjs');
|
||||
const { runGsdTools, cleanup, readFileNormalized } = require('./helpers.cjs');
|
||||
|
||||
// ─── helpers ──────────────────────────────────────────────────────────────────
|
||||
|
||||
@@ -510,8 +510,8 @@ function git(cwd, ...args) {
|
||||
* same way a markdown parser would.
|
||||
*/
|
||||
function extractHandleBranchingBash() {
|
||||
const content = fs.readFileSync(EXECUTE_PHASE_PATH, 'utf-8');
|
||||
const lines = content.split(/\r?\n/);
|
||||
const content = readFileNormalized(EXECUTE_PHASE_PATH);
|
||||
const lines = content.split('\n');
|
||||
|
||||
let start = -1;
|
||||
let end = -1;
|
||||
|
||||
@@ -610,7 +610,7 @@ const fs = require('fs');
|
||||
const path = require('path');
|
||||
const { spawnSync } = require('child_process');
|
||||
|
||||
const { createTempDir, cleanup } = require('./helpers.cjs');
|
||||
const { createTempDir, cleanup, readFileNormalized } = require('./helpers.cjs');
|
||||
|
||||
// Path to the command doc (relative to repo root)
|
||||
const GRAPHIFY_MD = path.join(__dirname, '..', 'commands', 'gsd', 'graphify.md');
|
||||
@@ -621,9 +621,14 @@ const GRAPHIFY_MD = path.join(__dirname, '..', 'commands', 'gsd', 'graphify.md')
|
||||
* closing ``` fence.
|
||||
*
|
||||
* Returns the bash source text (without the fence lines themselves).
|
||||
*
|
||||
* readFileNormalized() strips \r\n -> \n before the match below runs — the
|
||||
* extracted block is later spawned via spawnSync('bash', ...) in runBlock(),
|
||||
* so an un-normalized read on a Windows checkout would break bash mid-script
|
||||
* (DEFECT.TEST-SHELL-PIPELINE-NONPORTABLE, #2650).
|
||||
*/
|
||||
function extractStep3Block() {
|
||||
const content = fs.readFileSync(GRAPHIFY_MD, 'utf-8');
|
||||
const content = readFileNormalized(GRAPHIFY_MD);
|
||||
// Capture the full body of the ```bash fence that CONTAINS `graphify update .`
|
||||
// (including any leading preamble line), without crossing into other fences.
|
||||
const match = content.match(/```bash\r?\n((?:(?!```)[\s\S])*?graphify update \.(?:(?!```)[\s\S])*?)\r?\n```/);
|
||||
|
||||
@@ -154,6 +154,76 @@ function cleanup(tmpDir) {
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Read a text file with CRLF normalized to LF.
|
||||
*
|
||||
* DEFECT.TEST-SHELL-PIPELINE-NONPORTABLE (CONTEXT.md; recurring since #1700):
|
||||
* a test that reads a workflow/agent/reference `.md` file, slices or
|
||||
* regex-matches a fenced code block out of it, and hands that block to
|
||||
* `spawnSync('bash', ...)` breaks on a Windows checkout — `.gitattributes`
|
||||
* `eol=lf` is not always honored by `actions/checkout` on `windows-latest`,
|
||||
* so `readFileSync` can return `\r\n` line endings. Bash then treats the
|
||||
* trailing `\r` on every line as part of the token; an opening quote never
|
||||
* finds its match and the parser dies mid-script with "unexpected EOF while
|
||||
* looking for matching `"'" or a bare syntax error at the next `{`/`)`.
|
||||
*
|
||||
* `.split(/\r?\n/)` on the FENCE DELIMITER alone does not fix this — it only
|
||||
* protects the boundary match, not the captured body between the fences,
|
||||
* which still carries embedded `\r` characters (the exact bug #2650's
|
||||
* verification round found in tests/fix-2650-plan-phase-stall-detection.test.cjs,
|
||||
* despite that file's fence regex already using `\r?\n`).
|
||||
*
|
||||
* Normalizing ONCE at the read boundary, before any slicing/regex/fence
|
||||
* parsing runs, is cheaper and safer than normalizing at each extraction
|
||||
* call site: every downstream `indexOf`/`slice`/regex/`spawnSync` then
|
||||
* operates on LF-only content by construction, and a new `.md`-extraction
|
||||
* test is correct by default just by reading through this helper.
|
||||
*
|
||||
* @param {string} filePath - Absolute or relative path to a text file.
|
||||
* @returns {string} File content with every `\r\n` replaced by `\n`.
|
||||
*/
|
||||
function readFileNormalized(filePath) {
|
||||
return fs.readFileSync(filePath, 'utf-8').replace(/\r\n/g, '\n');
|
||||
}
|
||||
|
||||
/**
|
||||
* Read a workflow .md file plus every .md file under its sibling
|
||||
* `<workflow-basename>/steps/` directory, concatenated in document order
|
||||
* (host file first, then step files sorted by filename).
|
||||
*
|
||||
* ADR-1671's workflow fragmentization (#2930/#2932/#2993 et al.) moves whole
|
||||
* sections out of a host workflow (e.g. `plan-phase.md`) into lazily-loaded
|
||||
* step files under `gsd-core/workflows/<name>/steps/*.md`. A structural or
|
||||
* drift guard that reads the host file alone goes blind the moment a
|
||||
* section it cares about moves out — this is exactly the shape #2650's own
|
||||
* regression tests hit when #2993 relocated plan-phase.md's chunked-planning
|
||||
* spawn sites into `plan-phase/steps/chunked-planning-mode.md`. Any test
|
||||
* that needs to see the FULL picture (counting markers, asserting a marker
|
||||
* exists somewhere in the workflow) should read through this helper instead
|
||||
* of `fs.readFileSync(workflowPath)` alone, so the next relocation doesn't
|
||||
* silently blind it again. Originally local to
|
||||
* tests/plan-phase-drift-guard.test.cjs (readPlanPhaseCombined) — promoted
|
||||
* here so a second, divergent copy is never written (Generative Fix
|
||||
* Divergence class).
|
||||
*
|
||||
* @param {string} workflowPath - absolute path to the host workflow .md file.
|
||||
* @returns {string} host content, then '\n' + each step file's content in
|
||||
* sorted-filename order. An absent steps directory degrades to the host
|
||||
* content alone (not an error — most workflows have no steps/ dir).
|
||||
*/
|
||||
function readWorkflowCombined(workflowPath) {
|
||||
let combined = readFileNormalized(workflowPath);
|
||||
const stepsDir = path.join(path.dirname(workflowPath), path.basename(workflowPath, '.md'), 'steps');
|
||||
if (fs.existsSync(stepsDir)) {
|
||||
for (const entry of fs.readdirSync(stepsDir).sort()) {
|
||||
if (entry.endsWith('.md')) {
|
||||
combined += '\n' + readFileNormalized(path.join(stepsDir, entry));
|
||||
}
|
||||
}
|
||||
}
|
||||
return combined;
|
||||
}
|
||||
|
||||
/**
|
||||
* Parse a Markdown frontmatter block into a flat key→value map.
|
||||
*
|
||||
@@ -484,4 +554,4 @@ function clearSessionEnv() {
|
||||
for (const k of SESSION_ENV_KEYS) delete process.env[k];
|
||||
}
|
||||
|
||||
module.exports = { runGsdTools, createTempDir, createTempProject, createTempGitProject, cleanup, parseFrontmatter, isUsageOutput, captureConsole, toPosixPath, absPlanningPath, runNpm, isolatedNpmEnv, withIsolatedProcessState, delay, waitFor, resetRuntimeWarningCaches, SESSION_ENV_KEYS, saveSessionEnv, restoreSessionEnv, clearSessionEnv, TOOLS_PATH };
|
||||
module.exports = { runGsdTools, createTempDir, createTempProject, createTempGitProject, cleanup, readFileNormalized, readWorkflowCombined, parseFrontmatter, isUsageOutput, captureConsole, toPosixPath, absPlanningPath, runNpm, isolatedNpmEnv, withIsolatedProcessState, delay, waitFor, resetRuntimeWarningCaches, SESSION_ENV_KEYS, saveSessionEnv, restoreSessionEnv, clearSessionEnv, TOOLS_PATH };
|
||||
|
||||
@@ -13,7 +13,7 @@ const assert = require('node:assert/strict');
|
||||
const { execSync, execFileSync } = require('child_process');
|
||||
const fs = require('fs');
|
||||
const path = require('path');
|
||||
const { runGsdTools, createTempProject, createTempGitProject, cleanup } = require('./helpers.cjs');
|
||||
const { runGsdTools, createTempProject, createTempGitProject, cleanup, readFileNormalized } = require('./helpers.cjs');
|
||||
const { writeState } = require('./fixtures/index.cjs');
|
||||
|
||||
describe('phases clear command', () => {
|
||||
@@ -574,7 +574,11 @@ test('execute-phase.md: awk extracts resolves_phase from YAML frontmatter', () =
|
||||
// ────────────────────────────────────────────────────────────────────────
|
||||
describe('new-milestone.md: workstream-aware PROJECT.md guard (#2308)', () => {
|
||||
const workflowPath = path.join(__dirname, '..', 'gsd-core', 'workflows', 'new-milestone.md');
|
||||
const content = fs.readFileSync(workflowPath, 'utf8');
|
||||
// readFileNormalized() strips \r\n -> \n before either extractor below slices
|
||||
// a fence out of `content` — both fences are handed to execFileSync('bash', ...)
|
||||
// in runStep1/runStep6Commit, so an un-normalized read on a Windows checkout
|
||||
// would break bash mid-script (DEFECT.TEST-SHELL-PIPELINE-NONPORTABLE, #2650).
|
||||
const content = readFileNormalized(workflowPath);
|
||||
|
||||
// Locate the first ```bash fence strictly between two headings.
|
||||
function extractFenceBetween(markdown, startHeading, endHeading) {
|
||||
|
||||
@@ -90,15 +90,20 @@ const assert = require('node:assert/strict');
|
||||
const fs = require('node:fs');
|
||||
const path = require('node:path');
|
||||
const { spawnSync } = require('node:child_process');
|
||||
const { createTempDir, cleanup } = require('./helpers.cjs');
|
||||
const { createTempDir, cleanup, readFileNormalized } = require('./helpers.cjs');
|
||||
|
||||
const WORKFLOW_PATH = path.join(__dirname, '..', 'gsd-core', 'workflows', 'resume-project.md');
|
||||
|
||||
// Extract the first ```bash``` code block inside the
|
||||
// `<step name="check_incomplete_work">` element. That's the snippet the
|
||||
// runtime actually executes; it's what we want to validate.
|
||||
//
|
||||
// readFileNormalized() strips \r\n -> \n before the fence match below runs —
|
||||
// the extracted snippet is spawned via spawnSync('bash', ...) in
|
||||
// runSnippet(), so an un-normalized read on a Windows checkout would break
|
||||
// bash mid-script (DEFECT.TEST-SHELL-PIPELINE-NONPORTABLE, #2650).
|
||||
function extractCheckBlock() {
|
||||
const md = fs.readFileSync(WORKFLOW_PATH, 'utf8');
|
||||
const md = readFileNormalized(WORKFLOW_PATH);
|
||||
const stepStart = md.indexOf('<step name="check_incomplete_work">');
|
||||
assert.ok(stepStart >= 0, 'resume-project.md must contain a check_incomplete_work step');
|
||||
const stepEnd = md.indexOf('</step>', stepStart);
|
||||
|
||||
@@ -23,6 +23,7 @@ const { test, describe } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const fs = require('fs');
|
||||
const path = require('path');
|
||||
const { readWorkflowCombined } = require('./helpers.cjs');
|
||||
|
||||
const WORKFLOW_PATH = path.join(
|
||||
__dirname,
|
||||
@@ -32,25 +33,19 @@ const WORKFLOW_PATH = path.join(
|
||||
'plan-phase.md'
|
||||
);
|
||||
|
||||
const PLAN_PHASE_STEPS_DIR = path.join(__dirname, '..', 'gsd-core', 'workflows', 'plan-phase', 'steps');
|
||||
|
||||
/**
|
||||
* plan-phase.md was fragmented (#2993) into gsd-core/workflows/plan-phase/steps/*.md.
|
||||
* Content this drift guard asserts on may now live in the host file, an extracted
|
||||
* step file, or be split across both (e.g. a rule-label count). Guards that need to
|
||||
* see the full picture read this combined blob instead of `workflow` alone so they
|
||||
* don't go blind the next time a section moves out of the host.
|
||||
* don't go blind the next time a section moves out of the host. Delegates to the
|
||||
* shared tests/helpers.cjs readWorkflowCombined() — kept as a same-named local
|
||||
* wrapper so every existing call site here is unchanged (Generative Fix Divergence:
|
||||
* this was the original implementation, now promoted to a shared helper so
|
||||
* tests/fix-2650-plan-phase-stall-detection.test.cjs doesn't need a second copy).
|
||||
*/
|
||||
function readPlanPhaseCombined() {
|
||||
let combined = fs.readFileSync(WORKFLOW_PATH, 'utf8');
|
||||
if (fs.existsSync(PLAN_PHASE_STEPS_DIR)) {
|
||||
for (const entry of fs.readdirSync(PLAN_PHASE_STEPS_DIR).sort()) {
|
||||
if (entry.endsWith('.md')) {
|
||||
combined += '\n' + fs.readFileSync(path.join(PLAN_PHASE_STEPS_DIR, entry), 'utf8');
|
||||
}
|
||||
}
|
||||
}
|
||||
return combined;
|
||||
return readWorkflowCombined(WORKFLOW_PATH);
|
||||
}
|
||||
|
||||
// ─── Fixture ──────────────────────────────────────────────────────────────────
|
||||
|
||||
@@ -32,6 +32,7 @@ const assert = require('node:assert/strict');
|
||||
const fs = require('fs');
|
||||
const path = require('path');
|
||||
const { execFileSync } = require('node:child_process');
|
||||
const { readFileNormalized } = require('./helpers.cjs');
|
||||
|
||||
const COMMAND_PATH = path.join(__dirname, '..', 'commands', 'gsd', 'plan-review-convergence.md');
|
||||
const WORKFLOW_PATH = path.join(__dirname, '..', 'gsd-core', 'workflows', 'plan-review-convergence.md');
|
||||
@@ -256,7 +257,15 @@ describe('plan-review-convergence: --agy/--antigravity reviewer whitelist (#2293
|
||||
// ─── #2315: bare invocation respects review.default_reviewers ──────────────
|
||||
|
||||
describe('plan-review-convergence: #2315 respects review.default_reviewers (no-flag default)', () => {
|
||||
const workflow = fs.readFileSync(WORKFLOW_PATH, 'utf8');
|
||||
// readFileNormalized() strips \r\n -> \n before the two behavioral tests
|
||||
// below slice a bash block out of `workflow` and hand it to
|
||||
// execFileSync('bash', ...) — an un-normalized read on a Windows checkout
|
||||
// would break bash mid-script (DEFECT.TEST-SHELL-PIPELINE-NONPORTABLE,
|
||||
// #2650). Those two tests already skip on win32 for an unrelated POSIX-
|
||||
// shell-extraction reason, but `workflow` is shared with non-skipped
|
||||
// structural assertions in this same describe block, so normalizing here
|
||||
// is the single correct fix rather than a per-test patch.
|
||||
const workflow = readFileNormalized(WORKFLOW_PATH);
|
||||
const command = fs.readFileSync(COMMAND_PATH, 'utf8');
|
||||
const SKILL_PATH = path.join(__dirname, '..', 'skills', 'gsd-plan-review-convergence', 'SKILL.md');
|
||||
const skill = fs.readFileSync(SKILL_PATH, 'utf8');
|
||||
|
||||
@@ -19,7 +19,7 @@ const { execFileSync } = require('node:child_process');
|
||||
const fs = require('node:fs');
|
||||
const os = require('node:os');
|
||||
const path = require('node:path');
|
||||
const { cleanup } = require('./helpers.cjs');
|
||||
const { cleanup, readFileNormalized } = require('./helpers.cjs');
|
||||
|
||||
const QUICK_PATH = path.join(__dirname, '..', 'gsd-core', 'workflows', 'quick.md');
|
||||
|
||||
@@ -52,8 +52,8 @@ function git(cwd, ...args) {
|
||||
* a markdown parser would.
|
||||
*/
|
||||
function extractStep25Bash() {
|
||||
const content = fs.readFileSync(QUICK_PATH, 'utf-8');
|
||||
const lines = content.split(/\r?\n/);
|
||||
const content = readFileNormalized(QUICK_PATH);
|
||||
const lines = content.split('\n');
|
||||
|
||||
let start = -1;
|
||||
let end = -1;
|
||||
|
||||
@@ -1434,6 +1434,7 @@ const fs = require('node:fs');
|
||||
const os = require('node:os');
|
||||
const path = require('node:path');
|
||||
const { execFileSync } = require('node:child_process');
|
||||
const { readFileNormalized } = require('./helpers.cjs');
|
||||
|
||||
const WORKFLOW_PATH = path.join(__dirname, '..', 'gsd-core', 'workflows', 'next.md');
|
||||
|
||||
@@ -1448,8 +1449,8 @@ const WORKFLOW_PATH = path.join(__dirname, '..', 'gsd-core', 'workflows', 'next.
|
||||
* lines — scan to the closing `fi`.
|
||||
*/
|
||||
function extractResolverSnippet() {
|
||||
const content = fs.readFileSync(WORKFLOW_PATH, 'utf8');
|
||||
const lines = content.split(/\r?\n/);
|
||||
const content = readFileNormalized(WORKFLOW_PATH);
|
||||
const lines = content.split('\n');
|
||||
|
||||
// Find the canonical preamble — prefer _GSD_SHIM_NAME= line (handles both forms)
|
||||
let start = lines.findIndex((line) => /^_GSD_SHIM_NAME=/.test(line.trim()));
|
||||
|
||||
@@ -43,19 +43,23 @@
|
||||
*
|
||||
* Two complementary guards, neither of which is a tier-max ceiling:
|
||||
*
|
||||
* 1. Per-file baseline (the anti-creep): every workflow is pinned to its
|
||||
* exact size in `tests/workflow-size-baseline.json`. Any growth fails with
|
||||
* the file and delta; `npm run size:baseline` records a deliberate change
|
||||
* as a one-line reviewable diff. This replaced the tier-max tighten-only
|
||||
* ratchet (#597), which only bound the single largest file per tier and
|
||||
* left the other ~85 files able to grow silently.
|
||||
* 1. Differential attribution size ratchet (the anti-creep): every workflow's
|
||||
* byte growth against the base ref is reported with its exact delta by
|
||||
* `tests/emitted-attribution.test.cjs` (via `tests/helpers/emitted-diff.cjs`'s
|
||||
* size ratchet), which fails unless the growth is acknowledged in
|
||||
* `tests/emitted-drift-ack.json` (ADR-2719 §4). This REPLACED the per-file
|
||||
* `tests/workflow-size-baseline.json` snapshot (removed by #2724, ADR-2719
|
||||
* Phase 4 — it conflicted on 7 of 7 PRs that touched it), which itself had
|
||||
* replaced the original tier-max tighten-only ratchet (#597), which only
|
||||
* bound the single largest file per tier and left the other ~85 files able
|
||||
* to grow silently.
|
||||
*
|
||||
* 2. Tier hard caps (the outer bound): XL/LARGE/DEFAULT are absolute red
|
||||
* lines with real headroom, never raised in normal work. Crossing one
|
||||
* means lazy extraction (the `workflows/discuss-phase/modes/`
|
||||
* progressive-disclosure pattern), not a +N bump. New workflow files get
|
||||
* the Codex `project_doc_max_bytes` anchor (32 KiB) unless explicitly
|
||||
* tiered in the same PR.
|
||||
* tiered in the same PR — see `NEW_FILE_CAP` in `tests/helpers/emitted-diff.cjs`.
|
||||
*
|
||||
* Tiers:
|
||||
* - XL : top-level orchestrators (e.g., execute-phase, plan-phase)
|
||||
|
||||
@@ -1475,7 +1475,7 @@ const { execSync, spawnSync } = require('node:child_process');
|
||||
const fs = require('node:fs');
|
||||
const path = require('node:path');
|
||||
const os = require('node:os');
|
||||
const { cleanup } = require('./helpers.cjs');
|
||||
const { cleanup, readFileNormalized } = require('./helpers.cjs');
|
||||
|
||||
const REPO_ROOT = path.join(__dirname, '..');
|
||||
const EXECUTE_PHASE_PATH = path.join(REPO_ROOT, 'gsd-core', 'workflows', 'execute-phase.md');
|
||||
@@ -1497,7 +1497,15 @@ const EXECUTE_PHASE_PATH = path.join(REPO_ROOT, 'gsd-core', 'workflows', 'execut
|
||||
* Throws with a clear message if any step fails or sanity checks don't pass.
|
||||
*/
|
||||
function extractCwdGuardBash() {
|
||||
const content = fs.readFileSync(EXECUTE_PHASE_PATH, 'utf-8');
|
||||
// readFileNormalized() strips \r\n -> \n at the read boundary (helpers.cjs;
|
||||
// DEFECT.TEST-SHELL-PIPELINE-NONPORTABLE, #1700/#2650). The `\r?\n` in the
|
||||
// fence regex below only protects the FENCE DELIMITER match — it does
|
||||
// nothing for `\r` characters embedded in the CAPTURED BODY between the
|
||||
// fences, which is what actually reaches spawnSync('bash', ...) in
|
||||
// runGuard(). A prior version of this comment claimed the regex alone was
|
||||
// "CRLF-safe"; it was not — reading through readFileNormalized() first is
|
||||
// what makes the extracted body itself safe to execute on a Windows checkout.
|
||||
const content = readFileNormalized(EXECUTE_PHASE_PATH);
|
||||
|
||||
const stepMarker = '<step name="execute_waves">';
|
||||
const stepIdx = content.indexOf(stepMarker);
|
||||
@@ -1515,8 +1523,12 @@ function extractCwdGuardBash() {
|
||||
|
||||
const afterDrift = afterStep.slice(driftIdx + driftMarker.length);
|
||||
|
||||
// Extract the first ```bash|sh fenced block using a CRLF-safe regex.
|
||||
// \r?\n tolerates both LF (Unix) and CRLF (Windows autocrlf=true checkouts).
|
||||
// Fence delimiter match. `content` is already LF-only from
|
||||
// readFileNormalized() above, so `\r?\n` here is redundant, not load-bearing
|
||||
// — kept anyway (harmless on already-normalized input) because a bare `\n`
|
||||
// in a markdown-fence-shaped regex trips the local/no-crlf-fragile-split
|
||||
// ESLint rule (it flags the pattern shape statically and cannot see that
|
||||
// this call site's data already passed through the normalizing read).
|
||||
const fenceRe = /```(?:bash|sh)\r?\n([\s\S]*?)```/;
|
||||
const fenceMatch = fenceRe.exec(afterDrift);
|
||||
if (!fenceMatch) {
|
||||
|
||||
Reference in New Issue
Block a user