From ba503920044ff1ab89f391331fef6cd08ca5c3ff Mon Sep 17 00:00:00 2001 From: Gianluca Carucci Date: Fri, 31 Jul 2026 15:02:03 +0200 Subject: [PATCH 1/9] [US-394] fix: the pre-push gate checks formatting instead of applying it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Test-first: the guard was written and verified RED against the repo's own package.json before the gate changed. The gate ran `prettier:fix` and `mdlint:fix` REPO-WIDE in write mode, and the gate is the pre-push hook. The decisive problem is not noise, it is uselessness: at pre-push THE COMMITS ALREADY EXIST, so a write-mode formatter rewrites the working tree and cannot fix what is being pushed. Its output goes nowhere unless the author notices and amends — so the author either sweeps unrelated reformats into the next commit or pushes with `--no-verify`, and once bypassing is routine the hook asserts nothing. Observed three times in two days: #388 was pushed with --no-verify after the hook reformatted two unrelated pair-cli test files; the same two files were swept into #411 by a `git add -A` and had to be reverted; and they were excluded by hand from #408. - `format` — the explicit fix command (was the gate's write-mode step) - `format:check` — what the gate runs now - `gate:composition` — a tested module that reads the root package.json and fails if a write-mode formatter is ever put back. The regression is a one-word edit away and its symptom looks like author error rather than tooling behaviour, which is why a comment would not have been enough. - `**/.source/` prettier-ignored: generated by fumadocs-mdx at postinstall and already gitignored. In write mode the gate silently rewrote it on every push; in check mode it would have BLOCKED every push. A build artifact must not be able to do either. Includes the one-time sweep check mode requires: the two pair-cli version-check test files are formatted here. Sweeping unrelated files in the PR that forbids sweeping them is deliberate — the drift has to be cleared once for check mode to be viable, and after this the gate cannot produce such sweeps. Guard logic in a tested module under packages/dev-tools/src/quality-gates/ with a thin CLI entrypoint, per ADL 2026-07-13. The offender list is explicit rather than a `/:fix/` pattern: `lint:fix` is an eslint autofix, a different concern, and is asserted NOT to trip the guard. `pnpm quality-gate` green, and it now prints "✓ pre-push gate composition: check-mode only". Closes #394 Co-Authored-By: Claude Opus 5 --- .pair/adoption/tech/way-of-working.md | 2 +- .prettierignore | 2 +- .../kb-info/version-check-formatter.test.ts | 2 +- .../commands/kb-info/version-check.test.ts | 4 +- package.json | 7 +- packages/dev-tools/package.json | 3 +- .../pre-push-gate-composition-cli.ts | 17 ++++ .../pre-push-gate-composition.test.ts | 95 +++++++++++++++++++ .../pre-push-gate-composition.ts | 65 +++++++++++++ tools/prettier-config/.prettierignore | 7 ++ 10 files changed, 195 insertions(+), 9 deletions(-) create mode 100644 packages/dev-tools/src/quality-gates/pre-push-gate-composition-cli.ts create mode 100644 packages/dev-tools/src/quality-gates/pre-push-gate-composition.test.ts create mode 100644 packages/dev-tools/src/quality-gates/pre-push-gate-composition.ts diff --git a/.pair/adoption/tech/way-of-working.md b/.pair/adoption/tech/way-of-working.md index 695608676..80ba645f5 100644 --- a/.pair/adoption/tech/way-of-working.md +++ b/.pair/adoption/tech/way-of-working.md @@ -40,7 +40,7 @@ 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`). **Formatters never run in write mode inside the gate** (#394): the gate is the pre-push hook, where the commits already exist, so a write-mode formatter rewrites the working tree without touching what is being pushed — it only pollutes the next diff with unrelated files. Apply formatting deliberately with `pnpm format` and commit the result. - **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). diff --git a/.prettierignore b/.prettierignore index db0c46054..c0048af36 100644 --- a/.prettierignore +++ b/.prettierignore @@ -1 +1 @@ -**/.github/agents/*.md \ No newline at end of file +**/.github/agents/*.md 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/package.json b/package.json index db75c2812..6b4090087 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": "turbo prettier:fix mdlint:fix && ./tools/markdownlint-config/bin/markdownlint-fix.sh '*.md'", + "format:check": "turbo prettier:check mdlint:check", + "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..fccb69aef 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-cli.ts" }, "devDependencies": { "@pair/eslint-config": "workspace:*", diff --git a/packages/dev-tools/src/quality-gates/pre-push-gate-composition-cli.ts b/packages/dev-tools/src/quality-gates/pre-push-gate-composition-cli.ts new file mode 100644 index 000000000..d6b5cc5b3 --- /dev/null +++ b/packages/dev-tools/src/quality-gates/pre-push-gate-composition-cli.ts @@ -0,0 +1,17 @@ +/** + * Thin entrypoint for the pre-push gate composition check (#394). All logic and + * all tests live in `pre-push-gate-composition.ts`, per the gate-tooling ADL + * (2026-07-13): scripts are never unit-tested. + */ +import { readFileSync } from 'fs' +import { join } from 'path' +import { checkRootGate } from './pre-push-gate-composition' + +const rootPackageJson = join(__dirname, '../../../../package.json') +const result = checkRootGate(readFileSync(rootPackageJson, 'utf-8')) + +if (!result.ok) { + console.error(`\n❌ pre-push gate composition\n\n${result.message}\n`) + process.exit(1) +} +console.log('✓ pre-push gate composition: check-mode only') 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..b99675172 --- /dev/null +++ b/packages/dev-tools/src/quality-gates/pre-push-gate-composition.test.ts @@ -0,0 +1,95 @@ +import { describe, it, expect } from 'vitest' +import { readFileSync } from 'fs' +import { join } from 'path' +import { + findWriteModeFormatters, + checkRootGate, + 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. Twice in one day it polluted a PR here. +describe('the pre-push gate never runs a formatter in write mode (#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.sh']) + }) + + 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([]) + }) + + it('does not mistake lint:fix for a formatter — it is not one of the two', () => { + // `lint:fix` is an eslint autofix, a separate concern from formatting, and it + // is NOT in the gate. The guard must stay specific rather than banning every + // `:fix` string it sees. + expect(findWriteModeFormatters('turbo lint:fix')).toEqual([]) + }) + + it('names the remedy, so a failure is actionable', () => { + expect(PRE_PUSH_REMEDY).toContain('pnpm format') + }) +}) + +describe('checkRootGate reads the repo gate rather than trusting a copy (#394)', () => { + const pkg = (gate: string): string => JSON.stringify({ scripts: { 'quality-gate': gate } }) + + it('passes on a check-mode gate', () => { + expect(checkRootGate(pkg('turbo ts:check test lint && pnpm format:check')).ok).toBe(true) + }) + + it('fails and NAMES every offender', () => { + const r = checkRootGate(pkg('turbo prettier:fix mdlint:fix')) + expect(r.ok).toBe(false) + expect(r.message).toContain('prettier:fix') + expect(r.message).toContain('mdlint:fix') + expect(r.message).toContain('pnpm format') + }) + + it('fails loudly when there is no gate at all, rather than passing vacuously', () => { + expect(checkRootGate(JSON.stringify({ scripts: {} })).ok).toBe(false) + }) + + it('guards the ACTUAL root package.json of this repo', () => { + // The point of the story: not that a string can be checked, but that THIS + // repo's gate is check-mode. Reading the real file is what makes the + // regression impossible to reintroduce silently. + const root = join(__dirname, '../../../../package.json') + const result = checkRootGate(readFileSync(root, 'utf-8')) + expect(result.ok, result.message).toBe(true) + }) +}) diff --git a/packages/dev-tools/src/quality-gates/pre-push-gate-composition.ts b/packages/dev-tools/src/quality-gates/pre-push-gate-composition.ts new file mode 100644 index 000000000..7b7c067e8 --- /dev/null +++ b/packages/dev-tools/src/quality-gates/pre-push-gate-composition.ts @@ -0,0 +1,65 @@ +/** + * pre-push-gate-composition — keeps the pre-push gate free of write-mode + * formatters (story #394). + * + * The husky pre-push hook runs the root `quality-gate` script. That script used + * to include `prettier:fix` and `mdlint:fix`, repo-wide, in WRITE mode. + * + * The 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. In practice the author either sweeps unrelated reformats into the next + * commit or pushes with `--no-verify` — and once bypassing is routine the hook + * asserts nothing at all. + * + * Check mode is the correct shape for a hook: it reports, the developer fixes + * deliberately with `pnpm format`, and the fix lands in a commit. + * + * Per the gate-tooling ADL (2026-07-13) this logic lives here as a tested + * module; the root script is a thin entrypoint. + */ + +/** + * The two write-mode formatters, plus the shell entrypoint that bypasses turbo. + * Deliberately an explicit list rather than a `/:fix/` pattern: `lint:fix` is an + * eslint autofix, a different concern, and banning every `:fix` string would + * make the guard fire on things it has no opinion about. + */ +const WRITE_MODE_FORMATTERS = ['prettier:fix', 'mdlint:fix', 'markdownlint-fix.sh'] as const + +/** What a developer should run instead. Named in the failure so it is actionable. */ +export const PRE_PUSH_REMEDY = + 'Formatting is checked, not applied, before a push: run `pnpm format` and commit the result. ' + + 'Applying it here could not fix the commits being pushed anyway.' + +/** + * Every write-mode formatter present in a gate command, in the order they are + * declared. Returns all offenders rather than the first, so a partial fix cannot + * look clean. + */ +export function findWriteModeFormatters(gateCommand: string): string[] { + return WRITE_MODE_FORMATTERS.filter(f => gateCommand.includes(f)) +} + +/** + * Reads the repo's own `quality-gate` script and reports any write-mode + * formatter in it. Separated from `main` so the reading is testable without a + * process exit. + */ +export function checkRootGate(packageJsonText: string): { ok: boolean; message: string } { + const pkg = JSON.parse(packageJsonText) as { scripts?: Record } + const gate = pkg.scripts?.['quality-gate'] + if (typeof gate !== 'string') { + return { ok: false, message: 'No `quality-gate` script found in the root package.json.' } + } + const offenders = findWriteModeFormatters(gate) + if (offenders.length === 0) return { ok: true, message: 'quality-gate is check-mode only.' } + return { + ok: false, + message: + `quality-gate runs ${offenders.length} formatter(s) in WRITE mode: ${offenders.join(', ')}.\n` + + `At pre-push the commits already exist, so this rewrites the working tree and cannot fix\n` + + `what is being pushed — it only pollutes the next diff with unrelated files.\n` + + `Use the :check variants and keep the fix in \`pnpm format\`. ${PRE_PUSH_REMEDY}`, + } +} diff --git a/tools/prettier-config/.prettierignore b/tools/prettier-config/.prettierignore index a81306089..f8bf47095 100644 --- a/tools/prettier-config/.prettierignore +++ b/tools/prettier-config/.prettierignore @@ -7,3 +7,10 @@ out/ **/node_modules/ **/.cache/ + +# Generated build artifacts — never authored, so never formatted. +# Before #394 the gate ran prettier in WRITE mode and silently rewrote these on +# every push; in check mode they would BLOCK every push instead. A generated file +# must be able to do neither. +**/.source/ +**/test-results/ From a78bfee77d72f2e9c54e1ad9c383d66e0dc6d2dc Mon Sep 17 00:00:00 2001 From: Gianluca Carucci Date: Fri, 31 Jul 2026 15:30:25 +0200 Subject: [PATCH 2/9] =?UTF-8?q?[US-394]=20fix:=20review=20round=201=20?= =?UTF-8?q?=E2=80=94=20guard=20survives=20indirection,=20root=20md=20check?= =?UTF-8?q?ed,=20ignores=20delegate=20to=20gitignore?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings on PR #412, all resolved. Gate composition (the headline safeguard was defeatable): - the guard now expands `pnpm