openvibers/openvibe.codes Commits

Review Codes changes on separate Standards and Spec axes (#401)

OP OpenVibers committed 2 days ago · 333f52a39a · 8 files, +94 −15

* Review code changes against standards and task spec separately

* Allow review axes in persisted finding schema

* Retain findings from both review axes

* Preserve every finding from both review axes

Changes

ddiff --git a/engine/findings.js b/engine/findings.js
iindex 30b299a..21b1e82 100644
---- a/engine/findings.js
++++ b/engine/findings.js
@@@ -42,6 +42,7 @@ function record(item) {
line,
text: text.slice(0, 2000),
severity,
++ ...(item.axis === 'standards' || item.axis === 'spec' ? { axis: item.axis } : {}),
state: STATE.ACTIVE,
};
}
ddiff --git a/engine/proof.js b/engine/proof.js
iindex 446f41f..c490935 100644
---- a/engine/proof.js
++++ b/engine/proof.js
@@@ -193,7 +193,7 @@ function routesOf(pages) {
* @param {function} [what.emit] called with every item as it is collected
* @returns {Promise<{ items: object[], outcome: object, review: object|null }>} the proof
*/
--async function collectProof({ cwd, files = [], adapters = {}, author = null, emit = null } = {}) {
++async function collectProof({ cwd, files = [], adapters = {}, author = null, spec = '', emit = null } = {}) {
if (!cwd) throw new TypeError('proof needs the repo it is about');
const run = adapters.run;
if (typeof run !== 'function') throw new TypeError('proof needs a run adapter');
@@@ -277,7 +277,7 @@ async function collectProof({ cwd, files = [], adapters = {}, author = null, emi
try {
const tests = items.find((i) => i.kind === 'tests');
verdict = await adapters.review({
-- patch, summary: (diff && diff.summary) || '',
++ patch, summary: spec,
tests: tests ? tests.summary : null,
model: chosen.model, family: chosen.family,
});
ddiff --git a/engine/review.js b/engine/review.js
iindex 504a8d2..0a8be76 100644
---- a/engine/review.js
++++ b/engine/review.js
@@@ -96,20 +96,23 @@ function pickReviewer({ models = [], author = null } = {}) {
* 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 = [], reviewedHead = null, head = null } = {}) {
++function reviewPrompt({ patch = '', summary = '', tests = null, prior = [], reviewedHead = null, head = null, axis = null, standards = '' } = {}) {
const before = priorBrief(prior);
return [
'You are reviewing a change to a repository. Answer only with JSON:',
`{"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`,
++ axis === 'standards' ? 'Standards axis only: check the repository rules below and concrete code smells in the changed lines (duplication, misleading names, hidden side effects, tangled control flow). Do not judge whether the ticket was fulfilled.' : '',
++ axis === 'spec' ? 'Spec axis only: check the change against the ticket below. Do not judge repository conventions or general code smells.' : '',
++ axis === 'standards' && standards ? 'Repository rules:\n' + String(standards).slice(0, 8000) : '',
++ axis === 'standards' ? 'Flag only concrete violations introduced by this change, with a file, line and fix.' : '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 : '',
reviewedHead && head ? `This is an incremental re-review of ${reviewedHead}..${head}. The supplied diff is anchored on the last reviewed head; review only the changes in that range and reconcile the earlier findings above.` : '',
-- summary ? 'What the change was asked to do:\n' + String(summary).slice(0, 4000) : '',
++ axis !== 'standards' && 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) : '',
'',
'The diff:\n' + String(patch || '').slice(0, 120000),
@@@ -172,12 +175,14 @@ async function reviewWithAi({
prior = [],
reviewedHead = null,
head = null,
++ axis = null,
++ standards = '',
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, prior, reviewedHead, head });
++ const prompt = reviewPrompt({ patch, summary, tests, prior, reviewedHead, head, axis, standards });
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 }] }),
@@@ -217,7 +222,9 @@ async function reviewWithAi({
...(x ? { disclosure: x } : {}),
...(x && Number.isFinite(+x.cost_usd) ? { cost_usd: +x.cost_usd } : {}),
verdict: read.verdict,
-- summary: read.summary,
++ summary: read.summary.trim().split(/\s+/).slice(0, 400).join(' '),
++ raw_findings: read.findings,
++ reviewed: !!String(text || '').trim(),
// The persisted lifecycle: ACTIVE/RESOLVED records and the addressed/reappeared/still open/new line.
findings: ledger.findings,
lifecycle: ledger.lifecycle,
@@@ -225,7 +232,44 @@ async function reviewWithAi({
};
}
++/** Separate model contexts. A missing ticket is recorded as not run, never as an approval. */
++async function reviewTwoAxes(options = {}) {
++ const spec = String(options.summary || '').trim();
++ const prior = Array.isArray(options.prior) ? options.prior : [];
++ const standards = await reviewWithAi({ ...options, axis: 'standards', summary: '', prior: prior.filter((f) => !f.axis || f.axis === 'standards') });
++ const specification = spec ? await reviewWithAi({ ...options, axis: 'spec', prior: prior.filter((f) => !f.axis || f.axis === 'spec') }) : null;
++ const { merge, lifecycleLine, parse } = require('./findings');
++ if (!standards.reviewed || (specification && !specification.reviewed)) return {
++ ...standards, verdict: 'changes_requested', summary: 'a reviewer said nothing',
++ axes: { standards: { verdict: standards.reviewed ? standards.verdict : 'not_run' }, spec: { verdict: specification ? (specification.reviewed ? specification.verdict : 'not_run') : 'not_run' } },
++ findings: prior, lifecycle: null, lifecycle_line: null,
++ };
++ const standardFindings = parse(JSON.stringify({ findings: standards.raw_findings.map((f) => ({ ...f, axis: 'standards' })) }));
++ const specFindings = specification ? parse(JSON.stringify({ findings: specification.raw_findings.map((f) => ({ ...f, axis: 'spec' })) })) : [];
++ const current = [];
++ for (let i = 0; i < MAX_FINDINGS; i++) {
++ if (standardFindings[i]) current.push(standardFindings[i]);
++ if (specFindings[i]) current.push(specFindings[i]);
++ }
++ const merged = merge(spec ? prior : prior.filter((f) => f.axis !== 'spec'), current);
++ if (!spec) merged.findings.push(...prior.filter((f) => f.axis === 'spec'));
++ const failed = standards.verdict !== 'approved' || (specification && specification.verdict !== 'approved');
++ return {
++ ...standards,
++ ...(specification ? { cost_usd: (standards.cost_usd || 0) + (specification.cost_usd || 0) } : {}),
++ verdict: failed ? 'changes_requested' : 'approved',
++ summary: `Standards: ${standards.summary}; Spec: ${specification ? specification.summary : 'not run (no ticket spec)'}`.slice(0, 4000),
++ axes: {
++ standards: { verdict: standards.verdict, summary: standards.summary },
++ spec: specification ? { verdict: specification.verdict, summary: specification.summary } : { verdict: 'not_run', summary: 'no ticket spec' },
++ },
++ findings: merged.findings,
++ lifecycle: merged.lifecycle,
++ lifecycle_line: lifecycleLine(merged.lifecycle),
++ };
++}
++
module.exports = {
FAMILIES, HARNESS_FAMILY,
-- familyOf, authorFamily, pickReviewer, reviewPrompt, readReview, readVerdict, reviewWithAi,
++ familyOf, authorFamily, pickReviewer, reviewPrompt, readReview, readVerdict, reviewWithAi, reviewTwoAxes,
};
ddiff --git a/engine/transcript.schema.json b/engine/transcript.schema.json
iindex 6391a63..165ed97 100644
---- a/engine/transcript.schema.json
++++ b/engine/transcript.schema.json
@@@ -289,6 +289,7 @@
"line": { "type": ["integer", "null"], "minimum": 1 },
"text": { "type": "string", "minLength": 1, "maxLength": 2000 },
"severity": { "type": ["string", "null"], "enum": ["low", "medium", "high", "critical", null] },
++ "axis": { "type": "string", "enum": ["standards", "spec"] },
"state": { "type": "string", "enum": ["ACTIVE", "RESOLVED"] }
}
},
ddiff --git a/harness/job.js b/harness/job.js
iindex 0e1b62c..5b19a7f 100644
---- a/harness/job.js
++++ b/harness/job.js
@@@ -894,6 +894,7 @@ async function runJob({ input, harness, emit, writeResult, env = process.env, tm
files: result.files.map((f) => f.path),
adapters: proof,
author: { agent: job.agent, model: job.model },
++ spec: job.goal && job.goal.goal ? job.goal.goal : job.prompt,
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, review: collected.review || null };
ddiff --git a/harness/proof.js b/harness/proof.js
iindex 1771af3..f6bdb91 100644
---- a/harness/proof.js
++++ b/harness/proof.js
@@@ -23,7 +23,7 @@ const fs = require('fs');
const net = require('net');
const path = require('path');
const { execFile, spawn } = require('child_process');
--const { reviewWithAi, familyOf } = require('../engine/review');
++const { reviewTwoAxes, familyOf } = require('../engine/review');
const MAX_OUTPUT = 8000;
const MAX_PATCH = 200000;
@@@ -274,7 +274,7 @@ function runTokenOf(jobEnv = {}) {
* OPENAI_API_KEY is not Codes' to spend at OpenVibe.AI. Without a token there is no second opinion, and the adapter
* 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 = [], standards = '' } = {}) {
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.
@@@ -290,9 +290,9 @@ function reviewAdapter({ env = {}, jobEnv = {}, fetchImpl = globalThis.fetch, ba
err.code = 'review.no_model';
throw err;
}
-- return await reviewWithAi({
++ return await reviewTwoAxes({
baseUrl: baseUrl || aiRoot(jobEnv) || env.OPENVIBE_AI_URL || 'https://ai.openvibe.services/v1',
-- token, model: chosen, family: familyOf(chosen), patch, summary, tests, prior, fetchImpl,
++ token, model: chosen, family: familyOf(chosen), patch, summary, tests, prior, standards, fetchImpl,
});
};
}
@@@ -320,7 +320,8 @@ function createProofAdapters({ cwd, env = process.env, jobEnv = {}, fetchImpl =
run,
diff: diffAdapter(run),
screenshot: screenshotAdapter({ run, playwright: pw, chromePath: chrome, outDir: out, env, fetchImpl, startSiteFn }),
-- review: reviewAdapter({ env, jobEnv, fetchImpl, models: named }),
++ review: reviewAdapter({ env, jobEnv, fetchImpl, models: named,
++ standards: fs.existsSync(path.join(cwd, 'AGENTS.md')) ? fs.readFileSync(path.join(cwd, 'AGENTS.md'), 'utf8').slice(0, 8000) : '' }),
models: named,
env,
};
ddiff --git a/test/engine-findings.test.js b/test/engine-findings.test.js
iindex 254824e..9e422ff 100644
---- a/test/engine-findings.test.js
++++ b/test/engine-findings.test.js
@@@ -71,6 +71,7 @@ const json = (findings, verdict = 'changes_requested') => JSON.stringify({ verdi
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.reviewFinding.properties.axis.enum, ['standards', 'spec']);
assert.deepEqual(schema.$defs.reviewLifecycle.required, ['addressed', 'reappeared', 'still_open', 'new']);
});
done();
ddiff --git a/test/engine-review.test.js b/test/engine-review.test.js
iindex ba57661..2e49cad 100644
---- a/test/engine-review.test.js
++++ b/test/engine-review.test.js
@@@ -9,7 +9,7 @@
*/
const assert = require('assert');
const {
-- familyOf, authorFamily, pickReviewer, reviewPrompt, readReview, readVerdict, reviewWithAi,
++ familyOf, authorFamily, pickReviewer, reviewPrompt, readReview, readVerdict, reviewWithAi, reviewTwoAxes,
} = require('../engine/review');
const { check, done } = require('./helpers/boot');
@@@ -49,6 +49,36 @@ 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('standards and spec have separate contexts, and absent spec is not run', async () => {
++ const prompts = [];
++ const fetchImpl = async (_url, options) => {
++ prompts.push(JSON.parse(options.body).messages[0].content);
++ return OK();
++ };
++ const opts = { token: 'test-token', model: 'deepseek-chat', patch: '+x', summary: 'ticket requirement', standards: 'repository rule', fetchImpl };
++ const reviewed = await reviewTwoAxes(opts);
++ assert.strictEqual(reviewed.verdict, 'approved');
++ assert.strictEqual(prompts.length, 2);
++ assert.match(prompts[0], /repository rule/);
++ assert.ok(!prompts[0].includes('ticket requirement'));
++ assert.match(prompts[1], /ticket requirement/);
++ assert.ok(!prompts[1].includes('repository rule'));
++ const missing = await reviewTwoAxes({ ...opts, summary: '' });
++ assert.strictEqual(missing.axes.spec.verdict, 'not_run');
++ assert.strictEqual(prompts.length, 3);
++ const findingsFetch = async (_url, options) => {
++ const isSpec = JSON.parse(options.body).messages[0].content.includes('Spec axis only');
++ const findings = isSpec ? [{ file: 'spec.js', line: 3, text: 'ticket case is missing' }] : [
++ { file: 'a.js', line: 1, text: 'duplicated code' },
++ { file: 'b.js', line: 2, text: 'hidden side effect' },
++ { file: 'c.js', line: 4, text: 'misleading name' },
++ ];
++ return { status: 200, ok: true, json: async () => ({ choices: [{ message: { content: JSON.stringify({ verdict: 'changes_requested', summary: 'findings', findings }) } }] }) };
++ };
++ const withFindings = await reviewTwoAxes({ ...opts, fetchImpl: findingsFetch });
++ assert.deepEqual(withFindings.findings.map((f) => f.file), ['a.js', 'spec.js', 'b.js', 'c.js']);
++ });
++
await check('an incremental re-review names the reviewed and current heads', () => {
const prompt = reviewPrompt({ patch: '+fix', reviewedHead: 'abc123', head: 'def456' });
assert.ok(prompt.includes('incremental re-review of abc123..def456'));