Skip to content

bug(merge-tracker): three silent data-loss paths — dropped second update, wiped notes, no intra-run dedup #2392

Description

@Scott-Emberson

Three paths in merge-tracker.mjs lose user data without an error, a warning, or a non-zero exit. All three are in the duplicate-handling block. data/applications.md is user-layer data that is never regenerated, so each of these is unrecoverable once the TSVs are archived.

1. A second TSV updating the same row in one run is silently dropped

merge-tracker.mjs:730 locates the row to rewrite by its original text:

const lineIdx = appLines.indexOf(duplicate.raw);
if (lineIdx >= 0) {
  ...
  appLines[lineIdx] = updatedLine;
  updated++;
}

duplicate.raw is captured during the initial parse (:567) and never refreshed after the in-place write. syncPdfFlags (:279) does refresh its cached row, so the two paths disagree.

So in a single merge run with two TSVs matching the same tracker row (3.5 → 4.0, then → 4.5): the first update rewrites the line, and the second finds indexOf returning -1, skips the whole block, increments no counter, prints no warning, and still logs its optimistic 🔄 Update line beforehand. The TSV is then archived to merged/ as if it had been applied. The 4.5 evaluation is gone.

This is reachable whenever a batch run re-evaluates the same posting twice, which is exactly what the batch workflow does after a scoring change.

2. The score-upgrade path discards the existing Notes

merge-tracker.mjs:740:

notes: `Re-eval ${addition.date} (${oldScore}${newScore}). ${addition.notes}`,

duplicate.notes is dropped wholesale. Anything the user recorded with set-status --note (recruiter name, req number, interview context) is erased by a re-evaluation that happens to score higher.

This one compounds: the req/job-number guard added in #1524 reads extractReqNumber(app.notes) at :718 to decide whether two similar titles are genuinely distinct postings. Wiping the notes destroys the evidence that guard depends on, so a later merge is more likely to wrongly collapse two real postings.

3. No intra-run dedup, so one run can create the duplicate it exists to prevent

New rows go to newLines (:777) and are spliced in at the end (:797). They are never pushed to existingApps — line 567 is the only push, inside the initial parse. Duplicate detection at :660-721 therefore only ever sees rows that existed before the run started.

Two parallel workers evaluating the same posting each reserve their own number and write their own TSV. Both merge as new rows in a single run, producing exactly the company+role duplicate the file header claims to prevent. verify-pipeline.mjs then warns about it and dedup-tracker.mjs has to clean it up, at which point one row's status and notes are merged away.

Tests

I could not find coverage for any of the three in tests/merge-tracker.test.mjs or the merge sections of test-all.mjs. The invariants worth pinning, independent of how the fixes are written:

  • Two TSVs updating the same row in one run: both updates land, or the second fails loudly. Never a silent no-op with the TSV archived.
  • A score upgrade preserves the existing note content alongside the re-eval prefix, and a req number in the old note survives the merge.
  • Two TSVs for the same company+role in one run produce one row, not two.
  • A merge that no-ops for any reason must not archive the TSV that was not applied.

Happy to send a PR. Flagging first because fix 1 has a design choice in it (refresh the cached row versus track rows by index throughout), and fix 2 has a merge-semantics question about how old and new notes should be combined when both are non-empty.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions