feat(review-pr): add rate-anomaly safeguard for review requests

[Sayt-0]

Summary

Adds a rate-anomaly safeguard against abuse of review requests, layered on the
existing per-PR cache lock.

• src/rate-limit: counts docker-agent review/reply comments posted on a PR
  within a sliding window and, past a threshold, flags a rate anomaly. The
  workflow then skips the expensive review. Fails open, so an API error never
  blocks a legitimate review.
• Per-PR concurrency: groups on review-pr.yml and self-review-pr-trigger.yml
  collapse same-trigger bursts; the trigger cancels superseded context-capture
  runs so only the latest fans out to a review.

Scope after review

Following review feedback, the PR was narrowed:

• audit-log removed, deferred to a follow-up. GitHub Actions artifacts are a
  weak audit store (configurable/deletable retention, no query/export); the
  storage backend should be designed separately.
• check-staleness removed. Reviews already check out refs/pull/N/head
  (current head), so a commit pushed after the request is already included; the
  force-push case is therefore covered. Incremental review (only commits since
  the last review) is the real future want and is tracked separately.
• check-org-membership unchanged by this PR. The merge of main superseded
  the earlier author-verification commit with the evaluateMembership rewrite
  from feat(review-pr): authorize review_requested via the requester for external contributors #16/docs: explain org-member review flow for external contributor PRs #18, so the merged diff makes no change to that module.

Validation

Check	Result
build (tsup)	pass
typecheck (tsc)	pass
biome ci	pass (89 files)
actionlint	pass
unit tests	597 pass

Follow-ups (out of scope here)

• Design a queryable audit-log storage backend.
• Incremental review: review only commits pushed since the last review.
• Retire the deprecated /review comment branch.
• Sanitize @mention comments (like the diff) instead of gating on org membership.

[docker-agent]

Assessment: 🟢 APPROVE

The PR introduces three new modules (src/audit-log, src/rate-limit, src/check-staleness) and wires them into the review workflow with a clear security-first design. The changes are well-structured and the logic holds up under scrutiny.

What was reviewed:

• src/audit-log/index.ts — JSONL artifact, job summary, audit record emission: no data-loss paths in the CLI call chain
• src/rate-limit/index.ts — comment counting with since/updated_at vs. created_at: the explicit client-side created_at filter correctly compensates for GitHub's updated_at-based since parameter; no under-count scenario is possible since updated_at ≥ created_at always holds on GitHub
• src/check-staleness/index.ts — SHA comparison and stale-post logic: correct
• src/check-org-membership/index.ts — resolveUsername fallback on non-event paths: the PR-author fallback on trigger/input paths is intentional design; both paths are gated by trusted callers who control the PR number, so there is no bypass
• .github/workflows/review-pr.yml — rate check ordering, concurrency groups, needs: dependencies: correctly ordered; rate check precedes check creation as documented
• The fail-open behavior for the rate-limit step and the audit decision logging are consistent with the documented design

No confirmed or likely bugs were found in the changed code.

[derekmisler]

i'm sorry, i forgot to hit "submit" yesterday. some of these comments may be out-of-date now, though

[derekmisler]

This actually already exists, but doesn't use GH's concurrency, and only 1 review-per-PR can run within a window of time: https://github.com/docker/docker-agent-action/blob/main/review-pr/action.yml#L142-L146

[Sayt-0]

Correct that a per-PR collapse already exists via the cache lock. The new concurrency: group is additive rather than equivalent: it is keyed per (PR + trigger intent), queues instead of skipping, applies before a runner spins up, and has no race window (the lock has a documented one). The trigger workflow also uses cancel-in-progress to drop superseded context-capture runs. If relying on the lock plus the rate-limit check is preferred, the group can be dropped; it is left in place for now as a cheap, race-free pre-resource layer.

[derekmisler]

makes sense, thanks for the explanation!

[derekmisler]

i'm not totally sure if this is an actual problem, because i think we want the reviewer to review the whole PR. realistically, this change would lead to this user experience:

• user requests review
• review starts
• user pushes new commit
• review runs against everything before that commit
• reviewer posts stale review
• user re-requests review to include the last commit

so we're kind of forcing a user to run extra reviews. i think it's fine if the user pushed a commit after triggering a review, and the reviewer includes it in the reviewed diff

[derekmisler]

eventually, what's more important, is that we're running reviews only against commits that were pushed after the last review, so we're no re-reviewing the same diff over and over again.

[Sayt-0]

Removed in 7f5e3c3. Reviews check out refs/pull/N/head, so a commit pushed after the request is already part of the reviewed diff; the stale-pre-push concern did not apply, and the module only emitted a notice plus one audit field. The incremental-review goal (review only commits pushed since the last review) is separate work and is tracked as a follow-up.

[derekmisler]

