Skip to content

#513 Gate The Approval Check On The Claude Label #518

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
b-at-neu merged 2 commits into from
Aug 19, 2026
Merged

#513 Gate The Approval Check On The Claude Label #518

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e9bdf2b
#513 gate the approval check on the claude label
b-at-neu Aug 19, 2026
0cf9275
#513 trim the approval-check header comment
b-at-neu Aug 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .claude/agents/impl-agent.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,11 +97,14 @@ gh issue edit N --repo SGAOperations/aplio --remove-label "plan approved" --add-
--title "#N <Ticket Title In Title Case>" \
--body-file .temp/pr-N.md \
--assignee "<issue-assignee-login>" \
--label "claude" \
--head N-ticket-name-in-kebab-case
```

`--assignee` is the **issue's assignee login recorded in pre-flight** (`@me` if the issue had none) — the PR must carry the same owner as its issue or it never shows up in that operator's `ready for review` query. **Substitute the literal login string you read in pre-flight; never `$(...)` command substitution** — it isn't allow-listed and would silently produce an empty argument.

`--label "claude"` is what **activates the approval gate**: `.github/workflows/approval-check.yml` runs only on PRs carrying `claude`, so a PR opened without it merges with **no** approval gate at all.

**Always target `dev`** (the integration branch) — never open feature PRs against `main`. Releases flow `dev → main` via the `/release` command.

Note the PR number returned.
Expand Down
24 changes: 14 additions & 10 deletions .claude/docs/PIPELINE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,16 +112,17 @@ Rule: **every stage agent's first action is swapping its trigger label for its i

### PR labels

| Label | Set by | Type | Meaning |
| ------------------ | ----------------------------- | --------- | ------------------------------------------------------------------------------------------------ |
| `ready for review` | `impl-agent` / `revise-agent` | trigger | Dispatch `review-agent` |
| `reviewing` | `review-agent` | in-flight | Review underway |
| `needs revision` | `review-agent` | trigger | Dispatch `revise-agent` (subject to cycle cap) |
| `revising` | `revise-agent` | in-flight | Fixes underway |
| `approved` | `review-agent` | terminal | Findings are at/under the current cycle's bar (escalating, below); human merges on GitHub |
| `needs human` | Cockpit / `revise-agent` | gate | 5 cycles without convergence, or an ambiguous rebase conflict needing the author; pipeline stops |
| `refresh branch` | Cockpit / human | trigger | Dispatch `revise-agent` in refresh mode — rebase onto base and force-push, no code changes |
| `refreshing` | `revise-agent` | in-flight | Branch refresh underway; the PR's other labels (e.g. `approved`) are left in place |
| Label | Set by | Type | Meaning |
| ------------------ | -------------------------------- | --------- | ---------------------------------------------------------------------------------------------------------------------------- |
| `claude` | `impl-agent` (at `gh pr create`) | marker | Pipeline owns this PR — activates the `approved` merge gate in `approval-check.yml`; a pipeline PR missing it merges ungated |
| `ready for review` | `impl-agent` / `revise-agent` | trigger | Dispatch `review-agent` |
| `reviewing` | `review-agent` | in-flight | Review underway |
| `needs revision` | `review-agent` | trigger | Dispatch `revise-agent` (subject to cycle cap) |
| `revising` | `revise-agent` | in-flight | Fixes underway |
| `approved` | `review-agent` | terminal | Findings are at/under the current cycle's bar (escalating, below); human merges on GitHub |
| `needs human` | Cockpit / `revise-agent` | gate | 5 cycles without convergence, or an ambiguous rebase conflict needing the author; pipeline stops |
| `refresh branch` | Cockpit / human | trigger | Dispatch `revise-agent` in refresh mode — rebase onto base and force-push, no code changes |
| `refreshing` | `revise-agent` | in-flight | Branch refresh underway; the PR's other labels (e.g. `approved`) are left in place |

## Stages and models

Expand Down Expand Up @@ -158,6 +159,8 @@ Permission mode: every stage agent runs **`permissionMode: dontAsk`** (auto-deny

**Mitigation chosen instead of the hook: keep content emitters off the allowlist.** Since redirection can't be blocked, the allowlist itself is the control — so `echo`, `cat`, `head`, `tail`, `cut`, `diff` and `true` are excluded, because their whole purpose is emitting bytes to stdout and a redirect turns each into a clean file-write (`echo "…" > lib/types.ts`, `cat src > dst`). The 11 that remain emit search results, paths, or metadata. **This narrows the bypass; it does not eliminate it** — `grep -v x f > f` still strips lines, any allowed command can truncate a redirect target, and the long-standing `Bash(git *)` allow has always offered write primitives (`git apply`, `git checkout -- …`). So the "write files with the Write/Edit tools" rule above remains a **convention agents are expected to follow, not a technical guarantee** — `plan-agent`'s and `review-agent`'s read-only status rests on them following it. #326 is the real fix whenever it's worth the hook.

**CI merge gate — `approval-check.yml`.** PRs into `dev` are gated on the `approved` label **only when the PR carries `claude`** (a job-level `if:`). Every other PR — human, Dependabot — gets a `skipped` check run, which GitHub counts as satisfied, so it merges on its own merits. This is deliberately **fail-open**: an unlabelled pipeline PR is indistinguishable in CI from a human one and simply loses its gate. Nothing in the workflow can close that, so the mitigations live upstream — `impl-agent` passes `--label "claude"` at `gh pr create` (so the gate is live on the PR's first event) and the cockpit's **ungated-PR sweep** reports any tracked PR missing it. Never narrow the workflow's trigger to exclude a PR: a workflow that never runs creates no check run, leaving the required check pending forever.

## Pipeline output formats

Defined once here; the stage agents follow these exactly.
Expand Down Expand Up @@ -271,6 +274,7 @@ Closing the cockpit session also halts dispatch (it is the only dispatcher) but
| Nothing dispatches for an item | It has no trigger label (paused, in-flight, or gated) | `status` shows where it is; `resume #N` re-applies the right trigger |
| Nothing dispatches **and** `status` doesn't list it at all | It is unassigned, or owned by another operator (queries are assignee-filtered) | The tick's unowned sweep reports it — claim it with `work on #N`. If another operator owns it, that's their cockpit's job; `work on #N` offers an explicit take-over |
| No GitHub Actions check runs at all on a new push, while `Vercel` still runs | The PR conflicts with its base, so GitHub cannot build the merge ref that `pull_request` workflows run against. Vercel deploys from the head commit, so it is unaffected | `gh pr view <n> --json mergeable` reports `CONFLICTING`. Rebase onto the base branch and force-push; the checks return on the next push |
| `run-approval-check` shows **Skipped** on a pipeline PR | The PR is missing the `claude` label, so the approval gate is inactive | `gh pr edit <n> --repo SGAOperations/aplio --add-label "claude"` — the `labeled` event re-evaluates the job condition and the gate activates on that run |
| A stage misbehaved and you want to run it by hand | — | @-mention the subagent (`@agent-impl-agent implement #N`) or run `claude --agent impl-agent` |
| Cockpit session closed | All state is in labels | Start `/pipeline` again; it resumes from the labels. `retry #N` anything parked in an in-flight label |
| Labels manually changed on GitHub | Fine — labels are the source of truth | The next tick acts on whatever the labels say |
Expand Down
12 changes: 10 additions & 2 deletions .claude/skills/pipeline/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,12 @@ gh pr list --repo SGAOperations/aplio --assignee "@me" --label "needs human" --j
# Unowned sweep → report only, never act (see Ownership above)
gh issue list --repo SGAOperations/aplio --search "no:assignee" --limit 100 --json number,title,labels --jq '[.[] | select(.labels | map(.name) | any(. == "ready" or . == "plan changes requested" or . == "plan approved" or . == "plan review" or . == "blocked"))]'
gh pr list --repo SGAOperations/aplio --search "no:assignee" --limit 100 --json number,title,labels --jq '[.[] | select(.labels | map(.name) | any(. == "ready for review" or . == "needs revision" or . == "approved" or . == "needs human"))]'

