diff --git a/AGENTS.md b/AGENTS.md index 03bb415153..171c0b3734 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -420,8 +420,93 @@ PRs should include: - Run `bun test` before requesting review - Flag follow-up work with TODOs linked to issues +## Ship it review-ready — the first-pass checklist + +Auditing ~300 merged PRs shows a consistent shape: a first draft ships the **happy +path**, then a **hardening pass** (frequently a second person — the author who +vibecoded it opens, an engineer takes over the branch, adds commits, and merges) +adds the *same categories of change* every time. Those categories are below. Do +them in the **first** PR — they are the difference between "works in the demo" and +"survives production." Each item cites a real PR. + +1. **Handle the variants, not just the happy path.** Cover empty / null / + whitespace / duplicate / oversized inputs and every schema shape, not the one + in front of you. *(sections-editor #4008 added `@hide`, Lazy-wrapped, and + blank-title cases the first pass skipped; storage #4426 had to measure payload + size **before** `JSON.stringify`, not after; sandbox #4445 added pagination + + filename-collision disambiguation + stale-file pruning to a catalog writer.)* + +2. **Scope by tenant and permission.** Reads/writes are scoped to the current + user/org; other people's data is **read-only unless owned**. A validation or + dedup gate must inspect the **complete** payload, not one representative slice. + Never reuse a cache/list/React key across two shapes, and never conflate ids. + *(#4230 fix: teammates' threads must be read-only in the "All" view; #4416: the + publish gate had to union the committed **and** working-tree diffs; #4373: list + keys collided across skill/prompt of the same name.)* + +3. **Get concurrency right — no silent data loss.** For any "start B while A is in + flight" path, trace what happens to A's output and B's input under + concurrency-1 / a latch / a retry. Make side-effecting steps **non-retriable + unless idempotent**, claim fences/slots at **dispatch**, not at request time, + and coalesce fire-and-forget writes that share a path. *(#4365: a fence claimed + at POST time silently dropped an in-flight run's reply; #4409: a retriable + agent-loop step re-ran 3× and spliced generations; #4445: per-run catalog + re-sync raced itself.)* + +4. **Test each behavior you touch — in the right tier** (see [Testing](#testing)). + Add a test *per fix*. When you fix a bug, find the test that encodes the **old** + behavior and **invert** it — don't just append a new one. `grep` **all** tiers + (unit **and** `packages/e2e`) for any string or wire/storage contract you + changed. Storage changes need a real-Postgres test (in-memory fakes accept + columns the `update()` whitelist silently drops). E2E must not depend on a model + tier/provider absent in the test org. *(#4008 shipped a test with each fix; + #4446/#4430 had to invert tests that asserted the bug; #4350's e2e still + asserted pre-redesign copy; #4355 needed real-PG; #4365's e2e depended on a + provider tier.)* + +5. **Leave no dead code.** After you change who calls a symbol, narrow its export + to module-private and delete the newly-orphaned helpers/components/branches in + the **same** PR. Run `knip` before declaring done (see Gotcha #6). *(#4230 + deleted 297 lines of components the refactor orphaned; #4449, #4350, #4373 each + shipped a follow-up un-exporting a now-internal symbol knip flagged.)* + +6. **Complete the lifecycle and reset state.** New persisted state ships + **create + update + delete together**; make the writer idempotent (clear-then-write + for index/slug-named files); clean it up when the parent is deleted. A + "start/reset" transition must clear the previous cycle's terminal columns. + *(#4449 implemented create only — update ignored the field, delete orphaned the + subtree; #4355 left a stale `failure_reason` because `RUN_STARTED` didn't null + it.)* + +7. **Make risky and infra changes reversible and bounded.** Any change on a + boot/install/dispatch hot path gets its **own** default-off flag — never + piggyback on a neighbor's flag, and never let "deployed" mean "enabled." New + caches/artifacts need eviction (TTL + cap), bad-entry invalidation (publish only + **after** a health signal), and `.git/info/exclude` so they don't leak onto user + branches. Prefer the idle reaper to a fixed wall-clock timeout; emit heartbeats + during silent phases. *(#4357: golden cache shipped dormant behind + `GOLDEN_CACHE_ENABLED`, with GC and health-gated publish; #4445 git-excluded its + artifact; #4355 dropped a fixed timeout for progress-based reaping; #4409 added + heartbeats.)* Overstating a safety property in a comment/doc is itself a bug + (#4357, #4363). + +8. **Validate external input; don't over-engineer.** Validate URLs and user input. + Prefer schema-driven behavior to clever runtime inference — an engineer reverted + exactly that "infer from runtime data" cleverness in #4008. Use design-system + tokens, not raw palette (#4350: `text-emerald-600` → `text-success`), and run + `bun run fmt` before the first push (#4461 was a pure-format follow-up). + +9. **Keep type-safety at compile time.** Don't `as`-cast to read a field off a + union — narrow with an `in` / discriminant check so a rename is a **compile** + error, not a runtime regression only e2e catches (#4365). Respect + `noUncheckedIndexedAccess`. + ## Common Gotchas +> The [first-pass checklist](#ship-it-review-ready--the-first-pass-checklist) above +> captures the review-driven gotchas mined from PR history. The list below is the +> always-load-bearing set. + 1. **Never access environment variables directly in tools**—use StudioContext 2. **Never access HTTP context in tools**—use StudioContext for all state 3. **Database migrations**: Remember to run both Kysely migrations (`bun run migrate`) and Better Auth migrations (`bun run better-auth:migrate`)