diff --git a/.gitignore b/.gitignore index b3c2fcb4..7570c5d6 100644 --- a/.gitignore +++ b/.gitignore @@ -6,3 +6,4 @@ node_modules/ .stryker-tmp/ reports/mutation/ stryker-setup-*.js +.vitest/ diff --git a/README.md b/README.md index 46e1ea4c..1498d285 100644 --- a/README.md +++ b/README.md @@ -138,7 +138,10 @@ a final step synthesizes one severity-ranked report. `workflows/triage-findings.js` gathers findings (craft agents + GitHub PR comments), sifts out what the project's memory already settled, validates the rest, dedups and conflict-checks them, and renders one ordered fix plan + a triage ledger (and the issues to file for findings the PR did not -introduce) — no edits. +introduce) — no edits. A finding counts as not introduced by the PR only when it was shown at the +commit the PR forks from: pass that as `prBase` (`git merge-base origin/ `); +without it such findings come back as `needs-decision`. `base` is the separate ref the findings were +generated against, which validation pins to. Both review workflows and the standalone agents emit a structured run record after each run; see `docs/observability.md`. diff --git a/lib/pr-rejections.mjs b/lib/pr-rejections.mjs index 3c321732..45da043b 100644 --- a/lib/pr-rejections.mjs +++ b/lib/pr-rejections.mjs @@ -6,7 +6,7 @@ // craft finding's thread, lib/pr-reviewer-thread.mjs for any other reviewer's (realm @nick/craft, node #302). import fs from 'node:fs' import { threadDecision, NOT_AUTHORIZED, NOT_CRAFT_ROOT } from './pr-thread-rule.mjs' -import { reviewerThreadDecision, SELF_DISMISSED } from './pr-reviewer-thread.mjs' +import { reviewerThreadDecision, NOT_A_MAINTAINER } from './pr-reviewer-thread.mjs' import { outputLines, runIfMain } from './script-main.mjs' /** @typedef {import('./pr-thread-rule.mjs').ReviewThread} ReviewThread @typedef {import('./pr-thread-rule.mjs').DecisionRecord} DecisionRecord */ @@ -70,8 +70,8 @@ export function rejectionsFromThreads(answer) { if (forged) skipped.push(`${forged} thread(s) not read as craft findings: the marked root comment's author is not OWNER/MEMBER/COLLABORATOR`) const unauthorized = skipped.filter(s => s.endsWith(NOT_AUTHORIZED)).length if (unauthorized) skipped.push(`${unauthorized} rejection(s) not recorded: the replier is neither the PR author nor OWNER/MEMBER/COLLABORATOR`) - const selfDismissed = skipped.filter(s => s.endsWith(SELF_DISMISSED)).length - if (selfDismissed) skipped.push(`${selfDismissed} rejection(s) not recorded: an outside reviewer rejected their own remark`) + const notMaintainer = skipped.filter(s => s.endsWith(NOT_A_MAINTAINER)).length + if (notMaintainer) skipped.push(`${notMaintainer} rejection(s) of another reviewer's remark not recorded: the PR author is not OWNER/MEMBER/COLLABORATOR`) const unread = Math.max(threads.total, threads.nodes.length) - read.length if (unread > 0) skipped.push(`${unread} thread(s) past the ${PR_THREADS_MAX} read were not examined — nothing recorded from them`) return { decisions, skipped } diff --git a/lib/pr-reviewer-thread.mjs b/lib/pr-reviewer-thread.mjs index 024edac7..fcbd7502 100644 --- a/lib/pr-reviewer-thread.mjs +++ b/lib/pr-reviewer-thread.mjs @@ -1,13 +1,14 @@ // A rejection in the thread of a reviewer that is not craft (realm @nick/craft, node #302): a person or // their own tool comments on the PR, the comment carries no craft-finding marker, and the last word is a -// reply that rejects it, from the PR author or an OWNER/MEMBER/COLLABORATOR — the same reply rule as a -// craft thread (lib/pr-thread-rule.mjs, nodes #182, #197). It becomes a decision record without asking +// reply that rejects it, from an OWNER/MEMBER/COLLABORATOR. It becomes a decision record without asking // (node #223), so the same remark in the next PR is set aside by triage's sift (node #305). The root may -// come from anyone; one guard more than a craft thread needs: an outsider who rejects their own remark -// has settled nothing, or a contributor could post a remark and its rejection to silence a finding for -// the whole project (the reason a forged craft root is refused, node #186). No reply is no rejection -// (node #185). Read across a whole PR by lib/pr-rejections.mjs, after lib/pr-thread-rule.mjs says the -// thread is not craft's. +// come from anyone, so the reply rule is narrower than a craft thread's (lib/pr-thread-rule.mjs, nodes +// #182, #197): there the PR author's word counts, here only a maintainer's. Logins prove nothing — an +// outside PR author with a second account could post a remark under one and reject it under the other, +// silencing a finding for the whole project (the reason a forged craft root is refused, node #186) — so +// a PR author without that association never settles another reviewer's remark. No reply is no +// rejection (node #185). Read across a whole PR by lib/pr-rejections.mjs, after lib/pr-thread-rule.mjs +// says the thread is not craft's. import { REJECTING_ASSOCIATIONS, THREAD_COMMENTS_MAX, decisionId, recordProblem, threadAnchor, threadRejection } from './pr-thread-rule.mjs' /** @@ -18,8 +19,9 @@ import { REJECTING_ASSOCIATIONS, THREAD_COMMENTS_MAX, decisionId, recordProblem, // A reviewer's remark has no title line of its own: its first line, cut at a word, stands in for one. export const REMARK_TITLE_MAX = 120 -// The tail of the skip line for an outsider rejecting their own remark; lib/pr-rejections.mjs counts by it. -export const SELF_DISMISSED = 'who also wrote the remark and is not an owner, member or collaborator: not recorded' +// The tail of the skip line for a PR author with no maintainer association rejecting another reviewer's +// remark; lib/pr-rejections.mjs counts by it. +export const NOT_A_MAINTAINER = "the PR author but not an owner, member or collaborator: another reviewer's remark is not theirs to settle, not recorded" /** @param {unknown} v */ const str = v => String(v ?? '').trim() @@ -56,13 +58,14 @@ export function remarkThread(t) { const remarkName = (scope, title, reviewer) => `${scope || '?'}: ${title || '(remark without text)'} (reviewer ${reviewer || '?'})` /** - * Why a rejection cannot settle the remark, '' when it can: an outsider rejecting their own remark, or a - * remark with no text to title the decision. + * Why a rejection cannot settle the remark, '' when it can: a rejecter who is not an owner, member or + * collaborator (the PR author, whose word counts in a craft thread only), or a remark with no text to + * title the decision. * @param {NonNullable>} c @param {string} by @returns {string} */ function unsettled(c, by) { const assoc = str(c.cs[c.cs.length - 1]?.authorAssociation).toUpperCase() - if (c.reviewer && c.reviewer === by && !REJECTING_ASSOCIATIONS.includes(assoc)) return `rejected by ${by}, ${SELF_DISMISSED}` + if (!REJECTING_ASSOCIATIONS.includes(assoc)) return `rejected by ${by || '?'} (association ${assoc || 'unknown'}), ${NOT_A_MAINTAINER}` return c.title ? '' : 'the remark has no text to title the decision: record it by hand' } diff --git a/lib/pr-reviewer-thread.test.mjs b/lib/pr-reviewer-thread.test.mjs index 8c099bcc..402341c3 100644 --- a/lib/pr-reviewer-thread.test.mjs +++ b/lib/pr-reviewer-thread.test.mjs @@ -1,6 +1,6 @@ // A rejection in another reviewer's thread (realm @nick/craft, node #302): the root carries no craft marker -// and may come from anyone; the last word rejects it, from the PR author or an owner, member or -// collaborator; it is recorded like a craft thread's (lib/pr-thread-rule.mjs), and both kinds of thread +// and may come from anyone; the last word rejects it, from an owner, member or collaborator — the PR +// author only when they are one; it is recorded like a craft thread's (lib/pr-thread-rule.mjs), and both kinds of thread // read side by side across one PR (lib/pr-rejections.mjs). Fields as `gh api graphql` returns them. import { test } from 'vitest' import assert from 'node:assert/strict' @@ -20,8 +20,8 @@ const answer = (nodes, prAuthor = 'carol') => ({ data: { repository: { pullReque /** @param {unknown} a @returns {{ decisions: any[], skipped: string[] }} */ const read = a => /** @type {any} */ (rejectionsFromThreads(a)) -test('another reviewer\'s remark rejected by the PR author is a decision, titled by its first line, anchored at its line', () => { - const r = read(answer([thread([REMARK, comment('carol', 'CONTRIBUTOR', 'By design: the caller caps it.')])])) +test('another reviewer\'s remark rejected by a PR author who maintains the repo is a decision, titled by its first line, anchored at its line', () => { + const r = read(answer([thread([REMARK, comment('carol', 'OWNER', 'By design: the caller caps it.')])])) assert.deepEqual(r.decisions, [{ id: decisionId('FR-12: retry loop has no upper bound', 'src/retry.rs'), kind: 'decision', title: 'FR-12: retry loop has no upper bound', body: 'By design: the caller caps it.', scope: 'src/retry.rs', status: 'active', date: '2026-10-02', author: 'carol', commit: SHA, @@ -51,18 +51,25 @@ test('conservative: no reply, a fixed reply, a later word, a drive-by rejecter, assert.match(driveBy.skipped.join('\n'), /1 rejection\(s\) not recorded: the replier is neither the PR author/) const self = none([comment('carol', 'CONTRIBUTOR', 'unbounded retry'), comment('carol', 'CONTRIBUTOR', 'Not a bug.')]) assert.equal(self.decisions.length, 0) - assert.match(self.skipped.join('\n'), /rejected by carol, who also wrote the remark and is not an owner, member or collaborator: not recorded/) - assert.match(self.skipped.join('\n'), /1 rejection\(s\) not recorded: an outside reviewer rejected their own remark/) + assert.match(self.skipped.join('\n'), /rejected by carol \(association CONTRIBUTOR\), the PR author but not an owner, member or collaborator: another reviewer's remark is not theirs to settle, not recorded/) + assert.match(self.skipped.join('\n'), /1 rejection\(s\) of another reviewer's remark not recorded: the PR author is not OWNER\/MEMBER\/COLLABORATOR/) + // A second account (node #302): the remark from one login, the PR author's rejection from another. + const second = none([REMARK, comment('carol', 'CONTRIBUTOR', 'Not a bug.')]) + assert.equal(second.decisions.length, 0, 'an outside PR author cannot settle another reviewer\'s remark for the project') + assert.match(second.skipped.join('\n'), /\(reviewer snitch-bot\) — rejected by carol \(association CONTRIBUTOR\), the PR author but not/) + for (const assoc of ['NONE', 'FIRST_TIME_CONTRIBUTOR', 'FIRST_TIMER', '']) { + assert.equal(none([REMARK, comment('carol', assoc, 'Not a bug.')]).decisions.length, 0, assoc) + } const owner = none([comment('nick', 'OWNER', 'unbounded retry'), comment('nick', 'OWNER', 'Not a bug.')], 'nick') assert.equal(owner.decisions.length, 1, 'a maintainer may settle their own remark') }) test('bounds and anchors: past the comments read, no text, no commit — named, not recorded', () => { - const many = reviewerThreadDecision(thread([REMARK, comment('carol', 'CONTRIBUTOR', 'Not a bug.')], { comments: { totalCount: 60, nodes: [REMARK, comment('carol', 'CONTRIBUTOR', 'Not a bug.')] } }), 'carol') + const many = reviewerThreadDecision(thread([REMARK, comment('carol', 'OWNER', 'Not a bug.')], { comments: { totalCount: 60, nodes: [REMARK, comment('carol', 'OWNER', 'Not a bug.')] } }), 'carol') assert.match(String(many), /60 comments, past the 50 read/) - const blank = reviewerThreadDecision(thread([comment('x', 'NONE', ' \n'), comment('carol', 'CONTRIBUTOR', 'Not a bug.')]), 'carol') + const blank = reviewerThreadDecision(thread([comment('x', 'NONE', ' \n'), comment('carol', 'OWNER', 'Not a bug.')]), 'carol') assert.match(String(blank), /\(remark without text\) .* no text to title the decision/) - const noCommit = reviewerThreadDecision(thread([{ ...REMARK, originalCommit: null }, comment('carol', 'CONTRIBUTOR', 'Not a bug.')]), 'carol') + const noCommit = reviewerThreadDecision(thread([{ ...REMARK, originalCommit: null }, comment('carol', 'OWNER', 'Not a bug.')]), 'carol') assert.match(String(noCommit), /no path or commit to anchor the decision to/) assert.equal(reviewerThreadDecision({ path: 'a' }, 'carol'), null) }) @@ -81,7 +88,7 @@ test('both kinds of thread on one PR: a craft finding and another reviewer\'s re const craftRoot = comment('nick', 'OWNER', findingCommentBody({ severity: 'Medium', title: 'unwrap in parser may panic', why: 'panics', fix: 'return an error' }), 'https://x/pull/9#c1') const r = read(answer([ thread([craftRoot, comment('carol', 'CONTRIBUTOR', 'Not a bug: validated upstream.')], { path: 'src/parse.rs' }), - thread([REMARK, comment('carol', 'CONTRIBUTOR', "Won't fix: capped by the caller.")]), + thread([REMARK, comment('alice', 'MEMBER', "Won't fix: capped by the caller.")]), ])) assert.deepEqual(r.decisions.map(d => [d.scope, d.title]), [['src/parse.rs', 'unwrap in parser may panic'], ['src/retry.rs', 'FR-12: retry loop has no upper bound']]) }) diff --git a/lib/triage-noise-class.mjs b/lib/triage-noise-class.mjs index 472db2e5..020946c9 100644 --- a/lib/triage-noise-class.mjs +++ b/lib/triage-noise-class.mjs @@ -1,11 +1,14 @@ // The validator's noise classes in triage-findings (realm @nick/craft, node #303), read back from its // answer: "not this PR's" (class 1) on a real finding is a deferral carried by an issue on the consumer's -// forge, and only when premise_checked names what was opened at the base — a merge-base, or a -// `git show :`; else a human decides. Only an `accept` or a `defer` turns into that deferral: a -// class-1 `reject` is not a real problem and files nothing, a class-1 `needs-decision` still waits on a human. +// forge, and only when premise_checked names the code opened at the PR's fork point the triage was given +// as `prBase` — `git show :`, that commit and no other ref (node #309). Not the validation +// pin `base`: that is the ref the findings were generated against, usually the reviewed head, the PR's +// own code. A premise naming HEAD, the pin, another ref or only the word merge-base has shown nothing, +// and with no prBase given nothing can be shown; else a human decides. Only an `accept` or a `defer` +// turns into that deferral: a class-1 `reject` is not a real problem and files nothing, a class-1 +// `needs-decision` still waits on a human. // "Not decidable from the code" (class 5) is a needs-decision. // Inlined into src/triage-findings.js. - /** @typedef {{ severity: string, title: string, location: string, detail: string, source: string, stable_id: string }} IssueFinding */ /** @@ -17,27 +20,68 @@ export function noiseClassOf(v) { return Number.isInteger(n) && n >= 1 && n <= 6 ? n : 0 } +/** + * What the validator is told about where class 1 must be shown: the PR's fork point this triage was + * given, cited exactly as `classifiedVerdict` reads it, so the engine can tell the premise opened that + * commit rather than a ref the validator picked (node #309); with none, that a human decides. + * @param {string} prBase @returns {string} + */ +export function classOneBaseLine(prBase) { + const at = String(prBase ?? '').trim() + return at + ? `the PR forks from \`${at}\` (prBase) — open the cited code there and record in premise_checked exactly \`git show ${at}:\`; a premise naming any other ref (HEAD, the ref you validate against, a branch, a merge-base you computed) reads as unchecked and goes to a human` + : "no prBase (the PR's fork point) was given to this triage, so it cannot be checked here — name the class all the same; a human decides it" +} + +/** + * Whether a premise shows a file opened at exactly `ref`: `git show :`, at the start, after + * a space, or inside backticks, quotes or parentheses; the ref ends at the colon, so neither another + * ref nor one that only starts with this one passes. + * @param {string} premise @param {string} ref @returns {boolean} + */ +export function openedAt(premise, ref) { + const needle = `git show ${ref}:` + for (let i = premise.indexOf(needle); i !== -1; i = premise.indexOf(needle, i + 1)) { + const before = i === 0 ? ' ' : premise.charAt(i - 1) + const after = premise.charAt(i + needle.length) + if (/[\s`'"(]/.test(before) && after !== '' && !/[\s:`'")]/.test(after)) return true + } + return false +} + /** * The verdict a noise class settles: class 1 (not this PR's) on a finding the validator accepted or * deferred is a deferral carried by an issue on the consumer's forge — but only when premise_checked - * names the base it opened, else nothing settled it and a human decides; any other class-1 verdict - * stands (a `reject`: no real problem, no issue; a `needs-decision`: still the human's). Class 5 (not - * decidable from the code) is needs-decision. Every other verdict stands as the validator gave it. + * shows a file opened at `prBase`, the PR's fork point the triage was given, never a ref the validator + * picked — else nothing settled it and a human decides; with no prBase, always a human. Any other + * class-1 verdict stands (a `reject`: no real problem, no issue; a `needs-decision`: still the human's). + * Class 5 (not decidable from the code) is needs-decision. Every other verdict stands as given. * @template {{ verdict: string, reason: string, premise_checked: string }} V @param {V} v + * @param {string} [prBase] the commit the PR forks from; '' or absent when none was given * @returns {V & { action?: string }} */ -export function classifiedVerdict(v) { +export function classifiedVerdict(v, prBase = '') { const n = noiseClassOf(v) - if (n === 1 && v.verdict !== 'accept' && v.verdict !== 'defer') return v - // "Checked at the base" is what premise_checked shows: the merge-base taken, or a file read at a ref. - if (n === 1 && !/\bmerge-base\b|\bgit show [^\s:]+:\S/.test(String(v.premise_checked ?? ''))) { - return { ...v, verdict: 'needs-decision', reason: `claimed not this PR's, but nothing at the base was opened to show it — ${v.reason}` } - } - if (n === 1) return { ...v, verdict: 'defer', action: 'issue' } + if (n === 1) return notThisPrs(v, String(prBase ?? '').trim()) if (n === 5) return { ...v, verdict: 'needs-decision' } return v } +/** + * A class-1 verdict settled: a real problem shown at `prBase` is a deferral with an issue, one not shown + * there a human's; a `reject` or `needs-decision` stands. + * @template {{ verdict: string, reason: string, premise_checked: string }} V @param {V} v @param {string} prBase + * @returns {V & { action?: string }} + */ +export function notThisPrs(v, prBase) { + if (v.verdict !== 'accept' && v.verdict !== 'defer') return v + if (!prBase) return { ...v, verdict: 'needs-decision', reason: `claimed not this PR's, but no prBase (the PR's fork point) was given to check it at — ${v.reason}` } + if (!openedAt(String(v.premise_checked ?? ''), prBase)) { + return { ...v, verdict: 'needs-decision', reason: `claimed not this PR's, but nothing at the PR's fork point ${prBase} was opened to show it — ${v.reason}` } + } + return { ...v, verdict: 'defer', action: 'issue' } +} + /** * The issue a class-1 finding becomes: its title, and a body naming where it stands, why it is not this * PR's, and the source's own words. diff --git a/lib/triage-sift-engine.test.mjs b/lib/triage-sift-engine.test.mjs index b224c6cc..dfbd4e21 100644 --- a/lib/triage-sift-engine.test.mjs +++ b/lib/triage-sift-engine.test.mjs @@ -97,22 +97,39 @@ test('passed priorDecisions are used even when the recall finds nothing', async assert.equal(validated(without).length, 1, 'without the passed record nothing sets the finding aside') }) -test('a finding the validator shows already at the base is deferred with an issue action; unchecked, a human decides', async () => { +test('a finding the validator shows already at the PR\'s fork point is deferred with an issue action; unchecked, a human decides', async () => { const f = finding('Medium', 'unbounded queue growth', 'lib/q.rs:3') - const pre = { validate: validation(idOf(f), { verdict: 'defer', reason: 'stands at main', noise_class: 1, premise_checked: 'git show abc:lib/q.rs:3' }) } - const r = await run([f], pre) + // prBase is where the PR forks from; base is only the validation pin (usually the reviewed head). + const atBase = { prBase: 'abc1234', base: 'feed999' } + const pre = { validate: validation(idOf(f), { verdict: 'defer', reason: 'stands at main', noise_class: 1, premise_checked: 'git show abc1234:lib/q.rs:3' }) } + const r = await run([f], pre, atBase) const e = row(r, idOf(f)) assert.deepEqual([e.verdict, e.action], ['defer', 'issue']) assert.equal(r.reportValue.issues.length, 1) assert.equal(r.reportValue.issues[0].title, 'unbounded queue growth') - assert.match(r.reportValue.issues[0].body, /stands at main \(checked at git show abc:lib\/q\.rs:3\)/) + assert.match(r.reportValue.issues[0].body, /stands at main \(checked at git show abc1234:lib\/q\.rs:3\)/) assert.equal(filedRecord(r).triage.issues, 1) assert.match(validated(r)[0].prompt, /Check it AT THE BASE, not by the reviewer's word/) + assert.match(validated(r)[0].prompt, /`git show abc1234:`/, 'the validator is told the exact fork point to cite') + assert.match(validated(r)[0].prompt, /Validate against ref `feed999`/, 'base keeps its meaning as the validation pin') + for (const premise of ['`git show abc1234:lib/q.rs`', 'read it (git show abc1234:lib/q.rs)']) { + const quoted = await run([f], { validate: validation(idOf(f), { verdict: 'defer', reason: 'stands at main', noise_class: 1, premise_checked: premise }) }, atBase) + assert.deepEqual([row(quoted, idOf(f)).verdict, row(quoted, idOf(f)).action], ['defer', 'issue'], `the prompt's own form, quoted: ${premise}`) + } - const unchecked = await run([f], { validate: validation(idOf(f), { verdict: 'defer', reason: 'pre-existing', noise_class: 1, premise_checked: '' }) }) - assert.equal(row(unchecked, idOf(f)).verdict, 'needs-decision') - assert.equal(row(unchecked, idOf(f)).action, undefined) - assert.equal(unchecked.reportValue.issues, undefined) + // A ref the validator picked itself (node #309): HEAD, or the pin, is the PR's own code, and shows nothing. + for (const premise of ['', 'git show HEAD:lib/q.rs', 'git show main:lib/q.rs', 'git merge-base main HEAD', 'git show feed999:lib/q.rs']) { + const unchecked = await run([f], { validate: validation(idOf(f), { verdict: 'defer', reason: 'pre-existing', noise_class: 1, premise_checked: premise }) }, atBase) + assert.equal(row(unchecked, idOf(f)).verdict, 'needs-decision', premise) + assert.equal(row(unchecked, idOf(f)).action, undefined) + assert.equal(unchecked.reportValue.issues, undefined) + } + for (const args of [{}, { base: 'abc1234' }]) { + const noBase = await run([f], pre, args) + assert.equal(row(noBase, idOf(f)).verdict, 'needs-decision', `no prBase given (${JSON.stringify(args)}): class 1 is never checked, the pin does not stand in`) + assert.equal(noBase.reportValue.issues, undefined) + assert.match(validated(noBase)[0].prompt, /no prBase \(the PR's fork point\) was given/) + } const notReal = await run([f], { validate: validation(idOf(f), { verdict: 'reject', reason: 'no growth: bounded by cap', noise_class: 1, premise_checked: 'git show abc:lib/q.rs' }) }) assert.deepEqual([row(notReal, idOf(f)).verdict, row(notReal, idOf(f)).action], ['reject', undefined]) @@ -134,6 +151,15 @@ test('a repeat in the batch collapses onto the most severe copy, which alone is assert.equal(filedRecord(r).triage.sifted.repeats, 1) }) +test('a repeat row names its twin by a field, so the review ledger takes the twin\'s disposition, not the repeat\'s reject (node #312)', async () => { + const low = finding('Low', 'unbounded queue growth here', 'lib/q.rs:3') + const high = finding('High', 'queue growth unbounded here', 'lib/q.rs:5') + const r = await run([low, high]) + assert.equal(row(r, idOf(low)).repeat_of, idOf(high), 'the plan agent drops the field; the engine restores it') + assert.equal(row(r, idOf(high)).repeat_of, undefined) + const forgot = await run([low, high], { plan: () => ({ plan_markdown: 'BODY', summary: 'S', ledger: [] }) }) + assert.equal(row(forgot, idOf(low)).repeat_of, idOf(high), 'a row the plan dropped comes back with it')}) + test('a re-run never carries a sifted row: a finding the sift now raises again, and a former repeat, reach a validator', async () => { const f = finding('Medium') const first = await run([f]) diff --git a/lib/triage-sift.mjs b/lib/triage-sift.mjs index d0d72df1..3566d9e1 100644 --- a/lib/triage-sift.mjs +++ b/lib/triage-sift.mjs @@ -17,7 +17,7 @@ import { contradictionOf } from './finding-sort.mjs' * @typedef {import('./prior-decisions.mjs').PriorDecision} PriorDecision * @typedef {{ severity: string, title: string, location: string, detail: string, proposed_fix: string, thread_id: string, source: string }} GatheredFinding * @typedef {GatheredFinding & { stable_id: string, file: string, line: number, why: string, memoryNote: string }} SiftFinding - * @typedef {{ stable_id: string, verdict: string, reason: string, sifted: true, action?: string }} SiftEntry + * @typedef {{ stable_id: string, verdict: string, reason: string, sifted: true, action?: string, repeat_of?: string }} SiftEntry */ // How much two titles at one site must share to be one finding said twice. @@ -95,9 +95,15 @@ export function collapseRepeats(findings) { return { kept, repeated } } -/** The ledger row of a repeat. @param {{ f: SiftFinding, of: string }} r @returns {SiftEntry} */ +/** + * The ledger row of a repeat: `reject` here, since its twin alone is judged, and `repeat_of` naming that + * twin. The verdict is no disposition of its own: in the review ledger a repeat takes its twin's (closed, + * deferred, rejected), or a fix that missed the repeat's line would carry a real defect as dismissed + * (node #312). + * @param {{ f: SiftFinding, of: string }} r @returns {SiftEntry} + */ export function repeatEntry(r) { - return { stable_id: r.f.stable_id, verdict: 'reject', sifted: true, reason: `${SIFT_MARK} — repeats ${r.of} in this batch (same site, same title words); judged there` } + return { stable_id: r.f.stable_id, verdict: 'reject', sifted: true, repeat_of: r.of, reason: `${SIFT_MARK} — repeats ${r.of} in this batch (same site, same title words); judged there` } } /** diff --git a/lib/triage-sift.test.mjs b/lib/triage-sift.test.mjs index 9ddf5319..56c19c6d 100644 --- a/lib/triage-sift.test.mjs +++ b/lib/triage-sift.test.mjs @@ -2,7 +2,7 @@ // findings repeat one another, the ledger rows the sift writes, and what a noise class settles. import { test } from 'vitest' import assert from 'node:assert/strict' -import { collapseRepeats, conflictEntry, locationParts, repeatsFinding, setAsideEntry, severityRank, siftByMemory, siftFinding } from './triage-sift.mjs' +import { collapseRepeats, conflictEntry, locationParts, repeatEntry, repeatsFinding, setAsideEntry, severityRank, siftByMemory, siftFinding } from './triage-sift.mjs' import { classifiedVerdict, issueOf, noiseClassOf } from './triage-noise-class.mjs' /** @param {string} title @param {string} location @param {string} [severity] @param {string} [thread] */ @@ -47,6 +47,8 @@ test('collapseRepeats keeps the most severe copy and re-points earlier repeats o const r = collapseRepeats([a, b, c, d, a]) assert.deepEqual(r.kept.map(x => x.stable_id), [c.stable_id, d.stable_id]) assert.deepEqual(r.repeated.map(x => [x.f.stable_id, x.of]), [[a.stable_id, c.stable_id], [b.stable_id, c.stable_id]]) + const e = repeatEntry(/** @type {{ f: any, of: string }} */ (r.repeated[0])) + assert.deepEqual([e.stable_id, e.verdict, e.sifted, e.repeat_of], [a.stable_id, 'reject', true, c.stable_id], 'the twin rides as a field, not only in the reason') const same = collapseRepeats([b, f('parse null deref', 'a.rs:11', 'Medium')]) assert.equal(same.kept.length, 1, 'the same id twice is kept once') }) @@ -74,23 +76,38 @@ test('noiseClassOf reads 1..6 and nothing else', () => { for (const v of [{ noise_class: 0 }, { noise_class: 7 }, { noise_class: 1.5 }, { noise_class: 'x' }, {}, null]) assert.equal(noiseClassOf(v), 0) }) -test('classifiedVerdict: class 1 defers with an issue only when the base was opened; class 5 needs a decision', () => { - const v = { stable_id: 'i', verdict: 'defer', reason: 'r', premise_checked: 'git show b:x' } - assert.deepEqual(classifiedVerdict({ ...v, noise_class: 1 }), { ...v, noise_class: 1, verdict: 'defer', action: 'issue' }) - assert.equal(classifiedVerdict({ ...v, premise_checked: 'git merge-base origin/main HEAD → abc; read lib/q.rs there', noise_class: 1 }).action, 'issue') - for (const premise of [' ', 'lib/q.rs:3', 'read the caller at src/a.rs:10', 'it was there before']) { - const unchecked = classifiedVerdict({ ...v, premise_checked: premise, noise_class: 1 }) +test('classifiedVerdict: class 1 defers with an issue only when the given PR fork point (prBase) was opened; class 5 needs a decision', () => { + const B = 'origin/main' + const v = { stable_id: 'i', verdict: 'defer', reason: 'r', premise_checked: `git show ${B}:lib/q.rs` } + assert.deepEqual(classifiedVerdict({ ...v, noise_class: 1 }, B), { ...v, noise_class: 1, verdict: 'defer', action: 'issue' }) + const sha = '0123456789abcdef0123456789abcdef01234567' + assert.equal(classifiedVerdict({ ...v, premise_checked: `ran git show ${sha}:lib/q.rs:3`, noise_class: 1 }, sha).action, 'issue') + // The form the prompt asks for comes back in backticks, quotes or parentheses. + for (const premise of [`\`git show ${B}:lib/q.rs\``, `opened it (git show ${B}:lib/q.rs)`, `"git show ${B}:lib/q.rs"`, `'git show ${B}:lib/q.rs'`]) { + assert.equal(classifiedVerdict({ ...v, premise_checked: premise, noise_class: 1 }, B).action, 'issue', premise) + } + for (const premise of [' ', 'lib/q.rs:3', 'read the caller at src/a.rs:10', 'it was there before', + 'git show HEAD:lib/q.rs', 'git show feature:lib/q.rs', 'git merge-base origin/main HEAD → abc; read lib/q.rs there', + 'merge-base', 'opened origin/main', `git show ${B}x:lib/q.rs`, `git show ${B}:`, `\`git show ${B}x:lib/q.rs\``, + '`git show HEAD:lib/q.rs`', `(git show HEAD:lib/q.rs)`, `xgit show ${B}:lib/q.rs`]) { + const unchecked = classifiedVerdict({ ...v, premise_checked: premise, noise_class: 1 }, B) assert.equal(unchecked.verdict, 'needs-decision', premise) assert.equal(unchecked.action, undefined) - assert.match(unchecked.reason, /nothing at the base was opened to show it — r$/) + assert.match(unchecked.reason, /nothing at the PR's fork point origin\/main was opened to show it — r$/) + } + for (const none of [undefined, '', ' ']) { + const noBase = classifiedVerdict({ ...v, noise_class: 1 }, none) + assert.equal(noBase.verdict, 'needs-decision', 'with no prBase given nothing can show the problem stands there') + assert.equal(noBase.action, undefined) + assert.match(noBase.reason, /no prBase \(the PR's fork point\) was given to check it at — r$/) } const notReal = { ...v, verdict: 'reject', noise_class: 1 } - assert.deepEqual(classifiedVerdict(notReal), notReal, 'not a real problem files no issue') + assert.deepEqual(classifiedVerdict(notReal, B), notReal, 'not a real problem files no issue') assert.deepEqual(classifiedVerdict({ ...notReal, premise_checked: '' }), { ...notReal, premise_checked: '' }) - assert.deepEqual(classifiedVerdict({ ...v, verdict: 'accept', noise_class: 1 }), { ...v, noise_class: 1, verdict: 'defer', action: 'issue' }) - for (const premise of ['git show b:x', '']) { + assert.deepEqual(classifiedVerdict({ ...v, verdict: 'accept', noise_class: 1 }, B), { ...v, noise_class: 1, verdict: 'defer', action: 'issue' }) + for (const premise of [v.premise_checked, '']) { const open = { ...v, verdict: 'needs-decision', premise_checked: premise, noise_class: 1 } - assert.deepEqual(classifiedVerdict(open), open, 'a needs-decision stays the human\'s, with no issue') + assert.deepEqual(classifiedVerdict(open, B), open, 'a needs-decision stays the human\'s, with no issue') } assert.equal(classifiedVerdict({ ...v, noise_class: 5 }).verdict, 'needs-decision') for (const n of [0, 2, 3, 4, 6]) assert.deepEqual(classifiedVerdict({ ...v, noise_class: n }), { ...v, noise_class: n }) diff --git a/skills/addressing-findings/SKILL.md b/skills/addressing-findings/SKILL.md index bb3c3ff2..64318695 100644 --- a/skills/addressing-findings/SKILL.md +++ b/skills/addressing-findings/SKILL.md @@ -90,7 +90,10 @@ concrete, Rust-aware process and points at the topic skills for *how* to fix eac **Scaling:** small batch → steps 2–4 inline. Large batch (a fat `rust-audit` report, a many-comment PR) → dispatch the `craft:triage-findings` workflow, apply its plan, then run the -re-review loop. The workflow sifts by memory itself — it recalls the project's records for the +re-review loop. Pass `prBase` = the output of `git merge-base origin/ ` (the +commit the PR forks from): only there can a finding be shown not to be this PR's, and without it +every such finding goes to you as `needs-decision`. `base` stays what it was — the ref the findings +were generated against, which validation pins to. The workflow sifts by memory itself — it recalls the project's records for the findings' paths and merges in what `priorDecisions` (an object argument, the step-0 records) adds; its ledger rows with `sifted: true` (their reason opens with `SIFTED BEFORE VALIDATION`) were settled by the sift — a repeat or a record's answer — not judged on the code. It returns `issues` (title, body) for the findings that are not this PR's. @@ -102,7 +105,8 @@ decided in another PR is not decided again, and the opposite way: - **Repeats** — the same finding under another id in the same batch (same thread, or same file within a few lines and the same title words) is one finding: the most severe copy goes on, the - rest are rejected as its repeats. + rest are rejected as its repeats, each row naming that copy in `repeat_of` — in the review + ledger a repeat takes its copy's disposition, not its own `reject` (below). - **Answered by memory** — a finding an active decision answers is `reject`, one a deferred question answers `defer`, one a project rule covers `reject`; the ledger reason cites the record (who, when, link) and how it matched. Matching is the review engine's own: the anchor or @@ -120,7 +124,7 @@ judged on the code, never on the reviewer's own label: | Class | What | Verdict | |---|---|---| -| 1 | not this PR's — the problem stands at the base (the diff did not bring it), or lies outside the PR's task | a real problem: `defer`, carried by an issue (below) — checked at the merge-base (`git show :` named in the premise), never on the reviewer's "pre-existing"; not a real problem: `reject`, no issue | +| 1 | not this PR's — the problem stands at the base (the diff did not bring it), or lies outside the PR's task | a real problem: `defer`, carried by an issue (below) — checked at the PR's fork point the triage was given as `prBase` and no other ref (`git show :` named in the premise; HEAD, the validation `base`, another branch or the bare word merge-base shows nothing, and with no `prBase` given a human decides), never on the reviewer's "pre-existing"; not a real problem: `reject`, no issue | | 2 | inflated severity — the harm is unreachable or heals itself (a default requeue, only a metric) | judged on the merits, severity lowered | | 3 | a repeat — the same finding under a new id, or decided in another PR | settled by the sift | | 4 | a deliberate trade-off written in the project's docs or rules | `reject`, naming where it is written | @@ -325,6 +329,12 @@ After **Triage (step 3)** and again after **Verify (step 6)**, write disposition - a fix that landed **and** was verified → `closed` - a finding kept (not fixed) but explicitly justified in the PR body → `justified` - triage `accept` (not yet fixed), `needs-decision`, `conflict` → leave `open` + - a `sifted` row is **never** mapped by its own `reject`: no one judged it on the code. + A repeat (`repeat_of` names its twin) takes its twin's disposition as the twin's row maps + — `closed` when the twin's fix landed and was verified **and** reached the repeat's line too + (else leave `open`), `deferred` when the twin is deferred, `rejected` only when the twin is + rejected. Any other sifted `reject` (a record's answer) → leave `open`: the next review sets + it aside again from the recalled record, and drops that once the record is withdrawn. 3. **Match** a fix-loop finding to a review-ledger entry by `fp` when the finding carries one; otherwise by `file` + `ruleId` + a title match. If no entry matches, skip that finding silently. 4. **Why:** this is exactly what lets the next round-aware re-review's *adjudicate track* **carry** @@ -369,8 +379,10 @@ Before the loop works the findings, what was already rejected on the PR — a re "not a bug" / "by design" / "won't fix" … — is recorded as decisions with the author, the comment URL and the commit, without asking. That holds for a craft finding's thread and for any other reviewer's — a person's remark or their own tool's — so the same remark in the next PR is sifted -out. Conservative: only when that explicit reply is the last word, from the PR author or an owner, -member or collaborator of the repository; a thread resolved without a reply is never a rejection. +out. Conservative: only when that explicit reply is the last word, from an owner, member or +collaborator of the repository — or, in a craft finding's thread only, from the PR author; in +another reviewer's thread a PR author without that association never settles the remark. A thread +resolved without a reply is never a rejection. The rule, the `gh` query and the script → [pr-rejections.md](pr-rejections.md). ## Closing the loop on GitHub diff --git a/skills/addressing-findings/pr-rejections.md b/skills/addressing-findings/pr-rejections.md index 557b95dd..8b97bdd1 100644 --- a/skills/addressing-findings/pr-rejections.md +++ b/skills/addressing-findings/pr-rejections.md @@ -19,10 +19,10 @@ A thread yields a decision **only** when all of these hold: finding: named with why and counted, never recorded. - **Another reviewer's remark**: a comment without the marker — a person's, or their own tool's — from anyone. Its first line of text (heading and emphasis marks dropped, cut at a - word past 120 characters) stands for the title. One guard more: when the rejecting reply comes - from the remark's own author and that author is not an owner, member or collaborator, nothing - is recorded — an outsider cannot settle their own remark for the whole project. A remark - nobody rejected is not listed. + word past 120 characters) stands for the title. The reply rule is narrower here (rule 3): only + an owner, member or collaborator settles such a remark — a PR author without that association + never does, not even when the remark came from another login, since one outsider can hold two + accounts and silence a remark for the whole project. A remark nobody rejected is not listed. 2. **Its last word is a reply that rejects the finding**: it **opens** with `not a bug`, `not an issue`, `by design`, `won't fix` / `wontfix` / `will not fix`, `works as intended` / `working as intended` / `as intended`, `intentional` or `false positive` (optionally after `this is` / @@ -30,10 +30,12 @@ A thread yields a decision **only** when all of these hold: by any later comment, a phrase mid-sentence ("this was not a bug before the refactor, fixed"), or "fixed in …" is not a rejection. **No reply is no rejection** — a thread resolved silently may have been fixed outside the hunk. -3. **The replier may reject**: the PR author, or GitHub's `authorAssociation` of the reply is - `OWNER`, `MEMBER` or `COLLABORATOR`. craft posts its comments under your own `gh` account, so in - a self-review loop the poster is the PR author and their own reply counts. Anyone else's - rejection is not recorded; the script names each and counts them. +3. **The replier may reject**: in a craft-finding thread, the PR author, or GitHub's + `authorAssociation` of the reply is `OWNER`, `MEMBER` or `COLLABORATOR`. craft posts its comments + under your own `gh` account, so in a self-review loop the poster is the PR author and their own + reply counts. In another reviewer's thread, only that association counts: the PR author's reply + is recorded only when they are an owner, member or collaborator. Anyone else's rejection is not + recorded; the script names each and counts them. 4. **It can be anchored**: a path and the commit the finding was posted on. Every craft-finding thread that does not qualify is listed with why, and every other reviewer's diff --git a/skills/addressing-findings/schema.md b/skills/addressing-findings/schema.md index fe577a53..299dabb2 100644 --- a/skills/addressing-findings/schema.md +++ b/skills/addressing-findings/schema.md @@ -43,7 +43,8 @@ resolve the thread in step 8; reply uses `comments[0].databaseId`), `severity` = A persisted artifact (e.g. `triage-ledger.json` next to the plan) — one entry per finding keyed by `stable_id` with its final verdict + one-line reason, and `sifted: true` on a row the sift set -aside before validation (a repeat, a record's answer). Read it at the start of a re-run so +aside before validation (a repeat, a record's answer); a repeat's row also carries `repeat_of`, the +`stable_id` of the copy it was collapsed onto and judged with. Read it at the start of a re-run so already-`reject`/`defer`/`needs-decision` findings are carried, not re-litigated, and so the re-review loop can tell a recurring finding from a new one — except a `sifted` row: the sift decides it again from the memory as it stands now, and no decision is recorded for it. diff --git a/skills/develop/SKILL.md b/skills/develop/SKILL.md index 65f27177..4f11c627 100644 --- a/skills/develop/SKILL.md +++ b/skills/develop/SKILL.md @@ -153,8 +153,10 @@ The engine's argument for each round, as an object (never a string): One **round** is: 1. **Review** — launch the engine with the round's argument above. -2. **Triage** — a large report → the `craft:triage-findings` workflow; a small one → inline, as - `craft:addressing-findings` describes. +2. **Triage** — a large report → the `craft:triage-findings` workflow, with `prBase` = the output + of `git merge-base origin/ ` (the commit the branch forks from: only there + is a finding shown not to be this branch's; without it each such finding comes back as + `needs-decision`); a small one → inline, as `craft:addressing-findings` describes. 3. **Address** — `craft:addressing-findings` works the findings through its step 6 (verify): fixes the accepted ones, writes every rejection or justification as a decision and every deferral as an open question in project memory at once, without asking. Stop it there: its re-review (step 7) is diff --git a/src/triage-findings.js b/src/triage-findings.js index f830ef98..e2669634 100644 --- a/src/triage-findings.js +++ b/src/triage-findings.js @@ -1,7 +1,7 @@ export const meta = { name: 'triage-findings', description: 'Triage review findings (craft agents + GitHub PR comments) into one ordered, validated fix plan — no edits', - whenToUse: 'After a review or rust-audit produces many findings, or a PR has many inline comments, and you want them validated against the code, deduped, conflict-checked, and turned into an ordered fix plan. It reads ONLY the checkout the session runs in: there is no `repo` argument, and passing one is refused with nothing run (use `craft:review` with repo=, or start a session inside that repository).', + whenToUse: 'After a review or rust-audit produces many findings, or a PR has many inline comments, and you want them validated against the code, deduped, conflict-checked, and turned into an ordered fix plan. It reads ONLY the checkout the session runs in: there is no `repo` argument, and passing one is refused with nothing run (use `craft:review` with repo=, or start a session inside that repository). Pass `prBase` = `git merge-base origin/ ` (the commit the PR forks from): a finding is deferred as not this PR\'s only when shown there, else it comes back needs-decision. `base` is the ref the findings were generated against, which validation pins to.', phases: [ { title: 'Gather', detail: 'pull raw findings from the requested sources (rust-audit report, reviewer verdict, GitHub PR threads)' }, { title: 'Sift', detail: 'collapse repeats in the batch; recall the project memory and set aside what an active decision, deferred question or project rule already answers — no validator for those' }, @@ -11,7 +11,8 @@ export const meta = { } // Locator args (not payload): pr (GitHub PR number), report (path to a rust-audit report or saved -// verdict), base (ref to pin validation against), priorLedger (array of prior {stable_id, verdict, +// verdict), base (ref to pin validation against), prBase (the commit the PR forks from — the merge-base +// of trunk and the reviewed head — where "not this PR's" must be shown, realm @nick/craft, node #309), priorLedger (array of prior {stable_id, verdict, // reason} for idempotent re-runs), priorDecisions (the project's records the launcher already holds — they // merge into what this engine recalls itself, realm @nick/craft, node #218). At least one of pr/report must be given. // `args` may arrive as a parsed object or as a JSON string depending on the harness — normalize. @@ -153,6 +154,7 @@ function textArg(key) { const pr = textArg('pr') const report = textArg('report') const base = textArg('base') +const prBase = textArg('prBase') /** @type {unknown[]} */ const priorLedger = Array.isArray(argv['priorLedger']) ? /** @type {unknown[]} */ (argv['priorLedger']) : [] // Where craft itself lives, so the logger can find lib/craft-log-run.mjs. It selects NO repository: @@ -233,7 +235,7 @@ const PLAN_SCHEMA = { /** @typedef {RawFinding & { source: string }} SourcedFinding */ /** @typedef {ReturnType} SiftFinding */ /** @typedef {{ stable_id: string, verdict: string, reason: string, fix_pointer: string, premise_checked: string, noise_class?: number, severity?: string, action?: string }} Validation */ -/** @typedef {{ stable_id: string, verdict: string, reason: string, sifted?: boolean, action?: string }} LedgerEntry */ +/** @typedef {{ stable_id: string, verdict: string, reason: string, sifted?: boolean, action?: string, repeat_of?: string }} LedgerEntry */ /** @typedef {{ plan_markdown: string, ledger: LedgerEntry[], summary: string }} PlanResult */ // What this script adds to the plan before returning it — not part of PLAN_SCHEMA. /** @typedef {PlanResult & { notRun?: string[], telemetryLost?: string[], issues?: Array<{ stable_id: string, title: string, body: string }>, memory?: string, priorDecisionsNotApplied?: string[], ruleNotices?: string[] }} TriagePlan */ @@ -2551,9 +2553,15 @@ function collapseRepeats(findings) { return { kept, repeated } } -/** The ledger row of a repeat. @param {{ f: SiftFinding, of: string }} r @returns {SiftEntry} */ +/** + * The ledger row of a repeat: `reject` here, since its twin alone is judged, and `repeat_of` naming that + * twin. The verdict is no disposition of its own: in the review ledger a repeat takes its twin's (closed, + * deferred, rejected), or a fix that missed the repeat's line would carry a real defect as dismissed + * (node #312). + * @param {{ f: SiftFinding, of: string }} r @returns {SiftEntry} + */ function repeatEntry(r) { - return { stable_id: r.f.stable_id, verdict: 'reject', sifted: true, reason: `${SIFT_MARK} — repeats ${r.of} in this batch (same site, same title words); judged there` } + return { stable_id: r.f.stable_id, verdict: 'reject', sifted: true, repeat_of: r.of, reason: `${SIFT_MARK} — repeats ${r.of} in this batch (same site, same title words); judged there` } } /** @@ -2603,7 +2611,7 @@ async function siftByMemory(findings, records, checkScopes, judge, ruleJudge) { } } // <<< craft-inline -// >>> craft-inline lib/triage-noise-class.mjs noiseClassOf classifiedVerdict issueOf +// >>> craft-inline lib/triage-noise-class.mjs noiseClassOf classOneBaseLine openedAt classifiedVerdict notThisPrs issueOf /** * A validator's noise class (node #303), 0 when it named none or one outside 1..6. * @param {unknown} v @returns {number} @@ -2613,26 +2621,68 @@ function noiseClassOf(v) { return Number.isInteger(n) && n >= 1 && n <= 6 ? n : 0 } +/** + * What the validator is told about where class 1 must be shown: the PR's fork point this triage was + * given, cited exactly as `classifiedVerdict` reads it, so the engine can tell the premise opened that + * commit rather than a ref the validator picked (node #309); with none, that a human decides. + * @param {string} prBase @returns {string} + */ +function classOneBaseLine(prBase) { + const at = String(prBase ?? '').trim() + return at + ? `the PR forks from \`${at}\` (prBase) — open the cited code there and record in premise_checked exactly \`git show ${at}:\`; a premise naming any other ref (HEAD, the ref you validate against, a branch, a merge-base you computed) reads as unchecked and goes to a human` + : "no prBase (the PR's fork point) was given to this triage, so it cannot be checked here — name the class all the same; a human decides it" +} + +/** + * Whether a premise shows a file opened at exactly `ref`: `git show :`, at the start, after + * a space, or inside backticks, quotes or parentheses; the ref ends at the colon, so neither another + * ref nor one that only starts with this one passes. + * @param {string} premise @param {string} ref @returns {boolean} + */ +function openedAt(premise, ref) { + const needle = `git show ${ref}:` + for (let i = premise.indexOf(needle); i !== -1; i = premise.indexOf(needle, i + 1)) { + const before = i === 0 ? ' ' : premise.charAt(i - 1) + const after = premise.charAt(i + needle.length) + if (/[\s`'"(]/.test(before) && after !== '' && !/[\s:`'")]/.test(after)) return true + } + return false +} + /** * The verdict a noise class settles: class 1 (not this PR's) on a finding the validator accepted or * deferred is a deferral carried by an issue on the consumer's forge — but only when premise_checked - * names the base it opened, else nothing settled it and a human decides; any other class-1 verdict - * stands (a `reject`: no real problem, no issue; a `needs-decision`: still the human's). Class 5 (not - * decidable from the code) is needs-decision. Every other verdict stands as the validator gave it. + * shows a file opened at `prBase`, the PR's fork point the triage was given, never a ref the validator + * picked — else nothing settled it and a human decides; with no prBase, always a human. Any other + * class-1 verdict stands (a `reject`: no real problem, no issue; a `needs-decision`: still the human's). + * Class 5 (not decidable from the code) is needs-decision. Every other verdict stands as given. * @template {{ verdict: string, reason: string, premise_checked: string }} V @param {V} v + * @param {string} [prBase] the commit the PR forks from; '' or absent when none was given * @returns {V & { action?: string }} */ -function classifiedVerdict(v) { +function classifiedVerdict(v, prBase = '') { const n = noiseClassOf(v) - if (n === 1 && v.verdict !== 'accept' && v.verdict !== 'defer') return v - if (n === 1 && !/\bmerge-base\b|\bgit show [^\s:]+:\S/.test(String(v.premise_checked ?? ''))) { - return { ...v, verdict: 'needs-decision', reason: `claimed not this PR's, but nothing at the base was opened to show it — ${v.reason}` } - } - if (n === 1) return { ...v, verdict: 'defer', action: 'issue' } + if (n === 1) return notThisPrs(v, String(prBase ?? '').trim()) if (n === 5) return { ...v, verdict: 'needs-decision' } return v } +/** + * A class-1 verdict settled: a real problem shown at `prBase` is a deferral with an issue, one not shown + * there a human's; a `reject` or `needs-decision` stands. + * @template {{ verdict: string, reason: string, premise_checked: string }} V @param {V} v @param {string} prBase + * @returns {V & { action?: string }} + */ +function notThisPrs(v, prBase) { + if (v.verdict !== 'accept' && v.verdict !== 'defer') return v + if (!prBase) return { ...v, verdict: 'needs-decision', reason: `claimed not this PR's, but no prBase (the PR's fork point) was given to check it at — ${v.reason}` } + if (!openedAt(String(v.premise_checked ?? ''), prBase)) { + return { ...v, verdict: 'needs-decision', reason: `claimed not this PR's, but nothing at the PR's fork point ${prBase} was opened to show it — ${v.reason}` } + } + return { ...v, verdict: 'defer', action: 'issue' } +} + /** * The issue a class-1 finding becomes: its title, and a body naming where it stands, why it is not this * PR's, and the source's own words. @@ -2781,6 +2831,7 @@ phase('Validate') const pin = base ? `Validate against ref \`${base}\` (the ref the findings were generated against), not the live working tree.` : 'Validate against the currently checked-out tree.' +const classOneBase = classOneBaseLine(prBase) // The sentinel that marks a `needs-decision` nothing actually judged. It rides in the ledger entry's // `reason` (a carry-forward prefixes that reason, so `includes` is the test, not equality) and it @@ -2844,7 +2895,7 @@ Read the cited code, then decide ONE verdict: PREMISE DISCIPLINE: name the ONE claim your verdict rests on. If it lives outside the cited location — the dependency behaves this way, this is reachable from untrusted input, a caller already guards it, the sibling path does X — OPEN that code (dependency sources included) and record the file:line in premise_checked. This binds **reject** exactly as much as accept: "a caller must already validate this" waved through without opening the caller is the same unfounded claim as the finding it dismisses, and it silently discards a real bug. If you cannot open it, do not guess — verdict needs-decision, saying which premise is unverified. NOISE CLASSES (noise_class; 0 when none applies — a defect of this change). They are judged on the code, never on the source's own label: -1 — not this change's: the problem already stands at the PR's base (the diff did not bring it) or lies outside the change's task. Check it AT THE BASE, not by the reviewer's word ("pre-existing", "out of scope"): find the base (\`gh pr view --json baseRefName\` for a PR, else the remote's default branch), take \`git merge-base\` with the ref you validate, open the cited code there and record that command in premise_checked as \`git show :\` — a premise naming neither a merge-base nor a \`git show :\` reads as unchecked. Verdict defer when the problem is real — the triage carries it to an issue; reject when it is not (no issue is filed). +1 — not this change's: the problem already stands at the PR's base (the diff did not bring it) or lies outside the change's task. Check it AT THE BASE, not by the reviewer's word ("pre-existing", "out of scope"): ${classOneBase}. Verdict defer when the problem is real — the triage carries it to an issue; reject when it is not (no issue is filed). 2 — inflated severity: the claimed harm is unreachable or heals itself (a default requeue, only a metric). Judge the verdict on the merits and lower severity to what the code supports. 3 — a repeat of a finding already decided — reject, naming it. 4 — a deliberate trade-off written in the project's docs or rules — reject, naming where it is written. @@ -2866,7 +2917,7 @@ Keep reason to one line. fix_pointer empty unless verdict is accept.`, }) } // A noise class settles some verdicts (realm @nick/craft, node #303): class 1 becomes a deferral carried by an issue. -const validations = (await parallel(sift.toValidate.map(f => () => validateOne(f)))).filter(v => !!v).map(classifiedVerdict) +const validations = (await parallel(sift.toValidate.map(f => () => validateOne(f)))).filter(v => !!v).map(v => classifiedVerdict(v, prBase)) const accepted = validations.filter(v => v.verdict === 'accept') log(`Validated ${validations.length}: ${accepted.length} accept, ${validations.length - accepted.length} other.`) @@ -2922,8 +2973,8 @@ function finalLedger(plan) { * @param {LedgerEntry[]} planLedger @returns {LedgerEntry[]} */ function pinnedLedger(planLedger) { - /** A pinned row's own verdict and reason, and its `sifted` field when it has one. @param {{ stable_id: string }} e @param {LedgerEntry} v @returns {LedgerEntry} */ - const restored = (e, v) => ({ ...e, verdict: v.verdict, reason: v.reason, ...(v.sifted ? { sifted: true } : {}) }) + /** A pinned row's own verdict and reason, its `sifted` field and a repeat's `repeat_of` (realm @nick/craft, node #312) when it has them. @param {{ stable_id: string }} e @param {LedgerEntry} v @returns {LedgerEntry} */ + const restored = (e, v) => ({ ...e, verdict: v.verdict, reason: v.reason, ...(v.sifted ? { sifted: true } : {}), ...(v.repeat_of ? { repeat_of: v.repeat_of } : {}) }) // The prompt above ASKS the plan agent to copy the marker verbatim; asking is not a guarantee. A // summarising model paraphrases a one-line free-text reason as a matter of course, and the marker // is the ONLY thing that tells the next run this finding was never judged: lose it and the finding @@ -2942,7 +2993,7 @@ function pinnedLedger(planLedger) { if (!v) return e seen.add(e.stable_id) const mark = v.reason.includes(UNJUDGED_MARKER) ? UNJUDGED_MARKER : SIFT_MARK - const kept = String(e.reason || '').includes(mark) && e.verdict === v.verdict && e.sifted === v.sifted + const kept = String(e.reason || '').includes(mark) && e.verdict === v.verdict && e.sifted === v.sifted && e.repeat_of === v.repeat_of return kept ? e : restored(e, v) }) for (const [id, v] of pinned) if (!seen.has(id)) ledger.push(restored({ stable_id: id }, v)) diff --git a/workflows/triage-findings.js b/workflows/triage-findings.js index 7e40f593..ffeae9d3 100644 --- a/workflows/triage-findings.js +++ b/workflows/triage-findings.js @@ -1,7 +1,7 @@ export const meta = { name: 'triage-findings', description: 'Triage review findings (craft agents + GitHub PR comments) into one ordered, validated fix plan — no edits', - whenToUse: 'After a review or rust-audit produces many findings, or a PR has many inline comments, and you want them validated against the code, deduped, conflict-checked, and turned into an ordered fix plan. It reads ONLY the checkout the session runs in: there is no `repo` argument, and passing one is refused with nothing run (use `craft:review` with repo=, or start a session inside that repository).', + whenToUse: 'After a review or rust-audit produces many findings, or a PR has many inline comments, and you want them validated against the code, deduped, conflict-checked, and turned into an ordered fix plan. It reads ONLY the checkout the session runs in: there is no `repo` argument, and passing one is refused with nothing run (use `craft:review` with repo=, or start a session inside that repository). Pass `prBase` = `git merge-base origin/ ` (the commit the PR forks from): a finding is deferred as not this PR\'s only when shown there, else it comes back needs-decision. `base` is the ref the findings were generated against, which validation pins to.', phases: [ { title: 'Gather', detail: 'pull raw findings from the requested sources (rust-audit report, reviewer verdict, GitHub PR threads)' }, { title: 'Sift', detail: 'collapse repeats in the batch; recall the project memory and set aside what an active decision, deferred question or project rule already answers — no validator for those' }, @@ -137,6 +137,7 @@ function textArg(key) { const pr = textArg('pr') const report = textArg('report') const base = textArg('base') +const prBase = textArg('prBase') /** @type {unknown[]} */ const priorLedger = Array.isArray(argv['priorLedger']) ? /** @type {unknown[]} */ (argv['priorLedger']) : [] const craftRootArg = textArg('craftRoot') @@ -210,7 +211,7 @@ const PLAN_SCHEMA = { /** @typedef {RawFinding & { source: string }} SourcedFinding */ /** @typedef {ReturnType} SiftFinding */ /** @typedef {{ stable_id: string, verdict: string, reason: string, fix_pointer: string, premise_checked: string, noise_class?: number, severity?: string, action?: string }} Validation */ -/** @typedef {{ stable_id: string, verdict: string, reason: string, sifted?: boolean, action?: string }} LedgerEntry */ +/** @typedef {{ stable_id: string, verdict: string, reason: string, sifted?: boolean, action?: string, repeat_of?: string }} LedgerEntry */ /** @typedef {{ plan_markdown: string, ledger: LedgerEntry[], summary: string }} PlanResult */ /** @typedef {PlanResult & { notRun?: string[], telemetryLost?: string[], issues?: Array<{ stable_id: string, title: string, body: string }>, memory?: string, priorDecisionsNotApplied?: string[], ruleNotices?: string[] }} TriagePlan */ @@ -2482,9 +2483,15 @@ function collapseRepeats(findings) { return { kept, repeated } } -/** The ledger row of a repeat. @param {{ f: SiftFinding, of: string }} r @returns {SiftEntry} */ +/** + * The ledger row of a repeat: `reject` here, since its twin alone is judged, and `repeat_of` naming that + * twin. The verdict is no disposition of its own: in the review ledger a repeat takes its twin's (closed, + * deferred, rejected), or a fix that missed the repeat's line would carry a real defect as dismissed + * (node #312). + * @param {{ f: SiftFinding, of: string }} r @returns {SiftEntry} + */ function repeatEntry(r) { - return { stable_id: r.f.stable_id, verdict: 'reject', sifted: true, reason: `${SIFT_MARK} — repeats ${r.of} in this batch (same site, same title words); judged there` } + return { stable_id: r.f.stable_id, verdict: 'reject', sifted: true, repeat_of: r.of, reason: `${SIFT_MARK} — repeats ${r.of} in this batch (same site, same title words); judged there` } } /** @@ -2542,26 +2549,68 @@ function noiseClassOf(v) { return Number.isInteger(n) && n >= 1 && n <= 6 ? n : 0 } +/** + * What the validator is told about where class 1 must be shown: the PR's fork point this triage was + * given, cited exactly as `classifiedVerdict` reads it, so the engine can tell the premise opened that + * commit rather than a ref the validator picked (node #309); with none, that a human decides. + * @param {string} prBase @returns {string} + */ +function classOneBaseLine(prBase) { + const at = String(prBase ?? '').trim() + return at + ? `the PR forks from \`${at}\` (prBase) — open the cited code there and record in premise_checked exactly \`git show ${at}:\`; a premise naming any other ref (HEAD, the ref you validate against, a branch, a merge-base you computed) reads as unchecked and goes to a human` + : "no prBase (the PR's fork point) was given to this triage, so it cannot be checked here — name the class all the same; a human decides it" +} + +/** + * Whether a premise shows a file opened at exactly `ref`: `git show :`, at the start, after + * a space, or inside backticks, quotes or parentheses; the ref ends at the colon, so neither another + * ref nor one that only starts with this one passes. + * @param {string} premise @param {string} ref @returns {boolean} + */ +function openedAt(premise, ref) { + const needle = `git show ${ref}:` + for (let i = premise.indexOf(needle); i !== -1; i = premise.indexOf(needle, i + 1)) { + const before = i === 0 ? ' ' : premise.charAt(i - 1) + const after = premise.charAt(i + needle.length) + if (/[\s`'"(]/.test(before) && after !== '' && !/[\s:`'")]/.test(after)) return true + } + return false +} + /** * The verdict a noise class settles: class 1 (not this PR's) on a finding the validator accepted or * deferred is a deferral carried by an issue on the consumer's forge — but only when premise_checked - * names the base it opened, else nothing settled it and a human decides; any other class-1 verdict - * stands (a `reject`: no real problem, no issue; a `needs-decision`: still the human's). Class 5 (not - * decidable from the code) is needs-decision. Every other verdict stands as the validator gave it. + * shows a file opened at `prBase`, the PR's fork point the triage was given, never a ref the validator + * picked — else nothing settled it and a human decides; with no prBase, always a human. Any other + * class-1 verdict stands (a `reject`: no real problem, no issue; a `needs-decision`: still the human's). + * Class 5 (not decidable from the code) is needs-decision. Every other verdict stands as given. * @template {{ verdict: string, reason: string, premise_checked: string }} V @param {V} v + * @param {string} [prBase] the commit the PR forks from; '' or absent when none was given * @returns {V & { action?: string }} */ -function classifiedVerdict(v) { +function classifiedVerdict(v, prBase = '') { const n = noiseClassOf(v) - if (n === 1 && v.verdict !== 'accept' && v.verdict !== 'defer') return v - if (n === 1 && !/\bmerge-base\b|\bgit show [^\s:]+:\S/.test(String(v.premise_checked ?? ''))) { - return { ...v, verdict: 'needs-decision', reason: `claimed not this PR's, but nothing at the base was opened to show it — ${v.reason}` } - } - if (n === 1) return { ...v, verdict: 'defer', action: 'issue' } + if (n === 1) return notThisPrs(v, String(prBase ?? '').trim()) if (n === 5) return { ...v, verdict: 'needs-decision' } return v } +/** + * A class-1 verdict settled: a real problem shown at `prBase` is a deferral with an issue, one not shown + * there a human's; a `reject` or `needs-decision` stands. + * @template {{ verdict: string, reason: string, premise_checked: string }} V @param {V} v @param {string} prBase + * @returns {V & { action?: string }} + */ +function notThisPrs(v, prBase) { + if (v.verdict !== 'accept' && v.verdict !== 'defer') return v + if (!prBase) return { ...v, verdict: 'needs-decision', reason: `claimed not this PR's, but no prBase (the PR's fork point) was given to check it at — ${v.reason}` } + if (!openedAt(String(v.premise_checked ?? ''), prBase)) { + return { ...v, verdict: 'needs-decision', reason: `claimed not this PR's, but nothing at the PR's fork point ${prBase} was opened to show it — ${v.reason}` } + } + return { ...v, verdict: 'defer', action: 'issue' } +} + /** * The issue a class-1 finding becomes: its title, and a body naming where it stands, why it is not this * PR's, and the source's own words. @@ -2685,6 +2734,7 @@ phase('Validate') const pin = base ? `Validate against ref \`${base}\` (the ref the findings were generated against), not the live working tree.` : 'Validate against the currently checked-out tree.' +const classOneBase = classOneBaseLine(prBase) const UNJUDGED_MARKER = 'NOT JUDGED' @@ -2728,7 +2778,7 @@ Read the cited code, then decide ONE verdict: PREMISE DISCIPLINE: name the ONE claim your verdict rests on. If it lives outside the cited location — the dependency behaves this way, this is reachable from untrusted input, a caller already guards it, the sibling path does X — OPEN that code (dependency sources included) and record the file:line in premise_checked. This binds **reject** exactly as much as accept: "a caller must already validate this" waved through without opening the caller is the same unfounded claim as the finding it dismisses, and it silently discards a real bug. If you cannot open it, do not guess — verdict needs-decision, saying which premise is unverified. NOISE CLASSES (noise_class; 0 when none applies — a defect of this change). They are judged on the code, never on the source's own label: -1 — not this change's: the problem already stands at the PR's base (the diff did not bring it) or lies outside the change's task. Check it AT THE BASE, not by the reviewer's word ("pre-existing", "out of scope"): find the base (\`gh pr view --json baseRefName\` for a PR, else the remote's default branch), take \`git merge-base\` with the ref you validate, open the cited code there and record that command in premise_checked as \`git show :\` — a premise naming neither a merge-base nor a \`git show :\` reads as unchecked. Verdict defer when the problem is real — the triage carries it to an issue; reject when it is not (no issue is filed). +1 — not this change's: the problem already stands at the PR's base (the diff did not bring it) or lies outside the change's task. Check it AT THE BASE, not by the reviewer's word ("pre-existing", "out of scope"): ${classOneBase}. Verdict defer when the problem is real — the triage carries it to an issue; reject when it is not (no issue is filed). 2 — inflated severity: the claimed harm is unreachable or heals itself (a default requeue, only a metric). Judge the verdict on the merits and lower severity to what the code supports. 3 — a repeat of a finding already decided — reject, naming it. 4 — a deliberate trade-off written in the project's docs or rules — reject, naming where it is written. @@ -2745,7 +2795,7 @@ Keep reason to one line. fix_pointer empty unless verdict is accept.`, return { stable_id: id, verdict: 'needs-decision', reason: `${UNJUDGED_MARKER} — the validator agent died; this finding was never checked against the code`, fix_pointer: '', premise_checked: '(validator died — nothing was opened)' } }) } -const validations = (await parallel(sift.toValidate.map(f => () => validateOne(f)))).filter(v => !!v).map(classifiedVerdict) +const validations = (await parallel(sift.toValidate.map(f => () => validateOne(f)))).filter(v => !!v).map(v => classifiedVerdict(v, prBase)) const accepted = validations.filter(v => v.verdict === 'accept') log(`Validated ${validations.length}: ${accepted.length} accept, ${validations.length - accepted.length} other.`) @@ -2796,8 +2846,8 @@ function finalLedger(plan) { * @param {LedgerEntry[]} planLedger @returns {LedgerEntry[]} */ function pinnedLedger(planLedger) { - /** A pinned row's own verdict and reason, and its `sifted` field when it has one. @param {{ stable_id: string }} e @param {LedgerEntry} v @returns {LedgerEntry} */ - const restored = (e, v) => ({ ...e, verdict: v.verdict, reason: v.reason, ...(v.sifted ? { sifted: true } : {}) }) + /** A pinned row's own verdict and reason, its `sifted` field and a repeat's `repeat_of` (realm @nick/craft, node #312) when it has them. @param {{ stable_id: string }} e @param {LedgerEntry} v @returns {LedgerEntry} */ + const restored = (e, v) => ({ ...e, verdict: v.verdict, reason: v.reason, ...(v.sifted ? { sifted: true } : {}), ...(v.repeat_of ? { repeat_of: v.repeat_of } : {}) }) /** @type {Map} */ const pinned = new Map([...sifted, ...validations.filter(v => String(v.reason || '').includes(UNJUDGED_MARKER))].map(v => [v.stable_id, v])) if (!pinned.size) return planLedger @@ -2808,7 +2858,7 @@ function pinnedLedger(planLedger) { if (!v) return e seen.add(e.stable_id) const mark = v.reason.includes(UNJUDGED_MARKER) ? UNJUDGED_MARKER : SIFT_MARK - const kept = String(e.reason || '').includes(mark) && e.verdict === v.verdict && e.sifted === v.sifted + const kept = String(e.reason || '').includes(mark) && e.verdict === v.verdict && e.sifted === v.sifted && e.repeat_of === v.repeat_of return kept ? e : restored(e, v) }) for (const [id, v] of pinned) if (!seen.has(id)) ledger.push(restored({ stable_id: id }, v))