Align the Action's review output with the hosted reviewer - #35
Merged
Conversation
Four differences a user hits when the same repository is reviewed by both. THE SUMMARY IGNORED report_on. The pinned PR description was fed the raw result on purpose, so the counts would be "true" — and a workspace on report_on=P0,P1 got two P1s inline with a description above them announcing P2s and P3s that appear nowhere in the review. That reads as honesty and lands as a contradiction, and the surface the reader can act on is the one they were shown. It now reads result-reported.json, the same file the inline comments come from. The gate and the run report still read the raw result: those MEASURE and ENFORCE, and a display preference must never move them. THE 👀 WAS NEVER CLEARED. The Action places it to say "I am looking at this" and had no delete anywhere, so every pull request it has ever touched wears one for ever and an operator cannot tell an in-flight run from a finished one. A reaction nobody removes carries no information. Cleared on always(), because a failed run is the case where a stale pair of eyes is worst — it promises a review that is not coming — and only the reaction posted by this token is removed, so a maintainer's own 👀 survives. FINDINGS HAD NO TITLE. Both renderers already split a bold lead from the body; the hosted reviewer's model writes one and the Action's did not, so its findings arrived as one long paragraph that has to be read in full before it can be triaged. This is an instruction, not a renderer change: the rubric now asks for a short bold title, which is the shape the existing split already looks for. THE FOOTER SAID SOMETHING ELSE. Four occurrences of "Reviewed via [OrcaRouter]" now match the hosted footer's branding line. The receipt line and the share block stay hosted-only for now — both need usage figures plumbed into the notice paths, which is a separate change.
…ops being true The Action returned silently when it found nothing, leaving the pinned summary to report it. That summary lives in the PR description — folded, and above the fold only if you scroll — so somebody who asked for a review got no visible answer. It now posts the same sentence the hosted reviewer posts; two paths reviewing one repository should not disagree about what "nothing to flag" looks like. That reintroduces the timeline entry per push the silence was avoiding, which is what the reaction is for: the entry states the result once, and the 👍 is the at-a-glance version that updates in place. SETTLED, NOT PLACED. GitHub reactions have no update — add and remove are all there is — so a marker that is only ever added drifts: a head that was clean keeps its thumb after a later push breaks something, and the one question it exists to answer stops being answerable. A run with findings now removes it. Matched BY LOGIN, and the first version was wrong in a way that only shows up where it matters. It filtered on user.type === 'Bot', so a repository reviewed by both this Action and the hosted App has two bots reacting — and each would have deleted the other's 👍 on every run. getAuthenticated() answers for a PAT or an App token; the default GITHUB_TOKEN 401s there and its reactions carry github-actions[bot], so that is the fallback rather than widening the match.
Contributor
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe8e20abc3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
THE BOLD TITLE MADE A LATENT BUG UNIVERSAL. The bold-lead branch stripped every `**` from the explanation, which was survivable while only some findings reached it — the rubric now asks for a title on all of them, so every explanation would pass through. A finding about `src/**/*.js` came out as `src//*.js` and `x ** 2` as `x 2`. Only the title's own markup is removed now. THE OUTPUT SHAPE WAS INSIDE THE REPLACEABLE FILE. A dashboard rubric is copied OVER rules/severity-instruction.md, so the title directive reached only workspaces that had not customised their rubric — the ones least likely to notice the untitled output it exists to prevent. It lives in rules/output-shape.md now and is appended in both branches: the rubric decides WHICH severity a finding gets, the shape is how every finding is written down, and the renderer splits on it. THE 👀 CLEAR MATCHED BY IDENTITY. Under a PAT that selects the maintainer's OWN reaction, including one they added by hand before the run; with two overlapping runs, each cleared the other's acknowledgement mid-review. The POST now records the id it created — idempotent per (user, content), so a re-run records the id it legitimately owns — and only those ids are deleted. THE QUIET NOTE EXPLAINED A DIFFERENCE THAT NO LONGER EXISTED. "quiet mode: P2 shown in summary only" is true while the summary carries P2s the inline comments do not. With the summary now reading the report_on-filtered file, a workspace excluding P2 there had the note sitting under a zero. The flag is passed only when the summary still holds a P2. Verified: action.yml parses (22 steps), both embedded scripts pass node --check under the async wrapper github-script applies, the shape reaches the model in BOTH rubric branches (simulated), and the asterisk case is shown above.
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b243fca72b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Four findings, and the shape of them matters more than any one: rounds 2 and 3 on this branch were both entirely about the reaction code added in round 1. Narrowing the predicate once more would have earned a round 4, so this changes the design instead. ONE OWNER. The 👍 was settled inside the posting step, which is one ending out of several — a settings skip, an oversized diff, a guardrail block, an engine failure or a rejected post all finish without going near it, and each left a thumb endorsing a head nobody reviewed. Both reactions are now settled in the always() step, which sees every ending. The posting step only records its verdict. DECLINED UNDER A PAT. GitHub cannot be asked "which reaction did I create", so the only handle is the author — and when github-token is a maintainer's PAT, their manual 👍 and ours have the same author. Two rounds went into narrowing that predicate; at the boundary the honest answer is to leave reactions alone rather than guess and delete somebody's approval. The review body and the summary still say everything the thumb would have. ONLY WHAT WE CREATED. The 👀 POST is idempotent per (user, content): an existing reaction comes back 200 with its id, only a fresh one is 201. The previous version recorded either, so a maintainer's own 👀 was adopted and then deleted by a run that never placed it — and one of two overlapping runs cleared the other's acknowledgement mid-review. Recorded on 201 only. CLEAN IS DECIDED BEFORE QUIET. `comments` is the quiet-filtered list, so a quiet workspace whose findings were all P2 reached the clean branch empty and posted "No findings" with a 👍 over a summary that listed them three lines above. The verdict reads the report_on-filtered file: quiet decides what is POSTED, never whether the review FOUND anything. Verified: action.yml parses (22 steps), both embedded shell steps pass bash -n, and the 201-vs-200 parse was exercised against both response shapes.
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0ec2999b6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Fourth review round on this branch, and every finding in the last three has been in the reaction code added in the first. Each fix earned the next: an identity match, then recorded ids, then a status code to decide which ids to record, then a JSON parse for the id — and that parse was a greedy expression that picked the nested author id out of a compact response, so the DELETE would have hit the wrong resource. All of it existed to tell our reaction from the token owner's, which GitHub cannot answer after the fact. So the question is declined instead: ONE gate, covering both reactions — when github-token belongs to a user rather than an app, this leaves reactions alone entirely. The id file, the 201/200 distinction and the hand-rolled parse are deleted with it. Node is not available at the react step, so there was no good parse to reach for anyway. Accepted with it: two concurrent runs as the same app share one reaction, so the first to finish clears it while the second still works. A missing 👀 for a few minutes is cosmetic, and every mechanism that avoided it cost more correctness than it bought. MUTED IS NOT CLEAN, AND NOT A REVIEW. Deciding "found anything" from the pre-quiet file fixed one contradiction and created another: a quiet run whose findings were all P2 had foundCount > 0 and an empty posted list, fell through, and built a review from an empty array — "Found **0** issues in this PR: ." — beside a summary listing the muted findings. It is a third state, and it now posts nothing and records no clean verdict. THE CLEAN MARKER IS WRITTEN AFTER PUBLISHING SUCCEEDS. Written first, a run whose review and its comment fallback both failed still told the always() step it was clean, and the 👍 endorsed a verdict nobody ever saw. AND IT IS PER-RUN STATE. $RUNNER_TEMP survives between jobs on a self-hosted runner, so the marker is removed by both the startup and the final cleanup lists — otherwise one clean run leaves every later run on that runner claiming to be clean. Verified: parses, both shell steps pass bash -n, the posting script passes node --check under github-script's async wrapper, the three-way decision is exercised over all four combinations, and the settle step still runs before the cleanup that removes the marker it reads.
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6322363c5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
GATING ONLY THE REMOVAL WAS WORSE THAN NOT GATING AT ALL. Last round taught the settle step to leave reactions alone under a PAT, and left the POST unconditional — so a PAT-backed run put 👀 on the pull request and then declined to take it off. Declining is only coherent if it covers both ends; the same check now runs before the POST. THE THUMB BELONGS TO A HEAD. Nothing serialises invocations, so an older clean run can finish after a newer findings/skipped/failed run has withdrawn the thumb, and its local marker would put it back over a head it never saw. It compares the PR's current head against the one it reviewed, and an unconfirmed head counts as moved: adding on a failed lookup would contradict the rule the check exists for, and "could not tell" is not evidence. Withdrawal deliberately has no such guard. A thumb removed by a stale run is re-added by the current one when it completes, so that direction fails toward a moment without a 👍 rather than a 👍 over unreviewed code. Verified: parses, both shell steps pass bash -n, and the guard is exercised over same/moved/unknown head for both verdicts.
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50ac6b9a34
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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
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.
Five differences a user hits when the same repository is reviewed by both paths.
1. The summary ignored
report_onThe pinned PR description was fed the raw result on purpose, so its counts would be "true". A workspace on
report_on=P0,P1therefore saw two P1s inline and a description above them announcing P2s and P3s that appear nowhere in the review.That reads as honesty and lands as a contradiction — and the surface a reader can act on is the one they were shown. It now reads
result-reported.json, the same file the inline comments come from.The gate and the run report still read the raw result. Those measure and enforce; a display preference must never move them.
2. The 👀 was never cleared
Placed to say "I am looking at this", with no
reactions.deleteanywhere — so every pull request the Action has ever touched wears one for ever and an operator cannot tell an in-flight run from a finished one.Cleared on
always(), because a failed run is where a stale pair of eyes is worst: it promises a review that is not coming.3. A clean run said nothing
It returned silently and left the summary to report it. That summary is in the PR description — folded, and above the fold only if you scroll — so somebody who asked for a review got no visible answer. It now posts the same sentence the hosted reviewer posts.
4. There was no 👍, and reactions do not update
GitHub has add and remove, nothing else, so a marker that is only ever placed drifts: a head that was clean keeps its thumb after a later push breaks something. It is settled on every run — added when clean, removed when findings exist.
Matched by login. The first version filtered on
user.type === 'Bot', which is wrong exactly where it matters: a repository reviewed by both this Action and the hosted App has two bots reacting, and each would have deleted the other's 👍 every run.getAuthenticated()answers for a PAT or an App token; the defaultGITHUB_TOKEN401s there and its reactions carrygithub-actions[bot], so that is the fallback rather than widening the match.5. Findings had no title
Both renderers already split a bold lead from the body. The hosted reviewer's model writes one; this one did not, so findings arrived as one long paragraph that has to be read in full before it can be triaged.
An instruction, not a renderer change: the rubric asks for a short bold title, which is the shape the existing split already looks for.
Also
Four occurrences of
Reviewed via [OrcaRouter]now match the hosted footer's branding line.The receipt line (
Engine-reported: …) and the share block stay hosted-only — both need usage figures plumbed into the notice paths, which is a separate change.🤖 Generated with Claude Code