fix(npm): address 10 verified review findings in harness + rvagent before 0.2.0 publish

harness/ruview (@ruvnet/ruview): - guardrails: digit gate now sees numbers inside code spans; F1-style metric tokens followed by ':' or a nearby number are no longer scrubbed (fail-open regressions in the honesty gate) - mcp-server: tools/call requests serialize through a FIFO promise chain (hardware/mutating tools never overlap) while ping/tools/list stay immediate; stdin close drains in-flight responses before exit - tools: which() no longer memoizes negative lookups tools/ruview-mcp (@ruvnet/rvagent): - index: realpath invoked-directly guard — library import no longer connects a stdio transport to the consumer's process - http-transport: explicit allowedOrigins is exact-match only (localhost any-port convenience applies only with no configured allowlist); session map gains maxSessions=64 + 5min idle TTL sweep - train-count: job records persist the child pid and reconcile stale 'running' status after a server restart (exit-code marker or dead pid) - config: cog binary candidates ordered by process.arch .github/workflows/ruview-npm-release.yml: port the full ADR-265 D1 gate (version-literal check, unpacked-size budget, tarball-install smoke test) from npm-packages.yml so the publish path enforces what the header claims. Tests: harness 30→36, rvagent 99→112, all passing. Co-Authored-By: claude-flow <ruv@ruv.net>
2026-07-28 18:21:42 +00:00 · 2026-07-02 09:58:00 -04:00
parent d26fb0b8fc
commit d46eb8a4b2
15 changed files with 651 additions and 44 deletions
@@ -21,9 +21,13 @@ const METRIC_TERMS = [
 // 'map' additionally must not be a `.map` file suffix or a hyphenated
 // compound ("map-free", "map-reduce") — mAP the metric never appears as either.
 const METRIC_TERMS_SHORT = [/(?<![.\w])map\b(?!-)/, /\bf1\b/, /\bauc\b/, /\biou\b/];
-// Finding/option labels (F1, O2, …) count as labels unless followed by a
-// score context — "F1 score 0.91" stays a metric, "after F7 fixes" does not.
-const LABEL_TOKEN_RE = /\b[fo]\d+\b(?!\s*(?:score|=|\d|%))/g;
+// Finding/option labels (F1, O2, …) count as labels unless the token sits in a
+// metric context: an immediately following score/=/%/digit or colon ("F1: 0.91"),
+// or a number later in the same clause ("F1 reaches 0.91" — an F1-score claim).
+// Bare option refs ("F7 fixes", "O1–O9", "ADR-263 O2") carry no clause number of
+// their own and stay labels. (A surviving 'f1' still only fires as a metric when
+// its scrubbed line actually carries a number — see mentionsMetricTerm.)
+const LABEL_TOKEN_RE = /\b[fo]\d+\b(?!\s*(?:score|=|\d|%|:))(?![^\n.;]*\d)/g;
 const CODE_SPAN_RE = /`[^`]*`/g; // backticked identifiers are code, not claims
 const HAS_NUMBER_RE = /\d/;

@@ -95,9 +99,14 @@ export function claimCheck(text) {
      return;
    }

-    // A quantitative claim needs a number: a metric term in plain prose
-    // ("precision matters here") is not a taggable claim (ADR-263 F11).
-    if (!hasPercent && !HAS_NUMBER_RE.test(scrubbed)) return;
+    // A quantitative claim needs a number. Digits hidden in a code span still
+    // count — "accuracy reached `0.95`" is a claim — so test the line with only
+    // finding/option labels stripped, NOT the code-span-scrubbed copy: scrubbing
+    // dropped `0.95` and wrongly short-circuited both the untagged and the
+    // MEASURED-without-reproducer checks below. A bare metric word in prose
+    // ("precision matters here", "every accuracy number must be MEASURED") has no
+    // number and is not a taggable claim (ADR-263 F11).
+    if (!hasPercent && !HAS_NUMBER_RE.test(lower.replace(LABEL_TOKEN_RE, ' '))) return;

    // A metric/percent with no honesty tag at all.
    if (!tagged) {
@@ -111,7 +120,8 @@ export function claimCheck(text) {
      return;
    }

-    // Tagged MEASURED but cites no reproducer — still a gap.
+    // Tagged MEASURED but cites no reproducer — still a gap (reached now even
+    // when the only number is inside a code span, e.g. "accuracy `0.97` (MEASURED)").
    if (lower.includes('measured') && !hasReproducer) {
      findings.push({
        severity: 'medium',
@@ -68,16 +68,40 @@ async function handle(msg) {
 export function startMcpServer() {
  log(`starting v${SERVER_INFO.version} (protocol ${PROTOCOL_VERSION}, ${listTools().length} tools)`);
  const rl = createInterface({ input: process.stdin, crlfDelay: Infinity });
+
+  // tools/call runs are serialized through a FIFO promise chain: hardware/mutating
+  // tools (calibrate, serial monitor, flash) must never overlap. ping/tools/list/
+  // initialize/resources/prompts stay immediate (ADR-263 O2 — a health check must
+  // answer during a long tool run). `toolChain` also lets stdin-close drain the
+  // in-flight call so its response is flushed instead of dropped by process.exit.
+  let toolChain = Promise.resolve();
+
+  const dispatch = (msg) => handle(msg).catch((err) => {
+    if (msg && msg.id !== undefined) error(msg.id, -32603, String(err && err.message || err));
+    log('handler error:', String(err));
+  });
+
  rl.on('line', (line) => {
    const s = line.trim();
    if (!s) return;
    let msg;
    try { msg = JSON.parse(s); } catch { return log('bad JSON line dropped'); }
-    // Fire-and-forget: keep reading stdin while tool calls run (ADR-263 O2).
-    handle(msg).catch((err) => {
-      if (msg && msg.id !== undefined) error(msg.id, -32603, String(err && err.message || err));
-      log('handler error:', String(err));
+    if (msg && msg.method === 'tools/call') {
+      toolChain = toolChain.then(() => dispatch(msg)); // one tool at a time
+    } else {
+      dispatch(msg); // health/list/handshake answer immediately, even mid tool run
+    }
+  });
+
+  rl.on('close', () => {
+    // Wait for any queued/in-flight tool call to settle (its response written)
+    // before exiting — fire-and-forget used to race this and drop the response.
+    toolChain.then(() => {
+      log('stdin closed — exiting');
+      const done = () => process.exit(0);
+      // Pipe writes are async; flush buffered stdout before exit.
+      if (process.stdout.writableLength) process.stdout.once('drain', done);
+      else done();
    });
  });
-  rl.on('close', () => { log('stdin closed — exiting'); process.exit(0); });
 }
@@ -32,8 +32,9 @@ export function findRepoRoot(start = process.cwd()) {
  return null;
 }

-// Dep-free PATH scan, memoized for the process lifetime (ADR-263 O8) — no
-// shell subprocess per lookup.
+// Dep-free PATH scan (ADR-263 O8) — no shell subprocess per lookup. Only hits
+// are memoized: a miss can resolve later in a long-lived MCP session (the
+// operator installs python/the CLI mid-run), so misses are re-probed each call.
 const whichCache = new Map();
 export function which(cmd) {
  if (whichCache.has(cmd)) return whichCache.get(cmd);
@@ -54,7 +55,7 @@ export function which(cmd) {
      } catch { /* keep scanning */ }
    }
  }
-  whichCache.set(cmd, found);
+  if (found !== null) whichCache.set(cmd, found);
  return found;
 }

@@ -99,3 +99,50 @@ test('MCP server answers ping while a long tools/call is in flight (ADR-263 O2)'
    rmSync(repo, { recursive: true, force: true });
  }
 });
+
+test('tools/call executions are serialized — two slow calls run sequentially', { skip: !which('python') && !which('python3') ? 'python not on PATH' : false }, async () => {
+  // Two verify.py that each sleep 0.8 s. Serialized ⇒ ~1.6 s+; concurrent ⇒ ~0.8 s.
+  const repo = mkdtempSync(join(tmpdir(), 'ruview-mcp-serial-'));
+  const proofDir = join(repo, 'archive', 'v1', 'data', 'proof');
+  mkdirSync(proofDir, { recursive: true });
+  writeFileSync(join(proofDir, 'verify.py'), 'import time\ntime.sleep(0.8)\nprint("VERDICT: PASS")\n');
+
+  const s = startServer();
+  try {
+    s.send({ jsonrpc: '2.0', id: 1, method: 'initialize', params: {} });
+    await s.next(1);
+
+    const t0 = Date.now();
+    const a = s.next(20);
+    const b = s.next(21);
+    s.send({ jsonrpc: '2.0', id: 20, method: 'tools/call', params: { name: 'ruview_verify', arguments: { repo } } });
+    s.send({ jsonrpc: '2.0', id: 21, method: 'tools/call', params: { name: 'ruview_verify', arguments: { repo } } });
+    const [ra, rb] = await Promise.all([a, b]);
+    const elapsed = Date.now() - t0;
+
+    assert.equal(JSON.parse(ra.result.content[0].text).verdict, 'PASS');
+    assert.equal(JSON.parse(rb.result.content[0].text).verdict, 'PASS');
+    assert.ok(elapsed > 1400, `two 0.8 s tool calls finished in ${elapsed} ms — they overlapped instead of serializing`);
+  } finally {
+    s.close();
+    rmSync(repo, { recursive: true, force: true });
+  }
+});
+
+test('stdin close flushes an in-flight tools/call response before exit', async () => {
+  const child = spawn(process.execPath, [CLI, 'mcp', 'start'], { stdio: ['pipe', 'pipe', 'pipe'] });
+  let out = '';
+  child.stdout.on('data', (d) => { out += d; });
+  const exited = new Promise((res) => child.on('exit', res));
+
+  // Write a tools/call then immediately close stdin. The old fire-and-forget
+  // dispatch raced rl 'close' → process.exit and could drop this response.
+  child.stdin.write(JSON.stringify({ jsonrpc: '2.0', id: 42, method: 'tools/call', params: { name: 'ruview_onboard', arguments: {} } }) + '\n');
+  child.stdin.end();
+
+  await exited;
+  const msgs = out.trim().split('\n').filter(Boolean).map((l) => JSON.parse(l));
+  const resp = msgs.find((m) => m.id === 42);
+  assert.ok(resp, 'the in-flight tools/call response must be flushed to stdout before exit');
+  assert.equal(resp.result.isError, false);
+});
@@ -4,8 +4,9 @@

 import { test } from 'node:test';
 import assert from 'node:assert/strict';
-import { readdirSync, readFileSync } from 'node:fs';
-import { join, dirname } from 'node:path';
+import { readdirSync, readFileSync, mkdtempSync, writeFileSync, rmSync } from 'node:fs';
+import { join, dirname, delimiter } from 'node:path';
+import { tmpdir } from 'node:os';
 import { fileURLToPath } from 'node:url';
 import { claimCheck, summarize } from '../src/guardrails.js';
 import { TOOLS, TOOL_ALIASES, runTool, listTools, findRepoRoot, run, which } from '../src/tools.js';
@@ -58,6 +59,33 @@ test('guardrail still catches real short-token metric claims', () => {
  assert.equal(claimCheck('IoU 0.75 across rooms.').ok, false);
 });

+// Digits hidden in a code span still make a claim — scrubbing must not blind the
+// number gate to `0.95` (regression: code-span number bypassed the gate).
+test('guardrail flags an accuracy number stated inside a code span', () => {
+  const r = claimCheck('Count accuracy reached `0.95` in our tests.');
+  assert.equal(r.ok, false, JSON.stringify(r.findings));
+  assert.ok(r.findings.some((f) => /not tagged/i.test(f.reason)));
+});
+
+// A MEASURED claim whose only number hides in a code span must still reach the
+// missing-reproducer check (regression: the scrubbed gate short-circuited it).
+// Bare metric prose with no number at all (e.g. the README rule text) stays a pass.
+test('guardrail flags a MEASURED code-span number with no reproducer', () => {
+  const r = claimCheck('Detection accuracy `0.97` on the set (MEASURED).');
+  assert.equal(r.ok, false, JSON.stringify(r.findings));
+  assert.ok(r.findings.some((f) => /no reproducer/i.test(f.reason)));
+  assert.equal(claimCheck('Every accuracy number must be MEASURED against a baseline.').ok, true);
+});
+
+// F1-score phrasings ("F1: 0.91", "F1 reaches 0.91") were scrubbed as option
+// labels and slipped through; option refs alone must still not false-positive.
+test('guardrail catches F1-score claims but not bare option refs (ADR-263 F11)', () => {
+  assert.equal(claimCheck('F1: 0.91 on the held-out set.').ok, false, 'F1: value is a metric claim');
+  assert.equal(claimCheck('F1 reaches 0.91 on the held-out set.').ok, false, 'F1 with a nearby number is a claim');
+  assert.equal(claimCheck('Options O1–O9 are tracked in ADR-263 O2.').ok, true, 'option labels are not metrics');
+  assert.equal(claimCheck('ADR-263 O2 lands the exports fix.').ok, true);
+});
+
 test('summarize gives PASS/finding text', () => {
  assert.match(summarize(claimCheck('nothing here')), /PASS/);
  assert.match(summarize(claimCheck('100% accuracy')), /finding/);
@@ -156,10 +184,29 @@ test('run() bounds captured output instead of dying on big streams (ADR-263 O4)'
  assert.ok(r.stdout.includes('TAIL_MARKER'), 'tail must keep the end of the stream');
 });

-test('which() finds node and memoizes misses', () => {
+test('which() finds node and re-probes misses (hits are cached)', () => {
  assert.ok(which('node'), 'node must be on PATH in the test env');
  assert.equal(which('definitely-not-a-binary-xyz'), null);
-  assert.equal(which('definitely-not-a-binary-xyz'), null); // cached path
+  assert.equal(which('definitely-not-a-binary-xyz'), null); // re-probed, still absent
+});
+
+// ADR-263 O8: a miss must not be cached — an operator who installs a tool
+// mid-session (e.g. python after a python_missing failure) must be found next call.
+test('which() re-probes after a miss so a newly-installed tool is found', () => {
+  const dir = mkdtempSync(join(tmpdir(), 'ruview-which-'));
+  const name = 'ruview-probe-xyz';
+  const isWin = process.platform === 'win32';
+  const bin = join(dir, isWin ? `${name}.cmd` : name);
+  const prevPath = process.env.PATH;
+  try {
+    assert.equal(which(name), null, 'not on PATH yet → miss');
+    writeFileSync(bin, isWin ? '@echo off\n' : '#!/bin/sh\n', { mode: 0o755 });
+    process.env.PATH = dir + delimiter + prevPath;
+    assert.ok(which(name), 'installed mid-session → the miss must not have been cached');
+  } finally {
+    process.env.PATH = prevPath;
+    rmSync(dir, { recursive: true, force: true });
+  }
 });

 test('CLI run(): claim-check exits non-zero on a bad claim', async () => {