mirror of
https://github.com/expo/code-review-cli.git
synced 2026-11-15 03:38:09 -10:00
Compare commits
| Author | SHA1 | Date | |
|---|---|---|---|
 Kudo ChienandClaude Opus 5.5
|
e18dc16ba1 | ||
|
Brent Vatne
|
771835a110 |
No files matched your search
@@ -607,8 +607,11 @@ comments, and the run reviewed the whole PR. These runs get no label:
|
||||
|
||||
A run that gets no label, a `/skip-review` that `ecr ci` acts on, or a run that
|
||||
deletes the review comment removes the label, but only when the reviewer added
|
||||
it last. Nothing else changes the label, so a ready label added before a push
|
||||
stays until the next review posts.
|
||||
it last. `ecr ci` also removes it when new commits are pushed, before it reviews
|
||||
them, so the label never says "ready" for commits no review has seen. With a
|
||||
personal or GitHub App token that removal starts another run of the workflow,
|
||||
which cancels this one and reviews the same commits; the default `GITHUB_TOKEN`
|
||||
starts no runs.
|
||||
|
||||
- `ecr post-review` and `ecr review --pr <n> --post` post nothing on a PR with
|
||||
`/skip-review`, and `ecr ci` does nothing on a PR its trigger policy skips (such
|
||||
@@ -617,17 +620,16 @@ stays until the next review posts.
|
||||
- The reviewer recognises its labels by login, from the label's newest
|
||||
`labeled` or `unlabeled` event on the PR. It removes the label only when its
|
||||
own login added it last, and does not add it again when another login removed
|
||||
it last. Its login is `gh api user`, or, when that fails under GitHub Actions,
|
||||
`github-actions[bot]`, the login of the default `GITHUB_TOKEN`. So:
|
||||
it last. Its login is `gh api user` for a personal token, or GraphQL `viewer`
|
||||
for an installation token (the default `GITHUB_TOKEN`, `github-actions[bot]`, or
|
||||
a GitHub App's, `<app>[bot]`); when both fail under GitHub Actions it assumes
|
||||
`github-actions[bot]`. So:
|
||||
- With a personal token, that person's own removals count as the reviewer's,
|
||||
and the next ready review adds the label again.
|
||||
- Under the default `GITHUB_TOKEN`, other workflows' label changes are
|
||||
`github-actions[bot]` too, and count as the reviewer's.
|
||||
- A local `ecr review --pr <n> --post` and CI use different logins, so neither
|
||||
removes a label the other added.
|
||||
- A GitHub App token cannot read `gh api user`, and its login is not
|
||||
`github-actions[bot]`, so the reviewer never removes the label it added (nor
|
||||
recognises its own review comment).
|
||||
- On a PR with 3,000 events or more, the reviewer cannot tell who changed the
|
||||
label last, and leaves it as it is.
|
||||
- It works in the single comment mode. Per-scope comments have no one decision
|
||||
|
||||
@@ -6,10 +6,10 @@
|
||||
"name": "@expo/code-review-cli",
|
||||
"dependencies": {
|
||||
"@modelcontextprotocol/sdk": "1.30.0",
|
||||
"@opencode-ai/sdk": "1.18.4",
|
||||
"@opencode-ai/sdk": "1.19.5",
|
||||
"cheerio": "1.1.2",
|
||||
"minisearch": "7.2.0",
|
||||
"opencode-ai": "1.18.4",
|
||||
"opencode-ai": "1.19.5",
|
||||
"zod": "^4.4.3",
|
||||
},
|
||||
"devDependencies": {
|
||||
@@ -29,7 +29,7 @@
|
||||
|
||||
"@modelcontextprotocol/sdk": ["@modelcontextprotocol/[email protected]", "", { "dependencies": { "@hono/node-server": "^1.19.9 || ^2.0.5", "ajv": "^8.17.1", "ajv-formats": "^3.0.1", "content-type": "^1.0.5", "cors": "^2.8.5", "cross-spawn": "^7.0.5", "eventsource": "^3.0.2", "eventsource-parser": "^3.0.0", "express": "^5.2.1", "express-rate-limit": "^8.2.1", "hono": "^4.11.4", "jose": "^6.1.3", "json-schema-typed": "^8.0.2", "pkce-challenge": "^5.0.0", "raw-body": "^3.0.0", "zod": "^3.25 || ^4.0", "zod-to-json-schema": "^3.25.1" }, "peerDependencies": { "@cfworker/json-schema": "^4.1.1" }, "optionalPeers": ["@cfworker/json-schema"] }, "sha512-xKd8OIzlqNzcqcNumGAa6g+PW2kjD5vrpcKOnfldAUPP3j7lnqMPwlTXQm8gF+UwH72z0lqaRbjr9hqGz0eITA=="],
|
||||
|
||||
"@opencode-ai/sdk": ["@opencode-ai/[email protected]8.4", "", { "dependencies": { "cross-spawn": "7.0.6" } }, "sha512-p/3P0KtWknoLvpsk8QrUzCKd4Q1A5Z3RmACEvwuQBGxnUy4AGo379lUYMgt7fczFn53079oroLuo6BosQri+HA=="],
|
||||
"@opencode-ai/sdk": ["@opencode-ai/[email protected]9.5", "", { "dependencies": { "cross-spawn": "7.0.6" } }, "sha512-+bqu8gHiwmq96T9q5G34uLNHWCInIMsrE3hytCNHj1cVaxjY0AGJyNCT1rkeeZwgPU7A5TMTNwN+0XxCO2vqDg=="],
|
||||
|
||||
"@oxfmt/binding-android-arm-eabi": ["@oxfmt/[email protected]", "", { "os": "android", "cpu": "arm" }, "sha512-1q4q4Jc8FlOMVojEisyFAVyl8h1yawNv6phjgmhGVEDeyeOdsSnSr9x0+D4mOnEKvpO5L4mxKZ/DP9X6U3A/Mw=="],
|
||||
|
||||
@@ -271,31 +271,31 @@
|
||||
|
||||
"once": ["[email protected]", "", { "dependencies": { "wrappy": "1" } }, "sha512-lNaJgI+2Q5URQBkccEKHTQOPaXdUxnZZElQTZY0MFUAuaEqe1E+Nyvgdz/aIyNi6Z9MzO5dv1H8n58/GELp3+w=="],
|
||||
|
||||
"opencode-ai": ["[email protected]8.4", "", { "optionalDependencies": { "opencode-darwin-arm64": "1.18.4", "opencode-darwin-x64": "1.18.4", "opencode-darwin-x64-baseline": "1.18.4", "opencode-linux-arm64": "1.18.4", "opencode-linux-arm64-musl": "1.18.4", "opencode-linux-x64": "1.18.4", "opencode-linux-x64-baseline": "1.18.4", "opencode-linux-x64-baseline-musl": "1.18.4", "opencode-linux-x64-musl": "1.18.4", "opencode-windows-arm64": "1.18.4", "opencode-windows-x64": "1.18.4", "opencode-windows-x64-baseline": "1.18.4" }, "os": [ "linux", "win32", "darwin", ], "cpu": [ "x64", "arm64", ], "bin": { "opencode": "bin/opencode.exe" } }, "sha512-B8pFAs1g158ZU+C6eSiJz/5ZhG7Vr2mH7Z7ruYEHLElAlX9tehrRTnzTTgjSxHY2vwZ/5VplwR3odFU+Dai8jg=="],
|
||||
"opencode-ai": ["[email protected]9.5", "", { "optionalDependencies": { "opencode-darwin-arm64": "1.19.5", "opencode-darwin-x64": "1.19.5", "opencode-darwin-x64-baseline": "1.19.5", "opencode-linux-arm64": "1.19.5", "opencode-linux-arm64-musl": "1.19.5", "opencode-linux-x64": "1.19.5", "opencode-linux-x64-baseline": "1.19.5", "opencode-linux-x64-baseline-musl": "1.19.5", "opencode-linux-x64-musl": "1.19.5", "opencode-windows-arm64": "1.19.5", "opencode-windows-x64": "1.19.5", "opencode-windows-x64-baseline": "1.19.5" }, "os": [ "linux", "win32", "darwin", ], "cpu": [ "x64", "arm64", ], "hasInstallScript": true, "bin": { "opencode": "bin/opencode.exe" } }, "sha512-tEqMvQahfhdDj8vceYpXtoIrvjPXm5XRfetWnPN5YISueU7adNx9JcUA3zZ+2GKuDVgDpGdQe1ZLtCZDjqodvA=="],
|
||||
|
||||
"opencode-darwin-arm64": ["[email protected]8.4", "", { "os": "darwin", "cpu": "arm64" }, "sha512-/GdcUv3axBFYmpdCY3yMGRCBq3fwAJayflnn9stXtIHKuckc7ZaUYZmyudIUpmzkZP5W8XsAVMJomR4Rbr2+Ig=="],
|
||||
"opencode-darwin-arm64": ["[email protected]9.5", "", { "os": "darwin", "cpu": "arm64" }, "sha512-0MbSbKkmwVHtgP31GJyyECe6QVy4T7W4Wp7s2TsmXHTPPyf01ne3dWSX97X9dkNK28Lc6kt+zMPQUP/9GgDLZg=="],
|
||||
|
||||
"opencode-darwin-x64": ["[email protected]8.4", "", { "os": "darwin", "cpu": "x64" }, "sha512-0i/AmdFw3CerwIggKqtaDlWDCBfSgzXT+KPSvks/Dx6l9NfYHihLnbV+vKbr0lwzco1lsDo4JHE4j/zz7JZUKQ=="],
|
||||
"opencode-darwin-x64": ["[email protected]9.5", "", { "os": "darwin", "cpu": "x64" }, "sha512-+DO7IMPTCrX12UJXnL4pL3ckcwR6BR6f6tA9QIlVQ5nZx66T5U7FRMJT4n5rsOnJxLM/pZ2vfX1G8vrnvXO8FA=="],
|
||||
|
||||
"opencode-darwin-x64-baseline": ["[email protected]8.4", "", { "os": "darwin", "cpu": "x64" }, "sha512-1v/keJ/m+3UplGU2XdP5YfK25rTlGi7Kf5BVSV/ICObk6PiJWBAWOrB8bAQWDLhN/Tn7UfogBAgxaiRRfLPuEA=="],
|
||||
"opencode-darwin-x64-baseline": ["[email protected]9.5", "", { "os": "darwin", "cpu": "x64" }, "sha512-rftvAoHBWPVkvwBLr3SQWRZvcIjyMTN0SlhMzlt+lgvoQBBGNEmZQ/7sS/Ux1oHsJV/++UDuLVso4n5L95YD5Q=="],
|
||||
|
||||
"opencode-linux-arm64": ["[email protected]8.4", "", { "os": "linux", "cpu": "arm64" }, "sha512-3gBw9weE76jloNPGmPvzl3GJR8b6scwSAENxUKRdJ3WCkC7qcjJm49KFyfqFjO5++1H1Q2jbmqqxAgt5upqoMQ=="],
|
||||
"opencode-linux-arm64": ["[email protected]9.5", "", { "os": "linux", "cpu": "arm64" }, "sha512-u2HFRI6v1nNMVKiV34An8XrGBxJeZ+RCg2tYu3SqlGODKqKsr5RoEbaX1tnsfGb+rETh9AJVX9kS8dSZXPufsA=="],
|
||||
|
||||
"opencode-linux-arm64-musl": ["[email protected]8.4", "", { "os": "linux", "cpu": "arm64" }, "sha512-kno1QJz+JoY1YqGlrxcwGKqOh2/OFKdQ4UGUCN5K2evQRIhd9EfB4JvZkjzLxTzglda8lpP+PJltjCA4DHJAJw=="],
|
||||
"opencode-linux-arm64-musl": ["[email protected]9.5", "", { "os": "linux", "cpu": "arm64", "libc": "musl" }, "sha512-YxvpVPJ/jeevV93DVRiuOVbhkoJAuwQUadPDHPaw65aZkAKZ6OE7XySwCS+Rjlhzt7fPryisvAOjUtZkwfMBuQ=="],
|
||||
|
||||
"opencode-linux-x64": ["[email protected]8.4", "", { "os": "linux", "cpu": "x64" }, "sha512-mCiZuKQBqRHVhtp20YFDfNf5cUwTt6/I98MFJQDJdDTWggnkAVOnitQTmSt2cQwoPJ94VrRwDhZyY3pIY3+CQg=="],
|
||||
"opencode-linux-x64": ["[email protected]9.5", "", { "os": "linux", "cpu": "x64" }, "sha512-i1qh7VooLGML9xX6ud93y0JA8j/1aPeDYRKjsmYXrJX1TjIWHgBBMkEbEJPZthpWbsJg//XfBgR6Kb7+NPaJiQ=="],
|
||||
|
||||
"opencode-linux-x64-baseline": ["[email protected]8.4", "", { "os": "linux", "cpu": "x64" }, "sha512-LlVIv54gpM8I6QJVAoW5uvcaVO4z+y5DswfUCsxgaD0CWgjEbULaiAAZIS/L+N5rRw/jHeqcHSXiKinFZt0xww=="],
|
||||
"opencode-linux-x64-baseline": ["[email protected]9.5", "", { "os": "linux", "cpu": "x64" }, "sha512-9kA+st3XXtCXMPx5f3Ld1u19uoHTO2u2Btt8a2cqhJ4GIqVbD1mXLevj3LHFEmtuQoFGk+UstGnl38sCqXj5sQ=="],
|
||||
|
||||
"opencode-linux-x64-baseline-musl": ["[email protected]8.4", "", { "os": "linux", "cpu": "x64" }, "sha512-hM/uPMEuxLbwXueOmHsf3jjmfJ238/3r6FZMPumKVMR7jJHutp3ZWuOIL+ZzByxzsUK1f3RGbKaFULOMIieRSw=="],
|
||||
"opencode-linux-x64-baseline-musl": ["[email protected]9.5", "", { "os": "linux", "cpu": "x64", "libc": "musl" }, "sha512-kwt6hmTaI6nujjAzdAkXv3j2rDzXNOMfMk8Jf8bwacHtwUl8CWA2psDVg3nWyb4zByAa6F1jSIlmHCPHBq1K/Q=="],
|
||||
|
||||
"opencode-linux-x64-musl": ["[email protected]8.4", "", { "os": "linux", "cpu": "x64" }, "sha512-AlojHLyv7Cgn2g5Rj5QeHckEWyA20qdbEla2d1R9mXSaqlP6sX7izNti8Tzr/vMG53Uwmppyjb37uFB2fK+YTQ=="],
|
||||
"opencode-linux-x64-musl": ["[email protected]9.5", "", { "os": "linux", "cpu": "x64", "libc": "musl" }, "sha512-y6nUbH3ViajN37KaVVy0/KnWv/i48Bx5QPv9Ul9ONJya1ossV7CHVb+ZNFc61M5GqlVtjCvZV7T0eBMqhJG7mg=="],
|
||||
|
||||
"opencode-windows-arm64": ["[email protected]8.4", "", { "os": "win32", "cpu": "arm64" }, "sha512-B6m7N7ZPj/E6xx1ZaSjNW0gHTd0b3NTSf9mQiN3W89zji3oH9VOHLQdrP10rAd/2uZ5ZttxSkRqj/iwgAW+3TQ=="],
|
||||
"opencode-windows-arm64": ["[email protected]9.5", "", { "os": "win32", "cpu": "arm64" }, "sha512-aySF/MDji5KO3I5yl9Ec4g8kW/QIx4Jj5FXwBhh/FrDu0w6L8HAOGH7cyjFb3t8TW7pxRkH3Ei3rfkWAE7cScw=="],
|
||||
|
||||
"opencode-windows-x64": ["[email protected]8.4", "", { "os": "win32", "cpu": "x64" }, "sha512-TumOcMOvZ6vfs348LgGLK9eFEBLsh+wI/UXmQ8BhXW95YV1Wd26TutSzQi4L/wLftsDfl8Q6bKQ2trWtGBzBvQ=="],
|
||||
"opencode-windows-x64": ["[email protected]9.5", "", { "os": "win32", "cpu": "x64" }, "sha512-xCVbrA8A/qQTAdBIKyhK5tx3/mcq18yF82Mi8phry0MJb2631nnnpxF9m6FFfWAVHyer4qFGrO7hbWXesHQ1Gg=="],
|
||||
|
||||
"opencode-windows-x64-baseline": ["[email protected]8.4", "", { "os": "win32", "cpu": "x64" }, "sha512-4EpZ97uI50vVv9UT5sDeK3ff+sFzqmbBT4TNGkMF9uwNTKJYqoSVuOIZhIfLTLNlrEK2hd2/sf/4GQhIkM6iTw=="],
|
||||
"opencode-windows-x64-baseline": ["[email protected]9.5", "", { "os": "win32", "cpu": "x64" }, "sha512-yRHN+TsfLjfLmdQVAVWqTGfGP1yq8YtUFy9Jw33XhlPIp77slBdCJrj2PC4amuEl6BJtaIQDhDFbpJ8+a5z7Qw=="],
|
||||
|
||||
"oxfmt": ["[email protected]", "", { "dependencies": { "tinypool": "2.1.0" }, "optionalDependencies": { "@oxfmt/binding-android-arm-eabi": "0.60.0", "@oxfmt/binding-android-arm64": "0.60.0", "@oxfmt/binding-darwin-arm64": "0.60.0", "@oxfmt/binding-darwin-x64": "0.60.0", "@oxfmt/binding-freebsd-x64": "0.60.0", "@oxfmt/binding-linux-arm-gnueabihf": "0.60.0", "@oxfmt/binding-linux-arm-musleabihf": "0.60.0", "@oxfmt/binding-linux-arm64-gnu": "0.60.0", "@oxfmt/binding-linux-arm64-musl": "0.60.0", "@oxfmt/binding-linux-ppc64-gnu": "0.60.0", "@oxfmt/binding-linux-riscv64-gnu": "0.60.0", "@oxfmt/binding-linux-riscv64-musl": "0.60.0", "@oxfmt/binding-linux-s390x-gnu": "0.60.0", "@oxfmt/binding-linux-x64-gnu": "0.60.0", "@oxfmt/binding-linux-x64-musl": "0.60.0", "@oxfmt/binding-openharmony-arm64": "0.60.0", "@oxfmt/binding-win32-arm64-msvc": "0.60.0", "@oxfmt/binding-win32-ia32-msvc": "0.60.0", "@oxfmt/binding-win32-x64-msvc": "0.60.0" }, "peerDependencies": { "svelte": "^5.0.0", "vite-plus": "*" }, "optionalPeers": ["svelte", "vite-plus"], "bin": { "oxfmt": "bin/oxfmt" } }, "sha512-fViX6i+gJuZWY+jI/fnR6WRbRj70GZ9RlCd30MygJrHTUNc4DxvKHWw8vBjMjffv3PgU5qWDR0AzmojQByqaZA=="],
|
||||
|
||||
|
||||
@@ -63,7 +63,7 @@ The reporter maintains exactly one PR comment, plus optional inline threads (see
|
||||
|
||||
**Author + marker, never marker alone.** The comment marker is a hardcoded public literal, readable in the base-branch config, so anyone who can comment on the PR (including the untrusted PR author) could post a comment carrying the marker plus a forged embedded review state. A newest-marker-wins lookup would adopt that state and carry its `dismissed` list forward, silently suppressing real findings. So a comment counts as ours only if it carries the marker AND was authored by `ownLogin` — GitHub sets a comment's author from the authenticated identity and it cannot be spoofed [observed] ([github.ts:51-75](../src/reporters/github.ts); commit 93aaf30: "identify our own comment by author + marker, not the public marker alone, so a forged comment can't carry review state forward"; regression tests `src/__tests__/github-reporter.test.ts:25-34`). Any rewrite that reintroduces marker-only matching reopens this forged-dismissal-state attack.
|
||||
|
||||
**Fail closed on unresolvable identity.** `resolveOwnLogin` tries `gh api user --jq .login` first (works for a user/PAT token), and falls back to the literal `github-actions[bot]` only under GitHub Actions, because the default `GITHUB_TOKEN` is an installation token for which `/user` returns 403 [observed] ([github.ts:91-117](../src/reporters/github.ts)). When neither resolves, `ownLogin` is `null` and `selectOwnComments` treats NOTHING as ours [observed] ([github.ts:69-71, 114](../src/reporters/github.ts): `if (!ownLogin) return []`; test `github-reporter.test.ts:46-50`). It does not degrade to marker-only matching; it refuses to claim anything.
|
||||
**Fail closed on unresolvable identity.** `resolveOwnLogin` tries `gh api user --jq .login` first (works for a user/PAT token). An installation token, the default `GITHUB_TOKEN` or a GitHub App's, gets 403 there, so it next asks GraphQL for `viewer { login __typename }`, which GitHub documents for installation tokens; `viewerRestLogin` accepts only a Bot, since only an installation token gets that far, and adds the `[bot]` suffix REST uses for comment authors and event actors when GraphQL leaves it off [not verified live]. Only when both fail does it fall back to the literal `github-actions[bot]`, and only under GitHub Actions ([github.ts](../src/reporters/github.ts) `resolveOwnLogin`, `viewerRestLogin`). When neither resolves, `ownLogin` is `null` and `selectOwnComments` treats NOTHING as ours [observed] ([github.ts:69-71, 114](../src/reporters/github.ts): `if (!ownLogin) return []`; test `github-reporter.test.ts:46-50`). It does not degrade to marker-only matching; it refuses to claim anything.
|
||||
|
||||
**Full-tag marker matching.** `<!-- tag -->` is checked as a whole tag, not as a substring, so a root marker and a scoped marker `<!-- tag:scope -->` cannot cross-match — this is the "reviewdog #1911 lesson", a known bug class in other PR-comment tools where prefix-matching markers collide across scopes [observed] ([github.ts:180-185](../src/reporters/github.ts), citing the lesson verbatim). Deletion is scoped strictly to author-verified comments: `clear()` and the upsert's duplicate cleanup only ever delete comments returned by `selectOwnComments`, never a look-alike carrying the same marker from someone else [observed] ([github.ts:186-192, 351-366](../src/reporters/github.ts)).
|
||||
|
||||
@@ -81,11 +81,11 @@ The reporter maintains exactly one PR comment, plus optional inline threads (see
|
||||
|
||||
**Links fail soft.** `linkContextAsync` can be prebuilt and injected once so a CI fan-out builds one diff-lines + base-SHA context for all scopes instead of one pair of `gh` calls per scope — the reporter-side mirror of `memoizeSource` [observed] ([github.ts:33-37, 200-205](../src/reporters/github.ts)). Both underlying fetches fail soft: a missing diff index degrades in-diff findings to plain-text `file:line`, a missing base SHA degrades out-of-diff findings the same way — never a dead link, never a hard failure of the whole report [observed] ([github.ts:194-235](../src/reporters/github.ts); commits f9fecd5, 9446b42). This follows the repo rule that `ecr ci` must never fail a PR's checks on reviewer error [observed] (AGENTS.md: "`ecr ci` must never fail a PR's checks — reviewer errors degrade gracefully").
|
||||
|
||||
**The ready label follows the decision, and the bot removes only its own.** With `review.readyForHumanLabel` set, `report` and `reportAggregate` call `syncReadyLabel` after the comment is posted, and `postSkipNote` and `clear` call it with "not ready". `postSkipNote` is `ecr ci`'s break-glass path only: `ecr post-review` and `ecr review --pr --post` post nothing on break-glass, and a PR the trigger policy skips returns before any reporter call, so those leave the comment and the label as they are. Nothing else touches the label, so a label added before a push stays until the next review posts. Label failures only log: the comment is already up [observed] ([github.ts](../src/reporters/github.ts) `syncReadyLabel`, `readyForHuman`, `lastLabelEvent`; tests `src/__tests__/ready-label.test.ts`).
|
||||
**The ready label follows the decision, and the bot removes only its own.** With `review.readyForHumanLabel` set, `report` and `reportAggregate` call `syncReadyLabel` after the comment is posted, and `postSkipNote` and `clear` call it with "not ready". `postSkipNote` is `ecr ci`'s break-glass path only: `ecr post-review` and `ecr review --pr --post` post nothing on break-glass, and a PR the trigger policy skips returns before any reporter call, so those leave the comment and the label as they are. `ecr ci` also calls `removeReadyLabel` before it reviews new commits (a `pull_request` `synchronize` event, `isNewCommitsEvent`), so the label never describes a head no review has seen; the review adds it back if the new head is ready. With a personal or App token the removal is itself an `unlabeled` event that starts another run, which cancels this one under the scaffolded workflow's `cancel-in-progress` and reviews the same head; that run is not a `synchronize`, so it does not remove the label again. Label failures only log: the comment is already up [observed] ([github.ts](../src/reporters/github.ts) `syncReadyLabel`, `readyForHuman`, `lastLabelEvent`; tests `src/__tests__/ready-label.test.ts`).
|
||||
|
||||
**Ready means the run reviewed the whole PR.** `readyForHuman` is true when every review completed with full coverage (`incomplete` is empty), at least one reviewer applied, and the worst decision is `approve` or `approve_with_comments`; `reportAggregate` also requires no unmatched files. A review where no agent's `paths` matches approves with every `agentSummaries` row `notRun`, so that is what "no reviewer applied" checks; `incomplete` stays empty there, and a noise-only review has no rows at all. A run that reviewed part of the PR sets the reporter's `partialReview`, and then `syncReadyLabel` treats it as not ready: an explicit `--agents` subset in `ecr ci` and `ecr review --pr --post`, a `--scopes` run, whose other scopes come from the comment's stored state without a check against the head, and a deferred review saved with `--agents`, which records `partialReview` in the artifact for `ecr post-review` to pass on. A bare `/review` lets the router pick agents and is a full run. Without `--scopes` every active scope gets a result, failures included, so no other check for missing scopes is needed ([ci.ts](../src/commands/ci.ts) `runLegacyCi`, `runRoutedCi`; [deferred-review.ts](../src/core/deferred-review.ts)).
|
||||
|
||||
**Ownership comes from the label's own history.** The newest `labeled` or `unlabeled` event for the label (`lastLabelEvent`; the events API lists the oldest first) decides what is ours: a review that is not ready removes the label only when the newest event is our account's `labeled`, and a ready review does not add it again when the newest event is another account's `unlabeled`. No extra state is stored in the comment. The walk stops at the page cap; when it stops on a full page, a newer event may lie beyond it, so `syncReadyLabel` changes nothing and logs. "Our account" is `resolveOwnLogin`: `gh api user`, or `github-actions[bot]` for the default `GITHUB_TOKEN`. The check is by login, not by person: a personal token treats that person's own changes as ours, under the default token other workflows' label changes are `github-actions[bot]` too and count as ours, and a local `ecr review --pr --post` and CI run as different logins, so neither removes the other's label. A GitHub App token cannot read `/user`, so under Actions `resolveOwnLogin` falls back to `github-actions[bot]`, while the App's actor is expected to be `<app>[bot]` [not verified]: such a setup never removes the label it added, nor recognises its own comment.
|
||||
**Ownership comes from the label's own history.** The newest `labeled` or `unlabeled` event for the label (`lastLabelEvent`; the events API lists the oldest first) decides what is ours: a review that is not ready removes the label only when the newest event is our account's `labeled`, and a ready review does not add it again when the newest event is another account's `unlabeled`. No extra state is stored in the comment. The walk stops at the page cap; when it stops on a full page, a newer event may lie beyond it, so `syncReadyLabel` changes nothing and logs. "Our account" is `resolveOwnLogin` (see [GitHub reporter identity](#github-reporter-identity)). The check is by login, not by person: a personal token treats that person's own changes as ours, under the default token other workflows' label changes are `github-actions[bot]` too and count as ours, and a local `ecr review --pr --post` and CI run as different logins, so neither removes the other's label.
|
||||
|
||||
**Only the single comment labels, and the label is advisory.** In per-scope mode there is no one decision for the PR, `ecr ci` logs that it sets no label, and the root-tag `clear` of a mode switch removes ours (`runRoutedCi` in [ci.ts](../src/commands/ci.ts)). `/dismiss` re-renders with the stored decision, so it does not change the label. Label names match case-insensitively, as GitHub treats them, and the config check rejects a label that is, in any case, also `review.label`, `review.skipLabel` or a `<label>:<agent>` trigger. The decision comes from model output on untrusted PR content (see [LLP 0001](./0001-trust-model.principles.md)), so the label is advisory, never a merge gate.
|
||||
|
||||
|
||||
+2
-2
@@ -44,10 +44,10 @@
|
||||
},
|
||||
"dependencies": {
|
||||
"@modelcontextprotocol/sdk": "1.30.0",
|
||||
"@opencode-ai/sdk": "1.18.4",
|
||||
"@opencode-ai/sdk": "1.19.5",
|
||||
"cheerio": "1.1.2",
|
||||
"minisearch": "7.2.0",
|
||||
"opencode-ai": "1.18.4",
|
||||
"opencode-ai": "1.19.5",
|
||||
"zod": "^4.4.3"
|
||||
},
|
||||
"devDependencies": {
|
||||
|
||||
@@ -1,6 +1,9 @@
|
||||
import { test, expect } from "bun:test";
|
||||
import { mkdtemp, rm, writeFile } from "node:fs/promises";
|
||||
import { tmpdir } from "node:os";
|
||||
import path from "node:path";
|
||||
|
||||
import { shouldBypassTriggerGate, shouldReview } from "../commands/ci.js";
|
||||
import { isNewCommitsEvent, shouldBypassTriggerGate, shouldReview } from "../commands/ci.js";
|
||||
|
||||
const policy = { trigger: "all" as const, label: "ai-review", skipLabel: "ai-review:skip" };
|
||||
const labelMode = { ...policy, trigger: "label" as const };
|
||||
@@ -49,3 +52,32 @@ test("a pull_request event without --force does NOT bypass the trigger gate", ()
|
||||
// an unrelated flag does not bypass
|
||||
expect(shouldBypassTriggerGate(["--route"], { GITHUB_EVENT_NAME: "pull_request" })).toBe(false);
|
||||
});
|
||||
|
||||
test("isNewCommitsEvent: only a pull_request synchronize is new commits", async () => {
|
||||
const dir = await mkdtemp(path.join(tmpdir(), "ecr-event-"));
|
||||
try {
|
||||
const eventFor = async (action: string): Promise<string> => {
|
||||
const file = path.join(dir, `${action}.json`);
|
||||
await writeFile(file, JSON.stringify({ action, number: 1 }), "utf8");
|
||||
return file;
|
||||
};
|
||||
const env = async (name: string, action: string) => ({
|
||||
GITHUB_EVENT_NAME: name,
|
||||
GITHUB_EVENT_PATH: await eventFor(action),
|
||||
});
|
||||
expect(await isNewCommitsEvent(await env("pull_request", "synchronize"))).toBe(true);
|
||||
expect(await isNewCommitsEvent(await env("pull_request_target", "synchronize"))).toBe(true);
|
||||
expect(await isNewCommitsEvent(await env("pull_request", "opened"))).toBe(false);
|
||||
expect(await isNewCommitsEvent(await env("pull_request", "labeled"))).toBe(false);
|
||||
expect(await isNewCommitsEvent(await env("issue_comment", "created"))).toBe(false);
|
||||
expect(await isNewCommitsEvent({ GITHUB_EVENT_NAME: "pull_request" })).toBe(false);
|
||||
expect(
|
||||
await isNewCommitsEvent({
|
||||
GITHUB_EVENT_NAME: "pull_request",
|
||||
GITHUB_EVENT_PATH: path.join(dir, "missing.json"),
|
||||
}),
|
||||
).toBe(false);
|
||||
} finally {
|
||||
await rm(dir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
@@ -6,7 +6,7 @@ import path, { delimiter } from "node:path";
|
||||
import { resetTrustedToolCache } from "../core/exec.js";
|
||||
import { ReviewConfigSchema } from "../config/schema.js";
|
||||
import type { CoordinatorOutput } from "../core/schema.js";
|
||||
import { GitHubReporter, readyForHuman } from "../reporters/github.js";
|
||||
import { GitHubReporter, readyForHuman, viewerRestLogin } from "../reporters/github.js";
|
||||
|
||||
const TAG = "expo-ai-code-reviewer";
|
||||
const LABEL = "ready for human";
|
||||
@@ -85,7 +85,8 @@ test("config: review.readyForHumanLabel is optional and never empty", () => {
|
||||
|
||||
// A fake `gh` that logs each call. The PR's labels come from labels.json, and page N
|
||||
// of its issue events from events-N.json. FAKE_GH_LABEL_ERROR makes every label call
|
||||
// fail, and FAKE_GH_USER_ERROR makes `gh api user` fail.
|
||||
// fail, FAKE_GH_USER_ERROR makes `gh api user` fail, and FAKE_GH_VIEWER is what the
|
||||
// GraphQL `viewer` query returns (it fails when unset).
|
||||
const FAKE_GH = `#!/bin/sh
|
||||
printf '%s\\n' "$*" >> "$FAKE_GH_DIR/calls.log"
|
||||
case "$*" in
|
||||
@@ -101,6 +102,9 @@ case "$*" in
|
||||
"api user"*)
|
||||
if [ -n "$FAKE_GH_USER_ERROR" ]; then echo "HTTP 403" >&2; exit 1; fi
|
||||
printf 'ecr-bot\\n' ;;
|
||||
"api graphql"*)
|
||||
if [ -z "$FAKE_GH_VIEWER" ]; then echo "HTTP 401" >&2; exit 1; fi
|
||||
printf '%s' "$FAKE_GH_VIEWER" ;;
|
||||
*"-X GET "*"/comments"*) printf '[]' ;;
|
||||
*"-X POST "*"/comments --input "*) printf '{"id": 1}' ;;
|
||||
*) echo "unexpected gh call: $*" >&2; exit 1 ;;
|
||||
@@ -120,6 +124,7 @@ beforeEach(async () => {
|
||||
"FAKE_GH_DIR",
|
||||
"FAKE_GH_LABEL_ERROR",
|
||||
"FAKE_GH_USER_ERROR",
|
||||
"FAKE_GH_VIEWER",
|
||||
"GITHUB_ACTIONS",
|
||||
]) {
|
||||
saved[name] = process.env[name];
|
||||
@@ -128,6 +133,7 @@ beforeEach(async () => {
|
||||
process.env.FAKE_GH_DIR = dir;
|
||||
delete process.env.FAKE_GH_LABEL_ERROR;
|
||||
delete process.env.FAKE_GH_USER_ERROR;
|
||||
delete process.env.FAKE_GH_VIEWER;
|
||||
delete process.env.GITHUB_ACTIONS;
|
||||
resetTrustedToolCache();
|
||||
});
|
||||
@@ -343,6 +349,32 @@ test("report: with no known own login, the label is never removed", async () =>
|
||||
expect(labelCalls(await calls())).toEqual([]);
|
||||
});
|
||||
|
||||
test("report: a GitHub App token is known by its GraphQL viewer", async () => {
|
||||
// An installation token can't read `/user`; under Actions the fallback would guess
|
||||
// github-actions[bot], which is not this App.
|
||||
process.env.FAKE_GH_USER_ERROR = "1";
|
||||
process.env.GITHUB_ACTIONS = "true";
|
||||
process.env.FAKE_GH_VIEWER = JSON.stringify({ login: "review-app", __typename: "Bot" });
|
||||
await fixture({ labels: [LABEL], events: [[{ event: "labeled", actor: "review-app[bot]" }]] });
|
||||
await reporterFor(960, { ownLogin: null }).report(review({ decision: "request_changes" }));
|
||||
expect(labelCalls(await calls())).toEqual([
|
||||
"api -X DELETE repos/o/r/issues/960/labels/ready%20for%20human",
|
||||
]);
|
||||
});
|
||||
|
||||
test("report: under Actions without a viewer, the login falls back to github-actions[bot]", async () => {
|
||||
process.env.FAKE_GH_USER_ERROR = "1";
|
||||
process.env.GITHUB_ACTIONS = "true";
|
||||
await fixture({
|
||||
labels: [LABEL],
|
||||
events: [[{ event: "labeled", actor: "github-actions[bot]" }]],
|
||||
});
|
||||
await reporterFor(961, { ownLogin: null }).report(review({ decision: "request_changes" }));
|
||||
expect(labelCalls(await calls())).toEqual([
|
||||
"api -X DELETE repos/o/r/issues/961/labels/ready%20for%20human",
|
||||
]);
|
||||
});
|
||||
|
||||
test("report: a label spelled in another case on the PR is the same label", async () => {
|
||||
await fixture({
|
||||
labels: ["Ready For Human"],
|
||||
@@ -438,3 +470,25 @@ test("postSkipNote: a skipped review removes the bot's label", async () => {
|
||||
"api -X DELETE repos/o/r/issues/913/labels/ready%20for%20human",
|
||||
]);
|
||||
});
|
||||
|
||||
test("viewerRestLogin: names a bot the way REST does, and only a bot", () => {
|
||||
expect(viewerRestLogin({ login: "review-app", __typename: "Bot" })).toBe("review-app[bot]");
|
||||
expect(viewerRestLogin({ login: "review-app[bot]", __typename: "Bot" })).toBe("review-app[bot]");
|
||||
// Only an installation token asks `viewer`, so anything but a Bot is not trusted.
|
||||
expect(viewerRestLogin({ login: "octocat", __typename: "User" })).toBeNull();
|
||||
expect(viewerRestLogin({ login: "review-app" })).toBeNull();
|
||||
expect(viewerRestLogin({ login: "" })).toBeNull();
|
||||
expect(viewerRestLogin(null)).toBeNull();
|
||||
});
|
||||
|
||||
test("removeReadyLabel: removes the bot's label and leaves a person's", async () => {
|
||||
await fixture({ labels: [LABEL], events: [[{ event: "labeled", actor: "ecr-bot" }]] });
|
||||
await reporterFor(962).removeReadyLabel();
|
||||
expect(labelCalls(await calls())).toEqual([
|
||||
"api -X DELETE repos/o/r/issues/962/labels/ready%20for%20human",
|
||||
]);
|
||||
await writeFile(path.join(dir, "calls.log"), "", "utf8");
|
||||
await fixture({ labels: [LABEL], events: [[{ event: "labeled", actor: "octocat" }]] });
|
||||
await reporterFor(963).removeReadyLabel();
|
||||
expect(labelCalls(await calls())).toEqual([]);
|
||||
});
|
||||
@@ -64,6 +64,24 @@ async function resolvePrNumber(): Promise<number | null> {
|
||||
return match ? Number(match[1]) : null;
|
||||
}
|
||||
|
||||
/**
|
||||
* Whether the Actions event is new commits on the PR (`pull_request` or
|
||||
* `pull_request_target` with action `synchronize`). False when there is no event.
|
||||
*/
|
||||
export async function isNewCommitsEvent(env: NodeJS.ProcessEnv = process.env): Promise<boolean> {
|
||||
if (!env.GITHUB_EVENT_NAME?.startsWith("pull_request") || !env.GITHUB_EVENT_PATH) {
|
||||
return false;
|
||||
}
|
||||
try {
|
||||
const event = JSON.parse(await readFile(env.GITHUB_EVENT_PATH, "utf8")) as {
|
||||
action?: string;
|
||||
};
|
||||
return event.action === "synchronize";
|
||||
} catch {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
const CI_USAGE = `ecr ci — review the current GitHub PR and post/update one comment.
|
||||
|
||||
For GitHub Actions: reads the PR number + repo from the event/env, gets the diff
|
||||
@@ -581,6 +599,11 @@ async function runLegacyCi(
|
||||
`CI reviewer: break-glass check failed (continuing): ${errorMessage(error)}\n`,
|
||||
);
|
||||
}
|
||||
// The ready label described the previous head; the review adds it back if this one
|
||||
// is ready.
|
||||
if (await isNewCommitsEvent()) {
|
||||
await reporter.removeReadyLabel();
|
||||
}
|
||||
|
||||
const stack = resolveStackWalk(config.stack, noStackAware);
|
||||
const stackConfirm = resolveStackConfirm(config.stack, noStackAware);
|
||||
@@ -849,6 +872,11 @@ async function runRoutedCi(
|
||||
`CI reviewer: break-glass check failed (continuing): ${errorMessage(error)}\n`,
|
||||
);
|
||||
}
|
||||
// The ready label described the previous head; the review adds it back if this one
|
||||
// is ready.
|
||||
if (await isNewCommitsEvent()) {
|
||||
await bgReporter.removeReadyLabel();
|
||||
}
|
||||
|
||||
// Build ONE link context for all scopes (rate-limit hygiene): diff lines from the
|
||||
// already-fetched changed files, base OID from the memoized PR metadata (the same
|
||||
|
||||
+42
-5
@@ -640,6 +640,21 @@ export function selectOwnComments(
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* The REST login for a GraphQL `viewer` that is a bot, or null. Only an installation
|
||||
* token reaches the `viewer` query, and its identity is a Bot; anything else falls
|
||||
* back to the default GITHUB_TOKEN's login. REST names a bot `<slug>[bot]` (that is
|
||||
* what comment authors and label event actors carry), and GraphQL may leave the
|
||||
* suffix off. Exported for tests.
|
||||
*/
|
||||
export function viewerRestLogin(viewer: unknown): string | null {
|
||||
const { login, __typename: type } = (viewer ?? {}) as { login?: unknown; __typename?: unknown };
|
||||
if (type !== "Bot" || typeof login !== "string" || login === "") {
|
||||
return null;
|
||||
}
|
||||
return login.endsWith("[bot]") ? login : `${login}[bot]`;
|
||||
}
|
||||
|
||||
/**
|
||||
* Maintains exactly one PR comment, updating it in place across re-reviews (and
|
||||
* cleaning up duplicates) so the review converges instead of churning. Runs the
|
||||
@@ -658,9 +673,10 @@ export class GitHubReporter implements Reporter {
|
||||
/**
|
||||
* The login of the account this reporter comments as, so its own comment is
|
||||
* recognized by AUTHOR (see selectOwnComments for why the body marker is not enough).
|
||||
* Resolution: `gh api user` (a user/PAT token), else the scaffolded workflow's default
|
||||
* GITHUB_TOKEN identity, `github-actions[bot]`, when running under Actions — an
|
||||
* installation token can't read `/user`. Null when neither is available, which makes
|
||||
* Resolution: `gh api user` (a user/PAT token); else GraphQL `viewer`, which also
|
||||
* answers for an installation token (the default GITHUB_TOKEN or a GitHub App's),
|
||||
* since those can't read `/user`; else, under Actions, the default GITHUB_TOKEN
|
||||
* identity, `github-actions[bot]`. Null when none is available, which makes
|
||||
* selectOwnComments treat no comment as ours (fail closed). Memoized: the identity is
|
||||
* stable for the process, and every reporter method consults it.
|
||||
*/
|
||||
@@ -669,8 +685,8 @@ export class GitHubReporter implements Reporter {
|
||||
return Promise.resolve(this.options.ownLogin);
|
||||
}
|
||||
this.ownLoginResolution ??= (async () => {
|
||||
const gh = await resolveTrustedTool("gh");
|
||||
try {
|
||||
const gh = await resolveTrustedTool("gh");
|
||||
const { stdout } = await run(gh, ["api", "user", "--jq", ".login"], {
|
||||
cwd: this.options.cwd,
|
||||
});
|
||||
@@ -679,7 +695,20 @@ export class GitHubReporter implements Reporter {
|
||||
return login;
|
||||
}
|
||||
} catch {
|
||||
// The default GITHUB_TOKEN is an installation token: `/user` returns 403.
|
||||
// An installation token can't read `/user` (403).
|
||||
}
|
||||
try {
|
||||
const { stdout } = await run(
|
||||
gh,
|
||||
["api", "graphql", "-f", "query={ viewer { login __typename } }", "--jq", ".data.viewer"],
|
||||
{ cwd: this.options.cwd },
|
||||
);
|
||||
const login = viewerRestLogin(JSON.parse(stdout));
|
||||
if (login) {
|
||||
return login;
|
||||
}
|
||||
} catch {
|
||||
// Fall back to the default GITHUB_TOKEN's identity below.
|
||||
}
|
||||
return process.env.GITHUB_ACTIONS ? "github-actions[bot]" : null;
|
||||
})();
|
||||
@@ -2070,6 +2099,14 @@ export class GitHubReporter implements Reporter {
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Remove our ready label before reviewing new commits, so it never says "ready" for
|
||||
* a head no review has seen; the review's report adds it back when it is ready.
|
||||
*/
|
||||
async removeReadyLabel(): Promise<void> {
|
||||
await this.syncReadyLabel(false);
|
||||
}
|
||||
|
||||
// @ref LLP 0008#comment-lifecycle [implements] — the ready label follows the decision; only our own label is removed
|
||||
/**
|
||||
* Add `readyForHumanLabel` when `ready` (and the run is not partial), or remove it
|
||||
|
||||
Reference in new issue
Block a user