Skip to content

fix(channels): label channel trust as channel-wide and persistent (#5203) - #5230

Merged
bolichen97 merged 1 commit into
mainfrom
fix/channel-trust-label-scope-5203
Aug 24, 2026
Merged

fix(channels): label channel trust as channel-wide and persistent (#5203)#5230
bolichen97 merged 1 commit into
mainfrom
fix/channel-trust-label-scope-5203

Conversation

@bolichen97

@bolichen97 bolichen97 commented Aug 23, 2026

Copy link
Copy Markdown
Collaborator

Problem / Motivation

On the channels surface, the approval card's trust dropdown shows the chat surface's session-scoped label, "Trust all tools" — but the channel trust decision executes ch.trusted = True; ch._save() (src/kiro_crew/dashboard/handlers_channel.py, api_channel_approve_agent): it grants auto-approval to every agent in the channel and persists the grant to disk, surviving restarts. The label describes something much narrower than the grant it makes.

Why it matters

A user who believes they trusted one agent for one session has actually granted channel-wide, restart-surviving auto-approval. That is a consent-integrity defect on a security control — the same shape as #4421 (label narrower than the actual grant). This PR is only about the label telling the truth; whether channel trust should persist is a separate product question and deliberately out of scope.

What changed (motivation → approach → change)

Symptom: the shared TrustDropdown hardcodes the components.trustDropdown.trust_all_tools key, so every surface shows the session-scoped wording regardless of what its trust action actually does. Root cause: the component has no way for a surface to label a wider grant. Change: TrustDropdown gains an optional trustAllLabelKey prop that overrides the trust-all label key; ApprovalCard passes it through; ChannelPage supplies a new key, components.trustDropdown.trust_all_tools_channel — "Trust all tools in this channel — persists across restarts". The chat surface passes nothing and keeps its default label. No behavior change: the emitted action is still trust.

Rebased onto main after #5202 landed (which gave both components a hasCommand prop and turned the channels surface into a single-tier menu). The two changes compose: the channels card now passes hasCommand={false} and trustAllLabelKey, so the one remaining tier is the one whose scope this PR states. #5202's channel-surface test assertion was updated from the old session-scoped label to the new one, and a test pins the shipped combination.

The new string is added to en.manual.json (hand-authored, no source literal), all 11 translated catalogs (zh-CN, hi, es, fr, bn, pt, ru, de, ja, ko, it — following each language's style guide, em dash preserved per precedent), the regenerated en-XA pseudolocale, and the en.context.json translator sidecar.

Tests

website/src/test/TrustDropdown.test.tsx:

  • the override renders the channel-scoped label and the exact default string is absent;
  • the default label is unchanged when the prop is absent (and the channel string is absent);
  • the override item still emits the plain trust action;
  • the override key resolves through the active locale (zh-CN).

Existing 26 TrustDropdown tests pass unchanged, pinning that the default surface is untouched.

Manual verification

Rendered both surfaces in a browser (Vite + Playwright) on the rebased head, asserting the rendered menu items: channels surface = 1 tier reading "Trust all tools in this channel — persists across restarts"; chat surface = 3 tiers with the default "Trust all tools" intact; zh-CN renders its translation; at a 320px viewport the longer label wraps inside the menu's viewport-aware width cap rather than overflowing.

Screenshots / video

Channels surface — corrected label:

channel override, en

Chat surface — default unchanged:

chat default unchanged, en

More: zh-CN, narrow viewport (320px)

channel override, zh-CN

channel override at 320px, label wraps

Related Issues

Closes #5203

Related: #4421 (command-scoped tiers offered on the channel surface and 400-rejected) was fixed independently by #5202 and is now closed — this PR rebases onto it rather than duplicating it. #5204 (the card optimistically showing 'Trusted' when the decision POST fails) remains open and out of scope here.

Checklist

  • Single commit with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable) — N/A: no documented API/schema change; translator context added to en.context.json
  • No secrets, credentials, or internal references in the diff

@bolichen97
bolichen97 requested a review from a team August 23, 2026 08:19
@bolichen97
bolichen97 requested a review from a team as a code owner August 23, 2026 08:19
@bolichen97
bolichen97 requested a review from Zedmor August 23, 2026 08:19
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Aug 23, 2026
@github-actions

github-actions Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — ✅ PASS

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

UX-Verdict: PASS

The label now tells the truth about the grant — channel-wide scope and persistence are named at the point of consent, verified in en, zh-CN, and 320px renders.

[UX-REVIEWED] 99ca30e

@github-actions

github-actions Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

Verified against api_channel_approve_agent (trusted=True + _save()): the label now states the real grant; minimal prop pass-through is the right-sized fix.

[DESIGN-REVIEWED] 99ca30e

@github-actions

github-actions Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

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

Review details

The backend confirms the label's accuracy: ch.trusted = True + ch._save() at handlers_channel.py:329-330 makes the grant channel-wide and persisted, matching the new label "in this channel — persists across restarts." The approve endpoint rejects non-trust actions (line 324), so command-scoped tiers stay off the surface.

The change is a defensive label pass-through: trustAllLabelKey is optional and defaults to the original session-scoped label, so no other surface regresses. The new key is present in all 11 non-English locales, the pseudolocale, en.manual.json, and en.context.json, and correctly absent from generated en.json (referenced dynamically via a variable, not an extractable literal). Tests updated to match. No blocking: true AUTOSDE rule is touched. I found nothing that grounds a concrete input → call path → wrong outcome.

No findings.

[OPUS-REVIEWED] 99ca30e

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

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

@github-actions

github-actions Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 99ca30e36ecec4c64bcef1c794391d283fb5a33b and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 99ca30e

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

@github-actions

github-actions Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 99ca30e36ecec4c64bcef1c794391d283fb5a33b — 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 checks done. The backend claim verifies (handlers_channel.py:328-330 persists ch.trusted), the new prop has exactly one consumer, and the same label≠grant root cause has one unfixed sibling on the spawn-approval surface. Final review:

First-Principles-Verdict: CONCERNS

The label fix is real and derived, but trustAllLabelKey is an open string key with exactly one caller ever passing one value.

What this change ships

Intent: make the channel approval card's trust option name the grant it actually makes (channel-wide, persisted) — a FIX.

  1. Channel trust menu item now reads "Trust all tools in this channel — persists across restarts" — justified (handlers_channel.py:329-330 sets ch.trusted = True and saves).
  2. TrustDropdown/ApprovalCard gain an optional trustAllLabelKey override prop — one consumer, generalized.
  3. New catalog key in en.manual.json, 11 locales, en-XA, and the context sidecar — justified (documented i18n invariant).
  4. Trust-all label span gains min-w-0 so the longer label wraps — justified, declared under manual verification.
  5. Four PR-evidence screenshots under temp-screenshots/ — justified (temp-screenshots/README.md convention; cleanup job owns removal).

Watch

  • The stated root cause — "the component has no way for a surface to label a wider grant" — has 1 unfixed sibling in the opposite direction: ActivityViewer.tsx:287 renders the same TrustDropdown on spawn approvals, where every trust tier collapses to a one-shot approve (ActivityViewer.tsx:264), so "Trust all tools" there overstates the grant. Grep: TrustDropdown render sites = 3 (ChatInput, ActivityViewer, ApprovalCard); this PR fixes only the under-claiming one and does not say what is left.

Subtractions

  • Shrink trustAllLabelKey?: string to a boolean (e.g. channelScopedTrust): grep trustAllLabelKey= finds 1 consumer passing 1 constant (ChannelPage.tsx:167), and an open key lets any future surface label a trust grant with any string, unreviewed.

[FIRST-PRINCIPLES-REVIEWED] 99ca30e

@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 Aug 23, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator Author

Disposition for the First Principles CONCERNS verdict on df93374939ce:

  • Shrink trustAllLabelKey?: string to a boolean (e.g. channelTrust) — rebutted (kept as the label-key override; proportionality push-back on the narrower-API demand).

    A channelTrust boolean moves the channel-specific catalog key into the shared component — TrustDropdown would then hardcode knowledge about one of its three call sites' domains, while the other two (ChatInput, ActivityViewer) are session-scoped and pass nothing. The current shape keeps the shared component domain-agnostic and lets each surface own the statement of its own grant, which is the actual invariant this fix establishes ("the surface that knows what trust does is the one that words it"). It is also the same contract the component already speaks (every label it renders is a catalog key), and the next wider-than-session surface (e.g. a workspace-scoped grant) slots in without another API break, where a boolean would have to become an enum. The stringly-typed risk is bounded: the one production caller passes a literal that deadKeys/catalog-parity coverage keeps live, and a typo'd key renders as its own dotted fallback in every locale test run. Given one line of difference in surface area, coupling the shared component to a consumer's domain is the worse trade.

  • Watch: trust_command/trust_base still offered on the channel surface and 400-rejected — accepted-and-deferred (as the verdict itself notes: declared in the PR body, tracked in ChannelPage approval trust tiers: role passed as command, trust_command decision coerced to rejected #4421 and ApprovalCard optimistically shows 'Trusted/Approved' even when the decision POST fails #5204; fixing the tier plumbing is a behavior change this label-only PR deliberately does not make).

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Aug 23, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator Author

🤖 Kiro Crew Auto-Pipeline [operator: bolichen97#66809557]: Review-ready at df93374939ce — 62/62 checks green, PR Readiness passed, all 5 AI lanes verdicted (GPT ✅, Opus ✅, Design ✅, UX ✅, First Principles 🟡 advisory — dispositioned above), 0 unresolved threads. The single red round was an unrelated CliPanelCoverage flake on Frontend Tests shard 4 (passes locally on the same code; cleared by rerun, 0 code changes). Ready for human review and merge.

@bolichen97
bolichen97 enabled auto-merge (squash) August 23, 2026 18:59
@github-actions github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Aug 23, 2026
chenmingwei23
chenmingwei23 previously approved these changes Aug 23, 2026
)

The channels surface's trust decision grants auto-approval to every
agent in the channel and persists it to disk, but the approval card
reused the chat surface's session-scoped 'Trust all tools' label, so
the consent shown was narrower than the grant made.

TrustDropdown gains an optional trustAllLabelKey override; the channels
approval card passes a key naming the real grant ('Trust all tools in
this channel — persists across restarts'). No behavior change; the chat
surface keeps its default label. New string added to all 14 catalogs.
@bolichen97
bolichen97 force-pushed the fix/channel-trust-label-scope-5203 branch from df93374 to 99ca30e Compare August 23, 2026 21:48
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: passed Eligible automated validation passed for the current revision labels Aug 23, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator Author

🤖 Kiro Crew Auto-Pipeline [operator: bolichen97#66809557]: rebased onto main 9aeb46955 (was CONFLICTING) — new head 99ca30e36ece, still one commit.

Why it conflicted: #5202 landed in the meantime and touched the same three lines, giving TrustDropdown/ApprovalCard a hasCommand prop and reducing the channels menu to a single tier. Resolution keeps both: the channels card passes hasCommand={false} and trustAllLabelKey, so the one remaining tier is the one whose real scope this PR states.

Other changes in this push: #5202's channel-surface assertion (getByText('Trust all tools')) was updated to the new label — it pinned exactly the wording this PR corrects; a new test pins the shipped hasCommand={false} + override combination; screenshots re-taken against the rebased head (channels menu now shows 1 tier) and re-pinned to the new SHA; the PR body's #4421 note is corrected — #5202 fixed and closed it, so this PR rebases onto that fix rather than deferring it (#5204 remains open, out of scope).

Local gates green on the new head: tsc -b, 178 tests across TrustDropdown/ApprovalCard/ChannelPage/ChatInput, i18n suite 41 files/629 tests, i18n:check full chain, jscpd, brand gate.

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: checking Automated validation is still running labels Aug 23, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator Author

Disposition for the First Principles CONCERNS verdict on the rebased head 99ca30e36ece:

  • Watch: ActivityViewer.tsx renders the same dropdown on spawn approvals, where every tier collapses to a one-shot approve, so "Trust all tools" overstates the grant — accepted-and-deferred, and the gap is now named rather than left implicit.

    Verified: ApprovalEntry.onAction forwards the decision (and its pattern) only on the approval_type === 'chat' branch; every other type takes resolveApproval(id, action === 'rejected' ? 'reject' : 'approve'), so trust / trust_command / trust_base all land as a plain one-shot approve while decisionLabel['trust'] still renders "Trusted". That is this PR's defect in the opposite direction and a real consent-integrity break. Filed as Spawn-approval trust tiers grant one-shot approval but the card reports "Trusted" #5400 with both candidate directions (narrow the surface the way fix(channels): offer only session trust on channel approval cards (#4421) #5202 did for channels, or thread a real trust decision through the spawn resolve path) — actionable by someone other than the maintainer, so it is a task, not a question. Out of scope here: fixing it changes decision plumbing on a different surface, where this PR is a label-only change on the channels one.

  • Subtraction: shrink trustAllLabelKey?: string to a boolean (e.g. channelScopedTrust) — rebutted again, now answering the new argument rather than repeating the previous round's reasoning.

    The new framing is that an open key "lets any future surface label a trust grant with any string, unreviewed". Two things make that not hold. First, the string is not a runtime input: it is a literal at the call site, so a new surface labelling its grant is a reviewed diff line either way — a boolean would put the same decision in the same diff, just spelled differently. Second, a wrong key fails loudly and identically in every locale (i18next returns the dotted key as its own fallback), so the failure mode is a visibly broken menu item in review or in any locale test run, not a silently wrong consent label. What the boolean would add is real: TrustDropdown would then hardcode which of its three call sites is "the channel one", while ChatInput and ActivityViewer pass nothing — and Spawn-approval trust tiers grant one-shot approval but the card reports "Trusted" #5400 above is precisely a third surface that needs its own wording, which a channelScopedTrust boolean cannot express without becoming an enum. Keeping the label key with the surface that knows what its own trust action does is the invariant this fix establishes; I would rather keep it than trade it for one line of narrower typing.

@bolichen97

Copy link
Copy Markdown
Collaborator Author

🤖 Kiro Crew Auto-Pipeline [operator: bolichen97#66809557]: Review-ready at 99ca30e36ece (post-rebase) — 64/64 checks green, PR Readiness passed, 0 unresolved threads. All five AI lanes re-reviewed this head: GPT ✅, Opus ✅, Design ✅, UX ✅, First Principles 🟡 advisory — both its items dispositioned above (the ActivityViewer spawn-approval sibling is verified and filed as #5400; the boolean-prop subtraction is rebutted on its new framing). Ready for human review and merge.

@bolichen97
bolichen97 merged commit 7845e12 into main Aug 24, 2026
70 of 72 checks passed
@bolichen97
bolichen97 deleted the fix/channel-trust-label-scope-5203 branch August 24, 2026 00:57
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Aug 24, 2026
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.

Channel "Trust all tools" label understates a channel-wide persistent grant

3 participants