fix(execute): the merge lock refuses a review of an older commit (review-stale)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
6c0718c8d8
commit
050a77f513
SSH key fingerprint:
SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
4 changed files with 157 additions and 16 deletions
|
|
@ -2265,12 +2265,16 @@ never typed by the operator, in this order (Cherny's):
|
|||
next round's checks or for the merge.
|
||||
4. **`verdict-gate.mjs`** — the review's verdict decides, as a command:
|
||||
```bash
|
||||
node ${CLAUDE_PLUGIN_ROOT}/lib/review/verdict-gate.mjs --json "{project_dir}/review.md"
|
||||
node ${CLAUDE_PLUGIN_ROOT}/lib/review/verdict-gate.mjs --json "{project_dir}/review.md" --repo .
|
||||
```
|
||||
Record `review = {round, path, gate_exit, verdict}`. Exit 0 (ALLOW/WARN) →
|
||||
Phase 7.5. Exit 2 (could not decide, or a reviewer or engine that did not
|
||||
run) → Phase 7.5 with the merge closed: never green. Exit 1 (BLOCK) → the
|
||||
loop below.
|
||||
`--repo .` checks that an ALLOW/WARN review is of the commit being merged:
|
||||
its `scope_sha_end` is HEAD, or an ancestor of HEAD whose later commits
|
||||
change only the review lessons and the `CLAUDE.md` pointer (step 3's
|
||||
commit). Record `review = {round, path, gate_exit, verdict}`. Exit 0
|
||||
(ALLOW/WARN) → Phase 7.5. Exit 2 (could not decide, a reviewer or engine
|
||||
that did not run, or a reason that starts `review-stale`: a review of
|
||||
another commit) → Phase 7.5 with the merge closed: never green. Exit 1
|
||||
(BLOCK) → the loop below.
|
||||
|
||||
**A BLOCK is fixed by new plan steps, in a bounded loop.** The review writes
|
||||
every BLOCKER as a numbered `### Step R<n>` under `## Remediation Steps`
|
||||
|
|
@ -2583,7 +2587,7 @@ event, merge nothing, and their result is left as Phase 7.5 decided it
|
|||
a summary:
|
||||
|
||||
```bash
|
||||
node ${CLAUDE_PLUGIN_ROOT}/lib/review/verdict-gate.mjs --json "{review_path}"
|
||||
node ${CLAUDE_PLUGIN_ROOT}/lib/review/verdict-gate.mjs --json "{review_path}" --repo .
|
||||
```
|
||||
|
||||
`{review_path}` is `review.path` from Phase 7.2. No review (Phase 7.2 did not
|
||||
|
|
@ -2596,13 +2600,20 @@ exit 2 without the call.
|
|||
| 1 | BLOCK | closed |
|
||||
| 2 | not measured | closed: a review that could not decide is never green |
|
||||
|
||||
`--repo .` makes the gate check the review against the commit being merged.
|
||||
Exit 2 with a reason that starts `review-stale` is closed with the decline
|
||||
reason `review-stale` and the line "re-run /trekreview on this commit": the
|
||||
review is of another commit than this one, or something other than the
|
||||
review lessons and the `CLAUDE.md` pointer changed after it. Any other exit 2
|
||||
is `review-not-measured`.
|
||||
|
||||
The merge runs only on exit 0 and a `completed` result. **Closed:** emit
|
||||
`main-merge-declined` with the reason `review-block`, `review-not-measured` or
|
||||
`run-not-completed`. Nothing is finalized: a `completed` result becomes
|
||||
`main-merge-declined` with the reason `review-block`, `review-stale`,
|
||||
`review-not-measured` or `run-not-completed`. Nothing is finalized: a `completed` result becomes
|
||||
`partial` (`failed` and `stopped` stay), and the commits stay where they are
|
||||
for review. Say in one line why it is closed and what opens it: the
|
||||
BLOCKERs fixed through Phase 7.2's loop, a review that runs in full, or a run
|
||||
that completes.
|
||||
BLOCKERs fixed through Phase 7.2's loop, a review that runs in full, a review
|
||||
of this commit, or a run that completes.
|
||||
|
||||
**2. Open: the merge, and the one question.** Find the run's branch and where
|
||||
main is checked out, in a Bash call of its own:
|
||||
|
|
@ -2706,7 +2717,7 @@ JSON
|
|||
```
|
||||
|
||||
The reason is one of the literal words above (`review-block`,
|
||||
`review-not-measured`, `run-not-completed`, `no-main-branch`, `hold-for-review`,
|
||||
`review-stale`, `review-not-measured`, `run-not-completed`, `no-main-branch`, `hold-for-review`,
|
||||
`merge-conflict`, `merge-refused`), never the operator's free text (free text breaks the JSON).
|
||||
|
||||
On decline, nothing is cleared: the merged-waves record is kept.
|
||||
|
|
@ -2714,8 +2725,9 @@ On decline, nothing is cleared: the merged-waves record is kept.
|
|||
**Recovery surface:** on the single-session path, if declined,
|
||||
`--resume re-enters at the gate` — re-running
|
||||
`/trekexecute --resume --project <dir>` skips the completed steps and returns
|
||||
to the main-merge gate: the lock reads `review.md` again (a review fixed or
|
||||
re-run in between counts), and when it opens, the guard asks again. On the
|
||||
to the main-merge gate: the lock reads the review again (a review fixed or re-run
|
||||
in between counts) and runs the same gate against this repo, so a review of
|
||||
an older commit stays refused; when it opens, the guard asks again. On the
|
||||
multi-session wave path,
|
||||
re-run the same `/trekexecute` command instead, not `--resume` (Phase 2.5 runs
|
||||
a `--resume` as a single session): the waves are already merged on the current
|
||||
|
|
|
|||
|
|
@ -431,7 +431,7 @@ missing `exit_code` (C15, `PROGRESS_FAILED_CRITERIA_KEY`).
|
|||
|
||||
**Consumer:** `/trekplan` Phase 1 when `--brief review.md` is supplied and the consumer detects `type: trekreview` in frontmatter. The plan command branches into a remediation-plan path: BLOCKER + MAJOR findings become plan goals, the produced `plan.md` carries a `source_findings: [<id>, ...]` frontmatter list as the audit trail back to the consumed findings. MINOR + SUGGESTION are skipped for plan-input. Since 1.1 every BLOCKER is also written as a numbered `### Step R<n>` under `## Remediation Steps` (Files · Source finding · Test first · Verify · On failure), so a consumer can append it as a plan step without rewording it.
|
||||
|
||||
**Second consumer — the verdict gate (K7, 1.1):** `lib/review/verdict-gate.mjs <review.md>` reads the frontmatter `verdict` and exits 0 = ALLOW/WARN, 1 = BLOCK, 2 = could not decide. Exit 2 is never green: a missing file, a missing or unknown verdict, two verdict lines, a trailing JSON block that is not the last fence or lacks `verdict`/`counts`/`findings`, and a file whose JSON block, `## Findings (BLOCKER)` section or finding-id list contradicts a greener verdict all land there — and so does a WARN that exists because a reviewer or engine did not run (`missing-reviewer:` / `unattributable-payload` in the JSON block's `allow_blocked_by`, which a 1.1 review must carry): a review not performed in full stops a merge (operator decision 2026-10-06). Exactly one file per call. Whatever stops a merge or a release on a review reads this exit code, never a summary. Borrowed: the verdict goes to a file and a command gates on it (Kiro `feature-pipeline`); the executor reads the verdict but never writes it (Spec Kit).
|
||||
**Second consumer — the verdict gate (K7, 1.1):** `lib/review/verdict-gate.mjs <review.md>` reads the frontmatter `verdict` and exits 0 = ALLOW/WARN, 1 = BLOCK, 2 = could not decide. Exit 2 is never green: a missing file, a missing or unknown verdict, two verdict lines, a trailing JSON block that is not the last fence or lacks `verdict`/`counts`/`findings`, and a file whose JSON block, `## Findings (BLOCKER)` section or finding-id list contradicts a greener verdict all land there — and so does a WARN that exists because a reviewer or engine did not run (`missing-reviewer:` / `unattributable-payload` in the JSON block's `allow_blocked_by`, which a 1.1 review must carry): a review not performed in full stops a merge (operator decision 2026-10-06). Exactly one file per call. With `--repo <dir>` (`verdict-gate.mjs --json <review.md> --repo <dir>`, R6) an ALLOW/WARN review must also be of the commit being merged in `<dir>`: its `scope_sha_end` is HEAD, or an ancestor of HEAD whose later commits change only the review lessons (`.claude/rules/voyage-review-lessons.md`, inside its managed block) and the `CLAUDE.md` pointer (`lib/review/reviewed-commit.mjs`); otherwise exit 2 with a reason that starts `review-stale:`, which `/trekexecute`'s merge lock (Phase 7.2 step 4 and Phase 8, both with `--repo .`) maps to the decline reason `review-stale`. A `--repo` with no value is exit 2 as well; BLOCK stays exit 1. Whatever stops a merge or a release on a review reads this exit code, never a summary. Borrowed: the verdict goes to a file and a command gates on it (Kiro `feature-pipeline`); the executor reads the verdict but never writes it (Spec Kit).
|
||||
|
||||
**Path conventions:**
|
||||
- Project-dir mode (recommended): `{project_dir}/review.md` (one per review iteration; subsequent runs overwrite atomically).
|
||||
|
|
|
|||
2
tests/fixtures/jsonl-schemas.md
vendored
2
tests/fixtures/jsonl-schemas.md
vendored
|
|
@ -22,7 +22,7 @@
|
|||
| trekbrief-stats | ts, task, slug, mode, interview_turns, review_iterations, brief_quality, research_topics, auto_research, auto_result, intent_approved, project_dir, order_id, order_claimed_at, order_reason | commands/trekbrief.md (orchestrator-emit Phase 7; order_* added by lib/stats/stats-append.mjs) | trekbrief.md:1140-1164 | profile, phase_models, profile_source | none |
|
||||
| trekresearch-stats | ts, question, mode, scope, engine, slug, project_dir, brief_path, dimensions, dimensions_baseline, dimensions_baseline_preserved, effort, conv_turns, empty_turns, unique_sources, agents_local, agents_external, gemini_used, confidence, contradictions, open_questions | lib/stats/stats-append.mjs (trekresearch.md Stats tracking) | trekresearch.md:806-839 | profile, phase_models, parallel_agents, external_research_enabled, profile_source | none |
|
||||
| trekplan-stats | ts, task, mode, slug, brief_path, project_dir, codebase_size, codebase_files, agents_deployed, deep_dives, research_briefs_used, research_scout_used, critic_verdict, guardian_verdict, outcome | lib/stats/stats-append.mjs (trekplan.md Phase 12) | trekplan.md:1305-1333 | profile, phase_models, parallel_agents, profile_source | none |
|
||||
| trekexecute-stats (Phase 9 record) | ts, plan, plan_type, mode, result, steps_total, steps_passed, steps_failed, steps_skipped, failed_at_step | lib/stats/stats-append.mjs (trekexecute.md Phase 9) | trekexecute.md:2950-2972 | profile, phase_models, profile_source | none |
|
||||
| trekexecute-stats (Phase 9 record) | ts, plan, plan_type, mode, result, steps_total, steps_passed, steps_failed, steps_skipped, failed_at_step | lib/stats/stats-append.mjs (trekexecute.md Phase 9) | trekexecute.md:2962-2984 | profile, phase_models, profile_source | none |
|
||||
| trekexecute-stats (autonomy events) | ts, event, known_event, payload | lib/stats/event-emit.mjs `emit()` | event-emit.mjs:78-99 | payload.profile, payload.phase_models, payload.profile_source | none |
|
||||
| trekexecute-stats (PostToolUse Bash) | ts, session_id, command_excerpt, duration_ms, success | hooks/scripts/post-bash-stats.mjs (Bash PostToolUse) | post-bash-stats.mjs:42-54 | none (hook is plugin-level, not profile-aware) | command_excerpt (CWE-212) |
|
||||
| trekreview-stats | ts, slug, verdict, counts (BLOCKER/MAJOR/MINOR/SUGGESTION), reviewed_files_count, mode, duration_ms | lib/stats/stats-append.mjs (trekreview.md Phase 8) | trekreview.md:636-686 | profile, phase_models, profile_source | none |
|
||||
|
|
|
|||
|
|
@ -17,13 +17,14 @@
|
|||
|
||||
import { test } from 'node:test';
|
||||
import { strict as assert } from 'node:assert';
|
||||
import { copyFileSync, existsSync, mkdtempSync, readFileSync, readdirSync, rmSync } from 'node:fs';
|
||||
import { copyFileSync, existsSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, rmSync, writeFileSync } from 'node:fs';
|
||||
import { tmpdir } from 'node:os';
|
||||
import { dirname, join, resolve, sep } from 'node:path';
|
||||
import { fileURLToPath } from 'node:url';
|
||||
import { spawnSync } from 'node:child_process';
|
||||
import { findSection } from '../../lib/parsers/plan-schema.mjs';
|
||||
import { readFrontmatterFields } from '../../lib/skills/skill-frontmatter.mjs';
|
||||
import { childEnv } from '../helpers/child-env.mjs';
|
||||
|
||||
const ROOT = join(dirname(fileURLToPath(import.meta.url)), '..', '..');
|
||||
|
||||
|
|
@ -869,3 +870,131 @@ test('arch: lib/ never imports from scripts/, and the scan reports a planted imp
|
|||
const bad = files.flatMap((f) => importsIntoScripts(f, readFileSync(f, 'utf8')).map((s) => `${f.slice(ROOT.length + 1)}: ${s}`));
|
||||
assert.deepEqual(bad, [], 'lib/ imports from scripts/');
|
||||
});
|
||||
|
||||
// --- R6 ----------------------------------------------------------------------------
|
||||
// The merge lock (Phase 7.2 step 4 and the Phase 8 Main-merge gate) runs the
|
||||
// verdict gate with `--repo .`, so a review of an older commit than the one
|
||||
// being merged is refused (`review-stale`), and only the closing run's own
|
||||
// review opens the merge.
|
||||
|
||||
const MERGE_GATE = '### Main-merge gate (MAIN_MERGE_GATE — always pauses)';
|
||||
const PHASE_72 = '## Phase 7.2 — Simplify, review, verdict (P11)';
|
||||
const LOCK_LINE = 'node ${CLAUDE_PLUGIN_ROOT}/lib/review/verdict-gate.mjs --json "{review_path}" --repo .';
|
||||
const STEP4_LINE = 'node ${CLAUDE_PLUGIN_ROOT}/lib/review/verdict-gate.mjs --json "{project_dir}/review.md" --repo .';
|
||||
|
||||
/** A review_version 1.1 review.md of `sha`: ALLOW with no findings, or BLOCK over one BLOCKER. */
|
||||
function r6Review(verdict, sha) {
|
||||
const id = 'd'.repeat(40);
|
||||
const findings = verdict === 'BLOCK'
|
||||
? [{ id, severity: 'BLOCKER', rule_key: 'MISSING_TEST', file: 'a.mjs', line: 1, title: 't', detail: 'd', recommended_action: 'Add a test.' }]
|
||||
: [];
|
||||
const counts = { BLOCKER: findings.length, MAJOR: 0, MINOR: 0, SUGGESTION: 0 };
|
||||
const block = JSON.stringify({ verdict, counts, allow_blocked_by: [], findings }, null, 2);
|
||||
return [
|
||||
'---', 'type: trekreview', 'review_version: "1.1"', 'task: "t"', 'slug: s',
|
||||
'project_dir: .claude/projects/2026-10-23-s/', 'brief_path: .claude/projects/2026-10-23-s/brief.md',
|
||||
`scope_sha_end: ${sha}`, 'reviewed_files_count: 1', `verdict: ${verdict}`,
|
||||
findings.length ? `findings:\n - ${id}` : 'findings: []', '---', '', '# Review: t', '',
|
||||
'## Executive Summary', '', 'x', '', '## Coverage', '', '| File | Treatment | Reason |', '|---|---|---|',
|
||||
'| a.mjs | summary-only | default |', '', '## Remediation Summary', '', 'x', '', '```json', block, '```', '',
|
||||
].join('\n');
|
||||
}
|
||||
|
||||
/**
|
||||
* A throwaway repo (no git hooks): the reviewed commit, then a later commit
|
||||
* that changes source. `review(verdict, sha)` writes review.md outside the
|
||||
* repo and returns its folder.
|
||||
*/
|
||||
function staleRepo() {
|
||||
const base = mkdtempSync(join(tmpdir(), 'fix-round-r6-'));
|
||||
const repo = join(base, 'repo');
|
||||
const out = join(base, 'project');
|
||||
mkdirSync(repo);
|
||||
mkdirSync(out);
|
||||
mkdirSync(join(base, 'hooks-none'));
|
||||
const g = (...args) => {
|
||||
const r = spawnSync('git', args, { cwd: repo, encoding: 'utf8', env: childEnv() });
|
||||
assert.equal(r.status, 0, `git ${args.join(' ')}: ${r.stderr}`);
|
||||
return r.stdout.trim();
|
||||
};
|
||||
g('init', '-q', '-b', 'main');
|
||||
g('config', 'user.name', 'Test');
|
||||
g('config', 'user.email', 'test@example.invalid');
|
||||
g('config', 'commit.gpgsign', 'false');
|
||||
g('config', 'core.hooksPath', join(base, 'hooks-none'));
|
||||
writeFileSync(join(repo, 'a.mjs'), 'export const a = 1;\n');
|
||||
g('add', 'a.mjs');
|
||||
g('commit', '-q', '-m', 'feat: the reviewed work');
|
||||
const reviewed = g('rev-parse', 'HEAD');
|
||||
writeFileSync(join(repo, 'a.mjs'), 'export const a = 2;\n');
|
||||
g('add', 'a.mjs');
|
||||
g('commit', '-q', '-m', 'feat: a later source change');
|
||||
const head = g('rev-parse', 'HEAD');
|
||||
const review = (verdict, sha) => {
|
||||
writeFileSync(join(out, 'review.md'), r6Review(verdict, sha));
|
||||
return out;
|
||||
};
|
||||
return { repo, reviewed, head, review, cleanup: () => rmSync(base, { recursive: true, force: true }) };
|
||||
}
|
||||
|
||||
/** The lock line with its review path filled in, run in `repo` through bash(). */
|
||||
function runLock(cmd, dir, repo) {
|
||||
const filled = cmd.split('{review_path}').join(join(dir, 'review.md')).split('{project_dir}').join(dir);
|
||||
return bash(filled, repo);
|
||||
}
|
||||
|
||||
test('[R6] the Main-merge gate and Phase 7.2 step 4 each hold exactly one verdict-gate.mjs line, with --repo .', () => {
|
||||
const text = read(EXEC);
|
||||
assert.equal(line(section(text, MERGE_GATE), 'review/verdict-gate.mjs'), LOCK_LINE,
|
||||
'the Phase 8 lock does not run the verdict gate against the repo being merged');
|
||||
assert.equal(line(section(text, PHASE_72), 'review/verdict-gate.mjs'), STEP4_LINE,
|
||||
'Phase 7.2 step 4 does not run the verdict gate against the repo being merged');
|
||||
});
|
||||
|
||||
test('[R6] the extracted lock lines, run on an ALLOW review of an older commit with a later source commit, exit 2 with review-stale', () => {
|
||||
const text = read(EXEC);
|
||||
const lines = [line(section(text, MERGE_GATE), 'review/verdict-gate.mjs'), line(section(text, PHASE_72), 'review/verdict-gate.mjs')];
|
||||
const fx = staleRepo();
|
||||
try {
|
||||
for (const cmd of lines) {
|
||||
const ok = runLock(cmd, fx.review('ALLOW', fx.head), fx.repo);
|
||||
assert.equal(ok.status, 0, `control: an ALLOW review of HEAD opens the lock: ${ok.stdout}${ok.stderr}`);
|
||||
const r = runLock(cmd, fx.review('ALLOW', fx.reviewed), fx.repo);
|
||||
assert.equal(r.status, 2, `${cmd}: an ALLOW review of an older commit must exit 2: ${r.stdout}${r.stderr}`);
|
||||
const reason = JSON.parse(r.stdout).reason;
|
||||
assert.ok(reason.startsWith('review-stale'), `the reason must start review-stale: ${reason}`);
|
||||
}
|
||||
} finally { fx.cleanup(); }
|
||||
});
|
||||
|
||||
test('[R6] both decline lists name review-stale, the exit table says to re-run /trekreview on this commit, and --resume runs the same gate', () => {
|
||||
const gate = flat(section(read(EXEC), MERGE_GATE));
|
||||
const closed = gate.match(/\*\*Closed:\*\* emit `main-merge-declined` with the reason ([^.]*)\./);
|
||||
assert.ok(closed, 'the gate no longer says which reasons a closed lock emits');
|
||||
assert.ok(closed[1].includes('`review-stale`'), `the closed-lock reasons do not name review-stale: ${closed[1]}`);
|
||||
const literal = gate.match(/The reason is one of the literal words above \(([^)]*)\)/);
|
||||
assert.ok(literal, 'the gate no longer lists the literal decline reasons');
|
||||
assert.ok(literal[1].includes('`review-stale`'), `the literal decline reasons do not name review-stale: ${literal[1]}`);
|
||||
assert.match(gate, /a reason that starts `review-stale`[^.]*decline reason `review-stale`[^.]*re-run \/trekreview on this commit/,
|
||||
'the exit table does not map a review-stale reason to the decline reason and its "re-run /trekreview on this commit" line');
|
||||
assert.match(gate, /`--resume re-enters at the gate`[^.]*runs the same gate[^.]*review of an older commit stays refused/,
|
||||
'the --resume paragraph does not say the re-read runs the same gate');
|
||||
});
|
||||
|
||||
test('keep: Phase 7.2 step 4 run on a BLOCK review of an older commit still exits 1, so the fix loop opens', () => {
|
||||
const step4 = line(section(read(EXEC), PHASE_72), 'review/verdict-gate.mjs');
|
||||
const fx = staleRepo();
|
||||
try {
|
||||
const r = runLock(step4, fx.review('BLOCK', fx.reviewed), fx.repo);
|
||||
assert.equal(r.status, 1, `a BLOCK review must exit 1 before any staleness check: ${r.stdout}${r.stderr}`);
|
||||
assert.equal(JSON.parse(r.stdout).verdict, 'BLOCK');
|
||||
} finally { fx.cleanup(); }
|
||||
});
|
||||
|
||||
test('keep: the other verdict-gate callers carry no --repo', () => {
|
||||
for (const rel of ['commands/trekreview.md', 'agents/review-orchestrator.md', 'README.md', 'docs/release-runbook.md', 'tests/e2e-files-trial4.sh']) {
|
||||
const hits = read(rel).split('\n').filter((l) => /^\s*(?:out="\$\()?node \S*lib\/review\/verdict-gate\.mjs\b/.test(l));
|
||||
assert.ok(hits.length > 0, `${rel} no longer runs verdict-gate.mjs; the check would be empty`);
|
||||
for (const h of hits) assert.ok(!h.includes('--repo'), `${rel} gained --repo: ${h.trim()}`);
|
||||
}
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue