ci: dual-trigger pr-title + label workflows for fork-PR coverage#69
Conversation
Adds `pull_request_target` alongside the existing `pull_request` trigger on both `pr-title.yml` and `label.yml`. The dual-trigger pattern sidesteps the chicken-and-egg we hit in #64 (switching a trigger in a PR breaks that PR's own validation cycle) while extending coverage to fork PRs: - `pull_request` runs from the PR head branch — works on internal PRs with full permissions. - `pull_request_target` runs from the default branch with base-repo permissions — gets fork PRs the sticky comment on title failures and the auto-labeling that `pull_request` can't do (GITHUB_TOKEN is read-only on fork PRs under `pull_request`). Both triggers fire on internal PRs, producing a duplicate run. That's intentional and harmless: the pr-title sticky comment is marker-based (marker is the same, content is the same, so the second run is a no-op update), and `actions/labeler` with `sync-labels: true` is idempotent — same label set applied twice is effectively one write. A future follow-up PR can drop the `pull_request` trigger once we've confirmed `pull_request_target` is firing correctly in production. That simplification is tracked as a side note in the workflow headers. Closes #56. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Note Gemini is unable to generate a summary for this pull request due to the file types involved not being currently supported. |
|
Claude finished @EricAndrechek's task in 5m 56s —— View job PR Review — ci: dual-trigger pr-title + label workflows for fork-PR coverage
SummaryThe mechanics are correct and the prior Copilot concerns are all addressed in the current commits. Two minor issues worth noting before merge, neither a blocker. CorrectnessConcurrency group fix (
Orchestrator cleanup (
Security — NONEBoth Minor Issues1. Duplicate sticky comment race (LOW, cosmetic) When both Not a blocker — it's cosmetic and self-correcting on the next push. But worth tracking: if the orphaned-comment problem shows up in practice, the fix is to serialize the comment-step between the two runs (e.g., add a short 2. CHANGELOG.md entry inaccuracy (LOW, pre-existing)
Copilot's Open Second-Review Comments — All Addressed
Ship it — the two issues flagged above don't warrant holding this. The CHANGELOG entry will self-correct after the follow-up PR, and the duplicate-comment race is cosmetic. Copilot's second-review concerns are fully addressed. |
There was a problem hiding this comment.
Pull request overview
Extends CI coverage to fork PRs by adding pull_request_target alongside pull_request for PR title linting and auto-labeling workflows, while aiming to preserve internal PR behavior.
Changes:
- Add dual triggers (
pull_request+pull_request_target) to PR title lint workflow to enable sticky comments on fork PRs. - Add dual triggers (
pull_request+pull_request_target) to labeler workflow to enable auto-labeling on fork PRs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| .github/workflows/pr-title.yml | Adds pull_request_target trigger to allow base-repo permissions for fork PR sticky comments. |
| .github/workflows/label.yml | Adds pull_request_target trigger to allow base-repo permissions for labeling fork PRs. |
…gger concurrency groups
Two real bugs uncovered while validating this PR:
1. project-orchestrator.yml was marked "Invalid workflow file" by
GitHub Actions because of `pull_request_review_thread: [resolved,
unresolved]`. Despite being listed in GitHub's events-that-trigger
docs, the Actions parser rejects it ("Unexpected value"), and an
invalid workflow suppresses ALL other triggers — which is why
orchestrator hadn't fired on a single `pull_request_target` event
since #65 merged (15+ runs in history, all push-event failures).
Removed the trigger; thread resolution is detected via the next
`synchronize` or `check_suite: completed` event that follows,
which is fine in practice.
2. pr-title.yml and label.yml's dual-trigger setup shared a concurrency
group keyed only on PR number (with `cancel-in-progress: true`),
which meant the first-to-start run got cancelled by the second
whenever both `pull_request` and `pull_request_target` fired.
On fork PRs this could cancel the ONLY run with write permissions
(the `pull_request_target` one), killing the sticky comment and
auto-label paths we added the trigger for. Copilot caught this on
#69 review. Fix: include `github.event_name` in the concurrency
key so same-PR runs of different event types don't collide.
After this merges, orchestrator will actually fire on
pull_request_target events across all PRs (including this one via
the post-merge run) and the pr-title / label workflows get
deterministic dual-trigger coverage without cross-event cancellation.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Four threads from Copilot's post-concurrency-fix review:
- pr-title.yml: added `issues: write` to the permissions block. The
sticky-comment endpoint (`/repos/.../issues/{n}/comments`) requires
that scope even for PR comments — `pull-requests: write` alone
doesn't cover it. Without the fix, fork PRs would still fail to
post the sticky under pull_request_target, defeating the dual-
trigger rationale.
- project-orchestrator.yml: removed the dead `pull_request` branch
from the PR-number resolver and action-selector. The workflow's
`on:` block only has pull_request_target / pull_request_review /
check_suite — the `pull_request` case is unreachable and misleads
future maintainers into thinking pull_request is supported.
- AGENTS.md: fixed stale reference in the reviewer-tooling table —
was still citing `pull_request_review_thread` as an orchestrator
trigger despite the workflow having dropped it (parser rejection).
Updated the row to list only the actual supported triggers with
a footnote about the parser limitation.
- project-orchestrator.yml header comment: acknowledged Copilot's
concern about the thread-resolution gap (no trigger fires when a
reviewer only resolves threads without pushing). Noted that in
practice the coder's next push / CI's next completion always
follows thread resolution closely, with schedule/workflow_dispatch
as escape hatches if it ever matters.
Claude's review verdict: "Ship it" with the same sticky-comment
TOCTOU race caveat acknowledged in-thread as a low-severity self-
healing cosmetic issue (documented in the PR description).
CI lint failure this round was golangci-lint couldn't fetch its
online JSON schema (context deadline — external service timeout);
logged as a data point on #70 (CI efficiency / flakiness tracker).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
taitelee
left a comment
There was a problem hiding this comment.
Before this PR, we'd receive notifications on every PR opening, sending reviewers and managers tons of notifications. The issue with this, besides the fact that there are hundreds of emails being sent, is that the PR might be fully ready yet. Github instead now waits for PRs to fully pass checks via the Project Orchestrator. It watches the PR. Once the bots (Lint, Build, Test) are all green and all conversation threads are resolved, then changes can be reviewed by developers.
Because we removed the standard CODEOWNERS protection, we needed a new way to make sure no one merges "bad" code without an admin's eyes on it.
- A new Required Check called "Admin Approval" has been created.
- Even if every other test passes, the "Merge" button stays locked until a reviewer or admin submits an official "Approve" review.
Technical Highlights:
- Fork Coverage: By adding pull_request_target, we ensure that bots have the necessary "Write" permissions to post sticky comments and apply labels on external contributions which are actions that were previously blocked by GitHub's pull_request security sandbox.
- Internal Stability: Keeping the standard pull_request trigger alongside the new one prevents a "chicken-and-egg" failure during the transition.
- Idempotency: The duplicate runs on internal PRs are harmless as the labeler and comment-marker logic are idempotent (one write replaces or confirms the other without duplication).
Bundles all the CI-hygiene work from #70 into one PR so we can re-add `Lint` / `Test` / `Integration Tests` to the ruleset's required status checks once this lands. Closes #70. ## Summary of changes | # | Scope | Change | Files | |---|---|---|---| | 1 | DLQ flake root-cause | Wait for ClickHouse `/ping` + explicit `chConn.Ping()` retry loop | `tests/integration_test.go` | | 2 | golangci-lint flake | `verify: false` to skip `golangci-lint.run` schema fetch | `.github/workflows/ci.yml` | | 3 | Workflow cleanup | Drop `pull_request` trigger; revert `event_name` concurrency suffix | `.github/workflows/pr-title.yml`, `.github/workflows/label.yml` | | 4 | Token efficiency | Claude review waits for required CI; skips on red | `.github/workflows/claude-review.yml` | | 5 | Test coverage | Add `sdk-test` (every PR) and `e2e` (non-draft) jobs | `.github/workflows/ci.yml` | ## Scope 1 — `TestDLQIntegration` flake Two observed failure modes ([generic](https://github.com/Wave-RF/WaveHouse/actions/runs/24803345618/job/72591993314), [`connection reset by peer`](https://github.com/Wave-RF/WaveHouse/actions/runs/24805317816/job/72598466908)) both pointed at the same root cause: ClickHouse opens 9000/tcp before it's ready to accept native-protocol queries, so `wait.ForListeningPort(\"9000/tcp\")` returned too early. The next `chConn.Exec` could meet a half-ready server. Fix is belt-and-suspenders: - `wait.ForAll(wait.ForListeningPort, wait.ForHTTP(\"/ping\"))` — the HTTP `/ping` endpoint only returns 200 once the server has finished initializing. - Explicit `chConn.Ping(ctx)` retry loop after `clickhouse.Open` — `Open` is lazy and doesn't dial until the first query, so without this the first real `Exec` would still be the test of readiness. ## Scope 2 — golangci-lint flake [Reference run](https://github.com/Wave-RF/WaveHouse/actions/runs/24817100832) on main: \`\`\` [.golangci.yml] validate: compile schema: failing loading \"https://golangci-lint.run/jsonschema/golangci.v2.11.jsonschema.json\": context deadline exceeded \`\`\` `verify: false` skips the schema-validate pre-flight fetch. The actual linter run is unaffected. ## Scope 3 — drop `pull_request` from dual-trigger workflows #69 added both `pull_request` AND `pull_request_target` as a transition pattern (the new trigger landed in the same PR, so the new event wouldn't fire on that PR itself). Now that `pull_request_target` is on `main` and observed firing on subsequent PRs, the `pull_request` half is dead weight: it doubles the CI minutes on internal PRs, races with the sticky-comment write, and only `pull_request_target` has the right permissions on fork PRs anyway. Concurrency-group `${{ github.event_name }}` suffix reverted with the trigger removal — only one event fires now, so cross-event cancellation is no longer a concern. ## Scope 4 — gate Claude review on CI Before: Claude review ran on every `pull_request: opened` / `synchronize` regardless of CI state. PRs with red `Lint` / `Test` would burn OAuth tokens for a review that the human will bounce back as \"come back when CI is green.\" After: a first-step polls `gh pr checks --watch` until the PR's required checks (`Check`, `Build`, `Validate`) reach a terminal state, then short-circuits the rest of the job if any failed/cancelled/timed-out. Subsequent re-pushes that go green re-trigger the review normally. Out of scope: Gemini (managed App, not configurable) and Copilot (per-seat). ## Scope 5 — SDK + E2E jobs Gap surfaced by #63: `make test-sdk` and `make test-e2e` exist in the Makefile but weren't wired into `ci.yml`. SDK-only PRs ran zero TypeScript tests automatically. - **`sdk-test`** — `npm ci && npm test` in `clients/ts`. Runs on every PR (no path filter — a deps bump or a workflow tweak should still exercise the suite). Fast (~30s) once node_modules is cached. - **`e2e`** — `make test-e2e` invokes vitest in `tests/sdk`, whose `setup.ts` globalSetup spins up the full ClickHouse + WaveHouse compose stack. Gated on `pull_request.draft == false` so WIP pushes don't pay the Docker-build cost. Depends on the `build` job to fail-fast if the binary doesn't compile. Both use SHA-pinned `actions/setup-node@v6.4.0`. ## After this merges Re-add to `main branch protection` ruleset 15353356: \`\`\` required_status_checks: + \"Lint\" + \"Test\" + \"Integration Tests\" \`\`\` (Was temporarily removed pre-#66 when main's CI was failing. Reinstating after this PR's `verify: false` + DLQ flake fix have at least 3 clean runs on main.) ## Test plan - [ ] CI run on this PR — verify all jobs green (incl. new `SDK Tests` and `E2E Tests`) - [ ] Open a follow-up draft PR after merge to verify `claude-review.yml` skips when CI is intentionally red, then runs after a fix push - [ ] Confirm `pr-title.yml` + `label.yml` still fire (only one run per PR now, not two) - [ ] Re-add Lint/Test/Integration Tests to required ruleset checks via `gh api PUT` --- *— Posted by Claude Code on behalf of @EricAndrechek* --- ### Added late Scope 6: **Fix `project-orchestrator.yml` `gh api --jq` bug.** Surfaced when this very PR went `ready_for_review` — the orchestrator assigned Taite (preceding step passed) but then crashed on the board add/edit with `accepts 1 arg(s), received 4`. `gh api --jq` doesn't forward `--arg` to the embedded jq. Fix: pipe JSON to a standalone `jq` at all six callsites. Existing bug on main that nothing exercised because the `reeval` path only runs when a bot-clean non-Dependabot PR goes ready-for-review — none had since #65 shipped. *— Updated by Claude Code on behalf of @EricAndrechek* --------- Co-authored-by: Taite Lee <113070390+taitelee@users.noreply.github.com>
Summary
Adds
pull_request_targetalongsidepull_requestonpr-title.ymlandlabel.yml. Extends fork-PR coverage (sticky comment on title failures, auto-labeling) without hitting the chicken-and-egg that blocked a straight trigger swap in #64.How it works:
pull_requestruns from the PR head branch — works on internal PRs with full permissions.pull_request_targetruns from the default branch with base-repo permissions — catches fork PRs whereGITHUB_TOKENis otherwise read-only.actions/labelerwithsync-labels: trueis idempotent (same label set applied twice is effectively one write).A future follow-up PR can drop the
pull_requesttrigger once we've confirmedpull_request_targetis firing correctly in production.First live test of project-orchestrator
Worth flagging: this is the first post-merge PR that should exercise
project-orchestrator.ymlend-to-end. Expected behavior:Admin approvalcheck fails until Taite approvesCloses #56If anything in that sequence doesn't fire, it's useful signal for orchestrator fixes.
Test plan
Validatecheck passes (bothpull_requestandpull_request_targetruns)Apply labelsruns and appliesarea/infra,github_actionsproject-orchestratorfires onpull_request_target, assigns Taite, adds PR to board, sets Status=Readypr-title-lint-commentmarker managed)Admin approvalcheck fails until Taite approves, then succeedsRelated Issues
Closes #56