refactor: migrate inline owner-denial sequences to shared helper

[bolichen97]

Summary

Closes #6224.

Migrates the remaining inline owner-denial sequences (predicate → off-thread SEL
audit → stale-session 401 relabel → 403 owner_only) to a two-tier shared
helper in _shared.py, adds owner gates to 4 previously-unguarded mutating
routes, and introduces a registrar-walk invariant test that catches future
ungated routes without filtering by handler module.

Changes

Shared helpers (src/kiro_crew/dashboard/handlers/_shared.py)

• require_owner_dashboard_request(request, operation) — full async lifecycle:
  predicate → off-thread SEL audit → stale-session 401 relabel → 403
• _owner_denial_response(request, error_message, error_code) — the sync tail
  for domain wrappers that keep their own audit and/or error code

Inline sequences migrated

Two tiers, and which one a site gets is decided by whether its audit record or its
wire body is domain-specific — not by preference:

Full delegation (generic non_owner_block audit, standard owner_only body):

• handlers/agents.py — _require_owner
• handlers/mcp_apps.py

Tail only (_owner_denial_response, so the module keeps its own audit record
and its own error code — delegating these would have changed either the SEL
payload or the wire body):

• handlers/ask_question.py — keeps resources="/api/ask-question" + its error
  reason, and its {"error": "forbidden", "code": "owner_only"} body
• chat_handlers.py — keeps error="not the dashboard owner" and its forbidden
  body (now additionally coded owner_only)
• handlers_instances.py — both sites (api_instances_search_sessions and its
  same-file sibling api_instances_proxy) keep _audit(...), which emits
  log_tool_invocation under instances_* / dashboard:instances — the stream this
  module's SEL consumers watch — and keep their own messages ("federated session
  search is owner-only" / "remote-crew proxy is owner-only")
• handlers/aws_consent.py, handlers/messaging.py, handlers_cloud.py,
  apps/builtins/aws_control/backend/routes.py — each keeps its own audit

One site deliberately NOT migrated

source_providers._authorize_owner_request keeps its inline tail. It is not the
same four-step shape: it has four separate denial branches and only the
caller != owner_id one reaches the stale relabel, its body is a bare
{"error": "forbidden"} with no code, and that exact body is pinned by equality
assertions in two test files. Routing it through _shared would also have the
module that DEFINES stale_owner_session_response call back into the module that
imports it. Adding a code there is a wire change that belongs in the error-code
track, not in this consolidation.

New owner gates (4 routes)

• PUT /api/dashboard/config (files.py) — the gate fires BEFORE the config-load I/O
• POST /api/mcp/oauth/relay, POST /api/connections/mint,
  POST /api/connections/cancel (connections.py)

Registrar-walk invariant test (test/test_agent_config_owner_gate_invariant.py)

Walks ALL mutating routes from both registrars (no handler-module filter) and
asserts a non-owner gets 401/403 on every route outside the two documented
exclusion sets. A coherence floor prevents a vacuous pass, and
_MAX_KNOWN_UNGATED_ROUTES makes the debt list a shrink-only ratchet: adding
a route to it requires raising the ceiling in the same diff.

It paid for itself on its first run — it surfaced 4 mutating routes with no
owner gate that were documented nowhere: POST/PUT/DELETE /api/workspaces{,/{name}}
and POST /api/members/{slug}/thread. api_workspaces_delete removes a workspace
from config.json for any authenticated non-owner (e.g. the dashboard token minted
for an allow-listed Slack user, which has an empty app identity). Gating them
changes WHO may manage workspaces — a product decision, not this migration — so
they are recorded with a justification and tracked in #6470.

Test-fixture updates (why 14 test files are touched)

PUT /api/dashboard/config and the three connections routes are now gated, and the
gate reads request.app["state"] plus the authenticated claims. Fixtures that
registered those handlers on a bare web.Application() answered 500 (no state)
or 403 (no owner claim) before the branch each test names. test/dashboard_owner_helpers.py
installs exactly what the gate needs (as_owner(app)), and only when the app has no
state of its own, so a fixture carrying a real DashboardState keeps it. The gate's
own behaviour is tested directly, not incidentally, in
test_dashboard_files_coverage.TestDashboardConfigPutOwnerGate.

error-code-baseline.json

Regenerated via the sanctioned python test/test_error_code_contract.py --update.
Both directions improve: missing_code 1312 → 1310 (two prose-only refusals became
coded by routing through the shared tail) and _compliant 1285 → 1294. The
per-module ratchet in test_ask_question_roundtrip.py drops 21 → 20 because the
scanner is per-file and that one coded site now lives in _shared.py — the wire
contract is unchanged.

Verification

• Rebased onto main 24cb5e76c; squashed to one commit
• flake8, isort, mypy, scripts/check_black_formatting.py, scripts/check_brand_name.py (diff-scoped) all clean
• 1778 targeted tests green across every touched surface plus every test that
  references the changed symbols or the four newly-gated endpoints
• Full backend suite compared against a clean-main control from the same worktree

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

Sound two-tier consolidation; the new invariant's safety net covers only 2 of 13 registrars, and a confirmed non-owner-deletable route ships open.

Watch

• The PR's guarantee that "new routes … cannot silently escape the owner gate" holds only for the agents + agent_config registrars the test builds (agents_routes.register(app); agent_config_routes.register(app)). The other 11 registrars (chat, connections, sessions, system, taskrunner, …) register ~250 mutating routes that stay entirely outside the walk and the ratchet — a new ungated mutating route there is exactly the class this test claims to catch, and it passes CI silently. State the coverage boundary in the test docstring and track per-registrar extension, or the invariant will be over-trusted.
• DELETE /api/workspaces/{name} is a confirmed live gap — the diff's own comment: "removes a workspace from config.json for any authenticated dashboard session — including the session token minted for an allow-listed Slack user." That crosses the multi-human messaging boundary today; the deferral-to-owner-gate the workspace CRUD routes and POST /api/members/{slug}/thread #6470 rationale is coherent, but a human should explicitly accept shipping it open rather than discover it via the debt list.

Suggestions

• The generic-vs-domain audit split means the shared helper's non_owner_block stream and five bespoke SEL shapes now coexist permanently; document the two shapes as the sanctioned pair in the security/SEL spec so the next migrated site doesn't invent a third.

[DESIGN-REVIEWED] 0ff834a

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 0ff834a

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

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 0ff834a8d698da9abcd39976713bea60f178aae2 — 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 facts verified. Writing the review.

First-Principles-Verdict: CONCERNS

The PR ships as_owner as the shared owner-identity fixture, then hand-rolls the identical middleware twice more in the same diff.

What this change ships

Intent: stop nine handler modules from each hand-copying the owner-denial sequence, and close four ungated mutating routes — an ADDITION (consolidation + new gates) despite the refactor: type.

1. Non-owners can no longer write dashboard settings (PUT /api/dashboard/config) — justified, declared
2. Non-owners can no longer mint/cancel connections or relay MCP OAuth (3 routes) — justified, declared
3. Chat non-owner denial body gains code: "owner_only" — declared; derived from the AGENTS.md error-code invariant
4. Nine denial sites now share one helper pair in _shared.py — justified; 6 + 8 counted call sites
5. Any new ungated mutating route now fails a registrar-walk test — justified; supersedes nothing (the module-filtered test asserts stronger properties: exact body, app-token refusal, GETs stay open)
6. A 24-entry ungated-route debt list with a shrink-only ceiling — declared, tracked externally
7. Test helper as_owner/NoConfiguredOwner — justified, but duplicated within this diff (below)
8. Error-code baseline regenerated 1312→1310 via the sanctioned updater — declared
9. source_providers tail deliberately left inline — declared exception; I count exactly 1 remaining direct stale_owner_session_response caller outside _shared.py, matching the claim

Watch

• The identity-middleware pattern (grep X-Test-User", "local-app") now appears 6× under test/; 3 predate this PR (test_agents_endpoints_owner_auth.py, test_mcp_apps_call_endpoint.py, test_secrets_handler.py) and are left unfixed siblings of the helper's own job.
• Title says refactor: while the diff changes shipped behavior (4 new 403s, one wire body gains code). The description declares all of it, so this is a type-prefix mismatch, not dishonest framing — but AGENTS.md's "one logical change per commit" would have split the gates from the migration.

Subtractions

• Delete the hand-rolled _identity/_FakeState in test_agent_config_owner_gate_invariant._build_app and the _State/_identity pair in test_dashboard_files_coverage.config_client_app — both are byte-equivalent to as_owner/NoConfiguredOwner from test/dashboard_owner_helpers.py, shipped in this same diff; call as_owner(app) instead.

[FIRST-PRINCIPLES-REVIEWED] 0ff834a

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] 0ff834a

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

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

[bolichen97]

🤖 Response to Design Review and First Principles Review on 93aa792e9 — fixed in 0ff834a8d.

Adopted — the handlers_instances tier violation (both reviewers, and the more serious of the two findings). You are right and the diff contradicted my own stated criterion. api_instances_search_sessions had a domain audit (_audit("search_sessions", "denied", …) → log_tool_invocation under instances_search_sessions / dashboard:instances) and a domain body ("federated session search is owner-only"), and full delegation replaced both with the generic log_api_access / non_owner_block record. That is a silent hole in this module's instances_* SEL stream, not a cosmetic difference. Shrunk to the tail tier: _audit(...) restored verbatim, body restored via _owner_denial_response(request, "federated session search is owner-only").

Adopted — the same-file sibling First Principles caught. api_instances_proxy (handlers_instances.py:1137-1144) had the identical four-step shape and had been left inline entirely. It is now migrated to the same tail tier, keeping _audit("proxy", …) and "remote-crew proxy is owner-only". That removes the survivor First Principles counted at handlers_instances.py:1139.

Adopted — the tautological assertion. _gated_routes(app) == all_routes - _PRE_OWNER_EXCLUSIONS - _KNOWN_UNGATED_ROUTES re-evaluates _gated_routes's own definition and cannot fail; I added it to keep the test from being constant-only, which was the wrong reason. Replaced with a real invariant: the two exclusion sets must be disjoint. An entry in both is excluded twice, so removing it from one — the natural move when a pre-owner route grows up — silently leaves it excluded by the other and the walk never tests it.

Declined, with reasoning — source_providers._authorize_owner_request (the other survivor). It is not the same four-step sequence: it has four separate denial branches and only the caller != owner_id one reaches the stale relabel, so there is no single tail to hoist. Its body is a bare {"error": "forbidden"} with no code, and that exact body is pinned by equality assertions in test_stale_owner_session_signal.py and four places in test_source_providers.py; _owner_denial_response always emits a code, so migrating it is a wire change on four source-provider routes that belongs in the error-code track, not in this consolidation. It is also the module that defines stale_owner_session_response, so routing it through _shared points the definition site back through its own consumer. The PR description now states this explicitly instead of claiming "the remaining" without qualification.

Declined, with reasoning — deleting test_every_mutating_agents_route_refuses_non_owner. First Principles is right that the new walk covers a superset of those routes with the identical 401/403 assertion. I am keeping it anyway: the two tests live in different files, and the older one is the only thing that still holds if the new walk's exclusion sets are ever loosened or the file is renamed away. Duplication in a security invariant is cheap; discovering later that the only copy was deleted is not.

Noted as follow-up — collapsing the two tiers into one entry point (a custom-audit callable on the async helper, per Design Review's suggestion). Agreed that it would remove the predicate + comment boilerplate, and agreed it does not belong here: it changes the shared helper's signature while nine call sites are mid-migration.

[bolichen97]

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

• PR #2383 is PARTIALLY_COVERED relative to this PR. Coverage is explicitly incomplete; this finding is not a completion or closure claim. Recommended action for PR #2383: REBASE. The gate the PR wrote by hand is now a shared helper with a different error code; align on rebase. Nothing about the feature itself is superseded. Files: test/test_agent_config_owner_gate_invariant.py.
• PR #5161 is PARTIALLY_COVERED relative to this PR. Coverage is explicitly incomplete; this finding is not a completion or closure claim. Recommended action for PR #5161: REBASE. Main landed the boundary this PR builds on, not any route it gates. Rebasing onto f38562c lets the PR drop the duplicated predicate, reuse test/dashboard_owner_helpers.py, and prune its 20 entries from _KNOWN_UNGATED_ROUTES while keeping the whole security payload. Files: src/kiro_crew/dashboard/handlers/_shared.py, src/kiro_crew/dashboard/handlers/mcp.py, test/test_mcp_config_endpoints_owner_auth.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.