feat(ai): lint a draft against the reply rules before it publishes - #256
Conversation
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
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
e8c88b4 to
0ba16cd
Compare
jerelvelarde
left a comment
jerelvelarde
left a comment
There was a problem hiding this comment.
Read the whole thing and ran the rules against inputs the tests don't cover. The structure is right — report-mode default, verdict-not-copy, one rule module for both consumers — and the eval tests are green here (3 files, 72 tests). One real bug in the citation matcher, and it bites in the direction this PR says it most wants to avoid.
Blocker — a trailing slash defeats the citation rule
citesARetrievedSource strips trailing /#? from the source side (rules.ts:176) but the boundary check on the reply side (rules.ts:187-193) does not accept / as a terminator. So …/reference followed by / is treated as a path continuation — indistinguishable from the laundering case it was written to stop.
I ran this rather than reasoned about it. Probes, all padded past the 60-word cap so the rule actually bites:
BUG A src trailing-slash, reply cites it VERBATIM passed=false blocksPublish=true
BUG B src bare, reply adds slash passed=false blocksPublish=true
BUG C markdown link w/ slash, src w/ slash passed=false blocksPublish=true
OK D control: markdown, both bare passed=true
OK E laundering must fail passed=false ✓
BUG F trailing-slash + query passed=false blocksPublish=true
OK G anchor on a bare retrieved URL passed=true
Case A is the one that matters: the retrieved URL ends in /, the reply reproduces it exactly, and the rule says it cited nothing. Doc sites canonicalise with trailing slashes, so this is not an exotic input.
Nothing in rules.test.ts covers it — every fixture URL is bare (…/CopilotChat), including the citation boundaries block. That block correctly pins the appended-path case and the anchor case, which is why this reads as covered.
Why it blocks now, even though nothing is wired and report mode withholds nothing: the plan is "read what it would have collapsed against real traffic, then enforce once the false-positive rate is measured rather than assumed." This bug manufactures false positives in exactly that number — and in the direction that looks conservative, so the baseline would argue against enforcing for a reason that is an artifact. The measurement is the deliverable here, so it should be right before it collects.
One line, at rules.ts:187:
let next = haystack[at + needle.length];
// A single trailing slash still ends the URL; only a further path segment
// continues it.
if (next === '/') next = haystack[at + needle.length + 1];I applied that locally: A, B, C, F flip to pass, E still rejects the laundering, and the eval suite stays green (72/72, no diff against baseline). Worth a test per row above, since the current fixtures can't see this class.
Worth fixing while you're in there
blocksPublish is true on a rule that is not applicable. When the citation rule is inapplicable (plain-text fallback, long reply), passed is false and blocksPublish: !(cites || isShortEnoughForHandoff) is true. lintDraft gets it right by filtering r.applicable && r.blocksPublish (linter.ts:77), so there is no live bug — but RuleResult is exported API, and the next consumer reading blocksPublish alone collapses the exact draft the applicable field was added to protect. Either compute it as applicable && … at the source, or say on the field that it is only meaningful alongside applicable.
blocksPublish restates the passed expression in five of six rules, and twice over in no-dead-package (rules.ts:345-351). Where the comment already says "metric and gate agree for this rule", blocksPublish: !passed says that once instead of duplicating a three-term boolean — and removes the drift risk this module exists to prevent.
Optional: if the retrieved URL itself carries an anchor (…/CopilotChat#slots) and the reply links the clean page, that is not counted — the needle strips only trailing #. Rarer than the slash; noting it, not asking for it.
On the call you flagged
The passed / blocksPublish split is right, and I'd keep it. Both single-threshold alternatives are worse: matching production at two silently drops the doc's zero-invented-names criterion from the metric, and gating at one withholds answers production would publish. Carrying both numbers and naming which is which is the honest option, and the comment on RuleResult.blocksPublish explains it well enough that the next reader won't collapse them again.
Good
applicable as a third state is the right shape, and zero-retrieval staying a failure is the subtle half — treating it as not-applicable would have published Case A under enforcement. The harness n/a at 0/0 fix, and filtering the failing-cases block on applicable too, close the same hole in both outputs. And the index.ts comment now retracting a guarantee it did not deliver — dist/eval/harness.js still ships the fixtures — is the kind of correction that is easier to leave alone than to write.
Fix the slash and this is good to go.
CopilotKit#6927, answered 2026-09-06T17:40Z, 23 seconds after the issue opened. The reporter had done the work -- a reproduction, the wire-level symptom, a proposed fix. The reply praised the write-up, spent a paragraph announcing what it had not done, and handed the question back to engineering. Run against the rule set as it stands, that reply fails exactly one rule: no-banned-phrases, on the opener. The paragraph that makes it worse than silence passes, because the pattern read `i haven't read the source` and the reply said "inspected the source" -- a miss on the verb alone. So does the framing device that introduces it, "To be clear about my position:". Both are now caught, and the reply is a fixture (case-e) so the rule cannot silently stop firing on it. ON THE WIDTH OF THE PATTERN BANNED_PHRASES' own docblock is explicit that a false positive is the expensive direction: a linter failure collapses the draft into a handoff, so a rule that fires on ordinary prose costs a reporter a correct answer. The first draft of this pattern allowed a 60-character gap between verb and object, and flagged two ordinary reporter sentences in testing: "I haven't run the repro yet - can you share the code you used?" "I haven't run into this, but the implementation forwards headers only for stdio" Both are the reporter describing their own testing. The verb now binds directly to a closed object list, which separates the agent saying it did not look at the code from someone saying they have not run something. Fifteen cases are pinned across both directions, including "You haven't inspected the source" -- the same words in the reporter's voice rather than the agent's. Reverting the widened pattern fails six cases. Suite 1234 -> 1242. The new fixture is appended rather than inserted: harness.test.ts indexes HISTORICAL_FAILURES positionally, so inserting anywhere but the end silently reassigns which case each of those assertions is about. The new case is looked up by id instead, and that is noted where it would bite.
|
Pushed Why this belongs hereCopilotKit#6927 was answered 2026-09-06T17:40Z, 23 seconds after the issue opened and about 20 hours before #241 and #242 merged. The reporter had already done the work: a reproduction, the wire-level symptom, a proposed fix. The reply praised the write-up, spent a paragraph announcing what it had not done, and handed the question back. I ran that reply through
On the width, because this is the riskThe docblock is explicit that a false positive is the expensive direction — a linter failure collapses the draft into a handoff, so a rule that fires on ordinary prose costs a reporter a correct answer. That is the constraint I worked to, and my first draft failed it. The first version allowed a 60-character gap between verb and object. Probed against reporter-voice sentences, it flagged two: Both are the reporter describing their own testing. The verb now binds directly to a closed object list, which is what separates the agent saying it did not look at the code from someone saying they have not run something. Fifteen cases are pinned across both directions, including Reverting the widened pattern fails six cases. Suite 1234 → 1242, One thing worth knowing about the fixtures
The larger point, not for this PRThis PR is the reason the finding matters. #241 scores replies offline; nothing stops a draft publishing until the linter here lands. So a reply shaped exactly like #6927's would still go out today — the rules would score it badly and no one would be told. That is the gap this closes, and it is why report mode first is the right sequencing rather than a hedge. |
The needle strips its own trailing /#? before searching, so a reply that reproduces a canonicalised URL VERBATIM arrived at the boundary check with `/` as the next character and was read as a path continuation. The rule reported "cited nothing" against a reply quoting the retrieved URL exactly. Doc sites canonicalise with a trailing slash, so this is ordinary traffic. All four of the shapes below failed: src trailing-slash, reply cites it verbatim src bare, reply adds the slash markdown link with the slash on both sides trailing slash followed by a query Only a FURTHER path segment continues a URL, so the check now steps over one slash and judges what follows it. `…/reference/` ends; `…/reference/hooks/ useCopilotFabricated` still does not cite `…/reference`, which is the laundering the boundary check exists for. Stepping over every slash instead fails two tests. This mattered now rather than later because the direction is wrong in the way that hides itself. Report mode withholds nothing, so no reporter lost an answer -- but the number this PR exists to collect is the false-positive rate, and this manufactured false positives that look conservative. The baseline would have argued against enforcing, for an artifact. ALSO blocksPublish no longer restates the passed expression. Each verdict is bound once and both fields read the binding, which removes the drift this module was written to prevent from the module itself -- no-dead-package had the three-term boolean written out twice. blocksPublish for the citation rule is now `applicable && !satisfied`. It was true whenever a rule could not be evaluated at all: Pathfinder's plain-text fallback returns results with no sourceUrl, so a correct answer built from it has nothing it could cite. lintDraft already filtered on `applicable`, so there was no live bug -- but RuleResult is exported API, and a consumer reading blocksPublish alone would collapse the exact draft `applicable` was added to protect. Zero retrieval still blocks: "had nothing citable" and "cited nothing" remain different facts. Six tests for the slash class and two for the applicable/blocksPublish pair; every URL fixture in this file was bare, which is why the class read as covered. Suite 1242 -> 1250. Reverting the slash fix fails 4.
|
Fixed in I reproduced your table before touching anything, and got your rows exactly: After the fix all seven rows are what they should be, E included. Six tests cover the class — every URL fixture in that file was bare, which is exactly why it read as covered. One thing I checked beyond the one-liner, because stepping over a slash is close to excusing the thing the boundary check exists for: stepping over every slash rather than one fails two tests, The other two
Not takenThe anchor-on-the-retrieved-URL case you flagged as optional — Suite 1242 → 1250, One process note, since it is in this PR's history: |
jerelvelarde
left a comment
jerelvelarde
left a comment
There was a problem hiding this comment.
Verified against 9e9a6b66 by re-running the probes, not by reading the diff. All three items are closed.
The slash bug. Fixed, and I re-ran the original probe set plus two cases I had not covered:
OK A src trailing-slash, reply cites it verbatim passed=true
OK B src bare, reply adds slash passed=true
OK C markdown link w/ slash passed=true
OK D control, both bare passed=true
OK E laundering must fail passed=false ✓
OK E2 laundering when the SOURCE carries the slash passed=false ✓
OK F trailing-slash + query passed=true
OK G anchor on a bare retrieved URL passed=true
OK I slash then anchor passed=true
E2 is the one worth calling out — laundering still gets rejected when the retrieved URL is itself canonicalised with a slash, which is the case where stepping over a slash could plausibly have opened a hole. It does not.
Eval suite 72 → 97 tests, all green.
blocksPublish on a not-applicable rule. citationApplicable && !citationSatisfied — computed at the source, so the field is no longer a trap for a consumer reading it alone.
The restated expressions. Bound once now, so "metric and gate agree for this rule" is said by the code instead of by a comment above a duplicated boolean.
On 6ee04fe4, which I had not reviewed
I probed the widened self-commentary pattern separately, since it lands in a rule that blocks publishing and a false positive there costs a reporter a correct answer:
OK agent: "I haven't run this code or inspected the source" fires ✓
OK agent: "To be clear about my limits" fires ✓
OK reporter: "I haven't run the repro yet, can you share…" quiet ✓
OK reporter: "I haven't run into this, but the implementation…" quiet ✓
OK "I read the code and the header is dropped in the stdio path" quiet ✓
OK "I did not reproduce this on v1.61.0" quiet ✓
Requiring adjacency and closing the object list was the right call — the 60-character gap the comment describes would have caught both reporter sentences, and that is the failure this whole module is built to avoid. Good that the rejected draft is written down; the next person widening this pattern will need it.
Approving. The remaining anchor-in-source case (retrieved …/CopilotChat#slots, reply links the clean page) is still not counted, as expected — I flagged it as optional and it stays optional. Worth a line in the wiring PR if report mode shows it in real traffic.
2 participants
A draft is judged against the reply rules before it publishes, and in report mode the verdict is recorded without changing what goes out. Phase 3 of the response-quality work (CPK-8078), first slice.
Replaces #249, which GitHub closed automatically when
feat/response-quality-eval-harnesswas deleted on merging #241. Same branch, same two commits, rebased ontomainnow that #241 has landed — and unlike #249 this one actually gets aLint, Typecheck & Testrun, since #249's base meant the workflow'sbranches: [main, staging]filter never matched it. No review was lost; #249 had none.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, running the same rules the harness scores with, from the same module, so the thing measured and the thing enforced cannot drift.
The two prerequisites, carried from Jerel's review of #241
Both would have made the linter withhold correct answers — the same direction as the groundedness gate suppressing one, which is what #234 was.
docs.copilotkit.aiURL and satisfy the rule with a page that does not exist.assessGroundednessblanks URLs before it looks, so the identifier rule cannot catch it either. Now matched against the URLs actually retrieved, with a boundary check so an invented path appended to a retrieved section URL does not count.textSearchsetssourceUrl: undefinedon every result, so a correct answer built from it has nothing it could cite. Under a flat requirement it fails forever and collapses every time the fallback is in play.Closing the second needed a third state, so
RuleResultgainedapplicable: "did not cite" and "had nothing citable" are different facts. Zero retrieval is a third case again and still fails — treating it as not-applicable let a long uncited reply built on nothing publish under enforcement, which is the doc's Case A exactly.Report mode is the default
lintDraftcomputes the verdict and changes nothing unless asked to enforce, carryingwouldCollapseas the counterfactual. Enforcing means a misfiring rule withholds a correct answer from a real person, so the order is: report mode → read what it would have collapsed against real traffic → enforce once the false-positive rate is measured rather than assumed.It returns a verdict, never replacement copy. The caller substitutes its own — in the pipeline, the existing
SUPPRESSED_RESPONSE_TEXT. That copy already promises a human, and #231 records what happens when two layers each add their own.The metric and the publish gate are separated
A review pass flagged that failing
grounded-identifiersat one unsourced identifier is stricter thangroundedness.ts, which suppresses atSUPPRESS_AT_UNSOURCED_IDENTIFIERS = 2— its own comment reasoning that "one could be a formatting artifact; two is a pattern of fabrication". Matching production would have dropped the doc's criterion, which is zero invented names.So
RuleResultcarriesblocksPublishalongsidepassed: the harness scores the metric, one occurrence fails it and appears in the report; the linter gates on the pipeline's threshold, so one does not withhold the answer. Every other rule has the two agree.That is the call I would most like checked — a design decision rather than a fix, and a single threshold either way is defensible.
Not wired into the pipeline here
The wiring belongs in
pipeline.ts, and a report-mode linter changes nothing until someone reads its output. Now that #242 has landed, that wiring is unblocked and is the natural next slice.Verification
aipackage 345 tests,turbo run test10/10,tsc --noEmitclean.turbo run lintfails onmaintoday for reasons unrelated to this branch — the ESLint 9 flat-config migration is #250.Nine mutations, each killing the tests that name it: laundering-permissive citation check · always-applicable citation rule · report mode withholding · enforce counting inapplicable rules · zero-retrieval treated as not-applicable · linter gating on
passed· bareincludes· dead-package excused by a live package alone · case-sensitive compare.One initially survived — nothing pinned
formatReport's failing-cases output, so an unfiltered block printedn/afor a rule and then listed it as a failure. That test was added and the mutation now dies.