callstack__agent-device/docs/agents/pull-requests.md

# 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, and no conflict markers or unmerged
  paths remain.
- A local unit-only run is not CI-green. Use `pnpm test:unit` for the repo unit bundle, or
  `vitest run --project unit-core --project subprocess-stub` when invoking Vitest directly. The
  **Integration Tests** and **Coverage** jobs run the `provider-integration` project — verify those
  green on the actual PR head.
- Device-facing behavior is not merge-ready without real simulator/emulator/device evidence for the
  changed path. Fixture-backed tests prove contracts; they do not replace a live run that creates or
  observes the artifact/state the feature claims to handle. If live verification is blocked, state
  the blocker and the exact command/device needed, and downgrade the PR to residual risk rather than
  calling it ready.
- Command-surface changes preserve CLI, Node.js, daemon, MCP, help, and docs coverage
  where that surface is affected, without duplicating command contracts across layers.
- Runtime output stays agent-friendly: compact defaults, top offenders first for diagnostics/perf,
  bounded arrays in JSON, artifact paths for large raw data, progressive lookup for deeper detail.
- Close every manual `agent-device` session opened during verification
  (`docs/agents/device-verification.md`) and report any cleanup that could not be completed.
- Two readiness claims, never blurred: **published and reported** means the branch is pushed, the
  PR body carries the evidence gathered at a named commit, and CI on the head is the authority
  still to come; **merge-ready** means the required checks are green on the actual head and,
  where the change touches a device-facing path, the live evidence for that path exists (the
  device-facing bullet above; docs-only and pure-tooling changes owe none). "Don't wait for CI"
  licenses the first, not the second — say which one you are claiming.

## 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:

```sh
pnpm check:affected --base <your-merge-base> --head origin/main   # what main gained, by gate
```

If that plan names only files and gates disjoint from yours, the rebase buys nothing but another
full validation cycle. Evidence in the PR body is stamped with the commit it was gathered at, so a
rebase dates it rather than invalidating it, and CI on the new head re-establishes it. A merge
queue is the answer once independent migration units regularly land against each other; until
then this rule is.

## 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.

- `## Summary`: user/API behavior, not an implementation file tour. Lead with what changed for
  operators, clients, command authors, or platform behavior. A compact before/after helps when it
  clarifies the workflow or bug fix. For new or changed public APIs, include 1-3 concrete CLI/Node/MCP
  examples a reviewer can scan. `Closes #123` when applicable.
- `## Validation`: meaningful evidence in concise prose — scenario names, manual device/browser
  evidence, changed screenshots, CI status, notable failures/retries and their outcome. Avoid command
  accounting for routine local gates; name an exact command only when it is unusual, manually
  reproducible evidence, or needed to explain a residual risk. For docs-only changes, say why runtime
  validation does not apply instead of writing a command checklist.
- Call out real tradeoffs, known gaps, and follow-ups; omit boilerplate when there are none.
- Note touched-file count and whether scope expanded beyond the initial command family.
## Reviewing

- Review against the linked issue, not only the diff. State the issue's motivating behavior and
  verify the PR fixes *that*.
- Check relevant ADRs before reviewing architecture, routing, command-surface, platform-boundary,
  diagnostics, or testing-strategy changes. An ADR conflict is a review finding unless the PR updates
  or supersedes the ADR explicitly.
- Read dependency notes (`Blocked by: ...`, linked PRs, sibling branches) before judging correctness.
  A base/sequence problem outranks detail review.
- Trace the real production route from command surface through daemon/request routing to the platform
  backend. Tests that mock away the router, or exercise only a helper, do not prove the shipped path.
- Before adding an error classifier, trace every producer through normalization, wrapping,
  serialization, and transport; inventory sibling consumers and the existing reason-code vocabulary;
  then repair the deepest shared boundary that loses the signal. Message text is not a reason code.
- For each key regression test, identify what deletion or revert would make it fail. If reverting the
  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.
- Treat the CI Size workflow as review evidence; local size comparisons are not required by default.
  Escalate scrutiny when a PR adds roughly 700 or more net production lines (excluding tests,
  generated data, fixtures, and documentation) or increases npm unpacked size by more than 3 kB.
  Consider gross additions and deletions too, so a move-dominated change is not mistaken for pure
  growth. These thresholds trigger investigation, not automatic rejection: ask an independent
  reviewer whether a deeper owning interface, stronger types, less ceremony, reuse of an existing
  construction path, or deletion of superseded code can make the change materially smaller. The PR
  should itemize justified growth and record why a smaller design was rejected.