Repository navigation
fix(acp): preserve agent arguments and session ownership - #139776
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: blocked before merge. Reviewed September 6, 2026, 2:11 AM ET / 06:11 UTC. ClawSweeper reviewWhat this changesPreserves custom ACP agent arguments and saved sessions, consolidates turn cleanup, and repairs working-directory reset and model-selection handling. Merge readiness⛔ Blocked before merge - 2 items remain The prior legacy-model blocker is resolved, and no new blocking defect was found. This PR remains useful: current main still flattens custom agent arguments, and the merged delivery fix explicitly left this work for follow-up. Priority: P1 Review scores
Verification
How this fits togetherACP connects OpenClaw sessions to external coding agents. The acpx plugin turns configuration and saved session metadata into agent subprocesses, then returns output while tracking session and process cleanup. flowchart LR
A[Agent configuration] --> C[ACP session manager]
B[Saved session metadata] --> C
C --> D[Prepare arguments and validate models]
D --> E[acpx agent subprocess]
E --> F[Output and terminal result]
F --> G[Save session and release owned processes]
Before merge
Agent review detailsSecurityNone. PR surfaceSource -135, Tests +380, Docs +13, Config 0, Other 0. Total +258 across 26 files. View PR surface stats
Review metricsNone. Stored data modelPersistent data-model change detected: Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep arguments structured through launch and persistence, preserve existing session representations on reconnect, and let canonical lifecycle owners validate models and release processes. Do we have a high-confidence way to reproduce the issue? Yes, from source: current main converts custom-agent configuration into a scalar command, while pinned ACPX rejects scalar launches on Windows. No runtime reproduction was executed during this read-only review. Is this the best way to solve the issue? Yes. Using the existing argv contract and canonical turn lifecycle repairs the owning boundaries without adding configuration or reconstructing historical model intent. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against e9cedf96a463. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
Inspect wrapper argv before serialization and preserve native Windows command paths. Keep legacy reconnect intent unknown while validating persisted models before prompts. Align the history pagination fixture with current projection fields.
ba23679 to
76ebe2a
Compare
Dependency GuardThis PR changes dependency-related files. Maintainers should confirm these changes are intentional. Changed files:
Maintainer follow-up:
|
Dependency graph changes notedThis PR includes dependency graph changes. The dependency guard is informational because the PR author is a repository admin, a member of
Security review is still recommended before merge when the dependency graph change is intentional. |
|
@clawsweeper re-review Addressed the legacy-model finding in The regression builds metadata through the old initialization contract with no applied-model annotation, then exercises both original and fresh managers. Before the correction, legacy status failed after restart. Afterward, all 28 focused tests pass: status remains usable, rejected model replay never reaches the prompt runtime, and supported Codex/Claude selections survive restart. This also covers the requested rank-up evidence without claiming to reconstruct historical intent. Native Windows testing caught a separate wrapper-lease failure caused by inspecting JSON-escaped paths. Wrapper detection now consumes original argv, and configuration parsing preserves native Windows backslashes in relative/combined commands. Those repairs have passing local regressions and are being rerun in normal native Windows CI on the new head. The PR body records the initial failed proof and the current pending verification accurately. |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
The remaining review proof items are complete. Native Windows on the final head passed all seven applicable ACP regressions, including leased, relative and combined commands plus reconnect. Required CI is green for Authenticated smoke on exact merge The data-model detector's |
|
Merged via squash.
|
…9776) * fix(acp): preserve agent arguments and session ownership Keep portable argv through wrappers, leases, and reconnects; consolidate legacy turns onto canonical result cleanup. Retain cwd handle ownership, reset without backend startup, and preserve explicit model choices across reconnects. Fixes openclaw#136246; follows openclaw#139685. * fix(acp): preserve Windows paths and legacy model replay Inspect wrapper argv before serialization and preserve native Windows command paths. Keep legacy reconnect intent unknown while validating persisted models before prompts. Align the history pagination fixture with current projection fields.
Closes #136246. Follow-up to #139685. Thanks @eddiedon for reporting the Windows configuration failure.
What Problem This Solves
Fixes an issue where users configuring custom ACP agents on Windows would receive a raw-command rejection despite using the documented
commandandargsconfiguration. Arguments could also lose empty values or quoting, and converting an existing POSIX session command to argv could unexpectedly replace its session.The same review found that changing cwd could lose the handle that needed closing, resetting options could start a backend with the invalid settings being reset, legacy stream abandonment could leave a turn running, and an explicit model selection could silently fall back when model control was unavailable.
Why This Change Was Made
Keep command arguments structured from config through generated wrappers, model overrides, process leases, persistence, and reconnect. A single agent-bound prepared launch command replaces repeated quoting and reconstruction. Existing published ACPX scalar session records retain their representation when the command is unchanged.
Legacy
runTurnnow adapts the canonicalstartTurnlifecycle, so terminal results own cleanup and abandoning iteration cancels the exact turn. The manager retains cwd-stale handles until its existing replacement/shutdown owner closes them; reset closes only a cached handle and never starts a backend to erase options. Fresh explicit model requests fail visibly. Reconnect preserves the existing request shape because saved options do not retain caller intent; the canonical control replay still validates saved model choices before a prompt. Newly dropped inherited defaults are omitted during initialization.Production code: 300 added / 435 removed, net 135 lines removed. Tests and fixtures: 1,087 added / 707 removed. No dependency, schema version, protocol, or operator configuration keys are added. The assertion-safety allowance shrinks with the removed casts.
User Impact
Documented custom-agent configuration preserves executable paths and arguments on Windows and POSIX. Persistent sessions retain history across reconnect. Cwd changes and reset release owned backends, stopped consumers cannot leave legacy turns running, and explicit model choices are honored or rejected with an error. ACP setup/model documentation is updated.
Evidence
Independent Codex autoreview is clean through P2 after correcting both reported ownership findings.
codex-rs/app-server/src/request_processors/turn_processor.rs:1555andcodex-rs/app-server/src/bespoke_event_handling.rs:1394for interrupt/terminal behavior, andcodex-rs/core/src/config/mod.rs:945for explicit reasoning overrides.76ebe2a1783e7f012efc126e6b7c21b095e2af5f. Earlier interactive environment setup/transport attempts executed no tests and their leases were stopped.Follow-up: the local adaptive Vitest scheduler selected inconsistent worker counts for projects sharing
groupOrder. The unchanged full core suite passed with the runner's documented fixed-worker setting (OPENCLAW_VITEST_MAX_WORKERS=1); investigate adaptive project scheduling separately.CI recovery: refreshed onto main and corrected two stale pagination-fixture field names introduced across #139715 and #139753; no production/interface widening. The unrelated browser-route-handoff history=true E2E timed out while its live disclosure was collapsed, then passed unchanged in a focused native browser rerun. Its intermittent disclosure-expansion failure remains a test-harness follow-up; no timeout increase or speculative UI patch.
Legacy model handling: saved metadata does not encode caller intent, so reconnect keeps its existing semantics. Restart coverage is seeded through the old initialization contract without an applied-model annotation: status remains usable, unsupported model replay still rejects before a prompt, and supported Codex/Claude selections replay across a fresh manager. No new persistent provenance field or compatibility reader is introduced. Old unsupported overrides are not silently erased; reset-options remains the existing recovery path.
Current-main dependency proof: an independent ordinary checkout of merge
0817aa37b818ba18ecbf2e6c481a20633c407e35(PR parent76ebe2a1783e7f012efc126e6b7c21b095e2af5f) installed ACPX 0.13.2 and Codex ACP 1.6.2. All four probes exited 0: model-capability handling; actual process cleanup; authenticated legacy cancellation/reuse; complete delayed delivery (88 chunks), recall, delivery-failure cancellation and successful follow-up. Temporary credential homes and owned processes were cleaned.Data-model review note: the
package.jsonchange only selects the Windows regression test. No SQLite schema, schema version, or new model-provenance field is introduced. Compatibility is covered by real POSIX scalar-session reuse and the old-producer model restart sequences above.