Skip to content

fix(acp): start the claude CLI without the bypass-permissions flag - #10413

Open
amansk wants to merge 1 commit into
kirodotdev:mainfrom
amansk:fix/claude-cli-launcher
Open

fix(acp): start the claude CLI without the bypass-permissions flag#10413
amansk wants to merge 1 commit into
kirodotdev:mainfrom
amansk:fix/claude-cli-launcher

Conversation

@amansk

@amansk amansk commented Sep 12, 2026 •
edited
Loading

Copy link
Copy Markdown

Problem / Motivation

On the Claude Code backend, claude-agent-acp asks the Agent SDK for allowDangerouslySkipPermissions on every non-root session, which puts --allow-dangerously-skip-permissions on the claude command line. A CLI that refuses that flag in its environment (it does so as root, for example) fails every session/new with Claude Code process exited with code 1, so the backend is unusable there. The adapter gives an ACP client no way to turn the option off, and Crew does not ship the adapter, so it cannot patch that line either.

Why it matters

That flag is the capability to enter bypassPermissions, the one mode in which the adapter never sends session/request_permission and so never reaches Kiro Crew's host gate. Crew never selects that mode, so every session was requesting a capability the governance model exists to withhold, and on hosts whose CLI refuses it the Claude backend did not start at all.

What changed (motivation → approach → change)

Crew owns CLAUDE_CODE_EXECUTABLE, which the adapter forwards to the SDK as pathToClaudeCodeExecutable. Rather than a patched adapter or a text edit of its bundle, the claude backend now points that variable at a small node launcher shipped in the package, src/kiro_crew/acp/claude_launcher.mjs, and names the real CLI in KIROCREW_CLAUDE_CODE_EXECUTABLE. The launcher drops exactly that one flag and starts the real CLI with every other argument in order, stdio inherited, exit status and terminating signal propagated, and CLAUDE_CODE_EXECUTABLE set back to the real path. An operator-set CLAUDE_CODE_EXECUTABLE is still the CLI that runs; only the capability request is withheld. An explicit --permission-mode passes through unchanged, so the inherited-settings boundary the spec already documents is not closed by this. A launcher missing from the install starts the CLI directly, with a warning.

One follow-on in the same change: the launcher carries a #!/usr/bin/env node shebang and the executable bit, because the adapter also spawns CLAUDE_CODE_EXECUTABLE directly for claude auth status; without them that spawn fails with EACCES and every session logs an auth-status failure even though the session runs.

On Windows the launcher is not used: _point_adapter_at_claude_cli starts the CLI directly there, the pre-PR path, and logs that the capability is not withheld. An npm-installed claude resolves to a .cmd shim on Windows, and neither the SDK running a .mjs executable under node on win32 nor a cmd.exe routing of that shim has been exercised on a Windows host, so a wrong guess would fail every Windows Claude session at session/new where the direct path works today. Enabling it there waits on an end-to-end verification on a Windows host. An earlier revision of this PR shipped a COMSPEC routing for those shims; it is withdrawn.

The ACP spec no longer claims the adapter is vendored into the build; nothing in this repository ships it.

Tests

test/test_claude_launcher.py (16 tests):

  • _point_adapter_at_claude_cli sets the launcher as CLAUDE_CODE_EXECUTABLE and the resolved or operator-chosen CLI as the launcher target; no CLI found sets nothing and warns; a missing launcher falls back to the CLI directly with a warning; on Windows the CLI is started directly and the log says the capability is not withheld (test_windows_starts_the_cli_directly_until_verified).
  • The launcher ships in setup.cfg package data and MANIFEST.in, starts with the node shebang and is executable on POSIX.
  • Under node against fake CLIs: drops only the bypass flag and keeps argument order, names the real CLI in the child environment, passes an explicit --permission-mode through, propagates exit status, fails loudly on a missing or unstartable target, executes a native target directly.
  • The kiro spawn receives neither variable.

Manual verification

On macOS with agent.acp_backend = claude, kirocrew chat -m ... on this branch answers through the launcher, and the adapter's claude auth status spawn no longer logs EACCES (it did before the shebang and exec bit were added). A session with an explicit --permission-mode bypassPermissions and no capability flag starts on Claude Code 2.1.269 when not running as root, which is what the spec now states for inherited defaultMode: bypassPermissions. Not exercised on a Windows host, which is why Windows keeps the direct path.

Related Issues

None filed; found while bringing up the Claude backend on a host whose CLI refuses the flag.

Pattern harvest

Not generalizable: the defect is a capability request hard-wired inside a third-party adapter Crew does not ship; the fix is a one-off wrapper at the one seam Crew owns.

Checklist

  • At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable)
  • No secrets, credentials, or internal references in the diff

🤖 Generated with Claude Code

https://claude.ai/code/session_01FY8xmPpyfqCbxHPwM2gb9E

@amansk
amansk requested a review from a team as a code owner September 12, 2026 20:03
@amansk
amansk requested a review from iamwhatever September 12, 2026 20:03
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention labels Sep 12, 2026
@dwu96

dwu96 commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated.

Missing sections:

  • ## Problem / Motivation
  • ## Why it matters
  • ## What changed
  • ## Tests

Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle.

@github-actions

github-actions Bot commented Sep 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — ✅ PASS

Design-level review of 7602dfa9f0d227c089155dd6b997563f10fc324f via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Verified against the base tree: _vendor_acp_into_pkg appears only in the stale doc (not in setup.py), so the doc rewrite is a correction, not a smuggled behavior change; _to_thread_guarding_sandbox and agent_sdk.backend_install both exist. The design gate raises nothing actionable: the fix sits at the one seam Crew owns (CLAUDE_CODE_EXECUTABLE), fails open with warnings on missing launcher/CLI, keeps the unverified Windows path on pre-PR behavior with a named test and evidence path, states its scope (capability flag only, explicit --permission-mode untouched) exactly, and pins both wiring and launcher behavior. The residual coupling to the adapter's flag spelling degrades only to today's shipped behavior, loudly on the hosts that motivated the PR.

Design-Verdict: PASS

Interposes at the one seam Crew owns, withholds exactly one capability, fails open, defers unverified Windows — scope and residuals stated precisely and pinned.

[DESIGN-REVIEWED] 7602dfa

@github-actions

github-actions Bot commented Sep 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — 🔴 changes requested (blocking)

Reviewed 7602dfa9f0d227c089155dd6b997563f10fc324f via the fork AI-review pipeline; updated in place on each push.

1 of 1 blocking finding(s) are security-class and were withheld from adjudication, so the blocking verdict stands.

BLOCKING -- src/kiro_crew/acp/client.py:962 -- Agent can replace or remove the launcher to bypass the host gate (origin: validation)

if not _CLAUDE_LAUNCHER.is_file(): / env["CLAUDE_CODE_EXECUTABLE"] = str(_CLAUDE_LAUNCHER)
Agent edits launcher in a KiroCrew workspace/editable install -> next _spawn executes altered launcher or fail-open fallback -> bypass mode skips session/request_permission.
Anchor: residual/security
Fix: Use an integrity-protected launcher outside the writable workspace and fail closed if unavailable.
[BLOCK-MERGE] 7602dfa
[GPT-REVIEWED] 7602dfa

Adjudication (Opus 4.8) — is blocking on each finding proportionate?

I've reviewed the fenced finding F1 against the diff (as DATA) and the base tree.

