diff --git a/AGENTS.md b/AGENTS.md index e229098..e33a466 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -23,6 +23,9 @@ ## Commit and PR conventions +Before opening a PR, check the change against `docs/review-patterns.md` and run +`deno task fmt`. + Do not include any agent marketing material (e.g. "Generated with...", "Co-Authored-By: \") in commits, pull requests, issues, or comments. diff --git a/docs/review-patterns.md b/docs/review-patterns.md new file mode 100644 index 0000000..ed2a45d --- /dev/null +++ b/docs/review-patterns.md @@ -0,0 +1,100 @@ +# Review patterns + +Recurring maintainer review feedback, distilled from past PRs. Check a change +against this list before opening a PR. PR numbers point at the original +discussion. + +## Specs + +- Specs describe what _is_. No "current", "earlier", "now extends", or "planned + transition" language. Version-to-version differences go in commits and PR + descriptions (#29). +- A code change that touches spec'd behavior updates the spec in the same PR, + including stale parentheticals elsewhere in the doc (#42). +- Specify edge-case policy (timeouts, capacity exhaustion, overflow) before + writing tests that prescribe an outcome (#85, #132). +- Use the spec's vocabulary: directives (`open`/`close`/`text`), not + "descriptors" (#5). +- Don't externalize internal helpers into the spec just to unit-test them. A + `Term` does no IO, so test through the public API (#130). +- Spec'd C functions state how errors are represented (#130). Reference links + are complete URLs, not bare domains (#130). +- Facts that will never change belong in the normative section, not in + implementation notes (#5). + +## Native first + +- Renderer state lives in C/WASM. Store it once in native state; don't copy it + across the boundary on every call (#133, #139). +- Prefer encoding new declarations in the binary format and resolving them + during rendering, as `text({ caret })` does, over re-scanning ops in + TypeScript. A snapshot stays a snapshot (#137). +- Logic that any host language would need belongs in C, so non-JS frameworks get + the same behavior (#116). +- Follow Clay's intended mechanisms. Don't build a parallel code path to an + algorithm Clay already owns (e.g. text measurement) (#30). Link upstream Clay + issues for local workarounds so they can be removed later (#85). + +## API shape + +- String literals over numeric constants or TS enums (`"center-center"`, not + `ATTACH_POINT.CENTER_CENTER`). Use functions when a value is parameterized + (#29, #42). +- Collapse type variants: one event union, one array-taking signature, no + overloads that only tests use (#133). +- Names carry constraints: `id` when it must be unique, not `name` (#5). +- Static facts are fixed at `createTerm()`; dynamic changes arrive as events via + `term.update()`, which returns the bytes to write. Don't defer work to the + next `render()` (#106). +- Front-load configuration at creation so each paint stays one + `write(result.output)` (#101). + +## C and WASM correctness + +- Don't record side effects for operations that may no-op. `setcell()` returns + early outside the clip rect (#114). +- When growing WASM memory, account for every region (parser state, transfer + buffer, tables). Keep a comment describing the memory layout (#132). +- Check return values (e.g. `trie_add()` exhaustion) (#132). +- On overflow, keep structures balanced and report through the error channel. + Never silently corrupt or truncate; warn once if it's user-visible (#85, + #114). +- Never fabricate a default for unknown input: an unknown capability code must + not become a plausible-looking event (#132). +- Preserve signals in public functions (`wcwidth()` returns `-1` for + unprintable) (#49). +- Prefix C constants and put them in the header (`TERMINFO_*` in `terminfo.h`) + unless they mirror an external name (#131). + +## Tests + +- Assert layout as ASCII pictures: + `expect(trim(print(ansi, w, h))).toEqual(...)` (see `test/print.ts`). Extend + `print()` rather than asserting raw bytes (#85, #114). +- One spec per test file; put tests in the matching file (e.g. color behavior in + `test/color.test.ts`) (#93). +- Build input with named helpers (`arrowUp()`, `mousePress()`) instead of byte + arrays annotated by comments (#91). +- Keep fixtures minimal; drop wrappers the assertion doesn't need (#87). +- Parsers need adversarial coverage: incomplete and invalid input, every + byte-boundary split, conflicting replies in either order, capacity exhaustion, + numeric overflow, and recovery of the following key (#132). + +## Performance + +- Split unambiguous wins from trade-offs into separate PRs. Size-vs-speed + changes (`-Oz`, compression) need a benchmark showing the cost (#35, #50). +- Benchmark only APIs that matter: `createTerm()`, `render()`, `createInput()`. + Startup belongs in a fresh process (#52). +- Distrust regressions in benches the diff can't reach; investigate noise before + reacting (#56, #85, #102). + +## Hygiene + +- Run `deno task fmt` before opening a PR (#6, #28, #102). +- Add a changeset for user-facing changes (#114). +- Commits must be signed (#42). Follow the org PR template (#29). +- Don't add build workarounds (PATH hacks, shims) for local-only environment + issues when CI builds clean (#6, #32). +- Examples: `main()` at the top, helpers below (#48). Annotate raw escape + sequences (`// SGR reset`) (#48).