Repository navigation
refactor(acp): remove duplicate runtime lifecycle plumbing - #140163
Merged
Merged
Conversation
Contributor
|
🦞👀 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. |
steipete
marked this pull request as ready for review
September 6, 2026 14:19
Contributor
|
Codex review: blocked before merge. Reviewed September 6, 2026, 10:34 AM ET / 14:34 UTC. ClawSweeper reviewWhat this changesConsolidates ACP session queues, configuration parsing, wrapper creation, and error handling while adding cancellation compatibility tests. Merge readiness⛔ Blocked before merge - 2 items remain This remains useful maintainer-requested cleanup beyond the two merged lifecycle repairs. No actionable introduced defect was found in the inspected diff; the protected maintainer PR should remain open. Priority: P3 Review scores
Verification
How this fits togetherOpenClaw’s ACP control plane routes session commands to the acpx plugin, which launches external coding harnesses. This layer manages session reuse, output, cancellation, and cleanup of owned processes. flowchart TD
A[Session commands] --> B[ACP control plane]
B --> C[acpx plugin]
D[Validated configuration] --> C
C --> E[Separate session and lease queues]
E --> F[External coding harness]
F --> G[Output and terminal results]
G --> B
C --> H[Owned process cleanup]
Before merge
Agent review detailsSecurityNone. PR surfaceSource -232, Tests +44, Other -1. Total -189 across 13 files. View PR surface stats
Review metricsNone. Stored data modelPersistent data-model change detected: Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep lifecycle serialization and error normalization with their existing shared owners while preserving session identity, configuration defaults, and cleanup predicates. Do we have a high-confidence way to reproduce the issue? Not applicable: this PR proposes behavior-preserving cleanup, and source review identified no concrete introduced failure to reproduce. Is this the best way to solve the issue? Yes, the inspected changes reuse existing owners and remove redundant boundaries without changing defaults or storage formats; independent ACPX decoder inspection remains limited. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 37aaf073776c. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
Contributor
Author
|
Merged via squash.
|
github-actions Bot
pushed a commit
to Desicool/openclaw
that referenced
this pull request
Sep 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related: #139685, #139776
What Problem This Solves
ACP/acpx retained duplicate serialization, validation, wrapper-writing, and error-normalization paths after the recent output and cancellation repairs. This maintainer-requested follow-up reduces the code that must stay consistent when maintaining session lifecycle behavior.
Why This Change Was Made
Reuse the existing SDK keyed queue for session initialization and process lease transitions, keeping their lock domains separate. Let the config schema own parsing/defaults, share identical wrapper/root resolution logic, and call the existing capability and error owners directly. Remove the unused raw-config type, redundant casts, one-use forwarding helpers, and duplicate error boundaries.
Production TypeScript: +105 / -337, net −232 lines. Tests: +44 lines covering idle cancellation error causes/identity and persisted error state. The assertion baseline only shrinks. No configuration, protocol, dependency, storage, or public API changes.
User Impact
No intentional behavior change. Existing ACP output delivery, cancellation, session reuse, model selection, wrapper launch, and process cleanup behavior is preserved with fewer duplicate paths.
Evidence
pnpm buildpassed, including generated plugin/SDK declarations and plugin load checks.node scripts/check-changed.mjspassed all selected formatting, core/plugin typechecks, lint, dead-export and repository guards.ClawSweeper follow-up: both pre-merge proof requests are addressed without code changes.
dist/runtime.js:1799-1821and the matching tagged decoder source. It returns either null or a record whose agent/name/cwd are nonempty strings and mode is persistent/oneshot. A direct probe of the installed published runtime passed two valid and seven invalid handle cases. The removed cast and checks duplicated this decoder contract.persistMigratedCodexMcpConfig, TOML serialization, isolated-home preparation, generated wrapper contents/filenames, and lease/session record shapes remain intact. The shared writer retains the same mkdir/write/chmod sequence; launch construction reuses the already parsed command. All 27 existing wrapper/auth/migration tests pass on both original and candidate implementations, including legacy Codex command/MCP migration, maintained argv forwarding, isolated-home/rebuild isolation, existing auth preservation, and rejected chmod. Candidate coverage is included in the 505-test run; original-source compatibility runs passed separately. The data-model classification is a false positive for this refactor, so no new migration is needed.