F1 — Harm rung: fenced/unbounded (governance-ceiling bypass). Conditions I confirmed:

  • The launcher is a package code file: _CLAUDE_LAUNCHER = Path(__file__).with_name("claude_launcher.mjs") (src/kiro_crew/acp/client.py:209 in the diff), i.e. src/kiro_crew/acp/claude_launcher.mjs, sitting in the same package directory as client.py, hooks.py, security.py. To "replace" it the agent needs write access to package enforcement code — the same access that lets it rewrite _point_adapter_at_claude_cli itself, hooks.py, or security.py. The launcher is therefore no more writable, and offers no stronger a guarantee, than the Python that invokes it; this PR adds no new attack surface.
  • The "remove → fail-open fallback" branch (client.py:264-272 in the diff) sets env["CLAUDE_CODE_EXECUTABLE"] = claude_exe — byte-for-byte the pre-PR shipping behavior (the removed hunk at old client.py, diff lines 284-298). Recovery/baseline: that direct path is current production, and it is not itself a bypass — the flag is a latent capability; the mode gates, and the client writes defaultMode: default pre-spawn (claude-code-provider.md:100, and the diff states the launcher does not even close the separate --permission-mode bypassPermissions inherited-settings gap).
  • Real fix cost: an integrity-protected launcher outside the writable workspace would harden a mechanism that is strictly weaker than the un-protected client.py/hooks.py governing it; blocking here demands making all package enforcement code un-writable-by-the-agent, a pre-existing property this PR neither introduces nor can remedy.

The condition combination is extreme (package-code write = total pre-existing compromise) and the fallback merely restores today's production behavior, so a human would plausibly accept the residual — a FLAG.

[ADJUDICATION] 7602dfa total=0 uphold=0 downgrade=0
[GPT-ADJUDICATED] 7602dfa

[ADJUDICATION-FENCED] 7602dfa fenced=1 flagged=1
FLAG F1 src/kiro_crew/acp/client.py:962 -- Writing/removing the launcher requires write access to package enforcement code (same dir as client.py/hooks.py/security.py), which already defeats the whole gate; the missing-launcher fallback restores the exact pre-PR production behavior, which gates via the pre-spawn defaultMode:default, so no new bypass surface is introduced.
[GPT-ADJUDICATED-FENCED] 7602dfa

🏷️ Fenced finding(s) machine-flagged as likely edge case

The security fence keeps these findings blocking regardless of adjudication; the only clearance path is a human override recorded by a repository writer, who must independently verify a rationale before recording it — it is machine-authored, and a wrong override on a security-class finding ships exactly the class the fence exists to stop. (This lane's comment deliberately carries no override command.)

  • F1 src/kiro_crew/acp/client.py:962 — Writing/removing the launcher requires write access to package enforcement code (same dir as client.py/hooks.py/security.py), which already defeats the whole gate; the missing-launcher fallback restores the exact pre-PR production behavior, which gates via the pre-spawn defaultMode:default, so no new bypass surface is introduced.

@github-actions

github-actions Bot commented Sep 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5, fork) — ✅ PASS

Premise-level review of 7602dfa9f0d227c089155dd6b997563f10fc324f via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All claims verified against the base tree. The doc's vendoring claim (_vendor_acp_into_pkg) exists nowhere in code, _to_thread_guarding_sandbox has four existing sibling call sites, the spawn seam in _spawn is the only place the CLI is handed to the adapter, and the new env var has exactly one real consumer (the launcher). Final review:

First-Principles-Verdict: PASS

Before merge, confirm one end-to-end Claude session on Linux — the manual launcher verification cited is macOS-only, and session/new is the failure point.

Not justified as shipped

  • Item 6 — undeclared: the CLI resolution moving off the event loop (_to_thread_guarding_sandbox) is never mentioned in the description; it is harm-free (matches the 4 existing off-loop call sites in _spawn) but rode along unstated.
  • Item 9 — rides along: the spec rewrite dropping the vendoring claim is beyond the flag fix; it is evidenced, though — _vendor_acp_into_pkg greps to 0 hits in code, 1 hit in the doc itself, and setup.py has 0 hits for vendor|agentclientprotocol.

What this change ships

Inventory (10 items) — 8 justified

