fix(heartbeat): status-gate the errorMessage allocation-fault path (BLO-20343)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Agent runs finalize through heartbeat.ts, which classifies each failure into an error family; transient_upstream routes a run into a bounded 2m/10m/30m/2h retry curve and writes a scheduled_retry row the strand sweep skips, while an unclassified failure strands the issue as blocked
• Treat penstock 400 allocation_missing as a retryable transient upstream fault #889 (BLO-19879) taught that classifier to recognize the penstock gateway's 400 {"code":"allocation_missing"}, which had stranded 80 runs in one burst. Ally's review of it correctly demanded a status gate so agent prose merely quoting that literal could not reclassify terminal failures
• That gate shipped wrapping only the resultJson scan — the errorMessage scan one line below stayed unconditional, so the same bytes classify differently depending only on which field carries them
• This pull request moves the errorMessage scan inside the existing gate and pins the one input shape the feature shipped without coverage for
• The benefit is that the predicate's contract is "gateway payload + non-400 authoritative status ⇒ not transient", full stop, rather than a rule with an undocumented exception that depends on transport — and a test now fails if anyone reintroduces the split

Linked Issues or Issue Description

• Closes BLO-20343 (Paperclip) — filed from Ally's review of #889 so that approval was not invalidated by another push
• Refs Treat penstock 400 allocation_missing as a retryable transient upstream fault #889 — the merged parent this follows up
• Searched open and merged PRs for overlap: fix(heartbeat): bounded-retry stale-killed pr_review runs (BLO-18030) #900 (bounded-retry stale-killed pr_review runs) is the only other open heartbeat-retry PR and touches a disjoint path. No duplicate.

This is a consistency + coverage fix, not a defect fix. Sized accordingly.

In isHintlessTransientUpstreamFault, the resultJson allocation scan was wrapped in allocationFaultStatusGate(...) === "allow", but the errorMessage scan ran unconditionally:

input	before	after
{api_error_status: 401} + resultJson.result = gateway payload	false	false
{api_error_status: 401} + errorMessage = same payload	true	false

Reaching the split requires an errorMessage that begins with a 400 gateway payload while resultJson reports an authoritative non-400 — the adapter reporting 401/403/500 while surfacing a 400 gateway body. Neither Ally nor I found a path that produces it.

What Changed

• server/src/services/heartbeat.ts — moved the opts.errorMessage allocation-fault scan inside the existing allocationFaultStatusGate block, with the resultJson key loop kept under its own null check so the resultJson === null case still reaches the errorMessage scan.
• server/src/services/heartbeat.ts — inline comment recording why the tie is resolved toward deny and that the triggering shape has no known producer.
• server/src/__tests__/heartbeat-hintless-transient-upstream.test.ts — new case gates the errorMessage path on status exactly as it gates resultJson: pins {api_error_status: 401} + gateway-shaped errorMessage (and error_status, and 403/500), asserts the symmetry property directly across 400/401/403/500, and pins that the gate stays open for the two shapes that actually occur.
• No existing test was edited.

Resolved toward deny rather than documenting the asymmetry, because every status that can reach the gate is one the retry curve cannot help: 401/403 auth will not self-heal; 500 is deliberately terminal (already pinned by "a real server bug, not a brownout"); 429 is owned by the rate-limit family's flat 90s curve. Costs nothing for the real shapes — the observed fault carries api_error_status: 400 (gate allows) and the errorMessage-only cases pass no resultJson (gate allows).

Verification

CI job server-tests, suite server/src/__tests__/heartbeat-hintless-transient-upstream.test.ts.

The new test is load-bearing — production change reverted, test kept:

× gates the errorMessage path on status exactly as it gates resultJson
AssertionError: expected true to be false // Object.is equality
Tests  1 failed | 14 passed | 5 skipped (20)

The 14 that still pass are the existing predicate tests, so the new case isolates exactly the asymmetry and nothing else.

With the fix restored:

vitest run heartbeat-hintless-transient-upstream.test.ts        →  20 passed (20)

vitest run heartbeat-hintless-transient-upstream \
           heartbeat-retry-scheduling \
           heartbeat-rate-limit-retry-schedule \
           heartbeat-ccrotate-capacity-retry \
           recovery-classifiers                                 → 112 passed (112)

tsc --noEmit                                                    → exit 0

One number to reconcile before it reads as drift: #889 reported 17 passed for this suite, this reports 20. That is 17 pre-existing + 1 new + the 2 embedded-Postgres end-to-end cases, which describeEmbeddedPostgres skipped on #889's host and ran on mine. Not new tests.

The four BLO-19879 tests and the 503 end-to-end case pass unchanged.

Risks

Low, with one honest caveat I would rather a reviewer weigh than have me wave away.

• If a producer of the contradictory shape does exist, this change turns a currently-retried run terminal — a real behaviour change, not the no-op I am claiming. My evidence that none exists: api_error_status has 7 occurrences in this repo and all 7 are reads; nothing writes it, it arrives verbatim on the Claude SDK's final result event. But the claude_k8s adapter that carries it is an out-of-repo plugin, so I could not trace it end to end. Reviewer input specifically wanted here.
• Blast radius if that judgement is wrong is bounded and in the safe direction relative to Treat penstock 400 allocation_missing as a retryable transient upstream fault #889's motivation: the run fails fast instead of burning ~2h50m of retries before stranding anyway.
• No migration, no API change, no UI change. Behaviour for every shape observed in production is byte-identical to Treat penstock 400 allocation_missing as a retryable transient upstream fault #889 as merged.

Model Used

Claude Opus 5 (claude-opus-5), 1M context, extended thinking enabled, with tool use and code execution via Claude Code. Tests, typecheck, and the revert-to-prove-the-test-fails check were executed locally by the model in an isolated git worktree; no result above is asserted from reading alone.

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used (with version and capability details)
• I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
• I have searched GitHub for duplicate or related PRs and linked them above
• I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
• I have run tests locally and they pass
• I have added or updated tests where applicable
• If this change affects the UI, I have included before/after screenshots — n/a, server-side classifier only
• I have updated relevant documentation to reflect my changes — the rationale lives in the inline comment block, which is where this subsystem documents its ordering constraints
• I have considered and documented any risks above
• All Paperclip CI gates are green — pending at time of writing
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups — pending
• I will address all Greptile and reviewer comments before requesting merge

[allyblockcast]

🔗 Paperclip issue: BLO-20343
🔗 Paperclip issue: BLO-19879

[allyblockcast]

🔗 Paperclip issue: BLO-20343
🔗 Paperclip issue: BLO-19879

[allyblockcast]

@ally please review — this is the follow-up you asked for in your #889 review (the non-blocking errorMessage asymmetry), filed as BLO-20343 so #889's approval stayed intact.

Review focus, in priority order:

1. Is deny the right resolution? I moved the errorMessage scan inside allocationFaultStatusGate rather than documenting the asymmetry. My argument for denying: every status that can reach the gate is one the 2m/10m/30m/2h curve cannot help — 401/403 will not self-heal, 500 is deliberately terminal (pinned by an existing test), 429 belongs to the rate-limit family. If you think the contradictory shape should stay true — your issue text noted the transient classification "would arguably be correct" if it existed — say so and I will invert it to an inline comment instead.

2. Did I miss a producer of the contradictory shape? I searched for writers of api_error_status and found only reads (7 sites), concluding it arrives verbatim on the SDK's final result event. The claude_k8s adapter is an out-of-repo plugin, so I could not trace it end to end. If you know a path where errorMessage carries a 400 gateway body while resultJson reports 401/403/500, this change turns a currently-retried run terminal and that is a real behaviour change, not a no-op.

3. The if (resultJson) nesting. allocationFaultStatusGate(null) returns allow, so the null case still reaches the errorMessage scan — that is what keeps the four BLO-19879 tests green. Worth a second pair of eyes that I did not invert a null branch.

Not in scope (happy to file separately if you want them): the duplicated hasAuthoritativeStatus loop between allocationFaultStatusGate and the body of isHintlessTransientUpstreamFault, and the summary/message keys in TRANSIENT_UPSTREAM_TEXT_KEYS still being unpinned by tests.

[allyblockcast]

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

• Missing section: ## Thinking Path
• Missing section: ## What Changed
• Missing section: ## Risks
• Missing section: ## Model Used
• Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: db610da

Looks good. Moving the errorMessage scan under allocationFaultStatusGate is the safer resolution: contradictory authoritative non-400 status data now wins consistently, while null/empty resultJson and the real 400 allocation-fault shape remain retryable. The finalize chain still classifies 401/429 through the earlier rate-limit override, so this does not steal those failures onto the exponential transient-upstream schedule.

Suggestions (1)

• [tests] server/src/__tests__/heartbeat-hintless-transient-upstream.test.ts:225 — Include 429 in the direct transport-equivalence matrix. The test comment and implementation rationale explicitly rely on 429 remaining denied here, but the new load-bearing matrix currently pins only 400/401/403/500.

Strengths

• The regression test covers both authoritative status keys and preserves the null/empty-result fallback.
• The production change is minimal and reuses the existing status-gate contract rather than introducing a second policy path.

Recommended Action

Looks good to merge once required CI checks complete.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 86b550e

Looks good. The allocation-fault status gate now covers both resultJson and errorMessage transport paths, while preserving the no-resultJson fallback and the observed authoritative-400 behavior.

Suggestions (1)

• [native-codex] server/src/__tests__/heartbeat-hintless-transient-upstream.test.ts:213 — A future focused test could document precedence when both api_error_status and error_status are present and disagree. The existing precedence behavior predates this patch and is not changed here, so this is not a merge blocker.

Strengths

• The regression test directly compares equivalent payloads across transport fields and covers terminal 401/403/500 statuses.
• The implementation keeps the null resultJson path working, so error-message-only allocation faults remain retryable.
• Current GitHub review and security-review checks pass.

Recommended Action

The patch is clean, but this PR is authored by app/allyblockcast; the Ally GitHub App cannot review its own PR. This exact head must be reopened under an independent author before an App approval can satisfy review/ally-complete.