diff --git a/.github/workflows/smoke-eval.yml b/.github/workflows/smoke-eval.yml index 8a2acf6..03c5cef 100644 --- a/.github/workflows/smoke-eval.yml +++ b/.github/workflows/smoke-eval.yml @@ -3,7 +3,8 @@ # # It runs the smoke subset in evals/cases.ts with the production guidance, stops and fails past $8, and compares the run # with evals/baseline-smoke.json in the job summary: it fails when caught drops by more than 2 or the must-not case is -# flagged. With no baseline yet, the run is uploaded as the proposed one and the job passes with a notice. +# flagged. Until a baseline is first committed, the run is uploaded as the proposed one and the job passes with a notice; +# once one has been, a run without it fails. # # It uses the organization's Bedrock key in the code-review environment, which only the default branch can use. Never # add a pull_request trigger: the key would then run code from any pull request. @@ -41,6 +42,8 @@ jobs: steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: + # The full history, so the check can tell a deleted baseline from one never committed. + fetch-depth: 0 persist-credentials: false - uses: actions/setup-node@949feb2413d6458794dcd2491c4babbbce0c15c1 # v7.1.0 with: diff --git a/CHANGELOG.md b/CHANGELOG.md index 5efb8e8..28ef48e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), - The `api-compat` floor places each changed C# line in its member by reading the file's braces, with comments, strings and preprocessor lines left out, instead of its indentation, so unusual formatting no longer misplaces a change; a file whose braces don't balance counts as a public change ([#112](https://github.com/NCodeGroup/code-review/issues/112)). - A re-review posts a problem `correctness` and `security` both report, in the same file within three lines and with the same title words, once, at the higher severity and naming both reviewers, instead of two threads; its log counts these merges and the findings of the two that sit that close without matching, for the evals to measure ([#116](https://github.com/NCodeGroup/code-review/issues/116)). - The evals have a re-review case whose changes touch a file the selection floor gives to `security` and one whose changes touch a `sensitive:` path, which a case can now set for a repository that has none; `--subset re-review` runs the four re-review cases, whose output has each one's cost ([#117](https://github.com/NCodeGroup/code-review/issues/117)). +- The smoke eval fails when `evals/baseline-smoke.json` is missing after it has been committed, naming the commit that deleted it, so deleting the baseline can't turn the gate off; until the first baseline is committed it still passes with a notice. Its workflow checks out the full history for this ([#93](https://github.com/NCodeGroup/code-review/issues/93)). +- The tests check that the smoke eval workflow uploads its outputs only after the secret scan passes and fails the job when the run or the check fails, and that `evals/run.ts` writes its output before it fails a run over `--max-cost` ([#93](https://github.com/NCodeGroup/code-review/issues/93)). +- A private organization member's comment counts as the maintainer's for the loop limit, their open issues can track a finding, and the bot answers their `Autopilot:` replies: these use the engine's writer check, the author's association or else their permission on the repository, and a lookup that fails counts as no write access. The README says how such a member can use `/review`, whose gate still reads only the association ([#89](https://github.com/NCodeGroup/code-review/issues/89)). ### Fixed diff --git a/README.md b/README.md index 5cfe811..198c876 100644 --- a/README.md +++ b/README.md @@ -42,8 +42,11 @@ The reviewers see a pull request's description, its commit subjects and the issu their authors can write to the repository. The engine asks the repository's permissions when the event doesn't say so: GitHub reports an organization member whose membership is private as a contributor. The prompt says whether the pull request has a description, and when one was left out, so no reviewer reports a description it wasn't shown as missing. -A `/review` comment still counts only when its `author_association` names a writer, as the workflow's `if:` below -checks before the engine starts, so a private organization member can't start a review that way. +The loop limit's count of maintainer comments, the open issues a finding can be tracked in and the `Autopilot:` replies +the bot answers use the same check. A `/review` comment still counts only when its `author_association` names a writer, +as the workflow's `if:` below checks before the engine starts: a permission lookup there would let more people start a +run that holds the model credentials. So a private organization member can't start a review that way; +[Commands](#commands) says what they can do. Besides the reviewers, **rule checks** go through the rules the context stage picked for the pull request, twelve at a time, on Sonnet: each call checks its rules one by one against every part of the diff, searching the changed files for what the rule's check names. They report as `rule-check` and run on every review, outside the reviewer roster, so selection can't skip them. In the evals they caught about one more expected finding in 55, at about 70% more cost. `CODE_REVIEW_RULE_CHECKS=off` turns them off. @@ -194,6 +197,9 @@ the source above but hasn't been tried on a consumer yet. ## Commands Comment on a pull request, as a top-level comment, not a reply in a review thread. Only people with write access can. +GitHub reports an organization member whose membership is private as a contributor, so their commands are ignored. +To use them, change your membership from Private to Public on the organization's People page, or have an admin add +you to the repository as a collaborator. | Comment | What happens | |---|---| @@ -346,7 +352,9 @@ After three reviews with no comment from someone with write access in between, c automatic review stops before any model call and posts once "Paused: comment `/review` to resume". An acknowledged "Code review" check run is completed as "Review paused", so it never stays in progress. Comments the autopilot posts as the maintainer's account, which start with `Autopilot:`, don't count as the maintainer's. A -`/review` from someone with write access always reviews, and counts as their comment. +`/review` from someone with write access always reviews, and counts as their comment. A comment counts as from someone +with write access by its author's association or, when that doesn't say so, their permission on the repository; one +whose permission can't be read doesn't count. ### Re-reviews @@ -381,8 +389,9 @@ of review converge: says so instead, and its verdict is never Approve; - it closes the loop on the bot's own threads, never anyone else's: it resolves the thread of an earlier finding it found fixed when the thread is outdated or its lines changed since the last review. When it finds one still open - and the thread's last comment is an `Autopilot:` reply, it replies once to say why it stays open, ending with a line - that asks the maintainer (`@: needs you.`), and never a second time in that thread. + and the thread's last comment is an `Autopilot:` reply from someone with write access, it replies once to say why it + stays open, ending with a line that asks the maintainer (`@: needs you.`), and never a second time in that + thread. A `/review` of the commit already reviewed, or a last reviewed head that can't be fetched any more (after a force-push), gets a full review instead. A review whose reviewers all failed doesn't count as the last review, nor does a re-review @@ -508,7 +517,7 @@ Each case's pull request is built as the review step builds it: `author` and `au **Capping the cost.** `--max-cost ` stops a run once its cost, judges included, passes that many dollars: the case that crossed it finishes, the rest don't start, the output covers the cases that ran, and the run fails. `--subset ` runs a named set of cases from `evals/cases.ts` instead of `--only id,id`; a name that matches no case fails the run. -**Smoke eval.** The [Smoke eval](.github/workflows/smoke-eval.yml) workflow runs on every push to `main` that changes `prompts/`, `reviewers/`, `src/config.ts` or `guidance/`, and by hand (`gh workflow run smoke-eval.yml`). It runs in the `code-review` environment with the organization's Bedrock key, so it never runs on a pull request. It runs the `smoke` subset, 4 cases with the must-not case and one large diff, with the production guidance, `--precision off` and `--max-cost 8`. `node evals/smoke.ts check` then compares the run with `evals/baseline-smoke.json`, posts the comparison to the job summary, and fails when caught drops by more than 2, when the must-not case is flagged, or when the two cover different cases. `evals/out` is scanned for secrets before it's uploaded as the `smoke-eval` artifact, since artifacts on a public repository are public. With no baseline yet, the run passes with a notice, failing only on a flagged must-not case. To adopt a run as the baseline, the first time or after an intended change in what the smoke cases catch or a change to the subset, take `proposed-baseline-smoke.json` from that run's artifact, which is the run without its agent calls and raw findings, and commit it in a pull request: +**Smoke eval.** The [Smoke eval](.github/workflows/smoke-eval.yml) workflow runs on every push to `main` that changes `prompts/`, `reviewers/`, `src/config.ts` or `guidance/`, and by hand (`gh workflow run smoke-eval.yml`). It runs in the `code-review` environment with the organization's Bedrock key, so it never runs on a pull request. It runs the `smoke` subset, 4 cases with the must-not case and one large diff, with the production guidance, `--precision off` and `--max-cost 8`. `node evals/smoke.ts check` then compares the run with `evals/baseline-smoke.json`, posts the comparison to the job summary, and fails when caught drops by more than 2, when the must-not case is flagged, or when the two cover different cases. `evals/out` is scanned for secrets before it's uploaded as the `smoke-eval` artifact, since artifacts on a public repository are public. Until `evals/baseline-smoke.json` is first committed, the run passes with a notice, failing only on a flagged must-not case. Once it has been, a run without it fails and names the commit that deleted it, so deleting the file can't turn the gate off; the workflow checks out the full history so the check can tell, and the check fails on a shallow checkout. To adopt a run as the baseline, the first time or after an intended change in what the smoke cases catch or a change to the subset, take `proposed-baseline-smoke.json` from that run's artifact, which is the run without its agent calls and raw findings, and commit it in a pull request: ```powershell gh run download -n smoke-eval -D evals/out/smoke diff --git a/evals/run.ts b/evals/run.ts index 7601eba..aeb6698 100644 --- a/evals/run.ts +++ b/evals/run.ts @@ -264,6 +264,16 @@ export function tallyCase( // How the output names a finding that raised a concern. const raisedLabel = (finding: Finding & { reviewers?: string[] }) => `${finding.file}:${finding.line} (${(finding.reviewers ?? []).join(', ')})`; +// Writes the run's output to /