Skip to content

fix(adapter/claude-local): warn when adapterConfig.command is ignored by the ACP lane - #298

Open
claudegoogl-sudo wants to merge 2 commits into
masterfrom
fix/acp-ignore-command-warn
Open

fix(adapter/claude-local): warn when adapterConfig.command is ignored by the ACP lane#298
claudegoogl-sudo wants to merge 2 commits into
masterfrom
fix/acp-ignore-command-warn

Conversation

@claudegoogl-sudo

Copy link
Copy Markdown
Owner

Thinking Path

  • Paperclip is the open source app people use to manage AI agents for work
  • Adapters start agent processes. The Claude adapter (claude_local) has two execution engines: the CLI engine and the ACP engine.
  • The two engines read different config fields. The CLI engine uses adapterConfig.command as the Claude binary. The ACP engine reads only agentCommand / acpAgentCommand, then falls back to its own binary discovery.
  • An operator can point command at a wrapper that contains the agent process. If the run takes the ACP engine, that wrapper is never started. The run still starts, now uncontained, and nothing warns anywhere. This silent control-drop stayed undetected in a real deployment for days.
  • An adapter must not accept a dead security-relevant override in silence.
  • This pull request adds detection and a loud warning. It does not change which command either engine runs.
  • The benefit: a misconfigured wrapper now surfaces on the next run and on the adapter Test surface, with the exact field to use instead.

Linked Issues or Issue Description

No public issue exists for this change. Description below follows the bug report fields:

What happened?
resolveClaudeAcpCommand() in packages/adapters/claude-local/src/server/acp.ts reads only agentCommand / acpAgentCommand and then falls back to binary discovery. A non-empty adapterConfig.command is never consulted and never reported on the ACP lane. The CLI lane still honors adapterConfig.command (see execute.ts), so a command value that points at a containment wrapper works until the engine resolves to acp. Then the wrapper silently leaves the spawn path while runs keep starting.

Expected behavior
The adapter warns loudly when adapterConfig.command is set and the ACP lane ignores it. Resolution precedence stays unchanged.

Steps to reproduce
Set adapterConfig.command = "/x/wrapper.sh" on a claude_local agent with engine=acp, run the agent, and open the adapter Test surface. Before this change: no warning anywhere. After this change: one warning in the server log and the run log, plus one claude_acp_command_override_ignored warn check on the Test surface.

Agent adapter(s) involved
claude_local, ACP lane only. The CLI lane is unchanged.

What Changed

  • Add findIgnoredClaudeAcpCommandOverride(), a detection-only helper. It reports the dead command value only when no explicit ACP command (agentCommand / acpAgentCommand) is set. It never changes resolution.
  • Add formatIgnoredClaudeAcpCommandOverrideWarning(), one shared searchable warning line: adapterConfig.command is set but ignored in engine=acp; use agentCommand/acpAgentCommand — if this pointed at a wrapper, that wrapper is OFF the ACP spawn path (ignored value: ...).
  • Warn on the ACP run lane (createClaudeAcpExecutor): the warning goes to the server log (console.warn) and the run log (onLog("stderr", ...)), exactly once per run. The warning lives at the lane boundary because the command resolvers stay pure, and one run consults them several times. An engine gate keeps the CLI engine silent even if the closure is reached with engine=cli.
  • Warn on the ACP Test surface (testClaudeAcpEnvironment): a new warn check claude_acp_command_override_ignored, plus the same server-log line.
  • Update the agentCommand field hint in the config schema and the matching UI field: the CLI-only command field is ignored by this engine, and a run warns when it is set.
  • Add six regression tests: three warning-present cases and three silence guards.

Verification

  • pnpm exec vitest run packages/adapters/claude-local/src/server/acp.test.ts — 27 passed (21 tests before this change).
  • Red-on-pre check: ran the new test file against the pre-fix acp.ts source. 3 tests failed with expected [] to have a length of 1 but got +0 on both warning-present cases, plus the helper unit test. With the fix applied, all 27 pass.
  • pnpm --filter @paperclipai/adapter-claude-local typecheck — clean.
  • node scripts/check-no-internal-ids.mjs against the base — clean.
  • Manual equivalence: the warning test asserts the run still completes (exitCode === 0) with command set, which proves the detection changed no resolution precedence.

Risks

  • Low risk. Detection only. No config field changes meaning. No spawn path changes. The ACP fallback chain is untouched.
  • The warning fires at most once per run, and only when the ACP lane runs with a dead command value, or when the ACP Test surface runs. No log noise, no loops.
  • The warning echoes the ignored command value back. That value is operator-set adapter config — the same trust level the existing "Claude ACP server command is executable" check already renders.

For core feature work, check ROADMAP.md first and discuss it in #dev before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See CONTRIBUTING.md. This is a small focused hardening fix (Path 1), not core feature work.

Model Used

GLM via the prime_local runtime (local Paperclip adapter driving an event-stream agent CLI), extended reasoning + tool use + code execution, operating as agent "Coder" under the Paperclip runtime.

Checklist

  • I have included a thinking path that traces from project context to this change
  • I have specified the model used (with version and capability details)
  • I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
  • I have searched GitHub for duplicate or related PRs and linked them above
  • I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
  • I have not referenced internal/instance-local Paperclip issues or links (only public GitHub #NNN / github.com/paperclipai/paperclip URLs)
  • My branch name describes the change (e.g. docs/..., fix/...) and contains no internal Paperclip ticket id or instance-derived details
  • I have run tests locally and they pass
  • I have added or updated tests where applicable
  • I have updated relevant documentation to reflect my changes
  • I have considered and documented any risks above
  • All Paperclip CI gates are green — CI runs on this PR now; this box will be updated with measured gh pr checks output
  • Greptile is 5/5 with no open P2s, recommendations, or follow-ups — N/A: Greptile does not run on this fork repository; no Greptile review will appear here
  • I will address all Greptile and reviewer comments before requesting merge

Follows CONTRIBUTING.md PR template — sections present: Thinking Path, Linked Issues or Issue Description, What Changed, Verification, Risks, Model Used, Checklist.

claudegoogl-sudo and others added 2 commits September 7, 2026 11:22
… by the ACP lane

A containment wrapper wired through adapterConfig.command is honored only
by the CLI lane; the ACP lane resolves its server command from
agentCommand/acpAgentCommand alone, so such a wrapper silently left the
ACP spawn path while runs kept starting. Detect the dead override at the
lane boundary and warn on the server log, the run log, and the ACP Test
surface without changing resolution precedence. The warning stays silent
when an explicit ACP command (agentCommand/acpAgentCommand) is set and
for the CLI engine, which still honors the field.

Co-Authored-By: Paperclip <noreply@paperclip.ing>
…P server command hint

The ACP server command hint now states that the CLI-only command field is
ignored by the engine and that a run warns when it is set. Applied to the
server config schema and the matching UI field so operators see the trap
at the place they configure the adapter.

Co-Authored-By: Paperclip <noreply@paperclip.ing>
@claudegoogl-sudo

Copy link
Copy Markdown
Owner Author

Pinged for the required pre-merge security review on the board (reviewer: SecurityEngineer, who authored this remediation spec). Review focus: (1) warning placement at the lane boundary vs inside the resolvers, (2) suppression when an explicit ACP command is already set, (3) echo of the ignored operator-set value in the warning text. CI is running on this PR; the policy gate is green and the paths-filtered Storybook/review checks are skipped (measured, not assumed). I will treat any red check as P0 and fix before merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant