fix(agents): restore subagent announce chain from #22223#23166
fix(agents): restore subagent announce chain from #22223#23166
Conversation
…s all completions through agent path so parent LLM processes results. Third time fixing this regression.
| export type SubagentRunRecord = { | ||
| runId: string; | ||
| childSessionKey: string; | ||
| requesterSessionKey: string; | ||
| requesterOrigin?: DeliveryContext; | ||
| requesterDisplayKey: string; | ||
| task: string; | ||
| cleanup: "delete" | "keep"; | ||
| label?: string; | ||
| model?: string; | ||
| runTimeoutSeconds?: number; | ||
| createdAt: number; | ||
| startedAt?: number; | ||
| endedAt?: number; | ||
| outcome?: SubagentRunOutcome; | ||
| archiveAtMs?: number; | ||
| cleanupCompletedAt?: number; | ||
| cleanupHandled?: boolean; | ||
| suppressAnnounceReason?: "steer-restart" | "killed"; | ||
| expectsCompletionMessage?: boolean; | ||
| /** Number of times announce delivery has been attempted and returned false (deferred). */ | ||
| announceRetryCount?: number; | ||
| /** Timestamp of the last announce retry attempt (for backoff). */ | ||
| lastAnnounceRetryAt?: number; | ||
| }; |
There was a problem hiding this comment.
Duplicate SubagentRunRecord type diverges from store module
This inline SubagentRunRecord type no longer includes spawnMode, endedReason, or endedHookEmittedAt, but subagent-registry.store.ts still imports SubagentRunRecord from subagent-registry.types.ts (which retains all three fields) and explicitly sets spawnMode on line 104 of subagent-registry.store.ts. This means:
- Records loaded from disk via
loadSubagentRegistryFromDisk()will havespawnModeat runtime, but this type won't expose it. - The two
SubagentRunRecordtypes are now out of sync — the.types.tsversion is a superset.
This won't cause a runtime error (TypeScript structural typing allows it), but the .types.ts file and the store module should be updated to match, or this file should import the type from .types.ts and simply not use the removed fields. Otherwise future maintainers may be confused by the divergence.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/subagent-registry.ts
Line: 14-38
Comment:
**Duplicate `SubagentRunRecord` type diverges from store module**
This inline `SubagentRunRecord` type no longer includes `spawnMode`, `endedReason`, or `endedHookEmittedAt`, but `subagent-registry.store.ts` still imports `SubagentRunRecord` from `subagent-registry.types.ts` (which retains all three fields) and explicitly sets `spawnMode` on line 104 of `subagent-registry.store.ts`. This means:
1. Records loaded from disk via `loadSubagentRegistryFromDisk()` will have `spawnMode` at runtime, but this type won't expose it.
2. The two `SubagentRunRecord` types are now out of sync — the `.types.ts` version is a superset.
This won't cause a runtime error (TypeScript structural typing allows it), but the `.types.ts` file and the store module should be updated to match, or this file should import the type from `.types.ts` and simply not use the removed fields. Otherwise future maintainers may be confused by the divergence.
How can I resolve this? If you propose a fix, please make it concise.| try { | ||
| await callGateway({ | ||
| method: "sessions.delete", | ||
| params: { | ||
| key: entry.childSessionKey, | ||
| deleteTranscript: true, | ||
| emitLifecycleHooks: false, | ||
| }, | ||
| params: { key: entry.childSessionKey, deleteTranscript: true }, |
There was a problem hiding this comment.
Missing emitLifecycleHooks: false on sweeper delete
The previous version explicitly passed emitLifecycleHooks: false here to prevent lifecycle hooks from firing during background archive sweeps. By omitting this parameter, the gateway defaults to emitLifecycleHooks: true (see sessions.ts:473), meaning lifecycle hooks (including subagent_ended) will now fire for every archived session deletion. If plugins have registered subagent_ended hooks, this could trigger unexpected side-effects (e.g., farewell messages or thread unbinding) for sessions that were already cleaned up.
Was this change intentional? If so, please note it in the PR description.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/subagent-registry.ts
Line: 254-257
Comment:
**Missing `emitLifecycleHooks: false` on sweeper delete**
The previous version explicitly passed `emitLifecycleHooks: false` here to prevent lifecycle hooks from firing during background archive sweeps. By omitting this parameter, the gateway defaults to `emitLifecycleHooks: true` (see `sessions.ts:473`), meaning lifecycle hooks (including `subagent_ended`) will now fire for every archived session deletion. If plugins have registered `subagent_ended` hooks, this could trigger unexpected side-effects (e.g., farewell messages or thread unbinding) for sessions that were already cleaned up.
Was this change intentional? If so, please note it in the PR description.
How can I resolve this? If you propose a fix, please make it concise.| await callGateway({ | ||
| method: "sessions.delete", | ||
| params: { | ||
| key: params.childSessionKey, | ||
| deleteTranscript: true, | ||
| emitLifecycleHooks: false, | ||
| }, | ||
| params: { key: params.childSessionKey, deleteTranscript: true }, | ||
| timeoutMs: 10_000, |
There was a problem hiding this comment.
Missing emitLifecycleHooks: false on post-announce session delete
Same issue as in the sweeper: the previous version explicitly suppressed lifecycle hooks when deleting the child session after announcing results. Omitting emitLifecycleHooks: false means the gateway will now emit subagent_ended hooks during this cleanup delete, which may cause duplicate farewell messages or unexpected thread unbinding if subagent_ended hooks are registered by channel plugins.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/subagent-announce.ts
Line: 737-740
Comment:
**Missing `emitLifecycleHooks: false` on post-announce session delete**
Same issue as in the sweeper: the previous version explicitly suppressed lifecycle hooks when deleting the child session after announcing results. Omitting `emitLifecycleHooks: false` means the gateway will now emit `subagent_ended` hooks during this cleanup delete, which may cause duplicate farewell messages or unexpected thread unbinding if `subagent_ended` hooks are registered by channel plugins.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb9b0a5b46
ℹ️ 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".
| const archiveAfterMs = resolveArchiveAfterMs(cfg); | ||
| const spawnMode = params.spawnMode === "session" ? "session" : "run"; | ||
| const archiveAtMs = | ||
| spawnMode === "session" ? undefined : archiveAfterMs ? now + archiveAfterMs : undefined; | ||
| const archiveAtMs = archiveAfterMs ? now + archiveAfterMs : undefined; |
There was a problem hiding this comment.
Skip archive scheduling for persistent session-mode runs
registerSubagentRun now always sets archiveAtMs, so persistent mode="session" subagents are treated like one-shot runs. Those entries are later removed by sweepSubagentRuns (which calls sessions.delete), so thread-bound sessions that are documented/created as persistent will be auto-deleted after archiveAfterMinutes (default 60), breaking follow-up conversations in the bound thread.
Useful? React with 👍 / 👎.
| entry.cleanupHandled = true; | ||
| entry.cleanupCompletedAt = now; | ||
| entry.suppressAnnounceReason = "killed"; |
There was a problem hiding this comment.
Emit ended hooks when marking runs killed
markSubagentRunTerminated now marks killed runs as fully cleaned up (cleanupHandled + cleanupCompletedAt) and only persists state, but no longer emits subagent_ended. Since /kill and abort flows use this function, killed subagents skip immediate lifecycle cleanup (for example thread-unbind/farewell hooks), leaving stale routing/binding state after termination.
Useful? React with 👍 / 👎.
|
Thanks for the deep dive here. Closing as superseded by newer targeted announce-path fixes. This PR is a broad rollback to a historical snapshot and now diverges heavily from current announce/registry behavior. We should keep forward fixes small and focused (e.g. channel deliverability/routing and pairing-scope handling) rather than restoring an older full-file state. |
|
Is this fixed now? @tyler6204 I'm still on an old OpenClaw version because of this issue and I don't know if its safe to update now or not. Would appreciate any insight into this |
Summary
Restores the exact subagent announce logic from PR #22223 (commit fe57bea) which was overwritten by subsequent commits f555835 and 8178ea4.
Problem
Two commits landed after #22223 merged and rewrote
subagent-announce.ts, re-introducing the brokensendpath that bypasses the parent LLM:This caused:
Fix
Restored
src/agents/subagent-announce.tsandsrc/agents/subagent-registry.tsto exactly match the working state from fe57bea. Updated callers/tests to match the restored interface.All subagent completions route through
method: "agent"only. Nosendpath.Testing verified
Related
Greptile Summary
This PR reverts
subagent-announce.tsandsubagent-registry.tsto the working state from PR #22223 (commitfe57bea08), undoing changes introduced by two subsequent commits that re-introduced a brokensenddelivery path bypassing the parent LLM.Key changes:
method: "send"direct delivery path fromsubagent-announce.ts, ensuring all subagent completions route throughmethod: "agent"onlyspawnModetracking fromSubagentRunRecordin the registry (session-mode vs run-mode distinction for announce routing)subagent_delivery_targethook integration andresolveSubagentCompletionOriginfrom the announce flowemitSubagentEndedHookOnce,completeSubagentRun) from the registry — thesubagent_endedhook is no longer explicitly emitted from the registry modulesubagent-registry-cleanup.ts,subagent-registry-completion.ts,subagent-registry-queries.ts, andsubagent-registry-state.tsisDeliverableMessageChanneltoisInternalMessageChannel(semantically equivalent)Notable side-effects:
emitLifecycleHooks: falsewas removed from twosessions.deletecalls (sweeper and post-announce cleanup), which changes behavior: lifecycle hooks will now fire during these deletionsSubagentRunRecordtype insubagent-registry.tsnow diverges from the version insubagent-registry.types.ts(which still includesspawnMode,endedReason,endedHookEmittedAt) — the store module still references the old type and setsspawnModeon disk-loaded recordsConfidence Score: 3/5
sendpath, routing all completions throughmethod: "agent") is well-motivated and addresses a real regression. However, the removal ofemitLifecycleHooks: falsefrom twosessions.deletecalls changes behavior in a way that could cause duplicate farewell messages or unexpected hook side-effects for channel plugins. Additionally, theSubagentRunRecordtype now diverges betweensubagent-registry.tsandsubagent-registry.types.ts/subagent-registry.store.ts, creating maintenance risk. The extracted helper modules (subagent-registry-cleanup.ts,subagent-registry-completion.ts, etc.) are left orphaned without being deleted.src/agents/subagent-registry.ts(lifecycle hook behavior change in sweeper) andsrc/agents/subagent-announce.ts(lifecycle hook behavior change in post-announce cleanup)Last reviewed commit: cb9b0a5