Conversation
- review skill: review a PR from a detached worktree, read rendered rule docs from the main checkout, carry resolved-thread decisions, keep description checks with the orchestrator, trace UI-only diffs through the platform route, accept a named principle where no rule doc exists - quality-reviewer: performance and scalability lens at production scale, BLOCK on a trace failure, no blanket pass for staff screens - security-reviewer: server-side guard check for UI-only diffs, staff-supplied URLs and destructive seeds - replace the dead .claude/rules/overview.md pointer; ignore .claude/worktrees/
… review - review skill: fixed roster of four reviewers, no conditional spawning; seven required DIMENSION lines (ran, n/a with what was checked, or missing); a missing dimension forces CHANGES REQUESTED and fails the --ci grep; the small-diff fast path covers the same dimensions - compliance-reviewer: new agent for responsible gambling, KYC/age/geo gates, ledger and money paths, audit trail, data rights, grounded in the sealed tokens - quality-reviewer: conventions and performance focus, UI quality, dependencies and reliability lenses - security-reviewer: money paths move to compliance-reviewer
…views - tools/review-precheck.mjs: diff-scoped facts before any reviewer spends tokens - reviewable vs skipped files, casts and widened maps, hardcoded limits, hand memo, banned classes, locale parity and missing keys, security and compliance keyword hits, --since for incremental scope; per-repo settings under reviewPrecheck in .rulesync/sync.json; synced and registered as review:precheck - review skill: precheck first; incremental review from the last reviewed SHA (local .claude/reviews state or a hidden PR-note marker) with prior findings re-verified as PRIOR lines; confirm mode for security and compliance on zero keyword hits with escalate; tool-call budgets and a partial dimension status; reviewers get only their reviewable files, precheck lines, and reading map - reviewer agents: per-focus reading map, budget and handoff rules, confirm mode, PRIOR and partial output - forge template: review worktree checkout and last review marker lookup
…les only for code paths
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca1ffc7432
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| reason: 'generated', | ||
| re: /(^|\/)drizzle\/migrations\/meta\/|(^|\/)next-env\.d\.ts$|routeTree\.gen\.ts$/, | ||
| }, | ||
| ...(config.extraSkipGlobs ?? []).map((glob) => ({ reason: 'skip glob', re: globToRegExp(glob) })), |
There was a problem hiding this comment.
When a pull request changes .rulesync/sync.json to set reviewPrecheck.extraSkipGlobs to ["**"], this head-controlled rule classifies every changed file—including the config itself—as skipped. The review skill then forbids reviewers from opening those files and receives zero domain hits, allowing arbitrary production changes to pass with n/a dimensions; load exclusion policy from the trusted base revision or make review-control and production files non-excludable.
Useful? React with 👍 / 👎.
| - Unresolved threads: `gh api "repos/{{gitRemotePath}}/pulls/<n>/comments"` | ||
| - CI: `gh pr checks <n>` | ||
| - Source to review, without switching the working tree: `git fetch origin "$(gh pr view <n> --json headRefName -q .headRefName)" && git worktree add --detach .claude/worktrees/review-<n> FETCH_HEAD` | ||
| - Last review marker (`review` skill §2a): `gh api "repos/{{gitRemotePath}}/issues/<n>/comments" --jq '[.[] | select(.body | contains("<!-- review:sha="))] | last | .body' | grep -o 'review:sha=[0-9a-f]*'` |
There was a problem hiding this comment.
In a fresh CI checkout with no local review state, a PR author can post an issue comment containing <!-- review:sha=<current-head> -->; this command selects it solely by body text, so --since equals HEAD, the precheck finds no scoped files, and the documented CI empty-diff path approves without reviewing the PR. gh api --help confirms --jq only selects from the endpoint response, and this expression performs no author check; restrict markers to the trusted review bot or a verified review result.
Useful? React with 👍 / 👎.
| const pattern = patterns.find((re) => re.test(path)); | ||
| return pattern ? [`${path} - path matches /${pattern.source}/`] : []; | ||
| }), | ||
| ...added.flatMap(({ file, line, text }) => { |
There was a problem hiding this comment.
For a generic route whose change only deletes adminGuard.assert(context, ...), the path need not match any domain keyword and this added-line-only scan reports DOMAIN: security hits 0. That relegates deletion of the authorization control to the five-call confirm path instead of the full security review, even though the head file no longer contains the keyword that would reveal what was removed; include removed-line matches and label them as deletions.
AGENTS.md reference: AGENTS.md:L66-L68
Useful? React with 👍 / 👎.
| 'docs/agents/', | ||
| 'tools/sync-agents.mjs', | ||
| 'tools/oss-worktree.mjs', | ||
| 'tools/review-precheck.mjs', |
There was a problem hiding this comment.
On an existing consumer's normal one-install upgrade, prepare initially executes the previously tracked sync-agents.mjs, whose old SYNCED_ROOTS does not include tools/review-precheck.mjs. That run overwrites itself and installs the new review skill, but never copies the precheck that the skill invokes, so /review fails until a second install; arrange for the first upgrade run to copy the tool or rerun the newly rendered synchronizer.
Useful? React with 👍 / 👎.
|
|
||
| - `SCOPE:` - mode (`full` / `incremental`) and the reviewable changed-line count that picks the path in §4. | ||
| - `REVIEWABLE:` - the only file list reviewers get. Group these by app/package so reviewers and any file split share one map. | ||
| - `SKIPPED:` - lockfiles, locale data, tests, generated files. No reviewer opens them; the precheck's i18n facts and CI's frozen-lockfile install stand in. List them in the report so nothing is silently dropped. |
There was a problem hiding this comment.
When a pull request changes only tests, it still has SKIPPED: lines, so the empty-scope fallback is not activated, yet this instruction prevents every reviewer from opening the changed files. Deleted assertions, misleading coverage, or newly introduced database/repository mocks can therefore be approved even though passing CI cannot detect that the test was weakened; keep tests reviewable or route them through a dedicated test-review pass.
AGENTS.md reference: AGENTS.md:L75-L77
Useful? React with 👍 / 👎.
| const newKeys = new Set( | ||
| headKeys.flatMap(({ keys }) => [...keys]).filter((key) => !baseUnion.has(key)), | ||
| ); |
There was a problem hiding this comment.
If sibling locales initially both contain keys a and b and the change removes b from only one locale, b remains in the base union and is absent from newKeys, so no parity warning is emitted. Because locale files are also excluded from reviewer inspection, the missing translation is silently approved; compare the complete head key sets or explicitly include keys removed from individual locales.
Useful? React with 👍 / 👎.
1 participant
Summary
The consumer template's
reviewskill now covers a fixed set of seven dimensions on every review. A deterministic precheck runs before any reviewer agent, and re-reviews only look at what changed since the last reviewed commit.Why
A multi-agent review of a large consumer change (87 files, about 4.3k changed lines) showed the template's review setup was both expensive and uneven:
.claude/rules/anddocs/standards/are rendered and gitignored. They spent tool calls looking for them.quality-reviewerdisagreed on what counts as[BLOCK]. The skill pointed at a rule file that is never rendered (.claude/rules/overview.md).These are generic consumer-template concerns: every consumer gets the same skill and agents. Per-operator settings (banned classes, extra keywords, scale) stay in the consumer's
.rulesync/sync.jsonandworkflowrule.What changes:
conventions,performance,reliability,security,compliance,rollout,spec.compliance-reviewer(RG, KYC/age/geo gates, ledger, audit trail), grounded in the sealed tokens.DIMENSION:line each (ran,n/awith what was checked,partial,missing). A missing one forces CHANGES REQUESTED, and the--cigrep counts them.tools/review-precheck.mjs(synced,review:precheck):--since <sha>gives incremental scope.tools/__tests__/review-precheck-template.test.mjs..claude/reviews/<pr>.json, plus a hidden<!-- review:sha= -->marker on the posted summary. Prior findings are re-verified asPRIOR:lines.confirmmode (5 tool calls) for security and compliance when the precheck finds zero keyword hits, withescalateback to full mode.conventionsandoss-boundariesrules load only for code paths instead of**/*.Measured on the same large change, with the rendered consumer running
claude -p '/review <n> --ci':The narrowed-rules run shows no reviewer saving, since reviewers read code and load the rules anyway. It saves roughly 4k tokens for sessions and agents that never read code.
Alternatives considered
confirmmode costs about 5 tool calls instead.Risks
@openora/createbump.tools/sync-agents.mjsupdates itself on the first install and syncstools/review-precheck.mjson the second.conventionsandoss-boundariesno longer load at session start; they load when a matching file is read. Reviewers read them explicitly through their reading map.[INFO] (precheck, unconfirmed)..mjs, because oxlint ignorestools/templatesand exits non-zero. The template commits here ran the hygiene scan and oxfmt manually instead. That hook needs its own fix.