From f5bf82cb2a6145fb3887332b79669d1dba9d21bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nathan=20=F0=9F=94=B6=20Tarbert?= <66887028+NathanTarbert@users.noreply.github.com> Date: Wed, 26 Aug 2026 09:21:36 -0400 Subject: [PATCH 1/2] feat(ai): add the draft linter, in report mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 3 of the response-quality work (CPK-8078), first slice. The doc's flowchart hangs on one arrow: "if the draft breaks a rule, it doesn't get cleaned up and posted — it collapses into the two-sentence version." This is that arrow, and it runs the same rules the harness scores with, from the same module, so the thing measured and the thing enforced cannot drift. ## The two prerequisites Jerel named on #241, which had to come first Both would have made the linter withhold correct answers, which is the same failure direction as the groundedness gate suppressing one — the bug #234 was filed for. 1. The citation rule tested whether a link LOOKED like ours, so the URL was both the citation and the laundering: a reply could write its invented hook name inside a docs.copilotkit.ai link — /hooks/useCopilotFabricated — and satisfy the rule with a page that does not exist. The identifier rule cannot catch it either, because assessGroundedness blanks URLs before it looks. The check now matches against the URLs actually retrieved. 2. Pathfinder's plain-text fallback (textSearch) sets sourceUrl: undefined on every result, so a CORRECT answer built from it has nothing it could cite. Under a flat requirement that answer fails forever and, once these rules gate publishing, collapses into a handoff every time the fallback is in play. Closing 2 needed a third state, so RuleResult gains `applicable`: "did not cite" and "had nothing citable" are different facts. A not-applicable rule is never a failure, never withholds a draft, and is not counted in a pass rate — which also fixes a rule that was inapplicable everywhere reading as a clean sweep, since `passed === total` is trivially true at 0/0. formatReport prints `n/a` for those rather than `ok`. ## Report mode is the default `lintDraft` computes the verdict and changes nothing unless asked to enforce. Enforcing means a misfiring rule withholds a correct answer from a real person, so the sequence is: run in report mode, read what it would have collapsed against real traffic, then enforce once the false-positive rate is known rather than assumed. `wouldCollapse` carries the counterfactual so report mode is worth running. It returns a verdict, never replacement copy. The caller substitutes its own, and in the pipeline that is the existing SUPPRESSED_RESPONSE_TEXT — that copy already promises a human follow-up, and #231 records what happens when two layers each add their own promise. ## Not wired into the pipeline here, deliberately The wiring belongs in pipeline.ts, which #242 is already editing on another branch. Landing both would collide over the same function for no benefit, since a report-mode linter changes nothing until someone reads its output. Wiring follows once #242 is in. Verification: ai package 318 -> 334, full repo turbo run test 10/10, typecheck clean. Four mutations, each killing the tests that name it: the laundering-permissive citation check, an always-applicable citation rule, report mode withholding, and enforce counting inapplicable rules as failures. Two tests were updated rather than patched around: both cited arbitrary docs-shaped URLs absent from their fixture's sources, which is precisely the laundering the new check closes. Stacked on #241 (feat/response-quality-eval-harness) because it consumes that rule module; rebases onto whatever that review lands. Refs CPK-8078 --- packages/outpost/ai/src/eval/harness.ts | 30 ++++-- packages/outpost/ai/src/eval/linter.test.ts | 105 ++++++++++++++++++++ packages/outpost/ai/src/eval/linter.ts | 102 +++++++++++++++++++ packages/outpost/ai/src/eval/rules.test.ts | 86 +++++++++++++++- packages/outpost/ai/src/eval/rules.ts | 56 +++++++++-- packages/outpost/ai/src/index.ts | 2 + 6 files changed, 358 insertions(+), 23 deletions(-) create mode 100644 packages/outpost/ai/src/eval/linter.test.ts create mode 100644 packages/outpost/ai/src/eval/linter.ts diff --git a/packages/outpost/ai/src/eval/harness.ts b/packages/outpost/ai/src/eval/harness.ts index 6755625..db2dcd4 100644 --- a/packages/outpost/ai/src/eval/harness.ts +++ b/packages/outpost/ai/src/eval/harness.ts @@ -76,18 +76,26 @@ export function scoreCases(cases: EvalCase[]): EvalReport { return { id: c.id, results, - failed: results.filter((r) => !r.passed).map((r) => r.rule), + // A rule that could not be evaluated is not a failure. + failed: results.filter((r) => r.applicable && !r.passed).map((r) => r.rule), }; }); + // `total` counts only the cases where the rule could be evaluated, so a rule + // that was inapplicable everywhere reads as 0/0 rather than as a clean sweep. const perRule = Object.fromEntries( - RULES.map((rule) => [ - rule, - { - passed: scored.filter((s) => !s.failed.includes(rule)).length, - total: scored.length, - }, - ]), + RULES.map((rule) => { + const applicable = scored.filter( + (s) => s.results.find((r) => r.rule === rule)?.applicable, + ); + return [ + rule, + { + passed: applicable.filter((s) => !s.failed.includes(rule)).length, + total: applicable.length, + }, + ]; + }), ) as Record; return { @@ -103,7 +111,11 @@ export function formatReport(report: EvalReport): string { const lines = [`${report.cleanCases}/${report.totalCases} cases clean`, '']; for (const rule of RULES) { const { passed, total } = report.perRule[rule]; - lines.push(` ${passed === total ? 'ok ' : 'FAIL'} ${rule}: ${passed}/${total}`); + // `n/a` rather than `ok` when nothing exercised the rule — `passed === total` + // is trivially true at 0/0, which is how an unevaluated rule used to read as + // a clean sweep. + const verdict = total === 0 ? 'n/a ' : passed === total ? 'ok ' : 'FAIL'; + lines.push(` ${verdict} ${rule}: ${passed}/${total}`); } const dirty = report.cases.filter((c) => c.failed.length > 0); if (dirty.length) { diff --git a/packages/outpost/ai/src/eval/linter.test.ts b/packages/outpost/ai/src/eval/linter.test.ts new file mode 100644 index 0000000..62f390c --- /dev/null +++ b/packages/outpost/ai/src/eval/linter.test.ts @@ -0,0 +1,105 @@ +import { describe, it, expect } from 'vitest'; +import { lintDraft, describeVerdict } from './linter.js'; +import type { SearchResult } from '../types.js'; + +const DOCS: SearchResult[] = [ + { + title: 'CopilotChat', + content: 'Use the `CopilotChat` component with the `instructions` prop.', + score: 0.9, + sourceUrl: 'https://docs.copilotkit.ai/reference/components/chat/CopilotChat', + }, +]; + +/** A reply that breaks nothing: grounded, cited, no banned phrasing. */ +const GOOD = + 'Use the `CopilotChat` component with the `instructions` prop. ' + + 'https://docs.copilotkit.ai/reference/components/chat/CopilotChat'; + +/** Case D's shape: praise opener, self-commentary, no citation, over the cap. */ +const BAD = + 'Great question! Here is what I cannot do from here: I cannot read the source. ' + + 'To summarise what you have found, the versions disagree. '.repeat(6); + +describe('report mode', () => { + it('is the default, so wiring it in cannot change what publishes', () => { + expect(lintDraft(BAD, DOCS).mode).toBe('report'); + }); + + // The whole point of the mode: learn what enforcement would do to real + // traffic before letting it withhold anything from a real person. + it('publishes a failing draft while recording that it would not have', () => { + const verdict = lintDraft(BAD, DOCS); + + expect(verdict.publish).toBe(true); + expect(verdict.wouldCollapse).toBe(true); + expect(verdict.failed).toContain('no-banned-phrases'); + }); + + it('publishes a clean draft and says nothing would have collapsed', () => { + const verdict = lintDraft(GOOD, DOCS); + + expect(verdict.publish).toBe(true); + expect(verdict.wouldCollapse).toBe(false); + expect(verdict.failed).toEqual([]); + }); +}); + +describe('enforce mode', () => { + it('withholds a failing draft', () => { + const verdict = lintDraft(BAD, DOCS, 'enforce'); + + expect(verdict.publish).toBe(false); + expect(verdict.wouldCollapse).toBe(true); + }); + + it('publishes a clean draft', () => { + expect(lintDraft(GOOD, DOCS, 'enforce').publish).toBe(true); + }); + + // A rule that could not be evaluated must never withhold an answer. The live + // case: Pathfinder's plain-text fallback returns results with no sourceUrl, so + // a correct answer built from it has nothing it could cite — enforcing a + // citation there would collapse every such answer. + it('does not withhold on a rule that could not be evaluated', () => { + const urlLess: SearchResult[] = [ + { title: 'CopilotChat', content: 'Use the `CopilotChat` component.', score: 0.9 }, + ]; + const long = 'Use the `CopilotChat` component as documented. '.repeat(12); + + const verdict = lintDraft(long, urlLess, 'enforce'); + + expect(verdict.failed).not.toContain('cites-or-is-a-short-handoff'); + expect(verdict.publish).toBe(true); + }); + + it('reports every rule either way, so a log line can show the whole picture', () => { + expect(lintDraft(GOOD, DOCS, 'enforce').results).toHaveLength(6); + }); +}); + +describe('the reasons', () => { + it('name the rule and the offending text', () => { + const verdict = lintDraft('Great question! ' + 'padding word '.repeat(10), DOCS); + + expect(verdict.reasons.join(' ')).toContain('no-banned-phrases'); + expect(verdict.reasons.join(' ')).toContain('praise opener'); + }); +}); + +describe('describeVerdict', () => { + it('distinguishes a report-mode near-miss from an enforced collapse', () => { + expect(describeVerdict(lintDraft(BAD, DOCS), 'ticket-1')).toContain( + 'published anyway (report mode)', + ); + expect(describeVerdict(lintDraft(BAD, DOCS, 'enforce'), 'ticket-1')).toContain( + 'collapsed to a handoff', + ); + }); + + it('stays quiet-but-informative on a clean draft', () => { + expect(describeVerdict(lintDraft(GOOD, DOCS), 'ticket-1')).toBe( + '[DraftLinter] ticket-1: clean (report)', + ); + }); +}); diff --git a/packages/outpost/ai/src/eval/linter.ts b/packages/outpost/ai/src/eval/linter.ts new file mode 100644 index 0000000..6d7809b --- /dev/null +++ b/packages/outpost/ai/src/eval/linter.ts @@ -0,0 +1,102 @@ +/** + * The draft linter — step 3 of "Fix the Agent's Output". + * + * The doc's flowchart hangs on one arrow: *"If the draft breaks a rule, it + * doesn't get cleaned up and posted — it collapses into the two-sentence + * version. A short honest reply is always the fallback."* This module is that + * arrow. It runs the same rules the eval harness scores with — deliberately the + * same module, so the thing measured and the thing enforced cannot drift — and + * returns a decision. + * + * ## It returns a verdict, not copy + * + * `lintDraft` never produces replacement text. The caller substitutes its own, + * and in the pipeline that is the existing `SUPPRESSED_RESPONSE_TEXT` used by the + * groundedness gate. That is deliberate: that copy already promises a human + * follow-up, and #231 records what happens when two layers each add their own + * promise — the reporter is told twice. One owner for user-facing copy, one + * promise. + * + * ## Report-only by default + * + * `mode` defaults to `'report'`, which computes the verdict and changes nothing. + * Enforcing means a rule that misfires withholds a correct answer from a real + * person — the same failure direction as the groundedness gate suppressing one, + * which is the bug #234 was filed for. So the sequence is: run in report mode, + * read what it would have collapsed against real traffic, then enforce once the + * false-positive rate is known rather than assumed. + * + * A rule that could not be evaluated never collapses a draft. See + * `RuleResult.applicable` — Pathfinder's plain-text fallback returns results with + * no `sourceUrl` at all, so a correct answer built from it has nothing it could + * cite, and enforcing a citation there would collapse every such answer. + */ + +import { checkReply } from './rules.js'; +import type { RuleId, RuleResult } from './rules.js'; +import type { SearchResult } from '../types.js'; + +export type LintMode = 'report' | 'enforce'; + +export interface LintVerdict { + /** + * Whether the caller should publish the draft. + * + * Always true in `'report'` mode, whatever the rules found — reporting is for + * learning what enforcement would do, so it must not change behaviour. + */ + publish: boolean; + /** True when at least one applicable rule failed, regardless of mode. */ + wouldCollapse: boolean; + /** The applicable rules the draft broke, in rule order. */ + failed: RuleId[]; + /** One line per failure, naming the offending text. */ + reasons: string[]; + /** Every rule's result, for logging and for the eval harness. */ + results: RuleResult[]; + mode: LintMode; +} + +/** + * Judge a draft against the reply rules. + * + * `sources` must be the results the draft was generated from — the citation and + * identifier rules are lookups against them, so passing a different set silently + * turns the two strictest rules into no-ops. + */ +export function lintDraft( + reply: string, + sources: SearchResult[], + mode: LintMode = 'report', +): LintVerdict { + const results = checkReply(reply, sources); + const broken = results.filter((r) => r.applicable && !r.passed); + + return { + publish: mode === 'report' ? true : broken.length === 0, + wouldCollapse: broken.length > 0, + failed: broken.map((r) => r.rule), + reasons: broken.map((r) => `${r.rule}: ${r.detail}`), + results, + mode, + }; +} + +/** + * One log line describing what the linter decided, for report-mode runs. + * + * Report mode is only worth anything if somebody can read the outcome, and #197 + * is why this returns a string for the caller to log rather than reaching for a + * logger itself: nothing in this package has adopted the structured logger, so a + * caller-owned `console` line is the honest option rather than inventing a + * second convention here. + */ +export function describeVerdict(verdict: LintVerdict, context: string): string { + if (!verdict.wouldCollapse) { + return `[DraftLinter] ${context}: clean (${verdict.mode})`; + } + const action = verdict.publish + ? 'would have collapsed to a handoff, published anyway (report mode)' + : 'collapsed to a handoff'; + return `[DraftLinter] ${context}: ${action} — ${verdict.reasons.join(' | ')}`; +} diff --git a/packages/outpost/ai/src/eval/rules.test.ts b/packages/outpost/ai/src/eval/rules.test.ts index a4f157c..27e432a 100644 --- a/packages/outpost/ai/src/eval/rules.test.ts +++ b/packages/outpost/ai/src/eval/rules.test.ts @@ -14,7 +14,8 @@ const DOCS = [source('Use the `CopilotChat` component with the `instructions` pr /** Convenience: the ids of every rule the reply violated. */ const broken = (reply: string, sources: SearchResult[] = DOCS): string[] => checkReply(reply, sources) - .filter((r) => !r.passed) + // A not-applicable rule is not a failure. See RuleResult.applicable. + .filter((r) => r.applicable && !r.passed) .map((r) => r.rule); describe('checkReply', () => { @@ -67,18 +68,30 @@ describe('cites-or-is-a-short-handoff', () => { expect(broken(wordy)).toContain('cites-or-is-a-short-handoff'); }); - it('passes a long answer that links the docs', () => { + // Both of these cite the URL the fixture actually retrieved. An arbitrary + // docs-shaped URL no longer counts — see "the citation rule consults the + // retrieved sources" below for why. + it('passes a long answer that links the docs page it was given', () => { const wordy = 'You can configure this in several ways. '.repeat(12) + - ' https://docs.copilotkit.ai/guides/configuration'; + ' https://docs.copilotkit.ai/reference/components/chat/CopilotChat'; expect(broken(wordy)).not.toContain('cites-or-is-a-short-handoff'); }); - it('passes a long answer that links a repo file, since code is a real answer', () => { + it('passes a long answer that links a retrieved repo file, since code is a real answer', () => { + const CODE = [ + { + title: 'packages/runtime/src/agent.ts', + content: 'export function streamSubgraphEvents() {}', + score: 0.9, + sourceUrl: + 'https://github.com/CopilotKit/CopilotKit/blob/main/packages/runtime/src/agent.ts', + }, + ]; const wordy = 'This is handled by the adapter. '.repeat(12) + ' https://github.com/CopilotKit/CopilotKit/blob/main/packages/runtime/src/agent.ts'; - expect(broken(wordy)).not.toContain('cites-or-is-a-short-handoff'); + expect(broken(wordy, CODE)).not.toContain('cites-or-is-a-short-handoff'); }); it('passes a short handoff with no link at all', () => { @@ -90,6 +103,69 @@ describe('cites-or-is-a-short-handoff', () => { // "Nothing found -> two sentences, done." A no-answer that runs to 400 words is // the single thing the doc says changes the feel of the product most. +// Both of these block promoting the rules to a linter, where a false positive +// costs a reporter a correct answer. +describe('the citation rule consults the retrieved sources', () => { + const wordy = (tail: string) => 'You can configure this in several ways. '.repeat(12) + tail; + + // The URL was both the citation and the laundering: a reply could write its + // invented hook name INSIDE a docs.copilotkit.ai link and satisfy the rule, + // because the rule only checked that the link looked like ours. + it('rejects a link that matches no retrieved source', () => { + expect( + broken(wordy('See https://docs.copilotkit.ai/hooks/useCopilotFabricated'), DOCS), + ).toContain('cites-or-is-a-short-handoff'); + }); + + it('accepts a link that matches a retrieved source', () => { + expect( + broken( + wordy('See https://docs.copilotkit.ai/reference/components/chat/CopilotChat'), + DOCS, + ), + ).not.toContain('cites-or-is-a-short-handoff'); + }); + + it('accepts a retrieved source URL carrying an anchor or query', () => { + expect( + broken( + wordy( + 'See https://docs.copilotkit.ai/reference/components/chat/CopilotChat#slots', + ), + DOCS, + ), + ).not.toContain('cites-or-is-a-short-handoff'); + }); +}); + +// Pathfinder's plain-text fallback (`textSearch`) sets sourceUrl: undefined on +// every result, so under a flat requirement a CORRECT answer built from it could +// never cite and would always collapse into a handoff. The rule has to know the +// difference between "did not cite" and "had nothing citable". +describe('when no retrieved source carries a URL', () => { + const URL_LESS = [{ title: 'CopilotChat', content: 'Use the `CopilotChat` component.', score: 0.9 }]; + + it('marks the citation rule not applicable rather than failing it', () => { + const result = checkReply( + 'You can configure this in several ways. '.repeat(12), + URL_LESS, + ).find((r) => r.rule === 'cites-or-is-a-short-handoff'); + + expect(result?.applicable).toBe(false); + expect(result?.detail).toMatch(/no retrieved source carries a url/i); + }); + + it('does not count a not-applicable rule as a failure', () => { + expect(broken('You can configure this in several ways. '.repeat(12), URL_LESS)).toEqual([]); + }); + + it('still applies every other rule', () => { + expect(broken('Great question! Call `useCopilotFabricated()`.', URL_LESS)).toEqual( + expect.arrayContaining(['no-banned-phrases', 'grounded-identifiers']), + ); + }); +}); + describe('the handoff cap', () => { it('fails a handoff that pads past the cap', () => { const padded = diff --git a/packages/outpost/ai/src/eval/rules.ts b/packages/outpost/ai/src/eval/rules.ts index 86f1c50..ff4b824 100644 --- a/packages/outpost/ai/src/eval/rules.ts +++ b/packages/outpost/ai/src/eval/rules.ts @@ -137,9 +137,22 @@ const DEAD_PACKAGE = /@copilotkitnext\b/i; */ const LIVE_PACKAGE = /@copilotkit\/[a-z-]+/i; -/** A link that constitutes a citation: a docs page or a file in the repo. */ -const CITATION_LINK = - /https?:\/\/(?:[a-z0-9-]+\.)*(?:copilotkit\.ai|github\.com\/CopilotKit|github\.com\/ag-ui-protocol)\/\S+/i; +/** + * True when the reply links a source it was actually given. + * + * Checked against the retrieved `sources` rather than against a pattern for + * "looks like one of our URLs". Under a pattern test the URL was both the + * citation and the laundering: a reply could write its invented hook name + * *inside* a `docs.copilotkit.ai` link — `/hooks/useCopilotFabricated` — and + * satisfy the rule with a page that does not exist. The identifier rule cannot + * catch that either, because `assessGroundedness` blanks URLs before it looks. + * + * Substring rather than equality, so a cited URL may carry an anchor or a query + * the retrieved one did not (`…/CopilotChat#slots`). + */ +function citesARetrievedSource(reply: string, sources: SearchResult[]): boolean { + return sources.some((s) => s.sourceUrl && reply.includes(s.sourceUrl)); +} export const RULES = [ 'says-something', @@ -155,7 +168,20 @@ export type RuleId = (typeof RULES)[number]; export interface RuleResult { rule: RuleId; passed: boolean; - /** Why it failed, naming the offending text. Empty when it passed. */ + /** + * False when the rule could not be evaluated at all, as opposed to evaluated + * and passed. A not-applicable rule is never a failure and is never counted + * in a pass rate. + * + * The case that forced the distinction: Pathfinder's plain-text fallback + * (`textSearch`) sets `sourceUrl: undefined` on every result, so a CORRECT + * answer built from it has nothing it could possibly cite. Under a flat + * requirement that answer fails the citation rule forever and — once these + * rules gate publishing — collapses into a handoff every time the fallback is + * in play. "Did not cite" and "had nothing citable" are different facts. + */ + applicable: boolean; + /** Why it failed, or why it was not applicable. Empty when it passed. */ detail: string; } @@ -174,7 +200,9 @@ function countWords(text: string): number { */ export function checkReply(reply: string, sources: SearchResult[]): RuleResult[] { const words = countWords(reply); - const cites = CITATION_LINK.test(reply); + const cites = citesARetrievedSource(reply, sources); + // Whether citing was possible at all. See RuleResult.applicable. + const anySourceHasUrl = sources.some((s) => !!s.sourceUrl); // A reply that cites nothing is only acceptable as a short handoff, so the // two rules below are the two halves of that single sentence in the doc. @@ -187,6 +215,7 @@ export function checkReply(reply: string, sources: SearchResult[]): RuleResult[] return [ { rule: 'says-something', + applicable: true, passed: words >= MIN_REPLY_WORDS, detail: words >= MIN_REPLY_WORDS @@ -195,6 +224,7 @@ export function checkReply(reply: string, sources: SearchResult[]): RuleResult[] }, { rule: 'grounded-identifiers', + applicable: true, passed: groundedness.unsourcedIdentifiers.length === 0, detail: groundedness.unsourcedIdentifiers.length ? `names not present in any source: ${groundedness.unsourcedIdentifiers.join(', ')}` @@ -206,24 +236,32 @@ export function checkReply(reply: string, sources: SearchResult[]): RuleResult[] // which presented five independent signals as six and double-counted // every failure in both the per-rule table and the report. rule: 'cites-or-is-a-short-handoff', + // A short handoff needs no citation, so the length escape keeps the + // rule applicable even with nothing citable. It only goes + // not-applicable when the reply is long AND there was no URL to cite. + applicable: anySourceHasUrl || isShortEnoughForHandoff, passed: cites || isShortEnoughForHandoff, - detail: - cites || isShortEnoughForHandoff - ? '' - : `${words} words with no docs or repo link, over the ${HANDOFF_WORD_CAP}-word handoff cap; a reply this long has to cite what it came from`, + detail: !anySourceHasUrl && !isShortEnoughForHandoff + ? 'not evaluated: no retrieved source carries a URL, so nothing could be cited' + : cites || isShortEnoughForHandoff + ? '' + : `${words} words and no link to a retrieved source, over the ${HANDOFF_WORD_CAP}-word handoff cap; a reply this long has to cite what it came from`, }, { rule: 'no-banned-phrases', + applicable: true, passed: banned.length === 0, detail: banned.map(({ why }) => why).join('; '), }, { rule: 'no-hedged-names', + applicable: true, passed: hedged.length === 0, detail: hedged.length ? 'hedges an API name, which means it is guessing' : '', }, { rule: 'no-dead-package', + applicable: true, passed: !DEAD_PACKAGE.test(reply) || LIVE_PACKAGE.test(reply), detail: DEAD_PACKAGE.test(reply) && !LIVE_PACKAGE.test(reply) diff --git a/packages/outpost/ai/src/index.ts b/packages/outpost/ai/src/index.ts index 071cbcd..8c85ac6 100644 --- a/packages/outpost/ai/src/index.ts +++ b/packages/outpost/ai/src/index.ts @@ -56,5 +56,7 @@ export type { export { checkReply, RULES, HANDOFF_WORD_CAP, MIN_REPLY_WORDS } from './eval/rules.js'; export type { RuleId, RuleResult } from './eval/rules.js'; export { scoreCases, formatReport } from './eval/harness.js'; +export { lintDraft, describeVerdict } from './eval/linter.js'; +export type { LintMode, LintVerdict } from './eval/linter.js'; export type { EvalCase, CaseScore, EvalReport } from './eval/harness.js'; export * from './types.js'; From 6c4336ef9afa151ba55a33dbedcc86de4ee9f8ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nathan=20=F0=9F=94=B6=20Tarbert?= <66887028+NathanTarbert@users.noreply.github.com> Date: Wed, 26 Aug 2026 09:36:12 -0400 Subject: [PATCH 2/2] fix(ai): separate the quality metric from the publish gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the review of the draft linter. Six code findings, all confirmed by running the real functions first, plus one comment that asserted a guarantee it did not deliver. ## The one that would have shipped a hole `applicable` treated "retrieval returned nothing" the same as "retrieval returned results that carry no URL". With `sources: []` the citation rule went not-applicable and could not fail, so a long uncited reply built on zero retrieval published under enforcement — the doc's Case A exactly, and the one input where citing matters most. The `applicable` escape exists for Pathfinder's plain-text fallback, which returns results WITH `sourceUrl: undefined`; an empty result set is a different fact and still fails. The branch's own case-A fixture only avoided this by being 48 words, under the handoff cap, so nothing pinned it. Pinned now. ## The metric and the gate are not the same thing The linter failed `grounded-identifiers` at one unsourced identifier, while `groundedness.ts` suppresses at SUPPRESS_AT_UNSOURCED_IDENTIFIERS = 2 — reasoning, in its own comment, that "one could be a formatting artifact; two is a pattern of fabrication". So the linter was stricter than the pipeline it sits beside, which is the false-withholding direction both modules warn about. Rather than pick one, `RuleResult` now carries `blocksPublish` alongside `passed`. The doc's success criterion is ZERO invented API names, so one occurrence still fails the metric and shows up in a score; the publish gate uses the pipeline's own threshold, so one does not withhold the answer. For every other rule the two agree. The harness scores `passed`; the linter gates on `blocksPublish`. ## The laundering had only moved a level deeper `reply.includes(sourceUrl)` accepted anything APPENDED to a retrieved URL, and retrieval routinely returns section and index URLs — so citing `…/reference/hooks/useCopilotFabricated` satisfied a retrieved `…/reference`. The match now requires the URL to end at a boundary, keeping the anchor and query tolerance it was written for. Scheme and host are also lowercased on both sides: they are case-insensitive in practice, and a case-sensitive compare withheld a correctly-cited answer. ## The dead-package carve-out was a no-op on its own worst case Naming any live `@copilotkit/` package excused the dead one, with no requirement that the two be related. "Install `@copilotkit/react-core` and also add `@copilotkitnext/react` for the newer surface" passed — and naming both as if both were current IS the version-mixing failure Case B documents. The carve-out now also requires migration framing ("merged into", "switch the import", …), so the migration answer still gets through and a mixed-version answer does not. ## Also - The failing-cases block in `formatReport` did not filter on `applicable`, so it printed `n/a` for a rule in the per-rule table and then listed that same rule as a failure two lines later — the double-counting removed from `perRule`, re-created in the human-readable output. - The `index.ts` comment claimed that not re-exporting the fixtures kept them out of `dist` and the worker image. It does not: `tsc` emits per file and `index.ts` imports `./eval/harness.js`, so `dist/eval/harness.js` ships `HISTORICAL_FAILURES` with both strings intact. Comment corrected to say what is actually true and what closing it would take. Verification: ai package 334 -> 345, full repo turbo run test 10/10, typecheck clean. Six mutations, each killing the tests that name it. The failing-cases-filter mutation initially SURVIVED — nothing pinned the report output — so that test was added and the mutation now dies. Refs CPK-8078 --- packages/outpost/ai/src/eval/harness.test.ts | 32 +++++ packages/outpost/ai/src/eval/harness.ts | 7 +- packages/outpost/ai/src/eval/linter.test.ts | 36 ++++++ packages/outpost/ai/src/eval/linter.ts | 6 +- packages/outpost/ai/src/eval/rules.test.ts | 89 +++++++++++++- packages/outpost/ai/src/eval/rules.ts | 119 ++++++++++++++++--- packages/outpost/ai/src/index.ts | 17 ++- 7 files changed, 277 insertions(+), 29 deletions(-) diff --git a/packages/outpost/ai/src/eval/harness.test.ts b/packages/outpost/ai/src/eval/harness.test.ts index 32d45ec..853db09 100644 --- a/packages/outpost/ai/src/eval/harness.test.ts +++ b/packages/outpost/ai/src/eval/harness.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect } from 'vitest'; import { scoreCases, formatReport, HISTORICAL_FAILURES, TARGET_SHAPE } from './harness.js'; import { RULES } from './rules.js'; +import type { SearchResult } from '../types.js'; describe('scoreCases', () => { it('reports every rule for every case', () => { @@ -73,6 +74,37 @@ describe('an empty case list', () => { }); }); +// A not-applicable rule carries `passed: false`, so an unfiltered failing-cases +// block printed `n/a` for a rule and then listed it as a failure two lines later — +// re-creating the double-counting in the human-readable output. +describe('the report does not list an unevaluated rule as a failure', () => { + const urlLess: SearchResult[] = [ + { title: 'CopilotChat', content: 'Use the `CopilotChat` component.', score: 0.9 }, + ]; + const report = () => + formatReport( + scoreCases([ + { + id: 'c1', + question: 'q', + reply: 'Great question! ' + 'padding word '.repeat(30), + sources: urlLess, + provenance: 'test', + }, + ]), + ); + + it('marks the citation rule n/a in the per-rule table', () => { + expect(report()).toContain('n/a cites-or-is-a-short-handoff: 0/0'); + }); + + it('does not repeat it under the failing cases', () => { + const failingBlock = report().split('Failing cases:')[1] ?? ''; + expect(failingBlock).toContain('no-banned-phrases'); + expect(failingBlock).not.toContain('cites-or-is-a-short-handoff'); + }); +}); + describe('formatReport', () => { it('leads with the clean count and names each failing rule', () => { const text = formatReport(scoreCases(HISTORICAL_FAILURES)); diff --git a/packages/outpost/ai/src/eval/harness.ts b/packages/outpost/ai/src/eval/harness.ts index db2dcd4..2cc51f2 100644 --- a/packages/outpost/ai/src/eval/harness.ts +++ b/packages/outpost/ai/src/eval/harness.ts @@ -122,7 +122,12 @@ export function formatReport(report: EvalReport): string { lines.push('', 'Failing cases:'); for (const c of dirty) { lines.push(` ${c.id}`); - for (const r of c.results.filter((r) => !r.passed)) { + // Filtered on `applicable` as well: a not-applicable rule carries + // `passed: false`, so without this the report printed `n/a` for a rule + // two lines above and then listed it as a failure — re-creating in the + // human-readable output exactly the double-counting removed from + // `perRule`. + for (const r of c.results.filter((r) => r.applicable && !r.passed)) { lines.push(` - ${r.rule}: ${r.detail}`); } } diff --git a/packages/outpost/ai/src/eval/linter.test.ts b/packages/outpost/ai/src/eval/linter.test.ts index 62f390c..e4efd2c 100644 --- a/packages/outpost/ai/src/eval/linter.test.ts +++ b/packages/outpost/ai/src/eval/linter.test.ts @@ -103,3 +103,39 @@ describe('describeVerdict', () => { ); }); }); + +// The harness scores the doc's metric — zero invented API names — while the +// pipeline's groundedness gate suppresses only at two. A linter that collapsed at +// one would withhold answers production publishes. +describe('the metric and the publish gate are not the same', () => { + const SRC: SearchResult[] = [ + { + title: 'CopilotChat', + content: 'Use the `CopilotChat` component.', + score: 0.9, + sourceUrl: 'https://docs.copilotkit.ai/reference', + }, + ]; + const cite = ' https://docs.copilotkit.ai/reference'; + + it('fails the metric on one invented name but still publishes', () => { + const reply = + 'Call `useCopilotFabricated()` with CopilotChat as documented in the reference guide.' + + cite; + const verdict = lintDraft(reply, SRC, 'enforce'); + + expect(verdict.results.find((r) => r.rule === 'grounded-identifiers')?.passed).toBe(false); + expect(verdict.publish).toBe(true); + expect(verdict.failed).not.toContain('grounded-identifiers'); + }); + + it('withholds at the production threshold of two', () => { + const reply = + 'Call `useCopilotFabricated()` and `` as documented in the guide.' + + cite; + const verdict = lintDraft(reply, SRC, 'enforce'); + + expect(verdict.publish).toBe(false); + expect(verdict.failed).toContain('grounded-identifiers'); + }); +}); diff --git a/packages/outpost/ai/src/eval/linter.ts b/packages/outpost/ai/src/eval/linter.ts index 6d7809b..87abb9f 100644 --- a/packages/outpost/ai/src/eval/linter.ts +++ b/packages/outpost/ai/src/eval/linter.ts @@ -70,7 +70,11 @@ export function lintDraft( mode: LintMode = 'report', ): LintVerdict { const results = checkReply(reply, sources); - const broken = results.filter((r) => r.applicable && !r.passed); + // Gates on `blocksPublish`, not on `passed`. The harness scores the doc's + // metric — zero invented API names — while the pipeline's groundedness gate + // suppresses only at two, and a linter that collapsed at one would withhold + // answers production publishes. See RuleResult.blocksPublish. + const broken = results.filter((r) => r.applicable && r.blocksPublish); return { publish: mode === 'report' ? true : broken.length === 0, diff --git a/packages/outpost/ai/src/eval/rules.test.ts b/packages/outpost/ai/src/eval/rules.test.ts index 27e432a..14e47d1 100644 --- a/packages/outpost/ai/src/eval/rules.test.ts +++ b/packages/outpost/ai/src/eval/rules.test.ts @@ -129,9 +129,7 @@ describe('the citation rule consults the retrieved sources', () => { it('accepts a retrieved source URL carrying an anchor or query', () => { expect( broken( - wordy( - 'See https://docs.copilotkit.ai/reference/components/chat/CopilotChat#slots', - ), + wordy('See https://docs.copilotkit.ai/reference/components/chat/CopilotChat#slots'), DOCS, ), ).not.toContain('cites-or-is-a-short-handoff'); @@ -142,8 +140,89 @@ describe('the citation rule consults the retrieved sources', () => { // every result, so under a flat requirement a CORRECT answer built from it could // never cite and would always collapse into a handoff. The rule has to know the // difference between "did not cite" and "had nothing citable". +// Zero retrieval and retrieval-without-URLs are different facts. Treating them +// the same let the doc's Case A — a long, uncited reply built on nothing — publish +// under enforcement, which is the single input where citing matters most. +describe('when retrieval returned nothing at all', () => { + it('still requires a citation, so a long uncited non-answer fails', () => { + const failed = broken( + 'Deep Agents probably supports subagents in some form. '.repeat(8), + [], + ); + expect(failed).toContain('cites-or-is-a-short-handoff'); + }); + + it('still lets a short handoff through', () => { + expect( + broken('Nothing in the docs or source covers this. Routing it to the team.', []), + ).toEqual([]); + }); +}); + +// A bare `includes` accepted anything appended to a retrieved URL, so the +// laundering just moved a level deeper — retrieval routinely returns section and +// index URLs. +describe('citation boundaries', () => { + const SECTION = [ + { + title: 'Reference', + content: 'Use the `CopilotChat` component.', + score: 0.9, + sourceUrl: 'https://docs.copilotkit.ai/reference', + }, + ]; + const wordy = (tail: string) => 'You can configure this in several ways. '.repeat(12) + tail; + + it('rejects an invented path appended to a retrieved URL', () => { + expect( + broken( + wordy('See https://docs.copilotkit.ai/reference/hooks/useCopilotFabricated'), + SECTION, + ), + ).toContain('cites-or-is-a-short-handoff'); + }); + + it('accepts the retrieved URL itself, with an anchor', () => { + expect( + broken(wordy('See https://docs.copilotkit.ai/reference#slots'), SECTION), + ).not.toContain('cites-or-is-a-short-handoff'); + }); + + // Scheme and host are case-insensitive in practice, and both models and + // reporters echo mixed-case hostnames. A case-sensitive compare withheld a + // correctly-cited answer. + it('accepts a mixed-case scheme and host', () => { + expect(broken(wordy('See HTTPS://DOCS.COPILOTKIT.AI/reference'), SECTION)).not.toContain( + 'cites-or-is-a-short-handoff', + ); + }); +}); + +// Naming a live package alone excused the dead one with no requirement that the +// two be related — which waved through Case B's own failure mode, since naming +// both as if both were current IS version-mixing. +describe('the dead-package carve-out needs migration framing', () => { + it('rejects naming both packages as if both were current', () => { + expect( + broken( + 'Install `@copilotkit/react-core` and also add `@copilotkitnext/react` for the newer surface here.', + ), + ).toContain('no-dead-package'); + }); + + it('accepts a genuine migration instruction', () => { + expect( + broken( + "You're importing `@copilotkitnext/react`, which merged into `@copilotkit/react-core` v2 — switch the import.", + ), + ).not.toContain('no-dead-package'); + }); +}); + describe('when no retrieved source carries a URL', () => { - const URL_LESS = [{ title: 'CopilotChat', content: 'Use the `CopilotChat` component.', score: 0.9 }]; + const URL_LESS = [ + { title: 'CopilotChat', content: 'Use the `CopilotChat` component.', score: 0.9 }, + ]; it('marks the citation rule not applicable rather than failing it', () => { const result = checkReply( @@ -152,7 +231,7 @@ describe('when no retrieved source carries a URL', () => { ).find((r) => r.rule === 'cites-or-is-a-short-handoff'); expect(result?.applicable).toBe(false); - expect(result?.detail).toMatch(/no retrieved source carries a url/i); + expect(result?.detail).toMatch(/none carries a url/i); }); it('does not count a not-applicable rule as a failure', () => { diff --git a/packages/outpost/ai/src/eval/rules.ts b/packages/outpost/ai/src/eval/rules.ts index ff4b824..0471ff6 100644 --- a/packages/outpost/ai/src/eval/rules.ts +++ b/packages/outpost/ai/src/eval/rules.ts @@ -137,6 +137,19 @@ const DEAD_PACKAGE = /@copilotkitnext\b/i; */ const LIVE_PACKAGE = /@copilotkit\/[a-z-]+/i; +/** + * The migration framing that makes naming the dead package legitimate. + * + * Requiring only that a live `@copilotkit/` package appear somewhere was too + * loose: it excused the dead one with no requirement that the two be related, + * which waved through the exact failure Case B documents. "Install + * `@copilotkit/react-core` and also add `@copilotkitnext/react` for the newer + * surface" passed — and naming both packages as if both were current IS + * version-mixing, so the rule became a no-op on its own worst case. + */ +const MIGRATION_FRAMING = + /\b(?:merged into|replaced by|moved to|superseded by|switch (?:the )?(?:import|to)|instead of|use .{0,20}instead|no longer (?:exists|published|maintained)|is (?:dead|retired|deprecated))\b/i; + /** * True when the reply links a source it was actually given. * @@ -151,7 +164,38 @@ const LIVE_PACKAGE = /@copilotkit\/[a-z-]+/i; * the retrieved one did not (`…/CopilotChat#slots`). */ function citesARetrievedSource(reply: string, sources: SearchResult[]): boolean { - return sources.some((s) => s.sourceUrl && reply.includes(s.sourceUrl)); + // Scheme and host lowercased on both sides: they are case-insensitive in + // practice, and models and reporters both echo mixed-case hostnames. A + // case-sensitive compare withheld a correctly-cited answer. + const normalise = (text: string) => + text.replace(/[a-z]+:\/\/[^/\s]+/gi, (m) => m.toLowerCase()); + const haystack = normalise(reply); + + return sources.some((s) => { + if (!s.sourceUrl) return false; + const needle = normalise(s.sourceUrl).replace(/[/#?]+$/, ''); + let from = 0; + for (;;) { + const at = haystack.indexOf(needle, from); + if (at === -1) return false; + // A bare `includes` accepted anything APPENDED to a retrieved URL, so + // the laundering simply moved one level deeper: retrieval routinely + // returns a section or index URL, and + // `…/reference/hooks/useCopilotFabricated` counted as citing + // `…/reference`. The character after the match has to end the URL + // rather than continue its path. + const next = haystack[at + needle.length]; + if ( + next === undefined || + /[\s)\]}.,;"'<>]/.test(next) || + next === '#' || + next === '?' + ) { + return true; + } + from = at + 1; + } + }); } export const RULES = [ @@ -181,6 +225,23 @@ export interface RuleResult { * in play. "Did not cite" and "had nothing citable" are different facts. */ applicable: boolean; + /** + * Whether this failure should stop the draft publishing, as opposed to being + * worth reporting. + * + * The two are not the same, and collapsing them made the linter stricter than + * the pipeline it sits beside. `grounded-identifiers` is the case that forced + * the split: the doc's success criterion is *zero* invented API names, so one + * occurrence has to show up in a score — but `groundedness.ts` suppresses only + * at SUPPRESS_AT_UNSOURCED_IDENTIFIERS = 2, reasoning that "one could be a + * formatting artifact; two is a pattern of fabrication". Gating a publish at + * one withholds answers production would publish, which is the + * false-withholding direction both modules warn about. + * + * So the harness scores against `passed` and the linter gates on + * `blocksPublish`. For every other rule the two agree. + */ + blocksPublish: boolean; /** Why it failed, or why it was not applicable. Empty when it passed. */ detail: string; } @@ -215,6 +276,8 @@ export function checkReply(reply: string, sources: SearchResult[]): RuleResult[] return [ { rule: 'says-something', + // Metric and gate agree for this rule. + blocksPublish: !(words >= MIN_REPLY_WORDS), applicable: true, passed: words >= MIN_REPLY_WORDS, detail: @@ -225,9 +288,14 @@ export function checkReply(reply: string, sources: SearchResult[]): RuleResult[] { rule: 'grounded-identifiers', applicable: true, + // The doc's criterion is ZERO invented API names, so one fails the + // metric. The publish gate is the pipeline's own threshold, so one does + // not withhold the answer. See RuleResult.blocksPublish. passed: groundedness.unsourcedIdentifiers.length === 0, + blocksPublish: groundedness.suppress, detail: groundedness.unsourcedIdentifiers.length - ? `names not present in any source: ${groundedness.unsourcedIdentifiers.join(', ')}` + ? `names not present in any source: ${groundedness.unsourcedIdentifiers.join(', ')}` + + (groundedness.suppress ? '' : ' (below the suppression threshold)') : '', }, { @@ -236,25 +304,37 @@ export function checkReply(reply: string, sources: SearchResult[]): RuleResult[] // which presented five independent signals as six and double-counted // every failure in both the per-rule table and the report. rule: 'cites-or-is-a-short-handoff', - // A short handoff needs no citation, so the length escape keeps the - // rule applicable even with nothing citable. It only goes - // not-applicable when the reply is long AND there was no URL to cite. - applicable: anySourceHasUrl || isShortEnoughForHandoff, + // Metric and gate agree for this rule. + blocksPublish: !(cites || isShortEnoughForHandoff), + // Not-applicable ONLY when retrieval returned results that happen to + // carry no URL — Pathfinder's plain-text fallback, where a correct + // answer has nothing it could cite. + // + // `sources: []` is a different fact and must still fail: a long, + // uncited reply built on zero retrieval is the doc's Case A, the + // exact input where the citation requirement matters most. Treating + // the two the same let that reply publish under enforcement. + applicable: isShortEnoughForHandoff || sources.length === 0 || anySourceHasUrl, passed: cites || isShortEnoughForHandoff, - detail: !anySourceHasUrl && !isShortEnoughForHandoff - ? 'not evaluated: no retrieved source carries a URL, so nothing could be cited' - : cites || isShortEnoughForHandoff - ? '' - : `${words} words and no link to a retrieved source, over the ${HANDOFF_WORD_CAP}-word handoff cap; a reply this long has to cite what it came from`, + detail: + sources.length > 0 && !anySourceHasUrl && !isShortEnoughForHandoff + ? 'not evaluated: retrieval returned results but none carries a URL, so nothing could be cited' + : cites || isShortEnoughForHandoff + ? '' + : `${words} words and no link to a retrieved source, over the ${HANDOFF_WORD_CAP}-word handoff cap; a reply this long has to cite what it came from`, }, { rule: 'no-banned-phrases', + // Metric and gate agree for this rule. + blocksPublish: !(banned.length === 0), applicable: true, passed: banned.length === 0, detail: banned.map(({ why }) => why).join('; '), }, { rule: 'no-hedged-names', + // Metric and gate agree for this rule. + blocksPublish: !(hedged.length === 0), applicable: true, passed: hedged.length === 0, detail: hedged.length ? 'hedges an API name, which means it is guessing' : '', @@ -262,11 +342,18 @@ export function checkReply(reply: string, sources: SearchResult[]): RuleResult[] { rule: 'no-dead-package', applicable: true, - passed: !DEAD_PACKAGE.test(reply) || LIVE_PACKAGE.test(reply), - detail: - DEAD_PACKAGE.test(reply) && !LIVE_PACKAGE.test(reply) - ? 'mentions @copilotkitnext without naming the @copilotkit/ package that replaced it' - : '', + passed: + !DEAD_PACKAGE.test(reply) || + (LIVE_PACKAGE.test(reply) && MIGRATION_FRAMING.test(reply)), + blocksPublish: !( + !DEAD_PACKAGE.test(reply) || + (LIVE_PACKAGE.test(reply) && MIGRATION_FRAMING.test(reply)) + ), + detail: !DEAD_PACKAGE.test(reply) + ? '' + : LIVE_PACKAGE.test(reply) + ? 'names @copilotkitnext alongside a live package but not as a migration — reads as if both are current, which is the version-mixing failure' + : 'mentions @copilotkitnext without naming the @copilotkit/ package that replaced it', }, ]; } diff --git a/packages/outpost/ai/src/index.ts b/packages/outpost/ai/src/index.ts index 8c85ac6..451385c 100644 --- a/packages/outpost/ai/src/index.ts +++ b/packages/outpost/ai/src/index.ts @@ -47,12 +47,17 @@ export type { TopIssueInput, ScoredTopIssue, } from './front-door.js'; -// The rule set and the scorer are API — Phase 3's linter consumes them. -// HISTORICAL_FAILURES and TARGET_SHAPE deliberately are NOT: they are test data, -// and exporting them put reconstructed bad replies containing -// `useCopilotFabricatedRender` and `@copilotkitnext/react` into dist and the -// worker image, where they would surface in a bundle grep for the dead package. -// Import them from './eval/harness.js' directly in tests and offline runners. +// The rule set, the scorer and the linter are API. HISTORICAL_FAILURES and +// TARGET_SHAPE are not re-exported here because they are test data, not a public +// surface — import them from './eval/harness.js' directly in tests and offline +// runners. +// +// Note what this does NOT do: `tsc` emits per file and `index.ts` imports +// `./eval/harness.js` for `scoreCases`, so `dist/eval/harness.js` still ships +// `HISTORICAL_FAILURES` with its reconstructed replies — a bundle grep for +// `@copilotkitnext` will still hit them. Keeping them out of the build needs the +// fixtures moved outside the compiled graph, which is a separate change; the +// earlier version of this comment claimed a guarantee it did not deliver. export { checkReply, RULES, HANDOFF_WORD_CAP, MIN_REPLY_WORDS } from './eval/rules.js'; export type { RuleId, RuleResult } from './eval/rules.js'; export { scoreCases, formatReport } from './eval/harness.js';