From 4ce3e92acdd651739d16ce32b1d0acba37152f01 Mon Sep 17 00:00:00 2001 From: "daniel.ochoa" Date: Tue, 24 Mar 2026 11:08:13 -0500 Subject: [PATCH 1/2] FEAT-133: Fix approval dialog labels and add none tier - 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 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 --- apps/desktop/package.json | 2 +- apps/desktop/src/main/app.ts | 28 +++++---- apps/desktop/src/main/approval-policy.ts | 11 ++-- apps/desktop/src/main/approval-store.ts | 4 +- apps/desktop/src/main/settings-store.ts | 18 ++++++ apps/desktop/src/renderer/index.html | 66 +++++++++++++++++--- apps/desktop/src/shared/contracts.ts | 2 +- apps/desktop/test/approval-policy.test.ts | 8 +++ apps/desktop/test/settings-migration.test.ts | 36 +++++++++++ 9 files changed, 147 insertions(+), 28 deletions(-) diff --git a/apps/desktop/package.json b/apps/desktop/package.json index 37417022..0e1fa85c 100644 --- a/apps/desktop/package.json +++ b/apps/desktop/package.json @@ -1,6 +1,6 @@ { "name": "desktop", - "version": "0.7.0", + "version": "0.7.1", "description": "ClosedLoop Desktop", "author": "ClosedLoop AI ", "private": true, diff --git a/apps/desktop/src/main/app.ts b/apps/desktop/src/main/app.ts index a658c466..a006326c 100644 --- a/apps/desktop/src/main/app.ts +++ b/apps/desktop/src/main/app.ts @@ -29,7 +29,7 @@ import { } from "../server/operations/symphony-utils.js"; import { seedReposConfig } from "./seed-repos-config.js"; import { SUPPORTED_OPERATION_IDS, resolveOperationId } from "./approval-operations.js"; -import { shouldAutoApprove } from "./approval-policy.js"; +import { shouldAutoApprove, OPERATION_RISK_TIERS } from "./approval-policy.js"; import { ActivityLogStore } from "./activity-log-store.js"; import { ApprovalStore } from "./approval-store.js"; import { JobStore, isTerminalJobStatus } from "./job-store.js"; @@ -553,20 +553,17 @@ export class DesktopApplication { const configuredTier = (settings.autoApprovalRules[operationId] ?? settings.defaultApprovalTier) as RiskTier; - if (configuredTier === "auto" && !request.forceApproval) { - return { allow: true }; - } - const manualTier: Exclude = configuredTier === "auto" ? "high" : configuredTier; - if (shouldAutoApprove(operationId, manualTier, request.forceApproval ?? false)) { + if (shouldAutoApprove(operationId, configuredTier, request.forceApproval ?? false)) { return { allow: true }; } + const operationRisk = (OPERATION_RISK_TIERS as Record>)[operationId] ?? "high"; const reason = request.approvalReason?.trim() || - `Manual approval required for ${operationId} (${manualTier})`; + `${operationId} is ${operationRisk}-risk, but your auto-approve threshold is ${configuredTier}`; const pending = this.approvalStore.enqueue({ operationId, - riskTier: manualTier, + riskTier: operationRisk, method: request.method, path: request.path, body: request.body, @@ -750,11 +747,20 @@ export class DesktopApplication { relayOrigin?: string; apiOrigin?: string; webAppOrigin?: string; - defaultApprovalTier?: "auto" | "low" | "medium" | "high"; - autoApprovalRules?: Record; + defaultApprovalTier?: "auto" | "none" | "low" | "medium" | "high"; + autoApprovalRules?: Record; }) => { const currentSettings = this.settingsStore.getAll(); const nextPartial = { ...partial }; + // Normalize legacy "auto" tier to "high" (they behave identically) + if (nextPartial.defaultApprovalTier === "auto") { + nextPartial.defaultApprovalTier = "high"; + } + if (nextPartial.autoApprovalRules) { + for (const [key, val] of Object.entries(nextPartial.autoApprovalRules)) { + if (val === "auto") nextPartial.autoApprovalRules[key] = "high"; + } + } if (typeof partial.relayOrigin === "string") { nextPartial.relayOrigin = normalizeAndValidateOrigin(partial.relayOrigin); } @@ -782,7 +788,7 @@ export class DesktopApplication { throw new Error("Complete onboarding requires a sandbox base directory"); } - const updated = this.settingsStore.update(nextPartial); + const updated = this.settingsStore.update(nextPartial as Partial); if ( typeof partial.sandboxBaseDirectory === "string" && diff --git a/apps/desktop/src/main/approval-policy.ts b/apps/desktop/src/main/approval-policy.ts index 38cf1e6d..8dbb5c22 100644 --- a/apps/desktop/src/main/approval-policy.ts +++ b/apps/desktop/src/main/approval-policy.ts @@ -5,7 +5,7 @@ import type { OperationId } from "./approval-operations.js"; * Per-operation inherent risk tiers. The risk assigned reflects the * highest-risk HTTP method that each approval ID handles. */ -export const OPERATION_RISK_TIERS: Record> = { +export const OPERATION_RISK_TIERS: Record> = { health_check: "low", repos_config: "medium", filesystem: "medium", @@ -34,9 +34,10 @@ export const OPERATION_RISK_TIERS: Record learnings: "medium" }; -/** Converts a non-auto RiskTier to a numeric value for threshold comparison. */ -export function riskTierOrder(tier: Exclude): number { +/** Converts a RiskTier to a numeric value for threshold comparison. */ +export function riskTierOrder(tier: RiskTier): number { switch (tier) { + case "none": return 0; case "low": return 1; case "medium": return 2; case "high": return 3; @@ -49,10 +50,10 @@ export function riskTierOrder(tier: Exclude): number { */ export function shouldAutoApprove( operationId: string, - configuredTier: Exclude, + configuredTier: RiskTier, forceApproval: boolean ): boolean { if (forceApproval) return false; - const operationRisk = (OPERATION_RISK_TIERS as Record>)[operationId] ?? "high"; + const operationRisk = (OPERATION_RISK_TIERS as Record>)[operationId] ?? "high"; return riskTierOrder(operationRisk) <= riskTierOrder(configuredTier); } diff --git a/apps/desktop/src/main/approval-store.ts b/apps/desktop/src/main/approval-store.ts index a34f7a59..6fb288f2 100644 --- a/apps/desktop/src/main/approval-store.ts +++ b/apps/desktop/src/main/approval-store.ts @@ -8,7 +8,7 @@ export type PendingApproval = { id: string; createdAt: string; operationId: string; - riskTier: Exclude; + riskTier: Exclude; method: string; path: string; scopePath?: string; @@ -74,7 +74,7 @@ export class ApprovalStore { enqueue(input: { operationId: string; - riskTier: Exclude; + riskTier: Exclude; method: string; path: string; body: string; diff --git a/apps/desktop/src/main/settings-store.ts b/apps/desktop/src/main/settings-store.ts index c9423525..b009c068 100644 --- a/apps/desktop/src/main/settings-store.ts +++ b/apps/desktop/src/main/settings-store.ts @@ -66,6 +66,24 @@ export class SettingsStore { if (hadAuthApiOrigin) { this.store.delete("authApiOrigin" as keyof DesktopSettings); } + + // Migration: replace legacy "auto" tier with "high" (identical behavior). + if (raw.defaultApprovalTier === "auto") { + this.store.set("defaultApprovalTier", "high" as RiskTier); + } + const rules = raw.autoApprovalRules as Record | undefined; + if (rules) { + let rulesChanged = false; + for (const [key, val] of Object.entries(rules)) { + if (val === "auto") { + rules[key] = "high"; + rulesChanged = true; + } + } + if (rulesChanged) { + this.store.set("autoApprovalRules", rules as unknown as Record); + } + } } getAll(): DesktopSettings { diff --git a/apps/desktop/src/renderer/index.html b/apps/desktop/src/renderer/index.html index 19ab8a80..1a9d71b2 100644 --- a/apps/desktop/src/renderer/index.html +++ b/apps/desktop/src/renderer/index.html @@ -514,6 +514,48 @@ line-height: 1.4; } + .approval-risk-tier { + display: inline-flex; + align-items: center; + border-radius: 4px; + padding: 1px 7px; + font-size: 10px; + font-weight: 700; + letter-spacing: 0.04em; + text-transform: uppercase; + flex-shrink: 0; + } + + .approval-risk-tier.risk-low { + background: rgba(22, 163, 74, 0.12); + color: #16a34a; + } + + .approval-risk-tier.risk-medium { + background: rgba(245, 158, 11, 0.12); + color: #d97706; + } + + .approval-risk-tier.risk-high { + background: rgba(220, 38, 38, 0.12); + color: #dc2626; + } + + @media (prefers-color-scheme: dark) { + .approval-risk-tier.risk-low { + background: rgba(22, 163, 74, 0.18); + color: #4ade80; + } + .approval-risk-tier.risk-medium { + background: rgba(245, 158, 11, 0.18); + color: #fbbf24; + } + .approval-risk-tier.risk-high { + background: rgba(220, 38, 38, 0.18); + color: #f87171; + } + } + .approval-reason { color: var(--ink); font-size: 13px; @@ -1427,10 +1469,10 @@

Approval Policy

@@ -1445,7 +1487,7 @@

Approval Policy

- +
@@ -2114,6 +2156,13 @@

Always-Allow Rules

title.textContent = operationLabel(approval.operationId); header.appendChild(title); + if (approval.riskTier) { + const riskBadge = document.createElement("span"); + riskBadge.className = `approval-risk-tier risk-${approval.riskTier}`; + riskBadge.textContent = approval.riskTier; + header.appendChild(riskBadge); + } + const badge = document.createElement("span"); badge.className = `approval-badge ${statusKey}`; badge.textContent = resolved @@ -2232,7 +2281,7 @@

Always-Allow Rules

apiOrigin.value = settings.apiOrigin || ""; webAppOrigin.value = settings.webAppOrigin || ""; sandboxBaseDirectory.value = settings.sandboxBaseDirectory || ""; - defaultApprovalTier.value = settings.defaultApprovalTier || "high"; + defaultApprovalTier.value = settings.defaultApprovalTier === "auto" ? "high" : (settings.defaultApprovalTier || "high"); renderTierOverrides(settings.autoApprovalRules || {}); renderAlwaysAllowRules(settings.alwaysAllowRules || []); updateSandboxBaseWarning(); @@ -2247,7 +2296,7 @@

Always-Allow Rules

"health_check", "repos_config", "deploy", "filesystem" ]; - const TIER_OPTIONS = ["high", "medium", "low", "auto"]; + const TIER_OPTIONS = ["high", "medium", "low", "none"]; function renderTierOverrides(rules) { // Sync hidden input for save @@ -2272,7 +2321,8 @@

Always-Allow Rules

const opt = document.createElement("option"); opt.value = t; opt.textContent = t; - if (t === tier) opt.selected = true; + const normalizedTier = tier === "auto" ? "high" : tier; + if (t === normalizedTier) opt.selected = true; sel.appendChild(opt); } sel.addEventListener("change", () => { diff --git a/apps/desktop/src/shared/contracts.ts b/apps/desktop/src/shared/contracts.ts index 179c0992..dd08004f 100644 --- a/apps/desktop/src/shared/contracts.ts +++ b/apps/desktop/src/shared/contracts.ts @@ -35,7 +35,7 @@ export interface HealthResponse { port: number; } -export type RiskTier = "auto" | "low" | "medium" | "high"; +export type RiskTier = "none" | "low" | "medium" | "high"; export interface AlwaysAllowRule { id: string; diff --git a/apps/desktop/test/approval-policy.test.ts b/apps/desktop/test/approval-policy.test.ts index 5f2f4705..1ad0edc6 100644 --- a/apps/desktop/test/approval-policy.test.ts +++ b/apps/desktop/test/approval-policy.test.ts @@ -6,6 +6,7 @@ import { SUPPORTED_OPERATION_IDS, resolveOperationId } from "../src/main/approva // --- riskTierOrder --- test("riskTierOrder returns correct numeric ordering", () => { + assert.ok(riskTierOrder("none") < riskTierOrder("low")); assert.ok(riskTierOrder("low") < riskTierOrder("medium")); assert.ok(riskTierOrder("medium") < riskTierOrder("high")); }); @@ -30,6 +31,13 @@ test("policy high: auto-approves all mapped operations", () => { assert.equal(shouldAutoApprove("deploy", "high", false), true); }); +test("policy none: blocks all operations including low-risk", () => { + assert.equal(shouldAutoApprove("health_check", "none", false), false); + assert.equal(shouldAutoApprove("symphony_loop", "none", false), false); + assert.equal(shouldAutoApprove("deploy", "none", false), false); + assert.equal(shouldAutoApprove("unknown_op", "none", false), false); +}); + test("forceApproval overrides threshold", () => { assert.equal(shouldAutoApprove("health_check", "low", true), false); }); diff --git a/apps/desktop/test/settings-migration.test.ts b/apps/desktop/test/settings-migration.test.ts index 961fcf91..2377bc31 100644 --- a/apps/desktop/test/settings-migration.test.ts +++ b/apps/desktop/test/settings-migration.test.ts @@ -108,6 +108,42 @@ test("migration: fresh install applies defaults", () => { assert.equal("authApiOrigin" in all, false, "no stale authApiOrigin key should be present"); }); +// --- Approval tier "auto" → "high" migration --- + +test("migration: defaultApprovalTier 'auto' is rewritten to 'high'", () => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "settings-migration-auto-tier-")); + tempDirs.push(tmpDir); + + const storeName = "test-auto-tier"; + fs.writeFileSync( + path.join(tmpDir, `${storeName}.json`), + JSON.stringify({ + defaultApprovalTier: "auto", + autoApprovalRules: { deploy: "auto", health_check: "low" } + }) + ); + + const store = new SettingsStore({ cwd: tmpDir, name: storeName }); + const all = store.getAll(); + + assert.equal(all.defaultApprovalTier, "high", "defaultApprovalTier should be migrated to 'high'"); + assert.equal( + (all.autoApprovalRules as Record).deploy, + "high", + "autoApprovalRules 'auto' entries should be migrated to 'high'" + ); + assert.equal( + (all.autoApprovalRules as Record).health_check, + "low", + "non-auto autoApprovalRules entries should be preserved" + ); + + // Verify persisted JSON no longer contains "auto" + const persisted = JSON.parse(fs.readFileSync(path.join(tmpDir, `${storeName}.json`), "utf-8")); + assert.notEqual(persisted.defaultApprovalTier, "auto", "persisted defaultApprovalTier should not be 'auto'"); + assert.notEqual(persisted.autoApprovalRules?.deploy, "auto", "persisted autoApprovalRules should not contain 'auto'"); +}); + test("migration: already migrated install is a no-op — both values preserved", () => { const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "settings-migration-noop-")); tempDirs.push(tmpDir); From 82ae674826f546957cfb5f5785a1182be8f0fae4 Mon Sep 17 00:00:00 2001 From: "daniel.ochoa" Date: Tue, 24 Mar 2026 13:41:21 -0500 Subject: [PATCH 2/2] FEAT-133: Use positive assertions in migration test - Replace notEqual (auto) with equal (high) for stronger verification that persisted values are correctly migrated, not just not-auto --- apps/desktop/test/settings-migration.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/desktop/test/settings-migration.test.ts b/apps/desktop/test/settings-migration.test.ts index 2377bc31..6276c6ea 100644 --- a/apps/desktop/test/settings-migration.test.ts +++ b/apps/desktop/test/settings-migration.test.ts @@ -140,8 +140,8 @@ test("migration: defaultApprovalTier 'auto' is rewritten to 'high'", () => { // Verify persisted JSON no longer contains "auto" const persisted = JSON.parse(fs.readFileSync(path.join(tmpDir, `${storeName}.json`), "utf-8")); - assert.notEqual(persisted.defaultApprovalTier, "auto", "persisted defaultApprovalTier should not be 'auto'"); - assert.notEqual(persisted.autoApprovalRules?.deploy, "auto", "persisted autoApprovalRules should not contain 'auto'"); + assert.equal(persisted.defaultApprovalTier, "high", "persisted defaultApprovalTier should be 'high'"); + assert.equal(persisted.autoApprovalRules?.deploy, "high", "persisted autoApprovalRules.deploy should be 'high'"); }); test("migration: already migrated install is a no-op — both values preserved", () => {