Conversation
…kill descriptions - Split monolithic spec.md into spec-plan, spec-implement, spec-verify commands - Add standards-design skill for visual design quality and AI aesthetic anti-patterns - Trim all skill descriptions (~57% reduction) to fit 15k char context budget - Update accessibility skill, context monitor, spec stop guard, and workflow rules - Clean up installer, shell config, and remove claudeboard extension - Add Opus 4.6 badge to README
…xplore agent - Add _clean_npm_stale_dirs() to installer and launcher to remove .claude-code-* temp directories that cause npm ENOTEMPTY errors during install/upgrade - Fall back to success in installer when npm fails but claude command already exists - Block Task/Explore agent in tool_redirect hook with vexor-specific guidance - Update workflow-enforcement and vexor-search rules to ban Explore agent
Enable multiple Pilot sessions in the same project without interference.
Each session gets isolated state under ~/.pilot/sessions/{PID}/ for
continuation files, context cache, and stop guard. Includes Console
dashboard integration, plan association API, statusline task display,
and active sessions count indicator.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Claude finished @maxritter's task in 3m 0s —— View job Code Review Complete ✓I've completed a thorough review of PR #45. This is a substantial PR with well-architected features, but there are several important issues that need to be addressed. OverviewThis PR introduces three major features:
Files changed: 79 files (+2719/-1274) Critical Issues1. Encrypted File Committed to Repository
|
|
Caution Review failedThe pull request is closed. WalkthroughAdds session→plan associations and DB-backed plan store, new plan and Vexor HTTP routes with SSE, an EnsureWorkerDaemon to manage worker lifecycle, removes remote-worker mode and readiness checks, updates installer to detect devcontainer at runtime, broad UI refactors for multi-plan support, many tests, and a license replacement. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Browser / Viewer App
participant Server as Worker HTTP Routes
participant DB as SQLite
participant SSE as SSE clients (useStats)
Client->>Server: POST /api/plan/associate (sessionId, planPath, status)
Server->>DB: UPSERT session_plans (session_db_id, plan_path, plan_status)
DB-->>Server: OK (row)
Server->>SSE: emit "plan_association_changed" (sessionDbId, planPath, status)
SSE-->>Client: SSE event triggers
Client->>Server: GET /api/dashboard/sessions
Server->>DB: SELECT sdk_sessions LEFT JOIN session_plans ...
DB-->>Server: sessions with plan info
Server-->>Client: JSON sessions list
Client->>Client: update UI (PlanStatus, Dashboard)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 7
🤖 Fix all issues with AI agents
In `@console/src/services/sqlite/migrations.ts`:
- Around line 525-550: Duplicate DDL creates session_plans in two places:
migration010 (version 10) and SessionStore.ensureSessionPlansTable (version 21);
remove the inline table creation from SessionStore.ensureSessionPlansTable and
any schema version 21 references so the table is created only via migration010,
ensuring you keep the exact columns/constraints from migration010 (id,
session_db_id UNIQUE, plan_path, plan_status default, timestamps, FK to
sdk_sessions) and update SessionStore to rely on the migration-runner path (or
call into the same migration/apply routine) instead of running its own CREATE
TABLE logic.
In `@console/src/services/sqlite/plans/store.ts`:
- Around line 8-21: Remove the dead exported function ensureSessionPlansTable
from store.ts: delete the entire exported function declaration (the CREATE TABLE
SQL block) so the module no longer exports it, and update any module export list
that referenced ensureSessionPlansTable; confirm there are no remaining callers
(the authoritative migration is migration010 and the private
ensureSessionPlansTable in SessionStore.ts is used) so no other changes are
needed.
In `@console/src/services/sqlite/SessionStore.ts`:
- Line 47: The constructor (or initialization) currently calls
ensureSessionPlansTable(); if you removed the duplicate ensureSessionPlansTable
method in favor of using migration010, remove this invocation as well to avoid
calling a now-missing or redundant method; update the constructor/initializer
where ensureSessionPlansTable is invoked so it relies solely on migration010 for
creating the session_plans table (reference ensureSessionPlansTable and
migration010 to locate and update the call).
- Around line 640-663: The ensureSessionPlansTable inline migration in
SessionStore (method ensureSessionPlansTable) duplicates migration010 defined in
migrations.ts and uses a conflicting version number (21 vs 10); remove the
entire ensureSessionPlansTable method and its invocation from the SessionStore
constructor so the MigrationRunner and migrations.ts (migration010) are the
single source of truth for creating session_plans; ensure no other code
references ensureSessionPlansTable after removal and keep migration010 in the
migrations array as the authoritative migration.
In `@installer/steps/dependencies.py`:
- Around line 234-237: When npm install fails in the _run_bash_with_retry(...)
error path but command_exists("claude") is true, don't return the requested
version variable; instead probe the actual installed Claude by invoking the
existing claude binary (e.g., run "claude --version" or use any helper that
parses its output) and return that discovered version (or return a failure flag
plus None) so the UI isn't misled. Update the branch where
_configure_claude_defaults() is called to capture the real_installed_version and
return (True, real_installed_version) or log/warn that npm failed and an older
version remains, ensuring you use the same return shape the caller expects
instead of returning the original version variable.
- Around line 192-221: The _clean_npm_stale_dirs function currently calls "npm
prefix -g" and appends "lib/node_modules/@anthropic-ai", which fails on Windows;
change the subprocess.run call to use ["npm", "root", "-g"] and derive npm_root
= Path(result.stdout.strip()) without adding "lib/node_modules", then target
anthropic_dir = npm_root / "@anthropic-ai"; keep the existing loop that rmtree's
".claude-code-*" dirs. Also add a guard for npm not being available by catching
FileNotFoundError (and treating non-zero returncode as an early return) so the
function returns cleanly when npm is missing. Ensure install_claude_code still
calls _clean_npm_stale_dirs.
In `@installer/tests/unit/steps/test_dependencies.py`:
- Around line 149-162: The test test_install_claude_code_cleans_stale_dirs has
multiple patched parameters that go unused (mocks for _clean_npm_stale_dirs,
_remove_native_claude_binaries, _run_bash_with_retry,
_configure_claude_defaults, _get_forced_claude_version) causing Ruff
ARG002/RUF059; rename unused test parameters by prefixing them with an
underscore (e.g., mock_clean -> _mock_clean, mock_remove -> _mock_remove, etc.)
and if any patched return_value (like _get_forced_claude_version) is not used,
ensure its return is discarded or the patch parameter is prefixed with an
underscore so the linter treats it as intentionally unused; apply the same
renaming pattern to the other affected test(s) referenced around lines 210-226.
🧹 Nitpick comments (5)
installer/tests/unit/steps/test_migration.py (1)
137-144: Assert the migrated value, not just the key presence.
Right now the test only checks that"enable_python"exists, so a wrong value would still pass. Consider asserting the boolean value to strengthen coverage of the migration behavior.✏️ Suggested assertion
- assert "enable_python" in json.loads(new_config.read_text()) + assert json.loads(new_config.read_text())["enable_python"] is TrueAs per coding guidelines, "Review test code briefly. Focus on: Test coverage for the feature being tested."
installer/steps/shell_config.py (1)
58-59: Detection pattern doesn't match fish shell syntax.The check
'PATH="$HOME/.bun/bin'uses bash-style syntax with=, but fish usesset -gx PATH "$HOME/.bun/bin" $PATH(space, no equals). This means fish-only Bun PATH exports won't triggerhas_old.In practice this is mitigated since other markers (
CLAUDE_ALIAS_MARKER, aliases) are usually present, but for consistency consider also detecting the fish variant:Suggested fix
or "function pilot" in content - or 'PATH="$HOME/.bun/bin' in content + or 'PATH="$HOME/.bun/bin' in content + or 'PATH "$HOME/.bun/bin' in content # fish syntax )console/src/services/sqlite/plans/types.ts (1)
25-33: Remove duplicateDashboardSessioninterface definition inuseStats.ts.This interface is redundantly defined in both
console/src/services/sqlite/plans/types.ts(lines 25-33) andconsole/src/ui/viewer/hooks/useStats.ts(lines 54-62) with identical fields. The hook should import this type from the dedicated types file to maintain DRY and prevent divergence.console/src/ui/viewer/hooks/useStats.ts (1)
54-62: Type duplication:DashboardSessionis also defined inplans/types.ts.This interface is identical to the one in
console/src/services/sqlite/plans/types.ts(lines 24-32). Consider importing from a shared location to avoid duplication and potential drift.♻️ Suggested approach
Either:
- Import from
plans/types.tsif the UI can access the service layer types- Create a shared types file that both can import from
- If separation is intentional (frontend/backend boundary), add a comment explaining why
console/src/services/worker/http/routes/PlanRoutes.ts (1)
16-23: Unused import:getAllActivePlansis imported but never used.The function is imported from the plan store but no handler calls it.
♻️ Remove unused import
import { associatePlan, getPlanForSession, getPlanByContentSessionId, - getAllActivePlans, updatePlanStatus, clearPlanAssociation, } from "../../../sqlite/plans/store.js";
| /** | ||
| * Migration 010 - Add session_plans table for session→plan associations | ||
| * Tracks which session is working on which plan file | ||
| */ | ||
| export const migration010: Migration = { | ||
| version: 10, | ||
| up: (db: Database) => { | ||
| db.run(` | ||
| CREATE TABLE IF NOT EXISTS session_plans ( | ||
| id INTEGER PRIMARY KEY AUTOINCREMENT, | ||
| session_db_id INTEGER NOT NULL UNIQUE, | ||
| plan_path TEXT NOT NULL, | ||
| plan_status TEXT DEFAULT 'PENDING', | ||
| created_at TEXT NOT NULL, | ||
| updated_at TEXT NOT NULL, | ||
| FOREIGN KEY (session_db_id) REFERENCES sdk_sessions(id) ON DELETE CASCADE | ||
| ) | ||
| `); | ||
|
|
||
| console.log("✅ Created session_plans table for plan associations"); | ||
| }, | ||
|
|
||
| down: (db: Database) => { | ||
| db.run(`DROP TABLE IF EXISTS session_plans`); | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Duplicate migration: session_plans table is also created in SessionStore.ts with version 21.
This migration creates the session_plans table with version 10, but SessionStore.ts (lines 640-663) also creates the exact same table using schema version 21. This dual-migration approach creates several problems:
- Version mismatch: MigrationRunner uses version 10, SessionStore uses version 21
- Redundant code: The same DDL exists in two places
- Unpredictable behavior: Depending on which path runs first, the table may be created by either
The session_plans table should be created by only one mechanism. Consider removing the inline migration from SessionStore.ensureSessionPlansTable() and relying solely on migration010.
🤖 Prompt for AI Agents
In `@console/src/services/sqlite/migrations.ts` around lines 525 - 550, Duplicate
DDL creates session_plans in two places: migration010 (version 10) and
SessionStore.ensureSessionPlansTable (version 21); remove the inline table
creation from SessionStore.ensureSessionPlansTable and any schema version 21
references so the table is created only via migration010, ensuring you keep the
exact columns/constraints from migration010 (id, session_db_id UNIQUE,
plan_path, plan_status default, timestamps, FK to sdk_sessions) and update
SessionStore to rely on the migration-runner path (or call into the same
migration/apply routine) instead of running its own CREATE TABLE logic.
| this.renameSessionIdColumns(); | ||
| this.repairSessionIdColumnRename(); | ||
| this.addFailedAtEpochColumn(); | ||
| this.ensureSessionPlansTable(); |
There was a problem hiding this comment.
Remove call if inline migration is removed.
If the duplicate ensureSessionPlansTable method is removed in favor of migration010, this call should also be removed.
🤖 Prompt for AI Agents
In `@console/src/services/sqlite/SessionStore.ts` at line 47, The constructor (or
initialization) currently calls ensureSessionPlansTable(); if you removed the
duplicate ensureSessionPlansTable method in favor of using migration010, remove
this invocation as well to avoid calling a now-missing or redundant method;
update the constructor/initializer where ensureSessionPlansTable is invoked so
it relies solely on migration010 for creating the session_plans table (reference
ensureSessionPlansTable and migration010 to locate and update the call).
| private ensureSessionPlansTable(): void { | ||
| const applied = this.db.prepare("SELECT version FROM schema_versions WHERE version = ?").get(21) as | ||
| | SchemaVersion | ||
| | undefined; | ||
| if (applied) return; | ||
|
|
||
| this.db.run(` | ||
| CREATE TABLE IF NOT EXISTS session_plans ( | ||
| id INTEGER PRIMARY KEY AUTOINCREMENT, | ||
| session_db_id INTEGER NOT NULL UNIQUE, | ||
| plan_path TEXT NOT NULL, | ||
| plan_status TEXT DEFAULT 'PENDING', | ||
| created_at TEXT NOT NULL, | ||
| updated_at TEXT NOT NULL, | ||
| FOREIGN KEY (session_db_id) REFERENCES sdk_sessions(id) ON DELETE CASCADE | ||
| ) | ||
| `); | ||
|
|
||
| this.db | ||
| .prepare("INSERT OR IGNORE INTO schema_versions (version, applied_at) VALUES (?, ?)") | ||
| .run(21, new Date().toISOString()); | ||
|
|
||
| logger.debug("DB", "Created session_plans table for plan associations"); | ||
| } |
There was a problem hiding this comment.
Redundant inline migration duplicates migration010.
This method creates the same session_plans table that migration010 in migrations.ts already defines. The version numbers also conflict (21 here vs 10 in migrations.ts).
If the MigrationRunner is the authoritative migration system, this inline migration should be removed. If inline migrations in SessionStore are the preferred pattern, then migration010 should be removed from migrations.ts.
Given that migration010 is added to the formal migrations array, recommend removing this inline migration and relying on the MigrationRunner to apply migration010.
🛠️ Suggested fix: Remove duplicate inline migration
private addFailedAtEpochColumn(): void {
// ... existing code ...
}
- private ensureSessionPlansTable(): void {
- const applied = this.db.prepare("SELECT version FROM schema_versions WHERE version = ?").get(21) as
- | SchemaVersion
- | undefined;
- if (applied) return;
-
- this.db.run(`
- CREATE TABLE IF NOT EXISTS session_plans (
- id INTEGER PRIMARY KEY AUTOINCREMENT,
- session_db_id INTEGER NOT NULL UNIQUE,
- plan_path TEXT NOT NULL,
- plan_status TEXT DEFAULT 'PENDING',
- created_at TEXT NOT NULL,
- updated_at TEXT NOT NULL,
- FOREIGN KEY (session_db_id) REFERENCES sdk_sessions(id) ON DELETE CASCADE
- )
- `);
-
- this.db
- .prepare("INSERT OR IGNORE INTO schema_versions (version, applied_at) VALUES (?, ?)")
- .run(21, new Date().toISOString());
-
- logger.debug("DB", "Created session_plans table for plan associations");
- }Also remove the call from the constructor (line 47).
🤖 Prompt for AI Agents
In `@console/src/services/sqlite/SessionStore.ts` around lines 640 - 663, The
ensureSessionPlansTable inline migration in SessionStore (method
ensureSessionPlansTable) duplicates migration010 defined in migrations.ts and
uses a conflicting version number (21 vs 10); remove the entire
ensureSessionPlansTable method and its invocation from the SessionStore
constructor so the MigrationRunner and migrations.ts (migration010) are the
single source of truth for creating session_plans; ensure no other code
references ensureSessionPlansTable after removal and keep migration010 in the
migrations array as the authoritative migration.
- Remove dead ensureSessionPlansTable export from plans/store.ts - Add session.py to MODULE_ORDER in build.py for Cython compilation - Fix npm cleanup to use `npm root -g` for cross-platform compatibility - Probe actual installed Claude version on npm fallback path - Prefix unused mock params with _ to fix lint warnings - Simplify topbar by removing search and processing components - Replace prompts stat with last observation time on dashboard - Rename plan status to Specification Status - Add timestamps to spec viewer selector and metadata - Add branded footer component to dashboard layout
|
Claude finished @maxritter's task in 2m 32s —— View job Code Review Complete ✅I've completed a comprehensive review of PR #45. The PR introduces significant new features with generally solid architecture, but there are 3 critical issues that must be addressed before merge. OverviewChanges: 86 files (+2849/-1328) Features:
Commit 7b6560b addressed several issues from previous reviews, but critical problems remain. 🚨 Critical Issues (Must Fix Before Merge)1. Duplicate Migration Creates Schema Version ConflictFiles:
Problem: The
Impact:
Recommendation: Remove the duplicate inline migration from 2. Python urllib API Bug in register_plan.pyFile: Code: resp = urllib.request.urlopen(req, timeout=5)
return resp.status == 200Problem: Impact: This function always fails because it raises Evidence: This is a straightforward Python API error. The urllib.request.HTTPResponse object uses Recommendation: Change line 44 to 3. Session ID Fallback Breaks Multi-Session IsolationFiles:
Code: session_id = os.environ.get("PILOT_SESSION_ID", "").strip() or "default"Problem: When
Impact: This defeats the core multi-session parallel feature. If users run multiple sessions without the environment variable set, they will collide. Recommendation: Either:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@console/src/ui/viewer/views/Spec/index.tsx`:
- Around line 111-120: The periodic interval calls loadContent every
TIMING.SPEC_REFRESH_INTERVAL_MS and that loadContent unconditionally toggles the
isLoadingContent spinner, causing flicker; modify loadContent (or add a
loadContentSilent variant) to accept a silent boolean parameter that skips
setting isLoadingContent (and any UI-loading state) when true, keep the existing
behavior for user-initiated calls, and update the useEffect interval to call
loadContent(true) (or the silent variant) so background refreshes do not flip
the spinner while selectedSpec and content are refreshed in the background.
🧹 Nitpick comments (4)
console/src/ui/viewer/hooks/useStats.ts (1)
54-62: Consider importingDashboardSessionfrom the backend types.This interface duplicates
DashboardSessionfromconsole/src/services/sqlite/plans/types.ts(lines 24-32). Importing it would reduce duplication and ensure consistency if the schema evolves.That said, keeping a separate UI-layer type can be intentional for decoupling—skip if that's the design intent.
♻️ Suggested change
-export interface DashboardSession { - session_db_id: number; - content_session_id: string; - project: string; - status: string; - started_at: string; - plan_path: string | null; - plan_status: string | null; -} +export type { DashboardSession } from '../../services/sqlite/plans/types';console/src/ui/viewer/views/Spec/index.tsx (1)
181-192: Deduplicate and harden modifiedAt formatting.
Formatting is duplicated and can render “Invalid Date” if the backend returns a malformed value. A small helper keeps formatting consistent and guards invalid inputs.♻️ Suggested helper for modifiedAt formatting
const statusConfig = { PENDING: { color: 'warning', icon: 'lucide:clock', label: 'In Progress' }, COMPLETE: { color: 'info', icon: 'lucide:check-circle', label: 'Complete' }, VERIFIED: { color: 'success', icon: 'lucide:shield-check', label: 'Verified' }, } as const; +const formatModifiedAt = (iso?: string) => { + if (!iso) return ''; + const date = new Date(iso); + if (Number.isNaN(date.getTime())) return ''; + return new Intl.DateTimeFormat(undefined, { + month: 'short', + day: 'numeric', + hour: '2-digit', + minute: '2-digit', + }).format(date); +};- {specs.map((spec) => { - const date = spec.modifiedAt ? new Date(spec.modifiedAt) : null; - const dateStr = date - ? date.toLocaleDateString(undefined, { month: 'short', day: 'numeric' }) + - ' ' + date.toLocaleTimeString(undefined, { hour: '2-digit', minute: '2-digit' }) - : ''; - return ( + {specs.map((spec) => { + const dateStr = formatModifiedAt(spec.modifiedAt); + return ( <option key={spec.filePath} value={spec.filePath}> {spec.name} ({spec.status}){dateStr ? ` - ${dateStr}` : ''} </option> ); })}- <span>{new Date(currentSpec.modifiedAt).toLocaleString(undefined, { - month: 'short', day: 'numeric', hour: '2-digit', minute: '2-digit' - })}</span> + <span>{formatModifiedAt(currentSpec.modifiedAt)}</span>Also applies to: 276-283
installer/steps/dependencies.py (2)
192-218: Avoid silent failures in stale-dir cleanup.This is best-effort, but swallowing exceptions makes diagnosing ENOTEMPTY regressions difficult. Consider logging at debug level so failures are observable without blocking install.
🔧 Suggested change (add debug logging)
def _clean_npm_stale_dirs() -> None: @@ - except Exception: - pass + except Exception as exc: + logging.getLogger(__name__).debug( + "Failed to clean npm stale dirs under `@anthropic-ai`: %s", exc + )-import json +import json +import loggingAs per coding guidelines, Review installer code for: Proper error handling and user feedback.
220-233: Normalizeclaude --versionoutput before embedding in UI messages.The returned string goes directly into user-facing messages. If the CLI output includes a prefix or uses stderr, it could display awkwardly. Extract the semantic version (X.Y.Z) and fall back to the raw string only if extraction fails.
Suggested implementation
def _get_installed_claude_version() -> str | None: """Probe the actual installed Claude Code version via claude --version.""" try: result = subprocess.run( ["claude", "--version"], capture_output=True, text=True, ) - if result.returncode == 0 and result.stdout.strip(): - return result.stdout.strip() + if result.returncode == 0: + output = (result.stdout or result.stderr or "").strip() + match = re.search(r"\b\d+\.\d+\.\d+\b", output) + return match.group(0) if match else (output or None) except Exception: pass return NoneAdd
import reat the module level.
…iod, and refine hooks/UI - Remove remote worker mode from console: delete remote-config, response types, file-edit handler; simplify endpoint config and settings to local-only - Extract worker daemon initialization into testable EnsureWorkerDaemon module with dependency injection; hook commands now ensure daemon before executing - Remove per-handler tryEnsureWorkerRunning guards (daemon ensured at entry) - Add offline grace period for license validation with is_network_error flag - Enhance context_monitor and spec_stop_guard hooks with session-scoped plan detection - Add trivial edit detection to TDD enforcer (imports, removals, constants) - Clean up console UI: remove unused SVG icons, Footer, ActiveSessions components; refine Spec viewer with active plan tabs and archived plan dropdown - Add unit tests for EnsureWorkerDaemon and SessionQueueProcessor - Delete dead register_plan.py script and test_register_plan.py - Add plan registration docs to workflow-enforcement rules
|
Claude finished @maxritter's task in 3m 25s —— View job Code Review Complete ✅I've completed a comprehensive review of PR #45. This PR introduces significant architectural improvements with multi-session parallel support, spec command phases, and npm cleanup fixes. While the implementation is generally solid, there is 1 critical issue that must be addressed before merge. OverviewFiles changed: 100 files (+3614/-2183) Major features:
🚨 Critical Issue (Must Fix Before Merge)1. Duplicate Migration Creates Schema Version ConflictFiles:
Problem: The
Code Evidence: In export const migration010: Migration = {
version: 10, // ← Version 10
up: (db: Database) => {
db.run(`CREATE TABLE IF NOT EXISTS session_plans (...)`);
},
};In private ensureSessionPlansTable(): void {
const applied = this.db.prepare("SELECT version FROM schema_versions WHERE version = ?").get(21) as // ← Version 21
| SchemaVersion | undefined;
if (applied) return;
this.db.run(`CREATE TABLE IF NOT EXISTS session_plans (...)`); // Identical DDL
this.db.prepare("INSERT OR IGNORE INTO schema_versions (version, applied_at) VALUES (?, ?)").run(21, ...);
}In constructor(dbPath: string) {
// ...
this.ensureSessionPlansTable(); // ← Always called
}Impact:
Why This Matters: The migration system relies on version numbers to track what has been applied. Having the same table created under two different version numbers breaks this contract and creates technical debt that will cause issues during future schema evolution. Recommendation: Remove the duplicate
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@console/src/services/worker/http/routes/PlanRoutes.ts`:
- Around line 205-238: The handlers handleAssociatePlan and
handleAssociatePlanByContentId currently pass req.body.status directly to
associatePlan; add validation against a defined whitelist of allowed status
strings (e.g., an allowedStatuses constant or enum) before calling
associatePlan, and return a 400 Bad Request with a clear message when
req.body.status is missing or not one of the allowed values; apply the same
check to the other similar handler mentioned (lines ~274-283) so no invalid
status is persisted.
In `@console/src/ui/viewer/components/SummaryCard.tsx`:
- Around line 46-48: The Icon used in SummaryCard is decorative and should
support accessibility attributes so screen readers don't read it redundantly;
update the Icon component to accept and pass through aria-hidden and aria-label
props (and any other native SVG/HTML aria attrs) and then set aria-hidden="true"
on the Icon instance in SummaryCard (where Icon is rendered with
SECTION_ICONS[section.key]) so the decorative icon is ignored by assistive tech
while still allowing consumers to provide an aria-label when needed.
In `@console/src/ui/viewer/views/Dashboard/PlanStatus.tsx`:
- Around line 76-78: The Plan rows use plan.name as the React key which can
collide; update the PlanInfo type in useStats.ts to include an optional
filePath: string property and ensure the hook returns the backend-provided
filePath, then in PlanStatus.tsx change the map key to use plan.filePath
(falling back to plan.name if missing) when rendering <PlanRow ... /> so each
row has a stable unique key; update references to PlanInfo and the return value
of the useStats hook accordingly.
In `@console/tests/queue/session-queue-processor.test.ts`:
- Around line 46-51: Tests use real setTimeout to coordinate emitting messages
and aborting; replace these timers with deterministic sequencing by calling the
async iterator's next() to establish the pending wait, then immediately trigger
the event and abort. Concretely, in this test replace the initial setTimeout
block with: call queue[Symbol.asyncIterator]() to get the iterator, call
iterator.next() and await a microtask (or capture the pending promise) before
invoking store.claimAndDelete = () => makeMessage(SESSION_ID) and
events.emit("message"); then await the iterator result and finally call
controller.abort() (or await a subsequent iterator.next() and then abort) so
tests rely on iterator.next() promises instead of setTimeout; apply the same
pattern to the other tests that use setTimeout (the blocks around
controller.abort() and events.emit()).
🧹 Nitpick comments (3)
console/src/ui/viewer/hooks/useStats.ts (2)
32-40: LocalPlanInfois missing fields present in the canonical type.The
PlanInfointerface here omitsfilePathandmodifiedAtproperties that exist inPlanRoutes.ts(lines 31-41). If the API returns those fields, they'll be silently dropped. Consider importing the shared type or aligning the interfaces to avoid drift.💡 Suggested alignment
interface PlanInfo { name: string; status: 'PENDING' | 'COMPLETE' | 'VERIFIED'; completed: number; total: number; phase: 'plan' | 'implement' | 'verify'; iterations: number; approved: boolean; + filePath?: string; + modifiedAt?: string; }Alternatively, import from
PlanRoutes.tsif it's exported and accessible.
102-104: Minor duplication in activity data extraction.Lines 102-103 and 129 perform identical extraction logic. This is a minor redundancy that could be consolidated, but it's fine to leave as-is given the small scope.
Also applies to: 129-129
console/src/ui/viewer/components/SummaryCard.tsx (1)
9-23: Type section keys to prevent undefined icon lookups.
Record<string, string>won’t catch missing/typoed keys andSECTION_ICONS[section.key]can beundefined. Consider aSectionKeyunion to keepSECTION_ICONSandsectionsin sync.♻️ Proposed refactor
-const SECTION_ICONS: Record<string, string> = { +type SectionKey = "investigated" | "learned" | "completed" | "next_steps"; +const SECTION_ICONS: Record<SectionKey, string> = { investigated: "lucide:search", learned: "lucide:lightbulb", completed: "lucide:check-circle", next_steps: "lucide:arrow-right-circle", }; const sections = [ - { key: "investigated", label: "Investigated", content: summary.investigated }, - { key: "learned", label: "Learned", content: summary.learned }, - { key: "completed", label: "Completed", content: summary.completed }, - { key: "next_steps", label: "Next Steps", content: summary.next_steps }, + { key: "investigated", label: "Investigated", content: summary.investigated }, + { key: "learned", label: "Learned", content: summary.learned }, + { key: "completed", label: "Completed", content: summary.completed }, + { key: "next_steps", label: "Next Steps", content: summary.next_steps }, - ].filter((section) => section.content); + ] as { key: SectionKey; label: string; content: string | null }[]; + const visibleSections = sections.filter((section) => section.content);
| <Icon | ||
| icon={SECTION_ICONS[section.key]} | ||
| size={18} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.tsx" -o -name "*.ts" | head -20Repository: maxritter/claude-pilot
Length of output: 807
🏁 Script executed:
fd -t f "Icon" --extension tsx --extension ts | grep -i icon | head -20Repository: maxritter/claude-pilot
Length of output: 110
🏁 Script executed:
rg -l "export (function|const) Icon" --type=tsx --type=tsRepository: maxritter/claude-pilot
Length of output: 93
🏁 Script executed:
cat -n console/src/ui/viewer/components/ui/Icon.tsxRepository: maxritter/claude-pilot
Length of output: 530
🏁 Script executed:
cat -n console/src/ui/viewer/components/SummaryCard.tsxRepository: maxritter/claude-pilot
Length of output: 2916
Add accessibility support to Icon component for decorative icons.
The Icon component does not currently support aria-hidden or other accessibility attributes. Since the Icon is used decoratively alongside a section heading in SummaryCard (line 51), it should be marked as aria-hidden="true" to prevent screen readers from announcing it separately and causing redundant announcements with the h3 label.
Update the Icon component to accept and pass through aria-hidden and aria-label props for flexible accessibility handling.
🤖 Prompt for AI Agents
In `@console/src/ui/viewer/components/SummaryCard.tsx` around lines 46 - 48, The
Icon used in SummaryCard is decorative and should support accessibility
attributes so screen readers don't read it redundantly; update the Icon
component to accept and pass through aria-hidden and aria-label props (and any
other native SVG/HTML aria attrs) and then set aria-hidden="true" on the Icon
instance in SummaryCard (where Icon is rendered with SECTION_ICONS[section.key])
so the decorative icon is ignored by assistive tech while still allowing
consumers to provide an aria-label when needed.
| setTimeout(() => { | ||
| store.claimAndDelete = () => makeMessage(SESSION_ID) as any; | ||
| events.emit("message"); | ||
| }, 10); | ||
| setTimeout(() => controller.abort(), 50); | ||
|
|
There was a problem hiding this comment.
Reduce timing-based flakiness in async iterator tests.
These cases depend on real setTimeout delays to coordinate events/aborts, which can be nondeterministic on slow CI. Prefer deterministic coordination (e.g., iterator.next() then emit once the wait is established) to avoid timer-based flakes.
✅ Example approach (first test)
- setTimeout(() => {
- store.claimAndDelete = () => makeMessage(SESSION_ID) as any;
- events.emit("message");
- }, 10);
- setTimeout(() => controller.abort(), 50);
-
- const results: any[] = [];
- for await (const msg of processor.createIterator(SESSION_ID, controller.signal)) {
- results.push(msg);
- controller.abort();
- }
+ const iterator = processor.createIterator(SESSION_ID, controller.signal);
+ const pending = iterator.next(); // waitForMessage listener installed
+ store.claimAndDelete = () => makeMessage(SESSION_ID) as any;
+ events.emit("message");
+ const { value, done } = await pending;
+ const results: any[] = [];
+ if (!done && value) results.push(value);
+ controller.abort();Also applies to: 101-105, 134-150
🤖 Prompt for AI Agents
In `@console/tests/queue/session-queue-processor.test.ts` around lines 46 - 51,
Tests use real setTimeout to coordinate emitting messages and aborting; replace
these timers with deterministic sequencing by calling the async iterator's
next() to establish the pending wait, then immediately trigger the event and
abort. Concretely, in this test replace the initial setTimeout block with: call
queue[Symbol.asyncIterator]() to get the iterator, call iterator.next() and
await a microtask (or capture the pending promise) before invoking
store.claimAndDelete = () => makeMessage(SESSION_ID) and events.emit("message");
then await the iterator result and finally call controller.abort() (or await a
subsequent iterator.next() and then abort) so tests rely on iterator.next()
promises instead of setTimeout; apply the same pattern to the other tests that
use setTimeout (the blocks around controller.abort() and events.emit()).
…r search backend Session continuation fix: - Context monitor now prints resolved absolute path instead of literal $PILOT_SESSION_ID, preventing Write tool from using wrong session directory - Updated context-continuation rule with explicit path resolution instructions and stale file cleanup section - Added tests for _get_continuation_path and resolved path output Console UI cleanup: - Replace VectorDbStatus with VexorStatus widget (codebase indexing status, reindex button, polling for completion) - Add VexorRoutes backend with search, status, and reindex endpoints including per-command timeouts and concurrency limits - Add CodebaseResultCard and codebase search tab in Search view with AbortController timeout and error handling - Remove projects panel from sidebar, layout, and command palette - Replace favicon images with emoji SVG, remove Monaspace Radon font - Delete legacy viewer-template.html, unused images, fonts, and orphaned components (SearchFilters, SidebarProjects, useSpinningFavicon) - Rebuild viewer bundle Other changes: - Add RetentionScheduler, cross-session isolation tests, and observation compiler isolation tests - Update spec commands, workflow enforcement rules, and plugin config - Refine worker service, wrapper, and installer dependencies
|
Claude finished @maxritter's task in 3m 51s —— View job Code Review Complete ✅I've completed a comprehensive review of PR #45. This is a substantial PR introducing multi-session parallel support, spec command phases, and npm cleanup. The implementation is generally solid, and previous critical issues have been resolved. However, there is 1 remaining critical issue and several recommendations. OverviewPR Summary:
🚨 Critical Issue (Must Fix Before Merge)1. Session ID Collision Risk - Multi-Session Isolation BrokenFiles:
Problem: When session_id = os.environ.get("PILOT_SESSION_ID", "").strip() or "default"Impact: This defeats the core multi-session parallel feature. If users run multiple sessions without properly setting the environment variable:
Why This Matters: The PR's main selling point is "Run multiple Pilot sessions in the same project without interference." This fallback makes that promise unreliable. Recommendation: Generate a unique session ID when not provided: session_id = os.environ.get("PILOT_SESSION_ID", "").strip() or f"default-{os.getpid()}"Or alternatively, raise a clear error with instructions if the variable is missing in multi-session scenarios. ✅ Previously Identified Issues - RESOLVEDGood news! The following critical issues from previous reviews have been successfully fixed:
|
# [6.2.0](v6.1.1...v6.2.0) (2026-02-06) ### Bug Fixes * address PR [#45](#45) review findings and refine console UI ([7b6560b](7b6560b)) * clean stale npm temp dirs before Claude Code install and block Explore agent ([e674c01](e674c01)) * remove dead code, unused imports, and legacy integrations ([e3357ae](e3357ae)) * remove remote mode, extract worker daemon, add offline grace period, and refine hooks/UI ([71b7fc0](71b7fc0)) * resolve continuation path bug, clean up console UI, and add Vexor search backend ([b255702](b255702)) * split spec command into phases, add design skill, and optimize skill descriptions ([786ab2a](786ab2a)) ### Features * add multi-session parallel support with isolated session state ([8fbb34f](8fbb34f))
|
🎉 This PR is included in version 6.2.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
1 participant
Summary
~/.pilot/sessions/{PID}/for continuation files, context cache, and stop guard. Includes Console dashboard integration, plan association API, statusline task display, and active sessions count indicator./specinto separate phase skills (spec-plan,spec-implement,spec-verify) for better modularity and skill descriptions.Test plan
uv run pytest launcher/tests -q)cd console && bun test)basedpyright launcher— 0 errors)cd console && bun run typecheck)Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation