Skip to content

review(unified-comment): fix comments that overclaim what review.comment_verbosity: quiet drops #10330

Description

@JSONbored

⚠️ Definition of Done: this issue must be completed in full, in a single PR. Do not split this
work across multiple PRs, and do not defer any Deliverable below to a follow-up issue. A PR that
satisfies only some of the Deliverables, stubs a required test, or leaves a checkbox
partially-done does NOT resolve this issue and will be closed.

Context

src/review/unified-comment.ts documents review.comment_verbosity: quiet's effect on the rendered PR
comment in two places, and both overclaim what quiet actually leaves untouched.

The UnifiedReviewInput.commentVerbosity field doc:

/** `review.comment_verbosity`: how much of the comment's collapsible detail renders. `quiet` drops the
 *  Nits collapsible and every `extraCollapsibles` section entirely (blockers/gate result/signals always
 *  stay — this only trims decorative detail, never the merge/close-relevant signal); `detailed` renders
 *  every collapsible pre-expanded (`<details open>`) instead of collapsed. `normal`/undefined (default) ⇒
 *  byte-identical to today. (#2047) */

An inline comment in the comment-building function itself:

// #6067: the old always-rendered 9-row table is split into an always-visible "Decision drivers" list
// (only rows that can move the verdict) and a collapsed "Context & advisory signals" fold (everything
// else). Like the table it replaces, NEITHER is gated by `review.comment_verbosity: quiet` -- these are
// gate-relevant/context signals, not decorative detail (matches the file's existing verbosity contract:
// only Nits + extraCollapsibles are ever dropped by `quiet`).

Both comments assert quiet drops only the Nits collapsible and extraCollapsibles sections. That claim
is directly contradicted a few lines below the second comment, in the same function, where THREE more
sections are explicitly gated on verbosity !== "quiet":

if (nonRequiredFailingChecks && verbosity !== "quiet") {
  blocks.push(details("Flagged checks (non-blocking)", nonRequiredFailingChecks, undefined, collapsiblesOpen));
}
// ...
if (satisfactionBody && verbosity !== "quiet") {
  blocks.push(details("Linked issue satisfaction", satisfactionBody, undefined, collapsiblesOpen));
}
// ...
if (thresholdBacktestBody && verbosity !== "quiet") {
  blocks.push(details("Threshold backtest", thresholdBacktestBody, "never blocks the verdict", collapsiblesOpen));
}

This is confirmed intentional, tested behavior (test/unit/unified-comment.test.ts around line 468 asserts
on quiet dropping these sections) — the comments were simply never updated as each of these three sections
was added over time (#4414-class advisory holds, #2174 linked-issue satisfaction, #8138 threshold
backtest), each one gated on quiet at the moment it was introduced without the earlier "only Nits +
extraCollapsibles" prose being revisited.

Requirements

  • Both stale comments must be corrected to accurately list every section review.comment_verbosity: quiet
    actually drops today: the Nits collapsible, extraCollapsibles, "Flagged checks (non-blocking)", "Linked
    issue satisfaction", and "Threshold backtest".
  • This is a comment-only fix — no change to which sections are actually gated on quiet, no change to the
    rendering logic itself, and no change to test/unit/unified-comment.test.ts's existing assertions (they
    already correctly test the real behavior; only the prose describing it is wrong).

Deliverables

  • UnifiedReviewInput.commentVerbosity's field doc comment lists all five sections quiet drops
    (Nits, extraCollapsibles, Flagged checks (non-blocking), Linked issue satisfaction, Threshold
    backtest) instead of claiming only two.
  • The inline #6067 comment above decisionDriverBlock/advisorySignalsTable no longer claims "only
    Nits + extraCollapsibles are ever dropped by quiet" — it's corrected to match the actual current set
    of five sections, or reworded to avoid enumerating a list that has already gone stale twice (e.g.
    pointing a future reader to grep for verbosity !== "quiet" as the authoritative list instead of
    restating it in prose).

Both Deliverables are required in the same PR.

Test Coverage Requirements

This is a comment-only change with no executable-code diff, so it does not add any new coverable lines/
branches under src/** and the Codecov patch gate has nothing new to measure here. Run the existing
test/unit/unified-comment.test.ts suite locally (including its quiet-verbosity assertions around line
468) to confirm no behavior changed.

Expected Outcome

A future reader of src/review/unified-comment.ts's review.comment_verbosity: quiet documentation gets an
accurate picture of everything quiet currently drops, instead of a stale list that undercounts by three
sections.

Links & Resources

  • src/review/unified-comment.ts — the field doc (around lines 329-334) and the inline #6067 comment
    (around lines 911-915), plus the three verbosity !== "quiet" gates that contradict them (around lines
    907, 926, 934).
  • test/unit/unified-comment.test.ts (around line 468) — the existing test already correctly covering the
    real behavior.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    gittensor:bugGittensor-scored bug fix — scores a 0.05x multiplier.help wantedExtra attention is needed

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions