Skip to content

feat: ignore local enables with no governance key in enterprise builds - #23703

Open
iamwhatever wants to merge 1 commit into
mainfrom
feat/ee-unkeyed-local-enables
Open

iamwhatever wants to merge 1 commit into
mainfrom
feat/ee-unkeyed-local-enables

Conversation

@iamwhatever

@iamwhatever iamwhatever commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

Problem / Motivation

Goal: In an enterprise build, a local switch for a control that has no governance key can no longer turn that control on.

RFC rfc-crew-enterprise-edition (#23208) decision D3: remote rules are the only thing that can turn a control on; local config, environment variables, profiles and dashboard settings may only narrow it. A control with a governance key gets that from the evaluator. Five controls in the RFC appendix have no key yet, so no remote rule can turn them on, but a local switch still does today: app developer mode, the release channel file, the computer_use.json opt-in, tunnel.enabled and telemetry.otlp_endpoint.

Why it matters

Without this, a user or an agent with write access to the data home can turn on, in an enterprise build, a tunnel, a metrics endpoint off the host, desktop control, an app dev-mode serving root or a pre-release channel that the organisation's policy never allowed and has no way to forbid.

Not a goal

  • Local policy files (KIROCREW_SECURITY_POLICY, the bundled and home security_policy.json) beneath the remote policy. That half of D3 depends on the built-in baseline / deny-unnamed item and is handled with it.
  • Controls that already have a governance key: the evaluator change (deny-unnamed, baseline) covers them.
  • Bind address and dashboard origins (KIROCREW_BIND, KIROCREW_CORS_ORIGINS, dashboard.url): a follow-up PR, because they touch the listener.
  • Adding real governance keys for these controls, the policy URL, the login gate and the Red hardcodes.
  • Any change to the oss edition.

What changed (motivation → approach → change)

Goal: one place that says which local switches an enterprise build ignores, checked where each feature reads its own switch.

Approach: a small seam, src/kiro_crew/edition_locks.py. UNKEYED_LOCKS lists each control (ids follow the RFC appendix "Crew key" column), and local_enable_locked(<id>) returns False on oss and True on enterprise, logging one warning per control. An unknown id raises, so a typo fails in tests. Checking at each feature's read site, not at config load, covers every local writer at once (config.json, config.local.json, kirocrew config set, the dashboard, a hand edit) and also the switches that do not live in config.json at all (the channel file, installed.json, computer_use.json). When a later PR adds a real key for a control, it replaces that row with a governance check.

Control id Local source ignored Read site Setting API
app_dev_mode installed.json dev, the dev sentinel, POST /api/apps/{name}/dev, kirocrew app dev apps/dev_mode.py answers locked_by_policy (409 over HTTP), SEL row app_dev_mode denied
updates.channel $KIROCREW_HOME/channel platform/update_layout.py::release_channel answers stable POST /api/update/channel to a non-stable channel answers 409 after the layout refusals, audited update.channel
computer_use computer_use.json enabled computer_use/enable_state.py::is_enabled config GET carries locked_by_policy; an enable save answers 409
tunnel tunnel.enabled dashboard/server_runtime/tunnel.py::_start_aea_tunnel (the edition's own TunnelProvider is not local) GET /api/config/schema marks the path lockedByPolicy
telemetry.otlp_hosts telemetry.otlp_endpoint metrics/provider.py::_edition_locked_view, before any provider reads the config, for both _otlp_destinations and otlp_egress_active GET /api/config/schema marks the path lockedByPolicy

Local policy files and profiles moved to the baseline PR (item it_ba32ce2d). A first attempt clipped local security_policy.json tiers to the scopes the remote policy names, but the evaluator still permits an unnamed scope, so dropping a local value also dropped its narrowing: a local commands deny of curl * and a filesystem.write allowlist both stopped applying. The baseline PR fills unnamed scopes into the central ceiling before the tier fold, so the existing tighten-only intersect keeps those local narrowings and still stops a local tier from widening. That attempt was never pushed.

A locked value that is written is stored and ignored on read, so a save never loops on an error. Docs: platform-context.md (new "Local switches with no governance key" subsection with this table) and computer-use.md (an edition lock, not a governance scope; computer use stays ungoverned).

Backwards compatibility

Compatible: every check returns False on the oss edition, the only edition that has installs, so each feature reads its switch exactly as before.

Removes nothing: on oss no path, value or API changes; on enterprise (no installed base) the locked switches are ignored, which is the RFC's stated behaviour.

Tests

test/test_enterprise_unkeyed_locks.py, each enterprise case paired with its oss case:

  • an unknown control id raises; oss locks nothing; the enterprise lock logs once.
  • every config path in CONFIG_PATH_LOCKS names a listed control; the schema endpoint marks exactly those paths.
  • a nightly channel file reads as stable; the channel POST answers 409 locked_by_policy and writes no file.
  • dev mode: enable refused and SEL-audited; installed.json dev: true, the cache and the sentinel are all ignored.
  • computer_use.json enabled: true reads as off.
  • telemetry.otlp_endpoint yields no destination and otlp_egress_active is false.

Updated: test_computer_use_api.py coded-refusal ratchet 20 → 21 (the new 409); test_dashboard_server_composition_contract.py registers the tunnel owner's lazy import.

Proof: not provable -- other: new behaviour behind an edition flag, every new test fails without the source change by construction

Manual verification

N/A — unit coverage sufficient: each lock is a pure check at a read site, tested on both editions; no UI change.

Related Issues

Refs #23206

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; see the Licensing section of CONTRIBUTING.md.

@iamwhatever
iamwhatever requested review from a team and KiroCrewCaptain as code owners October 24, 2026 08:37
@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 e02295c8446bdbe101061b7bc7d1cdef3e8ffe4d 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 e02295c8446bdbe101061b7bc7d1cdef3e8ffe4d — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS does not.

Design-Verdict: PASS

Nothing blocks: the change only takes effect when is_enterprise() is true, and every reader of each locked switch (including dev_mode_granted_root via is_dev_mode) goes through the new check.

Notes

  • release_channel.recorded_channel() still reads the channel file as-is. That is right, because it only reports the channel in a bug report and turns nothing on. A short comment in edition_locks saying it is deliberately not locked would stop a later reviewer from "fixing" it.

[DESIGN-REVIEWED] e02295c

Description read

[DESCRIPTION-READ] 7db37166833cf7c6e1aa8979a129a3daadef21ac3ef4577b31f9d903f25702b6

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 e02295c8446bdbe101061b7bc7d1cdef3e8ffe4d — 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

Nothing blocks. Two new fields and one helper have no consumers, and the authority the PR cites is a draft RFC.

Notes

  • Item 6 — zero consumers: edition_locks.locked_controls() is called only in test/test_enterprise_unkeyed_locks.py (grepped locked_controls: 0 non-test hits). Subtraction: delete locked_controls() and its test assertion.
  • Item 5 — zero consumers: nothing in website/ reads lockedByPolicy in the schema or locked_by_policy in the computer-use GET snapshot (grepped both: 0 frontend hits). The 409 refusals already carry the lock. Subtraction: drop d["lockedByPolicy"] in agents.py and the "locked_by_policy" key in _snapshot(), then add them back when a UI reads them.
  • Item 1–4 — inherited: these items remove four controls and an OTLP egress path from the enterprise build on the strength of "RFC … decision D3", and rfc-crew-enterprise-edition.md is draft on base. The enterprise build has no installs, so this stays reversible and does not block. The decision it rests on is still not recorded.

What this change ships

Inventory (7 items) — 3 justified

Intent: stop an enterprise build's local switches from turning on controls that remote policy cannot govern yet. ADDITION.

  1. Enterprise build refuses or ignores app dev mode (HTTP 409, CLI error, SEL row) — inherited (draft RFC)
  2. Enterprise build pins the release channel to stable; a channel POST answers 409 — inherited (draft RFC)
  3. Enterprise build ignores the computer-use opt-in; an enable save answers 409 — inherited (draft RFC)
  4. Enterprise build ignores tunnel.enabled and telemetry.otlp_endpoint — inherited (draft RFC)
  5. Config schema and computer-use GET expose new lock fields — zero consumers
  6. A locked_controls() helper — zero consumers
  7. Black reformatting of two files that were on the black baseline, and two baseline rows dropped — justified
  8. Spec sections in platform-context.md and computer-use.md — justified
  9. One warning logged per locked control — justified

[FIRST-PRINCIPLES-REVIEWED] e02295c

Description read

[DESCRIPTION-READ] 0457b53f6eca9dfb5d8f3aaf77bbdb00529a01ac30946c3809939e3b4ad477d8

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 — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] e02295c

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

False positive or not applicable? A repository writer can comment:
/ai-review override fable e02295c8446bdbe101061b7bc7d1cdef3e8ffe4d 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 — ✅ no blocking findings

GPT 6.1 completed its review of e02295c8446bdbe101061b7bc7d1cdef3e8ffe4d and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] e02295c

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

@iamwhatever

Copy link
Copy Markdown
Contributor Author

Design Review notes on d0bf64a (advisory, no code change):

  • recorded_channel in release_channel.py: agreed, it only labels bug-report provenance and picks no update lane, so it cannot turn a channel on. Out of this PR's goal; left as is.
  • lockedByPolicy with no dashboard reader: the item this PR implements asks for the API to report a locked setting as locked by policy; rendering it in Settings is a frontend follow-up, not part of this goal.

@iamwhatever

Copy link
Copy Markdown
Contributor Author

First Principles notes on d0bf64a (advisory, no code change):

  • Item 6 (zero consumers): the API fields are the requested surface. RFC D3's work item asks that a locked setting show locked by policy in its setting API, and the 409s only cover writes, not reads. locked_controls() has no caller yet; it can go in the follow-up that renders the flag, so I am not pushing for it alone.
  • Item 3 (computer use): the RFC appendix lists Desktop control with enterprise default "off" and no key. This is an edition lock, not a governance scope, and oss is unchanged.
  • Item 7 (black reflow): AGENTS.md asks for the shorter baseline to be committed when touching a file makes it black-clean, which is what .github/black-baseline.txt records here.

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not ready yet. The design is sound and the two failures below are small, but they are real on this head.

Head reviewed: d0bf64a.

What I checked: each lock sits at the read site of its switch. I grepped the head for other readers of tunnel.enabled, the channel file, otlp_endpoint and the computer use enabled key. I found no other enable path. The only extra reader is release_channel.recorded_channel, and the Design Review already notes it. dev_mode_granted_root goes through is_dev_mode, so the UI root grant is locked too.

Must fix (both reproduced locally on this head, and both are the cause of the red Backend Tests (Windows) (2) and (4) shards):

  1. test/test_updates_handlers_facade.py::test_every_name_served_from_outside_the_family_is_visible_to_type_checkers fails. updates_channel.py now imports LOCKED_BY_POLICY and local_enable_locked. The updates facade does not name them in its TYPE_CHECKING block.
  2. test/test_facade_mirror_guards.py::test_no_test_patches_a_shared_name_on_an_owner_directly fails. The new test patches resolve_provider on updates_channel, which is one owner of a mirrored family.

Not repeated here: the zero-consumer lockedByPolicy / locked_by_policy fields and the unrelated black reflows (First Principles notes), and the unlocked recorded_channel (Design notes).

Comment thread src/kiro_crew/dashboard/handlers/updates_channel.py Outdated
Comment thread test/test_enterprise_unkeyed_locks.py Outdated
@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
RFC crew-enterprise-edition D3: in an enterprise build only the remote
policy may turn a control on. Five controls have no governance key, so
no remote rule can turn them on, yet a local switch still could: app dev
mode, the release channel file, the computer_use.json opt-in,
tunnel.enabled and telemetry.otlp_endpoint.

edition_locks.local_enable_locked() is checked where each feature reads
its switch. On enterprise the local value is ignored and logged once;
the setting APIs answer locked_by_policy. The oss edition is unchanged.
@iamwhatever
iamwhatever force-pushed the feat/ee-unkeyed-local-enables branch from d0bf64a to e02295c Compare October 24, 2026 09:35
@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
@bolichen97
bolichen97 dismissed their stale review October 24, 2026 10:05

Both failures are fixed at e02295c. See the new review.

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed at e02295c. Both failures from my earlier review are fixed, so I dismissed that review.

  1. Facade mirror for the updates handlers: updates_channel.py now imports LOCKED_BY_POLICY and local_enable_locked inside api_update_channel, the same function-local pattern the file already uses for _unix_peer_is_self. No new module global is shared with the facade, so the facade needs no new mirror entry.
  2. The new test now patches resolve_provider and detect_install_layout through the updates facade instead of on updates_channel directly.

Verified locally on this head: test/test_facade_mirror_guards.py (including test_no_test_patches_a_shared_name_on_an_owner_directly), test/test_updates_handlers_facade.py and test/test_enterprise_unkeyed_locks.py pass (96 passed). The delta since my last review is these two files only, and I found nothing new to raise. CI was still running the backend, Windows and E2E suites when I checked.

The code looks clean to me. The linked issue is tier T4, so the design call on this behavior stays with a maintainer; I am leaving this as a comment rather than an approval.

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision readiness: checking Automated validation is still running and removed readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision 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: passed Eligible automated validation passed for the current revision

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants