diff --git a/.changeset/bold-otters-cheer.md b/.changeset/bold-otters-cheer.md new file mode 100644 index 000000000..301df7f0f --- /dev/null +++ b/.changeset/bold-otters-cheer.md @@ -0,0 +1,7 @@ +--- +type: Fixed +pr: 4813 +--- +**verify-work stops flagging honest plans as commit_claim_mismatch** — the reconciliation measured `plan_head_before..HEAD`, a window that grows with every later plan's commits and the phase-completion commit, so any plan except the last read as a BLOCKER. The executor now records `plan_head_after` (HEAD at its measurement moment) and verify-work reconciles against that bounded window with exact equality; legacy SUMMARYs without the anchor fall back to a warning, and the #3968 failure modes still block. (#4670) + + diff --git a/agents/gsd-executor.md b/agents/gsd-executor.md index 0f54fd324..12e05cfe5 100644 --- a/agents/gsd-executor.md +++ b/agents/gsd-executor.md @@ -673,9 +673,11 @@ ledger (protocol 0c — a fresh shell per Bash call; the base comes from disk): ```bash PLAN_HEAD_BEFORE=$(cat "$(git rev-parse --git-dir)/gsd-plan-head-before-{phase}-{plan}") COMMITS_ACTUAL=$(git rev-list --count ${PLAN_HEAD_BEFORE}..HEAD) +PLAN_HEAD_AFTER=$(git rev-parse HEAD) ``` -Write BOTH into the frontmatter — `commits: ${COMMITS_ACTUAL}`, -`plan_head_before: ${PLAN_HEAD_BEFORE}` — including when the count is `0`. +Write ALL THREE into the frontmatter — `commits: ${COMMITS_ACTUAL}`, +`plan_head_before: ${PLAN_HEAD_BEFORE}`, `plan_head_after: ${PLAN_HEAD_AFTER}` — including +when the count is `0`. A `0` with code changes means the changes sit UNCOMMITTED: **HALT — do not write the SUMMARY with a narrated count**; surface `git status --short` in your return. A `0` with no code changes (docs-only) is legitimate. `/gsd:verify-work` flags mismatches as BLOCKER. diff --git a/gsd-core/workflows/verify-work.md b/gsd-core/workflows/verify-work.md index db56f62e1..f51e5799a 100644 --- a/gsd-core/workflows/verify-work.md +++ b/gsd-core/workflows/verify-work.md @@ -183,18 +183,31 @@ instrument — the executor's own narration is never the last word. For each `*- BASE=$(grep -oE '^plan_head_before: [0-9a-f]{7,40}' "$SUMMARY_FILE" | awk '{print $2}') CLAIMED=$(grep -oE '^commits: [0-9]+' "$SUMMARY_FILE" | grep -oE '[0-9]+' || echo absent) ACTUAL=$(git rev-list --count "${BASE}"..HEAD) +AFTER=$(grep -oE '^plan_head_after: [0-9a-f]{7,40}' "$SUMMARY_FILE" | awk '{print $2}') ``` - A `commits: absent` or `plan_head_before: absent` SUMMARY (pre-#3968 legacy) is reported as a WARNING with the measured git state, not a mismatch. -- `ACTUAL == CLAIMED` is consistent. `ACTUAL == CLAIMED + 1` is ALSO consistent: the - SUMMARY/metadata commit itself lands after the executor measured, so exactly one - post-measurement commit is expected. -- Anything else is a **BLOCKER** — the phase must not read as done: real project evidence - (#3968) showed 14 plans declaring `commits: 1` with zero git activity, their code sitting - uncommitted and one `git reset --hard` from loss. Record it as `commit_claim_mismatch` - with both numbers and the SUMMARY path; a mismatch means either the executor narrated - instead of measuring or commits were lost after the fact — both require reconciliation - before the phase can pass. +- **Bounded reconciliation (#4670).** A SUMMARY carrying `plan_head_after:` (the executor's + HEAD at its measurement moment — after the last task commit, before the SUMMARY commit) is + reconciled against the plan's OWN window: +```bash +if git merge-base --is-ancestor "$AFTER" HEAD 2>/dev/null \ + && [ "$(git rev-list --count "${BASE}..${AFTER}")" = "$CLAIMED" ]; then + : # consistent +fi +``` + Consistent → done. Anything else is a **BLOCKER** — `commit_claim_mismatch` with both + numbers and the SUMMARY path: commits claimed but never made (#3968), task commits lost + after the fact, or the plan's recorded window rewritten afterwards (a rebase/amend/cherry-pick + of those commits makes `$AFTER` a non-ancestor — recount that plan's commits manually + before treating it as a genuine mismatch). The unbounded `${BASE}..HEAD` count is NOT + evidence either way: it grows with every later plan's commits and execute-phase's own + phase-completion commit, so an honest plan would read as a mismatch (#4670). +- **Legacy fallback (#4670).** A SUMMARY with a base but no `plan_head_after:` (pre-#4670) + cannot be bounded to its own window — report the measured `${BASE}..HEAD` count as a + **WARNING** with the SUMMARY's task-commit list for manual counting. The old + `ACTUAL == CLAIMED` / `ACTUAL == CLAIMED + 1` tolerance was a guess that later plans' + commits defeat; it must never produce a BLOCKER on the unsound window. diff --git a/tests/fixtures/compact-content-benchmark-baseline.json b/tests/fixtures/compact-content-benchmark-baseline.json index e55b42c82..46d6b7652 100644 --- a/tests/fixtures/compact-content-benchmark-baseline.json +++ b/tests/fixtures/compact-content-benchmark-baseline.json @@ -33,14 +33,14 @@ "reductionPct": 11.9 }, "verify-work": { - "offTokens": 12992, - "onTokens": 11019, - "reductionPct": 15.19 + "offTokens": 13235, + "onTokens": 11262, + "reductionPct": 14.91 } }, "aggregate": { - "offTokens": 108630, - "onTokens": 91943, - "reductionPct": 15.36 + "offTokens": 108873, + "onTokens": 92186, + "reductionPct": 15.33 } } diff --git a/tests/no-bare-gsd-tools-command-position.test.cjs b/tests/no-bare-gsd-tools-command-position.test.cjs index ab0dabbdf..798e92941 100644 --- a/tests/no-bare-gsd-tools-command-position.test.cjs +++ b/tests/no-bare-gsd-tools-command-position.test.cjs @@ -103,7 +103,7 @@ const BARE_COMMAND_RE = new RegExp( // Each entry MUST carry a one-line reason; the test prints the allowlist on // failure so a reviewer can see exactly what is sanctioned. const PROSE_ALLOWLIST = [ - { file: 'agents/gsd-executor.md', line: 823, reason: 'describes the SDK return envelope of `gsd-tools query commit`; not an instruction to run the bare word' }, + { file: 'agents/gsd-executor.md', line: 825, reason: 'describes the SDK return envelope of `gsd-tools query commit`; not an instruction to run the bare word (#4670 shifted it from 823: the plan_head_after capture line added above moved the line, the mention is unchanged)' }, { file: 'agents/gsd-phase-researcher.md', line: 33, reason: 'package-legitimacy provenance rule names the command as the source of an OK verdict; descriptive' }, { file: 'agents/gsd-roadmapper.md', line: 660, reason: 'parenthetical "e.g." naming SDK queries a user *could* run; not an agent instruction (#4134 shifted it from 647: the H1 template section added above moved the line, the mention is unchanged)' }, { file: 'agents/gsd-intel-updater.md', line: 40, reason: 'cross-platform note names the `gsd-tools intel ` CLI surface descriptively ("CLI invocations go through..."); not an agent instruction' }, diff --git a/tests/summary-commit-verification.test.cjs b/tests/summary-commit-verification.test.cjs index 22b5bea30..672684cf0 100644 --- a/tests/summary-commit-verification.test.cjs +++ b/tests/summary-commit-verification.test.cjs @@ -43,8 +43,16 @@ describe('#3968 — measured commit claims', () => { 'verify-work must run a commit-claim reconciliation over each SUMMARY'); assert.ok(verify.includes('ACTUAL=$(git rev-list --count "${BASE}"..HEAD)'), 'the reconciliation uses the SAME instrument as the executor (rev-list over the recorded base)'); - assert.ok(/ACTUAL == CLAIMED \+ 1/.test(verify), - 'the post-measurement SUMMARY commit is an expected +1, not a false BLOCKER'); + // #4670: the +1 tolerance is retired for bounded SUMMARYs (exact equality + // over plan_head_before..plan_head_after); the literal survives only in + // the legacy-fallback retirement sentence. Pin the shipped rule, not the + // retired tolerance: + assert.match(verify, /Bounded reconciliation \(#4670\)/, + 'the bounded exact-equality reconciliation is the shipped rule'); + const reconciliationIdx = verify.indexOf('Commit-claim reconciliation'); + const legacySentenceIdx = verify.indexOf('`ACTUAL == CLAIMED + 1` tolerance was a guess'); + assert.ok(legacySentenceIdx > reconciliationIdx, + 'the +1 tolerance appears only as the retired legacy rule inside the reconciliation block'); assert.ok(/BLOCKER/.test(verify.slice(verify.indexOf('Commit-claim reconciliation'), verify.indexOf('Commit-claim reconciliation') + 1800)), 'a mismatch must be flagged BLOCKER — the phase must not read as done'); }); @@ -59,3 +67,61 @@ describe('#3968 — measured commit claims', () => { 'the uncommitted_files field is defined by the porcelain command, not a narrated []'); }); }); + +// ── #4670 — the commit-claim window is bounded to the plan's own history ───── +// `plan_head_before..HEAD` grows with every LATER plan's commits and +// execute-phase's phase-completion commit, so an honest plan flagged as +// `commit_claim_mismatch` BLOCKER as soon as anything landed after it. The +// executor now also records `plan_head_after:` (HEAD at its measurement +// moment — after the last task commit, before the SUMMARY commit), and +// verify-work reconciles against that bounded window with EXACT equality; +// legacy SUMMARYs without the anchor fall back to the WARNING path. + +describe('#4670 — bounded commit-claim reconciliation', () => { + test('executor records plan_head_after — the plan window bound (#4670)', () => { + const executor = read('agents/gsd-executor.md'); + assert.ok( + executor.includes('plan_head_after: ${PLAN_HEAD_AFTER}'), + 'the SUMMARY frontmatter must carry plan_head_after, captured at the measurement moment' + ); + assert.ok( + /PLAN_HEAD_AFTER=\$\(git rev-parse HEAD\)/.test(executor), + 'PLAN_HEAD_AFTER must be captured from HEAD at the same measurement moment as the count' + ); + }); + + test('verify-work bounds the commit-claim window to the plan own history (#4670)', () => { + const verify = read('gsd-core/workflows/verify-work.md'); + const afterIdx = verify.indexOf('AFTER=$(grep -oE \'^plan_head_after: [0-9a-f]{7,40}\' "$SUMMARY_FILE" | awk \'{print $2}\')'); + assert.ok(afterIdx !== -1, 'the reconciliation must extract plan_head_after'); + assert.ok( + verify.includes('git merge-base --is-ancestor "$AFTER" HEAD'), + 'the plan end must be verified to still be in history (ancestor check)' + ); + assert.ok( + verify.includes('git rev-list --count "${BASE}..${AFTER}"'), + 'the measured count must be bounded to the plan own window (BASE..AFTER)' + ); + }); + + test('bounded mismatch stays a BLOCKER — #3968 failures still caught (#4670)', () => { + const verify = read('gsd-core/workflows/verify-work.md'); + const boundedIdx = verify.indexOf('#4670'); + assert.ok(boundedIdx !== -1, 'the bounded reconciliation section must exist'); + const section = verify.slice(boundedIdx, boundedIdx + 2400); + assert.match(section, /BLOCKER/, 'a bounded mismatch must remain a BLOCKER'); + assert.match(section, /commit_claim_mismatch/, 'the mismatch verdict name is kept'); + }); + + test('legacy SUMMARYs without plan_head_after fall back to the warning path (#4670)', () => { + const verify = read('gsd-core/workflows/verify-work.md'); + const afterNeedle = 'plan_head_after'; + assert.ok(verify.includes(afterNeedle), 'plan_head_after must appear in verify-work'); + // The legacy clause: a base without the anchor cannot be judged by the + // unsound unbounded window — it is a WARNING with the measured state. + assert.match( + verify, /WARNING[\s\S]{0,400}plan_head_after/, + 'SUMMARYs without plan_head_after must fall back to the WARNING path, not the unsound BLOCKER' + ); + }); +});