claude-code-review.yml's "Post a notice that self-review was skipped" step
(added in #440, closing #434) carries about 30 lines of inline shell with no
CI coverage. Every comparable piece of shell in this repo has been extracted
to scripts/ plus a composite action with an offline test suite --- see
classify-push-failure.sh, detect-review-request.sh, sum-costs.sh,
build-reviewer-args.sh, select-existing-issue.sh. This one is the
exception.
What is untested
The step decides two things, and both were verified only by an ad-hoc stub
during #440's review rounds, not by anything that runs again:
- Whether the notice posted, recorded as
posted=true/posted=false on
the skip-notice step output.
- The body assembly --- a
printf block whose output has to stay valid
GitHub alert markup, and which must keep carrying an actions/runs/<id>
link, because the collapse step matches comments on exactly that.
Why the second one is the load-bearing half
The posted output gates the collapse step (#440's round-2 finding 1). If the
body ever loses its run-URL link, the collapse step silently stops matching the
notice, and a PR accumulates one notice per push with nothing folding them ---
a regression with no failing check anywhere, since nothing asserts the link is
present.
That is a decidable condition over a string, which is precisely the shape
algorithmatize-checks says should not rest on a reviewer noticing.
Suggested shape
Follow the report-push-failure precedent, which is the closest analogue (it
also assembles a comment body and posts it, and has both an offline table test
and a dry-run e2e call from _selftest.yml):
scripts/build-self-review-skip-notice.sh --- takes the workflow path and
run URL, writes the body. Testable with no network.
- A test asserting the body contains the run URL in the form the collapse
step's own regex (actions/runs/(?<r>[0-9]+)) matches --- ideally applying
that literal regex, so the two cannot drift apart.
- A
dry-run input on the composite so _selftest.yml can exercise the real
uses: path, per the open-failure-issue precedent.
Note the usual bootstrapping gap if this becomes a composite: its first @v2
caller cannot land in the same PR that introduces it.
(Raised by me in #440's PR body and left as an open offer; the maintainer
merged without taking it, so filing rather than leaving it in a merged PR's
description.)
claude-code-review.yml's "Post a notice that self-review was skipped" step(added in #440, closing #434) carries about 30 lines of inline shell with no
CI coverage. Every comparable piece of shell in this repo has been extracted
to
scripts/plus a composite action with an offline test suite --- seeclassify-push-failure.sh,detect-review-request.sh,sum-costs.sh,build-reviewer-args.sh,select-existing-issue.sh. This one is theexception.
What is untested
The step decides two things, and both were verified only by an ad-hoc stub
during #440's review rounds, not by anything that runs again:
posted=true/posted=falseonthe
skip-noticestep output.printfblock whose output has to stay validGitHub alert markup, and which must keep carrying an
actions/runs/<id>link, because the collapse step matches comments on exactly that.
Why the second one is the load-bearing half
The
postedoutput gates the collapse step (#440's round-2 finding 1). If thebody ever loses its run-URL link, the collapse step silently stops matching the
notice, and a PR accumulates one notice per push with nothing folding them ---
a regression with no failing check anywhere, since nothing asserts the link is
present.
That is a decidable condition over a string, which is precisely the shape
algorithmatize-checkssays should not rest on a reviewer noticing.Suggested shape
Follow the
report-push-failureprecedent, which is the closest analogue (italso assembles a comment body and posts it, and has both an offline table test
and a
dry-rune2e call from_selftest.yml):scripts/build-self-review-skip-notice.sh--- takes the workflow path andrun URL, writes the body. Testable with no network.
step's own regex (
actions/runs/(?<r>[0-9]+)) matches --- ideally applyingthat literal regex, so the two cannot drift apart.
dry-runinput on the composite so_selftest.ymlcan exercise the realuses:path, per theopen-failure-issueprecedent.Note the usual bootstrapping gap if this becomes a composite: its first
@v2caller cannot land in the same PR that introduces it.
(Raised by me in #440's PR body and left as an open offer; the maintainer
merged without taking it, so filing rather than leaving it in a merged PR's
description.)