Stop the autoSync daemon on a permanent mark-synced failure

[grimicorn-agent]

What changed and why

A permanent mark-synced failure (a dead token or forbidden account: HTTP 401/403) left autoSync on, so the daemon re-attempted the same records every pass forever (issue #133). The delete path already guards this via deletePermanentlyFailed; this mirrors that pattern for the mark-synced branch.

This branch was rebased onto main's bulk mark-synced (#128), which replaced the per-record PATCH with chunked markRecordsSynced. The permanent-failure handling is now layered onto that bulk path:

• markSyncedChunk classifies a failed chunk into a single MarkAbortReason: 'timeout' (hung server), 'permanent' (dead token / forbidden account: 401/403), 'transient' (a systemic 429/5xx that may be a blip), or null (a plain per-chunk failure that doesn't abort). Any non-null reason aborts the remaining chunks; only 'permanent' also stops the daemon. Permanence is classified via the shared isPermanentApiFailure guard, so the rule lives in the API seam.
• markRecordsSynced surfaces the aborting chunk's reason up as that single MarkAbortReason discriminant on MarkSyncedResult, replacing the previous timedOut boolean — one field, so "aborted" and "why" can never disagree.
• markWrittenRecordsSynced returns whether the autoSync daemon should stop — true only when abortReason === 'permanent' and a daemon was running (autoSyncEnabled) — so runDefaultSync returns false (stops rescheduling) exactly as the delete path does. A timeout or transient failure keeps autoSync alive to retry.
• The failure headline is worded from the same (abortReason, autoSyncEnabled) the returned stop signal uses, so it never claims a daemon stop that didn't happen (a one-shot markpost sync never had one) and only says records were skipped when a trailing chunk was actually unsent. It points the user to the classified reason markRecordsSynced logged rather than prescribing markpost config (wrong for a 403 plan limit / sign-ups disabled).

Implementation decisions

• Permanence is a run-level abort reason, not a per-record outcome. In the bulk world a whole chunk fails together, so a dedicated MARK_PERMANENTLY_FAILED per-record tag would be redundant — the failed records stay MARK_FAILED (pending) and the run carries the 'permanent' reason that stops the daemon.
• A transient systemic failure (429/5xx) aborts the run but doesn't stop the daemon. It backs off the remaining chunks (main's behavior) but keeps autoSync alive — a lone 5xx can be a blip, so the next pass should retry rather than shutting down.
• Permanence classification is extracted into isPermanentApiFailure (api.ts) and reused by the mark-synced, delete, and outer-sync-catch paths.

Tests

• Unit (records/api): markRecordsSynced reports abortReason: 'permanent' on 401/403 and 'transient' on a 503 (both aborting the remaining chunks); direct coverage for the isPermanentApiFailure guard.
• Integration (index): a permanent abort stops the daemon and reports pending/marked counts correctly when the outcomes array is truncated; timeout and transient aborts keep the daemon alive; the transient headline only claims records were skipped when a chunk was actually unsent; and a one-shot sync never claims "auto-sync was stopped".

lint, typecheck, build, and the full suite (791 tests) are green.

No services, env vars, or external setup required. This is a CLI daemon behavior change; verify via markpost sync with autoSync on against a token that 403s.

Closes #133

Follow-up suggestions

• Sanitize server-derived text in logErrorMessage — markSyncedChunk logs a server-controlled error.message to stderr via logErrorMessage without sanitizeForTerminal, unlike every other server-derived print in src/index.ts; a malicious/compromised server could embed a terminal escape. Best fixed uniformly inside logErrorMessage (multiple callers). (suggested: P3, effort: S, evidence: src/libs/errors.ts logErrorMessage / src/libs/records.ts markSyncedChunk catch)

[grimicorn-agent]

Independent code review trail

Ran the independent reviewer (Opus, fresh context, diff on stdin) across several rounds until the core logic was confirmed correct ("No correctness bug in the abort/stop path"). Summary of what was flagged and done:

Correctness / behavior

• One-shot sync falsely claimed "auto-sync was stopped" (no daemon on a plain markpost sync) — fixed: the clause is emitted only when a daemon was actually running.
• Return value vs. headline could diverge — fixed: markWrittenRecordsSynced now returns stoppingAutoSync (the exact value the message derives from), and its doc matches.
• Permanent-vs-timeout abort precedence — fixed + tested: a permanent failure outranks a concurrent timeout so the daemon can't reschedule into a dead token.
• Headline prescribed markpost config, wrong for a 403 plan-limit/sign-ups-disabled — fixed: points the user to the per-record classified reason instead.

Code standards

• Adjacent-boolean swap hazard (timedOut/permanentlyFailed threaded through four functions) — fixed: collapsed to a single MarkAbortReason discriminant; headline built in one place; reportMarkFailures takes a ready string; autoSyncEnabled passed as an options object.
• Duplicated isSystemic && isPermanent classification — fixed: extracted isPermanentApiFailure into the API seam (api.ts), reused by the mark-synced and delete paths.
• Over-commenting — trimmed the most redundant comment blocks.

Tests

• Added the abort batch-abort coverage (first batch, later batch with pending/marked counts, permanent-outranks-timeout, timeout-keeps-daemon, per-record-MARK_FAILED-does-not-abort), a direct isPermanentApiFailure guard test, and a one-shot "does not claim auto-sync stopped" test. Extracted a shared arrangeBatchedMarkSync helper (rule of three). Tightened a transient-headline assertion to match text unique to the generic branch.

Deferred (out of scope, flagged as a follow-up in the PR body)

• A sustained 429 still fires every remaining mark-synced PATCH. This is pre-existing behavior; changing it is a separate concern from the permanent-failure fix (and an earlier attempt to abort on any transient systemic error was reverted because a lone 5xx blip would strand records that would have settled). Narrowing an abort to rate-limit-only is tracked as a @todo in code and a follow-up suggestion on this PR.

[grimicorn]

Fix conflicts

[grimicorn-agent]

Agent code review trail

Independent Opus reviewer run in a loop over the merge + fixes. Summary per round (findings condensed; each was verified against tsc, eslint, prettier, and the 791-test suite before the next round).

Round 1

• Transient systemic abort indistinguishable from a completed run → fixed: added 'transient' to MarkAbortReason and a matching headline branch, so a 429/5xx abort reads as "stopped the run early" rather than a scatter of per-record failures.
• abort and abortReason derived by two independent guards (could disagree if isPermanent widened past isSystemic) → fixed: permanence classified once, folded into the abort decision.
• Stale @todo describing already-implemented 429 back-off → fixed: removed (the bulk path already backs off on a systemic 429).
• 25-record test comment misstated chunk size → fixed: reworded (the short outcomes array is a mocked abort stand-in).

Round 2 — reviewed a stale diff (uncommitted round-1 fixes weren't in origin/main...HEAD); all four findings were already addressed. No action.

Round 3

• Transient headline made two unconditional claims that can be false ("never attempted" on a last-chunk abort; "retried next run" on a one-shot sync) → fixed: hasUnattempted gates the skipped clause; the retry wording was later aligned to the soft "may be re-written next run" hedge the timeout/generic branches already use (round 5).
• abortReason: null test name/comment now misleading (a 5xx maps to 'transient', not null) → fixed: renamed to the per-chunk (non-systemic) case; added a sibling asserting no overclaim.

Round 4

• abort boolean redundant with abortReason !== null; type could express a contradiction → fixed: dropped abort; the loop gates on abortReason !== null, so "aborted" and "why" can't disagree.
• Stale doc comment → fixed.
• Daemon-stop signal only returned from the failure branch → fixed: the success path returns the same computed signal (one source).

Round 5 (final)

• Two stale comments (header + MarkSyncedChunkResult doc still referenced the removed abort / said transient aborts with null) → fixed.
• Outer sync catch re-derived permanence via error.isPermanent → fixed: routed through the shared isPermanentApiFailure seam, matching the mark-synced and delete paths.
• Permanent headline omitted the "never attempted" nuance the transient branch has → fixed: added, gated on hasUnattempted.
• 'auth/' in the transient-branch comment can't reach that branch (a 401/403 is caught by the permanent guard above) → fixed: dropped.
• Reviewer confirmed no correctness or security defect in the abort/daemon-stop control flow.

Skipped (with reason)

• logErrorMessage prints server-controlled error.message to stderr without sanitizeForTerminal, unlike every other server-derived print. Real but pre-existing and cross-cutting (several callers); best fixed uniformly inside logErrorMessage, not in this merge. Recorded as a follow-up suggestion in the PR body.
• Rule-of-three helper for permanent ? false : autoSync. Two of the three sites are main's pre-existing delete/outer-catch paths acting on different variables; abstracting would refactor untouched adjacent code out of scope.

lint / typecheck / build / 791 tests green.

[grimicorn-agent]

Resolved the merge conflicts with main (bulk mark-synced, #128). Conflicts were present in src/index.ts, src/libs/records.ts, tests/index.test.ts, and tests/libs/records.test.ts. Main had replaced the per-record PATCH with chunked markRecordsSynced; I preserved both sides' intent by moving the permanent-failure handling onto the bulk path (permanence is now a run-level MarkAbortReason, not the removed per-record MARK_PERMANENTLY_FAILED outcome). Discarded no work from either side. The only human feedback was "Fix conflicts" — done. lint/typecheck/build/791 tests green.