fix(settings): require issues:write for detected comment mode independent of sample - #468
Merged
JSONbored merged 5 commits intoJun 8, 2026
Conversation
…dent of sample writesPrPublicSurface checked comment capability with shouldPublishPrComment(settings) (default minerStatus not_checked), so for the default detected_contributors_only comment mode it returned false unless the previewed sample was a confirmed miner -- dropping issues:write from required/missing install permissions for non-confirmed samples even though the repo comments for confirmed miners. Check with the maximal confirmed status, matching the label path.
|
Note Gittensory Gate skippedPR closed before full evaluation. No late first comment was created.
Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers. |
JSONbored
previously approved these changes
Jun 8, 2026
JSONbored
left a comment
Owner
There was a problem hiding this comment.
@philluiz2323 this is ready from my side.
A few notes:
- The fix correctly evaluates comment capability with the maximal confirmed status, matching the label path.
- The regression test covers the default
detected_contributors_onlymode and the non-confirmed sample case that previously hidissues: write. - No code changes requested.
Owner
|
Fix failing CI, please @philluiz2323 |
# Conflicts: # scripts/actionlint.mjs
Contributor
Author
Fixed. |
JSONbored
approved these changes
Jun 8, 2026
JSONbored
left a comment
Owner
There was a problem hiding this comment.
@philluiz2323 this is ready from my side.
A few notes:
- The fix correctly evaluates comment capability with the maximal confirmed status, matching the label path.
- The regression test covers the default
detected_contributors_onlymode and the non-confirmed sample case that previously hidissues: write. - The previous validate blocker is cleared on the current head; no code changes requested.
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #467.
writesPrPublicSurface(which gates whether the install preview requiresissues: write) checked label capability maximally withshouldApplyPrLabel(settings, "confirmed"), but checked comment capability withshouldPublishPrComment(settings)— whose miner-status arg defaults to"not_checked". ForcommentMode: "detected_contributors_only"(the DB/API/schema default) that returnsfalse, so when a maintainer previews anot_found/unavailablesample the function concluded the repo doesn't comment and droppedissues: writefrom required/missing permissions — even though the repo does comment for confirmed miners.Required permissions are meant to be sample-independent (that's the purpose of the
|| shouldPublishPrComment(settings) || shouldApplyPrLabel(settings, "confirmed")clauses); this asymmetry made the comment permission flicker with the sample's miner status while the label permission stayed correct.Change
"confirmed"status, matching the label path.detected_contributors_only+comment_only, non-confirmed sample →issues: writestays required and a missingissuespermission is surfaced.Verification
issues: write).Distinct from #419/#420/#433/#436 (those were
pull_requests: writeover-privilege; this isissues: writeunder-reporting).