Fix #66535: wire up abort signal for /compact command#66623
Fix #66535: wire up abort signal for /compact command#66623mushuiyu886 wants to merge 3 commits into
Conversation
…provided When --profile is explicitly passed, applyCliProfileEnv() now clears OPENCLAW_LAUNCHD_LABEL only when it conflicts with the profile-derived label. This allows resolveLaunchAgentLabel() to derive the correct label from OPENCLAW_PROFILE instead of short-circuiting on a stale inherited value from a parent gateway process. Matching labels are preserved to respect explicit operator overrides. Fixes openclaw#65643
…EMBEDDED_RUNS The /compact command invoked compactEmbeddedPiSession without passing an abortSignal and without registering the compaction session into ACTIVE_EMBEDDED_RUNS. As a result, chat.abort, abortEmbeddedPiRun, and /stop had no effect on an in-progress compaction — the user had to wait until compactionTimeoutMs expired. Changes: - Check that the previous embedded run fully drained before registering the compaction handle, returning a retry prompt if it has not - Create an AbortController in the /compact command handler and pass its signal via the already-defined abortSignal field on CompactEmbeddedPiSessionParams - Build a minimal EmbeddedPiQueueHandle for the compaction and register it with setActiveEmbeddedRun before starting - Clean up with clearActiveEmbeddedRun in a finally block to ensure the handle is always removed - Export setActiveEmbeddedRun, clearActiveEmbeddedRun, and EmbeddedPiQueueHandle from pi-embedded-runner.ts and pi-embedded.ts Fixes openclaw#66535
Greptile SummaryThis PR wires an The Confidence Score: 2/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/auto-reply/reply/commands-compact.ts
Line: 2
Comment:
**Missing export: `EmbeddedPiQueueHandle` not exported from `pi-embedded.ts`**
`EmbeddedPiQueueHandle` is defined in `src/agents/pi-embedded-runner/runs.ts` but is never re-exported through `pi-embedded-runner.ts` → `pi-embedded.ts`. This import will fail at compile time with `Module '"../../agents/pi-embedded.js"' has no exported member 'EmbeddedPiQueueHandle'`.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/auto-reply/reply/commands-compact.ts
Line: 132-176
Comment:
**Missing runtime exports: `setActiveEmbeddedRun` / `clearActiveEmbeddedRun`**
`commands-compact.runtime.ts` does not export `setActiveEmbeddedRun` or `clearActiveEmbeddedRun`. Both calls — `runtime.setActiveEmbeddedRun(...)` on line 132 and `runtime.clearActiveEmbeddedRun(...)` on line 176 — will fail at compile time and at runtime. The PR description says these re-exports were added to `commands-compact.runtime.ts`, `pi-embedded-runner.ts`, and `pi-embedded.ts`, but none of those changes appear in the diff.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/auto-reply/reply/commands-compact.ts
Line: 134-180
Comment:
**`result` is possibly `undefined` after try/finally**
`let result` is declared without an initializer. TypeScript will type it as `CompactResult | undefined`, so the immediate `result.ok` and `result.reason` accesses on lines 180+ will produce "Object is possibly undefined" type errors. If `compactEmbeddedPiSession` rejects (e.g. on abort), the exception propagates past the `finally` so `result` is never read — but TypeScript cannot prove that statically.
A simple fix is to lift the result assignment above the `try` block and keep the `finally` only for cleanup, or use a definite-assignment assertion if the type is otherwise guaranteed:
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(compact): wire up abort signal and r..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -1,4 +1,5 @@ | |||
| import { resolveAgentDir, resolveSessionAgentId } from "../../agents/agent-scope.js"; | |||
| import type { EmbeddedPiQueueHandle } from "../../agents/pi-embedded.js"; | |||
There was a problem hiding this comment.
EmbeddedPiQueueHandle not exported from pi-embedded.ts
EmbeddedPiQueueHandle is defined in src/agents/pi-embedded-runner/runs.ts but is never re-exported through pi-embedded-runner.ts → pi-embedded.ts. This import will fail at compile time with Module '"../../agents/pi-embedded.js"' has no exported member 'EmbeddedPiQueueHandle'.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/auto-reply/reply/commands-compact.ts
Line: 2
Comment:
**Missing export: `EmbeddedPiQueueHandle` not exported from `pi-embedded.ts`**
`EmbeddedPiQueueHandle` is defined in `src/agents/pi-embedded-runner/runs.ts` but is never re-exported through `pi-embedded-runner.ts` → `pi-embedded.ts`. This import will fail at compile time with `Module '"../../agents/pi-embedded.js"' has no exported member 'EmbeddedPiQueueHandle'`.
How can I resolve this? If you propose a fix, please make it concise.| runtime.setActiveEmbeddedRun(sessionId, compactHandle, params.sessionKey); | ||
|
|
||
| let result; | ||
| try { | ||
| result = await runtime.compactEmbeddedPiSession({ | ||
| sessionId, | ||
| sessionKey: params.sessionKey, | ||
| allowGatewaySubagentBinding: true, | ||
| messageChannel: params.command.channel, | ||
| groupId: targetSessionEntry.groupId, | ||
| groupChannel: targetSessionEntry.groupChannel, | ||
| groupSpace: targetSessionEntry.space, | ||
| spawnedBy: targetSessionEntry.spawnedBy, | ||
| senderId: params.command.senderId, | ||
| senderName: params.ctx.SenderName, | ||
| senderUsername: params.ctx.SenderUsername, | ||
| senderE164: params.ctx.SenderE164, | ||
| sessionFile: runtime.resolveSessionFilePath( | ||
| sessionId, | ||
| targetSessionEntry, | ||
| runtime.resolveSessionFilePathOptions({ | ||
| agentId: sessionAgentId, | ||
| storePath: params.storePath, | ||
| }), | ||
| ), | ||
| workspaceDir: params.workspaceDir, | ||
| agentDir: sessionAgentDir, | ||
| config: params.cfg, | ||
| skillsSnapshot: targetSessionEntry.skillsSnapshot, | ||
| provider: params.provider, | ||
| model: params.model, | ||
| thinkLevel: params.resolvedThinkLevel ?? (await params.resolveDefaultThinkingLevel()), | ||
| bashElevated: { | ||
| enabled: false, | ||
| allowed: false, | ||
| defaultLevel: "off", | ||
| }, | ||
| customInstructions, | ||
| trigger: "manual", | ||
| senderIsOwner: params.command.senderIsOwner, | ||
| ownerNumbers: params.command.ownerList.length > 0 ? params.command.ownerList : undefined, | ||
| abortSignal: compactAbortController.signal, | ||
| }); | ||
| } finally { | ||
| runtime.clearActiveEmbeddedRun(sessionId, compactHandle, params.sessionKey); |
There was a problem hiding this comment.
setActiveEmbeddedRun / clearActiveEmbeddedRun
commands-compact.runtime.ts does not export setActiveEmbeddedRun or clearActiveEmbeddedRun. Both calls — runtime.setActiveEmbeddedRun(...) on line 132 and runtime.clearActiveEmbeddedRun(...) on line 176 — will fail at compile time and at runtime. The PR description says these re-exports were added to commands-compact.runtime.ts, pi-embedded-runner.ts, and pi-embedded.ts, but none of those changes appear in the diff.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/auto-reply/reply/commands-compact.ts
Line: 132-176
Comment:
**Missing runtime exports: `setActiveEmbeddedRun` / `clearActiveEmbeddedRun`**
`commands-compact.runtime.ts` does not export `setActiveEmbeddedRun` or `clearActiveEmbeddedRun`. Both calls — `runtime.setActiveEmbeddedRun(...)` on line 132 and `runtime.clearActiveEmbeddedRun(...)` on line 176 — will fail at compile time and at runtime. The PR description says these re-exports were added to `commands-compact.runtime.ts`, `pi-embedded-runner.ts`, and `pi-embedded.ts`, but none of those changes appear in the diff.
How can I resolve this? If you propose a fix, please make it concise.| let result; | ||
| try { | ||
| result = await runtime.compactEmbeddedPiSession({ | ||
| sessionId, | ||
| sessionKey: params.sessionKey, | ||
| allowGatewaySubagentBinding: true, | ||
| messageChannel: params.command.channel, | ||
| groupId: targetSessionEntry.groupId, | ||
| groupChannel: targetSessionEntry.groupChannel, | ||
| groupSpace: targetSessionEntry.space, | ||
| spawnedBy: targetSessionEntry.spawnedBy, | ||
| senderId: params.command.senderId, | ||
| senderName: params.ctx.SenderName, | ||
| senderUsername: params.ctx.SenderUsername, | ||
| senderE164: params.ctx.SenderE164, | ||
| sessionFile: runtime.resolveSessionFilePath( | ||
| sessionId, | ||
| targetSessionEntry, | ||
| runtime.resolveSessionFilePathOptions({ | ||
| agentId: sessionAgentId, | ||
| storePath: params.storePath, | ||
| }), | ||
| ), | ||
| workspaceDir: params.workspaceDir, | ||
| agentDir: sessionAgentDir, | ||
| config: params.cfg, | ||
| skillsSnapshot: targetSessionEntry.skillsSnapshot, | ||
| provider: params.provider, | ||
| model: params.model, | ||
| thinkLevel: params.resolvedThinkLevel ?? (await params.resolveDefaultThinkingLevel()), | ||
| bashElevated: { | ||
| enabled: false, | ||
| allowed: false, | ||
| defaultLevel: "off", | ||
| }, | ||
| customInstructions, | ||
| trigger: "manual", | ||
| senderIsOwner: params.command.senderIsOwner, | ||
| ownerNumbers: params.command.ownerList.length > 0 ? params.command.ownerList : undefined, | ||
| abortSignal: compactAbortController.signal, | ||
| }); | ||
| } finally { | ||
| runtime.clearActiveEmbeddedRun(sessionId, compactHandle, params.sessionKey); | ||
| } | ||
|
|
||
| const compactLabel = | ||
| result.ok || isCompactionSkipReason(result.reason) |
There was a problem hiding this comment.
result is possibly undefined after try/finally
let result is declared without an initializer. TypeScript will type it as CompactResult | undefined, so the immediate result.ok and result.reason accesses on lines 180+ will produce "Object is possibly undefined" type errors. If compactEmbeddedPiSession rejects (e.g. on abort), the exception propagates past the finally so result is never read — but TypeScript cannot prove that statically.
A simple fix is to lift the result assignment above the try block and keep the finally only for cleanup, or use a definite-assignment assertion if the type is otherwise guaranteed:
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/auto-reply/reply/commands-compact.ts
Line: 134-180
Comment:
**`result` is possibly `undefined` after try/finally**
`let result` is declared without an initializer. TypeScript will type it as `CompactResult | undefined`, so the immediate `result.ok` and `result.reason` accesses on lines 180+ will produce "Object is possibly undefined" type errors. If `compactEmbeddedPiSession` rejects (e.g. on abort), the exception propagates past the `finally` so `result` is never read — but TypeScript cannot prove that statically.
A simple fix is to lift the result assignment above the `try` block and keep the `finally` only for cleanup, or use a definite-assignment assertion if the type is otherwise guaranteed:
How can I resolve this? If you propose a fix, please make it concise.![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: 05cea516d3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ownerNumbers: params.command.ownerList.length > 0 ? params.command.ownerList : undefined, | ||
| }); | ||
| }; | ||
| runtime.setActiveEmbeddedRun(sessionId, compactHandle, params.sessionKey); |
There was a problem hiding this comment.
Callers now invoke runtime.setActiveEmbeddedRun(...) and runtime.clearActiveEmbeddedRun(...) in /compact, but src/auto-reply/reply/commands-compact.runtime.ts still only re-exports abortEmbeddedPiRun, compactEmbeddedPiSession, isEmbeddedPiRunActive, and waitForEmbeddedPiRunEnd. At runtime those new properties are undefined, so /compact will throw a TypeError before compaction starts instead of handling the command.
Useful? React with 👍 / 👎.
1 participant
Summary
Fixes #66535
The
/compactcommand invokedcompactEmbeddedPiSessionwithout passing anabortSignaland without registering the compaction session intoACTIVE_EMBEDDED_RUNS. As a result,chat.abort,abortEmbeddedPiRun, and/stophad no effect on an in-progress compaction.Changes
src/auto-reply/reply/commands-compact.ts:AbortController, pass its signal via the already-definedabortSignalfield onCompactEmbeddedPiSessionParamsEmbeddedPiQueueHandleand register/unregister withsetActiveEmbeddedRun/clearActiveEmbeddedRunsrc/auto-reply/reply/commands-compact.runtime.ts: Re-exportsetActiveEmbeddedRun,clearActiveEmbeddedRun, andEmbeddedPiQueueHandle.src/agents/pi-embedded-runner.ts&src/agents/pi-embedded.ts: ExportsetActiveEmbeddedRun,clearActiveEmbeddedRun, andEmbeddedPiQueueHandletype.How it works
AbortControllerand a minimalEmbeddedPiQueueHandleACTIVE_EMBEDDED_RUNSviasetActiveEmbeddedRun(sessionId, handle, sessionKey)abortSignalthrough tocompactWithSafetyTimeout(which already supports it)clearActiveEmbeddedRunremoves the handle in afinallyblockTesting
wizard/setup.tserrors are pre-existing and unrelated