fix(ai): strip an ID-selector prefix before the grounding lookup - #239
Conversation
`identifierSegments` split only on `.`, so an ID selector reached the grounding lookup with its prefix attached. Grounding is a substring check, so `#copilotKitPanel` could never match a source writing the bare `copilotKitPanel` — which is how the docs write it. Two such names then met SUPPRESS_AT_UNSOURCED_IDENTIFIERS and withheld a fully grounded answer, with nothing logged. Same failure class #222 was opened to close. The class-selector case passed only because `split('.')` happened to strip its prefix; `#` had no equivalent path. Now splits on `[.#]`, so the prefix is gone before both the shape check and the grounding lookup. Reproduced by execution before fixing: 'Target `#copilotKitPanel` and `#copilotKitSidebar` to reposition it.' against a source containing both bare names returned unsourcedIdentifiers for both, suppress: true, penalty 0.30. Three tests, added red: the prefix strip, the ID-and-class spellings of one name folding to a single key so one name cannot fill both threshold slots, and the suppression consequence itself. Refs #234
jerelvelarde
left a comment
jerelvelarde
left a comment
There was a problem hiding this comment.
Reviewed deeply plus an adversarial round on every finding. No blockers — this is a correct, minimal fix and the reasoning in the body holds up.
Verified
- The widened split provably cannot over-split.
IDENTIFIER_PATH(/^[.#]?[A-Za-z_$][\w$-]*(?:\.[A-Za-z_$][\w$-]*)*$/) runs before the split and admits#only in position 0 —\wexcludes#, and the dotted-tail alternation requires[A-Za-z_$]after each.. I ran the adversarial cases:this.#copilotFoo,a#b()and#copilot#Kitare all rejected byIDENTIFIER_PATHbeforeidentifierSegmentsever sees them. - The class-selector path from #222 is untouched.
CSS_CLASS_PATTERNpushesmatch[1](prefix already stripped by the capture group) straight intohitsand never reaches this function, and the lookbehind guard is unmodified. That was my main worry given how recently that file was hardened, and it is clean. - The shape check still discriminates, because it runs on post-split segments.
- All three new tests die under the obvious mutation, and no consumer of
unsourcedIdentifiersdepends on the prefix being present.
On the half you deliberately left out
I agree with the call, and I want to record why for whoever picks it up. Widening CSS_CLASS_PATTERN to [.#] adds suppression, which is the direction that withholds correct answers — the failure #222 and this PR both exist to stop. And you are right about the trap: this.#copilotFoo passes the lookbehind because the character before # is ., not a word character. So the lookbehind that makes . safe does not make # safe, and that asymmetry needs its own tests rather than being folded in here. The code comment is the right place for it.
Two nits, neither blocking:
- The PR body says the ID-selector case "worked and the class case didn't" — it is the other way round in one sentence. Cosmetic.
treats the ID and class spellings of one name as the same identifieris the test I would most want a second assertion on: it pins that one name cannot fill both threshold slots, which is the property that actually prevents a spurious suppression atSUPPRESS_AT_UNSOURCED_IDENTIFIERS = 2. Worth asserting the resultingsuppress: falseexplicitly as well as the segment set.
Approving.
CPK-8073 Phase 0.1 — Stop the groundedness gate withholding correct answers (#234)
GitHub: #234. Fix is written and green — see below.
With Reproduced by execution before fixing: Fix: split on
PR #239 open, off A |
2 participants
Closes #234.
extractCopilotKitIdentifiersreports an identifier with its selector prefix still attached, and grounding is a substring check, so an ID selector could never match a source that writes the name without the#— which is how the docs write it.IDENTIFIER_PATHadmits both.and#in the leading position, butidentifierSegmentsonly ever split on., so the class-selector case worked and the ID case didn't.With
SUPPRESS_AT_UNSOURCED_IDENTIFIERS = 2, two ID selectors in a fully grounded answer suppressed it outright, with nothing logged. That's a correct answer withheld from a real reporter, which is the same failure class #222 was opened to close.Reproduced by execution before touching the code:
The fix is
token.split(/[.#]/), so the prefix is gone before both the shape check and the grounding lookup.IDENTIFIER_PATHonly admits#leading, so the widened split can't over-split a mid-token#.Tests
Three, each written red first:
strips a leading ID-selector prefix from a backticked nametreats the ID and class spellings of one name as the same identifier— so one name can't fill both threshold slots on its owndoes not suppress ID-selector names the sources actually contain— the user-visible consequence, not just the extractoraipackage 270 → 273. Full repoturbo run test10/10 packages.tsc --noEmitclean.One thing deliberately left out
This closes the false-suppression half only. The two prefixes still reach the extractor by different routes:
CSS_CLASS_PATTERNfinds a class selector anywhere in the response, deliberately including inside a```cssfence (there's an existing test for that), while an ID selector arrives only viaBACKTICKED_PATTERN, which can't span a newline and so never matches inside a fence. So a fabricated#copilotKitFakein a css fence is still invisible where.copilotKitFakeis caught.Closing that means widening
CSS_CLASS_PATTERNto[.#], which adds suppression — the direction that withholds correct answers — and#carries a trap.doesn't:this.#copilotFoopasses the lookbehind, because the character before#is.rather than a word character. That wants its own change and its own tests, so there's a comment in the code recording it rather than a silent gap. Happy to fold it in here instead if you'd rather.