diff --git a/docs/code/worker.md b/docs/code/worker.md index 2c534e8d..fab5302f 100644 --- a/docs/code/worker.md +++ b/docs/code/worker.md @@ -542,6 +542,8 @@ Pending, failing, and not-yet-reported CI is checked at the configured workspace Relay-backed workspaces perform these API calls with the local `GITHUB_TOKEN`. A fine-grained token—or the customer-owned App used by a no-relay worker—needs **Actions: Read** and **Commit statuses: Read** in addition to the normal PR and contents permissions. Existing App installations must be re-approved after adding permissions. No extra webhook event subscription is required because CI is polled. GitHub does not currently expose its separate Checks permission for fine-grained PATs, so check-run-only CI providers are not watched unless they also publish a commit status; GitHub Actions is fully supported through the Actions API. +A rejected or missing credential (HTTP 401, or no `GITHUB_TOKEN` at all) and a credential-scoped permission denial (HTTP 403 with a GitHub credential message such as `Resource not accessible`) share one throttle per affected repository and operation: the watcher logs at most one actionable warning per operation per hour instead of repeating the same failure on every poll. The warning names the PR and explains whether the credential must be renewed or the missing permission granted — for CI reads, Actions: Read and Commit statuses: Read; for the exhaustion comment, Issues: Write or Pull requests: Write. CI reads and comment posts are throttled separately so a CI warning cannot hide a write-permission failure. A credential failure while posting the exhaustion comment is retried on the next poll instead of silently dropping the escalation, and a non-credential failure (for example a locked issue or persistent 5xx) is retried a bounded number of times before the watcher stops trying for that head SHA. CI auto-fix resumes automatically once the credential can read CI again; the watcher logs a single recovery line per repository and operation, and only after the same endpoint that failed succeeds (a metadata-only PR fetch does not clear a CI or review-read throttle). In fleet mode each repository has its own token, so one repository's credential failure never hides another's in either the CI watcher or review polling. Other 403 responses — rate limits, archived read-only repositories, locked issues, and blocked repositories — are not credential failures and keep their normal per-tick error reporting and retry path. + ## Mention the bot on any PR The worker also reacts to mentions on pull requests it did not create. When a teammate writes a comment like `@devintern address the review feedback` on any PR in the repository, the worker picks it up on the next poll and handles it through the same pipeline. Detection is a repository-wide sweep of new comments (two requests per interval, regardless of how many PRs are open), so mentions work without any webhook setup. diff --git a/packages/code/src/lib/acquirers/ci-failure-watcher.ts b/packages/code/src/lib/acquirers/ci-failure-watcher.ts index 3cd3ae37..88a1704f 100644 --- a/packages/code/src/lib/acquirers/ci-failure-watcher.ts +++ b/packages/code/src/lib/acquirers/ci-failure-watcher.ts @@ -39,6 +39,7 @@ import { RowParseError } from "../state/row-parse"; import type { Acquirer } from "../../worker"; import type { CiFailureFeedback } from "../review/formatter"; import { serializePrRun } from "./review-polling"; +import { describeGitHubAuthFailure, isGitHubAuthError } from "../code-host/github/reviews"; import type { CiProvider, @@ -277,6 +278,36 @@ interface CachedCiSnapshot { type PollOutcome = "active" | "unchanged-green" | "removed"; +/** API surface a CI credential failure came from; throttled independently. */ +type CiAuthOperation = "poll" | "comment"; + +/** Result of one escalation-comment attempt. */ +type EscalationOutcome = "posted" | "retry" | "gave-up"; + +interface CiAuthFailureEntry { + repo: string; + operation: CiAuthOperation; + lastWarnAt: number | null; + active: boolean; +} + +/** What one PR poll actually exercised, so recovery is only claimed where tested. */ +interface CiPollProgress { + /** + * True once every configured CI stream was fetched for this PR. PR-state + * reads alone (fork PRs, a missing head SHA) are metadata-only and cannot + * prove the CI endpoints recovered. + */ + ciQueried: boolean; +} + +/** + * Non-credential escalation-comment failures tolerated before freezing the + * escalation for this head. A permanent 403/422/5xx on the comment endpoint + * must not be retried (and warned about) on every poll forever. + */ +const MAX_ESCALATION_POST_FAILURES = 3; + interface GreenPollSchedule { /** Index of the delay to use after the next unchanged-green observation. */ nextBackoffIndex: number; @@ -304,6 +335,26 @@ export class CiFailureWatcherAcquirer implements Acquirer { private timer: ReturnType | null = null; private busy = false; private greenPollSchedules = new Map(); + /** + * Per-repo/per-operation throttle state for permanent credential failures. + * Fleet mode resolves a distinct token per repo, so a 403 in one repo must + * not hide an unrelated 401 in another; a comment-post failure also must not + * be silenced by a CI-read warning (they need different permissions). + */ + private authFailures = new Map(); + /** + * Repo operations whose credential failed during the current tick. Credential + * errors can surface outside the per-PR fetch try/catch (for example posting + * the escalation comment), and such an operation must not be reported as + * recovered just because its PR state fetched successfully. + */ + private tickAuthFailedOps = new Set(); + /** + * Consecutive non-credential escalation-comment failures per PR, scoped to + * the head SHA that was being escalated, so a permanently failing post is + * retried a bounded number of times instead of on every poll forever. + */ + private escalationPostFailures = new Map(); constructor(options: CiFailureWatcherAcquirerOptions) { this.options = { @@ -358,13 +409,22 @@ export class CiFailureWatcherAcquirer implements Acquirer { this.busy = true; try { + this.tickAuthFailedOps.clear(); const watchedPrs = this.options.watchedChanges?.() ?? this.options.workerState.listOpenAgentPrs(); const watchedKeys = new Set(watchedPrs.map((pr) => this.prKey(pr.repo, pr.prNumber))); + const watchedRepos = new Set(watchedPrs.map((pr) => pr.repo)); for (const key of this.greenPollSchedules.keys()) { if (!watchedKeys.has(key)) this.greenPollSchedules.delete(key); } + for (const [key, entry] of this.authFailures) { + if (!watchedRepos.has(entry.repo)) this.authFailures.delete(key); + } + for (const key of this.escalationPostFailures.keys()) { + if (!watchedKeys.has(key)) this.escalationPostFailures.delete(key); + } + const ciSucceededRepos = new Set(); for (const pr of watchedPrs) { const key = this.prKey(pr.repo, pr.prNumber); const schedule = this.greenPollSchedules.get(key); @@ -372,8 +432,10 @@ export class CiFailureWatcherAcquirer implements Acquirer { continue; } + const progress: CiPollProgress = { ciQueried: false }; try { - const outcome = await this.pollPr(pr.repo, pr.prNumber); + const outcome = await this.pollPr(pr.repo, pr.prNumber, progress); + if (progress.ciQueried) ciSucceededRepos.add(pr.repo); if (outcome === "unchanged-green") { this.scheduleGreenPoll(key, schedule?.nextBackoffIndex ?? 0); } else { @@ -383,16 +445,84 @@ export class CiFailureWatcherAcquirer implements Acquirer { } } catch (error) { this.greenPollSchedules.delete(key); + if (isGitHubAuthError(error)) { + // A rejected or under-scoped credential fails identically on every + // request, so re-reporting it per PR per tick floods the log. + this.warnAuthFailure(pr.repo, pr.prNumber, error as Error, "poll"); + continue; + } console.warn( `⚠️ [${this.name}] polling ${pr.repo}#${pr.prNumber} failed: ${(error as Error).message}`, ); } } + // Only a tick where every CI stream was fetched proves polling works + // again; a metadata-only success (fork PR, missing head SHA) must not + // clear a CI throttle it never re-tested. Credential failures raised + // outside the per-PR fetch (escalation comments) are tracked per + // operation and do not block CI polling's own recovery. + for (const repo of ciSucceededRepos) { + if (!this.tickAuthFailedOps.has(CiFailureWatcherAcquirer.authKey(repo, "poll"))) { + this.noteAuthRecovery(repo, "poll"); + } + } } finally { this.busy = false; } } + /** One credential warning per repo per operation per rolling hour. */ + private static AUTH_WARN_INTERVAL_MS = 60 * 60 * 1000; + + private static authKey(repo: string, operation: CiAuthOperation): string { + return `${repo.toLowerCase()}\u0000${operation}`; + } + + private warnAuthFailure( + repo: string, + prNumber: number, + error: Error, + operation: CiAuthOperation, + ): void { + const key = CiFailureWatcherAcquirer.authKey(repo, operation); + this.tickAuthFailedOps.add(key); + const entry = this.authFailures.get(key) ?? { + repo, + operation, + lastWarnAt: null, + active: false, + }; + entry.active = true; + const now = this.now(); + if ( + entry.lastWarnAt !== null && + now - entry.lastWarnAt < CiFailureWatcherAcquirer.AUTH_WARN_INTERVAL_MS + ) { + this.authFailures.set(key, entry); + return; + } + entry.lastWarnAt = now; + this.authFailures.set(key, entry); + const activity = operation === "comment" ? "posting an escalation comment on" : "polling"; + console.warn( + `⚠️ [${this.name}] GitHub rejected the credential while ${activity} ${repo}#${prNumber}: ` + + `${error.message} ${describeGitHubAuthFailure(error, operation === "comment" ? "comment" : "ci")} ` + + "See docs/code/worker.md#ci-failures-on-the-agents-prs.", + ); + } + + /** Announce one operation's recovery and rearm its throttle. */ + private noteAuthRecovery(repo: string, operation: CiAuthOperation): void { + const key = CiFailureWatcherAcquirer.authKey(repo, operation); + const entry = this.authFailures.get(key); + if (!entry?.active) return; + this.authFailures.delete(key); + const scope = operation === "comment" ? "escalation comment posting" : "CI polling"; + console.log( + `✅ [${this.name}] GitHub credential accepted again for ${repo}; ${scope} recovered`, + ); + } + /** Promptly reconcile one watched change after an authenticated provider hint. */ async reconcile(repo: string, prNumber: number): Promise { if (this.busy || !(this.options.enabled?.() ?? true)) return; @@ -401,7 +531,7 @@ export class CiFailureWatcherAcquirer implements Acquirer { this.busy = true; const key = this.prKey(repo, prNumber); try { - const outcome = await this.pollPr(repo, prNumber); + const outcome = await this.pollPr(repo, prNumber, { ciQueried: false }); if (outcome === "unchanged-green") { this.scheduleGreenPoll(key, 0); } else { @@ -484,14 +614,20 @@ export class CiFailureWatcherAcquirer implements Acquirer { ); state.consecutiveFailures = 0; state.escalatedSha = undefined; + this.escalationPostFailures.delete(this.prKey(repo, prNumber)); workerState.setCiFixState(repo, prNumber, state); } if (state.consecutiveFailures >= this.maxAttempts) { if (!state.escalatedSha && fresh.length > 0) { - state.escalatedSha = headSha; - workerState.setCiFixState(repo, prNumber, state); - await this.escalateToHuman(repo, prNumber, pending); + // Persist the escalation only once the comment landed (or retrying it + // is abandoned), so a transient credential/API failure is retried + // instead of permanently silencing the escalation for this head SHA. + const escalation = await this.escalateToHuman(repo, prNumber, pending, headSha); + if (escalation !== "retry") { + state.escalatedSha = headSha; + workerState.setCiFixState(repo, prNumber, state); + } } else if (verbose) { console.log( ` [${this.name}] ${this.describe(repo, prNumber)}: retry budget exhausted; waiting for human`, @@ -547,8 +683,10 @@ export class CiFailureWatcherAcquirer implements Acquirer { } } if (!ok && state.consecutiveFailures >= this.maxAttempts && !state.escalatedSha) { - state.escalatedSha = headSha; - await this.escalateToHuman(repo, prNumber, pending); + const escalation = await this.escalateToHuman(repo, prNumber, pending, headSha); + if (escalation !== "retry") { + state.escalatedSha = headSha; + } } workerState.setCiFixState(repo, prNumber, state); console.log( @@ -564,7 +702,11 @@ export class CiFailureWatcherAcquirer implements Acquirer { } /** Poll a single PR; triggers at most one fix attempt per poll. */ - private async pollPr(repo: string, prNumber: number): Promise { + private async pollPr( + repo: string, + prNumber: number, + progress: CiPollProgress, + ): Promise { const { workerState, provider, verbose } = this.options; // 1. PR state (ETag-cached): unwatch closed/merged PRs, track head SHA. @@ -612,6 +754,9 @@ export class CiFailureWatcherAcquirer implements Acquirer { headSha, prResult, ); + // Every configured stream was fetched: CI polling genuinely exercised the + // endpoints a credential failure would have hit. + progress.ciQueried = true; // 4. Split failures into fresh vs already-handled. Mark only after a // successful invocation so crashes/no-op runs remain retryable. @@ -699,12 +844,21 @@ export class CiFailureWatcherAcquirer implements Acquirer { } } - /** Post the give-up comment and freeze further attempts until head moves. */ + /** + * Post the give-up comment. Callers only freeze further attempts + * (`escalatedSha`) once the comment landed or retrying it is abandoned, so a + * transient failure is retried instead of permanently silencing the + * escalation. Credential failures use the per-repo/per-operation throttle and + * keep retrying (they recover once the token is fixed); non-credential + * failures are capped so a permanently rejected post cannot be retried and + * warned about on every poll forever. + */ private async escalateToHuman( repo: string, prNumber: number, failures: PendingFailure[], - ): Promise { + headSha: string, + ): Promise { const names = failures.map((f) => `- ${f.name}`).join("\n"); const body = "⚠️ I could not fix the following CI failure(s) automatically after " + @@ -712,17 +866,41 @@ export class CiFailureWatcherAcquirer implements Acquirer { "I have stopped retrying to avoid churn. " + (this.options.escalationRecoveryText ?? "Push a new commit (or mention me) and I will take another look."); + const key = this.prKey(repo, prNumber); try { await this.options.provider.postComment(repo, prNumber, body); + this.escalationPostFailures.delete(key); + this.noteAuthRecovery(repo, "comment"); console.log( `📣 [${this.name}] ${this.describe(repo, prNumber)}: posted escalation comment after ` + `${this.maxAttempts} failed attempt(s)`, ); + return "posted"; } catch (error) { + if (isGitHubAuthError(error)) { + // A credential failure is a configuration problem: keep retrying (the + // warning is throttled per repo per hour) so the escalation lands as + // soon as the token is fixed. + this.warnAuthFailure(repo, prNumber, error as Error, "comment"); + return "retry"; + } + const previous = this.escalationPostFailures.get(key); + const failedAttempts = previous?.headSha === headSha ? previous.failures + 1 : 1; + this.escalationPostFailures.set(key, { headSha, failures: failedAttempts }); + if (failedAttempts >= MAX_ESCALATION_POST_FAILURES) { + this.escalationPostFailures.delete(key); + console.warn( + `⚠️ [${this.name}] could not post escalation comment on ${this.describe(repo, prNumber)} ` + + `after ${failedAttempts} attempt(s): ${(error as Error).message}; ` + + "not retrying until the head changes", + ); + return "gave-up"; + } console.warn( `⚠️ [${this.name}] could not post escalation comment on ${this.describe(repo, prNumber)}: ` + - `${(error as Error).message}`, + `${(error as Error).message}; retrying on the next poll`, ); + return "retry"; } } diff --git a/packages/code/src/lib/acquirers/review-polling.ts b/packages/code/src/lib/acquirers/review-polling.ts index f37da565..3eb5798b 100644 --- a/packages/code/src/lib/acquirers/review-polling.ts +++ b/packages/code/src/lib/acquirers/review-polling.ts @@ -36,7 +36,7 @@ import { reconcileOpenAgentPrs, } from "../review/agent-pr-reconciler"; import type { ConditionalResult, PolledPr } from "../review/agent-pr-reconciler"; -import { isGitHubAuthError } from "../code-host/github/reviews"; +import { describeGitHubAuthFailure, isGitHubAuthError } from "../code-host/github/reviews"; import { nextScheduleOccurrence } from "../automation/config"; import type { CronOrIntervalSchedule } from "../automation/config"; import { parseEnvInteger } from "../config/env-integer"; @@ -84,6 +84,36 @@ export interface ReviewPollingGitHub { ): Promise; } +/** + * Endpoint group a polling pass can fail on. Recovery is tracked per stage so + * a successful metadata-only PR fetch cannot clear a reviews/comments throttle + * that was never re-tested. + */ +type PollStage = "pr-state" | "reviews" | "comments" | "write"; + +/** Human label for a stage, used in credential warnings. */ +const POLL_STAGE_LABELS: Record = { + "pr-state": "reading PR state", + reviews: "reading reviews", + comments: "reading review comments", + write: "posting a review comment", +}; + +interface AuthFailureEntry { + repo: string; + stage: PollStage; + lastWarnAt: number | null; + active: boolean; +} + +/** Stage progress for one PR pass; mutated by {@link ReviewPollingAcquirer.pollPr}. */ +interface PollProgress { + /** Stages whose requests completed during this pass. */ + completed: Set; + /** Stage whose request is currently in flight. */ + current?: PollStage; +} + export interface ReviewPollingAcquirerOptions { intervalSeconds: number; /** Whether feedback acquisition should run (state/base reconciliation always runs). */ @@ -584,8 +614,15 @@ export class ReviewPollingAcquirer implements Acquirer { /** Cached scheduled-window state; `undefined` = not loaded from the cursor yet. */ private conflictWindowState: ConflictWindowState | null | undefined; private lastFeedbackPollAt: number; - /** Last time a rejected-credential warning was printed (rate-limited). */ - private lastAuthWarnAt = 0; + /** + * Per-repo/per-stage throttle state for permanent credential failures. Fleet + * mode resolves a distinct token per repo, so a 403 in one repo must not + * hide an unrelated 401 in another, and a write failure must not be silenced + * by a read warning (or cleared by a successful read). + */ + private authFailures = new Map(); + /** Per-repo throttle for "404 may mean access lost" warnings. */ + private inaccessibleWarnAt = new Map(); constructor(options: ReviewPollingAcquirerOptions) { this.options = options; @@ -702,12 +739,36 @@ export class ReviewPollingAcquirer implements Acquirer { for (const key of this.prCache.keys()) { if (!watchedKeys.has(key)) this.clearPrCache(key); } + const watchedRepos = new Set(watchedPrs.map((pr) => pr.repo)); + for (const [key, entry] of this.authFailures) { + if (!watchedRepos.has(entry.repo)) this.authFailures.delete(key); + } + for (const repo of this.inaccessibleWarnAt.keys()) { + if (!watchedRepos.has(repo)) this.inaccessibleWarnAt.delete(repo); + } + /** Stages that completed in this tick, per repo (union over its PRs). */ + const succeededStages = new Map>(); + /** Stages that failed with a credential error in this tick, per repo. */ + const failedStages = new Map>(); + const recordStages = ( + target: Map>, + repo: string, + stages: Iterable, + ): void => { + let entry = target.get(repo); + if (!entry) { + entry = new Set(); + target.set(repo, entry); + } + for (const stage of stages) entry.add(stage); + }; for (const pr of watchedPrs) { // Defensive: rows registered mid-run for a foreign repo (or when // startup pruning could not run) are skipped, never acted on. if (allowedRepos && allowedRepos.length > 0 && !allowedRepos.includes(pr.repo)) { continue; } + const progress: PollProgress = { completed: new Set() }; try { await this.pollPr( pr.repo, @@ -715,13 +776,19 @@ export class ReviewPollingAcquirer implements Acquirer { pr.createdAt, pollFeedback, fresh.get(agentPrKey(pr.repo, pr.prNumber)), + progress, ); } catch (error) { + recordStages(succeededStages, pr.repo, progress.completed); if (isGitHubAuthError(error)) { - // A rejected credential fails identically on every request, so - // re-reporting it per PR per tick floods error tracking with - // duplicates. Warn the operator throttled and keep polling. - this.warnAuthFailure(error as Error); + // Credential failures (401/rejected, 403/under-scoped) are + // configuration problems that fail identically on every PR every + // tick. Warn the operator at most once per repo per stage per hour + // and intentionally skip captureError so a permanently rejected + // credential does not flood error tracking with duplicates. + const stage = progress.current ?? "write"; + recordStages(failedStages, pr.repo, [stage]); + this.warnAuthFailure(pr.repo, pr.prNumber, error as Error, stage); continue; } captureError(error, { @@ -733,6 +800,19 @@ export class ReviewPollingAcquirer implements Acquirer { console.warn( `⚠️ [${this.name}] polling ${pr.repo}#${pr.prNumber} failed: ${(error as Error).message}`, ); + continue; + } + recordStages(succeededStages, pr.repo, progress.completed); + } + // Only a tick where the same stage succeeded proves that stage's + // credential works again. A metadata-only PR fetch must not clear a + // reviews/comments throttle it never exercised, and one PR's failure + // must not mask another's success in the same repo. + for (const [repo, stages] of succeededStages) { + const failed = failedStages.get(repo); + for (const stage of stages) { + if (failed?.has(stage)) continue; + this.noteAuthRecovery(repo, stage); } } } finally { @@ -740,25 +820,61 @@ export class ReviewPollingAcquirer implements Acquirer { } } - /** One auth-related warning per hour, not one per PR per tick. */ + /** One credential warning per repo per stage per rolling hour. */ private static AUTH_WARN_INTERVAL_MS = 60 * 60 * 1000; - private warnAuthFailure(error: Error): void { + private static authKey(repo: string, stage: PollStage): string { + return `${repo.toLowerCase()}\u0000${stage}`; + } + + private warnAuthFailure(repo: string, prNumber: number, error: Error, stage: PollStage): void { + const key = ReviewPollingAcquirer.authKey(repo, stage); + const entry = this.authFailures.get(key) ?? { + repo, + stage, + lastWarnAt: null, + active: false, + }; + entry.active = true; const now = this.now(); - if (now - this.lastAuthWarnAt < ReviewPollingAcquirer.AUTH_WARN_INTERVAL_MS) { + if ( + entry.lastWarnAt !== null && + now - entry.lastWarnAt < ReviewPollingAcquirer.AUTH_WARN_INTERVAL_MS + ) { + this.authFailures.set(key, entry); return; } - this.lastAuthWarnAt = now; + entry.lastWarnAt = now; + this.authFailures.set(key, entry); console.warn( - `⚠️ [${this.name}] ${error.message} Renew GITHUB_TOKEN ` + - "(or reconnect the workspace's GitHub credentials); polling continues.", + `⚠️ [${this.name}] GitHub rejected the credential while ${POLL_STAGE_LABELS[stage]} ` + + `on ${repo}#${prNumber}: ` + + `${error.message} ${describeGitHubAuthFailure(error, stage === "write" ? "comment" : "reviews")}`, + ); + } + + /** Announce one stage's recovery and rearm that stage's throttle. */ + private noteAuthRecovery(repo: string, stage: PollStage): void { + const key = ReviewPollingAcquirer.authKey(repo, stage); + const entry = this.authFailures.get(key); + if (!entry?.active) return; + this.authFailures.delete(key); + console.log( + `✅ [${this.name}] GitHub credential accepted again for ${repo}; ` + + `${POLL_STAGE_LABELS[stage]} recovered`, ); } private warnPrInaccessible(repo: string, prNumber: number): void { const now = this.now(); - if (now - this.lastAuthWarnAt < ReviewPollingAcquirer.AUTH_WARN_INTERVAL_MS) return; - this.lastAuthWarnAt = now; + const lastWarnAt = this.inaccessibleWarnAt.get(repo); + if ( + lastWarnAt !== undefined && + now - lastWarnAt < ReviewPollingAcquirer.AUTH_WARN_INTERVAL_MS + ) { + return; + } + this.inaccessibleWarnAt.set(repo, now); console.warn( `⚠️ [${this.name}] GitHub returned 404 for ${repo}#${prNumber}. ` + "The PR may still be open but inaccessible to this worker's GitHub credential; " + @@ -772,13 +888,15 @@ export class ReviewPollingAcquirer implements Acquirer { prNumber: number, watchedSinceMs: number, pollFeedback: boolean, - prefetched?: ConditionalResult, - ): Promise { + prefetched: ConditionalResult | undefined, + progress: PollProgress, + ): Promise<"ok" | "inaccessible"> { const { workerState, github, addressPr, resolveConflicts } = this.options; // 1. PR state (ETag-cached): unwatch confirmed closed/merged PRs. When the // reconciliation pass already fetched this PR, its result is reused // (and already applied), so no second request is spent here. + progress.current = "pr-state"; const prSource = agentPrStateCursorSource(repo, prNumber); const prKey = agentPrKey(repo, prNumber); const prResult = @@ -790,14 +908,16 @@ export class ReviewPollingAcquirer implements Acquirer { )); if (prResult.gone) { this.warnPrInaccessible(repo, prNumber); - return; + // A 404 can mean lost access, so it cannot prove the credential works. + return "inaccessible"; } if (!prefetched) { const closure = applyAgentPrFetch(workerState, { repo, prNumber }, prResult); if (closure) { console.log(`👁️ [${this.name}] ${repo}#${prNumber} is ${closure.reason}; unwatching`); this.clearPrCache(prKey); - return; + progress.completed.add("pr-state"); + return "ok"; } } @@ -812,19 +932,31 @@ export class ReviewPollingAcquirer implements Acquirer { if (cachedPr) await this.maybeSyncBase(repo, prNumber, cachedPr); } - if (!pollFeedback) return; + // Reconciliation and base sync may spend additional PR-state requests; only + // reaching this point proves the whole pr-state stage succeeded. + progress.completed.add("pr-state"); + progress.current = undefined; + if (!pollFeedback) return "ok"; // 2. Reviews (ETag-cached): new human changes_requested reviews. // 3. Inline review comments since the last seen timestamp. // Both passes must run (each advances its own cursor), so no short-circuit. + progress.current = "reviews"; const reviewed = await this.collectActionableReviews(repo, prNumber); + progress.completed.add("reviews"); + progress.current = "comments"; const commented = await this.collectActionableComments(repo, prNumber, watchedSinceMs); + progress.completed.add("comments"); + progress.current = undefined; if (!reviewed && !commented) { - return; + return "ok"; } console.log(`\n📌 [${this.name}] new review feedback on ${repo}#${prNumber}`); + progress.current = "write"; const ok = await addressPr(repo, prNumber); + progress.completed.add("write"); + progress.current = undefined; if (ok === "deferred") { console.log( `⏳ [${this.name}] ${repo}#${prNumber} deferred; will retry when a harness is available`, @@ -838,6 +970,7 @@ export class ReviewPollingAcquirer implements Acquirer { : `⚠️ [${this.name}] ${repo}#${prNumber} feedback run did not complete cleanly`, ); } + return "ok"; } /** Mark unprocessed human `changes_requested` reviews as actionable. */ diff --git a/packages/code/src/lib/code-host/github/reviews.ts b/packages/code/src/lib/code-host/github/reviews.ts index 32ba6fce..4d27d29f 100644 --- a/packages/code/src/lib/code-host/github/reviews.ts +++ b/packages/code/src/lib/code-host/github/reviews.ts @@ -129,21 +129,122 @@ export function isGitHubNotFound(error: unknown): boolean { } /** - * Whether an error thrown by a GitHub API client call is an HTTP 401 - * (`Bad credentials`) or a missing-credential failure: the credential is - * expired, revoked, or rejected. Unlike 5xx/429 responses this is not - * transient — every request with the same credential fails identically, so - * long-running callers treat it as a configuration problem (renew - * `GITHUB_TOKEN`) instead of re-reporting it as a per-tick failure forever. + * Whether an error thrown by a GitHub API client call is a credential failure: + * an HTTP 401 (`Bad credentials`), a missing-credential failure, or a 403 + * permission denial with a known credential body (`Resource not accessible`, + * `Must have admin rights`). The credential is expired, revoked, rejected, or + * under-scoped. Unlike 5xx/429 responses this is not transient — every request + * with the same credential fails identically, so long-running callers treat it + * as a configuration problem (renew `GITHUB_TOKEN` or grant the missing + * permission) instead of re-reporting it as a per-tick failure forever. + * + * 404 remains false because callers cannot distinguish a deleted resource from + * one this credential cannot see. 403 responses without a credential-denial + * body also remain false: GitHub reuses 403 for rate limits, abuse detection, + * archived read-only repositories, locked issues, and blocked repositories. + * Those are either transient or genuine per-request failures and must keep + * their normal capture/retry handling. * * @param error - Error thrown by `apiRequest` / `conditionalGet` */ export function isGitHubAuthError(error: unknown): boolean { + return classifyGitHubAuthError(error) !== null; +} + +/** Credential failure classes that need different operator action. */ +export type GitHubAuthFailureKind = "invalid-credential" | "missing-permission"; + +/** + * 403 bodies that prove the credential itself lacks access rather than the + * resource being temporarily unavailable (rate limit, archived repo, locked + * issue, blocked repository). Matched case-insensitively against the API + * error detail. + */ +const GITHUB_CREDENTIAL_DENIAL_PATTERNS = [ + "resource not accessible", + "bad credentials", + "must have admin rights", +] as const; + +/** + * Classify a GitHub credential failure so callers can give correct guidance: + * a 401/missing token must be renewed, while a 403 with a credential-denial + * body may be a valid token that simply lacks a permission. Unknown 403s are + * not credential failures and keep the normal error path. + * + * @param error - Error thrown by `apiRequest` / `conditionalGet` + * @returns The failure class, or `null` when this is not a credential failure + */ +export function classifyGitHubAuthError(error: unknown): GitHubAuthFailureKind | null { const message = error instanceof Error ? error.message : String(error); - return ( + if ( message.includes("GitHub API error (401)") || message.includes("No GitHub authentication configured") + ) { + return "invalid-credential"; + } + if (!message.includes("GitHub API error (403)")) { + return null; + } + const lower = message.toLowerCase(); + const credentialDenied = GITHUB_CREDENTIAL_DENIAL_PATTERNS.some((pattern) => + lower.includes(pattern), ); + return credentialDenied ? "missing-permission" : null; +} + +/** + * API surface a credential failure came from. Read contexts need different + * permissions than the write context used for posting PR comments, so the + * guidance must not name permissions the failing request does not use. + */ +export type GitHubAuthFailureContext = "reviews" | "ci" | "comment"; + +/** Permission hint for each context, or "" when the read path has no extra scope. */ +function requiredPermissionHint(context: GitHubAuthFailureContext): string { + switch (context) { + case "ci": + return " (CI polling needs Actions: Read and Commit statuses: Read)"; + case "comment": + return " (posting pull-request comments needs Issues: Write or Pull requests: Write)"; + default: + return ""; + } +} + +/** + * Build actionable guidance for a credential failure. Non-credential errors + * yield an empty string. + * + * @param error - Error thrown by `apiRequest` / `conditionalGet` + * @param context - API surface that failed; selects the permission hint + * @returns Guidance sentence(s) for the operator + */ +export function describeGitHubAuthFailure( + error: unknown, + context: GitHubAuthFailureContext = "reviews", +): string { + const kind = classifyGitHubAuthError(error); + if (kind === "invalid-credential") { + return ( + "The GitHub credential is invalid, expired, or revoked. " + + "Renew GITHUB_TOKEN (or reconnect the workspace's GitHub credentials); " + + (context === "comment" + ? "the escalation retries and resumes automatically once the credential works." + : "polling continues and resumes automatically once the credential works.") + ); + } + if (kind === "missing-permission") { + return ( + "The GitHub token is valid but lacks a permission this request needs. " + + "Grant the missing permission" + + requiredPermissionHint(context) + + (context === "comment" + ? "; the escalation retries and lands automatically once the permission is granted." + : "; polling continues and recovers automatically once the permission is granted.") + ); + } + return ""; } /** Build an API error message; 401s get actionable credential guidance. */ @@ -152,8 +253,8 @@ function apiErrorMessage(status: number, detail: string): string { if (status === 401) { return ( `${base}. The GitHub credential was rejected as expired or revoked. ` + - "Set a valid GITHUB_TOKEN (or reconnect the workspace's GitHub credentials), " + - "then restart the worker." + "Set a valid GITHUB_TOKEN (or reconnect the workspace's GitHub credentials) " + + "to restore access." ); } return base; diff --git a/packages/code/tests/ci-failure-watcher-acquirer.test.ts b/packages/code/tests/ci-failure-watcher-acquirer.test.ts index bcba5d18..452dbd7e 100644 --- a/packages/code/tests/ci-failure-watcher-acquirer.test.ts +++ b/packages/code/tests/ci-failure-watcher-acquirer.test.ts @@ -100,6 +100,14 @@ describe("CiFailureWatcherAcquirer", () => { prCalls?: number; actionsCalls?: number; statusCalls?: number; + /** When set, fetchPr rejects with this error instead of returning data. */ + prError?: Error; + /** Per-repo fetchPr rejection (takes precedence over prError). */ + prErrorFor?: (repo: string, prNumber: number) => Error | undefined; + /** When set, fetchWorkflowRuns rejects with this error. */ + actionsError?: Error; + /** When set, fetchCommitStatus rejects with this error. */ + statusError?: Error; } function makeGithub(gh: FakeGitHubState): CiFailureWatcherGitHub { @@ -107,6 +115,8 @@ describe("CiFailureWatcherAcquirer", () => { async fetchPr(_repo, _n, etag): Promise> { gh.prCalls = (gh.prCalls ?? 0) + 1; gh.seenPrEtag = etag; + const error = gh.prErrorFor?.(_repo, _n) ?? gh.prError; + if (error) throw error; if (gh.prEtagHit) { return { data: null, etag, notModified: true }; } @@ -131,6 +141,7 @@ describe("CiFailureWatcherAcquirer", () => { ): Promise> { gh.actionsCalls = (gh.actionsCalls ?? 0) + 1; gh.seenActionsEtag = etag; + if (gh.actionsError) throw gh.actionsError; if (gh.actionsEtagHit) { return { data: null, etag, notModified: true }; } @@ -139,6 +150,7 @@ describe("CiFailureWatcherAcquirer", () => { async fetchCommitStatus(_repo, _sha, etag): Promise> { gh.statusCalls = (gh.statusCalls ?? 0) + 1; gh.seenStatusEtag = etag; + if (gh.statusError) throw gh.statusError; if (gh.statusEtagHit) { return { data: null, etag, notModified: true }; } @@ -162,6 +174,8 @@ describe("CiFailureWatcherAcquirer", () => { fixResults?: CiFixResult[]; enabled?: () => boolean; now?: () => number; + /** Failures consumed one per postComment call (success when empty). */ + postCommentErrors?: Array; } = {}, ) { const fixed: string[] = []; @@ -170,6 +184,8 @@ describe("CiFailureWatcherAcquirer", () => { const github: CiFailureWatcherGitHub = { ...base, postComment: async (_repo, _n, body) => { + const failure = overrides.postCommentErrors?.shift(); + if (failure) throw failure; comments.push(body); }, }; @@ -243,6 +259,292 @@ describe("CiFailureWatcherAcquirer", () => { expect(workerState.listOpenAgentPrs()).toHaveLength(1); }); + test("credential failures warn at most once per hour with actionable guidance", async () => { + workerState.recordAgentPr({ repo: "acme/widgets", prNumber: 42 }); + workerState.recordAgentPr({ repo: "acme/widgets", prNumber: 43 }); + let now = 1_000; + const gh: FakeGitHubState = { + prState: "open", + headSha: sha1, + workflowRuns: [], + prError: new Error( + "GitHub API error (403): Resource not accessible by personal access token", + ), + }; + const { acquirer } = makeAcquirer(gh, { now: () => now }); + const warns: string[] = []; + const originalWarn = console.warn; + console.warn = (...args: unknown[]) => { + warns.push(args.join(" ")); + }; + try { + await acquirer.tick(); + // Two failing PRs, but a permanent credential failure warns once. + expect(warns).toHaveLength(1); + expect(warns[0]).toContain("[poll:ci-failures]"); + expect(warns[0]).toContain("acme/widgets#42"); + expect(warns[0]).toContain("403"); + expect(warns[0]).toContain("Actions: Read"); + expect(warns[0]).toContain("Commit statuses: Read"); + expect(warns[0]).toContain("docs/code/worker.md"); + + now += 60_000; + await acquirer.tick(); + expect(warns).toHaveLength(1); + + now += 60 * 60_000; + await acquirer.tick(); + expect(warns).toHaveLength(2); + } finally { + console.warn = originalWarn; + } + }); + + test("rate-limit and server failures still warn per PR and per tick", async () => { + workerState.recordAgentPr({ repo: "acme/widgets", prNumber: 42 }); + workerState.recordAgentPr({ repo: "acme/widgets", prNumber: 43 }); + const gh: FakeGitHubState = { + prState: "open", + headSha: sha1, + workflowRuns: [], + prError: new Error("GitHub API error (403): API rate limit exceeded"), + }; + const { acquirer } = makeAcquirer(gh); + const warns: string[] = []; + const originalWarn = console.warn; + console.warn = (...args: unknown[]) => { + warns.push(args.join(" ")); + }; + try { + await acquirer.tick(); + expect(warns).toHaveLength(2); + expect(warns.every((line) => !line.includes("Actions: Read"))).toBe(true); + + gh.prError = new Error("GitHub API error (500): Internal Server Error"); + await acquirer.tick(); + expect(warns).toHaveLength(4); + expect(warns.slice(2).every((line) => !line.includes("Actions: Read"))).toBe(true); + } finally { + console.warn = originalWarn; + } + }); + + test("recovery logs once and rearms the credential warning throttle", async () => { + workerState.recordAgentPr({ repo: "acme/widgets", prNumber: 42 }); + let now = 1_000; + const gh: FakeGitHubState = { + prState: "open", + headSha: sha1, + workflowRuns: [], + prError: new Error( + "GitHub API error (403): Resource not accessible by personal access token", + ), + }; + const { acquirer } = makeAcquirer(gh, { now: () => now }); + const warns: string[] = []; + const logs: string[] = []; + const originalWarn = console.warn; + const originalLog = console.log; + console.warn = (...args: unknown[]) => { + warns.push(args.join(" ")); + }; + console.log = (...args: unknown[]) => { + logs.push(args.join(" ")); + }; + const recoveryLines = () => + logs.filter((line) => line.includes("GitHub credential accepted again")).length; + try { + await acquirer.tick(); + expect(warns).toHaveLength(1); + expect(recoveryLines()).toBe(0); + + gh.prError = undefined; + await acquirer.tick(); + expect(recoveryLines()).toBe(1); + + // Subsequent healthy ticks stay quiet. + await acquirer.tick(); + expect(recoveryLines()).toBe(1); + + // A new failure warns promptly rather than waiting out the old hour. + gh.prError = new Error( + "GitHub API error (403): Resource not accessible by personal access token", + ); + now += 60_000; + await acquirer.tick(); + expect(warns).toHaveLength(2); + expect(recoveryLines()).toBe(1); + } finally { + console.warn = originalWarn; + console.log = originalLog; + } + }); + + test("distinct repo credential failures each warn in the same tick", async () => { + workerState.recordAgentPr({ repo: "acme/a", prNumber: 1 }); + workerState.recordAgentPr({ repo: "acme/b", prNumber: 2 }); + const gh: FakeGitHubState = { + prState: "open", + headSha: sha1, + workflowRuns: [], + prErrorFor: (repo) => + repo === "acme/a" + ? new Error("GitHub API error (403): Resource not accessible by personal access token") + : new Error("GitHub API error (401): Bad credentials"), + }; + const { acquirer } = makeAcquirer(gh); + const warns: string[] = []; + const originalWarn = console.warn; + console.warn = (...args: unknown[]) => { + warns.push(args.join(" ")); + }; + try { + await acquirer.tick(); + // One repo's 403 must not suppress another repo's 401. + expect(warns).toHaveLength(2); + const permission = warns.find((line) => line.includes("acme/a#1"))!; + expect(permission).toContain("403"); + expect(permission).toContain("Actions: Read"); + expect(permission).toContain("Commit statuses: Read"); + expect(permission).not.toContain("expired or revoked"); + const invalid = warns.find((line) => line.includes("acme/b#2"))!; + expect(invalid).toContain("401"); + expect(invalid).toContain("invalid, expired, or revoked"); + expect(invalid).not.toContain("Actions: Read"); + } finally { + console.warn = originalWarn; + } + }); + + test("a mixed tick does not report recovery or rearm the throttle", async () => { + workerState.recordAgentPr({ repo: "acme/a", prNumber: 1 }); + workerState.recordAgentPr({ repo: "acme/b", prNumber: 2 }); + let now = 1_000; + const gh: FakeGitHubState = { + prState: "open", + headSha: sha1, + workflowRuns: [], + prErrorFor: (repo) => + repo === "acme/a" + ? new Error("GitHub API error (403): Resource not accessible by personal access token") + : undefined, + }; + const { acquirer } = makeAcquirer(gh, { now: () => now }); + const warns: string[] = []; + const logs: string[] = []; + const originalWarn = console.warn; + const originalLog = console.log; + console.warn = (...args: unknown[]) => { + warns.push(args.join(" ")); + }; + console.log = (...args: unknown[]) => { + logs.push(args.join(" ")); + }; + const recoveryLines = () => + logs.filter((line) => line.includes("GitHub credential accepted again")).length; + try { + await acquirer.tick(); + expect(warns).toHaveLength(1); + expect(recoveryLines()).toBe(0); + + // acme/b succeeds while acme/a still fails: that success must not rearm + // acme/a's warning throttle. + now += 60_000; + await acquirer.tick(); + expect(warns).toHaveLength(1); + expect(recoveryLines()).toBe(0); + } finally { + console.warn = originalWarn; + console.log = originalLog; + } + }); + + test("CI-stream permission failures share the one-per-repo hourly warning", async () => { + workerState.recordAgentPr({ repo: "acme/widgets", prNumber: 42 }); + workerState.recordAgentPr({ repo: "acme/widgets", prNumber: 43 }); + let now = 1_000; + const gh: FakeGitHubState = { + prState: "open", + headSha: sha1, + workflowRuns: [], + actionsError: new Error( + "GitHub API error (403): Resource not accessible by personal access token", + ), + }; + const { acquirer } = makeAcquirer(gh, { now: () => now }); + const warns: string[] = []; + const originalWarn = console.warn; + console.warn = (...args: unknown[]) => { + warns.push(args.join(" ")); + }; + try { + // fetchPr succeeds; the 403 comes from the Actions stream instead. + await acquirer.tick(); + expect(warns).toHaveLength(1); + expect(warns[0]).toContain("403"); + expect(warns[0]).toContain("Actions: Read"); + expect(warns[0]).toContain("Commit statuses: Read"); + + now += 60_000; + await acquirer.tick(); + expect(warns).toHaveLength(1); + + now += 60 * 60_000; + await acquirer.tick(); + expect(warns).toHaveLength(2); + } finally { + console.warn = originalWarn; + } + }); + + test("metadata-only success does not clear the CI-stream credential throttle", async () => { + workerState.recordAgentPr({ repo: "acme/widgets", prNumber: 42 }); + let now = 1_000; + const gh: FakeGitHubState = { + prState: "open", + headSha: sha1, + workflowRuns: [], + statusError: new Error( + "GitHub API error (403): Resource not accessible by personal access token", + ), + }; + const { acquirer } = makeAcquirer(gh, { now: () => now }); + const warns: string[] = []; + const logs: string[] = []; + const originalWarn = console.warn; + const originalLog = console.log; + console.warn = (...args: unknown[]) => { + warns.push(args.join(" ")); + }; + console.log = (...args: unknown[]) => { + logs.push(args.join(" ")); + }; + const recoveryLines = () => + logs.filter((line) => line.includes("GitHub credential accepted again")).length; + try { + await acquirer.tick(); + expect(warns).toHaveLength(1); + expect(warns[0]).toContain("Actions: Read"); + + // A fork PR returns right after the PR-state fetch without querying the + // CI streams, so it cannot prove those reads recovered. + gh.statusError = undefined; + gh.headRepoFullName = "someone/fork"; + now += 60_000; + await acquirer.tick(); + expect(recoveryLines()).toBe(0); + + // Real CI streams are queried again and succeed → recovery. + gh.headRepoFullName = undefined; + now += 60_000; + await acquirer.tick(); + expect(recoveryLines()).toBe(1); + } finally { + console.warn = originalWarn; + console.log = originalLog; + } + }); + test("relay reconciliation bypasses the timer for one registered change", async () => { workerState.recordAgentPr({ repo: "acme/widgets", prNumber: 42 }); const gh: FakeGitHubState = { @@ -610,6 +912,195 @@ describe("CiFailureWatcherAcquirer", () => { expect(workerState.getCiFixState("acme/widgets", 42).escalatedSha).toBe(sha1); }); + test("a failed escalation comment is retried instead of permanently suppressing it", async () => { + workerState.recordAgentPr({ repo: "acme/widgets", prNumber: 42 }); + const gh: FakeGitHubState = { + prState: "open", + headSha: sha1, + workflowRuns: [failingRun(850)], + jobLogs: "error: build failed", + }; + let now = 1_000; + const { acquirer, comments } = makeAcquirer(gh, { + maxAttempts: 1, + fixResults: [false], + now: () => now, + postCommentErrors: [ + new Error("GitHub API error (403): Resource not accessible by personal access token"), + ], + }); + const warns: string[] = []; + const logs: string[] = []; + const originalWarn = console.warn; + const originalLog = console.log; + console.warn = (...args: unknown[]) => { + warns.push(args.join(" ")); + }; + console.log = (...args: unknown[]) => { + logs.push(args.join(" ")); + }; + const recoveryLines = () => + logs.filter((line) => line.includes("GitHub credential accepted again")).length; + try { + await acquirer.tick(); + // The escalation comment did not land, so the PR must stay retryable. + expect(comments).toHaveLength(0); + expect(workerState.getCiFixState("acme/widgets", 42).escalatedSha).toBeUndefined(); + expect(warns).toHaveLength(1); + expect(warns[0]).toContain("403"); + // The failing request is a comment POST, so the guidance must name the + // write permission rather than the CI read scopes (which already work). + expect(warns[0]).toContain("Issues: Write"); + expect(warns[0]).not.toContain("Actions: Read"); + // A successful PR fetch must not claim credential recovery while the + // post itself was rejected. + expect(recoveryLines()).toBe(0); + + now += 60_000; + await acquirer.tick(); + expect(comments).toHaveLength(1); + expect(workerState.getCiFixState("acme/widgets", 42).escalatedSha).toBe(sha1); + expect(recoveryLines()).toBe(1); + } finally { + console.warn = originalWarn; + console.log = originalLog; + } + }); + + test("a CI-read warning does not hide a comment-post permission failure", async () => { + workerState.recordAgentPr({ repo: "acme/widgets", prNumber: 42 }); + const gh: FakeGitHubState = { + prState: "open", + headSha: sha1, + workflowRuns: [failingRun(860)], + jobLogs: "error: build failed", + actionsError: new Error( + "GitHub API error (403): Resource not accessible by personal access token", + ), + }; + const { acquirer, comments } = makeAcquirer(gh, { + maxAttempts: 1, + fixResults: [false], + postCommentErrors: [ + new Error("GitHub API error (403): Resource not accessible by personal access token"), + ], + }); + const warns: string[] = []; + const originalWarn = console.warn; + console.warn = (...args: unknown[]) => { + warns.push(args.join(" ")); + }; + try { + // First tick: the CI read is rejected → CI guidance. + await acquirer.tick(); + expect(warns).toHaveLength(1); + expect(warns[0]).toContain("Actions: Read"); + + // Same hour, now the CI read works but the escalation comment POST is + // rejected: its own throttle slot must still warn with write guidance. + gh.actionsError = undefined; + await acquirer.tick(); + expect(warns).toHaveLength(2); + expect(warns[1]).toContain("posting an escalation comment"); + expect(warns[1]).toContain("Issues: Write"); + expect(warns[1]).not.toContain("Actions: Read"); + expect(comments).toHaveLength(0); + } finally { + console.warn = originalWarn; + } + }); + + test("non-credential escalation post failures are capped instead of retried forever", async () => { + workerState.recordAgentPr({ repo: "acme/widgets", prNumber: 42 }); + const gh: FakeGitHubState = { + prState: "open", + headSha: sha1, + workflowRuns: [failingRun(861)], + jobLogs: "error: build failed", + }; + const errors = Array.from( + { length: 10 }, + () => new Error("GitHub API error (422): Validation Failed"), + ); + const postCommentErrors = errors.slice(); + const { acquirer, comments } = makeAcquirer(gh, { + maxAttempts: 1, + fixResults: [false], + postCommentErrors, + }); + const warns: string[] = []; + const originalWarn = console.warn; + console.warn = (...args: unknown[]) => { + warns.push(args.join(" ")); + }; + const postWarns = () => + warns.filter((line) => line.includes("could not post escalation comment")); + try { + for (let i = 0; i < 6; i++) await acquirer.tick(); + expect(comments).toHaveLength(0); + // Three attempts, then the escalation freezes until the head changes. + expect(postWarns()).toHaveLength(3); + expect(postWarns()[2]).toContain("after 3 attempt(s)"); + expect(postWarns()[2]).toContain("not retrying until the head changes"); + expect(workerState.getCiFixState("acme/widgets", 42).escalatedSha).toBe(sha1); + // No further POSTs are attempted at this head. + expect(postCommentErrors).toHaveLength(7); + } finally { + console.warn = originalWarn; + } + }); + + test("an escalation comment retried after a restart persists the escalation once it lands", async () => { + workerState.recordAgentPr({ repo: "acme/widgets", prNumber: 42 }); + workerState.setCiFixState("acme/widgets", 42, { consecutiveFailures: 1 }); + const gh: FakeGitHubState = { + prState: "open", + headSha: sha1, + workflowRuns: [failingRun(851)], + }; + const { acquirer, fixed, comments } = makeAcquirer(gh, { + maxAttempts: 1, + postCommentErrors: [new Error("GitHub API error (500): Internal Server Error")], + }); + + // Budget was already exhausted before this tick: no repair is attempted, + // and a failed post must leave the escalation unrecorded. + await acquirer.tick(); + expect(fixed).toHaveLength(0); + expect(comments).toHaveLength(0); + expect(workerState.getCiFixState("acme/widgets", 42).escalatedSha).toBeUndefined(); + + await acquirer.tick(); + expect(comments).toHaveLength(1); + expect(workerState.getCiFixState("acme/widgets", 42).escalatedSha).toBe(sha1); + }); + + test("non-credential 403s keep the per-PR capture path", async () => { + workerState.recordAgentPr({ repo: "acme/widgets", prNumber: 42 }); + const gh: FakeGitHubState = { + prState: "open", + headSha: sha1, + workflowRuns: [], + prError: new Error("GitHub API error (403): Repository was archived so is read-only."), + }; + const { acquirer } = makeAcquirer(gh); + const warns: string[] = []; + const originalWarn = console.warn; + console.warn = (...args: unknown[]) => { + warns.push(args.join(" ")); + }; + try { + await acquirer.tick(); + await acquirer.tick(); + // Archived repositories are not credential failures: report every tick, + // without permission guidance. + expect(warns).toHaveLength(2); + expect(warns.every((line) => !line.includes("Actions: Read"))).toBe(true); + } finally { + console.warn = originalWarn; + } + }); + test("a deferred repair preserves the retry budget and remains eligible", async () => { workerState.recordAgentPr({ repo: "acme/widgets", prNumber: 42 }); const gh: FakeGitHubState = { diff --git a/packages/code/tests/github-reviews-client.test.ts b/packages/code/tests/github-reviews-client.test.ts index 3d7aad0e..3401d108 100644 --- a/packages/code/tests/github-reviews-client.test.ts +++ b/packages/code/tests/github-reviews-client.test.ts @@ -185,6 +185,33 @@ describe("GitHubReviewsClient auth failure path", () => { true, ); expect(isGitHubAuthError(new Error("No GitHub authentication configured."))).toBe(true); + expect( + isGitHubAuthError( + new Error("GitHub API error (403): Resource not accessible by personal access token"), + ), + ).toBe(true); + expect( + isGitHubAuthError( + new Error("GitHub API error (403): Resource not accessible by integration"), + ), + ).toBe(true); + // Rate limits are transient, not credential failures. + expect(isGitHubAuthError(new Error("GitHub API error (403): API rate limit exceeded"))).toBe( + false, + ); + expect( + isGitHubAuthError(new Error("GitHub API error (403): You have triggered abuse detection")), + ).toBe(false); + // GitHub reuses 403 for non-credential conditions; these must keep the + // normal capture/retry path instead of being hidden behind auth throttles. + for (const detail of [ + "Repository was archived so is read-only.", + "This issue is locked.", + "Repository access blocked", + "Forbidden", + ]) { + expect(isGitHubAuthError(new Error(`GitHub API error (403): ${detail}`))).toBe(false); + } expect(isGitHubAuthError(new Error("GitHub API error (404): Not Found"))).toBe(false); expect(isGitHubAuthError(new Error("GitHub API error (500): Server Error"))).toBe(false); expect(isGitHubAuthError(null)).toBe(false); diff --git a/packages/code/tests/review-polling-acquirer.test.ts b/packages/code/tests/review-polling-acquirer.test.ts index d2cd0d9f..234ccd49 100644 --- a/packages/code/tests/review-polling-acquirer.test.ts +++ b/packages/code/tests/review-polling-acquirer.test.ts @@ -1125,6 +1125,239 @@ describe("ReviewPollingAcquirer", () => { } }); + test("403 permission denials are throttled like rejected credentials", async () => { + workerState.recordAgentPr({ repo: "acme/widgets", prNumber: 1 }); + workerState.recordAgentPr({ repo: "acme/widgets", prNumber: 2 }); + + const forbidden = new Error( + "GitHub API error (403): Resource not accessible by personal access token", + ); + const warns: string[] = []; + const originalWarn = console.warn; + console.warn = (...args: unknown[]) => { + warns.push(args.join(" ")); + }; + try { + const acquirer = new ReviewPollingAcquirer({ + intervalSeconds: 60, + workerState, + queue, + github: { + async fetchPr() { + throw forbidden; + }, + async fetchReviews() { + throw new Error("must not be reached"); + }, + async fetchReviewCommentsSince() { + throw new Error("must not be reached"); + }, + }, + addressPr: async () => true, + }); + + // Both PRs hit the same permanent permission failure, but only the + // first triggers a warning this hour. + await acquirer.tick(); + expect(warns).toHaveLength(1); + // A 403 means the token may be valid but under-scoped, so the guidance + // must not tell the operator to renew it. + expect(warns[0]).toContain("lacks a permission"); + expect(warns[0]).not.toContain("invalid, expired, or revoked"); + } finally { + console.warn = originalWarn; + } + }); + + test("fleet-mode credential failures are throttled per repository", async () => { + workerState.recordAgentPr({ repo: "acme/a", prNumber: 1 }); + workerState.recordAgentPr({ repo: "acme/b", prNumber: 2 }); + let now = 1_000; + const failures = new Map([ + [ + "acme/a", + new Error("GitHub API error (403): Resource not accessible by personal access token"), + ], + ["acme/b", new Error("GitHub API error (401): Bad credentials")], + ]); + const warns: string[] = []; + const logs: string[] = []; + const originalWarn = console.warn; + const originalLog = console.log; + console.warn = (...args: unknown[]) => { + warns.push(args.join(" ")); + }; + console.log = (...args: unknown[]) => { + logs.push(args.join(" ")); + }; + const recoveryLines = (repo: string) => + logs.filter( + (line) => line.includes("GitHub credential accepted again") && line.includes(repo), + ).length; + try { + const acquirer = new ReviewPollingAcquirer({ + intervalSeconds: 60, + now: () => now, + workerState, + queue, + github: { + async fetchPr(repo) { + const failure = failures.get(repo); + if (failure) throw failure; + return { data: { state: "open" }, etag: 'W/"pr"', notModified: false }; + }, + async fetchReviews() { + return { data: [], etag: 'W/"rev"', notModified: false }; + }, + async fetchReviewCommentsSince() { + return []; + }, + }, + addressPr: async () => true, + }); + + await acquirer.tick(); + // A 403 in repo A must not hide repo B's 401. + expect(warns).toHaveLength(2); + expect(warns.find((line) => line.includes("acme/a#1"))).toContain("lacks a permission"); + expect(warns.find((line) => line.includes("acme/b#2"))).toContain( + "invalid, expired, or revoked", + ); + + now += 60_000; + await acquirer.tick(); + expect(warns).toHaveLength(2); + expect(recoveryLines("acme/a")).toBe(0); + + // Repo B recovers while repo A still fails: B rearms and reports + // recovery, A stays throttled. + failures.delete("acme/b"); + now += 60_000; + await acquirer.tick(); + expect(warns).toHaveLength(2); + expect(recoveryLines("acme/b")).toBe(1); + expect(recoveryLines("acme/a")).toBe(0); + } finally { + console.warn = originalWarn; + console.log = originalLog; + } + }); + + test("metadata-only success does not clear a reviews/comments throttle", async () => { + workerState.recordAgentPr({ repo: "acme/widgets", prNumber: 42 }); + let now = 1_000; + let failReviews = true; + let pollFeedback = true; + const warns: string[] = []; + const logs: string[] = []; + const originalWarn = console.warn; + const originalLog = console.log; + console.warn = (...args: unknown[]) => { + warns.push(args.join(" ")); + }; + console.log = (...args: unknown[]) => { + logs.push(args.join(" ")); + }; + const recoveryLines = () => + logs.filter((line) => line.includes("GitHub credential accepted again")).length; + try { + const acquirer = new ReviewPollingAcquirer({ + intervalSeconds: 60, + now: () => now, + workerState, + queue, + shouldPollFeedback: () => pollFeedback, + github: { + async fetchPr() { + return { + data: { state: "open", mergeable_state: "clean" }, + etag: 'W/"pr"', + notModified: false, + }; + }, + async fetchReviews() { + if (failReviews) { + throw new Error( + "GitHub API error (403): Resource not accessible by personal access token", + ); + } + return { data: [], etag: 'W/"rev"', notModified: false }; + }, + async fetchReviewCommentsSince() { + return []; + }, + }, + addressPr: async () => true, + }); + + // The PR-state read works; only the reviews endpoint is rejected. + await acquirer.tick(); + expect(warns).toHaveLength(1); + expect(warns[0]).toContain("reading reviews"); + + // Relay healthy again and feedback polling skipped: the PR-state fetch + // succeeds, but the reviews endpoint was never re-tested, so no recovery + // may be announced and the throttle must stay armed. + pollFeedback = false; + now += 60_000; + await acquirer.tick(); + expect(recoveryLines()).toBe(0); + expect(warns).toHaveLength(1); + + // Feedback polling resumed but reviews still fail: the old throttle + // holds, so no duplicate warning inside the hour. + pollFeedback = true; + now += 60_000; + await acquirer.tick(); + expect(warns).toHaveLength(1); + + // The reviews endpoint finally succeeds → recovery claimed exactly once. + failReviews = false; + now += 60_000; + await acquirer.tick(); + expect(recoveryLines()).toBe(1); + } finally { + console.warn = originalWarn; + console.log = originalLog; + } + }); + + test("non-credential 403s keep the normal per-PR capture path", async () => { + workerState.recordAgentPr({ repo: "acme/widgets", prNumber: 1 }); + workerState.recordAgentPr({ repo: "acme/widgets", prNumber: 2 }); + + const warns: string[] = []; + const originalWarn = console.warn; + console.warn = (...args: unknown[]) => { + warns.push(args.join(" ")); + }; + try { + const acquirer = new ReviewPollingAcquirer({ + intervalSeconds: 60, + workerState, + queue, + github: { + async fetchPr() { + throw new Error("GitHub API error (403): Repository was archived so is read-only."); + }, + async fetchReviews() { + throw new Error("must not be reached"); + }, + async fetchReviewCommentsSince() { + throw new Error("must not be reached"); + }, + }, + addressPr: async () => true, + }); + + await acquirer.tick(); + expect(warns).toHaveLength(2); + expect(warns.every((line) => !line.includes("lacks a permission"))).toBe(true); + } finally { + console.warn = originalWarn; + } + }); + test("non-auth polling failures warn per PR as before", async () => { workerState.recordAgentPr({ repo: "acme/widgets", prNumber: 1 }); workerState.recordAgentPr({ repo: "acme/widgets", prNumber: 2 });