Conversation
- Merge duplicate test directories (pilot/tests/hooks/ → pilot/hooks/tests/) fixing 16 broken tests from API drift (exit codes, mock paths, removed functions) - Add pilot/hooks/tests/ to CI pipeline and installer tests to pre-commit - Add `background: true` to all 4 verification agents for native parallel execution - Update spec-plan, spec-verify, and task-and-workflow rules for background agent consistency - Move settings.json installation to plugin directory, migrate old merged settings - Make file_checker warnings non-blocking (additionalContext instead of block) - Strengthen EnterPlanMode/ExitPlanMode blocking with exit code 2 - Add playwright-cli session isolation to spec-verify, spec-implement, and testing rules - Fix Vexor install UX: model pre-download is best-effort, not a failure
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
Claude finished @maxritter's task in 4m 9s —— View job Code ReviewA well-scoped PR with clean implementations across all areas. All 1154 tests pass and the consolidation rationale is sound. Below are observations by area. Test Consolidation (
|
| Area | Status |
|---|---|
| Test consolidation | ✅ Clean, correct structure |
| Settings migration | ✅ Correct, well-tested; one forward-compat concern |
| Hook hardening | ✅ Correct behavior |
| Playwright session isolation | ✅ Comprehensive and consistent |
| Pre-commit hook | ✅ Works; tail -5 is minor UX concern |
| CI coverage | ✅ Properly updated |
Issues requiring attention before merge: None — all findings are minor improvements or observations. The FORCE_CLAUDE_VERSION forward-compatibility concern is the one worth tracking if users rely on that feature, but it likely only affects edge-case pinned installs.
|
Caution Review failedThe pull request is closed. WalkthroughMigrate Pilot-managed settings from Changes
Sequence Diagram(s)sequenceDiagram
participant Installer
participant HomeFS as Home FS (~/.claude)
participant PluginDir as Plugin Dir (~/.claude/pilot)
participant Logger
Installer->>HomeFS: check for baseline file (pilot_baseline)
alt baseline exists
Installer->>HomeFS: read baseline and current settings
Installer->>PluginDir: write migrated settings (preserve user keys, remove Pilot-managed keys)
Installer->>HomeFS: delete baseline file
Logger->>Installer: info migration complete
else no baseline
Installer->>Logger: no migration needed (clean install)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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.
🧹 Nitpick comments (2)
.github/workflows/release.yml (1)
136-138: Coverage reporting doesn't include the newly addedpilot/hooks/tests/directory.Tests in
pilot/hooks/tests/will run, but--cov=installer --cov=launcherwon't measure coverage forpilot.hookscode. If coverage data for that module matters, add--cov=pilot.hooks(or--cov=pilot).- python3 -m pytest installer/tests/unit/ launcher/tests/unit/ pilot/hooks/tests/ -v \ - --cov=installer --cov=launcher \ + python3 -m pytest installer/tests/unit/ launcher/tests/unit/ pilot/hooks/tests/ -v \ + --cov=installer --cov=launcher --cov=pilot.hooks \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 136 - 138, The pytest command in the release workflow runs tests under pilot/hooks/tests but does not collect coverage for that package; update the pytest invocation (the line starting with "python3 -m pytest installer/tests/unit/ launcher/tests/unit/ pilot/hooks/tests/ -v \") to include coverage for the pilot package by adding either --cov=pilot.hooks or --cov=pilot so the pilot.hooks code is measured alongside --cov=installer and --cov=launcher.installer/steps/dependencies.py (1)
385-388: Consider extracting the repeated best-effort pattern into a small helper.The same 3-line block appears four times. A tiny wrapper would reduce duplication and centralize the message:
♻️ Optional refactor
+def _best_effort_model_setup(ui: Any = None, **kwargs: Any) -> None: + """Attempt local model download; log info and continue on failure.""" + if not _setup_vexor_local_model(ui, **kwargs): + if ui: + ui.info("Embedding model will download on first use")Then each call site becomes
_best_effort_model_setup(ui)or_best_effort_model_setup(ui, device="mlx").Also applies to: 410-413, 421-424, 427-430
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@installer/steps/dependencies.py` around lines 385 - 388, There’s a repeated 3-line “best-effort” pattern after model setup calls (example: the block that checks if not _setup_vexor_local_model(ui): if ui: ui.info("Embedding model will download on first use") return True) — extract that into a small helper (e.g. _best_effort_model_setup(ui, device=None)) that runs the call, logs the "Embedding model will download on first use" message when the setup returns False, and returns the boolean result; then replace the four duplicated blocks (the call sites around _setup_vexor_local_model(ui) and the other three similar setup calls) with a single call to the helper, passing device or context when needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 136-138: The pytest command in the release workflow runs tests
under pilot/hooks/tests but does not collect coverage for that package; update
the pytest invocation (the line starting with "python3 -m pytest
installer/tests/unit/ launcher/tests/unit/ pilot/hooks/tests/ -v \") to include
coverage for the pilot package by adding either --cov=pilot.hooks or --cov=pilot
so the pilot.hooks code is measured alongside --cov=installer and
--cov=launcher.
In `@installer/steps/dependencies.py`:
- Around line 385-388: There’s a repeated 3-line “best-effort” pattern after
model setup calls (example: the block that checks if not
_setup_vexor_local_model(ui): if ui: ui.info("Embedding model will download on
first use") return True) — extract that into a small helper (e.g.
_best_effort_model_setup(ui, device=None)) that runs the call, logs the
"Embedding model will download on first use" message when the setup returns
False, and returns the boolean result; then replace the four duplicated blocks
(the call sites around _setup_vexor_local_model(ui) and the other three similar
setup calls) with a single call to the helper, passing device or context when
needed.
The directory existence check fails in CI due to git-crypt decryption creating placeholder files. The remaining 7 tests already verify Search is fully removed from exports, imports, routes, and navigation.
|
Claude finished @maxritter's task in 42s —— View job Incremental Review
New push:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
console/tests/ui/search-removal.test.ts (1)
49-56:⚠️ Potential issue | 🟡 MinorTest name claims to verify the
'g r'key binding, but no assertion covers it.The description
"keyboard shortcuts do not include 'g r' for Search"implies the shortcut key itself is checked, yet the assertions only look for the human-readable label ("Go to Search") and the action URI ("navigate:/search"). The actual"g r"token inshortcuts.tsis never asserted, leaving a silent coverage gap for the key binding.🔧 Proposed fix — assert the key string or tighten the test name
Option A — add the missing key assertion:
it("keyboard shortcuts do not include 'g r' for Search", () => { const source = readFileSync( "src/ui/viewer/constants/shortcuts.ts", "utf-8" ); expect(source).not.toContain("Go to Search"); expect(source).not.toContain("navigate:/search"); + expect(source).not.toContain('"g r"'); // or whatever literal is used in shortcuts.ts });Option B — rename to match what is actually tested:
- it("keyboard shortcuts do not include 'g r' for Search", () => { + it("keyboard shortcuts do not include Search label or navigate:/search action", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/tests/ui/search-removal.test.ts` around lines 49 - 56, The test "keyboard shortcuts do not include 'g r' for Search" fails to assert the actual key token; update the test in search-removal.test.ts to either (A) add an assertion that the shortcuts source (from "src/ui/viewer/constants/shortcuts.ts") does not contain the key string "g r" (or whatever exact token is used for that binding) in addition to the existing checks for "Go to Search" and "navigate:/search", or (B) change the test name to reflect that it only checks for the label and URI (e.g., "keyboard shortcuts do not include Search label or action"). Locate the test function (it("keyboard shortcuts do not include 'g r' for Search", ...)) and modify it accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@console/tests/ui/search-removal.test.ts`:
- Around line 49-56: The test "keyboard shortcuts do not include 'g r' for
Search" fails to assert the actual key token; update the test in
search-removal.test.ts to either (A) add an assertion that the shortcuts source
(from "src/ui/viewer/constants/shortcuts.ts") does not contain the key string "g
r" (or whatever exact token is used for that binding) in addition to the
existing checks for "Go to Search" and "navigate:/search", or (B) change the
test name to reflect that it only checks for the label and URI (e.g., "keyboard
shortcuts do not include Search label or action"). Locate the test function
(it("keyboard shortcuts do not include 'g r' for Search", ...)) and modify it
accordingly.
- Check ~/.claude/pilot/settings.json in _get_forced_claude_version fallback - Add --cov=pilot.hooks to CI pytest coverage tracking - Extract duplicated _run_with_input helper to module-level function - Increase pre-commit tail output from 5 to 20 lines - Add missing "g r" key assertion in search-removal test
Incremental Review
|
## [6.9.1](v6.9.0...v6.9.1) (2026-02-20) ### Bug Fixes * consolidate test infrastructure, harden parallel spec workflows ([#80](#80)) ([df6fd5a](df6fd5a)) * make vexor model pre-download best-effort during installation ([4b2f2da](4b2f2da)) * use sys.executable instead of uv in spec_validators tests ([9145e19](9145e19))
|
🎉 This PR is included in version 6.9.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
1 participant
Summary
pilot/tests/hooks/intopilot/hooks/tests/, fixing 16 broken tests from API drift (exit codes, mock paths, removed functions). All 1154 Python tests pass.pilot/hooks/tests/to CI pipeline andinstaller/tests/unit/trigger to pre-commit hook.background: trueto all 4 verification agent definitions; updated spec-plan, spec-verify, and task-and-workflow rules for consistency (run_in_background=truekept as fallback).-s=$PILOT_SESSION_IDsession isolation to spec-verify, spec-implement, playwright-cli, testing, and verification rules to prevent cross-session browser interference.~/.claude/settings.json), added migration for old merged settings, fixed Vexor install UX (model pre-download is best-effort).additionalContext); EnterPlanMode/ExitPlanMode blocking strengthened with exit code 2 and clearer messaging.Test plan
installer/tests/unit/+launcher/tests/unit/+pilot/hooks/tests/)Summary by CodeRabbit
Bug Fixes
Chores
UI
Tests