Intent: make the Claude backend start on hosts whose CLI refuses the bypass-permissions flag, by withholding a capability Crew never uses — a FIX.

  1. POSIX Claude sessions no longer request the bypass-permissions capability; a refusing CLI (e.g. root) now starts — justified
  2. An operator-set CLAUDE_CODE_EXECUTABLE binary still runs, but now through the launcher — justified
  3. New env var KIROCREW_CLAUDE_CODE_EXECUTABLE carries the real CLI to the launcher — justified
  4. Windows keeps the pre-PR direct path and logs that the capability is not withheld — justified
  5. A launcher missing from the install falls back to the direct path with a warning — justified
  6. CLI resolution now runs off the event loop — undeclared (see above)
  7. The launcher ships in package data (setup.cfg, MANIFEST.in) — justified
  8. The launcher is directly executable (shebang + exec bit) so the adapter's auth-status spawn works — justified
  9. The ACP spec no longer claims setup.py vendors the adapter — rides along (see above)
  10. New spec section and 16-test file pin the wiring and the launcher — justified

[FIRST-PRINCIPLES-REVIEWED] 7602dfa

@github-actions

github-actions Bot commented Sep 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed 7602dfa9f0d227c089155dd6b997563f10fc324f via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 7602dfa

@amansk
fix(acp): start the claude CLI without the bypass-permissions flag
7602dfa 
claude-agent-acp asks the Agent SDK for allowDangerouslySkipPermissions
on every non-root session, which puts --allow-dangerously-skip-permissions
on the claude command line. That flag is the capability to enter
bypassPermissions, the one mode in which the adapter never sends
session/request_permission. Crew never selects that mode, and the adapter
gives an ACP client no way to turn the option off.

The claude backend now points CLAUDE_CODE_EXECUTABLE at a small node
launcher shipped in the package. It drops that one flag and starts the
real CLI with every other argument, stdio and the environment unchanged.
An operator-set CLAUDE_CODE_EXECUTABLE is still the CLI that runs.
Sessions no longer fail on a CLI that refuses the flag in its environment.

The ACP spec no longer claims the adapter is vendored into the build;
nothing in this repository ships it.
@amansk
amansk force-pushed the fix/claude-cli-launcher branch from 0f43489 to 7602dfa Compare September 12, 2026 20:35
@amansk

amansk commented Sep 12, 2026

Copy link
Copy Markdown
Author

Dispositions for the review findings, addressed in 7602dfa9f (now a single commit):

  • Design / First Principles / GPT 5.6 / Opus, Windows COMSPEC branch (fixed by subtraction): the second commit is withdrawn entirely, which removes the unquoted batch path at claude_launcher.mjs:76, the resolveSpawnConfig export, and the two test-only env vars. Windows now takes the pre-PR direct path: _point_adapter_at_claude_cli starts the CLI directly on platform_compat.IS_WINDOWS and logs that the capability is not withheld there, so no Windows host is routed through an unverified spawn. Pinned by test_windows_starts_the_cli_directly_until_verified; the spec paragraph states what enabling it on Windows waits on.
  • Design, watch on inherited defaultMode: bypassPermissions (verified, spec updated): the CLI accepts an explicit --permission-mode bypassPermissions without --allow-dangerously-skip-permissions when not running as root (checked on Claude Code 2.1.269), so such a session starts; as root it refuses exactly as before the launcher existed. claude-code-provider.md now says which.

@amansk

amansk commented Sep 12, 2026

Copy link
Copy Markdown
Author

Disposition for the GPT 5.6 finding on 7602dfa9f (client.py, missing-launcher fallback):

Rebutted, no change. The launcher ships beside client.py, hooks.py and security.py inside the installed package: replacing or removing it needs write access to the enforcement code itself, which already defeats every gate in this module, so the launcher adds no surface that was not already the package's. The fallback when the file is absent starts the CLI directly, which is exactly the production path before this PR (the pre-spawn defaultMode: default settings seed still gates that session), so it restores today's behaviour rather than opening a new bypass. Failing closed instead would turn a damaged install into "no Claude sessions at all" for a residual the adjudication itself classed as a FLAG. The spec paragraph already records the fallback and its warning.

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

Reviewers

@iamwhatever iamwhatever Awaiting requested review from iamwhatever iamwhatever is a code owner automatically assigned from kirodotdev/kirocrew-team

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@amansk @dwu96