refactor: extract shared hardened gh runner for all gh spawn paths (#342)

[bolichen97]

Summary

Closes #342.

Three gh-spawning surfaces (dashboard PR sidebar source_providers, Issue Radar github_client, Code Review Sage discovery/pipeline) each carried their own copy of the hardened-runner pattern — trusted-binary resolution, minimal child env, SEL spawn audit — so every hardening fix had to land three times, and drift had already produced latent bugs:

• D1 (env leak): code_review_sage/sage_lib/pipeline.py::list_open_prs spawned subprocess.run(argv) with no env= at all — the child inherited the gateway's full environment, AWS/Slack/SSH secrets included.
• D2 (audit gap): sage's discovery.run_gh_json / current_login emitted no SEL audit events.
• D3 (allowlist drift): three different gh env passthrough lists.
• D4 (override drift): a SET-but-empty override env var errored in one copy and was silently ignored in the other two.
• D5 (resolver drift): differing candidate filtering, error-noise handling, and caching.

The shared module

New src/kiro_crew/github_runner.py — top-level, no kiro_crew.dashboard.* imports ever (dissolving Issue Radar's and Sage's cross-layer import of the private _validate_provider_executable). Heavy deps (sel, apps.registry.minimal_env, config.loader) stay lazy so Sage's standalone sage_lib import path keeps working.

• validate_provider_executable / provider_executable_candidates + constants: moved verbatim from source_providers as public symbols; source_providers re-exports them under the historical private names (identity-asserted in tests), so gitlab_client's glab resolution and all existing importers are untouched.
• resolve_gh(override_env=, cache=): caller-specific override → generic KIROCREW_GH_BIN → well-known dirs → PATH. Strictest-superset semantics: a SET-but-empty override fails loudly (D4), "path does not exist" noise is filtered from setup messages (D5), win32 refused, cached with reset_cache().
• gh_env(pin_host=): one canonical gh-scoped passthrough union on the minimal_env base (D3), plus GH_PAGER=cat / NO_COLOR=1, with ambient ssh-agent/git-ssh identity stripped (gh api is HTTPS/token auth; the gateway's ssh identity was pure surplus credential surface).
• run_gh(argv, *, timeout, input_text, audit_caller, pin_host): the single sync spawn chokepoint — absolute trusted binary required (no caller can regress to a PATH lookup), env=gh_env(), bounded timeout, SEL audit on ok / non-zero exit / timeout / spawn OSError, tagged per caller so Issue Radar keeps its historical issue_radar.gh_run SEL operation identity. Errors re-raise transparently so each caller keeps its own taxonomy.
• parse_github_repo_url + RepoUrlError: moved from github_client, re-exported there; errors.RepoUrlError aliases the same class so all route-level except clauses keep catching it.

Call-site migration (behavior-preserving)

• source_providers: async _run_json, caching, glab behavior, base env keys, and PATH/GH_HOST pins unchanged; only the validation/candidates source of truth moved, and its gh auth key set now derives from the canonical union (minus the enterprise tokens its github.com-pinned child can never use).
• issue_radar: _gh_bin/_gh_env/_gh_run are thin wrappers keeping GhSetupError(reason=)/GhCliError mapping and _stderr_tail sanitization; _gh_run additionally pins GH_HOST=github.com (Issue Radar is github.com-only by design and its API paths never pass --hostname).
• code_review_sage: gh_bin/gh_env delegate (guarded import preserves the standalone path); run_gh_json/current_login/pipeline.list_open_prs route through run_gh, closing D1 and D2.
• test/test_spawn_audit.py: four benign-spawn exemptions collapse into one github_runner.py::run_gh entry.

Tests

New test/test_github_runner.py (resolver precedence/caching/fail-loud override, exact env-set assertions with a polluted gateway env, SEL audit on ok/exit/timeout/OSError, pin-host semantics, re-export identity seams). D1 lock-in test added to sage's TestListOpenPrs asserting the spawn now carries the minimal env. Existing suites pass; monkeypatch targets that pinned the old private import locations were retargeted to the runner's public names.

Pre-push review fleet

• GPT 5.6 Sol: 1 Medium (spawn OSError escaped unaudited) — fixed (audited with coarse exception class, then re-raised).
• Opus 5: 0 blocking, 4 advisories — all applied (enterprise-token exclusion for the pinned sidebar child, audit-loss logged at warning, pin_host wired for Issue Radar, fixture delenv + stale comment cleanup).

Verification

isort / flake8 / mypy clean; full python -m pytest run: 39,517 passed — remaining failures reproduce identically on unmodified main in this environment (host has nobody-owned /tmp ancestry and nobody-owned /usr/bin/gh, which the ownership validation correctly refuses; CI runners are unaffected). Touched-area suites (sage, issue_radar, source_providers, spawn audit, env consolidation): green. scripts/check_brand_name.py and scripts/docs-lint.sh pass. Docs updated in the same commit (issue-radar.md, learn-cron-dashboard.md).

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

This is a behavior-preserving refactor extracting three duplicated gh-runner copies into a shared github_runner.py module. I've verified the key security-sensitive paths:

• resolve_gh preserves validation semantics (all OSError still converted to ValueError inside validate_provider_executable, override still fails loud, cache keyed on override name+values so Issue Radar/Sage don't cross-contaminate).
• run_gh refuses non-absolute argv[0]; all callers pass an absolute resolved path.
• Audit-or-deny is preserved/strengthened (critical invoked before spawn; outcome events best-effort).
• gh_env strips ambient ssh identity and passes only gh-scoped keys; sidebar's widened _PROVIDER_AUTH_ENV_KEYS["gh"] still pins GH_HOST=github.com afterward, and pin_host covers Issue Radar's bare API paths.
• No blocking syscall newly introduced on the event loop — subprocess.run existed in each caller before.

No residual-class defect (no reachable injection/traversal/auth-bypass/credential-exposure, no added crash/data-loss, no removed guard without replacement) and no blocking: true AUTOSDE rule violation on changed lines.

No findings.

[OPUS-REVIEWED] 7bd7a78

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

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

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 7bd7a78

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

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

Root-cause consolidation of a real, drift-caused secret leak; right layer (top-level module dissolving the apps→dashboard private import), fully backward-compatible re-exports.

[DESIGN-REVIEWED] 7bd7a78

[bolichen97]

Addressed both failing checks in 0f28835012313c078cdd8a36c3a04081689eb91a:

GPT 5.6 blocking finding (sage tests writing real SEL): valid — while KIROCREW_HOME is isolated per test by _isolate_app_home, config_dir() caches the resolved home at its first call in the process, so a pre-isolation import could route the new run_gh SEL audit through the cached REAL data dir. Fixed at the suite level: a new autouse _mute_shared_runner_audit fixture in code_review_sage/tests/conftest.py no-ops github_runner._audit_run for the whole suite (the audit contract itself is covered in test/test_github_runner.py against a mocked SEL), which is deterministic regardless of cache state.

Cross-Platform Portability: the one in-scope added line was a "/usr/bin/gh" literal in a sage test patch — replaced with a neutral "/resolved-gh/gh" fixture path.

Design Review watch item (docstring overstating run_gh as the chokepoint for every gh spawn while the sidebar keeps its own async sandboxed spawn): docstrings reworded — the module single-sources the trust POLICY for all four surfaces; spawning is shared for the sync app-side callers, and the sandbox rationale now notes standard-mode routing as a viable follow-up rather than a constraint.

[bolichen97]

GPT 5.6 blocking finding on 0f28835 (SEL audit could fail open) addressed in 7bd7a78:

run_gh now follows the same audit-or-deny contract as the sidebar's _run_json: a fail-closed invoked event (critical=True, synchronous) is written BEFORE the spawn — if SEL storage is unwritable/full, the gh call is refused with SetupError("gh spawn audit unavailable — refusing to run gh unaudited") and the child never runs. Outcome events (ok / non-zero exit / timeout / spawn OSError) remain best-effort with a warning on write failure, matching the sidebar's semantics where the terminal events are best-effort once invoked has landed.

Caller mapping: Issue Radar surfaces the refusal as a retryable GhCliError (transient host problem, not a connect-dialog setup issue); Sage maps it to GhError. Tests updated: invoked-before-spawn ordering, refusal-with-no-spawn when SEL is down, and outcome-write failure not breaking a completed call.