Review Comments

Review PR Comments

Fetch all review comments from a pull request, address each one (fix code, add docs, or explain the rationale), and reply to each comment on GitHub with the resolution.

Steps

1. Fetch comments — use gh api to get all review comments:

   gh api repos/{owner}/{repo}/pulls/$ARGUMENTS/comments \
     --jq '.[] | {id, path, line, body, in_reply_to_id}'

   Filter to top-level comments only (in_reply_to_id == null) — those are the ones that need responses.

2. Understand each comment — read the referenced file and line to understand the concern. Group related comments if they touch the same issue.

3. Address each comment — make the appropriate code change (fix, refactor, add docs, add tests). If you disagree with a suggestion, prepare a clear rationale.

4. Reply to each comment — post a reply using:

   gh api repos/{owner}/{repo}/pulls/$ARGUMENTS/comments -X POST \
     -f body="<resolution>" \
     -F in_reply_to=<comment-id>

   Keep replies concise: state what was done (e.g., "Fixed. Replaced generic fallback with explicit throw.") or explain why it was left as-is.

5. Close every codecov gap — target 100% patch coverage. The Codecov bot posts a PR comment/check; treat its uncovered lines as review comments that MUST be resolved with tests. Every line codecov marks uncovered has to be covered — no exceptions for "pre-existing" files. If a file shows up in the PR diff with uncovered lines, cover them even if you didn't write them.

   a. Find what's actually uncovered. The bot summary only gives totals; pull the per-line data from the Codecov API and intersect it with the PR's added lines (this is exactly what codecov/patch scores):

   # Per-line diff coverage (head_cov == 0 → miss):
   curl -s "https://api.codecov.io/api/v2/github/{owner}/repos/{repo}/compare?pullid=$ARGUMENTS" \
     > /tmp/cc.json
   # Then: for each file with has_diff, list lines where
   #   coverage.head == 0 && is_diff, and intersect with the file's
   #   git-diff added-line numbers (git diff --unified=0 origin/main...HEAD).

   Codecov ignores are in codecov.yml (*.g.dart, *.freezed.dart, l10n/*.dart here) — skip those.

   b. Codecov is often stale/partial — trust local coverage for new code. Coverage is sharded; if any shard failed or the run is mid-flight, the bot shows a partial picture (e.g. "1% of diff") that inflates the miss list with trivial lines (@override, const ctors). Regenerate locally and intersect with the git-diff added lines for the authoritative set:

   fvm flutter test --coverage <the test dirs that exercise the diff>
   # parse coverage/lcov.info: DA:<line>,0 == uncovered

   Run enough test dirs that every diff file is genuinely exercised (a file covered only by tests you didn't run shows as a false miss).

   c. Write real tests for each uncovered line — mirror the sibling case already tested (e.g. a new .map/switch arm → copy the aiConfig case for savedTaskFilter; a debounce-cancel line → emit two notifications so the timer is non-null when cancelled). Extend parameterized variantCases/variantsByBucket tables rather than duplicating whole test bodies.

   d. Uncoverable private-ctor lines (const FooKeys._(); in a static-only keys class) can't be hit and dart coverage ignores no inline comment — convert the class to abstract final class FooKeys { ... } so the constructor line disappears entirely.

   e. Re-run coverage until the (added ∩ uncovered) set is empty, then run the affected suites to confirm still-green.

6. Verify — run analyzer and affected tests to confirm all changes compile and pass.

7. Babysit the PR until it is actually done. Opening a PR and replying once is not the end of the job. A PR is finished only when all three hold at the same time:

   • mergeable — gh pr view <n> --json mergeable is MERGEABLE
   • all green — every check passed, not merely "not failing": zero pending and zero failures, including codecov/patch
   • all replied — every top-level review comment has a reply, with a real fix or a stated reason for declining

   Pushing requires authorization. Commits, rebases and pushes need explicit user or orchestrator approval (AGENTS.md, "Issue Tracking"). Being asked to address review comments authorizes the code changes, not the push — confirm before the first push of a session, and never force-push a branch you did not create in this session. --force-with-lease guards against clobbering a concurrent update; it is not a substitute for approval.

   Reviews arrive after pushes, so pushing fixes restarts the loop: the reviewer re-reviews the new commit and may file new findings. Bots also rate-limit and arrive late (CodeRabbit will say "next review available in N minutes" and skip the run entirely). Keep watching until the three conditions hold together.

   Poll on the structured status rather than the display columns — the table format is human-facing, and a failed API or auth call prints to stderr and would otherwise read as "no pending checks".

   Note gh pr checks has no --json flag (checked on gh 2.45); the structured source is gh pr view --json statusCheckRollup. Verify whatever command you poll with actually works before wrapping it in an until loop: a command that errors makes the loop exit immediately and every subsequent report a lie.

   For a CheckRun, status is the lifecycle (COMPLETED) and conclusion carries the verdict (SUCCESS / FAILURE / CANCELLED) — a failed check is COMPLETED, so keying on status alone reports a red run as done and green. Read the conclusion for completed checks and the state for StatusContexts:

   # one "<verdict>\t<name>" line per check; empty output means the query
   # failed, not that everything passed — so treat it as not-done.
   rollup() {
     gh pr view "$1" --json statusCheckRollup --jq '
       .statusCheckRollup[]
       | if .status == "COMPLETED" then .conclusion
         elif .status then .status
         else .state end
       + "\t" + (.name // .context)'
   }
   # Capture once per iteration and check the exit status: an errored or
   # empty result is "not done", never "done and green".
   while :; do
     out=$(rollup <n>) || { echo "poll failed"; sleep 30; continue; }
     [ -n "$out" ] || { echo "empty rollup — treating as not done"; sleep 30; continue; }
     printf '%s\n' "$out" | grep -qE '^(IN_PROGRESS|QUEUED|PENDING)' || break
     sleep 30
   done
   bad=$(printf '%s\n' "$out" | grep -vE '^(SUCCESS|NEUTRAL|SKIPPED)')
   [ -z "$bad" ] && echo "all green (${#out} bytes of verdicts)" || printf '%s\n' "$bad"

   Three ways these snippets lie if written casually, all worth guarding: an errored command inside $( ) yields empty output that a grep -q pending reads as "nothing pending"; grep … || echo "all green" turns no output at all into a pass; and a background poller whose result is never collected lets the summary be written before it finishes. Capture the output, check the status, and wait for the poller before reporting.

   Cross-check the total against gh pr checks <n> before declaring green — a poller that exits on its first iteration otherwise reports "settled" while checks are still queued.

   Run that in the background (run_in_background: true, or … & with the PID kept) so replying to comments proceeds concurrently rather than blocking on CI.

   Then re-check for comments filed against the new commits — including top-level ones with no reply yet:

   gh api returns one page (30 comments) unless --paginate is passed, and --jq then runs per page — so a comment and its reply landing on different pages makes an answered comment look unanswered, and a comment on a later page look absent. Fetch every page and aggregate once with jq -s:

   # `set -o pipefail` so an API failure fails the pipeline instead of
   # producing an empty list that reads as "nothing unanswered".
   ( set -o pipefail
     gh api --paginate repos/{owner}/{repo}/pulls/<n>/comments --jq '.[]' | jq -s -r '
       [.[] | select(.in_reply_to_id != null) | .in_reply_to_id] as $replied
       | [.[] | select(.in_reply_to_id == null)] as $top
       | "top-level: \($top | length), answered: \([$top[] | select(.id as $i | $replied | index($i))] | length)",
         ($top[] | select(.id as $i | ($replied | index($i)) | not)
          | "UNANSWERED \(.id) \(.user.login) \(.path)")'
   ) || echo "comment query FAILED — do not report all-replied"

   Print the counted totals, not just the unanswered lines: "top-level: 26, answered: 22" is checkable, whereas empty output is indistinguishable from a query that never ran.

   Post replies with -F body=@file rather than an inline shell string. Review bodies contain backticks, quotes and code fences; nested shell quoting silently mangles them, and a failed POST inside a loop can still look like it succeeded.

   Report the real state — "27 pass, 1 pending" is the honest answer while a check is still running, not "all green".