diff --git a/AGENTS.md b/AGENTS.md index be6f88bf..eabde791 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -26,12 +26,52 @@ Rules closer to the code take precedence. Before editing `.github/`, `src/`, `de - Make narrow, owned diffs. Every changed line must trace to the request, a failing test, or a verified compatibility constraint. - Prefer existing utilities, stores, services, and test harnesses. Do not add dependencies or speculative abstractions unless the task requires them. -- Production changes under `src/`, `desktop/src/`, or `adapters/` require a same-area regression test unless a maintainer explicitly approves an exception. +- Production changes under `src/`, `desktop/src/`, or `adapters/` require a same-area regression test unless a maintainer explicitly approves an exception. A test that only covers the hop you just changed satisfies this rule and still lets the next change break — see "Writing a test that holds" below. - Keep TypeScript ESM style: 2-space indentation, no semicolons, `PascalCase` components, and `camelCase` functions/hooks/stores. - Use structured parsers and existing boundaries instead of ad hoc string manipulation. Add comments only for non-obvious control flow or external constraints. - Do not commit generated output such as `artifacts/`, coverage reports, `node_modules/`, build directories, or Rust `target/` trees. - When publishing is explicitly requested, use Conventional Commit subjects and normal product branch prefixes such as `fix/`, `feat/`, or `docs/`; do not create `codex/` branches in this repository. +## Writing a Test That Holds + +Most regressions here are repairs of a recent repair: 21 of the last 70 `fix` commits +edit lines another `fix` wrote within 30 days. Coverage is not the missing signal — +`ContextUsageIndicator.tsx` sits at 87% branch coverage and was fixed three times in +ninety minutes. What those tests had in common is shape, so choose it deliberately. + +- **Drive the transition; never hand-write the state it produces.** Component tests in + `desktop/src` call `setState` 744 times and a real store action 3 times. State you + assigned is self-consistent by construction and cannot expose "transition A did not + update B" — which is where these bugs live. Use `handleServerMessage`, store actions, + and real user events. +- **Assert the invariant, not today's output.** `2262973a4` shipped + `expect(getByText('deepseek-reasoner'))` at a moment when the screen showed another + model's number: it wrote the bug in as a passing assertion, and the next fix had to + invert that exact line. Ask what must be true after this step, not what it prints now. +- **Cover both directions of any rule that drops or merges something.** The replay guard + was tested for "a replay must be discarded" and never for "a genuine repeat must be + kept", so it shipped dropping real replies. +- **Test the join, not each end.** Server, store, and component each had a test for + `runtime_config_applied`; nothing crossed them, and deleting the term that joins them + (`ChatInput.tsx` `refreshNonce`) left 314 tests green. +- **Never retune an existing test's inputs to keep it green.** `128f75ab5` changed five + tests' props (`messageCount={0}` → `{1}`) instead of accepting that they described + states a real session cannot reach. If a test only passes after you edit its inputs, + the test was describing the implementation. +- **Do not mock the module under test.** A hand-written factory freezes an interface + snapshot: the store can be renamed or gutted and the test still passes. +- **If you are comparing content to decide identity, the identity exists upstream.** + Deduping by text cannot separate a replay from a legitimate repeat; forward the id + (`uuid`, `toolUseId`) instead of guessing. + +Blind spots to check rather than trust: + +- `desktop/electron/` is not instrumented at all (`vitest.config.ts` collects only + `desktop/src`), so main-process diffs score zero covered lines. +- Bun's LCOV emits no branch records, so `src/` and `adapters/` report **100% branch + coverage** for data that was never collected (`pct(0, 0) === 100`). Only `desktop/` + has real branch numbers. + ## Verification 1. Run the narrowest relevant test while iterating. diff --git a/desktop/AGENTS.md b/desktop/AGENTS.md index bacd3161..aa277538 100644 --- a/desktop/AGENTS.md +++ b/desktop/AGENTS.md @@ -4,6 +4,9 @@ These rules apply to `desktop/` changes in addition to the root instructions. - Before adding or editing anything under `desktop/src/components/`, read `desktop/src/components/AGENTS.md`. It is the authoritative index of reusable components, the placement rules for new ones, and the required style/i18n/a11y/test conventions. Do not add a component that duplicates one listed there, and do not add new files to `components/shared/` or `components/common/`. - Reuse the existing desktop store/API patterns. Use `lucide-react` for common icons and keep operational UI dense, stable, and readable. +- A new feature panel is its own module from the start. `Settings.tsx` reached 4639 lines holding seven unrelated panels before it was split into `pages/settings/*`; the four most-repeatedly-fixed files in the repository are also its four largest. Put a panel in its own file, and put anything two panels share in an explicit shared module rather than leaving it in the page that happens to host both. +- Wire the component into its route in the same change that creates it. `src/__tests__/componentReachability.test.ts` fails on any `.tsx` no entry point can reach. Three components once lost their last import, and the coverage gate read "zero coverage" as "needs a test" — someone wrote suites for two of them, and a UI redesign then restyled all three. +- Every translation key a component uses must be added to all five files in `src/i18n/locales/`, including keys chosen inside an expression (`t(count === 1 ? 'a' : 'b')`) — a literal-only scan misses those in both directions. - Add focused Vitest or Testing Library coverage for UI, store, or API behavior. Run it first, then follow `bun run check:impact`; desktop product changes normally select `bun run check:desktop`. - Chat transport, WebSocket lifecycle, first-turn runtime selection, reconnect, or session changes also require the offline `bun run check:chat-contract` when selected, plus `bun run check:agent-flow` for the end-to-end session/tool/permission/reconnect protocol. - Permission dialog, tool-call rendering, or approval-flow changes should also run `bun run check:desktop-ui-smoke`: it exercises the real dialog in a real browser against the mock runtime, with no provider.