Skip to content

chore: revert per-push review trigger; drop synchronize from summary#7

Merged
shuse2 merged 1 commit into
mainfrom
chore/revert-per-push-review-trigger
Apr 29, 2026
Merged

chore: revert per-push review trigger; drop synchronize from summary#7
shuse2 merged 1 commit into
mainfrom
chore/revert-per-push-review-trigger

Conversation

@shuse2

@shuse2 shuse2 commented Apr 29, 2026

Copy link
Copy Markdown
Member

Summary

  • Restore on-demand-only trigger for the PR review caller example (pre-9bc9666 behavior): /review comment or needs-review label. No more auto-run on every PR push.
  • Drop synchronize from the PR summary caller; add paths-ignore for docs.
  • Update README trigger descriptions for both workflows.

Why

Commit 9bc9666 ("fix: wire review agent findings to aggregator via structured_output") bundled an unrelated trigger expansion into the example caller — switching review from on-demand to running on every PR push (opened, synchronize, reopened, ready_for_review). Downstream callers (LiskHQ/lisk-{backend,web,mobile,infra}) copied this and now burn Anthropic tokens at scale:

  • ~27 review runs + ~27 summary runs / 24h across the four downstream repos
  • Each review = 30–67 Anthropic API calls (multi-agent fan-out)
  • One PR (mobi-186-...) racked up 14 review runs in 24h from repeated pushes alone

cancel-in-progress doesn't refund tokens already spent before cancel, so concurrency limits don't help.

What changed

examples/workflows/claude-pr-review-caller.yml

  • on.pull_request.types: [opened, synchronize, reopened, ready_for_review, labeled][labeled]
  • check-trigger.if: removed the auto-trigger branch (opened/synchronize/reopened/ready_for_review); kept the labeled == needs-review and /review comment branches
  • Updated leading comment block + concurrency comment to reflect on-demand semantics
  • Preserved the security hardening from 9bc9666 (env-var refactor of get-pr / get-model steps to prevent shell injection via ${{ }} interpolation)

examples/workflows/claude-pr-summary-caller.yml

  • on.pull_request.types: [opened, synchronize, reopened, ready_for_review][opened, reopened, ready_for_review]
  • Added paths-ignore: ['**/*.md', 'docs/**'] to skip doc-only PRs

README.md

  • Updated trigger descriptions for PR Review (on-demand only) and PR Summary (no synchronize, paths-ignore for docs)

Not in this PR

  • The upstream reusable workflows (.github/workflows/claude-pr-{review,summary}.yml) — both are workflow_call only, no triggers to fix there.
  • The downstream caller files in LiskHQ/lisk-{backend,web,mobile,infra} — those need the same change applied per repo. Will follow up.
  • Deletion of the Summary workflow (overlaps with Review) — flagged for separate decision after monitoring.

Test plan

  • YAML parses (verified locally: python3 -c \"import yaml; yaml.safe_load(open(...))\")
  • After downstream rollout: open a test PR in one downstream repo, push a commit, confirm no review/summary run is triggered
  • Apply needs-review label, confirm review runs
  • Comment /review, confirm review runs (and /review opus//review haiku route to the right model)
  • Open a docs-only PR, confirm Summary skips
  • 24h post-rollout: re-run the per-repo gh run list baseline; expect Review/Summary counts to drop ~5-10×

Restores pre-9bc9666 on-demand-only behavior for the PR review caller
example. 9bc9666 added auto-runs on every PR push (opened, synchronize,
reopened, ready_for_review) bundled into a structured_output fix; that
trigger expansion caused token-burn storms downstream — one observed PR
had 14 review runs in 24h from repeated pushes, and each review fans
out to 30-67 Anthropic API calls.

Review now runs only on:
  - /review comment (optionally /review opus or /review haiku)
  - needs-review label

Summary loses the synchronize trigger (still runs on opened/reopened/
ready_for_review) and gains paths-ignore for docs.

Security hardening from 9bc9666 (env-var refactor of get-pr / get-model
steps to avoid shell injection in ${{ }} interpolation) is preserved.

README updated to match new trigger semantics for both workflows.

Downstream callers in LiskHQ/lisk-{backend,web,mobile,infra} need the
same change applied to their .github/workflows/claude-pr-*.yml files;
this PR fixes the example so future copies don't reintroduce the issue.
@shuse2 shuse2 merged commit a1ade8c into main Apr 29, 2026
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