fix(terminal): complete subcommands for tools the user installed

[dwu96]

Problem

Terminal subcommand completion works for git and for nothing else on macOS. Typing gh opens no menu; git opens a full one. Same for docker, kubectl, and every other tool in _KNOWN.

Why it matters

The command tier exists to answer "which subcommand did you mean" for exactly these CLIs. On macOS — the platform most KiroCrew users are on — it completed only the handful of tools shipped in /usr/bin, so the feature reads as broken rather than as scoped. git looked like it worked; it was the accident of /usr/bin/git existing.

Fix (symptom → root cause → change)

Symptom: gh produces no entries, git produces 150.

Root cause: _is_trusted_dir required every component of the resolved chain — and _resolve required the binary itself — to be owned by uid 0. Homebrew installs into a prefix owned by the installing user: /opt/homebrew is uid 503, /opt/homebrew/bin is drwxrwxr-x group admin, and the real binary sits in /opt/homebrew/Cellar/gh/<v>/bin/gh, also user-owned. So _sanitized_path dropped the entry, and even /usr/local/bin/gh (a root-owned symlink) failed at the target check. /usr/bin/git was the only allowlisted tool that passed.

Change: one predicate, _trusted_owner, applied to every chain component and to the resolved binary:

• accept uid 0 or the uid this gateway runs as;
• refuse world-writable, always;
• refuse group-writable unless the gid is an administrator group (_ADMIN_WRITE_GIDS = gid 80 admin on macOS only, whose members can already sudo; no gid qualifies on Linux, where root/wheel membership does not imply sudo rights);
• a root gateway gets no group-write exemption at all — its children are root too, so a merely-admin account could otherwise substitute a binary that executes with root privileges. For euid 0 (and for a host with no uids at all) the predicate reduces exactly to the rule it replaces: st_uid == 0 and not (st_mode & (S_IWGRP | S_IWOTH)).

Ownership alone cannot see one important case: a checkout's own bin/ or scripts/. Those are owned by the same user, match no _PROJECT_LOCAL_SEGMENTS name, and a .envrc or dev shell commonly prepends them to PATH — so they would win resolution over the genuine tool. PATH entries and resolved targets inside KIROCREW_PROJECT_DIR or workspace_root() are therefore refused by location (_under_agent_writable_root), mirroring the provider-CLI check in handlers/source_providers.py, which already took this same ownership trade for the same reason. That lookup fails closed: if the workspace lookup raises or a root cannot be canonicalized, the root set is None and every entry is refused (no completions, one warning) rather than the unresolved root being silently dropped.

The trade, stated plainly

The agent shares the gateway's uid, so a directory this filter now keeps is a directory the agent could plant a binary in. Three things bound that:

1. _KNOWN is a closed allowlist — a plant must shadow a specific real tool name and win PATH order against the genuine install;
2. the plant does not choose the argv (the protocol fixes it: __complete / --list-cmds);
3. an agent that can write files already holds far more reliable execution paths on the same machine — ~/.zshrc, a git hook, a LaunchAgent — that no PATH filter touches, so refusing the user's own install buys no real containment.

The trees the agent most plausibly writes are refused by location regardless. There is still no operator opt-in to widen this further.

docs/system-specs/modules/learn-cron-dashboard.md is updated in the same commit, since it documented the rule this replaces sentence by sentence.

Tests

test/test_terminal_commands.py, +12 cases (153 pass in the file):

