Conversation
…re clean working tree
…t installer support
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
Claude finished @maxritter's task in 1m 58s —— View job Code Review — PR #62
|
|
Caution Review failedThe pull request is closed. WalkthroughAdds a full settings feature: backend SettingsRoutes (GET/PUT) persisting to ~/.pilot/config.json, a frontend SettingsView with hook and ModelSelect, sidebar route, tests, documentation updates, and installer enhancements for macOS arm64 MLX support and related tooling installs. Changes
Sequence DiagramssequenceDiagram
participant Browser as Browser (SettingsView)
participant Hook as useSettings Hook
participant Server as Express (SettingsRoutes)
participant FS as File System (~/.pilot/config.json)
Browser->>Hook: user changes model selection
Hook->>Hook: update local state (isDirty=true)
Browser->>Hook: user clicks Save
Hook->>Server: PUT /api/settings { ... }
activate Server
Server->>FS: read current config
Server->>Server: validate & merge with defaults
Server->>FS: write temp file then atomic rename
deactivate Server
Server-->>Hook: 200 OK (updated settings)
Hook->>Browser: update state (isDirty=false, saved=true)
sequenceDiagram
participant App as App (mount)
participant Hook as useSettings Hook
participant Server as Express (SettingsRoutes)
participant FS as File System (~/.pilot/config.json)
App->>Hook: mount SettingsView
Hook->>Server: GET /api/settings
activate Server
Server->>FS: read config (or use defaults)
Server->>Server: merge & return settings
deactivate Server
Server-->>Hook: 200 OK {settings}
Hook->>App: provide settings + handlers
App->>App: render SettingsView with values
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (15)
console/tests/hooks/useSettings.test.ts (1)
70-78: Reading the source file to check for string literals is a test antipattern — replace with a behavioral test.This couples the test to the source file path and content rather than verifiable behavior. A refactored fetch or rename breaks the test without breaking functionality, and it can't catch runtime issues.
For a lightweight behavioral check without a DOM, mock
globalThis.fetchand verify the hook calls the right URL and method:it('save() calls PUT /api/settings', async () => { const putPayload: unknown[] = []; globalThis.fetch = async (url: RequestInfo, init?: RequestInit) => { if (init?.method === 'PUT') putPayload.push(JSON.parse(init.body as string)); return { ok: true, json: async () => ({}) } as Response; }; // renderHook or direct invocation ... });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/tests/hooks/useSettings.test.ts` around lines 70 - 78, The test currently reads the source file contents to assert string literals; change it to a behavioral test that mocks globalThis.fetch and verifies the hook's behavior instead. In the test for useSettings, mock fetch to capture requests, render the hook (useSettings) via renderHook or mount, call the hook's save() function, and assert that fetch was called with method 'PUT' and URL '/api/settings' and that the hook's returned state properties (isLoading, isDirty, saved or similar) update appropriately; reference the useSettings hook and its save method/state to locate where to change the test.console/src/ui/viewer/hooks/useSettings.ts (3)
22-39:DEFAULT_SETTINGSis duplicated verbatim inSettingsRoutes.ts— divergence risk.The client-side defaults (used for pre-fetch rendering) and server-side defaults (used for the GET response when no config exists) must stay in sync. If they diverge, users see one set of defaults before the fetch completes and a different set afterward.
Consider extracting shared constants to a single source (e.g., a shared module or generating client defaults from the server response's schema).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/src/ui/viewer/hooks/useSettings.ts` around lines 22 - 39, DEFAULT_SETTINGS in useSettings.ts is duplicated from SettingsRoutes.ts which risks divergence; consolidate the canonical defaults into a single shared export and have both the client hook (DEFAULT_SETTINGS / ModelSettings usage in useSettings.ts) and the server route (SettingsRoutes.ts) import that constant instead of redefining it. Create a shared module (e.g., settingsDefaults or settingsSchema) that exports the ModelSettings-typed DEFAULT_SETTINGS and import it from both places, or alternatively derive the client-side defaults from the server response/schema at build-time; update references in useSettings.ts (DEFAULT_SETTINGS) and SettingsRoutes.ts to import from the new shared module. Ensure the shared export remains the single source of truth and update any tests or types that reference the old local constants.
60-74: Missing cleanup inuseEffect— add anAbortControllerto cancel in-flight fetch on unmount.♻️ Proposed fix
useEffect(() => { - fetch('/api/settings') + const controller = new AbortController(); + fetch('/api/settings', { signal: controller.signal }) .then((r) => { if (!r.ok) throw new Error(`API error: ${r.status}`); return r.json(); }) .then((data: ModelSettings) => { setSettings(data); setIsLoading(false); }) .catch((err: Error) => { + if (err.name === 'AbortError') return; setError(err.message || 'Failed to load settings'); setIsLoading(false); }); + return () => controller.abort(); }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/src/ui/viewer/hooks/useSettings.ts` around lines 60 - 74, The useEffect currently starts a fetch('/api/settings') that may continue after unmount; update it to create an AbortController, pass controller.signal to fetch, and in the cleanup return call controller.abort() to cancel the request; ensure you handle AbortError in the catch so setError/setIsLoading are not incorrectly called when the fetch is aborted, keeping usage of setSettings, setIsLoading, and setError unchanged otherwise.
9-14:MODEL_DISPLAY_NAMEStyped asRecord<string, string>loses exhaustiveness — consider typing asRecord<ModelFull, string>.If a model is added to
MODEL_CHOICES_FULL, TypeScript won't enforce adding a display name entry.♻️ Proposed refactor
-export const MODEL_DISPLAY_NAMES: Record<string, string> = { +export const MODEL_DISPLAY_NAMES: Record<ModelFull, string> = {This requires moving the
ModelFulltype declaration aboveMODEL_DISPLAY_NAMES(currentlyModelFullis at line 6, already before line 9, so the order is fine).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/src/ui/viewer/hooks/useSettings.ts` around lines 9 - 14, Change MODEL_DISPLAY_NAMES to be typed with the specific union type ModelFull instead of Record<string, string> so TypeScript enforces exhaustiveness (e.g., use Record<ModelFull, string> for the MODEL_DISPLAY_NAMES declaration), ensure the ModelFull type is declared above the MODEL_DISPLAY_NAMES constant, and update any places that rely on the broader string key type to accept the narrower ModelFull type.installer/steps/settings_merge.py (1)
40-43: Consider inverting the condition to eliminate thepassbranch.The
pass/elseinversion is correct but somewhat unusual. A direct condition is clearer:♻️ Proposed refactor
- if baseline is not None and in_baseline and current[key] == baseline[key]: - pass - else: - result[key] = current[key] + if baseline is None or not in_baseline or current[key] != baseline[key]: + result[key] = current[key]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@installer/steps/settings_merge.py` around lines 40 - 43, Invert the conditional in the settings_merge logic to remove the no-op pass branch: instead of "if baseline is not None and in_baseline and current[key] == baseline[key]: pass else: result[key] = current[key]" change the test so the branch that assigns into result runs directly when the value differs or baseline is absent. Locate the block using variables baseline, in_baseline, current, key and update the condition to something like "if baseline is None or not in_baseline or current[key] != baseline[key]" and then assign result[key] = current[key]; remove the pass branch entirely.installer/steps/dependencies.py (1)
477-491:install_golangci_lintusesmasterbranch of install script — consider pinning.The install command fetches from
golangci-lint/master/install.shat head, which could change without notice. Pinning to a tagged release (e.g.,v2.1.6/install.sh) would improve reproducibility.This is also a minor security consideration: piping an unpinned remote script to
sh.As per coding guidelines, "Idempotency (can be run multiple times safely)" — currently idempotent via
command_existscheck. Cross-platform: correctly gated oncommand_exists("go").🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@installer/steps/dependencies.py` around lines 477 - 491, The install_golangci_lint function currently downloads the install script from the master branch; change install_cmd to fetch a pinned release tag instead (e.g., use the known release path like vX.Y.Z/install.sh) so the script is reproducible and more secure, and keep the existing command_exists("golangci-lint") and command_exists("go") guards; update the install_cmd string construction (the install_cmd local variable) to reference the chosen tag and ensure the rest of the function still calls _run_bash_with_retry(install_cmd, timeout=120).console/tests/ui/model-routing-info.test.ts (1)
12-38: Source-string assertions are brittle — consider supplementing with render tests.Tests 2-5 verify implementation details by scanning source text (e.g., checking for
'useSettings','??'). These will break on harmless refactors like renaming an import alias or switching from??to a ternary. They're fine as a quick migration guard, but longer-term, a render-based test that mountsModelRoutingInfowith mocked settings would be more resilient.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/tests/ui/model-routing-info.test.ts` around lines 12 - 38, Replace brittle source-text assertions in model-routing-info.test.ts by mounting the ModelRoutingInfo component and asserting rendered output instead of scanning source strings: import and render the ModelRoutingInfo component (symbol: ModelRoutingInfo) with React Testing Library, mock the useSettings hook or provide the settings context to simulate different settings, then assert the DOM contains the settings link ('#/settings'), dynamic model display names (from MODEL_DISPLAY_NAMES) and fallback behavior (DEFAULT_SETTINGS or visible '??' equivalent) under those mocked scenarios; remove the tests that call readFileSync and look for 'useSettings', 'MODEL_DISPLAY_NAMES', 'DEFAULT_SETTINGS', 'Sonnet 4.5', '"Opus 4.6"', and '??' strings and replace them with render-based assertions that query text/roles in the rendered output.installer/tests/unit/steps/test_dependencies.py (1)
842-853: Mock logic is overly obscure but functionally correct.
mock_cmd.side_effect = lambda cmd: cmd != "golangci-lint" and Falseevaluates toFalsefor every input, which is equivalent toreturn_value=False. The test correctly verifies thatinstall_golangci_lintreturnsFalsewhen neithergolangci-lintnorgois in PATH.Simpler equivalent
- mock_cmd.side_effect = lambda cmd: cmd != "golangci-lint" and False + mock_cmd.return_value = False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@installer/tests/unit/steps/test_dependencies.py` around lines 842 - 853, The mock for command_exists in test_install_golangci_lint_skips_without_go is needlessly obscure; replace the side_effect on mock_cmd with a clear explicit return indicating neither binary is present (e.g., set mock_cmd.return_value = False) so the test more obviously simulates absence of both "golangci-lint" and "go" when calling install_golangci_lint; keep the rest of the test (patching _run_bash_with_retry and asserting result is False and mock_run.assert_not_called()) unchanged.console/src/services/worker/http/routes/SettingsRoutes.ts (2)
58-65: Synchronous file reads on the request thread.
readFileSyncblocks the event loop. For a low-traffic settings endpoint this is tolerable, but be aware it'll stall all concurrent requests while reading. If the file grows or sits on a slow mount, latency increases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/src/services/worker/http/routes/SettingsRoutes.ts` around lines 58 - 65, The readConfig function currently uses fs.readFileSync which blocks the event loop; change readConfig to an async function that uses fs.promises.readFile (or util.promisify) to read this.configPath and JSON.parse the result, returning Promise<Record<string, unknown>> and catching errors to return {} on failure; then update all call sites to await readConfig (or handle the returned Promise) so no synchronous file I/O occurs on the request thread.
17-43: Extract model choices and settings into a shared module to avoid duplication.
MODEL_CHOICES_FULL,MODEL_CHOICES_AGENT,ModelSettings, andDEFAULT_SETTINGSare defined identically in bothconsole/src/services/worker/http/routes/SettingsRoutes.tsandconsole/src/ui/viewer/hooks/useSettings.ts. This duplication violates DRY—changes to these values must be made in both places, and the separate test files mean drift could go unnoticed.Consider creating a shared constants file (e.g.,
console/src/shared/settings.ts) imported by both server and client, or have the client derive model choices from the GET/api/settingsresponse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/src/services/worker/http/routes/SettingsRoutes.ts` around lines 17 - 43, Duplicate definitions of MODEL_CHOICES_FULL, MODEL_CHOICES_AGENT, ModelSettings, and DEFAULT_SETTINGS exist in both SettingsRoutes.ts and useSettings.ts; extract these into a single shared module (e.g., a new settings constants module) and import them from both places, or alternatively expose model choices via the GET /api/settings response and have useSettings.ts derive its values from that API; update references to the symbols MODEL_CHOICES_FULL, MODEL_CHOICES_AGENT, ModelSettings, and DEFAULT_SETTINGS in both files to import from the new shared module (or consume the API response) and remove the local duplicated definitions.console/tests/ui/views-index.test.ts (1)
7-20: Consider sharing the module import across tests.Both
itblocks independentlyawait import(...)the same module. While Bun caches it, a sharedlet modhoisted todescribescope is cleaner and avoids re-importing in each test.♻️ Suggested refactor
describe('views/index exports', () => { + let mod: Record<string, unknown>; + beforeAll(async () => { + mod = await import('../../src/ui/viewer/views/index.js'); + }); + it('exports SettingsView', async () => { - const mod = await import('../../src/ui/viewer/views/index.js'); expect(typeof mod.SettingsView).toBe('function'); }); it('exports all existing views', async () => { - const mod = await import('../../src/ui/viewer/views/index.js'); expect(typeof mod.DashboardView).toBe('function'); // ... }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/tests/ui/views-index.test.ts` around lines 7 - 20, Both tests duplicate the same dynamic import; hoist a single module variable (e.g., let mod) to the describe scope and populate it once in a beforeAll() by awaiting import('../../src/ui/viewer/views/index.js'), then update both it blocks to read the shared mod variable (leaving the same assertions for SettingsView, DashboardView, MemoriesView, SessionsView, SpecView, UsageView, VaultView) so the module is imported only once and tests remain identical.console/tests/settings-routes.test.ts (2)
152-273: Missing test: PUT error path whenwriteConfigAtomicfails (HTTP 500 branch).The
handlePutimplementation catcheswriteConfigAtomicerrors and responds withres.status(500).json({ error: ... }), but there is no test exercising this code path. It's straightforward to cover by spying onfs.writeFileSyncto throw.Do you want me to generate the test case for the 500 error path?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/tests/settings-routes.test.ts` around lines 152 - 273, Add a unit test that triggers the 500 error branch by making (routes as any).handlePut throw when attempting to write the config: spyOn fs.writeFileSync (used by writeConfigAtomic) and mock it to throw an Error, then call handlePut with a valid req (e.g., body: { model: 'opus' }) and assert that the mock response (makeMockRes) has statusCode 500 and that m.body contains an error property; reference the handlePut function and the fs.writeFileSync spy to locate where to inject the failure.
97-101:DEFAULT_SETTINGS1M check omits agents.Only
commandsvalues are iterated for the[1m]check.DEFAULT_SETTINGS.agentsvalues should be verified too, since agents are restricted toMODEL_CHOICES_AGENT(no 1M variants) and an accidental future default change could silently pass.♻️ Proposed addition
it('should have no 1M models in defaults', () => { for (const model of Object.values(DEFAULT_SETTINGS.commands)) { expect(model).not.toContain('[1m]'); } + for (const model of Object.values(DEFAULT_SETTINGS.agents)) { + expect(model).not.toContain('[1m]'); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/tests/settings-routes.test.ts` around lines 97 - 101, The test "should have no 1M models in defaults" currently only iterates Object.values(DEFAULT_SETTINGS.commands); extend it to also iterate Object.values(DEFAULT_SETTINGS.agents) and assert each agent value does not contain '[1m]'. Reference DEFAULT_SETTINGS, commands, agents, and MODEL_CHOICES_AGENT in the change so the test covers both defaults categories (commands and agents) to prevent accidental 1M model defaults slipping in.console/tests/ui/SidebarNav.test.ts (1)
12-18: Same source-inspection brittleness asApp.test.ts.Grepping raw TSX source for
'#/settings','Settings', and'lucide:settings'is fragile for the same reasons: refactoring nav items into a data array or changing quote style would break these assertions without breaking production behaviour. This is acceptable as a lightweight smoke test, but worth considering a data-driven check against the nav config array if/when one is introduced.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/tests/ui/SidebarNav.test.ts` around lines 12 - 18, The current test in SidebarNav.test.ts reads raw TSX source of SidebarNav.tsx and greps for literal strings which is brittle; change it to import the SidebarNav component (or the navItems array if one exists) and perform a data-driven or rendered check instead: render SidebarNav (or inspect navItems) and assert there is an item with href '#/settings' or text 'Settings' and/or icon identifier 'lucide:settings' (reference symbols: test name "source includes Settings nav item", the SidebarNav component in SidebarNav.tsx, and any exported navItems array). This removes brittle source-string matching while keeping the intent of verifying the Settings nav entry.console/tests/ui/App.test.ts (1)
12-17: Source-inspection test is brittle — consider a more robust approach.The test reads the raw
.tsxsource file and greps for string literals. This will silently give a false negative if the route path is extracted to a constant (const SETTINGS_ROUTE = '/settings'), or a false positive if the strings appear in a comment. Minimal improvement: check for a structural pattern (e.g., the compiled.jsoutput) or, if a JSDOM-capable environment is available, render and assert on the router tree.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/tests/ui/App.test.ts` around lines 12 - 17, The current test in the App.test spec reads the raw .tsx source and greps for string literals, which is brittle; update the test to verify the router structure instead: import the App component (or its compiled JS output) and either render it with a JSDOM-aware test renderer (e.g., `@testing-library/react`) and assert the presence of the "/settings" route or the SettingsView route component in the rendered router tree, or import the runtime constant (e.g., SETTINGS_ROUTE) and assert it matches the expected value; target the App component and SettingsView/SETTINGS_ROUTE symbols so the test stays correct if routes are factored into constants or comments change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@console/src/ui/viewer/hooks/useSettings.ts`:
- Around line 100-113: The save function (save in useSettings hook) currently
lets errors reject without updating hook state; update save to catch failures
and set an error state (either reuse setError or add a new saveError state) so
callers get UI feedback instead of unhandled rejections, and ensure the hook's
return type (UseSettingsResult) includes saveError: string | null; specifically
wrap the fetch/then chain in try/catch (or append .catch) to call setError or
setSaveError with a descriptive message and still manage setIsDirty/setSaved
only on success.
In `@console/src/ui/viewer/views/Settings/index.tsx`:
- Around line 182-184: The context-display cell uses (settings.commands[cmd] ??
'') while the ModelSelect uses (settings.commands[cmd] ??
DEFAULT_SETTINGS.commands[cmd]); update the context cell to use the same
fallback so it reflects defaults correctly — i.e., read the model string via
(settings.commands[cmd] ?? DEFAULT_SETTINGS.commands[cmd]) when computing the
includes('[1m]') check; locate the ModelSelect and the <td className="text-xs
text-base-content/40"> context cell in Settings/index.tsx and make their source
of truth consistent (both using DEFAULT_SETTINGS.commands[cmd] as the fallback).
In `@console/src/ui/viewer/views/Settings/ModelSelect.tsx`:
- Around line 4-10: The ModelSelect component lacks an accessible name when no
id/label pair is provided; update ModelSelectProps to accept an optional
ariaLabel?: string, update the ModelSelect functional component to pass
aria-label={ariaLabel} to the rendered <select> (only when id is not present or
always pass it), and ensure the onChange/value/disabled behavior remains
unchanged; reference the ModelSelectProps interface and the ModelSelect
component's <select> element to locate where to add the ariaLabel prop and
attribute.
In `@console/src/ui/viewer/views/Usage/ModelRoutingInfo.tsx`:
- Around line 62-64: The paragraph contains a hardcoded sentence about routing
defaults; update it to reflect live configuration by deriving the text from the
computed cfg (e.g., use cfg.planning, cfg.verification, cfg.implementation or
iterate cfg.phases) instead of the static "Opus...Sonnet" string, or remove the
sentence entirely; locate the JSX where mainModel is used in
ModelRoutingInfo.tsx and replace the hardcoded copy with a string constructed
from cfg values so the displayed defaults update when the user changes
configuration.
In `@console/tests/infrastructure/process-manager.test.ts`:
- Around line 236-250: The test should assert setup succeeded before calling
cleanStalePidFile and then assert the PID file was removed; specifically, after
writePidFile(PidInfo) add an assertion using existsSync(PID_FILE) and/or
readPidFile() to verify the file exists and contains pid 2147483647, then call
cleanStalePidFile() and assert the file no longer exists (existsSync(PID_FILE)
is false) or readPidFile() returns undefined/null; update the assertions around
writePidFile, readPidFile, existsSync, and PID_FILE to reflect this positive
setup check and a definitive post-cleanup check.
In `@console/tests/settings-routes.test.ts`:
- Around line 260-261: The assertion is trivially true because "|| p !==
configPath" matches any path not exactly equal to configPath; update the check
on writtenPaths so it specifically looks for the temporary write path instead of
allowing any unrelated path. Replace the predicate in the hasTmp calculation
(referencing writtenPaths, configPath, hasTmp) with one that requires the path
to include the temp marker and not equal configPath (e.g., p.includes('.tmp') &&
p !== configPath) or simply p.includes('.tmp'), then assert that hasTmp is true.
- Around line 248-252: The test fails TS2556 because mockImplementation uses a
loose any[] spread against overloaded fs.writeFileSync; change the mock to use
the exact parameter tuple type: replace the current mockImplementation((p:
fs.PathOrFileDescriptor, ...args: any[]) => {...}) with
mockImplementation((...args: Parameters<typeof fs.writeFileSync>) => {
writtenPaths.push(String(args[0])); return origWriteFileSync(...args); }); this
uses Parameters<typeof fs.writeFileSync> and ensures origWriteFileSync (bound
via fs.writeFileSync.bind(fs)) is called with correctly-typed arguments while
still recording the written path.
In `@docs/site/src/content/blog/model-selection.md`:
- Line 175: Update the "Pilot Console" link text to clarify it only works when
Pilot is running locally and remove the redundant inline-code URL; specifically
change the hyperlink "[Pilot Console](http://localhost:41777/#/settings)" to
something like "Pilot Console (requires Pilot running locally)" and delete the
repeated `localhost:41777/#/settings` inline code, or replace the link with a
documentation/hosted URL if one exists; ensure the sentence still tells users
they can set models per `/spec` phase, slash command, and sub-agent and that
restarting Pilot applies changes.
In `@installer/steps/dependencies.py`:
- Around line 293-333: The _clone_vexor_fork function currently ignores failures
when updating an existing clone; change the three subprocess.run calls that
perform ["git", "fetch", "checkout", "pull"] to either use check=True or inspect
result.returncode, capture text output, and on non-zero exit log a warning
(including stdout/stderr and VEXOR_MLX_BRANCH/VEXOR_FORK_URL context) and return
None instead of silently returning vexor_dir so downstream
_install_vexor_from_local won't install from a potentially stale copy; keep the
existing safe-create/clone path intact and only alter the update branch to
surface errors to the user.
---
Nitpick comments:
In `@console/src/services/worker/http/routes/SettingsRoutes.ts`:
- Around line 58-65: The readConfig function currently uses fs.readFileSync
which blocks the event loop; change readConfig to an async function that uses
fs.promises.readFile (or util.promisify) to read this.configPath and JSON.parse
the result, returning Promise<Record<string, unknown>> and catching errors to
return {} on failure; then update all call sites to await readConfig (or handle
the returned Promise) so no synchronous file I/O occurs on the request thread.
- Around line 17-43: Duplicate definitions of MODEL_CHOICES_FULL,
MODEL_CHOICES_AGENT, ModelSettings, and DEFAULT_SETTINGS exist in both
SettingsRoutes.ts and useSettings.ts; extract these into a single shared module
(e.g., a new settings constants module) and import them from both places, or
alternatively expose model choices via the GET /api/settings response and have
useSettings.ts derive its values from that API; update references to the symbols
MODEL_CHOICES_FULL, MODEL_CHOICES_AGENT, ModelSettings, and DEFAULT_SETTINGS in
both files to import from the new shared module (or consume the API response)
and remove the local duplicated definitions.
In `@console/src/ui/viewer/hooks/useSettings.ts`:
- Around line 22-39: DEFAULT_SETTINGS in useSettings.ts is duplicated from
SettingsRoutes.ts which risks divergence; consolidate the canonical defaults
into a single shared export and have both the client hook (DEFAULT_SETTINGS /
ModelSettings usage in useSettings.ts) and the server route (SettingsRoutes.ts)
import that constant instead of redefining it. Create a shared module (e.g.,
settingsDefaults or settingsSchema) that exports the ModelSettings-typed
DEFAULT_SETTINGS and import it from both places, or alternatively derive the
client-side defaults from the server response/schema at build-time; update
references in useSettings.ts (DEFAULT_SETTINGS) and SettingsRoutes.ts to import
from the new shared module. Ensure the shared export remains the single source
of truth and update any tests or types that reference the old local constants.
- Around line 60-74: The useEffect currently starts a fetch('/api/settings')
that may continue after unmount; update it to create an AbortController, pass
controller.signal to fetch, and in the cleanup return call controller.abort() to
cancel the request; ensure you handle AbortError in the catch so
setError/setIsLoading are not incorrectly called when the fetch is aborted,
keeping usage of setSettings, setIsLoading, and setError unchanged otherwise.
- Around line 9-14: Change MODEL_DISPLAY_NAMES to be typed with the specific
union type ModelFull instead of Record<string, string> so TypeScript enforces
exhaustiveness (e.g., use Record<ModelFull, string> for the MODEL_DISPLAY_NAMES
declaration), ensure the ModelFull type is declared above the
MODEL_DISPLAY_NAMES constant, and update any places that rely on the broader
string key type to accept the narrower ModelFull type.
In `@console/tests/hooks/useSettings.test.ts`:
- Around line 70-78: The test currently reads the source file contents to assert
string literals; change it to a behavioral test that mocks globalThis.fetch and
verifies the hook's behavior instead. In the test for useSettings, mock fetch to
capture requests, render the hook (useSettings) via renderHook or mount, call
the hook's save() function, and assert that fetch was called with method 'PUT'
and URL '/api/settings' and that the hook's returned state properties
(isLoading, isDirty, saved or similar) update appropriately; reference the
useSettings hook and its save method/state to locate where to change the test.
In `@console/tests/settings-routes.test.ts`:
- Around line 152-273: Add a unit test that triggers the 500 error branch by
making (routes as any).handlePut throw when attempting to write the config:
spyOn fs.writeFileSync (used by writeConfigAtomic) and mock it to throw an
Error, then call handlePut with a valid req (e.g., body: { model: 'opus' }) and
assert that the mock response (makeMockRes) has statusCode 500 and that m.body
contains an error property; reference the handlePut function and the
fs.writeFileSync spy to locate where to inject the failure.
- Around line 97-101: The test "should have no 1M models in defaults" currently
only iterates Object.values(DEFAULT_SETTINGS.commands); extend it to also
iterate Object.values(DEFAULT_SETTINGS.agents) and assert each agent value does
not contain '[1m]'. Reference DEFAULT_SETTINGS, commands, agents, and
MODEL_CHOICES_AGENT in the change so the test covers both defaults categories
(commands and agents) to prevent accidental 1M model defaults slipping in.
In `@console/tests/ui/App.test.ts`:
- Around line 12-17: The current test in the App.test spec reads the raw .tsx
source and greps for string literals, which is brittle; update the test to
verify the router structure instead: import the App component (or its compiled
JS output) and either render it with a JSDOM-aware test renderer (e.g.,
`@testing-library/react`) and assert the presence of the "/settings" route or the
SettingsView route component in the rendered router tree, or import the runtime
constant (e.g., SETTINGS_ROUTE) and assert it matches the expected value; target
the App component and SettingsView/SETTINGS_ROUTE symbols so the test stays
correct if routes are factored into constants or comments change.
In `@console/tests/ui/model-routing-info.test.ts`:
- Around line 12-38: Replace brittle source-text assertions in
model-routing-info.test.ts by mounting the ModelRoutingInfo component and
asserting rendered output instead of scanning source strings: import and render
the ModelRoutingInfo component (symbol: ModelRoutingInfo) with React Testing
Library, mock the useSettings hook or provide the settings context to simulate
different settings, then assert the DOM contains the settings link
('#/settings'), dynamic model display names (from MODEL_DISPLAY_NAMES) and
fallback behavior (DEFAULT_SETTINGS or visible '??' equivalent) under those
mocked scenarios; remove the tests that call readFileSync and look for
'useSettings', 'MODEL_DISPLAY_NAMES', 'DEFAULT_SETTINGS', 'Sonnet 4.5', '"Opus
4.6"', and '??' strings and replace them with render-based assertions that query
text/roles in the rendered output.
In `@console/tests/ui/SidebarNav.test.ts`:
- Around line 12-18: The current test in SidebarNav.test.ts reads raw TSX source
of SidebarNav.tsx and greps for literal strings which is brittle; change it to
import the SidebarNav component (or the navItems array if one exists) and
perform a data-driven or rendered check instead: render SidebarNav (or inspect
navItems) and assert there is an item with href '#/settings' or text 'Settings'
and/or icon identifier 'lucide:settings' (reference symbols: test name "source
includes Settings nav item", the SidebarNav component in SidebarNav.tsx, and any
exported navItems array). This removes brittle source-string matching while
keeping the intent of verifying the Settings nav entry.
In `@console/tests/ui/views-index.test.ts`:
- Around line 7-20: Both tests duplicate the same dynamic import; hoist a single
module variable (e.g., let mod) to the describe scope and populate it once in a
beforeAll() by awaiting import('../../src/ui/viewer/views/index.js'), then
update both it blocks to read the shared mod variable (leaving the same
assertions for SettingsView, DashboardView, MemoriesView, SessionsView,
SpecView, UsageView, VaultView) so the module is imported only once and tests
remain identical.
In `@installer/steps/dependencies.py`:
- Around line 477-491: The install_golangci_lint function currently downloads
the install script from the master branch; change install_cmd to fetch a pinned
release tag instead (e.g., use the known release path like vX.Y.Z/install.sh) so
the script is reproducible and more secure, and keep the existing
command_exists("golangci-lint") and command_exists("go") guards; update the
install_cmd string construction (the install_cmd local variable) to reference
the chosen tag and ensure the rest of the function still calls
_run_bash_with_retry(install_cmd, timeout=120).
In `@installer/steps/settings_merge.py`:
- Around line 40-43: Invert the conditional in the settings_merge logic to
remove the no-op pass branch: instead of "if baseline is not None and
in_baseline and current[key] == baseline[key]: pass else: result[key] =
current[key]" change the test so the branch that assigns into result runs
directly when the value differs or baseline is absent. Locate the block using
variables baseline, in_baseline, current, key and update the condition to
something like "if baseline is None or not in_baseline or current[key] !=
baseline[key]" and then assign result[key] = current[key]; remove the pass
branch entirely.
In `@installer/tests/unit/steps/test_dependencies.py`:
- Around line 842-853: The mock for command_exists in
test_install_golangci_lint_skips_without_go is needlessly obscure; replace the
side_effect on mock_cmd with a clear explicit return indicating neither binary
is present (e.g., set mock_cmd.return_value = False) so the test more obviously
simulates absence of both "golangci-lint" and "go" when calling
install_golangci_lint; keep the rest of the test (patching _run_bash_with_retry
and asserting result is False and mock_run.assert_not_called()) unchanged.
| const save = useCallback(async () => { | ||
| await fetch('/api/settings', { | ||
| method: 'PUT', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify(settings), | ||
| }).then((r) => { | ||
| if (!r.ok) throw new Error(`Save failed: ${r.status}`); | ||
| return r.json(); | ||
| }).then((data: ModelSettings) => { | ||
| setSettings(data); | ||
| setIsDirty(false); | ||
| setSaved(true); | ||
| }); | ||
| }, [settings]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
save() errors are not surfaced through the hook's error state — callers must catch the rejected Promise.
On a failed PUT (network error or !r.ok), the error propagates as a rejected Promise but setError() is never called. A call-site like <button onClick={() => save()}> will produce an unhandled rejection with no UI feedback. Either expose a saveError state, or catch internally and update error:
♻️ Proposed fix (expose saveError or update error on failure)
+ const [saveError, setSaveError] = useState<string | null>(null);
const save = useCallback(async () => {
- await fetch('/api/settings', {
- method: 'PUT',
- headers: { 'Content-Type': 'application/json' },
- body: JSON.stringify(settings),
- }).then((r) => {
- if (!r.ok) throw new Error(`Save failed: ${r.status}`);
- return r.json();
- }).then((data: ModelSettings) => {
- setSettings(data);
- setIsDirty(false);
- setSaved(true);
- });
+ setSaveError(null);
+ try {
+ const r = await fetch('/api/settings', {
+ method: 'PUT',
+ headers: { 'Content-Type': 'application/json' },
+ body: JSON.stringify(settings),
+ });
+ if (!r.ok) throw new Error(`Save failed: ${r.status}`);
+ const data: ModelSettings = await r.json();
+ setSettings(data);
+ setIsDirty(false);
+ setSaved(true);
+ } catch (err) {
+ setSaveError((err as Error).message ?? 'Save failed');
+ }
}, [settings]);
- return { settings, isLoading, error, isDirty, saved, updateModel, updateCommand, updateAgent, save };
+ return { settings, isLoading, error, saveError, isDirty, saved, updateModel, updateCommand, updateAgent, save };Remember to add saveError: string | null to UseSettingsResult.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@console/src/ui/viewer/hooks/useSettings.ts` around lines 100 - 113, The save
function (save in useSettings hook) currently lets errors reject without
updating hook state; update save to catch failures and set an error state
(either reuse setError or add a new saveError state) so callers get UI feedback
instead of unhandled rejections, and ensure the hook's return type
(UseSettingsResult) includes saveError: string | null; specifically wrap the
fetch/then chain in try/catch (or append .catch) to call setError or
setSaveError with a descriptive message and still manage setIsDirty/setSaved
only on success.
| <td className="text-xs text-base-content/40"> | ||
| {(settings.commands[cmd] ?? '').includes('[1m]') ? '1M' : '200K'} | ||
| </td> |
There was a problem hiding this comment.
Context column fallback is inconsistent with the value source.
Line 176 falls back to DEFAULT_SETTINGS.commands[cmd] for the <ModelSelect> value, but line 183 falls back to '' for the context display. If settings.commands[cmd] were ever undefined (e.g., a new command added to defaults but not yet in the user's saved config), the selector would show the correct default model but the context column would always show "200K" even if the default were a [1m] variant.
Currently none of the defaults use [1m], and the server merges defaults before returning, so this isn't actively broken — but the inconsistency is fragile.
Proposed fix
<td className="text-xs text-base-content/40">
- {(settings.commands[cmd] ?? '').includes('[1m]') ? '1M' : '200K'}
+ {(settings.commands[cmd] ?? DEFAULT_SETTINGS.commands[cmd] ?? '').includes('[1m]') ? '1M' : '200K'}
</td>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <td className="text-xs text-base-content/40"> | |
| {(settings.commands[cmd] ?? '').includes('[1m]') ? '1M' : '200K'} | |
| </td> | |
| <td className="text-xs text-base-content/40"> | |
| {(settings.commands[cmd] ?? DEFAULT_SETTINGS.commands[cmd] ?? '').includes('[1m]') ? '1M' : '200K'} | |
| </td> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@console/src/ui/viewer/views/Settings/index.tsx` around lines 182 - 184, The
context-display cell uses (settings.commands[cmd] ?? '') while the ModelSelect
uses (settings.commands[cmd] ?? DEFAULT_SETTINGS.commands[cmd]); update the
context cell to use the same fallback so it reflects defaults correctly — i.e.,
read the model string via (settings.commands[cmd] ??
DEFAULT_SETTINGS.commands[cmd]) when computing the includes('[1m]') check;
locate the ModelSelect and the <td className="text-xs text-base-content/40">
context cell in Settings/index.tsx and make their source of truth consistent
(both using DEFAULT_SETTINGS.commands[cmd] as the fallback).
| interface ModelSelectProps { | ||
| value: string; | ||
| choices: readonly string[]; | ||
| onChange: (model: string) => void; | ||
| disabled?: boolean; | ||
| id?: string; | ||
| } |
There was a problem hiding this comment.
Add aria-label prop as a fallback when no id/label pair is guaranteed.
id is optional, and the <select> has no aria-label. If a caller omits id (or renders the component without a matching <label htmlFor>), screen readers have no way to announce the field's purpose.
♻️ Proposed fix
interface ModelSelectProps {
value: string;
choices: readonly string[];
onChange: (model: string) => void;
disabled?: boolean;
id?: string;
+ ariaLabel?: string;
}
-export function ModelSelect({ value, choices, onChange, disabled = false, id }: ModelSelectProps) {
+export function ModelSelect({ value, choices, onChange, disabled = false, id, ariaLabel }: ModelSelectProps) {
return (
<select
id={id}
+ aria-label={ariaLabel}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@console/src/ui/viewer/views/Settings/ModelSelect.tsx` around lines 4 - 10,
The ModelSelect component lacks an accessible name when no id/label pair is
provided; update ModelSelectProps to accept an optional ariaLabel?: string,
update the ModelSelect functional component to pass aria-label={ariaLabel} to
the rendered <select> (only when id is not present or always pass it), and
ensure the onChange/value/disabled behavior remains unchanged; reference the
ModelSelectProps interface and the ModelSelect component's <select> element to
locate where to add the ariaLabel prop and attribute.
| Quick Mode uses <span className="font-mono">{mainModel}</span>. | ||
| Routing defaults use Opus for planning and verification orchestration, Sonnet for implementation. | ||
| </p> |
There was a problem hiding this comment.
Hardcoded description of defaults remains static after user configuration changes.
When a user customizes all phases to Sonnet, the paragraph still reads "Routing defaults use Opus for planning and verification orchestration, Sonnet for implementation." Consider deriving this text from the live cfg values (already computed above) or removing the prescriptive sentence since the table already communicates the current routing:
♻️ Suggested fix
- Routing defaults use Opus for planning and verification orchestration, Sonnet for implementation.
+ {planModel === verifyModel
+ ? `${planModel} is used for planning and verification orchestration; ${implModel} for implementation.`
+ : `Planning and verification use ${planModel}/${verifyModel}; implementation uses ${implModel}.`}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Quick Mode uses <span className="font-mono">{mainModel}</span>. | |
| Routing defaults use Opus for planning and verification orchestration, Sonnet for implementation. | |
| </p> | |
| Quick Mode uses <span className="font-mono">{mainModel}</span>. | |
| {planModel === verifyModel | |
| ? `${planModel} is used for planning and verification orchestration; ${implModel} for implementation.` | |
| : `Planning and verification use ${planModel}/${verifyModel}; implementation uses ${implModel}.`} | |
| </p> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@console/src/ui/viewer/views/Usage/ModelRoutingInfo.tsx` around lines 62 - 64,
The paragraph contains a hardcoded sentence about routing defaults; update it to
reflect live configuration by deriving the text from the computed cfg (e.g., use
cfg.planning, cfg.verification, cfg.implementation or iterate cfg.phases)
instead of the static "Opus...Sonnet" string, or remove the sentence entirely;
locate the JSX where mainModel is used in ModelRoutingInfo.tsx and replace the
hardcoded copy with a string constructed from cfg values so the displayed
defaults update when the user changes configuration.
| it('should remove PID file if process is dead', () => { | ||
| const testInfo: PidInfo = { | ||
| pid: 999999, | ||
| pid: 2147483647, | ||
| port: 41777, | ||
| startedAt: new Date().toISOString() | ||
| }; | ||
| writePidFile(testInfo); | ||
|
|
||
| cleanStalePidFile(); | ||
|
|
||
| expect(existsSync(PID_FILE)).toBe(false); | ||
| if (existsSync(PID_FILE)) { | ||
| const current = readPidFile(); | ||
| expect(current?.pid).not.toBe(2147483647); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Weakened assertion no longer positively verifies stale-file removal
The test is named "should remove PID file if process is dead," but on the success path (file correctly removed), existsSync(PID_FILE) is false, the if block is skipped, and zero assertions execute — the test passes vacuously. The assertion only fires when the file still exists after cleanup, which is the failure path. This means:
- A broken
cleanStalePidFilethat silently no-ops would pass this test, as long as something else writes a new PID file concurrently (makingcurrent?.pid≠2147483647). - If
writePidFilein the setup also silently fails,existsSyncis false from the start and the test again passes trivially without ever exercising the function.
A more robust pattern would add a pre-assertion to confirm setup succeeded and then assert the file is gone after cleanup (accepting parallel-test ambiguity only as a documented exception, not silently):
🛡️ Proposed fix
writePidFile(testInfo);
+
+ // Verify setup: the file must exist before we test cleanup
+ expect(existsSync(PID_FILE)).toBe(true);
+
cleanStalePidFile();
- if (existsSync(PID_FILE)) {
- const current = readPidFile();
- expect(current?.pid).not.toBe(2147483647);
- }
+ // After cleanup the file should be gone, OR (if a parallel test raced in)
+ // the stored PID must be different from the dead one we wrote.
+ if (existsSync(PID_FILE)) {
+ const current = readPidFile();
+ expect(current?.pid).not.toBe(2147483647);
+ } else {
+ // Primary success path: stale file was removed
+ expect(existsSync(PID_FILE)).toBe(false);
+ }The change at line 238 using 2147483647 (INT32_MAX) as a dead-PID sentinel is a good improvement — it's well outside valid PID ranges on all supported platforms.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@console/tests/infrastructure/process-manager.test.ts` around lines 236 - 250,
The test should assert setup succeeded before calling cleanStalePidFile and then
assert the PID file was removed; specifically, after writePidFile(PidInfo) add
an assertion using existsSync(PID_FILE) and/or readPidFile() to verify the file
exists and contains pid 2147483647, then call cleanStalePidFile() and assert the
file no longer exists (existsSync(PID_FILE) is false) or readPidFile() returns
undefined/null; update the assertions around writePidFile, readPidFile,
existsSync, and PID_FILE to reflect this positive setup check and a definitive
post-cleanup check.
| const origWriteFileSync = fs.writeFileSync.bind(fs); | ||
| const spy = spyOn(fs, 'writeFileSync').mockImplementation((p: fs.PathOrFileDescriptor, ...args: any[]) => { | ||
| writtenPaths.push(String(p)); | ||
| return origWriteFileSync(p, ...args); | ||
| }); |
There was a problem hiding this comment.
Pipeline failure: fix TS2556 — cannot spread any[] into overloaded fs.writeFileSync.
fs.writeFileSync has multiple overloaded signatures and TypeScript cannot resolve which overload to use when the rest of the arguments is typed as any[]. This is the root cause of the reported TS2556 CI failure.
🐛 Proposed fix
- const spy = spyOn(fs, 'writeFileSync').mockImplementation((p: fs.PathOrFileDescriptor, ...args: any[]) => {
- writtenPaths.push(String(p));
- return origWriteFileSync(p, ...args);
- });
+ const spy = spyOn(fs, 'writeFileSync').mockImplementation((p: fs.PathOrFileDescriptor, ...args: any[]) => {
+ writtenPaths.push(String(p));
+ return (origWriteFileSync as (p: fs.PathOrFileDescriptor, ...a: any[]) => void)(p, ...args);
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const origWriteFileSync = fs.writeFileSync.bind(fs); | |
| const spy = spyOn(fs, 'writeFileSync').mockImplementation((p: fs.PathOrFileDescriptor, ...args: any[]) => { | |
| writtenPaths.push(String(p)); | |
| return origWriteFileSync(p, ...args); | |
| }); | |
| const origWriteFileSync = fs.writeFileSync.bind(fs); | |
| const spy = spyOn(fs, 'writeFileSync').mockImplementation((p: fs.PathOrFileDescriptor, ...args: any[]) => { | |
| writtenPaths.push(String(p)); | |
| return (origWriteFileSync as (p: fs.PathOrFileDescriptor, ...a: any[]) => void)(p, ...args); | |
| }); |
🧰 Tools
🪛 GitHub Actions: Dev Pre-release
[error] 251-251: TypeScript error TS2556: A spread argument must either have a tuple type or be passed to a rest parameter. (Occurred during bun run typecheck with 'tsc --noEmit')
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@console/tests/settings-routes.test.ts` around lines 248 - 252, The test fails
TS2556 because mockImplementation uses a loose any[] spread against overloaded
fs.writeFileSync; change the mock to use the exact parameter tuple type: replace
the current mockImplementation((p: fs.PathOrFileDescriptor, ...args: any[]) =>
{...}) with mockImplementation((...args: Parameters<typeof fs.writeFileSync>) =>
{ writtenPaths.push(String(args[0])); return origWriteFileSync(...args); });
this uses Parameters<typeof fs.writeFileSync> and ensures origWriteFileSync
(bound via fs.writeFileSync.bind(fs)) is called with correctly-typed arguments
while still recording the written path.
| ## [Model Configuration](#model-configuration) | ||
|
|
||
| Claude Code gives you multiple ways to set your model, with a clear priority chain. Higher-priority settings override lower ones: | ||
| **If you use Claude Pilot:** The easiest way to configure models is through the [Pilot Console](http://localhost:41777/#/settings) (`localhost:41777/#/settings`). You can set a different model for each `/spec` phase, slash command, and sub-agent, then restart Pilot to apply. Pilot injects your preferences into all the right places automatically — no manual file editing needed. |
There was a problem hiding this comment.
Localhost link in public docs is dead for readers not running Pilot.
The hyperlink [Pilot Console](http://localhost:41777/#/settings) only resolves when Pilot is actively running on the reader's machine. Readers browsing the public docs site without Pilot running will land on a connection-refused page. Consider adding a brief parenthetical, e.g. "(requires Pilot to be running locally)", and simplifying the redundant inline-code repetition of the same URL.
✏️ Suggested wording
-**If you use Claude Pilot:** The easiest way to configure models is through the [Pilot Console](http://localhost:41777/#/settings) (`localhost:41777/#/settings`). You can set a different model for each `/spec` phase, slash command, and sub-agent, then restart Pilot to apply. Pilot injects your preferences into all the right places automatically — no manual file editing needed.
+**If you use Claude Pilot:** The easiest way to configure models is through the Pilot Console at `localhost:41777/#/settings` (requires Pilot to be running locally). You can set a different model for each `/spec` phase, slash command, and sub-agent, then restart Pilot to apply. Pilot injects your preferences into all the right places automatically — no manual file editing needed.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| **If you use Claude Pilot:** The easiest way to configure models is through the [Pilot Console](http://localhost:41777/#/settings) (`localhost:41777/#/settings`). You can set a different model for each `/spec` phase, slash command, and sub-agent, then restart Pilot to apply. Pilot injects your preferences into all the right places automatically — no manual file editing needed. | |
| **If you use Claude Pilot:** The easiest way to configure models is through the Pilot Console at `localhost:41777/#/settings` (requires Pilot to be running locally). You can set a different model for each `/spec` phase, slash command, and sub-agent, then restart Pilot to apply. Pilot injects your preferences into all the right places automatically — no manual file editing needed. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/site/src/content/blog/model-selection.md` at line 175, Update the "Pilot
Console" link text to clarify it only works when Pilot is running locally and
remove the redundant inline-code URL; specifically change the hyperlink "[Pilot
Console](http://localhost:41777/#/settings)" to something like "Pilot Console
(requires Pilot running locally)" and delete the repeated
`localhost:41777/#/settings` inline code, or replace the link with a
documentation/hosted URL if one exists; ensure the sentence still tells users
they can set models per `/spec` phase, slash command, and sub-agent and that
restarting Pilot applies changes.
| def _clone_vexor_fork() -> Path | None: | ||
| """Clone the vexor fork with MLX support to ~/.pilot/vexor.""" | ||
| vexor_dir = Path.home() / ".pilot" / "vexor" | ||
|
|
||
| if vexor_dir.exists(): | ||
| try: | ||
| subprocess.run( | ||
| ["git", "fetch", "origin", VEXOR_MLX_BRANCH], | ||
| capture_output=True, | ||
| cwd=vexor_dir, | ||
| timeout=60, | ||
| ) | ||
| subprocess.run( | ||
| ["git", "checkout", VEXOR_MLX_BRANCH], | ||
| capture_output=True, | ||
| cwd=vexor_dir, | ||
| timeout=30, | ||
| ) | ||
| subprocess.run( | ||
| ["git", "pull", "origin", VEXOR_MLX_BRANCH], | ||
| capture_output=True, | ||
| cwd=vexor_dir, | ||
| timeout=60, | ||
| ) | ||
| return vexor_dir | ||
| except Exception: | ||
| return None | ||
|
|
||
| try: | ||
| vexor_dir.parent.mkdir(parents=True, exist_ok=True) | ||
| result = subprocess.run( | ||
| ["git", "clone", "--branch", VEXOR_MLX_BRANCH, "--single-branch", VEXOR_FORK_URL, str(vexor_dir)], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=120, | ||
| ) | ||
| if result.returncode == 0: | ||
| return vexor_dir | ||
| except Exception: | ||
| pass | ||
| return None |
There was a problem hiding this comment.
Silent git failures when updating an existing clone.
When ~/.pilot/vexor already exists (lines 298-319), all three subprocess.run calls (fetch, checkout, pull) lack check=True and their return codes are never inspected. If any of them fail (e.g., network down), execution continues silently and returns the possibly-stale directory at line 317.
This is likely intentional as a resilience choice (a stale clone is better than no clone), but it means the subsequent _install_vexor_from_local may install from outdated sources without any warning.
Optional: log a warning on non-zero exit
subprocess.run(
["git", "fetch", "origin", VEXOR_MLX_BRANCH],
capture_output=True,
cwd=vexor_dir,
timeout=60,
)
- subprocess.run(
+ checkout = subprocess.run(
["git", "checkout", VEXOR_MLX_BRANCH],
capture_output=True,
cwd=vexor_dir,
timeout=30,
)
- subprocess.run(
+ pull = subprocess.run(
["git", "pull", "origin", VEXOR_MLX_BRANCH],
capture_output=True,
cwd=vexor_dir,
timeout=60,
)
+ if checkout.returncode != 0 or pull.returncode != 0:
+ # Stale clone may still work; proceed with best-effort
+ pass
return vexor_dirAs per coding guidelines, "Proper error handling and user feedback" should be ensured for installer code.
🧰 Tools
🪛 Ruff (0.15.1)
[error] 299-299: subprocess call: check for execution of untrusted input
(S603)
[error] 300-300: Starting a process with a partial executable path
(S607)
[error] 305-305: subprocess call: check for execution of untrusted input
(S603)
[error] 306-306: Starting a process with a partial executable path
(S607)
[error] 311-311: subprocess call: check for execution of untrusted input
(S603)
[error] 312-312: Starting a process with a partial executable path
(S607)
[warning] 317-317: Consider moving this statement to an else block
(TRY300)
[warning] 318-318: Do not catch blind exception: Exception
(BLE001)
[error] 323-323: subprocess call: check for execution of untrusted input
(S603)
[error] 324-324: Starting a process with a partial executable path
(S607)
[error] 331-332: try-except-pass detected, consider logging the exception
(S110)
[warning] 331-331: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@installer/steps/dependencies.py` around lines 293 - 333, The
_clone_vexor_fork function currently ignores failures when updating an existing
clone; change the three subprocess.run calls that perform ["git", "fetch",
"checkout", "pull"] to either use check=True or inspect result.returncode,
capture text output, and on non-zero exit log a warning (including stdout/stderr
and VEXOR_MLX_BRANCH/VEXOR_FORK_URL context) and return None instead of silently
returning vexor_dir so downstream _install_vexor_from_local won't install from a
potentially stale copy; keep the existing safe-create/clone path intact and only
alter the update branch to surface errors to the user.
…isolation - Fix command injection writing to wrong path (~/.claude/pilot/commands/ instead of ~/.claude/commands/) - Inject model into global ~/.claude/settings.json so Console settings always override installer three-way merge - Add model_config.py, settings_injector.py, tips.py to Cython MODULE_ORDER - Fix multi-line import strip regex eating next line's indentation - Restore accidentally deleted statusline/tips.py - Fix TS2556 spread argument error in settings-routes test - Default model changed to Opus 4.6 - Reorganize Settings UI: General + Spec Flow sections, default column, sticky save bar, collapsible pricing, 1M subscription note - Remove ModelRoutingInfo from Usage page, increase chart height - Add Settings screenshot to website console carousel - Mock get_max_context_tokens in tests for environment isolation - Update research-tools rule for WebFetch/WebSearch blocking
|
Claude finished @maxritter's task in 1m 6s —— View job Incremental Code Review — PR #62
Incremental Review (new push)✅ Resolved from prior review#9 — Atomic write test assertion fixed ( expect(writtenPaths.some(p => p.includes('.tmp'))).toBe(true);This is correct. ✅ #5 — TS2556 fix — atomic write spy ( Still open (unresolved from prior review)
CI workflows are still in progress — check back once the |
# [6.8.0](v6.7.7...v6.8.0) (2026-02-18) ### Features * model selection settings, Apple Silicon Vexor acceleration, and worktree sync fixes ([#62](#62)) ([4e3bfb2](4e3bfb2))
|
🎉 This PR is included in version 6.8.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
1 participant
Summary
localhost:41777/#/settings) lets users configure the model for every component: main session (Quick Mode), each/speccommand phase, and each sub-agent. Preferences are stored in~/.pilot/config.jsonand automatically injected into the installed plugin files on every Pilot startup. Defaults to Sonnet 4.6 for all components.cdare now combined in a single shell invocation.prettierandgolangci-lintinstaller support.vexor searchcommand syntax in rules (was missing thesearchsubcommand), clarified Bash tooltimeoutparameter.Changes
Console
SettingsRoutes.ts—GET/PUT /api/settingswith validation and atomic writesModelRoutingInfoin Usage tab now shows dynamic model names from user configLauncher
model_config.py— Config schema, read/write utilities, validationsettings_injector.py— Injects model preferences into~/.claude/pilot/on startupconfig.py— Dynamicget_max_context_tokens()andget_compaction_threshold_pct()worktree.py— Removed auto-stash fromsync_worktree, removedos.chdirfromcleanup_worktreePlugin
pilot/settings.json— Default model set tosonnet(Sonnet 4.6)sonnet/opusdefaultsTest plan
uv run pytest -qcd console && bun testlocalhost:41777/#/settings— verify Settings page loads with dropdowns~/.claude/pilot/settings.jsonupdatedSummary by CodeRabbit
New Features
Documentation
Tests