feat(webllm): section-level rewrite + number-preservation guardrail (#63)

[Vaishnavi1709]

Summary

Phase 1 of the in-browser AI rewrite epic. Each RoleEntry now exposes a Rewrite section button that asks Qwen2.5-1.5B to rewrite the whole role's bullets at once — the model owns count/order (can merge, dedupe, drop filler, reorder), and a deterministic, model-free regex guardrail compares numeric tokens in vs. out so the UI can name the specific dropped/invented metric inline ("⚠ AI altered a metric — removed $5K…").

Closes #63

What's in the diff

New webllm modules (all unit-tested, no real model fetched in CI)

• src/lib/webllm/post-process.ts (+ test) — cleanRewriteLine factored out of rewrite-bullet.ts; strips Rewritten: echo, list markers, straight + smart quotes, paired bold/italic, prompt-echo lines.
• src/lib/webllm/preserve-numbers.ts (+ test) — single-pass ATOM regex classifies each numeric token exactly once. Handles $/€/£/¥, percent, magnitude (5K, 10MB, 2GB), comma groups, decimals, year ranges, and bare-integer headcounts in people-management context. Sign-flips (15% → -15%) are flagged. of deliberately NOT in the verb prefix (over-triggers on "1 of 5"). 22 tests.
• src/lib/webllm/rewrite-section.ts (+ test) — rewriteSectionWithLlm(bullets, engine) → SectionRewriteResult with max_tokens = min(60 × bullets.length, 768), post-processed output split on \n. 19 tests.

New / modified UI

• src/components/features/SectionRewrite.tsx — uses <Button> from @design-system (no raw <button>). Renders before/after side-by-side, green border when numbersPreserved, amber border + token-naming warning when not. Returns null when WebGPU is unavailable. Auto-dismisses the proposal when the underlying bullets change (so you can't see a "Original (2) | Proposed (3)" mismatch after an edit).
• src/hooks/useSectionRewriteLock.ts (+ test) — synchronous atomic tryAcquireSectionRewriteLock() returns null on contention. The disabled button is just UI; the real guarantee is this atomic check, so two onClick handlers fired in the same React batch can't both succeed. 6 tests.
• src/components/features/ReconstructedRole.tsx — wires one SectionRewrite per role, fed bulletOverrides?.[b.index] ?? b.text so the model sees what the user actually edited.

Foundational

• src/lib/webllm/web-llm.ts — MODEL_ID bumped Qwen2 → Qwen2.5-1.5B.
• src/lib/webllm/rewrite-bullet.ts — refactored to use cleanRewriteLine. No behavior change (9 existing tests still pass).
• src/lib/analytics.ts — webllm_section_rewrite_started, _completed (with numbers_preserved), plus a distinct webllm_first_section_rewrite one-shot key. Per-bullet webllm_first_rewrite left untouched per AC.

Bundled fix (App.tsx wiring)

Discovered while manually testing this PR: editing any bullet caused it to jump to the trailing "Other bullets" group, which blocked end-to-end verification of the guardrail. Root cause was in App.tsx — it passed state.result (original parsed) with edited.score (re-graded bullets), so groupBulletsByExperience built its line→exp map from original descriptions and looked up edited bullet text → no match → "Other bullets."

Fix is one line: pass { ...state.result, parsed: edited.parsed } to Result. rawText stays original on purpose (EvidencePanel shows what the PDF extracted, not what the user typed). Two regression tests in src/lib/edit/apply-overrides.test.ts — one proves the working path, the other pins the bug pattern so anyone "simplifying" the wiring back to state.result will see the test fail.

Why bundled rather than separate: the bug pre-dated this branch but the displacement made testing the guardrail end-to-end impractical (the edited bullet's $50K would land in a different group than the role I was running rewrite on). The fix is 3 lines of code + 2 tests, narrowly scoped, and the alternative was to ship #63 with a known testing gap.

Known scope notes (worth flagging to reviewer)

• No <Card> from @design-system for the inline before/after panel. That primitive owns top-level panel chrome (rounded-xl border p-5); the inline result strip uses the same lightweight container pattern as RewriteButton's RewriteResult (rounded border + feedback bg). Forcing Card would visually overweight the surface.
• rewriteSectionWithLlm returns SectionRewriteResult, not Promise<string[]>. The struct surfaces numbersPreserved/droppedNumbers/addedNumbers the warning UI needs; returning bare string[] would force the component to re-run the guardrail.
• Per-role wiring only. Project/achievement sections don't get SectionRewrite — matches the issue's "Wire into RoleEntry" wording. If you want them too, easy follow-up.

Test plan

• npm run typecheck clean
• npm run test green (523/523 tests, was 496)
• npm run build green
• Manually verified in npm run dev:
  • Per-role "Rewrite section" buttons render under each experience role; absent in Safari/Firefox (no WebGPU)
  • Cold-start downloads Qwen2.5-1.5B (~1.2GB) on first click only; cached in IndexedDB after
  • Clicking a sibling role's button while one is in flight shows "Another rewrite running…" disabled (lock works)
  • Successful rewrite shows green-bordered before/after with "Use this — copy all bullets" / "Discard"
  • Editing a bullet under a rewritten role auto-dismisses the proposal (no stale (2) | (3) mismatch)
  • Per-bullet sparkle ✨ rewrite still works (regression check on the refactored post-process)
  • Edited bullets stay attached to their role (App.tsx fix)

[s-annam]

PR Review: feat(webllm): section-level rewrite + number-preservation guardrail (#63)

Summary

High-quality Phase-1 implementation. Every acceptance criterion in #63 is met, logic is cleanly domain-segregated under src/lib/webllm/ with thorough unit tests (523/523 green), CI is fully green (fallow pass, verify pass), and the two intentional deviations from the issue's plan are documented in the PR body with sound reasoning.

Spec Alignment (issue #63)

Requirement	Status	Notes
MODEL_ID → Qwen2.5-1.5B	✅	web-llm.ts
post-process.ts shared, no dup; bullet path slices line[0], section keeps all	✅	Both call sites import cleanRewriteLine
rewrite-section.ts, max_tokens = min(60×N, 768)	✅	sectionMaxTokens + N=0 floor guard
preserve-numbers.ts model-free; currency/%/magnitude/commas/years/headcounts	✅	Single-pass ATOM regex; sign-flip detection; of correctly excluded
SectionRewrite.tsx states + null on no-WebGPU + inline ⚠ naming tokens	✅	Mirrors RewriteButton
Wire per-role into RoleEntry, keep per-bullet	✅	Fed bulletOverrides ?? text (#82-aware)
Telemetry: 3 section events + distinct webllm_first_section_rewrite	✅	Per-bullet key untouched
One-rewrite-at-a-time concurrency	✅	useSectionRewriteLock — synchronous atomic acquire, not just disabled

Highlights

• preserve-numbers.ts is the standout. Single ATOM-regex pass classifying each token exactly once cleanly avoids the year/date-range and verb-prefix/noun-suffix double-emit traps. Multiset diff semantics, sign-flip catch (15%→-15%), and the deliberate of exclusion show real care. 22 tests.
• The lock is correct for the right reason. Synchronous check-and-increment in one turn is the actual guarantee; the disabled button is correctly treated as just the (one-render-late) UI surface. Idempotent release in finally.
• Stale-proposal auto-dismiss compares content not identity (bulletsEqual), with a comment explaining why trimmedBullets is a fresh array each render.
• Bundled App.tsx fix is genuinely scoped (1 line + 2 regression tests) and the rawText-stays-original vs parsed-gets-edited split is deliberate and documented.

Key Findings (none blocking)

1. [Suggestion] Card deviation contradicts a written AC. Issue #63 step 5 says before/after "in a shared/Card"; the PR uses a lightweight rounded border strip instead, justified as matching RewriteButton's RewriteResult chrome. The reasoning is sound and consistent with the sibling — but it does override an explicit AC, so worth a team ✅ rather than a silent pass. Per CLAUDE.md, if Card lacks a lightweight variant, the canonical move is to add the variant, not bypass it.
2. [Suggestion] Component has no direct test → drives the fallow CRAP advisories. The 4 bot comments (CRAP 90/42/30/30) are advisory only — the fallow CI check passed — but they're all "complexity + zero coverage." A single renderToStaticMarkup smoke test (idle label, locked label, proposed panel with a dropped-number warning) would knock the score down and cover the labelFor/warning-format branches cheaply.
3. [Nit] "No behavior change" on rewrite-bullet.ts is slightly overstated. The old postProcess stripped straight quotes only; cleanRewriteLine now also strips smart quotes, bold/italic, list markers, and prompt-echo lines. Strictly more cleaning (an improvement, 9 tests still pass) — just not "no change."
4. [Nit] Branch is 1 commit behind main (off #119, main at #121). Rebase before merge if you want CI on the merged state.

Verdict

APPROVE — 0 blocking findings. Spec fully implemented, CI green, well-tested, deviations documented. The two suggestions (Card AC, component smoke test) are worth a follow-up but shouldn't gate merge.

[s-annam]

Re-review (fresh approval) — PR #123

All four findings from the first pass are resolved. Re-ran the gates on the rebased branch: typecheck clean, 535/535 tests (was 523), fallow + verify both green.

First-pass finding	Resolution
[Suggestion] Card AC deviation	Filed #124 for the team decision (recommends a shared InlineResult primitive, keeps Card panel-scoped); thread intentionally left open to land Option A vs B there. Right call — turns a silent deviation into a tracked decision.
[Suggestion] fallow CRAP advisories	SectionRewrite.test.ts added — 12 renderToStaticMarkup smoke tests covering labelFor (locked + per-status), formatTokens (1/2/N), NumberPreservationWarning (named tokens + role=alert), and ProposedSection (success/warning chrome, counts, copy-label flip). fallow now passes.
[Nit] "no behavior change" framing	Acknowledged; correctly no code change — strictly-more cleaning, covered by post-process.test.ts.
[Nit] 1 commit behind main	Rebased; branch is now current with main (0 behind).

The test-only exports on the helpers are documented with a clear motivation comment and are consumed by the new test (not flagged by fallow). Branch is MERGEABLE and up to date with main.

Verdict

APPROVE. Clean follow-through. Ship it.