Skip to content

feat(action): add fail-open category/severity publication controls (#478) - #529

Open
nitishagar wants to merge 1 commit into
alibaba:mainfrom
nitishagar:feat/478-fail-open-publication-controls
Open

feat(action): add fail-open category/severity publication controls (#478)#529
nitishagar wants to merge 1 commit into
alibaba:mainfrom
nitishagar:feat/478-fail-open-publication-controls

Conversation

@nitishagar

Copy link
Copy Markdown
Contributor

Summary

First increment toward #478 (the full issue is a roadmap). Adds category/severity-aware, fail-open publication controls to the reusable GitHub Action:

  1. Render a CLI-consistent [category · severity] badge on every comment that carries metadata (inline and summary), matching buildBadge() in cmd/opencodereview/output.go.
  2. One opt-in routing destination: move low-severity and/or selected-category findings from inline comments to the PR summary, via two new action.yml inputs (route_severity_below, route_categories).
  3. Fail-open semantics: no finding is ever silently dropped — unknown/malformed metadata on a finding never matches the policy, and a malformed policy itself degrades to no-routing.
  4. Accounting reconciliation: a new routed bucket is disjoint from summary/skipped/failed, so destination counts still sum to the raw input total (inline + summary + skipped + failed + routed == total).

With no routing input set, behavior is byte-equivalent to today except for the additive badge prefix on comments that carry category/severity metadata. No Go changes — category/severity already serialize into the Action's input JSON via model.LlmComment.

Key design decisions (invariant → mechanism)

  • I1 (fail-open): routeComment requires the finding's own category/severity to be a known enum member before it can match; unknown metadata falls through to the normal inline path (visible). buildPolicy returns a NO_ROUTING sentinel on any malformed input.
  • I2 (accounting): new routed bucket + comments_routed output; the partition loop routes each finding into exactly one bucket via mutually-exclusive continue branches.
  • I3 (defaults): empty policy = no-op; the badge is an additive prefix that renders "" (no line) when metadata is absent, so metadata-less comments are byte-identical to today.
  • I4 (idempotency): routing is a placement decision in the partition loop — routed findings continue before reviewComments.push, so they never enter toSend/toRetry/either createReview call, and carry no idempotency id. No double-post surface on retry.
  • I6 (badge consistency): shared buildBadge byte-matches the CLI degeneration. Control-char sanitization is intentionally stricter than the CLI's sanitizeTerminal (strips \t/\n too) to defend the Markdown comment-body layout — a documented divergence that only manifests for malformed model output and is safer (clean enum values match exactly).

At-or-below severity semantics

route_severity_below=low routes only low; route_severity_below=medium routes medium AND low (inclusive at-or-below). The lowest tier is still routable, matching the spec's fail-open intent. Category routing is OR-combined with severity routing (a finding matching either condition routes exactly once).

Test coverage

New tests cover every invariant: badge degeneration + control-char sanitization (I6), fail-open on malformed policy/metadata (I1), accounting reconciliation across mixed buckets (I2), defaults-preserve-behavior (I3), routed findings never reach createReview + carry no idempotency id (I4), severity boundary inclusivity, both-conditions-routes-once, and end-to-end severity/category routing.

All existing tests pass unchanged (no existing fixture carries category/severity, so the additive badge is "" for them).

Validation

  • node scripts/github-actions/post-review-comments.test.js → exit 0 (existing + 15 new tests)
  • npm run test:github-actions → exit 0 (includes check-translation-sync.test.js)
  • node -c syntax check + action.yml YAML parse
  • Adversarial implementation + test review (PASS, round 2)
  • Security review (1 Medium finding re: Markdown injection dispositioned as pre-existing — comment.content is already rendered unsanitized; the badge introduces no new trust boundary. Noted for a separate repo-wide hardening pass.)
  • Manual: trace a 3-finding fixture ({critical inline, low inline, no-line}) with route_severity_below=low → expect critical inline, low summary (routed), no-line summary; counts total=3, inline=1, routed=1, summary=1, skipped=0, failed=0.

Closes #478 (first increment).

@github-actions

github-actions Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

🔍 OpenCodeReview found 1 issue(s) in this PR.

  • ✅ Successfully posted inline: 1 comment(s)

Comment on lines +1177 to +1178
const catRaw = comment && comment.category != null ? String(comment.category).trim().toLowerCase() : "";
const sevRaw = comment && comment.severity != null ? String(comment.severity).trim().toLowerCase() : "";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent metadata normalization between routeComment and buildBadge

routeComment normalizes metadata with String(...).trim().toLowerCase(), while buildBadge uses sanitizeMetadata(...) which strips control characters (\x00-\x1f, \x7f-\x9f) but does NOT trim or lowercase.

This creates a divergence: if model output contains an embedded control character (e.g., "bug\x01"), routeComment would see "bug\x01" and fail to match the policy (correct fail-open), but buildBadge would strip it to "bug" and render [bug] in the badge. The user sees a clean badge label but the finding wasn't routed despite matching the policy category.

Consider using sanitizeMetadata here as well (followed by .trim().toLowerCase()) so both functions agree on what constitutes a valid category/severity value:

const catRaw = sanitizeMetadata(comment && comment.category).trim().toLowerCase();
const sevRaw = sanitizeMetadata(comment && comment.severity).trim().toLowerCase();

This ensures consistent behavior: if the badge renders [bug], the routing policy also recognizes it as bug.

@lizhengfeng101 lizhengfeng101 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merge conflicts

…libaba#478)

Add category/severity-aware, fail-open publication controls to the reusable
GitHub Action: render a CLI-consistent `[category · severity]` badge on every
comment, and add one opt-in routing destination that moves low-severity or
selected-category findings from inline comments to the PR summary.

No finding is ever silently dropped: unknown/malformed metadata on a finding
never matches the policy (routes to its normal inline destination), and a
malformed policy itself degrades to no-routing. A new `routed` accounting
bucket is disjoint from summary/skipped/failed, so destination counts still
sum to the raw input total.

- buildBadge: byte-matches the CLI's buildBadge degeneration
  ([cat · sev] / [cat] / [sev] / ""), with control-char sanitization that is
  intentionally stricter than the CLI (strips \t/\n to defend Markdown layout).
- buildPolicy / routeComment: pure fail-open policy decision. A finding matches
  when its severity is at-or-below the threshold OR its category is in the list;
  unknown metadata never matches.
- Partition loop: routing is a placement decision (route OUT of reviewComments),
  so routed findings never enter any createReview write path (no double-post on
  retry) and carry no idempotency id.
- Accounting: new comments_routed output and summary bullet; render order is
  counts -> no-line -> routed -> failed.
- action.yml: opt-in route_severity_below and route_categories string inputs
  (empty defaults = today's behavior) plus the comments_routed output.

With no routing input set, behavior is byte-equivalent to today except for the
additive badge prefix on comments that carry category/severity metadata.
@nitishagar
nitishagar force-pushed the feat/478-fail-open-publication-controls branch from 2bd4d35 to ee47f20 Compare July 28, 2026 02:59
@nitishagar

Copy link
Copy Markdown
Contributor Author

@lizhengfeng101 Resolved conflicts. Please take a look when you get a chance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add fail-open, non-destructive finding publication controls to the reusable Action

2 participants