Skip to content

feat(ai): lint a draft against the reply rules before it publishes - #249

Closed
NathanTarbert wants to merge 2 commits into
feat/response-quality-eval-harnessfrom
feat/draft-linter
Closed

feat(ai): lint a draft against the reply rules before it publishes#249
NathanTarbert wants to merge 2 commits into
feat/response-quality-eval-harnessfrom
feat/draft-linter

Conversation

@NathanTarbert

Copy link
Copy Markdown
Collaborator

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.

Based on feat/response-quality-eval-harness (#241), not main — it consumes that rule module, so the diff here is only this work. Rebases onto whatever that review lands.

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 had to come first

Both would have made the linter withhold correct answers — the same direction as the groundedness gate suppressing one, which is what #234 was filed for.

  • Laundering. The citation rule tested whether a link looked like ours, so a reply could put its invented hook name inside a docs.copilotkit.ai URL and satisfy the rule with a page that does not exist. assessGroundedness blanks URLs before it looks, so the identifier rule cannot catch it either. Now matched against the URLs actually retrieved.
  • The 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 it fails forever and, once rules gate publishing, collapses every time the fallback is in play.

Closing the second needed a third state, so RuleResult gained applicable: "did not cite" and "had nothing citable" are different facts. That also fixed a rule inapplicable everywhere reading as a clean sweep, since passed === total is trivially true at 0/0 — formatReport now prints n/a.

Two deliberate choices

Report mode is the default. lintDraft computes the verdict and changes nothing unless asked to enforce, carrying wouldCollapse as 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.

Not wired into pipeline.ts here. #242 is editing that file on another branch, and a report-mode linter changes nothing until someone reads its output, so wiring follows once #242 lands.

Review round, and the judgment call worth a second opinion

A review pass turned up seven items; all seven held under verification against the real functions. Two are worth calling out.

A hole that would have shipped. applicable treated "retrieval returned nothing" the same as "retrieval returned results with 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. That is Case A exactly, the one input where citing matters most, and the case-A fixture only avoided exposing it by sitting under the handoff cap at 48 words. Empty retrieval now still fails; the escape is scoped to results that carry no URL.

The metric and the gate are not the same thing, and that needed a decision. The linter failed grounded-identifiers at one unsourced identifier while groundedness.ts suppresses at SUPPRESS_AT_UNSOURCED_IDENTIFIERS = 2 — its own comment reasoning that "one could be a formatting artifact; two is a pattern of fabrication". So the linter was stricter than the pipeline beside it.

Matching production would have dropped the doc's actual criterion, which is zero invented names. So RuleResult now carries blocksPublish alongside passed: 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'd most like checked — it is a design decision rather than a fix, and the alternative (one threshold, whichever way) is defensible too.

Also from that round: the laundering had only moved a level deeper (includes accepted anything appended to a retrieved URL, and retrieval returns section URLs, so …/reference/hooks/useCopilotFabricated satisfied …/reference); the citation compare was case-sensitive on the host, which withholds a correctly-cited answer; the dead-package carve-out was a no-op on Case B's own failure, since naming a live package excused the dead one and naming both as current is version-mixing; and the index.ts comment claimed a dist guarantee that does not hold — tsc emits per file, so dist/eval/harness.js ships the fixtures regardless.

Verification

ai package 318 → 345. Full repo turbo run test 10/10. tsc --noEmit clean.

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 · bare includes · dead-package excused by a live package alone · case-sensitive compare.

One mutation initially survived — nothing pinned formatReport's failing-cases output, so an unfiltered block printed n/a for a rule and then listed it as a failure. That test was added and the mutation now dies. Recording it because a surviving mutation is the only honest signal that a fix wasn't actually tested.

Two tests were updated rather than patched around: both cited arbitrary docs-shaped URLs absent from their fixture's sources, which is the laundering the new check closes.

@linear-code

linear-code Bot commented Aug 31, 2026

Copy link
Copy Markdown
CPK-8078 Phase 3 — Let it say less: prompt rewrite plus a rule checker on every draft (doc steps 2, 3)

Doc steps 2 and 3, plus the brevity rules. Root cause #2: a short honest answer is currently impossible to produce.

The prompt as it stands

generator.ts:29-42, unchanged on main:

- Always include code examples when relevant (TypeScript/React preferred)
- Structure responses with **bold headers**, bullet points, and code blocks
- End with a relevant follow-up suggestion or "Was this helpful?"

Formatting is mandatory; having something to say is not. That is the whole of root cause #2. Add verdict-first, one approach not three, a real two-sentence route, and a word cap on handoffs (the doc suggests 60).

The linter has to land with or before the loosening

"A short honest reply is always the fallback" is only true if something enforces it. Nothing in the repo does today — no banned-phrase, hedged-name, or version-mixing check anywhere. The doc's flowchart hangs on one arrow: a draft that breaks a rule collapses into the two-sentence version rather than being cleaned up and posted.

Checks, all mechanical:

  • Banned phrases — "Great question", "What I can't do from here", issue-writing advice, self-commentary about the agent's own limits, claims it can't see the thread.
  • Hedged names — "or the equivalent hook" means it's guessing; drop the name.
  • Version mixing — v1 and v2 hooks never in one snippet. Never mention @copilotkitnext at all; it merged into v2.
  • Every named identifier appears in retrieved context, or it doesn't get written.
  • A v1 question gets a real v1 answer plus "v2 is the path forward" — not a redirect, not a different question answered.

Fixes appendix cases B and D.

Step 2 — stop replying to our own team

Nothing in the AI path maps maintainer accounts. teamMember lookups exist only in discord-bot/src/commands/assign.ts:49 and lib/tickets.ts:44. The staff filter the doc mentions can't recognise maintainers whose Discord accounts were never linked to their team records — which is how appendix case C happened (replied to Nathan, complimented his community spirit, claimed it couldn't read the thread).

Needs maintainer account mapping plus a hard allowlist, and the reply gate: once a maintainer posts in a thread, the agent is out unless -mentioned.

Overlaps #174 — replace the one-response counter with a human-presence gate. Probably the same change; worth checking before building both.

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
@NathanTarbert
NathanTarbert force-pushed the feat/response-quality-eval-harness branch from d1b74de to 1ef00fd Compare September 4, 2026 15:03
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
NathanTarbert deleted the branch feat/response-quality-eval-harness September 7, 2026 13:23
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant