Expose cross-project session listing and session deletion over RPC#315
Conversation
Implementation PlanProblem analysisThe fleet-centric dashboard (implementation issue 307, design issue 311) needs to list sessions across all projects and delete sessions via RPC. Today:
Deliverables
Files to create or modify
Testing approach
Risks and open questions
Plan created by mach6 |
Vitest coverage
|
Progress UpdateImplemented the full plan:
Validation:
The branch also carries one prior commit fixing pre-existing test failures (Copilot-retired model IDs, probe-verified replacements) — kept separate for reviewability. Commit: Progress tracked by mach6 |
Code ReviewCriticalNone. ImportantFinding 1 — Active-session guard is a raw lexical string compare (bypassable) · The Note: code-reviewer traced the intended dashboard workflow (paths sourced from Finding 2 — The safety guard is completely untested. SuggestionsFinding 3 — The outer Finding 4 — Path outside sessions directory is not rejected · Issue #312 acceptance criteria: " Finding 5 — The success test asserts only Finding 6 — The handler maps Finding 7 — Duplicated The new Finding 8 — Unreachable
Strengths
Agents run: code-reviewer, error-auditor, test-reviewer, completeness-checker, simplifier Reviewed by mach6 |
Review AssessmentIndependent verification of the review above (#315 (comment)), each finding checked against source. Classifications
Tally: 5 genuine · 2 nitpick · 0 false positive · 1 deferred Action PlanFix before merge, priority order:
Follow-up (not blocking):
Skip: Finding 8 (harmless dead-code nitpick). Assessment by mach6 |
- Enforce sessions-directory containment in SessionManager.deleteSession; custom --session-dir setups pass their dir via a new allowedDirs option - Canonicalize the path before every guard (suffix, active-session, containment, existence) and before deletion, so a non-canonical spelling cannot bypass protection; add defense-in-depth activeSessionPath check - Canonicalize both operands of the RPC active-session compare - Extract shared toRpcSessionInfo() helper for list_sessions/list_all_sessions - Add tests: canonicalized active-session guard, containment refusal, method/trash branches + error-hint composition, Date->ISO DTO mapping - Document containment/canonicalization in rpc.md and CHANGELOG
Progress UpdateAddressed review findings 1, 2, 4, 5, 6, 7 from the assessment. Genuine issues fixed:
Tests added:
Docs: Not in this batch:
Validation: Commit: Progress tracked by mach6 |
Code Review (second pass — post-fix)Re-review of the current state after commit CriticalNone. ImportantFinding 1 — The fix canonicalizes with
This codebase already knows the right pattern — Finding 2 — One session file with a missing/invalid header timestamp throws and blanks the entire
Finding 3 — RPC server-side handlers ( Answering the first review's open question directly: no test exercises the RPC handler's active-session guard. The three SuggestionsFinding 4 — The The double-failure test mocks Strengths
Also noted (below threshold): Agents run: code-reviewer, error-auditor, test-reviewer, completeness-checker Reviewed by mach6 |
Review Assessment (second pass)Independent verification of the review above (#315 (comment)), each finding checked against source at HEAD Classifications
Tally: 3 genuine (blocking) · 1 nitpick · 0 false positive · 0 deferred · 2 genuine-minor Action PlanFix before merge, priority order (security/integrity → correctness → tests → minor):
Skip: Finding 4 (nitpick — trivial coverage gap on a defensive hint branch; optionally fold a No deferred items. Assessment by mach6 |
…er test gaps
Addresses the second-pass review findings on the RPC session commands:
- Symlink canonicalization (finding 1): deleteSession now canonicalizes
with realpathSync (dereferencing symlinked parent components), not just
lexical resolve(), so a symlinked path cannot bypass the containment or
active-session guards. Allowed roots are canonicalized the same way.
Corrected the JSDoc, CHANGELOG, and docs/rpc.md which overstated the
protection lexical resolve() provided.
- created-timestamp guard (finding 2): buildSessionInfo now falls back to
file mtime when the header timestamp is missing/malformed, so one bad
file no longer throws RangeError through toISOString() and blanks the
entire list_all_sessions response.
- Testable RPC handlers (finding 3): extracted deleteSessionForRpc and
listAllSessionsForRpc helpers (getPerformanceStatsData precedent) and
removed the redundant lexical active-session pre-check; core is
authoritative.
- spawnSync trash call gains a timeout; removed the unreachable
startsWith('-') branch (canonicalized paths are always absolute).
Adds tests for symlinked-parent bypass refusal, active-session-via-symlink
refusal, trash+stderr hint composition, missing/malformed timestamp
fallback, and full server-side handler wiring.
Progress UpdateAddressed all five action-plan items from the second-pass assessment. Genuine issues fixed:
Minor items fixed:
Tests added (13):
Not changed:
Validation: Commit: Progress tracked by mach6 |
Code Review (third pass — post-fix,
|
Review Assessment (third pass)Independent verification of the review above (#315 (comment)), each finding checked against source at HEAD Verification notes
Classifications
Tally: 3 genuine (2 blocking + 1 low-priority) · 1 nitpick · 0 false positive · 0 deferred Action PlanFix before merge, priority order:
Skip: Finding 4 (TOCTOU) — no action for the single-user local store; note as a known theoretical limitation. Test flags: Findings 1 and 2 both introduce testable behavior and, per the repo's "new testable code ships with tests" rule, must not merge without the tests above. Assessment by mach6 |
Review Assessment — Addendum (supersedes the prior action plan)Maintainer direction after the third-pass review: the whole containment guard is the wrong tool for the threat model and should be removed, not further hardened. This addendum revises the action plan accordingly. The mechanical findings above are still individually correct — but most of them are edge cases inside a guard that shouldn't exist in this form, so fixing them one by one has been chasing a moving target across three cycles. Why containment is the wrong frame here
Decision (maintainer): take Option 1 — drop the containment guard, match Revised classification under Option 1
Revised action plan
Net: removing ~a guard's worth of code retires Findings 1, 2, and 4 outright and aligns This addendum reflects a maintainer scope decision; it does not itself change code. Assessment by mach6 |
delete_session now uses the same unrestricted, cross-project path addressing as switch_session: resolve() for the active-session compare and deletion, no sessions-directory containment guard. Removes the realpathSync canonicalization helpers, the allowedDirs option, and its RPC/TUI wiring. This is a trusted local channel and the dashboard (#307) owns its own authz; the containment guard defended a non-threat and its lexical path-matching generated a recurring class of bypass findings (symlink deref, empty-allowedDirs -> cwd widening, TOCTOU) across three review cycles. Acceptance criterion (c) on #312 struck with rationale. listAll() no longer swallows failures: removed the outer catch{return []} and the inner per-directory catch{push([])}. A missing sessions dir is still a legitimate empty state; any real I/O failure now propagates so list_all_sessions responds success:false and the TUI selector surfaces the error instead of a silent empty list. Kept guards: active session, non-.jsonl, nonexistent file. Rewrote the delete and RPC-handler tests for the unrestricted model, added a listAll fail-loud test.
Progress UpdateActed on the third-pass assessment's maintainer decision (Option 1): removed the sessions-directory containment guard rather than hardening it further, and made cross-project listing fail loud. Containment removed (dissolves the third-pass findings 1, 2, and 4):
Tests:
Docs / issue:
Validation: pre-commit hook full suite green (4008 passed, 0 failed) · biome clean · Commit: Progress tracked by mach6 |
Code Review (fourth pass — post guard-removal)Re-review at HEAD CriticalNone. ImportantNone. SuggestionsFinding 1 — CLI cross-project session resolution turns a The Finding 2 — TUI delete wiring (the component's real The PR's actual TUI-side behavioral change — Finding 3 — TUI "Failed to load sessions" path (fail-loud The stated user-visible value of the fail-loud change is that the selector shows an error instead of a silently-empty list — but no test passes a rejecting loader to Finding 4 — No test proves a Finding 5 — The last two lines of Finding 6 — The wrapper's only content is Strengths
Agents run: code-reviewer, error-auditor, test-reviewer, completeness-checker, simplifier Reviewed by mach6 |
Review Assessment (fourth pass)Independent verification of the review above (#315 (comment)), each finding checked against source at HEAD Classifications
Tally: 1 genuine (low) · 4 nitpick · 0 false positive · 1 deferred Completeness: the completeness-checker independently verified every effective acceptance criterion of issue 312 (post-waiver) is met at HEAD, the criterion-(c) waiver is properly recorded on the issue, and docs/CHANGELOG accurately describe the shipped behavior with no stale claims from intermediate commits. Action Plan
Skip: Findings 2, 4, 5, 6 — thin-wiring test gaps over already-tested core logic, trivial Defer (no tracking issue): Finding 3 — pre-existing generic error path the PR merely makes reachable. Bottom line: No finding blocks merge. Fix finding 1 if you want the consistency polish; otherwise this PR is ready for Assessment by mach6 |
Progress UpdateFixed finding 1 from the fourth-pass assessment (the only genuine issue). Fix: Findings 2, 4, 5, 6 skipped and finding 3 deferred per the assessment (nitpicks / pre-existing path — none blocking). Validation: typecheck clean · biome clean · Commit: Progress tracked by mach6 |
1 participant
Closes #312
Adds
list_all_sessionsanddelete_sessionRPC commands so external frontends (the planned web dashboard) can list sessions across all projects and delete sessions without importing SessionManager directly. Moves the trash-first deletion logic from the TUI session selector into core.Implementation plan posted as a comment below.