diff --git a/.agents/agents/review-guardian.md b/.agents/agents/review-guardian.md index 5a107513b..dfca28e40 100644 --- a/.agents/agents/review-guardian.md +++ b/.agents/agents/review-guardian.md @@ -25,6 +25,14 @@ and generated-output drift before human review time is spent. - Treat packaged skills, project agents, workflow wrappers, local actions, changelog entries, wiki output, and generated reports as first-class review surfaces when touched. +- Require explicit validation evidence for workflow, local-action, and + packaged-wrapper changes. Prefer deterministic local harnesses, fake + ``gh``/``git`` scripts, temporary repositories, ``actionlint`` for workflows, + shell/YAML checks, or a temporary validation PR when GitHub-only behavior + cannot be exercised locally. +- When a workflow pushes commits with ``GITHUB_TOKEN``, verify that required + checks are dispatched or mirrored for the bot-authored commit before treating + the workflow as safe. - Treat ``.github/wiki`` pointer changes as workflow-managed state when they line up with wiki preview or wiki maintenance automation, rather than as automatic evidence of accidental scope creep. @@ -37,6 +45,8 @@ and generated-output drift before human review time is spent. - A maintainer wants a fresh rigorous review pass on an existing pull request. - A change touches workflows, generated outputs, synchronized assets, or other surfaces where a generic summary would miss important risk. +- A pull request changes workflow permissions, triggers, local action scripts, + packaged workflow wrappers, or bot-authored commit behavior. ## Boundaries diff --git a/.agents/skills/pull-request-review/SKILL.md b/.agents/skills/pull-request-review/SKILL.md index d17021d4a..fcc816d6f 100644 --- a/.agents/skills/pull-request-review/SKILL.md +++ b/.agents/skills/pull-request-review/SKILL.md @@ -17,12 +17,16 @@ possible. 2. Inspect behavior before style. Start with source, workflows, generated outputs, synchronized assets, tests, and documentation that can change release or automation behavior. -3. Produce the review using the findings-first contract. Read +3. When the pull request changes workflows, local actions, or packaged workflow + wrappers, build an executable validation strategy before declaring the + surface clean. Read + [references/workflow-action-validation.md](references/workflow-action-validation.md). +4. Produce the review using the findings-first contract. Read [references/review-contract.md](references/review-contract.md). -4. Call out missing or weak tests, missing docs, changelog gaps, generated +5. Call out missing or weak tests, missing docs, changelog gaps, generated artifact drift, and consumer-sync impacts whenever the changed surfaces make them relevant. -5. Only after the findings, add a brief summary or note residual risk. If no +6. Only after the findings, add a brief summary or note residual risk. If no issues are found, say that clearly and mention any remaining verification gaps. @@ -38,6 +42,13 @@ possible. - When ``.github/wiki`` moves, verify whether the wiki preview or wiki maintenance workflow is expected to refresh the submodule pointer before treating that change as unrelated drift or scope creep. +- When workflow automation pushes commits with ``GITHUB_TOKEN``, verify that + required checks are dispatched or mirrored for the bot-authored commit before + treating the workflow as safe. +- For workflow, local-action, or packaged-wrapper changes, prefer deterministic + validation such as shell/YAML checks, fake ``gh``/``git`` harnesses, + temporary repositories, or a temporary validation PR when local simulation + cannot cover the behavior. - Prefer precise repository file references in every finding. - Review what changed, but reason about downstream consumer impact when the PR touches packaged assets or synchronized defaults. @@ -49,6 +60,7 @@ possible. | Need | Reference | |------|-----------| | Decide which changed surfaces deserve the closest scrutiny | [references/surface-priorities.md](references/surface-priorities.md) | +| Validate workflow, local-action, and packaged-wrapper changes | [references/workflow-action-validation.md](references/workflow-action-validation.md) | | Format the review output in the expected findings-first shape | [references/review-contract.md](references/review-contract.md) | ## Anti-patterns @@ -58,5 +70,8 @@ possible. behavioral or workflow risk exists. - Do not ignore generated files, synced assets, or workflow wrappers when the pull request touched their sources. +- Do not mark workflow or action changes clean without an explicit validation + strategy and evidence, or a clear residual-risk note when behavior cannot be + exercised before merge. - Do not claim a pull request is clean without mentioning the verification scope or any residual gaps. diff --git a/.agents/skills/pull-request-review/references/review-contract.md b/.agents/skills/pull-request-review/references/review-contract.md index 7d5633a8f..164e81cf9 100644 --- a/.agents/skills/pull-request-review/references/review-contract.md +++ b/.agents/skills/pull-request-review/references/review-contract.md @@ -29,9 +29,13 @@ Required review themes: - Generated-output drift. - Workflow, CI, or release automation risk. - Consumer-sync or packaged-asset side effects. +- Workflow/action validation evidence for changes under ``.github/workflows``, + ``.github/actions``, or ``resources/github-actions``. If no issues are found: - State that clearly. - Mention the scope that was reviewed. -- Call out any remaining test or verification gaps. +- Call out any remaining test or verification gaps, including workflow + behavior that could not be exercised locally or through a temporary + validation run. diff --git a/.agents/skills/pull-request-review/references/surface-priorities.md b/.agents/skills/pull-request-review/references/surface-priorities.md index 76683ddfc..f950762ce 100644 --- a/.agents/skills/pull-request-review/references/surface-priorities.md +++ b/.agents/skills/pull-request-review/references/surface-priorities.md @@ -33,6 +33,12 @@ Typical review questions - Did the implementation change behavior without matching tests? - Did the workflow wrapper stay aligned with the reusable workflow? +- Did workflow or action changes include executable validation evidence, not + only YAML review? +- If a workflow pushes bot-authored commits, does it dispatch or mirror + required checks for the new commit? +- Are permissions, local action checkouts, and event-specific inputs valid for + reusable workflows and packaged consumer wrappers? - Did packaged assets change without explaining the consumer impact? - Did docs, README, changelog, or generated outputs remain consistent with the implementation? diff --git a/.agents/skills/pull-request-review/references/workflow-action-validation.md b/.agents/skills/pull-request-review/references/workflow-action-validation.md new file mode 100644 index 000000000..833402de7 --- /dev/null +++ b/.agents/skills/pull-request-review/references/workflow-action-validation.md @@ -0,0 +1,112 @@ +Workflow And Action Validation +============================== + +Workflow, local-action, and packaged-wrapper changes MUST receive an explicit +validation strategy during review. These surfaces often fail only after merge, +after a workflow-dispatched run, or after a bot-authored commit exercises a +path that normal pull-request CI does not trigger. + +When To Apply +------------- + +Use this checklist whenever a pull request touches any of these surfaces: + +- ``.github/workflows/**`` +- ``.github/actions/**`` +- ``resources/github-actions/**`` +- workflow-related docs that describe permissions, dispatching, branch + protection, sync behavior, generated previews, or release automation + +Required Questions +------------------ + +Ask these questions before deciding the workflow surface is safe: + +- Does the workflow or action push commits with ``GITHUB_TOKEN``? +- If it pushes bot-authored commits, do required checks run, get dispatched, or + get mirrored for the new commit? +- Are the required permissions declared in both the reusable workflow and any + packaged consumer wrapper? +- Are local composite action paths available from the repository and ref used + by the workflow? +- Does the changed automation behave correctly for same-repository pull + requests, forked pull requests, ``push``, ``workflow_dispatch``, and + ``workflow_call`` where those events are supported? +- Does the workflow rely on files from ``main`` that might not exist in a + consumer repository or in the installed DevTools package version? +- Does the workflow update generated state such as ``.github/wiki``, Pages + previews, release notes, changelog entries, labels, or project metadata? +- Does the workflow need queueing, cancellation, or required-check mirroring so + branch protection sees the latest commit state? + +Validation Strategies +--------------------- + +Prefer deterministic local validation when it can cover the changed behavior: + +- run ``bash -n`` for shell scripts; +- parse changed YAML files or run ``actionlint`` after ensuring it is available; +- use fake ``gh`` and ``git`` wrappers to exercise action scripts without + calling GitHub; +- create temporary Git repositories to validate merge, rebase, conflict, + branch, tag, or submodule behavior; +- run changed PHP helper scripts directly against synthetic fixtures; +- verify packaged wrapper inputs, permissions, and reusable workflow paths stay + aligned with the canonical workflow. + +Actionlint Availability +----------------------- + +When workflow files are changed, prefer running ``actionlint`` locally. If it is +missing, install it before review when the platform has a safe package manager: + +- macOS with Homebrew: ``brew install actionlint``; +- macOS with MacPorts: ``sudo port install actionlint``; +- Debian-based Linux: download the official ``actionlint`` release archive for + the host architecture, unpack the binary into a temporary directory, and run + it from there or install it into a user-local ``PATH`` such as + ``~/.local/bin``. + +``actionlint`` is distributed as a Go binary. It may use ``shellcheck`` for +``run:`` blocks, and Homebrew installs that dependency automatically. On +Windows, prefer WSL for now unless the repository already documents a native +Windows installation path. + +Use the broadest relevant command for the changed surfaces, for example: + +.. code-block:: bash + + actionlint .github/workflows/*.yml resources/github-actions/*.yml + +Record the command and result in the review evidence. If installation is not +possible, record that as a residual validation gap instead of silently skipping +workflow linting. + +When local simulation cannot cover the behavior, call out the gap and prefer a +temporary validation branch or pull request. Close that validation PR after +recording the evidence in the real PR or issue. Do not require noisy temporary +PRs for every workflow change when a deterministic local harness covers the +risk. + +Bot-Authored Commit Rule +------------------------ + +Any workflow that pushes commits with ``GITHUB_TOKEN`` MUST be reviewed for +required-check side effects. GitHub usually does not trigger another normal +``pull_request`` workflow run for commits pushed by the built-in workflow +token. If branch protection requires checks on the latest commit, the workflow +SHOULD dispatch the required workflow or publish matching commit statuses for +that bot-authored commit. + +Review Output Expectations +-------------------------- + +The review MUST record one of the following: + +- validation evidence, including commands, harnesses, or GitHub runs used; +- a concrete finding when validation reveals a bug or missing permission; +- a residual-risk note when the behavior cannot be exercised before merge. + +If the pull request body already contains sufficient validation evidence, +reference it and verify that it covers the changed automation path. If the body +does not, call out the missing validation in the review. diff --git a/.github/actions/review/render-request/run.cjs b/.github/actions/review/render-request/run.cjs index a4b42522b..918168594 100644 --- a/.github/actions/review/render-request/run.cjs +++ b/.github/actions/review/render-request/run.cjs @@ -15,11 +15,11 @@ function classifyTouchedSurfaces(files) { (file) => file === 'README.md' || file.startsWith('docs/'), ], [ - 'Workflow or local GitHub Action logic changed; scrutinize permissions, triggers, release flow, and CI side effects.', + 'Workflow or local GitHub Action logic changed; require executable validation of permissions, triggers, bot-authored commits, and CI side effects.', (file) => file.startsWith('.github/workflows/') || file.startsWith('.github/actions/'), ], [ - 'Consumer workflow wrappers changed; verify they still mirror the reusable workflow contract safely.', + 'Consumer workflow wrappers changed; verify permissions, inputs, and reusable workflow refs remain aligned with the packaged contract.', (file) => file.startsWith('resources/github-actions/'), ], [ @@ -80,6 +80,7 @@ function renderComment({pull, files, focusAreas}) { `Use $pull-request-review with the review-guardian agent to review PR #${pull.number} (${pull.html_url}).`, 'Lead with findings ordered by severity. Include repository file references whenever possible.', 'Prioritize bugs, regressions, missing tests, missing docs, generated-output drift, and workflow or CI impacts.', + 'For workflow/action changes, record validation evidence or residual risk; pay special attention to GITHUB_TOKEN pushes and required-check dispatch/mirroring.', `Base: ${pull.base.ref}`, `Head: ${pull.head.ref} @ ${pull.head.sha.slice(0, 7)}`, '```', @@ -99,6 +100,7 @@ function renderSummary({pull, focusAreas}) { '### Findings-first expectations', '- Lead with bugs, regressions, missing tests, missing docs, generated-output drift, and workflow or CI risk.', '- Include repository file references whenever possible.', + '- For workflow/action changes, record executable validation evidence or residual risk, especially around bot-authored commits and required checks.', ]; if (focusAreas.length > 0) { diff --git a/.github/wiki b/.github/wiki index 6523430c4..716f76b9c 160000 --- a/.github/wiki +++ b/.github/wiki @@ -1 +1 @@ -Subproject commit 6523430c4fcc16b4b940efb31fd136ff239ace30 +Subproject commit 716f76b9c9d85660a93789b2986af20e1ef3af6d diff --git a/CHANGELOG.md b/CHANGELOG.md index 492fbfac5..64a2bbee7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - Auto-resolve pull-request conflicts limited to workflow-managed `.github/wiki` pointers and `CHANGELOG.md` `Unreleased` drift (#192) +- Teach the pull-request review skill, review-guardian agent, and review request brief to require explicit validation strategies for workflow, local-action, and packaged-wrapper changes, including local `actionlint` installation guidance (#241) ### Fixed