enhance(execute): isolated-executor rejected/over-reaching run fails safe — never default recovery to main (#1292) (#1303)
* enhance(execute): isolated-executor rejected/over-reaching run fails safe (#1292) When an isolated (worktree) executor run is rejected — the user declines to merge it, the orchestrator surfaces recovery for a blocked/halted plan, or the run over-reached the requested scope — the orchestrator must no longer default/propose recovery by editing the primary checkout (`main`). Absent an explicit guardrail, the LLM orchestrator could improvise "continue on main", inverting the isolation contract at the moment it matters most. Added an ISOLATED-RUN RECOVERY — FAIL SAFE policy: default to a safe halt that offers a fresh, narrowly-scoped worktree or inspect/discard; editing the primary checkout requires explicit, clearly-labeled confirmation and is never the default/proposed option. To respect the ADR-857 phase-6 host-loop size cap on execute-phase.md (it sits just under the pre-phase-6 baseline), the policy is delivered as an extracted reference fragment rather than inline: - New `execute-phase/steps/worktree-recovery-policy.md` holds the recovery policy (the existing FAIL-CLOSED rule #48 for base/HEAD mismatches + the #1292 fail-safe guardrail). No #48 behavior change — moved verbatim. - execute-phase.md references the fragment at the worktree-spawn recovery point, the step-5.5 merge decision, and the stalled-agent "switch to inline execution" menu (which for an isolated run now follows the fail-safe policy). Net effect: execute-phase.md shrinks below its cap. - quick.md carries the fail-safe guardrail inline at its post-return merge/discard decision (quick.md is not size-capped). Scoped to the recovery offer only — no automatic scope-overreach detection (explicitly out of scope per the issue) and no new config key. Adds content regression tests, a USER-GUIDE note, and a workflow size-baseline update. Closes #1292 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(changeset): Changed fragment for #1292 isolated-executor fail-safe recovery Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
5
.changeset/nimble-quails-hum.md
Normal file
5
.changeset/nimble-quails-hum.md
Normal file
@@ -0,0 +1,5 @@
|
||||
---
|
||||
type: Changed
|
||||
pr: 1303
|
||||
---
|
||||
**Isolated-executor recovery now fails safe** — when an isolated (worktree) executor run is rejected (you decline to merge it) or over-reached the requested scope, `/gsd:execute-phase` and `/gsd:quick` no longer default or propose recovery by editing the primary checkout (`main`). The orchestrator halts safely and offers a fresh, narrowly-scoped worktree or inspect/discard; editing the primary checkout requires explicit, clearly-labeled confirmation. (#1292)
|
||||
@@ -262,6 +262,10 @@ The discuss-phase captures implementation decisions in CONTEXT.md under a `<deci
|
||||
└── FAIL -> Issues logged for /gsd-verify-work
|
||||
```
|
||||
|
||||
### Isolated-run Recovery (fail-safe)
|
||||
|
||||
When a worktree-isolated run is rejected — the user declines to merge it, or the run over-reached the requested scope, or the orchestrator surfaces recovery guidance for a blocked plan — GSD halts safely and offers two options: (a) re-attempt in a fresh, narrowly-scoped worktree, or (b) inspect or discard the rejected worktree without merging. GSD never defaults recovery to editing the primary checkout (`main`). Any path that edits the primary checkout requires explicit, clearly-labeled confirmation from the user first. This behavior is unconditional and applies to both `/gsd-execute-phase` (worktree executor waves) and `/gsd-quick` (quick-mode isolated runs).
|
||||
|
||||
---
|
||||
|
||||
## UI Design Contract
|
||||
|
||||
@@ -684,7 +684,7 @@ increases monotonically across waves. `{status}` is `complete` (success),
|
||||
|
||||
Immediately after each worktree `Agent()` spawn returns metadata, atomically append `{agent_id, worktree_path, branch, expected_base}` to `WAVE_WORKTREE_MANIFEST`. If any field is missing, stop and ask for recovery instead of scanning all agent worktrees.
|
||||
|
||||
> **ORCHESTRATOR FAIL-CLOSED RULE (#48):** `worktree_branch_check` is verify-only — an executor that hits a base/HEAD-namespace mismatch prints `FATAL:` and exits **42** instead of self-recovering. If any executor result reports a `FATAL:`/`exit 42` (or its commits never appear because it halted at the check), mark that plan **blocked**: do NOT merge or clean up its worktree (preserve it for inspection), do NOT count the wave as successful, and surface the mismatch with recovery guidance to the user. The orchestrator — the worktree lifecycle owner — performs any base correction (e.g. recreate the worktree on `{EXPECTED_BASE}`); the sub-agent never does. Never proceed past a halted executor on the assumption it succeeded.
|
||||
> **Worktree recovery policy (#48 + #1292):** See `execute-phase/steps/worktree-recovery-policy.md` — FAIL-CLOSED rule for base/HEAD-namespace mismatches AND isolated-run fail-safe recovery.
|
||||
|
||||
> **ORCHESTRATOR RULE — CODEX RUNTIME**: After calling Agent() above to spawn executor agent(s), 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.
|
||||
|
||||
@@ -753,6 +753,8 @@ increases monotonically across waves. `{status}` is `complete` (success),
|
||||
ask for one recovery path: `continue waiting`, `kill and retry`, or
|
||||
`kill and switch to inline execution`.
|
||||
|
||||
If the stalled executor ran in an isolated worktree, `kill and switch to inline execution` edits the primary checkout — see worktree recovery policy (`execute-phase/steps/worktree-recovery-policy.md`). Prefer `kill and retry` in a fresh worktree; inline execution requires explicit confirmation, never the default.
|
||||
|
||||
**This fallback applies automatically to all runtimes.** Claude Code's Agent() normally
|
||||
returns synchronously, but the fallback ensures resilience if it doesn't.
|
||||
|
||||
@@ -855,6 +857,8 @@ increases monotonically across waves. `{status}` is `complete` (success),
|
||||
|
||||
**If no worktrees found at runtime:** Skip silently — agents may have been spawned without worktree isolation, or the orchestrator already cleaned them up.
|
||||
|
||||
If the user declines to merge a worktree or a worktree over-reached scope, apply the worktree recovery policy (`execute-phase/steps/worktree-recovery-policy.md`) — never default to editing `main`.
|
||||
|
||||
5.6. **Post-merge build & test gate:**
|
||||
|
||||
After merging all worktrees in a wave (parallel mode), or after the last plan completes
|
||||
|
||||
@@ -0,0 +1,9 @@
|
||||
# Worktree Recovery Policy
|
||||
|
||||
## ORCHESTRATOR FAIL-CLOSED RULE (#48)
|
||||
|
||||
> **ORCHESTRATOR FAIL-CLOSED RULE (#48):** `worktree_branch_check` is verify-only — an executor that hits a base/HEAD-namespace mismatch prints `FATAL:` and exits **42** instead of self-recovering. If any executor result reports a `FATAL:`/`exit 42` (or its commits never appear because it halted at the check), mark that plan **blocked**: do NOT merge or clean up its worktree (preserve it for inspection), do NOT count the wave as successful, and surface the mismatch with recovery guidance to the user. The orchestrator — the worktree lifecycle owner — performs any base correction (e.g. recreate the worktree on `{EXPECTED_BASE}`); the sub-agent never does. Never proceed past a halted executor on the assumption it succeeded.
|
||||
|
||||
## ISOLATED-RUN RECOVERY — FAIL SAFE (#1292)
|
||||
|
||||
> **ISOLATED-RUN RECOVERY — FAIL SAFE (#1292):** When an isolated (worktree) run is *rejected* — the user declines to merge it, the orchestrator surfaces recovery guidance for a blocked/halted plan, or the run over-reached the requested scope — the worktree-isolation contract MUST hold through recovery. Do **NOT** propose continuing on `main`/the primary checkout as the default or recommended recovery path. Default to a **safe halt** and offer: (a) re-attempt in a **fresh, narrowly-scoped worktree**, or (b) inspect or discard the rejected worktree without merging. Any path that edits the primary checkout requires an **explicit, clearly-labeled confirmation** from the user first — editing `main` directly is never the proposed or default option for a run the user configured to be isolated.
|
||||
@@ -761,6 +761,9 @@ After executor returns:
|
||||
gsd_run query worktree.cleanup-wave --manifest "$QUICK_WORKTREE_MANIFEST" || exit 1
|
||||
```
|
||||
If `workflow.use_worktrees` is `false`, skip this step.
|
||||
|
||||
> **ISOLATED-RUN RECOVERY — FAIL SAFE (#1292):** When an isolated (worktree) run is *rejected* — the user declines to merge it, the orchestrator surfaces recovery guidance for a blocked/halted plan, or the run over-reached the requested scope — the worktree-isolation contract MUST hold through recovery. Do **NOT** propose continuing on `main`/the primary checkout as the default or recommended recovery path. Default to a **safe halt** and offer: (a) re-attempt in a **fresh, narrowly-scoped worktree**, or (b) inspect or discard the rejected worktree without merging. Any path that edits the primary checkout requires an **explicit, clearly-labeled confirmation** from the user first — editing `main` directly is never the proposed or default option for a run the user configured to be isolated.
|
||||
|
||||
2. Verify summary exists at `${QUICK_DIR}/${quick_id}-SUMMARY.md`
|
||||
3. Extract commit hash from executor output
|
||||
4. Report completion status
|
||||
|
||||
@@ -20,6 +20,16 @@ const fs = require('fs');
|
||||
const path = require('path');
|
||||
|
||||
const WORKFLOW_PATH = path.join(__dirname, '..', 'gsd-core', 'workflows', 'execute-phase.md');
|
||||
const QUICK_WORKFLOW_PATH = path.join(__dirname, '..', 'gsd-core', 'workflows', 'quick.md');
|
||||
const RECOVERY_POLICY_PATH = path.join(
|
||||
__dirname,
|
||||
'..',
|
||||
'gsd-core',
|
||||
'workflows',
|
||||
'execute-phase',
|
||||
'steps',
|
||||
'worktree-recovery-policy.md'
|
||||
);
|
||||
|
||||
describe('execute-phase worktree: shared artifact ownership (#1571)', () => {
|
||||
test('workflow file exists', () => {
|
||||
@@ -130,3 +140,120 @@ describe('execute-phase worktree: shared artifact ownership (#1571)', () => {
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('isolated-run recovery fail-safe (#1292)', () => {
|
||||
test('worktree-recovery-policy.md fragment exists', () => {
|
||||
assert.ok(
|
||||
fs.existsSync(RECOVERY_POLICY_PATH),
|
||||
'execute-phase/steps/worktree-recovery-policy.md must exist (ADR-857 extraction pattern)'
|
||||
);
|
||||
});
|
||||
|
||||
test('execute-phase.md references the worktree-recovery-policy.md fragment', () => {
|
||||
const content = fs.readFileSync(WORKFLOW_PATH, 'utf-8');
|
||||
assert.ok(
|
||||
content.includes('execute-phase/steps/worktree-recovery-policy.md'),
|
||||
'execute-phase.md must reference the extracted worktree-recovery-policy.md fragment'
|
||||
);
|
||||
assert.ok(
|
||||
content.includes('#1292'),
|
||||
'execute-phase.md must reference #1292 in the worktree recovery policy pointer'
|
||||
);
|
||||
});
|
||||
|
||||
test('worktree-recovery-policy.md fragment contains ISOLATED-RUN RECOVERY guardrail referencing #1292', () => {
|
||||
const content = fs.readFileSync(RECOVERY_POLICY_PATH, 'utf-8');
|
||||
assert.ok(
|
||||
content.includes('ISOLATED-RUN RECOVERY'),
|
||||
'worktree-recovery-policy.md must contain the ISOLATED-RUN RECOVERY guardrail label'
|
||||
);
|
||||
assert.ok(
|
||||
content.includes('#1292'),
|
||||
'worktree-recovery-policy.md ISOLATED-RUN RECOVERY guardrail must reference #1292'
|
||||
);
|
||||
});
|
||||
|
||||
test('worktree-recovery-policy.md fragment forbids defaulting recovery to main/primary checkout', () => {
|
||||
const content = fs.readFileSync(RECOVERY_POLICY_PATH, 'utf-8');
|
||||
assert.ok(
|
||||
content.includes('never the proposed or default'),
|
||||
'worktree-recovery-policy.md guardrail must state editing main is "never the proposed or default" option'
|
||||
);
|
||||
});
|
||||
|
||||
test('worktree-recovery-policy.md fragment offers fresh narrowly-scoped worktree as recovery path', () => {
|
||||
const content = fs.readFileSync(RECOVERY_POLICY_PATH, 'utf-8');
|
||||
assert.ok(
|
||||
content.includes('fresh, narrowly-scoped worktree'),
|
||||
'worktree-recovery-policy.md guardrail must offer a "fresh, narrowly-scoped worktree" as recovery path'
|
||||
);
|
||||
});
|
||||
|
||||
test('worktree-recovery-policy.md fragment requires explicit confirmation before editing primary checkout', () => {
|
||||
const content = fs.readFileSync(RECOVERY_POLICY_PATH, 'utf-8');
|
||||
assert.ok(
|
||||
content.includes('explicit, clearly-labeled confirmation'),
|
||||
'worktree-recovery-policy.md guardrail must require "explicit, clearly-labeled confirmation" before editing the primary checkout'
|
||||
);
|
||||
});
|
||||
|
||||
test('worktree-recovery-policy.md fragment contains FAIL-CLOSED rule (#48) with exit-42 content', () => {
|
||||
const content = fs.readFileSync(RECOVERY_POLICY_PATH, 'utf-8');
|
||||
assert.ok(
|
||||
content.includes('worktree_branch_check'),
|
||||
'worktree-recovery-policy.md must contain "worktree_branch_check" from the FAIL-CLOSED rule (#48)'
|
||||
);
|
||||
assert.ok(
|
||||
content.includes('42'),
|
||||
'worktree-recovery-policy.md must contain "42" (exit 42) from the FAIL-CLOSED rule (#48)'
|
||||
);
|
||||
});
|
||||
|
||||
test('execute-phase.md step 5.5 decline/over-reach sentence references fragment and forbids main-default', () => {
|
||||
const content = fs.readFileSync(WORKFLOW_PATH, 'utf-8');
|
||||
assert.ok(
|
||||
content.includes('declines to merge a worktree'),
|
||||
'execute-phase.md step 5.5 pointer must mention "declines to merge a worktree" as the trigger for fail-safe policy'
|
||||
);
|
||||
assert.ok(
|
||||
content.includes('never default to editing'),
|
||||
'execute-phase.md step 5.5 pointer must state "never default to editing" main'
|
||||
);
|
||||
});
|
||||
|
||||
test('quick.md contains ISOLATED-RUN RECOVERY guardrail referencing #1292', () => {
|
||||
const content = fs.readFileSync(QUICK_WORKFLOW_PATH, 'utf-8');
|
||||
assert.ok(
|
||||
content.includes('ISOLATED-RUN RECOVERY'),
|
||||
'quick.md must contain the ISOLATED-RUN RECOVERY guardrail label'
|
||||
);
|
||||
assert.ok(
|
||||
content.includes('#1292'),
|
||||
'quick.md ISOLATED-RUN RECOVERY guardrail must reference #1292'
|
||||
);
|
||||
});
|
||||
|
||||
test('quick.md guardrail forbids defaulting recovery to main/primary checkout', () => {
|
||||
const content = fs.readFileSync(QUICK_WORKFLOW_PATH, 'utf-8');
|
||||
assert.ok(
|
||||
content.includes('never the proposed or default'),
|
||||
'quick.md guardrail must state editing main is "never the proposed or default" option'
|
||||
);
|
||||
});
|
||||
|
||||
test('quick.md guardrail offers fresh narrowly-scoped worktree as recovery path', () => {
|
||||
const content = fs.readFileSync(QUICK_WORKFLOW_PATH, 'utf-8');
|
||||
assert.ok(
|
||||
content.includes('fresh, narrowly-scoped worktree'),
|
||||
'quick.md guardrail must offer a "fresh, narrowly-scoped worktree" as recovery path'
|
||||
);
|
||||
});
|
||||
|
||||
test('quick.md guardrail requires explicit confirmation before editing primary checkout', () => {
|
||||
const content = fs.readFileSync(QUICK_WORKFLOW_PATH, 'utf-8');
|
||||
assert.ok(
|
||||
content.includes('explicit, clearly-labeled confirmation'),
|
||||
'quick.md guardrail must require "explicit, clearly-labeled confirmation" before editing the primary checkout'
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -24,7 +24,7 @@
|
||||
"docs-update.md": 54770,
|
||||
"edit-phase.md": 12883,
|
||||
"eval-review.md": 9923,
|
||||
"execute-phase.md": 93142,
|
||||
"execute-phase.md": 93122,
|
||||
"execute-plan.md": 31365,
|
||||
"explore.md": 10497,
|
||||
"extract-learnings.md": 12849,
|
||||
@@ -57,7 +57,7 @@
|
||||
"pr-branch.md": 9561,
|
||||
"profile-user.md": 20650,
|
||||
"progress.md": 29387,
|
||||
"quick.md": 46439,
|
||||
"quick.md": 47251,
|
||||
"reapply-patches.md": 20393,
|
||||
"remove-phase.md": 8469,
|
||||
"remove-workspace.md": 7507,
|
||||
|
||||
Reference in New Issue
Block a user