* test(#3684): failing-first rows for the verified-unmarked resume * fix(#3684): resume verified-unmarked phases at update_roadmap * test(#3684): heading-shaped roadmap fixture, plain phase.complete calls * fix(#3684): fit under the pre-phase-6 margin, fix pins and verify call * fix(#3684): padding-normalize the marked-complete join, assert STATE idempotency * test(#3684): anchor fixes, node jq mirror, characterized STATE delta * chore(#3684): backfill changeset pr number --------- Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
5
.changeset/brave-jays-tumble.md
Normal file
5
.changeset/brave-jays-tumble.md
Normal file
@@ -0,0 +1,5 @@
|
||||
---
|
||||
type: Fixed
|
||||
pr: 3814
|
||||
---
|
||||
**Resuming `/gsd-execute-phase` on a phase whose verification passed but whose run died before marking complete now finishes the job** — the phase is marked complete, progress state advances, phase todos close, and the transition handoff runs, instead of every resume reporting "nothing to do" while the roadmap checkbox stays unticked forever. Already-completed phases keep exiting cleanly, and verification is never redone. (#3684)
|
||||
@@ -350,6 +350,10 @@ are done. Blocked-and-incomplete must never be reported as finished.
|
||||
|
||||
```bash
|
||||
VERIFY_STATUS=$(gsd_run query verification status "${PHASE_DIR}" --pick status)
|
||||
# #3684: checkbox = marked-complete; report fields can claim a no-op write (#3685).
|
||||
ANALYZE=$(gsd_run query roadmap.analyze)
|
||||
if [[ "$ANALYZE" == @file:* ]]; then ANALYZE=$(cat "${ANALYZE#@file:}"); fi
|
||||
PHASE_MARKED=$(echo "$ANALYZE"|jq -r --arg p "$PHASE_NUMBER" 'def n:sub("^0+(?=[0-9])";"");.phases[]|select(((.number//.phase_number|tostring|n))==($p|n))|.roadmap_complete'|head -1)
|
||||
```
|
||||
|
||||
Evaluate in this exact order — the first matching condition decides the outcome; do not evaluate
|
||||
@@ -366,8 +370,6 @@ later conditions once one matches:
|
||||
→ exit. Do not fall through to condition 3; this is not a completion state.
|
||||
3. **No filter is active, and every filtered plan was filtered by `has_summary` alone** (no
|
||||
blocked-plan skip occurred):
|
||||
- **`VERIFY_STATUS` is anything other than `missing`**: the phase genuinely finished. Report
|
||||
"No matching incomplete plans" → exit, unchanged.
|
||||
- **`VERIFY_STATUS == missing`**: the plans are all summarized but the run never reached the
|
||||
tail gates. Report:
|
||||
`"All {plan_count} plans are summarized but no VERIFICATION.md exists — resuming at the phase gates (#2868)."`
|
||||
@@ -382,6 +384,13 @@ later conditions once one matches:
|
||||
skip `aggregate_results`, `code_review_gate` or `regression_gate` on this path — the manual
|
||||
workaround this replaces skipped all three, and that gap is the reason this route exists
|
||||
rather than telling users to spawn the verifier by hand.
|
||||
- **`VERIFY_STATUS` ≠ `missing` + `PHASE_MARKED` is `true`**: genuinely finished.
|
||||
Report "No matching incomplete plans" → exit, unchanged.
|
||||
- **`VERIFY_STATUS` ≠ `missing` + `PHASE_MARKED` not `true`** — the run died between
|
||||
`verify_phase_goal` and `update_roadmap` (#3684): verification EXISTS — do not redo
|
||||
it or the gates already run. Report `"Phase {X} is verified but never marked
|
||||
complete — resuming at update_roadmap (#3684)."` and continue directly at
|
||||
`update_roadmap`; the tail steps then run in their normal order.
|
||||
|
||||
Report:
|
||||
```
|
||||
|
||||
@@ -2,6 +2,6 @@
|
||||
"$comment": "Growth ack (#2914 fragment). RE-ARMED for #3683 (+238B, gated extract-learnings wiring in auto_copy_learnings); prior #3003 reason retained after the em-dash. Reason: #3003 threads a plan-declared deletion list from plan frontmatter to the cleanup-wave deletions guard. execute-phase.md gains --deletions \"$PLAN_DELETIONS\" on the record-agent call; 92326 -> 92356 LF bytes (+30). Supersedes the spent #2856 fragment entry (tests/emitted-drift-acks/2856-live-dom-uat.json, merged into next so its ripple is already absorbed at the base and it can no longer clear anything), which also named execute-phase.md and would otherwise double-ack the same path — the same supersede that entry itself performed on the spent #3370 fragment, which had performed it on #3324. Only this path needs an entry: currentSizes() (tests/helpers/emitted-runtime.cjs:916-929) reads gsd-core/workflows/ and agents/ with a NON-recursive readdirSync that skips directories, so the two other grown shipped files are outside the growth ratchet — gsd-core/workflows/execute-phase/steps/per-plan-worktree-gate.md (+885) sits in a subdirectory and gsd-core/templates/phase-prompt.md (+285) is under templates/. Their emitted-hash ripples are attributable to this diff and need no acknowledgment.",
|
||||
"version": 1,
|
||||
"paths": {
|
||||
"execute-phase.md": "gsd-core/workflows/execute-phase.md +238B: auto_copy_learnings instructs running extract-learnings for the just-completed phase when features.global_learnings is enabled (gate check first; extraction failure explicitly non-fatal) before the copy — the step the registry always named as a producer becomes one (#3683). Supersedes the spent #3003 entry (+30B, --deletions \"$PLAN_DELETIONS\"), same supersede chain that entry itself performed on #2856."
|
||||
"execute-phase.md": "gsd-core/workflows/execute-phase.md +1786B: #3684 wires condition 3 of discover_and_group_plans to roadmap.analyze's roadmap_complete (bash read + three-way branch) — the verified-but-never-marked-complete shape now resumes at update_roadmap instead of exiting clean forever. Supersedes the spent #3683 entry (gated extract-learnings in auto_copy_learnings), same chain #3683 ← #3003 ← #2856 ← #3370 ← #3324."
|
||||
}
|
||||
}
|
||||
|
||||
@@ -943,3 +943,242 @@ describe('issue #3210: execute-phase auto-mode carve-out exempts precondition-un
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
// ─── #3684 — verified-but-never-marked-complete resume ───────────────────────
|
||||
//
|
||||
// #2868 covered stranding BEFORE verification; the symmetric gap one step later
|
||||
// (VERIFICATION.md written, run died before update_roadmap) made condition 3's
|
||||
// "genuinely finished" bullet exit cleanly forever — the phase stays verified on
|
||||
// disk but permanently unticked, with update_roadmap / auto_copy_learnings /
|
||||
// close_phase_todos / the transition handoff never running. The fix: that branch
|
||||
// reads the ROADMAP's own completion marker (roadmap.analyze's roadmap_complete —
|
||||
// the #2245-hardened, #3537-padding-tolerant read; NEVER a completion report
|
||||
// field, which #3685 shows can claim a write that didn't happen) and resumes at
|
||||
// update_roadmap when the marker is unticked, without redoing verification.
|
||||
|
||||
describe('execute-phase workflow: #3684 verified-unmarked resume', () => {
|
||||
function stepText() {
|
||||
const content = fs.readFileSync(WORKFLOW_PATH, 'utf-8');
|
||||
const start = content.indexOf('<step name="discover_and_group_plans">');
|
||||
const end = content.indexOf('</step>', start);
|
||||
return content.slice(start, end);
|
||||
}
|
||||
|
||||
test('condition 3 reads the roadmap marker, not VERIFY_STATUS alone', () => {
|
||||
const step = stepText();
|
||||
assert.ok(
|
||||
/roadmap\.analyze/.test(step),
|
||||
'the finished branch must query roadmap.analyze for the completion marker',
|
||||
);
|
||||
assert.ok(
|
||||
/roadmap_complete/.test(step),
|
||||
'the finished branch must read roadmap_complete — the authoritative checkbox state',
|
||||
);
|
||||
// Report fields may be NAMED in the prohibition prose — what is
|
||||
// forbidden is reading them as the signal (a command/jq consumption).
|
||||
const branchBash = step.split('\n').filter((l) => /jq|gsd_run|pick/.test(l)).join('\n');
|
||||
assert.ok(
|
||||
!/roadmap_updated|state_updated/.test(branchBash),
|
||||
'completion report fields must never be READ as the already-complete signal (#3685)',
|
||||
);
|
||||
});
|
||||
|
||||
test('verified-unmarked resume continues at update_roadmap', () => {
|
||||
const step = stepText();
|
||||
const branch = step.slice(step.indexOf('VERIFY_STATUS` ≠ `missing` + `PHASE_MARKED` not `true`'));
|
||||
assert.ok(
|
||||
branch.includes('update_roadmap'),
|
||||
'the unmarked-resume branch must continue at update_roadmap',
|
||||
);
|
||||
assert.ok(
|
||||
/not.*redo|do not redo|without redoing|verification already|already exists/i.test(branch),
|
||||
'the branch must state verification is not redone',
|
||||
);
|
||||
assert.ok(
|
||||
branch.includes('#3684'),
|
||||
'the branch must cite #3684',
|
||||
);
|
||||
});
|
||||
|
||||
test('genuinely-finished exit and the 2868 branch are unchanged', () => {
|
||||
const step = stepText();
|
||||
assert.ok(
|
||||
step.includes('"No matching incomplete plans"'),
|
||||
'the genuinely-finished clean exit text stays',
|
||||
);
|
||||
assert.ok(
|
||||
/`VERIFY_STATUS == missing`/.test(step),
|
||||
'the #2868 missing-verification branch stays',
|
||||
);
|
||||
assert.ok(
|
||||
step.includes('resuming at the phase gates (#2868)'),
|
||||
'the #2868 resume message stays byte-recognizable',
|
||||
);
|
||||
});
|
||||
|
||||
function buildStrandedFixture(t, { ticked }) {
|
||||
const proj = createTempProject('gsd-3684-');
|
||||
t.after(() => cleanup(proj));
|
||||
const phaseDir = path.join(proj, '.planning', 'phases', '01-alpha');
|
||||
fs.mkdirSync(phaseDir, { recursive: true });
|
||||
fs.writeFileSync(path.join(phaseDir, 'P001-PLAN.md'), [
|
||||
'---', 'wave: 1', 'objective: Do the thing', 'autonomous: true', 'depends_on: []', '---', '',
|
||||
'# Plan 001', '', '<objective>Do the thing</objective>', '', '<task>Do it</task>',
|
||||
].join('\n'));
|
||||
fs.writeFileSync(path.join(phaseDir, 'P001-SUMMARY.md'), '# Summary\nDone.\n');
|
||||
fs.writeFileSync(path.join(phaseDir, '01-alpha-VERIFICATION.md'), [
|
||||
'---', 'status: passed', '---', '', '# Verification', '', 'PASS',
|
||||
].join('\n'));
|
||||
// Heading-shaped ROADMAP (analyze's parser keys on "### Phase N:" under
|
||||
// a sections heading); the checkbox line carries the marked-complete
|
||||
// marker roadmap_complete reads (- [x]/**Phase 1** vs - [ ]).
|
||||
fs.writeFileSync(path.join(proj, '.planning', 'ROADMAP.md'), [
|
||||
'# Roadmap v1.0', '', '## Phases', '',
|
||||
'### Phase 1: Alpha', '**Goal:** First phase', '',
|
||||
`- [${ticked ? 'x' : ' '}] **Phase 1: Alpha** - First phase`, '',
|
||||
].join('\n'));
|
||||
return { proj, phaseDir };
|
||||
}
|
||||
|
||||
test('routing queries yield passed + unmarked on the stranded fixture', (t) => {
|
||||
const { proj } = buildStrandedFixture(t, { ticked: false });
|
||||
const verify = runGsdTools(`verification status ${path.join(proj, '.planning', 'phases', '01-alpha')} --pick status`, proj);
|
||||
const analyze = runGsdTools(['roadmap.analyze', '--json'], proj);
|
||||
assert.ok(analyze.success, `roadmap.analyze should succeed: ${analyze.error}`);
|
||||
const phases = JSON.parse(analyze.output).phases || [];
|
||||
const p1 = phases.find((p) => String(p.number) === '1');
|
||||
assert.ok(p1, `phase 1 should appear in analyze output: ${JSON.stringify(phases)}`);
|
||||
assert.equal(p1.roadmap_complete, false, 'unticked checkbox must read as not complete');
|
||||
// The step's decision inputs: VERIFY_STATUS != missing AND !PHASE_MARKED →
|
||||
// the #3684 resume route, not the finished exit.
|
||||
assert.ok(verify.success, `verification status query should succeed: ${verify.error}`);
|
||||
assert.notEqual(String(verify.output).trim(), 'missing', 'the stranded fixture has a verification artifact');
|
||||
});
|
||||
|
||||
test('routing queries yield marked after completion', (t) => {
|
||||
const { proj } = buildStrandedFixture(t, { ticked: true });
|
||||
const analyze = runGsdTools(['roadmap.analyze', '--json'], proj);
|
||||
const phases = JSON.parse(analyze.output).phases || [];
|
||||
const p1 = phases.find((p) => String(p.number) === '1');
|
||||
assert.ok(p1, 'phase 1 should appear');
|
||||
assert.equal(p1.roadmap_complete, true, 'ticked checkbox must read as complete');
|
||||
});
|
||||
|
||||
test('phase.complete is idempotent on the already-complete fixture', (t) => {
|
||||
const { proj } = buildStrandedFixture(t, { ticked: false });
|
||||
const first = runGsdTools('phase complete 1', proj);
|
||||
assert.ok(first.success, `first completion should succeed: ${first.error}`);
|
||||
const roadmapAfterFirst = fs.readFileSync(path.join(proj, '.planning', 'ROADMAP.md'), 'utf-8');
|
||||
assert.ok(/\[x\]/.test(roadmapAfterFirst), 'first completion ticks the checkbox');
|
||||
const second = runGsdTools('phase complete 1', proj);
|
||||
assert.ok(second.success, 'second completion should succeed (no-op)');
|
||||
const roadmapAfterSecond = fs.readFileSync(path.join(proj, '.planning', 'ROADMAP.md'), 'utf-8');
|
||||
assert.equal(roadmapAfterSecond, roadmapAfterFirst, 'second run must leave ROADMAP byte-identical (criterion 3)');
|
||||
});
|
||||
});
|
||||
|
||||
describe('execute-phase workflow: #3684 review findings — join normalization', () => {
|
||||
const WORKFLOW_JQ_RE = /PHASE_MARKED=\$\(echo "\$ANALYZE"\|jq -r --arg p "\$PHASE_NUMBER" '([^']+)'\|head -1\)/;
|
||||
|
||||
function extractWorkflowJq() {
|
||||
const content = fs.readFileSync(WORKFLOW_PATH, 'utf-8');
|
||||
const m = content.match(WORKFLOW_JQ_RE);
|
||||
assert.ok(m, 'the workflow must carry the PHASE_MARKED jq line');
|
||||
return m[1];
|
||||
}
|
||||
|
||||
test('the join key is padding-normalized on both sides (drifted shapes route correctly)', (t) => {
|
||||
// The join must tolerate the real-world drift every other resolver in the
|
||||
// pipeline tolerates (#3537/#3572/#2528): directory token "01" vs ROADMAP
|
||||
// heading "1" and vice versa. An exact-string compare misroutes a
|
||||
// verified AND ticked phase into the resume branch forever — the same
|
||||
// "permanently stuck" shape #3684 exists to fix, one level up.
|
||||
const jqFilter = extractWorkflowJq();
|
||||
assert.ok(
|
||||
/sub\("\^0\+\(\?=\[0-9\]\)";""\)/.test(jqFilter),
|
||||
`the jq must strip leading zeros on both sides: ${jqFilter}`,
|
||||
);
|
||||
|
||||
const proj = createTempProject('gsd-3684-pad-');
|
||||
t.after(() => cleanup(proj));
|
||||
const phaseDir = path.join(proj, '.planning', 'phases', '01-alpha');
|
||||
fs.mkdirSync(phaseDir, { recursive: true });
|
||||
fs.writeFileSync(path.join(phaseDir, 'P001-SUMMARY.md'), '# Summary\n');
|
||||
// Heading UNPADDED while the directory token is PADDED — the drifted shape.
|
||||
fs.writeFileSync(path.join(proj, '.planning', 'ROADMAP.md'), [
|
||||
'# Roadmap v1.0', '', '## Phases', '',
|
||||
'### Phase 1: Alpha', '**Goal:** g', '',
|
||||
'- [x] **Phase 1: Alpha** - done', '',
|
||||
].join('\n'));
|
||||
|
||||
const analyze = runGsdTools('roadmap.analyze --json', proj);
|
||||
assert.ok(analyze.success, `roadmap.analyze should succeed: ${analyze.error}`);
|
||||
// Evaluate the extracted filter's join semantics via a node mirror of
|
||||
// `sub("^0+(?=[0-9])";"")` — the bench hosts no jq binary, so the
|
||||
// filter string itself is pinned structurally above and its normalization
|
||||
// semantics are replayed here against real analyze output.
|
||||
const stripPad = (s) => String(s).replace(/^0+(?=[0-9])/, '');
|
||||
const phases = JSON.parse(analyze.output).phases || [];
|
||||
const evalJoin = (p) => {
|
||||
const hit = phases.find((ph) => stripPad(ph.number ?? ph.phase_number) === stripPad(p));
|
||||
return hit ? String(hit.roadmap_complete) : '';
|
||||
};
|
||||
// $p as init derives it (directory token "01") and the unpadded form —
|
||||
// both must find the ticked phase under the unpadded heading.
|
||||
for (const p of ['01', '1']) {
|
||||
assert.equal(evalJoin(p), 'true', `padded $p=${p} must match the unpadded heading`);
|
||||
}
|
||||
});
|
||||
|
||||
test('idempotency row carries STATE.md and tolerates only the last_updated delta', (t) => {
|
||||
// Criterion 3 says "no observable change to roadmap OR tracked progress
|
||||
// state". The ROADMAP half is byte-identity; the STATE half legitimately
|
||||
// touches only last_updated (the transition runs unconditionally — the
|
||||
// triage's own empirical finding). Assert both halves explicitly.
|
||||
const proj = createTempProject('gsd-3684-state-');
|
||||
t.after(() => cleanup(proj));
|
||||
const phaseDir = path.join(proj, '.planning', 'phases', '01-alpha');
|
||||
fs.mkdirSync(phaseDir, { recursive: true });
|
||||
fs.writeFileSync(path.join(phaseDir, 'P001-PLAN.md'), [
|
||||
'---', 'wave: 1', 'objective: x', 'autonomous: true', 'depends_on: []', '---', '', '# P', '',
|
||||
'<objective>x</objective>', '', '<task>t</task>',
|
||||
].join('\n'));
|
||||
fs.writeFileSync(path.join(phaseDir, 'P001-SUMMARY.md'), '# Summary\n');
|
||||
fs.writeFileSync(path.join(phaseDir, '01-alpha-VERIFICATION.md'), '---\nstatus: passed\n---\n');
|
||||
fs.writeFileSync(path.join(proj, '.planning', 'ROADMAP.md'), [
|
||||
'# Roadmap v1.0', '', '## Phases', '',
|
||||
'### Phase 1: Alpha', '**Goal:** g', '',
|
||||
'- [ ] **Phase 1: Alpha** - todo', '',
|
||||
].join('\n'));
|
||||
fs.writeFileSync(path.join(proj, '.planning', 'STATE.md'), [
|
||||
'---', 'current_phase: 1', 'current_phase_name: Alpha', 'status: executing', '',
|
||||
'last_updated: 2026-01-01', '---', '', '# STATE', '',
|
||||
].join('\n'));
|
||||
|
||||
const first = runGsdTools('phase complete 1', proj);
|
||||
assert.ok(first.success, `first completion should succeed: ${first.error}`);
|
||||
const roadmap1 = fs.readFileSync(path.join(proj, '.planning', 'ROADMAP.md'), 'utf-8');
|
||||
const state1 = fs.readFileSync(path.join(proj, '.planning', 'STATE.md'), 'utf-8');
|
||||
assert.ok(/\[x\]/.test(roadmap1), 'first completion ticks the checkbox');
|
||||
|
||||
const second = runGsdTools('phase complete 1', proj);
|
||||
assert.ok(second.success, 'second completion should succeed');
|
||||
const roadmap2 = fs.readFileSync(path.join(proj, '.planning', 'ROADMAP.md'), 'utf-8');
|
||||
const state2 = fs.readFileSync(path.join(proj, '.planning', 'STATE.md'), 'utf-8');
|
||||
assert.equal(roadmap2, roadmap1, 'ROADMAP byte-identical on re-run');
|
||||
// Characterized delta (empirically pinned): a re-run never rewrites or
|
||||
// removes existing STATE content — it may only refresh the last_updated
|
||||
// stamp and ADD metrics keys the first run withheld (percent: the #3318
|
||||
// withhold lifts once the scope reads complete). Assert state2 is an
|
||||
// ordered superset of state1 modulo the stamp.
|
||||
const keep = (s) => s.split('\n').filter((l) => !/^last_updated:/.test(l));
|
||||
const lines1 = keep(state1);
|
||||
const lines2 = keep(state2);
|
||||
let i2 = 0;
|
||||
for (const line of lines1) {
|
||||
const at = lines2.indexOf(line, i2);
|
||||
assert.ok(at !== -1, `re-run must not rewrite STATE content; missing after ${i2}: ${line}`);
|
||||
i2 = at + 1;
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user