PR Review Lessons (Dedalus)

Durable rules distilled from senior review feedback on Kevin's open PRs. Most code was Cursor/agent-generated and locally plausible but not verified against the real runtime, spec, or architecture. The reviewers (Windsor Nguyen, CTO; Shengming Liang, PM) kept enforcing the same things: one owner per job, one source of truth, no polling, tight types, and empirical verification at the surface the user hits.

The throughline: agent-written code passes lint and "looks done" but encodes accidental complexity (multiple state stores, polling, sparse metadata patches) and unverified claims (PR body that does not match the diff, config that never renders). The fix is almost always deletion and simplification, not more code.

Durable rules

1. One function, one job — webhook handlers stay thin. A handler mirrors identity first, then runs a few explicit side effects. It is not a state machine. If you cannot name a single ownership boundary, split it. Respect event semantics: a profile-sync event (user.updated) must not re-run signup provisioning or unban/resume. Windsor, PR #2093: "When this breaks, we cannot tell whether Clerk sync, abuse enforcement, billing state, or credit gating failed without reading the whole file."

2. One canonical source of truth — no split-brain state. Write the decision once, as a complete snapshot under one key. Never synchronize equivalent state across several stores (Clerk metadata + a DB attempt row + webhook polling + resume markers). Never write sparse nested patches — deep-merge preserves stale fields (an old radarScore). Windsor, PR #2093.

3. Know the external API's write semantics before writing through it. Clerk updateUser overwrites metadata; updateUserMetadata deep-merges and deletes keys set to null. Using the wrong one is the reconciliation bug. Read the contract (rtfm) instead of fetch/spread/write by hand. Windsor, PR #2093.

4. Polling for information is a design smell. Windsor, PR #2093: "if LLM-generated code is ever polling for information, something has (usually) gone terribly wrong with its design." Backoff loops that wait for another writer paper over ordering. Make one component the synchronous owner so downstream reads committed state. See No-Fallbacks Policy.

5. Fail closed from the one canonical value; weigh reversibility. Side effects (credit award, ban) decide from the single risk verdict and fail closed. Pick the reversible failure: "Accounts are cheap to keep; false bans and split-brain billing state are not." Windsor, PR #2093.

6. Typed optionality — no explicit | undefined, no loose unknown. Write peakValue?: number, not peakValue?: number | undefined (violates typescript-eslint(no-restricted-types)). undefined/unknown as a data type is almost always too loose — type it specifically. If a value truly can be undefined, hoist a named union (type Foo = SomePrimitive | undefined) at the top of the file: intentionally annoying so the type is a conscious choice. Windsor, PR #2297. See TypeScript.

7. The PR body must match the final diff; verify mechanism claims empirically. PR #2168 claimed a three-step .gitignore allowlist while the diff ignored .cursor/ as a directory — git will not recurse into an ignored parent for child negations, so the allowlist was broken, and the first "fix" still shipped .cursor/ not .cursor/*. Prove the mechanism (git check-ignore <path>, curl -L the real route) and make the description reflect what shipped. Windsor, PR #2168 (blocked twice). See Empirical Verification.

8. Verify at the surface the user hits, not the config layer. PR #2276 set exampleProjects but never containerExample, so the section returned null — "the test plan says there is a container example section, but /nytw never renders it." Short links 404'd under a real request (curl -L /byoa, Playwright page.goto) despite a correct-looking map. Test rendered DOM and real HTTP, not that config exists. Shengming, PR #2276.

9. "Looks done" is not "to spec" — check every range/branch. PR #2297's 7d view still rendered hourly bars; 30d repeated weekday labels with no calendar date. Walk each mode of the spec (24h hourly, 7d daily, 30d/90d weekly with real dates). Shengming, PR #2297.

Recurring correctness bugs (catch before the bot)

These were auto-flagged by Bugbot/Greptile across PRs. They are predictable agent mistakes — catch them in self-review:

• Async response races — rapid range switches let a stale fetch overwrite fresh state; guard with a sequence ref / AbortController and clear stale error on success. (#2094, #2297)
• Colliding global IDs — area-fill-${dataKey} collided across chart instances; use useId() for any DOM/SVG id. (#2297)
• Dead code shipped — unused exports, unused props (waitlistCampaign). Delete on the way through; code is debt. (#2093, #2276)
• Time-based idempotency keys — ..._login_${Date.now()} defeats Stripe retry dedup; keys are stable per logical operation, unique across operations. (#2093)
• Unbounded exponential backoff — delay *= 2 blew the timeout budget; cap with Math.min(delay*factor, MAX) (better: don't poll — rule 4). (#2093)
• Lying empty states — "No usage recorded yet" showed when machines existed but were filtered inactive; distinguish "no data" from "all filtered out". (#2062)

Where this is enforced

• Global Cursor rule: config/cursor/rules/pr-review-lessons.mdc (always-on, all harnesses).
• Dedalus harness: config/dedalus/agents/rules/{review,typescript,react}.md, config/dedalus/agents/agents/code-reviewer.md, config/dedalus/cursor-rules/pre-ship-checklist.mdc.
• Canonical style: TypeScript (optionality), React (async/ids/dead code).
• Skill: skills/engineering/pr-review/SKILL.md (recurring-theme checklist).

Concept Position

---

Timeline

• 2026-07-01 | Concepts category refresh added this page to the Agent harness and runtime primitives family, linked it to Concept System Map, and kept it standalone because it owns this reusable mental model: Durable rules distilled from senior review feedback on Kevin's open PRs. Most code was. Source: User request, 2026-07-01
• 2026-06-03 | Compiled from open PRs 2062, 2093, 2094, 2168, 2276, 2297, 2300. Heaviest signal: Windsor on #2093 (one source of truth, thin handlers, no polling, fail-closed) and #2297 (typed optionality), and the twice-blocked .gitignore/PR-body mismatch on #2168. Shengming on #2276/#2297 (verify rendered surface, match the spec). Source: GitHub PR reviews + issue comments, dedalus-labs/dedalus