From 8c6dc84732e29f65bb678b76cc9203be9202ed4f Mon Sep 17 00:00:00 2001 From: Artur Jonkisz Date: Sat, 20 Jun 2026 16:06:04 +0100 Subject: [PATCH 01/22] Reshuffle Roles model UI: inline list control + Model section, fix save hang Co-authored-by: bobbit-ai --- src/app/proposal-panels.ts | 6 +- src/app/role-manager-page.ts | 152 ++++++++++++++++++++++++++++++----- src/app/role-manager.css | 64 ++++++++++++++- src/app/settings-page.ts | 43 ++++++++++ 4 files changed, 242 insertions(+), 23 deletions(-) diff --git a/src/app/proposal-panels.ts b/src/app/proposal-panels.ts index dcaf15462..7e0bfdf2a 100644 --- a/src/app/proposal-panels.ts +++ b/src/app/proposal-panels.ts @@ -572,9 +572,9 @@ interface GoalFormConfig { onCustomizeRole?: () => void; onResetRole?: () => void; onRoleDraftChange?: (patch: Partial) => void; - onRoleEditorTabChange?: (tab: "prompt" | "tools" | "model") => void; + onRoleEditorTabChange?: (tab: "prompt" | "tools") => void; onRoleToggleToolGroup?: (group: string) => void; - roleEditTab?: "prompt" | "tools" | "model"; + roleEditTab?: "prompt" | "tools"; roleCollapsedGroups?: ReadonlySet; roleList?: RoleData[]; roleListLoading?: boolean; @@ -2796,7 +2796,7 @@ let _proposalInlineRoles: Record = {}; let _proposalSelectedRoleName: string | null = null; let _proposalCustomizingWorkflow = false; let _proposalCustomizingRole = false; -let _proposalRoleEditTab: "prompt" | "tools" | "model" = "prompt"; +let _proposalRoleEditTab: "prompt" | "tools" = "prompt"; let _proposalRoleCollapsedGroups = new Set(); let _proposalTabsInitializedFrom: string | null = null; // Role data caches for the modal, project-scoped: roles can be customised per diff --git a/src/app/role-manager-page.ts b/src/app/role-manager-page.ts index 1a9600ad1..a2b3f48a5 100644 --- a/src/app/role-manager-page.ts +++ b/src/app/role-manager-page.ts @@ -13,7 +13,7 @@ import { renderIdleBlobCanvas } from "../ui/bobbit-render.js"; import { state, renderApp } from "./state.js"; import { setHashRoute } from "./routing.js"; import { type ConfigOrigin, getConfigScope, setConfigScope, getConfigProjectId, renderOriginBadge, isInherited, renderConfigScopeRow, customizeItem, revertOverride, getCurrentProjectName } from "./config-scope.js"; -import { renderModelRow, formatModelPref } from "./settings-page.js"; +import { renderModelRow, formatModelPref, formatRoleDefaultModelLabel, ensureModelDefaultsLoaded } from "./settings-page.js"; // ============================================================================ // HELPERS @@ -63,11 +63,16 @@ let editAccessory = "none"; let editToolPolicies: Record = {}; let editModelOverride = ""; let editThinkingOverride = ""; -let editTab: "prompt" | "tools" | "model" = "prompt"; +let editTab: "prompt" | "tools" = "prompt"; let saving = false; let deleting = false; +// Inline list-row model auto-save: name of the role currently showing a +// transient "Saved" flash next to its label, plus the clear-timer handle. +let savedModelFlashRole: string | null = null; +let savedModelFlashTimer: ReturnType | null = null; + // Group policies loaded from server let groupPolicies: Record = {}; @@ -117,6 +122,9 @@ export async function loadRolePageData(): Promise { saving = false; deleting = false; renderApp(); + // Load default model prefs + model registry so inherited list rows can show + // a resolved " · default" label without requiring a Settings visit. + ensureModelDefaultsLoaded(); const [r, t, gp] = await Promise.all([fetchRolesScoped(), fetchTools(), fetchGroupPolicies()]); roles = r; availableTools = t; @@ -254,6 +262,11 @@ async function handleSave(): Promise { const updated = roles.find((r) => r.name === selectedRole!.name); if (updated) showEdit(updated); else showList(); + // Explicitly rerender: showEdit -> setHashRoute("role-edit", name) is a + // no-op when the hash already matches (saving from the role's own edit + // page), so no hashchange fires and the "Saving…" button would otherwise + // stay stuck. initEditState already reset saving=false in memory. + renderApp(); return; } } @@ -261,6 +274,35 @@ async function handleSave(): Promise { renderApp(); } +/** + * Persist an inline list-row model/thinking change for a role and flash "Saved". + * Uses the full role payload required by the PUT path. On failure, `updateRole` + * has already surfaced the error via `showConnectionError`; we just rerender. + */ +async function persistInlineModel(role: RoleData, model: string, thinkingLevel: string): Promise { + const ok = await updateRole(role.name, { + label: role.label, + promptTemplate: role.promptTemplate, + accessory: role.accessory, + toolPolicies: role.toolPolicies ?? {}, + model, + thinkingLevel, + }, getConfigProjectId() || undefined); + if (!ok) { + renderApp(); + return; + } + roles = await fetchRolesScoped(); + savedModelFlashRole = role.name; + renderApp(); + if (savedModelFlashTimer) clearTimeout(savedModelFlashTimer); + savedModelFlashTimer = setTimeout(() => { + savedModelFlashRole = null; + savedModelFlashTimer = null; + if (currentView === "list") renderApp(); + }, 1800); +} + async function handleDelete(): Promise { if (!selectedRole) return; const confirmed = await confirmAction( @@ -309,6 +351,10 @@ export interface RoleListOptions { scope?: RoleRendererScope; /** Optional empty-state action button shown when roles is empty. */ emptyAction?: TemplateResult; + /** When set, list rows render an inline model+thinking control with + * auto-save wired to these callbacks. Omitted by list-only callers + * (e.g. the goal-draft modal). */ + modelControl?: RoleRowModelControl; } export interface RoleEditorDraft { @@ -318,12 +364,12 @@ export interface RoleEditorDraft { toolPolicies: Record; model: string; thinkingLevel: string; - activeTab: "prompt" | "tools" | "model"; + activeTab: "prompt" | "tools"; } export interface RoleEditorCallbacks { onDraftChange: (patch: Partial) => void; - onTabChange: (tab: "prompt" | "tools" | "model") => void; + onTabChange: (tab: "prompt" | "tools") => void; onToggleToolGroup: (group: string) => void; } @@ -460,6 +506,20 @@ async function handleScopeChange(scope: string): Promise { renderApp(); } +/** + * Inline model + thinking control config for list rows. Only the main Roles + * page supplies this (enables auto-save); the goal-draft modal omits it so its + * embedded list stays list-only by default. + */ +export interface RoleRowModelControl { + /** Role name currently showing a transient "Saved" flash. */ + savedFlashRole?: string | null; + /** Persist a model choice ("" clears the override) for the row's role. */ + onModelChange: (role: RoleData, model: string) => void; + /** Persist a thinking-level change (override rows only) for the row's role. */ + onThinkingChange: (role: RoleData, thinkingLevel: string) => void; +} + interface RoleRowOptions { role: RoleData; index: number; @@ -468,11 +528,49 @@ interface RoleRowOptions { onSelect: (role: RoleData) => void; onEdit?: (role: RoleData) => void; onDelete?: (role: RoleData) => void; + modelControl?: RoleRowModelControl; +} + +/** + * Inline model/thinking control rendered in the middle of a list row. Reuses + * the shared `renderModelRow`. Inherited rows (no `role.model`) show the + * resolved default label and dim the thinking selector; override rows show the + * actual model with clear/Test from `renderModelRow`. + */ +function renderRoleRowModelControl(role: RoleData, control: RoleRowModelControl): TemplateResult { + const overridden = !!(role.model && role.model.length > 0); + const stateClass = overridden ? "role-row-model-control--override" : "role-row-model-control--inherited"; + const stopRow = (e: Event) => e.stopPropagation(); + return html` +
+ ${renderModelRow( + "", + "", + overridden ? (role.model ?? "") : "", + (v) => control.onModelChange(role, v), + overridden ? (role.thinkingLevel ?? "") : "", + (v) => { + // Inherited rows must never auto-save a thinking-only override + // while no model override exists. renderModelRow's reactive + // clamp also fires onThinkingChange via queueMicrotask — ignore + // it here so an inherited row stays inherited. + if (!overridden) return; + control.onThinkingChange(role, v); + }, + "", + { fallbackLabel: overridden ? "(use default)" : formatRoleDefaultModelLabel(role.name) }, + )} +
+ `; } /** Stateless row renderer; used by both the page list and the goal-draft modal. */ export function renderRoleListRow(opts: RoleRowOptions): TemplateResult { - const { role, index, selected, customized, onSelect, onEdit, onDelete } = opts; + const { role, index, selected, customized, onSelect, onEdit, onDelete, modelControl } = opts; const origin = (role as any).origin as ConfigOrigin | undefined; const overrides = (role as any).overrides as ConfigOrigin | undefined; const inherited = isInherited(origin); @@ -489,9 +587,10 @@ export function renderRoleListRow(opts: RoleRowOptions): TemplateResult { @keydown=${(e: KeyboardEvent) => { if (e.key === "Enter" || e.key === " ") { e.preventDefault(); onSelect(role); } }}> ${idleBlob(role.accessory ?? "none", 42, index, index)}
- ${role.label}${customized ? html` ` : nothing} + ${role.label}${customized ? html` ` : nothing}${modelControl?.savedFlashRole === role.name ? html` Saved` : nothing} ${role.name} ${renderOriginBadge(origin, overrides, (role as any).originPackName)}
+ ${modelControl ? renderRoleRowModelControl(role, modelControl) : nothing}
${onEdit ? html`
`; @@ -567,6 +667,18 @@ function renderListView(): TemplateResult { onEdit: (r) => showEdit(r), onDelete: (r) => handleDeleteFromList(r), emptyAction, + modelControl: { + savedFlashRole: savedModelFlashRole, + onModelChange: (role, model) => { + // Inherited -> override: preserve existing thinking if present. + // Clear -> fully revert to inherited/default display. + void persistInlineModel(role, model, model ? (role.thinkingLevel ?? "") : ""); + }, + onThinkingChange: (role, thinking) => { + // Only fired for override rows (guarded in the inline control). + void persistInlineModel(role, role.model ?? "", thinking); + }, + }, }); if (loading || roles.length === 0) return list; @@ -901,14 +1013,24 @@ export function renderRoleEditor(opts: RoleEditorOptions): TemplateResult { + +
+

Model

+ ${renderRoleModelTab({ + model: draft.model, + thinkingLevel: draft.thinkingLevel, + onModelChange: (v) => callbacks.onDraftChange({ model: v }), + onThinkingChange: (v) => callbacks.onDraftChange({ thinkingLevel: v }), + readOnly, + })} +
+
-
@@ -925,8 +1047,7 @@ export function renderRoleEditor(opts: RoleEditorOptions): TemplateResult { onAssistantPromptTabChange, onAssistantPromptChange, }) - : draft.activeTab === "tools" - ? renderRoleToolAccessTab({ + : renderRoleToolAccessTab({ toolPolicies: draft.toolPolicies, availableTools: tools, groupPolicies: gp, @@ -939,13 +1060,6 @@ export function renderRoleEditor(opts: RoleEditorOptions): TemplateResult { }, onToggleGroup: callbacks.onToggleToolGroup, readOnly, - }) - : renderRoleModelTab({ - model: draft.model, - thinkingLevel: draft.thinkingLevel, - onModelChange: (v) => callbacks.onDraftChange({ model: v }), - onThinkingChange: (v) => callbacks.onDraftChange({ thinkingLevel: v }), - readOnly, })} @@ -964,7 +1078,7 @@ export function renderRoleEditor(opts: RoleEditorOptions): TemplateResult { // because every `onDraftChange` call is a noop. // ============================================================================ let _inspectorRoleName: string | null = null; -let _inspectorActiveTab: "prompt" | "tools" | "model" = "prompt"; +let _inspectorActiveTab: "prompt" | "tools" = "prompt"; let _inspectorCollapsedGroups: Set = new Set(); /** diff --git a/src/app/role-manager.css b/src/app/role-manager.css index a72101620..9bc311e75 100644 --- a/src/app/role-manager.css +++ b/src/app/role-manager.css @@ -146,7 +146,7 @@ } .role-row-info { - flex: 1; + flex: 0 1 220px; min-width: 0; display: flex; flex-direction: column; @@ -201,6 +201,57 @@ background: color-mix(in oklch, var(--destructive) 10%, transparent); } +/* ---------------------------------------------------------------------------- + * Inline model + thinking control (list rows) + * Reuses the shared renderModelRow; placed in the gap between identity and + * the edit/delete actions. Only the main Roles page supplies it. + * -------------------------------------------------------------------------- */ +.role-row-model-control { + flex: 1 1 300px; + min-width: 180px; + max-width: 520px; +} + +/* Compact placement: hide the (empty) label column and hint paragraph that + * renderModelRow emits, so the control fits a list row. Scoped to list rows + * only — global Settings model rows are untouched. */ +.role-row-model-control [data-testid="model-row"] > div:first-child > span:first-child { + display: none; +} +.role-row-model-control [data-testid="model-row"] > p { + display: none; +} + +/* Inherited rows: dim the whole control and disable the thinking selector so a + * thinking-only change can't create an override (model picker stays active). */ +.role-row-model-control--inherited { + opacity: 0.7; +} +.role-row-model-control--inherited [data-testid="model-row"] { + color: var(--muted-foreground); +} +/* Disable just the thinking picker (last child inside the control box) so the + * model picker stays usable while inherited rows can't create a thinking-only + * override. */ +.role-row-model-control--inherited [data-testid="model-row"] > div:first-child > div:nth-child(2) > div:last-child { + pointer-events: none; + opacity: 0.6; +} + +/* Override rows: accent the control border with a light primary tint. */ +.role-row-model-control--override [data-testid="model-row"] > div:first-child > div:nth-child(2) { + border-color: color-mix(in oklch, var(--primary) 45%, var(--border)); + background: color-mix(in oklch, var(--primary) 8%, var(--background)); +} + +/* "Saved" flash next to the role label after an inline auto-save. */ +.role-row-saved-flash { + font-size: 11px; + font-weight: 500; + color: var(--primary); + opacity: 0.9; +} + /* ============================================================================ * EDIT VIEW — TWO COLUMN * ============================================================================ */ @@ -445,10 +496,21 @@ .role-row { padding: 8px 12px; gap: 10px; + flex-wrap: wrap; } .role-row-slug { display: none; } + /* Let the inline model control drop beneath the label at narrow widths + * while keeping the edit/delete actions reachable on the first line. */ + .role-row-info { + flex: 1 1 auto; + } + .role-row-model-control { + flex: 1 1 100%; + order: 3; + max-width: none; + } .role-row-action-btn { width: 28px; height: 28px; diff --git a/src/app/settings-page.ts b/src/app/settings-page.ts index 15f9414b3..141d59dc5 100644 --- a/src/app/settings-page.ts +++ b/src/app/settings-page.ts @@ -1736,6 +1736,49 @@ export function formatModelPref(value: string, fallbackLabel: string = "Auto (be return slash > 0 ? value.slice(slash + 1) : value; } +// ============================================================================ +// ROLES PAGE — shared default-model display helpers +// +// Roles UI display heuristic (NOT authoritative runtime resolution). Inherited +// role list rows need a deterministic at-a-glance default to show. Roles in +// this allowlist display the review-model default; all other roles (including +// custom) display the session-model default. Runtime model selection remains +// contextual — verification/QA sessions use review defaults regardless of the +// role name. Keep this allowlist deliberate; it is pinned by tests. +// ============================================================================ +export const REVIEW_DEFAULT_ROLE_NAMES = new Set([ + "architect", + "code-reviewer", + "reviewer", + "security-reviewer", + "spec-auditor", + "qa-tester", +]); + +export function getRoleDefaultModelPrefKey(roleName: string): "default.sessionModel" | "default.reviewModel" { + return REVIEW_DEFAULT_ROLE_NAMES.has(roleName) ? "default.reviewModel" : "default.sessionModel"; +} + +/** Effective default model + thinking a role inherits when it has no override. */ +export function getRoleDefaultModel(roleName: string): { model: string; thinking: string } { + return getRoleDefaultModelPrefKey(roleName) === "default.reviewModel" + ? { model: prefReviewModel, thinking: prefReviewThinking } + : { model: prefSessionModel, thinking: prefSessionThinking }; +} + +/** Label for an inherited role row, e.g. "gpt-5.5 · default" or + * "Auto (best available) · default" when the default pref is unset. */ +export function formatRoleDefaultModelLabel(roleName: string): string { + return `${formatModelPref(getRoleDefaultModel(roleName).model)} \u00b7 default`; +} + +/** Ensure default model prefs + the model registry are loaded for the Roles + * page even when the user opens Roles directly without visiting Settings. + * loadModelsState() is idempotent and calls renderApp() once resolved. */ +export function ensureModelDefaultsLoaded(): void { + loadModelsState(); +} + function openModelPicker(currentValue: string, onChange: (v: string) => void) { // Build a pseudo-Model from the current pref so the selector can highlight it let currentModel = null; From 7aa9057bd1cb7c7094bce458b3eeac7ef4d495db Mon Sep 17 00:00:00 2001 From: Artur Jonkisz Date: Sat, 20 Jun 2026 16:23:33 +0100 Subject: [PATCH 02/22] test(roles): cover model reshuffle list/detail + save-hang regression MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add browser-fixture coverage for the Roles model reshuffle against the agreed design hooks (role-row-model-control, roles-model-section, data-model-state): - List view: inherited rows resolve the per-role default via the review/session allowlist heuristic and render ' · default' with data-model-state= inherited and no clear/Test; inline model pick + thinking auto-saves, persists across reload, and flips to data-model-state=override with clear/Test; clearing an override reverts model + thinking to inherited; no navigation on interaction. - Detail editor: Model is its own section between Accessory and the tab bar; tab bar is Prompt + Tool Access only (no roles-tab-model); model edit is draft-based and persists via Save. - Save-hang regression: editing a field + Save returns the button from 'Saving…' to a disabled 'Save' without navigating. Gated on the success-path refetch and an rAF flush so it is red against the pre-fix code and green after, robust to renderApp()'s requestAnimationFrame coalescing. - Update the old fixture test that clicked roles-tab-model to assert the Model section instead. - Enrich the fixture's /api/models payload (input/contextWindow/maxTokens/cost) so the shared ModelSelector dialog renders selectable rows. Verified red-before/green-after by temporarily applying the design implementation (list inline control, Model section, loadRolePageData prefs fetch, handleSave renderApp fix) locally, then reverting — no production files changed in this commit. Tests are currently red pending the implementation task landing the hooks. Co-authored-by: bobbit-ai --- .../settings-admin-fixture-entry.ts | 10 +- .../settings-admin-fixture.spec.ts | 277 +++++++++++++++++- 2 files changed, 268 insertions(+), 19 deletions(-) diff --git a/tests/ui-fixtures/settings-admin-fixture-entry.ts b/tests/ui-fixtures/settings-admin-fixture-entry.ts index a5933a5ea..a99f265c8 100644 --- a/tests/ui-fixtures/settings-admin-fixture-entry.ts +++ b/tests/ui-fixtures/settings-admin-fixture-entry.ts @@ -25,10 +25,14 @@ const DEFAULT_IMAGE_MODELS = [ { id: "imagen-4.0-fast-generate-001", name: "Imagen 4 Fast", provider: "google", api: "google-imagen", authenticated: true }, ]; +// Full model shape so the shared ModelSelector dialog renders selectable rows +// (it reads input/contextWindow/maxTokens/cost when building each item — a +// minimal {id,provider,reasoning} stub throws on `model.input.includes`). +const MODEL_COST = { input: 0, output: 0, cacheRead: 0, cacheWrite: 0 }; const DEFAULT_MODELS = [ - { id: "claude-opus-4-1", provider: "anthropic", reasoning: true }, - { id: "claude-sonnet", provider: "anthropic", reasoning: true }, - { id: "gpt-4o", provider: "openai", reasoning: false }, + { id: "claude-opus-4-1", name: "Claude Opus 4.1", provider: "anthropic", api: "anthropic-messages", contextWindow: 1_000_000, maxTokens: 128_000, reasoning: true, input: ["text", "image"], cost: MODEL_COST, authenticated: true }, + { id: "claude-sonnet", name: "Claude Sonnet", provider: "anthropic", api: "anthropic-messages", contextWindow: 1_000_000, maxTokens: 64_000, reasoning: true, input: ["text", "image"], cost: MODEL_COST, authenticated: true }, + { id: "gpt-4o", name: "GPT-4o", provider: "openai", api: "openai-responses", contextWindow: 128_000, maxTokens: 16_000, reasoning: false, input: ["text", "image"], cost: MODEL_COST, authenticated: true }, ]; function defaultRoles(): RoleData[] { diff --git a/tests/ui-fixtures/settings-admin-fixture.spec.ts b/tests/ui-fixtures/settings-admin-fixture.spec.ts index 17df9e258..cfbc12878 100644 --- a/tests/ui-fixtures/settings-admin-fixture.spec.ts +++ b/tests/ui-fixtures/settings-admin-fixture.spec.ts @@ -19,6 +19,27 @@ const IMAGE_SELECTOR_SRC = path.resolve("src/ui/dialogs/ImageModelSelector.ts"); const TEST_MODEL = "anthropic/claude-opus-4-1"; const TEST_THINKING = "high"; +// Distinct session/review default model prefs so the Roles list inherited-display +// heuristic (REVIEW_DEFAULT_ROLE_NAMES → default.reviewModel; others → +// default.sessionModel) is observable per-row. `coder` is a normal role (session +// default); `reviewer` is in the review-default allowlist (review default). +const SESSION_DEFAULT_MODEL = "openai/gpt-4o"; +const SESSION_DEFAULT_LABEL = "gpt-4o"; +const REVIEW_DEFAULT_MODEL = "anthropic/claude-sonnet"; +const REVIEW_DEFAULT_LABEL = "claude-sonnet"; + +// Shared selectors for the relocated model controls (agreed design hooks). +const LIST_MODEL_CONTROL = "[data-testid='role-row-model-control']"; +const DETAIL_MODEL_SECTION = "[data-testid='roles-model-section']"; + +function roleRow(page: Page, name: string) { + return page.locator(`.role-row[data-role-name='${name}']`); +} + +async function storedRole(page: Page, name: string): Promise { + return await page.evaluate((n) => (window as any).__getSettingsAdminRoles().find((r: any) => r.name === n), name); +} + test.beforeAll(() => { fs.mkdirSync(BUNDLE_DIR, { recursive: true }); buildBundle({ @@ -260,12 +281,16 @@ test.describe("Settings/admin UI fixture", () => { expect(await tab.locator("button").filter({ hasText: /^System$/ }).count()).toBe(0); }); - test("role manager model tab renders persisted overrides and saves clear", async ({ page }) => { + test("role manager Model section renders persisted overrides and saves clear", async ({ page }) => { await loadRoles(page, "#/roles/coder"); + // Tab bar is now Prompt + Tool Access only — Model is its own section. await expect(page.locator(".roles-tab").filter({ hasText: "Prompt" })).toBeVisible(); - await page.locator("[data-testid='roles-tab-model']").click(); - await expect(page.locator("[data-testid='roles-model-tab']")).toBeVisible(); - await expect(page.locator("[data-testid='model-row']")).toBeVisible(); + await expect(page.locator("[data-testid='roles-tab-model']")).toHaveCount(0); + const section = page.locator(DETAIL_MODEL_SECTION); + await expect(section).toBeVisible(); + // The explanatory note text is preserved inside the section. + await expect(section.locator("[data-testid='roles-model-tab']")).toBeVisible(); + await expect(section.locator("[data-testid='model-row']")).toBeVisible(); await page.evaluate(({ model, thinking }) => { (window as any).__putSettingsAdminRole("coder", { model, thinkingLevel: thinking }); @@ -273,13 +298,9 @@ test.describe("Settings/admin UI fixture", () => { await reloadFixture(page); await loadRoles(page, "#/roles/coder"); - await page.locator("[data-testid='roles-tab-model']").click(); - const modelRow = page.locator("[data-testid='model-row']"); + const modelRow = page.locator(`${DETAIL_MODEL_SECTION} [data-testid='model-row']`); await expect(modelRow.locator("button").filter({ hasText: "claude-opus-4-1" })).toBeVisible(); - await expect.poll(async () => { - const roles = await page.evaluate(() => (window as any).__getSettingsAdminRoles()); - return roles.find((r: any) => r.name === "coder")?.thinkingLevel; - }).toBe(TEST_THINKING); + await expect.poll(async () => (await storedRole(page, "coder"))?.thinkingLevel).toBe(TEST_THINKING); const clearBtn = modelRow.locator("[data-testid='model-clear-btn']"); await clearBtn.click(); @@ -287,16 +308,240 @@ test.describe("Settings/admin UI fixture", () => { const saveBtn = page.locator("[data-testid='role-save-btn'] button").first(); await expect(saveBtn).toBeEnabled(); await saveBtn.click(); - await expect.poll(async () => { - const roles = await page.evaluate(() => (window as any).__getSettingsAdminRoles()); - return roles.find((r: any) => r.name === "coder")?.model ?? ""; - }).toBe(""); + await expect.poll(async () => (await storedRole(page, "coder"))?.model ?? "").toBe(""); await page.evaluate(() => (window as any).__putSettingsAdminRole("coder", { thinkingLevel: "" })); await expect.poll(async () => { - const roles = await page.evaluate(() => (window as any).__getSettingsAdminRoles()); - const coder = roles.find((r: any) => r.name === "coder"); + const coder = await storedRole(page, "coder"); return [coder?.model ?? "", coder?.thinkingLevel ?? ""]; }).toEqual(["", ""]); }); + + // ──────────────────────────────────────────────────────────────────────── + // Roles model reshuffle — list view inline model + thinking control + // (agreed hooks: role-row-model-control, data-model-state). These run the + // real `renderRoleManagerPage()` against the fixture's role + preference + // store, so they pin the production render path, not a mock. + // ──────────────────────────────────────────────────────────────────────── + + test("list view inherited rows show resolved default model + thinking heuristic", async ({ page }) => { + // Open Roles directly (no Settings visit first) with distinct defaults. + await resetFixture(page, { + prefs: { + "default.sessionModel": SESSION_DEFAULT_MODEL, + "default.reviewModel": REVIEW_DEFAULT_MODEL, + }, + }); + await loadRoles(page, "#/roles"); + + const coderControl = roleRow(page, "coder").locator(LIST_MODEL_CONTROL); + const reviewerControl = roleRow(page, "reviewer").locator(LIST_MODEL_CONTROL); + await expect(coderControl).toBeVisible(); + await expect(reviewerControl).toBeVisible(); + + // Both rows are inherited (no per-role override). + await expect(coderControl).toHaveAttribute("data-model-state", "inherited"); + await expect(reviewerControl).toHaveAttribute("data-model-state", "inherited"); + + // Heuristic: coder (normal) → session default; reviewer (review allowlist) → + // review default. Each shown as " · default". + await expect(coderControl).toContainText(SESSION_DEFAULT_LABEL); + await expect(coderControl).toContainText(/·\s*default/); + await expect(reviewerControl).toContainText(REVIEW_DEFAULT_LABEL); + await expect(reviewerControl).toContainText(/·\s*default/); + + // Inherited rows expose neither clear nor Test (modelValue is empty). + await expect(coderControl.locator("[data-testid='model-clear-btn']")).toHaveCount(0); + await expect(coderControl.locator("[data-testid='model-test-btn']")).toHaveCount(0); + await expect(reviewerControl.locator("[data-testid='model-clear-btn']")).toHaveCount(0); + }); + + test("list view inherited rows fall back to default label when prefs unset", async ({ page }) => { + // No session/review default prefs configured → still suffixed as a default. + await resetFixture(page, { prefs: {} }); + await loadRoles(page, "#/roles"); + const coderControl = roleRow(page, "coder").locator(LIST_MODEL_CONTROL); + await expect(coderControl).toHaveAttribute("data-model-state", "inherited"); + await expect(coderControl).toContainText(/·\s*default/); + await expect(coderControl.locator("[data-testid='model-clear-btn']")).toHaveCount(0); + }); + + test("list view inline model pick + thinking auto-saves and persists across reload", async ({ page }) => { + await resetFixture(page, { prefs: { "default.sessionModel": SESSION_DEFAULT_MODEL } }); + await loadRoles(page, "#/roles"); + + const control = roleRow(page, "coder").locator(LIST_MODEL_CONTROL); + await expect(control).toHaveAttribute("data-model-state", "inherited"); + + // Pick a model inline. Interacting with the control must NOT navigate to the + // editor (design requires stopPropagation on the inline container). + await control.locator("[data-testid='model-row'] button[title='Choose model']").click(); + await page.locator(`agent-model-selector [data-model-id="claude-opus-4-1"]`).dispatchEvent("click"); + + // Auto-saved: stored role gains the model, no Save button needed. + await expect.poll(async () => (await storedRole(page, "coder"))?.model ?? "").toBe(TEST_MODEL); + // Stayed on the list (no navigation to #/roles/coder). + await expect.poll(() => page.evaluate(() => window.location.hash)).toBe("#/roles"); + + // Override hooks now present: state flips and clear/Test appear. + await expect(control).toHaveAttribute("data-model-state", "override"); + await expect(control.locator("[data-testid='model-clear-btn']")).toBeVisible(); + await expect(control.locator("[data-testid='model-test-btn']")).toBeVisible(); + + // Change thinking now that an override is active → auto-saves. + await control.locator("[data-testid='model-row'] button[role='combobox']").click(); + await page.getByRole("option", { name: "High", exact: true }).click(); + await expect.poll(async () => (await storedRole(page, "coder"))?.thinkingLevel ?? "").toBe(TEST_THINKING); + + // Persist across reload. + await reloadFixture(page); + await loadRoles(page, "#/roles"); + const control2 = roleRow(page, "coder").locator(LIST_MODEL_CONTROL); + await expect(control2).toHaveAttribute("data-model-state", "override"); + await expect(control2.locator("[data-testid='model-row']")).toContainText("claude-opus-4-1"); + const after = await storedRole(page, "coder"); + expect([after?.model ?? "", after?.thinkingLevel ?? ""]).toEqual([TEST_MODEL, TEST_THINKING]); + }); + + test("list view clearing an override reverts model + thinking to inherited", async ({ page }) => { + await resetFixture(page, { + prefs: { "default.sessionModel": SESSION_DEFAULT_MODEL }, + roles: [ + { + name: "coder", label: "Coder", promptTemplate: "You write code.", + accessory: "none", toolPolicies: {}, model: TEST_MODEL, thinkingLevel: TEST_THINKING, + createdAt: 1, updatedAt: 1, origin: "builtin", + } as any, + ], + }); + await loadRoles(page, "#/roles"); + + const control = roleRow(page, "coder").locator(LIST_MODEL_CONTROL); + await expect(control).toHaveAttribute("data-model-state", "override"); + const clearBtn = control.locator("[data-testid='model-clear-btn']"); + await expect(clearBtn).toBeVisible(); + + await clearBtn.click(); + + // Both model and thinking revert to empty so the row inherits/defaults. + await expect.poll(async () => { + const c = await storedRole(page, "coder"); + return [c?.model ?? "", c?.thinkingLevel ?? ""]; + }).toEqual(["", ""]); + + // Row flips back to inherited display. + await expect(control).toHaveAttribute("data-model-state", "inherited"); + await expect(control).toContainText(SESSION_DEFAULT_LABEL); + await expect(control).toContainText(/·\s*default/); + await expect(control.locator("[data-testid='model-clear-btn']")).toHaveCount(0); + }); + + // ──────────────────────────────────────────────────────────────────────── + // Roles model reshuffle — detail editor layout + // ──────────────────────────────────────────────────────────────────────── + + test("detail editor renders Model as a section between Accessory and the tabs", async ({ page }) => { + await loadRoles(page, "#/roles/coder"); + + const section = page.locator(DETAIL_MODEL_SECTION); + await expect(section).toBeVisible(); + // No Model tab; tab bar is exactly Prompt + Tool Access. + await expect(page.locator("[data-testid='roles-tab-model']")).toHaveCount(0); + const tabs = page.locator(".roles-tab-bar .roles-tab"); + await expect(tabs).toHaveCount(2); + await expect(tabs.nth(0)).toHaveText("Prompt"); + await expect(tabs.nth(1)).toHaveText("Tool Access"); + + // Vertical order: Accessory section → Model section → tab bar. + const order = await page.evaluate(() => { + const top = (el: Element | null | undefined) => (el ? el.getBoundingClientRect().top : null); + const accessory = Array.from(document.querySelectorAll(".roles-section-title")) + .find((e) => e.textContent?.trim() === "Accessory"); + return { + accessory: top(accessory), + model: top(document.querySelector("[data-testid='roles-model-section']")), + tabBar: top(document.querySelector(".roles-tab-bar")), + }; + }); + expect(order.accessory).not.toBeNull(); + expect(order.model).not.toBeNull(); + expect(order.tabBar).not.toBeNull(); + expect(order.accessory!).toBeLessThan(order.model!); + expect(order.model!).toBeLessThan(order.tabBar!); + }); + + test("detail editor model edit is draft-based and persists via Save", async ({ page }) => { + await loadRoles(page, "#/roles/coder"); + const section = page.locator(DETAIL_MODEL_SECTION); + await expect(section).toBeVisible(); + + // Pick a model in the section. + await section.locator("[data-testid='model-row'] button[title='Choose model']").click(); + await page.locator(`agent-model-selector [data-model-id="claude-opus-4-1"]`).dispatchEvent("click"); + + // Detail = Save button, NOT auto-save: the stored role is unchanged until Save. + await expect(section.locator("[data-testid='model-row']")).toContainText("claude-opus-4-1"); + expect((await storedRole(page, "coder"))?.model ?? "").toBe(""); + + const saveBtn = page.locator("[data-testid='role-save-btn'] button").first(); + await expect(saveBtn).toBeEnabled(); + await saveBtn.click(); + await expect.poll(async () => (await storedRole(page, "coder"))?.model ?? "").toBe(TEST_MODEL); + + // Persists across reload, still shown in the Model section. + await reloadFixture(page); + await loadRoles(page, "#/roles/coder"); + await expect(page.locator(`${DETAIL_MODEL_SECTION} [data-testid='model-row']`)).toContainText("claude-opus-4-1"); + }); + + // ──────────────────────────────────────────────────────────────────────── + // Roles model reshuffle — Save hang regression + // + // Reproduces the race where handleSave's success path calls setHashRoute to a + // hash that is already current (no-op → no hashchange → no rerender), leaving + // the Save button stuck on "Saving…". Fails against the pre-fix code; passes + // once handleSave calls renderApp() in the success path. + // ──────────────────────────────────────────────────────────────────────── + + test("Save button returns from Saving… to idle after a successful save without navigating", async ({ page }) => { + await loadRoles(page, "#/roles/coder"); + const hashBefore = await page.evaluate(() => window.location.hash); + + // Edit the Label field (first text input in the editor) to enable Save. + const labelInput = page.locator("[data-testid='role-editor'] input").first(); + await labelInput.fill("Coder Edited"); + + const saveBtn = page.locator("[data-testid='role-save-btn'] button").first(); + await expect(saveBtn).toBeEnabled(); + await expect(saveBtn).toHaveText("Save"); + + await saveBtn.click(); + + // Gate on the success path running fully: the PUT, then the post-save role + // refetch (GET /api/roles). Both happen in the buggy and fixed builds, so by + // this point `saving` has been left true (buggy) or reset to false (fixed). + // `renderApp` coalesces through requestAnimationFrame, so the PUT logs before + // the "Saving…" paint — gating on the PUT alone would race the pre-render + // idle "Save" and falsely pass. + await expect.poll(async () => { + const log = await page.evaluate(() => (window as any).__getSettingsAdminFetchLog()); + const putIdx = log.findIndex((e: any) => e.method === "PUT" && e.url.includes("/api/roles/coder")); + return putIdx >= 0 && log.slice(putIdx + 1).some((e: any) => e.method === "GET" && e.url.split("?")[0] === "/api/roles"); + }).toBe(true); + + // Deterministically flush any pending rAF render (no wall-clock sleep). + await page.evaluate(() => new Promise((r) => requestAnimationFrame(() => requestAnimationFrame(() => r())))); + + // Stable end state: fixed build re-renders to a disabled "Save"; the pre-fix + // build leaves the button stuck on "Saving…" (no renderApp() in the + // hash-unchanged success path), so this assertion times out → red. + await expect(saveBtn).toHaveText("Save", { timeout: 3_000 }); + await expect(saveBtn).toBeDisabled(); + + // No navigation away from the edit page. + await expect(page.locator("[data-testid='role-editor']")).toBeVisible(); + expect(await page.evaluate(() => window.location.hash)).toBe(hashBefore); + // The edit actually persisted. + expect((await storedRole(page, "coder"))?.label).toBe("Coder Edited"); + }); }); From 61950de3da91934290f78c7b786d3c346a4aea48 Mon Sep 17 00:00:00 2001 From: Artur Jonkisz Date: Sat, 20 Jun 2026 16:41:08 +0100 Subject: [PATCH 03/22] test(roles): add browser E2E for model reshuffle (detail layout, save-hang, list hooks) Co-authored-by: bobbit-ai --- tests/e2e/ui/roles-model-reshuffle.spec.ts | 131 +++++++++++++++++++++ 1 file changed, 131 insertions(+) create mode 100644 tests/e2e/ui/roles-model-reshuffle.spec.ts diff --git a/tests/e2e/ui/roles-model-reshuffle.spec.ts b/tests/e2e/ui/roles-model-reshuffle.spec.ts new file mode 100644 index 000000000..eee956582 --- /dev/null +++ b/tests/e2e/ui/roles-model-reshuffle.spec.ts @@ -0,0 +1,131 @@ +/** + * Browser E2E: Roles model reshuffle — real gateway/browser coverage. + * + * Complements the file:// ui-fixture coverage (tests/ui-fixtures/*) by driving + * the actual app against an in-process gateway: + * - Detail editor: Model is its own section, positioned between the Accessory + * section and the tab bar; the tab bar exposes only Prompt + Tool Access + * (the old `roles-tab-model` tab is gone). + * - Save-hang regression: editing a field and clicking Save returns the Save + * button from "Saving…" to an idle/disabled "Save" WITHOUT navigating away. + * Before the handleSave renderApp() fix this stuck on "Saving…" because + * setHashRoute() is a no-op when saving from the role's own edit page. + * - List rows expose the inline model control hooks (role-row-model-control + * + data-model-state) so CSS/E2E can target inherited vs override state. + * + * Uses a built-in role ("coder") so the spec does not depend on external model + * availability — it never opens the model selector, only asserts layout, + * tabs, list hooks, and the save lifecycle. + */ +import { test, expect } from "../gateway-harness.js"; +import { openApp, navigateToHash } from "./ui-helpers.js"; + +const ROLE = "coder"; + +test.describe("Roles model reshuffle", () => { + test("detail editor: Model section between Accessory and tabs; only Prompt + Tool Access tabs @smoke", async ({ page }) => { + await openApp(page); + await navigateToHash(page, `#/roles/${ROLE}`); + + // Editor for the requested role is mounted. + const editor = page.locator(`[data-testid="role-editor"][data-role-name="${ROLE}"]`); + await expect(editor).toBeVisible({ timeout: 15_000 }); + + // Model section is present and visible. + const modelSection = editor.locator('[data-testid="roles-model-section"]'); + await expect(modelSection).toBeVisible(); + + // There is no Model tab anymore — only Prompt + Tool Access. + await expect(editor.locator('[data-testid="roles-tab-model"]')).toHaveCount(0); + const tabs = editor.locator(".roles-tab-bar .roles-tab"); + await expect(tabs).toHaveCount(2); + await expect(tabs.nth(0)).toHaveText(/Prompt/); + await expect(tabs.nth(1)).toHaveText(/Tool Access/); + await expect( + editor.locator(".roles-tab-bar .roles-tab").filter({ hasText: /^Model$/ }), + ).toHaveCount(0); + + // Section order: Accessory section, then Model section, then tab bar. + const order = await editor.evaluate((root) => { + const main = root.querySelector(".roles-edit-main") ?? root; + const nodes = Array.from(main.children) as HTMLElement[]; + const indexOf = (pred: (el: HTMLElement) => boolean) => + nodes.findIndex(pred); + const accessoryIdx = indexOf((el) => + el.classList.contains("roles-edit-section") && + /Accessory/.test(el.querySelector(".roles-section-title")?.textContent ?? "")); + const modelIdx = indexOf((el) => el.getAttribute("data-testid") === "roles-model-section"); + const tabBarIdx = indexOf((el) => el.classList.contains("roles-tab-bar")); + return { accessoryIdx, modelIdx, tabBarIdx }; + }); + expect(order.accessoryIdx, "Accessory section present").toBeGreaterThanOrEqual(0); + expect(order.modelIdx, "Model section present").toBeGreaterThanOrEqual(0); + expect(order.tabBarIdx, "tab bar present").toBeGreaterThanOrEqual(0); + expect(order.accessoryIdx).toBeLessThan(order.modelIdx); + expect(order.modelIdx).toBeLessThan(order.tabBarIdx); + }); + + test("save-hang regression: Save returns from Saving… to idle without navigation", async ({ page }) => { + await openApp(page); + await navigateToHash(page, `#/roles/${ROLE}`); + + const editor = page.locator(`[data-testid="role-editor"][data-role-name="${ROLE}"]`); + await expect(editor).toBeVisible({ timeout: 15_000 }); + + const hashBefore = await page.evaluate(() => window.location.hash); + + const saveBtn = page.locator('[data-testid="role-save-btn"] button'); + // Initially no changes ⇒ Save is disabled. (Button text is padded with + // whitespace by the layout, so match tolerantly and assert it is NOT "Saving…".) + await expect(saveBtn).toBeDisabled(); + await expect(saveBtn).toHaveText(/^\s*Save\s*$/); + + // Make a change to enable Save. The Label input drives the editor draft. + const labelInput = editor.locator('input[placeholder="e.g. Documentation Writer"]').first(); + await expect(labelInput).toBeVisible(); + const original = await labelInput.inputValue(); + const mutated = `${original} `; // trailing space — a real, reversible change + await labelInput.fill(mutated); + + // Change enables Save. + await expect(saveBtn).toBeEnabled({ timeout: 5_000 }); + + await saveBtn.click(); + + // The crux: the button must settle back to an idle/disabled "Save" — + // NOT stay stuck on "Saving…". Poll the rendered label + disabled state. + await expect(saveBtn).toHaveText(/^\s*Save\s*$/, { timeout: 10_000 }); + await expect(saveBtn).not.toContainText("Saving"); + await expect(saveBtn).toBeDisabled(); + + // And we must still be on the same edit route (no navigation occurred). + const hashAfter = await page.evaluate(() => window.location.hash); + expect(hashAfter).toBe(hashBefore); + await expect(editor).toBeVisible(); + + // Restore the label so the role config is left clean for any later test. + await labelInput.fill(original); + await expect(saveBtn).toBeEnabled({ timeout: 5_000 }); + await saveBtn.click(); + await expect(saveBtn).toBeDisabled({ timeout: 10_000 }); + }); + + test("list rows expose inline model-control hooks (role-row-model-control + data-model-state)", async ({ page }) => { + await openApp(page); + await navigateToHash(page, "#/roles"); + + // Wait for the roles list to render at least one inline model control. + const controls = page.locator('[data-testid="role-row-model-control"]'); + await expect(controls.first()).toBeVisible({ timeout: 15_000 }); + const count = await controls.count(); + expect(count).toBeGreaterThan(0); + + // Every control carries a deterministic inherited|override state hook. + const states = await controls.evaluateAll((els) => + els.map((el) => el.getAttribute("data-model-state"))); + expect(states.length).toBe(count); + for (const s of states) { + expect(["inherited", "override"]).toContain(s); + } + }); +}); From 58e19a6669a259bf62b77a19fe289872173fca3b Mon Sep 17 00:00:00 2001 From: Artur Jonkisz Date: Sat, 20 Jun 2026 16:51:57 +0100 Subject: [PATCH 04/22] test: click explicit role edit action in marketplace E2E Co-authored-by: bobbit-ai --- tests/e2e/ui/marketplace.spec.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/e2e/ui/marketplace.spec.ts b/tests/e2e/ui/marketplace.spec.ts index 01dc484ee..70e1addc1 100644 --- a/tests/e2e/ui/marketplace.spec.ts +++ b/tests/e2e/ui/marketplace.spec.ts @@ -399,7 +399,10 @@ test.describe("Marketplace UI", () => { // finding #2 — a market-pack entity is READ-ONLY: opening its editor shows // a "Manage in Marketplace" note, NOT the legacy customize/revert buttons // (those call override endpoints that can't remove an installed pack). - await roleRow.click(); + // Click the explicit Edit action: role rows now contain an inline model + // control in the middle, so a center-point row click can legitimately target + // that control instead of navigating to the editor. + await roleRow.locator('button[title="Edit"]').click(); await expect(page.locator('[data-testid="market-readonly-note"]')).toBeVisible({ timeout: 10_000 }); await expect(page.locator(".config-action-btn")).toHaveCount(0); From 1b1bd31105c8aff6a494ca0b91ec479b817802a9 Mon Sep 17 00:00:00 2001 From: Artur Jonkisz Date: Sat, 20 Jun 2026 17:11:00 +0100 Subject: [PATCH 05/22] docs: document Roles model UI reshuffle and save-hang fix Co-authored-by: bobbit-ai --- docs/design/per-role-model-overrides.md | 45 +++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/docs/design/per-role-model-overrides.md b/docs/design/per-role-model-overrides.md index 2ed58afcc..ddc68a639 100644 --- a/docs/design/per-role-model-overrides.md +++ b/docs/design/per-role-model-overrides.md @@ -589,3 +589,48 @@ model under a different id" class of bug. | `tests/manual-integration/role-model-override.test.ts` | Manual binding-verification test. | | `AGENTS.md` | Add a Recipes bullet: **"Per-role model override"** → role yaml `model:`/`thinkingLevel:`; bound at session start by `tryAutoSelectModel` / `tryApplyDefaultThinkingLevel` (`session-manager.ts:2814`); reviewer/QA path in `verification-harness.ts` 3 sites. | | `docs/internals.md` | Brief note under "Per-project config" or a new "Per-role model" subsection cross-linking this design doc. | + +--- + +## 8. Roles Model UI Reshuffle & Save-Hang Fix + +To make per-role model and thinking level overrides visible at a glance and editable in one place, the Roles UI was reshuffled (not redesigning the underlying model) to introduce list-row inline model controls, a relocated detail-page Model section, and a bug-fix for a save-button race condition. + +### 8.1 Roles List View Inline Controls & Heuristic + +The Roles list page (`src/app/role-manager-page.ts`) now renders an inline model + thinking level control in the middle of each list row by reusing the existing `renderModelRow` component. + +* **Auto-Save on Change:** Interacting with the inline control (selecting a model, changing thinking level, or clearing an override) immediately persists the change to the backend via `updateRole` (scoped to the current project). A transient `"Saved"` confirmation badge briefly flashes next to the role name on success. +* **Propagation Stopping:** Events (`click`, `keydown`) inside the inline control container call `stopPropagation` to prevent triggering the outer list-row's click handler (which navigates to the role editor). +* **Overridden State:** When a role override is active, the model picker displays with an filled accent style, the thinking dropdown is fully interactive, and both clear (`×`) and Test (flask) buttons are exposed. +* **Inherited State Heuristic:** When no role override exists, the list row displays a resolved, greyed-out effective default model label formatted as ` · default` with the thinking dropdown disabled and clear/Test buttons hidden. Since inherited default resolution is contextual at session-start (reviewer sessions use review defaults, others use session defaults), the list UI uses a deterministic display-only heuristic: + * **Review Default Allowlist (`default.reviewModel`):** `architect`, `code-reviewer`, `reviewer`, `security-reviewer`, `spec-auditor`, and `qa-tester` display the review-model default. + * **Session Default (`default.sessionModel`):** All other roles (including custom roles) display the session-model default. + +### 8.2 Role Detail Editor Relocation + +In the role detail editor (`renderRoleEditor`): + +* **Removed Tab:** The dedicated "Model" tab is removed; the tab bar now exposes only "Prompt" and "Tool Access". +* **Static Section:** Model and thinking controls are presented as a static section placed between the **Accessory** selector and the tab bar, styled identically to the Accessory selector. +* **Draft-Based Saves:** Unlike the list page, modifications within the detail-page Model section do not auto-save. They are draft-based and only committed when the user clicks the main **Save** button. +* **Read-Only Inspector:** The read-only inspector (`renderRoleInspector` used in the goal-proposal modal) is updated to reflect the new vertical layout, rendering the Model section as read-only. + +### 8.3 Save-Hang Bug Fix + +* **Root Cause:** Inside the `handleSave` function of `role-manager-page.ts`, the successful PUT path calls `showEdit(updated)` -> `setHashRoute("role-edit", name)`. If the user saved changes while already on that role's edit page, `window.location.hash` was already current. Consequently, `setHashRoute` was a no-op that did not trigger a `hashchange` event. Because rendering was only bound to route/hash transitions, the DOM was never refreshed; the Save button remained stuck showing "Saving…" even though `saving` was set to `false` in memory. +* **Fix:** Explicitly call `renderApp()` in the success path of `handleSave` (after `showEdit`/`showList`) so that the DOM is guaranteed to re-render and return the button to the idle "Save" state regardless of whether the hash changed. + +### 8.4 Test Coverage + +* **UI Fixture Tests (`tests/ui-fixtures/settings-admin-fixture.spec.ts`):** + * Asserts inherited list rows display the correct default model label according to the allowance heuristic (e.g., `coder` shows session default, `reviewer` shows review default). + * Asserts inline model selection auto-saves, flips row state to override, reveals clear/Test buttons, does not trigger edit page navigation, and survives page reloads. + * Asserts clearing an override reverts the row back to the inherited display state and clears backend fields. + * Asserts the detail page vertical section ordering (Accessory -> Model section -> Tab bar) and draft-based saving. + * Asserts the Save button returns to idle "Save" after saving (regression check for the save-hang). +* **Browser E2E (`tests/e2e/ui/roles-model-reshuffle.spec.ts`):** + * Real browser-driven spec verifying section layout ordering, tab bar composition (exposing only Prompt and Tool Access), and the Save button transition lifecycle under direct text changes on a real gateway. +* **Marketplace E2E (`tests/e2e/ui/marketplace.spec.ts`):** + * Adjusted row-edit navigation. Because list rows now host interactive inline model controls, the test specifically clicks the explicit Pencil Edit action (`button[title="Edit"]`) in the row instead of clicking anywhere on the row. + From 1fe141641982baf587be70444e383ed7ee5c9346 Mon Sep 17 00:00:00 2001 From: Artur Jonkisz Date: Sat, 20 Jun 2026 18:14:58 +0100 Subject: [PATCH 06/22] Add role model/thinking source metadata to /api/roles Expose per-field source hierarchy (current override / inherited / default) and editability for role model + thinkingLevel via a new ConfigCascade.resolveRoleModelResolution() helper, emitted as the backwards-compatible modelResolution field on /api/roles and /api/roles/:name. Lets the Roles UI render accurate source badges without re-deriving the cascade order. Co-authored-by: bobbit-ai --- src/app/api.ts | 34 ++++++++ src/server/agent/config-cascade.ts | 135 +++++++++++++++++++++++++++++ src/server/server.ts | 20 ++++- tests/config-cascade.test.ts | 84 ++++++++++++++++++ 4 files changed, 270 insertions(+), 3 deletions(-) diff --git a/src/app/api.ts b/src/app/api.ts index 47e6b390e..1d291f554 100644 --- a/src/app/api.ts +++ b/src/app/api.ts @@ -2361,6 +2361,34 @@ export async function enqueueInboxManual( // ROLE API // ============================================================================ +/** + * Where a role's `model` / `thinkingLevel` field resolved from in the config + * cascade, relative to the currently-editable scope. Emitted by the server on + * `/api/roles` + `/api/roles/:name` so the Roles list/detail can render an + * accurate source badge and decide inline editability without re-deriving the + * cascade order. See `RoleFieldSource` in `src/server/agent/config-cascade.ts`. + */ +export type RoleFieldSourceKind = "role" | "inherited-role" | "default"; + +export interface RoleFieldSource { + /** Resolved field value when a role layer supplies it; omitted for `default`. */ + value?: string; + source: RoleFieldSourceKind; + /** Cascade layer the value came from (omitted for `default`). */ + origin?: "builtin" | "server" | "user" | "project"; + /** Market-pack name when the value comes from a pack-defined role; null otherwise. */ + originPackName?: string | null; + /** False only for pack-managed (read-only) roles; true otherwise. */ + editable: boolean; + /** Short human label for the source ("Project", "Server", "Built-in", pack name…). */ + sourceLabel: string; +} + +export interface RoleModelResolution { + model: RoleFieldSource; + thinkingLevel: RoleFieldSource; +} + export interface RoleData { name: string; label: string; @@ -2373,6 +2401,12 @@ export interface RoleData { thinkingLevel?: string; createdAt: number; updatedAt: number; + /** + * Per-field source hierarchy for model/thinkingLevel (cascade origin + + * editability). Present on cascade-resolved role payloads from `/api/roles` + * and `/api/roles/:name`; absent on locally-constructed role drafts. + */ + modelResolution?: RoleModelResolution; } // ============================================================================ diff --git a/src/server/agent/config-cascade.ts b/src/server/agent/config-cascade.ts index 8cdc7ac5f..c58df9ad4 100644 --- a/src/server/agent/config-cascade.ts +++ b/src/server/agent/config-cascade.ts @@ -78,6 +78,53 @@ export interface ResolvedPolicy { overrides?: ConfigOrigin; } +/** + * Where a single role field (`model` / `thinkingLevel`) resolved from, relative + * to the currently-editable scope, plus enough metadata for the Roles UI to + * render an accurate source badge and decide whether the field can be edited + * inline. + * + * - `role` — the value comes from the current editable role layer (the project + * layer when a `projectId` is supplied, otherwise the server layer); the user + * can edit/clear it directly. + * - `inherited-role` — the value comes from a lower/ancestor role layer (an + * ancestor project, the server layer, a built-in role, or a market pack). The + * row can still be customized-then-edited unless `editable` is false. + * - `default` — no role layer supplies the field; the effective value falls back + * to the global session/review model + thinking defaults (the client formats + * the default label and may further distinguish `auto`). + */ +export type RoleFieldSourceKind = "role" | "inherited-role" | "default"; + +export interface RoleFieldSource { + /** Resolved field value when a role layer supplies it; omitted for `default`. */ + value?: string; + source: RoleFieldSourceKind; + /** Cascade layer the value came from (omitted for `default`). */ + origin?: ConfigOrigin; + /** Market-pack name when the value comes from a pack-defined role; null otherwise. */ + originPackName?: string | null; + /** False only for pack-managed (read-only) roles; true otherwise. */ + editable: boolean; + /** Short human label for the source ("Project", "Server", "Built-in", pack name…). */ + sourceLabel: string; +} + +export interface RoleModelResolution { + model: RoleFieldSource; + thinkingLevel: RoleFieldSource; +} + +/** Short human label for a cascade origin (used in role source badges). */ +function originSourceLabel(o: ConfigOrigin): string { + switch (o) { + case "builtin": return "Built-in"; + case "server": return "Server"; + case "user": return "User"; + case "project": return "Project"; + } +} + /** * Explicit server-level store accessors. * @@ -219,6 +266,94 @@ export class ConfigCascade { return this.resolveRoleField(roleName, "promptTemplate", projectId); } + /** + * Resolve the source hierarchy for a role's `model` and `thinkingLevel` + * fields so the Roles UI can render accurate source badges and decide inline + * editability without re-deriving the cascade order itself. + * + * The per-field walk follows the same layers as {@link resolveRoleField} + * (current project local → ancestor projects → server → builtin), then falls + * back to the resolved whole-role winner for values that only exist in a + * market pack. Editability is gated solely by pack-managed status (market + * packs are read-only), matching the whole-role customize/revert rules. + */ + resolveRoleModelResolution(roleName: string, projectId?: string): RoleModelResolution { + const entry = this.resolveRolesEntries(projectId).find(e => e.name === roleName); + const packName = entry && entry.origin.kind === "market" + ? (entry.origin.manifest?.name ?? null) + : null; + const packManaged = packName != null; + return { + model: this.resolveRoleFieldSource(roleName, "model", projectId, entry, packManaged, packName), + thinkingLevel: this.resolveRoleFieldSource(roleName, "thinkingLevel", projectId, entry, packManaged, packName), + }; + } + + private resolveRoleFieldSource( + roleName: string, + field: "model" | "thinkingLevel", + projectId: string | undefined, + entry: ResolvedEntity | undefined, + packManaged: boolean, + packName: string | null, + ): RoleFieldSource { + const editable = !packManaged; + if (projectId) { + // Current editable layer: the project's own local override. + const local = this.localRoleField(projectId, roleName, field); + if (local !== undefined) { + return { value: local, source: "role", origin: "project", editable, sourceLabel: "Project" }; + } + // Inherited from an ancestor project layer. + if (this.projectRegistry) { + for (const anc of this.projectRegistry.getAncestors(projectId)) { + const av = this.localRoleField(anc.id, roleName, field); + if (av !== undefined) { + return { value: av, source: "inherited-role", origin: "project", editable, sourceLabel: "Inherited project" }; + } + } + } + // Inherited from the server / builtin role layers. + const sv = this.readRoleField(this.serverStores.getRoles(), roleName, field); + if (sv !== undefined) { + return { value: sv, source: "inherited-role", origin: "server", editable, sourceLabel: "Server" }; + } + const bv = this.readRoleField(this.builtins.getRoles(), roleName, field); + if (bv !== undefined) { + return { value: bv, source: "inherited-role", origin: "builtin", editable, sourceLabel: "Built-in" }; + } + } else { + // Current editable layer: the server-level role store. + const sv = this.readRoleField(this.serverStores.getRoles(), roleName, field); + if (sv !== undefined) { + return { value: sv, source: "role", origin: "server", editable, sourceLabel: "Server" }; + } + const bv = this.readRoleField(this.builtins.getRoles(), roleName, field); + if (bv !== undefined) { + return { value: bv, source: "inherited-role", origin: "builtin", editable, sourceLabel: "Built-in" }; + } + } + // Pack / winner fallback: covers fields that only exist on a role defined + // by a market pack (which the plain-layer walk above does not read). + if (entry) { + const wv = (entry.item as any)[field]; + if (typeof wv === "string" && wv.trim().length > 0) { + const origin = scopeToOrigin(entry.origin.scope); + return { + value: wv, + source: "inherited-role", + origin, + originPackName: packName, + editable, + sourceLabel: packName ?? originSourceLabel(origin), + }; + } + } + // No role layer supplies the field — the client renders the session/review + // (or auto) default for this label. + return { source: "default", editable, sourceLabel: "Default" }; + } + // ── Roles ──────────────────────────────────────────────────── resolveRoles(projectId?: string): ResolvedItem[] { diff --git a/src/server/server.ts b/src/server/server.ts index d63444be5..65fa0c0a9 100644 --- a/src/server/server.ts +++ b/src/server/server.ts @@ -2647,6 +2647,20 @@ async function handleApiRoute( originPackId: r.originPackId ?? null, originPackName: r.originPackName ?? null, }); + /** + * Serialize a cascade-resolved role with origin metadata PLUS the per-field + * `modelResolution` (model/thinkingLevel source hierarchy + editability) the + * Roles list/detail UI uses to render accurate source badges. Backwards + * compatible: existing top-level fields (model, thinkingLevel, origin…) are + * preserved; `modelResolution` is purely additive. + */ + const withRoleResolution = ( + r: { item: Record; origin: unknown; overrides?: unknown; originPackId?: string | null; originPackName?: string | null }, + projectId?: string, + ): Record => ({ + ...withOrigin(r), + modelResolution: configCascade.resolveRoleModelResolution(String(r.item.name), projectId), + }); // Roles/tools resolution is recomputed per call; the slash-skills TTL cache // and the ToolManager mtime-keyed scan cache both need busting after a // marketplace pack-list mutation (design §9.1 / finding #1) so newly @@ -7656,7 +7670,7 @@ async function handleApiRoute( if (url.pathname === "/api/roles" && req.method === "GET") { const projectId = url.searchParams.get("projectId") || undefined; const resolved = configCascade.resolveRoles(projectId); - json({ roles: resolved.map(r => withOrigin(r as any)) }); + json({ roles: resolved.map(r => withRoleResolution(r as any, projectId)) }); return; } @@ -7768,11 +7782,11 @@ async function handleApiRoute( const resolved = configCascade.resolveRoles(qProjectId); const found = resolved.find(r => r.item.name === name); if (!found) { json({ error: "Role not found" }, 404); return; } - json(withOrigin(found as any)); + json(withRoleResolution(found as any, qProjectId)); } else { const role = roleManager.getRole(name); if (!role) { json({ error: "Role not found" }, 404); return; } - json(role); + json({ ...role, modelResolution: configCascade.resolveRoleModelResolution(name) }); } return; } diff --git a/tests/config-cascade.test.ts b/tests/config-cascade.test.ts index 41853e31c..a97ca6fb8 100644 --- a/tests/config-cascade.test.ts +++ b/tests/config-cascade.test.ts @@ -103,6 +103,90 @@ describe("ConfigCascade — Role.model and Role.thinkingLevel three-layer resolu assert.equal(projCoder!.item.thinkingLevel, "high"); }); + it("resolveRoleModelResolution reports source hierarchy + editability per field", () => { + const builtinsDir = mkTemp(); + writeRoleYaml(builtinsDir, "coder", { + model: "anthropic/claude-haiku", + thinkingLevel: "low", + }); + // Role with no field overrides anywhere → both fall back to default. + writeRoleYaml(builtinsDir, "plain", {}); + const builtins = new BuiltinConfigProvider(builtinsDir); + + const serverDir = mkTemp(); + const serverRoleStore = new RoleStore(serverDir); + // Server overrides only the model for coder; thinkingLevel still inherited. + serverRoleStore.put({ + name: "coder", + label: "Coder", + promptTemplate: "p", + accessory: "none", + model: "anthropic/claude-sonnet", + createdAt: 0, + updatedAt: 0, + }); + + const projectDir = mkTemp(); + const projectRoleStore = new RoleStore(projectDir); + // Project overrides only thinkingLevel (thinking-only override); model + // stays inherited from the server layer. + projectRoleStore.put({ + name: "coder", + label: "Coder", + promptTemplate: "p", + accessory: "none", + thinkingLevel: "high", + createdAt: 0, + updatedAt: 0, + }); + + const serverStores = { + getRoles: () => serverRoleStore.getAllLocal(), + getPersonalities: () => [], + getWorkflows: () => [], + getTools: () => [], + getToolGroupPolicies: () => ({}), + }; + const fakePcm = { + getOrCreate: (id: string) => id === "proj1" ? { roleStore: projectRoleStore } : undefined, + } as any; + + const cascade = new ConfigCascade(builtins, serverStores, fakePcm); + + // System scope (no project): server is the editable layer. + const sys = cascade.resolveRoleModelResolution("coder"); + assert.equal(sys.model.source, "role"); + assert.equal(sys.model.origin, "server"); + assert.equal(sys.model.value, "anthropic/claude-sonnet"); + assert.equal(sys.model.editable, true); + assert.equal(sys.model.sourceLabel, "Server"); + // thinkingLevel only exists in builtin → inherited at system scope. + assert.equal(sys.thinkingLevel.source, "inherited-role"); + assert.equal(sys.thinkingLevel.origin, "builtin"); + assert.equal(sys.thinkingLevel.value, "low"); + + // Project scope: project is the editable layer. + const proj = cascade.resolveRoleModelResolution("coder", "proj1"); + // model not overridden in project → inherited from server. + assert.equal(proj.model.source, "inherited-role"); + assert.equal(proj.model.origin, "server"); + assert.equal(proj.model.value, "anthropic/claude-sonnet"); + assert.equal(proj.model.editable, true); + // thinking-only override at the project layer. + assert.equal(proj.thinkingLevel.source, "role"); + assert.equal(proj.thinkingLevel.origin, "project"); + assert.equal(proj.thinkingLevel.value, "high"); + assert.equal(proj.thinkingLevel.sourceLabel, "Project"); + + // Role with no field anywhere → default fallback (no value). + const plain = cascade.resolveRoleModelResolution("plain", "proj1"); + assert.equal(plain.model.source, "default"); + assert.equal(plain.model.value, undefined); + assert.equal(plain.model.editable, true); + assert.equal(plain.thinkingLevel.source, "default"); + assert.equal(plain.thinkingLevel.value, undefined); + }); + it("falls through to builtin when neither server nor project define the role", () => { const builtinsDir = mkTemp(); writeRoleYaml(builtinsDir, "tester", { From e24f82b71aeb4e8515e0124260d554d441648062 Mon Sep 17 00:00:00 2001 From: Artur Jonkisz Date: Sat, 20 Jun 2026 18:16:01 +0100 Subject: [PATCH 07/22] test(roles): pin polished list model-control contract (source badges, thinking-only, pack read-only, no-overflow) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add/update fixture + browser E2E tests for the polished Roles list model controls per the design doc. These are test-first and currently red against the two in-flight implementation branches; they encode the agreed hook contract: - role-row-model-source / role-row-thinking-source compact source badges (Role override, Inherited role override · , Session/Review/Auto default) - data-model-state ∈ inherited | override | thinking-override | readonly - role-row-thinking-clear-btn to reset thinking independently - read-only pack rows show 'Managed by pack ' + effective model, no edits - E2E bounding-box checks: sub-controls + select chevrons stay within bounds (catches the current chevron-overflow bug), control stays compact - narrow-viewport: edit/delete actions remain reachable and unclipped Co-authored-by: bobbit-ai --- tests/e2e/ui/roles-model-reshuffle.spec.ts | 73 ++++++++- .../settings-admin-fixture.spec.ts | 151 ++++++++++++++++-- 2 files changed, 212 insertions(+), 12 deletions(-) diff --git a/tests/e2e/ui/roles-model-reshuffle.spec.ts b/tests/e2e/ui/roles-model-reshuffle.spec.ts index eee956582..051636587 100644 --- a/tests/e2e/ui/roles-model-reshuffle.spec.ts +++ b/tests/e2e/ui/roles-model-reshuffle.spec.ts @@ -120,12 +120,81 @@ test.describe("Roles model reshuffle", () => { const count = await controls.count(); expect(count).toBeGreaterThan(0); - // Every control carries a deterministic inherited|override state hook. + // Every control carries a deterministic state hook. The polished list adds + // thinking-only and read-only (pack) states alongside inherited/override. const states = await controls.evaluateAll((els) => els.map((el) => el.getAttribute("data-model-state"))); expect(states.length).toBe(count); for (const s of states) { - expect(["inherited", "override"]).toContain(s); + expect(["inherited", "override", "thinking-override", "partial-override", "readonly"]).toContain(s); } }); + + test("list rows: inline model control is compact and its chevrons do not overflow", async ({ page }) => { + await openApp(page); + await navigateToHash(page, "#/roles"); + + const control = page.locator('[data-testid="role-row-model-control"]').first(); + await expect(control).toBeVisible({ timeout: 15_000 }); + + const geom = await control.evaluate((root) => { + const cb = root.getBoundingClientRect(); + const within = (el: Element | null, parent: DOMRect, tol = 1) => { + if (!el) return true; + const r = el.getBoundingClientRect(); + return ( + r.left >= parent.left - tol && r.right <= parent.right + tol && + r.top >= parent.top - tol && r.bottom <= parent.bottom + tol + ); + }; + // 1) Every sub-control (buttons, selects, chevrons) stays within the + // control's own bounds — nothing spills out of the row cell. + const subControls = Array.from(root.querySelectorAll('button, select, [role="combobox"], svg')); + const allContained = subControls.every((el) => within(el, cb)); + + // 2) Each select/combobox chevron stays within ITS OWN control bounds + // (the "arrows must not overflow their option/control" requirement). + const selects = Array.from(root.querySelectorAll('[role="combobox"], select')); + let chevronContained = true; + for (const sel of selects) { + const selRect = sel.getBoundingClientRect(); + for (const svg of Array.from(sel.querySelectorAll('svg'))) { + if (!within(svg, selRect, 1)) chevronContained = false; + } + } + return { allContained, chevronContained, height: cb.height }; + }); + + expect(geom.allContained, "all sub-controls/chevrons contained within the control").toBe(true); + expect(geom.chevronContained, "select chevrons stay within their own control bounds").toBe(true); + // Compact: at most a small two-line control, not a sprawling block. + expect(geom.height, "inline control stays compact").toBeLessThanOrEqual(140); + }); + + test("list rows remain usable at a narrow viewport (edit/delete reachable)", async ({ page }) => { + await page.setViewportSize({ width: 380, height: 800 }); + await openApp(page); + await navigateToHash(page, "#/roles"); + + const row = page.locator(".role-row").first(); + await expect(row).toBeVisible({ timeout: 15_000 }); + + const editBtn = row.locator(".role-row-action-btn:not(.delete)").first(); + const deleteBtn = row.locator(".role-row-action-btn.delete").first(); + await expect(editBtn).toBeVisible(); + await expect(deleteBtn).toBeVisible(); + + // Action buttons must not be clipped off the right edge of the viewport. + const vw = page.viewportSize()!.width; + for (const btn of [editBtn, deleteBtn]) { + const box = await btn.boundingBox(); + expect(box, "action button has a layout box").not.toBeNull(); + expect(box!.x).toBeGreaterThanOrEqual(0); + expect(box!.x + box!.width).toBeLessThanOrEqual(vw + 1); + } + + // The edit action still works at narrow width. + await editBtn.click(); + await expect(page.locator('[data-testid="role-editor"]')).toBeVisible({ timeout: 10_000 }); + }); }); diff --git a/tests/ui-fixtures/settings-admin-fixture.spec.ts b/tests/ui-fixtures/settings-admin-fixture.spec.ts index cfbc12878..a794a5423 100644 --- a/tests/ui-fixtures/settings-admin-fixture.spec.ts +++ b/tests/ui-fixtures/settings-admin-fixture.spec.ts @@ -32,6 +32,28 @@ const REVIEW_DEFAULT_LABEL = "claude-sonnet"; const LIST_MODEL_CONTROL = "[data-testid='role-row-model-control']"; const DETAIL_MODEL_SECTION = "[data-testid='roles-model-section']"; +// ── Polish Roles list model rows — agreed list-control contract ───────────── +// The design doc replaces the old collapsed " · default" suffix with +// explicit, compact source badges for BOTH model and thinking, and makes a +// thinking-only override a first-class, clearable state. These hooks are the +// agreed contract between the implementation tasks (server source metadata + +// compact list controls) and this test suite. Implementation should expose: +// - [data-testid='role-row-model-source'] — model source badge +// - [data-testid='role-row-thinking-source'] — thinking source badge +// - [data-testid='role-row-thinking-clear-btn'] — reset thinking independently +// - [data-testid='role-row-model-readonly'] — read-only pack summary line +// - data-model-state ∈ inherited | override | thinking-override | readonly +const LIST_MODEL_SOURCE = "[data-testid='role-row-model-source']"; +const LIST_THINKING_SOURCE = "[data-testid='role-row-thinking-source']"; +const LIST_THINKING_CLEAR = "[data-testid='role-row-thinking-clear-btn']"; + +// Design-doc source-label language (recommended badges). +const SRC_ROLE_OVERRIDE = /Role override/; +const SRC_INHERITED_ROLE = /Inherited role override/; +const SRC_SESSION_DEFAULT = /Session default/; +const SRC_REVIEW_DEFAULT = /Review default/; +const SRC_ANY_DEFAULT = /default/i; + function roleRow(page: Page, name: string) { return page.locator(`.role-row[data-role-name='${name}']`); } @@ -324,7 +346,7 @@ test.describe("Settings/admin UI fixture", () => { // store, so they pin the production render path, not a mock. // ──────────────────────────────────────────────────────────────────────── - test("list view inherited rows show resolved default model + thinking heuristic", async ({ page }) => { + test("list view inherited rows show resolved default model + thinking with source badges", async ({ page }) => { // Open Roles directly (no Settings visit first) with distinct defaults. await resetFixture(page, { prefs: { @@ -343,12 +365,17 @@ test.describe("Settings/admin UI fixture", () => { await expect(coderControl).toHaveAttribute("data-model-state", "inherited"); await expect(reviewerControl).toHaveAttribute("data-model-state", "inherited"); - // Heuristic: coder (normal) → session default; reviewer (review allowlist) → - // review default. Each shown as " · default". + // The effective model is always shown (never an icon-only / "…"-only row): + // coder (normal) → session default; reviewer (review allowlist) → review default. await expect(coderControl).toContainText(SESSION_DEFAULT_LABEL); - await expect(coderControl).toContainText(/·\s*default/); await expect(reviewerControl).toContainText(REVIEW_DEFAULT_LABEL); - await expect(reviewerControl).toContainText(/·\s*default/); + + // Source is communicated by explicit badges for BOTH model and thinking, + // replacing the old collapsed "· default" suffix. + await expect(coderControl.locator(LIST_MODEL_SOURCE)).toHaveText(SRC_SESSION_DEFAULT); + await expect(coderControl.locator(LIST_THINKING_SOURCE)).toHaveText(SRC_SESSION_DEFAULT); + await expect(reviewerControl.locator(LIST_MODEL_SOURCE)).toHaveText(SRC_REVIEW_DEFAULT); + await expect(reviewerControl.locator(LIST_THINKING_SOURCE)).toHaveText(SRC_REVIEW_DEFAULT); // Inherited rows expose neither clear nor Test (modelValue is empty). await expect(coderControl.locator("[data-testid='model-clear-btn']")).toHaveCount(0); @@ -356,16 +383,46 @@ test.describe("Settings/admin UI fixture", () => { await expect(reviewerControl.locator("[data-testid='model-clear-btn']")).toHaveCount(0); }); - test("list view inherited rows fall back to default label when prefs unset", async ({ page }) => { - // No session/review default prefs configured → still suffixed as a default. + test("list view inherited rows fall back to an Auto default badge when prefs unset", async ({ page }) => { + // No session/review default prefs configured → effective model is Auto, but + // the row still shows a meaningful default source badge (never blank). await resetFixture(page, { prefs: {} }); await loadRoles(page, "#/roles"); const coderControl = roleRow(page, "coder").locator(LIST_MODEL_CONTROL); await expect(coderControl).toHaveAttribute("data-model-state", "inherited"); - await expect(coderControl).toContainText(/·\s*default/); + await expect(coderControl).toContainText(/Auto/); + await expect(coderControl.locator(LIST_MODEL_SOURCE)).toHaveText(SRC_ANY_DEFAULT); await expect(coderControl.locator("[data-testid='model-clear-btn']")).toHaveCount(0); }); + test("list view inherited role override surfaces an 'Inherited role override' badge with source", async ({ page }) => { + // The current scope does NOT override, but a lower (server) role layer + // supplies the model. The server reports this via modelResolution so the + // list can label it distinctly from a current-scope override or a default. + await resetFixture(page, { + prefs: { "default.sessionModel": SESSION_DEFAULT_MODEL }, + roles: [ + { + name: "coder", label: "Coder", promptTemplate: "You write code.", + accessory: "none", toolPolicies: {}, model: TEST_MODEL, + createdAt: 1, updatedAt: 1, origin: "project", + modelResolution: { + model: { value: TEST_MODEL, source: "inherited-role", origin: "server", editable: true }, + thinkingLevel: { value: "", source: "default", editable: true }, + }, + } as any, + ], + }); + await loadRoles(page, "#/roles"); + const control = roleRow(page, "coder").locator(LIST_MODEL_CONTROL); + await expect(control).toBeVisible(); + // Effective model is shown. + await expect(control).toContainText("claude-opus-4-1"); + // Distinct inherited-role badge naming where it came from. + await expect(control.locator(LIST_MODEL_SOURCE)).toHaveText(SRC_INHERITED_ROLE); + await expect(control.locator(LIST_MODEL_SOURCE)).toHaveText(/Server/i); + }); + test("list view inline model pick + thinking auto-saves and persists across reload", async ({ page }) => { await resetFixture(page, { prefs: { "default.sessionModel": SESSION_DEFAULT_MODEL } }); await loadRoles(page, "#/roles"); @@ -387,6 +444,8 @@ test.describe("Settings/admin UI fixture", () => { await expect(control).toHaveAttribute("data-model-state", "override"); await expect(control.locator("[data-testid='model-clear-btn']")).toBeVisible(); await expect(control.locator("[data-testid='model-test-btn']")).toBeVisible(); + // And the model source badge now reads "Role override". + await expect(control.locator(LIST_MODEL_SOURCE)).toHaveText(SRC_ROLE_OVERRIDE); // Change thinking now that an override is active → auto-saves. await control.locator("[data-testid='model-row'] button[role='combobox']").click(); @@ -429,11 +488,83 @@ test.describe("Settings/admin UI fixture", () => { return [c?.model ?? "", c?.thinkingLevel ?? ""]; }).toEqual(["", ""]); - // Row flips back to inherited display. + // Row flips back to inherited display with a default source badge. await expect(control).toHaveAttribute("data-model-state", "inherited"); await expect(control).toContainText(SESSION_DEFAULT_LABEL); - await expect(control).toContainText(/·\s*default/); + await expect(control.locator(LIST_MODEL_SOURCE)).toHaveText(SRC_SESSION_DEFAULT); + await expect(control.locator("[data-testid='model-clear-btn']")).toHaveCount(0); + }); + + test("list view thinking-only override is visible, editable, and clearable without a model override", async ({ page }) => { + // A role with a thinking override but NO model override. The old behavior + // hid this (rendered as a plain inherited row); the polished list must show + // it as a first-class state with the inherited/default model still visible. + await resetFixture(page, { + prefs: { "default.sessionModel": SESSION_DEFAULT_MODEL }, + roles: [ + { + name: "coder", label: "Coder", promptTemplate: "You write code.", + accessory: "none", toolPolicies: {}, thinkingLevel: TEST_THINKING, + createdAt: 1, updatedAt: 1, origin: "builtin", + } as any, + ], + }); + await loadRoles(page, "#/roles"); + + const control = roleRow(page, "coder").locator(LIST_MODEL_CONTROL); + await expect(control).toBeVisible(); + // First-class thinking-only state (accept either agreed spelling). + await expect(control).toHaveAttribute("data-model-state", /thinking-override|partial-override/); + // Model still resolves to the inherited default — never an icon-only row. + await expect(control).toContainText(SESSION_DEFAULT_LABEL); + await expect(control.locator(LIST_MODEL_SOURCE)).toHaveText(SRC_SESSION_DEFAULT); + // Thinking is clearly an override and exposes an independent reset. + await expect(control.locator(LIST_THINKING_SOURCE)).toHaveText(SRC_ROLE_OVERRIDE); + const thinkingClear = control.locator(LIST_THINKING_CLEAR); + await expect(thinkingClear).toBeVisible(); + + await thinkingClear.click(); + + // Clearing thinking only resets thinking; model was never overridden. + await expect.poll(async () => { + const c = await storedRole(page, "coder"); + return [c?.model ?? "", c?.thinkingLevel ?? ""]; + }).toEqual(["", ""]); + // Row reverts to a fully inherited display. + await expect(control).toHaveAttribute("data-model-state", "inherited"); + await expect(control.locator(LIST_THINKING_SOURCE)).toHaveText(SRC_SESSION_DEFAULT); + }); + + test("list view read-only pack role shows its source + effective values and no edit affordances", async ({ page }) => { + // A role installed from a market pack is read-only: it must still show the + // effective model + thinking and explain WHY it cannot be edited inline. + await resetFixture(page, { + prefs: { "default.sessionModel": SESSION_DEFAULT_MODEL }, + roles: [ + { + name: "researcher", label: "Researcher", promptTemplate: "You research.", + accessory: "none", toolPolicies: {}, model: TEST_MODEL, thinkingLevel: TEST_THINKING, + createdAt: 1, updatedAt: 1, origin: "server", originPackName: "research-pack", + modelResolution: { + model: { value: TEST_MODEL, source: "inherited-role", originPackName: "research-pack", editable: false }, + thinkingLevel: { value: TEST_THINKING, source: "inherited-role", originPackName: "research-pack", editable: false }, + }, + } as any, + ], + }); + await loadRoles(page, "#/roles"); + + const control = roleRow(page, "researcher").locator(LIST_MODEL_CONTROL); + await expect(control).toBeVisible(); + await expect(control).toHaveAttribute("data-model-state", "readonly"); + // Effective model + thinking remain visible. + await expect(control).toContainText("claude-opus-4-1"); + // The reason names the managing pack. + await expect(control).toContainText(/Managed by pack research-pack/); + // No inline mutation affordances on a read-only pack row. await expect(control.locator("[data-testid='model-clear-btn']")).toHaveCount(0); + await expect(control.locator(LIST_THINKING_CLEAR)).toHaveCount(0); + await expect(control.locator("[data-testid='model-row'] button[title='Choose model']")).toHaveCount(0); }); // ──────────────────────────────────────────────────────────────────────── From abfb5101f8861911bc882e85e92467948be45417 Mon Sep 17 00:00:00 2001 From: Artur Jonkisz Date: Sat, 20 Jun 2026 18:17:55 +0100 Subject: [PATCH 08/22] Compact, source-aware Roles list model/thinking controls Co-authored-by: bobbit-ai --- src/app/role-manager-page.ts | 241 ++++++++++++++++++++++++++++++----- src/app/role-manager.css | 131 ++++++++++++++++--- 2 files changed, 320 insertions(+), 52 deletions(-) diff --git a/src/app/role-manager-page.ts b/src/app/role-manager-page.ts index a2b3f48a5..99eca49bd 100644 --- a/src/app/role-manager-page.ts +++ b/src/app/role-manager-page.ts @@ -13,7 +13,7 @@ import { renderIdleBlobCanvas } from "../ui/bobbit-render.js"; import { state, renderApp } from "./state.js"; import { setHashRoute } from "./routing.js"; import { type ConfigOrigin, getConfigScope, setConfigScope, getConfigProjectId, renderOriginBadge, isInherited, renderConfigScopeRow, customizeItem, revertOverride, getCurrentProjectName } from "./config-scope.js"; -import { renderModelRow, formatModelPref, formatRoleDefaultModelLabel, ensureModelDefaultsLoaded } from "./settings-page.js"; +import { renderModelRow, formatModelPref, getRoleDefaultModel, getRoleDefaultModelPrefKey, ensureModelDefaultsLoaded } from "./settings-page.js"; // ============================================================================ // HELPERS @@ -280,6 +280,19 @@ async function handleSave(): Promise { * has already surfaced the error via `showConnectionError`; we just rerender. */ async function persistInlineModel(role: RoleData, model: string, thinkingLevel: string): Promise { + const projectId = getConfigProjectId(); + // Project scope: an inherited role has no project-layer override yet, so the + // PUT /api/roles/:name?projectId= path 404s ("Role not found in project"). + // Copy the resolved role into the project layer first so every editable list + // row can be saved inline. System-scope builtins are auto-promoted to a + // server override by the PUT handler, so no explicit customize is needed there. + if (projectId && isInherited((role as any).origin as ConfigOrigin | undefined)) { + const customized = await customizeItem("roles", role.name, "project", projectId); + if (!customized) { + renderApp(); + return; + } + } const ok = await updateRole(role.name, { label: role.label, promptTemplate: role.promptTemplate, @@ -287,7 +300,7 @@ async function persistInlineModel(role: RoleData, model: string, thinkingLevel: toolPolicies: role.toolPolicies ?? {}, model, thinkingLevel, - }, getConfigProjectId() || undefined); + }, projectId || undefined); if (!ok) { renderApp(); return; @@ -531,39 +544,199 @@ interface RoleRowOptions { modelControl?: RoleRowModelControl; } +// ---------------------------------------------------------------------------- +// Inline list-row model/thinking source resolution +// +// Each list row shows two independent fields (model + thinking) with a compact +// source badge. When the server attaches field-level `modelResolution` +// metadata we use it; otherwise we degrade using `origin` / `isInherited` plus +// the default-pref display heuristic so the row always shows meaningful info. +// ---------------------------------------------------------------------------- + +const THINKING_LEVEL_LABELS: Record = { + off: "Off", minimal: "Minimal", low: "Low", medium: "Medium", high: "High", xhigh: "Extra high", +}; + +/** Optional backwards-compatible field-level source metadata from /api/roles. + * Attached by the server on `role.modelResolution`; absent on older API data. */ +interface RoleFieldResolution { + value?: string; + source?: "role" | "inherited-role" | "default" | "auto"; + origin?: ConfigOrigin; + originPackName?: string | null; + editable?: boolean; +} +interface RoleModelResolution { + model?: RoleFieldResolution; + thinkingLevel?: RoleFieldResolution; +} + +type RoleFieldSourceKind = "role" | "inherited-role" | "default" | "auto"; + +interface RoleFieldDisplay { + /** True when the value comes from a role-level override (current or inherited role). */ + override: boolean; + kind: RoleFieldSourceKind; + /** Compact source badge text, e.g. "Role override", "Session default". */ + badge: string; + /** Resolved value text (model id / thinking label) for the source line. */ + effectiveLabel: string; +} + +interface RoleModelDisplay { + /** Pack-managed roles are read-only inline (manage via the Marketplace). */ + packReadonly: boolean; + packName?: string | null; + model: RoleFieldDisplay; + thinking: RoleFieldDisplay; + state: "override" | "thinking-override" | "inherited" | "readonly"; +} + +function roleOriginDetailLabel(origin?: ConfigOrigin, packName?: string | null): string { + if (packName) return `pack ${packName}`; + switch (origin) { + case "project": return "Project"; + case "server": return "Server"; + case "builtin": return "Built-in"; + case "user": return "User"; + default: return ""; + } +} + +function thinkingLevelLabel(value: string): string { + return THINKING_LEVEL_LABELS[value] ?? value; +} + +/** + * Resolve the model + thinking display (effective value + source badge) for a + * list row. Prefers server `modelResolution` metadata when present; otherwise + * degrades using `origin` / `isInherited` plus the default-pref heuristic. + */ +function computeRoleModelDisplay(role: RoleData): RoleModelDisplay { + const origin = (role as any).origin as ConfigOrigin | undefined; + const packName = (role as any).originPackName as string | null | undefined; + const packId = (role as any).originPackId as string | null | undefined; + const meta = (role as any).modelResolution as RoleModelResolution | undefined; + const packReadonly = !!(packName || packId); + const inheritedRole = isInherited(origin); + const prefKey = getRoleDefaultModelPrefKey(role.name); + const def = getRoleDefaultModel(role.name); + const defaultBadge = prefKey === "default.reviewModel" ? "Review default" : "Session default"; + + const inheritedBadge = (m: { origin?: ConfigOrigin; originPackName?: string | null }): string => { + const detail = roleOriginDetailLabel(m.origin ?? origin, m.originPackName ?? packName); + return detail ? `Inherited role override · ${detail}` : "Inherited role override"; + }; + + const fromMeta = (m: RoleFieldResolution, format: (v: string) => string, emptyLabel: string): RoleFieldDisplay => { + const value = m.value ?? ""; + const effectiveLabel = value ? format(value) : emptyLabel; + switch (m.source) { + case "role": return { override: true, kind: "role", badge: "Role override", effectiveLabel }; + case "inherited-role": return { override: true, kind: "inherited-role", badge: inheritedBadge(m), effectiveLabel }; + case "auto": return { override: false, kind: "auto", badge: "Auto default", effectiveLabel }; + default: return { override: false, kind: "default", badge: defaultBadge, effectiveLabel }; + } + }; + + const overrideField = (value: string, format: (v: string) => string): RoleFieldDisplay => + inheritedRole + ? { override: true, kind: "inherited-role", badge: inheritedBadge({}), effectiveLabel: format(value) } + : { override: true, kind: "role", badge: "Role override", effectiveLabel: format(value) }; + + // MODEL + let model: RoleFieldDisplay; + if (meta?.model?.source) { + model = fromMeta(meta.model, (v) => formatModelPref(v), formatModelPref(def.model)); + } else if (role.model) { + model = overrideField(role.model, (v) => formatModelPref(v)); + } else { + model = def.model + ? { override: false, kind: "default", badge: defaultBadge, effectiveLabel: formatModelPref(def.model) } + : { override: false, kind: "auto", badge: "Auto default", effectiveLabel: formatModelPref("") }; + } + + // THINKING + const defThinkingLabel = def.thinking ? thinkingLevelLabel(def.thinking) : "Default"; + let thinking: RoleFieldDisplay; + if (meta?.thinkingLevel?.source) { + thinking = fromMeta(meta.thinkingLevel, (v) => thinkingLevelLabel(v), defThinkingLabel); + } else if (role.thinkingLevel) { + thinking = overrideField(role.thinkingLevel, (v) => thinkingLevelLabel(v)); + } else { + thinking = def.thinking + ? { override: false, kind: "default", badge: defaultBadge, effectiveLabel: defThinkingLabel } + : { override: false, kind: "auto", badge: "Auto default", effectiveLabel: "Default" }; + } + + let state: RoleModelDisplay["state"]; + if (packReadonly) state = "readonly"; + else if (role.model) state = "override"; + else if (role.thinkingLevel) state = "thinking-override"; + else state = "inherited"; + + return { packReadonly, packName, model, thinking, state }; +} + /** - * Inline model/thinking control rendered in the middle of a list row. Reuses - * the shared `renderModelRow`. Inherited rows (no `role.model`) show the - * resolved default label and dim the thinking selector; override rows show the - * actual model with clear/Test from `renderModelRow`. + * Inline model/thinking control rendered in the middle of a list row. + * + * Editable rows reuse the shared `renderModelRow` (model picker + clear/Test + + * thinking select) as the interactive line, then add a compact source line + * showing where each field's effective value comes from. Model and thinking are + * independent: clearing the model leaves any thinking override in place (shown + * as a thinking-only override), and thinking can be set/cleared on its own. + * Read-only pack rows render a static effective-value summary plus the reason. */ function renderRoleRowModelControl(role: RoleData, control: RoleRowModelControl): TemplateResult { - const overridden = !!(role.model && role.model.length > 0); - const stateClass = overridden ? "role-row-model-control--override" : "role-row-model-control--inherited"; + const display = computeRoleModelDisplay(role); const stopRow = (e: Event) => e.stopPropagation(); + + if (display.packReadonly) { + return html` +
+
+
Model${display.model.effectiveLabel}
+
Thinking${display.thinking.effectiveLabel}
+
+ Managed by pack ${display.packName} +
+ `; + } + + const sourceLine = (key: string, f: RoleFieldDisplay, testid: string) => html` + + ${key} + ${f.override ? nothing : html`${f.effectiveLabel}`} + ${f.badge} + + `; + return html` -
- ${renderModelRow( - "", - "", - overridden ? (role.model ?? "") : "", - (v) => control.onModelChange(role, v), - overridden ? (role.thinkingLevel ?? "") : "", - (v) => { - // Inherited rows must never auto-save a thinking-only override - // while no model override exists. renderModelRow's reactive - // clamp also fires onThinkingChange via queueMicrotask — ignore - // it here so an inherited row stays inherited. - if (!overridden) return; - control.onThinkingChange(role, v); - }, - "", - { fallbackLabel: overridden ? "(use default)" : formatRoleDefaultModelLabel(role.name) }, - )} +
+ ${renderModelRow( + "", + "", + role.model ?? "", + (v) => control.onModelChange(role, v), + role.thinkingLevel ?? "", + (v) => control.onThinkingChange(role, v), + "", + { fallbackLabel: "(use default)" }, + )} +
+
+ ${sourceLine("Model", display.model, "role-row-model-source")} + ${sourceLine("Thinking", display.thinking, "role-row-thinking-source")} +
`; } @@ -670,12 +843,14 @@ function renderListView(): TemplateResult { modelControl: { savedFlashRole: savedModelFlashRole, onModelChange: (role, model) => { - // Inherited -> override: preserve existing thinking if present. - // Clear -> fully revert to inherited/default display. - void persistInlineModel(role, model, model ? (role.thinkingLevel ?? "") : ""); + // Model and thinking are independent overrides. Setting/clearing the + // model preserves any existing thinking override — clearing the model + // alone yields a thinking-only override (shown clearly in the row). + void persistInlineModel(role, model, role.thinkingLevel ?? ""); }, onThinkingChange: (role, thinking) => { - // Only fired for override rows (guarded in the inline control). + // Thinking is editable/clearable independently of the model, so even + // inherited rows can gain (or drop) a thinking-only override. void persistInlineModel(role, role.model ?? "", thinking); }, }, @@ -900,7 +1075,7 @@ export function renderRoleModelTab(opts: RoleModelTabOptions): TemplateResult { const thinkingDisplay = thinkingLevel || "(default)"; return html`

- Overrides the global default for sessions running as this role. Leave blank to inherit. + Role override — empty inherits the role/default hierarchy (inherited role override, then session/review default).

@@ -916,7 +1091,7 @@ export function renderRoleModelTab(opts: RoleModelTabOptions): TemplateResult { } return html`

- Overrides the global default for sessions running as this role. Leave blank to inherit. + Role override — empty inherits the role/default hierarchy (inherited role override, then session/review default).

${renderModelRow( "Model", diff --git a/src/app/role-manager.css b/src/app/role-manager.css index 9bc311e75..633c6526a 100644 --- a/src/app/role-manager.css +++ b/src/app/role-manager.css @@ -208,42 +208,135 @@ * -------------------------------------------------------------------------- */ .role-row-model-control { flex: 1 1 300px; - min-width: 180px; - max-width: 520px; + min-width: 0; + max-width: 360px; + display: flex; + flex-direction: column; + gap: 3px; + overflow: hidden; } /* Compact placement: hide the (empty) label column and hint paragraph that * renderModelRow emits, so the control fits a list row. Scoped to list rows * only — global Settings model rows are untouched. */ -.role-row-model-control [data-testid="model-row"] > div:first-child > span:first-child { - display: none; -} +.role-row-model-control [data-testid="model-row"] > div:first-child > span:first-child, .role-row-model-control [data-testid="model-row"] > p { display: none; } -/* Inherited rows: dim the whole control and disable the thinking selector so a - * thinking-only change can't create an override (model picker stays active). */ -.role-row-model-control--inherited { - opacity: 0.7; +/* Tighten the renderModelRow control box for list density. */ +.role-row-model-control [data-testid="model-row"] { + gap: 0; } -.role-row-model-control--inherited [data-testid="model-row"] { - color: var(--muted-foreground); +.role-row-model-control [data-testid="model-row"] > div:first-child > div:nth-child(2) { + padding: 2px; + gap: 2px; } -/* Disable just the thinking picker (last child inside the control box) so the - * model picker stays usable while inherited rows can't create a thinking-only - * override. */ -.role-row-model-control--inherited [data-testid="model-row"] > div:first-child > div:nth-child(2) > div:last-child { - pointer-events: none; - opacity: 0.6; +/* The thinking Select defaults to a 180px min-width which bloats the row and + * pushes its chevron past the control edge. Clamp it so the chevron (a + * flex-shrink-0, justify-between child) stays inside the button bounds. */ +.role-row-model-control [data-testid="model-row"] [role="combobox"] { + min-width: 0 !important; + width: auto !important; + max-width: 132px; } -/* Override rows: accent the control border with a light primary tint. */ -.role-row-model-control--override [data-testid="model-row"] > div:first-child > div:nth-child(2) { +/* Override / thinking-override rows: accent the control box with a primary tint. */ +.role-row-model-control--override [data-testid="model-row"] > div:first-child > div:nth-child(2), +.role-row-model-control--thinking-override [data-testid="model-row"] > div:first-child > div:nth-child(2) { border-color: color-mix(in oklch, var(--primary) 45%, var(--border)); background: color-mix(in oklch, var(--primary) 8%, var(--background)); } +/* Compact source line under the control: key + (effective value) + source badge. */ +.role-row-model-sources { + display: flex; + flex-wrap: wrap; + align-items: center; + gap: 3px 10px; + font-size: 11px; + line-height: 1.4; + color: var(--muted-foreground); +} +.rrm-src { + display: inline-flex; + align-items: center; + gap: 4px; + min-width: 0; +} +.rrm-src-key { + font-weight: 600; + opacity: 0.75; +} +.rrm-src-val { + max-width: 120px; + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; + color: var(--foreground); +} +.rrm-badge { + display: inline-flex; + align-items: center; + padding: 1px 6px; + border-radius: 999px; + font-size: 10px; + font-weight: 500; + white-space: nowrap; + border: 1px solid transparent; +} +.rrm-badge--role { + color: var(--primary); + background: color-mix(in oklch, var(--primary) 12%, transparent); + border-color: color-mix(in oklch, var(--primary) 35%, transparent); +} +.rrm-badge--inherited-role { + color: var(--foreground); + background: var(--secondary); + border-color: var(--border); +} +.rrm-badge--default, +.rrm-badge--auto { + color: var(--muted-foreground); + background: var(--muted); +} +.rrm-badge--pack { + color: var(--muted-foreground); + background: var(--muted); + border-color: var(--border); +} + +/* Read-only pack rows: static effective-value summary, no pickers. */ +.role-row-model-control--readonly { + gap: 4px; +} +.role-row-model-readonly { + display: flex; + flex-direction: column; + gap: 1px; + font-size: 12px; +} +.rrm-ro-row { + display: flex; + gap: 6px; + align-items: baseline; + min-width: 0; +} +.rrm-ro-key { + font-weight: 600; + font-size: 11px; + color: var(--muted-foreground); + opacity: 0.75; + width: 52px; + flex-shrink: 0; +} +.rrm-ro-val { + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; + color: var(--foreground); +} + /* "Saved" flash next to the role label after an inline auto-save. */ .role-row-saved-flash { font-size: 11px; From 3e42dd1ae261861f850ed1b76182720eac6ce140 Mon Sep 17 00:00:00 2001 From: Artur Jonkisz Date: Sat, 20 Jun 2026 18:27:08 +0100 Subject: [PATCH 09/22] Align Roles list controls with server metadata + test hook contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Use server modelResolution metadata for source badges (Role override, Inherited role override · , Session/Review/Auto default); degrade gracefully to the default-pref heuristic when metadata is absent. - Add independent role-row-thinking-clear-btn; model clear resets the whole override, thinking clear resets thinking only. - First-class thinking-override state; read-only pack rows show effective values + 'Managed by pack ' with no pickers. Co-authored-by: bobbit-ai --- src/app/role-manager-page.ts | 105 +++++++++++++++++------------------ src/app/role-manager.css | 17 ++++++ 2 files changed, 67 insertions(+), 55 deletions(-) diff --git a/src/app/role-manager-page.ts b/src/app/role-manager-page.ts index 99eca49bd..152874c22 100644 --- a/src/app/role-manager-page.ts +++ b/src/app/role-manager-page.ts @@ -3,8 +3,8 @@ import { icon } from "@mariozechner/mini-lit"; import { Button } from "@mariozechner/mini-lit/dist/Button.js"; import { Input } from "@mariozechner/mini-lit/dist/Input.js"; import { html, nothing, type TemplateResult } from "lit"; -import { ArrowLeft, Pencil, Plus, Trash2 } from "lucide"; -import { fetchTools, updateRole, deleteRole, gatewayFetch, fetchAssistantPrompts, updateAssistantPrompt, fetchGroupPolicies, type RoleData, type ToolInfo, type AssistantPromptInfo } from "./api.js"; +import { ArrowLeft, Pencil, Plus, Trash2, X } from "lucide"; +import { fetchTools, updateRole, deleteRole, gatewayFetch, fetchAssistantPrompts, updateAssistantPrompt, fetchGroupPolicies, type RoleData, type ToolInfo, type AssistantPromptInfo, type RoleFieldSource } from "./api.js"; import { errorFromResponse, errorDetails } from "./error-helpers.js"; import { connectToSession } from "./session-manager.js"; import { showConnectionError, confirmAction } from "./dialogs.js"; @@ -557,26 +557,12 @@ const THINKING_LEVEL_LABELS: Record = { off: "Off", minimal: "Minimal", low: "Low", medium: "Medium", high: "High", xhigh: "Extra high", }; -/** Optional backwards-compatible field-level source metadata from /api/roles. - * Attached by the server on `role.modelResolution`; absent on older API data. */ -interface RoleFieldResolution { - value?: string; - source?: "role" | "inherited-role" | "default" | "auto"; - origin?: ConfigOrigin; - originPackName?: string | null; - editable?: boolean; -} -interface RoleModelResolution { - model?: RoleFieldResolution; - thinkingLevel?: RoleFieldResolution; -} - -type RoleFieldSourceKind = "role" | "inherited-role" | "default" | "auto"; +type RoleFieldSourceUiKind = "role" | "inherited-role" | "default" | "auto"; interface RoleFieldDisplay { /** True when the value comes from a role-level override (current or inherited role). */ override: boolean; - kind: RoleFieldSourceKind; + kind: RoleFieldSourceUiKind; /** Compact source badge text, e.g. "Role override", "Session default". */ badge: string; /** Resolved value text (model id / thinking label) for the source line. */ @@ -609,64 +595,67 @@ function thinkingLevelLabel(value: string): string { /** * Resolve the model + thinking display (effective value + source badge) for a - * list row. Prefers server `modelResolution` metadata when present; otherwise - * degrades using `origin` / `isInherited` plus the default-pref heuristic. + * list row. Prefers the server's field-level `modelResolution` metadata when + * present; otherwise degrades using the default-pref display heuristic. A + * present top-level `model` / `thinkingLevel` with no metadata is shown as a + * "Role override" — only metadata can attribute a value to an inherited role. */ function computeRoleModelDisplay(role: RoleData): RoleModelDisplay { - const origin = (role as any).origin as ConfigOrigin | undefined; - const packName = (role as any).originPackName as string | null | undefined; + const directPackName = (role as any).originPackName as string | null | undefined; const packId = (role as any).originPackId as string | null | undefined; - const meta = (role as any).modelResolution as RoleModelResolution | undefined; - const packReadonly = !!(packName || packId); - const inheritedRole = isInherited(origin); + const meta = role.modelResolution; const prefKey = getRoleDefaultModelPrefKey(role.name); const def = getRoleDefaultModel(role.name); - const defaultBadge = prefKey === "default.reviewModel" ? "Review default" : "Session default"; - const inheritedBadge = (m: { origin?: ConfigOrigin; originPackName?: string | null }): string => { - const detail = roleOriginDetailLabel(m.origin ?? origin, m.originPackName ?? packName); + // The default-tier badge follows the session/review heuristic, downgrading to + // "Auto default" only when no default model is configured at all (the tier is + // effectively auto-selected). Applied consistently to model + thinking. + const autoTier = !def.model; + const defaultBadge = autoTier + ? "Auto default" + : (prefKey === "default.reviewModel" ? "Review default" : "Session default"); + const defaultKind: RoleFieldSourceUiKind = autoTier ? "auto" : "default"; + + const packName = directPackName ?? meta?.model?.originPackName ?? meta?.thinkingLevel?.originPackName ?? undefined; + // Pack-managed roles (or any field the server flags non-editable) are + // read-only inline — manage them via the Marketplace. + const metaReadonly = !!meta && (meta.model.editable === false || meta.thinkingLevel.editable === false); + const packReadonly = !!(directPackName || packId) || metaReadonly; + + const inheritedBadge = (m: RoleFieldSource): string => { + const detail = m.sourceLabel || roleOriginDetailLabel(m.origin as ConfigOrigin | undefined, m.originPackName ?? packName); return detail ? `Inherited role override · ${detail}` : "Inherited role override"; }; - const fromMeta = (m: RoleFieldResolution, format: (v: string) => string, emptyLabel: string): RoleFieldDisplay => { + const fromMeta = (m: RoleFieldSource, format: (v: string) => string, emptyLabel: string): RoleFieldDisplay => { const value = m.value ?? ""; const effectiveLabel = value ? format(value) : emptyLabel; switch (m.source) { case "role": return { override: true, kind: "role", badge: "Role override", effectiveLabel }; case "inherited-role": return { override: true, kind: "inherited-role", badge: inheritedBadge(m), effectiveLabel }; - case "auto": return { override: false, kind: "auto", badge: "Auto default", effectiveLabel }; - default: return { override: false, kind: "default", badge: defaultBadge, effectiveLabel }; + default: return { override: false, kind: defaultKind, badge: defaultBadge, effectiveLabel }; } }; - const overrideField = (value: string, format: (v: string) => string): RoleFieldDisplay => - inheritedRole - ? { override: true, kind: "inherited-role", badge: inheritedBadge({}), effectiveLabel: format(value) } - : { override: true, kind: "role", badge: "Role override", effectiveLabel: format(value) }; - // MODEL let model: RoleFieldDisplay; - if (meta?.model?.source) { + if (meta) { model = fromMeta(meta.model, (v) => formatModelPref(v), formatModelPref(def.model)); } else if (role.model) { - model = overrideField(role.model, (v) => formatModelPref(v)); + model = { override: true, kind: "role", badge: "Role override", effectiveLabel: formatModelPref(role.model) }; } else { - model = def.model - ? { override: false, kind: "default", badge: defaultBadge, effectiveLabel: formatModelPref(def.model) } - : { override: false, kind: "auto", badge: "Auto default", effectiveLabel: formatModelPref("") }; + model = { override: false, kind: defaultKind, badge: defaultBadge, effectiveLabel: formatModelPref(def.model) }; } // THINKING const defThinkingLabel = def.thinking ? thinkingLevelLabel(def.thinking) : "Default"; let thinking: RoleFieldDisplay; - if (meta?.thinkingLevel?.source) { + if (meta) { thinking = fromMeta(meta.thinkingLevel, (v) => thinkingLevelLabel(v), defThinkingLabel); } else if (role.thinkingLevel) { - thinking = overrideField(role.thinkingLevel, (v) => thinkingLevelLabel(v)); + thinking = { override: true, kind: "role", badge: "Role override", effectiveLabel: thinkingLevelLabel(role.thinkingLevel) }; } else { - thinking = def.thinking - ? { override: false, kind: "default", badge: defaultBadge, effectiveLabel: defThinkingLabel } - : { override: false, kind: "auto", badge: "Auto default", effectiveLabel: "Default" }; + thinking = { override: false, kind: defaultKind, badge: defaultBadge, effectiveLabel: defThinkingLabel }; } let state: RoleModelDisplay["state"]; @@ -693,25 +682,31 @@ function renderRoleRowModelControl(role: RoleData, control: RoleRowModelControl) const stopRow = (e: Event) => e.stopPropagation(); if (display.packReadonly) { + const reason = display.packName + ? html`Managed by pack ${display.packName}` + : html`Read-only`; return html`
-
+
Model${display.model.effectiveLabel}
Thinking${display.thinking.effectiveLabel}
- Managed by pack ${display.packName} + ${reason}
`; } - const sourceLine = (key: string, f: RoleFieldDisplay, testid: string) => html` + const sourceLine = (key: string, f: RoleFieldDisplay, testid: string, clearTestid?: string) => html` ${key} ${f.override ? nothing : html`${f.effectiveLabel}`} ${f.badge} + ${clearTestid && f.override ? html`` : nothing} `; @@ -735,7 +730,7 @@ function renderRoleRowModelControl(role: RoleData, control: RoleRowModelControl)
${sourceLine("Model", display.model, "role-row-model-source")} - ${sourceLine("Thinking", display.thinking, "role-row-thinking-source")} + ${sourceLine("Thinking", display.thinking, "role-row-thinking-source", "role-row-thinking-clear-btn")}
`; @@ -843,10 +838,10 @@ function renderListView(): TemplateResult { modelControl: { savedFlashRole: savedModelFlashRole, onModelChange: (role, model) => { - // Model and thinking are independent overrides. Setting/clearing the - // model preserves any existing thinking override — clearing the model - // alone yields a thinking-only override (shown clearly in the row). - void persistInlineModel(role, model, role.thinkingLevel ?? ""); + // Picking a model keeps any existing thinking override; the model + // clear button (X) resets the whole role override (model + thinking). + // Thinking can still be reset on its own via role-row-thinking-clear-btn. + void persistInlineModel(role, model, model ? (role.thinkingLevel ?? "") : ""); }, onThinkingChange: (role, thinking) => { // Thinking is editable/clearable independently of the model, so even diff --git a/src/app/role-manager.css b/src/app/role-manager.css index 633c6526a..61c6c6cf3 100644 --- a/src/app/role-manager.css +++ b/src/app/role-manager.css @@ -305,6 +305,23 @@ background: var(--muted); border-color: var(--border); } +/* Independent thinking reset button on the source line. */ +.rrm-clear-btn { + display: inline-flex; + align-items: center; + justify-content: center; + padding: 1px; + border-radius: 4px; + border: none; + background: transparent; + color: var(--muted-foreground); + cursor: pointer; + transition: background 150ms, color 150ms; +} +.rrm-clear-btn:hover { + color: var(--foreground); + background: var(--secondary); +} /* Read-only pack rows: static effective-value summary, no pickers. */ .role-row-model-control--readonly { From d9325dc6d85a82bfec8f37f3dc85d4233503592b Mon Sep 17 00:00:00 2001 From: Artur Jonkisz Date: Sat, 20 Jun 2026 19:16:31 +0100 Subject: [PATCH 10/22] Fix Roles model-control inheritance + precedence findings - Inline model clear/reset preserves an existing thinking override (no thinkingLevel:"" write); thinking is reset only by its own control. - Inherited-role rows surface the effective model/thinking from modelResolution even when top-level role fields are absent (no misleading "(use default)"); row state follows current-scope overrides. - Thinking-only edits no longer promote an inherited model into a fresh current-scope model override. - resolveRoleFieldSource respects PackResolver precedence: a winning market/global-user/project role field shadows lower server/builtin fields with the same role name in the metadata. Adds pinning tests for all four findings. Co-authored-by: bobbit-ai --- src/app/role-manager-page.ts | 74 +++++++++++---- src/server/agent/config-cascade.ts | 46 ++++++++- tests/config-cascade.test.ts | 55 +++++++++++ .../settings-admin-fixture.spec.ts | 93 ++++++++++++++++++- 4 files changed, 244 insertions(+), 24 deletions(-) diff --git a/src/app/role-manager-page.ts b/src/app/role-manager-page.ts index 152874c22..44d60ae1d 100644 --- a/src/app/role-manager-page.ts +++ b/src/app/role-manager-page.ts @@ -567,6 +567,35 @@ interface RoleFieldDisplay { badge: string; /** Resolved value text (model id / thinking label) for the source line. */ effectiveLabel: string; + /** + * Raw effective value (model id / thinking level) for the row's interactive + * picker — including inherited-role values that have no top-level + * `role.model` / `role.thinkingLevel`. Empty string when the field falls + * back to a pure default (so the picker renders its "(use default)" label). + */ + effectiveValue: string; +} + +/** + * The model/thinking values that are CURRENT-SCOPE overrides (the layer the + * inline list row would write to), as opposed to inherited/default values. + * + * Used both to derive the row `data-model-state` and to persist inline edits + * without accidentally promoting an inherited value into a fresh current-scope + * override (e.g. changing only thinking must not bake the inherited model in, + * and clearing the model must not wipe a thinking override). Prefers the + * server's `modelResolution` (source === "role" ⇒ current editable layer); + * falls back to the top-level fields for drafts that carry no metadata. + */ +function currentScopeRoleOverrides(role: RoleData): { model: string; thinkingLevel: string } { + const meta = role.modelResolution; + if (meta) { + return { + model: meta.model.source === "role" ? (meta.model.value ?? "") : "", + thinkingLevel: meta.thinkingLevel.source === "role" ? (meta.thinkingLevel.value ?? "") : "", + }; + } + return { model: role.model ?? "", thinkingLevel: role.thinkingLevel ?? "" }; } interface RoleModelDisplay { @@ -631,9 +660,12 @@ function computeRoleModelDisplay(role: RoleData): RoleModelDisplay { const value = m.value ?? ""; const effectiveLabel = value ? format(value) : emptyLabel; switch (m.source) { - case "role": return { override: true, kind: "role", badge: "Role override", effectiveLabel }; - case "inherited-role": return { override: true, kind: "inherited-role", badge: inheritedBadge(m), effectiveLabel }; - default: return { override: false, kind: defaultKind, badge: defaultBadge, effectiveLabel }; + // Inherited-role values have no top-level `role.model`/`role.thinkingLevel`, + // so `effectiveValue` is what surfaces the real model/thinking in the picker + // instead of a misleading "(use default)". + case "role": return { override: true, kind: "role", badge: "Role override", effectiveLabel, effectiveValue: value }; + case "inherited-role": return { override: true, kind: "inherited-role", badge: inheritedBadge(m), effectiveLabel, effectiveValue: value }; + default: return { override: false, kind: defaultKind, badge: defaultBadge, effectiveLabel, effectiveValue: "" }; } }; @@ -642,9 +674,9 @@ function computeRoleModelDisplay(role: RoleData): RoleModelDisplay { if (meta) { model = fromMeta(meta.model, (v) => formatModelPref(v), formatModelPref(def.model)); } else if (role.model) { - model = { override: true, kind: "role", badge: "Role override", effectiveLabel: formatModelPref(role.model) }; + model = { override: true, kind: "role", badge: "Role override", effectiveLabel: formatModelPref(role.model), effectiveValue: role.model }; } else { - model = { override: false, kind: defaultKind, badge: defaultBadge, effectiveLabel: formatModelPref(def.model) }; + model = { override: false, kind: defaultKind, badge: defaultBadge, effectiveLabel: formatModelPref(def.model), effectiveValue: "" }; } // THINKING @@ -653,15 +685,18 @@ function computeRoleModelDisplay(role: RoleData): RoleModelDisplay { if (meta) { thinking = fromMeta(meta.thinkingLevel, (v) => thinkingLevelLabel(v), defThinkingLabel); } else if (role.thinkingLevel) { - thinking = { override: true, kind: "role", badge: "Role override", effectiveLabel: thinkingLevelLabel(role.thinkingLevel) }; + thinking = { override: true, kind: "role", badge: "Role override", effectiveLabel: thinkingLevelLabel(role.thinkingLevel), effectiveValue: role.thinkingLevel }; } else { - thinking = { override: false, kind: defaultKind, badge: defaultBadge, effectiveLabel: defThinkingLabel }; + thinking = { override: false, kind: defaultKind, badge: defaultBadge, effectiveLabel: defThinkingLabel, effectiveValue: "" }; } + // State follows the CURRENT-SCOPE override (not the resolved winner): an + // inherited-role value must read as "inherited", not "override". + const overrides = currentScopeRoleOverrides(role); let state: RoleModelDisplay["state"]; if (packReadonly) state = "readonly"; - else if (role.model) state = "override"; - else if (role.thinkingLevel) state = "thinking-override"; + else if (overrides.model) state = "override"; + else if (overrides.thinkingLevel) state = "thinking-override"; else state = "inherited"; return { packReadonly, packName, model, thinking, state }; @@ -720,9 +755,9 @@ function renderRoleRowModelControl(role: RoleData, control: RoleRowModelControl) ${renderModelRow( "", "", - role.model ?? "", + display.model.effectiveValue, (v) => control.onModelChange(role, v), - role.thinkingLevel ?? "", + display.thinking.effectiveValue, (v) => control.onThinkingChange(role, v), "", { fallbackLabel: "(use default)" }, @@ -838,15 +873,18 @@ function renderListView(): TemplateResult { modelControl: { savedFlashRole: savedModelFlashRole, onModelChange: (role, model) => { - // Picking a model keeps any existing thinking override; the model - // clear button (X) resets the whole role override (model + thinking). - // Thinking can still be reset on its own via role-row-thinking-clear-btn. - void persistInlineModel(role, model, model ? (role.thinkingLevel ?? "") : ""); + // Model and thinking are independent. Setting OR clearing the model + // preserves any existing thinking override — clearing the model must + // never write thinkingLevel:"" (thinking is reset only by its own + // dedicated clear control, role-row-thinking-clear-btn). + void persistInlineModel(role, model, currentScopeRoleOverrides(role).thinkingLevel); }, onThinkingChange: (role, thinking) => { - // Thinking is editable/clearable independently of the model, so even - // inherited rows can gain (or drop) a thinking-only override. - void persistInlineModel(role, role.model ?? "", thinking); + // Thinking is editable/clearable independently of the model. Persist + // only the current-scope model OVERRIDE (empty when inherited) so a + // thinking-only change never promotes an inherited model into a fresh + // current-scope model override. + void persistInlineModel(role, currentScopeRoleOverrides(role).model, thinking); }, }, }); diff --git a/src/server/agent/config-cascade.ts b/src/server/agent/config-cascade.ts index c58df9ad4..0da97ac8d 100644 --- a/src/server/agent/config-cascade.ts +++ b/src/server/agent/config-cascade.ts @@ -298,6 +298,49 @@ export class ConfigCascade { packName: string | null, ): RoleFieldSource { const editable = !packManaged; + + // Rank a cascade band to mirror PackResolver precedence (low→high): + // builtin < server-market < server < global-user-market < global-user + // < project-market < project. Market packs sit just below their scope's + // user pack (design §3.2). + const bandRank = (scope: PackScope, isMarket: boolean): number => { + const base = { builtin: 0, server: 2, "global-user": 4, project: 6 }[scope]; + return base - (isMarket ? 1 : 0); + }; + // The directly-editable band for this view: the project's own local layer + // when scoped to a project, otherwise the server-level store. + const editableRank = projectId ? bandRank("project", false) : bandRank("server", false); + + // The whole-role winner is the highest band that *defines* the role. When + // it supplies the field, that value outranks any lower-band field carried + // by a server/builtin role with the same name (finding #4) — so a market / + // global-user / project role shadows the legacy plain-layer walk below. + if (entry) { + const wv = (entry.item as any)[field]; + if (typeof wv === "string" && wv.trim().length > 0) { + const origin = scopeToOrigin(entry.origin.scope); + const winnerRank = bandRank(entry.origin.scope, entry.origin.kind === "market"); + if (winnerRank >= editableRank) { + // At or above the editable band. Equal rank ⇒ the winner IS the + // editable layer (a direct, user-editable override). Higher rank + // ⇒ an effective value the user cannot edit at this scope. + const isEditableLayer = winnerRank === editableRank && entry.origin.kind !== "market"; + return { + value: wv, + source: isEditableLayer ? "role" : "inherited-role", + origin, + originPackName: packName, + editable, + sourceLabel: packName ?? originSourceLabel(origin), + }; + } + // winnerRank < editableRank: the editable layer does not define the + // field (else the winner would BE that band). Fall through to the + // field-level walk, which finds this value as a lower-band inherited + // override (or the pack fallback at the end). + } + } + if (projectId) { // Current editable layer: the project's own local override. const local = this.localRoleField(projectId, roleName, field); @@ -334,7 +377,8 @@ export class ConfigCascade { } } // Pack / winner fallback: covers fields that only exist on a role defined - // by a market pack (which the plain-layer walk above does not read). + // by a lower-precedence market pack (server-market / builtin-pack) which + // the plain-layer walk above does not read. if (entry) { const wv = (entry.item as any)[field]; if (typeof wv === "string" && wv.trim().length > 0) { diff --git a/tests/config-cascade.test.ts b/tests/config-cascade.test.ts index a97ca6fb8..0d2c15c19 100644 --- a/tests/config-cascade.test.ts +++ b/tests/config-cascade.test.ts @@ -187,6 +187,61 @@ describe("ConfigCascade — Role.model and Role.thinkingLevel three-layer resolu assert.equal(plain.thinkingLevel.value, undefined); }); + it("metadata reports the winning higher-precedence role (global-user) over a shadowed server field (finding #4)", () => { + // Builtin + server both define `coder` with a model; a global-user user-pack + // role also defines `coder` with a DIFFERENT model. The global-user band sits + // ABOVE server in PackResolver precedence, so it is the whole-role winner and + // its field must be reported — NOT the lower-precedence server model. + const builtinsDir = mkTemp(); + writeRoleYaml(builtinsDir, "coder", { model: "anthropic/claude-haiku", thinkingLevel: "low" }); + const builtins = new BuiltinConfigProvider(builtinsDir); + + const serverDir = mkTemp(); + const serverRoleStore = new RoleStore(serverDir); + serverRoleStore.put({ + name: "coder", label: "Coder", promptTemplate: "p", accessory: "none", + model: "anthropic/claude-sonnet", thinkingLevel: "medium", + createdAt: 0, updatedAt: 0, + }); + + // Global-user user pack lives under /.bobbit/config/roles/.yaml. + const homeDir = mkTemp(); + writeRoleYaml(path.join(homeDir, ".bobbit", "config"), "coder", { model: "anthropic/claude-opus-4" }); + + const serverStores = { + getRoles: () => serverRoleStore.getAllLocal(), + getPersonalities: () => [], + getWorkflows: () => [], + getTools: () => [], + getToolGroupPolicies: () => ({}), + }; + const fakePcm = { getOrCreate: () => undefined } as any; + + const cascade = new ConfigCascade(builtins, serverStores, fakePcm); + cascade.setGlobalUserBase(homeDir); + + // Sanity: the whole-role winner is the global-user pack, shadowing server/builtin. + const winner = cascade.resolveRoles().find(r => r.item.name === "coder"); + assert.ok(winner); + assert.equal(winner!.origin, "user"); + assert.equal(winner!.item.model, "anthropic/claude-opus-4"); + + // Metadata must follow that precedence for the model field: the global-user + // value wins; it is above the (system-scope) editable server layer, so it is + // reported as an inherited-role override, not the shadowed server model. + const meta = cascade.resolveRoleModelResolution("coder"); + assert.equal(meta.model.source, "inherited-role"); + assert.equal(meta.model.origin, "user"); + assert.equal(meta.model.value, "anthropic/claude-opus-4"); + assert.equal(meta.model.editable, true); + + // The global-user role omits thinkingLevel, so the field falls through the + // lower layers: server supplies it as the system-scope editable layer. + assert.equal(meta.thinkingLevel.source, "role"); + assert.equal(meta.thinkingLevel.origin, "server"); + assert.equal(meta.thinkingLevel.value, "medium"); + }); + it("falls through to builtin when neither server nor project define the role", () => { const builtinsDir = mkTemp(); writeRoleYaml(builtinsDir, "tester", { diff --git a/tests/ui-fixtures/settings-admin-fixture.spec.ts b/tests/ui-fixtures/settings-admin-fixture.spec.ts index a794a5423..db2904c9d 100644 --- a/tests/ui-fixtures/settings-admin-fixture.spec.ts +++ b/tests/ui-fixtures/settings-admin-fixture.spec.ts @@ -423,6 +423,78 @@ test.describe("Settings/admin UI fixture", () => { await expect(control.locator(LIST_MODEL_SOURCE)).toHaveText(/Server/i); }); + test("list view shows inherited-role effective model + thinking even when top-level fields are absent (finding #2)", async ({ page }) => { + // The role carries NO top-level model/thinkingLevel; the effective values + // live only in modelResolution (inherited from the server role layer). The + // row MUST surface those effective values in the picker, not "(use default)". + await resetFixture(page, { + prefs: { "default.sessionModel": SESSION_DEFAULT_MODEL }, + roles: [ + { + name: "coder", label: "Coder", promptTemplate: "You write code.", + accessory: "none", toolPolicies: {}, + createdAt: 1, updatedAt: 1, origin: "project", + modelResolution: { + model: { value: TEST_MODEL, source: "inherited-role", origin: "server", editable: true }, + thinkingLevel: { value: TEST_THINKING, source: "inherited-role", origin: "server", editable: true }, + }, + } as any, + ], + }); + await loadRoles(page, "#/roles"); + + const control = roleRow(page, "coder").locator(LIST_MODEL_CONTROL); + await expect(control).toBeVisible(); + // Current scope did not override → the row reads as inherited, NOT override. + await expect(control).toHaveAttribute("data-model-state", "inherited"); + // Effective model surfaces in the picker (no "(use default)" placeholder). + const modelPicker = control.locator("[data-testid='model-row'] button[title='Choose model']"); + await expect(modelPicker).toContainText("claude-opus-4-1"); + await expect(modelPicker).not.toContainText("(use default)"); + // Effective thinking surfaces in the thinking selector. + await expect(control.locator("[data-testid='model-row'] button[role='combobox']")).toContainText("High"); + // Both fields are badged as inherited-role overrides naming their source. + await expect(control.locator(LIST_MODEL_SOURCE)).toHaveText(SRC_INHERITED_ROLE); + await expect(control.locator(LIST_THINKING_SOURCE)).toHaveText(SRC_INHERITED_ROLE); + }); + + test("list view thinking-only change does not promote an inherited model into an override (finding #3)", async ({ page }) => { + // Model is inherited (no current-scope override). Changing only the thinking + // level must persist ONLY the thinking change — it must never bake the + // inherited model into a fresh current-scope model override. + await resetFixture(page, { + prefs: { "default.sessionModel": SESSION_DEFAULT_MODEL }, + roles: [ + { + name: "coder", label: "Coder", promptTemplate: "You write code.", + accessory: "none", toolPolicies: {}, + createdAt: 1, updatedAt: 1, origin: "project", + modelResolution: { + model: { value: TEST_MODEL, source: "inherited-role", origin: "server", editable: true }, + thinkingLevel: { value: "", source: "default", editable: true }, + }, + } as any, + ], + }); + await loadRoles(page, "#/roles"); + + const control = roleRow(page, "coder").locator(LIST_MODEL_CONTROL); + await expect(control).toHaveAttribute("data-model-state", "inherited"); + + // Change ONLY thinking via the inline thinking selector. + await control.locator("[data-testid='model-row'] button[role='combobox']").click(); + await page.getByRole("option", { name: "High", exact: true }).click(); + + // Only thinkingLevel is persisted; the inherited model is NOT written as an + // override. (The persisted store is the source of truth here — the fixture's + // GET does not recompute modelResolution the way the real server does, so we + // assert on the stored role, which pins the finding-#3 fix precisely.) + await expect.poll(async () => { + const c = await storedRole(page, "coder"); + return [c?.model ?? "", c?.thinkingLevel ?? ""]; + }).toEqual(["", TEST_THINKING]); + }); + test("list view inline model pick + thinking auto-saves and persists across reload", async ({ page }) => { await resetFixture(page, { prefs: { "default.sessionModel": SESSION_DEFAULT_MODEL } }); await loadRoles(page, "#/roles"); @@ -462,7 +534,7 @@ test.describe("Settings/admin UI fixture", () => { expect([after?.model ?? "", after?.thinkingLevel ?? ""]).toEqual([TEST_MODEL, TEST_THINKING]); }); - test("list view clearing an override reverts model + thinking to inherited", async ({ page }) => { + test("list view clearing the model preserves an existing thinking override (finding #1)", async ({ page }) => { await resetFixture(page, { prefs: { "default.sessionModel": SESSION_DEFAULT_MODEL }, roles: [ @@ -482,17 +554,28 @@ test.describe("Settings/admin UI fixture", () => { await clearBtn.click(); - // Both model and thinking revert to empty so the row inherits/defaults. + // Clearing the model must NOT write thinkingLevel:"" — the thinking + // override is preserved (it is reset only by its own control). await expect.poll(async () => { const c = await storedRole(page, "coder"); return [c?.model ?? "", c?.thinkingLevel ?? ""]; - }).toEqual(["", ""]); + }).toEqual(["", TEST_THINKING]); - // Row flips back to inherited display with a default source badge. - await expect(control).toHaveAttribute("data-model-state", "inherited"); + // Row drops to a first-class thinking-only override: model now inherits the + // session default, thinking is still a Role override with its own reset. + await expect(control).toHaveAttribute("data-model-state", /thinking-override|partial-override/); await expect(control).toContainText(SESSION_DEFAULT_LABEL); await expect(control.locator(LIST_MODEL_SOURCE)).toHaveText(SRC_SESSION_DEFAULT); + await expect(control.locator(LIST_THINKING_SOURCE)).toHaveText(SRC_ROLE_OVERRIDE); await expect(control.locator("[data-testid='model-clear-btn']")).toHaveCount(0); + + // The dedicated thinking reset then clears the remaining override → fully inherited. + await control.locator(LIST_THINKING_CLEAR).click(); + await expect.poll(async () => { + const c = await storedRole(page, "coder"); + return [c?.model ?? "", c?.thinkingLevel ?? ""]; + }).toEqual(["", ""]); + await expect(control).toHaveAttribute("data-model-state", "inherited"); }); test("list view thinking-only override is visible, editable, and clearable without a model override", async ({ page }) => { From c68ce4b6d296415e4cc547418494fd239216168b Mon Sep 17 00:00:00 2001 From: Artur Jonkisz Date: Sat, 20 Jun 2026 19:47:09 +0100 Subject: [PATCH 11/22] Fix role model-source winner precedence + inline save race Finding #1: resolveRoleFieldSource now reports the actual PackResolver whole-role winner whenever it supplies model/thinkingLevel, instead of falling through to the plain server/builtin walk for winners that sit below the editable band but above server/builtin (server-market, global-user, ancestor-project). The winnerRank>=editableRank guard only governs the role vs inherited-role label now, not whether the winner's value is used. Adds cascade tests for server-market, global-user (project scope), and project-scoped winners shadowing server/builtin. Finding #2: inline list-row model/thinking saves are no longer fire-and-forget. A per-role pending draft + serialized, coalescing save loop builds each PUT from the merged draft rather than a stale row snapshot, so picking a model then quickly changing thinking before the refetch no longer clobbers the just-selected model. Adds a deterministic save-race fixture test (slow PUT) that fails on the pre-fix code. Co-authored-by: bobbit-ai --- src/app/role-manager-page.ts | 78 +++++++- src/server/agent/config-cascade.ts | 64 +++---- tests/config-cascade.test.ts | 170 ++++++++++++++++++ .../settings-admin-fixture.spec.ts | 49 +++++ 4 files changed, 316 insertions(+), 45 deletions(-) diff --git a/src/app/role-manager-page.ts b/src/app/role-manager-page.ts index 44d60ae1d..335b163e7 100644 --- a/src/app/role-manager-page.ts +++ b/src/app/role-manager-page.ts @@ -73,6 +73,18 @@ let deleting = false; let savedModelFlashRole: string | null = null; let savedModelFlashTimer: ReturnType | null = null; +// Inline list-row save race guard. Inline model/thinking edits are auto-saved +// asynchronously, but a save round-trips through customize + PUT + a full +// refetch before the row objects refresh. If a user picks a model and then +// quickly changes thinking, the second handler would otherwise read the model +// off the STALE row object (still showing no override) and clobber the +// just-selected model with "". To prevent that we keep a per-role pending draft +// (the latest desired { model, thinkingLevel } for that scope) and a per-role +// serialized save loop that coalesces rapid edits into ordered PUTs built from +// the merged draft rather than from any stale row snapshot. +const pendingInlineEdits = new Map(); +const inlineSaveActive = new Set(); + // Group policies loaded from server let groupPolicies: Record = {}; @@ -316,6 +328,57 @@ async function persistInlineModel(role: RoleData, model: string, thinkingLevel: }, 1800); } +/** + * Merge a single inline field change into the role's pending draft and kick off + * (or coalesce into) its serialized save loop. The draft base is the role's + * existing pending draft when one is in flight, else the current-scope + * overrides off the freshly-rendered row — so the "model picked, then thinking + * changed before refetch" race never reads a stale model/thinking off the row. + */ +function queueInlineEdit(role: RoleData, patch: { model?: string; thinkingLevel?: string }): void { + const base = pendingInlineEdits.get(role.name) ?? currentScopeRoleOverrides(role); + pendingInlineEdits.set(role.name, { + model: patch.model ?? base.model, + thinkingLevel: patch.thinkingLevel ?? base.thinkingLevel, + }); + scheduleInlineSave(role.name); +} + +/** + * Per-role serialized save loop. Drains the pending draft for `name` one PUT at + * a time; a newer edit that arrives mid-save is picked up on the next loop turn + * rather than racing the in-flight save. Always builds the PUT from the latest + * fetched row object (so origin/customize state is fresh) merged with the + * pending model/thinking draft. + */ +function scheduleInlineSave(name: string): void { + if (inlineSaveActive.has(name)) return; // a loop is already draining this role + inlineSaveActive.add(name); + void (async () => { + try { + let desired = pendingInlineEdits.get(name); + while (desired) { + // Use the freshest row object so a save that already promoted the + // role into the project layer is not re-customized, and label/prompt/ + // accessory/tools reflect the latest fetch. + const current = roles.find(r => r.name === name); + if (!current) { pendingInlineEdits.delete(name); break; } + await persistInlineModel(current, desired.model, desired.thinkingLevel); + const after = pendingInlineEdits.get(name); + if (after && after.model === desired.model && after.thinkingLevel === desired.thinkingLevel) { + // No newer edit arrived during the save — drain complete. + pendingInlineEdits.delete(name); + desired = undefined; + } else { + desired = after; // a newer edit landed mid-save; persist it next. + } + } + } finally { + inlineSaveActive.delete(name); + } + })(); +} + async function handleDelete(): Promise { if (!selectedRole) return; const confirmed = await confirmAction( @@ -876,15 +939,18 @@ function renderListView(): TemplateResult { // Model and thinking are independent. Setting OR clearing the model // preserves any existing thinking override — clearing the model must // never write thinkingLevel:"" (thinking is reset only by its own - // dedicated clear control, role-row-thinking-clear-btn). - void persistInlineModel(role, model, currentScopeRoleOverrides(role).thinkingLevel); + // dedicated clear control, role-row-thinking-clear-btn). The pending + // draft (not the stale row) supplies the preserved thinking value so a + // rapid follow-up thinking edit cannot clobber this model choice. + queueInlineEdit(role, { model }); }, onThinkingChange: (role, thinking) => { - // Thinking is editable/clearable independently of the model. Persist - // only the current-scope model OVERRIDE (empty when inherited) so a + // Thinking is editable/clearable independently of the model. The pending + // draft supplies the preserved current-scope model OVERRIDE so a // thinking-only change never promotes an inherited model into a fresh - // current-scope model override. - void persistInlineModel(role, currentScopeRoleOverrides(role).model, thinking); + // current-scope model override — and a model picked moments earlier + // (still mid-save) is preserved instead of being overwritten with "". + queueInlineEdit(role, { thinkingLevel: thinking }); }, }, }); diff --git a/src/server/agent/config-cascade.ts b/src/server/agent/config-cascade.ts index 0da97ac8d..b4a9d01dc 100644 --- a/src/server/agent/config-cascade.ts +++ b/src/server/agent/config-cascade.ts @@ -311,34 +311,37 @@ export class ConfigCascade { // when scoped to a project, otherwise the server-level store. const editableRank = projectId ? bandRank("project", false) : bandRank("server", false); - // The whole-role winner is the highest band that *defines* the role. When - // it supplies the field, that value outranks any lower-band field carried - // by a server/builtin role with the same name (finding #4) — so a market / - // global-user / project role shadows the legacy plain-layer walk below. + // The whole-role PackResolver winner is the effective role at this scope. + // Whenever it *supplies* the field, that value/source IS the effective + // metadata and must outrank the plain server/builtin field walk below — + // even for market / global-user / ancestor-project winners that sit BELOW + // the current editable band but ABOVE server/builtin. The previous + // `winnerRank >= editableRank` guard let those lower-but-still-winning + // bands fall through and report a shadowed server/builtin field instead + // of the real winner (finding #1). if (entry) { const wv = (entry.item as any)[field]; if (typeof wv === "string" && wv.trim().length > 0) { const origin = scopeToOrigin(entry.origin.scope); const winnerRank = bandRank(entry.origin.scope, entry.origin.kind === "market"); - if (winnerRank >= editableRank) { - // At or above the editable band. Equal rank ⇒ the winner IS the - // editable layer (a direct, user-editable override). Higher rank - // ⇒ an effective value the user cannot edit at this scope. - const isEditableLayer = winnerRank === editableRank && entry.origin.kind !== "market"; - return { - value: wv, - source: isEditableLayer ? "role" : "inherited-role", - origin, - originPackName: packName, - editable, - sourceLabel: packName ?? originSourceLabel(origin), - }; - } - // winnerRank < editableRank: the editable layer does not define the - // field (else the winner would BE that band). Fall through to the - // field-level walk, which finds this value as a lower-band inherited - // override (or the pack fallback at the end). + // Equal rank with a non-market winner ⇒ the winner IS the directly + // editable layer (a user-authored override at this scope). Anything + // else (higher, lower, or a market pack at the same rank) is an + // effective value inherited from another band the user cannot edit + // in place at this scope. + const isEditableLayer = winnerRank === editableRank && entry.origin.kind !== "market"; + return { + value: wv, + source: isEditableLayer ? "role" : "inherited-role", + origin, + originPackName: packName, + editable, + sourceLabel: packName ?? originSourceLabel(origin), + }; } + // Winner role does not define the field ⇒ the runtime field-merge + // (resolveRoleField) pulls it from a lower plain layer. Fall through to + // the field-level walk below, which mirrors that field-merge order. } if (projectId) { @@ -376,23 +379,6 @@ export class ConfigCascade { return { value: bv, source: "inherited-role", origin: "builtin", editable, sourceLabel: "Built-in" }; } } - // Pack / winner fallback: covers fields that only exist on a role defined - // by a lower-precedence market pack (server-market / builtin-pack) which - // the plain-layer walk above does not read. - if (entry) { - const wv = (entry.item as any)[field]; - if (typeof wv === "string" && wv.trim().length > 0) { - const origin = scopeToOrigin(entry.origin.scope); - return { - value: wv, - source: "inherited-role", - origin, - originPackName: packName, - editable, - sourceLabel: packName ?? originSourceLabel(origin), - }; - } - } // No role layer supplies the field — the client renders the session/review // (or auto) default for this label. return { source: "default", editable, sourceLabel: "Default" }; diff --git a/tests/config-cascade.test.ts b/tests/config-cascade.test.ts index 0d2c15c19..81d34a310 100644 --- a/tests/config-cascade.test.ts +++ b/tests/config-cascade.test.ts @@ -242,6 +242,176 @@ describe("ConfigCascade — Role.model and Role.thinkingLevel three-layer resolu assert.equal(meta.thinkingLevel.value, "medium"); }); + it("metadata reports a server-market winner over a shadowed builtin field at system scope (finding #1)", () => { + // builtin defines `coder` with a model + thinkingLevel; the server store has + // NO coder, so a server-market pack role is the whole-role winner. The + // server-market band (rank 1) sits BELOW the system-scope editable server + // band (rank 2) but ABOVE builtin (rank 0). The previous guard only trusted + // the winner when winnerRank >= editableRank, so it fell through and reported + // the lower builtin model. The winner's model must be reported instead. + const builtinsDir = mkTemp(); + writeRoleYaml(builtinsDir, "coder", { model: "anthropic/claude-haiku", thinkingLevel: "low" }); + const builtins = new BuiltinConfigProvider(builtinsDir); + + const serverStores = { + getRoles: () => [], // server store does NOT define coder + getPersonalities: () => [], + getWorkflows: () => [], + getTools: () => [], + getToolGroupPolicies: () => ({}), + }; + const fakePcm = { getOrCreate: () => undefined } as any; + + const marketRole = { + name: "coder", label: "Coder", promptTemplate: "p", accessory: "none", + model: "anthropic/claude-opus-4", createdAt: 0, updatedAt: 0, + }; + const marketProvider = { + marketEntries: (scope: string) => scope === "server" + ? [{ + id: "market:server:mk", kind: "market", scope: "server", path: "/virtual/mk", + readOnly: true, layout: "defaults-tree", + manifest: { name: "mk", description: "", version: "1", contents: { roles: ["coder"], tools: [], skills: [], entrypoints: [] } }, + preloaded: { roles: [{ name: "coder", item: marketRole }] }, + }] + : [], + } as any; + + const cascade = new ConfigCascade(builtins, serverStores, fakePcm, undefined, marketProvider); + + // Sanity: the server-market pack is the whole-role winner. + const winner = cascade.resolveRoles().find(r => r.item.name === "coder"); + assert.ok(winner); + assert.equal(winner!.origin, "server"); + assert.equal(winner!.originPackName, "mk"); + assert.equal(winner!.item.model, "anthropic/claude-opus-4"); + + const meta = cascade.resolveRoleModelResolution("coder"); + // Model: the winning server-market value, NOT the shadowed builtin haiku. + assert.equal(meta.model.source, "inherited-role"); + assert.equal(meta.model.origin, "server"); + assert.equal(meta.model.value, "anthropic/claude-opus-4"); + assert.equal(meta.model.originPackName, "mk"); + assert.equal(meta.model.sourceLabel, "mk"); + // Pack-managed → read-only inline. + assert.equal(meta.model.editable, false); + // thinkingLevel: market role omits it → falls through to the builtin field. + assert.equal(meta.thinkingLevel.source, "inherited-role"); + assert.equal(meta.thinkingLevel.origin, "builtin"); + assert.equal(meta.thinkingLevel.value, "low"); + assert.equal(meta.thinkingLevel.editable, false); + }); + + it("metadata reports a global-user winner over a shadowed server field at PROJECT scope (finding #1)", () => { + // builtin + server both define `coder` model; a global-user user-pack role + // also defines a DIFFERENT model. At PROJECT scope the project layer has no + // coder override, so the global-user role (rank 4) is the whole-role winner — + // BELOW the project editable band (rank 6) but ABOVE the server band (rank 2). + // The previous guard fell through and reported the shadowed server model; the + // global-user model must be reported instead, and it stays editable (user + // pack, not a market pack). + const builtinsDir = mkTemp(); + writeRoleYaml(builtinsDir, "coder", { model: "anthropic/claude-haiku", thinkingLevel: "low" }); + const builtins = new BuiltinConfigProvider(builtinsDir); + + const serverDir = mkTemp(); + const serverRoleStore = new RoleStore(serverDir); + serverRoleStore.put({ + name: "coder", label: "Coder", promptTemplate: "p", accessory: "none", + model: "anthropic/claude-sonnet", thinkingLevel: "medium", + createdAt: 0, updatedAt: 0, + }); + + const homeDir = mkTemp(); + writeRoleYaml(path.join(homeDir, ".bobbit", "config"), "coder", { model: "anthropic/claude-opus-4" }); + + const projectDir = mkTemp(); + const projectRoleStore = new RoleStore(projectDir); // empty: no coder override + + const serverStores = { + getRoles: () => serverRoleStore.getAllLocal(), + getPersonalities: () => [], + getWorkflows: () => [], + getTools: () => [], + getToolGroupPolicies: () => ({}), + }; + const fakePcm = { + getOrCreate: (id: string) => id === "proj1" ? { roleStore: projectRoleStore } : undefined, + } as any; + + const cascade = new ConfigCascade(builtins, serverStores, fakePcm); + cascade.setGlobalUserBase(homeDir); + + // Sanity: at project scope the global-user role is the winner. + const winner = cascade.resolveRoles("proj1").find(r => r.item.name === "coder"); + assert.ok(winner); + assert.equal(winner!.origin, "user"); + assert.equal(winner!.item.model, "anthropic/claude-opus-4"); + + const meta = cascade.resolveRoleModelResolution("coder", "proj1"); + // Model: the winning global-user value, NOT the shadowed server sonnet. + assert.equal(meta.model.source, "inherited-role"); + assert.equal(meta.model.origin, "user"); + assert.equal(meta.model.value, "anthropic/claude-opus-4"); + assert.equal(meta.model.editable, true); + // thinkingLevel: global-user role omits it → server supplies the value. + assert.equal(meta.thinkingLevel.source, "inherited-role"); + assert.equal(meta.thinkingLevel.origin, "server"); + assert.equal(meta.thinkingLevel.value, "medium"); + assert.equal(meta.thinkingLevel.editable, true); + }); + + it("metadata reports a project-scoped winner over a shadowed server field as an editable role override (finding #1)", () => { + // The project layer defines `coder` model; server defines a different model. + // At project scope the project role is the winner AND the editable layer, so + // the field reads as a direct `role` override (editable), never the shadowed + // server value. + const builtinsDir = mkTemp(); + writeRoleYaml(builtinsDir, "coder", { model: "anthropic/claude-haiku", thinkingLevel: "low" }); + const builtins = new BuiltinConfigProvider(builtinsDir); + + const serverDir = mkTemp(); + const serverRoleStore = new RoleStore(serverDir); + serverRoleStore.put({ + name: "coder", label: "Coder", promptTemplate: "p", accessory: "none", + model: "anthropic/claude-sonnet", thinkingLevel: "medium", + createdAt: 0, updatedAt: 0, + }); + + const projectDir = mkTemp(); + const projectRoleStore = new RoleStore(projectDir); + projectRoleStore.put({ + name: "coder", label: "Coder", promptTemplate: "p", accessory: "none", + model: "anthropic/claude-opus-4", + createdAt: 0, updatedAt: 0, + }); + + const serverStores = { + getRoles: () => serverRoleStore.getAllLocal(), + getPersonalities: () => [], + getWorkflows: () => [], + getTools: () => [], + getToolGroupPolicies: () => ({}), + }; + const fakePcm = { + getOrCreate: (id: string) => id === "proj1" ? { roleStore: projectRoleStore } : undefined, + } as any; + + const cascade = new ConfigCascade(builtins, serverStores, fakePcm); + + const meta = cascade.resolveRoleModelResolution("coder", "proj1"); + // Model: project override wins, reported as an editable role override. + assert.equal(meta.model.source, "role"); + assert.equal(meta.model.origin, "project"); + assert.equal(meta.model.value, "anthropic/claude-opus-4"); + assert.equal(meta.model.editable, true); + assert.equal(meta.model.sourceLabel, "Project"); + // thinkingLevel: project role omits it → falls through to the server field. + assert.equal(meta.thinkingLevel.source, "inherited-role"); + assert.equal(meta.thinkingLevel.origin, "server"); + assert.equal(meta.thinkingLevel.value, "medium"); + }); + it("falls through to builtin when neither server nor project define the role", () => { const builtinsDir = mkTemp(); writeRoleYaml(builtinsDir, "tester", { diff --git a/tests/ui-fixtures/settings-admin-fixture.spec.ts b/tests/ui-fixtures/settings-admin-fixture.spec.ts index db2904c9d..4a7d1f27d 100644 --- a/tests/ui-fixtures/settings-admin-fixture.spec.ts +++ b/tests/ui-fixtures/settings-admin-fixture.spec.ts @@ -534,6 +534,55 @@ test.describe("Settings/admin UI fixture", () => { expect([after?.model ?? "", after?.thinkingLevel ?? ""]).toEqual([TEST_MODEL, TEST_THINKING]); }); + test("list view rapid model-then-thinking edit does not clobber the just-picked model (save race, finding #2)", async ({ page }) => { + // Repro for the inline save race: a user picks a model and then changes + // thinking BEFORE the model save round-trips (customize + PUT + refetch). The + // old fire-and-forget handler built the second PUT from the stale row object + // (model still empty), clearing the model just selected. The pending-draft + // merge + serialized save loop must preserve the model. + await resetFixture(page, { prefs: { "default.sessionModel": SESSION_DEFAULT_MODEL } }); + await loadRoles(page, "#/roles"); + + // Slow the role PUTs so the thinking change is dispatched while the model + // save is still in flight (the refetch that refreshes the row has NOT run). + // Wrapping window.fetch keeps the fixture's mock intact and avoids the + // network layer (the app calls window.fetch directly, not page.route). + await page.evaluate(() => { + const orig = window.fetch; + window.fetch = (async (input: RequestInfo | URL, init?: RequestInit) => { + const method = (init?.method || "GET").toUpperCase(); + const url = typeof input === "string" ? input : (input as any).url ?? String(input); + if (method === "PUT" && /\/api\/roles\//.test(url)) { + await new Promise((r) => setTimeout(r, 400)); + } + return orig(input, init); + }) as typeof window.fetch; + }); + + const control = roleRow(page, "coder").locator(LIST_MODEL_CONTROL); + await expect(control).toHaveAttribute("data-model-state", "inherited"); + + // Pick a model inline — this kicks off the (now slow) save. + await control.locator("[data-testid='model-row'] button[title='Choose model']").click(); + await page.locator(`agent-model-selector [data-model-id="claude-opus-4-1"]`).dispatchEvent("click"); + + // Immediately change thinking, WITHOUT waiting for the model save to settle. + await control.locator("[data-testid='model-row'] button[role='combobox']").click(); + await page.getByRole("option", { name: "High", exact: true }).click(); + + // Both edits must land: the model is preserved (not clobbered with "") and + // the thinking override is applied. On the pre-fix code the model ends "". + await expect.poll(async () => { + const c = await storedRole(page, "coder"); + return [c?.model ?? "", c?.thinkingLevel ?? ""]; + }).toEqual([TEST_MODEL, TEST_THINKING]); + + // And the row reflects the override end-state. + await expect(control).toHaveAttribute("data-model-state", "override"); + await expect(control.locator(LIST_MODEL_SOURCE)).toHaveText(SRC_ROLE_OVERRIDE); + await expect(control.locator(LIST_THINKING_SOURCE)).toHaveText(SRC_ROLE_OVERRIDE); + }); + test("list view clearing the model preserves an existing thinking override (finding #1)", async ({ page }) => { await resetFixture(page, { prefs: { "default.sessionModel": SESSION_DEFAULT_MODEL }, From b35e8972ccabf3abf6e948b32fb2c4f307bdec55 Mon Sep 17 00:00:00 2001 From: Artur Jonkisz Date: Sat, 20 Jun 2026 19:57:58 +0100 Subject: [PATCH 12/22] test(lifecycle-hub): stabilize provider dispatch flake under load Raise the fixture default per-provider timeout from 1000ms to 8000ms so ordinary fast providers are not killed by worker startup/contention during the full suite. Relax the timeout-path elapsed assertion from <1000ms to <4000ms so it still proves dispatch returns well before the 5000ms hang and records a timeout, without flaking on loaded CI. Intent preserved: the slow provider keeps a bounded 200ms budget and still times out. Co-authored-by: bobbit-ai --- tests/lifecycle-hub.test.ts | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/tests/lifecycle-hub.test.ts b/tests/lifecycle-hub.test.ts index c725a2abe..d2ab5bce4 100644 --- a/tests/lifecycle-hub.test.ts +++ b/tests/lifecycle-hub.test.ts @@ -25,7 +25,10 @@ function fixtureProvider(tmp: string, id: string, body: string, budget: { maxTok kind: "memory", module: path.basename(file), hooks: ["sessionSetup"], - budget: { maxTokens: budget.maxTokens ?? 400, timeoutMs: budget.timeoutMs ?? 1_000 }, + // Default to a generous per-provider timeout so ordinary fast fixtures are not + // killed by worker startup/contention under the full suite. Tests that exercise + // the timeout path pass an explicit, small timeoutMs. + budget: { maxTokens: budget.maxTokens ?? 400, timeoutMs: budget.timeoutMs ?? 8_000 }, config: { enabled: true }, listName: id, sourceFile: path.join(tmp, "pack.yaml"), @@ -82,7 +85,11 @@ describe("LifecycleHub", () => { const result = await hub(tmp, [slow, fast], moduleHost).dispatch("sessionSetup", base(tmp)); const elapsed = performance.now() - t0; - assert.ok(elapsed < 1_000, `dispatch should return promptly, got ${elapsed}ms`); + // The slow provider hangs for 5000ms but has a 200ms per-provider budget. Dispatch + // must abort it and return well before the hang completes, even under loaded + // full-suite/CI contention. Assert comfortably below the 5000ms hang rather than a + // tight sub-second bound that flakes on worker startup. + assert.ok(elapsed < 4_000, `dispatch should return well before the 5000ms hang, got ${elapsed}ms`); assert.equal(result.blocks.length, 1); assert.equal(result.blocks[0].providerId, "fast"); assert.equal(result.diagnostics.length, 1); From c158164702bb56550a477ee0f0452e9a8bba3c86 Mon Sep 17 00:00:00 2001 From: Artur Jonkisz Date: Sat, 20 Jun 2026 20:19:52 +0100 Subject: [PATCH 13/22] fix(roles): stop render-time autosave of inherited model/thinking + correct edit affordances Address Roles model-control review findings: - #1 (HIGH): feed only current-scope override values into the inline list picker; inherited/default effective values are shown read-only in the source line. Prevents renderModelRow's render-time thinking clamp from auto-saving a spurious local override just by rendering the list. - #2: gate model-clear / thinking-reset on source === 'role' (current-scope override) only; inherited-role fields no longer show no-op clear controls. - #3: in config-cascade, a field winner ABOVE the editable band (e.g. global-user while editing the server layer) is reported editable:false, since the server-layer PUT cannot override it. Pinning tests: inherited unsupported thinking does not clamp/persist on render; inherited-role values expose no clear/reset; system-scope global-user winner is read-only at the server save target. Co-authored-by: bobbit-ai --- src/app/role-manager-page.ts | 60 +++++++++++------- src/server/agent/config-cascade.ts | 11 +++- tests/config-cascade.test.ts | 53 +++++++++++++++- .../settings-admin-fixture.spec.ts | 63 ++++++++++++++++--- 4 files changed, 154 insertions(+), 33 deletions(-) diff --git a/src/app/role-manager-page.ts b/src/app/role-manager-page.ts index 335b163e7..e037b9802 100644 --- a/src/app/role-manager-page.ts +++ b/src/app/role-manager-page.ts @@ -623,18 +623,24 @@ const THINKING_LEVEL_LABELS: Record = { type RoleFieldSourceUiKind = "role" | "inherited-role" | "default" | "auto"; interface RoleFieldDisplay { - /** True when the value comes from a role-level override (current or inherited role). */ - override: boolean; + /** + * Source kind. `"role"` (current-scope override) is the ONLY kind that feeds + * the editable picker and exposes clear/reset; everything else displays the + * effective value + badge read-only (findings #1, #2). + */ kind: RoleFieldSourceUiKind; /** Compact source badge text, e.g. "Role override", "Session default". */ badge: string; /** Resolved value text (model id / thinking label) for the source line. */ effectiveLabel: string; /** - * Raw effective value (model id / thinking level) for the row's interactive - * picker — including inherited-role values that have no top-level - * `role.model` / `role.thinkingLevel`. Empty string when the field falls - * back to a pure default (so the picker renders its "(use default)" label). + * Raw effective value (model id / thinking level) used ONLY for the + * read-only source-line label (incl. inherited-role / default values). + * + * It is deliberately NOT fed into the editable picker: doing so let the + * shared `renderModelRow` clamp an inherited/unsupported thinking value at + * render time and auto-persist it as a spurious current-scope override + * (review finding #1). The picker is fed only `currentScopeRoleOverrides`. */ effectiveValue: string; } @@ -723,12 +729,12 @@ function computeRoleModelDisplay(role: RoleData): RoleModelDisplay { const value = m.value ?? ""; const effectiveLabel = value ? format(value) : emptyLabel; switch (m.source) { - // Inherited-role values have no top-level `role.model`/`role.thinkingLevel`, - // so `effectiveValue` is what surfaces the real model/thinking in the picker - // instead of a misleading "(use default)". - case "role": return { override: true, kind: "role", badge: "Role override", effectiveLabel, effectiveValue: value }; - case "inherited-role": return { override: true, kind: "inherited-role", badge: inheritedBadge(m), effectiveLabel, effectiveValue: value }; - default: return { override: false, kind: defaultKind, badge: defaultBadge, effectiveLabel, effectiveValue: "" }; + // `effectiveValue` only feeds the read-only source-line label; the + // editable picker is fed `currentScopeRoleOverrides` so inherited + // values are never clamped/auto-saved (finding #1). + case "role": return { kind: "role", badge: "Role override", effectiveLabel, effectiveValue: value }; + case "inherited-role": return { kind: "inherited-role", badge: inheritedBadge(m), effectiveLabel, effectiveValue: value }; + default: return { kind: defaultKind, badge: defaultBadge, effectiveLabel, effectiveValue: "" }; } }; @@ -737,9 +743,9 @@ function computeRoleModelDisplay(role: RoleData): RoleModelDisplay { if (meta) { model = fromMeta(meta.model, (v) => formatModelPref(v), formatModelPref(def.model)); } else if (role.model) { - model = { override: true, kind: "role", badge: "Role override", effectiveLabel: formatModelPref(role.model), effectiveValue: role.model }; + model = { kind: "role", badge: "Role override", effectiveLabel: formatModelPref(role.model), effectiveValue: role.model }; } else { - model = { override: false, kind: defaultKind, badge: defaultBadge, effectiveLabel: formatModelPref(def.model), effectiveValue: "" }; + model = { kind: defaultKind, badge: defaultBadge, effectiveLabel: formatModelPref(def.model), effectiveValue: "" }; } // THINKING @@ -748,9 +754,9 @@ function computeRoleModelDisplay(role: RoleData): RoleModelDisplay { if (meta) { thinking = fromMeta(meta.thinkingLevel, (v) => thinkingLevelLabel(v), defThinkingLabel); } else if (role.thinkingLevel) { - thinking = { override: true, kind: "role", badge: "Role override", effectiveLabel: thinkingLevelLabel(role.thinkingLevel), effectiveValue: role.thinkingLevel }; + thinking = { kind: "role", badge: "Role override", effectiveLabel: thinkingLevelLabel(role.thinkingLevel), effectiveValue: role.thinkingLevel }; } else { - thinking = { override: false, kind: defaultKind, badge: defaultBadge, effectiveLabel: defThinkingLabel, effectiveValue: "" }; + thinking = { kind: defaultKind, badge: defaultBadge, effectiveLabel: defThinkingLabel, effectiveValue: "" }; } // State follows the CURRENT-SCOPE override (not the resolved winner): an @@ -797,16 +803,28 @@ function renderRoleRowModelControl(role: RoleData, control: RoleRowModelControl) `; } - const sourceLine = (key: string, f: RoleFieldDisplay, testid: string, clearTestid?: string) => html` + // Only a CURRENT-SCOPE role override (source === "role") gets the picker's + // effective value and the clear/reset affordances. Inherited-role and + // default fields show their effective value + source in the read-only + // source line, but the picker is fed the empty current-scope override (so it + // renders "(use default)" and never clamps/auto-saves an inherited value), + // and no model-clear / thinking-reset control is exposed for them — clearing + // an empty local field would be a confusing no-op (review findings #1, #2). + const overrides = currentScopeRoleOverrides(role); + + const sourceLine = (key: string, f: RoleFieldDisplay, testid: string, clearTestid?: string) => { + const currentOverride = f.kind === "role"; + return html` ${key} - ${f.override ? nothing : html`${f.effectiveLabel}`} + ${currentOverride ? nothing : html`${f.effectiveLabel}`} ${f.badge} - ${clearTestid && f.override ? html`` : nothing} `; + }; return html`
control.onModelChange(role, v), - display.thinking.effectiveValue, + overrides.thinkingLevel, (v) => control.onThinkingChange(role, v), "", { fallbackLabel: "(use default)" }, diff --git a/src/server/agent/config-cascade.ts b/src/server/agent/config-cascade.ts index b4a9d01dc..78c6433f3 100644 --- a/src/server/agent/config-cascade.ts +++ b/src/server/agent/config-cascade.ts @@ -330,12 +330,21 @@ export class ConfigCascade { // effective value inherited from another band the user cannot edit // in place at this scope. const isEditableLayer = winnerRank === editableRank && entry.origin.kind !== "market"; + // A winner ABOVE the editable band shadows any write to the editable + // layer: e.g. a `global-user` override while editing the server layer. + // `PUT /api/roles/:name` writes the server (or project) layer, which + // cannot override a higher-precedence winner, so the field is NOT + // editable in place at this scope — reporting it editable would make + // the inline control appear to no-op (review finding #3). Lower + // winners (builtin under server, server under project) remain editable + // because the editable-layer write DOES shadow them. + const shadowsEditableLayer = winnerRank > editableRank; return { value: wv, source: isEditableLayer ? "role" : "inherited-role", origin, originPackName: packName, - editable, + editable: editable && !shadowsEditableLayer, sourceLabel: packName ?? originSourceLabel(origin), }; } diff --git a/tests/config-cascade.test.ts b/tests/config-cascade.test.ts index 81d34a310..5eb43efaf 100644 --- a/tests/config-cascade.test.ts +++ b/tests/config-cascade.test.ts @@ -233,13 +233,62 @@ describe("ConfigCascade — Role.model and Role.thinkingLevel three-layer resolu assert.equal(meta.model.source, "inherited-role"); assert.equal(meta.model.origin, "user"); assert.equal(meta.model.value, "anthropic/claude-opus-4"); - assert.equal(meta.model.editable, true); + // The global-user winner sits ABOVE the system-scope editable server layer. + // `PUT /api/roles/:name` writes the server layer, which cannot override that + // higher-precedence value, so the model field must be reported read-only — + // presenting it as editable would make the inline control a no-op (finding #3). + assert.equal(meta.model.editable, false); // The global-user role omits thinkingLevel, so the field falls through the - // lower layers: server supplies it as the system-scope editable layer. + // lower layers: server supplies it as the system-scope editable layer, so it + // IS a directly-editable role override (the write target owns it). assert.equal(meta.thinkingLevel.source, "role"); assert.equal(meta.thinkingLevel.origin, "server"); assert.equal(meta.thinkingLevel.value, "medium"); + assert.equal(meta.thinkingLevel.editable, true); + }); + + it("marks an above-current-scope winner (global-user at system scope) read-only, but a below-scope winner (builtin) editable (finding #3)", () => { + // `coder` is defined at builtin (model haiku) and global-user (model opus). + // At SYSTEM scope the editable write target is the server layer (rank 2). + // - global-user (rank 4) > server: a server-layer PUT cannot override it → + // model must be read-only (editable:false), else the inline control no-ops. + // - builtin (rank 0) < server: a server-layer PUT DOES shadow it, so a + // builtin-sourced field stays editable (customize-then-edit is valid). + const builtinsDir = mkTemp(); + writeRoleYaml(builtinsDir, "coder", { model: "anthropic/claude-haiku", thinkingLevel: "low" }); + // `solo` is defined ONLY at builtin (no higher layer shadows it). + writeRoleYaml(builtinsDir, "solo", { model: "anthropic/claude-haiku" }); + const builtins = new BuiltinConfigProvider(builtinsDir); + + const homeDir = mkTemp(); + writeRoleYaml(path.join(homeDir, ".bobbit", "config"), "coder", { model: "anthropic/claude-opus-4" }); + + const serverStores = { + getRoles: () => [], // server defines neither role + getPersonalities: () => [], + getWorkflows: () => [], + getTools: () => [], + getToolGroupPolicies: () => ({}), + }; + const fakePcm = { getOrCreate: () => undefined } as any; + + const cascade = new ConfigCascade(builtins, serverStores, fakePcm); + cascade.setGlobalUserBase(homeDir); + + // global-user winner above the server write target → read-only model field. + const coder = cascade.resolveRoleModelResolution("coder"); + assert.equal(coder.model.source, "inherited-role"); + assert.equal(coder.model.origin, "user"); + assert.equal(coder.model.value, "anthropic/claude-opus-4"); + assert.equal(coder.model.editable, false); + + // builtin winner below the server write target → still editable. + const solo = cascade.resolveRoleModelResolution("solo"); + assert.equal(solo.model.source, "inherited-role"); + assert.equal(solo.model.origin, "builtin"); + assert.equal(solo.model.value, "anthropic/claude-haiku"); + assert.equal(solo.model.editable, true); }); it("metadata reports a server-market winner over a shadowed builtin field at system scope (finding #1)", () => { diff --git a/tests/ui-fixtures/settings-admin-fixture.spec.ts b/tests/ui-fixtures/settings-admin-fixture.spec.ts index 4a7d1f27d..fc54226ac 100644 --- a/tests/ui-fixtures/settings-admin-fixture.spec.ts +++ b/tests/ui-fixtures/settings-admin-fixture.spec.ts @@ -423,10 +423,15 @@ test.describe("Settings/admin UI fixture", () => { await expect(control.locator(LIST_MODEL_SOURCE)).toHaveText(/Server/i); }); - test("list view shows inherited-role effective model + thinking even when top-level fields are absent (finding #2)", async ({ page }) => { + test("list view shows inherited-role effective model + thinking in the source line, not the editable picker, with no clear/reset (findings #1, #2)", async ({ page }) => { // The role carries NO top-level model/thinkingLevel; the effective values - // live only in modelResolution (inherited from the server role layer). The - // row MUST surface those effective values in the picker, not "(use default)". + // live only in modelResolution (inherited from the server role layer). + // - finding #1: inherited values must NOT be fed into the editable picker + // (that lets renderModelRow clamp + auto-persist them as a spurious local + // override on render). The picker shows the inherit placeholder; the + // effective values are displayed in the read-only source line instead. + // - finding #2: inherited-role fields expose NO model-clear / thinking-reset + // affordances (clearing an empty local field would be a confusing no-op). await resetFixture(page, { prefs: { "default.sessionModel": SESSION_DEFAULT_MODEL }, roles: [ @@ -447,15 +452,55 @@ test.describe("Settings/admin UI fixture", () => { await expect(control).toBeVisible(); // Current scope did not override → the row reads as inherited, NOT override. await expect(control).toHaveAttribute("data-model-state", "inherited"); - // Effective model surfaces in the picker (no "(use default)" placeholder). + // finding #1: picker shows the inherit placeholder, never the inherited value. const modelPicker = control.locator("[data-testid='model-row'] button[title='Choose model']"); - await expect(modelPicker).toContainText("claude-opus-4-1"); - await expect(modelPicker).not.toContainText("(use default)"); - // Effective thinking surfaces in the thinking selector. - await expect(control.locator("[data-testid='model-row'] button[role='combobox']")).toContainText("High"); - // Both fields are badged as inherited-role overrides naming their source. + await expect(modelPicker).toContainText("(use default)"); + await expect(modelPicker).not.toContainText("claude-opus-4-1"); + await expect(control.locator("[data-testid='model-row'] button[role='combobox']")).toContainText("(use default)"); + // The effective inherited values are surfaced in the source line + badged. + await expect(control.locator(LIST_MODEL_SOURCE)).toContainText("claude-opus-4-1"); await expect(control.locator(LIST_MODEL_SOURCE)).toHaveText(SRC_INHERITED_ROLE); + await expect(control.locator(LIST_THINKING_SOURCE)).toContainText("High"); await expect(control.locator(LIST_THINKING_SOURCE)).toHaveText(SRC_INHERITED_ROLE); + // finding #2: no clear/reset affordances for inherited-role fields. + await expect(control.locator("[data-testid='model-clear-btn']")).toHaveCount(0); + await expect(control.locator(LIST_THINKING_CLEAR)).toHaveCount(0); + }); + + test("list view rendering an inherited unsupported thinking value never auto-persists a clamped local override (finding #1)", async ({ page }) => { + // The inherited thinking value (xhigh) is unsupported by the inherited model + // in the registry. The old code fed it through the shared picker, whose + // render-time clamp fired onThinkingChange and auto-saved a spurious + // current-scope thinking override just from VIEWING the list. Now inherited + // values bypass the editable picker, so merely rendering must persist nothing. + await resetFixture(page, { + prefs: { "default.sessionModel": SESSION_DEFAULT_MODEL }, + roles: [ + { + name: "coder", label: "Coder", promptTemplate: "You write code.", + accessory: "none", toolPolicies: {}, + createdAt: 1, updatedAt: 1, origin: "project", + modelResolution: { + model: { value: TEST_MODEL, source: "inherited-role", origin: "server", editable: true }, + thinkingLevel: { value: "xhigh", source: "inherited-role", origin: "server", editable: true }, + }, + } as any, + ], + }); + await loadRoles(page, "#/roles"); + + const control = roleRow(page, "coder").locator(LIST_MODEL_CONTROL); + await expect(control).toBeVisible(); + await expect(control).toHaveAttribute("data-model-state", "inherited"); + + // Give any render-time clamp microtask a chance to (wrongly) fire a save. + await page.waitForTimeout(300); + + // Nothing was written: the role keeps no local model/thinking override. + await expect.poll(async () => { + const c = await storedRole(page, "coder"); + return [c?.model ?? "", c?.thinkingLevel ?? ""]; + }).toEqual(["", ""]); }); test("list view thinking-only change does not promote an inherited model into an override (finding #3)", async ({ page }) => { From b45681c358c670826208c66fccac4c63bb016cec Mon Sep 17 00:00:00 2001 From: Artur Jonkisz Date: Sat, 20 Jun 2026 20:40:17 +0100 Subject: [PATCH 14/22] docs(roles): document Roles list inline model controls and modelResolution metadata Co-authored-by: bobbit-ai --- docs/design/per-role-model-overrides.md | 73 +++++++++++++++++++++++++ docs/internals.md | 56 ++++++++++++++++++- docs/rest-api.md | 4 +- 3 files changed, 129 insertions(+), 4 deletions(-) diff --git a/docs/design/per-role-model-overrides.md b/docs/design/per-role-model-overrides.md index ddc68a639..dac41fb08 100644 --- a/docs/design/per-role-model-overrides.md +++ b/docs/design/per-role-model-overrides.md @@ -634,3 +634,76 @@ In the role detail editor (`renderRoleEditor`): * **Marketplace E2E (`tests/e2e/ui/marketplace.spec.ts`):** * Adjusted row-edit navigation. Because list rows now host interactive inline model controls, the test specifically clicks the explicit Pencil Edit action (`button[title="Edit"]`) in the row instead of clicking anywhere on the row. +--- + +## 9. Polished Roles Model Row Controls & Source Hierarchy (Release 0.14) + +### 9.1 Server-Side Metadata and Cascade Resolution (`modelResolution`) + +The server exposes granular field-level model/thinking resolution metadata via `resolveRoleModelResolution(roleName, projectId)` in `src/server/agent/config-cascade.ts`. This goes beyond flat role definitions to provide the client with the precise source, origin, and editability of each field. + +#### Source Hierarchy +The resolution engine walks the cascade (highest priority to lowest): +1. **Direct Role Override:** User-defined override at the current scope (Project or Server-wide). +2. **Inherited Role Override:** An override defined at an ancestor project layer, the server-wide layer, or via built-in / market-pack defaults. +3. **Effective Defaults:** If no role layer provides a value, it falls back to global preferences (`default.sessionModel` or `default.reviewModel` based on role type). + +#### Heuristic for Default Models +Since inherited model resolution is contextual at session-start, the UI uses a deterministic display-only heuristic to determine the fallback default: +* **Review Defaults (`default.reviewModel`):** Applied to review-centric roles (`architect`, `code-reviewer`, `reviewer`, `security-reviewer`, `spec-auditor`, and `qa-tester`). +* **Session Defaults (`default.sessionModel`):** Applied to all other roles, including custom-created roles. +* **Auto Default:** Rendered when no global defaults are configured (the AI gateway auto-selects a model). + +#### Metadata Fields +GET `/api/roles` and GET `/api/roles/:name` attach a `modelResolution` object mapping `model` and `thinkingLevel` to `RoleFieldSource` descriptors: +* `value`: The resolved model string (e.g. `anthropic/claude-opus-4-8`) or thinking level. +* `source`: `"role"` (current scope), `"inherited-role"` (inherited scope), or `"default"` (fallback). +* `origin`: `"project"`, `"server"`, or `"builtin"`. +* `originPackName`: The name of the source market-pack, if applicable. +* `editable`: Boolean indicating if editing is permitted. Gated by pack-managed status (pack roles are read-only) or whether a higher-precedence winner (like a global server user-override) shadows this scope, preventing confusing no-ops. +* `sourceLabel`: Clean human-friendly label (e.g., `Project`, `Server`, `Built-in`, `Inherited project`, or market pack name). + +--- + +### 9.2 Main Roles List Row Behavior & Visual Polish + +The list row inline controls inside `role-manager-page.ts` are designed to be extremely compact, source-aware, and aligned. + +#### Inline Selector Layout +* **Two-Line Layout:** Line 1 hosts the interactive selectors (reusing a compact variant wrapper of `renderModelRow` with contained chevrons). Line 2 houses the compact, monochromatic source badges below the selectors, aligning with the layout without overflowing boundaries or wasting vertical space. +* **Instant Auto-Save:** Interacting with a selector immediately persists the change to the backend via `updateRole` (scoped to the current project context). On success, a transient green `"Saved"` flash badge briefly renders next to the role label. +* **Rapid Save Coalescing (Queue & Lock):** To guard against race conditions—such as a user changing model and immediately changing thinking level before the network fetch completes—a per-role pending edit map (`pendingInlineEdits`) and lock set (`inlineSaveActive`) implement a sequential save queue. Edits are merged and written sequentially, preventing stale-state overwrites. +* **Independent Field Overrides:** Model and thinking can be overridden and cleared independently. A role can have a thinking-only override with no visible model override. Clearing the model override via the `×` button preserves any custom thinking level (falling back to the default/inherited model) and vice-versa. +* **Affordance Precision:** Clear and reset buttons are only rendered when there is an active current-scope override (`source === "role"`). If the field is inherited or default, no reset button is shown, and the dropdown shows `(use default)`, preventing confusing no-ops. +* **Event Isolation:** Select container events call `stopPropagation()` to prevent triggering the parent list-row click action, ensuring users do not accidentally navigate to the details editor when adjusting models. +* **Market Pack / Above-Current-Scope Read-Only:** Read-only roles render static effective labels plus a badge like `Managed by pack ` or `Read-only`, completely preventing inline modification. + +--- + +### 9.3 Detail Editor Behavior & Section Relocation + +The role detail page (`renderRoleEditor`): +* **Tab Simplification:** The separate "Model" tab has been removed; only the "Prompt" and "Tool Access" tabs remain. +* **Vertical Section Ordering:** Model and thinking controls are now laid out as a static vertical section positioned between the **Accessory** grid and the tab bar. +* **Draft-Based Saves:** Unlike the list row's immediate auto-saves, edits in the detail editor Model section are strictly draft-based. They are only committed to the backend when the user clicks the main **Save** button in the header. + +--- + +### 9.4 Visual QA Evidence & Checklist + +The following visual assertions have been verified in the development environment and are pinned by automated tests (`tests/e2e/ui/roles-model-reshuffle.spec.ts` and `tests/ui-fixtures/settings-admin-fixture.spec.ts`). + +#### Visual Verification Checklist +1. **Compact Rows:** Selectors and source badges are aligned, compact, and fit neatly inside the list rows. No overflow or layout brokenness. +2. **Source Badges Visible:** Labels like `Role override` (green), `Inherited role override · Server` (blue/grey), or `Session default` (grey) render correctly on line 2. +3. **Model & Thinking Always Visible:** The current effective labels (e.g. `claude-opus-4-8 · default` or overridden names) are legible. +4. **Chevrons Contained:** Dropdown arrows are perfectly positioned inside the controls. +5. **Responsive Action Buttons:** The Pencil (Edit) and Trash (Delete) buttons on the extreme right of the row remain fully reachable and clickable even at narrow viewport widths. +6. **Save-Hang Guard:** Detail editor's Save button correctly reverts to the idle "Save" state (not stuck on "Saving...") after a successful save. + +#### Visual Screenshots +Screenshots captured during visual validation can be found in: +* `.bobbit/tmp/roles-list-inherited.png` — Default and inherited role rows with greyed-out default indicators and disablement. +* `.bobbit/tmp/roles-list-overridden.png` — Active role overrides displaying interactive selectors and clear (`×`) / Test (flask) buttons. +* `.bobbit/tmp/roles-detail-layout.png` — Polished detail editor vertical structure with the static Model section. + diff --git a/docs/internals.md b/docs/internals.md index fd7a18ac7..3c99064ff 100644 --- a/docs/internals.md +++ b/docs/internals.md @@ -1432,9 +1432,61 @@ This is what makes "my `code-reviewer` role always runs on opus" work without ch **Naming model is explicitly unaffected** - `default.namingModel` and `pickFallbackAigwNamingModel` still drive title generation regardless of role. -### UI +### UI & API Metadata -The role-manager page (`src/app/role-manager-page.ts`) has a third tab next to **Prompt** and **Tool Access**, labelled **Model**. It reuses the model picker and thinking dropdown components from the settings page, with a leading "(use default)" option that maps to the empty string → omitted from YAML. The standard origin badge / Customize / Revert flow operates on the whole role record, so touching either field flips builtin→overridden and Revert clears them along with any other overrides. +The Roles model and thinking configuration integrates directly across both list and detail views, driven by server-resolved metadata. + +#### 1. Server-Side API Metadata (`modelResolution`) + +To enable accurate, source-aware rendering of effective values and inheritance, the `/api/roles` endpoints (both GET list and GET single role) return additive `modelResolution` metadata. This is computed by `resolveRoleModelResolution(roleName, projectId)` in `src/server/agent/config-cascade.ts`. + +It provides a granular, field-level audit of both `model` and `thinkingLevel`: + +```ts +export interface RoleModelResolution { + model: RoleFieldSource; + thinkingLevel: RoleFieldSource; +} + +export interface RoleFieldSource { + /** Resolved field value when a role layer supplies it; omitted for `default` */ + value?: string; + source: "role" | "inherited-role" | "default"; + /** Cascade layer the value came from (omitted for `default`) */ + origin?: "project" | "server" | "builtin"; + /** Market-pack name when the value comes from a pack-defined role; null otherwise */ + originPackName?: string | null; + /** False only for pack-managed/read-only roles or when shadowed above current scope; true otherwise */ + editable: boolean; + /** Short human label for the source ("Project", "Server", "Built-in", or pack name) */ + sourceLabel: string; +} +``` + +The metadata ensures that: +* **Shadowing is respected:** If a higher-precedence winner (e.g. a global server-user override) shadows the current project scope, the server reports the field as non-editable (`editable: false`). The UI displays it without misleading clear/reset or edit affordances that would secretly no-op. +* **Source labels are accurate:** Clear distinction between direct local overrides (`source: "role"`), inherited layers (`source: "inherited-role"`), or system-wide fallbacks (`source: "default"`). + +#### 2. Roles List View Inline Controls + +The main Roles page (`src/app/role-manager-page.ts`) renders compact, two-line model + thinking level inline controls in the middle of each list row by reusing a compact layout variant of the `renderModelRow` component: + +* **Two-Line Layout:** Line 1 displays the interactive model selector and thinking level dropdown. Line 2 displays compact, monochromatic source badges (e.g., `Model: [Session default] Thinking: [Session default]` or `[Inherited role override · Server]`). +* **Auto-Save on Change:** Interacting with the inline control immediately persists the changes to the backend. On success, a transient green `"Saved"` confirmation badge briefly flashes next to the role name. +* **Coalesced Saves (Race Guard):** Rapid sequential edits (such as picking a model and immediately changing thinking before the network refetch completes) are queued via `pendingInlineEdits` and handled sequentially in a serialized save loop (`scheduleInlineSave`). This prevents a newer change from reading stale data from the un-refetched row object and clobbering the previous field with empty strings. +* **Independent Overrides & Model Clears:** Model and thinking level can be overridden independently. Clicking the clear (`×`) button next to the model selector removes only the model override, preserving any custom thinking override (shown as a `thinking-override` state), and vice-versa. +* **No Misleading Reset Affordances:** Clearing and reset buttons are only exposed for active current-scope overrides (`source === "role"`). Inherited or default values display the placeholder label `(use default)` and have no clear buttons, avoiding confusing no-ops. +* **Propagation Stopping:** Dropdown click/keydown events are stopped via `stopPropagation` so that interacting with the selectors does not trigger the outer row's click-to-edit navigation. +* **Read-Only/Pack Rows:** Roles installed from a Market Pack (or non-editable due to scope shadow rules) render a static effective-value summary (Model + Thinking labels) alongside a badge like `Managed by pack ` or `Read-only`, completely disabling inline editing. + +#### 3. Role Detail Editor Relocation + +In the role detail editor (`renderRoleEditor`): + +* **Removed Tab:** The dedicated "Model" tab is removed; the tab bar now exposes only "Prompt" and "Tool Access". +* **Static Section:** Model and thinking controls are presented as a static section placed vertically between the **Accessory** selector and the tab bar, styled identically to the Accessory selector. +* **Draft-Based Saves:** Unlike the list page's immediate auto-saves, modifications within the detail-page Model section do not auto-save. They are draft-based and are only committed when the user clicks the main **Save** button in the top navigation bar. +* **Read-Only Inspector:** The read-only inspector (`renderRoleInspector` used in the goal-proposal modal) mimics this vertical layout, rendering the Model section as read-only. --- diff --git a/docs/rest-api.md b/docs/rest-api.md index bcfe0e68d..6312b83ce 100644 --- a/docs/rest-api.md +++ b/docs/rest-api.md @@ -463,9 +463,9 @@ See [docs/cache-hit-rate.md](cache-hit-rate.md) for full formula and null semant | Method | Path | Description | |---|---|---| -| `GET` | `/api/roles` | List all roles | +| `GET` | `/api/roles` | List all roles. Response roles include the resolved `modelResolution` field-level source and inheritance metadata. | | `POST` | `/api/roles` | Create a role (`{ name, label, promptTemplate, toolPolicies?, allowedTools?, accessory?, model?, thinkingLevel? }`). `toolPolicies` is the source of truth for tool access; `allowedTools` is accepted for backward compatibility and merged as `always-allow` entries. `model` is `"/"` format and overrides `default.sessionModel` / `default.reviewModel` for sessions of this role. `thinkingLevel` is one of `"off" | "minimal" | "low" | "medium" | "high" | "xhigh"` (clamped to the role's model capability at write time when `model` is set; see [docs/thinking-levels.md](thinking-levels.md)). | -| `GET` | `/api/roles/:name` | Get a role (includes `toolPolicies` and derived `allowedTools`) | +| `GET` | `/api/roles/:name` | Get a role (includes `toolPolicies`, derived `allowedTools`, and resolved `modelResolution` field-level source and inheritance metadata) | | `PUT` | `/api/roles/:name` | Update a role. Accepts `toolPolicies` (Record of tool/group name → policy), `model` (`"/"`), and `thinkingLevel` (`"off" | "minimal" | "low" | "medium" | "high" | "xhigh"`, clamped to the role's model capability when `model` is set). Policy values are validated against: `always-allow`, `ask-once`, `always-ask`, `never-ask`, `never`. Malformed `model` strings are rejected with 400. | | `DELETE` | `/api/roles/:name` | Delete a role | From 0e24a1972a6a353eb385259066bf781e5ce0f025 Mon Sep 17 00:00:00 2001 From: Artur Jonkisz Date: Sat, 20 Jun 2026 21:22:28 +0100 Subject: [PATCH 15/22] fix(roles): make list thinking Select compact-by-construction to stop chevron overflow Co-authored-by: bobbit-ai --- src/app/role-manager-page.ts | 5 ++++- src/app/role-manager.css | 12 +++++++----- src/app/settings-page.ts | 9 +++++++-- 3 files changed, 18 insertions(+), 8 deletions(-) diff --git a/src/app/role-manager-page.ts b/src/app/role-manager-page.ts index e037b9802..ee91ffd04 100644 --- a/src/app/role-manager-page.ts +++ b/src/app/role-manager-page.ts @@ -841,7 +841,10 @@ function renderRoleRowModelControl(role: RoleData, control: RoleRowModelControl) overrides.thinkingLevel, (v) => control.onThinkingChange(role, v), "", - { fallbackLabel: "(use default)" }, + // Compact list placement: a fixed-width, non-fit-content thinking + // Select so its trigger never injects an inline min-width:180px + // (the source of the row-control chevron overflow). + { fallbackLabel: "(use default)", thinkingWidth: "132px", thinkingFitContent: false }, )}
diff --git a/src/app/role-manager.css b/src/app/role-manager.css index 61c6c6cf3..d341f6469 100644 --- a/src/app/role-manager.css +++ b/src/app/role-manager.css @@ -232,12 +232,14 @@ padding: 2px; gap: 2px; } -/* The thinking Select defaults to a 180px min-width which bloats the row and - * pushes its chevron past the control edge. Clamp it so the chevron (a - * flex-shrink-0, justify-between child) stays inside the button bounds. */ +/* The thinking Select is rendered compact-by-construction for list rows + * (renderModelRow opts: thinkingWidth=132px, fitContent=false), so its trigger + * carries an inline width:132px and NO inline min-width:180px. These rules are + * a defensive cap only — they reinforce the fixed width without fighting it via + * !important, so the chevron (a flex-shrink-0, justify-between child) always + * stays inside the button bounds and inside the row control. */ .role-row-model-control [data-testid="model-row"] [role="combobox"] { - min-width: 0 !important; - width: auto !important; + min-width: 0; max-width: 132px; } diff --git a/src/app/settings-page.ts b/src/app/settings-page.ts index 141d59dc5..b714f1ab3 100644 --- a/src/app/settings-page.ts +++ b/src/app/settings-page.ts @@ -1814,7 +1814,7 @@ export function renderModelRow( thinkingValue: string, onThinkingChange: (v: string) => void, thinkingDefault: string = "medium", - opts?: { fallbackLabel?: string }, + opts?: { fallbackLabel?: string; thinkingWidth?: string; thinkingFitContent?: boolean }, ) { const modelDisplay = formatModelPref(modelValue, opts?.fallbackLabel); @@ -1940,7 +1940,12 @@ export function renderModelRow( onChange: (value: string) => { onThinkingChange(value); }, size: "sm", variant: "ghost", - fitContent: true, + // Settings callers pass neither opt → Select keeps its default + // 180px fit-content sizing. Compact list rows pass an explicit + // fixed width + fitContent:false so the trigger never injects an + // inline min-width:180px that overflows the narrow row cell. + ...(opts?.thinkingWidth ? { width: opts.thinkingWidth } : {}), + fitContent: opts?.thinkingFitContent ?? true, })}
From 43fb81c6bfaf9fd2fc35dcf8a79399044ce29f75 Mon Sep 17 00:00:00 2001 From: Artur Jonkisz Date: Sat, 20 Jun 2026 21:46:58 +0100 Subject: [PATCH 16/22] design: Roles list model/thinking row polish mockup Adds design/roles-model-row-polish-options.html with three layout options (A recommended) addressing the model/thinking row feedback: no ellipsis on key info, readable Built-in/Custom/Pack origins, flat informational source captions (not fake buttons), and symmetric in-box reset for model + thinking. Theme tokens only; light/dark verified; narrow-width wrap demonstrated. Co-authored-by: bobbit-ai --- design/roles-model-row-polish-options.html | 678 +++++++++++++++++++++ 1 file changed, 678 insertions(+) create mode 100644 design/roles-model-row-polish-options.html diff --git a/design/roles-model-row-polish-options.html b/design/roles-model-row-polish-options.html new file mode 100644 index 000000000..5ef146fcb --- /dev/null +++ b/design/roles-model-row-polish-options.html @@ -0,0 +1,678 @@ + + + + + +Roles list — model/thinking row polish options + + + + +
+ +
+
+

Roles list — model / thinking row polish

+

Three layout options for the inline model + thinking control. Recommendation marked below. Theme tokens only.

+
+
+ + +
+
+ +
+
The fixes

What each option must deliver, verified visually below.

+
1 · No ellipsis on key info

Full model name + source always readable.

+
2 · Readable origin

Built-in / Custom / Pack spelled out, never “…”.

+
3 · Source = information

Inheritance is flat muted text, not a fake button.

+
4 · Consistent reset

Model + thinking reset live in the same place, same affordance.

+
5 · Compact + professional

Density preserved; works down to narrow widths.

+
+ + + + +
+

Option A — Control box + quiet source caption Recommended

+

+ Keep the existing renderModelRow control box exactly as-is, but (a) reset lives inside the box + for both model and thinking (identical small × buttons, only shown when that field is overridden), + and (b) attribution moves to one flat caption line below — leading ↳, plain muted text, no pills, no borders, no hover. + Overridden fields are signalled by the box’s primary tint plus a quiet “● Override” word. Widening the list to ~820px + removes all truncation of model names and source text. +

+
+
+
+
+
+

Why A is recommended

+
    +
  • Smallest delta from production. Reuses the current control box + source-line slot; only the reset placement and caption styling change.
  • +
  • Reset is unambiguous & symmetric. Model × and thinking × are the same control in the same box (fix #4). The caption never carries a button (fix #3).
  • +
  • One scan line per concept. Row 1 = what it is + what it runs; caption = where that comes from. Full names, no “…” (fixes #1, #2, #5).
  • +
+
+ +
+ + + + +
+

Option B — Two explicit labelled field rows

+

+ Model and Thinking each get their own KEY · control · source line. Most explicit and self-documenting, + and reset is obviously per-field. Cost: taller rows (≈2× height) and it drifts further from the current single-control + layout, so it needs more new CSS. Good if we want maximum clarity over density. +

+
+
+
+
+ +
+ + + + +
+

Option C — Compact single line

+

+ Everything on one line: control box + trailing muted source text. Densest option, closest to today’s height. + The trade-off: at this density the source text has to stay terse and the model box still competes for width, so it’s + the most likely to clip on narrow viewports. Reset is in-box (same as A) for consistency. +

+
+
+
+
+ +
+ + + + +
+

Narrow-width behaviour (Option A @ 360px)

+

+ At narrow widths the control wraps beneath the identity, the slug hides, the model box goes full-width, and the source + caption wraps to its own line. Edit/delete stay reachable on the header line. Key info still never relies on “…”. +

+
+
+
+
+ +
+

Consistency rationale (per role-manager.css / renderModelRow)

+
    +
  • Same control: rows reuse the renderModelRow box (model picker, divider, thinking select). No new picker.
  • +
  • Same tokens: override tint is color-mix(in oklch, var(--primary) 45/8%, …) — already in .role-row-model-control--override.
  • +
  • Same badges: origin badge mirrors renderOriginBadge pill shape; source line replaces the .rrm-badge pills with flat text so nothing reads as a button.
  • +
  • Reset relocation: the only structural change vs. today is moving the thinking reset out of the caption and into the control box next to the model reset, so both are data-testid-able as in-box buttons.
  • +
  • Stable hooks: keep data-testid="role-row-model-control", data-model-state, role-row-model-source, role-row-thinking-source; thinking reset moves to an in-box model-thinking-clear-btn hook.
  • +
+
+ +
+ + + + From 21b449d934e88fe8de13e323205db2394776450c Mon Sep 17 00:00:00 2001 From: Artur Jonkisz Date: Sat, 20 Jun 2026 22:03:58 +0100 Subject: [PATCH 17/22] feat(roles): readable Option A model row (flat source caption, in-box resets) Reshuffle the inline model/thinking control on the Roles list per the approved mockup (design/roles-model-row-polish-options.html, Option A): - Widen the list and move the origin badge out of the ellipsising slug into a .role-meta row so Built-in/Custom/pack labels are never reduced to '...'. - Replace the button-like .rrm-badge source pills with a flat, informational source caption: inherited fields show the resolved value + source, override fields show a quiet accent '* Role override' marker (no chip/hover). - Move the thinking reset INTO the control box beside the model reset (renderModelRow gains an opt-in onThinkingClear -> model-thinking-clear-btn), so model and thinking reset from the same place (no split-brain UX). - Let model names + source text use the available width (no aggressive truncation); theme tokens only. Settings model rows are unchanged (the new behaviour is opt-in via the helper parameter). Fixture test updated for the relocated thinking-clear hook. Co-authored-by: bobbit-ai --- src/app/role-manager-page.ts | 60 ++++++--- src/app/role-manager.css | 120 +++++++++--------- src/app/settings-page.ts | 12 +- .../settings-admin-fixture.spec.ts | 10 +- 4 files changed, 117 insertions(+), 85 deletions(-) diff --git a/src/app/role-manager-page.ts b/src/app/role-manager-page.ts index ee91ffd04..624a2f2ef 100644 --- a/src/app/role-manager-page.ts +++ b/src/app/role-manager-page.ts @@ -3,7 +3,7 @@ import { icon } from "@mariozechner/mini-lit"; import { Button } from "@mariozechner/mini-lit/dist/Button.js"; import { Input } from "@mariozechner/mini-lit/dist/Input.js"; import { html, nothing, type TemplateResult } from "lit"; -import { ArrowLeft, Pencil, Plus, Trash2, X } from "lucide"; +import { ArrowLeft, Pencil, Plus, Trash2 } from "lucide"; import { fetchTools, updateRole, deleteRole, gatewayFetch, fetchAssistantPrompts, updateAssistantPrompt, fetchGroupPolicies, type RoleData, type ToolInfo, type AssistantPromptInfo, type RoleFieldSource } from "./api.js"; import { errorFromResponse, errorDetails } from "./error-helpers.js"; import { connectToSession } from "./session-manager.js"; @@ -787,9 +787,8 @@ function renderRoleRowModelControl(role: RoleData, control: RoleRowModelControl) if (display.packReadonly) { const reason = display.packName - ? html`Managed by pack ${display.packName}` - : html`Read-only`; + ? html`Managed by pack ${display.packName} — edit it in the Marketplace.` + : html`Read-only in this scope.`; return html`
Model${display.model.effectiveLabel}
Thinking${display.thinking.effectiveLabel}
- ${reason} +
+ ${reason} +
`; } @@ -812,18 +813,27 @@ function renderRoleRowModelControl(role: RoleData, control: RoleRowModelControl) // an empty local field would be a confusing no-op (review findings #1, #2). const overrides = currentScopeRoleOverrides(role); - const sourceLine = (key: string, f: RoleFieldDisplay, testid: string, clearTestid?: string) => { - const currentOverride = f.kind === "role"; + // Flat, informational source caption (Option A). NOT a pill/chip/button: + // - Override field: " Role override" (quiet accent text + leading dot via + // CSS). The value lives in the editable box; its reset is the in-box ×. + // - Inherited field: "↳ · " — muted text with + // the resolved value readable in the foreground colour, never ellipsised to + // a bare "…" for the source/origin text. + const sourceLine = (key: string, f: RoleFieldDisplay, testid: string) => { + if (f.kind === "role") { + return html` + + ${key} + Role override + `; + } return html` - - ${key} - ${currentOverride ? nothing : html`${f.effectiveLabel}`} - ${f.badge} - ${clearTestid && currentOverride ? html`` : nothing} - - `; + + + ${key} + ${f.effectiveLabel} + · ${f.badge} + `; }; return html` @@ -843,13 +853,20 @@ function renderRoleRowModelControl(role: RoleData, control: RoleRowModelControl) "", // Compact list placement: a fixed-width, non-fit-content thinking // Select so its trigger never injects an inline min-width:180px - // (the source of the row-control chevron overflow). - { fallbackLabel: "(use default)", thinkingWidth: "132px", thinkingFitContent: false }, + // (the source of the row-control chevron overflow). `onThinkingClear` + // puts the thinking reset INSIDE the box next to the model reset, so + // both fields reset from the same place (no split-brain UX). + { + fallbackLabel: "(use default)", + thinkingWidth: "132px", + thinkingFitContent: false, + onThinkingClear: () => control.onThinkingChange(role, ""), + }, )}
${sourceLine("Model", display.model, "role-row-model-source")} - ${sourceLine("Thinking", display.thinking, "role-row-thinking-source", "role-row-thinking-clear-btn")} + ${sourceLine("Thinking", display.thinking, "role-row-thinking-source")}
`; @@ -875,7 +892,10 @@ export function renderRoleListRow(opts: RoleRowOptions): TemplateResult { ${idleBlob(role.accessory ?? "none", 42, index, index)}
${role.label}${customized ? html` ` : nothing}${modelControl?.savedFlashRole === role.name ? html` Saved` : nothing} - ${role.name} ${renderOriginBadge(origin, overrides, (role as any).originPackName)} + + ${role.name} + ${renderOriginBadge(origin, overrides, (role as any).originPackName)} +
${modelControl ? renderRoleRowModelControl(role, modelControl) : nothing}
diff --git a/src/app/role-manager.css b/src/app/role-manager.css index d341f6469..a4f14d2fa 100644 --- a/src/app/role-manager.css +++ b/src/app/role-manager.css @@ -114,7 +114,9 @@ display: flex; flex-direction: column; gap: 8px; - max-width: 600px; + /* Widened from 600px so full model names + source/origin text are readable + * without ellipsising the key info (Option A). */ + max-width: 880px; margin: 0 auto; } @@ -146,7 +148,7 @@ } .role-row-info { - flex: 0 1 220px; + flex: 0 0 190px; min-width: 0; display: flex; flex-direction: column; @@ -161,6 +163,15 @@ white-space: nowrap; } +/* Slug + origin badge on one line: only the slug ellipsises; the origin badge + * (Built-in / Custom / pack) is shrink-0 so it is never reduced to "…". */ +.role-meta { + display: flex; + align-items: center; + gap: 6px; + min-width: 0; +} + .role-row-slug { font-size: 11px; font-family: var(--font-mono, monospace); @@ -168,6 +179,12 @@ overflow: hidden; text-overflow: ellipsis; white-space: nowrap; + min-width: 0; +} + +.role-meta .config-origin-badge, +.role-meta [data-testid="origin-pack-chip"] { + flex-shrink: 0; } .role-row-actions { @@ -207,13 +224,13 @@ * the edit/delete actions. Only the main Roles page supplies it. * -------------------------------------------------------------------------- */ .role-row-model-control { - flex: 1 1 300px; - min-width: 0; - max-width: 360px; + flex: 1 1 340px; + min-width: 180px; + max-width: 520px; display: flex; flex-direction: column; - gap: 3px; - overflow: hidden; + gap: 4px; + min-height: 0; } /* Compact placement: hide the (empty) label column and hint paragraph that @@ -250,79 +267,62 @@ background: color-mix(in oklch, var(--primary) 8%, var(--background)); } -/* Compact source line under the control: key + (effective value) + source badge. */ +/* ---------------------------------------------------------------------------- + * Source caption under the control box (Option A). + * Flat, INFORMATIONAL text only — no pills, no borders, no hover, no button + * affordance. Communicates where each field's value comes from. Resets live in + * the control box (model × + thinking ×), never on this line. + * -------------------------------------------------------------------------- */ .role-row-model-sources { display: flex; flex-wrap: wrap; - align-items: center; - gap: 3px 10px; - font-size: 11px; - line-height: 1.4; + align-items: baseline; + gap: 2px 14px; + font-size: 11.5px; + line-height: 1.45; color: var(--muted-foreground); + cursor: default; } .rrm-src { display: inline-flex; - align-items: center; + align-items: baseline; + flex-wrap: wrap; gap: 4px; min-width: 0; } +.rrm-src-arrow { + opacity: 0.6; +} .rrm-src-key { font-weight: 600; - opacity: 0.75; + opacity: 0.7; } +/* Resolved effective value — readable in the foreground colour, full text + * (wraps rather than truncating so key info is never reduced to "…"). */ .rrm-src-val { - max-width: 120px; - overflow: hidden; - text-overflow: ellipsis; - white-space: nowrap; color: var(--foreground); + min-width: 0; + word-break: break-word; } -.rrm-badge { - display: inline-flex; - align-items: center; - padding: 1px 6px; - border-radius: 999px; - font-size: 10px; - font-weight: 500; - white-space: nowrap; - border: 1px solid transparent; -} -.rrm-badge--role { - color: var(--primary); - background: color-mix(in oklch, var(--primary) 12%, transparent); - border-color: color-mix(in oklch, var(--primary) 35%, transparent); -} -.rrm-badge--inherited-role { - color: var(--foreground); - background: var(--secondary); - border-color: var(--border); -} -.rrm-badge--default, -.rrm-badge--auto { - color: var(--muted-foreground); - background: var(--muted); -} -.rrm-badge--pack { - color: var(--muted-foreground); - background: var(--muted); - border-color: var(--border); +.rrm-src-from { + opacity: 0.85; } -/* Independent thinking reset button on the source line. */ -.rrm-clear-btn { +/* "Override" state marker: a quiet accent word + leading dot. Deliberately + * flat (no border/background) so it reads as a STATE, not a tappable chip. */ +.rrm-src-state { display: inline-flex; align-items: center; - justify-content: center; - padding: 1px; - border-radius: 4px; - border: none; - background: transparent; - color: var(--muted-foreground); - cursor: pointer; - transition: background 150ms, color 150ms; + gap: 5px; + color: var(--primary); + font-weight: 600; } -.rrm-clear-btn:hover { - color: var(--foreground); - background: var(--secondary); +.rrm-src-state::before { + content: ""; + width: 6px; + height: 6px; + border-radius: 50%; + background: var(--primary); + flex-shrink: 0; } /* Read-only pack rows: static effective-value summary, no pickers. */ diff --git a/src/app/settings-page.ts b/src/app/settings-page.ts index b714f1ab3..7fe520e20 100644 --- a/src/app/settings-page.ts +++ b/src/app/settings-page.ts @@ -1814,7 +1814,7 @@ export function renderModelRow( thinkingValue: string, onThinkingChange: (v: string) => void, thinkingDefault: string = "medium", - opts?: { fallbackLabel?: string; thinkingWidth?: string; thinkingFitContent?: boolean }, + opts?: { fallbackLabel?: string; thinkingWidth?: string; thinkingFitContent?: boolean; onThinkingClear?: () => void }, ) { const modelDisplay = formatModelPref(modelValue, opts?.fallbackLabel); @@ -1948,6 +1948,16 @@ export function renderModelRow( fitContent: opts?.thinkingFitContent ?? true, })}
+ + ${opts?.onThinkingClear && thinkingValue ? html` + + ` : ""} ${testResult && !testing ? html` diff --git a/tests/ui-fixtures/settings-admin-fixture.spec.ts b/tests/ui-fixtures/settings-admin-fixture.spec.ts index fc54226ac..74d23d69a 100644 --- a/tests/ui-fixtures/settings-admin-fixture.spec.ts +++ b/tests/ui-fixtures/settings-admin-fixture.spec.ts @@ -38,14 +38,16 @@ const DETAIL_MODEL_SECTION = "[data-testid='roles-model-section']"; // thinking-only override a first-class, clearable state. These hooks are the // agreed contract between the implementation tasks (server source metadata + // compact list controls) and this test suite. Implementation should expose: -// - [data-testid='role-row-model-source'] — model source badge -// - [data-testid='role-row-thinking-source'] — thinking source badge -// - [data-testid='role-row-thinking-clear-btn'] — reset thinking independently +// - [data-testid='role-row-model-source'] — model source caption (flat text) +// - [data-testid='role-row-thinking-source'] — thinking source caption (flat text) +// - [data-testid='model-thinking-clear-btn'] — reset thinking independently +// (in the control box, beside the model reset — Option A keeps both resets +// in the same place rather than splitting them across the source line) // - [data-testid='role-row-model-readonly'] — read-only pack summary line // - data-model-state ∈ inherited | override | thinking-override | readonly const LIST_MODEL_SOURCE = "[data-testid='role-row-model-source']"; const LIST_THINKING_SOURCE = "[data-testid='role-row-thinking-source']"; -const LIST_THINKING_CLEAR = "[data-testid='role-row-thinking-clear-btn']"; +const LIST_THINKING_CLEAR = "[data-testid='model-thinking-clear-btn']"; // Design-doc source-label language (recommended badges). const SRC_ROLE_OVERRIDE = /Role override/; From 3a8999d3059d4e49548c15981c0ac06cc05d2a8c Mon Sep 17 00:00:00 2001 From: Artur Jonkisz Date: Sat, 20 Jun 2026 22:38:43 +0100 Subject: [PATCH 18/22] polish(roles): self-explanatory inherited model captions + clear reset labels Option A copy/affordance polish (no layout redesign): - Inherited list captions now read as a sentence: "Model inherits from session default: " / " role override: " instead of a cryptic "Session default"/"Review default" pill, with hover titles spelling out what session vs review default means. - Drop the leftwards arrow marker that could be mistaken for a reset button; the caption is plain non-interactive text. - Name the in-box override resets precisely via opt-in renderModelRow params: "Reset model override" / "Reset thinking override" (with aria-labels); Settings rows keep "Reset model to auto"/"Reset thinking to default". - Fixture source-caption regexes relaxed to the new wording. Co-authored-by: bobbit-ai --- src/app/role-manager-page.ts | 52 ++++++++++++++----- src/app/role-manager.css | 6 ++- src/app/settings-page.ts | 8 +-- .../settings-admin-fixture.spec.ts | 10 ++-- 4 files changed, 53 insertions(+), 23 deletions(-) diff --git a/src/app/role-manager-page.ts b/src/app/role-manager-page.ts index 624a2f2ef..6fee6c3ab 100644 --- a/src/app/role-manager-page.ts +++ b/src/app/role-manager-page.ts @@ -643,6 +643,11 @@ interface RoleFieldDisplay { * (review finding #1). The picker is fed only `currentScopeRoleOverrides`. */ effectiveValue: string; + /** + * Hover tooltip explaining the inherited source (e.g. what "session default" + * means). Only set for inherited tiers; the override caption needs none. + */ + title?: string; } /** @@ -713,6 +718,14 @@ function computeRoleModelDisplay(role: RoleData): RoleModelDisplay { ? "Auto default" : (prefKey === "default.reviewModel" ? "Review default" : "Session default"); const defaultKind: RoleFieldSourceUiKind = autoTier ? "auto" : "default"; + // Hover copy that spells out what each inherited source means, so the flat + // "session/review default" captions are never cryptic. Plain text only. + const defaultTitle = autoTier + ? "No default model is configured for this role, so the gateway auto-selects the best available model. Set a default in Settings → Models." + : (prefKey === "default.reviewModel" + ? "Review default — the model and thinking effort used by reviewer / QA-kind roles. Change it in Settings → Models." + : "Session default — the model and thinking effort used by normal agent sessions. Change it in Settings → Models."); + const inheritedRoleTitle = "Inherited from a lower-scope role override. Pick a model here to override it for the current scope."; const packName = directPackName ?? meta?.model?.originPackName ?? meta?.thinkingLevel?.originPackName ?? undefined; // Pack-managed roles (or any field the server flags non-editable) are @@ -722,7 +735,7 @@ function computeRoleModelDisplay(role: RoleData): RoleModelDisplay { const inheritedBadge = (m: RoleFieldSource): string => { const detail = m.sourceLabel || roleOriginDetailLabel(m.origin as ConfigOrigin | undefined, m.originPackName ?? packName); - return detail ? `Inherited role override · ${detail}` : "Inherited role override"; + return detail ? `${detail} role override` : "a lower-scope role override"; }; const fromMeta = (m: RoleFieldSource, format: (v: string) => string, emptyLabel: string): RoleFieldDisplay => { @@ -733,8 +746,8 @@ function computeRoleModelDisplay(role: RoleData): RoleModelDisplay { // editable picker is fed `currentScopeRoleOverrides` so inherited // values are never clamped/auto-saved (finding #1). case "role": return { kind: "role", badge: "Role override", effectiveLabel, effectiveValue: value }; - case "inherited-role": return { kind: "inherited-role", badge: inheritedBadge(m), effectiveLabel, effectiveValue: value }; - default: return { kind: defaultKind, badge: defaultBadge, effectiveLabel, effectiveValue: "" }; + case "inherited-role": return { kind: "inherited-role", badge: inheritedBadge(m), effectiveLabel, effectiveValue: value, title: inheritedRoleTitle }; + default: return { kind: defaultKind, badge: defaultBadge, effectiveLabel, effectiveValue: "", title: defaultTitle }; } }; @@ -745,7 +758,7 @@ function computeRoleModelDisplay(role: RoleData): RoleModelDisplay { } else if (role.model) { model = { kind: "role", badge: "Role override", effectiveLabel: formatModelPref(role.model), effectiveValue: role.model }; } else { - model = { kind: defaultKind, badge: defaultBadge, effectiveLabel: formatModelPref(def.model), effectiveValue: "" }; + model = { kind: defaultKind, badge: defaultBadge, effectiveLabel: formatModelPref(def.model), effectiveValue: "", title: defaultTitle }; } // THINKING @@ -756,7 +769,7 @@ function computeRoleModelDisplay(role: RoleData): RoleModelDisplay { } else if (role.thinkingLevel) { thinking = { kind: "role", badge: "Role override", effectiveLabel: thinkingLevelLabel(role.thinkingLevel), effectiveValue: role.thinkingLevel }; } else { - thinking = { kind: defaultKind, badge: defaultBadge, effectiveLabel: defThinkingLabel, effectiveValue: "" }; + thinking = { kind: defaultKind, badge: defaultBadge, effectiveLabel: defThinkingLabel, effectiveValue: "", title: defaultTitle }; } // State follows the CURRENT-SCOPE override (not the resolved winner): an @@ -798,7 +811,7 @@ function renderRoleRowModelControl(role: RoleData, control: RoleRowModelControl)
Thinking${display.thinking.effectiveLabel}
- ${reason} + ${reason}
`; @@ -813,26 +826,32 @@ function renderRoleRowModelControl(role: RoleData, control: RoleRowModelControl) // an empty local field would be a confusing no-op (review findings #1, #2). const overrides = currentScopeRoleOverrides(role); - // Flat, informational source caption (Option A). NOT a pill/chip/button: + // Flat, informational source caption (Option A). NOT a pill/chip/button, and + // no clickable-looking marker — the caption reads as a sentence so it is + // obviously descriptive rather than actionable: // - Override field: " Role override" (quiet accent text + leading dot via // CSS). The value lives in the editable box; its reset is the in-box ×. - // - Inherited field: "↳ · " — muted text with - // the resolved value readable in the foreground colour, never ellipsised to - // a bare "…" for the source/origin text. + // - Inherited field: " inherits from : " — muted + // text spelling out the hierarchy, with the resolved value readable in the + // foreground colour and never ellipsised to a bare "…". A hover title + // explains what "session/review default" means. const sourceLine = (key: string, f: RoleFieldDisplay, testid: string) => { if (f.kind === "role") { return html` - + ${key} Role override `; } + // Inherited-role phrases already read as " role override"; the + // session/review/auto default badges are lowercased to flow mid-sentence. + const phrase = f.kind === "inherited-role" ? f.badge : f.badge.toLowerCase(); return html` - - + ${key} + inherits from ${phrase}: ${f.effectiveLabel} - · ${f.badge} `; }; @@ -861,6 +880,11 @@ function renderRoleRowModelControl(role: RoleData, control: RoleRowModelControl) thinkingWidth: "132px", thinkingFitContent: false, onThinkingClear: () => control.onThinkingChange(role, ""), + // In the Roles list these resets clear a per-role OVERRIDE + // (reverting to the inherited default), so name them precisely + // rather than the generic Settings "Reset to auto/default". + clearModelTitle: "Reset model override", + clearThinkingTitle: "Reset thinking override", }, )} diff --git a/src/app/role-manager.css b/src/app/role-manager.css index a4f14d2fa..b453fe89d 100644 --- a/src/app/role-manager.css +++ b/src/app/role-manager.css @@ -290,8 +290,10 @@ gap: 4px; min-width: 0; } -.rrm-src-arrow { - opacity: 0.6; +/* Descriptive lead-in ("inherits from :") — quiet, non-interactive + * text so the caption reads as a sentence, never a clickable control. */ +.rrm-src-inherits { + opacity: 0.85; } .rrm-src-key { font-weight: 600; diff --git a/src/app/settings-page.ts b/src/app/settings-page.ts index 7fe520e20..81d5bfca0 100644 --- a/src/app/settings-page.ts +++ b/src/app/settings-page.ts @@ -1814,7 +1814,7 @@ export function renderModelRow( thinkingValue: string, onThinkingChange: (v: string) => void, thinkingDefault: string = "medium", - opts?: { fallbackLabel?: string; thinkingWidth?: string; thinkingFitContent?: boolean; onThinkingClear?: () => void }, + opts?: { fallbackLabel?: string; thinkingWidth?: string; thinkingFitContent?: boolean; onThinkingClear?: () => void; clearModelTitle?: string; clearThinkingTitle?: string }, ) { const modelDisplay = formatModelPref(modelValue, opts?.fallbackLabel); @@ -1890,7 +1890,8 @@ export function renderModelRow( ${modelValue ? html` diff --git a/tests/ui-fixtures/settings-admin-fixture.spec.ts b/tests/ui-fixtures/settings-admin-fixture.spec.ts index 74d23d69a..da47546ad 100644 --- a/tests/ui-fixtures/settings-admin-fixture.spec.ts +++ b/tests/ui-fixtures/settings-admin-fixture.spec.ts @@ -49,11 +49,13 @@ const LIST_MODEL_SOURCE = "[data-testid='role-row-model-source']"; const LIST_THINKING_SOURCE = "[data-testid='role-row-thinking-source']"; const LIST_THINKING_CLEAR = "[data-testid='model-thinking-clear-btn']"; -// Design-doc source-label language (recommended badges). +// Source-caption language. Inherited captions read as a self-explanatory +// sentence ("Model inherits from session default: …" / "… role +// override: …"), so these match the source phrase case-insensitively. const SRC_ROLE_OVERRIDE = /Role override/; -const SRC_INHERITED_ROLE = /Inherited role override/; -const SRC_SESSION_DEFAULT = /Session default/; -const SRC_REVIEW_DEFAULT = /Review default/; +const SRC_INHERITED_ROLE = /role override/i; +const SRC_SESSION_DEFAULT = /session default/i; +const SRC_REVIEW_DEFAULT = /review default/i; const SRC_ANY_DEFAULT = /default/i; function roleRow(page: Page, name: string) { From 670e92500e755dc5e9f982b30b518388e90ef641 Mon Sep 17 00:00:00 2001 From: Artur Jonkisz Date: Sun, 21 Jun 2026 00:03:49 +0100 Subject: [PATCH 19/22] polish roles inline model defaults Co-authored-by: bobbit-ai --- src/app/role-manager-page.ts | 81 +++++-------- src/app/role-manager.css | 41 +------ src/app/settings-page.ts | 22 +++- .../settings-admin-fixture.spec.ts | 111 ++++++++---------- 4 files changed, 95 insertions(+), 160 deletions(-) diff --git a/src/app/role-manager-page.ts b/src/app/role-manager-page.ts index 6fee6c3ab..a989698c9 100644 --- a/src/app/role-manager-page.ts +++ b/src/app/role-manager-page.ts @@ -710,20 +710,20 @@ function computeRoleModelDisplay(role: RoleData): RoleModelDisplay { const prefKey = getRoleDefaultModelPrefKey(role.name); const def = getRoleDefaultModel(role.name); - // The default-tier badge follows the session/review heuristic, downgrading to + // The default-tier badge follows the session/reviewer heuristic, downgrading to // "Auto default" only when no default model is configured at all (the tier is // effectively auto-selected). Applied consistently to model + thinking. const autoTier = !def.model; const defaultBadge = autoTier ? "Auto default" - : (prefKey === "default.reviewModel" ? "Review default" : "Session default"); + : (prefKey === "default.reviewModel" ? "Reviewer default" : "Session default"); const defaultKind: RoleFieldSourceUiKind = autoTier ? "auto" : "default"; - // Hover copy that spells out what each inherited source means, so the flat - // "session/review default" captions are never cryptic. Plain text only. + // Hover copy that spells out what each inherited source means, so the inline + // "session/reviewer default" labels are never cryptic. Plain text only. const defaultTitle = autoTier ? "No default model is configured for this role, so the gateway auto-selects the best available model. Set a default in Settings → Models." : (prefKey === "default.reviewModel" - ? "Review default — the model and thinking effort used by reviewer / QA-kind roles. Change it in Settings → Models." + ? "Reviewer default — the model and thinking effort used by reviewer / QA-kind roles. Change it in Settings → Models." : "Session default — the model and thinking effort used by normal agent sessions. Change it in Settings → Models."); const inheritedRoleTitle = "Inherited from a lower-scope role override. Pick a model here to override it for the current scope."; @@ -742,9 +742,9 @@ function computeRoleModelDisplay(role: RoleData): RoleModelDisplay { const value = m.value ?? ""; const effectiveLabel = value ? format(value) : emptyLabel; switch (m.source) { - // `effectiveValue` only feeds the read-only source-line label; the - // editable picker is fed `currentScopeRoleOverrides` so inherited - // values are never clamped/auto-saved (finding #1). + // `effectiveValue` is metadata only; the editable picker is fed + // `currentScopeRoleOverrides` so inherited values are never + // clamped/auto-saved (finding #1). case "role": return { kind: "role", badge: "Role override", effectiveLabel, effectiveValue: value }; case "inherited-role": return { kind: "inherited-role", badge: inheritedBadge(m), effectiveLabel, effectiveValue: value, title: inheritedRoleTitle }; default: return { kind: defaultKind, badge: defaultBadge, effectiveLabel, effectiveValue: "", title: defaultTitle }; @@ -788,11 +788,12 @@ function computeRoleModelDisplay(role: RoleData): RoleModelDisplay { * Inline model/thinking control rendered in the middle of a list row. * * Editable rows reuse the shared `renderModelRow` (model picker + clear/Test + - * thinking select) as the interactive line, then add a compact source line - * showing where each field's effective value comes from. Model and thinking are - * independent: clearing the model leaves any thinking override in place (shown - * as a thinking-only override), and thinking can be set/cleared on its own. - * Read-only pack rows render a static effective-value summary plus the reason. + * thinking select) as the interactive line. Inherited/default sources render as + * fallback text inside the controls, while overrides are communicated by the + * filled value plus reset/Test affordances. Model and thinking are independent: + * clearing the model leaves any thinking override in place, and thinking can be + * set/cleared on its own. Read-only pack rows render a static effective-value + * summary plus the reason. */ function renderRoleRowModelControl(role: RoleData, control: RoleRowModelControl): TemplateResult { const display = computeRoleModelDisplay(role); @@ -819,40 +820,19 @@ function renderRoleRowModelControl(role: RoleData, control: RoleRowModelControl) // Only a CURRENT-SCOPE role override (source === "role") gets the picker's // effective value and the clear/reset affordances. Inherited-role and - // default fields show their effective value + source in the read-only - // source line, but the picker is fed the empty current-scope override (so it - // renders "(use default)" and never clamps/auto-saves an inherited value), - // and no model-clear / thinking-reset control is exposed for them — clearing - // an empty local field would be a confusing no-op (review findings #1, #2). + // default fields feed the picker an empty current-scope override so it never + // clamps/auto-saves inherited values, but their source + effective value are + // now shown inside the picker fallback itself. const overrides = currentScopeRoleOverrides(role); - - // Flat, informational source caption (Option A). NOT a pill/chip/button, and - // no clickable-looking marker — the caption reads as a sentence so it is - // obviously descriptive rather than actionable: - // - Override field: " Role override" (quiet accent text + leading dot via - // CSS). The value lives in the editable box; its reset is the in-box ×. - // - Inherited field: " inherits from : " — muted - // text spelling out the hierarchy, with the resolved value readable in the - // foreground colour and never ellipsised to a bare "…". A hover title - // explains what "session/review default" means. - const sourceLine = (key: string, f: RoleFieldDisplay, testid: string) => { - if (f.kind === "role") { - return html` - - ${key} - Role override - `; - } - // Inherited-role phrases already read as " role override"; the - // session/review/auto default badges are lowercased to flow mid-sentence. - const phrase = f.kind === "inherited-role" ? f.badge : f.badge.toLowerCase(); - return html` - - ${key} - inherits from ${phrase}: - ${f.effectiveLabel} - `; + const inlineFallback = (f: RoleFieldDisplay): string | undefined => { + if (f.kind === "role") return undefined; + const source = f.kind === "inherited-role" ? f.badge : f.badge.toLowerCase(); + return `(Uses ${source}: ${f.effectiveLabel})`; + }; + const inlineTitle = (field: "model" | "thinking", f: RoleFieldDisplay): string | undefined => { + if (f.kind === "role") return undefined; + const source = f.kind === "inherited-role" ? f.badge : f.badge.toLowerCase(); + return `${field === "model" ? "Model" : "Thinking"} inherits from ${source}: ${f.effectiveLabel}. ${f.title ?? ""}`.trim(); }; return html` @@ -876,7 +856,8 @@ function renderRoleRowModelControl(role: RoleData, control: RoleRowModelControl) // puts the thinking reset INSIDE the box next to the model reset, so // both fields reset from the same place (no split-brain UX). { - fallbackLabel: "(use default)", + fallbackLabel: inlineFallback(display.model), + thinkingFallbackLabel: inlineFallback(display.thinking), thinkingWidth: "132px", thinkingFitContent: false, onThinkingClear: () => control.onThinkingChange(role, ""), @@ -885,13 +866,11 @@ function renderRoleRowModelControl(role: RoleData, control: RoleRowModelControl) // rather than the generic Settings "Reset to auto/default". clearModelTitle: "Reset model override", clearThinkingTitle: "Reset thinking override", + modelTitle: inlineTitle("model", display.model), + thinkingTitle: inlineTitle("thinking", display.thinking), }, )} -
- ${sourceLine("Model", display.model, "role-row-model-source")} - ${sourceLine("Thinking", display.thinking, "role-row-thinking-source")} -
`; } diff --git a/src/app/role-manager.css b/src/app/role-manager.css index b453fe89d..f2613674d 100644 --- a/src/app/role-manager.css +++ b/src/app/role-manager.css @@ -267,12 +267,7 @@ background: color-mix(in oklch, var(--primary) 8%, var(--background)); } -/* ---------------------------------------------------------------------------- - * Source caption under the control box (Option A). - * Flat, INFORMATIONAL text only — no pills, no borders, no hover, no button - * affordance. Communicates where each field's value comes from. Resets live in - * the control box (model × + thinking ×), never on this line. - * -------------------------------------------------------------------------- */ +/* Read-only pack rows: static effective-value summary, no pickers. */ .role-row-model-sources { display: flex; flex-wrap: wrap; @@ -290,44 +285,10 @@ gap: 4px; min-width: 0; } -/* Descriptive lead-in ("inherits from :") — quiet, non-interactive - * text so the caption reads as a sentence, never a clickable control. */ -.rrm-src-inherits { - opacity: 0.85; -} -.rrm-src-key { - font-weight: 600; - opacity: 0.7; -} -/* Resolved effective value — readable in the foreground colour, full text - * (wraps rather than truncating so key info is never reduced to "…"). */ -.rrm-src-val { - color: var(--foreground); - min-width: 0; - word-break: break-word; -} .rrm-src-from { opacity: 0.85; } -/* "Override" state marker: a quiet accent word + leading dot. Deliberately - * flat (no border/background) so it reads as a STATE, not a tappable chip. */ -.rrm-src-state { - display: inline-flex; - align-items: center; - gap: 5px; - color: var(--primary); - font-weight: 600; -} -.rrm-src-state::before { - content: ""; - width: 6px; - height: 6px; - border-radius: 50%; - background: var(--primary); - flex-shrink: 0; -} -/* Read-only pack rows: static effective-value summary, no pickers. */ .role-row-model-control--readonly { gap: 4px; } diff --git a/src/app/settings-page.ts b/src/app/settings-page.ts index 81d5bfca0..42a1c1645 100644 --- a/src/app/settings-page.ts +++ b/src/app/settings-page.ts @@ -1814,9 +1814,20 @@ export function renderModelRow( thinkingValue: string, onThinkingChange: (v: string) => void, thinkingDefault: string = "medium", - opts?: { fallbackLabel?: string; thinkingWidth?: string; thinkingFitContent?: boolean; onThinkingClear?: () => void; clearModelTitle?: string; clearThinkingTitle?: string }, + opts?: { + fallbackLabel?: string; + thinkingFallbackLabel?: string; + thinkingWidth?: string; + thinkingFitContent?: boolean; + onThinkingClear?: () => void; + clearModelTitle?: string; + clearThinkingTitle?: string; + modelTitle?: string; + thinkingTitle?: string; + }, ) { const modelDisplay = formatModelPref(modelValue, opts?.fallbackLabel); + const thinkingFallbackLabel = opts?.thinkingFallbackLabel ?? opts?.fallbackLabel; // Determine the selected model's capabilities. When the model is // unknown (not in allModels yet — registry still loading, or the saved @@ -1874,7 +1885,8 @@ export function renderModelRow( hover:bg-secondary transition-colors focus:outline-none focus:ring-2 focus:ring-ring flex-1 min-w-0 text-left ${modelValue ? "text-foreground" : "text-muted-foreground"}" - title="Choose model" + title=${opts?.modelTitle ?? "Choose model"} + data-testid="model-picker-btn" @click=${() => openModelPicker(modelValue, onModelChange)} > ${icon(Sparkles, "sm")} @@ -1930,12 +1942,12 @@ export function renderModelRow(
${Select({ - value: displayedThinking || (opts?.fallbackLabel ? "" : thinkingDefault), + value: displayedThinking || (thinkingFallbackLabel ? "" : thinkingDefault), options: [ - ...(opts?.fallbackLabel ? [{ value: "", label: opts.fallbackLabel, icon: icon(Brain, "sm") }] : []), + ...(thinkingFallbackLabel ? [{ value: "", label: thinkingFallbackLabel, icon: icon(Brain, "sm") }] : []), ...supportedLevels.map(lvl => ({ value: lvl, label: thinkingLabels[lvl], icon: icon(Brain, "sm") })), ] as SelectOption[], onChange: (value: string) => { onThinkingChange(value); }, diff --git a/tests/ui-fixtures/settings-admin-fixture.spec.ts b/tests/ui-fixtures/settings-admin-fixture.spec.ts index da47546ad..3230eaead 100644 --- a/tests/ui-fixtures/settings-admin-fixture.spec.ts +++ b/tests/ui-fixtures/settings-admin-fixture.spec.ts @@ -33,30 +33,17 @@ const LIST_MODEL_CONTROL = "[data-testid='role-row-model-control']"; const DETAIL_MODEL_SECTION = "[data-testid='roles-model-section']"; // ── Polish Roles list model rows — agreed list-control contract ───────────── -// The design doc replaces the old collapsed " · default" suffix with -// explicit, compact source badges for BOTH model and thinking, and makes a -// thinking-only override a first-class, clearable state. These hooks are the -// agreed contract between the implementation tasks (server source metadata + -// compact list controls) and this test suite. Implementation should expose: -// - [data-testid='role-row-model-source'] — model source caption (flat text) -// - [data-testid='role-row-thinking-source'] — thinking source caption (flat text) -// - [data-testid='model-thinking-clear-btn'] — reset thinking independently -// (in the control box, beside the model reset — Option A keeps both resets -// in the same place rather than splitting them across the source line) -// - [data-testid='role-row-model-readonly'] — read-only pack summary line -// - data-model-state ∈ inherited | override | thinking-override | readonly -const LIST_MODEL_SOURCE = "[data-testid='role-row-model-source']"; -const LIST_THINKING_SOURCE = "[data-testid='role-row-thinking-source']"; +// Inherited/default sources render inline inside the shared model/thinking +// controls. Override rows rely on the filled value + in-box reset/Test buttons, +// with no redundant source captions below the control. +const LIST_MODEL_PICKER = "[data-testid='model-picker-btn']"; const LIST_THINKING_CLEAR = "[data-testid='model-thinking-clear-btn']"; +const LIST_THINKING_PICKER = "[data-testid='model-row'] button[role='combobox']"; -// Source-caption language. Inherited captions read as a self-explanatory -// sentence ("Model inherits from session default: …" / "… role -// override: …"), so these match the source phrase case-insensitively. -const SRC_ROLE_OVERRIDE = /Role override/; -const SRC_INHERITED_ROLE = /role override/i; -const SRC_SESSION_DEFAULT = /session default/i; -const SRC_REVIEW_DEFAULT = /review default/i; -const SRC_ANY_DEFAULT = /default/i; +const USES_SESSION_DEFAULT = /\(Uses session default:/i; +const USES_REVIEWER_DEFAULT = /\(Uses reviewer default:/i; +const USES_AUTO_DEFAULT = /\(Uses auto default:/i; +const USES_ROLE_OVERRIDE = /\(Uses .*role override:/i; function roleRow(page: Page, name: string) { return page.locator(`.role-row[data-role-name='${name}']`); @@ -350,7 +337,7 @@ test.describe("Settings/admin UI fixture", () => { // store, so they pin the production render path, not a mock. // ──────────────────────────────────────────────────────────────────────── - test("list view inherited rows show resolved default model + thinking with source badges", async ({ page }) => { + test("list view inherited rows show resolved default model + thinking inline", async ({ page }) => { // Open Roles directly (no Settings visit first) with distinct defaults. await resetFixture(page, { prefs: { @@ -374,12 +361,11 @@ test.describe("Settings/admin UI fixture", () => { await expect(coderControl).toContainText(SESSION_DEFAULT_LABEL); await expect(reviewerControl).toContainText(REVIEW_DEFAULT_LABEL); - // Source is communicated by explicit badges for BOTH model and thinking, - // replacing the old collapsed "· default" suffix. - await expect(coderControl.locator(LIST_MODEL_SOURCE)).toHaveText(SRC_SESSION_DEFAULT); - await expect(coderControl.locator(LIST_THINKING_SOURCE)).toHaveText(SRC_SESSION_DEFAULT); - await expect(reviewerControl.locator(LIST_MODEL_SOURCE)).toHaveText(SRC_REVIEW_DEFAULT); - await expect(reviewerControl.locator(LIST_THINKING_SOURCE)).toHaveText(SRC_REVIEW_DEFAULT); + // Source is communicated inline inside each inherited control. + await expect(coderControl.locator(LIST_MODEL_PICKER)).toContainText(USES_SESSION_DEFAULT); + await expect(coderControl.locator(LIST_THINKING_PICKER)).toContainText(USES_SESSION_DEFAULT); + await expect(reviewerControl.locator(LIST_MODEL_PICKER)).toContainText(USES_REVIEWER_DEFAULT); + await expect(reviewerControl.locator(LIST_THINKING_PICKER)).toContainText(USES_REVIEWER_DEFAULT); // Inherited rows expose neither clear nor Test (modelValue is empty). await expect(coderControl.locator("[data-testid='model-clear-btn']")).toHaveCount(0); @@ -387,19 +373,19 @@ test.describe("Settings/admin UI fixture", () => { await expect(reviewerControl.locator("[data-testid='model-clear-btn']")).toHaveCount(0); }); - test("list view inherited rows fall back to an Auto default badge when prefs unset", async ({ page }) => { + test("list view inherited rows fall back to an Auto default label when prefs unset", async ({ page }) => { // No session/review default prefs configured → effective model is Auto, but - // the row still shows a meaningful default source badge (never blank). + // the row still shows a meaningful default source inline (never blank). await resetFixture(page, { prefs: {} }); await loadRoles(page, "#/roles"); const coderControl = roleRow(page, "coder").locator(LIST_MODEL_CONTROL); await expect(coderControl).toHaveAttribute("data-model-state", "inherited"); await expect(coderControl).toContainText(/Auto/); - await expect(coderControl.locator(LIST_MODEL_SOURCE)).toHaveText(SRC_ANY_DEFAULT); + await expect(coderControl.locator(LIST_MODEL_PICKER)).toContainText(USES_AUTO_DEFAULT); await expect(coderControl.locator("[data-testid='model-clear-btn']")).toHaveCount(0); }); - test("list view inherited role override surfaces an 'Inherited role override' badge with source", async ({ page }) => { + test("list view inherited role override surfaces source inline", async ({ page }) => { // The current scope does NOT override, but a lower (server) role layer // supplies the model. The server reports this via modelResolution so the // list can label it distinctly from a current-scope override or a default. @@ -422,18 +408,17 @@ test.describe("Settings/admin UI fixture", () => { await expect(control).toBeVisible(); // Effective model is shown. await expect(control).toContainText("claude-opus-4-1"); - // Distinct inherited-role badge naming where it came from. - await expect(control.locator(LIST_MODEL_SOURCE)).toHaveText(SRC_INHERITED_ROLE); - await expect(control.locator(LIST_MODEL_SOURCE)).toHaveText(/Server/i); + // Distinct inherited-role source naming is shown inline. + await expect(control.locator(LIST_MODEL_PICKER)).toContainText(USES_ROLE_OVERRIDE); + await expect(control.locator(LIST_MODEL_PICKER)).toContainText(/Server/i); }); - test("list view shows inherited-role effective model + thinking in the source line, not the editable picker, with no clear/reset (findings #1, #2)", async ({ page }) => { + test("list view shows inherited-role effective model + thinking inline with no clear/reset (findings #1, #2)", async ({ page }) => { // The role carries NO top-level model/thinkingLevel; the effective values // live only in modelResolution (inherited from the server role layer). - // - finding #1: inherited values must NOT be fed into the editable picker - // (that lets renderModelRow clamp + auto-persist them as a spurious local - // override on render). The picker shows the inherit placeholder; the - // effective values are displayed in the read-only source line instead. + // - finding #1: inherited values must NOT be fed into the picker as selected + // local overrides (that lets renderModelRow clamp + auto-persist them as a + // spurious local override on render). They display as fallback text. // - finding #2: inherited-role fields expose NO model-clear / thinking-reset // affordances (clearing an empty local field would be a confusing no-op). await resetFixture(page, { @@ -456,16 +441,13 @@ test.describe("Settings/admin UI fixture", () => { await expect(control).toBeVisible(); // Current scope did not override → the row reads as inherited, NOT override. await expect(control).toHaveAttribute("data-model-state", "inherited"); - // finding #1: picker shows the inherit placeholder, never the inherited value. - const modelPicker = control.locator("[data-testid='model-row'] button[title='Choose model']"); - await expect(modelPicker).toContainText("(use default)"); - await expect(modelPicker).not.toContainText("claude-opus-4-1"); - await expect(control.locator("[data-testid='model-row'] button[role='combobox']")).toContainText("(use default)"); - // The effective inherited values are surfaced in the source line + badged. - await expect(control.locator(LIST_MODEL_SOURCE)).toContainText("claude-opus-4-1"); - await expect(control.locator(LIST_MODEL_SOURCE)).toHaveText(SRC_INHERITED_ROLE); - await expect(control.locator(LIST_THINKING_SOURCE)).toContainText("High"); - await expect(control.locator(LIST_THINKING_SOURCE)).toHaveText(SRC_INHERITED_ROLE); + // finding #1: picker displays inherited effective values as fallback text, + // not as local overrides (so no clear/reset affordances appear). + const modelPicker = control.locator("[data-testid='model-row'] [data-testid='model-picker-btn']"); + await expect(modelPicker).toContainText(USES_ROLE_OVERRIDE); + await expect(modelPicker).toContainText("claude-opus-4-1"); + await expect(control.locator(LIST_THINKING_PICKER)).toContainText(USES_ROLE_OVERRIDE); + await expect(control.locator(LIST_THINKING_PICKER)).toContainText("High"); // finding #2: no clear/reset affordances for inherited-role fields. await expect(control.locator("[data-testid='model-clear-btn']")).toHaveCount(0); await expect(control.locator(LIST_THINKING_CLEAR)).toHaveCount(0); @@ -553,7 +535,7 @@ test.describe("Settings/admin UI fixture", () => { // Pick a model inline. Interacting with the control must NOT navigate to the // editor (design requires stopPropagation on the inline container). - await control.locator("[data-testid='model-row'] button[title='Choose model']").click(); + await control.locator("[data-testid='model-row'] [data-testid='model-picker-btn']").click(); await page.locator(`agent-model-selector [data-model-id="claude-opus-4-1"]`).dispatchEvent("click"); // Auto-saved: stored role gains the model, no Save button needed. @@ -565,8 +547,9 @@ test.describe("Settings/admin UI fixture", () => { await expect(control).toHaveAttribute("data-model-state", "override"); await expect(control.locator("[data-testid='model-clear-btn']")).toBeVisible(); await expect(control.locator("[data-testid='model-test-btn']")).toBeVisible(); - // And the model source badge now reads "Role override". - await expect(control.locator(LIST_MODEL_SOURCE)).toHaveText(SRC_ROLE_OVERRIDE); + // No redundant source caption under override rows. + await expect(control.locator("[data-testid='role-row-model-source']")).toHaveCount(0); + await expect(control.locator("[data-testid='role-row-thinking-source']")).toHaveCount(0); // Change thinking now that an override is active → auto-saves. await control.locator("[data-testid='model-row'] button[role='combobox']").click(); @@ -612,7 +595,7 @@ test.describe("Settings/admin UI fixture", () => { await expect(control).toHaveAttribute("data-model-state", "inherited"); // Pick a model inline — this kicks off the (now slow) save. - await control.locator("[data-testid='model-row'] button[title='Choose model']").click(); + await control.locator("[data-testid='model-row'] [data-testid='model-picker-btn']").click(); await page.locator(`agent-model-selector [data-model-id="claude-opus-4-1"]`).dispatchEvent("click"); // Immediately change thinking, WITHOUT waiting for the model save to settle. @@ -628,8 +611,8 @@ test.describe("Settings/admin UI fixture", () => { // And the row reflects the override end-state. await expect(control).toHaveAttribute("data-model-state", "override"); - await expect(control.locator(LIST_MODEL_SOURCE)).toHaveText(SRC_ROLE_OVERRIDE); - await expect(control.locator(LIST_THINKING_SOURCE)).toHaveText(SRC_ROLE_OVERRIDE); + await expect(control.locator("[data-testid='role-row-model-source']")).toHaveCount(0); + await expect(control.locator("[data-testid='role-row-thinking-source']")).toHaveCount(0); }); test("list view clearing the model preserves an existing thinking override (finding #1)", async ({ page }) => { @@ -663,8 +646,8 @@ test.describe("Settings/admin UI fixture", () => { // session default, thinking is still a Role override with its own reset. await expect(control).toHaveAttribute("data-model-state", /thinking-override|partial-override/); await expect(control).toContainText(SESSION_DEFAULT_LABEL); - await expect(control.locator(LIST_MODEL_SOURCE)).toHaveText(SRC_SESSION_DEFAULT); - await expect(control.locator(LIST_THINKING_SOURCE)).toHaveText(SRC_ROLE_OVERRIDE); + await expect(control.locator(LIST_MODEL_PICKER)).toContainText(USES_SESSION_DEFAULT); + await expect(control.locator(LIST_THINKING_PICKER)).toContainText("High"); await expect(control.locator("[data-testid='model-clear-btn']")).toHaveCount(0); // The dedicated thinking reset then clears the remaining override → fully inherited. @@ -698,9 +681,9 @@ test.describe("Settings/admin UI fixture", () => { await expect(control).toHaveAttribute("data-model-state", /thinking-override|partial-override/); // Model still resolves to the inherited default — never an icon-only row. await expect(control).toContainText(SESSION_DEFAULT_LABEL); - await expect(control.locator(LIST_MODEL_SOURCE)).toHaveText(SRC_SESSION_DEFAULT); + await expect(control.locator(LIST_MODEL_PICKER)).toContainText(USES_SESSION_DEFAULT); // Thinking is clearly an override and exposes an independent reset. - await expect(control.locator(LIST_THINKING_SOURCE)).toHaveText(SRC_ROLE_OVERRIDE); + await expect(control.locator(LIST_THINKING_PICKER)).toContainText("High"); const thinkingClear = control.locator(LIST_THINKING_CLEAR); await expect(thinkingClear).toBeVisible(); @@ -713,7 +696,7 @@ test.describe("Settings/admin UI fixture", () => { }).toEqual(["", ""]); // Row reverts to a fully inherited display. await expect(control).toHaveAttribute("data-model-state", "inherited"); - await expect(control.locator(LIST_THINKING_SOURCE)).toHaveText(SRC_SESSION_DEFAULT); + await expect(control.locator(LIST_THINKING_PICKER)).toContainText(USES_SESSION_DEFAULT); }); test("list view read-only pack role shows its source + effective values and no edit affordances", async ({ page }) => { @@ -745,7 +728,7 @@ test.describe("Settings/admin UI fixture", () => { // No inline mutation affordances on a read-only pack row. await expect(control.locator("[data-testid='model-clear-btn']")).toHaveCount(0); await expect(control.locator(LIST_THINKING_CLEAR)).toHaveCount(0); - await expect(control.locator("[data-testid='model-row'] button[title='Choose model']")).toHaveCount(0); + await expect(control.locator("[data-testid='model-row'] [data-testid='model-picker-btn']")).toHaveCount(0); }); // ──────────────────────────────────────────────────────────────────────── @@ -788,7 +771,7 @@ test.describe("Settings/admin UI fixture", () => { await expect(section).toBeVisible(); // Pick a model in the section. - await section.locator("[data-testid='model-row'] button[title='Choose model']").click(); + await section.locator("[data-testid='model-row'] [data-testid='model-picker-btn']").click(); await page.locator(`agent-model-selector [data-model-id="claude-opus-4-1"]`).dispatchEvent("click"); // Detail = Save button, NOT auto-save: the stored role is unchanged until Save. From 75366b7bed9a7fc4e7e4a0d3bd9b191513a61875 Mon Sep 17 00:00:00 2001 From: Artur Jonkisz Date: Sun, 21 Jun 2026 14:33:07 +0100 Subject: [PATCH 20/22] Polish roles inline model controls Co-authored-by: bobbit-ai --- src/app/role-manager-page.ts | 17 ++++- src/app/role-manager.css | 20 ++++-- .../settings-admin-fixture.spec.ts | 70 +++++++++++++++++++ 3 files changed, 97 insertions(+), 10 deletions(-) diff --git a/src/app/role-manager-page.ts b/src/app/role-manager-page.ts index a989698c9..fb4a56d7e 100644 --- a/src/app/role-manager-page.ts +++ b/src/app/role-manager-page.ts @@ -696,6 +696,17 @@ function thinkingLevelLabel(value: string): string { return THINKING_LEVEL_LABELS[value] ?? value; } +function renderRoleDefinitionOriginBadge(origin?: ConfigOrigin, overrides?: ConfigOrigin, originPackName?: string | null): TemplateResult | string { + if (!origin) return ""; + if (!overrides) return renderOriginBadge(origin, undefined, originPackName); + return html` + + ${renderOriginBadge(origin, undefined, originPackName)} + role definition overrides ${overrides} + + `; +} + /** * Resolve the model + thinking display (effective value + source badge) for a * list row. Prefers the server's field-level `modelResolution` metadata when @@ -858,7 +869,7 @@ function renderRoleRowModelControl(role: RoleData, control: RoleRowModelControl) { fallbackLabel: inlineFallback(display.model), thinkingFallbackLabel: inlineFallback(display.thinking), - thinkingWidth: "132px", + thinkingWidth: "clamp(164px, 44%, 220px)", thinkingFitContent: false, onThinkingClear: () => control.onThinkingChange(role, ""), // In the Roles list these resets clear a per-role OVERRIDE @@ -897,7 +908,7 @@ export function renderRoleListRow(opts: RoleRowOptions): TemplateResult { ${role.label}${customized ? html` ` : nothing}${modelControl?.savedFlashRole === role.name ? html` Saved` : nothing} ${role.name} - ${renderOriginBadge(origin, overrides, (role as any).originPackName)} + ${renderRoleDefinitionOriginBadge(origin, overrides, (role as any).originPackName)}
${modelControl ? renderRoleRowModelControl(role, modelControl) : nothing} @@ -1286,7 +1297,7 @@ export function renderRoleEditor(opts: RoleEditorOptions): TemplateResult {

Identity

- ${renderOriginBadge(origin, overrides, (role as any).originPackName)} + ${renderRoleDefinitionOriginBadge(origin, overrides, (role as any).originPackName)} ${headerExtras ?? nothing}
diff --git a/src/app/role-manager.css b/src/app/role-manager.css index f2613674d..83f248fbb 100644 --- a/src/app/role-manager.css +++ b/src/app/role-manager.css @@ -172,6 +172,13 @@ min-width: 0; } +.role-origin-definition-badge { + display: inline-flex; + align-items: center; + gap: 4px; + min-width: 0; +} + .role-row-slug { font-size: 11px; font-family: var(--font-mono, monospace); @@ -250,14 +257,13 @@ gap: 2px; } /* The thinking Select is rendered compact-by-construction for list rows - * (renderModelRow opts: thinkingWidth=132px, fitContent=false), so its trigger - * carries an inline width:132px and NO inline min-width:180px. These rules are - * a defensive cap only — they reinforce the fixed width without fighting it via - * !important, so the chevron (a flex-shrink-0, justify-between child) always - * stays inside the button bounds and inside the row control. */ + * (renderModelRow opts: thinkingWidth=clamp(164px, 44%, 220px), + * fitContent=false), so its trigger gets more room than the default compact row + * while still avoiding the global Settings min-width:180px behavior. These + * rules are a defensive cap only, preserving chevron containment in narrow rows. */ .role-row-model-control [data-testid="model-row"] [role="combobox"] { min-width: 0; - max-width: 132px; + max-width: 220px; } /* Override / thinking-override rows: accent the control box with a primary tint. */ @@ -582,7 +588,7 @@ flex: 1 1 auto; } .role-row-model-control { - flex: 1 1 100%; + flex: 0 0 100%; order: 3; max-width: none; } diff --git a/tests/ui-fixtures/settings-admin-fixture.spec.ts b/tests/ui-fixtures/settings-admin-fixture.spec.ts index 3230eaead..2b6122a46 100644 --- a/tests/ui-fixtures/settings-admin-fixture.spec.ts +++ b/tests/ui-fixtures/settings-admin-fixture.spec.ts @@ -373,6 +373,51 @@ test.describe("Settings/admin UI fixture", () => { await expect(reviewerControl.locator("[data-testid='model-clear-btn']")).toHaveCount(0); }); + test("list view thinking picker has room and wraps cleanly on narrow screens", async ({ page }) => { + await page.setViewportSize({ width: 1280, height: 800 }); + await resetFixture(page, { + prefs: { + "default.sessionModel": SESSION_DEFAULT_MODEL, + "default.reviewModel": REVIEW_DEFAULT_MODEL, + }, + }); + await loadRoles(page, "#/roles"); + + const reviewerControl = roleRow(page, "reviewer").locator(LIST_MODEL_CONTROL); + const thinking = reviewerControl.locator(LIST_THINKING_PICKER); + await expect(thinking).toContainText(USES_REVIEWER_DEFAULT); + const desktopWidth = await thinking.evaluate((el) => el.getBoundingClientRect().width); + expect(desktopWidth).toBeGreaterThanOrEqual(164); + expect(desktopWidth).toBeLessThanOrEqual(222); + expect(await thinking.evaluate((el) => { + const rect = el.getBoundingClientRect(); + const svgs = Array.from(el.querySelectorAll("svg")); + const chevron = svgs[svgs.length - 1]; + if (!chevron) return false; + const iconRect = chevron.getBoundingClientRect(); + return iconRect.left >= rect.left && iconRect.right <= rect.right; + })).toBe(true); + + await page.setViewportSize({ width: 390, height: 740 }); + await reloadFixture(page); + await loadRoles(page, "#/roles"); + const row = roleRow(page, "reviewer"); + const mobileControl = row.locator(LIST_MODEL_CONTROL); + const actions = row.locator(".role-row-actions"); + await expect(mobileControl).toBeVisible(); + await expect(actions).toBeVisible(); + const mobile = await row.evaluate((el) => { + const rowRect = el.getBoundingClientRect(); + const control = el.querySelector("[data-testid='role-row-model-control']")?.getBoundingClientRect(); + const actions = el.querySelector(".role-row-actions")?.getBoundingClientRect(); + return control && actions ? { + controlFitsRow: control.left >= rowRect.left && control.right <= rowRect.right, + actionsFitRow: actions.left >= rowRect.left && actions.right <= rowRect.right, + } : null; + }); + expect(mobile).toEqual({ controlFitsRow: true, actionsFitRow: true }); + }); + test("list view inherited rows fall back to an Auto default label when prefs unset", async ({ page }) => { // No session/review default prefs configured → effective model is Auto, but // the row still shows a meaningful default source inline (never blank). @@ -385,6 +430,31 @@ test.describe("Settings/admin UI fixture", () => { await expect(coderControl.locator("[data-testid='model-clear-btn']")).toHaveCount(0); }); + test("role-definition origin badge does not read like a model override", async ({ page }) => { + await resetFixture(page, { + prefs: { "default.sessionModel": SESSION_DEFAULT_MODEL }, + roles: [ + { + name: "coder", label: "Coder", promptTemplate: "You write code.", + accessory: "none", toolPolicies: {}, + createdAt: 1, updatedAt: 1, origin: "server", overrides: "builtin", + modelResolution: { + model: { value: "", source: "default", editable: true }, + thinkingLevel: { value: "", source: "default", editable: true }, + }, + } as any, + ], + }); + await loadRoles(page, "#/roles"); + + const row = roleRow(page, "coder"); + const control = row.locator(LIST_MODEL_CONTROL); + await expect(control).toHaveAttribute("data-model-state", "inherited"); + await expect(row.locator(".role-meta")).toContainText("role definition overrides builtin"); + await expect(control.locator("[data-testid='model-clear-btn']")).toHaveCount(0); + await expect(control.locator("[data-testid='model-test-btn']")).toHaveCount(0); + }); + test("list view inherited role override surfaces source inline", async ({ page }) => { // The current scope does NOT override, but a lower (server) role layer // supplies the model. The server reports this via modelResolution so the From 19e952923e257daf83330f37d04372c6ee33b1ca Mon Sep 17 00:00:00 2001 From: Artur Jonkisz Date: Sun, 21 Jun 2026 18:27:51 +0100 Subject: [PATCH 21/22] polish roles row width allocation Co-authored-by: bobbit-ai --- src/app/role-manager-page.ts | 2 +- src/app/role-manager.css | 22 ++++++++++--------- .../settings-admin-fixture.spec.ts | 4 ++-- 3 files changed, 15 insertions(+), 13 deletions(-) diff --git a/src/app/role-manager-page.ts b/src/app/role-manager-page.ts index fb4a56d7e..4ef23a60d 100644 --- a/src/app/role-manager-page.ts +++ b/src/app/role-manager-page.ts @@ -869,7 +869,7 @@ function renderRoleRowModelControl(role: RoleData, control: RoleRowModelControl) { fallbackLabel: inlineFallback(display.model), thinkingFallbackLabel: inlineFallback(display.thinking), - thinkingWidth: "clamp(164px, 44%, 220px)", + thinkingWidth: "clamp(220px, 36%, 360px)", thinkingFitContent: false, onThinkingClear: () => control.onThinkingChange(role, ""), // In the Roles list these resets clear a per-role OVERRIDE diff --git a/src/app/role-manager.css b/src/app/role-manager.css index 83f248fbb..62a39e412 100644 --- a/src/app/role-manager.css +++ b/src/app/role-manager.css @@ -114,9 +114,9 @@ display: flex; flex-direction: column; gap: 8px; - /* Widened from 600px so full model names + source/origin text are readable - * without ellipsising the key info (Option A). */ - max-width: 880px; + /* Use the available page width so the inline model/thinking controls can + * breathe instead of truncating inside a narrow centered column. */ + max-width: min(100%, 1100px); margin: 0 auto; } @@ -231,9 +231,9 @@ * the edit/delete actions. Only the main Roles page supplies it. * -------------------------------------------------------------------------- */ .role-row-model-control { - flex: 1 1 340px; + flex: 1 1 520px; min-width: 180px; - max-width: 520px; + max-width: none; display: flex; flex-direction: column; gap: 4px; @@ -257,13 +257,12 @@ gap: 2px; } /* The thinking Select is rendered compact-by-construction for list rows - * (renderModelRow opts: thinkingWidth=clamp(164px, 44%, 220px), - * fitContent=false), so its trigger gets more room than the default compact row - * while still avoiding the global Settings min-width:180px behavior. These - * rules are a defensive cap only, preserving chevron containment in narrow rows. */ + * (renderModelRow opts: thinkingWidth=clamp(220px, 36%, 360px), + * fitContent=false), giving it enough room for the inline default label while + * still avoiding the global Settings min-width:180px overflow behaviour. */ .role-row-model-control [data-testid="model-row"] [role="combobox"] { min-width: 0; - max-width: 220px; + max-width: 360px; } /* Override / thinking-override rows: accent the control box with a primary tint. */ @@ -592,6 +591,9 @@ order: 3; max-width: none; } + .role-row-model-control [data-testid="model-row"] [role="combobox"] { + max-width: min(190px, 46vw); + } .role-row-action-btn { width: 28px; height: 28px; diff --git a/tests/ui-fixtures/settings-admin-fixture.spec.ts b/tests/ui-fixtures/settings-admin-fixture.spec.ts index 2b6122a46..f2b7377af 100644 --- a/tests/ui-fixtures/settings-admin-fixture.spec.ts +++ b/tests/ui-fixtures/settings-admin-fixture.spec.ts @@ -387,8 +387,8 @@ test.describe("Settings/admin UI fixture", () => { const thinking = reviewerControl.locator(LIST_THINKING_PICKER); await expect(thinking).toContainText(USES_REVIEWER_DEFAULT); const desktopWidth = await thinking.evaluate((el) => el.getBoundingClientRect().width); - expect(desktopWidth).toBeGreaterThanOrEqual(164); - expect(desktopWidth).toBeLessThanOrEqual(222); + expect(desktopWidth).toBeGreaterThanOrEqual(220); + expect(desktopWidth).toBeLessThanOrEqual(362); expect(await thinking.evaluate((el) => { const rect = el.getBoundingClientRect(); const svgs = Array.from(el.querySelectorAll("svg")); From 7f492fc06abb70a052bbe5fd48c60494bcc673bb Mon Sep 17 00:00:00 2001 From: Artur Jonkisz Date: Sun, 21 Jun 2026 20:27:58 +0100 Subject: [PATCH 22/22] keep roles list compact at 880px Co-authored-by: bobbit-ai --- src/app/role-manager.css | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/app/role-manager.css b/src/app/role-manager.css index 62a39e412..c6b01f8cf 100644 --- a/src/app/role-manager.css +++ b/src/app/role-manager.css @@ -114,9 +114,9 @@ display: flex; flex-direction: column; gap: 8px; - /* Use the available page width so the inline model/thinking controls can - * breathe instead of truncating inside a narrow centered column. */ - max-width: min(100%, 1100px); + /* Keep the Roles list compact, while row internals allocate enough space for + * inline model/thinking labels without expanding the whole page. */ + max-width: 880px; margin: 0 auto; }