Test	Locks in
test_admits_an_install_owned_by_the_gateways_own_user	_resolve returns a user-owned install (the reported bug)
test_ownership_accepts_root_and_this_user_but_no_one_else	a third account's install is still refused
test_ownership_rejects_world_writable_whoever_owns_it	world-writable never qualifies
test_ownership_rejects_group_writable_for_an_ordinary_group	a non-admin group's write bit disqualifies
test_ownership_accepts_group_writable_only_for_an_administrator_group	the gid-80 exemption is macOS-only
test_a_root_gateway_gets_no_administrator_group_exemption	euid 0 reduces to the old predicate
test_ownership_on_a_host_without_uids_keeps_the_administrator_branch	Windows/None behaviour, incl. no exemption
test_the_whole_chain_must_be_trusted_not_just_the_leaf	a hostile ancestor still refuses
test_refuses_a_third_partys_file_inside_a_trusted_directory	directory trust does not transfer to contents
test_drops_path_entries_inside_the_agent_writable_project	<repo>/bin on PATH is dropped
test_refuses_a_binary_whose_target_is_inside_the_agent_writable_project	the symlink form of the same
test_a_sibling_of_a_root_is_not_inside_it	…/workspace-other is outside …/workspace
test_an_undetermined_root_set_refuses_every_entry, test_a_failing_workspace_lookup_yields_no_roots_rather_than_an_empty_set, test_an_uncanonicalizable_root_yields_no_roots	the root lookup fails closed, not open

Ownership tests feed fabricated os.stat results (_stat_chain, _fake_st) rather than real temp paths on purpose: the chain above a temp dir is world-writable /tmp on Linux and a per-user dir on macOS, so asserting against it would test the host instead of the predicate.

Manual verification

Reproduced the original diagnosis on this host before changing anything: /opt/homebrew/bin/gh → /opt/homebrew/Cellar/gh/2.96.0/bin/gh is uid 503 (refused by the old rule at both the directory and the file check), /usr/local/bin/gh is a root-owned symlink whose target is the same user-owned file (refused at the target check), /usr/bin/git is uid 0 (the one that passed).

Screenshots

N/A — no user-visible UI change. The rendered menu is unchanged; only which binaries are eligible to populate it.

Local gates

pytest (full suite, on the pre-rebase commit): 39,730 passed. Three failures, none from this diff — two reproduce on clean origin/main on this macOS host (test_the_environment_is_an_allowlist_not_a_filtered_inherit, which sees an OS-injected __CF_USER_TEXT_ENCODING; test_cron_cancel::test_run_command_sandboxed_can_be_cancelled_mid_run), and test_dev_fleet_app::test_toctou_clean_unmerged_force_omits_git_force passes in isolation and as a whole file on this branch (a pre-existing ordering flake under parallel full-suite runs). isort and flake8 clean. mypy reports only three pre-existing hooks.py xattr errors, which are macOS-typeshed artifacts (os.listxattr/getxattr/setxattr are Linux-only in typeshed) and do not appear on CI's Linux runners; the changed files are clean.

[github-actions]

Design Review (Fable 5) — ✅ PASS

Advisory design-level review of a0b774d14f84730a3be0a21e1e7a496a8b450f6e — updated in place on each push; does not block merge.

Design-Verdict: PASS

A dead-on-macOS feature fixed at its root cause (the uid-0-only chain rule), following the precedent validate_provider_executable already set, fail-closed and fully reversible.

The widened trust predicate is proportionate to this project's single-user trust boundary: the residual risk (same-uid agent planting a shadow binary) is honestly argued and bounded by the closed allowlist, the location-based refusal of agent-writable trees, and the strict sandbox — and the rejected alternative (operator trusted-dir opt-in) is documented with its reason. Error paths on the new workspace-root lookup fail closed rather than open, the spec doc is updated in the same commit, and no config surface, schema, or one-way door is introduced. The one thing to know (not a blocker): _trusted_owner and github_runner.check_provider_path_component are now two deliberately-divergent provenance policies (group-write and sticky-dir handling differ); the cross-referencing docstrings are what keep that from silently drifting.

[DESIGN-REVIEWED] a0b774d

[github-actions]

GPT 5.6 Review — ✅ human override accepted

Human judgment by @dwu96 overrides the GPT 5.6 finding for a0b774d14f84730a3be0a21e1e7a496a8b450f6e; the recorded reason is authoritative for this commit.

This comment is updated in place on each push.

The model was not re-run because an authorized human decision supersedes it.

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

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

This is a well-contained, thoroughly-documented security-hardening change to the terminal command-completion PATH sanitizer. Let me verify the core logic paths for fail-closed behavior.

Reviewing the changed logic:

• _trusted_owner: world-writable → False; group-writable admitted only when gateway is a non-root user and the group is an admin group; then requires st_uid == 0 or st_uid == own_uid. All branches match the tests and are fail-safe.
• _sanitized_path: computes roots once, drops entries via not _within(p, roots) and _is_trusted_dir(p). When roots is None, _within returns True → entry dropped → fails closed.
• _resolve: _under_agent_writable_root(real) → refuses when roots unresolvable; ownership re-checked on the resolved target. Fails closed.
• _agent_writable_roots: broad except returns None sentinel (refuse-all), never an empty tuple (admit-all). Correct fail-closed direction, verified by tests.

The change loosens the prior "uid 0 only" chain rule to also accept the gateway user's own uid — but this is a deliberate, reasoned relaxation with a compensating location-based guard (_under_agent_writable_root excludes the project/workspace trees the agent actually writes), mirroring the existing github_runner.validate_provider_executable policy. Not an unguarded removal.

The lazy workspace_root import inside _agent_writable_roots is in-function (advisory top-level-imports, blocking:false), but it is deliberately deferred so an import regression degrades to "no completions" rather than a per-keystroke HTTP 500 — the same fault-isolation the surrounding docstring argues for. Moving it to module top would defeat that documented intent, so no actionable finding.

No credential-path, injection, crash, or fail-open regression on any changed line.

No findings.

[OPUS-REVIEWED] a0b774d

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

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

[dwu96]

Dispositions for bc793a2acaf559713d86dfcab11abd4b35f23ab3.

GPT 5.6 — BLOCKING, terminal_commands.py:426 — "user-writable PATH entries permit completion-triggered code execution; restore the uid-0-only requirement"

Escalated to a human maintainer — not fixed, not rebutted. The finding is factually correct and it is not a defect in the implementation: it is the PR's stated trade, and the prescribed fix is to revert the change. That is a posture decision a bot should not settle in either direction, and Design Review independently asked for exactly this ("a human should ratify that boundary shift, not a bot").

What is on the table, stated so a maintainer can rule on it rather than reconstruct it:

• The exploit the finding names is real and narrow. An agent-planted ~/.local/bin/gh shadowing a _KNOWN name gets keystroke-triggered execution with no approval gate. It must both shadow a specific real tool name and win PATH order.
• The equivalence argument is why this PR judges the trade acceptable. An agent that can write files already holds ~/.zshrc, a git hook, and a LaunchAgent — none of which any PATH filter touches, and none of which are on security._SENSITIVE_HOME_DIRS. If that argument is accepted, the marginal containment bought by refusing the user's own Homebrew install is ~zero.
• The repo already took this exact trade in a sibling. handlers/source_providers.py::_validate_provider_executable accepts st_uid in (0, uid), refuses other accounts and world-writable paths, and refuses the agent-writable trees — on the explicit grounds that requiring a root-owned copy "made every stock brew install gh fail". This PR is that policy applied to the completion tier, plus the same _agent_writable_roots exclusion.
• The cost of the status quo. On macOS the tier completes only tools in /usr/bin. In practice that is git and nothing else — gh, docker, kubectl never complete, which is the reported bug.
• If the maintainer declines the shift, the narrower option that still fixes the bug is to trust only recognised platform package-manager prefixes (/opt/homebrew, /usr/local/Homebrew, /home/linuxbrew/.linuxbrew) when they are owned by the gateway's user, leaving ~/.local/bin, ~/go/bin and npm-global refused. That kills the ~/.local/bin/gh scenario by construction while still completing Homebrew tools. Say the word and I will cut it that way instead.

No code change is being made in response to this finding, because "restore uid-0-only" is a revert and the narrower alternative is a different design that needs the same human sign-off.

Design Review — 🟡 CONCERNS, watch item 1 (same boundary shift)

Escalated, same as above — this is the ratification you asked for, requested explicitly rather than inherited silently.

Design Review — 🟡 CONCERNS, watch item 2 — "the tier now hinges on _agent_writable_roots() never failing; one broken import or uncanonicalizable root silently kills all command completion (one log warning)"

Partly rebutted, partly accepted-and-deferred.

Rebutted: the failure cannot present as "completions mysteriously stopped". workspace_root() has 9 call sites across 7 modules and sits on session and task creation, so a persistent failure takes most of the gateway with it — completions would be the least visible symptom, not the only one. The transient case (an OSError on one canonicalization) costs one negative-cache TTL, not a lasting outage.

Accepted-and-deferred: there is a genuine observability gap, and it is not the one named. The audit vocabulary cannot distinguish refused because the root set was unavailable from the ordinary cmd_unknown, so an operator reading the SEL trail cannot tell a fail-closed refusal from "that tool isn't installed". Closing it needs a new reason word in handlers/terminal.py's vocabulary plus the docs table that enumerates it — a separate diff from this one, and it should not ride along inside a security-posture change. Filed as follow-up; the fail-closed direction and the logger.warning stay as the interim signal.

Opus 4.8 — ✅ no findings

Nothing to answer.

[dwu96]

/ai-review override gpt a0b774d: This applies the ownership policy already on main for provider CLIs (github_runner.validate_provider_executable, which accepts st_uid in (0, uid) and refuses the agent-writable trees) to the completion tier, with the same refusals plus a stricter root-gateway rule.

Human ratification of the boundary shift, as Design Review asked for. The override is not a claim that the finding is factually wrong — the ~/.local/bin/gh scenario is real — it is a decision that this PR is not the place the policy gets decided, because the policy is already merged.

What is already on main

src/kiro_crew/github_runner.py — the single source of the provider-CLI trust policy, re-exported by dashboard/handlers/source_providers.py (line 46):

• :183 — if path_stat.st_uid not in (0, uid): raise ValueError(...). The relaxed default accepts a binary and every ancestor owned by root or the gateway's own uid. That is the exact predicate this PR adopts.
• :186-191 — world-writable refused (a sticky directory tolerated only because the ownership check still decides). This PR is stricter: no sticky tolerance, because a PATH entry has no legitimate reason to sit under a world-writable tree.
• :194-208 — the rationale, verbatim: "Default policy — if gh works in your terminal, it works here… including the ordinary user-owned Homebrew/Linuxbrew/asdf installs: requiring a root-owned copy made every stock brew install gh fail and pushed users into a sudo cp ritual for a CLI they had already installed and authenticated." Same platform fact, same conclusion.
• :142 / :249 — agent_writable_roots(), refusing anything inside the project checkout or workspace root as "the one substitution vector the model itself controls". This PR's _under_agent_writable_root is that check, applied to PATH entries and to resolved symlink targets.
• :229 — a root gateway is refused outright there. This PR does not refuse the tier for a root gateway (that would remove git completion which root-run containers have today), but it does deny root the group-write exemption, so for euid 0 the predicate reduces exactly to the rule being replaced: st_uid == 0 and not (st_mode & (S_IWGRP | S_IWOTH)).

Provenance: introduced by #630 ("fix: accept the user's own gh/glab CLI install"), extracted to the shared module by #2407 ("refactor: extract shared hardened gh runner for all gh spawn paths").

It is also already written down as accepted policy in the module spec — docs/system-specs/modules/learn-cron-dashboard.md:477: "the gateway user's OWN install is accepted — Homebrew/Linuxbrew/asdf symlink layouts included — and only provenance the user did not choose is refused… and anything inside the agent-writable project checkout or workspace root (github_runner.agent_writable_roots(), the one substitution vector the model controls)… so provenance was traded for containment."

Why the completion tier is not a weaker place to apply it

The provider path runs an authenticated gh/glab with credentials and network reach. This tier runs <tool> __complete under sandboxed_spawn_argv(..., "strict") — every credential directory hidden, an allowlisted env with no HOME, stdin /dev/null, stdout capped, a 2s tree-killing timeout — to read a subcommand table compiled into the binary. If accepting a user-owned install is right for the credentialed path, it is not wrong for the uncredentialed one.

Two further bounds the provider path does not need: _KNOWN is a closed allowlist, so a plant must shadow a specific real tool name and win PATH order against the genuine install; and the argv is fixed by the protocol rather than by the plant.

The residual, accepted explicitly

An agent-planted ~/.local/bin/gh shadowing a _KNOWN name would be executed by a keystroke. That is accepted on the same ground main already accepted it: an agent that can write files holds ~/.zshrc, a git hook and a LaunchAgent — none on security._SENSITIVE_HOME_DIRS, none covered by any denied-command rule, and none reachable by any PATH filter — so refusing the user's own Homebrew install buys no real containment while costing every gh/docker/kubectl completion on macOS.

This revision (a0b774d14) also corrects the in-code and spec citations to point at github_runner rather than the re-exporting source_providers.py, which is where that policy lived before #2407.

[github-actions]

Human judgment recorded

@dwu96 marked the gpt AI finding as false positive, not applicable, or explicitly accepted for a0b774d14f84730a3be0a21e1e7a496a8b450f6e.

This applies the ownership policy already on main for provider CLIs (github_runner.validate_provider_executable, which accepts st_uid in (0, uid) and refuses the agent-writable trees) to the completion tier, with the same refusals plus a stricter root-gateway rule.

This decision applies only to this commit. A new push requires a new judgment.

[dwu96]

Final disposition — everything raised on this PR is now answered.

Design Review — ✅ PASS, one non-blocking note: "_trusted_owner and github_runner.check_provider_path_component are now two deliberately-divergent provenance policies (group-write and sticky-dir handling differ); the cross-referencing docstrings are what keep that from silently drifting"

Accepted as stated — the divergence is intentional in both directions, and I am not unifying them in this PR. Recording where they differ and why, so a future reader does not read either as an oversight:

• Sticky world-writable directories. github_runner tolerates one (/tmp-style 1777) because the owner check still decides who may replace an entry, and a provider CLI can legitimately be reached through such a prefix. This tier refuses it outright: a PATH entry has no legitimate reason to live under a world-writable tree, so the tolerance would buy nothing and cost a real hole. This tier is stricter.
• Group-write. github_runner refuses group-writable unconditionally; this tier admits it for an administrator group (macOS admin, gid 80) when the gateway is not root, because that is the mode Homebrew leaves on its own bin and an admin-group member can already sudo. This tier is looser, in exactly one narrow place, and it reduces to the stricter rule when euid is 0.
• Root gateway. github_runner refuses provider execution outright; this tier keeps running (a root-run container would otherwise lose the git completion it has today) but grants root no group-write exemption, so for euid 0 the predicate is exactly the pre-change rule.

Unifying them into one shared predicate would have to pick one behaviour per axis and would therefore change one of the two call sites — a credentialed CLI spawn and an uncredentialed keystroke probe with different blast radii. That is a refactor with its own risk, not a tidy-up, and it does not belong in a security-posture change. The docstring cross-references (_sanitized_path, _under_agent_writable_root) are the drift guard, as noted; the module spec now names both policies too.

Opus 4.8 — ✅ no findings; the one observation it raised (the lazy workspace_root import) it also resolved as non-actionable

Nothing to answer: it verified the deferred import is deliberate — an import regression degrades to "no completions" instead of a per-keystroke HTTP 500 — which is what the surrounding docstring argues for. No change made.

GPT 5.6 — ✅ human override accepted (@dwu96)

Boundary decision ratified by a repo writer with the citations in the override comment; not a code change. Recorded here for the reader: the ownership predicate this PR adopts is the one github_runner.validate_provider_executable already applies on main.

Nothing else is open. Automated validation is green (PR Readiness SUCCESS, 51 checks, 0 failing); this PR now waits only on human review.