Extract a transport-ready session service#60
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f446c5b4d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const name = String(body.name || "").trim(); | ||
| targetSession.setSessionName(name); | ||
| const state = currentStateWithThinkingLevels(targetSession); | ||
| if (!name) return sendJson(res, 400, { ok: false, error: "name is required" }); |
There was a problem hiding this comment.
When a user deletes a previously assigned title in the inline rename editor, the client intentionally submits an empty name so setSessionName("") restores the generated session title. This new validation rejects that request and restores the old title in the UI, removing the existing ability to clear a custom session name.
Useful? React with 👍 / 👎.
| } | ||
| noteViewerLeaseFromRequest(req, targetSession, body.clientId); | ||
| const state = currentStateWithThinkingLevels(targetSession); | ||
| const state = await sessionService.open(requestedId, typeof body.cwd === "string" && body.cwd.trim() ? body.cwd : undefined); |
There was a problem hiding this comment.
When a stale or externally deleted session ID is submitted, switchToSessionId throws a plain Error("Session not found") through sessionService.open; unlike the previous route-level catch, the outer handler classifies that as a 500 rather than a 404. This makes an expected missing-session condition appear as a server failure and can break client recovery from stale session lists.
Useful? React with 👍 / 👎.
Review: request changes — close, but not yet a clean zero-behavior-change refactorThe extraction quality is largely excellent: However, a route-by-route parity audit against 1. Restore observable behavior (merge blockers)1.1
|
| Route | Old → New | Cause |
|---|---|---|
POST /api/web-header-action/invoke, POST /api/web-git-tab/invoke (missing session) |
404 → 500 | route-local catch maps by message only and doesn't recognize SessionServiceError (server.ts:1286-1305; service.ts:36-40) |
POST /api/sessions/open (unknown session) |
404 → 500 | switchToSessionId throws a plain Error; outer catch only preserves SessionServiceError status (server.ts:1497-1504,1518-1520; old main:3047-3064) |
GET /api/session/tree (tree unavailable) |
400 → 500 | conversationTreeForSession throws plain Error (projection.ts:359-362); no route-local catch (server.ts:1312-1314; old main:2677-2686) |
POST /api/session/cwd (unknown session) |
404 → 400 | resolution moved inside the broad 400 try (server.ts:1482-1494; old main:3029-3036) |
POST /api/session/tree/navigate (invalid session + invalid targetId) |
404 → 400 | targetId validated before session resolution (server.ts:1316-1320; old main:2688-2700) |
Suggested fix: throw SessionServiceError with the correct status from the service/deps layer (e.g. wrap switchToSessionId, tree projection) and make the shared error handler preserve it, instead of per-route message matching.
1.3 Tree navigation broadcast/response ordering (server.ts:1032-1047,1320-1326 vs main:2703-2719)
Old order: navigation resolves → state_changed → HTTP response → terminal session_runtime_changed. New order emits the terminal broadcast before the response. Restore the original ordering (old code achieved it by calling sendJson in the try-return expression before finally ran).
1.4 POST /api/new-chat / /api/sessions/new with unknown source sessionId (service.ts:161-165 vs main:3018-3027)
Old behavior tolerated an unknown/absent source session and created from the global cwd with no previous session file; new create() always require()s it → 404. Also: an empty-cwd source session previously fell back to global piCwd; the new cwd || deps.cwd(previous) picks the source session's cwd. Restore both.
1.5 Revert semantic edits smuggled into moves
service.ts:63:part.arguments || part.args || {}— old code waspart.arguments || {}(main:2737-2748). This addstoolArgsvalues to/api/messagesresponses that previously returned{}. Revert (propose separately if desired).service.ts:90-96:setModelnow skips empty-string thinking level; old route calledsetThinkingLevelfor every string (main:2837-2838). Revert.
1.6 Restore streaming for working-tree git images (server/shared/git.ts:122-127, server.ts:1253-1257 vs main:328-356)
Unstaged images were streamed via pipeReadStream; they are now fully buffered. Restore streaming (memory/first-byte behavior for large images).
1.7 Whitespace
git diff --check main...HEAD fails: server/session/projection.ts:533: new blank line at EOF. Fix.
2. Make the typed contract real (merge blocker)
dto.ts currently declares a full SessionService interface, a SessionServiceEvent union, and jsonRoundTrip — none of which are used:
LocalSessionServicedoes notimplements SessionService; signatures diverge widely (interfacecreate(cwd?, previousSessionFile?) → CreateSessionResultDtovs implcreate(sessionId?, cwd?) → Record<string, unknown>;statsreturnsSessionStatsDtovs{sessionId, stats};promptpositional vs object;navigateTreevsnavigate;respondExtensionUi/acquireViewer/releaseViewer/fs/git/artifacts/subscribeabsent).SessionServiceEventis emitted nowhere, and its names (pi,state,stats) don't match the actual wire (pi_event,state_changed,session_stats_changed).BaseSessionStateDtoomits fields the real state carries (runtimeStartedAt,runtime,webFooters,webHeaderActions,webGitTabs— fine if decoration is documented as host-side, butSessionInfoDtoalso omitsisCurrent/runtime/unread, andShellResultDtoomitscommand/cwd/excludeFromContextwhich the route returns).jsonRoundTripis unused (the test defines its own).
An unenforced contract is a drift seed — the exact disease this refactor exists to cure. Required:
LocalSessionService implements <interface>for every method that exists today, with signatures and result DTOs matching actual route responses (typecheck must fail on divergence).- Remove the not-yet-implemented members (
subscribe,SessionServiceEvent,fs/git/artifacts, viewer methods) from the interface — they return in the runner-transport PR when they become real. Keep them in the design doc, not in shipped types. - Delete
jsonRoundTripor use it in the serializability tests.
3. Required tests (all of them; none optional)
Add parity tests that would have caught every finding above, so the next slice can't regress them:
- Route status matrix: missing/unknown session returns the documented status for
state,messages,sessions/open,session/cwd,web-header-action/invoke,web-git-tab/invoke,session/tree,session/tree/navigate(404 wheremainreturned 404; 400 where 400). - Rename semantics: empty name clears the session name and returns 200; no duplicate
state_changedbroadcast for a single rename. - New-session fallback:
POST /api/sessions/newwith an unknownsessionIdcreates from global cwd (200), not 404. - Messages parity: a
toolResultwhose tool call exposes onlyargsproducestoolArgs: {}(or land the improvement separately with its own test). - Model edge: empty-string
thinkingLevelbehaves as onmain. - Broadcast ordering: for tree navigate, assert the HTTP response is written before the terminal
session_runtime_changed(mock-mode WS + fetch interleaving). - Interface conformance: a compile-time
satisfies/implementscheck forLocalSessionServiceagainst the trimmed interface (covered by §2.1, verified by typecheck). - Serializability: every service method result in the fixture tests passes strict-deep-equal JSON round-trip (extend the existing projection fixtures to cover
messages,models,commands,stats,state).
Also add the CI guard from the plan: a check (grep or lint rule) that route bodies in server.ts contain no direct targetSession. member access — the audit found the remaining direct uses are confined to helpers and WS hello, so encode that boundary now (server.ts:1356, 1544-1555 are the current exceptions to either fix or explicitly allowlist).
4. Re-verify visuals before merge
A full E2E run in a review worktree showed two reproducible mobile visual diffs: tests/e2e/visual.spec.ts:163 (hero showcase, ~4%) and :199 (conversation tree, ~5%). Other E2E failures were environment artifacts (a spec asserting the checkout path contains pi-web; mock-server churn under 4-shard load). Since the PR description reports a clean SHARDS=1 matrix, re-run those two specs on the real checkout and paste the passing output in this PR before merge.
5. Validation checklist for the follow-up push
npm run typecheck(now proves interface conformance)npm run test:unitincluding the new parity testsPI_WEB_E2E_SHARDS=1 npm testfull matrix, with the two visual specs confirmednpm run buildgit diff --check
Tracked follow-up (not this PR): LocalSessionService is a facade over an 18-closure deps bag (server.ts:1099-1119); lifecycle/operations (createNewLiveSession, startSessionPrompt, startSessionRetry, navigateSession, executeSlashCommand, switchEmptySessionCwd) still live in server.ts. That's acceptable for this slice, but the runner transport cannot exist until those move behind the service — it should be the explicit next PR, and the trimmed interface members (subscribe, fs/git/artifacts, viewers) return there.
Verification of 012c447 — 9/10 fixes clean; 1 new regression; 1 leftoverRe-audited commit 012c447 against the parity review. typecheck and 109/109 unit tests pass in a clean worktree. Verified fixed ✅
❌ New regression: default new-chat lost the current-session fallback (
|
|
Addressed the remaining review items in Behavior/order
Required parity coverage
Requested mobile visuals (real checkout)The final full run also passed both snapshots as part of the complete mobile project: Final validation
The branch is clean and pushed. Please re-audit |
|
Verification of d2b8103 — typecheck + 116/116 unit tests pass in a clean worktree. This commit closes the test/guard items: missing-session status matrix (10 routes), rename clears + exactly one One change still required — the ordering implementation. const { finish, ...payload } = await sessionService.navigate(...);
sendJson(res, 200, { ok: true, ...payload });
finish(); // lease release + terminal broadcast, causally after the responseThen, to merge: paste passing output for |
|
Addressed the final ordering blocker in The timer heuristic is gone. try {
return sendJson(res, 200, { ok: true, ...result });
} finally {
finish();
}Navigation failures still finalize immediately. Successful navigation releases its work lease and emits the terminal runtime event only after I also replaced the inherently cross-socket client-delivery-order assertion (HTTP and WS delivery can be observed in either order even when server calls are causal) with two non-flaky checks:
The navigation integration test passed 10/10 repeated runs after this change. Mobile visuals on exact commit
|
|
Verification of 219a41c — finalizer implemented as requested: causal All code items from the review are now resolved. Remaining before merge: passing output for Note for the follow-up runner-transport PR (not this one): |
|
Final evidence for exact commit Requested mobile visualsCommand: npx playwright test tests/e2e/visual.spec.ts --project=mobile --grep 'hero showcase|conversation tree'Output: Full test matrixCommand: PI_WEB_E2E_SHARDS=1 npm testFinal output: The full run also included and passed both requested mobile visuals: Production buildCommand: npm run buildOutput: Additional final checksGitHub CI for The |
|
Visual snapshots requested in the review, shown inline for direct inspection. These are the committed mobile baselines at Mobile hero showcase
Mobile conversation tree
|
|
Dogfooding found one additional resumed-session issue after the review was otherwise complete. Fixed in What happenedThe Git footer extension cached This was not a mobile CSS/rendering problem: on the affected existing tab, Fix
The installed global/project copies were updated for live verification; the canonical tracked example carries the fix. Validation on
|
1 participant
Summary
Refactors the local pi-web session host into the shared, transport-ready shape proposed in the latest review on #43. This intentionally contains no runtime/provider behavior and is intended to preserve local behavior exactly.
Reviewable slices
1572070)12b621b)SessionServicecontract and JSON DTOs37d0a70)4dc9e94)server.ts9f446c5)targetSessionserver.tsdrops from 3,146 lines onmainto 1,574 lines, with behavior moved into typed modules rather than duplicated.Regression protection
Validation
npm run typechecknpm run test:unit— 109 passedPI_WEB_E2E_SHARDS=1 npm test— complete auth/desktop/tablet/mobile matrix passednpm run buildgit diff --checkFollow-up work can now add the runner as a thin transport over this service instead of maintaining a capability-limited parallel session implementation.