Skip to content

[grug] Sentinel escalates a low-severity finding to "high/critical" in its own notice, contradicting Elder's verdict in the same comment #775

Description

@quadseven

Why

Sentinel's abandoned-review notice on quadseven/infra#1998 said:

this PR was merged with its blocking check still failing while Elder's last
review on this commit was still blocking (1 finding(s), severity
high/critical
).

This means the finding(s) below may have SHIPPED - please verify.

Then, in the SAME comment:

Elder's last verdict summary

Elder markings - 0 blocking, 1 total

And the finding itself was tagged 👁 low | maintainability | dead-code.

So one comment simultaneously claims "0 blocking" and "still blocking ...
severity high/critical" about the same review.

Why it matters

Sentinel is doing genuinely valuable work here - it correctly caught that I
merged before reading the bot-comment result, which is a real process failure
worth flagging. That signal should stay loud.

But the severity wording is wrong in a way that costs trust:

  • A low maintainability nit is reported as a possible high/critical
    shipped incident.
  • The claim "blocking check still failing" contradicts 0 blocking from Elder,
    quoted three lines below it.
  • If low findings routinely read as high/critical, the honest response becomes
    skimming Sentinel - which defeats a guard whose entire value is being taken
    seriously.

This one happened to be a false positive too (Elder flagged a line the PR itself
deleted), so the notice read as "a high/critical finding may have shipped" for
something that was neither high, nor critical, nor real.

What

  • Derive the severity in the notice from the actual finding severities,
    not a fixed string.
  • Reconcile "still blocking" with Elder's own blocking count. If Elder says
    0 blocking, Sentinel should say the merge skipped a NON-blocking review,
    which is a different and much milder statement.
  • Consider tiering the notice: a merged-with-unanswered-low is worth a
    note; a merged-with-unanswered-critical is worth alarm. Same event, very
    different urgency.

Acceptance criteria

  • A PR merged with only low findings outstanding produces a notice that
    says low.
  • Sentinel's blocking language agrees with Elder's blocking count in the
    same comment.
  • The genuine signal - "you merged without answering the bots" - is NOT
    weakened. It was correct here and should keep firing.

Context

Filed from the same session as #771 (shell control-flow FP), #772
(move-awareness), #773 (Elder cancel/re-enqueue deadlock) and PR #774 (base-sha
churn cancelling in-flight reviews).

Size: S

Refs #707

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions