feat: [edit] A parsed entry cannot be deleted from the reconstructed résumé — only user-ADDED ones (achievement, role, education, project) (#856)

[s-annam]

Implements #856: parsed entries (achievement, role, education, project) can now be deleted from the reconstructed résumé, not only user-added ones.

Design is the removedEntries tombstone set the issue specified — mark, run every index-keyed override pass, then filter once at the end of applyOverrides, so no override rebinds to a neighbour.

Deviations from the issue plan

Three deliberate departures from the issue's implementation plan, all additive to its intent:

1. NO removedEntriesRef (plan Step 1). The removedBulletsRef it mirrors exists solely so removeBullet can read the committed set to decide its boolean return; removeEntry returns void, so a ref beside removedEntries would have had no reader. Omitted rather than shipped dead.

2. ADDED a UI index-remap the plan does not mention, and it is load-bearing. The plan's Step 4 resolves the delete key as parsedEntryKey(<section>, i) where i is the render index. Because Step 3 filters tombstoned entries out of the parsed arrays, i stops being the parsed index after the first deletion — so that line would (a) re-delete the already-gone entry instead of the clicked one, and (b) leave every neighbouring onAchievementField(i, …) / educationOverrides[i] / descriptionOverrides key rebound to the wrong entry, which is the exact failure the tombstone design prevents one level down. survivingParsedIndices (exported from useEditableParse.ts) is the single map every section resolves through. It enumerates survivors rather than subtracting the set's size, so a stale tombstone naming an index the parse no longer has cannot over-shift.

3. The entry's own source-line removal (plan Step 3) reuses the SCORER's ownership relation rather than a new "normalise-and-match-first-line" matcher. normalizeTitleKey + stripAchievementLabel (already private in group-bullets.ts) are what suppressTitleOwnedBullets hides an entry-owned line on; isTitleOwnedLine exposes the inverse, sharing one extracted matchesOwnedKey so the two cannot drift. This is not cosmetic: a title-only achievement/project is parsed OUT of a •-marked line, so that line is in the graded pool with no description to attribute it to — deleting the entry without dropping the line leaves its content grading the résumé forever. It is best-effort and documented as such (the parsed model keeps no source-line provenance, so a glued one-line header matches nothing and the line stays in the raw-text view); safe when it misses, since the lines it can miss are the UNMARKED ones the bullet pool ignores.

Two smaller notes:

• Entry keys in all four sections moved from the render position to the entry key. A position key would hand a deleted row's in-flight edit state to its successor — newly reachable now that parsed rows can disappear.
• One drive-by, forced: ReconstructedRole.tsx:262 carried a pre-existing "exactly" inside a /** */ block, which scripts/hooks/check_conventions.py rule 2 blocks on (block comments are deliberately scanned) — it fired on every edit to that file. Reworded to "just as". Comment text only.

Also fixed while testing: parseRemovedEntryKey matches the index half as DIGITS rather than coercing with Number. Number("") is 0 and Number.isInteger(0) is true, so a truncated "projects:" key would have silently deleted the section's FIRST entry. Pinned by a test.

Files changed

• src/hooks/useEditableParse.ts
• src/hooks/useAnalyzedResume.ts
• src/lib/edit/apply-overrides.ts
• src/lib/score/group-bullets.ts
• src/components/features/EntryRemove.ts
• src/components/features/ReconstructedResume.tsx
• src/components/features/ReconstructedRole.tsx
• src/components/features/ReconstructedEducationSkills.tsx
• src/components/features/ExperienceSection.test.tsx
• src/components/features/ExperienceSection.other-bullets.test.tsx
• src/components/features/ExperienceSection.prune-hold.test.tsx
• src/components/features/ReconstructedEducationSkills.test.ts
• src/components/features/ReconstructedResume.remove-parsed-entry.test.tsx
• src/hooks/useEditableParse.removed-entry.test.tsx
• src/lib/edit/apply-overrides.removed-entry.test.ts
• src/lib/pdf/render-roundtrip-entry-remove.repro.test.ts

Validation

• npm run typecheck — pass — tsc -b --noEmit — no output, exit 0
• npm run lint — pass — eslint . — no output, exit 0
• npm test — pass — Test Files 363 passed | 8 skipped (371); Tests 5901 passed | 10 skipped (5911); Duration 19.77s

Adversarial review

Not yet run. The run halted at submit (a stray fake git remote made gh stack submit ambiguous), so the review pass never executed. Reviewer notes below are from npx fallow audit only.

Handoff

New exports the next issue can build on:

• useEditableParse.ts

  • EditableParse.removedEntries: ReadonlySet<string> — parsedEntryKey tombstones.
  • EditSnapshot.removedEntries?: string[] — OPTIONAL, so pre-[edit] A parsed entry cannot be deleted from the reconstructed résumé — only user-ADDED ones (achievement, role, education, project) #856 persisted drafts replay; replay defaults it to []. Any new consumer must keep it optional.
  • removeEntry(key: string) — widened from "added id" to "added id OR parsedEntryKey". Also deletes that key's addedBullets bucket for BOTH kinds.
  • survivingParsedIndices(section, removedEntries, renderedCount): number[] — render position → PARSED index. ANY new surface that renders a parsed section and writes an index-keyed override MUST resolve through this.

• lib/score/group-bullets.ts: isTitleOwnedLine(line, title) — the entry-owns-this-line relation, shared with suppressTitleOwnedBullets.

• components/features/EntryRemove.ts: removeEntryWithBullets(entryKey, bullets, {onRemoveEntry, onRemoveBullet}) — the one definition of the delete gesture. Bullets first, then the entry.

• applyOverrides gained an 18th positional param removedEntries: ReadonlySet<string> = new Set(). A 19th param must be appended AFTER it, and applyRemovedEntries must stay the LAST pass (it runs after applyAddedEntriesAndBullets) or index-keyed overrides rebind.

• AchievementsSection is now exported from ReconstructedResume.tsx (test-only, like ExperienceSection).

Two landmines documented in code, worth knowing:

• Inside ExperienceSection, the experience:<n> strings in rewriteApplyBySection are in TWO different index spaces — the map KEY is the rewrite-chain section id (RENDER position, matching buildResumeSections), while entryKey is the addedBullets bucket key (PARSED index). They coincide only until a parsed role is deleted.
• Achievement and project bullets are still read-only per-row (issue explicitly out of scope). Step 4 threaded onRemoveBullet into both sections for the delete cascade, so that follow-up is now mostly prop-threading down to ResumeBulletRow.

Not touched, per the issue's "Not in scope": per-bullet edit/remove on achievements/projects, an undo-delete toast, and the 18-param positional signature of applyOverrides.

Closes #856

[cloudflare-workers-and-pages]

Deploying offlinecv with    Cloudflare Pages

Latest commit:	3f08d0d
Status:	⚡️  Build in progress...

View logs

[s-annam]

Verdict: APPROVE

(Posted as a review comment because GitHub restricts PR authors from self-submitting formal approval reviews)

Reviewed against issue #856. All 10 acceptance criteria are fully met with comprehensive test coverage across 4 dedicated test suites. All offlinecv verification gates (check:fixtures, lint, typecheck, dead-code audit, and full test suite) are green.

Acceptance Criteria Checklist (#856)

• Parsed achievement deletion: Verified across re-renders (ReconstructedResume.remove-parsed-entry.test.tsx, useEditableParse.removed-entry.test.tsx).
• Experience, education, project deletion: All 4 sections support parsed entry removal via tombstoning.
• Bullet & raw text removal: removeEntryWithBullets drops associated bullets from the graded pool (sections) and rawText.
• Download PDF & Round-trip invariant: Verified via render-roundtrip-entry-remove.repro.test.ts.
• Index integrity: Overriding an entry and deleting an earlier entry preserves override alignment via survivingParsedIndices.
• Edit state & snapshots: hasEdits tracks removedEntries, captured in EditSnapshot, restored on replay.
• Backwards compatibility: Legacy snapshots without removedEntries replay cleanly without throwing (snap.removedEntries ?? []).
• Reset support: reset clears removedEntries.
• No regressions: Added-entry removal, bullet removal, and prune-on-blur remain green.
• Full verify suite: 363 test files / 5901 tests passed.

Gates & Quality Audit

• Fixture PII (Gate 3a): Passed (check:fixtures green, 0 real personas).
• Design System & Reuse (Gate 3b): Passed (RemoveButton and design system primitives reused consistently; no raw interactive elements in feature code).
• Style Tokens (Gate 3c): Passed (0 hardcoded hex or raw Tailwind palette violations).
• Dead Code & Fallow (Gate 3d): Passed (0 dead exports, verify suite green).
• Description Accuracy (Gate 3f): Accurate. All 3 deliberate design deviations from the issue's initial plan (removedEntriesRef omission, UI survivingParsedIndices index-remap, and reusing isTitleOwnedLine from group-bullets.ts) are well-justified, accurately described, and tested.

---

Reviewed by: Gemini 3.7 Flash (high)

[s-annam]

⚠️ This is a SELF-REVIEW and carries no approval weight

This PR is authored by s-annam, which is also the account this review is posted from. GitHub will not accept an approving review from a PR's own author, and posting the same verdict as a COMMENT would not make it one either. This PR still needs a second-party reviewer before it merges. Read everything below as the author's own audit trail, not as a sign-off.

---

The change does what #856 specified, the tombstone ordering is right, and the deviations from the issue's plan are improvements on it rather than shortcuts. Zero blocking findings. Everything below is Secondary or Nit.

Acceptance criteria (#856) — 10 / 10 met

AC	Status	Evidence
Parsed achievement deletable, survives re-render	✅	useEditableParse.removed-entry.test.tsx:231,254
Same for role / education / project	✅	role :275; education + achievement render-roundtrip-entry-remove.repro.test.ts:137,143; project apply-overrides.removed-entry.test.ts:145,183
Bullets leave the score + rawText	✅	:275, apply-overrides.removed-entry.test.ts:232
Absent from Download PDF, re-parses without it	✅	render-roundtrip-entry-remove.repro.test.ts:128–151
Index integrity — survivors keep their own overrides	✅	apply-overrides.removed-entry.test.ts:105–167, all four sections + a two-in-one-fold case
hasEdits + EditSnapshot + replay	✅	:327
Pre-#856 snapshot replays without error	✅	:347
reset clears removedEntries	✅	:333–334
No regression to added-entry removal	✅	:379
npm run verify green	✅	reproduced below

Gates

Gate	Result
tsc -b --noEmit	pass
eslint .	pass
check:fixtures	pass — N/A by construction, the diff touches no tests/fixtures/** binary
check:baselines	pass
check:core	pass
Full test suite (OFFLINECV_FULL_TESTS=1)	363 files passed / 8 skipped · 5901 tests passed / 10 skipped
vite build	pass — 1028 modules, 3.28s
Style tokens (3c)	clean — every regex hit was the literal #856, not a hex colour
Skill/script command bugs (3e)	N/A — no scripts/** or .claude/skills/** file in the diff
fallow audit	exit 1 — 3 complexity findings, 35 clone groups. Report-only, and all three complexity findings (RoleHeader, applyExperienceHeaderOverrides, groupBulletsByExperience) are on functions this diff does not touch; fallow re-attributes them because the diff touched their files

The reported numbers match the PR body's Validation section exactly (363/5901), so that claim round-trips.

Description accuracy (3f) — accurate, and unusually so

Every checkable claim in the body was round-tripped against the diff and holds:

• Deviation 1 (no removedEntriesRef) — confirmed; useEditableParse.ts:921 documents the absent reader, and removeEntry does return void.
• Deviation 2 (UI index remap) — confirmed and correctly characterised as load-bearing. survivingParsedIndices is exported, and all four sections resolve through it; the issue's Step 4 as literally written would have re-deleted the wrong entry after the first deletion.
• Deviation 3 (reuse the scorer's ownership relation) — confirmed; matchesOwnedKey is the single extracted predicate and both suppressTitleOwnedBullets and isTitleOwnedLine route through it.
• The forced drive-by — verified independently, not taken on trust. scripts/hooks/check_conventions.py:103-106 scans src/components/** after strip_line_comments, which strips // but not /** */, so the pre-existing exactly at ReconstructedRole.tsx:262 genuinely fired on every edit to that file. Comment-text-only, and it rides a PR already in range rather than earning its own.
• parseRemovedEntryKey digit matching — confirmed, and pinned by the "projects:" case at apply-overrides.removed-entry.test.ts:197. The Number("") === 0 trap was real.
• Files-changed list — matches git diff --name-only exactly, 16 for 16.
• "Adversarial review: not yet run" — correct to state plainly rather than let a reader assume it happened.

One thing the body flags as a landmine that is worth restating, because it is the sharpest edge here: inside ExperienceSection the two experience:<n> strings are in different index spaces (rewrite-chain section id = render position; entryKey = parsed index). They coincide until the first parsed-role deletion. The code comments it in place, which is the right treatment.

Secondary

Three, all inline. In short: EntryRemove.ts is domain logic sitting under src/components/; ReconstructedResume.tsx grew 1356 → 1489 on a file CLAUDE.md names as known debt; and a one-click destructive delete now bypasses the per-bullet confirmation strip that guards removing a single bullet.

Nits

• Duplicate-title first-match in applyRemovedEntries (inline). Near-zero real impact — noting it so the next reader does not have to re-derive why it is safe.
• fallow attributes 3 clone groups to ExperienceSection.test.tsx / .other-bullets / .prune-hold, which this PR touched by +3 lines each. Pre-existing duplication, re-attributed by proximity. Report-only; not worth a change.

Nothing was auto-fixed, deliberately

With 0 blockers the reviewer loop would normally apply small fixes and push. It did not here, because none of the findings qualify: all three Secondary items are structural judgement calls (where a file lives, how a component is decomposed, whether a destructive action confirms) rather than non-behavioural mechanical nits, and the Step 5.6 suggestion-block form does not fit any of them either. The branch is untouched and still exactly one commit (bfa94b5), so the one-commit merge-queue invariant holds as-is.

---

Head SHA reviewed: bfa94b5a465d56e1536b3cb797c052ff61b47cd5

Reviewed by: Claude Opus 5 (high)

[s-annam]

Secondary — a one-click destructive delete that skips the confirmation its own bullets have.

RemoveButton (ReconstructedAdd.tsx:132) is a bare <Button>; it does not confirm. And removeEntryWithBullets calls onRemoveBullet directly, which bypasses the per-bullet confirmation strip (removes.pending, #626) that guards removing a single bullet in the Experience path.

So the current gradient is inverted: removing one bullet asks first; removing an entry silently destroys the entry and all N of its bullets on one click. Before this PR that only applied to user-ADDED entries — trivially re-creatable, so no confirm was needed. A parsed entry is different: it carries parser output, and the only recovery is resetAll (which discards every edit in the session) or re-uploading the PDF. #856 scoped out an undo toast; it did not scope out a confirm, and the asymmetry with #626 is the part that stands out.

Not blocking — the behaviour is what the issue asked for and the tests pin it. Worth a follow-up issue rather than a change in this PR, given the round-trip and index work here is already large and well-tested. Same applies to label="Remove project" (:814), "Remove role" (ReconstructedRole.tsx:440) and "Remove education" (ReconstructedEducationSkills.tsx:166).

[s-annam]

Deferred to #860 — #860. No code change in this PR, per your own recommendation.

The issue carries the full argument: the inverted gradient vs #626, why "added entries were trivially re-creatable" stops holding once parsed entries are deletable, that resetAll / re-upload is the only recovery, and that #856 scoped out an undo toast but never a confirm. It lists all four call sites you named — Remove achievement (ReconstructedResume.tsx:1058), Remove project (:814), Remove role (ReconstructedRole.tsx:440), Remove education (ReconstructedEducationSkills.tsx:166).

Its Reuse analysis lands on extending the #626 inline-confirm strip rather than reaching for Dialog — a modal for a row-level delete diverges from the very bullet path this is meant to align with.

Leaving this thread open for you to close.

[s-annam]

Secondary — this file grew on the one file CLAUDE.md tells us not to grow.

ReconstructedResume.tsx: 1356 → 1489 lines (+133). From CLAUDE.md:

⚠️ Known debt — do not imitate: ReconstructedResume.tsx (1338) … If you are editing one, prefer extracting your change into a new sibling over growing the file further.

The PR did extract a sibling — EntryRemove.ts — which is the right instinct; it just is not where the mass is. fallow now reports three functions in this one file over the ~200 LOC guidance: ReconstructedResume (370), ExperienceSection (320), ProjectsSection (143), with AchievementsSection alongside them.

The honest framing: this PR did not create that debt and the four sections were already co-located, so splitting them out here would balloon a diff that is already 1846 insertions and would make the index-remap change — the part that actually needs careful review — harder to see. Recommend not doing it in this PR, and instead filing the section split as its own issue. Flagging it because the file is now 151 lines past the number CLAUDE.md quotes, so that quote is stale and the next person to read it will under-estimate the problem.

[s-annam]

Split deferred to #861 — #861 — taking your recommendation not to do it here.

The one part I did act on is the stale quote, since that was the concrete harm you named: CLAUDE.md’s known-debt line now reads ReconstructedResume.tsx (1489) and ReconstructedRole.tsx (490), replacing 1338 / 483 (5ec090c). The next reader gets the real number instead of one 151 lines light.

#861 carries the fallow breakdown (ReconstructedResume 370, ExperienceSection 320, ProjectsSection 143), asks for each section as a pure move in its own commit, and pins no behaviour change as an acceptance criterion — a test that needs editing means the move was not pure. It also requires the CLAUDE.md figures be refreshed again post-split, so this does not go stale a second time.

Leaving this thread open for you to close.