diff --git a/AGENTS.md b/AGENTS.md index 3b77dcb13..f5fde6759 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -3,8 +3,9 @@ `agent-device` is a CLI and daemon for automating Apple, Android, HarmonyOS, Vega, Linux, and web targets. A long-lived daemon owns sessions; registry-derived commands route to platform runtimes. -This file contains repository-specific traps and invariants that are expensive to rediscover. Load -task procedures only when needed: +## Task routing + +Load only the procedures relevant to the task: | When the task involves | Read | | --- | --- | @@ -14,35 +15,28 @@ task procedures only when needed: | Selector capture, polling, or interaction fast paths | `docs/agents/selector-capture.md` | | Adding or changing a CLI flag | `docs/agents/cli-flags.md` | | Opening or reviewing a PR | `docs/agents/pull-requests.md` | -| Running against a real device | `docs/agents/device-verification.md` | +| Apple runner changes or manual device verification | `docs/agents/device-verification.md` | | Writing issues or PRDs, and triage labels | `docs/agents/issue-tracker.md`, `docs/agents/triage-labels.md` | | Web backend setup or diagnostics | `docs/agents/web-backend.md` | -Versioned CLI help is the source of truth for command behavior. Start with `agent-device help -workflow`, then the relevant topic help. +Versioned CLI help owns command semantics. Read the relevant topic when behavior is unclear; +normal app-driving startup follows the skill's routing card. ## Incident-derived principles -- Guarantees erode at path boundaries. Classify every interaction dispatch path in - `packages/contracts/src/interaction-guarantees.ts`; a registry claim is not proof that the native - implementation satisfies the guarantee's definition. +- Classify interaction dispatch paths in `packages/contracts/src/interaction-guarantees.ts` and + verify each claim against the native implementation. - Delegation on error proves no success-path parity. A fast path may succeed on a candidate the shared rules would refuse. -- Prove that a code path can fire before measuring it. An A/B test with an unreachable arm is two - green runs without evidence. -- A green regression test counts only after it has been observed red against the pre-fix code. Plant - a violation for new structural gates and verify the gate names the invariant. -- Repair recurring failures at their owning interface with types, a registry, or one construction - path. A custom guard that reconstructs another source of truth needs redesign, not another - exception. -- Treat explanatory implementation comments as a failed design review. Do not narrate control flow, - preserve review history, or justify a workaround in code. Express the invariant through names, - types, module boundaries, and tests; put history in the PR or an ADR. Allow only public API docs, - tool directives, and a brief citation to an external constraint that cannot be encoded. -- Key behavior on typed reasons and details, never error text. Existing message sniffs are owned debt - and must not be copied. -- Snapshot output is the token budget: do not add per-node metadata when response-level metadata can - be emitted once. Append warnings through the shared response builder; never replace prior warnings. +- Prove both paths are reachable before measuring a fast path against its baseline. +- Repair recurring failures at the owning type, registry, or construction path; do not add guards + that reconstruct another source of truth. +- Keep control-flow narration and review history out of implementation comments. Encode invariants + in names, types, boundaries, and tests; reserve comments for public API docs, tool directives, + and non-obvious constraints that cannot be encoded. +- Key behavior on typed reasons and details, never error text; do not copy existing message sniffs. +- Keep metadata at response level when it applies to the whole snapshot. Append warnings through + the shared response builder; never replace prior warnings. - Before preserving a compatibility shape, run `git tag --contains `. Unreleased API has no external compatibility obligation. @@ -66,10 +60,10 @@ and progress in `@agent-device/capture-kit` (`request-cancel`, `request-progress contracts in `@agent-device/contracts`; CLI flags in `src/commands/cli-grammar`; cross-surface schema composition in `src/cli-schema`. -The enforced registries are self-declaring. A failing completeness, parity, coverage, timeout, -layering, or construction gate means the new cell or path is unclassified; do not suppress or -allowlist it. Interaction responses are built only through `buildInteractionResponseData`, and -cross-language rules change through golden tables under `contracts/fixtures/`. +Resolve registry completeness failures at the missing declaration. Diagnose other gate failures +at their reported invariant; do not suppress them or add an allowlist to get a pass. Build interaction +responses through `buildInteractionResponseData`; change cross-language rules through golden tables +under `contracts/fixtures/`. ## Hard repository rules @@ -78,9 +72,9 @@ cross-language rules change through golden tables under `contracts/fixtures/`. - Apple target changes keep the kernel device model, runtime-fact admission, dispatch resolution, Apple discovery, and xctestrun preparation in sync. - iOS simulator-set scoping must never hide the host macOS desktop target. -- Skills may carry a minimal start/routing card; command semantics belong in versioned CLI help. -- Do not add compatibility or fallback behavior without explicit approval. Complete migrations and - remove superseded paths. +- Skills stay minimal routing cards. +- Add compatibility or fallback behavior only when explicitly requested or approved. Otherwise, + complete the migration and remove superseded paths. - Keep changes within one command family or module group unless the task explicitly crosses a boundary. Platform-neutral work does not license inspecting every platform implementation. @@ -93,7 +87,6 @@ cross-language rules change through golden tables under `contracts/fixtures/`. - Tests mirror source topology one-to-one. Split a source module and its test together; do not add to the legacy `interaction.test.ts` or platform `index.test.ts` aggregations. Pure moves carry their tests unchanged; rename-only hunks owe no new coverage. -- Shared fixtures are named exports in a sibling fixture module, not repeated inline literals. - `src/daemon/handlers/session.ts` is already over budget; extract the relevant platform-specific concept before adding behavior. @@ -103,30 +96,14 @@ cross-language rules change through golden tables under `contracts/fixtures/`. repository-wide, not path-scoped. - A fresh worktree requires `pnpm install --frozen-lockfile && pnpm build`. Until then package and optional-peer resolution may point at another checkout and produce false failures. -- Source-checkout daemon state is worktree-scoped, but devices are not. Use `pnpm daemon:state-dir` - to inspect it and different devices for concurrent worktrees. -- Run one full gate per host at a time. Subprocess-backed tests under concurrent worktrees produce - timeout-shaped contention failures. -- Before pushing, run `pnpm check:affected --run && git push`. Use `pnpm check` for broad refactors. - GitHub remains authoritative for native, device, provider, and full-coverage lanes. +- Parallel work needs disjoint edit ownership and distinct devices. Run one full gate per host; + concurrent subprocess-backed suites can produce contention timeouts. - The layering scan reads tracked files only. Stage a new module before trusting its result. - Fallow baselines are path-keyed. Move the matching baseline entry when renaming a file; never bulk regenerate baselines to accept unrelated findings. -- The first Node process after a newly signed Apple runner launches may block during Gatekeeper - verification. Warm it with a throwaway `node -e 0` before measuring. -- `DEVICE_IN_USE` has two flavors. "already in use by session X" is this daemon — follow its - `close --session` hint. "owned by session X in workspace Y" is another worktree's device - claim — non-retriable; run the error's `device status`/`device release --stale` recovery, - never PID hunting. -- A changing timeout failure set that passes in isolation is host contention. Reproduce the same - test on `origin/main` under the same load before treating it as a regression. ## Runtime and diagnostics seams -The OS-neutral Apple runner lives under `packages/platform-apple/src/runner/`. For connection errors, -retry policy, or command typing, start at `runner-contract.ts`; transport stays below session/client -behavior, and xctestrun build/cache logic stays outside request execution. - Diagnostics use `@agent-device/capture-kit/diagnostics`. Request diagnostics belong in the session request log; session artifact paths come from `src/daemon/session-store.ts`. App/device logs remain in `app.log`; Apple runner and xcodebuild output remains in `runner.log`. @@ -150,11 +127,8 @@ translate through the interaction root frame. Prefer selector or ref tests over ## Documentation ownership -Before adding guidance, decide whether the command surface, CLI grammar/help, MCP projection, daemon -runtime, ADR, or task procedure owns it. Link to executable registries instead of copying their -contents. Keep a sentence in this file only when no gate, lint rule, versioned help, ADR, or -decision-site comment can own it. `CONTEXT.md` is glossary-only: no implementation paths, -architecture decisions, migration state, or workflows. +Keep this file to repository traps and routing. Put task procedures in `docs/agents/`, decisions in +ADRs, and command semantics in versioned help. Link to executable registries instead of duplicating +their rules. `CONTEXT.md` is glossary-only: no implementation paths, decisions, or migration status. -Behavior changes update their owning help/metadata and user docs. In the final summary, -state whether docs or skills changed and why. +Behavior changes update owning help/metadata and user docs. Report docs or skill changes when relevant. diff --git a/docs/agents/cli-flags.md b/docs/agents/cli-flags.md index c5eb02725..22edc90af 100644 --- a/docs/agents/cli-flags.md +++ b/docs/agents/cli-flags.md @@ -1,16 +1,14 @@ # Adding a CLI Flag -A new flag touches only the layers that need to understand it. Stop at the layer where it stops -mattering — threading it further is the common failure, not stopping too early. +Thread a flag only through the layers that consume it: 1. `packages/contracts/src/cli-flags.ts`: add to `CliFlags`; add the definition to the matching `src/commands/cli-grammar/flag-definitions-*.ts` owner and the relevant group in `flag-groups.ts` (for example `SNAPSHOT_FLAGS`). Then update the command family metadata/schema that exposes the flag; find the owner with `rg -n "|supportedFlags|allowedFlags" src/commands src/cli-schema src/cli/parser`. For - schema-only CLI commands (`cdp`, `auth`, `connect`, `proxy`, `react-devtools`, `web`) the owner is - `SCHEMA_ONLY_CLI_COMMAND_SCHEMAS` in `src/cli-schema/command-overrides.ts`. New flags are - operator-only by default. Add a flag to `PROJECT_CONFIG_FLAG_KEYS` in + schema-only CLI commands, the owner is `SCHEMA_ONLY_CLI_COMMAND_SCHEMAS` in + `src/cli-schema/command-overrides.ts`. New flags are operator-only by default. Add a flag to `PROJECT_CONFIG_FLAG_KEYS` in `src/cli-schema/cli-config.ts` only when repository control is safe; this positive allowlist is the completeness gate. 2. `src/commands/cli-grammar/*`: read the CLI flag into command input. diff --git a/docs/agents/device-verification.md b/docs/agents/device-verification.md index c502cc706..acaaa8c9a 100644 --- a/docs/agents/device-verification.md +++ b/docs/agents/device-verification.md @@ -1,20 +1,18 @@ # Manual Device Verification -Read this before running `agent-device` by hand against a simulator, emulator, or physical device. +Read this for Apple runner changes or manual `agent-device` runs on simulators, emulators, or +physical devices. Live verification steps apply when exercising a device-facing path. -## Before the run: defeat staleness - -Dev-loop staleness has three layers, and each produces a convincing false negative. +## Build freshness - After changing runtime code reached through `bin/agent-device.mjs` or the daemon: `pnpm build`, then `pnpm clean:daemon` — the daemon does not self-reload. - Before any Android verification from source: `pnpm build`, `pnpm build:android`, `pnpm clean:daemon`. `build:android` refreshes and verifies both bundled Android helper artifacts for the current package version. -- `shutdown` deliberately HANDS OFF a healthy simulator runner. The adopted runner keeps serving the - old Swift binary until you kill its process or the source fingerprint changes, so "my change did - nothing" measured against an adopted runner is a classic false negative. If Swift runner code - changed, run `pnpm build:xcuitest`. +- `shutdown` hands off a healthy simulator runner; a new daemon may adopt the old binary. After + Swift runner changes, run `pnpm build:xcuitest` before verification. Use the session cleanup + procedure below if ownership is stuck. ## Prove the path under test was actually active @@ -25,19 +23,30 @@ Dev-loop staleness has three layers, and each produces a convincing false negati simulator, physical-device, Metro/dev-client, and app-surface steps. An already-installed `com.callstack.agentdevicelab` is not sufficient — the README's Metro/dev-build and `snapshot -i` checks must prove the expected app surface is running. -- For Android RN/Expo/dev-client apps on any local Metro port, `adb reverse tcp: tcp:` is - harmless and should be run before opening the app or URL. +- For Android RN/Expo/dev-client apps that use local Metro, configure + `adb reverse tcp: tcp:` for the app's Metro port before opening the app or URL. -## Session hygiene +## Worktree ownership and runner diagnostics + +- Source-checkout daemon state is worktree-scoped, but devices are not. Use `pnpm daemon:state-dir` + to inspect it and different devices for concurrent worktrees. +- The first Node process after a newly signed Apple runner launches may block during Gatekeeper + verification. Warm it with a throwaway `node -e 0` before measuring. +- `DEVICE_IN_USE` has two flavors. "already in use by session X" is this daemon — follow its + `close --session` hint. "owned by session X in workspace Y" is another worktree's device + claim — non-retriable; run the error's `device status`/`device release --stale` recovery, + never PID hunting. -Every manually opened session is a resource that must be closed — including exploratory sessions and -failed verification attempts. +The OS-neutral Apple runner lives under `packages/platform-apple/src/runner/`. For connection errors, +retry policy, or command typing, start at `runner-contract.ts`; transport stays below session/client +behavior, and xctestrun build/cache logic stays outside request execution. + +## Session hygiene -- Every `agent-device open` needs a matching `close` with the same `--session`, `--platform`, - `--udid`, and `--state-dir` before the agent finishes. +- Close manually opened sessions, including failed verification attempts, using their original + `--session`, `--platform`, `--udid`, and `--state-dir` values. - Use a purpose-specific session name for experiments, and an isolated `--state-dir` under `/private/tmp` when you need cleanup isolation beyond the current worktree's default daemon. -- Track opened sessions in working notes; close each one before the final response. - If `close` is blocked or ownership looks stuck, inspect it with `agent-device device status --stale` (daemonless), stop the owning daemon with `agent-device daemon stop --state-dir ` (add `--clean` to remove retained runners), and @@ -48,8 +57,7 @@ failed verification attempts. ## Sandboxed environments -Start the daemon outside the sandbox with escalation. The daemon binds localhost, and sandboxed runs -fail before any product code executes, with `listen EPERM: operation not permitted 127.0.0.1` or -repeated `Failed to start daemon`/metadata cleanup messages. Those are not agent-device regressions — -rerun with escalation. Unit tests, typecheck, lint, and build can stay sandboxed unless they need -devices or listener access. +The daemon binds localhost. If the sandbox rejects the listener with `listen EPERM`, rerun with +host access when permitted. Generic `Failed to start daemon` or cleanup errors alone do not prove a +sandbox cause; inspect the underlying failure. Run other checks in the sandbox unless their tools +require host access. diff --git a/docs/agents/domain.md b/docs/agents/domain.md index f62e944e7..e9fac6e36 100644 --- a/docs/agents/domain.md +++ b/docs/agents/domain.md @@ -1,14 +1,7 @@ # Domain Docs -Single-context repo. Before architecture, diagnosis, TDD, triage, PRD, or roadmap work, read -`CONTEXT.md` for domain vocabulary and the relevant ADRs in `docs/adr/`. Selector-capture work also -reads `docs/agents/selector-capture.md`. - -Use `CONTEXT.md` vocabulary in issue titles, refactor proposals, test names, and architecture notes. -If a proposed change contradicts an ADR, say so explicitly and explain why the decision should be -reopened. - -`AGENTS.md` routes to the rest of this directory by task type. +Use `CONTEXT.md` for domain vocabulary and `docs/adr/README.md` to locate decisions relevant to the +change. A proposed ADR contradiction requires an explicit decision update. ## Test-harness vocabulary diff --git a/docs/agents/issue-tracker.md b/docs/agents/issue-tracker.md index b9ae64a47..01ab4b20a 100644 --- a/docs/agents/issue-tracker.md +++ b/docs/agents/issue-tracker.md @@ -2,33 +2,19 @@ Issues and PRDs for this repo live as GitHub issues in `callstack/agent-device`. Use the `gh` CLI for issue operations. -## Pull requests as a triage surface +## Triage scope -PRs as a request surface: no. - -External PRs are not part of the triage request queue. Use `gh pr` commands only when a task explicitly asks for PR review, CI, or PR maintenance. +External PRs are not task requests in the issue-triage queue. Reading linked PRs to verify an issue's +dependencies is allowed; it does not expand the task into PR maintenance. ## Conventions -Write issues as concise implementation contracts. Start with the purpose. Include a concrete API, -data shape, or CLI example when the issue changes a boundary. State required behavior, observable -completion conditions, and dependencies. Use plain, direct sentences. Remove persuasion, repeated -context, and filler, but never remove a normative requirement to make the issue shorter. - -`gh` authentication may only be available through the user's login shell. In sandboxed agent environments, run GitHub operations through the login shell and request escalation for network/auth access, for example: - -```sh -/bin/zsh -lc 'gh issue view --comments' -``` - -If `gh` still reports that it is not authenticated, do not attempt to reconfigure credentials. Report the exact command that needs to be run from the user's authenticated shell. +Write issues as implementation contracts: purpose, required behavior, observable completion +conditions, and dependencies. Include an API, data shape, or CLI example for boundary changes. -- Create an issue with `gh issue create --title "..." --body "..."`. -- Read an issue with `gh issue view --comments`. -- List issues with `gh issue list --state open --json number,title,body,labels,comments`. -- Comment with `gh issue comment --body "..."`. -- Apply or remove labels with `gh issue edit --add-label "..."` or `--remove-label "..."`. -- Close with `gh issue close --comment "..."`. +Sandboxed `gh` may lack host login credentials. Retry the focused command with host authentication +access when permitted before treating it as an authentication failure. Do not reconfigure +credentials; report the command and remaining blocker if the retry fails. For label meanings and state flow, see `docs/agents/triage-labels.md`. @@ -36,7 +22,10 @@ For label meanings and state flow, see `docs/agents/triage-labels.md`. - Treat `Blocked by: ...` lines, linked prerequisite issues, and branch-base notes as part of the issue contract. - Before scheduling or reviewing work, check blockers and decide whether the work should wait, stack on a prerequisite branch, or explicitly rescope. -- Do not mark an issue or PR ready when it duplicates, conflicts with, or depends on unmerged blocker semantics. +- Do not mark an issue ready for implementation while prerequisite semantics are unresolved. + A dependent PR can be published with its base and blockers explicit; merge-readiness follows + `docs/agents/pull-requests.md`. - When closing an umbrella issue, verify child issue states and the key implementation PRs instead of relying only on checked boxes. -When a skill says "publish to the issue tracker", create a GitHub issue. +For authorized issue publication, use GitHub issues. A skill workflow alone is not authorization +to publish. diff --git a/docs/agents/pull-requests.md b/docs/agents/pull-requests.md index d432e99f1..40594c21b 100644 --- a/docs/agents/pull-requests.md +++ b/docs/agents/pull-requests.md @@ -1,121 +1,83 @@ # Pull Requests -## 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, 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 fuzz-worker` 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 — - 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 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. -- 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. +## Publication scope + +A request to open or ship a PR includes branch creation, commits, push, and PR creation after +validation; merging and releasing require separate authorization. Apply authorization already given. +If repository or skill guidance still blocks an authorized step, cite the exact instruction and the +blocked action. + +## Readiness and validation + +Use `docs/agents/testing.md` to select checks and establish regression evidence. Before pushing, +run `pnpm check:affected --run` successfully on the commit being pushed. A serialized gate stage may +run it on the pushed head instead, but its result must be in the PR body before publication is +reported. If a gate fails, diagnose it and rerun after the cause is resolved; report unresolved +failures without claiming validation passed. + +- **Published**: branch pushed, PR opened, and validation recorded at a named commit. +- **Merge-ready**: required checks pass on the actual head, with live evidence for device-facing + changes. Docs-only and pure-tooling changes do not need device runs. Reporting publication does + not require waiting for CI. + +A local unit run does not establish provider integration or coverage. When those checks are +selected, verify the **Integration Tests** and **Coverage** jobs on the PR head. Run +`pnpm check:fallow --base origin/main` when code-quality or dead-code risk warrants it. + +For device-facing changes, fixtures do not replace a live run of the changed path. If blocked, +record the command and device needed and the remaining risk. Close manual sessions using +`docs/agents/device-verification.md` and report incomplete cleanup. + +Gross diff budget: 1,000 lines by `git diff --stat origin/main...HEAD`. Rename-only move PRs titled +`refactor(move)` are exempt when `git diff -M90% --stat origin/main...HEAD` proves no material +content change. ## 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): `. -Reviewers read that commit as enforcement: it decides whether the change is gated at all, never -something to skim past. +Use conventional commit prefixes; no `[codex]` tags. Implementation commits come first. Enforcement +edits — pins, baselines, ownership tables, exports maps, `.fallowrc`, gate manifests — go in one +final commit titled `chore(gates): `. ## 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: +Rebase for conflicts or when upstream changes affect a dependency or gate relevant to the PR. +Inspect what changed with: ```sh -pnpm check:affected --base --head origin/main # what main gained, by gate +pnpm check:affected --base --head origin/main ``` -If that plan names only files and gates disjoint from yours, the rebase buys nothing but another -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. +Disjoint upstream changes alone do not require a rebase. After rebasing, follow the same validation +lifecycle on the new head; retain earlier evidence with its original commit attribution. ## 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. 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`: 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. +Ready-for-review by default; draft when requested or intentionally incomplete. Keep the body under +250 words using these sections: + +- `## Summary`: changed behavior and why. Include 1-3 CLI/Node/MCP examples for public API changes + and `Closes #123` when applicable. Note the touched-file count and any expansion of scope. +- `## Validation`: tested commit SHA, relevant results, CI status, and unresolved risks. Name exact + commands when needed to reproduce evidence or explain a limitation. Use remote-accessible evidence, + not local paths. For docs-only changes, explain why runtime validation does not apply. ## 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 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. -- 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 - 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. -- 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. -- 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. +- Read linked issue dependencies and relevant ADRs. Resolve prerequisite/base conflicts before + implementation details. An ADR conflict needs an explicit update or superseding decision. +- For routing or command-surface changes, trace the production path through daemon and backend; + helper tests that bypass the router do not prove that path. Check CLI, Node.js, daemon, MCP, + help, and docs where the surface is affected. +- A regression attributed to an earlier PR belongs at the seam that PR missed. For typed-error + changes, trace producers through normalization and transport and inspect sibling consumers; + message matching must not replace a lost reason code. +- For recurring failures, check whether the fix belongs in the owning type, registry, or construction + path instead of another custom guard. Regression proof belongs in `docs/agents/testing.md`. +- Check interaction responses for compact defaults, bounded JSON arrays, and artifact paths for + large evidence; preserve existing warnings and typed error details. +- Use the CI Size workflow for size evidence; local comparisons are not required by default. + At roughly 700 net production lines (excluding tests, generated data, fixtures, and docs) or + more than 3 kB npm unpacked growth, obtain an independent review of whether a smaller owning + interface or deletion of superseded code would suffice. Consider gross churn for move-heavy + changes. These are investigation thresholds, not automatic rejection; explain justified growth + and why a smaller design was rejected. diff --git a/docs/agents/testing.md b/docs/agents/testing.md index af6d9a813..79bf6128d 100644 --- a/docs/agents/testing.md +++ b/docs/agents/testing.md @@ -1,7 +1,6 @@ # Testing Notes -Repository-specific testing traps you cannot learn from the test runner alone. Executable gate -ownership lives in `scripts/check-affected/` and `scripts/gate/`. +Gate ownership lives in `scripts/check-affected/` and `scripts/gate/`. ## Which gates a change needs @@ -22,8 +21,9 @@ pnpm check:affected --json pnpm gate --help ``` -`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. +`check:affected --run` reports coverage obligations without instrumenting; when related tests are +selected, it runs one capped `vitest related` command. Run local coverage to investigate a specific +coverage question or CI failure. Two selection traps recur: @@ -32,8 +32,9 @@ Two selection traps recur: - 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 showing their failure direction. +Docs-only changes with no runtime behavior impact need no runtime tests or new tests asserting prose. +Keep required gates; after those and focused checks pass, repeat or broaden only for changes, +failures, or unresolved risks. ## Platform and live-device policy @@ -45,7 +46,7 @@ Apple runner changes run `pnpm check:xctest-selection` and build the affected ta `#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 may need signing and automation permission: - System policy may refuse the unsigned bundle (`library load disallowed by system policy`, shown as `Early unexpected exit … crashed with signal kill`). Rebuild signed: @@ -65,10 +66,9 @@ Read the entry file before running a lane. Do not copy its environment matrix he ## Shared test utilities 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. +that defines them (`session-factories.ts`, `device-fixtures.ts`, `store-factory.ts`); avoid importing +unrelated helpers through a barrel. 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 run and removes it after every worker exits — skip per-test cleanup. An interrupted run may leave a @@ -86,9 +86,9 @@ signalled directly. ## Regression evidence -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. +Observe a regression test fail without the fix, then pass with it. For new or changed structural +gates, plant a violation and verify the intended gate names it. Pure test moves retain their tests; +verify discovery at the new path. If selection rules change, plant a failure to prove selection. A callback-based canary must observe semantic success, not just lifecycle completion — e.g. React Native Gesture Handler's @@ -131,9 +131,8 @@ 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, 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. +Use its dependent, command, and guarantee-cell report to scope inspection. `pnpm check:affected --run` +selects gates independently from the diff; it does not consume the depgraph report. ## Gate ownership @@ -166,17 +165,18 @@ schemas, `contracts/fixtures/` tables) are unaffected. ## Speed rules +Changing timeout failures that pass alone may be host contention. Reproduce on `origin/main` under +the same load before classifying them as regressions. + - 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 - 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 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 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; disabling isolation and changing pools did not + improve measured performance. Import 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=` (clamped to host CPUs, ignored in CI).