fix(ci): scope the posted-review check to bot comments - #212
Merged
Conversation
Two gaps in the guard that landed in #210, both found by review on #211. The comment scan had no author filter, so ANY comment carrying this run's id and the `Review mode:` marker satisfied it: a quote of the job URL, a paste of this workflow, an acknowledgement citing the review it answers. That is exactly the case the step exists to catch — the action posted nothing — being masked by someone talking about it. It now filters on `.user.type == "Bot"` before grepping. Filtering on type rather than on a login means an app rename does not fail every review closed, while the actual threat, a human comment, is excluded. Verified against the record: the dead run on #210 still reports not-posted, the real review on #211 still reports posted, and four human comments on #210 are now excluded from consideration. An execution file that exists but holds no result record was also no longer diagnosed. It used to be a hard error; once `posted` became the verdict it passed through as `turns=None is_error=None cost=None` with nothing naming the corruption, so a partial write or an action version skew would vanish rather than be noticed. It now warns, and deliberately does not fail, since it says nothing about whether a review was posted.
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
注册 for free
to join this conversation on GitHub.
Already have an account?
登录 to comment
1 participant
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.
Two gaps in the guard that landed in #210, both found by the review on #211 — the first review to run under the new guard, which is a reasonable sign it is working.
The comment scan had no author filter
postedwas decided by grepping every comment on the PR for this run's id and theReview mode:marker. Nothing checked who wrote the comment, so any comment carrying both satisfied it: a quote of the job URL, a paste of this workflow file, an acknowledgement citing the review it answers.That is precisely the case the step exists to catch — the action posted nothing — being masked by someone talking about it.
Now filtered on
.user.type == "Bot"before the grep. Filtering on type rather than on a login is deliberate: an app rename would otherwise fail every review closed, while the actual threat here is a human comment, which type excludes. A different bot would still have to reproduce this run's id and the marker to matter.Verified against the record, not just reasoned:
33263251357on #21033263651631on #211A corrupt execution file was no longer diagnosed
An execution file that exists but holds no parseable
resultused to be a hard error. Oncepostedbecame the verdict, it passed through asturns=None is_error=None cost=Nonewith nothing naming the corruption — so a partial write or an action version skew would disappear instead of being noticed.It now emits a
::warning::, and deliberately does not fail: it says nothing about whether a review was posted, which is the question this step answers.Tested by extracting the guard and running it: corrupt-file-present warns and exits 0, file-absent warns and exits 0, and a run that posted nothing still exits 1.
Not changed, and why
The review also raised that coupling the gate to the prompt's
Review mode:wording is fragile, since there is no compiler link between the two files. That is a fair characterisation and the workflow already says so in the comment above the check. The alternative it suggests — having the action emit a structured marker — is not available to us: the marker has to come from something we control, and the action's own output format is not. The prose contract is the strongest signal on offer, and it is documented as a contract rather than left implicit.Notes