Skip to content

fix(parser): ignore language proficiency rows - #865

Open
qtjg wants to merge 1 commit into
offlinecv:mainfrom
qtjg:mb/issue-833-language-proficiency
Open

fix(parser): ignore language proficiency rows#865
qtjg wants to merge 1 commit into
offlinecv:mainfrom
qtjg:mb/issue-833-language-proficiency

Conversation

@qtjg

@qtjg qtjg commented Aug 18, 2026

Copy link
Copy Markdown

Resolves #833

Summary

Ignore spoken-language proficiency rows such as Language: Fluent in Spanish when extracting résumé skills, while preserving programming-language rows and bare spoken-language lists. The label and proficiency wording are checked together, so Languages: Python, Go, TypeScript remains unchanged and non-language labels such as Certifications: Certified in AWS Solutions Architecture are 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 passed
  • npm run typecheck — passed
  • npx vitest run src/lib/heuristics/corpus.test.ts src/lib/heuristics/corpus-roundtrip.test.ts — passed
  • npm run lint — passed
  • npm run check:fixtures — passed
  • npm run check:baselines — passed
  • npm run check:core — passed
  • npm run build — passed

The full repository verification also reproduces an unrelated existing BroadcastChannel/MessageEvent failure in src/hooks/useLibraryChanges.test.tsx; 358 test files passed and 1 failed.

@s-annam s-annam left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 German
  • Languages: Elementary proficiency in French
  • Languages: JLPT N2 Japanese Proficiency

That last one is drawn by two committed fixturesgoogle-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.

  1. The isLanguageProficiencyCell(cell) guard in matchCellLabel is unreachable, and the docblock above it no longer describes the guard set. 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 see one. Harmless as defensive alignment with the existing NON_SKILL_SUBLABEL_RE mirror, and worth keeping for that reason, but the docblock at skills.ts:632 explains only the Interests/Hobbies mirror and should name this one too. Related: this call site passes the raw cell while skills.ts:341 passes the already-stripBulleted text; stripBullet is 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:fixtures green (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 at skills.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. The useLibraryChanges.test.tsx BroadcastChannel/MessageEvent failure 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)

