Skip to content

feat(devx): gate .claude/skills/** on the ADR merge-approval check - #9407

Merged
os-zhuang merged 1 commit into
mainfrom
claude/issue-9395-adr-gate-skills-paths
Aug 17, 2026
Merged

feat(devx): gate .claude/skills/** on the ADR merge-approval check#9407
os-zhuang merged 1 commit into
mainfrom
claude/issue-9395-adr-gate-skills-paths

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Fixes#9395

Implements the maintainer ruling on #9319 decision 2 (2026-08-17, 「同意」 batch): the ADR maintainer approval gate's path predicate extends to .claude/skills/**, so Prime Directive #14's human-merge reservation is machine-enforced on the lane's own protocol surface instead of declared. The #8012 armed-auto-merge clause reaches the new path class identically.

This PR is not itself skill-surface — it touches scripts/ and .github/workflows/ only, so it lands through the queue normally. The gate's own run on this diff says so: ✅ No files under docs/adr/** / .claude/skills/** in this diff (2 changed file(s)).

What changed

scripts/check-adr-merge-approval.mjs

.github/workflows/adr-merge-approval.yml — comments and the step name only. No trigger change, and no job rename (the job name is the required status context registered in scripts/check-required-contexts.mjs; renaming it would leave the ruleset waiting for a context that never reports).

Trigger verification (verified, not assumed)

Two directions, both recorded in the workflow header:

  1. Structural: no paths / paths-ignore filter exists on any of the three triggers, so path membership cannot affect whether the workflow runs. A path filter skips the whole workflow, so the check context is never created — a required context that never reports hangs the merge queue until the ruleset's 60-minute timeout (objectui#3523). Adding one for the new prefix would be a regression, not an improvement.
  2. Empirical: PR docs(pm): label writes re-read current labels in the same action — stale snapshots are invalid snapshots (#9226) #9238 — a .claude/skills/**-only diff, before any widening — already produced a completed ADR maintainer approval check run: job 95293672713, conclusion success, via the zero-lookup clean path. The run existed; only the verdict on it changes. auto_merge_enabled / auto_merge_disabled are likewise path-agnostic, so clause (2) observes arming on the new surface on the same terms.

Reverse verification (predicted, then observed)

Predicted directions were written down before each run; all four matched. The old script is git show origin/main:scripts/check-adr-merge-approval.mjs, run side by side with the new one over identical inputs.

Offline replay (real CLI, --files-json / --reviews-json / --pull-json), diff = .claude/skills/pm-dispatch/SKILL.md + package.json (a mixed diff, deliberately):

#inputspredictedobserved
1OLD gate, zero reviews, disarmedGREEN (the #9238 hole)exit=0No files under docs/adr/ in this diff (2 changed file(s))
2NEW gate, same inputsRED, missing approvalexit=11 file(s) under .claude/skills/**Prime Directive #14
3NEW gate, APPROVED + disarmedGREENexit=0review standing is APPROVED (approved by 'hotlong') and auto-merge is OFF
4NEW gate, APPROVED + ARMEDRED for the armingexit=1AUTO-MERGE IS ARMED … enabled by 'os-zhuang'

Rows 3 and 4 are the clause-(2) emptiness proof on the new path class: identical reviews, identical files, only the arming bit differs, opposite verdicts. Without it the widening would be inert on exactly the state #9238 was in.

Gate mode over a real git diff (a throwaway detached worktree at origin/main with one synthetic edit to a .claude/skills/** file, never committed, never pushed, removed afterwards):

  • OLD: exit=0, ✅ No files under docs/adr/ in this diff (1 changed file(s), git diff against merge-base with origin/main).
  • NEW: exit=1, and the log line .claude/skills/** touched -- consulting reviews of PR #9379 (resolved via HEAD commit subject) proves the predicate matched and the gated path was entered. The local red is then the fail-loud path (HTTP 401: Bad credentials — this container has no usable GitHub token), not the approval verdict; the verdict-level demonstration is the offline replay above. Reported as observed rather than as the template's presumption.

Tests

All at cc1a065 (git rev-parse --short HEAD after the final commit):

  • pnpm check:adr-merge-approval✓ 96 assertions (was 66 on origin/main: +30). Every pre-existing assertion still runs — no existing check is weakened.
  • Gate families re-derived from the actual changed paths with node scripts/pm/dispatch-gates.mjs, all green: check:node-version, check:required-contexts, check:shard-attestation, check:workflow-status-functions, plus check:nul-bytes (any-edit) and npx eslint scripts/check-adr-merge-approval.mjs --no-inline-config (clean).

node scripts/pm/dispatch-gates.mjs --tier scripts/check-adr-merge-approval.mjs .github/workflows/adr-merge-approval.yml, verbatim:

Model tier — no path-derived mandate: the surface hits none of the 1 declared glob(s), derived here, not recalled.
The tier stays the PM's per-card judgment call (floor sonnet · default opus · ceiling fable).
Clause ② is NOT reachable from paths: a card that changes contract accept/reject behaviour or widens the public surface is fable-mandatory too, judged from the card CONTENT. This line is a FLOOR, never a clearance.

New assertions worth naming (the ones that fail if the widening is faked):

  • skill-prefix-matches-only-dot-claude-skills and the-published-skills-catalog-is-NOT-governed — the trailing slash and the catalog exclusion, both near misses pinned.
  • skills-armed-clause-changes-a-verdict-that-would-otherwise-be-green — the emptiness proof for the new class.
  • a-mixed-skills-diff-is-gate-scoped and proportion-changes-no-verdict — a 1-of-4 governed diff must be as red as a 1-of-1 one.
  • a-skills-red-does-NOT-cite-the-adr-rule / an-adr-red-does-NOT-cite-the-skill-rule / a-mixed-red-carries-BOTH-rule-texts — the distinct-wording requirement, asserted on rendered text.
  • historical-pr-9238-is-red-BECAUSE-of-the-widened-predicate — narrow the predicate back to the ADR prefix and this one fails while every other historical replay stays green.

Out of scope, filed

Changeset

None: scripts/ + .github/workflows/ only, nothing published changes. The skip-changeset label is applied to this PR.


Generated by Claude Code

…9395)
Implements the maintainer ruling on #9319 decision 2 (2026-08-17): the
`ADR maintainer approval` gate's path predicate extends to
`.claude/skills/**`, so Prime Directive #14's human-merge reservation is
machine-enforced on the lane's own protocol surface rather than declared.
- the single ADR prefix becomes a GOVERNED_SURFACES table: identical pass
conditions (an APPROVED standing + no armed auto-merge), distinct failure
wording, each surface naming its own rule;
- the #8012 armed-auto-merge clause reaches the new path class identically,
with its own emptiness proof (identical reviews, opposite verdicts);
- one path hit governs the whole PR -- a mixed diff is never judged by
proportion, pinned by comparing a 1-of-4 verdict with a 1-of-1 one;
- the report renderer is pure, so the "each surface names its own rule"
requirement is asserted on the text an operator actually reads;
- PR #9238 -- the skills-only PR the merge queue landed with zero reviews --
joins the historical replay fixtures as a real capture, pinned RED.
The workflow needed no trigger change: it carries no `paths` filter (a path
filter would skip the whole workflow and hang the queue on a required context
that never reports), and #9238 already produced a completed `ADR maintainer
approval` check run through the clean path.
No existing check is weakened: every prior assertion still runs, and the
self-test goes from 66 to 96 assertions.
Fixes#9395
@os-zhuangClaude

Copy link
Copy Markdown
ContributorAuthor

ACCEPT — PM review at cc1a065 (dispatching seat, session session_01NYgmGheCzM6NrHZN436Cxf).

Verified against the diff, not the report: two files exactly (gate script + workflow); the docs/adr/ predicate survives untouched (all removed lines are comment-header rewrites into the GOVERNED_SURFACES framing); distinct failure wording per surface with the #9319 ruling and PD #14 quoted in place; required job name unchanged; self-tests 66 → 96 including the #9238 real-capture replay pinned RED and the armed-auto-merge clause on the new path class. The four-leg reverse verification (old-gate-green / new-gate-red / approved-green / armed-red on the same mixed diff) is exactly the acceptance evidence the card demanded.

Both out-of-scope findings routed correctly (#9403 AGENTS.md PD #14 text, #9404 published skills/ catalog — deliberate non-inclusion per the ruling's named scope).

Flipped ready + armed merge-when-ready; the in-flight ESLint (which carries the gate self-test family) and Type Check gate the landing via the required set. Not a governed-surface diff — lands through the lane. Merge closes #9395.


Generated by Claude Code

@os-zhuang
os-zhuang marked this pull request as ready for review August 17, 2026 23:19
@os-zhuang
os-zhuang enabled auto-merge August 17, 2026 23:19
@os-zhuang
os-zhuang added this pull request to the merge queueAug 17, 2026
Merged via the queue into main with commit c1d026eAug 17, 2026
25 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-9395-adr-gate-skills-paths branch August 17, 2026 23:42
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci/cdsize/lskip-changesetPR has no user-facing published change; bypasses the changeset gate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

gate: extend ADR maintainer approval path predicate to .claude/skills/** — make Prime Directive #14 enforced (#9319 ruling)

2 participants

@os-zhuang@claude