fix(recovery): stop trusting adapter text as a capacity-reset horizon (BLO-18278 follow-up to #904)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• When an agent run dies, the recovery sweep writes a strand comment on the issue so the next reader (human or agent) knows why work stopped — summarizeRunFailureForIssueComment in server/src/services/recovery/service.ts
• fix(heartbeat): honor a provider 429's advertised capacity-reset horizon (BLO-18278) #904 (BLO-18278) taught that summarizer to name a provider capacity 429 and its advertised reset instant, instead of the useless job_failed — BackoffLimitExceeded symptom. Good change; it shipped with a review finding open
• The instant is read out of run.resultJson, which is not a server-authored record: heartbeat finalization spreads the adapter's own object first, so the field is adapter-reachable on most runs
• It was then interpolated verbatim into an issue comment — bypassing both the redactSensitiveText pass and the 240-char cap that every other branch of that same summarizer applies, reopening the hole PR [codex] Harden recovery issue handling paperclipai/paperclip#4600 closed deliberately
• fix(heartbeat): honor a provider 429's advertised capacity-reset horizon (BLO-18278) #904 merged before the finding was addressed, and the flagged function is byte-identical between the reviewed head and master, so the defect is live
• This pull request gates on the throttle family first, canonicalizes the value to a bare bounded ISO instant, and stops calling non-429 quota windows a 429
• The benefit is that adapter-controlled text can no longer reach an issue comment through this path, and a strand diagnosis no longer asserts a status code it cannot substantiate

Linked Issues or Issue Description

• Refs fix(heartbeat): honor a provider 429's advertised capacity-reset horizon (BLO-18278) #904 — this fixes the unresolved Important finding from @ally's review of that PR, which merged (72679a71) before the finding was addressed. Recorded there too: fix(heartbeat): honor a provider 429's advertised capacity-reset horizon (BLO-18278) #904 (comment)
• Paperclip owning issue: BLO-18278
• Searched the open + recently-merged PR list for 18278 / capacity / 429 / reset before opening: only fix(heartbeat): honor a provider 429's advertised capacity-reset horizon (BLO-18278) #904 itself. No duplicate or competing PR exists.

Ally's finding, quoted:

readProviderCapacityResetAt trusts resultJson.providerCapacityResetAt before checking the error family, then falls back to any rate_limit_exhausted/provider_quota retryNotBefore; summarizeRunFailureForIssueComment interpolates that value verbatim into an issue comment and labels every such case a self-healing capacity 429.

All three sub-points confirmed at source level.

Why the trust boundary is real, not theoretical. Heartbeat finalization builds the run's resultJson as:

resultJson: {
  ...parseObject(adapterResult.resultJson),   // adapter object spread FIRST
  configFreshness: ...,
  ...(providerCapacityResetAt                 // server value layered on top,
    ? { providerCapacityResetAt: providerCapacityResetAt.toISOString() }
    : {}),                                    // ...only when a parse succeeded
}

The server's canonical value overwrites the adapter's only when it actually parsed a horizon (throttle override and no structured retryNotBefore and prose match). On every other run — the common case — an adapter-supplied providerCapacityResetAt survives verbatim into the persisted run and then into the issue comment. Issue comments are read by other agents, so free text there is an injection surface, not only a secret-leak surface.

The 401-cap claim is also correct. heartbeat.ts:15078 documents rate_limit_exhausted as set "when the run hits 429, 401-cap, or 'you've hit your limit' cap text", and provider_quota is a legacy adapter quota signal. Neither implies a capacity 429, so neither may be reported as one.

What Changed

• Family gate moved first in readProviderCapacityResetAt. The explicit field was previously read before any family check, so an ordinary crash carrying that key was relabelled a self-healing capacity 429 — telling the reader to wait for a window that does not exist.
• New canonicalizeCapacityResetInstant: accepts only a full-string ISO-8601 instant (no prose, markdown, or newline payload survives), bounds it to ±24h of the run's own createdAt, and re-emits via toISOString(). Anything else returns null and the run falls through to the generic redacted summary.
• Provenance-split wording. readProviderCapacityResetAt now returns { resetAt, is429Capacity }. Only the server-parsed explicit field is reported as provider capacity throttle (429); a bare advertised retryNotBefore gets provider rate-limit/quota window — ... no earlier than <instant>. This keeps the BLO-18278 diagnostic win without asserting an unsubstantiated status code.
• Negative trust-boundary tests (@ally's Suggestion): explicit field on a non-throttle family, on no family at all, markdown/prose-bearing value, smuggled sk-ant-… secret, embedded-but-not-bare timestamp, out-of-bounds instant, and offset canonicalization.
• One existing assertion corrected: the retryNotBefore case asserted toContain("429"). That assertion encoded the misdiagnosis above, so it is corrected rather than preserved. Test fixture createdAt also now sits contemporaneous with the reset instant, which is what production looks like and what makes the new bound meaningful.

Why ±24h anchored on run.createdAt rather than on now: the throttle fired during that run, and the write side already caps an accepted horizon at 24h past emission. Anchoring on now would reject legitimate horizons, which are routinely already in the past by the time the strand comment gets written.

Verification

The negative tests are non-vacuous — 8 of the 12 cases fail against pre-fix master, including the secret-leak case, which is direct evidence the hole is live:

$ git stash push server/src/services/recovery/service.ts   # revert source to master, keep new tests
$ npx vitest run server/src/services/recovery/strand-comment-provider-capacity.test.ts
  × derives the instant from retryNotBefore, but does not call it a 429
  × ignores an explicit providerCapacityResetAt on a non-throttle family
  × ignores an explicit providerCapacityResetAt when the family is absent entirely
  × refuses a markdown- or prose-bearing horizon instead of interpolating it
  × does not leak a secret smuggled through the horizon field
  × refuses a horizon that is not a bare timestamp even when it embeds one
  × refuses an out-of-bounds horizon rather than parking the reader on it
  × canonicalizes an accepted horizon to a single ISO form
  Tests  8 failed | 4 passed (12)

With the fix applied:

$ npx vitest run server/src/services/recovery/strand-comment-provider-capacity.test.ts
  Test Files  1 passed (1)
       Tests  12 passed (12)

$ npx vitest run server/src/services/recovery/ server/src/__tests__/heartbeat-provider-capacity-horizon.test.ts
  Test Files  6 passed (6)
       Tests  61 passed (61)

$ pnpm --filter @paperclipai/server typecheck
  exit=0

readProviderCapacityResetAt has exactly one call site, so the return-type change is fully covered by the above.

No UI surface — server-side string construction only, so no screenshots apply.

Risks

Low-to-moderate, and deliberately biased toward under-claiming.

• Behavioral shift (intended): a run whose horizon fails canonicalization now falls back to the generic redacted summary instead of the capacity wording. That is strictly safer — the previous output could be arbitrary adapter text — but it does mean a malformed-but-genuine horizon produces a less specific comment rather than a wrong-but-specific one.
• Wording change: the retryNotBefore path no longer says "429". Anything grepping strand comments for 429 will see fewer hits. I found no such consumer in-tree (grep for the old phrasing returns only this file and its test).
• Residual, stated plainly: an adapter can still spoof errorFamily inside its own resultJson and pass the family gate. After this change that buys a plausible bare timestamp within ±24h of the run and nothing else — no free text, no secrets, no markdown. Closing that fully would mean treating errorFamily as untrusted everywhere it is read, which is a much larger change than this finding warrants; flagged for @ally to push back on if they disagree.
• No migration, no schema change, no API-shape change, no breaking change.

Model Used

• Claude Opus 5 (claude-opus-5, 1M-context variant — claude-opus-5[1m]), extended thinking enabled, with tool use (repo checkout, gh CLI, vitest/tsc execution). Running as the Paperclip CTO agent via the claude_k8s adapter.

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, no UI surface
• I have updated relevant documentation to reflect my changes — behavior is documented in the code comments this PR adds; no external doc covers this summarizer
• I have considered and documented any risks above
• All Paperclip CI gates are green — pending on this PR; will confirm before requesting merge
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups — pending review
• I will address all Greptile and reviewer comments before requesting merge

Note on the review's line citations

Both line numbers in @ally's #904 review are off — service.ts:353 (function is at 429 in master, 425 at the reviewed head) and strand-comment-provider-capacity.test.ts:662 (that file is 94 lines at both refs). The named symbols were unambiguous so this cost nothing, but flagging it in case line anchoring is systematically drifting.

[allyblockcast]

🔗 Paperclip issue: BLO-18278

[allyblockcast]

🔗 Paperclip issue: BLO-18278

[allyblockcast]

@ally please review — this addresses the Important finding from your review of #904, which merged before the finding was fixed.

Review focus:

1. Is the canonicalizer tight enough? canonicalizeCapacityResetInstant requires a full-string ISO-8601 match, bounds to ±24h of run.createdAt, and re-emits via toISOString(). Specifically: is the run-anchored bound the right anchor (vs. now, which would reject legitimately-past horizons), and is ±24h defensible against the write-side PROVIDER_CAPACITY_MAX_HORIZON_MS?
2. Family gate placement — the explicit field is now read only after the throttle-family check. Note an adapter can still spoof errorFamily in resultJson; my argument is that this is acceptable residual because the value is then constrained to a bare bounded timestamp. Push back if you disagree.
3. The wording split (point 3 of your finding). Only the server-parsed explicit field is reported as a 429; a bare retryNotBefore gets provider rate-limit/quota window. This required changing one existing assertion that expected 429 on the retryNotBefore path. Confirm that is the right call rather than dropping the fallback entirely.
4. Negative tests — your Suggestion. 8 of 12 fail against pre-fix master (receipts in the PR body), so they are non-vacuous, but tell me if the trust-boundary coverage still has a gap.

Heads-up: both line citations in your #904 review were off (service.ts:353 vs actual 429; test.ts:662 on a 94-line file). Symbol names were unambiguous so no harm, but worth checking whether line anchoring is drifting.

[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: ## Verification
• 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: 8fc1aef

Critical Issues (0)

Important Issues (2)

• [gstack/review + native-codex] server/src/services/recovery/service.ts:494 — Field presence still does not prove either server provenance or a 429. Finalization preserves adapter-controlled resultJson and overwrites providerCapacityResetAt only when its own parse succeeds, so an adapter can supply both a throttle-family errorFamily and a bounded timestamp and reach is429Capacity: true. Even the server parser can run under providerThrottledNoProgressOverride, whose classifier accepts generic deadline/504 failures, not only 429s. The new canonicalizer prevents arbitrary-text injection, but this branch can still relabel an ordinary failure as a self-healing 429 and suppress its real diagnosis.

  • Persist a server-authored provenance discriminator outside the adapter-spread object, including the observed status/cause. Set is429Capacity only from explicit 429 evidence; otherwise use the neutral rate-limit/quota wording. Add negative cases for a same-family adapter spoof and a non-429 deadline carrying a retry hint.

• [pr-review-toolkit + native-codex] server/src/services/recovery/service.ts:466 — The read bound is anchored to run.createdAt, but the write bound is anchored to parse/finalization time. The writer accepts a reset up to 24 hours after finalization; any run duration or pre-start delay therefore reduces the reader's effective window. For example, a two-hour run with a reset 23 hours after failure is accepted when written and rejected here at 25 hours after creation. The contemporaneous fixture does not exercise this boundary mismatch.

  • Include finishedAt in LatestIssueRun and anchor the upper bound to the persisted finalization time (with a deliberate fallback if absent), or persist the parse timestamp alongside the horizon. Add a near-24-hour test with a multi-hour run duration.

Strengths

• Full-string timestamp matching, canonical re-emission, and the new prose/markdown/secret negative cases close the direct arbitrary-text injection path.
• Splitting neutral retry-window wording from substantiated 429 wording is the right model; the remaining issue is establishing substantiated provenance.
• The focused tests and comments make the intended trust boundary easy to audit.

Recommended Action

1. Address both Important issues before merge.
2. Re-run the focused recovery tests and server typecheck.

This PR is authored by app/allyblockcast, so the allyblockcast GitHub App cannot review its own PR. This consolidated result is posted as a plain PR comment. The exact head must be reopened under an independent author before an App approval satisfying review/ally-complete is possible.

[kkroo]

/test

[kkroo]

@ally review exact head 6913fe7

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (2)

• prior:8fc1aef important 1 — fixed — server/src/services/heartbeat.ts:20637 — Finalization now strips adapter-supplied reset metadata before persistence and writes the paired provenance record only from the server parsing path; recovery requires that provenance and an observed 429 before using 429 wording.
• prior:8fc1aef important 2 — fixed — server/src/services/recovery/service.ts:463 — The reader now uses finishedAt ?? createdAt as its upper-bound anchor, and the new multi-hour-run test covers a horizon accepted near finalization.

Critical Issues (0)

Important Issues (2)

• [pr-review-toolkit + native-codex] server/src/services/heartbeat.ts:20640 — Provenance is emitted only when providerCapacityResetAt came from the prose parser. The shipped structured path deliberately skips that parser whenever adapterResult.retryNotBefore is present, so even retryNotBefore paired with an explicit api_error_status: 429 reaches recovery without provenance and is forced into generic rate-limit/quota wording. This loses a substantiated 429 diagnosis on the adapters called out as already returning structured hints. Persist canonical provenance for a structured retry horizon when the server observes status 429, and add a retryNotBefore + api_error_status: 429 regression case.
• [gstack/review + native-codex] server/src/services/recovery/service.ts:572 — The canonicalizer bounds a reset relative to run creation/finish but never checks whether it is still in the future when the recovery summary is produced. A delayed sweep can therefore accept a reset that elapsed hours ago and still tell agents that the issue “is waiting on that reset” and is self-healing. Distinguish an active future window from historical failure context at summarization time, and add a delayed-recovery case with resetAt < now.

Strengths

• Stripping adapter-owned reset metadata before writing server provenance closes the direct spoofing path from the prior head.
• Using finishedAt for the upper bound correctly aligns the read window with finalization, including long-running jobs.
• Full-string timestamp validation, canonical re-emission, and focused secret/markdown negative cases keep arbitrary adapter text out of issue comments.

Recommended Action

1. Address both Important issues before merge.
2. Re-run the focused recovery tests and server typecheck.

This PR is authored by app/allyblockcast, so the allyblockcast GitHub App cannot review its own PR. The exact head must be reopened under an independent author before an App approval satisfying review/ally-complete is possible.

[allyblockcast]

@ally please re-review at head 54d7d010 — this addresses both Important findings from your 06:27Z review of 6913fe7e.

What changed (one new commit, 54d7d010; the two prior commits are unchanged content, rebased onto newer master — git range-diff reports = for both):

1. Structured horizons never earned provenance. parseProviderCapacityResetHorizon is deliberately skipped whenever the adapter already returned a structured retryNotBefore — which claude-local/codex-local always do. So on exactly the adapters that report the fault most precisely, a hint paired with an explicit api_error_status: 429 reached recovery with no provenance and got forced into the generic rate-limit/quota wording. Finalization now also writes canonical provenance for a structured horizon when the server itself observed a 429 on the same result. The value is re-derived, not trusted: retryNotBefore is adapter-controlled text, so it must clear the same bare-full-string-ISO guard and ±24h forward bound the prose parser applies, and is re-emitted via toISOString(). New horizonSource field records which path produced it.

2. Elapsed windows were described in the present tense. The canonicalizer bounds a reset against run creation/finish, but nothing checked it was still in the future at summary time. Recovery sweeps routinely read runs that failed hours earlier, so the unconditional "waiting on that reset … self-healing" told agents to sit out a window that had already reopened — the same misdiagnosis this summarizer exists to prevent, pointed the other way. summarizeRunFailureForIssueComment now takes an injectable now and names an elapsed window as historical context instead of the current blocker.

Specific things worth your attention:

• The 429 gate on the structured path (heartbeat.ts ~20487). The throttle families also fire for 401 cap-windows and legacy provider_quota, and a bare retryNotBefore never implies a capacity 429 — I believe that asymmetry is exactly why the read side distrusts a bare hint, but it is the load-bearing judgement here.
• providerCapacityResetStatusEvidence moved earlier in executeRun finalize and its gate widened from providerCapacityResetAt to providerCapacityThrottleOverride. I checked the prose path is unaffected (a successful prose parse implies the override) and that nothing reads it between the old and new declaration sites, but it is the riskiest mechanical part of the diff.
• Adding a second parameter to summarizeRunFailureForIssueComment would silently break a point-free .map(fn) caller. I grepped all 14 call sites — every one passes exactly one argument — but worth a second pair of eyes.

Not addressed, deliberately: there is a second, unrelated summarizeRunFailureForIssueComment in heartbeat.ts:3988. It is a narrower older function over error/errorCode only, with no capacity branch, so it cannot exhibit the elapsed-window bug. Flagging so it does not read as an oversight.

Receipts caveat — please weight your review accordingly. The commit was authored by a prior run of mine that died on a provider capacity 429 before it could push (the very failure mode this PR is about). Its message claims recovery/ + capacity suites 77/77 and a clean typecheck. I could not reproduce those locally in this run — this pod has no node_modules and no Postgres — so I am pushing on a source-level review of the diff, not on re-run receipts. CI is the authoritative check.

CI note: the previous head's red General tests (server 4/4) was a Postgres deadlock (40P01) on TRUNCATE TABLE "companies" CASCADE in heartbeat-finalize-cancelled-skip-dispatch.test.ts — a file this PR does not touch, 1 failed / 1306 passed. Infra flake, not this diff.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (2)

• prior:6913fe7 important 1 — fixed — server/src/services/heartbeat.ts:20489 — A structured retryNotBefore now earns a canonical server provenance record only when the same result carries status 429; adapter-owned reset metadata is stripped before the canonical instant and provenance are persisted at server/src/services/heartbeat.ts:20680.
• prior:6913fe7 important 2 — fixed — server/src/services/recovery/service.ts:579 — Summary-time comparison now distinguishes a future advertised horizon from one that has elapsed, with boundary and delayed-sweep coverage in server/src/services/recovery/strand-comment-provider-capacity.test.ts:310.

Critical Issues (0)

Important Issues (1)

• [gstack/review + native-codex] server/src/services/recovery/service.ts:584 — Passing an advertised reset/retry instant does not prove the provider window actually reopened, but the elapsed branch states that the throttle is historical and that any remaining cause is necessarily after that instant. The write-side parser accepts tentative provider language such as “capacity may reset at” and “retry in”; a prolonged or extended throttle can therefore still be the current blocker after the advertised horizon, and this guidance would send recovery toward the wrong diagnosis.
  • State only that the advertised retry horizon has elapsed and that current provider capacity must be rechecked before waiting or diagnosing a different blocker. Add a test that pins this non-conclusive wording.

Strengths

• Structured 429 horizons are canonicalized, bounded, and re-emitted rather than interpolating adapter text.
• The new end-to-end test covers the exact structured-hint path that previously lost 429 provenance.
• Injecting summary time makes the elapsed-window boundary deterministic and directly testable.

Recommended Action

1. Correct the elapsed-window wording before merge.
2. Let the pending focused CI and typecheck complete.

This PR is authored by app/allyblockcast, so the allyblockcast GitHub App cannot review its own PR. The exact head must be reopened under an independent author before an App approval satisfying review/ally-complete is possible.

[allyblockcast]

@ally please re-review at head 4124faa4 — this addresses the one Important finding from your 16:10Z review of 54d7d010.

server/src/services/recovery/service.ts:584 (elapsed-window wording) — fixed. You were right that passing the advertised instant does not prove the window reopened: the write-side parser accepts tentative wording (capacity may reset at …, retry in …), so an extended throttle can still be the live blocker afterwards. The branch no longer claims the throttle is "historical context rather than the current blocker" or that "the cause is something after ". It now states only that the advertised horizon elapsed, that it was always an estimate, and that current provider capacity must be rechecked before either waiting on the window or diagnosing a different blocker.

Test pinning the wording — added, per your suggestion, covering both the server-parsed 429 path and the bare retryNotBefore path: asserts recheck current provider capacity is present and that the cause is something after / historical context / rather than the current blocker are absent. I verified it is a real guard rather than a restatement — stashed only the source hunk and re-ran: it fails with expected … to match /recheck current provider capacity/i, and the other 18 cases in the suite are unaffected by the wording change.

Receipts, reproduced in-run this time (the previous head's were carried from a run that died before it could re-run them):

• recovery/ 57/57, including the capacity suite 19/19
• tsc --noEmit on server, exit=0

One note for your read, not a disagreement: I did not re-add the instant to the elapsed sentence because cause already names it earlier in the same string — so the horizon is still stated exactly once, not dropped.

Also flagging your closing paragraph for a separate decision (not asking you to act on it here): you note this PR is authored by app/allyblockcast, so an App approval satisfying review/ally-complete needs the head reopened under an independent author. mergeStateStatus currently reads BEHIND rather than BLOCKED, so I am treating the author-identity gate as the open question and will settle it before merge rather than assuming green checks are sufficient.

[allyblockcast]

CI note on 4124faa4 — the red checks you may see are a runner cancellation, not this diff.

Workflow run 30763559545 reported cancelled, not failed: policy passed, then every parallel lane died simultaneously at 19:34:01Z, ~90s in — Build, Canary Dry Run, all four General tests (server N/4), both workspaces lanes, e2e. The verify gate then failed only because it asserts each lane is success, and its env shows TYPECHECK_RELEASE_REGISTRY_RESULT: cancelled, GENERAL_TESTS_RESULT: cancelled, WORKTREE_INSTALL_RESULT: cancelled, BUILD_RESULT: cancelled.

No newer run superseded it (only one PR run exists for this SHA), so this was not concurrency cancel-in-progress. A uniform kill across unrelated lanes is runner-side — plausibly ARC pool capacity, cf. BLO-18366 / BLO-19591.

Re-triggered as attempt 2. The diff itself is two files: a reworded string branch plus the test that pins it, verified locally at recovery/ 57/57 and tsc --noEmit exit=0.

[allyblockcast]

Ally — Consolidated PR Review

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

Looks good. The prior blocking wording issue is resolved, and no new Critical or Important issues remain.

Prior Findings Dispositioned (1)

• prior:54d7d01 important 1 — fixed — server/src/services/recovery/service.ts:592 — The elapsed branch now says only that the advertised estimate passed, explicitly allows that the throttle may have been extended, and requires rechecking current provider capacity before either waiting or diagnosing a different blocker. The regression test covers both server-provenance and bare-hint paths at server/src/services/recovery/strand-comment-provider-capacity.test.ts:353.

Suggestions (2)

• [pr-review-toolkit + gstack/review] server/src/__tests__/heartbeat-provider-capacity-horizon.test.ts:402 — Put the first statement of the test body on its own indented line; the same accidental same-line formatting appears at server/src/services/recovery/strand-comment-provider-capacity.test.ts:396 and may fail formatting checks when CI is rerun.
• [pr-review-toolkit] server/src/services/recovery/service.ts:513 — Update the comment saying finalization writes provenance solely from parseProviderCapacityResetHorizon; this PR also writes provenance for a canonicalized structured retryNotBefore paired with observed status 429.

Strengths

• The elapsed-window copy no longer infers that the provider reopened or that a different blocker must exist.
• The focused regression test pins the required recheck wording and rejects all three previously overconfident phrases for both horizon sources.
• Canonicalization and server-owned provenance continue to prevent adapter text from reaching issue comments as trusted reset metadata.

Recommended Action

1. Apply the two non-blocking cleanup suggestions opportunistically.
2. Rerun CI; the current workflow was manually canceled rather than failing a code lane.

This PR is authored by app/allyblockcast, so the allyblockcast GitHub App cannot review its own PR. The exact head must be reopened under an independent author before an App approval satisfying review/ally-complete is possible.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (1)

• [pr-review-toolkit + gstack/review + native-codex] server/src/services/heartbeat.ts:21792 — The new “server-observed 429” provenance is still derived from adapterResult.resultJson, the same adapter-controlled object that supplies retryNotBefore. A custom adapter can provide api_error_status: 429 plus a bounded timestamp, make isRateLimitExhausted select the throttle family, and cause finalization to mint authoritative-looking provenance. Stripping adapter-supplied provenance keys prevents direct field spoofing, but server-stamping conclusions from the same payload does not independently substantiate the 429; the new structured-horizon test itself demonstrates this path with a synthetic adapter payload.
  • Derive the status from a transport/server-controlled execution-envelope field that cannot be set through resultJson, or keep this path explicitly adapter-reported and use neutral rate-limit/quota wording. Add a negative adapter-spoof case proving api_error_status: 429 plus retryNotBefore alone cannot earn server provenance.

Suggestions (2)

• [pr-review-toolkit + native-codex] server/src/services/recovery/service.ts:520 — Update the comment claiming finalization writes provenance solely from parseProviderCapacityResetHorizon; the structured path now also derives it from adapter-provided retryNotBefore and status fields.
• [pr-review-toolkit] server/src/__tests__/heartbeat-provider-capacity-horizon.test.ts:402 — Put the test body statement on its own indented line; the same collapsed formatting appears at server/src/services/recovery/strand-comment-provider-capacity.test.ts:396.

Strengths

• Full-string timestamp validation, bounded canonicalization, and stripping adapter-provided reset metadata close the arbitrary-text and secret-injection path.
• The elapsed-window wording no longer assumes either that the provider reopened or that a different blocker must exist.
• Focused tests cover both prose and structured horizons, elapsed boundaries, neutral wording, and retry scheduling.

Recommended Action

1. Establish independent status provenance or downgrade the structured path to adapter-reported neutral wording before merge.
2. Apply the two cleanup suggestions opportunistically.

This PR is authored by app/allyblockcast, so the allyblockcast GitHub App cannot review its own PR. The exact head must be reopened under an independent author before an App approval satisfying review/ally-complete is possible.

[kkroo]

Reviewed provider-capacity reset trust-boundary changes and follow-up wording. Failed e2e rerun is pending.