mirror of
https://github.com/Continuum-AI-Corp/Orca-Code-Review.git
synced 2026-09-27 09:29:03 +00:00
b4fe7ee66b* feat: one review tier, and a report_on severity filter
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>
* fix: an empty report_on is a setting, not a missing one
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>
* docs: report_on in the README, and detach the retired tier label
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>
* Revert the retired-label cleanup
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.
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
100 lines
3.4 KiB
100 lines
3.4 KiB