Skip to content

feat: global extended context toggle and compact settings UI#63

Merged
maxritter merged 4 commits intomainfrom
dev
Feb 18, 2026
Merged

feat: global extended context toggle and compact settings UI#63
maxritter merged 4 commits intomainfrom
dev

Conversation

@maxritter
Copy link
Owner

@maxritter maxritter commented Feb 18, 2026

Summary

  • Replace per-model 1M context selection with a single global "Extended Context (1M)" toggle that applies [1m] to all models simultaneously (main session, commands, and sub-agents)
  • Redesign settings UI: unified single-table layout with fixed column widths for consistent vertical alignment, compact toggle card with Max/Enterprise subscription disclaimer
  • Legacy [1m] values in existing configs are silently stripped without auto-enabling extended context — users must opt in explicitly via the toggle
  • Hook improvements: refactored language checkers, new test coverage for context monitor, spec validators, and tool redirect

Test plan

  • Python tests: uv run pytest launcher/tests/unit/test_model_config.py launcher/tests/unit/test_settings_injector.py (49 pass)
  • TypeScript tests: bun test tests/hooks/useSettings.test.ts tests/settings-routes.test.ts (33 pass)
  • Manual: open settings page, verify toggle and dropdowns work, save and restart Pilot
  • Manual: verify legacy config with [1m] models migrates cleanly (stripped, toggle off)

Summary by CodeRabbit

  • New Features

    • Global "Extended Context" toggle to enable 1M token context across all models (requires Max/Enterprise subscription)
  • Improvements

    • Simplified model selection to Sonnet and Opus for main sessions, commands, and sub-agents
    • Settings redesigned into a unified, data-driven table with an Extended Context control
    • Dashboard now renders status cards directly (changed order; removed loading guard)
  • Documentation

    • README updated to reflect global 1M context toggle and subscription details
  • Tests

    • Updated tests to align with simplified model choices and extendedContext behavior
  • Chores

    • Added .DS_Store ignore entry and enhanced pre-commit gate script for multi-stage checks

…mprovements

Replace per-model 1M context selection with a single global "Extended
Context (1M)" toggle. When enabled, all models (main session, commands,
and sub-agents) get the [1m] suffix applied at injection time.

Settings UI redesigned: unified single-table layout with fixed column
widths for consistent vertical alignment, compact toggle card, reduced
spacing. Legacy [1m] values in existing configs are stripped silently
without auto-enabling extended context — users must opt in explicitly.

Also includes hook improvements: refactored checkers, new test coverage
for context monitor, spec validators, and tool redirect.
@vercel
Copy link

vercel bot commented Feb 18, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
claude-pilot Ignored Ignored Feb 18, 2026 2:07pm

Request Review

@coderabbitai
Copy link

coderabbitai bot commented Feb 18, 2026

Walkthrough

Consolidates model selection to base variants (sonnet, opus) and adds a global extendedContext boolean to enable 1M-token context across models; updates settings route, hook, UI, tests, README, .gitignore, and pre-commit gates accordingly.

Changes

Cohort / File(s) Summary
Config & Docs
/.gitignore, README.md
Added .DS_Store ignore entry; README updated to describe a global "Extended Context (1M)" toggle, updated subscription requirement wording (Max/Enterprise), and limited model choices to Sonnet/Opus.
Backend: Settings Route
console/src/services/worker/http/routes/SettingsRoutes.ts
Unified model lists into exported MODEL_CHOICES = ["sonnet","opus"]; added extendedContext: boolean to ModelSettings and DEFAULT_SETTINGS; added stripLegacy1m() to normalize legacy [1m] suffixes; updated validation, merge, GET/PUT handling to preserve unspecified keys and persist extendedContext.
Frontend: Settings Hook
console/src/ui/viewer/hooks/useSettings.ts
Replaced per-model constants with exported MODEL_CHOICES and ModelChoice type; ModelSettings includes extendedContext; DEFAULT_SETTINGS updated; hook exposes updateExtendedContext(enabled) and integrates extendedContext into fetch/save flows.
Frontend: Settings View
console/src/ui/viewer/views/Settings/index.tsx
Switched to MODEL_CHOICES; added Extended Context toggle wired to updateExtendedContext; refactored settings UI into data-driven rows (GENERAL_ROWS, SPEC_ROWS, AGENT_ROWS); added SectionHeader and DefaultBadge; preserved save behavior.
Frontend: Dashboard View
console/src/ui/viewer/views/Dashboard/index.tsx
Removed loading guard (always renders grid), reordered status cards to [VexorStatus, PlanStatus, VaultStatus, WorkerStatus], and made minor import/formatting adjustments.
Tests
console/tests/hooks/useSettings.test.ts, console/tests/settings-routes.test.ts
Rewrote tests to expect MODEL_CHOICES only (no [1m] variants); added assertions for DEFAULT_SETTINGS.extendedContext === false; added tests for legacy [1m] auto-enablement and PUT/GET behavior validating sanitized base-model payloads and extendedContext handling.
Git Hooks
.githooks/pre-commit
Replaced single-gate flow with staged multi-stage gates: Python unit-tests for Python-area changes, console tests/typecheck/build/stage for console changes; stricter failure handling and sequential gate messaging.

Sequence Diagram(s)

mermaid
sequenceDiagram
participant UI as "Client UI"
participant API as "Settings API\n(SettingsRoutes)"
participant FS as "Settings File\n(Persistent Storage)"
UI->>API: GET /settings
API->>FS: read settings file
FS-->>API: settings JSON
API->>API: mergeWithDefaults() (stripLegacy1m, apply MODEL_CHOICES)
API-->>UI: merged settings (includes extendedContext)
UI->>API: PUT /settings {settings, extendedContext}
API->>API: validateSettings() (validate MODEL_CHOICES, extendedContext)
API->>FS: write merged settings atomically
FS-->>API: write result
API-->>UI: updated settings response

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat: global extended context toggle and compact settings UI' accurately summarizes the main changes: introducing a global Extended Context toggle and redesigning the settings UI into a more compact layout.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch dev

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@claude
Copy link

claude bot commented Feb 18, 2026

Claude finished @maxritter's task in 2m 28s —— View job


Code Review

  • Gather context and read repository guidelines
  • Review console/src changes (SettingsRoutes, useSettings, Settings UI, Dashboard)
  • Review pilot/hooks changes
  • Review test coverage
  • Post final review

Summary

Overall this is a clean, well-structured PR. The global extended context toggle is a meaningful UX improvement over per-model selection, the settings UI refactor is solid, and the hook refactoring (moving from stderr printing to return values) is a welcome architectural improvement. Test coverage is good. A few issues worth addressing below.


Issues

1. Architecture mismatch: Python hooks still detect extended context via [1m] suffix

File: pilot/hooks/_util.py:39-52

if isinstance(model, str) and model in ("sonnet", "sonnet[1m]", "opus", "opus[1m]"):
    return model
# ...
return 1_000_000 if "[1m]" in model else 200_000

_get_max_context_tokens() and _read_model_from_config() still detect extended context by checking for [1m] in the model string from ~/.pilot/config.json. But the new settings UI writes extendedContext: boolean as a separate field and actively strips [1m] from stored model names.

This creates a dependency on the launcher (encrypted, not reviewable here) to translate extendedContext: true → appending [1m] to the model field when writing config, or to write it back in a form the hooks can read. If the launcher writes extendedContext: true but leaves the model as opus (no [1m]), the context monitor will always report 200K context, giving incorrect compaction thresholds.

If the launcher does write [1m] back, that's fine — but it's worth documenting this contract explicitly, since _util.py and the new settings system are now tightly coupled through an undocumented convention.

Suggestion: Either update _util.py to also check extendedContext from config, or add a comment documenting the invariant.

Fix this →


2. stripLegacy1m uses simple string replace — could affect model names with [1m] anywhere

File: console/src/services/worker/http/routes/SettingsRoutes.ts:69-71

private static stripLegacy1m(model: string): string {
  return model.replace("[1m]", "");
}

String.replace only replaces the first occurrence, and it strips [1m] from anywhere in the string. If a future model name contained [1m] as a non-suffix (unlikely but possible), this would silently corrupt it. Using a suffix-anchored replace like model.replace(/\[1m\]$/, "") would be more defensive.

Fix this →


3. save() in useSettings.ts swallows errors silently

File: console/src/ui/viewer/hooks/useSettings.ts:105-119

const save = useCallback(async () => {
  await fetch("/api/settings", { ... })
    .then(...)
    .then((data: ModelSettings) => {
      setSettings(data);
      setIsDirty(false);
      setSaved(true);
    });
}, [settings]);

The save() function has no .catch() — if the fetch fails (network error, 500), the promise rejects and the error propagates to the caller. The SettingsView wraps this in a try/catch and sets saveError, so it works in practice, but it's unconventional to have the hook's save() throw rather than handle errors internally. Consider either adding error handling in the hook or documenting that callers must catch.


4. Missing test: save() error propagation in useSettings

File: console/tests/hooks/useSettings.test.ts

The test file only checks static exports and source-level strings — there's no test for the hook's reactive behavior (fetch failure → error propagation, dirty state management, etc.). The SettingsRoutes tests are thorough, but useSettings tests are essentially smoke tests. Consider adding integration-level tests using fetch mocks for the hook's state transitions.


5. context_monitor.py: save_cache called with None for shown_learn when list is empty

File: pilot/hooks/context_monitor.py:184

save_cache(total_tokens, session_id, new_learn_shown if new_learn_shown else None)

save_cache checks if shown_learn: to decide whether to merge — passing None and passing [] have the same effect, so this is fine functionally. But it's slightly inconsistent: sometimes [] is passed, sometimes None. Using None consistently when empty, or always passing the list, would be cleaner.


Observations / Minor Notes

