fix(parser): ignore language proficiency rows - #865
Conversation
Resolves offlinecv#833 Ignore spoken-language proficiency rows such as `Language: Fluent in Spanish` while preserving programming-language and bare spoken-language lists. The change includes regression coverage and updates the affected corpus snapshots. Verified locally with focused parser tests, corpus and round-trip tests, typecheck, lint, fixture checks, baseline checks, core-package checks, and production build. The full suite also reproduces an unrelated existing BroadcastChannel/MessageEvent failure in src/hooks/useLibraryChanges.test.tsx.
s-annam
left a comment
There was a problem hiding this comment.
The precision goal is right and the corpus moves the right way — Fluent in Spanish is gone from both fixtures and the #833 truth exception clears cleanly against an exactly-matching 11-entry truth. But the discriminator is a bare substring test over the whole body, and it takes the whole cell with it. Two confirmed regressions follow, and neither is visible to verify because no fixture happens to pair the shapes.
Verdict rule: ≥1 Blocking → REQUEST_CHANGES. 2 Blocking, 2 Secondary, 1 Nit. Nothing was committed or pushed.
Every claim below was measured on this branch against origin/main with throwaway probes over the real extractSkills / tokenizeSkillLine (probe files removed; tree left clean).
Blocking
1. LANGUAGE_PROFICIENCY_BODY_RE matches inside real programming-language names, and the drop is whole-cell — an entire skills row vanishes
\bbasic\b fires on Visual Basic, \bnative\b on React Native, and \bproficient\b on the ubiquitous Proficient in <list> phrasing. Because the drop happens in tokenizeCell before the split, every sibling token on the row dies with the offender:
| Input | origin/main |
this branch |
|---|---|---|
Languages: C, C++, Visual Basic |
["C","C++","Visual Basic"] |
[] |
Languages: Kotlin, Swift, React Native |
3 tokens | [] |
Languages: Proficient in Java, Python, Go |
["Proficient in Java","Python","Go"] |
[] |
Languages: Java, Python (proficient), Go |
["Java","Python (proficient)","Go"] |
[] |
The label conjunct is Languages in all four, so the label is effectively deciding — the exact failure #833's root-cause section ruled out: "denying the label would delete the single most valuable row on the page." This is a strictly larger loss than the one token the fix targets.
Not hypothetical on this corpus. openresume-laverne-word-quartz.pdf — the fixture this PR re-snapshots — draws Computer: Proficient in Windows and Mac OS, Microsoft Word, PowerPoint, and Excel, spared only by its label. multi-degree-coursework.pdf draws Languages: Python, Go, C, C++, Java, JavaScript, Swift, PHP, HTML, CSS — one Visual Basic away from returning nothing.
Why this reads as under-implementing the plan, not a judgement call: #833 step 1 specifies the body conjunct as "whose body reads as a proficiency predication rather than a delimited list". Only the proficiency-word half shipped; the delimited-list half is missing. The regexes in the issue illustrate that two-part rule rather than replacing it.
One shape that satisfies every AC — drop only when every delimited fragment is a proficiency predication:
const fragments = debulleted
.slice(match[0].length)
.split(SKILL_SPLIT_RE)
.map((f) => f.trim())
.filter((f) => f !== "");
// A list whose fragments are mostly plain tokens is a programming-language row
// that happens to contain "Basic"/"React Native"/"(proficient)"; only an
// all-proficiency body is a spoken-language proficiency statement.
return fragments.length > 0 && fragments.every((f) => LANGUAGE_PROFICIENCY_BODY_RE.test(f));Against the ACs: Fluent in Spanish 1/1 → dropped. Python, Go, TypeScript 0/3 → kept. Spanish, French, Mandarin 0/3 → kept (the stated limit, preserved). C, C++, Visual Basic 1/3 → kept. Proficient in Java, Python, Go 1/3 → kept. Note splitRespectingParens is what handles commas — reuse the existing splitter rather than a new one.
Whatever shape you land on, please pin the four rows above as unit assertions. The new test at skills.test.ts:838 covers only clean rows, so nothing currently guards this.
2. Dropping the cell destroys the category anchor, so a soft-wrapped proficiency row misfiles its tail into the previous category
Three lines — Frameworks: React, Vue / Languages: English (native), Spanish (fluent), / French:
value |
categories |
|
|---|---|---|
origin/main |
["React","Vue","English (native)","Spanish (fluent)","French"] |
Frameworks:[React,Vue], Languages:[English (native),Spanish (fluent),French] |
| this branch | ["React","Vue","French"] |
Frameworks:[React,Vue,French] |
French is now a Framework. The mechanism: the language cell yields zero tokens, so extractSkills continues at skills.ts:680 before matchCellLabel ever runs, no Languages category is opened, and the wrapped continuation falls into the "bare cell extends the last category" branch at skills.ts:691. Any wrap isSoftWrapContinuation declines reproduces it.
This is worse than the loss in #1 — it is silently wrong data rather than missing data, and skills feeds the JD-match and job-search keyword surfaces the issue itself calls load-bearing.
Fixing #1 does not fix this. Under the all-fragments rule above, English (native), Spanish (fluent), is still legitimately dropped, so the leak survives. It needs its own answer — either register the category label before the token-count continue, or drop the offending fragments rather than the cell — which is also the cleanest way to make #1 and #2 fall out of one change.
Secondary
3. LANGUAGE_LABEL_RE is stricter than its sibling, so the targeted defect survives on common label variants
/^languages?$/i tested against the untrimmed capture. Measured, all still admitted on this branch:
Foreign Languages: Fluent in Spanish→["Fluent in Spanish"]Spoken Languages: Native German, conversational French→["Native German","conversational French"]Languages : Fluent in Spanish(space before colon) →["Fluent in Spanish"]
The first two are #833's defect, unfixed. The third is a capture artifact: SUBLABEL_BODY ([A-Z][A-Za-z &/]+) admits a space, so the capture is "Languages ". NON_SKILL_SUBLABEL_RE carries a deliberate \s*$ for exactly this and matchCellLabel .trim()s — this regex does neither, so Interests : Tennis is correctly dropped while Languages : Fluent in Spanish escapes. /^(?:foreign\s+|spoken\s+|other\s+)?languages?\s*$/i, mirroring the sibling's leading-qualifier tolerance, covers all three.
4. The proficiency vocabulary misses the standard scale wordings — including one already in the corpus
\bproficient\b does not match the noun proficiency, and only working\s+proficiency is enumerated. Still admitted:
Languages: Full professional proficiency in GermanLanguages: Elementary proficiency in FrenchLanguages: JLPT N2 Japanese Proficiency
That last one is drawn by two committed fixtures — google-docs/google-docs-skia-proxy-multiline-bullets-coursework.pdf and unknown/student-projects-activities-singlecol.pdf (verified with pdftotext) — and both baselines are untouched by this PR, confirming the row still lands in skills. proficienc(y|ies), plus elementary / limited, closes it. Worth doing here rather than as a follow-up, since the fixtures are already in the corpus.
Nits
Non-blocking; neither changes the verdict.
- The
isLanguageProficiencyCell(cell)guard inmatchCellLabelis unreachable, and the docblock above it no longer describes the guard set.extractSkills:680continues on a zero-token cell andtokenizeCellalready returned early for exactly these, somatchCellLabel— which has no other callers — can never see one. Harmless as defensive alignment with the existingNON_SKILL_SUBLABEL_REmirror, and worth keeping for that reason, but the docblock atskills.ts:632explains only the Interests/Hobbies mirror and should name this one too. Related: this call site passes the rawcellwhileskills.ts:341passes the already-stripBulleted text;stripBulletis idempotent so it is correct either way, but taking the captured label and body as parameters would make the "two decisions stay aligned" contract structural instead of by-convention.
Acceptance criteria — #833
| AC | Result |
|---|---|
google-docs-skia-proxy-role-first-experience.pdf — no Fluent in Spanish, no Language category |
✅ skillsCount 12 → 11, matching the 11-entry truth skills exactly |
Languages: Python, Go, TypeScript → 3 skills under Languages. Unit test |
✅ covered |
Languages: Spanish, French, Mandarin still admitted (stated limit) |
✅ covered |
Certified in AWS Solutions Architecture under a non-language label unaffected |
✅ covered |
multi-degree-coursework.pdf Languages row unchanged |
✅ expected.json untouched; that row carries no proficiency word |
corpus.test.ts passes; precision up on target, down on none |
✅ on the corpus — but see Blocking 1/2, which the corpus does not exercise |
corpus-roundtrip.test.ts passes, no new KNOWN_FAILURES rows |
✅ no baseline file in the diff |
npm run verify passes |
✅ reproduced locally, green through build |
Plan step 3 also holds: no languages field exists in docs/canonical-resume-model.md or the parsed types, so dropping is the right answer and none was added. Step 5 holds too — UNFILED_TRUTH_CEILING is untouched at 7, correct since the cleared entry was status: "open", never unfiled.
Description accuracy (gate 3f)
## Summary claims the change preserves programming-language rows. Blocking 1 shows it does not, for Visual Basic / React Native / Proficient in … / X (basic). Same defect, so not counted twice — but once the predicate is narrowed, the summary needs a line naming which language rows are still dropped, so the limit is stated rather than discovered.
The ## Verification list is otherwise accurate and reproduced here.
Gates
- 3a fixture PII — no binary added or changed; only
.expected.json/.truth.json.npm run check:fixturesgreen (it sweeps truth sidecars since #654). - 3b design system / 3c style tokens — n/a, nothing under
src/components/. - 3d fallow — 2 complexity findings (
isSoftWrapContinuation,extractSkills) + 1 clone group atskills.test.ts:589-664, all pre-existing and outside this diff. Report-only. - 3e skill/script command bugs — n/a, no bash/
gh/script files in the diff. npm run verify— green end to end here, full suite included. TheuseLibraryChanges.test.tsxBroadcastChannel/MessageEventfailure noted in the PR body did not reproduce; it looks environment-dependent, and nothing in this diff touches it.
Branch hygiene
The branch carries 4 commits (1b083e3, 616c09c, b20c622, 8eaae56). main merges through a merge queue that derives the squash message from the branch, so as-is this lands test(fixtures): refresh Word skill snapshot in main permanently. Collapse to one before enqueueing — /collapse-pr does it. Not counted as a finding, and this review pushed nothing (≥1 Blocking).
Reviewed by: Claude Opus 5 (high)
| const debulleted = stripBullet(cell); | ||
| const match = debulleted.match(SUBLABEL_PREFIX_RE); | ||
| if (!match || !LANGUAGE_LABEL_RE.test(match[1])) return false; | ||
| return LANGUAGE_PROFICIENCY_BODY_RE.test(debulleted.slice(match[0].length)); |
There was a problem hiding this comment.
Blocking. This tests the proficiency vocabulary against the whole body, so a delimited programming-language row containing one matching word is dropped entirely — and tokenizeCell drops the cell before the split, so every sibling token on the row dies too.
Measured on this branch vs origin/main:
| Input | origin/main |
this branch |
|---|---|---|
Languages: C, C++, Visual Basic |
["C","C++","Visual Basic"] |
[] |
Languages: Kotlin, Swift, React Native |
3 tokens | [] |
Languages: Proficient in Java, Python, Go |
["Proficient in Java","Python","Go"] |
[] |
Languages: Java, Python (proficient), Go |
["Java","Python (proficient)","Go"] |
[] |
\bbasic\b catches Visual Basic, \bnative\b catches React Native, \bproficient\b catches the common Proficient in <list> phrasing. The label is effectively deciding, which is what #833 ruled out.
#833 step 1 specifies the body conjunct as "a proficiency predication rather than a delimited list" — the delimited-list half is missing. Requiring every delimited fragment to be a proficiency predication satisfies all four ACs and rejects all four rows above:
const fragments = debulleted
.slice(match[0].length)
.split(SKILL_SPLIT_RE)
.map((f) => f.trim())
.filter((f) => f !== "");
return fragments.length > 0 && fragments.every((f) => LANGUAGE_PROFICIENCY_BODY_RE.test(f));(splitRespectingParens is what handles commas — worth reusing the existing splitter.) Please pin the four rows as unit assertions; the new test covers only clean rows.
| if (labelMatch && NON_SKILL_SUBLABEL_RE.test(labelMatch[1])) return; | ||
| if ( | ||
| labelMatch && | ||
| (NON_SKILL_SUBLABEL_RE.test(labelMatch[1]) || isLanguageProficiencyCell(debulleted)) |
There was a problem hiding this comment.
Blocking. Returning here yields zero tokens, so extractSkills continues at skills.ts:680 before matchCellLabel runs and no Languages category is ever opened. A soft-wrapped continuation then falls into the "bare cell extends the last category" branch at skills.ts:691 and is filed under the previous label.
Frameworks: React, Vue / Languages: English (native), Spanish (fluent), / French:
origin/main→Frameworks:[React,Vue],Languages:[English (native),Spanish (fluent),French]- this branch →
Frameworks:[React,Vue,French]
French becomes a Framework — silently wrong data rather than missing data, on a surface that feeds JD-match and job-search keywords.
Note the all-fragments fix for the other blocker does not resolve this: English (native), Spanish (fluent), is still legitimately dropped. Either register the category label before the zero-token continue, or drop the offending fragments rather than the whole cell — the latter makes both blockers fall out of one change.
| * added to NON_SKILL_SUBLABEL_RE: on most engineering résumés `Languages:` | ||
| * heads the programming-language row, so the label is ambiguous and the body | ||
| * must decide whether this is a proficiency statement. */ | ||
| const LANGUAGE_LABEL_RE = /^languages?$/i; |
There was a problem hiding this comment.
Secondary. Stricter than its sibling NON_SKILL_SUBLABEL_RE, so #833's defect survives on common variants. All still admitted on this branch:
Foreign Languages: Fluent in Spanish→["Fluent in Spanish"]Spoken Languages: Native German, conversational French→["Native German","conversational French"]Languages : Fluent in Spanish→["Fluent in Spanish"]
The third is a capture artifact: SUBLABEL_BODY ([A-Z][A-Za-z &/]+) admits a space, so the capture is "Languages ". NON_SKILL_SUBLABEL_RE carries a deliberate \s*$ for exactly this and matchCellLabel .trim()s the capture — this regex does neither, so Interests : Tennis is dropped while Languages : Fluent in Spanish escapes.
/^(?:foreign\s+|spoken\s+|other\s+)?languages?\s*$/i mirrors the sibling's leading-qualifier tolerance and covers all three.
| * programming-language list by its proficiency wording rather than by the | ||
| * label. */ | ||
| const LANGUAGE_PROFICIENCY_BODY_RE = | ||
| /\b(fluent|native|bilingual|conversational|proficient|intermediate|beginner|basic|working\s+proficiency|mother\s+tongue)\b/i; |
There was a problem hiding this comment.
Secondary. The vocabulary misses the standard scale wordings: \bproficient\b does not match the noun proficiency, and only working\s+proficiency is enumerated. Still admitted:
Languages: Full professional proficiency in GermanLanguages: Elementary proficiency in FrenchLanguages: JLPT N2 Japanese Proficiency
The last is drawn by two committed fixtures — google-docs/google-docs-skia-proxy-multiline-bullets-coursework.pdf and unknown/student-projects-activities-singlecol.pdf (verified with pdftotext) — and both baselines are untouched by this PR, confirming the row still reaches skills.
Adding proficienc(y|ies) plus elementary / limited closes it. Worth doing here rather than as a follow-up, since the fixtures already exist.
| if ( | ||
| !m || | ||
| NON_SKILL_SUBLABEL_RE.test(m[1]) || | ||
| isLanguageProficiencyCell(cell) |
There was a problem hiding this comment.
Nit. Unreachable: extractSkills:680 continues on a zero-token cell and tokenizeCell already returned early for exactly these, so matchCellLabel — which has no other callers — can never be handed one. Fine to keep as defensive alignment with the NON_SKILL_SUBLABEL_RE mirror, but the docblock just above (skills.ts:632) explains only the Interests/Hobbies mirror and should name this guard too.
Related: this passes the raw cell while skills.ts:341 passes the already-stripBulleted text. Correct either way since stripBullet is idempotent, but taking the captured label and body as parameters would make the "two decisions stay aligned" contract structural rather than by-convention.
Resolves #833
Summary
Ignore spoken-language proficiency rows such as
Language: Fluent in Spanishwhen extracting résumé skills, while preserving programming-language rows and bare spoken-language lists. The label and proficiency wording are checked together, soLanguages: Python, Go, TypeScriptremains unchanged and non-language labels such asCertifications: Certified in AWS Solutions Architectureare unaffected.The change adds focused regression coverage, removes the resolved #833 truth exception, and updates the affected Google Docs and Word corpus snapshots.
Verification
npx vitest run src/lib/heuristics/extract/skills.test.ts— 49 passednpm run typecheck— passednpx vitest run src/lib/heuristics/corpus.test.ts src/lib/heuristics/corpus-roundtrip.test.ts— passednpm run lint— passednpm run check:fixtures— passednpm run check:baselines— passednpm run check:core— passednpm run build— passedThe full repository verification also reproduces an unrelated existing
BroadcastChannel/MessageEventfailure insrc/hooks/useLibraryChanges.test.tsx; 358 test files passed and 1 failed.