feat(side-chat): auto-approve read-only tools and add /btw alias

[pepmach]

What changed

Side Chat previously ran every turn under REJECT_ALL, so even lookups were refused. This PR lets side turns use read-only tools, and adds /btw as an alias for /side.

Backend — ToolApprovalPolicy.READ_ONLY (src/kiro_crew/llm_helpers.py)

• New policy member: the dashboard "Reads" approval mode's semantics ported to surfaces with no interactive approver. The always-enforced deny floor runs first (unchanged); then the request goes through the same HookManager.on_tool_call gate the Reads mode uses — is_read_only_bash for shell; otherwise the host-known read-only built-in identity (_HOST_READ_ONLY_BUILTIN_TOOLS: fs_read, glob, grep, web_fetch, web_search), taken from the non-model-authored _meta.kiro.toolName with no MCP server behind it and only when the event carries the mcp_identity_trusted provenance flag; name-grant verification on every auto-approve. The agent-influenced ACP kind and the title may narrow but never prove — a read kind or a read-looking title alone is refused. Bash moves from REJECT_ALL to bounded unattended execution: a shell call runs with no approver only when is_read_only_bash accepts it under the deny-by-default prefix allowlist, and every other shell call is rejected. Where the interactive Reads mode falls back to the approval card, this policy rejects (SEL reason read_only_policy), and it rejects before the interactive callback so a caller passing one cannot widen the policy.
• Fail-closed: READ_ONLY without a hooks manager rejects everything (read_only_policy_no_hooks) — there is no classifier to prove a call read-only.
• dashboard/handlers/side.py flips REJECT_ALL → READ_ONLY, builds the hook gate from the loaded config (default-constructed = all built-in rules on config-load failure), and passes the side session's own identity triple (side:{slot}, agent, app) so SEL rows and governance profile resolution describe the side surface.

Disclosed boundary (option a, accepted): a tool in the agent's allowedTools is auto-approved by kiro-cli itself and never raises a permission request, so no host policy sees it. This was already true under REJECT_ALL; the user blanket-consented those tools. A derived <agent>--readonly spec with allowedTools: [] is a possible follow-up if a hard guarantee is wanted.

Frontend — /btw alias + stale copy

• SIDE_RE in pages/chat/ChatInput.tsx now matches /side and /btw; interception and the steer bypass come free since both use it.
• /btw registered as a frontend command in SlashCommandMenu.tsx, sharing /side's description key so the two rows can never drift and locale catalogs stay untouched.
• The side composer's footer hint ("Context only · Tools and MCPs are unavailable here") was stale under the new policy — reworded to "Read-only · Lookups work here, but changes don't. Use the main chat to take action." across en.manual and all 12 locales (pseudolocale regenerated). The backend empty-output fallback message was updated to match.

Housekeeping in this diff

• Black baseline: the three touched Python files graduated to fully black-clean; the gate itself instructed pruning them (--update-baseline), which carries a small amount of mechanical reformatting in pre-existing lines of those files.
• Comment-history gate: llm_helpers.py carried four issue-number narrations above its baseline on current main (added by an earlier merge); the gate fails any diff touching the file, and its stated remedy is rewording the matched lines, so this PR rewords them (content preserved, #8812/#9082 references dropped).

Tests

• test/test_llm_helpers_tool_gate.py: 21 new tests against the REAL HookManager — read-only shell approved, mutating shell rejected (not a security block), host-known read built-in approved, read kind alone / read-looking title alone / host-known name without trusted provenance / MCP tool with a read kind all rejected, execute-kind rejected, no-hooks fails closed, deny floor still wins under READ_ONLY, config grants cannot widen the policy. test/test_hooks.py: TestClassifierOnlyHostTrustedProof pins _is_host_read_only_builtin and the allowlist-to-read-scope mapping (test_host_read_only_builtins_map_only_to_read_scopes).
• test/test_side.py: side turn streams under READ_ONLY with a real hook gate and the side session's own identity.
• website/src/test/SideSlashCommand.test.tsx: /btw and /btw <msg> intercept like /side; prefix look-alikes (/btwx, /sidebar) do not.
• website/src/test/SideSlashCommand.steer.test.tsx: /btw <msg> bypasses steer into a side turn while a main turn is running.
• Suites run locally: 63 backend tests across the touched files (all pass), 19 frontend tests (all pass), full i18n suite 665 tests (all pass), tsc, eslint, mypy, flake8, black/isort, and the three baselined gates (black, comment-history, subprocess-encoding) — all green at this head.

Evidence (isolated pod, pod-e2e harness, head 1a5fc60)

/bt in the composer offers the alias:

/btw what is 2+2? answer in one word opens Side Chat, dispatches the turn, and the footer shows the new read-only hint:

Captured by the packaged pod-e2e Playwright harness against an isolated pod of this branch (5/5 spec assertions passed, pod torn down cleanly).

Pattern harvest

Rule candidate: when a feature changes what a surface can do, grep the locale catalogs for that surface's capability copy — a static UI hint ("tools are unavailable here") can contradict the new behavior even when all code paths are updated, and only looking at the rendered pixels caught it here.

Rule candidate: vite/rolldown incremental builds can leave BOTH the old and new value of an edited i18n string in one emitted chunk — verify a copy change reached dist by grepping for the OLD string's absence, not the new string's presence, and force a clean build when both appear.

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

Design-level review of b9d14a2e0774b0d6700b40bab10b2d468b0cc750 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: CONCERNS

The gate itself is sound and fail-closed; the disclosed allowedTools bypass turns from latent to live once side chat can fetch untrusted content.

Watch

Auto-approving web_fetch/web_search on a no-approver surface opens an active prompt-injection channel ("Disclosed boundary": a tool in the agent's allowedTools "never raises a permission request, so no host policy sees it") — injected page content → model calls a user-granted write tool → kiro-cli auto-approves it with no host gate, no approver, and a footer now promising "Lookups work here, but changes don't." Under REJECT_ALL the model couldn't fetch fresh attacker content, so "already true" understates what this PR changes. The deferred <agent>--readonly spec with allowedTools: [] is the load-bearing half of the read-only promise, not polish.
Clears when: the derived readonly agent spec (or equivalent hard guarantee that side sessions carry no allowedTools grants) lands in a fast-follow PR, or a human explicitly accepts the interim gap.

Suggestions

• If the follow-up spec lags, soften the footer to not assert "changes don't" — the UI should not promise a guarantee only the tool gate half-holds.

[DESIGN-REVIEWED] b9d14a2

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of b9d14a2e0774b0d6700b40bab10b2d468b0cc750 — 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.

I've verified everything I need: temp-screenshots/ is a PR-template-mandated evidence convention (.github/PULL_REQUEST_TEMPLATE.md:47), AcpEvent.mcp_identity_trusted pre-exists on base (acp/types.py:613, untouched by the patch), sel._infer_source is the canonical surface classifier delegated to by governance_profiles.py:228, and the /btw alias rides the existing /side mechanism (SIDE_RE, FRONTEND_COMMAND_NAMES 3→4). Here is the review.

First-Principles-Verdict: CONCERNS

/btw is a second spelling of /side with no named user who failed to reach /side; the footer now promises more than the policy enforces.

Not justified as shipped

• Item 3 (/btw alias) — duplicate of website/src/pages/chat/ChatInput.tsx (SIDE_RE): the capability is /side, verbatim; the only support offered is "reads naturally at the composer" — inherited preference, no report, misclick, or complaint named.
• Item 8 (MCP identity now reaches the gate for HOOK_BASED callers too) — undeclared: the new mcp_server_name/mcp_tool_name kwargs in _resolve_permission change deny/grant glob matching for every HOOK_BASED stream_and_collect surface (cron, heartbeat), and the description never says so. The harm is real (a canonical-name deny rule silently unmatched), so it stays; declare it.

What this change ships

Inventory (10 items) — 8 justified

Intent: let Side Chat answer lookup questions by running provably read-only tools, plus a /btw shortcut — an ADDITION.

1. Side Chat now runs read-only tools (read built-ins, read-only shell) instead of refusing everything — justified
2. Everything not provably read-only is still rejected, now with policy-state SEL reasons, never a card — justified
3. /btw in composer and slash menu behaves exactly like /side — duplicate of website/src/pages/chat/ChatInput.tsx (SIDE_RE), inherited: no named failing user
4. Side footer hint reworded to "Read-only · Lookups work here…" in en + 12 locales — justified
5. Backend empty-output fallback message reworded to match — justified
6. Dashboard-bound governance profiles now govern side turns; side SEL rows say "dashboard", not "slack" — justified
7. New ToolApprovalPolicy.READ_ONLY, fail-closed without hooks (1 consumer: dashboard/handlers/side.py) — justified
8. HOOK_BASED stream_and_collect callers now hand the gate canonical MCP identity, changing deny/grant matching there — undeclared, rides along
9. Two evidence PNGs under temp-screenshots/ — justified (PR-template convention)
10. Black-baseline pruning + mechanical reformatting of touched files — justified (gate-mandated), rides along

Watch

• Item 3: an alias is a deliberate second spelling — both must stay in lockstep forever (the PR already needs 3 tests and a shared desc key to hold them together). No harm nameable without "reads naturally". Clears when: a report of users failing to find /side is linked, or the alias is dropped.
• Items 1/4: the new copy "changes don't [work here]" is false for any tool in the agent's allowedTools — the description itself concedes kiro-cli auto-approves those with "no host policy sees it" and defers the <agent>--readonly spec. The strengthened user-facing promise ships before the mechanism that would make it true. Clears when: the derived allowedTools: [] side spec ships, or the copy stops claiming changes are impossible.
• Item 8: undeclared matching change on unattended HOOK_BASED surfaces. Clears when: the description names the affected surfaces.

Subtractions

• Drop /btw: the |btw alternative in SIDE_RE (ChatInput.tsx), the FRONTEND_COMMAND_NAMES entry and COMMAND_DESC_KEY row (SlashCommandMenu.tsx), and its 3 tests — /side already is the capability (1 mechanism, 2 spellings after this PR).

[FIRST-PRINCIPLES-REVIEWED] b9d14a2

[github-actions]

UX Review (Fable 5) — 🟡 CONCERNS

UX-level review of b9d14a2e0774b0d6700b40bab10b2d468b0cc750 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Reconciliation complete. The diff's user-visible surface: the /btw slash-menu row (shot-02 — reader: "fairly sure... yes, it says it won't interrupt anything", matching the handler, which rides SIDE_RE into the same side-chat open), the reworded side-composer footer (shot-01 — reader correctly read "can only look things up, not change anything"), and a reworded backend empty-output fallback message that appears in no screenshot. No persistent element changes form or place, so no lens-13 case; the recordings list being empty is fine.

UX-Verdict: CONCERNS

The alias and the read-only footer read perfectly cold; the reworded empty-output fallback is unshown and re-introduces the jargon the footer just shed.

Watch

• Vocabulary drift between the two strings describing the same boundary: the footer says "Lookups work here, but changes don't" (plain, blind-reader-verified), while the new fallback in side.py says "write, execute, and MCP tools are intentionally unavailable here" — unexpanded "MCP" and tool-mechanism vocabulary in a chat bubble a first-timer hits exactly when confused (rare state × mild-friction impact × persistent copy). Fix: reword the fallback to the footer's vocabulary, e.g. "Side Chat can only look things up — to make changes, ask in the main chat."

Evidence gaps

• The changed empty-output fallback message (dashboard/handlers/side.py) appears in no committed screenshot; a shot of a side turn ending with that fallback would close it.

[UX-REVIEWED] b9d14a2

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed b9d14a2e0774b0d6700b40bab10b2d468b0cc750 — this comment is updated in place on each push.

Review details

I've traced the full control flow. Let me verify my read of the candidate against the actual classifier and governance wiring before concluding.

The single candidate claims side chat's READ_ONLY auto-approval of web_fetch/web_search opens an unattended context-exfiltration path.

Working to kill it:

• (a)/(b)/(c) are mechanically plausible (untrusted parent context → injected web_fetch → _is_host_read_only_builtin → auto_approve(read_only=True) → outbound GET), but the decisive question is whether this is a defect in the changed lines versus the intended, governed behavior.
• web_fetch/web_search are deliberately in _HOST_READ_ONLY_BUILTIN_TOOLS, pinned by test_host_read_only_builtins_map_only_to_read_scopes to the network.egress read scope — an explicit, tested classification, not an oversight.
• The PR's sel._infer_source change (side: → dashboard) is precisely the compensating control: side turns now resolve under the same dashboard-surface governance profile as the parent slot, so an operator restricting network.egress binds side turns too (verified by test_dashboard_bound_profile_governs_a_side_turn, which shows a dashboard-bound profile denies web_fetch on side:*).
• This is the same read-only auto-approve semantics the interactive dashboard "Reads" mode and other unattended callers already run under platform-wide; the exfiltration surface is neither introduced nor widened by this PR relative to that design — the guard (network.egress governance) exists and is now wired in.

The candidate is self-rated "low" and concedes it "may be an accepted, governed design decision." I cannot re-derive confidence ≥80 that it is a defect rather than the deliberate feature. Dropped.

Step 2 — I checked the _resolve_permission READ_ONLY ordering (an approved read-only call returns True at the auto-approve branch and never falls through to the trailing read_only_policy reject; withheld/allow results correctly reject), the classifier-only non-shell branch (kind narrows, host-trusted-builtin proves, else allow→reject), and the fail-closed hooks is None path. No additional grounded defect at ≥80.

No findings.

[OPUS-REVIEWED] b9d14a2

Verdict parsed from the review's SHA-scoped output markers for commit b9d14a2e0774b0d6700b40bab10b2d468b0cc750.

False positive or not applicable? A repository writer can comment:
/ai-review override fable b9d14a2e0774b0d6700b40bab10b2d468b0cc750: <one-sentence reason>

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of b9d14a2e0774b0d6700b40bab10b2d468b0cc750 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] b9d14a2

False positive or not applicable? A repository writer can comment:
/ai-review override gpt b9d14a2e0774b0d6700b40bab10b2d468b0cc750: <one-sentence reason>

[bolichen97]

The enforcement here is genuinely well built, and I want to lead with that because my objection is not to the mechanism.

I tried hard to spoof the allowlist and could not, on the normal path. _is_host_read_only_builtin returns False the moment mcp_server_name is non-empty (hooks.py:1321), and kiro-cli stamps mcpServerName for every MCP-served call — so an MCP server naming its write tool fs_read gets a non-empty server name, fails the proof, falls through to allow(), and is rejected by the terminal READ_ONLY clause (llm_helpers.py:2417-2427). readOnlyHint from a manifest is never consulted, a read-looking title is never consulted, and kind can only narrow: hooks.py:1201 refuses a non-read kind even for a host-known read tool. The identity comes from _meta.kiro.toolName stamped by the host (acp/_dispatch.py:1700-1715), not from anything a model authored. Blast radius is genuinely side-chat-only — ToolApprovalPolicy.READ_ONLY has exactly one call site at head (handlers/side.py:459) — and there is no YOLO or trust widening, because side turns call stream_and_collect directly and the READ_ONLY reject runs before on_tool_approval.

Why I am still requesting changes: the invariants that argument rests on are documented as test-pinned, and are not pinned.

1. docs/system-specs/modules/side.md cites eight tests for the "READ_ONLY proof is host-trusted only" row, and all eight are absent at head 21979bf6. I fetched test/test_llm_helpers_tool_gate.py and test/test_hooks.py and could not find test_read_only_policy_rejects_an_mcp_tool_with_a_read_kind, ..._approves_a_host_known_read_tool, ..._rejects_a_host_known_read_tool_under_a_non_read_kind, ..._rejects_a_read_kind_on_a_mutating_tool, ..._rejects_a_read_kind_with_no_host_identity, ..._rejects_a_read_looking_title_alone, test_hook_based_policy_still_approves_a_read_kind_tool, or TestClassifierOnlyHostTrustedProof. test_hooks.py contains zero occurrences of classifier_only. The uncovered cases are exactly the negative paths the safety argument depends on — above all "an MCP tool named fs_read is refused", which is the answer to the spoofing question and is asserted nowhere.

2. hooks.py:1301 says "a test pins that" for the _HOST_READ_ONLY_BUILTIN_TOOLS ↔ read-scope mapping. The constant appears in hooks.py, llm_helpers.py and side.md only — no test file anywhere in the tree. I checked the mapping by hand and it currently holds (all five names resolve to filesystem.read / network.egress in governance.BUILTIN_TOOL_SCOPES), but fs_write or code could join that frozenset with CI green.

3. The body misdescribes the shipped mechanism. It says auto-approval is "is_read_only_bash for shell, the {read, fetch} ACP-kind allowlist otherwise", but hooks.py:1201-1206 explicitly refuses a read kind alone and requires the host-known built-in identity. That describes an earlier revision. On a PR that removes a consent step, the description is what a reviewer audits against.

One real seam worth closing while you are in here: the proof infers "built-in" from the absence of mcpServerName, so a harness that stamps toolName and omits mcpServerName makes an MCP tool called fs_read auto-approve unattended. _dispatch.py:1284 already models exactly this degradation as mcp_identity_trusted=bool(_mcp_server_name and _tool_name), and _is_host_read_only_builtin does not consult it.

Two user-facing items:

4. /btw collides with ordinary prose. SIDE_RE = /^\/(?:side|btw)(?:\s+([\s\S]+))?$/ (ChatInput.tsx:25). "btw" is a common sentence opener, so /btw can you also fix the tests silently reroutes to an ephemeral, unsaved, read-only side turn instead of the main agent — with no confirmation and no undo, and it inherits the auto-approval. First Principles also notes no named user was failing to reach /side.

5. The new footer promise outruns enforcement. "Lookups work here, but changes don't" is affirmative across en.manual.json and 12 locales, but your own disclosed boundary says a tool in the agent's allowedTools is auto-approved by kiro-cli and never reaches this gate. I confirmed there is no compensating check. The hole is pre-existing; the promise that it does not exist is new.

Also worth noting the largest behavioural jump, which the body underplays: bash goes from REJECT_ALL to executing shell unattended in side chat. It is properly bounded — deny-by-default prefix allowlist, redirect/substitution/backgrounding refusal, and the sensitive-path and exfil floors run first, so cat ~/.aws/credentials is still denied — but it deserves to be stated as the headline rather than inferred.

Three lanes are 🟡 CONCERNS on this head (Design, First Principles, UX) with zero author dispositions on the PR, so by the repo's own definition it is not review-ready even with a green rollup. No override was issued by anyone — the /ai-review override strings in the thread are inside the bots' own template footers. Readiness sits at checking for a benign reason: Coverage Combine is still queued.

[pepmach]

Thanks Bolin -- the backend items are in at ce2981a; the two user-facing ones are still open and I say what I intend to do with them below.

1. The cited tests exist now. Every test the "READ_ONLY proof is host-trusted only" row in docs/system-specs/modules/side.md cites is in test/test_llm_helpers_tool_gate.py at this head, one per claim in the row: test_read_only_policy_approves_a_host_known_read_tool, test_read_only_policy_approves_a_read_only_shell_command, test_read_only_policy_does_not_trust_a_read_kind_alone, test_read_only_policy_rejects_an_mcp_tool_with_a_read_kind, test_read_only_policy_rejects_a_read_kind_with_no_host_identity, test_read_only_policy_rejects_a_read_kind_on_a_mutating_tool, test_read_only_policy_rejects_a_host_known_read_tool_under_a_non_read_kind, test_read_only_policy_rejects_a_read_looking_title_alone, plus test_read_only_policy_rejects_a_host_known_name_without_trusted_provenance, test_read_only_policy_refuses_a_write_the_config_grant_matches, test_read_only_policy_refuses_an_app_own_server_grant and test_read_only_policy_classifies_a_read_the_grant_also_matches. The row cites what is there, not what was planned.

2. The mapping is pinned. test_host_read_only_builtins_map_only_to_read_scopes (test/test_hooks.py:984) walks _HOST_READ_ONLY_BUILTIN_TOOLS and asserts every name resolves to a read scope, so the comment at the constant now points at a test that runs.

3. The body describes the shipped mechanism. It now reads: is_read_only_bash for shell; otherwise the host-known read-only built-in identity (_HOST_READ_ONLY_BUILTIN_TOOLS), taken from the non-model-authored _meta.kiro.toolName with no MCP server behind it and only when the event carries the mcp_identity_trusted provenance flag; a read kind or a read-looking title alone is refused. It also says plainly that bash moves from REJECT_ALL to bounded unattended execution under the deny-by-default prefix allowlist, with every other shell call rejected.

The identity seam is closed. HookManager takes mcp_identity_trusted (src/kiro_crew/hooks.py:582) and the built-in proof requires that positive signal rather than inferring built-in from an absent mcpServerName (:1119-1183), the same fact _dispatch.py already models. A harness that stamps toolName and omits mcpServerName therefore gets a refusal, not an auto-approve: test_untrusted_identity_refuses_a_host_known_name (test/test_hooks.py:1014) and test_read_only_policy_rejects_a_host_known_name_without_trusted_provenance pin it.

4. /btw and prose, and 5. the footer promise -- still open. For the alias the options are dropping it or asking for a confirmation before the reroute; for the footer, the copy in en.manual.json and the 12 locales has to say exactly what the gate enforces, including the allowedTools boundary. I will decide both and push before asking for another look, so this head is not the one to re-review.

Head ce2981a. Ran test/test_llm_helpers_tool_gate.py, test/test_side.py, test/test_hooks.py with -n0 (226 passed), plus black, isort, flake8, mypy and the comment-history and subprocess-encoding gates.