fix(claude-review): post no review-count comment when the cap is disabled by kyle-sexton · Pull Request #621 · melodic-software/ci-workflows

kyle-sexton · 2026-09-24T17:11:49Z

No related issue: operator request to drop the review-count status comment every fleet caller disables.

Summary

Every caller in the fleet sets max-reviews-per-pr: 0, yet the lane still looked up and upserted the "Claude has reviewed this PR N times" comment after every review. With the cap disabled the comment carries a count nothing reads.

Fix

Check the per-PR review count returns before the comment lookup when the cap is <= 0.
Update the review-count status comment returns before any write when the cap is <= 0; its body now always states the cap, since it only runs with one.
A positive cap keeps the comment and gate unchanged.
Input description and README updated.

The security lane posts no count comment; its last-reviewed head marker is functional (incremental relevance) and is untouched.

Verification

New tests in claude-review-outcome-wiring.test.cjs execute both step scripts against a recording client: cap 0 makes no API call (no read, no create/update); cap 5 still lists comments and creates one.
node --test .github/scripts/*.test.cjs: 264 pass, 0 fail. actionlint clean.

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-24T17:13:34.061917Z	`74d37c8`	Draft marked ready

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

claude · 2026-09-24T17:12:14Z

Claude finished @kyle-sexton's task in 2m 2s —— View job

Claude code review

Gather context (PR diff, changed files)
Review claude-review.yml workflow changes
Review claude-review-outcome-wiring.test.cjs test changes
Review README changes
Post findings

No high-signal findings. Traced the fix end-to-end and it holds up:

Check the per-PR review count (L594) now short-circuits on !capEnabled before touching listComments, and still unconditionally sets capped=false / count=0 first, so every downstream step gating on steps.review-count.outputs.capped != 'true' (11 call sites) behaves identically whether the lookup ran or was skipped.
Update the review-count status comment (L1369) independently re-derives the same max > 0 predicate and returns before reading PRIOR_COUNT/PRIOR_COMMENT_ID, so it can't run with a stale/absent comment-id from the now-skipped lookup.
The removed count >= max → capEnabled && count >= max guard is a no-op simplification: the function already returns above when !capEnabled, so dropping the redundant check doesn't change behavior.
The always-included cap note in the upsert body (`...skips further automatic reviews after ${max}...`) is safe because that step no longer runs at all when max is disabled — no more "which cap?" branch to keep in sync.
README (L896-901) and the input description (L259-266) were updated in step with the code; no stale claims about the comment "doubling as the human signal" when the cap is off.
New tests (claude-review-outcome-wiring.test.cjs) extract each step's script body via the same indentation-anchored slicing pattern already used elsewhere in the file and exercise both steps against a recording client for cap 0 and cap 5 — the assertions (calls arrays) match what the script logic above actually does.

One note, not a finding: the max > 0 predicate is duplicated verbatim across the two independent github-script blocks (no shared JS context between steps), which is inherent to the format rather than something this PR introduced or could reasonably factor out.

I was not able to independently execute node --test .github/scripts/*.test.cjs or actionlint in this environment (no tool access to run them here) — the PR body's "264 pass, 0 fail" / "actionlint clean" claims are author-reported and unverified by me. The logic trace above is my own independent check of the diff, not a restatement of that claim.

claude · 2026-09-24T17:12:21Z

Claude finished @kyle-sexton's task in 1m 45s —— View job

Security review

Gather context (PR body, changed files)
Diff .github/workflows/claude-review.yml, README.md, and the new test file against origin/main
Check for trust-boundary / injection / permission changes
Post findings

Scope: this PR only changes the review-count cap logic in two actions/github-script steps (Check the per-PR review count and Update the review-count status comment) plus docs/tests. No workflow permissions:, secrets, action pins, or fork-handling (HEAD_REPO check) were touched.

Findings: none.

Both scripts read max-reviews-per-pr via env: (MAX_REVIEWS_PER_PR) rather than interpolating it into the script body, so no script-injection surface is introduced or changed. That input is caller-supplied config, not attacker-controlled PR content.
The new early-return (if (!capEnabled || ...) return; in claude-review.yml#L594, if (!(Number.isFinite(max) && max > 0)) { ...; return; } in claude-review.yml#L1369) is logically equivalent to the prior capEnabled && count >= max gating for the capped/count outputs, just skipping the now-unnecessary API round-trip. No fail-open/fail-closed change to the actual review gate.
The fork guard (HEAD_REPO check before writing the comment) is untouched, so a fork-triggered run still can't hit the write path with an escalated token.
The security lane (claude-security-review.yml) is unmodified, so its independent "last-reviewed head" freshness marker is unaffected, consistent with the PR description.
New tests in claude-review-outcome-wiring.test.cjs extract and execute the step scripts against a stubbed github/core, not live credentials or untrusted input — no concern there.

No security-relevant issues identified in this change.

github-actions · 2026-09-24T17:14:25Z

Last security-reviewed head: 74d37c82a9a0609446bf96136979cd2b1758c052. On the next push, the relevance gate compares only the commits since this SHA; delete this comment to force a full re-review.

github-actions · 2026-09-24T17:14:34Z

Claude has reviewed this PR 2 times. The lane skips further automatic reviews after 5; deleting this comment resets the count.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

claude · 2026-09-24T17:15:39Z

Claude finished @kyle-sexton's task in 2m 5s —— View job

Claude code review

Gather context (PR diff, changed files)
Review claude-review.yml workflow changes
Review claude-review-outcome-wiring.test.cjs test changes
Review README changes
Post findings

No high-signal findings.

The only change since the last review (74d37c8 → ff951d9) is a Biome reformat of runCountStep's two multi-line arrow functions in claude-review-outcome-wiring.test.cjs (git diff 74d37c8..ff951d9 touches only whitespace/line-wrapping) — no logic changed. I independently re-traced the full diff against origin/main rather than just diffing the last commit, and it holds up:

Check the per-PR review count short-circuits on !capEnabled before listComments, still setting capped=false/count=0 first — every one of the 12 downstream steps.review-count.outputs.capped != 'true' gate sites behaves identically whether the lookup ran or was skipped.
Update the review-count status comment independently re-derives the same max > 0 predicate and returns before touching PRIOR_COUNT/PRIOR_COMMENT_ID, so a re-enabled cap self-heals its count from the marker comment's own body rather than trusting stale output — no drift across a disable/re-enable cycle.
The new tests in claude-review-outcome-wiring.test.cjs extract each step's script via the same indentation-anchored slicing already used elsewhere in the file and assert the calls array against a recording client for cap 0 (no API calls either step) and cap 5 (listComments then createComment) — the assertions match what the script logic actually does.
README (L896-901) and the input description (L259-266) match the new behavior; no stale claim about the comment doubling as the human signal when the cap is off.

I was not able to execute node --test .github/scripts/*.test.cjs or actionlint in this environment (no Bash execution permission here) — the PR body's "264 pass, 0 fail" / "actionlint clean" claims are author-reported and unverified by me. This matches what the two prior review passes on this PR (Claude code review and Claude security review, both on 74d37c8) already concluded, and I found nothing new on top of the format-only commit.
· Branch: fix/claude-review-no-count-comment-when-uncapped

No related issue: ships ci-workflows v0.28.1 (no review-count comment when the cap is disabled) to the synced lane callers. ## Summary ci-workflows v0.28.1 stops the code-review lane from reading or posting its "Claude has reviewed this PR N times" comment when `max-reviews-per-pr` is 0 or less. The `claude-lanes` caller component sets 0, so moving its pins to v0.28.1 removes that comment from dotfiles, github-iac, medley and provisioning. ## Fix - `components/claude-lanes/claude-review.yml` and `claude-security-review.yml` pin `cc0462990687534e9597de9e00ab89d3dcca61d2 # v0.28.1`. - `components/runner-policy/policy.json`: contracts for both lanes at `cc046299…`, verbatim copies of the v0.28.0 (`39390344…`) entries. - `components/runner-policy/README.md`: rollout record. `git diff 39390344..cc046299 -- .github/workflows/` touches only `claude-review.yml` (ci-workflows#621); no input, secret, permission or routing moved. The repin lane (#615) declined to copy forward because the `max-reviews-per-pr` description text changed. ## Verification - `npm run test:runner-policy`: 257 pass, 0 fail. `npm run lint:runner-policy`: passed. - `harness/shell/run-tests.sh` over `claude-lanes.test.sh` and `repin-callers.test.sh`: 2 passed. ## Related - melodic-software/ci-workflows#621, release v0.28.1. - #615 (repin lane PR; its remaining delta after this merges is `sync.yml`, `managed-files-guard` and this repo's own caller). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

…ure visibility (#622) No related issue: operator-approved removal of lane bookkeeping. ## Summary Slims `claude-review.yml` (1,462 to 241 lines) and `claude-security-review.yml` (1,749 to 228 lines) to three purposes: running the review (plugin command, `pull_request` triggers, draft skip), security (fork skip, bot skip, privileged-trigger tripwire, least-privilege permissions, one named secret, credential strip, `display_report: false`, `exclude-comments-by-actor`), and failure visibility (`claude-lane-outcome` classification and an always-on status job per lane). ## Fix Removed from both lanes: - **Dispatch re-review:** `pr-number` input, "Resolve PR context", dispatch delivery snapshot/collect, and the `workflow_dispatch` triggers in both self callers. Steps read `github.event.pull_request.*` directly. - **Retry:** the retry gate, back-off, retry attempt, "Resolve the effective review attempt", and `retry-delay-seconds`. One attempt with `continue-on-error` and a step timeout; job timeouts sized for one attempt (15 and 25 minutes). - **Freshness:** the `claude-lane-freshness` step, every `superseded` gate, and the job-level per-head concurrency blocks. Callers own concurrency. - **Kill-switches:** `CLAUDE_LANES_DISABLED`, `CLAUDE_REVIEW_DISABLED`, `CLAUDE_SECURITY_REVIEW_DISABLED`. - **Marker comments:** both infra-status post/clear steps. - **Inputs:** `prompt` (the plugin command text is now inline in `prompt:`), `skip-actors` (replaced by `!endsWith(github.actor, '[bot]')`), and `status-check` (status jobs run on `always()`). Removed from the code-review lane: - **Review-count cap:** `max-reviews-per-pr`, the count check, and the count comment upsert. - **Standards mount:** `standards-ref`, the `STANDARDS_REVIEW_APP_*` secrets, the token/clear/checkout steps, and the `--add-dir` branch. - **Inputs:** `track-progress` (hardcoded true), `display-report` (hardcoded false), `timeout-minutes`, and `allowed-bots`. The #443 author-association clause is unreachable once every bot actor skips, so it is gone and `allowed_bots` is no longer passed. - **Outputs:** `review-ran`. Removed from the security lane: - **Path gating:** `paths`, `paths-file`, the `changes` job with its incremental last-reviewed-head listing (#259), and "Persist the last-reviewed head". The lane runs on every non-draft PR. - **Ruling:** "Rule on an in-scope non-run". - **Outputs:** `relevant`, `review-ran`, `review-failed`, `failure-class`. Shared composites: deleted `claude-lane-freshness` and `claude-lane-marker-comment`. `claude-lane-outcome` drops `dispatch-evidence.cjs`, the `event-name`/`delivery-evidence` inputs, and the `no-delivery` class. The lanes keep their `claude-lane-outcome@ac06265` (v0.27.0) pin. Tests and docs: deleted the tests of removed features; pruned compose-args, status-check, plugin-path, declared-outputs, outcome-wiring and outcome-step; renamed `claude-lane-bot-association.test.cjs` to `claude-lane-job-gates.test.cjs`, which now pins the bot, draft and fork skips and the tripwire for both lanes. Rewrote the README lane sections and deleted `security-review-absent-mitigation.md`. Beyond the brief: - The code-review lane now skips fork PRs like the security lane. With the status check always on, a secretless fork run would otherwise redden `claude-review-status` on every fork PR. - `cursor[bot]` pushes are no longer reviewed. It was in `allowed_bots` on both lanes and passed the #443 clause when the PR author was OWNER, MEMBER or COLLABORATOR; the blanket bot skip now skips it. ## Verification - `node --test .github/scripts/*.test.cjs`: 157 tests, 157 pass, 0 fail. - `node --test .github/actions/claude-lane-outcome/*.test.cjs`: 15 tests, 15 pass, 0 fail. - `actionlint`: clean. - `npx -y @biomejs/biome@2.5.11 check --config-path=fixtures/typescript/good/biome.json` on the 7 changed `.cjs` files: clean. - `npx markdownlint-cli2 README.md`: 0 issues. `typos` on the changed files: clean. `lychee --offline README.md`: 0 errors. - The pinned-revision check in `claude-review-outcome-wiring.test.cjs` ran against `ac06265` (not skipped) and passes: both consumed outputs, `review-failed` and `failure-class`, are declared at that pin. ## Related - #280 - #619 - #621 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

…eview-lane guards (#4465) No related issue: operator-approved removal of review-lane bookkeeping. ## Summary Removes the repo-owned bookkeeping around the two Claude review lanes and the local skill-evidence system from #4210. Each lane is now its caller plus the reusable's `status-check`. The lanes are not re-pinned and `.github/standards/**` is untouched; a later PR does both. ## Fix **Lane callers (CI)** - `claude-review.yml`: removed the `review-skill-evidence` job and its header paragraph. The reusable call's inputs and the `workflow_dispatch` trigger are unchanged. - `claude-security-review.yml`: removed the `skip-actors` and `security-review-evidence` jobs. `skip-actors` is now the inline literal `dependabot[bot],claude[bot],melodic-ai[bot],melodic-standards-sync[bot],cursor[bot]`. - `ci.yml` ci-status: removed the `skill_evidence_checkout` and `skill_evidence` steps, plus their lane-coverage opt-outs. - Deleted `.github/claude-skip-actors`, `scripts/read-skip-actors.sh`, `verify-claude-review-skill.sh`, `verify-security-review-evidence.sh`, `pr-skill-evidence-ci.sh`, their tests, and `scripts/lib/review-lane-guard.sh`. **Skill-evidence system (source-control 0.58.0, breaking)** - Deleted `scripts/skill-evidence.sh` and its suite, and the `pr-ready-evidence-{gate,mcp-gate,verdict}` hooks with their registrations and tests. - Removed the `pr_ready_evidence_gate_enabled` and `skill_evidence_store` options. - Removed the `pr_skill_evidence` key: its grammar in `config-resolution.md`, the setup report row, and the map in `.claude/source-control.md`. - pull-request `ready` now merges the base, runs the security review over the PR diff and the verify gate on the merged head, then flips. It no longer checks or renders evidence. Prep classifies changed files by a table instead of the map. Create no longer writes `branch.<name>.pr-number`. - babysit: removed the PR-body parser, the `skillEvidence` record, the `skill_evidence_gap` worker reason, and their tests. **claude-ops 0.61.0** - `skill-usage.jsonl` rows no longer carry `sha` or `pr`. Their only reader was `skill-evidence.sh`; `audit_skill_visibility.py`, `skill-pair-cooccurrence.sh` and the observability pruner never read them. A store write now spawns 3 git processes instead of 4 (measured with strace). **`.github/claude-security-paths`**: deleted. Its only readers were the map's `security` class and the ci.yml reporter; the security lane stopped reading it under the ADR 0038 addendum. **Records**: dated addenda in ADR 0002 (skip-actors), ADR 0037 (evidence system removed), and ADR 0038 (lanes are the callers plus status-check). Also updated AGENTS.md and both READMEs. ## Verification - `actionlint` and `zizmor --offline` on the three changed workflows: clean. - `scripts/check-lane-coverage.sh --check`: 5 lanes reachable, 60 gate steps fed, 2 opted out. - `scripts/check-changelog-parity.sh --check`, `--check-bump origin/main`, `--check-preserved origin/main`: pass. - `scripts/check-hook-userconfig-argv.sh` and `scripts/check-purged-em-dashes.sh`: pass. - `scripts/check-changed-skills.sh origin/main`: 4 skills checked, 0 failed. - `markdownlint-cli2` on the 22 changed markdown files: 0 issues. `shellcheck` on the changed shell files: clean. - babysit `python3 -m unittest discover`: 703 tests OK. `scripts/run-ruff.sh check` and `format --check` on the 9 changed Python files: clean. - `pr-linkage-spawn-budget.test.sh`: 23 passed. `skill-usage-audit.test.sh`: 31 passed. `claude-ops-paths.test.sh`: 37 passed. - `scripts/affected-tests.sh --run --jobs 16` (306 suites): 9 failed. Re-run on a clean `origin/main` worktree, 8 of them fail there too (markdown-format, the three worktree-create/containment suites, check-html-assets, work-item-tracker, check-script-contract, code-metrics dispatch). The ninth, `lib/hook-utils.test.sh`, passed 524/524 on its own re-run on this branch. ## Related - #4210 (introduced the skill-evidence system) - ADR 0037, ADR 0038, ADR 0002 - melodic-software/ci-workflows#621 (review-count comment dropped when the cap is disabled) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

kyle-sexton marked this pull request as ready for review September 24, 2026 17:11

style: biome-format the review-count step test

ff951d9

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

kyle-sexton merged commit cc04629 into main Sep 24, 2026
30 checks passed

kyle-sexton deleted the fix/claude-review-no-count-comment-when-uncapped branch September 24, 2026 17:49

kyle-sexton mentioned this pull request Sep 24, 2026

chore(claude-lanes): re-pin lane callers to ci-workflows v0.28.1 melodic-software/standards#617

Merged

kyle-sexton mentioned this pull request Sep 24, 2026

feat(claude-lanes)!: slim both review lanes to run, security and failure visibility #622

Merged

kyle-sexton mentioned this pull request Sep 24, 2026

refactor(source-control)!: remove the skill-evidence system and the review-lane guards melodic-software/claude-code-plugins#4465

Merged

Navigation Menu

Repository navigation

fix(claude-review): post no review-count comment when the cap is disabled - #621

fix(claude-review): post no review-count comment when the cap is disabled#621
kyle-sexton merged 2 commits into
mainfrom
fix/claude-review-no-count-comment-when-uncapped

kyle-sexton commented Sep 24, 2026

Uh oh!

chatgpt-codex-connector Bot commented Sep 24, 2026 •

edited

Loading

Uh oh!

claude Bot commented Sep 24, 2026 •

edited

Loading

Uh oh!

claude Bot commented Sep 24, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 24, 2026

Uh oh!

github-actions Bot commented Sep 24, 2026 •

edited

Loading

Uh oh!

claude Bot commented Sep 24, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

kyle-sexton commented Sep 24, 2026

Summary

Fix

Verification

Related

Uh oh!

chatgpt-codex-connector Bot commented Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codex Review Summary

Uh oh!

claude Bot commented Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Claude code review

Uh oh!

claude Bot commented Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Security review

Uh oh!

github-actions Bot commented Sep 24, 2026

Uh oh!

github-actions Bot commented Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude Bot commented Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Claude code review

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

chatgpt-codex-connector Bot commented Sep 24, 2026 •

edited

Loading

claude Bot commented Sep 24, 2026 •

edited

Loading

claude Bot commented Sep 24, 2026 •

edited

Loading

github-actions Bot commented Sep 24, 2026 •

edited

Loading

claude Bot commented Sep 24, 2026 •

edited

Loading