From 18c50ac320b7e4bfc5c3b2ee7dd0c0647cd414d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sat, 5 Sep 2026 09:04:53 +0200 Subject: [PATCH 1/2] docs: clarify agent task scope and validation --- AGENTS.md | 44 ++++++++++++++++-------------- docs/agents/device-verification.md | 18 +++++++++++- docs/agents/pull-requests.md | 20 ++++++++++++-- docs/agents/testing.md | 9 ++++-- 4 files changed, 64 insertions(+), 27 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 3b77dcb13..6c5c595c5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -3,8 +3,25 @@ `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 execution + +- Complete implementation, review, validation, and publication within the requested scope. + Resolve routine choices from code and context; ask only when an answer changes scope, correctness, + or authorization. Continue independent work while awaiting an answer. +- User instructions take precedence over repository and skill guidance, subject to system and + developer constraints. Reuse approval already granted for the action. If guidance blocks progress, + link the exact file, quote the instruction, and explain what remains blocked and why. +- Incorporate corrections and answer side questions while preserving the active objective unless + the user cancels or replaces it. Preserve unrelated work in the checkout. +- When delegation is available and permitted, use it for independent investigation, implementation + with disjoint file ownership, or review when it improves the outcome. Assign bounded scopes and + expected evidence; verify results. Serialize full gates and device use. +- Lead updates and final responses with the outcome. Use concise, plain prose; include changed + behavior, relevant validation, and unresolved limits. Finish unblocked work. + +## Task routing + +Load only the procedures relevant to the task: | When the task involves | Read | | --- | --- | @@ -14,12 +31,12 @@ 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. +When running or changing CLI commands, use versioned help as the behavior reference: start with +`agent-device help workflow`, then the relevant topic help. ## Incident-derived principles @@ -103,30 +120,15 @@ 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. +- Select gates through `docs/agents/testing.md`; follow `docs/agents/pull-requests.md` before pushing. - 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`. diff --git a/docs/agents/device-verification.md b/docs/agents/device-verification.md index c502cc706..5c51f323c 100644 --- a/docs/agents/device-verification.md +++ b/docs/agents/device-verification.md @@ -1,6 +1,7 @@ # 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 @@ -28,6 +29,21 @@ Dev-loop staleness has three layers, and each produces a convincing false negati - 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. +## 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. + +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 manually opened session is a resource that must be closed — including exploratory sessions and diff --git a/docs/agents/pull-requests.md b/docs/agents/pull-requests.md index d432e99f1..674d2e7f8 100644 --- a/docs/agents/pull-requests.md +++ b/docs/agents/pull-requests.md @@ -1,5 +1,12 @@ # Pull Requests +## Publication scope + +A request to ship or open a PR authorizes creating a branch, committing, pushing, and opening the PR +once validation is complete. It does not authorize merging or releasing. Carry those authorized +steps through without asking again; a review-only request authorizes inspection, not edits or +publication. If publication is blocked, finish the reviewable local work and report the blocker. + ## Readiness - Static gates first: required checks pass, `pnpm check:fallow --base origin/main` is clean when @@ -32,9 +39,9 @@ ## 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 +Apply the red/green requirements in `docs/agents/testing.md` when changing behavior or enforcement. +Use focused checks and review appropriate to the change, then one **successful** +`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 @@ -119,3 +126,10 @@ when asked or when the work is intentionally incomplete. Keep the whole body at 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. + +## Guidance maintenance + +The task-execution and verification guidance applies +[OpenAI's GPT-6 Astra prompting recommendations](https://developers.openai.com/api/docs/guides/latest-model?model=gpt-6-astra#prompting-best-practices) +to this repository. Keep model-specific API settings out of contributor instructions; preserve the +repo's gate ownership, context budgets, and explicit publication scope when updating guidance. diff --git a/docs/agents/testing.md b/docs/agents/testing.md index af6d9a813..51ccbb734 100644 --- a/docs/agents/testing.md +++ b/docs/agents/testing.md @@ -32,8 +32,10 @@ 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. +New or changed structural gates need a planted violation proving their failure direction. ## Platform and live-device policy @@ -166,6 +168,9 @@ 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. From 501426464c587f4efb6ff6028ba4ffc34b99821c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sat, 5 Sep 2026 09:17:07 +0200 Subject: [PATCH 2/2] docs: remove redundant and conflicting agent guidance --- AGENTS.md | 80 +++++-------- docs/agents/cli-flags.md | 8 +- docs/agents/device-verification.md | 32 ++---- docs/agents/domain.md | 11 +- docs/agents/issue-tracker.md | 37 +++--- docs/agents/pull-requests.md | 178 ++++++++++------------------- docs/agents/testing.md | 35 +++--- 7 files changed, 134 insertions(+), 247 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 6c5c595c5..f5fde6759 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -3,22 +3,6 @@ `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. -## Task execution - -- Complete implementation, review, validation, and publication within the requested scope. - Resolve routine choices from code and context; ask only when an answer changes scope, correctness, - or authorization. Continue independent work while awaiting an answer. -- User instructions take precedence over repository and skill guidance, subject to system and - developer constraints. Reuse approval already granted for the action. If guidance blocks progress, - link the exact file, quote the instruction, and explain what remains blocked and why. -- Incorporate corrections and answer side questions while preserving the active objective unless - the user cancels or replaces it. Preserve unrelated work in the checkout. -- When delegation is available and permitted, use it for independent investigation, implementation - with disjoint file ownership, or review when it improves the outcome. Assign bounded scopes and - expected evidence; verify results. Serialize full gates and device use. -- Lead updates and final responses with the outcome. Use concise, plain prose; include changed - behavior, relevant validation, and unresolved limits. Finish unblocked work. - ## Task routing Load only the procedures relevant to the task: @@ -35,31 +19,24 @@ Load only the procedures relevant to the task: | 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` | -When running or changing CLI commands, use versioned help as the behavior reference: 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. @@ -83,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 @@ -95,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. @@ -110,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. @@ -120,9 +96,8 @@ 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. -- Run one full gate per host at a time. Subprocess-backed tests under concurrent worktrees produce - timeout-shaped contention failures. -- Select gates through `docs/agents/testing.md`; follow `docs/agents/pull-requests.md` before pushing. +- 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. @@ -152,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 5c51f323c..acaaa8c9a 100644 --- a/docs/agents/device-verification.md +++ b/docs/agents/device-verification.md @@ -3,19 +3,16 @@ 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 @@ -26,8 +23,8 @@ 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. ## Worktree ownership and runner diagnostics @@ -46,14 +43,10 @@ behavior, and xctestrun build/cache logic stays outside request execution. ## Session hygiene -Every manually opened session is a resource that must be closed — including exploratory sessions and -failed verification attempts. - -- 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 @@ -64,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 674d2e7f8..40594c21b 100644 --- a/docs/agents/pull-requests.md +++ b/docs/agents/pull-requests.md @@ -2,134 +2,82 @@ ## Publication scope -A request to ship or open a PR authorizes creating a branch, committing, pushing, and opening the PR -once validation is complete. It does not authorize merging or releasing. Carry those authorized -steps through without asking again; a review-only request authorizes inspection, not edits or -publication. If publication is blocked, finish the reviewable local work and report the blocker. - -## 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 - -Apply the red/green requirements in `docs/agents/testing.md` when changing behavior or enforcement. -Use focused checks and review appropriate to the change, then one **successful** -`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. +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. - -## Guidance maintenance - -The task-execution and verification guidance applies -[OpenAI's GPT-6 Astra prompting recommendations](https://developers.openai.com/api/docs/guides/latest-model?model=gpt-6-astra#prompting-best-practices) -to this repository. Keep model-specific API settings out of contributor instructions; preserve the -repo's gate ownership, context budgets, and explicit publication scope when updating guidance. +- 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 51ccbb734..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: @@ -35,7 +35,6 @@ Two selection traps recur: 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. -New or changed structural gates need a planted violation proving their failure direction. ## Platform and live-device policy @@ -47,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: @@ -67,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 @@ -88,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 @@ -133,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 @@ -174,14 +171,12 @@ 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).