# Pr Fix > Fix PR review comments from GitHub: fetch all comments, analyze each one (bug vs nitpick vs informational), present a summary, ask for confirmation before making code changes, add test coverage when requested, and verify nothing breaks. Use when the user shares a PR URL and asks to fix, address, or resolve review comments/feedback. - Skill: `ellaliu0401/pr-fix` (Agent Skill) - Install (CLI): `npx skillmds@latest add ellaliu0401/pr-fix` - Raw SKILL.md: https://api.skillmd.com/api/skills/ellaliu0401/pr-fix/raw - Safety review: pending - Works with: Claude Code, Claude.ai, OpenAI Codex - Category: Coding & Dev Tools - Author: EllaLiu0401 (https://skillmd.com/u/ellaliu0401) - Updated: 2026-09-22 - Page: https://skillmd.com/skills/ellaliu0401/pr-fix --- # PR Review Comment Fix Workflow Fix review feedback on a GitHub PR: analyze all comments, present findings, get user approval, make changes, add tests, verify. ## Step 1: Fetch All PR Data (parallel) ``` gh api repos/{owner}/{repo}/pulls/{number}/comments # inline review comments gh api repos/{owner}/{repo}/pulls/{number}/reviews # review summaries gh api repos/{owner}/{repo}/issues/{number}/comments # issue-level comments gh pr view {number} --json title,body,headRefName,files # PR metadata + file list ``` ### Switch to PR branch **MANDATORY**: Before analyzing or editing code, check the current branch and switch to the PR branch: ``` git checkout {headRefName} git pull origin {headRefName} ``` The local working tree may be on a different branch — reading files without switching will produce incorrect analysis. ### Filter out bot noise Only analyze comments from **human reviewers** (team members). Skip: - `coderabbitai[bot]`, `claude[bot]`, `sonarqubecloud[bot]`, `github-actions[bot]`, `swarmia[bot]` - Any user where `"type": "Bot"` **AI-assisted reviewer comments** (e.g., "Agent Cube / Judge Grok", "Agent Cube / Judge GPT") are posted by human accounts but generated by AI. Treat these with extra scrutiny — they may contain false positives (e.g., flagging duplication that doesn't exist). Always verify by reading the actual code before accepting a CRITICAL/WARNING from an AI judge. Group remaining comments by reviewer login for clarity. ## Step 2: Read Project Rules Before analyzing, read the project's architecture docs: 1. **Always**: `AGENTS.md` (source of truth for all constraints) 2. **Based on scope**: relevant `planning/*.md` docs (see `planning/README.md` for index) 3. **Check codebase**: search for similar patterns/features already implemented as reference ## Step 3: Objective Verification (MANDATORY — before any categorization) ⚠️ **CRITICAL: Never accept a reviewer's claim at face value.** Reviewer comments are opinions — they may be correct, partially correct, or wrong. Before deciding whether to fix anything, you MUST independently verify every claim against the actual codebase. **The failure mode to avoid:** Agreeing with a reviewer because they sound authoritative, because the user seems to want you to fix it, or because the comment "sounds reasonable." This leads to hallucination-driven changes that break working code or add unnecessary complexity. ### For EVERY comment, complete this verification gate: 1. **Read the actual code being discussed** — not just the diff snippet in the comment. Read the full file (or relevant section) to understand the complete context. Comments often reference 5-line snippets but miss the 50-line context that explains _why_ the code is written that way. 2. **Verify the factual claim** — If the reviewer says "this will break when X", prove it: trace the code path, check the types, read the tests. If the reviewer says "this pattern isn't used elsewhere", search the codebase to confirm. If the reviewer says "this violates rule Y", read rule Y and check if it actually applies to this specific case. 3. **Assess from the feature's perspective** — What is this PR trying to accomplish? Does the reviewer's suggestion serve the feature's goal, or does it optimize for a concern orthogonal to the PR's purpose? A suggestion that is "correct in general" but irrelevant to the feature's intent is noise, not signal. 4. **Check if the existing code is actually wrong** — The current code passed CI, was written with intent, and may have been reviewed before. The default assumption should be "the code is correct until proven otherwise", not "the reviewer is correct until proven otherwise." 5. **Evaluate the net impact** — Would the suggested change make the codebase _objectively_ better (fewer bugs, clearer intent, better performance, stronger type safety)? Or is it a lateral move (different but not better) or a subjective preference? ### Verification verdict (record for each comment): ``` - Claim: {what the reviewer asserts} - Evidence: {what you found by reading the actual code / searching the codebase} - Verdict: CONFIRMED / PARTIALLY VALID / INCORRECT / SUBJECTIVE PREFERENCE - Reasoning: {2-3 sentences of logical justification} ``` **If you cannot find concrete evidence that the reviewer's claim is correct, do NOT default to fixing it.** Instead, mark it as "Needs clarification" and present both sides to the user. ## Step 3b: Categorize Each Comment (after verification) Only AFTER completing the objective verification above, categorize each comment: | Category | Criteria | Action | |----------|----------|--------| | **Bug / Must-fix** | **Verified** incorrect behavior, security issue, or rule violation with evidence | Fix required | | **Valid improvement** | **Verified** DRY, robustness, consistency issue — simple fix, net positive | Fix recommended | | **Test coverage** | Reviewer asks for tests or flags missing coverage | **Always add tests** | | **Already fixed** | Author replied "fixed in commit X" or code shows fix | Verify fix is in place | | **Nitpick / Informational** | Style preference, FYI, "not blocking" | No change needed | | **Subjective / Unverified** | Reviewer's claim could not be confirmed by reading the code | Present both sides to user | | **Over-engineering risk** | Suggestion adds complexity without clear value | Push back (KISS) | | **Incorrect** | Reviewer's claim is factually wrong based on codebase evidence | Skip, explain why | ### Analysis checklist per comment - [ ] **Did I actually read the code?** (not just the diff — the full relevant context) - [ ] **Did I verify the claim with evidence?** (not just "it sounds right") - [ ] Is this comment about a real bug or just a suggestion? - [ ] Does the current code already address this? (check latest commit) - [ ] Is the suggested fix the **simplest** solution? (KISS — always ask this) - [ ] Would the fix affect other functionality? (search for all importers/callers) - [ ] Does the codebase already have a pattern we should follow? - [ ] Does AGENTS.md or a planning doc have a relevant rule? - [ ] **Does the rule apply to this file's scope?** Backend rules (`apps/api/`) don't apply to frontend files (`apps/web/`) and vice versa. E.g., `redirect: 'manual'` for SSRF is a backend concern, not needed in client-side code. - [ ] **Terraform IAM organization by mechanism**: If a review asks to move one service permission (e.g., S3 read) to a different Terraform file, check how that task role is audited today. For ECS services using `local._task_iam_statements` → Fargate `additional_task_iam_statements`, keeping runtime grants together may be clearer than splitting one grant into a managed policy attachment just because the resource is S3. Still update the module README when scopes change. - [ ] If deleting a file/export, who imports it? (verify zero external consumers) - [ ] If moving code between files, does the function signature stay identical? (no TS breakage) - [ ] If changing UI layout, do E2E tests reference moved/removed elements? - [ ] If an AI judge flags a CRITICAL/WARNING, verify by reading the actual code — AI reviewers often misread cross-file relationships. - [ ] **Framework-level protection**: When a reviewer flags a timing / race condition, verify whether the framework prevents it at a lower level before implementing a defensive fix. Read the actual framework source code — official docs often don't cover internal mechanisms. E.g., Next.js `router.replace` uses an action queue that discards superseded navigations (`app-router-instance.js` `dispatchAction`), preventing `searchParams` from showing intermediate values during fast typing. A `lastSyncedRef` for this scenario would be YAGNI. - [ ] **Cross-layer consistency**: If the same value (brand, locale, config) is resolved in multiple layers (JS, templates, Terraform, subject lines), verify all layers use the same fallback chain. Fixing one layer without fixing others creates divergence. - [ ] **Dead fallbacks**: If a fallback references a field (e.g., `user.app_metadata.brand`), verify something actually writes that field. Don't add fallbacks to phantom data. - [ ] **Planning doc alignment**: Check if the fix contradicts the target direction in `planning/*.md`. If so, either align with the plan or update the planning doc. - [ ] **Second-order side effects**: If the fix changes a variable's value domain (e.g., from "always non-null" to "possibly null"), search for all downstream consumers of that variable in the same function/module. Code you didn't touch can break if it had an implicit assumption about the old value domain. - [ ] **Dedup / control-flow skip**: If the fix adds a conditional skip (dedup, early return, guard clause), trace what code is now bypassed and check if any side effects in the skipped path are still needed (e.g., metadata updates, timestamps, counters). - [ ] **SDK parameter existence**: If the fix adds parameters to an SDK call, verify the parameter is in the SDK's validation schema (Zod, io-ts, etc.) — don't trust parameter names or docs alone. Read the actual schema in `node_modules`. - [ ] **JSONB type handling**: If the fix processes a JSONB column value (metadata, config, etc.), verify the code handles both `object` (from DB reads) and `string` (from `JSON.stringify`) inputs. `typeof x !== 'string'` is always true for DB-sourced JSONB. - [ ] **Metadata merge on resurrection**: If the fix touches tombstone resurrection (soft-delete → restore), verify metadata is merged (spread), not overwritten. Critical fields like `slackAppId` survive only through merge. - [ ] **Specialized helper preference**: Before manually constructing a context for `withSystemDb` / `withScopedDb`, grep the file's exports for a more specific helper (`withBootstrapSystemDb`, `withScopedDbForOrg`, etc.) — the parameterized variant usually exists. - [ ] **Sentry log-level threshold**: `logger.warn` (level 40) is silently dropped by `sentryLogHook` (threshold >= 50). User-visible content failures need `logger.error` or explicit `Sentry.captureException`. - [ ] **Sentry `beforeSend` over `ignoreErrors`**: For new noise filters in `sentry.*.config.ts`, default to `beforeSend` (preserves replays + tagged captures) and read the `sentry-observability` skill. - [ ] **Sentry framework-class anchor**: Filters matching framework-internal class names (`ResponseAborted`, `BailoutToCSRError`, etc.) must comment the verified framework version — no semver stability on internal types (`sentry-observability` Rule 11). - [ ] **Sentry no 100% blackout**: For "expected noise" lifecycle errors, keep at least a 1% canary so infra-anomaly volume signals survive (`sentry-observability` Rule 12). - [ ] **Sentry scrub before sample**: `beforeSend` runs scrubbing at the top, before any sampling or filter branch (`sentry-observability` Rule 13). - [ ] **Sentry filter test pass-through case**: Every `beforeSend` test must include a negative-case fixture that survives the filter (`sentry-observability` Rule 15). - [ ] **Global error handler classification**: Handlers fan errors via a transient-network → typed-API → unknown ladder; only the unknown branch fires `captureException` + `internal_error` (`frontend-error-handling` §1). - [ ] **Narrow `isTransientNetworkError`**: Combine `instanceof TypeError|DOMException` with `is-network-error` + explicit DOMException name check; never plain `instanceof TypeError` (`frontend-error-handling` §2). - [ ] **Toast cooldown**: Per-category 5 s cooldown via module-level Map prevents spam during simultaneous query failures (`frontend-error-handling` §3). - [ ] **Server-resolved flag → client Provider**: Feature flags reach client via Provider; never re-resolved client-side with a hardcoded fallback (`frontend-error-handling` §4). - [ ] **Strip null JSONB in API mappers**: `fast-json-stringify` may emit `{}` for runtime `null`. Mapper omits null keys, or TypeBox schema declares `Type.Union([..., Type.Null()])` (`backend-db-conventions`). - [ ] **Multi-turn agent capture per turn**: Walk every assistant message for structured tags during the stream — `result.output` alone is wrong for any multi-turn scenario (`backend-db-conventions`). - [ ] **Picker default normalization**: When the stored value equals the platform default, normalize to `''`/`null` so the preset row is the only highlighted option (`frontend-code-checks` §44). - [ ] **Custom-override → form read-only**: If the resource has a custom override active, render the picker disabled with an "Edit via override API" hint (`frontend-code-checks` §45). - [ ] **Optimistic toggle auto-rollback via effect**: Drive optimistic state from `useEffect([serverState])`, not stale-closure `onError` callbacks (`frontend-code-checks` §46). - [ ] **Test env-var snapshot+restore**: Tests that mutate `process.env` snapshot in `beforeEach`, restore (or `delete`) in `afterEach`. Otherwise mutations leak across files (`frontend-code-checks` §47). - [ ] **Buffer-then-flush vs centralized fan-out**: When a backend fix routes events via an in-memory buffer that drains after the run, check `planning/engine/sessions.md` for the centralization mandate before merging. - [ ] **Append-then-write retry safety**: If the fix adds `appendEvent(...) → withScopedDb(...)`, wrap the downstream write with `.catch((err) => log.warn(...))` so SQS retries don't duplicate the appended event. - [ ] **Real-DB integration tests for branched WHERE guards**: When the fix adds a function with multi-branch `WHERE` (e.g. `WHERE x IS NULL OR y = 'auto'`), add a Testcontainers test for each branch — especially the safety branch that protects user data. - [ ] **OpenAPI `required` + nullable**: For nullable DB columns surfaced in responses, use `required` + `T | null`, not `Optional`. The latter changes client semantics from "value is null" to "key may be absent". - [ ] **Server Component callback prop antipattern**: If the fix touches a Server Component that receives `t` / `formatDate` as props, replace the props with `getTranslations()` / `getFormatter()` calls inside the component. - [ ] **Paired container className parity**: Loading / error / empty / content containers must share the same className shape — diff them character-by-character. - [ ] **Legacy URL redirects on page deletion**: When deleting/consolidating page routes, verify `next.config.ts` has `redirects()` entries for old paths → new paths. Use `permanent: false` (302) for recent migrations. - [ ] **E2e page object heading alignment**: When a page title changes, update `expectLoaded()` heading regex. Add tab-specific navigation methods (`gotoTeamTab()`) when pages gain tabbed navigation. Update all callers. - [ ] **Navigation consolidation negative assertions**: When fixing nav consolidation tests, assert both the new link (with `href` validation via `getAttribute`) AND the absence of old nav items (`queryByText().not.toBeInTheDocument()`). ## Step 4: Present Summary to User **MANDATORY**: Present the full analysis BEFORE making any code change. Format: ```markdown ## PR #{number} Comment Analysis ### From {reviewer_name}: **Comment 1: {short title}** > {quote the comment} - Reviewer's claim: {what the reviewer asserts} - Verification: {what you actually found by reading the code} - Verdict: CONFIRMED / PARTIALLY VALID / INCORRECT / SUBJECTIVE PREFERENCE - Category: {Bug / Valid improvement / Nitpick / Subjective / Incorrect / ...} - Recommendation: {Fix / Skip / Already addressed / Push back} - Reasoning: {logical justification — why fix or why not, based on evidence} - Impact: {what changes, what files affected} **Comment 2: ...** ### Summary | # | Comment | Verdict | Category | Recommendation | |---|---------|---------|----------|----------------| | 1 | ... | CONFIRMED | Bug | Fix | | 2 | ... | SUBJECTIVE | Nitpick | Skip | | 3 | ... | CONFIRMED | Test coverage | Add tests | | 4 | ... | INCORRECT | — | Skip (explain) | ``` ### After presenting, ASK: > "Do you want me to proceed with the recommended fixes? Or would you like to adjust any of the recommendations?" **Wait for user confirmation before writing any code.** ## Step 5: Implement Fixes After user approval, for each fix: ### Code changes 1. **Read the file first** — never edit blind 2. **Check existing patterns** — search the codebase for similar code as reference 3. **Make the minimal change** — KISS, no scope creep 4. **Follow AGENTS.md rules** — DRY, no `any`, no `eslint-disable`, etc. 5. **Verify SDK schemas before adding parameters** — when a reviewer suggests adding SDK parameters (e.g., `orderDirection`), read the SDK's Zod schema in `node_modules/.pnpm/{package}/dist/*.mjs` to confirm the parameter exists. Zod `safeParse` silently strips unknown keys — the parameter compiles fine but is a no-op at runtime. 6. **JSONB columns return objects, not strings** — Kysely returns JSONB columns as parsed JS objects. Never use `typeof x === 'string'` to guard JSONB data from the DB — it will always be false. Use runtime type narrowing: `typeof x === 'object' && x !== null` for objects, handle both string and object inputs. 7. **No `as` type assertions** — this codebase uses `@typescript-eslint/consistent-type-assertions: never`. For `JSON.parse` results, use `const x: unknown = JSON.parse(...)` + runtime type narrowing (`typeof x === 'object' && x !== null` → spread is valid on narrowed `object` type). Never use `as Record` or similar. 8. **Specialized helper check** — when a fix touches a helper family (`with-system-db.ts`, `with-scoped-db.ts`, `withTenancyContext`, etc.), grep the file's exports and pick the most specific helper. A bootstrap path manually constructing a `SystemDbContext` and calling `withSystemDb(ctx, fn)` should be `withBootstrapSystemDb(reason, fn)` — the parameterized helper exists for exactly this case (PR #1059). 9. **Sentry log-level threshold** — when reviewer feedback says "log this failure" or "this should reach Sentry", confirm the target level. `logger.warn` is level 40 and is silently dropped by `packages/sentry/src/log-hook.ts:27` (threshold >= 50). For user-visible content failures, fix to `logger.error` so the hook captures, or call `Sentry.captureException` explicitly (PR #997). Add structured payload context (`blockType`, `eventType`, etc.) so the Sentry issue can be bisected (see the dedicated `sentry-observability` skill for full Sentry / replay / canary-sampling patterns). 10. **`beforeSend` over `ignoreErrors` for noise filters** — when a fix adds Sentry filtering for transient errors (`Failed to fetch`, etc.), prefer `beforeSend` with canary sampling + tag-aware bypass over `ignoreErrors`. `ignoreErrors` runs before replay attachment (drops the on-error replay) and matches on message only (drops tagged diagnostic captures). Read the `sentry-observability` skill before editing `sentry.*.config.ts` (PR #1039). ### Test coverage (always add when flagged) Determine the right test type based on what changed: | Change type | Test type | Location | |-------------|-----------|----------| | Pure function / util | Unit test (Vitest) | Co-located `*.test.ts` | | API endpoint | Integration test | `apps/api/test/integration/` | | React component | Component test (RTL) | Co-located `*.test.tsx` | | UI flow / localStorage / redirect | E2E test (Playwright) | `apps/e2e/tests/*.e2e.ts` | | Multi-endpoint sequence | Flow test | `apps/api/test/flows/` | **Before writing tests**: 1. Read `planning/testing.md` for conventions 2. Find an existing test file of the same type as reference 3. Follow the exact same patterns (imports, describe structure, fixtures) 4. **Test side effects, not just primary behavior**: For each new branch path (if/else, dedup skip, early return), ask "what side effects does this path have?" and assert them. E.g., if a dedup path skips a write, verify that dependent metadata (timestamps, counters) is still updated from an alternative source. 5. **Test name must match assertion**: Read the test name aloud, then check if the mock data and assertions actually verify that exact scenario. A test named "writes when differs" that actually tests the "match" path is a coverage gap in disguise. ## Step 6: Verify No Breakage Run these checks after all changes: ``` task typecheck # Full TypeScript check across all packages task lint # ESLint + Prettier (fix formatting issues automatically) ``` If lint fails on the new file, run `npx prettier --write {file}` and re-check. Also verify manually: - [ ] All existing logout/auth/login entry points still work with the change - [ ] No imports broken - [ ] New test file matches the correct Playwright project / Vitest config ## Step 7: Report Results After verification passes: ```markdown ## Changes Complete ### Files modified: - `path/to/file.ts` — {what changed} ### Files created: - `path/to/test.e2e.ts` — {what tests were added} ### Verification: - TypeCheck: PASS - Lint: PASS - Impact: {confirm no other functionality affected} ### Suggested commit message: \`\`\` fix(scope): short description of the fix \`\`\` ``` Remind the user to commit manually (never commit automatically). ## Step 8: Reply to Comments on GitHub After all fixes are implemented and verified, reply to each reviewer comment directly on GitHub using `gh api`: ```bash gh api repos/{owner}/{repo}/pulls/{number}/comments/{comment_id}/replies \ -f 'body=Reply text here' ``` ### Reply format - Use **Before / After** format to show what changed concisely - Keep replies **English, concise, polite, enthusiastic, logical** - For items not fixed, explain the reasoning clearly (e.g., "Keeping as-is because...") - Reference architecture rules when relevant (e.g., "Per frontend-architecture.md §4") ### Reply template ``` Good catch — {brief acknowledgment}. **Before:** {old code or behavior, one line} **After:** {new code or behavior, one line} {Optional: brief explanation of the design decision} ``` ### Reply template — pushback with reasoning When declining a suggestion, cite the rule, the reference pattern, and the concrete tradeoff. Don't just say "no" — explain why the suggestion subverts a higher principle. ``` Acknowledged — {summary of suggestion}. {Why the suggestion conflicts with X}: {concrete trace of the cost}. Reference: {AGENTS.md §section / planning/foo.md / existing pattern at file:line}. Happy to add it back if you'd prefer, but the {KISS / consistency / observability} cost felt too high. ``` **Worked example (PR #1038 — declining `console.warn` on the 499 branch):** > Skipped the `console.warn` though — with the tighter check, the 499 branch is by definition expected client churn (per agents.md "swallowing expected errors" exception). LB access logs already surface the 499 count if we ever need to spot abnormal disconnect spikes, and adding warn-level noise to CloudWatch on every tab close felt against the KISS spirit. Happy to add it back if you'd prefer. **Worked example (PR #997 — addressing logger threshold + accepting an "investigate" comment):** > Investigated — this is actually a non-issue. `stripChatTitle` already returns a trimmed string (implementation: `return text.replaceAll(CHAT_TITLE_RE_GLOBAL, '').trim()`), so `text` in `persistContentBlock` is already trimmed before `onMessage`. Both sides of the dedup go through `stripChatTitle` (which includes `.trim()`), so whitespace differences cannot cause a mismatch. > > Simplified the redundant `text.trim().length > 0` to `text.length > 0` in cebd2dd for clarity. The reply pattern: state the investigation, cite the implementation, name the code change (or non-change). Reviewers trust this far more than a bare "fixed" or "skipped". ## Key Principles - **Verify before you believe** — every reviewer claim must be checked against actual code. Sounding reasonable ≠ being correct. Read the code, trace the logic, find evidence. - **Code is the source of truth, not opinions** — if the code works, passes CI, and follows project patterns, the burden of proof is on the reviewer to show why it's wrong, not on you to assume it is. - **Never hallucinate agreement** — if you can't verify a claim, say so. "I couldn't confirm this" is always better than silently going along. Presenting both sides to the user is the correct action when uncertain. - **Distinguish objective issues from subjective preferences** — bugs, type errors, and rule violations are objective. "I would have written it differently" is subjective. Only objective issues warrant code changes. - **Always ask before changing code** — present analysis with verification evidence first, wait for approval - **Always add tests** when reviewers flag missing coverage - **KISS** — for every fix, ask "is this the simplest solution?" - **Check references** — search codebase for existing patterns before inventing new ones - **Don't break things** — run typecheck + lint after every change - **Follow AGENTS.md** — it prevails over all other docs - **Minimal scope** — only fix what the reviewer asked about, nothing more. Never include unrelated changes (e.g., reverting a commit from another PR) — they attract extra review scrutiny and can introduce flaky tests. - **Verify rule scope** — backend rules don't apply to frontend files and vice versa - **Trace the full variable lifecycle** — when a fix changes a variable's possible values, grep for every downstream use in the function. The bug is often not in the code you changed, but in the code you didn't change that assumed the old behavior. - **Scrutinize AI reviewer comments** — Agent Cube / bot judges can produce false positives; always verify by reading code - **Reply to every comment** — even if skipping a suggestion, explain the reasoning on GitHub ## Provenance - Base workflow established from recurring PR review/fix cycles across the codebase. - Rules on second-order side effects, dedup control-flow tracing, test side-effect coverage, and test-name/assertion alignment added after PR #997 (`V2-380/fix-intermediate-messages`), where a dedup skip path left `touchSession.lastEventAt` unset because the code assumed `assistantEvent` was always non-null (Apr 2026). - Framework-level protection check added after PR #1017 (`fix/integrations-search-lag`), where a reviewer flagged a `useEffect` race condition in `useState` ↔ `searchParams` sync that was actually prevented by Next.js's action queue discard mechanism. Reading `app-router-instance.js` source was the only way to verify — official docs were silent on this behavior (Apr 2026). - SDK schema verification, JSONB type awareness, no-`as`-assertion, and metadata merge rules added after PR #919 (`feat/composio-org-level-sync`), where: (1) `orderDirection: 'desc'` was silently stripped by `@composio/core`'s Zod `safeParse` because the param wasn't in the schema; (2) `mergeMetadataJson` used `typeof existing !== 'string'` which always evaluated true for JSONB objects from Kysely, skipping the merge and losing `slackAppId` during tombstone resurrection; (3) `as Record` was rejected by ESLint's `consistent-type-assertions: never` rule, requiring runtime type narrowing instead (Apr 2026). - Specialized helper preference (Step 5 #8) added after PR #1059 (`fix/structured-output-bootstrap-rls`), where the bootstrap path manually built a `SystemDbContext` and called `withSystemDb` instead of using the dedicated `withBootstrapSystemDb(reason, fn)` helper (May 2026). - Sentry log-level threshold and `beforeSend` preference (Step 5 #9 / #10) added after PR #997 (`V2-380/fix-intermediate-messages`) and PR #1039 (`fix/sentry-ignore-fetch-network-failure`). PR #997 surfaced that `logger.warn` (level 40) is dropped by the pino → Sentry hook (threshold >= 50), letting user-visible content drops disappear silently. PR #1039 surfaced that `ignoreErrors` filters dropped on-error replays and tagged diagnostic captures, so noise filters need `beforeSend` with canary sampling + tag-aware bypass instead. Detailed Sentry patterns are in the dedicated `sentry-observability` skill (May 2026). - Pushback reply template (Step 8) added after PR #1038 (`fix/connect-web-27-session-stream-abort`), where declining a `console.warn` on the 499 branch required citing AGENTS.md "swallowing expected errors" and explaining the LB-access-logs alternative. The "investigate-then-report" reply variant comes from PR #997's response pattern around `stripChatTitle`'s built-in `.trim()` (May 2026). - Analysis-checklist additions (Step 3) for buffer-then-flush, append-then-write retry safety, real-DB tests on branched WHERE, OpenAPI `required` + nullable, Server Component callback antipattern, and paired container parity added after the same backend / frontend PRs above (Apr–May 2026). - Sentry checklist additions (framework-class anchor, no 100% blackout, scrub-before-sample, pass-through filter test) and global-error-handling additions (classification ladder, narrow `isTransientNetworkError`, toast cooldown, server flag → Provider) added after PR #1110 (`fix/sentry-response-aborted-filter`), PR #1069 (`fix/global-error-handler-classification`), and PR #1072 (`fix/voice-label-default-voice` SSR-divergence angle). API mapper / multi-turn capture checks come from PR #1116 (`fix/hotel-sms-toggle-state-source-of-truth`) and PR #1071 (`fix(workloads): persist chat title from any assistant turn`). Form-UX checks (picker default normalization, custom-override read-only, optimistic auto-rollback) come from PR #1078 follow-up + PR #1116. Test env-var snapshot pattern comes from PR #1110 (May 2026). - Analysis-checklist additions (Step 3) for legacy URL redirects on page consolidation, e2e page object heading + tab-navigation alignment, and navigation consolidation negative assertions added after PR #1208 (`AP-491/settings-tabbed-navigation`). Consolidating `/team` + `/organisation` into a tabbed `/settings` page surfaced three gaps: (1) deleted routes without `redirects()` → bookmarks 404; (2) e2e `expectLoaded()` matched stale heading and `goto()` landed on wrong default tab; (3) nav test only asserted new item without checking old items were removed (May 2026).