|
| 1 | +# Pull Requests |
| 2 | + |
| 3 | +## Readiness |
| 4 | + |
| 5 | +- Static gates first: required checks pass, `pnpm check:fallow --base origin/main` is clean when |
| 6 | + code-quality/dead-code risk is relevant, CI guards are green, and no conflict markers or unmerged |
| 7 | + paths remain. |
| 8 | +- A local unit-only run is not CI-green. Use `pnpm test:unit` for the repo unit bundle, or |
| 9 | + `vitest run --project unit-core --project subprocess-stub` when invoking Vitest directly. The |
| 10 | + **Integration Tests** and **Coverage** jobs run the `provider-integration` project — verify those |
| 11 | + green on the actual PR head. |
| 12 | +- Device-facing behavior is not merge-ready without real simulator/emulator/device evidence for the |
| 13 | + changed path. Fixture-backed tests prove contracts; they do not replace a live run that creates or |
| 14 | + observes the artifact/state the feature claims to handle. If live verification is blocked, state |
| 15 | + the blocker and the exact command/device needed, and downgrade the PR to residual risk rather than |
| 16 | + calling it ready. |
| 17 | +- Command-surface changes preserve CLI, Node.js, daemon, MCP, help, docs, and SkillGym coverage |
| 18 | + where that surface is affected, without duplicating command contracts across layers. |
| 19 | +- Runtime output stays agent-friendly: compact defaults, top offenders first for diagnostics/perf, |
| 20 | + bounded arrays in JSON, artifact paths for large raw data, progressive lookup for deeper detail. |
| 21 | +- Close every manual `agent-device` session opened during verification |
| 22 | + (`docs/agents/device-verification.md`) and report any cleanup that could not be completed. |
| 23 | + |
| 24 | +## PR body |
| 25 | + |
| 26 | +Conventional commit prefixes (`feat:`, `fix:`, `chore:`, `perf:`, `refactor:`, `docs:`, `test:`, |
| 27 | +`build:`, `ci:`). No bracketed bot tags like `[codex]`. Ready-for-review by default; draft only when |
| 28 | +asked or when the work is intentionally incomplete. |
| 29 | + |
| 30 | +- `## Summary`: user/API behavior, not an implementation file tour. Lead with what changed for |
| 31 | + operators, clients, command authors, or platform behavior. A compact before/after helps when it |
| 32 | + clarifies the workflow or bug fix. For new or changed public APIs, include 1-3 concrete CLI/Node/MCP |
| 33 | + examples a reviewer can scan. `Closes #123` when applicable. |
| 34 | +- `## Validation`: meaningful evidence in concise prose — scenario names, manual device/browser |
| 35 | + evidence, changed screenshots, CI status, notable failures/retries and their outcome. Avoid command |
| 36 | + accounting for routine local gates; name an exact command only when it is unusual, manually |
| 37 | + reproducible evidence, or needed to explain a residual risk. For docs-only changes, say why runtime |
| 38 | + validation does not apply instead of writing a command checklist. |
| 39 | +- Call out real tradeoffs, known gaps, and follow-ups; omit boilerplate when there are none. |
| 40 | +- Note touched-file count and whether scope expanded beyond the initial command family. |
| 41 | + |
| 42 | +## Reviewing |
| 43 | + |
| 44 | +- Review against the linked issue, not only the diff. State the issue's motivating behavior and |
| 45 | + verify the PR fixes *that*. |
| 46 | +- Check relevant ADRs before reviewing architecture, routing, command-surface, platform-boundary, |
| 47 | + diagnostics, or testing-strategy changes. An ADR conflict is a review finding unless the PR updates |
| 48 | + or supersedes the ADR explicitly. |
| 49 | +- Read dependency notes (`Blocked by: ...`, linked PRs, sibling branches) before judging correctness. |
| 50 | + A base/sequence problem outranks detail review. |
| 51 | +- Trace the real production route from command surface through daemon/request routing to the platform |
| 52 | + backend. Tests that mock away the router, or exercise only a helper, do not prove the shipped path. |
| 53 | +- For each key regression test, identify what deletion or revert would make it fail. If reverting the |
| 54 | + implementation still passes, the test is vacuous. |
| 55 | +- Check for hidden behavior changes separately from intended refactors: output shape, |
| 56 | + warning/error propagation, artifact paths, fallback/retry tiers. |
| 57 | +- Verify tests cover the issue's motivating failure, not just the new abstraction. Prefer |
| 58 | + before/after evidence when an issue reports a concrete divergence. |
| 59 | +- Green CI is necessary but insufficient for device-facing or routing-sensitive work. |
| 60 | +- Check whether the tightening pass removed code/tests the change made obsolete. |
0 commit comments