fix(acp): classify a kindless mcp tool call by its transport

[vishalvignesh]

Problem / Motivation

An MCP tool invoked from a backend-routed child sub-session is auto-denied
before the user ever sees a prompt. The observed symptom in kirocrew chat is a
tool that never ran a command being refused outright, reported as
⛔ permission auto-rejected (missing security context) or silently denied by
the CLI's fail-closed shell backstop.

The refusal is not reproducible from a top-level session, which is what makes it
look intermittent: a crew-spawned subagent gets its own ACP session and is
top-level there, so the child gates never apply and the same tool succeeds. Only
a child sub-session — one with sub_session_id set — hits the gates.

Why it matters

Any MCP server a user has configured is unreachable from a child sub-session
whenever the backend omits kind on the tool_call frame, and there is no way
to opt out. child_mcp_identity_trusted is the designed escape hatch for
exactly this case, but it itself requires a resolved classification, so
child_unconditional_grant_eligible is false and no session grant, global
grant, or parent policy can rescue the call. The user's only recourse is to
disable permission checks wholesale, which trades a false deny for a real loss
of protection.

What changed (motivation → approach → change)

Symptom. A permission_request for an MCP tool is refused with no prompt.

Root cause. In _build_tool_call_event, the shell-classification cache
write was guarded by a resolved kind alone:

_kind_resolved = isinstance(update.get("kind"), str) and bool(update.get("kind"))
if tool_call_id and shell_cache is not None and _kind_resolved:
    shell_cache[_ck] = is_shell

A tool_call frame carrying no kind therefore wrote nothing. The following
permission_request — which carries no kind of its own and depends on the
cache — read shell_classified=False. Three independent consumers turn that
into a refusal:

• cli_chat.py::_unverifiable_shell — the fail-closed backstop — denies it.
• the low-fidelity-unaware consumer in session_handle.py auto-rejects it,
  audited as child_low_fidelity_unaware_consumer.
• types.py::child_mcp_identity_trusted, the escape hatch, requires
  shell_classified, so it cannot fire either.

Declining to cache an unresolved classification is correct — caching False
would let a later event read a resolved non-shell verdict that no
classification ever produced. The defect is that a kindless frame is treated as
carrying no classification signal at all, when an MCP frame carries a second,
stronger one.

Approach. Confine the transport proof to the identity-only lane, and mint no
classification. An earlier revision of this change wrote the shell cache from
the transport signal; review surfaced that a cached False reads back as a
RESOLVED non-shell classification (shell_classified=True), which flips
child_low_fidelity to False and un-gates the title-keyed
auto_approve_tools content matching — a kindless mutating call with a
read-looking, agent-authored title could then auto-approve without a prompt.
The reworked change leaves _dispatch.py untouched and instead lets the two
refusing consumers honor the identity proof directly:

• types.py::child_mcp_identity_trusted no longer requires
  shell_classified. The cache-provenance _meta.kiro server/tool pair is
  itself proof the call is MCP-served and therefore not a host shell command —
  a host shell or builtin can never carry a server name. The property already
  feeds only UNCONDITIONAL grant paths (session trust-all, global YOLO,
  parent_policy=auto), whose approve decision reads no agent-authored event
  data, so honoring identity without a classification widens nothing that
  matches on content.
• cli_chat.py::_unverifiable_shell gains the same narrow escape: an event
  whose trusted identity fields were populated by the tool_call cache hit is
  proven MCP-served, so there are no command bytes for the backstop to demand,
  and the request proceeds to the normal interactive prompt. The escape is not
  child-gated, so a top-level kindless MCP frame also moves from auto-deny to
  a prompt -- a deliberate widening in the safe direction (a human decides
  instead of a silent refusal).
• cli_chat.py::_chat opts the CLI into the child-fidelity contract
  (provider.child_fidelity_aware = True). Without this the third refusing
  consumer -- the session handle's fail-close gate
  (child_low_fidelity_unaware_consumer) -- rejects every low-fidelity child
  permission request before the CLI's escape can run. The CLI qualifies for
  the contract because its approval path runs no content-matching
  auto-approve: the child-context fail-close first, then the hook gate, then
  the _unverifiable_shell fail-close, then an interactive prompt that shows
  only non-model-authored context (the cached command, the _meta.kiro MCP
  identity, the target path); the
  non-interactive -m path stays fail-closed and denies rather than prompts.
  Because the opt-in admits every low-fidelity child event -- not just
  MCP-served ones -- the CLI's own first check re-applies the boundary: a
  low-fidelity child event is rejected (child_unverified_context) unless
  its identity is verified -- consumed as not event.child_unconditional_grant_eligible, the same hoisted boundary the
  dashboard runner, Slack gateway, and subagent manager already use, rather
  than a fourth hand-spelling of it. A child edit whose params never reached the
  cache would otherwise prompt without a Path line, and approving it would
  execute an undisclosed write; the trusted transport identity is the one
  context that survives an empty params cache and is shown to the human --
  the same args-blind consent contract the dashboard's interactive card
  provides.

The split is safe in both directions by construction:

• child_low_fidelity stays True for a kindless frame, so every
  content-matching auto-approve path (trusted patterns, trust-reads, title-keyed
  auto_approve_tools) stays gated. Interactive approvers still receive the
  ⚠️ UNVERIFIED child request annotation.
• A frame whose kind resolved to execute caches is_shell=True, which keeps
  both the identity property and the CLI escape closed for shell calls — the
  transport identity never waives a shell check.
• The identity fields are populated only from the backend's own _meta.kiro on
  the tool_call frame, guarded by the explicit mcp_identity_trusted
  provenance flag; inline _meta on the agent-reachable permission payload
  earns nothing.
• A frame carrying neither signal still resolves nothing: no classification, no
  identity trust, and deny-by-default everywhere it applied before.

Scope. client.py::_extract_tool_event writes the shell cache
unconditionally, which is a hole in the opposite direction (a wrong
classification rather than a false deny). It is deliberately left alone here and
is worth its own issue.

Tests

test/test_acp_runtime.py — four tests around the split:

• test_trusted_mcp_transport_earns_identity_trust_without_a_classification —
  drives _build_tool_call_event with a kindless frame carrying
  _meta.kiro.mcpServerName, then build_permission_event, and asserts both
  halves of the split: no shell-cache write, shell_classified False,
  child_mcp_identity_trusted True, child_unconditional_grant_eligible
True, and — the security lock — child_low_fidelity still True, so
  title matching stays gated.
• test_trusted_mcp_transport_never_waives_a_reported_shell_kind — a frame
  with kind: "execute" and a server name still caches True, and the
  resulting permission event's child_mcp_identity_trusted is False. Locks
  the never-waive direction.
• test_inline_mcp_server_name_on_a_permission_frame_earns_no_classification —
  inline _meta on the agent-reachable permission payload manufactures
  nothing: no cache write, no identity trust, no grant eligibility. Locks
  non-forgeability.
• test_child_mcp_identity_trusted_isolates_verified_identity (existing) —
  updated: an unresolved classification with trusted identity now reads
  identity-trusted while the composite stays low-fidelity.

test/test_acp_runtime.py —
test_aware_consumer_receives_kindless_mcp_child_permission drives the
handle's dispatch loop end-to-end (subagent registration, kindless
_meta-carrying tool_call, permission frame) and asserts the opted-in
consumer receives the event -- still low-fidelity, identity trusted -- and
the child_low_fidelity_unaware_consumer fail-close never fires.

test/test_cli.py —
test_chat_opts_into_the_child_fidelity_contract_before_start pins the
CLI's opt-in, and that it happens before start() so an early child frame
cannot race the gate.
test_low_fidelity_child_without_identity_is_rejected_not_prompted pins the
admission boundary (a param-less child edit is rejected with
child_unverified_context, the human never asked), and
test_identity_trusted_low_fidelity_child_still_reaches_the_prompt pins the
one admission through it (the prompt shows the non-forgeable server/tool
pair).
test_kindless_mcp_tool_reaches_the_prompt_instead_of_auto_denying drives the
shared-runtime parser end-to-end and asserts the user-visible outcome: the
prompt is shown (one read), the tool is approved, the audited outcome is
allowed rather than a silent deny — and shell_classified stays False
throughout, so the escape demonstrably rides the identity fields alone.

Manual verification

The child-path claim is verified by driving the handle's own dispatch loop
rather than by hand-built events:
test_aware_consumer_receives_kindless_mcp_child_permission feeds the raw
subagent registration, the kindless _meta-carrying tool_call, and the
permission frame through AcpSessionHandle and asserts the event is yielded
to an opted-in consumer with the trusted identity attached (and the
fail-close audit never fires). The originally reported symptom was observed
in kirocrew chat against a live backend that omits kind on MCP frames;
the non-reproducing sibling path is explained by the sub_session_id
discriminator (a crew-spawned subagent is top-level in its own ACP session,
so the child gates never apply to it).

Reverting the production hunks with the tests kept (the prove-the-test check)
fails the new assertions, confirming they lock the fix. The touched surfaces
run clean; flake8, mypy, isort, black, the docs-lint and spec-index
checks pass. The website/ lanes are not exercised — this diff touches no
frontend file.

Related Issues

Adjacent to the provenance-conflation problem described in #6228 (closed) and
#6938. This PR does not restructure the fidelity split; it fixes the narrower
defect that a kindless MCP frame never earns a classification in the first
place.

Pattern harvest

Rule candidate: review-prompt

Pattern: a security decision cached under one provenance signal, read later by a
consumer that cannot recompute it, where a legitimate frame carrying a
different trustworthy signal falls through to the fail-closed branch. The
guard was written as "did we see the signal I expected" rather than "did any
non-agent-authored signal resolve this," so the absence of one signal was
conflated with the absence of all of them. Worth asking of any write to a
cache that a fail-closed gate later reads: is every trustworthy provenance for
this value enumerated here, and does each one only ever resolve in the safe
direction?

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

[github-actions]

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

Premise-level review of 5dff2ba6c1bfac2604751d5da75cd1559fe69ec9 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 base-file checks confirm the description's claims: the base child_mcp_identity_trusted requires shell_classified (src/kiro_crew/acp/types.py:750), _unverifiable_shell denies on not shell_classified (src/kiro_crew/cli_chat.py:493), the session handle fail-closes low-fidelity child requests for fidelity-unaware consumers (src/kiro_crew/acp/session_handle.py:2528) so the CLI opt-in is genuinely required, the -m path already fail-closes (_can_prompt, cli_chat.py:794), and the relaxed property has 4+ counted real consumers (dashboard chat_runner.py:8567, slack gateway.py:1984, subagent_manager/run.py:1366, plus the new CLI gate). The flipped test pin is rebutted with the base's own documented invariant that mcp_server_name is populated only for MCP-served tools (types.py:617), and the rejected simpler alternative (writing the shell cache from the transport signal) is named with its failure mode. The acknowledged unfixed sibling (client.py::_extract_tool_event) is declared and out of scope.

First-Principles-Verdict: PASS

Verify the widest item: kindless MCP child calls now auto-approve under trust-all/YOLO/parent_policy=auto on dashboard, Slack, and subagent manager — not only the reported CLI defect.

What this change ships

Intent: stop a backend that omits kind on MCP tool_call frames from making every MCP tool auto-denied in child sub-sessions — a FIX.

Inventory (7 items)

1. Kindless MCP child tool call in kirocrew chat reaches the approval prompt instead of auto-deny — justified
2. Top-level kindless MCP call in kirocrew chat now prompts instead of auto-denying — justified
3. Kindless MCP child calls become eligible for unconditional grant paths everywhere (dashboard, Slack, subagent manager) — justified
4. CLI now receives all low-fidelity child requests, denying unverified ones with a printed notice and audit code child_unverified_context — justified
5. A shell-classified (kind: execute) frame stays excluded from the identity escape — justified
6. Specs (acp-client.md, security.md) updated in the same commit — justified
7. Prior test pin "unresolved classification disqualifies identity trust" rewritten, rebutted via the documented MCP-only mcp_server_name invariant — justified

[FIRST-PRINCIPLES-REVIEWED] 5dff2ba

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

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

Design-Verdict: PASS

Root-cause fix confined to the identity-only lane: the pin it rewrites is refuted by an in-repo-documented stronger signal, and every adjacent boundary gains a new pin.

The rewritten assertion (shell_classified=False → not identity-trusted, from #6228) is treated as a decision, not a gap: its stated rationale — "nothing proves this is not a shell tool" — is directly answered by the base tree's own documentation that _meta.kiro.mcpServerName is engine-set for MCP-served calls only (_dispatch.py:1718-1733, cli_chat.py:539-541), and this grants trust to no new principal, since kind was equally backend-authored. The never-waive direction (kind: execute still caches True), non-forgeability (inline permission-payload _meta earns nothing), and the content-matching gate (child_low_fidelity stays True) each get an explicit new pin, and the earlier cache-writing revision was rejected for a stated, correct reason. The CLI's opt-in re-applies the handle's boundary via the hoisted child_unconditional_grant_eligible rather than a re-spelling, and child_low_fidelity returning False for non-child events (types.py:704) keeps the new fail-close scoped to child requests. Specs updated in the same diff.

Suggestions

• The "_meta.kiro.mcpServerName only on genuinely MCP-served frames" semantics is now single-signal load-bearing for unconditional child grants; harness-parity.md never names it — add it as a parity invariant so a future adapter can't quietly break it.

[DESIGN-REVIEWED] 5dff2ba

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] 5dff2ba

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

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

Review details

No findings.
[GPT-REVIEWED] 5dff2ba

[vishalvignesh]

