fix(dashboard): single-source the one-shot approval decision mapping

[chenmingwei23]

1. What is the problem?

The one-shot approval endpoint POST /api/approvals/{id}/{action} honors exactly approve, reject and reject_once, and records no standing grant. A trust verb (trust, trust_reads, trust_command, trust_base) has no representation there at all -- it belongs on a grant-recording endpoint instead.

Three surfaces shipped the same defect independently, each by computing its own decision that mapped a trust verb onto approve:

• Spawn-approval trust tiers grant one-shot approval but the card reports "Trusted" #5400 -- spawn-approval card
• CollapsibleToolGroup wires the #5400 trust/one-shot mismatch, latent behind hasPermission #5434 -- collapsed tool row
• ChatInput's own toApiDecision still maps trust->approve on the one-shot fallback (#5400 class) #5486 -- ChatInput's own mapping

#8193 asks for "a chokepoint" so a fourth author cannot reintroduce it. The finding of this PR is that a chokepoint as literally specified is unbuildable, and why.

Two chokepoints already exist and are already single:

layer	check
website/src/api/client.ts:3172	resolveApproval typed 'approve' | 'reject' | 'reject_once'
src/kiro_crew/dashboard/handlers/sessions.py:1507	returns 400 for any action outside those three

Both reject a trust verb sent to the one-shot endpoint -- but the verb never arrives as itself. A call-site mapping converts it into approve before the request is made, so by the time either check runs, the information that standing trust was requested has been destroyed. A guard cannot recover what the caller erased before calling. The mapping is the defect, so nothing downstream of the mapping can see it.

What was actually wrong was that the mapping was spelled three times, none importable by the others:

site	shape
components/ChatInput.tsx:193	private toApiDecision, fail-closed
pages/ChatPage.tsx:5433	private toApiDecision useCallback, fail-closed
pages/chat/ActivityViewer.tsx:297	inline ternary, fail-OPEN

2. Why this issue matters to the user

The failure mode is quiet and it sits on a consent path. The tool runs once, the UI says standing trust was granted, and the backend recorded nothing. The user's next identical action prompts again -- which reads as the grant having been forgotten rather than never made. Three surfaces shipped it independently, which is the signal that the rule was not written anywhere a fourth author would have to read.

3. How our fix solves it

Source is the only layer where the verb still exists as itself, so the mapping is made single and a lint rule keeps it that way.

• utils/approvalDecision.ts now owns toApiDecision -- beside isRejectedDecision, the predicate that already single-sources this exact vocabulary and already carries the "adding the next token must be a one-line change here" doctrine.
• ChatInput and ChatPage drop their private copies and import it. Both were already fail-closed, and CollapsibleToolGroup never emits rejected_once (only approved / trust / rejected), so the two mappings were equivalent over their reachable inputs: behaviour unchanged.
• ActivityViewer's ApprovalEntry replaces its inline fail-OPEN ternary. Measured reachability first: the card renders exactly two buttons and the complete reachable argument set at the endpoint is {approve, reject}, so behaviour is unchanged today. It was nonetheless the last in-tree instance of the shape all three regressions took, safe only by the accident that its two callers pass literals -- and a render gate is one edit away from being widened.
• eslint-rules/approval-one-shot-decision.js checks the decision argument against a fail-closed allowlist. Registered 'error', not 'warn', so it fails the build rather than riding the --max-warnings 0 ratchet.

Why the rule is an allowlist

A denylist of ternaries would have caught none of the three defects it cites: each was a module-private mapper, not an inline ternary, and hoisting a ternary one line up into a local changes the argument's node type and nothing else. Enumerating every resolveApproval decision argument in the repo gives Literal x3, Identifier x6, CallExpression x4 -- so the rule admits exactly those three shapes and reports anything else:

shape	admitted when
literal	it names an action the endpoint honors, so a literal trust verb is caught too
call	it is the shared toApiDecision, not a private mapper
identifier	it relies on its declared type (parameter, import, destructure); reported when scope analysis shows it assigned a computed decision, by declarator initializer or a later write
anything else	never -- reported, so a fourth shape is judged deliberately

Admitting pre-narrowed identifiers is deliberate. A stricter literal-or-mapper rule would flag six legitimate call sites already typed 'approve' | 'reject', and routing those through a string-accepting mapper would be a downgrade, not a fix.

