chore(ci): tag releases, and judge a review by what it posted - #210
Conversation
Two release-process fixes. **Tag non-nightly releases.** A changelog could not be read as a range, because nothing marked where a release landed. A new `tag` job creates `v<version>` at the released commit. It is a separate job from `publish` on purpose: `publish` holds `id-token: write` for OIDC trusted publishing and tagging needs `contents: write`, and granting both to one job would mean the job that can publish to npm can also move refs here. It takes no checkout either, so no credential is written into a working tree. Nightlies are excluded structurally, by being published from another workflow, rather than by an `if:` that has to stay correct. **Stop failing a review for taking zero turns.** The guard failed the run when `num_turns` was 0, which held while the two dead reviews on record (#182, #196) both reported it. That correlation broke: #208 and #209 both reported `num_turns: 0` alongside `subtype: "success"`, two minutes of wall clock and over $2 of billed inference, having posted full reviews whose findings were real and were acted on. Both went red anyway, which is this step's own failure mode pointed backwards, and a check that is red when everything worked gets ignored exactly like one that is green when nothing happened. The metric was never the thing worth asking about. The step now asks the outcome: did this run leave a completed review on the pull request? Every comment the action posts embeds its job URL, and the prompt already requires the top-level comment to state the review mode, so `runs/<id>` plus `Review mode:` identifies this run's completed review. Verified against the record: #182 posted 167 characters reading "I'll analyze this and get back to you" and carries no marker, while #196, #208 and #209 all carry it. The execution file is now optional, since upstream does not always write it (anthropics/claude-code-action#1226). Denials drop to a warning. A denial on a review that posted is a completeness caveat, and a run that posts nothing already fails. All 22 denials across #208 and #209 were the model reaching for `git`, which is not on the allowlist and will not be: it cannot be safely scoped, since `-c diff.external=`, `-c core.pager=` and `--exec-path` all reach arbitrary execution from a read-only-looking subcommand. The prompt now says so and points at `gh pr diff`, and tells the reviewer not to describe a `git diff` it did not run, which both reviews did in their own checklists.
|
@claude /review Specifically, please check the instructions against how tool selection is performed and ensure language is strong enough to reduce the likelyhood of reaching for git |
|
Claude finished @thecodedrift's task in 5m 40s —— View job Review mode: incremental — read 0 prior review thread(s) before reviewing. Focused ask: is the new "don't reach for
|
Three points from review on #210. The run-id match was a bare substring, so it also accepted any id this one is a numeric prefix of: `runs/123456` matches `runs/1234567`. Today's ids are 11 digits and a collision needs a 12-digit one, so it is not reachable yet, but identifying THIS run's comment rather than an older review's is the entire job of that check, and "exact except for numeric-prefix collisions" is not that. Anchored on the right. The tag idempotency check asked only whether the tag existed, so a tag pointing at the WRONG commit would have been read as success. That is the exact state the job exists to prevent: the value of tagging is that a compare range can be trusted without hunting for the version-bump commit, and a misplaced tag makes that silently false while every check stays green. It now compares the existing ref's SHA and fails on a mismatch. It deliberately does not move the tag, since re-pointing a published tag changes history under anyone who already fetched it, and failing costs nothing that matters because the publish is a separate job that has already succeeded. The prompt told the reviewer what to use for the diff and for metadata but named no substitute for file history, while the sentence beside it pushes exactly the "why is this line like this" investigation that history serves. Being told only what you cannot use is what makes an agent keep trying, so it now says plainly that history is unavailable and that findings must rest on the diff and the current file contents.
All three inline findings were real and are fixed in
The second now fails rather than warning, for the reason your comment gives: a tag that cannot be trusted defeats the point of tagging, and a warning inside a green run is how that gets ignored. One half of the third is deliberately still open. You noted that if the prompt change does not eliminate denials, nothing would surface it — the same blind spot this PR just closed for — AI Coding Agent |
The prompt told the reviewer git was unavailable without saying what to use instead, which is the state most likely to make it keep trying. It now maps each command earlier reviews actually attempted onto the allowed tool that answers it: git diff origin/main...HEAD -> gh pr diff git log origin/main..HEAD -> gh pr view --json commits git log -1 --format=%H -> gh pr view --json headRefOid Those are measured, not guessed. They are what the runs on #208 and #209 reached for and had refused, and every one was already reachable through a tool on the allowlist, so the model was never blocked on anything. All four `gh pr view --json` fields were confirmed against a live PR before being written into the prompt. No change to --allowedTools: `gh pr view` was already there. Per-file history is the one real gap and is now described as such rather than left implicit. Also records the two rejected ways of giving git back, so the question is not re-opened from scratch. Prefix-scoping `Bash(git diff:*)` does miss the `-c`/`--exec-path` vectors, since those strings do not start with `git diff`, but it rests the guarantee on how the matcher treats `&&` and `$(…)` rather than on a capability boundary. Wrapper scripts close git's flag surface and do nothing about that same question, which belongs to the matcher rather than to git. If per-file history is ever genuinely needed, the note points at a workflow step precomputing it into a file, the shape `Fetch prior review threads` already uses.
|
@claude /review Vet the new changes - see if they satisfied your concerns from your last review |
|
Claude finished @thecodedrift's task in 5m 36s —— View job I'll analyze this and get back to you. |
The denials were not the model being careless. The action injects system-level instructions telling it that it can stage, commit, push, `git rm`, `git status` and `git diff`, ending with: IMPORTANT: For PR diffs, use: Bash(git diff origin/main...HEAD) That is the literal string the runs on #208 and #209 attempted and had refused, and the checklist entry claiming a `git diff` had run was reporting the step it had been instructed to take rather than inventing one. Those instructions are written for the code-WRITING mode and are injected regardless of what `--allowedTools` grants; this invocation is review-only, with no git and no `contents: write`, so none of them hold. The correction moves to `--append-system-prompt`, because a correction in `prompt:` alone is a user turn arguing with a system prompt. The action exposes no way to suppress the base prompt, so contradicting it at the same level is the available fix. The `prompt:` mapping stays: one wins the contradiction, the other names the substitute for each command, which is what stops the model looking for another way round. Verified the args tokenize as four tokens, so the appended prompt reaches the CLI as one argument rather than splitting on its punctuation.
|
@claude /review Vet the new changes - see if they satisfied your concerns from your last review |
|
Claude finished @thecodedrift's task in 4m 41s —— View job I'll analyze this and get back to you. |
Three points from review on #210. The run-id match was a bare substring, so it also accepted any id this one is a numeric prefix of: `runs/123456` matches `runs/1234567`. Today's ids are 11 digits and a collision needs a 12-digit one, so it is not reachable yet, but identifying THIS run's comment rather than an older review's is the entire job of that check, and "exact except for numeric-prefix collisions" is not that. Anchored on the right. The tag idempotency check asked only whether the tag existed, so a tag pointing at the WRONG commit would have been read as success. That is the exact state the job exists to prevent: the value of tagging is that a compare range can be trusted without hunting for the version-bump commit, and a misplaced tag makes that silently false while every check stays green. It now compares the existing ref's SHA and fails on a mismatch. It deliberately does not move the tag, since re-pointing a published tag changes history under anyone who already fetched it, and failing costs nothing that matters because the publish is a separate job that has already succeeded. The prompt told the reviewer what to use for the diff and for metadata but named no substitute for file history, while the sentence beside it pushes exactly the "why is this line like this" investigation that history serves. Being told only what you cannot use is what makes an agent keep trying, so it now says plainly that history is unavailable and that findings must rest on the diff and the current file contents.
Two release-process fixes, plus four retroactive tags already pushed outside this PR.
标签 (already pushed, no code here)
Nothing marked where a release landed, so a changelog could not be read as a range. The remote had zero tags:
v0.9.0existed only in a local checkout and had never been pushed, despite the README saying tagging started there.v0.9.0e2aee51v0.10.01ca5b6fv0.10.13138049v0.10.20f55da9Each was located by the commit that changed
packages/cli/package.json's version, and each was verified against its parent's version rather than by matching the commit subject, since every one of them is calledchore: version packages.v0.10.2..mainis 347 commits.Tagging future releases
A new
tagjob createsv<version>at the released commit.Separate from
publishdeliberately.publishholdsid-token: writefor OIDC trusted publishing; tagging needscontents: write. Granting both to one job would mean the job that can publish to npm can also move refs in this repository. Nothing here needs the npm identity and nothing there needs to write refs, so they stay apart. The tag job takes noid-token, and no checkout at all — the ref is created through the API, so there is no working tree and no credential in a.git/config.Nightlies are excluded structurally, by being published from
release-cli-nightly.yml, rather than by anif:that has to stay correct.Idempotent for the same reason the publish step re-checks npm: a re-run on an already-tagged release is a duplicate to absorb, not a failure.
标签 are lightweight and therefore unsigned. Accepted: they are pointers, and
main's own commits are already rewritten and unsigned by rebase-and-merge, so a signed tag over them would attest to nothing the commit does not already say.The review guard was failing good reviews
It failed the run when
num_turnswas 0. That held while the two dead reviews on record (#182 at $1.08, #196 at $2.20) both reported it. The correlation has since broken in the direction that matters: on #208 and #209 the SDK reportednum_turns: 0alongsidesubtype: "success", roughly two minutes of wall clock and over $2 of billed inference, having posted full reviews whose findings were real and were acted on in those PRs. Both runs went red anyway.That is this step's own failure mode pointed backwards. A check that is red when everything worked gets ignored exactly like one that is green when nothing happened.
num_turnsis not a liveness signal for the review plugin, and the execution file is unreliable besides — upstream does not write it at all when the SDK throws (anthropics/claude-code-action#1226). So metrics are now diagnostic only, and the pass/fail question is asked of the outcome: did this run leave a completed review on the pull request?Two things make that answerable precisely. Every comment the action posts embeds its own job URL, so
runs/<run id>identifies this run's comment rather than an earlier review. And the prompt already requires the top-level comment to state the review mode, soReview mode:marks a review that reached its own instructions instead of stopping early.Measured against the record:
Review mode:That marker is a contract with the prompt, and the workflow says so: reword the review-mode instruction without changing the check and this step fails every run.
One bug caught before it shipped
The first version of the check piped raw comment bodies to
grep. A body spans many lines, soruns/<id>andReview mode:land on different lines and the second grep never matches. Measured against #208: the raw form finds 0 where it should find 1, so that guard would have failed every review including the good ones.--jq '.[].body | @json'puts each body on one line. Noted in the workflow so it does not get "simplified" back.Denials become a warning
A denial on a review that posted is a completeness caveat; a run that posts nothing already fails above.
All 22 denials across #208 and #209 were the model reaching for
git.gitis not on the allowlist and will not be: it cannot be safely scoped, because-c diff.external=,-c core.pager=and--exec-pathall reach arbitrary execution from a read-only-looking subcommand. The prompt now states that plainly, points atgh pr diff, and tells the reviewer not to describe agit diffit did not run — which both reviews did in their own checklists while the call was in fact denied.If denials become routine again, the fix is the prompt, not
--allowedTools. That is written into the workflow.Verification
The guard was extracted and run against four cases:
is_error: trueEnd-to-end detection against real PRs: #182 →
posted=false, #208 →posted=true. Both workflow files parse as YAML and the step's shell passesbash -n.Notes