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:

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.<service>_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:

## 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<string, unknown> 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:

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:

## 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:

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<string, unknown> 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).