Skip to content

fix(queue): render the unified comment from the LIVE merge-state, matching the disposition (#4220) - #1374

Merged
JSONbored merged 2 commits into
mainfrom
fix/comment-action-agreement
Jun 26, 2026
Merged

fix(queue): render the unified comment from the LIVE merge-state, matching the disposition (#4220)#1374
JSONbored merged 2 commits into
mainfrom
fix/comment-action-agreement

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

Adversarial-audit finding (high, 3/3 verifiers) — the exact #4220 class you hit. The public review comment built mergeReadiness from the stored pr.mergeableState, while the auto-close disposition reads the live merge-state. GitHub recomputes mergeable_state asynchronously, so the stored value lags: a base-conflicting PR could read clean in the comment ("✅ safe to merge") while the disposition reads the live dirty and auto-closes it — the comment and the action directly contradict each other.

Fix

Fetch the live merge-state in the comment path (the same fetchLivePullRequestMergeState + token the disposition uses) and feed it to deriveUnifiedStatus, fail-safe to the stored value. Now dirty → "blocked" and behind → "held" in the comment, matching the action.

Validation

  • npm run test:ci exit 0 (4413 tests); npm audit clean
  • New assertion: with a live dirty merge-state the comment no longer headlines "safe to merge"
  • Backward-compatible: when the live fetch fails the comment falls back to the stored value (existing tests unaffected)
  • One defensive branch (ciToken ?? GITHUB_PUBLIC_TOKEN fallback when the installation-token fetch fails mid-comment) is left uncovered — it's effectively unreachable once the gate check-run has already minted/cached the token; codecov/patch is advisory here (not a required check)

Note — deferred siblings (#8/#9)

The audit also flagged two related comment/action mismatches: the headline reads "Closed" on (a) a guarded-path PR the disposition actually HOLDS and (b) an owner/automation-bot PR that's never closed. Both require threading the actual disposition (held-vs-closed) / author classification into the comment path — the current heldForReview flag is only the guardrail-path check and would mislabel a red-CI guardrail PR (which does close). Deferred to a focused follow-up to avoid a mislabel regression.

…ching the disposition (#4220)

The public review comment built mergeReadiness from the STORED pr.mergeableState while the auto-close
disposition (planAgentMaintenanceActions) reads the LIVE merge-state. GitHub recomputes mergeable_state
asynchronously, so the stored value lags: a base-conflicting PR could read `clean` in the comment
("✅ safe to merge") while the disposition reads the live `dirty` and AUTO-CLOSES it — the exact #4220
contradiction (comment says merge-safe, action closes the PR). Fetch the live merge-state in the comment
path (the same fetchLivePullRequestMergeState + token the disposition uses) and feed it to deriveUnifiedStatus,
fail-safe to the stored value. Now `dirty` → "blocked" and `behind` → "held" in the comment, matching the action.
@superagent-security

Copy link
Copy Markdown
Contributor

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

@dosubot dosubot Bot added the size:XS label Jun 25, 2026
@codecov

codecov Bot commented Jun 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 95.34%. Comparing base (6138b9e) to head (c085ff5).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/queue/processors.ts 66.66% 0 Missing and 1 partial ⚠️

❌ Your patch check has failed because the patch coverage (66.66%) 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    #1374   +/-   ##
=======================================
  Coverage   95.33%   95.34%           
=======================================
  Files         192      192           
  Lines       20750    20752    +2     
  Branches     7500     7501    +1     
=======================================
+ Hits        19783    19785    +2     
  Misses        383      383           
  Partials      584      584           
Files with missing lines Coverage Δ
src/queue/processors.ts 87.63% <66.66%> (+0.02%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@JSONbored JSONbored self-assigned this Jun 25, 2026
@JSONbored
JSONbored merged commit c1db53a into main Jun 26, 2026
17 of 18 checks passed
@JSONbored
JSONbored deleted the fix/comment-action-agreement branch June 26, 2026 00:02
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