# Ungated-PR sweep → report only, never act (see Ungated report below)
gh pr list --repo SGAOperations/aplio --assignee "@me" --json number,title,labels --jq '[.[] | select((.labels | map(.name)) as $l | ($l | any(. == "ready for review" or . == "reviewing" or . == "needs revision" or . == "revising" or . == "approved" or . == "refresh branch" or . == "refreshing" or . == "needs human")) and ($l | index("claude") | not)) | {number, title}]'
```

Then, in order: **(1)** reconcile merged PRs (below), **(2)** handle human gates, **(3)** **unless draining,** dispatch for every actionable trigger item (all Agent calls in one message), **(4)** report the unowned sweep if its set changed, **(5)** schedule the next wakeup (**skip while draining**).
Then, in order: **(1)** reconcile merged PRs (below), **(2)** handle human gates, **(3)** **unless draining,** dispatch for every actionable trigger item (all Agent calls in one message), **(4)** report the unowned and ungated-PR sweeps if their sets changed, **(5)** schedule the next wakeup (**skip while draining**).

**Merged-PR reconciliation (each tick):** the `approved` query above is open-only, so a merged PR silently drops out of it — never trust in-session memory for "awaiting merge." Diff the set of PRs you have **announced as approved** against the live `approved` result; for each announced PR no longer present, confirm and announce it **once**:

Expand All @@ -77,6 +80,10 @@ gh pr view <n> --repo SGAOperations/aplio --json headRefOid,statusCheckRollup --

An **empty** sweep result is only meaningful if the `--jq` filter works — verify it once against a known unassigned, trigger-labeled issue rather than trusting silence.

**Ungated-PR sweep (each tick):** `approval-check.yml` gates a PR on `approved` **only if it carries `claude`** (`.claude/docs/PIPELINE.md` → "Permission rationale"), so a pipeline PR that lost that label merges with no gate at all — and CI cannot tell it from a human PR. The sweep query above lists exactly those. Report them **only when the set changes**, in one line, and **never add the label automatically**:

> ⚠️ Pipeline PRs without the `claude` label (approval gate inactive): #501. Say "gate #501" or add the label on GitHub.

**Denial report (each tick):** stage agents auto-deny disallowed commands (`dontAsk`) instead of prompting; a `PreToolUse` hook logs each one to **`.agents/denials.log`** (gitignored, base repo). Read it each tick and track how many lines are new since the previous tick. If denials **cluster** — say **≥3 new**, or the same command repeated — report it **once**, e.g. _"⚠️ 4 commands auto-denied this tick (e.g. `npx prisma migrate …` ×2, `printf … >` ×1) — the pipeline likely needs a permission/instruction change."_ Do **not** prompt or act on it automatically; this is visibility so the human knows when to harden the pipeline. A few isolated denials are normal and need no report.

## Dispatching
Expand Down Expand Up @@ -166,10 +173,11 @@ Interpret intent, not literal syntax:
Dispatch the plan agent the same tick.

- **"scope out X" / "break down X"** — Stage 0 deserves a stronger model than haiku; suggest the human run `/scope` in their main session.
- **"status"** — re-run the tick queries **live** and build the table from them (never from session memory): each in-flight item + stage, each item waiting on the human, and each PR currently labeled `approved` (the live `gh pr list --assignee "@me" --label approved` result — a merged PR has already dropped out, so it must not appear). It **inherits the assignee filter**, so it reports only this operator's items; append the unowned sweep result as a separate **"unowned"** line so a stalled ticket is diagnosable from one command.
- **"status"** — re-run the tick queries **live** and build the table from them (never from session memory): each in-flight item + stage, each item waiting on the human, and each PR currently labeled `approved` (the live `gh pr list --assignee "@me" --label approved` result — a merged PR has already dropped out, so it must not appear). It **inherits the assignee filter**, so it reports only this operator's items; append the unowned sweep result as a separate **"unowned"** line, and the ungated-PR sweep result as an **"ungated"** line, so a stalled ticket or a PR with no approval gate is diagnosable from one command.
- **"pause #N"** — remove the item's current trigger label; confirm what was removed. Same ownership rule as opt-in: if the item belongs to **another operator**, say so and stop rather than touch its labels.
- **"resume #N" / "retry #N"** — re-apply the trigger label for where it stalled (issue stuck in `planning` → `ready`; PR stuck in `revising` → `needs revision`; PR stuck in `refreshing` → `refresh branch`; etc.). Same ownership rule as opt-in: if the item is **unassigned**, add `--add-assignee "@me"` in the same command (re-applying a trigger to an unassigned item is a no-op for every cockpit); if it belongs to **another operator**, say so and stop rather than re-trigger.
- **"refresh #N"** — apply `refresh branch` to that PR and dispatch it this tick, bypassing the per-merge cap. Use it to force a fresh preview deployment on a PR left quota-red. Same ownership rule as opt-in.
- **"gate #N"** — apply the missing `claude` marker to a PR the ungated sweep reported: `gh pr edit <n> --repo SGAOperations/aplio --add-label "claude"`. The `labeled` event re-evaluates `approval-check.yml`'s condition, so the approval gate is live on that run. Same ownership rule as opt-in.

## Stop controls

Expand Down
7 changes: 4 additions & 3 deletions .github/workflows/approval-check.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# Gates PRs into `dev` on the `approved` pipeline label. `labeled`/`unlabeled`
# re-trigger so the verdict updates as the pipeline swaps labels.
# Gates `dev` PRs on `approved`, only when labeled `claude` — others skip.
# NEVER narrow the trigger or rename the job: no check run = pending forever.
# `branches: [dev]` is safe only because the required-check registration is
# `dev`-scoped too — NEVER add this to the `main/dev` ruleset (id 15248252);
# dev → main release PRs carry no pipeline labels.
Expand All @@ -18,6 +18,7 @@ permissions:
jobs:
run-approval-check:
runs-on: ubuntu-latest
if: contains(github.event.pull_request.labels.*.name, 'claude')
timeout-minutes: 5
steps:
- name: Check pipeline labels
Expand Down Expand Up @@ -58,7 +59,7 @@ jobs:
EOF

if ! grep -Fxq "approved" "$RUNNER_TEMP/labels.txt"; then
echo "::error::Missing the 'approved' label. A PR into dev is merge-able only after review-agent (or a human, for manual PRs) applies 'approved'."
echo "::error::Missing the 'approved' label. This PR carries 'claude', so it is a pipeline PR and merges only once review-agent applies 'approved'. If it is not a pipeline PR, remove the 'claude' label and this check will skip."
FAILED=1
fi

Expand Down
Loading
Loading