perf(#1032): use blobless fetch with timeout for pre-review clone deepening - #1033
perf(#1032): use blobless fetch with timeout for pre-review clone deepening#1033fullsend-ai-coder[bot] wants to merge 3 commits into
Conversation
…pening The git fetch --unshallow in pre-review.sh transferred all objects including blobs, adding 1–18 minutes per run on large repos. Tier 2 risk assessment only reads commits and trees (git log, git show --name-only, --grep), so blobs are wasted bandwidth and disk. Two changes: - Add --filter=blob:none to fetch only commit and tree objects, using the named remote `origin` (required for --filter, configured by actions/checkout). The -c http.extraheader auth works unchanged. - Wrap the fetch in `timeout 120` so a slow server falls through to the existing degraded-signal warning path instead of consuming the review budget. Closes #1032
|
🤖 Finished Review · ✅ Success · Started 9:17 PM UTC · Completed 9:29 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $3.14 |
|
Risk Assessment: moderate (2/5) DetailsPR grew from 2 files/8 lines to 5 files/379 lines since prior assessment, but 206 of the new lines are tests, keeping test ratio at 0.20 and mitigating size risk; 4 protected paths elevate Tier 1 change-size composite to 3, and high regression history on pre-review.sh (10 avg fix/revert commits in 90d) elevates Tier 2, but bot authorship, zero security-sensitive files, no CI/dependency changes, and well-scoped issue alignment keep the weighted composite at 2.1, rounding to moderate (2). Previous runRisk Assessment: moderate (2/5) DetailsSmall, well-scoped bot PR (2 files, 8 lines) matching issue requirements exactly, but protected paths and zero test coverage elevate Tier 1, and high recent regression history on pre-review.sh elevates Tier 2, yielding moderate overall risk. |
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
|
fullsend-ai-review
Bot
added
the
requires-manual-review
The blobless deepen leaves origin registered as a promisor remote, so git lazily fetches any blob it needs. The review sandbox cannot serve that fetch — its egress policy allows the GitHub REST API, not the git wire protocol — so the fetch blocks until the run's timeout. Rename detection is the one thing in the Tier 2 command set that reads blob content: inexact (similarity) detection has to compare file contents, and 3 of 4 rename commits sampled from a real repository failed this way against an unreachable promisor. Exact renames are resolved from the blob hash alone, which is why a fixture that renames without editing would not catch this. - set diff.renames=false in the deepened clone, so every Tier 2 history command is blob-free; a rename reported as delete+add still names both paths, so the coupling signal is unchanged - switch the SKILL.md change-coupling command from 'git show --name-only' to 'git diff-tree -r --no-commit-id --name-only --no-renames', and document that history must not be read with show -p/diff/log -p/blame - add --kill-after=5 so a fetch ignoring SIGTERM is still bounded - warn distinctly when GNU timeout is absent (local macOS runs) instead of reporting it as a fetch failure - cover the deepening block with tests: it had none. They run the real fetch against a local bare repo, and assert the resulting clone is non-shallow, blobless, rename-detection-off, and that the coupling command runs with the promisor pointed at a nonexistent path Assisted-by: Claude (fix), Grok (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
f316a65 to
e59212e
Compare
|
🤖 Review · Commit: |
|
🤖 Review · Commit: |
|
Two-model review (Claude + Grok) done on Outstanding: contain the promisor registration (recommend before merge)
…leaving ~14k objects missing (measured on
Suggested addition to git -C "${_TARGET_DIR}" config --unset remote.origin.promisor || true
git -C "${_TARGET_DIR}" config --unset remote.origin.partialclonefilter || trueBoth keys are required. Unsetting only
Also worth folding in
DisclosureFetching by remote name also widens the ref corpus. Measured on I authored the two fix commits here, so this needs a second pair of eyes on the approval rather than mine. |
The blobless fetch does not just shrink the transfer — it converts the target repo into a partial clone, writing remote.origin.promisor and remote.origin.partialclonefilter into the .git/config that ships to the sandbox, with the historical blobs absent. Git's own design note is explicit that this mode 'requires that the user be online and the origin remote ... be available for on-demand fetching of missing objects'. The review sandbox is deliberately not online for git: the egress policy grants the GitHub REST API to gh and node, not the git wire protocol. Any command that reaches for a missing blob therefore blocks until the run's timeout rather than failing. diff.renames=false covers every command in the SKILL.md Tier 2 table, but not one the sub-agent improvises (show -p, log -p, blame). Dropping the registration turns those into an immediate 'unable to read object'. Both keys must go: partialclonefilter on its own re-creates the promisor remote. The unsets are unconditional because the keys are written during fetch setup, so a fetch that fails part-way registers them too. - unset both promisor keys after the fetch, success or failure - scope the credential header to origin's own host, restoring the guarantee the literal-URL form used to give - surface fetch stderr on failure: timeout, auth failure and a server refusing --filter are all actionable and otherwise look identical - SKILL.md: add -m --root to the coupling command, fixing two pre-existing undercounts (merges printed nothing, exact renames named only the destination), and document that a missing-blob error is expected rather than a broken clone - note in-code that fetching by remote name widens the ref corpus from one ref to all branches and tags, which feeds Tier 2 revert frequency - tests: assert both keys are unset and that a historical blob is genuinely absent, rather than asserting the filter config that the containment now removes Assisted-by: Claude (fix), Claude (review), Grok (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
|
Containment landed in |
|
🤖 Finished Review · ✅ Success · Started 5:08 PM UTC · Completed 5:28 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $8.11 |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
1 participant
Summary
--filter=blob:noneto thegit fetch --unshallowinpre-review.shso only commit and tree objects are fetched — blobs are not needed for Tier 2 risk assessment (git log, tree-only diffs,--grep)timeout --kill-after=5 120so a slow pack generation falls through to the existing degraded-signal warning instead of consuming the review budgetorigin(required for--filter; configured by actions/checkout;-c http.extraheaderauth works unchanged)diff.renames=falsein the deepened clone and make the Tier 2 change-coupling command blob-free, so the risk sub-agent can still read history inside the sandboxContext
Since #861,
pre-review.shauto-defaultsREVIEW_GIT_FETCH_DEPTH=0when risk assessment is enabled, runninggit fetch --unshallowon the target repo. On the fullsend e2e fixture repos (~1.6–1.8 GB), this adds 1–18 minutes per review run and intermittently breaks the behaviour suite's 12-minute dispatch-settle window (fullsend#6523).Why the extra commit
A blobless clone registers
originas a promisor remote: git will lazily fetch any blob it needs. The review sandbox cannot serve that fetch — its egress policy allows the GitHub REST API, not the git wire protocol — so the fetch blocks until the run's timeout. Rename detection is the one operation in the Tier 2 command set that reads blob content, because inexact (similarity) detection has to compare file contents. Measured on a real repository with the promisor pointed at a nonexistent path: 3 of 4 rename commits madegit show --name-onlyfail; the remaining one was an exact rename, which git resolves from the blob hash alone.Fix, in two layers:
diff.renames=falsein the deepened clone, and the change-coupling command switched fromgit show --name-onlytogit diff-tree -r --no-commit-id --name-only --no-renames -m --root. A rename reported as delete+add still names both paths, so the coupling signal is unchanged;-m --rootadditionally repairs two pre-existing undercounts, sincegit show --name-onlyprinted nothing for merge commits and only the destination path for exact renames.remote.origin.promisorandremote.origin.partialclonefilterin the.git/configthat ships to the sandbox. Both are unset afterwards, unconditionally (they are written during fetch setup, so a part-way failure registers them too). Both are required:partialclonefilteralone re-creates the promisor remote. With them gone, a command that does need a historical blob fails in 0s withunable to read objectinstead of hanging until the run's 20-minute timeout.GIT_NO_LAZY_FETCHwas rejected as the control: it landed upstream in git 2.45, and the sandbox image is1:2.43.0-1ubuntu7.3— it works there only via an Ubuntu backport, which a base-image change would silently remove.Verification
Measured, not assumed:
--filter=blob:noneis honoured on--unshallow.git59M → 24M, history 1 → 5762 commitsoriginis requiredgit logshapes on a blobless clonegit show --name-onlyon inexact-rename commits--no-renamesanddiff.renames=falseboth fix it, identical file listsgit fsckclean, retry succeeds — the sub-agent's shallow check handles itoriginidentityactions/checkoutrunsgit remote add origin https://github.com/<source_repo>, so it is the target repo over HTTPSgit log --all5,807 → 6,019 commits) — an input to Tier 2 revert frequency, disclosed here and in-coderemote.origin.promisorpartialclonefilterre-registers the remote, so both keys must goTesting
bash scripts/pre-review-test.sh— all pass, including 18 new deepening assertions that run the real--unshallow --filter=blob:nonefetch against a local bare repo (no network) and assert the clone ends up non-shallow, blobless, rename-detection-off, and readable with the promisor pointed at a nonexistent pathdiff.renames=falseis droppedmake check-bundleconfirms the bundledpre-review.shmatches the sourcescripts/post-retro-test.shfails 16 GitLab/curl tests on this workstation, identically on pristineorigin/main— pre-existing and unrelatedCloses #1032