Skip to content

refactor: split ACP manager session flows#88752

Merged
steipete merged 1 commit into
mainfrom
refactor/acp-initialize-session-flow
May 31, 2026
Merged

refactor: split ACP manager session flows#88752
steipete merged 1 commit into
mainfrom
refactor/acp-initialize-session-flow

Conversation

@steipete

@steipete steipete commented May 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

  • split ACP manager session init, status, cancel, and startup identity reconciliation into focused control-plane helpers
  • centralize shared manager service callback types in manager.types.ts and reuse them across existing helpers
  • move initialize/cancel/startup identity tests out of broad runtime config/manager suites into targeted test files
  • fix the rebased Tailscale hostname mock typing that was failing CI check-test-types

Verification

  • node scripts/run-oxlint.mjs src/gateway/server-tailscale.test.ts src/acp/control-plane/manager.core.ts src/acp/control-plane/manager.initialize-session.ts src/acp/control-plane/manager.status.ts src/acp/control-plane/manager.cancel-session.ts src/acp/control-plane/manager.startup-identity-reconcile.ts src/acp/control-plane/manager.close-session.ts src/acp/control-plane/manager.turn-runner.ts src/acp/control-plane/manager.runtime-options-commands.ts src/acp/control-plane/manager.types.ts src/acp/control-plane/manager.test.ts src/acp/control-plane/manager.initialize-session.test.ts src/acp/control-plane/manager.cancel-session.test.ts src/acp/control-plane/manager.startup-identity-reconcile.test.ts src/acp/control-plane/manager.runtime-config.test.ts
  • pnpm tsgo:prod
  • pnpm test src/gateway/server-tailscale.test.ts src/acp/control-plane/manager.test.ts src/acp/control-plane/manager.initialize-session.test.ts src/acp/control-plane/manager.cancel-session.test.ts src/acp/control-plane/manager.startup-identity-reconcile.test.ts src/acp/control-plane/manager.runtime-config.test.ts src/acp/control-plane/manager.runtime-handles.test.ts
  • pnpm format:check src/acp/control-plane/manager.core.ts src/acp/control-plane/manager.initialize-session.ts src/acp/control-plane/manager.status.ts src/acp/control-plane/manager.cancel-session.ts src/acp/control-plane/manager.startup-identity-reconcile.ts src/acp/control-plane/manager.close-session.ts src/acp/control-plane/manager.turn-runner.ts src/acp/control-plane/manager.runtime-options-commands.ts src/acp/control-plane/manager.types.ts src/acp/control-plane/manager.test.ts src/acp/control-plane/manager.initialize-session.test.ts src/acp/control-plane/manager.cancel-session.test.ts src/acp/control-plane/manager.startup-identity-reconcile.test.ts src/acp/control-plane/manager.runtime-config.test.ts
  • git diff --check
  • pnpm check:test-types
  • .agents/skills/autoreview/scripts/autoreview --mode local
  • .agents/skills/autoreview/scripts/autoreview --mode branch --base origin/main

Real behavior proof:
Behavior addressed: ACP manager session-flow ownership is split out of AcpSessionManager without changing initialization, status, cancel, startup identity reconciliation, close, turn, or runtime-option behavior.
Real environment tested: Local OpenClaw checkout, Node/pnpm repo toolchain.
Exact steps or command run after this patch: Focused ACP manager/runtime config/runtime handle tests, gateway Tailscale test, prod/test type checks, lint, format check, diff check, and autoreview.
Evidence after fix: All listed local commands passed; autoreview reported no accepted/actionable findings.
Observed result after fix: manager.core.ts is down to 612 LOC, with init/status/cancel/startup identity flows in focused modules and matching focused tests.
What was not tested: Live ACP backend session initialization/cancel/status against a real external ACP provider.

@openclaw-barnacle openclaw-barnacle Bot added size: XL maintainer Maintainer-authored PR labels May 31, 2026
@clawsweeper

clawsweeper Bot commented May 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed May 31, 2026, 3:16 PM ET / 19:16 UTC.

