Skip to content

fix(tui): label session-scoped approval honestly, not "always" (#3766)#3773

Merged
Hmbown merged 1 commit into
mainfrom
codex/0866-3766-approval-session-copy
Jun 29, 2026
Merged

fix(tui): label session-scoped approval honestly, not "always" (#3766)#3773
Hmbown merged 1 commit into
mainfrom
codex/0866-3766-approval-session-copy

Conversation

@Hmbown

@Hmbown Hmbown commented Jun 29, 2026

Copy link
Copy Markdown
Owner

Summary

Closes #3766. Approval option 2 / a maps to ReviewDecision::ApprovedForSession (stored only in the in-memory app.approval_session_approved set), but the UI said "always" / "Approve always for this kind" — implying permanence. The actual cross-session persistence path is the separate [s] save-rule action. This corrects the copy so the trust boundary is honest.

Changes

  • Compact controls (crates/tui/src/tui/widgets/mod.rs): [a] now renders session (en) / 本会话 (zh-Hans) instead of always / 始终.
  • Full option label (ApprovalOptionApproveAlways): now session-scoped in en, es-419, ja, pt-BR, vi. zh-Hans / zh-Hant already said 本会话 / 本會話 and are unchanged.
  • Keyboard hints (a, option 2) unchanged.
  • Regression test approval_option_two_reads_as_session_scoped_not_always asserts both full and compact paths say "session" and never "always".

Validation

  • cargo test -p codewhale-tui --bin codewhale-tui approval → 167 passed (incl. benign_a_two_approves_for_session and the new test).

Acceptance criteria

  • Compact controls no longer render "always" for the session option
  • Full option text no longer renders "Approve always"
  • Label clearly communicates current-session scope
  • Approval stays session-scoped (no accidental persistence)
  • benign_a_two_approves_for_session still passes
  • Added a copy regression test

The approval modal's option 2 / `a` maps to
`ReviewDecision::ApprovedForSession` and is stored only in the in-memory
`app.approval_session_approved` set, yet the compact controls said "always"
and the full option said "Approve always for this kind". That is a
trust-boundary copy bug: the real cross-session persistence path is the
separate `[s]` save-rule action, so users could believe a session-only
approval was permanent.

- Compact controls now render `[a] session` (en) / `本会话` (zh-Hans).
- Full option label is now session-scoped in en/es-419/ja/pt-BR/vi; the
  zh-Hans/zh-Hant strings already said 本会话/本會話 and are unchanged.
- Keyboard hints (`a`, option 2) are unchanged.
- Adds `approval_option_two_reads_as_session_scoped_not_always`, asserting
  both the full and compact paths say "session" and never "always".

Fixes #3766
@Hmbown Hmbown added this to the v0.8.66 milestone Jun 29, 2026
@claude

claude Bot commented Jun 29, 2026

Copy link
Copy Markdown

Claude finished @Hmbown's task in 1m 13s —— View job


PR Review: fix(tui): label session-scoped approval honestly, not "always"

  • Read changed files and diff
  • Verify locale changes across all locales
  • Review widget code and regression test
  • Summarize findings

Summary

This is a correct, well-scoped fix. Option 2 / [a] maps to ReviewDecision::ApprovedForSession, which is stored in the in-memory approval_session_approved set and does not persist across sessions. Calling it "always" was a trust-boundary lie. The fix is accurate.

What's Good

  • The behavioral invariant is untouched — only the display string changes. benign_a_two_approves_for_session continues to pass.
  • The regression test approval_option_two_reads_as_session_scoped_not_always covers both the full-card and compact paths and asserts the absence of "always" — exactly the right shape for a copy-correctness test.
  • zh-Hans and zh-Hant already used session-scoped language (本会话/本會話), so those locales are consistent and correctly left unchanged.
  • The comment added to push_compact_approval_controls explains the semantic link between [a] and ReviewDecision::ApprovedForSession, which is genuinely non-obvious to a future reader.

Findings

1. Stale identifiers now semantically wrong (not a blocker, but creates future confusion)

crates/tui/src/tui/widgets/mod.rs:1902 — the function is still called option_approve_always and dispatches through MessageId::ApprovalOptionApproveAlways. The JSON key is also "ApprovalOptionApproveAlways". These names now contradict the displayed text and the actual behaviour. This is all pre-existing naming, so it's out of scope to fix here, but it's worth a follow-up rename to option_approve_for_session / ApprovalOptionApproveForSession to stop future contributors from (incorrectly) concluding the option is persistent.

2. ZhHant (Traditional Chinese) compact controls still fall through to English (pre-existing, not a regression)

push_compact_approval_controls at line 1764 only special-cases Locale::ZhHans and falls back to English for everything else including ZhHant. Since zh-Hant.json already has a session-scoped full-card label (本會話同類自動批准), ZhHant compact controls showing "session" in English is inconsistent but also pre-existing. Not introduced by this PR.

3. Regression test uses only the default (en) locale

approval_option_two_reads_as_session_scoped_not_always checks "this session" and "always" which are English strings. It doesn't exercise zh-Hans, es-419, pt-BR, vi, or ja. That's acceptable coverage for a copy-correctness test (the JSON changes speak for themselves), but a quick assert!(!rendered.to_lowercase().contains("siempre")) for es-419 would be belt-and-suspenders. Optional, not required.

4. No correctness bugs, panics, or unsafe code introduced

All changed paths are pure rendering — no fallible operations, no new allocations beyond existing patterns.

Verdict

Ready to merge. The fix is correct and the regression test gives appropriate coverage. The stale option_approve_always / ApprovalOptionApproveAlways identifiers are a pre-existing naming issue worth a follow-up issue, not a blocker for this PR.

@Hmbown Hmbown merged commit ceb7c60 into main Jun 29, 2026
19 checks passed
@Hmbown Hmbown deleted the codex/0866-3766-approval-session-copy branch June 29, 2026 16:54
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.

v0.8.66: Correct approval UI copy for session-scoped approvals

1 participant