Skip to content

fix(server): show review diffs for checkout projects outside the server root - #8559

Closed
Gigioxx wants to merge 2 commits into
pingdotgg:mainfrom
Gigioxx:fix/review-diff-checkout-projects
Closed

Gigioxx wants to merge 2 commits into
pingdotgg:mainfrom
Gigioxx:fix/review-diff-checkout-projects

Conversation

@Gigioxx

@Gigioxx Gigioxx commented Aug 28, 2026 •

Copy link
Copy Markdown
Contributor

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.assertWorkspaceBoundCwd only 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 because CheckpointDiffQuery has no such boundary.

Change

  • Server: assertWorkspaceBoundCwd also accepts a cwd that exactly matches a registered active project's workspace root, using a new metadata-free ProjectionSnapshotQuery.hasActiveProjectAtWorkspaceRoot existence query (no repository-identity git spawn on the preview path). Query errors fail closed.
  • Web: DiffPanel no 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.
  • Test mocks that build the full ProjectionSnapshotQueryShape gain the new method, including the newer AgentSessionScanner, ProviderService, and serverRuntimeStartup mocks added on main since the PR opened.

Scope and approval

Bug fix for #8506. The issue still carries needs-triage and 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

  • Established the problem on the Windows nightly (0.0.36-nightly): a checkout-mode project outside the server root rendered the wrong repository's diff in the Working tree tab. Earlier head was verified against a dev server with a project outside the server root: correct working-tree diff, no rejections in the trace.
  • Rebased onto main at 0cf482b in this session. Conflicts were only in full-shape test mocks (main added getProjectShells, getEventReplayStats, getImportedAgentSessionSources) and the DiffPanel retry 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 (from apps/server) on ReviewService, ProjectionSnapshotQuery, CheckpointDiffQuery, ProjectSetupScriptRunner, serverRuntimeStartup, ProviderSessionReaper, OrchestrationEngine tests: 7 files, 108 tests passed. ReviewService.test.ts covers a project root outside the server cwd being accepted; ProjectionSnapshotQuery.test.ts covers the existence query for a present and a missing root.
  • Server and web 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 is excludes 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 is Effect.die), so it is unrelated to this change.
  • vp lint on changed files: no new findings (two existing warnings in DiffPanel.tsx outside this change). vp fmt --check: clean.
  • Not checked in this session: a real desktop or WSL run on the rebased head. The screenshots below are from the earlier head.

Before (Windows nightly, thread in repo2, panel shows repo's diff):

Wrong repository diff before the fix

After (project outside the server root shows its own working-tree diff):

Correct diff after the fix

Fixes #8506

Changes made by Claude Fable 5 via Claude Code. Rebase and body update by Claude Opus 5.5 via Claude Code.

@coderabbitai

coderabbitai Bot commented Aug 28, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

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 configuration

Configuration used: Repository: pingdotgg/t3code/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: caf522d9-719e-4155-b35d-80665e972018

📥 Commits

Reviewing files that changed from the base of the PR and between c57a04b and c78c0a9.

📒 Files selected for processing (12)
  • apps/server/src/checkpointing/CheckpointDiffQuery.test.ts
  • apps/server/src/orchestration/Layers/OrchestrationEngine.test.ts
  • apps/server/src/orchestration/Layers/ProjectionSnapshotQuery.test.ts
  • apps/server/src/orchestration/Layers/ProjectionSnapshotQuery.ts
  • apps/server/src/orchestration/Services/ProjectionSnapshotQuery.ts
  • apps/server/src/project/ProjectSetupScriptRunner.test.ts
  • apps/server/src/provider/Layers/ProviderSessionReaper.test.ts
  • apps/server/src/review/ReviewService.test.ts
  • apps/server/src/review/ReviewService.ts
  • apps/server/src/server.test.ts
  • apps/server/src/serverRuntimeStartup.test.ts
  • apps/web/src/components/DiffPanel.tsx

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.


📝 Walkthrough

Walkthrough

The 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.

Changes

Workspace-root diff preview

Layer / File(s) Summary
Active-project workspace-root lookup
apps/server/src/orchestration/Services/ProjectionSnapshotQuery.ts, apps/server/src/orchestration/Layers/ProjectionSnapshotQuery.ts, apps/server/src/orchestration/Layers/ProjectionSnapshotQuery.test.ts, apps/server/src/checkpointing/CheckpointDiffQuery.test.ts, apps/server/src/orchestration/Layers/OrchestrationEngine.test.ts, apps/server/src/project/ProjectSetupScriptRunner.test.ts, apps/server/src/provider/Layers/ProviderSessionReaper.test.ts, apps/server/src/server.test.ts, apps/server/src/serverRuntimeStartup.test.ts
The query contract and implementation add an exact workspace-root lookup for active projects. Tests cover matching and missing roots, and query mocks implement the added method.
Diff preview working-directory validation
apps/server/src/review/ReviewService.ts, apps/server/src/review/ReviewService.test.ts
ReviewService accepts a working directory that matches an active project workspace root. A test checks that the preview retains that directory and passes it to repository detection.
Active working-directory diff query
apps/web/src/components/DiffPanel.tsx
DiffPanel uses the preview query at the active working directory and removes the retry and fallback query using the configured workspace root.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to c78c0

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 Review

Security architecture risk: 🔵 Low · up to c78c0

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
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — A caller holding review:write can now request repository patches and supported file contents at registered active roots outside the configured roots. The demonstrated scope is the connected server environment and data readable by its process, not arbitrary other environments or tenants.

Trust Boundaries and Controls

  • observed — Both review RPCs require review:write in the authenticated session. The inspected authorization and project-root lookup are environment-wide and contain no caller/project ownership comparison; that absence alone does not establish a tenant-isolation regression.
  • observed — Existing Git controls disable external diff and text conversion, use literal file pathspecs, and bound command output. Working-tree expansion checks lexical and real-path containment, regular-file type, and file size before reading.

Resilience and Maintainability Implications

  • inferred — The active-root decision is a point-in-time check, followed by independent driver operations. Concurrent deletion, root updates, or filesystem retargeting are not coupled to completion of the read. No requirement for immediate in-flight revocation or demonstrated exploit was established.

Hardening Proposals

  • proposed — If project registration is intended to authorize a stable filesystem target rather than a directory name, define that identity contract and bind authorization and driver access to the same canonical target. Also define whether project removal must revoke already-started reads.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 12 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Issue #8506 requires working review diffs for changed code. The PR updates ReviewService to accept a cwd that exactly matches an active project workspace root. It adds `ProjectionSnapshotQuery.hasAc…
Out of Scope Changes check ✅ Passed The changes stay within issue #8506. Server changes authorize review previews for registered checkout project roots. Web changes remove an unsafe fallback that could show the wrong diff. Test-double u…
Title check ✅ Passed The title is concise, uses the conventional commit format, and clearly describes the primary fix for review diffs in checkout projects outside the server root.
Description check ✅ Passed The description covers the problem, implementation, scope, verification results, known test limitation, and UI evidence. It also identifies that issue #8506 still lacks maintainer triage, but the desc…
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Aug 28, 2026
@macroscopeapp

macroscopeapp Bot commented Aug 28, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at a8e3297

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:

  • No code objects were reviewed. Approvability was decided on eligibility alone.

You can add or adjust custom eligibility rules. Learn more.

@jadeva

jadeva commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

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. CheckpointDiffQuery takes its cwd from the projection (worktreePath ?? workspaceRoot) and applies no boundary check, so turn diffs work regardless of where the project lives. ReviewService.getDiffPreview is the only diff path behind assertWorkspaceBoundCwd. Projects in T3-created worktrees are unaffected because those sit under config.worktreesDir, which is why this presents as "diffs only work in worktrees".

On the "expands the server's filesystem boundary beyond its configured roots" flag: the rest of the server does not hold that boundary. WorkspaceFileSystem and WorkspaceSearchIndex never read ServerConfig; they scope to the project's own workspace root. WorkspacePaths.normalizeWorkspaceRoot accepts any resolvable directory. Grepping config.cwd across apps/server/src, every other use is a default cwd or a bootstrap/naming value. ReviewService.ts:73 is the only place it acts as an access boundary. So this isn't one relaxation away from the server's access model, it's the single place that diverges from it, and that divergence is what breaks checkout projects. Restricting a read-only diff to config.cwd while the agent writes freely in the project root doesn't buy much.

I hit this independently and wrote the same fix before reading this diff: same hasActiveProjectAtWorkspaceRoot existence query, same fail-closed handling, same removal of the client-side retry. To be clear about what I tested, it was my own equivalent patch rather than this branch. Production desktop build on Windows 11, project outside the server root, Working tree and Branch changes now render, turn diffs unaffected. Focused tests, server and web typecheck, and lint all pass.

One note for anyone reproducing: adding the method to ProjectionSnapshotQueryShape breaks six test files that build the full shape rather than partial mocks (CheckpointDiffQuery, OrchestrationEngine, ProjectSetupScriptRunner, ProviderSessionReaper, serverRuntimeStartup, and ReviewService).

@Gigioxx
Gigioxx force-pushed the fix/review-diff-checkout-projects branch from d913333 to c78c0a9 Compare October 1, 2026 05:03
@Gigioxx

Gigioxx commented Oct 1, 2026

Copy link
Copy Markdown
Contributor Author

[claude-opus-5-5] Responding on behalf of Guille

Rebased onto current main. Thanks @jadeva for the note on full-shape mocks: besides the original six, main has since added three more (AgentSessionScanner, ProviderService, serverRuntimeStartup error case), which now include hasActiveProjectAtWorkspaceRoot in a8e3297. Server and web typecheck pass; focused tests listed in the description.

@juliusmarminge

Copy link
Copy Markdown
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews size:M 30-99 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Git/Last turn doesn't work on Windows in nightly

3 participants