Repository navigation
Conversation
…-only pull requests A change to a file config.yml's sensitive: matches runs correctness and security on Opus for the parts of the diff with it, and verification on Opus for its findings. A documentation-only pull request, or a template update of template-owned files, runs hygiene alone on Haiku with no context, selection or synthesis. CODE_REVIEW_TIERS still overrides any reviewer, and the check run's reviewer table shows each reviewer's models. Closes #42
![ncode-review[bot]](https://avatars.githubusercontent.com/u/12114478?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 NCode Review at 0b94102: 6 new findings on the changed lines.
🤖 Review completeVerdict: 💬 Comment — This PR adds review depths. Changes to 8 reviewers run — 7 findings (run details)
Skipped: api-compat, data-model, infrastructure, intent, performance Findings
Trial: findings are posted as review threads and in the check run. $0.72 on Amazon Bedrock, 86s, at 0879cc8. (how to interact) |
A file with a code or script extension, or a git hook, never counts as documentation or as a template-owned file a template update can change, so a pull request touching one gets the standard review even in docs/ or alongside .template-version. Also tests a deep review whose floor already chose security, and fixes a stray colon in the partTiers comment.
There was a problem hiding this comment.
🤖 NCode Review at 0879cc8: 7 new findings on the changed lines.
| // Code or a script that runs, wherever it is: a git hook, or a file with a code or script extension, even in docs/ or a | ||
| // template update. A light review never takes it. | ||
| const executablePath = /\.(cs|fs|ts|tsx|js|mjs|cjs|py|go|rs|java|ps1|psm1|psd1|sh|bash|zsh|cmd|bat|rb|pl|tf|tofu|hcl)$|(^|\/)\.githooks\//i; | ||
|
|
There was a problem hiding this comment.
🟠 Should fix: Light-review eligibility rests on an incomplete denylist of executable extensions F1 · architecture, security
documentationOnly counts any file under doc/ or docs/ as documentation unless executablePath lists its extension. With .template-version in the PR, it also counts any file of any type under .claude/commands/ or .github/(agents|prompts|skills)/. Unlisted types still pass: .jsx, .php, .c/.h, .kt, .swift, .json, .yml, .sql, .csproj, Dockerfile, Makefile and files with no extension.
Consequence: A contributor can add .template-version plus a payload such as .github/skills/x/run, or an unlisted script under docs/, and get only a Haiku hygiene review: no correctness, security, synthesis or verification. Prompt-injection text in skill or command files also gets only the weakest model. The list has to be extended after every miss.
Fix: Make the light review an allowlist. Accept documentation extensions (.md, .txt, .rst) and image or asset types under docs folders, and template-owned files only when they are .md (plus the two version files). Treat any other extension, or no extension, as standard. Reuse productCode/infrastructurePath rather than keeping a separate list.
Evidence
executablePath is a denylist applied after isDocumentation(f.path), whose docPattern (^|\/)docs?\/ matches any file under docs/. The templateOwned patterns /^\.github\/(agents|prompts|skills)\// and /^\.claude\/(agents\/plan-executor\.md$|commands\/)/ accept any file type.
Confidence 0.45.
There was a problem hiding this comment.
Autopilot: fixed in #85 (310fd32): confirmed. Under the denylist, a template update could take any non-Markdown file under .github/skills/, .github/prompts/ or .claude/commands/, and any unlisted type in docs/ (.json, .yml, Makefile, .php, no extension) got the light review. It is now an allowlist: .md/.txt/.rst anywhere, images and PDFs in doc(s)/, and the two template version files. Everything else gets the standard review, and so do PublicAPI.*.txt, CMakeLists.txt and pip requirements and constraints files. The separate template-owned and executable lists are gone.
| ]; | ||
| assert.equal(reviewDepth(update, []), 'light'); | ||
| assert.ok(documentationOnly([file('.copier-answers.yml')])); | ||
| }); |
There was a problem hiding this comment.
🟡 Nit: No test for a script inside template-owned folders F2 · tests
The script exclusion is tested only with .ps1/.sh files and a git hook. No test pins a .py or .js file under .github/skills/ or .claude/commands/ in a template update.
Consequence: A change to executablePath or templateOwned could quietly turn such a PR into a light review without any test failing.
Fix: Add a case: reviewDepth([file('.template-version'), file('.github/skills/x/scripts/run.py')], []) returns 'standard'.
Evidence
The 'reviewDepth is standard for a template update that changes a script or git hook…' test uses only .template/Update-Project.ps1, .githooks/commit-msg and docs/*.ps1/.sh.
Confidence 0.35.
There was a problem hiding this comment.
|
|
||
| test('a deep review goes deep only on the parts of the diff with a sensitive file', async () => { | ||
| const input = gathered(['src/Reports/Chart.cs', 'src/Payments/Ledger.cs'], { sensitive: ['src/Payments/**'], chunks: [section('src/Reports/Chart.cs'), section('src/Payments/Ledger.cs')] }); | ||
| const { result, calls } = await run(input); |
There was a problem hiding this comment.
🟡 Nit: No test for a light review whose hygiene call failed but returned findings F3 · tests
The light branch sets the verdict to 'comment' only when failed && !findings.length. No test covers a hygiene call that fails but still returns findings.
Consequence: A change to that condition could approve an unfinished review, or downgrade a valid verdict, without any test failing.
Fix: Add a light-review test where hygiene returns a blocker and also errors, and assert the verdict and summary.
Evidence
src/review.ts light branch: if (failed && !findings.length) verdict = 'comment'. The existing failure test returns no findings.
Confidence 0.30.
There was a problem hiding this comment.
Autopilot: fixed in #85 (310fd32): this exact case can't happen. A reviewer's status is failed only when all of its calls failed, and then it has no findings. Writing the test did turn up a real gap: in a multi-part light review where hygiene failed on one part, the summary didn't mention the failure, and with no findings the review approved. It now says hygiene failed on 1 of 2 parts of the diff (...), and comments instead of approving. Tests cover this with no findings and with a blocker, which still requests changes.
| for (const name of depthEnv) delete process.env[name]; | ||
| Object.assign(process.env, options.env ?? {}); | ||
| try { | ||
| const result = await review({ provider: 'bedrock', proxyUrl: 'http://127.0.0.1:1' }, input, charters, () => {}, agent); |
There was a problem hiding this comment.
🟡 Nit: Plural wording and truncation of the sensitive-file list are untested F4 · tests
checkSummary picks 'matches' or 'match' by file count, but only the single-file case is asserted. The multi-file wording and the 20-file truncation are not exercised.
Consequence: A regression in the plural wording or the truncation in the check run would go unnoticed.
Fix: Add a checkSummary assertion with two sensitive files that expects 'match sensitive:', and one that exceeds the truncation limit.
Evidence
src/results.ts: (results.sensitiveFiles ?? []).length === 1 ? "matches" : "match".
Confidence 0.30.
|
|
||
| Most pull requests get the **standard** review above. Two kinds get another ([#42](https://github.com/NCodeGroup/code-review/issues/42)), named at the start of the summary and in the check run: | ||
|
|
||
| - **Deep review (sensitive paths).** When a changed file that isn't generated matches a `sensitive:` glob in the repository's [settings](#repository-settings), the parts of the diff with such a file get `correctness` and `security`, added if selection didn't choose them, on Opus instead of Sonnet, and verification, when it's on, checks the findings in those files on Opus. The other parts of a large diff keep their usual models. In the evals, Opus reviewers caught four times what the all-Haiku baseline did, at about twice Sonnet's cost, so Opus is kept to the code a repository marks sensitive. The check run's Model column shows each reviewer's models, both when its parts differ. Documentation in a sensitive folder makes the review deep too, not light. |
There was a problem hiding this comment.
🟡 Nit: Unclear sentence about the Model column F5 · hygiene
In "The check run's Model column shows each reviewer's models, both when its parts differ", the word 'both' has nothing to pair with, so the sentence doesn't say when more than one model is shown.
Consequence: Readers can't tell what the Model column shows, which is the behavior the sentence is meant to document.
Fix: Rewrite as: "The check run's Model column lists every model a reviewer ran on, so a reviewer whose parts differ shows more than one."
Evidence
callModels() in src/results.ts returns every distinct model for a role.
Confidence 0.40.
|
|
||
| // Each reviewer's model on one part of the diff. On a deep review's sensitive parts, correctness and security go to | ||
| // Opus over the repository's tiers: setting. CODE_REVIEW_TIERS still overrides any reviewer, and anyone unnamed runs on Haiku. | ||
| export function partTiers(depth: ReviewDepth, deepPart: boolean, repository: Record<string, ModelTier>, override?: string): Record<string, ModelTier> { |
There was a problem hiding this comment.
🟡 Nit: Could partTiers take fewer parameters? F6 · rule-check
partTiers(depth, deepPart, repository, override) takes four positional data parameters, and depth and deepPart always travel as a pair. The parameter-count rule comes from C# guidance, so it may not apply to this TypeScript code.
Consequence: Readability only. Callers and tests pass four positional arguments, including '' for override, which makes calls easy to get wrong.
Fix: If you agree, merge depth and deepPart into one per-part depth value, or pass the overrides as a record the way reviewerTiers does.
Evidence
Rule: at most 2–3 data parameters. Signature: partTiers(depth, deepPart, repository, override?).
Confidence 0.30.
There was a problem hiding this comment.
Autopilot: declined: the 2-3 parameter rule is C# guidance. partTiers is an internal pure function with one production caller, and its tests read clearly with positional arguments. Merging depth and deepPart would only move the branching into the caller.
| - Per-repository settings in `.github/code-review/`, read from the default branch's checkout only, so a pull request that edits them is reviewed with the default branch's copy: `config.yml` (`sensitive:`, `skip:`, `policies:`, `tiers:`), `guidance.md` rules for the context stage, and `reviewers/*.md` project reviewers that join the roster as `project:<name>`. Files `skip:` leaves out are listed in the check run as skipped by config, and a pull request whose every file it skips runs no model; policies reach every reviewer's conventions within a budget of their own, and a setting the engine can't use, including a `tiers:` name that matches no reviewer, is a warning in the check run, never a failed review ([#43](https://github.com/NCodeGroup/code-review/issues/43)). | ||
| - A per-review cost ceiling guards against a runaway review: $8 plus $4 for each part of the diff, overridable with `CODE_REVIEW_MAX_RUN_USD`. Reviewer calls not started once it is reached are skipped and shown as `skipped: run budget` in the check run, synthesis still runs on what was found, and the summary warns that the review is incomplete ([#50](https://github.com/NCodeGroup/code-review/issues/50)). | ||
| - A verification stage, off by default: with `CODE_REVIEW_VERIFICATION=on`, a Sonnet call with the checkout proves or refutes each merged blocker and should-fix, a file's findings at a time at up to $0.50 a call. Confirmed findings get the code path in their evidence, refuted ones are dropped and listed with the reason under "Refuted by verification" in the check run and in the trace, and ones it can't settle become questions one severity lower. The summary names the refuted findings, and verification turns a request for changes with nothing serious left into a comment, never an approval. Nits aren't verified, and the calls show as `verification` in the cost table. It stays off until the eval shows a gain, and the eval report now counts posted blockers and should-fixes and refuted findings ([#41](https://github.com/NCodeGroup/code-review/issues/41)). | ||
| - Deep and light reviews. A pull request that changes a file `sensitive:` in `.github/code-review/config.yml` matches gets `correctness` and `security` on Opus for the parts of the diff with it, `security` added if selection didn't choose it, and verification on Opus for the findings in those files; the summary and check run say "Deep review (sensitive paths)". A pull request that changes only documentation, the changelog, or a template update's template-owned files, with no code, script or git hook among them, gets `hygiene` alone on Haiku, with no context, selection, synthesis or verification call, its findings posted as reported, and says "Light review (documentation only)". `CODE_REVIEW_TIERS` still overrides any reviewer's model, and the check run's reviewer table has a Model column that lists each model a reviewer ran on ([#42](https://github.com/NCodeGroup/code-review/issues/42)). |
There was a problem hiding this comment.
🟡 Nit: Should the description close #42? F7 · conventions
This PR implements #42 and removes the README note that sensitive: doesn't change reviews yet, but the PR description is empty, so it has no Closes #42.
Consequence: Issue #42 stays open after merge even though the docs no longer describe the work as pending.
Fix: Add Closes #42 to the PR description, unless follow-up work under #42 remains.
Evidence
Rule: a PR that completes a tracked pending-work item includes Closes #N in its description (repository.instructions.md via ADR-0264).
Confidence 0.30.
There was a problem hiding this comment.
Autopilot: declined: nothing to change. The description has Closes #42, added after this review ran, and #42 is closed.
Stacked on #69; GitHub retargets this to main when #69 merges.
A pull request that changes a file
sensitive:in.github/code-review/config.ymlmatches now gets a deep review:correctnessandsecurity(security always added) on Opus for the part(s) of the diff with that file, and verification (when on) on Opus for findings in sensitive files. A documentation-only pull request (gather's doc pattern, CHANGELOG.md, or a template update of template-owned files) runshygienealone on Haiku with no context, selection, synthesis or verification call, and posts its findings directly. The summary and check run say "Deep review (sensitive paths)" or "Light review (documentation only)", and the check run's reviewer table gains a Model column.CODE_REVIEW_TIERSstill overrides any reviewer. No small-code light tier.test/depth.test.tscovers the tier choice for deep, light, standard and override.Decisions
tiers:, andCODE_REVIEW_TIERSoverrides both.correctness/securityon Opus; the other parts keep their usual reviewers and models. If a sensitive file is too large to fit in any part, the deep reviewers go on the first part, where they can read the file from the checkout..template/,.template-version,.githooks/commit-msg,CLAUDE.md,.claude/agents/plan-executor.md,.claude/commands/,.github/{agents,prompts,skills}/,project-template.instructions.md, plus.copier-answers.yml). These count toward the light review only when.template-versionor.copier-answers.ymlchanges too. Merged files such as CI workflows keep the standard review.PublicAPI.*.txtisn't counted as documentation, even though it matches.txt, so API changes keepapi-compat.Closes #42