Pr Review

PR Review Workflow

Overview

End-to-end PR review: understand business context → validate business flow → evaluate approach → check code → deduplicate with ROI filter → post inline comments to GitHub.

Step 0: Business Context Gate

Before reading any code, establish the business context. A PR review without understanding the business problem is guesswork.

1. Read the PR description + linked Jira/task spec. Can you answer these three questions?

   • What business problem does this solve? (What was broken, missing, or inefficient?)
   • Who is affected? (End users, operators, internal team, on-call?)
   • What does "done" look like? (Acceptance criteria, expected user experience change)

2. If the business context is unclear, stop. Flag as P1 blocker and request clarification before proceeding to code review. A vague or absent PR description is the first red flag — not a minor issue to note at the end.

3. Extract acceptance criteria to validate against later. When evaluating the code, every behavioral change should map back to a stated business goal.

This is the highest-value step. A PR that solves the wrong business problem or implements an incorrect business flow needs to be caught here, not after 30 inline comments on code style.

Step 1: Gather Context

Collect all inputs in parallel:

1. gh pr view <number> --json state,title,commits,reviews,comments,updatedAt
2. gh api repos/{owner}/{repo}/pulls/{number}/comments  (existing inline comments)
3. gh pr diff <number>                                   (full diff)
4. Read project rules and planning docs (see below)

Read Project Rules (mandatory, before analyzing any code)

Always read AGENTS.md first — it is the single source of truth for all non-lintable constraints. It defines:

• Core principles (K.I.S.S., minimalistic, clean, elegant, best practice)
• Documentation hierarchy (agents.md > planning/ > features/ > implementation/)
• Coding standards, development principles, and all mandatory rules
• API architecture, database conventions, frontend rules, i18n, testing

Then read planning docs based on which areas the PR touches:

For DS compliance checks: also inspect node_modules/@aetheronhq/ui/dist/index.d.ts exports and node_modules/@aetheronhq/ui/dist/styles.css to know what components and CSS utility classes the DS provides.

Key data to extract

• PR title, state, commit list
• All existing reviewer comments (who said what, on which file:line)
• Author's response/triage comment (often marks issues as "pre-existing" or "addressed")

Step 2: Validate Business Flow

Before diving into code quality, validate that the PR implements the right business flow.

1. Map the PR to the user journey. Trace the change through: user action → system behavior → outcome. Does the flow make sense from the user's perspective? Is there a simpler path to the same business goal?

2. Check for business flow correctness. Business flow issues have the highest impact — they can require a complete rewrite. Watch for:

• Blind spots — even experienced engineers can miss edge cases in flows they rarely touch
• Narrow thinking — the happy path works, but what about cancellations, partial failures, retries, or concurrent users?

3. When the business flow is wrong, flag it immediately. Don't proceed to code-level review if the flow itself is incorrect or significantly inefficient. A perfectly coded implementation of the wrong flow is worse than a rough implementation of the right one. Frame it constructively: "The implementation is clean, but I think the business flow has an issue — here's what the user would actually experience."

When business flow concerns exist, they take priority over everything else.

Step 3: Analyze the PR

Foundational Review Principles

These four principles govern every check below. Violating them produces low-quality reviews.

1. Evidence-Based Review — verify against real code, not assumptions. Every claim in a review comment must be backed by evidence from the actual codebase. Before flagging an issue:

• Read the real type definition / SDK export — don't assume a field is optional; check the .d.ts or source. If the SDK types say id: string (required), an empty-string guard is unnecessary.
• Trace the actual call path — don't guess what a function does; read it. If ScopedDb.selectFrom already applies deletedAt IS NULL, flagging it as missing is noise.
• Check test fixtures and CI — if you claim "this test will fail", verify against the actual mock shape. If you claim "this throws at runtime", confirm the runtime type doesn't prevent it.
• When uncertain, say so — "I couldn't verify whether X is guaranteed here — worth checking" is more valuable than a false-positive P1.

2. Third-Party Verification — web-search external API docs, don't invent constraints. When a review touches external services (Stripe, Composio, Vapi, AWS, Auth0, etc.):

