Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 30 additions & 6 deletions docs/agents/pull-requests.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,27 @@
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.
- Gross diff budget: 1,000 lines by `git diff --stat origin/main...HEAD` (three dots: merge base
to head, so commits `main` gained since your base never count). The exception is a rename-only
move PR titled `refactor(move)`, proven by `git diff -M90% --stat origin/main...HEAD` showing
pure rename/move with no material content diff.

## Validation lifecycle

Focused red/green checks while developing, then review rounds, then final fixes — then one
**successful** full `pnpm check:affected --run` on the exact commit that is pushed. Run it yourself
before pushing, or let a serialized gate stage run it on the pushed head and append the exact-head
result to the PR body; either way the body records that result before the PR is reported as
published. A failed attempt is diagnostic, not a stop sign: fix the cause, push the fix, and rerun
until it is clean. Never claim a run you did not see complete on that head; exact-head CI is the
authority from there.

## Commits

Implementation commit(s) come first. Enforcement edits — pins, baselines, ownership tables, exports
maps, `.fallowrc`, gate manifests — land in one final commit titled `chore(gates): <what and why>`.
Reviewers read that commit as enforcement: it decides whether the change is gated at all, never
something to skim past.

## Rebasing onto a moving `main`

Expand All @@ -45,17 +66,18 @@ answer once independent migration units regularly land against each other; until

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.
when asked or when the work is intentionally incomplete. Keep the whole body at or under 250 words.

- `## 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. 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.
- `## Validation`: name the tested commit SHA, then a concise statement of what ran and its
outcome — scenario names, manual device/browser 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. Never point at a local file path; the body must stand on its own for a remote
reviewer. 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 the touched-file count and whether scope grew beyond the initial command family.

Expand All @@ -71,6 +93,8 @@ when asked or when the work is intentionally incomplete.
- 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.
- A fix that cites an earlier PR as the cause adds its regression at the seam that PR missed, not
at the layer being edited.
- 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
Expand Down
73 changes: 34 additions & 39 deletions docs/agents/testing.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,9 @@ ownership lives in `scripts/check-affected/` and `scripts/gate/`.
Three tiers:

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.
2. On the exact pushed head (locally, or by a gate stage recording it on the PR):
`pnpm check:affected --run`. It derives the relevant local gates and lists checks CI or a
native toolchain owns.
3. For a broad refactor, or when the full deterministic gate is requested: `pnpm check`.

GitHub stays authoritative for provider integration, full coverage, native builds, device lanes, and
Expand All @@ -21,20 +22,18 @@ 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.
`check:affected --run` reports coverage obligations without instrumenting; it runs one capped
`vitest related` command. Use the 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, which is what catches internal `apple` leaking onto
the wire.
- A response emitting `platform` or `appleOs` needs provider integration and coverage evidence;
only the provider project catches internal `apple` leaking onto the wire.
- A workspace package manifest or TypeScript config can rewire all consumers, so the affected
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 shows their failure direction.
need a planted violation showing their failure direction.

## Platform and live-device policy

Expand Down Expand Up @@ -71,10 +70,10 @@ barrel on purpose: one barrel made every test evaluate every helper's transitive
`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.
Use `mkdtempForTest` or `mkdtempForTestSync`. Global setup redirects `TMPDIR` for the run and
removes it after every worker exits — skip per-test cleanup. An interrupted run may leave a
directory behind; the next run prunes it once its owner 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
Expand All @@ -91,11 +90,11 @@ A regression test must be seen failing without the production change: revert the
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, not just lifecycle completion.
Example: React Native Gesture Handler's
A callback-based canary must observe semantic success, not just lifecycle completion — e.g. React
Native Gesture Handler's
[`onFinalize`](https://docs.swmansion.com/react-native-gesture-handler/docs/fundamentals/callbacks-events/)
also fires when recognition fails or is interrupted — use an activation-dependent callback, or
assert the callback's success state before publishing a pass.
also fires on failed or interrupted recognition. Use an activation-dependent callback, or assert
success state before publishing a pass.

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
Expand All @@ -105,8 +104,8 @@ For structured classifiers, pair the positive case with the closest negative. Wh
can be identical with and without a typed reason, the negative test must prove the message alone
cannot activate retry, fallback, or recovery.

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.
Test through public interfaces where practical; never add production exports or test-only
dependency injection — a missing seam needs a real product seam.

## Properties, fuzzing, and mutation

Expand All @@ -116,10 +115,9 @@ 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
outcome they planted, so silent acceptance and wrong error codes are failures. Cases run in a
worker process, so the two faults a case cannot report about itself — never returning, and killing
the process it runs in — are reported as `hang` and `crash` against the exact input rather than
taking the caller down with them. Promote a discovered case with the command the harness prints —
never hand-copy an unshrunk input.
worker process, so a case that never returns or kills its process is reported as `hang` or `crash`
against the exact input. Promote a discovered 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`. It measures
whether tests distinguish changed decision logic. Do not infer redundancy from line coverage alone.
Expand All @@ -133,10 +131,9 @@ pnpm depgraph affected packages/host-kit/src/command.ts
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. Feed the plan into `pnpm check:affected --run` — do not keep a parallel gate
list in prose.
It reports value-edge dependents, affected gates, reaching public commands, 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

Expand All @@ -150,28 +147,26 @@ limitations (manual-only, opaque owners) belong in the gate declarations, not he

## Concurrency torture lane

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:
The harness uses a deterministic scheduler for modeled lock grants and a real request-scope
serialization guard. A seed reproduces the scheduler trace and terminal invariant:

```sh
pnpm test:concurrency-torture
TORTURE_SEED=1234 pnpm test:concurrency-torture
```

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.
modeled boundary is documented in the harness module; every failure prints its exact replay command.

## Real-subprocess-spawn tests
## Test evidence and versioned inputs

`SUBPROCESS_STUB_TESTS` enumerates the few files that spawn a real subprocess per case. They ran
serialized in their own Vitest project until #1823's kill criterion: now un-serialized in
`unit-core`'s default forks pool, reverted if a timeout-shaped failure appears within 20 consecutive
CI runs. Still excluded from the mutation lane either way. There is no unit-test retry layer — fix
or remove flakes.
Run- or commit-stamped benchmark output is never committed under `scripts/`: produce it at run
time or fetch it from the evidence branch. Versioned inputs (fuzz corpus, Maestro fixtures,
schemas, `contracts/fixtures/` tables) are unaffected.

## Speed rules

- Unit tests have no retry layer. Fix or remove flakes instead of hiding them behind retries.
- 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
Expand All @@ -181,7 +176,7 @@ or remove flakes.
- Test files over 1,000 lines may be no longer than at the merge-base with `origin/main`, and no
new test file may cross that line. Split the family before adding tests; shrinking needs no
gate edit.
- 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.
- Keep isolation enabled and the pool on forks — both were measured and did not help. The useful
optimization is importing the module under test, not a platform barrel.
- Local Vitest runs use a four-worker cap. Override it when a run needs a different host share:
`AGENT_DEVICE_VITEST_MAX_WORKERS=<n>` (clamped to host CPUs, ignored in CI).
Loading