fix(preserve-numbers): give year classification a context gate (#876) - #919
Conversation
s-annam
left a comment
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
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
s-annam
left a comment
s-annam
left a comment
There was a problem hiding this comment.
Round 3, reviewed against #876's ACs first, the diff second, the description last.
Rounds 1 and 2 are cleared. I re-ran both earlier rounds' failing tables against this head rather than taking the description's word for it:
| Earlier finding | Status on 6af63d4 |
|---|---|
R1/R2 — false inventions (Delivered 2000 units → Completed 2000 units, (2050), Objective-C 2000, +4 more) |
fixed — all 7 ok, added: [] |
R1 — YEAR_FOLLOW_CUE dash class ≠ RANGE_DASH (3 of 6 dashes lost a whole date range) |
fixed — RANGE_DASH.source reused, all 6 covered by test |
R2 — 6 undefended year forms (PyCon (2019), B.S. …, 2019, Employee of the Year 2021, AWS Certified Architect 2019, Shipped the platform 2018, Worked at Acme Corp 2019) |
fixed — all 6 tracked |
R1 — leading anchor fired on bare whitespace · c\.? matched a standalone C · year window aliased PEOPLE_CONTEXT_WINDOW |
fixed |
| R2 — drop determiner cues and record the cost | done, and correctly documented |
Corpus check. Sweeping tests/fixtures/rewrite/*.json + tests/fixtures/pdfs/*/*.truth.json for year values occurring exactly once in their string: 30 of 31 tracked, against 10 untracked when I ran the same sweep in round 1. The one miss (during the 2024 product launch) is the determiner case we agreed to give up — not a finding. npm run verify exits 0 here too.
The mechanism is right and the AC's headline is met. What blocks is that the cue list added to fix round 2 over-triggers on ordinary quantities, and one direction of the guardrail quietly stopped working.
Everything below is a verified behaviour change against origin/main, reproduced twice — once by me, once by an independent /code-review pass that ran the same differential from transpiled builds. Nothing here is a hypothesis, and CI is green through all of it because no added test probes these shapes.
Why the severities are what they are: since #778 applyNumberPreservation (post-process.ts:283) is a hard block, so a false dropped discards the whole section's rewrite, and a missed added lets the model invent a fact unchallenged. The docblock names the first cost itself — "A false positive here therefore costs a whole section's rewrite."
Blocking
1. The bare past-tense verb family claims the single most common résumé quantity phrasing
Line 319 puts awarded|certified|earned|completed|launched|published|promoted|shipped|graduated|founded|established|joined directly against )\s*$, so the verb needs only to be the last token before the digit. That is exactly the shape of a bullet counting things:
in : ["Shipped 2000 features.", "Reviewed 2000 PRs."]
out: ["Reviewed 2000 PRs and features."]
main: ok=true this PR: ok=false dropped=["2000"]
["Completed 2000 tickets.", "Logged 2000 hours."] -> ["Logged 2000 hours and tickets."] same
["Launched 2000 campaigns.", "Tracked 2000 leads."] -> ["Tracked 2000 leads and campaigns."] same
I asked for this verb family in round 2, so this is the cost of my own request — but the form I asked it to recover was Shipped the platform 2018, and that is already carried by the line 321 branch (verb + determiner + noun + year). The bare arm on line 319 adds Shipped 2000 and, as far as I can tell, no test case that line 321 does not already cover. Worth checking whether line 319's verbs can simply be dropped, or need a following date-ish token rather than any 4-digit number.
2. The context window slices mid-word, so \b cues match word fragments
isYearContext takes a fixed 32-character slice before the digit. That slice routinely cuts through a word, and every alternative is \b-anchored — but the cut itself fabricates the boundary. The at\s+(?:[A-Za-z0-9&.'-]+\s+)+$ branch then fires on the tail of any word ending in at. Same sentence, same trailing phrase; only the padding changes where the window lands:
CLAIMED "Team that absorbed a 4x surge during a 2024 launch." window="at absorbed a 4x surge during a "
CLAIMED "Team great absorbed a 4x surge during a 2024 launch." window="at absorbed a 4x surge during a "
CLAIMED "Team format absorbed a 4x surge during a 2024 launch." window="at absorbed a 4x surge during a "
CLAIMED "Team combat absorbed a 4x surge during a 2024 launch." window="at absorbed a 4x surge during a "
unclaimed "Team X absorbed a 4x surge during a 2024 launch." window=" X absorbed a 4x surge during a "
unclaimed "Team XX absorbed a 4x surge during a 2024 launch." window="XX absorbed a 4x surge during a "
that, great, format, combat — none is temporal; each truncates to a bare at. This is not a lexicon miss, it is the gate being positionally unstable: a cue's verdict is a function of the digit's absolute offset, so a case that passes as a test can fail in prose that says the same thing, and #876's false-positive audit cannot mean much while that holds. It affects YEAR_FOLLOW_CUE's trailing slice too, and the same pattern sits in the headcount window (compelled → led) — pre-existing, not yours to fix, but one helper covers both.
const rawStart = Math.max(0, matchStart - YEAR_CONTEXT_WINDOW);
const before =
// a window that cuts mid-word fabricates a \b; drop the partial token
rawStart > 0 && /\w/.test(bullet[rawStart - 1] ?? "")
? bullet.slice(rawStart, matchStart).replace(/^\S*/, "")
: bullet.slice(rawStart, matchStart);3. The add side no longer defends an invented year at all — the docblock says "lenient", but the behaviour is "undefended"
Line 619 records that year "remains lenient" on the invention side. Leniency would mean an output year claim needs only the value present in the input. What actually happens is that a 4-digit number with no temporal cue is now claim: "none", so it is never a candidate on the add side in the first place — and a fabricated year ships silently:
["Delivered units to production."] -> ["Delivered 2000 units to production."]
main: added: ["2000"] this PR: ok=true
["Received the Excellence Award."] -> ["Received the 2019 Excellence Award."]
main: added: ["2019"] this PR: ok=true
["Worked on the platform."] -> ["Worked on the platform in 2019."]
main: added: ["2019"] this PR: added: ["2019"] (still caught — has a cue)
The third row is the one the docblock's example describes; the first two are the gap. A model inventing a credential year onto an award line is a realistic failure and it is the guardrail's core job to catch it. eval/rubric.ts re-derives numbersPreservedRate from this function, so the loss is invisible in the metric that would otherwise surface it. Either defend an output 4-digit whose value is absent from the input regardless of claim, or record this as an explicit accepted residual — but the docblock as written does not describe what the code does.
Secondary
4. [A-Z] is inert under the i flag — free fix, verified 82/82 green
YEAR_PREFIX_CUE is built with "i", which makes [A-Z] equivalent to [A-Za-z] and case-folds the suffixes. The branch reads "CamelCase conference name" and compiles to "any word ending in con/fest/summit/awards/…":
DevCon: true KubeCon: true <- intended
silicon: true Falcon: true beacon: true lexicon: true bacon: true <- not
JS has no inline case-sensitivity modifier, so it needs its own regex. I verified this keeps all 82 tests green:
/** Case-SENSITIVE: CamelCase conference/product names. Kept out of
* YEAR_PREFIX_CUE because that regex is /i, which makes [A-Z] match a-z. */
const YEAR_PREFIX_CUE_CAMEL =
/\b[A-Z][A-Za-z0-9]*(?:Con|Conf|Summit|Meetup|Expo|Fest|Symposium|Workshop|Awards?)\s*$/;…dropping that alternative from line 325 and testing YEAR_PREFIX_CUE.test(before) || YEAR_PREFIX_CUE_CAMEL.test(before).
5. The credential branch binds wider than the description says, and the extra arm is dead — free fix, verified 82/82 green
The description lists this as "credentials (certified architect)". | binds across the whole group, so architect, developer, master, expert, associate, professional, specialist, administrator, practitioner each claim a year standalone: Senior developer 2000 hours logged, Lead architect 2000 tickets triaged, Master 2000 records reconciled. I deleted those arms, leaving (?:certified\s+[A-Za-z0-9&.'-]+)\s+, and all 82 tests still pass — AWS Certified Architect 2019 is carried entirely by the certified <word> arm. Reads like it was meant to be certified\s+(?:[A-Za-z0-9&.'-]+|architect|specialist|…).
6. \d{1,2}\s*[/.-] swallows a range whose left endpoint is short, so strictness depends on the left endpoint's digit count
The date-slash branch pre-empts isRangeEndpoint (year is tested first, line 445), and its - makes 50-2000 a strict year while 1000-2000 is a lenient range — identical construct, different rule:
["Processed 50-2000 tickets.", "Closed 2000 escalations."] -> ["Processed 50 to 2000 tickets."] ok=false dropped=["2000"]
["Processed 1000-2000 tickets.", "Closed 2000 escalations."] -> ["Processed 1000 to 2000 tickets."] ok=true
main is clean on both. The new "treats range endpoints uniformly across the 1900-2099 boundary" test covers only the 1000- variant, which is why it passes. Restricting this branch to / and . and leaving - to range detection looks like the fix.
7. The leading date-anchor accepts RANGE_DASH, so a bullet that opens with a numeric range reads as a year anchor
["2000 - 3000 units delivered.", "Stocked 2000 SKUs."] -> ["Delivered units and stocked 2000 SKUs."]
main: ok=true this PR: ok=false dropped=["2000"]
Requiring the token after the dash to be a year or present/ongoing — which YEAR_FOLLOW_CUE already does — would close it.
8. Residual to write down: [(,] is the broadest cue in the lexicon and the one entry missing from the description
The strict count-parity rule is only as good as the cue list's precision, and the two-occurrence shape is where a false claim costs a section:
["Cut infra spend, 2000 servers decommissioned.", "Retired 2000 legacy VMs."]
-> ["Cut infra spend and retired 2000 legacy VMs."] main: ok=true here: dropped=["2000"]
["Rebuilt checkout (2000 SKUs).", "Audited 2000 SKUs."] same shape via `(`
["Shipped at Acme Corp warehouse 2000 pallets.", "Moved 2000 pallets."] same via `at\s+(word\s+)+`
I asked for the ,/( cues in round 2, so I am not asking you to rip them out — findings 1, 2 and 5 remove most of the paths into this. What I would like is the residual recorded: [(,] is absent from the description's otherwise complete enumeration of what YEAR_PREFIX_CUE matches, and nothing in the suite pins the two-occurrence configuration that the new strict rule actually governs. A line in ## Residual / Accepted Trade-off plus one test would close #876's audit requirement honestly.
Nits
9. Unrelated Prettier reflow in the test file
Four hunks (~197, ~226, ~385) are pure formatting on untouched tests — harmless, but they widen the round-2-vs-round-3 diff.
Gates
| Gate | Result |
|---|---|
/code-review generic pass (high) |
run — independently reproduced findings 4, 5, 8 and contributed 1, 3, 6, 7, each re-verified here against origin/main before inclusion |
| 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 — "No issues in 2 changed files"; the lone complexity finding is inherited, not attributed to this diff |
| 3e skill & script command review | skipped — no scripts/** or SKILL.md change |
| 3f description accuracy | one misdescription (finding 5), one omitted cue (finding 8), one docblock claim contradicted by the code (finding 3); ## Residual / Accepted Trade-off is otherwise accurate and is exactly what round 2 asked for; npm run verify exit 0 confirmed locally |
| ReDoS check on the new regexes | pass — the nested (?:[A-Za-z0-9&.'-]+\s+)+ is safe: its character classes are disjoint from \s, so the split is deterministic (200k matches on a 32-char worst case in 26 ms) |
Acceptance criteria — #876
| AC | Verdict |
|---|---|
Context gate on year mirroring headcount's verb/noun check |
met — isYearContext, same window/prefix/follow shape |
Issue's repro flips to dropped: ["1900"] |
met — verified |
| Residual "does NOT extend to year" test updated | met |
| "Needs its own false-positive audit" | not met — the audit is real and it cleared round 2's regressions, but it does not cover the two-occurrence configuration the new strict rule governs, the add side (finding 3), or the positional instability in finding 2. Eight verified regressions against main survive it. |
The trajectory here is good — the invention-side split was the right call and it cleared seven regressions at once, and the corpus went from 10 untracked years to 1. Findings 4 and 5 are free. The substantive work is 1, 2 and 3.
Reviewed by: Claude Opus 5 (high)
| "(?:\\b(?:" + | ||
| "in|since|during|until|through|between|before|after|around|circa|c\\.|" + | ||
| "as\\s+of|class\\s+of|cohort\\s+of|batch\\s+of|" + | ||
| "awarded|certified|earned|completed|launched|published|promoted|shipped|graduated|founded|established|joined|" + |
There was a problem hiding this comment.
Blocking 1. These verbs are anchored to )\s*$, so the verb only has to be the last token before the digit — which is the shape of a bullet counting things, not dating one. Verified against origin/main:
["Shipped 2000 features.", "Reviewed 2000 PRs."] -> ["Reviewed 2000 PRs and features."]
main: ok=true here: ok=false dropped=["2000"]
Same for Completed 2000 tickets and Launched 2000 campaigns. 2000 is present verbatim in the output; the section rewrite is discarded anyway.
I asked for this verb family in round 2, but the form I wanted recovered (Shipped the platform 2018) is already carried by the verb + determiner + noun + year branch on line 321. Check whether this bare arm can just go — or make it require a date-ish token rather than any 4-digit number.
| "winner,?\\s*|won,?\\s*|recipient\\s+of(?:\\s+the)?|speaker\\s+at|talk\\s+at|presented\\s+at|worked\\s+at|" + | ||
| "(?:awarded|certified|earned|completed|launched|published|promoted|shipped|graduated|founded|established|joined|built|deployed|released|delivered|led)\\s+(?:the|a|an|our|this)\\s+[A-Za-z0-9&.'-]+\\s+|" + | ||
| "(?:employee|person|engineer|team|member|volunteer)\\s+of\\s+the\\s+year\\s+|" + | ||
| "(?:certified\\s+[A-Za-z0-9&.'-]+|architect|specialist|developer|administrator|practitioner|associate|professional|expert|master)\\s+|" + |
There was a problem hiding this comment.
Secondary 5. | binds across the whole group, so only the first arm is prefixed by certified — architect, developer, master, expert, associate, professional, specialist, administrator and practitioner each claim a year standalone. Senior developer 2000 hours logged, Lead architect 2000 tickets triaged, Master 2000 records reconciled all become year claims.
The PR description calls this cue "credentials (certified architect)", which is the narrower thing. I deleted the bare arms and all 82 tests still pass — AWS Certified Architect 2019 is carried entirely by the certified <word> arm:
"(?:certified\\s+[A-Za-z0-9&.'-]+)\\s+|" +
If the bare roles are wanted, the group probably meant certified\s+(?:[A-Za-z0-9&.'-]+|architect|specialist|…).
| "(?:employee|person|engineer|team|member|volunteer)\\s+of\\s+the\\s+year\\s+|" + | ||
| "(?:certified\\s+[A-Za-z0-9&.'-]+|architect|specialist|developer|administrator|practitioner|associate|professional|expert|master)\\s+|" + | ||
| "at\\s+(?:[A-Za-z0-9&.'-]+\\s+)+|" + | ||
| "[A-Z][A-Za-z0-9]*(?:Con|Conf|Summit|Meetup|Expo|Fest|Symposium|Workshop|Awards?)|" + |
There was a problem hiding this comment.
Secondary 4. This regex is built with the "i" flag, which makes [A-Z] equivalent to [A-Za-z] and case-folds the suffix list. The branch reads as "a CamelCase conference/product name" but compiles to "any word ending in con/conf/fest/summit/awards/…":
DevCon: true KubeCon: true <- intended
silicon: true Falcon: true beacon: true lexicon: true <- not
So Migrated silicon 2000 wafers and Deployed on Falcon 2000 hardware classify a quantity as a year. JS has no inline case-sensitivity modifier, so this needs its own regex — verified 82/82 green:
/** Case-SENSITIVE: CamelCase conference/product names. Kept out of
* YEAR_PREFIX_CUE because that regex is /i, which makes [A-Z] match a-z. */
const YEAR_PREFIX_CUE_CAMEL =
/\b[A-Z][A-Za-z0-9]*(?:Con|Conf|Summit|Meetup|Expo|Fest|Symposium|Workshop|Awards?)\s*$/;…then drop this line and test YEAR_PREFIX_CUE.test(before) || YEAR_PREFIX_CUE_CAMEL.test(before).
| "spring|summer|fall|autumn|winter|" + | ||
| "q[1-4]|h[1-2]|fy" + | ||
| ")\\s*|" + | ||
| `[(,]\\s*|` + |
There was a problem hiding this comment.
Secondary 8. A bare , or ( before the digit is the broadest cue in the lexicon, and it is the one entry missing from the description's enumeration of what YEAR_PREFIX_CUE matches.
I asked for it in round 2 (to recover B.S. …, 2019 and PyCon (2019)), so I am not asking you to remove it — but its residual is real and should be written down:
["Cut infra spend, 2000 servers decommissioned.", "Retired 2000 legacy VMs."]
-> ["Cut infra spend and retired 2000 legacy VMs."]
main: ok=true here: ok=false dropped=["2000"]
A line in ## Residual / Accepted Trade-off plus one test pinning this two-occurrence shape — which nothing in the suite currently exercises — would close #876's audit requirement.
| ")\\s*|" + | ||
| `[(,]\\s*|` + | ||
| `(?:\\b(?:19\\d\\d|20\\d\\d)\\s*(?:${RANGE_DASH.source}|/|to|and)\\s*)|` + | ||
| `(?:\\b\\d{1,2}\\s*[/.-]\\s*))$`, |
There was a problem hiding this comment.
Secondary 6. This branch is tested before isRangeEndpoint (line 445), and the - in [/.-] means it swallows a range whose left endpoint is 1–2 digits. Strictness then depends on the left endpoint's digit count for an otherwise identical construct:
["Processed 50-2000 tickets.", "Closed 2000 escalations."] -> ["Processed 50 to 2000 tickets."] ok=false dropped=["2000"]
["Processed 1000-2000 tickets.", "Closed 2000 escalations."] -> ["Processed 1000 to 2000 tickets."] ok=true
main is clean on both. The new "treats range endpoints uniformly across the 1900-2099 boundary" test only covers the 1000- variant, which is why it passes. Restricting this branch to / and . and leaving - to range detection looks like the fix.
| if (year < 1900 || year > 2099) return false; | ||
|
|
||
| const matchStart = match.index; | ||
| const before = bullet.slice( |
There was a problem hiding this comment.
Blocking 2. This fixed-width slice routinely cuts through a word, and every alternative in YEAR_PREFIX_CUE is \b-anchored — but the cut itself fabricates the boundary, so at\s+(?:word\s+)+$ fires on the tail of any word ending in at. Same sentence, same trailing phrase, only the padding differs:
CLAIMED "Team that absorbed a 4x surge during a 2024 launch." window="at absorbed a 4x surge during a "
CLAIMED "Team great absorbed a 4x surge during a 2024 launch." window="at absorbed a 4x surge during a "
CLAIMED "Team format absorbed a 4x surge during a 2024 launch." window="at absorbed a 4x surge during a "
unclaimed "Team XX absorbed a 4x surge during a 2024 launch." window="XX absorbed a 4x surge during a "
A cue's verdict becomes a function of the digit's absolute offset, so a case that passes as a test can fail in prose that says the same thing. Suggested:
const rawStart = Math.max(0, matchStart - YEAR_CONTEXT_WINDOW);
const before =
// a window that cuts mid-word fabricates a \b; drop the partial token
rawStart > 0 && /\w/.test(bullet[rawStart - 1] ?? "")
? bullet.slice(rawStart, matchStart).replace(/^\S*/, "")
: bullet.slice(rawStart, matchStart);The after slice wants the mirror (trim a trailing partial token). The headcount window has the same latent issue (compelled → led) — pre-existing, but one shared helper covers both.
| .slice(0, matchStart) | ||
| .trim() | ||
| .replace(/^[-*•⁃–—\s]+/, ""); | ||
| if (leadingText.length === 0 && LEADING_DATE_ANCHOR_SEPARATOR.test(after)) { |
There was a problem hiding this comment.
Secondary 7. LEADING_DATE_ANCHOR_SEPARATOR accepts RANGE_DASH, so a bullet that opens with a numeric range reads as a date anchor:
["2000 - 3000 units delivered.", "Stocked 2000 SKUs."] -> ["Delivered units and stocked 2000 SKUs."]
main: ok=true here: ok=false dropped=["2000"]
Requiring the token after the dash to be a year or present/ongoing — which YEAR_FOLLOW_CUE already does — would close it.
| // Invention: the same lookup, except that a headcount the output asserts is | ||
| // "present" only if the input asserted that value too (rule 3). `phase 5` → | ||
| // `5 engineers` reuses the digit while making a claim the résumé never made. | ||
| // `year` remains lenient here (#876): an output year claim only needs the |
There was a problem hiding this comment.
Blocking 3. This comment says year "remains lenient" on the add side, but that is not what the code does. Leniency would mean an output year claim needs only the value present in the input. Because a non-temporal 4-digit is now claim: "none", it is never a candidate on the add side at all — so a fabricated year ships silently:
["Delivered units to production."] -> ["Delivered 2000 units to production."]
main: added: ["2000"] here: ok=true
["Received the Excellence Award."] -> ["Received the 2019 Excellence Award."]
main: added: ["2019"] here: ok=true
["Worked on the platform."] -> ["Worked on the platform in 2019."]
main: added: ["2019"] here: added: ["2019"] (still caught — has a cue)
The third row is the case this comment describes; the first two are the gap. A model inventing a credential year onto an award line is realistic, and catching invented facts is the guardrail's core job. eval/rubric.ts re-derives numbersPreservedRate from this function, so the loss is invisible in the metric that would surface it.
Either defend an output 4-digit whose value is absent from the input regardless of claim, or record it as an explicit accepted residual — but the comment should describe what the code does.
|
Thank you for the thorough, precise review and for identifying these subtle edge cases. All blocking, secondary, and residual items have been addressed in commit 1. Bare Past-Tense Verbs Removed (Blocking 1)
2. Positional Stability in Context Window Slicing (Blocking 2)
3. Add-Side Defense of Invented 4-Digit Years / Quantities (Blocking 3)
4. Case-Sensitive CamelCase Conference Names (Secondary 4)
5. Narrowed Credential Prefix Binding (Secondary 5)
6. Date Slash / Range Separation (Secondary 6)
7. Leading Date Anchor Separator (Secondary 7)
8. Documented Accepted Residual & Added Test (Residual 8)
9. Verification
|
Acceptance Criteria Update: "Needs its own false-positive audit"To explicitly close the remaining "not met" Acceptance Criterion from the review:
All 8 identified regression paths against |
s-annam
left a comment
s-annam
left a comment
There was a problem hiding this comment.
Round 4, reviewed at 5388178. Every finding from round 3 is genuinely fixed, and I re-ran each one before saying so:
| Round 3 finding | Status |
|---|---|
| B1 — bare past-tense verbs claim ordinary quantities | fixed — Shipped/Completed/Launched 2000 <noun> all classify none |
B2 — 32-char window slices mid-word, fabricating \b |
fixed — swept that/great/format/combat plus 9 other tails across 46 prefix offsets each; every one is now positionally stable |
| B3 — add side does not defend invented years | fixed — ["Received the Excellence Award."] → ["Received the 2019 Excellence Award."] now returns added: ["2019"] |
S4 — [A-Z] inert under /i |
fixed — silicon/Falcon no longer cue, KubeCon still does |
| S5 — credential alternation binds standalone role nouns | fixed — Senior Architect 2019 no longer cues |
S6 — \d{1,2}[/.-] makes 50-2000 strict, 1000-2000 lenient |
fixed |
S7 — leading anchor accepts a bare RANGE_DASH |
fixed, but over-corrected — see Secondary 2 |
S8 — [(,] residual |
now pinned by a test and disclosed in the description |
contextSliceBefore / contextSliceAfter are the right shape for B2. The is4DigitYearValue widening on the add side is also a good call: I went looking for a case where it turns an otherwise-ok: true section into a false revert and could not construct one — every candidate (2k→2000, $2000→2000, 2,000→2000) either already reverted on the drop side or normalises to the same key. It adds a second symptom to an existing failure; it never creates a new one.
What is left is one claim in the description the code does not deliver, and two coverage regressions.
Blocking
1. No realistic award or certification name reaches the cue that is supposed to defend it — while ## Summary says those forms are defended.
Two independent causes, and fixing either one alone is not enough.
(a) The arms cap at exactly one word. preserve-numbers.ts:328 and :330 both end in a single [A-Za-z0-9&.'-]+\s+ anchored to $. The at\s+(?:[A-Za-z0-9&.'-]+\s+)+ arm two lines below puts + on the group and does span multi-word names; these two do not.
(b) YEAR_CONTEXT_WINDOW = 32 truncates the cue even when the arm would match. "AWS Certified Solutions Architect " is 34 characters and "Google Cloud Certified Professional Cloud Architect " is 51, so contextSliceBefore correctly discards the partial leading token — and certified goes with it.
Verified on 5388178, all returning claim: "none", which means a rewrite that deletes the year scores ok: true and ships:
none "Awarded the Excellence Award 2019"
none "Won the Innovation Prize 2019"
none "Received the Chairman Award 2021"
none "AWS Certified Solutions Architect 2019"
none "Certified Kubernetes Administrator 2021"
none "Google Cloud Certified Professional Cloud Architect 2019"
YEAR "Awarded the Award 2019" <- the only award shape that matches
YEAR "AWS Certified Architect 2019" <- the only credential shape that matches
The two that work are exactly the two the new tests assert, which is what makes this invisible: "narrows credential prefixes so generic nouns do not claim years" pins AWS Certified Architect 2019, and nothing covers the multi-word form every real credential actually has. Award names are essentially never one word.
It is Blocking on the description as well as on the coverage. ## Summary states that `[Action verb] the [Noun] [Year]` and `certified <word>` "defend genuine attributive and credential year forms". For genuine forms they do not.
A fix, which I applied locally and verified: 90/90 in preserve-numbers.test.ts still green, every decoy still rejected. Three changes together:
// 1. new arm, next to the "employee of the year" one — anchor on the award noun
// rather than counting words after a determiner
"(?:awards?|prizes?|medals?|fellowships?|scholarships?|honou?rs?|certifications?|certificates?)\\s+|" +
// 2. replace the certified arm — bound the run, and keep determiners out
"(?:certified\\s+(?!(?:the|a|an|our|this)\\b)(?:[A-Za-z0-9&.'-]+\\s+){1,4})|" +
// 3. the window has to hold a full credential name
const YEAR_CONTEXT_WINDOW = 64;After: Awarded the Excellence Award 2019 ✅, Won the Innovation Prize 2019 ✅, Received the Chairman Award 2021 ✅, AWS Certified Solutions Architect 2019 ✅, Certified Kubernetes Administrator 2021 ✅, Google Cloud Certified Professional Cloud Architect 2019 ✅ — and still none for Delivered 2000 units to production., Shipped 2000 features., Certified the 2000 units shipped., Senior Architect 2019 promotion, Reduced infra cost across every regional cluster by 2000 hours annually., Migrated the entire billing platform and shipped 2000 features.
Arm (1) also stops needing the verb list: Won the Innovation Prize 2019 matches even though bare won is not in the alternation. Raising the window to 64 is what makes (2) reach past the truncation — I checked that the longer window does not pull in a spurious cue on the merge decoys above, but it is the change worth a second look, so please re-run the whole suite rather than trusting my one file.
Secondary
2. 2019 – Led the platform migration lost its date anchor (preserve-numbers.ts:301).
Round 3's S7 was that the leading anchor accepted a bare RANGE_DASH, so 2000 - 3000 units read as a date. Requiring a companion year after the dash fixes that, but it also drops the dash-then-prose bullet anchor, which is an ordinary résumé line:
none "2019 - Led the platform migration"
none "2019 – Led the platform migration"
none "• 2019 - Led the platform migration"
YEAR "2019: Led the platform migration" <- the colon form still works
That is a regression against both main and 6af63d4, and nothing in the suite covers it — the new test only asserts the false-positive direction.
The distinguishing signal is whether a number follows the dash, not whether a year does. One extra alternative keeps both behaviours (verified, 90/90 green):
`^\\s*(?:[:.)]` +
`|(?:${RANGE_DASH.source}\\s*(?:(?:19\\d\\d|20\\d\\d)|present|current|now|ongoing)\\b)` +
`|(?:${RANGE_DASH.source}(?!\\s*\\d)))\\s*`- Led the platform migration ✅, – Led the platform migration ✅, - 2021: Lead Architect ✅, – Present ✅, : Founded ✅; still rejects - 3000 units delivered ❌, -3000 units ❌, - 12 people trained ❌. The lookahead has to sit tight against the dash — RANGE_DASH\s*(?!\d) backtracks \s* to empty and matches - 3000 anyway.
3. Deleting the whole bare-verb family took the safe verbs with the dangerous ones (preserve-numbers.ts:327).
My round-3 B1 was right about the failure, but the fix went wider than it had to. These regressed against main and are now unclaimed, so a rewrite can silently delete the year:
none "Graduated 2019 from MIT"
none "Founded 2019 and scaled it"
none "Awarded 2019 for excellence"
none "Promoted 2019 to staff engineer"
The discriminator is what follows the digit, and the file already has it: FUNCTION_WORD_FOLLOWS, the helper bareIntegerClaim uses to stop a management verb claiming a headcount when a non-people noun follows. The same test separates these two families:
count "Shipped 2000 features." count "Completed 2000 tickets."
count "Launched 2000 campaigns." count "Tracked 2000 leads and campaigns."
YEAR "Graduated 2019 from MIT" YEAR "Founded 2019 and scaled it"
YEAR "Awarded 2019 for excellence" YEAR "Promoted 2019 to staff engineer"
10 of the 11 phrasings I tried land correctly; the miss is Joined 2019 as engineer number 4, since as is not in the list. So: restore the bare-verb alternation, gated on FUNCTION_WORD_FOLLOWS.test(after). That reuses a constant already in the file and is the same prefix-verb-plus-suffix-check shape headcount uses.
This one is genuinely optional — it is a coverage trade #876 explicitly licenses ("falling back to unclaimed otherwise"). I raise it because the fix is cheap and reuses something already there, not because the current state is wrong.
Nits (non-blocking)
4. YEAR_PREFIX_CUE's docblock still omits the [(,] arm (preserve-numbers.ts:319). The PR body documents it well; the code comment enumerates "temporal prepositions, verbs, credential/award phrases, month names, seasons, quarters, and range connectors" and stops. A bare ( or , is the most surprising arm in the regex and the one carrying the pinned residual — it should be the one the docblock calls out.
5. fallow flags classifyAtom at CRAP 37.1 / cyclomatic 11 (preserve-numbers.ts:490). Report-only inside verify and not a blocker per CLAUDE.md; noted only because the year branch is what pushed it over.
Gates
| Gate | Result |
|---|---|
| 3a fixture PII | skipped — no fixtures in the diff |
| 3b design-system / reuse | skipped — no component files |
| 3c style tokens | pass (the only grep hits are #876 matching the hex pattern) |
| 3d fallow | pass — 1 complexity finding, report-only (Nit 5) |
| 3e skill/script commands | skipped — no scripts/** or skill files |
| 3f description accuracy | one overclaim (Blocking 1); otherwise accurate and unusually specific — the [(,] residual, the certified <word> wording and the leading-anchor mechanics all match the code exactly |
npm run verify |
exit 0 locally (26 files / 445 tests via test:changed); CI verify and fallow both green |
Acceptance criteria — #876
- Context gate for
year— met (isYearContext, gated on prefix / follow / anchor cues). - Repro flips — met;
"Founded the program in 1900." + "Operated out of suite 1900."now reports the dropped1900. - Its own false-positive audit — met this round. Nine tests added covering each round-3 finding, and I independently swept 13 phrasings across 46 prefix offsets apiece with no instability. The one surviving false revert (comma cue, two same-value occurrences) is pinned by a test and disclosed in the body — which is what closing an audit looks like, as opposed to the absence of one.
Process note
The branch now carries two commits (6af63d4 + 5388178). It is a fork, so I will not force-push it — but it needs collapsing to one commit before it reaches the merge queue, or main keeps the round-3 fix message as a bullet forever. Either amend into one commit and force-push your own fork, or whoever merges squashes deliberately.
Reviewed by: Claude Opus 4.8 (high)
| "in|since|during|until|through|between|before|after|around|circa|c\\.|" + | ||
| "as\\s+of|class\\s+of|cohort\\s+of|batch\\s+of|" + | ||
| "winner,?\\s*|won,?\\s*|recipient\\s+of(?:\\s+the)?|speaker\\s+at|talk\\s+at|presented\\s+at|worked\\s+at|" + | ||
| "(?:awarded|certified|earned|completed|launched|published|promoted|shipped|graduated|founded|established|joined|built|deployed|released|delivered|led)\\s+(?:the|a|an|our|this)\\s+[A-Za-z0-9&.'-]+\\s+|" + |
There was a problem hiding this comment.
Blocking 1(a) — this arm ends in a single [A-Za-z0-9&.'-]+\s+ anchored to $, so exactly one word may sit between the determiner and the year. Every realistic award name has more:
none "Awarded the Excellence Award 2019"
none "Won the Innovation Prize 2019"
none "Received the Chairman Award 2021"
YEAR "Awarded the Award 2019" <- the only shape that matches
claim: "none" means a rewrite that deletes the year scores ok: true and ships. The at\s+(?:[A-Za-z0-9&.'-]+\s+)+ arm three lines below already uses + on the group and does span multi-word names.
Rather than counting words, anchor on the award noun — it catches all three above, rejects the merge decoys (Delivered the , Delivered the first batch of , Shipped the new , Completed the 2nd wave of ), and stops needing the verb list at all (Won the Innovation Prize 2019 matches even though bare won is not in the alternation). Add next to the employee of the year arm:
"(?:awards?|prizes?|medals?|fellowships?|scholarships?|honou?rs?|certifications?|certificates?)\\s+|" +Verified together with the other two changes in the body: 90/90 in preserve-numbers.test.ts still green.
| "winner,?\\s*|won,?\\s*|recipient\\s+of(?:\\s+the)?|speaker\\s+at|talk\\s+at|presented\\s+at|worked\\s+at|" + | ||
| "(?:awarded|certified|earned|completed|launched|published|promoted|shipped|graduated|founded|established|joined|built|deployed|released|delivered|led)\\s+(?:the|a|an|our|this)\\s+[A-Za-z0-9&.'-]+\\s+|" + | ||
| "(?:employee|person|engineer|team|member|volunteer)\\s+of\\s+the\\s+year\\s+|" + | ||
| "(?:certified\\s+[A-Za-z0-9&.'-]+)\\s+|" + |
There was a problem hiding this comment.
Blocking 1(a), second arm — same one-word cap. Real certification names are three to five words:
none "AWS Certified Solutions Architect 2019"
none "Certified Kubernetes Administrator 2021"
none "Google Cloud Certified Professional Cloud Architect 2019"
YEAR "AWS Certified Architect 2019" <- the only shape that matches
The new test "narrows credential prefixes so generic nouns do not claim years" pins exactly that last line, which is what hides this: the assertion passes on the one shape that works.
Bound the run instead of fixing it at one, and keep determiners out so Certified the 2000 units shipped. stays unclaimed:
"(?:certified\\s+(?!(?:the|a|an|our|this)\\b)(?:[A-Za-z0-9&.'-]+\\s+){1,4})|" +This alone is not sufficient — see the window comment on YEAR_CONTEXT_WINDOW.
| * Context window (in characters) scanned before and after a 4-digit number | ||
| * when checking for year context cues (#876). | ||
| */ | ||
| const YEAR_CONTEXT_WINDOW = 32; |
There was a problem hiding this comment.
Blocking 1(b) — 32 characters is shorter than the credential names the cue on line 330 is meant to match, so widening that arm alone does not fix it. "AWS Certified Solutions Architect " is 34 characters and "Google Cloud Certified Professional Cloud Architect " is 51; contextSliceBefore then (correctly) discards the truncated leading token, and certified goes with it.
With the arm widened but the window left at 32, AWS Certified Solutions Architect 2019 starts matching but Certified Kubernetes Administrator 2021 and the Google one still do not. At 64 all three do.
I checked that the longer window does not pull in a spurious cue on the merge decoys — Reduced infra cost across every regional cluster by 2000 hours annually. and Migrated the entire billing platform and shipped 2000 features. both stay none — and the file's 90 tests stay green. Still, this is the change in the set most worth a second look, since it widens what every cue can see, not just the credential one. Please re-run the full suite rather than trusting my single file.
| * a year (1900-2099) or ongoing marker so numeric ranges like `2000 - 3000 units` do not match. | ||
| */ | ||
| const LEADING_DATE_ANCHOR_SEPARATOR = new RegExp( | ||
| `^\\s*(?:[:.)]|(?:${RANGE_DASH.source}\\s*(?:(?:19\\d\\d|20\\d\\d)|present|current|now|ongoing)\\b))\\s*`, |
There was a problem hiding this comment.
Secondary 2 — this correctly kills my round-3 finding (2000 - 3000 units reading as a date anchor), but it also drops the dash-then-prose bullet anchor, which is an ordinary résumé line:
none "2019 - Led the platform migration"
none "2019 – Led the platform migration"
none "• 2019 - Led the platform migration"
YEAR "2019: Led the platform migration" <- the colon form still works
A regression against both main and 6af63d4, and uncovered — the new test only asserts the false-positive direction.
The signal that separates them is whether a number follows the dash, not whether a year does. One more alternative keeps both:
`^\\s*(?:[:.)]` +
`|(?:${RANGE_DASH.source}\\s*(?:(?:19\\d\\d|20\\d\\d)|present|current|now|ongoing)\\b)` +
`|(?:${RANGE_DASH.source}(?!\\s*\\d)))\\s*`Verified: accepts - Led the platform migration, – Led the platform migration, - 2021: Lead Architect, – Present, : Founded; still rejects - 3000 units delivered, -3000 units, - 12 people trained. 90/90 green.
One trap worth naming: the lookahead has to sit tight against the dash. RANGE_DASH\s*(?!\d) backtracks \s* to empty and matches - 3000 anyway.
| "(?:\\b(?:" + | ||
| "in|since|during|until|through|between|before|after|around|circa|c\\.|" + | ||
| "as\\s+of|class\\s+of|cohort\\s+of|batch\\s+of|" + | ||
| "winner,?\\s*|won,?\\s*|recipient\\s+of(?:\\s+the)?|speaker\\s+at|talk\\s+at|presented\\s+at|worked\\s+at|" + |
There was a problem hiding this comment.
Secondary 3 — the bare past-tense verb alternation that used to sit here is gone, which fixes my round-3 blocker but takes the safe verbs with the dangerous ones. These regressed against main and are now unclaimed, so a rewrite can silently delete the year:
none "Graduated 2019 from MIT"
none "Founded 2019 and scaled it"
none "Awarded 2019 for excellence"
none "Promoted 2019 to staff engineer"
The discriminator is what follows the digit, and the file already has it — FUNCTION_WORD_FOLLOWS, which bareIntegerClaim uses to stop a management verb claiming a headcount when a non-people noun follows. The same test splits these two families:
count "Shipped 2000 features." count "Completed 2000 tickets."
count "Launched 2000 campaigns." count "Tracked 2000 leads and campaigns."
YEAR "Graduated 2019 from MIT" YEAR "Founded 2019 and scaled it"
YEAR "Awarded 2019 for excellence" YEAR "Promoted 2019 to staff engineer"
10 of the 11 phrasings I tried land correctly; the miss is Joined 2019 as engineer number 4, since as is not in the list. So: restore the alternation, gated on FUNCTION_WORD_FOLLOWS.test(after) — reusing a constant already in the file, in the same prefix-verb-plus-suffix-check shape headcount uses.
Genuinely optional. This is a coverage trade #876 licenses ("falling back to unclaimed otherwise"); I raise it because the fix is cheap and already sitting in the file, not because the current state is wrong.
| * Temporal prepositions, verbs, credential/award phrases, month names, | ||
| * seasons, quarters, and range connectors that signal a 4-digit number (1900–2099) |
There was a problem hiding this comment.
Nit 4 (non-blocking) — the enumeration stops before the [(,] arm, which is the most surprising thing in this regex and the one carrying the pinned residual. The PR description explains it well; the docblock a future reader lands on does not.
| * Temporal prepositions, verbs, credential/award phrases, month names, | |
| * seasons, quarters, and range connectors that signal a 4-digit number (1900–2099) | |
| * Temporal prepositions, verbs, credential/award phrases, month names, | |
| * seasons, quarters, range connectors, and a bare `(` or `,` — the last of | |
| * these defends `B.S. CS, 2019` and `Speaker (2019)`, at the cost of the | |
| * residual pinned by the "comma/parenthesis cues across two occurrences" | |
| * test. All signal a 4-digit number (1900-2099) |
s-annam
left a comment
s-annam
left a comment
There was a problem hiding this comment.
Addendum to my review above — two things a second pass turned up after I posted. Neither changes the verdict; both sit next to findings already in that review.
Also worth recording, since it is the kind of thing that is easy to worry about and hard to disprove: the widened add-side candidacy (isClaimed(atom) || is4DigitYearValue(atom)) was checked independently a second time and is clean. Every atom the OR clause newly makes a candidate was already a candidate on main, where the year classification was unconditional — so it cannot introduce a false added that main did not already produce. Confirmed against "Completed module 1920 training.", which behaves identically on both.
Reviewed by: Claude Opus 4.8 (high)
| return true; | ||
| } | ||
|
|
||
| // 3. Leading date anchor at the start of a bullet (e.g. "2019: Founded company", "• 2019 - Started role") |
There was a problem hiding this comment.
Secondary 2, additional evidence — this comment's second example does not work. • 2019 - Started role is exactly the dash-then-prose form LEADING_DATE_ANCHOR_SEPARATOR (line 301) stopped accepting, so the line that documents the behaviour is the clearest statement that it is missing:
checkNumbersPreserved(
["• 2019 - Started role as senior engineer."],
["Started role as senior engineer."],
)
// HEAD: { ok: true, dropped: [] } <- the year is gone and nothing reports it
// origin/main: { ok: false, dropped: ["2019"] }
// control, the colon example in the same comment, works on both:
checkNumbersPreserved(
["2019: Founded company and grew it fast."],
["Founded company and grew it fast."],
) // { ok: false, dropped: ["2019"] }The fix in the line-301 comment restores it. Whichever way you go, this comment and the regex need to agree — right now the comment is the spec and the code is not meeting it.
There was a problem hiding this comment.
Acknowledged and resolved in commit 7131640. LEADING_DATE_ANCHOR_SEPARATOR now includes the lookahead (?:${RANGE_DASH.source}(?!\s*\d)), successfully restoring dash-then-prose bullet anchors (including • 2019 - Started role as senior engineer.) while continuing to reject numeric ranges (2000 - 3000 units). Dedicated unit tests asserting both forms have been added in preserve-numbers.test.ts.
| * Connectives, qualifiers, range markers, or month/season names that signal | ||
| * a 4-digit number is a year when appearing immediately after the digit (#876). | ||
| */ | ||
| const YEAR_FOLLOW_CUE = new RegExp( |
There was a problem hiding this comment.
Secondary 4 (new, non-blocking) — a year leading an award name is undefended, which is the mirror image of the Blocking finding on line 328 and not covered by the residual the description declares.
checkNumbersPreserved(
["2019 Excellence Award recipient for outstanding performance."],
["Recognized for outstanding performance."],
) // { ok: true, dropped: [] } <- silently drops the year## Summary uses "2019 Excellence Award" as its worked example of the year logic, so it is a slightly awkward one to leave unclaimed. This is not the documented determiner trade — there is no the/a/this in front of it; the bullet simply opens with the year, and case 3 wants a :/./)/dash separator that an attributive year does not have.
YEAR_PREFIX_CUE gets the noun-before-year direction (once line 328 is fixed); this is the noun-after-year direction, so it belongs here. A case-sensitive TitleCase follow cue is narrow enough to avoid the obvious decoys:
/^(?:\s+[A-Z][A-Za-z0-9&.'-]*)*\s+(?:Award|Prize|Medal|Fellowship|Scholarship|Honou?r)s?\b/Matches Excellence Award recipient, Innovation Prize winner, Founders Award, Award recipient; rejects certificates issued, awards granted (lowercase), units delivered, Certified Solutions Architect. The one cost worth naming: a TitleCase 2000 Awards would read as a year. That seems a fair trade to me, but it is your call, and I have not run this one against the suite — unlike the three fixes in the main review, treat this as a sketch.
5388178 to
7131640
Compare
|
All items from round 4 review have been resolved in squashed commit 1. Multi-Word Awards & Credential Coverage (Blocking 1)
2. Dash-Then-Prose Bullet Date Anchors Restored (Secondary 2)
3. Safe Bare Verbs with Function Word Suffix Check (Secondary 3)
4. Docblock & Nits (Nit 4 & 5)
5. Squashed Commit
6. Verification
|
2 participants
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.year/ 4-digit years (1900–2099) defend against invented values while remaining lenient on additions: an output year claim only needs its numeric value present in the input in any form (e.g."2019 Excellence Award"->"Award in 2019"), preventing legitimate temporal rewording from being flagged as an invented year while catching model hallucinations of new 4-digit numbers.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], multi-word awards and credentials) defend genuine attributive and credential year forms.[(,]cues immediately preceding a 4-digit number are included to defend degrees and parenthesized talk years (e.g."B.S. CS, 2019","Speaker (2019)"). In the edge-case configuration where two occurrences of the same 4-digit quantity exist with a preceding comma and one is merged away without a year cue ("Cut infra spend, 2000 servers..."+"Retired 2000 legacy VMs"->"Cut infra spend and retired 2000 legacy VMs"), strict count parity reports a drop. This is an explicit accepted residual trade-off for degree and award defense.Changes
src/lib/webllm/preserve-numbers.ts:YEAR_CONTEXT_WINDOW = 64to accommodate full multi-word credential titles (e.g."Google Cloud Certified Professional Cloud Architect").contextSliceBeforeandcontextSliceAfterhelpers to eliminate artificial word boundary (\b) matches from partial word slicing when windows cut mid-word.awards?|prizes?|medals?|fellowships?|scholarships?|honou?rs?|certifications?|certificates?) toYEAR_PREFIX_CUE.(?:certified\s+(?!(?:the|a|an|our|this)\b)(?:[A-Za-z0-9&.'-]+\s+){1,4}).YEAR_PREFIX_VERB_CUEwithFUNCTION_WORD_FOLLOWScheck inisYearContextto defend safe past-tense verbs (Graduated 2019 from MIT,Founded 2019 and scaled it) while ignoring count quantities (Shipped 2000 features).YEAR_PREFIX_CUE_CAMEL: matches CamelCase conference names (DevCon,KubeCon) without lowercase false matches (silicon,falcon).LEADING_DATE_ANCHOR_SEPARATORto support dash-then-prose bullet anchors (2019 - Led the migration) and companion years/ongoing markers while rejecting numeric ranges (2000 - 3000 units).YEAR_FOLLOW_CUE: matches temporal qualifiers (onwards,present,current,now,ongoing), ranges (to <year>,until <year>,through <year>,and <year>), months, andRANGE_DASH//followed by companion years (1900–2099) or ongoing markers.missingFromandcheckNumbersPreservedto defend against invented 4-digit numbers (1900–2099) on the addition side while allowing lenient temporal rewords.src/lib/webllm/preserve-numbers.test.ts:Excellence Award 2019,Innovation Prize 2019,AWS Certified Solutions Architect 2019,Certified Kubernetes Administrator 2021,Google Cloud Certified Professional Cloud Architect 2019).2019 - Led the platform migration,2019 – Led the platform migration,• 2019 - Led the platform migration).Verification
npm run verifypassed cleanly: 383 test files passed, 6,471 / 6,471 tests passing.