Skip to content

feat: let a new session start on a configured approval tier - #8984

Closed
chenmingwei23 wants to merge 1 commit into
mainfrom
feat/default-approval-mode-6812
Closed

feat: let a new session start on a configured approval tier#8984
chenmingwei23 wants to merge 1 commit into
mainfrom
feat/default-approval-mode-6812

Conversation

@chenmingwei23

@chenmingwei23 chenmingwei23 commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

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:70 sets approvalMode: 'normal'
  • src/kiro_crew/dashboard/state.py starts a new slot with self._trust = False
    and self._trust_reads = False

so 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 this
sha. 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:

  1. The tiers are declared once, in ApprovalModePicker.tsx (APPROVAL_SEGMENTS,
    with ApprovalModeKey derived from it). This offers exactly that set minus
    yolo, in the order the code declares, and adds no mode.
  2. The footer already renders its tier from the slot's own state:
    ChatPage.tsx and ChatPane.tsx both compute
    displayMode = approvalMode === 'yolo' ? 'yolo' : slot?.trust ? 'trust' : slot?.trust_reads ? 'trust_reads' : 'normal', and slot_projection.py
    already puts trust and trust_reads on the wire. So display cannot drift
    from enforcement, because it is derived from it.
  3. agent.yolo_duration is the nearest sibling setting: same panel, same enum
    shape, same read/write pair. The new control is a copy of its card.

Change.

  • config/sections.py -- the field (default normal) and
    _normalize_default_approval_mode, modelled on the yolo_duration sibling.
  • config/loader.py -- wiring, one line beside yolo_duration.
  • dashboard/handlers/core.py -- the _EDITABLE_CONFIG entry, so a Settings
    write is validated against the same two values.
  • dashboard/chat_handlers.py -- _apply_default_approval_mode, called from
    api_chat_slot_create gated on the handler's existing is_new_slot, inside
    the existing suspend_slots_push() window so the first frame a client sees
    already carries the tier.
  • pages/settings/SecurityPanel.tsx -- DefaultApprovalModeCard, copied from
    YoloDurationCard in the same panel: same SettingsCard shell, same
    radiogroup with the data-setting-label deep-link anchor, the same
    api.kirocrewConfig read / api.patchConfig write pair on the
    kirocrewConfig query, and the same pair of inline ErrorNotices.
  • 13 locale catalogs -- four new keys.

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_modes governance scope
declares always_permitted (platform/governance.py:1140) -- the modes a policy
may never forbid. yolo is the only deniable mode and is not offerable. A clamp
here would be a check that can never fail: approval_mode_permitted
short-circuits to True without 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.

yolo is excluded mechanically, not as a taste call. api_chat_mode states
that yolo "is global and ignores slot entirely": it arms the process-global
SafetyOverride with its own duration and expiry (agent.yolo_duration). "The
tier 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 yolo all normalize to normal, the
interactive floor. A config-read failure does the same, and the card surfaces
that rather than passing normal off as stored.

A live chat is never re-tiered. The call is gated on is_new_slot, which the
handler 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 trust and argued that case at length. trust is 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, the
dashboard's own create endpoint. Chats created by other paths (fork, channel,
cron, the session-control verb) keep their existing behaviour -- notably
session_control.py already inherits the caller's tier, which correctly beats a
global default for an agent-created chat.

i18n

Six keys under pages.settings.securityPanel, added to en.manual.json (where
hand-authored English lives; en.json is generated) and to all 12 other
catalogs, because catalogParity.test.ts requires every English key in every
catalog. en-XA.json is regenerated with scripts/gen-pseudolocale.mjs, never
hand-edited.

Two of the four values needed no new translation at all:
default_approval_mode_save_failed copies each locale's own approved
third_party_apps_save_failed ("Could not save this setting."), the identical
sentence in the identical role; default_approval_mode_load_failed is derived
from each locale's approved yolo_duration_load_failed, whose English differs
only 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 surfaces
cannot drift into two names for one tier. Note the spelling split that preserves:
the declared key is trust_reads while its label key is reads_label.
The picker's *_desc strings were deliberately not reused -- trust_desc
reads "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_desc uses "dich" while fr uses "vous"), so a second-person sentence would
have 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.md documents, 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 \u escapes 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 tiers
equal the scope's always_permitted set; yolo absent at all three
declarations; the three declarations agree; normalization of every offerable
tier plus yolo/auto/interactive/reads/junk/non-strings to normal;
default is normal; per-mode state matches the picker (normal writes 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-section
file matching SecurityPanel.tailnet.test.tsx: the three declared tiers in
declared order; no yolo; the effective value shown for unset / trust_reads /
trust; an unrecognised stored value falls back to Normal; the write carries the
declared 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_reads writes "auto" (1); trust writes "" (1). The last two
fail in opposite directions on the same assertion pair, which is what
establishes they are independent. Frontend, five: add yolo (3); label spelling
as the declared key (4); unset falls back to trust (4); write when already
selected (1); drop the read-failure notice (1).

One mutation earned a code change: adding yolo did not redden the no-YOLO
assertion, because optionLabel has no yolo case and falls through to the
Normal 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-mode and the test asserts the key set -- after which that
mutation reddens 3 tests instead of 2. The attribute is data-approval-mode
rather than data-mode because data-mode is already the theme attribute
(useTheme.tsx sets it on <html>).

Gates run locally: black, isort, flake8 clean on the Python files;
eslint clean on both frontend files; check-i18n-strings.mjs OK;
catalogParity / deadKeys / keyReference 110 tests pass; all 11 per-locale
style tests pass (83). tsc -b reports 15 errors, all @storybook/* module
resolution in src/stories/* -- reproduced identically on plain main with
none 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 main today

Default approval mode, unset, Normal selected

2. trust_reads -- the middle tier, under its existing "Reads" label

Default approval mode set to Reads

3. A stored trust is REFUSED and renders as Normal -- the security property, photographed

A stored trust is refused and shows Normal

Two 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.mjs against the
isolated entry website/capture/default-approval-mode.html, both included in this
PR so the frames are reproducible rather than hand-made. It follows the repo's
existing isolated-capture pattern (capture/knowledge-ingestion-settings.*): the
REAL SecurityPanel against the real stylesheet, theme tokens and live i18n
catalog, with a server snapshot seeded into the same ['kirocrewConfig'] query key
the card reads in production. Reaching /settings/security/approval through the
full 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 normal fallback for all three scenes and quietly
misrepresent two of them.

Manual verification

  • All four i18n values were printed and inspected per locale, not counted. That
    is how three bad characters were caught (see below); a key-count check passes on
    all three.
  • Every codepoint in the new values is validated as an assigned Unicode character
    by a script whose control fires on a planted unassigned one.
  • The three frames above were opened and read, not just written -- the selected
    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 trust
default.
POST /api/chat/slots serves two principals -- a human on the
dashboard new-chat tab (request["app"] == "") and an app token holding
/api/chat (non-empty). I had gated the tier on is_new_slot only, so an
operator who set trust would have had app-created sessions start with the auto
approval policy that parent_trusted then extends to their subagents: an
auto-approve grant no human clicked, for a principal whose grant does not include
it. Fixed by adding not request_app to 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_app is this module's existing spelling for a human dashboard caller
(the slot-ownership check, and the directive_user_origin sites), and this very
handler 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 auto session policy is written for an app-created session. Deleting
both 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 purpose
is 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_desc is scoped to one chat ("In this chat, ..."), which is wrong on a row
about 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_desc and reads_desc carry no chat scope, so they are reused
verbatim -- 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_desc by removing the leading
scope 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_desc and yolo_desc, which differ only in scope -- was
measured 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_id on the create body), and
then 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_slot returns 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_id was already the idiom at two other sites in this
handler. 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's
from ...sections import (...) block is a FROZEN pre-split snapshot, and the
comment directly above it says a coercer added after the split "must not join it";
test_config_module_boundaries pins that set. I had added the normalizer to the
block. It now goes through _sections._normalize_default_approval_mode(...), the
pattern that comment mandates and that _sections._safe_avatar already uses.

config-baseline.json was stale. Adding a schema field requires regenerating
the committed baseline in the same change -- the failure message says so and names
the script. Regenerated with scripts/generate_config_baseline.py: +19 lines, all
of 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 security row, which listed no config endpoint even
though 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_READER
failed 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.py and handlers/core.py -- nothing a notification-copy
concurrency 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 app claim 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.py and POST /api/chat/slots -- the
row 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 Merge annotation states it directly: "No test in the
merged 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 test
passed". My test file's renders are auto-cleaned (globals: true enables
testing-library's afterEach cleanup) so it is not retaining panels, and its 13
renders are a marginal addition to a ~950-file suite. Coverage Gate is downstream
of 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_slot is a snapshot of state._slots, and reading the config for this
feature put an await between that check and get_or_create_slot. That is a TOCTOU
window: 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 with between the two lines.
The reviewer's suggested fix was to recompute is_new_slot after the await; hoisting
is 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
_app tag to be empty. That asserts the property on the OBJECT rather than on a
re-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
await is reintroduced into the window (source-level on purpose -- the property is
about 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 _app
conjunct reddens the second.

While making that move I also removed the unconditional initialisation of
_default_approval_mode and 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.py was not black-formatted. My local check passed
because CI pins --target-version py310 and my invocation did not.

The other red was Backend Tests (3.12, 4) failing on ordering assertions in
test_snapshot.py::TestNotificationCopyWhenNoLiveFileExists. I reported it as the
base'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:

  1. First I said "same test each time". Wrong: a re-run failed a DIFFERENT assertion in
    the same class, which is what makes it a class-level timing problem.
  2. Then I treated it as an open flake needing a ruling. Also wrong: the flake had
    ALREADY BEEN FIXED on main, in d7fb9c55e ("read the copy ordering before the
    worker is released", fix(test): read the copy ordering before the worker is released #9047) and 36ffa67ea ("scope the notification-copy ordering
    triggers 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 the
rebase 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:

  • the 55 tests in this PR's backend file: pass
  • test_config_baseline.py + test_config_module_boundaries.py: pass, so the
    regenerated config-baseline.json is still in parity with the schema on the NEW main
  • TestNotificationCopyWhenNoLiveFileExists: 26 pass, now carrying main's fixes
  • frontend card tests 17, i18n suite 664, per-locale style tests 83: pass
  • en-XA.json regenerated and byte-compared: no drift
  • black --target-version py310, flake8, feature-map gate: clean

Sixth round: two more, both upheld and both fixed

An app-worker slot is not a chat the user opened. Design Critique's dc-* worker
is 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-critique mode exists specifically to keep it OFF the chat sidebar, so a
standing 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 modes
ChatPage's filteredSlots renders. A denylist would have to be amended for every
future 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_MODES so the two cannot
silently 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 else branch, so a failed audit leaves
the 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_mode swallows the
same 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 await there would sit inside
suspend_slots_push(), and removing an await from this region is exactly what closed
the stale-verdict race in the previous round.

Both mutation-verified: adding design-critique to 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 above
PASSED 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

trust is not persistable. Only normal and trust_reads can be stored. This is
a security boundary, not a scoping preference, and it is why the closing keywords below
are deliberately absent.

trust is the only tier that also writes the session approval_policy "auto" --
unattended tool auto-approve, which parent_trusted extends to spawned subagents.
config.json is agent-writable (agent.py calls it LLM-writable) and, although
is_sensitive_write_path returns True for it, that write is auto-approved inside a
session that is ALREADY trusted. A persistable trust would therefore let one
session'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:

  • Settings option for the default approval mode of new sessions #6812 is PARTLY addressed. Its "(or Reads)" case is delivered: a user who
    works at Reads no longer re-picks it per session. Its Trust case is NOT
    delivered.
  • Add a global default trust level for new chat sessions #8418 is NOT addressed. It asks specifically for Trust as the sticky global
    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.
  • The per-chat footer picker is unchanged and still offers Trust and YOLO. Only
    PERSISTING them is refused. A test pins that _SLOT_SCOPED_TRUST_MODES keeps
    trust, so a later reader does not "tidy" the two sets into agreement.

The clamp is enforced on READ, so a value that reaches config.json by 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.json containing trust (and yolo, and cased/whitespaced variants) and
asserts KiroCrewConfig.load() reports normal. It carries a control that a stored
trust_reads IS honoured, because without it every assertion would pass vacuously --
normal is 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_permitted is unchanged. The set-parity test was relaxed
from equality to a SUBSET for that reason -- every persistable tier must still be one
policy may never forbid, but trust is 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"' at
SecurityPanel.tsx(905,12) -- which took down seven lanes at once, since every one of
them 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, from APPROVAL_SEGMENTS (ApprovalModePicker.tsx:27) -- the
    RUNTIME vocabulary the footer picker renders. Unchanged by this PR; the diff does not
    touch that file at all.
  • DefaultApprovalModeKey, from DEFAULT_APPROVAL_MODE_KEYS
    (SecurityPanel.tsx:870) -- the PERSISTABLE set, and the only one narrowed.

SecurityPanel.tsx never imports the runtime type, so no shared type was shrunk. The
error was a now-unreachable case 'trust' inside this file's own optionLabel, a
Settings-only helper: dead code, removed. tsc -b now reports 15 errors, all
@storybook/* resolution in src/stories/*, zero in SecurityPanel and zero outside
src/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_SEGMENTS still contains trust, and that the persistable set is a strict
subset 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 trust out 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_mode still carried a live trust branch writing
set_approval_policy(session_key, "auto"). The grant was only UNREACHABLE -- correct
for exactly as long as nothing could supply trust -- and a later widening of the
persistable 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 contains
trust, so testing against it would tie this path's reach to a set kept for another
purpose), and asserts mode in _DEFAULT_APPROVAL_MODES. Two tests: the helper
refuses trust and 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_mode owns that path and writes its own
policy, 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.py said "the two persistable tiers are exactly the
modes ... always_permitted" above a two-element tuple (now "the TWO ... a strict
SUBSET", with the reason trust is excluded separately); core.py said "the three
values here"; and this body argued the three-tier design including the subagent
consequence of a persisted trust.

Two UX findings, both fixed:

  • The card no longer sits under "YOLO (auto-approve)". It has its own section
    ("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.
  • The card now SAYS why Trust is absent: "Trust is not offered here. It runs tools
    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)

Load failure notice

Save failure

Save failure notice

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-accent and
a 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 cfgError the effective
value 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 trust is clamped to
normal on read, and the frame for it was pixel-identical to the unset frame -- so
whoever 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 normal and cannot be
reported 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 the
agent 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.py records the verification in its own comment: a security ceiling kept
in config.json "would leave it writable by any auto-approved agent shell
(verified: is_sensitive_bash_command("echo x > .../config.json") is None)".
config.json IS in _WRITE_PROTECTED_HOME_PATHS, but that covers the agent tool
path only -- the shell path is not caught, and protected on one path only is not
protected. The computer-use primary enable lived in config.json and was moved
into _CREW_SECRET_LEAVES for exactly this reason
, with the principle named in
that 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:

command default_approval_mode.json config.json
echo x > <leaf> CAUGHT NOT caught
echo x | tee <leaf> CAUGHT --
cat <leaf> CAUGHT --

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.json on any path. An absent
leaf, a malformed leaf, a non-string value, and a value outside the persistable
set all resolve to normal, the interactive floor. There is deliberately no
fallback: 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.json and asserts the feature stays OFF, one asserts the
absent-leaf path yields no trust flags -- and the first is mutation-verified by
reintroducing a config.json fallback 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, which
is 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.json
sibling, both stricter: that handler gates on the app-token check alone, while
this one also requires is_owner_dashboard_request, because an allow-listed
messaging identity carries a dashboard credential whose subject is not the owner
and whose app claim is EMPTY -- the app-token guard alone would admit it to a
standing 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.json entirely, that endpoint can no longer reach it, no other key
used 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_mode leaves
AgentConfig, its vocabulary tuple and its normalizer leave config/sections.py,
the loader's coercer call goes, config-baseline.json is regenerated without the
key, and the frontend card stops calling /api/config/kirocrew. Removing the
field also orphaned a comment block in sections.py that described "the TWO that
remain" 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 Refs for that reason. #6812 is half addressed: its Reads case
works, its Trust case does not.

The override cue first shipped with text-warning, which the phantom-classes gate
correctly rejected: the theme declares no warning token, so Tailwind emitted no rule
and the line rendered with no colour at all. It now uses text-warn, the declared
token 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) failed
test_work_ledger.py::test_two_conductors_binding_one_worker_at_once_yield_exactly_one_binding
with assert 1 == 3. Diagnosed on evidence rather than by re-running:

  • The test file did not exist in this branch. It arrived on main in 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.
  • No shared surface. On main, test/test_work_ledger.py contains zero references
    to kiro_crew.config, dashboard/chat_handlers, dashboard/handlers/core, or
    default_approval -- there is no path from this diff to that assertion. This PR
    does not touch work_ledger.py.
  • The failing assertion is the SECOND one, outcomes.count(CODE_ALREADY_BOUND) == 3, which got 1. Four conductors race to bind one worker; one should win and
    three should be told ALREADY_BOUND. Getting 1 means two losers appended NO outcome
    at all, so they raised something the test's except wl.WorkLedgerError does not
    catch. That is an exception-type gap on Windows, not a slow runner -- consistent
    with it being a count rather than a duration.
  • Passes 3/3 locally on Linux.

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 half
addressed 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

  • At most two commits (one here), Conventional Commits title (feat: ...)
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable) -- the config field carries its own
    label and help text, which is what GET /api/config/schema serves
  • No secrets, credentials, or internal references in the diff

Contribution License Agreement

Placeholder per the template -- CLA wording to be supplied by OSPO.

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

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

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

  • PR body is stale relative to the shipped design: it claims the offerable tiers are "exactly the modes the approval_modes governance scope declares always_permitted," but always_permitted is ("normal", "trust", "trust_reads") and the persistable set deliberately drops trust — the single most consequential decision here (the Add a global default trust level for new chat sessions #8418 requesters wanted Trust). Update the description to state the trust exclusion and its standing-grant rationale, or reviewers and release notes will describe a feature the code refuses.

[DESIGN-REVIEWED] 21389ea

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 21389eaef8c7f1e0f3881a8020869eea198071dc — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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 ships

Intent: stop re-picking the approval tier in the footer for every new chat — an ADDITION.

  1. Settings → Security gains a "Default approval mode for new chats" card (Normal / Reads) — justified
  2. A dashboard-created chat now starts on that tier — justified
  3. New config key agent.default_approval_mode, default normal — justified
  4. Each option now shows a description under its label (picker's normal_desc/reads_desc) — justified
  5. An always-visible note explaining why Trust is not offered — justified (Add a global default trust level for new chat sessions #8418)
  6. The Approval section splits into two headings; a new "New chat defaults" heading appears — undeclared
  7. A warning line when the stored value is not persistable — undeclared, one consumer, hand-edit only
  8. Writing this key is owner-only, unlike every other editable key — justified
  9. Inheriting the tier writes a SEL entry, and a failed write refuses the tier — undeclared
  10. A declined default logs an info line naming the guard — undeclared

Watch

  • One user-driven sibling of the same cause is left unfixed: chat_fork.py:834 mints a sidebar chat inheriting agent/model/project/folder/tags but never reads the new key, so forking still lands on the floor. Disclosed ("Chats created by other paths (fork, channel, cron…) keep their existing behaviour"), so deferred — but a fork is a new chat to the user.
  • The body's security argument still describes the superseded revision: "The three offerable tiers are exactly the modes the approval_modes governance scope declares always_permitted" — two ship, and the test asserts a subset. The same stale "three tiers" claim ships in SecurityPanel.defaultApprovalMode.test.tsx's header.

Subtractions

  • Drop _apply_default_approval_mode's mode parameter, its if mode != "trust_reads" guard, the assert mode in _DEFAULT_APPROVAL_MODES (chat_handlers.py:289) and its bool return: 1 production caller (chat_handlers.py:2818) can only pass "trust_reads" (the gate requires in _DEFAULT_APPROVAL_MODES and != "normal"), discards the return, and audits before calling. Inline the two statements — the assert is the "decorative guard" the body itself refuses to ship.
  • Drop default_approval_mode_overridden (1 key × 13 catalogs, the overridden branch in SecurityPanel.tsx, 2 tests): _EDITABLE_CONFIG's enum already refuses every non-persistable value, so it fires only on a hand-edited config.json, where the radio already shows Normal rather than the written value.
  • Drop the four source-text assertions in test/test_default_approval_mode.py ('if _default_approval_mode != "normal":' in src, "and not slot._app" in src, "_default_approval_mode in _DEFAULT_APPROVAL_MODES" in src, the refusal-reason string list): they pin the handler's wording, and the HTTP-level tests in the same file already cover the app-owned, non-owner, app-worker and audit-failure refusals.

[FIRST-PRINCIPLES-REVIEWED] 21389ea

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — 🔴 changes requested (blocking)

GPT 5.6 found at least one blocking issue that must be resolved before merging 21389eaef8c7f1e0f3881a8020869eea198071dc. 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/dashboard/chat_handlers.py:2805 -- The audit-or-deny write is only queued
sel().log_api_access(
Unwritable SEL -> enqueue succeeds -> trust_reads is granted -> no durable audit event exists.
Anchor: backend-security-controls
Fix: Pass critical=True so persistence failure refuses the grant.
[BLOCK-MERGE] 21389ea
[GPT-REVIEWED] 21389ea

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

Confirmed against code. Line 2805 calls log_api_access(...) without critical=True. Per sel.py:2137-2214, the default (critical=False) path only enqueues to the background writer (line 2192); enqueue success raises no exception, and the actual filesystem persistence happens later in the writer thread where a failure is merely warned (line 2207-2211). The try/except at chat_handlers.py:2804-2818 therefore catches only enqueue-time failures, not persistence failures — so the grant at line 2818 (_apply_default_approval_mode) proceeds even when no durable audit event lands. critical=True (documented at sel.py:2149-2156 as the audit-or-deny mechanism, already supported by log_api_access at sel.py:2387) is exactly what the surrounding comment (lines 2790-2799) claims this code does but does not.

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 (chat_handlers.py:2766,2775-2776), the normal operating path for this PR's feature, plus a SEL persistence failure — precisely what critical= exists to handle. Remedy cost is one keyword argument with the mechanism already present; the rarity/extremity argument required for FLAG cannot be completed because the reaching path is normal operation, not a contradictory or unproducible edge.

[ADJUDICATION] 21389ea total=0 uphold=0 downgrade=0
[GPT-ADJUDICATED] 21389ea
[ADJUDICATION-FENCED] 21389ea fenced=1 flagged=0
UPHOLD-FENCED F1 src/kiro_crew/dashboard/chat_handlers.py:2805 -- Non-critical log_api_access only enqueues; the try/except cannot catch background persistence failure, so an unattended trust_reads grant proceeds unaudited on the feature's normal path, and the one-keyword critical=True fix leaves no rarity argument to justify a FLAG.
[GPT-ADJUDICATED-FENCED] 21389ea

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

@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 Sep 6, 2026
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

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

Review details

I've verified the candidate's crux by opening the code myself:

  • GET /api/config/kirocrew (api_kirocrew_config, core.py:1699) returns _masked_config_dict(KiroCrewConfig.load()).
  • _masked_config_dict (core.py:215) does cfg.to_dict(), and to_dict (loader.py:3776) does "agent": asdict(self.agent) — serializing the loaded dataclass.
  • KiroCrewConfig.load() (loader.py:2728) sets default_approval_mode=_normalize_default_approval_mode(...), which clamps any trust/yolo/junk to "normal".
  • api.kirocrewConfigGET /api/config/kirocrew (client.ts:2904).

So configured in the frontend is always "normal" or "trust_reads", and overridden (which requires configured be outside DEFAULT_APPROVAL_MODE_KEYS) is always false. The frontend test seeds the mocked query with a raw trust the real endpoint cannot produce, so it doesn't guard this. (a)(b)(c) all re-derived. It is a UX/observability gap, not a security hole/crash/data-loss/removed-guard, so it is advisory.

No blocking issues; one advisory finding.

FINDING — website/src/pages/settings/SecurityPanel.tsx:2381 — the overridden cue is dead in production: GET /api/config/kirocrew serializes the loader-normalized agent.default_approval_mode (asdict(self.agent) after _normalize_default_approval_mode clamps trust/yolo/junk to "normal"), so configured is never outside DEFAULT_APPROVAL_MODE_KEYS and !DEFAULT_APPROVAL_MODE_KEYS.some(k => k === configured) is always false — a hand-edited unpersistable value is silently clamped with no cue, the exact "SILENTLY is the problem" gap the comment claims to close → Fix: surface the raw stored value (or an overridden boolean) from the read path and drive the cue off that, or drop the dead branch and its strings.

[OPUS-REVIEWED] 21389ea

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

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

@chenmingwei23
chenmingwei23 force-pushed the feat/default-approval-mode-6812 branch from ccd5d2f to 70c526a Compare September 6, 2026 11:11
@chenmingwei23
chenmingwei23 requested a review from a team September 6, 2026 11:11
@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 Sep 6, 2026
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — 🟡 CONCERNS

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

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

  • Save-failed copy: "Could not save. New chats still start on the setting shown before your click." — the reader: "I can't tell whether the green 'Normal' is the thing I clicked (which failed) or the old setting that's still in force… only one state is shown" (shot-05). Since there is no optimistic update, the highlight IS the setting in force; the temporal phrasing implies it might not be. Rare state, but the misunderstanding is about which approval tier is live. Fix: "Could not save. New chats still use the highlighted option."
  • Trust contradiction: the standing note says Trust "is not offered here" while the override warning says "trust" is "stored for this setting" — reader: "I don't understand how both can be true" (shot-03). Fix: name the provenance in default_approval_mode_overridden, e.g. "stored outside this page".
  • shot-04: "Ask the agent" clips at the right edge when the load-failed message wraps to two lines — reader: "cut off… makes me less sure it's usable." Let the ErrorNotice link wrap under the message instead of sharing the row.

Evidence gaps

  • The new "New chat defaults" section heading (new_chat_defaults_section) and the card's in-panel placement below "YOLO (auto-approve)" appear in no screenshot — a full Settings → Security → Approval frame would close it.

Suggestions

  • In the load-failed state the copy says "Reload to see it" but both radios stay enabled with nothing selected; disable them on cfgError so a user can't overwrite a value they can't see.

[UX-REVIEWED] 21389ea

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Sep 6, 2026
@chenmingwei23
chenmingwei23 force-pushed the feat/default-approval-mode-6812 branch from 70c526a to f1f79cd Compare September 6, 2026 11:20
@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 Sep 6, 2026
@chenmingwei23
chenmingwei23 force-pushed the feat/default-approval-mode-6812 branch from f1f79cd to 0ef98a4 Compare September 6, 2026 11:29
@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 Sep 6, 2026
@chenmingwei23
chenmingwei23 force-pushed the feat/default-approval-mode-6812 branch from 0ef98a4 to 20e779c Compare September 6, 2026 11:56
@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 Sep 6, 2026
@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 Sep 6, 2026
@chenmingwei23
chenmingwei23 force-pushed the feat/default-approval-mode-6812 branch from acc8208 to 667be42 Compare September 6, 2026 15:06
@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 Sep 6, 2026
@chenmingwei23
chenmingwei23 force-pushed the feat/default-approval-mode-6812 branch from 667be42 to 874d151 Compare September 6, 2026 18:25
@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 Sep 6, 2026
@chenmingwei23
chenmingwei23 force-pushed the feat/default-approval-mode-6812 branch from 874d151 to 6281cb5 Compare September 6, 2026 19:45
@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 Sep 6, 2026
@chenmingwei23
chenmingwei23 force-pushed the feat/default-approval-mode-6812 branch from 6281cb5 to 39fd61b Compare September 6, 2026 19:55
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 6, 2026
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.
@chenmingwei23

chenmingwei23 commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Handing this off: where a standing approval grant should be stored

Closing this unmerged under a standing instruction not to work security-related
items. The analysis below is the part worth keeping, so it is recorded here rather
than lost with the PR. Written for someone arriving with no prior context.

The finding

A default-approval-mode grant is a security ceiling by this codebase's own
definition, not a preference.
security.py states the principle in its comment
on the computer-use primary enable: enabling that capability is "a security
ceiling, not a preference." A tier that decides whether a new session's tools run
without a human approving each call is the same class of control, and it applies
to every session created afterwards.

config.json carries write protection on the agent tool path only. It is
listed in _WRITE_PROTECTED_HOME_PATHS, which is easy to read as sufficient. It
is not. The shell path is not covered, and this repository already carries the
verification in security.py (see the comment block at lines 8399-8401): storing
a ceiling in config.json "would leave it writable by any auto-approved agent
shell (verified: is_sensitive_bash_command("echo x > .../config.json") is
None)". Protected on one path only is not protected.

_CREW_SECRET_LEAVES (security.py:8176) is the mechanism that covers both.
Leaves listed there are refused on the tool path via is_sensitive_path and on
the shell forms (>, tee, cat, and extraction verbs). Current members include
the denied-command opt-out, the computer-use primary enable, the deploy-target
pointer, the backup redaction switch, and Browser Mode's enable gate. The write
pattern is uniform: a dashboard PUT handler is the only writer and opens the path
directly rather than through the gate, so the operator's own Settings control
keeps working while an agent's file and shell tools are refused. Reads are
protected as well as writes where the value itself is useful to an attacker.

The migration being proposed here has a direct precedent. The computer-use
primary enable lived in config.json and was moved into _CREW_SECRET_LEAVES for
exactly this reason; Browser Mode's gate followed. So moving an approval-tier
grant there follows a decided pattern rather than inventing a trust boundary.

The general observation, which outlives this setting

A ceiling stored beside preferences inherits the protection of preferences.
That is why this is not a small change. The store a control lives in determines
who can modify it, and a control placed in a preferences file is protected like a
preference no matter how carefully the code that reads it is written. The question
to ask of any new control is not what its value means but who can write the
place it is read from
-- and a grant is only as trustworthy as the
least-privileged party who can modify its source. Every layer downstream of a
writable source is reasoning about a value that party controls.

State of this branch

The branch is left in place and the issue remains open. The final commit on it
does implement the migration described above: the grant moved to a dedicated
keystone leaf, reads fail closed to the interactive floor with no fallback to the
preferences file, the clamp runs on read as well as write, and the dashboard
handler is the only writer. Two behavioural tests cover it -- one writes the grant
into the preferences file and asserts the feature stays off, one asserts the
absent-leaf path grants nothing -- and the first was checked by reintroducing the
fallback and confirming the test fails.

This should not merge in its current shape. Not because the migration is
unfinished -- the final commit implements it -- but because a change to where a
security ceiling is stored must go through this repository's security review before
anyone relies on it, and this one has not.

That work has NOT been reviewed against this repository's security review process,
and it should not be assumed correct on the strength of this comment. Anyone
resuming should re-review it from the beginning. It is recorded here only so the
next person knows the direction was already investigated and does not have to
rediscover the mechanism.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant