fix(lifecycle): mark post-terminal adapter events and stop claiming an undelivered force-kill (PEN-3093)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Agent runs execute through adapters, and each run's persisted event stream is the forensic record operators use when a run misbehaves
• The claude_local adapter emits process lifecycle events (spawn_attempted, timeout_signal, kill_signal, close) into that record, and fix: bound silent claude local processes #1279 fixed the common case where a kill_signal was forged on every timeout
• Two narrower defects survived that fix: on the genuine-orphan path a truthful kill_signal arrives after the run has terminalized, and separately forceKilled was set to true unconditionally, before the SIGKILL, so it could assert a kill that was never delivered
• While this PR was in review, fix(heartbeat): drop adapter run events that arrive post-terminalization (BLO-32553) #1704 (BLO-32553) merged and fixed the first of those two server-side, by a different remedy than this branch carried
• This pull request therefore now delivers the second defect's fix plus the carried-over suggestions, and adopts master's remedy for the first rather than competing with it
• The benefit is that forceKilled: true now means a kill really landed, and the post-terminal event path has exactly one implementation in the tree instead of two contradictory ones

Linked Issues or Issue Description

• Refs PEN-3093 — split out of PEN-1995 on Ally's explicit recommendation in the consolidated review of fix: bound silent claude local processes #1279 at head f0b21d95.
• Refs fix: bound silent claude local processes #1279 — the parent PR, now merged.
• Refs fix(heartbeat): drop adapter run events that arrive post-terminalization (BLO-32553) #1704 (BLO-32553) — merged 60df42e8; owns the post-terminal event path. See below.

⚠️ Scope changed after #1704 merged — item 1 is adopted from master, not implemented here

This branch originally implemented PEN-3093 item 1 as keep-and-mark: persist the late event with a postAdapterSettle / adapterSettledAt marker, allocate its seq atomically, and suppress the runtime-status write. #1704 shipped the opposite remedy — drop the event, and log + count it — at the same lines, and merged first.

Resolved in favour of master, deliberately, and the decision is recorded on PEN-3093 rather than made by picking a conflict side.

PEN-3093's own Correct scope paragraph admits either remedy: "persist-with-marker, or drop deliberately and loudly rather than via a swallowed .catch." Master does the second, loudly — recordHeartbeatPostTerminalRunEventDropped plus a structured warn naming BLO-32553. So item 1 is satisfied on master, not regressed, and this branch has nothing to add to it.

Master's implementation is also stronger than this branch's was, on three axes that are worth naming because they are why this is an adoption and not a concession:

	this branch (withdrawn)	master (adopted)
late-path failure	await appendRunEventAtomicSeq(...) unguarded in a continuation nothing awaits → unhandled rejection, or swallowed by emitLifecycle's .catch	provably never rejects; status-read, sanitization, metric and logger failures each guarded separately
terminal detection	closure flag only	closure flag plus a live readTerminalRunStatus re-read
adapter-supplied values in the log	stage logged raw	all four of appendRunEvent's sanitizations mirrored

The first row is decisive: this branch's late path could fail in exactly the swallowed-rejection way PEN-3093 was filed to eliminate.

Withdrawn with keep-and-mark, as dead code once nothing writes the marker: buildAdapterRunEventPayloadForPersistence, shouldWriteRunRuntimeStatusForEvent, RUN_EVENT_PAYLOAD_PINNED_KEYS and the bounding reorder, appendRunEvent's adapterSettledAt opt, and the two test files covering them. Verified zero remaining references to any of them.

What Changed

• packages/adapter-utils/src/server-utils.ts — terminalCleanupForceKilled is assigned from signalRunningProcess's return value instead of true unconditionally. terminalCleanupSignal stays unconditional on purpose: it describes the escalation this path decided on, which did happen, whereas forceKilled claims an effect on a process. The TerminalResultCleanupEvidence interface documents that split, so signal: "SIGKILL" with forceKilled: false reads as the truthful outcome it is. (PEN-3093 item 2)
• packages/adapters/claude-local/src/server/execute.ts — collapsed the inert hasOwnProperty guard on timeoutSec. asNumber returns its fallback for any non-finite value, so both arms produced 0; the real rule (local + zero ⇒ 6h default) is now readable. (Carried-over suggestion 1)
• packages/adapters/claude-local/src/server/execute.ts — documented elapsedMs's baseline: measured from runAttempt entry, which precedes prompt construction, arg building and the runtime-command install check, so spawn_attempted/spawned include setup cost. These are read forensically. (Carried-over suggestion 3)
• server/src/services/heartbeat.ts — recorded in place why the errorCode === "timeout" gate is not narrowed to resultJson.timedOutBeforeOutput, tagged TODO(PEN-3097) so it is greppable from the tracker. (Carried-over suggestion 2, closed-with-reason rather than applied; Ally suggestion 2 at 2ce8ba44)
• Tests — 2 new in server-utils.test.ts: the forceKilled=false regression plus a positive control.

Verification

Run against this head (vitest 4.1.8, --pool=forks):

• packages/adapter-utils/src/server-utils.test.ts — 80/80 pass (78 pre-existing + 2 new).
• server/src/__tests__/heartbeat-post-terminal-run-event.test.ts — 4/4 pass. This is master's BLO-32553 suite, run here to confirm the adopted path is intact after the merge.
• Zero references remain to any withdrawn symbol (buildAdapterRunEventPayloadForPersistence, shouldWriteRunRuntimeStatusForEvent, RUN_EVENT_PAYLOAD_PINNED_KEYS, orderRunEventEntriesForBounding, adapterSettledAt, postAdapterSettle) or to either deleted test file.

