feat(ci): trigger pr-review on pull_request events instead of hourly cron #362
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: konairius/homelab#362
Reference in New Issue
Block a user
Switches the PR review bot from a homelab-hosted hourly cron that polls four repos over the Gitea API to a per-repo, event-driven pipeline that reviews only the PR that triggered it.
What changes (this diff — homelab only)
when:→event: pull_request(Woodpecker raises this on PR open AND push to an open PR) +event: manualfor on-demand re-review (--var PR=<n>).CI_REPO+CI_COMMIT_PULL_REQUEST. Drops the cross-repo discovery loop andMAX_PRScap.pr-review:sha=dedup marker (now guards pipeline retries), Haiku triage → model pick → review → Gitea post, never-approve posture.Rollout — out of band, NOT in this diff (order matters)
pull_requestevent toclaude_code_oauth_token+review_bot_tokenon every reviewed repo — else Woodpecker withholds them and every PR pipeline fails auth. Do this before merging.homelab --require-approval forks(public repo — fork PRs can't reach the tokens without a maintainer approval click).shelly-manager,authentik-samba-bridge(folder-converted),homeassistant; registerhomeassistantin Woodpecker.pr-reviewcron (id 6) last, once all four PR triggers are verified live.🤖 Automated review — model
claude-sonnet-5(complexity: normal), commitf9c5ab6b1b0236b32e3c0d9e9d363fbc51a61287.Summary
This PR rewrites
.woodpecker/pr-review.ymlfrom an hourly cross-repo-discovery cron into an event-driven, single-PR pipeline triggered by Woodpecker'spull_requestevent (plus a manual--var PR=<n>path), and moves the two agent prompts to a shared location with a shallow-clone fallback for other repos. The control flow (fetch PR → skip draft/WIP/closed → dedup by head-sha marker → clone, triage, review, post) is a faithful, correctly-simplified port of the old per-PR body; I did not find a concrete defect in the shell logic itself. There is one design assumption worth confirming before the "Rollout" checklist proceeds (see Suggestions), and the rollout itself (granting thepull_requestevent to the two tokens on every repo) is correctly called out in the PR body as a prerequisite, not part of this diff.🟡 Suggestions
.woodpecker/pr-review.yml:72—SLUG="$$CI_REPO"replaces the old hardcoded canonical-slugREPOSlist. The comment block this diff deletes (old lines 17-20) explicitly documented a real gotcha: "gitea_admin/homelab 307-redirects to [k.renner/homelab] and a naked POST would not follow the redirect." The new code trustsCI_REPOto already be the canonicalk.renner/*slug for every downstreamcurlcall (PR fetch, reviews list, and — in the second step — the review POST). IfCI_REPOis ever populated with a non-canonical owner alias for any of the four target repos, the dedup GET and/or the review POST would silently fail or redirect-fail, defeating the whole point of this PR. Worth confirming with a realpull_requesttrigger (env | grep CI_REPOin a debug step, orwoodpecker repo info <repo>) before deleting the cron, since there's no test coverage that would catch this otherwise. If it's already known to be canonical, restoring a one-line note to that effect would preserve the institutional knowledge the deleted comment carried..woodpecker/pr-review.yml:39-42— the new comment documents that homelab (public) requires approval for forked-PR pipelines so a fork can't rewrite this file to exfiltrate tokens. The rollout checklist in the PR body defers granting thepull_requestevent (and, implicitly, equivalent fork-approval settings) to the other three repos to a follow-up, out-of-band step. Worth double-checking each of those repos gets the same fork-approval treatment before this file is copied there — the comment currently only justifies the posture for homelab..woodpecker/pr-review.yml:104(REVIEWED=$$(curl -sf ... | jq ... || echo 0)) — carried over unchanged from the old file, but worth noting: in a pipe withoutpipefail,|| echo 0only catches a nonzero exit fromjq, not fromcurl. If the reviews-list GET fails (e.g. transient 5xx),jqstill gets empty stdin and errors too, so the fallback happens to work today — but it's fragile and pre-existing, not introduced here.🟢 Nits
.woodpecker/pr-review.yml:120(PROMPTS="$$WS/ci/agents/pr-review") — if a non-homelab reviewed repo ever grows its ownci/agents/pr-review/prompt.md(e.g. copy-pasted by mistake), it would silently take precedence over the shared-prompt fallback, quietly forking "one source of truth for the review rubric." Not a bug, just a sharp edge worth a one-line comment if it ever bites.Tests
No tests exist for this file or its siblings (
issue-fix.yml,alert-monitor.yml) today — consistent with the rest of.woodpecker/, which has no CI-config test harness. Given the pipeline can't be unit-tested short of actually firing apull_requestevent, the practical verification is the manual--var PR=<n>trigger path this PR keeps, which is the right fallback for a dry run.Recommendation
Looks reasonable — no blocking issues; the CI_REPO canonicalization point above is worth a quick confirmation before deleting the cron (step 4 of the rollout checklist), but it's a pre-merge-worth-checking assumption, not a defect I can point at in this diff.
Woodpecker exposes --var values to YAML ${...} substitution and when-filters only, not as a step's shell environment, so the documented '--var PIPELINE=pr-review --var PR=<n>' re-review read an unset shell var and silently no-opped with a green build (caught by the review bot on PR #49). Trigger on pull_request only; re-review by pushing a commit (new head sha clears the dedup marker). Resolve the PR from CI_COMMIT_PULL_REQUEST alone. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>🤖 Automated review — model
claude-sonnet-5(complexity: normal), commite96056dabdda32b3f8ff4aa521b28e12635425bf.Summary
This PR rewrites
.woodpecker/pr-review.ymlfrom an hourly cron that polls fourrepos for open PRs into an event-driven pipeline that reacts to a single
pull_requestevent and reviews only the triggering PR (CI_REPO+CI_COMMIT_PULL_REQUEST). It also relocates the agent-prompt lookup so the filecan be deployed unchanged to repos that don't carry
ci/agents/pr-review/locally (falling back to a shallow clone of
k.renner/homelab@master). Theshell logic is internally consistent —
set -euis respected, theCI_COMMIT_PULL_REQUEST:-default avoids an unbound-variable failure, thedraft/WIP/state/dedup checks are preserved from the old code, and the referenced
ci/agents/pr-review/{prompt,triage-prompt}.mdfiles exist in this checkout. Ifound no concrete logic bugs in the diff itself. The residual risk is entirely
in things that live outside this repo (Woodpecker's event semantics, and the
per-repo token/approval grants called out in the PR's own rollout checklist),
which is worth flagging explicitly since this is a security-sensitive change
(secrets now execute in response to externally-triggered
pull_requesteventson a public repo).
🟡 Suggestions
PR description doesn't match the shipped diff. The PR body says the
when:block becomesevent: pull_request+event: manualforon-demand re-review (
--var PR=<n>). The actual diff removes the manualtrigger entirely (
.woodpecker/pr-review.yml:50-51in the checkout) and addsa comment explicitly justifying why a manual
--vartrigger wouldn't work(
.woodpecker/pr-review.yml:44-49: Woodpecker only exposes--varto YAML${...}/whenevaluation, not to the step's shell environment). The codeand its own comment are internally consistent and this reasoning looks
correct — the PR body is just stale/wrong on this point. Worth fixing the
description so the PR history doesn't claim a capability that isn't there,
since a future reader (or the next agent) may look for a manual re-review
path that doesn't exist.
Secret exposure now depends entirely on fork-approval being correctly
configured on every reviewed repo, not just homelab. This pipeline runs
with
CLAUDE_CODE_OAUTH_TOKENandREVIEW_TOKENand is now triggered by anexternally-influenceable
pull_requestevent instead of an admin-controlledcron. The comment at
.woodpecker/pr-review.yml:39-42notes homelab is setto
--require-approval forks, and the PR checklist marks that item done forhomelab specifically. The other three rollout targets
(
shelly-manager,authentik-samba-bridge,homeassistant) aren'tmentioned — if any of those are or become public without the same
fork-approval setting, a PR from an untrusted fork could edit this same file
(it's deployed "unchanged" per the file's own header) to exfiltrate both
tokens. Worth either confirming those repos are private, or adding the same
--require-approval forksgate to each before wiring thepull_requestevent grant, per the rollout checklist item that's still unchecked.
Losing the poll may drop PRs that never fire a fresh
pull_requestevent. The old hourly cron would eventually pick up any open PR regardless
of why it became reviewable. The new design only reviews on the event
Woodpecker classifies as
pull_request(per the file's own comment: PR openreview" without a new commit may or may not fire that event depending on
how Woodpecker maps Gitea's
pull_requestwebhook actions — worthdouble-checking against a real draft→ready transition once this is live,
since a silent gap here is hard to notice (no error, just no review posted).
🟢 Nits
.woodpecker/pr-review.yml:126-129— the fallbackgit cloneofk.renner/homelabfor prompts isn't checked for success before use. If itfails, the next
cat "$$PROMPTS/triage-prompt.md"will die underset -euwith a generic "no such file" error rather than a clear "could not fetch
agent prompts" message. Not a functional bug (it still fails loud, not
silent), just a slightly less helpful failure mode than the rest of the
script's explicit
|| { echo ...; }error messages.Tests
No tests exist for this file, consistent with the repo's other Woodpecker
pipeline definitions (
alert-monitor.yml,issue-fix.yml,security-scan.ymletc. are all similarly untested shell-in-YAML). Given this is a security- and
cost-sensitive pipeline (spends Claude tokens and posts to Gitea automatically
on external triggers), the realistic verification is a live smoke test after
merge: open a throwaway PR, confirm the pipeline fires once, posts one review,
and that pushing a follow-up commit produces exactly one fresh review rather
than a duplicate. The PR's own rollout checklist already calls for this kind of
staged verification, which is the right approach here.
Recommendation
Looks reasonable — no blocking issues; comments above (especially confirming
fork-approval on every rollout target before granting the
pull_requesteventto the tokens) are worth addressing before flipping this on for the other
repos.
🤖 Automated review — model
claude-opus-4-8(complexity: complex), commit2a6506667293e98f804564b9949971864643b13c.Summary
Converts the PR-review pipeline from an hourly cross-repo cron to a per-repo,
event-driven
pull_requesttrigger, and updates the two agent prompts to match(the reviewer now reads the Woodpecker checkout in its CWD instead of a
hand-cloned
work/target). The change is coherent and internally consistent —the prompt edits, the removed manual clone, and the removed
work/cleanup allline up — and it genuinely removes the write-scoped token that the old model
persisted in
work/target/.git/config. No blocking issues; the notes below areverification points and optional hardening.
🟡 Suggestions
Correctness depends on
CI_REPObeing the canonicalk.renner/…slug(
.woodpecker/pr-review.yml:76,SLUG="$$CI_REPO"). The old code hardcodedk.renner/*specifically because it noted a 307 redirect from thegitea_admin/homelabslug that a naked request won't follow (old comment,now deleted). Every
curlhere uses-sfwithout-L, so if Woodpeckerever surfaces a non-canonical owner slug in
CI_REPO, the first GET at.woodpecker/pr-review.yml:88returns the redirect body instead of JSON andthe subsequent
jq -r '.state'aborts underset -eu. It will fail loudlyrather than post a wrong review, so this is low-risk — but worth confirming
once that
CI_REPOfor these four repos is exactly the canonical owner theGitea API accepts without redirect.
Homelab self-reviews load the rubric from the PR-head checkout
(
.woodpecker/pr-review.yml:129-130):PROMPTS="$$WS/ci/agents/pr-review"is used whenever
prompt.mdexists in the workspace, which for a homelab PRis the PR's own (possibly modified) copy — exactly what PR #362 itself
edits. So a PR can be reviewed under a rubric it changed. This adds no
incremental risk beyond Woodpecker's existing trust model (an approved fork
pipeline already runs fork-controlled
.woodpeckerconfig wholesale, and thePR body confirms
--require-approval forks), and the private repos correctlyfall back to
homelab@masterprompts. Flagging only so the dependency on thefork-approval gate is explicit.
Fallback prompt clone has no error handling (
.woodpecker/pr-review.yml:132).Under
set -eu, a transient failure ofgit clone … homelababorts thewhole step with no review posted (only affects the non-homelab repos, which
always take this path). Consider
|| { echo "prompt clone failed — skipping"; exit 0; }so an unrelated network blip doesn't hard-fail the pipeline.
🟢 Nits
pull_requestevent may check out amerge-ref rather than the bare head sha, while
diff.patch, the reviewcommit_id, and the dedup marker are all anchored to.head.sha(
.woodpecker/pr-review.yml:228,231). The on-disk context the agent readscould therefore differ slightly from the head diff. Almost always immaterial
(the diff is authoritative and the agent only uses the checkout for context),
but it's the one place the "checkout == head" claim in the new prompt could be
imprecise.
Tests
This is CI/infra YAML with no unit-test harness in the repo, so the absence of
tests is consistent with the surrounding conventions. The real safety net is the
staged rollout, which the PR body documents well and in the right order (grant
the
pull_requestevent to both secrets before merge, deploy the file tothe other repos, delete cron id 6 last). The one item worth double-checking
live is the
CI_REPOslug point above — the cheapest verification is a singlemanual PR-event run on one private repo before deleting the cron.
Recommendation
Looks reasonable — no blocking issues; comments above are optional.