Full-context local PR workspace checkout - #55
Conversation
- Checkout full PR head tree with depth-1 fetch for agent cwd - Use GitHub listFiles for changed paths and inline anchors - Rename search budget env vars and add ADR 0017
PR Agent ReviewNote This PR fundamentally reworks the local PR workspace from a materialization-on-demand model (where only changed files were lazily fetched from git objects) to a full shallow checkout of the PR head, providing agents with the complete repository tree for context. Diff metadata (unified patches, commentable anchors) now comes from the GitHub PR files API (
|
![zeus-review[bot]](https://avatars.githubusercontent.com/in/3697123?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Full review is in the PR conversation. Expand below to copy fixes for your coding agent.
Fix all findings (agent prompt)
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate.
Repository: prathamdby/pr-agent
Pull request: #55
Head SHA: 0e1fba1bbc39e2094edc158885d16d012a6a4399
Findings:
[P1] @src/github/listPullRequestFiles.ts line 42
When truncation occurs, either stop returning a numeric `omittedCount` (making it always `0` or `undefined` with a descriptive warning), or change the field name/type to make the lower-bound nature explicit (e.g. `omittedCountLowerBound: number`).
[P2] @src/github/listPullRequestFiles.ts lines 77-82
Move `totalChanges` accumulation before the truncation check, or compute it from a separate pass/summary. Git's `--numstat` approach or GitHub's PR summary endpoint can give the full count.
[P2] @src/prWorkspace/localPrWorkspace.ts lines 269-274
Align the conditions: treat empty patches consistently as omitted in both subsystems. Change line 273 to `patchOmitted: file.patchOmitted === true || file.patch == null || file.patch === ''`.
|
|
||
| let truncated = false; | ||
| let omittedCount = 0; | ||
| let omittedCountLowerBound = false; |
There was a problem hiding this comment.
P1 · omittedCount only captures current page tail across paginated responses
src/github/listPullRequestFiles.ts · line 42
When truncation occurs mid-page, omittedCount only counts remaining items on the current page (line 42: data.length - consumed). No subsequent pages are fetched or counted. The warning message uses omittedCountLowerBound (line 89) and prefixes with "at least", but the numeric omittedCount field in ListPullRequestFilesResult has no such qualifier. Any consumer reading the number directly will see a misleading count that is only the current page's tail, not all omitted files across all pages.
Prompt to fix
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate.
Repository: prathamdby/pr-agent
Pull request: #55
Head SHA: 0e1fba1bbc39e2094edc158885d16d012a6a4399
[P1] @src/github/listPullRequestFiles.ts line 42
When truncation occurs, either stop returning a numeric `omittedCount` (making it always `0` or `undefined` with a descriptive warning), or change the field name/type to make the lower-bound nature explicit (e.g. `omittedCountLowerBound: number`).
| patchOmitted = undefined; | ||
| } else { | ||
| patchCapReached = true; | ||
| patchOmittedCount++; |
There was a problem hiding this comment.
P2 · totalChanges under-counted when files truncated by maxPrFilesListed
src/github/listPullRequestFiles.ts · lines 77-82
totalChanges += file.changes runs only for files that pass the maxPrFilesListed truncation check (lines 77-82). When truncation occurs, files after the limit are skipped via break before their changes are accumulated. The old code computed totalChanges from git diff --numstat for ALL files pre-truncation, so it always reflected the full PR change count. Now stats.totalChanges will under-report large PRs, affecting review-effort heuristics that depend on this value.
Prompt to fix
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate.
Repository: prathamdby/pr-agent
Pull request: #55
Head SHA: 0e1fba1bbc39e2094edc158885d16d012a6a4399
[P2] @src/github/listPullRequestFiles.ts lines 77-82
Move `totalChanges` accumulation before the truncation check, or compute it from a separate pass/summary. Git's `--numstat` approach or GitHub's PR summary endpoint can give the full count.
| const patch = file.patch ?? ""; | ||
| if (file.patchOmitted || !file.patch) { | ||
| patchOmittedPaths.add(file.filename); | ||
| } else if (patch.length > 0) { |
There was a problem hiding this comment.
P2 · Empty-string patches have inconsistent omitted flags across subsystems
src/prWorkspace/localPrWorkspace.ts · lines 269-274
When file.patch is an empty string (""), line 270 adds the file to patchOmittedPaths (because !file.patch is true), but line 273's patchOmitted evaluates to false (because file.patch == null is false). getDiffForPath returns "[patch omitted]" while the diff index records patchOmitted: false with empty commentable ranges. This inconsistency is benign currently but creates a confusing mismatch for any code reading one flag and expecting the other to agree.
Prompt to fix
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate.
Repository: prathamdby/pr-agent
Pull request: #55
Head SHA: 0e1fba1bbc39e2094edc158885d16d012a6a4399
[P2] @src/prWorkspace/localPrWorkspace.ts lines 269-274
Align the conditions: treat empty patches consistently as omitted in both subsystems. Change line 273 to `patchOmitted: file.patchOmitted === true || file.patch == null || file.patch === ''`.
There was a problem hiding this comment.
Fix all findings (agent prompt)
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate.
Repository: prathamdby/pr-agent
Pull request: #55
Head SHA: fac2d286f36f6b50c3e1ed8b467b5f9dd992dbbd
Findings:
[P2] @src/prWorkspace/localPrWorkspace.ts lines 249-277
Use a single consistent expression for both branches: extract `const hasPatch = !file.patchOmitted && file.patch != null && file.patch.length > 0` and reuse it for `patchByPath` insertion and `filesForIndex.patch`/`patchOmitted` assignment.
| params: PrepareLocalPrWorkspaceParams, | ||
| ): Promise<LocalPrWorkspace> { | ||
| const { cfg, owner, repo, prNumber, baseSha, headSha, installationToken, baseRef } = params; | ||
| const { cfg, owner, repo, prNumber, headSha, installationToken, prFiles } = params; |
There was a problem hiding this comment.
P2 · filesForIndex computes patch twice, creating inconsistency risk
src/prWorkspace/localPrWorkspace.ts · lines 249-277
The loop at lines 249-277 constructs filesForIndex with a redundant patch computation: line 253 computes patch = file.patch ?? "" for the patchByPath/patchOmittedByCapPaths logic, but line 270 recomputes patch: file.patchOmitted || !file.patch ? undefined : file.patch. These two expressions can disagree when file.patch is an empty string "": the first treats it as "no patch content" (skips patchByPath), while the second treats !"" as truthy and sets patch: undefined and patchOmitted: true. Although GitHub rarely returns empty-string patches, if one arrives the diff index would mark it as patchOmitted while getDiffForPath would return "" (empty string) — the file would be treated as having no diff in the index but would silently return empty string from the diff tool, creating an inconsistent UX.
Prompt to fix
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate.
Repository: prathamdby/pr-agent
Pull request: #55
Head SHA: fac2d286f36f6b50c3e1ed8b467b5f9dd992dbbd
[P2] @src/prWorkspace/localPrWorkspace.ts lines 249-277
Use a single consistent expression for both branches: extract `const hasPatch = !file.patchOmitted && file.patch != null && file.patch.length > 0` and reuse it for `patchByPath` insertion and `filesForIndex.patch`/`patchOmitted` assignment.
|
/review-quality |
PR Agent Quality ReviewNote This PR fundamentally restructures the local PR workspace from a changed-file-only materialization model to a full shallow checkout of the PR head tree, sourcing diff metadata from the GitHub API instead of local git diff. It extracts the paginated file listing logic into a shared module (
|
There was a problem hiding this comment.
Note
Full review is in the PR conversation. Expand below to copy fixes for your coding agent.
Fix all findings (agent prompt)
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate.
Repository: prathamdby/pr-agent
Pull request: #55
Head SHA: b8469552d990477c18c90cab0f2e7d27d9b941d1
Findings:
[P2] @src/agent/localWorkspaceTools.ts lines 119-124
Consolidate file-size enforcement into a single location. Either have assertReadablePath always check size (including for pre-indexed paths) and remove the redundant check from readWorkspaceFile, or remove the size check from assertReadablePath entirely and enforce it only in the tool layer. The current split is confusing and the check in assertReadablePath is dead code for the common path.
[P2] @src/github/listPullRequestFiles.ts lines 91-107
Simplify the three-branch patch cap logic into a single decision: compute patch and patchOmitted as a pair based on (rawPatch != null, patchCapReached, budget remaining). The current if/else-if/else structure with redundant assignments for patchCapReached (lines 91-95 and 92-102 are functionally identical) makes the intent harder to follow than a plain ternary or early-return helper.
[P2] @src/prWorkspace/localPrWorkspace.ts lines 298-310
Remove the 'readable' return variant and simplify assertReadablePath to a boolean 'isReadable(path): Promise<boolean>'. The path-exists-and-size-check logic is unnecessary since the checkout is fully populated before setReadOnly is called. Move the size check entirely into readWorkspaceFile and getWorkspaceBlame where it's enforced.
[P3 — no inline thread] searchWorkspace silently catches all assertion errors
The try-catch around assertPathAllowedForAsk (line 159) silently swallows any thrown error, not just the expected sensitive-path rejection. If assertPathAllowedForAsk were to throw for any other reason, the file would be silently skipped.
[P3 — no inline thread] getBlameForPath uses 'HEAD' instead of captured headSha
The blame git command uses the literal string 'HEAD' rather than the captured headSha closure variable. While functionally correct because the checkout was set to PR_HEAD_REF, this creates a subtle dependency on the checkout step ordering. If a future refactor removes or reorders the checkout, blame could silently point at the wrong commit.
[P3 — no inline thread] Test casts lack full type safety on tool executor return
The test casts e.g. 'as { files: unknown[]; truncated: boolean; omittedCountLowerBound: number; warning?: string }' instead of using the actual return type from listPullRequestFiles executor. Missing the totalChanges field that the real executor now returns.
| return { path: normalized, size: info.size, content }; | ||
| const buf = await readFile(safePath); | ||
| if (buf.length > limits.maxFileBytes) { | ||
| return { |
There was a problem hiding this comment.
P2 · Redundant file-size check duplicates assertReadablePath gate
src/agent/localWorkspaceTools.ts · lines 119-124
readWorkspaceFile checks buf.length > limits.maxFileBytes after assertReadablePath already verified the file exists and is under maxFileBytes. Since assertReadablePath's 'already' fast path skips the size check (all files are pre-indexed without size filtering), the size enforcement is actually split: assertReadablePath only checks size for files NOT in checkoutPaths (which never happens), and readWorkspaceFile redundantly checks again. This splits the enforcement contract across two layers.
Prompt to fix
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate.
Repository: prathamdby/pr-agent
Pull request: #55
Head SHA: b8469552d990477c18c90cab0f2e7d27d9b941d1
[P2] @src/agent/localWorkspaceTools.ts lines 119-124
Consolidate file-size enforcement into a single location. Either have assertReadablePath always check size (including for pre-indexed paths) and remove the redundant check from readWorkspaceFile, or remove the size check from assertReadablePath entirely and enforce it only in the tool layer. The current split is confusing and the check in assertReadablePath is dead code for the common path.
| } else if (rawPatch != null && patchCapReached) { | ||
| patch = undefined; | ||
| patchOmitted = true; | ||
| patchOmittedCount++; |
There was a problem hiding this comment.
P2 · Patch-omission branching duplicates logic across three branches
src/github/listPullRequestFiles.ts · lines 91-107
The patch cap logic has three branches (patch within cap, patch cap reached, raw patch null) that all effectively set patch/patchedOmitted to the same combination via different paths. The third branch (lines 99-100) and the else-if on line 91 for cap-reached files both set patch=undefined and patchOmitted=true. The null-patch case on lines 99-100 sets patchOmitted to undefined rather than true, which is semantically different (null patch from API vs exceeding byte cap) but both end up treated identically in downstream consumers via the 'patch ?? ""' pattern.
Prompt to fix
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate.
Repository: prathamdby/pr-agent
Pull request: #55
Head SHA: b8469552d990477c18c90cab0f2e7d27d9b941d1
[P2] @src/github/listPullRequestFiles.ts lines 91-107
Simplify the three-branch patch cap logic into a single decision: compute patch and patchOmitted as a pair based on (rawPatch != null, patchCapReached, budget remaining). The current if/else-if/else structure with redundant assignments for patchCapReached (lines 91-95 and 92-102 are functionally identical) makes the intent harder to follow than a plain ternary or early-return helper.
| let checkoutPaths = new Set<string>(); | ||
|
|
||
| async function materializePath(path: string): Promise<"materialized" | "already" | "refused"> { | ||
| async function assertReadablePath(path: string): Promise<"readable" | "already" | "refused"> { |
There was a problem hiding this comment.
P2 · assertReadablePath contains dead code branches
src/prWorkspace/localPrWorkspace.ts · lines 298-310
The function returns 'already' when checkoutPaths.has(normalized) is true. Since checkoutPaths is fully pre-populated by indexCheckedOutFiles() during setup, and the checkout is made read-only immediately after, every reachable file returns 'already'. The stat-and-size-check branch (lines 302-308) that returns 'readable' is dead code — it can never be reached in practice. This dead code creates confusion: callers must handle three return states ('readable', 'already', 'refused') when only two are exercised.
Prompt to fix
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate.
Repository: prathamdby/pr-agent
Pull request: #55
Head SHA: b8469552d990477c18c90cab0f2e7d27d9b941d1
[P2] @src/prWorkspace/localPrWorkspace.ts lines 298-310
Remove the 'readable' return variant and simplify assertReadablePath to a boolean 'isReadable(path): Promise<boolean>'. The path-exists-and-size-check logic is unnecessary since the checkout is fully populated before setReadOnly is called. Move the size check entirely into readWorkspaceFile and getWorkspaceBlame where it's enforced.
- Gate searchWorkspace with ask path safety checks - Rename checkout APIs and remove dead blame deepen env - Split patch-cap omission from absent GitHub patches - Narrow local workspace tools to limits-only config
- Rename omittedCount to omittedCountLowerBound in API surface - Use pulls.get additions+deletions for full totalChanges - Treat empty patches as omitted in workspace and diff index
- Enforce read size limits only in workspace tools - Replace assertReadablePath with isPathInCheckout lookup - Extract patch budget resolver and pathAllowedForAsk gate - Blame at headSha; typed listPullRequestFiles test helper
c70801d to
a4d809c
Compare
There was a problem hiding this comment.
Fix all findings (agent prompt)
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate.
Repository: prathamdby/pr-agent
Pull request: #55
Head SHA: a4d809c4c1a74052638836af1d487b1034291480
Findings:
[P1] @test/localPrWorkspace.test.ts lines 146-147
Remove the `delete.txt` assertion — the rename is represented by `renamed.txt` with `oldPath`. Update the `.toSorted()` expectation at line 141 to `["renamed.txt", "src.txt"]`.
[P2] @src/prWorkspace/localPrWorkspace.ts lines 345-353
Move `indexCheckedOutFiles(agentCwd)` after `removeSymlinks(agentCwd)` (and after the askpass/token file cleanup) so `checkoutPaths` reflects only the files that will survive.
| @@ -80,8 +146,9 @@ describe("local PR workspace", () => { | |||
| "deleted", | |||
There was a problem hiding this comment.
P1 · Test expects delete.txt in changedFiles but rename only yields renamed.txt entry
test/localPrWorkspace.test.ts · lines 146-147
The test at line 146-147 asserts workspace.changedFiles.find((file) => file.path === "delete.txt")?.status is "deleted". However, the rename (delete.txt → renamed.txt) is represented by the GitHub API / test helper as a single entry with filename: "renamed.txt", status: "renamed", previousFilename: "delete.txt". mapGithubStatus maps this to { path: "renamed.txt", status: "renamed", oldPath: "delete.txt" } — no separate entry for delete.txt exists in changedFiles. The assertion on line 146 will fail because find() returns undefined, and ?.status evaluates to undefined, not "deleted".
Prompt to fix
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate.
Repository: prathamdby/pr-agent
Pull request: #55
Head SHA: a4d809c4c1a74052638836af1d487b1034291480
[P1] @test/localPrWorkspace.test.ts lines 146-147
Remove the `delete.txt` assertion — the rename is represented by `renamed.txt` with `oldPath`. Update the `.toSorted()` expectation at line 141 to `["renamed.txt", "src.txt"]`.
| const { stdout: fetchedHead } = await git(["rev-parse", "HEAD"]); | ||
| if (fetchedHead.trim().toLowerCase() !== headSha.toLowerCase()) { | ||
| throw new Error( | ||
| `Fetched PR head ${fetchedHead.trim()} does not match expected headSha ${headSha}`, |
There was a problem hiding this comment.
P2 · checkoutPaths computed before removeSymlinks creates stale entries
src/prWorkspace/localPrWorkspace.ts · lines 345-353
checkoutPaths is populated by indexCheckedOutFiles(agentCwd) at line 345, BEFORE removeSymlinks(agentCwd) at line 352. If the PR head tree contains symlinks (which removeSymlinks deletes), those paths remain in checkoutPaths. Later, isPathInCheckout returns true for those paths, but refuseUnlessReadableFile will stat() the now-deleted file and return "Path is missing from the checkout.". The caller gets an opaque refusal with no indication the path was a removed symlink.
Prompt to fix
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate.
Repository: prathamdby/pr-agent
Pull request: #55
Head SHA: a4d809c4c1a74052638836af1d487b1034291480
[P2] @src/prWorkspace/localPrWorkspace.ts lines 345-353
Move `indexCheckedOutFiles(agentCwd)` after `removeSymlinks(agentCwd)` (and after the askpass/token file cleanup) so `checkoutPaths` reflects only the files that will survive.
- Move indexCheckedOutFiles after removeSymlinks in workspace prep - Align rename test with git-detected rename metadata
1 participant
Summary
pulls.listFilesvia sharedlistPullRequestFiles(ADR 0017); publish behavior is unchangedLOCAL_WORKSPACE_SEARCH_MAX_FILESandLOCAL_WORKSPACE_SEARCH_MAX_TOTAL_BYTES; updates prompts, config docs, and testsTest plan
pnpm test(476 tests)pnpm typecheckPR Agent Description
PR Type
Enhancement, Documentation, Tests
Description
Changes Diagram
File Walkthrough
Enhancement (7 files)
Full head checkout + PR metadata diff
src/prWorkspace/localPrWorkspace.tsgit diffwith GitHub PR file list patchesGIT_WORK_TREENew shared GitHub file listing module
src/github/listPullRequestFiles.tsBinary detection and search budget limits
src/agent/localWorkspaceTools.tsUse fetchPullRequestFiles before prepare
src/prWorkspace/prRepositoryView.tsRename materialization config to search config
src/config.ts + settings/*Pass Config to buildLocalWorkspaceTools
src/agent/{ask,description,review}RunSetup.tsRemove inline listPullRequestFiles logic
src/agent/githubTools.tsDocumentation (4 files)
Prompt updates for full-checkout context
src/agent/*Prompts.ts (5 files)ADR 0017: full-context workspace decision
docs/adr/0017-full-context-local-pr-workspace.mdMark ADR 0015 partially superseded
docs/adr/0015-agent-runner-local-pr-workspace.mdUpdate repo docs for full-context workspace
README.md + CONTEXT.mdTests (3 files)
Update tests for full-checkout workspace
test/localPrWorkspace.test.tsNew tests for searchWorkspace tool
test/localWorkspaceTools.test.tsMock listPullRequestFiles in view tests
test/prRepositoryView.test.ts