refactor(heuristics): one date-token lexicon in regex.ts (#916) - #926
Conversation
Deploying offlinecv with
|
| Latest commit: |
ba1f223
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ee94b329.offlinecv.pages.dev |
| Branch Preview URL: | https://refactor-916-shared-date-lex.offlinecv.pages.dev |
`regex.ts` held the canonical month/season tokens as module-private consts,
so every module needing them re-hardcoded its own copy. The copies had
already drifted — some spelled September `sept?`, others `sep|sept` — which
is the failure `regex.ts:154-166` documents as education's `DATE_LEAD_RE`
loose-month false-reject.
Export the vocabularies and derive every call site from them.
The issue's table names 7 copies. The same three vocabularies were in fact
re-hardcoded in 18 places across 5 files, including three inside `regex.ts`
itself (`PRESENT_RE` and two inline in `DATE_RANGE_RE`) and two in
`line-primitives.ts`, which its table missed. Consolidating only the listed 7
would have left a month literal one line above a derived season literal, so
all 18 are gone; the extras are called out in the PR body.
Each vocabulary is exported twice, and the pair is load-bearing:
- `MONTH` / `SEASON` / `OPEN_ENDED` — wrapped non-capturing, what almost
every site wants.
- `MONTH_ALT` / `SEASON_ALT` / `OPEN_ENDED_ALT` — the bare alternation, for
the two jobs the wrapped form cannot do. `education.ts` folds months,
seasons and `present` into ONE group under a single shared `[a-z]*`, where
a pre-wrapped token would nest a second tail and change the language; and
`DATE_RANGE_RE` splices the open-ended words into a CAPTURING group whose
numbering callers index by position, so the token must bring no group of
its own.
`STRICT_MONTH` is deliberately NOT exported, departing from the issue's
acceptance criteria. Every use is inside `regex.ts`, so exporting it adds an
importer-less symbol — `fallow` flags it as an unused export, and correctly.
The change that needs it outside this module is #925; it should be exported
there, where it gains a real consumer.
Behaviour-preserving, and checked as such rather than assumed. A temporary
harness captured every affected compiled pattern's `.source` plus the output
of the six exported and private predicates that consume them, over 3,631
generated date-shaped inputs, before and after: 25,417 assertions, zero
differences. The harness was removed before this commit.
The only pattern-source changes are three equivalence classes:
- Case (`jan` → `Jan`). Every one of the 18 sites compiles with `i` or `gi`.
- `sept?` → `Sep|Sept`. Identical under the shared `[a-z]*` tail: both
reduce to `sep[a-z]*`.
- `Ongoing|Now` → `Now|Ongoing`. An alternation reorder of `\b`-anchored
words with no common prefix.
Corpus snapshots and JSON baselines are untouched — no rebaseline.
639d0e1 to
ba1f223
Compare
s-annam
left a comment
There was a problem hiding this comment.
The behaviour-preservation claim holds under an independent differential, not just under the corpus. I rebuilt all 15 affected pattern substitutions from both the pre- and post-refactor sources and compared them as languages over 19,248 generated candidates (all twelve months in abbreviated/full/trailing-period form, near-misses Marketing/Mayor/Septa/Presentation/Ongoings/Nowhere, seasons, real + 20XX + apostrophe years, every separator, and full ranges): zero divergences, including on the four whose .source is genuinely not case-identical — sept?→Sep|Sept and the Ongoing|Now→Now|Ongoing reorder. The three equivalence classes in the body are the complete set, and each one is sound. Tree was clean after a full run, so no snapshot moved.
Verdict rule applied: 0 Blocking → APPROVE. One Secondary and three Nits below, none of them merge-blocking.
Acceptance criteria (#916)
| AC | Result |
|---|---|
MONTH, SEASON, STRICT_MONTH + open-ended exported from regex.ts |
partially met, justified — see below |
| All 7 re-hardcoded copies gone | met, and 11 more besides — 18 total, count verified against the diff |
| Corpus snapshots byte-identical | met — git status clean after the full suite (383 files / 6450 tests) |
npm run verify green |
met — typecheck, lint, check:nul, check:fixtures, check:baselines, check:core, full suite, vite build, fallow audit all green here |
| No new import cycle | met — regex.ts still imports only sections.config.ts; all four consumers added names to an import block they already had |
On STRICT_MONTH: I verified the justification rather than taking it. Adding export to it and re-running the gate produces exactly what the body predicts —
● Unused exports (1)
:177 STRICT_MONTH
✗ dead code: 1 issue · 5 changed files
— attributed to this diff. So the AC as written cannot be satisfied without manufacturing the finding gate 3d exists to catch, and #925 is a real open issue that gives it a consumer. That is a disclosed and correct deviation, not a gap, so it does not block. (Nit 3 covers the bookkeeping.)
Scope question, answered
Yes to all 18 — don't split it. Leaving a hardcoded month literal one row above a derived season literal is a worse state than either endpoint, and the seven-site boundary was a description of the drift, not a design.
## Review focus, answered
- Is the
_ALT/ wrapped pair worth two names per vocabulary? Yes. 11 of the 18 sites take the wrapped form; collapsing to bare-only pushes(?:…)into all 11 and invites exactly the nesting bug the pair prevents. Keep both. - Does the
isInlineDatedProgramcomment hold the shared-tail constraint? The mechanism is clear. What it doesn't say is the consequence — that${MONTH}there yields(?:…)[a-z]*[a-z]*, which is not merely redundant but changes what the group can consume. One clause naming that would make the comment self-enforcing. - The riskiest hunk isn't in the focus list. It's
stripInstitutionDate(education.ts:884-905): the function-localSEASONandOPENdeletions mean two module-level constants now resolve where lexically-scoped ones used to, and the same identifierSEASONappears in three composed sub-patterns (REDACTED,DATE, and the(?:${SEASON}\s+)?\b\d{4}\btail). It's correct — I diffed all three — but it's the hunk where a shadowing mistake would have been silent, and it's the one a reader should be pointed at.
Secondary
1. A 19th copy survives in a file this PR touched — line-primitives.ts:454.
if (/^(present|current|now|ongoing)$/i.test(endRaw)) {parseDateRange re-lists the open-ended vocabulary verbatim, 226 lines above the stripDateRange copy this PR did consolidate, in a module that now imports OPEN_ENDED_ALT. The body enumerates "2 in line-primitives.ts" and says "all 18 are gone"; within the five files it audited there were 19, and this one is still a literal.
It matters more than the count: parseDateRange is the function that decides is_current, so this is the copy whose drift would silently change a parsed résumé rather than a strip. The replacement is language-identical (.test() only, so the capture group is unused):
if (new RegExp(`^(?:${OPEN_ENDED_ALT})$`, "i").test(endRaw)) {Not auto-fixed and not offered as a one-click suggestion: it is a regex edit inside a parse path, which is over the line for a fix that merges unread, and line 454 is not a + line so it has no anchor.
Nits (non-blocking)
2. In-function RegExp construction where both modules build every other pattern at module scope. Inline on entry-blocks.ts:72-73 and line-primitives.ts:683-685.
Framing it honestly, because the obvious reading is wrong: this is not a performance finding. I measured it (in-function vs hoisted, 400k calls, two runs) at roughly a 4× ratio on the construction itself but only ~0.7µs per call in absolute terms — about 40µs across a whole résumé, against a parse that is ~82% pdfjs extraction. The machine was loaded (1-min average 13.6 on 10 cores) so treat the ratio, not the absolute, as the signal, and neither is a reason to change anything.
The reason to hoist is consistency: SEASON_LEAD_RE two hunks up in the same file is module-scope, as is everything in regex.ts, and isDateOnlyLine is called per line. Worth noting there's a real reason this isn't a mechanical hoist — a g-flagged regex at module scope carries shared lastIndex, which is safe under String.replace (it resets) but is the exact statefulness stripDateRange guards three lines earlier with DATE_RANGE_RE.lastIndex = 0. So it's a deliberate call, not a cleanup.
3. #916's first AC checkbox will close untickable, recorded only here. When this merges, Closes #916 closes an issue whose AC 1 names STRICT_MONTH. The reasoning is sound and prominently disclosed — but it lives in a PR body, and the next person reads the issue. A one-line comment on #916 saying AC 1 is deliberately partial and why, cross-linking #925, puts it where it will be found.
4. src/lib/edit/field-validators.ts:42 is a sixth file with the same copy — and this PR is what unblocks it.
// Open-ended end-date words. Mirrors PRESENT_RE's alternation; inlined (rather
// than reusing that `\b`-bounded RegExp) so it composes cleanly inside the
// fully-anchored single-field pattern below.
const PRESENT_WORDS = "Present|Current|Now|Ongoing";That comment's rationale is now obsolete: OPEN_ENDED_ALT is the bare, unbounded alternation it says it needed, and the file already imports from ../heuristics/regex.ts. Out of scope for #916 (which scoped itself to heuristics/), so a follow-up rather than a change here — but it's the cleanest demonstration that the _ALT export was the right shape. (score.ts:191 holds a month copy too; that one is correctly left alone, since score.ts is deliberately near-zero-dep.)
Gates
Ran: full suite (383 files / 6450 tests, 0 failures, tree clean afterwards), typecheck, lint, check:nul, check:fixtures (61 PDFs + 16 sidecars — no fixture touched, ran anyway), check:baselines (7 pre-existing unfiled entries, none from this diff), check:core, vite build, fallow audit --base origin/main (exit 0; ✓ No issues in 5 changed files; the 15 complexity findings are inherited and gate-excluded — the body's claim is accurate). One commit, no attribution trailers.
Skipped as inapplicable: fixture-PII binary inspection (no binaries added), design-system / token gates (no component or style changes), skill-and-script command review (no scripts/** or SKILL.md in the diff).
Description accuracy: accurate, with one undercount. Every checkable claim round-tripped — the 18-site count matches the diff file by file, 383 files / 6450 tests matches my run exactly, the fallow exit-0 and dead-code-0 claims are true, regex.ts:142-162 and regex.ts:347 both point where they say, #925's title matches the defect described, and the three equivalence classes are complete and correct. The one thing it overstates is exhaustiveness (Secondary 1). Disclosing the AC deviation in its own section, and documenting the deliberately-preserved isInlineDatedProgram bug rather than quietly fixing it inside a refactor, is the right instinct on both counts.
Nothing pushed: every finding is either a regex edit in a parse path, a file outside this diff, or an issue comment — none of which belong in a fix commit that merges unread. The branch is a named contributor's, so it would not have been collapsed either; it is already at one commit, so the invariant holds as-is.
Reviewed by: Claude Opus 5 (high)
| .replace(new RegExp(String.raw`\b${MONTH}\.?`, "gi"), "") | ||
| .replace(new RegExp(String.raw`\b${SEASON}\b`, "gi"), "") |
There was a problem hiding this comment.
Nit (non-blocking) — these two are now built on every call, where the literals they replaced were too, but every other pattern in this module and in regex.ts is built once at module scope. isDateOnlyLine runs per line via education chunking and isEntryHeaderShape.
Not a perf finding: I measured ~0.7µs per call (≈40µs across a résumé, against a parse that is ~82% pdfjs extraction), on a loaded machine, so the ratio is the only trustworthy part and neither number justifies a change. The reason is consistency.
And it is not a mechanical hoist, which is why I am leaving it to you rather than suggesting it: a gi regex at module scope carries shared lastIndex. That is safe under String.replace (which resets it), but it is the exact statefulness this codebase guards explicitly elsewhere — stripDateRange resets DATE_RANGE_RE.lastIndex by hand two lines from its own copy of this pattern. Fine to leave as-is.
|
Follow-up bookkeeping, after the fact — this merged with the Secondary on #931 also carries the |
Summary
regex.tsheld the canonical month/season tokens as module-private consts, so every module needing them re-hardcoded its own copy — and the copies had already drifted, some spelling Septembersept?and otherssep|sept. That drift is the failureregex.ts:154-166documents as education'sDATE_LEAD_REloose-month false-reject.This exports the vocabularies and derives every call site from them.
Closes #916
Scope is larger than the issue's table — please read this bit
The issue names 7 copies. The same three vocabularies were in fact re-hardcoded in 18 places across 5 files, including ones its table missed:
regex.tsitself —PRESENT_RE, plus two inline inDATE_RANGE_RE.line-primitives.ts—SEASON_LEAD_REand the open-ended strip instripDateRange. (Safe to touch:line-primitivesalready importsregex.ts; the cycle guard documented atregex.ts:347runs the other way.)education.ts—inferDatePrecision's month literal,ATTENDANCE_RANGE_END's open-ended list, and the function-localOPENinstripInstitutionDate.sections.ts, and the season line one row below the listed month line inentry-blocks.ts.Consolidating only the listed 7 would have left a hardcoded month literal one line above a derived season literal, so all 18 are gone. Happy to split this if you'd rather keep the PR to the issue's exact list — say so and I'll cut it back.
Each vocabulary is exported twice, and the pair is load-bearing
MONTH/SEASON/OPEN_ENDED— wrapped non-capturing. What almost every site wants.MONTH_ALT/SEASON_ALT/OPEN_ENDED_ALT— the bare alternation, for the two jobs the wrapped form cannot do:isInlineDatedProgramfolds months, seasons andpresentinto ONE group under a single trailing[a-z]*. A pre-wrappedMONTHwould nest a second tail and change the matched language.DATE_RANGE_REinterpolates the open-ended words into(...)whose group numbering callers index by position, so the token must bring no group of its own.One deviation from the acceptance criteria
The AC says
MONTH,SEASON,STRICT_MONTHand the open-ended tokens are exported.STRICT_MONTHis not exported here, deliberately.Every use of it is inside
regex.tsitself, so exporting it adds a symbol with no importer —fallowflags it as an unused export, and it's right to. The change that actually needs it outside this module is #925 (swapping education's loose[a-z]*month strip for this enumerated form); it should be exported by that PR, where it gains a real consumer. A comment at its declaration records this.Every other clause of the AC is met, and the export is one line away if you'd rather take it now.
Behaviour-preserving — checked, not assumed
The AC asks for byte-identical corpus snapshots, but a snapshot only proves the corpus doesn't happen to exercise a difference. So equivalence was pinned directly.
A temporary harness captured every affected compiled pattern's
.sourceand.flags, plus the output of the six exported and private predicates that consume them (STRONG_DATE_TOKEN_RE,DATE_LEAD_RE,CLEAN_FIELD_DATE_RE,isInlineDatedProgram,stripInstitutionDate,isDateOnlyLine,isEntryHeaderShape), over 3,631 generated date-shaped inputs — all twelve months in abbreviated / full / trailing-period forms, seasons, real and redacted (20XX) years, apostrophe years, every separator, open-ended tails, and deliberate near-miss prose (Marathon,Mayor,Presentation,Augment).25,417 assertions, before vs after: zero differences. The harness was removed before the commit.
The only pattern-source changes are three equivalence classes:
jan→Janiorgisept?→Sep|Sept[a-z]*tail — both reduce tosep[a-z]*Ongoing|Now→Now|Ongoing\b-anchored words with no common prefixDeliberately left alone
entry-blocks.ts'spresent\|current\|expected\|graduation\|graduated\|anticipated. A different vocabulary — graduation-context words — sharing only two members with the open-ended end-date one. Not a copy, so not consolidated.[a-z]*over-strip inisInlineDatedProgram. Filed separately as parser: isInlineDatedProgram's loose [a-z]* month tail eats ordinary program words — "Marketing 2020" is rejected as a program #925: the loose tail meansMarketing 2020is rejected as a program (mareatsMarketing), which is the same hazard [edit-interaction] display polish nits — empty sections still render "+Add"; project uses middot between month/year; awards separator differs from source #380 already fixed forATTENDANCE_RANGE_ENDone screen above. This PR preserves that bug exactly, on purpose — fixing it here would have hidden a real behaviour change inside a refactor.Review focus
src/lib/heuristics/regex.ts:142-162— is the_ALT/ wrapped pair worth two exported names per vocabulary, or would you rather every site compose from the bare form and dropMONTH/SEASON/OPEN_ENDEDentirely?src/lib/heuristics/extract/education.ts(isInlineDatedProgram) — the one site where a wrapped token would silently change the language. Does the comment make the shared-tail constraint clear enough that the next person doesn't "simplify" it to${MONTH}?Test plan
npm run typecheckcleannpm run lintcleannpm run verifygreen (also re-run by thepre-pushhook)383 files / 6450 tests passed, 8 skippedgit statusclean after a full run, so nothing was rebaselinedfallowclean —dead code 0, exit 0. The 15 complexity findings it reports are inherited frommainand gate-excluded; no new ones.regex.ts, andregex.tsstill imports onlysections.config.tsNo fixture binaries touched, so the fixture-PII preflight does not apply.