fix: the second lekerepo trial's faults that made a result wrong or misleading
voyage-lekerepo trial 2 (2026-10-08), report section A; the operator decided
they are fixed before the next release:
- A1: a /code-review finding whose category is simplification, efficiency,
style or readability enters as CODE_REVIEW_IMPROVEMENT (MINOR), not MAJOR.
- A2: recommended_action is the engine's own fix when transcribed, otherwise
a test-first step; never the failure scenario dressed as a remedy.
- A3: a /code-review finding outside the exact range it was given is declined
and listed, but no longer holds ALLOW back; /security-review's still does.
- A4: the manifest must_contain recipe uses grep -E -e ... -- so a pattern
like --sep is not read as an option.
- A5: indented sub-points belong to their criterion (counted), and a
criterion with sub-points and no command warns BRIEF_SC_SUBPOINTS.
- A6: a brief requiring bash 3.2 whose criterion runs plain bash warns
BRIEF_SC_BASH_NOT_PINNED; /trekbrief proposes /bin/bash.
- A7: the plan template's and /trekplan's checkpoints stage the step's files.
Tests fail on 5f9faee first (tests/lib/trial2-fixes.test.mjs, 11 of 14).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
8094b00861
commit
7c11c2aa9f
18 changed files with 300 additions and 39 deletions
|
|
@ -386,15 +386,22 @@ export function runContract(reviewerPayloads, options = {}) {
|
|||
const judged = judgeFilter(deduped);
|
||||
const reasoned = reasonablenessFilter(judged.kept);
|
||||
// Engine findings on files outside the review's SHA range (engine-findings.mjs
|
||||
// `out_of_scope`): real claims nobody here judged, so they forbid ALLOW.
|
||||
// `out_of_scope`). /security-review diffs against origin's default branch,
|
||||
// wider than this review: a real claim nobody here judged, so it forbids
|
||||
// ALLOW. /code-review was given exactly this range, so its citation outside
|
||||
// it is about code the review did not change: declined, listed, never
|
||||
// dropped — but it does not lock the verdict on WARN (voyage-lekerepo
|
||||
// trial 2, 2026-10-08: one remark on a frozen file held ALLOW back for good).
|
||||
const outOfScope = (options.engineOutOfScope ?? [])
|
||||
.map((f) => ({ ...f, suppressed_reason: 'engine:out-of-scope' }));
|
||||
const unverified = [...judged.unverified, ...reasoned.unverified, ...outOfScope];
|
||||
const outOfScopeBlocking = outOfScope.filter((f) => f.engine !== 'code-review');
|
||||
const outOfScopeNoted = outOfScope.filter((f) => f.engine === 'code-review');
|
||||
const unverified = [...judged.unverified, ...reasoned.unverified, ...outOfScopeBlocking];
|
||||
// What the coordinator set aside without ruling on it, one line each, so a
|
||||
// reader sees every finding that did not reach a verdict tier. Borrowed:
|
||||
// "Declined to judge … nothing you set aside is dropped silently"
|
||||
// (Superpowers `skills/requesting-code-review/code-reviewer.md:43-48`, MIT).
|
||||
const declined_to_judge = unverified.map((f) => ({
|
||||
const declined_to_judge = [...unverified, ...outOfScopeNoted].map((f) => ({
|
||||
file: f.file ?? null,
|
||||
line: f.line ?? null,
|
||||
rule_key: f.rule_key ?? null,
|
||||
|
|
|
|||
|
|
@ -14,13 +14,22 @@
|
|||
// { "engine": "code-review" | "security-review",
|
||||
// "findings": [ { "file": "...", "line": 12, "summary": "...",
|
||||
// "failure_scenario": "...", "category": "correctness",
|
||||
// "fix": "<optional: the fix the engine itself proposed>",
|
||||
// "rule_key": "<optional narrower catalogue key>" } ] }
|
||||
//
|
||||
// Output: { reviewer, findings: [...catalogue-conformant...], out_of_scope: [...] }
|
||||
// - rule_key: the given one when it is in the catalogue AND at least as
|
||||
// severe as the engine's own key, else CODE_REVIEW_DEFECT (MAJOR) /
|
||||
// SECURITY_REVIEW_VULNERABILITY (BLOCKER). A "narrower" key can never
|
||||
// lower an engine finding's tier.
|
||||
// lower an engine finding's tier. One exception, decided by the engine's
|
||||
// own category and not by the transcriber: a /code-review finding in an
|
||||
// IMPROVEMENT_CATEGORIES category is CODE_REVIEW_IMPROVEMENT (MINOR), since
|
||||
// /code-review reports no severity and a simplification proposal is not a
|
||||
// MAJOR defect (voyage-lekerepo trial 2, 2026-10-08).
|
||||
// - recommended_action: the engine's own `fix` when the transcription
|
||||
// carries one; otherwise a test-first step built from the failure
|
||||
// scenario. Never the scenario dressed as a remedy (trial 2: /trekplan
|
||||
// would have taken failure scenarios as step goals).
|
||||
// - File paths are made repo-relative (an absolute path under `repoRoot`
|
||||
// loses that prefix) before they are compared with the scope.
|
||||
// - A finding on a file outside the review scope is NOT dropped silently: it
|
||||
|
|
@ -69,6 +78,9 @@ function parseLine(v) {
|
|||
|
||||
const tier = (key) => SEVERITY_VALUES.indexOf(getRule(key).severity);
|
||||
|
||||
/** /code-review categories that name an improvement, not a defect. */
|
||||
export const IMPROVEMENT_CATEGORIES = Object.freeze(new Set(['simplification', 'efficiency', 'style', 'readability']));
|
||||
|
||||
/**
|
||||
* Convert one engine's transcribed findings into a coordinator payload.
|
||||
* @param {{ engine: string, findings: object[] }} input
|
||||
|
|
@ -96,8 +108,12 @@ export function toReviewerPayload(input, opts = {}) {
|
|||
const line = parseLine(f.line);
|
||||
const narrower = typeof f.rule_key === 'string' && RULE_KEYS.has(f.rule_key)
|
||||
&& tier(f.rule_key) <= tier(spec.rule_key);
|
||||
const rule_key = narrower ? f.rule_key : spec.rule_key;
|
||||
const improvement = input.engine === 'code-review'
|
||||
&& IMPROVEMENT_CATEGORIES.has(String(f.category ?? '').trim().toLowerCase());
|
||||
const engineKey = improvement ? 'CODE_REVIEW_IMPROVEMENT' : spec.rule_key;
|
||||
const rule_key = narrower ? f.rule_key : engineKey;
|
||||
const scenario = String(f.failure_scenario ?? '').trim();
|
||||
const fix = String(f.fix ?? '').trim();
|
||||
const loc = line > 0 ? `${file}:${line}` : file;
|
||||
const out = {
|
||||
id: computeFindingId(file, line, rule_key),
|
||||
|
|
@ -112,9 +128,10 @@ export function toReviewerPayload(input, opts = {}) {
|
|||
// An action, not the title again: `Resolve: <title>` read as "restating
|
||||
// the title" to the coordinator's Actionability filter, and 8 of 8
|
||||
// engine findings were set aside (voyage-lekerepo trial, 2026-10-06).
|
||||
recommended_action: scenario
|
||||
? `Fix ${loc} so this no longer happens: ${scenario}`
|
||||
: `Fix the defect at ${loc} that ${input.engine === 'security-review' ? '/security-review' : '/code-review'} reported`,
|
||||
recommended_action: fix
|
||||
|| (scenario
|
||||
? `Write a test that reproduces this at ${loc} (${scenario}), then change ${loc} until it passes`
|
||||
: `Write a test that shows the defect at ${loc} that ${input.engine === 'security-review' ? '/security-review' : '/code-review'} reported, then change ${loc} until it passes`),
|
||||
engine: input.engine,
|
||||
};
|
||||
if (scope && !scope.has(file)) out_of_scope.push(out);
|
||||
|
|
|
|||
|
|
@ -1,19 +1,23 @@
|
|||
// lib/review/rule-catalogue.mjs
|
||||
// Canonical rule catalogue for /trekreview.
|
||||
//
|
||||
// 14 rule keys, 4-tier severity (matches brief contract).
|
||||
// 15 rule keys, 4-tier severity (matches brief contract).
|
||||
// llm-security 5-tier alignment is a v1.1 candidate.
|
||||
//
|
||||
// Ordre 5 (review_version 1.1) added the two engine keys: findings from Claude
|
||||
// Code's own `/code-review` and `/security-review` enter the coordinator under
|
||||
// them when no narrower key fits (wrap, not a new engine).
|
||||
// them when no narrower key fits (wrap, not a new engine). A third engine key,
|
||||
// CODE_REVIEW_IMPROVEMENT (MINOR), takes a /code-review finding whose category
|
||||
// is an improvement rather than a defect (simplification, efficiency, style,
|
||||
// readability): the second voyage-lekerepo trial (2026-10-08) saw a pure
|
||||
// simplification proposal enter as MAJOR and drive the verdict.
|
||||
//
|
||||
// `memory_route` decides where a finding class that recurs across two
|
||||
// review.md files is proposed as project memory (P5, lib/review/recurrence.mjs):
|
||||
// 'check' - mechanical: a deterministic check can catch it;
|
||||
// 'rule' - judgement: a line in the target repo's `.claude/rules/` file;
|
||||
// null - never proposed: review hygiene (about the review itself, not the
|
||||
// target code), or a catch-all key (the two engine keys) whose
|
||||
// target code), or a catch-all key (the three engine keys) whose
|
||||
// members share nothing but their source.
|
||||
// Routing borrowed from Pocock `skills/in-progress/retro/SKILL.md:19` (MIT).
|
||||
|
||||
|
|
@ -120,6 +124,13 @@ export const RULE_CATALOGUE = Object.freeze([
|
|||
description: "A defect reported by Claude Code's /code-review that no narrower rule_key covers.",
|
||||
memory_route: null,
|
||||
}),
|
||||
Object.freeze({
|
||||
rule_key: 'CODE_REVIEW_IMPROVEMENT',
|
||||
severity: 'MINOR',
|
||||
category: 'maintenance',
|
||||
description: "An improvement (simplification, efficiency, style, readability) reported by Claude Code's /code-review, not a defect.",
|
||||
memory_route: null,
|
||||
}),
|
||||
Object.freeze({
|
||||
rule_key: 'SECURITY_REVIEW_VULNERABILITY',
|
||||
severity: 'BLOCKER',
|
||||
|
|
|
|||
|
|
@ -270,8 +270,28 @@ export function validateBriefContent(text, opts = {}) {
|
|||
// judges by reading, and that stays allowed. Prose-only criteria (no
|
||||
// command) are not flagged.
|
||||
if (fm.type !== 'trekreview') {
|
||||
// bash 3.2 required but measured under whatever `bash` is first on PATH:
|
||||
// on a Mac with a newer bash installed that is 5.x, so the constraint is
|
||||
// never measured (voyage-lekerepo trial 2, 2026-10-08).
|
||||
const constraints = (extractSection(body, 'Constraints') ?? '') + (extractSection(body, 'Non-Functional Requirements') ?? '');
|
||||
const needsBash32 = /\bbash\s*3\.2\b/i.test(constraints);
|
||||
for (const c of parseSuccessCriteria(body)) {
|
||||
if (c.subpoints && !c.command) {
|
||||
warnings.push(issue(
|
||||
'BRIEF_SC_SUBPOINTS',
|
||||
`Success criterion ${c.label} has ${c.subpoints} sub-point(s) and no command of its own: the review measures none of them`,
|
||||
'Give the criterion one command that checks all its sub-points (`bash tests/<script>.sh` exits 0), ' +
|
||||
'or write each sub-point as its own criterion with its own command.',
|
||||
));
|
||||
}
|
||||
if (!c.command) continue;
|
||||
if (needsBash32 && /^bash\s/.test(c.command)) {
|
||||
warnings.push(issue(
|
||||
'BRIEF_SC_BASH_NOT_PINNED',
|
||||
`Success criterion ${c.label} runs \`${c.command}\`, but the brief requires bash 3.2: \`bash\` is whichever bash comes first on PATH, which may be newer`,
|
||||
`Write it as \`/bin/bash ${c.command.replace(/^bash\s+/, '')}\` (/bin/bash tests/<script>.sh) so the check runs under the system bash.`,
|
||||
));
|
||||
}
|
||||
const r = runnableCheck(c, { cwd: opts.cwd });
|
||||
if (r.runnable) continue;
|
||||
warnings.push(issue(
|
||||
|
|
|
|||
|
|
@ -164,6 +164,12 @@ function parseSection(markdown, heading, prefix) {
|
|||
const criteria = [];
|
||||
const push = (text, command, reason) =>
|
||||
criteria.push({ label: `${prefix}${criteria.length + 1}`, text, command, reason });
|
||||
// A bullet indented deeper than the section's first bullet is a sub-point of
|
||||
// the criterion above it, never a criterion of its own: one criterion with
|
||||
// ten sub-points became SC4..SC13, each `not-a-command` (voyage-lekerepo
|
||||
// trial 2, 2026-10-08). The parent counts them in `subpoints`.
|
||||
let baseIndent = null;
|
||||
let lastBullet = null;
|
||||
for (const line of lines) {
|
||||
if (line.marker) continue;
|
||||
if (line.fenced) {
|
||||
|
|
@ -175,10 +181,17 @@ function parseSection(markdown, heading, prefix) {
|
|||
}
|
||||
const bullet = line.text.match(BULLET);
|
||||
if (!bullet) continue;
|
||||
const indent = line.text.match(/^\s*/)[0].replace(/\t/g, ' ').length;
|
||||
if (baseIndent === null) baseIndent = indent;
|
||||
if (indent > baseIndent && lastBullet) {
|
||||
lastBullet.subpoints = (lastBullet.subpoints ?? 0) + 1;
|
||||
continue;
|
||||
}
|
||||
const text = bullet[1].trim();
|
||||
if (text === '') continue;
|
||||
const { command, reason } = firstCommand(text);
|
||||
push(text, command, reason);
|
||||
lastBullet = criteria[criteria.length - 1];
|
||||
}
|
||||
return criteria;
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue