feat: one review tier, and a report_on severity filter - #28
Merged
Conversation
The recipe routes every branch to one model, and action.yml has hardcoded PASSES=1 with no promotion for a while now. This drops what was left of the two-tier machinery and adds the publish-side severity filter the dashboard already offers. Single tier - action.yml: 22 steps -> 21. Gone: the PR-label read (one fewer paginated API call per run), the promote step, the second per-tier report step, and TIER_STATE / RESULT_CHEAP / PROMOTE / HELD / strong_ran. - The tier reported to the control plane is `standard`. It used to be `strong`, from when a cheap pass could escalate; with one pass the label described the reviewer rather than the run. - RESULT_STRONG -> RESULT_FINAL, result-strong.json -> result-final.json. - summary-comment.mjs: the `Tier:` line is gone, and the blocking count now always follows --block-on. It used to follow --fix-first on a held run, which meant the summary and the merge gate could disagree about what blocks the PR. report_on - New scripts/report-filter.mjs, applied between the engine and the quiet filter: result.json -> result-reported.json -> result-posted.json. - The published set is report_on UNION block_on, so a severity that blocks the merge can never be withheld from the timeline. - Absent report_on (a gateway that predates the field) is pass-through; an empty value means "only what blocks". - The gate and the run report keep reading the unfiltered result. Display filtering never moves enforcement or the numbers. Docs and template - README, the workflow template and the recipes describe one review per push instead of a fast-then-strong escalation. Tests - settings.test.mjs: the action.yml wiring assertions now go through a sliceStep helper that fails when a boundary step is missing. Two of them had been silently matching most of the file since their end-boundary step was removed. - fact-proxy.test.mjs: same fix, same cause. - report.test.mjs: asserts every report.mjs invocation sits behind the `report` input, as a count equality rather than a fixed number. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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: 8bd7e71038
ℹ️ 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".
Review round 1 found four real problems, three of them introduced here. The report_on step guarded --show on `[ -n "$REPORT_ON" ]`, which sent an empty value down the no-flag branch — and report-filter.mjs reads an absent flag as "no setting, pass everything through". So a workspace configured as "publish only what blocks" got every severity published instead: the exact inversion the comment above it claimed to be preventing. --show now goes out unconditionally. It is safe to do that because the settings step always writes a value (the settings-disabled path writes every severity explicitly), and if that step fails outright it fails the job rather than leaving the output empty. report-filter.mjs shipped with no tests, in a repo where every other script has a .test.mjs, and the bug above sat exactly in the seam that most needed one. New report-filter.test.mjs covers the three states of --show, the union with block-on, the shared severity parsing, and the usage exits. settings.test.mjs pins the unconditional --show so the guard cannot come back. The `fix-first` input description promised the severities were sent to the reviewer to fix first. They are not, and were not: the only consumer is the exhaustive early-stop loop, so the input does nothing at all on a default (non-exhaustive) run. Description rewritten to say that. Its env line on the summary and gate steps is now dead — those consumers went with the held-run branch — so both are removed; the Review step keeps its copy. The startup cleanup lost result-cheap.json and result-strong.json when the filenames changed, but a reused self-hosted runner hard-cancelled under the older action still has them, holding finding text and source snippets. That step exists for exactly the files an earlier cleanup never reached, so the legacy names go back alongside the new ones. The recipe still documented x-cr-prev-tier as none | cheap | strong while inviting workspace owners to write rules on it, and it had grown a second copy of the fact block. One block now, documenting the one value, and saying plainly that a rule matching cheap or strong will fall through to the default. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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: 6114f6369a
ℹ️ 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".
Review round 2, four findings, all real.
The README promised P2 → Comment unconditionally and did not mention the
Report severities control at all. With report_on narrowed — and the shipped
default for a new workspace IS narrower than every severity — a P2 finding is
counted in the summary and never posted on the diff, with nothing anywhere
explaining the gap. The severity table now has a "Posted inline" column and a
P3 row, and says what the two settings are: merge policy decides what blocks,
Report severities decides what is posted, a blocking severity is always posted
whatever the display setting says, and the summary always counts everything.
The bundled setup skill still taught the cascade in four places. The installer
copies it and agents treat it as the configuration authority, so it was
actively teaching removed behaviour: `fix-first` described as withholding the
strong tier, `exhaustive` as extra passes on the strong tier, and SKILL.md
calling fix-first an escalation control. Fixing SKILL.md also fixed a
pre-existing platforms.test.mjs failure.
The workflow template's fix-first comment carried the same over-promise the
input description had ("ask for a concrete fix on these first"). It is the
exhaustive early-stop set and nothing else.
An older version created the repository label `orca-review:strong` and attached
it to a PR when it was promoted, to carry the tier between runs. Nothing reads
or writes it now, so a PR promoted before the upgrade keeps a label asserting a
tier that no longer exists — state this action created and would otherwise
abandon on someone's PR. New step detaches it: a blind removeLabel swallowing
404, not list-then-remove, because reading the labels is the paginated call this
change deleted and paying it every run to discover whether a migration artifact
is present costs more than the removal. continue-on-error, because cleanup must
never turn a passing review red. Before the gate, because the gate exits
non-zero on a blocking finding and a blocked PR is exactly the kind that was
promoted. Deletable once no open PR predates the release, and the comment says
so.
Also swept two stale comments the earlier commit missed: summary-comment.mjs
still documented the held-run exception as live, and action.yml called
RESULT_FINAL a per-tier snapshot overwritten by the escalation pass. And the
new step landed between the summary and the gate, so the four test slices that
used the gate as the summary's end boundary now stop at the new step instead —
including one that was still a bare indexOf pair.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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: 3d4e498fcd
ℹ️ 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".
Assume no open PR carries `orca-review:strong`. The step was migration cleanup for a label an older version attached, and it cost an API call on every run, for ever, to undo a one-time artifact — not worth carrying in the action for that. Reverts the step, its test, and the four slice boundaries that had moved to it.
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: 55e878a77f
ℹ️ 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 was referenced Aug 26, 2026
akf66
added a commit
that referenced
this pull request
Aug 26, 2026
There is no add- or remove-label step anywhere in action.yml. Three files still described one, and the one that mattered most is the copy written into every user's repo. #28 retired the review cascade and updated `workflows/orca-code-review.yml`, but the skill keeps its own copy of that workflow in `assets/`. The example was fixed; the template users actually receive was not. Corrected: - skills/.../assets/workflow.yml — "tier state label + clean/fallback PR comments" -> "clean/fallback PR comments", matching the example - references/inputs.md — github-token no longer "manages the tier label"; "Models | per tier" is now one model, because there is one tier - action.yml — the input description those two were copied from `issues: write` stays. It is still required, just for a different reason than the comment claimed: PR comments, and the emoji reaction on a /orcacode-review command. Only the justification was wrong. Left alone because they are still true: the x-cr-prev-tier fact the proxy injects (kept deliberately so a future size-based routing policy needs no Action change), and the `tier` field in the run report — now noted in inputs.md as always "standard", so nobody reads it as varying. Five tests close the gap that caused this. The example workflow and the skill template must agree, comment-stripped, on `permissions:`, `on:` and `concurrency:`, and neither may mention a tier label. Verified by reverting the fix: the tier-label test goes red.
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.
The recipe routes every branch to one model and
action.ymlhas hardcodedPASSES=1with no promotion for a while now. This removes what was left of the two-tier machinery, and adds the publish-side severity filter the dashboard already offers but the Action ignored.Single tier
action.ymlgoes 22 steps → 21. Removed:Promote tierstepTIER_STATE,RESULT_CHEAP,PROMOTE,HELD,strong_ranand all their consumersThe tier sent to the control plane is now
standard. It used to bestrong, from when a cheap pass could escalate to a strong one — with a single pass the label described which reviewer picked a finding up rather than anything about the run.RESULT_STRONG→RESULT_FINAL,result-strong.json→result-final.json.summary-comment.mjsloses theTier:line, and the blocking count now always follows--block-on. It used to follow--fix-firston a held run, which meant the summary and the merge gate could report different answers to "what is blocking this PR".report_on
New
scripts/report-filter.mjs, applied between the engine and the quiet filter:report_onat all (a gateway that predates the field) is pass-through. An empty value means "only what blocks".The merge gate and the run report keep reading the unfiltered
result.json. Display filtering never moves enforcement or the numbers.Docs and template
README, the workflow template and both recipes now describe one review per push.
exhaustiveis described as additional passes over the same diff, which is what it does.Tests
Two structural suites had rotted quietly and this is worth calling out.
settings.test.mjsandfact-proxy.test.mjsslicedaction.ymlwith a bareindexOfpair; when a boundary step is renamed or removed,indexOfreturns -1,slice(start, -1)runs to the end of the file, and every assertion in the test matches against most ofaction.yml— passing for the wrong reason. Both now go through asliceStephelper that asserts both boundaries exist.report.test.mjsnow assertsinvocations.length === guards.lengthovernode "$REPORT"rather than a fixed count, so the "every report call sits behind thereportinput" invariant holds no matter how many report steps there are.Local: 21 test files pass. Four failures remain on my Windows box in
settings/report/installer/platforms, all pre-existing onmain(repeated in-process HTTP servers crash node with0xC0000409) — same count as the baseline, one fewer insettings.Order of operations
The control plane needs to accept
standardbefore this ships, or the run report is dropped — silently, since reporting is best-effort. That side is already merged and awaiting deploy.