From d4b352b33ddfa00cf76455cf2e9eea5432d08245 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Mon, 24 Aug 2026 10:57:57 +0200 Subject: [PATCH 1/2] refactor(lint): replace the facade import scan with a lint rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The surviving half of `contracts-entry-closure.test.ts` walked ~490 candidate files and parsed each one to assert that nothing value-imports the two wide contracts facades. `eslint/no-restricted-imports` already states exactly that, and `allowTypeImports` already draws the one distinction that made the walker seem necessary: `import type` is erased, so it stays legal. Verified rather than assumed, because the override semantics are not additive: a same-rule override REPLACES the parent, so a top-level rule would have been silently dropped for `src/**`, and the existing `"off"` entry for `exec.ts` and the test tree would have exempted the files that carried most of the cost #1959 removed. So the paths are added per zone, and the blanket `"off"` becomes a facade-only config that keeps the `node:child_process` exemption it existed for. Planted red in all three zones — `src/core/capabilities.ts`, a `src/__tests__` file, and `packages/capture-kit/src` — each flagged, while a type-only import in the same probe file was not. A first probe read as a pass because the sed that built it produced a type-only import; the zone was re-probed with a real value import rather than trusting the green. Misconfiguration fails loudly, which is why this is safe to rely on: a typo'd rule name makes oxlint exit 1 with "Rule not found in plugin", not pass silently (the failure mode #1976 records for the `rg` assertions). What a linter cannot replace, and stays: the eager-closure budgets. Those are a transitive-weight property — a module already imported grows an import, and the cost arrives without any single file's import list changing. Per-file rules cannot see that, and `no-restricted-imports` can only ban specifiers named in advance, which is precisely what #1950/#1956/#1959 could not have named. --- .oxlintrc.json | 62 +++++++++++- src/__tests__/contracts-entry-closure.test.ts | 98 ------------------- 2 files changed, 59 insertions(+), 101 deletions(-) delete mode 100644 src/__tests__/contracts-entry-closure.test.ts diff --git a/.oxlintrc.json b/.oxlintrc.json index e0330e37d0..33a1bf1b2d 100644 --- a/.oxlintrc.json +++ b/.oxlintrc.json @@ -15,13 +15,21 @@ "argsIgnorePattern": "^_", "caughtErrorsIgnorePattern": "^_", "destructuredArrayIgnorePattern": "^_", - "fix": { "imports": "safe-fix", "variables": "suggestion" }, + "fix": { + "imports": "safe-fix", + "variables": "suggestion" + }, "varsIgnorePattern": "^_" } ], "eslint/prefer-const": "error", "eslint/no-useless-escape": "error", - "eslint/no-empty": ["error", { "allowEmptyCatch": true }], + "eslint/no-empty": [ + "error", + { + "allowEmptyCatch": true + } + ], "typescript/no-explicit-any": "off", "typescript/only-throw-error": "off", "typescript/no-var-requires": "error", @@ -38,6 +46,16 @@ { "name": "node:child_process", "message": "Use process helpers from src/utils/exec.ts instead of importing node:child_process directly." + }, + { + "name": "@agent-device/contracts/platform", + "allowTypeImports": true, + "message": "Value-importing this facade evaluates all 32 vocabulary modules it re-exports. Import the symbol from the module that owns it (@agent-device/contracts/). `import type` stays fine \u2014 it is erased." + }, + { + "name": "@agent-device/contracts/interaction", + "allowTypeImports": true, + "message": "Value-importing this facade evaluates all 18 vocabulary modules it re-exports. Import the symbol from the module that owns it (@agent-device/contracts/). `import type` stays fine \u2014 it is erased." } ] } @@ -47,7 +65,45 @@ { "files": ["src/utils/exec.ts", "src/**/*.test.ts", "src/**/__tests__/**/*.ts"], "rules": { - "eslint/no-restricted-imports": "off" + "eslint/no-restricted-imports": [ + "error", + { + "paths": [ + { + "name": "@agent-device/contracts/platform", + "allowTypeImports": true, + "message": "Value-importing this facade evaluates all 32 vocabulary modules it re-exports. Import the symbol from the module that owns it (@agent-device/contracts/). `import type` stays fine \u2014 it is erased." + }, + { + "name": "@agent-device/contracts/interaction", + "allowTypeImports": true, + "message": "Value-importing this facade evaluates all 18 vocabulary modules it re-exports. Import the symbol from the module that owns it (@agent-device/contracts/). `import type` stays fine \u2014 it is erased." + } + ] + } + ] + } + }, + { + "files": ["packages/*/src/**/*.ts"], + "rules": { + "eslint/no-restricted-imports": [ + "error", + { + "paths": [ + { + "name": "@agent-device/contracts/platform", + "allowTypeImports": true, + "message": "Value-importing this facade evaluates all 32 vocabulary modules it re-exports. Import the symbol from the module that owns it (@agent-device/contracts/). `import type` stays fine \u2014 it is erased." + }, + { + "name": "@agent-device/contracts/interaction", + "allowTypeImports": true, + "message": "Value-importing this facade evaluates all 18 vocabulary modules it re-exports. Import the symbol from the module that owns it (@agent-device/contracts/). `import type` stays fine \u2014 it is erased." + } + ] + } + ] } }, { diff --git a/src/__tests__/contracts-entry-closure.test.ts b/src/__tests__/contracts-entry-closure.test.ts deleted file mode 100644 index 4e4fce3868..0000000000 --- a/src/__tests__/contracts-entry-closure.test.ts +++ /dev/null @@ -1,98 +0,0 @@ -import { expect, test } from 'vitest'; -import fs from 'node:fs'; -import path from 'node:path'; -import { eagerClosureOf, eagerlyEvaluatedModules } from './eager-import-closure.fixtures.ts'; - -/** - * `@agent-device/contracts/platform` and `/interaction` union 32 and 18 vocabulary - * modules. A file that value-imports either one evaluates the whole union to reach a - * single function, and because permanent hubs sat behind them (#1959) that union rode - * into ~470 of the suite's ~970 test graphs. - * - * Every one of those modules now has its own entry subpath, so the narrow import is - * always available. These two tests keep it that way from both directions: the first - * pins the four hubs the issue named, and the second closes the general case so the - * clump cannot re-form behind a hub nobody thought to list. - * - * Type-only importers are untouched and stay legal — `import type` is erased, so it - * evaluates nothing. That is the same distinction the walker itself draws, which is - * why this reads the AST through it rather than matching specifier text. - */ - -const repoRoot = path.resolve(import.meta.dirname, '../..'); -const CLUMP_ENTRIES = [ - '@agent-device/contracts/platform', - '@agent-device/contracts/interaction', -] as const; -const CLUMP_FACADES = [ - 'packages/contracts/src/facades/platform.ts', - 'packages/contracts/src/facades/interaction.ts', -].map((file) => path.resolve(repoRoot, file)); - -const HUBS = [ - 'src/core/command-descriptor/registry.ts', - 'src/core/capabilities.ts', - 'src/core/interactors/register-builtins.ts', - 'src/core/command-descriptor/platform-execution-entry.ts', -]; - -function sourceFiles(): string[] { - const found: string[] = []; - const walk = (dir: string): void => { - for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { - const full = path.join(dir, entry.name); - if (entry.isDirectory()) { - if (entry.name !== 'node_modules') walk(full); - } else if (entry.name.endsWith('.ts')) found.push(full); - } - }; - walk(path.join(repoRoot, 'src')); - for (const pkg of fs.readdirSync(path.join(repoRoot, 'packages'))) { - const src = path.join(repoRoot, 'packages', pkg, 'src'); - if (fs.existsSync(src)) walk(src); - } - return found; -} - -test('the hubs behind the contracts clump never evaluate either wide facade', () => { - const offenders = HUBS.flatMap((hub) => { - const carried = eagerClosureOf(path.resolve(repoRoot, hub)).filter((file) => - CLUMP_FACADES.includes(file), - ); - return carried.map((file) => `${hub} -> ${path.relative(repoRoot, file)}`); - }); - - expect( - offenders, - 'These hubs sit in hundreds of test graphs, so whatever they evaluate, the suite pays for ' + - 'everywhere. Import the symbol from the vocabulary module that owns it ' + - '(@agent-device/contracts/) instead of the wide facade.', - ).toEqual([]); -}); - -test('no source file value-imports the wide contracts facades', () => { - const offenders: string[] = []; - let typeOnlyImporters = 0; - for (const file of sourceFiles()) { - const source = fs.readFileSync(file, 'utf8'); - // Text-filter before parsing: a file that never names the specifier cannot import - // it, and parsing all ~3000 sources costs more than the unit lane's budget allows. - const mentions = CLUMP_ENTRIES.filter((entry) => source.includes(`${entry}'`)); - if (mentions.length === 0) continue; - const evaluated = new Set(eagerlyEvaluatedModules(file, source)); - for (const entry of mentions) { - if (evaluated.has(entry)) offenders.push(`${path.relative(repoRoot, file)} -> ${entry}`); - else typeOnlyImporters += 1; - } - } - - // Non-vacuity: an empty offender list also describes a scan that parsed nothing, so - // require that the surviving type-only importers were seen and classified as erased. - expect(typeOnlyImporters).toBeGreaterThan(300); - expect( - offenders.sort(), - 'Value-importing these facades evaluates every module they re-export from. Each of those ' + - 'modules has its own entry subpath in packages/contracts/package.json — import from that. ' + - '`import type` from the facades stays fine: it is erased, so it evaluates nothing.', - ).toEqual([]); -}); From b0960c1d50ddfd596d038761cb10dd23816f8f1c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Mon, 24 Aug 2026 12:35:03 +0200 Subject: [PATCH 2/2] docs(agents): simplify testing and pull-request guidance wording testing.md sat 15 bytes under the 10k per-doc check:agent-guidance cap. Rewrite both docs in shorter, plainer sentences without dropping any fact, threshold, or identifier (backtick-identifier sets verified unchanged against the previous revision). Also fix testing.md's gate catalog sentence being separated from its code block and the missing blank line before pull-requests.md's Reviewing section. --- docs/agents/pull-requests.md | 109 +++++++++---------- docs/agents/testing.md | 200 +++++++++++++++++------------------ 2 files changed, 154 insertions(+), 155 deletions(-) diff --git a/docs/agents/pull-requests.md b/docs/agents/pull-requests.md index 8c62b000a8..bccf054af3 100644 --- a/docs/agents/pull-requests.md +++ b/docs/agents/pull-requests.md @@ -3,94 +3,95 @@ ## Readiness - Static gates first: required checks pass, `pnpm check:fallow --base origin/main` is clean when - code-quality/dead-code risk is relevant, CI guards are green, and no conflict markers or unmerged + code-quality/dead-code risk is relevant, CI guards are green, no conflict markers or unmerged paths remain. - A local unit-only run is not CI-green. Use `pnpm test:unit` for the repo unit bundle, or - `vitest run --project unit-core --project subprocess-stub` when invoking Vitest directly. The - **Integration Tests** and **Coverage** jobs run the `provider-integration` project — verify those - green on the actual PR head. + `vitest run --project unit-core --project subprocess-stub` directly. The **Integration Tests** + and **Coverage** jobs run the `provider-integration` project — verify those green on the actual + PR head. - Device-facing behavior is not merge-ready without real simulator/emulator/device evidence for the - changed path. Fixture-backed tests prove contracts; they do not replace a live run that creates or - observes the artifact/state the feature claims to handle. If live verification is blocked, state - the blocker and the exact command/device needed, and downgrade the PR to residual risk rather than - calling it ready. -- Command-surface changes preserve CLI, Node.js, daemon, MCP, help, and docs coverage - where that surface is affected, without duplicating command contracts across layers. + changed path. Fixture-backed tests prove contracts; they do not replace a live run that creates + or observes the artifact/state the feature claims to handle. If live verification is blocked, + state the blocker and the exact command/device needed, and downgrade the PR to residual risk — + do not call it ready. +- Command-surface changes preserve CLI, Node.js, daemon, MCP, help, and docs coverage where that + surface is affected, without duplicating command contracts across layers. - Runtime output stays agent-friendly: compact defaults, top offenders first for diagnostics/perf, bounded arrays in JSON, artifact paths for large raw data, progressive lookup for deeper detail. - Close every manual `agent-device` session opened during verification - (`docs/agents/device-verification.md`) and report any cleanup that could not be completed. -- Two readiness claims, never blurred: **published and reported** means the branch is pushed, the - PR body carries the evidence gathered at a named commit, and CI on the head is the authority - still to come; **merge-ready** means the required checks are green on the actual head and, - where the change touches a device-facing path, the live evidence for that path exists (the - device-facing bullet above; docs-only and pure-tooling changes owe none). "Don't wait for CI" - licenses the first, not the second — say which one you are claiming. + (`docs/agents/device-verification.md`) and report any cleanup you could not complete. +- Two readiness claims, never blurred. **Published and reported**: the branch is pushed, the PR + body carries evidence gathered at a named commit, and CI on the head is the authority still to + come. **Merge-ready**: required checks are green on the actual head and, for device-facing + paths, the live evidence exists (docs-only and pure-tooling changes owe none). "Don't wait for + CI" licenses the first claim, not the second — say which one you are claiming. ## Rebasing onto a moving `main` -`main` has no "require branches up to date" rule; a rebase is not owed to GitHub. Rebase when -there is a conflict, or when the commits `main` gained since your base touch a surface your -change depends on or that decides your gates: +`main` has no "require branches up to date" rule; a rebase is not owed to GitHub. Rebase when there +is a conflict, or when the commits `main` gained since your base touch a surface your change +depends on or that decides your gates: ```sh pnpm check:affected --base --head origin/main # what main gained, by gate ``` If that plan names only files and gates disjoint from yours, the rebase buys nothing but another -full validation cycle. Evidence in the PR body is stamped with the commit it was gathered at, so a -rebase dates it rather than invalidating it, and CI on the new head re-establishes it. A merge -queue is the answer once independent migration units regularly land against each other; until -then this rule is. +full validation cycle. PR-body evidence is stamped with the commit it was gathered at, so a rebase +dates it rather than invalidating it; CI on the new head re-establishes it. A merge queue is the +answer once independent migration units regularly land against each other; until then this rule is. ## PR body Conventional commit prefixes (`feat:`, `fix:`, `chore:`, `perf:`, `refactor:`, `docs:`, `test:`, -`build:`, `ci:`). No bracketed bot tags like `[codex]`. Ready-for-review by default; draft only when -asked or when the work is intentionally incomplete. +`build:`, `ci:`). No bracketed bot tags like `[codex]`. Ready-for-review by default; draft only +when asked or when the work is intentionally incomplete. -- `## Summary`: user/API behavior, not an implementation file tour. Lead with what changed for - operators, clients, command authors, or platform behavior. A compact before/after helps when it - clarifies the workflow or bug fix. For new or changed public APIs, include 1-3 concrete CLI/Node/MCP - examples a reviewer can scan. `Closes #123` when applicable. +- `## Summary`: user/API behavior, not a file tour. Lead with what changed for operators, clients, + command authors, or platform behavior. A compact before/after helps when it clarifies the + workflow or fix. For new or changed public APIs, give 1-3 concrete CLI/Node/MCP examples a + reviewer can scan. `Closes #123` when applicable. - `## Validation`: meaningful evidence in concise prose — scenario names, manual device/browser - evidence, changed screenshots, CI status, notable failures/retries and their outcome. Avoid command - accounting for routine local gates; name an exact command only when it is unusual, manually - reproducible evidence, or needed to explain a residual risk. For docs-only changes, say why runtime - validation does not apply instead of writing a command checklist. + evidence, changed screenshots, CI status, notable failures/retries and their outcome. Skip + command accounting for routine local gates; name an exact command only when it is unusual, + manually reproducible evidence, or needed to explain a residual risk. For docs-only changes, say + why runtime validation does not apply. - Call out real tradeoffs, known gaps, and follow-ups; omit boilerplate when there are none. -- Note touched-file count and whether scope expanded beyond the initial command family. +- Note the touched-file count and whether scope grew beyond the initial command family. + ## Reviewing - Review against the linked issue, not only the diff. State the issue's motivating behavior and verify the PR fixes *that*. - Check relevant ADRs before reviewing architecture, routing, command-surface, platform-boundary, - diagnostics, or testing-strategy changes. An ADR conflict is a review finding unless the PR updates - or supersedes the ADR explicitly. -- Read dependency notes (`Blocked by: ...`, linked PRs, sibling branches) before judging correctness. - A base/sequence problem outranks detail review. -- Trace the real production route from command surface through daemon/request routing to the platform - backend. Tests that mock away the router, or exercise only a helper, do not prove the shipped path. + diagnostics, or testing-strategy changes. An ADR conflict is a finding unless the PR updates or + supersedes the ADR explicitly. +- Read dependency notes (`Blocked by: ...`, linked PRs, sibling branches) before judging + correctness. A base/sequence problem outranks detail review. +- Trace the real production route from command surface through daemon/request routing to the + platform backend. Tests that mock away the router, or exercise only a helper, do not prove the + shipped path. - Before adding an error classifier, trace every producer through normalization, wrapping, - serialization, and transport; inventory sibling consumers and the existing reason-code vocabulary; - then repair the deepest shared boundary that loses the signal. Message text is not a reason code. -- For each key regression test, identify what deletion or revert would make it fail. If reverting the + serialization, and transport; inventory sibling consumers and the existing reason-code + vocabulary; then repair the deepest shared boundary that loses the signal. Message text is not a + reason code. +- For each key regression test, name what deletion or revert would make it fail. If reverting the implementation still passes, the test is vacuous. - For recurring failures, prefer a design that makes the class impossible at the owning interface; keep one small regression as evidence rather than enumerating examples. If a custom guard needs - repeated exceptions or reconstructs compiler/schema behavior, move the invariant to its source of - truth instead of extending the guard. + repeated exceptions or reconstructs compiler/schema behavior, move the invariant to its source + of truth instead of extending the guard. - Check for hidden behavior changes separately from intended refactors: output shape, warning/error propagation, artifact paths, fallback/retry tiers. - Verify tests cover the issue's motivating failure, not just the new abstraction. Prefer before/after evidence when an issue reports a concrete divergence. - Green CI is necessary but insufficient for device-facing or routing-sensitive work. - Check whether the tightening pass removed code/tests the change made obsolete. -- Treat the CI Size workflow as review evidence; local size comparisons are not required by default. - Escalate scrutiny when a PR adds roughly 700 or more net production lines (excluding tests, - generated data, fixtures, and documentation) or increases npm unpacked size by more than 3 kB. - Consider gross additions and deletions too, so a move-dominated change is not mistaken for pure - growth. These thresholds trigger investigation, not automatic rejection: ask an independent - reviewer whether a deeper owning interface, stronger types, less ceremony, reuse of an existing - construction path, or deletion of superseded code can make the change materially smaller. The PR - should itemize justified growth and record why a smaller design was rejected. +- The CI Size workflow is review evidence; local size comparisons are not required by default. + Escalate scrutiny at roughly 700 or more net production lines (excluding tests, generated data, + fixtures, documentation) or more than 3 kB npm unpacked growth. Consider gross additions and + deletions too, so a move-dominated change is not mistaken for pure growth. These thresholds + trigger investigation, not automatic rejection: ask an independent reviewer whether a deeper + owning interface, stronger types, less ceremony, reuse of an existing construction path, or + deletion of superseded code can make the change materially smaller. The PR should itemize + justified growth and record why a smaller design was rejected. diff --git a/docs/agents/testing.md b/docs/agents/testing.md index bda12b1882..23cdce629c 100644 --- a/docs/agents/testing.md +++ b/docs/agents/testing.md @@ -1,23 +1,19 @@ # Testing Notes -This file contains the repository-specific testing traps a contributor cannot derive from the test -runner alone. Executable gate ownership lives in `scripts/check-affected/` and `scripts/gate/`. +Repository-specific testing traps you cannot learn from the test runner alone. Executable gate +ownership lives in `scripts/check-affected/` and `scripts/gate/`. ## Which gates a change needs -Use three tiers: +Three tiers: -1. While editing, run a focused test or `pnpm check:quick`. -2. Before pushing, run `pnpm check:affected --run`. It derives relevant local gates and reports - checks owned by CI or a native toolchain. -3. For a broad refactor or explicitly requested full deterministic gate, run `pnpm check`. +1. While editing: a focused test or `pnpm check:quick`. +2. Before pushing: `pnpm check:affected --run`. It derives the relevant local gates and lists the + checks that CI or a native toolchain owns. +3. For a broad refactor, or when the full deterministic gate is requested: `pnpm check`. -GitHub remains authoritative for provider integration, full coverage, native builds, device lanes, -and history-backed compatibility. To inspect the current gate catalog or a plan, use: - -`check:affected --run` reports coverage obligations but never enables coverage instrumentation. It -runs one capped `vitest related` command for local feedback; run the dedicated coverage scripts only -when diagnosing a red CI result. +GitHub stays authoritative for provider integration, full coverage, native builds, device lanes, and +history-backed compatibility. To inspect the gate catalog or a plan: ```sh pnpm check:affected @@ -25,31 +21,38 @@ pnpm check:affected --json pnpm gate --help ``` +`check:affected --run` reports coverage obligations but never turns coverage instrumentation on. It +runs one capped `vitest related` command. Run the dedicated coverage scripts only to diagnose a red +CI result. + Two selection traps recur: -- A response that emits `platform` or `appleOs` needs provider integration and coverage evidence; - unit tests do not run the provider project that catches internal `apple` leaking onto the wire. +- A response that emits `platform` or `appleOs` needs provider integration and coverage evidence. + Unit tests do not run the provider project, which is what catches internal `apple` leaking onto + the wire. - A workspace package manifest or TypeScript config can rewire all consumers, so the affected - selector deliberately fails open to the full gate set. + selector fails open to the full gate set on purpose. Docs-only changes with no behavior impact need no runtime tests. Structural guidance gates still -need a planted violation that demonstrates their failure direction. +need a planted violation that shows their failure direction. ## Platform and live-device policy HarmonyOS has no provisioned CI emulator, physical device, DevEco image, or HDC installation. Unit, -provider, and coverage tests mock the typed HDC seam; real validation is local hardware evidence -following `docs/agents/device-verification.md`. Do not add a CI lane that assumes a developer host. +provider, and coverage tests mock the typed HDC seam. Real validation is local hardware evidence per +`docs/agents/device-verification.md`. Do not add a CI lane that assumes a developer host. Apple runner changes run `pnpm check:xctest-selection` and build the affected target. The source -`#if` guard is the XCTest lane classification; do not maintain a second test-name list. Pure runner -decisions use the macOS host lane, while iOS/XCTest semantics require a simulator lane. +`#if` guard is the XCTest lane classification — never maintain a second test-name list. Pure runner +decisions use the macOS host lane; iOS/XCTest semantics need a simulator lane. + +Local host-lane XCTest runs hit two snags CI never does: -Local host-lane XCTest runs hit two snags CI never does. System policy may refuse the unsigned -bundle (`library load disallowed by system policy`, surfacing as `Early unexpected exit … crashed -with signal kill`); rebuild signed, with `CODE_SIGN_IDENTITY="Apple Development"` or a manual -identity from `security find-identity -v -p codesigning`. The first run also needs XCUITest -automation permission for the host app. +- System policy may refuse the unsigned bundle (`library load disallowed by system policy`, shown + as `Early unexpected exit … crashed with signal kill`). Rebuild signed: + `CODE_SIGN_IDENTITY="Apple Development"`, or pick an identity from + `security find-identity -v -p codesigning`. +- The first run needs XCUITest automation permission for the host app. Live smoke commands and their environment contracts live with their harnesses: @@ -58,72 +61,69 @@ Live smoke commands and their environment contracts live with their harnesses: `test/integration/smoke-ios-simulator-coverage.test.ts` - concurrency: `test/integration/nightly/concurrency-torture.test.ts` -Read those entry files before running a lane; do not copy their changing environment matrix here. +Read the entry file before running a lane. Do not copy its environment matrix here — it changes. ## Shared test utilities -Before creating fixtures, inspect the modules under `src/__tests__/test-utils/` and import the named -builders directly from the module that defines them (for example `session-factories.ts`, -`device-fixtures.ts`, `store-factory.ts`). There is deliberately no barrel: importing through one -made every test evaluate all helpers' transitive graphs. Shared `DeviceInfo`, session, snapshot, -store, runtime-fact, and mocked-binary values belong in a sibling fixture module rather than -repeated test literals. - -Use `mkdtempForTest` or `mkdtempForTestSync`. The Vitest global setup redirects `TMPDIR` for the -whole run and removes it after every worker finishes; do not add per-test cleanup for directories -those helpers create. An interrupted run may leave a directory temporarily, and the next run prunes -it only after both the owner process and every process using its `TMPDIR` are gone. - -Mock the seam the subject consumes. A daemon handler that binds a runtime gets fake runtime facts and -facets, not a mock of generic dispatch. The old generic dispatch mocks are migration debt; do not add -to them. When an ADR 0019 command migrates, its tests move to the runtime seam in the same PR. - -Vitest workers may signal only themselves and direct children. `hermetic-signal-setup.ts` rejects -other process-table writes. Tests with fabricated PIDs mock `signalPidsBestEffort`, -`signalProcessGroupBestEffort`, or the tool-provider seam; a real child spawned by the test may be +Before creating fixtures, look in `src/__tests__/test-utils/`. Import named builders from the module +that defines them (`session-factories.ts`, `device-fixtures.ts`, `store-factory.ts`). There is no +barrel on purpose: one barrel made every test evaluate every helper's transitive graph. Shared +`DeviceInfo`, session, snapshot, store, runtime-fact, and mocked-binary values belong in a sibling +fixture module, not in repeated test literals. + +Use `mkdtempForTest` or `mkdtempForTestSync`. Global setup redirects `TMPDIR` for the whole run and +removes it after every worker exits — do not add per-test cleanup for those directories. An +interrupted run may leave a directory behind; the next run prunes it once the owner process and +every process using its `TMPDIR` are gone. + +Mock the seam the subject consumes. A daemon handler that binds a runtime gets fake runtime facts +and facets, not a mock of generic dispatch. Generic dispatch mocks are migration debt — do not add +more. When an ADR 0019 command migrates, its tests move to the runtime seam in the same PR. + +Vitest workers may signal only themselves and their direct children; `hermetic-signal-setup.ts` +rejects other process-table writes. Tests with fabricated PIDs mock `signalPidsBestEffort`, +`signalProcessGroupBestEffort`, or the tool-provider seam. A real child the test spawned may be signalled directly. ## Regression evidence -A regression test must be observed failing without the production change. Revert the implementation, -run the smallest owning test, record the failing test count, then restore it. Apply the same proof to -test relocation and structural gates: plant a type error or violation and verify the intended gate -discovers and names it. +A regression test must be seen failing without the production change: revert the implementation, run +the smallest owning test, record the failing count, restore. Apply the same proof to test relocation +and structural gates — plant a type error or violation and watch the intended gate find and name it. -A callback-based canary must observe the subject's semantic success state, not merely lifecycle -completion. For example, React Native Gesture Handler's +A callback-based canary must observe the subject's semantic success, not just lifecycle completion. +Example: React Native Gesture Handler's [`onFinalize`](https://docs.swmansion.com/react-native-gesture-handler/docs/fundamentals/callbacks-events/) -also runs when recognition fails or is interrupted; use an activation-dependent callback or assert -the callback's success state before publishing a passing result. +also fires when recognition fails or is interrupted — use an activation-dependent callback, or +assert the callback's success state before publishing a pass. -A device replay is automatic regression coverage only when an automatic PR or scheduled lane selects -and executes it. Name the owning lane and confirm the scenario ran on the exact PR head; placing a -replay in a manual or otherwise unselected tier is test material, not automatic evidence. +A device replay counts as automatic regression coverage only when an automatic PR or scheduled lane +selects and runs it. Name the owning lane and confirm the scenario ran on the exact PR head. A +replay in a manual or unselected tier is test material, not automatic evidence. -For structured classifiers, pair the positive case with the closest negative case. When an error -message can be identical with and without a typed reason, the negative test must prove the message -alone cannot activate retry, fallback, or recovery. +For structured classifiers, pair the positive case with the closest negative. When an error message +can be identical with and without a typed reason, the negative test must prove the message alone +cannot activate retry, fallback, or recovery. -Tests use public interfaces where practical. Do not create production exports or test-only dependency -injection solely for a test; a missing seam must be a real product seam. +Test through public interfaces where practical. Never add production exports or test-only dependency +injection just for a test; a missing seam must be a real product seam. ## Properties, fuzzing, and mutation -Pure parser and geometry changes extend shared fast-check arbitraries and properties rather than -adding only another example. Keep examples for a real past bug or named decision. Reuse -`PROPERTY_RUNS` budgets so property files stay within the unit slow-test gate. +Pure parser and geometry changes extend the shared fast-check arbitraries and properties — not just +another example. Keep examples for a real past bug or a named decision. Reuse `PROPERTY_RUNS` +budgets so property files stay inside the unit slow-test gate. -Parser fuzz targets live in `scripts/fuzz/targets.ts`; validation generators carry the invalid +Parser fuzz targets live in `scripts/fuzz/targets.ts`. Validation generators carry the invalid outcome they planted, so silent acceptance and wrong error codes are failures. Promote a discovered -case through the command printed by the harness rather than hand-copying an unshrunk input. +case with the command the harness prints — never hand-copy an unshrunk input. -Mutation is report-only and limited to the registry in `scripts/mutation/modules.ts`. Use it to -measure whether tests distinguish changed decision logic; do not infer redundancy from line coverage -alone. +Mutation is report-only and limited to the registry in `scripts/mutation/modules.ts`. It measures +whether tests distinguish changed decision logic. Do not infer redundancy from line coverage alone. -## Before editing a shared module (`pnpm depgraph affected`) +## Before editing a shared module -Run the dependency query before touching a high-fan-in module: +Run `pnpm depgraph affected` before touching a high-fan-in module: ```sh pnpm depgraph affected src/utils/exec.ts @@ -131,24 +131,23 @@ pnpm depgraph affected src/daemon/ref-frame.ts --json --limit 25 ``` It reports value-edge dependents, affected gates, public commands whose handler chains reach the -module, live scenario owners, and interaction-guarantee cells. Type-only and dynamic edges are -classified separately. Run the resulting plan through `pnpm check:affected --run`; do not maintain a -parallel gate list in prose. +module, live scenario owners, and interaction-guarantee cells; type-only and dynamic edges are +classified separately. Feed the plan into `pnpm check:affected --run` — do not keep a parallel gate +list in prose. ## Gate ownership `CHECK_CATALOG` is the executable check registry. CI owns a check only through the shared `run-gate` -action with a literal gate id, and `pnpm check:gate-manifest` verifies registration, workflow -ownership, path reachability, and routed device lanes. Raw shell text cannot declare ownership. +action with a literal gate id; `pnpm check:gate-manifest` verifies registration, workflow ownership, +path reachability, and routed device lanes. Raw shell text cannot declare ownership. -When adding a check, update the catalog and its executable model. When changing path ownership, -plant a path that would previously be misrouted and observe the selector or manifest fail before -fixing it. Keep workflow limitations such as manual-only or opaque owners in the gate declarations, -not duplicated here. +New check: update the catalog and its executable model. Changed path ownership: plant a path that +would previously be misrouted and watch the selector or manifest fail before fixing it. Workflow +limitations (manual-only, opaque owners) belong in the gate declarations, not here. ## Concurrency torture lane -The concurrency harness uses a deterministic scheduler for modeled lock grants and a separate real +The harness uses a deterministic scheduler for modeled lock grants plus a separate real request-scope serialization guard. A seed reproduces the scheduler trace and terminal invariant: ```sh @@ -156,29 +155,28 @@ pnpm test:concurrency-torture TORTURE_SEED=1234 pnpm test:concurrency-torture ``` -Lock plans come from the production request-lock decisions; do not hand-author a parallel plan in the -harness. The modeled boundary is documented in its harness module, and every failure prints the -exact replay command. +Lock plans come from the production request-lock decisions — never hand-author a parallel plan. The +modeled boundary is documented in the harness module, and every failure prints its exact replay +command. ## The `subprocess-stub` project -`SUBPROCESS_STUB_TESTS` enumerates the few files that spawn real subprocesses per case. They run in a -serialized Vitest project so host contention does not turn internal budgets into generic timeouts. -Membership requires naming the real spawned process; environment isolation alone is not a reason. -There is no unit-test retry layer—fix or remove flakes. +`SUBPROCESS_STUB_TESTS` enumerates the few files that spawn real subprocesses per case. They run in +a serialized Vitest project so host contention cannot turn internal budgets into generic timeouts. +Membership requires naming the real spawned process; environment isolation alone does not qualify. +There is no unit-test retry layer — fix or remove flakes. ## Speed rules -- Unit tests do not wait production time. Prefer budget-derived cadence, assert that a caller passes - the correct timeout to its tool seam, or use an existing clock seam. -- Wall clock is bounded by the slowest test file because Vitest parallelizes files. Splitting a - monolith along source topology is a performance improvement as well as a readability improvement. -- The slow-test reporter enforces unit and integration budgets. Existing pins only shrink; a new pin - requires measured justification. -- Test files above 1,000 lines are pinned to their merge-base size and may only shrink. Split the +- Unit tests do not wait production time. Prefer budget-derived cadence, assert the caller passes + the right timeout to its tool seam, or use an existing clock seam. +- Vitest parallelizes files, so wall clock is bounded by the slowest file. Splitting a monolith + along source topology is a performance win, not just a readability win. +- The slow-test reporter enforces unit and integration budgets. Existing pins only shrink; a new + pin needs measured justification. +- Test files over 1,000 lines are pinned to their merge-base size and may only shrink. Split the family before adding tests; never raise the pin. -- Keep Vitest isolation enabled and the pool on forks. Both alternatives were measured and did not - improve the suite; importing the module under test rather than a platform barrel is the useful - optimization. -- Raise the two-worker local cap only for a solo run: `AGENT_DEVICE_VITEST_MAX_WORKERS=`, - clamped to host CPUs, ignored in CI. +- Keep isolation enabled and the pool on forks — both alternatives were measured and did not help. + The useful optimization is importing the module under test, not a platform barrel. +- Raise the two-worker local cap only for a solo run: `AGENT_DEVICE_VITEST_MAX_WORKERS=` + (clamped to host CPUs, ignored in CI).