Skip to content
This repository was archived by the owner on Jun 8, 2026. It is now read-only.

FEAT-133: Fix approval dialog labels and add none tier - #37

Merged
shafty023 merged 2 commits into
mainfrom
FEAT-138
Mar 24, 2026
Merged

FEAT-133: Fix approval dialog labels and add none tier#37
shafty023 merged 2 commits into
mainfrom
FEAT-138

Conversation

@shafty023

Copy link
Copy Markdown
Contributor
  • Fix misleading approval reason: show operation risk tier, not user threshold
  • Add color-coded risk badge (low/medium/high) to approval card header
  • Replace 'auto' tier with 'none' (prompt for all operations)
  • Add 'none' to riskTierOrder (returns 0, blocks all auto-approvals)
  • Migrate persisted 'auto' values to 'high' in SettingsStore constructor
  • Clarify dropdown labels to describe when user will be prompted
  • Preserve Exclude<RiskTier, 'none'> for operation risk types (type safety)

Testing:

  • just desktop-typecheck passes
  • just desktop-test passes (288/288)
  • just desktop-lint passes

Risks:

  • Users with persisted 'auto' tier get migrated to 'high' (same behavior)
  • The 'none' tier is new; users must explicitly select it

- Fix misleading approval reason: show operation risk tier, not user threshold
- Add color-coded risk badge (low/medium/high) to approval card header
- Replace 'auto' tier with 'none' (prompt for all operations)
- Add 'none' to riskTierOrder (returns 0, blocks all auto-approvals)
- Migrate persisted 'auto' values to 'high' in SettingsStore constructor
- Clarify dropdown labels to describe when user will be prompted
- Preserve Exclude<RiskTier, 'none'> for operation risk types (type safety)

Testing:
- just desktop-typecheck passes
- just desktop-test passes (288/288)
- just desktop-lint passes

Risks:
- Users with persisted 'auto' tier get migrated to 'high' (same behavior)
- The 'none' tier is new; users must explicitly select it
Comment thread apps/desktop/test/settings-migration.test.ts Outdated
@closedloop-ai-stage

Copy link
Copy Markdown

Code Review Summary

Status: Approved

Reviewers: Bug Hunter A, Bug Hunter B, Unified Auditor, Premise Reviewer, electron-specialist

Findings

Severity Count
Blocking 0
High 0
Medium 1

MEDIUM Issues (consider)

  1. [P3] [apps/desktop/test/settings-migration.test.ts:143] Weak assert.notEqual assertions allow false-positive passes

The persisted-JSON verification uses negative assertions that pass when keys are absent.
Use assert.equal to confirm the migrated values are set to the expected string.

Validation Stats

  • Agent failures: 0 partitions skipped
  • Cross-file grouped: 0 findings consolidated

Recommendation: Approve - no blocking or high-severity issues. Consider strengthening test assertions.

- Replace notEqual (auto) with equal (high) for stronger verification
  that persisted values are correctly migrated, not just not-auto
@shafty023
shafty023 merged commit 178c806 into main Mar 24, 2026
2 checks passed
@shafty023
shafty023 deleted the FEAT-138 branch March 24, 2026 18:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant