fix(heartbeat): stop coalescing from inheriting the status-only guard onto a declared normal-model wake (BLO-32634) - #1718
Conversation
… onto a declared normal-model wake (BLO-32634)
Reproduced first: a monitor fire coalescing into a run row already stamped
`status_only` kept the whole recovery guard tuple, so the monitor's own
scheduled write was refused by `isStatusOnlyCheapRecoveryContext`. The
reproduction also surfaced a second defect the issue only hypothesised —
`planning_only` supplies no `modelProfile`, so an inherited `cheap` rode along
beside `recoveryIntent: planning_only`, a partial tuple no caller can construct
directly.
Both come from the same gap: `mergeCoalescedContextSnapshot` is
`{...existing, ...incoming}` with drop-lists covering only GitHub keys, and
`withRecoveryModelProfileHint(x, "normal_model")` is scrub-only — deleting keys
from the INCOMING snapshot says nothing to a spread that reads the EXISTING one.
A deliberate normal-model declaration was therefore indistinguishable from a
wake that had never considered run class.
Make the declaration positive and let it own the block:
- `withRecoveryModelProfileHint` stamps `recoveryWorkClass` for all three
classes, including `normal_model`.
- The merge drops the guard block as a unit when the incoming wake declares a
class — the `isNewReviewInstance` idiom already in that function. A wake
SILENT about run class still inherits, so this is not the blanket drop-list
the issue rules out.
- The monitor-wake snapshot declares `normal_model` explicitly.
Co-Authored-By: Claude <noreply@anthropic.com>
1 similar comment
…ard-block drop (BLO-32634) The GitHub-key drop already has this guard: parseObject returns its argument by reference, so a clear implemented against `existing` rather than `merged` would strip the guard off the still-queued run row the snapshot came from. Pin the same invariant for the new deleter. Co-Authored-By: Claude <noreply@anthropic.com>
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 73c5ae9
The core mechanism is right, and it is built the way the two precedent blocks in this
function were built. merged is a fresh object so the drop cannot alias existing
through parseObject; keying on a positive recoveryWorkClass declaration rather than
dropping unconditionally is the correct call and is pinned by its own sign-guard test;
and clearing the tuple does make isStatusOnlyCheapRecoveryContext
(routes/issues.ts:6813) return false, so it genuinely fixes the write-refusal the issue
describes. Both findings below are about the blast radius of that clear, not about
whether it works.
Critical Issues (0)
Important Issues (2)
-
[gstack/review]
server/src/services/recovery/model-profile-hint.ts:48— the shipped
resumeGuidancestring still asserts an invariant this PR falsifies: "while its
recovery action is active, every wake on it is status-only." After this change a
monitor fire on that issue clears the whole guard block, so a normal-model run on an
issue with a live recovery action is exactly what now happens. This string is not
internal — it is spread into three 403 bodies (routes/issues.ts:6876,:6994,
:7028) and read by the refused agent. The comment directly above it
(model-profile-hint.ts:36-42) records that BLO-25878 cost three consecutive runs
because agents mis-read this block as a promise; leaving the sentence stale re-opens the
same class of mis-read from the other direction, with the agent now under-estimating
what can run.- Narrow the sentence to carve out an explicitly-declared normal-model wake (e.g. "every
recovery wake on it is status-only; an assignee-scheduled monitor fire is not"), so
the guidance and the merge agree.
- Narrow the sentence to carve out an explicitly-declared normal-model wake (e.g. "every
-
[native-codex]
server/src/services/heartbeat.ts:8498-8502— the guard-clear is
reachable by the guarded party, and it lifts cost containment while leaving the recovery
action active. Monitor arming consults the assignee relation but never the run's recovery
class:assertCanManageIssueMonitorreturns early for the assignee at
routes/issues.ts:2071, and the only recovery-aware gate onPATCH /issues/:idis
assertCheapRecoveryIssueAssigneeProfileAllowed(:11100), which only blocks assigning
the cheap profile downstream. So an agent executing a status-only recovery run can arm
a monitor on its own issue; when that monitor fires it coalesces onto the still
queued/scheduled_retryrecovery run (TASK_SCOPE_COALESCIBLE_RUN_STATUSES,
heartbeat.ts:725— the same shape as this PR's own test fixture), all seven guard keys
drop, and the run comes back with the cheap profile unpinned and deliverable writes
re-opened. The PR assertsrecoveryActionIdsurvives the drop
(__tests__/heartbeat-context-summary.test.ts:67), so the recovery action stays active
and un-dispositioned on a run that is no longer bounded — which is the state
STATUS_ONLY_RECOVERY_GUARD_CONTEXTexists to prevent.- If this is intended, please say so in the block comment, since it is the one
consequence the (otherwise very thorough) comment does not address. If it is not:
keepingmodelProfileandallowDeliverableWorkpinned whilerecoveryActionIdis
present would give the monitor its write path — the actual bug — without lifting the
cost guard.
- If this is intended, please say so in the block comment, since it is the one
Suggestions (3)
- [pr-review-toolkit/types]
model-profile-hint.ts:67—RECOVERY_GUARD_CONTEXT_KEYS: readonly string[]widens away the literal union, so the compiler can no longer relate
this export toRecoveryModelProfileHintKey.readonly RecoveryModelProfileHintKey[]
(or just dropping the annotation and letting it infer from theas constarray) keeps
the literals and still satisfies bothkey in incominganddelete merged[key]at the
merge. The new drift test covers the risk at runtime; the type could carry it for free. - [pr-review-toolkit/code]
heartbeat.ts:13367vs:13393—contextSnapshotnow
declaresnormal_modelbut the siblingpayloadbuilt immediately above it does not,
whereas all 11 recovery dispatch sites inrecovery/service.tsapply the hint to both.
Benign today —normalizeModelProfileWakeContext(heartbeat.ts:5528) only copies
payload.modelProfileinto the snapshot when the snapshot has none, and this payload
carries none — but that function is precisely a payload→snapshot bleed path, so matching
the established idiom is cheap insurance. - [gstack/review]
paperclipModelProfileis in the dropped set but is written only by
the dispatch pipeline (heartbeat.ts:27355) and never supplied by any caller — the
helper scrubs it — so every declared-class coalesce now strips the applied-profile record
from the run row. Nothing in the tree reads the key (only the write/delete pair at
:27355/:27358), so this is observability rather than behaviour; worth a look only if
that field is queried out-of-band for cost attribution.
Strengths
- Exporting
RECOVERY_GUARD_CONTEXT_KEYSand enforcing it with "declares every guard key
it can write" closes the writer/dropper drift gap structurally instead of by comment —
the failure mode where a new guard key silently stops being dropped is now a test
failure, not a code-review dependency. - The
recoveryWorkClasskey is the right shape for the underlying problem: it makes
"I am normal-model" a positive statement, where the old scrub-only path was
indistinguishable from silence. Preserving inheritance on silence is the conservative
half and it is explicitly tested. - The non-mutation test mirrors the
parseObjectaliasing hazard already documented for
the GitHub block — the right precedent to copy, and easy to have missed. - The partial-tuple case (
planning_onlyinheriting a stalecheap, satisfying
isPlanningOnlyRecoveryContextwhile running on the wrong model) is a real latent bug
found and fixed alongside the reported one.
Recommended Action
- Address the two Important items before merge — the stale
resumeGuidanceis a
one-line fix; the guard-clear blast radius needs an explicit intended/not-intended call. - Consider the Suggestions opportunistically.
- Note the branch is
BEHINDmaster andGeneral tests (server 1-4/4)were still
queuedat this head;Typecheck + Release Registry,e2e, andsecurity-revieware
green.
Thinking Path
Linked Issues or Issue Description
done) — same function, different keys, different consequenceWhat Changed
server/src/services/recovery/model-profile-hint.tsRECOVERY_WORK_CLASS_KEY(recoveryWorkClass).withRecoveryModelProfileHintnow stamps it for all three classes, includingnormal_model, which previously wrote nothing at all.RECOVERY_GUARD_CONTEXT_KEYS— the block the merge drops as a unit. Exported so the two modules cannot drift; a unit test asserts every key the helper can write is listed.server/src/services/heartbeat.tsmergeCoalescedContextSnapshot: when the incoming wake declares a run class, it owns the guard block — whatever it does not re-supply is cleared rather than inherited. A wake silent about run class inherits exactly as before.dispatchClaimedIssueMonitor: the monitor-wake snapshot now declaresnormal_modelexplicitly instead of being merely silent.mergeCoalescedContextSnapshotsuite (deliberately shipped as one block — see Verification) and 2 in the model-profile-hint suite.Verification
The reproduction came first. The issue was explicitly source-read, not measured; its AC1 asked for a reproduction or a recorded negative result. Both defects reproduce. Before the fix, on
2ebf80098:The second failure is a defect the issue only hypothesised (its AC4):
planning_onlysupplies nomodelProfile, so an inheritedcheaprode along besiderecoveryIntent: planning_only— a tuple no caller can construct directly, and one that satisfiesisPlanningOnlyRecoveryContextwhile evadingisStatusOnlyCheapRecoveryContext.After the fix:
Tests 18 passed, andmodel-profile-hint.test.tsTests 6 passed.The pairing is the point, and a reviewer should check it specifically. The obvious patch — add the guard keys to the drop-list unconditionally — makes the first test pass and fails in the expensive direction, stripping the guard off a genuinely status-only run whenever any unrelated wake arrives. Two of the four new tests are sign guards against exactly that, and they pass on the unfixed tree too, which is what makes them load-bearing rather than decorative:
keeps the status-only recovery guard when the incoming wake is silent about run classapplies the status-only guard when the recovery wake is the incoming oneTo confirm the fix is not the blanket version, delete the
declaresRecoveryWorkClasscondition and keep the loop: the first sign guard goes red.One unrelated failure exists in the tree and is pre-existing, verified by stashing this diff and re-running on a clean checkout:
packages/adapters/codex-local/.../execute.auth-precedence.test.ts › logs and emits a run event when sandbox login is shadowed by host auth.Risks
normal_modelsemantics change: 18 call sites now stamp a marker that makes their declaration authoritative through a coalesce. I read them; all are deliberate ("this retry is not cost-guarded"), and the clearest isheartbeat.ts's process-loss retry, which spreads an inherited snapshot and then explicitly scrubs it to normal-model — intent that previously survived the spread but not the coalesce. None looked like a lazy default.wakeReasonbeing a monitor kind) satisfies AC2's letter, fixes monitors only, requires every future clean-wake lane to be added to a set, and delivers none of AC4. I took the generalisable shape deliberately; happy to be overruled.recoveryAssigneeAdapterOverrideswrites{modelProfile: "cheap"}onto newly created recovery/review issues as a persistent issue field. That is a second, independent channel for cheap dispatch. It does not affect the write-refusal this PR fixes (isStatusOnlyCheapRecoveryContextreads only the run context snapshot) and it does not apply to source issues, so a monitor on an ordinary issue is untouched by it. I did not change it.recoveryWorkClass. They are only ever theexistingside of a merge, where absence means "silent", which is the pre-existing behaviour. No migration needed.heartbeat_runslive-check in the issue's verifying signal is not something I could execute — no DB or cluster read path from this lane. The paired test is the verification; the post-deploy query is left for whoever has that access.Model Used
Claude Opus 4.5 (
claude-opus-4-5), 1M context, extended thinking, with tool use and code execution via Claude Code.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template