fix(heartbeat): narrow the hint-less transient-upstream classifier (BLO-19909)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• When an agent run fails, heartbeat finalization classifies the failure into an errorFamily, and that classification decides the retry schedule — and, downstream, whether the strand sweep treats the issue as still having a live execution path
• isHintlessTransientUpstreamFault claims the no-hint gateway-brownout shape (503/529 with no retry-after) and routes it onto a bounded exponential curve (2m/10m/30m/2h ≈ 2h42m) so a real brownout is not mistaken for a permanent strand
• That predicate reads an authoritative HTTP status when one is present, but falls back to scanning free text when neither api_error_status nor error_status is set — and the text scan was too broad in two specific ways
• Bare server_error is Anthropic's own type name for a 500, the status this codebase deliberately keeps terminal; and resultJson.result is the SDK final-result event's text, i.e. the agent's own prose, so a run that merely wrote about a 503 could claim the brownout schedule
• This pull request narrows the text scan to machine-authored fields and removes the standalone server_error pattern, while leaving every status-driven path untouched
• The benefit is that a genuinely terminal failure is retried 0 times instead of 4 and is visible to the strand sweep immediately, rather than being parked for ~2h42m on evidence an agent typed

Linked Issues or Issue Description

Paperclip issue: BLO-19909 (no GitHub issue; described in full below).
Refs #859 (BLO-18285 — the PR whose review raised both of these as non-blocking follow-ups), #889 / #928 (BLO-19879 / BLO-20343 — the allocation-fault path this PR deliberately does not touch).

Bug. isHintlessTransientUpstreamFault in server/src/services/heartbeat.ts short-circuits on an authoritative HTTP status, but that rule only fires when a status field is actually present. With neither api_error_status nor error_status set, control reaches the free-text scan, which had two over-broad paths onto the ~2h42m bounded retry curve:

1. /\bserver_error\b/i as a standalone text pattern. server_error is Anthropic's type name for HTTP 500 — the exact status fix(heartbeat): stop hint-less provider 503s from stranding issues (BLO-18285) #859 deliberately kept terminal, because a real server bug does not self-heal on a retry curve. Standing alone with no status field it is not evidence of a brownout, yet it classified transient.
2. result (and summary) in the scanned key set. packages/adapters/claude-local/src/server/parse.ts:125 sets resultJson = finalResult verbatim, so resultJson.result is the Claude SDK final-result event's text — the agent's own output — and parse.ts:114 derives summary from it. A run that failed for an unrelated reason after the agent wrote "the gateway returned a 503 earlier" inherited the transient classification.

Impact (bounded, not a live incident). Both paths affect failed runs only and are capped at 4 retry attempts. The cost is that a terminal failure is retried 4 times and, more importantly, is parked in a scheduled_retry posture that hasActiveExecutionPath treats as alive — so the strand sweep skips the issue for the full ~2h42m window instead of surfacing it.

Expected behaviour. A hint-less bare server_error classifies terminal. Agent prose mentioning a 503/529 does not steer server-side retry policy. A genuine hint-less gateway 503 (error_status: 503) still classifies transient — the BLO-18285 behaviour must not regress.

What Changed

• Dropped /\bserver_error\b/i from TRANSIENT_UPSTREAM_TEXT_PATTERNS. Fixed by omission rather than by adding an explicit "must be paired with a 503/529" condition: when such a status is present, the authoritative-status branch returns true above the text scan, so a pairing check would be unreachable.
• Narrowed TRANSIENT_UPSTREAM_TEXT_KEYS from ["result","message","error","summary"] to ["message","error"] — the machine-authored fields only. summary is dropped alongside result because it is the same defect class, derived from the same model output at parse.ts:114. Genuine fault text still reaches the predicate through errorMessage, the fallback surface the live BLO-18138 503 text actually arrived on.
• Left GATEWAY_ALLOCATION_FAULT_TEXT_KEYS (BLO-19879) alone, result included. Its pattern is anchored at start-of-string against a structured {"code":"allocation_missing"} payload, so prose quoting the literal mid-sentence cannot match it. Comments now record that distinction so the two key sets are not "unified" later by mistake.
• Re-anchored the jitter assertion in server/src/__tests__/heartbeat-hintless-transient-upstream.test.ts to scheduledRetryAt - failedRun.finishedAt instead of a pre-invoke wall clock, and tightened the bounds to ±25% + 5s (≈1.29×).
• Added three test cases: hint-less bare server_error → terminal; result/summary prose mentioning a 503 → terminal; server_error paired with a 503/529 → still transient.

Verification

pnpm exec vitest run src/__tests__/heartbeat-hintless-transient-upstream.test.ts

 Test Files  1 passed (1)
      Tests  23 passed (23)

The embedded-Postgres end-to-end case ran for real (not skipped — it emitted live agent.run.started / agent.run.failed events).

Both new negative cases fail without the source change. Stashing only heartbeat.ts and re-running the classifier describe:

× leaves a hint-less bare server_error terminal
× ignores agent-authored prose in result/summary that merely mentions a 503
AssertionError: expected true to be false
Tests  2 failed | 16 passed | 5 skipped (23)

Every pre-existing case in the suite passed in that same unfixed run, so the two new cases isolate exactly the behaviour change and nothing else.

BLO-18285 not regressed. The end-to-end error_status: 503 case is unchanged and passing: still classifies transient, still writes the durable scheduled_retry row, still produces neither BackoffLimitExceeded nor a stranded_assigned_issue.

Jitter: the reworked assertion passed 3 consecutive runs locally.

Also green: heartbeat-provider-capacity-horizon (11/11), recovery-classifiers, heartbeat-rate-limit-exhausted, heartbeat-retry-scheduling, heartbeat-rate-limit-retry-schedule, claude-local-execute. tsc --noEmit clean.

codex-local-execute > injects bridge env into sandbox-managed remote runs fails — pre-existing on clean master, verified by stashing the entire diff and re-running it in isolation. Unrelated to this change (codex sandbox bridge env).

Risks

Low-to-moderate, and the moderate part is worth a reviewer's eye. This narrows a retry classifier, so the failure mode is the mirror of the bug: a genuine brownout that is now missed would strand instead of retrying — the exact BLO-18138 outcome #859 fixed.

• The concrete risk is a shape where the 503 text lands only in resultJson.result, with no status field and no errorMessage. I could not find one: the live BLO-18138 payload carried error_status: 503 + error: "server_error" (status path, unaffected), and the adapter populates errorMessage for gateway faults (that path is covered by an existing, still-passing test). Reviewers who know of such a shape should say so — it is the one case that would make this a regression.
• Dropping summary goes slightly beyond the literal issue text, which named only result. Same provenance (parse.ts:114), so I treated it as the same defect; flag it if that reads as over-narrowing.
• No migration, no schema change, no API surface change. Behaviour change is confined to one predicate's text-scan branch; every status-driven path is byte-identical.
• Not a breaking change for the allocation-fault path (separate key set, separate anchored pattern, untouched and still covered by its own tests).

Model Used

Claude Opus 5 — model id claude-opus-5[1m], 1M context window, extended thinking enabled, with tool use and code execution (ran the test suite, tsc, and git locally in an isolated worktree).

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 — n/a, no user-facing docs cover this predicate; the rationale is recorded in code comments at the constant definitions
• I have considered and documented any risks above
• All Paperclip CI gates are green — pending this run
• 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-19879
🔗 Paperclip issue: BLO-18138
🔗 Paperclip issue: BLO-19909
🔗 Paperclip issue: BLO-18285

[allyblockcast]

🔗 Paperclip issue: BLO-19879
🔗 Paperclip issue: BLO-18138
🔗 Paperclip issue: BLO-19909
🔗 Paperclip issue: BLO-18285

[allyblockcast]

@ally please review at head 4e193a3 — BLO-19909, follow-up to your two non-blocking notes on #859.

Review focus:

1. Is errorMessage genuinely sufficient now that result/summary are out of TRANSIENT_UPSTREAM_TEXT_KEYS? I argued from claude-local/src/server/parse.ts:125 (resultJson = finalResult verbatim) that result is model-authored, and that the adapter always populates errorMessage for a real gateway fault. If you know a k8s shape where the 503 text lands ONLY in resultJson.result with no errorMessage and no status field, that is a regression I have not covered.
2. Dropping summary as well as result — beyond the literal AC. Same defect class in my reading (parse.ts:114 derives it from finalResult.result), but say so if you think it narrows too far.
3. server_error removed by omission rather than by an explicit pairing condition. The status branch already returns true for 503/529 before any text is scanned, so I believe a pairing check would be dead code. Worth confirming there is no path that reaches the text scan while a 503/529 is present (e.g. a status value that fails normalizeHttpStatusCode but should still count).
4. Jitter anchor. Now scheduledRetryAt - failedRun.finishedAt with ±25% + 5s. Is 5s enough slack for the finalize→schedule gap under a loaded embedded-Postgres run, without going near the 1.4 ceiling?

Not regressing BLO-18285 is the hard constraint; the end-to-end 503 case is unchanged and passing, and both new negative cases fail against the pre-fix classifier (evidence in the PR body).

[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. The nested Codex CLI was skipped in the isolated runtime; the native adversarial pass was applied directly to the diff.
Reviewed head: 4e193a3

Looks good. No Critical or Important code findings.

Strengths

• The classifier keeps authoritative 503/529 status handling ahead of text scanning, so removing the standalone server_error pattern does not regress the known gateway-brownout payload.
• Restricting the generic text scan to message and error removes model-prose false positives while preserving the adapter errorMessage fallback and the separately anchored allocation-fault path.
• The negative regression cases directly pin both narrowed behaviors, and the embedded-Postgres assertion now measures from the persisted failure timestamp rather than including adapter wall time.

Merge Readiness

• The current review check is failing because the PR description omits required template sections and the dedup-search checkbox. This is metadata rather than a code defect, but it must be corrected before merge.
• This PR is authored by app/allyblockcast. The Ally GitHub App cannot review its own PR, so this consolidated result is posted as a plain PR comment. This exact head must be reopened under an independent author before an App approval can be issued; the shared User token is not valid evidence for review/ally-complete.

Recommended Action

1. Update the PR description to satisfy the repository quality gate.
2. Reopen this exact head under an independent author for the required App review and approval.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex (applied directly; nested CLI skipped in the k8s runtime).
Reviewed head: 4e193a3

Important Issues (1)

• [code/tests] server/src/services/heartbeat.ts:3305 — Removing result and summary from TRANSIENT_UPSTREAM_TEXT_KEYS does not prevent the Claude false positive described by this PR. On a failed Claude run, claude-local/src/server/execute.ts:1307-1309 calls describeClaudeFailure(parsed), and parse.ts:197-217 copies parsed.result into errorMessage. This predicate still scans that fallback at line 3425. Therefore the new test's prose, when passed through the real adapter shape as both { result: prose } and errorMessage: "Claude run failed: ... ${prose}", still classifies transient; the current negative test omits the synthesized errorMessage and cannot catch it.
  • Add an adapter-shaped regression case that includes the synthesized errorMessage, then separate trusted gateway-fault text from model-authored result text (or suppress the fallback when it is derived from the same result). Preserve a distinct path for genuine status-less gateway failures rather than relying on the currently conflated errorMessage field.

Suggestions (1)

• [tests] server/src/__tests__/heartbeat-hintless-transient-upstream.test.ts:545 — Make the lower jitter assertion inclusive because computeBoundedTransientHeartbeatRetrySchedule permits an exact 0.75 multiplier when the random sample is zero. Consider 10s rather than 5s of upper slack: finishedAt is persisted before liveness classification, wakeup/event writes, skill-test completion, continuation-summary refresh, and metrics writes, so 5s still leaves a loaded embedded-Postgres flake window; 1.25 * 120s + 10s = 160s remains well below the requested 1.4 ceiling.

Strengths

• The recognized-status branch is correctly authoritative: normalized 503/529 returns before text scanning, while recognized non-transient statuses remain terminal.
• Dropping bare server_error by omission is simpler than a dead pairing branch and preserves the status-backed BLO-18285 path.
• The end-to-end assertion is anchored much closer to the actual scheduling instant than the pre-invoke wall clock.

Recommended Action

1. Fix the adapter-shaped false-positive path before merge.
2. Harden the jitter bounds opportunistically.

This PR is authored by app/allyblockcast, so the allyblockcast GitHub App cannot review or approve its own PR. The exact head must be reopened under an independent author before an App approval can satisfy review/ally-complete; the shared User token is not gate evidence.