openvibers/openvibe.codes Commits
engine: a review remembers its findings across revisions (#371)
Changes
@@@ -251,6 +251,22 @@ validate({ verdict: 'confirmed', /* … */ });isLead({ verdict: 'needs_validation' }); // true```++### `findings.js` — a review that remembers what it already said++++The review's ledger across revisions. `parse(text)` reads the findings of a JSON answer (or the older numbered++`VERDICT: FIX` bullets); `merge(prior, current)` reconciles them into ACTIVE/RESOLVED records and counts what was++addressed, reappeared, is still open and is new; `priorBrief(records)` is the compact block a re-review prompt gets;++`lifecycleLine()` is the one line a person reads. The resolved set is capped, and an approval is an empty list — no++synthetic "no issues" finding.++++```js++const { fromReview, priorBrief, lifecycleLine } = require('./engine/findings');++++const first = fromReview('{"verdict":"changes_requested","findings":[{"file":"a.js","line":1,"text":"it races"}]}');++const again = fromReview('{"verdict":"approved","findings":[]}', first.findings);++// again.line = 'Findings: 1 addressed, 0 reappeared, 0 still open, 0 new'++```++## Tests`test/engine-*.test.js` in the Codes repo, run by the repo's own runner (`npm test`).nnew file mode 100644@@@ -0,0 +1,151 @@++'use strict';++++/**++ * A review's findings across revisions: the stable id, the ACTIVE/RESOLVED lifecycle, and the line a person reads.++ *++ * A finding is identified by the repository-relative file and the whitespace-normalised, lowercased text++ * (`engine/finding.js`'s findingId), so re-wrapping the same sentence keeps its id while a genuine rewording becomes++ * a new finding. The first review creates ACTIVE records; a later review is given the prior records and reconciles++ * them: a prior finding that is no longer raised is addressed (RESOLVED), a prior RESOLVED one that comes back has++ * reappeared (ACTIVE), a prior ACTIVE one still raised is still open, and anything new is new. The resolved set is++ * capped, oldest first, because a long review history is not worth its prompt tokens.++ *++ * parse(text) the findings in a review's answer (JSON findings or legacy numbered VERDICT bullets)++ * merge(prior, current) { findings, lifecycle, line } — the merged lifecycle++ * fromReview(text, prior) parse + merge++ * priorBrief(records) the compact block a re-review prompt gets instead of the whole earlier answer++ * lifecycleLine(lifecycle) "Findings: 1 addressed, 0 reappeared, 2 still open, 1 new"++ * STATE, MAX_FINDINGS, RESOLVED_CAP++ *++ * Quiet by default lives in the review prompt (engine/review.js) and in this cap: at most three findings, and no++ * synthetic "no issues" finding — an approval is an empty list, not a comment.++ */++++const { findingId } = require('./finding');++++const STATE = { ACTIVE: 'ACTIVE', RESOLVED: 'RESOLVED' };++const MAX_FINDINGS = 3;++const RESOLVED_CAP = 20;++const SEVERITIES = ['low', 'medium', 'high', 'critical'];++++/** One finding, from what a reviewer said. A record without a file and words has no identity and is dropped. */++function record(item) {++ if (!item || typeof item !== 'object') return null;++ const file = String(item.file || '').trim().replace(/^\.\//, '');++ const text = String(item.text || item.description || item.summary || '').trim();++ if (!file || !text) return null;++ const line = Number.isInteger(item.line) && item.line > 0 ? item.line : null;++ const severity = SEVERITIES.includes(String(item.severity || '').toLowerCase()) ? String(item.severity).toLowerCase() : null;++ return {++ finding_id: findingId(file, text),++ file,++ line,++ text: text.slice(0, 2000),++ severity,++ state: STATE.ACTIVE,++ };++}++++/** The JSON object in an answer, when it wrote one (the prompt asks for JSON). */++function jsonOf(text) {++ const raw = String(text || '').trim();++ const start = raw.indexOf('{'), end = raw.lastIndexOf('}');++ if (start < 0 || end <= start) return null;++ try { return JSON.parse(raw.slice(start, end + 1)); } catch { return null; }++}++++/** A file:line at the start of a legacy numbered bullet. */++const BULLET_FILE = /^([\w./-]+\.[A-Za-z0-9]+):(\d+)\s*(.*)$/;++const BULLET_FILE_NO_LINE = /^([\w./-]+\.[A-Za-z0-9]+)\s*(?:[—:-]\s*)?(.*)$/;++++/**++ * The findings in a review answer: the JSON `findings` array when the reviewer wrote one, else the numbered bullets++ * of the older `VERDICT: FIX` protocol ("1. server/x.js:31 the cache never expires").++ *++ * @param {string} text the review++ * @returns {object[]} the records, capped at MAX_FINDINGS++ */++function parse(text) {++ const doc = jsonOf(text);++ if (doc && Array.isArray(doc.findings)) {++ return doc.findings.map(record).filter(Boolean).slice(0, MAX_FINDINGS);++ }++ const out = [];++ const lines = String(text || '').split('\n');++ const start = lines.findIndex((l) => /^\W*VERDICT:\s*(SHIP|FIX)/i.test(l));++ for (const line of lines.slice(start < 0 ? 0 : start + 1)) {++ const m = /^\s*\d+[.)]\s+(.*)$/.exec(line);++ if (!m) continue;++ const body = m[1].trim();++ const withLine = BULLET_FILE.exec(body), withoutLine = BULLET_FILE_NO_LINE.exec(body);++ const hit = withLine || withoutLine;++ if (!hit) continue;++ const rec = record({ file: hit[1], line: withLine ? +hit[2] : null, text: withLine ? hit[3] : hit[2] });++ if (rec) out.push(rec);++ if (out.length >= MAX_FINDINGS) break;++ }++ return out;++}++++/** The line a person reads at the top of a re-review, or null when there is nothing to say. */++function lifecycleLine(lifecycle) {++ if (!lifecycle) return null;++ const { addressed = 0, reappeared = 0, still_open = 0, new: fresh = 0 } = lifecycle;++ if (!addressed && !reappeared && !still_open && !fresh) return null;++ return `Findings: ${addressed} addressed, ${reappeared} reappeared, ${still_open} still open, ${fresh} new`;++}++++/**++ * Reconcile this review's findings with the prior ledger.++ *++ * @param {object[]} prior the earlier records (ACTIVE and RESOLVED)++ * @param {object[]} current this review's records++ * @returns {{ findings: object[], lifecycle: object, line: string|null }} the merged ledger and its line++ */++function merge(prior = [], current = []) {++ const prev = Array.isArray(prior) ? prior.filter((p) => p && p.finding_id) : [];++ const cur = Array.isArray(current) ? current.filter((c) => c && c.finding_id) : [];++ const byId = new Map(prev.map((p) => [p.finding_id, p]));++ const seen = new Set();++ const lifecycle = { addressed: 0, reappeared: 0, still_open: 0, new: 0 };++ const findings = [];++ for (const c of cur) {++ if (seen.has(c.finding_id)) continue;++ seen.add(c.finding_id);++ const p = byId.get(c.finding_id);++ if (!p) lifecycle.new++;++ else if (p.state === STATE.RESOLVED) lifecycle.reappeared++;++ else lifecycle.still_open++;++ findings.push({ ...c, state: STATE.ACTIVE });++ }++ for (const p of prev) {++ if (p.state === STATE.RESOLVED || seen.has(p.finding_id)) continue;++ lifecycle.addressed++;++ findings.push({ ...p, state: STATE.RESOLVED });++ }++ // Keep the ledger readable: the oldest resolved findings leave first.++ const resolved = findings.filter((f) => f.state === STATE.RESOLVED);++ if (resolved.length > RESOLVED_CAP) {++ const drop = new Set(resolved.slice(0, resolved.length - RESOLVED_CAP).map((f) => f.finding_id));++ return { findings: findings.filter((f) => !drop.has(f.finding_id)), lifecycle, line: lifecycleLine(lifecycle) };++ }++ return { findings, lifecycle, line: lifecycleLine(lifecycle) };++}++++/** Parse this review and reconcile it with the prior ledger. */++function fromReview(text, prior = []) {++ return merge(prior, parse(text));++}++++/** The compact block a re-review prompt gets: the ids and words, so the reviewer can tell addressed from reworded. */++function priorBrief(records, limit = 8000) {++ const list = Array.isArray(records) ? records.filter((r) => r && r.finding_id) : [];++ if (!list.length) return '';++ const lines = list.slice(0, 20).map((r) => `- [${r.state}] ${r.file}${r.line ? ':' + r.line : ''} — ${r.text} (id ${r.finding_id})`);++ return lines.join('\n').slice(0, limit);++}++++module.exports = {++ STATE, MAX_FINDINGS, RESOLVED_CAP,++ record, parse, merge, fromReview, priorBrief, lifecycleLine,++};@@@ -20,6 +20,7 @@* engine.proof the proof of a finished attempt: its tests, its pages, its diff, and a second opinion* engine.review the second opinion itself: a model of another family, through OpenVibe.AI* engine.finding one finding in the agent.finding@1 shape: confirmed, a lead that needs validation, or rejected++ * engine.findings a review's findings across revisions: stable ids, ACTIVE/RESOLVED and the lifecycle line** The modules do not depend on each other in a cycle, and none of them reads the portal's log folder.*/@@@ -41,6 +42,7 @@ const redact = require('./redact');const proof = require('./proof');const review = require('./review');const finding = require('./finding');++const findings = require('./findings');const goal = require('./goal');module.exports = {@@@ -61,6 +63,7 @@ module.exports = {proof,review,finding,++ findings,goal,// The names a caller usually wants, in one place.classify: router.classify,@@@ -296,7 +296,8 @@ async function collectProof({ cwd, files = [], adapters = {}, author = null, emiawait say({kind: 'review', label: 'review', ok: verdict.verdict === 'approved', skipped: false,family: verdict.family, model: verdict.model,-- summary: verdict.summary || (verdict.verdict === 'approved' ? 'approved' : 'changes requested'),++ summary: [verdict.summary || (verdict.verdict === 'approved' ? 'approved' : 'changes requested'), verdict.lifecycle_line]++ .filter(Boolean).join(' · '),});}}@@@ -14,13 +14,18 @@* familyOf(model) the family a model belongs to ('anthropic', 'openai', 'deepseek', …)* authorFamily({ agent, model }) the family that wrote the change* pickReviewer({ models, author }) a model of another family, or null when there is none-- * reviewWithAi({ … }) the review as { family, verdict, summary }++ * reviewWithAi({ … }) the review as { family, verdict, summary, findings, lifecycle, lifecycle_line }++ * readReview(text) { verdict, summary, findings } from JSON or from the words*-- * The verdict is 'approved' or 'changes_requested'. The model is asked for JSON; a model that answers in prose is-- * read by its words ('approved', 'looks good' → approved; 'changes', 'needs work' → changes_requested), because a-- * review that cannot be read is not a review.++ * The verdict is 'approved' or 'changes_requested'. The model is asked for JSON with its findings; a model that answers++ * in prose is read by its words ('approved', 'looks good' → approved; 'changes', 'needs work' → changes_requested),++ * because a review that cannot be read is not a review. Quiet by default: only issues this diff introduces, at most++ * three, no style, no speculation, and no synthetic "no issues" finding. Re-review is given the prior ledger so a++ * finding is addressed, reappeared, still open or new rather than lost in a fresh paragraph.*/++const { MAX_FINDINGS, priorBrief, fromReview, lifecycleLine, parse: parseFindings } = require('./findings');++/** The families this knows, by the model's own name. */const FAMILIES = [{ family: 'anthropic', re: /claude/i },@@@ -85,14 +90,23 @@ function pickReviewer({ models = [], author = null } = {}) {return null;}--/** The prompt the reviewer gets: the change, the tests it claims to pass, and what is being asked of it. */--function reviewPrompt({ patch = '', summary = '', tests = null } = {}) {++/**++ * The prompt the reviewer gets: the change, what it was for, the tests, and the prior findings on a re-review.++ * Quiet by default — the rules are the point: only what this diff introduces, at most three findings, no style, no++ * speculation, and an approval is an empty list rather than a "no issues" note.++ */++function reviewPrompt({ patch = '', summary = '', tests = null, prior = [] } = {}) {++ const before = priorBrief(prior);return ['You are reviewing a change to a repository. Answer only with JSON:',-- '{"verdict":"approved"|"changes_requested","summary":"one paragraph, what you checked and what you found"}.',-- 'Answer "approved" when the change does what it says and holds together; "changes_requested" when it is wrong,',-- 'unsafe, or leaves the work unfinished. Be concrete: name the file and the line.',++ `{"verdict":"approved"|"changes_requested","summary":"one sentence, what you checked","findings":[{"file":"path","line":12,"text":"what is wrong and the concrete fix","severity":"low"|"medium"|"high"|"critical"}]}`,++ 'Flag only issues this change introduces: a correctness bug, a security problem, a missing or broken test, or a',++ `requirement the change leaves undone. At most ${MAX_FINDINGS} findings, worst first. No style nits, no "may be`,++ 'unsafe" without the path that reaches it, and no re-raising a design choice the change was asked to make. If a',++ 'high-impact point is plausible but not proven, say "uncertain" in its text and keep the severity low. If nothing',++ 'is wrong, "approved" with an empty findings list; never write a "no issues" finding or a comment.','',++ before ? 'A previous review of an earlier revision raised the findings below. Treat them as the ledger: say in your\nsummary which are addressed, and report one again only when it still stands in this revision; then add only new\nissues this revision introduced. The ids stay stable across re-wrapping.\n' + before : '',summary ? 'What the change was asked to do:\n' + String(summary).slice(0, 4000) : '',tests ? 'The tests the author says pass:\n' + String(tests).slice(0, 2000) : '','',@@@ -100,23 +114,34 @@ function reviewPrompt({ patch = '', summary = '', tests = null } = {}) {].filter(Boolean).join('\n');}--/** The answer as JSON, or as the words in it. */--function readVerdict(text) {++/** The answer as JSON, or as the words in it, with the findings it raised (an empty list on approval). */++function readReview(text) {const raw = String(text || '').trim();-- if (!raw) return { verdict: 'changes_requested', summary: 'the reviewer said nothing' };++ if (!raw) return { verdict: 'changes_requested', summary: 'the reviewer said nothing', findings: [] };const start = raw.indexOf('{');const end = raw.lastIndexOf('}');if (start >= 0 && end > start) {try {const doc = JSON.parse(raw.slice(start, end + 1));++ const findings = Array.isArray(doc.findings) ? doc.findings.map((f) => ({++ file: String(f.file || '').trim(), line: Number.isInteger(f.line) ? f.line : null,++ text: String(f.text || f.description || '').trim(), severity: f.severity || null,++ })).filter((f) => f.file && f.text).slice(0, MAX_FINDINGS) : [];const verdict = String(doc.verdict || '').toLowerCase();-- if (/approv|looks good|lgtm/.test(verdict)) return { verdict: 'approved', summary: String(doc.summary || raw).slice(0, 4000) };-- if (/change|reject|not good/.test(verdict)) return { verdict: 'changes_requested', summary: String(doc.summary || raw).slice(0, 4000) };-- if (doc.summary) return { verdict: 'changes_requested', summary: String(doc.summary).slice(0, 4000) };++ if (/approv|looks good|lgtm/.test(verdict)) return { verdict: 'approved', summary: String(doc.summary || raw).slice(0, 4000), findings };++ if (/change|reject|not good/.test(verdict)) return { verdict: 'changes_requested', summary: String(doc.summary || raw).slice(0, 4000), findings };++ if (doc.summary) return { verdict: 'changes_requested', summary: String(doc.summary).slice(0, 4000), findings };} catch { /* not JSON: read the words */ }}-- if (/\bapprov|looks good|looks fine|lgtm|no changes\b/i.test(raw)) return { verdict: 'approved', summary: raw.slice(0, 4000) };-- return { verdict: 'changes_requested', summary: raw.slice(0, 4000) };++ const findings = parseFindings(raw);++ if (/\bapprov|looks good|looks fine|lgtm|no changes\b/i.test(raw)) return { verdict: 'approved', summary: raw.slice(0, 4000), findings };++ return { verdict: 'changes_requested', summary: raw.slice(0, 4000), findings };++}++++/** The verdict alone (the older callers). */++function readVerdict(text) {++ const read = readReview(text);++ return { verdict: read.verdict, summary: read.summary };}/**@@@ -140,12 +165,13 @@ async function reviewWithAi({patch = '',summary = '',tests = null,++ prior = [],fetchImpl = globalThis.fetch,} = {}) {if (!token) throw new Error('the review needs the task\'s token');if (!model) throw new Error('the review needs a model');const root = String(baseUrl).replace(/\/+$/, '');-- const prompt = reviewPrompt({ patch, summary, tests });++ const prompt = reviewPrompt({ patch, summary, tests, prior });const headers = { 'content-type': 'application/json', authorization: `Bearer ${token}` };let res = await fetchImpl(`${root}/chat/completions`, {method: 'POST', headers, body: JSON.stringify({ model, messages: [{ role: 'user', content: prompt }] }),@@@ -170,7 +196,9 @@ async function reviewWithAi({const text = body && body.choices && body.choices[0] && body.choices[0].message && body.choices[0].message.content? body.choices[0].message.content: (body && body.content && Array.isArray(body.content) ? body.content.map((c) => c && c.text).filter(Boolean).join(' ') : '');-- const verdict = readVerdict(text);++ const read = readReview(text);++ // A reviewer that said nothing reviewed nothing: its prior findings are kept, never silently marked addressed.++ const ledger = String(text || '').trim() ? fromReview(text, prior) : { findings: Array.isArray(prior) ? prior : [], lifecycle: null, line: null };// The route disclosure (D3'): the model that actually ran, the deployment and the cost, inside x_openvibe.const x = body && body.x_openvibe && typeof body.x_openvibe === 'object' ? body.x_openvibe : null;const resolved = x && x.resolved_model ? String(x.resolved_model) : (body && body.model ? String(body.model) : model);@@@ -181,12 +209,16 @@ async function reviewWithAi({...(x && x.resolved_model ? { resolved_model: String(x.resolved_model) } : {}),...(x ? { disclosure: x } : {}),...(x && Number.isFinite(+x.cost_usd) ? { cost_usd: +x.cost_usd } : {}),-- verdict: verdict.verdict,-- summary: verdict.summary,++ verdict: read.verdict,++ summary: read.summary,++ // The persisted lifecycle: ACTIVE/RESOLVED records and the addressed/reappeared/still open/new line.++ findings: ledger.findings,++ lifecycle: ledger.lifecycle,++ lifecycle_line: lifecycleLine(ledger.lifecycle),};}module.exports = {FAMILIES, HARNESS_FAMILY,-- familyOf, authorFamily, pickReviewer, reviewPrompt, readVerdict, reviewWithAi,++ familyOf, authorFamily, pickReviewer, reviewPrompt, readReview, readVerdict, reviewWithAi,};@@@ -46,6 +46,19 @@"type": "array","description": "The structured findings of the task's review, in the agent.finding@1 shape (OpenVibe.Contracts): a confirmed finding carries severity and confidence; a needs_validation lead carries a claimed cause, exact blockers and a validation plan and no severity; a rejected claim carries its reason. Empty or absent when no review produced a record.","items": { "$ref": "#/$defs/finding" }++ },++ "review": {++ "type": "object",++ "description": "The persisted review ledger: its findings across revisions with a stable finding_id and an ACTIVE/RESOLVED state, the reconciliation counts, and the addressed/reappeared/still open/new line. A review that did not run is `unreviewed` with no findings, never a zero-finding approval.",++ "additionalProperties": false,++ "required": ["verdict", "findings", "lifecycle"],++ "properties": {++ "verdict": { "type": "string", "enum": ["approved", "changes_requested", "unreviewed"] },++ "summary": { "type": "string", "maxLength": 4000 },++ "findings": { "type": "array", "maxItems": 20, "items": { "$ref": "#/$defs/reviewFinding" } },++ "lifecycle": { "$ref": "#/$defs/reviewLifecycle" },++ "lifecycle_line": { "type": ["string", "null"] }++ }}},"additionalProperties": true,@@@ -265,6 +278,30 @@"findingLevel": {"type": "string","enum": ["informational", "low", "medium", "high", "critical"]++ },++ "reviewFinding": {++ "type": "object",++ "additionalProperties": false,++ "required": ["finding_id", "file", "text", "state"],++ "properties": {++ "finding_id": { "type": "string", "minLength": 1, "maxLength": 200, "description": "Repository-relative file + whitespace-normalised, lowercased text." },++ "file": { "type": "string", "minLength": 1, "maxLength": 500 },++ "line": { "type": ["integer", "null"], "minimum": 1 },++ "text": { "type": "string", "minLength": 1, "maxLength": 2000 },++ "severity": { "type": ["string", "null"], "enum": ["low", "medium", "high", "critical", null] },++ "state": { "type": "string", "enum": ["ACTIVE", "RESOLVED"] }++ }++ },++ "reviewLifecycle": {++ "type": "object",++ "additionalProperties": false,++ "required": ["addressed", "reappeared", "still_open", "new"],++ "properties": {++ "addressed": { "type": "integer", "minimum": 0 },++ "reappeared": { "type": "integer", "minimum": 0 },++ "still_open": { "type": "integer", "minimum": 0 },++ "new": { "type": "integer", "minimum": 0 }++ }}}}@@@ -819,7 +819,7 @@ async function runJob({ input, harness, emit, writeResult, env = process.env, tmauthor: { agent: job.agent, model: job.model },emit: (item) => say({ t: 'proof', kind: item.kind, label: item.label, ok: item.ok, skipped: !!item.skipped, summary: item.summary, family: item.family || null, model: item.model || null }),});-- result.proof = { items: collected.items.map((i) => ({ kind: i.kind, label: i.label, ok: i.ok, skipped: !!i.skipped, summary: i.summary, family: i.family || null, model: i.model || null, requested_model: i.requested_model || null, detail: i.shots ? { shots: i.shots.map((s) => shotWithBytes(s, pictures)) } : (i.kind === 'diff' ? { files: i.files, stats: i.stats } : null) })), outcome: collected.outcome };++ result.proof = { items: collected.items.map((i) => ({ kind: i.kind, label: i.label, ok: i.ok, skipped: !!i.skipped, summary: i.summary, family: i.family || null, model: i.model || null, requested_model: i.requested_model || null, detail: i.shots ? { shots: i.shots.map((s) => shotWithBytes(s, pictures)) } : (i.kind === 'diff' ? { files: i.files, stats: i.stats } : null) })), outcome: collected.outcome, review: collected.review || null };} catch (e) {say({ t: 'note', level: 'warning', text: `The proof could not be collected: ${scrub(e.message)}` });result.proof = { items: [], outcome: null };@@@ -275,7 +275,7 @@ function runTokenOf(jobEnv = {}) {* says so rather than pretending one happened.*/function reviewAdapter({ env = {}, jobEnv = {}, fetchImpl = globalThis.fetch, baseUrl, models = [] } = {}) {-- return async function review({ patch = '', summary = '', tests = null, model = null, family = null } = {}) {++ return async function review({ patch = '', summary = '', tests = null, model = null, family = null, prior = [] } = {}) {// The token the attempt's own harness carries: only the job's own input, under the name C3 gave that harness.// The machine's own OPENVIBE_AI_TOKEN names its owner, not this task's person, so it is never spent here.const token = runTokenOf(jobEnv);@@@ -292,7 +292,7 @@ function reviewAdapter({ env = {}, jobEnv = {}, fetchImpl = globalThis.fetch, ba}return await reviewWithAi({baseUrl: baseUrl || aiRoot(jobEnv) || env.OPENVIBE_AI_URL || 'https://ai.openvibe.services/v1',-- token, model: chosen, family: familyOf(chosen), patch, summary, tests, fetchImpl,++ token, model: chosen, family: familyOf(chosen), patch, summary, tests, prior, fetchImpl,});};}nnew file mode 100644@@@ -0,0 +1,77 @@++'use strict';++// The persisted review ledger: stable ids, ACTIVE/RESOLVED, and the addressed/reappeared/still open/new line.++++const assert = require('node:assert/strict');++const fs = require('node:fs');++const path = require('node:path');++const { check, done } = require('./helpers/boot');++const F = require('../engine/findings');++const { findingId } = require('../engine/finding');++++const json = (findings, verdict = 'changes_requested') => JSON.stringify({ verdict, summary: 'read it', findings });++++(async () => {++ await check('a JSON review yields records with the stable id, and the cap is three', () => {++ const recs = F.parse(json([++ { file: 'server/x.js', line: 31, text: 'The cache NEVER expires', severity: 'high' },++ { file: 'server/y.js', line: 5, text: 'The retry debits twice' },++ { file: 'server/z.js', text: 'No test reaches this' },++ { file: 'server/ignored.js', text: 'Over the quiet cap' },++ ]));++ assert.equal(recs.length, 3);++ assert.equal(recs[0].finding_id, findingId('server/x.js', 'The cache never expires'));++ assert.deepEqual([recs[0].state, recs[0].severity], ['ACTIVE', 'high']);++ assert.equal(recs[2].line, null);++ });++++ await check('the legacy VERDICT bullets are read too, so an older reviewer keeps its ids', () => {++ const recs = F.parse('VERDICT: FIX\n1. server/x.js:31 it races with the writer\n2. a design note\n');++ assert.equal(recs.length, 1);++ assert.equal(recs[0].file, 'server/x.js');++ assert.equal(recs[0].line, 31);++ assert.match(recs[0].text, /races/);++ });++++ await check('a second review reconciles: addressed, reappeared, still open and new', () => {++ const first = F.fromReview(json([++ { file: 'a.js', line: 1, text: 'the cache never expires' },++ { file: 'b.js', line: 2, text: 'the retry debits twice' },++ ]));++ assert.deepEqual(first.lifecycle, { addressed: 0, reappeared: 0, still_open: 0, new: 2 });++ assert.equal(first.line, 'Findings: 0 addressed, 0 reappeared, 0 still open, 2 new');++ const second = F.fromReview(json([++ { file: 'a.js', line: 1, text: 'The cache never expires' },++ { file: 'c.js', line: 3, text: 'a new leak' },++ ], 'changes_requested'), first.findings);++ assert.deepEqual(second.lifecycle, { addressed: 1, reappeared: 0, still_open: 1, new: 1 });++ assert.equal(second.line, 'Findings: 1 addressed, 0 reappeared, 1 still open, 1 new');++ const cache = second.findings.find((f) => f.file === 'a.js'), retry = second.findings.find((f) => f.file === 'b.js');++ assert.equal(cache.state, 'ACTIVE');++ assert.equal(retry.state, 'RESOLVED');++ });++++ await check('a resolved finding that comes back has reappeared, and an approval resolves what it no longer raises', () => {++ const one = F.fromReview(json([{ file: 'a.js', line: 1, text: 'the cache never expires' }]));++ const fixed = F.fromReview(json([], 'approved'), one.findings);++ assert.deepEqual(fixed.lifecycle, { addressed: 1, reappeared: 0, still_open: 0, new: 0 });++ assert.equal(fixed.findings[0].state, 'RESOLVED');++ const back = F.fromReview(json([{ file: 'a.js', line: 1, text: 'the cache never expires' }]), fixed.findings);++ assert.deepEqual(back.lifecycle, { addressed: 0, reappeared: 1, still_open: 0, new: 0 });++ assert.equal(back.findings[0].state, 'ACTIVE');++ });++++ await check('no prior and no current findings says nothing, and the prior brief is compact', () => {++ assert.equal(F.fromReview('{"verdict":"approved","findings":[]}').line, null);++ assert.equal(F.priorBrief([]), '');++ const recs = F.fromReview(json([{ file: 'a.js', line: 1, text: 'the cache never expires' }])).findings;++ assert.match(F.priorBrief(recs), /\[ACTIVE\] a\.js:1 — the cache never expires \(id a\.js:the cache never expires\)/);++ });++++ await check('the transcript schema publishes the persisted review shape', () => {++ const schema = JSON.parse(fs.readFileSync(path.join(__dirname, '..', 'engine', 'transcript.schema.json'), 'utf8'));++ assert.deepEqual(schema.properties.review.required, ['verdict', 'findings', 'lifecycle']);++ assert.deepEqual(schema.$defs.reviewFinding.properties.state.enum, ['ACTIVE', 'RESOLVED']);++ assert.deepEqual(schema.$defs.reviewLifecycle.required, ['addressed', 'reappeared', 'still_open', 'new']);++ });++ done();++})();@@@ -9,7 +9,7 @@*/const assert = require('assert');const {-- familyOf, authorFamily, pickReviewer, reviewPrompt, readVerdict, reviewWithAi,++ familyOf, authorFamily, pickReviewer, reviewPrompt, readReview, readVerdict, reviewWithAi,} = require('../engine/review');const { check, done } = require('./helpers/boot');@@@ -49,6 +49,41 @@ const OK = async () => ({ status: 200, ok: true, json: async () => ({ choices: [assert.ok(/approved/.test(prompt) && /changes_requested/.test(prompt), 'the verdict is asked for');});++ await check('quiet by default: only this diff, at most three, no style, and a prior ledger on a re-review', () => {++ const prompt = reviewPrompt({ patch: '+x', summary: 'add x' });++ assert.match(prompt, /only issues this change introduces/i);++ assert.match(prompt, /at most 3 findings/i);++ assert.match(prompt, /No style nits/i);++ assert.match(prompt, /empty findings list/i);++ const prior = [{ finding_id: 'a.js:it races', file: 'a.js', line: 1, text: 'it races', state: 'ACTIVE', severity: 'high' }];++ const again = reviewPrompt({ patch: '+y', prior });++ assert.match(again, /previous review of an earlier revision/i);++ assert.match(again, /a\.js:1 — it races \(id a\.js:it races\)/);++ });++++ await check('findings are read from JSON findings and from the legacy numbered bullets', () => {++ const doc = readReview('{"verdict":"changes_requested","summary":"one bug","findings":[{"file":"a.js","line":2,"text":"it races","severity":"high"}]}');++ assert.deepEqual([doc.verdict, doc.findings.length, doc.findings[0].file], ['changes_requested', 1, 'a.js']);++ const prose = readReview('VERDICT: FIX\n1. a.js:2 it races with the writer');++ assert.equal(prose.findings.length, 1);++ assert.equal(prose.findings[0].line, 2);++ assert.deepEqual(readReview('{"verdict":"approved","summary":"ok","findings":[]}').findings, []);++ });++++ await check('a re-review reconciles its findings and keeps a dead reviewer\'s ledger', async () => {++ const prior = [{ finding_id: 'a.js:it races', file: 'a.js', line: 1, text: 'it races', state: 'ACTIVE', severity: 'high' }];++ const approved = async () => ({ status: 200, ok: true, json: async () => ({ choices: [{ message: { content: '{"verdict":"approved","summary":"fixed","findings":[]}' } }] }) });++ const fixed = await reviewWithAi({ token: 't', model: 'deepseek-chat', patch: '+x', prior, fetchImpl: approved });++ assert.deepEqual(fixed.lifecycle, { addressed: 1, reappeared: 0, still_open: 0, new: 0 });++ assert.strictEqual(fixed.lifecycle_line, 'Findings: 1 addressed, 0 reappeared, 0 still open, 0 new');++ assert.strictEqual(fixed.findings[0].state, 'RESOLVED');++ // A reviewer that says nothing reviewed nothing: the prior ledger stays ACTIVE, not silently addressed.++ const silent = async () => ({ status: 200, ok: true, json: async () => ({ choices: [{ message: { content: '' } }] }) });++ const kept = await reviewWithAi({ token: 't', model: 'deepseek-chat', patch: '+x', prior, fetchImpl: silent });++ assert.strictEqual(kept.lifecycle, null);++ assert.strictEqual(kept.findings[0].state, 'ACTIVE');++ });++await check('the review is one chat completion, with the token, read as a verdict', async () => {const calls = [];const fetchImpl = async (url, options) => { calls.push({ url, options }); return OK(); };@@@ -62,6 +97,7 @@ const OK = async () => ({ status: 200, ok: true, json: async () => ({ choices: [assert.strictEqual(JSON.parse(calls[0].options.body).model, 'deepseek-chat');assert.strictEqual(verdict.verdict, 'approved');assert.strictEqual(verdict.family, 'deepseek');++ assert.deepEqual(verdict.findings, [], 'an approval is an empty findings list, not a "no issues" note');});await check('the review records the resolved model and cost from the API\'s disclosure', async () => {
@@ -251,6 +251,22 @@ validate({ verdict: 'confirmed', /* … */ });
@@ -251,6 +251,22 @@ validate({ verdict: 'confirmed', /* … */ });
isLead({ verdict: 'needs_validation' }); // true
isLead({ verdict: 'needs_validation' }); // true
```
```
+### `findings.js` — a review that remembers what it already said
+
+The review's ledger across revisions. `parse(text)` reads the findings of a JSON answer (or the older numbered
+`VERDICT: FIX` bullets); `merge(prior, current)` reconciles them into ACTIVE/RESOLVED records and counts what was
+addressed, reappeared, is still open and is new; `priorBrief(records)` is the compact block a re-review prompt gets;
+`lifecycleLine()` is the one line a person reads. The resolved set is capped, and an approval is an empty list — no
+synthetic "no issues" finding.
+
+```js
+const { fromReview, priorBrief, lifecycleLine } = require('./engine/findings');
+
+const first = fromReview('{"verdict":"changes_requested","findings":[{"file":"a.js","line":1,"text":"it races"}]}');
+const again = fromReview('{"verdict":"approved","findings":[]}', first.findings);
+// again.line = 'Findings: 1 addressed, 0 reappeared, 0 still open, 0 new'
+```
+
## Tests
## Tests
`test/engine-*.test.js` in the Codes repo, run by the repo's own runner (`npm test`).
`test/engine-*.test.js` in the Codes repo, run by the repo's own runner (`npm test`).
new file mode 100644
new file mode 100644
@@ -0,0 +1,151 @@
@@ -0,0 +1,151 @@
+'use strict';
+
+/**
+ * A review's findings across revisions: the stable id, the ACTIVE/RESOLVED lifecycle, and the line a person reads.
+ *
+ * A finding is identified by the repository-relative file and the whitespace-normalised, lowercased text
+ * (`engine/finding.js`'s findingId), so re-wrapping the same sentence keeps its id while a genuine rewording becomes
+ * a new finding. The first review creates ACTIVE records; a later review is given the prior records and reconciles
+ * them: a prior finding that is no longer raised is addressed (RESOLVED), a prior RESOLVED one that comes back has
+ * reappeared (ACTIVE), a prior ACTIVE one still raised is still open, and anything new is new. The resolved set is
+ * capped, oldest first, because a long review history is not worth its prompt tokens.
+ *
+ * parse(text) the findings in a review's answer (JSON findings or legacy numbered VERDICT bullets)
+ * merge(prior, current) { findings, lifecycle, line } — the merged lifecycle
+ * fromReview(text, prior) parse + merge
+ * priorBrief(records) the compact block a re-review prompt gets instead of the whole earlier answer
+ * lifecycleLine(lifecycle) "Findings: 1 addressed, 0 reappeared, 2 still open, 1 new"
+ * STATE, MAX_FINDINGS, RESOLVED_CAP
+ *
+ * Quiet by default lives in the review prompt (engine/review.js) and in this cap: at most three findings, and no
+ * synthetic "no issues" finding — an approval is an empty list, not a comment.
+ */
+
+const { findingId } = require('./finding');
+
+const STATE = { ACTIVE: 'ACTIVE', RESOLVED: 'RESOLVED' };
+const MAX_FINDINGS = 3;
+const RESOLVED_CAP = 20;
+const SEVERITIES = ['low', 'medium', 'high', 'critical'];
+
+/** One finding, from what a reviewer said. A record without a file and words has no identity and is dropped. */
+function record(item) {
+ if (!item || typeof item !== 'object') return null;
+ const file = String(item.file || '').trim().replace(/^\.\//, '');
+ const text = String(item.text || item.description || item.summary || '').trim();
+ if (!file || !text) return null;
+ const line = Number.isInteger(item.line) && item.line > 0 ? item.line : null;
+ const severity = SEVERITIES.includes(String(item.severity || '').toLowerCase()) ? String(item.severity).toLowerCase() : null;
+ return {
+ finding_id: findingId(file, text),
+ file,
+ line,
+ text: text.slice(0, 2000),
+ severity,
+ state: STATE.ACTIVE,
+ };
+}
+
+/** The JSON object in an answer, when it wrote one (the prompt asks for JSON). */
+function jsonOf(text) {
+ const raw = String(text || '').trim();
+ const start = raw.indexOf('{'), end = raw.lastIndexOf('}');
+ if (start < 0 || end <= start) return null;
+ try { return JSON.parse(raw.slice(start, end + 1)); } catch { return null; }
+}
+
+/** A file:line at the start of a legacy numbered bullet. */
+const BULLET_FILE = /^([\w./-]+\.[A-Za-z0-9]+):(\d+)\s*(.*)$/;
+const BULLET_FILE_NO_LINE = /^([\w./-]+\.[A-Za-z0-9]+)\s*(?:[—:-]\s*)?(.*)$/;
+
+/**
+ * The findings in a review answer: the JSON `findings` array when the reviewer wrote one, else the numbered bullets
+ * of the older `VERDICT: FIX` protocol ("1. server/x.js:31 the cache never expires").
+ *
+ * @param {string} text the review
+ * @returns {object[]} the records, capped at MAX_FINDINGS
+ */
+function parse(text) {
+ const doc = jsonOf(text);
+ if (doc && Array.isArray(doc.findings)) {
+ return doc.findings.map(record).filter(Boolean).slice(0, MAX_FINDINGS);
+ }
+ const out = [];
+ const lines = String(text || '').split('\n');
+ const start = lines.findIndex((l) => /^\W*VERDICT:\s*(SHIP|FIX)/i.test(l));
+ for (const line of lines.slice(start < 0 ? 0 : start + 1)) {
+ const m = /^\s*\d+[.)]\s+(.*)$/.exec(line);
+ if (!m) continue;
+ const body = m[1].trim();
+ const withLine = BULLET_FILE.exec(body), withoutLine = BULLET_FILE_NO_LINE.exec(body);
+ const hit = withLine || withoutLine;
+ if (!hit) continue;
+ const rec = record({ file: hit[1], line: withLine ? +hit[2] : null, text: withLine ? hit[3] : hit[2] });
+ if (rec) out.push(rec);
+ if (out.length >= MAX_FINDINGS) break;
+ }
+ return out;
+}
+
+/** The line a person reads at the top of a re-review, or null when there is nothing to say. */
+function lifecycleLine(lifecycle) {
+ if (!lifecycle) return null;
+ const { addressed = 0, reappeared = 0, still_open = 0, new: fresh = 0 } = lifecycle;
+ if (!addressed && !reappeared && !still_open && !fresh) return null;
+ return `Findings: ${addressed} addressed, ${reappeared} reappeared, ${still_open} still open, ${fresh} new`;
+}
+
+/**
+ * Reconcile this review's findings with the prior ledger.
+ *
+ * @param {object[]} prior the earlier records (ACTIVE and RESOLVED)
+ * @param {object[]} current this review's records
+ * @returns {{ findings: object[], lifecycle: object, line: string|null }} the merged ledger and its line
+ */
+function merge(prior = [], current = []) {
+ const prev = Array.isArray(prior) ? prior.filter((p) => p && p.finding_id) : [];
+ const cur = Array.isArray(current) ? current.filter((c) => c && c.finding_id) : [];
+ const byId = new Map(prev.map((p) => [p.finding_id, p]));
+ const seen = new Set();
+ const lifecycle = { addressed: 0, reappeared: 0, still_open: 0, new: 0 };
+ const findings = [];
+ for (const c of cur) {
+ if (seen.has(c.finding_id)) continue;
+ seen.add(c.finding_id);
+ const p = byId.get(c.finding_id);
+ if (!p) lifecycle.new++;
+ else if (p.state === STATE.RESOLVED) lifecycle.reappeared++;
+ else lifecycle.still_open++;
+ findings.push({ ...c, state: STATE.ACTIVE });
+ }
+ for (const p of prev) {
+ if (p.state === STATE.RESOLVED || seen.has(p.finding_id)) continue;
+ lifecycle.addressed++;
+ findings.push({ ...p, state: STATE.RESOLVED });
+ }
+ // Keep the ledger readable: the oldest resolved findings leave first.
+ const resolved = findings.filter((f) => f.state === STATE.RESOLVED);
+ if (resolved.length > RESOLVED_CAP) {
+ const drop = new Set(resolved.slice(0, resolved.length - RESOLVED_CAP).map((f) => f.finding_id));
+ return { findings: findings.filter((f) => !drop.has(f.finding_id)), lifecycle, line: lifecycleLine(lifecycle) };
+ }
+ return { findings, lifecycle, line: lifecycleLine(lifecycle) };
+}
+
+/** Parse this review and reconcile it with the prior ledger. */
+function fromReview(text, prior = []) {
+ return merge(prior, parse(text));
+}
+
+/** The compact block a re-review prompt gets: the ids and words, so the reviewer can tell addressed from reworded. */
+function priorBrief(records, limit = 8000) {
+ const list = Array.isArray(records) ? records.filter((r) => r && r.finding_id) : [];
+ if (!list.length) return '';
+ const lines = list.slice(0, 20).map((r) => `- [${r.state}] ${r.file}${r.line ? ':' + r.line : ''} — ${r.text} (id ${r.finding_id})`);
+ return lines.join('\n').slice(0, limit);
+}
+
+module.exports = {
+ STATE, MAX_FINDINGS, RESOLVED_CAP,
+ record, parse, merge, fromReview, priorBrief, lifecycleLine,
+};
@@ -20,6 +20,7 @@
@@ -20,6 +20,7 @@
* engine.proof the proof of a finished attempt: its tests, its pages, its diff, and a second opinion
* engine.proof the proof of a finished attempt: its tests, its pages, its diff, and a second opinion
* engine.review the second opinion itself: a model of another family, through OpenVibe.AI
* engine.review the second opinion itself: a model of another family, through OpenVibe.AI
* engine.finding one finding in the agent.finding@1 shape: confirmed, a lead that needs validation, or rejected
* engine.finding one finding in the agent.finding@1 shape: confirmed, a lead that needs validation, or rejected
+ * engine.findings a review's findings across revisions: stable ids, ACTIVE/RESOLVED and the lifecycle line
*
*
* The modules do not depend on each other in a cycle, and none of them reads the portal's log folder.
* The modules do not depend on each other in a cycle, and none of them reads the portal's log folder.
*/
*/
@@ -41,6 +42,7 @@ const redact = require('./redact');
@@ -41,6 +42,7 @@ const redact = require('./redact');
const proof = require('./proof');
const proof = require('./proof');
const review = require('./review');
const review = require('./review');
const finding = require('./finding');
const finding = require('./finding');
+const findings = require('./findings');
const goal = require('./goal');
const goal = require('./goal');
module.exports = {
module.exports = {
@@ -61,6 +63,7 @@ module.exports = {
@@ -61,6 +63,7 @@ module.exports = {
proof,
proof,
review,
review,
finding,
finding,
+ findings,
goal,
goal,
// The names a caller usually wants, in one place.
// The names a caller usually wants, in one place.
classify: router.classify,
classify: router.classify,
@@ -296,7 +296,8 @@ async function collectProof({ cwd, files = [], adapters = {}, author = null, emi
@@ -296,7 +296,8 @@ async function collectProof({ cwd, files = [], adapters = {}, author = null, emi
await say({
await say({
kind: 'review', label: 'review', ok: verdict.verdict === 'approved', skipped: false,
kind: 'review', label: 'review', ok: verdict.verdict === 'approved', skipped: false,
family: verdict.family, model: verdict.model,
family: verdict.family, model: verdict.model,
- summary: verdict.summary || (verdict.verdict === 'approved' ? 'approved' : 'changes requested'),
+ summary: [verdict.summary || (verdict.verdict === 'approved' ? 'approved' : 'changes requested'), verdict.lifecycle_line]
+ .filter(Boolean).join(' · '),
});
});
}
}
}
}
@@ -14,13 +14,18 @@
@@ -14,13 +14,18 @@
* familyOf(model) the family a model belongs to ('anthropic', 'openai', 'deepseek', …)
* familyOf(model) the family a model belongs to ('anthropic', 'openai', 'deepseek', …)
* authorFamily({ agent, model }) the family that wrote the change
* authorFamily({ agent, model }) the family that wrote the change
* pickReviewer({ models, author }) a model of another family, or null when there is none
* pickReviewer({ models, author }) a model of another family, or null when there is none
- * reviewWithAi({ … }) the review as { family, verdict, summary }
+ * reviewWithAi({ … }) the review as { family, verdict, summary, findings, lifecycle, lifecycle_line }
+ * readReview(text) { verdict, summary, findings } from JSON or from the words
*
*
- * The verdict is 'approved' or 'changes_requested'. The model is asked for JSON; a model that answers in prose is
- * read by its words ('approved', 'looks good' → approved; 'changes', 'needs work' → changes_requested), because a
- * review that cannot be read is not a review.
+ * The verdict is 'approved' or 'changes_requested'. The model is asked for JSON with its findings; a model that answers
+ * in prose is read by its words ('approved', 'looks good' → approved; 'changes', 'needs work' → changes_requested),
+ * because a review that cannot be read is not a review. Quiet by default: only issues this diff introduces, at most
+ * three, no style, no speculation, and no synthetic "no issues" finding. Re-review is given the prior ledger so a
+ * finding is addressed, reappeared, still open or new rather than lost in a fresh paragraph.
*/
*/
+const { MAX_FINDINGS, priorBrief, fromReview, lifecycleLine, parse: parseFindings } = require('./findings');
+
/** The families this knows, by the model's own name. */
/** The families this knows, by the model's own name. */
const FAMILIES = [
const FAMILIES = [
{ family: 'anthropic', re: /claude/i },
{ family: 'anthropic', re: /claude/i },
@@ -85,14 +90,23 @@ function pickReviewer({ models = [], author = null } = {}) {
@@ -85,14 +90,23 @@ function pickReviewer({ models = [], author = null } = {}) {
return null;
return null;
}
}
-/** The prompt the reviewer gets: the change, the tests it claims to pass, and what is being asked of it. */
-function reviewPrompt({ patch = '', summary = '', tests = null } = {}) {
+/**
+ * The prompt the reviewer gets: the change, what it was for, the tests, and the prior findings on a re-review.
+ * Quiet by default — the rules are the point: only what this diff introduces, at most three findings, no style, no
+ * speculation, and an approval is an empty list rather than a "no issues" note.
+ */
+function reviewPrompt({ patch = '', summary = '', tests = null, prior = [] } = {}) {
+ const before = priorBrief(prior);
return [
return [
'You are reviewing a change to a repository. Answer only with JSON:',
'You are reviewing a change to a repository. Answer only with JSON:',
- '{"verdict":"approved"|"changes_requested","summary":"one paragraph, what you checked and what you found"}.',
- 'Answer "approved" when the change does what it says and holds together; "changes_requested" when it is wrong,',
- 'unsafe, or leaves the work unfinished. Be concrete: name the file and the line.',
+ `{"verdict":"approved"|"changes_requested","summary":"one sentence, what you checked","findings":[{"file":"path","line":12,"text":"what is wrong and the concrete fix","severity":"low"|"medium"|"high"|"critical"}]}`,
+ 'Flag only issues this change introduces: a correctness bug, a security problem, a missing or broken test, or a',
+ `requirement the change leaves undone. At most ${MAX_FINDINGS} findings, worst first. No style nits, no "may be`,
+ 'unsafe" without the path that reaches it, and no re-raising a design choice the change was asked to make. If a',
+ 'high-impact point is plausible but not proven, say "uncertain" in its text and keep the severity low. If nothing',
+ 'is wrong, "approved" with an empty findings list; never write a "no issues" finding or a comment.',
'',
'',
+ before ? 'A previous review of an earlier revision raised the findings below. Treat them as the ledger: say in your\nsummary which are addressed, and report one again only when it still stands in this revision; then add only new\nissues this revision introduced. The ids stay stable across re-wrapping.\n' + before : '',
summary ? 'What the change was asked to do:\n' + String(summary).slice(0, 4000) : '',
summary ? 'What the change was asked to do:\n' + String(summary).slice(0, 4000) : '',
tests ? 'The tests the author says pass:\n' + String(tests).slice(0, 2000) : '',
tests ? 'The tests the author says pass:\n' + String(tests).slice(0, 2000) : '',
'',
'',
@@ -100,23 +114,34 @@ function reviewPrompt({ patch = '', summary = '', tests = null } = {}) {
@@ -100,23 +114,34 @@ function reviewPrompt({ patch = '', summary = '', tests = null } = {}) {
].filter(Boolean).join('\n');
].filter(Boolean).join('\n');
}
}
-/** The answer as JSON, or as the words in it. */
-function readVerdict(text) {
+/** The answer as JSON, or as the words in it, with the findings it raised (an empty list on approval). */
+function readReview(text) {
const raw = String(text || '').trim();
const raw = String(text || '').trim();
- if (!raw) return { verdict: 'changes_requested', summary: 'the reviewer said nothing' };
+ if (!raw) return { verdict: 'changes_requested', summary: 'the reviewer said nothing', findings: [] };
const start = raw.indexOf('{');
const start = raw.indexOf('{');
const end = raw.lastIndexOf('}');
const end = raw.lastIndexOf('}');
if (start >= 0 && end > start) {
if (start >= 0 && end > start) {
try {
try {
const doc = JSON.parse(raw.slice(start, end + 1));
const doc = JSON.parse(raw.slice(start, end + 1));
+ const findings = Array.isArray(doc.findings) ? doc.findings.map((f) => ({
+ file: String(f.file || '').trim(), line: Number.isInteger(f.line) ? f.line : null,
+ text: String(f.text || f.description || '').trim(), severity: f.severity || null,
+ })).filter((f) => f.file && f.text).slice(0, MAX_FINDINGS) : [];
const verdict = String(doc.verdict || '').toLowerCase();
const verdict = String(doc.verdict || '').toLowerCase();
- if (/approv|looks good|lgtm/.test(verdict)) return { verdict: 'approved', summary: String(doc.summary || raw).slice(0, 4000) };
- if (/change|reject|not good/.test(verdict)) return { verdict: 'changes_requested', summary: String(doc.summary || raw).slice(0, 4000) };
- if (doc.summary) return { verdict: 'changes_requested', summary: String(doc.summary).slice(0, 4000) };
+ if (/approv|looks good|lgtm/.test(verdict)) return { verdict: 'approved', summary: String(doc.summary || raw).slice(0, 4000), findings };
+ if (/change|reject|not good/.test(verdict)) return { verdict: 'changes_requested', summary: String(doc.summary || raw).slice(0, 4000), findings };
+ if (doc.summary) return { verdict: 'changes_requested', summary: String(doc.summary).slice(0, 4000), findings };
} catch { /* not JSON: read the words */ }
} catch { /* not JSON: read the words */ }
}
}
- if (/\bapprov|looks good|looks fine|lgtm|no changes\b/i.test(raw)) return { verdict: 'approved', summary: raw.slice(0, 4000) };
- return { verdict: 'changes_requested', summary: raw.slice(0, 4000) };
+ const findings = parseFindings(raw);
+ if (/\bapprov|looks good|looks fine|lgtm|no changes\b/i.test(raw)) return { verdict: 'approved', summary: raw.slice(0, 4000), findings };
+ return { verdict: 'changes_requested', summary: raw.slice(0, 4000), findings };
+}
+
+/** The verdict alone (the older callers). */
+function readVerdict(text) {
+ const read = readReview(text);
+ return { verdict: read.verdict, summary: read.summary };
}
}
/**
/**
@@ -140,12 +165,13 @@ async function reviewWithAi({
@@ -140,12 +165,13 @@ async function reviewWithAi({
patch = '',
patch = '',
summary = '',
summary = '',
tests = null,
tests = null,
+ prior = [],
fetchImpl = globalThis.fetch,
fetchImpl = globalThis.fetch,
} = {}) {
} = {}) {
if (!token) throw new Error('the review needs the task\'s token');
if (!token) throw new Error('the review needs the task\'s token');
if (!model) throw new Error('the review needs a model');
if (!model) throw new Error('the review needs a model');
const root = String(baseUrl).replace(/\/+$/, '');
const root = String(baseUrl).replace(/\/+$/, '');
- const prompt = reviewPrompt({ patch, summary, tests });
+ const prompt = reviewPrompt({ patch, summary, tests, prior });
const headers = { 'content-type': 'application/json', authorization: `Bearer ${token}` };
const headers = { 'content-type': 'application/json', authorization: `Bearer ${token}` };
let res = await fetchImpl(`${root}/chat/completions`, {
let res = await fetchImpl(`${root}/chat/completions`, {
method: 'POST', headers, body: JSON.stringify({ model, messages: [{ role: 'user', content: prompt }] }),
method: 'POST', headers, body: JSON.stringify({ model, messages: [{ role: 'user', content: prompt }] }),
@@ -170,7 +196,9 @@ async function reviewWithAi({
@@ -170,7 +196,9 @@ async function reviewWithAi({
const text = body && body.choices && body.choices[0] && body.choices[0].message && body.choices[0].message.content
const text = body && body.choices && body.choices[0] && body.choices[0].message && body.choices[0].message.content
? body.choices[0].message.content
? body.choices[0].message.content
: (body && body.content && Array.isArray(body.content) ? body.content.map((c) => c && c.text).filter(Boolean).join(' ') : '');
: (body && body.content && Array.isArray(body.content) ? body.content.map((c) => c && c.text).filter(Boolean).join(' ') : '');
- const verdict = readVerdict(text);
+ const read = readReview(text);
+ // A reviewer that said nothing reviewed nothing: its prior findings are kept, never silently marked addressed.
+ const ledger = String(text || '').trim() ? fromReview(text, prior) : { findings: Array.isArray(prior) ? prior : [], lifecycle: null, line: null };
// The route disclosure (D3'): the model that actually ran, the deployment and the cost, inside x_openvibe.
// The route disclosure (D3'): the model that actually ran, the deployment and the cost, inside x_openvibe.
const x = body && body.x_openvibe && typeof body.x_openvibe === 'object' ? body.x_openvibe : null;
const x = body && body.x_openvibe && typeof body.x_openvibe === 'object' ? body.x_openvibe : null;
const resolved = x && x.resolved_model ? String(x.resolved_model) : (body && body.model ? String(body.model) : model);
const resolved = x && x.resolved_model ? String(x.resolved_model) : (body && body.model ? String(body.model) : model);
@@ -181,12 +209,16 @@ async function reviewWithAi({
@@ -181,12 +209,16 @@ async function reviewWithAi({
...(x && x.resolved_model ? { resolved_model: String(x.resolved_model) } : {}),
...(x && x.resolved_model ? { resolved_model: String(x.resolved_model) } : {}),
...(x ? { disclosure: x } : {}),
...(x ? { disclosure: x } : {}),
...(x && Number.isFinite(+x.cost_usd) ? { cost_usd: +x.cost_usd } : {}),
...(x && Number.isFinite(+x.cost_usd) ? { cost_usd: +x.cost_usd } : {}),
- verdict: verdict.verdict,
- summary: verdict.summary,
+ verdict: read.verdict,
+ summary: read.summary,
+ // The persisted lifecycle: ACTIVE/RESOLVED records and the addressed/reappeared/still open/new line.
+ findings: ledger.findings,
+ lifecycle: ledger.lifecycle,
+ lifecycle_line: lifecycleLine(ledger.lifecycle),
};
};
}
}
module.exports = {
module.exports = {
FAMILIES, HARNESS_FAMILY,
FAMILIES, HARNESS_FAMILY,
- familyOf, authorFamily, pickReviewer, reviewPrompt, readVerdict, reviewWithAi,
+ familyOf, authorFamily, pickReviewer, reviewPrompt, readReview, readVerdict, reviewWithAi,
};
};
@@ -46,6 +46,19 @@
@@ -46,6 +46,19 @@
"type": "array",
"type": "array",
"description": "The structured findings of the task's review, in the agent.finding@1 shape (OpenVibe.Contracts): a confirmed finding carries severity and confidence; a needs_validation lead carries a claimed cause, exact blockers and a validation plan and no severity; a rejected claim carries its reason. Empty or absent when no review produced a record.",
"description": "The structured findings of the task's review, in the agent.finding@1 shape (OpenVibe.Contracts): a confirmed finding carries severity and confidence; a needs_validation lead carries a claimed cause, exact blockers and a validation plan and no severity; a rejected claim carries its reason. Empty or absent when no review produced a record.",
"items": { "$ref": "#/$defs/finding" }
"items": { "$ref": "#/$defs/finding" }
+ },
+ "review": {
+ "type": "object",
+ "description": "The persisted review ledger: its findings across revisions with a stable finding_id and an ACTIVE/RESOLVED state, the reconciliation counts, and the addressed/reappeared/still open/new line. A review that did not run is `unreviewed` with no findings, never a zero-finding approval.",
+ "additionalProperties": false,
+ "required": ["verdict", "findings", "lifecycle"],
+ "properties": {
+ "verdict": { "type": "string", "enum": ["approved", "changes_requested", "unreviewed"] },
+ "summary": { "type": "string", "maxLength": 4000 },
+ "findings": { "type": "array", "maxItems": 20, "items": { "$ref": "#/$defs/reviewFinding" } },
+ "lifecycle": { "$ref": "#/$defs/reviewLifecycle" },
+ "lifecycle_line": { "type": ["string", "null"] }
+ }
}
}
},
},
"additionalProperties": true,
"additionalProperties": true,
@@ -265,6 +278,30 @@
@@ -265,6 +278,30 @@
"findingLevel": {
"findingLevel": {
"type": "string",
"type": "string",
"enum": ["informational", "low", "medium", "high", "critical"]
"enum": ["informational", "low", "medium", "high", "critical"]
+ },
+ "reviewFinding": {
+ "type": "object",
+ "additionalProperties": false,
+ "required": ["finding_id", "file", "text", "state"],
+ "properties": {
+ "finding_id": { "type": "string", "minLength": 1, "maxLength": 200, "description": "Repository-relative file + whitespace-normalised, lowercased text." },
+ "file": { "type": "string", "minLength": 1, "maxLength": 500 },
+ "line": { "type": ["integer", "null"], "minimum": 1 },
+ "text": { "type": "string", "minLength": 1, "maxLength": 2000 },
+ "severity": { "type": ["string", "null"], "enum": ["low", "medium", "high", "critical", null] },
+ "state": { "type": "string", "enum": ["ACTIVE", "RESOLVED"] }
+ }
+ },
+ "reviewLifecycle": {
+ "type": "object",
+ "additionalProperties": false,
+ "required": ["addressed", "reappeared", "still_open", "new"],
+ "properties": {
+ "addressed": { "type": "integer", "minimum": 0 },
+ "reappeared": { "type": "integer", "minimum": 0 },
+ "still_open": { "type": "integer", "minimum": 0 },
+ "new": { "type": "integer", "minimum": 0 }
+ }
}
}
}
}
}
}
@@ -819,7 +819,7 @@ async function runJob({ input, harness, emit, writeResult, env = process.env, tm
@@ -819,7 +819,7 @@ async function runJob({ input, harness, emit, writeResult, env = process.env, tm
author: { agent: job.agent, model: job.model },
author: { agent: job.agent, model: job.model },
emit: (item) => say({ t: 'proof', kind: item.kind, label: item.label, ok: item.ok, skipped: !!item.skipped, summary: item.summary, family: item.family || null, model: item.model || null }),
emit: (item) => say({ t: 'proof', kind: item.kind, label: item.label, ok: item.ok, skipped: !!item.skipped, summary: item.summary, family: item.family || null, model: item.model || null }),
});
});
- result.proof = { items: collected.items.map((i) => ({ kind: i.kind, label: i.label, ok: i.ok, skipped: !!i.skipped, summary: i.summary, family: i.family || null, model: i.model || null, requested_model: i.requested_model || null, detail: i.shots ? { shots: i.shots.map((s) => shotWithBytes(s, pictures)) } : (i.kind === 'diff' ? { files: i.files, stats: i.stats } : null) })), outcome: collected.outcome };
+ result.proof = { items: collected.items.map((i) => ({ kind: i.kind, label: i.label, ok: i.ok, skipped: !!i.skipped, summary: i.summary, family: i.family || null, model: i.model || null, requested_model: i.requested_model || null, detail: i.shots ? { shots: i.shots.map((s) => shotWithBytes(s, pictures)) } : (i.kind === 'diff' ? { files: i.files, stats: i.stats } : null) })), outcome: collected.outcome, review: collected.review || null };
} catch (e) {
} catch (e) {
say({ t: 'note', level: 'warning', text: `The proof could not be collected: ${scrub(e.message)}` });
say({ t: 'note', level: 'warning', text: `The proof could not be collected: ${scrub(e.message)}` });
result.proof = { items: [], outcome: null };
result.proof = { items: [], outcome: null };
@@ -275,7 +275,7 @@ function runTokenOf(jobEnv = {}) {
@@ -275,7 +275,7 @@ function runTokenOf(jobEnv = {}) {
* says so rather than pretending one happened.
* says so rather than pretending one happened.
*/
*/
function reviewAdapter({ env = {}, jobEnv = {}, fetchImpl = globalThis.fetch, baseUrl, models = [] } = {}) {
function reviewAdapter({ env = {}, jobEnv = {}, fetchImpl = globalThis.fetch, baseUrl, models = [] } = {}) {
- return async function review({ patch = '', summary = '', tests = null, model = null, family = null } = {}) {
+ return async function review({ patch = '', summary = '', tests = null, model = null, family = null, prior = [] } = {}) {
// The token the attempt's own harness carries: only the job's own input, under the name C3 gave that harness.
// The token the attempt's own harness carries: only the job's own input, under the name C3 gave that harness.
// The machine's own OPENVIBE_AI_TOKEN names its owner, not this task's person, so it is never spent here.
// The machine's own OPENVIBE_AI_TOKEN names its owner, not this task's person, so it is never spent here.
const token = runTokenOf(jobEnv);
const token = runTokenOf(jobEnv);
@@ -292,7 +292,7 @@ function reviewAdapter({ env = {}, jobEnv = {}, fetchImpl = globalThis.fetch, ba
@@ -292,7 +292,7 @@ function reviewAdapter({ env = {}, jobEnv = {}, fetchImpl = globalThis.fetch, ba
}
}
return await reviewWithAi({
return await reviewWithAi({
baseUrl: baseUrl || aiRoot(jobEnv) || env.OPENVIBE_AI_URL || 'https://ai.openvibe.services/v1',
baseUrl: baseUrl || aiRoot(jobEnv) || env.OPENVIBE_AI_URL || 'https://ai.openvibe.services/v1',
- token, model: chosen, family: familyOf(chosen), patch, summary, tests, fetchImpl,
+ token, model: chosen, family: familyOf(chosen), patch, summary, tests, prior, fetchImpl,
});
});
};
};
}
}
new file mode 100644
new file mode 100644
@@ -0,0 +1,77 @@
@@ -0,0 +1,77 @@
+'use strict';
+// The persisted review ledger: stable ids, ACTIVE/RESOLVED, and the addressed/reappeared/still open/new line.
+
+const assert = require('node:assert/strict');
+const fs = require('node:fs');
+const path = require('node:path');
+const { check, done } = require('./helpers/boot');
+const F = require('../engine/findings');
+const { findingId } = require('../engine/finding');
+
+const json = (findings, verdict = 'changes_requested') => JSON.stringify({ verdict, summary: 'read it', findings });
+
+(async () => {
+ await check('a JSON review yields records with the stable id, and the cap is three', () => {
+ const recs = F.parse(json([
+ { file: 'server/x.js', line: 31, text: 'The cache NEVER expires', severity: 'high' },
+ { file: 'server/y.js', line: 5, text: 'The retry debits twice' },
+ { file: 'server/z.js', text: 'No test reaches this' },
+ { file: 'server/ignored.js', text: 'Over the quiet cap' },
+ ]));
+ assert.equal(recs.length, 3);
+ assert.equal(recs[0].finding_id, findingId('server/x.js', 'The cache never expires'));
+ assert.deepEqual([recs[0].state, recs[0].severity], ['ACTIVE', 'high']);
+ assert.equal(recs[2].line, null);
+ });
+
+ await check('the legacy VERDICT bullets are read too, so an older reviewer keeps its ids', () => {
+ const recs = F.parse('VERDICT: FIX\n1. server/x.js:31 it races with the writer\n2. a design note\n');
+ assert.equal(recs.length, 1);
+ assert.equal(recs[0].file, 'server/x.js');
+ assert.equal(recs[0].line, 31);
+ assert.match(recs[0].text, /races/);
+ });
+
+ await check('a second review reconciles: addressed, reappeared, still open and new', () => {
+ const first = F.fromReview(json([
+ { file: 'a.js', line: 1, text: 'the cache never expires' },
+ { file: 'b.js', line: 2, text: 'the retry debits twice' },
+ ]));
+ assert.deepEqual(first.lifecycle, { addressed: 0, reappeared: 0, still_open: 0, new: 2 });
+ assert.equal(first.line, 'Findings: 0 addressed, 0 reappeared, 0 still open, 2 new');
+ const second = F.fromReview(json([
+ { file: 'a.js', line: 1, text: 'The cache never expires' },
+ { file: 'c.js', line: 3, text: 'a new leak' },
+ ], 'changes_requested'), first.findings);
+ assert.deepEqual(second.lifecycle, { addressed: 1, reappeared: 0, still_open: 1, new: 1 });
+ assert.equal(second.line, 'Findings: 1 addressed, 0 reappeared, 1 still open, 1 new');
+ const cache = second.findings.find((f) => f.file === 'a.js'), retry = second.findings.find((f) => f.file === 'b.js');
+ assert.equal(cache.state, 'ACTIVE');
+ assert.equal(retry.state, 'RESOLVED');
+ });
+
+ await check('a resolved finding that comes back has reappeared, and an approval resolves what it no longer raises', () => {
+ const one = F.fromReview(json([{ file: 'a.js', line: 1, text: 'the cache never expires' }]));
+ const fixed = F.fromReview(json([], 'approved'), one.findings);
+ assert.deepEqual(fixed.lifecycle, { addressed: 1, reappeared: 0, still_open: 0, new: 0 });
+ assert.equal(fixed.findings[0].state, 'RESOLVED');
+ const back = F.fromReview(json([{ file: 'a.js', line: 1, text: 'the cache never expires' }]), fixed.findings);
+ assert.deepEqual(back.lifecycle, { addressed: 0, reappeared: 1, still_open: 0, new: 0 });
+ assert.equal(back.findings[0].state, 'ACTIVE');
+ });
+
+ await check('no prior and no current findings says nothing, and the prior brief is compact', () => {
+ assert.equal(F.fromReview('{"verdict":"approved","findings":[]}').line, null);
+ assert.equal(F.priorBrief([]), '');
+ const recs = F.fromReview(json([{ file: 'a.js', line: 1, text: 'the cache never expires' }])).findings;
+ assert.match(F.priorBrief(recs), /\[ACTIVE\] a\.js:1 — the cache never expires \(id a\.js:the cache never expires\)/);
+ });
+
+ await check('the transcript schema publishes the persisted review shape', () => {
+ const schema = JSON.parse(fs.readFileSync(path.join(__dirname, '..', 'engine', 'transcript.schema.json'), 'utf8'));
+ assert.deepEqual(schema.properties.review.required, ['verdict', 'findings', 'lifecycle']);
+ assert.deepEqual(schema.$defs.reviewFinding.properties.state.enum, ['ACTIVE', 'RESOLVED']);
+ assert.deepEqual(schema.$defs.reviewLifecycle.required, ['addressed', 'reappeared', 'still_open', 'new']);
+ });
+ done();
+})();
@@ -9,7 +9,7 @@
@@ -9,7 +9,7 @@
*/
*/
const assert = require('assert');
const assert = require('assert');
const {
const {
- familyOf, authorFamily, pickReviewer, reviewPrompt, readVerdict, reviewWithAi,
+ familyOf, authorFamily, pickReviewer, reviewPrompt, readReview, readVerdict, reviewWithAi,
} = require('../engine/review');
} = require('../engine/review');
const { check, done } = require('./helpers/boot');
const { check, done } = require('./helpers/boot');
@@ -49,6 +49,41 @@ const OK = async () => ({ status: 200, ok: true, json: async () => ({ choices: [
@@ -49,6 +49,41 @@ const OK = async () => ({ status: 200, ok: true, json: async () => ({ choices: [
assert.ok(/approved/.test(prompt) && /changes_requested/.test(prompt), 'the verdict is asked for');
assert.ok(/approved/.test(prompt) && /changes_requested/.test(prompt), 'the verdict is asked for');
});
});
+ await check('quiet by default: only this diff, at most three, no style, and a prior ledger on a re-review', () => {
+ const prompt = reviewPrompt({ patch: '+x', summary: 'add x' });
+ assert.match(prompt, /only issues this change introduces/i);
+ assert.match(prompt, /at most 3 findings/i);
+ assert.match(prompt, /No style nits/i);
+ assert.match(prompt, /empty findings list/i);
+ const prior = [{ finding_id: 'a.js:it races', file: 'a.js', line: 1, text: 'it races', state: 'ACTIVE', severity: 'high' }];
+ const again = reviewPrompt({ patch: '+y', prior });
+ assert.match(again, /previous review of an earlier revision/i);
+ assert.match(again, /a\.js:1 — it races \(id a\.js:it races\)/);
+ });
+
+ await check('findings are read from JSON findings and from the legacy numbered bullets', () => {
+ const doc = readReview('{"verdict":"changes_requested","summary":"one bug","findings":[{"file":"a.js","line":2,"text":"it races","severity":"high"}]}');
+ assert.deepEqual([doc.verdict, doc.findings.length, doc.findings[0].file], ['changes_requested', 1, 'a.js']);
+ const prose = readReview('VERDICT: FIX\n1. a.js:2 it races with the writer');
+ assert.equal(prose.findings.length, 1);
+ assert.equal(prose.findings[0].line, 2);
+ assert.deepEqual(readReview('{"verdict":"approved","summary":"ok","findings":[]}').findings, []);
+ });
+
+ await check('a re-review reconciles its findings and keeps a dead reviewer\'s ledger', async () => {
+ const prior = [{ finding_id: 'a.js:it races', file: 'a.js', line: 1, text: 'it races', state: 'ACTIVE', severity: 'high' }];
+ const approved = async () => ({ status: 200, ok: true, json: async () => ({ choices: [{ message: { content: '{"verdict":"approved","summary":"fixed","findings":[]}' } }] }) });
+ const fixed = await reviewWithAi({ token: 't', model: 'deepseek-chat', patch: '+x', prior, fetchImpl: approved });
+ assert.deepEqual(fixed.lifecycle, { addressed: 1, reappeared: 0, still_open: 0, new: 0 });
+ assert.strictEqual(fixed.lifecycle_line, 'Findings: 1 addressed, 0 reappeared, 0 still open, 0 new');
+ assert.strictEqual(fixed.findings[0].state, 'RESOLVED');
+ // A reviewer that says nothing reviewed nothing: the prior ledger stays ACTIVE, not silently addressed.
+ const silent = async () => ({ status: 200, ok: true, json: async () => ({ choices: [{ message: { content: '' } }] }) });
+ const kept = await reviewWithAi({ token: 't', model: 'deepseek-chat', patch: '+x', prior, fetchImpl: silent });
+ assert.strictEqual(kept.lifecycle, null);
+ assert.strictEqual(kept.findings[0].state, 'ACTIVE');
+ });
+
await check('the review is one chat completion, with the token, read as a verdict', async () => {
await check('the review is one chat completion, with the token, read as a verdict', async () => {
const calls = [];
const calls = [];
const fetchImpl = async (url, options) => { calls.push({ url, options }); return OK(); };
const fetchImpl = async (url, options) => { calls.push({ url, options }); return OK(); };
@@ -62,6 +97,7 @@ const OK = async () => ({ status: 200, ok: true, json: async () => ({ choices: [
@@ -62,6 +97,7 @@ const OK = async () => ({ status: 200, ok: true, json: async () => ({ choices: [
assert.strictEqual(JSON.parse(calls[0].options.body).model, 'deepseek-chat');
assert.strictEqual(JSON.parse(calls[0].options.body).model, 'deepseek-chat');
assert.strictEqual(verdict.verdict, 'approved');
assert.strictEqual(verdict.verdict, 'approved');
assert.strictEqual(verdict.family, 'deepseek');
assert.strictEqual(verdict.family, 'deepseek');
+ assert.deepEqual(verdict.findings, [], 'an approval is an empty findings list, not a "no issues" note');
});
});
await check('the review records the resolved model and cost from the API\'s disclosure', async () => {
await check('the review records the resolved model and cost from the API\'s disclosure', async () => {