From fe8e20abc3636d8f43f730a16b2f03c2d6f28301 Mon Sep 17 00:00:00 2001 From: ZhenghuaBao Date: Wed, 26 Aug 2026 16:36:23 +0800 Subject: [PATCH 1/6] Align the Action's review output with the hosted reviewer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- action.yml | 82 ++++++++++++++++++++++++++++++----- rules/severity-instruction.md | 8 ++++ 2 files changed, 79 insertions(+), 11 deletions(-) diff --git a/action.yml b/action.yml index 5c2de50..eba8637 100644 --- a/action.yml +++ b/action.yml @@ -485,7 +485,7 @@ runs: // Own marker (distinct from the summary and oversized-skip ones). const MARKER = ''; const footer = - `\n\nReviewed via [OrcaRouter](https://orcarouter.ai) โ€” ` + + `\n\n[OrcaCode Review](https://www.orcarouter.ai/code-review) โ€” ` + `Route Smarter. Ship Safer. Spend Less.`; const body = `${MARKER}\n## ๐Ÿณ ${brand}\n\n` + @@ -597,7 +597,7 @@ runs: // deletes this notice). const MARKER = ''; const footer = - `\n\nReviewed via [OrcaRouter](https://orcarouter.ai) โ€” ` + + `\n\n[OrcaCode Review](https://www.orcarouter.ai/code-review) โ€” ` + `Route Smarter. Ship Safer. Spend Less.`; const outcome = pass ? `and the check **passes** (\`on-oversized-diff: "pass"\`), so this skip does not block your merge.` @@ -1049,7 +1049,7 @@ runs: ? ` ยท request id \`${String(block.requestId).slice(0, 80)}\`` : ''; const footer = - `\n\nReviewed via [OrcaRouter](https://orcarouter.ai) โ€” ` + + `\n\n[OrcaCode Review](https://www.orcarouter.ai/code-review) โ€” ` + `Route Smarter. Ship Safer. Spend Less.${idTag}`; const body = `${MARKER}\n## ๐Ÿณ ${brand}\n\n` + @@ -1091,10 +1091,13 @@ runs: # there anything left to say" and after a narrowed report_on there can be # nothing left to say while the raw result is full of findings. # - # The gate, the summary counts and the run report keep reading $RESULT โ€” the - # unfiltered file. report_on changes what lands on the PR, never what is - # enforced or measured; the union with block_on inside the script is what stops - # a display preference from hiding a merge blocker. + # The gate and the run report keep reading $RESULT โ€” the unfiltered file. + # report_on changes what lands on the PR, never what is enforced or measured; + # the union with block_on inside the script is what stops a display preference + # from hiding a merge blocker. + # + # THE SUMMARY IS DISPLAY, so it reads the filtered copy โ€” it did not, and the + # pinned description contradicted the review three lines below it. - name: report_on severity filter if: steps.settings.outputs.decision != 'skip' && steps.guard.outputs.decision != 'skip' shell: bash @@ -1166,7 +1169,7 @@ runs: // Attribution for the Apache-2.0 engine lives in NOTICE + README, not // in every comment โ€” OCR is consumed as a package, not redistributed. const footer = - `\n\nReviewed via [OrcaRouter](https://orcarouter.ai) โ€” ` + + `\n\n[OrcaCode Review](https://www.orcarouter.ai/code-review) โ€” ` + `Route Smarter. Ship Safer. Spend Less.`; if (comments.length === 0) { @@ -1265,9 +1268,19 @@ runs: uses: actions/github-script@v7 env: PR_NUMBER: ${{ steps.pr.outputs.number }} - # Deliberately the UNfiltered result: the summary shows TRUE counts - # (its quiet-mode note explains why fewer P2s appear inline). - RESULT: ${{ runner.temp }}/result.json + # THE FILTERED COPY, the same one the inline comments come from. + # + # This used to be the raw result, deliberately, so the summary could show + # "true counts". That reads as honesty and lands as a contradiction: a + # workspace on report_on=P0,P1 saw two P1s inline and a description + # pinned above them announcing P2s and P3s that appear nowhere in the + # review. The reader has no way to tell which surface to believe, and + # the one they can act on is the one they were shown. + # + # Two surfaces of one review must not disagree. The gate and the run + # report still read $RESULT โ€” those MEASURE and ENFORCE, and a display + # preference must never move them. This one is display. + RESULT: ${{ runner.temp }}/result-reported.json PREV_BODY: ${{ runner.temp }}/prev-summary.md SUMMARY_SCRIPT: ${{ github.action_path }}/scripts/summary-comment.mjs INJECT_SCRIPT: ${{ github.action_path }}/scripts/inject-summary.mjs @@ -1397,6 +1410,53 @@ runs: --url "$ORCAROUTER_URL" \ --engine-version "$ENGINE_VERSION" + # The ๐Ÿ‘€ said "I am looking at this". Once the review has been posted โ€” or + # has failed and said so โ€” that is no longer true, and a reaction nobody + # removes stops carrying information: every PR the reviewer has ever touched + # wears one for ever, so the operator cannot tell an in-flight run from a + # finished one. Placing it and leaving it is the same as not placing it. + # + # always(), because a FAILED run has to clear it too. That is the case where a + # stale pair of eyes is worst: it promises a review that is not coming. + # + # Only OUR OWN reaction. The endpoint deletes by id, so this lists first and + # removes the one whose user matches the token's identity โ€” a maintainer who + # also reacted ๐Ÿ‘€ keeps theirs. + # + # Best-effort throughout: `|| true` on the deletes and no `set -e` + # dependency, because failing a run over a leftover emoji would trade a + # cosmetic problem for a real one. + - name: Clear the ๐Ÿ‘€ acknowledgement + if: always() + shell: bash + env: + GH_TOKEN: ${{ inputs.github-token }} + REPO: ${{ github.repository }} + PR_NUMBER: ${{ steps.pr.outputs.number }} + EVENT_NAME: ${{ github.event_name }} + COMMENT_ID: ${{ github.event.comment.id }} + run: | + # The identity the token posts as, which is who we placed the eyes as. + ME=$(gh api user --jq .login 2>/dev/null || echo "github-actions[bot]") + clear() { + ids=$(gh api "$1" --paginate \ + --jq '.[] | select(.content=="eyes") | select(.user.login=="'"$ME"'") | .id' \ + 2>/dev/null || true) + for id in $ids; do + if gh api -X DELETE "$1/$id" >/dev/null 2>&1; then + echo "cleared ๐Ÿ‘€ -> $1/$id" + else + echo "warning: could not clear ๐Ÿ‘€ at $1/$id (non-fatal)" + fi + done + } + if [ -n "${PR_NUMBER:-}" ]; then + clear "repos/$REPO/issues/$PR_NUMBER/reactions" + fi + if [ "$EVENT_NAME" = "issue_comment" ] && [ -n "${COMMENT_ID:-}" ]; then + clear "repos/$REPO/issues/comments/$COMMENT_ID/reactions" + fi + - name: Clean up engine output if: always() shell: bash diff --git a/rules/severity-instruction.md b/rules/severity-instruction.md index 4c76b4d..cb8f710 100644 --- a/rules/severity-instruction.md +++ b/rules/severity-instruction.md @@ -1,3 +1,11 @@ +MANDATORY OUTPUT SHAPE: after the severity tag, open every comment with a SHORT TITLE in **bold** โ€” at most about ten words naming the defect โ€” then a blank line, then the explanation. The title says what is WRONG, not what to do, and takes no full stop. + + [P1] **fetchAll drops the last item of every page** + + The loop bound `i < items.length - 1` never pushes the final element, so each page contributes one row fewer than it holds. Use `i < items.length`. + +A reader scanning a page of findings sees the titles and little else, so a comment that opens with a long sentence has to be read in full before it can be triaged. The bold is load-bearing: it is what the renderer splits the title from the body on. + MANDATORY OUTPUT FORMAT: every review comment you emit MUST begin with a severity tag as its literal first characters โ€” exactly one of [P0], [P1], [P2], or [P3], written in square brackets. A comment without a leading tag is invalid; never emit one. Before finishing, re-read each comment and confirm it starts with [P0], [P1], [P2], or [P3]. Choose the tag by this rubric: - [P0] Blocker (must not merge): an exploitable security flaw (SQL/command injection, XSS, eval/Function on untrusted input, auth or access-control bypass, a committed secret or credential), data loss, a crash on a normal execution path, or a broken build / type error. - [P1] High (fix before merge): a real but contained bug โ€” null/undefined dereference, unhandled async rejection, race condition, resource leak, missing input validation at a trust boundary, or logic that produces a wrong result. From 73810f27ed0aa863630b1c3b5eb5414b56165b17 Mon Sep 17 00:00:00 2001 From: ZhenghuaBao Date: Wed, 26 Aug 2026 16:43:28 +0800 Subject: [PATCH 2/6] A clean run says so, and carries a thumb that is withdrawn when it stops being true MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- action.yml | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 73 insertions(+), 3 deletions(-) diff --git a/action.yml b/action.yml index eba8637..9f05ab7 100644 --- a/action.yml +++ b/action.yml @@ -1172,12 +1172,82 @@ runs: `\n\n[OrcaCode Review](https://www.orcarouter.ai/code-review) โ€” ` + `Route Smarter. Ship Safer. Spend Less.`; + // THE ๐Ÿ‘ IS THE PR-LEVEL ANSWER, and it is settled on every run rather + // than only 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. + // + // OURS BY LOGIN, not by "is a bot". Filtering on user.type === 'Bot' + // was the first version and it is wrong in a way that only shows up + // where it matters: a repository reviewed by BOTH this Action and the + // hosted App has two bots reacting, and that filter would have had + // each one deleting the other's ๐Ÿ‘ on every run. The endpoint deletes + // by id, so the identity has to be exact. + // + // getAuthenticated() answers for a PAT or an App token; the default + // GITHUB_TOKEN is an installation token and 401s there, and its + // reactions carry `github-actions[bot]`. So: ask, else assume the + // default โ€” never widen to "any bot". + // + // Best-effort: a reaction is worth no failed run, and the review body + // below is the authoritative statement either way. + let meLogin = 'github-actions[bot]'; + try { + meLogin = (await github.rest.users.getAuthenticated()).data.login; + } catch (e) { + // Expected on the default token. + } + const settleThumb = async (clean) => { + try { + const mine = (await github.paginate(github.rest.reactions.listForIssue, { + ...repo, issue_number: num, per_page: 100, + })).filter((r) => r.content === '+1' && r.user && r.user.login === meLogin); + if (clean) { + if (mine.length === 0) { + await github.rest.reactions.createForIssue({ ...repo, issue_number: num, content: '+1' }); + } + } else { + for (const r of mine) { + await github.rest.reactions.deleteForIssue({ ...repo, issue_number: num, reaction_id: r.id }); + } + } + } catch (e) { + console.log('reaction not settled (non-fatal):', e.message); + } + }; + if (comments.length === 0) { - // Clean run: no separate notice โ€” the upserted summary comment - // (next step) already reports the clean state without adding a - // new timeline entry on every push. + // A CLEAN RUN SAYS SO. This used to return silently and leave the + // pinned summary to report it โ€” but the summary sits in the PR + // description, folded and above the fold only if you scroll, so a + // reviewer who asked for a review got no visible answer at all. The + // hosted reviewer posts this same sentence; two paths reviewing one + // repository should not disagree about what "nothing to flag" looks + // like. + // + // Still one timeline entry per push, which was the original worry. + // That is what the ๐Ÿ‘ is for: the entry states the result once, and + // the reaction is the at-a-glance version that updates in place. + await settleThumb(true); + const cleanBody = + `## ๐Ÿณ ${brand}\n\n` + + `โœ… **No findings** โ€” nothing to flag in this PR. Great work!`; + try { + await github.rest.pulls.createReview({ + ...repo, pull_number: num, commit_id: sha, + body: cleanBody + footer, event: 'COMMENT', + }); + } catch (e) { + console.log('Clean review failed, posting as a comment:', e.message); + await github.rest.issues.createComment({ + ...repo, issue_number: num, body: cleanBody + footer, + }); + } return; } + // Findings exist, so any ๐Ÿ‘ from an earlier head is now false. + await settleThumb(false); // Severity from the LEADING [P0]/[P1]/[P2]/[P3] tag only (mirrors gate.mjs); // a later mention must not override the missing-tag P1 fail-safe. From b243fca72b6f47f2b507da3c4418d2d18ff995a1 Mon Sep 17 00:00:00 2001 From: ZhenghuaBao Date: Wed, 26 Aug 2026 16:48:25 +0800 Subject: [PATCH 3/6] Four review findings on the alignment branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- action.yml | 83 +++++++++++++++++++++++++---------- rules/output-shape.md | 7 +++ rules/severity-instruction.md | 8 ---- 3 files changed, 66 insertions(+), 32 deletions(-) create mode 100644 rules/output-shape.md diff --git a/action.yml b/action.yml index 9f05ab7..ccdf248 100644 --- a/action.yml +++ b/action.yml @@ -278,9 +278,21 @@ runs: EVENT_NAME: ${{ github.event_name }} COMMENT_ID: ${{ github.event.comment.id }} run: | + # THE ID THIS RUN CREATED, recorded so the clear step can delete exactly + # it. Matching on the token's identity instead was wrong twice over: with + # a PAT it selects the maintainer's OWN ๐Ÿ‘€, including one they added by + # hand before the workflow ran, and with two overlapping runs each would + # clear the other's acknowledgement while its review was still going. + # + # POST is idempotent per (user, content): reacting when one is already + # there returns the EXISTING id rather than creating a second, so a + # re-run records the id it will legitimately own. + : > "$RUNNER_TEMP/cr-eyes.ids" react() { - if gh api -X POST "$1" -f content=eyes >/dev/null 2>&1; then - echo "reacted ๐Ÿ‘€ -> $1" + id=$(gh api -X POST "$1" -f content=eyes --jq .id 2>/dev/null || true) + if [ -n "$id" ]; then + echo "$1 $id" >> "$RUNNER_TEMP/cr-eyes.ids" + echo "reacted ๐Ÿ‘€ -> $1 (id $id)" else echo "warning: failed to react at $1 (non-fatal)" fi @@ -670,6 +682,7 @@ runs: BASE: origin/${{ steps.pr.outputs.base }} HEAD: ${{ steps.pr.outputs.head_sha }} INSTRUCTION: ${{ github.action_path }}/rules/severity-instruction.md + OUTPUT_SHAPE: ${{ github.action_path }}/rules/output-shape.md # Untrusted-data framing prepended to the project-conventions doc that # we extract from the BASE revision and inline into the background file # (see the review step): tells the engine the doc may only suppress @@ -793,6 +806,18 @@ runs: else cp "$INSTRUCTION" "$BACKGROUND" fi + # APPENDED IN BOTH BRANCHES, because it is not part of the rubric. The + # rubric decides WHICH severity a finding gets and a workspace may + # replace it wholesale; the output SHAPE is how every finding is written + # down, and the renderer splits a title from a body on it. Living inside + # the replaceable file, the shape contract reached only workspaces that + # had not customised their rubric โ€” the ones least likely to notice the + # untitled output it exists to prevent. + if [ -s "$OUTPUT_SHAPE" ]; then + echo "" >> "$BACKGROUND" + echo "" >> "$BACKGROUND" + cat "$OUTPUT_SHAPE" >> "$BACKGROUND" + fi # Append the first conventions doc found at the base revision. $BASE is # origin/; when it is the default branch it is a protected ref @@ -1273,7 +1298,12 @@ runs: // 1) The model sometimes writes its own **bold** lead โ€” use it verbatim. const lead = raw.match(/^\*\*(.+?)\*\*\s+([\s\S]+)$/); if (lead) { - return `${badge} **${mk(lead[1])}**\n\n${lead[2].replace(/\*\*/g, '').trim()}`; + // THE BODY KEEPS ITS ASTERISKS. Stripping every `**` was tolerable + // while only some findings took this branch; the rubric now asks + // for a bold title on all of them, so every explanation would pass + // through it โ€” and `x ** 2` or `src/**/*.js` in a finding came out + // corrupted. Only the title's own markup is removed, by mk(). + return `${badge} **${mk(lead[1])}**\n\n${lead[2].trim()}`; } // 2) Else split only when the first sentence is short enough to title. const rest = raw.replace(/\*\*/g, ''); @@ -1424,7 +1454,19 @@ runs: // Mode notes: exhaustive pass count (renders only when > 1) and the // quiet-mode explainer next to the true counts. args.push('--passes', process.env.PASSES || '1'); - if (process.env.QUIET === 'true') args.push('--quiet'); + // ONLY WHEN THERE IS STILL A P2 TO EXPLAIN. The note reads "quiet mode: + // P2 shown in summary only", which is true while the summary carries + // P2s the inline comments do not. Now that the summary reads the + // report_on-filtered file, a workspace excluding P2 there has a summary + // with zero P2s โ€” and the note would sit under a 0 explaining why it is + // shown. The flag describes a difference; with no difference, no note. + const summaryHasP2 = (() => { + try { + const r = JSON.parse(fs.readFileSync(process.env.RESULT, 'utf8')); + return (r.comments || []).some((c) => /^\s*\[?P2\]?/i.test(String(c.content || ''))); + } catch (e) { return true; } // unreadable: keep the old behaviour + })(); + if (process.env.QUIET === 'true' && summaryHasP2) args.push('--quiet'); const summaryMd = execFileSync('node', args, { encoding: 'utf8' }); // Preserve the author's description text; only the marker region changes. @@ -1506,26 +1548,19 @@ runs: EVENT_NAME: ${{ github.event_name }} COMMENT_ID: ${{ github.event.comment.id }} run: | - # The identity the token posts as, which is who we placed the eyes as. - ME=$(gh api user --jq .login 2>/dev/null || echo "github-actions[bot]") - clear() { - ids=$(gh api "$1" --paginate \ - --jq '.[] | select(.content=="eyes") | select(.user.login=="'"$ME"'") | .id' \ - 2>/dev/null || true) - for id in $ids; do - if gh api -X DELETE "$1/$id" >/dev/null 2>&1; then - echo "cleared ๐Ÿ‘€ -> $1/$id" - else - echo "warning: could not clear ๐Ÿ‘€ at $1/$id (non-fatal)" - fi - done - } - if [ -n "${PR_NUMBER:-}" ]; then - clear "repos/$REPO/issues/$PR_NUMBER/reactions" - fi - if [ "$EVENT_NAME" = "issue_comment" ] && [ -n "${COMMENT_ID:-}" ]; then - clear "repos/$REPO/issues/comments/$COMMENT_ID/reactions" - fi + # ONLY WHAT THIS RUN RECORDED. See the note beside the react step: a + # by-identity match deletes a maintainer's own ๐Ÿ‘€ under a PAT, and lets + # concurrent runs clear each other. + IDS="$RUNNER_TEMP/cr-eyes.ids" + [ -s "$IDS" ] || { echo "no ๐Ÿ‘€ recorded by this run; nothing to clear"; exit 0; } + while read -r endpoint id; do + [ -n "$endpoint" ] && [ -n "$id" ] || continue + if gh api -X DELETE "$endpoint/$id" >/dev/null 2>&1; then + echo "cleared ๐Ÿ‘€ -> $endpoint/$id" + else + echo "warning: could not clear ๐Ÿ‘€ at $endpoint/$id (non-fatal)" + fi + done < "$IDS" - name: Clean up engine output if: always() diff --git a/rules/output-shape.md b/rules/output-shape.md new file mode 100644 index 0000000..daea7d1 --- /dev/null +++ b/rules/output-shape.md @@ -0,0 +1,7 @@ +MANDATORY OUTPUT SHAPE: after the severity tag, open every comment with a SHORT TITLE in **bold** โ€” at most about ten words naming the defect โ€” then a blank line, then the explanation. The title says what is WRONG, not what to do, and takes no full stop. + + [P1] **fetchAll drops the last item of every page** + + The loop bound `i < items.length - 1` never pushes the final element, so each page contributes one row fewer than it holds. Use `i < items.length`. + +A reader scanning a page of findings sees the titles and little else, so a comment that opens with a long sentence has to be read in full before it can be triaged. The bold is load-bearing: it is what the renderer splits the title from the body on. diff --git a/rules/severity-instruction.md b/rules/severity-instruction.md index cb8f710..4c76b4d 100644 --- a/rules/severity-instruction.md +++ b/rules/severity-instruction.md @@ -1,11 +1,3 @@ -MANDATORY OUTPUT SHAPE: after the severity tag, open every comment with a SHORT TITLE in **bold** โ€” at most about ten words naming the defect โ€” then a blank line, then the explanation. The title says what is WRONG, not what to do, and takes no full stop. - - [P1] **fetchAll drops the last item of every page** - - The loop bound `i < items.length - 1` never pushes the final element, so each page contributes one row fewer than it holds. Use `i < items.length`. - -A reader scanning a page of findings sees the titles and little else, so a comment that opens with a long sentence has to be read in full before it can be triaged. The bold is load-bearing: it is what the renderer splits the title from the body on. - MANDATORY OUTPUT FORMAT: every review comment you emit MUST begin with a severity tag as its literal first characters โ€” exactly one of [P0], [P1], [P2], or [P3], written in square brackets. A comment without a leading tag is invalid; never emit one. Before finishing, re-read each comment and confirm it starts with [P0], [P1], [P2], or [P3]. Choose the tag by this rubric: - [P0] Blocker (must not merge): an exploitable security flaw (SQL/command injection, XSS, eval/Function on untrusted input, auth or access-control bypass, a committed secret or credential), data loss, a crash on a normal execution path, or a broken build / type error. - [P1] High (fix before merge): a real but contained bug โ€” null/undefined dereference, unhandled async rejection, race condition, resource leak, missing input validation at a trust boundary, or logic that produces a wrong result. From b0ec2999b6523efd9ecc0f7ada8ac5dd88c41a39 Mon Sep 17 00:00:00 2001 From: ZhenghuaBao Date: Wed, 26 Aug 2026 17:02:34 +0800 Subject: [PATCH 4/6] Give the reactions one owner, and decline where identity is ambiguous MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- action.yml | 151 +++++++++++++++++++++++++++++++++-------------------- 1 file changed, 95 insertions(+), 56 deletions(-) diff --git a/action.yml b/action.yml index ccdf248..d9ee35b 100644 --- a/action.yml +++ b/action.yml @@ -278,21 +278,26 @@ runs: EVENT_NAME: ${{ github.event_name }} COMMENT_ID: ${{ github.event.comment.id }} run: | - # THE ID THIS RUN CREATED, recorded so the clear step can delete exactly - # it. Matching on the token's identity instead was wrong twice over: with - # a PAT it selects the maintainer's OWN ๐Ÿ‘€, including one they added by - # hand before the workflow ran, and with two overlapping runs each would - # clear the other's acknowledgement while its review was still going. + # ONLY WHAT THIS RUN CREATED, decided by the status code. The POST is + # idempotent per (user, content): where a reaction already exists GitHub + # answers 200 with the EXISTING id, and only a fresh one is 201. An + # earlier version recorded either, which meant a maintainer who had + # already put ๐Ÿ‘€ on the PR โ€” with their PAT as github-token โ€” had it + # adopted and then deleted by a run that never placed it. # - # POST is idempotent per (user, content): reacting when one is already - # there returns the EXISTING id rather than creating a second, so a - # re-run records the id it will legitimately own. + # 201 also excludes the overlapping-run case: the second run gets 200 for + # the first run's reaction and records nothing, so it cannot clear an + # acknowledgement that still has a review behind it. : > "$RUNNER_TEMP/cr-eyes.ids" react() { - id=$(gh api -X POST "$1" -f content=eyes --jq .id 2>/dev/null || true) - if [ -n "$id" ]; then + out=$(gh api -i -X POST "$1" -f content=eyes 2>/dev/null || true) + code=$(printf '%s' "$out" | head -1 | tr -d '\r' | awk '{print $2}') + id=$(printf '%s' "$out" | sed -n 's/.*"id": *\([0-9][0-9]*\).*/\1/p' | head -1) + if [ "$code" = "201" ] && [ -n "$id" ]; then echo "$1 $id" >> "$RUNNER_TEMP/cr-eyes.ids" echo "reacted ๐Ÿ‘€ -> $1 (id $id)" + elif [ -n "$code" ]; then + echo "๐Ÿ‘€ already present at $1 (HTTP $code); not adopting it" else echo "warning: failed to react at $1 (non-fatal)" fi @@ -1173,6 +1178,12 @@ runs: HEAD_SHA: ${{ steps.pr.outputs.head_sha }} # The quiet-filtered copy โ€” see the "Quiet mode filter" step above. RESULT: ${{ runner.temp }}/result-posted.json + # PRE-QUIET, for the one question quiet must not answer: did this review + # FIND anything? Quiet decides what is posted; a run whose P2s it muted + # has still found them, and the summary says so. Reading the posted file + # to declare a run clean put "No findings" and a ๐Ÿ‘ on a pull request + # whose own summary listed the findings three lines above. + RESULT_FOUND: ${{ runner.temp }}/result-reported.json with: github-token: ${{ inputs.github-token }} script: | @@ -1197,52 +1208,33 @@ runs: `\n\n[OrcaCode Review](https://www.orcarouter.ai/code-review) โ€” ` + `Route Smarter. Ship Safer. Spend Less.`; - // THE ๐Ÿ‘ IS THE PR-LEVEL ANSWER, and it is settled on every run rather - // than only 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. - // - // OURS BY LOGIN, not by "is a bot". Filtering on user.type === 'Bot' - // was the first version and it is wrong in a way that only shows up - // where it matters: a repository reviewed by BOTH this Action and the - // hosted App has two bots reacting, and that filter would have had - // each one deleting the other's ๐Ÿ‘ on every run. The endpoint deletes - // by id, so the identity has to be exact. - // - // getAuthenticated() answers for a PAT or an App token; the default - // GITHUB_TOKEN is an installation token and 401s there, and its - // reactions carry `github-actions[bot]`. So: ask, else assume the - // default โ€” never widen to "any bot". - // - // Best-effort: a reaction is worth no failed run, and the review body - // below is the authoritative statement either way. - let meLogin = 'github-actions[bot]'; - try { - meLogin = (await github.rest.users.getAuthenticated()).data.login; - } catch (e) { - // Expected on the default token. - } - const settleThumb = async (clean) => { + // THE THUMB IS NOT SETTLED HERE. This step is one of several ways a run + // ends โ€” settings skip, oversized diff, a guardrail block, an engine + // failure, a rejected post โ€” and a marker only this step maintains is + // one the other endings leave stale, endorsing a head nobody reviewed. + // It is settled in the always() step near the end of this file, which + // sees every ending. All this step does is record what it concluded. + const markClean = () => { try { - const mine = (await github.paginate(github.rest.reactions.listForIssue, { - ...repo, issue_number: num, per_page: 100, - })).filter((r) => r.content === '+1' && r.user && r.user.login === meLogin); - if (clean) { - if (mine.length === 0) { - await github.rest.reactions.createForIssue({ ...repo, issue_number: num, content: '+1' }); - } - } else { - for (const r of mine) { - await github.rest.reactions.deleteForIssue({ ...repo, issue_number: num, reaction_id: r.id }); - } - } + fs.writeFileSync(process.env.RUNNER_TEMP + '/cr-clean', '1'); } catch (e) { - console.log('reaction not settled (non-fatal):', e.message); + console.log('could not record the clean verdict (non-fatal):', e.message); } }; - if (comments.length === 0) { + // DID THE REVIEW FIND ANYTHING โ€” not "is there anything left to post". + // `comments` is the quiet-filtered list, so a quiet workspace whose + // findings were all P2 arrives here empty while its summary reports + // them. Declaring that clean contradicts the summary on the same PR. + let foundCount = comments.length; + try { + const found = JSON.parse(fs.readFileSync(process.env.RESULT_FOUND, 'utf8')); + foundCount = (found.comments || []).length; + } catch (e) { + // Unreadable: fall back to what we can see rather than guess clean. + } + + if (foundCount === 0) { // A CLEAN RUN SAYS SO. This used to return silently and leave the // pinned summary to report it โ€” but the summary sits in the PR // description, folded and above the fold only if you scroll, so a @@ -1254,7 +1246,7 @@ runs: // Still one timeline entry per push, which was the original worry. // That is what the ๐Ÿ‘ is for: the entry states the result once, and // the reaction is the at-a-glance version that updates in place. - await settleThumb(true); + markClean(); const cleanBody = `## ๐Ÿณ ${brand}\n\n` + `โœ… **No findings** โ€” nothing to flag in this PR. Great work!`; @@ -1271,8 +1263,6 @@ runs: } return; } - // Findings exist, so any ๐Ÿ‘ from an earlier head is now false. - await settleThumb(false); // Severity from the LEADING [P0]/[P1]/[P2]/[P3] tag only (mirrors gate.mjs); // a later mention must not override the missing-tag P1 fail-safe. @@ -1538,7 +1528,7 @@ runs: # Best-effort throughout: `|| true` on the deletes and no `set -e` # dependency, because failing a run over a leftover emoji would trade a # cosmetic problem for a real one. - - name: Clear the ๐Ÿ‘€ acknowledgement + - name: Settle the review reactions if: always() shell: bash env: @@ -1552,7 +1542,11 @@ runs: # by-identity match deletes a maintainer's own ๐Ÿ‘€ under a PAT, and lets # concurrent runs clear each other. IDS="$RUNNER_TEMP/cr-eyes.ids" - [ -s "$IDS" ] || { echo "no ๐Ÿ‘€ recorded by this run; nothing to clear"; exit 0; } + # NOT AN EARLY EXIT. Recording no ๐Ÿ‘€ is ordinary โ€” the reaction was + # already there, or the POST failed โ€” and the thumb below still has to be + # settled. An `exit 0` here is what would leave a stale ๐Ÿ‘ on exactly the + # runs that never got to acknowledge themselves. + [ -s "$IDS" ] || : > "$IDS" while read -r endpoint id; do [ -n "$endpoint" ] && [ -n "$id" ] || continue if gh api -X DELETE "$endpoint/$id" >/dev/null 2>&1; then @@ -1562,6 +1556,51 @@ runs: fi done < "$IDS" + # ------------------------------------------------------------------ + # THE ๐Ÿ‘, SETTLED WHERE EVERY ENDING IS VISIBLE. + # + # It used to be added 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. A + # thumb only that step maintains is one every other ending leaves in + # place, endorsing a head nobody reviewed. This step runs on always(). + # + # DECLINED UNDER A PAT, and that is the point rather than a limitation. + # GitHub has no way to ask "which reaction did I create" after the fact, + # so the only handle is the author โ€” and when github-token is a + # maintainer's PAT, their manual ๐Ÿ‘ and ours are the same author. Two + # rounds of review went into narrowing that predicate; the honest answer + # at the boundary is to leave reactions alone rather than to guess and + # delete somebody's approval. The review body and the summary still say + # everything the thumb would have. + ME_TYPE=$(gh api user --jq .type 2>/dev/null || echo "Bot") + if [ "$ME_TYPE" != "Bot" ]; then + echo "github-token belongs to a user, not an app; leaving ๐Ÿ‘ alone" + exit 0 + fi + ME=$(gh api user --jq .login 2>/dev/null || echo "github-actions[bot]") + THUMBS="repos/$REPO/issues/$PR_NUMBER/reactions" + MINE=$(gh api "$THUMBS" --paginate \ + --jq '.[] | select(.content=="+1") | select(.user.login=="'"$ME"'") | .id' \ + 2>/dev/null || true) + if [ -f "$RUNNER_TEMP/cr-clean" ]; then + # Clean, and only this run's own marker says so. Adding when one is + # already there is a no-op at the API, so this is safe to repeat. + if [ -z "$MINE" ]; then + gh api -X POST "$THUMBS" -f content=+1 >/dev/null 2>&1 \ + && echo "๐Ÿ‘ -> $THUMBS" || echo "warning: could not add ๐Ÿ‘ (non-fatal)" + else + echo "๐Ÿ‘ already present" + fi + else + # Findings, a skip, or a failure โ€” in every one of those a ๐Ÿ‘ from an + # earlier head is now false. + for id in $MINE; do + gh api -X DELETE "$THUMBS/$id" >/dev/null 2>&1 \ + && echo "withdrew ๐Ÿ‘ -> $THUMBS/$id" || echo "warning: could not withdraw ๐Ÿ‘ (non-fatal)" + done + fi + - name: Clean up engine output if: always() shell: bash From e6322363c5f94d4986f43b6eab8a1e2729bee14f Mon Sep 17 00:00:00 2001 From: ZhenghuaBao Date: Wed, 26 Aug 2026 17:14:24 +0800 Subject: [PATCH 5/6] Stop answering the identity question more precisely each round MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- action.yml | 122 +++++++++++++++++++++++++++++++---------------------- 1 file changed, 72 insertions(+), 50 deletions(-) diff --git a/action.yml b/action.yml index d9ee35b..3ecb48f 100644 --- a/action.yml +++ b/action.yml @@ -217,6 +217,7 @@ runs: "$RUNNER_TEMP/policy-block.json" \ "$RUNNER_TEMP/pr.diff" "$RUNNER_TEMP/diff-guard.json" "$RUNNER_TEMP/prev-summary.md" \ "$RUNNER_TEMP/wallclock-timeout" "$RUNNER_TEMP/cr-usage.jsonl" \ + "$RUNNER_TEMP/cr-clean" \ "$RUNNER_TEMP/result.l1.json" "$RUNNER_TEMP/result.l2.json" \ "$RUNNER_TEMP/result-extra.l1.json" "$RUNNER_TEMP/result-extra.l2.json" @@ -278,26 +279,25 @@ runs: EVENT_NAME: ${{ github.event_name }} COMMENT_ID: ${{ github.event.comment.id }} run: | - # ONLY WHAT THIS RUN CREATED, decided by the status code. The POST is - # idempotent per (user, content): where a reaction already exists GitHub - # answers 200 with the EXISTING id, and only a fresh one is 201. An - # earlier version recorded either, which meant a maintainer who had - # already put ๐Ÿ‘€ on the PR โ€” with their PAT as github-token โ€” had it - # adopted and then deleted by a run that never placed it. + # JUST REACT. This used to record the id the POST returned so the clear + # step could delete exactly it โ€” first unconditionally, then only on a + # 201 โ€” and each version needed a JSON parse that node is not yet + # available for at this point in the job. The last one was a greedy + # expression that picked the nested author id out of a compact response, + # so the DELETE would have targeted the wrong resource. # - # 201 also excludes the overlapping-run case: the second run gets 200 for - # the first run's reaction and records nothing, so it cannot clear an - # acknowledgement that still has a review behind it. - : > "$RUNNER_TEMP/cr-eyes.ids" + # The id existed to tell OUR reaction from the token owner's. That + # question is settled once, in the settle step, by declining to manage + # reactions at all when github-token is a user rather than an app โ€” so + # there is nothing here to record, and nothing to parse. + # + # Two concurrent runs as the same app share one reaction, so the first to + # finish clears it while the second is still working. Accepted: a missing + # ๐Ÿ‘€ for a few minutes is cosmetic, and every mechanism that avoided it + # cost more correctness elsewhere than it bought. react() { - out=$(gh api -i -X POST "$1" -f content=eyes 2>/dev/null || true) - code=$(printf '%s' "$out" | head -1 | tr -d '\r' | awk '{print $2}') - id=$(printf '%s' "$out" | sed -n 's/.*"id": *\([0-9][0-9]*\).*/\1/p' | head -1) - if [ "$code" = "201" ] && [ -n "$id" ]; then - echo "$1 $id" >> "$RUNNER_TEMP/cr-eyes.ids" - echo "reacted ๐Ÿ‘€ -> $1 (id $id)" - elif [ -n "$code" ]; then - echo "๐Ÿ‘€ already present at $1 (HTTP $code); not adopting it" + if gh api -X POST "$1" -f content=eyes >/dev/null 2>&1; then + echo "reacted ๐Ÿ‘€ -> $1" else echo "warning: failed to react at $1 (non-fatal)" fi @@ -1234,6 +1234,18 @@ runs: // Unreadable: fall back to what we can see rather than guess clean. } + // MUTED IS NOT CLEAN, AND IT IS NOT A REVIEW EITHER. Quiet drops every + // reported P2, so a run can have found things and still have nothing + // to post. Falling through built a review from an empty array โ€” + // "Found **0** issues in this PR: ." โ€” beside a description summary + // that listed the muted findings. Quiet's own promise is silence, so + // this posts nothing AND records no clean verdict: the run is not + // clean, it is quiet. + if (foundCount > 0 && comments.length === 0) { + console.log('quiet mode muted every finding; posting nothing'); + return; + } + if (foundCount === 0) { // A CLEAN RUN SAYS SO. This used to return silently and leave the // pinned summary to report it โ€” but the summary sits in the PR @@ -1246,7 +1258,6 @@ runs: // Still one timeline entry per push, which was the original worry. // That is what the ๐Ÿ‘ is for: the entry states the result once, and // the reaction is the at-a-glance version that updates in place. - markClean(); const cleanBody = `## ๐Ÿณ ${brand}\n\n` + `โœ… **No findings** โ€” nothing to flag in this PR. Great work!`; @@ -1261,6 +1272,12 @@ runs: ...repo, issue_number: num, body: cleanBody + footer, }); } + // AFTER ONE OF THEM SUCCEEDED. Written first, a run whose review AND + // its comment fallback both failed still told the always() step it + // was clean โ€” so the ๐Ÿ‘ endorsed a verdict that was never published. + // A throw from the fallback skips this and the thumb is withdrawn, + // which is the right answer for a run that could not say anything. + markClean(); return; } @@ -1538,23 +1555,41 @@ runs: EVENT_NAME: ${{ github.event_name }} COMMENT_ID: ${{ github.event.comment.id }} run: | - # ONLY WHAT THIS RUN RECORDED. See the note beside the react step: a - # by-identity match deletes a maintainer's own ๐Ÿ‘€ under a PAT, and lets - # concurrent runs clear each other. - IDS="$RUNNER_TEMP/cr-eyes.ids" - # NOT AN EARLY EXIT. Recording no ๐Ÿ‘€ is ordinary โ€” the reaction was - # already there, or the POST failed โ€” and the thumb below still has to be - # settled. An `exit 0` here is what would leave a stale ๐Ÿ‘ on exactly the - # runs that never got to acknowledge themselves. - [ -s "$IDS" ] || : > "$IDS" - while read -r endpoint id; do - [ -n "$endpoint" ] && [ -n "$id" ] || continue - if gh api -X DELETE "$endpoint/$id" >/dev/null 2>&1; then - echo "cleared ๐Ÿ‘€ -> $endpoint/$id" - else - echo "warning: could not clear ๐Ÿ‘€ at $endpoint/$id (non-fatal)" - fi - done < "$IDS" + # ONE GATE FOR BOTH REACTIONS: is this token an app? + # + # GitHub cannot be asked "which reaction did I create" after the fact, so + # the only handle is the author โ€” and when github-token is a maintainer's + # PAT, their manual ๐Ÿ‘€ or ๐Ÿ‘ and ours have the same author. Three rounds + # of review went into narrowing that predicate with recorded ids and + # status codes; every version needed a parse, and the parses were where + # the bugs were. Declining at the boundary removes the question instead + # of answering it more precisely each time. + # + # The review body and the summary say everything the reactions would. + ME_TYPE=$(gh api user --jq .type 2>/dev/null || echo "Bot") + if [ "$ME_TYPE" != "Bot" ]; then + echo "github-token belongs to a user, not an app; leaving reactions alone" + exit 0 + fi + ME=$(gh api user --jq .login 2>/dev/null || echo "github-actions[bot]") + + # ------------------------------------------------------------------ + # ๐Ÿ‘€ โ€” it said "I am looking at this", and the run has stopped looking. + clear_eyes() { + ids=$(gh api "$1" --paginate \ + --jq '.[] | select(.content=="eyes") | select(.user.login=="'"$ME"'") | .id' \ + 2>/dev/null || true) + for id in $ids; do + gh api -X DELETE "$1/$id" >/dev/null 2>&1 \ + && echo "cleared ๐Ÿ‘€ -> $1/$id" || echo "warning: could not clear ๐Ÿ‘€ (non-fatal)" + done + } + if [ -n "${PR_NUMBER:-}" ]; then + clear_eyes "repos/$REPO/issues/$PR_NUMBER/reactions" + fi + if [ "$EVENT_NAME" = "issue_comment" ] && [ -n "${COMMENT_ID:-}" ]; then + clear_eyes "repos/$REPO/issues/comments/$COMMENT_ID/reactions" + fi # ------------------------------------------------------------------ # THE ๐Ÿ‘, SETTLED WHERE EVERY ENDING IS VISIBLE. @@ -1565,20 +1600,6 @@ runs: # thumb only that step maintains is one every other ending leaves in # place, endorsing a head nobody reviewed. This step runs on always(). # - # DECLINED UNDER A PAT, and that is the point rather than a limitation. - # GitHub has no way to ask "which reaction did I create" after the fact, - # so the only handle is the author โ€” and when github-token is a - # maintainer's PAT, their manual ๐Ÿ‘ and ours are the same author. Two - # rounds of review went into narrowing that predicate; the honest answer - # at the boundary is to leave reactions alone rather than to guess and - # delete somebody's approval. The review body and the summary still say - # everything the thumb would have. - ME_TYPE=$(gh api user --jq .type 2>/dev/null || echo "Bot") - if [ "$ME_TYPE" != "Bot" ]; then - echo "github-token belongs to a user, not an app; leaving ๐Ÿ‘ alone" - exit 0 - fi - ME=$(gh api user --jq .login 2>/dev/null || echo "github-actions[bot]") THUMBS="repos/$REPO/issues/$PR_NUMBER/reactions" MINE=$(gh api "$THUMBS" --paginate \ --jq '.[] | select(.content=="+1") | select(.user.login=="'"$ME"'") | .id' \ @@ -1619,5 +1640,6 @@ runs: "$RUNNER_TEMP/policy-block.json" \ "$RUNNER_TEMP/pr.diff" "$RUNNER_TEMP/diff-guard.json" "$RUNNER_TEMP/prev-summary.md" \ "$RUNNER_TEMP/wallclock-timeout" "$RUNNER_TEMP/cr-usage.jsonl" \ + "$RUNNER_TEMP/cr-clean" \ "$RUNNER_TEMP/result.l1.json" "$RUNNER_TEMP/result.l2.json" \ "$RUNNER_TEMP/result-extra.l1.json" "$RUNNER_TEMP/result-extra.l2.json" From 50ac6b9a34df1ba09f5f71f1b8c7793168085a05 Mon Sep 17 00:00:00 2001 From: ZhenghuaBao Date: Wed, 26 Aug 2026 17:25:59 +0800 Subject: [PATCH 6/6] Gate the acknowledgement too, and bind the thumb to the head it reviewed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- action.yml | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/action.yml b/action.yml index 3ecb48f..49ea232 100644 --- a/action.yml +++ b/action.yml @@ -279,6 +279,19 @@ runs: EVENT_NAME: ${{ github.event_name }} COMMENT_ID: ${{ github.event.comment.id }} run: | + # THE SAME GATE AS THE CLEAR, and its absence here was a regression the + # last round introduced: gating only the removal left a PAT-backed run + # POSTing ๐Ÿ‘€ and never taking it off, which is worse than the state + # before any of this. Declining is only coherent if it covers both ends. + # + # Checked once here rather than inside react(), so a token that cannot be + # identified costs one API call, not two. + ME_TYPE=$(gh api user --jq .type 2>/dev/null || echo "Bot") + if [ "$ME_TYPE" != "Bot" ]; then + echo "github-token belongs to a user, not an app; not reacting" + exit 0 + fi + # JUST REACT. This used to record the id the POST returned so the clear # step could delete exactly it โ€” first unconditionally, then only on a # 201 โ€” and each version needed a JSON parse that node is not yet @@ -1552,6 +1565,7 @@ runs: GH_TOKEN: ${{ inputs.github-token }} REPO: ${{ github.repository }} PR_NUMBER: ${{ steps.pr.outputs.number }} + HEAD_SHA: ${{ steps.pr.outputs.head_sha }} EVENT_NAME: ${{ github.event_name }} COMMENT_ID: ${{ github.event.comment.id }} run: | @@ -1604,6 +1618,25 @@ runs: MINE=$(gh api "$THUMBS" --paginate \ --jq '.[] | select(.content=="+1") | select(.user.login=="'"$ME"'") | .id' \ 2>/dev/null || true) + # ONLY IF THIS RUN STILL DESCRIBES THE PR. 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, + # endorsing a head that run never saw. + # + # Withdrawal needs no such check and deliberately does not have one: a + # thumb removed by a stale run is re-added by the current one on its own + # completion, so the failure direction is a moment without a ๐Ÿ‘ rather + # than a ๐Ÿ‘ over unreviewed code. + # AN UNCONFIRMED HEAD COUNTS AS MOVED. Adding on a failed lookup would + # contradict the rule this check exists for โ€” the thumb must not endorse + # a head this run did not review, and "could not tell" is not evidence + # that it did. A clean run that loses the race or the API call simply + # goes without, and the next run puts it back. + CURRENT_SHA=$(gh api "repos/$REPO/pulls/$PR_NUMBER" --jq .head.sha 2>/dev/null || echo "") + if [ -f "$RUNNER_TEMP/cr-clean" ] && [ "$CURRENT_SHA" != "$HEAD_SHA" ]; then + echo "cannot confirm this run still describes the PR head (${HEAD_SHA} vs ${CURRENT_SHA:-unknown}); leaving ๐Ÿ‘ alone" + exit 0 + fi if [ -f "$RUNNER_TEMP/cr-clean" ]; then # Clean, and only this run's own marker says so. Adding when one is # already there is a no-op at the API, so this is safe to repeat.