diff --git a/.changeset/eager-foxes-roam.md b/.changeset/eager-foxes-roam.md new file mode 100644 index 000000000..bcbc75c08 --- /dev/null +++ b/.changeset/eager-foxes-roam.md @@ -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.) diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 68dc4288d..d28fe97cd 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -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 | diff --git a/gsd-core/bin/shared/config-schema.manifest.json b/gsd-core/bin/shared/config-schema.manifest.json index d1c653d31..5519c7cc3 100644 --- a/gsd-core/bin/shared/config-schema.manifest.json +++ b/gsd-core/bin/shared/config-schema.manifest.json @@ -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", diff --git a/gsd-core/workflows/plan-phase.md b/gsd-core/workflows/plan-phase.md index f6ae9b75e..f83e93e6b 100644 --- a/gsd-core/workflows/plan-phase.md +++ b/gsd-core/workflows/plan-phase.md @@ -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. diff --git a/gsd-core/workflows/plan-phase/steps/chunked-planning-mode.md b/gsd-core/workflows/plan-phase/steps/chunked-planning-mode.md index c44177743..e6727b23b 100644 --- a/gsd-core/workflows/plan-phase/steps/chunked-planning-mode.md +++ b/gsd-core/workflows/plan-phase/steps/chunked-planning-mode.md @@ -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. diff --git a/gsd-core/workflows/plan-phase/steps/stall-detection-helpers.md b/gsd-core/workflows/plan-phase/steps/stall-detection-helpers.md new file mode 100644 index 000000000..28e0bd998 --- /dev/null +++ b/gsd-core/workflows/plan-phase/steps/stall-detection-helpers.md @@ -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 "@": + # -newermt's "@" shorthand is a GNU-date convenience the shipped + # BSD find(1) on macOS does NOT understand ("Can't parse date/time: + # @", 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" +} +``` diff --git a/src/config.cts b/src/config.cts index bcd259d3a..13770e218 100644 --- a/src/config.cts +++ b/src/config.cts @@ -88,6 +88,8 @@ const SCHEMA_DEFAULTS: Record = { '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). diff --git a/tests/code-review-pipeline-regression.test.cjs b/tests/code-review-pipeline-regression.test.cjs index c941724f2..a57b5933d 100644 --- a/tests/code-review-pipeline-regression.test.cjs +++ b/tests/code-review-pipeline-regression.test.cjs @@ -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'); diff --git a/tests/drift-detection.test.cjs b/tests/drift-detection.test.cjs index b6d569ea8..1f52a1c35 100644 --- a/tests/drift-detection.test.cjs +++ b/tests/drift-detection.test.cjs @@ -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; diff --git a/tests/emitted-drift-acks/2650-plan-phase-stall-detection.json b/tests/emitted-drift-acks/2650-plan-phase-stall-detection.json new file mode 100644 index 000000000..eb45e529a --- /dev/null +++ b/tests/emitted-drift-acks/2650-plan-phase-stall-detection.json @@ -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." + } +} diff --git a/tests/fix-2650-plan-phase-stall-detection.test.cjs b/tests/fix-2650-plan-phase-stall-detection.test.cjs new file mode 100644 index 000000000..a384c342c --- /dev/null +++ b/tests/fix-2650-plan-phase-stall-detection.test.cjs @@ -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