Settings UI (Settings/index.tsx)

  • The unified table layout is clean and the fixed colgroup widths (45%/35%/20%) will give consistent alignment. Good improvement over separate cards.
  • The DefaultBadge component doesn't handle the case where model is not in MODEL_DISPLAY_NAMES — it falls back to the raw key (model ?? model), which is fine.
  • GENERAL_ROWS includes "main" as a key handled specially in the render logic. This implicit convention (checking isMain = row.key === "main") works, but a discriminated type or separate component would be more explicit.

check_file_length refactor (_util.py)

  • Changing from printing to stderr → returning a string is the right call. It makes the function testable (confirmed by test__util.py) and composable.
  • The callers (checkers) correctly append length_warning to the reason string.

Hook response building helpers (_util.py)

  • Centralizing post_tool_use_block, pre_tool_use_deny, etc. into _util.py is a good DRY improvement. Tests confirm they produce correct JSON structures.

spec_stop_guard.py simplification

  • Significant reduction (+8/-60). The new version is much leaner. The cooldown-based escape hatch and AskUserQuestion detection are preserved. Good.

tool_redirect.py

  • EXPLORE_HINT is checked before the BLOCKS dict, so Task(subagent_type="Explore") correctly gets a hint rather than potentially being caught by the Task block entry. The ordering is intentional and correct.

Test coverage additions

  • test_context_monitor.py, test_spec_plan_validator.py, test_spec_verify_validator.py, test_tool_redirect.py are all new and cover the main code paths well.
  • test__util.py gains 19 tests for JSON helpers and check_file_length. Coverage is solid.

Summary

Correctness Mostly good. The [1m]/extendedContext contract between settings and Python hooks needs clarification (issue #1).
Tests Well-covered for hooks and routes. useSettings hook tests are thin.
UI Clean redesign, compact toggle layout works well.
Code quality Hook refactoring improves testability and composability.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (4)
console/src/ui/viewer/hooks/useSettings.ts (1)

105-120: Mixed await + .then() style in save.

The function uses await fetch(...).then(...).then(...). This works but is inconsistent — either pure async/await or pure promise chaining would be clearer. The same applies to the fetch in useEffect (line 60–72) which uses pure .then().

Optional: pure async/await version
  const save = useCallback(async () => {
-   await fetch("/api/settings", {
+   const r = 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);
-     });
+   });
+   if (!r.ok) throw new Error(`Save failed: ${r.status}`);
+   const data: ModelSettings = await r.json();
+   setSettings(data);
+   setIsDirty(false);
+   setSaved(true);
  }, [settings]);
🤖 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 105 - 120, In
useSettings (hook) refactor the mixed await + .then() in the save function to a
consistent async/await style: inside save() await fetch("/api/settings", {
method: "PUT", headers: {...}, body: JSON.stringify(settings) }), check
response.ok and throw with status if not ok, then await response.json() to get
ModelSettings and call setSettings(data), setIsDirty(false), setSaved(true);
also apply the same consistency to the earlier fetch in the useEffect (line
~60–72) by converting its promise chain to async/await and adding proper
try/catch error handling to surface failures (use the existing state setters or
error logging as appropriate).
console/src/services/worker/http/routes/SettingsRoutes.ts (2)

17-44: MODEL_CHOICES and DEFAULT_SETTINGS are duplicated across server and client.

Identical definitions exist in both SettingsRoutes.ts (lines 17, 26–44) and useSettings.ts (lines 3, 19–37). A single shared source of truth would prevent drift.

Not blocking for this PR since both are kept in sync and covered by tests, but worth tracking.

🤖 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 -
44, MODEL_CHOICES, ModelSettings and DEFAULT_SETTINGS are duplicated between
SettingsRoutes and useSettings; extract them into a single shared module (e.g.,
settings/constants or a shared types module) and import that module from both
SettingsRoutes and useSettings to ensure one source of truth; update exports to
export MODEL_CHOICES, ModelSettings and DEFAULT_SETTINGS and replace local
definitions in the SettingsRoutes and useSettings files with imports, run tests
to confirm no behavior change.

69-71: stripLegacy1m only handles lowercase [1m].

String.replace with a literal won't match case variations like [1M]. This is probably fine if the system always stored lowercase, but worth a note.

Slightly more robust alternative
  private static stripLegacy1m(model: string): string {
-   return model.replace("[1m]", "");
+   return model.replace(/\[1m\]/i, "");
  }
🤖 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 69 -
71, stripLegacy1m currently only removes the exact lowercase substring "[1m]"
which misses variants like "[1M]" or multiple occurrences; update the private
static stripLegacy1m(model: string): string to remove all case variations and
occurrences by using a case-insensitive, global match (e.g. use a regex with the
i and g flags) on the model string and return the cleaned result so both "[1m]"
and "[1M]" (and any duplicates) are stripped.
console/src/ui/viewer/views/Settings/index.tsx (1)

273-273: Sticky save bar may overlap page content on smaller viewports.

The pb-16 on the container (line 106) provides some padding, but the fixed bar height + padding could exceed that on edge cases. Consider matching the padding to the actual bar height or using a spacer element.

🤖 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` at line 273, The fixed sticky
save bar div (className "fixed bottom-0 left-0 right-0 bg-base-100 border-t
border-base-300 px-6 py-2 flex items-center gap-4 z-50") can overlap content
because the parent container only uses a static "pb-16" padding; update the
layout by making the bottom spacing dynamic to match the actual bar height
instead of a hardcoded pb-16 — either (A) replace or augment the parent
container's "pb-16" with a spacer whose height matches the bar (e.g., add a
spacer element directly above the fixed bar with height equal to the bar's
computed height or CSS variable), or (B) compute/set a CSS variable for the bar
height and apply it to the parent padding-bottom so the container padding uses
the exact same value as the fixed bar's height; locate the save bar by its
className and the page wrapper with "pb-16" to implement the 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 @.gitignore:
- Line 239: Replace the incorrect uppercase pattern ".DS_STORE" in .gitignore
with the correct macOS metadata filename ".DS_Store" so the ignore rule matches
on case-sensitive filesystems; locate the line containing ".DS_STORE" and update
it to ".DS_Store".

---

Nitpick comments:
In `@console/src/services/worker/http/routes/SettingsRoutes.ts`:
- Around line 17-44: MODEL_CHOICES, ModelSettings and DEFAULT_SETTINGS are
duplicated between SettingsRoutes and useSettings; extract them into a single
shared module (e.g., settings/constants or a shared types module) and import
that module from both SettingsRoutes and useSettings to ensure one source of
truth; update exports to export MODEL_CHOICES, ModelSettings and
DEFAULT_SETTINGS and replace local definitions in the SettingsRoutes and
useSettings files with imports, run tests to confirm no behavior change.
- Around line 69-71: stripLegacy1m currently only removes the exact lowercase
substring "[1m]" which misses variants like "[1M]" or multiple occurrences;
update the private static stripLegacy1m(model: string): string to remove all
case variations and occurrences by using a case-insensitive, global match (e.g.
use a regex with the i and g flags) on the model string and return the cleaned
result so both "[1m]" and "[1M]" (and any duplicates) are stripped.

In `@console/src/ui/viewer/hooks/useSettings.ts`:
- Around line 105-120: In useSettings (hook) refactor the mixed await + .then()
in the save function to a consistent async/await style: inside save() await
fetch("/api/settings", { method: "PUT", headers: {...}, body:
JSON.stringify(settings) }), check response.ok and throw with status if not ok,
then await response.json() to get ModelSettings and call setSettings(data),
setIsDirty(false), setSaved(true); also apply the same consistency to the
earlier fetch in the useEffect (line ~60–72) by converting its promise chain to
async/await and adding proper try/catch error handling to surface failures (use
the existing state setters or error logging as appropriate).

In `@console/src/ui/viewer/views/Settings/index.tsx`:
- Line 273: The fixed sticky save bar div (className "fixed bottom-0 left-0
right-0 bg-base-100 border-t border-base-300 px-6 py-2 flex items-center gap-4
z-50") can overlap content because the parent container only uses a static
"pb-16" padding; update the layout by making the bottom spacing dynamic to match
the actual bar height instead of a hardcoded pb-16 — either (A) replace or
augment the parent container's "pb-16" with a spacer whose height matches the
bar (e.g., add a spacer element directly above the fixed bar with height equal
to the bar's computed height or CSS variable), or (B) compute/set a CSS variable
for the bar height and apply it to the parent padding-bottom so the container
padding uses the exact same value as the fixed bar's height; locate the save bar
by its className and the page wrapper with "pb-16" to implement the change.

…st gates

Update launcher tests to match current hook behavior: hooks return exit
code 0 and communicate via JSON on stdout (stop_block, pre_tool_use_deny,
additionalContext) instead of exit codes and stderr.

Fix get_max_context_tokens to use extended_context flag instead of model
string. Fix Dashboard test by adding Workspace comment label.

Add pre-commit hook gates: Python unit tests run when launcher/pilot
hooks change, Console unit tests + typecheck run when console source
changes — catches failures before push.
@claude
Copy link

claude bot commented Feb 18, 2026

Claude finished @maxritter's task in 50s —— View job


Incremental Code Review

  • Gather context and read repository guidelines
  • Review changed files from new push
  • Check for resolved issues from prior review
  • Post incremental review

This push added changes to .githooks/pre-commit and console/src/ui/viewer/views/Dashboard/index.tsx (plus encrypted launcher files, skipped).


New issues in this push

1. Pre-commit: Python tests run with tail -5 piped through if ! — exit code lost

File: .githooks/pre-commit:12

if ! uv run pytest launcher/tests/ pilot/hooks/tests/ -q --tb=short 2>&1 | tail -5; then

When you pipe uv run pytest ... | tail -5, the exit code of the if ! expression becomes the exit code of tail, not pytest. tail exits 0 on success regardless of what pytest produced. This means a failing pytest run will not trigger the exit 1 — the hook will silently pass even when tests fail.

The same issue applies to the console unit test block:

if ! (cd console && bun test 2>&1 | tail -5); then

The console block uses a subshell (...), so bun test's exit code is preserved through the pipe... actually wait — even in a subshell, cmd | tail returns tail's exit code unless pipefail is set. Since set -e is set but set -o pipefail is not, the pipe exit code is from the last command (tail).

Fix: add set -o pipefail after set -e, or capture output to a temp file:

set -e
set -o pipefail

Fix this →


2. Dashboard: workspace status cards now always visible regardless of selectedProject

File: console/src/ui/viewer/views/Dashboard/index.tsx:34-48

The previous code conditionally rendered the workspace section only when !selectedProject || selectedProject === workerStatus.workspaceProject. The new code removes this condition entirely:

{/* Workspace status cards — not filtered by project */}
<div className="grid grid-cols-1 md:grid-cols-2 gap-6 [&>*]:h-full">

The comment acknowledges this ("not filtered by project"), so this appears intentional. But workerStatus.workspaceProject was previously the mechanism to show workspace-specific cards only for the relevant project. If someone selects a project that isn't the workspace project, they'll now see workspace cards that don't relate to their selected project, which could be confusing. Worth confirming this UX change is intentional.


✅ Previously reported issues — status

Issue Status
#1 Python hooks [1m]/extendedContext contract undocumented Still open_util.py unchanged in this push
#2 stripLegacy1m should use suffix-anchored regex Still openSettingsRoutes.ts unchanged
#3 save() in useSettings.ts swallows errors silently Still openuseSettings.ts unchanged
#4 Missing useSettings hook behavior tests Still open — test file unchanged
#5 context_monitor.py None vs [] inconsistency Still opencontext_monitor.py unchanged
.DS_STORE casing (CodeRabbit) Still open.gitignore unchanged
stripLegacy1m case sensitivity (CodeRabbit) Still open

Configs with model: "opus[1m]" (old format, no extendedContext key) were
silently downgraded to 200K context because migration stripped [1m]
without setting extendedContext=true. Now detects legacy [1m] in any
model value and auto-enables extended context to preserve user intent.
@claude
Copy link

claude bot commented Feb 18, 2026

Claude finished @maxritter's task in 1m 13s —— View job


Incremental Code Review

  • Gather context and read repository guidelines
  • Review changed files from new push
  • Check for resolved issues from prior review
  • Post incremental review

This push modified SettingsRoutes.ts, settings-routes.test.ts, and worker-service.cjs (minified bundle, not reviewed). Launcher files are encrypted and skipped.


New issues in this push

1. PR description contradicts code: legacy [1m] does auto-enable extendedContext

File: console/src/services/worker/http/routes/SettingsRoutes.ts:74-126

The PR description states:

Legacy [1m] values in existing configs are silently stripped without auto-enabling extended context — users must opt in explicitly via the toggle

But the code does the opposite:

let hasLegacy1m =
  typeof raw.model === "string" && raw.model.includes("[1m]");
// ...
if (v.includes("[1m]")) hasLegacy1m = true;
// ...
const extendedContext = raw.extendedContext === true || hasLegacy1m;

Legacy [1m] does auto-enable extendedContext: true. The tests confirm this intent (lines 155–177 are named "should auto-enable extendedContext when legacy 1m model found").

The code behavior is arguably better UX (don't silently downgrade users' context window), but the PR description is wrong and should be updated. Worth clarifying which behavior is intentional.


2. hasLegacy1m not set for legacy [1m] in agents

File: console/src/services/worker/http/routes/SettingsRoutes.ts:114-123

The agents loop strips [1m] but never sets hasLegacy1m:

for (const [k, v] of Object.entries(...)) {
  if (typeof v === "string") {
    const stripped = SettingsRoutes.stripLegacy1m(v);  // strips suffix
    // ↑ no hasLegacy1m = true here, unlike the commands loop
    if (MODEL_CHOICES.includes(stripped)) {
      mergedAgents[k] = stripped;
    }
  }
}

Compare to commands (line 98): if (v.includes("[1m]")) hasLegacy1m = true;

Result: a config with plan-verifier: "opus[1m]" in agents will have the suffix stripped but extendedContext will not be auto-enabled, inconsistent with the same value in commands. Add if (v.includes("[1m]")) hasLegacy1m = true; inside the agents loop.

Fix this →


✅ Resolved issues from prior reviews

None of the previously open issues were addressed in this push.


Still open (from prior reviews)

Issue Location
stripLegacy1m should use suffix-anchored regex replace(/\[1m\]$/, "") SettingsRoutes.ts:70
Pre-commit pipefail missing — pytest | tail -5 silently passes on failure .githooks/pre-commit:8,15,33
Python hooks _util.py: [1m]/extendedContext contract undocumented pilot/hooks/_util.py
save() in useSettings.ts swallows errors silently (no .catch()) useSettings.ts:105-119
Missing useSettings hook behavior tests useSettings.test.ts
context_monitor.py None vs [] inconsistency in save_cache call context_monitor.py:184
.DS_STORE should be .DS_Store (case-sensitive filesystems) .gitignore:239
Sticky save bar pb-16 may not match actual bar height Settings/index.tsx:273

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
.githooks/pre-commit (1)

48-48: npm run build is inconsistent with the bun-based toolchain used in the rest of this hook

Gates 2 and 3 use bun test / bun run typecheck but the build step falls back to npm run build. Consider aligning on one package-manager binary throughout.

♻️ Proposed refactor
-  if ! (cd console && npm run build 2>&1 | tail -3); then
+  if ! (cd console && bun run build 2>&1 | tail -3); then
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.githooks/pre-commit at line 48, The build gate in the pre-commit hook uses
"npm run build" which is inconsistent with the bun-based commands used
elsewhere; update the command "cd console && npm run build 2>&1 | tail -3" to
use bun (e.g., "cd console && bun run build 2>&1 | tail -3" or "cd console &&
bun build 2>&1 | tail -3") so the hook consistently uses the bun toolchain,
keeping the same subshell, output piping and exit status check intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.githooks/pre-commit:
- Line 8: The pre-commit hook uses "set -e" but not "set -o pipefail", so
pipelines that end in "tail" hide failures from earlier commands (pytest, bun
test, bun run typecheck, npm run build) and the "if !" guards never trigger; fix
by adding "set -o pipefail" immediately after the existing "set -e" at the top
of the script so any failing stage in a pipeline causes a non-zero exit and the
existing if ! checks will correctly detect failures.
- Line 28: Update the error message in the pre-commit hook so it instructs
developers to run the correct package manager used elsewhere in the hook:
replace the current echo that says "npm install" with one that says "bun
install" (refer to the existing echo line that prints "[pre-commit] ERROR:
console/node_modules missing. Run: cd console && npm install"). Ensure the
message matches the other console commands in the hook and clearly directs the
user to "cd console && bun install".

---

Nitpick comments:
In @.githooks/pre-commit:
- Line 48: The build gate in the pre-commit hook uses "npm run build" which is
inconsistent with the bun-based commands used elsewhere; update the command "cd
console && npm run build 2>&1 | tail -3" to use bun (e.g., "cd console && bun
run build 2>&1 | tail -3" or "cd console && bun build 2>&1 | tail -3") so the
hook consistently uses the bun toolchain, keeping the same subshell, output
piping and exit status check intact.

Add set -o pipefail so pipe failures propagate correctly — without it,
tail always exits 0, masking test/typecheck failures. Simplify gate
structure: with pipefail + set -e, failures auto-exit without if-wrappers.

Fix .DS_STORE → .DS_Store (macOS uses mixed case). Fix npm → bun in
error message for consistency.
@claude
Copy link

claude bot commented Feb 18, 2026

Claude finished @maxritter's task in 1m 16s —— View job


Incremental Code Review

  • Gather context and read repository guidelines
  • Review changed files from new push (.githooks/pre-commit, .gitignore)
  • Check resolved/open issues from prior reviews
  • Post incremental review

This push modified .githooks/pre-commit and .gitignore. Changes are focused on the pre-commit hook rewrite (adding test gates) and adding .DS_Store to gitignore.


✅ Resolved from prior reviews

Issue Resolution
set -o pipefail missing — pytest | tail silently passes failures Fixed: set -eo pipefail now on line 8. Pipelines like uv run pytest ... | tail -5 correctly propagate exit codes with -e causing the script to abort on failure.
Error message said npm install but project uses bun Fixed: Line 25 now reads cd console && bun install.
.DS_STORE casing wrong for case-sensitive filesystems Fixed: .DS_Store (correct macOS casing) added to .gitignore.

Still open: one remaining pre-commit issue

npm run build is inconsistent with the bun toolchain

File: .githooks/pre-commit:39

(cd console && npm run build 2>&1) | tail -3

All other console commands in this hook use bun (bun test, bun run typecheck), but the build step still uses npm. This is the same nitpick flagged by CodeRabbit in the previous review and it was not addressed in this push.

Fix this →


Still open (from prior reviews, no detail repeated)

Issue Location
stripLegacy1m should use suffix-anchored regex replace(/\[1m\]$/, "") SettingsRoutes.ts:70
Python hooks _util.py: [1m]/extendedContext contract undocumented pilot/hooks/_util.py
save() in useSettings.ts no .catch() — errors propagate to callers useSettings.ts:105-119
Missing useSettings hook behavior tests useSettings.test.ts
context_monitor.py None vs [] inconsistency in save_cache call context_monitor.py:184
hasLegacy1m not set for legacy [1m] in agents loop SettingsRoutes.ts:114-123
PR description says legacy [1m] is stripped without auto-enabling — code does the opposite SettingsRoutes.ts

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (1)
console/tests/settings-routes.test.ts (1)

155-177: Good legacy migration coverage, but missing the agents case

Tests for opus[1m] (model) and sonnet[1m] (command) auto-enabling extendedContext are present and correct. However, there is no corresponding test for a config where only an agent value carries a [1m] suffix. Once the agents loop in SettingsRoutes.mergeWithDefaults is fixed to update hasLegacy1m (see comment on lines 109–124 of SettingsRoutes.ts), a parallel test here will be needed:

it('should auto-enable extendedContext when legacy 1m agents found', async () => {
  fs.writeFileSync(configPath, JSON.stringify({ model: 'opus', agents: { 'plan-challenger': 'sonnet[1m]' } }));
  const m = makeMockRes();
  const req: Partial<Request> = {};
  await (routes as any).handleGet(req as Request, m.res as Response);
  expect(m.body.agents['plan-challenger']).toBe('sonnet');
  expect(m.body.extendedContext).toBe(true);
});
🤖 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 155 - 177, Add a test to
cover the agents case where an agent value has a legacy "[1m]" suffix so
extendedContext is auto-enabled: add a new it block in
console/tests/settings-routes.test.ts that writes a config with model 'opus' and
agents { 'plan-challenger': 'sonnet[1m]' }, calls routes.handleGet, and asserts
that m.body.agents['plan-challenger'] has been normalized to 'sonnet' and
m.body.extendedContext is true; this mirrors the existing model/commands tests
and will validate the fix in SettingsRoutes.mergeWithDefaults (ensure the agents
loop updates the hasLegacy1m flag).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.githooks/pre-commit:
- Line 11: The hook aborts due to SIGPIPE when using head -1 with set -eo
pipefail; remove the piping to head (i.e., change the assignment to
PYTHON_CHANGED to use the full output of git diff --cached --name-only --
'launcher/' 'pilot/hooks/' without | head -1) so the command always exits 0 and
PYTHON_CHANGED remains truthy when files are staged; update the analogous
CONSOLE_CHANGED/other occurrences that use | head -1 the same way to eliminate
the pipefail risk.
- Line 39: Replace the inconsistent npm invocation in the pre-commit hook: the
line using "(cd console && npm run build 2>&1) | tail -3" should use bun like
the other lines (e.g., "bun run build") so the hook consistently relies on bun;
update that command to "(cd console && bun run build 2>&1) | tail -3" to match
the existing use of "bun test" and "bun run typecheck".

In `@console/src/services/worker/http/routes/SettingsRoutes.ts`:
- Around line 109-124: In the agents-processing block inside SettingsRoutes,
detect legacy "[1m]" the same way as the commands loop: before calling
SettingsRoutes.stripLegacy1m on each agent value (the loop iterating over
rawAgents entries and assigning into mergedAgents), check if the original string
v includes "[1m]" and set hasLegacy1m = true when so; then continue to strip
with SettingsRoutes.stripLegacy1m and validate against MODEL_CHOICES as
currently implemented. Ensure you reference the existing variables/funcs
mergedAgents, hasLegacy1m, MODEL_CHOICES, SettingsRoutes.stripLegacy1m, and
rawAgents so the change is localized to that agents loop.

---

Nitpick comments:
In `@console/tests/settings-routes.test.ts`:
- Around line 155-177: Add a test to cover the agents case where an agent value
has a legacy "[1m]" suffix so extendedContext is auto-enabled: add a new it
block in console/tests/settings-routes.test.ts that writes a config with model
'opus' and agents { 'plan-challenger': 'sonnet[1m]' }, calls routes.handleGet,
and asserts that m.body.agents['plan-challenger'] has been normalized to
'sonnet' and m.body.extendedContext is true; this mirrors the existing
model/commands tests and will validate the fix in
SettingsRoutes.mergeWithDefaults (ensure the agents loop updates the hasLegacy1m
flag).

exit 1
fi
# --- 1. Python unit tests ---
PYTHON_CHANGED=$(git diff --cached --name-only -- 'launcher/' 'pilot/hooks/' | head -1)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

set -eo pipefail + head -1 causes spurious hook abort when ≥2 files are staged

With pipefail in effect, code like somecmd | head -n1 will sometimes cause an error, depending on whether the output of somecmd exceeds the size of the pipe buffer or not. When two or more Python or console files are staged simultaneously, git diff gets SIGPIPE (exit 141) as soon as head -1 closes the pipe. Exit status 141 indicates the program was terminated by SIGPIPE (141 − 128 = 13). When running scripts with set -o pipefail, this terminates the script.

Net result: the hook aborts before any tests run whenever a multi-file commit touches launcher/, pilot/hooks/, or console/.

The head -1 was likely added as a performance short-circuit, but since git diff --cached --name-only is fast for a local index check, it's not necessary:

🐛 Proposed fix — remove head -1 to eliminate the SIGPIPE entirely
-PYTHON_CHANGED=$(git diff --cached --name-only -- 'launcher/' 'pilot/hooks/' | head -1)
+PYTHON_CHANGED=$(git diff --cached --name-only -- 'launcher/' 'pilot/hooks/')
-CONSOLE_CHANGED=$(git diff --cached --name-only -- 'console/src/' 'console/scripts/' 'console/package.json' 'console/tsconfig.json' 'console/vite.config.ts' | head -1)
+CONSOLE_CHANGED=$(git diff --cached --name-only -- 'console/src/' 'console/scripts/' 'console/package.json' 'console/tsconfig.json' 'console/vite.config.ts')

git diff --cached --name-only exits 0 in all cases; removing head -1 makes PYTHON_CHANGED/CONSOLE_CHANGED capture the full list of changed files (still non-empty ⟹ truthy), with no SIGPIPE risk.

Also applies to: 20-20

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.githooks/pre-commit at line 11, The hook aborts due to SIGPIPE when using
head -1 with set -eo pipefail; remove the piping to head (i.e., change the
assignment to PYTHON_CHANGED to use the full output of git diff --cached
--name-only -- 'launcher/' 'pilot/hooks/' without | head -1) so the command
always exits 0 and PYTHON_CHANGED remains truthy when files are staged; update
the analogous CONSOLE_CHANGED/other occurrences that use | head -1 the same way
to eliminate the pipefail risk.

echo "[pre-commit] Console typecheck passed."

echo "[pre-commit] Rebuilding console artifacts..."
(cd console && npm run build 2>&1) | tail -3
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

npm run build is inconsistent — the rest of the script uses bun

Lines 30 and 35 use bun test and bun run typecheck; line 39 reverts to npm run build. If developers have only bun installed (not npm), the build step will fail unexpectedly after the tests pass.

🛠️ Proposed fix
-  (cd console && npm run build 2>&1) | tail -3
+  (cd console && bun run build 2>&1) | tail -3
📝 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.

Suggested change
(cd console && npm run build 2>&1) | tail -3
(cd console && bun run build 2>&1) | tail -3
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.githooks/pre-commit at line 39, Replace the inconsistent npm invocation in
the pre-commit hook: the line using "(cd console && npm run build 2>&1) | tail
-3" should use bun like the other lines (e.g., "bun run build") so the hook
consistently relies on bun; update that command to "(cd console && bun run build
2>&1) | tail -3" to match the existing use of "bun test" and "bun run
typecheck".

Comment on lines +109 to 124
if (
rawAgents &&
typeof rawAgents === "object" &&
!Array.isArray(rawAgents)
) {
for (const [k, v] of Object.entries(
rawAgents as Record<string, unknown>,
)) {
if (typeof v === "string") {
const stripped = SettingsRoutes.stripLegacy1m(v);
if (MODEL_CHOICES.includes(stripped)) {
mergedAgents[k] = stripped;
}
}
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Agents loop doesn't set hasLegacy1m — asymmetry with the commands loop

The commands loop (lines 94–104) correctly does if (v.includes("[1m]")) hasLegacy1m = true; before stripping. The agents loop strips and validates but never updates hasLegacy1m. A config that has only legacy [1m] agent values (but a clean main model and commands) will silently get extendedContext: false after migration, losing the 1M context the user previously had.

🐛 Proposed fix
       for (const [k, v] of Object.entries(
         rawAgents as Record<string, unknown>,
       )) {
         if (typeof v === "string") {
+          if (v.includes("[1m]")) hasLegacy1m = true;
           const stripped = SettingsRoutes.stripLegacy1m(v);
           if (MODEL_CHOICES.includes(stripped)) {
             mergedAgents[k] = stripped;
           }
         }
       }
📝 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.

Suggested change
if (
rawAgents &&
typeof rawAgents === "object" &&
!Array.isArray(rawAgents)
) {
for (const [k, v] of Object.entries(
rawAgents as Record<string, unknown>,
)) {
if (typeof v === "string") {
const stripped = SettingsRoutes.stripLegacy1m(v);
if (MODEL_CHOICES.includes(stripped)) {
mergedAgents[k] = stripped;
}
}
}
}
if (
rawAgents &&
typeof rawAgents === "object" &&
!Array.isArray(rawAgents)
) {
for (const [k, v] of Object.entries(
rawAgents as Record<string, unknown>,
)) {
if (typeof v === "string") {
if (v.includes("[1m]")) hasLegacy1m = true;
const stripped = SettingsRoutes.stripLegacy1m(v);
if (MODEL_CHOICES.includes(stripped)) {
mergedAgents[k] = stripped;
}
}
}
}
🤖 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 109 -
124, In the agents-processing block inside SettingsRoutes, detect legacy "[1m]"
the same way as the commands loop: before calling SettingsRoutes.stripLegacy1m
on each agent value (the loop iterating over rawAgents entries and assigning
into mergedAgents), check if the original string v includes "[1m]" and set
hasLegacy1m = true when so; then continue to strip with
SettingsRoutes.stripLegacy1m and validate against MODEL_CHOICES as currently
implemented. Ensure you reference the existing variables/funcs mergedAgents,
hasLegacy1m, MODEL_CHOICES, SettingsRoutes.stripLegacy1m, and rawAgents so the
change is localized to that agents loop.

@maxritter maxritter merged commit 01a315c into main Feb 18, 2026
19 of 20 checks passed
github-actions bot pushed a commit that referenced this pull request Feb 18, 2026
## [6.8.2](v6.8.1...v6.8.2) (2026-02-18)

### Bug Fixes

* global extended context toggle, compact settings UI, and hook improvements ([#63](#63)) ([01a315c](01a315c))
@github-actions
Copy link
Contributor

🎉 This PR is included in version 6.8.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant