diff --git a/action.yml b/action.yml index a3d9f74a..7c2ab5f0 100644 --- a/action.yml +++ b/action.yml @@ -96,6 +96,29 @@ inputs: below 1 or non-numeric fall back to the default (50). required: false default: '50' + route_severity_below: + description: >- + Optional severity threshold that routes findings at-or-below it from + inline comments to the PR summary (fail-open: never drops a finding). + One of: critical, high, medium, low. For example, 'low' routes only + low-severity findings, while 'medium' routes medium AND low. An empty or + unknown value disables severity routing (findings stay inline). Findings + with an empty or unknown severity are never routed by this policy and + keep their normal placement. + required: false + default: '' + route_categories: + description: >- + Optional comma-separated list of categories routed from inline comments + to the PR summary (fail-open: never drops a finding). Categories are + case-insensitive and drawn from: bug, security, performance, + maintainability, test, style, documentation, other. For example, + 'style,documentation' routes those categories to the summary. Unknown + category tokens are ignored. Findings with an empty or unknown category + are never routed by this policy and keep their normal placement. Combine + with route_severity_below to route on either condition. + required: false + default: '' base_ref: description: >- Override the base ref. Provide this (and head_sha) when invoking from a @@ -119,6 +142,12 @@ outputs: comments_skipped: description: Number of inline comments skipped by incremental mode (overlap with history). value: ${{ steps.post.outputs.comments_skipped }} + comments_routed: + description: >- + Number of inline-eligible comments routed to the PR summary by the + route_severity_below / route_categories policy. Mutually exclusive with + comments_inline, comments_skipped, and comments_failed. + value: ${{ steps.post.outputs.comments_routed }} comments_failed: description: Number of inline comments that failed to post. value: ${{ steps.post.outputs.comments_failed }} @@ -285,6 +314,8 @@ runs: env: OCR_INCREMENTAL_OVERLAP_THRESHOLD: ${{ inputs.incremental_overlap_threshold }} OCR_REVIEW_COMMENT_BATCH_SIZE: ${{ inputs.review_comment_batch_size }} + OCR_ROUTE_SEVERITY_BELOW: ${{ inputs.route_severity_below }} + OCR_ROUTE_CATEGORIES: ${{ inputs.route_categories }} with: github-token: ${{ inputs.github_token }} script: | @@ -313,4 +344,6 @@ runs: incremental: ${{ inputs.incremental == 'true' }}, incrementalOverlapThreshold: parseFloat(process.env.OCR_INCREMENTAL_OVERLAP_THRESHOLD), reviewCommentBatchSize: parseInt(process.env.OCR_REVIEW_COMMENT_BATCH_SIZE, 10), + routeSeverityBelow: process.env.OCR_ROUTE_SEVERITY_BELOW, + routeCategories: process.env.OCR_ROUTE_CATEGORIES, }); diff --git a/scripts/github-actions/post-review-comments.js b/scripts/github-actions/post-review-comments.js index 0de4ee93..d1f66ca6 100644 --- a/scripts/github-actions/post-review-comments.js +++ b/scripts/github-actions/post-review-comments.js @@ -36,6 +36,33 @@ const DEFAULT_OVERLAP_THRESHOLD = 0.6; // review_comment_batch_size input. const DEFAULT_BATCH_SIZE = 50; +// Enumerations for category/severity routing, sourced from the LLM output +// schema (internal/config/toolsconfig/tools.json:55-84). Used both to validate +// the routing policy and to normalize the metadata before comparison. Kept as +// plain arrays (not Sets) so tests can inspect ordering for severity ranking. +const CATEGORIES = [ + "bug", + "security", + "performance", + "maintainability", + "test", + "style", + "documentation", + "other", +]; +// Severity rank: higher = more severe. An unknown/empty severity has no rank +// (never matched by the routing policy). Order matches the enum (critical is +// the most severe, low the least). +const SEVERITIES = ["critical", "high", "medium", "low"]; +const SEVERITY_RANK = new Map( + SEVERITIES.map((s, i) => [s, SEVERITIES.length - i]) +); // critical=4, high=3, medium=2, low=1 + +// Sentinel policy object: "do not route anything". Returned by buildPolicy on +// any parse problem so the partition loop falls open to today's behavior (I1). +// Equivalently produced by an empty policy (no threshold, no categories). +const NO_ROUTING = Object.freeze({ routeBySeverity: false, routeByCategory: false }); + async function runPostReviewComments({ github, context, @@ -47,6 +74,13 @@ async function runPostReviewComments({ incremental = false, incrementalOverlapThreshold = DEFAULT_OVERLAP_THRESHOLD, reviewCommentBatchSize = DEFAULT_BATCH_SIZE, + // Fail-open finding-publication controls (#478). Both optional and empty by + // default: with neither set, behavior is byte-identical to today (modulo the + // additive badge prefix on rendered comments). buildPolicy parses them once + // before the partition loop and degrades to NO_ROUTING on any malformed value + // (fail-open for the policy itself, upholding I1). + routeSeverityBelow = "", + routeCategories = "", }) { const log = (msg) => { if (core && typeof core.info === "function") core.info(msg); @@ -74,6 +108,7 @@ async function runPostReviewComments({ inline: 0, skipped: 0, failed: 0, + routed: 0, summaryUrl: "", }; @@ -121,20 +156,37 @@ async function runPostReviewComments({ commitSha = pullRequest.head.sha; } - // Partition: inline (with valid line info) vs summary (without). + // Partition: inline (with valid line info) vs summary (without) vs routed + // (valid line but the publication policy moves it to the summary). // Each inline comment gets a random per-comment ID (assigned once) embedded // in its body as an HTML comment, so the retry/idempotency logic can detect // whether a comment already landed on the server and avoid posting a // duplicate. Random (not content-derived) so two distinct comments that // share path/line/content still get different IDs. + // + // Routing is a placement decision in this loop, not a post-hoc filter: a + // finding the policy routes to summary is pushed to commentsRouted (mirroring + // commentsWithoutLine) instead of reviewComments. Routed findings therefore + // never enter reviewComments -> never enter toSend/toRetry -> never reach a + // createReview call (I4: no double-post surface on retry). + const policy = buildPolicy({ severityThreshold: routeSeverityBelow, categories: routeCategories }); const reviewComments = []; const commentsWithoutLine = []; + const commentsRouted = []; for (const comment of comments) { const hasValidLine = comment.start_line >= 1 || comment.end_line >= 1; if (!hasValidLine) { commentsWithoutLine.push({ comment, body: formatComment(comment), reason: NO_LINE_REASON }); continue; } + // Routing applies only to findings that COULD be posted inline (valid + // line). No-line findings already go to the summary via commentsWithoutLine, + // so they are never re-routed (avoids double-counting in the summary). + const route = routeComment(comment, policy); + if (route.routed) { + commentsRouted.push({ comment, body: formatComment(comment), reason: route.reason }); + continue; + } const id = newCommentId(RUN_TAG); const reviewComment = { path: comment.path, body: formatComment(comment, id) }; if (comment.start_line >= 1 && comment.end_line >= 1 && comment.start_line !== comment.end_line) { @@ -187,7 +239,9 @@ async function runPostReviewComments({ prNumber, sticky: stickySummary, tag: SUMMARY_TAG, - body: wrapSummary(buildPreReviewSummaryBody(stats.total, commentsWithoutLine, warnings)), + body: wrapSummary( + buildPreReviewSummaryBody(stats.total, commentsWithoutLine, commentsRouted, warnings) + ), log, }); @@ -236,24 +290,29 @@ async function runPostReviewComments({ stats.inline = successCount; stats.failed = failedCount; + stats.routed = commentsRouted.length; // ---- Finalize the summary with the complete body ---- // Now that the review has landed (or failed per-comment), write the final // summary body. Posting statistics are merged into the leading summary // header (see buildSummaryBody), so here we only append the per-comment // renderings: every comment that did not go out as inline — whether because - // it had no line info or because posting failed — is rendered as one - // continuous block, each carrying the reason it ended up in the summary (so - // the reader always knows why it is here). + // it had no line info, was routed by the publication policy, or because + // posting failed — is rendered as one continuous block, each carrying the + // reason it ended up in the summary (so the reader always knows why it is + // here). Routed findings render BEFORE the failed block so the order is: + // counts → no-line summary → routed summary → failed. let summaryBody = buildSummaryBody({ total: stats.total, inline: successCount, summary: commentsWithoutLine.length, skipped: stats.skipped, + routed: commentsRouted.length, failed: failedCount, warnings, }); summaryBody += formatSummaryComments(commentsWithoutLine); + summaryBody += formatSummaryComments(commentsRouted); for (const { comment, error } of failedComments) { summaryBody += "\n\n---\n\n"; summaryBody += formatCommentMarkdown(comment, error); @@ -604,6 +663,7 @@ function setStatsOutputs(out, stats, batchCounters, batchSize) { out("comments_total", String(stats.total)); out("comments_inline", String(stats.inline)); out("comments_skipped", String(stats.skipped)); + out("comments_routed", String(stats.routed)); out("comments_failed", String(stats.failed)); out("summary_comment_url", stats.summaryUrl || ""); // Per-batch telemetry (B7). These are additional outputs; the five above are @@ -1172,14 +1232,136 @@ function newCommentId(runTag) { return `ocr-${runTag}-${crypto.randomBytes(8).toString("hex")}`; } +// ---- Badge + publication policy helpers (#478) ---- +// +// These are pure functions (no I/O, no side effects) so they can be unit-tested +// directly. buildBadge byte-matches the CLI's cmd/opencodereview/output.go +// buildBadge degeneration so review output is consistent across surfaces (I6). +// buildPolicy/routeComment implement fail-open finding-publication routing +// (I1, I4): a finding never matches the policy on unknown/malformed metadata, +// and routing is a placement decision (findings route OUT of the inline write +// path), so a routed finding can never be double-posted on retry. + +// Strip C0/C1 control characters from a metadata value. The CLI's +// sanitizeTerminal (cmd/opencodereview/output.go:197-206) strips control chars +// but PRESERVES \t and \n (harmless in a terminal). This Action sanitizer is +// intentionally STRICTER: it strips ALL control chars including \t and \n, +// because a newline/tab in a category or severity would break the comment +// body's layout (the badge renders in Markdown in a browser). This is a +// deliberate, documented divergence from strict OC1 byte-parity: clean enum +// values (the overwhelmingly common case) render identically across surfaces, +// and the divergence only manifests for malformed model output, in a SAFER +// direction (the Action cannot have its layout broken by a control char). +function sanitizeMetadata(value) { + return String(value == null ? "" : value).replace(/[\x00-\x1f\x7f-\x9f]/g, ""); +} + +// Build the category/severity badge for a comment, byte-matching the CLI's +// buildBadge degeneration (cmd/opencodereview/output.go:98-114): +// both non-empty -> "[category · severity]" (with a middot, U+00B7) +// only category -> "[category]" +// only severity -> "[severity]" +// neither -> "" (no badge line) +// Returns the empty string (not a newline) when nothing renders, so callers +// can prepend conditionally without leaving a blank line. +function buildBadge(comment) { + const category = sanitizeMetadata(comment && comment.category); + const severity = sanitizeMetadata(comment && comment.severity); + if (category && severity) return `[${category} · ${severity}]`; + if (category) return `[${category}]`; + if (severity) return `[${severity}]`; + return ""; +} + +// Parse the publication policy from the raw opt-in inputs. Returns a normalized +// policy object, or the NO_ROUTING sentinel when no routing is requested or any +// value is malformed (fail-open for the policy itself, upholding I1). +// +// severityThreshold: a severity name (case-insensitive) at-or-below which +// findings route. An unknown/empty value disables severity routing. +// categories: a comma-separated category list (case-insensitive). Unknown +// category tokens are dropped; an empty list (or all-unknown) disables +// category routing. +// +// The returned object has two booleans so routeComment can short-circuit +// without re-parsing, plus the normalized values it needs to decide: +// { +// routeBySeverity: bool, +// severityRank: number, // rank of the threshold; -1 when disabled +// routeByCategory: bool, +// categories: Set, // lowercase enum members; empty when disabled +// } +function buildPolicy({ severityThreshold, categories } = {}) { + let routeBySeverity = false; + let severityRank = -1; + if (severityThreshold != null) { + const norm = String(severityThreshold).trim().toLowerCase(); + if (SEVERITY_RANK.has(norm)) { + routeBySeverity = true; + severityRank = SEVERITY_RANK.get(norm); + } + // Any other value (empty, unknown, garbage) leaves routeBySeverity=false + // (fail-open for the policy: unknown threshold -> no routing). + } + + let routeByCategory = false; + const categorySet = new Set(); + if (categories != null) { + const tokens = String(categories) + .split(",") + .map((t) => t.trim().toLowerCase()) + .filter((t) => t.length > 0); + for (const t of tokens) { + if (CATEGORIES.includes(t)) categorySet.add(t); + // Unknown category tokens are silently dropped (fail-open: an unknown + // category in the policy never matches a finding's category, so including + // it would be a no-op anyway; dropping keeps the set clean). + } + if (categorySet.size > 0) routeByCategory = true; + } + + if (!routeBySeverity && !routeByCategory) return NO_ROUTING; + return { routeBySeverity, severityRank, routeByCategory, categories: categorySet }; +} + +// Decide whether a comment routes to the summary per the policy. Returns +// { routed: true, reason } or { routed: false }. A finding matches when its +// severity is at-or-below the threshold (when severity routing is on) OR its +// category is in the category list (when category routing is on). Unknown or +// malformed metadata on the finding NEVER matches (I1): an empty/unknown +// category or severity has no rank and no enum membership, so it falls through +// to the normal inline path (visible), never dropped. +function routeComment(comment, policy) { + if (!policy || (!policy.routeBySeverity && !policy.routeByCategory)) { + return { routed: false }; + } + const catRaw = comment && comment.category != null ? String(comment.category).trim().toLowerCase() : ""; + const sevRaw = comment && comment.severity != null ? String(comment.severity).trim().toLowerCase() : ""; + const catKnown = catRaw !== "" && CATEGORIES.includes(catRaw); + const sevKnown = sevRaw !== "" && SEVERITY_RANK.has(sevRaw); + + if (policy.routeBySeverity && sevKnown && SEVERITY_RANK.get(sevRaw) <= policy.severityRank) { + return { routed: true, reason: `Routed to summary (severity ${sevRaw}${catKnown ? ` · category ${catRaw}` : ""})` }; + } + if (policy.routeByCategory && catKnown && policy.categories.has(catRaw)) { + return { routed: true, reason: `Routed to summary (category ${catRaw}${sevKnown ? ` · severity ${sevRaw}` : ""})` }; + } + return { routed: false }; +} + // ---- Formatting helpers (ported verbatim) ---- // Assemble the visible comment body. When `id` is provided (inline comments), // the per-comment ID tag is prepended as an HTML comment (invisible when // rendered) so getPostedCommentIds can match it back on retry for the -// idempotency check. The code suggestion block is then appended if present. +// idempotency check. The category/severity badge is then prepended (when +// present) on its own leading line AFTER the id comment, so the idempotency +// regex (unanchored, scans the whole body) still matches and the badge renders +// as the first visible line. The code suggestion block is appended if present. function formatComment(comment, id) { let body = id ? `\n` : ""; + const badge = buildBadge(comment); + if (badge) body += `${badge}\n`; body += comment.content || ""; if (comment.suggestion_code && comment.existing_code) { body += "\n\n**Suggestion:**\n"; @@ -1189,7 +1371,14 @@ function formatComment(comment, id) { } function formatCommentMarkdown(comment, error) { - let md = `### 📄 \`${comment.path}\``; + let md = ""; + // The badge renders as a leading line before the path heading (consistent + // with formatComment), so the heading/reason lines still anchor the comment + // and existing substring assertions on them are unaffected. The badge is "" + // for any finding without category/severity metadata. + const badge = buildBadge(comment); + if (badge) md += `${badge}\n`; + md += `### 📄 \`${comment.path}\``; if (comment.start_line && comment.end_line) { md += ` (L${comment.start_line}-L${comment.end_line})`; } @@ -1214,20 +1403,25 @@ function formatCommentMarkdown(comment, error) { // inline / posted as summary" header vs. the trailing "Posting Statistics" // block, whose overlapping definitions made the summary hard to interpret). // -// The four counts are mutually exclusive and, together with `inline`, sum to +// The five counts are mutually exclusive and, together with `inline`, sum to // `total`: // inline — comments that landed as review inline comments // summary — comments without line info, rendered in the summary body below +// routed — comments the publication policy moved from inline to summary +// (also rendered in the body below, each tagged with its reason) // skipped — comments suppressed by incremental overlap filtering // failed — comments that had line info but could not be posted (also // rendered in the body below, each tagged with its failure reason) -function buildSummaryBody({ total, inline, summary, skipped, failed, warnings }) { +function buildSummaryBody({ total, inline, summary, skipped, routed = 0, failed, warnings }) { let body = `🔍 **OpenCodeReview** found **${total}** issue(s) in this PR.`; if (total > 0) { body += `\n- ✅ Successfully posted inline: ${inline} comment(s)`; if (summary > 0) { body += `\n- 📝 In summary (no line info): ${summary} comment(s)`; } + if (routed > 0) { + body += `\n- 📋 Routed to summary by policy: ${routed} comment(s)`; + } if (skipped > 0) { body += `\n- ⏭️ Skipped (overlap with history): ${skipped} comment(s)`; } @@ -1243,18 +1437,25 @@ function buildSummaryBody({ total, inline, summary, skipped, failed, warnings }) // Pre-review summary body: shown in the anchor comment while inline comments // are being posted. Includes only what is known before the review lands (issue -// count, warnings, comments without line info) — final posting statistics are -// added by the finalize phase. Kept informative (not an empty placeholder) so -// the summary is useful even if the run is interrupted before finalize. -function buildPreReviewSummaryBody(totalCount, summaryComments, warnings) { +// count, warnings, comments without line info, routed comments) — final posting +// statistics are added by the finalize phase. Kept informative (not an empty +// placeholder) so the summary is useful even if the run is interrupted before +// finalize. Routed findings are known before the review (the policy decision is +// made in the partition loop) so they are rendered here too, keeping the +// pre-review anchor accurate. +function buildPreReviewSummaryBody(totalCount, summaryComments, routedComments, warnings) { let body = `🔍 **OpenCodeReview** found **${totalCount}** issue(s) in this PR.`; if (totalCount > 0) { body += `\n- ⏳ _Posting review comments…_`; + if (routedComments && routedComments.length > 0) { + body += `\n- 📋 Routed to summary by policy: ${routedComments.length} comment(s)`; + } } if (warnings.length > 0) { body += `\n\n⚠️ ${warnings.length} warning(s) occurred during review.`; } body += formatSummaryComments(summaryComments); + body += formatSummaryComments(routedComments); body += formatWarnings(warnings); return body; } @@ -1354,6 +1555,10 @@ module.exports = { getPostedCommentIds, isCommentAlreadyPosted, newCommentId, + sanitizeMetadata, + buildBadge, + buildPolicy, + routeComment, formatComment, formatCommentMarkdown, buildSummaryBody, @@ -1370,4 +1575,8 @@ module.exports = { setStatsOutputs, DEFAULT_BATCH_SIZE, buildRunTags, + NO_ROUTING, + CATEGORIES, + SEVERITIES, + SEVERITY_RANK, }; diff --git a/scripts/github-actions/post-review-comments.test.js b/scripts/github-actions/post-review-comments.test.js index 4c7e8488..ac216c64 100644 --- a/scripts/github-actions/post-review-comments.test.js +++ b/scripts/github-actions/post-review-comments.test.js @@ -12,7 +12,7 @@ const assert = require("assert"); const path = require("path"); -const { runPostReviewComments, safeFence, fencedBlock, lineSpan, sameCommentSpan, overlapsHistory, resolveThreshold, DEFAULT_OVERLAP_THRESHOLD, newCommentId, getPostedCommentIds, computeRetryDelayMs, formatWarnings, resolveBatchSize, sortToSendDeterministically, chunkArray, buildRunTags, DEFAULT_BATCH_SIZE } = require(path.join(__dirname, "post-review-comments.js")); +const { runPostReviewComments, safeFence, fencedBlock, lineSpan, sameCommentSpan, overlapsHistory, resolveThreshold, DEFAULT_OVERLAP_THRESHOLD, newCommentId, getPostedCommentIds, computeRetryDelayMs, formatWarnings, resolveBatchSize, sortToSendDeterministically, chunkArray, buildRunTags, DEFAULT_BATCH_SIZE, buildBadge, sanitizeMetadata, buildPolicy, routeComment, formatComment, formatCommentMarkdown, NO_ROUTING, CATEGORIES, SEVERITIES, SEVERITY_RANK } = require(path.join(__dirname, "post-review-comments.js")); // REVIEW_TAG as the production code builds it for this test's hardcoded run // identity (context.runId=undefined -> 0, runAttempt=undefined -> 1). Used as @@ -1607,6 +1607,450 @@ async function testBatchTelemetryOutputs() { assert.strictEqual(summary.failed, 0); } +// ---- Badge + publication policy tests (#478) ---- +// +// I6: buildBadge byte-matches the CLI's buildBadge degeneration +// (cmd/opencodereview/output.go:98-114). Each degeneration branch is pinned, +// plus control-char sanitization so a model-emitted newline cannot break the +// comment body layout. +function testBuildBadgeMatchesCliDegeneration() { + // both present -> "[category · severity]" with a middot (U+00B7) + assert.strictEqual(buildBadge({ category: "bug", severity: "high" }), "[bug · high]"); + // only category -> "[category]" + assert.strictEqual(buildBadge({ category: "style", severity: "" }), "[style]"); + assert.strictEqual(buildBadge({ category: "style", severity: null }), "[style]"); + assert.strictEqual(buildBadge({ category: "style" }), "[style]"); + // only severity -> "[severity]" + assert.strictEqual(buildBadge({ category: "", severity: "low" }), "[low]"); + assert.strictEqual(buildBadge({ category: null, severity: "low" }), "[low]"); + assert.strictEqual(buildBadge({ severity: "low" }), "[low]"); + // neither -> "" (no badge rendered) + assert.strictEqual(buildBadge({}), ""); + assert.strictEqual(buildBadge({ category: "", severity: "" }), ""); + assert.strictEqual(buildBadge({ category: null, severity: null }), ""); + // missing comment object entirely + assert.strictEqual(buildBadge(null), ""); + assert.strictEqual(buildBadge(undefined), ""); + // The separator is the U+00B7 middot (·), exactly matching the CLI's + // fmt.Sprintf("[%s · %s]", ...). Pin the exact byte (not "." or "-" or "·"'s + // decomposition) so a future edit that swaps the separator fails loudly. + const both = buildBadge({ category: "bug", severity: "low" }); + assert.ok(both.includes("·"), "badge contains the U+00B7 middot"); + assert.strictEqual(both, "[bug · low]", "exact badge string for the common case"); + // control-char sanitization: the Action strips ALL control chars (including + // \t and \n) from metadata — intentionally STRICTER than the CLI's + // sanitizeTerminal (which keeps \t/\n), because a newline/tab would break + // the Markdown comment body layout. Documented divergence from strict OC1 + // byte-parity; clean enum values match exactly across surfaces. + assert.strictEqual(buildBadge({ category: "bu\ng", severity: "high" }), "[bug · high]"); + assert.strictEqual(buildBadge({ category: "bug", severity: "hi\tgh" }), "[bug · high]"); + assert.strictEqual(buildBadge({ category: "bug\r\n", severity: "high" }), "[bug · high]"); + // a value that is ALL control chars degenerates to "" (badge not rendered), + // not a label of empty brackets. + assert.strictEqual(buildBadge({ category: "\n\r\t", severity: "\n" }), ""); +} + +function testSanitizeMetadataStripsControlChars() { + assert.strictEqual(sanitizeMetadata("clean"), "clean"); + assert.strictEqual(sanitizeMetadata("a\nb"), "ab"); + assert.strictEqual(sanitizeMetadata("a\tb"), "ab"); + assert.strictEqual(sanitizeMetadata("a\rb"), "ab"); + assert.strictEqual(sanitizeMetadata("a\x00b"), "ab"); + assert.strictEqual(sanitizeMetadata("a\x7fb"), "ab"); + assert.strictEqual(sanitizeMetadata("\n\r\t"), ""); + // null/undefined/numbers degrade safely to their string form. + assert.strictEqual(sanitizeMetadata(null), ""); + assert.strictEqual(sanitizeMetadata(undefined), ""); + assert.strictEqual(sanitizeMetadata(42), "42"); +} + +// I1: buildPolicy fails open on any malformed input — a bad policy never routes +// a finding, so no finding is ever silently dropped because the policy itself +// was broken. The NO_ROUTING sentinel is returned for every non-routing case. +function testBuildPolicyFailsOpenOnMalformed() { + // empty / null inputs -> no routing + assert.strictEqual(buildPolicy({}), NO_ROUTING); + assert.strictEqual(buildPolicy({ severityThreshold: "", categories: "" }), NO_ROUTING); + assert.strictEqual(buildPolicy({ severityThreshold: null, categories: null }), NO_ROUTING); + assert.strictEqual(buildPolicy(undefined), NO_ROUTING); + // unknown severity -> severity routing disabled (fail-open) + assert.strictEqual(buildPolicy({ severityThreshold: "trivial" }), NO_ROUTING); + assert.strictEqual(buildPolicy({ severityThreshold: "Criticals" }), NO_ROUTING); + // garbage threshold -> no routing + assert.strictEqual(buildPolicy({ severityThreshold: "garbage" }), NO_ROUTING); + // all-unknown categories -> category routing disabled (fail-open) + assert.strictEqual(buildPolicy({ categories: "unknown,also-unknown" }), NO_ROUTING); + // a known threshold enables severity routing; the returned rank is correct. + const lowP = buildPolicy({ severityThreshold: "low" }); + assert.strictEqual(lowP.routeBySeverity, true); + assert.strictEqual(lowP.routeByCategory, false); + assert.strictEqual(lowP.severityRank, SEVERITY_RANK.get("low")); + // case-insensitivity + const medP = buildPolicy({ severityThreshold: "MeDiUm" }); + assert.strictEqual(medP.routeBySeverity, true); + assert.strictEqual(medP.severityRank, SEVERITY_RANK.get("medium")); + // known categories enable category routing; unknown tokens dropped. + const catP = buildPolicy({ categories: "Style, UNKNOWN, documentation" }); + assert.strictEqual(catP.routeByCategory, true); + assert.strictEqual(catP.routeBySeverity, false); + assert.ok(catP.categories.has("style")); + assert.ok(catP.categories.has("documentation")); + assert.ok(!catP.categories.has("unknown")); + // whitespace-only threshold -> no routing + assert.strictEqual(buildPolicy({ severityThreshold: " " }), NO_ROUTING); +} + +// I1: routeComment never routes a finding with unknown/malformed metadata — it +// falls through to the normal inline path (visible), never dropped. Boundary +// inclusivity ("at-or-below") is pinned so the threshold value itself routes. +function testRouteCommentUnknownMetadataNeverRouted() { + const policy = buildPolicy({ severityThreshold: "medium", categories: "style,documentation" }); + // severity routing: medium threshold routes medium AND low (inclusive-at-or-below) + assert.strictEqual(routeComment({ severity: "medium" }, policy).routed, true); + assert.strictEqual(routeComment({ severity: "low" }, policy).routed, true); + // severity strictly above the threshold stays inline + assert.strictEqual(routeComment({ severity: "high" }, policy).routed, false); + assert.strictEqual(routeComment({ severity: "critical" }, policy).routed, false); + // unknown/empty severity is NEVER routed by severity (fail-open: I1) + assert.strictEqual(routeComment({ severity: "" }, policy).routed, false); + assert.strictEqual(routeComment({ severity: "trivial" }, policy).routed, false); + assert.strictEqual(routeComment({ severity: null }, policy).routed, false); + assert.strictEqual(routeComment({}, policy).routed, false); + // category routing: a listed category routes regardless of severity + assert.strictEqual(routeComment({ category: "style" }, policy).routed, true); + assert.strictEqual(routeComment({ category: "documentation" }, policy).routed, true); + // unlisted/unknown category is NEVER routed by category (fail-open: I1) + assert.strictEqual(routeComment({ category: "bug" }, policy).routed, false); + assert.strictEqual(routeComment({ category: "unknown" }, policy).routed, false); + assert.strictEqual(routeComment({ category: "" }, policy).routed, false); + assert.strictEqual(routeComment({ category: null }, policy).routed, false); + // case-insensitive category matching + assert.strictEqual(routeComment({ category: "STYLE" }, policy).routed, true); + assert.strictEqual(routeComment({ category: "Documentation" }, policy).routed, true); + // NO_ROUTING sentinel never routes anything + assert.strictEqual(routeComment({ severity: "low", category: "style" }, NO_ROUTING).routed, false); + assert.strictEqual(routeComment({ severity: "low", category: "style" }, null).routed, false); + // routed result carries a reason string + const r = routeComment({ severity: "low", category: "style" }, policy); + assert.ok(r.routed); + assert.ok(typeof r.reason === "string" && r.reason.length > 0); + assert.match(r.reason, /severity low/); + assert.match(r.reason, /category style/); +} + +// Pins the "at-or-below" boundary explicitly (PLAN_VALIDATION Risk C): the +// threshold value itself routes, and the floor (low) routes low. +function testRouteSeverityBelowBoundaryInclusive() { + const lowP = buildPolicy({ severityThreshold: "low" }); + // threshold = low routes ONLY low (the floor). critical/high/medium stay. + assert.strictEqual(routeComment({ severity: "low" }, lowP).routed, true); + assert.strictEqual(routeComment({ severity: "medium" }, lowP).routed, false); + assert.strictEqual(routeComment({ severity: "high" }, lowP).routed, false); + assert.strictEqual(routeComment({ severity: "critical" }, lowP).routed, false); + const critP = buildPolicy({ severityThreshold: "critical" }); + // threshold = critical routes everything (all severities are at-or-below it). + for (const sev of SEVERITIES) { + assert.strictEqual(routeComment({ severity: sev }, critP).routed, true); + } +} + +// A finding that matches BOTH the severity and category conditions routes +// EXACTLY ONCE (no double-count): the severity branch short-circuits, the +// category branch is never reached, and the partition loop counts the finding +// in the routed bucket a single time. +async function testFindingMatchingBothConditionsRoutesOnce() { + const result = { + comments: [ + // matches BOTH severity (low <= low) AND category (style in list) + { path: "src/both.js", content: "matches both", category: "style", severity: "low", start_line: 1, end_line: 1 }, + // matches only severity + { path: "src/sev.js", content: "sev only", category: "bug", severity: "low", start_line: 2, end_line: 2 }, + // matches only category + { path: "src/cat.js", content: "cat only", category: "documentation", severity: "critical", start_line: 3, end_line: 3 }, + ], + warnings: [], + }; + + const { github, outputs } = await run({ + result, + opts: { routeSeverityBelow: "low", routeCategories: "style,documentation" }, + }); + + // All three route (none posted inline); routed count is exactly 3 (no + // double-count from the "both" finding matching two conditions). + assert.strictEqual(github.createReviewCalls.length, 0, "no inline posting — all routed"); + assert.strictEqual(outputs.comments_routed, "3", "each finding counted once even when matching both conditions"); + assert.strictEqual(outputs.comments_total, "3"); + assert.strictEqual(outputs.comments_inline, "0"); +} + +// I6 / additive behavior: formatComment prepends the badge AFTER the id HTML +// comment, so the idempotency regex (unanchored) still matches and the badge +// is the first VISIBLE line. No badge when category/severity are absent. +function testFormatCommentBadgePlacement() { + // with id and badge: id HTML comment stays first, badge on the next line + const withBadge = formatComment({ content: "body", category: "bug", severity: "high" }, "ocr-1-1-abcd"); + assert.ok(withBadge.startsWith("\n"), "id HTML comment is the first bytes"); + assert.ok(withBadge.startsWith("\n[bug · high]\n"), "badge follows id line"); + assert.ok(withBadge.endsWith("body"), "content preserved at the end"); + // without id: badge is the first line + const noId = formatComment({ content: "body", category: "style", severity: "low" }); + assert.ok(noId.startsWith("[style · low]\n")); + // no metadata -> no badge line at all (byte-identical to pre-change output) + const noBadge = formatComment({ content: "body" }, "ocr-1-1-abcd"); + assert.strictEqual(noBadge, "\nbody"); + // suggestion block still appends after the badge + const withSuggestion = formatComment( + { content: "c", category: "bug", severity: "high", existing_code: "old", suggestion_code: "new" }, + "ocr-1-1-abcd" + ); + assert.match(withSuggestion, /\[bug · high\]/); + assert.match(withSuggestion, /\*\*Suggestion:\*\*/); + assert.match(withSuggestion, /```suggestion/); +} + +// I6: formatCommentMarkdown prepends the badge as a leading line before the +// path heading (PLAN_VALIDATION Risk A confirmed placement). +function testFormatCommentMarkdownBadgePlacement() { + const md = formatCommentMarkdown({ path: "a.js", content: "body", category: "bug", severity: "high" }); + // badge is the first line, before the heading + assert.ok(md.startsWith("[bug · high]\n"), "badge is the leading line"); + assert.match(md, /### 📄 `a.js`/); + assert.match(md, /body/); + // no metadata -> no badge line, heading is first (byte-identical to pre-change) + const noBadge = formatCommentMarkdown({ path: "a.js", content: "body" }); + assert.ok(noBadge.startsWith("### 📄 `a.js`")); + assert.ok(!noBadge.includes("·")); +} + +// I3: with no routing input set, placement is identical to today (modulo the +// additive badge prefix, which is "" for findings without metadata). Exercises +// the full runPostReviewComments path with default empty policy. +async function testNoRoutingInputPreservesBehavior() { + const result = { + comments: [ + { path: "src/a.js", content: "inline content", start_line: 10, end_line: 10 }, + { path: "docs/no-line.md", content: "no-line content", start_line: 0, end_line: 0 }, + ], + warnings: [], + }; + + const { github, outputs } = await run({ result }); + + // The inline comment is posted via the batch review (not routed). + assert.strictEqual(github.createReviewCalls.length, 1, "one batch review"); + const sent = github.createReviewCalls[0].comments; + assert.strictEqual(sent.length, 1); + assert.strictEqual(sent[0].path, "src/a.js"); + // no routed bucket (output defaults to 0) + assert.strictEqual(outputs.comments_routed, "0"); + assert.strictEqual(outputs.comments_inline, "1"); + assert.strictEqual(outputs.comments_total, "2"); +} + +// I2 + OC4: severity routing moves at-or-below findings to the summary and +// counts reconcile to the total. +async function testRouteSeverityBelowRoutesToSummary() { + const result = { + comments: [ + { path: "src/critical.js", content: "critical finding", category: "bug", severity: "critical", start_line: 1, end_line: 1 }, + { path: "src/low.js", content: "low finding", category: "style", severity: "low", start_line: 2, end_line: 2 }, + { path: "docs/no-line.md", content: "no-line finding", category: "documentation", severity: "low", start_line: 0, end_line: 0 }, + ], + warnings: [], + }; + + const { github, outputs } = await run({ + result, + opts: { routeSeverityBelow: "low" }, + }); + + // only the critical inline finding is posted (low is routed, no-line stays in summary) + assert.strictEqual(github.createReviewCalls.length, 1, "one batch review"); + const sent = github.createReviewCalls[0].comments; + assert.strictEqual(sent.length, 1, "only critical inline finding posted"); + assert.strictEqual(sent[0].path, "src/critical.js"); + // counts reconcile (I2): inline + routed + summary == total (skipped=failed=0) + assert.strictEqual(outputs.comments_total, "3"); + assert.strictEqual(outputs.comments_inline, "1"); + assert.strictEqual(outputs.comments_routed, "1"); + assert.strictEqual(outputs.comments_failed, "0"); + assert.strictEqual(outputs.comments_skipped, "0"); + // routed finding rendered in the summary with its reason + const body = github.updatedComments[0].body; + assert.match(body, /📋 Routed to summary by policy: 1 comment\(s\)/); + assert.match(body, /low finding/); + assert.match(body, /Routed to summary \(severity low/); + // no-line finding still rendered in summary too + assert.match(body, /no-line finding/); +} + +// OC4: comma-list category routing moves listed categories to the summary. +async function testRouteCategoriesRoutesToSummary() { + const result = { + comments: [ + { path: "src/bug.js", content: "bug finding", category: "bug", severity: "high", start_line: 1, end_line: 1 }, + { path: "src/style.js", content: "style finding", category: "style", severity: "low", start_line: 2, end_line: 2 }, + { path: "docs/doc.md", content: "doc finding", category: "documentation", severity: "low", start_line: 3, end_line: 3 }, + ], + warnings: [], + }; + + const { github, outputs } = await run({ + result, + opts: { routeCategories: "style,documentation" }, + }); + + // only the bug finding is posted inline; style + documentation routed + assert.strictEqual(github.createReviewCalls.length, 1); + const sent = github.createReviewCalls[0].comments; + assert.strictEqual(sent.length, 1); + assert.strictEqual(sent[0].path, "src/bug.js"); + assert.strictEqual(outputs.comments_inline, "1"); + assert.strictEqual(outputs.comments_routed, "2"); + assert.strictEqual(outputs.comments_total, "3"); + const body = github.updatedComments[0].body; + assert.match(body, /📋 Routed to summary by policy: 2 comment\(s\)/); + assert.match(body, /style finding/); + assert.match(body, /doc finding/); +} + +// I4: routed findings never enter the createReview write path, so they cannot +// be double-posted on retry. Verified by inspecting createReviewCalls bodies. +async function testRoutedFindingsNeverCallCreateReview() { + const result = { + comments: [ + { path: "src/keep.js", content: "keep inline", category: "bug", severity: "critical", start_line: 1, end_line: 1 }, + { path: "src/route.js", content: "route me", category: "style", severity: "low", start_line: 2, end_line: 2 }, + ], + warnings: [], + }; + + const { github, outputs } = await run({ + result, + // Inject a batch error so the per-comment retry path runs; routed findings + // must STILL not appear in any createReview call (batch or per-comment). + githubOpts: { + bulkErrorSpec: { message: "Bad Gateway", status: 502 }, + // No batchLanded / echoPosted -> full retry of toSend (the non-routed set). + }, + opts: { routeCategories: "style" }, + }); + + // Every createReview call must contain ONLY the kept finding's path, never + // the routed one. This is the faithful proxy for "cannot be double-posted": + // a finding absent from every write call cannot land twice. + for (const call of github.createReviewCalls) { + const paths = (call.comments || []).map((c) => c.path); + assert.ok(!paths.includes("src/route.js"), `routed finding appeared in createReview call: ${JSON.stringify(paths)}`); + assert.ok(paths.includes("src/keep.js"), `kept finding missing from createReview call: ${JSON.stringify(paths)}`); + } + assert.strictEqual(outputs.comments_routed, "1"); + // at least the batch + one per-comment retry happened + assert.ok(github.createReviewCalls.length >= 2, "batch then per-comment retry ran"); +} + +// I2: accounting reconciles across mixed inputs — +// inline + summary + skipped + failed + routed == total. +async function testAccountingReconcilesToTotal() { + const history = [{ path: "src/overlap.js", line: 5, start_line: 5, side: "RIGHT", user: { login: "github-actions[bot]" } }]; + const result = { + comments: [ + // routed by severity (low) + { path: "src/routed.js", content: "routed", category: "style", severity: "low", start_line: 1, end_line: 1 }, + // inline (posted successfully via per-comment retry) + { path: "src/inline.js", content: "inline", category: "bug", severity: "high", start_line: 2, end_line: 2 }, + // skipped by incremental overlap + { path: "src/overlap.js", content: "overlap", category: "bug", severity: "high", start_line: 5, end_line: 5 }, + // failed to post (422 non-retryable during per-comment retry) + { path: "src/fail.js", content: "fail", category: "bug", severity: "high", start_line: 3, end_line: 3 }, + // no-line (summary) + { path: "docs/noline.md", content: "no-line", category: "documentation", severity: "low", start_line: 0, end_line: 0 }, + ], + warnings: [], + }; + + const { outputs } = await run({ + result, + githubOpts: { + history, + // A 502 on the BATCH call forces the per-comment retry path, where + // perCommentError actually fires (it only runs on per-comment calls). + bulkErrorSpec: { message: "Bad Gateway", status: 502 }, + perCommentError: (rc) => { + if (rc && rc.path === "src/fail.js") { + return { message: "Line could not be resolved", status: 422 }; + } + return null; + }, + }, + opts: { incremental: true, routeSeverityBelow: "low" }, + }); + + const total = Number(outputs.comments_total); + const inline = Number(outputs.comments_inline); + const summary = 1; // the no-line finding (always summary) + const skipped = Number(outputs.comments_skipped); + const routed = Number(outputs.comments_routed); + const failed = Number(outputs.comments_failed); + assert.strictEqual(inline + summary + skipped + routed + failed, total, "counts sum to total (I2)"); + assert.strictEqual(total, 5); + assert.strictEqual(routed, 1, "low-severity valid-line finding routed"); + assert.strictEqual(inline, 1, "high-severity finding posted inline"); + assert.strictEqual(skipped, 1, "overlap skipped by incremental"); + assert.strictEqual(failed, 1, "fail.js failed to post"); +} + +// I1 / fail-open for the policy itself: malformed routing inputs degrade to +// no-routing, so the Action behaves exactly like today (no finding dropped +// because the policy string was garbage). +async function testMalformedRoutingPolicyFailsOpen() { + const result = { + comments: [ + { path: "src/a.js", content: "a", category: "bug", severity: "low", start_line: 1, end_line: 1 }, + { path: "src/b.js", content: "b", category: "style", severity: "high", start_line: 2, end_line: 2 }, + ], + warnings: [], + }; + + // unknown severity threshold -> no routing; both stay inline + const { outputs } = await run({ + result, + opts: { routeSeverityBelow: "trivial" }, + }); + assert.strictEqual(outputs.comments_routed, "0"); + assert.strictEqual(outputs.comments_inline, "2"); + // all-unknown categories -> no routing + const { outputs: o2 } = await run({ + result, + opts: { routeCategories: "nonsense,garbage" }, + }); + assert.strictEqual(o2.comments_routed, "0"); + assert.strictEqual(o2.comments_inline, "2"); +} + +// I4: routed findings carry no id (formatComment called without an id arg), +// so even a hypothetical leak into a write path could not match the +// idempotency regex. Defense-in-depth check on the routed item shape. +async function testRoutedFindingsCarryNoIdempotencyId() { + const result = { + comments: [ + { path: "src/route.js", content: "route me", category: "style", severity: "low", start_line: 2, end_line: 2 }, + ], + warnings: [], + }; + + const { github } = await run({ + result, + opts: { routeCategories: "style" }, + }); + // No createReview call at all (the only finding was routed). + assert.strictEqual(github.createReviewCalls.length, 0); + // The routed body in the summary must NOT contain an ocr-... id comment. + const body = github.updatedComments[0].body; + assert.doesNotMatch(body, /ocr-\d+-\d+-[a-f0-9]+/, "routed summary body carries no idempotency id"); +} + async function main() { await testFailedInlineCommentsAreSummarized(); await testWarningsListedAfterSummaryComments(); @@ -1660,6 +2104,22 @@ async function main() { await testBatchReconcileUnavailableStopsVisibly(); await testBatchSizeInvalidFallsBackToDefault(); await testBatchTelemetryOutputs(); + // Badge + publication policy (#478) + testBuildBadgeMatchesCliDegeneration(); + testSanitizeMetadataStripsControlChars(); + testBuildPolicyFailsOpenOnMalformed(); + testRouteCommentUnknownMetadataNeverRouted(); + testRouteSeverityBelowBoundaryInclusive(); + await testFindingMatchingBothConditionsRoutesOnce(); + testFormatCommentBadgePlacement(); + testFormatCommentMarkdownBadgePlacement(); + await testNoRoutingInputPreservesBehavior(); + await testRouteSeverityBelowRoutesToSummary(); + await testRouteCategoriesRoutesToSummary(); + await testRoutedFindingsNeverCallCreateReview(); + await testAccountingReconcilesToTotal(); + await testMalformedRoutingPolicyFailsOpen(); + await testRoutedFindingsCarryNoIdempotencyId(); console.log("All post-review-comments tests passed."); }