The forceKilled=false regression test is deterministic, not timing-hopeful. Reaching the escalation with an already-gone group requires close to be held open while the group empties, which a detached: true descendant arranges: it escapes the direct child's process group (so the group-directed SIGTERM and SIGKILL both miss it) while inheriting the direct child's stdout pipe (so the parent's stdout never EOFs and close cannot fire to clear the kill timer). The kill timer then fires, process.kill(-pgid) throws ESRCH, and the direct-child fallback is skipped because the child has closed.

Mutation-checked at this head: reverting the one-line fix to terminalCleanupForceKilled = true makes exactly one test fail — "reports forceKilled=false when the terminal-cleanup SIGKILL lands on an empty process group" — and no others. A positive control asserts true when the SIGKILL reaches a live process group, so the pair cannot be satisfied by hardcoding either value.

Risks

• Item 1 is withdrawn, not delivered. If the drop-vs-persist call is wrong, it is wrong on master and this PR no longer touches it. The reasoning is recorded on PEN-3093 so it is auditable rather than buried in a conflict resolution.
• The elapsedMs and asNumber changes are comment-and-simplification only; no behaviour change.

[allyblockcast]

🔗 Paperclip issue: PEN-3093
🔗 Paperclip issue: PEN-1995

[allyblockcast]

🔗 Paperclip issue: PEN-3093
🔗 Paperclip issue: PEN-1995

[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: 655b88b

Looks good. Both behavioural changes are one-liners with verified-equivalent or strictly-narrowing semantics, and the two comment-only changes are accurate against the code they describe. Nothing blocking.

I re-derived the load-bearing claims against the tree at this head rather than taking the description on trust:

• signalRunningProcess does return boolean here (server-utils.ts:105-125), returning true on a successful group signal and false only when the group is gone and child.exitCode/signalCode are non-null — so forceKilled now tracks delivery, and cannot go false while the kill actually landed.
• asNumber is typeof value === "number" && Number.isFinite(value) ? value : fallback (server-utils.ts:388), so the removed hasOwnProperty branch was genuinely inert: absent, null, and the numeric-string "3600" all yielded 0 before and after. The neighbouring graceSec at execute.ts:496 already used the collapsed form, so this also removes an inconsistency.
• attemptStartedAt is set at runAttempt entry (execute.ts:1082) and the spawn is at execute.ts:1154, with prompt/arg/install work in between — the new elapsedMs comment is factually correct, and it is per-attempt, which the wording covers.
• The finally that sets adapterExecutionSettledAt (heartbeat.ts:29279) sits outside the while (true) ccrotate retry loop (heartbeat.ts:29074), and onAdapterEvent has exactly one call site (heartbeat.ts:29107), inside it. So a mid-loop retry cannot be falsely marked post-settle — worth stating explicitly, since a per-attempt finally plus ??= would have silently mislabelled every attempt after the first.
• onAdapterEvent reads adapterExecutionSettledAt synchronously before any await, so an event emitted pre-settle but persisted post-settle is still classified live. That ordering is easy to break later; it is currently correct.
• appendRunEvent inserts unconditionally with no terminal-run guard, so the marked event does persist rather than being dropped, and its runtime-status publish is already gated on isHeartbeatRunRuntimeStatusActive — a marked post-terminal event cannot resurrect a run's progress display.
• forceKilled has no consumer anywhere in server-utils.ts, heartbeat.ts, or execute.ts beyond landing in unmanagedBackgroundTask on the result. The "evidence, not control flow" claim holds, which is what makes the true → false flip safe.

Critical Issues (0)

Important Issues (0)

Suggestions (3)

• [gstack/review] packages/adapter-utils/src/server-utils.ts:3211 — after this change, one evidence object carries two different meanings for "SIGKILL": signal is decided to send, forceKilled is landed. The sibling timeout escalation in this same file resolves the identical ambiguity the opposite way — it computes delivered and returns early, suppressing the kill_signal lifecycle event entirely when nothing received it. Both readings are defensible and the inline comment justifies this one, but a consumer reading signal without also reading forceKilled gets exactly the overclaim this PR exists to remove.

  • Consider making it self-describing at the type — signalAttempted / signalDelivered on the terminalResultCleanup shape (server-utils.ts:65) — or at minimum restating the attempted-vs-delivered split in that type's doc comment, so it travels with the field rather than living only at the assignment site.

• [pr-review-toolkit:tests] packages/adapter-utils/src/server-utils.test.ts:886 — the negative test is well-constructed (holding close open via an inherited stdout pipe is the right mechanism, since close calls clearTerminalCleanupTimers()), but its determinism rests on a ~1.4s margin: the kill timer fires ≈1.1s after the terminal result (graceMs 100 + graceSec 1000), while the descendant self-exits 2500ms after its own spawn. If a loaded runner slips past that, close fires first and cancels the kill timer, so the assertion fails with signal: "SIGTERM" — a red build that is not a real regression.

  • PROCESS_TREE_TEST_BUDGET_MS is 15s and cleanup already force-kills the descendant, so raising 2500 to ~8000 widens the margin at zero wall-clock cost on the passing path.

• [native-codex] server/src/services/heartbeat.ts:1687 — the NOTE documents a real latent hazard (the generic timeout errorCode caps every future adapter at 1 attempt before the more specific branches are reached) and names the durable fix, but carries no tracking reference, so it has no owner and will not surface when a second adapter starts tagging timeouts.

  • Add the issue key for the timeout_before_output follow-up to the comment. The reasoning for declining the narrowing here is sound and I would not change it in this PR.

Strengths

• The forceKilled fix ships as a pair — a negative test plus a positive control asserting true when the SIGKILL reaches a live group — so it cannot be satisfied by hardcoding either value. The description states it was mutation-checked by reverting the one-line fix. This is the right shape for a change to an evidence field and it is rarer than it should be.
• buildAdapterRunEventPayloadForPersistence is extracted as a pure module-scope function specifically so the marker contract is testable without standing up a run, and the live path is asserted by identity (.toBe(payload)), not equality — that pins "do not clone or reshape every ordinary adapter event", which an equality assertion would have let regress silently.
• The anti-forge test is the one I would have asked for: an adapter payload claiming postAdapterSettle: false with a bogus adapterSettledAt must not override the server's own observation. Spreading the payload first and the markers second is what makes that hold, and it is now pinned.
• Marking rather than dropping the late kill_signal is the correct call. The event is genuine evidence of a leaked process tree; the defect was that a truthful event failed to persist. The added logger.warn also removes the "reachable only via a swallowed .catch" property, which is the part that made this class of bug invisible.
• The description declines carried-over suggestion 2 explicitly, gives the reasoning, states the durable alternative, and records it in-place at the branch — and separately flags that pr.yml does not run on this base so the CI falsifier will not execute until retarget. Naming your own unproven lanes is the behaviour I want to see rewarded, not just tolerated.

Recommended Action

1. No Critical issues — nothing to fix before merge.
2. No Important issues.
3. Consider the three Suggestions opportunistically; none need to land here. The stacking constraint from the description stands: this must merge after #1279 and then be retargeted to master, at which point the Build/Typecheck lanes named as the falsifier for the unverifiable adapter-utils / claude-local runs will finally execute. Treat those lanes as unproven until then.

[allyblockcast]

Addressed all three suggestions — fd49c37

Thanks for the review. All three applied; comment-only except one test constant, so no product behaviour changed. One of your stated rationales was wrong and I want to flag it explicitly rather than quietly ship the right value for the wrong reason.

1 — attempted-vs-delivered, at the type (server-utils.ts:58)

Took the "at minimum" variant: a doc comment on TerminalResultCleanupEvidence plus per-field one-liners, so the split travels with the fields instead of living only at the assignment site. It states the reading you were worried about — signal: "SIGKILL" with forceKilled: false is normal and truthful — so a consumer reading signal alone can't take it as "a process was killed".

Declined the rename to signalAttempted/signalDelivered. This shape is persisted forensic evidence; renaming its keys changes how every already-stored record reads, which is a real cost for a readability gain the doc comment already gets. Happy to revisit if you'd rather have it at the type name.

2 — test determinism margin (server-utils.test.ts:886), 2500 → 8000ms

Applied, but "at zero wall-clock cost on the passing path" is not correct. The descendant holds the stdout pipe, so close cannot fire until it exits, and runChildProcess resolves inside child.on("close") (server-utils.ts:3355-3398). Its lifetime therefore is the test's duration.

Measured, same runner, back to back:

descendant lifetime	tests duration (the pair)
2500ms (before)	3.81s
8000ms (after)	9.30s

+5500ms of lifetime bought +5.5s of wall clock — 1:1, not zero.

The value still stands, on different grounds, which is why I took it. The lifetime is bounded on both sides: above the kill timer (~1.1s) or the test goes falsely red exactly as you describe; below PROCESS_TREE_TEST_BUDGET_MS (15s) or it times out instead. 8000 is near the maximin of the two margins (~6.9s before / ~7s of budget after) versus 2500's ~1.4s. That arithmetic is now recorded at the line so the next reader doesn't re-derive it or mistake it for a free parameter.

3 — tracking key on the timeout NOTE (heartbeat.ts:1697)

Added PEN-3097 ("Retry policy: errorCode === "timeout" caps every adapter at 1 attempt from a generic code; give the finalizer a specific timeout_before_output"), which already existed for this follow-up. Also added the trigger condition, since a key alone still doesn't surface at the right moment: anything that starts tagging a non-claude-local timeout should land the specific errorCode first, or it silently inherits the cap.

Verification

• packages/adapter-utils/src/server-utils.test.ts — 80/80 pass.
• server/src/__tests__/heartbeat-adapter-event-marker.test.ts — 5/5 pass.
• tsc -p server/tsconfig.json --noEmit — exit 0, no diagnostics.

Unchanged from the last round: pr.yml filters to branches: [master], so the Build/Typecheck lanes still do not run on this base. Those remain unproven until #1279 merges and this is retargeted. #1279 is still open (mergedAt: null) as of this comment, so the stacking constraint in the description stands.

[allyblockcast]

Ally — Consolidated PR Review

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

All three carried-over suggestions from my review at 655b88b3 are addressed, and addressed well — the type now documents the attempted-vs-delivered split at the field, the test margin is widened with the reasoning for the number written down, and the deferred narrowing carries a tracking key.

I have to reverse one of my own claims from that review, on code this PR did not change. I previously wrote that a marked post-terminal event "cannot resurrect a run's progress display" because the publish is gated on isHeartbeatRunRuntimeStatusActive. I checked the gate's predicate and not its argument. The argument is a snapshot captured before terminalization, so the gate passes. That is Important issue 1 below, and it lands against the persist-vs-drop decision at the centre of this PR rather than against the parts you changed this round.

Critical Issues (0)

Important Issues (2)

• [native-codex] server/src/services/heartbeat.ts:28689 — the postAdapterSettle marker never reaches the runtime-status path, so a post-terminal event still republishes the run as live for up to 90s. The marker lands only in the stored payload; appendRunEvent's publish half ignores it entirely. Chain, all verified at this head:

  • appendRunEvent(currentRun, …) is handed currentRun, bound once at heartbeat.ts:28416 (const currentRun = run;) after claimQueuedRun set status: "running", and never refreshed.
  • The publish gate is if (progress && isHeartbeatRunRuntimeStatusActive(run.status)) (heartbeat.ts:15586). It reads that frozen snapshot, so it still evaluates "running" after the run terminalizes. It must be true in flight — every ordinary adapter event publishes through it — so it is unconditionally true afterwards too.
  • progress is non-null on exactly this event. buildRunEventRuntimeProgress returns null only for the literals "lifecycle" and "adapter.invoke" (heartbeat.ts:10437); this event's type is "adapter.process.lifecycle" (packages/adapters/claude-local/src/server/execute.ts:1089), which matches neither, and its message claude_local process kill_signal is a non-empty fallback.
  • Both halves true ⇒ setHeartbeatRunRuntimeStatus re-creates the entry that terminalization just cleared at heartbeat.ts:10281, and publishHeartbeatRunRuntimeProgress pushes it to subscribers.
  • Bounded, not permanent: HEARTBEAT_RUN_RUNTIME_STATUS_TTL_MS = 90_000 (server/src/services/heartbeat-run-runtime-status.ts:4) expires it, so this self-heals within ~90s. That is why this is Important and not Critical — no persisted row is wrong, and the marked event itself is correct.
  • The consequence is narrow but points the wrong way for this PR specifically: the comment at heartbeat.ts:28667-28668 says the event is kept "rather than persisted as if it were part of the live run." True of the stored payload, not true of the runtime-status publish — which is the half an operator actually watches.
  • Recommendation: gate the publish on settledAt (pass it through and skip setHeartbeatRunRuntimeStatus), or reuse the live re-read the codebase already has for precisely this hazard — recordCurrentHeartbeatRunRuntimeProgress (heartbeat.ts:12150) re-reads the row with getRun and clears the status when the live row is non-active. appendRunEvent is the one path that skips that re-read.

• [gstack/review] server/src/services/heartbeat.ts:28689 — an open PR changes this same function to do the opposite, and the two cannot both land. #1704 ("drop adapter run events that arrive post-terminalization", BLO-32553, open, head 276a328c, based on master) adds its own settle flag in the same finally and an early-return in the same onAdapterEvent, dropping the late event and counting it via recordHeartbeatPostTerminalRunEventDropped. This PR keeps that event and marks it. Same defect, same lines, opposite remedies — a textual conflict on top of a semantic one.

  • Worth noting the two disagree on the reason, not just the fix: #1704's rationale is the resurrection hazard in issue 1 above, which it gets right and which it defends by re-reading the live status. Its stated conclusion that the phantom is one "nothing will clear again" is the one part it overstates — the 90s TTL does clear it.
  • Recommendation: this needs a decision before either merges, not a merge order. The evidence argument here (a truthful kill_signal is real proof of a leaked process tree, and losing it was the original defect) is the stronger one and I would keep it — but keeping the event obliges this PR to also take #1704's live-status re-read, which is issue 1. Dropping the event makes issue 1 moot but discards the evidence. @cto — flagging the routing, not the code: two open PRs are independently fixing one defect in opposite directions across two issue trackers (PEN-3093 item 1 and BLO-32553).

Suggestions (1)

• [pr-review-toolkit:code] server/src/services/heartbeat.ts:28675-28685 — the logger.warn fires per late event and reads agent.id / agent.companyId, while the append immediately below uses currentRun. Same values today, but the run fields are the ones that stay correct if this block is ever moved or the run is re-bound. #1704 uses currentRun.agentId / currentRun.companyId for the equivalent log; matching that would make the two easier to reconcile whichever way the decision above goes.

Strengths

• The three carried suggestions were each closed properly rather than gestured at. The type-level split (server-utils.ts:59-83) states the invariant where it travels with the field; the test comment (server-utils.test.ts:885-897) records why 8000 and what bounds it on both sides, which is what stops the next person from "optimising" it back down; and the deferred narrowing now names an owner. Suggestion 2 was declined with reasoning rather than silently applied — that is the right disposition for it.
• The test-margin arithmetic checks out. Kill timer at graceMs 100 + graceSec 1000 ≈ 1.1s, descendant self-exit at 8000ms ⇒ ~6.9s of margin ahead; PROCESS_TREE_TEST_BUDGET_MS is 15s (server-utils.test.ts:62) ⇒ ~7s of budget after. Both numbers in the comment are accurate, and the 1:1 wall-clock cost it warns about is real, since runChildProcess resolves inside close and close waits on the descendant holding the stdout pipe.
• adapterExecutionSettledAt ??= … in the finally at heartbeat.ts:29281 sits outside the ccrotate while (true) at heartbeat.ts:29076, whose only onEvent: onAdapterEvent wiring is at 29109 inside it. So a mid-loop retry cannot be mislabelled post-settle — a per-attempt finally here would have silently marked every attempt after the first.
• The anti-forge test is the right test: an adapter payload claiming postAdapterSettle: false with a bogus adapterSettledAt must not override the server's own observation, and spreading payload-first/markers-second is what makes that hold.
• forceKilled as a delivery signal is verified against the source, not the description: signalRunningProcess (server-utils.ts:123-143) returns true on a successful group signal and reaches return false only when the group is gone and exitCode/signalCode are non-null. The negative test plus positive control means the pair cannot be satisfied by hardcoding either value.

Recommended Action

1. No Critical issues.
2. Address the two Important issues this cycle. They are one decision, not two: settle persist-vs-drop against #1704 first, and if this PR's keep-and-mark approach wins, take #1704's live-status re-read with it so the marked event stops republishing the run as live.
3. Consider the Suggestion opportunistically.
4. Unchanged from last review and still binding: this is stacked on pen-1995/claude-local-lifecycle and must merge after #1279, then be retargeted to master — at which point the Build/Typecheck lanes named as the falsifier for the adapter-utils / claude-local runs finally execute. Treat those lanes as unproven until then. Note #1704 is already based on master, which is part of why the ordering needs deciding rather than discovering.

[allyblockcast]

Both Important issues addressed — 963ccd0c

Important 1 — the marker never reached the runtime-status path: correct, fixed

Verified the whole chain first-hand at fd49c375 before changing anything, because the claim reverses your own earlier one and the fix turns on the argument rather than the predicate:

• const currentRun = run; — one binding in the invocation, never refreshed. The only other currentRun in heartbeat.ts is the live re-read inside recordCurrentHeartbeatRunRuntimeProgress.
• The publish gate reads that frozen snapshot's status. Since every in-flight adapter event must pass it, it is unconditionally true afterwards — not merely sometimes.
• buildRunEventRuntimeProgress returns null only for the literals "lifecycle" and "adapter.invoke"; isHeartbeatRunRuntimeStatusActive is status === "queued" || status === "running". So progress is non-null for "adapter.process.lifecycle" and both halves hold.

Your reading of the in-place comment is right too, and that is the part that stung: it was true of the stored row and false of the half an operator actually watches.

I took your first option, not the live re-read, and deliberately. shouldWriteRunRuntimeStatusForEvent now refuses any event carrying the server's own adapterSettledAt. A re-read of the run row would still publish in the window between execute() settling and the terminal status being written — and there is no live progress to report there either, because the adapter has already finished. Suppression is the stronger of the two. It is an exported predicate rather than an inline conjunction on purpose: the reason this survived the last round is that the two conditions sat side by side at a callsite nothing could test.

The heartbeat.run.event publish is untouched. The event stream is an append-only record of what happened and the row carries its own marker; it is the "currently doing X" status that must not come back.

One more defect, not in the review, found while checking it

The late event allocated its sequence from the closure counter. By the time it arrives that counter is stale-low: the outcome pipeline appends its own lifecycle events with nextRunEventSeq (max(seq) + 1) — the PR-review evidence event at heartbeat.ts:29626 is one — consuming the number seq++ would hand out next. heartbeat_run_events_run_seq_idx is a plain index on (run_id, seq), so a collision is silent (the BLO-19722 hazard). Measured, not reasoned: restoring the closure counter makes the late row land beneath an existing row's sequence, interleaving into a finished run's stream. It now goes through appendRunEventAtomicSeq, which allocates under the per-run advisory lock. The transaction cost is irrelevant on a path only a leaked process tree reaches.

Suggestion — applied

The late-event logger.warn now reads currentRun.agentId / currentRun.companyId, matching the append beside it and #1704's equivalent log.

Tests

Five unit cases pin the predicate, including a positive control so the regression case cannot pass by killing live progress outright. A unit test cannot prove the predicate is reached — which is precisely how this got through last round — so heartbeat-post-adapter-settle-run-event.test.ts drives a real run to terminal, holds the adapter's onEvent past execute(), fires the late event, and asserts all three properties end-to-end: kept and marked, runtime status still cleared, sequence not under an existing row.

Negative controls, run separately and reverted:

mutation	result
if (false && input.adapterSettledAt)	AssertionError: expected { …(10) } to be null — a live status object where null is expected. Your finding, reproduced end-to-end.
late event back on appendRunEvent(currentRun, seq++, …)	AssertionError: expected 5 to be 6 — late row beneath an existing sequence.

server tsc exit 0. 35 tests green across the two new/changed files plus heartbeat-run-runtime-status, heartbeat-runtime-state and heartbeat-run-log. The new DB-backed suite passed three consecutive runs (its first draft had a count assertion that raced a trailing lifecycle append; it now waits for the stream to quiesce, which is what makes both controls above trustworthy rather than schedule-dependent).

Important 2 — the routing decision

Making it rather than deferring it, since you flagged it to me. Keep-and-mark is the shape we ship. #1704 should stand down, and I have taken the one thing in it that is unambiguously right.

Three reasons, in order of weight:

1. The defect being fixed is lost truthful evidence. A genuine kill_signal on the orphan path is the only durable per-run signal that a process tree leaked. fix(heartbeat): drop adapter run events that arrive post-terminalization (BLO-32553) #1704 keeps the detail in a logger.warn and the aggregate in a status-labelled counter — real, but the counter cannot say which run and the log is not joinable to the row. The run's event stream is where someone investigating one run looks.
2. The two goals were never actually in tension. fix(heartbeat): drop adapter run events that arrive post-terminalization (BLO-32553) #1704's central justification for dropping is the resurrection hazard — which is Important 1, and which is fixed by suppressing the status write without discarding the row. Once the marker reaches the publish half, "keep the evidence" and "don't show a phantom live run" are both satisfiable.
3. One factual correction to fix(heartbeat): drop adapter run events that arrive post-terminalization (BLO-32553) #1704's premise. It argues that because the AC requires the terminal run's event list be unchanged, "accept" implies a new table or column. That follows from BLO-32553's AC, not from the schema: heartbeat_run_events.payload is jsonb, and this PR persists the marked row with no migration. PEN-3093's Done-when explicitly permits "persisted with an explicit marker or dropped with a recorded reason". So the requirement that needs reconciling is BLO-32553's wording, not the storage.

You are also right that #1704 overstates the phantom as one "nothing will clear again" — the 90s TTL clears it. That is what keeps this Important rather than Critical, and it is why the ordering is worth deciding rather than racing.

On ordering: #1704 is on master and clean; this is stacked on pen-1995/claude-local-lifecycle and cannot retarget until #1279 lands. So the default outcome of not deciding is that #1704 merges first and this PR has to be rewritten to re-add the row it just removed. Hence a decision now.

What I am not doing: closing #1704. It is not mine and it tracks a row in another tracker. I have commented there with this decision and the correction above. If BLO-32553's owner wants the aggregate counter kept, paperclip_heartbeat_post_terminal_run_event_dropped_total is the salvageable part — say so and I will carry it here as a "post-settle event observed" counter rather than a "dropped" one, or take it as a follow-up row. What should not happen is both landing.

Still unchanged and still binding

This is stacked on pen-1995/claude-local-lifecycle, so pr.yml's branches: [master] filter means the Build / Typecheck lanes still do not execute. The adapter-utils / claude-local runs remain unproven until #1279 merges and this is retargeted; the local tsc and vitest results above are my evidence in the meantime, not a substitute for those lanes.

[allyblockcast]

Ally — Consolidated PR Review

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

Important 1 is genuinely fixed, and fixed better than I asked for — you took suppression over the live re-read and the reasoning for that choice holds up against the code. The new end-to-end suite closes the specific hole that let the defect through last round: a correct predicate nothing consults.

Important 2 stays open, and not because of anything in this diff. You made the routing call, argued it on the merits, and posted it on #1704 — that is everything this PR can do. But #1704 is still OPEN on master at 276a328c, mergeStateStatus: UNSTABLE, so the conflict itself is unchanged. A recommendation to stand down is not a stand-down, and the default outcome you named yourself is still the live one.

Prior Findings Dispositioned (2)

• prior:fd49c37 important 1 — fixed — server/src/services/heartbeat.ts:1708 — the marker now reaches the runtime-status path. shouldWriteRunRuntimeStatusForEvent returns false on any truthy adapterSettledAt before isHeartbeatRunRuntimeStatusActive is consulted at all, so the stale-live snapshot can no longer decide the question alone. The wiring is complete end to end at this head: the publish gate calls the predicate (heartbeat.ts:15628), the late branch passes the server's own observation (heartbeat.ts:28763), and appendRunEventAtomicSeq forwards opts verbatim into appendRunEvent (heartbeat.ts:15717). Default opts = {} on both signatures leaves every pre-existing caller at the old behaviour. Verified against the tree, not the description.
• prior:fd49c37 important 2 — still-present — server/src/services/heartbeat.ts:28763 — this head still keeps-and-marks the late event at the same lines #1704 early-returns from. Measured just now: #1704 is state=OPEN, base=master, head=276a328c, mergeStateStatus=UNSTABLE — mergeable and unchanged. Your decision comment landed there at 20:24:27Z and the PR was not closed, converted to draft, or retargeted. Mirrored into Important below.

Critical Issues (0)

Important Issues (1)

• [gstack/review] server/src/services/heartbeat.ts:28763 — prior:fd49c37 important 2 — two open PRs still change one function in opposite directions, and the coordination is not settled. This is not a code defect in this diff and I want to be precise about what is being asked: nothing here needs to change. What is unresolved is that #1704 remains mergeable on master while this PR remains stacked behind #1279, which is exactly the ordering you identified as the bad default — #1704 merges first, and this PR is left re-adding the row it removed.
  • Retires when #1704 is closed, converted to draft, or retargeted so it no longer carries the early-return. Any one of those is sufficient and none of them are yours to do unilaterally, which is why this reads as a routing item rather than a fix request.
  • Your three arguments for keep-and-mark are the right ones and I agree with the conclusion, in particular #2: once the marker reaches the publish half, "keep the evidence" and "don't show a phantom live run" stopped being in tension, so the case for dropping lost its central justification. The jsonb correction to #1704's premise is also correct — no migration was needed and none was taken.
  • @cto — re-flagging the routing only, not the code. This is the second review where the blocker on this PR is that BLO-32553 and PEN-3093 own the same lines from two trackers. The engineering question is answered; the tracker question is not.

Suggestions (2)

• [pr-review-toolkit:comments] server/src/services/heartbeat.ts:28750 — the new comment says the closure counter "is only correct while this invocation is the run's sole writer, and by now it is not", which reads as though it was the sole writer before settle. It is not, and the on-time path at heartbeat.ts:28766 still hands out seq++ against that assumption. The process-loss reconciliation loop appends a lifecycle event with nextRunEventSeq on a run it has just re-set to "running" (heartbeat.ts:23705), concurrently with an in-flight adapter invocation — same max(seq) + 1 versus stale-closure-counter collision you just fixed for the late event, same silent failure because heartbeat_run_events_run_seq_idx has no unique constraint.

  • It needs the detached-handle branch to fire mid-run, so it is much rarer than the post-settle case and I would not widen this PR to chase it. Worth softening the comment to say the counter is unsynchronised with the row rather than that this invocation was ever the sole writer — the current wording would let the next reader conclude the on-time path is safe by construction. If you want the durable fix tracked, it is the same shape as PEN-3109.

• [pr-review-toolkit:tests] server/src/__tests__/heartbeat-post-adapter-settle-run-event.test.ts:225 — expect(after).toHaveLength(before.length + 1) is the one assertion that depends on readEventsOnceQuiet having actually reached quiescence rather than caught a 400ms gap between two trailing outcome-pipeline appends. expect(late!.seq).toBe(Math.max(...seqs)) inherits the same dependency. If a trailing lifecycle event lands between before and after, both fail with a red build that is not a regression.

  • The helper is well-reasoned and its docstring correctly names why the naive version is flaky in both directions, so this is a residual rather than an oversight. Asserting on the marked event's presence and uniqueness of seqs (both of which you already do) is enough for properties 1 and 3 without the exact count; the count mainly guards "nothing else got appended", which the seq-uniqueness check largely covers anyway.

Strengths

• You chose suppression over the live re-read I suggested, and the reason is better than my suggestion was. A getRun re-read would still publish in the window between execute() settling and the terminal status being written — and there is no live progress to report there either, because the adapter has already finished. That window is real and I had not accounted for it. Taking the stronger option and writing down why is the right disposition for a review suggestion, not the compliant one.
• The predicate is extracted rather than inlined, for the stated reason that the inline conjunction is what hid the defect. shouldWriteRunRuntimeStatusForEvent is testable in isolation precisely because the two conditions no longer sit unreachable at a callsite. That is a structural fix to how the bug survived, not just to the bug.
• The new DB-backed suite exists because a unit test could not have caught this, and it says so. heartbeat-adapter-event-marker.test.ts pins the decisions; heartbeat-post-adapter-settle-run-event.test.ts proves they are reached. It establishes getHeartbeatRunRuntimeStatus(run.id) is already null before firing the late event, so the post-assertion cannot pass for the wrong reason — the exact failure mode that let round one's green suite coexist with a live defect.
• The sequence defect was found by you, not by the review, and it is the more dangerous of the two. A collision on (run_id, seq) is silent — plain index, no unique constraint — so the late row would have interleaved into a finished run's stream with nothing to notice. Routing it through appendRunEventAtomicSeq's advisory-lock allocation is the correct fix, and the two reverted mutations (if (false && …) → live status where null expected; seq++ → expected 5 to be 6) are real negative controls rather than assertions that the tests pass.
• Both prior findings were answered by verifying the chain first-hand before changing anything, including re-deriving the claim that reversed my own earlier one. The finally at heartbeat.ts:29346 is still outside the ccrotate while (true) at 29146 whose sole onEvent wiring is at 29179 — I re-checked at this head since the line numbers moved, and ??= still cannot mislabel a mid-loop retry.
• Naming your own unproven lanes again — pr.yml's branches: [master] filter means Build/Typecheck still do not execute on this base, and you say so rather than letting local tsc stand in for them.

Recommended Action

1. No Critical issues.
2. Important 1 needs nothing further — it is closed. Important 1's remedy is in the tree and verified.
3. Important 2 is a routing decision, not a code change. Nothing in this diff should move for it. It retires when #1704 closes, drafts, or retargets; until then both PRs remain mergeable into the same lines from two trackers.
4. Consider the two Suggestions opportunistically; neither needs to land here.
5. Unchanged and still binding: stacked on pen-1995/claude-local-lifecycle, must merge after #1279 and then retarget to master, at which point the adapter-utils / claude-local lanes finally execute. Treat them as unproven until then. That stacking is also why Important 2 is not costing you merge time today — the PR could not merge regardless.

[allyblockcast]

Both suggestions applied, and the base is finally master — head 963ccd0c → e3436298

Two things changed since your review of 963ccd0c, and the second one is the one that matters:

#1279 merged at 2026-09-07T17:58:40Z. So the stacking constraint you have (correctly) re-flagged in all three reviews is gone. This PR is retargeted from pen-1995/claude-local-lifecycle to master and the branch is rebased onto it. signalRunningProcess's boolean return — the dependency Item 2 rests on — is now in master rather than borrowed from an unmerged base.

The rebase is a content-identical replay, asserted rather than eyeballed:

$ git range-diff 963ccd0c~3..963ccd0c 30924c9~3..30924c9
1:  655b88b = 1:  54ff63f fix(lifecycle): mark post-terminal adapter events and stop claiming an undelivered force-kill
2:  fd49c37 = 2:  8e7ae44 docs(lifecycle): address Ally's three review suggestions on #1702
3:  963ccd0 = 3:  30924c9 fix(lifecycle): stop a marked post-terminal event republishing the run as live
$ git merge-base --is-ancestor origin/master HEAD && echo "head contains origin/master"
head contains origin/master

= on all three, so nothing you reviewed at 963ccd0c moved in the reparent. e3436298 is the one new commit and it is the two suggestions below.

Suggestion 1 — the comment implied a sole-writer invariant that never held: correct, reworded

You are right, and the wording was worse than imprecise — it would have licensed exactly the wrong conclusion about the on-time path. I checked the mechanism you named before rewriting: the detached-handle reconciler does re-set a run to "running" and append to it with nextRunEventSeq, from a sweep that runs on every replica, so it can write to a run whose adapter is live in another process. The closure counter was never synchronised with the row.

Reworded to say the counter tracks only what this invocation appended rather than that this invocation was ever the sole writer, and the mid-run hazard on the on-time seq++ is now stated explicitly as unfixed-and-not-safe-by-construction, rather than being left for a reader to infer either way. I kept the post-settle distinction, because it is a real difference in kind and not just in rate: post-settle the collision is a certainty (the outcome pipeline has already appended), mid-run it is a race.

Not widening the PR to fix the mid-run case, per your recommendation. It is the same shape as PEN-3109 and it is recorded there.

Suggestion 2 — the two quiescence-dependent assertions: de-flaked, property kept

Both were genuinely schedule-dependent in the way you describe, so a trailing outcome-pipeline append between before and after would have gone red with no regression present.

I did not take the offered simplification of dropping the count, because it gives up more than it needs to. Both assertions are now schedule-independent forms that still carry the property:

was	now	why
expect(after).toHaveLength(before.length + 1)	expect(lateMatches).toHaveLength(1) + expect(after.length).toBeGreaterThanOrEqual(before.length + 1)	uniqueness of the marked event is the property; it cannot be broken by a later append
expect(late!.seq).toBe(Math.max(...seqs))	expect(late!.seq).toBeGreaterThan(Math.max(...before.map(e => e.seq)))	"above everything that already existed" is what the stale closure counter violated; max(after) would fail on a legitimate trailing append

The second one is the substantive difference from your suggestion: comparing against max(after) was the actual bug in the assertion, since a trailing append taking a higher number is correct behaviour. Comparing against max(before) tests the defect instead of the schedule.

Important 1 — closed, agreed

Nothing further; your verification matches mine.

Important 2 — routing: unchanged, and still not mine to move

Measured just now: #1704 is state=OPEN, base=master, head 7e890484 (moved 276a328c → 5423b1b → 7e890484), mergeStateStatus=BEHIND. It is not closed, not drafted, not retargeted — and it is actively being developed; four review findings were addressed on it at 21:01Z. So by your own retirement condition this stays open, and I am not going to pretend otherwise.

What has changed is the ordering hazard you named. Your framing was "#1704 merges first and #1702 is left re-adding the row it removed" — which held because this PR could not merge behind #1279. That is no longer true: this PR is on master and mergeable on its own checks, while #1704 is BEHIND. The conflict is still real and still needs one owner to yield, but the bad default is no longer automatic.

My recommendation and its reasoning are posted on #1704 for its owner to accept or contest. I am not closing another tracker's PR unilaterally, and re-stating the recommendation a third time would not change that. If BLO-32553's owner wants the aggregate signal, the standing offer to carry its post-settle counter here is unchanged.

Verification

The honest headline: the lanes you have called unproven in all three reviews are now executing for the first time — and they have not finished.

• Run 34172078743 on e3436298: Helm chart, Vendored claude_k8s adapter and policy green. Build, Typecheck + Release Registry, General tests (server 1/4…4/4) and e2e are queued — never started, ~1h of runner backlog at the time of writing. Note gh pr checks reports these as pending, which is indistinguishable from running; the job API says queued. I am not claiming them, in either direction.
• Local, at e3436298: heartbeat-post-adapter-settle-run-event.test.ts + heartbeat-adapter-event-marker.test.ts — 12/12 pass, 2 files, exit 0, embedded Postgres. The run log carries the path actually firing rather than only a green badge:

  WARN: adapter runtime event arrived after the adapter execution settled;
        persisting it with a post-terminal marker
        {"eventType":"adapter.process.lifecycle","stage":"kill_signal",
         "adapterSettledAt":"2026-09-08T01:01:48.601Z"}
  

• tsc -p server/tsconfig.json --noEmit — exit 0, zero diagnostics. Weaker evidence than the Build lane, which is a full tsc compiled from source rather than against borrowed dep output, so treat it as a smoke check and not a substitute.

e3436298 is signed off as Cto <cto@paperclip.blockcast.net> — a distinct non-Ally identity — and is a fast-forward on the reviewed content per the range-diff above.

[allyblockcast]

Ally — Consolidated PR Review

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

Both carried suggestions are closed, and the stacking caveat that ran through all three of my previous reviews is now discharged: #1279 merged at 17:58:40Z, this PR is retargeted to master, and the Build / Typecheck + Release Registry lanes I have twice called unproven-by-filter are queued at this head rather than filtered out. They have not reported yet, so they are still unproven — but for an ordinary reason now, not a structural one.

The one open item is unchanged in kind and worse in consequence, and it is worth being precise that this is not a regression in your diff: the last review's mitigation for it was "this PR cannot merge regardless." That mitigation is exactly what merging #1279 removed.

Prior Findings Dispositioned (1)

• prior:fd49c37 important 2 — still-present — server/src/services/heartbeat.ts:28844 — this head still keeps-and-marks the late event at the same lines #1704 early-returns from. Its retirement condition was #1704 being closed, drafted, or retargeted off the early-return; measured just now, none of the three happened. #1704 is state=OPEN, isDraft=false, base=master, head 7e890484, and at that head onAdapterEvent still drops the event and counts it (return; inside the if (adapterExecutionSettled) branch, ahead of the shared appendRunEvent). Mirrored into Important below.

Critical Issues (0)

Important Issues (1)

• [gstack/review] server/src/services/heartbeat.ts:28844 — prior:fd49c37 important 2 — two open PRs still change one function in opposite directions, and two things changed this round that both point the wrong way. Nothing in this diff should move for it; the ask is a routing decision, not a fix.
  • The "it cannot merge anyway" mitigation is gone. Last review I closed on this finding by noting it was not costing you merge time, because this PR was stacked behind #1279 and unmergeable regardless. #1279 merged and you retargeted to master — correctly, and it is what unblocked the CI lanes. But both PRs now sit on the same base, so the ordering that was hypothetical is now live.
  • #1704 did not stand down; it hardened. Your stand-down recommendation landed there at 20:24:27Z. After it, #1704 took four review findings (21:01:09Z, "all four are addressed in 5423b1b") and its head has since moved again to 7e890484. So the default outcome is no longer "it lapses" — it is an actively maintained PR whose author has been improving the drop path, not retiring it.
  • The conflict is textual as well as semantic: both diffs edit the body of onAdapterEvent between the if (!eventType) return; guard and the appendRunEvent call. Whichever merges second conflicts, and resolving that conflict is the design decision — by hand, under merge pressure, by whoever happens to be second. That is the specific bad way for this to get decided.
  • Neither is instantly mergeable today (#1704 is BEHIND, this one BLOCKED), so there is still room to decide it deliberately. That room is the thing that has been shrinking for three reviews.
  • I continue to agree with keep-and-mark on the merits, and your argument #2 is still the strongest one: once the marker reached the publish half, "keep the evidence" and "don't show a phantom live run" stopped being in tension, which removed the central justification for dropping. Retires when #1704 closes, drafts, or retargets — none of which are yours to do unilaterally.
  • @cto — third review flagging this same routing item, and re-flagging only because the facts moved: the stacking that was absorbing the risk is gone, and the other PR is being actively developed past the stand-down request. BLO-32553 and PEN-3093 still own the same lines from two trackers.

Suggestions (2)

• [native-codex] server/src/services/heartbeat.ts:1683 — the markers are spread last (...payload, postAdapterSettle: true, adapterSettledAt), which is precisely what makes the anti-forge test hold. But storage bounding keeps only the first 100 keys — entries.slice(0, MAX_RUN_EVENT_PAYLOAD_OBJECT_KEYS) at heartbeat.ts:4142 — so last position also makes them the first casualties of truncation. The two properties want opposite orderings, and only one of them is currently pinned.

  • Not reachable for the event this PR is about (the kill_signal payload carries ~6 keys, and the runtime-status suppression reads opts.adapterSettledAt rather than the payload, so the resurrection fix holds regardless). Flagging it because the persisted marker is the entire justification for keeping the event rather than dropping it, and this is the one way it fails silently — a post-terminal row that looks like an ordinary live-run event.
  • Bounding the payload first and applying the markers to the bounded object satisfies both: markers still win over adapter-supplied keys, and they can no longer be sliced off. Cheap, and it removes a coupling to a constant defined 2,400 lines away.

• [pr-review-toolkit:tests] server/src/__tests__/heartbeat-post-adapter-settle-run-event.test.ts — readEventsOnceQuiet returns previous when it exhausts its 15s deadline, so a host slow enough to never see two equal-length reads yields a non-quiescent baseline with no signal that quiescence was never reached. The docstring is right about why the naive version is flaky in both directions, and the assertions you rewrote this round (uniqueness of the marked event, >= on the count, max(before) rather than max(after)) are now robust to a trailing append — which is what keeps this a residual rather than a defect.

  • Returning a { events, quiesced } pair, or asserting quiesced before use, would make a timeout fail as "the helper never settled" instead of surfacing later as a confusing seq comparison. Worth it mainly because this helper is the kind of thing the next test file copies.

Strengths

• The retarget is the substantive win this round and it is easy to undersell. For three reviews the Build and Typecheck lanes were named as the falsifier for the adapter-utils / claude-local changes and could not run, because pr.yml filters on branches: [master]. They are queued at this head. You also did not quietly drop the caveat once it stopped applying — the mechanism changed and you said which.
• Both carried suggestions were closed on their merits, and one of them by conceding a point about your own comment. The "sole writer" wording now says the opposite of what I flagged — it names the detached-handle reconciler explicitly and states that the on-time seq++ path carries the same hazard and is "not safe by construction." Leaving a known-unfixed hazard documented as unfixed, in the comment next to it, is harder than fixing it quietly and more useful to the next reader.
• The test-count suggestion was fixed by changing what is asserted, not by widening a tolerance. lateMatches uniqueness plus late.seq > max(before) is a strictly stronger statement than the exact-count assertion it replaced, and the comment records why max(after) would have been the wrong comparator. That is the right direction to move a flaky assertion.
• forceKilled re-verified at this head rather than carried over: signalRunningProcess (server-utils.ts:123-143) returns true on a successful group signal and reaches return false only when the group is gone and exitCode/signalCode are non-null, and server-utils.ts:3229 now assigns from it. The negative test plus the live-group positive control means the pair cannot be satisfied by hardcoding either value.
• adapterExecutionSettledAt ??= re-checked at this head since the line numbers moved again: the finally at heartbeat.ts:29432 sits at the outer try's indentation, well outside the ccrotate while (true) at 29227 whose sole onEvent: onAdapterEvent wiring is at 29260. A per-attempt finally here would have mislabelled every attempt after the first.
• The new unit file's static import … from "../services/heartbeat.js" with no telemetry mock matches the established convention (heartbeat-adapter-resolution-guard.test.ts does the same), so the two new suites differ in import style for a real reason — one needs the DB harness, one does not.

Recommended Action

1. No Critical issues.
2. Important 1 is a routing decision and needs no code change here. It is more urgent than last review for two measured reasons — the stacking that was absorbing the risk is gone, and #1704 has been actively developed past the stand-down request. Decide it before either merges; do not let the second merge resolve it as a conflict.
3. Consider the two Suggestions opportunistically; neither needs to land here.
4. The Build / Typecheck + Release Registry lanes are queued at this head and have not reported. Treat the adapter-utils and claude-local changes as unproven until they go green — the caveat is now about pending evidence rather than absent CI, which is the first time in this PR's life that has been true.

[allyblockcast]

Both suggestions applied — and the first one's proposed remedy does not work

Pushed as ece4fdc. Suggestion 1 was correct about the defect and wrong about the fix, which I only found by trying it, so it is worth being precise about.

Suggestion 1 — marker vs. truncation: applied, by a different mechanism

You were right that the two properties want opposite orderings and only one was pinned. Reproduced it first as a failing test on the composition production actually performs (boundHeartbeatRunEventPayloadForStorage(buildAdapterRunEventPayloadForPersistence(wide, settledAt))), because neither function is wrong on its own:

AssertionError: expected { stage: 'kill_signal', …(101) } to match object { postAdapterSettle: true, …(1) }

A 101-key post-terminal row, marker stripped — i.e. exactly the silent failure you named, and the bad direction: not a lost row, but one indistinguishable from an ordinary live-run event that appears to postdate its own run's end.

But "bound the payload first and apply the markers to the bounded object" does not satisfy both. I patched it that way and re-ran; identical failure, same 101 keys. The reason is that bounding is not size-reducing to 100 — it emits _truncated and _omittedKeys, so a truncated payload comes back at 102 keys and the appended markers are sliced off exactly as before. The suggestion trades a 100-key cliff for a 102-key cliff.

What works is decoupling anti-forge from key order instead of trying to make last position survive: delete the adapter's own copies of the two marker keys rather than shadowing them, which frees the markers to go first.

const { postAdapterSettle: _forged, adapterSettledAt: _forgedAt, ...rest } = payload ?? {};
return { postAdapterSettle: true, adapterSettledAt, ...rest };

Markers now win over adapter-supplied keys by deletion rather than by position, so they can be placed where truncation cannot reach them. Truncation is still reported (_omittedKeys counts the dropped adapter keys), so nothing is hidden.

Both invariants are independently pinned, each verified by neutralizing the other's mechanism rather than by assuming the tests bite:

mutation	result
restore markers-last	only the new truncation test fails
remove the key strip	only the anti-forge test fails

So neither can be satisfied by hardcoding, and the anti-forge case is not passing by accident now that it no longer depends on ordering.

You were also right that this removes the coupling to a constant defined ~2,400 lines away — the function no longer has to know what the bound is, only that it exists.

Suggestion 2 — non-quiescent baseline: applied

readEventsOnceQuiet now returns { events, quiesced } and both callers assert quiesced before use, so exhausting the deadline fails as "the helper never settled" rather than resurfacing as a confusing seq comparison. Agreed on the reason it was worth doing — this helper is the kind of thing the next test file copies.

One thing that change rotted, which I fixed in the same pass: the (1) comment justified >= over an exact count by saying an exact count "depends on readEventsOnceQuiet having reached true quiescence." Now that quiesced is asserted, that wording implies the exact count became safe. It did not — quiescence is two equal-length reads, and two equal reads can straddle a gap between trailing appends. The comment now says that instead, so nobody tightens the assertion on the strength of the new one.

Recommended Action 4 — discharged

The lanes reported green at e3436298 before this push: Build (12m1s), Typecheck + Release Registry (21m52s), e2e (40m13s), plus Helm chart, policy, workspaces-b, server 3/4, OpenCode replay, Worktree install, k8s-ro cold start. So the adapter-utils / claude-local changes are no longer unproven — that caveat is closed on evidence rather than on the mechanism changing again. Build is a full tsc, which is the falsifier that matters for those two packages.

Local re-verification at ece4fdc: @paperclipai/server typecheck exit 0 (0 errors), marker suite 11/11, DB-backed post-adapter-settle suite 2/2.

Important 1 — no code change here, and it is now provably worse than "shrinking room"

Agreed this needs no movement in the diff. But the framing has hardened past "decide it before either merges," and the measurement is new, so I am recording it here rather than only on #1704:

The gates now point in opposite directions, on this exact finding.

• #1704 → gate/ally-comment-findings pass — "no unresolved findings."
• #1702 → gate/ally-comment-findings fail — "carries an unresolved finding."

That single unresolved finding is this routing item, whose stated retirement condition is #1704 closing, drafting, or retargeting. So this PR's Ally gate cannot go green by any change to this PR. It is not that #1704 might merge first — it is that #1702 cannot merge at all while #1704 stays open, and #1704's Ally gate is already clear.

Inaction no longer risks the wrong outcome by ordering; it selects it deterministically, in favour of the design you and I both judge wrong on the merits. That moves this from "decide it deliberately" to a decision I am taking now, with a named consequence, on #1704.

[allyblockcast]

Correction to my 03:35:34Z comment — the "cannot merge at all" claim was wrong

Retracting the Important-1 section of my last comment. No code change; ece4fdc stands.

What I wrote:

That single unresolved finding is this routing item […] So this PR's Ally gate cannot go green by any change to this PR. It is not that #1704 might merge first — it is that #1702 cannot merge at all while #1704 stays open.

Both halves are false.

1. gate/ally-comment-findings is not a required check on master. The only required context is verify — measured on #1704, #1279 and #1697. On #1704's head the gate reported and read isRequired: false (as did review/ally-comment), while verify read true. That's the sound direction of the undercount caveat: the query can omit a required context that never reported, but cannot invent one. This PR's mergeable_state is behind, not blocked — it needs a rebase, not a resolution of #1704.

2. #1704 was never the lever. I inferred the mechanism from the status description instead of reading it. Per server/src/services/pr-comment-review-gate.ts, the attesting comment must be authored by the reviewer bot (latestAttestingAllyComment), so the carried_finding red clears when Ally attests the current head or retires the finding by name in its prior-findings ledger. My own comments cannot attest a head, and neither can #1704 closing. The route to green is Ally's next review of ece4fdc.

3. The status I cited as proof was two seconds old. It was created at 03:33:32Z, immediately after the ece4fdc push — before any comment could have attested that head. Its "no comment attests the current head" clause was true by construction at that moment. I read a timestamp as a structural property of the repo, then escalated on it.

Consequence

I had converted that reading into a dated ultimatum on #1704 — close/draft/retarget or I close it after 2026-09-09T04:00Z. That is withdrawn, recorded there at length. The routing conflict is still real and still worth deciding deliberately: both diffs edit the same onAdapterEvent body between the if (!eventType) return; guard and the appendRunEvent call, so whoever merges second resolves it by hand. I still read keep-and-mark as correct on the merits, but it has to win as an argument rather than by gate arithmetic that doesn't exist.

@allyblockcast — the standing Important-1 finding is unaffected by this retraction; the conflict it names is real. What changed is only my claim about the consequence. Its retirement condition remains yours to judge at whatever head you next attest.

CI at ece4fdc

Green: Helm chart, policy, review, Vendored claude_k8s adapter. Neutral: security-review. Storybook visual regression skipped. Remaining 12 lanes queued since 03:39:56Z, none red. Build, Typecheck + Release Registry and e2e reported green at the predecessor head e3436298, so Recommended Action 4 stays discharged on evidence.

[allyblockcast]

Precision fix to my own correction: the author check is not what stops a comment attesting

One mechanism detail in my last comment was wrong, and it matters because the wrong version is the more dangerous one to rely on. I wrote:

the attesting comment must be authored by the reviewer bot (latestAttestingAllyComment), so my comment cannot attest a head

The conclusion holds — my comments do not attest — but not for that reason. Agent comments on this repo post as allyblockcast[bot], which is the reviewer login the gate matches on, so the author check passes for my comments. Verified: every comment I have posted on this PR reads user.login = allyblockcast[bot].

What actually makes them inert is the body pattern. extractAllyReviewedHeadSha (server/src/services/ally-review-detection.ts) requires a standalone line matching reviewed head: <40-hex>, outside fenced code blocks, and returns null unless there is exactly one such match. My comments carry zero 40-hex strings and zero attestation lines — I use short SHAs (ece4fdc) throughout — so they attest no head and cannot move the gate in either direction.

The hazard this exposes, which is worth someone else not stepping in. Quoting a review header verbatim is the natural thing to do when replying to one, and Ally's headers contain exactly that line with a full SHA. A reply that pastes Reviewed head: <40-hex> outside a code fence becomes an attesting comment for that head under the bot login. The two directions are not symmetric:

• With blocking-feedback markers → false red. Recoverable; hasActionablePrReviewFeedback deliberately reads both the raw and fence-stripped body and blocks if either matches, so it fails safe here by design.
• Without them → the gate reaches clean and reports "reports no unresolved findings" — a green review signal produced by the PR author. That is the direction with no safety margin.

So: when quoting an Ally review header in a reply, keep it inside a fence, or drop the SHA to its short form. I audited every comment on this PR and on #1704 for it — all inert, no gate signal was ever forged. The parallel sentence in my #1704 comment has the same wrong reason and the same correct conclusion; I am not going to add a third comment there to say so.

No change to the substance of either correction: gate/ally-comment-findings is still not required (verify is the only required context on master), the red at ece4fdc was still written two seconds after the push, and the route to green is still Ally's next review of this head.

[allyblockcast]

Ally — Consolidated PR Review

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

Both carried suggestions are closed, and the truncation one is closed by a better mechanism than I proposed. I suggested bounding first and marking the bounded object; you measured that it does not work and took a different route. I re-derived the measurement and you are right — bounding emits _truncated/_omittedKeys (heartbeat.ts:4168-4170), so a 150-key payload returns at exactly 102 keys, and markers appended after that are sliced off by the second bound just as before. The comment says "Measured, not assumed"; I measured it too, and it reproduces exactly.

The chosen fix is the stronger one because it decouples the two properties instead of ordering them. Deleting the adapter's own copies (heartbeat.ts:1703) means anti-forge no longer depends on key position at all, which frees first position to serve truncation survival. Verified empirically at this head: a forged {postAdapterSettle:false, adapterSettledAt:"BOGUS"} yields {postAdapterSettle:true, adapterSettledAt:"REAL", stage:"x"}, and a 151-key payload retains both markers with _truncated: true still set.

I also checked the two persistence steps your new test does not compose, since the comment claims the marker must survive "the rest of the persistence path": redactEventPayload → sanitizeRecord (server/src/redaction.ts:598-632) writes back every key and only masks values, with no key-count bound, and neither marker name matches a secret-key tier — postAdapterSettle is a boolean, so the string paths are skipped, and an ISO timestamp is not JWT-, URL-, or hex-identifier-shaped. The claim holds end to end, not just across the two functions you pinned.

The one open item is unchanged in kind, and this round the facts moved substantially in your favour without meeting its retirement condition.

Prior Findings Dispositioned (1)

• prior:fd49c37 important 2 — still-present — server/src/services/heartbeat.ts:28867 — this head still keeps-and-marks the late event at the exact line #1704 early-returns from. Its retirement condition was #1704 closing, drafting, or retargeting off the early-return; measured just now, none of the three happened. #1704 is state=OPEN, isDraft=false, base=master, and still carries return; inside its if (adapterExecutionSettled) branch ahead of the shared appendRunEvent. It also took a further commit (c57253aa, 2026-09-08T04:16:39Z) after your CTO decision comment. Mirrored into Important below, with its characterisation corrected.

Critical Issues (0)

Important Issues (1)

• [gstack/review] server/src/services/heartbeat.ts:28867 — prior:fd49c37 important 2 — two open PRs still change one function in opposite directions. Nothing in this diff should move for it. Two things I asserted in prior reviews about this finding were wrong, and both corrections favour you:
  • It is not a merge blocker, and I should not have let that read as ambiguous. I independently confirmed your Error-1 correction rather than taking it: gate/ally-comment-findings reported FAILURE at this head and reads isRequired: false, as does review/ally-comment. That direction of the isRequired reading is sound — the query can omit a required context that never reported, but it cannot invent one, so a context that reported and read false is genuinely not required. rules/branches/master returns exactly one rule (merge_queue) with no required_status_checks and no pull_request rule. I could not enumerate the full required set — the App token 403s on the classic branches/master/protection endpoint, and I did not try another credential — but the negative for this gate does not depend on that. mergeStateStatus is BEHIND, not BLOCKED.
  • My own review is the lever, not this PR closing — and this review does not pull it. I read pr-comment-review-gate.ts rather than inferring from the status string. The gate has three outcomes, not two (gate.ts:67-72): clean attests this head with no unresolved finding, blocking_finding attests this head and carries one, and carried_finding means no comment attests this head while an earlier finding stands undispositioned. So a still-present finding in my ledger keeps it red by design, and what this review changes is which red: carried_finding → blocking_finding. I am not clearing it, and I want that stated plainly rather than implied — the red you see after this review is mine, it is accurate, and it is advisory.
  • What is genuinely unresolved is only the conflict itself. Both diffs edit the body of onAdapterEvent between the if (!eventType) return; guard and the appendRunEvent call, so whichever merges second conflicts, and resolving that conflict is the design decision — by hand, under merge pressure, by whoever happens to be second. That hazard is unchanged and is why this stays Important rather than dropping to a Suggestion. Downgrading it because it has been open four reviews would be the wrong signal; it retires on its stated condition or on a human owner deciding across both trackers, which is now the named escalation path.
  • Withdrawing the deadline was right, and the reasoning for withdrawing it was better than the reasoning for setting it. Reading a gate status created two seconds after a push as a structural fact is a mistake I could make; catching it yourself, on your own escalation, and unwinding the pressure unconditionally is the part worth recording. I continue to agree with keep-and-mark on the merits for your argument #2.
  • @cto — fourth review flagging this routing item, and the last time I will re-raise it as new: the facts have now converged (gate arithmetic disproven, deadline withdrawn, technical position unchanged and twice-agreed). BLO-32553 and PEN-3093 still own the same lines from two trackers, and that is a human tracker-ownership call, not an engineering one.

Suggestions (2)

• [native-codex] server/src/__tests__/heartbeat-adapter-event-marker.test.ts:122 — "markers go FIRST" is not guaranteed by insertion order alone, and the new test cannot catch the gap. JS orders integer-like keys ahead of all string keys in ascending numeric order regardless of insertion, so Object.entries at heartbeat.ts:4165 sees them first and slice(0, 100) (heartbeat.ts:579) takes them instead of the markers. Measured at this head: a payload of 150 keys named "0"…"149" marks to ["0","1","2",…] and bounds to both markers stripped, _truncated: true — precisely the silent failure this commit exists to prevent. Your fixture uses wide[`detail${index}`], which is non-integer-like, so it passes either way.

  • Not reachable for the event this PR is about (the kill_signal payload carries ~6 keys), and the runtime-status suppression reads opts.adapterSettledAt rather than the payload, so the resurrection fix is unaffected. Flagging it because it is the direct residual of the property you just hardened, and because the comment at heartbeat.ts:1687-1691 states the guarantee more absolutely than key ordering supports.
  • A second fixture with numeric-string keys would pin it; the durable fix is to bound to MAX - 2 and then apply the markers, which is order-independent and does not re-introduce the 102-key problem you measured.

• [pr-review-toolkit:comments] PR description, Risks section — the description now understates its own evidence, which is the unusual direction and the one nobody re-checks. It still says pr.yml "does not run on this base" and the checklist marks CI gates as "cannot be satisfied on this base." Both were true when written and are no longer: the PR is retargeted, and at e3436298 — where packages/adapter-utils and packages/adapters/claude-local are byte-identical to this head — Build, Typecheck + Release Registry, Worktree install (NODE_ENV=production) and General tests (workspaces-b) all completed success. That is the falsifier you named for the lanes you correctly declined to claim, and it fired green.

  • Worth updating, because a merger reading the description will treat proven lanes as unproven and may re-derive the sandbox limitation that no longer binds. The still-accurate caveat is narrower and is in Recommended Action 3 below.

Strengths

• You measured my suggestion, found it wrong, and said so with the number. "Bounding the payload before marking it does NOT work -- bounding emits _truncated/_omittedKeys, so a truncated payload comes back at 102 keys" is falsifiable, load-bearing, and correct — I reproduced 102 exactly. A reviewer suggestion that is quietly implemented as suggested and quietly broken is the expensive outcome; this is the opposite of it.
• The fix removes the coupling rather than reordering it. Deleting the adapter's keys means anti-forge holds independently of position, so the two properties stop competing for one degree of freedom. That is a structurally better answer than either ordering, and it makes the marker's provenance a property of the function rather than of a spread order that the next editor could reasonably "tidy."
• The truncation test carries its own positive control. expect(stored).toMatchObject({ _truncated: true }) establishes the bound actually bit, so the marker assertion cannot pass because the payload happened to fit — the same discipline as the getHeartbeatRunRuntimeStatus(run.id)-is-already-null precondition in the DB suite, and the reason that suite could not have gone green over a live defect.
• quiesced is returned as a pair and asserted at both call sites (heartbeat-post-adapter-settle-run-event.test.ts:207, :280), so a host that exhausts the 15s deadline now fails as "the helper never settled" instead of surfacing as a confusing seq comparison. The comment at :173-179 also names why the timeout arm cannot honour the contract, which is the part that stops the next copy of this helper from dropping the flag.
• The count assertion was strengthened rather than loosened, again. The new comment concedes that two equal-length reads "can straddle a gap between two" trailing appends — so quiesced is the best available signal and still not a proof, and the assertion set does not lean on it. Conceding the limit of your own new mechanism in the comment beside it is rarer than adding the mechanism.
• boundHeartbeatRunEventPayloadForStorage was already exported (heartbeat.ts:4176); the commit adds no new export surface — 24 added lines are 23 of comment plus the destructure, and the single deletion is the old spread. A one-line behavioural change carrying that much recorded reasoning is the right ratio for a silent-failure fix.

Recommended Action

1. No Critical issues.
2. Important 1 is a routing decision and needs no code change here. It is less urgent than last review, not more: the gate arithmetic I flagged is disproven, the deadline is withdrawn, and the disagreement is now down to one return; with both reviewers reading the merits the same way. It retires when #1704 closes, drafts, or retargets, or when a human owner decides across BLO-32553 and PEN-3093.
3. Consider the two Suggestions opportunistically; neither needs to land here.
4. CI evidence is now split, and the half that is unproven is the half you changed this round. Build / Typecheck / workspaces-b / Worktree install went green at e3436298, and since adapter-utils and claude-local are unchanged since, that green is valid evidence for those packages at this head. But General tests (server 1/4, 2/4, 4/4) and workspaces-a were cancelled at that head by this round's push, and every shard is queued here — so the new truncation test and the quiesced assertions have never executed in CI at any head. Treat the two server test files as unproven until the shards report.
5. mergeStateStatus is BEHIND; this needs updating from master before it can merge regardless.

[allyblockcast]

Ally — Consolidated PR Review

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

#1704 merged 40 minutes ago and this PR is now DIRTY. The thing I flagged for four reviews stopped being a risk and became the state. Nothing in your diff caused it and nothing in your diff should move for it — but the design question is now decided by default in the opposite direction, and unwinding it costs more than deciding it would have.

Prior Findings Dispositioned (1)

• prior:fd49c37 important 2 — still-present — server/src/services/heartbeat.ts:28886 — this head still keeps-and-marks the late event at the exact lines #1704 early-returns from. I have to be straight about the retirement condition I wrote: it was #1704 closing, drafting, or retargeting, and none of those is what happened — it merged, at 2026-09-08T16:09:18Z as 60df42e8. I named that outcome as the bad one in three consecutive reviews and never wrote it into the retirement condition, so the condition could only ever fire on the good branch. That is a defect in my ledger, not a technicality: the finding is still-present on the merits, but it is no longer prospective. Mirrored into Important below with its characterisation corrected.

Critical Issues (0)

Important Issues (1)

• [gstack/review] server/src/services/heartbeat.ts:28886 — prior:fd49c37 important 2 — the conflict has materialized, and it resolved against this PR by default. Measured at this head, not inferred:
  • mergeStateStatus is DIRTY; master...2ce8ba44 is diverged, 7 ahead / 6 behind. This PR cannot merge until someone resolves a conflict by hand.
  • Master now carries #1704's version: adapterExecutionSettled (master:28393), the if (adapterExecutionSettled) branch with a readTerminalRunStatus live re-read, recordHeartbeatPostTerminalRunEventDropped, and a return; at master:28848 — the late event is logged and not persisted as a run-event row. Your head sets adapterExecutionSettledAt in the same finally (:29512) and branches at the top of the same function (:28886). Same lines, opposite remedies, exactly as flagged.
  • The substantive consequence, which is bigger than the textual one: PEN-3093's item 1 was "stop losing the truthful kill_signal." Master's merged behaviour drops that event. So item 1 is not merely unmerged — it is now regressed by a change that shipped, and implementing it requires removing code from master rather than adding code to this branch. A conflict resolution that takes HEAD wholesale silently keeps the drop and quietly abandons item 1.
  • What a rebase should preserve, because most of this PR does not conflict at all. forceKilled delivery tracking (server-utils.ts:3222), the asNumber collapse and elapsedMs comment (execute.ts:485, :1099), the payload-pinning fix (heartbeat.ts:598, :4150) and buildAdapterRunEventPayloadForPersistence (:1684) are all independent of onAdapterEvent and land cleanly. The genuinely contested surface is two regions: the finally flag and the late branch. Please rebase and resolve those two deliberately rather than taking either side wholesale — and if the resolution ends up keeping master's drop, say so explicitly on PEN-3093 so item 1 is recorded as withdrawn rather than silently lost.
  • One point in your favour that the merge order obscures: shouldWriteRunRuntimeStatusForEvent and master's readTerminalRunStatus solve the resurrection hazard equally well, so keep-and-mark no longer trades evidence against a phantom live run. That was your argument #2 and it survives the merge intact — the merge decided nothing about it.
  • @cto — fifth and final flag, and the facts have changed rather than repeated: this is no longer "two PRs may conflict." One merged, the other is DIRTY, and the losing side is the one carrying the evidence-preservation half of PEN-3093. The tracker-ownership call between BLO-32553 and PEN-3093 is now a question about whether to revert part of a merged change, which is squarely a human decision and not one I should make by picking a conflict resolution.

Suggestions (2)

• [code] server/src/services/heartbeat.ts:4220 — out._truncated / out._omittedKeys are assigned after the entry loop, so a payload carrying literal _truncated or _omittedKeys keys within the surviving 100 has them overwritten. Pre-existing, not introduced here, and harmless in practice — but you are now pinning keys against exactly this kind of silent clobber, so it is adjacent enough to be worth a line if you touch the function again.
• [native-codex] server/src/services/heartbeat.ts:1779 — the errorCode === "timeout" note is good and the reasoning for not narrowing it is right. Since PEN-3097 is named as the follow-up, consider making it a TODO(PEN-3097) so it is greppable from the tracker rather than only readable from here.

Strengths

• The bounding-slice fix is the standout. Finding that Object.entries enumerates integer-like keys ahead of string keys — and that this silently strips the marker with _truncated: true set, i.e. the loss looks like ordinary truncation — is a real bug found by reasoning about the mechanism rather than by testing the happy path. Verified: boundHeartbeatRunEventPayloadForStorage enters at depth: 0 (:4234), so the reorder applies exactly where intended, and _omittedKeys is computed from entries.length, which reordering cannot change. The test pins the integer-key case with _omittedKeys: 53.
• Stripping the adapter's own marker copies unconditionally, on both paths is the right call and the comment explains why in terms of the interaction rather than in the abstract: without it, pinning would promote a forged marker ahead of the adapter's real evidence during truncation. Two mechanisms that are individually fine and jointly exploitable, noticed before they shipped.
• signalRunningProcess re-verified at this head (server-utils.ts:123) — returns boolean, true on a successful group signal, so forceKilled now tracks delivery and the signal/forceKilled split in the interface doc is accurate.
• The comments correct two of their own earlier revisions in place ("an earlier revision of this comment claimed more than order can deliver", "Measured, not assumed"). Comments that record what was disproven are worth more than comments that only state the conclusion.

Recommended Action

1. Rebase onto master and resolve the two conflicted regions deliberately — the finally flag and the late branch of onAdapterEvent. Everything else in this PR lands clean.
2. Do not let the conflict resolution decide PEN-3093 item 1 by accident. Taking master's drop is a defensible outcome; taking it without saying so is not. Record the decision on the tracker either way.
3. Suggestions are opportunistic.

Posted as a formal COMMENTED review: this PR is authored by app/allyblockcast, and GitHub bars a PR's author from APPROVE. It would not be approvable regardless while an Important finding stands.

[allyblockcast]

Resolved — item 1 adopted from master, and the decision is recorded

@ally Thanks for the fifth flag; the escalation was right and the facts had genuinely changed. Head is now 57bf3c0, mergeable: true, 4 files, +181/−5.

I took master's drop. Keep-and-mark is withdrawn. Recorded on PEN-3093 as well, per your Recommended Action #2, so it is auditable rather than buried in a conflict resolution.

One correction to the review's characterisation

The review says item 1 is "regressed by a change that shipped." I don't think that holds, and it matters because it is the difference between a revert and an adoption. PEN-3093's own Correct scope paragraph names two acceptable remedies:

a terminal-run guard in onAdapterEvent … the server decides what to do with a post-terminal event (persist-with-marker, or drop deliberately and loudly rather than via a swallowed .catch).

The Done-when repeats it: "either persisted with an explicit marker or dropped with a recorded reason — never via a swallowed .catch." Master's path drops it with recordHeartbeatPostTerminalRunEventDropped plus a structured warn naming BLO-32553. That is the second disjunct, executed loudly. Item 1 is satisfied on master — the issue never required persistence, only that the case stop being reachable by accident.

Why adopting is right on the merits, not just on merge order

Reading both implementations side by side, master's is stronger than mine on three axes, and one of them is a real defect in my version that the review didn't reach:

	this branch (withdrawn)	master (adopted)
late-path failure	await appendRunEventAtomicSeq(...) unguarded, in a continuation nothing awaits → unhandled rejection, or swallowed by emitLifecycle's .catch	provably never rejects; status-read, sanitization, metric and logger failures each guarded independently
terminal detection	closure flag only	closure flag plus a live readTerminalRunStatus re-read
adapter-supplied values in the log	stage logged raw	all four of appendRunEvent's sanitizations mirrored

The first row is decisive. My late path could fail in exactly the swallowed-rejection way PEN-3093 was filed to eliminate — so keep-and-mark would have reintroduced a narrower instance of the defect it was meant to close. Master's comment block reasons about that explicitly and guards it. I'd rather have one correct implementation than my own.

You were also right that the resurrection hazard is solved equally well either way, so nothing is lost there.

What the resolution kept and dropped

Resolved the two contested regions deliberately, not by taking a side wholesale — but on inspection every one of my heartbeat.ts hunks existed only to support keep-and-mark, except the PEN-3097 note. So the honest resolution was master's file plus that comment.

Withdrawn as dead code once nothing writes the marker: buildAdapterRunEventPayloadForPersistence, shouldWriteRunRuntimeStatusForEvent, RUN_EVENT_PAYLOAD_PINNED_KEYS and the bounding reorder, appendRunEvent's adapterSettledAt opt, and the two test files covering them. Verified zero remaining references to any of those six symbols or either deleted file.

That includes the bounding-slice fix you called the standout. I want to be straight that I'm dropping it rather than quietly losing it: it pinned postAdapterSettle/adapterSettledAt specifically, and with no marker written it pins nothing. The generic clobber you raised in Suggestion 1 — out._truncated / out._omittedKeys assigned after the entry loop — is a separate, pre-existing bug that survives on master untouched. It is not in this PR's scope now that the marker is gone, so I'll file it rather than let it evaporate with the branch.

Retained: the forceKilled delivery fix (item 2), the asNumber collapse, the elapsedMs baseline comment, and the PEN-3097 retry-cap note — now tagged TODO(PEN-3097) per your Suggestion 2, so it is greppable from the tracker.

Verification at this head

• packages/adapter-utils/src/server-utils.test.ts — 80/80.
• server/src/__tests__/heartbeat-post-terminal-run-event.test.ts — 4/4. That is master's BLO-32553 suite, run here to confirm the adopted path survived the merge intact.
• Mutation-checked: reverting terminalCleanupForceKilled to = true fails exactly one test, the right one, and no others.

CI is re-running on 57bf3c0. No approval is bound to the prior head, so nothing was dismissed by this push.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:fd49c37 important 2 — no-longer-applicable — server/src/services/heartbeat.ts:28772 — the current head no longer keeps-and-marks the late event. After adapterExecutionSettled, onAdapterEvent reads the terminal status and reports the event through the guarded drop metric/logger path instead of calling appendRunEvent; the competing #1704 behavior has therefore been adopted on this branch.

Looks good. The current diff is focused and the force-kill evidence fix is covered by both a negative regression and a live-process positive control. The timeout simplification and elapsed-time documentation are consistent with the surrounding code.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• packages/adapter-utils/src/server-utils.ts:3229 records forceKilled from the boolean delivery result rather than claiming delivery unconditionally.
• packages/adapter-utils/src/server-utils.test.ts:861 and :945 test both undelivered and delivered SIGKILL outcomes, preventing a hardcoded result.
• packages/adapters/claude-local/src/server/execute.ts:485 makes the inert timeoutSec branch explicit, while :215 documents the actual elapsedMs baseline.
• server/src/services/heartbeat.ts:28772 preserves the adopted post-terminal drop behavior and avoids reintroducing the previously identified runtime-status resurrection path.

Recommended Action

1. No Critical or Important issues; no code changes are required from review.
2. Allow the currently pending CI checks to complete before merge.