Summary
The PR extracts ACP manager initialization, status, cancel, and startup identity reconciliation flows into focused control-plane helper modules and moves related tests into narrower files.

PR surface: Source +190, Tests +38. Total +228 across 14 files.

Reproducibility: not applicable. this is a refactor PR rather than a bug report. I compared the current-main manager flows to the PR helpers and reviewed the moved tests, but did not run tests in this read-only review.

Review metrics: none identified.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🌊 off-meta tidepool
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Maintainers can request one live ACP backend smoke for initialize/status/cancel if they want runtime proof beyond the listed local checks.

Risk before merge

  • [P1] No live ACP backend smoke is shown for initialize/status/cancel after the split; the listed local tests and type/lint checks cover parity but not an external ACP runtime integration path.

Maintainer options:

  1. Request live ACP smoke
    Ask for a small live ACP backend check that initializes a session, reads status, and cancels/closes it on the PR head before merge.
  2. Accept local parity proof
    Maintainership can choose to accept the focused unit/type/lint proof because the diff is a refactor and no behavioral delta was found in review.

Next step before merge

  • [P1] No automated repair is needed; the remaining action is maintainer review of an XL protected ACP session-flow refactor and its proof depth.

Security
Cleared: No concrete security or supply-chain concern was found; the diff is limited to TypeScript ACP control-plane refactoring and tests, with no workflows, lockfiles, dependencies, package scripts, credentials, or downloaded artifacts changed.

Review details

Best possible solution:

Land the refactor only after maintainers accept the local parity proof or add a focused live ACP backend smoke for the moved session flows.

Do we have a high-confidence way to reproduce the issue?

Not applicable; this is a refactor PR rather than a bug report. I compared the current-main manager flows to the PR helpers and reviewed the moved tests, but did not run tests in this read-only review.

Is this the best way to solve the issue?

Yes, this is an appropriate cleanup shape: the manager remains the entry point while session-flow logic moves into focused helpers with shared callback types. The main remaining question is proof depth, not a different implementation layer.

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 7c5a412b38d2.

Label changes

Label changes:

  • add P3: This is a refactor-only ACP manager cleanup with focused tests and no requested user-visible behavior change.
  • add merge-risk: 🚨 session-state: The diff moves ACP initialization, status, cancellation, and startup identity state transitions out of the core manager, so parity mistakes could stale or mis-associate session state.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The PR is maintainer-labeled and internal-refactor-only; its body provides local unit/type/lint proof, while live ACP backend proof remains a maintainer risk choice rather than an external-contributor gate.

Label justifications:

  • P3: This is a refactor-only ACP manager cleanup with focused tests and no requested user-visible behavior change.
  • merge-risk: 🚨 session-state: The diff moves ACP initialization, status, cancellation, and startup identity state transitions out of the core manager, so parity mistakes could stale or mis-associate session state.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The PR is maintainer-labeled and internal-refactor-only; its body provides local unit/type/lint proof, while live ACP backend proof remains a maintainer risk choice rather than an external-contributor gate.
Evidence reviewed

PR surface:

Source +190, Tests +38. Total +228 across 14 files.

View PR surface stats
Area Files Added Removed Net
Source 9 551 361 +190
Tests 5 409 371 +38
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 14 960 732 +228

What I checked:

  • Repository policy applied: The full root policy was read; it requires deep PR review for session/control-plane changes and keeps protected maintainer-labeled items open for explicit maintainer handling. (AGENTS.md:1, 7c5a412b38d2)
  • Live PR state and proof gap: The live pull request is open with maintainer and size: XL labels; the body lists focused local tests, type/lint/format checks, diff check, and autoreview, while explicitly noting no live ACP backend init/cancel/status test. (9c256f2e105b)
  • Current-main session flow baseline: Current main keeps ACP startup identity reconciliation, initialization, status, and cancellation logic inside AcpSessionManager, giving a direct baseline for the extraction comparison. (src/acp/control-plane/manager.core.ts:144, 7c5a412b38d2)
  • PR helper delegation: The PR head keeps AcpSessionManager as the entry point and delegates the extracted flows to runManagerStartupIdentityReconcile, runManagerInitializeSession, runManagerGetSessionStatus, and runManagerCancelSession with the same manager-owned dependencies. (src/acp/control-plane/manager.core.ts:132, 9c256f2e105b)
  • Dependency contract checked: The ACP runtime contract still exposes ensureSession, optional getStatus, cancel, and identity-bearing handles/statuses that the extracted helpers continue to use through the existing manager boundary. (packages/acp-core/src/runtime/types.ts:152, 7c5a412b38d2)
  • Focused tests moved with behavior: The PR adds targeted initialize, cancel, and startup identity reconciliation tests that preserve the moved assertions from the broad manager/runtime-config suites. (src/acp/control-plane/manager.initialize-session.test.ts:16, 9c256f2e105b)

