Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
- 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)).
- When a pull request closes, merged or not, each finding posted as a thread gets an outcome, by rules alone with no model call or model key: fixed (its line changed afterwards, or its thread is outdated and resolved), dismissed (a writer's 👎, resolved without a change, or a writer's declining reply), acknowledged (a writer's 👍 or reply) or ignored. The newest summary comment, or a final one, gets a table of finding, reviewer, severity and outcome with the pull request's state, and a hidden `<!-- ncode-review:outcomes {...} -->` marker that also holds each finding's review cost, provider and model (as many findings as fit in a comment, counting the rest). Summary comments now record their review's cost, provider and models in a hidden marker that a superseded summary keeps, along with its outcomes marker. A closed pull request gets no acknowledgement and no "Code review" check run. Callers need `closed` in their `pull_request_target` types, added with the pin bump to this release, since earlier releases would review a closed pull request; the README's caller has it, this repository's workflow gets it with that bump, and a closed run waits for a review in progress instead of cancelling it ([#47](https://github.com/NCodeGroup/code-review/issues/47)).
- A monthly report: on the 1st, the `Monthly report` workflow reads the pull requests closed in the month just ended in each NCodeGroup public repository with `.github/workflows/code-review.yml`, and opens the issue "Review report YYYY-MM" with precision per reviewer and per repository (fixed ÷ (fixed + dismissed), with acknowledged and ignored shown apart), findings per pull request, the total cost split between the Claude API and Amazon Bedrock, the cost per fixed finding, and the change in each from the previous month's report. Findings come from the outcomes marker and cost from the summary comments' review markers, from the bot's comments only. A re-run updates the month's issue instead of opening another, a manual run takes a month and a list of repositories, and it needs only the workflow's `GITHUB_TOKEN` ([#48](https://github.com/NCodeGroup/code-review/issues/48)).
- 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)).
- 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, by an allowlist (`.md`, `.txt` and `.rst` files, the changelog among them, and images and PDFs in a `docs/` folder), and in a template update its version files, gets `hygiene` alone on Haiku, with no context, selection, synthesis or verification call, its findings posted as reported, and says "Light review (documentation only)". Any other file, wherever it is, a script or config file in `docs/` or a template's skill script included, gets the standard review, and so do a public API listing, `CMakeLists.txt` and pip requirements files. The same allowlist decides what an oversized diff leaves out first, so a script in `docs/` is no longer ranked with the documentation. A light review whose `hygiene` call failed on any part of the diff says so, and doesn't approve. `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)).

### Changed

Expand All @@ -36,7 +36,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
- The `api-compat` reviewer joins every review that changes a public or protected member, its declaration or its body, of a public type in a packed project: one with `PublicAPI.Shipped.txt` beside its `.csproj`, or `IsPackable` true in it, read from the default branch so a pull request can't change the answer. The `data-model` reviewer joins every review whose diff touches `[JsonPropertyName]`, `[DataMember]`, `JsonSerializerContext`, `IEntityTypeConfiguration` or a `DbContext`, or that changes a `.proto` or `.schema.json` file. Both relied on the selection call before ([#55](https://github.com/NCodeGroup/code-review/issues/55)).
- A missing test is a should-fix only when the untested path is new behavior with a plausible failure a user would see and no test below it covers it. A wrapper over tested pieces, a test that would only pin current behavior, or a second test for a covered path is a nit. Eval concerns can name a `minSeverity`, and five cases from [#30](https://github.com/NCodeGroup/code-review/pull/30) check both sides ([#37](https://github.com/NCodeGroup/code-review/issues/37)).
- The inline threads read the pull request's earlier review comments through the same pager as the summary comment, up to 10,000 instead of 1,000. When a pull request has more, no threads are posted, with a warning, instead of findings already posted on comments past the limit being posted again; the check run still has every finding ([#38](https://github.com/NCodeGroup/code-review/issues/38)).
- The Release workflow fails before it tags when `CHANGELOG.md` has no dated `## [<version>] - YYYY-MM-DD` section with entries for the version, and publishes that section as the release's notes instead of generated ones. The README says why Dependabot proposed no bump of the action to consumers, every release so far being marked as a pre-release, and how to bump them meanwhile ([#33](https://github.com/NCodeGroup/code-review/issues/33)).
- The Release workflow fails before it tags when `CHANGELOG.md` has no dated `## [<version>] - YYYY-MM-DD` section with entries, bulleted or numbered, for the version, outside any code fence, and publishes that section as the release's notes instead of generated ones. The README says why Dependabot proposed no bump of the action to consumers, every release so far being marked as a pre-release, and how to bump them meanwhile ([#33](https://github.com/NCodeGroup/code-review/issues/33)).

### Fixed

Expand Down
4 changes: 2 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ Each model call has its own cost cap, and a whole review has a **cost ceiling**

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.
- **Light review (documentation only).** When every changed file is documentation (a `.md`, `.txt` or `.rst` file, or anything in a `doc/` or `docs/` folder, `CHANGELOG.md` among them, but not a `PublicAPI.*.txt` listing), or the pull request is a template update, changing `.template-version` or `.copier-answers.yml` and otherwise only files the project template owns or documentation, and no changed file is code, a script or a git hook (a `.ps1` or `.sh` file in `docs/` or in the template's `.template/` folder included), `hygiene` alone reviews it, on Haiku, and its findings are posted as it reported them, except one that says a changed file isn't changed, which no review posts: no context, selection, rule-check, synthesis or verification call. It costs a few cents instead of $0.60 to $2.10. A small code change still gets the standard review. Agent instructions such as `CLAUDE.md` and `.github/skills/` are Markdown, so they count as documentation; a repository that wants them reviewed closely lists them in `sensitive:`.
- **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 lists every model a reviewer ran on, so a reviewer whose parts differ shows more than one. Documentation in a sensitive folder makes the review deep too, not light.
- **Light review (documentation only).** When every changed file is documentation, a `.md`, `.txt` or `.rst` file, `CHANGELOG.md` among them, or an image or a PDF in a `doc/` or `docs/` folder, or is `.template-version` or `.copier-answers.yml`, as in a template update that changes only its Markdown, `hygiene` alone reviews it, on Haiku, and its findings are posted as it reported them, except one that says a changed file isn't changed, which no review posts: no context, selection, rule-check, synthesis or verification call. It costs a few cents instead of $0.60 to $2.10. A small code change still gets the standard review, and so does any other file, wherever it is: a script, a config file or a file with no extension in `docs/`, a git hook, a template's script or skill file that isn't Markdown, a `PublicAPI.*.txt` listing, `CMakeLists.txt`, or a pip `requirements*.txt` or `constraints*.txt` file. Agent instructions such as `CLAUDE.md` and `.github/skills/` are Markdown, so they count as documentation; a repository that wants them reviewed closely lists them in `sensitive:`.

`CODE_REVIEW_TIERS` overrides any reviewer's model in both, as it does in a standard review: `correctness=sonnet` keeps a deep review's `correctness` on Sonnet.

Expand Down
19 changes: 15 additions & 4 deletions src/gather.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,16 +95,27 @@
return generatedPatterns.some((pattern) => pattern.test(file));
}

const docPattern = /(\.md|\.txt|\.rst)$|(^|\/)docs?\//i;
const testPattern = /(^|\/)(tests?|__tests__|spec)\/|\.(test|spec|tests)\.[a-z]+$|Tests?\.cs$/i;
export const publicApiPath = /(^|\/)PublicAPI\.[A-Za-z]+\.txt$/;

// Documentation, as an allowlist: a Markdown, reStructuredText or text file anywhere, and an image or a PDF in a doc/ or
// docs/ folder. Any other file, one with no extension included, isn't, wherever it is: a script, a config file or a git
// hook in docs/ or in a template's folders is code. A text file that is a build or dependency list, such as
// CMakeLists.txt or a pip requirements file, isn't documentation, and neither is a public API listing: it is the API.
// The light review and the review order both use this one definition.
const documentationFile = /\.(md|rst|txt)$/i;
const documentationAsset = /(^|\/)docs?\/.+\.(png|jpe?g|gif|svg|webp|pdf)$/i;
const listingFile = /(^|\/)(CMakeLists\.txt|(requirements|constraints)[^/]*\.txt)$/i;

export function isDocumentation(file: string): boolean {

Check notice on line 109 in src/gather.ts

View check run for this annotation

NCode Review / Code review

F1 Nit: The documentation allowlist depends on a growing list of `.txt` names to exclude

Every `.txt` file counts as documentation unless its name matches `publicApiPath` or `listingFile`, so other machine-read `.txt` files still get classified as docs, such as license or dependency lists, test fixtures and data inputs. Consequence: Each new kind of non-prose `.txt` needs another entry here. Until someone adds it, a pull request that changes such a file gets only the light hygiene review and is ordered last when the diff is over budget. How much this matters depends on how common these files are in the target repositories. Fix: Count `.txt` as documentation only by location or name, such as the repository root or doc/ and docs/. Or keep the exclusions in one shared list that the README and the tests also read.
return docPattern.test(file);
if (publicApiPath.test(file) || listingFile.test(file)) return false;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Nit: The documentation allowlist depends on a growing list of .txt names to exclude F1 · architecture

Every .txt file counts as documentation unless its name matches publicApiPath or listingFile, so other machine-read .txt files still get classified as docs, such as license or dependency lists, test fixtures and data inputs.

Consequence: Each new kind of non-prose .txt needs another entry here. Until someone adds it, a pull request that changes such a file gets only the light hygiene review and is ordered last when the diff is over budget. How much this matters depends on how common these files are in the target repositories.

Fix: Count .txt as documentation only by location or name, such as the repository root or doc/ and docs/. Or keep the exclusions in one shared list that the README and the tests also read.

Evidence

src/gather.ts: documentationFile = /\.(md|rst|txt)$/i, then exceptions for listingFile (CMakeLists.txt, requirements/constraints) and publicApiPath.

Confidence 0.30.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Autopilot: declined for now: a nit on code that has already merged, after three review rounds on this chain. The point stands: .txt counts as documentation everywhere except the named exclusions. Narrowing .txt to doc/, docs/ and the repository root (or one shared exclusion list that the README and tests read) is now an item in the follow-up checklist #84.

return documentationFile.test(file) || documentationAsset.test(file);
}

const testPattern = /(^|\/)(tests?|__tests__|spec)\/|\.(test|spec|tests)\.[a-z]+$|Tests?\.cs$/i;

// Product code first, then tests, then everything else: when the diff is over budget, what's left out is the least risky.
export function reviewOrder(file: string): number {
if (docPattern.test(file)) return 3;
if (isDocumentation(file)) return 3;
if (testPattern.test(file)) return 1;
if (/^(\.github|\.template|\.claude|build)\//.test(file)) return 2;
return 0;
Expand Down
2 changes: 1 addition & 1 deletion src/results.ts
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,7 @@ export function checkSummary(results: Results): string {
if (results.depth === 'deep') {
lines.push('', `**${depthLabel.deep}**: ${files(results.sensitiveFiles ?? [])} ${(results.sensitiveFiles ?? []).length === 1 ? 'matches' : 'match'} \`sensitive:\` in \`.github/code-review/config.yml\`, so \`correctness\` and \`security\` reviewed the parts of the diff with them on Opus, unless \`CODE_REVIEW_TIERS\` names them, and verification, when it's on, checked the findings in them on Opus.`);
} else if (results.depth === 'light') {
lines.push('', `**${depthLabel.light}**: every changed file is documentation, a changelog or part of a template update, so \`hygiene\` alone reviewed it, and its findings are posted with no context, selection or synthesis call.`);
lines.push('', `**${depthLabel.light}**: every changed file is documentation, the changelog among it, or a template version file, so \`hygiene\` alone reviewed it, and its findings are posted with no context, selection or synthesis call.`);
}
const upstreamTotal = (results.upstream?.length ?? 0) + (results.upstreamOmitted ?? 0);
if (upstreamTotal) {
Expand Down
Loading
Loading