• Search the actual docs for constraints before flagging. Example: Stripe rejects timestamp older than 35 calendar days AND more than 5 minutes in the future — cite the docs URL.
• Check SDK behavior — e.g., does react-markdown v10 render raw HTML by default? (No — no rehype-raw plugin.) Check, don't guess.
• Cite the reference in your comment: "Per Stripe docs (https://docs.stripe.com/api/billing/meter-event/create), timestamp must be within past 35 days."
• Never fabricate API constraints — a wrong external constraint in a review wastes the author's time verifying your fiction.

3. User Perspective — trace the user journey, not just the code path. For every behavioral change, ask: "What does the end user / operator / on-call actually experience?"

• UI/UX — Visual consistency: Labels, badges, and status indicators must match across all views. If one view shows "ACTIVE" and another shows "active", that's a bug the user sees.
• UI/UX — Interaction integrity: Filter and search state must never orphan (dropdown shows a value that doesn't exist in options). Focus must never be stolen unexpectedly (e.g., agent dropdown change shouldn't yank focus to textarea). Pagination must not shift unrelated content (global "Show More" shouldn't reorder groups above the fold).
• UI/UX — Information preservation: Search results must retain context (show category chips when group headers are hidden). Grouped views must be stable on expand — clicking "Show More" must not shuffle existing items.
• UI/UX — Workflow continuity: Filter/search state should survive page reload (URL-backed via ?search= / ?category=). Loading and error states must always be visible — silent failures are UX bugs. URLs should be shareable (if a user copies the URL and sends it to a teammate, the teammate should see the same view).
• Operator experience: Log-level choices affect on-call. debug level for a revenue-impacting skip means no one notices until monthly revenue dips. Revenue, billing, and security events need warn or info minimum. Dashboard visibility matters — if a behavioral change causes an alerting spike, call it out in the PR description.
• Business impact: Silent data loss (e.g., swallowing billing errors) is a business bug, not just a code bug. Trace the failure mode to the business consequence.

4. Don't Manufacture Issues — if the code is clean, say so. A review that pads findings with noise erodes trust. Before posting a comment, ask: "Would this cause a real problem, or am I nitpicking to fill space?"

• Don't force every observation into a critique — if a pattern is unusual but correct, note it as "VERIFIED" rather than inventing a concern.
• Nitpicks must be labeled as such — use [NITPICK] or P3 so the author knows it's optional.
• Don't flag theoretical risks with no practical exploit path — "if someone passes negative infinity here" is not useful unless the input is user-controlled.
• Positive verification is part of a thorough review — marking verified-safe patterns (e.g., "SECURITY VERIFIED — tenant isolation holds because ScopedDb always applies orgId filter", "CHAIN VERIFIED — delete flow has layered ownership checks") shows the review was rigorous, not just looking for problems.

Approach-Level Review (before diving into code details)

Before examining individual files, evaluate the PR holistically:

1. Is the problem clearly defined? Read the PR description and linked issue/task. If the PR doesn't articulate what problem it's solving, flag it immediately — code review without a clear problem statement is guesswork. A good PR description answers: what was broken or missing, who it affects, and what "done" looks like. If the description is vague or absent, request clarification before investing in line-by-line review.

2. Is this the best approach, or just one that works? "It works" is necessary but not sufficient. Ask:

• Is this treating the root cause or just suppressing a symptom? (E.g., adding a null-check where the real fix is ensuring the value is never null upstream.)
• Is there a simpler, more direct solution? (Per AGENTS.md: "Minimalistic — every line of code is a liability.")
• Does this follow the existing architecture, or does it introduce a parallel pattern that will need reconciliation later?
• Would the approach scale if the same problem appears in 5 more places?

3. What are the trade-offs? Every non-trivial change has costs. Evaluate:

• Complexity cost: Does this introduce new abstractions, indirection layers, or special cases? Are they justified?
• Maintenance burden: Will the next developer understand why this was done this way? Does it create implicit coupling that's easy to break?
• Dependency growth: Does it pull in new libraries or tighten coupling to external services?
• Scope creep: Does the PR do more than it claims? Unrelated changes should be in a separate PR.

4. When the approach is fundamentally wrong, say so. If the PR is locally correct but structurally misguided — wrong abstraction layer, wrong data model, solving the wrong problem — don't just leave P2 nitpicks and approve. Flag it as P1 with a concrete alternative direction. A clean rewrite of a small PR is cheaper than maintaining a bad abstraction for years. Frame it constructively: "The code itself is clean, but I think the approach has a structural issue — here's what I'd suggest instead."

When approach-level concerns exist, they take priority over code-level findings. A perfectly written implementation of the wrong approach is still the wrong approach.

For each changed file, evaluate against the following categories:

Architecture & Code Rules (from AGENTS.md + planning docs)

Check against all rules read from AGENTS.md. Common violations to watch for:

• Route → Service → Repository layering (routes never import repositories directly)
• TypeBox schemas, typed errors, OpenAPI coverage for all new endpoints
• Database: composite PKs, RLS, UUIDv7, .limit() on all list queries, request.db() in routes
• No any, no ts-ignore, no eslint-disable — fix the code, don't weaken the rules
• Error handling: no unnecessary try/catch, let errors propagate to boundaries (API global handler, React error boundaries)
• Stateless functional services over classes; import * as serviceName for namespacing
• External SDK errors mapped via normalizeError, not caught-and-rethrown in services
• Use ValidationError / NotFoundError / ForbiddenError — never raw throw new Error() in routes/services
• Terraform: update module README when changing variables/resources/scopes
• No client-side secrets (NEXT_PUBLIC_* forbidden)
• Specialized helper preference — when a helper family exists (with-system-db.ts exports withSystemDb + withBootstrapSystemDb; with-scoped-db.ts exports withScopedDb + withScopedDbForOrg + ...), the most specific helper wins. A bootstrap path manually constructing a SystemDbContext and calling withSystemDb(ctx, fn) should be withBootstrapSystemDb(reason, fn) — review-time check: rg "withSystemDb\\(" apps/api/src and audit each site for whether a more specific helper applies (PR #1059).
• No runtime reads of repo-root files in bundled / Lambda code — code that runs in a bundled Lambda (or any artifact that doesn't ship the repo tree) must not readFile / readdir a path relative to process.cwd() / the repo root at runtime; the file isn't in the bundle. Inline the content as a code constant, or load it from a packaged asset / S3. Flag any readFileSync(join(process.cwd(), ...)) on an apps/api / apps/workloads runtime path (PR #1460 / #1557).

Behavioral Change Flagging

When code changes observable behavior (not just refactoring), explicitly flag it and ask whether it's intentional:

• Side effects of refactoring: Extracting a function may change when/where a side effect fires. E.g., wiring handleResetChat to a dropdown onChange now steals focus on every agent change — was that intended, or should focus only fire from explicit "New Chat" actions?
• Removed UI elements: If a label, badge, or status indicator is removed while sibling elements are kept, flag the visual inconsistency. "System message label removed but agent label kept — intentional?"
• Changed error propagation: If errors that were previously swallowed now propagate (or vice versa), trace the operational impact. "Stripe errors now throw → on-call will see webhook retry spikes during outages where it previously saw silence."
• Changed data scope: Switching from user-scoped to org-scoped queries changes who sees what. Trace the tenancy implications.
• Variable value-domain changes (second-order side effects): When a PR changes a variable's possible values (e.g., from "always non-null" to "possibly null" via a new conditional/dedup path), search for ALL downstream consumers of that variable in the same function. The consumer code may not be in the PR diff but can break silently. E.g., adding dedup that sets assistantEvent = null breaks a downstream touchSession(lastEventAt: assistantEvent.createdAt) that assumed non-null.
• Conditional skip / dedup bypassing side effects: When a PR adds a conditional skip (dedup, early return, guard clause), trace every side effect in the skipped code path. If any are still needed (metadata updates, timestamps, counters, cache invalidation), flag the gap. The primary behavior may work perfectly while metadata drifts silently.
• Buffer-then-flush vs centralized fan-out: When a PR persists agent / SDK events via an in-memory buffer that drains after the run completes (bufferedEvents.push(...) in onMessage, for (const ev of bufferedEvents) await appendEvent(ev) after runAgent resolves), check planning/engine/sessions.md for the centralization mandate. Buffer-then-flush means refresh during a 60s+ tool-using run still sees nothing — exactly the V2-380-class symptom mid-run. Either flag for centralization (write through onMessage live) or push back to tighten the AC wording in the PR description to "post-run only" (PR #997).

Concurrency & Race Conditions

• Async race conditions: When an async operation is started without await and a dependent action follows immediately (e.g., loadSession(id); focusComposer()), the dependent action can fire before the async work completes. On slow networks, users may interact with stale state.
• Database concurrency: SELECT → decide → INSERT/UPDATE inside a transaction gives atomicity but not isolation. Two concurrent callers on the same unique key (e.g., sync-write and reconcile on (orgId, provider, externalId)) can both SELECT "not found" and both INSERT, causing a 23505 unique violation. Handle with: catch 23505 + retry, or INSERT ... ON CONFLICT, or SELECT ... FOR UPDATE.
• Status flip-flop across write paths: When multiple code paths write to the same column (e.g., sync-write writes 'ACTIVE' uppercase, reconcile writes best?.status lowercase from SDK), every write triggers a spurious UPDATE. Normalize with a shared canonicalization function.
• React concurrent mode: requestAnimationFrame is not guaranteed to fire after React has committed state in concurrent mode. Use useEffect keyed on the relevant state instead.
• Timer/interval leaks: setTimeout / setInterval in components must be cleared on unmount. Without cleanup, callbacks fire on unmounted components.
• React effect trigger vs latest-read separation: For effects that hydrate persisted state (localStorage/session/URL-backed state), check whether deps represent true business triggers (namespace keys, route params, entity ids) or unstable tool identities (loadSession, resetChat, translation callbacks). If the effect should react only to the trigger but needs latest callbacks, use useEffectEvent rather than listing callback identities that can re-run hydration on ordinary renders.
• Self-cancel from synchronous state writes: Trace whether an async action called inside an effect synchronously sets a state value that is also in that effect's deps. Example: loadSession(storedId) sets sessionId before awaiting history; if sessionId is a dep, the effect cleanup can mark the first run cancelled=true, causing .finally() cleanup (setHydrating(false), readiness updates) to be skipped.
• Hydration readiness side-effect gaps: When an effect marks a namespace/state as PENDING, every branch must either mark it READY or intentionally keep it blocked with a user-visible reason. Early returns for stale sessionId, selected-session bypasses, missing stored ids, or superseded loads must be traced so persistence watchers do not silently drop new state.
• Cancelled load cleanup gaps: If a namespace/key change cancels an in-flight async load, verify the new run resets loading/hydrating state before any early return. Otherwise the old .finally() may skip cleanup due to cancelled=true, and the new empty namespace may suppress the empty state forever.
• Framework-level race prevention: Before flagging a timing race condition, verify whether the framework prevents it internally. E.g., Next.js router.replace wraps navigation in startTransition and uses an action queue that discards superseded pending navigations (app-router-instance.js dispatchAction). Consecutive rapid router.replace calls don't produce intermediate searchParams values — the old navigate is discarded before its state is committed. Read the framework source when official docs are silent on internal behavior. Don't flag theoretical race conditions that the framework's architecture prevents.
• No external network I/O inside the request-scoped DB lifecycle: A request's db() holds a pooled connection for the handler's duration; awaiting a slow external call (PMS, Composio, S3 upload) mid-handler pins that connection and starves the pool under load. Move post-commit side effects to an afterCommit hook / queue so the connection is released before the network call runs (PR #1557).
• Independent post-commit (afterCommit) callbacks: When multiple post-commit side effects are registered, each must be isolated — one throwing must not skip the others. Wrap each in its own try/catch (log + continue) rather than await a(); await b(); where a's failure drops b (PR #1557).
• Global state vs error-shape discriminators in catch blocks: When a catch block uses global state (request.signal.aborted, module-level flags, request-scoped booleans) to decide how to handle an error, verify no other error path can reach the same state. E.g., request.signal.aborted === true holds for any error that happens after a client disconnect — including genuine upstream failures (DNS, ECONNREFUSED, TLS) that coincided with the disconnect — causing silent swallowing of real bugs. Prefer error-shape discriminators: err instanceof DOMException && err.name === 'AbortError'. This matches the codebase's existing pattern in useChat.ts:605 / useChat.ts:723 / query-retry.ts:43.

External API Integration

• Pagination completeness: Any call to a paginated external API (e.g., connectedAccounts.list) must drain all pages via cursor/offset. Returning only the first page silently truncates data. After switching from user-scoped to org-scoped queries, the result set can grow dramatically.
• Safety caps on pagination: Cursor-based loops (do { ... } while (cursor)) must have a MAX_PAGES cap. A stuck cursor (API returns the same value repeatedly) can infinite-loop. Log a warning when the cap is hit so truncation is visible in observability.
• Time window constraints: External APIs often reject timestamps outside a window. E.g., Stripe meter events reject timestamps older than 35 calendar days or more than 5 minutes in the future. When deriving timestamps from stored data (e.g., interaction.endedAt), the value can be arbitrarily old during replays.
• Idempotency key stability: Idempotency keys (e.g., Stripe eventId) must be stable across retries. Using an application-generated UUIDv7 (interaction.id) that regenerates on transaction rollback breaks idempotency — use a stable external ID (e.g., providerId) instead.
• SDK documented guidance: When using an SDK hook (e.g., Composio afterExecute) for a purpose beyond its documented intent (data transformation), document the deviation and risk mitigations.
• SDK schema verification — silent parameter stripping: When an SDK uses Zod safeParse for input validation (most modern SDKs do), unknown keys are silently stripped. A parameter that looks correct in the call site can be a complete no-op if it's not in the SDK's schema. Always verify by reading the actual SDK schema in node_modules/.pnpm/{package}/dist/*.mjs — search for the relevant z.object({...}) definition. E.g., @composio/core@0.6.4's ConnectedAccountListParamsSchema includes orderBy but NOT orderDirection, so orderDirection: 'desc' is dead code (PR #919).
• SDK source over docs: When SDK docs are incomplete or ambiguous on default sort order, available parameters, or API scope, read the SDK source directly. Check what's in the Zod schema, what fields are mapped to the API request, and what transformations are applied. This is the only reliable way to verify behavior.

Security

• SSRF prevention (backend only): Server-side code (apps/api/) that accepts a URL and fetches it must validate the hostname against an allowlist and add redirect: 'manual' to block redirect-based SSRF. This rule does NOT apply to client-side code (apps/web/).
• Unbounded async fan-out: Promise.all over user-controlled-length arrays needs a cap or concurrency limit.
• Ownership chain verification: When a function gates access (e.g., verifyAccountOwnership), trace ALL callers. A broad catch that converts all errors to NotFoundError can silently pass through transient failures (429, 500), weakening IDOR protection. Only map the specific "not found" error; let others propagate.
• Error fallbacks weakening security: If a delete/refresh flow treats NotFoundError as "already gone, skip cleanup", then converting transient errors to NotFoundError means transient failures silently skip cleanup. Narrow the catch to the exact 404-equivalent.
• Tenant isolation on new patterns: When introducing includeDeleted, withTombstones, or similar query modifiers, verify they don't bypass the orgId tenant filter. Read the actual ScopedDb implementation to confirm.
• Default-deny direction of failures: When a safety cap (e.g., MAX_PAGES) truncates data, the failure direction matters. For ownership verification, truncation should result in NotFoundError (deny access), not silent pass-through. For data listing, truncation loses data but doesn't open a security hole — log a warning.
• Symlink / path-traversal escape on filesystem reads: Server code that resolves a user- or config-derived path under a base directory must canonicalize and re-check containment — path.join + a startsWith(base) check is bypassable via .. and symlinks. Use fs.realpath() (resolves symlinks) then assert the resolved path is still under the base, and lstat() to reject symlinks before reading. Applies to template loaders, attachment fetchers, any readFile(userControlledSegment) (PR #1460).
• Rendering LLM / untrusted HTML: sandbox="" alone does not stop passive subresource beacons (<img src>); require an iframe-local CSP placed before all content, and verify the sanitizer strips CSS @import / preserves <head>. See frontend-code-checks §54 (PR #1460 / #1557).

Operational Readiness

• Log level appropriateness: Revenue-impacting skips, security-relevant events, and unexpected upstream data must log at warn or info, not debug. debug is invisible at production log levels. The "no Stripe customer" skip is fine at debug (expected state); "no billable duration" needs warn (unexpected, may indicate upstream schema drift).
• logger.warn doesn't reach Sentry: The sentryLogHook in packages/sentry/src/log-hook.ts:27 only captures at level >= 50 (error / fatal). For user-visible content failures (e.g. .catch(() => log.warn({ err }, 'Failed to persist')) swallowing assistant prose), the warn level means the failure rate is uncomputable from Sentry — silent data loss with no alert. Recommend either bumping to logger.error so the hook fires, or calling Sentry.captureException(err, { tags: { ... } }) explicitly. Validation: apps/workloads/src/lib/logger.test.ts:76 (PR #997).
• Structured logging vs console.warn: console.warn doesn't appear in CloudWatch structured log queries or Datadog dashboards. Operationally meaningful events must use the pino logger, not console.*.
• Observability counters: When a planning doc specifies metrics (e.g., syncFailed, statusDriftDetected), verify the implementation actually tracks and logs them. Missing counters mean missing SLO signals.
• Logger error payload structure for triage: When a catch logs an error, the payload should include enough structured context for triage: e.g. log.error({ err, blockType: 'text', sessionId, agentId }, '...') rather than { err } alone. Without blockType, four call shapes (text, tool_use, thinking, tool_result) collapse into one Sentry issue and can't be bisected (PR #997).
• Audit trail completeness: When withScopedDb is called, check whether userId, requestId, and reason are set. Audit trigger columns getting null means forensic queries can't trace who triggered the write.
• Runbook/rollout notes: When a PR changes error propagation or alerting behavior, the PR description must document the operational impact. E.g., "On-call will see webhook retry spikes during Stripe outages where it previously saw silence."

Sentry & Replay Configuration

When a PR touches sentry.client.config.ts / sentry.server.config.ts / Sentry.captureException sites / Sentry filters / replay configuration, also apply the patterns in the dedicated sentry-observability skill. Common review checks:

• ignoreErrors vs beforeSend: ignoreErrors drops events at the SDK level — also dropping the on-error replay buffer (replays attach AFTER ignoreErrors runs) and any explicit Sentry.captureException with diagnostic tags. For noise filtering with canary sampling and tag-aware bypass, beforeSend is correct.
• Tag-aware bypass coverage: When a beforeSend filter bypasses tagged captures ('errorBoundary' in event.tags), audit ALL Sentry.captureException sites for the bypass tag. Common misses: useChat.ts loadSession capture, QueryProvider.tsx retry-exhausted fallback (PR #1039).
• Replay coupling documentation: beforeSend returning null discards the on-error replay too, even with replaysOnErrorSampleRate: 1. Require an inline comment so the next reader doesn't assume replay survives.
• Engine-agnostic constant naming: When a regex covers multiple browsers (Failed to fetch Chromium + Firefox, Load failed Safari), name it TRANSIENT_FETCH_FAILURE, not CHROMIUM_FETCH_FAILURE.
• Canary sampling math: 2 ** 32 reads better than 0x100000000 and avoids the off-by-one of 0xFFFFFFFF (which maps the max sample to exactly 1.0).
• Framework-internal class anchoring: Filters that match error.name === 'ResponseAborted' (Next.js) / BailoutToCSRError / FST_ERR_VALIDATION (Fastify) must comment the verified framework version. No semver stability on internal types — silent no-op on major bumps. See sentry-observability Rule 11 (PR #1110).
• No 100% blackout on lifecycle errors: For "expected noise" classes (ResponseAborted, Failed to fetch, AbortError), beforeSend must keep at least a 1% canary so infra-anomaly volume signals (keep-alive misconfig, ALB idle-timeout drift, CORS regressions) survive. See sentry-observability Rule 12.
• Scrub before sample/branch: Header / body scrubbing in beforeSend runs at the TOP of the function, before any sampling or filter branch. Otherwise the 1% canary leaks Authorization tokens. See sentry-observability Rule 13.
• Runtime symmetry or YAGNI: Filter additions in client config must have matching server-side filters (or a documented asymmetry) — and vice versa. Don't add edge-runtime filters when no edge routes exist. See sentry-observability Rule 14.
• Pass-through (negative) test fixture: Every beforeSend filter test must include at least one event that survives the filter. Positive-only tests pass when the matcher silently breaks (regex typo, framework rename). See sentry-observability Rule 15.

Global Error Handler Patterns

When a PR touches QueryProvider.tsx, any useActionErrorHandler-style hook, MutationCache.onError / QueryCache.onError, or global toast helpers, also apply the patterns in the dedicated frontend-error-handling skill:

• Classification ladder: Global handlers must early-return per error class (transient network → typed ApiErrorWrapper → unknown). The unknown branch is the only one that fires Sentry.captureException + the generic internal_error toast. A flat capture-everything handler makes Sentry volume signals useless. See frontend-error-handling §1 (PR #1069).
• Narrow isTransientNetworkError: Predicate must combine instanceof TypeError|DOMException with the is-network-error library + explicit DOMException name check. Plain error instanceof TypeError silently downgrades real "cannot read properties of null" bugs to network-blip toasts. See §2.
• Toast cooldown: Per-category 5 s cooldown (module-level Map keyed on classification, not message) prevents spam during multi-query offline scenarios. See §3.
• Server flag → client Provider: Feature flags resolved server-side must reach client components via Provider, not be re-resolved client-side with hardcoded fallbacks. SSR/CSR divergence is invisible in unit tests. See §4 (PR #1072).

API Response Mappers / Multi-Turn Capture

When a PR touches API response mappers or agent-runner code, also check backend-db-conventions:

• Strip null JSONB before serialization: fast-json-stringify can emit {} for runtime null JSONB values, which the FE then mis-reads as "feature enabled". Mapper must explicitly skip null keys, or the TypeBox schema must declare Type.Union([..., Type.Null()]) (PR #1116).
• Multi-turn agent capture per turn: Code reading structured tags (<chat_title>, citations, telemetry) from agent runs must walk every assistant message during the stream — result.output is whatever the final turn was, often a tool call. Use a "last seen wins" comment to document the choice (PR #1071).
• System-row lookups fully qualified: shared system rows (system agent, stable template, platform default) must be filtered on isSystem + channel = 'stable' + system org, not slug alone — slug is operator-editable and can be shadowed by a tenant row or a draft. See backend-db-conventions "System-row lookups" (PR #1460 / #1557).
• Integer schema fields + tenant ON CONFLICT predicate: size/count/byte fields use Type.Integer({ minimum: 0 }), not Type.Number(); an upsert's onConflict must mirror createTenantTable's partial unique index (WHERE deleted_at IS NULL) or it throws "no unique constraint matching" at runtime. See backend-db-conventions (PR #1460).

Cross-Service Data Consistency

• Multiple write paths for the same data: When sync-write, reconcile, and detail-endpoint all derive the same field (e.g., effectiveStatus), all paths must use the same normalization. One writing uppercase and another writing lowercase causes UPDATE churn on every cycle.
• Data carry-forward correctness: When reading existing rows before upserting, prefer live rows over tombstones. If a tombstone and a live row coexist for the same key, carrying forward the tombstone's metadata overwrites the live row's data.
• DRY across app boundaries: Near-verbatim copies of business logic across apps/api and apps/workloads (e.g., upsertManagedApp vs upsertByExternalId) are a maintenance hazard. "Must stay in sync" comments are not enforcement. Flag for extraction to a shared package.
• Enum/status value consistency: When an external SDK returns lowercase values (e.g., "active") but the codebase uses uppercase ("ACTIVE"), normalize at the boundary. Inconsistency causes spurious writes, wrong UI filtering, and status flip-flop.
• JSONB column type awareness: Kysely returns JSONB columns as parsed JS objects (not strings). Code that checks typeof metadata === 'string' will always be false for JSONB data from the DB. This is a common source of silent bugs in merge/comparison logic — e.g., mergeMetadataJson using typeof existing !== 'string' to guard against non-string input actually skips the merge for every DB-sourced value, losing critical fields like slackAppId during tombstone resurrection (PR #919). When reviewing metadata merge/compare logic, verify the code handles both string inputs (from JSON.stringify) and object inputs (from DB reads).
• Metadata preservation on resurrection: When a tombstone row is resurrected, metadata must be merged (spread existing + incoming), not overwritten. Fields like slackAppId in metadata are critical for cleanup flows (deleteIntegration → parseSlackAppId → Slack App cleanup). Overwriting loses them, creating orphans that are unrecoverable from the row alone.
• Strip output markers consistently across paths: When the same content is emitted on a live path (SSE) and a persisted path (DDB / Postgres), every path that reads or stores the text must run the same scrub function. E.g., SSE strips <chat_title>...</chat_title> via drainTagBuffer (stream-publisher.ts:64), and the final-output write strips via stripChatTitle(result.output) — but a new intermediate-text persistence branch that writes block.text raw leaks the literal tag into the DB and renders inside the assistant bubble on refresh. Walk every write-site and confirm the same scrub runs (PR #997).
• Append-then-write retry safety: When a job appends an event (appendEvent(...)) and then writes a downstream row (withScopedDb(...)), ask: if the downstream write throws, does SQS retry the whole job? If yes, the appendEvent has already run → the next attempt creates a duplicate event. Required mitigation: wrap the downstream write with .catch((err) => log.warn({ err }, '...')) so it's idempotent across retries, OR move the append AFTER the write (subject to the inverse risk: write succeeds but event missing). Default is the catch-and-log pattern, matching persistContentBlock / deps.onMessage (PR #992).
• Dedup with .at(-1) only matches the last buffered item: When a final-output write is deduped against a buffered list via bufferedEvents.at(-1), an SDK final output that matches an EARLIER buffered event still produces a duplicate write. If the assumption is "last buffered item is the final output", document it inline with a comment naming the SDK guarantee. Otherwise widen the dedup to bufferedEvents.some(...) or normalize via a content hash (PR #997).

Cross-Layer & Multi-Context Consistency

• Fallback chain alignment: When the same value is resolved in multiple layers, verify all layers use the same fallback chain in the same order.
• Dead fallbacks / phantom data dependencies: If code adds a fallback to a field, verify something in the system actually writes that field. A fallback to unwritten data is dead code.
• Planning doc alignment: When a PR adds new patterns, check whether existing planning docs describe a different target direction. Flag contradictions as blocking.

Planning Doc Consistency (blocking)

• Cross-doc conflicts: When a PR changes behavior documented in another planning doc, that doc must be updated in the same PR. E.g., if composio-multitenancy.md says "no migration needed" but the PR introduces a migration, the planning doc must be updated. Flag as blocking.
• Superseded sections: When a PR replaces a mechanism described in a planning doc (e.g., removing webhookAuth plugin), add a superseded callout in the old doc pointing to the new one.
• Executable rollout ordering: Deployment steps labeled "MANDATORY order" must be actually executable in that order. A "Pre-deploy sanity check" listed after deploy steps is self-contradictory.
• Idempotency semantics: Upsert/reconcile sections should spell out when a write is a no-op vs update. Without this, implementers may write "select → always update" causing unnecessary DB churn.

Error Handling (beyond AGENTS.md basics)

• Never silently swallow errors: catch {} or catch { setError(true) } with no logging is forbidden.
• Scope catch blocks narrowly: Only catch the specific error you can handle. A bare catch on connectedAccounts.get() that converts 429/500/network errors to NotFoundError masks transient failures and weakens security gates.
• Don't conflate API errors with empty data: Mapping 404 → [] conflates "not found" with "empty". Let 404 propagate as an error.
• Abort/cancel detection must use error shape, not signal state: When swallowing fetch abort errors under the AGENTS.md "expected errors" exception, discriminate via err instanceof DOMException && err.name === 'AbortError' — the spec-compliant shape thrown by both browsers and Node undici. Flag any catch block that uses request.signal.aborted, controller.signal.aborted, or similar global signal state as the discriminator: those return true for any error that lands after abort, silently hiding genuine upstream failures (DNS / ECONNREFUSED / TLS) when they coincide with a client disconnect. For route handlers, the swallow path should return 499 (Nginx convention, non-error in Datadog / Sentry dashboards), not 200 or 500.
• Pushback on "add console.warn for visibility" in expected-error catches: When a reviewer requests a log on a branch that legitimately swallows expected noise (abort, 404→null, parse-or-fallback), check AGENTS.md before agreeing. AGEN

…(truncated)