openvibers/openvibe.codes Commits

Proof follow-up: the reviewer spends only the job's token, and no test match is a skip (#171)

OP OpenVibers committed 14 days ago · 03659dafd9 · 4 files, +68 −17

Two gaps the first pass left, both false proof:

- The review fell back to the machine's own OPENVIBE_AI_TOKEN (or a stray OPENVIBE_CODES_TASK_TOKEN)
  when the job carried no C3 run token. That token names the machine's owner, not the task's person:
  the review would have been billed to the wrong account. The token now comes only from the job's
  input, under the name C3 gave the attempt's harness, including the OpenAI-compatible harness's
  OPENVIBE_CODES_API_KEY and its endpoint.
- A narrowed test filter that matches no test (a docs or asset change, package.json alone) made the
  runner exit 1 and the proof call the task unfinished. The runner's own "no test files matched" is
  now a skip, said in the outcome. A change no test name reaches no longer runs the whole suite
  without the owner's guard (CODES_PROOF_FULL_SUITE=1), which the old plan did when the filter was
  empty.

Checked: npm test — 64/64 files. Added engine-proof and harness-proof checks for the no-match skip,
the unguarded-suite case, the machine-token refusal, and the OpenAI-compatible run token.

Changes

ddiff --git a/engine/proof.js b/engine/proof.js
iindex a44307a..302be5e 100644
---- a/engine/proof.js
++++ b/engine/proof.js
@@@ -17,8 +17,8 @@
   * The four steps, in order:
   *
   *   tests        the repo's own runner, narrowed to the tests the change reaches. The whole suite runs only when
-- *                the change touches what every test shares and the owner's guard allows it (CODES_PROOF_FULL_SUITE=1):
-- *                a machine may not spend an hour of its owner's battery on every task.
++ *                the owner's guard allows it (CODES_PROOF_FULL_SUITE=1): a machine may not spend an hour of its
++ *                owner's battery on every task. A filter that matches no test is a skip, not a failure.
   *   screenshots  phone (390) and desktop (1440) of the pages the diff touches, because a page that overflows is a
   *                bug the diff alone never shows.
   *   diff         the stats: what changed, how many lines, which files.
@@@ -37,6 +37,9 @@ const PAGE_FILE = /^(?:public\/|server\/render\/|server\/http\/)|\.(?:html?|css)
  /** The files that make every test matter: the shared ones. A change here runs the whole suite (when allowed). */
  const SHARED_FILE = /^(?:package\.json|package-lock\.json|engine\/index\.js|server\/app\.js|server\/db\.js|server\/config\.js)$/;
  
++/** The runners' own words for "the filter matched no test": not a failure, nothing ran. */
++const NO_TEST_MATCH = /no test files? (?:matched|found)|no tests? (?:found|ran|to run)|running 0 tests/i;
++
  /** The kinds of proof item, in the order they are collected. */
  const KINDS = ['tests', 'screenshots', 'diff', 'review'];
  
@@@ -94,7 +97,8 @@ function nodeTestScript(script) {
   *
   * The runner is the repo's: 'npm test' when package.json has it (or the file it runs, when that is `node <file>`),
   * then pytest, go and cargo. The suite is narrowed to the tests the change reaches; the whole suite runs only when
-- * the change touches what every test shares and the owner's guard allows it (CODES_PROOF_FULL_SUITE=1).
++ * the owner's guard allows it (CODES_PROOF_FULL_SUITE=1) — a change that touches what every test shares, or one no
++ * test name reaches.
   *
   * @param {object} what
   * @param {object} what.pkg the repo's package.json (or null)
@@@ -117,10 +121,13 @@ function testPlan({ pkg = null, files = [], env = {} } = {}) {
          const base = direct ? { command: 'node', args: [direct] } : { command: 'npm', args: ['test'] };
          const narrowed = direct ? [direct, ...words] : ['test', '--', ...words];
          if (shared && allowFull) return { ...base, mode: 'full', reason: 'the change touches what every test shares, and the full suite is allowed' };
--        if (!shared) {
--            if (!words.length) return { ...base, mode: 'targeted', reason: 'no test name matches the changed files, so the suite runs as it is' };
--            return { command: base.command, args: narrowed, mode: 'targeted', reason: 'only the tests the change reaches (' + words.join(', ') + ')' };
++        if (!words.length) {
++            // Nothing in the diff names a test, so there is nothing to narrow to. The whole suite is not run without
++            // the owner's guard: a machine may not spend an hour of its owner's battery on every task.
++            if (allowFull) return { ...base, mode: 'full', reason: 'no test name matches the changed files, and the full suite is allowed' };
++            return { command: '', args: [], mode: 'none', reason: 'no test name matches the changed files, and the full suite is not allowed (CODES_PROOF_FULL_SUITE=1)' };
          }
++        if (!shared) return { command: base.command, args: narrowed, mode: 'targeted', reason: 'only the tests the change reaches (' + words.join(', ') + ')' };
          return { command: base.command, args: narrowed, mode: 'targeted', reason: 'the change touches what every test shares, but the full suite is not allowed (CODES_PROOF_FULL_SUITE=1)' };
      }
      if (pkg && (pkg.go || (pkg.dependencies && pkg.dependencies.go))) return { command: 'go', args: ['test', './...'], mode: 'full', reason: "the repo's own Go suite" };
@@@ -210,10 +217,14 @@ async function collectProof({ cwd, files = [], adapters = {}, author = null, emi
          const result = await run([plan.command, ...plan.args], { cwd, timeoutMs: adapters.testTimeoutMs || 600000 });
          const output = String((result && result.output) || '');
          const ok = !!result && result.code === 0;
++        // The runner's filter matched nothing: no test reaches the change by name, so nothing is proved and nothing
++        // failed. Saying "the tests failed" would be a false alarm on a docs or asset change.
++        const noMatch = !ok && NO_TEST_MATCH.test(output);
          await say({
--            kind: 'tests', label: 'tests', ok, skipped: false, mode: plan.mode, reason: plan.reason,
++            kind: 'tests', label: 'tests', ok: noMatch ? true : ok, skipped: noMatch, mode: plan.mode, reason: plan.reason,
              command: [plan.command, ...plan.args].join(' '),
--            summary: ok ? plan.reason : (output.trim().split('\n').slice(-6).join(' ') || 'the tests failed (exit ' + result.code + ')'),
++            summary: noMatch ? 'no test matches the changed files, so none ran'
++                : (ok ? plan.reason : (output.trim().split('\n').slice(-6).join(' ') || 'the tests failed (exit ' + result.code + ')')),
          });
      }
  
ddiff --git a/harness/proof.js b/harness/proof.js
iindex 6a60b43..39a757e 100644
---- a/harness/proof.js
++++ b/harness/proof.js
@@@ -253,10 +253,17 @@ function aiRoot(env = {}) {
      if (env.OPENVIBE_AI_URL) return env.OPENVIBE_AI_URL;
      if (env.OPENAI_BASE_URL) return env.OPENAI_BASE_URL;
      if (env.DEEPSEEK_BASE_URL) return env.DEEPSEEK_BASE_URL;
++    if (env.OPENVIBE_CODES_BASE_URL) return env.OPENVIBE_CODES_BASE_URL;
      if (env.ANTHROPIC_BASE_URL) return String(env.ANTHROPIC_BASE_URL).replace(/\/+$/, '') + '/v1';
      return '';
  }
  
++/** The token C3 put in the job's input, under the name the attempt's harness was given. */
++function runTokenOf(jobEnv = {}) {
++    return jobEnv.OPENVIBE_AI_TOKEN || jobEnv.OPENAI_API_KEY || jobEnv.ANTHROPIC_AUTH_TOKEN
++        || jobEnv.DEEPSEEK_API_KEY || jobEnv.OPENVIBE_CODES_API_KEY || '';
++}
++
  /**
   * The second opinion: OpenVibe.AI, with the task's run token.
   *
@@@ -267,9 +274,9 @@ function aiRoot(env = {}) {
   */
  function reviewAdapter({ env = {}, jobEnv = {}, fetchImpl = globalThis.fetch, baseUrl, models = [] } = {}) {
      return async function review({ patch = '', summary = '', tests = null, model = null, family = null } = {}) {
--        // The token the attempt's own harness carries: any of the names C3 gives, that harness's provider first.
--        const runToken = jobEnv.OPENVIBE_AI_TOKEN || jobEnv.OPENAI_API_KEY || jobEnv.ANTHROPIC_AUTH_TOKEN || jobEnv.DEEPSEEK_API_KEY || '';
--        const token = runToken || env.OPENVIBE_CODES_TASK_TOKEN || env.OPENVIBE_AI_TOKEN || '';
++        // 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);
          if (!token) {
              const err = new Error('no task token is connected yet, so no model could review this change');
              err.code = 'review.no_token';
ddiff --git a/test/engine-proof.test.js b/test/engine-proof.test.js
iindex aeacd7c..12eb9d5 100644
---- a/test/engine-proof.test.js
++++ b/test/engine-proof.test.js
@@@ -13,13 +13,14 @@ const { check, done } = require('./helpers/boot');
  
  (async () => {
      /** A fake machine: the repo's package.json, a passing test run, a clean page and a second model's opinion. */
--    const machine = ({ pkg = { scripts: { test: 'node test/run.js' } }, files = [], testExit = 0, review = null, shots = [] } = {}) => {
++    const machine = ({ pkg = { scripts: { test: 'node test/run.js' } }, files = [], testExit = 0, testOutput = null, review = null, shots = [] } = {}) => {
          const calls = [];
++        const output = testOutput == null ? (testExit === 0 ? '12/12 test files passed' : 'FAIL test/foo.test.js\nAssertionError') : testOutput;
          const run = async (argv, options) => {
              calls.push(argv);
              if (argv[0] === 'node' && argv[1] === '-e') return { code: 0, output: JSON.stringify(pkg) };
--            if (argv[0] === 'node' && argv[1] === 'test/run.js') return { code: testExit, output: testExit === 0 ? '12/12 test files passed' : 'FAIL test/foo.test.js\nAssertionError' };
--            if (argv[0] === 'npm') return { code: testExit, output: testExit === 0 ? '12/12 test files passed' : 'FAIL test/foo.test.js\nAssertionError' };
++            if (argv[0] === 'node' && argv[1] === 'test/run.js') return { code: testExit, output };
++            if (argv[0] === 'npm') return { code: testExit, output };
              return { code: 0, output: '' };
          };
          return {
@@@ -69,6 +70,25 @@ const { check, done } = require('./helpers/boot');
              assert.deepStrictEqual(allowed.args, ['test/run.js']);
          });
  
++        await check('a change no test name reaches does not run the whole suite unguarded', async () => {
++            const plan = testPlan({ pkg: { scripts: { test: 'node test/run.js' } }, files: ['engine/index.js'], env: {} });
++            assert.strictEqual(plan.mode, 'none');
++            assert.ok(/full suite is not allowed/.test(plan.reason), plan.reason);
++            const allowed = testPlan({ pkg: { scripts: { test: 'node test/run.js' } }, files: ['engine/index.js'], env: { CODES_PROOF_FULL_SUITE: '1' } });
++            assert.strictEqual(allowed.mode, 'full');
++        });
++
++        await check('a narrowed run that matches no test is a skip, not a failed proof', async () => {
++            const m = machine({ files: ['README.md'], testExit: 1, testOutput: 'no test files matched' });
++            const proof = await collectProof({ cwd: '/repo', adapters: m.adapters });
++            const test = proof.items.find((i) => i.kind === 'tests');
++            assert.strictEqual(test.ok, true);
++            assert.strictEqual(test.skipped, true);
++            assert.ok(/no test matches/.test(test.summary), test.summary);
++            assert.strictEqual(proof.outcome.state, 'done');
++            assert.ok(/tests was skipped/.test(proof.outcome.note), proof.outcome.note);
++        });
++
          await check('a repo with no test command of its own says so instead of failing', async () => {
              const m = machine({ pkg: { scripts: {} } });
              const proof = await collectProof({ cwd: '/repo', adapters: m.adapters });
ddiff --git a/test/harness-proof.test.js b/test/harness-proof.test.js
iindex bd1183e..5970960 100644
---- a/test/harness-proof.test.js
++++ b/test/harness-proof.test.js
@@@ -171,7 +171,7 @@ const OK = async () => ({ status: 200, ok: true, json: async () => ({ choices: [
          await check('the review goes to OpenVibe.AI as chat completions, with the token', async () => {
              const calls = [];
              const fetchImpl = async (url, options) => { calls.push({ url, options }); return OK(); };
--            const review = reviewAdapter({ env: { OPENVIBE_CODES_TASK_TOKEN: 'test-token' }, fetchImpl, models: ['deepseek-chat'] });
++            const review = reviewAdapter({ jobEnv: { OPENVIBE_AI_TOKEN: 'test-token' }, fetchImpl, models: ['deepseek-chat'] });
              const verdict = await review({ patch: '+x', summary: 'a change', family: 'anthropic', model: 'deepseek-chat' });
              assert.strictEqual(verdict.verdict, 'approved');
              assert.strictEqual(verdict.family, 'deepseek');
@@@ -194,10 +194,23 @@ const OK = async () => ({ status: 200, ok: true, json: async () => ({ choices: [
          });
  
          await check('a personal provider key in the environment is not spent as a run token', async () => {
--            const review = reviewAdapter({ env: { OPENAI_API_KEY: 'a-personal-key' }, fetchImpl: OK, models: ['deepseek-chat'] });
++            const review = reviewAdapter({ env: { OPENAI_API_KEY: 'a-personal-key', OPENVIBE_AI_TOKEN: 'the-owner' }, fetchImpl: OK, models: ['deepseek-chat'] });
              await assert.rejects(() => review({ patch: '+x', family: 'anthropic' }), (e) => e.code === 'review.no_token');
          });
  
++        await check('an OpenAI-compatible harness\'s run token and endpoint are the ones the job carries', async () => {
++            const calls = [];
++            const fetchImpl = async (url, options) => { calls.push({ url, options }); return OK(); };
++            const review = reviewAdapter({
++                jobEnv: { OPENVIBE_CODES_BASE_URL: 'https://ai.openvibe.services/v1', OPENVIBE_CODES_API_KEY: 'run-token' },
++                fetchImpl,
++            });
++            const verdict = await review({ patch: '+x', family: 'anthropic', model: 'deepseek-chat' });
++            assert.strictEqual(verdict.verdict, 'approved');
++            assert.strictEqual(calls[0].url, 'https://ai.openvibe.services/v1/chat/completions');
++            assert.strictEqual(calls[0].options.headers.authorization, 'Bearer run-token');
++        });
++
          await check('a provider that speaks the messages dialect is tried when chat completions is not there', async () => {
              const calls = [];
              const fetchImpl = async (url) => {
@@@ -205,7 +218,7 @@ const OK = async () => ({ status: 200, ok: true, json: async () => ({ choices: [
                  if (url.endsWith('/chat/completions')) return { status: 404, ok: false, json: async () => ({}) };
                  return { status: 200, ok: true, json: async () => ({ content: [{ type: 'text', text: 'changes requested: the parser still throws' }] }) };
              };
--            const review = reviewAdapter({ env: { OPENVIBE_AI_TOKEN: 'test-token' }, fetchImpl });
++            const review = reviewAdapter({ jobEnv: { OPENVIBE_AI_TOKEN: 'test-token' }, fetchImpl });
              const verdict = await review({ patch: '+x', family: 'anthropic', model: 'claude-sonnet-4' });
              assert.ok(calls[1].endsWith('/messages'), calls[1]);
              assert.strictEqual(verdict.verdict, 'changes_requested');