that's not totally true, there's a window of time where concurrent reviews are prevented. we can increase that window of time easily enough (people found it annoying, though).

[derekmisler]

this is an interesting idea! i don't think storing them in GH is the right approach, though (i can't remember if artifacts have 90-day or 7-day time limits). this feels like something we'd want to store somewhere we can easily query. we should exclude this idea from this PR and talk about it more in depth for a later PR.

[Sayt-0]

Removed from this PR in 7f5e3c3 and deferred to a follow-up, so the storage backend (a queryable store rather than Actions artifacts) can be designed separately. The src/audit-log module, its two workflow steps, the tsup entry, and the SECURITY.md / README sections are gone. Nothing in the merge-gating path depended on it.

[derekmisler]

we should talk through this one some more, because i think i'm missing something. we just want to allow docker org members to request reviews from docker-agent, regardless of the PR author, right? so we don't actually have to check docker org membership status of the PR author (probably ever; we might be able to remove the current implementation where we prevent reviews completely if the PR author is not a member of the docker org).

auto-reviews already can't run unless the author isn't a member of the docker org (i think security already prevents all CI checks, including this one, from running for outside contributors).

also, since /review is deprecated, i don't think we need to support that specific comment flow anymore. what i think is more important is sanitizing comments like we do with the diff, so don't have to check org membership status at all and the agent can reply to anyone

[Sayt-0]

This PR makes no change to src/check-org-membership: the merge of main superseded the earlier author-verification commit with the evaluateMembership rewrite from #16 / #18, so the merged diff does not touch this file. The points raised are good follow-ups, tracked separately: retire the deprecated /review branch, and sanitize @mention comments instead of gating replies on org membership. One nuance for that discussion: on the automatic pull_request path the PR-author org check is currently the sole gate (the native "only triage/write can request a reviewer" control only covers the review_requested path), so dropping it warrants confirming the auto-review threat model first.

[Sayt-0]

Follow-up on the review. Scope narrowed and the branch brought up to date with main (clean merge, no conflicts).

Changes in response to the review:

