diff --git a/scripts/github/dependency-guard.mjs b/scripts/github/dependency-guard.mjs index d50e5e06a3d9..927972401f31 100644 --- a/scripts/github/dependency-guard.mjs +++ b/scripts/github/dependency-guard.mjs @@ -157,24 +157,44 @@ export function isDependencyGuardMarkerComment(comment, marker, trustedAuthors) return Boolean(login && trustedAuthors.has(login) && comment.body?.includes(marker)); } -function renderApprovedDependencyComment(approval) { +function renderDependencyChangeLines({ + lockfileChanges, + dependencyFiles = [], + dependencyManifestChanges, +}) { + const files = new Set([...lockfileChanges, ...dependencyFiles]); + for (const change of dependencyManifestChanges) { + if (change.previousPath) { + files.add(change.previousPath); + } + files.add(change.path); + } + return [...files].map((path) => `- ${markdownCode(path)}`); +} + +function renderApprovedDependencyComment(approval, changes) { return [ dependencyGraphGuardMarker, "", approval.kind === "author" - ? "### Dependency graph changes noted" - : "### Dependency graph changes approved", + ? "### ⚠️ Dependency graph changes" + : "### ✅ Dependency graph changes approved", "", approval.kind === "author" - ? "This PR changes dependency resolution. The guard is informational because the PR author has repository Maintain or Admin access." - : "A maintainer approved this revision with an explicit dependency approval comment. SecOps approval is not required.", + ? "This PR makes dependency graph changes. This comment is informational because the PR author has repository Maintain or Admin access." + : "A maintainer approved this revision with an explicit dependency approval comment.", "", `- Current SHA: ${markdownCode(approval.sha)}`, `- Maintainer: @${sanitizeGuardDisplayValue(approval.login)}`, `- Repository role: ${markdownCode(approval.role)}`, ...(approval.kind === "comment" ? [`- Approval comment: ${approval.url}`] : []), "", - "Review resolved package changes and dependency policy before merging. A later push requires a fresh approval comment for an external contributor's PR.", + ...(approval.kind === "author" + ? ["These dependency graph changes were made:", ...renderDependencyChangeLines(changes), ""] + : []), + approval.kind === "author" + ? "Carefully review these changes before merging." + : "A later push requires a fresh approval comment for an external contributor's PR.", ].join("\n"); } @@ -254,13 +274,6 @@ export function renderBlockedDependencyComment({ }) { const safeBranch = sanitizeGuardDisplayValue(baseBranch ?? "main"); const baseRef = shellQuote(`origin/${safeBranch}`); - const reasons = []; - for (const path of new Set([...lockfileChanges, ...dependencyFiles])) { - reasons.push(`- ${markdownCode(path)} changed.`); - } - for (const change of dependencyManifestChanges) { - reasons.push(renderManifestChangeLine(change)); - } const autoscrubLines = renderAutoscrubStatusLines(autoscrubStatus); const removalSteps = lockfileChanges.length > 0 @@ -279,12 +292,14 @@ export function renderBlockedDependencyComment({ return [ dependencyGraphGuardMarker, "", - "### Maintainer dependency review required", + "### ⚠️ Maintainer dependency review required", "", - "This external contributor PR changes dependency resolution. A maintainer with repository Maintain or Admin access must review the resolved packages and dependency policy before merging.", + "This external contributor PR changes the dependency graph. A maintainer must review these changes before merging.", "", - "Detected dependency graph changes:", - ...reasons, + `Current SHA: ${markdownCode(headSha ?? "")}`, + "", + "These dependency graph changes were made:", + ...renderDependencyChangeLines({ lockfileChanges, dependencyFiles, dependencyManifestChanges }), ...autoscrubLines, ...removalSteps, "", @@ -294,9 +309,7 @@ export function renderBlockedDependencyComment({ dependencyApprovalCommand, "```", "", - "Post the comment after this guard notice identifies the current head SHA below. Do not edit an earlier comment. A normal GitHub Approve review does not satisfy this check. SecOps approval is not required; this check updates automatically.", - "", - `Current head SHA: ${markdownCode(headSha ?? "")}. A later push requires a fresh approval comment.`, + "A later push requires a fresh approval comment.", ].join("\n"); } @@ -314,7 +327,10 @@ function renderAutoscrubStatusLines(status) { return [ "", "Auto-scrub was not attempted because this PR changes package manifest dependency graph fields:", - ...status.changes.map(renderManifestChangeLine), + ...renderDependencyChangeLines({ + lockfileChanges: [], + dependencyManifestChanges: status.changes, + }), "", "Dependency graph changes require maintainer review. Please remove lockfile changes manually if they are not needed.", ]; @@ -337,15 +353,6 @@ function renderAutoscrubStatusLines(status) { return []; } -function renderManifestChangeLine(change) { - const location = change.previousPath - ? `${markdownCode(change.previousPath)} moved to ${markdownCode(change.path)}` - : markdownCode(change.path); - const fields = - change.fields.length > 0 ? ` changed ${change.fields.map(markdownCode).join(", ")}` : ""; - return `- ${location}${fields}.`; -} - export function githubApi(token, options = {}) { const api = createGitHubApi(token, { ...options, userAgent: "openclaw-dependency-guard" }); return { @@ -707,7 +714,14 @@ export async function reviewDependencyChanges( dependencyGraphChanges, headSha: pullRequest.head.sha, }) - : withApprovalRequest(guard, renderApprovedDependencyComment(guard.approval)); + : withApprovalRequest( + guard, + renderApprovedDependencyComment(guard.approval, { + lockfileChanges, + dependencyFiles, + dependencyManifestChanges, + }), + ); await upsertComment(existingGuardComment, body); await writeSummary(body); return; diff --git a/scripts/github/security-sensitive-guard.mjs b/scripts/github/security-sensitive-guard.mjs index 5791992050d3..ddb8245be92c 100644 --- a/scripts/github/security-sensitive-guard.mjs +++ b/scripts/github/security-sensitive-guard.mjs @@ -14,25 +14,56 @@ function code(value) { } function renderComment({ changes, pullRequest, approval }) { + if (changes.length > 0 && approval?.kind === "comment") { + return [ + marker, + "", + "### ✅ Maintainer security changes approved", + "", + "A maintainer approved this revision with an explicit security approval comment.", + "", + `- Current SHA: ${code(approval.sha)}`, + `- Maintainer: @${sanitizeGuardDisplayValue(approval.login)}`, + `- Repository role: ${code(approval.role)}`, + `- Approval comment: ${approval.url}`, + "", + "A later push requires a fresh approval comment for an external contributor's PR.", + ].join("\n"); + } const heading = changes.length === 0 ? "Security-sensitive guard cleared" : approval?.kind === "author" - ? "Security-sensitive changes noted" - : approval - ? "Maintainer security review complete" - : "Maintainer security review required"; - const lines = [ - marker, - "", - `### ${heading}`, - "", - `Current revision: ${code(pullRequest.head.sha)}`, - ]; + ? "⚠️ Security sensitive changes" + : "⚠️ Maintainer security review required"; + const lines = [marker, "", `### ${heading}`, ""]; + if (changes.length > 0 && approval?.kind === "author") { + lines.push( + "This PR makes security sensitive changes. This comment is informational because the PR author has repository Maintain or Admin access.", + "", + `- Current SHA: ${code(pullRequest.head.sha)}`, + `- Maintainer: @${sanitizeGuardDisplayValue(approval.login)}`, + `- Repository role: ${code(approval.role)}`, + ); + } else if (changes.length > 0 && !approval) { + lines.push( + "This external contributor PR changes sensitive security components. A maintainer must review these changes before merging.", + "", + `Current SHA: ${code(pullRequest.head.sha)}`, + ); + } else { + lines.push(`Current revision: ${code(pullRequest.head.sha)}`); + } if (changes.length === 0) { lines.push("", "This PR no longer changes files in the maintainer security-review tier."); } else { - lines.push("", "Review these security responsibilities:", ""); + lines.push( + "", + approval?.kind === "author" + ? "These security sensitive changes were made:" + : "These sensitive security changes were made:", + "", + ); for (const change of changes.slice(0, 25)) { lines.push(`- ${code(change.path)}: ${change.reason}`); } @@ -41,28 +72,25 @@ function renderComment({ changes, pullRequest, approval }) { } lines.push(""); if (approval?.kind === "author") { - lines.push( - `Informational: author @${approval.login} has repository ${code(approval.role)} access.`, - ); - } else if (approval) { - lines.push( - `@${approval.login} approved this revision with ${code("/allow-security-sensitive-change")} and repository ${code(approval.role)} access.`, - ); + lines.push("Carefully review these changes before merging."); } else { lines.push( - "A GitHub user account with repository `maintain` or `admin` access must post a new comment containing `/allow-security-sensitive-change` after this notice names the current revision. SecOps approval is not required for this tier.", - "Use only the command, or include `/allow-dependencies-change` on a separate line if both guards need approval. A normal GitHub Approve review does not satisfy this check.", + "After reviewing the changes, post a new PR comment containing only approval commands, each on its own line:", + "", + "```text", + "/allow-security-sensitive-change", + "```", + "", + "A later push requires a fresh approval comment.", ); } + } + if (changes.length === 0) { lines.push( "", - "A later push requires a new approval comment after the notice updates. Editing an old comment does not renew approval; deleting the command removes its approval.", + "Separate CODEOWNERS requirements still apply to security policy and enforcement files.", ); } - lines.push( - "", - "Separate CODEOWNERS requirements still apply to security policy and enforcement files.", - ); return lines.join("\n"); } diff --git a/test/scripts/dependency-guard-script.test.ts b/test/scripts/dependency-guard-script.test.ts index 95e5d8bf184d..dc89351daf28 100644 --- a/test/scripts/dependency-guard-script.test.ts +++ b/test/scripts/dependency-guard-script.test.ts @@ -143,6 +143,7 @@ describe("dependency guard script", () => { expect(result.status, result.stderr).toBe(0); expect(result.statuses.map((call) => call.body?.state)).toEqual(["failure", "success"]); expect(result.stdout).toContain("informational"); + expect(result.stdout).toContain("- `pnpm-workspace.yaml`\n"); }); it("does not transfer command approval to a duplicate PR with the same head", () => { @@ -263,7 +264,7 @@ describe("dependency guard script", () => { expect(result.status).toBe(1); expect(result.statuses.at(-1)?.body?.state).toBe("failure"); expect(result.stdout).toContain( - "`extensions/old/package.json` moved to `extensions/new/package.json`", + "- `extensions/old/package.json`\n- `extensions/new/package.json`\n", ); }); @@ -503,17 +504,14 @@ describe("dependency guard script", () => { expect(body).toContain(""); expect(body).toContain("Maintainer dependency review required"); - expect(body).toContain("`pnpm-lock.yaml` changed."); - expect(body).toContain("`tools/nested/pnpm-lock.yaml` changed."); - expect(body).toContain("`package.json` changed `dependencies`."); + expect(body).toContain("- `pnpm-lock.yaml`\n"); + expect(body).toContain("- `tools/nested/pnpm-lock.yaml`\n"); + expect(body).toContain("- `package.json`\n"); expect(body).toContain( "git checkout 'origin/main' -- 'pnpm-lock.yaml' 'tools/nested/pnpm-lock.yaml'", ); expect(body).toContain("```text\n/allow-dependencies-change\n```"); - expect(body).toContain("Post the comment after this guard notice identifies the current head"); - expect(body).toContain("A normal GitHub Approve review does not satisfy this check"); - expect(body).toContain("SecOps approval is not required"); - expect(body).toContain(`Current head SHA: \`${headSha}\``); + expect(body).toContain(`Current SHA: \`${headSha}\``); expect(body).toContain("A later push requires a fresh approval comment."); }); @@ -674,7 +672,7 @@ describe("dependency guard script", () => { "only push deterministic cleanup commits to PR branches that maintainers can modify", ); expect(unsafeBody).toContain("changes package manifest dependency graph fields"); - expect(unsafeBody).toContain("`package.json` changed `dependencies`"); + expect(unsafeBody).toContain("- `package.json`\n"); expect(unsafeBody).toContain("Dependency graph changes require maintainer review"); expect(mixedBody).toContain("also changes dependency-related files"); expect(mixedBody).toContain("`patches/example.patch`"); diff --git a/test/scripts/security-sensitive-guard-script.test.ts b/test/scripts/security-sensitive-guard-script.test.ts index af8759aa0661..0c852cd97561 100644 --- a/test/scripts/security-sensitive-guard-script.test.ts +++ b/test/scripts/security-sensitive-guard-script.test.ts @@ -38,6 +38,7 @@ const notice = { const approval = { id: 11, user: approver, + html_url: "https://github.com/openclaw/openclaw/pull/7#issuecomment-11", body: "/allow-security-sensitive-change", created_at: "2026-01-01T00:01:00Z", updated_at: "2026-01-01T00:01:00Z", @@ -168,7 +169,7 @@ describe("security-sensitive guard entry point", () => { const result = runGuard({ authorRole }); expect(result.status, result.stderr).toBe(0); expect(result.statuses).toEqual(["failure", "success"]); - expect(result.comment).toContain("Informational"); + expect(result.comment).toContain("informational"); }); it.each([ @@ -330,7 +331,8 @@ describe("security-sensitive guard entry point", () => { const result = runGuard({ comments: [notice, approval], approverRole, event: commentEvent }); expect(result.status, result.stderr).toBe(0); expect(result.statuses).toEqual(["failure", "success"]); - expect(result.comment).toContain("@maintainer approved"); + expect(result.comment).toContain("- Maintainer: @maintainer"); + expect(result.comment).toContain(`- Approval comment: ${approval.html_url}`); expect( result.requests .filter((request) => request.path.includes("/statuses/"))