fix(openclaw-tps-mail): surface and reconcile a failed cur/ record write after a terminal transition

[tps-anvil]

A failed cur/ stamp write after a durable acked/failed transition was silently ignored by patchMailFile.

What changed

• Return stamp-write outcomes and report failures with actor, inbound id, record or lock path, and the error code when available.
• Retry failed live stamps; hold unresolved startup stamps from recovery and retention.
• Keep main's other plugin diagnostics.

Evidence

A real OpenClaw lifecycle test is a follow-up.

Measured on cab4372:

• Plugin suite: 314 pass / 0 fail.
• bun run build, bun run lint:ci, plugin npm run build and CI npm run typecheck:test: exit 0.

Closes #492.

Summary by CodeRabbit

• Bug Fixes
  • Failed acknowledgment and failure-stamp writes are now reported with diagnostic details and retried when possible.
  • Startup reconciliation retries terminal stamp updates, while missing, unreadable, or ambiguous records are handled without incorrectly processing their obligations.
  • Unresolved obligations are retained for recovery instead of being removed during retention cleanup.
  • Mail-lock failures now include path information, and missing-record detection is more precise.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: d6d7a3ab-9e20-4d96-9498-21c6795b00b0

📥 Commits

Reviewing files that changed from the base of the PR and between 70bccde and b143687.

📒 Files selected for processing (9)

• .changelog/unreleased/fixed-492-cur-record-stamp-reconcile.md
• packages/cli/src/utils/mail.ts
• plugins/openclaw-tps-mail/src/diagnostics.ts
• plugins/openclaw-tps-mail/src/index.ts
• plugins/openclaw-tps-mail/src/obligations.ts
• plugins/openclaw-tps-mail/test/cur-record-write.test.ts
• plugins/openclaw-tps-mail/test/diagnostics.test.ts
• plugins/openclaw-tps-mail/test/obligation-retention.test.ts
• plugins/openclaw-tps-mail/test/patch-mail-file.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

The plugin now reports failed cur-record stamp writes, retries eligible acknowledgment and failure stamps, and reconciles terminal obligations with cur records during startup. Startup recovery and retention handling now account for records whose obligation state is unknown.

Changes

Terminal stamp recovery

Layer / File(s)	Summary
Stamp write outcomes and diagnostics plugins/openclaw-tps-mail/src/diagnostics.ts, plugins/openclaw-tps-mail/src/index.ts, plugins/openclaw-tps-mail/src/obligations.ts, packages/cli/src/utils/mail.ts, plugins/openclaw-tps-mail/test/patch-mail-file.test.ts, plugins/openclaw-tps-mail/test/diagnostics.test.ts	Cur-record updates now verify record identity and return structured success or failure results. Diagnostic formatting includes available path, ID, code, obligation state, and retry status. Read and lock errors can include their paths.
Live terminal stamp retries plugins/openclaw-tps-mail/src/index.ts, plugins/openclaw-tps-mail/test/cur-record-write.test.ts, .changelog/unreleased/fixed-492-cur-record-stamp-reconcile.md	Acknowledgment and failure settlement use a retrying stamp helper. Tests cover transient and persistent failures, retry cancellation when an account stops, and successful stamping. The changelog describes failure reporting and retries.
Startup reconciliation and retention plugins/openclaw-tps-mail/src/index.ts, plugins/openclaw-tps-mail/src/obligations.ts, plugins/openclaw-tps-mail/test/patch-mail-file.test.ts, plugins/openclaw-tps-mail/test/cur-record-write.test.ts, plugins/openclaw-tps-mail/test/obligation-retention.test.ts	Startup reconciles terminal obligations with cur-record stamps. Unreadable or ambiguous records are tracked as unknown; affected obligations are skipped during recovery and held during retention sweeping.

Priority: ➖ Normal

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant ackObligation
  participant retryTerminalStamp
  participant patchMailFile
  participant CurRecord
  ackObligation->>retryTerminalStamp: request ack stamp
  retryTerminalStamp->>patchMailFile: apply stamp
  patchMailFile->>CurRecord: write terminal stamp
  CurRecord-->>retryTerminalStamp: return write result
  retryTerminalStamp-->>ackObligation: return success or failure

Loading

Suggested reviewers: heskew, tps-sherlock

Merge Risk: ⚪ Minimal · up to b1436

No actionable issue introduced by this change remains identified. The PR is mergeable after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to b1436

The recovery design preserves identity checks, locking, and conservative handling of unresolved state. A changed public return contract could cause older integrations to mistake a failed write for success, although no affected integration has been identified.

Retained concerns

• Low · reliability · inferred: The published patchMailFile contract is incompatible with older callers. A caller retaining the boolean success check would treat the new failure-result object as truthy; an old two-argument invocation also omits the newly required inbound identity. This could conceal failed terminal writes in an older integration. Repository callers are migrated, and no affected external consumer is established.

Security review details

Security Blast Radius

• observed — The inspected production reconciliation caller supplies the configured account mail root and agent identity. Its direct state effects concern that agent's cur records and obligation retention; the module export alone does not establish additional tenant or remote reachability.

Trust Boundaries and Controls

• observed — Startup lookup treats duplicate identities, scan limits, and read failures as unresolved rather than selecting a record optimistically. The final locked mutation independently checks inbound identity before writing.

Resilience and Maintainability Implications

• observed — Failure diagnostics carry actor, inbound identity when available, record or lock path, error code when available, and obligation-presence state. They accompany explicit failure outcomes and recovery holds rather than converting an unresolved write into success.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 34.62% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 26 functions across 8 files. (1 skipped: …	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: surfacing and reconciling failed cur/ record writes after terminal transitions.
Linked Issues check	✅ Passed	Issue #492 requires a failed terminal cur/ stamp to be reported with the inbound ID, record path, and error code, then retried or reconciled, with an injected-failure test. The changes report stamp …
Out of Scope Changes check	✅ Passed	The changes to diagnostics, obligation tracking, retention, locking, and tests support reporting and retrying terminal stamp failures while avoiding unsafe recovery or deletion of unresolved obligatio…

Full details: Docstring Coverage

Explanation

Docstring coverage is 34.62% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 26 functions across 8 files. (1 skipped: 1 unsupported.)

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• 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

• X
• Mastodon
• Reddit
• LinkedIn

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

[tps-anvil]

Claim sweep (cli#492, measured on e08dd50)

Every sentence the PR adds or changes, checked against the code.

src/index.ts prose

1. "Every ack/nack stamp written AFTER a durable terminal transition goes through this, and only this" — PASS. grep ackedAt|nackedAt in src/index.ts: the only writers are the ack call (ackObligation), the nack call (settleObligation), and the reconcile pass — all updateExistingCurRecord.
2. "it REUSES the CLI's own mailbox lock … both lock the same <mailDir>/<agent>/.mail-lock" — PASS. promote() locks acquireMailLock(dirs.root) → mailLockPath(root); the helper locks mailLockPath(dirname(dirname(path))), the same agent mailbox root.
3. "it is EXISTING-ONLY: a missing or unreadable record is a NAMED refusal, never a create" — PASS. readCurRecord returns record-missing / record-unreadable; no branch creates a record.
4. "REPLACES atomically (dot-temp + rename), so a crash can never leave a torn record" — PASS. Unique dot temp in the same dir, then renameSync; a kill leaves target old-or-new, and a leftover temp is inert.
5. "It never throws." — PASS. Read, lock, and write are each guarded; lock.release() in finally.
6. "a failed write is surfaced (logged by message id, record path and error code)" — PASS. Both callers log inboundId, stamped.path, stamped.code; the new test asserts all three.
7. "and reconciled on the next scan — never silently ignored (cli#492)" — PASS, scoped. Reconcile covers acked/failed (a doc line now names exactly those); unconfirmed carries no cur/ stamp by design; a record that is missing/unreadable is skipped and retried on a later scan; a reconcile failure is logged by name. Conditional: reconciliation needs a later scan while the record still exists — if the record is gone there is no stamp to reconcile.
8. "An unverifiable lock owner is NOT 'no lock': fail closed, never write outside the lock" — PASS. The tryAcquireMailLock throw returns lock-unverified with no write.
9. ack-stamp-failed text "the obligation is acked and the stamp is reconciled on the next scan" — PASS with the same scope note as (7).
10. Reconcile doc "Idempotent; a failure here is logged by name so the next scan tries again." — PASS. Guarded by !cur.ackedAt / !cur.nackedAt; a failure warns.
11. Startup comment "an acked or failed obligation … is re-stamped here … BEFORE the recovery loop below decides whether to re-dispatch" — PASS. The call sits before the curDir recovery loop.

.changelog/unreleased/fixed-492-cur-record-stamp-reconcile.md

12. "Every ack/nack stamp now goes through one locked, existing-only, atomic writer that reuses the CLI's mailbox lock, so a failed write no longer leaves the on-disk record and the plugin's view diverged in silence." — PASS, same basis as (1)-(7).

PR body

13. Counts and statuses — PASS. Each number names its commit: e08dd50 (head) and 77ee454 (main). The node-load, tsc, lint:ci and changelog results are re-runnable commands, not assertions about other branches.

Test names / operator strings

14. Test titles state the asserted behaviour ("logs id/path/code and is reconciled on the next scan", "the successful write path is unchanged") and match the assertions. — PASS.

[tps-flint]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[tps-kern]

Review — tpsdev-ai/cli PR #500 (cur/ stamp reconcile, head b143687)

Verdict: APPROVE. All five focus properties hold, each pinned by tests I ran green. Repo visibility re-checked today via the API: public. The findings below are one minor truthfulness question and environment disclosures — nothing blocking, nothing requiring private handling.

Scope note: the branch carries its full stack (merge-base with main is 70bccde; main has since advanced on other paths). I reviewed the net delta GitHub will land (9 files, +1050/−44) and verified it introduces no reversals of main's newer work — notably it keeps and builds on the #469 rule (d5a09c2 is an ancestor of the fork point, so the existing-only helper this PR tightens is the locked one).

Focus adjudications

The reconcile cannot regress a newer state — verified. patchMailFile (index.ts:632) drives the record only through updateExistingRecord's mutate-existing path, and its mutator: refuses a replaced identity (current.id !== inboundId → ID_MISMATCH throw, never stamps a stranger's record); returns null — no write — when current[stampKey] is already set (already-stamped ⇒ ok, never re-stamps, never overwrites); and the reconcile loop itself skips records whose stamp is present (index.ts:690) and applies only the additive terminal patch (ackedAt+read, or nackedAt+nackReason) the durable obligation says is owed. Nothing un-stamps, nothing moves a record backward, and a missing record is terminal (record-missing, no retry). Pinned by cur-record-write.test.ts: "a replacement identity is refused on the live stamp path", "a missing record is reported after the durable transition", and the restart matrix (stamps deleted from a settled record are re-stamped exactly once per boot, never re-dispatched).

The #469 existing-only update rule is followed — verified. Every stamp goes through updateExistingRecord (packages/cli/src/utils/mail.ts:189): read-modify-write under the mail lock with scratch+rename, never creating a record. The PR tightens the helper's error contract: gone now requires err?.path === path (mail.ts:224) so an ENOENT from the lock file or scratch space can't be misread as the record being gone, and lock-acquisition failures now carry the lock's own path (via mailLockPath), which patchMailFile's catch maps so the diagnostic names the lock — not a phantom record path (index.ts:652-655). Pinned by "lock/scratch acquisition reports the failing path in live and startup stamps".

No retry loop without a bound — verified. stampTerminalCur (index.ts:957): delays [1000, 4000, 16000], attempt 3 gets undefined → stop with retriesExhausted; record-missing never retries; every timer is account-lifetime-tracked (index.ts:822) and isLiveContext-gated, so account teardown cancels pending retries. Pinned exactly: "initial attempt + 3 retries = 4 logged failures, then no more… no attempt beyond the bound… nothing retries after the bound", and "stopping the account cancels the pending stamp retries". The reconcile pass itself is single-shot per boot, and findCurPath bounds the cur/ scan at 4096 entries (SCAN_LIMIT).

Diagnostics name id, path and code but no message content — verified. formatStampDiagnostic (diagnostics.ts:13) emits kind, id, actor, path, code, obligation presence, no retries left, and a remedy — never envelope/subject/body text; the reconcile's info lines carry id+path only; the nack reason (rec.failure, a named code) is written to the record but never logged. Pinned by diagnostics.test.ts and the id/path/code assertions throughout cur-record-write.test.ts.

A reconcile never re-delivers or re-acks — verified. reconcileTerminalCurStamps (index.ts:660) reads obligations/cur records defensively (readObligationResult never throws; torn or malformed records become unknown, not truth) and its only write is the stamp. It touches no dispatch path. The interlock is the load-bearing part: at boot, the reconcile runs FIRST (index.ts:2384); its unresolved set then (a) blocks crash-recovery re-dispatch of exactly those records (index.ts:2402 — a record whose terminal state cannot be proven is never re-delivered), and (b) holds those obligations from the retention sweep (obligations.ts:589, heldForRecovery), with a full obligations-directory read failure ("*") skipping the sweep entirely. Pinned by the "holds aged obligations across restarts without redispatch" matrix (both states × four failure stages, two restarts: one diagnostic per restart, dispatch() stays null, obligation bytes unchanged) and the retention cases (j)/(k).

Findings

1. [minor, question] index.ts:677-678 / :691-692 — the reconciled stamp is reconcile-time, not transition-time. The reconcile writes ackedAt/nackedAt: new Date().toISOString(); the obligation already carries the truthful transition time (lastTransitionAt, present since the field's introduction, with inboundTimestamp fallback for legacy records). After a reconcile, the cur record claims the terminal transition happened at reconcile time. Retention is unaffected (the sweep ages by the obligation's lastTransitionAt, not the cur stamp — verified), so the impact is record truthfulness/observability only. Consider rec.lastTransitionAt ?? new Date().toISOString(), or a comment pinning the current choice as deliberate.
2. [nit] index.ts:652 — the busy branch of patchMailFile reports the record path, not the lock path. The catch path carefully maps lock-path errors to the lock; the non-blocking busy status (currently unreachable — no caller sets nonBlocking) would name the record. Fine as-is; worth a one-line comment if a non-blocking caller ever appears.
3. [environment disclosure, not the PR] The plugin's standalone tsc build is red in my worktrees on this head and at origin/main: the pre-existing gateway-adapter block (isEnabled: (account) => …, index.ts:1692-1693 on head) loses contextual typing when the openclaw dev dependency's types don't materialize, plus — on main only — a stale openclaw/plugin-sdk/channel-contract import that this branch actually fixes. Likewise the suite's two remaining failures ("the trap, demonstrated" ×2, ENOENT …/node_modules/openclaw/package.json) are the dev dependency not being materialized in my tree. Control at origin/main fails the same builds (with more errors), so I attribute all of the above to my environment, not to this PR; CI is out of my lane.

What I ran / could not see

Internal author (tps-anvil): bun install --frozen-lockfile + root build (packages/agent + packages/cli — direct packages/cli build exits 0; the root script's first || true step emitted a pre-existing TS18046 at mail.ts:698, outside this PR's hunks, and does not fail the chain), plugin build attempted (see finding 3), and the full plugin suite through its HOME-isolated launcher (TMPDIR=/tmp required by its own guard — which refused to run inside the operator home, correctly): 303 pass / 2 fail, the two failures being the openclaw-dev-dependency materialization above, unrelated to the four changed test files, all of which pass (including the 557-line cur-record-write matrix, patch-mail-file, diagnostics, obligation-retention). The launcher's env allowlist and TPS_MAIL_DIR/TPS_TEST_KEYS_DIR isolation meant no run touched live mail state; my own runs left no processes behind. I could not see any truncated content that affected these conclusions; the one thing I did not run is the repo-root suites (agent/cli/root-test), which CI owns.

[tps-sherlock]

Sherlock review — PR #500, head b1436870 (surface + reconcile a failed cur/ stamp)

Repo visibility: PUBLIC (GET repos/tpsdev-ai/cli → .visibility = "public", checked before writing). Author tps-anvil is a tps-* agent, so this is an internal PR and I built and ran the tree. Scope: 8 files — production changes in plugins/openclaw-tps-mail/src/{index.ts,obligations.ts,diagnostics.ts} and packages/cli/src/utils/mail.ts.

Evidence (my own runs). Worktree ~/work/review-500-sherlock @ b1436870; repo build (bun run build) exit 0. Plugin suite run through the isolated launcher (node plugins/openclaw-tps-mail/scripts/run-tests.mjs, TMPDIR outside the home, HOME pointed at an empty dir): 301 pass / 4 fail / 1 error across 305 tests. The four failures are environmental, not this diff: node-load (cli#394) ×2, the trap, demonstrated, and a final-selection module load — all require the plugin's built dist/src/index.js and the openclaw peerDependency, neither present in this worktree (the node-load test states the precondition itself: "the workspace … and this plugin are built, so dist/src/index.js … exist"). Every cli#492 case and every reconciliation/retention case passes. The launcher's HOME-ISOLATION GUARD was clean once HOME pointed at an empty dir (it had flagged concurrent live-agent writes to the real ~/.tps on the first run — its own note: "on a host where live agents write ~/.tps, their activity shows here too"). No Harper started; production (:9926) untouched.

Focus — the diagnostic names id, path and code; no message content

formatStampDiagnostic (diagnostics.ts:13) composes only the named facts:

const fields = [ `tps-mail: ${facts.kind}:`, ...(facts.id === undefined ? [] : [facts.id]),
  `actor=${facts.actor}`, `path=${facts.path}`, ...(facts.code === undefined ? [] : [`code=${facts.code}`]) ];

There is no body/content/subject/nackReason field, and all three call sites (index.ts:665, :694, :971) pass only {kind, actor, id, path, code, obligation, retriesExhausted}. code is a system err.code (EACCES/ENOENT), a scan marker (SCAN_LIMIT/AMBIGUOUS_ID), or undefined; path is a mail-file path; id is the inbound id — never message content. diagnostics.test.ts pins the exact output strings. The nack body (nackReason: rec.failure) is written to the record, never to the log. Confirmed.

Focus — a reconcile never re-delivers or re-acks

reconcileTerminalCurStamps (index.ts:660) does exactly one write per terminal obligation — the terminal stamp that failed — and nothing else: it reads the obligation + cur record, skips when cur[key] is already set, and calls patchMailFile(curPath, patch, key, rec.inboundId), which routes through updateExistingRecord (existing-only, locked; cli#469 rule). It never dispatches, sends, or acks. The returned unknownInbounds set is then used to suppress the paths that could re-deliver or re-touch an uncertain inbound: crash recovery (index.ts:2402), the retention sweep (:2465), and the re-arm loop (:2483) all skip unknownInbounds. The tests assert this end-to-end — expect(next.dispatch()).toBeNull() and the … holds an aged timestamped inbound … without redispatch cases. The live retry (stampTerminalCur) re-runs only the stamp write. Confirmed: no re-delivery, no re-ack.

Supporting checks (all green here)

• The _reindex-style identity guard: patchMailFile throws ID_MISMATCH when current.id !== inboundId, so a replaced record is never stamped (tested for acked and failed).
• A concurrent stamp between precheck and lock is preserved (if (cur[key]) continue plus the locked re-read); a second reconciliation preserves mtime/inode and logs nothing.
• packages/cli/src/utils/mail.ts: the ENOENT branch is narrowed to err?.code === "ENOENT" && err?.path === path, so a missing lock/scratch ENOENT is no longer mistaken for a gone record; lock-acquisition failures now carry mailLockPath(root) so the diagnostic names the lock.

Minor notes (non-blocking)

• [1] index.ts:660 — a terminal obligation whose cur/ record is absent is held from the sweep. findCurPath(..., onReadError) reports ENOENT when no cur file matches, so reconcileTerminalCurStamps marks that inbound unknown, and the sweep then retains it (the tests pin this: "missing cur lookup holds the obligation", and the retention suite now requires a stamped cur record to sweep). This is deliberate and logged (held N for unresolved cur/ recovery), so it is not silent — but an obligation whose cur record never reappears is retained indefinitely. Worth a bounded follow-up (retention is Kern's lane); not a defect in this diff.
• [2] Retry bound. stampTerminalCur is bounded to the initial attempt + [1000, 4000, 16000] ms, timers unref'd and cancelled when the context stops — tested by "no retries left" and "stopping the account cancels the pending stamp retries".

Verdict: APPROVE. The diagnostic exposes only id/path/code and a state summary (no message content); reconciliation only re-stamps the durable terminal state and its unknownInbounds set actively suppresses re-dispatch/re-acquire; the failed-stamp path is bounded, logged and covered. The four suite failures are a missing build artifact and openclaw peer dep, unrelated to this change.