fix(parser): retain single-letter language skills

[qtjg]

PR title

fix(parser): retain single-letter language skills

PR body

Resolves #832

Summary

The skills parser previously dropped one-character programming languages because isSkillToken applied an unconditional two-character minimum. This change allowlists the defensible single-letter languages C, R, and D case-insensitively while preserving the existing noise floor for other stray one-character glyphs.

Changes

• Added a named SINGLE_LETTER_SKILLS allowlist for c, r, and d.
• Added regression coverage proving C, R, and D survive while X remains rejected.
• Removed the resolved skills knownWrong entry from the LaTeX fixture and rebaked the affected corpus snapshots.
• Updated the Google Docs fixture to retain its independent Fluent in Spanish disagreement under issue [parser] skills — a "Language: Fluent in Spanish" proficiency row is admitted as a skill token #833; the separate C-drop disagreement is now resolved by [parser] skills — single-letter language tokens (C, R, D) are dropped by isSkillToken's 2-character floor #832.
• Lowered UNFILED_TRUTH_CEILING from 9 to 8 after filing/resolving the two repeated C-drop measurements.

Verification

• Focused heuristic skills tests pass: 46 tests.
• Corpus regression tests pass: 64 tests, 1 intentionally skipped.
• npm run check:baselines completes with the expected existing unfiled-fixture warnings; the remaining filed Google Docs skill disagreement is linked to [parser] skills — a "Language: Fluent in Spanish" proficiency row is admitted as a skill token #833.
• Typecheck, lint, and production build pass.
• The full npm run verify run reached 358 passing test files and 5,864 passing tests, but is blocked by an unrelated existing useLibraryChanges BroadcastChannel/jsdom failure with three unhandled MessageEvent errors. No issue [parser] skills — single-letter language tokens (C, R, D) are dropped by isSkillToken's 2-character floor #832 tests fail.

[s-annam]

The parser change here is correct and well-scoped — it implements #832's prescribed shape almost exactly, and I verified the behaviour independently rather than taking the diff's word for it. One blocking item, one coverage gap, and a duplication fact the maintainer needs before merging anything.

First, the thing that isn't about your code: #840 resolves the same issue (#832) over the identical ten-file set, has been through two review rounds, and is currently approved and awaiting merge. The production hunk in skills.ts is byte-identical between the two PRs. That's not a mark against this PR — it's a scheduling collision nobody told you about, and it's on us, not you. But it is load-bearing for whoever merges, so it's stated up front.

---

Blocking

1. UNFILED_TRUTH_CEILING is left one slot loose — src/lib/heuristics/corpus.test.ts:186

main sits at 9, and there were exactly 9 unfiled ground-truth entries — the ratchet was tight. This PR resolves two of them (the multi-degree-coursework entry is deleted; the google-docs one flips to issue: 833), so the measured count is now 7, which I confirmed by running the gate on this head:

✓ issue-linked baselines: … 1 ground-truth `knownWrong` entry/entries charged to a live issue, 7 unfiled.

The assertion is toBeLessThanOrEqual(UNFILED_TRUTH_CEILING), so 8 passes — but it leaves a free slot, and the constant's own docblock says the point of the bound is that "undescribed debt may not GROW." At 8, the next PR can add one brand-new unfiled disagreement and this gate stays green. That is the exact regression the ratchet exists to catch.

#832's step 4 also asks for a lowering by 2, and pins the tiebreak: "Sibling issues from the same audit also lower it — on a rebase conflict, take the LOWER number." A sibling already moved 10 → 9 before this branch, so applying the literal 8 from the issue text under-tightens by one.

-const UNFILED_TRUTH_CEILING = 8;
+const UNFILED_TRUTH_CEILING = 7;

One character, and npx vitest run src/lib/heuristics/corpus.test.ts should stay green — I checked that 7 is the true floor, not a guess. It's Blocking rather than a nit only because it silently widens a gate, and because it changes what the test asserts, which puts it out of reach of the reviewer-side auto-fix.

---

Secondary

2. Regression coverage is narrower than #832's acceptance criterion

AC 1 names a specific token set: C / R / D accepted, and x / J / • / ( rejected. The new test asserts only that X is absent from tokenizeSkillLine("C, R, D, X").

I ran the full AC set against this head, and the behaviour is entirely correct — nothing ships broken:

"C, R, D, X"              -> ["C","R","D"]
"Skills: x, J, •, ("      -> []
"Skills: Python, x, J, C" -> ["Python","C"]
"Languages: c, r, d"      -> ["c","r","d"]     # case-insensitivity holds

The gap is that none of rows 2–4 is asserted. J matters most: #832 explicitly says "Do NOT add J, K, Q, or F", so a test pinning J as rejected is the guard against someone widening SINGLE_LETTER_SKILLS later without an argument. Row 3 is the positive control that separates "rejects noise" from "rejects everything" — without it, a bug that made tokenizeSkillLine return [] on any line containing a stray glyph would pass. A suggestion block covering all three is inline on the test.

3. The description's arithmetic doesn't support its own number

"Lowered UNFILED_TRUTH_CEILING from 9 to 8 after filing/resolving the two repeated C-drop measurements."

Resolving two measurements from a tight 9 lands on 7, not 8. The stated rationale is right; the number it produces isn't. Same root as finding 1 — noting it separately because the body is what a bisecting maintainer reads later.

4. npm run verify was not observed green, here or in CI

The body reports verify blocked by a useLibraryChanges BroadcastChannel/jsdom failure, called "unrelated existing." I couldn't reproduce it — npx vitest run src/hooks/useLibraryChanges.test.tsx passes 3/3 on this head, and there's no open issue tracking it. It may well be a load-dependent flake under the full suite; either way it is genuinely unrelated to this diff, so it isn't held against the change. But note there are no CI checks reported on this branch at all (fork PRs need a maintainer to approve the workflow run), so nothing external corroborates green yet.

---

Nits

5. "knownWrong": {} is the only empty one in the corpus. Of 16 truth sidecars, 10 omit the key entirely and none carries an empty object. Suggestion inline.

6. PR body retains the literal template scaffolding — # PR title and # PR body are still in the rendered description above the real content. The content underneath is substantive and specific, which is more than most first PRs manage; the headings just want deleting.

---

Gates run

Gate	Result
npm run check:fixtures	✓ 60 PDFs + 16 sidecars, all personas synthetic
npm run check:baselines	✓ 7 unfiled (see finding 1)
npx vitest run …/skills.test.ts …/corpus.test.ts	✓ 110 passed, 1 skipped
npm run typecheck	✓
npx eslint on changed sources	✓
3a fixture provenance (manual)	✓ every gained token verified drawn — see below
3b design-system / 3c tokens / 3e script-command	n/a — no component, style, or script files touched
One-commit invariant	✓ single commit, clean message, no attribution trailers

Fixture re-bakes verified against the page, not the diff. Five .expected.json files each gain exactly +1 skill and +1 extracted char. pdftotext confirms every one of those tokens is genuinely drawn:

• google-docs-skia-proxy-role-first-experience.pdf → Programming Languages: C, HTML, CSS, Java, VHDL, Assembly
• multi-degree-coursework.pdf → Languages: Python, Go, C, C++, Java, JavaScript, Swift, PHP, HTML, CSS
• deedy-resume-{macfonts,openfonts}.pdf → C • C++ • CSS • PHP • Assembly
• pdflib-leading-glyph-skills-header.pdf → Proficient in: Python, SQL, R

No fixture gained a spurious single-character skill, and no truth value was edited to match the parser — the sidecars still record what the page draws, which is the rule that matters most here.

---

How this compares to #840

Both PRs touch the same ten files. Differences, in full:

	#840 (approved)	#855 (this PR)
skills.ts production hunk	identical	identical (constant sits above looksLikeContactLink instead of below — arguably closer to #832's "beside the other vocabulary constants")
UNFILED_TRUTH_CEILING	7	8 — finding 1
Negative tests (x, J, •, ()	asserted	not asserted — finding 2
Positive-control mixed line	asserted	not asserted
Strengthens the existing #221 test with C	yes	no (left at base)
multi-degree knownWrong	key removed	{} — finding 5

Nothing in #855 is behaviour #840 doesn't already have, and on the two points where they differ substantively, #840 is tighter. Said plainly so the maintainer can decide without re-deriving it — and said without prejudice to this PR, which is independently competent work.

---

Verdict

REQUEST_CHANGES — the rule applied: ≥1 Blocking → REQUEST_CHANGES, regardless of how small the fix is. Finding 1 is one character. With it applied, the Secondary and Nit items are all non-blocking and this would approve.

Nothing was pushed to the branch: this is a fork PR, and the ceiling value is a behavioural change to a gate, so it is a finding for the author rather than a reviewer auto-fix under any circumstances.

---

Reviewed by: Claude Opus 5 (high)

[s-annam]

Blocking (finding 1). The measured unfiled count on this head is 7, not 8 — npm run check:baselines lists exactly seven ⚠ … status "unfiled" entries after this PR resolves the two C-drop ones. main's 9 was tight against 9 real entries, so resolving two lands on 7.

Leaving it at 8 keeps the assertion green but opens one free slot, which contradicts this constant's own docblock three lines above ("undescribed debt may not GROW") — the next PR could add a brand-new unfiled disagreement and this gate would not notice.

#832 step 4 asks for a lowering by 2 and says to take the LOWER number when a sibling issue has already moved it; a sibling moved 10 → 9 before this branch, which is why the literal 8 from the issue text is now one too high.

I'm leaving this as prose rather than a one-click suggestion because it changes what a gate asserts — that's outside the bound on reviewer-applied fixes, even at one character.

[s-annam]

Secondary (finding 2). The behaviour is right — I ran #832's whole AC token set against this head and every row passes. The gap is that only the X rejection is asserted.

J is the one worth pinning: #832 explicitly says "Do NOT add J, K, Q, or F", so a test holding J as rejected is what stops SINGLE_LETTER_SKILLS being widened later without an argument. And the mixed line is a positive control — without it, a regression that made this return [] for any line containing a stray glyph would still pass.

Verified outputs, so the suggestion below is asserting real behaviour rather than a guess:

"Languages: C, R, D"      -> ["C","R","D"]
"Skills: x, J, •, ("      -> []
"Skills: Python, x, J, C" -> ["Python","C"]

Suggested change

	it("keeps the defensible single-letter languages and rejects stray glyphs (#832)", () => {
	const result = tokenizeSkillLine("C, R, D, X");
	expect(result).toEqual(expect.arrayContaining(["C", "R", "D"]));
	expect(result).not.toContain("X");
	});
	it("keeps the defensible single-letter languages (#832)", () => {
	expect(tokenizeSkillLine("Languages: C, R, D")).toEqual(
	expect.arrayContaining(["C", "R", "D"]),
	);
	});
	it("still rejects other single-character noise tokens (#832)", () => {
	// `J` is a real language name but also a plausible stray glyph; #832
	// deliberately keeps it out of the allowlist. Pin that so a later widening
	// of SINGLE_LETTER_SKILLS has to argue for itself.
	expect(tokenizeSkillLine("Skills: x, J, •, (")).toEqual([]);
	});
	it("keeps an allowlisted letter alongside real skills on one line (#832)", () => {
	// Positive control: distinguishes rejecting noise from rejecting everything.
	expect(tokenizeSkillLine("Skills: Python, x, J, C")).toEqual(["Python", "C"]);
	});

[s-annam]

Confirming this line rather than flagging it: the early return true is exactly what #832 asked for, and it earns its keep — an allowlisted letter skips the numeric / URL / date-range / word-count checks below, none of which a bare letter can trip, so the early return states the intent instead of relying on them to be no-ops. toLowerCase() gives the case-insensitivity the issue's step 2 wanted, and I verified "Languages: c, r, d" survives as ["c","r","d"]. No change requested here.

[s-annam]

Nit (finding 5), non-blocking. With the entry resolved, this leaves the only empty knownWrong in the corpus — 10 of the 16 truth sidecars omit the key entirely and none carries {}. Dropping the key matches the neighbours.

Suggested change

	],
	"knownWrong": {
	"skills": {
	"issue": null,
	"status": "unfiled",
	"note": "The single-letter token “C” is DROPPED from the Languages row while “C++” survives. Second independent measurement of the same drop (see google-docs/google-docs-skia-proxy-role-first-experience)."
	}
	}
	"knownWrong": {}
	]

[s-annam]

Welcome, @qtjg — this is your first PR here, so a bit of context that the review above assumes but doesn't spell out.

What stands. The parser change is right. You landed on #832's prescribed shape, including the early return true and the case-insensitive lookup, and every fixture re-bake checks out against what the PDFs actually draw. That last part is the one we're strictest about, and you got it right without being told: the truth sidecars still record the page, not the parser. One clean commit, no attribution trailers, a real description rather than a stub. That's a better-calibrated first PR than most.

What's needed next. One character: UNFILED_TRUTH_CEILING wants 7, not 8 — the measured count after your change is 7, and leaving the ceiling above it hands a free slot to the next PR. Everything else in the review is a suggestion block you can apply from the GitHub UI in one click, or ignore with a reply saying why.

Our review policy, since it's about to matter to you. When a review finds zero blockers, we apply the small non-behavioural nits ourselves and approve in the same pass, so you don't get a nit → push → re-review round trip. When there is a blocker, we leave your branch completely alone — no commits, no force-push, nothing lands on your fork without you. That's why this review pushed nothing: it found one blocker, so the branch is yours and untouched. (Fork PRs also need a maintainer to approve the CI run, which is why you're seeing no checks yet.)

One thing you should know before spending more time on this, and it is genuinely not your fault: PR #840 is a competing fix for the same issue #832, opened earlier, already through two review rounds, and currently approved and waiting on @s-annam to merge. The production hunk is byte-identical to yours. Nobody flagged #832 as taken and there was no way for you to see that from the issue — that's a gap on our side, not a mistake on yours. The comparison table in the review lays out how the two differ so the decision is transparent rather than arbitrary. Whichever way it goes, please don't read it as a verdict on the work; it's a scheduling collision.

If you'd like to keep contributing, say so and we'll point you at something with no PR already in flight. Questions or pushback on any finding — including finding 1, if you think 8 is defensible — tag @s-annam and argue it. Disagreement is welcome here; a reviewer being wrong is a normal outcome, not an awkward one.

[s-annam]

Same story as #854, Mayank — closing this one too, and again the reason is our scheduling, not your work.

#832 was unclaimed, and @shubhransh-gupta's #840 had already been through two review rounds and is approved and queued to merge. Your skills.ts hunk is byte-identical to his, which is the clearest possible evidence you both read the issue correctly and implemented exactly what it asked.

The two places they differ, both in #840's favour:

• UNFILED_TRUTH_CEILING. You set 8; the measured count on your branch is 7 (confirmed by running check:baselines against it). 8 passes the toBeLessThanOrEqual assertion while leaving one free slot for a future undescribed disagreement, which contradicts the constant's own docblock — the point of the ratchet is that it stays tight. [parser] skills — single-letter language tokens (C, R, D) are dropped by isSkillToken's 2-character floor #832's step 4 says to lower by 2 and to take the lower number when a sibling has already moved it, and a sibling had moved 10 → 9, so the literal 8 in the issue text was stale by the time you read it. That's the issue's fault, not yours.
• Test coverage. [parser] skills — single-letter language tokens (C, R, D) are dropped by isSkillToken's 2-character floor #832 explicitly warns against admitting J, and your suite asserts only the X rejection. fix(skills): allow single-letter languages C, R, and D (#832) #840 tests J, • and ( plus a positive control.

Your fixture work was correct and I want to say so specifically, because it's the part most contributors get wrong here: all five .expected.json re-bakes gain exactly +1 skill and +1 char, and each gained token is genuinely drawn on the page — verified with pdftotext, C on google-docs-skia-proxy, multi-degree-coursework and both deedy variants, R on pdflib-leading-glyph. You did not edit a truth value to match the parser, which is the failure mode that rule exists to catch.

One process note for next time, unrelated to the outcome: no CI ran on this branch at all — fork PRs wait on workflow approval, so nothing external corroborated green. Worth pinging when that happens rather than assuming the checks are just slow.

If you want another, #833 is unassigned and yours if you say so — a Language: Fluent in Spanish proficiency row gets admitted as a skill token. It's in the same predicate neighbourhood as isSkillToken, which you've now read closely, and #840's own work already flagged a fixture for it. Comment on it and I'll assign it before you start.

Tag @s-annam with any pushback. Two collisions in one day is a bad first impression of how we run the backlog and it's on us — I'd like you to have a clean run at the next one.

[s-annam]

And a correction on the handling here, @qtjg — separate from the decision.

I told you this needed one character (UNFILED_TRUTH_CEILING: 8 → 7), then closed it eleven minutes later without waiting for you. That's the wrong order. I should have either let you push the fix, or said upfront that #840 was going to win on test coverage regardless and not framed it as something you could act on. What I actually did was hand you a task and shut the door while you were reading it.

Worth saying plainly too: this PR was one character from mergeable, and #840 — the one I chose — still isn't merged either. The reasoning holds (it was first, and it tests the J and • rejections yours doesn't), but I closed a nearly-finished PR and my comment above didn't own that.

#833 is held for you — there's a note on it asking that nobody else take it. Comment there and I'll assign it properly; GitHub won't let me assign someone who hasn't commented on the issue yet. Drop it freely if it's not what you want.