diff --git a/scripts/github/dependency-guard.mjs b/scripts/github/dependency-guard.mjs index 31fcb245c165..cc4ef930999e 100644 --- a/scripts/github/dependency-guard.mjs +++ b/scripts/github/dependency-guard.mjs @@ -437,14 +437,31 @@ async function readBase64FileAtRef(api, { owner, repo, path, ref }) { async function collectDependencyManifestChanges(api, { owner, repo, pullRequest, files }) { const { isDependencyManifest } = loadSecurityReviewPolicy(); const changes = []; + let mergeBaseSha; for (const file of files) { const basePath = file.previous_filename ?? file.filename; const headPath = file.filename; if (!isDependencyManifest(basePath) && !isDependencyManifest(headPath)) { continue; } + if (!mergeBaseSha) { + // Match the PR diff: unrelated dependency updates on the target branch + // must neither invent nor hide manifest changes introduced by this PR. + // Page two omits file patches; only the comparison metadata is needed. + const baseSha = pullRequest.base?.sha; + const comparison = await api.request( + `/repos/${owner}/${repo}/compare/${baseSha}...${pullRequest.head?.sha}?per_page=1&page=2`, + ); + if ( + comparison?.base_commit?.sha !== baseSha || + !/^[a-f0-9]{40}$/u.test(comparison?.merge_base_commit?.sha ?? "") + ) { + throw new GitHubDiffDataError("GitHub returned an invalid dependency manifest merge base."); + } + mergeBaseSha = comparison.merge_base_commit.sha; + } const baseManifest = isDependencyManifest(basePath) - ? await readJsonFileAtRef(api, { owner, repo, path: basePath, ref: pullRequest.base?.sha }) + ? await readJsonFileAtRef(api, { owner, repo, path: basePath, ref: mergeBaseSha }) : null; const headManifest = isDependencyManifest(headPath) ? await readJsonFileAtRef(api, { owner, repo, path: headPath, ref: pullRequest.head?.sha }) diff --git a/test/fixtures/github-guard-fetch.mjs b/test/fixtures/github-guard-fetch.mjs index 989e199c2c7e..aa80b7d2201c 100644 --- a/test/fixtures/github-guard-fetch.mjs +++ b/test/fixtures/github-guard-fetch.mjs @@ -22,7 +22,10 @@ globalThis.fetch = async (url, options = {}) => { } }; const key = `${method} ${parsed.pathname}`; - const route = fixture.routes[key]; + const queryKey = `${key}${parsed.search}`; + const route = Object.hasOwn(fixture.routes, queryKey) + ? fixture.routes[queryKey] + : fixture.routes[key]; if (route === undefined) { if (method !== "GET" && /\/(?:statuses\/|issues\/)/u.test(parsed.pathname)) { recordStatus(); diff --git a/test/scripts/dependency-guard-script.test.ts b/test/scripts/dependency-guard-script.test.ts index d7717b0839ae..85d6f76d81b4 100644 --- a/test/scripts/dependency-guard-script.test.ts +++ b/test/scripts/dependency-guard-script.test.ts @@ -27,6 +27,8 @@ import { useAutoCleanupTempDirTracker } from "../helpers/temp-dir.js"; const headSha = "a".repeat(40); const staleSha = "b".repeat(40); const rolloutSha = "c".repeat(40); +const mergeBaseSha = "d".repeat(40); +const comparisonPath = `/repos/openclaw/openclaw/compare/${staleSha}...${headSha}`; const { isDependencyFile, isDependencyManifest, isPackageLockfile } = loadSecurityReviewPolicy(); const tempDirs = useAutoCleanupTempDirTracker(afterEach); @@ -87,6 +89,10 @@ function runDependencyGuard( base: { ref: "main", repo: { full_name: "openclaw/openclaw" } }, }, [`GET ${pullPath}/files`]: [{ filename: "pnpm-workspace.yaml" }], + [`GET ${comparisonPath}`]: { + base_commit: { sha: staleSha }, + merge_base_commit: { sha: staleSha }, + }, [`GET ${issuePath}/comments`]: [], [`GET ${issuePath}/labels`]: [], "GET /repos/openclaw/openclaw/collaborators/contributor/permission": { role_name: "write" }, @@ -306,6 +312,74 @@ describe("dependency guard script", () => { expect(result.statuses.at(-1)?.body?.state).toBe("success"); }); + it.each([ + { role: "write", headVersion: "1", expected: "success", notice: false }, + { role: "admin", headVersion: "1", expected: "success", notice: false }, + { role: "write", headVersion: "2", expected: "failure", notice: true }, + ])( + "evaluates PR manifest changes from the merge base ($role author, version $headVersion)", + ({ role, headVersion, expected, notice }) => { + const content = (version: string, test: string) => ({ + type: "file", + encoding: "base64", + content: Buffer.from( + JSON.stringify({ devDependencies: { example: version }, scripts: { test } }), + ).toString("base64"), + }); + const manifestPath = "/repos/openclaw/openclaw/contents/package.json"; + const result = runDependencyGuard({ + [`GET ${pullPath}/files`]: [{ filename: "package.json" }], + [`GET ${comparisonPath}`]: { + base_commit: { sha: staleSha }, + merge_base_commit: { sha: mergeBaseSha }, + }, + [`GET ${manifestPath}?ref=${mergeBaseSha}`]: content("1", "old"), + [`GET ${manifestPath}?ref=${staleSha}`]: content("2", "old"), + [`GET ${manifestPath}?ref=${headSha}`]: content(headVersion, "new"), + [`GET /repos/openclaw/openclaw/dependency-graph/compare/${staleSha}...${headSha}`]: [], + "GET /repos/openclaw/openclaw/collaborators/contributor/permission": { role_name: role }, + }); + expect(result.status, result.stderr).toBe(0); + expect(result.statuses.at(-1)?.body?.state).toBe(expected); + expect(result.calls.some((call) => call.body?.body)).toBe(notice); + if (notice) { + expect(result.stdout).toContain("/allow-dependencies-change"); + } + }, + ); + + it.each(["added", "removed"])("still detects a manifest that is %s", (status) => { + const manifestPath = "/repos/openclaw/openclaw/contents/package.json"; + const content = { + type: "file", + encoding: "base64", + content: Buffer.from(JSON.stringify({ dependencies: { example: "1" } })).toString("base64"), + }; + const result = runDependencyGuard({ + [`GET ${pullPath}/files`]: [{ filename: "package.json", status }], + [`GET ${manifestPath}?ref=${staleSha}`]: status === "added" ? { httpError: 404 } : content, + [`GET ${manifestPath}?ref=${headSha}`]: status === "removed" ? { httpError: 404 } : content, + [`GET /repos/openclaw/openclaw/dependency-graph/compare/${staleSha}...${headSha}`]: [], + }); + expect(result.status, result.stderr).toBe(0); + expect(result.statuses.at(-1)?.body?.state).toBe("failure"); + expect(result.stdout).toContain("/allow-dependencies-change"); + }); + + it.each([ + { base_commit: { sha: headSha }, merge_base_commit: { sha: mergeBaseSha } }, + { base_commit: { sha: staleSha }, merge_base_commit: { sha: "invalid" } }, + ])("fails closed when the manifest merge base is invalid: %j", (comparison) => { + const result = runDependencyGuard({ + [`GET ${pullPath}/files`]: [{ filename: "package.json" }], + [`GET ${comparisonPath}`]: comparison, + }); + expect(result.status).toBe(1); + expect(result.stderr).toContain("merge base"); + expect(result.statuses.map((call) => call.body?.state)).toEqual(["failure"]); + expect(result.calls.some((call) => call.path === "/graphql")).toBe(false); + }); + it.each([ { lateApproval: false, writeError: false, headChanged: false }, { lateApproval: true, writeError: false, headChanged: false },