Abort mark-synced on a repeated non-transient 4xx

[grimicorn-agent]

What & why

markpost sync's mark-synced step PATCHes written records synced. Before this change a request-shape failure — a malformed payload (400) or a contract-validation rejection (422, e.g. markpost tightening the PATCH attributes it accepts) — failed a chunk and every remaining chunk fired and failed identically. This aborts the run once it has evidence the request itself is categorically wrong, instead of firing more doomed PATCHes.

Closes #137

Merged with main's bulk mark-synced refactor (#128). main replaced the per-record PATCH loop this PR was originally built on with a single bulk PATCH /api/records chunked at 100 records. The original per-record probe machinery (single-record loop, markSyncedStopReason/probeStopReason, last-record confirmation probe) no longer applies — the bulk endpoint validates the whole envelope and 422s before any write, and drops foreign uuids silently rather than 4xx-ing per record, so a 400/422 there is categorical by nature. This PR's contribution is re-expressed natively on the bulk model (below); no work from either side is dropped, and main's bulk chunking/partial-success detection is preserved.

How it works

• New classification (src/libs/api.ts). ApiRequestError.isFatalRequest + the isFatalRequestError guard flag only 400/422 as request-shape failures. A transient 429, a per-record 404, an auth 401/403, and any 5xx are deliberately excluded — they don't mean the request shape is wrong, so they never trigger a request-shape abort (429/401/403/5xx still abort as systemic, main's existing behavior).
• New outcome MARK_ABORTED (src/libs/records.ts). A bulk chunk rejected with a request-shape 4xx is tagged MARK_FAILED while the run continues; only the chunk that actually stops the run is re-tagged MARK_ABORTED, so a completed run never leaves a stray MARK_ABORTED. A non-JSON error body at any status still degrades safely to MARK_FAILED.
• Two-chunk confirmation (markRecordsSynced). The run aborts only once a second consecutive chunk is rejected with the SAME error message and nothing has synced yet. The CLI builds every chunk's payload identically, so two independently-built chunks failing the same categorical way is strong evidence the envelope shape is wrong. A lone rejection, two rejections with different messages (which look like isolated per-record problems), a rejection separated from the last by a clean/other-failure chunk, or any rejection after a success (a success proves the shape valid) all keep the run going rather than strand syncable records behind an unconfirmed abort.
• Reporting (src/index.ts). An abort gets its own headline; the "the rest were not attempted" clause is dropped when the abort landed on the final chunk (nothing left unattempted), and a systemic early stop notes how many records were never sent.

Key decisions

• Require two matching rejections, not one: aborting on a single (or the first) 400/422 could strand every trailing chunk if a chunk ever rejects for a per-record reason. Requiring a second chunk to fail with the same message keeps the abort evidence-based; the cost of not aborting early is only a few extra doomed requests, which is far cheaper than an unrecoverable strand.
• Honest per-record outcomes: request-shape chunks are MARK_FAILED unless the run actually aborts, so an outcome named "aborted" only ever appears on a run that did abort.
• Discarded the per-record probe machinery: it existed solely to disambiguate per-record 4xx from request-shape 4xx in a per-record loop; the bulk endpoint has no per-record 4xx, so the probe/unanimity/markSyncedStopReason/probeStopReason scaffolding is obsolete and was removed rather than reverting main's bulk refactor.

Tests