Likely related people:

  • steipete: Peter Steinberger appears in current-main history for the ACP manager introduction and the immediately preceding extraction commits; the PR author metadata maps the current proposal to the same GitHub login. (role: recent area contributor; confidence: high; commits: 4b1d2faa9990, 2a30b937cba7, 720071a6c619; files: src/acp/control-plane/manager.core.ts, src/acp/control-plane/manager.runtime-handle-ensure.ts, src/acp/control-plane/manager.runtime-options-commands.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. labels May 31, 2026
@steipete
steipete force-pushed the refactor/acp-initialize-session-flow branch from 9c256f2 to 0fc3b6c Compare May 31, 2026 19:17
@openclaw-barnacle openclaw-barnacle Bot added the gateway Gateway runtime label May 31, 2026
@steipete
steipete force-pushed the refactor/acp-initialize-session-flow branch from 0fc3b6c to 681c17f Compare May 31, 2026 19:21
@openclaw-barnacle openclaw-barnacle Bot removed the gateway Gateway runtime label May 31, 2026
@steipete
steipete merged commit 118b9ca into main May 31, 2026
153 of 155 checks passed
@steipete
steipete deleted the refactor/acp-initialize-session-flow branch May 31, 2026 19:26
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jun 1, 2026
@steipete
refactor: split ACP manager session flows
1987a5e 
Split ACP manager session-flow ownership into focused helpers for initialization, status reads, cancellation, and startup identity reconciliation.

Verification:
- `node scripts/run-oxlint.mjs src/acp/control-plane/manager.core.ts src/acp/control-plane/manager.initialize-session.ts src/acp/control-plane/manager.status.ts src/acp/control-plane/manager.cancel-session.ts src/acp/control-plane/manager.startup-identity-reconcile.ts src/acp/control-plane/manager.close-session.ts src/acp/control-plane/manager.turn-runner.ts src/acp/control-plane/manager.runtime-options-commands.ts src/acp/control-plane/manager.types.ts src/acp/control-plane/manager.test.ts src/acp/control-plane/manager.initialize-session.test.ts src/acp/control-plane/manager.cancel-session.test.ts src/acp/control-plane/manager.startup-identity-reconcile.test.ts src/acp/control-plane/manager.runtime-config.test.ts`
- `pnpm tsgo:prod`
- `pnpm test src/acp/control-plane/manager.test.ts src/acp/control-plane/manager.initialize-session.test.ts src/acp/control-plane/manager.cancel-session.test.ts src/acp/control-plane/manager.startup-identity-reconcile.test.ts src/acp/control-plane/manager.runtime-config.test.ts src/acp/control-plane/manager.runtime-handles.test.ts`
- `pnpm format:check src/acp/control-plane/manager.core.ts src/acp/control-plane/manager.initialize-session.ts src/acp/control-plane/manager.status.ts src/acp/control-plane/manager.cancel-session.ts src/acp/control-plane/manager.startup-identity-reconcile.ts src/acp/control-plane/manager.close-session.ts src/acp/control-plane/manager.turn-runner.ts src/acp/control-plane/manager.runtime-options-commands.ts src/acp/control-plane/manager.types.ts src/acp/control-plane/manager.test.ts src/acp/control-plane/manager.initialize-session.test.ts src/acp/control-plane/manager.cancel-session.test.ts src/acp/control-plane/manager.startup-identity-reconcile.test.ts src/acp/control-plane/manager.runtime-config.test.ts`
- `git diff --check`
- `pnpm check:test-types`
- `.agents/skills/autoreview/scripts/autoreview --mode branch --base origin/main`
- GitHub PR checks for openclaw#88752 passed

Real behavior proof:
Behavior addressed: ACP manager session-flow ownership is split out of `AcpSessionManager` without changing initialization, status, cancel, startup identity reconciliation, close, turn, or runtime-option behavior.
Real environment tested: Local OpenClaw checkout, Node/pnpm repo toolchain, GitHub Actions PR CI.
Exact steps or command run after this patch: Focused ACP manager/runtime config/runtime handle tests plus prod/test type checks, lint, format check, diff check, autoreview, and PR CI.
Evidence after fix: All listed local commands passed, autoreview reported no accepted/actionable findings, and GitHub PR checks passed.
Observed result after fix: `manager.core.ts` is down to 612 LOC, with init/status/cancel/startup identity flows in focused modules and matching focused tests.
What was not tested: Live ACP backend session initialization/cancel/status against a real external ACP provider.
SYU8384 pushed a commit to SYU8384/openclaw that referenced this pull request Jun 3, 2026
@steipete
refactor: split ACP manager session flows
90bc698 
Split ACP manager session-flow ownership into focused helpers for initialization, status reads, cancellation, and startup identity reconciliation.

Verification:
- `node scripts/run-oxlint.mjs src/acp/control-plane/manager.core.ts src/acp/control-plane/manager.initialize-session.ts src/acp/control-plane/manager.status.ts src/acp/control-plane/manager.cancel-session.ts src/acp/control-plane/manager.startup-identity-reconcile.ts src/acp/control-plane/manager.close-session.ts src/acp/control-plane/manager.turn-runner.ts src/acp/control-plane/manager.runtime-options-commands.ts src/acp/control-plane/manager.types.ts src/acp/control-plane/manager.test.ts src/acp/control-plane/manager.initialize-session.test.ts src/acp/control-plane/manager.cancel-session.test.ts src/acp/control-plane/manager.startup-identity-reconcile.test.ts src/acp/control-plane/manager.runtime-config.test.ts`
- `pnpm tsgo:prod`
- `pnpm test src/acp/control-plane/manager.test.ts src/acp/control-plane/manager.initialize-session.test.ts src/acp/control-plane/manager.cancel-session.test.ts src/acp/control-plane/manager.startup-identity-reconcile.test.ts src/acp/control-plane/manager.runtime-config.test.ts src/acp/control-plane/manager.runtime-handles.test.ts`
- `pnpm format:check src/acp/control-plane/manager.core.ts src/acp/control-plane/manager.initialize-session.ts src/acp/control-plane/manager.status.ts src/acp/control-plane/manager.cancel-session.ts src/acp/control-plane/manager.startup-identity-reconcile.ts src/acp/control-plane/manager.close-session.ts src/acp/control-plane/manager.turn-runner.ts src/acp/control-plane/manager.runtime-options-commands.ts src/acp/control-plane/manager.types.ts src/acp/control-plane/manager.test.ts src/acp/control-plane/manager.initialize-session.test.ts src/acp/control-plane/manager.cancel-session.test.ts src/acp/control-plane/manager.startup-identity-reconcile.test.ts src/acp/control-plane/manager.runtime-config.test.ts`
- `git diff --check`
- `pnpm check:test-types`
- `.agents/skills/autoreview/scripts/autoreview --mode branch --base origin/main`
- GitHub PR checks for openclaw#88752 passed

Real behavior proof:
Behavior addressed: ACP manager session-flow ownership is split out of `AcpSessionManager` without changing initialization, status, cancel, startup identity reconciliation, close, turn, or runtime-option behavior.
Real environment tested: Local OpenClaw checkout, Node/pnpm repo toolchain, GitHub Actions PR CI.
Exact steps or command run after this patch: Focused ACP manager/runtime config/runtime handle tests plus prod/test type checks, lint, format check, diff check, autoreview, and PR CI.
Evidence after fix: All listed local commands passed, autoreview reported no accepted/actionable findings, and GitHub PR checks passed.
Observed result after fix: `manager.core.ts` is down to 612 LOC, with init/status/cancel/startup identity flows in focused modules and matching focused tests.
What was not tested: Live ACP backend session initialization/cancel/status against a real external ACP provider.
sablehead pushed a commit to sablehead/openclaw that referenced this pull request Jun 10, 2026
@steipete
refactor: split ACP manager session flows
96aef3c 
Split ACP manager session-flow ownership into focused helpers for initialization, status reads, cancellation, and startup identity reconciliation.

Verification:
- `node scripts/run-oxlint.mjs src/acp/control-plane/manager.core.ts src/acp/control-plane/manager.initialize-session.ts src/acp/control-plane/manager.status.ts src/acp/control-plane/manager.cancel-session.ts src/acp/control-plane/manager.startup-identity-reconcile.ts src/acp/control-plane/manager.close-session.ts src/acp/control-plane/manager.turn-runner.ts src/acp/control-plane/manager.runtime-options-commands.ts src/acp/control-plane/manager.types.ts src/acp/control-plane/manager.test.ts src/acp/control-plane/manager.initialize-session.test.ts src/acp/control-plane/manager.cancel-session.test.ts src/acp/control-plane/manager.startup-identity-reconcile.test.ts src/acp/control-plane/manager.runtime-config.test.ts`
- `pnpm tsgo:prod`
- `pnpm test src/acp/control-plane/manager.test.ts src/acp/control-plane/manager.initialize-session.test.ts src/acp/control-plane/manager.cancel-session.test.ts src/acp/control-plane/manager.startup-identity-reconcile.test.ts src/acp/control-plane/manager.runtime-config.test.ts src/acp/control-plane/manager.runtime-handles.test.ts`
- `pnpm format:check src/acp/control-plane/manager.core.ts src/acp/control-plane/manager.initialize-session.ts src/acp/control-plane/manager.status.ts src/acp/control-plane/manager.cancel-session.ts src/acp/control-plane/manager.startup-identity-reconcile.ts src/acp/control-plane/manager.close-session.ts src/acp/control-plane/manager.turn-runner.ts src/acp/control-plane/manager.runtime-options-commands.ts src/acp/control-plane/manager.types.ts src/acp/control-plane/manager.test.ts src/acp/control-plane/manager.initialize-session.test.ts src/acp/control-plane/manager.cancel-session.test.ts src/acp/control-plane/manager.startup-identity-reconcile.test.ts src/acp/control-plane/manager.runtime-config.test.ts`
- `git diff --check`
- `pnpm check:test-types`
- `.agents/skills/autoreview/scripts/autoreview --mode branch --base origin/main`
- GitHub PR checks for openclaw#88752 passed

Real behavior proof:
Behavior addressed: ACP manager session-flow ownership is split out of `AcpSessionManager` without changing initialization, status, cancel, startup identity reconciliation, close, turn, or runtime-option behavior.
Real environment tested: Local OpenClaw checkout, Node/pnpm repo toolchain, GitHub Actions PR CI.
Exact steps or command run after this patch: Focused ACP manager/runtime config/runtime handle tests plus prod/test type checks, lint, format check, diff check, autoreview, and PR CI.
Evidence after fix: All listed local commands passed, autoreview reported no accepted/actionable findings, and GitHub PR checks passed.
Observed result after fix: `manager.core.ts` is down to 612 LOC, with init/status/cancel/startup identity flows in focused modules and matching focused tests.
What was not tested: Live ACP backend session initialization/cancel/status against a real external ACP provider.
@clawsweeper clawsweeper Bot mentioned this pull request Jun 21, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

maintainer Maintainer-authored PR merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: XL status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@steipete