Skip to content

feat(governance): let a policy name decision models, points and endpoints - #23719

Open
iamwhatever wants to merge 1 commit into
mainfrom
feat/ee-list-decisions
Open

iamwhatever wants to merge 1 commit into
mainfrom
feat/ee-list-decisions

Conversation

@iamwhatever

@iamwhatever iamwhatever commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

Problem / Motivation

Goal: A security policy can name the exact decision models, decision points and provider endpoints the decision seam may use, and a request outside them is not sent.

capabilities.decisions is an on/off gate with no list (RFC crew-enterprise-edition, appendix rows "Decision providers" partial, "Decision models" decisions.models, "Decision kinds" decisions.points and "Decision endpoints" decisions.endpoints, all no).

Why it matters

A fleet that approves one model at one endpoint for some decision points must be able to refuse the rest, rather than choosing between the whole seam and nothing.

Not a goal

  • No enterprise-only branch and no change to _query_level: with no policy, or a policy without these lists, decisions behave as today.
  • No change to consent, sampling, the local-model runtime or capabilities.decisions_local.
  • The other three partial controls are separate PRs.

What changed (motivation → approach → change)

Goal → the existing CapabilityGate inner-ruleset shape → one catalog row change, one probe, one call site.

  • governance_parts/catalog.py: capabilities.decisions gains scope_matchers models (identifier), points (identifier), endpoints (host).
  • decisions/capability.targets_denied: asks points:<point> and, for the Jev lane, models:<model> and endpoints:<host> for every reading of the host; each evaluated and audited, never short-circuited; fail-closed. The items are bracketed with the governance answer generation, as denied_sides does, so one request is never answered by two policies.
  • decisions/gate.decide: calls it once the lane and model are known, before the scrub. A refusal returns None and writes no decision row, like the capability refusal. models and endpoints bind the decision provider, which only the Jev lane uses; the judge's LLM lane runs on the session's own model provider through the session runner, which picks the served model itself, so it is asked about its point only.
  • governance_parts/matching.url_policy_hosts: every host a client could connect to for the endpoint (the standard library plain and host-percent-decoded, urllib3, yarl; non-ASCII in IDNA form), so a root dot, a backslash, a percent escape or an ideographic dot cannot dodge an endpoint deny. The same helper is added identically in feat(governance): let a policy name the OTLP collector hosts #23716 and feat(governance): let a policy name the feature content hosts #23717.
  • Docs: governance.md (decision seam section).

Call site wired: decisions/gate.py::decide (through _targets_denied).

Backwards compatibility

Compatible: with no policy, or a policy that does not name these lists, every item is permitted and decide sends as today. The extra SEL rows appear only on a consented install, past the existing capability check.

Removes nothing: the lists only narrow when a policy names them.

Tests

  • test/test_governance_list_decisions.py: each key parses with its catalog matcher; a list allowing X permits X and denies Y; gate off denies; no policy or no list permits.
  • test/test_decisions_gate.py (TestDecisionLists): no policy sends; a list naming this request's point, model or endpoint sends; a list not naming it sends nothing, writes no row and audits the items; gate off sends nothing; the LLM lane is asked about its point only; a check error denies; a policy swap between items denies and is audited; root-dot, backslash, ideographic-dot and percent spellings cannot dodge an endpoint deny.

Proof: not provable -- other: new feature; the tests assert behaviour that main does not have

Manual verification

N/A — unit coverage drives decide with a real policy and the real evaluator.

Related Issues

Refs #23206. RFC: #23208.

Checklist

  • At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|style|refactor|perf|test|chore|ci|build|revert: ...)
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable)
  • No secrets, credentials, or internal references in the diff

Contribution License Agreement

Contributions are accepted under the project's Apache-2.0 license.

@iamwhatever
iamwhatever requested review from a team and KiroCrewCaptain as code owners October 24, 2026 09:31
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Oct 24, 2026
@github-actions

github-actions Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

UX Review (Opus 5.5) — ⏭️ skipped

