feat(ai): score a reply against the response-quality rules - #241
Merged
Conversation
This was referenced Aug 23, 2026
jerelvelarde
requested changes
Aug 25, 2026
jerelvelarde
left a comment
jerelvelarde
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed deeply, then ran an adversarial round over every blocking finding — all three were refuted, so nothing here is a correctness bug in the sense of shipping broken code. The rules module is genuinely careful: no g-flag lastIndex bugs, banned phrases anchored tightly, and grounded-identifiers delegating to assessGroundedness rather than reimplementing it. Making the rules their own module because the harness and the Phase 3 linter must not drift is exactly the right instinct, and it is the reason this PR is worth more than its line count.
Two things I would fix before merge, then a list you can take or leave.
The metric is maximised by saying nothing
Every rule is a prohibition, so checkReply('', sources) passes all six: words = 0, cites = false, isShortEnoughForHandoff = true, and assessGroundedness('') returns the empty assessment. Same for "No." or "Escalating.". cleanCases is the headline number, and the empty reply is its optimum.
That matters most in exactly the live mode you describe as the follow-up: an agent regression that degrades toward empty or near-empty replies would show up as the score improving. Whatever the fix — a floor rule, a separate "answered at all" signal, weighting — the headline number needs to not reward silence.
An empty case list reports all-green
scoreCases([]) gives every rule {passed: 0, total: 0}, and formatReport prints "0/0 cases clean" followed by six ok lines because passed === total. A live run whose fixture loading silently produced no cases renders as a clean report. That is the house's worst failure mode — silence indistinguishable from success — in the tool built to detect it. Refuse to score an empty set.
The rest, roughly in order of how much they'd bother me
rules.ts:150—source-link-or-handoffandhandoff-is-shortevaluate the identical expressioncites || isShortEnoughForHandoff. They can never disagree, so theperRuletable presents five independent signals as six andformatReportprints two FAIL lines for one condition. Your own summary double-counts case D because of it.rules.ts:88— nothing strips quoted reporter text, so the correct answer to a question about the retired package failsno-dead-package. "You're importing from@copilotkitnext/react, which merged into@copilotkit/react-corev2 — switch the import" is the right reply, and in Phase 3 it collapses into a handoff. The one reporter who most needs the migration answer is the one who cannot get it.index.ts:52—HISTORICAL_FAILURESandTARGET_SHAPEare exported from the package's public entry point.tsconfig.jsoncompiles all ofsrc/**/*.ts, so reconstructed bad replies containinguseCopilotFabricatedRenderand@copilotkitnext/reactland indistand in the worker image, and will show up in bundle greps for the dead package name. Test data, not API.harness.ts:185— case D's fixture is corrupted by operator precedence:.repeat(2)binds to the last string literal only, so the reply ends "...easier to triage. The team will take it from here. this is easier to triage. The team will take it from here." Nothing breaks (87 > 60 either way) but the fixture is not what it reads as.harness.ts:141— case A is 48 words, comfortably under the 60-word cap, so it passes both length rules.rules.ts's own comment says a no-answer "is currently ~400 words of hedging", and case A is the no-answer — so the reconstruction drops the property the doc treats as the headline failure, and only case D exercises the length rules. Worth noting alongside thegrounded-identifiersthinness you already disclose.harness.ts:22— theSHADOW_MODEpointer is stale (and the PR body repeats it).ai-response.ts:178is the doc comment ofrequiredEscalationReason; the shadow-mode arm is around line 824. The substance is right, the line number is not, in a comment whose whole job is to tell someone where to look.
On promoting these to a linter
Two things to reconcile before Phase 3 does that, since a false positive there costs a reporter a correct answer:
- The citation check does not consult
sources, so a reply that writes its invented hook name inside adocs.copilotkit.aiURL passes all six — the URL is both the citation and the laundering. - A correct 100-word answer built from Pathfinder's
textSearchfallback, which setssourceUrl: undefinedon every result, cannot passsource-link-or-handoffat all.
Neither blocks merging this as a scoring-only module, which is what it is today. Both block the linter.
To your two open questions: not wiring it into CI is right for now — the golden set is four reconstructions, and a required check on that is a check on the reconstructions. And I would take the sequencing decision to move the eval suite from step 8 to Phase 1 again; the case for it in CPK-8072 is the most convincing part of that ticket.
CPK-8076 Phase 1 — Replay harness: measure answer quality before changing it (doc step 8)
Doc step 8, moved from last to second. This is the one sequencing change I'd argue for against the doc. Steps 5–7 are the real change and also the easiest to get subtly wrong. Right now there is no way to tell whether any of them helped — the only signal is reading replies by hand. Building the harness first turns every later phase from opinion into measurement, and it's what the doc's own "how we'll know it worked" list requires. Why it's small
Ground truth on day oneThe four appendix cases from the doc, each with a known-correct answer:
Plus the maintainer's own reply in thread A, which the doc holds up as the target shape: verdict, proof, minimum code, one caveat. What it scoresAll mechanically checkable, no LLM judge needed for the first cut:
RelatedOverlaps #217 (no instrumentation for any target metric) and #100 (feedback signal exists, nothing aggregates it). This harness is the offline half; those two are the production half. The "over 80% helpful" gate on the Orca cutover (#59) needs both. Consider also wiring #235 (CI has no mutation check) while here — 11 mutations across #222/#223/#224 left the suite green, which is the same class of problem: tests that pass without proving anything. |
NathanTarbert
added a commit
that referenced
this pull request
Aug 26, 2026
Addresses the review on #242. The anchoring fix in the previous commit was asymmetric. `/^\s*PATH:/im` was matched against the whole block, body included, and `^\s*` allows code indentation while `i` allows lowercase — so a DOCUMENTATION snippet whose content quotes source code got a truthy `path`, was treated as a code block, and lost its docs URL. Verified against the real function with a snippet from the self-hosting guide: TITLE: Self-hosting the CopilotKit Runtime SOURCE: https://docs.copilotkit.ai/guides/self-hosting CONTENT: const handler = copilotRuntimeNextJSAppRouter({ path: "/api/copilotkit", }); before: title '"/api/copilotkit",' sourceUrl undefined after: title 'Self-hosting the CopilotKit Runtime' sourceUrl 'https://docs.copilotkit.ai/guides/self-hosting' Same defect as the one fixed for code blocks, reintroduced on the docs side — a body line winning over a real header. A docs page with no URL cannot be cited, and #241's source-link-or-handoff rule then collapses that answer into a two-sentence handoff, so the cost lands on the reporter. Headers are now read only from the region above `CONTENT:`, which removes the class rather than the two spellings that happened to be noticed. A block is code when it has a PATH and no TITLE, derived from the headers rather than from PATH alone. Also from the same review: - `SearchResult.kind` carries docs-vs-code through to the prompt. GROUNDING_RULES asserts "code entries are shown with their file path" and that the code wins a conflict with the docs, but buildSystemPrompt rendered both as an identical `[Source N: title]` — so the instruction most in need of being reliable, now that "you have NOT read the source" is gone, was resolvable only by guessing. Sources are labelled SOURCE CODE / DOCS, and an unlabelled source stays unlabelled rather than being asserted as either. - Each tool now gets half the retrieval budget instead of a full `defaultLimit` each. Over-fetching paid for 16 snippets to keep 8, and cost docs recall on the majority path: a purely docs-answerable question would have got 4 docs hits rather than 8, the rest going to code that merely cleared min_score. - `blobUrl` logs instead of returning undefined silently. A bare slug, an SSH remote, or a missing REPOSITORY on one index would leave EVERY code hit with nothing to cite and no trace — the silent-degradation shape this change exists to remove, one layer up. - `search()`'s doc comment claimed private tool names remove the silent no-retrieval mode. They do not: `SearchTool` is a compile-time union and cannot know what the server exposes, so a server-side rename still yields one console.error and []. Comment corrected and pointed at #244. Verification: ai package 293 -> 303, full repo turbo run test 10/10, typecheck clean. On mutations: reverting the header region alone, or the isCode rule alone, does NOT fail a test — three layers independently prevent this (title reads TITLE before PATH, isCode requires no TITLE, headers come only from above CONTENT:). Reverting the two structural ones together fails two tests. Dropping the prompt label, and restoring the full per-tool limit, each fail the tests that name them. Refs CPK-8077
NathanTarbert
added a commit
that referenced
this pull request
Aug 26, 2026
Addresses the review on #241. Both blockers were right, and both were the same shape: the metric reporting success where there was none. ## The score was maximised by saying nothing Every rule was a prohibition, so `checkReply('')` passed all six — and so did 'No.' and 'Escalating.'. In the live mode that matters most, an agent regressing toward empty replies would have shown up as the score IMPROVING, in the tool built to catch exactly that. New `says-something` rule with MIN_REPLY_WORDS = 8, set against the doc's own floor rather than picked: the shortest acceptable reply is a Route, defined as "two sentences. What we confirmed, if anything, and that a human is picking it up." The doc's reference handoff is 15 words, so 8 leaves headroom while still rejecting a bare acknowledgement. A SILENT reply is not a short reply, it is no reply, and is never scored. ## An empty case list reported all-green `scoreCases([])` gave every rule {passed: 0, total: 0}, and formatReport printed six `ok` lines because `passed === total`. A live run whose fixture loading silently produced nothing rendered as a perfect score. Now refused with a message that names the likely cause. ## Also from the same review - **`source-link-or-handoff` and `handoff-is-short` were one rule wearing two names** — identical expressions, so they could never disagree. That presented five independent signals as six and double-counted every failure, including in the summary I wrote for case D. Collapsed into `cites-or-is-a-short-handoff`. - **The migration answer was the one reply the rules forbade.** "Never mention @copilotkitnext" is right as a default and wrong as an absolute: someone importing from it needs to be told what to import instead. Naming it is now allowed when the reply also names a live `@copilotkit/` package, which is what makes it a migration instruction rather than a stray reference. Otherwise the reporter who most needs that answer is the only one who cannot get it. - **Test fixtures were exported from the package entry point.** HISTORICAL_ FAILURES and TARGET_SHAPE 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 name. Import them from './eval/harness.js' directly instead. - **Case D's fixture was corrupted by operator precedence** — `.repeat(2)` bound to the last string literal only, so the reply ended with a stray duplicate sentence rather than the padding the comment described. It is now 120 words, which is what makes it a fixture for the length rule. - **The SHADOW_MODE pointer was wrong** in a comment whose whole job is to say where to look. The gate is `ai-response.ts:824`; `:178` is unrelated code. Verified before changing. Verification: ai package 305 -> 318, full repo turbo run test 10/10, typecheck clean. Three mutations, each killing exactly the tests that name it: says-something always passing, scoring an empty set, and a flat dead-package ban. Not addressed here, because both are about promoting these rules to a linter rather than about the scorer: a reply can launder an invented name inside a docs URL and pass the citation rule, and a correct answer built from the plain-text fallback (sourceUrl undefined on every result) cannot pass it at all. Both belong with CPK-8078. Refs CPK-8076
NathanTarbert
force-pushed
the
feat/response-quality-eval-harness
branch
from
August 26, 2026 12:31
448d402 to
7557e9b
Compare
NathanTarbert
added a commit
that referenced
this pull request
Aug 31, 2026
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
Collaborator
Author
|
Thanks Jerel — both blockers were the same shape, and it's the right shape to have caught: the metric reporting success where there was none. Fixed in The score was maximised by saying nothing
New The empty case listRefused now, with a message naming the likely cause. You put it better than I would have — silence indistinguishable from success, in the tool built to detect that. The rest, all taken
Your two linter itemsBoth were prerequisites rather than extras, so they're done in #249 rather than here — the citation rule now matches against retrieved URLs instead of a docs-shaped pattern, and Also glad the sequencing argument held up — moving the eval suite ahead of steps 5–7 is the call I was least sure of.
|
Phase 1 of the response-quality work (CPK-8076). The Agent's Output Doc lists its success criteria as deliberately mechanical — "zero invented API names, this one is mechanically checkable, so any occurrence is a bug, not a judgment call" — and this is that check, plus the aggregation that turns it into a number. Six rules, each traceable to a line in the doc: identifiers must appear in the retrieved sources, a reply either cites or stays under the 60-word handoff cap, no praise openers or self-commentary or false "I can't see the thread" claims, no hedged API names, no mention of the retired @copilotkitnext. One rule set, two consumers, and that is the reason it lives in its own module rather than inside the harness: the doc's step 3 linter runs the same rules BEFORE a reply posts, collapsing a failing draft into the two-sentence handoff. If the linter and the harness disagreed about what "invented API name" means, the score would stop predicting the behaviour. The grounding rule delegates to assessGroundedness for the same reason — production already gates on it. The golden set is the doc's four appendix failures, and all four are caught. They are RECONSTRUCTIONS from the doc's descriptions and quoted fragments, not transcripts; each carries a provenance link to the original thread so the reconstruction can be checked. So they pin that the rule set catches each documented failure mode. They do not measure the current agent, which needs the live mode: real threads through AIPipeline.generateSupportResponse, scored by the same rules. SHADOW_MODE gates only the platform post-back, so that needs no new safety machinery, just a caller. The maintainer reply the doc holds up as correct is included and passes all six. A rule set that fires on everything is as useless as one that never fires, and a false positive here costs a reporter a correct answer — the same failure direction as the groundedness gate withholding one. Known thinness, measured rather than assumed: grounded-identifiers only fires on case B. Cases A, C and D name no CopilotKit-shaped identifier, so the strictest rule passes vacuously on three of the four. 57 tests. Mutation-checked rather than trusted green: forcing grounded-identifiers to pass, loosening the praise-opener pattern to bare /question/i, and dropping the cites-or-short disjunction each kill the tests that name them. Refs CPK-8076
Addresses the review on #241. Both blockers were right, and both were the same shape: the metric reporting success where there was none. ## The score was maximised by saying nothing Every rule was a prohibition, so `checkReply('')` passed all six — and so did 'No.' and 'Escalating.'. In the live mode that matters most, an agent regressing toward empty replies would have shown up as the score IMPROVING, in the tool built to catch exactly that. New `says-something` rule with MIN_REPLY_WORDS = 8, set against the doc's own floor rather than picked: the shortest acceptable reply is a Route, defined as "two sentences. What we confirmed, if anything, and that a human is picking it up." The doc's reference handoff is 15 words, so 8 leaves headroom while still rejecting a bare acknowledgement. A SILENT reply is not a short reply, it is no reply, and is never scored. ## An empty case list reported all-green `scoreCases([])` gave every rule {passed: 0, total: 0}, and formatReport printed six `ok` lines because `passed === total`. A live run whose fixture loading silently produced nothing rendered as a perfect score. Now refused with a message that names the likely cause. ## Also from the same review - **`source-link-or-handoff` and `handoff-is-short` were one rule wearing two names** — identical expressions, so they could never disagree. That presented five independent signals as six and double-counted every failure, including in the summary I wrote for case D. Collapsed into `cites-or-is-a-short-handoff`. - **The migration answer was the one reply the rules forbade.** "Never mention @copilotkitnext" is right as a default and wrong as an absolute: someone importing from it needs to be told what to import instead. Naming it is now allowed when the reply also names a live `@copilotkit/` package, which is what makes it a migration instruction rather than a stray reference. Otherwise the reporter who most needs that answer is the only one who cannot get it. - **Test fixtures were exported from the package entry point.** HISTORICAL_ FAILURES and TARGET_SHAPE 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 name. Import them from './eval/harness.js' directly instead. - **Case D's fixture was corrupted by operator precedence** — `.repeat(2)` bound to the last string literal only, so the reply ended with a stray duplicate sentence rather than the padding the comment described. It is now 120 words, which is what makes it a fixture for the length rule. - **The SHADOW_MODE pointer was wrong** in a comment whose whole job is to say where to look. The gate is `ai-response.ts:824`; `:178` is unrelated code. Verified before changing. Verification: ai package 305 -> 318, full repo turbo run test 10/10, typecheck clean. Three mutations, each killing exactly the tests that name it: says-something always passing, scoring an empty set, and a flat dead-package ban. Not addressed here, because both are about promoting these rules to a linter rather than about the scorer: a reply can launder an invented name inside a docs URL and pass the citation rule, and a correct answer built from the plain-text fallback (sourceUrl undefined on every result) cannot pass it at all. Both belong with CPK-8078. Refs CPK-8076
The four eval files were committed unformatted. Nothing here changes behaviour — 318 tests unchanged — but the repo's prettier config is the convention and a formatting drift in new files becomes diff noise on every later change to them. Worth noting why this was not caught earlier: CI's job is named "Lint, Typecheck & Test" and runs no lint step, because ESLint 9 cannot read the repo's .eslintrc.cjs and the flat-config migration is still open (#141). ci.yml:314 documents that. There is also no prettier check anywhere in CI, so formatting is currently unenforced end to end.
NathanTarbert
force-pushed
the
feat/response-quality-eval-harness
branch
from
September 4, 2026 15:03
d1b74de to
1ef00fd
Compare
NathanTarbert
added a commit
that referenced
this pull request
Sep 4, 2026
Addresses the review on #242. The anchoring fix in the previous commit was asymmetric. `/^\s*PATH:/im` was matched against the whole block, body included, and `^\s*` allows code indentation while `i` allows lowercase — so a DOCUMENTATION snippet whose content quotes source code got a truthy `path`, was treated as a code block, and lost its docs URL. Verified against the real function with a snippet from the self-hosting guide: TITLE: Self-hosting the CopilotKit Runtime SOURCE: https://docs.copilotkit.ai/guides/self-hosting CONTENT: const handler = copilotRuntimeNextJSAppRouter({ path: "/api/copilotkit", }); before: title '"/api/copilotkit",' sourceUrl undefined after: title 'Self-hosting the CopilotKit Runtime' sourceUrl 'https://docs.copilotkit.ai/guides/self-hosting' Same defect as the one fixed for code blocks, reintroduced on the docs side — a body line winning over a real header. A docs page with no URL cannot be cited, and #241's source-link-or-handoff rule then collapses that answer into a two-sentence handoff, so the cost lands on the reporter. Headers are now read only from the region above `CONTENT:`, which removes the class rather than the two spellings that happened to be noticed. A block is code when it has a PATH and no TITLE, derived from the headers rather than from PATH alone. Also from the same review: - `SearchResult.kind` carries docs-vs-code through to the prompt. GROUNDING_RULES asserts "code entries are shown with their file path" and that the code wins a conflict with the docs, but buildSystemPrompt rendered both as an identical `[Source N: title]` — so the instruction most in need of being reliable, now that "you have NOT read the source" is gone, was resolvable only by guessing. Sources are labelled SOURCE CODE / DOCS, and an unlabelled source stays unlabelled rather than being asserted as either. - Each tool now gets half the retrieval budget instead of a full `defaultLimit` each. Over-fetching paid for 16 snippets to keep 8, and cost docs recall on the majority path: a purely docs-answerable question would have got 4 docs hits rather than 8, the rest going to code that merely cleared min_score. - `blobUrl` logs instead of returning undefined silently. A bare slug, an SSH remote, or a missing REPOSITORY on one index would leave EVERY code hit with nothing to cite and no trace — the silent-degradation shape this change exists to remove, one layer up. - `search()`'s doc comment claimed private tool names remove the silent no-retrieval mode. They do not: `SearchTool` is a compile-time union and cannot know what the server exposes, so a server-side rename still yields one console.error and []. Comment corrected and pointed at #244. Verification: ai package 293 -> 303, full repo turbo run test 10/10, typecheck clean. On mutations: reverting the header region alone, or the isCode rule alone, does NOT fail a test — three layers independently prevent this (title reads TITLE before PATH, isCode requires no TITLE, headers come only from above CONTENT:). Reverting the two structural ones together fails two tests. Dropping the prompt label, and restoring the full per-tool limit, each fail the tests that name them. Refs CPK-8077
NathanTarbert
added a commit
that referenced
this pull request
Sep 4, 2026
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
jerelvelarde
approved these changes
Sep 4, 2026
jerelvelarde
left a comment
jerelvelarde
left a comment
Collaborator
There was a problem hiding this comment.
Re-reviewed and ran the mutations. Both blockers are closed. ai/src is 318 tests.
The metric no longer rewards silence. says-something at MIN_REPLY_WORDS = 8, and the number is derived rather than picked — the doc's own reference handoff is 15 words, so the floor sits below the shortest legitimate reply and above nothing-at-all. Excluding SILENT from scoring is the right distinction: a withheld reply is not a short reply, and conflating them would have put the bug straight back in.
Mutations, run rather than read:
| Mutation | Result |
|---|---|
says-something forced to passed: true |
5 failed / 313 passed |
scoreCases empty-set refusal removed |
1 failed / 317 passed |
Five kills on the first one is a better answer than I expected — the rule is load-bearing across the case set, not pinned by a single guard test.
The rest, checked: the duplicate pair really is collapsed to one cites-or-is-a-short-handoff; the migration answer is allowed when the reply also names a live @copilotkit/ package, which was the case where the flat ban hurt the one reporter who most needed it; case D's .repeat(3) now applies to the sentence it was meant to; and the SHADOW_MODE pointer is corrected to ai-response.ts:824.
Approving.
Two things to carry, neither blocking
The fixtures still ship. You said this plainly rather than letting the comment stand — tsc emits per file and index.ts imports ./eval/harness.js, so dist/eval/harness.js carries the reconstructed bad replies regardless of what the entry point re-exports. Keeping them out of the public surface is still worth doing; just noting the comment now describes reality, which is what I actually wanted.
Merge order, because #249 is stacked on this branch. #249 has feat/response-quality-eval-harness as its base, and ci.yml filters pull_request to branches: [main, staging] — so Lint, Typecheck & Test has never run on #249 at all. Its only check is zizmor, which uses a bare pull_request: trigger precisely so it reports on every PR as a required check. That required check passes, and GitHub therefore reports #249 as CLEAN and mergeable: 618 lines of the draft linter presenting as green with no test signal behind it.
Nothing for you to fix in this PR. But #249 should not be read as green until it is retargeted at main and pushed, or until ci.yml's pull_request trigger drops its branch filter the way security_zizmor.yml already does. I would rather fix the trigger — this will happen again on the next stack.
NathanTarbert
added a commit
that referenced
this pull request
Sep 7, 2026
Found by Jerel reviewing #241. `pull_request` was filtered to `branches: [main, staging]`, so a PR based on another PR's branch got no CI at all. `Static analysis (zizmor)` uses a bare `pull_request:` trigger because it is a required status check, so it still reported and still passed — and GitHub then read the PR as CLEAN and mergeable. #249 sat exactly that way, verified before changing anything: `gh pr checks 249` returned one check, zizmor, passing; `mergeStateStatus=CLEAN`. So 618 lines of the draft linter presented as green with a workflow linter as their only signal and no test run behind them. A check that reports success without evaluating anything is worse than no check, and it is the same shape as the defects this stack has been about. The filter is gone from `pull_request`. `push` keeps its filter. That is where the original reasoning applies: `main` and `staging` deploy, and Railway's deploy triggers wait for a check suite on the pushed commit. No other branch deploys, so no other branch needs a push-triggered suite — its pull-request run covers it. Retargeting #249 at `main` would have fixed that one PR. This fixes the next stack too, which was Jerel's preference and is the better trade for one dropped line. Verified the workflow parses and keeps all 14 steps: `on.pull_request` is now null, `on.push.branches` is unchanged.
NathanTarbert
added a commit
that referenced
this pull request
Sep 7, 2026
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
This was referenced Sep 7, 2026
NathanTarbert
added a commit
that referenced
this pull request
Sep 7, 2026
Found by Jerel reviewing #241. `pull_request` was filtered to `branches: [main, staging]`, so a PR based on another PR's branch got no CI at all. `Static analysis (zizmor)` uses a bare `pull_request:` trigger because it is a required status check, so it still reported and still passed — and GitHub then read the PR as CLEAN and mergeable. #249 sat exactly that way, verified before changing anything: `gh pr checks 249` returned one check, zizmor, passing; `mergeStateStatus=CLEAN`. So 618 lines of the draft linter presented as green with a workflow linter as their only signal and no test run behind them. A check that reports success without evaluating anything is worse than no check, and it is the same shape as the defects this stack has been about. The filter is gone from `pull_request`. `push` keeps its filter. That is where the original reasoning applies: `main` and `staging` deploy, and Railway's deploy triggers wait for a check suite on the pushed commit. No other branch deploys, so no other branch needs a push-triggered suite — its pull-request run covers it. Retargeting #249 at `main` would have fixed that one PR. This fixes the next stack too, which was Jerel's preference and is the better trade for one dropped line. Verified the workflow parses and keeps all 14 steps: `on.pull_request` is now null, `on.push.branches` is unchanged.
NathanTarbert
added a commit
that referenced
this pull request
Sep 7, 2026
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
NathanTarbert
added a commit
that referenced
this pull request
Sep 7, 2026
… layer The non-blocking follow-ups from Jerel's reviews of #241 and #242. ## The fixtures really do ship, and not re-exporting them was never the fix Verified against a built `dist` rather than reasoned about: `tsc` emits per file and `index.ts` imports `./eval/harness.js`, so while HISTORICAL_FAILURES and TARGET_SHAPE lived in `harness.ts` the reconstructed bad replies shipped in `dist/eval/harness.js` no matter what the entry point declared. `grep -rl "useCopilotFabricatedRender" dist/` hit it. They now live in `eval/__fixtures__/historical-failures.ts`, and `ai/tsconfig.json` excludes `**/*.test.ts`, `**/__tests__/**` and `**/__fixtures__/**` from the build. `harness.ts` no longer imports them, which it cannot: a compiled module importing an excluded one emits a broken build. Excluding test material also fixes something wider that was never raised — every `.test.ts` in the package was being compiled into `dist`. That is now 0 files. Post-build audit: no fixture reply text, no invented hook name, no `__fixtures__` directory. `@copilotkitnext` still appears in `dist/eval/rules.js`, and has to — that is the rule which bans it. A bundle grep for the dead package name hits the rule, not a fabricated example of it. The comment in `index.ts` now says that instead of claiming an exclusion it did not deliver. ## Each parser layer is pinned separately Three layers independently prevent a docs block being read as code — the title prefers TITLE over PATH, `isCode` requires the absence of TITLE, and headers are read only from above `CONTENT:`. The redundancy is deliberate, and it meant no single layer was pinned: reverting any one alone left the suite green, so someone simplifying one away in six months would have got a clean run and a quietly reduced defence. Two tests isolate a layer each: - a block carrying BOTH TITLE and PATH must read as docs and keep its SOURCE. Kills the title-order revert and the `isCode` revert. - a docs block with no SOURCE header and a line-initial `SOURCE:` inside its content must not adopt that line as its citation. Kills the header-region revert. Verified: each single-layer revert now fails exactly one test, where before all three survived alone. `isCode` also gains a comment naming what it depends on — that a code hit never carries a TITLE, which is a server-side contract we do not own. If a code result ever gains one, `source` falls back to an absent `SOURCE` header and the file path silently stops being citable. The both-headers test is what makes that change in behaviour visible. Verification: ai package 377 tests, typecheck 10/10, test 10/10.
NathanTarbert
added a commit
that referenced
this pull request
Sep 8, 2026
… layer The non-blocking follow-ups from Jerel's reviews of #241 and #242. ## The fixtures really do ship, and not re-exporting them was never the fix Verified against a built `dist` rather than reasoned about: `tsc` emits per file and `index.ts` imports `./eval/harness.js`, so while HISTORICAL_FAILURES and TARGET_SHAPE lived in `harness.ts` the reconstructed bad replies shipped in `dist/eval/harness.js` no matter what the entry point declared. `grep -rl "useCopilotFabricatedRender" dist/` hit it. They now live in `eval/__fixtures__/historical-failures.ts`, and `ai/tsconfig.json` excludes `**/*.test.ts`, `**/__tests__/**` and `**/__fixtures__/**` from the build. `harness.ts` no longer imports them, which it cannot: a compiled module importing an excluded one emits a broken build. Excluding test material also fixes something wider that was never raised — every `.test.ts` in the package was being compiled into `dist`. That is now 0 files. Post-build audit: no fixture reply text, no invented hook name, no `__fixtures__` directory. `@copilotkitnext` still appears in `dist/eval/rules.js`, and has to — that is the rule which bans it. A bundle grep for the dead package name hits the rule, not a fabricated example of it. The comment in `index.ts` now says that instead of claiming an exclusion it did not deliver. ## Each parser layer is pinned separately Three layers independently prevent a docs block being read as code — the title prefers TITLE over PATH, `isCode` requires the absence of TITLE, and headers are read only from above `CONTENT:`. The redundancy is deliberate, and it meant no single layer was pinned: reverting any one alone left the suite green, so someone simplifying one away in six months would have got a clean run and a quietly reduced defence. Two tests isolate a layer each: - a block carrying BOTH TITLE and PATH must read as docs and keep its SOURCE. Kills the title-order revert and the `isCode` revert. - a docs block with no SOURCE header and a line-initial `SOURCE:` inside its content must not adopt that line as its citation. Kills the header-region revert. Verified: each single-layer revert now fails exactly one test, where before all three survived alone. `isCode` also gains a comment naming what it depends on — that a code hit never carries a TITLE, which is a server-side contract we do not own. If a code result ever gains one, `source` falls back to an absent `SOURCE` header and the file path silently stops being citable. The both-headers test is what makes that change in behaviour visible. Verification: ai package 377 tests, typecheck 10/10, test 10/10.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase 1 of the response-quality work — CPK-8076, from the Agent's Output Doc.
The doc's success criteria are already mechanical, on purpose: "zero invented API names — this one is mechanically checkable, so any occurrence is a bug, not a judgment call." This is that check, plus the aggregation that turns it into a number you can compare across runs.
Six rules, each traceable to a line in the doc:
grounded-identifierssource-link-or-handoffhandoff-is-shortno-banned-phrasesno-hedged-namesno-dead-package@copilotkitnextWhy the rules are their own module
They're needed twice, and the two uses must not drift. The harness scores a reply after the fact; the doc's step 3 linter runs the same rules before a reply posts, and a failure collapses the draft into the handoff. If the linter and the harness disagreed about what "invented API name" means, the score would stop predicting the behaviour. So
grounded-identifiersdelegates toassessGroundedness— the signal production already gates on — rather than reimplementing it.That makes this PR the foundation for Phase 3 as well as Phase 1.
The golden set, and what it does and doesn't prove
The doc's four appendix failures, all four caught:
These are reconstructions, not transcripts. The doc describes each reply's shape and quotes fragments; the full originals are in the linked threads. Each case carries a
provenancelink so the reconstruction can be checked against the real thing. So they pin that the rule set catches each documented failure mode — they do not measure the current agent, because they aren't its output.Measuring the agent is the live mode, described at the top of
harness.ts: real threads throughAIPipeline.generateSupportResponse, scored by these same rules.SHADOW_MODEgates only the platform post-back (ai-response.ts:178), so retrieval and generation already run fully without posting — that mode needs a caller, not new safety machinery. It's the obvious follow-up and it's what makes Phase 2 provable rather than plausible.Pinned in both directions
The maintainer reply the doc holds up as the target shape is included and passes all six rules. A rule set that fires on everything is as useless as one that never fires, and a false positive here costs a reporter a correct answer — the same failure direction as the groundedness gate withholding one, which is what #239 is about.
Note it's 65 words and still passes, because it cites. That's the
cites || shortdisjunction working as intended rather than a flat length limit.Known thinness, measured rather than assumed
grounded-identifiersonly actually fires on case B. Cases A, C and D name no CopilotKit-shaped identifier, so the strictest rule passes vacuously on three of the four. Worth more cases before anyone reads a per-rule pass rate as meaningful.Verification
57 tests across the two files,
aipackage 270 → 305. Full repoturbo run test10/10 packages.tsc --noEmitclean.Mutation-checked rather than trusted green — each of these kills exactly the test that names it:
grounded-identifiersto passfails on a name that appears in none of the sources/question/idoes not fire on innocent uses of the same wordscites || shortdisjunctionpasses a long answer that links …testsOpen questions, happy to go either way