skills/zernie__vigiles
1
0
Fork 0
mirror of https://github.com/zernie/vigiles.git synced 2026-09-14 20:53:57 +08:00
Code Issues Packages Projects Releases Wiki Activity

ci: replace the hand-rolled path classifier with dorny/paths-filter (#226)

Browse Source 
The `changes` job was a shell script: one API call for the changed-file
list, two `grep -qvE`, and a hand-written fallback for every way that list
can come back unusable. dorny/paths-filter already does all of it.

predicate-quantifier: some-with-excludes is load-bearing — under the
default `some`, a file under site/ matches `'**'` and sets root=true for a
site-only diff, which is the exact hole the filter exists to prevent.

The test now reads the filters with a YAML parser and evaluates them with
the action's own matcher library (picomatch), and asserts the pin and the
predicate-quantifier separately.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
zernie
2026-09-10 05:18:08 +05:00
committed by GitHub
parent 236615e978
commit eabf05732c
5 changed files with 198 additions and 95 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.github/workflows/ci.yml
+68 -58
View File
@@ -15,15 +15,37 @@ jobs:
# ~15 minutes each, because the browser install wedged.
#
# A workflow-level `paths:` cannot express this — it decides whether to run the
# workflow AT ALL and says nothing about individual jobs. Hence this job: one API
# call, no checkout, ~10s, and an `if:` on the jobs whose inputs are disjoint from
# what changed.
# workflow AT ALL and says nothing about individual jobs. Hence this job, plus an
# `if:` on the jobs whose inputs are disjoint from what changed.
#
# 🔴 THE RULE INSIDE: any doubt runs everything. An empty list, a failed call, a
# push whose `before` is all zeros (a new branch), a manual dispatch — all fall
# through to true. A SKIPPED job and a PASSED job look identical in the checks
# list, so the cost of a wrong skip is a green tick over work nobody did, while
# the cost of a wrong run is a few minutes.
# 🔴 THE RULE INSIDE: any doubt runs everything. A SKIPPED job and a PASSED job
# look identical in the checks list, so the cost of a wrong skip is a green tick
# over work nobody did, while the cost of a wrong run is a few minutes. The action
# errs the same way by construction — "all files are considered as added if there
# is no common ancestor with base branch or no previous commit", i.e. a new branch
# or a shallow history runs everything.
#
# ── WHY AN ACTION AND NOT OUR OWN `grep` (2026-09-09) ────────────────────────
# This was a hand-written shell classifier: one `gh api` call for the file list,
# then two `grep -qvE`, then a fallback for every way that list can come back
# unusable (empty, failed call, a push whose `before` is all zeros). Every one of
# those branches is a thing `dorny/paths-filter` already does, and does more
# carefully — it is the standard action for exactly this, and the edge cases were
# the half we wrote by hand.
#
# THE COST, stated because it is real: on a `push` the action diffs with git and
# therefore needs a checkout, while the old classifier needed none and finished in
# ~3s. On a `pull_request` it reads the REST API and still needs nothing. Push
# here only ever means a merge into main, so the checkout is paid once per merge
# and the job still rounds to the same single billable minute.
#
# 🔴 `predicate-quantifier: some-with-excludes` IS LOAD-BEARING, NOT DECORATION.
# Under the default `some`, patterns are OR-ed (`patterns.some(...)` in the
# action's own filter.ts), so `['**', '!site/**']` would match a file under site/
# via `**` and set root=true for a site-only diff — the exact hole this filter
# exists to prevent, silently. `some-with-excludes` makes a negated pattern a
# final exclusion: "included by >=1 pattern AND excluded by 0". Pinned by
# src/ci-path-filter.test.ts, which fails if the setting is missing.
changes:
runs-on: ubuntu-latest
timeout-minutes: 5
@@ -31,58 +53,46 @@ jobs:
root: ${{ steps.f.outputs.root }}
site: ${{ steps.f.outputs.site }}
steps:
- id: f
env:
GH_TOKEN: ${{ github.token }}
run: |
set -u
files=""
case "${{ github.event_name }}" in
pull_request)
files=$(gh api --paginate "repos/${{ github.repository }}/pulls/${{ github.event.number }}/files" \
--jq '.[].filename' 2>/dev/null || true) ;;
push)
before="${{ github.event.before }}"
if [ -n "$before" ] && [ "$before" != "0000000000000000000000000000000000000000" ]; then
files=$(gh api "repos/${{ github.repository }}/compare/$before...${{ github.sha }}" \
--jq '.files[].filename' 2>/dev/null || true)
fi ;;
esac
if [ -z "$files" ]; then
echo "root=true" >> "$GITHUB_OUTPUT"
echo "site=true" >> "$GITHUB_OUTPUT"
echo "no usable file list for ${{ github.event_name }} — running everything"
exit 0
fi
echo "$files" | sed 's/^/ /'
# `push` ONLY — see the cost note above. Default depth is fine: the action
# fetches what it needs on top of it (`initial-fetch-depth`, default 100).
- uses: actions/checkout@v4
if: github.event_name == 'push'
# `root` — everything except the site. False ONLY when every changed file
# is under site/, because nothing outside site/ reads anything inside it.
#
# 🔴 THAT SENTENCE WAS FALSE ONCE, AND IT COST A RED MAIN.
# src/core/linter-contract.test.ts read two files under site/ off disk;
# #219 was a site-only PR that deleted one, so these jobs were skipped,
# the PR merged GREEN and main broke on an ENOENT — a skipped job and a
# passing job are the same tick in the checks list, so nothing said so.
# The read is gone (that test moved to the site suite, where it belongs),
# and src/ci-path-filter.test.ts now FAILS if any root test reads site/
# again. The invariant above is checked, not asserted.
if echo "$files" | grep -qvE '^site/'; then root=true; else root=false; fi
- uses: dorny/paths-filter@v4.0.3
id: f
with:
predicate-quantifier: some-with-excludes
filters: |
# `root` — everything except the site. False ONLY when every changed
# file is under site/, because nothing outside site/ depends on
# anything inside it.
#
# 🔴 THAT SENTENCE WAS FALSE ONCE, AND IT COST A RED MAIN. Two root
# tests read files under site/ off disk; #219 was a site-only PR that
# deleted one, so these jobs were skipped, the PR merged GREEN and main
# broke on an ENOENT. Both reads are gone (#223 — one test moved to the
# site suite, one snapshot moved to tools/measured/ so the site imports
# it instead), and src/ci-path-filter.test.ts now FAILS if anything the
# root suite runs depends on site/ again. The invariant is checked, not
# asserted.
root:
- '**'
- '!site/**'
# `site` — the demo. It imports the built engine through three aliases
# (@engine/scan-files, @engine/audit-report, @engine/spec), so any src/
# change can reach it and must run it. What CANNOT reach it: prose and
# agent config. Verified 2026-08-19 by grepping the three bundles the site
# actually loads — the lint rule table (`DEFAULT_RULES`, `frontmatter-valid`)
# appears in none of them, so a rules change is invisible to the demo.
# Its check pages are generated from site/src/checks/checks.ts, NOT from
# docs/, so docs cannot reach it through the page generator either.
if echo "$files" | grep -qvE '^(docs/|\.claude/|[^/]*\.md$)'; then site=true; else site=false; fi
echo "root=$root" >> "$GITHUB_OUTPUT"
echo "site=$site" >> "$GITHUB_OUTPUT"
[ "$root" = true ] || echo "site-only change — the root jobs are skipped"
[ "$site" = true ] || echo "prose/agent-config only — the site job is skipped"
# `site` — the demo. It imports the built engine through three aliases
# (@engine/scan-files, @engine/audit-report, @engine/spec), so any src/
# change can reach it and must run it. What CANNOT reach it: prose and
# agent config. Verified 2026-08-19 by grepping the three bundles the
# site actually loads — the lint rule table (`DEFAULT_RULES`,
# `frontmatter-valid`) appears in none of them, so a rules change is
# invisible to the demo. Its check pages are generated from
# site/src/checks/checks.ts, NOT from docs/, so docs cannot reach it
# through the page generator either.
site:
- '**'
- '!docs/**'
- '!.claude/**'
- '!*.md'
test:
needs: changes
package-lock.json
Generated
+9
View File
@@ -41,6 +41,7 @@
"@types/markdown-it": "^14.1.2",
"@types/minimatch": "^5.1.2",
"@types/node": "^20.19.39",
"@types/picomatch": "^4.0.2",
"@typescript-eslint/eslint-plugin": "^8.58.0",
"@typescript-eslint/parser": "^8.58.0",
"@vitest/coverage-v8": "^4.1.8",
@@ -51,6 +52,7 @@
"eslint-plugin-sonarjs": "^4.0.2",
"globals": "^17.4.0",
"jest": "^30.4.2",
"picomatch": "^4.0.4",
"prettier": "^3.8.1",
"tsx": "^4.21.0",
"typedoc": "^0.28.19",
@@ -4451,6 +4453,13 @@
"dev": true,
"license": "MIT"
},
"node_modules/@types/picomatch": {
"version": "4.0.3",
"resolved": "https://registry.npmjs.org/@types/picomatch/-/picomatch-4.0.3.tgz",
"integrity": "sha512-iG0T6+nYJ9FAPmx9SsUlnwcq1ZVRuCXcVEvWnntoPlrOpwtSTKNDC9uVAxTsC3PUvJ+99n4RpAcNgBbHX3JSnQ==",
"dev": true,
"license": "MIT"
},
"node_modules/@types/prismjs": {
"version": "1.26.6",
"resolved": "https://registry.npmjs.org/@types/prismjs/-/prismjs-1.26.6.tgz",
package.json
+2
View File
@@ -103,6 +103,7 @@
"@types/markdown-it": "^14.1.2",
"@types/minimatch": "^5.1.2",
"@types/node": "^20.19.39",
"@types/picomatch": "^4.0.2",
"@typescript-eslint/eslint-plugin": "^8.58.0",
"@typescript-eslint/parser": "^8.58.0",
"@vitest/coverage-v8": "^4.1.8",
@@ -113,6 +114,7 @@
"eslint-plugin-sonarjs": "^4.0.2",
"globals": "^17.4.0",
"jest": "^30.4.2",
"picomatch": "^4.0.4",
"prettier": "^3.8.1",
"tsx": "^4.21.0",
"typedoc": "^0.28.19",
src/ci-path-filter.test.ts
+111 -36
View File
@@ -1,5 +1,5 @@
/**
* The `changes` job's path classifier — tested against the REAL patterns in ci.yml.
* The `changes` job's path classifier — tested against the REAL filters in ci.yml.
*
* ── WHY THIS HAS A TEST AT ALL ──────────────────────────────────────────────────
* A job that is SKIPPED and a job that PASSED render identically in the checks
@@ -8,9 +8,22 @@
* is to already suspect it. That is the same failure mode as an advisory hook whose
* success state is silence, and it gets the same treatment: assert both directions.
*
* The patterns are EXTRACTED FROM THE WORKFLOW rather than restated here. A copy
* The filters are EXTRACTED FROM THE WORKFLOW rather than restated here. A copy
* would drift, and a test that agrees with its own copy of the rule proves nothing
* about the rule that runs.
*
* ── WHAT CHANGED 2026-09-09 ─────────────────────────────────────────────────────
* The classifier was a hand-written shell script; it is now `dorny/paths-filter`.
* So this file can no longer re-run the rule by shelling out to `grep` with the
* workflow's own pattern. It does the nearest honest thing instead: it reads the
* filters with a YAML parser and evaluates them with the SAME matcher library the
* action uses (picomatch), through a transcription of the action's own predicate.
*
* That transcription is the one copied thing here, and its risk is named: if the
* action changes how it combines patterns, this file agrees with the old rule. Two
* things bound that risk — the action version is PINNED (`@v4.0.3`, not a floating
* major), and the setting the predicate depends on is asserted separately below,
* because it is the one whose absence inverts the result in silence.
*/
import { describe, it, expect } from "vitest";
import {
@@ -22,43 +35,99 @@ import {
} from "node:fs";
import { tmpdir } from "node:os";
import { createRequire } from "node:module";
import yaml from "js-yaml";
import picomatch from "picomatch";
import ts from "typescript";
import { extname, join, resolve } from "node:path";
import { pathToFileURL } from "node:url";
import { execFileSync } from "node:child_process";
const CI = resolve(__dirname, "..", ".github", "workflows", "ci.yml");
/** Pull the ERE out of `if echo "$files" | grep -qvE '<pattern>'; then <name>=true`. */
function patternFor(flag: "root" | "site"): string {
const yml = readFileSync(CI, "utf8");
const re = new RegExp(`grep -qvE '([^']+)'; then ${flag}=true`);
const m = re.exec(yml);
if (m === null)
throw new Error(
`no grep line for \`${flag}\` in ci.yml — the classifier was renamed or ` +
`restructured, and this test can no longer see the rule it is asserting`,
);
// A YAML block scalar is literal, so the pattern reaches grep exactly as written
// here — no unescaping step, and none is wanted: adding one would silently
// rewrite the rule before asserting on it.
return m[1];
interface Step {
uses?: string;
with?: Record<string, string>;
}
/** Re-run the workflow's own decision: `grep -qvE` succeeds ⇒ the flag is true. */
function decide(flag: "root" | "site", files: readonly string[]): boolean {
try {
execFileSync("grep", ["-qvE", patternFor(flag)], {
// Faithful to the shell: an empty list is an empty stream, not a blank line.
input: files.length > 0 ? files.join("\n") + "\n" : "",
stdio: ["pipe", "ignore", "ignore"],
});
return true;
} catch {
return false;
}
/** The `dorny/paths-filter` step the `changes` job actually declares. */
function filterStep(): { version: string; with: Record<string, string> } {
const wf = yaml.load(readFileSync(CI, "utf8")) as {
jobs?: { changes?: { steps?: Step[] } };
};
const step = (wf.jobs?.changes?.steps ?? []).find((x) =>
(x.uses ?? "").startsWith("dorny/paths-filter@"),
);
if (step?.uses === undefined || step.with === undefined)
throw new Error(
"no `dorny/paths-filter` step with a `with:` block in the `changes` job — " +
"the classifier was replaced, and this test can no longer see the rule " +
"it is asserting",
);
return { version: step.uses.split("@")[1], with: step.with };
}
/** The filters, parsed out of the step's YAML block scalar. */
function filters(): Record<string, string[]> {
return yaml.load(filterStep().with["filters"] ?? "") as Record<
string,
string[]
>;
}
/**
* The action's OWN `some-with-excludes` predicate, transcribed from
* `paths-filter/src/filter.ts` @v4.0.3, including its picomatch options:
*
* const MatchOptions = { dot: true }
* const includes = matchers.filter(m => !m.state.negated)
* const excludes = matchers.filter(m => m.state.negated)
* isExclude = str => excludes.some(m => !m(str)) // un-invert picomatch
* // included by >=1 pattern AND excluded by 0
*
* `dot: true` is not a detail: without it `**` would not match `.claude/…`, and an
* agent-config-only diff would set root=false and skip the root jobs in silence.
*/
function fileMatches(file: string, patterns: readonly string[]): boolean {
const matchers = patterns.map((p) => picomatch(p, { dot: true }, true));
const excluded = matchers
.filter((m) => m.state.negated)
.some((m) => !m(file));
if (excluded) return false;
return matchers.filter((m) => !m.state.negated).some((m) => m(file));
}
/** A filter is true when ANY changed file matches it. */
function decide(flag: "root" | "site", files: readonly string[]): boolean {
const patterns = filters()[flag];
if (patterns === undefined)
throw new Error(`the \`changes\` job declares no \`${flag}\` filter`);
return files.some((f) => fileMatches(f, patterns));
}
describe("the changes job is wired to the action, not to a hand-rolled rule", () => {
it("pins the action to an exact version, not a floating major", () => {
// The transcribed predicate above is only safe against a pinned version: a
// floating `@v4` could change how patterns combine and leave this file
// agreeing with a rule that no longer runs.
expect(filterStep().version).toMatch(/^v\d+\.\d+\.\d+$/);
});
it("declares predicate-quantifier: some-with-excludes", () => {
// 🔴 THE SETTING WHOSE ABSENCE INVERTS THE RESULT, SILENTLY. The default is
// `some`, i.e. `patterns.some(...)` — under it a file under site/ matches the
// `'**'` pattern and sets root=true for a site-only diff, which is the exact
// hole the filter exists to prevent. Asserted separately from the behaviour
// below because the behaviour is checked through a transcription of the
// predicate this setting selects, so it cannot catch its own absence.
expect(filterStep().with["predicate-quantifier"]).toBe(
"some-with-excludes",
);
});
it("declares both flags the dependent jobs read", () => {
expect(Object.keys(filters()).sort()).toEqual(["root", "site"]);
});
});
// The actual diff of PR #167, the change that exposed the missing filter.
const PR167 = [
".github/workflows/ci.yml",
@@ -115,15 +184,21 @@ describe("the changes job classifies a diff", () => {
expect(decide("site", ["src/fixtures/CLAUDE.md"])).toBe(true);
});
it("an empty diff is not a licence to skip", () => {
// The workflow bails to true before reaching grep when the list is empty; this
// pins the reason rather than the branch — grep -qv over nothing finds no
// non-matching line, so the pattern alone would say `false` for BOTH flags.
it("an empty diff matches nothing — and that is not the fallback", () => {
// With no changed files there is nothing to match, so both flags are false.
// That is trivially right: a run with an empty diff has nothing to check.
//
// ⚠️ WHAT THIS DOES NOT COVER, stated so nobody reads it as the safety net.
// The dangerous case is not "no files" but "the diff could not be
// DETERMINED", and that case now belongs to the action: it documents that
// "all files are considered as added if there is no common ancestor with base
// branch or no previous commit", i.e. a new branch or a shallow history runs
// EVERYTHING. The hand-rolled classifier had to spell that fallback out in
// shell (empty list · failed `gh api` · a push whose `before` is all zeros);
// dropping those branches is most of why the action is worth adopting, and it
// is also why this file can no longer assert them — they are not ours.
expect(decide("root", [])).toBe(false);
expect(decide("site", [])).toBe(false);
const yml = readFileSync(CI, "utf8");
expect(yml).toMatch(/if \[ -z "\$files" \]; then\n\s+echo "root=true"/);
expect(yml).toMatch(/running everything/);
});
});
src/hook-runtime.ts
+8 -1
View File
@@ -522,12 +522,19 @@ export async function runHookProgramCommand(
// Everything else stays BLOCKED, and the escapes are whitelists of commands
// that are WRITES — see `isLoadPathRepairEvent` for why no command is one.
const conflicted = conflictedLoadPathFiles(file);
// 🔴 THE THROWN MESSAGE IS THE ONLY THING THAT NAMES THE REAL CAUSE when the
// merge-conflict heuristic above does not fire. Without it this said just
// "cannot be loaded" — a diagnosis that sends the reader looking in the wrong
// place, which is the defect this runtime has already shipped twice (the
// loader that advised `npm run build` when the answer was `npm install`).
// The comment above promises to name the cause; this is what keeps it.
const thrown = err instanceof Error ? err.message : String(err);
const cause =
conflicted.length > 0
? `cannot be loaded — ${conflicted.join(", ")} contains merge-conflict ` +
`markers, so Node cannot resolve \`vigiles/hook\` from it (the hook itself ` +
`may be fine)`
: "cannot be loaded";
: `cannot be loaded — ${thrown}`;
if (
isLoadPathRepairEvent(event, file, {
// The root the REST of this runtime already uses: `hookStampPath` and