• Transport classification enables title-only auto-approval span=119f3295ea64 — fixed in 14f0b1f.

Kindless mutating child MCP call with cached params and a read-looking title -> child_low_fidelity=False -> title heuristic auto-approves without prompting.

The finding held: writing shell_cache[id]=False from the transport signal read back as a resolved classification, flipped child_low_fidelity to False, and un-gated the title-keyed auto_approve_tools path whose own comment says the agent-authored title "a child could forge" must stay gated.

The rework follows the suggested fix — the transport-only proof is now tracked separately and kindless calls stay ineligible for content-matching auto-approval:

• _dispatch.py is reverted to main entirely: no shell-cache write from the transport signal, so shell_classified stays kind-only and child_low_fidelity stays True for a kindless frame — the title heuristic remains gated.
• The identity proof moves to the identity-only lane: types.py::child_mcp_identity_trusted drops its shell_classified requirement (the cache-provenance _meta.kiro server name is itself proof the call is MCP-served and not a host shell command), and cli_chat.py::_unverifiable_shell honors the same escape. Both feed only paths that read no agent-authored content; not is_shell keeps a kind-resolved shell authoritative, so the identity split stays closed for shell calls.
• Locked by test_trusted_mcp_transport_earns_identity_trust_without_a_classification, which asserts the kindless MCP child event is grant-eligible AND still child_low_fidelity=True, plus the updated test_child_mcp_identity_trusted_isolates_verified_identity. Reverting the production hunks with the tests kept fails those assertions.

[vishalvignesh]

• The headline symptom survives the fix on the default backend — fixed in a711e75.

session_handle.py gates on _perm_event.child_low_fidelity and not self.child_fidelity_aware ... kirocrew chat builds its own kiro-backend provider with child_fidelity_aware at its default False — no CLI code sets it.

The finding held and the suggested fix is what shipped, both halves:

• cli_chat.py::_chat now sets provider.child_fidelity_aware = True immediately after building the provider — before start(), so an early child frame cannot race the gate (test_chat_opts_into_the_child_fidelity_contract_before_start pins the ordering). The opt-in comment records the contract the CLI satisfies: hook gate → _unverifiable_shell fail-close → a prompt built from non-model-authored fields only; the non-interactive -m path denies rather than prompts.
• The demanded handle-dispatch test exists: test_aware_consumer_receives_kindless_mcp_child_permission feeds the raw subagent registration, the kindless _meta-carrying tool_call, and the permission frame through AcpSessionHandle's own dispatch loop and asserts the event is yielded to the opted-in consumer (identity trusted, still low-fidelity) with the child_low_fidelity_unaware_consumer audit never firing.
• The PR body's Manual verification section was corrected accordingly — the handle-loop test is now the stated evidence for the child path, replacing the unsupported reproduction claim.

[vishalvignesh]

• Watch: the third refusing consumer (session_handle.py fail-close gate) still fires — fixed in a711e75: cli_chat.py::_chat opts into the child-fidelity contract, so the handle delivers the event to the CLI's prompt path instead of rejecting it. Locked by a handle-dispatch-loop test (test_aware_consumer_receives_kindless_mcp_child_permission) asserting the child_low_fidelity_unaware_consumer audit never fires for an opted-in consumer.
• Watch: the _unverifiable_shell escape is not child-gated, widening top-level behavior undeclared — fixed (declared): the PR body's Approach section now states the top-level widening explicitly — a top-level kindless MCP frame moves from auto-deny to a prompt, a deliberate widening in the safe direction (a human decides instead of a silent refusal). No code change: the reviewer's own assessment (low risk, lands on a prompt) matches ours.
• Watch: client.py writes the shell cache unconditionally (deferred sibling) — accepted-and-deferred, as the review itself confirmed: opposite failure direction (wrong classification rather than false deny), named in the PR body's Scope paragraph. As a fork contributor I cannot apply labels/assignees on this repo's issues, so the deferral is recorded here and in the body rather than as a labeled issue; happy to file it as an issue on request.

[vishalvignesh]

• The new handle test swallows an existing regression test — fixed in 7802f51.

The hunk at test/test_acp_runtime.py removes exactly one line, -async def test_answer_task_cap_marks_dead_instead_of_growing_unbounded(): ... and leaves the cap test's docstring and body dangling after the new test's finally: block.

Confirmed and exactly as described — the insertion consumed the neighboring test's decorator and def line, folding its body into the new test's tail where it still executed and passed, which is why the suite stayed green around the defect. The subtraction suggested is what shipped: the @pytest.mark.asyncio decorator and def line are reinstated, the new test ends at its finally:, and the cap test survives under its own name. Verified by collection count (309 items in the file vs 308 with the rider) and by running both tests independently.

