Add a skill to review prs

Author: tung2744

.claude/skills/review-pr/SKILL.md

@@ -0,0 +1,80 @@
+---
+name: review-pr
+description: Produce a structured code review report for the current branch's PR (purpose, API interface changes, code quality, bugs, security, performance), with a mandatory second verification pass. Use when the user asks "what does this PR do", "review this branch/PR", or asks to confirm/double-check review findings and look for more issues.
+argument-hint: "[remote/branch to compare against, defaults to upstream main]"
+---
+
+Produce a code review report for the commits on the current branch that are not yet on the upstream main branch. This skill has three phases: scoping, an initial report, and a mandatory verification pass that re-derives every finding with an objective check (not re-reading the same code and nodding along).
+
+## Phase 1: Scope the PR correctly
+
+Do NOT assume the local `master`/`main` branch is up to date — in this repo it is frequently stale by hundreds of commits. Establish the true upstream base first:
+
+1. Run `git remote -v` and identify the canonical upstream remote (the one pointing at `authgear/authgear-server`, conventionally named `authgear`; ask the user if ambiguous or absent).
+2. `git fetch <upstream-remote> main --quiet`
+3. `git merge-base HEAD <upstream-remote>/main` — this is the true base, regardless of what local `master` points at.
+4. `git log --oneline <base>..HEAD` to enumerate exactly the commits in scope. Sanity-check this list against `git log --oneline -5` from the repo status — if the top commits don't match what the user expects, stop and re-check the remote/branch choice before proceeding.
+5. `git diff --stat <upstream-remote>/main..HEAD` — if the stat includes files unrelated to the commit subjects (e.g. translation files, unrelated scripts), that means upstream `main` has advanced independently on other work; this is normal and not part of the PR. Don't let it inflate the reported scope — cross-check with `git show --stat <commit>` for each commit in step 4 individually to get the true per-commit file list.
+
+## Phase 2: Write the report
+
+Structure the report under these six headings. Base every claim on an actual diff read (`git show <commit>`, `git diff <base>..HEAD -- <path>`), not on commit message text alone — commit messages describe intent, not necessarily what the code does.
+
+1. **Major purpose** — 3-6 sentences synthesizing what the set of commits accomplishes and why, grouped by theme if the commits span multiple concerns.
+
+2. **API interface changes** — enumerate concretely:
+   - GraphQL: new/changed types, fields, args in `schema.graphql` and the resolver file that backs them (`pkg/admin/graphql/`, `pkg/portal/graphql/`)
+   - Go: new/changed exported struct fields, function signatures in `pkg/lib/`
+   - HTTP/config: new endpoints, changed request/response shapes, `authgear.yaml` schema changes
+   - Note whether each change is additive (safe) or a rename/removal (breaking)
+
+3. **Code quality issues** — look specifically for:
+   - Formatting/lint violations: run `gofmt -l <changed .go files>` and `gofmt -d` on any that fail; for frontend, note `npm run typecheck`/`lint` if easy to run
+   - i18n regressions: hardcoded user-facing strings in `portal/src` or `authui/src` that bypass `renderToString`/`FormattedMessage`/locale-data, including things like `Intl.DisplayNames`/`Intl.NumberFormat` calls hardcoded to a fixed locale instead of the active one
+   - Dead/unreachable code introduced by the diff (e.g. a switch branch that can never be hit given the other cases)
+   - Regressions in refactors: a prop/constraint/validation that existed on one code path before the change and silently disappeared on a new code path introduced by the same diff (compare old branch vs new branch of an `if`/ternary the diff introduced)
+
+4. **Bugs** — trace actual runtime behavior, don't just eyeball it. Prioritize:
+   - Off-by-one and boundary math in date/time range logic
+   - Label/copy vs. implementation mismatches (does the UI string still describe what the code now does after the diff?)
+   - State persistence logic that can't distinguish "never set" from "explicitly set to empty/default" (common in localStorage-backed preference code: `if (parsed.length === 0) return defaults` silently overrides a deliberate "clear all" action)
+   - Validation/constraints dropped when a component is swapped or a new branch is added (e.g. a `min`/`max` prop supported by the old component but not the new one it was replaced with in some branch)
+
+   For each bug, state the concrete failure scenario (inputs/state → wrong output), not just "this looks suspicious."
+
+5. **Security issues** — check every new/changed handler, resolver, and query for:
+   - **Authorization/tenant scoping**: does every new GraphQL resolver, Admin API handler, or Site Admin handler enforce the same authz/role checks as sibling resolvers, and is every DB query scoped by `app_id`/tenant so one tenant cannot read or mutate another's data (IDOR)?
+   - **Injection**: are all new SQL queries built through the existing query builder / parameterized placeholders (`db.SelectBuilder`, `?`/`$1` placeholders) with no raw string concatenation of user input, including inside JSONB path expressions (`data#>>'{...}'`) where a dynamic path segment could come from user input?
+   - **Secrets/PII exposure**: does the diff log, return in an error message, or expose in a GraphQL field anything that should be redacted (tokens, full phone numbers/emails beyond what's already exposed, internal IDs that leak cross-tenant info)?
+   - **SSRF/webhook risk**: if the diff adds or changes an outbound HTTP call (webhooks, image fetch, redirect URL), is the target validated/allow-listed the same way existing outbound calls are?
+   - **XSS/output encoding**: for AuthUI template or portal-rendered HTML changes, is user-controlled data passed through the existing escaping helpers rather than raw-inserted?
+   - **Input validation**: are new config fields, GraphQL args, and query params validated (length, enum membership, range) consistently with sibling fields, before use in a query or written to config?
+   - Treat findings here as high-priority even if the report has few other findings — call out explicitly if a check produced no findings ("no authz regressions found in the resolvers touched by this diff") rather than omitting the section.
+
+6. **Performance issues** — check for:
+   - N+1 or repeated-round-trip patterns: multiple independent DB queries derived from the same base query/filter that could be one query or run concurrently, especially in hot-path screens (dashboards, list views)
+   - Missing bounds: new queries/list endpoints without a `LIMIT`/pagination cap, or unbounded time ranges that can return arbitrarily large result sets
+   - Missing indexes: new `WHERE`/`GROUP BY`/`ORDER BY` columns on large tables (`_audit_log` and other high-volume tables) that aren't covered by an existing index — check `pkg/lib/infra/db/migration/` for the relevant table's index list
+   - Frontend: new state/effects that cause avoidable re-fetches or re-renders (e.g. a query re-running on every keystroke without debounce, a `useMemo`/`useCallback` dependency array that's broader than necessary), and large/blocking computations on the main thread for large datasets
+
+## Phase 3: Verification pass (mandatory, do not skip)
+
+Re-derive every reported bug, security issue, and quality issue marked as objective (formatting, build errors, missing index) using an independent check — do not just re-read the code a second time and confirm your own reasoning:
+
+- **Go formatting/build claims**: run `gofmt -l`/`gofmt -d` and `go build ./<affected packages>/...` (and `go vet` if suspicious) and quote the actual output.
+- **Date/time/off-by-one bugs**: simulate the exact logic in isolation. For TypeScript/luxon logic, use `node -e` with the real library from `node_modules` (e.g. `require('luxon')` from the `portal/` directory) reproducing the exact function with representative inputs, and print the actual result — don't hand-derive it only in prose.
+- **State-machine/persistence bugs**: simulate the save/load round trip with representative inputs (empty selection, partial selection, malformed data) in a small inline script and show the actual output for each case.
+- **Prop/constraint-loss bugs**: grep the replacement component's prop types/interface to confirm the constraint genuinely has no equivalent (not just "wasn't passed in this call site") — cite the exact interface definition.
+- **Authz/tenant-scoping claims**: grep sibling resolvers/handlers for the authz call or `app_id` filter they use, and confirm the new code either has the matching call or is genuinely missing it — cite both the sibling's pattern and the new code's diff.
+- **Injection claims**: cite the exact line constructing the query/path and confirm whether the interpolated value is attacker-controlled input or a fixed/internal constant.
+- **Index/performance claims**: grep the migration files for the touched table to confirm whether an index covering the new query's filter/group columns exists or not — cite the migration file, don't guess.
+- Re-check `git diff --stat` scope once more against `git show --stat` per commit to make sure no claim is actually about upstream `main`'s unrelated churn rather than this PR's diff.
+
+While verifying, also spend one pass looking at any files that were touched by the PR but not yet fully read (check the per-commit `--stat` output from Phase 1 against what's actually been opened) — new bugs are often in the files that got the least attention in Phase 2.
+
+## Output format
+
+Present the final report as:
+- The six Phase 2 headings, each finding stated concretely with a `file:line` reference. Include the Security and Performance headings even when empty — state explicitly that no issues were found rather than omitting the section.
+- For a follow-up verification request specifically, present bugs/quality/security/performance issues as: original claim → verification method used → outcome (confirmed / refined / retracted), followed by any newly found issues from the extra pass.
+- Keep prose tight — this is a report to act on, not an essay.