fix(acp): opt-in parent completion routing for non-threaded run mode#40279
fix(acp): opt-in parent completion routing for non-threaded run mode#40279dewgie-bot wants to merge 10 commits into
Conversation
Greptile SummaryThis PR adds an opt-in config flag ( Key changes:
The fix is well-scoped, has good test coverage, and is safe to merge. Confidence Score: 5/5
Last reviewed commit: bf33756 |
| { | ||
| "name": "openclaw", | ||
| "version": "2026.3.8", | ||
| "version": "2026.3.7-acpresp-2", |
There was a problem hiding this comment.
Version downgrade breaks CalVer ordering
The version is being changed from 2026.3.8 to 2026.3.7-acpresp-2. This is a CalVer regression — March 7 is earlier than March 8. Semver-aware tools (npm, package managers, CI pipelines, dependabot) compare 2026.3.7-acpresp-2 as less than 2026.3.8, which means:
- Automated changelogs and release tooling may skip or misorient this release.
- Users on
2026.3.8would not receive this patch as an "upgrade." - Any monotonic version check will treat this as a downgrade.
The version should be bumped forward from the current base, e.g. 2026.3.8-acpresp-1 or simply 2026.3.9 to maintain ordering.
| "version": "2026.3.7-acpresp-2", | |
| "version": "2026.3.8-acpresp-1", |
Prompt To Fix With AI
This is a comment left during a code review.
Path: package.json
Line: 3
Comment:
**Version downgrade breaks CalVer ordering**
The version is being changed from `2026.3.8` to `2026.3.7-acpresp-2`. This is a CalVer regression — March 7 is earlier than March 8. Semver-aware tools (npm, package managers, CI pipelines, dependabot) compare `2026.3.7-acpresp-2` as **less than** `2026.3.8`, which means:
- Automated changelogs and release tooling may skip or misorient this release.
- Users on `2026.3.8` would not receive this patch as an "upgrade."
- Any monotonic version check will treat this as a downgrade.
The version should be bumped forward from the current base, e.g. `2026.3.8-acpresp-1` or simply `2026.3.9` to maintain ordering.
```suggestion
"version": "2026.3.8-acpresp-1",
```
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: 349c3c6710
ℹ️ 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".
| { | ||
| "name": "openclaw", | ||
| "version": "2026.3.8", | ||
| "version": "2026.3.7-acpresp-2", |
There was a problem hiding this comment.
Revert accidental package version downgrade
This change downgrades the root package version from 2026.3.8 to 2026.3.7-acpresp-2 in a non-release bugfix commit, which can break release/publish flow (npm won’t accept publishing a lower version than one already published) and makes update/version ordering inconsistent for consumers. This should stay monotonic unless this commit is explicitly performing a coordinated release.
Useful? React with 👍 / 👎.
|
@greptile-apps please re-review this PR. @codex please review. |
| function isNonThreadedAcpCompletionDeliveryEnabled(cfg: OpenClawConfig): boolean { | ||
| return cfg.acp?.dispatch?.nonThreadedCompletionToParent !== false; | ||
| } |
There was a problem hiding this comment.
Default is opt-out, not opt-in
nonThreadedCompletionToParent !== false evaluates to true whenever the field is undefined (i.e. not set at all). This means all existing deployments that have never set this flag will immediately start routing non-threaded run completions back to the parent chat after this PR lands — the opposite of the "opt-in" framing in the PR description and the risk mitigation:
"Mitigation: opt-in toggle, test coverage around run-mode routing behavior."
The schema.help.ts description does say "When true (default)" which at least makes the behaviour explicit, but it directly contradicts the PR description's "opt-in config flag" framing. Any deployment that relied on the previous silent-completion behaviour (e.g. because it has its own downstream completion handler) will silently start getting double-delivery.
If the intent is genuinely opt-in, the default should be false:
| function isNonThreadedAcpCompletionDeliveryEnabled(cfg: OpenClawConfig): boolean { | |
| return cfg.acp?.dispatch?.nonThreadedCompletionToParent !== false; | |
| } | |
| function isNonThreadedAcpCompletionDeliveryEnabled(cfg: OpenClawConfig): boolean { | |
| return cfg.acp?.dispatch?.nonThreadedCompletionToParent === true; | |
| } |
If the intent is to fix the bug for everyone by default (opt-out of the old broken behaviour), the PR description, help text, and risk section should be updated to reflect that existing installs are affected and must explicitly set false to preserve the old behaviour.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/acp-spawn.ts
Line: 84-86
Comment:
**Default is opt-out, not opt-in**
`nonThreadedCompletionToParent !== false` evaluates to `true` whenever the field is `undefined` (i.e. not set at all). This means **all existing deployments** that have never set this flag will immediately start routing non-threaded run completions back to the parent chat after this PR lands — the opposite of the "opt-in" framing in the PR description and the risk mitigation:
> "Mitigation: opt-in toggle, test coverage around run-mode routing behavior."
The `schema.help.ts` description does say "When true (default)" which at least makes the behaviour explicit, but it directly contradicts the PR description's "opt-in config flag" framing. Any deployment that relied on the previous silent-completion behaviour (e.g. because it has its own downstream completion handler) will silently start getting double-delivery.
If the intent is genuinely **opt-in**, the default should be `false`:
```suggestion
function isNonThreadedAcpCompletionDeliveryEnabled(cfg: OpenClawConfig): boolean {
return cfg.acp?.dispatch?.nonThreadedCompletionToParent === true;
}
```
If the intent is to fix the bug for everyone by default (opt-out of the old broken behaviour), the PR description, help text, and risk section should be updated to reflect that existing installs are affected and must explicitly set `false` to preserve the old behaviour.
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: c711a6fb3d
ℹ️ 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".
| const useInlineDelivery = | ||
| hasDeliveryTarget && spawnMode === "session" && !streamToParentRequested; | ||
| hasDeliveryTarget && | ||
| !streamToParentRequested && | ||
| (spawnMode !== "run" || isNonThreadedAcpCompletionDeliveryEnabled(cfg)); |
There was a problem hiding this comment.
Skip inline ACP run delivery for internal webchat origin
This condition now enables inline delivery for mode="run" whenever a source channel/to is present, but it does not exclude webchat (the internal channel). In that case spawnAcpDirect sends deliver: true with an internal origin, and the gateway delivery path cannot deliver back to webchat (it falls back to channel selection / non-webchat routing), so non-threaded ACP completions from webchat can still fail to return to the originating chat or be routed incorrectly. The new default therefore regresses/does not fix the intended UX for internal-channel runs.
Useful? React with 👍 / 👎.
|
Update after latest feedback: What I changed
Validation run
@greptile-apps please re-review this PR. @codex please review. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf33756dab
ℹ️ 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".
| const useInlineDelivery = | ||
| hasDeliveryTarget && spawnMode === "session" && !streamToParentRequested; | ||
| hasExternalDeliveryTarget && | ||
| !streamToParentRequested && | ||
| (spawnMode !== "run" || isNonThreadedAcpCompletionDeliveryEnabled(cfg)); |
There was a problem hiding this comment.
Limit opt-in completion relay to non-threaded runs
The new useInlineDelivery condition applies acp.dispatch.nonThreadedCompletionToParent to all mode="run" spawns, including thread=true runs. In that case this path is no longer non-threaded: a bound thread is created and to is set to channel:${boundThreadId}, so enabling the flag for non-threaded parent relay also changes behavior for thread-bound run workflows and can unexpectedly post inline output into the child thread. If this flag is intended to affect only non-threaded runs (as documented), gate it on the spawn actually being non-threaded (for example, no thread binding requested/resolved).
Useful? React with 👍 / 👎.
|
Pushed latest updates to this PR (head now includes What I changed in this update:
Validation performed:
@codex please review. |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
Closing this as implemented after Codex review. Current What I checked:
So I’m closing this as already implemented rather than keeping a duplicate issue open. Review notes: reviewed against 9f4ef6162dbd; fix evidence: release v2026.4.22, commit 00bd2cf7a376. |
Summary
runtime="acp",mode="run",thread=false) could complete successfully but not post completion back to the parent/originating chat.Change Type
Scope
User-visible / Behavior Changes
acp.dispatch.nonThreadedCompletionToParent(true|false)true: non-threaded ACP run completions are routed back to parent/main chat/session.false: previous behavior remains.Security Impact
Repro + Verification
Validated on local gateway with non-threaded ACP Codex runs:
false) and run non-threaded ACP marker task.true) and run same marker task.Files changed
src/agents/acp-spawn.tssrc/agents/acp-spawn.test.tssrc/config/types.acp.tssrc/config/zod-schema.tssrc/config/schema.labels.tssrc/config/schema.help.tspackage.jsonRisks and Mitigations
AI Assistance
This PR was AI-assisted.
This PR was created in conjunction with and with approval from @tkozzer.