Synced from GitHub
fix(server): review guides keep excerpts in files with spaces or escaped names#22053
Problem
A review guide drops every excerpt from a changed file whose name holds a space, tab, double quote or backslash. Before storing a guide, the server reads the PR's hunks with git diff --unified=0 and keeps only excerpts that overlap one. The hunk parser takes the text after --- a/ and +++ b/ literally, but Git writes these names in two other forms:
- A name with a tab, quote or backslash arrives quoted and escaped, as in
+++ "b/src/legacy\\rates.ts".core.quotePath=falseonly turns off quoting for non-ASCII bytes. - An unquoted name with a space ends with a tab, as in
+++ b/src/price format.ts<TAB>.
Neither form matches the name from --name-only -z, so the file has no hunks. Its excerpts are removed and the file moves to the "Other files" stop. To reproduce, open a PR that edits src/price format.ts and write a guide.
Change
parseReviewGuideChange now reads each header's name the way Git writes it. It cuts at the first literal tab, because an unquoted name never contains one and a quoted name escapes its own. It decodes quoted names with the shared unquoteGitPatchPath and then removes the a/ or b/ prefix. /dev/null still means the file was added or deleted, and unquoted names are used as written. The guard that treats header-looking lines inside hunks as source lines is unchanged. sidePath in diffFileRevisions.ts uses the same first-tab rule for these headers.
Scope and approval
There's no issue for this. It qualifies as a small fix for an obvious bug: the parser and the --name-only -z list it is matched against disagree about the same file's name. Names Git writes plainly behave as before.
This is independent of #21988, which adds --no-textconv to the same patch read. That PR changes the diff arguments in PullRequestReviewGuideService.ts, and this one changes the parser in pullRequestReviewGuideNormalize.ts. Either can merge first.
Verification
- A new parser test feeds Git's header forms for a space, a tab, a backslash and a deleted quoted name, then runs excerpts through
normalizeReviewGuide. It fails on main and passes on this branch. vp test run src/pullRequest/inapps/server: 8 files, 254 tests passed.tsc-rs --noEmitinapps/serverexited 0.vp lintandvp fmt --checkon the changed files passed.- With real Git 2.56.0, main's parser finds no hunks in the guide's own diff output for four such files. This branch finds hunks for all four.
- Web app on Linux in headed Chrome 155, against a local Forgejo 17 PR that edits
src/index.ts,src/price format.tsandsrc/legacy\rates.ts. On each build I opened the PR, clicked Guide, then Write guide. Claude Sonnet 5.5 wrote both guides.
The new test fails on main and passes on this branch.
New parser test failing on main and passing on this branch
The guide's git diff output for four files, parsed by main and by this branch.
Main finds no hunks for the four files; this branch finds all four
Main: the stop about formatPrice keeps its prose but shows "Context, no code".
Main: the formatPrice stop has no code excerpt
Main: both files end up in "Other files". src/index.ts, which has a plain name, keeps its excerpt in stop 3.
Main: src/legacy\rates.ts and src/price format.ts listed under Other files
This branch: the formatPrice stop shows its excerpt from src/price format.ts, with notes on lines 2 and 3.
This branch: the formatPrice stop shows the excerpt from src/price format.ts
This branch: the rate stop shows its excerpt from src/legacy\rates.ts.
This branch: the legacyRate stop shows the excerpt from src/legacy\rates.ts
Not checked:
- macOS, Windows, the desktop app, and the iOS and Android clients. The fix is in the server, which stores the guide every client reads.
- Hosted forges. The patch read doesn't depend on the forge.
- Names that Git writes with octal escapes, such as control characters. Those depend on
unquoteGitPatchPath, which has its own tests.
Prior PRs: none for this parser. It reuses the shared unquoteGitPatchPath decoder that diffFileRevisions.ts already uses for these headers.
Production fix, tests, PR description and images written with Claude Opus 5.5 in T3 Code Nightly.
🤖 Generated with Claude Opus 5.5 in T3 Code Nightly
Checks
23 successful · 13 skipped
Added 1 commit
- fix(server): read quoted and tab-ended Git paths in review-guide hunks59e5a81
Synced from GitHub
Approvability
Verdict: Approved at 59e5a81
Macroscope's review found this PR approvable — This is a narrowly scoped server bug fix that makes review-guide hunk parsing agree with Git's quoting and tab-terminated path formats. Existing plain-path behavior is preserved, and focused tests cover the affected filename cases.
You can add or adjust custom eligibility rules. Learn more.
Synced from GitHub
No actionable comments were generated in the recent review. 🎉
ℹ️ Recent review info
⚙️ Run configuration
- Configuration used: Path: .coderabbit.config.ts
- Review profile: CHILL
- Plan: Advanced
- Run ID:
b781f16f-8820-410d-9f35-e78ed081e4fd
📥 Commits
Reviewing files that changed from the base of the PR and between d5fdfdcf05330680c2947bbb458772f63dba964f and 59e5a812f9776f11bd66bf379b239f0f0924e2fa.
📒 Files selected for processing: 2
apps/server/src/pullRequest/pullRequestReviewGuideNormalize.test.tsapps/server/src/pullRequest/pullRequestReviewGuideNormalize.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review.
📝 Walkthrough
The review guide normalizer now parses quoted and tab-terminated Git diff paths. Tests cover special-character filenames, deleted-file hunks, and excerpt normalization.
Changes: Git Diff Path Parsing
Layer / File(s) Summary Parse and normalize diff header paths
apps/server/src/pullRequest/pullRequestReviewGuideNormalize.ts,apps/server/src/pullRequest/pullRequestReviewGuideNormalize.test.tsThe normalizer decodes quoted paths, strips the expected a/orb/prefix, and uses the parsed old and new paths. Tests cover spaces, tabs, backslashes, deleted files, and excerpt normalization.Priority: ⬇️ Low
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix
Suggested reviewers:
juliusmarminge
Merge Risk: ⚪ Minimal · up to 59e5a
No actionable issue remains in the changed review-guide path handling. The PR is mergeable after normal checks.
Architecture Summary
Architecture risk: 🟡 Medium · up to 59e5a
The change affects 1 system.
Changed systems: apps/server
Architecture concerns No architecture-level concerns identified.
Review details
Systems and components
- observed — apps/server (service) was modified; 2 changed files map to changed impact.
Before / after behavior
- observed — Modified behavior in apps/server/src/pullRequest/pullRequestReviewGuideNormalize.test.ts: Added a parser and normalization test covering unusual Git path encodings, including spaces, tabs, backslashes, and deleted files. It verifies decoded filenames, old-only hunks for deletions, and excerpt normalization to line 2 for the three added files.
- observed — Modified behavior in apps/server/src/pullRequest/pullRequestReviewGuideNormalize.ts: Added the
unquoteGitPatchPathimport used to decode quoted Git patch paths. - observed — Modified behavior in apps/server/src/pullRequest/pullRequestReviewGuideNormalize.ts: Added
headerPath, which extracts the path token before the first literal tab, decodes quoted Git paths, removes the requesteda/orb/prefix, and returnsnullwhen the prefix does not match. - observed — Modified behavior in apps/server/src/pullRequest/pullRequestReviewGuideNormalize.ts: Replaced fixed-prefix slicing for
---and+++headers withheaderPath, allowing quoted paths and retaining the old-path fallback when no new path is available.
Reliability and maintainability
- inferred — Risk-relevant change factors for apps/server: blast_radius_1; direct_dependents_1
Pre-merge checks: Passed 4
Check name Status Explanation Title check Passed The title clearly and concisely describes the main fix for review-guide excerpts in files with spaces or escaped names. Description check Passed The description includes complete Problem, Change, Scope and approval, and Verification sections. It explains the bug, implementation, approval exception, tests, manual checks, evidence, and known lim… Linked Issues check Passed Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request. Full details: Description check
Explanation
The description includes complete Problem, Change, Scope and approval, and Verification sections. It explains the bug, implementation, approval exception, tests, manual checks, evidence, and known limitations.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create a new PR
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
Note
Written by gpt-6.1-sol on behalf of Maria
Verified in a real dev environment
verified unchanged head 59e5a812f9776f11bd66bf379b239f0f0924e2fa on this linux vm. reproduced first on github/main at 6575ce3250, then installed and ran the exact pr head. only one dev server ran at a time.
used playwright from the repo dependencies with /usr/bin/google-chrome. added a real git project and thread through the app. a real codex gpt-6.1-sol turn created and pushed a small pricing change to a local forgejo instance. both builds generated guides through pr detail → guide → write guide, using codex gpt-6.1-sol and identical guide instructions against the same commits (aa4a307cad68 → 121d88e6cd37). no mocks, seeded databases, or replay provider.
- main: the ordinary
src/index.tsexcerpt survived. excerpts for filenames containing spaces, tabs, a quote, and a backslash were dropped, including the deleted tab-named file. those five files appeared under "other files". - pr: all six excerpts survived in the persisted guide and rendered in the client. five show the added validation lines; the deleted file shows its three removed lines on the old side.
- nearby behavior: the ordinary filename still renders, guide navigation advances between stops, and the stored guide remains usable after a page reload.
- blacksmith: the existing normalizer suite passed 9/9; the existing guide-service suite passed 17/17. hosted ci reports 29 successful checks, zero failing or pending. no source changes or pushes were needed.
before, formatting stop loses its code:
main formatting stop has no code excerpt
before, all five unusual filenames fall into other files:
main lists five unusual filenames under other files
after, the same formatting change retains its excerpt:
pr renders the code excerpt for the filename with a space
after, the deleted tab-named file retains its old-side excerpt:
pr renders the deleted file excerpt on the old side
walkthrough of ordinary, backslash, quote, tab, space, and deleted-file excerpts:
guide navigation through all six real file excerpts
video playback and seeking passed in t3's browser. github renders this external mp4 as an image link rather than an inline player; the four evidence images render correctly there.
short command transcript and persisted excerpt paths
Not verified
native desktop, ios, android, macos, and windows clients were not exercised on this linux vm. local forgejo was the real source-control host for guide generation; hosted github/git.cafe guide generation and remote/relay/tunnel connections were not exercised. other providers and additional octal-escaped/non-ascii filename cases were not exercised. the existing diff header display truncates literal tab filenames after the tab; the stored guide paths and rendered code excerpts retain the correct files. that separate display behavior was left unchanged.