Revision 71567571b686acbd50a8dfa9616f6ecace71a890 touches no user-facing surface (no changes under website/ or committed screenshots), so the UX review was skipped. Does not block merge.

@github-actions

github-actions Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

Design Review (Opus 5.5) — ✅ PASS

Design-level review of 71567571b686acbd50a8dfa9616f6ecace71a890 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS does not.

Design-Verdict: PASS

No design-level blockers. The change adds one catalog row to the existing inner-ruleset shape, and with no list named, decisions behave exactly as before.

Notes

[DESIGN-REVIEWED] 7156757

Description read

[DESCRIPTION-READ] 286de4ed5fd187bb9907cea8ea59ee455d253ccf358fa3d990f3f6b6e679915a

That sha256 names everything this verdict read: the PR description, plus the 1 evidence file(s) the workflow collected from it and handed this review. It covers that evidence because the media strip replaces every attachment URL with the same placeholder, so an edit swapping one attachment for another would leave the description's captured text identical while what this review judged changed. Recompute it with .github/scripts/pr-description-capture.sh over the same inputs; if it differs, the description or that evidence changed after this verdict was formed and any finding drawn from either is unproven.

@github-actions

github-actions Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

First Principles Review (Opus 5.5) — ✅ PASS

Premise-level review of 71567571b686acbd50a8dfa9616f6ecace71a890 — 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 does not.

First-Principles-Verdict: PASS

No default moves: the lists only narrow when a policy names them. Two riders reach past the decision seam.

