fix(hooks): keep the tool-approval import chain free of the cryptography wheel

[bolichen97]

Summary

hooks.on_tool_call — the verdict path for every ACP tool call — lazily imports kiro_crew.slack.gateway for _is_read_only_tool. At import time that module reaches wecom.media and weixin.media (via channels) and secrets.vault (via autonudge → irq → cron_script), and all three imported cryptography.hazmat at module top for their AES routines. On a host whose cryptography native wheel does not load (a platform-mismatched build — the AL2 x86_64 wheel on an Apple-silicon Mac, dlopen: slice is not valid mach-o file) every tool approval raised ImportError, not just a WeCom media download. Found because the Amazon edition's end-to-end drafter tests, which run the real approval chain, failed on every Mac dev host.

Change

The three imports move inside the functions that use them (wecom.media.decrypt_media, weixin.media.decrypt_aes_ecb, SecretVault._encrypt_entry / _decrypt_entry). The dependency did not go away; it moved to the only paths that pay for it.

Tests

test/test_approval_chain_no_cryptography.py, in a subprocess with cryptography made unimportable via a meta_path finder:

• the approval hook's import chain (slack.gateway, channels, both media modules, secrets.vault) loads and _is_read_only_tool("Read") answers;
• each decrypt path still fails clearly with an ImportError naming cryptography;
• a static AST check over wecom/, weixin/, slack/, secrets/ and channels.py refuses a top-level import from growing back (auth/store.py is not on the chain and keeps its).

Existing test_wecom_media.py / test_secret*.py: 202 pass. flake8, isort, black gate clean.

Pattern harvest

• A lazily-imported module inherits the import-time failure modes of everything IT imports at top level. on_tool_call was careful to import slack.gateway lazily, but the transitive top-level imports made the security path depend on a native wheel that had nothing to do with approving a tool. When a hot or security-critical path imports a large module lazily, the property to protect is "that module's import chain has no optional/native dependency at module scope" — and it is cheap to pin with a subprocess that blocks the dependency.
• Move a heavy import to the function that uses it, not to a try/except at module top. A guarded top-level import (try: import cryptography except ImportError: cryptography = None) makes every caller check for None; a function-scope import keeps the failure exactly where the capability is exercised and the error message names the missing piece.

Rule candidate: a module imported lazily on a security-critical or hot path must itself have no optional/native dependency at module scope -- pin it with a subprocess test that blocks the dependency and imports the chain.
Not generalizable: which specific modules sat on this chain (wecom/weixin media, secrets.vault) is repository history, not a rule.

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] e6b3994

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

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

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

Fixes the cryptography instance but keeps the structural cause: the approval verdict still imports the entire channel tree to reach a string heuristic.

Watch

• Root cause vs symptom: _is_read_only_tool is a pure tool-name verb heuristic, yet it lives in slack/gateway.py, so hooks.on_tool_call's lazy from kiro_crew.slack.gateway import _is_read_only_tool still drags channels → every gateway → aiohttp and friends onto the security path. The next platform-mismatched native wheel or optional import anywhere under that tree reproduces "every tool approval raises ImportError", and the new guard won't see it — both the subprocess test and the AST ratchet block only cryptography, exactly the "which modules sat on this chain is repository history, not a rule" limitation the description itself names.

Suggestions

• Move _is_read_only_tool into hooks.py or a dependency-free helper (slack.gateway imports it from there): the approval path then imports nothing channel-shaped, retiring the failure class instead of one dependency. The three lazy imports and the vault chain pin stay valuable regardless.

[DESIGN-REVIEWED] e6b3994

[github-actions]

GPT 5.6 Review — 🔴 changes requested (blocking)

GPT 5.6 found at least one blocking issue that must be resolved before merging e6b39947e65135766f8a7a4c7dea09f9d6373818. 1 of 1 blocking finding(s) are security-class and were withheld from adjudication, so the blocking verdict stands.

This comment is updated in place on each push.

BLOCKING -- test/test_approval_chain_no_cryptography.py:46 -- subprocess writes bytecode outside test isolation
return subprocess.run(
Clean checkout -> _run() imports uncached source modules -> __pycache__ files persist in the checkout.
Anchor: no-test-side-effects
Fix: set PYTHONDONTWRITEBYTECODE=1 in the subprocess environment.

FINDING -- src/kiro_crew/wecom/media.py:124 -- function-local from cryptography... imports here, in weixin/media.py, and secrets/vault.py hide dependency failure until live crypto operations -> Fix: use guarded module-level imports with explicit availability checks.

[GPT-REVIEWED] e6b3994
[BLOCK-MERGE] e6b3994

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

Adjudicable block is empty (0 findings). One fenced finding, F1.

F1 — test/test_approval_chain_no_cryptography.py:46, "subprocess writes bytecode outside test isolation."

• Condition: subprocess.run([sys.executable, "-c", ...]) at line 46 sets no PYTHONDONTWRITEBYTECODE and inherits the parent cwd (repo root); importing the named source modules writes .pyc into their __pycache__.
• Containment / recovery: __pycache__/ is declared in .gitignore; the checkout residue guard scans only immediate children of the repo root (conftest.py:2444 _REPO_ROOT.iterdir()) and defers to git check-ignore (conftest.py:2452-2488), so a nested, gitignored __pycache__ is never reported as residue. The parent pytest process imports these same kiro_crew modules during a normal run and produces the identical artifacts regardless of this test.
• Harm: LOW — a gitignored, guard-invisible build artifact inside the checkout, not the operator's home/tempfile base/~/.kiro. Evidence record is complete and the "side effect" is one a human plausibly accepts.

Verdict: FLAG — the finding is real but its residual risk (gitignored bytecode the residue guard cannot even see, produced by any Python run) is one a repository writer would plausibly accept; the trivial PYTHONDONTWRITEBYTECODE=1 fix remains available but need not block.

[ADJUDICATION] e6b39947e65135766f8a7a4c7dea09f9d6373818 total=0 uphold=0 downgrade=0
[GPT-ADJUDICATED] e6b39947e65135766f8a7a4c7dea09f9d6373818

[ADJUDICATION-FENCED] e6b39947e65135766f8a7a4c7dea09f9d6373818 fenced=1 flagged=1
FLAG F1 test/test_approval_chain_no_cryptography.py:46 -- The subprocess only writes gitignored `.pyc` into `__pycache__`, which the checkout residue guard (conftest.py:2444,2452-2488) never scans and which any normal pytest run already creates, so the residual risk is one a human writer plausibly accepts.
[GPT-ADJUDICATED-FENCED] e6b39947e65135766f8a7a4c7dea09f9d6373818

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

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

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

First-Principles-Verdict: CONCERNS

The fix is real but stops one level short: the approval verdict still imports the entire channel tree to run a 30-line string check that lives in slack/gateway.py for no load-bearing reason.

What this change ships

Intent: make every tool approval survive a host where the cryptography native wheel cannot load — a FIX.

1. Tool approvals no longer die with ImportError on a wheel-mismatched host — justified (reported defect)
2. WeCom/Weixin media decrypt and vault encrypt/decrypt now fail at call time, naming cryptography — justified, same fix
3. Subprocess regression test importing the whole approval chain with cryptography blocked — justified
4. Static AST ratchet over five hand-listed dirs — duplicate of the subprocess test (see Subtractions)
5. jsonl linear-scan test rewritten to trace-based — already on main (test: pin the reader's linear scan by trace, not wall-clock ratio (#8606) #8814); stale diff base, not this PR's delta
6. kiro-light white-canvas theme + hooks test + contract doc — already on main (feat(theme): white chat canvas with grey shell in kiro-light #8711); same
7. Per-chat Terminal menu entry + 12 locale strings — already on main (feat(side-panel): restore per-chat Terminal in the + menu #8703); same
8. Eight screenshots under temp-screenshots/ — repo convention (953 existing files), belongs to items 6–7

Watch

• Mechanism, not cause. The defect exists because _is_read_only_tool (slack/gateway.py:573 — pure re plus two tuples) lives inside the Slack gateway, so hooks.py:1058 must drag slack.gateway → channels → every channel gateway → wecom/weixin media, autonudge → irq → cron_script → secrets.vault into every approval. The PR's own harvest names the property ("the chain has no optional/native dependency at module scope") but pins it for exactly one dependency. Moving the function plus _READ_ONLY_TOOL_PREFIXES/_WRITE_INDICATORS into hooks.py is in scope and cycle-free — slack.gateway already imports hooks at module top (hooks.py:1046) — and removes the whole class of failure, not one instance.
• Items 5–7 are three undeclared features inside a fix(hooks) diff. Their subjects match merged main commits exactly, so I read them as a stale diff base rather than riders; if any were not already landed, this is a feature PR wearing a fix title.

Subtractions

• Drop test_no_module_on_the_channel_chain_imports_cryptography_at_top_level (test/test_approval_chain_no_cryptography.py): test_the_approval_hook_import_chain_loads_without_cryptography already fails on any top-level cryptography import reachable from the real chain — the AST scan is a second, weaker spelling (literal-name only, hand-listed dirs, needs the auth/store.py exemption) of the same property.
• Prefer relocating _is_read_only_tool into hooks.py over the three-file lazy-import pattern: it deletes the approval-path justification for all three function-scope imports and the ratchet with it (the wecom/weixin lazy imports may stay on import-cost grounds alone).

[FIRST-PRINCIPLES-REVIEWED] e6b3994

[github-actions]

UX Review (Fable 5) — ✅ PASS

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

All evidence reviewed: the diff, all six theme screenshots, both terminal-menu screenshots, and the surrounding SidePanel/theming code. Emitting the review.

UX-Verdict: PASS

White-canvas swap and per-chat Terminal are coherent, screenshot-proven, and every invisible-surface edge case (bubble, capsules, edit box, pinned prompt, steer) is handled and pinned.

Suggestions

• Two controls now share the bare label "Terminal" with different scopes (nav-rail app-wide dock vs per-chat tab); the disambiguating string menu_terminal_desc ("Shell on the gateway host, scoped to this chat") renders only in the empty-state launcher — add it as title on the compact + menu's Terminal DropdownMenuItem so hover disambiguates without restyling the menu.

[UX-REVIEWED] e6b3994

[bolichen97]

Folded into #8799 (commits cherry-picked verbatim: fix(hooks): keep the tool-approval import chain free of the cryptography wheel + its test fix) so the insider cherry-pick is a single PR. Closing without merge.