feat(mt#1343): reviewThreads field in session_pr_review_context

[minsky-ai]

Summary

Adds a reviewThreads field to SessionPrReviewContextResult so the reviewer can see existing thread state on round N+1 instead of going in blind and duplicating findings. Implements mt#1343 in the mt#1335 in-band-review-semantics cluster. Foundation for mt#1345 (reply + auto-resolve loop) and a precondition for any iterative review to converge.

Motivation & Context

ADR-008 (PR #838) called out that without thread state in the reviewer's context, every review round restarts from scratch and duplicates findings — the dominant cause of multi-round burn observed in mt#1110_calibration (PR #763: 7 rounds, 3 distinct legit findings). REST get_review_comments returns flat comments without isResolved/isOutdated; the GraphQL pullRequest.reviewThreads query is the only way to get the structured thread state.

Design / Approach

• GraphQL fetch with pagination. pullRequest.reviewThreads(first: 50, after: $cursor) paged through until hasNextPage is false. Capped at 200 threads with truncated: true signaled in the result.
• Parallel composition. The new fetch runs alongside the existing pr / checks / diff / parsedDiff / taskSpec parallel fetches in pr-review-context-subcommand.ts.
• Non-fatal failure. getPRReviewThreads never throws on network/auth/GraphQL failures — returns { threads: [], truncated: false } and logs at debug level. Token acquisition failures degrade the same way.
• Identity flow. Routes through tokenProvider.getServiceToken(owner/repo) — matching the scoped pattern in asks-github-client.ts. FallbackTokenProvider ignores the scope and returns the user PAT directly; GitHubAppTokenProvider issues a per-repo installation token.
• DI test seam. getPRReviewThreads accepts an optional octokitOverride parameter — same pattern landed in mt#1346 (PR feat(mt#1346): GitHub Check Run annotations as parallel review surface #889). The project's no-global-module-mocks rule forbids mock.module(), so DI is canonical.

Key Changes

• src/domain/repository/github-pr-operations.ts — new types (ReviewThread, ReviewThreadComment, ReviewThreadsResult), GraphQL query constant, and getPRReviewThreads(gh, prNumber, octokitOverride?) exported function. Error handling changed to graceful degradation: token and GraphQL failures return empty result instead of rethrowing.
• src/domain/session/commands/pr-review-context-subcommand.ts — SessionPrReviewContextResult gains reviewThreads: ReviewThread[] and reviewThreadsTruncated: boolean. fetchReviewThreads uses tokenProvider.getServiceToken(owner/repo) scoped to the repository. Wired into the parallel-fetch Promise.all.
• src/domain/repository/github-pr-operations-review-threads.test.ts — 12 unit tests.

Testing

12 unit tests covering all spec-required cases:

• Single-page mapping (id, path, line, isResolved).
• Comment field mapping (author, body, createdAt; null author).
• Outdated thread (line null, isOutdated true).
• Multi-line range (startLine populated).
• Single-line absent-startLine (field omitted from result).
• Pagination (cursor passed correctly across multiple pages).
• Truncation at 200 (truncated true, capped count, 5 graphql calls).
• Empty PR (empty array, not undefined).
• Shape conformance to exported ReviewThread interface.
• truncatedComments true/false boundary (totalCount > 10 vs <= 10).
• Recoverable GraphQL error returns empty result instead of throwing.
• Token acquisition failure returns empty result (graceful degradation).

Tests inject a mock Octokit via the octokitOverride parameter — no module-level mocking.

Out of scope

• Reviewer prompt update to consume reviewThreads (mt#1345).
• Resolve / unresolve mutations (mt#1342, separate task).

Test plan

• 12/12 unit tests pass (bun test src/domain/repository/github-pr-operations-review-threads.test.ts).
• Lint clean.
• Typecheck clean.
• Reviewer to verify ADR-008 alignment + GraphQL query shape against GitHub API docs.

(Had Claude implement this — turn-limit cutoff before tests landed; main-agent recovery added the test suite using the DI seam pattern from mt#1346.)

[minsky-reviewer]

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

---

The reviewer ran but produced no findings. This is not an approval — the model emitted no submit_finding, submit_inline_comment, or conclude_review calls.

[minsky-reviewer]

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

---

⚠️ Reviewer did not emit a conclude_review call. Event derived from severity counts: REQUEST_CHANGES (4 BLOCKING / 2 NON-BLOCKING / 0 PRE-EXISTING findings). Executive summary unavailable.

Findings

• [BLOCKING] src/domain/repository/github-pr-operations.ts:722 — GraphQL limits comments to first 10 per thread with no pagination or truncation signal — silently drops older comments
  In REVIEW_THREADS_QUERY (src/domain/repository/github-pr-operations.ts:722), the query requests comments(first: 10) but there is no pagination loop to fetch remaining comments and no per-thread truncated indicator in the returned shape. The exported ReviewThread interface and mapping (lines ~780–810) imply comments is the ordered list for the thread, but with this query it will include at most 10 items, potentially omitting older comments on long threads. This is a silent data loss that can mislead reviewers and break iterative review convergence. Please either: (a) page through all comments for each thread (or request a sufficiently high cap and document any truncation), and/or (b) surface a per-thread truncation flag so consumers can detect incomplete data. Update tests to cover >10-comment threads and ordering guarantees.
• [BLOCKING] src/domain/session/commands/pr-review-context-subcommand.ts:146 — Token provider uses user token unconditionally for service token, diverging from stated identity flow and potentially bypassing app auth
  In fetchReviewThreads (src/domain/session/commands/pr-review-context-subcommand.ts:133–170), the code constructs a token provider with const userToken = cfg.github?.token ?? ""; const tokenProvider = createTokenProvider(cfg.github ?? {}, userToken); and then calls getToken: () => tokenProvider.getServiceToken(). This appears to route the service token through a provider initialized with the user PAT value, potentially returning the user token rather than the app installation token. The PR description claims identity flow routes through gh.getToken() "using the same TokenProvider path as other PR operations," but other code paths typically use gh.getToken() from the backend’s configured provider (app token). This change risks privilege escalation or permission mismatch and contradicts "Non-fatal failure"/"Identity flow" design. Please: (1) confirm createTokenProvider semantics; (2) if service token should be app token, initialize the provider exactly as elsewhere (pull from session/backend rather than ad hoc config), or add getUserToken fallback explicitly like other ops; (3) add tests for missing/invalid tokens.
• [BLOCKING] src/domain/repository/github-pr-operations.ts:812 — Error handling contradicts "Non-fatal failure" requirement; throws MinskyError instead of returning empty result
  getPRReviewThreads (src/domain/repository/github-pr-operations.ts:770–820) wraps the entire fetch in a try/catch and rethrows a new MinskyError on any non-Minsky error. The PR description and subcommand’s fetchReviewThreads emphasize that review-thread fetch should be non-fatal and degrade to empty results so the rest of review context proceeds. Exposing a throwing function here causes upstream callers that don’t handle it to fail hard. Either (a) make getPRReviewThreads return { threads: [], truncated: false } on recoverable GraphQL/token/network errors, logging at debug, or (b) clearly document that callers must catch and swallow errors (and enforce via types/tests). Currently, only the subcommand’s wrapper swallows; other potential call sites could inadvertently crash.
• [BLOCKING] src/domain/repository/github-pr-operations-review-threads.test.ts:1 — Spec/tests file path mismatch with PR description; repository contains different test filename
  The diff adds tests at src/domain/repository/github-pr-operations-review-threads.test.ts, but the PR description and Testing section refer to src/domain/repository/github-pr-review-threads.test.ts. This mismatch suggests either stale documentation or an incorrect test file path. Ensure naming is consistent and aligned with project conventions (other tests under this directory), and CI config picks up the intended file. If a previous file exists with the other name, de-duplicate or remove to avoid confusion.
• [NON-BLOCKING] src/domain/repository/github-pr-operations.ts:748 — Missing explicit comment ordering; code assumes "oldest first" but GraphQL default may differ
  ReviewThread.comments is documented as "Ordered list of comments in the thread (oldest first)" (src/domain/repository/github-pr-operations.ts:748), but the GraphQL query does not specify an orderBy for comments. Depending on GitHub’s default, the order may be newest-first or unspecified. If consumers rely on chronological order, specify ordering via GraphQL (if supported) or explicitly sort by createdAt after mapping. Add a test covering two comments with out-of-order timestamps to assert the intended order.
• [NON-BLOCKING] src/domain/repository/github-pr-operations.ts:706 — Stop condition incurs extra page fetch at exact cap; can avoid one network call
  The truncation logic fetches the next page even when allThreads.length has already reached MAX_REVIEW_THREADS, as noted in tests. While acceptable, you can check if (!hasNextPage || allThreads.length >= MAX_REVIEW_THREADS) before issuing the next GraphQL call to spare one request on large PRs. Add a comment or a small optimization.

Inline comments

• src/domain/repository/github-pr-operations.ts:697 — Nit: Consider exporting MAX_REVIEW_THREADS or documenting its rationale (200) alongside ADR-008 reference. If downstream consumers need to present a notice, they may also want the cap value for messaging.

[minsky-reviewer]

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

---

Good progress adding reviewThreads with pagination and DI, plus tests. However, I’m blocking on three issues: (1) the GraphQL response is dereferenced without null-guards, which can throw and violate the non-fatal contract; (2) per-thread comment truncation (comments(first: 10)) isn’t exposed to the session result API, creating silent data loss for long threads; and (3) the session result shape adds required fields without versioning or opt-in, risking downstream breakage. Non-blocking notes: clarify ADR/schema coverage and consider a minor pagination optimization to avoid an extra fetch at the cap. Address the blockers and this should be mergeable.

Findings

• [BLOCKING] src/domain/repository/github-pr-operations.ts:820 — Possible runtime TypeError on null GraphQL subfields — violates non-fatal contract
  After the GraphQL call, the code unconditionally dereferences response.repository.pullRequest.reviewThreads to assign const page = response.repository.pullRequest.reviewThreads;. If the repository or pullRequest node is null (e.g., 403/404, cross-repo permissions, or PR not found in scope), Octokit will return data with null subfields rather than throwing. This will cause a runtime TypeError during property access, escaping your try/catch (which only wraps the graphql call) and breaking the "non-fatal failure" guarantee documented in the function header. Add a null-guard immediately after the call and return the emptyResult when response.repository?.pullRequest?.reviewThreads is absent.
• [BLOCKING] src/domain/repository/github-pr-operations.ts:760 — Silent comment truncation without explicit contract propagation to caller
  The GraphQL query requests comments(first: 10) for each thread (line ~760), but ReviewThread.comments is documented as "Ordered list of comments ... (up to 10)" and a boolean truncatedComments is added. However, the public-facing SessionPrReviewContextResult in src/domain/session/commands/pr-review-context-subcommand.ts does not surface truncatedComments per thread; it only exposes reviewThreads and a top-level reviewThreadsTruncated. Downstream consumers cannot detect per-thread truncation to display a "more comments" notice as your comment implies. Either (a) include truncatedComments in the session result shape or (b) fetch all comments (paginate) so per-thread truncation never occurs. As-is this is a silent behavior change that will mislead reviewers on long threads.
• [BLOCKING] src/domain/session/commands/pr-review-context-subcommand.ts:27 — Session result shape change is not versioned — backwards compatibility risk
  SessionPrReviewContextResult gains two required fields: reviewThreads and reviewThreadsTruncated (lines 27–37). This is a breaking change for any consumer expecting the previous shape. The PR description mentions reviewer prompt changes are out of scope, but adding required fields without a version bump or defaulting in the transport/protocol can break existing clients. Make these fields optional, gate behind a feature flag, or rev the API contract so downstream consumers can roll forward safely.
• [NON-BLOCKING] src/domain/repository/github-pr-operations.ts:742 — Spec/ADR alignment unclear — missing schema link or field coverage (e.g., thread state)
  The PR description cites ADR-008 and GitHub's GraphQL pullRequest.reviewThreads as the canonical source for thread state. The current query does not request fields like isResolved, isOutdated, isCollapsed (good — included), but it omits other potentially relevant state such as isPending or viewerCanResolve/viewerCanUnresolve, and it does not capture per-comment metadata like updatedAt, url, or system notes. If ADR-008 expects full parity with GitHub UI state used later for auto-resolve flows (mt#1345), consider documenting the chosen subset and justifying exclusions in-code with comments and a link to the schema docs to ease future maintenance.
• [NON-BLOCKING] src/domain/repository/github-pr-operations.ts:900 — Pagination loop may perform an unnecessary extra fetch at exactly 200 threads
  When allThreads.length reaches MAX_REVIEW_THREADS (200) during processing of a page, you set truncated = true and hasNextPage = false, but this happens inside the node loop for the already-fetched page. In the common case of exactly 200 total threads (e.g., 4 full pages of 50), you will still have fetched the 4th (final) page even if the 3rd page already completed the cap. A micro-optimization: compute remaining capacity before pushing nodes and early-break without fetching the subsequent page. Tests already mention this, but if API quotas matter, consider short-circuiting earlier to reduce one GraphQL call.

Inline comments

• src/domain/repository/github-pr-operations-review-threads.test.ts:210 — Nice that you called out the extra page fetch in the comment. If you do adopt the earlier short-circuit in production code, please update this assertion to the new expected number of calls (4) and keep the explanatory note in sync.

[edobry]

@minsky-reviewer please re-review — fixes pushed addressing prior round's findings.

[minsky-reviewer]

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

---

The reviewer ran but produced no findings. This is not an approval — the model emitted no submit_finding, submit_inline_comment, or conclude_review calls.