diff --git a/.github/workflows/eval.yml b/.github/workflows/eval.yml index 888d84b..39ad73e 100644 --- a/.github/workflows/eval.yml +++ b/.github/workflows/eval.yml @@ -16,9 +16,9 @@ on: - smoke - re-review verification: - description: CODE_REVIEW_VERIFICATION + description: CODE_REVIEW_VERIFICATION (on by default, as in production) type: choice - default: 'off' + default: 'on' options: - 'off' - 'on' diff --git a/CHANGELOG.md b/CHANGELOG.md index 07ce6cc..091e45c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), ## [Unreleased] +## [1.0.0] - 2026-11-01 + ### Added - Settings checks: `tiers:` can't put `security` below Sonnet (a lower tier is raised to Sonnet, with a warning), and a `skip:` glob that matches nearly every path, such as `**`, and a `CODE_REVIEW_SHADOW` name that matches no reviewer are warnings in the check run ([#94](https://github.com/NCodeGroup/code-review/issues/94)). @@ -13,6 +15,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), - Timing instrumentation: each model call's record in the trace has when it started (`startedAt`) and when its first response came (`firstResponseMs`), the trace has the engine's stage times (`timings`), `node tools/timings.ts` summarizes downloaded trace artifacts, `npm run bench:review` times a review on a fake agent with production's call durations, and `npm run bench:tests` times each test file ([#128](https://github.com/NCodeGroup/code-review/issues/128)). - The README's Interface section writes down the caller–engine contract v1.0.0 will freeze: the action's inputs, the triggers and types a caller needs, each job's permissions, environment and secrets, and the `base` checkout ([#57](https://github.com/NCodeGroup/code-review/issues/57)). - The Eval workflow runs a named eval subset (`smoke` or `re-review`) by hand from `main`, with verification on or off, a `CODE_REVIEW_TIERS` override, up to two runs and a cap of $5, $10 or $25 a run, in the `code-review` environment, and uploads the outputs after a secret scan, so evals need no local Bedrock key. `CODE_REVIEW_TIERS` takes `synthesis=` and `CODE_REVIEW_SYNTHESIS_EFFORT` sets the synthesis call's effort, to measure synthesis on a cheaper setup ([#135](https://github.com/NCodeGroup/code-review/issues/135)). +- The smoke eval's first baseline, `evals/baseline-smoke.json`, from the run on `main` after the decisions rule was narrowed: caught 2 of 3, the must-not case quiet ([#52](https://github.com/NCodeGroup/code-review/issues/52)). ### Changed @@ -46,6 +49,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), - The tests run faster on Windows: `test/rereview.test.ts` builds each git repository its tests only read once instead of once per test, and the fetch time-limit test's server drops its connection after the limit, so the killed git's `git-remote-http` no longer holds the folder through the cleanup's retries; that test now checks the folder is removed on Windows too ([#136](https://github.com/NCodeGroup/code-review/issues/136)). - Callers record outcomes in a job of their own, `outcomes`, which runs only when a pull request closes, outside the `code-review` environment and without `id-token: write`, the Bedrock key or a checkout, since it makes no model call; the review job no longer runs on `closed`. This repository's workflow and the README's caller have it, and the README's examples pin v0.4.0 ([#47](https://github.com/NCodeGroup/code-review/issues/47)). - Reviewers no longer question a decision the pull request adds for a scope or limit it states, such as leaving existing data as it is: they question it only where it contradicts the code or another decision. The smoke eval's must-not case flagged such a question ([#52](https://github.com/NCodeGroup/code-review/issues/52)). +- Verification is on by default; `CODE_REVIEW_VERIFICATION=off` turns it off. On the smoke evals it caught as much as without it, with no must-not flag, at 5 blockers and should-fixes against 7 and 8, for about 3% more. The Eval workflow runs it by default too ([#41](https://github.com/NCodeGroup/code-review/issues/41)). +- The README's Evals section records the 2026-11-01 results: verification on and off, synthesis on Sonnet and at medium effort, which stays on Opus at the default effort since Sonnet posted more serious findings, and the re-review cases' cost, with no overlapping findings for `mergeOverlapping` to merge ([#123](https://github.com/NCodeGroup/code-review/issues/123), [#135](https://github.com/NCodeGroup/code-review/issues/135)). ### Fixed @@ -59,6 +64,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), - A re-review shows the reviewers the replies in the earlier findings' threads from an organization member whose membership is private, and a finding's outcome counts their replies: when a reply's author association doesn't name a writer, both ask the repository's permissions, as the description's check does, looking up at most 20 repliers per review ([#124](https://github.com/NCodeGroup/code-review/issues/124)). - A finding's outcome is read from the marker that ends its thread's first comment, as everything else reads it, so a one-click suggestion whose code holds a marker-like line can't change which finding the outcome records ([#95](https://github.com/NCodeGroup/code-review/issues/95)). - The `api-compat` floor reads a C# verbatim string whose text starts with an escaped quote, such as `@"""Hi"" there"`, as a verbatim string, instead of as a raw string that ran to the end of the file and made every change in it count as public. A measurement across NCodeGroup's C# found no file where conditional compilation unbalances the braces, so the scanner stays ([#122](https://github.com/NCodeGroup/code-review/issues/122)). +- A finding's outcome follows a writer's latest `Autopilot: fixed in ` reply when the pull request gained that commit after the review, even when the fix is on other lines, and a writer's `Autopilot: not changed` reply, even when the line changed for another reason; the monthly report's hand count found both counted the other way ([#97](https://github.com/NCodeGroup/code-review/issues/97)). ### Security @@ -147,7 +153,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), - The first release: a GitHub composite action that reviews a pull request with reviewers on the Claude Agent SDK and posts the findings as the "Code review" check run, a summary comment and inline threads. -[unreleased]: https://github.com/NCodeGroup/code-review/compare/v0.4.0...HEAD +[unreleased]: https://github.com/NCodeGroup/code-review/compare/v1.0.0...HEAD +[1.0.0]: https://github.com/NCodeGroup/code-review/compare/v0.4.0...v1.0.0 [0.4.0]: https://github.com/NCodeGroup/code-review/compare/v0.3.1...v0.4.0 [0.3.1]: https://github.com/NCodeGroup/code-review/compare/v0.3.0...v0.3.1 [0.3.0]: https://github.com/NCodeGroup/code-review/compare/v0.2.1...v0.3.0 diff --git a/README.md b/README.md index 9e3f7be..88dbb57 100644 --- a/README.md +++ b/README.md @@ -56,7 +56,7 @@ A rule in the learned guidance (`guidance/`) that came only from instruction fil Each model call has its own cost cap, and a whole review has a **cost ceiling** that guards against a runaway review only: $8 plus $4 for each part of the diff, so $12 for most pull requests and $32 for one reviewed in six parts, far above the $0.60 to $2.10 a normal review costs and the $6 to $7 of a 2 MB pull request. Once the calls so far reach it, the reviewer and rule-check calls that haven't started are skipped, each such reviewer shows `skipped: run budget` in the check run, synthesis still runs on what was found, verification calls that haven't started are skipped too, leaving their findings unverified, and the summary warns that the review is incomplete and doesn't approve. Shadow calls count toward it too, but the shadow-prompt twins start after every live reviewer call, and a skipped shadow reviewer doesn't make the review incomplete. To change it, set `CODE_REVIEW_MAX_RUN_USD` to a positive number of dollars in the calling job's `env`; any other value warns and keeps the default. -**Verification** is off by default. `CODE_REVIEW_VERIFICATION=on` adds a stage after synthesis: each blocker and should-fix it merged goes to a Sonnet call, or an Opus call for a deep review's sensitive files, with Read, Grep and Glob in the checkout, behind the same path guard as the reviewers, with its diff hunk and its reviewers' charters, a file's findings per call at up to $0.50 each ($1 on Opus). The call proves or refutes the finding with a concrete code path. A confirmed finding stays, with the path added to its evidence; a refuted one is dropped and listed in the check run under "Refuted by verification", with the reason, but only when the refutation cites a line of the checkout outside the lines the pull request adds, and never a blocker or a `security` finding: those, and a refutation that rests on the change's own lines, stay as a question one severity lower, with the refutation in the evidence; one it can neither prove nor refute stays as a question, one severity lower. Nits aren't verified. The summary, which synthesis wrote before verification, gets a sentence that names the findings verification refuted and dropped, and counts separately the ones it couldn't settle and the refuted ones it kept, both now questions one severity lower. Verification never makes a review an approval: a request for changes with no blocker or should-fix left becomes a comment, since a verifier misled by the change's own code can refute a real finding. A [re-review](#re-reviews)'s findings go through it too. Verification calls count toward the cost ceiling: once the run's spending reaches it, the calls that haven't started are skipped, but the ones already running finish, so a review can end past it, as with the reviewer calls. The calls show as `verification` in the check run's cost table and the trace records each one. It stays off until an eval shows it catches as much at fewer blockers and should-fixes, for what it costs ([#41](https://github.com/NCodeGroup/code-review/issues/41)). +**Verification** is on by default; `CODE_REVIEW_VERIFICATION=off` turns it off. It is a stage after synthesis: each blocker and should-fix it merged goes to a Sonnet call, or an Opus call for a deep review's sensitive files, with Read, Grep and Glob in the checkout, behind the same path guard as the reviewers, with its diff hunk and its reviewers' charters, a file's findings per call at up to $0.50 each ($1 on Opus). The call proves or refutes the finding with a concrete code path. A confirmed finding stays, with the path added to its evidence; a refuted one is dropped and listed in the check run under "Refuted by verification", with the reason, but only when the refutation cites a line of the checkout outside the lines the pull request adds, and never a blocker or a `security` finding: those, and a refutation that rests on the change's own lines, stay as a question one severity lower, with the refutation in the evidence; one it can neither prove nor refute stays as a question, one severity lower. Nits aren't verified. The summary, which synthesis wrote before verification, gets a sentence that names the findings verification refuted and dropped, and counts separately the ones it couldn't settle and the refuted ones it kept, both now questions one severity lower. Verification never makes a review an approval: a request for changes with no blocker or should-fix left becomes a comment, since a verifier misled by the change's own code can refute a real finding. A [re-review](#re-reviews)'s findings go through it too. Verification calls count toward the cost ceiling: once the run's spending reaches it, the calls that haven't started are skipped, but the ones already running finish, so a review can end past it, as with the reviewer calls. The calls show as `verification` in the check run's cost table and the trace records each one. The smoke evals of 2026-11-01 caught as much with it on as off, with 0 must-not flags either way, at 5 blockers and should-fixes against 7 and 8, for about 3% more ([#41](https://github.com/NCodeGroup/code-review/issues/41)). 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: @@ -177,10 +177,10 @@ outcomes. It shares the review's concurrency group, so it waits for a review in ## Interface -This is the contract between a caller's workflow and the engine, which v1.0.0 will freeze. From v1.0.0 on, removing or +This is the contract between a caller's workflow and the engine, which v1.0.0 freezes. From v1.0.0 on, removing or renaming an input, changing what an input's default means, or needing another trigger type, permission, secret, -environment or checkout is a breaking change and a new major version; a new optional input or output isn't. Before -1.0.0 a minor release may still change it, and its changelog section says how. +environment or checkout is a breaking change and a new major version; a new optional input or output isn't. Releases +before 1.0.0 changed it in minor releases; their changelog sections say how. | Input | Required | Default | What it is | |---|---|---|---| @@ -230,6 +230,14 @@ afterwards, so a mistake is fixed by releasing the next version. ### Dependabot and pre-releases +From v1.0.0 on, releases aren't marked as pre-releases, so Dependabot's `github-actions` updater proposes each one in a +consumer's action updates. project-template gives the bump a Dependabot group of its own and a workflow, +`code-review-auto-merge.yml` ([NCodeGroup/project-template#184](https://github.com/NCodeGroup/project-template/pull/184)), +that auto-merges the bump for a minor or patch release once the required checks pass and no review thread is +unresolved. That is the one exception its ADR-0006 records to the rule that a person merges every pull request. A +major release changes this interface, so a person merges it, and a repository without that workflow merges every bump +by hand. + No consumer got a Dependabot pull request for v0.2.0 to v0.3.1, because each is marked as a pre-release. Since [dependabot-core#13731](https://github.com/dependabot/dependabot-core/pull/13731) (December 2025), the `github-actions` updater drops a tag whose GitHub release is marked as a pre-release, and a tag whose name is a @@ -243,7 +251,8 @@ tag on the pinned commit and looks for a newer allowed tag then rewrites the SHA and its `# vX.Y.Z` comment. This repository is public, so access isn't the cause, and grouping only puts the bump in the same pull request as the consumer's other action updates. -Until releases stop being marked as pre-releases, bump each consumer by hand: in a pull request, change its +A consumer that pins a v0 release moves to v1.0.0 by hand, since Dependabot doesn't propose a release when the pinned +one is hidden as a pre-release: in a pull request, change its `NCodeGroup/code-review@ # v` to the new release's commit, `gh api repos/NCodeGroup/code-review/commits/v --jq .sha`, and tag. To let Dependabot propose a release instead, clear its flag, `gh release edit v -R NCodeGroup/code-review --prerelease=false`, which an immutable release allows @@ -313,19 +322,20 @@ Each review, all from `ncode-review[bot]`: When a pull request closes, merged or not, the bot records what became of each finding it posted as a thread, by rules alone: no model call, so the run needs no model key and costs nothing. It reads each thread, its replies and the reactions on the finding, and compares the commit the finding was posted on with the closing head. The first rule that -holds decides: +holds decides, after the autopilot's replies below: | Outcome | When | |---|---| -| fixed | The finding's line was edited or removed, or code was added right next to it, afterwards; or its thread is outdated and resolved. When GitHub's comparison can't show the line (a diff too large for a patch, or more than 300 changed files), an outdated thread counts as a changed line, and the run warns. | -| dismissed | A 👎 from a writer, a thread resolved without a change, or a writer's reply that declines it, such as "won't fix", "false positive" or "by design". | +| fixed | The finding's line was edited or removed, or code was added right next to it, afterwards; or its thread is outdated and resolved. When GitHub's comparison can't show the line (a diff too large for a patch, or more than 300 changed files), an outdated thread counts as a changed line, and the run warns. Or a writer's reply `Autopilot: fixed in ` naming a commit the pull request gained after the finding's review, even when the fix is on other lines, such as a test. | +| dismissed | A 👎 from a writer, a thread resolved without a change, or a writer's reply that declines it, such as "won't fix", "false positive" or "by design". Or a writer's reply `Autopilot: not changed`, even when the line changed for another reason. | | acknowledged | A 👍 or a reply from a writer, without a change. | | ignored | None of these. | Writers are people with write access: for a reply, its author's association (owner, member or collaborator) or, when that doesn't say so, their permission on the repository, with at most 20 repliers looked up; for a reaction, the -reactor's permission on the repository. A reply starting `Autopilot: fixed in` counts only when the -finding's line changed, which already makes it fixed. The newest summary comment gets a table of finding, reviewer, +reactor's permission on the repository. Of a writer's replies, the latest `Autopilot: fixed in ` or +`Autopilot: not changed` decides before the other rules; a `fixed in` reply that names no commit after the review +counts for nothing. The newest summary comment gets a table of finding, reviewer, severity and outcome, with the pull request's state, and a hidden `` marker holding, for each finding, its reviewers, severity and outcome, the commit it was reviewed at, and that review's cost, provider and model, for the monthly report. The table shows 100 findings at most, and the marker as many as fit in a comment (about @@ -563,7 +573,7 @@ default-branch jobs get an Anthropic token. `-WhatIf` shows what would change. The cases come from review comments the learned guidance has read too, so the evals use **held-out guidance** by default: `guidance-holdout/`, built by `npm run train -- --generalize-only --holdout-only`, which leaves out the review comments on the cases' pull requests and the decision records they cite. A catch then shows what the bot learned in general, not the answer to that case. `--guidance full` uses the production `guidance/` instead, which scores about twice as high on these cases for that reason. `guidance-holdout/` is derived, so it is ignored by Git; build it once. -`CODE_REVIEW_RULE_CHECKS=off` turns off the rule checks ([How a review runs](#how-a-review-runs)) to measure what they add. `CODE_REVIEW_VERIFICATION=on` turns on verification; the report counts the blockers and should-fixes posted, and the findings verification refuted, to compare with it off. `CODE_REVIEW_TIERS=synthesis=sonnet` runs synthesis on Sonnet instead of Opus, and `CODE_REVIEW_SYNTHESIS_EFFORT` (`low`, `medium`, `high`, `xhigh` or `max`) sets its effort, for measuring synthesis's cost against its quality. Run each setup at least twice: a run of the first 16 cases varies by about 2 of their 55 expected findings. +`CODE_REVIEW_RULE_CHECKS=off` turns off the rule checks ([How a review runs](#how-a-review-runs)) to measure what they add. `CODE_REVIEW_VERIFICATION=off` turns verification off; the report counts the blockers and should-fixes posted, and the findings verification refuted, to compare with it off. `CODE_REVIEW_TIERS=synthesis=sonnet` runs synthesis on Sonnet instead of Opus, and `CODE_REVIEW_SYNTHESIS_EFFORT` (`low`, `medium`, `high`, `xhigh` or `max`) sets its effort, for measuring synthesis's cost against its quality. Run each setup at least twice: a run of the first 16 cases varies by about 2 of their 55 expected findings. Shadow reviewers ([Shadow reviewers and prompts](#shadow-reviewers-and-prompts)) are scored on their own: `--shadow performance` (or `CODE_REVIEW_SHADOW`) runs `performance` in shadow, and the report gives its catches, must-not violations and findings apart from the live reviewers', whose scores never include them. Precision counts only posted findings, so never theirs; the review's cost includes their calls. @@ -584,7 +594,9 @@ gh run download -n smoke-eval -D evals/out/smoke Copy-Item evals/out/smoke/proposed-baseline-smoke.json evals/baseline-smoke.json ``` -**Eval workflow.** The [Eval](.github/workflows/eval.yml) workflow runs a named subset by hand from `main` (`gh workflow run eval.yml -f subset=re-review -f verification=on`), in the `code-review` environment with the organization's Bedrock key, so no local key is needed. Its inputs are `subset` (`smoke` or `re-review`), `verification`, `tiers` (`CODE_REVIEW_TIERS`), `synthesis-effort`, `runs` (1 or 2) and `max-cost` per run ($5, $10 or $25). It always uses the production guidance and `--precision off`. Each run's summary goes to the job summary, and `evals/out` is uploaded as `eval-` after the same secret scan as the smoke eval. Compare runs with `npm run eval:compare` after `gh run download -n eval- -D evals/out/`. +**Eval workflow.** The [Eval](.github/workflows/eval.yml) workflow runs a named subset by hand from `main` (`gh workflow run eval.yml -f subset=re-review -f verification=on`), in the `code-review` environment with the organization's Bedrock key, so no local key is needed. Its inputs are `subset` (`smoke` or `re-review`), `verification` (`on` by default), `tiers` (`CODE_REVIEW_TIERS`), `synthesis-effort`, `runs` (1 or 2) and `max-cost` per run ($5, $10 or $25). It always uses the production guidance and `--precision off`. Each run's summary goes to the job summary, and `evals/out` is uploaded as `eval-` after the same secret scan as the smoke eval. Compare runs with `npm run eval:compare` after `gh run download -n eval- -D evals/out/`. + +**Results, 2026-11-01.** On the smoke subset with the production guidance and `--precision off`, two runs each, verification off ([41278266855](https://github.com/NCodeGroup/code-review/actions/runs/41278266855), [41280733222](https://github.com/NCodeGroup/code-review/actions/runs/41280733222)) caught 2 of 3 and 2 of 3 expected findings, with 0 must-not flags, 7 and 8 blockers and should-fixes, and $4.14 and $4.04; with it on ([41278712152](https://github.com/NCodeGroup/code-review/actions/runs/41278712152), [41280198652](https://github.com/NCodeGroup/code-review/actions/runs/41280198652)) it caught 2 of 3 and 2 of 3, with 0 must-not flags, 5 and 5 blockers and should-fixes, and $4.21 and $4.16, of which the verification calls were $0.14, about 3%, so verification is on by default ([#41](https://github.com/NCodeGroup/code-review/issues/41)); this departs from that issue's held-out full-set protocol for cost, since the smoke subset is the gate the repository runs on every prompt change. Synthesis on Sonnet ([41279253481](https://github.com/NCodeGroup/code-review/actions/runs/41279253481)) caught the same 2 of 3 with 0 must-not flags at half synthesis's cost ($0.42 against $0.89 for the 4 cases) and 45% less time (68 s against 122 s), but posted 12 blockers and should-fixes against Opus's 7 and 8, which the autopilot would act on; `CODE_REVIEW_SYNTHESIS_EFFORT=medium` ([41279660175](https://github.com/NCodeGroup/code-review/actions/runs/41279660175)) saved nothing ($0.95, 134 s, 8 blockers and should-fixes), so synthesis stays on Opus at the SDK's default effort ([#135](https://github.com/NCodeGroup/code-review/issues/135)). The re-review subset (4 cases) cost $0.45 with verification off ([41278561619](https://github.com/NCodeGroup/code-review/actions/runs/41278561619)): $0.02 for `re-review-declined-trigger`, $0.07 for `re-review-merge-of-main`, $0.10 for `re-review-security-floor` and $0.26 for `re-review-sensitive-path`, the deep one on Opus; and $0.12 with it on ([41278639968](https://github.com/NCodeGroup/code-review/actions/runs/41278639968)), with the prompt cache warm: $0.006, $0.005, $0.022 and $0.090. Neither run posted a finding, so nothing was verified; both had 0 must-not flags and both missed `re-review-sensitive-path`'s expected concern ([#123](https://github.com/NCodeGroup/code-review/issues/123)). The log counted 0 merged and 0 `near` findings, so `mergeOverlapping` doesn't merge `near` ones and stays as it is. The runs cost $24.83 in all. **Growing the set from verdicts.** `npm run eval:confirm` lists the `real` and `not-real` verdicts nobody has confirmed yet (`--verdict real`, `--verdict unsure` or `--sample 5` of each narrow it), each with the command that confirms it. `npm run eval:confirm -- --case --fingerprint ` adds a `real` finding to its case in `cases.json` as an expected concern, and a `not-real` one as a must-not-flag concern, and marks the verdict confirmed. `--as expect` or `--as must-not` overrides the judge, and is needed for an `unsure` one. Review the new concern's wording in `cases.json` before committing it. In PowerShell, quote the separator: `npm run eval:confirm '--' --case --fingerprint `. diff --git a/evals/baseline-smoke.json b/evals/baseline-smoke.json new file mode 100644 index 0000000..8a0eea4 --- /dev/null +++ b/evals/baseline-smoke.json @@ -0,0 +1,541 @@ +{ + "summary": { + "label": "smoke-f9f8d1d123432f1fc3be12e2082b3e05a39d945e", + "tiers": "", + "verification": "off", + "expected": 3, + "caught": 2, + "caughtRaw": 2, + "violations": 0, + "findings": 28, + "blockersAndShouldFixes": 7, + "refuted": 0, + "findingsPerCase": { + "mean": 7, + "max": 10 + }, + "reviewCostUsd": 4.07788147, + "reviewCostPerCase": { + "mean": 1.0194703675, + "max": 1.6195724500000002 + }, + "costUsd": 4.140123470000001, + "shadow": { + "reviewers": [], + "caught": 0, + "violations": 0, + "findings": 0 + }, + "maxCostUsd": 8, + "casesRun": 4, + "casesSelected": 4 + }, + "cases": [ + { + "id": "heap-allocation-key-export", + "verdicts": [ + { + "concern": "The public key is exported through heap arrays (new byte[] / ArrayBufferWriter), which the allocation ADR (ADR-0116) and the production conventions reserve for APIs that demand an array; use stackalloc or a pooled buffer writer. The same allocation in the sibling key-type descriptors (Okp, Akp and the base SecretKeyTypeDescriptor), which fix #317 also changed, counts.", + "caught": true, + "caughtRaw": true, + "by": [ + "src/NCode.Identity.Secrets/KeyTypes/SecretKeyTypeDescriptor.cs:107 (conventions, rule-check)", + "src/NCode.Identity.Jose.EdDsa/KeyTypes/OkpSecretKeyTypeDescriptor.cs:98 (rule-check)", + "src/NCode.Identity.Secrets/KeyTypes/AkpSecretKeyTypeDescriptor.cs:101 (rule-check)" + ] + } + ], + "forbidden": [], + "findings": [ + { + "severity": "nit", + "file": "src/NCode.Identity.OpenId.Management/Endpoints/Secrets/DefaultSecretPublicKeyConverter.cs", + "line": 46, + "title": "Public-key export guard and sequence duplicated in the registration handler", + "problem": "`Convert` and `DefaultClientRegistrationEndpointHandler.ExportPublicKeys` each repeat the same steps: `TryGet(secret.SecretType)`, `SupportsPublicKeyExport`, `HasPublicKey()`, `DeserializePublicSecret`, then `ExportPublicKey`.", + "consequence": "A later change to what counts as exportable public material has to be made in two places. If only one is updated, the public-key endpoint and the registration `jwks` will disagree, which is the drift ADR-0105 consolidated away.", + "fix": "Move the shared sequence into one helper, for example an extension next to `HasPublicKey` in the Secrets.Persistence abstractions, and call it from both places. `ISecretPublicKeyConverter` is internal to Management, so the helper needs to live where both projects can reach it.", + "evidence": "Both methods contain `!SecretKeyTypeCatalog.TryGet(secret.SecretType, out var descriptor) || !descriptor.SupportsPublicKeyExport || !secret.HasPublicKey()` followed by `SecretSerializer.DeserializePublicSecret(secret)` and `descriptor.ExportPublicKey(secretKey)`.", + "confidence": 0.35, + "reviewers": [ + "hygiene" + ], + "id": "F1" + }, + { + "severity": "nit", + "file": "src/NCode.Identity.Secrets/KeyTypes/SecretKeyTypeDescriptor.cs", + "line": 107, + "title": "SPKI exported through the array-returning `ExportSubjectPublicKeyInfo()`", + "problem": "`algorithm.ExportSubjectPublicKeyInfo()` here and `key.ExportSubjectPublicKeyInfo()` in `AkpSecretKeyTypeDescriptor` (line 101) each allocate a throwaway `byte[]` that is only base64url-encoded.", + "consequence": "Each key generation adds an avoidable heap allocation, against the buffer convention of ADR-0115/0116. The data is public, so nothing leaks; this is about allocation and convention only.", + "fix": "Use `TryExportSubjectPublicKeyInfo` into a rented non-sensitive buffer, or write to `BufferFactory.CreatePooledBufferWriter(isSensitive: false)`, then base64url-encode from the written span. Apply the same change in both descriptors.", + "evidence": "Rule: rent transient buffers from BufferFactory instead of `new byte[]`/`.ToArray()`, and encode public SPKI with a non-sensitive pooled writer (csharp-production.instructions.md, ADR-0115/0116).", + "confidence": 0.4, + "reviewers": [ + "conventions", + "rule-check" + ], + "id": "F2" + }, + { + "severity": "nit", + "file": "src/NCode.Identity.Jose.EdDsa/KeyTypes/OkpSecretKeyTypeDescriptor.cs", + "line": 98, + "title": "OKP public key staged in `new byte[]` and an undisposed `ArrayBufferWriter`", + "problem": "The public key goes into `new byte[curve.PublicKeySize]`, and the SPKI is written to `BufferFactory.CreateArrayBufferWriter()`. Both are transient, and the writer is never disposed.", + "consequence": "Extra heap allocations on each generation. If `CreateArrayBufferWriter` returns a pooled or disposable writer, its buffer is never returned; reviewers did not confirm which it is.", + "fix": "Use `stackalloc` for the fixed-size public key, and `using var` a `BufferFactory.CreatePooledBufferWriter(isSensitive: false)` for the SPKI.", + "evidence": "`var publicKey = new byte[curve.PublicKeySize];` and `var subjectPublicKeyInfo = BufferFactory.CreateArrayBufferWriter();` with no `using`.", + "confidence": 0.35, + "reviewers": [ + "rule-check" + ], + "id": "F3" + }, + { + "severity": "nit", + "file": "tests/NCode.Identity.Jose.EdDsa.Tests/OkpIntegrationTests.cs", + "line": 207, + "title": "Is the hand-written OKP SPKI encoder pinned to externally produced bytes?", + "problem": "The test compares the SPKI that `OkpKeyEncoder.WriteSubjectPublicKeyInfo` stores with the PEM exported from the deserialized key, and that export likely uses the same encoder.", + "consequence": "If the SPKI layout is misread, both sides would carry the same mistake: a wrong Ed25519/Ed448 SPKI could be stored and still pass.", + "fix": "If no OpenSSL-produced fixture already pins this encoding elsewhere, add one for Ed25519 and Ed448 (record the command in a comment) and assert the generated SPKI parses to the same public key.", + "evidence": "Rule: pin a protocol encoder or parser to bytes another implementation wrote.", + "confidence": 0.3, + "reviewers": [ + "rule-check" + ], + "id": "F4" + }, + { + "severity": "nit", + "file": "src/NCode.Identity.OpenId.Management/Endpoints/Secrets/ISecretPublicKeyConverter.cs", + "line": 30, + "title": "Doc comment describes the change rather than the current state", + "problem": "The summary says \"a private key generated before its public key was stored\", which ties the comment to this change.", + "consequence": "A reader who never saw the earlier version gets a comment anchored to past history, against the timeless-present documentation rule.", + "fix": "Reword to \"a private key stored without a public key\".", + "evidence": "`/// material: a symmetric secret, or a private key generated before its public key was stored.`", + "confidence": 0.35, + "reviewers": [ + "rule-check" + ], + "id": "F5" + }, + { + "severity": "nit", + "file": "src/NCode.Identity.Secrets/KeyTypes/SecretKeyTypeDescriptor.cs", + "line": 105, + "title": "Call nested inside another call's argument", + "problem": "`Base64Url.EncodeToString(algorithm.ExportSubjectPublicKeyInfo())` nests the export call inside the encode call, and `AkpSecretKeyTypeDescriptor` does the same with `key.ExportSubjectPublicKeyInfo()`.", + "consequence": "This breaks the house rule against nested calls in arguments. It affects readability only.", + "fix": "Assign the SPKI to a local such as `spki` and encode the local. If you adopt the pooled-buffer fix above, this goes away with it.", + "evidence": "Rule: do not nest a call inside another call's arguments.", + "confidence": 0.35, + "reviewers": [ + "rule-check" + ], + "id": "F6" + }, + { + "severity": "nit", + "file": "src/NCode.Identity.OpenId.Persistence.EntityFramework.Core/Entities/SecretEntity.cs", + "line": 1, + "title": "Unrelated removal of the UTF-8 byte-order mark", + "problem": "Line 1 loses its leading BOM, which is unrelated to the new `PublicKey` property.", + "consequence": "The diff and blame show a spurious header change, and tools that detect encoding from the BOM may treat the file differently.", + "fix": "Restore the BOM on line 1.", + "evidence": "diff: `-#region Copyright Preamble` / `+#region Copyright Preamble`.", + "confidence": 0.5, + "suggestion": { + "startLine": 1, + "endLine": 1, + "replacement": "#region Copyright Preamble" + }, + "reviewers": [ + "hygiene" + ], + "id": "F7" + }, + { + "severity": "nit", + "file": "src/NCode.Identity.Secrets/KeyTypes/AkpSecretKeyTypeDescriptor.cs", + "line": 101, + "title": "AKP public-key export allocates a transient byte[]", + "problem": "`key.ExportSubjectPublicKeyInfo()` returns a throwaway `byte[]` that is passed straight to `Base64Url.EncodeToString`.", + "consequence": "This is an unneeded transient allocation, and it breaks the pooled-buffer convention.", + "fix": "Write the SPKI into a non-sensitive pooled writer or rented buffer and encode from its span.", + "evidence": "Rule: rent transient buffers from BufferFactory instead of `new byte[]`/`.ToArray()`.", + "confidence": 0.4, + "reviewers": [ + "rule-check" + ], + "id": "F8" + }, + { + "severity": "nit", + "file": "src/NCode.Identity.OpenId.Authentication/Endpoints/Registration/DefaultClientRegistrationEndpointHandler.cs", + "line": 1740, + "title": "Public-key export sequence repeated in the registration handler", + "problem": "ExportPublicKeys repeats the sequence in DefaultSecretPublicKeyConverter.Convert: catalog TryGet, SupportsPublicKeyExport, HasPublicKey, DeserializePublicSecret, ExportPublicKey. The two copies can drift. ISecretPublicKeyConverter is internal to the Management package, so the Authentication project may not be able to reference it; I did not check.", + "consequence": "If one copy changes, for example in how a missing public key is handled, the other will not follow.", + "fix": "Move the shared sequence into one helper that both callers use, if the project layering allows it.", + "evidence": "Rule: reuse shared helpers; move logic repeated within a change into one implementation", + "confidence": 0.3, + "reviewers": [ + "rule-check" + ], + "id": "F9" + } + ], + "reviewCostUsd": 1.6195724500000002, + "costUsd": 1.65613645 + }, + { + "id": "secret-buffer-length", + "verdicts": [ + { + "concern": "The rented pool memory can be longer than requested, and the secret is replaced and unescaped over the whole block, so the decoded secret carries the unused remainder; a client_secret_basic secret containing % or + then never matches (401). Slice the memory to the encoded length.", + "caught": true, + "caughtRaw": true, + "by": [ + "NCode.Identity.OpenId.Core/Clients/Handlers/BasicClientAuthenticationHandler.cs:79 (correctness, security, completeness, rule-check)" + ] + } + ], + "forbidden": [], + "findings": [ + { + "severity": "blocker", + "file": "NCode.Identity.OpenId.Core/Clients/Handlers/BasicClientAuthenticationHandler.cs", + "line": 134, + "title": "Basic header base64 decode is mis-sized, unsliced and unchecked in Release", + "problem": "The buffer is sized with `Base64Url.GetByteCountForDecode` (unpadded base64url), but the data is decoded with standard `Convert.TryFromBase64Chars`. `base64BytesWritten` is never used to slice `base64Bytes`, and the decode result is checked only by `Debug.Assert`. For example, \"YWI=\" sizes to 3 bytes but decodes to 2.", + "consequence": "Padded credentials, which RFC 7617 Basic values normally are, leave trailing zero bytes that become `\\0` characters in the client id or secret, so valid clients fail authentication. In Release, malformed base64 is no longer rejected with ErrorInvalidHeader and garbage is processed as credentials. The removed 'check for exceptions' TODO is left unaddressed. Strict UTF-8 decoding can also throw DecoderFallbackException, which surfaces as a 500.", + "fix": "Size the buffer with a standard-base64 upper bound, e.g. `(len / 4) * 3` or `Base64.GetMaxDecodedFromUtf8Length`. Check the `TryFromBase64Chars` result at runtime and return `ErrorInvalidHeader` on failure. Slice to `base64Bytes[..base64BytesWritten]` before `GetCharCount`/`TryGetChars`. Treat a failed UTF-8 decode (result false or DecoderFallbackException) as an invalid header too.", + "evidence": "Line 131: `Base64Url.GetByteCountForDecode(encodedCredentials.Length)`; line 134: `Convert.TryFromBase64Chars(encodedCredentials.Span, base64Bytes, out var base64BytesWritten)`; line 135: `Debug.Assert(base64Result && base64BytesWritten == base64ByteCount)`; lines 141/144 then use the whole `base64Bytes`.", + "confidence": 0.8, + "reviewers": [ + "hygiene", + "correctness", + "security", + "conventions", + "architecture", + "completeness", + "rule-check" + ], + "id": "F1" + }, + { + "severity": "should-fix", + "file": "NCode.Identity.OpenId.Core/Clients/Handlers/BasicClientAuthenticationHandler.cs", + "line": 79, + "title": "UriDecode unescapes the whole rented buffer, not just encoded.Length", + "problem": "`destination = buffer.Span` covers the whole rented buffer, but `Replace` writes only the first `encoded.Length` chars, and `Uri.TryUnescapeDataString(destination, destination, ...)` then also processes the unwritten tail. The unescape result is checked only by `Debug.Assert`.", + "consequence": "If the pool over-allocates (MemoryPool.Shared does; check SecureMemoryPool), secrets containing '%' or '+' get trailing stale or zero characters and fail authentication. In Release, a failed unescape leaves `unescapedLength` meaningless, so the secret is silently mangled instead of rejected.", + "fix": "Use `var destination = buffer.Span[..encoded.Length];` for both `Replace` and `TryUnescapeDataString`, and slice the result to `unescapedLength`. Check the bool result at runtime and map failure to an invalid header or invalid_client error.", + "evidence": "`var lease = pool.Rent(encoded.Length); var buffer = lease.Memory; var destination = buffer.Span; source.Replace(destination, '+', ' '); Uri.TryUnescapeDataString(destination, destination, out var unescapedLength); Debug.Assert(decodeResult);`", + "confidence": 0.6, + "reviewers": [ + "correctness", + "security", + "completeness", + "rule-check" + ], + "id": "F2" + }, + { + "severity": "should-fix", + "file": "NCode.Identity.OpenId.Core/Clients/Handlers/CommonClientAuthenticationHandler.cs", + "line": 113, + "title": "Secret byte buffer passed unsliced; TryGetBytes checked only by Debug.Assert", + "problem": "The whole `clientSecretBytes` from `CryptoPool.Rent(byteCount, ...)` is passed to `AuthenticateClientAsync` instead of `[..bytesWritten]`, and the encode result is checked only by `Debug.Assert`.", + "consequence": "If CryptoPool returns memory longer than `byteCount`, the extra bytes enter the secret comparison and valid clients fail. In Release, an encoding failure goes unnoticed.", + "fix": "Pass `clientSecretBytes[..bytesWritten]` and replace the `Debug.Assert` with a runtime check. Do this unless CryptoPool is confirmed to return exactly sized memory.", + "evidence": "`using var _ = CryptoPool.Rent(byteCount, isSensitive: true, out Memory clientSecretBytes); ... Debug.Assert(decodeResult && bytesWritten == byteCount); return await AuthenticateClientAsync(publicClient, clientSecretBytes, cancellationToken);`", + "confidence": 0.5, + "reviewers": [ + "rule-check" + ], + "id": "F3" + }, + { + "severity": "should-fix", + "file": "NCode.Identity.OpenId.Core/Clients/Handlers/BasicClientAuthenticationHandler.cs", + "line": 148, + "title": "Empty-credentials and empty-clientId checks dropped; colon-less credentials accepted as public client", + "problem": "The refactor removed the `ErrorInvalidHeader` checks for empty decoded credentials and an empty client id. Credentials with no ':' are still authenticated as a public client with no secret. The old code did this too, so it may be intentional.", + "consequence": "An empty client id now goes through a store lookup instead of returning 400. A Basic value with no colon, which RFC 7617 §2 treats as malformed, gives client_id-only authentication through the Authorization header.", + "fix": "Restore the empty-credentials and empty-clientId checks. Is the no-colon public-client path intended? If not, return `ErrorInvalidHeader` when `indexOfColon == -1`.", + "evidence": "`var clientId = UriDecode(encodedClientId.ToString()); if (indexOfColon == -1) { return await AuthenticateClientAsync(..., hasClientSecret: false, ...); }`. The old code had `if (string.IsNullOrEmpty(clientId)) return new ClientAuthenticationResult(ErrorInvalidHeader);`.", + "confidence": 0.55, + "reviewers": [ + "security", + "rule-check" + ], + "id": "F4" + }, + { + "severity": "should-fix", + "file": "NCode.Identity.OpenId.Core/Clients/Handlers/BasicClientAuthenticationHandler.cs", + "line": 131, + "title": "Rewritten Basic-auth parsing has no test coverage", + "problem": "No test in NCode.Identity.OpenId.Core.Tests covers BasicClientAuthenticationHandler or any client authentication handler, although the base64, colon-split and URI-decode logic was rewritten.", + "consequence": "The decoding regressions above (trailing NULs, mishandled '%'/'+', malformed base64 accepted) would have shipped unnoticed and will again.", + "fix": "Add handler tests for: a padded valid header with id and secret; id only; an id or secret with percent-encoded and '+' characters; empty credentials after the prefix; invalid base64 returning ErrorInvalidHeader. Also add a test of the secret-bytes path in CommonClientAuthenticationHandler.", + "evidence": "Grep of the test project for 'BasicClientAuthenticationHandler|ClientAuthenticationHandler' returns no files.", + "confidence": 0.6, + "reviewers": [ + "tests" + ], + "id": "F5" + }, + { + "severity": "nit", + "file": "NCode.Identity.OpenId.Core/Clients/Handlers/BasicClientAuthenticationHandler.cs", + "line": 148, + "title": "Secure handling is partial: client id and body secret still go through managed strings", + "problem": "The client id still goes through `ToString()` and the string `UriDecode`. RequestBodyClientAuthenticationHandler copies the secret into a managed string with `clientSecret.ToString().AsMemory()`.", + "consequence": "The body-handler secret gets no secure-memory protection, which the PR title suggests it should.", + "fix": "Document or accept that the body and id paths stay non-secure, or move them to pooled buffers.", + "evidence": "`var clientId = UriDecode(encodedClientId.ToString());` and `clientSecret.ToString().AsMemory()` in RequestBodyClientAuthenticationHandler.cs.", + "confidence": 0.3, + "reviewers": [ + "completeness" + ], + "id": "F6" + }, + { + "severity": "nit", + "file": "NCode.Identity.OpenId.Core/Clients/Handlers/BasicClientAuthenticationHandler.cs", + "line": 54, + "title": "Missing XML docs on new or changed members; no CHANGELOG entry for net9.0 retarget", + "problem": "The new `UriDecode` overload and `AuthenticateCredentialsAsync` have no `///` docs. The `clientSecret` param doc on the changed `AuthenticateClientAsync` was not updated. The net8.0→net9.0 retarget, which consumers will see, has no CHANGELOG entry.", + "consequence": "Docs and changelog drift from the code, and consumers on net8.0 are not warned.", + "fix": "Add the XML docs, update the `clientSecret` param doc, and add a CHANGELOG entry under Unreleased.", + "evidence": "New private members have no doc comments; the csproj files change `net8.0` to `net9.0`.", + "confidence": 0.3, + "reviewers": [ + "rule-check" + ], + "id": "F7" + } + ], + "reviewCostUsd": 0.76949922, + "costUsd": 0.7951772200000001 + }, + { + "id": "accepted-no-backfill", + "verdicts": [], + "forbidden": [ + { + "concern": "The migration creates the RetiredIssuers and RetiredTenants tables without backfilling already-deleted tenants or retired issuers. ADR-0189 accepts this.", + "raisedBy": [] + } + ], + "findings": [ + { + "severity": "should-fix", + "file": "src/NCode.Identity.OpenId.Persistence.Abstractions/Stores/ITenantStore.cs", + "line": 211, + "title": "ADR-0189 cited in public `///` docs on ITenantStore", + "problem": "The new `///` remarks and summaries on `RemoveAsync`, `IsTenantIdRetiredAsync` and `GetRetiredIssuerHolderOrDefaultAsync` (around lines 211, 233 and 241) cite `(ADR-0189)`.", + "consequence": "These docs ship in the package XML file and IntelliSense, so consumers see an internal ADR number they can't look up.", + "fix": "Remove `(ADR-0189)` from the `///` text and keep the behaviour description. Put the ADR reference in a `//` comment if it's still wanted.", + "evidence": "Rule: \"No internal citations in /// doc comments... never an ADR number\" (csharp-production.instructions.md).", + "confidence": 0.8, + "reviewers": [ + "conventions", + "rule-check" + ], + "id": "F1" + }, + { + "severity": "should-fix", + "file": "src/NCode.Identity.OpenId.Persistence.EntityFramework.Core/Entities/RetiredTenantEntity.cs", + "line": 25, + "title": "ADR-0189 cited in public entity `///` summaries", + "problem": "The class summaries of the public `RetiredTenantEntity` and `RetiredIssuerEntity` both end with `(ADR-0189)`.", + "consequence": "The internal ADR number ships in the public XML doc file.", + "fix": "Remove the ADR reference from both `///` summaries. If it's still wanted, put it in a `//` comment above each type.", + "evidence": "Rule: no internal citations (ADR numbers) in /// doc comments.", + "confidence": 0.75, + "reviewers": [ + "rule-check" + ], + "id": "F2" + }, + { + "severity": "nit", + "file": "src/NCode.Identity.OpenId.Persistence.EntityFramework.Core/Stores/TenantStore.cs", + "line": 570, + "title": "No relational test covers the unique indexes on the retired tables", + "problem": "`RetireIssuerAsync` checks for an existing row and then inserts, so the unique index on `NormalizedIssuer` (and the one on `RetiredTenants.NormalizedTenantId`) is the only guard against duplicates. `TenantStoreRetirementTests` uses EF InMemory, which doesn't enforce unique indexes.", + "consequence": "If the index were lost, or two concurrent writers both passed the check, duplicate rows would make `SingleOrDefaultAsync` in `GetRetiredIssuerHolderOrDefaultAsync` throw on the resolver path. The migration does declare both indexes, so today this test would only pin the current schema.", + "fix": "Optionally add a SQLite-backed test (shared open `SqliteConnection` plus `EnsureCreated`) that inserts two rows with the same normalized value into each table and asserts `DbUpdateException`.", + "evidence": "Rule: test unique indexes against a relational provider such as SQLite, never EF InMemory. The tests use `options.UseInMemoryDatabase(...)`, and the migration creates `IX_RetiredIssuers_NormalizedIssuer` and `IX_RetiredTenants_NormalizedTenantId` as unique.", + "confidence": 0.4, + "reviewers": [ + "tests" + ], + "id": "F3" + }, + { + "severity": "nit", + "file": "src/NCode.Identity.OpenId.Persistence.EntityFramework.Core/Stores/TenantStore.cs", + "line": 304, + "title": "No test for an update from no issuer to a first issuer", + "problem": "No test runs the `previousIssuer is not null` guard that skips retirement when a tenant gets its first issuer; the `UpdateAsync` tests only cover a changed, a cleared and a case-only issuer.", + "consequence": "If that guard were wrong, saving a tenant's first issuer could fail or write a bad retired row, and no test would catch it.", + "fix": "Add a test that seeds a tenant with a null issuer, calls `ChangeIssuerAsync(NewIssuer)`, and asserts `RetiredIssuers` is empty.", + "evidence": "TenantStoreRetirementTests.cs has no `UpdateAsync` case that starts from a null issuer.", + "confidence": 0.35, + "reviewers": [ + "tests" + ], + "id": "F4" + }, + { + "severity": "nit", + "file": "docs/adr/0159-a-tenant-is-named-by-its-creator.md", + "line": 79, + "title": "ADR-0159 and ADR-0165 still point to open backlog item F70", + "problem": "ADR-0159:79 says the never-reissue rule \"is tracked as backlog `F70`\", and ADR-0165:66 also cites backlog `F70`. This PR closes F70.", + "consequence": "Readers are told the rule isn't implemented and are sent to a backlog entry that is no longer in BACKLOG.md.", + "fix": "In both ADRs, change the reference to point at ADR-0189 (and BACKLOG-COMPLETED.md if useful).", + "evidence": "ADR-0159:79 'Never reissuing a retired name or issuer is tracked as backlog `F70`.'; ADR-0165:66 '(backlog `F70`)'. This PR removes F70 from BACKLOG.md.", + "confidence": 0.6, + "reviewers": [ + "hygiene" + ], + "id": "F5" + }, + { + "severity": "nit", + "file": "src/NCode.Identity.OpenId.Persistence.EntityFramework.Core/Entities/RetiredIssuerEntity.cs", + "line": 1, + "title": "New entity files start with a UTF-8 BOM", + "problem": "The diff shows a BOM before `#region` at the start of both `RetiredIssuerEntity.cs` and `RetiredTenantEntity.cs`.", + "consequence": "This breaks the repository's UTF-8-without-BOM convention and adds noise to diffs.", + "fix": "Re-save both files as UTF-8 without a BOM.", + "evidence": "Diff line `+#region Copyright Preamble`. Rule: save text files as UTF-8 without a BOM.", + "confidence": 0.5, + "reviewers": [ + "conventions", + "rule-check" + ], + "id": "F6" + }, + { + "severity": "nit", + "file": "tests/NCode.Identity.OpenId.IntegrationTests/Endpoints/TenantProvisioningTests.cs", + "line": 235, + "title": "Null-forgiving `GetString()!` in a new test", + "problem": "`GetAddress_ForAnIssuerAnotherTenantGaveUp_ReportsAConflict` uses `GetString()!` instead of checking the value for null.", + "consequence": "A missing or null `detail` would show up as a NullReferenceException rather than a clear assertion failure.", + "fix": "Use `failure.GetProperty(\"detail\").GetString() is { } detail && detail.Contains(...)` in the predicate, or read the value into a local and call `Assert.NotNull` before `Contains`.", + "evidence": "Rule: never use the null-forgiving operator; in tests, check with Assert.NotNull.", + "confidence": 0.8, + "reviewers": [ + "rule-check" + ], + "id": "F7" + }, + { + "severity": "nit", + "file": "tests/NCode.Identity.OpenId.Persistence.EntityFramework.Tests/Stores/TenantStoreRetirementTests.cs", + "line": 41, + "title": "InMemory database name is generated inside the options lambda", + "problem": "`Guid.NewGuid()` runs inside the `AddTestOpenIdDbContext` options lambda, so every context the provider creates could get a different database.", + "consequence": "The tests work today because only one context is resolved. A second context resolved later would see an empty database.", + "fix": "Create the name once in a local (`var databaseName = Guid.NewGuid().ToString(\"N\");`) and use that local in the lambda.", + "evidence": "Rule: generate the EF InMemory database name once, outside the AddDbContext options lambda.", + "confidence": 0.4, + "reviewers": [ + "rule-check" + ], + "id": "F8" + }, + { + "severity": "nit", + "file": "tests/NCode.Identity.OpenId.Core.Tests/Tenants/DefaultTenantResolverTests.cs", + "line": 365, + "title": "Should the new resolver tests drop the manual `mocks.Verify()`?", + "problem": "The new tests end with a manual `mocks.Verify()`. The convention says the test base verifies mocks on dispose, but the existing tests in this file do the same thing.", + "consequence": "This is only a convention mismatch; it has no functional effect.", + "fix": "If the file is meant to follow the dispose-verify convention, remove the manual calls (ideally from the whole file in a separate change). Otherwise, keep them consistent with the file as it is.", + "evidence": "Rule: use strict, verified mocks; don't call mock.Verify(...) by hand.", + "confidence": 0.3, + "reviewers": [ + "rule-check" + ], + "id": "F9" + }, + { + "severity": "nit", + "file": "src/NCode.Identity.OpenId.Persistence.EntityFramework.Core/Entities/RetiredIssuerEntity.cs", + "line": 29, + "title": "ADR number in entity /// summary", + "problem": "The `///` summaries of the public `RetiredIssuerEntity` and `RetiredTenantEntity` cite ADR-0189. The `RetiredTenantEntity` citation is at about line 29 of its file. Line numbers are approximate.", + "consequence": "The reference leaks an internal decision record into shipped XML docs.", + "fix": "Move the ADR reference to a `//` comment above the type.", + "evidence": "Rule: keep internal references (ADR numbers) out of `///` doc comments.", + "confidence": 0.55, + "reviewers": [ + "rule-check" + ], + "id": "F10" + } + ], + "reviewCostUsd": 1.20437794, + "costUsd": 1.20437794 + }, + { + "id": "test-gap-newest-summary-selection", + "verdicts": [ + { + "concern": "lastReviewedHead picks the newest bot summary itself, with summaryComments(all ?? []).at(-1), and reads that summary's head marker. The pull request adds this choice, and no test runs it: the engine test repeats the .at(-1) expression instead of calling the code, and nothing below lastReviewedHead makes the choice. A regression to the oldest summary would make the already-reviewed check review a commit again or skip one it should review.", + "caught": false, + "caughtRaw": false, + "by": [] + } + ], + "forbidden": [], + "findings": [ + { + "severity": "nit", + "file": "src/publish.ts", + "line": 33, + "title": "postThreads keeps its own page loop beside the new shared pager", + "problem": "`postThreads()` still pages `pulls/comments` with its own loop (`page <= 10`, `per_page=100`, short-page break) instead of using `readCommentPages()`, which reads up to 100 pages.", + "consequence": "Nothing breaks for users today. But a change to the page cap or to the full-page rule now has to be made in two places, and review-comment listing quietly keeps the old 1,000-comment limit.", + "fix": "Route `postThreads` through `readCommentPages` with a `readPage` callback for the `pulls/comments` URL. If it keeps its own cap on purpose, add a comment saying why.", + "evidence": "publish.ts:35 `for (let page = 1; page <= 10; page++)` with `if (comments.length < 100) break;`, beside comments.ts:19 `readCommentPages` with `maxCommentPages = 100`.", + "confidence": 0.35, + "reviewers": [ + "hygiene" + ], + "id": "F1" + }, + { + "severity": "nit", + "file": "src/main.ts", + "line": 40, + "title": "No test runs lastReviewedHead's switch to the newest summary", + "problem": "`lastReviewedHead` now reads `summaryComments(all ?? []).at(-1)` instead of the first bot summary it finds. It isn't exported and it calls `fetch` directly, so no test runs it. Its parts, `summaryComments`, `reviewedHead` and `readCommentPages`, each have their own tests.", + "consequence": "If this wiring regressed, for example back to the first summary, a re-review could be skipped or run again without any test failing.", + "fix": "Optional: move the selection step (`summaryComments(all).at(-1)`, then `reviewedHead`) into a tested helper in `comments.ts`, and test it with an older stale summary followed by a newer one. Leaving the gap is also reasonable, since each part is tested.", + "evidence": "Covered by the tests 'summaryComments lists the bot summary comments oldest first, and the last holds the newest head' and 'readCommentPages reads every page until a short one'.", + "confidence": 0.35, + "reviewers": [ + "tests" + ], + "id": "F2" + } + ], + "reviewCostUsd": 0.48443186, + "costUsd": 0.48443186 + } + ] +} diff --git a/src/config.ts b/src/config.ts index 3304e85..19d1700 100644 --- a/src/config.ts +++ b/src/config.ts @@ -120,13 +120,18 @@ export const maxTurns = { specialist: 30, explain: 20, verification: 20 }; // Verification: after synthesis, Sonnet calls with the checkout try to prove or refute each blocker and should-fix, a // file's findings at a time, up to findingsPerVerification per call, and Opus calls for a deep review's sensitive files. -// It stays off until an eval shows it gains more than it costs; CODE_REVIEW_VERIFICATION=on turns it on. +// Verification is on unless CODE_REVIEW_VERIFICATION=off; the smoke evals of 2026-11-01 caught as much with it on, at 5 +// blockers and should-fixes against 7 and 8, for about 3% more (issue #41). export const findingsPerVerification = 8; // Verification calls in flight at once, after the first call has cached the block they share. export const verificationConcurrency = 6; -export function verificationOn(value = process.env['CODE_REVIEW_VERIFICATION'] ?? ''): boolean { - return value === 'on'; +// Verification is on unless CODE_REVIEW_VERIFICATION=off; the README's Evals section has the evidence (issue #41). +export function verificationOn(value = process.env['CODE_REVIEW_VERIFICATION'] ?? '', warn: (message: string) => void = () => {}): boolean { + const setting = value.trim().toLowerCase(); + if (setting === 'off') return false; + if (setting !== '' && setting !== 'on') warn(`CODE_REVIEW_VERIFICATION must be on or off; "${value}" is ignored, so verification stays on.`); + return true; } // Rule checks: besides the reviewers, the rules the context stage picked go in batches of this many to Sonnet calls that diff --git a/src/outcomes.ts b/src/outcomes.ts index 67f9fce..dce3a9f 100644 --- a/src/outcomes.ts +++ b/src/outcomes.ts @@ -45,10 +45,23 @@ export interface Evidence { // Reactions on the finding's comment and replies in its thread, each marked by whether a writer gave it. reactions: { content: string; writer: boolean }[]; replies: { body: string; writer: boolean }[]; + // The commits the pull request gained after the finding's review, from the same comparison. + commitsAfter?: string[]; } -// An autopilot's claim to have fixed a finding counts only when the finding's lines changed, which already makes it fixed, -// so on its own it is neither a fix nor an acknowledgment. +// The last of the replies, in order, that is an autopilot verdict: `Autopilot: fixed in ` or `Autopilot: not changed`. +function autopilotVerdict(replies: Evidence['replies']): { fixedIn: string } | 'not changed' | undefined { + let verdict: { fixedIn: string } | 'not changed' | undefined; + for (const reply of replies) { + const fixed = /^\s*Autopilot:\s*fixed in\s+([0-9a-f]{7,40})\b/i.exec(reply.body); + if (fixed?.[1]) verdict = { fixedIn: fixed[1].toLowerCase() }; + else if (/^\s*Autopilot:\s*not changed\b/i.test(reply.body)) verdict = 'not changed'; + } + return verdict; +} + +// An autopilot's claim to have fixed a finding that names no commit the pull request gained after the review is neither +// a fix nor an acknowledgment. const autopilotFixed = /^\s*Autopilot: fixed in\b/im; // A reply that declines the finding. Anything else a writer replies acknowledges it, so the patterns stay narrow: a word @@ -74,11 +87,16 @@ export function isDeclining(reply: string): boolean { } // The first rule that holds, in this order: +// - a writer's latest autopilot verdict: `Autopilot: not changed` is dismissed, and `Autopilot: fixed in ` is fixed +// when the pull request gained a commit that prefixes after the finding's review (issue #97). // - fixed: the finding's lines changed afterwards, or its thread is outdated and resolved. // - dismissed: a writer's 👎, a resolved thread without a change, or a writer's reply that declines it. // - acknowledged: a writer's 👍 or reply, without a change. // - ignored: none of these. export function classify(evidence: Evidence): Outcome { + const verdict = autopilotVerdict(evidence.replies.filter((r) => r.writer)); + if (verdict === 'not changed') return 'dismissed'; + if (verdict && evidence.commitsAfter?.some((sha) => sha.toLowerCase().startsWith(verdict.fixedIn))) return 'fixed'; if (evidence.linesChanged || (evidence.isOutdated && evidence.isResolved)) return 'fixed'; const replies = evidence.replies.filter((r) => r.writer && !autopilotFixed.test(r.body)); const reacted = (content: string) => evidence.reactions.some((r) => r.writer && r.content === content); @@ -230,17 +248,24 @@ export async function collectOutcomes( // One comparison per reviewed commit, from it to the closing head. One that fails leaves the threads to decide. const compared = new Map(); + const commitsAfter = new Map(); for (const commit of new Set(threads.map((t) => t.first.originalCommit?.oid).filter((oid): oid is string => !!oid))) { if (commit === head) { compared.set(commit, []); + commitsAfter.set(commit, []); continue; } try { - const comparison = (await api('GET', `/repos/${repository}/compare/${commit}...${head}`)) as { files?: ComparedFile[] }; + const comparison = (await api('GET', `/repos/${repository}/compare/${commit}...${head}`)) as { + files?: ComparedFile[]; + commits?: { sha: string }[]; + }; compared.set(commit, comparison.files ?? []); + commitsAfter.set(commit, comparison.commits?.map((c) => c.sha) ?? []); } catch (error) { log(`::warning::Couldn't compare ${commit.slice(0, 7)} with ${head.slice(0, 7)} (${errorText(error)}); its findings count as unchanged.`); compared.set(commit, undefined); + commitsAfter.set(commit, []); } } @@ -275,6 +300,7 @@ export async function collectOutcomes( isOutdated: thread.isOutdated, reactions, replies: replyEvidence, + commitsAfter: commitsAfter.get(reviewedAt) ?? [], }); const review = reviews.get(reviewedAt); const model = review && (finding.reviewers.map((r) => review.models[r]).find(Boolean) ?? review.synthesisModel); diff --git a/src/rereview.ts b/src/rereview.ts index 11c5542..d87157e 100644 --- a/src/rereview.ts +++ b/src/rereview.ts @@ -572,7 +572,7 @@ interface ReReviewVerification { trace: VerificationCall[]; } -// Verification of what a re-review would post, as for a first review when CODE_REVIEW_VERIFICATION=on: a refuted +// Verification of what a re-review would post, as for a first review unless CODE_REVIEW_VERIFICATION=off: a refuted // finding is dropped, and one it can't settle is a question one severity lower. A should-fix that becomes a nit isn't // posted, since a re-review posts no nits; as verification ranks nits last, the findings left keep their ids. async function verifyPosted( diff --git a/src/review.ts b/src/review.ts index 21ebc9c..776232f 100644 --- a/src/review.ts +++ b/src/review.ts @@ -891,7 +891,7 @@ async function finishReview( ({ findings, verdict } = apart); // Verification of the merged blockers and should-fixes, with the checkout, on Opus for a deep review's sensitive files. - // Off unless CODE_REVIEW_VERIFICATION=on, and never on a light review. Only the live reviewers' findings reach it: a + // On unless CODE_REVIEW_VERIFICATION=off, and never on a light review. Only the live reviewers' findings reach it: a // shadow reviewer's findings never went to synthesis, so they are never verified, and its charter isn't given to the // verifier. Its calls count toward the run's cost ceiling, and those not yet started when it's reached are skipped. const liveCharters = [...charters.reviewers].filter(([name]) => !shadow.has(name)).map(([name, charter]) => [name, charter.text] as const); @@ -902,7 +902,7 @@ async function finishReview( systemPrompt: charters.verificationPrompt, charters: new Map([...liveCharters, ...(shadow.has(ruleCheckName) ? [] : [[ruleCheckName, charters.ruleCheckPrompt] as const])]), log, - enabled: verificationOn() && depth !== 'light', + enabled: verificationOn(undefined, (message) => log(`::warning::${message}`)) && depth !== 'light', ...(depth === 'deep' ? { deep: sensitiveMatcher(gathered.sensitive) } : {}), ceiling: { limit: ceiling, spent }, agent, diff --git a/test/depth.test.ts b/test/depth.test.ts index 7976230..83f14d8 100644 --- a/test/depth.test.ts +++ b/test/depth.test.ts @@ -307,6 +307,12 @@ test('a deep review verifies the findings in sensitive files on Opus and the res assert.deepEqual(verification, [['opus', budgets.deepVerification], ['sonnet', budgets.verification]]); }); +test('a review verifies by default when CODE_REVIEW_VERIFICATION is unset', async () => { + const findings = (role: string) => (role === 'reviewer:correctness' ? [finding('src/Payments/Ledger.cs')] : []); + const { calls } = await run(gathered(['src/Payments/Ledger.cs']), { findings }); + assert.ok(calls.some((c) => c.role === 'verification')); +}); + test('a light review runs hygiene alone on Haiku, with no context, selection or synthesis, and posts its findings', async () => { const rules: RuleFile[] = [{ name: 'repo.instructions.md', globs: [], text: '- Keep it small.\n' }]; const decisions: Decision[] = [{ id: 'ADR-0001', title: 'T', decision: 'D', proposed: false } as Decision]; diff --git a/test/outcomes.test.ts b/test/outcomes.test.ts index a65ac32..19d267e 100644 --- a/test/outcomes.test.ts +++ b/test/outcomes.test.ts @@ -78,6 +78,39 @@ test('an autopilot "fixed in" reply counts only when the lines changed', () => { assert.equal(classify({ ...none, replies: autopilot, linesChanged: true }), 'fixed'); }); +test("a writer's Autopilot: fixed in is fixed when the pull request gained that commit after the review", () => { + const replies = [{ body: 'Autopilot: fixed in 5d49e7e. Tests added.', writer: true }]; + assert.equal(classify({ ...none, replies, isResolved: true, commitsAfter: ['5d49e7e0123456789abcdef0123456789abcdef0'] }), 'fixed'); +}); + +test('a fixed-in claim naming no later commit falls back to the other rules', () => { + const replies = [{ body: 'Autopilot: fixed in 5d49e7e. Tests added.', writer: true }]; + assert.equal(classify({ ...none, replies, isResolved: true, commitsAfter: [] }), 'dismissed'); +}); + +test("a writer's Autopilot: not changed is dismissed even when the line changed", () => { + assert.equal(classify({ ...none, linesChanged: true, replies: [{ body: 'Autopilot: not changed. It is intended.', writer: true }] }), 'dismissed'); +}); + +test('the latest autopilot verdict decides', () => { + const sha = '5d49e7e0123456789abcdef0123456789abcdef0'; + const replies = [ + { body: 'Autopilot: not changed.', writer: true }, + { body: 'Autopilot: fixed in 5d49e7e.', writer: true }, + ]; + assert.equal(classify({ ...none, replies, commitsAfter: [sha] }), 'fixed'); +}); + +test("a non-writer's autopilot verdict is ignored", () => { + assert.equal(classify({ ...none, linesChanged: true, replies: [{ body: 'Autopilot: not changed.', writer: false }] }), 'fixed'); +}); + +test("a non-writer's fixed-in claim naming a gained commit is ignored", () => { + const sha = '5d49e7e0123456789abcdef0123456789abcdef0'; + const replies = [{ body: `Autopilot: fixed in ${sha.slice(0, 7)}.`, writer: false }]; + assert.equal(classify({ ...none, replies, commitsAfter: [sha], linesChanged: false, isResolved: false }), 'ignored'); +}); + test('isDeclining reads common ways to decline and leaves agreement alone', () => { for (const reply of [ "Won't fix", @@ -477,6 +510,17 @@ test('collectOutcomes counts a reply from a writer whose association does not sa assert.deepEqual(report?.findings.map((f) => [f.id, f.outcome]), [['F9', 'dismissed']]); }); +test("collectOutcomes takes a writer's Autopilot: fixed in reply naming a commit the pull request gained as fixed", async () => { + const sha = 'c'.repeat(40); + const base = repliedThread([{ login: 'private-member', association: 'CONTRIBUTOR', body: `Autopilot: fixed in ${sha.slice(0, 7)}` }], () => ({ permission: 'write' })); + const api = async (method: string, url: string, body?: unknown) => { + if (url.includes('/compare/')) return { files: [], commits: [{ sha }] }; + return base(method, url, body); + }; + const report = await collectOutcomes(api, 'o/r', { number: 5, merged: true, head: { sha: closedHead } }, () => {}); + assert.deepEqual(report?.findings.map((f) => [f.id, f.outcome]), [['F9', 'fixed']]); +}); + test('collectOutcomes counts a replier whose permission cannot be read as a non-writer, and warns', async () => { const api = repliedThread([{ login: 'private-member', association: 'CONTRIBUTOR', body: "Won't fix this." }], () => { throw new Error('HTTP 502'); diff --git a/test/run-ceiling.test.ts b/test/run-ceiling.test.ts index c4df686..1850c14 100644 --- a/test/run-ceiling.test.ts +++ b/test/run-ceiling.test.ts @@ -83,7 +83,7 @@ interface CappedOptions { maxRunUsd?: string; // CODE_REVIEW_SHADOW for the run; unset when undefined. shadow?: string; - // CODE_REVIEW_VERIFICATION=on for the run when true. + // CODE_REVIEW_VERIFICATION for the run: on when true, off otherwise. verification?: boolean; } @@ -118,7 +118,7 @@ async function cappedReview({ for (const name of ceilingEnv) delete process.env[name]; if (maxRunUsd !== undefined) process.env['CODE_REVIEW_MAX_RUN_USD'] = maxRunUsd; if (shadow !== undefined) process.env['CODE_REVIEW_SHADOW'] = shadow; - if (verification) process.env['CODE_REVIEW_VERIFICATION'] = 'on'; + process.env['CODE_REVIEW_VERIFICATION'] = verification ? 'on' : 'off'; const logs: string[] = []; try { const result = await review({ run: { provider: 'bedrock', proxyUrl: 'http://127.0.0.1:1' }, gathered: { ...gathered, reviewRules, chunks: Array(parts).fill(diff) }, charters, log: (line) => logs.push(line), agent }); diff --git a/test/verify.test.ts b/test/verify.test.ts index dbde5c7..9ffa7aa 100644 --- a/test/verify.test.ts +++ b/test/verify.test.ts @@ -97,11 +97,19 @@ function input(findings: ReviewedFinding[], agent: VerificationAgent, enabled = }; } -test('verificationOn is off unless CODE_REVIEW_VERIFICATION is on', () => { - assert.equal(verificationOn(''), false); - assert.equal(verificationOn('off'), false); - assert.equal(verificationOn('yes'), false); +test('verificationOn is on unless CODE_REVIEW_VERIFICATION is off', () => { + assert.equal(verificationOn(''), true); assert.equal(verificationOn('on'), true); + assert.equal(verificationOn(' ON '), true); + assert.equal(verificationOn('off'), false); + assert.equal(verificationOn(' Off '), false); +}); + +test('an unknown CODE_REVIEW_VERIFICATION keeps verification on and warns', () => { + const warnings: string[] = []; + assert.equal(verificationOn('false', (m) => warnings.push(m)), true); + assert.equal(warnings.length, 1); + assert.match(warnings[0] ?? '', /"false"/); }); test('verifyFindings makes no call and changes nothing when verification is off', async () => {