fix(sessions): skip A2A flow for persistent sessions#67278
Conversation
Greptile SummaryThis PR adds a guard in Confidence Score: 5/5Safe to merge — the fix is minimal, logically sound, and leaves the spawnMode === 'run' / unregistered-session paths completely unaffected. The change is two lines that gate on a well-typed, optional registry field. The early return is void-compatible, the registry lookup key matches the existing A2A flow argument, and null/missing entries fall through to the original behavior. No P0/P1 issues identified. No files require special attention. Reviews (1): Last reviewed commit: "fix(sessions): skip A2A flow for persist..." | Re-trigger Greptile |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f95fceeaa9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Persistent sessions (spawnMode=session) accumulate internal A2A messages (ping-pong rounds + announce step) as permanent User messages in their transcript. These polluted messages are replayed on session resume, causing the agent to process stale internal directives and get stuck. Skip the entire A2A flow when the target session is persistent, since multi-round cross-review workflows (/X) are orchestrated by the main agent's explicit sessions_send calls, not by A2A ping-pong. Co-Authored-By: Claude Opus 4.6 <[email protected]>
c4185ef to
d0db44f
Compare
…trap test The vi.fn(() => undefined) inferred as Mock<() => undefined> which is incompatible with the declared type () => string | undefined. Co-Authored-By: Claude Opus 4.6 <[email protected]>
# Conflicts: # extensions/matrix/src/matrix/sdk/crypto-bootstrap.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3baf5d125c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The early return for persistent sessions skipped the A2A announce flow but still returned delivery status "pending", misleading orchestration callers into waiting for a completion signal that would never arrive. Hoist the spawnMode check before delivery construction so the returned status is "skipped" when A2A is not attempted. Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c38de15b5c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…A2A skip The skipA2A check in sessions-send-tool relies on getLatestSubagentRunByChildSessionKey() to detect persistent child sessions. The TTL sweep was removing session-mode records 5 minutes after cleanupCompletedAt, silently flipping skipA2A back to false and reintroducing transcript pollution for long-lived sessions. Session-mode records should outlive the per-round TTL: the child session is still alive, consistent with shouldKeepThreadBindingAfterRun which keeps the thread binding for session-mode + non-KILLED runs. Only sweep once the session is truly terminated (KILLED/ERROR). Co-Authored-By: Claude Opus 4.7 <[email protected]>
CI oxlint flagged the single-line `if (skipA2A) return;` as violating the `curly` rule. Co-Authored-By: Claude Opus 4.7 <[email protected]>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9fb82275a7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…reset Without this, a persistent child that has completed its initial round (endedReason=complete) was kept alive in the registry by the sweep policy forever: sessions.delete and sessions.reset do not flip the endedReason to killed (markSubagentRunTerminated skips entries with endedAt set), so the record would leak indefinitely and could later misclassify an unrelated sessions_send on the same childSessionKey as session-mode. cleanupSessionBeforeMutation is the common path for both delete and reset, so wire the purge there. The purge is scoped to spawnMode= "session" to avoid racing with in-flight run-mode entries. Co-Authored-By: Claude Opus 4.7 <[email protected]>
openclaw-barnacle
Bot
added
gateway
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa004ef7ab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Reset keeps the session key and only rotates sessionId/transcript, so the spawnMode="session" marker that sessions_send uses for skipA2A must stay in place. Removing it on reset forced later sends on the same key back into the A2A flow, reintroducing the persistent transcript pollution this PR is trying to prevent. Only delete, which retires the key entirely, now triggers the purge. Co-Authored-By: Claude Opus 4.7 <[email protected]>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c8aa8cdba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
CI note:
|
|
Tracking issue for the main-branch test failure: #67965. |
…endedReason killSubagentRun marks the run terminated on KILLED/ERROR but does not delete the session entry, so the child session key remains valid for sessions.send. Treating KILLED/ERROR as "session terminated" in the sweep guard would drop the skipA2A marker while the child is still usable, regressing A2A pollution. Gate session-mode rows out of the per-round TTL entirely; their sole removal path is purgeSubagentRunsForSessionKey on session-delete.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2bff02b1b1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
purgeSubagentRunsForSessionKey only scanned in-memory subagentRuns, but read paths (getSubagentRunsSnapshotForRead) merge disk state so other worker processes can observe runs. A session-mode row created by another worker would stay on disk after session-delete, and because sweepSubagentRuns now skips all spawnMode:"session" entries, it would linger indefinitely and risk skipA2A misclassification. Restore from disk with mergeOnly before the purge so cross-worker rows are visible, then persist the reduced set to drop disk-only residue.
|
Hit this in production — adding a real-world reproduction to support the merge. Environment
What happenedThe main agent held a persistent session The Gemini CLI adapter then processed
On the following turn, when asked "how many rounds have we had?", the adapter retrieved the polluted history and reported:
i.e. Confirming this PR is not yet in 2026.4.15Verified against the installed dist bundle: Impact
Would be valuable to get this reviewed and merged; happy to provide additional trace data if needed. |
Match the skipA2A invariant at the cleanup announce path: subagent
natural completion routes a completion event through
runSubagentAnnounceFlow, which is delivered into the persistent session
as an inbound user message and triggers a stray model reply
("Background task <runId>...") that bubbles back to the main session
via the announce queue. Short-circuit startSubagentAnnounceCleanupFlow
on spawnMode === "session" so persistent sessions receive no internal
A2A/announce traffic from any path.
Co-Authored-By: Claude Opus 4.7 <[email protected]>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7f3a0706c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // triggers a stray model reply ("Background task <runId>...") in the main | ||
| // session via the announce queue. | ||
| if (entry.spawnMode === "session") { | ||
| void finalizeSubagentCleanup(runId, entry.cleanup, false).catch((err) => { |
There was a problem hiding this comment.
When spawnMode === "session", this new branch intentionally skips runSubagentAnnounceFlow but still calls finalizeSubagentCleanup(..., false). In finalizeSubagentCleanup, didAnnounce=false is handled as announce failure/retry, which schedules retry timers and ultimately marks the task delivery as failed via safeSetSubagentTaskDeliveryStatus even though the subagent run completed successfully. That makes persistent-session completions look like delivery failures in background-task tracking and produces unnecessary retry/give-up churn for every successful session-mode run.
Useful? React with 👍 / 👎.
1 participant
Summary
spawnMode=session) accumulate internal A2A messages (ping-pong rounds + announce step) as permanent User messages in their transcriptspawnModefrom the subagent registry — covering both thesessions_sendentry path and the subagent cleanup announce pathRoot Cause
runSessionsSendA2AFlowwrites all ping-pong and announce messages as permanent User messages into the target session's store. For oneshot sessions this is harmless (session is discarded after use), but for persistent sessions these messages accumulate and are replayed on every resume viasession/load.Additionally, subagent natural completion (not user-triggered
sessions_send) routes a completion event throughrunSubagentAnnounceFlowduring cleanup. This second path is short-circuited atstartSubagentAnnounceCleanupFlowto enforce the same invariant — without it, a completion event is delivered into the persistent session as an inbound user message and triggers a stray model reply ("Background task ...") that bubbles back to the main session via the announce queue.Why A2A skip is safe for persistent sessions
Multi-round cross-review workflows are orchestrated by the main agent's explicit
sessions_sendcalls, not by A2A ping-pong. Skipping A2A does not affect any existing workflow.Test plan
🤖 Generated with Claude Code