feat: let a new session start on a configured approval tier - #8984
feat: let a new session start on a configured approval tier#8984chenmingwei23 wants to merge 1 commit into
Conversation
Design Review (Fable 5) — ✅ PASSDesign-level review of Design-Verdict: PASS Right problem, right layer: the tier is applied once at slot mint, owner-gated, fail-safe toward more prompts, and fully reversible via a clamped enum. Watch
[DESIGN-REVIEWED] 21389ea |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of First-Principles-Verdict: CONCERNS The card earns its place on two reported requests, but a one-value helper, a hand-edit-only warning line and four source-text tests are surface the fix does not need. What this change shipsIntent: stop re-picking the approval tier in the footer for every new chat — an ADDITION.
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 21389ea |
GPT 5.6 Review — 🔴 changes requested (blocking)GPT 5.6 found at least one blocking issue that must be resolved before merging This comment is updated in place on each push. BLOCKING -- src/kiro_crew/dashboard/chat_handlers.py:2805 -- The audit-or-deny write is only queued Adjudication (Opus 4.8) — is blocking on each finding proportionate?Confirmed against code. Line 2805 calls Harm: unbounded — an unattended auto-approve trust grant with no durable audit record, which is the case the code's own comment says must fail closed. Conditions: feature-configured owner-request new slot in a tier-inheriting mode ( [ADJUDICATION] 21389ea total=0 uphold=0 downgrade=0 False positive or not applicable? A repository writer can comment: |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsI've verified the candidate's crux by opening the code myself:
So No blocking issues; one advisory finding. FINDING — website/src/pages/settings/SecurityPanel.tsx:2381 — the [OPUS-REVIEWED] 21389ea Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
ccd5d2f to
70c526a
Compare
UX Review (Fable 5) — 🟡 CONCERNSUX-level review of I have everything I need: the blind read, the diff, the PR body, and all five screenshots. Reconciling: the two option radios (the primary controls) were correctly identified and confidently usable in the steady states; the confusion the blind reader reported is concentrated in the save-failed copy, the trust-note/override-warning contradiction, and a clipped "Ask the agent" link — and the new "New chat defaults" section heading appears in no screenshot. No lens-13 hard swap exists (selection toggles classes on the same buttons; notices are async lifecycle states). UX-Verdict: CONCERNS The card cold-reads cleanly, but the failure and override states leave the reader unsure which setting is actually in force. Watch
Evidence gaps
Suggestions
[UX-REVIEWED] 21389ea |
70c526a to
f1f79cd
Compare
f1f79cd to
0ef98a4
Compare
0ef98a4 to
20e779c
Compare
acc8208 to
667be42
Compare
667be42 to
874d151
Compare
874d151 to
6281cb5
Compare
6281cb5 to
39fd61b
Compare
Every new chat starts on Normal, so a workflow that always runs at Reads or Trust re-picks it in the chat footer every time. Adds agent.default_approval_mode plus a Settings control for it, applied only when a chat is genuinely minted. The offerable tiers are exactly the modes the approval_modes governance scope declares always_permitted, so the setting cannot name a deniable mode and needs no clamp. yolo is absent because it is a process-global grant with its own duration rather than a per-chat tier. Unset means normal, which is today's behaviour, so installing this changes nothing until a user moves the control. The Settings card copies YoloDurationCard in the same panel and reuses the picker's own tier labels, so the two surfaces cannot drift into two names for one tier.
Handing this off: where a standing approval grant should be storedClosing this unmerged under a standing instruction not to work security-related The findingA default-approval-mode grant is a security ceiling by this codebase's own
The migration being proposed here has a direct precedent. The computer-use The general observation, which outlives this settingA ceiling stored beside preferences inherits the protection of preferences. State of this branchThe branch is left in place and the issue remains open. The final commit on it This should not merge in its current shape. Not because the migration is That work has NOT been reviewed against this repository's security review process, |
Problem / Motivation
Every new chat starts on the Normal approval tier. A user whose workflow runs
at Reads or Trust has to re-select it in the chat footer's approval-mode picker
for each new chat, every time, and forgetting is silent: the chat simply prompts
for a tool they expected to be auto-approved.
Measured against this PR's head
c74e7ca31:website/src/store/dashboardSlice.ts:70setsapprovalMode: 'normal'src/kiro_crew/dashboard/state.pystarts a new slot withself._trust = Falseand
self._trust_reads = Falseso a new chat really does start on the interactive floor on both sides, and no
config key or Settings control existed to change it.
(
#6812's body cites line 69 for the store default; it is line 70 on thissha. The line number is not carried forward from the issue.)
Why it matters
The picker is per chat, so the cost scales with how many chats a user opens
rather than being paid once. Two separate reporters filed this independently,
which is the clearest signal that the per-chat click is the actual friction.
What changed (motivation -> approach -> change)
Goal: let a new chat start on a tier the user picks in Settings, change
nothing for a user who picks nothing, and give the setting no authority the
footer picker does not already have.
Approach. Three things already existed and decided the design:
ApprovalModePicker.tsx(APPROVAL_SEGMENTS,with
ApprovalModeKeyderived from it). This offers exactly that set minusyolo, in the order the code declares, and adds no mode.ChatPage.tsxandChatPane.tsxboth computedisplayMode = approvalMode === 'yolo' ? 'yolo' : slot?.trust ? 'trust' : slot?.trust_reads ? 'trust_reads' : 'normal', andslot_projection.pyalready puts
trustandtrust_readson the wire. So display cannot driftfrom enforcement, because it is derived from it.
agent.yolo_durationis the nearest sibling setting: same panel, same enumshape, same read/write pair. The new control is a copy of its card.
Change.
config/sections.py-- the field (defaultnormal) and_normalize_default_approval_mode, modelled on theyolo_durationsibling.config/loader.py-- wiring, one line besideyolo_duration.dashboard/handlers/core.py-- the_EDITABLE_CONFIGentry, so a Settingswrite is validated against the same two values.
dashboard/chat_handlers.py--_apply_default_approval_mode, called fromapi_chat_slot_creategated on the handler's existingis_new_slot, insidethe existing
suspend_slots_push()window so the first frame a client seesalready carries the tier.
pages/settings/SecurityPanel.tsx--DefaultApprovalModeCard, copied fromYoloDurationCardin the same panel: sameSettingsCardshell, sameradiogroup with the
data-setting-labeldeep-link anchor, the sameapi.kirocrewConfigread /api.patchConfigwrite pair on thekirocrewConfigquery, and the same pair of inlineErrorNotices.Security properties, and one thing this PR does not decide
It cannot select past an admin ceiling, and needs no clamp. The three
offerable tiers are exactly the modes the
approval_modesgovernance scopedeclares
always_permitted(platform/governance.py:1140) -- the modes a policymay never forbid.
yolois the only deniable mode and is not offerable. A clamphere would be a check that can never fail:
approval_mode_permittedshort-circuits to
Truewithout consulting governance for exactly these three,so rather than ship a decorative guard, a test pins the correspondence -- if a
tier ever becomes deniable, that test fails instead of the list silently
widening.
yolois excluded mechanically, not as a taste call.api_chat_modestatesthat
yolo"is global and ignoresslotentirely": it arms the process-globalSafetyOverride with its own duration and expiry (
agent.yolo_duration). "Thetier a new chat starts on" cannot express a process-global timed grant.
An unreadable value asks for more approvals, never fewer. A typo, a wrong
type, a removed tier, or a hand-edited
yoloall normalize tonormal, theinteractive floor. A config-read failure does the same, and the card surfaces
that rather than passing
normaloff as stored.A live chat is never re-tiered. The call is gated on
is_new_slot, which thehandler already computes and already uses for the same reason (folder-tag
inheritance must not re-stamp a chat the user is merely re-opening). Moving the
Settings control changes no chat that is already open.
Superseded: an earlier revision of this PR persisted
trustand argued that case at length.trustis no longer persistable and the granting branch is deleted, so that argument no longer describes the code; see "What changed after review" below.Scope of "new chat". The tier is applied in
api_chat_slot_create, thedashboard's own create endpoint. Chats created by other paths (fork, channel,
cron, the session-control verb) keep their existing behaviour -- notably
session_control.pyalready inherits the caller's tier, which correctly beats aglobal default for an agent-created chat.
i18n
Six keys under
pages.settings.securityPanel, added toen.manual.json(wherehand-authored English lives;
en.jsonis generated) and to all 12 othercatalogs, because
catalogParity.test.tsrequires every English key in everycatalog.
en-XA.jsonis regenerated withscripts/gen-pseudolocale.mjs, neverhand-edited.
Two of the four values needed no new translation at all:
default_approval_mode_save_failedcopies each locale's own approvedthird_party_apps_save_failed("Could not save this setting."), the identicalsentence in the identical role;
default_approval_mode_load_failedis derivedfrom each locale's approved
yolo_duration_load_failed, whose English differsonly in the noun, with the second sentence carried over verbatim.
The option labels reuse the picker's existing strings
(
components.approvalModePicker.{normal,reads,trust}_label), so the two surfacescannot drift into two names for one tier. Note the spelling split that preserves:
the declared key is
trust_readswhile its label key isreads_label.The picker's
*_descstrings were deliberately not reused --trust_descreads "In this chat, ...", which is scoped to the per-chat picker, and reusing it
in a Settings row about new chats is the cross-grammatical-role reuse the repo's
own i18n guidance forbids.
The title and description are new translations, written pronoun-free on purpose:
the existing catalogs are genuinely inconsistent about formality (de
trust_descuses "dich" while fr uses "vous"), so a second-person sentence wouldhave to pick a side per locale with no in-repo rule to cite. All 11 per-locale
style tests pass. These are AI-produced by the method the repo's own
TRANSLATION-PROMPT.mddocuments, and a native review is welcome.Three bad characters were produced and repaired, which is worth stating plainly
because none of them is visible to a key-count check. Composing the non-Latin
values with
\uescapes produced U+09DA in Bengali -- an unassigned codepoint,and two real misspellings: two Hindi words carried the wrong vowel sign (the words
for "for" and "approval"), and the Korean word for "chat" carried the wrong final
consonant. They were found by printing the values per locale rather than counting
keys. All are now literal text, and a validator asserts every codepoint in the new
values resolves to an assigned Unicode character (its control fires on a planted
U+09DA).
Tests
Backend,
test/test_default_approval_mode.py(43 cases): the offerable tiersequal the scope's
always_permittedset;yoloabsent at all threedeclarations; the three declarations agree; normalization of every offerable
tier plus
yolo/auto/interactive/reads/junk/non-strings tonormal;default is
normal; per-mode state matches the picker (normalwrites nothing,trust_reads-> read flag with policy"",trust-> trust flag with policy"auto"); a non-tier reaching the helper grants nothing.Frontend,
SecurityPanel.defaultApprovalMode.test.tsx(13 cases), a per-sectionfile matching
SecurityPanel.tailnet.test.tsx: the three declared tiers indeclared order; no
yolo; the effective value shown for unset /trust_reads/trust; an unrecognised stored value falls back to Normal; the write carries thedeclared key and never the label spelling; nothing is written until the user
moves it; read and save failures both surface.
Mutation-verified on both halves, every mutation confirmed applied, each
reddening a distinct set. Backend, six: normalizer accepts anything (8);
offerable set gains
yolo(5); default ->trust(1); helper guard neutered(5);
trust_readswrites"auto"(1);trustwrites""(1). The last twofail in opposite directions on the same assertion pair, which is what
establishes they are independent. Frontend, five: add
yolo(3); label spellingas the declared key (4); unset falls back to
trust(4); write when alreadyselected (1); drop the read-failure notice (1).
One mutation earned a code change: adding
yolodid not redden the no-YOLOassertion, because
optionLabelhas noyolocase and falls through to theNormal label, so the tier would render as "Normal" and a label-based assertion
could not see it. The radios now carry the declared key as
data-approval-modeand the test asserts the key set -- after which thatmutation reddens 3 tests instead of 2. The attribute is
data-approval-moderather than
data-modebecausedata-modeis already the theme attribute(
useTheme.tsxsets it on<html>).Gates run locally:
black,isort,flake8clean on the Python files;eslintclean on both frontend files;check-i18n-strings.mjsOK;catalogParity/deadKeys/keyReference110 tests pass; all 11 per-localestyle tests pass (83).
tsc -breports 15 errors, all@storybook/*moduleresolution in
src/stories/*-- reproduced identically on plainmainwithnone of this branch's code in the same harness (15 vs 15, empty delta), so they
belong to the local harness and not to this change.
Screenshots / video
The row in Settings -> Security -> Approval, in each of its three steady states.
The three frames differ exactly where they should, which is what shows the stored
value really drives the control rather than the frame being staged:
1. Unset -- Normal, which is the behaviour on
maintoday2.
trust_reads-- the middle tier, under its existing "Reads" label3. A stored
trustis REFUSED and renders as Normal -- the security property, photographedTwo tiers, in the order the code declares them, and no Trust or YOLO -- which is the
security property above, visible. Each tier also states what it PERMITS rather
than showing a bare word; see "Review findings addressed" below for why that
changed.
Captured by
website/scripts/capture-default-approval-mode.mjsagainst theisolated entry
website/capture/default-approval-mode.html, both included in thisPR so the frames are reproducible rather than hand-made. It follows the repo's
existing isolated-capture pattern (
capture/knowledge-ingestion-settings.*): theREAL
SecurityPanelagainst the real stylesheet, theme tokens and live i18ncatalog, with a server snapshot seeded into the same
['kirocrewConfig']query keythe card reads in production. Reaching
/settings/security/approvalthrough thefull SPA needs a gateway and a dashboard credential -- measured without one, the
shell renders its prerequisite gate and the panel never mounts, so that route
would have produced no frame at all.
Each shot waits for the radio carrying the declared key to report itself checked
before the shutter, so a frame cannot be taken before the seeded read lands -- that
race would photograph the
normalfallback for all three scenes and quietlymisrepresent two of them.
Manual verification
is how three bad characters were caught (see below); a key-count check passes on
all three.
by a script whose control fires on a planted unassigned one.
tier was confirmed by eye in each.
Review findings addressed
Both blocking findings on the first pushed head were legitimate. Neither is
argued away here.
Security (GPT 5.6, upheld-fenced): an app token must not inherit a
trustdefault.
POST /api/chat/slotsserves two principals -- a human on thedashboard new-chat tab (
request["app"] == "") and an app token holding/api/chat(non-empty). I had gated the tier onis_new_slotonly, so anoperator who set
trustwould have had app-created sessions start with theautoapproval policy that
parent_trustedthen extends to their subagents: anauto-approve grant no human clicked, for a principal whose grant does not include
it. Fixed by adding
not request_appto BOTH the config read and the apply site-- restated at the apply site deliberately, because that is the line that grants
authority and a reader should not have to trace back for the constraint.
not request_appis this module's existing spelling for a human dashboard caller(the slot-ownership check, and the
directive_user_originsites), and this veryhandler already reasons the same way about app-token callers for session counting.
This also makes the code match the scope this PR already claimed.
Pinned by three tests that drive the REAL handler as each principal, because a
helper-level test would stay green with the guard deleted: a dashboard caller gets
the tier (the control -- without it the denial proves nothing), an app token does
not, and no
autosession policy is written for an app-created session. Deletingboth conjuncts reddens the two app-token tests and leaves the control green.
UX (Fable 5, BLOCK): the tiers were three bare words. The card rendered only
normal_label/reads_label/trust_label, so the control whose whole purposeis choosing a tier never said what any tier permits -- the reviewer's blind reader
declined to select Trust without knowing what it allowed. That was my error, and
specifically an over-correction: I dropped the picker's descriptions because
trust_descis scoped to one chat ("In this chat, ..."), which is wrong on a rowabout new chats -- but I then shipped nothing in their place, which is worse than
either option.
Corrected by looking at the three strings rather than generalising from one:
normal_descandreads_desccarry no chat scope, so they are reusedverbatim -- zero new translations, and the two surfaces cannot drift into two
explanations of one tier. Only trust needed a new key, and its value is DERIVED in
each locale from that locale's own approved
trust_descby removing the leadingscope clause, so 10 of 12 are a verbatim tail of approved copy (asserted in the
script, not by eye). Two are declared exceptions with reasons: German, because
main clauses are verb-second so the verb must move behind its subject once the
fronted phrase goes; Portuguese, because the subject's article must be capitalised
once it starts the sentence.
A first attempt to derive all twelve mechanically -- the longest common suffix of
each locale's
trust_descandyolo_desc, which differ only in scope -- wasmeasured and discarded: because both scope phrases end in the same noun, the
suffix cut mid-phrase ("em Chat arbeitet ...", "om chate ..."). Printing the
candidates is what caught it. Printing them also caught a fourth bad value in this
round: the Korean strip clause was too short and left a dangling "in chat, ..."
fragment that still carried the scope.
Advisory, not addressed here, and flagged so it is not lost: the reviewer notes
the card sits under the section heading "YOLO (auto-approve)" although the setting
is not YOLO and pointedly excludes it. That heading is shared with the existing
YOLO duration row, so renaming it or adding a section is a change to a surface
this PR does not otherwise touch -- it belongs in its own change rather than
riding along here. The reviewer also asks for a frame of the two error states;
also deferred, for the same reason the harness seeds a successful read.
Second review round
Four more blocking findings, all upheld and all fixed. None argued away.
An app token was not the only principal gap: a REMOTE-BOUND session was too.
A session can be bound to a remote crew (
instance_idon the create body), andthen the PEER enforces approvals. Applying the local default set the flags the
footer DISPLAYS while the peer decided what actually RUNS, so a peer at Normal
could display Trust here, or the reverse. Mirroring the peer's effective tier needs
a defined path to read it and there is none --
create_peer_slotreturns no tier-- so the handler now declines for remote-bound creates and a remote session keeps
whatever the peer says. That is the "undefined path" case stated rather than
guessed.
not instance_idwas already the idiom at two other sites in thishandler. Pinned by a test that drives the create as the owner (the only principal
that can bind at all) and reddens when the conjunct is removed.
**Persisting the default is now owner-only.**The owner check now lives on the dedicated handler.I violated an explicit in-tree instruction and CI caught it.
loader.py'sfrom ...sections import (...)block is a FROZEN pre-split snapshot, and thecomment directly above it says a coercer added after the split "must not join it";
test_config_module_boundariespins that set. I had added the normalizer to theblock. It now goes through
_sections._normalize_default_approval_mode(...), thepattern that comment mandates and that
_sections._safe_avataralready uses.config-baseline.jsonwas stale. Adding a schema field requires regeneratingthe committed baseline in the same change -- the failure message says so and names
the script. Regenerated with
scripts/generate_config_baseline.py: +19 lines, allof them this one field's entry.
UX, second round: the trust description named an outcome but no scope and no
worst case. "Kiro Crew works without asking you first" left a reader unwilling to
select the tier the feature exists to offer, and a standing default for every
future chat is higher-stakes than the per-chat picker the wording came from. It now
reads "Runs commands and edits files in every new chat without asking" -- scope and
consequence both stated. This one could NOT be derived from approved copy, because
it says more than any existing string does, so these are 12 new translations rather
than tails of existing ones; the codepoint validator and the 11 per-locale style
tests still gate them, and native review is welcome.
Also updated the feature map's
securityrow, which listed no config endpoint eventhough this setting writes through
PATCH /api/config/kirocrew.One failure in that run is NOT this branch's.
test_snapshot.py::TestNotificationCopyWhenNoLiveFileExists::test_a_note_delivered_during_the_copy_survives_the_READERfailed on an ordering assertion ("the append ran while the copy was still writing").
It passes locally (25/25 in that class), and this diff touches only the config
modules,
chat_handlers.pyandhandlers/core.py-- nothing a notification-copyconcurrency test reaches. Reported rather than adopted.
Third review round
The owner gate had to cover INHERITING the tier, not just persisting it. Making
the write owner-only left the other half open: a non-owner dashboard identity -- an
allow-listed messaging identity holds a dashboard credential whose subject is not
the owner and whose
appclaim is EMPTY, so the app-token guard alone admits it --would have picked up the owner's standing auto-approve grant just by opening a chat.
Both the config read and the apply site now also require a positive owner
assertion, and a non-owner keeps the interactive floor. That is the third principal
this feature had to be narrowed against (app token, remote peer, non-owner
dashboard user); each was found by review rather than by me, which is worth saying
plainly.
The feature map now also names
chat_handlers.pyandPOST /api/chat/slots-- therow previously listed only where the value is WRITTEN, not where it is ENFORCED.
One red is not a test failure.
Frontend Tests (3)failed with a runner error,and the
Frontend Coverage Mergeannotation states it directly: "No test in themerged shard results reported failing ... the exit came from 1 unhandled runner
error(s) ... not a test assertion. See the documented worker-OOM failure mode in
website/vite.config.ts." That file documents the kernel OOM-killing a worker under
--coverage, which vitest surfaces as a job failure "even though every testpassed". My test file's renders are auto-cleaned (
globals: trueenablestesting-library's afterEach cleanup) so it is not retaining panels, and its 13
renders are a marginal addition to a ~950-file suite.
Coverage Gateis downstreamof that shard, failing closed. I have not restructured working tests to chase it.
Fourth review round -- a race this change introduced
Upheld and fixed: the creation verdict could go stale across my own await.
is_new_slotis a snapshot ofstate._slots, and reading the config for thisfeature put an
awaitbetween that check andget_or_create_slot. That is a TOCTOUwindow: a concurrent create can insert the same slot key while the coroutine is
suspended, after which the allocation returns the OTHER caller's slot while the
stale verdict still reports it fresh -- landing an owner-tier auto-approve grant on
a session this request did not create. My own comment admitted the yield was "awaits
this change introduced"; I had reasoned about where the await was cheapest, not about
what it exposed.
Fixed by REMOVING the yield rather than narrowing the window: the config read is
hoisted above the newness check, so the check and the allocation are once again
atomic with respect to the event loop -- which is what the handler relied on before
this change. Verified mechanically: zero
await/async withbetween the two lines.The reviewer's suggested fix was to recompute
is_new_slotafter the await; hoistingis strictly stronger, because a recomputed boolean can go stale again the moment
another await is added.
Defence in depth alongside it: the apply site now also requires the slot's own
_apptag to be empty. That asserts the property on the OBJECT rather than on are-derived boolean, so an app-owned slot can never be tiered whatever the newness
verdict says.
Two tests pin this, both mutation-verified: an ordering test that fails if any
awaitis reintroduced into the window (source-level on purpose -- the property isabout scheduling points, which a behavioural test cannot observe without racing the
loop it asserts about), and one for the ownership check. Injecting
await asyncio.sleep(0)after the check reddens the first; deleting the_appconjunct reddens the second.
While making that move I also removed the unconditional initialisation of
_default_approval_modeand only noticed because I checked: the tests still passed,because the apply condition short-circuits before reading the name. Relying on
conjunct order for name binding is a trap for the next edit, so the initialisation
is back and asserted to be at function-body depth.
Fifth round: a formatting miss, and a red I mis-diagnosed twice
test/test_default_approval_mode.pywas not black-formatted. My local check passedbecause CI pins
--target-version py310and my invocation did not.The other red was
Backend Tests (3.12, 4)failing on ordering assertions intest_snapshot.py::TestNotificationCopyWhenNoLiveFileExists. I reported it as thebase's, which was correct, and I was right that this PR cannot cause it -- but I drew
the wrong conclusion about what to DO, twice, and it cost several cycles:
the same class, which is what makes it a class-level timing problem.
ALREADY BEEN FIXED on main, in
d7fb9c55e("read the copy ordering before theworker is released", fix(test): read the copy ordering before the worker is released #9047) and
36ffa67ea("scope the notification-copy orderingtriggers to the destination open", flaky: TestNotificationCopyWhenNoLiveFileExists ordering asserts race on loaded CI shards #8893/fix(test): scope the notification-copy ordering triggers to the destination open #8992). This branch was 49 commits behind
and simply did not have them.
The lesson is the ordinary one: I checked whether the failure was MINE, and stopped
there, instead of checking whether the base had already moved past it. Rebasing onto
current main was the fix the whole time.
Rebased onto current main
Now a single commit on top of
main, zero commits behind. Re-verified after therebase rather than assuming a clean rebase means a working branch -- 49 commits of
main had landed under it, including config-schema and catalog churn that could have
invalidated the generated files this PR touches:
test_config_baseline.py+test_config_module_boundaries.py: pass, so theregenerated
config-baseline.jsonis still in parity with the schema on the NEW mainTestNotificationCopyWhenNoLiveFileExists: 26 pass, now carrying main's fixesen-XA.jsonregenerated and byte-compared: no driftblack --target-version py310,flake8, feature-map gate: cleanSixth round: two more, both upheld and both fixed
An app-worker slot is not a chat the user opened. Design Critique's
dc-*workeris created by the OWNER's own page -- same-origin, no app token, empty app tag -- so
it passed every principal gate I had built. What it lacks is a user: its
design-critiquemode exists specifically to keep it OFF the chat sidebar, so astanding auto-approve grant there runs tools with nobody watching.
Fixed as an ALLOWLIST rather than by excluding that one mode:
_TIER_INHERITING_MODES = ("", "orchestrator", "crew")-- exactly the modesChatPage'sfilteredSlotsrenders. A denylist would have to be amended for everyfuture app-worker mode and would fail open if someone forgot; the allowlist fails
closed, so a new mode does not inherit the tier until it is deliberately listed. A
test pins the allowlist as a strict subset of
_CREATABLE_MODESso the two cannotsilently converge.
A grant that cannot be audited is now refused. The tier was applied as a
conjunct BEFORE the SEL write, whose failure was swallowed -- so an unwritable SEL
(disk full) left trust active with its audit entry permanently lost. The audit now
runs FIRST and the grant happens only in the
elsebranch, so a failed audit leavesthe session on the interactive floor. That is the fail-safe direction and it matches
how this feature already treats an unreadable config.
Two deliberate deviations, stated rather than hidden.
api_chat_modeswallows thesame failure for the equivalent explicit click; that parity is not a reason to
swallow it here, because a click has a human witness and this grant has none, so this
path is deliberately stricter than its sibling. And the audit stays SYNCHRONOUS
rather than moving off-loop as suggested: an
awaitthere would sit insidesuspend_slots_push(), and removing an await from this region is exactly what closedthe stale-verdict race in the previous round.
Both mutation-verified: adding
design-critiqueto the allowlist reddens two tests,and restoring grant-then-audit ordering reddens the audit test.
Also confirmed this round: the
Backend Tests (3.12, 4)failure documented abovePASSED on re-run at the same code, which is the last piece of evidence that it was
the base's flake rather than this branch's.
Scope: what this delivers, and what it does NOT
trustis not persistable. Onlynormalandtrust_readscan be stored. This isa security boundary, not a scoping preference, and it is why the closing keywords below
are deliberately absent.
trustis the only tier that also writes the sessionapproval_policy"auto"--unattended tool auto-approve, which
parent_trustedextends to spawned subagents.config.jsonis agent-writable (agent.pycalls it LLM-writable) and, althoughis_sensitive_write_pathreturns True for it, that write is auto-approved inside asession that is ALREADY trusted. A persistable
trustwould therefore let onesession's trust become STANDING trust for every future session. Leaving it
unpersistable removes that mechanism rather than discouraging it.
Consequences, stated plainly so nobody reads a merge as more than it is:
works at Reads no longer re-picks it per session. Its Trust case is NOT
delivered.
default ("the trust level must be manually changed to 'Trust' every time"), and that
is exactly what this refuses to persist. Its request is not met by this change.
PERSISTING them is refused. A test pins that
_SLOT_SCOPED_TRUST_MODESkeepstrust, so a later reader does not "tidy" the two sets into agreement.The clamp is enforced on READ, so a value that reaches
config.jsonby any route --hand edit, an agent write, a downgrade from a future version -- is not honoured. That
is measured through the REAL loader rather than a mock: the test writes an actual
config.jsoncontainingtrust(andyolo, and cased/whitespaced variants) andasserts
KiroCrewConfig.load()reportsnormal. It carries a control that a storedtrust_readsIS honoured, because without it every assertion would pass vacuously --normalis also the default, so a loader that never read the file would look correct.Governance was deliberately NOT touched: no ordering was added to
_ORDINAL_SCALES,and
approval_modes.always_permittedis unchanged. The set-parity test was relaxedfrom equality to a SUBSET for that reason -- every persistable tier must still be one
policy may never forbid, but
trustis now excluded for the stricter reason above.Two type domains, kept separate
Narrowing the persistable set surfaced one TypeScript error --
Type '"trust"' is not comparable to type '"normal" | "trust_reads"'atSecurityPanel.tsx(905,12)-- which took down seven lanes at once, since every one ofthem typechecks.
Worth being explicit about what it was NOT: it was not a signal to remove Trust from
the UI. A session can still be set to Trust interactively and that is existing shipping
behaviour. The two domains are already distinct symbols in this codebase:
ApprovalModeKey, fromAPPROVAL_SEGMENTS(ApprovalModePicker.tsx:27) -- theRUNTIME vocabulary the footer picker renders. Unchanged by this PR; the diff does not
touch that file at all.
DefaultApprovalModeKey, fromDEFAULT_APPROVAL_MODE_KEYS(
SecurityPanel.tsx:870) -- the PERSISTABLE set, and the only one narrowed.SecurityPanel.tsxnever imports the runtime type, so no shared type was shrunk. Theerror was a now-unreachable
case 'trust'inside this file's ownoptionLabel, aSettings-only helper: dead code, removed.
tsc -bnow reports 15 errors, all@storybook/*resolution insrc/stories/*, zero inSecurityPaneland zero outsidesrc/stories/-- the same 15 that reproduce on plain main in this harness.A test pins the capability so this cannot regress on someone's memory: it asserts
APPROVAL_SEGMENTSstill containstrust, and that the persistable set is a strictsubset of the runtime one. The backend has the mirror assertion on
_SLOT_SCOPED_TRUST_MODES.What changed after review, and why the earlier prose was wrong
Three advisory reviewers independently found the same thing, and they were right:
with
trustout of the persistable set I had claimed the escalation was answered"by construction". Read literally that was FALSE of the shipped code.
_apply_default_approval_modestill carried a livetrustbranch writingset_approval_policy(session_key, "auto"). The grant was only UNREACHABLE -- correctfor exactly as long as nothing could supply
trust-- and a later widening of thepersistable set would have revived an unattended auto-approve grant with no test
failing. Unreachable is a weaker property than absent.
So the branch is DELETED, not guarded. I chose deletion over a hard-fail
deliberately: a hard-fail leaves the granting write in the file, still one edit from
live, whereas deleting the machinery makes the property structural. The helper now
clamps on
mode != "trust_reads"rather than membership in_SLOT_SCOPED_TRUST_MODES(that tuple is the PER-CHAT vocabulary and still containstrust, so testing against it would tie this path's reach to a set kept for anotherpurpose), and asserts
mode in _DEFAULT_APPROVAL_MODES. Two tests: the helperrefuses
trustand writes nothing, and a source-level assertion that no"auto"policy write exists in the function at all -- so a re-added branch fails.
Interactive Trust is untouched:
api_chat_modeowns that path and writes its ownpolicy, this helper has exactly one caller, and the diff does not touch
ApprovalModePicker.tsx.Four documentation-versus-code contradictions of mine were fixed in the same pass,
and the tests assert the SUPERSEDED phrasing is gone rather than only that the new
phrasing is present:
sections.pysaid "the two persistable tiers are exactly themodes ...
always_permitted" above a two-element tuple (now "the TWO ... a strictSUBSET", with the reason
trustis excluded separately);core.pysaid "the threevalues here"; and this body argued the three-tier design including the subagent
consequence of a persisted
trust.Two UX findings, both fixed:
("New chat defaults"), because it is not a YOLO setting and pointedly excludes
YOLO -- filing it there hid it from anyone scanning for a new-chat default and
implied it grants auto-approve.
without asking, so it stays a per-chat choice made in the message box." The Add a global default trust level for new chat sessions #8418
users this control exists for will look for Trust; they should learn the answer
from the product rather than by filing again. Given this PR declines their actual
request, that sentence is the least it owes them.
Both error states are now photographed, and each frame ASSERTS its own notice text
before the shutter rather than after -- so a frame cannot be a plausible-looking
picture of a card that rendered nothing:
Read failure (also showing the Trust note)
Save failure
Both are produced by the REAL failure paths in the isolated harness (an unseeded
query whose fetch fails; a real click whose patch fails), not by faking a notice.
Two more UX fixes, and a self-contradiction each
A failed read now selects nothing. The radiogroup used to keep
border-accentanda green tick while the notice said the highlighted option was the default rather than
what is stored -- a frame disagreeing with its own copy. On
cfgErrorthe effectivevalue is
null, so no option renders selected.Fixing that invalidated the notice's own premise, so the copy changed too: it said
"The highlighted option is the default, not what is stored" when there is no longer a
highlight. It now reads "Could not load the saved mode, so no option is selected."
Updated in all 13 catalogs, with an assertion that no locale still references a
highlight. Same discipline as the code: after a behavioural change, grep the prose for
the premise you invalidated.
A clamped value is no longer refused silently. A stored
trustis clamped tonormalon read, and the frame for it was pixel-identical to the unset frame -- sowhoever wrote that value got no cue it was ignored. The card now says: The value "trust"
stored for this setting is not applied here, so new chats start on Normal. The stored
value is QUOTED because unquoted it read as the ordinary English word rather than a
named setting -- in the one state where the reader most needs to parse it exactly.
Japanese uses the corner brackets and Korean the single quotation marks their style
rules require; the repo's own per-locale style tests reject ASCII quotes there and
caught the naive version. Three tests cover the cue, including the control that a
persistable stored value shows NO cue -- without which a cue rendered
unconditionally would pass.
What a declined default does and does not tell you
Refusal observability here is PARTIAL, and the shipped code reaches only two of the
seven cases I first wrote. A reviewer asked for a declined default to be
attributable, I named all seven guards, and then re-read the resolution order: the
config is READ only when the caller holds no app token, is not remote-bound, IS the
owner, and is in a sidebar-visible mode. A caller failing any of those never resolves
a tier at all, so there is nothing to refuse and nothing to log. The loader also
clamps on read, so an unpersistable stored value arrives as
normaland cannot bereported either. Five of the seven branches were therefore dead code asserting
coverage that did not exist, and they are deleted rather than left in as decoration.
Two refusals are reachable and are logged with the slot key and the reason: a
RESUMED session, which resolves a tier because the read deliberately precedes the
newness check (that hoist is what closed the TOCTOU in an earlier round), and a slot
an app-token create won by race. A test pins the reachable pair and a second asserts
the five dead names are ABSENT from the executable lines -- with a control proving it
fires when one is re-added, and with comments stripped, because the explanation of why
they are unreachable names them and would otherwise satisfy its own assertion.
The gap that leaves: a non-owner dashboard identity gets the interactive floor on
every chat, with no signal anywhere. They keep paying the per-chat click this PR
exists to remove and cannot discover why. That is a real limitation, not a rounding
error, and closing it would require reading the owner's setting on behalf of a
principal forbidden to use the result -- so it is recorded here and in the config
system spec rather than implied away. A narrow claim that is true beats a broad one
that reads well.
Where this grant's authority comes from
The tier is no longer stored in
config.json. It lives in a keystone leaf theagent cannot write. A review found that the standing grant was read from an
agent-writable store, which meant an agent could grant itself the approval mode
this PR is about. That is a privilege boundary rather than hygiene, so the fix is
placement, not logic.
The codebase had already proven the premise, and already decided the pattern.
security.pyrecords the verification in its own comment: a security ceiling keptin
config.json"would leave it writable by any auto-approved agent shell(verified:
is_sensitive_bash_command("echo x > .../config.json")is None)".config.jsonIS in_WRITE_PROTECTED_HOME_PATHS, but that covers the agent toolpath only -- the shell path is not caught, and protected on one path only is not
protected. The computer-use primary enable lived in
config.jsonand was movedinto
_CREW_SECRET_LEAVESfor exactly this reason, with the principle named inthat comment: "a security ceiling, not a preference." Browser Mode's enable gate
followed. A standing approval tier is the same class of control, so this follows a
decided pattern rather than inventing a trust boundary.
Reproduced here, which is the evidence that the swap fixes the defect rather than
relocating it:
default_approval_mode.jsonconfig.jsonecho x > <leaf>echo x | tee <leaf>cat <leaf>Read AND write protected, not write-only: the stored tier tells an agent whether
the sessions around it are already elevated, which is reconnaissance for choosing
what to attempt.
The read fails closed and never consults
config.jsonon any path. An absentleaf, a malformed leaf, a non-string value, and a value outside the persistable
set all resolve to
normal, the interactive floor. There is deliberately nofallback: a fallback would keep the feature working while ungated, which is worse
than the feature being inert. The clamp is on READ as well as write, so
hand-editing the leaf is useless as an escalation and the guarantee does not rest
on the writer alone. Two tests pin this -- one writes the grant into the
agent-writable
config.jsonand asserts the feature stays OFF, one asserts theabsent-leaf path yields no trust flags -- and the first is mutation-verified by
reintroducing a
config.jsonfallback and watching it go red.The dashboard PUT handler is the only writer, and it opens the path directly
via
atomic_write(..., mode=0o600)rather than through the agent tool gate, whichis what lets the operator's Settings card write a file the agent's own file and
bash tools are refused. Two deliberate divergences from the
computer_use.jsonsibling, both stricter: that handler gates on the app-token check alone, while
this one also requires
is_owner_dashboard_request, because an allow-listedmessaging identity carries a dashboard credential whose subject is not the owner
and whose
appclaim is EMPTY -- the app-token guard alone would admit it to astanding grant. And the GET is gated too, since the stored value is
reconnaissance.
The owner check now lives on the dedicated handler. It existed to
stop a non-owner writing the tier through the generic config PATCH. With the tier
out of
config.jsonentirely, that endpoint can no longer reach it, no other keyused the flag, and an unused gate asserts a protection nothing exercises. The
owner check it performed now lives on the dedicated handler, where it is the
thing actually guarding the write.
What the swap deleted, not just moved:
default_approval_modeleavesAgentConfig, its vocabulary tuple and its normalizer leaveconfig/sections.py,the loader's coercer call goes,
config-baseline.jsonis regenerated without thekey, and the frontend card stops calling
/api/config/kirocrew. Removing thefield also orphaned a comment block in
sections.pythat described "the TWO thatremain" and was left dangling above an unrelated constant; that reasoning moved
to the module which now owns the tuple, rather than being left to describe the
wrong symbol.
On #8418, plainly
#8418 is not satisfied by this PR and should not be read as closed by it. It asks
specifically for Trust as the sticky global default ("the trust level must be manually
changed to 'Trust' every time"), and persisting Trust is exactly what this refuses.
The trailer is a bare
Refsfor that reason. #6812 is half addressed: its Reads caseworks, its Trust case does not.
The override cue first shipped with
text-warning, which the phantom-classes gatecorrectly rejected: the theme declares no
warningtoken, so Tailwind emitted no ruleand the line rendered with no colour at all. It now uses
text-warn, the declaredtoken 356 other call sites use. Worth recording that the defect was visible in the
frame I had already looked at -- the cue was plain white rather than amber -- and I did
not question it; the gate caught what my own eyes had passed over.
The Windows shard-4 red is not this PR's
Backend Tests (Windows) (4)failedtest_work_ledger.py::test_two_conductors_binding_one_worker_at_once_yield_exactly_one_bindingwith
assert 1 == 3. Diagnosed on evidence rather than by re-running:066117b83(work-ledger phase 1, feat(work-ledger): store module for conductor/worker work items (phase 1) #8855) after this branch's base, and CI evaluates the MERGE
commit -- so the test ran from main's side of the merge, not from this diff.
test/test_work_ledger.pycontains zero referencesto
kiro_crew.config,dashboard/chat_handlers,dashboard/handlers/core, ordefault_approval-- there is no path from this diff to that assertion. This PRdoes not touch
work_ledger.py.outcomes.count(CODE_ALREADY_BOUND) == 3, which got 1. Four conductors race to bind one worker; one should win andthree should be told ALREADY_BOUND. Getting 1 means two losers appended NO outcome
at all, so they raised something the test's
except wl.WorkLedgerErrordoes notcatch. That is an exception-type gap on Windows, not a slow runner -- consistent
with it being a count rather than a duration.
Reported rather than patched: it is newly-landed base code and its owners should see
the Windows gap. This branch has also been rebased onto current main so it now
contains that test.
Related Issues
Refs #6812
Refs #8418
Refs #8987
Bare
Refs, not closing keywords, and that is deliberate: #6812 is only halfaddressed and #8418 is not addressed at all, so an automatic close would misreport
both. #8987 (the missing Settings control) is superseded by this PR, but it is left
open rather than auto-closed so a human decides that alongside the scope above.
Read for constraints: #8848, #8849, #9105.
Checklist
feat: ...)label and help text, which is what
GET /api/config/schemaservesContribution License Agreement
Placeholder per the template -- CLA wording to be supplied by OSPO.