fix(preserve-numbers): give year classification a context gate (#876) - #919
Conversation
s-annam
left a comment
There was a problem hiding this comment.
The diagnosis is right, the mechanism is the one #876 asked for (isYearContext mirroring the headcount verb/noun gate), and the docblock rewrite carries the reasoning honestly. The issue's repro is genuinely fixed.
What blocks it is cue precision, not cue design. #876 closed with: "This needs its own false-positive audit — a badly-scoped gate would create the missed-legitimate-year-reword failure mode that docblock rule 1 explicitly warns about." I ran that audit, and it comes back negative in both directions: the gate invents claims on non-years, and it stops defending the most common real year forms — including strings already sitting in this repo's fixtures.
Method. I imported main's checkNumbersPreserved alongside this branch's and diffed their verdicts on identical inputs. Everything below is a verified behaviour change introduced by this PR, reproduced twice (once by me, once by an independent /code-review pass). Nothing here is a hypothesis.
Why these are Blocking. applyNumberPreservation (post-process.ts:283) is a hard block since #778: either a false dropped or a false added discards the whole section's rewrite and shows a revert notice. That is the cost the module docblock names — "A false positive here therefore costs a whole section's rewrite." The other direction is worse in kind: an untracked year means the guardrail silently lets the model erase a fact, which is its core job.
CI is green for all of this, because none of the six added tests probes either direction.
Blocking
1. Non-temporal cues invent year claims, discarding good rewrites
YEAR_PREFIX_CUE includes bare and, to, by, from, between, before, after, and the transitive achievement verbs (completed, started, earned, published, awarded). None imply a year; all routinely precede a plain count. Because rule 3 requires an output-side year claim to be matched by an input-side one, any reword that moves a 4-digit quantity next to one of these reads as an invented year:
| input | rewritten to | main |
this PR |
|---|---|---|---|
Delivered 2000 units to the plant. |
Completed 2000 units for the plant. |
ok | ✗ added: ["2000"] |
Handled 2000 escalations across 3 products. |
Handled 3 products and 2000 escalations. |
ok | ✗ added: ["2000"] |
Serving 2000 active users. |
Grew the base to 2000 active users. |
ok | ✗ added: ["2000"] |
Fixed 1980 defects. |
Completed 1980 defect fixes. |
ok | ✗ added: ["1980"] |
Processed 2050 claims. |
Processed claims (2050). |
ok | ✗ added: ["2050"] |
The last row is a separate mechanism: the [(\[‘'"~]alternative claims a year for **any** parenthesised 4-digit number, so(2050)claims exactly as(2020)` does. Those two are not separable by context, so the parens rule buys a test case at the price of every parenthesised quantity.
Note this shape is structurally identical to your own passing test "catches an invented year claim from an unrelated pre-existing digit" (suite 1900 → founded in 1900): one unclaimed 4-digit becoming one claimed 4-digit on the same key. Only cue precision separates the one true positive from the five false ones.
2. The uncued year — broken in both directions
A real year with no lexical cue — the attributive form, which is arguably the dominant year idiom in résumé prose — is now unclaimed. That costs coverage on the drop side and manufactures inventions on the reword side.
Drops the guardrail no longer catches (main caught every one):
"Recipient of the 2019 Excellence Award." -> "Recipient of the Excellence Award." main: ["2019"] now: []
"The 2019 migration reduced latency." -> "The migration reduced latency." main: ["2019"] now: []
"Led the 2021 platform rewrite." -> "Led the platform rewrite." main: ["2021"] now: []
"Winner, 2020 Innovation Award." -> "Innovation Award winner." main: ["2020"] now: []
"Presented at KubeCon 2022." -> "Presented at KubeCon." main: ["2022"] now: []
And the mirror — a faithful reword reported as invention:
"Recipient of the 2019 Excellence Award." -> "Won the Excellence Award in 2019."
main: ok:true this PR: added: ["2019"]
Nothing was invented; the model moved the year into temporal phrasing, which is a rewrite the prompt actively encourages. Worth noting a second consumer: eval/rubric.ts re-derives numbersPreservedRate from this function, so an untracked year counts as preserved — the coverage loss is invisible in the metric that would otherwise surface it.
3. YEAR_FOLLOW_CUE's dash class ≠ RANGE_DASH, so year tracking depends on the PDF's font
The new class is [-–—/] — ASCII hyphen, U+2013, U+2014. RANGE_DASH (line 165) deliberately covers U+2010–U+2015 and U+2212, and its docblock says exactly why: "A PDF extractor emits any of these where the author typed one dash, so recognising only - would make range detection depend on the font." The new class reintroduces the problem it was written to prevent. On a spaced range, Acme Corp 2019 <dash> 2021 senior engineer. → Acme Corp senior engineer.:
| dash | main |
this PR |
|---|---|---|
- U+002D |
dropped: ["2019","2021"] |
dropped: ["2019"] |
– U+2013 |
dropped: ["2019","2021"] |
dropped: ["2019"] |
‒ U+2012 figure dash |
dropped: ["2019","2021"] |
✗ ok: true |
‑ U+2011 non-breaking hyphen |
dropped: ["2019","2021"] |
✗ ok: true |
− U+2212 minus |
dropped: ["2019","2021"] |
✗ ok: true |
Two failures stacked. For three of the six dashes an entire employment date range can be deleted and scored clean. For the other two the range is asymmetric — the left endpoint claims via the follow cue, the right endpoint matches neither cue (isRangeEndpoint needs a tight digit-dash-digit), so half the range is defended. Reusing RANGE_DASH.source fixes the font dependence; the asymmetry needs a prefix-side counterpart to the follow cue.
Corpus check. Sweeping every string in tests/fixtures/rewrite/*.json and tests/fixtures/pdfs/*/*.truth.json (325 strings, 24 containing a bare 1900–2099 integer): 25 year occurrences stay tracked, 10 become untracked — 2020 – 2022, Sep. 2025 – Apr. 2026, 01/2019 - 02/2022, a 2024 product launch. Two more lexicon mechanisms show up there: the …)\s*$ anchor cannot cross an abbreviation period, so Sep 2025 matches but Sep. 2025 does not; and / is in the follow class but not the prefix class, so a numeric date's year has no preceding cue.
On the fix direction
The two blocking directions pull against each other, so it is worth choosing deliberately rather than patching the lexicon twice.
I prototyped the obvious move — mirror FUNCTION_WORD_FOLLOWS and let a prefix cue claim only when the digit does not run into a noun. Across 15 phrases (9 genuine years, 6 false positives) it removed all 6 and kept all 9. But it makes Blocking 2 strictly worse: "digit immediately followed by a noun" is the attributive-year shape.
The cleaner option, I think, is to split the two sides rather than tune one lexicon to serve both: keep year lenient on the invention side (rule 3, line 591) and apply the context gate only to the drop side. #876's entire repro is drop-side, so the gate still does the job it was filed for; and every false invention above — all of Blocking 1, and the reword half of Blocking 2 — disappears, because a year the output claims would once again only need the value present in the input. The cost is that suite 1900 → founded in 1900 stops being caught, which is behaviour #876 never asked for and which the added test introduced on its own initiative. Blocking 3 and the drop-side half of Blocking 2 are then plain lexicon work.
That is a suggestion, not a requirement — you own the call, and if you keep the symmetric gate, the lexicon needs to handle the attributive year in both directions.
Secondary
4. The leading-anchor rule fires on any bullet-initial 4-digit number
leadingText.length === 0 && /^[\s:–—.)/]/.test(after || " ") — the bare \s in that class means every bullet-initial in-range number followed by a space is a year. So 2000 users onboarded in Q1 claims while Onboarded 2000 users in Q1 does not, and a pure word-order rewrite between them reports added: ["2000"] where main was clean. It also sits awkwardly against your own test asserting Delivered 2000 units to production. is not a year — same number, same non-temporal role, opposite verdict on position alone. Requiring a real anchor separator (: or a dash) rather than bare whitespace would close it.
5. Test coverage is drop-side only, so CI stays green through all of the above
Every added case supplies a cue and asserts it claims, or asserts a drop. The invention direction is covered only by the constructed suite 1900 → founded in 1900 case — and that is the direction post-process.ts treats as the worse half. The nearest negative case, "does not treat non-temporal 4-digit bare integers as year claims", checks only the drop side of Delivered 2000 units; the invention side of that same string is what fails. Whatever fix you pick, the audit belongs in the suite — the reword pairs above and the corpus strings in Blocking 3 are ready-made cases.
6. The description reports only the upside of a change #876 flagged as two-sided
## Summary and ## Changes describe the gate accurately — no overclaim about what was built, and the module 2048 example is correctly in range. But the body records no audit, no residual, and no accepted cost, while Resolves #876 would close the issue with its audit requirement unmet. Since the gate strictly narrows what year defends, a short "what this stops tracking, and why that trade is worth it" note belongs in both the PR body and the docblock, the way rules 1–3 already record their trades.
Nits
7. c\.? matches a standalone C
The optional period means the alternative fires on a bare C, case-insensitively — ANSI C 2000, Objective-C 2000, or a table cell C 2000 all classify an arbitrary quantity as a year (verified: Refactored 2000 legacy modules. → Refactored Objective-C 2000 legacy modules. reports added: ["2000"]). circa is already in the alternation, so requiring the period costs nothing.
8. PEOPLE_CONTEXT_WINDOW is now also the year window
isYearContext reuses the headcount constant. Same value, unrelated concern — a neutral CONTEXT_WINDOW keeps the two gates independently tunable.
Gates
| Gate | Result |
|---|---|
/code-review generic pass (high) |
run — findings folded in above; independently reproduced Blocking 1–3 |
| 3a fixture PII | skipped — no fixture binary in the diff |
| 3b design-system / reuse | skipped — no src/components/** change |
| 3c style tokens | skipped — no styling change |
| 3d fallow / dead code | pass (CI fallow green) |
| 3e skill & script command review | skipped — no scripts/** or SKILL.md change |
| 3f description accuracy | accurate on what was built; omits the residual → Secondary 6 |
verify and fallow are green on CI. That is consistent with these findings rather than evidence against them — see Secondary 5.
Acceptance criteria — #876
- ✅ Give
yearclassification a context gate inbareIntegerClaim, mirroring the headcount verb/noun checks —isYearContext. - ✅ Require a temporal cue near the digit, falling back to unclaimed otherwise.
- ✅ The issue's repro (
in 1900dropped whilesuite 1900survives) is caught, and the documented-residual test is replaced with an active one. - ❌ "This needs its own false-positive audit." Blocking 1–3 are that failure mode, in both directions.
Verdict rule: ≥1 Blocking → REQUEST_CHANGES. Three of four ACs are met and the design is the right one — this is lexicon precision plus one architectural call (the drop-side/invention-side split), not a rework.
Reviewed by: Claude Opus 5 (high)
| const YEAR_PREFIX_CUE = new RegExp( | ||
| "(?:\\b(?:" + | ||
| "in|since|during|by|from|until|through|between|before|after|around|circa|c\\.?|" + | ||
| "as\\s+of|class\\s+of|cohort\\s+of|batch\\s+of|and|to|" + |
There was a problem hiding this comment.
Blocking 1. Bare and and to (and on the next line the transitive achievement verbs) are not temporal — they routinely precede a plain count. Combined with rule 3's strict invention check, any reword that moves a 4-digit quantity next to one of these reports an invented year and applyNumberPreservation discards the whole section.
Verified against main:
["Handled 2000 escalations across 3 products."] -> ["Handled 3 products and 2000 escalations."]
main: ok:true this PR: added: ["2000"]
["Serving 2000 active users."] -> ["Grew the base to 2000 active users."]
main: ok:true this PR: added: ["2000"]
["Fixed 1980 defects."] -> ["Completed 1980 defect fixes."]
main: ok:true this PR: added: ["1980"]
Contrast YEAR_FOLLOW_CUE, which is disciplined about exactly this: it spells and\s+\d{4} and to\s+(?:\d{4}|present|…) rather than bare and/to. The prefix side wants the same treatment — a preceding companion year, not the bare connective.
Also on line 298: the [(\[‘'"~]alternative claims a year for *any* parenthesised in-range number, soProcessed claims (2050).claims exactly as(2020)` does. Those two aren't separable by context.
| // while making a claim the résumé never made. | ||
| const added = missingFrom(outputAtoms, (atom) => | ||
| atom.claim === "headcount" | ||
| atom.claim === "headcount" || atom.claim === "year" |
There was a problem hiding this comment.
Blocking 2 / the architectural call. Adding year here is what converts every cue-lexicon gap into a false invention, on top of the coverage loss the gate already causes on the drop side:
["Recipient of the 2019 Excellence Award."] -> ["Won the Excellence Award in 2019."]
main: ok:true this PR: added: ["2019"]
Nothing was invented — the model moved the year into temporal phrasing, which is a rewrite the prompt encourages.
Consider reverting just this line and keeping year lenient on the invention side, applying the context gate only to the drop side (line 577). #876's repro is entirely drop-side, so the gate still does the job it was filed for, and every false invention in this review disappears. The cost is that suite 1900 → founded in 1900 stops being caught — behaviour #876 never asked for, introduced by the added test on its own initiative.
See the review body for the full argument; your call either way.
| "through\\s+(?:\\d{4}|present|current|now|ongoing)|" + | ||
| "and\\s+\\d{4}|" + | ||
| "jan(?:uary)?|feb(?:ruary)?|mar(?:ch)?|apr(?:il)?|may|jun(?:e)?|jul(?:y)?|aug(?:ust)?|sep(?:t|tember)?|oct(?:ober)?|nov(?:ember)?|dec(?:ember)?" + | ||
| ")\\b|[)\\]’'\"`]|\\s*[-–—/]\\s*(?:\\d{4}|present|current|now|ongoing)\\b)", |
There was a problem hiding this comment.
Blocking 3. This dash class is [-–—/] — ASCII hyphen, U+2013, U+2014 — but RANGE_DASH (line 165) deliberately covers U+2010–U+2015 and U+2212, and its docblock says why: "A PDF extractor emits any of these where the author typed one dash, so recognising only - would make range detection depend on the font." This class reintroduces exactly that.
["Acme Corp 2019 <dash> 2021 senior engineer."] -> ["Acme Corp senior engineer."]:
| dash | main |
this PR |
|---|---|---|
- U+002D |
["2019","2021"] |
["2019"] |
– U+2013 |
["2019","2021"] |
["2019"] |
‒ U+2012 |
["2019","2021"] |
ok:true |
‑ U+2011 |
["2019","2021"] |
ok:true |
− U+2212 |
["2019","2021"] |
ok:true |
For three of the six dashes a whole employment date range is deleted and scored clean. For the other two it is asymmetric: the left endpoint claims here, the right endpoint matches neither cue (isRangeEndpoint needs a tight digit-dash-digit), so half the range is defended.
Interpolating RANGE_DASH.source fixes the font dependence — the same trick PEOPLE_NOUN_FOLLOW already uses on line 187. The asymmetry needs a prefix-side counterpart to this cue.
| .slice(0, matchStart) | ||
| .trim() | ||
| .replace(/^[-*•⁃–—\s]+/, ""); | ||
| if (leadingText.length === 0 && /^[\s:–—.)/]/.test(after || " ")) { |
There was a problem hiding this comment.
Secondary. The leading \s in /^[\s:–—.)/]/ makes this fire on any bullet-initial in-range number followed by a space, not just a date anchor. So 2000 users onboarded in Q1 is a year claim while Onboarded 2000 users in Q1 is not, and a pure word-order rewrite between the two reports added: ["2000"] where main was clean.
It also sits awkwardly against the test you added asserting Delivered 2000 units to production. is not a year — same number, same non-temporal role, opposite verdict decided by position alone.
Requiring a real anchor separator (: or a dash) rather than bare whitespace would close it.
| */ | ||
| const YEAR_PREFIX_CUE = new RegExp( | ||
| "(?:\\b(?:" + | ||
| "in|since|during|by|from|until|through|between|before|after|around|circa|c\\.?|" + |
There was a problem hiding this comment.
Nit. c\.? — the optional period means this matches a standalone C, case-insensitively. ANSI C 2000, Objective-C 2000, or a table cell C 2000 all classify an arbitrary quantity as a year:
["Refactored 2000 legacy modules."] -> ["Refactored Objective-C 2000 legacy modules."]
main: ok:true this PR: added: ["2000"]
circa is already spelled out in the alternation, so requiring the period (c\.) costs nothing.
|
|
||
| const matchStart = match.index; | ||
| const before = bullet.slice( | ||
| Math.max(0, matchStart - PEOPLE_CONTEXT_WINDOW), |
There was a problem hiding this comment.
Nit. PEOPLE_CONTEXT_WINDOW is the headcount gate's constant; reusing it here couples two unrelated concerns to one value. A neutral CONTEXT_WINDOW (or a separate YEAR_CONTEXT_WINDOW) keeps them independently tunable. Same value today, so no behaviour change either way.
|
Thank you for the thorough, precise review and for sharing the verification harness. All blocking, secondary, and nit items have been addressed in 1. Decoupled Invention Gate from Context Gate (Blocking 2 & 1)
2. Refined Temporal Cues in
|
s-annam
left a comment
There was a problem hiding this comment.
Reviewed against #876's acceptance criteria first, the diff second, the description last.
The mechanism is right and the headline AC is met. #876 asked for a context gate on year mirroring headcount's verb/noun check, plus moving year into the count-aware presence bucket — that is what landed, and the issue's exact repro flips from ok: true to dropped: ["1900"]. npm run verify passes locally (exit 0); fallow reports no issues in the two changed files.
The problem is the cue list's calibration, and #876 called this shot in advance: "This needs its own false-positive audit — a badly-scoped gate would create the missed-legitimate-year-reword failure mode." The gate as written misses in both directions at once, and both are regressions against main:
- Too narrow — six common résumé year forms are no longer defended at all.
maincaught them; this branch returnsok: trueon a rewrite that deletes them. - Too broad — the non-temporal cues (
the|a|an|our|this,of|at|for) still claim ordinary quantities as years, and becauseyearnow takes the strict drop rule, those false claims have been upgraded from harmless to section-discarding.
Net: it loses true positives and keeps (then sharpens) the false ones. Everything below was verified by running checkNumbersPreserved on this branch against origin/main.
Blocking
1. Six common year forms are silently no longer defended
YEAR_PREFIX_CUE has no cue for punctuation-adjacent years or for the credential/award verb family, and at\s+[A-Za-z0-9_-]+ spans only a single token. Before this PR every 1900–2099 integer was a defended year, so each of these is a regression — the rewrite deletes a real year and the guardrail scores it clean:
PR main
["Speaker at PyCon (2019)."] -> ["Speaker at PyCon."] ok dropped 2019
["B.S. Computer Science, 2019."] -> ["B.S. Computer Science."] ok dropped 2019
["Awarded Employee of the Year 2021."] -> [... "Year."] ok dropped 2021
["AWS Certified Architect 2019."] -> ["AWS Certified Architect."] ok dropped 2019
["Shipped the platform 2018."] -> ["Shipped the platform."] ok dropped 2018
["Worked at Acme Corp 2019."] -> ["Worked at Acme Corp."] ok dropped 2019
Controls confirm the cue list is what separates them: "Shipped the platform in 2018." is caught (the in cue), and "Presented at KubeCon 2022." is caught while "Worked at Acme Corp 2019." is not — the only difference is that Acme Corp is two tokens. The PR's own test exercises only the one-word company (KubeCon 2022), which is why the gap is invisible in CI.
These are education-section dates, certifications, conference talks and award lines — the highest-density year forms on a résumé. Wanted: cues for a year adjacent to (, , or a terminal ., an awarded|certified|earned|completed|launched|published|promoted verb family, and a multi-token span after at.
2. Two new false-revert paths, both discarding a whole section silently
The docblock's opening sets the priority this violates: "A false positive here therefore costs a whole section's rewrite, which is what drives the rules under Comparison semantics."
(a) The year survives, but in a phrasing the cue list does not recognise (preserve-numbers.ts:592). The strict count-parity rule then cannot see it:
in : ["Presented at KubeCon in 2022.", "Refactored 2022 legacy modules."]
out: ["KubeCon 2022 speaker; refactored legacy modules."]
PR: ok=false dropped=["2022"] | main: ok=true
2022 is right there in the output. This is finding 1's coupling: the stricter presence rule is only as safe as the cue list is complete, and it is not complete.
(b) Non-temporal cues claim ordinary quantities (preserve-numbers.ts:298,301). the|a|an|our|this and of|at|for are not temporal; any 1900–2099 quantity behind one is claimed as a year. Four plain-English merges under MERGE_AND_PRUNE_RULE, all clean on main, all false reverts here:
["Delivered the 2000 units.", "Tracked 2000 tickets."] -> ["Delivered and tracked 2000 items."]
["Reduced by 2000 hours.", "Cut 2000 tickets."] -> ["Cut 2000 hours and tickets."]
["A total of 2000 records.", "Indexed 2000 rows."] -> ["Indexed 2000 records and rows."]
["Ran campaign for 2000 customers.", "Emailed 2000 leads."] -> ["Reached 2000 customers and leads."]
main: ok=true | here: ok=false dropped=["2000"] (all four)
PEOPLE_VERB_PREFIX already excludes bare of for exactly this over-trigger reason — the year list should follow the same discipline.
This half is inherent, not a tuning miss: the 2019 migration and the 2000 units are syntactically identical, so a determiner cue cannot separate them. I measured the removal cost. Dropping the|a|an|our|this and the bare of|at|for fails exactly one test, "catches dropped attributive years (#876)", and within it only two of five cases:
STILL CAUGHT | Recipient of the 2019 Excellence Award. (recipient\s+of(?:\s+the)? — own cue)
now MISSED | The 2019 migration reduced latency.
now MISSED | Led the 2021 platform rewrite.
STILL CAUGHT | Winner, 2020 Innovation Award. (winner,? — own cue)
STILL CAUGHT | Presented at KubeCon 2022. (at\s+[A-Za-z0-9_-]+ — own cue)
The issue's repro and all 79 other tests still pass. The specific phrase cues already carry most attributive coverage; only the two determiner-only cases go, and those are precisely the ones indistinguishable from the 2000 units.
3. A range's two endpoints get different presence rules
isYearContext condition 4 (preserve-numbers.ts:378) returns year for any range endpoint in 1900–2099, so a tight range straddling that boundary gets range (lenient) on one endpoint and year (strict) on the other. That breaks the dash re-spelling rule 1 exists to permit:
["Processed 1000-2000 tickets.", "Closed 2000 escalations."] -> ["Processed 1000 to 2000 tickets."]
PR: ok=false dropped=["2000"] | main: ok=true
# structurally identical, no year-range endpoint:
["Processed 50-100 tickets.", "Closed 100 escalations."] -> ["Processed 50 to 100 tickets."]
PR: ok=true | main: ok=true
Condition 4 is also redundant with bareIntegerClaim's own isRangeEndpoint fallback — the only thing it changes is the claim kind, and hence the presence rule. Suggest dropping it so range endpoints stay range uniformly; genuine date ranges are already covered by the prefix/follow cues (2019 - 2021, to \d{4}, and \d{4}), which the 6-dash test exercises.
4. Module docblock rule 3 now describes the opposite of what the code does
Rule 3 (preserve-numbers.ts:94-97) was rewritten to say suite 1900 → founded in 1900 is caught, and that an output atom read as a HEADCOUNT or a YEAR is present only when the value is claimed on the input side. The code keeps year lenient on the invention side, and the inline comment at :604-606 says so outright. Verified:
in : ["Operated out of suite 1900."] out: ["Founded the program in 1900."]
=> ok=true added=[] # rule 3 claims this is caught
in : ["Completed phase 5."] out: ["Led 5 engineers."]
=> ok=false added=["5"] # headcount control, correctly caught
The PR description agrees with the code, not with rule 3 — so rule 3 is the wrong artifact, and the file now contradicts itself about one branch. This is the design contract the next person reads before touching missingFrom's isPresent callbacks.
Fix: keep the lenient behaviour (your reasoning for it is sound — "2019 Excellence Award" → "Award in 2019" is a legitimate and common reword) and revert rule 3's add-side sentence to headcount-only, dropping suite 1900 → founded in 1900 from the caught list. Then state the exception explicitly: year takes the strict rule on the DROP side (rule 1) but stays lenient on the ADD side, because a year migrates into temporal context during a reword far more often than a headcount does.
Secondary
5. The new "invented years" test cannot fail
preserve-numbers.test.ts:748 asserts .added is empty across six pairs — but year is lenient on the add side (inputKeys.has), so every pair passes whenever the digit appears anywhere in the input. I ran all six against origin/main's pre-gate module: identical results, ok=true added=[] for all six. The block guards nothing this PR introduced, which also explains the contrived "Refactored Objective-C 2000 legacy modules." output — it reads as tuned to pass rather than drawn from a plausible rewrite.
Re-point these at the drop side, where the gate actually changed behaviour. The four cases in Blocking 2(b) and the six in Blocking 1 are the regression tests this change needs. By contrast "does not treat non-temporal 4-digit bare integers as year claims (#876)" (:780) is a real guard — it fails on main, passes here. That is the shape to follow.
Nits (non-blocking)
preserve-numbers.ts:372— the leading-anchor regex is rebuilt withnew RegExp(...)on every call, in a function running per numeric atom per bullet.RANGE_DASH.sourceis already interpolated into module-scope constants in this file; hoist this one the same way.preserve-numbers.ts:303/:324— the twelve-month alternation is duplicated verbatim between the two cue regexes. Extract aMONTH_NAMEconstant and interpolate.source, matching howRANGE_DASHis reused two constants above.preserve-numbers.ts:29(unchanged line, so no anchor) — "Tokens covered" still readsYears (1900-2099) and date ranges: 2019, 2019-2021unconditionally. The neighbouring line qualifies headcounts as "in people-management context"; years now need the same qualifier, and after Blocking 1 this line materially overstates coverage.preserve-numbers.test.tsis not Prettier-clean (several added lines exceed the print width). Not CI-enforced —lintiseslint .andverifydoes not run Prettier — so cosmetic only:npx prettier --write src/lib/webllm/preserve-numbers.test.ts.- Two commits on the branch;
mainsquashes from the branch, so please collapse to one before merge. I deliberately did not do it for you — this is a fork branch, and force-pushing it would rewrite your local work.
Gates
| Gate | Result |
|---|---|
npm run verify |
pass (exit 0) |
lint / typecheck |
pass |
| fallow (3d) | no issues in the 2 changed files; 1 inherited finding excluded |
| Fixture PII (3a) | n/a — no fixtures touched |
| Design system / reuse (3b) | n/a — no components touched |
| Style tokens (3c) | n/a — grep hits were #876 issue refs, not colours |
| Skill/script command bugs (3e) | n/a — no .claude/** or scripts/** changes |
| Description accuracy (3f) | accurate, unusually so — it states the lenient add-side behaviour correctly, which is what identifies the docblock as the defect in Blocking 4, and it lists the determiner cues openly. It omits their false-positive cost (Blocking 2b) and the coverage regression (Blocking 1). |
A green verify is not evidence against any of this: every case above is a behaviour the existing tests do not cover.
Provenance for whoever picks this up: fork PR, 8th from this contributor; #876 was filed by the maintainer, not the author.
Reviewed by: Claude Opus 5 (high)
| const YEAR_PREFIX_CUE = new RegExp( | ||
| "(?:\\b(?:" + | ||
| "in|since|during|by|from|until|through|between|before|after|around|circa|c\\.|at|of|for|" + |
There was a problem hiding this comment.
Blocking — the cue list is too narrow: six common year forms are no longer defended.
Before this PR every 1900–2099 integer was a defended year. These all regress — main reports the drop, this branch returns ok: true and the rewrite deletes a real year silently:
["Speaker at PyCon (2019)."] -> ["Speaker at PyCon."]
["B.S. Computer Science, 2019."] -> ["B.S. Computer Science."]
["Awarded Employee of the Year 2021."] -> ["Awarded Employee of the Year."]
["AWS Certified Architect 2019."] -> ["AWS Certified Architect."]
["Shipped the platform 2018."] -> ["Shipped the platform."]
Control: "Shipped the platform in 2018." is caught, so the cue list is exactly what separates them.
These are education dates, certifications, talks and award lines — the highest-density year forms on a résumé. Needs cues for a year adjacent to (, , or a terminal ., plus an awarded|certified|earned|completed|launched|published|promoted verb family.
This also couples to the strict drop rule below: that rule is only as safe as this list is complete.
| "as\\s+of|class\\s+of|cohort\\s+of|batch\\s+of|" + | ||
| "founded|established|graduated|joined|winner,?\\s*|won,?\\s*|recipient\\s+of(?:\\s+the)?|" + | ||
| "the|a|an|our|this|" + | ||
| "at\\s+[A-Za-z0-9_-]+|" + |
There was a problem hiding this comment.
Blocking (same finding) — this cue spans a single token, so it is company-name-shape dependent.
["Presented at KubeCon 2022."] -> ["Presented at KubeCon."] => dropped 2022 ✅
["Worked at Acme Corp 2019."] -> ["Worked at Acme Corp."] => ok: true ❌ (main: dropped 2019)
The only difference is that Acme Corp is two tokens. The test added in this PR exercises only the one-word case (KubeCon 2022), which is why the gap is invisible in CI — worth a multi-token case alongside the fix.
| "in|since|during|by|from|until|through|between|before|after|around|circa|c\\.|at|of|for|" + | ||
| "as\\s+of|class\\s+of|cohort\\s+of|batch\\s+of|" + | ||
| "founded|established|graduated|joined|winner,?\\s*|won,?\\s*|recipient\\s+of(?:\\s+the)?|" + | ||
| "the|a|an|our|this|" + |
There was a problem hiding this comment.
Blocking — and simultaneously too broad: these are not temporal cues.
the|a|an|our|this (here) and of|at|for (line 298) claim any 1900–2099 quantity behind an article or generic preposition as a year. Because this PR also moves year into the strict drop bucket, those false claims are upgraded from harmless to section-discarding. Four plain-English merges under MERGE_AND_PRUNE_RULE, all clean on main, all false reverts here:
["Delivered the 2000 units.", "Tracked 2000 tickets."] -> ["Delivered and tracked 2000 items."]
["Reduced by 2000 hours.", "Cut 2000 tickets."] -> ["Cut 2000 hours and tickets."]
["A total of 2000 records.", "Indexed 2000 rows."] -> ["Indexed 2000 records and rows."]
["Ran campaign for 2000 customers.", "Emailed 2000 leads."] -> ["Reached 2000 customers and leads."]
main: ok=true | here: ok=false dropped=["2000"] (all four)
PEOPLE_VERB_PREFIX already excludes bare of for exactly this over-trigger reason.
This is inherent, not a tuning miss — the 2019 migration and the 2000 units are syntactically identical, so a determiner cue cannot separate them. I measured the removal cost: dropping the|a|an|our|this and bare of|at|for fails one test ("catches dropped attributive years") and only two of its five cases — The 2019 migration and Led the 2021 platform rewrite. Recipient of the …, Winner, … and at KubeCon … survive on their own dedicated cues, and the #876 repro plus all 79 other tests still pass.
| // exists to allow. | ||
| const dropped = missingFrom(inputAtoms, (atom) => { | ||
| if (atom.claim !== "headcount") return outputKeys.has(atom.key); | ||
| if (atom.claim !== "headcount" && atom.claim !== "year") { |
There was a problem hiding this comment.
Blocking — the strict drop rule fires when the year plainly survived.
With the cue list incomplete (line 298), count-parity cannot see a surviving year that landed in unrecognised phrasing:
in : ["Presented at KubeCon in 2022.", "Refactored 2022 legacy modules."]
out: ["KubeCon 2022 speaker; refactored legacy modules."]
PR: ok=false dropped=["2022"] | main: ok=true
2022 is right there in the output, and per the module docblock a false positive here discards the entire section rewrite silently — the expensive direction. This finding and the coverage gap are one problem: tightening presence is only safe once the cue list is complete.
| } | ||
|
|
||
| // 4. Tight range endpoint joining another digit (e.g. "2019-2021") | ||
| if (isRangeEndpoint(match, bullet)) { |
There was a problem hiding this comment.
Blocking — the two endpoints of one range get different presence rules.
Condition 4 returns year for any range endpoint in 1900–2099, so a tight range straddling that boundary is range (lenient) on one side and year (strict) on the other. That breaks the dash re-spelling rule 1 exists to permit:
["Processed 1000-2000 tickets.", "Closed 2000 escalations."] -> ["Processed 1000 to 2000 tickets."]
PR: ok=false dropped=["2000"] | main: ok=true
# structurally identical, no year-range endpoint:
["Processed 50-100 tickets.", "Closed 100 escalations."] -> ["Processed 50 to 100 tickets."]
PR: ok=true | main: ok=true
Condition 4 is also redundant with bareIntegerClaim's own isRangeEndpoint fallback — the only thing it changes is the claim kind, and hence the presence rule. Suggest dropping it so range endpoints stay range uniformly; genuine date ranges are already covered by the prefix/follow cues (2019 - 2021, to \d{4}, and \d{4}) that the 6-dash test exercises.
| * → `5 engineers` or `suite 1900` → `founded in 1900` reuses the digit and | ||
| * invents the claim, and under a plain rule-1 lookup it scored clean. So an | ||
| * output atom the surrounding prose reads as a HEADCOUNT or a YEAR counts as |
There was a problem hiding this comment.
Blocking — this now states the opposite of what the code does.
Rule 3 says an output atom read as a HEADCOUNT or a YEAR is present only when the value is claimed on the input side, and cites suite 1900 → founded in 1900 as caught. The code keeps year lenient on the invention side, and the inline comment at line 604 says so outright:
in : ["Operated out of suite 1900."] out: ["Founded the program in 1900."]
=> ok=true added=[] # rule 3 claims this is caught
in : ["Completed phase 5."] out: ["Led 5 engineers."]
=> ok=false added=["5"] # headcount control, correctly caught
Your PR description agrees with the code, not with this paragraph — so this is the wrong artifact, and the file now contradicts itself about one branch. It is the design contract the next person reads before touching missingFrom's isPresent callbacks.
Fix: keep the lenient behaviour (the reasoning is sound — "2019 Excellence Award" → "Award in 2019" is a legitimate, common reword) and revert this sentence to headcount-only, dropping suite 1900 → founded in 1900 from the caught list. Then say the exception outright: year takes the strict rule on the DROP side (rule 1) but stays lenient on the ADD side, because a year migrates into temporal context during a reword far more often than a headcount does.
| .replace(/^[-*•⁃–—\s]+/, ""); | ||
| if ( | ||
| leadingText.length === 0 && | ||
| new RegExp(`^\\s*(?:[:.)]|${RANGE_DASH.source})\\s*`).test(after) |
There was a problem hiding this comment.
Nit — this regex is rebuilt on every call, inside a function that runs per numeric atom per bullet. RANGE_DASH.source is already interpolated into module-scope constants in this file (PEOPLE_NOUN_FOLLOW, YEAR_PREFIX_CUE); hoist this one the same way.
| "founded|established|graduated|joined|winner,?\\s*|won,?\\s*|recipient\\s+of(?:\\s+the)?|" + | ||
| "the|a|an|our|this|" + | ||
| "at\\s+[A-Za-z0-9_-]+|" + | ||
| "jan(?:uary)?\\.?|feb(?:ruary)?\\.?|mar(?:ch)?\\.?|apr(?:il)?\\.?|may|jun(?:e)?\\.?|jul(?:y)?\\.?|aug(?:ust)?\\.?|sep(?:t|tember)?\\.?|oct(?:ober)?\\.?|nov(?:ember)?\\.?|dec(?:ember)?\\.?|" + |
There was a problem hiding this comment.
Nit — this twelve-month alternation is duplicated verbatim in YEAR_FOLLOW_CUE (line 324). Extracting a MONTH_NAME constant and interpolating .source matches how RANGE_DASH is reused two constants above, and keeps the two lists from drifting apart.
| expect(result.added).toEqual([]); | ||
| }); | ||
|
|
||
| it("does not falsely report invented years when quantities move next to connectives/verbs (#876)", () => { |
There was a problem hiding this comment.
Secondary — this test cannot fail.
year is lenient on the add side (inputKeys.has), so .added is empty for every pair as long as the digit appears anywhere in the input. I ran all six pairs against origin/main's pre-gate module: identical results, ok=true added=[] for all six. The block guards nothing this PR introduced.
That also explains the contrived "Refactored Objective-C 2000 legacy modules." output in the last pair — it reads as tuned to pass rather than drawn from a plausible rewrite.
Re-point these at the drop side, where the gate actually changed behaviour. The four merge cases in the YEAR_PREFIX_CUE thread and the six coverage cases are the regression tests this change needs. By contrast "does not treat non-temporal 4-digit bare integers as year claims" at line 780 is a real guard — it fails on main and passes here; that is the shape to follow.
e2127d5 to
654b46d
Compare
|
All review findings from the latest round have been resolved in squashed commit 1. Common Resume Year Forms Defended (Blocking 1)
2. Determiners & Generic Prepositions Removed (Blocking 2b)
3. Range Endpoints Uniformity (Blocking 3)
4. Docblock & Contract Alignment (Blocking 4)
5. Nits & Code Quality
6. Verification
|
654b46d to
6af63d4
Compare
Summary
Resolves #876.
Gives
yearclassification inbareIntegerClaima surrounding context gate (isYearContext), preventing unrelated 4-digit numbers (e.g."suite 1900","module 2048","2000 units") from unconditionally claiming as years.Context Gate & Architectural Trade-off
isYearContextgates whether a 4-digit number is claimed as a year. A genuinely dropped year ("in 1900") is tracked asclaim = "year", preventing an unrelated surviving digit ("suite 1900") from masking the drop.yearclaims remain lenient on additions (inputKeys.has(atom.key)), allowing legitimate temporal rewording (e.g."2019 Excellence Award"→"Award in 2019") without false-positive reverts.the|a|an|our|this) and prepositions (of|at|for) are excluded from claiming bare 4-digit numbers as years without dedicated temporal phrasing. This avoids false reverts on plain-English merges of 4-digit quantities ("Delivered the 2000 units.","Reduced by 2000 hours."), while specific phrase cues (recipient of the...,winner, ...,at [Company/Conference]...,[Action verb] the [Noun] [Year]) defend genuine attributive and credential year forms.Changes
src/lib/webllm/preserve-numbers.ts:YEAR_CONTEXT_WINDOW = 32.MONTH_NAMEandLEADING_DATE_ANCHOR_SEPARATORmodule-level constants.YEAR_PREFIX_CUE: matches temporal prepositions (in,since,during,until,through,between,before,after,around,circa,c\.), phrases (as of,class of,cohort of,batch of), verbs and multi-token action phrases (awarded,certified,earned,completed,launched,published,promoted,shipped,graduated,founded,established,joined), award titles (employee of the year), credentials (certified architect), conference/company names afterat, month names, seasons, quarters, companion range years ((?:19\d\d|20\d\d)\s*(?:${RANGE_DASH.source}|/|to|and)), and date slashes.YEAR_FOLLOW_CUE: matches temporal qualifiers (onwards,present,current,now,ongoing), ranges (to \d{4},until \d{4},through \d{4},and \d{4}), months, andRANGE_DASH//followed by companion years or ongoing markers.isYearContext: checks prefix cues, follow cues, and leading bullet date anchors (requiring an anchor separator:,.,), orRANGE_DASH).year.src/lib/webllm/preserve-numbers.test.ts:Verification
npm run verifypassed cleanly: 383 test files passed, 6,451 / 6,451 tests passing.