• Removed src/audit-log and src/check-staleness (modules, tests, workflow steps, tsup entries, and the SECURITY.md / README sections). Both deferred to follow-ups.
• Kept the rate-anomaly safeguard (the rate-limit module plus the per-PR concurrency: groups).
• src/check-org-membership is unchanged by this PR (superseded by main's evaluateMembership rewrite from feat(review-pr): authorize review_requested via the requester for external contributors #16 / docs: explain org-member review flow for external contributor PRs #18).

[docker-agent]

Assessment: 🟡 NEEDS ATTENTION

The rate-anomaly safeguard is well-structured overall: the sliding-window logic is correct, the fail-open behavior is properly realized, and the workflow correctly consumes the anomalous output. Two issues found in the new code:

1. [MEDIUM] src/rate-limit/index.ts — Full PR Reviews (/pulls/{pr}/reviews endpoint) are not counted; only issue comments and inline diff-thread comments are queried. Since the bot posts reviews via the Reviews API, the rate counter stays at 0 regardless of review frequency.
2. [LOW] .github/workflows/review-pr.yml — The rate check is gated on is_review != 'false', leaving the reply-to-mention job (a separate job) with no throttle. May be intentional scope deferral, but worth an explicit comment if so.

[docker-agent]

[MEDIUM] detectRateAnomaly() skips /pulls/{pr}/reviews — full PR reviews are not counted toward the rate limit

The new detectRateAnomaly() function queries two endpoints:

• issues.listComments — issue-level (top-level PR) comments
• pulls.listReviewComments — inline diff-thread comments

It does not query pulls.listReviews (/pulls/{pr}/reviews), which is the endpoint for full PR Review objects — the kind that carry the <!-- docker-agent-review --> marker in their body field. This is how the docker-agent-action bot posts its code reviews (a single Review object with inline comments attached).

As a result, every full review the bot posts goes uncounted. An org member can request 20 reviews in a 10-minute window, the rate counter stays at 0, and the anomaly threshold is never reached.

Fix: add a third paginated fetch for PR Reviews and merge the results before counting:

const prReviews: CommentLike[] = await octokit.paginate(octokit.rest.pulls.listReviews, {
  owner: opts.owner,
  repo: opts.repo,
  pull_number: opts.prNumber,
  per_page: 100,
});

const count = [...issueComments, ...reviewComments, ...prReviews].filter((c) =>
  isAgentReviewComment(c, opts.botLogin, windowStartMs),
).length;

Note: Octokit's CommentLike interface already fits the shape of Review objects (they have user, body, and submitted_at). You may need to remap submitted_at → created_at or extend CommentLike to accept either field.

[docker-agent]

[LOW] Check rate anomaly is scoped to review requests only — @docker-agent mention floods are unthrottled

The Check rate anomaly step has:

if: |
  steps.membership.outputs.is_member == 'true' &&
  steps.command.outputs.is_review != 'false' &&
  steps.draft.outputs.skip != 'true'

When an @docker-agent mention arrives (not a /review command), steps.command.outputs.is_review is 'false', so the rate check is skipped. The reply-to-mention job (line ~785) is a separate job with no rate-limit integration at all.

This means an org member can flood @docker-agent mentions on a PR and drive up LLM inference costs with no throttle. The PR title scopes itself to "review requests", so this may be an intentional deferral — if so, a comment in the workflow noting the known gap would be helpful. If unintentional, the reply-to-mention job should also consult dist/rate-limit.js with its own window/threshold (likely a looser one, since mention replies are cheaper than full reviews).

[docker-agent]

Assessment: 🟢 APPROVE

The rate-anomaly safeguard is well-designed and correctly implements a fail-open safety model. The core sliding-window logic in detectRateAnomaly is sound, the bot-identity matching covers both docker-agent and docker-agent[bot] token variants, pagination is used on all three API endpoints, and the <!-- docker-agent-review --> marker is correctly excluded from REPLY_MARKERS to avoid double-counting inline finding comments as replies.

Fail-open correctness confirmed: All error paths — missing token, invalid repo slug, and uncaught exceptions — call core.setOutput('anomalous', 'false') before returning, ensuring API failures never block a legitimate review. The workflow's steps.rate.outputs.anomalous != 'true' gate is correctly a negative check so an empty/unset output also fails open.

Minor observation (no action required): The two early-return guards in main() (lines ~195–205 of src/rate-limit/index.ts) set anomalous and count but omit window and threshold outputs. The workflow only reads anomalous, so there is no functional impact — these outputs would just appear empty in the Actions log on those rarely-hit paths. Worth a follow-up if richer diagnostics are desired.

Concurrency groups: The per-PR concurrency group on review-pr.yml correctly serialises same-PR bursts. The acknowledged github.run_id fallback for the workflow_run/fork path is an acceptable known limitation.

No bugs introduced by this PR. Ship it.

[derekmisler]

solid feature overall. the fail-open design, marker-based counting, and the distinction between review runs and reply runs are all the right calls. two real issues in the exported detectRateAnomaly (missing input guards that main() handles but the library export doesn't) plus a handful of non-blocking suggestions, mostly around the listReviews pagination and test coverage for main().

[derekmisler]

suggestion (non-blocking): these three octokit.paginate calls are awaited sequentially, so on a busy PR you're waiting for each one to finish before starting the next. since they're independent, Promise.all gets them running in parallel and cuts wall-clock time by ~2/3:

const [issueComments, reviewComments, reviews] = await Promise.all([
  octokit.paginate(octokit.rest.issues.listComments, { owner, repo, issue_number: prNumber, since, per_page: 100 }),
  octokit.paginate(octokit.rest.pulls.listReviewComments, { owner, repo, pull_number: prNumber, since, per_page: 100 }),
  octokit.paginate(octokit.rest.pulls.listReviews, { owner, repo, pull_number: prNumber, per_page: 100 }),
]);

not blocking given continue-on-error: true, but worth picking up.

[derekmisler]

suggestion (non-blocking): listComments and listReviewComments accept since so they're server-side filtered to the window. listReviews has no since parameter (you noted this in the comment), so octokit.paginate fetches every review ever posted on the PR. on a long-lived, heavily-reviewed PR that could be hundreds of pages of API calls, which risks exhausting the job timer and falling through to the fail-open path, i.e. the rate safeguard silently disappears on the exact PRs most likely to need it.

a cleaner approach: use octokit.paginate.iterator and break once you've passed the window boundary. GitHub returns reviews oldest-first, so once you see items older than windowStartMs you can stop (or just cap at a reasonable max like 500 reviews before bailing out):

const reviews: ReviewLike[] = [];
for await (const page of octokit.paginate.iterator(octokit.rest.pulls.listReviews, {
  owner: opts.owner, repo: opts.repo, pull_number: opts.prNumber, per_page: 100,
})) {
  reviews.push(...page.data);
  // all items on this page are older than the window: no point fetching more
  if (page.data.every(r => !r.submitted_at || Date.parse(r.submitted_at) < windowStartMs)) break;
}

(double-check the sort order in the GitHub docs before assuming direction, but it is oldest-first)

[derekmisler]

issue: detectRateAnomaly is an exported function, so it can be called as a library without going through main(). the CLI path is safe because parsePositiveInt rejects NaN, 0, and negatives. but the exported function has no guard, so if opts.windowSeconds is NaN, Infinity, or a negative number:

windowStartMs = now - NaN * 1000  // NaN
new Date(NaN).toISOString()       // throws: RangeError: Invalid time value

that propagates to the caller uncaught. a simple guard at the top of detectRateAnomaly covers it:

if (!Number.isFinite(opts.windowSeconds) || opts.windowSeconds <= 0) {
  throw new RangeError(`windowSeconds must be a positive finite number, got ${opts.windowSeconds}`);
}
if (opts.threshold <= 0) {
  throw new RangeError(`threshold must be a positive integer, got ${opts.threshold}`);
}

the threshold <= 0 case is also worth guarding here (see the comment on the anomalous line below): count >= 0 is always true, so threshold: 0 permanently throttles everything.

[derekmisler]

issue: count >= opts.threshold, when threshold is 0, 0 >= 0 is true, so every call is flagged anomalous and all reviews are blocked. main() protects against this via parsePositiveInt (which rejects 0 and falls back to 8), but any direct caller of detectRateAnomaly with threshold: 0 (misconfiguration, bad test setup, etc.) will silently permanently throttle the bot.

the guard I mentioned on line 127 covers this too, so it's one small addition in one place.

[derekmisler]

suggestion (non-blocking): the jsdoc at the top of the file documents four outputs: anomalous, count, window, threshold. but all three early-exit / fail-open paths (here, the invalid-repo check ~10 lines down, and the catch block) only emit anomalous and count. window and threshold are left as empty strings.

today's callers only read anomalous so nothing breaks. but it's inconsistent with your own spec, and anyone who tries to read steps.rate.outputs.threshold in a debug step or a future consumer will get an empty string and be confused.

easy fix: set all four in every exit path. you have windowSeconds and threshold already parsed from env vars at this point, so it's just:

core.setOutput('window', String(windowSeconds));
core.setOutput('threshold', String(threshold));

[derekmisler]

suggestion (non-blocking): the coverage of detectRateAnomaly here is really thorough, good work on that. one gap though: main() (the actual CLI entry point) has zero test coverage. that means all of this is untested:

• env-var name correctness (RATE_MAX_REQUESTS vs RATE_THRESHOLD, etc.)
• parsePositiveInt fallback behavior
• all three setOutput paths (success / missing-token / invalid-repo / api-error)
• the core.warning / core.info log messages
• the early-return fail-open conditions

a mistake in any of these ships silently. vi.stubEnv makes it pretty painless:

describe('main()', () => {
  it('fails open and emits anomalous=false when token is missing', async () => {
    vi.stubEnv('GITHUB_TOKEN', '');
    vi.stubEnv('GH_TOKEN', '');
    vi.stubEnv('RATE_PR_NUMBER', '5');
    // call main(), assert core.setOutput called with ('anomalous', 'false')
  });

  it('emits all four outputs on a successful run', async () => {
    vi.stubEnv('GITHUB_TOKEN', 'tok');
    vi.stubEnv('GITHUB_REPOSITORY', 'docker/repo');
    vi.stubEnv('RATE_PR_NUMBER', '5');
    // mock detectRateAnomaly, assert window + threshold outputs are set
  });
});

[derekmisler]

quick follow-up: CI caught two issues in the new commit.

unit test failure (emits all four outputs on a successful anomalous run): the test is hitting a fail-open path instead of the success path, outputting anomalous=false and count=0. the most likely cause is that vi.clearAllMocks() in beforeEach is clearing the mockImplementation set by routePaginate from a previous test run, or the CI runner environment has GITHUB_REPOSITORY set to docker/docker-agent-action (which is a valid owner/repo string, so slashIdx >= 0) and routePaginate is returning the wrong dataset for that repo path. worth running pnpm test locally with --reporter=verbose to see exactly which fail-open branch is firing. also try adding vi.resetAllMocks() in beforeEach (instead of clearAllMocks) to make sure mock implementations don't leak between tests.

lint error (src/rate-limit/index.ts:171): biome flags the non-null assertion (r as ReviewLike).submitted_at!. since the check on line 170 already guards !(r as ReviewLike).submitted_at, the ! on line 171 is redundant. replace it with ?? '' or restructure slightly:

const submittedAt = (r as ReviewLike).submitted_at;
!(submittedAt) || Date.parse(submittedAt) < windowStartMs,

lint error (src/rate-limit/__tests__/rate-limit.test.ts): biome wants the chained .rejects.toThrow(RangeError) calls split across multiple lines. run pnpm biome format --write src/rate-limit/__tests__/rate-limit.test.ts and it'll fix the formatting automatically.

[derekmisler]

nitpick (super non-blocking): if github.event.pull_request.number is somehow absent (low probability, but possible if the event shape changes), the group key collapses to pr-review-trigger--<comment.id>, which could let two different PRs land in the same group. a || github.run_id at the end is a cheap safety net:

group: pr-review-trigger-${{ github.event.pull_request.number || github.run_id }}-${{ github.event.comment.id || 'review-request' }}

[derekmisler]

all issues addressed and ci is green. approving!