Repository navigation
feat(review): provision worktree dependencies from a host cache on the persistent runners #10129
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
8fe8c66
3fa88cf
d686bc3
b7d6a51
00ae2cc
a630b7c
7197b61
1adf3a5
aae92c0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,116 @@ | ||
| #!/usr/bin/env bash | ||
| # Provision the review dependency cache on the persistent runners | ||
| # (qwen-code-pr-review.yml, 'Provision review dependency cache' — #10108). | ||
| # | ||
| # The review worktree `fetch-pr` builds is a bare checkout, and a full | ||
| # `npm ci` plus the prepare build does not fit inside a review agent's tool | ||
| # budget — so every probe that decided the right evidence was "run the test" | ||
| # burned its budget on a doomed install and the round downgraded to a | ||
| # disclosed read-only audit (PR #9729 rounds 13/15, PR #9940's own review). | ||
| # On the persistent pool the install can instead happen ONCE, here, before | ||
| # the agent starts and outside any agent's budget: this script keeps | ||
| # $HOME/.qwen-review-deps/<lockfile-sha256> populated — a real `npm ci` | ||
| # result (root and nested node_modules, every workspace's built dist, the | ||
| # lockfile that produced them, npm's own completeness marker) snapshotted by | ||
| # hardlink and renamed into place atomically, marker file last — and | ||
| # `fetch-pr` link-farms the entry matching the PR's OWN lockfile into the | ||
| # worktree it creates (QWEN_REVIEW_DEPS_CACHE, exported below). | ||
| # | ||
| # A cold cache or a PR that changes the lockfile falls back to today's | ||
| # behaviour inside `fetch-pr`, so this script must never fail the job: the | ||
| # step's continue-on-error is the belt, the guards below are the suspenders, | ||
| # and its timeout-minutes bounds a hung registry the way the capture-tools | ||
| # step bounds a hung mirror. Exercised — real bash, stubbed npm/df — by | ||
| # scripts/tests/qwen-pr-review-workflow.test.js. | ||
|
|
||
| # Deliberately +e: every failure below degrades to the pre-cache behaviour, | ||
| # and the trailing `exit 0` makes that total. | ||
| set -u | ||
| set +e | ||
| CACHE_ROOT="$HOME/.qwen-review-deps" | ||
| mkdir -p "$CACHE_ROOT" || exit 0 | ||
| # Export unconditionally and FIRST: the worktree's lockfile picks the entry, | ||
| # so an OLDER entry can still serve a PR based before a lockfile bump even | ||
| # when this run's population fails or is skipped. | ||
| echo "QWEN_REVIEW_DEPS_CACHE=$CACHE_ROOT" >> "$GITHUB_ENV" | ||
| if [ ! -f package-lock.json ]; then | ||
| echo 'no package-lock.json in the checkout; nothing to provision' | ||
| exit 0 | ||
| fi | ||
| HASH="$(sha256sum package-lock.json | cut -d' ' -f1)" | ||
| ENTRY="$CACHE_ROOT/$HASH" | ||
| if [ -f "$ENTRY/.qwen-review-deps-complete" ]; then | ||
| # Freshen the mtime the prune below sorts by, so an entry that is still | ||
| # serving reviews is never the one evicted. | ||
| touch "$ENTRY" 2>/dev/null | ||
| echo "dependency cache warm: $ENTRY" | ||
| exit 0 | ||
| fi | ||
| # Disk gate, same contract as build-test's: an `npm ci` that dies on ENOSPC | ||
| # leaves a partial tree AND a full disk for every later step. The install | ||
| # plus the snapshot need roughly two tree copies. | ||
| free_kb="$(df -Pk "$HOME" 2>/dev/null | awk 'NR==2 {print $4}')" | ||
| if [ -n "${free_kb:-}" ] && [ "$free_kb" -lt 10485760 ]; then | ||
| echo "::warning::dependency cache not populated: ${free_kb}KB free under \$HOME (need ~10G)" | ||
| exit 0 | ||
| fi | ||
| echo "dependency cache cold for lockfile ${HASH}; installing (once per lockfile per runner, outside every agent's budget)" | ||
| # Plain `npm ci`, prepare hook INCLUDED: the hook builds every workspace's | ||
| # dist, which is exactly the prebuilt-sibling half of what the cache exists | ||
| # to serve. | ||
| if ! npm ci --no-audit --no-fund --progress=false; then | ||
| echo '::warning::npm ci failed; reviews on this runner keep the uncached behaviour' | ||
| exit 0 | ||
| fi | ||
| # Stage INSIDE the cache root so the hardlinks and the final rename stay on | ||
| # one filesystem; the rename is what makes a torn snapshot impossible to | ||
| # observe (the completeness marker lands in the stage, so it exists only in | ||
| # entries that arrived whole). | ||
| STAGE="$(mktemp -d "$CACHE_ROOT/.stage.XXXXXX")" || exit 0 | ||
| snapshot() { | ||
| # Hardlink first (instant, and npm replaces files rather than editing in | ||
| # place, so shared inodes stay what `npm ci` wrote); plain copy as the | ||
| # cross-device fallback. | ||
| local rel="$1" | ||
| mkdir -p "$STAGE/$(dirname "$rel")" && | ||
| { cp -al "$rel" "$STAGE/$rel" 2>/dev/null || { rm -rf "${STAGE:?}/$rel"; cp -a "$rel" "$STAGE/$rel"; }; } | ||
| } | ||
| ok=true | ||
| snapshot package-lock.json || ok=false | ||
| while IFS= read -r path; do | ||
| snapshot "${path#./}" || { ok=false; break; } | ||
| done < <(find . \( -name .git -prune \) -o \( -type d -name node_modules -print -prune \)) | ||
| while IFS= read -r path; do | ||
| snapshot "${path#./}" || { ok=false; break; } | ||
| done < <(find . \( -name .git -o -name node_modules \) -prune -o -type d -name dist ! -path ./dist -print -prune) | ||
| if [ "$ok" != true ]; then | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [R1-8] Deferred to the next round: this round ran under the budget brake (two prior rounds exhausted the agent budget before finishing), so it implemented only the blocking subset — the nine Critical findings, now fixed with witnessed tests in commit a630b7c. This Suggestion remains queued and is in scope for the next round. 中文说明[R1-8] 推迟到下一轮:本轮在预算刹车下运行(此前两轮在完成任何工作前耗尽了预算),因此只实现了阻断子集——九个 Critical 发现,已在提交 a630b7c 中以有见证的测试修复。该 Suggestion 仍在队列中,属于下一轮范围。 |
||
| echo '::warning::dependency-cache snapshot failed; discarding the stage' | ||
| rm -rf "$STAGE" | ||
| exit 0 | ||
|
Comment on lines
+153
to
+155
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Deferred to the next round (batch cap). Verified real against the current script: the snapshot-failure, seal-failure, and manifest-mv-failure paths all 中文说明顺延至下一轮(本轮批次上限)。已对当前脚本核实属实:快照失败、密封失败、清单 mv 失败三条路径都在脚本末尾的 workspace 清理之前 |
||
| fi | ||
| : > "$STAGE/.qwen-review-deps-complete" | ||
| if mv -T "$STAGE" "$ENTRY" 2>/dev/null; then | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [R1-18] Deferred to the next round: this round ran under the budget brake (two prior rounds exhausted the agent budget before finishing), so it implemented only the blocking subset — the nine Critical findings, now fixed with witnessed tests in commit a630b7c. This Suggestion remains queued and is in scope for the next round. 中文说明[R1-18] 推迟到下一轮:本轮在预算刹车下运行(此前两轮在完成任何工作前耗尽了预算),因此只实现了阻断子集——九个 Critical 发现,已在提交 a630b7c 中以有见证的测试修复。该 Suggestion 仍在队列中,属于下一轮范围。
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [R1-18] Deferred to the next round: this round ran under the budget brake (two prior rounds exhausted the agent budget before finishing), so it implemented only the blocking subset — the nine Critical findings, now fixed with witnessed tests in commit a630b7c. This Suggestion remains queued and is in scope for the next round. 中文说明[R1-18] 推迟到下一轮:本轮在预算刹车下运行(此前两轮在完成任何工作前耗尽了预算),因此只实现了阻断子集——九个 Critical 发现,已在提交 a630b7c 中以有见证的测试修复。该 Suggestion 仍在队列中,属于下一轮范围。 |
||
| echo "dependency cache populated: $ENTRY" | ||
| else | ||
| # Lost a same-host race, or the entry appeared some other way — either way | ||
| # the stage is surplus. | ||
| rm -rf "$STAGE" | ||
| if [ -f "$ENTRY/.qwen-review-deps-complete" ]; then | ||
| echo "dependency cache populated concurrently: $ENTRY" | ||
| else | ||
| echo '::warning::could not move the staged dependency cache into place' | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [R2-3] Deferred to the next round: this round ran under the budget brake (two prior rounds exhausted the agent budget before finishing), so it implemented only the blocking subset — the nine Critical findings, now fixed with witnessed tests in commit a630b7c. This Suggestion remains queued and is in scope for the next round. Note: this round's R1-1 fix changed the same arm's surroundings (the warm path now verifies rev+manifest and rebuilds), so the next round should re-probe this against the current script before implementing. 中文说明[R2-3] 推迟到下一轮:本轮在预算刹车下运行(此前两轮在完成任何工作前耗尽了预算),因此只实现了阻断子集——九个 Critical 发现,已在提交 a630b7c 中以有见证的测试修复。该 Suggestion 仍在队列中,属于下一轮范围。 注意:本轮 R1-1 修复改动了同一分支的周边(warm 路径现在校验 rev+清单并重建),下一轮实现前应先对当前脚本重新探测。 |
||
| fi | ||
| fi | ||
| # The built state now lives in the cache; drop it from the workspace so the | ||
| # next job's checkout clean and ownership repair do not crawl a two-gigabyte | ||
| # tree that no longer serves anything. | ||
| find . \( -name .git -prune \) -o \( -type d -name node_modules -print -prune \) | while IFS= read -r path; do rm -rf "$path"; done | ||
| find . \( -name .git -o -name node_modules \) -prune -o -type d -name dist -print -prune | while IFS= read -r path; do rm -rf "$path"; done | ||
| # Prune: newest three entries stay (~2G each; distinct installs share no | ||
| # inodes). Sweep stale stages too — an entry name is 64 hex chars, so | ||
| # neither pattern can touch the other's files. | ||
| find "$CACHE_ROOT" -maxdepth 1 -mindepth 1 -type d -printf '%T@ %f\n' 2>/dev/null | | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [R1-5] Deferred to the next round: this round ran under the budget brake (two prior rounds exhausted the agent budget before finishing), so it implemented only the blocking subset — the nine Critical findings, now fixed with witnessed tests in commit a630b7c. This Suggestion remains queued and is in scope for the next round. 中文说明[R1-5] 推迟到下一轮:本轮在预算刹车下运行(此前两轮在完成任何工作前耗尽了预算),因此只实现了阻断子集——九个 Critical 发现,已在提交 a630b7c 中以有见证的测试修复。该 Suggestion 仍在队列中,属于下一轮范围。
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [R1-5] Deferred to the next round: this round ran under the budget brake (two prior rounds exhausted the agent budget before finishing), so it implemented only the blocking subset — the nine Critical findings, now fixed with witnessed tests in commit a630b7c. This Suggestion remains queued and is in scope for the next round. 中文说明[R1-5] 推迟到下一轮:本轮在预算刹车下运行(此前两轮在完成任何工作前耗尽了预算),因此只实现了阻断子集——九个 Critical 发现,已在提交 a630b7c 中以有见证的测试修复。该 Suggestion 仍在队列中,属于下一轮范围。 |
||
| sort -rn | awk '{print $2}' | grep -Ex '[0-9a-f]{64}' | tail -n +4 | | ||
| while IFS= read -r old; do rm -rf "${CACHE_ROOT:?}/$old"; done | ||
| find "$CACHE_ROOT" -maxdepth 1 -name '.stage.*' -mmin +240 -exec rm -rf {} + 2>/dev/null | ||
| exit 0 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -101,6 +101,11 @@ import { | |
| clearRoundStamps, | ||
| } from './lib/deadline.js'; | ||
| import { certifierMatchesRound, roundModelIdFrom } from './lib/round-model.js'; | ||
| import { | ||
| DEPS_CACHE_ENV, | ||
| provisionWorktreeDependencies, | ||
| type WorktreeDependencyProvision, | ||
| } from './lib/dep-provision.js'; | ||
|
|
||
| interface PrMetadata { | ||
| headRefName: string; | ||
|
|
@@ -181,6 +186,17 @@ type FetchPrResult = PlanReport & { | |
| baseRefName: string; | ||
| headRefName: string; | ||
| isCrossRepository: boolean; | ||
| /** | ||
| * What the host dependency cache did to the worktree, when the environment | ||
| * names one (`QWEN_REVIEW_DEPS_CACHE`, set on CI's persistent runners — | ||
| * issue #10108). `provisioned: true` means the worktree already resolves | ||
| * its dependencies (a link farm into the cache, npm's completeness marker | ||
| * in place, prebuilt sibling `dist` copied in), so `build-test` skips its | ||
| * install and probes can run tests without one. Anything else carries the | ||
| * `reason` and the review behaves exactly as it did before the cache | ||
| * existed. Absent entirely when no cache is configured (every local run). | ||
| */ | ||
| dependencies?: WorktreeDependencyProvision; | ||
| diffStat: { files: number; additions: number; deletions: number }; | ||
| /** | ||
| * The merge-base diff is EMPTY: the branch tree is byte-identical to its | ||
|
|
@@ -989,6 +1005,45 @@ async function runFetchPr(args: FetchPrArgs): Promise<void> { | |
| ); | ||
| } | ||
|
|
||
| // 4b. Provision the worktree's dependencies from the host cache, when the | ||
| // environment names one (CI's persistent runners do — issue #10108). | ||
| // A link farm takes seconds where the install it replaces does not | ||
| // fit an agent's tool budget, and it runs HERE so every agent — | ||
| // probes and verifiers included, not just Agent 7 — finds the tree | ||
| // already installed. Best-effort by contract: the provisioner | ||
| // records a reason instead of throwing, and the guard below exists | ||
| // only for the throw it promises not to make — a fetch that built a | ||
| // perfectly good worktree must not die on a cache fault whose | ||
| // fallback (the disclosed read-only round) is the pre-cache status | ||
| // quo. Absent from the report entirely when no cache is configured, | ||
| // so local reviews read the plan they always did. | ||
| const depsCacheRoot = process.env[DEPS_CACHE_ENV]; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [R1-6] Deferred to the next round: this round ran under the budget brake (two prior rounds exhausted the agent budget before finishing), so it implemented only the blocking subset — the nine Critical findings, now fixed with witnessed tests in commit a630b7c. This Suggestion remains queued and is in scope for the next round. 中文说明[R1-6] 推迟到下一轮:本轮在预算刹车下运行(此前两轮在完成任何工作前耗尽了预算),因此只实现了阻断子集——九个 Critical 发现,已在提交 a630b7c 中以有见证的测试修复。该 Suggestion 仍在队列中,属于下一轮范围。
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [R1-6] Deferred to the next round: this round ran under the budget brake (two prior rounds exhausted the agent budget before finishing), so it implemented only the blocking subset — the nine Critical findings, now fixed with witnessed tests in commit a630b7c. This Suggestion remains queued and is in scope for the next round. 中文说明[R1-6] 推迟到下一轮:本轮在预算刹车下运行(此前两轮在完成任何工作前耗尽了预算),因此只实现了阻断子集——九个 Critical 发现,已在提交 a630b7c 中以有见证的测试修复。该 Suggestion 仍在队列中,属于下一轮范围。 |
||
| let dependencies: WorktreeDependencyProvision | undefined; | ||
| if (depsCacheRoot) { | ||
| try { | ||
| dependencies = provisionWorktreeDependencies(wt, depsCacheRoot); | ||
| } catch (err) { | ||
| dependencies = { | ||
| provisioned: false, | ||
| source: null, | ||
| reason: `provisioning threw: ${(err as Error).message}`, | ||
| linked: 0, | ||
| failed: 0, | ||
| selfLinked: 0, | ||
| distCopied: 0, | ||
| }; | ||
| } | ||
| writeStderrLine( | ||
| dependencies.provisioned | ||
| ? `Dependencies provisioned from ${dependencies.source}: ` + | ||
| `${dependencies.linked} packages linked ` + | ||
| `(${dependencies.selfLinked} workspace self-links), ` + | ||
| `${dependencies.distCopied} prebuilt dist trees copied.` | ||
| : `Dependencies NOT provisioned (${dependencies.reason}); ` + | ||
| `build-test installs on its own path as before.`, | ||
| ); | ||
| } | ||
|
|
||
| mkdirSync(REVIEW_TMP_DIR, { recursive: true }); | ||
|
|
||
| // 5. Capture the diff to a file and partition it. The capture is decoded | ||
|
|
@@ -1610,6 +1665,7 @@ async function runFetchPr(args: FetchPrArgs): Promise<void> { | |
| baseRefName: meta.baseRefName, | ||
| headRefName: meta.headRefName, | ||
| isCrossRepository: meta.isCrossRepository, | ||
| ...(dependencies ? { dependencies } : {}), | ||
| // Two gates, because the SKILL acts on this by recommending the PR be | ||
| // closed as superseded — the one ruling here that is expensive to get | ||
| // wrong. `diffPath` (set only on a SUCCESSFUL capture): a capture that | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[R1-8] Deferred to the next round: this round ran under the budget brake (two prior rounds exhausted the agent budget before finishing), so it implemented only the blocking subset — the nine Critical findings, now fixed with witnessed tests in commit a630b7c. This Suggestion remains queued and is in scope for the next round.
中文说明
[R1-8] 推迟到下一轮:本轮在预算刹车下运行(此前两轮在完成任何工作前耗尽了预算),因此只实现了阻断子集——九个 Critical 发现,已在提交 a630b7c 中以有见证的测试修复。该 Suggestion 仍在队列中,属于下一轮范围。