Skip to content

fix(mail): first delivery into cur/ never replaces an existing record (#482) - #503

Open
tps-anvil wants to merge 16 commits into
mainfrom
fix/482-cur-no-replace
Open

tps-anvil wants to merge 16 commits into
mainfrom
fix/482-cur-no-replace

Conversation

@tps-anvil

@tps-anvil tps-anvil commented Oct 3, 2026 •

Copy link
Copy Markdown
Collaborator

Closes #482.

First delivery never replaces or writes through an existing cur/ record.
A delivery never replaces an existing cur/ record: both writers place it with an exclusive hard link.

Evidence

Measured on 1973cb4:

  • bun run build: exit 0.
  • tsc --noEmit -p packages/{agent,cli}/tsconfig.json: exit 0 each.
  • MailClient (packages/agent/test/io.test.ts): 14 pass / 0 fail, exit 0.
  • test/mailbox-policy.test.ts: 3 pass / 0 fail, exit 0.
  • packages/cli/test/mail-promote-scratch.test.ts: 4 pass / 0 fail, exit 0.
  • packages/cli/test/mail-slice-a-followups.test.ts: 52 pass / 0 fail, exit 0.
  • test/mail-cur-writers.test.ts: 12 pass / 0 fail, exit 0.

Measured on 238511f:

  • bun run lint:ci: exit 0.
  • node scripts/changelog-fragments.mjs check: exit 0.

…#482)

promote() and MailClient moved a record into cur/ with a rename, so a
colliding filename overwrote an already-delivered record. Both now place
the record with the shared placeCurRecord() exclusive link: on a collision
an identical delivery is dead-lettered as a replay and a different record
under the same filename is dead-lettered as an integrity error naming the
record id, leaving the delivered record untouched.
@tps-anvil
tps-anvil requested a review from a team as a code owner October 3, 2026 20:02
@tps-anvil

Copy link
Copy Markdown
Collaborator Author

482 sweep — every sentence the diff adds or changes, and its call

Measured against the code the diff introduces. git diff origin/main...HEAD (source, comments, fragments, test names/strings). One line per sentence/claim; "ok" unless noted.

packages/agent/src/lib/mailbox-policy.ts

  • "Both mailbox first-delivery writers (the CLI's promote() and this package's MailClient) place a record in cur/ under its filename." — ok: both call placeCurRecord.
  • "The placement uses an EXCLUSIVE link, so an existing record is never replaced — a plain rename would silently overwrite an already-delivered record when a filename collides." — ok: linkSync fails EEXIST; origin/main used renameSync.
  • "On the EEXIST collision the two records are compared by content; see placeCurRecord." — ok.
  • "An identical delivery already sits at the destination: an idempotent duplicate." — ok: status: "duplicate".
  • "A DIFFERENT record already sits at the destination: an integrity error." — ok: status: "collision".
  • "Parse a record file, or null when it is unreadable or not a JSON object." — ok: readRecordJson.
  • "The record's signed envelope: its stored envelope, or a signed JSON body." — ok: storedEnvelope.
  • "Only from, to, subject, body and replyToId are compared; the transport identity (messageId), the signing timestamp, the signature, the trust tier and the delegation chain are excluded." — ok: the JSON.stringify uses exactly those five keys. (Rewritten from an open-ended list that omitted trust.)
  • "so a re-signed re-delivery of the same message compares equal" — ok by construction of the compared set.
  • "A different message, or a record with no usable envelope, does not." — ok: null content → not equal → collision.
  • "The record's envelope id: a promoted envelopeId, else a signed body's messageId." — ok: recordEnvelopeId.
  • "Place sourcePath at curPath for a mailbox's FIRST delivery, never replacing an existing record." — ok.
  • "The placement is an exclusive link, which fails with EEXIST rather than overwriting." — ok.
  • "On a collision the records are compared by delivery content: an identical record is a duplicate (the caller dead-letters it as a replay, leaving the delivered record untouched); a different record under the same filename is a collision (an integrity error)." — ok: CLI rejectToDlq(...,"replay")/("invalid"); MailClient returns class replay/invalid → deadLetter.
  • "An unreadable destination — or one with no usable envelope — is never treated as identical." — ok: null guard.
  • "A non-EEXIST link failure is thrown." — ok.
  • "On placed, the source and destination are hard links to one inode: the caller unlinks the source to leave a single copy." — ok: CLI rmSync(scratchPath), MailClient unlinkSync(srcPath).
  • "EEXIST means a NAME is taken, not that a delivered record is there. Only a regular file can be a first-delivered record: a directory or any other entry at the destination is a storage fault, which the caller treats as retryable." — ok: statSync + isFile(); the CLI maps the throw to storage-unavailable, MailClient leaves the record in new/.

packages/agent/src/io/mail.ts

  • commitToCur JSDoc (place with an EXCLUSIVE link; collision → duplicate/integrity; append failure → move back) — ok.
  • "Placed: cur/ and the source are hard links to one inode. Drop the source link, then commit the consumed id durably." — ok.

packages/cli/src/utils/mail.ts

  • "First delivery into cur/ uses an EXCLUSIVE link, so an existing record is never replaced. On a filename collision the two records are compared by content: an identical one is a duplicate (dead-lettered like a replay, the delivered record untouched); a different one is an integrity error." — ok.

test/mail-cur-writers.test.ts

  • "First delivery into cur/ uses placeCurRecord()'s exclusive link, so it never replaces an existing record: an identical same-filename record is a duplicate, a different one is an integrity error." — ok.
  • ALLOWED why: "placeCurRecord — the shared exclusive first-delivery into cur/ (never replaces)" — ok.
  • "Follow-up: the per-process bridge queue." — unchanged, not this PR's claim.

.changelog/unreleased/fixed-482-first-delivery-no-replace.md

  • Lede: "First delivery into cur/ never replaces an existing record; a colliding filename is a duplicate or an integrity error (Closes Mail first delivery can overwrite an existing cur/ record with the same filename #482)." — ok.
  • "Previously promote() and MailClient moved a record into cur/ with a rename, so a colliding filename could overwrite an already-delivered record." — ok after rewrite: origin/main renameSync overwrote only on a different-id filename collision (a same-messageId replay was already caught by the replay gate). Removed "replayed" to avoid claiming the replay gate did not exist.
  • "an identical record is an idempotent duplicate (dead-lettered as a replay)" — ok.
  • "a different record under the same filename is dead-lettered as an integrity error naming the record id" — ok.
  • "The delivered record is left untouched." — ok.

Tests (names / comments)

  • "a second delivery with the same filename and identical content is a duplicate no-op" — ok (CLI + MailClient).
  • "a second delivery with the same filename and different content is an integrity error" — ok (CLI + MailClient).
  • "A second record under the SAME filename must not replace the delivered record. Identical content is an idempotent duplicate (replay); different content is an integrity error. Both leave cur/ untouched." — ok.
  • "Same filename, same message content, a DIFFERENT envelope id (re-signed)." — ok.

Not fixed / noted

  • placeCurRecord's destination parameter is named curPath so the cur-writer scan recognizes the one cur/ writer; the writers' calls to the imported helper are not resolved by the scan (unchanged limitation, "Unrecognized destinations may be missed").
  • The agent package's own bun run lint:ci (biome lint ./src) reports "No files were processed ... ignored" — pre-existing (agent src is not part of the root lint:ci CI step); not caused by this diff.

@coderabbitai

coderabbitai Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 2d497b6a-c415-4491-94de-6ff49aff78e8
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

tps-flint and others added 15 commits October 3, 2026 14:53
Mechanical merge: no conflicts.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…st titles say dead-lettered (#482)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… delivery content (#482)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ender-controlled wrapper (#482)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…r test reaches collision handling (#482)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…can never rewrite cur/ (#482)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ing replay (#482)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ages/pi-tps-mail

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…at (#482)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…cturally, never by content (#482)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…/ record; a consumed ID is a replay

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…o-replace guarantee

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…cement (#482)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mail first delivery can overwrite an existing cur/ record with the same filename

2 participants