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

+ +
+ +
+ + + + +
+

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)

+ +
+ +
+ + + + diff --git a/docs/design/per-role-model-overrides.md b/docs/design/per-role-model-overrides.md index 2ed58afcc..dac41fb08 100644 --- a/docs/design/per-role-model-overrides.md +++ b/docs/design/per-role-model-overrides.md @@ -589,3 +589,121 @@ 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. + +--- + +## 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 a4e465054..a25f175d8 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 3206cb8a7..57b0ee301 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 | 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/app/proposal-panels.ts b/src/app/proposal-panels.ts index 3d96802f5..8db3162d5 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; @@ -2827,7 +2827,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..4ef23a60d 100644 --- a/src/app/role-manager-page.ts +++ b/src/app/role-manager-page.ts @@ -4,7 +4,7 @@ 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 { 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"; @@ -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, getRoleDefaultModel, getRoleDefaultModelPrefKey, ensureModelDefaultsLoaded } from "./settings-page.js"; // ============================================================================ // HELPERS @@ -63,11 +63,28 @@ 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; + +// 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 = {}; @@ -117,6 +134,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 +274,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 +286,99 @@ 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 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, + accessory: role.accessory, + toolPolicies: role.toolPolicies ?? {}, + model, + thinkingLevel, + }, projectId || 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); +} + +/** + * 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( @@ -309,6 +427,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 +440,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 +582,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 +604,291 @@ interface RoleRowOptions { onSelect: (role: RoleData) => void; onEdit?: (role: RoleData) => void; onDelete?: (role: RoleData) => void; + 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", +}; + +type RoleFieldSourceUiKind = "role" | "inherited-role" | "default" | "auto"; + +interface RoleFieldDisplay { + /** + * 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) 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; + /** + * Hover tooltip explaining the inherited source (e.g. what "session default" + * means). Only set for inherited tiers; the override caption needs none. + */ + title?: 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 { + /** 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; +} + +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 + * 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 directPackName = (role as any).originPackName as string | null | undefined; + const packId = (role as any).originPackId as string | null | undefined; + const meta = role.modelResolution; + const prefKey = getRoleDefaultModelPrefKey(role.name); + const def = getRoleDefaultModel(role.name); + + // 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" ? "Reviewer default" : "Session default"); + const defaultKind: RoleFieldSourceUiKind = autoTier ? "auto" : "default"; + // 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" + ? "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."; + + 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 ? `${detail} role override` : "a lower-scope role override"; + }; + + const fromMeta = (m: RoleFieldSource, format: (v: string) => string, emptyLabel: string): RoleFieldDisplay => { + const value = m.value ?? ""; + const effectiveLabel = value ? format(value) : emptyLabel; + switch (m.source) { + // `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 }; + } + }; + + // MODEL + let model: RoleFieldDisplay; + if (meta) { + model = fromMeta(meta.model, (v) => formatModelPref(v), formatModelPref(def.model)); + } 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: "", title: defaultTitle }; + } + + // THINKING + const defThinkingLabel = def.thinking ? thinkingLevelLabel(def.thinking) : "Default"; + let thinking: RoleFieldDisplay; + if (meta) { + thinking = fromMeta(meta.thinkingLevel, (v) => thinkingLevelLabel(v), defThinkingLabel); + } 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: "", title: defaultTitle }; + } + + // 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 (overrides.model) state = "override"; + else if (overrides.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. + * + * Editable rows reuse the shared `renderModelRow` (model picker + clear/Test + + * 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); + const stopRow = (e: Event) => e.stopPropagation(); + + if (display.packReadonly) { + const reason = display.packName + ? 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} +
+
+ `; + } + + // Only a CURRENT-SCOPE role override (source === "role") gets the picker's + // effective value and the clear/reset affordances. Inherited-role and + // 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); + 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` +
+
+ ${renderModelRow( + "", + "", + overrides.model, + (v) => control.onModelChange(role, v), + overrides.thinkingLevel, + (v) => control.onThinkingChange(role, v), + "", + // 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). `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: inlineFallback(display.model), + thinkingFallbackLabel: inlineFallback(display.thinking), + thinkingWidth: "clamp(220px, 36%, 360px)", + 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", + modelTitle: inlineTitle("model", display.model), + thinkingTitle: inlineTitle("thinking", display.thinking), + }, + )} +
+
+ `; } /** 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 +905,13 @@ 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.name} ${renderOriginBadge(origin, overrides, (role as any).originPackName)} + ${role.label}${customized ? html` ` : nothing}${modelControl?.savedFlashRole === role.name ? html` Saved` : nothing} + + ${role.name} + ${renderRoleDefinitionOriginBadge(origin, overrides, (role as any).originPackName)} +
+ ${modelControl ? renderRoleRowModelControl(role, modelControl) : nothing}
${onEdit ? html`
`; @@ -567,6 +988,26 @@ function renderListView(): TemplateResult { onEdit: (r) => showEdit(r), onDelete: (r) => handleDeleteFromList(r), emptyAction, + modelControl: { + savedFlashRole: savedModelFlashRole, + onModelChange: (role, model) => { + // 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). 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. 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 — and a model picked moments earlier + // (still mid-save) is preserved instead of being overwritten with "". + queueInlineEdit(role, { thinkingLevel: thinking }); + }, + }, }); if (loading || roles.length === 0) return list; @@ -788,7 +1229,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).

@@ -804,7 +1245,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", @@ -856,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}
@@ -901,14 +1342,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 +1376,7 @@ export function renderRoleEditor(opts: RoleEditorOptions): TemplateResult { onAssistantPromptTabChange, onAssistantPromptChange, }) - : draft.activeTab === "tools" - ? renderRoleToolAccessTab({ + : renderRoleToolAccessTab({ toolPolicies: draft.toolPolicies, availableTools: tools, groupPolicies: gp, @@ -939,13 +1389,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 +1407,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..c6b01f8cf 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; + /* 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; } @@ -146,7 +148,7 @@ } .role-row-info { - flex: 1; + flex: 0 0 190px; min-width: 0; display: flex; flex-direction: column; @@ -161,6 +163,22 @@ 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-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); @@ -168,6 +186,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 { @@ -201,6 +225,113 @@ 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 520px; + min-width: 180px; + max-width: none; + display: flex; + flex-direction: column; + gap: 4px; + min-height: 0; +} + +/* 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, +.role-row-model-control [data-testid="model-row"] > p { + display: none; +} + +/* Tighten the renderModelRow control box for list density. */ +.role-row-model-control [data-testid="model-row"] { + gap: 0; +} +.role-row-model-control [data-testid="model-row"] > div:first-child > div:nth-child(2) { + padding: 2px; + gap: 2px; +} +/* The thinking Select is rendered compact-by-construction for list 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: 360px; +} + +/* 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)); +} + +/* Read-only pack rows: static effective-value summary, no pickers. */ +.role-row-model-sources { + display: flex; + flex-wrap: wrap; + 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: baseline; + flex-wrap: wrap; + gap: 4px; + min-width: 0; +} +.rrm-src-from { + opacity: 0.85; +} + +.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; + font-weight: 500; + color: var(--primary); + opacity: 0.9; +} + /* ============================================================================ * EDIT VIEW — TWO COLUMN * ============================================================================ */ @@ -445,10 +576,24 @@ .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: 0 0 100%; + 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/src/app/settings-page.ts b/src/app/settings-page.ts index 204965ac3..0799b76b1 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; @@ -1771,9 +1814,20 @@ export function renderModelRow( thinkingValue: string, onThinkingChange: (v: string) => void, thinkingDefault: string = "medium", - opts?: { fallbackLabel?: 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 @@ -1831,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")} @@ -1847,7 +1902,8 @@ export function renderModelRow( ${modelValue ? html` + ` : ""} ${testResult && !testing ? html` diff --git a/src/server/agent/config-cascade.ts b/src/server/agent/config-cascade.ts index 8cdc7ac5f..78c6433f3 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,133 @@ 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; + + // 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 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"); + // 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"; + // 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 && !shadowsEditableLayer, + 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) { + // 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" }; + } + } + // 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 6aefa4bd3..53b281369 100644 --- a/src/server/server.ts +++ b/src/server/server.ts @@ -2657,6 +2657,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 @@ -7679,7 +7693,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; } @@ -7791,11 +7805,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..5eb43efaf 100644 --- a/tests/config-cascade.test.ts +++ b/tests/config-cascade.test.ts @@ -103,6 +103,364 @@ 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("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"); + // 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, 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)", () => { + // 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/e2e/ui/marketplace.spec.ts b/tests/e2e/ui/marketplace.spec.ts index 0e64556f9..7b9f083ad 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); 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..051636587 --- /dev/null +++ b/tests/e2e/ui/roles-model-reshuffle.spec.ts @@ -0,0 +1,200 @@ +/** + * 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 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", "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-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..f2b7377af 100644 --- a/tests/ui-fixtures/settings-admin-fixture.spec.ts +++ b/tests/ui-fixtures/settings-admin-fixture.spec.ts @@ -19,6 +19,40 @@ 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']"; + +// ── Polish Roles list model rows — agreed list-control contract ───────────── +// 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']"; + +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}']`); +} + +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 +294,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 +311,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 +321,592 @@ 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 inline", 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"); + + // 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(reviewerControl).toContainText(REVIEW_DEFAULT_LABEL); + + // 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); + 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 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(220); + expect(desktopWidth).toBeLessThanOrEqual(362); + 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). + 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_PICKER)).toContainText(USES_AUTO_DEFAULT); + 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 + // 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 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 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 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, { + 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"); + // 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); + }); + + 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 }) => { + // 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"); + + 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'] [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. + 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(); + // 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(); + 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 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'] [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. + 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("[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 }) => { + 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(); + + // 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(["", TEST_THINKING]); + + // 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_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. + 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 }) => { + // 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_PICKER)).toContainText(USES_SESSION_DEFAULT); + // Thinking is clearly an override and exposes an independent reset. + await expect(control.locator(LIST_THINKING_PICKER)).toContainText("High"); + 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_PICKER)).toContainText(USES_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'] [data-testid='model-picker-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'] [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. + 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"); + }); });