fix: add executor stall recovery contract (#3329)
* fix: add executor stall recovery contract * chore: add changeset for executor recovery * chore: keep execute phase within size budget
This commit is contained in:
5
.changeset/sturdy-rams-forage.md
Normal file
5
.changeset/sturdy-rams-forage.md
Normal file
@@ -0,0 +1,5 @@
|
||||
---
|
||||
type: Fixed
|
||||
pr: 3329
|
||||
---
|
||||
Add executor stall detection and safe-resume contracts so interrupted execute-phase runs surface partial-plan drift before dispatching duplicate executor work.
|
||||
@@ -216,6 +216,8 @@ All workflow toggles follow the **absent = enabled** pattern. If a key is missin
|
||||
| `workflow.auto_prune_state` | boolean | `false` | When `true`, automatically prune stale entries from STATE.md at phase boundaries instead of prompting |
|
||||
| `workflow.pattern_mapper` | boolean | `true` | Run the `gsd-pattern-mapper` agent between research and planning to map new files to existing codebase analogs |
|
||||
| `workflow.subagent_timeout` | number | `600` | Timeout in seconds for individual subagent invocations. Increase for long-running research or execution phases |
|
||||
| `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. |
|
||||
| `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) introduced during a phase before the post-execute codebase-drift gate takes action. See [#2003](https://github.com/gsd-build/get-shit-done/issues/2003). Added in v1.39 |
|
||||
| `workflow.drift_action` | string | `warn` | What to do when `workflow.drift_threshold` is exceeded after `/gsd-execute-phase`. `warn` prints a message suggesting `/gsd-map-codebase --paths …`; `auto-remap` spawns `gsd-codebase-mapper` scoped to the affected paths. Added in v1.39 |
|
||||
|
||||
@@ -47,6 +47,8 @@ const VALID_CONFIG_KEYS = new Set([
|
||||
'review.ollama_host', 'review.lm_studio_host', 'review.llama_cpp_host',
|
||||
'workflow.cross_ai_execution', 'workflow.cross_ai_command', 'workflow.cross_ai_timeout',
|
||||
'workflow.subagent_timeout',
|
||||
'executor.stall_detect_interval_minutes',
|
||||
'executor.stall_threshold_minutes',
|
||||
'workflow.inline_plan_threshold',
|
||||
'hooks.context_warnings',
|
||||
'hooks.workflow_guard',
|
||||
|
||||
@@ -384,6 +384,8 @@ function cmdConfigSet(cwd, keyPath, value, raw) {
|
||||
*/
|
||||
const SCHEMA_DEFAULTS = {
|
||||
'context_window': 200000,
|
||||
'executor.stall_detect_interval_minutes': 5,
|
||||
'executor.stall_threshold_minutes': 10,
|
||||
};
|
||||
|
||||
function cmdConfigGet(cwd, keyPath, raw, defaultValue) {
|
||||
|
||||
@@ -81,6 +81,8 @@ Read worktree config:
|
||||
|
||||
```bash
|
||||
USE_WORKTREES=$(gsd-sdk query config-get workflow.use_worktrees 2>/dev/null || echo "true")
|
||||
EXECUTOR_STALL_INTERVAL_MINUTES=$(gsd-sdk query config-get executor.stall_detect_interval_minutes 2>/dev/null || echo "5")
|
||||
EXECUTOR_STALL_THRESHOLD_MINUTES=$(gsd-sdk query config-get executor.stall_threshold_minutes 2>/dev/null || echo "10")
|
||||
```
|
||||
|
||||
If the project uses git submodules, worktree isolation is unsafe **only when a plan touches a submodule path** — the executor commit protocol cannot correctly handle submodule commits inside isolated worktrees. The previous behavior unconditionally disabled worktree isolation whenever `.gitmodules` existed, which penalised every plan in a submodule project even when the plan was nowhere near a submodule. Compute submodule paths once and intersect them per-plan with the plan's declared `files_modified` frontmatter.
|
||||
@@ -146,6 +148,22 @@ MVP_MODE=$(gsd-sdk query phase.mvp-mode "${PHASE_NUMBER}" $MVP_FLAG_ARG --pick a
|
||||
TDD_MODE=$(gsd-sdk query config-get workflow.tdd_mode 2>/dev/null || echo "false")
|
||||
```
|
||||
|
||||
<step name="safe_resume_gate">
|
||||
Before trusting `STATE.md` or dispatching any executor, derive `CURRENT_PLAN_ID`
|
||||
from the active incomplete plan in `INIT`, then search recent history:
|
||||
```bash
|
||||
CURRENT_PLAN_ID="{phase_number}-{plan_padded}"
|
||||
SUMMARY_PATH="{phase_dir}/{plan_padded}-SUMMARY.md"
|
||||
PLAN_COMMITS=$(git log --oneline --grep="${CURRENT_PLAN_ID}" -30)
|
||||
```
|
||||
If production commits exist and `SUMMARY.md is missing`, stop before spawning a
|
||||
new executor; continuing risks duplicate work and stale `STATE.md`/ROADMAP progress.
|
||||
Offer these recovery options:
|
||||
- `close out manually` — inspect commits, write SUMMARY.md, then update STATE/ROADMAP.
|
||||
- `re-execute from scratch` — revert or supersede partial commits before dispatch.
|
||||
- `mark-and-skip` — record the anomaly and move on only with explicit confirmation.
|
||||
</step>
|
||||
|
||||
**MVP+TDD gate.** Task-scoped enforcement runs inside plan execution (immediately before each implementation step), where `TASK_FILE`, `PLAN_ID`, and `TASK_ID` are defined. Keep the same predicate and RED-commit contract:
|
||||
```bash
|
||||
if [ "$MVP_MODE" = "true" ] && [ "$TDD_MODE" = "true" ]; then
|
||||
@@ -494,6 +512,8 @@ increases monotonically across waves. `{status}` is `complete` (success),
|
||||
Before spawning, capture the current HEAD:
|
||||
```bash
|
||||
EXPECTED_BASE=$(git rev-parse HEAD)
|
||||
DISPATCH_TS=$(date -u +"%Y-%m-%dT%H:%M:%SZ")
|
||||
EXPECTED_BRANCH=$(git rev-parse --abbrev-ref HEAD)
|
||||
```
|
||||
|
||||
**Sequential dispatch for parallel execution (waves with 2+ agents):**
|
||||
@@ -666,6 +686,7 @@ increases monotonically across waves. `{status}` is `complete` (success),
|
||||
# For each plan in this wave, check if the executor finished:
|
||||
SUMMARY_EXISTS=$(test -f "{phase_dir}/{plan_number}-{plan_padded}-SUMMARY.md" && echo "true" || echo "false")
|
||||
COMMITS_FOUND=$(git log --oneline --all --grep="{phase_number}-{plan_padded}" --since="1 hour ago" | head -1)
|
||||
COMMITS_SINCE_DISPATCH=$(git log "${EXPECTED_BRANCH}" --since="${DISPATCH_TS}" --oneline | head -1)
|
||||
```
|
||||
|
||||
**If SUMMARY.md exists AND commits are found:** The agent completed successfully —
|
||||
@@ -676,6 +697,13 @@ increases monotonically across waves. `{status}` is `complete` (success),
|
||||
activity. If commits are still appearing, wait longer. If no activity, report
|
||||
the plan as failed and route to the failure handler in step 6.
|
||||
|
||||
**Configurable stall surveillance (#3212):** Every `${EXECUTOR_STALL_INTERVAL_MINUTES}`
|
||||
minutes while waiting, inspect `git log "${EXPECTED_BRANCH}" --since="${DISPATCH_TS}"`
|
||||
for activity. If no completion signal, no SUMMARY.md, and no expected-branch
|
||||
commits appear for `${EXECUTOR_STALL_THRESHOLD_MINUTES}` minutes, pause and
|
||||
ask for one recovery path: `continue waiting`, `kill and retry`, or
|
||||
`kill and switch to inline execution`.
|
||||
|
||||
**This fallback applies automatically to all runtimes.** Claude Code's Agent() normally
|
||||
returns synchronously, but the fallback ensures resilience if it doesn't.
|
||||
|
||||
|
||||
@@ -9,6 +9,16 @@ Read config.json for planning behavior settings.
|
||||
@~/.claude/get-shit-done/references/git-integration.md
|
||||
</required_reading>
|
||||
|
||||
<atomic_close_out_invariant>
|
||||
For each executed plan, the only complete close-out order is:
|
||||
`production-code commit(s) -> SUMMARY commit -> STATE/ROADMAP update`.
|
||||
|
||||
The only legal half-state is mid-production-commits while the executor is still
|
||||
actively working. Once production commits for a plan exist, returning without a
|
||||
committed SUMMARY.md is an illegal partial-plan state. The next execute-phase
|
||||
resume must detect that condition before dispatching another executor.
|
||||
</atomic_close_out_invariant>
|
||||
|
||||
<available_agent_types>
|
||||
Valid GSD subagent types (use exact names — do not fall back to 'general-purpose'):
|
||||
- gsd-executor — Executes plan tasks, commits, creates SUMMARY.md
|
||||
|
||||
@@ -115,6 +115,18 @@ For each phase that should be complete:
|
||||
- SUMMARY.md missing → phase was not properly closed
|
||||
- VERIFICATION.md missing → quality check was skipped
|
||||
|
||||
### Partial-plan Drift Detection
|
||||
|
||||
**Signal:** commits exist but SUMMARY.md is missing for the current or recently
|
||||
active plan.
|
||||
|
||||
Run the same comparison as the execute-phase safe-resume verifier: identify the
|
||||
active plan from STATE.md/phase artifacts, search git history for that plan id,
|
||||
then compare against the expected SUMMARY.md path. If production commits exist
|
||||
but SUMMARY.md is missing, flag a high-confidence partial-plan drift anomaly.
|
||||
This usually means an executor was interrupted after implementation commits but
|
||||
before atomic close-out.
|
||||
|
||||
### Abandoned Work Detection
|
||||
|
||||
**Signal:** Large gap between last commit and current time, with STATE.md showing mid-execution.
|
||||
|
||||
@@ -49,6 +49,8 @@ export const VALID_CONFIG_KEYS: ReadonlySet<string> = new Set([
|
||||
'review.ollama_host', 'review.lm_studio_host', 'review.llama_cpp_host',
|
||||
'workflow.cross_ai_execution', 'workflow.cross_ai_command', 'workflow.cross_ai_timeout',
|
||||
'workflow.subagent_timeout',
|
||||
'executor.stall_detect_interval_minutes',
|
||||
'executor.stall_threshold_minutes',
|
||||
'workflow.inline_plan_threshold',
|
||||
'hooks.context_warnings',
|
||||
'hooks.workflow_guard',
|
||||
|
||||
98
tests/bug-3212-execute-phase-stall-safe-resume.test.cjs
Normal file
98
tests/bug-3212-execute-phase-stall-safe-resume.test.cjs
Normal file
@@ -0,0 +1,98 @@
|
||||
'use strict';
|
||||
|
||||
// allow-test-rule: source-text-is-product [#3212]
|
||||
// The bug is in workflow/config contracts consumed by agents at runtime.
|
||||
|
||||
const { describe, test } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const { spawnSync } = require('node:child_process');
|
||||
const fs = require('fs');
|
||||
const os = require('os');
|
||||
const path = require('path');
|
||||
|
||||
const ROOT = path.join(__dirname, '..');
|
||||
|
||||
function read(relativePath) {
|
||||
return fs.readFileSync(path.join(ROOT, relativePath), 'utf8');
|
||||
}
|
||||
|
||||
function runGsd(args, cwd) {
|
||||
return spawnSync(process.execPath, [path.join(ROOT, 'get-shit-done/bin/gsd-tools.cjs'), ...args], {
|
||||
cwd,
|
||||
encoding: 'utf8',
|
||||
});
|
||||
}
|
||||
|
||||
describe('bug #3212 execute-phase stall detection and safe resume', () => {
|
||||
test('config schemas register executor stall detector keys', () => {
|
||||
const cjs = require('../get-shit-done/bin/lib/config-schema.cjs');
|
||||
const sdk = read('sdk/src/query/config-schema.ts');
|
||||
|
||||
for (const key of ['executor.stall_detect_interval_minutes', 'executor.stall_threshold_minutes']) {
|
||||
assert.ok(cjs.VALID_CONFIG_KEYS.has(key), `CJS VALID_CONFIG_KEYS must include ${key}`);
|
||||
assert.ok(sdk.includes(`'${key}'`), `SDK VALID_CONFIG_KEYS must include ${key}`);
|
||||
}
|
||||
});
|
||||
|
||||
test('configuration docs describe stall detector defaults', () => {
|
||||
const docs = read('docs/CONFIGURATION.md');
|
||||
|
||||
assert.match(docs, /`executor\.stall_detect_interval_minutes`\s*\|\s*number\s*\|\s*`5`/);
|
||||
assert.match(docs, /`executor\.stall_threshold_minutes`\s*\|\s*number\s*\|\s*`10`/);
|
||||
});
|
||||
|
||||
test('config-get returns schema defaults for executor stall detector keys', (t) => {
|
||||
const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3212-'));
|
||||
t.after(() => fs.rmSync(tmp, { recursive: true, force: true }));
|
||||
fs.mkdirSync(path.join(tmp, '.planning'));
|
||||
fs.writeFileSync(path.join(tmp, '.planning/config.json'), '{}\n');
|
||||
|
||||
const interval = runGsd(['config-get', 'executor.stall_detect_interval_minutes', '--raw'], tmp);
|
||||
const threshold = runGsd(['config-get', 'executor.stall_threshold_minutes', '--raw'], tmp);
|
||||
|
||||
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');
|
||||
});
|
||||
|
||||
test('execute-phase verifies partial-plan drift before dispatch', () => {
|
||||
const workflow = read('get-shit-done/workflows/execute-phase.md');
|
||||
|
||||
assert.match(workflow, /<step name="safe_resume_gate"/, 'execute-phase must define a safe_resume_gate step');
|
||||
assert.match(workflow, /git log --oneline --grep="\$\{CURRENT_PLAN_ID\}"/, 'safe resume gate must check commits for the current plan id');
|
||||
assert.match(workflow, /SUMMARY.md is missing/, 'safe resume gate must detect production commits with missing SUMMARY.md');
|
||||
assert.match(workflow, /close out manually/, 'safe resume gate must offer manual close-out recovery');
|
||||
assert.match(workflow, /re-execute from scratch/, 'safe resume gate must offer re-execute recovery');
|
||||
assert.match(workflow, /mark-and-skip/, 'safe resume gate must offer mark-and-skip recovery');
|
||||
});
|
||||
|
||||
test('execute-phase has configurable executor stall surveillance after dispatch', () => {
|
||||
const workflow = read('get-shit-done/workflows/execute-phase.md');
|
||||
|
||||
assert.match(workflow, /EXECUTOR_STALL_INTERVAL_MINUTES=.*executor\.stall_detect_interval_minutes/);
|
||||
assert.match(workflow, /EXECUTOR_STALL_THRESHOLD_MINUTES=.*executor\.stall_threshold_minutes/);
|
||||
assert.match(workflow, /DISPATCH_TS=/, 'execute-phase must record dispatch timestamp');
|
||||
assert.match(workflow, /EXPECTED_BRANCH=/, 'execute-phase must record expected branch');
|
||||
assert.match(workflow, /git log "\$\{EXPECTED_BRANCH\}" --since="\$\{DISPATCH_TS\}"/, 'stall check must inspect branch commits since dispatch');
|
||||
assert.match(workflow, /continue waiting/, 'stall warning must offer continue waiting');
|
||||
assert.match(workflow, /kill and retry/, 'stall warning must offer kill and retry');
|
||||
assert.match(workflow, /kill and switch to inline execution/, 'stall warning must offer inline fallback');
|
||||
});
|
||||
|
||||
test('execute-plan documents atomic close-out invariant', () => {
|
||||
const workflow = read('get-shit-done/workflows/execute-plan.md');
|
||||
|
||||
assert.match(workflow, /<atomic_close_out_invariant>/, 'execute-plan must contain a formal atomic close-out invariant');
|
||||
assert.match(workflow, /production-code commit\(s\) -> SUMMARY commit -> STATE\/ROADMAP update/, 'invariant must name the legal close-out sequence');
|
||||
assert.match(workflow, /only legal half-state is mid-production-commits/, 'invariant must define the only legal half-state');
|
||||
});
|
||||
|
||||
test('forensics includes the partial-plan drift detector', () => {
|
||||
const workflow = read('get-shit-done/workflows/forensics.md');
|
||||
|
||||
assert.match(workflow, /Partial-plan Drift Detection/);
|
||||
assert.match(workflow, /commits exist but SUMMARY.md is missing/);
|
||||
assert.match(workflow, /safe-resume verifier/);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user