[vishalvignesh]

• Global fidelity opt-in admits unrelated unverified child edits span=02b9488070a4 — fixed in 28b1dd0.

Child edit lacking cached parameters -> CLI prompt omits the target path -> approval executes an undisclosed write.

The finding held: the opt-in made the handle deliver EVERY low-fidelity child event, not just the MCP-served ones the CLI can present honestly, and _prompt_allows omits the Path line when the params cache is empty. The suggested fix is what shipped: _answer_permission now fail-closes FIRST — a low-fidelity child event is rejected (child_unverified_context) unless child_mcp_identity_trusted is true, mirroring the boundary AcpEvent.child_unconditional_grant_eligible draws for the unconditional grant paths.

Locked by two tests: test_low_fidelity_child_without_identity_is_rejected_not_prompted (a param-less child edit is rejected, the human never asked) and test_identity_trusted_low_fidelity_child_still_reaches_the_prompt (the one admission shows the non-forgeable server/tool pair). Spec updated in the same commit (acp-client.md records the boundary).

[vishalvignesh]

• "_answer_permission is hook gate" contradicts the new pre-hook child-context rejection span=44f906073002 — fixed in f94546d.

Fix: document the child fail-close before the hook gate.

Held: the opt-in comment described the pre-existing pipeline and was not updated when the child fail-close landed ahead of it. The comment now reads "child-context fail-close (rejecting a low-fidelity child request without a verified identity) -> hook gate -> _unverifiable_shell fail-close -> interactive human prompt", and the same ordering was corrected in the PR body and acp-client.md.

[vishalvignesh]

• Watch: the CLI child gate hand-rolls the negation of child_unconditional_grant_eligible — fixed in f94546d, taking the review's subtraction verbatim: the two-getattr conjunction is replaced with not event.child_unconditional_grant_eligible (safe because LLMEvent IS AcpEvent per providers/base.py, so the property always exists), making the CLI the fourth consumer of the hoisted boundary rather than a second spelling of it. The comment and acp-client.md now say "consume the property" instead of "mirrors" it. Truth table is identical, and the boundary tests (test_low_fidelity_child_without_identity_is_rejected_not_prompted, test_identity_trusted_low_fidelity_child_still_reaches_the_prompt) pass unchanged.

[bolichen97]

@vishalvignesh Thanks for this. Nothing here has landed on main: src/kiro_crew/acp/types.py still requires shell_classified in child_mcp_identity_trusted, src/kiro_crew/cli_chat.py still has the bare cache-miss deny in _unverifiable_shell, and child_unverified_context appears nowhere on main. So the fix is still wanted.

One coordination item before it can merge: #9053 (@karraghu) rewrites the same guard line. Both PRs start from the identical cli_chat.py blob and both edit if not event.shell_classified: in _unverifiable_shell, both add a new deny code next to _UNVERIFIED_SHELL_CODE, and both add fields to AcpEvent in src/kiro_crew/acp/types.py. They also share docs/system-specs/modules/acp-client.md, docs/system-specs/modules/security.md and test/test_cli.py.

The two escapes are complementary, not duplicated. Yours covers a kindless MCP tool_call frame and keys on the cached _meta.kiro server/tool identity; #9053 covers a KAS spawn that sends no tool_call frame at all and keys on _meta.kiro.consent. Neither implements the other, so both should land. Whichever goes second has to re-express its escape inside the merged branch, and one reviewer should check the fail-close ordering of the combined condition.

Suggested path: agree with @karraghu which lands first, then rebase (this branch is 272 commits behind) and re-express the escape on top. Expect smaller textual conflicts with #6307 in types.py and #6595 at the _chat provider anchor.

Posted from the 2026-09-08 open-PR relationship audit (read-only, one auditor per PR); reply here if any of this is wrong.

[bolichen97]

Rebased onto main 02d7a2d0 by a maintainer as part of the 2026-09-08 open-PR audit.

Conflicts: none, clean rebase (the branch was 272 commits behind; types.py:734 and cli_chat.py:493 were still pre-fix on main, so the change is still needed).

Gates run locally on the changed files only: black --check, isort --check-only, flake8 all clean; pytest test/test_acp_runtime.py 311 passed and pytest test/test_cli.py 373 passed, 2 skipped.

Please review the rebased result. Note this PR rewrites the same _unverifiable_shell guard line as #9053 (spawn attestation), so whichever lands second will need the merged fail-close order checked.

A maintainer push makes the maintainer the last pusher, so under the repo's last-push rule a second approver is needed. Reply here if anything looks wrong.