Skip to content

[codex] Preserve review path resolution failures#3357

Merged
juliusmarminge merged 1 commit into
mainfrom
codex/preserve-review-path-causes
Jun 20, 2026
Merged

[codex] Preserve review path resolution failures#3357
juliusmarminge merged 1 commit into
mainfrom
codex/preserve-review-path-causes

Conversation

@juliusmarminge

@juliusmarminge juliusmarminge commented Jun 20, 2026

Copy link
Copy Markdown
Member

Summary

  • only treat missing review paths as a lexical-path fallback
  • surface other canonicalization failures as structured VcsRepositoryDetectionError values
  • retain the exact platform error as cause and cover the failure path

Verification

  • vp test apps/server/src/review/ReviewService.test.ts
  • vp check (passes with 20 pre-existing warnings)
  • vp run typecheck

Note

Medium Risk
Changes how review cwd is canonicalized before workspace-boundary checks; misclassification could weaken validation or break legitimate previews, but scope is limited to review path resolution.

Overview
Review workspace path validation no longer treats every realPath failure as a silent fallback to the lexical resolved path.

canonicalizePath in ReviewService now only falls back when the platform error is NotFound (path does not exist yet). Any other PlatformError during canonicalization fails with a structured VcsRepositoryDetectionError (operation: ReviewService.assertWorkspaceBoundCwd.canonicalizePath), a clear detail message, and the original PlatformError preserved as cause.

A new test drives an invalid cwd (embedded null) through getDiffPreview and asserts that failure shape, that PlatformError is on the cause, and that VCS detect is not invoked when canonicalization fails early.

Reviewed by Cursor Bugbot for commit 5b28127. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Preserve non-NotFound path resolution failures in ReviewService.canonicalizePath

Previously, canonicalizePath in ReviewService.ts silently swallowed all realPath errors by returning the resolved path. Now it only falls back on NotFound errors; any other PlatformError causes getDiffPreview to fail early with a VcsRepositoryDetectionError that includes the original error as cause and contextual metadata. A new test in ReviewService.test.ts covers this path using an invalid NUL-containing cwd.

Macroscope summarized 5b28127.

Co-authored-by: codex <codex@users.noreply.github.com>
@juliusmarminge juliusmarminge added the vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. label Jun 20, 2026
@coderabbitai

coderabbitai Bot commented Jun 20, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: a83e9db5-57de-48c6-bd3e-bc79aebc9da1

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/preserve-review-path-causes

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

@github-actions github-actions Bot added the size:S 10-29 changed lines (additions + deletions). label Jun 20, 2026
@macroscopeapp

macroscopeapp Bot commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved

This is a small, focused bug fix that improves error handling by preserving unexpected path-resolution failures instead of silently swallowing them. The change includes a corresponding test and has limited scope.

You can customize Macroscope's approvability policy. Learn more.

@juliusmarminge
juliusmarminge merged commit bf1a650 into main Jun 20, 2026
17 checks passed
@juliusmarminge
juliusmarminge deleted the codex/preserve-review-path-causes branch June 20, 2026 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S 10-29 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant