feat: add ClawSweeper review status sections
This commit is contained in:
parent
8e73bcd135
commit
ed5a07b3bb
4
.github/workflows/sweep.yml
vendored
4
.github/workflows/sweep.yml
vendored
@ -640,7 +640,7 @@ jobs:
|
||||
fetch-depth: 0
|
||||
persist-credentials: false
|
||||
|
||||
- name: Create target read token
|
||||
- name: Create target review token
|
||||
id: target-read-token
|
||||
uses: actions/create-github-app-token@1b10c78c7865c340bc4f6099eb2f838309f1e8c3 # v3.1.1
|
||||
with:
|
||||
@ -649,7 +649,7 @@ jobs:
|
||||
owner: ${{ needs.plan.outputs.target_repo_owner }}
|
||||
repositories: ${{ needs.plan.outputs.target_repo_name }}
|
||||
permission-contents: read
|
||||
permission-issues: read
|
||||
permission-issues: write
|
||||
permission-pull-requests: read
|
||||
|
||||
- uses: ./clawsweeper/.github/actions/setup-pnpm
|
||||
|
||||
11
README.md
11
README.md
@ -26,9 +26,14 @@ It has two independent lanes:
|
||||
maintainer-facing comment, runtime metadata, and GitHub snapshot hash.
|
||||
- **Durable review comments:** ClawSweeper syncs one marker-backed public review
|
||||
comment per item and edits it in place instead of posting repeated comments.
|
||||
Pull request comments include hidden verdict markers, and actionable PR
|
||||
follow-up includes a hidden `clawsweeper-action:fix-required` marker for the
|
||||
trusted ClawSweeper repair loop. See
|
||||
When a review starts and no ClawSweeper comment exists yet, it posts a short
|
||||
crustacean-friendly status placeholder first, then replaces that same comment
|
||||
with the completed review. Completed comments include a dedicated security
|
||||
review section for supply-chain, permission, secret-handling, and code
|
||||
execution concerns. Pull request comments include hidden verdict markers, and
|
||||
actionable PR follow-up includes a hidden
|
||||
`clawsweeper-action:fix-required` marker for the trusted ClawSweeper repair
|
||||
loop. See
|
||||
[`docs/pr-review-comments.md`](docs/pr-review-comments.md).
|
||||
- **Guarded apply:** apply mode re-fetches live GitHub state, checks labels,
|
||||
maintainer authorship, paired issue/PR state, snapshot drift, and repository
|
||||
|
||||
@ -27,6 +27,11 @@ Each synced comment includes the durable identity marker:
|
||||
ClawSweeper edits that comment in place instead of posting repeated comments.
|
||||
Report front matter stores the synced comment id, URL, hash, and sync time.
|
||||
|
||||
When review starts and no ClawSweeper-owned comment exists yet, the review
|
||||
shard posts a short status placeholder with the same durable identity marker.
|
||||
The placeholder is intentionally light and crustacean-friendly, then the final
|
||||
review sync edits that exact comment in place.
|
||||
|
||||
For a PR that needs work, the visible comment starts with:
|
||||
|
||||
```text
|
||||
@ -40,6 +45,9 @@ report has:
|
||||
merge verdict or maintainer follow-up summary
|
||||
- `Required change before merge:` or `Maintainer follow-up before merge:` from
|
||||
the work-candidate reason or next action
|
||||
- `Security review:` from the typed `securityReview` field, so supply-chain,
|
||||
permission, secret-handling, and code-execution concerns have a dedicated
|
||||
visible pass
|
||||
- `Review findings:` for Codex `/review`-style findings, using typed priority,
|
||||
confidence, file, and line-range data from the report
|
||||
- `Best possible solution:` only when it adds a distinct end-state that is not
|
||||
|
||||
@ -56,7 +56,7 @@ ownership beyond this PR. If the PR author is only the proposer/reporter, you
|
||||
may mention that in evidence or summary when useful, but do not make them a
|
||||
likely owner.
|
||||
|
||||
For PRs, include a dedicated security review pass in addition to the functional review. Inspect whether the diff could introduce a security or supply-chain regression, especially when it touches CI workflows, GitHub Action refs, dependency sources, lockfiles, install/build/release scripts, package publishing metadata, secrets handling, permissions, downloaded artifacts, generated/vendor/minified files, or other code execution paths. Check whether those changes are consistent with the PR title, body, discussion, and stated purpose before deciding. Be cautious when a small or unrelated functional change also introduces new third-party code execution, broadens secret or permission access, changes package resolution, adds lifecycle hooks, downloads and executes artifacts, or mixes infrastructure changes into otherwise cosmetic work. Do not infer malicious intent without concrete evidence, but note unexplained security-sensitive changes in `risks` and `evidence` with the observable risk, relevant file/path, and why it matters.
|
||||
For PRs, include a dedicated security review pass in addition to the functional review. Inspect whether the diff could introduce a security or supply-chain regression, especially when it touches CI workflows, GitHub Action refs, dependency sources, lockfiles, install/build/release scripts, package publishing metadata, secrets handling, permissions, downloaded artifacts, generated/vendor/minified files, or other code execution paths. Check whether those changes are consistent with the PR title, body, discussion, and stated purpose before deciding. Be cautious when a small or unrelated functional change also introduces new third-party code execution, broadens secret or permission access, changes package resolution, adds lifecycle hooks, downloads and executes artifacts, or mixes infrastructure changes into otherwise cosmetic work. Do not infer malicious intent without concrete evidence. Always summarize this pass in `securityReview`; set `status: "cleared"` when the diff has no concrete security or supply-chain concern, `status: "needs_attention"` when there is a concrete concern, and `status: "not_applicable"` for non-PR items without a security-sensitive report. Put concrete security concerns in `securityReview.concerns` with file/line when possible, and also include blocking concerns in `risks` and `evidence` when they affect the merge/close decision.
|
||||
|
||||
For PRs, also emit Codex `/review`-style findings in `reviewFindings`.
|
||||
Review the diff as another engineer's proposed patch and list every discrete,
|
||||
@ -231,6 +231,13 @@ For PRs, use these fields for the concise reviewer feedback that should appear
|
||||
near the top of the public ClawSweeper comment; the rest of the evidence can
|
||||
stay in the collapsed details.
|
||||
|
||||
Always fill `securityReview`. This is the dedicated public security section,
|
||||
separate from functional findings. Use `cleared` with a concise summary when no
|
||||
security or supply-chain issue was found, `needs_attention` with one or more
|
||||
typed concerns when the patch or discussion raises a concrete security issue,
|
||||
and `not_applicable` for ordinary non-PR issue triage where no patch security
|
||||
review applies.
|
||||
|
||||
Always fill the work-lane fields too. For non-candidates, use
|
||||
`workCandidate: "none"`, low confidence/priority, an empty `workPrompt`, and
|
||||
empty arrays. For manual-review items, use `workCandidate: "manual_review"` and
|
||||
|
||||
@ -13,6 +13,7 @@
|
||||
"risks",
|
||||
"bestSolution",
|
||||
"reviewFindings",
|
||||
"securityReview",
|
||||
"overallCorrectness",
|
||||
"overallConfidenceScore",
|
||||
"fixedRelease",
|
||||
@ -185,6 +186,59 @@
|
||||
}
|
||||
}
|
||||
},
|
||||
"securityReview": {
|
||||
"type": "object",
|
||||
"additionalProperties": false,
|
||||
"description": "Dedicated security and supply-chain review for the item. For PRs, summarize whether the diff introduces security-sensitive concerns. For issues, use not_applicable unless the issue itself is security-sensitive.",
|
||||
"required": ["status", "summary", "concerns"],
|
||||
"properties": {
|
||||
"status": {
|
||||
"type": "string",
|
||||
"enum": ["cleared", "needs_attention", "not_applicable"]
|
||||
},
|
||||
"summary": {
|
||||
"type": "string",
|
||||
"description": "One concise sentence explaining the security review result."
|
||||
},
|
||||
"concerns": {
|
||||
"type": "array",
|
||||
"description": "Concrete security or supply-chain concerns found during the dedicated pass. Empty when cleared or not applicable.",
|
||||
"items": {
|
||||
"type": "object",
|
||||
"additionalProperties": false,
|
||||
"required": ["title", "body", "severity", "confidenceScore", "file", "line"],
|
||||
"properties": {
|
||||
"title": {
|
||||
"type": "string",
|
||||
"description": "Short concern title, at most 80 characters."
|
||||
},
|
||||
"body": {
|
||||
"type": "string",
|
||||
"description": "Brief explanation of the observable security risk and why it matters."
|
||||
},
|
||||
"severity": {
|
||||
"type": "string",
|
||||
"enum": ["high", "medium", "low"]
|
||||
},
|
||||
"confidenceScore": {
|
||||
"type": "number",
|
||||
"minimum": 0,
|
||||
"maximum": 1
|
||||
},
|
||||
"file": {
|
||||
"type": ["string", "null"],
|
||||
"description": "Repository-relative file path when tied to a file; null for issue-wide or discussion-only concerns."
|
||||
},
|
||||
"line": {
|
||||
"type": ["integer", "null"],
|
||||
"minimum": 1,
|
||||
"description": "Relevant line when tied to a specific file; null when not applicable."
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
},
|
||||
"overallCorrectness": {
|
||||
"type": "string",
|
||||
"enum": ["patch is correct", "patch is incorrect", "not a patch"],
|
||||
|
||||
@ -64,6 +64,8 @@ type ApplyKind = ItemKind | "all";
|
||||
type DecisionKind = "close" | "keep_open";
|
||||
type WorkCandidateKind = "none" | "manual_review" | "queue_fix_pr";
|
||||
type OverallCorrectness = "patch is correct" | "patch is incorrect" | "not a patch";
|
||||
type SecurityReviewStatus = "cleared" | "needs_attention" | "not_applicable";
|
||||
type SecurityConcernSeverity = "high" | "medium" | "low";
|
||||
type CloseReason =
|
||||
| "implemented_on_main"
|
||||
| "cannot_reproduce"
|
||||
@ -125,6 +127,16 @@ interface Item {
|
||||
activeLockReason?: string | null;
|
||||
}
|
||||
|
||||
export interface ReviewStartStatusCommentOptions {
|
||||
number: number;
|
||||
kind: string;
|
||||
title: string;
|
||||
position?: number;
|
||||
total?: number;
|
||||
shardIndex?: number;
|
||||
shardCount?: number;
|
||||
}
|
||||
|
||||
interface ExistingReview {
|
||||
path: string;
|
||||
markdown: string;
|
||||
@ -176,6 +188,21 @@ interface ReviewFinding {
|
||||
lineEnd: number;
|
||||
}
|
||||
|
||||
interface SecurityConcern {
|
||||
title: string;
|
||||
body: string;
|
||||
severity: SecurityConcernSeverity;
|
||||
confidenceScore: number;
|
||||
file: string | null;
|
||||
line: number | null;
|
||||
}
|
||||
|
||||
interface SecurityReview {
|
||||
status: SecurityReviewStatus;
|
||||
summary: string;
|
||||
concerns: SecurityConcern[];
|
||||
}
|
||||
|
||||
interface Decision {
|
||||
decision: DecisionKind;
|
||||
closeReason: CloseReason;
|
||||
@ -187,6 +214,7 @@ interface Decision {
|
||||
risks: string[];
|
||||
bestSolution: string;
|
||||
reviewFindings: ReviewFinding[];
|
||||
securityReview: SecurityReview;
|
||||
overallCorrectness: OverallCorrectness;
|
||||
overallConfidenceScore: number;
|
||||
fixedRelease?: string | null;
|
||||
@ -498,6 +526,7 @@ const DEFAULT_REASONING_EFFORT = "high";
|
||||
const DEFAULT_SERVICE_TIER = "fast";
|
||||
const REVIEW_POLICY_VERSION = "2026-04-29-policy-v12";
|
||||
const REVIEW_COMMENT_MARKER_PREFIX = "<!-- clawsweeper-review";
|
||||
const REVIEW_START_STATUS_MARKER_PREFIX = "<!-- clawsweeper-review-status";
|
||||
const AUTOMERGE_LABEL = "clawsweeper:automerge";
|
||||
const PROTECTED_LABELS = new Set(["security", "beta-blocker", "release-blocker", "maintainer"]);
|
||||
const ALLOWED_REASONS = new Set<CloseReason>([
|
||||
@ -512,6 +541,12 @@ const ALLOWED_REASONS = new Set<CloseReason>([
|
||||
const ALL_REASONS = new Set<CloseReason>([...ALLOWED_REASONS, "none"]);
|
||||
const DECISIONS = new Set<DecisionKind>(["close", "keep_open"]);
|
||||
const WORK_CANDIDATES = new Set<WorkCandidateKind>(["none", "manual_review", "queue_fix_pr"]);
|
||||
const SECURITY_REVIEW_STATUSES = new Set<SecurityReviewStatus>([
|
||||
"cleared",
|
||||
"needs_attention",
|
||||
"not_applicable",
|
||||
]);
|
||||
const SECURITY_CONCERN_SEVERITIES = new Set<SecurityConcernSeverity>(["high", "medium", "low"]);
|
||||
const OVERALL_CORRECTNESS_VALUES = new Set<OverallCorrectness>([
|
||||
"patch is correct",
|
||||
"patch is incorrect",
|
||||
@ -531,6 +566,7 @@ const DECISION_SCHEMA_KEYS = new Set([
|
||||
"risks",
|
||||
"bestSolution",
|
||||
"reviewFindings",
|
||||
"securityReview",
|
||||
"overallCorrectness",
|
||||
"overallConfidenceScore",
|
||||
"fixedRelease",
|
||||
@ -547,6 +583,15 @@ const DECISION_SCHEMA_KEYS = new Set([
|
||||
"workLikelyFiles",
|
||||
]);
|
||||
const EVIDENCE_SCHEMA_KEYS = new Set(["label", "detail", "file", "line", "command", "sha"]);
|
||||
const SECURITY_REVIEW_SCHEMA_KEYS = new Set(["status", "summary", "concerns"]);
|
||||
const SECURITY_CONCERN_SCHEMA_KEYS = new Set([
|
||||
"title",
|
||||
"body",
|
||||
"severity",
|
||||
"confidenceScore",
|
||||
"file",
|
||||
"line",
|
||||
]);
|
||||
const REVIEW_FINDING_SCHEMA_KEYS = new Set([
|
||||
"title",
|
||||
"body",
|
||||
@ -569,6 +614,7 @@ const REVIEW_SECTIONS = {
|
||||
changeSummary: "What This Changes",
|
||||
bestSolution: "Best Possible Solution",
|
||||
reviewFindings: "Review Findings",
|
||||
securityReview: "Security Review",
|
||||
workCandidate: "Work Candidate",
|
||||
repairWorkPrompt: "Repair Work Prompt",
|
||||
evidence: "Evidence",
|
||||
@ -961,6 +1007,38 @@ function parseReviewFinding(value: unknown, path: string): ReviewFinding {
|
||||
};
|
||||
}
|
||||
|
||||
function parseSecurityConcern(value: unknown, path: string): SecurityConcern {
|
||||
const record = requireRecord(value, path);
|
||||
rejectUnexpectedKeys(record, SECURITY_CONCERN_SCHEMA_KEYS, path);
|
||||
const line = requireNullableInteger(record.line, `${path}.line`);
|
||||
if (line !== null && line <= 0) throw new Error(`${path}.line must be positive`);
|
||||
return {
|
||||
title: requireString(record.title, `${path}.title`),
|
||||
body: requireString(record.body, `${path}.body`),
|
||||
severity: requireEnum(record.severity, SECURITY_CONCERN_SEVERITIES, `${path}.severity`),
|
||||
confidenceScore: requireConfidenceScore(record.confidenceScore, `${path}.confidenceScore`),
|
||||
file: requireNullableString(record.file, `${path}.file`),
|
||||
line,
|
||||
};
|
||||
}
|
||||
|
||||
function parseSecurityReview(value: unknown, path: string): SecurityReview {
|
||||
const record = requireRecord(value, path);
|
||||
rejectUnexpectedKeys(record, SECURITY_REVIEW_SCHEMA_KEYS, path);
|
||||
const concerns = Array.isArray(record.concerns)
|
||||
? record.concerns.map((entry, index) =>
|
||||
parseSecurityConcern(entry, `${path}.concerns[${index}]`),
|
||||
)
|
||||
: (() => {
|
||||
throw new Error(`${path}.concerns must be an array`);
|
||||
})();
|
||||
return {
|
||||
status: requireEnum(record.status, SECURITY_REVIEW_STATUSES, `${path}.status`),
|
||||
summary: requireString(record.summary, `${path}.summary`),
|
||||
concerns,
|
||||
};
|
||||
}
|
||||
|
||||
function requireEnum<T extends string>(value: unknown, allowed: Set<T>, path: string): T {
|
||||
if (typeof value === "string" && allowed.has(value as T)) return value as T;
|
||||
throw new Error(`${path} has invalid value`);
|
||||
@ -1002,6 +1080,7 @@ export function parseDecision(value: unknown): Decision {
|
||||
),
|
||||
bestSolution: requireString(record.bestSolution, "decision.bestSolution"),
|
||||
reviewFindings,
|
||||
securityReview: parseSecurityReview(record.securityReview, "decision.securityReview"),
|
||||
overallCorrectness: requireEnum(
|
||||
record.overallCorrectness,
|
||||
OVERALL_CORRECTNESS_VALUES,
|
||||
@ -2474,6 +2553,11 @@ function codexFailureDecision(status: number | null, stderr: string, stdout = ""
|
||||
risks: ["No close action taken because the review did not complete."],
|
||||
bestSolution: "Retry the Codex review after fixing the execution failure.",
|
||||
reviewFindings: [],
|
||||
securityReview: {
|
||||
status: "not_applicable",
|
||||
summary: "Security review did not run because the Codex review failed before completion.",
|
||||
concerns: [],
|
||||
},
|
||||
overallCorrectness: "not a patch",
|
||||
overallConfidenceScore: 0,
|
||||
fixedRelease: null,
|
||||
@ -2962,6 +3046,30 @@ function reviewFindingDetailedLine(finding: ReviewFinding): string {
|
||||
].join("\n");
|
||||
}
|
||||
|
||||
function securityConcernSummaryLine(concern: SecurityConcern): string {
|
||||
const location = securityConcernLocation(concern);
|
||||
const suffix = location === "not tied to a single file" ? "" : ` — \`${location}\``;
|
||||
return `- [${concern.severity}] ${concern.title.trim()}${suffix}`;
|
||||
}
|
||||
|
||||
function securityConcernDetailedLine(concern: SecurityConcern): string {
|
||||
return [
|
||||
securityConcernSummaryLine(concern),
|
||||
` ${sentence(concern.body)}`,
|
||||
` Confidence: ${confidenceText(concern.confidenceScore)}`,
|
||||
].join("\n");
|
||||
}
|
||||
|
||||
function securityReviewLine(review: SecurityReview): string {
|
||||
const prefix =
|
||||
review.status === "needs_attention"
|
||||
? "Security review needs attention"
|
||||
: review.status === "cleared"
|
||||
? "Security review cleared"
|
||||
: "Security review";
|
||||
return `${prefix}: ${sentence(review.summary)}`;
|
||||
}
|
||||
|
||||
function closeIntro(reason: CloseReason): string {
|
||||
switch (reason) {
|
||||
case "implemented_on_main":
|
||||
@ -3127,6 +3235,58 @@ function reportReviewFindings(markdown: string): ReviewFinding[] {
|
||||
return findings;
|
||||
}
|
||||
|
||||
function defaultSecurityReview(markdown: string): SecurityReview {
|
||||
const type = frontMatterValue(markdown, "type");
|
||||
return {
|
||||
status: type === "pull_request" ? "not_applicable" : "not_applicable",
|
||||
summary:
|
||||
type === "pull_request"
|
||||
? "No dedicated security review was recorded in this older report."
|
||||
: "No patch security review is needed for this non-PR item.",
|
||||
concerns: [],
|
||||
};
|
||||
}
|
||||
|
||||
function reportSecurityReview(markdown: string): SecurityReview {
|
||||
const section = reviewSectionValue(markdown, "securityReview");
|
||||
if (!section.trim()) return defaultSecurityReview(markdown);
|
||||
const status = section.match(/^Status:\s*(cleared|needs_attention|not_applicable)$/m)?.[1] as
|
||||
| SecurityReviewStatus
|
||||
| undefined;
|
||||
const summary = section.match(/^Summary:\s*(.+)$/m)?.[1]?.trim();
|
||||
if (!status || !summary) return defaultSecurityReview(markdown);
|
||||
const concerns: SecurityConcern[] = [];
|
||||
let current: SecurityConcern | null = null;
|
||||
for (const line of section.split("\n")) {
|
||||
const heading = line.match(/^- \*\*\[(high|medium|low)\] (.*?):\*\*(?:\s*`([^`:]+):(\d+)`)?$/);
|
||||
if (heading?.[1] && heading[2]) {
|
||||
if (current) concerns.push(current);
|
||||
current = {
|
||||
title: heading[2],
|
||||
body: "",
|
||||
severity: heading[1] as SecurityConcernSeverity,
|
||||
confidenceScore: 0,
|
||||
file: heading[3] ?? null,
|
||||
line: heading[4] ? Number(heading[4]) : null,
|
||||
};
|
||||
continue;
|
||||
}
|
||||
if (!current) continue;
|
||||
const body = line.match(/^\s+- body: (.*)$/);
|
||||
if (body?.[1]) {
|
||||
current.body = body[1];
|
||||
continue;
|
||||
}
|
||||
const confidence = line.match(/^\s+- confidence: ([0-9.]+)$/);
|
||||
if (confidence?.[1]) {
|
||||
const score = Number(confidence[1]);
|
||||
current.confidenceScore = Number.isFinite(score) ? Math.min(1, Math.max(0, score)) : 0;
|
||||
}
|
||||
}
|
||||
if (current) concerns.push(current);
|
||||
return { status, summary, concerns };
|
||||
}
|
||||
|
||||
function reportDecision(markdown: string, closeReason: CloseReason): Decision {
|
||||
const fixedRelease = frontMatterValue(markdown, "fixed_release");
|
||||
const fixedSha = frontMatterValue(markdown, "fixed_sha");
|
||||
@ -3142,6 +3302,7 @@ function reportDecision(markdown: string, closeReason: CloseReason): Decision {
|
||||
risks: [],
|
||||
bestSolution: reviewSectionValue(markdown, "bestSolution"),
|
||||
reviewFindings: reportReviewFindings(markdown),
|
||||
securityReview: reportSecurityReview(markdown),
|
||||
overallCorrectness: reportOverallCorrectness(markdown),
|
||||
overallConfidenceScore: reportOverallConfidenceScore(markdown),
|
||||
fixedRelease: fixedRelease && fixedRelease !== "unknown" ? fixedRelease : null,
|
||||
@ -3208,6 +3369,7 @@ function renderCloseComment(options: {
|
||||
bestSolution?: string;
|
||||
evidence: Evidence[];
|
||||
likelyOwners?: LikelyOwner[];
|
||||
securityReview?: SecurityReview;
|
||||
reviewLine: string;
|
||||
}): string {
|
||||
const evidence = options.evidence.slice(0, 6).map(closeEvidenceLine);
|
||||
@ -3219,6 +3381,12 @@ function renderCloseComment(options: {
|
||||
if (bestSolutionLine && publicReviewTextDiffers(bestSolutionLine, summaryLine)) {
|
||||
details.push("Best possible solution:", "", bestSolutionLine);
|
||||
}
|
||||
if (options.securityReview) {
|
||||
details.push("", "Security review:", "", securityReviewLine(options.securityReview));
|
||||
if (options.securityReview.concerns.length) {
|
||||
details.push("", ...options.securityReview.concerns.map(securityConcernDetailedLine));
|
||||
}
|
||||
}
|
||||
if (evidence.length) details.push("", "What I checked:", "", ...evidence);
|
||||
if (likelyOwners.length) details.push("", "Likely related people:", "", ...likelyOwners);
|
||||
|
||||
@ -3239,6 +3407,7 @@ function renderCloseCommentFromReport(markdown: string, reason: CloseReason): st
|
||||
bestSolution: reviewSectionValue(markdown, "bestSolution"),
|
||||
evidence: reportEvidence(markdown),
|
||||
likelyOwners: reportLikelyOwners(markdown),
|
||||
securityReview: reportSecurityReview(markdown),
|
||||
reviewLine: closeReviewLineFromReport(markdown),
|
||||
}),
|
||||
Number(frontMatterValue(markdown, "number")),
|
||||
@ -3282,6 +3451,7 @@ function normalizeComment(
|
||||
bestSolution: decision.bestSolution,
|
||||
evidence: decision.evidence,
|
||||
likelyOwners: decision.likelyOwners,
|
||||
securityReview: decision.securityReview,
|
||||
reviewLine: closeReviewLineFromDecision(decision, git, runtime),
|
||||
});
|
||||
}
|
||||
@ -3306,6 +3476,7 @@ function renderKeepOpenCommentFromReport(markdown: string): string {
|
||||
const evidence = reportEvidence(markdown).slice(0, 6).map(closeEvidenceLine);
|
||||
const likelyOwners = reportLikelyOwners(markdown).slice(0, 5).map(likelyOwnerLine);
|
||||
const reviewFindings = reportReviewFindings(markdown);
|
||||
const securityReview = reportSecurityReview(markdown);
|
||||
const summary = reviewSectionValue(markdown, "summary");
|
||||
const changeSummary = reviewSectionValue(markdown, "changeSummary");
|
||||
const bestSolution = reviewSectionValue(markdown, "bestSolution");
|
||||
@ -3354,6 +3525,7 @@ function renderKeepOpenCommentFromReport(markdown: string): string {
|
||||
"",
|
||||
nextStepLine,
|
||||
);
|
||||
lines.push("", "Security review:", "", securityReviewLine(securityReview));
|
||||
if (isPullRequest && reviewFindings.length) {
|
||||
lines.push(
|
||||
"",
|
||||
@ -3376,6 +3548,14 @@ function renderKeepOpenCommentFromReport(markdown: string): string {
|
||||
`Overall confidence: ${confidenceText(reportOverallConfidenceScore(markdown))}`,
|
||||
);
|
||||
}
|
||||
if (securityReview.concerns.length) {
|
||||
details.push(
|
||||
"",
|
||||
"Security concerns:",
|
||||
"",
|
||||
...securityReview.concerns.map(securityConcernDetailedLine),
|
||||
);
|
||||
}
|
||||
if (validation.length) details.push("", "Acceptance criteria:", "", ...validation);
|
||||
if (evidence.length) details.push("", "What I checked:", "", ...evidence);
|
||||
if (likelyOwners.length) details.push("", "Likely related people:", "", ...likelyOwners);
|
||||
@ -3639,6 +3819,40 @@ function markedReviewCommentBody(number: number, body: string): string {
|
||||
: `${body.trimEnd()}\n\n${reviewCommentMarker(number)}`;
|
||||
}
|
||||
|
||||
function reviewStartStatusCommentMarker(number: number): string {
|
||||
return `${REVIEW_START_STATUS_MARKER_PREFIX}:started item=${number} -->`;
|
||||
}
|
||||
|
||||
export function renderReviewStartStatusComment(options: ReviewStartStatusCommentOptions): string {
|
||||
const subject = options.kind === "pull_request" ? "pull request" : "issue";
|
||||
const progress =
|
||||
Number.isInteger(options.position) && Number.isInteger(options.total)
|
||||
? ` This is item ${options.position}/${options.total} in the current shard.`
|
||||
: "";
|
||||
const shard =
|
||||
Number.isInteger(options.shardIndex) && Number.isInteger(options.shardCount)
|
||||
? ` Shard ${options.shardIndex}/${options.shardCount}.`
|
||||
: "";
|
||||
const title = options.title.trim();
|
||||
const heading = title
|
||||
? `I am starting a fresh review of this ${subject}: ${title}`
|
||||
: `I am starting a fresh review of this ${subject}.`;
|
||||
return markedReviewCommentBody(
|
||||
options.number,
|
||||
[
|
||||
"ClawSweeper status: review started.",
|
||||
"",
|
||||
`${heading}${progress}${shard}`,
|
||||
"",
|
||||
"This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking.",
|
||||
"",
|
||||
"Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted.",
|
||||
"",
|
||||
reviewStartStatusCommentMarker(options.number),
|
||||
].join("\n"),
|
||||
);
|
||||
}
|
||||
|
||||
export function isCodexReviewCommentBody(body: string): boolean {
|
||||
return (
|
||||
body.includes("Codex review:") ||
|
||||
@ -3709,9 +3923,12 @@ function commentBodyMatches(comment: Record<string, unknown> | undefined, body:
|
||||
}
|
||||
|
||||
const PATCHABLE_REVIEW_COMMENT_AUTHORS = new Set(
|
||||
["openclaw-clawsweeper[bot]", process.env.CLAWSWEEPER_COMMENT_AUTHOR_LOGIN].filter(
|
||||
(login): login is string => typeof login === "string" && login.length > 0,
|
||||
),
|
||||
[
|
||||
"clawsweeper",
|
||||
"clawsweeper[bot]",
|
||||
"openclaw-clawsweeper[bot]",
|
||||
process.env.CLAWSWEEPER_COMMENT_AUTHOR_LOGIN,
|
||||
].filter((login): login is string => typeof login === "string" && login.length > 0),
|
||||
);
|
||||
|
||||
function commentAuthorLogin(comment: Record<string, unknown> | undefined): string | undefined {
|
||||
@ -3802,6 +4019,35 @@ function upsertReviewComment(
|
||||
return issueReviewComment(number, [markedBody]);
|
||||
}
|
||||
|
||||
function postReviewStartStatusComment(options: {
|
||||
item: Item;
|
||||
position: number;
|
||||
total: number;
|
||||
shardIndex: number;
|
||||
shardCount: number;
|
||||
}): "posted" | "existing" {
|
||||
if (issueReviewComment(options.item.number)) return "existing";
|
||||
const body = renderReviewStartStatusComment({
|
||||
number: options.item.number,
|
||||
kind: options.item.kind,
|
||||
title: options.item.title,
|
||||
position: options.position,
|
||||
total: options.total,
|
||||
shardIndex: options.shardIndex,
|
||||
shardCount: options.shardCount,
|
||||
});
|
||||
const payload = writeCommentPayload(options.item.number, body);
|
||||
ghWithRetry([
|
||||
"api",
|
||||
`repos/${targetRepo()}/issues/${options.item.number}/comments`,
|
||||
"--method",
|
||||
"POST",
|
||||
"--input",
|
||||
payload,
|
||||
]);
|
||||
return "posted";
|
||||
}
|
||||
|
||||
function closeItem(options: { number: number; kind: ItemKind; reason: CloseReason }): void {
|
||||
if (options.kind === "pull_request") {
|
||||
ghWithRetry(["pr", "close", String(options.number)]);
|
||||
@ -3904,6 +4150,43 @@ function renderReviewFindingsReportSection(decision: Decision): string {
|
||||
return lines.join("\n");
|
||||
}
|
||||
|
||||
function securityConcernLocation(concern: SecurityConcern): string {
|
||||
if (!concern.file) return "not tied to a single file";
|
||||
return `${concern.file}${concern.line ? `:${concern.line}` : ""}`;
|
||||
}
|
||||
|
||||
function renderSecurityReviewReportSection(decision: Decision): string {
|
||||
const lines = [
|
||||
`Status: ${decision.securityReview.status}`,
|
||||
"",
|
||||
`Summary: ${sentence(decision.securityReview.summary)}`,
|
||||
"",
|
||||
"Concerns:",
|
||||
"",
|
||||
];
|
||||
if (!decision.securityReview.concerns.length) {
|
||||
lines.push("- none");
|
||||
return lines.join("\n");
|
||||
}
|
||||
lines.push(
|
||||
decision.securityReview.concerns
|
||||
.map((concern) => {
|
||||
const location = securityConcernLocation(concern);
|
||||
const heading =
|
||||
location === "not tied to a single file"
|
||||
? `- **[${concern.severity}] ${concern.title}:**`
|
||||
: `- **[${concern.severity}] ${concern.title}:** \`${location}\``;
|
||||
return [
|
||||
heading,
|
||||
` - body: ${sentence(concern.body)}`,
|
||||
` - confidence: ${confidenceText(concern.confidenceScore)}`,
|
||||
].join("\n");
|
||||
})
|
||||
.join("\n"),
|
||||
);
|
||||
return lines.join("\n");
|
||||
}
|
||||
|
||||
function markdownFor(options: {
|
||||
item: Item;
|
||||
context: ItemContext;
|
||||
@ -3950,6 +4233,7 @@ function markdownFor(options: {
|
||||
: "- none";
|
||||
const bestSolution = options.decision.bestSolution.trim() || "_Not provided._";
|
||||
const reviewFindings = renderReviewFindingsReportSection(options.decision);
|
||||
const securityReview = renderSecurityReviewReportSection(options.decision);
|
||||
const workCandidateSection = renderWorkCandidateReportSection(options.decision);
|
||||
const repairWorkPromptSection = renderRepairWorkPromptReportSection(options.decision);
|
||||
return `---
|
||||
@ -4052,6 +4336,10 @@ ${bestSolution}
|
||||
|
||||
${reviewFindings}
|
||||
|
||||
## ${REVIEW_SECTIONS.securityReview}
|
||||
|
||||
${securityReview}
|
||||
|
||||
## ${REVIEW_SECTIONS.workCandidate}
|
||||
|
||||
${workCandidateSection}${repairWorkPromptSection}
|
||||
@ -4175,6 +4463,24 @@ function reviewCommand(args: Args): void {
|
||||
);
|
||||
const context = collectItemContext(item);
|
||||
const snapshotHash = itemSnapshotHash(item, context);
|
||||
try {
|
||||
const startComment = postReviewStartStatusComment({
|
||||
item,
|
||||
position: completed + 1,
|
||||
total: candidates.length,
|
||||
shardIndex,
|
||||
shardCount,
|
||||
});
|
||||
console.error(
|
||||
`[review] ${new Date().toISOString()} shard=${shardIndex}/${shardCount} start-comment=${startComment} #${item.number}`,
|
||||
);
|
||||
} catch (error) {
|
||||
console.error(
|
||||
`[review] ${new Date().toISOString()} shard=${shardIndex}/${shardCount} start-comment=failed #${item.number}: ${
|
||||
error instanceof Error ? error.message : String(error)
|
||||
}`,
|
||||
);
|
||||
}
|
||||
let decision: Decision;
|
||||
try {
|
||||
decision = runCodex({
|
||||
|
||||
@ -28,6 +28,7 @@ import {
|
||||
parseDecision,
|
||||
protectedLabels,
|
||||
relatedTitleSearchTerms,
|
||||
renderReviewStartStatusComment,
|
||||
reviewArtifactDestination,
|
||||
reviewAutomationMarkersFromReport,
|
||||
reviewActionForDecision,
|
||||
@ -123,6 +124,11 @@ function closeDecision(overrides = {}) {
|
||||
risks: [],
|
||||
bestSolution: "Keep the implementation as-is.",
|
||||
reviewFindings: [],
|
||||
securityReview: {
|
||||
status: "not_applicable",
|
||||
summary: "No patch security review is needed for this issue cleanup decision.",
|
||||
concerns: [],
|
||||
},
|
||||
overallCorrectness: "not a patch",
|
||||
overallConfidenceScore: 0.75,
|
||||
fixedRelease: null,
|
||||
@ -843,11 +849,31 @@ test("comment matcher recognizes old and new Codex review comments", () => {
|
||||
});
|
||||
|
||||
test("review comment patching only targets ClawSweeper-owned comments", () => {
|
||||
assert.equal(canPatchReviewComment({ user: { login: "clawsweeper" } }), true);
|
||||
assert.equal(canPatchReviewComment({ user: { login: "clawsweeper[bot]" } }), true);
|
||||
assert.equal(canPatchReviewComment({ user: { login: "openclaw-clawsweeper[bot]" } }), true);
|
||||
assert.equal(canPatchReviewComment({ user: { login: "steipete" } }), false);
|
||||
assert.equal(canPatchReviewComment(undefined), false);
|
||||
});
|
||||
|
||||
test("review start status comment is marker-backed and crustacean-friendly", () => {
|
||||
const comment = renderReviewStartStatusComment({
|
||||
number: 74453,
|
||||
kind: "pull_request",
|
||||
title: "fix webhook limiter",
|
||||
position: 1,
|
||||
total: 3,
|
||||
shardIndex: 0,
|
||||
shardCount: 2,
|
||||
});
|
||||
|
||||
assert.match(comment, /ClawSweeper status: review started\./);
|
||||
assert.match(comment, /claws on keyboard/);
|
||||
assert.match(comment, /<!-- clawsweeper-review-status:started item=74453 -->/);
|
||||
assert.match(comment, /<!-- clawsweeper-review item=74453 -->/);
|
||||
assert.doesNotMatch(comment, /Codex review:/);
|
||||
});
|
||||
|
||||
test("pull request keep-open review comments label the change summary", () => {
|
||||
const comment = renderReviewCommentFromReport(
|
||||
`${reportFrontMatter({
|
||||
@ -901,6 +927,86 @@ Reason: Maintainers should review the tests after the targeted lane is green.
|
||||
assert.match(comment, /<!-- clawsweeper-verdict:needs-human item=74265 sha=abc123def456/);
|
||||
});
|
||||
|
||||
test("pull request review comments include dedicated security review", () => {
|
||||
const comment = renderReviewCommentFromReport(
|
||||
`${reportFrontMatter({
|
||||
type: "pull_request",
|
||||
number: "74265",
|
||||
decision: "keep_open",
|
||||
close_reason: "none",
|
||||
work_candidate: "none",
|
||||
pull_head_sha: "abc123def456",
|
||||
})}
|
||||
|
||||
## Summary
|
||||
|
||||
Keep this PR open for maintainer review.
|
||||
|
||||
## What This Changes
|
||||
|
||||
Updates a workflow permission for review comments.
|
||||
|
||||
## Best Possible Solution
|
||||
|
||||
Land the workflow permission change after normal CI.
|
||||
|
||||
## Security Review
|
||||
|
||||
Status: needs_attention
|
||||
|
||||
Summary: The workflow now asks for issue write permission, so the permission scope needs maintainer confirmation.
|
||||
|
||||
Concerns:
|
||||
|
||||
- **[medium] Confirm issue write scope:** \`.github/workflows/sweep.yml:652\`
|
||||
- body: The review shard now writes comments during review, so maintainers should confirm the app permission is intended.
|
||||
- confidence: 0.82
|
||||
|
||||
## Review Findings
|
||||
|
||||
Overall correctness: patch is correct
|
||||
|
||||
Overall confidence: 0.85
|
||||
|
||||
Full review comments:
|
||||
|
||||
- none
|
||||
|
||||
## Work Candidate
|
||||
|
||||
Candidate: none
|
||||
|
||||
Confidence: low
|
||||
|
||||
Priority: low
|
||||
|
||||
Reason: Normal maintainer review is sufficient.
|
||||
|
||||
## Evidence
|
||||
|
||||
- **workflow:** Review shard requests issue write permission.
|
||||
|
||||
## Likely Related People
|
||||
|
||||
- **@alice:** recent workflow maintainer
|
||||
- reason: touched the workflow recently
|
||||
- commits: abc123
|
||||
- files: .github/workflows/sweep.yml
|
||||
- confidence: high
|
||||
|
||||
## Risks / Open Questions
|
||||
|
||||
- none
|
||||
`,
|
||||
"none",
|
||||
);
|
||||
|
||||
assert.match(comment, /Security review:/);
|
||||
assert.match(comment, /Security review needs attention:/);
|
||||
assert.match(comment, /Confirm issue write scope/);
|
||||
assert.match(comment, /Review details/);
|
||||
});
|
||||
|
||||
test("pull request keep-open review comments surface Codex-style findings", () => {
|
||||
const comment = renderReviewCommentFromReport(
|
||||
`${reportFrontMatter({
|
||||
@ -1268,6 +1374,11 @@ test("decision parser enforces required schema-shaped evidence", () => {
|
||||
}),
|
||||
/decision\.workCandidate/,
|
||||
);
|
||||
assert.throws(() => {
|
||||
const decision = closeDecision();
|
||||
delete decision.securityReview;
|
||||
return parseDecision(decision);
|
||||
}, /decision\.securityReview/);
|
||||
const workCandidate = parseDecision(
|
||||
closeDecision({
|
||||
decision: "keep_open",
|
||||
@ -1299,6 +1410,14 @@ test("review prompt routes PR likely owners through feature history", () => {
|
||||
assert.match(prompt, /use names without email addresses/);
|
||||
});
|
||||
|
||||
test("review prompt requires a dedicated securityReview section", () => {
|
||||
const prompt = readFileSync("prompts/review-item.md", "utf8");
|
||||
|
||||
assert.match(prompt, /Always summarize this pass in `securityReview`/);
|
||||
assert.match(prompt, /Always fill `securityReview`/);
|
||||
assert.match(prompt, /status: "needs_attention"/);
|
||||
});
|
||||
|
||||
test("review parser strips environment access caveats from risks", () => {
|
||||
const parsed = parseDecision(
|
||||
closeDecision({
|
||||
|
||||
Loading…
Reference in New Issue
Block a user