Skip to content

fix(review): headline a held PR as held, not Closed, when the disposition won't close it - #1412

Merged
JSONbored merged 1 commit into
mainfrom
claude/comment-disposition-wording
Jun 26, 2026
Merged

fix(review): headline a held PR as held, not Closed, when the disposition won't close it#1412
JSONbored merged 1 commit into
mainfrom
claude/comment-disposition-wording

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

The unified PR comment derived its headline from the gate verdict, so a close verdict always rendered "Closed" — even when the disposition would NOT actually close the PR. Two cases mislabel the action (audit #8/#9, the #4220 comment↔action class):

The fix threads the disposition's held-vs-closed reality into deriveUnifiedStatus:

  • neverClosed (author is the repo owner or a protected automation bot) → a close verdict renders held.
  • a guarded-path close (heldForReview) renders held unless ciState === "failed" — which mirrors the planner's redVerifiedRequiredCi (agent-actions.ts:316), the only thing that closes a guarded PR.
  • a genuine contributor close (red required CI, or a non-guarded block) still headlines "Closed".

The disposition was already correct (it never closes these); only the comment text was wrong. This is the piece deferred from #1374, now done with the author + guard nuance modeled so a red-CI guardrail PR that does close is not mislabeled.

No GitHub issue — internal review-subsystem audit finding (#8/#9).

Scope

  • Backend (src/) only — unified-comment.ts, unified-comment-bridge.ts, processors.ts
  • No API/schema, DB/migration, wrangler.jsonc, or UI change
  • Narrow, one coherent change

Validation

  • npm run test:ci — green (4470 passed | 4 skipped)
  • npm run test:coverage — every changed line and branch covered (verified against coverage/lcov.info)
  • npm audit --audit-level=moderate — 0 vulnerabilities; typecheck/ui:typecheck clean; git diff --check clean
  • New/updated tests: the full held-vs-closed matrix in deriveUnifiedStatus (owner/bot → held incl. red CI; guarded close → held unless red CI; genuine contributor close → Closed) and a bridge test that a gate-failure PR renders held under neverClosed. Corrected the prior test that asserted the old (buggy) "guarded close stays blocked".

Safety

  • No secrets / wallets / hotkeys / coldkeys / trust scores / reward values added
  • Comment-text-only change; the merge/close/hold action is unchanged
  • No public-surface term leakage

…tion won't close it

The unified comment derived its headline from the gate verdict, so a 'close'
verdict always rendered 'Closed' — even when the disposition would NOT close the
PR. Two cases mislabel: an owner / automation-bot PR (never auto-closed) and a
guarded-path PR (held for owner review unless a red required check forces the
close). The comment said 'Closed' while the action was a hold.

Thread the disposition's held-vs-closed reality into deriveUnifiedStatus:
 - neverClosed (author is repo owner or a protected automation bot) → a close
   verdict renders 'held'.
 - a guarded-path close (heldForReview) renders 'held' unless CI is failing —
   ciState 'failed' mirrors the planner's redVerifiedRequiredCi, which is the
   only thing that closes a guarded PR.
A genuine contributor close (red required CI, or a non-guarded block) still
headlines 'Closed'. Action-only change; the disposition was already correct.
@dosubot dosubot Bot added the size:S label Jun 26, 2026
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@JSONbored
JSONbored merged commit af6be76 into main Jun 26, 2026
16 checks passed
@JSONbored
JSONbored deleted the claude/comment-disposition-wording branch June 26, 2026 02:17
@codecov

codecov Bot commented Jun 26, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 71.42857% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.35%. Comparing base (9b69f3b) to head (55634ee).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/queue/processors.ts 33.33% 0 Missing and 2 partials ⚠️

❌ Your patch status has failed because the patch coverage (71.42%) is below the target coverage (97.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1412      +/-   ##
==========================================
- Coverage   95.36%   95.35%   -0.01%     
==========================================
  Files         192      192              
  Lines       20779    20786       +7     
  Branches     7512     7519       +7     
==========================================
+ Hits        19816    19821       +5     
  Misses        383      383              
- Partials      580      582       +2     
Files with missing lines Coverage Δ
src/review/unified-comment-bridge.ts 95.12% <100.00%> (+0.06%) ⬆️
src/review/unified-comment.ts 98.03% <100.00%> (+0.03%) ⬆️
src/queue/processors.ts 88.03% <33.33%> (-0.14%) ⬇️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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.

1 participant