fix(mt#4435): Authenticate the reviewer's background schedulers as its own GitHub App by minsky-ai[bot] · Pull Request #3254 · edobry/minsky

minsky-ai · 2026-08-22T06:39:39Z

Summary

The reviewer service runs two credential systems in one process, and both background
schedulers were reading the one this service does not provision.

The review path authenticates with the reviewer App's own credentials —
MINSKY_REVIEWER_APP_ID / _PRIVATE_KEY / _INSTALLATION_ID, read via requireEnv
(config.ts:152-155), so the service cannot boot without them. That is why reviews post as
minsky-reviewer[bot].
The schedulers called createTokenProvider(cfg.github ?? {}, cfg.github?.token ?? "")
against the DOMAIN config, whose github.serviceAccount is populated from a different
namespace entirely — MINSKY_APP_ID / MINSKY_APP_PRIVATE_KEY_FILE /
MINSKY_APP_INSTALLATION_ID (environment.ts:31-34). This service has no reason to set those,
so the provider fell through to FallbackTokenProvider("") and every request went out
unauthenticated against GitHub's 60-per-hour per-IP budget.

Both schedulers already received the working credentials and threw them away —
void config; // held for future use, in pr-watch-scheduler.ts and
asks-reconcile-scheduler.ts alike.

The symptom was not a crash. runWatcher catches each watch's error into a counter every
caller dropped, so a fully rate-limited scheduler logged poll_complete each cycle and delivered
nothing: 18 rate-limit errors across 41 minutes, in pairs on nearly every cycle after onset,
with no escalation. The module docblock asserted the client was "backed by the Minsky implementer
GitHub App token" and reasoned about a 5000/hour App budget — both were intent, never
behavior, which is what kept this invisible.

Key changes

services/reviewer/src/github-token-provider.ts (new) — one seam both schedulers share,
building a GitHubAppTokenProvider from ReviewerConfig. Missing or malformed credentials
throw; the defect being fixed was precisely a silent fallback.
Both schedulers refuse to start on missing credentials rather than polling uselessly, and
both now thread config through to the provider.
Rate-limit rejections are classified distinctly in the domain watcher, keyed on the message
rather than the status code — GitHub returns 403 for both a rate-limit and a permission
denial, and those need opposite responses.
The scheduler consumes the error count it used to drop. A cycle in which every inspected
watch failed starts an exponential backoff (capped so it still re-probes inside GitHub's hourly
reset) and escalates once at 3 consecutive cycles — grounded in the observed 9-cycle
incident rather than a round number.

Why the reviewer App and not the implementer App. The accepted "Hybrid read/write split for
GitHub operations" decision makes reads identity-agnostic ("any authenticated user can read
PRs they have access to"), and both schedulers only read. Its Open Question 1 names this exact
case — "trigger to revisit: when Minsky gets a hosted/headless execution mode" — and the
identity/provenance position paper calls the reviewer's separate credentials "the load-bearing
isolation that makes 'adversarial review'" work. Importing the implementer App here would erode
that for no benefit and require env vars this service lacks. No new provisioning is needed: the
credentials are already requireEnv-mandatory, so they exist wherever the service boots at all.

Testing

Execution evidence:

SC1 / SC2 / SC6 + AT1 — the seam, and the scheduler refusing to start without it.

$ cd services/reviewer && bun test --preload ../../tests/setup.ts \
    src/github-token-provider.test.ts src/pr-watch-scheduler.test.ts \
    src/asks-reconcile-scheduler.test.ts
 32 pass  0 fail  49 expect() calls

AT1 asserts startPrWatchScheduler returns null for an empty private key and for a NaN app id,
with a paired control asserting it returns a live handle for valid credentials — without that,
both refusals would pass equally well for a scheduler that never starts at all.

SC3 + AT2 (classification half) — including the discriminating negative case.

$ bun test --preload ./tests/setup.ts packages/domain/src/pr-watch/watcher.test.ts
 40 pass  0 fail  141 expect() calls

34 of those are pre-existing and are the control for the watcher edit. The 6 new ones assert
isGitHubRateLimitError on the exact production string, the authenticated per-user form, the
secondary-limit wording — and that "Resource not accessible by integration" is not
classified as a rate limit. That last is the case that matters: a status-code check cannot
separate the two, and misclassifying it would back off 30 minutes against a fault waiting never
fixes.

SC4 / SC5 + AT2 (deferral half) / AT3 — asserted on the extracted pure decision function
evaluateCycleOutcome: exponential growth, the 30-tick cap, escalate-exactly-once at the
threshold, no re-escalation while the fault persists, re-escalation after a recovery, and that a
zero-watch or partially-failing cycle is not a total failure.

AT5 — no live unauthenticated-capable construction remains.

$ grep -rn 'createTokenProvider' services/reviewer/src --include='*.ts' \
    | grep -v '//' | grep -v '^\S*: *\*'
  (zero lines)

All 4 remaining textual mentions are comments/docblocks quoting the old expression deliberately.
The spec's original grep for this AT was corrected during implementation — it matched those
comments and so would have reported failure on a correct fix.

Full related sweep: bun scripts/run-related-tests.ts … → 5 related test files passed.
validate_typecheck 0 errors across 8 projects. validate_lint 0 errors / 0 warnings across
3900 files. format:check clean.

Negative control: reverting to the shipped expression, evaluated against this service's actual config shape

$ bun -e 'import {createTokenProvider} from "@minsky/domain/auth";
          const p = createTokenProvider({}, "");
          console.log(p.isServiceAccountConfigured(), JSON.stringify(await p.getServiceToken("edobry/minsky")));'
  false ""

An empty token string — the unauthenticated request, demonstrated directly rather than
inferred from the logs. The post-fix provider returns isServiceAccountConfigured() === true,
which is the assertion the new test makes.

Negative control — the test fixture: asks-reconcile-scheduler.test.ts carried privateKey: ""
and passed for as long as it existed, because the scheduler never used the credentials it was
handed. That is the production defect in miniature. Adding the credential guard made it fail
(expect(handle).not.toBeNull() → received null); the fixture now carries a real-shaped key.

Deviations recorded in the spec

Three, each amended in the criterion's own text rather than explained elsewhere:

AT5's grep now excludes comments (above).
SC5's backoff keys on total-failure, not the reset header. The domain GithubPrClient
interface returns domain types and throws — no response object or headers reach the caller,
so honoring the reset header would mean widening three methods plus the Octokit adapter for a
signal only one of several faults carries. Total-failure backoff reaches the same goal and also
covers a revoked credential and a GitHub outage. Trade-off accepted: recovery is noticed up to
30 minutes late rather than at the exact reset instant.
SC3 shipped PARTIALLY — classification yes, reset time no. The criterion's parenthetical
asks for the log line to carry "the reset time GitHub returns." It does not, for the same
structural reason as (2): nothing surfaces x-ratelimit-reset to the caller. Deliberately not
worked around by parsing a timestamp out of the message text — GitHub's rate-limit body does
not reliably carry one, and a parser that silently yields nothing would be a can't-fail probe.
Whoever widens GithubPrClient for (2) should add it to this log line in the same change.

Live verification

UNVERIFIED — deferred to §10 post-deploy, because the fault is environment-bound. The
assertion that matters operationally is "the deployed scheduler now authenticates," and that
cannot be produced from a session workspace: it depends on the reviewer service's own env, which
holds the App credentials this change reaches for. Nothing here is blocked on operator access —
the check simply has to run against the deployed process.

Post-deploy, §10 will read the reviewer's logs and confirm zero rate limit exceeded lines
and poll_complete cycles with errors: 0 — which is AT4. Until that runs, this change is
not confirmed working in production; deploy-SUCCESS alone would only prove the container started.

Deploy verification: required and not waived. isDeploySurfaceFile() returns true for all 8
changed files (verified by running the predicate, not by recalling the pattern set), so §10 runs
against the merge timestamp.

Context

Diagnosed while investigating why PR #3253 had zero reviews; that turned out to be a separate
defect (mt#4434, GitHub's 20,000-line diff cap). These rate-limit errors sat in the same log window
and are independent of it — two distinct failures in one 41-minute window.

Closes mt#4435.

…b App The reviewer service runs two credential systems in one process, and both background schedulers were reading the one this service does not provision. The review path authenticates with the reviewer App's own credentials — MINSKY_REVIEWER_APP_ID / _PRIVATE_KEY / _INSTALLATION_ID, read via requireEnv, so the service cannot boot without them. That is why reviews post as minsky-reviewer[bot]. The schedulers instead called createTokenProvider(cfg.github ?? {}, cfg.github?.token ?? "") against the DOMAIN config, whose github.serviceAccount is populated from a different namespace entirely (MINSKY_APP_ID / MINSKY_APP_PRIVATE_KEY_FILE / MINSKY_APP_INSTALLATION_ID). This service has no reason to set those, so the provider fell through to FallbackTokenProvider("") and every request went out unauthenticated against GitHub's 60-per-hour per-IP budget. Both schedulers already RECEIVED the working credentials and explicitly threw them away — `void config; // held for future use` at pr-watch-scheduler.ts and asks-reconcile-scheduler.ts alike. The symptom was not a crash. runWatcher catches each watch's error into a counter every caller dropped, so a fully rate-limited scheduler logged poll_complete each cycle and delivered nothing: 18 rate-limit errors across a 41-minute window, in pairs on nearly every cycle after onset, with no escalation. The docblock asserted the client was "backed by the Minsky implementer GitHub App token" and reasoned about a 5000/hour App budget — both were intent, never behavior, which is what kept it invisible. Changes: - New services/reviewer/src/github-token-provider.ts — one seam both schedulers share, building a GitHubAppTokenProvider from ReviewerConfig. Missing or malformed credentials THROW rather than degrading; the defect being fixed was precisely a silent fallback. - Both schedulers now refuse to start on missing credentials instead of polling uselessly, and both thread config through to the provider. - Rate-limit rejections are classified distinctly in the domain watcher, keyed on the message rather than the status code: GitHub returns 403 for both a rate-limit and a permission denial, and those need opposite responses. - The scheduler now consumes the error count it used to drop: a cycle where every inspected watch failed starts an exponential backoff (capped so it still re-probes within GitHub's hourly reset) and escalates once at 3 consecutive cycles, grounded in the observed 9-cycle incident rather than a round number. Why the reviewer App and not the implementer App: the accepted "Hybrid read/write split for GitHub operations" decision makes reads identity-agnostic, and both schedulers only read. Its Open Question 1 names this exact case ("trigger to revisit: when Minsky gets a hosted/headless execution mode"), and the identity/provenance position paper calls the reviewer's separate credentials "load-bearing isolation" — so importing the implementer App here would erode that for no benefit and require env vars this service lacks. The asks-reconcile test fixture carried privateKey: "" and passed, because the scheduler never used the credentials it was handed. That is the production defect in miniature; the fixture now carries a real-shaped key. This IS deploy surface: isDeploySurfaceFile() returns true for all 8 changed files, so post-merge deploy verification runs rather than being waived.

…aired control AT1 asserts the SCHEDULER declines to run without usable credentials, which the unit tests covered only at the validator and provider level. Adds three cases against startPrWatchScheduler directly: empty private key and NaN app id both return null, and a third case with valid credentials returns a live handle. That third case is the control. Without it both refusals would pass just as well for a scheduler that never starts under any circumstances, which is the failure shape a null-return assertion invites. Also records two implementation deviations in the spec rather than leaving them for the reviewer: the shared-seam acceptance grep now excludes comments (the fix deliberately quotes the old expression in three docblocks, so the bare grep returns matches on a correct fix), and the backoff keys on total-failure rather than a reset header, because the domain GithubPrClient interface throws Errors and surfaces no response headers to key on.

minsky-reviewer · 2026-08-22T06:39:46Z

Minsky Reviewer Status

Verdict: APPROVED — no blocking findings
Review: View review
Model: openai/gpt-5 | Tokens: 470K prompt, 5K completion | Duration: 86s
Mode: normal

Commands

/review — request a fresh review

minsky-reviewer

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2

Solid fix direction with good tests, but two operator-visible inconsistencies need correction before merge. Both schedulers log a “started/enabled” line before validating credentials; when credentials are missing, they immediately refuse to start, producing contradictory logs (claims started/enabled, then “will NOT start”). Move the findMissingReviewerCredentials guard ahead of those logs. Minor: PR-watch startup logs the configured interval, not the jittered effective interval actually used; consider logging both. Also, the new rate-limit classifier may over-match generic messages — acceptable as a first pass but worth tightening later. I reviewed all touched files in the diff. I did not sweep broader docs beyond code comments; no public-surface changes appear to require docs updates.

Findings

[BLOCKING] services/reviewer/src/pr-watch-scheduler.ts:1 — Misleading "started" log is emitted before the credential guard and can claim the scheduler started when it immediately refuses to start
In startPrWatchScheduler, the code logs log.info("pr_watch_scheduler.started", …) before checking findMissingReviewerCredentials(config). When credentials are missing or malformed, the function then logs an error and returns null. This produces an operator-facing log sequence that first claims the scheduler "started" and then says it "will NOT start," which is contradictory and can mask the true state during incidents. The credential guard must run before any "started/enabled" log to avoid false-positive startup confirmations. Suggested fix: move the missing-credential check to precede the "started" log (and mirror this ordering consistently across both schedulers).
[BLOCKING] services/reviewer/src/asks-reconcile-scheduler.ts:1 — Misleading "enabled" log is emitted before the credential guard and can claim the scheduler is enabled/starting when it immediately refuses to start
In startAsksReconcileScheduler, the code logs log.info("asks_reconcile_scheduler.enabled", …) before checking findMissingReviewerCredentials(config). When credentials are missing or malformed, the function then logs an error and returns null. This yields contradictory operator logs ("enabled" followed by "will NOT start"). To avoid false-positive startup confirmations, run the missing-credential guard before any "enabled/started" log (and keep the order consistent with the PR-watch scheduler after fixing it per the sibling finding).
[NON-BLOCKING] services/reviewer/src/pr-watch-scheduler.ts:135 — Operator log reports the configured interval, not the jittered effective interval actually used
startPrWatchScheduler computes a per-instance jitter and uses effectiveIntervalMs = intervalMs + jitter for the timer, but the startup log pr_watch_scheduler.started records only schedulerConfig.intervalMs. This can mislead operators comparing observed cadence to logs. Suggestion: include the computed effectiveIntervalMs (and jitter) in the startup log fields, or emit a separate log line naming both configured and effective intervals.
[NON-BLOCKING] packages/domain/src/pr-watch/watcher.ts:146 — Rate-limit classifier may over-match unrelated messages containing "rate limit exceeded"
isGitHubRateLimitError returns true for any message matching /rate limit exceeded/i or /secondary rate limit/i. While practical, this can misclassify non-GitHub contexts or wrapper messages (e.g., an upstream proxy or different service that includes similar wording) as a GitHub rate-limit. Consider scoping the regex to GitHub-specific phrasings seen in practice (e.g., anchoring to API rate limit exceeded or including GitHub/Resource not accessible exclusions), or keying on a structured error code if available from the Octokit client.

Documentation impact

no-update-needed — All changes are internal scheduler/authentication behavior and operator logs in code comments. No public API, CLI, or docs behavior change is introduced by this diff. I did not find any docs in-repo that describe the reviewer schedulers’ credential source or backoff semantics to be invalidated; the updates are code-level and test-only.

…hor the classifier Both BLOCKING findings were one class: the credential guard ran AFTER the "started"/"enabled" log, so a scheduler that refused to start had already claimed it started. The reviewer was right, and the evidence was already in this PR's own test output — `pr_watch_scheduler.started` immediately followed by `missing_github_credentials ... will NOT start` — which I read past. That is the same claim-without-the-behavior shape as the docblock that hid the original defect, so it gets a regression test rather than only a reordering: asserting no `started` event is logged on refusal, with a paired control asserting it IS logged on a successful start. R1 BLOCKING (both fixed): - pr-watch-scheduler.ts: credential guard moved above the `started` log, alongside the enabled/container guards. - asks-reconcile-scheduler.ts: same, above the `enabled` log. R1 NON-BLOCKING (both fixed): - The `started` line now reports `effectiveIntervalMs` beside `intervalMs`. Jitter is computed before the log so the line can carry the interval the timer actually runs on; reporting only the configured value makes an observed cadence look like drift against the config. - isGitHubRateLimitError is anchored on `api rate limit exceeded` rather than a bare `rate limit exceeded`, which matched any message merely CONTAINING the phrase — a PR title, a quoted upstream error. A false positive there is not cosmetic: it would trigger a 30-minute backoff against a fault waiting does not fix. Two new cases cover the over-match. 34 tests across the 5 related files pass; typecheck 0 errors across 8 projects; lint 0 errors / 0 warnings across 3900 files.

minsky-reviewer

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2

Verified the R1 BLOCKING issues were addressed: both schedulers now guard credentials BEFORE emitting a started/enabled log, and both use a shared createReviewerTokenProvider(config) seam. The watcher gained a robust rate-limit classifier with targeted tests, and the scheduler implements total-failure backoff plus single escalation, matching the amended spec. I found no new critical defects introduced by the fixes. Minor nit: the asks-reconcile header still mentions the implementer App token; suggest updating that comment to reflect the reviewer App usage. Otherwise, changes look solid and aligned with the task spec. Event is APPROVE.

Findings

[NON-BLOCKING] services/reviewer/src/asks-reconcile-scheduler.ts:24 — Header comment still references implementer App token; code now authenticates with reviewer App
The module docstring under "Invocation mechanism" states that a makeProductionGithubReviewClient is constructed from the Minsky implementer GitHub App token. As of mt#4435, the scheduler authenticates via createReviewerTokenProvider(config) using the reviewer service's own App (see lines 113-120). Consider updating the comment to avoid misleading future readers about which credential namespace is used.

Spec verification

Criterion	Status	Evidence
The reviewer's pr-watch scheduler makes authenticated GitHub requests. Its token source is the same one the rest of the reviewer service already uses successfully — note that the review path itself authenticates fine against this repo, so a working credential exists in the process.	Met	services/reviewer/src/pr-watch-scheduler.ts:259-267 — replaces domain config token with `createReviewerTokenProvider(config)` built from `ReviewerConfig` (the reviewer's own App).
An empty or missing token is a startup-visible condition, not a silent degradation: constructing a GitHub client with empty auth logs an error naming the missing configuration, or fails fast. A `?? ""` fallback that silently produces unauthenticated calls is removed.	Met	services/reviewer/src/pr-watch-scheduler.ts:324-341 — `findMissingReviewerCredentials(config)` check runs before start; logs `pr_watch_scheduler.missing_github_credentials` and returns null. Mirror in services/reviewer/src/asks-reconcile-scheduler.ts:120-139 for the asks scheduler.
A rate-limit response is classified and logged as such (with the reset time GitHub returns), distinctly from `unexpected error processing watch`.

PARTIALLY AMENDED (2026-08-22): classification shipped; the RESET TIME did not. | Met | packages/domain/src/pr-watch/watcher.ts:66-96 introduces isGitHubRateLimitError and packages/domain/src/pr-watch/watcher.ts:106-125 logs a distinct pr-watch: GitHub rate limit exceeded — … with classification: "github-rate-limit". Reset time omission is accepted per amended criterion. |
| Repeated per-cycle watch failures escalate rather than accumulating in a dropped counter: when every active watch errors for N consecutive cycles, the scheduler surfaces it once (operator-visible) instead of logging the same pair forever. | Met | services/reviewer/src/pr-watch-scheduler.ts:409-449 — uses evaluateCycleOutcome to track a total-failure streak and logs a single pr_watch_scheduler.all_watches_failing escalation when the threshold is crossed. |
| On rate-limit exhaustion the scheduler backs off until the reset time rather than re-polling every 60 seconds into a limit it knows is exhausted.

AMENDED during implementation (2026-08-22) — backoff keys on TOTAL FAILURE, not on the reset header. | Met | services/reviewer/src/pr-watch-scheduler.ts:146-196 defines evaluateCycleOutcome with exponential backoff capped at 30 ticks and services/reviewer/src/pr-watch-scheduler.ts:396-407 consumes it via ticksToSkip. This matches the amended criterion. |
| Both schedulers are fixed at a shared seam: asks-reconcile-scheduler.ts carries the identical createTokenProvider(cfg.github ?? {}, userToken) construction and is unauthenticated for the same reason. A grep for that construction across services/reviewer/src/** returns no remaining unauthenticated-capable call site. | Met | services/reviewer/src/asks-reconcile-scheduler.ts:113-120 — now uses createReviewerTokenProvider(config); prior createTokenProvider(cfg.github ?? {}, …) removed. The pr-watch scheduler likewise uses the shared seam at services/reviewer/src/pr-watch-scheduler.ts:259-267. |

Adoption sweep

Symbol	Kind	Consumers found	Classification	Notes
packages/domain/src/pr-watch/watcher.ts:isGitHubRateLimitError	function	packages/domain/src/pr-watch/watcher.ts:115 — used in catch branch to classify errors, packages/domain/src/pr-watch/watcher.test.ts:38-101 — direct unit tests import and assert behavior	Adopted	New exported predicate; consumed internally by watcher and covered by tests.

Documentation impact

no-update-needed — Behavioral changes are internal to the reviewer service’s background schedulers and domain watcher logging. No CLI/API surface or documented flags changed. Comments in-code were updated to reflect the new auth path; docs/ was not modified and does not describe these internals.

…claim (R2) R2 non-blocking: the asks-reconcile header still said the review client is "constructed from the Minsky implementer GitHub App token" while the code now authenticates as the reviewer App. This is the same class I corrected in pr-watch-scheduler.ts on the first commit and missed next door — a class-not-instance miss on my part. It matters more than an ordinary stale comment here: a docblock asserting App-authentication that the code did not perform is exactly what kept the original defect invisible, so shipping one in the PR that fixes it would reproduce the failure mode while describing it. Both mentions in the file are corrected, and the module docblock records what it previously claimed so the next reader can see the claim was intent rather than behavior. Swept the other touched files for the same class: the remaining "implementer" mentions are either historical corrections or statements that are accurate — the reviewer App's token IS distinct from the implementer's, which is now true in fact and not only in prose. Comment-only; format:check clean, related tests pass.

minsky-reviewer

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2

Re-verify pass focused on the fix commit’s changes to asks-reconcile and the shared reviewer-token seam. Prior BLOCKING issues about unauthenticated scheduler calls have been addressed: both schedulers now use createReviewerTokenProvider(config) built from the reviewer App creds, and both refuse to start when credentials are missing via findMissingReviewerCredentials(...). The asks-reconcile docblock was corrected to match behavior. In the PR-watch domain, rate-limit classification and the total-failure backoff/escalation policy remain consistent with the amended spec. I found no new defects introduced by this commit; type usage and token plumbing align with the domain clients (listReviews context only requires getToken). Documentation impact is internal-only; no user-facing changes. Overall, this iteration resolves the prior blockers without introducing regressions — APPROVE.

Spec verification

Criterion	Status	Evidence
The reviewer's pr-watch scheduler makes authenticated GitHub requests. Its token source is the same one the rest of the reviewer service already uses successfully — note that the review path itself authenticates fine against this repo, so a working credential exists in the process.	Met	services/reviewer/src/pr-watch-scheduler.ts:129-147 — `createReviewerTokenProvider(config)` builds a TokenProvider from the reviewer App credentials; `makeProductionGithubPrClient(tokenProvider)` uses it for all reads. Docblock explains move away from domain `createTokenProvider` fallback.
An empty or missing token is a startup-visible condition, not a silent degradation: constructing a GitHub client with empty auth logs an error naming the missing configuration, or fails fast. A `?? ""` fallback that silently produces unauthenticated calls is removed.	Met	services/reviewer/src/pr-watch-scheduler.ts:175-197 — `findMissingReviewerCredentials(config)` gate logs `pr_watch_scheduler.missing_github_credentials` and returns null. services/reviewer/src/asks-reconcile-scheduler.ts:152-172 — same guard logs `asks_reconcile_scheduler.missing_github_credentials` and refuses to start. No `?? ""` fallbacks remain at either seam.
A rate-limit response is classified and logged as such (with the reset time GitHub returns), distinctly from `unexpected error processing watch`. (PARTIALLY AMENDED: classification shipped; reset time omitted until headers are surfaced.)	Met	packages/domain/src/pr-watch/watcher.ts:73-102, 129-144 — `isGitHubRateLimitError` predicate and distinct `log.error("pr-watch: GitHub rate limit exceeded — …", { classification: "github-rate-limit" })`. Reset time intentionally omitted per spec amendment.
Repeated per-cycle watch failures escalate rather than accumulating in a dropped counter: when every active watch errors for N consecutive cycles, the scheduler surfaces it once (operator-visible) instead of logging the same pair forever.	Met	services/reviewer/src/pr-watch-scheduler.ts:57-93, 265-304 — `evaluateCycleOutcome` and use in scheduler; escalates exactly once when streak hits threshold via `pr_watch_scheduler.all_watches_failing`.
On rate-limit exhaustion the scheduler backs off until the reset time rather than re-polling every 60 seconds into a limit it knows is exhausted. (AMENDED: backoff keys on TOTAL FAILURE, capped exponential.)	Met	services/reviewer/src/pr-watch-scheduler.ts:57-93 and 241-262 — total-failure detection drives exponential backoff with `ticksToSkip` and `MAX_BACKOFF_SKIP_TICKS` cap; matches amended criterion.
Both schedulers are fixed at a shared seam: `asks-reconcile-scheduler.ts` carries the identical `createTokenProvider(cfg.github ?? {}, userToken)` construction and is unauthenticated for the same reason. A grep for that construction across `services/reviewer/src/**` returns no remaining unauthenticated-capable call site.	Met	services/reviewer/src/asks-reconcile-scheduler.ts:116-124 — replaced old domain `createTokenProvider(...)` with `createReviewerTokenProvider(config)`; services/reviewer/src/pr-watch-scheduler.ts:129-147 already uses same seam. No remaining `createTokenProvider(cfg.github ?? {}, ...)` in these files.

Documentation impact

no-update-needed — Change scopes background schedulers’ authentication and error-handling; no user-facing CLI, API, or documented configuration keys changed. Reviewer env vars used (MINSKY_REVIEWER_*) already existed and remain the same. No docs in-repo appear to describe scheduler auth identity or backoff semantics beyond code comments.

minsky-reviewer

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2

Re-verify pass against the current diff: the prior BLOCKING issues (unauthenticated schedulers and silent fallback) are addressed. Both schedulers now authenticate via the reviewer App using a shared seam (createReviewerTokenProvider) and refuse to start when credentials are missing using findMissingReviewerCredentials, with startup-visible logs. The PR-watch domain adds rate-limit classification keyed on message text, and the scheduler introduces a total-failure backoff and single-shot escalation, matching the amended spec. Tests cover the new seam, the credential guard, the classification predicate, and the backoff policy. I found no new critical defects introduced by this iteration. Documentation impact is internal-only. Verdict: APPROVE.

Spec verification

Criterion	Status	Evidence
The reviewer's pr-watch scheduler makes authenticated GitHub requests. Its token source is the same one the rest of the reviewer service already uses successfully — note that the review path itself authenticates fine against this repo, so a working credential exists in the process.	Met	services/reviewer/src/pr-watch-scheduler.ts:255-262 switches to `createReviewerTokenProvider(config)` (reviewer App creds) instead of `createTokenProvider(cfg.github, …)`. services/reviewer/src/asks-reconcile-scheduler.ts:121-128 does the same for asks-reconcile. New seam implemented in services/reviewer/src/github-token-provider.ts.
An empty or missing token is a startup-visible condition, not a silent degradation: constructing a GitHub client with empty auth logs an error naming the missing configuration, or fails fast. A `?? ""` fallback that silently produces unauthenticated calls is removed.	Met	services/reviewer/src/pr-watch-scheduler.ts:334-351 guards with `findMissingReviewerCredentials(config)` and logs `pr_watch_scheduler.missing_github_credentials` then returns null. services/reviewer/src/asks-reconcile-scheduler.ts:214-234 adds identical guard and refuses to start. The silent `?? ""` construction was removed from both schedulers.
A rate-limit response is classified and logged as such (with the reset time GitHub returns), distinctly from `unexpected error processing watch`. (PARTIALLY AMENDED: classification shipped; the RESET TIME did not.)	Met	packages/domain/src/pr-watch/watcher.ts:128-180 adds `isGitHubRateLimitError` and uses it to emit `pr-watch: GitHub rate limit exceeded — …` with `classification: "github-rate-limit"`; the reset-time omission is accepted per the amended criterion.
Repeated per-cycle watch failures escalate rather than accumulating in a dropped counter: when every active watch errors for N consecutive cycles, the scheduler surfaces it once (operator-visible) instead of logging the same pair forever.	Met	services/reviewer/src/pr-watch-scheduler.ts:109-168 defines policy; 412-449 applies it, logging `pr_watch_scheduler.all_watches_failing` exactly once when the streak crosses the threshold.
On rate-limit exhaustion the scheduler backs off until the reset time rather than re-polling every 60 seconds into a limit it knows is exhausted. (AMENDED: backoff keys on TOTAL FAILURE, not on the reset header.)	Met	services/reviewer/src/pr-watch-scheduler.ts:109-168 (`evaluateCycleOutcome`) returns exponential `skipTicks` capped to 30; 399-409 consumes `ticksToSkip` to defer cycles. Matches the amended total-failure-based backoff.
Both schedulers are fixed at a shared seam: `asks-reconcile-scheduler.ts` carries the identical `createTokenProvider(cfg.github ?? {}, userToken)` construction and is unauthenticated for the same reason. A grep for that construction across `services/reviewer/src/**` returns no remaining unauthenticated-capable call site.	Met	Both schedulers now import and use the shared `createReviewerTokenProvider` (services/reviewer/src/pr-watch-scheduler.ts:70-77, 255-262; services/reviewer/src/asks-reconcile-scheduler.ts:47-55, 121-128). The prior `createTokenProvider(cfg.github ?? {}, …)` sites are removed in this diff.

Adoption sweep

Symbol	Kind	Consumers found	Classification	Notes
packages/domain/src/pr-watch/watcher.ts:isGitHubRateLimitError	function	packages/domain/src/pr-watch/watcher.test.ts — imports and asserts classification behavior	Adopted	Exported for direct testing of classification logic; no additional wiring required by spec.

Documentation impact

no-update-needed — Change scopes internal background schedulers’ authentication and backoff behavior. No CLI, API, or user-facing configuration keys were added or renamed. Inline module docblocks were updated; no docs/ files exist for these schedulers and no existing user docs describe their prior unauthenticated behavior, so there is nothing to invalidate.

edobry added 2 commits August 22, 2026 02:35

minsky-ai Bot added the authorship/co-authored Co-authored by human and AI agent label Aug 22, 2026

minsky-reviewer Bot suggested changes Aug 22, 2026

View reviewed changes

minsky-reviewer Bot approved these changes Aug 22, 2026

View reviewed changes

edobry merged commit 589166e into main Aug 22, 2026
20 checks passed

edobry deleted the task/mt-4435 branch August 22, 2026 07:00

minsky-reviewer Bot approved these changes Aug 22, 2026

View reviewed changes

Navigation Menu

fix(mt#4435): Authenticate the reviewer's background schedulers as its own GitHub App - #3254

fix(mt#4435): Authenticate the reviewer's background schedulers as its own GitHub App#3254
edobry merged 4 commits into
mainfrom
task/mt-4435

minsky-ai Bot commented Aug 22, 2026 •

edited

Loading

Uh oh!

minsky-reviewer Bot commented Aug 22, 2026 •

edited

Loading

Uh oh!

minsky-reviewer Bot left a comment

Uh oh!

minsky-reviewer Bot left a comment

Uh oh!

minsky-reviewer Bot left a comment

Uh oh!

Uh oh!

minsky-reviewer Bot left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

minsky-ai Bot commented Aug 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Key changes

Testing

Deviations recorded in the spec

Live verification

Context

Uh oh!

minsky-reviewer Bot commented Aug 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Minsky Reviewer Status

Commands

Uh oh!

minsky-reviewer Bot left a comment

Choose a reason for hiding this comment

Findings

Documentation impact

Uh oh!

minsky-reviewer Bot left a comment

Choose a reason for hiding this comment

Findings

Spec verification

Adoption sweep

Documentation impact

Uh oh!

minsky-reviewer Bot left a comment

Choose a reason for hiding this comment

Spec verification

Documentation impact

Uh oh!

Uh oh!

minsky-reviewer Bot left a comment

Choose a reason for hiding this comment

Spec verification

Adoption sweep

Documentation impact

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

minsky-ai Bot commented Aug 22, 2026 •

edited

Loading

minsky-reviewer Bot commented Aug 22, 2026 •

edited

Loading