From b0a590bedd1fbb72317433c78a8302c6c0f76b7a Mon Sep 17 00:00:00 2001 From: Carlos Mora Date: Wed, 23 Sep 2026 17:34:43 -0500 Subject: [PATCH 1/6] fix(profiles): require confirmation before applying an empty profile (#1349) In extensions/gentle-ai.ts, runProfilesPanelAction applied empty profiles without confirmation on the global apply path. Because an empty profile has zero routing entries, writeModelConfigAsync overwrote models.json with {} and withOmittedAgentsClearedAsync cleared every discoverable subagent in subagents.json, destroying the user'\''s model routing configuration. 1. Prompt for explicit confirmation via ctx.ui.confirm when applying a profile with zero routing entries (Object.keys(normalized).length === 0), naming the destructive effect on global routing and agent inheritance. 2. Abort immediately if declined, preserving models.json, subagents.json, and the store active marker. --- extensions/gentle-ai.ts | 7 +++++ tests/gentle-ai.test.ts | 59 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+) diff --git a/extensions/gentle-ai.ts b/extensions/gentle-ai.ts index d5814b9a5..a16d4ffe9 100644 --- a/extensions/gentle-ai.ts +++ b/extensions/gentle-ai.ts @@ -4229,6 +4229,13 @@ async function runProfilesPanelAction( } const normalized = normalizeModelConfig(file.profiles[result.name]) ?? {}; const orchestratorEntry = readProfileOrchestrator(normalized); + if (Object.keys(normalized).length === 0) { + const approved = await ctx.ui.confirm( + "Apply empty profile?", + `Profile "${result.name}" has no routing entries. Applying it will replace global routing in ${sanitizeTerminalText(modelConfigPath(ctx.cwd))} with an empty configuration and return every agent to inherit its default model. Continue?`, + ); + if (!approved) return file; + } // Applying spans three files — the store, models.json, and Pi's global // settings.json — and there is no cross-file rename, so order the writes to // keep the store truthful and compensate on failure: claim the profile in diff --git a/tests/gentle-ai.test.ts b/tests/gentle-ai.test.ts index c0c00f168..e941f9aba 100644 --- a/tests/gentle-ai.test.ts +++ b/tests/gentle-ai.test.ts @@ -395,6 +395,8 @@ function routingConsumerFixture(t: test.TestContext, agents = ["worker"]) { { provider: "openai", id: "beta" }, { provider: "nan", id: "glm5.3" }, ]; + let onConfirm: (title: string, message: string) => Promise = async () => true; + const confirmCalls: Array<[string, string]> = []; const ctx = { cwd: root, hasUI: true, @@ -403,6 +405,10 @@ function routingConsumerFixture(t: test.TestContext, agents = ["worker"]) { find: (provider: string, id: string) => registryModels.find((model) => model.provider === provider && model.id === id), }, ui: { + confirm: async (title: string, message: string) => { + confirmCalls.push([title, message]); + return onConfirm(title, message); + }, notify(message: string, severity: string) { notifications.push({ message, severity }); }, custom: async (factory: (tui: unknown, theme: Theme, keybindings: unknown, done: (result: unknown) => void) => RoutingConsumerPanel) => { let result: unknown; @@ -425,6 +431,8 @@ function routingConsumerFixture(t: test.TestContext, agents = ["worker"]) { liveSwitches, refuseSetModel() { setModelResult = false; }, rejectThinkingLevel() { thinkingRejects = true; }, + confirmCalls, + onConfirm(handler: (title: string, message: string) => Promise) { onConfirm = handler; }, onPanel(action: typeof onPanel) { onPanel = action; }, onInput(action: (panel: RoutingConsumerPanel) => void) { onInput = action; }, run: (name: string) => commands.get(name)!.handler("", ctx), @@ -2016,6 +2024,57 @@ test("a profile store entry with only the orchestrator key counts zero roles", a assert.match(applied, /Orchestrator set to nan\/glm5\.3 · high/); }); +test("applying an empty profile asks for confirmation and aborts when declined", async (t) => { + const { fixture, storePath, writeStore, writeSettings } = profilesStoreFixture(t); + writeSettings(); + mkdirSync(fixture.configHome, { recursive: true }); + writeFileSync(fixture.globalPath, `${JSON.stringify({ worker: { model: "openai/alpha" } }, null, 2)}\n`); + writeStore({ + empty: {}, + team: { worker: { model: "openai/alpha" } }, + }, "team"); + + fixture.onConfirm(async () => false); + + applyOnce(fixture); + await fixture.run("gentle:profiles"); + + assert.equal(fixture.confirmCalls.length, 1, "confirm dialog must be displayed when applying an empty profile"); + const [title, message] = fixture.confirmCalls[0]; + assert.equal(title, "Apply empty profile?"); + assert.match(message, /has no routing entries/); + assert.match(message, /replace global routing/); + + const models = JSON.parse(readFileSync(fixture.globalPath, "utf8")); + assert.deepEqual(models, { worker: { model: "openai/alpha" } }, "global routing must NOT be wiped when declined"); + + const store = JSON.parse(readFileSync(storePath, "utf8")); + assert.equal(store.active, "team", "active profile must not change when declined"); +}); + +test("applying an empty profile with explicit confirmation replaces global routing with empty config", async (t) => { + const { fixture, storePath, writeStore, writeSettings } = profilesStoreFixture(t); + writeSettings(); + mkdirSync(fixture.configHome, { recursive: true }); + writeFileSync(fixture.globalPath, `${JSON.stringify({ worker: { model: "openai/alpha" } }, null, 2)}\n`); + writeStore({ + empty: {}, + team: { worker: { model: "openai/alpha" } }, + }, "team"); + + fixture.onConfirm(async () => true); + + applyOnce(fixture); + await fixture.run("gentle:profiles"); + + assert.equal(fixture.confirmCalls.length, 1, "confirm dialog must be displayed when applying an empty profile"); + const models = JSON.parse(readFileSync(fixture.globalPath, "utf8")); + assert.deepEqual(models, {}, "global routing must be replaced with empty config when confirmed"); + + const store = JSON.parse(readFileSync(storePath, "utf8")); + assert.equal(store.active, "empty", "active profile must be set to empty when confirmed"); +}); + test("applying a profile replaces materialized routing for agents the profile omits", async (t) => { const { fixture, writeStore, writeSettings } = profilesStoreFixture(t); writeSettings(); From 3395831e07b5047662fec081d8459b432e6394c7 Mon Sep 17 00:00:00 2001 From: Carlos Mora Date: Thu, 24 Sep 2026 06:50:37 -0500 Subject: [PATCH 2/6] fix(profiles): prompt before applying orchestrator-only profiles without agent routes (#1349) Address CodeRabbit review finding on PR #1384: 1. In extensions/gentle-ai.ts, check for the presence of agent routing entries separately from the orchestrator key when applying profiles. 2. Prompt for confirmation whenever no agent routes are present, including orchestrator-only profiles, preventing unconfirmed clearing of omitted agents. 3. Add regression tests in tests/gentle-ai.test.ts covering orchestrator-only profile application decline and confirmation. --- extensions/gentle-ai.ts | 3 +- odd/tasks/pr-1384-review-fixes.md | 27 +++++++++++++++ tests/gentle-ai.test.ts | 56 +++++++++++++++++++++++++++++++ 3 files changed, 85 insertions(+), 1 deletion(-) create mode 100644 odd/tasks/pr-1384-review-fixes.md diff --git a/extensions/gentle-ai.ts b/extensions/gentle-ai.ts index a16d4ffe9..d396c0002 100644 --- a/extensions/gentle-ai.ts +++ b/extensions/gentle-ai.ts @@ -4229,7 +4229,8 @@ async function runProfilesPanelAction( } const normalized = normalizeModelConfig(file.profiles[result.name]) ?? {}; const orchestratorEntry = readProfileOrchestrator(normalized); - if (Object.keys(normalized).length === 0) { + const hasAgentRoutes = Object.keys(normalized).some((name) => !isProfileOrchestratorKey(name)); + if (!hasAgentRoutes) { const approved = await ctx.ui.confirm( "Apply empty profile?", `Profile "${result.name}" has no routing entries. Applying it will replace global routing in ${sanitizeTerminalText(modelConfigPath(ctx.cwd))} with an empty configuration and return every agent to inherit its default model. Continue?`, diff --git a/odd/tasks/pr-1384-review-fixes.md b/odd/tasks/pr-1384-review-fixes.md new file mode 100644 index 000000000..9af86de9f --- /dev/null +++ b/odd/tasks/pr-1384-review-fixes.md @@ -0,0 +1,27 @@ +# PR #1384 Review Fixes + +## Objective + +Close the verified CodeRabbit review finding on PR #1384 (`#1349` empty profile wipe guard) by requiring confirmation when applying orchestrator-only profiles that lack agent routes. + +## Problem + +Commit `b0a590be` checked `Object.keys(normalized).length === 0` to prompt for confirmation. However, a profile that contains only an orchestrator entry (`orchestrator: { model: "..." }`) has `Object.keys(normalized).length === 1`. Applying it skipped confirmation, replaced global routing with the orchestrator-only config, and wiped materialized routes for all omitted agents in `subagents.json`. + +## Scope + +- In `extensions/gentle-ai.ts`, check whether any agent routes exist (excluding the reserved orchestrator key) before applying a profile. +- Add regression coverage in `tests/gentle-ai.test.ts` for orchestrator-only profiles. +- Verify typecheck and tests. + +## Constraints + +- Keep the patch minimal and limited to PR #1384 review findings. +- Technical artifacts remain in English. +- Do not commit, push, or merge without explicit user direction. +- Strict TDD discipline: RED test confirmed before implementation fix. + +## Tasks + +- [x] **T1 — Guard orchestrator-only profiles against unconfirmed agent route wipes.** Check for absence of agent routes (e.g. `!Object.keys(normalized).some((name) => !isProfileOrchestratorKey(name))`) in the profile apply flow. +- [x] **T2 — Regression tests and typecheck verification.** Verify orchestrator-only profile application prompts for confirmation and aborts when declined. diff --git a/tests/gentle-ai.test.ts b/tests/gentle-ai.test.ts index e941f9aba..d09341be9 100644 --- a/tests/gentle-ai.test.ts +++ b/tests/gentle-ai.test.ts @@ -2075,6 +2075,62 @@ test("applying an empty profile with explicit confirmation replaces global routi assert.equal(store.active, "empty", "active profile must be set to empty when confirmed"); }); +test("applying an orchestrator-only profile asks for confirmation and aborts when declined", async (t) => { + const { fixture, storePath, writeStore, writeSettings } = profilesStoreFixture(t); + writeSettings(); + mkdirSync(fixture.configHome, { recursive: true }); + writeFileSync(fixture.globalPath, `${JSON.stringify({ worker: { model: "openai/alpha" } }, null, 2)}\n`); + writeStore({ + orchOnly: { orchestrator: { model: "nan/glm5.3", thinking: "high" } }, + team: { worker: { model: "openai/alpha" } }, + }, "team"); + + fixture.onConfirm(async () => false); + + applyOnce(fixture); + await fixture.run("gentle:profiles"); + + assert.equal(fixture.confirmCalls.length, 1, "confirm dialog must be displayed when applying an orchestrator-only profile"); + const [title, message] = fixture.confirmCalls[0]; + assert.equal(title, "Apply empty profile?"); + assert.match(message, /has no routing entries/); + assert.match(message, /replace global routing/); + + const models = JSON.parse(readFileSync(fixture.globalPath, "utf8")); + assert.deepEqual(models, { worker: { model: "openai/alpha" } }, "global routing must NOT be wiped when declined"); + + const store = JSON.parse(readFileSync(storePath, "utf8")); + assert.equal(store.active, "team", "active profile must not change when declined"); +}); + +test("applying an orchestrator-only profile with explicit confirmation updates orchestrator and active profile", async (t) => { + const { fixture, storePath, writeStore, writeSettings, settingsPath } = profilesStoreFixture(t); + writeSettings(); + mkdirSync(fixture.configHome, { recursive: true }); + writeFileSync(fixture.globalPath, `${JSON.stringify({ worker: { model: "openai/alpha" } }, null, 2)}\n`); + writeStore({ + orchOnly: { orchestrator: { model: "nan/glm5.3", thinking: "high" } }, + team: { worker: { model: "openai/alpha" } }, + }, "team"); + + fixture.onConfirm(async () => true); + + applyOnce(fixture); + await fixture.run("gentle:profiles"); + + assert.equal(fixture.confirmCalls.length, 1, "confirm dialog must be displayed when applying an orchestrator-only profile"); + const models = JSON.parse(readFileSync(fixture.globalPath, "utf8")); + assert.deepEqual(models, { orchestrator: { model: "nan/glm5.3", thinking: "high" } }, "global routing must hold only orchestrator"); + + const store = JSON.parse(readFileSync(storePath, "utf8")); + assert.equal(store.active, "orchOnly", "active profile must be set to orchOnly when confirmed"); + + const settings = JSON.parse(readFileSync(settingsPath, "utf8")); + assert.equal(settings.defaultProvider, "nan"); + assert.equal(settings.defaultModel, "glm5.3"); + assert.equal(settings.defaultThinkingLevel, "high"); +}); + test("applying a profile replaces materialized routing for agents the profile omits", async (t) => { const { fixture, writeStore, writeSettings } = profilesStoreFixture(t); writeSettings(); From efc7b52abbabcdcd2b8770a0e3a4644802ec4261 Mon Sep 17 00:00:00 2001 From: Juan Barbat Date: Wed, 30 Sep 2026 17:06:30 -0300 Subject: [PATCH 3/6] fix(profiles): clarify orchestrator-only apply confirmation --- extensions/gentle-ai.ts | 18 ++++++-- odd/tasks/pr-1384-review-fixes.md | 19 ++++++++ tests/gentle-ai.test.ts | 73 +++++++++++++++++++++++++------ 3 files changed, 93 insertions(+), 17 deletions(-) diff --git a/extensions/gentle-ai.ts b/extensions/gentle-ai.ts index d5ff0e560..6ecaa4c22 100644 --- a/extensions/gentle-ai.ts +++ b/extensions/gentle-ai.ts @@ -4150,10 +4150,20 @@ async function runProfilesPanelAction( const orchestratorEntry = readProfileOrchestrator(normalized); const hasAgentRoutes = Object.keys(normalized).some((name) => !isProfileOrchestratorKey(name)); if (!hasAgentRoutes) { - const approved = await ctx.ui.confirm( - "Apply empty profile?", - `Profile "${result.name}" has no routing entries. Applying it will replace global routing in ${sanitizeTerminalText(modelConfigPath(ctx.cwd))} with an empty configuration and return every agent to inherit its default model. Continue?`, - ); + // A genuinely empty profile and an orchestrator-only profile both wipe + // every materialized agent route, but they read very differently to the + // user: the orchestrator entry survives the apply and reconfigures the + // orchestrator, so the dialog must not promise an entirely empty config. + const [confirmTitle, confirmMessage] = orchestratorEntry !== undefined + ? [ + "Apply orchestrator-only profile?", + `Profile "${result.name}" has an orchestrator entry but no agent routing entries. Applying it will replace global routing in ${sanitizeTerminalText(modelConfigPath(ctx.cwd))} with the orchestrator entry alone, clear every materialized agent route so every agent returns to inherit its default model, set the configured orchestrator entry in settings.json, and attempt to switch this session to that orchestrator model. Continue?`, + ] + : [ + "Apply empty profile?", + `Profile "${result.name}" has no routing entries. Applying it will replace global routing in ${sanitizeTerminalText(modelConfigPath(ctx.cwd))} with an empty configuration and return every agent to inherit its default model. Continue?`, + ]; + const approved = await ctx.ui.confirm(confirmTitle, confirmMessage); if (!approved) return file; } // Applying spans three files — the store, models.json, and Pi's global diff --git a/odd/tasks/pr-1384-review-fixes.md b/odd/tasks/pr-1384-review-fixes.md index 9af86de9f..e5405ade4 100644 --- a/odd/tasks/pr-1384-review-fixes.md +++ b/odd/tasks/pr-1384-review-fixes.md @@ -25,3 +25,22 @@ Commit `b0a590be` checked `Object.keys(normalized).length === 0` to prompt for c - [x] **T1 — Guard orchestrator-only profiles against unconfirmed agent route wipes.** Check for absence of agent routes (e.g. `!Object.keys(normalized).some((name) => !isProfileOrchestratorKey(name))`) in the profile apply flow. - [x] **T2 — Regression tests and typecheck verification.** Verify orchestrator-only profile application prompts for confirmation and aborts when declined. +- [x] **T3 — Correct confirmation copy and strengthen safety assertions.** Implementation and independent nan QA complete: dialog distinguishes empty/orchestrator-only profiles and explains settings plus attempted live-session change. Decline preserves models, subagents, profile store, settings and live switches; confirmation clears omitted routes; nonempty/repo-pinned applies do not prompt. Work-unit commit pending explicit user authorization. +- [ ] **T4 — Validate the correction candidate and report delivery readiness.** Run candidate assessment and native review under the enabled user-owned RDD switch. Commit, push and PR merge remain pending explicit user direction. + +## QA follow-up (2026-09-30) + +The operator accepted independent nan QA findings on target `03ea59771bb7d18adc28f8702890b306bf7d1069`. The confirmation works, but the orchestrator-only message incorrectly promises an empty config and omits settings/live-session effects. Existing tests do not explicitly assert preservation of every affected surface or confirmed omitted-agent clearing. + +- Allowed source surfaces: `extensions/gentle-ai.ts`, `tests/gentle-ai.test.ts`. +- Route: delegated `gentle-ai-worker`; multi-file writer trigger (two nontrivial source/test files). +- TDD: strict, retained from this feature document's Constraints; observe failing copy regression before changing production code, then GREEN. +- Focused runner: `node --experimental-strip-types --test --test-name-pattern="profile" tests/gentle-ai.test.ts`. +- Closure checks: `node --experimental-strip-types --test tests/gentle-ai.test.ts`; `node scripts/check-types.mjs`; `git diff --check`. +- Environment: target requires pi-coding-agent 0.99.1; parent dependencies previously contained 0.85.1. Use matching dependencies in an isolated test worktree, without installing or modifying operator routing. +- Delivery strategy: ask-on-risk; forecast approximately 80–160 additional authored lines, one cohesive correction unit; no size-driven split required yet. +- Local branch `pr-1384` fast-forwarded to updated PR head `03ea5977`; no history rewritten. +- Progress: T3 verified locally. Writer (glm-4.7/high) observed RED for title and conditional live-switch wording, then GREEN. Independent nan/mimo-v2.6-flash QA observed focused 35/35 and full extension 94/94 tests, typecheck 187 baseline diagnostics/no regressions, and full-tree diff check exit 0 using pi 0.99.1 dependencies. Temporary test worktrees removed; parent dependencies and operator config unchanged. +- T4 blocked: native assessment returned schema-incompatible/unassessable, requiring independent verification (completed). Native inspect then stopped with managed_assets_outdated, offering `/home/jbarbat/.local/share/gentle-ai/main/gentle-ai sync --agent pi`. No lineage started and no review approval obtained. Synchronizing installed assets is outside this source-only correction and awaits operator direction. +- Remaining checks: manual TUI Escape/cancel smoke and installed packed-package check not performed; no new CI run because corrections remain local. +- Delivery: no correction commit, push or PR merge executed. Engram mirror unavailable (session already ended). Next step: operator decides whether to sync managed assets for native review and authorize delivery separately. diff --git a/tests/gentle-ai.test.ts b/tests/gentle-ai.test.ts index c27c14c72..0263da8ff 100644 --- a/tests/gentle-ai.test.ts +++ b/tests/gentle-ai.test.ts @@ -2049,6 +2049,7 @@ test("applying a profile persists its orchestrator and never leaks the key into { kind: "model", provider: "nan", id: "glm5.3" }, { kind: "thinking", level: "max" }, ], "the live session switches to the profile's orchestrator"); + assert.equal(fixture.confirmCalls.length, 0, "a nonempty global apply never asks for confirmation"); }); test("applying a profile whose orchestrator model is unknown to the registry persists the default and says the session did not switch", async (t) => { @@ -2115,14 +2116,22 @@ test("a profile store entry with only the orchestrator key counts zero roles", a }); test("applying an empty profile asks for confirmation and aborts when declined", async (t) => { - const { fixture, storePath, writeStore, writeSettings } = profilesStoreFixture(t); + const { fixture, storePath, writeStore, writeSettings, settingsPath } = profilesStoreFixture(t); writeSettings(); mkdirSync(fixture.configHome, { recursive: true }); writeFileSync(fixture.globalPath, `${JSON.stringify({ worker: { model: "openai/alpha" } }, null, 2)}\n`); + const subagentsPath = join(fixture.root, ".pi", "subagents.json"); + writeFileSync(subagentsPath, `${JSON.stringify({ model_profiles: { worker: { model: "openai/beta", effort: "high" } } }, null, 2)}\n`); writeStore({ empty: {}, team: { worker: { model: "openai/alpha" } }, }, "team"); + const before = { + models: readFileSync(fixture.globalPath, "utf8"), + subagents: readFileSync(subagentsPath, "utf8"), + store: readFileSync(storePath, "utf8"), + settings: readFileSync(settingsPath, "utf8"), + }; fixture.onConfirm(async () => false); @@ -2134,11 +2143,17 @@ test("applying an empty profile asks for confirmation and aborts when declined", assert.equal(title, "Apply empty profile?"); assert.match(message, /has no routing entries/); assert.match(message, /replace global routing/); - - const models = JSON.parse(readFileSync(fixture.globalPath, "utf8")); + assert.match(message, /empty configuration/); + + assert.equal(readFileSync(fixture.globalPath, "utf8"), before.models, "declined apply must preserve models.json byte-identically"); + assert.equal(readFileSync(subagentsPath, "utf8"), before.subagents, "declined apply must preserve subagents.json byte-identically"); + assert.equal(readFileSync(storePath, "utf8"), before.store, "declined apply must preserve the profiles store byte-identically"); + assert.equal(readFileSync(settingsPath, "utf8"), before.settings, "declined apply must preserve settings.json byte-identically"); + assert.deepEqual(fixture.liveSwitches, [], "declined apply must not switch the live session"); + const models = JSON.parse(before.models); assert.deepEqual(models, { worker: { model: "openai/alpha" } }, "global routing must NOT be wiped when declined"); - const store = JSON.parse(readFileSync(storePath, "utf8")); + const store = JSON.parse(before.store); assert.equal(store.active, "team", "active profile must not change when declined"); }); @@ -2147,6 +2162,8 @@ test("applying an empty profile with explicit confirmation replaces global routi writeSettings(); mkdirSync(fixture.configHome, { recursive: true }); writeFileSync(fixture.globalPath, `${JSON.stringify({ worker: { model: "openai/alpha" } }, null, 2)}\n`); + const subagentsPath = join(fixture.root, ".pi", "subagents.json"); + writeFileSync(subagentsPath, `${JSON.stringify({ model_profiles: { worker: { model: "openai/beta", effort: "high" } } }, null, 2)}\n`); writeStore({ empty: {}, team: { worker: { model: "openai/alpha" } }, @@ -2163,17 +2180,28 @@ test("applying an empty profile with explicit confirmation replaces global routi const store = JSON.parse(readFileSync(storePath, "utf8")); assert.equal(store.active, "empty", "active profile must be set to empty when confirmed"); + + const subagents = JSON.parse(readFileSync(subagentsPath, "utf8")); + assert.deepEqual(subagents, {}, "confirmed empty apply clears the seeded omitted worker route (shared clearing semantics drop the empty key)"); }); test("applying an orchestrator-only profile asks for confirmation and aborts when declined", async (t) => { - const { fixture, storePath, writeStore, writeSettings } = profilesStoreFixture(t); + const { fixture, storePath, writeStore, writeSettings, settingsPath } = profilesStoreFixture(t); writeSettings(); mkdirSync(fixture.configHome, { recursive: true }); writeFileSync(fixture.globalPath, `${JSON.stringify({ worker: { model: "openai/alpha" } }, null, 2)}\n`); + const subagentsPath = join(fixture.root, ".pi", "subagents.json"); + writeFileSync(subagentsPath, `${JSON.stringify({ model_profiles: { worker: { model: "openai/beta", effort: "high" } } }, null, 2)}\n`); writeStore({ orchOnly: { orchestrator: { model: "nan/glm5.3", thinking: "high" } }, team: { worker: { model: "openai/alpha" } }, }, "team"); + const before = { + models: readFileSync(fixture.globalPath, "utf8"), + subagents: readFileSync(subagentsPath, "utf8"), + store: readFileSync(storePath, "utf8"), + settings: readFileSync(settingsPath, "utf8"), + }; fixture.onConfirm(async () => false); @@ -2182,14 +2210,22 @@ test("applying an orchestrator-only profile asks for confirmation and aborts whe assert.equal(fixture.confirmCalls.length, 1, "confirm dialog must be displayed when applying an orchestrator-only profile"); const [title, message] = fixture.confirmCalls[0]; - assert.equal(title, "Apply empty profile?"); - assert.match(message, /has no routing entries/); - assert.match(message, /replace global routing/); - - const models = JSON.parse(readFileSync(fixture.globalPath, "utf8")); - assert.deepEqual(models, { worker: { model: "openai/alpha" } }, "global routing must NOT be wiped when declined"); - - const store = JSON.parse(readFileSync(storePath, "utf8")); + assert.equal(title, "Apply orchestrator-only profile?"); + assert.match(message, /has an orchestrator entry but no agent routing entries/); + assert.match(message, /clear every materialized agent route/); + assert.match(message, /inherit/); + assert.match(message, /orchestrator entry/); + assert.match(message, /settings\.json/); + assert.match(message, /attempt to switch this session/, "the dialog must not promise an unconditional live switch; registry/auth refusal can keep the session model"); + assert.doesNotMatch(message, /empty configuration/, "orchestrator-only must not promise an entirely empty config"); + + assert.equal(readFileSync(fixture.globalPath, "utf8"), before.models, "declined apply must preserve models.json byte-identically"); + assert.equal(readFileSync(subagentsPath, "utf8"), before.subagents, "declined apply must preserve subagents.json byte-identically"); + assert.equal(readFileSync(storePath, "utf8"), before.store, "declined apply must preserve the profiles store byte-identically"); + assert.equal(readFileSync(settingsPath, "utf8"), before.settings, "declined apply must preserve settings.json byte-identically"); + assert.deepEqual(fixture.liveSwitches, [], "declined apply must not switch the live session"); + + const store = JSON.parse(before.store); assert.equal(store.active, "team", "active profile must not change when declined"); }); @@ -2198,6 +2234,8 @@ test("applying an orchestrator-only profile with explicit confirmation updates o writeSettings(); mkdirSync(fixture.configHome, { recursive: true }); writeFileSync(fixture.globalPath, `${JSON.stringify({ worker: { model: "openai/alpha" } }, null, 2)}\n`); + const subagentsPath = join(fixture.root, ".pi", "subagents.json"); + writeFileSync(subagentsPath, `${JSON.stringify({ model_profiles: { worker: { model: "openai/beta", effort: "high" } } }, null, 2)}\n`); writeStore({ orchOnly: { orchestrator: { model: "nan/glm5.3", thinking: "high" } }, team: { worker: { model: "openai/alpha" } }, @@ -2219,6 +2257,14 @@ test("applying an orchestrator-only profile with explicit confirmation updates o assert.equal(settings.defaultProvider, "nan"); assert.equal(settings.defaultModel, "glm5.3"); assert.equal(settings.defaultThinkingLevel, "high"); + + const subagents = JSON.parse(readFileSync(subagentsPath, "utf8")); + assert.deepEqual(subagents, {}, "confirmed orchestrator-only apply clears the seeded omitted worker route (shared clearing semantics drop the empty key)"); + assert.equal("orchestrator" in subagents, false, "the orchestrator key never materializes as an agent route"); + assert.deepEqual(fixture.liveSwitches, [ + { kind: "model", provider: "nan", id: "glm5.3" }, + { kind: "thinking", level: "high" }, + ], "the live session switches to the profile's orchestrator"); }); test("applying a profile replaces materialized routing for agents the profile omits", async (t) => { @@ -2896,6 +2942,7 @@ test("applying a profile in a pinned repository re-pins the clone and writes no assert.equal(readFileSync(fixture.globalPath, "utf8"), modelsBefore, "no global routing was written"); assert.equal(readFileSync(settingsPath, "utf8"), settingsBefore, "no orchestrator was written"); assert.equal(existsSync(join(fixture.root, ".pi", "subagents.json")), false, "no materialized routing was written"); + assert.equal(fixture.confirmCalls.length, 0, "a repo-pinned apply never asks for confirmation"); assert.match(fixture.notifications.at(-1)?.message ?? "", /repo-scoped/); }); From 7f1699f2a8d371621f13d0fe217984ab57e1aa26 Mon Sep 17 00:00:00 2001 From: Juan Barbat Date: Fri, 2 Oct 2026 11:14:04 -0300 Subject: [PATCH 4/6] fix(profiles): confirm every global profile apply with the routing diff (#1349) Address maintainer feedback on PR #1384: applying a populated profile globally (Ctrl+S then enter) replaced every materialized agent route with no confirmation, the exact destructive sequence reported in #1349. 1. In extensions/gentle-ai.ts runProfilesPanelAction case "apply", read the current global routing from the shared authority and compute a sorted replaced/cleared/added diff against the profile snapshot, excluding the orchestrator key. 2. Prompt for confirmation on every global apply via ctx.ui.confirm. A populated profile dialog names the concrete per-agent changes; empty and orchestrator-only profiles keep their specialized dialogs from efc7b52a. 3. Decline aborts before any store claim, write, settings change, or live-session switch, preserving all four surfaces byte-identically. 4. Add populated-apply regressions (decline preserves everything; confirm applies the diff) and flip the nonempty-apply assertion to expect the dialog. Repo-pinned applies stay silent. Verified: focused profile suite 37/37, full tests/gentle-ai.test.ts 96/96, typecheck 187 baseline diagnostics with no regressions, git diff --check. --- extensions/gentle-ai.ts | 42 +++++++++ odd/tasks/pr-1384-populated-apply-confirm.md | 53 +++++++++++ tests/gentle-ai.test.ts | 96 +++++++++++++++++++- 3 files changed, 190 insertions(+), 1 deletion(-) create mode 100644 odd/tasks/pr-1384-populated-apply-confirm.md diff --git a/extensions/gentle-ai.ts b/extensions/gentle-ai.ts index 6ecaa4c22..d3a9e2667 100644 --- a/extensions/gentle-ai.ts +++ b/extensions/gentle-ai.ts @@ -4149,6 +4149,36 @@ async function runProfilesPanelAction( const normalized = normalizeModelConfig(file.profiles[result.name]) ?? {}; const orchestratorEntry = readProfileOrchestrator(normalized); const hasAgentRoutes = Object.keys(normalized).some((name) => !isProfileOrchestratorKey(name)); + // Every global apply confirms before anything is written: applying replaces + // the whole routing map in models.json and clears materialized routes the + // profile omits, so the dialog must name the concrete diff. The current + // routing is read from the same authority every other consumer uses; an + // unreadable config is tolerated as empty, exactly like readModelConfigAsync. + const savedRouting = await readModelRoutingAuthorityAsync( + modelConfigPath(ctx.cwd), + legacyProjectModelConfigPath(ctx.cwd), + ); + const currentRouting = savedRouting.status === "valid" ? savedRouting.config : {}; + const agentNames = [ + ...new Set([ + ...Object.keys(currentRouting).filter((name) => !isProfileOrchestratorKey(name)), + ...Object.keys(normalized).filter((name) => !isProfileOrchestratorKey(name)), + ]), + ].sort(); + const replacedRoutes: string[] = []; + const clearedRoutes: string[] = []; + const addedRoutes: string[] = []; + for (const name of agentNames) { + const from = currentRouting[name]; + const to = normalized[name]; + if (to === undefined) { + clearedRoutes.push(`${name}: ${formatOrchestratorSelection(from)} → inherit (cleared)`); + } else if (from === undefined) { + addedRoutes.push(`${name}: ${formatOrchestratorSelection(to)} (added)`); + } else if (from.model !== to.model || from.thinking !== to.thinking) { + replacedRoutes.push(`${name}: ${formatOrchestratorSelection(from)} → ${formatOrchestratorSelection(to)}`); + } + } if (!hasAgentRoutes) { // A genuinely empty profile and an orchestrator-only profile both wipe // every materialized agent route, but they read very differently to the @@ -4165,6 +4195,18 @@ async function runProfilesPanelAction( ]; const approved = await ctx.ui.confirm(confirmTitle, confirmMessage); if (!approved) return file; + } else { + // A populated profile keeps the same abort semantics as the empty and + // orchestrator-only dialogs: declining leaves every surface untouched. + const changes = [...replacedRoutes, ...clearedRoutes, ...addedRoutes]; + const changeSummary = changes.length > 0 + ? changes.join("; ") + : "its agent routes already match the current global routing"; + const approved = await ctx.ui.confirm( + `Apply profile "${result.name}"?`, + `Profile "${result.name}" has agent routing entries. Applying it will replace global routing in ${sanitizeTerminalText(modelConfigPath(ctx.cwd))} with this profile's routes: ${changeSummary}. Continue?`, + ); + if (!approved) return file; } // Applying spans three files — the store, models.json, and Pi's global // settings.json — and there is no cross-file rename, so order the writes to diff --git a/odd/tasks/pr-1384-populated-apply-confirm.md b/odd/tasks/pr-1384-populated-apply-confirm.md new file mode 100644 index 000000000..8a8ef6768 --- /dev/null +++ b/odd/tasks/pr-1384-populated-apply-confirm.md @@ -0,0 +1,53 @@ +# PR #1384 Populated-Apply Confirmation + +## Objective + +Close the maintainer-flagged gap on PR #1384: applying a *populated* profile +globally still replaces every materialized route without confirmation +(`Ctrl+S` → `enter` sequence from #1349). Global applies now always confirm, +naming what changes before anything is written. + +## Decisions (operator, 2026-09-30) + +- Behavior: confirm **every** global profile apply. Populated profiles get a + dialog naming replaced routes, cleared agents, and added agents. The + empty / orchestrator-only dialogs from commit `efc7b52a` stay specialized. +- Repo-pinned applies stay silent (repo-scoped, no global state touched). +- Delivery: local only. No push, no PR comment unless the operator later + authorizes it. + +## Scope + +- `extensions/gentle-ai.ts`: extend the `case "apply"` guard — read the + current global routing via `readModelRoutingAuthorityAsync` before the + prompt, compute a diff (replaced / cleared / added agent routes), prompt on + every global apply, abort on decline before any write or live switch. +- `tests/gentle-ai.test.ts`: flip the "nonempty global apply never asks for + confirmation" assertion; add populated-apply regressions (decline preserves + all four surfaces byte-identically; confirm applies and reports). + +## Out of scope + +- #1557 key rebinding (`a`) reconciliation — follow-up when that branch lands. +- Snapshot/undo of successful applies (jonathanludena's general case). +- `odd/tasks/pr-1384-review-fixes.md` cleanup — operator decision at delivery. + +## Tasks + +- [x] T1 — RED: populated-apply regression (prompt shown, decline preserves + models.json/subagents.json/store/settings byte-identically and no live + switch). Observed: `confirmCalls.length 0 !== 1` on both new tests and on + the flipped orchestrator-persistence assertion. +- [x] T2 — Implement the universal confirmation with route diff. +- [x] T3 — GREEN: focused profile tests (37 pass), full + `tests/gentle-ai.test.ts` (96 pass), typecheck (187 diagnostics, baseline + unchanged), `git diff --check` clean. +- [ ] T4 — Work-unit commit (local only) and delivery report. + +## Constraints + +- Strict TDD: observe RED before changing production code. +- Technical artifacts in English; dialog copy stays consistent with the + existing `efc7b52a` style. +- Allowed edit surfaces: `extensions/gentle-ai.ts`, `tests/gentle-ai.test.ts` + (plus this task doc). diff --git a/tests/gentle-ai.test.ts b/tests/gentle-ai.test.ts index 0263da8ff..ab735aaa7 100644 --- a/tests/gentle-ai.test.ts +++ b/tests/gentle-ai.test.ts @@ -2024,6 +2024,9 @@ test("applying a profile persists its orchestrator and never leaks the key into worker: { model: "openai/alpha" }, }, }); + // Every global apply now confirms, so this populated apply approves the + // dialog explicitly before running. + fixture.onConfirm(async () => true); applyOnce(fixture); await fixture.run("gentle:profiles"); @@ -2049,7 +2052,8 @@ test("applying a profile persists its orchestrator and never leaks the key into { kind: "model", provider: "nan", id: "glm5.3" }, { kind: "thinking", level: "max" }, ], "the live session switches to the profile's orchestrator"); - assert.equal(fixture.confirmCalls.length, 0, "a nonempty global apply never asks for confirmation"); + assert.equal(fixture.confirmCalls.length, 1, "every global apply asks for confirmation"); + assert.match(fixture.confirmCalls[0]?.[0] ?? "", /Apply profile/); }); test("applying a profile whose orchestrator model is unknown to the registry persists the default and says the session did not switch", async (t) => { @@ -2267,6 +2271,96 @@ test("applying an orchestrator-only profile with explicit confirmation updates o ], "the live session switches to the profile's orchestrator"); }); +test("applying a populated profile asks for confirmation naming the diff and aborts when declined", async (t) => { + const { fixture, storePath, writeStore, writeSettings, settingsPath } = profilesStoreFixture(t); + writeSettings(); + mkdirSync(fixture.configHome, { recursive: true }); + // Current global routing: "worker" will be replaced, "helper" will be + // cleared back to inherit, and "tracer" only exists in the profile. + writeFileSync(fixture.globalPath, `${JSON.stringify({ worker: { model: "openai/alpha", thinking: "high" }, helper: { model: "openai/beta" } }, null, 2)}\n`); + const subagentsPath = join(fixture.root, ".pi", "subagents.json"); + writeFileSync(subagentsPath, `${JSON.stringify({ model_profiles: { worker: { model: "openai/alpha", effort: "high" }, helper: { model: "openai/beta" } } }, null, 2)}\n`); + const helperPath = join(fixture.root, ".pi", "agents", "helper.md"); + writeMarkdown(helperPath, "---\nname: helper\ndescription: Helper\nmodel: openai/beta\n---\nbody\n"); + writeStore({ + team: { + orchestrator: { model: "nan/glm5.3", thinking: "max" }, + worker: { model: "openai/beta", thinking: "max" }, + tracer: { model: "openai/alpha" }, + }, + }); + const before = { + models: readFileSync(fixture.globalPath, "utf8"), + subagents: readFileSync(subagentsPath, "utf8"), + store: readFileSync(storePath, "utf8"), + settings: readFileSync(settingsPath, "utf8"), + }; + + fixture.onConfirm(async () => false); + + applyOnce(fixture); + await fixture.run("gentle:profiles"); + + assert.equal(fixture.confirmCalls.length, 1, "a populated global apply must ask for confirmation"); + const [title, message] = fixture.confirmCalls[0]; + assert.equal(title, 'Apply profile "team"?'); + assert.match(message, /replace global routing/, "the dialog must name the file whose routing is replaced"); + assert.match(message, /models\.json/); + assert.match(message, /worker: openai\/alpha · high → openai\/beta · max/, "the replaced route must name both the old and the new route"); + assert.match(message, /helper: openai\/beta → inherit/, "the cleared agent must be named with its return to inherit"); + assert.match(message, /tracer: openai\/alpha \(added\)/, "the added agent must be named"); + + assert.equal(readFileSync(fixture.globalPath, "utf8"), before.models, "declined apply must preserve models.json byte-identically"); + assert.equal(readFileSync(subagentsPath, "utf8"), before.subagents, "declined apply must preserve subagents.json byte-identically"); + assert.equal(readFileSync(storePath, "utf8"), before.store, "declined apply must preserve the profiles store byte-identically"); + assert.equal(readFileSync(settingsPath, "utf8"), before.settings, "declined apply must preserve settings.json byte-identically"); + assert.deepEqual(fixture.liveSwitches, [], "declined apply must not switch the live session"); +}); + +test("applying a populated profile with explicit confirmation applies the routing diff", async (t) => { + const { fixture, storePath, writeStore, writeSettings } = profilesStoreFixture(t); + writeSettings(); + mkdirSync(fixture.configHome, { recursive: true }); + writeFileSync(fixture.globalPath, `${JSON.stringify({ worker: { model: "openai/alpha", thinking: "high" }, helper: { model: "openai/beta" } }, null, 2)}\n`); + const subagentsPath = join(fixture.root, ".pi", "subagents.json"); + writeFileSync(subagentsPath, `${JSON.stringify({ model_profiles: { worker: { model: "openai/alpha", effort: "high" }, helper: { model: "openai/beta" } } }, null, 2)}\n`); + const helperPath = join(fixture.root, ".pi", "agents", "helper.md"); + writeMarkdown(helperPath, "---\nname: helper\ndescription: Helper\nmodel: openai/beta\n---\nbody\n"); + const tracerPath = join(fixture.root, ".pi", "agents", "tracer.md"); + writeMarkdown(tracerPath, "---\nname: tracer\ndescription: Tracer\n---\nbody\n"); + writeStore({ + team: { + orchestrator: { model: "nan/glm5.3", thinking: "max" }, + worker: { model: "openai/beta", thinking: "max" }, + tracer: { model: "openai/alpha" }, + }, + }); + + fixture.onConfirm(async () => true); + + applyOnce(fixture); + await fixture.run("gentle:profiles"); + + assert.equal(fixture.confirmCalls.length, 1, "a populated global apply must ask for confirmation"); + const models = JSON.parse(readFileSync(fixture.globalPath, "utf8")); + assert.deepEqual(models, { + orchestrator: { model: "nan/glm5.3", thinking: "max" }, + worker: { model: "openai/beta", thinking: "max" }, + tracer: { model: "openai/alpha" }, + }, "confirmed apply replaces global routing with the profile"); + const subagents = JSON.parse(readFileSync(subagentsPath, "utf8")); + assert.equal("helper" in subagents.model_profiles, false, "the cleared agent loses its materialized route"); + assert.equal(subagents.model_profiles.worker?.model, "openai/beta", "the replaced route is materialized"); + assert.equal(subagents.model_profiles.tracer?.model, "openai/alpha", "the added agent is materialized"); + assert.doesNotMatch(readFileSync(helperPath, "utf8"), /^model:/m, "the cleared agent's frontmatter route is dropped"); + const store = JSON.parse(readFileSync(storePath, "utf8")); + assert.equal(store.active, "team", "active profile must be set to team when confirmed"); + assert.deepEqual(fixture.liveSwitches, [ + { kind: "model", provider: "nan", id: "glm5.3" }, + { kind: "thinking", level: "max" }, + ], "the live session switches to the profile's orchestrator"); +}); + test("applying a profile replaces materialized routing for agents the profile omits", async (t) => { const { fixture, writeStore, writeSettings } = profilesStoreFixture(t); writeSettings(); From e89c271eae6dddebaa58808127a850d9ef7700f4 Mon Sep 17 00:00:00 2001 From: Juan Barbat Date: Fri, 2 Oct 2026 12:04:24 -0300 Subject: [PATCH 5/6] fix(profiles): disclose unreadable routing authority in populated apply dialog (#1349) Close the convergent QA finding (glm5.3 adversarial-tester, glm5.2 exploratory-tester): when readModelRoutingAuthorityAsync does not return a valid status, the populated-apply dialog computed its diff against an empty map, presented existing routes as merely "(added)", and never mentioned that routes may be replaced or cleared back to inherit. 1. In extensions/gentle-ai.ts runProfilesPanelAction case "apply", when the routing authority is not valid at prompt time the populated dialog uses an alternative message disclosing the unreadable global routing and the replace/clear-to-inherit effect instead of the per-agent diff. 2. The valid-authority message, guard structure, decline semantics, empty and orchestrator-only dialogs, and repo-pinned silence are unchanged. 3. Add regressions for decline (disclosure, no "(added)", four surfaces byte-identical, no live switch) and confirm (apply proceeds unchanged). Verified: focused profile suite 39/39, full tests/gentle-ai.test.ts 98/98, typecheck 187 baseline diagnostics with no regressions, git diff --check. --- extensions/gentle-ai.ts | 24 ++++--- odd/tasks/pr-1384-populated-apply-confirm.md | 17 +++++ tests/gentle-ai.test.ts | 71 ++++++++++++++++++++ 3 files changed, 104 insertions(+), 8 deletions(-) diff --git a/extensions/gentle-ai.ts b/extensions/gentle-ai.ts index d3a9e2667..47385c865 100644 --- a/extensions/gentle-ai.ts +++ b/extensions/gentle-ai.ts @@ -4198,14 +4198,22 @@ async function runProfilesPanelAction( } else { // A populated profile keeps the same abort semantics as the empty and // orchestrator-only dialogs: declining leaves every surface untouched. - const changes = [...replacedRoutes, ...clearedRoutes, ...addedRoutes]; - const changeSummary = changes.length > 0 - ? changes.join("; ") - : "its agent routes already match the current global routing"; - const approved = await ctx.ui.confirm( - `Apply profile "${result.name}"?`, - `Profile "${result.name}" has agent routing entries. Applying it will replace global routing in ${sanitizeTerminalText(modelConfigPath(ctx.cwd))} with this profile's routes: ${changeSummary}. Continue?`, - ); + // When the routing authority is unreadable the diff above was computed + // against an empty map, so the dialog must disclose the unreadable + // routing and the replace/clear-to-inherit effect instead of presenting + // existing routes as merely "(added)" (the #1349 wipe-bug class). + let confirmMessage: string; + if (savedRouting.status !== "valid") { + const modelsPath = sanitizeTerminalText(modelConfigPath(ctx.cwd)); + confirmMessage = `Profile "${result.name}" has agent routing entries, but the current global routing in ${modelsPath} could not be read, so existing routes are not listed. Applying it will replace global routing in ${modelsPath} with this profile's routes, so every existing agent route may be replaced or cleared back to inherit. Continue?`; + } else { + const changes = [...replacedRoutes, ...clearedRoutes, ...addedRoutes]; + const changeSummary = changes.length > 0 + ? changes.join("; ") + : "its agent routes already match the current global routing"; + confirmMessage = `Profile "${result.name}" has agent routing entries. Applying it will replace global routing in ${sanitizeTerminalText(modelConfigPath(ctx.cwd))} with this profile's routes: ${changeSummary}. Continue?`; + } + const approved = await ctx.ui.confirm(`Apply profile "${result.name}"?`, confirmMessage); if (!approved) return file; } // Applying spans three files — the store, models.json, and Pi's global diff --git a/odd/tasks/pr-1384-populated-apply-confirm.md b/odd/tasks/pr-1384-populated-apply-confirm.md index 8a8ef6768..cc13594e8 100644 --- a/odd/tasks/pr-1384-populated-apply-confirm.md +++ b/odd/tasks/pr-1384-populated-apply-confirm.md @@ -44,6 +44,23 @@ naming what changes before anything is written. unchanged), `git diff --check` clean. - [ ] T4 — Work-unit commit (local only) and delivery report. +## QA R4 closure (2026-09-30) + +- Finding: with an invalid routing authority the apply guard fell back to + `currentRouting = {}`, so the populated dialog labeled every profile route + `(added)` and never disclosed the replace/clear-to-inherit effect. +- Fix: when `readModelRoutingAuthorityAsync` is not `valid` at prompt time, the + populated dialog uses an alternative message disclosing the unreadable + current routing and that applying replaces global routing so every existing + agent route may be replaced or cleared back to inherit. Guard structure, + decline semantics, empty/orchestrator-only dialogs, and repo-pinned silence + unchanged. +- T5 — RED observed: `AssertionError ... the dialog must disclose that the + current global routing is unreadable` with actual message ending + `worker: openai/alpha (added). Continue?` on both new tests. +- T6 — GREEN: focused profile tests 39 pass, full `tests/gentle-ai.test.ts` + 98 pass, typecheck 187 diagnostics (baseline), `git diff --check` clean. + ## Constraints - Strict TDD: observe RED before changing production code. diff --git a/tests/gentle-ai.test.ts b/tests/gentle-ai.test.ts index ab735aaa7..0919fed8d 100644 --- a/tests/gentle-ai.test.ts +++ b/tests/gentle-ai.test.ts @@ -2317,6 +2317,77 @@ test("applying a populated profile asks for confirmation naming the diff and abo assert.deepEqual(fixture.liveSwitches, [], "declined apply must not switch the live session"); }); +test("applying a populated profile with an unreadable routing authority discloses the replace effect instead of added labels", async (t) => { + const { fixture, storePath, writeStore, writeSettings, settingsPath } = profilesStoreFixture(t); + writeSettings(); + mkdirSync(fixture.configHome, { recursive: true }); + // Malformed JSON makes readModelRoutingAuthorityAsync report the global + // models.json as invalid: real routes exist on disk but cannot be diffed. + writeFileSync(fixture.globalPath, "{ not json\n"); + const subagentsPath = join(fixture.root, ".pi", "subagents.json"); + writeFileSync(subagentsPath, `${JSON.stringify({ model_profiles: { worker: { model: "openai/alpha", effort: "high" } } }, null, 2)}\n`); + writeStore({ + team: { + orchestrator: { model: "nan/glm5.3", thinking: "max" }, + worker: { model: "openai/alpha" }, + }, + }); + const before = { + models: readFileSync(fixture.globalPath, "utf8"), + subagents: readFileSync(subagentsPath, "utf8"), + store: readFileSync(storePath, "utf8"), + settings: readFileSync(settingsPath, "utf8"), + }; + + fixture.onConfirm(async () => false); + + applyOnce(fixture); + await fixture.run("gentle:profiles"); + + assert.equal(fixture.confirmCalls.length, 1, "a populated global apply must ask for confirmation"); + const [title, message] = fixture.confirmCalls[0]; + assert.equal(title, 'Apply profile "team"?'); + assert.match(message, /could not be read/, "the dialog must disclose that the current global routing is unreadable"); + assert.match(message, /replace global routing/, "the dialog must name the global replacement"); + assert.match(message, /inherit/, "the dialog must disclose the clear-back-to-inherit effect"); + assert.doesNotMatch(message, /\(added\)/, "unread current routes must not be presented as merely added"); + + assert.equal(readFileSync(fixture.globalPath, "utf8"), before.models, "declined apply must preserve models.json byte-identically"); + assert.equal(readFileSync(subagentsPath, "utf8"), before.subagents, "declined apply must preserve subagents.json byte-identically"); + assert.equal(readFileSync(storePath, "utf8"), before.store, "declined apply must preserve the profiles store byte-identically"); + assert.equal(readFileSync(settingsPath, "utf8"), before.settings, "declined apply must preserve settings.json byte-identically"); + assert.deepEqual(fixture.liveSwitches, [], "declined apply must not switch the live session"); +}); + +test("applying a populated profile with an unreadable routing authority still applies when confirmed", async (t) => { + const { fixture, storePath, writeStore, writeSettings } = profilesStoreFixture(t); + writeSettings(); + mkdirSync(fixture.configHome, { recursive: true }); + writeFileSync(fixture.globalPath, "{ not json\n"); + writeStore({ + team: { + orchestrator: { model: "nan/glm5.3", thinking: "max" }, + worker: { model: "openai/alpha" }, + }, + }); + + fixture.onConfirm(async () => true); + + applyOnce(fixture); + await fixture.run("gentle:profiles"); + + assert.equal(fixture.confirmCalls.length, 1, "a populated global apply must ask for confirmation"); + const [, message] = fixture.confirmCalls[0]; + assert.match(message, /could not be read/, "the dialog must disclose that the current global routing is unreadable"); + const models = JSON.parse(readFileSync(fixture.globalPath, "utf8")); + assert.deepEqual(models, { + orchestrator: { model: "nan/glm5.3", thinking: "max" }, + worker: { model: "openai/alpha" }, + }, "confirmed apply proceeds as before despite the unreadable authority"); + const store = JSON.parse(readFileSync(storePath, "utf8")); + assert.equal(store.active, "team", "active profile must be set to team when confirmed"); +}); + test("applying a populated profile with explicit confirmation applies the routing diff", async (t) => { const { fixture, storePath, writeStore, writeSettings } = profilesStoreFixture(t); writeSettings(); From 777ac6d3ce4054eb659d435ecf72248dca72d568 Mon Sep 17 00:00:00 2001 From: Juan Barbat Date: Fri, 2 Oct 2026 15:18:50 -0300 Subject: [PATCH 6/6] fix(profiles): name materialized routes and orchestrator effects in populated apply dialog (#1349) Close the two unresolved CodeRabbit Major findings on PR #1384. 1. The populated-apply diff now runs against the effective current routing: saved global routing merged with the materialized routes (agent frontmatter, subagents.json model_profiles) of every discoverable agent the saved routing is silent about, via a new readGlobalEffectiveModelConfigFromAsync helper reused by readEffectiveModelConfigAsync. Approval can no longer clear a materialized-only route the dialog never named, or claim routes already match when a clear would happen. 2. When a populated profile carries an orchestrator entry, both the readable and unreadable dialog variants disclose that approval sets the orchestrator in settings.json and attempts a live-session switch, with the same wording as the orchestrator-only dialog. 3. The unreadable-authority disclosure, empty and orchestrator-only dialogs, decline semantics, and repo-pinned silence are unchanged. Verified: RED observed for all four new tests; focused profile suite 43/43, full tests/gentle-ai.test.ts 102/102, typecheck 187 baseline diagnostics with no regressions, git diff --check. --- extensions/gentle-ai.ts | 37 +++++- odd/tasks/pr-1384-populated-apply-confirm.md | 24 ++++ tests/gentle-ai.test.ts | 121 +++++++++++++++++++ 3 files changed, 178 insertions(+), 4 deletions(-) diff --git a/extensions/gentle-ai.ts b/extensions/gentle-ai.ts index 47385c865..0aaf1b288 100644 --- a/extensions/gentle-ai.ts +++ b/extensions/gentle-ai.ts @@ -2122,7 +2122,21 @@ function readGlobalEffectiveModelConfig(cwd: string): AgentModelConfig { async function readEffectiveModelConfigAsync(cwd: string): Promise { const pinned = pinnedEffectiveModelConfig(cwd); if (pinned) return pinned; - const effective = cloneModelConfig(await readModelConfigAsync(cwd)); + return readGlobalEffectiveModelConfigFromAsync(cwd, await readModelConfigAsync(cwd)); +} + +/** + * The saved global routing merged with the materialized stores of every + * discoverable agent it is silent about — the same effective view + * `readEffectiveModelConfigAsync` builds, but starting from an already-read + * saved routing so callers that must distinguish an unreadable authority can + * keep that distinction while still seeing materialized routes. + */ +async function readGlobalEffectiveModelConfigFromAsync( + cwd: string, + base: AgentModelConfig, +): Promise { + const effective = cloneModelConfig(base); const profilesByPath = new Map>(); for (const agent of await listDiscoverableAgentsAsync(cwd)) { if (isProviderReviewRole(agent.name) || agent.name in effective) continue; @@ -4158,7 +4172,14 @@ async function runProfilesPanelAction( modelConfigPath(ctx.cwd), legacyProjectModelConfigPath(ctx.cwd), ); - const currentRouting = savedRouting.status === "valid" ? savedRouting.config : {}; + // The apply pads omitted discoverable agents with clear entries, so the + // diff must run against the effective current routing: the saved global + // routing plus the materialized routes (frontmatter, subagents.json) of + // agents the saved routing is silent about. models.json alone would hide + // materialized-only routes the approval actually clears. + const currentRouting = savedRouting.status === "valid" + ? await readGlobalEffectiveModelConfigFromAsync(ctx.cwd, savedRouting.config) + : {}; const agentNames = [ ...new Set([ ...Object.keys(currentRouting).filter((name) => !isProfileOrchestratorKey(name)), @@ -4202,16 +4223,24 @@ async function runProfilesPanelAction( // against an empty map, so the dialog must disclose the unreadable // routing and the replace/clear-to-inherit effect instead of presenting // existing routes as merely "(added)" (the #1349 wipe-bug class). + // When the profile carries an orchestrator entry, approval also writes + // settings.json and tries to move the live session, so both variants + // must disclose those effects in the same words the orchestrator-only + // dialog uses. An unconditional "will switch" is never claimed: the + // registry/auth can refuse the live move. + const orchestratorEffects = orchestratorEntry !== undefined + ? ", set the configured orchestrator entry in settings.json, and attempt to switch this session to that orchestrator model" + : ""; let confirmMessage: string; if (savedRouting.status !== "valid") { const modelsPath = sanitizeTerminalText(modelConfigPath(ctx.cwd)); - confirmMessage = `Profile "${result.name}" has agent routing entries, but the current global routing in ${modelsPath} could not be read, so existing routes are not listed. Applying it will replace global routing in ${modelsPath} with this profile's routes, so every existing agent route may be replaced or cleared back to inherit. Continue?`; + confirmMessage = `Profile "${result.name}" has agent routing entries, but the current global routing in ${modelsPath} could not be read, so existing routes are not listed. Applying it will replace global routing in ${modelsPath} with this profile's routes, so every existing agent route may be replaced or cleared back to inherit${orchestratorEffects}. Continue?`; } else { const changes = [...replacedRoutes, ...clearedRoutes, ...addedRoutes]; const changeSummary = changes.length > 0 ? changes.join("; ") : "its agent routes already match the current global routing"; - confirmMessage = `Profile "${result.name}" has agent routing entries. Applying it will replace global routing in ${sanitizeTerminalText(modelConfigPath(ctx.cwd))} with this profile's routes: ${changeSummary}. Continue?`; + confirmMessage = `Profile "${result.name}" has agent routing entries. Applying it will replace global routing in ${sanitizeTerminalText(modelConfigPath(ctx.cwd))} with this profile's routes: ${changeSummary}${orchestratorEffects}. Continue?`; } const approved = await ctx.ui.confirm(`Apply profile "${result.name}"?`, confirmMessage); if (!approved) return file; diff --git a/odd/tasks/pr-1384-populated-apply-confirm.md b/odd/tasks/pr-1384-populated-apply-confirm.md index cc13594e8..d79d11b20 100644 --- a/odd/tasks/pr-1384-populated-apply-confirm.md +++ b/odd/tasks/pr-1384-populated-apply-confirm.md @@ -44,6 +44,30 @@ naming what changes before anything is written. unchanged), `git diff --check` clean. - [ ] T4 — Work-unit commit (local only) and delivery report. +## CodeRabbit closure round (2026-10-02, both Major threads) + +- Finding 1 (materialized-only routes): the populated dialog now diffs against + the effective current routing — saved global routing merged with materialized + routes via the new `readGlobalEffectiveModelConfigFromAsync` helper (reuses + `listDiscoverableAgentsAsync` / `readMaterializedRoutingEntryAsync`, same as + `readEffectiveModelConfigAsync`, which now delegates to it). `models.json` + alone hid routes the approval actually clears. Unreadable-authority + disclosure branch unchanged and still authoritative. +- Finding 2 (orchestrator effects): when a populated profile has an + `orchestrator` entry, both the readable and unreadable populated messages + append "set the configured orchestrator entry in settings.json, and attempt + to switch this session to that orchestrator model" (same wording as the + orchestrator-only dialog; never an unconditional switch promise). No + orchestrator entry → messages unchanged. +- T7 — RED observed: 4 new tests fail — dialog claimed "its agent routes + already match the current global routing" while approval would clear + helper's materialized route (2 tests); readable and unreadable populated + messages lacked the settings.json / live-switch disclosure (2 tests). +- T8 — GREEN: focused profile tests 43 pass, full `tests/gentle-ai.test.ts` + 102 pass, typecheck 187 diagnostics (baseline unchanged), `git diff --check` + clean. Confirm-path regression added (materialized-only route cleared on + approval); existing message assertions untouched. + ## QA R4 closure (2026-09-30) - Finding: with an invalid routing authority the apply guard fell back to diff --git a/tests/gentle-ai.test.ts b/tests/gentle-ai.test.ts index 0919fed8d..91300e010 100644 --- a/tests/gentle-ai.test.ts +++ b/tests/gentle-ai.test.ts @@ -2317,6 +2317,127 @@ test("applying a populated profile asks for confirmation naming the diff and abo assert.deepEqual(fixture.liveSwitches, [], "declined apply must not switch the live session"); }); +test("applying a populated profile names materialized-only routes it would clear before asking", async (t) => { + const { fixture, storePath, writeStore, writeSettings, settingsPath } = profilesStoreFixture(t); + writeSettings(); + mkdirSync(fixture.configHome, { recursive: true }); + // The saved global routing matches the profile exactly: "helper" is routed + // only where the runtime actually resolves it (agent frontmatter and + // subagents.json model profiles), and the apply clears it because the profile + // omits it. models.json alone never speaks for "helper". + writeFileSync(fixture.globalPath, `${JSON.stringify({ worker: { model: "openai/alpha" } }, null, 2)}\n`); + const subagentsPath = join(fixture.root, ".pi", "subagents.json"); + writeFileSync(subagentsPath, `${JSON.stringify({ model_profiles: { helper: { model: "openai/beta", effort: "high" } } }, null, 2)}\n`); + const helperPath = join(fixture.root, ".pi", "agents", "helper.md"); + writeMarkdown(helperPath, "---\nname: helper\ndescription: Helper\nmodel: openai/beta\n---\nbody\n"); + writeStore({ + team: { worker: { model: "openai/alpha" } }, + }); + const before = { + models: readFileSync(fixture.globalPath, "utf8"), + subagents: readFileSync(subagentsPath, "utf8"), + store: readFileSync(storePath, "utf8"), + settings: readFileSync(settingsPath, "utf8"), + }; + + fixture.onConfirm(async () => false); + + applyOnce(fixture); + await fixture.run("gentle:profiles"); + + assert.equal(fixture.confirmCalls.length, 1, "a populated global apply must ask for confirmation"); + const [title, message] = fixture.confirmCalls[0]; + assert.equal(title, 'Apply profile "team"?'); + assert.match(message, /helper: openai\/beta · high → inherit/, "the materialized-only route the apply clears must be named"); + assert.doesNotMatch(message, /already match/, "routes matching the saved routing must not hide a cleared materialized-only route"); + + assert.equal(readFileSync(fixture.globalPath, "utf8"), before.models, "declined apply must preserve models.json byte-identically"); + assert.equal(readFileSync(subagentsPath, "utf8"), before.subagents, "declined apply must preserve subagents.json byte-identically"); + assert.equal(readFileSync(storePath, "utf8"), before.store, "declined apply must preserve the profiles store byte-identically"); + assert.equal(readFileSync(settingsPath, "utf8"), before.settings, "declined apply must preserve settings.json byte-identically"); + assert.deepEqual(fixture.liveSwitches, [], "declined apply must not switch the live session"); +}); + +test("applying a populated profile clears the materialized-only route it named when confirmed", async (t) => { + const { fixture, writeStore, writeSettings } = profilesStoreFixture(t); + writeSettings(); + mkdirSync(fixture.configHome, { recursive: true }); + writeFileSync(fixture.globalPath, `${JSON.stringify({ worker: { model: "openai/alpha" } }, null, 2)}\n`); + const subagentsPath = join(fixture.root, ".pi", "subagents.json"); + writeFileSync(subagentsPath, `${JSON.stringify({ model_profiles: { helper: { model: "openai/beta", effort: "high" } } }, null, 2)}\n`); + const helperPath = join(fixture.root, ".pi", "agents", "helper.md"); + writeMarkdown(helperPath, "---\nname: helper\ndescription: Helper\nmodel: openai/beta\n---\nbody\n"); + writeStore({ + team: { worker: { model: "openai/alpha" } }, + }); + + fixture.onConfirm(async () => true); + + applyOnce(fixture); + await fixture.run("gentle:profiles"); + + assert.equal(fixture.confirmCalls.length, 1, "a populated global apply must ask for confirmation"); + const [, message] = fixture.confirmCalls[0]; + assert.match(message, /helper: openai\/beta · high → inherit/, "the dialog must name the materialized-only route before it is cleared"); + const subagents = JSON.parse(readFileSync(subagentsPath, "utf8")); + assert.equal("helper" in subagents.model_profiles, false, "confirming the apply clears the materialized-only route the dialog named"); + assert.doesNotMatch(readFileSync(helperPath, "utf8"), /^model:/m, "the materialized-only frontmatter route is dropped"); +}); + +test("applying a populated profile with an orchestrator entry discloses the settings and live-session effects", async (t) => { + const { fixture, writeStore, writeSettings } = profilesStoreFixture(t); + writeSettings(); + mkdirSync(fixture.configHome, { recursive: true }); + writeFileSync(fixture.globalPath, `${JSON.stringify({ worker: { model: "openai/alpha" } }, null, 2)}\n`); + writeStore({ + team: { + orchestrator: { model: "nan/glm5.3", thinking: "max" }, + worker: { model: "openai/beta" }, + }, + }); + + fixture.onConfirm(async () => false); + + applyOnce(fixture); + await fixture.run("gentle:profiles"); + + assert.equal(fixture.confirmCalls.length, 1, "a populated global apply must ask for confirmation"); + const [, message] = fixture.confirmCalls[0]; + assert.match(message, /replace global routing/, "the readable variant must keep naming the routing diff"); + assert.match(message, /settings\.json/, "the dialog must disclose that the orchestrator entry is set in settings.json"); + assert.match(message, /attempt to switch this session/, "the dialog must disclose the attempted live-session switch"); + assert.doesNotMatch(message, /will switch this session/, "the dialog must not promise an unconditional live switch"); + assert.deepEqual(fixture.liveSwitches, [], "declined apply must not switch the live session"); +}); + +test("applying a populated profile with an orchestrator entry discloses the effects when the routing is unreadable", async (t) => { + const { fixture, writeStore, writeSettings } = profilesStoreFixture(t); + writeSettings(); + mkdirSync(fixture.configHome, { recursive: true }); + // Malformed models.json keeps the unreadable-authority disclosure branch in + // charge: the orchestrator effects must be added there, not swap it out. + writeFileSync(fixture.globalPath, "{ not json\n"); + writeStore({ + team: { + orchestrator: { model: "nan/glm5.3", thinking: "max" }, + worker: { model: "openai/alpha" }, + }, + }); + + fixture.onConfirm(async () => false); + + applyOnce(fixture); + await fixture.run("gentle:profiles"); + + assert.equal(fixture.confirmCalls.length, 1, "a populated global apply must ask for confirmation"); + const [, message] = fixture.confirmCalls[0]; + assert.match(message, /could not be read/, "the unreadable-authority disclosure must stay authoritative"); + assert.match(message, /settings\.json/, "the dialog must disclose that the orchestrator entry is set in settings.json"); + assert.match(message, /attempt to switch this session/, "the dialog must disclose the attempted live-session switch"); + assert.doesNotMatch(message, /will switch this session/, "the dialog must not promise an unconditional live switch"); + assert.deepEqual(fixture.liveSwitches, [], "declined apply must not switch the live session"); +}); + test("applying a populated profile with an unreadable routing authority discloses the replace effect instead of added labels", async (t) => { const { fixture, storePath, writeStore, writeSettings, settingsPath } = profilesStoreFixture(t); writeSettings();