Repository navigation
feat: ignore local enables with no governance key in enterprise builds - #23703
iamwhatever wants to merge 1 commit into
Conversation
UX Review (Opus 5.5) — ⏭️ skippedRevision |
Design Review (Opus 5.5) — ✅ PASSDesign-level review of Design-Verdict: PASS Nothing blocks: the change only takes effect when Notes
[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 |
First Principles Review (Opus 5.5) — ✅ PASSPremise-level review of 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
What this change shipsInventory (7 items) — 3 justifiedIntent: stop an enterprise build's local switches from turning on controls that remote policy cannot govern yet. ADDITION.
[FIRST-PRINCIPLES-REVIEWED] e02295c Description read[DESCRIPTION-READ] 0457b53f6eca9dfb5d8f3aaf77bbdb00529a01ac30946c3809939e3b4ad477d8 That sha256 names the PR description this verdict read. Recompute it with |
Opus 5.5 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
GPT 6.1 Review — ✅ no blocking findingsGPT 6.1 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
|
Design Review notes on d0bf64a (advisory, no code change):
|
|
First Principles notes on d0bf64a (advisory, no code change):
|
bolichen97
left a comment
There was a problem hiding this comment.
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):
test/test_updates_handlers_facade.py::test_every_name_served_from_outside_the_family_is_visible_to_type_checkersfails.updates_channel.pynow importsLOCKED_BY_POLICYandlocal_enable_locked. Theupdatesfacade does not name them in itsTYPE_CHECKINGblock.test/test_facade_mirror_guards.py::test_no_test_patches_a_shared_name_on_an_owner_directlyfails. The new test patchesresolve_provideronupdates_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).
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.
d0bf64a to
e02295c
Compare
Both failures are fixed at e02295c. See the new review.
bolichen97
left a comment
There was a problem hiding this comment.
Re-reviewed at e02295c. Both failures from my earlier review are fixed, so I dismissed that review.
- Facade mirror for the updates handlers:
updates_channel.pynow importsLOCKED_BY_POLICYandlocal_enable_lockedinsideapi_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. - The new test now patches
resolve_provideranddetect_install_layoutthrough theupdatesfacade instead of onupdates_channeldirectly.
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.
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.jsonopt-in,tunnel.enabledandtelemetry.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
KIROCREW_SECURITY_POLICY, the bundled and homesecurity_policy.json) beneath the remote policy. That half of D3 depends on the built-in baseline / deny-unnamed item and is handled with it.KIROCREW_BIND,KIROCREW_CORS_ORIGINS,dashboard.url): a follow-up PR, because they touch the listener.ossedition.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_LOCKSlists each control (ids follow the RFC appendix "Crew key" column), andlocal_enable_locked(<id>)returnsFalseonossandTrueon 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.app_dev_modeinstalled.jsondev, the dev sentinel,POST /api/apps/{name}/dev,kirocrew app devapps/dev_mode.pylocked_by_policy(409 over HTTP), SEL rowapp_dev_modedeniedupdates.channel$KIROCREW_HOME/channelplatform/update_layout.py::release_channelanswersstablePOST /api/update/channelto a non-stable channel answers 409 after the layout refusals, auditedupdate.channelcomputer_usecomputer_use.jsonenabledcomputer_use/enable_state.py::is_enabledlocked_by_policy; an enable save answers 409tunneltunnel.enableddashboard/server_runtime/tunnel.py::_start_aea_tunnel(the edition's ownTunnelProvideris not local)GET /api/config/schemamarks the pathlockedByPolicytelemetry.otlp_hoststelemetry.otlp_endpointmetrics/provider.py::_edition_locked_view, before any provider reads the config, for both_otlp_destinationsandotlp_egress_activeGET /api/config/schemamarks the pathlockedByPolicyLocal policy files and profiles moved to the baseline PR (item it_ba32ce2d). A first attempt clipped local
security_policy.jsontiers 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 localcommandsdeny ofcurl *and afilesystem.writeallowlist 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) andcomputer-use.md(an edition lock, not a governance scope; computer use stays ungoverned).Backwards compatibility
Compatible: every check returns
Falseon theossedition, the only edition that has installs, so each feature reads its switch exactly as before.Removes nothing: on
ossno 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 itsosscase:osslocks nothing; the enterprise lock logs once.CONFIG_PATH_LOCKSnames a listed control; the schema endpoint marks exactly those paths.nightlychannel file reads asstable; the channel POST answers 409locked_by_policyand writes no file.installed.jsondev: true, the cache and the sentinel are all ignored.computer_use.jsonenabled: truereads as off.telemetry.otlp_endpointyields no destination andotlp_egress_activeis false.Updated:
test_computer_use_api.pycoded-refusal ratchet 20 → 21 (the new 409);test_dashboard_server_composition_contract.pyregisters 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
feat|fix|docs|style|refactor|perf|test|chore|ci|build|revert: ...)Contribution License Agreement
Contributions are accepted under the project's Apache-2.0 license; see the Licensing section of CONTRIBUTING.md.