Repository navigation
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: pingdotgg/t3code/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (12)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe change adds an active-project workspace-root query, uses it to validate diff preview working directories, and updates DiffPanel to use the active working directory without retrying with the configured workspace root. ChangesWorkspace-root diff preview
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change lets review diffs work for checkout projects outside the server root, and it removes the fallback that could show another repository's diff. No merge-blocking risk was found in the supplied changes. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change narrowly extends diff access to registered active project roots while retaining authentication, rejection of unregistered outside paths, and file-containment checks. Risk appears limited, but directory-identity guarantees and access during concurrent project removal are not fully established. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
ApprovabilityVerdict: Approved at Macroscope's review found this PR approvable — This is a focused bug fix that preserves existing workspace restrictions while allowing review diffs for exact active project roots outside the server root. The web change removes an incorrect fallback that could display an unrelated repository's diff, and the added production logic is small and covered by targeted tests. Notes:
You can add or adjust custom eligibility rules. Learn more. |
|
Confirming this on Windows 11 with the desktop app, plus a couple of things that might help move it along. A repro detail that narrows it sharply: turn diffs render fine. Only the Working tree and Branch changes tabs are empty. On the "expands the server's filesystem boundary beyond its configured roots" flag: the rest of the server does not hold that boundary. I hit this independently and wrote the same fix before reading this diff: same One note for anyone reproducing: adding the method to |
d913333 to
c78c0a9
Compare
[claude-opus-5-5] Responding on behalf of GuilleRebased onto current main. Thanks @jadeva for the note on full-shape mocks: besides the original six, main has since added three more ( |
|
Thanks for working on this. We merged the orchestrator V2 rewrite in #2829, and we are closing this PR as part of that transition. The patch conflicts with the rewrite in apps/server/src/orchestration/Layers/ProjectionSnapshotQuery.ts, apps/server/src/orchestration/Services/ProjectionSnapshotQuery.ts, apps/web/src/components/DiffPanel.tsx. Even where the conflict is small enough to rebase, we are asking for fresh PRs against the new base so we can review and verify the behavior in V2. Sorry for the extra work this creates. If the change is still needed on V2, please rebuild it on current main, verify it there, and open a new PR linking back here. We're closing the current implementation without assuming the underlying request is resolved. |
Problem
On the desktop app (and the WSL backend) the server runs with a cwd that is the user's home or the T3 home, not the project.
ReviewService.assertWorkspaceBoundCwdonly accepted cwds inside that server cwd or the worktrees dir, so any checkout-mode project outside those roots had its Working tree and Branch changes diff rejected. The web client then silently retried at the server's cwd, which rendered another repository's diff or an empty "No patch available" panel with no error (#8506). Turn diffs are unaffected becauseCheckpointDiffQueryhas no such boundary.Change
assertWorkspaceBoundCwdalso accepts a cwd that exactly matches a registered active project's workspace root, using a new metadata-freeProjectionSnapshotQuery.hasActiveProjectAtWorkspaceRootexistence query (no repository-identity git spawn on the preview path). Query errors fail closed.DiffPanelno longer retries the branch diff preview at the server cwd, so a genuine rejection shows its error instead of a wrong or empty diff. Mobile already surfaces the error and needs no change.ProjectionSnapshotQueryShapegain the new method, including the newerAgentSessionScanner,ProviderService, andserverRuntimeStartupmocks added on main since the PR opened.Scope and approval
Bug fix for #8506. The issue still carries
needs-triageand has no maintainer triage comment yet; it has independent confirmations on Windows from other users (#8506 (comment), and #8559 (comment) with an equivalent patch tested on a production Windows build). Maintainer triage of #8506 is still needed.Verification
getProjectShells,getEventReplayStats,getImportedAgentSessionSources) and theDiffPanelretry block; the new method was added next to main's entries, and main's deletion of the startup heartbeat test was kept.vp test run(fromapps/server) onReviewService,ProjectionSnapshotQuery,CheckpointDiffQuery,ProjectSetupScriptRunner,serverRuntimeStartup,ProviderSessionReaper,OrchestrationEnginetests: 7 files, 108 tests passed.ReviewService.test.tscovers a project root outside the server cwd being accepted;ProjectionSnapshotQuery.test.tscovers the existence query for a present and a missing root.tsc --noEmit: both exit 0 (server first flagged the three newer full-shape mocks, fixed in a8e3297).vp test run src/project/AgentSessionScanner.test.ts src/serverRuntimeStartup.test.ts: 94 passed, 1 failed. The failure isexcludes sandboxes reached through a symlink into the worktrees dir, a macOS temp-path symlink case in the scanner that never reaches the projection query (the stub added here isEffect.die), so it is unrelated to this change.vp linton changed files: no new findings (two existing warnings inDiffPanel.tsxoutside this change).vp fmt --check: clean.Before (Windows nightly, thread in
repo2, panel showsrepo's diff):After (project outside the server root shows its own working-tree diff):
Fixes #8506
Changes made by Claude Fable 5 via Claude Code. Rebase and body update by Claude Opus 5.5 via Claude Code.