feat(ci): workflow-driven admin approval + Task Board orchestration#65
Merged
Conversation
Eliminates GitHub's native CODEOWNERS auto-request path in favor of two workflows that together give stronger control over when human reviewers are notified and what it takes to merge: - admin-approval.yml: required status check. Fails the "Admin approval" check unless at least one approving review is from an admin (Eric or Taite). Dependabot PRs bypass this check because dependabot-automerge.yml handles their flow separately. - auto-assign-reviewer.yml: assigns the non-author admin (Eric's PR → Taite; Taite's PR → Eric; others → both) *only once* the PR is bot-clean (required checks green AND all review threads resolved). Draft PRs get flipped to ready at the same moment. Idempotent — a reviewer who has already been requested or already reviewed won't be re-requested on subsequent re-fires. Deleted .github/CODEOWNERS (one-ping-on-open conflicted with the "don't notify until bots are clean" goal). Ruleset updated to set require_code_owner_review=false (no-op now) and add "Admin approval" to required_status_checks, so admin approval is still merge-blocking but enforced by workflow rather than by CODEOWNERS. Net behavior change: reviewers get exactly one notification at the moment their attention helps, regardless of draft vs non-draft, and regardless of what files the PR touches. No churn across pushes; no premature ping at open time. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the repository's review and approval process by moving away from GitHub's native CODEOWNERS feature. The goal is to improve the developer experience by ensuring that human reviewers are only notified when a pull request is truly ready for review, rather than immediately upon opening. This is achieved through custom workflows that handle reviewer assignment and enforce mandatory admin approvals as required status checks. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
github-actions
Bot
added
github_actions
|
Claude finished @EricAndrechek's task in 3m 11s —— View job Code ReviewThis is a thoughtful replacement of the CODEOWNERS mechanism. Multiple rounds of Copilot/Gemini review have addressed the main correctness and security issues. I'll focus on what's still open. Security —
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request replaces the CODEOWNERS file with a workflow-driven system for reviewer assignment and admin approval, updating AGENTS.md and the CHANGELOG.md. Feedback identifies that the core implementation workflow files are missing from the PR, and the proposed logic contains a potential deadlock where the required Admin approval check prevents the assignment of the reviewer needed to provide that approval. Additionally, a mandatory documentation update for docs/development.md was missed.
There was a problem hiding this comment.
Pull request overview
Replaces GitHub CODEOWNERS-based reviewer auto-request + merge gating with two GitHub Actions workflows that (1) enforce an “admin must approve” required status check and (2) delay reviewer notifications until the PR is bot-clean and threads are resolved.
Changes:
- Add required Admin approval workflow check that passes only with an admin approval (Dependabot bypass).
- Add Auto-assign reviewer workflow to request the appropriate admin reviewer(s) only after required checks pass + threads are resolved (and optionally flip draft → ready).
- Remove
.github/CODEOWNERSand update docs/changelog to reflect the new governance flow.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
CHANGELOG.md |
Documents the governance change from CODEOWNERS to workflow-driven reviewer assignment + admin approval gate. |
AGENTS.md |
Updates repo governance/review tooling documentation to match the new workflow-driven approach. |
.github/workflows/auto-assign-reviewer.yml |
New workflow to delay reviewer assignment until the PR is “bot-clean” + threads resolved. |
.github/workflows/admin-approval.yml |
New workflow implementing a required “admin approval” status check. |
.github/CODEOWNERS |
Deleted to stop CODEOWNERS auto-request behavior. |
`pull_request_review_thread` is a webhook event but not a GitHub Actions workflow trigger — including it in `on:` causes the whole workflow to fail validation at GitHub and never fire on any event. Observed on PR #65: auto-assign-reviewer never ran despite multiple pull_request and pull_request_review events that should have matched. Removed the invalid trigger. Thread-resolution detection now relies on the next push / review / check_suite event firing (which happens frequently during normal iteration). One-event lag vs immediate resolution detection, but the workflow is re-entrant and idempotent so no state is lost. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`gh pr view` / `gh pr edit` / `gh pr ready` default to inferring the repo from the local git checkout. Our workflow doesn't do `actions/checkout` (no reason to — we only read API data), so those commands fail with "fatal: not a git repository". Explicitly passing `-R "$REPO"` on each gh pr command uses the repo from the env var and bypasses the git lookup. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The workflow reads statusCheckRollup via gh pr view --json, which uses GraphQL under the hood. With the minimal permissions block we had, the token was missing access and GraphQL returned "Resource not accessible by integration." Adding checks: read + statuses: read covers both the new check-runs API and the legacy commit-status API that statusCheckRollup aggregates. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
EricAndrechek
force-pushed
the
feat/workflow-based-admin-approval
branch
from
e4a4af4 to
5374530
Compare
Substantive bugs caught by Copilot: - admin-approval.yml treated any historical APPROVED review as sufficient, meaning a later CHANGES_REQUESTED or dismissal wouldn't invalidate the gate. Now evaluates the latest review state per user (sort by submitted_at, take most-recent per user, require APPROVED). - auto-assign-reviewer.yml exited the job on the first candidate that was already requested or had already reviewed, which broke the "request both admins" path for non-maintainer authors. Now uses a continue-and-collect pattern so each candidate is evaluated independently. - auto-assign-reviewer.yml concurrency group referenced check_suite.pull_requests[0].number which can be empty on branch- push check suites, collapsing the key. Added `github.run_id` fallback. - Dropped `contents: write` from auto-assign-reviewer.yml. `gh pr ready` + `gh pr edit --add-reviewer` both use the updatePullRequest mutation which only needs `pull-requests: write`. Doc sync (Gemini MUST + Copilot): - AGENTS.md Dependabot-automation section was claiming codeowner-approval-per-ruleset behavior that no longer exists. Now states Dependabot PRs bypass the `Admin approval` required check. - docs/development.md "CI & review automation" section updated to describe the new `Admin approval` gate and the auto-assign-reviewer flow instead of CODEOWNERS. Not accepted: - Gemini [CRITICAL] flagged the workflow files as missing from the PR. Actually present — Gemini's built-in .github/workflows/** exclusion means it literally can't see them. Documented limitation, nothing to fix in this PR. - Gemini [HIGH] flagged a potential deadlock (Admin approval as required check + workflow waits for required checks = loop). Already handled — auto-assign-reviewer.yml's required list explicitly excludes Admin approval. - Copilot flagged the first:100 cap on reviewThreads pagination. Valid concern but pragmatically our PRs will never hit 100 threads. Tracked as a nice-to-have; not a 2026-04 blocker. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
the
documentation
Dropping contents: write was right (Copilot) but dropping contents entirely was too aggressive — statusCheckRollup needs commit-node read access to populate. Adding contents: read (not write) keeps least-privilege while letting the GraphQL query succeed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per Eric's direction, pivots #65 from "assign reviewer via GitHub review-request" to "assign reviewer via Task Board card state + assignee field" — the board becomes the single signal for who needs to look at what, and GitHub's native review-request notification layer is intentionally not used. Deletes .github/workflows/auto-assign-reviewer.yml (subsumed). Adds .github/workflows/project-orchestrator.yml, a state machine that drives project #7: - PR bot-clean (required checks green + threads resolved): add PR to board, Status=Ready, assign non-author admin to the PR's assignees field (Eric's PR → Taite; Taite's PR → Eric; others → both). Draft PRs flipped to ready at the same moment. - Review submitted with CHANGES_REQUESTED: PR card → In review (reviewer waiting on coder), linked issue card → Ready (coder attention needed). - Author re-requests review (explicit "I addressed feedback" signal via GitHub's re-request button): PR card → Ready, linked issue card → In review. - APPROVED review / merge: no-op; admin-approval + auto-merge + existing project automation close the loop. Manual step intentionally preserved: reviewer moves PR card Ready→In progress when they start reviewing (GitHub doesn't emit a "review started" event we could hook). Dependabot PRs skip the orchestrator entirely. They go through dependabot-automerge.yml for patch/minor bumps and don't appear on the board — hands-off merge, no human touch. That's the reason admin-approval.yml stays (its Dependabot bypass is what preserves the hands-off path). Docs updated: AGENTS.md §"Task Board state machine" describes the full flow; docs/development.md "CI & review automation" rewritten to frame the coder/reviewer ping-pong in board terms instead of GitHub-review-request terms. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PROJECT_BOARD_TOKEN only has project scope (per how Eric configured it). gh pr / gh api / GraphQL calls need standard repo scopes, so they use GITHUB_TOKEN; gh project calls use PROJECT_TOKEN override. Single workflow, two tokens, minimum privilege for each. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
EricAndrechek
changed the title
There was a problem hiding this comment.
Pull request overview
This PR migrates reviewer assignment and merge-governance from GitHub’s CODEOWNERS mechanism to workflow-driven checks and a Task Board–based orchestration flow.
Changes:
- Introduces a required
Admin approvalworkflow status check to enforce at least one admin approval (with Dependabot bypass). - Adds a new
project-orchestratorworkflow to manage Task Board state and assign the non-author admin once a PR is “bot-clean”. - Updates repo documentation/CHANGELOG to reflect the new governance model and removes
.github/CODEOWNERS.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/development.md | Updates development docs to describe admin-approval gating and Task Board–driven review flow. |
| CHANGELOG.md | Records the shift from CODEOWNERS to workflow-driven approval/assignment and board orchestration. |
| AGENTS.md | Updates automation/governance documentation to match the new workflow-based process. |
| .github/workflows/project-orchestrator.yml | New workflow implementing Task Board state transitions and admin assignment once PRs are bot-clean. |
| .github/workflows/admin-approval.yml | New required status check enforcing admin approval (with Dependabot bypass). |
| .github/CODEOWNERS | Removed to stop auto-request-on-open behavior and rely on workflows instead. |
Seven findings, all legitimate: Security: - project-orchestrator.yml switched from pull_request to pull_request_target. With pull_request, a malicious same-repo branch could modify the workflow to exfiltrate PROJECT_BOARD_TOKEN (workflow definition comes from PR head). pull_request_target uses the default-branch workflow definition, safe since we never check out PR code. Correctness: - Added pull_request_review_thread trigger. Previously claimed in docs but not wired — thread resolution didn't re-trigger the orchestrator. (Earlier I believed pull_request_review_thread wasn't a valid workflow trigger, based on a YAML-schema linter error. GitHub actually supports it; the linter was wrong.) - review_requested gate tightened to check sender == PR author. Previously fired on any reviewer assignment including initial ones, which could incorrectly flip cards back to Ready. - Pagination cap bumped from 200 to 10000 on gh project item-list calls. 200 would miss items once the board grows. Doc sync: - admin-approval.yml header referenced the deleted auto-assign- reviewer.yml → now points at project-orchestrator.yml. - docs/development.md Dependabot section said workflow-touching bumps need codeowner approval. With CODEOWNERS removed, that's stale — Dependabot PRs now bypass Admin approval entirely, so patch/minor workflow bumps auto-merge too (trust model: CI passing + fetch-metadata classification). - AGENTS.md review-tooling row updated to list the actual triggers (pull_request_target / pull_request_review / pull_request_review_thread / check_suite). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR replaces CODEOWNERS-driven reviewer gating with workflow-enforced admin approval and a project-board-driven review state machine, so humans are only notified once a PR is actually “ready”.
Changes:
- Add a required
Admin approvalworkflow check that requires an approval from an admin (with Dependabot bypass). - Add
project-orchestratorworkflow to drive Project #7 task board state and assign non-author admins when a PR becomes bot-clean. - Remove
.github/CODEOWNERSand update docs/changelog to reflect the new governance model.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/development.md | Documents new admin-approval gate and Task Board-as-signal workflow behavior. |
| CHANGELOG.md | Records the governance/automation shift from CODEOWNERS to workflows. |
| AGENTS.md | Updates automation/governance documentation to match the new workflows. |
| .github/workflows/project-orchestrator.yml | Implements PR/issue Project #7 orchestration and admin assignment logic. |
| .github/workflows/dependabot-automerge.yml | Updates comments to reflect CODEOWNERS removal and admin-approval bypass. |
| .github/workflows/admin-approval.yml | Adds required status check enforcing admin approval (with Dependabot bypass). |
| .github/CODEOWNERS | Removed in favor of workflow-based enforcement. |
taitelee
previously approved these changes
Apr 22, 2026
taitelee
left a comment
taitelee
left a comment
Contributor
There was a problem hiding this comment.
Key improvement on developer experience. Removing the "ping-on-open" noise from CODEOWNERS while maintaining the merge-gate via the new admin-approval.yml status check is a great balance.
- Workflow Logic: The state machine in project-orchestrator.yml (specifically the transitions between Ready and In review based on thread resolution/author re-requests) looks good.
- Dependabot: The bypass in admin-approval.yml for Dependabot keeps the security bumps moving without manual intervention.
…st gate Copilot's post-push review on #65 flagged two real bugs in the previous refactor: 1. `gh api graphql` `resource(url).projectItems` lookups were using GITHUB_TOKEN, which doesn't carry org-project read access. Lookups could return empty nodes even when the item exists on the board. Switched to PROJECT_TOKEN for all five lookup sites (three in reeval, two across the transition paths). 2. The `review_requested` gate checked `sender == author` but that matches both: - Initial request when author opens PR with reviewers sidebar pre-populated (not a re-request) - Explicit re-request click Now additionally checks whether the newly-requested reviewer has a prior review on this PR. Only treats as `re_requested` if yes — which is the distinguishing signal between "first-time request" (no prior review) and "re-request after feedback" (prior CHANGES_REQUESTED or APPROVED review exists). Deferred (Copilot LOW): reviewThreads(first: 100) pagination cap. Pragmatic: our PRs won't hit 100 threads in any realistic scenario, and the failure mode is "prematurely mark bot-clean" — not data loss. Tracked for a future iteration. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Shifts merge gating and reviewer coordination from CODEOWNERS to GitHub Actions workflows, making “admin approval” an explicit required status check and using Project #7 (Task Board) + PR assignees as the primary review signal once a PR is bot-clean.
Changes:
- Add required
Admin approvalworkflow to enforce at least one admin (Eric/Taite) approval, with Dependabot bypass. - Add
project-orchestratorworkflow to add/manage PR (and linked issues) on Project #7 and assign non-author admin(s) when bot-clean. - Remove
.github/CODEOWNERSand update docs/AGENTS/CHANGELOG to reflect the new governance model.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/development.md | Updates contributor docs for new admin-approval + Task Board orchestration model. |
| CHANGELOG.md | Documents migration from CODEOWNERS gating to workflow-driven admin approval/orchestration. |
| AGENTS.md | Updates repo governance/automation guidance to match the new workflows. |
| .github/workflows/project-orchestrator.yml | New workflow implementing the Task Board + assignee state machine for PR review flow. |
| .github/workflows/dependabot-automerge.yml | Updates Dependabot auto-merge documentation/comments to reflect Admin approval bypass and CODEOWNERS removal. |
| .github/workflows/admin-approval.yml | New required status check enforcing admin approval for merges (with Dependabot bypass). |
| .github/CODEOWNERS | Deleted in favor of workflow-based review/approval enforcement. |
Two hygiene fixes from the latest bot review round: - admin-approval.yml: dropped `edited` from pull_request_review trigger types. `edited` fires on review-body changes (which can't alter the APPROVED/CHANGES_REQUESTED state), so re-running the approval check on edits was pure noise. Claude caught this. - project-orchestrator.yml: dropped `issues: write` from the workflow permissions block — the workflow only reads PRs and writes project items (which go through PROJECT_TOKEN on the GraphQL mutation path). Copilot caught that the scope was requested but not used. Deferred (tracked as nice-to-haves, not blockers): - per_page=100 on /reviews API (admin-approval) — pragmatic limit for realistic PR scale. - Hardcoded required-check list (Check/Build/Validate) in the orchestrator readiness gate — could be read from the ruleset dynamically, but adds complexity. - Multiple ### Changed sections within Unreleased — pattern from successive Unreleased accumulation; best cleaned up at the release-cut PR rather than mid-iteration. - `re_requested` false-positive on comment-only prior reviews (Claude) — blast radius is limited (worst case: PR card marked Ready slightly early), will address if observed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR replaces CODEOWNERS-driven review gating/notifications with workflow-driven admin approval enforcement plus Project #7 (“Task Board”) orchestration, aiming to notify humans only once a PR is actually bot-clean and ready for review.
Changes:
- Add required
Admin approvalworkflow to enforce at least one approving review from an admin (with Dependabot bypass). - Add
project-orchestrator.ymlto manage Project #7 card state + PR assignees as the primary “who’s up next” signal. - Remove
.github/CODEOWNERSand update docs/changelog to reflect the new review + automation model.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/development.md | Updates CI/review automation docs to describe admin-approval gating and Task Board as the primary signal. |
| CHANGELOG.md | Records the shift from CODEOWNERS to workflows for review assignment/approval gating and board orchestration. |
| AGENTS.md | Updates governance/review automation documentation to reflect workflow-driven admin assignment + approval enforcement. |
| .github/workflows/project-orchestrator.yml | New workflow implementing Task Board + assignee-driven state machine based on checks, threads, and review events. |
| .github/workflows/dependabot-automerge.yml | Updates commentary to reflect Dependabot bypass of the new admin-approval gate and removal of CODEOWNERS usage. |
| .github/workflows/admin-approval.yml | New required-status workflow implementing the admin approval merge gate (with Dependabot bypass). |
| .github/CODEOWNERS | Deleted to eliminate CODEOWNERS-based reviewer auto-request and gating. |
Comments suppressed due to low confidence (1)
CHANGELOG.md:21
## Unreleasednow contains multiple separate### Changedheadings. This makes the changelog harder to scan and deviates from the usual Keep a Changelog structure (one heading per type per release). Consider moving this new bullet into the existing### Changedsection further down and keeping a single### Changedblock under Unreleased.
### Changed
- **Reviewer assignment, admin approval, and Task Board orchestration moved from CODEOWNERS to workflows.** Removed `.github/CODEOWNERS`; admin approval on every PR is now enforced by `.github/workflows/admin-approval.yml` as a required status check (fails unless Eric or Taite has an `APPROVED` review, Dependabot PRs bypass via `dependabot-automerge.yml`). Reviewer assignment + Task Board state orchestration handled by `.github/workflows/project-orchestrator.yml`: when a PR becomes bot-clean (required checks green + all review threads resolved), the workflow adds it to the Task Board (project #7), sets Status=`Ready`, and assigns the non-author admin to the PR's `assignees` field (not as a GitHub review-request — the board card + assignment is the single signal). Subsequent events drive a full state machine: `CHANGES_REQUESTED` review moves PR→`In review` and linked issue→`Ready`; author re-requesting review moves PR→`Ready` and issue→`In review`; merge handled by existing project automation. One manual step intentionally preserved: reviewer moves PR card `Ready`→`In progress` when they start reviewing. Ruleset updated: `require_code_owner_review: false` (no CODEOWNERS), `Admin approval` added to `required_status_checks`.
### Fixed
- **`agent` label trigger was unreachable AND under-gated**: `.github/workflows/claude-agent.yml`'s labeled-trigger branch gated on `github.event.sender.author_association`, which doesn't exist on the `sender` User object in `issues.labeled` webhook payloads — it's only set on content objects like `comment` / `review` / `issue`. The check silently evaluated false, making the `agent` label path never fire (flagged in Claude's post-merge review of #55). Fix: removed the unreachable association check AND added a first-step permission verification that calls the collaborators/permission API and requires `admin` / `maintain` / `write` before proceeding — closing the privilege-escalation gap Gemini flagged where a `triage`-role user could otherwise trigger the agent.
- **`CONTRIBUTING.md`** — formatting guidance said `gofmt`; corrected to `gofumpt` to match what `make fmt` runs and `make fmt-check` enforces in CI. `gofmt`-formatted code would fail CI. Flagged in Claude's post-merge review of #55.
### Changed
- **`goreleaser-action` CLI version pin**: `release.yml` now passes `version: "~> v2"` to `goreleaser-action` instead of `version: latest`. Picks up patch / minor GoReleaser bumps automatically but breaks loudly on a v3 major bump rather than silently changing release binaries. Flagged in Claude's post-merge review of #55.
Three more from Copilot: 1. admin-approval.yml: replaced `sort_by + reverse + unique_by` with explicit `group_by(user) | map(max_by(submitted_at))` for selecting each user's latest review. `unique_by` sorts internally and could keep an arbitrary entry within a user-group depending on input order — group_by + max_by is unambiguous. 2. project-orchestrator.yml changes_requested path: linked issues that aren't already on the project board were silently skipped, breaking the state-machine transition. Now adds the issue to the board if missing, then applies the transition. 3. Same fix on the re_requested path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Claude's final review nits: 1. re_requested path silently no-op'd if the PR card was missing (manual removal or some other edge). The changes_requested path had the self-heal (item-add if missing); the re_requested path didn't. Symmetry restored. 2. `action="approved"` was set but not consumed by any later step — not a bug (APPROVED triggers admin-approval + auto-merge + the project's merge automation, no orchestrator work needed) but the dead string was a clarity gap. Changed to `action="none"` with an explicit comment about the deliberate no-op path. Claude's review verdict: "Ship it." Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
github-project-automation
Bot
moved this from In review
to Done
in WaveHouse Task Board
EricAndrechek
added a commit
that referenced
this pull request
Apr 22, 2026
…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>
EricAndrechek
added a commit
that referenced
this pull request
Apr 23, 2026
'gh api --jq' takes a single jq expression — it can't pass --arg to the embedded jq. Invoking it as '--jq --arg n "$N" "..."' makes gh see --arg, n, $N, and the query as four separate positional tokens, which explodes with: accepts 1 arg(s), received 4 Surfaced on PR #71's ready-state transition: Taite was assigned (the preceding step) but the board add/edit never ran. Switched all six callsites to pipe the raw JSON to a standalone jq invocation so --arg works normally. Existing bug on main, not introduced by #71 — caught because the reeval path only runs when a bot-clean PR goes ready-for-review, and none had since the orchestrator shipped in #65.
4 tasks
taitelee
added a commit
that referenced
this pull request
Apr 23, 2026
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>
This was referenced Apr 23, 2026
EricAndrechek
added a commit
that referenced
this pull request
Apr 23, 2026
…acklog Copilot R6 flagged that my round-4 `promote`-based gate on the reeval Assign step had an inverse gap: if a PR card was already on the board with Status = In progress / In review / Done (e.g., someone manually moved it before the first bot-clean re-eval), then promote=false, and the Assign step would silently skip reviewer assignment AND review request — even though those should happen on first bot-clean regardless of the board state. Fix: remove the board-status gate from the orchestrator entirely and push idempotency into the composite action instead. The composite now: - queries current PR assignees + requested-reviewers - computes the subset of desired reviewers that are missing from BOTH lists - exits early with a log message if everyone's already present - otherwise adds/requests only the missing users That handles both directions cleanly: - PR already In progress with reviewer set → composite no-ops (no re-spam, same behavior as before) - PR already In progress without reviewer (the gap) → composite sets the missing reviewer No-op behavior on SKIP_BOARD also works — assignment doesn't depend on the board token at all. Didn't consolidate the CHANGELOG Unreleased section (Copilot R6 #1) — the duplicate ### Fixed / ### Changed headers are pre-existing from #55/#65/#71 accumulating, and full consolidation is release-cut scope. Replied to that thread deferring.
EricAndrechek
added a commit
that referenced
this pull request
Apr 28, 2026
…acklog Copilot R6 flagged that my round-4 `promote`-based gate on the reeval Assign step had an inverse gap: if a PR card was already on the board with Status = In progress / In review / Done (e.g., someone manually moved it before the first bot-clean re-eval), then promote=false, and the Assign step would silently skip reviewer assignment AND review request — even though those should happen on first bot-clean regardless of the board state. Fix: remove the board-status gate from the orchestrator entirely and push idempotency into the composite action instead. The composite now: - queries current PR assignees + requested-reviewers - computes the subset of desired reviewers that are missing from BOTH lists - exits early with a log message if everyone's already present - otherwise adds/requests only the missing users That handles both directions cleanly: - PR already In progress with reviewer set → composite no-ops (no re-spam, same behavior as before) - PR already In progress without reviewer (the gap) → composite sets the missing reviewer No-op behavior on SKIP_BOARD also works — assignment doesn't depend on the board token at all. Didn't consolidate the CHANGELOG Unreleased section (Copilot R6 #1) — the duplicate ### Fixed / ### Changed headers are pre-existing from #55/#65/#71 accumulating, and full consolidation is release-cut scope. Replied to that thread deferring.
31 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces GitHub's CODEOWNERS-based reviewer auto-request + merge-gate with two workflows that together give stronger control over when human reviewers are notified, and orchestrate the Task Board as the single signal for who's holding the baton.
Why
You wanted human reviewers pinged only once the PR is actually ready for review (bots clean, required checks green, threads resolved) — regardless of draft state. CODEOWNERS' one-ping-on-PR-open behavior conflicted with that goal and couldn't express per-author exceptions (Dependabot). The workflow approach also lets us drive the Task Board card state as the real coordination channel — GitHub review-request notifications become noise we can ignore.
What changes
admin-approval.yml(new required status check): fails theAdmin approvalcheck unless at least one approving review is from an admin (Eric or Taite). Dependabot PRs bypass (matches bothdependabot[bot]andapp/dependabotlogins). Evaluates the latest review state per user via jq so a subsequent CHANGES_REQUESTED / dismissal invalidates a stale APPROVED.project-orchestrator.yml(new): drives project docs: add initial Astro-based documentation site #7 as the single signal for review state.assigneesfield (not a review request — one signal channel only).CHANGES_REQUESTED→ PR card "In review", linked issue card "Ready".resource(url).projectItems(no full-board scans).PROJECT_BOARD_TOKENis missing (fresh forks, contributor env)..github/CODEOWNERSdeleted — one-ping-on-open is gone; admin approval is now enforced by the required status check instead.require_code_owner_review: false(no CODEOWNERS),Admin approvaladded torequired_status_checks.Behavior change
Test plan
Admin approvalfails on this PR until Taite (or Eric) approves it.project-orchestratorruns withoutPROJECT_BOARD_TOKENerrors; board ops skip cleanly if token missing (not applicable on this repo since it's set, but the guard is tested by an empty-secret scenario).Related Issues
Closes #43 (originally); this PR replaces the initial CODEOWNERS-based approach from #55 with a workflow-driven one after deciding Dependabot hands-off auto-merge was non-negotiable (requires the admin-approval bypass that native CODEOWNERS can't express).
Notes
PR body originally described
auto-assign-reviewer.ymland GitHub-review-request-based signaling. That design pivoted toproject-orchestrator.ymlwith assignee-based signaling mid-PR — Copilot flagged the body as stale and this revision reflects the current implementation.