Conversation
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
… a router-ceded terminal (#11689 residual)
|
Validation pass after rebasing onto current
Reviewed the session marker lifecycle: router-ceded task/error terminals mark the current turn, teardown |
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
Problem
Since fd2e343 (#11689), every routed one-shot completion posts a spurious second message to the origin channel: the router posts the real completion, then seconds later swarm synthesis posts
<label> stopped.(or a sanitized chunk of the session's rawlastOutput) to the same room, threaded to the user's message.Mechanism, on current
develop:TASKS op=create→runPromptAndClose/runPromptViaSmithers,actions/tasks.ts) emitstask_completeon success, then in itsfinallyalways callsservice.stopSession→AcpService.closeSessionemits astoppedcarryingresponse = lastOutput(raw agent output), plus a second explicitstopped. This sequence is deterministic — it happens on every routed success, not as a rare race.task_completeskip inSwarmCoordinatorService.runSwarmCompletedeliberately returns without claiming thesynthesizedCompletionSessionsdedupe slot (so a later genuine stop on a reused session isn't swallowed), andstoppedis deliberately not router-owned (shouldInjectinsub-agent-router.tsexcludes it).stoppedfalls through every guard and synthesizes.server-helpers-swarm.tsposts it unconditionally (swarm_synthesisis EPHEMERAL, so the roomId+text delivery dedupe is bypassed) tooriginRoomIdwithreplyToExternalMessageId— i.e. the user's Discord channel.The same mechanism leaks a terminal
stoppedmid-respawn on error paths the router deliberately suppresses (account failover / state-lost):tasks.tsstops the session before the router stampshandedOffToSuccessorSessionId, so the #11720/#11721 handoff skips can't catch it (they help cache staleness, not a not-yet-stamped store).Pre-#11689 the top-of-function dedupe claim made these teardown stops invisible; the commit converted the #11634 duplicate-completion into a spurious stop-post instead of eliminating the double post. Tests at
swarm-coordinator-service.test.tspinned the wrong behavior as expected (modeled as a rare user-stop race, but the one-shot runners produce the identical sequence deterministically on every routed success).Fix
Track the cession in memory: when the router-owned skip cedes a
task_complete/error, record the session in a newrouterCededTerminalSessionsset. A subsequentstoppedon a ceded session is recognized as teardown plumbing of that same turn and skipped — without claiming the synthesis dedupe slot.The marker is strictly turn-scoped, so nothing goes silent:
stop(): bounded memory; an idle-session stop after the grace window behaves exactly as before.stopped) — user sees 3 completions for 1 task #11711 store re-read, so it also covers the suppressed-failover leak where teardown lands before the router stamps the handoff marker. The store-backed handoff skips are kept — they cover a restarted coordinator / resumed-then-handed-off session where the in-memory marker is gone.Per-session terminal serialization (
maybeFireSwarmComplete) guarantees a same-tick teardownstoppedobserves the cession recorded by the terminal that preceded it.Tests
Reproduced first (all three fail on unfixed
develop, with the exact spurious payloadcompletionSummary: "build-site stopped."targeted at the origin room):does NOT synthesize the teardown stopped that follows a router-owned task_complete (#11689 residual)— the deterministic one-shot success sequence (task_complete→ closeSessionstoppedw/ raw lastOutput → explicitstopped) produces zero synthesis.does NOT leak a teardown stopped after a router-owned error the router suppresses (failover/state-lost)— no terminal<label> stopped.mid-respawn.suppresses the stopped racing a router-owned terminal on the same session (same-tick teardown)— rewrites the old race test that pinned the wrong behavior.a stopped AFTER the session resumes still synthesizes (cession is per-turn; sessions are reused)— preserves the reuse guarantee the old test at :842 guarded.stopped) — user sees 3 completions for 1 task #11711 tests now warm the enrichment cache via a non-terminal event so they keep exercising the store-backed handoff re-read layer (previously warmed via a router-ownedtask_complete, which now also sets the in-memory marker).Unchanged and still green: genuine user
stoppedwith no preceding terminal posts; router-inactive/no-origin sessions still synthesize; custom-validator completions still post.Verification
Refs #11689 (fd2e343), #11634, #11711, #11720, #11721.