What the rule does not catch, stated rather than implied: with no type information it cannot distinguish a narrowed identifier from a widened one, so a parameter declared string and passed straight through still passes. What it catches is a decision computed at the call site -- inline, hoisted into a local, or delegated to a private mapper. Closing the remainder needs a type-aware lane (parserOptions.project), which is a separate change with its own cost. The rule's header says so too, so the next reader is not misled about its reach.

Deliberately not in this PR: the routing predicate (TRUST_ACTIONS / approvalRoute in apps/mochi, plus ChatInput's inline 4-verb array). Two spellings of a 4-verb array is a real smell, but they already agree, unifying them changes no behaviour, and it crosses an app boundary. It is not this item.

4. What tests we did

Every check here was proven able to fail before being relied on.

The premise first. #8193's premise is conditional on #5400/#5434/#5486 being fixed. Verified on a fresh detached worktree at clean origin/main 6d1b51704 with git diff origin/main = 0 lines: all three behaviours are gone, and no fourth live consumer exists (useSceneInteraction, both ProjectDetailPage mutations, NotificationFeed and NotificationDetailPanel all pass literals or pre-narrowed unions). The one deliberate trust-to-one-shot downgrade (ChatInput:1115, unattended sources) is unreachable, because approvalTrustGrantable = !!activeSlot && !approvalIsUnattended gates every trust button.

Reachability of :297, by construction rather than by reading. Mounted the card, enumerated every button it renders, clicked each one, and captured every argument reaching api.resolveApproval. Result: buttons [Approve, Reject], reachable arguments {approve, reject} -- no trust verb. That is why this is a hardening, not a live-defect fix, and the PR says so.

Tests lead with violations. approvalOneShotDecisionRule.test.ts lints snippets through the real eslint.config.js with allowInlineConfig: false. A suite that only lints the clean tree passes when the rule works, when it is broken, when it is unwired, and when it does not run -- four states with one green. So nine of the twelve cases assert on fixtures that must be flagged; three are negative controls.

Five proofs that those tests can fail for the right reason:

proof	result
four bypass fixtures against a ternary-only denylist	4 named tests RED, then GREEN under the allowlist
fail-closed arm removed	reddens exactly flags an unrecognized argument shape, no collateral
isSharedMapperCall forced true	reddens exactly flags a private mapping helper, no collateral
rule unwired from eslint.config.js	reddens the wiring test plus every violation fixture
original ternary reapplied to ActivityViewer via a file-based patch	npx eslint errors at the call site

Regression runs (targeted, never the full suite): approvalOneShotDecisionRule (12), ChatInput.trustOneShot (5), ChatPage.collapsedGroupTrust (2), ActivityViewerCoverage (48), CollapsibleToolGroupCov80 (21) -- 88 passed, 0 failed; plus ChatInput.approval, ChatInput.approvalNudge, ChatMessageList, toolApproval (102 passed). npx tsc -b exit 0. npx eslint src/ --max-warnings 0 exit 0, exactly as CI runs it, with all 13 real call sites passing the stricter rule. check-i18n-strings.mjs OK at 460/1015.

The ChatInput.trustOneShot contract was rewritten rather than deleted: it used to brace-match toApiDecision out of the component and rebuild it with new Function, a hack its own docstring apologised for because the helper was private. It now imports the function that actually ships, and adds an assertion that ChatInput does not re-declare the mapping locally -- the one regression the lint rule cannot catch, since the rule inspects the argument at the call site, not the file for a second definition.

5. Any other suggestions on the work

Both advisory CONCERNS on the first revision were real and are fixed. Design Review found that the rule as first written was a denylist of two node types and therefore could not back the guarantee the description claimed -- a private mapper or a hoisted ternary passed it in silence, and the private-mapper shape is what all three cited defects actually were. That falsified a sentence in this description, so the rule was inverted to the allowlist above and the claim narrowed to what it actually guarantees. Its suggested remedy (literal or toApiDecision only) was directionally right but too strict as written, which the shape census showed: it would have flagged six already-narrowed call sites. First Principles Review found SHARED_MAPPER exported with zero consumers and unused even inside its own file; it is now the constant the mapper check tests against, with the export gone, and OneShotAction drops its export for the same reason.

Do not read the base reds as this PR's. main is red on 6d1b51704, the sha this branch is cut from, with Backend Tests (3.12, 3), Backend Tests (Windows) (3) and Coverage Gate failing in shell command-shape parsing. This is a frontend-only diff with zero Python lines. In the event, those three did not run at all.

What remains open on #8193, and why this uses Refs rather than Closes: the issue also asks whether the routing predicate should be promoted out of apps/mochi. That is a cross-app refactor on a consent path with no behavioural win, and it stays a maintainer decision. #8193 should stay open for it.

One correction to the issue's own table. It lists ActivityViewer's ApprovalEntry as constrained by "two buttons only, approved / rejected". True of the render site, but silent about the mapping -- and the issue's own hazard sentence ("nothing stops a new component from writing resolveApproval(id, action === 'reject' ? 'reject' : 'approve')") described a shape that was already in the tree at that exact line.

Pattern harvest

Rule candidate: eslint -- shipped in this PR as website/eslint-rules/approval-one-shot-decision.js
Pattern: a call site computes a value into a privileged enum, upstream of every layer that would have rejected the original value

The generalizable shape is not "a trust verb becomes approve". It is an enum narrowing performed at the call site while the validating layers sit downstream of it. Both guards here were already single and already correct -- a typed client and a 400-returning handler -- and both were structurally blind, because the caller destroyed the distinguishing value before either could observe it. That is the part worth carrying forward: a chokepoint can only enforce what still reaches it.

Any endpoint whose action vocabulary is narrower than its callers' decision vocabulary has this shape, and the tell is a per-call-site mapping between the two. Such a mapping is invisible to the type at the boundary (it returns the narrow type, so it type-checks) and invisible to the validator behind it (it emits a legal value). Three independent surfaces reached for it without seeing each other, which is what makes this a shape rather than an accident -- and why the fix is a source-level rule plus a single fail-closed mapping, not another runtime check.

A second, sharper lesson came out of review: the first version of that rule reproduced the very error it was written to prevent. It matched the shape the defect had in the description (an inline ternary) rather than the shape it had in the three real regressions (a private mapper), so a denylist of ternaries would have caught none of them. When harvesting a rule from a set of defects, check the rule against each defect's actual source, not against the summary.

Scope note: the rule is deliberately narrow -- it governs resolveApproval's decision argument, not every narrow-enum boundary in the dashboard. Generalizing needs an inventory of those boundaries, which does not exist yet; this is the first entry.

Why no screenshot: Nothing rendered changes, and that was measured rather than assumed. For ActivityViewer's ApprovalEntry -- the only site whose mapping semantics differ from what it replaces -- the card was mounted, every button it renders was enumerated and clicked, and every argument reaching api.resolveApproval was captured: buttons [Approve, Reject], arguments {approve, reject}, identical before and after. The rendered decision label is driven by the raw localDecision, which this diff does not touch. ChatInput and ChatPage swap a private mapping for a byte-equivalent shared one (CollapsibleToolGroup emits only approved / trust / rejected, never rejected_once, so their two spellings agreed over every reachable input). The remaining changes are a lint rule, its config registration, comments and tests, none of which render. The behavioural delta is confined to what a future widened caller would receive: reject instead of approve.

Refs #8193

[github-actions]

UX Review (Fable 5) — ✅ PASS

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

This PR is entirely an internal refactor: it collapses three private copies of the approval-decision mapping into one shared toApiDecision in utils/approvalDecision.ts, adds an ESLint rule pinning the call sites, and rewrites tests to import the real helper. I checked every touched surface for user-visible change:

• No new or changed user-facing strings — all added text is code comments, test prose, and lint-rule messages (developer-facing).
• No rendered UI change — ActivityViewer's two buttons still pass the literals 'approved'/'rejected', which map identically through the shared function; ChatPage's collapsed-group approve path and ChatInput's path keep the same reachable behavior. The only mapping delta ('rejected_once' → reject_once instead of reject in ChatPage) makes the sent verb more truthful and is unreachable from any rendered control in this diff.
• No screenshots, no layout, no flow, no state changes to evaluate.

The fail-closed direction (unknown verb → reject, never approve) preserves the UX truth-telling property the earlier bugs (#5400/#5434/#5486) violated — the UI can no longer report a standing trust grant the backend never recorded.

UX-Verdict: PASS

Pure internal single-sourcing; no user-facing string, control, flow, or pixel changes, and the fail-closed mapping keeps the UI honest about trust grants.

[UX-REVIEWED] 3a8ce2b

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 3a8ce2bdb36717bb9ca806f45c0dd966614c8879 and found no blocking issues.

This comment is updated in place on each push.

Review details

FINDING -- website/eslint-rules/approval-one-shot-decision.js:195 -- writtenExpressions(variable) permits lookup/switch assignments, allowing trust to map to 'approve' without lint failure -> Fix: reject locally written identifiers unless assigned by the shared mapper.
[GPT-REVIEWED] 3a8ce2b

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

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

Right shape: the mapping is the defect, so single-sourcing it at the only layer that still sees the verb, plus a fail-closed allowlist lint, is root-cause and proportionate.

[DESIGN-REVIEWED] 3a8ce2b

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

Confirmed: the two buttons pass 'approved'/'rejected' literals; toApiDecision('approved') → 'approve' and toApiDecision('rejected') → 'reject', identical to the old inline ternary. The runtime change is pure hardening, no regression.

Assessment of CANDIDATE 1: The technical claim about the lint rule is accurate — the Identifier branch admits a hoisted MemberExpression (const d = table[action]; resolveApproval(id, d)) or an aliasing identifier, whereas the same shape inline hits the fail-closed unknownShape branch. But the observable outcome is entirely hypothetical: the candidate itself states it "could not construct an in-tree call site exercising the gap today," and the harm is a future author writing a hoisted trust-verb-laundering mapper. That is precisely the "if a caller were to" shape the falsification bar forbids — (a) has no input occurring in practice and (c) is speculative. It is also incompleteness in a defense-in-depth lint guard whose own docstring enumerates what it deliberately does not catch, sitting above the render gates and the fail-closed runtime toApiDecision. Below the 80 bar. Dropped.

No new grounded findings surfaced in Step 2.

No findings.

[OPUS-REVIEWED] 3a8ce2b

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

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

[github-actions]

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of 3a8ce2bdb36717bb9ca806f45c0dd966614c8879 — 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.

All claims verified: the shared toApiDecision has 4 real consumers (ChatInput:1114, ChatPage:7900 and 9221, ActivityViewer:298); CollapsibleToolGroup emits only approved/trust/rejected so the reject_once widening in ChatPage is unreachable as claimed; the local-eslint-rule mechanism and its violation-led test pattern already exist (eslint-rules/i18n-strict.js, i18nStrictRule.test.ts); the remaining resolveApproval callers pass literals or pre-narrowed unions. The one redundancy: the rewritten trust-contract test adds a regex assertion forbidding a local toApiDecision re-declaration in ChatInput, which the lint rule shipped in the same PR already reports by binding resolution in every file.

First-Principles-Verdict: PASS

Cause-level fix: the mapping is made single at the only layer that still sees the trust verb, and the gate is derived from three counted shipped defects.

What this change ships

Intent: stop a standing-trust click from ever again being silently downgraded to a one-shot approve, by single-sourcing the decision mapping and gating re-introduction. This is a FIX (of a defect class; the three live instances were already fixed, and the PR says so).

1. One shared mapping now decides what the one-shot approval endpoint is sent — justified (4 consumers counted: ChatInput:1114, ChatPage:7900/9221, ActivityViewer:298)
2. ChatInput's private copy of the mapping deleted — justified, net deletion, identical body
3. ChatPage's private copy deleted; rejected_once there now maps to reject_once — declared; unreachable (CollapsibleToolGroup emits only approved/trust/rejected, verified)
4. Activity-panel card no longer defaults unknown verbs to approve — declared hardening, reachable behavior unchanged
5. Build now fails on a call-site-computed approval decision — justified; derived from 3 counted defects, reuses the existing local-rule mechanism (eslint-rules/i18n-strict.js)
6. Violation-led rule test suite — justified, mirrors i18nStrictRule.test.ts
7. Trust-contract test imports the real function instead of rebuilding it via new Function — justified deletion of a hack
8. Regex test forbidding a local re-declaration in ChatInput — duplicate of the lint rule in this same PR

Subtractions

• Drop the does not re-declare the mapping locally regex assertions in ChatInput.trustOneShot.test.tsx — the lint rule already reports a same-named local binding in every file (its own fixture flags a private mapper that SHADOWS the shared name pins that), so the regex is a second spelling of the same guard covering one file, and it hard-codes the import path the rule resolves by binding.

[FIRST-PRINCIPLES-REVIEWED] 3a8ce2b

[chenmingwei23]

Design Review's BLOCK on d8bc0a6f2 was correct, including its guess at the cause. Fixed on f7c159b7c. Recording the mechanism because the recipe that produced it looks safe and is not.

What happened. PR Hygiene caps a PR at two commits and reserves the second for a mechanical follow-up, so after the round-2 review fixes I squashed with the recipe that check's own error text suggests:

git reset --soft origin/main
git commit

I ran it without the git rebase origin/main that precedes it in that recipe. My local origin/main ref had advanced past my base, so reset --soft moved HEAD forward onto 6b7847856 while --soft kept my index at the tree built on 6d1b51704. Committing that index against the newer parent expressed the difference as deletions. The rebase step is not tidiness -- it is what makes the reset safe, because it moves the TREE to the same base the ref moved to.

What that silently reverted, none of it intended and none of it in the description:

• c791f0f1d -- security.py's quote-aware shell walk (_iter_shell_chars, _matching_close_paren, _shell_quote_walk), replaced by the quote-unaware paren counters whose bypasses the deleted docstrings document verbatim.
• 6b7847856 (fix(dashboard): persist a slot create inside the slots suspension #8665) -- the slot-persist-inside-suspension fix in chat_handlers.py.
• ~698 lines of the tests that pin both, including test_a_quoted_paren_cannot_swallow_the_refspec and test_an_unprovable_boundary_fails_closed, plus all of test_chat_slot_create_folder.py.

The consequence the review named is the one that matters: the regression tests went with the hardening, so nothing would have gone red on a reopened protected-branch escape route.

The fix. Rebuilt the branch directly on current main (369bb6d0b) carrying only the nine website/ files. Verified first that main had touched none of those nine between my old base and now, so taking my versions could not revert anything of its own.

Verified after: security.py carries its three hardening helpers again (18 symbol references), both named pinning tests are back, test_chat_slot_create_folder.py is present, and git diff origin/main -- . ':(exclude)website' is empty. GitHub now reports 9 changed files, +519/-78, one commit whose parent is main's tip. tsc -b exit 0, eslint src/ --max-warnings 0 exit 0, 88 targeted tests passing.

Nothing about the reviewed change itself moved -- the nine files are byte-identical to what was on d8bc0a6f2. The only delta is that the diff no longer carries someone else's reverted work.

[bolichen97]

Tech Lead review: APPROVE.

Correctness of the mapping, per call site, against the reachable input set. The shared toApiDecision is approved -> approve, rejected_once -> reject_once, else reject (fail-closed).

• ChatInput.tsx -- the deleted private copy was byte-identical to the shared one. Zero delta.
• ChatPage.tsx -- the old mapping was action === 'approved' ? 'approve' : 'reject'; the new one adds a rejected_once arm. Verified at source that both CollapsibleToolGroup mounts (7897, 9218) omit canTrust, and that the component emits only approved / trust / rejected literals (submitDecision at CollapsibleToolGroup.tsx:246-248) -- never rejected_once. The two spellings therefore agree over every reachable input, including trust, which both send to reject.
• ActivityViewer.tsx -- the replaced ternary was fail-OPEN (action === 'rejected' ? 'reject' : 'approve'). ApprovalEntry renders exactly two buttons passing the literals approved and rejected; both map identically old and new. The only delta is what a future widened caller receives: reject instead of approve. On a consent path that is the correct direction, and it removes the last in-tree instance of the shape #5400/#5434/#5486 each shipped.

Also checked: the vocabulary stays consistent with the isRejectedDecision predicate already in this module (rejected_once is a reject in both), and dropping toApiDecision from the ChatPage useCallback deps at 7904 is correct now that it is a stable module import.

Signals. PR Readiness success; all 64 check-runs success or skipped, including Frontend Lint & Type Check with the new rule registered at error on the whole tree; all five AI lanes PASS; zero review comments; zero prior reviews.

The one advisory finding is real and non-blocking. GPT is right that writtenExpressions admits a lookup or switch assignment (const d = MAP[action] is a MemberExpression, in neither COMPUTED_DECISION_TYPES nor the call branch), so that re-introduction path passes the rule. This is a gap in the new guard's reach, not a defect in the shipped mapping, and the description pre-discloses the class ("Closing the remainder needs a type-aware lane") -- though its "hoisted into a local" phrasing is broader than what the rule actually covers. Worth a follow-up that either adds MemberExpression to the reported shapes or tightens that sentence; it does not gate this merge, since the runtime behaviour of all four consumers is unchanged and the rule is strictly better than the three private mappings it replaces.