fix(skills): allow single-letter languages C, R, and D (#832) - #840
fix(skills): allow single-letter languages C, R, and D (#832)#840shubhransh-gupta wants to merge 1 commit into
Conversation
|
Hi — ping for review when convenient 👋 Fixes #832: allows single-letter skill tokens Happy to tweak if you'd prefer a different allowlist approach. Thanks! |
s-annam
left a comment
s-annam
left a comment
There was a problem hiding this comment.
The parser change itself is exactly right — it matches #832's implementation plan verbatim, the allowlist is scoped to the defensible three, the early return is placed correctly, and I verified on the fixtures that every newly-admitted token is genuinely drawn on the page (no spurious single characters anywhere in the corpus). What's missing is the bookkeeping the fix obliges: the corpus gate is red on this branch, so CI cannot pass as-is.
Reviewed at fd8b734c12eb20a61aa7ca936cc585bc52acddae.
Blocking
1. npx vitest run src/lib/heuristics/corpus.test.ts fails — 5 fixtures. CI (verify) will be red.
Four are stale baked snapshots the fix invalidates, one is the ratchet's second tooth:
FAIL google-docs/google-docs-skia-proxy-role-first-experience.pdf skillsCount 11 → 12
FAIL latex/deedy-resume-macfonts.pdf skillsCount 22 → 23
FAIL latex/deedy-resume-openfonts.pdf skillsCount 22 → 23
FAIL unknown/pdflib-leading-glyph-skills-header.pdf skillsCount 8 → 9
FAIL latex/multi-degree-coursework.pdf
AssertionError: 'skills' now matches ground truth (expected 18, parsed 18, matched 18)
— remove its knownWrong entry (cites #null)
I checked each new token against the drawn page — all four are correct gains, not noise (C on the two deedy fixtures and on skia-proxy, R on pdflib-leading-glyph-skills-header, whose Skills block genuinely reads Proficient in: Python, SQL, R). So this is a re-bake, not a regression:
npm run bake-fixtures # or: UPDATE_FIXTURES=1 npx vitest run src/lib/heuristics/corpus.test.tsRe-baking gives exactly skillsCount +1 and extractedCharCount +1 on those four .expected.json files and nothing else — please confirm that's all your diff shows before committing it.
2. Fixes #832 while steps 3 and 4 of the issue's plan, and two of its acceptance criteria, are unimplemented.
The merge closes #832, so the ground-truth bookkeeping would ship as "done" and never be reopened. npm run check:baselines on this branch still reports 9 unfiled — the AC asks for 2 fewer (7).
tests/fixtures/pdfs/latex/multi-degree-coursework.truth.json→skills: delete theknownWrongentry. It records only theCdrop, which this PR fixes; leaving it is what fails fixture 5 above. (Per the issue: when the fix lands in the same PR, delete rather than flip — and change no truth values.)tests/fixtures/pdfs/google-docs/google-docs-skia-proxy-role-first-experience.truth.json→skills: this entry must stay, because the field still disagrees for a second, unrelated reason —Fluent in Spanishis still admitted as a skill token (#833, open). Flip it to"status": "open","issue": 833, and rewrite thenoteso it describes only that remaining disagreement; theC-drop half is no longer true after this PR. That takes it out of the unfiled count too.src/lib/heuristics/corpus.test.ts→UNFILED_TRUTH_CEILING: 9 → 7. (The issue says "lower by 2 (10 → 8)", but #838 already landed 10 → 9 onmain; the docblock's rule is that the ceiling tracks the actual count, so 7 is the number here. On a rebase conflict, take the lower.)
After those three edits, corpus.test.ts and check:baselines should both be green — please paste the check:baselines tail showing 7 unfiled.
Secondary
3. Test plan doesn't cover the gates this change actually moves. The one listed box (skills.test.ts) is unticked, and a skills-tokenizer change necessarily crosses the baked corpus, the truth ratchet, and the baselines gate — which is precisely how the five failures above got past the PR. Please run and tick npm run verify before the next push.
Nits (non-blocking)
4. The negative test asserts toEqual([]) on a line where every token is expected to be rejected, so it would still pass if the Skills: label path ever started dropping the whole cell — the assertion can't distinguish "rejected the noise" from "rejected everything". A positive control inside the same line pins it: expect(tokenizeSkillLine("Skills: Python, x, J, C")).toEqual(["Python", "C"]) (verified — that is what it returns on this branch). Inline comment below.
5. The commit subject (Allow single-letter language skills C, R, and D in parser.) isn't in the Conventional Commits shape the PR title already uses (fix(skills): …). Low impact — the merge queue derives the squash message from the PR title — but worth matching when you amend for the fixes above. Keep it to one commit (amend, don't stack).
Gates run
| Gate | Result |
|---|---|
npm run typecheck |
pass |
npm run lint |
pass |
npx vitest run src/lib/heuristics/extract/skills.test.ts |
pass (47) |
npx vitest run src/lib/heuristics/corpus.test.ts |
FAIL (5) — Blocking 1 |
npx vitest run src/lib/heuristics/corpus-roundtrip.test.ts |
pass (61) — the single-char skill survives parse → export → re-parse, no new KNOWN_FAILURES rows |
npm run check:baselines |
9 unfiled (unchanged; AC wants 7) — Blocking 2 |
npx fallow audit --base origin/main |
no issues in the 2 changed files |
| Fixture PII (3a) | skipped — no fixture binaries or truth sidecars touched |
| Design system / tokens (3b, 3c) | skipped — no UI code touched |
| Script/skill command review (3e) | skipped — no scripts touched |
Verdict rule: ≥1 Blocking → REQUEST_CHANGES. No auto-fix was applied and nothing was pushed to your branch, per that rule. Everything above is mechanical — the hard part (the parser change) is already done and correct.
Reviewed by: Claude Opus 5 (high)
| const SINGLE_LETTER_SKILLS = new Set(["c", "r", "d"]); | ||
|
|
||
| function isSkillToken(tok: string): boolean { | ||
| if (tok.length === 1 && SINGLE_LETTER_SKILLS.has(tok.toLowerCase())) return true; |
There was a problem hiding this comment.
This line is correct and matches #832's plan exactly — including the early return, which is what lets an allowlisted token skip the numeric / URL / date-range / word-count checks below.
What it also does, though, is move four baked corpus snapshots and clear one knownWrong exemption, and those files have to move in the same commit or CI is red. See Blocking 1 and 2 in the review body — npm run bake-fixtures, delete the multi-degree-coursework skills exemption, flip the skia-proxy one to open/#833, and lower UNFILED_TRUTH_CEILING 9 → 7.
| }); | ||
|
|
||
| it("still rejects other single-character noise tokens", () => { | ||
| expect(tokenizeSkillLine("Skills: x, J, •, (")).toEqual([]); |
There was a problem hiding this comment.
Nit (non-blocking): every token on this line is expected to be rejected, so toEqual([]) also passes in the world where the Skills: sub-label path drops the whole cell — it cannot tell "rejected the noise" from "rejected everything".
A positive control in the same line pins it:
expect(tokenizeSkillLine("Skills: Python, x, J, C")).toEqual(["Python", "C"]);I ran that on this branch — it returns exactly ["Python", "C"], so it asserts the allowlist admits C, rejects x and J, and that the surrounding cell path is live, all at once. Keep the •/( case too if you like; just don't let it be the only assertion.
|
Welcome, @shubhransh-gupta — this is your first PR here, so a few extra words on what the review above means and how we work. The short version: the change itself is correct, and On the changeThe parser fix is right, and it matches the implementation plan in #832. Each newly-admitted single-letter token was checked against what is actually drawn on the page — no spurious single characters slipped in. That was the risky part of #832 and you got it right. What's left, and why it blocksBoth items are mechanical:
These would have turned CI red on merge, which is the only reason they're marked blocking rather than left as review notes. How we handle small stuff — our nit policyWorth knowing up front, since it'll apply to your next PR: when a review turns up zero blocking findings, we don't bounce the PR back for nits. A maintainer fixes the small items directly, pushes a commit to your branch, and approves in the same pass — including that fix commit. It saves a round-trip on things like a missing SPDX header or a reworded comment. That did not happen here: the presence of blocking findings means we leave your branch untouched, so nothing was pushed to it and the code you see is still exactly the code you wrote. If you'd rather we never push to your branches at all, say so and we'll respect it. One mechanical note: this PR comes from your fork, so maintainer pushes only work if "Allow edits by maintainers" is checked on the PR. If you want the nit-fix path available on future PRs, leave that on. Pointers
Anything unclear — the process, the fixture corpus, why a gate is shaped the way it is, or if you disagree with a finding — just tag me here or open a thread. Pushback on a review is welcome; I'd rather discuss it than have you silently absorb a call you think is wrong. Thanks again for picking this up. |
|
Thanks for the thorough review, @s-annam — all blocking items addressed in one amended commit (rebased onto latest Blocking 1 — corpus re-bake
Blocking 2 — ground-truth bookkeeping
Secondary + nit
|
47f073a to
b212752
Compare
|
CI fix pushed — |
|
@s-annam — friendly re-review ping 👋 All blocking items from the earlier review are addressed and CI is green on the latest push (
Ready for another look whenever you have a moment. Thanks! |
s-annam
left a comment
s-annam
left a comment
There was a problem hiding this comment.
Every blocking item from the last round is genuinely fixed, and I verified the bookkeeping independently rather than taking the summary's word for it — all four re-baked snapshots gained a token that is really drawn on the page, and every gate I can run locally is green. One Blocking item remains, and it is not something you introduced this round: the commit carries a Co-authored-by: Cursor trailer, which this repo forbids and which would land in main permanently. It was already there at fd8b734, the sha I reviewed last time, and I missed it. Apologies for surfacing it only now — it is a one-command fix and it is the only thing between this PR and an approval.
Reviewed at b2127526c1ea528fb12504f4a6672278beafda0e.
Last round's blockers — all clear
| Last round | Now |
|---|---|
Blocking 1 — corpus.test.ts red on 5 fixtures |
Fixed. 65 tests pass. The four re-bakes are skillsCount +1 / extractedCharCount +1 and nothing else, exactly as asked. |
Blocking 2 — Fixes #832 with steps 3–4 unimplemented |
Fixed. check:baselines reports 7 unfiled, 1 charged to a live issue. |
| Secondary 3 — test plan didn't cover the gates | Partly — verify was run (thank you), but the PR body still doesn't say so. See Secondary 2. |
| Nit 4 — negative test couldn't distinguish rejecting noise from rejecting everything | Fixed. Positive control added and it is exact. |
| Nit 5 — commit subject not Conventional Commits | Fixed. fix(skills): …, one commit. |
I re-derived the four re-bakes from the drawn page rather than trusting the diff:
deedy-resume-macfonts/-openfonts— both drawC • C++ • CSS • PHP • Assembly; the gain isC.google-docs-skia-proxy-role-first-experience— drawsProgramming Languages: C, HTML, CSS, Java, VHDL, Assembly; all six now parse.pdflib-leading-glyph-skills-header— drawsProficient in: Python, SQL, R; the gain isR.
No other fixture's snapshot moved, which is the proof that nothing spurious was admitted across the corpus.
The truth-sidecar work is right in both directions: multi-degree-coursework has its knownWrong deleted (not flipped) with no truth values touched, and skia-proxy is flipped to "issue": 833, "status": "open" with a note that describes only the remaining Fluent in Spanish disagreement. #833 is open, so the entry is legitimately charged.
Acceptance criteria (#832)
All seven met. isSkillToken is exercised through tokenizeSkillLine, its only public path, which is the stronger test; multi-degree now parses both C and C++; roundtrip adds no KNOWN_FAILURES rows; check:baselines is down by exactly 2.
I also traced the early return for holes and found none: isSkillToken has a single call site (tokenizeCell), the token is already .trim()ed and stripped of trailing .!?,; before it arrives, and every check the early return skips (/^\d+$/, looksLikeContactLink, the date-range regex, the >6-word guard) is structurally unreachable for a one-character letter. The export path middot-joins skills with no minimum-length filter, so a lone C survives the round trip — which the roundtrip suite confirms empirically.
Blocking
1. The commit carries Co-authored-by: Cursor <cursoragent@cursor.com>, which would land in main verbatim and permanently.
CLAUDE.md → Hard rules (no exceptions): "No AI attribution in git. … Never commit attribution trailers (like Co-Authored-By: or Claude-Session:) or PR badges." It is one of only three rules kept in CLAUDE.md rather than delegated to a gate, precisely because breaking it is silent and permanent — docs/CONTRIBUTING-PROCESS.md records ~16 stray trailers that reached main before anyone noticed.
This is not cosmetic, and it is not something a maintainer can strip at merge time:
mainis governed by an activemain-merge-queueruleset withmerge_method: SQUASH, so the queue composes the squash commit itself — nobody hand-edits the message on the way in.- The repo's
squash_merge_commit_messageisCOMMIT_MESSAGES, so your commit body becomes the squash body verbatim. - GitHub parses
Co-authored-by:as a real trailer, soCursor <cursoragent@cursor.com>would be recorded as a co-author of the commit inmainand in the contributor graph.
Fix — amend and force-push. This is your own fork branch, so a force-push here is safe and entirely yours to make:
git commit --amend # delete the Co-authored-by line; keep the subject and body as they are
git push --force-with-lease origin fix/issue-832-single-letter-skillsCursor adds that trailer automatically, so it is worth switching it off in Cursor's commit settings — otherwise it will come back on your next PR.
Nothing else about the commit needs to change: the subject is correct Conventional Commits, the body accurately describes the whole change, and the branch is a single commit rebased cleanly onto main.
Secondary
2. The PR description now covers about a third of its own diff.
The ## Summary still names only the allowlist and the unit tests — the two source-file bullets. Seven of the ten changed files are the bookkeeping half (four re-baked snapshots, two truth sidecars, UNFILED_TRUTH_CEILING 9 → 7) and go unmentioned, and the ## Test plan still lists a single unticked skills.test.ts box even though you ran npm run verify and said so in a comment.
The description is what a bisecting maintainer reads a year from now when the code has stopped being fresh in anyone's mind, so an accurate one is worth the minute. Suggested replacement:
## Summary
- Allowlist `C`, `R`, and `D` in `isSkillToken` without lowering the global 2-char noise floor
- Re-bake four corpus snapshots whose `skillsCount` legitimately gains the newly-admitted token
- Ground-truth bookkeeping for #832: delete `multi-degree-coursework`'s `skills` `knownWrong`
entry (fixed here), flip `skia-proxy`'s to `#833`/`open` for the remaining
"Fluent in Spanish" disagreement, and lower `UNFILED_TRUTH_CEILING` 9 → 7
- Add unit tests for the allowlisted languages, rejected noise tokens, and a mixed-line
positive control
## Test plan
- [x] `npx vitest run src/lib/heuristics/extract/skills.test.ts`
- [x] `npx vitest run src/lib/heuristics/corpus.test.ts`
- [x] `npx vitest run src/lib/heuristics/corpus-roundtrip.test.ts`
- [x] `npm run check:baselines` — 7 unfiled
- [x] `npm run verify`Nits (non-blocking — ignore any you disagree with)
3. expect.arrayContaining on the C, R, D assertion is looser than it needs to be; the exact return is ["C", "R", "D"], which I verified on this branch. Suggestion inline.
4. The #221 test was widened to carry a #832 case. Now that #832 has its own describe block right below, reverting that line keeps each test attributable to the issue it guards. Suggestion inline.
5. No action needed, just so you know the shape of your own suite: the "still rejects other single-character noise tokens" test passes identically with the fix reverted — x, J, •, ( are all rejected by the pre-existing 2-char floor. That is exactly what a negative control is for, so it earns its place; it is the positive control on the line below it that actually pins the new code.
Gates run
| Gate | Result |
|---|---|
npm run typecheck |
pass |
npm run lint |
pass |
npx vitest run src/lib/heuristics/extract/skills.test.ts |
pass (48) |
npx vitest run src/lib/heuristics/corpus.test.ts |
pass (65, 1 skipped) — was 5 failures last round |
npx vitest run src/lib/heuristics/corpus-roundtrip.test.ts |
pass (63, 1 skipped) — no new KNOWN_FAILURES rows |
npm run check:baselines |
7 unfiled, 1 charged to live issue #833 — matches the AC |
npm run check:fixtures |
pass — 60 PDFs + 16 truth sidecars, all personas synthetic |
npx fallow audit --base origin/main |
pass — no issues in the 10 changed files (the 4 complexity findings are inherited and untouched by this diff) |
CI verify + fallow |
both green on b212752 |
| Fixture PII (3a) | ran — no new binaries; the two edited truth sidecars are swept by check:fixtures |
| Design system / tokens (3b, 3c) | skipped — no UI code touched |
| Script / skill command review (3e) | skipped — no scripts or skills touched |
Verdict rule: ≥1 Blocking → REQUEST_CHANGES. Nothing was pushed to your branch and nothing was auto-fixed — with a blocking finding present we leave a contributor's branch alone, and on a fork we would not force-push it in any case. The two nits are inline as one-click suggestions if you want them.
Genuinely good work on the bookkeeping round — that was the fiddly, unglamorous half of #832 and you got every piece of it right.
Reviewed by: Claude Opus 5 (high)
| expect(tokenizeSkillLine("Languages: C, R, D")).toEqual( | ||
| expect.arrayContaining(["C", "R", "D"]), | ||
| ); |
There was a problem hiding this comment.
Nit (non-blocking). expect.arrayContaining passes even if the tokenizer returns extra tokens alongside these three, so it under-specifies what the fix guarantees. The exact return on this branch is ["C", "R", "D"] — I ran it — so a strict toEqual costs nothing and pins the behaviour precisely.
| expect(tokenizeSkillLine("Languages: C, R, D")).toEqual( | |
| expect.arrayContaining(["C", "R", "D"]), | |
| ); | |
| expect(tokenizeSkillLine("Languages: C, R, D")).toEqual(["C", "R", "D"]); |
| expect(tokenizeSkillLine("Languages: Python, Go, C++, Java, C")).toEqual( | ||
| expect.arrayContaining(["Python", "Go", "C++", "Java", "C"]), |
There was a problem hiding this comment.
Nit (non-blocking). This is the #221 sub-label test, widened to also carry a #832 case. Now that #832 has its own describe block immediately below, reverting this line keeps each test attributable to the issue it guards — useful later when someone bisects one of the two and wants to know which assertions are load-bearing for which fix.
Both forms pass (["Python", "Go", "C++", "Java", "C"] is the actual return either way), so this is purely about attribution.
| expect(tokenizeSkillLine("Languages: Python, Go, C++, Java, C")).toEqual( | |
| expect.arrayContaining(["Python", "Go", "C++", "Java", "C"]), | |
| expect(tokenizeSkillLine("Languages: Python, Go, C++, Java")).toEqual( | |
| expect.arrayContaining(["Python", "Go", "C++", "Java"]), |
|
Thanks for the fast turnaround, @shubhransh-gupta — and for taking on the unglamorous half of #832. Resolved: every blocking item from the last round. I re-derived the bookkeeping from the fixtures rather than taking the summary's word for it — all four re-baked snapshots gained a token that is genuinely drawn on the page ( Still standing: one item, and it isn't a regression on your part. The commit has carried a One addition to the nit policy I described earlier, since this round is the case it didn't cover: when we can't push to your branch — either because a blocker means we leave it alone, or because a fork hasn't enabled maintainer edits — the small stuff goes out as GitHub Nothing else is outstanding. Once that trailer is gone this should approve cleanly. Questions or pushback on any of it, just tag me (@s-annam) here. |
Allowlist C/R/D in isSkillToken, re-bake four corpus snapshots, file skills knownWrong bookkeeping (delete multi-degree entry, flip skia-proxy to offlinecv#833), lower UNFILED_TRUTH_CEILING 9→7, add positive control test.
b212752 to
668f470
Compare
s-annam
left a comment
s-annam
left a comment
There was a problem hiding this comment.
Approving. The one blocking finding from the last round is gone, and I re-derived that rather than taking the amend on trust: b212752 and 668f470 have the identical tree (8048166) and the identical parent, so the only thing that changed is the commit message — the Co-authored-by: Cursor trailer is deleted and no code moved with it. git log -1 --format=%B on the head is now four lines of description and nothing else, which is what the merge queue will squash into main.
Verdict rule: 0 Blocking → APPROVE. Secondary and Nits below are non-blocking and I have pushed nothing to your branch (see the last section for why, since it differs from what I described two rounds ago).
Blocking
None.
Secondary
The PR description covers about a quarter of the diff. ## Summary names the allowlist and the unit tests. The diff also re-bakes five expected.json snapshots (not four — latex/multi-degree-coursework.expected.json moves skillsCount 17 → 18 alongside the two deedy fixtures, skia-proxy and pdflib-leading-glyph), deletes one knownWrong entry, flips another to #833/open, and drops UNFILED_TRUTH_CEILING 9 → 7. Those are the half of #832 that took the most care, and a reader of the PR page cannot see that they happened.
Two things keep this off the blocking list. The commit message body is accurate and complete — it names all four workstreams — and that is the text the merge queue composes main's squash message from, so the permanent record is right. And the omission is under-claiming, not over-claiming: nothing in the body asserts a behaviour the code lacks. Worth fixing on the PR page anyway, because the PR is what the next person reads when they bisect to this commit and want to know why five snapshots moved.
Nits
- The docblock records what is in the set, not what was deliberately kept out. Inline on
skills.ts:141. - The test plan lists only
skills.test.ts. That suite could not have caught the actual risk in this change — a spurious single character admitted on some other fixture. The gates that cover it arecorpus.test.ts(the whole snapshot corpus) andcorpus-roundtrip.test.ts(the middot-joined skills header has to survive parse → export → re-parse), pluscheck:baselinesfor the ceiling. Both AC lines in #832 name them. Non-blocking because you demonstrably ran them — the snapshots are correctly re-baked andverifyis green — the box just does not say so.
What I verified
Independently, against the binaries and the gates, not against the summary:
- Every re-baked token is genuinely drawn on the page.
pdftotexton each:C • C++ • CSS • PHP • Assemblyon both deedy fixtures,Programming Languages: C, HTML, CSS, Java, VHDL, Assemblyon skia-proxy,Languages: Python, Go, C, C++, …on multi-degree-coursework, andProficient in: Python, SQL, Ron pdflib-leading-glyph. Five fixtures, five real tokens,+1char each. No fixture gains a spurious single character. - AC checklist for #832: all met. The allowlist sits before the floor with an early return; matching is case-insensitive; the set is exactly
c/r/dwith noJ/K/Q/Fcreep;multi-degree-coursework's entry is deleted rather than flipped (correct — the fix lands in the same PR) with no truth values touched; skia-proxy carries#833and the note still names both disagreements, which is what the issue thread asked for;UNFILED_TRUTH_CEILINGdrops by 2 frommain's current 9, honouring the "take the LOWER number on a rebase conflict" instruction. - Gates run locally on
668f470:skills.test.ts+corpus.test.ts+corpus-roundtrip.test.ts→ 175 passed, no newKNOWN_FAILURESrows.check:baselines→ 7 unfiled, matching the new ceiling exactly.check:fixtures→ 60 PDFs and 16 sidecars, all personas synthetic.typecheck,lintclean.fallow audit --base origin/main→ no issues in the 10 changed files; the four complexity findings it prints are inherited and report-only. - Gate 3b (design system) and 3c (style tokens) do not apply — this diff touches no component and no class name.
On the auto-fix path, since this round is the case that decides it
Two rounds ago I said that with zero blockers we fix the small stuff ourselves, push it, and approve in the same pass. There are zero blockers now and I still pushed nothing, so the reason should be on the record rather than left as an inconsistency.
main merges through a queue that derives the squash message from the branch, so a PR has to reach the queue as one commit. When the branch is ours, the auto-fix push is followed immediately by a collapse back to one commit. On a fork branch that second half is unavailable: collapsing means force-pushing over your branch, which rewrites your authorship and destroys whatever you have locally, and we do not do that to a contributor's branch under any circumstance. So a nit push here would leave the PR at two commits with no permitted repair — trading a one-sentence docblock for a real invariant violation.
Hence: nit 1 is written out below as text to fold into your existing commit with git commit --amend if you want it, and it is genuinely fine to skip. Allow edits by maintainers being on is still useful, but the collapse constraint, not permissions, is what governs here.
Nothing outstanding. This is good work on the unglamorous half of the issue, and the amend was clean.
Reviewed by: Claude Opus 5 (high)
| /** One-character tokens that are real, commonly-listed languages. The | ||
| * `tok.length < 2` floor in `isSkillToken` is a noise guard against stray | ||
| * glyphs left by column splitting; these are the only single characters that | ||
| * are not noise, so they are allowlisted rather than lowering the floor. */ | ||
| const SINGLE_LETTER_SKILLS = new Set(["c", "r", "d"]); |
There was a problem hiding this comment.
Nit — the docblock says what is in the set, not what was deliberately kept out.
#832 makes the exclusions the interesting decision: J, K, Q and F are all real language names and were left out on purpose, because each is also a plausible stray glyph and admitting them "trades a rare recall win for a common precision loss." That reasoning exists only in the issue. A year from now someone reads these four lines, notices J is missing, and re-litigates it from scratch — or just adds it.
One sentence fixes it. Non-blocking, and fine to skip. If you do want it, fold it into the existing commit with git commit --amend rather than adding a second commit — the review body explains why I am not offering this as a one-click suggestion:
/** One-character tokens that are real, commonly-listed languages. The
* `tok.length < 2` floor in `isSkillToken` is a noise guard against stray
* glyphs left by column splitting; these are the only single characters that
* are not noise, so they are allowlisted rather than lowering the floor.
* Deliberately excludes J/K/Q/F (#832): each is a real language name, but
* each is also a plausible stray glyph, so admitting them trades a rare
* recall win for a common precision loss. */
|
Approved — thanks for the quick amend, @shubhransh-gupta. Resolved: the One thing worth flagging, because this round is the first where the nit policy I described earlier actually applied and I still didn't use it. Zero blockers means we normally fix the small stuff ourselves and push it to your branch — but Nice work on the half of #832 nobody enjoys — the sidecar bookkeeping and the ceiling drop were the fiddly part and you got both directions right. A maintainer will take it from here on merging. Questions or pushback on any of it, tag @s-annam. |
2 participants
Fixes #832
Summary
isSkillTokenwithout lowering the global 2-char noise floorTest plan
npx vitest run src/lib/heuristics/extract/skills.test.ts