fix: know a GitHub App token's login, and drop the ready label when new commits arrive

README.md

@@ -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

llp/0008-sources-and-reporters.explainer.md

@@ -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.
 

src/__tests__/ci-trigger.test.ts

@@ -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 });
+  }
+});

src/__tests__/ready-label.test.ts

@@ -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([]);
+});