From ca15ce3162e89f7d52a2022b2870e69b68e4d996 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Wed, 29 Apr 2026 08:33:09 +0100 Subject: [PATCH] fix(router): use deterministic security gates in finalizers --- docs/OPERATIONS.md | 4 ++-- scripts/apply-result.mjs | 6 ++++-- scripts/finalize-open-prs.mjs | 14 ++++++++++++-- scripts/post-flight.mjs | 18 +++++++++++++++--- 4 files changed, 33 insertions(+), 9 deletions(-) diff --git a/docs/OPERATIONS.md b/docs/OPERATIONS.md index 14c1e81..7c9fa93 100644 --- a/docs/OPERATIONS.md +++ b/docs/OPERATIONS.md @@ -104,7 +104,7 @@ no-PR outcome and the audit file records the skip. ## Security Boundary -Security-sensitive work is centrally managed outside ProjectClownfish. The importer skips those clusters by default, the job schema rejects `security_sensitive: true`, the planner marks hydrated security-sensitive items only from explicit security labels or structured ClawSweeper security markers, `review-results` fails mutating recommendations against those items, and `apply-result` blocks live targets with security-sensitive labels/title/body. +Security-sensitive work is centrally managed outside ProjectClownfish. The importer skips those clusters by default, the job schema rejects `security_sensitive: true`, the planner marks hydrated security-sensitive items only from explicit security labels or structured ClawSweeper security markers, `review-results` fails mutating recommendations against those items, and live merge/close finalizers re-check those deterministic signals before mutating. Use the central OpenClaw security path for: @@ -113,7 +113,7 @@ Use the central OpenClaw security path for: - SSRF, XSS, CSRF, RCE, auth-token leakage, or similar security-class bugs. This boundary is intentionally conservative. If a cluster is borderline, do not stage it here. -For adopted automerge jobs, do not classify security from review prose. ClawSweeper must emit a deterministic marker such as `` when the automerge loop should route the PR to central security handling. +For adopted automerge jobs, do not classify security from review prose at planning, repair, merge, or closeout time. ClawSweeper must emit a deterministic marker such as `` when the automerge loop should route the PR to central security handling. ## Auto-Closure diff --git a/scripts/apply-result.mjs b/scripts/apply-result.mjs index f479951..9d6dad6 100644 --- a/scripts/apply-result.mjs +++ b/scripts/apply-result.mjs @@ -3,7 +3,7 @@ import fs from "node:fs"; import path from "node:path"; import { execFileSync } from "node:child_process"; import { createHash } from "node:crypto"; -import { assertAllowedOwner, hasSecuritySignalText, parseArgs, parseJob, repoRoot, validateJob } from "./lib.mjs"; +import { assertAllowedOwner, hasDeterministicSecuritySignal, parseArgs, parseJob, repoRoot, validateJob } from "./lib.mjs"; import { defaultCloseComment, externalMessageProvenance } from "./external-messages.mjs"; const MAINTAINER_AUTHOR_ASSOCIATIONS = new Set(["OWNER", "MEMBER", "COLLABORATOR"]); @@ -774,7 +774,9 @@ function validateLowSignalLiveState(repo, target, live, kind) { } function hasSecuritySignal(issue) { - return hasSecuritySignalText(issue.title, issue.body, issue.labels ?? []); + if (hasDeterministicSecuritySignal({ labels: issue.labels ?? [] })) return true; + const comments = ghPaged(`repos/${result.repo}/issues/${issue.number}/comments?per_page=100`).map((comment) => comment.body ?? ""); + return hasDeterministicSecuritySignal({ comments }); } function fetchIssue(repo, number) { diff --git a/scripts/finalize-open-prs.mjs b/scripts/finalize-open-prs.mjs index 449f108..1134ea8 100644 --- a/scripts/finalize-open-prs.mjs +++ b/scripts/finalize-open-prs.mjs @@ -5,7 +5,7 @@ import { execFileSync } from "node:child_process"; import { assertLiveWorkerCapacity, currentProjectRepo, - hasSecuritySignalText, + hasDeterministicSecuritySignal, parseArgs, parseJob, readMaxLiveWorkers, @@ -170,7 +170,7 @@ function classifyPullRequest(pull, publishedRecords) { if (pull.isDraft) blockers.push("draft"); if (String(pull.baseRefName ?? "") !== "main") blockers.push(`base is ${pull.baseRefName || "unknown"}`); - if (hasSecuritySignalText(pull.title, pull.body, pull.labels ?? [])) blockers.push("security_hold"); + if (hasDeterministicPullSecuritySignal(pull)) blockers.push("security_hold"); if (isSecurityRoutedAction(latestApplyAction)) blockers.push("security_route"); if (pull.mergeable === "UNKNOWN") { blockers.push("mergeability_unknown"); @@ -298,6 +298,16 @@ function summarizeChecks(checks) { }; } +function hasDeterministicPullSecuritySignal(pull) { + return hasDeterministicSecuritySignal({ + labels: pull.labels ?? [], + comments: [ + (pull.comments ?? []).map((comment) => comment.body), + (pull.reviews ?? []).map((review) => review.body), + ], + }); +} + function displayCheckName(check) { const workflow = String(check.workflowName ?? ""); const name = String(check.name ?? check.context ?? "unknown check"); diff --git a/scripts/post-flight.mjs b/scripts/post-flight.mjs index cd7ee83..52d786d 100644 --- a/scripts/post-flight.mjs +++ b/scripts/post-flight.mjs @@ -2,7 +2,7 @@ import fs from "node:fs"; import path from "node:path"; import { execFileSync } from "node:child_process"; -import { assertAllowedOwner, hasSecuritySignalText, parseArgs, parseJob, repoRoot, validateJob } from "./lib.mjs"; +import { assertAllowedOwner, hasDeterministicSecuritySignal, parseArgs, parseJob, repoRoot, validateJob } from "./lib.mjs"; import { externalMessageProvenance, postMergeCloseoutComment } from "./external-messages.mjs"; const PASSING_CHECK_CONCLUSIONS = new Set(["SUCCESS", "SKIPPED", "NEUTRAL"]); @@ -242,7 +242,7 @@ function finalizePostMergeCloseout({ action, actionName, target, fixRef, fixUrl, merge_commit_sha: finalized.merge_commit_sha ?? null, }; } - if (hasSecuritySignalText(live.title, live.body, live.labels ?? [])) { + if (hasLiveSecuritySignal(target, live.labels ?? [])) { return { ...base, status: "blocked", reason: "security-sensitive target requires central security triage" }; } if (dryRun) { @@ -300,11 +300,23 @@ function ensureLabel(repo, name, color, description) { } } +function hasLiveSecuritySignal(number, labels) { + if (hasDeterministicSecuritySignal({ labels })) return true; + const bodies = ghWithRetry([ + "api", + `repos/${result.repo}/issues/${number}/comments?per_page=100`, + "--paginate", + "--jq", + ".[].body", + ]); + return hasDeterministicSecuritySignal({ comments: [bodies] }); +} + function validateMergeableFixPr({ pull, view, preflight }) { if (pull.state !== "open") return `pull request is ${pull.state}`; if (pull.draft || view.isDraft) return "pull request is draft"; if (String(view.baseRefName ?? pull.base?.ref ?? "") !== "main") return "pull request base is not main"; - if (hasSecuritySignalText(pull.title, pull.body, pull.labels ?? [])) { + if (hasLiveSecuritySignal(pull.number, pull.labels ?? [])) { return "security-sensitive PR requires central security triage"; } if (view.mergeable !== "MERGEABLE") return `mergeable state is ${view.mergeable || "unknown"}`;