Notes

  • Item 4 — undeclared: _match_host now compares IDNA forms for every host-matcher scope, including network.egress (the hunk return fnmatch.fnmatch(_idna_labels(item...), _idna_labels(pattern...))). The description only mentions the decisions seam. Subtraction: drop the _match_host edit and emit IDNA only inside url_policy_hosts, which already does that for requests; or name the egress-wide change.
  • Item 5 — one consumer, generalized: url_policy_hosts is a second URL→host reader beside matching._url_host. It has 1 consumer (capability.targets_denied), while _url_host serves 3 egress sites in tool_calls.py (grepped _url_host(). The description says "The same helper is added identically in feat(governance): let a policy name the OTLP collector hosts #23716 and feat(governance): let a policy name the feature content hosts #23717", which makes three copies across PRs. Subtraction: land it once, in a single PR, and have the other two import it.

What this change ships

Inventory (5 items) — 3 justified

Intent: let an enterprise policy restrict which decision models, points and endpoints the decision seam may use. ADDITION.

  1. Policy can name capabilities.decisions.points (allow/deny) — justified
  2. Policy can name models and endpoints, binding the Jev lane only — justified
  3. A refused request sends nothing, writes no decision row, and is audited in SEL — justified
  4. Every host-list match now treats a non-ASCII name and its IDNA form as the same host — undeclared
  5. New public url_policy_hosts, a multi-parser host reader — one consumer, generalized

[FIRST-PRINCIPLES-REVIEWED] 7156757

Description read

[DESCRIPTION-READ] 80445d1cbdafa1f8bbd076e5ec86df5939acb6581680d9b227b2155cf3c75e3a

That sha256 names the PR description this verdict read. Recompute it with .github/scripts/pr-description-capture.sh; if it differs, the description changed after this verdict was formed and any finding drawn from the description is unproven.

@github-actions

github-actions Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

Opus 5.5 Review — 🔴 changes requested (blocking)

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

The new IDNA normalisation in _match_host stops a non-ASCII glob entry from matching, so existing network.egress deny entries no longer refuse hosts they refused before this change.

BLOCKING — src/kiro_crew/platform/governance_parts/matching.py:122
_idna_labels(item.strip().casefold()), _idna_labels(pattern.strip().casefold()) with if label.isascii() or any(c in label for c in "*?["): labels.append(label)
Trigger: a policy has "network": {"egress": {"mode": "deny", "deny": ["bücher*.de"]}} and the agent fetches https://bücher-shop.de/x. tool_calls calls _url_host, which returns the Unicode host bücher-shop.de, and resolve passes it to _MATCHERS["host"], which is _match_host. The item label is converted to xn--…, but the pattern label contains a glob character, so it stays Unicode and fnmatch never matches.
Outcome: resolve(...) now returns permitted=True. Before this change, fnmatch('bücher-shop.de', 'bücher*.de') refused the fetch. So an administrator's egress deny ceiling no longer applies, and an allow list with the same kind of entry now refuses hosts it used to permit. This hits every host-matcher scope, not just the new endpoints list.
Anchor: residual/security
Fix: treat it as a match if either the raw casefolded comparison (the old behaviour) or the IDNA-normalised comparison matches, so the normalisation can only add matches and never remove one.

[BLOCK-MERGE] 7156757
[OPUS-REVIEWED] 7156757

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

False positive or not applicable? A repository writer can comment:
/ai-review override fable 71567571b686acbd50a8dfa9616f6ecace71a890 false-positive: <reason, 20 to 500 characters>

@github-actions

github-actions Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

GPT 6.1 Review — 🔴 changes requested (blocking)

GPT 6.1 found at least one blocking issue that must be resolved before merging 71567571b686acbd50a8dfa9616f6ecace71a890. 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 -- src/kiro_crew/platform/governance_parts/matching.py:122 -- IDNA conversion disables existing Unicode host globs

_idna_labels(item.strip().casefold()), _idna_labels(pattern.strip().casefold())
Trigger: A policy denies network.egress with ["büch*.example"], and web_fetch requests https://bücher.example/; gate_decision → classify_tool_args → resolve → _match_host reaches this line.
Outcome: The host becomes xn--bcher-kva.example, but the wildcard pattern remains Unicode. The previously matching deny now misses, and the governance gate permits the forbidden fetch.
Anchor: residual/security
Fix: Preserve the original Unicode-glob comparison alongside IDNA equivalence instead of comparing an encoded host exclusively against an unencoded Unicode wildcard pattern.

[BLOCK-MERGE] 7156757
[GPT-REVIEWED] 7156757

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

The fenced finding (F1) stands: I'm upholding it as written. Nothing else was submitted for adjudication.

F1: This is unbounded harm, of the governance-ceiling kind. It is a regression in this PR: a network.egress deny that matched before this PR now misses.

  • Item side: web_fetch takes its host from _url_host (src/kiro_crew/platform/governance_parts/matching.py:232), which reads urlparse().netloc and leaves a Unicode label as Unicode. At line 122, _idna_labels then turns bücher.example into xn--bcher-kva.example.
  • Pattern side: _idna_labels (matching.py:131-133) leaves any label containing *?[ unencoded, so büch* stays Unicode. fnmatch then never matches, and there is no fallback to a Unicode comparison. Before this PR the same deny matched, because both sides stayed Unicode.
  • Rarity: The only condition is an operator writing a deny glob with a non-ASCII label, which is uncommon but ordinary. Nothing recovers from it, and the miss is silent. The fix is cheap: also compare the raw Unicode strings.
[ADJUDICATION] 71567571b686acbd50a8dfa9616f6ecace71a890 total=0 uphold=0 downgrade=0
[GPT-ADJUDICATED] 71567571b686acbd50a8dfa9616f6ecace71a890

[ADJUDICATION-FENCED] 71567571b686acbd50a8dfa9616f6ecace71a890 fenced=1 flagged=0
UPHOLD-FENCED F1 src/kiro_crew/platform/governance_parts/matching.py:122 -- _url_host keeps a Unicode host and _idna_labels encodes it while leaving a Unicode glob label untouched, so an operator's ordinary Unicode-glob network.egress deny that matched before this PR now silently misses and permits the fetch.
[GPT-ADJUDICATED-FENCED] 71567571b686acbd50a8dfa9616f6ecace71a890

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 71567571b686acbd50a8dfa9616f6ecace71a890 false-positive: <reason, 20 to 500 characters>

@iamwhatever

Copy link
Copy Markdown
Contributor Author

Fixed in 72a3966: GPT BLOCKING on decisions/capability.py (a root-dot spelling jev.corp.example. dodged an endpoint deny). targets_denied now asks endpoints:<host> for every reading of the endpoint host (governance_parts.matching.url_policy_hosts: root dot removed, NFKC-folded, backslash read both ways), so both a root dot and a backslash authority are refused (test_an_endpoint_spelling_cannot_dodge_a_deny). SAST: the TestDecisionLists autouse fixture now patches through _floor_monkeypatch (kirocrew.test-autouse-shared-monkeypatch).

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Oct 24, 2026
@iamwhatever
iamwhatever force-pushed the feat/ee-list-decisions branch from 72a3966 to 6024f7e Compare October 24, 2026 10:41
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Oct 24, 2026
@iamwhatever

Copy link
Copy Markdown
Contributor Author

Fixed in 6024f7e: both BLOCKING findings on matching.py:298 (ideographic full stop U+3002 / U+FF61, which aiohttp/yarl reads as a dot). Same class as #23716 and #23717, so the approach changed rather than adding a spelling: url_policy_hosts now includes yarl.URL(...).raw_host (the transport JevOracle uses), urllib3.parse_url, the standard library's plain and percent-decoded readings, and the IDNA form of each. Any non-plain reading also adds "", which an allow list refuses. Tests: the ideographic-dot and percent cases of test_an_endpoint_spelling_cannot_dodge_a_deny.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Oct 24, 2026
@iamwhatever
iamwhatever force-pushed the feat/ee-list-decisions branch from 6024f7e to a5ed698 Compare October 24, 2026 11:37
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Oct 24, 2026
@iamwhatever

Copy link
Copy Markdown
Contributor Author

Round 3, fixed in a5ed698.

  1. GPT BLOCKING (gate.py:1165, the LLM judge's served model can differ from the configured id): fixed by narrowing, not by a session-layer change. models and endpoints bind the decision PROVIDER, which only the Jev lane uses. The judge's LLM lane runs on the session's own model provider through the session runner, which picks the served model itself, so that lane is now asked about points only. The catalog comment, governance.md and TestDecisionLists.test_the_llm_lane_is_asked_about_its_point_only say so. Binding the session runner's served model to a policy list is a separate control, not one of the four this PR adds.
  2. GPT BLOCKING (matching.py:334, a valid IDNA endpoint refused): a non-ASCII reading is now asked only in its IDNA form, and "" is added only when it has none, so an xn--bcher-kva.example allow entry admits bücher.example. Shared helper, identical in feat(governance): let a policy name the OTLP collector hosts #23716 and feat(governance): let a policy name the feature content hosts #23717, which also now decodes only the host part of the authority.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Oct 24, 2026
@iamwhatever

Copy link
Copy Markdown
Contributor Author

Round 4, fixed in the shared url_policy_hosts / _match_host (identical in #23716 7c396eb, #23717 6a6d928, #23719 7156757):

  • scoped IPv6: the zone (%eth0) is dropped, so fe80::* matches fe80::1;
  • IP literals in any spelling the resolver accepts (167772165, 0x0a000005, ::ffff:10.0.0.5) are also asked in canonical form (security.canonicalize_ip then ipaddress);
  • _match_host compares non-ASCII labels in IDNA form on both sides, so a bücher.example entry and an xn--bcher-kva.example request, or the reverse, are one host;
  • Backend Tests (3.12, 2) / (Windows) (2): the governance facade manifest now names the new matching symbols (test_facade_mirror_guards).

This class came back in four rounds, so per AGENTS.md I am narrowing rather than chasing a fifth spelling. A reading no parser can canonicalize is asked as "", which an allow list refuses and a deny list does not. That is the posture network.egress already has, now stated in url_policy_hosts and governance.md: an allow list is the hard bound. The deny-mode residual is deferred to #23767 with the design it needs. Tests: test_a_deny_names_every_spelling_of_its_host (#23716), and the unicode case plus the decimal-IP assertion in test_an_endpoint_spelling_cannot_dodge_a_deny (#23719).

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Oct 24, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

readiness: action required A blocking check or review needs attention

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant