fix(#3968): measure commit claims at all three surfaces — ledger, verifier BLOCKER, porcelain HANDOFF (#4230)
* test(#3968): commit claims must be measured against git, never narrated * fix(#3968): measure commit claims at all three surfaces Emitted-Drift-Ack-Growth: gsd-executor.md — #3968 plan commit ledger and measured commits contract (held under the agent size cap) Emitted-Drift-Ack-Growth: verify-work.md — #3968 commit-claim reconciliation with the same rev-list instrument Emitted-Drift-Ack-Growth: pause-work.md — #3968 uncommitted_files from git status --porcelain * fix(#3968): retired slash syntax, allowlist line pin, git-compare test pin * fix(#3968): persist the ledger on disk and reconcile with the same rev-list instrument Emitted-Drift-Ack-Growth: gsd-executor.md — #3968 plan commit ledger and measured commits contract (held under the agent size cap) Emitted-Drift-Ack-Growth: verify-work.md — #3968 commit-claim reconciliation with the same rev-list instrument * fix(#3968): hold the gsd-executor size cap with a compact ledger contract Emitted-Drift-Ack-Growth: gsd-executor.md — #3968 plan commit ledger and measured commits contract (held under the agent size cap) * fix(#3968): allowlist pin and HALT regex track the final prose * chore(#3968): changeset for measured commit claims * chore(#3968): backfill changeset pr number * fix(#3968): quote the BASE expansion (SC2086) * ci: raise the test-lane budget 21 to 32 minutes (measured cost grew past the cap) --------- Co-authored-by: sim <sim@local>
This commit is contained in:
5
.changeset/zesty-cranes-dance.md
Normal file
5
.changeset/zesty-cranes-dance.md
Normal file
@@ -0,0 +1,5 @@
|
||||
---
|
||||
type: Fixed
|
||||
pr: 4230
|
||||
---
|
||||
**Executor commit claims are measured, not narrated** — the executor records the pre-plan HEAD and derives `commits:` from `git rev-list` (HALT if code sits uncommitted), `/gsd:verify-work` reconciles the claim against git with the same instrument and flags a mismatch as a BLOCKER, and HANDOFF's `uncommitted_files` comes from `git status --porcelain`. (#3968)
|
||||
4
.github/workflows/test.yml
vendored
4
.github/workflows/test.yml
vendored
@@ -171,7 +171,7 @@ jobs:
|
||||
# change beyond sharing the same job-level `timeout-minutes`.
|
||||
# tests/ci-test-job-timeout-budget.test.cjs holds every lane here to a
|
||||
# headroom factor over its own measured cost.
|
||||
timeout-minutes: 21
|
||||
timeout-minutes: 32
|
||||
env:
|
||||
GSD_PLUGIN_ROOT: .ci-gsd-plugin-root-disabled
|
||||
# #2665: a live-config leak fails the run on Linux/macOS lanes. Windows
|
||||
@@ -411,7 +411,7 @@ jobs:
|
||||
continue-on-error: true
|
||||
env:
|
||||
CI_JOB_LABEL: "test (${{ matrix.os }}, ${{ matrix.node-version }}${{ matrix.shard && format(', shard {0}', matrix.shard) || '' }})"
|
||||
CI_JOB_TIMEOUT_MINUTES: '21'
|
||||
CI_JOB_TIMEOUT_MINUTES: '32'
|
||||
run: node scripts/ci-check-job-near-cap.cjs
|
||||
|
||||
test-inert:
|
||||
|
||||
@@ -537,6 +537,18 @@ git add src/types/user.ts
|
||||
```bash
|
||||
gsd_run query commit-to-subrepo "{type}({phase}-{plan}): {concise task description}" --files file1 file2 ...
|
||||
```
|
||||
**0c. Plan commit ledger (#3968, single-repo — before the first commit):**
|
||||
Each Bash call is a FRESH shell, so the ledger persists on disk like the #3097 sentinel above
|
||||
(a variable would be unset at SUMMARY time and `rev-list ..HEAD` would measure zero).
|
||||
Per-plan filename, so sequential plans cannot contaminate each other:
|
||||
```bash
|
||||
_GSD_LEDGER="$(git rev-parse --git-dir)/gsd-plan-head-before-{phase}-{plan}"
|
||||
[ -f "$_GSD_LEDGER" ] || git rev-parse HEAD > "$_GSD_LEDGER"
|
||||
```
|
||||
The SUMMARY's `commits:` is MEASURED from this ledger, the base recorded as
|
||||
`plan_head_before:` for `/gsd:verify-work`'s same-instrument check. Multi-repo keeps commit-to-subrepo
|
||||
JSON hashes instead.
|
||||
|
||||
Returns JSON with per-repo commit hashes: `{ committed: true, repos: { "backend": { hash: "abc", files: [...] }, ... } }`. Record all hashes for SUMMARY.
|
||||
|
||||
**Otherwise (standard single-repo):**
|
||||
@@ -649,10 +661,22 @@ This file is the canonical output of this step. The orchestrator reads `.plannin
|
||||
actuals:
|
||||
tokens: 74000 # chars/4 over the files you actually changed
|
||||
tasks: 5 # tasks completed
|
||||
commits: 7 # commits made
|
||||
commits: 7 # MEASURED: git rev-list --count ${PLAN_HEAD_BEFORE}..HEAD (#3968)
|
||||
```
|
||||
These pair with the plan's `estimate` to calibrate future estimates (ADR-2629). Do not round to look closer to the estimate — a flattering number corrupts every later projection.
|
||||
|
||||
**`commits:` is measured, never narrated (#3968).** At SUMMARY write, read the persisted
|
||||
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)
|
||||
```
|
||||
Write BOTH into the frontmatter — `commits: ${COMMITS_ACTUAL}`,
|
||||
`plan_head_before: ${PLAN_HEAD_BEFORE}` — 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.
|
||||
|
||||
**Title:** `# Phase [X] Plan [Y]: [Name] Summary`
|
||||
|
||||
**One-liner must be substantive:**
|
||||
|
||||
@@ -106,13 +106,23 @@ timestamp=$(gsd_run query current-timestamp full --raw)
|
||||
"decisions": [
|
||||
{"decision": "{what}", "rationale": "{why}", "phase": "{phase_number}"}
|
||||
],
|
||||
"uncommitted_files": [],
|
||||
"uncommitted_files": ["XY path", "..."], # #3968: MEASURED — see below
|
||||
"next_action": "{specific first action when resuming}",
|
||||
"context_notes": "{mental state, approach, what you were thinking}"
|
||||
}
|
||||
```
|
||||
|
||||
Any recorded `async_jobs` entries are the primary resume context on the next session — check them first before treating a PLAN-without-SUMMARY as incomplete work.
|
||||
|
||||
**`uncommitted_files` is measured, never asserted (#3968).** Populate it from an actual call,
|
||||
not from memory — a narrated `[]` over a dirty tree is how 14 plans' worth of uncommitted
|
||||
code went invisible in the wild:
|
||||
```bash
|
||||
UNCOMMITTED=$(git status --porcelain)
|
||||
# One array entry per line ("XY path"); truncate the list at 50 entries and note the
|
||||
# elided count, but NEVER round it to empty — a non-empty porcelain output is the single
|
||||
# most load-bearing fact a resume session needs.
|
||||
```
|
||||
</step>
|
||||
|
||||
<step name="write">
|
||||
|
||||
@@ -172,6 +172,27 @@ ls "$phase_dir"/*-SUMMARY.md 2>/dev/null || true
|
||||
```
|
||||
|
||||
Read each SUMMARY.md to extract testable deliverables.
|
||||
|
||||
**Commit-claim reconciliation (#3968).** A SUMMARY's `commits:` frontmatter is a MEASURED
|
||||
number (the executor derives it from its on-disk plan commit ledger and records the base as
|
||||
`plan_head_before:`), and this is where that claim is checked against reality with the SAME
|
||||
instrument — the executor's own narration is never the last word. For each `*-SUMMARY.md`:
|
||||
```bash
|
||||
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)
|
||||
```
|
||||
- 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.
|
||||
</step>
|
||||
|
||||
<step name="extract_tests">
|
||||
|
||||
@@ -65,7 +65,16 @@ const LANE_COSTS = [
|
||||
// figure — rounded up from the higher, cancelled-run observation, since a
|
||||
// cancelled run's own timestamp is still real elapsed time even though the
|
||||
// job never finished.
|
||||
measuredMinutes: 14,
|
||||
// #4070's method applied again: the 14-minute figure went stale after
|
||||
// #4207's run-tests temp-root regression suite landed — its rows spawn
|
||||
// real runner instances on the windows scoped lane (each booting the full
|
||||
// build), and windows shards 1-2 were CANCELLED at 99% of the 21-minute
|
||||
// cap on three consecutive runs (33722188315 and two reruns; ubuntu and
|
||||
// macos lanes green throughout, other PRs' windows lanes green — the
|
||||
// long pole is this lane's windows matrix alone). A cancelled run's own
|
||||
// timestamp is still real elapsed time: >=21 minutes, so 21 is the
|
||||
// honest floor and the budget moves 21 -> 32 (1.5x headroom).
|
||||
measuredMinutes: 21,
|
||||
// Sharded three ways as of #2952, so this is ONE shard's cost, not the
|
||||
// whole unit suite. Shard 1 is the long pole because the unsharded aux
|
||||
// suites (integration/security/install/slow) ride on it — #4070 fixed the
|
||||
@@ -242,7 +251,7 @@ test('mutation.yml mutate job timeout budgets (#4036)', async (t) => {
|
||||
|
||||
test('near-cap check CI_JOB_TIMEOUT_MINUTES literals match each job\'s own timeout-minutes (#4036)', async (t) => {
|
||||
const staticLanes = [
|
||||
{ workflowFile: 'test.yml', jobKey: 'test', envLiteral: '21' },
|
||||
{ workflowFile: 'test.yml', jobKey: 'test', envLiteral: '32' },
|
||||
{ workflowFile: 'test.yml', jobKey: 'test-full', envLiteral: '45' },
|
||||
{ workflowFile: 'install-smoke.yml', jobKey: 'smoke', envLiteral: '12' },
|
||||
];
|
||||
|
||||
@@ -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: 795, 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: 819, reason: 'describes the SDK return envelope of `gsd-tools query commit`; not an instruction to run the bare word' },
|
||||
{ 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: 647, reason: 'parenthetical "e.g." naming SDK queries a user *could* run; not an agent instruction' },
|
||||
{ file: 'agents/gsd-intel-updater.md', line: 40, reason: 'cross-platform note names the `gsd-tools intel <subcommand>` CLI surface descriptively ("CLI invocations go through..."); not an agent instruction' },
|
||||
|
||||
61
tests/summary-commit-verification.test.cjs
Normal file
61
tests/summary-commit-verification.test.cjs
Normal file
@@ -0,0 +1,61 @@
|
||||
'use strict';
|
||||
|
||||
/**
|
||||
* #3968 — commit claims must be measured, never narrated.
|
||||
*
|
||||
* Across 14 plans / 3 phases in a real project, `commits: 1` appeared in every
|
||||
* SUMMARY while `git reflog` showed ZERO git activity in the window, and
|
||||
* HANDOFF.json asserted `uncommitted_files: []` over a dirty tree — invisible
|
||||
* because nothing downstream cross-checks the narration against git. The fix
|
||||
* (maintainer decision, option c) spans three shipped surfaces; their text IS
|
||||
* the runtime-loaded contract, so shape assertions are the faithful check.
|
||||
*/
|
||||
|
||||
const { test, describe } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const fs = require('node:fs');
|
||||
const path = require('node:path');
|
||||
|
||||
const ROOT = path.join(__dirname, '..');
|
||||
const read = (p) => fs.readFileSync(path.join(ROOT, p), 'utf8');
|
||||
|
||||
describe('#3968 — measured commit claims', () => {
|
||||
test('executor measures commits, never narrates them', () => {
|
||||
const executor = read('agents/gsd-executor.md');
|
||||
// The ledger: HEAD captured at the plan's first commit, once per plan.
|
||||
assert.ok(executor.includes('gsd-plan-head-before'),
|
||||
'the ledger must persist on disk (each Bash call is a fresh shell — a variable measures zero)');
|
||||
assert.ok(executor.includes('git rev-list --count'),
|
||||
'the SUMMARY commit count must come from git rev-list --count, not narration');
|
||||
assert.ok(executor.includes('plan_head_before: ${PLAN_HEAD_BEFORE}'),
|
||||
'the base is recorded in the frontmatter so the verifier reconciles with the same instrument');
|
||||
// A measured zero WITH code changes is a HALT, never a narrated success.
|
||||
assert.ok(/HALT — do not write the\s+SUMMARY/i.test(executor),
|
||||
'a measured zero with uncommitted code changes must halt the SUMMARY write');
|
||||
// actuals.commits sources the measured count; the ADR-2629 calibration shape is kept.
|
||||
assert.ok(/commits: 7 .*MEASURED/.test(executor),
|
||||
'the actuals block sources its count from the measurement');
|
||||
});
|
||||
|
||||
test('verify-work flags SUMMARY-vs-git mismatch as BLOCKER', () => {
|
||||
const verify = read('gsd-core/workflows/verify-work.md');
|
||||
assert.ok(verify.includes('Commit-claim reconciliation'),
|
||||
'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');
|
||||
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');
|
||||
});
|
||||
|
||||
test('HANDOFF uncommitted_files come from git status --porcelain', () => {
|
||||
const pause = read('gsd-core/workflows/pause-work.md');
|
||||
assert.ok(pause.includes('git status --porcelain'),
|
||||
'uncommitted_files must be populated from an actual git status --porcelain call');
|
||||
// The asserted-empty template literal is gone as the only source.
|
||||
const idx = pause.indexOf('uncommitted_files');
|
||||
assert.ok(idx === -1 || /porcelain/.test(pause.slice(Math.max(0, idx - 3000), idx + 3000)),
|
||||
'the uncommitted_files field is defined by the porcelain command, not a narrated []');
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user