Comment thread src/lib/heuristics/extract/skills.ts Outdated
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));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/mainFrameworks:[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.

Comment thread src/lib/heuristics/extract/skills.ts Outdated
* 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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/lib/heuristics/extract/skills.ts Outdated
* 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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 German
  • Languages: Elementary proficiency in French
  • Languages: 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.

Comment thread src/lib/heuristics/extract/skills.ts Outdated
if (
!m ||
NON_SKILL_SUBLABEL_RE.test(m[1]) ||
isLanguageProficiencyCell(cell)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@qtjg

This comment has been minimized.

@s-annam s-annam left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: REQUEST_CHANGES — 3 Blocking findings, each independently reproduced by running the PR's own code against a targeted repro. Rule applied: ≥1 Blocking → REQUEST_CHANGES (no auto-fix, no collapse).

All three share one root cause worth naming up front: isLanguageProficiencyCell decides on a single cell's fragment list, but that decision point interacts badly with the section's other existing mechanics — the soft-wrap join that can fold two source lines into one cell before the check runs, and the else if (categories.length > 0) continuation-append that can't tell a same-row wrap-continuation from a merely-adjacent unrelated line. Fixing all three likely wants one shared fix, not three separate patches.

Blocking

  1. skills.ts:333fragments.every(...) is vacuously true on a single-fragment cell, so a lone skill whose name contains proficiency vocabulary is silently dropped in full, contradicting the docblock's own claim ("applied per fragment so Visual Basic and React Native do not cause an entire programming-language row to disappear" — that guarantee only holds for 2+ items). Repro: tokenizeSkillLine("Languages: Visual Basic")[], and tokenizeSkillLine("Languages: BASIC")[]. A résumé whose sole listed language-category skill is Visual Basic or BASIC loses it with no trace. Suggested fix: gate on fragments.length > 1 && fragments.every(...) at minimum, and add regression coverage for the single-item case either way (undropped, or an explicit single-item heuristic).

  2. skills.ts:352isLanguageProficiencyCell(debulleted) runs on the cell after the pre-existing soft-wrap join (isSoftWrapContinuation Condition B) may already have merged this Language: line with a following, unrelated comma-bearing skill line into one cell. Once merged, the fragment list contains real skill tokens, so fragments.every(...) fails, and the whole blob — including the original proficiency phrase — is NOT dropped and tokenizes normally. Repro (two adjacent lines, a common layout): Languages: Fluent in Spanish, conversational French followed by Project Management, Agile, ScrumextractSkills(...).value = ["Fluent in Spanish", "conversational French Project Management", "Agile", "Scrum"]. This reintroduces the exact #833 defect (proficiency phrase admitted as a skill) plus a garbled cross-line merged token, under a layout shape this PR should cover (a labeled proficiency bullet immediately followed by an unlabeled skills bullet).

  3. skills.ts:697 — when a proficiency row is dropped, its label is retained as an empty-skills category anchor (categories.push({ label, skills: [] })) purely so a genuine wrap-continuation of that same row can reattach later. But the existing append path at skills.ts:707 (else if (categories.length > 0)) can't distinguish a true same-row continuation from a merely-adjacent, unrelated bare cell — so an unrelated bare list gets mislabeled under the dropped row's category. Repro: Languages: Fluent in Spanish (single item, no trailing comma — does not soft-wrap-join with the next line) followed by an unrelated bare Python, Go, TypeScriptcategories = [{"label":"Languages","skills":["Python","Go","TypeScript"]}]. Three programming languages get mislabeled under a spoken-language category header, and — worse — the structured categories view is now emitted where, pre-fix, an uncategorised bare list would have suppressed it entirely (the field would have been absent, not present-but-wrong).

All three verified by direct execution against the PR's actual head commit (febb6c8) in an isolated worktree, not just static reading — none of the three trigger on the PR's own skills.test.ts/corpus.test.ts suites, which is why they survived to review.

Secondary

None beyond the Blocking findings above.

Nits

  • skills.ts:655-659 (matchCellLabel) — the diff reformats if (!m || NON_SKILL_SUBLABEL_RE.test(m[1])) return undefined; onto five lines with no behavior change. Pure churn; consider reverting to the single-line form to keep the diff focused on the actual fix.
  • extractSkills's cyclomatic/cognitive complexity grew with the new nested if (cellTokens.length === 0) { if (...) {...} continue; } block. fallow audit --base origin/main attributes 0 new issues to the changed files (this and isSoftWrapContinuation/splitColumnCells are reported as pre-existing/inherited, non-blocking, report-only per repo convention) — flagging only so it's on record if this file's complexity gets revisited.

AC checklist (issue #833)

  • google-docs-skia-proxy-role-first-experience.pdf — no Fluent in Spanish skill, no Language category. Truth exception removed accordingly.
  • Languages: Python, Go, TypeScript still parses as three skills under Languages.
  • Bare spoken-language list (Spanish, French, Mandarin, no proficiency wording) still admitted — matches the issue's own documented limit.
  • Certifications: Certified in AWS Solutions Architecture unaffected.
  • multi-degree-coursework.pdf's programming-language Languages row unchanged (not touched by the diff).
  • corpus.test.ts passes (65 tests, 1 pre-existing skip).
  • corpus-roundtrip.test.ts passes, no new KNOWN_FAILURES rows (corpus-edit-roundtrip.known-failures.json untouched by the diff).
  • [~] npm run verify — not run as the single full command this session, but its components were run directly and all passed: typecheck, lint, check:fixtures, fallow audit, plus the full skills.test.ts (49 tests) / corpus.test.ts / corpus-roundtrip.test.ts suites. The PR body's own verification log claims a full green verify plus a disclosed, unrelated useLibraryChanges.test.tsx flake; that file passed in isolation here too, consistent with a full-suite timing flake rather than a regression.

The AC list is satisfied on its own terms — the Blocking findings above are regressions the ACs didn't anticipate (single-item rows, cross-line soft-wrap interaction, category mislabeling), not AC gaps.

Description accuracy (3f)

Accurate as far as it goes — every claim in the Summary round-trips to the code for the cases it describes, and the disclosed useLibraryChanges.test.tsx failure is handled honestly (named as unrelated, and verified so here). It doesn't claim immunity from the three bugs above, but the docblock on LANGUAGE_PROFICIENCY_BODY_RE (skills.ts:315-319) does implicitly overclaim per-fragment safety for the single-item case (Finding 1).

Gates run

  • /code-review (effort: high, background fork) — 3 findings, all reproduced and included above as Blocking.
  • 3a fixture PII — npm run check:fixtures passed (60 PDFs, 16 sidecars); the modified .truth.json sidecar is a knownWrong removal, not new PII surface.
  • 3b design-system/reuse — n/a, no src/components/** touched.
  • 3c style tokens — n/a, no src/** styling touched.
  • 3d fallow — npx fallow audit --base origin/main: 0 issues attributed to the 7 changed files; 3 pre-existing complexity findings excluded as inherited. Report-only per repo convention regardless.
  • 3e skill/script command bugs — n/a, no .claude/skills/** or scripts/** touched.
  • 3f description accuracy — accurate, see above.
  • npm run typecheck, npm run lint — both clean.

Head reviewed: febb6c8. 👀 posted at review start.


Reviewed by: Claude Sonnet 5 (high)

.filter((fragment) => fragment !== "");
return (
fragments.length > 0 &&
fragments.every((fragment) => LANGUAGE_PROFICIENCY_BODY_RE.test(fragment))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking. fragments.every(...) is vacuously true when there's only one fragment, so a lone skill whose name contains proficiency vocabulary (e.g. Visual Basic, BASIC) is silently dropped in full — not just the proficiency word, the whole skill. This contradicts the docblock two lines up, which claims the per-fragment check specifically prevents Visual Basic/React Native from disappearing; that only holds when the row has 2+ items.

Repro: tokenizeSkillLine("Languages: Visual Basic")[] (verified against febb6c8).

Suggest gating on fragments.length > 1 && fragments.every(...), with explicit test coverage either way for the single-item case.

if (labelMatch && NON_SKILL_SUBLABEL_RE.test(labelMatch[1])) return;
if (
labelMatch &&
(NON_SKILL_SUBLABEL_RE.test(labelMatch[1]) || isLanguageProficiencyCell(debulleted))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking. This runs on debulleted, which may already be the product of the pre-existing soft-wrap join (isSoftWrapContinuation Condition B) merging this Language: line with a following, unrelated comma-bearing skill line into one cell. Once merged, the fragment list contains real skill tokens, so isLanguageProficiencyCell returns false and the entire merged blob — including the original proficiency phrase — is tokenized as skills instead of being dropped.

Repro (two adjacent lines): Languages: Fluent in Spanish, conversational French then Project Management, Agile, ScrumextractSkills(...).value = ["Fluent in Spanish", "conversational French Project Management", "Agile", "Scrum"]. This reintroduces the exact #833 defect plus a garbled cross-line token, under a plausible real layout (a labeled proficiency bullet immediately followed by an unlabeled skills bullet).

// a token.
if (cellTokens.length === 0) {
if (label !== undefined && isLanguageProficiencyCell(cell)) {
categories.push({ label, skills: [] });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking. This empty-category placeholder exists so a genuine wrap-continuation of this same dropped row can reattach later (see the docblock addition on matchCellLabel). But the append path a few lines below (else if (categories.length > 0), skills.ts:707) can't tell a true same-row continuation from a merely-adjacent, unrelated bare cell — it appends either one.

Repro: Languages: Fluent in Spanish (single item, no trailing comma, so it does NOT soft-wrap-join with the next line) followed by an unrelated bare Python, Go, TypeScriptcategories = [{"label":"Languages","skills":["Python","Go","TypeScript"]}]. Three programming languages get mislabeled under a spoken-language header, and the structured categories view is now emitted where, pre-fix, an uncategorised bare list would have suppressed it entirely.

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.

[parser] skills — a "Language: Fluent in Spanish" proficiency row is admitted as a skill token

2 participants