Migrate built-in coding agents to Generic ACP#95
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughOpenTag migrates provider-specific executor handling to a generic ACP runtime, adds Cursor, OpenCode, Hermes, and OpenClaw support, introduces readiness and process-tree conformance checks, rejects legacy Claude direct-adapter configuration, and updates documentation, scripts, tests, and Node.js requirements. ChangesACP runtime
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request transitions OpenTag to a unified, generic ACP-first (Agent Client Protocol) runner architecture, replacing direct adapters for Codex, Claude Code, and Hermes with a generic ACP host and built-in launch profiles. It introduces support for Cursor, OpenCode, and OpenClaw, updates the Node.js requirement to version 22, and adds a comprehensive ACP conformance test suite. Feedback is provided regarding a potential compatibility issue with git init -b on older Git versions, suggesting a fallback to git init and git branch -m instead.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/runner/src/acp-executor.ts (1)
628-656: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDerive cancellation confirmation from observed behavior, not
supportsCancel.
supportsCancelonly declares protocol capability. It does not prove that cancellation was accepted or that the process tree exited, yet it directly controls the result summary. Return termination status fromterminateChildand track ACP notification/stop outcomes; early cancellation before spawning can be confirmed immediately.Also applies to: 771-807, 991-999
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/runner/src/acp-executor.ts` around lines 628 - 656, The cancellation result in stopResult must use observed termination state rather than the protocol capability flag supportsCancel. Make terminateChild return whether termination completed, track ACP cancellation notification and stop outcomes through the execution flow, and mark cancellation as confirmed when it occurs before spawning; pass that derived status into stopResult so the summary and nextAction reflect actual provider-process termination.
🧹 Nitpick comments (2)
scripts/release/check-cli-package.mjs (1)
111-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover Cursor and OpenCode in the installed-package release gate.
These are new built-in ACP agents, but this probe can still pass if their packaged launch definitions or Registry provenance are broken.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/release/check-cli-package.mjs` around lines 111 - 134, Extend the installed-package release validation loop in the expected agent definitions to include Cursor and OpenCode, using their packaged launch arguments and required Registry provenance checks. Ensure manifests and definitions for both new built-in ACP agents are validated alongside codex and claude-code, while preserving the existing validation behavior.packages/local-runtime/test/config.test.ts (1)
57-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the OpenClaw launch mapping, not only its capabilities.
This test still passes if
command,profile, orgatewayUrlis dropped beforecreateBuiltInAcpExecutors. Inspect the generated launch descriptor or spy on the factory arguments so the configuration contract named by the test is covered.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/local-runtime/test/config.test.ts` around lines 57 - 70, Update the test around executorsFromConfig to inspect the generated OpenClaw launch descriptor or spy on the built-in ACP factory arguments, asserting that command, profile, and gatewayUrl from parseDaemonConfig are preserved. Keep the existing capability assertions and verify the mapped launch configuration, not just executor metadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package.json`:
- Around line 17-18: Resolve the duplicate npm scripts by updating
smoke:acp-conformance to invoke its intended distinct conformance suite, such as
openclaw-acp-conformance.ts, or remove the redundant script if no separate suite
exists; keep smoke:builtin-acp-conformance mapped to builtin-acp-conformance.ts.
In `@packages/cli/src/config.ts`:
- Around line 200-202: Update the supportsCancel field in the configuration
schema to default to false so custom ACP agents must explicitly attest to
confirmed cancellation capability; leave supportsProfile and readinessTimeoutMs
unchanged.
In `@packages/cli/test/executors.test.ts`:
- Around line 33-47: Update the assertions in the executor output test to
validate capabilities against the correct executor lines rather than using
global substring checks. Assert complete Codex, Hermes, and OpenClaw entries, or
isolate and parse each executor’s line before checking fields such as completion
and secrets, while preserving the expected capability values.
In `@packages/runner/src/acp-executor.ts`:
- Around line 483-528: Update terminateChild and the Windows branch of
signalProcessTree to terminate the entire ACP process tree rather than only the
adapter process, using a tree-wide mechanism such as a Job Object or taskkill
/T. Preserve the existing graceful-then-forced termination sequence and add a
Windows-specific test covering descendant processes.
In `@packages/runner/src/acp-registry.ts`:
- Around line 164-169: Update packageIsPinned to require npm versions with at
least three numeric components before treating a package as pinned, while
preserving the existing uvx handling and valid exact-version behavior. Add
floating-agent@1.2 to the needs_setup regression cases in
packages/runner/test/acp-registry.test.ts lines 100-117.
In `@packages/runner/src/security.ts`:
- Around line 260-262: Update isExplicitlySafeEnvName in
packages/runner/src/security.ts (lines 260-262) to compare environment names
exactly on POSIX while retaining case-insensitive matching on Windows. Extend
packages/runner/test/security.test.ts (lines 106-116) with a differently cased
sensitive variable and assert that it is removed.
In `@scripts/dev/start-lark.sh`:
- Around line 178-204: Align detect_executor, validate_executor, and
assert_executor_available with the ACP registry’s complete built-in executor set
by adding Cursor, OpenCode, and OpenClaw. Update the executor prompt/list to
include all built-in IDs, and preflight every launcher, including Codex and
Claude Code; use npx availability checks for executors backed by registry
packages rather than treating any launcher as implicitly available.
In `@scripts/test/builtin-acp-conformance.ts`:
- Around line 259-276: Update boundedRun to enforce runTimeoutMs by racing
executor.run against a rejecting deadline rather than only setting timedOut and
awaiting the run. When the deadline fires, cancel the run and ensure
cancellation cleanup is bounded by a separate grace period, so neither the agent
run nor cancel can stall indefinitely; preserve the existing success result and
timeout diagnostics for runs completing before the deadline.
In `@scripts/test/live-e2e-smoke.mjs`:
- Around line 33-38: Update the requiredCommands configuration to preflight
every selected ACP launcher: conditionally include npx when builtInAcpAgents
selects Codex, Claude Code, or OpenCode, and include the Cursor launcher when
Cursor is selected. Preserve the existing Hermes and OpenClaw checks.
---
Outside diff comments:
In `@packages/runner/src/acp-executor.ts`:
- Around line 628-656: The cancellation result in stopResult must use observed
termination state rather than the protocol capability flag supportsCancel. Make
terminateChild return whether termination completed, track ACP cancellation
notification and stop outcomes through the execution flow, and mark cancellation
as confirmed when it occurs before spawning; pass that derived status into
stopResult so the summary and nextAction reflect actual provider-process
termination.
---
Nitpick comments:
In `@packages/local-runtime/test/config.test.ts`:
- Around line 57-70: Update the test around executorsFromConfig to inspect the
generated OpenClaw launch descriptor or spy on the built-in ACP factory
arguments, asserting that command, profile, and gatewayUrl from
parseDaemonConfig are preserved. Keep the existing capability assertions and
verify the mapped launch configuration, not just executor metadata.
In `@scripts/release/check-cli-package.mjs`:
- Around line 111-134: Extend the installed-package release validation loop in
the expected agent definitions to include Cursor and OpenCode, using their
packaged launch arguments and required Registry provenance checks. Ensure
manifests and definitions for both new built-in ACP agents are validated
alongside codex and claude-code, while preserving the existing validation
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c3bf8112-32fe-4a60-bdbf-ce12c20aed28
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (81)
README.mdREADME.zh-CN.mdapps/opentagd/src/index.tsapps/opentagd/test/config.test.tsdocs/acp-agent-integration.mddocs/acp-first-agent-runtime-design.mddocs/agent-install.mddocs/configuration.mddocs/lark-local-start.mddocs/live-e2e-smoke-harness.mddocs/platforms/teams.en.mddocs/platforms/teams.zh-CN.mddocs/real-integration-smoke-test.mdexamples/github-to-echo/README.mdexamples/github-to-pr/README.mdpackage.jsonpackages/cli/README.mdpackages/cli/package.jsonpackages/cli/src/catalogs/capabilities.tspackages/cli/src/catalogs/executors.tspackages/cli/src/config.tspackages/cli/src/index.tspackages/cli/test/capabilities.test.tspackages/cli/test/config.test.tspackages/cli/test/docs-contract.test.tspackages/cli/test/executors.test.tspackages/core/src/mention.tspackages/core/src/schema.tspackages/core/test/mention.test.tspackages/core/test/schema.test.tspackages/discord/src/render.tspackages/dispatcher/test/presentation.test.tspackages/dispatcher/test/server.test.tspackages/local-runtime/README.mdpackages/local-runtime/package.jsonpackages/local-runtime/src/config.tspackages/local-runtime/src/runtime.tspackages/local-runtime/test/config.test.tspackages/local-runtime/test/doctor.test.tspackages/local-runtime/test/hermes-daemon.test.tspackages/runner/README.mdpackages/runner/package.jsonpackages/runner/src/acp-agent.tspackages/runner/src/acp-executor.tspackages/runner/src/acp-registry.tspackages/runner/src/builtin-acp.tspackages/runner/src/claude-code.tspackages/runner/src/codex.tspackages/runner/src/hermes-profile.tspackages/runner/src/hermes.tspackages/runner/src/index.tspackages/runner/src/security.tspackages/runner/test/acp-agent.test.tspackages/runner/test/acp-executor.test.tspackages/runner/test/acp-registry.test.tspackages/runner/test/builtin-acp-conformance-script.test.tspackages/runner/test/builtin-acp.test.tspackages/runner/test/claude-code.test.tspackages/runner/test/codex.test.tspackages/runner/test/executor-capability.test.tspackages/runner/test/fixtures/acp-agent.mjspackages/runner/test/hermes-profile.test.tspackages/runner/test/hermes.test.tspackages/runner/test/security.test.tspackages/teams/README.mdpackages/teams/src/render.tspackages/teams/test/render.test.tspackages/telegram/src/render.tsscripts/dev/run-gh-claude-local-test.shscripts/dev/run-github-webhook-live-test.shscripts/dev/run-slack-claude-local-test.shscripts/dev/run-slack-ui-trigger-local-test.shscripts/dev/start-github-slack-claude-test.shscripts/dev/start-lark.shscripts/release/check-cli-package.mjsscripts/test/builtin-acp-conformance.tsscripts/test/hermes-v018-profile-contract.mjsscripts/test/live-e2e-smoke.mjsscripts/test/openclaw-acp-conformance.tsskills/opentag/SKILL.mdskills/opentag/references/codex-runner.md
💤 Files with no reviewable changes (12)
- scripts/test/hermes-v018-profile-contract.mjs
- packages/local-runtime/test/hermes-daemon.test.ts
- scripts/dev/start-github-slack-claude-test.sh
- packages/runner/test/claude-code.test.ts
- packages/runner/test/hermes.test.ts
- packages/runner/src/codex.ts
- packages/runner/src/claude-code.ts
- packages/runner/src/hermes.ts
- packages/runner/test/codex.test.ts
- scripts/dev/run-gh-claude-local-test.sh
- scripts/dev/run-slack-ui-trigger-local-test.sh
- scripts/dev/run-slack-claude-local-test.sh
mingyooagi
changed the title
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request migrates OpenTag's coding agent execution layer from direct, custom adapters to a unified, generic Agent Client Protocol (ACP) host. It removes the dedicated claude-code and codex executors in favor of built-in ACP launch profiles, while expanding support to include cursor, opencode, hermes, and openclaw as built-in ACP agents. Additionally, the minimum Node.js requirement is bumped from version 20 to 22, configuration schemas are updated to enforce explicit workspace conformance (workspaceCwd: "required"), and a new built-in ACP conformance test suite is introduced to verify readiness, workspace paths, and process-tree cancellation. Since there are no review comments provided, I have no feedback to address.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
1 participant
Summary
taskkill /T /F, and real conformance deadlines.+2512 / -2571, net-59).Additional ACP-compatible agents now require a built-in definition/configuration entry instead of another executor implementation.
Validation
corepack pnpm test— 120 files, 1551 tests passedcorepack pnpm typecheckcorepack pnpm lint— all 23 workspace projects passedcorepack pnpm buildnpm_config_cache=/tmp/opentag-npm-cache npm run release:check— package, clean install, CLI help/doctor, and installed ACP launch definitions passedgit diff --checkOpenClaw cancellation status
OpenClaw is admitted as a built-in ACP agent with
supportsCancel: false. Normal ACP runs are supported. OpenTag still sends the ACP cancellation request and forcibly closes its local bridge process, but it does not claim that Gateway-owned shell/tool subprocesses have terminated. The batch therefore marks OpenClaw's process-tree cancellation casenot_applicable; the separate strict hard-cancellation probe remains available.The upstream hard-cancellation fix is tracked in openclaw/openclaw#107721.
A local smoke against OpenClaw 2026.7.1 reached the ACP bridge. The local Gateway was not running at
127.0.0.1:18789, so readiness was correctly recorded asneeds_setup, cancellation asnot_applicable, and conformance failures as zero. This is provider setup state, not a merge blocker for this PR.Summary by CodeRabbit
New Features
Bug Fixes
Documentation