[lilyshen0722]

What

A native turn that dies with its process is now re-run instead of being parked where nothing polls it (TASK-264).

Native turns (Scout today, hosted agents after #2224) run in-process. When the backend restarts mid-turn, on a deploy or a crash:

• the event stays delivered with no ack;
• the 10-minute sweep requeued it to pending, but native agents don't poll, so the person never got an answer and the event sat until the 168 h delete;
• the turn's AgentRun stayed running forever. interrupted was in the enum, but nothing wrote it.

How

• Shared helpers. resolveInProcessInstallation and dispatchInProcess are extracted from enqueue unchanged, so enqueue and the sweep make one routing decision and use one settle path.
• New sweep pass. Before the requeue, garbageCollect runs redispatchStuckInProcess:
  • a stuck delivered event whose installation runs in-process is claimed by swapping its delivery nonce;
  • two sweeps can't both run it: two replicas, or a sweep racing a late settle;
  • the dead turn's late settle can't end the new delivery, because it holds the old nonce;
  • the claim counts as a delivery, so the existing 3-attempt cap and the expiry pass still bound a poison event;
  • if the turn's AgentRun already succeeded (only the ack was lost), the event is acked, not answered twice;
  • the dead turn's queued/running run is marked interrupted.
• Abandoned runs. interruptAbandonedRuns marks any run still queued/running 30 minutes after start as interrupted. A native turn is bounded at about two minutes. The daily caps count runs by start time, so this changes no budget.
• Failure isolation. The pass has its own try, so if it fails, the existing requeue still runs.

An event whose agent no longer runs in-process falls through to the requeue exactly as before.

Not covered

If a turn posts its reply and the process dies before the ack, with its AgentRun not yet marked succeeded, the re-run can answer a second time. Posting is the turn's last step and the ack follows immediately, so the window is milliseconds; an extra reply is the cheaper error than no reply. Re-running pending events left by a Durable Object → native migration is TASK-265's job.

Tests

• agentEventService.deliveryNonce, on in-memory Mongo:
  • re-run under a fresh nonce, with the dead run unable to end it;
  • two racing sweeps re-run once;
  • an already-succeeded run is acked, not re-run;
  • an event at the cap goes to the expiry pass;
  • an abandoned run is marked interrupted, while live and finished runs are untouched.
• The existing "outlives the requeue" arm now models an agent that is no longer in-process, which is the case the requeue still serves.
• Mutations, each caught: no re-dispatch; no succeeded-run check; a claim that ignores the nonce.
• Wider run: 87 related suites (event delivery, the sweep, the scheduler, the native runtime, AgentRun) pass, 1,000 tests. tsc and lint are 0 on agentEventService.ts. The test file's lint findings match main's existing ones; the .js test corpus isn't gated.

🤖 Generated with Claude Code

https://claude.ai/code/session_015NDyNbwmCco62PAAviSL1k

[lilyshen0722]

Written by Sprint Review, a Commonly agent — gate requested in pod thread 77851.

Bound to 51f84269be15cb22020517897b7d50049b160bad. gh pr view and refs/pull/2226/head agree. One commit, 2 files, +350/−99.

BASE: 29/29 on agentEventService.deliveryNonce.test.js, 66/66 across all four agentEvent* suites. Worth noting the tier — this suite runs against mongodb-memory-server, so the CAS below is pinned against a real mongod rather than a mock. For a claim filter that is the whole safety argument, that is the right tier and it is the reason I can report the next table as behaviour rather than shape.

Every property you asked me to focus on is pinned

#	mutation	result
1	claim filter drops the nonce CAS	1 failed — two sweeps racing (two replicas) re-run it once
2	claim reuses the old nonce instead of minting	3 failed — incl. the dead run cannot end the new delivery
3	always re-run, never ack a finished run	1 failed — acks instead of answering twice …
4	find ignores the attempt cap	1 failed — leaves an event at the attempt cap …
5	claim no longer counts as a delivery ($inc removed)	1 failed
6	abandoned-run interrupt made a no-op	1 failed

BASE restored after each. So: one run across replicas, the dead turn's late settle cannot end the new delivery, the attempt cap still bounds a poison event, and an already-succeeded run is acked rather than answered twice — all four measured, not read.

Two details I checked because they are the usual way a filter like this fails quietly:

• deliveryNonce: stale.deliveryNonce ?? null is doing real work. For a legacy event with no nonce the predicate becomes {deliveryNonce: null}, which in Mongo matches absent or null — so the CAS still holds for pre-nonce rows, and once a winner writes a hex nonce the loser's predicate no longer matches. The find is .lean() with no projection, so there is no missing-field path that would make the predicate read undefined.
• The run lookup keys on the event's agentName, while runs are stored lowercased by nativeRuntimeService. I went looking for a case mismatch that would make a succeeded run invisible and double-answer the person; AgentEvent's schema declares agentName: { lowercase: true }, so stored event names are already lowercase and the keys agree. Not a bug — but it is the one place where this would have been silent.

The extraction is provably behaviour-preserving

Most of the +350/−99 is moving enqueue's inline routing and dispatch into resolveInProcessInstallation and dispatchInProcess so the sweep shares one decision. I compared the moved regions textually, normalised for indentation:

installation lookup block   6 lines   IDENTICAL
sibling-inheritance block  21 lines   IDENTICAL
dispatch block             51 lines   IDENTICAL

That matters because two of the moved predicates turn out to be unpinned: inverting return installationRuntimeType === 'native' ? installationDoc : null to return every active install leaves all 66 green, and disabling the sibling-runtime inheritance at runtime also leaves all 66 green. Neither is a regression — the code is byte-identical to main — but the extraction made that one-line runtime gate load-bearing for a second caller, so it is worth an arm. If it ever inverted, an external poller's event would be dispatched in-process, runAgent would return a resolved failedResult, and dispatchInProcess's success handler would ack it — the event parked where nothing polls, which is the exact failure class this PR exists to remove.

(One methodology note, since it bit me: my first attempt at the inheritance mutation used if (false), which failed all 29 tests at once. That is a compile/collection error, not discrimination — I redid it with a runtime-false condition to get a clean read.)

CodeQL is red, and it is red about code this PR did not change

CodeQL fails at this head: 1 new alert including 1 high severity security vulnerability — js/sql-injection, "Database query built from user-controlled sources", at agentEventService.ts:1144. That line is the AgentInstallation.findOne inside the new helper, i.e. the 6-line block I verified above as IDENTICAL to main. The alert is new only in the sense that the lines are new to the diff; the taint path is exactly as old as main.

I then checked whether it is a real reachable injection rather than assuming the relocation settles it. Tracing the most directly request-fed caller (routes/dev.ts:112, which passes a body agentName and podId): agentName reaches the query through .toLowerCase(), which throws on a non-string before any query is built, and podId has already survived a Pod.findById cast plus an isMember check on the same request. So on that path an operator object cannot land in the filter. I traced that one caller, not all eleven — I am not claiming the others are proven.

Worth knowing: CodeQL is not a required check. Branch protection on main requires exactly one context, Test & Coverage, and no review. So this does not mechanically block the merge, and Analyze (javascript-typescript) itself succeeded — the red row is the alert gate, not a broken analysis.

My recommendation is not to hold the PR for it, but not to let it pass silently either: dismiss the alert with "relocated, pre-existing" recorded as the reason, or add a string/ObjectId guard at the helper boundary, which would close a real (currently unexploitable) operator-injection shape and take the gate green. A red CodeQL that everyone learns to wave through is worse than either.

Smaller notes

• dispatchInProcess acks on resolution, ignoring result.status, so a native run that returns a resolved failedResult — a config refusal, or a daily-cap decline — is acked as native-runtime-completed rather than recorded as a failure. Pre-existing and textually unchanged (it is inside the 51 identical lines), so not this PR's to fix; flagged because the sweep now drives that same path on a second trigger.
• interruptAbandonedRuns is unscoped by agent or pod, which is safe today: AgentRun.create has exactly one caller (nativeRuntimeService.ts:895), interrupted is already in the schema enum, startedAt defaults to Date.now so no row is missed by the $lt, and {status, startedAt} is indexed. If a second runtime ever writes AgentRun rows with turns longer than 30 minutes, this sweep would mis-mark them.
• #2224 and #2226 do conflict at the routing line — git merge-tree on the pair reports conflict markers, so the rebase you planned for whichever lands second is genuinely needed, not just prudent.

eslint on the changed file: 0 errors, 8 warnings. tsc --noEmit: 50 errors, identical to the baseline count, none in agentEventService.ts. CI otherwise: 13 pass, 3 skipping.

GATE: PASS

[lilyshen0722]

Written by Sprint Review, a Commonly agent — re-gate requested in pod thread 77876.

Bound to 14b86ea74849b77fcb1f2db64f78e18634d79e12. My PASS 5535410951 (at 51f84269) is superseded. One commit on top, 3 files, +43/−15.

CodeQL, which is what I was asked to check

Green at this head, read off the check run rather than inferred:

CodeQL:                          success — No new alerts in code changed by this pull request
Analyze (javascript-typescript): success
Analyze (python) / (actions):    success

Worth saying plainly: I reported that alert as relocated pre-existing code and recommended either a dismissal or a boundary guard. A guard was the better of the two options I offered, and for a reason I undersold — a dismissal would have left the actual taint path on main (the same findOne, alert 1556) annotated rather than closed. This fixes it everywhere instead of explaining it once.

The fix is pinned — but it took four mutations to show it

The first three I ran all came back green, and a faster reviewer would have filed "the security arm is vacuous." It is not:

mutation	result	why
sanitization block removed	67 passed	new Types.ObjectId('[object Object]') throws, so the lookup never runs
hex test accepts anything	67 passed	same — the ObjectId wrap still throws
raw podKey passed instead of the ObjectId wrap	67 passed	the hex test still rejects it
both removed — the exact pre-fix shape	1 failed — a podId shaped like a query operator is never looked up or run in-process	nothing left to block it

The hex test and the new Types.ObjectId() wrap are each independently sufficient, so each single mutation leaves the other standing and they mask one another. Individually-green mutations on redundant guards are not evidence of a weak test — only the combined mutation answers the question, and it reds. The arm itself is well built: it mocks findOne to return a native install, so a lookup that happened would route in-process and call runAgent, and then asserts neither occurred.

One genuinely unpinned piece: dropping typeof agentName !== 'string' || typeof instanceId !== 'string' and keeping only the hex test leaves 67 green — the hex test alone stops the operator-shaped podId. Low impact, since a non-string agentName previously threw on .toLowerCase() inside enqueue's try/catch and fell back to the external queue, which is the same outcome return null now produces. But nothing covers a non-string agent or instance value.

The regression this change could have caused, and why it doesn't

The new failure mode is return null, which means "not in-process" and sends the event to the pending queue that no native agent polls — precisely the bug this PR exists to fix. So a legitimate caller that fails validation would silently reintroduce it. I checked the call sites rather than assuming: the internal retry passes event.podId, admin.ts passes installation.podId, the rest pass a route-level podId already cast by an upstream Pod.findById. All are ObjectIds or 24-hex strings, whose String() passes the test. None passes a pod document, which is the shape that would stringify to [object Object] and fail.

It also closes a quieter hole in the same line: podId: undefined used to have Mongoose strip the key entirely, so the lookup matched any pod's install for that agent. That now returns null.

Regression sweep

BASE 67/67 across the four agentEvent* suites. Broader run over agentEvent|native|hosted|registry|agentState|agentInstallationCleanup|tasksApi|messageClaim: 91 suites, 832 passed, 2 skipped, 0 failed (my own pattern and my own counts, not a re-report of the numbers in the ask). eslint on the changed file: 0 errors, 8 warnings. tsc --noEmit: 50, the baseline. CI: 14 pass, 2 skipping — nothing pending or failing at this head.

Everything from my first review stands unchanged, including that the extracted blocks remain textually identical to main and that the === 'native' routing gate is still unpinned.

GATE: PASS