fix(repo): add pull_request_template scaffolding ## Related contract#490
Merged
Conversation
Add .github/pull_request_template.md scaffolding the sections the pr-issue-linkage check enforces: a native closing-keyword placeholder (Closes #) plus ## Summary / ## Fix / ## Verification / ## Related, with guidance in HTML comments. The validator (ci-workflows reusable) is the SSOT; the template only mirrors its required ## Related heading and closing-keyword contract so web/editor authors scaffold a passing body without a red-CI round-trip. Comments are stripped by the validator, so an unedited template correctly fails until filled — matching the check's explicit anti-vacuous-pass design. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011V31qpAHP3jfs76B9d5Rfo
|
Claude finished @kyle-sexton's task in 1m 20s —— View job PR Review
Summary: This is a clean, well-scoped addition. The template correctly mirrors the validator contract and the anti-vacuous-pass guard works as designed. No blocking issues found. CorrectnessContract alignment is accurate. The template's Anti-vacuous-pass guard holds. Both failure modes (empty "No linked issue" escape hatch is documented. The guidance comment on the closing-keyword block describes the fallback string correctly. One gap: the 🟡 Nit — SecurityNo credentials, tokens, or executable code introduced. The template is inert markdown; no attack surface. CLAUDE.md / REVIEW.md alignmentAll six blocking criteria (object-level authz, tenant scoping, secrets, injection, audit logging, atomicity) are inapplicable to a Markdown file — none triggered. The "do not report" clause correctly suppresses CI-enforced items; markdownlint, editorconfig, and typos results are all claimed clean and not flagged here. Verdict: Approve. The single nit above is non-blocking. The template adds clear value by surfacing the contract up-front and the design (fail-until-filled) is sound. |
The pull_request_template ## Related guidance only showed the linking case (Refs #N), leaving the "No linked issue" path under-documented: an author could replace the closing keyword and leave ## Related empty, which the pr-issue-linkage validator rejects (its only failure mode is an empty section). Note that the section stays required and to use N/A when nothing applies. Comment-only; the validator strips HTML comments, so the template's fail-until-filled behavior is unchanged. Addresses the claude[bot] review nit on #490. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 4s —— View job PR Review (re-review after synchronize)
The synchronize pushed one new commit ( What changedThe second commit ( These appear at CorrectnessPrevious nit: resolved. The gap identified in the first review — the
Both failure modes (empty
SecurityNo change to the security posture — still inert markdown, no executable code, no credentials. CLAUDE.md alignmentThe file is a repository governance artifact, not a plugin, so plugin-specific rules (repo-agnostic paths, VerdictApprove. The previous nit is addressed and no new issues are introduced. The template is clean, the contract is fully documented, and the anti-vacuous-pass guard remains intact. |
Contributor
Author
|
Addressing the claude[bot] review (run 29693725085).
Verdict was Approve (nit non-blocking); the single nit is now closed. No thread to resolve — this is an issue-level review comment, not an inline thread. |
kyle-sexton
added a commit
that referenced
this pull request
Jul 20, 2026
#593) ## Summary This repo edits its own skills daily via the autonomous pipeline, yet no CI lane invoked the `skill-quality` checker — 120 `evals/evals.json` went unexecuted and the 17-check static gate (`skill-quality:check`) was invoked by nothing. This wires that checker into CI as a deterministic skill-regression net. Implements **stage 1 only** of #530 (the "cheap, now" deterministic lane). Stages 2-5 (model-graded judges, frozen regression suite, pass^k autopilot-unlock metric, clean-state isolation) remain a separate decompose target per the ratified operator decision on the issue. ## Fix New `skill-quality-gate` job in `.github/workflows/ci.yml`, added to the `ci-status` required aggregate: - **Static gate (`scripts/check-changed-skills.sh`)** — on a PR, for each skill under `plugins/*/skills/**` the diff touches, runs `plugins/skill-quality/scripts/check-skill.sh` with `CHECK_SKILL_SKILLS_ROOT` set to the owning plugin's `skills/` dir and `CHECK_SKILL_BASE_REF` set to the PR base. Because the checker's trigger-keyword-preservation check diffs against that base, a rewrite that silently drops a `description` trigger phrase (an auto-invocation regression) fails the check — replacing the work lane's manual high-blast-radius diff read with a deterministic one. Vendor subtrees map to the owning skill; deleted skills are filtered; any FAIL (or env error) exits non-zero. `markdownlint` (check 6) is deliberately skipped here (`CHECK_SKILL_SKIP_MARKDOWNLINT=1`) — SKILL.md markdown is already gated by the hygiene lane. - **validate-evals** — the existing `check-jsonschema` composite action schema-validates every `evals/evals.json` against the bundled `plugins/skill-quality/reference/evals.schema.json`. - **Self-test (`scripts/check-changed-skills.test.sh`)** — runs first so a broken orchestrator cannot mask a real regression behind a green gate. **No silent-skip (#445 precedent):** the job itself never skips (a skipped required job reports success to branch protection) — only the inner PR-diff step is event-gated (mirroring `hook-utils-sync`'s `--check-bump`), and the self-test + eval-schema steps give push-to-main a passing path. Missing base ref or missing checker fails closed (exit 2). **No new reusable action:** reuses this repo's own local script plus the already-referenced `check-jsonschema` composite action — no change to `melodic-software/ci-workflows`. **No plugin modified → no version bump** (consistent with CI-only PR #490, which bumped nothing). No plugin frontmatter, triggers, hooks, or cross-plugin contracts are touched. ## Verification Local, in the worktree: - **Orchestrator unit tests** — `bash scripts/check-changed-skills.test.sh` → `PASS=8 FAIL=0` (covers changed-skill detection, vendor-subtree mapping, dedupe, deletion filtering, env passthrough, pass/fail aggregation, fail-closed on bad base ref / missing checker). - **ShellCheck** — `shellcheck --rcfile .shellcheckrc scripts/check-changed-skills.sh scripts/check-changed-skills.test.sh` → clean. - **End-to-end, real `check-skill.sh` against real skills** (`scripts/check-changed-skills.sh origin/main`): - Happy path (a changed skill with no regression) → `PASS`, exit 0. - Simulated regression (dropped the `'lint my skill'` trigger from `skill-quality/check`) → `FAIL: dropped trigger keyword(s) vs origin/main (auto-invocation regression): 'lint my skill'`, exit 1. - **validate-evals** — `check-jsonschema --schemafile plugins/skill-quality/reference/evals.schema.json` over all `plugins/*/skills/*/evals/evals.json` → 120/120 pass today (glob matches exactly the 120 eval sets, excludes vendor). Closes #603 ## Related - #530 — this PR is stage 1 of the staged quality-net umbrella; stages 2-5 stay a decompose target. - #445 — mechanical-conformance CI backlog; this extends gating from conformance to behavior and follows its silent-skip-gate anti-pattern warning. - #477-#480 — the rule→skill absorption lifecycle whose regression net this begins to supply. - #513 — the mini-SDLC verify lane that consumes these gates. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
kyle-sexton
added a commit
that referenced
this pull request
Jul 20, 2026
#617) (#622) Closes #617 ## Summary Docs-only PRs — operator precedent codifications and other `docs/topics/` churn, a large share of current throughput per #617 — ran the full plugin contract suite (`plugin-gate`: Node + Python installs, every `plugins/**/*.test.sh`, manifest + catalog validation) and the full `miro-plugin` Node build for a diff that cannot affect either. Both lanes now short-circuit their expensive steps when the diff is confined to a small, positive, provably-inert allowlist, reporting an honest evaluated-and-not-applicable success. ## Fix **Mechanism — in-job detection, never a job skip.** GitHub's docs are explicit: *"If a workflow is skipped due to path filtering, branch filtering or a commit message, then checks associated with that workflow will remain in a 'Pending' state. A pull request that requires those checks to be successful will be blocked from merging."* whereas *"If, however, a job within a workflow is skipped due to a conditional, it will report its status as 'Success.'"* ([troubleshooting-required-status-checks](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/collaborating-on-repositories-with-code-quality-features/troubleshooting-required-status-checks)). So workflow-level `paths:`/`paths-ignore:` is ruled out for required checks. It is doubly ruled out here: the sole required check is the `ci-status` aggregate, which fails unless every lane's `result` is exactly `success` — a conditionally-*skipped* job's result is `skipped`, not `success`, so even a job-level `if:` would red the aggregate. The safe pattern is therefore the one #593/#609 already use: the job always runs, self-tests its detector first, and gates only its inner expensive steps, staying `success` and reporting to branch protection either way. New files: - **`scripts/check-docs-only.sh`** — emits `docs_only=true|false` to `$GITHUB_OUTPUT`; `true` iff every path changed vs the base ref matches an allowlist prefix. Fail-closed toward *running* the full suite: an unresolvable base ref, an empty/missing allowlist, or an empty diff all emit `false` with a stderr note and exit 0 (a code PR is not an error — "has code" is never encoded as a non-zero exit that would red every code PR). - **`scripts/docs-only-paths.txt`** — the allowlist as external, extensible **data** (same shape as #609's token list). **Allowlist, not blocklist, by design**: a blocklist ("skip on any doc the validators don't currently read") silently turns false-green the day a validator starts reading a new doc; a positive list fails safe — any unlisted path, including a brand-new code input, forces the full suite. Seeded with `docs/topics/` only. Adding an entry is a two-part proof (inert + a self-test case). - **`scripts/check-docs-only.test.sh`** — the honesty proof, run unconditionally in both wrapped jobs before anything expensive. In `ci.yml`, `plugin-gate` and `miro-plugin` each gain: `fetch-depth: 0`, an always-run detector self-test, a PR-only detect step (`id: scope`), an `if: steps.scope.outputs.docs_only != 'true'` guard on every install/test step, and an explicit *"not applicable to a docs-only diff — reporting success"* log step. On `push` the detect step doesn't run, the output is empty, and every real step runs — main always gets the full suite. **Job classification** (only jobs that *cannot* be affected by a docs-only diff are scoped; when unsure, not scoped): | Lane | Scoped? | Rationale | |---|---|---| | `plugin-gate` | **Yes** | Heaviest lane. `run-plugin-tests.sh` is hermetic (tests build their own fixture repos); `validate-plugins.sh` reads only `README.md`, `docs/PLUGIN-ARTIFACT-PROTOCOL.md`, `docs/CATALOG-TAXONOMY.md` (all excluded from the allowlist) + `plugins/**` + toolchain files. A `docs/topics/`-only diff touches none of these. | | `miro-plugin` | **Yes** | Every step is `working-directory: plugins/miro`; a docs-only diff provably cannot touch it. | | `hygiene` | No | Markdownlint / typos / comment-hygiene **do** read the changed docs — this is the lane that *should* run on a docs PR. | | `skill-quality-gate` | No | Already self-scopes to changed skills (empty set on a non-skill docs diff); eval-schema step is cheap. | | `zizmor` | No | Advisory reusable workflow; scoping a reusable call partly belongs to `ci-workflows`. | | `hook-utils-sync`, `standards-contract-sync`, `cross-plugin-source-drift`, `silent-skip-gate`, `runner-policy` | No (deferred) | Provably docs-inert, but off the critical path (fast bash tests, no heavy installs) — wrapping them adds surface for negligible latency gain. Trigger to revisit: if any becomes install-bearing. | **Deferred with trigger:** scoping `miro-plugin` to `plugins/miro/**` specifically (skipping it on *any* non-miro diff, not just docs-only) is a larger, separate optimization broader than #617's docs-only framing; trigger: a dedicated follow-up issue for per-lane path scoping. ## Verification In the worktree (`ubuntu`/Git Bash): - **`bash scripts/check-docs-only.test.sh` → `PASS=16 FAIL=0`.** Cases pin: `docs/topics/`-only (incl. new nested files) → `true`; `README.md`, `docs/PLUGIN-ARTIFACT-PROTOCOL.md`, `docs/CATALOG-TAXONOMY.md` → **`false`** (the grep payoff — these are consumed by `plugin-gate`); any non-topics `docs/**`, any `plugins/**` incl. `SKILL.md`, `scripts/`, `.github/`, toolchain lockfiles, a sibling prefix (`docs/topics-archive/`), and mixed docs+code → `false`; unresolvable base ref → `false` exit 0; empty allowlist → `false`; `$GITHUB_OUTPUT` written. - **`shellcheck --rcfile .shellcheckrc`** on both scripts → clean. **`shfmt -d`** → no diff. **`actionlint .github/workflows/ci.yml`** → clean. - **Provenance of the "inert" claim:** `git grep` over `plugins/**/*.test.sh`, `validate-plugin-contracts.mjs`, `generate-catalog.mjs`, and the manifests confirmed `generate-catalog.mjs --check` consumes `README.md` and `docs/CATALOG-TAXONOMY.md` and `validate-plugin-contracts.mjs` requires `docs/PLUGIN-ARTIFACT-PROTOCOL.md` — which is exactly why those are excluded from the allowlist and the docs-only win is `docs/topics/`-scoped, not "all markdown". - **zizmor:** base ref flows via `env: BASE_REF` (no `${{ }}` interpolation inside `run:`), matching the existing `hook-utils-sync`/`skill-quality-gate` steps; `persist-credentials: false` retained; no new permissions. Honest scope note: the win is narrow by construction — it covers `docs/topics/`-only PRs and deliberately does **not** cover README/philosophy edits, `SKILL.md` edits (code), or plugin-`CHANGELOG` edits (code). Narrow-but-sound is the point. ## Related - #532 — false-green / liveness-assertion convention this honors (evaluated-and-not-applicable success, not a silent skip). - #593 — the merged `skill-quality-gate` whose never-skip, self-test-first, event-gated job shape this mirrors. - #609 — open PR also touching `ci.yml` (adds the `portability-lint` lane + one line to the `ci-status` needs list). Different file regions from this PR (new job block + needs line vs. the `plugin-gate`/`miro-plugin` step bodies); composes without conflict. Whichever merges second rebases. - No `plugins/<name>/` directory is touched, so no version bump / CHANGELOG entry (matching CI-only precedent #490/#593). Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
kyle-sexton
added a commit
that referenced
this pull request
Jul 20, 2026
…y diffs (#628) (#659) ## Summary Docs-only PRs still paid the `hygiene` job's path-scoped linter cost — `actionlint` and four `check-jsonschema` steps — for a diff that provably cannot affect any of them. #617 (via #622) short-circuited `plugin-gate` and `miro-plugin`, which cut runner-minutes but not the merge-latency bottleneck. This applies the SAME self-test-first, fail-closed, inner-step-gated discipline to `hygiene`'s genuinely path-scoped subset, so those runs report an honest evaluated-and-not-applicable success instead of running. `ShellCheck` and `exec-bit` are deliberately NOT gated: both scan the whole repo extension- and directory-agnostically (`ShellCheck` lints every tracked `*.sh`/`*.bash` via `git ls-files`; `exec-bit` flags every tracked shebang file recorded `100644` via `git grep -- .`), so a shell or shebang file added under an otherwise docs-only prefix like `docs/topics/` is real input they must still catch. They run on every diff alongside the docs-linting checks that read the changed docs directly: `markdown`, `typos`, `editorconfig`, `gitleaks`, `eol-renormalize`, `comment-hygiene`, `machine-specific-paths`. > **Scope of the perf win (honest):** the initial revision of this PR also gated `ShellCheck` (~53s, the dominant cost) and `exec-bit`. Bot review (P2, thread on `ci.yml`) surfaced that both are whole-repo scanners, so gating them opened a **fail-closed hole** — a docs-only PR adding a mode-`100644` shebang file (or a `*.sh`) under `docs/topics/` skipped both and the ternary mapped them to `success`, letting a file the commit procedure requires `exec-bit` to catch merge unlinted. Ungating them (commit `f73be27`) closes the hole but removes the dominant ShellCheck skip, so the remaining docs-only win is limited to `actionlint` + the four `check-jsonschema` steps. A perf-recovery follow-up (cache/pin the ShellCheck install, which downloads a release tarball every run) is tracked separately and lives in `melodic-software/ci-workflows`, not this repo. ## Fix Reuses the existing `scripts/check-docs-only.sh` detector and `scripts/docs-only-paths.txt` allowlist from #622 unchanged — no new detector. In the `hygiene` job (`.github/workflows/ci.yml`): - Added the always-run `Test the docs-only detector` self-test and a PR-only `Detect a docs-only diff` step (`id: scope`), mirroring `plugin-gate`/`miro-plugin`. The checkout already had `fetch-depth: 0`, so the base ref resolves. - Gated exactly the five path-scoped steps with `if: steps.scope.outputs.docs_only != 'true'`: `actionlint`, `marketplace_schema`, `plugin_schema`, `dependabot_schema`, `workflow_schema`. `shellcheck` and `exec_bit` stay unconditional. - The job never skips and the aggregator is never weakened. The `hygiene` job uses `continue-on-error: true` per step plus a `CHECK_RESULTS` feed into `scripts/aggregate-hygiene-results.sh`, which fails closed on any non-`success` outcome (including `skipped`). A gated step that skips would therefore red the job — so each gated entry in `CHECK_RESULTS` maps the intentional docs-only skip to `success` in the expression layer: ```yaml actionlint=${{ steps.scope.outputs.docs_only == 'true' && 'success' || steps.actionlint.outcome }} ``` This is honest, not a swallow: the ternary condition (`docs_only == 'true'`) is the exact inverse of each step's gate, so `success` is fed ONLY for a step that provably did not run — no real outcome is ever masked. The two whole-repo scanners (`shellcheck`, `exec-bit`) feed their raw `.outcome`. The aggregator script is untouched and still fails closed on any real `failure` (or a `skipped` it isn't told to tolerate). This is the same evaluated-and-not-applicable success `plugin-gate` reports with its log step, expressed here through the feed the hygiene job already uses. - Added an explicit `Report docs-irrelevant checks not applicable` log step (gated on `docs_only == 'true'`) so a human reading a green run understands why those checks show skipped. Also extended the two non-workflow artifacts per the established two-part-proof discipline: - `scripts/docs-only-paths.txt` — updated the inertness-proof comment to name only the genuinely path-scoped gated subset (`actionlint` + the four schema checks, none of which read `docs/topics/`) and to state explicitly that `ShellCheck` and `exec-bit` scan the whole repo and stay unconditional (as do the docs-linters). Allowlist itself unchanged (still `docs/topics/` only — no growth). - `scripts/check-docs-only.test.sh` — added three cases pinning the remaining `check-jsonschema` inputs to `false` (`.claude-plugin/marketplace.json`, `plugins/*/.claude-plugin/plugin.json`, `.github/dependabot.yml`); the `actionlint` / workflow-schema inputs (`.github/workflows/ci.yml`) were already pinned false. These fail the moment someone widens the allowlist to cover an input a gated check reads. ## Verification Run in the worktree (Git Bash): - **`bash scripts/check-docs-only.test.sh` → `PASS=19 FAIL=0`.** Confirms `docs/topics/`-only → `true` and every gated check's input (`.github/workflows/ci.yml`, `.claude-plugin/marketplace.json`, `plugins/*/.claude-plugin/plugin.json`, `.github/dependabot.yml`, lockfiles) → `false`. - **`scripts/aggregate-hygiene-results.sh --self-test` → `Hygiene result aggregation contract passed.`** The aggregator is unchanged and still fails closed on a named empty/`failure`/non-`success` outcome. - **`actionlint .github/workflows/ci.yml` → clean.** Validates the five added gate conditions and the five ternary expressions in `CHECK_RESULTS`. - **`shellcheck --rcfile .shellcheckrc` and `shfmt -d`** on the edited/reused scripts → clean, no diff. Honest scope of what this PR's own CI proves: this PR touches `.github/` and `scripts/`, so on it `docs_only=false` and the full hygiene suite runs — a live proof only of the run-path (nothing is wrongly skipped on a code diff). The skip-path is not live-observable here by construction; it is proven by the detector unit test (`docs/topics/`-only → `true`) plus the gate/expression logic being the exact inverse of each other. Correctness of the intentional-skip → `success` mapping rests on that unit test and the expression, not on a fabricated observation. **Versioning/changelog:** none. No `plugins/<name>/` directory is touched — this is a repo-root CI-only change — so per the precedent #622 followed (matching CI-only #490/#593) there is no plugin version bump or CHANGELOG entry. Closes #628 ## Related - #628 — this issue. - #617 (closed) / #622 (merged) — the established precedent this extends: same `scripts/check-docs-only.sh` detector, same self-test-first / fail-closed / inner-step-gated discipline, applied to `hygiene` instead of `plugin-gate`/`miro-plugin`. - #532 — the false-green / liveness class this must not reintroduce. A job-level skip (or teaching the aggregator to pass on `skipped`) is exactly that shape and is deliberately avoided: the job always runs, the detector self-tests first, and only inner steps are gated with their skip reported as an honest not-applicable success. 🤖 Generated with a Claude Code implementation subagent (issue #628) --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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
1 participant
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.
Closes #462
Summary
PRs fail the
pr-issue-linkageCI check because no.github/pull_request_template.mdexisted to scaffold the## Relatedsection and native closing keyword the check requires. Authors discovered the contract only after a red CI run. This adds the template so web/editor authors scaffold a passing body up front, and documents the contract in one place.Fix
Add
.github/pull_request_template.mdscaffolding, in order, aCloses #closing-keyword placeholder plus## Summary/## Fix/## Verification/## Related. Section-level guidance lives in HTML comments. The reusable validator (melodic-software/ci-workflowspr-issue-linkage.yml, pinned at90f1c54) is the single source of truth — the template only mirrors the exact## Relatedheading and the closing-keyword contract it enforces; no validator logic is duplicated.Scope note on accuracy: this does not retroactively fix the agent-authored PRs that failed this check. Those used
gh pr create --body "...", which fully overrides.github/pull_request_template.md(cli/cli#10751), so templates never applied to them. This template helps human/web/editor authors and documents the contract; the agent path is covered separately by front-loading the contract into worker briefs, and the/source-control:pull-requestcreate-flow half is tracked in #487.By design, an unedited template does not pass the check: the validator strips HTML comments before validating (its explicit anti-"vacuous pass" guard), so the placeholders must be filled with real linkage before a PR goes green.
Verification
Repo-pinned gates run against the new file — all clean:
markdownlint-cli2v0.23.1 (config.markdownlint-cli2.jsonc, schema pinned v0.23.0):Summary: 0 issues.editorconfig-checkerv3.8.0 (config.editorconfig-checker.json): exit 0, no output.typosv1.44.0 (config_typos.toml): exit 0, no output.Mirror against the validator's own regex (copied verbatim from the pinned reusable and run locally):
["## Related" empty, "Missing closing keyword"]— correctly fails-until-filled (comments stripped, no#Ndigit), matching the check's design.Closes #462+## Relatedbullets) →[]— clean pass./^##\s+related$/imatches## Related; rejects### Relatedand## Related Issues.This PR body itself demonstrates a passing body: it carries
Closes #462and a non-empty## Relatedsection.Related
## Related) — no template scaffolds the required sections #462/source-control:pull-requestcreate flow emits the skeleton): source-control:pull-request: emit ## Related + closing-keyword skeleton in create flow #487🤖 Generated with Claude Code