tests/libs/api.test.ts, tests/libs/records.test.ts, tests/index.test.ts: status-code boundaries (400/422 → request-shape; 404/401/403/429/5xx don't); the two-chunk-confirmation state machine (two matching rejections abort; different messages / interleaved failure / post-success rejection do not); the honest MARK_FAILED→MARK_ABORTED re-tagging; and the abort/timeout/systemic reporting paths. npm run lint, npm run typecheck, and 795 tests pass.

Viewable

CLI behavior; exercised via markpost sync (the mark-synced step when autoDelete is off). No UI/URL.

Follow-up suggestions

• Classify the systemic mark-synced stop in the report — a systemic abort (401/403/429/5xx) falls to the generic "Failed to mark N synced" headline with no classified cause; describeSystemicFailure already produces the actionable text (e.g. "Authentication failed (HTTP 401)…") and could be surfaced there so a cron log says why the run stopped (suggested: P3, effort: S, evidence: src/index.ts markFailureHeadline / src/libs/records.ts markRecordsSynced systemic branch)

[grimicorn-agent]

Independent code review trail

An independent Opus reviewer (which did not write the code) reviewed the diff over multiple rounds; each round's findings were applied before the next. Summary:

Round 1 — classification too broad. Original design aborted on any non-429 4xx. Flagged that a 404 (a record deleted server-side between fetch and PATCH) is per-record, not request-shape, and would wrongly abort. Fixed: narrowed the fatal set to an explicit [400, 422]; added 404/401/403 guard tests.

Round 2 — single 4xx aborted the whole run. A lone per-record 422 would strand every later record forever (the pending set re-fetches in the same order and re-aborts). Fixed: abort only when a whole batch is unanimously rejected AND nothing has synced yet.

Round 3 — one-record tail batch / empty batch. every() is trivially true on a 1-element (or empty) batch. Fixed: added length > 1 guard; extracted the pure markSyncedStopReason and unit-tested the boundaries.

Round 4 — contiguous bad block > batch width; headline over-claimed. A rejected batch that lands after unrelated failures mis-attributed the cause. Fixed: neutral headline; corrected anySynced capture.

Rounds 5-7 — abort had no confirmation. Aborting on the first rejected batch still stranded a contiguous per-record-bad block. Fixed: added a confirmation probe; then made it probe the last record (far from a front-loaded bad block); then made it report plain failures when nothing is left to strand. Added probeStopReason (pure, unit-tested) and scale-coverage tests.

Rounds 8-10 — probe robustness. Fixed: probe capped at once per invocation (no hammering a rate-limited record); a probe-synced record tracked via probeSyncedIndex so a later stop doesn't mis-report it as pending; abort headline reworded to describe what was actually observed.

Final round — control flow confirmed sound. The reviewer traced the boundary cases (single-record tail batch, rejected final batch, probe timeout, probe-synced index surviving a later stop) and each lands correctly.

Intentionally not actioned (recorded as a follow-up in the PR body): confirming the abort with a single last-record probe leaves a rare tail case — if that last record is itself the one per-record-bad record, the abort is a false positive. A two-record probe would remove it. Left as a scoped follow-up rather than expanding this PR.

npm run lint, npm run typecheck, and 740 tests pass.

[grimicorn]

Fix conflicts

[grimicorn-agent]

Independent code review trail (post-merge)

After merging main (the bulk mark-synced refactor #128) and re-expressing this PR's abort-on-request-shape-4xx intent on the bulk model, an independent Opus reviewer (which did not write the code) reviewed the diff across four rounds; each round's findings were applied before the next.

Round 1 — abort-on-first-4xx could strand records. Aborting the whole run on the first 400/422 chunk assumed the bulk endpoint never 4xxs per-record. Also flagged: stale api.ts doc comments referencing the removed markSyncedStopReason, and a redundant abort/stoppedBy pair that could drift. Fixed: replaced the immediate abort with a two-chunk confirmation, collapsed the redundant fields into one ChunkStop discriminant, corrected the api.ts comments.

Round 2 — two-strike still not evidence-based. Two different per-record rejections across two chunks could satisfy a bare two-strike rule without being an envelope fault. Also: an abort on the final chunk claimed "the rest were not attempted" when everything was attempted. Fixed: require the two request-shape rejections to carry the same error message (envelope-level evidence); drop the "not attempted" clause when nothing was left unattempted.

Round 3 — consecutiveness + honest outcome tags. A non-request-shape chunk between two identical rejections shouldn't count toward the abort; and tagging records MARK_ABORTED on a run that then completes is a trap for future consumers. Fixed: reset the message tracker on any non-request-shape chunk (so the match is truly consecutive); tag request-shape chunks MARK_FAILED and re-tag only the actually-stopping chunk MARK_ABORTED; named the STOP_* discriminants; bundled the report's reason + unattempted-count into one object.

Round 4 — systemic early stop wording. A systemic abort (401/403/429/5xx) fell to the generic "Failed to mark N synced" headline, implying all N were attempted. Fixed: the generic branch now notes how many pending records were never sent when the run stopped early. Also hoisted a duplicated MARK_FAILED outcome list.

Intentionally not actioned:

• Reset anySynced on a non-request-shape chunk (round 4 suggestion): rejected — once any chunk has synced, the envelope is proven valid for the whole run, so resetting it would let a later two-chunk rejection abort a run that already proved the shape works. That would be a regression.
• Carry the classified systemic cause into the headline: deferred as a scoped follow-up (recorded in the PR body) rather than expanding this PR — describeSystemicFailure already produces the text; wiring it through the report is a small separate change.
• "No tests" finding: the reviewer was fed only the src/ diff, so it couldn't see the test changes; tests/libs/records.test.ts and tests/index.test.ts cover every new branch (two-chunk abort, different-message/interleaved/post-success no-abort, honest re-tagging, final-chunk unattempted-0, systemic never-attempted, 404/429 non-fatal).

npm run typecheck, npm run lint, and 795 tests pass.