Skip to content

fix(triage): close the release review's sift findings - #258

Merged
gurinderu merged 9 commits into
mainfrom
fix/triage-sift-guards
Oct 23, 2026
Merged

gurinderu merged 9 commits into
mainfrom
fix/triage-sift-guards

Conversation

@gurinderu

Copy link
Copy Markdown
Owner

Closes the Medium findings the full review of release 0.34.0 raised in the triage sift (realm @nick/craft, case №37):

  • "Not this PR's" counts as checked only when the validator cites git show <prBase>:path — a new prBase argument (the PR's fork point; HEAD, other refs and the validation pin do not count; backticked/quoted forms accepted). Without prBase the finding goes to the human. addressing-findings, develop, the engine's docs and README pass/document it (#303, #309).
  • In another reviewer's PR thread, only an OWNER/MEMBER/COLLABORATOR rejection is recorded to memory — a PR author alone no longer settles it, closing the second-account bypass (#302).
  • A sifted batch repeat carries repeat_of and takes its twin's disposition in the review ledger instead of its own reject (#312).
  • An accidentally committed vitest report is removed and .vitest/ ignored.

Dropped Low findings:

  • skills/rust-review/SKILL.md's pre-PR gate still names triage-findings without prBase (fails safe: class 1 goes to the human).
  • prBase is matched as an exact string, so an abbreviated SHA in the premise reads as unchecked (fails safe).
  • A maintainer whose org membership is private may be reported as CONTRIBUTOR and their rejection skipped (fails safe).
  • #312's rule is executed by the skill's prose; the test proves only that repeat_of survives the plan.

…e's own base

A class-1 finding counted as checked at the base on any `git show <ref>:` or the bare word
merge-base, so `git show HEAD:x` turned a defect the PR introduced into a deferral and an issue
filed without asking. The premise must now name `git show <base>:<path>` for the base the triage
was given; with no base, class 1 always goes to a human. The validator prompt names the exact base.
…maintainer's rejection

The self-dismissal guard compared logins only, so an outside PR author with a second account
could post a remark under one and reject it under the other, and the rejection became a
project-wide decision. In a non-craft thread the rejecting reply must now come from an OWNER,
MEMBER or COLLABORATOR; the PR author's word still counts in a craft-finding thread.
… reject

A batch repeat's ledger row was `reject`, and the fix loop wrote it to the review ledger as
`rejected`, so when the twin's fix missed the repeat's line the next re-review carried a real
defect as dismissed. The row now names its twin in `repeat_of` (restored over a plan agent that
drops it), and the skill maps a repeat by its twin's disposition and never maps a sifted row by
its own reject.
Keeps the function under the complexity ceiling after the base check was added.
…alidation pin

`base` is the ref the findings were generated against, usually the reviewed head, so citing it
showed the PR's own code. A new `prBase` argument names the commit the PR forks from; class 1
counts as checked only on `git show <prBase>:<path>`, and with no prBase it goes to a human.
The skill's triage dispatch passes prBase = git merge-base origin/<trunk> <reviewed head>.
…arentheses

The prompt asks for `git show <prBase>:<path>` in backticks, and the check refused that form.
A backtick, quote or parenthesis may now stand before git; another ref, or one that merely
starts with prBase, is still refused.
The develop skill's triage dispatch, the engine's whenToUse and the README now name prBase =
git merge-base origin/<trunk> <reviewed head>, so class-1 findings can be deferred outside the
addressing-findings flow too.
@gurinderu
gurinderu merged commit d627c65 into main Oct 23, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant