fix(acpx): prevent duplicate -c config overrides for Codex ACP command#74339
fix(acpx): prevent duplicate -c config overrides for Codex ACP command#74339SymbolStar wants to merge 3 commits into
Conversation
…and already has them When a user configures the ACPX Codex command with -c model=X and/or -c model_reasoning_effort=Y, and the runtime also passes model/thinking overrides, appendCodexAcpConfigOverrides previously appended duplicate -c args blindly. This could cause unexpected behavior with the adapter. Now check if the command string already contains -c <key>=... before appending, respecting configured values as the baseline. This prevents doubling of -c args in the spawned codex-acp process. Also adds tests covering: - Configured command args not being duplicated (openclaw#74305 scenario) - Partial overlap (only missing keys are appended) - The wrapper command variant with pre-existing -c args Fixes openclaw#74305
Greptile SummaryAdds a Confidence Score: 4/5Safe to merge; fixes a real crash with no regressions on the reviewed path. No P0/P1 issues found. Two P2 style observations: (1) a silent precedence rule where a runtime model override is quietly dropped when the configured command already carries that key, and (2) a trivially redundant ?? guard. Neither affects correctness of the bug fix. No files require special attention. Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/acpx/src/runtime.ts
Line: 341-346
Comment:
**Runtime override silently dropped when key already present**
When a configured command already contains `-c model=<A>` and the runtime scope (e.g. `sessions_spawn`) supplies a *different* model `<B>`, the runtime value is silently discarded: the function returns the configured command unchanged. This is correct for the identical-value duplicate case, but a caller who genuinely wants to *switch* models at runtime will be surprised to find their override ignored with no log or error.
Consider adding a comment documenting the precedence rule, or logging a warning when the configured and runtime values diverge.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: extensions/acpx/src/runtime.ts
Line: 330-331
Comment:
**Redundant nullish coalescing**
`i + 1 < parts.length` is already verified by the outer condition, so `parts[i + 1]` is guaranteed to be a non-undefined `string` here. The `?? ""` fallback is unreachable.
```suggestion
if (parts[i + 1]!.startsWith(`${key}=`)) {
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(acpx): prevent duplicate -c config o..." | Re-trigger Greptile |
| if (override.model && !commandHasConfigKey(command, "model")) { | ||
| configArgs.push(`model=${override.model}`); | ||
| } | ||
| if (override.reasoningEffort && !commandHasConfigKey(command, "model_reasoning_effort")) { | ||
| configArgs.push(`model_reasoning_effort=${override.reasoningEffort}`); | ||
| } |
There was a problem hiding this comment.
When a configured command already contains -c model=<A> and the runtime scope (e.g. sessions_spawn) supplies a different model <B>, the runtime value is silently discarded: the function returns the configured command unchanged. This is correct for the identical-value duplicate case, but a caller who genuinely wants to switch models at runtime will be surprised to find their override ignored with no log or error.
Consider adding a comment documenting the precedence rule, or logging a warning when the configured and runtime values diverge.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/acpx/src/runtime.ts
Line: 341-346
Comment:
**Runtime override silently dropped when key already present**
When a configured command already contains `-c model=<A>` and the runtime scope (e.g. `sessions_spawn`) supplies a *different* model `<B>`, the runtime value is silently discarded: the function returns the configured command unchanged. This is correct for the identical-value duplicate case, but a caller who genuinely wants to *switch* models at runtime will be surprised to find their override ignored with no log or error.
Consider adding a comment documenting the precedence rule, or logging a warning when the configured and runtime values diverge.
How can I resolve this? If you propose a fix, please make it concise.| if (parts[i] === "-c" && i + 1 < parts.length) { | ||
| if ((parts[i + 1] ?? "").startsWith(`${key}=`)) { |
There was a problem hiding this comment.
i + 1 < parts.length is already verified by the outer condition, so parts[i + 1] is guaranteed to be a non-undefined string here. The ?? "" fallback is unreachable.
| if (parts[i] === "-c" && i + 1 < parts.length) { | |
| if ((parts[i + 1] ?? "").startsWith(`${key}=`)) { | |
| if (parts[i + 1]!.startsWith(`${key}=`)) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/acpx/src/runtime.ts
Line: 330-331
Comment:
**Redundant nullish coalescing**
`i + 1 < parts.length` is already verified by the outer condition, so `parts[i + 1]` is guaranteed to be a non-undefined `string` here. The `?? ""` fallback is unreachable.
```suggestion
if (parts[i + 1]!.startsWith(`${key}=`)) {
```
How can I resolve this? If you propose a fix, please make it concise.|
Codex review: needs real behavior proof before merge. Workflow note: Future ClawSweeper reviews update this same comment in place. How this review workflow works
Summary Reproducibility: yes. source-reproducible: current main appends Codex ACP model/thinking startup PR rating Rank-up moves:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. Real behavior proof Risk before merge
Maintainer options:
Next step before merge Security Review findings
Review detailsBest possible solution: Keep the fix ACPX-local, but parse the Codex ACP command so explicit runtime model/thinking overrides replace conflicting configured Do we have a high-confidence way to reproduce the issue? Yes, source-reproducible: current main appends Codex ACP model/thinking startup Is this the best way to solve the issue? No. Skipping already-present keys avoids identical duplicates, but it is not the best fix because it silently discards explicit runtime overrides that the docs define as child-session overrides. Label justifications:
Full review comments:
Overall correctness: patch is incorrect What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 258524973798. |
|
Hi @steipete — friendly ping 🙏 CI is green now (rebased on latest main). Would appreciate a look when you get a chance! |
|
Merged latest main, resolved 1 conflict in |
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
|
Fixed |
|
Heads up: this PR needs to be updated against current |
Root Cause
When a user configures the ACPX Codex command with
-c model=gpt-5.5 -c model_reasoning_effort=medium, and the runtime model override scope is active (e.g. viasessions_spawnwithmodel/thinking),appendCodexAcpConfigOverridesblindly appended additional-cargs to the command string without checking if those keys were already present. This caused duplicate-c model=.../-c model_reasoning_effort=...args to be passed to the codex-acp adapter.Additionally, on v2026.4.26 (the reported version), the older wrapper code and codex-acp 0.11.x dependency had different arg handling that contributed to the failure. Current main already improved the wrapper normalization and upgraded to codex-acp 0.12.0, but the duplicate-append issue remained.
Changes
extensions/acpx/src/runtime.ts: AddcommandHasConfigKey()that checks if a command string already contains a-c <key>=...arg. ModifiedappendCodexAcpConfigOverrides()to skip appending a-coverride if that config key is already present in the command string, preventing duplication.extensions/acpx/src/runtime.test.ts: Added tests covering:-c modeland-c model_reasoning_effortis not duplicated-cargscommandHasConfigKeyutility behavior-coverrides, no runtime model passedFixes #74305