NAS-142136 / 27.0.0-BETA.1 / Publish the Claude review as a shared workflow, with pr-title and CI for this repo - #3
Merged
Conversation
Aaron and William are happy with the review variation running in truenas/api-client-ts and asked for it here, keeping the existing one available in case the team prefers it. So this publishes both: - `claude-review.yml` — the original comment-only review, restored from the work on NAS-142094 that was held back while it settled. - `claude-review-gated.yml` — the api-client-ts variant: structured output scored by a threshold script, inline comments, prior review threads handed to the reviewer, and a superseded banner on the stale summary. Three things the port had to decide: - The severity rubric lives here as `review/rubric.md` and is appended to the caller's own prompt file, rather than being copied into each repo. The gate and the schema are here; three copies of the rubric would drift from the thing that scores them, and a check would start passing or failing for reasons nobody wrote down. - The scripts and schema are checked out into the caller's workspace at run time, under `.claude-review/` and excluded from git. A reusable workflow cannot see the ref it was called at — `job_workflow_sha` is exactly that, but actionlint 1.7.7 rejects the property and this repo's CI runs actionlint — so the ref is the `tooling-ref` input, defaulting to `master` like every current caller. - The concurrency group is distinct from `claude-review.yml`'s, so a repo trialling the new one does not have each workflow cancel the other's runs. They still must not both run on one PR: both post as github-actions[bot], and `gh pr comment --edit-last` would edit whichever summary that bot wrote last. CI grows a `review-assets` job. Nothing in this repo executes those files — the consumer does — so without it a syntax error or a renamed path would first show up as someone else's review job dying halfway, which is the one failure a gate must not have. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MMo51bM6QV14CK3Kgyp3DQ
truenas-ui-components and api-client-ts both run semantic-release off the squashed PR title, and both carried their own Conventional Commits gate to protect it. The README said the former was "the only repo running semantic-release"; that stopped being true when api-client-ts started publishing. The two copies had drifted in the one place that matters. The optional `<ticket> / <version> / ` prefix was `.+ / ` in ui-components and `[^:]+ / ` in api-client-ts, and the greedy version swallows the real type: in "fix: adjust a / b: c" it matches "fix: adjust a / " and leaves "b" as the type. The gate passes, semantic-release sees a type with no release rule, and the merge publishes nothing — a failure that shows up as an absence, which nobody is watching for. The shared one takes the strict pattern. That makes this a release gate wearing a style gate's clothes, so the caller's .releaserc.json parserOpts has to match it. There is no way to enforce that from here; the README and the workflow header both spell out the pattern to keep in sync, and adopting repos change their .releaserc.json in the same PR. Checked against the last 40 merged titles in both repos: no title changes verdict or parsed type, other than the colon case above. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MMo51bM6QV14CK3Kgyp3DQ
This repository published `check-ticket.yml` without ever running it. PRs here were the only ones in the set not required to name a ticket, and the workflow was the one published file nothing executed before a merge. `pr-ticket.yml` calls it by relative path, the way ci.yml's self-test calls check-member.yml, so a PR that breaks the gate fails its own check instead of surfacing in a consumer's next one. It is a separate file from ci.yml because of the trigger. The check needs `edited` — retitling is how a red check gets fixed, and without it the corrected title never re-runs — and ci.yml must not take `edited` in exchange, or actionlint, input-refs, review-assets and the self-test would rebuild every time someone edited a description. `pr-title.yml` stays unadopted. It matters only where a squash merge feeds the PR title to semantic-release, and there is no package.json or .releaserc here to release from. Prefixes stay at the default of NAS, which is all this repo's history uses. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011tXnFePKrH8VRg4uYzJkJq
bugclerk
changed the title
Contributor
…lure The gate's message for an empty structured output said the run "failed or was cut short", and pointed at the review step. On the PR that adopts this workflow that reads as a broken reviewer, when it is neither: the review step reports success in a few seconds and reviews nothing. claude-code-action refuses to run when the pull request changes the workflow file that invokes it — the guard that stops a PR editing its own reviewer — so it skips, emits no structured output, and this gate fails closed on it. Confirmed on iXsystems/truenas-ui-components#175 and truenas-connect/ui#370, where every other step passed and the action logged "Skipping action due to workflow validation". Two things about it are easy to get wrong, so both are now stated: the ref a caller is pinned to is irrelevant, and re-running cannot help. The calling workflow has to reach the caller's default branch first, which makes this a once-per-repo red check on the migration PR. The fail-closed behaviour is unchanged and correct — this only stops it misdirecting the reader. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011tXnFePKrH8VRg4uYzJkJq
william-gr
reviewed
Aug 11, 2026
william-gr
left a comment
william-gr
left a comment
Contributor
There was a problem hiding this comment.
Why are we keeping the old claude review?
Publishing both was meant to let a repo keep the review it knew while trying the new one. William's call on review, and it is the right one: two workflows doing one job is a choice every consumer has to make and then keep making, and if the gated review turns out to be wrong the fix is to change it here — git history is the revert path, not a second file kept alive in case. So claude-review.yml goes, and with it everything written around the premise that a caller picks between two: - the comparison table now reads against the inline claude.yml each consumer actually has, which is what a migration is measured against; - "do not run both on one PR" becomes "do not keep your inline review running alongside this one" — the comment collision is real either way, but it is a migration hazard now, not a menu; - the tooling-ref note no longer says "gated only", there being nothing to distinguish it from. The file name keeps the -gated suffix. It describes what the workflow does rather than which of two it is, and renaming it would break the three open migration PRs that already name this path. Adoption table now tracks the three migration PRs, and says plainly that each will show one red Automatic PR review — the PR that installs the reviewer is the one PR it cannot run on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011tXnFePKrH8VRg4uYzJkJq
…l copy The comment-only variant never reached master — it was added and removed within this branch — so saying it "was published alongside" this one describes something no reader of master ever saw. Reworded, and it now says where the `-gated` in the file name came from, since that is the only trace the name still carries. The adoption note also called truenas-ui-components' local check-member.yml a byte-identical copy of the one here. It is not, and has not been since the payload guard landed: that copy is the earlier version, which throws on a non-PR event instead of reporting 'false'. Its migration deletes it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011tXnFePKrH8VRg4uYzJkJq
The `-gated` suffix only ever meant "not the other one", and there is no other one. With a single review workflow the suffix reads as a promise that a non-gated variant exists somewhere, which is the question it now makes a reader ask. Also drops the paragraphs explaining that a comment-only variant was written and dropped. It never reached master, so on master it describes something no reader has seen — archaeology about a file that only ever existed on this branch. Git history has it, which was the argument for dropping the file in the first place. Renamed rather than left alone because nothing on master references either name yet: the three migration PRs that name this path are all still open, and are updated alongside this. The same rename after they merge would be a breaking change to every consumer. `ci.yml`'s tooling-path check greps this filename, so it moves too — otherwise it would silently check nothing and pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011tXnFePKrH8VRg4uYzJkJq
…s it
Every migration PR was failing this gate, and the cause was not the review.
Left unset, the action exchanges its OIDC token for an Anthropic GitHub App
token, and setupGitHubToken only reaches that exchange when no token was
provided:
const providedToken = process.env.OVERRIDE_GITHUB_TOKEN;
if (providedToken) return providedToken; // no exchange, no validation
The exchange is the only caller of the workflow-file validation — the check
that the calling workflow matches the version on the default branch. So a
PR that adds or migrates claude.yml could not be reviewed by it: the action
skipped, the step went green in four seconds having done nothing, and the
threshold gate fails closed on empty output. Confirmed on
truenas-ui-components#175 and truenas-connect/ui#370.
Guarding the exchange that way is reasonable — a PR should not mint an app
token for a workflow nobody has merged. It just has nothing to do with
reviewing, and GITHUB_TOKEN carries the job's own `permissions:` block,
which is where that restriction already lives.
What this costs: comments come from github-actions[bot] rather than the
Claude app, which is the identity mark-review-stale.mjs already edits with;
and on a fork PR GITHUB_TOKEN is read-only, so posting would fail there.
require-write-access skips fork PRs from non-members, leaving a write-access
author working from a fork as the one case this does not serve.
The gate's own message pointed at "the PR changes the workflow file" as an
expected, unavoidable failure. It is neither now, so it says what a
recurrence would actually mean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011tXnFePKrH8VRg4uYzJkJq
AlexKarpov98
changed the title
Findings are interpolated straight into workflow-command position, and every
field in one is written by the model. Workflow commands are line-oriented, so
a newline in a summary ends the annotation and the runner reads the next line
fresh:
::error file=a.ts,line=3::MEDIUM: broke it
::stop-commands::xyz
The second line is a command, not text. `maxLength: 200` in the schema bounds
how much text arrives, not which bytes, and nothing validates the payload
against that schema before this script reads it — `structured_output` is
whatever the action emitted.
The likelier version of the same bug needs no adversary: a summary that wraps
onto a second line loses everything after the break, and the annotation on the
diff shows half a finding.
So the message is escaped per GitHub's own rules (%, CR, LF), and property
values additionally for `:` and `,`, which is what separates `file=` from
`line=` — a path containing either used to truncate the properties. `%` is
substituted first, or it would re-escape the escapes.
`where` can be empty, since `file` is required by the schema and not enforced
here, so its space is appended with it rather than interpolated around it:
`::error ::…` was a command with a trailing space in its name.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EsxwaA1vMSLJA2m1iQd3JW
The previous summary was found by taking the newest comment whose body contains the reviewed-sha marker, with no filter on who wrote it. The marker is in the body, so it travels: quote the summary to reply to it, or to disagree with a finding, and your comment now carries it too — and being newer, wins. The step then PATCHes that comment, prepending a superseded banner to words someone else wrote. It succeeds, because GITHUB_TOKEN carries `issues: write` across the repo rather than over the bot's own comments. Meanwhile the real summary is left unmarked and goes on reading as current, which is the single thing this step exists to prevent. Filtering on `user.type === 'Bot'` rather than a login, because the identity the summary is posted under has already moved once — the Claude app before this workflow passed `github_token`, github-actions[bot] after — and this should not have to move with it. It also matches what the reviewer itself does: `gh pr comment --edit-last` is scoped to the token's own comments. Second, smaller: the marker is re-matched against the banner-stripped body, so a match on the raw body does not guarantee one here. It threw a TypeError into the catch, which reported "could not mark the previous review stale: Cannot read properties of null" — a message about a null, not about a comment. Exercised against a stubbed API: with a human quoting the summary after it, the old code patched the quote and the new code patches the summary; a marker that only ever appears in a human's comment now reports nothing to mark. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EsxwaA1vMSLJA2m1iQd3JW
…p it A skipped job satisfies a required status check. That is the whole of it: if a caller marks `Automatic PR review` required, every path that skips the job reports green, and the check says a PR was reviewed when nothing reviewed it. Two of those paths are meant to skip — the label, and an author without write access. A failed check-member was not. `is_member` comes back empty, the condition is unmet, the job skips, and the merge gate quietly opens. The comment here claimed that case "stays fail-closed either way", which was true of the thing check-member guards (no review runs, no tokens are spent on a stranger's PR) and false of the gate the workflow is named for. So a failed check-member is now admitted and rejected in the job's first step, before the checkout. The failure lands on the required check instead of being absent from it, and a runner start costs seconds and no API tokens. Narrow in practice, which is why it was worth being explicit about rather than leaving to inference: check-member catches a permission lookup it cannot make and answers 'false', so reaching this step means the job itself died — runner or action infrastructure, and a re-run. The step's message says so, since the reader arriving at it has a red check and no findings to explain it. skip-label keeps the same skips-as-green property deliberately; its input description already says so, and the README now says it for all three paths in one place, next to the decision it bears on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EsxwaA1vMSLJA2m1iQd3JW
william-gr
approved these changes
Aug 11, 2026
Contributor
|
This PR has been merged and conversations have been locked. |
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
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.
What this is
Aaron asked for the review running in
truenas/api-client-tsto be published here so the other repos can call it. William's call was to not keep the old comment-only review alongside it — if we don't like this one we change it rather than carry two. So this publishes one review workflow; the comment-only variant was written, reviewed and dropped before it ever reachedmaster.What lands
.github/workflows/claude-review.ymlreview/schema.jsonreview/rubric.mdreview/check-review-threshold.mjsreview/collect-review-threads.mjsreview/mark-review-stale.mjs.github/workflows/pr-title.yml.github/workflows/pr-ticket.ymlcheck-ticketon its own PRs.github/workflows/ci.ymlHow a repo calls it
The caller keeps its own
on:trigger — branch filters andpaths-ignorediffer per repo, and aworkflow_callhas no say in what triggers its caller. The secret is named rather than inherited because the repos call it different things (CLAUDE_API_KEYvsCLAUDE_TOKEN).Decisions worth a second opinion
prompt-file, which keeps saying what to look for in that codebase.tooling-ref.review/is checked out into the caller's workspace at run time, and a reusable workflow cannot see the ref it was called at.github.job_workflow_shais exactly that, but actionlint 1.7.7 rejects the property and our CI runs actionlint — so it is an input, defaulting tomaster.uses:does not evaluate expressions, and a configurable version is how the consumers ended up on v1.0.182, v1.0.154 and v1.0.134. Bump it here and every caller moves.The
github_tokenline is load-bearingWithout it every migration PR fails, and not for a reason that looks like the review:
That is the OIDC→app-token exchange refusing, not the review failing —
setupGitHubTokenonly reaches the exchange when no token was provided. A sensible guard on Anthropic's own credentials, but it meant the PR that installs the reviewer was the one PR it could never run on: the step went green in ~4 seconds having done nothing, and the threshold gate correctly failed closed on empty output.Passing
github_token: ${{ github.token }}skips the exchange. GitHub has already scoped that token to the job'spermissions:block, which is where the equivalent restriction belongs. Costs: comments come fromgithub-actions[bot]rather than the Claude app (which is the identitymark-review-stale.mjsalready edits with), and on a fork PRGITHUB_TOKENis read-only —require-write-accessskips those anyway, leaving a write-access author working from a fork as the one gap.Verified live
Running against
iXsystems/truenas-ui-components#175, pinned to this branch: inline comments on the right lines, one summary comment carrying thereviewed-shamarker the stale banner keys on, structured output scored by the gate, and a MEDIUM finding that correctly identified the temporary branch pin in that PR's own workflow file.Follow-ups, not in this PR
Migrations, one small PR per repo:
truenas-ui-components#175,truenas-connect/ui#370,api-client-ts#33. Each replaces its inlineclaude.yml— a repo must not run both, since both post asgithub-actions[bot]andgh pr comment --edit-lastwould edit whichever summary that bot wrote last.truenas/webuihas not been started.Note for whoever merges
Consumers reference
@master, so anything landing here is live in every consumer immediately,review/included —tooling-refdefaults tomaster, so an edit to the rubric changes how every review grades on its next run.