From 7973bf0d0b8ba0d907b5dd734f52e302524bbedf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= Date: Sat, 5 Sep 2026 16:47:55 +0200 Subject: [PATCH] :memo: Move knip + outdated to a maintain: prefix `npm run check` should be the fast, offline correctness ladder only (tsc + oxlint + oxfmt + cspell, ~3s), so an agent can run it as a confirmation gate during feature work. check:knip / check:outdated were advisory whole-project / network scans, and check-outdated exits non-zero whenever any dep is behind. Keeping them in check made `npm run check` (and the CI build gate) fail on dependency freshness, which must not block an unrelated feature PR. Rename them to maintain:knip / maintain:outdated, aggregate under `npm run maintain`, and run it in CI as a dedicated non-blocking job (continue-on-error) that surfaces findings without ever gating a merge. Update the script-prefix convention, the feedback-tier table, and AGENTS.md: the agent may now run `npm run check`; only `maintain` stays out of the feature loop. --- .github/workflows/ci.yml | 15 +++++++++++++++ AGENTS.md | 8 ++++++-- CONTRIBUTING.md | 29 ++++++++++++++++------------- README.md | 4 +++- package.json | 7 ++++--- 5 files changed, 44 insertions(+), 19 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8a36c84..1c9bdd0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -26,6 +26,21 @@ jobs: name: coverage path: coverage + # Advisory scans (dead code, dependency freshness). Non-blocking: surfaced on + # the PR for visibility, but must never gate a merge — so continue-on-error and + # intentionally NOT in `publish`'s `needs`. + maintain: + runs-on: ubuntu-latest + continue-on-error: true + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-node@v4 + with: + node-version-file: .node-version + cache: "npm" + - run: npm ci + - run: npm run maintain + publish: if: startsWith(github.ref, 'refs/tags/') needs: build diff --git a/AGENTS.md b/AGENTS.md index 9ddbfbc..a8e64f8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -10,12 +10,16 @@ first-action facts. Do not restate evolving prose here — it will drift. - Project: F#-style pattern matching for TypeScript/ESM. Node `>=26` (pinned via `.node-version`), ESM-only (no CommonJS shim). - **Mandatory while iterating:** `npm run test` (runs `check:tsc`, then the unit suite). This is the gate you are responsible for. - **On commit:** write a good message (see [CONTRIBUTING.md § Commit messages](./CONTRIBUTING.md#commit-messages)). Lefthook's pre-commit hook already runs the fast, offline, staged-file checks (`tsc` + `oxlint` + `oxfmt` + `cspell`) — don't run them by hand. If the hook fails on style, `npm run fix`, restage, recommit. -- **`npm run check` is NOT part of the feature loop.** It adds `knip` (dead-code/deps) and `check:outdated` (registry) — repo-maintenance scans. Run it only on an explicit maintenance / update-deps branch; CI runs it on the PR regardless. +- **Optional final gate:** `npm run check` (the fast, offline, whole-project correctness ladder: `tsc → oxlint → oxfmt → cspell`). Safe to run whenever you want a project-wide confirmation; pre-commit already covers staged files. +- **`npm run maintain` is NOT part of the feature loop.** `maintain:knip` (dead-code/deps) and `maintain:outdated` (registry) are advisory maintenance scans. Run them only on an explicit maintenance / update-deps branch; CI surfaces them via a non-blocking job, never as a gate. ```sh - npm run test # the bot's definition of done; commit normally after + npm run test # mandatory gate + npm run check # optional project-wide confirmation ``` + The bot's definition of done: `npm run test` green, commit normally after. + ## Never do Don't silence the type system to force a green run. As an agent these are forbidden: diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 08f41ff..804238c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -7,9 +7,9 @@ This document is for maintainers and contributors working on the project itself. CI and review will bounce these even though `npm run check` and the linters don't catch them. They're the high-frequency things a contributor (or an agent) reaches for by default: - **Source imports use `.ts` extensions, never `.js`.** `node --strip-types` resolves the `.ts` form at test time; `rewriteRelativeImportExtensions` emits `.js` in `dist/`. "Pre-fixing" an import to `.js` breaks the inner loop. (rationale: README § Tooling decisions) -- **A new `npm run` script must reuse an existing prefix** (`check:` / `fix:` / `test:` / `watch:` / `publish:`). If none fits, that's a signal the script doesn't belong in the pipeline — not a reason to invent a new prefix. (see [Script prefix convention](#script-prefix-convention)) +- **A new `npm run` script must reuse an existing prefix** (`check:` / `fix:` / `test:` / `watch:` / `maintain:` / `publish:`). If none fits, that's a signal the script doesn't belong in the pipeline — not a reason to invent a new prefix. (see [Script prefix convention](#script-prefix-convention)) - **`oxlint-disable` directives live in source, not `.oxlintrc.json`.** The trade-off must sit next to the code it silences. This is a _human_ last-resort convention; agents must not add these — see [AGENTS.md § Never do](./AGENTS.md#never-do). (rationale: README § Tooling decisions) -- **Don't put slow / network / whole-project checks in pre-commit.** `check:knip` (~4s) and `check:outdated` (~6.5s, network) are deliberately excluded from the hook to keep it fast and offline. (see [Why these splits](#why-these-splits)) +- **Don't put slow / network / whole-project scans in `check` or pre-commit.** `maintain:knip` (~4s, whole project) and `maintain:outdated` (~6.5s, registry) are advisory, not correctness — they live under the `maintain:` prefix and run in CI as a **non-blocking** job, never gating a merge. (see [Why these splits](#why-these-splits) and [Script prefix convention](#script-prefix-convention)) - **There is no local `npm run publish`, and `publish:publint` / `publish:attw` don't go in `check`.** They validate the publishable artifact (`dist/`) and run only in the CI `publish` job. (see [Publishing workflow](#publishing-workflow)) ## Commit messages @@ -26,6 +26,7 @@ Script names in `package.json` use a prefix that signals _when_ the script is in - `fix:*` — mutating counterpart of a `check:*` script. Aggregated by `npm run fix`. Use after `npm run check` to auto-resolve issues; the diff is the review surface. - `test:*` — test scripts. `npm run test` is the canonical entry point (`check:tsc` + unit tests); `test:unit` skips the typecheck for fast local iteration; `test:ci` adds c8 coverage and is the CI variant. - `watch:*` — long-running watchers, started manually via `npm run watch` for the inner dev loop. Sits "before" pre-commit in the feedback ladder (see Feedback tiers below). Currently a single child (`watch:test`); future `watch:oxlint` / `watch:tsc` would aggregate under the same `watch` umbrella. +- `maintain:*` — advisory repo-maintenance scans (dead code, dependency freshness): read-only, but whole-project and/or network-bound, so they are **not** correctness gates. Aggregated by `npm run maintain`. Run on an explicit maintenance / update-deps branch, and in CI as a non-blocking job (continue-on-error) that surfaces findings without ever failing a feature PR. - `publish:*` — runs only at publish time, in the CI `publish` job (immediately before `npm publish`). There is **no** local `npm run publish` script — publishing is CI-only by policy. The `publish:` prefix still documents intent: this script validates the _publishable artifact_ (e.g., `dist/`) rather than the source. A new script should pick the prefix that matches its lifecycle, not invent a new one. If no existing prefix fits, that's a signal the script doesn't belong in the standard pipeline. @@ -34,27 +35,29 @@ A new script should pick the prefix that matches its lifecycle, not invent a new The tools are organized into a feedback ladder. Each tier catches different things at different costs; the rule of thumb is "earlier tiers fire more often, faster tiers catch less, slower tiers are more thorough": -| Tier | When | What it runs | Time | -| ----------------- | ---------- | ----------------------------------------------------------- | ----- | -| `npm run watch` | manual | `watch:test` — re-runs tests on file save | ~0.1s | -| Pre-commit (auto) | on stage | tsc + oxlint + oxfmt + cspell (staged files only) | ~1.3s | -| Pre-push (auto) | on push | `npm test` (full tsc + unit tests) | ~3.5s | -| `npm run check` | manual | Full project audit: all `check:*` scripts + knip + outdated | ~10s | -| `npm run fix` | manual | Auto-resolve fixable issues (lint, format) | ~3s | -| CI build (auto) | on push/PR | `npm run check` + `npm run test:ci` | ~30s+ | -| CI publish (auto) | on tag | `publish:publint` + `publish:attw`, then `npm publish` | ~10s | +| Tier | When | What it runs | Time | +| -------------------------------- | ---------------------- | --------------------------------------------------------------------- | ----- | +| `npm run watch` | manual | `watch:test` — re-runs tests on file save | ~0.1s | +| Pre-commit (auto) | on stage | tsc + oxlint + oxfmt + cspell (staged files only) | ~1.3s | +| Pre-push (auto) | on push | `npm test` (full tsc + unit tests) | ~3.5s | +| `npm run check` | manual | Correctness gates: tsc + oxlint + oxfmt + cspell (whole project) | ~3s | +| `npm run fix` | manual | Auto-resolve fixable issues (lint, format) | ~3s | +| `npm run maintain` | manual / CI (advisory) | `maintain:knip` + `maintain:outdated` (whole-project + network scans) | ~10s | +| CI build (auto) | on push/PR | `npm run check` + `npm run test:ci` | ~30s+ | +| CI maintain (auto, non-blocking) | on push/PR | `npm run maintain` — reports, never fails the build | ~10s | +| CI publish (auto) | on tag | `publish:publint` + `publish:attw`, then `npm publish` | ~10s | ### Why these splits? - **`watch:*` is a manual tier, not a hook.** The developer starts it on demand (it has to be killed with Ctrl-C) and it runs in a dedicated terminal pane. It sits as the earliest tier in the feedback ladder, catching failures the moment a file is saved — before staging, before commit. The umbrella `watch` script is designed to aggregate multiple `watch:*` children (currently just `watch:test`); if more watchers are added later (e.g. `watch:oxlint`), the umbrella would switch to running them concurrently rather than sequentially. - **`check:tsc`, `check:oxlint`, `check:oxfmt`, `check:cspell`** are in pre-commit because they are fast (~0.2–0.5s each), fully offline, and naturally scope to staged files via the `LEFTHOOK_FILES` env var convention. They give instant feedback on what you typed. - **`test` (and the `tsc` it includes) is in pre-push** because it runs the whole test suite across the whole project. The pre-commit `LEFTHOOK_FILES` convention doesn't apply to the test runner, so pre-commit isn't the right home. Pre-push runs after all commits are made but before the push leaves the machine, catching regressions that span multiple commits. -- **`check:knip` and `check:outdated` are NOT in pre-commit** — knip scans the whole project (~4s, would noticeably slow the hook), and `check-outdated` queries the npm registry (~6.5s, network-dependent, advisory not correctness). Both run in `npm run check` and CI; pre-commit stays fast and offline. +- **`maintain:knip` and `maintain:outdated` are advisory, not correctness** — knip scans the whole project (~4s) and `check-outdated` queries the npm registry (~6.5s, network-dependent, and it exits non-zero whenever any dep is behind). That's why they live under `maintain:`, are excluded from pre-commit and from `check`, and run in CI as a **non-blocking** job: a stale dependency must never block an unrelated feature PR. - **`publish:publint` and `publish:attw` are NOT in `check`** — they belong to the `publish:` prefix because they validate the _publishable artifact_ (`dist/`) and require a fresh build. Running them on every commit would be wasteful. They run in the CI `publish` job immediately before `npm publish`. ### Before pushing -Run `npm run check` locally. If you only trust pre-commit + CI, know that anything `check:knip` or `check:outdated` would catch will be caught by CI on the PR before merge. +Run `npm run check && npm run test` locally — both are the fast, offline correctness gates, safe to run any time. Anything `maintain:knip` or `maintain:outdated` would catch is reported by the non-blocking CI `maintain` job; run `npm run maintain` yourself only on a maintenance / update-deps branch. ## Publishing workflow diff --git a/README.md b/README.md index 2efca56..1371614 100644 --- a/README.md +++ b/README.md @@ -8,6 +8,7 @@ Pattern matching for TypeScript/ESM environments (F#-style, not regex). - **Test:** `npm run test`, `npm run test:ci` - **Watch:** `npm run watch` (re-runs tests on file save; the earliest feedback tier) - **Checks:** `npm run check`, `npm run fix` +- **Maintenance (advisory):** `npm run maintain` — `knip` + `check-outdated`; run on a maintenance / update-deps branch, not part of the feature loop - **Individual fixes:** `npm run fix:oxfmt`, `npm run fix:oxlint` ### Tooling @@ -36,7 +37,7 @@ The choice and configuration of each tool above is the result of deliberate trad - **`attw --profile esm-only`** is semantically correct: this package is intentionally ESM-only (no CommonJS shim), so CJS resolution scenarios are out of scope by design, not a bug. - **The `publish:` prefix has no local aggregator.** `publint` and `attw` validate the _publishable artifact_ (`dist/`), not the source, and require a fresh build. They run only in the CI `publish` job immediately before `npm publish` — there is intentionally no `npm run publish`. - **`check:tsc` runs first** in the `npm run check` chain so a type error short-circuits the rest (faster feedback than letting oxlint/oxfmt run and then failing on tsc at the end). -- **`check:knip` and `check:outdated` are NOT in pre-commit** — knip scans the whole project (~4s, would noticeably slow the hook), and `check-outdated` queries the npm registry (~6.5s, network-dependent, advisory not correctness). Both run in `npm run check` and CI; pre-commit stays fast and offline. +- **The `check:` / `maintain:` split is correctness gates vs. advisory scans.** `npm run check` is the fast, offline, whole-project correctness ladder (`tsc → oxlint → oxfmt → cspell`) and can run anywhere, including the agent loop. `knip` (~4s, whole project) and `check-outdated` (~6.5s, queries the npm registry) are advisory, not correctness — a stale dependency or an unused export must not fail a feature PR — so they moved to `npm run maintain`, kept out of pre-commit, and run in CI as a **non-blocking** job (see `.github/workflows/ci.yml`). Note `check-outdated` exits non-zero whenever any dep is outdated, which is exactly why it must not gate merges. - **The pre-commit hook sets the `LEFTHOOK_FILES` env var** to the staged-files list, and the affected scripts use `${LEFTHOOK_FILES:-}` to default to the whole project when invoked manually. This keeps `package.json#scripts` as the single source of truth for the underlying commands — `lefthook.yml` only describes _what to run on which files_. - **`tslib` and `type-fest` are deliberately not used.** `tslib` is a runtime helper for old ES3/ES5 targets (the project targets ES2024); `type-fest` was never imported. knip caught both. @@ -62,6 +63,7 @@ Script names follow a prefix convention that signals _when_ they run: - `check:*` — read-only verification. Aggregated by `npm run check`. Used in pre-commit hooks and CI's build job. - `fix:*` — mutating counterpart of `check:*`. Aggregated by `npm run fix`. Use after `npm run check` to auto-resolve issues. - `test:*` — test scripts. `npm run test` runs the full suite; `test:unit` / `test:ci` are scope-specific variants. +- `maintain:*` — advisory repo-maintenance scans (dead code, dependency freshness). Aggregated by `npm run maintain`. Whole-project and/or network-bound, so **not** correctness gates: run on a maintenance branch, and in CI as a non-blocking job that reports without failing. - `publish:*` — runs only at publish time, in the CI `publish` job (immediately before `npm publish`). There is no local `npm run publish` script — publishing is CI-only by policy. ## Contributing diff --git a/package.json b/package.json index e85b49e..0336560 100644 --- a/package.json +++ b/package.json @@ -35,10 +35,8 @@ }, "scripts": { "build": "tsc -p tsconfig.build.json", - "check": "npm run check:tsc && npm run check:oxlint && npm run check:oxfmt && npm run check:cspell && npm run check:knip && npm run check:outdated", + "check": "npm run check:tsc && npm run check:oxlint && npm run check:oxfmt && npm run check:cspell", "check:cspell": "cspell lint ${LEFTHOOK_FILES:-.}", - "check:knip": "knip --include dependencies,exports,files", - "check:outdated": "check-outdated --ignore-pre-releases --ignore-packages @oxfmt/binding-darwin-arm64,@oxfmt/binding-darwin-x64,@oxfmt/binding-linux-arm64-gnu,@oxfmt/binding-linux-arm64-musl,@oxfmt/binding-linux-x64-gnu,@oxfmt/binding-linux-x64-musl,@oxfmt/binding-win32-x64-msvc,@oxlint/binding-darwin-arm64,@oxlint/binding-darwin-x64,@oxlint/binding-linux-arm64-gnu,@oxlint/binding-linux-arm64-musl,@oxlint/binding-linux-x64-gnu,@oxlint/binding-linux-x64-musl,@oxlint/binding-win32-x64-msvc,@oxlint-tsgolint/darwin-arm64,@oxlint-tsgolint/darwin-x64,@oxlint-tsgolint/linux-arm64,@oxlint-tsgolint/linux-x64,@oxlint-tsgolint/win32-arm64,@oxlint-tsgolint/win32-x64", "check:oxfmt": "oxfmt --check ${LEFTHOOK_FILES:-.}", "check:oxlint": "oxlint ${LEFTHOOK_FILES:-src}", "check:tsc": "tsc", @@ -46,6 +44,9 @@ "fix": "npm run fix:oxlint && npm run fix:oxfmt", "fix:oxfmt": "oxfmt ${LEFTHOOK_FILES:-.}", "fix:oxlint": "oxlint --fix src", + "maintain": "npm run maintain:knip && npm run maintain:outdated", + "maintain:knip": "knip --include dependencies,exports,files", + "maintain:outdated": "check-outdated --ignore-pre-releases --ignore-packages @oxfmt/binding-darwin-arm64,@oxfmt/binding-darwin-x64,@oxfmt/binding-linux-arm64-gnu,@oxfmt/binding-linux-arm64-musl,@oxfmt/binding-linux-x64-gnu,@oxfmt/binding-linux-x64-musl,@oxfmt/binding-win32-x64-msvc,@oxlint/binding-darwin-arm64,@oxlint/binding-darwin-x64,@oxlint/binding-linux-arm64-gnu,@oxlint/binding-linux-arm64-musl,@oxlint/binding-linux-x64-gnu,@oxlint/binding-linux-x64-musl,@oxlint/binding-win32-x64-msvc,@oxlint-tsgolint/darwin-arm64,@oxlint-tsgolint/darwin-x64,@oxlint-tsgolint/linux-arm64,@oxlint-tsgolint/linux-x64,@oxlint-tsgolint/win32-arm64,@oxlint-tsgolint/win32-x64", "test": "npm run check:tsc && node --test --strip-types \"src/**/*.test.ts\"", "test:ci": "c8 --reporter=text --reporter=lcov --reporter=html node --test --strip-types \"src/**/*.test.ts\"", "test:unit": "node --test --strip-types \"src/**/*.test.ts\"",