From e9bdf2b5f8c1916deb7aed5d85b7fc7847f7b362 Mon Sep 17 00:00:00 2001 From: b-at-neu Date: Wed, 19 Aug 2026 18:17:49 -0400 Subject: [PATCH 1/2] #513 gate the approval check on the claude label MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The gate exists to stop mid-pipeline PRs merging early, not to enforce review universally, so PRs outside the pipeline now skip it — which also unblocks Dependabot with no special case. Skipping at the job level still reports the required check as `skipped`; filtering the trigger would leave it pending forever. Trade-off is fail-open: a pipeline PR missing `claude` loses its gate, so impl-agent applies the label at `gh pr create` and the cockpit sweeps for tracked PRs without it. Co-Authored-By: Claude Opus 5 --- .claude/agents/impl-agent.md | 3 +++ .claude/docs/PIPELINE.md | 24 ++++++++++++++---------- .claude/skills/pipeline/SKILL.md | 12 ++++++++++-- .github/workflows/approval-check.yml | 16 +++++++++++++--- CLAUDE.md | 2 +- 5 files changed, 41 insertions(+), 16 deletions(-) diff --git a/.claude/agents/impl-agent.md b/.claude/agents/impl-agent.md index 881b2fb9..8f22af95 100644 --- a/.claude/agents/impl-agent.md +++ b/.claude/agents/impl-agent.md @@ -97,11 +97,14 @@ gh issue edit N --repo SGAOperations/aplio --remove-label "plan approved" --add- --title "#N " \ --body-file .temp/pr-N.md \ --assignee "" \ + --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. diff --git a/.claude/docs/PIPELINE.md b/.claude/docs/PIPELINE.md index 47f956aa..b533dcdc 100644 --- a/.claude/docs/PIPELINE.md +++ b/.claude/docs/PIPELINE.md @@ -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 @@ -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. @@ -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 --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 --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 | diff --git a/.claude/skills/pipeline/SKILL.md b/.claude/skills/pipeline/SKILL.md index 1c889f54..b092bd8c 100644 --- a/.claude/skills/pipeline/SKILL.md +++ b/.claude/skills/pipeline/SKILL.md @@ -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**: @@ -77,6 +80,10 @@ gh pr view --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 @@ -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 --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 diff --git a/.github/workflows/approval-check.yml b/.github/workflows/approval-check.yml index 25d8534f..d733f6e5 100644 --- a/.github/workflows/approval-check.yml +++ b/.github/workflows/approval-check.yml @@ -1,5 +1,14 @@ -# Gates PRs into `dev` on the `approved` pipeline label. `labeled`/`unlabeled` -# re-trigger so the verdict updates as the pipeline swaps labels. +# Gates PRs into `dev` on the `approved` pipeline label — but **only** for +# pipeline PRs, i.e. those carrying `claude` (the job-level `if:` below). Every +# other PR (human, Dependabot) gets a `skipped` check run, which GitHub counts +# as a satisfied required check, so it merges on its own merits. +# `labeled`/`unlabeled` re-trigger so both the gate condition and the verdict +# update as labels change — adding `claude` activates the gate on that run. +# NEVER narrow the trigger (`paths`, `branches`, dropping an event) to skip the +# check: a workflow that never runs creates no check run, so the required check +# sits pending forever and blocks the merge permanently. Skip at the JOB level. +# Renaming the job `run-approval-check` does the same — that ID is the check +# registered as required. # `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. @@ -18,6 +27,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 @@ -58,7 +68,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 diff --git a/CLAUDE.md b/CLAUDE.md index 801317e8..fae5b46c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -34,7 +34,7 @@ Next.js 16 (App Router, React 19) · Prisma 7 · Tailwind CSS 4 · shadcn/ui (Ra - **Commit:** subject `#XXX message in lowercase imperative mood` (no colon after the number, **under 80 chars**, no trailing period); then — only if the _why_ isn't obvious — a blank line and a short body (wrap ~72, a few lines max; narrative belongs in the PR, not the commit); then a blank line and a `Co-Authored-By: Claude Sonnet 4.6 ` trailer. Commit each logical unit separately. **Write the message to a file and `git commit -F .temp/commit-msg.txt`** — inline multi-line `-m … -m …` collapses on Windows, dropping the subject and the co-authorship. Delete tracked files with `git rm`. **The subject-line format is enforced locally by a `commit-msg` hook** (installed automatically via `npm run prepare` / `npm ci`) **and in CI by `run-commit-message-check`**, which validates every commit in a PR by invoking that same hook — so the two cannot drift. - **Branch:** `XXX-ticket-name-in-kebab-case`, branched off `dev`. -- **PR:** title `#XXX Ticket Name In Title Case`; **base branch `dev`** (never open feature PRs against `main`); body contains `Closes #XXX`; assign the **issue's assignee** (fallback: yourself — `@me`). `dev → main` is promoted only by `/release`. +- **PR:** title `#XXX Ticket Name In Title Case`; **base branch `dev`** (never open feature PRs against `main`); body contains `Closes #XXX`; assign the **issue's assignee** (fallback: yourself — `@me`); **pipeline-authored PRs also carry the `claude` label** — it is what activates the `approved` merge gate (`approval-check.yml`), and without it the PR merges ungated. `dev → main` is promoted only by `/release`. - **Rebase conflicts in pipeline:** `revise-agent` attempts autonomous resolution for structurally unambiguous conflicts (non-overlapping sections, generated files, dual independent imports). It escalates to `needs human` only when both sides modified the same logical unit. Agents should document every resolution in the revision summary. Full protocol: `.claude/docs/PIPELINE.md` → "Rebase conflict protocol". ## Preview databases (Neon branch budget) From 0cf9275ea92a41f9e02ab2247575a964fa8e521e Mon Sep 17 00:00:00 2001 From: b-at-neu Date: Wed, 19 Aug 2026 18:27:41 -0400 Subject: [PATCH 2/2] #513 trim the approval-check header comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ENGINEERING.md §7 caps comments at two lines; PIPELINE.md's new "CI merge gate" paragraph already carries the full rationale. Co-Authored-By: Claude Opus 5 --- .github/workflows/approval-check.yml | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/.github/workflows/approval-check.yml b/.github/workflows/approval-check.yml index d733f6e5..158f0aa0 100644 --- a/.github/workflows/approval-check.yml +++ b/.github/workflows/approval-check.yml @@ -1,14 +1,5 @@ -# Gates PRs into `dev` on the `approved` pipeline label — but **only** for -# pipeline PRs, i.e. those carrying `claude` (the job-level `if:` below). Every -# other PR (human, Dependabot) gets a `skipped` check run, which GitHub counts -# as a satisfied required check, so it merges on its own merits. -# `labeled`/`unlabeled` re-trigger so both the gate condition and the verdict -# update as labels change — adding `claude` activates the gate on that run. -# NEVER narrow the trigger (`paths`, `branches`, dropping an event) to skip the -# check: a workflow that never runs creates no check run, so the required check -# sits pending forever and blocks the merge permanently. Skip at the JOB level. -# Renaming the job `run-approval-check` does the same — that ID is the check -# registered as required. +# 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.