diff --git a/.pair/adoption/decision-log/2026-07-31-pre-push-gate-is-check-only.md b/.pair/adoption/decision-log/2026-07-31-pre-push-gate-is-check-only.md new file mode 100644 index 000000000..cb407616a --- /dev/null +++ b/.pair/adoption/decision-log/2026-07-31-pre-push-gate-is-check-only.md @@ -0,0 +1,69 @@ +# Decision: The pre-push quality gate checks formatting; it never applies it + +## Date + +2026-07-31 + +## Status + +Active + +## Category + +Convention Adoption + +## Context + +The husky **pre-push** hook runs the repo-root `quality-gate` script, which included `turbo prettier:fix mdlint:fix` plus `markdownlint-fix.sh '*.md'` — repo-wide formatters in **write** mode. + +The decisive problem is not noise, it is uselessness: **at pre-push the commits already exist**. A write-mode formatter rewrites the *working tree* and cannot touch what is being pushed, so its output goes nowhere unless the author notices and amends. Repo-wide, it also rewrites files the author never touched. + +Observed **three times in two days** on this repo, on the same two `pair-cli` version-check test files, each time because the hook reformatted files the author never touched: + +- **PR #388** (story #281, `feature/US-281-assess-cost-report-mode`) was pushed with `--no-verify` after the hook rewrote them; +- the same two files were then swept into **PR #411** (story #407, `chore/US-407-preserve-nested-references-subdir`) by a `git add -A` and had to be reverted; +- and excluded by hand from **PR #408** (story #278, `feature/US-278-quickstart-bootstrap-path`). + +The escape hatch is `--no-verify`, and once bypassing is routine the hook asserts nothing at all. + +A second failure mode surfaced while implementing this: generated artifacts. In write mode, generated trees (`.source/`, `playwright-report/`, `test-results/`) were silently rewritten on every push. In **check** mode the same trees would **block** every push — a generated file must be able to do neither, so "gitignored ⇒ never formatted/checked" becomes a correctness requirement rather than a nicety. + +## Decision + +**Formatters run in check mode inside the gate; formatting is applied deliberately, by the developer, as a commit.** + +- `pnpm format:check` is what the gate runs. It is the only formatting step reachable from `quality-gate`. It **aggregates** rather than short-circuits (`pnpm prettier:check; _p=$?; pnpm mdlint:check; _m=$?; exit $((_p > 1 || _m > 1 ? 2 : (_p || _m)))`): with `&&`, a single prettier violation hid every markdownlint violation in the tree, so a contributor with both kinds of drift paid two push cycles — push fails → `pnpm format` → push → fails again on markdown. One run now names everything to fix. Verified with a deliberate violation of each kind: the `&&` form reported only the prettier one. The aggregation preserves the DISTINCTION between a violation and a broken tool: either checker exiting above 1 (a wrapper that died — e.g. an unresolved `@pair/prettier-config` after a partial install) surfaces as exit **2**, not as the 1 that means "run `pnpm format`". Collapsing both to 1 would send a contributor to the fix command for a failure the fix command cannot touch. +- `pnpm format` (= `pnpm prettier:fix && pnpm mdlint:fix`) is the write-mode command a developer runs when the check fails. Both compose the existing root scripts rather than re-typing their bodies, so check and fix cover the same file set by construction (including root-level `*.md`, which only the root `mdlint:*` scripts reach — `turbo` runs tasks in workspace members only). +- The gate is guarded against regression by `pnpm gate:composition` → `packages/dev-tools/src/quality-gates/pre-push-gate-composition.ts`, which reads the real root `package.json`, **expands runner references transitively** (`pnpm x`, `pnpm run x`, flagged forms like `pnpm -s x`, and the `npm run` / `yarn` spellings — a single token between the runner and the script name would otherwise defeat the expansion), and fails if any write-mode formatter is reachable from `quality-gate`. The guard must survive the indirection (`pnpm format:check`), because the likeliest regression is a one-word edit (`pnpm format`) or a redefinition of `format:check`. The offender list is **symmetric across the tools** — turbo task, `bin` alias / `.sh` entrypoint, and the raw CLI with its write flag (`prettier --write`, `markdownlint --fix`, `eslint --fix`) — since a list covering one tool's shell form and not the other's leaves an equally plausible regression shape open. The **intended** invariant is "no step reachable from the gate writes files", not merely "no write-mode *formatter*", and the offender list is widened to match it beyond the formatters: eslint's autofix (`lint:fix` → `eslint . --fix`), plus the two root scripts this repo already defines whose job IS to write — `sync-version` (→ `sync-version-in-docs.ts`, `writeFileSync` across every `.md`/`.mdx` it walks; its `--check` dry-run is spared, bounded to the same command segment) and `test:perf` (→ `benchmark-update-link.ts`, a scratch KB tree plus `reports/performance/benchmark-report.json`, no dry-run, banned outright). Nothing in the gate runs any of them today — which is when widening a guard is free. It also fails if the gate stops *running* the guard (a mention such as `echo gate:composition` does not count), or if the remedy it advertises (`pnpm format`) ceases to exist. + + **What is actually enforced is that explicit list, not the invariant in general.** A newly added, differently named writer — or one reached through a package script, whose body the guard never reads — passes green and is caught by review, not here. The list is deliberately explicit rather than a `/:fix/`-style pattern: a guard that fires on strings it has no opinion about gets disabled. So "add the write script to `WRITE_MODE_FORMATTERS`" is part of adding a write script to this repo. +- Both formatter wrappers (`tools/prettier-config/bin/*`, `tools/markdownlint-config/bin/*`) additionally honour the repo's own `.gitignore` files (repo root + the package being checked), so gitignored/generated output is never formatted and never blocks a push — without enumerating generated directories reactively. The ignore assembly lives in ONE sourced helper per tool (`bin/_ignore-args.sh`, `bin/_ignore-file.sh`), because check and fix seeing the same file set is an invariant and duplication is how it drifts (`dup:check` scans TypeScript only, so shell drift is invisible to it). +- The two tools resolve those patterns against different bases, so the root `.gitignore` is **re-anchored to the cwd for markdownlint** (`bin/_reanchor-gitignore.awk`): prettier resolves each ignore file's patterns against that file's directory — git semantics — while markdownlint-cli resolves them against the cwd, which would make a path-anchored entry (`apps/website/gen/`) match nothing from inside that package and block every push from it. + +## Alternatives Considered + +- **lint-staged / pre-commit auto-fix** (the alternative the story offered): a pre-*commit* hook formatting only staged files *would* be correct in principle — the fix lands in the commit being created. Rejected for now: it adds a dependency and a second hook, moves the failure earlier into every commit (this repo commits per task, often mid-work), and cannot cover the "already committed, about to push" case the gate exists for. Check mode at pre-push achieves the goal (no unrelated rewrite, violations still fail before push) with no new tooling. Revisitable as its own story if manual `pnpm format` proves annoying in practice. +- **Keep write mode, restrict it to changed files**: rejected. Still cannot fix the commits being pushed, and computing "changed files" at pre-push means the diff against the upstream ref — logic the hook does not have and that varies by push shape. +- **Drop the formatters from the gate entirely**: rejected. That silently removes the assurance; a formatting violation in a changed file would reach CI (where no `format:check` job exists) unnoticed. +- **Enumerate generated directories in the shared ignore files**: rejected as the primary mechanism. It is reactive — each new generated tree blocks a push until someone adds a line — and it was already the cause of two blocked pushes here. Delegating to `.gitignore` makes the rule declarative. +- **A comment/doc line saying "don't put `:fix` in the gate"**: rejected as the only safeguard. The regression is one word and its symptom (a diff full of unrelated files) reads as author error, not tooling behaviour; the executable guard is what makes it non-silent. + +## Consequences + +- `pnpm quality-gate` no longer modifies the working tree. A formatting violation in a changed file still fails the push, with `pnpm format` named in the failure message. +- Contributors must run `pnpm format` themselves after a `format:check` failure; documented in [DEVELOPMENT.md](../../../DEVELOPMENT.md) (Quality Gates + Root-Level Commands) **and in the published copy** `apps/website/content/docs/contributing/development-setup.mdx`, which restates the same gate description for the docs site. The two copies are hand-kept: `docs:staleness` checks skill/CLI counts and dead `/docs` links only, and `format:check` does not read `apps/website/content` prose — so any future change to the gate's composition has to touch both files. To make the next divergence visible, the load-bearing paragraph (gate list + "never formats" + two-step remedy) is kept **byte-identical between the two files except for the ADL link form** (relative path vs GitHub blob URL) — a `diff` of the two paragraphs is therefore a one-line diff when they are in sync, which a hand-kept pair that starts life already unequal never gives you. The claim is scoped to that paragraph on purpose: the surrounding root-command blocks legitimately list different commands (the contributor page omits the release ones), so demanding byte-equality there would make the drift signal report three lines on a pair that IS in sync — which is how a reader learns to ignore it. +- The hop count from `packages/dev-tools/src/quality-gates/` to the repo root lives in one module (`repo-root.ts`), imported by all four gate tools in that folder — it had drifted into four copies of the same `resolve(__dirname, '..', '..', '..', '..')`, which is the duplication ADR-014 records as already broken once by a folder move. +- The guard (`gate:composition`) runs inside the gate, and its unit test asserts the real root `package.json` — so removing the guard from the gate fails the test suite that the gate itself runs. +- The new gate tool follows the ADR-014 shape shared by its siblings in `packages/dev-tools/src/quality-gates/`: an importable, unit-tested module plus `main()` behind a `require.main === module` guard (no separate `-cli.ts`). +- The wrappers' ignore delegation — which the guard cannot see, since it inspects `package.json` scripts only — is covered by the `format-ignore-delegation.sh` smoke test (both tools, check and fix, inside a throwaway git repo with its own prettier config so discovery cannot escape the fixture), per the gate-tooling ADL (2026-07-13): script/CLI surfaces are verified by smoke test, not by unit test. The check-mode half asserts **exit status** (1 = violations, anything else = broken wrapper) and the absence of tool-error output, because both tools name the offending path inside their own error text — a text-only assertion passes on a wrapper that died. The re-anchoring awk is covered table-driven in the same scenario (one fixture `.gitignore`, three cwd positions), since its branches fail as a silent OVER-ignore rather than an error. +- `turbo.json` declares the ignore sources as `globalDependencies` (root `.gitignore`, the shared ignore files, both `bin/` dirs). The formatter tasks are cacheable, so without it an edit to `.gitignore` touches no package file and turbo replays a pre-change result — a stale PASS, or a stale FAIL a developer cannot clear. +- Enforcement point is the local hook only. CI runs `lint` and coverage but no `format:check`, and `--no-verify` still skips the hook — deliberately unchanged here (CI workflow changes are out of scope for #394). Tracked as story **#413**. +- `format:check` covers workspace packages plus root-level `*.md` only: `.pair/**`, `scripts/**` and root-level JSON are checked in neither mode (pre-existing, unchanged here). Tracked as story **#414**. +- **The remedy is two-step for one file set, because format scope and mirror scope disagree.** `packages/knowledge-hub/dataset/.skills///SKILL.md` IS in format scope (workspace package); its generated twin `.claude/skills/pair-/SKILL.md` is NOT (`.claude/` is not a workspace member — `pnpm-workspace.yaml` lists `apps/*`, `packages/*`, `tools/*`, and the root `mdlint:check` extra step globs a non-recursive `'*.md'`). Meanwhile `packages/knowledge-hub/src/tools/skill-md-mirror.ts` asserts the two are byte-equal by regenerating the `.claude` copy through the real `pair update` transform. So a format-only edit to the dataset copy turns a green `format:check` into a red `skills:conformance` **later in the same gate**, and a contributor following "run `pnpm format`" alone lands back on `--no-verify`. The mechanism pre-dates #394 (the old gate ran `mdlint:fix` in-gate, hiding it), but this decision promotes `pnpm format` to the documented happy path, so the two-step remedy — re-sync the `.claude/skills/**` copies with `pair update` in the same commit — is stated in all three places that advertise the remedy: `DEVELOPMENT.md`, the `development-setup.mdx` twin, and `PRE_PUSH_REMEDY` (unit-tested). Hit for real on this branch: clearing main's MD049 drift in `assess-cost/SKILL.md` broke the mirror guard until the twin was propagated. The **structural** fix is bringing both copies into one format scope — noted on story **#414**, whose body lists `.claude/**/*.json` but neither `.claude/skills/**/*.md` nor this mirror-guard coupling. +- Framework-scope guidance (`.pair/knowledge/.../quality-gates.md`) showed `prettier:fix` as the example gate command; corrected here to `format:check` with the reason stated in the cell, which closes the framework half of #415. The **bootstrap dataset template no longer does** — `dataset/.pair/adoption/tech/way-of-working.md` was corrected in this story (gate line + Custom Gate Registry row, the row also promoted to `Required: Yes`, since a check that neither fixes nor blocks is weaker than the auto-fixer it replaced). So a newly bootstrapped project no longer inherits the anti-pattern — neither in its own adoption file nor from the framework example. #415 is closed by this story. Promoting this project decision to framework guidance is a separate, framework-audience decision; tracked as story **#415**. + +## Adoption Impact + +- [way-of-working.md](../tech/way-of-working.md): Quality Gates section — the gate contents bullet names `format:check` + `gate:composition`, plus a short "formatters never run in write mode" bullet pointing to this ADL; the Custom Gate Registry row for gate 1 describes it as "formatting check (never fix)". +- [tech-stack.md](../tech/tech-stack.md): the markdownlint entry notes the gate reaches `mdlint:check` only, `mdlint:fix` being reachable through `pnpm format`. +- **This ADL** has no knowledge-base/dataset mirror: sibling ADLs in `adoption/decision-log/` are adoption-only records (see [2026-07-13-gate-tooling-code-in-tested-modules.md](./2026-07-13-gate-tooling-code-in-tested-modules.md)), and the decision is project-level. That is about the *record*, not about its reach: the shipped **template** was corrected here (above), because a template telling adopters their gate runs a write-mode formatter propagates the defect regardless of where the decision is recorded. Promoting the *rationale* into framework guidance stays a separate, framework-audience call. diff --git a/.pair/adoption/tech/tech-stack.md b/.pair/adoption/tech/tech-stack.md index 7d5b1d46c..8b31531ad 100644 --- a/.pair/adoption/tech/tech-stack.md +++ b/.pair/adoption/tech/tech-stack.md @@ -88,7 +88,7 @@ Use `turbo` from the repository root to run cross-workspace tasks (e.g. `turbo b - eslint-plugin-jsx-a11y v6.10.2 (for accessibility linting) - globals v15.0.0 - prettier v3.6.2 (configured via workspace `tools/prettier-config`) - - markdownlint-cli v0.47.0 (configured via workspace `tools/markdownlint-config`, wired into `pnpm quality-gate` via `mdlint:check` / `mdlint:fix`) + - markdownlint-cli v0.47.0 (configured via workspace `tools/markdownlint-config`, wired into `pnpm quality-gate` via `mdlint:check` only — the write-mode `mdlint:fix` is reachable through `pnpm format`, never from the gate) - jscpd v5.0.12 (copy/paste duplication scanner, configured via `.jscpd.json`, wired into `pnpm quality-gate` via `dup:check`; current threshold 5% — baseline ~1.4%) ## Git hooks diff --git a/.pair/adoption/tech/way-of-working.md b/.pair/adoption/tech/way-of-working.md index 695608676..6e6589154 100644 --- a/.pair/adoption/tech/way-of-working.md +++ b/.pair/adoption/tech/way-of-working.md @@ -40,7 +40,8 @@ Maps this project's GitHub Projects board columns to the 5 canonical macrostates ## Quality Gates - `pnpm quality-gate` is the adopted project-level quality gate command. -- Quality gate includes: type checking (`ts:check`), testing (`test`), linting (`lint`), formatting (`prettier:fix`), markdown lint (`mdlint:fix`). +- Quality gate includes: type checking (`ts:check`), testing (`test`), linting (`lint`), formatting and markdown lint in **check mode** (`format:check`), plus a guard that the gate stays check-mode (`gate:composition`). +- **No step reachable from the gate writes files**: the gate reports, `pnpm format` / `pnpm lint:fix` fix deliberately. `gate:composition` enforces this through an **explicit offender list** — the two formatters, eslint autofix, and the repo's write scripts (`sync-version`, `test:perf`) — so **adding a new write-mode script to this repo means adding it to that list**; a differently named writer passes the guard green. See ADL [2026-07-31-pre-push-gate-is-check-only.md](../decision-log/2026-07-31-pre-push-gate-is-check-only.md). - **Pre-merge tiering**: `disabled` (default) — every PR runs the full pre-merge check suite. Set to `enabled` to opt into risk-tier-scoped pre-merge checks (lighter checks on lower-risk PRs) per [tier-aware-pipeline.md](../../knowledge/guidelines/infrastructure/cicd-strategy/tier-aware-pipeline.md); `/pair-capability-setup-gates` reads this flag before generating the pipeline. - **Coverage guardrail**: `enabled` — pair dogfoods its own capability: the [`Coverage guardrail` step](../../../.github/workflows/ci.yml) in CI sources [`coverage-gate.sh`](../../knowledge/assets/coverage-gate.sh), extracts the line-coverage % from each package's istanbul `coverage-summary.json`, and blocks a PR whose coverage drops below the human-committed baseline in [`tech/coverage-baseline.md`](./coverage-baseline.md) (maintaining/improving passes — not an absolute wall). The framework **default remains `disabled`** (the dataset template ships off); this line is pair's project-level opt-in only. See [coverage guardrail](../../knowledge/guidelines/infrastructure/cicd-strategy/tier-aware-pipeline.md#coverage-guardrail-opt-in-regression-gate-consumed-by-this-pipeline) + [config format](../../knowledge/assets/coverage-config-example.md); `/pair-capability-setup-gates` reads this flag before generating the pipeline. **Coverage baseline commit-back**: `disabled` — the separate, nested opt-in ratchet (#372, framework default also `disabled`): when `enabled`, a **push to the base branch** (never a PR run, never a fork) proposes a raised `baseline.` as a **bot pull request** from `chore/coverage-baseline-ratchet`, never a push to `main`, and requires a repo-scoped `COVERAGE_RATCHET_TOKEN` (`contents: write` + `pull requests: write`, no protection bypass) — without it the step warns and the gate's verdict is unchanged. It stays `disabled` here until story #234's branch protection is applied and that secret is provisioned (ADR-018 lands with that story, so it is not linked from here yet); see ADL [2026-07-30-coverage-ratchet-pr-not-push.md](../decision-log/2026-07-30-coverage-ratchet-pr-not-push.md). - **Gate & tooling code:** a gate's logic lives in a tested module in its owning package (white-box unit tests); scripts/CLIs are thin entrypoints and a root gate delegates (`pnpm --filter `). Scripts are never unit-tested — CLI-level checks go to smoke tests. See ADL [2026-07-13-gate-tooling-code-in-tested-modules.md](../decision-log/2026-07-13-gate-tooling-code-in-tested-modules.md). Gate/tooling packages are organized by bounded context, not one package per tool family — a new tool family sharing an existing package's bounded context is a new folder there, not a new package. See [ADR-014](adr/adr-014-tool-package-boundary-by-bounded-context.md). @@ -51,7 +52,7 @@ Maps this project's GitHub Projects board columns to the 5 canonical macrostates | Order | Gate | Command | Scope Key | Required | Description | | ----- | ------------ | --------------------------------- | --------- | -------- | ----------------------------------------------- | -| 1 | Quality Gate | `pnpm quality-gate` | quality | Yes | build test and formatting check&fix | +| 1 | Quality Gate | `pnpm quality-gate` | quality | Yes | build, test, formatting check (never fix) | | 2 | Smoke tests | `pnpm smoke-tests` | testing | Yes | smoke test to check all e2e cli release process | | 3 | E2E tests | `pnpm --filter @pair/website e2e` | testing | Yes | Playwright E2E tests (builds + starts Next.js) | diff --git a/.pair/knowledge/guidelines/quality-assurance/quality-standards/quality-gates.md b/.pair/knowledge/guidelines/quality-assurance/quality-standards/quality-gates.md index 4b33c3413..d32a46ad9 100644 --- a/.pair/knowledge/guidelines/quality-assurance/quality-standards/quality-gates.md +++ b/.pair/knowledge/guidelines/quality-assurance/quality-standards/quality-gates.md @@ -157,7 +157,7 @@ Projects can define additional quality gates beyond the standard ones (Lint, Typ | ----------- | ---------------------------------------------------------------------------------- | | Order | Execution sequence among custom gates (standard gates always run first) | | Gate | Human-readable gate name | -| Command | Shell command to execute (e.g., `pnpm prettier:fix`) | +| Command | Shell command to execute (e.g., `pnpm format:check`) — a gate step must not WRITE | | Scope Key | Maps to `/pair-capability-verify-quality` `$scope` argument for selective execution | | Required | `Yes` = failure → FAIL verdict; `No` (Advisory) = failure → WARNING, not a blocker | | Description | Brief explanation of what the gate checks | diff --git a/DEVELOPMENT.md b/DEVELOPMENT.md index 808a3bb4a..5c25252e0 100644 --- a/DEVELOPMENT.md +++ b/DEVELOPMENT.md @@ -62,15 +62,18 @@ This is a **pnpm monorepo** using **Turbo** for task orchestration and build cac ```bash pnpm install # Install all dependencies -pnpm quality-gate # Full quality check (ts:check + test + lint + prettier + mdlint + hygiene) +pnpm quality-gate # Full quality check (ts:check + test + lint + format check + hygiene) +pnpm format # Apply formatting (prettier + markdownlint, write mode) +pnpm format:check # Check formatting only — what the gate runs; never writes pnpm test # Run all tests (Turbo) pnpm build # Build all packages (Turbo) pnpm lint # Lint all packages (Turbo) pnpm lint:fix # Auto-fix linting issues (Turbo) pnpm ts:check # Type-check all TypeScript (Turbo) pnpm test:coverage # Tests with coverage report -pnpm prettier:check # Check formatting -pnpm prettier:fix # Auto-format code +pnpm prettier:check # Check formatting (prettier only) +pnpm prettier:fix # Auto-format code (prettier only) +pnpm mdlint:check # Check markdown only (markdownlint) pnpm clean # Clean build artifacts and caches pnpm sync-deps # Update all dependencies recursively pnpm deps:outdated # Show outdated dependencies @@ -118,7 +121,20 @@ Before committing, always run: pnpm quality-gate ``` -This runs (in order): `ts:check`, `test`, `lint`, `prettier:fix`, `mdlint:fix`, `hygiene:check`, `docs:staleness`. +This runs (in order): `ts:check`, `test`, `lint`, `format:check` (prettier + markdownlint, **check mode**), `gate:composition`, `hygiene:check`, `docs:staleness`, `skills:conformance`, `dup:check`. + +The gate never formats. It is the pre-push hook, where the commits already exist: a write-mode +formatter would rewrite the working tree without touching what is being pushed, so it only pollutes +the next diff. On a `format:check` failure, run `pnpm format` and commit the result. Exit **2** instead of 1 means a +formatter wrapper itself failed (a broken install, not drift) — read its output rather than running +`pnpm format`. **Two-step remedy:** +if `pnpm format` touched `packages/knowledge-hub/dataset/.skills/**`, re-sync the generated +`.claude/skills/**` copies (`pair update`) in the same commit, or `skills:conformance` fails later in the +same gate — the dataset copy is inside format scope, its generated twin is not (`.claude/` is not a +workspace member), and the mirror guard asserts the two are byte-equal through the real `pair update` +transform. `gate:composition` guards the gate against a write-mode step (formatter or eslint autofix) +creeping back in. See ADL +[2026-07-31-pre-push-gate-is-check-only.md](.pair/adoption/decision-log/2026-07-31-pre-push-gate-is-check-only.md). ### Custom Gate Registry diff --git a/apps/pair-cli/src/commands/kb-info/version-check-formatter.test.ts b/apps/pair-cli/src/commands/kb-info/version-check-formatter.test.ts index 5909c13e4..59b56adba 100644 --- a/apps/pair-cli/src/commands/kb-info/version-check-formatter.test.ts +++ b/apps/pair-cli/src/commands/kb-info/version-check-formatter.test.ts @@ -82,7 +82,7 @@ describe('formatVersionCheckHuman', () => { ) const output = formatVersionCheckHuman(result) - const installedLine = output.split('\n').find((l) => l.includes('Installed:')) ?? '' + const installedLine = output.split('\n').find(l => l.includes('Installed:')) ?? '' expect(installedLine).toContain('non-stable') }) }) diff --git a/apps/pair-cli/src/commands/kb-info/version-check.test.ts b/apps/pair-cli/src/commands/kb-info/version-check.test.ts index 090a9d678..b71a6b683 100644 --- a/apps/pair-cli/src/commands/kb-info/version-check.test.ts +++ b/apps/pair-cli/src/commands/kb-info/version-check.test.ts @@ -28,9 +28,7 @@ describe('compareVersions', () => { const result = compareVersions(installed({ version: '1.1.0' }), current({ version: '1.2.0' })) expect(result.status).toBe('drift') - expect(result.migrationUrl).toBe( - 'https://pair.foomakers.com/docs/migrations/v1.1.0-to-v1.2.0', - ) + expect(result.migrationUrl).toBe('https://pair.foomakers.com/docs/migrations/v1.1.0-to-v1.2.0') }) it('reports drift WITHOUT a migration URL on downgrade (installed newer than current)', () => { diff --git a/apps/website/content/docs/contributing/development-setup.mdx b/apps/website/content/docs/contributing/development-setup.mdx index ff0e75ae6..0ad09495c 100644 --- a/apps/website/content/docs/contributing/development-setup.mdx +++ b/apps/website/content/docs/contributing/development-setup.mdx @@ -74,7 +74,20 @@ Before committing, always run: pnpm quality-gate ``` -This runs (in order): `ts:check`, `test`, `lint`, `prettier:fix`, `mdlint:fix`, `hygiene:check`, `docs:staleness`. +This runs (in order): `ts:check`, `test`, `lint`, `format:check` (prettier + markdownlint, **check mode**), `gate:composition`, `hygiene:check`, `docs:staleness`, `skills:conformance`, `dup:check`. + +The gate never formats. It is the pre-push hook, where the commits already exist: a write-mode +formatter would rewrite the working tree without touching what is being pushed, so it only pollutes +the next diff. On a `format:check` failure, run `pnpm format` and commit the result. Exit **2** instead of 1 means a +formatter wrapper itself failed (a broken install, not drift) — read its output rather than running +`pnpm format`. **Two-step remedy:** +if `pnpm format` touched `packages/knowledge-hub/dataset/.skills/**`, re-sync the generated +`.claude/skills/**` copies (`pair update`) in the same commit, or `skills:conformance` fails later in the +same gate — the dataset copy is inside format scope, its generated twin is not (`.claude/` is not a +workspace member), and the mirror guard asserts the two are byte-equal through the real `pair update` +transform. `gate:composition` guards the gate against a write-mode step (formatter or eslint autofix) +creeping back in. See ADL +[the pre-push gate is check-only](https://github.com/foomakers/pair/blob/main/.pair/adoption/decision-log/2026-07-31-pre-push-gate-is-check-only.md). ### Gate registry @@ -88,8 +101,8 @@ This runs (in order): `ts:check`, `test`, `lint`, `prettier:fix`, `mdlint:fix`, Husky is configured to run checks automatically: -- **Pre-commit** — Linting and formatting checks. -- **Pre-push** — Tests run before pushing. +- **Pre-commit** — Type checking (`pnpm ts:check`). +- **Pre-push** — The full quality gate (`pnpm quality-gate`), formatting included in **check** mode. ### Troubleshooting hooks @@ -97,20 +110,24 @@ Husky is configured to run checks automatically: |---------|----------| | Hooks not running | Run `pnpm install` after cloning. Verify `.husky/` exists. | | Permission errors | `chmod +x .husky/*` | -| Pre-commit fails | Run hook commands manually (`pnpm lint`, `pnpm test`) to debug. | +| Pre-commit fails | Run the hook command manually (`pnpm ts:check`) to debug. | +| Pre-push fails on formatting | Run `pnpm format`, commit the result, push again — the gate never formats for you. | ## Common commands ```bash pnpm install # Install all dependencies -pnpm quality-gate # Full quality check +pnpm quality-gate # Full quality check (never writes — formatting is checked only) +pnpm format # Apply formatting (prettier + markdownlint, write mode) +pnpm format:check # Check formatting only — what the gate runs pnpm build # Build all packages pnpm test # Run all tests pnpm lint # Lint all packages pnpm lint:fix # Auto-fix linting issues pnpm ts:check # Type-check all TypeScript -pnpm prettier:check # Check formatting -pnpm prettier:fix # Auto-format code +pnpm prettier:check # Check formatting (prettier only) +pnpm prettier:fix # Auto-format code (prettier only) +pnpm mdlint:check # Check markdown only (markdownlint) pnpm clean # Clean build artifacts and caches ``` diff --git a/package.json b/package.json index db75c2812..9fc0858b2 100644 --- a/package.json +++ b/package.json @@ -28,9 +28,12 @@ "docs:staleness": "pnpm --filter @pair/website docs:staleness", "skills:conformance": "pnpm --filter @pair/knowledge-hub skills:conformance", "dup:check": "jscpd apps packages", - "quality-gate": "turbo ts:check test lint && turbo prettier:fix mdlint:fix && ./tools/markdownlint-config/bin/markdownlint-fix.sh '*.md' && pnpm hygiene:check && pnpm docs:staleness && pnpm skills:conformance && pnpm dup:check", + "quality-gate": "turbo ts:check test lint && pnpm format:check && pnpm gate:composition && pnpm hygiene:check && pnpm docs:staleness && pnpm skills:conformance && pnpm dup:check", "e2e": "pnpm --filter @pair/website e2e", - "smoke-tests": "./scripts/smoke-tests/run-all.sh --cleanup" + "smoke-tests": "./scripts/smoke-tests/run-all.sh --cleanup", + "format": "pnpm prettier:fix && pnpm mdlint:fix", + "format:check": "pnpm prettier:check; _p=$?; pnpm mdlint:check; _m=$?; exit $((_p > 1 || _m > 1 ? 2 : (_p || _m)))", + "gate:composition": "pnpm --filter @pair/dev-tools pre-push-gate:check" }, "devDependencies": { "@changesets/cli": "catalog:", diff --git a/packages/dev-tools/package.json b/packages/dev-tools/package.json index 9b7ccbefd..1d5935f45 100644 --- a/packages/dev-tools/package.json +++ b/packages/dev-tools/package.json @@ -16,7 +16,8 @@ "code-hygiene:check": "ts-node src/quality-gates/code-hygiene-check.ts", "sync-version": "ts-node src/quality-gates/sync-version-in-docs.ts", "benchmark-update-link": "ts-node src/quality-gates/benchmark-update-link.ts", - "determine-version": "ts-node src/release/determine-version.ts" + "determine-version": "ts-node src/release/determine-version.ts", + "pre-push-gate:check": "ts-node src/quality-gates/pre-push-gate-composition.ts" }, "devDependencies": { "@pair/eslint-config": "workspace:*", diff --git a/packages/dev-tools/src/quality-gates/benchmark-update-link.ts b/packages/dev-tools/src/quality-gates/benchmark-update-link.ts index 716366547..65e03279d 100644 --- a/packages/dev-tools/src/quality-gates/benchmark-update-link.ts +++ b/packages/dev-tools/src/quality-gates/benchmark-update-link.ts @@ -15,14 +15,15 @@ * (package script `benchmark-update-link`, delegated from the repo-root `test:perf` * script). Exit 0 = all targets met, Exit 1 = a hard threshold was missed. * - * REPO_ROOT is resolved from this file's location: packages/dev-tools/src/quality-gates - * -> src -> dev-tools -> packages -> repo root (up 4). + * REPO_ROOT comes from `./repo-root` — the hop count to the repo root is defined + * once for this folder, not per tool. */ import { execSync } from 'child_process' import { existsSync, mkdirSync, readdirSync, rmSync, writeFileSync } from 'fs' -import { join, resolve } from 'path' +import { join } from 'path' + +import { REPO_ROOT } from './repo-root' -const REPO_ROOT = resolve(__dirname, '..', '..', '..', '..') const BENCHMARK_DIR = join(REPO_ROOT, '.benchmark-tmp') const CLI_DIST_PATH = join(REPO_ROOT, 'apps/pair-cli/dist/cli.js') const REPORT_PATH = join(REPO_ROOT, 'reports/performance/benchmark-report.json') diff --git a/packages/dev-tools/src/quality-gates/code-hygiene-check.ts b/packages/dev-tools/src/quality-gates/code-hygiene-check.ts index 5f9af432a..1108158fc 100644 --- a/packages/dev-tools/src/quality-gates/code-hygiene-check.ts +++ b/packages/dev-tools/src/quality-gates/code-hygiene-check.ts @@ -7,15 +7,14 @@ * (package script `code-hygiene:check`, delegated from the repo-root `hygiene:check` * script). Exit 0 = clean, Exit 1 = violations found. * - * Runs `git grep` with an explicit `cwd` (the repo root, resolved from this file's - * location: packages/dev-tools/src/quality-gates -> src -> dev-tools -> packages - * -> repo root, up 4) so the scan behaves identically regardless of the caller's - * working directory (e.g. `pnpm --filter` sets cwd to the package dir, not the repo root). + * Runs `git grep` with an explicit `cwd` (the repo root, from `./repo-root` — the + * hop count is defined once for this folder) so the scan behaves identically + * regardless of the caller's working directory (e.g. `pnpm --filter` sets cwd to + * the package dir, not the repo root). */ import { execSync } from 'child_process' -import { resolve } from 'path' -const REPO_ROOT = resolve(__dirname, '..', '..', '..', '..') +import { REPO_ROOT } from './repo-root' // Path of this module, relative to the repo root — excluded from its own scan so // the patterns below (deliberately split so they don't match this file) never diff --git a/packages/dev-tools/src/quality-gates/pre-push-gate-composition.test.ts b/packages/dev-tools/src/quality-gates/pre-push-gate-composition.test.ts new file mode 100644 index 000000000..f30b298d3 --- /dev/null +++ b/packages/dev-tools/src/quality-gates/pre-push-gate-composition.test.ts @@ -0,0 +1,423 @@ +import { describe, it, expect } from 'vitest' +import { readFileSync } from 'fs' +import { + findWriteModeFormatters, + expandScriptReferences, + referencesScript, + checkRootGate, + checkThisRepoGate, + ROOT_PACKAGE_JSON, + GUARD_SCRIPT, + REMEDY_SCRIPT, + PRE_PUSH_REMEDY, +} from './pre-push-gate-composition' + +// #394. The husky pre-push hook runs the root `quality-gate` script, which ran +// `prettier:fix` / `mdlint:fix` REPO-WIDE in write mode. +// +// The decisive argument is not noise, it is uselessness: at pre-push the commits +// already exist, so a write-mode formatter rewrites the WORKING TREE and cannot +// touch what is being pushed. The reformats go nowhere unless the author notices +// and amends — so the author either sweeps foreign files into the next commit or +// pushes with `--no-verify`. Once bypassing is routine the hook asserts nothing. +// +// This guard exists because the regression is a one-word edit away, and its +// symptom (a diff polluted with unrelated files) looks like author error rather +// than tooling behaviour. Three times in two days it polluted a PR here (#388, +// #408, #411 — see the ADL's Context). +describe('the pre-push gate never runs a write-mode step (#394)', () => { + it('flags prettier:fix', () => { + expect(findWriteModeFormatters('turbo ts:check test lint && turbo prettier:fix')).toEqual([ + 'prettier:fix', + ]) + }) + + it('flags mdlint:fix', () => { + expect(findWriteModeFormatters('turbo mdlint:fix && pnpm hygiene:check')).toEqual([ + 'mdlint:fix', + ]) + }) + + it('flags the markdownlint-fix shell entrypoint, not only the turbo task', () => { + expect( + findWriteModeFormatters("./tools/markdownlint-config/bin/markdownlint-fix.sh '*.md'"), + ).toEqual(['markdownlint-fix']) + }) + + // The offender list must be SYMMETRIC across the two tools: listing markdownlint's + // shell entrypoint while omitting prettier's makes the prettier `.sh`/bin form an + // equally plausible regression that walks straight past the guard. + it("flags the prettier-fix shell entrypoint too, not only markdownlint's", () => { + expect(findWriteModeFormatters('./tools/prettier-config/bin/prettier-fix.sh')).toEqual([ + 'prettier-fix', + ]) + }) + + it('flags the bin aliases the tool packages declare (`turbo prettier-fix`)', () => { + expect(findWriteModeFormatters('turbo prettier-fix')).toEqual(['prettier-fix']) + expect(findWriteModeFormatters('turbo markdownlint-fix')).toEqual(['markdownlint-fix']) + }) + + it('flags the raw CLIs with their write flag, bypassing every alias', () => { + expect(findWriteModeFormatters('prettier --write .')).toEqual(['prettier --write']) + expect(findWriteModeFormatters("markdownlint --fix '**/*.md'")).toEqual(['markdownlint --fix']) + }) + + it('does not pair a check-mode invocation with a --write from another command', () => { + // The write flag belongs to the second command; `prettier:check` is innocent. + expect(findWriteModeFormatters('turbo prettier:check && other-tool --write')).toEqual([]) + }) + + it('flags every offender, so a partial fix cannot look clean', () => { + expect(findWriteModeFormatters('turbo prettier:fix mdlint:fix')).toEqual([ + 'prettier:fix', + 'mdlint:fix', + ]) + }) + + it('accepts the check-mode composition', () => { + expect( + findWriteModeFormatters( + 'turbo ts:check test lint && pnpm format:check && pnpm hygiene:check && pnpm dup:check', + ), + ).toEqual([]) + }) + + // eslint's autofix is not a formatter, but it WRITES: `lint:fix` resolves to + // `eslint . --fix`, which modifies files the branch never touched — AC1 of #394 + // ("pushing a branch never modifies a file the branch did not change"), one word + // away from the gate. Nothing runs it today; the guard covers it anyway. + it('flags lint:fix — eslint autofix writes files just like a formatter', () => { + expect(findWriteModeFormatters('turbo lint:fix')).toEqual(['lint:fix']) + }) + + it('flags the lint-fix shell entrypoint and the raw `eslint --fix`', () => { + expect(findWriteModeFormatters('./tools/eslint-config/bin/lint-fix.sh')).toEqual(['lint-fix']) + expect(findWriteModeFormatters('eslint . --config eslint.config.cjs --fix')).toEqual([ + 'eslint --fix', + ]) + }) + + it('leaves check-mode eslint alone (a --fix belonging to another command)', () => { + expect(findWriteModeFormatters('eslint . && other-tool --fix')).toEqual([]) + }) + + // The invariant is "no step reachable from the gate WRITES", and formatters are not + // the only writers this repo already has. `pnpm sync-version` → sync-version-in-docs.ts + // writeFileSync's every .md/.mdx it walks, i.e. it rewrites version strings in docs the + // branch never touched — AC1's exact failure mode, from a real, currently-defined root + // script whose only job is to write. + it('flags sync-version — it rewrites version strings in every doc it walks', () => { + expect(findWriteModeFormatters('turbo lint && pnpm sync-version 0.4.2')).toEqual([ + 'sync-version', + ]) + }) + + it('flags the sync-version module path too (same entry, `-` is a word boundary)', () => { + expect( + findWriteModeFormatters('ts-node src/quality-gates/sync-version-in-docs.ts 0.4.2'), + ).toEqual(['sync-version']) + }) + + // sync-version has a legitimate dry-run (`--check`, exit 1 on drift) that a gate could + // reasonably run. Sparing it keeps the guard from banning the one non-writing form — + // bounded to the same command segment, so a `--check` next door does not launder a write. + it('spares `sync-version --check` (dry-run) but not a --check in the next command', () => { + expect(findWriteModeFormatters('pnpm sync-version 0.4.2 --check')).toEqual([]) + expect(findWriteModeFormatters('pnpm sync-version 0.4.2 && other-tool --check')).toEqual([ + 'sync-version', + ]) + }) + + // `pnpm test:perf` → benchmark-update-link.ts writes a scratch KB tree and + // reports/performance/benchmark-report.json. No check mode exists: banned outright. + it('flags test:perf and the benchmark module it runs (no dry-run exists)', () => { + expect(findWriteModeFormatters('pnpm test:perf')).toEqual(['test:perf']) + expect(findWriteModeFormatters('ts-node src/quality-gates/benchmark-update-link.ts')).toEqual([ + 'benchmark-update-link', + ]) + }) + + it('names the remedy, so a failure is actionable', () => { + expect(PRE_PUSH_REMEDY).toContain(`pnpm ${REMEDY_SCRIPT}`) + }) + + // The remedy is two-step or it is a trap: `pnpm format` fixes the dataset SKILL.md and + // cannot reach its generated .claude twin (not a workspace member), while skill-md-mirror + // asserts byte equality — so format:check-green becomes skills:conformance-red later in + // the SAME gate. Reproduced on the real MD049 drift this branch cleared. + it('the remedy warns that a dataset .skills edit needs the .claude mirror re-synced', () => { + expect(PRE_PUSH_REMEDY).toContain('packages/knowledge-hub/dataset/.skills/**') + expect(PRE_PUSH_REMEDY).toContain('.claude/skills/**') + expect(PRE_PUSH_REMEDY).toContain('skills:conformance') + }) +}) + +// The gate no longer NAMES a formatter — it delegates to `pnpm format:check`. +// A scan of the gate string alone would therefore be defeated by the very +// indirection #394 introduced: `&& pnpm format` in the gate, or redefining +// `format:check` as `turbo prettier:fix`, both restore repo-wide write-mode +// formatting with a green guard. Expansion is what makes the guard real. +describe('script references are expanded transitively before scanning (#394)', () => { + it('inlines a `pnpm