From adfa18251a9a29b4c611cffa23cf357728423f48 Mon Sep 17 00:00:00 2001 From: James Ross Date: Thu, 9 Apr 2026 01:18:59 -0700 Subject: [PATCH 1/4] Implement native review-state query --- ARCHITECTURE.md | 7 +- README.md | 14 +- docs/CLI.md | 5 + docs/MCP.md | 9 + .../review-state-query.md} | 86 ++- docs/method/process.md | 5 +- src/cli-args.ts | 58 +- src/cli.ts | 27 +- src/mcp.ts | 53 +- src/review-state.ts | 667 ++++++++++++++++++ tests/cli.test.ts | 78 +- tests/docs.test.ts | 12 +- tests/mcp.test.ts | 80 ++- tests/review-state.test.ts | 271 +++++++ 14 files changed, 1333 insertions(+), 39 deletions(-) rename docs/{method/backlog/cool-ideas/PROCESS_review-state-query.md => design/0034-review-state-query/review-state-query.md} (64%) create mode 100644 src/review-state.ts create mode 100644 tests/review-state.test.ts diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 8b78b7d..f5f3d13 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -26,9 +26,12 @@ src/ domain.ts drift.ts errors.ts + frontmatter.ts generate.ts index.ts mcp.ts + renderers.ts + review-state.ts ``` @@ -101,7 +104,7 @@ Two-way sync between the local backlog and GitHub Issues: ## Testing -12 test files in `tests/` using Vitest: +13 test files in `tests/` using Vitest: - `api.test.ts` - `cli.test.ts` @@ -113,6 +116,7 @@ Two-way sync between the local backlog and GitHub Issues: - `generate.test.ts` - `github-adapter.test.ts` - `mcp.test.ts` +- `review-state.test.ts` - `ship-sync.test.ts` - `witness.test.ts` @@ -128,5 +132,6 @@ shell command execution during witness capture. - `@flyingrobots/bijou-node` - `@git-stunts/plumbing` - `@modelcontextprotocol/sdk` +- `yaml` - `zod` diff --git a/README.md b/README.md index b5c613d..ea0eb9b 100644 --- a/README.md +++ b/README.md @@ -307,8 +307,10 @@ in one sentence, the cycle is too big. Split it. - Backlog maintenance. Closing the cycle packet does not mean `main` has accepted it yet. - Review-stage visibility is not yet a repo-native METHOD query. - Branch and PR context carry that state for now. + Review-stage visibility now has a repo-native METHOD query: + `method review-state`. It summarizes branch and PR state when forge + context is available; the underlying review work still lives on the + branch and PR. 6. **PR / review** - review the full cycle packet until merge or rejection. @@ -364,6 +366,7 @@ If you can answer these questions by reading the repo, you do not need a standup: - What is actively open in this workspace? -> `method status` +- What is under review? -> `method review-state` - What is committed? -> each design doc names its sponsors and hill - What is next? -> `ls docs/method/backlog/asap/` - What failed and why? -> `ls docs/method/retro/` @@ -372,9 +375,10 @@ a standup: If any of these are unclear, the docs are incomplete. Fix the docs, not the process. -Review state is not yet part of METHOD's repo-native coordination -surface. For now it rides on branch and PR context outside the core -filesystem queries. +Review state now has a repo-native query surface through +`method review-state`. It still reads branch and PR context rather than +the filesystem alone, but that coordination question no longer requires +ad hoc `gh` reconstruction. ### BEARING.md diff --git a/docs/CLI.md b/docs/CLI.md index cfbd891..6f74ad5 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -40,6 +40,11 @@ Show backlog lanes, active cycles, and legend health. Check active cycle playback questions against test descriptions in tests/. First cut scans tests/**/*.test.* and tests/**/*.spec.* only. +### `method review-state [--pr NUMBER | --current-branch] [--json]` + +Query PR review / merge-readiness state for the current branch or an explicit PR. +Defaults to --current-branch when no selector flag is provided. + ### `method mcp` Start an MCP (Model Context Protocol) server on stdio. diff --git a/docs/MCP.md b/docs/MCP.md index 7b191ce..320b873 100644 --- a/docs/MCP.md +++ b/docs/MCP.md @@ -21,6 +21,15 @@ workspace-agnostic — a single instance can serve multiple projects. ## Tools +### `method_review_state` + +Get PR review / merge-readiness state for the current branch or an explicit PR + +**Parameters:** + +- `pr` (optional) `number` — Explicit PR number to inspect +- `currentBranch` (optional) `boolean` — Resolve the PR from the current branch (default when pr is omitted) + ### `method_status` Get the current status of the METHOD workspace (backlog lanes, active cycles, legend health) diff --git a/docs/method/backlog/cool-ideas/PROCESS_review-state-query.md b/docs/design/0034-review-state-query/review-state-query.md similarity index 64% rename from docs/method/backlog/cool-ideas/PROCESS_review-state-query.md rename to docs/design/0034-review-state-query/review-state-query.md index 1546300..16dfe49 100644 --- a/docs/method/backlog/cool-ideas/PROCESS_review-state-query.md +++ b/docs/design/0034-review-state-query/review-state-query.md @@ -1,19 +1,85 @@ --- title: "Review State Query" -legend: PROCESS -lane: cool-ideas -owner: "METHOD maintainers" -priority: medium -acceptance_criteria: - - "The review-state surface reports CI/check status, unresolved review-thread count, review decision, and bot-review pending state for the active PR." - - "The output contract is explicit for both CLI and MCP: `method review-state` supports `--json`, `method_review_state` returns the standard structured-content envelope, and both share the same result schema." - - "The output includes a deterministic merge_ready boolean plus a canonical blocker schema naming exactly which states still prevent merge." - - "Selector precedence is explicit: `method review-state` defaults to `--current-branch`, rejects `--pr` with `--current-branch`, and defines deterministic no-PR and ambiguous-PR outputs." - - "Humans and agents can answer 'is this PR actually mergeable yet?' without manually reconstructing GitHub thread/check state." +legend: "PROCESS" +cycle: "0034-review-state-query" +source_backlog: "docs/method/backlog/cool-ideas/PROCESS_review-state-query.md" --- # Review State Query +Source backlog item: `docs/method/backlog/cool-ideas/PROCESS_review-state-query.md` +Legend: PROCESS + +## Sponsors + +- Human: Backlog operator +- Agent: Implementation agent + +## Hill + +Land a METHOD-native review-state surface that answers "what is under +review and is it merge-ready?" without forcing humans or agents to +reconstruct `gh` state ad hoc. The first cut must ship one shared +engine behind both CLI and MCP, with deterministic selector behavior, +structured blockers, and explicit handling for unresolved threads, +checks, sticky review decisions, and observable bot cooldowns. + +## Playback Questions + +### Human + +- [ ] Can I run `method review-state` on the current branch or an + explicit PR and get a bounded summary of blockers and + merge-readiness? +- [ ] When no PR exists or multiple PRs match the branch, do I get a + deterministic result instead of a vague shell failure? + +### Agent + +- [ ] Does the CLI `--json` output exactly match the MCP + `structuredContent.result` contract? +- [ ] Do unresolved review threads remain the primary blockers when + `reviewDecision` is still `CHANGES_REQUESTED`? +- [ ] Does the result surface explicit bot cooldown / review-in-progress + states only when they are observable in GitHub data? + +## Accessibility and Assistive Reading + +- Linear truth / reduced-complexity posture: + default CLI output must stay short, ordered, and label-driven so a + human can scan it top-to-bottom without parsing raw JSON or nested + GitHub state. +- Non-visual or alternate-reading expectations: + JSON mode and MCP output must carry the full machine contract so + screen-reader or agent callers do not lose fidelity compared with the + prose summary. + +## Localization and Directionality + +- Locale / wording / formatting assumptions: + the first cut is English-only and preserves GitHub enum vocabulary + (`APPROVED`, `CHANGES_REQUESTED`, `REVIEW_REQUIRED`) rather than + inventing localized labels. +- Logical direction / layout assumptions: + output is plain left-to-right text / JSON with no layout-specific + affordances. + +## Agent Inspectability and Explainability + +- What must be explicit and deterministic for agents: + selector rules, result field presence, blocker types, and null/zero + behavior for `no-pr` / `ambiguous-pr` cases must be fixed and tested. +- What must be attributable, evidenced, or governed: + cooldown blockers must only appear when the latest observable bot + signal actually contains a rate-limit message; the engine must not + invent hidden policy state. + +## Non-goals + +- [ ] TBD + +## Backlog Context + METHOD currently says review state is not part of the repo-native coordination surface. A dedicated query or signpost that summarizes what is under review, awaiting merge, or blocked in PR state would make the diff --git a/docs/method/process.md b/docs/method/process.md index 5000d2d..c9b510a 100644 --- a/docs/method/process.md +++ b/docs/method/process.md @@ -18,8 +18,9 @@ METHOD cycles run as a calm pull-design-test-playback-close-review-ship-sync loo boundaries or on a dedicated triage branch. - Repo-level ship surfaces such as `BEARING.md` and `CHANGELOG.md` reflect merged `main` state, not branch-local closeout state. -- Review visibility is currently outside METHOD's repo-native - coordination surface; branch and PR context carry it for now. +- Review visibility is available through `method review-state`, which + summarizes branch and PR context without turning METHOD into a forge + cockpit. - All cycle work must be done on a branch named `cycles/####-slug`. - Once a full cycle is complete (after the retro), the operator must push the branch and open a PR to the target branch (usually `main`). diff --git a/src/cli-args.ts b/src/cli-args.ts index d55971e..7f54086 100644 --- a/src/cli-args.ts +++ b/src/cli-args.ts @@ -8,6 +8,7 @@ export type ParsedCommand = | { command: 'pull'; item: string } | { command: 'close'; cycle?: string; driftCheck?: 'yes' | 'no'; outcome: Outcome } | { command: 'drift'; cycle?: string } + | { command: 'review-state'; pr?: number; currentBranch?: boolean; json?: boolean } | { command: 'status' } | { command: 'mcp' } | { command: 'sync'; adapter: 'github'; push?: boolean; pull?: boolean } @@ -41,6 +42,8 @@ export function parseCliArgs(argv: readonly string[]): ParsedCommand { return parseCloseArgs(rest); case 'drift': return parseDriftArgs(rest); + case 'review-state': + return parseReviewStateArgs(rest); case 'status': if (rest.length > 0) { throw new MethodError('`status` does not take any arguments.'); @@ -77,7 +80,7 @@ export function parseCliArgs(argv: readonly string[]): ParsedCommand { } } -export const CLI_TOPICS = ['init', 'inbox', 'pull', 'close', 'status', 'drift', 'mcp', 'sync'] as const; +export const CLI_TOPICS = ['init', 'inbox', 'pull', 'close', 'status', 'drift', 'review-state', 'mcp', 'sync'] as const; export function usage(topic?: string): string { if (topic === 'init') { @@ -104,6 +107,15 @@ export function usage(topic?: string): string { ].join('\n'); } + if (topic === 'review-state') { + return [ + 'Usage: method review-state [--pr NUMBER | --current-branch] [--json]', + '', + 'Query PR review / merge-readiness state for the current branch or an explicit PR.', + 'Defaults to --current-branch when no selector flag is provided.', + ].join('\n'); + } + if (topic === 'mcp') { return 'Usage: method mcp\n\nStart an MCP (Model Context Protocol) server on stdio.'; } @@ -129,6 +141,7 @@ export function usage(topic?: string): string { ' pull Promote a backlog item into a cycle.', ' close [cycle] Write a retro for an active cycle.', ' drift [cycle] Check active cycle playback questions against tests.', + ' review-state Query PR review / merge-readiness state.', ' status Show backlog, active cycles, and legend health.', ' mcp Start the MCP server over stdio.', ' sync github [--push|--pull] Sync backlog with GitHub Issues.', @@ -238,6 +251,41 @@ function parseDriftArgs(args: readonly string[]): ParsedCommand { return { command: 'drift', cycle: args[0] }; } +function parseReviewStateArgs(args: readonly string[]): ParsedCommand { + let pr: number | undefined; + let currentBranch: boolean | undefined; + let json = false; + + for (let index = 0; index < args.length; index += 1) { + const value = args[index]; + if (value === '--json') { + json = true; + continue; + } + if (value === '--current-branch') { + currentBranch = true; + continue; + } + if (value === '--pr') { + const rawValue = requireOptionValue(args, index, '--pr'); + pr = parsePositiveInteger(rawValue, '--pr'); + index += 1; + continue; + } + if (value.startsWith('--pr=')) { + pr = parsePositiveInteger(value.slice('--pr='.length), '--pr'); + continue; + } + throw new MethodError(`Unknown option: ${value}`); + } + + if (pr !== undefined && currentBranch) { + throw new MethodError('`review-state` accepts either `--pr` or `--current-branch`, not both.'); + } + + return { command: 'review-state', pr, currentBranch, json }; +} + function requireOptionValue(args: readonly string[], index: number, optionName: string): string { const value = args[index + 1]; if (value === undefined || value.startsWith('-')) { @@ -259,3 +307,11 @@ function parseOutcomeValue(value: string): Outcome { } throw new MethodError('`--outcome` must be `hill-met`, `partial`, or `not-met`.'); } + +function parsePositiveInteger(value: string, optionName: string): number { + const parsed = Number(value); + if (!Number.isInteger(parsed) || parsed <= 0) { + throw new MethodError(`${optionName} must be a positive integer.`); + } + return parsed; +} diff --git a/src/cli.ts b/src/cli.ts index 8a9861b..1eb1931 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -1,5 +1,4 @@ #!/usr/bin/env node - import { realpathSync } from 'node:fs'; import { relative, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; @@ -12,17 +11,16 @@ import { initWorkspace, Workspace } from './index.js'; import { loadConfig } from './config.js'; import { createMcpServer } from './mcp.js'; import { GitHubAdapter } from './adapters/github.js'; - +import { queryReviewState, renderReviewStateText, type ReviewStateQueryOptions, type ReviewStateResult } from './review-state.js'; type Writer = Pick; type ConfirmPrompt = (options: { title: string; defaultValue: boolean }) => Promise; - export interface RunCliOptions { cwd?: string; stdout?: Writer; stderr?: Writer; confirmPrompt?: ConfirmPrompt; + reviewStateQuery?: (options: ReviewStateQueryOptions) => Promise; } - export async function runCli( argv: readonly string[], options: RunCliOptions = {}, @@ -33,7 +31,6 @@ export async function runCli( const ctx = createNodeContext(); const promptConfirm = options.confirmPrompt ?? ((promptOptions) => confirm({ title: promptOptions.title, defaultValue: promptOptions.defaultValue, ctx })); - try { const parsed = parseCliArgs(argv); if (parsed.command === 'help') { @@ -87,8 +84,23 @@ export async function runCli( return report.exitCode; } + if (parsed.command === 'review-state') { + const reviewStateQuery = options.reviewStateQuery ?? queryReviewState; + const result = await reviewStateQuery({ + cwd: root, + pr: parsed.pr, + currentBranch: parsed.currentBranch, + }); + if (parsed.json) { + stdout.write(`${JSON.stringify(result, null, 2)}\n`); + } else { + stdout.write(`${renderReviewStateText(result)}\n`); + } + return 0; + } + if (parsed.command === 'mcp') { - const server = createMcpServer(); + const server = createMcpServer({ reviewStateQuery: options.reviewStateQuery ?? queryReviewState }); const transport = new StdioServerTransport(); await server.connect(transport); // Let it run indefinitely @@ -163,7 +175,6 @@ export async function runCli( return 0; } } - const status = workspace.status(); stdout.write(renderStatus(status)); return 0; @@ -173,11 +184,9 @@ export async function runCli( return 1; } } - export async function main(argv: readonly string[] = process.argv.slice(2)): Promise { return runCli(argv); } - if (process.argv[1] !== undefined && fileURLToPath(import.meta.url) === realpathSync(resolve(process.argv[1]))) { main().then((code) => { process.exitCode = code; diff --git a/src/mcp.ts b/src/mcp.ts index df3cbd8..7cd60cf 100644 --- a/src/mcp.ts +++ b/src/mcp.ts @@ -4,6 +4,7 @@ import { basename, relative } from 'node:path'; import { Workspace } from './index.js'; import { GitHubAdapter, type GitHubSyncResult } from './adapters/github.js'; import type { Cycle, Outcome, WorkspaceStatus } from './domain.js'; +import { queryReviewState, renderReviewStateText, type ReviewStateQueryOptions } from './review-state.js'; const workspaceProperty = { workspace: { type: 'string' as const, description: 'Absolute path to the METHOD workspace root directory' } }; @@ -26,6 +27,19 @@ export interface McpToolDef { } export const MCP_TOOLS: McpToolDef[] = [ + { + name: 'method_review_state', + description: 'Get PR review / merge-readiness state for the current branch or an explicit PR', + inputSchema: { + type: 'object', + properties: { + ...workspaceProperty, + pr: { type: 'number', description: 'Explicit PR number to inspect' }, + currentBranch: { type: 'boolean', description: 'Resolve the PR from the current branch (default when pr is omitted)' }, + }, + required: ['workspace'], + }, + }, { name: 'method_status', description: 'Get the current status of the METHOD workspace (backlog lanes, active cycles, legend health)', @@ -96,7 +110,11 @@ export const MCP_TOOLS: McpToolDef[] = [ }, ]; -export function createMcpServer() { +export interface CreateMcpServerOptions { + reviewStateQuery?: (options: ReviewStateQueryOptions) => Promise>>; +} + +export function createMcpServer(options: CreateMcpServerOptions = {}) { const server = new Server( { name: 'method', version: '0.3.0' }, { capabilities: { tools: {} } } @@ -117,6 +135,25 @@ export function createMcpServer() { const workspace = new Workspace(workspacePath); workspace.ensureInitialized(); + if (request.params.name === 'method_review_state') { + const pr = validateOptionalInteger(args.pr, 'pr'); + const currentBranch = validateOptionalBoolean(args.currentBranch, 'currentBranch'); + if (pr !== undefined && currentBranch === true) { + throw new Error('method_review_state accepts either pr or currentBranch, not both.'); + } + const reviewStateQuery = options.reviewStateQuery ?? queryReviewState; + const result = await reviewStateQuery({ + cwd: workspace.root, + pr, + currentBranch, + }); + return successResult( + 'method_review_state', + renderReviewStateText(result), + result, + ); + } + if (request.params.name === 'method_status') { const status = workspace.status(); const summary = validateOptionalBoolean(args.summary, 'summary') ?? false; @@ -362,7 +399,7 @@ function summarizeGitHubSync(pushResults: GitHubSyncResult[], pullResults: GitHu return `GitHub sync processed ${changed.length} item(s) with ${failures} error(s).`; } -function successResult(tool: string, text: string, result: Record) { +function successResult(tool: string, text: string, result: unknown) { return toolResult(tool, text, result, false); } @@ -378,7 +415,7 @@ function errorResult(tool: string, message: string) { }; } -function toolResult(tool: string, text: string, result: Record, isError: boolean) { +function toolResult(tool: string, text: string, result: unknown, isError: boolean) { return { content: [{ type: 'text' as const, text }], structuredContent: { @@ -444,6 +481,16 @@ function validateBoolean(value: unknown, name: string): boolean { return value; } +function validateOptionalInteger(value: unknown, name: string): number | undefined { + if (value === undefined) { + return undefined; + } + if (typeof value !== 'number' || !Number.isInteger(value) || value <= 0) { + throw new Error(`${name} must be a positive integer.`); + } + return value; +} + function validateOutcome(value: unknown): Outcome { if (value === 'hill-met' || value === 'partial' || value === 'not-met') { return value; diff --git a/src/review-state.ts b/src/review-state.ts new file mode 100644 index 0000000..6e119b1 --- /dev/null +++ b/src/review-state.ts @@ -0,0 +1,667 @@ +import { execFile } from 'node:child_process'; +import { promisify } from 'node:util'; + +const execFileAsync = promisify(execFile); + +const REVIEW_STATE_STATUSES = ['ready', 'blocked', 'no-pr', 'ambiguous-pr'] as const; +const REVIEW_DECISIONS = ['APPROVED', 'CHANGES_REQUESTED', 'REVIEW_REQUIRED', 'NONE'] as const; +const BOT_REVIEW_STATES = ['none', 'pending', 'approved', 'changes_requested', 'commented'] as const; +const BLOCKER_TYPES = [ + 'selection', + 'review_decision', + 'unresolved_threads', + 'pending_checks', + 'failing_checks', + 'bot_review', + 'policy_cooldown', +] as const; + +type ReviewStateStatus = typeof REVIEW_STATE_STATUSES[number]; +type ReviewDecision = typeof REVIEW_DECISIONS[number]; +type BotReviewState = typeof BOT_REVIEW_STATES[number]; +type ReviewStateBlockerType = typeof BLOCKER_TYPES[number]; + +export interface ReviewCheck { + name: string; + status: string; + url: string | null; +} + +export interface ReviewStateBlocker { + type: ReviewStateBlockerType; + message: string; + source: string; +} + +export interface ReviewStateResult { + status: ReviewStateStatus; + pr_number: number | null; + pr_url: string | null; + review_decision: ReviewDecision; + unresolved_thread_count: number; + checks: { + passing: ReviewCheck[]; + pending: ReviewCheck[]; + failing: ReviewCheck[]; + }; + bot_review_state: BotReviewState; + approval_count: number; + changes_requested_count: number; + merge_ready: boolean; + blockers: ReviewStateBlocker[]; +} + +export interface ReviewStateQueryOptions { + cwd: string; + pr?: number; + currentBranch?: boolean; + client?: ReviewStateClient; +} + +export interface ReviewStateClient { + ghJson(cwd: string, args: readonly string[]): Promise; + gitText(cwd: string, args: readonly string[]): Promise; +} + +interface PullRequestCandidate { + number: number; + url?: string | null; +} + +interface GhOwner { + login?: string | null; +} + +interface GhRepoViewResponse { + owner?: GhOwner | null; + name?: string | null; +} + +interface GhCheckRun { + __typename?: string | null; + completedAt?: string | null; + conclusion?: string | null; + context?: string | null; + detailsUrl?: string | null; + name?: string | null; + startedAt?: string | null; + state?: string | null; + status?: string | null; + targetUrl?: string | null; + workflowName?: string | null; +} + +interface GhReviewAuthor { + login?: string | null; +} + +interface GhReview { + author?: GhReviewAuthor | null; + body?: string | null; + state?: string | null; + submittedAt?: string | null; +} + +interface GhComment { + author?: GhReviewAuthor | null; + body?: string | null; + createdAt?: string | null; + updatedAt?: string | null; +} + +interface GhPrViewResponse { + number?: number | null; + url?: string | null; + reviewDecision?: string | null; + statusCheckRollup?: GhCheckRun[] | null; + reviews?: GhReview[] | null; + comments?: GhComment[] | null; +} + +interface ReviewThreadsQueryResponse { + data?: { + repository?: { + pullRequest?: { + reviewThreads?: { + pageInfo?: { + hasNextPage?: boolean | null; + endCursor?: string | null; + } | null; + nodes?: Array<{ + isResolved?: boolean | null; + isOutdated?: boolean | null; + }> | null; + } | null; + } | null; + } | null; + } | null; +} + +interface TimedSignal { + source: string; + timestamp: number; + body?: string; +} + +interface BotReviewSignal extends TimedSignal { + state?: string; +} + +interface BotCheckSignal extends TimedSignal { + status: string; +} + +export async function queryReviewState(options: ReviewStateQueryOptions): Promise { + const pr = options.pr; + const currentBranch = options.currentBranch ?? pr === undefined; + if (pr !== undefined && currentBranch) { + throw new Error('review-state accepts either --pr or --current-branch, not both.'); + } + + const client = options.client ?? createExecReviewStateClient(); + const selection = pr === undefined + ? await resolveCurrentBranchPr(options.cwd, client) + : { kind: 'selected' as const, prNumber: pr }; + + if (selection.kind === 'no-pr') { + return selectionResult('no-pr', selection.message); + } + if (selection.kind === 'ambiguous-pr') { + return selectionResult('ambiguous-pr', selection.message); + } + + const [repo, prView] = await Promise.all([ + client.ghJson(options.cwd, ['repo', 'view', '--json', 'owner,name']), + client.ghJson( + options.cwd, + ['pr', 'view', String(selection.prNumber), '--json', 'number,url,reviewDecision,statusCheckRollup,reviews,comments'], + ), + ]); + + const owner = repo.owner?.login?.trim(); + const name = repo.name?.trim(); + if (!owner || !name) { + throw new Error('Could not resolve GitHub repository owner/name from gh repo view.'); + } + + const unresolvedThreadCount = await fetchUnresolvedThreadCount(options.cwd, client, owner, name, selection.prNumber); + return buildReviewStateResult({ + prNumber: requirePositiveInteger(prView.number, 'GitHub PR number'), + prUrl: normalizeOptionalString(prView.url), + reviewDecision: normalizeReviewDecision(prView.reviewDecision), + unresolvedThreadCount, + statusChecks: prView.statusCheckRollup ?? [], + reviews: prView.reviews ?? [], + comments: prView.comments ?? [], + }); +} + +export function renderReviewStateText(result: ReviewStateResult): string { + const lines = [ + `Review state: ${result.status}`, + result.pr_number === null ? 'PR: -' : `PR: #${result.pr_number} ${result.pr_url ?? '-'}`, + `Review decision: ${result.review_decision}`, + `Unresolved threads: ${result.unresolved_thread_count}`, + `Checks: passing=${result.checks.passing.length} pending=${result.checks.pending.length} failing=${result.checks.failing.length}`, + `Bot review: ${result.bot_review_state}`, + `Approvals: ${result.approval_count}`, + `Changes requested: ${result.changes_requested_count}`, + `Merge ready: ${result.merge_ready ? 'yes' : 'no'}`, + ]; + + if (result.blockers.length === 0) { + lines.push('Blockers: none'); + return lines.join('\n'); + } + + lines.push('Blockers:'); + for (const blocker of result.blockers) { + lines.push(`- ${blocker.type} [${blocker.source}] ${blocker.message}`); + } + return lines.join('\n'); +} + +export function createExecReviewStateClient(): ReviewStateClient { + return { + async ghJson(cwd: string, args: readonly string[]): Promise { + const stdout = await runCommand('gh', args, cwd); + return JSON.parse(stdout) as T; + }, + async gitText(cwd: string, args: readonly string[]): Promise { + return runCommand('git', args, cwd); + }, + }; +} + +async function runCommand(command: string, args: readonly string[], cwd: string): Promise { + try { + const { stdout } = await execFileAsync(command, [...args], { + cwd, + encoding: 'utf8', + timeout: 30_000, + env: { ...process.env, GH_PAGER: 'cat' }, + }); + return stdout; + } catch (error: unknown) { + const stdout = typeof error === 'object' && error !== null && 'stdout' in error ? String((error as { stdout?: string }).stdout ?? '') : ''; + const stderr = typeof error === 'object' && error !== null && 'stderr' in error ? String((error as { stderr?: string }).stderr ?? '') : ''; + const message = [stdout.trim(), stderr.trim()].filter((value) => value.length > 0).join('\n'); + throw new Error(message.length > 0 ? message : `${command} ${args.join(' ')} failed.`); + } +} + +async function resolveCurrentBranchPr( + cwd: string, + client: ReviewStateClient, +): Promise< + | { kind: 'selected'; prNumber: number } + | { kind: 'no-pr'; message: string } + | { kind: 'ambiguous-pr'; message: string } +> { + const branch = (await client.gitText(cwd, ['rev-parse', '--abbrev-ref', 'HEAD'])).trim(); + if (branch.length === 0 || branch === 'HEAD') { + return { kind: 'no-pr', message: 'No PR found for current branch (detached HEAD).' }; + } + + const matches = await client.ghJson( + cwd, + ['pr', 'list', '--head', branch, '--state', 'open', '--json', 'number,url'], + ); + + if (matches.length === 0) { + return { kind: 'no-pr', message: `No PR found for current branch: ${branch}.` }; + } + if (matches.length > 1) { + return { + kind: 'ambiguous-pr', + message: `Multiple PRs match current branch (${branch}); rerun with --pr .`, + }; + } + + return { kind: 'selected', prNumber: requirePositiveInteger(matches[0]?.number, 'GitHub PR number') }; +} + +async function fetchUnresolvedThreadCount( + cwd: string, + client: ReviewStateClient, + owner: string, + repo: string, + prNumber: number, +): Promise { + const query = [ + 'query($owner: String!, $repo: String!, $pr: Int!, $cursor: String) {', + ' repository(owner: $owner, name: $repo) {', + ' pullRequest(number: $pr) {', + ' reviewThreads(first: 100, after: $cursor) {', + ' pageInfo { hasNextPage endCursor }', + ' nodes { isResolved isOutdated }', + ' }', + ' }', + ' }', + '}', + ].join('\n'); + + let total = 0; + let cursor: string | undefined; + while (true) { + const args = [ + 'api', + 'graphql', + '-F', + `owner=${owner}`, + '-F', + `repo=${repo}`, + '-F', + `pr=${prNumber}`, + '-f', + `query=${query}`, + ]; + if (cursor !== undefined) { + args.push('-F', `cursor=${cursor}`); + } + + const response = await client.ghJson(cwd, args); + const reviewThreads = response.data?.repository?.pullRequest?.reviewThreads; + const nodes = reviewThreads?.nodes ?? []; + total += nodes.filter((thread) => thread.isResolved !== true && thread.isOutdated !== true).length; + + const nextCursor = reviewThreads?.pageInfo?.endCursor ?? undefined; + if (reviewThreads?.pageInfo?.hasNextPage === true && nextCursor !== undefined && nextCursor !== cursor) { + cursor = nextCursor; + continue; + } + break; + } + return total; +} + +function buildReviewStateResult(input: { + prNumber: number; + prUrl: string | undefined; + reviewDecision: ReviewDecision; + unresolvedThreadCount: number; + statusChecks: GhCheckRun[]; + reviews: GhReview[]; + comments: GhComment[]; +}): ReviewStateResult { + const checks = classifyChecks(input.statusChecks); + const humanDecisions = latestDecisiveStates(input.reviews.filter((review) => !isBotLogin(review.author?.login))); + const approvalCount = [...humanDecisions.values()].filter((state) => state === 'APPROVED').length; + const changesRequestedCount = [...humanDecisions.values()].filter((state) => state === 'CHANGES_REQUESTED').length; + const botSignals = analyzeBotSignals(input.reviews, input.comments, input.statusChecks); + + const blockers: ReviewStateBlocker[] = []; + if (input.unresolvedThreadCount > 0) { + blockers.push({ + type: 'unresolved_threads', + message: `${input.unresolvedThreadCount} unresolved review thread${input.unresolvedThreadCount === 1 ? '' : 's'}.`, + source: 'github', + }); + } + if (checks.failing.length > 0) { + blockers.push({ + type: 'failing_checks', + message: `${checks.failing.length} failing check${checks.failing.length === 1 ? '' : 's'}.`, + source: 'github', + }); + } + if (checks.pending.length > 0) { + blockers.push({ + type: 'pending_checks', + message: `${checks.pending.length} pending check${checks.pending.length === 1 ? '' : 's'}.`, + source: 'github', + }); + } + if (input.reviewDecision === 'REVIEW_REQUIRED') { + blockers.push({ + type: 'review_decision', + message: 'reviewDecision is REVIEW_REQUIRED.', + source: 'github', + }); + } else if (input.reviewDecision === 'CHANGES_REQUESTED' && input.unresolvedThreadCount === 0) { + blockers.push({ + type: 'review_decision', + message: 'reviewDecision still CHANGES_REQUESTED.', + source: 'github', + }); + } + if (botSignals.cooldownMessage !== undefined) { + blockers.push({ + type: 'policy_cooldown', + message: botSignals.cooldownMessage, + source: botSignals.source, + }); + } else if (botSignals.state === 'pending') { + blockers.push({ + type: 'bot_review', + message: `${displayBotSource(botSignals.source)} review in progress.`, + source: botSignals.source, + }); + } else if (botSignals.state === 'changes_requested') { + blockers.push({ + type: 'bot_review', + message: `${displayBotSource(botSignals.source)} requested changes.`, + source: botSignals.source, + }); + } + + const mergeReady = blockers.length === 0; + return { + status: mergeReady ? 'ready' : 'blocked', + pr_number: input.prNumber, + pr_url: input.prUrl ?? null, + review_decision: input.reviewDecision, + unresolved_thread_count: input.unresolvedThreadCount, + checks, + bot_review_state: botSignals.state, + approval_count: approvalCount, + changes_requested_count: changesRequestedCount, + merge_ready: mergeReady, + blockers, + }; +} + +function selectionResult(status: Extract, message: string): ReviewStateResult { + return { + status, + pr_number: null, + pr_url: null, + review_decision: 'NONE', + unresolved_thread_count: 0, + checks: { passing: [], pending: [], failing: [] }, + bot_review_state: 'none', + approval_count: 0, + changes_requested_count: 0, + merge_ready: false, + blockers: [{ type: 'selection', message, source: 'github' }], + }; +} + +function classifyChecks(items: GhCheckRun[]): ReviewStateResult['checks'] { + const passing: ReviewCheck[] = []; + const pending: ReviewCheck[] = []; + const failing: ReviewCheck[] = []; + + for (const item of items) { + const check = toReviewCheck(item); + if (isPendingCheck(item)) { + pending.push(check); + continue; + } + if (isFailingCheck(item)) { + failing.push(check); + continue; + } + if (check.status.length > 0) { + passing.push(check); + } + } + + return { passing, pending, failing }; +} + +function toReviewCheck(item: GhCheckRun): ReviewCheck { + return { + name: normalizeCheckName(item), + status: normalizeCheckStatus(item), + url: normalizeOptionalString(item.detailsUrl) ?? normalizeOptionalString(item.targetUrl) ?? null, + }; +} + +function normalizeCheckName(item: GhCheckRun): string { + const base = normalizeOptionalString(item.name) ?? normalizeOptionalString(item.context) ?? 'unknown'; + const workflow = normalizeOptionalString(item.workflowName); + if (workflow !== undefined && workflow !== base) { + return `${workflow} / ${base}`; + } + return base; +} + +function normalizeCheckStatus(item: GhCheckRun): string { + if (item.__typename === 'CheckRun') { + if (normalizeOptionalString(item.status)?.toUpperCase() !== 'COMPLETED') { + return normalizeOptionalString(item.status)?.toUpperCase() ?? 'UNKNOWN'; + } + return normalizeOptionalString(item.conclusion)?.toUpperCase() ?? 'COMPLETED'; + } + return normalizeOptionalString(item.state)?.toUpperCase() ?? 'UNKNOWN'; +} + +function isPendingCheck(item: GhCheckRun): boolean { + const status = normalizeCheckStatus(item); + return ['PENDING', 'IN_PROGRESS', 'QUEUED', 'EXPECTED', 'WAITING', 'REQUESTED'].includes(status); +} + +function isFailingCheck(item: GhCheckRun): boolean { + const status = normalizeCheckStatus(item); + return ['FAILURE', 'FAILED', 'ERROR', 'CANCELLED', 'ACTION_REQUIRED', 'TIMED_OUT', 'STALE', 'STARTUP_FAILURE'].includes(status); +} + +function latestDecisiveStates(reviews: GhReview[]): Map { + const latest = new Map(); + for (const review of reviews) { + const author = normalizeOptionalString(review.author?.login); + const state = normalizeOptionalString(review.state)?.toUpperCase(); + if (author === undefined || (state !== 'APPROVED' && state !== 'CHANGES_REQUESTED')) { + continue; + } + const timestamp = parseTimestamp(review.submittedAt); + const previous = latest.get(author); + if (previous === undefined || timestamp >= previous.timestamp) { + latest.set(author, { timestamp, state }); + } + } + return new Map([...latest.entries()].map(([author, value]) => [author, value.state])); +} + +function analyzeBotSignals(reviews: GhReview[], comments: GhComment[], checks: GhCheckRun[]): { + state: BotReviewState; + source: string; + cooldownMessage?: string; +} { + const botReviews: BotReviewSignal[] = []; + for (const review of reviews) { + const source = normalizeOptionalString(review.author?.login); + if (source === undefined || !isBotLogin(source)) { + continue; + } + botReviews.push({ + source, + state: normalizeOptionalString(review.state)?.toUpperCase(), + body: normalizeOptionalString(review.body), + timestamp: parseTimestamp(review.submittedAt), + }); + } + + const botComments: TimedSignal[] = []; + for (const comment of comments) { + const source = normalizeOptionalString(comment.author?.login); + if (source === undefined || !isBotLogin(source)) { + continue; + } + botComments.push({ + source, + body: normalizeOptionalString(comment.body), + timestamp: Math.max(parseTimestamp(comment.createdAt), parseTimestamp(comment.updatedAt)), + }); + } + + const botChecks: BotCheckSignal[] = []; + for (const check of checks) { + const source = normalizeOptionalString(check.context) ?? normalizeOptionalString(check.name); + if (source === undefined || !isBotCheck(source)) { + continue; + } + botChecks.push({ + source, + status: normalizeCheckStatus(check), + timestamp: Math.max(parseTimestamp(check.startedAt), parseTimestamp(check.completedAt)), + }); + } + + const latestBotComment = latestTimedSignal(botComments); + const latestBotReview = latestTimedSignal(botReviews); + const latestBotCheck = latestTimedSignal(botChecks); + const latestSignal = latestTimedSignal([...botComments, ...botReviews, ...botChecks]); + const cooldownMessage = latestSignal?.body === undefined ? undefined : extractCooldownMessage(latestSignal.body); + const latestHumanMeaningfulTimestamp = Math.max(latestBotReview?.timestamp ?? 0, latestBotComment?.timestamp ?? 0); + + if (cooldownMessage !== undefined) { + return { + state: 'commented', + source: latestSignal?.source ?? 'bot', + cooldownMessage, + }; + } + + if (latestBotCheck !== undefined && isPendingBotStatus(latestBotCheck.status) && latestBotCheck.timestamp >= latestHumanMeaningfulTimestamp) { + return { state: 'pending', source: latestBotCheck.source }; + } + + if (latestBotReview?.state === 'APPROVED') { + return { state: 'approved', source: latestBotReview.source }; + } + if (latestBotReview?.state === 'CHANGES_REQUESTED') { + return { state: 'changes_requested', source: latestBotReview.source }; + } + if (latestBotCheck !== undefined && isSuccessfulBotStatus(latestBotCheck.status) && latestBotCheck.timestamp >= latestHumanMeaningfulTimestamp) { + return { state: 'approved', source: latestBotCheck.source }; + } + if (latestSignal !== undefined) { + return { state: 'commented', source: latestSignal.source }; + } + return { state: 'none', source: 'bot' }; +} + +function latestTimedSignal(signals: T[]): T | undefined { + return [...signals].sort((left, right) => right.timestamp - left.timestamp)[0]; +} + +function isPendingBotStatus(status: string): boolean { + return ['PENDING', 'IN_PROGRESS', 'QUEUED', 'EXPECTED', 'WAITING', 'REQUESTED'].includes(status); +} + +function isSuccessfulBotStatus(status: string): boolean { + return ['SUCCESS', 'COMPLETED', 'NEUTRAL', 'SKIPPED'].includes(status); +} + +function extractCooldownMessage(body: string): string | undefined { + if (!/rate limit exceeded/iu.test(body)) { + return undefined; + } + const waitMatch = /please wait \*\*(.+?)\*\* before requesting another review/iu.exec(body); + if (waitMatch?.[1] !== undefined) { + return `Review cooldown active: wait ${waitMatch[1]} before requesting another review.`; + } + return 'Review cooldown active due to an explicit bot rate-limit message.'; +} + +function displayBotSource(source: string): string { + if (source.toLowerCase() === 'coderabbitai' || source.toLowerCase() === 'coderabbit') { + return 'CodeRabbit'; + } + return source; +} + +function isBotLogin(login: string | null | undefined): boolean { + if (login === undefined || login === null) { + return false; + } + return login === 'coderabbitai' || /\[bot\]$/iu.test(login) || /\bbot\b/iu.test(login); +} + +function isBotCheck(name: string): boolean { + return /coderabbit/iu.test(name) || /\bbot\b/iu.test(name); +} + +function normalizeReviewDecision(value: string | null | undefined): ReviewDecision { + const candidate = normalizeOptionalString(value)?.toUpperCase(); + if (candidate === 'APPROVED' || candidate === 'CHANGES_REQUESTED' || candidate === 'REVIEW_REQUIRED') { + return candidate; + } + return 'NONE'; +} + +function normalizeOptionalString(value: unknown): string | undefined { + if (typeof value !== 'string') { + return undefined; + } + const normalized = value.trim(); + return normalized.length === 0 ? undefined : normalized; +} + +function parseTimestamp(value: string | null | undefined): number { + const normalized = normalizeOptionalString(value); + if (normalized === undefined) { + return 0; + } + const parsed = Date.parse(normalized); + return Number.isNaN(parsed) ? 0 : parsed; +} + +function requirePositiveInteger(value: unknown, label: string): number { + if (typeof value !== 'number' || !Number.isInteger(value) || value <= 0) { + throw new Error(`${label} must be a positive integer.`); + } + return value; +} diff --git a/tests/cli.test.ts b/tests/cli.test.ts index b28ed55..5af3b42 100644 --- a/tests/cli.test.ts +++ b/tests/cli.test.ts @@ -1,7 +1,7 @@ import { existsSync, mkdtempSync, mkdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; import { dirname, join } from 'node:path'; import { tmpdir } from 'node:os'; -import { afterEach, describe, expect, it } from 'vitest'; +import { afterEach, describe, expect, it, vi } from 'vitest'; import { runCli } from '../src/cli.js'; class MemoryWriter { @@ -269,6 +269,82 @@ describe('method CLI', () => { expect(stdout.output).toContain('bad-code 0 -'); }); + it('queries review-state with a shared engine result and renders human-readable output', async () => { + const root = createTempRoot(); + await runCli(['init'], { cwd: root, stdout: new MemoryWriter(), stderr: new MemoryWriter() }); + const stdout = new MemoryWriter(); + const reviewStateQuery = vi.fn().mockResolvedValue({ + status: 'blocked', + pr_number: 18, + pr_url: 'https://example.test/pr/18', + review_decision: 'CHANGES_REQUESTED', + unresolved_thread_count: 2, + checks: { passing: [], pending: [], failing: [] }, + bot_review_state: 'commented', + approval_count: 0, + changes_requested_count: 1, + merge_ready: false, + blockers: [ + { type: 'unresolved_threads', message: '2 unresolved review threads.', source: 'github' }, + ], + }); + + const exitCode = await runCli(['review-state'], { + cwd: root, + stdout, + stderr: new MemoryWriter(), + reviewStateQuery, + }); + + expect(exitCode).toBe(0); + expect(reviewStateQuery).toHaveBeenCalledWith({ cwd: root, pr: undefined, currentBranch: undefined }); + expect(stdout.output).toContain('Review state: blocked'); + expect(stdout.output).toContain('PR: #18 https://example.test/pr/18'); + expect(stdout.output).toContain('Blockers:'); + }); + + it('emits review-state JSON when requested', async () => { + const root = createTempRoot(); + await runCli(['init'], { cwd: root, stdout: new MemoryWriter(), stderr: new MemoryWriter() }); + const stdout = new MemoryWriter(); + + const exitCode = await runCli(['review-state', '--pr', '18', '--json'], { + cwd: root, + stdout, + stderr: new MemoryWriter(), + reviewStateQuery: vi.fn().mockResolvedValue({ + status: 'ready', + pr_number: 18, + pr_url: 'https://example.test/pr/18', + review_decision: 'APPROVED', + unresolved_thread_count: 0, + checks: { passing: [], pending: [], failing: [] }, + bot_review_state: 'approved', + approval_count: 1, + changes_requested_count: 0, + merge_ready: true, + blockers: [], + }), + }); + + expect(exitCode).toBe(0); + expect(stdout.output).toContain('"status": "ready"'); + expect(stdout.output).toContain('"pr_number": 18'); + }); + + it('rejects mutually exclusive review-state selectors before running GitHub queries', async () => { + const stderr = new MemoryWriter(); + + const exitCode = await runCli(['review-state', '--pr', '18', '--current-branch'], { + cwd: createTempRoot(), + stdout: new MemoryWriter(), + stderr, + }); + + expect(exitCode).toBe(1); + expect(stderr.output).toContain('either `--pr` or `--current-branch`'); + }); + it('refuses close when the drift check is not complete', async () => { const root = createTempRoot(); await runCli(['init'], { cwd: root, stdout: new MemoryWriter(), stderr: new MemoryWriter() }); diff --git a/tests/docs.test.ts b/tests/docs.test.ts index c6894d3..4858356 100644 --- a/tests/docs.test.ts +++ b/tests/docs.test.ts @@ -90,11 +90,11 @@ describe('METHOD docs', () => { expect(readme).toContain('5. **Close** - write the retro and witness packet on the branch.'); expect(readme).toContain('6. **PR / review** - review the full cycle packet until merge or'); expect(readme).toContain('7. **Ship sync on `main`** - after merge, update repo-level ship'); - expect(readme).toContain('Review-stage visibility is not yet a repo-native METHOD query.'); - expect(readme).toContain('Branch and PR context carry that state for now.'); + expect(readme).toContain('Review-stage visibility now has a repo-native METHOD query:'); + expect(readme).toContain('`method review-state`'); expect(readme).toContain('What is actively open in this workspace? -> `method status`'); - expect(readme).not.toContain('What is under review? -> the branch and its closed cycle packet'); - expect(readme).toContain('Review state is not yet part of METHOD\'s repo-native coordination'); + expect(readme).toContain('What is under review? -> `method review-state`'); + expect(readme).toContain('Review state now has a repo-native query surface through'); expect(readme).not.toContain('5. **PR -> main** - review until merge.'); expect(readme).not.toContain('6. **Close** - merge. Retro in `docs/method/retro//`.'); expect(readme).toContain('It is updated during ship sync after merge.'); @@ -104,7 +104,7 @@ describe('METHOD docs', () => { expect(process).toContain('7. Review the complete cycle packet on a branch or PR.'); expect(process).toContain('8. After merge, update repo-level ship surfaces on `main` such as'); expect(process).toContain('reflect merged `main` state, not branch-local closeout state.'); - expect(process).toContain('Review visibility is currently outside METHOD\'s repo-native'); + expect(process).toContain('Review visibility is available through `method review-state`'); }); it('keeps METHOD distinct from forge-specific PR tooling', () => { @@ -711,6 +711,7 @@ describe('METHOD docs', () => { expect(cli).toContain('method pull'); expect(cli).toContain('method close'); expect(cli).toContain('method drift'); + expect(cli).toContain('method review-state'); expect(cli).toContain('method status'); expect(cli).toContain('method mcp'); expect(cli).toContain('method sync'); @@ -721,6 +722,7 @@ describe('METHOD docs', () => { // MCP doc names every tool expect(mcp).toContain('method_status'); + expect(mcp).toContain('method_review_state'); expect(mcp).toContain('method_inbox'); expect(mcp).toContain('method_pull'); expect(mcp).toContain('method_close'); diff --git a/tests/mcp.test.ts b/tests/mcp.test.ts index c99acf1..6e2bd2b 100644 --- a/tests/mcp.test.ts +++ b/tests/mcp.test.ts @@ -7,6 +7,7 @@ import { createMcpServer } from '../src/mcp.js'; import { Server } from '@modelcontextprotocol/sdk/server/index.js'; import { ListToolsRequestSchema, CallToolRequestSchema } from '@modelcontextprotocol/sdk/types.js'; import { initWorkspace, Workspace } from '../src/index.js'; +import type { ReviewStateResult } from '../src/review-state.js'; const tempRoots: string[] = []; @@ -24,7 +25,7 @@ function createTempRoot(): string { return root; } -function createCallToolHarness() { +function createCallToolHarness(options: { reviewStateQuery?: () => Promise } = {}) { let callToolHandler: any; const mockServer = { setRequestHandler: vi.fn((schema, handler) => { @@ -35,7 +36,7 @@ function createCallToolHarness() { }; vi.spyOn(Server.prototype, 'setRequestHandler').mockImplementation(mockServer.setRequestHandler); - createMcpServer(); + createMcpServer({ reviewStateQuery: options.reviewStateQuery }); expect(callToolHandler).toBeDefined(); return callToolHandler; } @@ -66,6 +67,7 @@ describe('MCP Server', () => { const toolNames = result.tools.map((t: any) => t.name); expect(toolNames).toContain('method_status'); + expect(toolNames).toContain('method_review_state'); expect(toolNames).toContain('method_inbox'); expect(toolNames).toContain('method_pull'); expect(toolNames).toContain('method_close'); @@ -80,10 +82,84 @@ describe('MCP Server', () => { } const statusTool = result.tools.find((tool: any) => tool.name === 'method_status'); expect(statusTool.inputSchema.properties.summary).toBeDefined(); + const reviewStateTool = result.tools.find((tool: any) => tool.name === 'method_review_state'); + expect(reviewStateTool.inputSchema.properties.pr).toBeDefined(); + expect(reviewStateTool.inputSchema.properties.currentBranch).toBeDefined(); vi.restoreAllMocks(); }); + it('Does `method_review_state` return the shared review-state result under structuredContent.result?', async () => { + const root = createTempRoot(); + initWorkspace(root); + const callToolHandler = createCallToolHarness({ + reviewStateQuery: vi.fn().mockResolvedValue({ + status: 'ready', + pr_number: 18, + pr_url: 'https://example.test/pr/18', + review_decision: 'APPROVED', + unresolved_thread_count: 0, + checks: { passing: [], pending: [], failing: [] }, + bot_review_state: 'approved', + approval_count: 2, + changes_requested_count: 0, + merge_ready: true, + blockers: [], + }), + }); + + const result = await callToolHandler({ + params: { + name: 'method_review_state', + arguments: { + workspace: root, + pr: 18, + }, + }, + }); + + expect(result.isError).toBe(false); + expect(result.structuredContent.tool).toBe('method_review_state'); + expect(result.structuredContent.result.pr_number).toBe(18); + expect(result.structuredContent.result.merge_ready).toBe(true); + expect(result.content[0].text).toContain('Review state: ready'); + vi.restoreAllMocks(); + }); + + it('Does `method_review_state` reject invalid selector types before querying GitHub?', async () => { + const root = createTempRoot(); + initWorkspace(root); + const callToolHandler = createCallToolHarness(); + + const badPr = await callToolHandler({ + params: { + name: 'method_review_state', + arguments: { + workspace: root, + pr: '18', + }, + }, + }); + + expect(badPr.isError).toBe(true); + expect(badPr.structuredContent.error.message).toContain('pr must be a positive integer'); + + const badSelector = await callToolHandler({ + params: { + name: 'method_review_state', + arguments: { + workspace: root, + pr: 18, + currentBranch: true, + }, + }, + }); + + expect(badSelector.isError).toBe(true); + expect(badSelector.structuredContent.error.message).toContain('either pr or currentBranch'); + vi.restoreAllMocks(); + }); + it('Does `method_status` preserve the legacy full-status default when the caller omits the new summary flag?', async () => { const root = createTempRoot(); initWorkspace(root); diff --git a/tests/review-state.test.ts b/tests/review-state.test.ts new file mode 100644 index 0000000..26a2a92 --- /dev/null +++ b/tests/review-state.test.ts @@ -0,0 +1,271 @@ +import { describe, expect, it } from 'vitest'; +import { queryReviewState, type ReviewStateClient } from '../src/review-state.js'; + +describe('review-state engine', () => { + it('returns a deterministic no-pr result when the current branch has no open PR', async () => { + const client = createClient({ + gitBranch: 'cycles/0034-review-state-query', + prList: [], + }); + + const result = await queryReviewState({ + cwd: '/tmp/method', + client, + }); + + expect(result.status).toBe('no-pr'); + expect(result.pr_number).toBeNull(); + expect(result.merge_ready).toBe(false); + expect(result.blockers).toEqual([ + { + type: 'selection', + message: 'No PR found for current branch: cycles/0034-review-state-query.', + source: 'github', + }, + ]); + }); + + it('returns an ambiguous-pr result when multiple PRs match the current branch', async () => { + const client = createClient({ + gitBranch: 'feature/review-state', + prList: [{ number: 17, url: 'https://example.test/pr/17' }, { number: 18, url: 'https://example.test/pr/18' }], + }); + + const result = await queryReviewState({ + cwd: '/tmp/method', + client, + }); + + expect(result.status).toBe('ambiguous-pr'); + expect(result.blockers[0]).toEqual({ + type: 'selection', + message: 'Multiple PRs match current branch (feature/review-state); rerun with --pr .', + source: 'github', + }); + }); + + it('treats unresolved threads as the live blockers and suppresses stale CHANGES_REQUESTED duplication', async () => { + const client = createClient({ + gitBranch: 'feature/review-state', + prList: [{ number: 18, url: 'https://example.test/pr/18' }], + prView: { + number: 18, + url: 'https://example.test/pr/18', + reviewDecision: 'CHANGES_REQUESTED', + statusCheckRollup: [], + reviews: [ + { author: { login: 'reviewer-one' }, state: 'CHANGES_REQUESTED', submittedAt: '2026-04-09T07:00:00Z' }, + ], + comments: [], + }, + unresolvedThreads: [ + { isResolved: false, isOutdated: false }, + { isResolved: true, isOutdated: false }, + ], + }); + + const result = await queryReviewState({ + cwd: '/tmp/method', + client, + }); + + expect(result.status).toBe('blocked'); + expect(result.unresolved_thread_count).toBe(1); + expect(result.changes_requested_count).toBe(1); + expect(result.blockers).toContainEqual({ + type: 'unresolved_threads', + message: '1 unresolved review thread.', + source: 'github', + }); + expect(result.blockers.some((blocker) => blocker.type === 'review_decision')).toBe(false); + }); + + it('reports pending bot review when a bot status check is newer than earlier blocking reviews', async () => { + const client = createClient({ + prView: { + number: 21, + url: 'https://example.test/pr/21', + reviewDecision: 'APPROVED', + statusCheckRollup: [ + { + __typename: 'CheckRun', + workflowName: 'CI', + name: 'test', + status: 'COMPLETED', + conclusion: 'SUCCESS', + detailsUrl: 'https://example.test/checks/test', + }, + { + __typename: 'StatusContext', + context: 'CodeRabbit', + state: 'PENDING', + targetUrl: 'https://example.test/checks/coderabbit', + startedAt: '2026-04-09T07:30:00Z', + }, + ], + reviews: [ + { author: { login: 'reviewer-one' }, state: 'APPROVED', submittedAt: '2026-04-09T07:15:00Z' }, + { author: { login: 'coderabbitai' }, state: 'CHANGES_REQUESTED', submittedAt: '2026-04-09T07:00:00Z' }, + ], + comments: [], + }, + unresolvedThreads: [], + }); + + const result = await queryReviewState({ + cwd: '/tmp/method', + pr: 21, + client, + }); + + expect(result.bot_review_state).toBe('pending'); + expect(result.blockers).toContainEqual({ + type: 'bot_review', + message: 'CodeRabbit review in progress.', + source: 'CodeRabbit', + }); + expect(result.blockers).toContainEqual({ + type: 'pending_checks', + message: '1 pending check.', + source: 'github', + }); + }); + + it('detects explicit bot cooldown messages from the latest bot signal', async () => { + const client = createClient({ + prView: { + number: 22, + url: 'https://example.test/pr/22', + reviewDecision: 'APPROVED', + statusCheckRollup: [], + reviews: [ + { author: { login: 'reviewer-one' }, state: 'APPROVED', submittedAt: '2026-04-09T07:15:00Z' }, + ], + comments: [ + { + author: { login: 'coderabbitai' }, + createdAt: '2026-04-09T07:31:00Z', + body: [ + '## Rate limit exceeded', + '', + 'Please wait **14 minutes and 30 seconds** before requesting another review.', + ].join('\n'), + }, + ], + }, + unresolvedThreads: [], + }); + + const result = await queryReviewState({ + cwd: '/tmp/method', + pr: 22, + client, + }); + + expect(result.bot_review_state).toBe('commented'); + expect(result.blockers).toContainEqual({ + type: 'policy_cooldown', + message: 'Review cooldown active: wait 14 minutes and 30 seconds before requesting another review.', + source: 'coderabbitai', + }); + }); + + it('returns ready when checks pass, threads are resolved, and no reviewer or bot is blocking', async () => { + const client = createClient({ + prView: { + number: 23, + url: 'https://example.test/pr/23', + reviewDecision: 'APPROVED', + statusCheckRollup: [ + { + __typename: 'CheckRun', + workflowName: 'CI', + name: 'test', + status: 'COMPLETED', + conclusion: 'SUCCESS', + detailsUrl: 'https://example.test/checks/test', + }, + { + __typename: 'StatusContext', + context: 'CodeRabbit', + state: 'SUCCESS', + targetUrl: 'https://example.test/checks/coderabbit', + startedAt: '2026-04-09T07:33:00Z', + }, + ], + reviews: [ + { author: { login: 'reviewer-one' }, state: 'APPROVED', submittedAt: '2026-04-09T07:20:00Z' }, + ], + comments: [], + }, + unresolvedThreads: [], + }); + + const result = await queryReviewState({ + cwd: '/tmp/method', + pr: 23, + client, + }); + + expect(result.status).toBe('ready'); + expect(result.merge_ready).toBe(true); + expect(result.bot_review_state).toBe('approved'); + expect(result.approval_count).toBe(1); + expect(result.blockers).toEqual([]); + }); +}); + +function createClient(options: { + gitBranch?: string; + prList?: Array<{ number: number; url: string }>; + prView?: Record; + unresolvedThreads?: Array<{ isResolved: boolean; isOutdated: boolean }>; +}): ReviewStateClient { + return { + async ghJson(_cwd: string, args: readonly string[]): Promise { + if (args[0] === 'pr' && args[1] === 'list') { + return (options.prList ?? []) as T; + } + if (args[0] === 'repo' && args[1] === 'view') { + return { + owner: { login: 'flyingrobots' }, + name: 'method', + } as T; + } + if (args[0] === 'pr' && args[1] === 'view') { + return (options.prView ?? { + number: 999, + url: 'https://example.test/pr/999', + reviewDecision: 'NONE', + statusCheckRollup: [], + reviews: [], + comments: [], + }) as T; + } + if (args[0] === 'api' && args[1] === 'graphql') { + return { + data: { + repository: { + pullRequest: { + reviewThreads: { + pageInfo: { + hasNextPage: false, + endCursor: null, + }, + nodes: options.unresolvedThreads ?? [], + }, + }, + }, + }, + } as T; + } + throw new Error(`Unexpected gh call: ${args.join(' ')}`); + }, + async gitText(_cwd: string, args: readonly string[]): Promise { + if (args.join(' ') === 'rev-parse --abbrev-ref HEAD') { + return `${options.gitBranch ?? 'main'}\n`; + } + throw new Error(`Unexpected git call: ${args.join(' ')}`); + }, + }; +} From d6b7e1bf94320026434c17a3f85be6985ae809cd Mon Sep 17 00:00:00 2001 From: James Ross Date: Thu, 9 Apr 2026 01:52:48 -0700 Subject: [PATCH 2/4] Fix review-state follow-up feedback --- docs/MCP.md | 4 +- .../review-state-query.md | 4 -- src/mcp.ts | 6 +-- src/review-state.ts | 30 ++++++++++--- tests/mcp.test.ts | 43 ++++++++++++------- tests/review-state.test.ts | 36 ++++++++++++++++ 6 files changed, 92 insertions(+), 31 deletions(-) diff --git a/docs/MCP.md b/docs/MCP.md index 320b873..967985d 100644 --- a/docs/MCP.md +++ b/docs/MCP.md @@ -23,11 +23,11 @@ workspace-agnostic — a single instance can serve multiple projects. ### `method_review_state` -Get PR review / merge-readiness state for the current branch or an explicit PR +Get PR review / merge-readiness state for the current branch or an explicit PR. `pr` and `currentBranch` are mutually exclusive; when `pr` is omitted, current-branch resolution is the default behavior. **Parameters:** -- `pr` (optional) `number` — Explicit PR number to inspect +- `pr` (optional) `integer` — Explicit PR number to inspect - `currentBranch` (optional) `boolean` — Resolve the PR from the current branch (default when pr is omitted) ### `method_status` diff --git a/docs/design/0034-review-state-query/review-state-query.md b/docs/design/0034-review-state-query/review-state-query.md index 16dfe49..32bc0a0 100644 --- a/docs/design/0034-review-state-query/review-state-query.md +++ b/docs/design/0034-review-state-query/review-state-query.md @@ -74,10 +74,6 @@ checks, sticky review decisions, and observable bot cooldowns. signal actually contains a rate-limit message; the engine must not invent hidden policy state. -## Non-goals - -- [ ] TBD - ## Backlog Context METHOD currently says review state is not part of the repo-native diff --git a/src/mcp.ts b/src/mcp.ts index 7cd60cf..74ddbee 100644 --- a/src/mcp.ts +++ b/src/mcp.ts @@ -21,7 +21,7 @@ export interface McpToolDef { description: string; inputSchema: { type: 'object'; - properties: Record; + properties: Record; required: string[]; }; } @@ -29,12 +29,12 @@ export interface McpToolDef { export const MCP_TOOLS: McpToolDef[] = [ { name: 'method_review_state', - description: 'Get PR review / merge-readiness state for the current branch or an explicit PR', + description: 'Get PR review / merge-readiness state for the current branch or an explicit PR. `pr` and `currentBranch` are mutually exclusive; when `pr` is omitted, current-branch resolution is the default behavior.', inputSchema: { type: 'object', properties: { ...workspaceProperty, - pr: { type: 'number', description: 'Explicit PR number to inspect' }, + pr: { type: 'integer', minimum: 1, description: 'Explicit PR number to inspect' }, currentBranch: { type: 'boolean', description: 'Resolve the PR from the current branch (default when pr is omitted)' }, }, required: ['workspace'], diff --git a/src/review-state.ts b/src/review-state.ts index 6e119b1..394e837 100644 --- a/src/review-state.ts +++ b/src/review-state.ts @@ -154,14 +154,23 @@ interface BotCheckSignal extends TimedSignal { export async function queryReviewState(options: ReviewStateQueryOptions): Promise { const pr = options.pr; const currentBranch = options.currentBranch ?? pr === undefined; + if (pr === undefined && currentBranch !== true) { + throw new Error('review-state requires either --pr or --current-branch; cannot use currentBranch:false with no PR.'); + } if (pr !== undefined && currentBranch) { throw new Error('review-state accepts either --pr or --current-branch, not both.'); } const client = options.client ?? createExecReviewStateClient(); - const selection = pr === undefined - ? await resolveCurrentBranchPr(options.cwd, client) - : { kind: 'selected' as const, prNumber: pr }; + let selection: + | { kind: 'selected'; prNumber: number } + | { kind: 'no-pr'; message: string } + | { kind: 'ambiguous-pr'; message: string }; + if (currentBranch) { + selection = await resolveCurrentBranchPr(options.cwd, client); + } else { + selection = { kind: 'selected', prNumber: requirePositiveInteger(pr, 'PR number') }; + } if (selection.kind === 'no-pr') { return selectionResult('no-pr', selection.message); @@ -498,20 +507,27 @@ function isFailingCheck(item: GhCheckRun): boolean { } function latestDecisiveStates(reviews: GhReview[]): Map { - const latest = new Map(); + const latest = new Map(); for (const review of reviews) { const author = normalizeOptionalString(review.author?.login); const state = normalizeOptionalString(review.state)?.toUpperCase(); - if (author === undefined || (state !== 'APPROVED' && state !== 'CHANGES_REQUESTED')) { + if ( + author === undefined + || (state !== 'APPROVED' && state !== 'CHANGES_REQUESTED' && state !== 'DISMISSED') + ) { continue; } const timestamp = parseTimestamp(review.submittedAt); const previous = latest.get(author); if (previous === undefined || timestamp >= previous.timestamp) { - latest.set(author, { timestamp, state }); + latest.set(author, { timestamp, state: state === 'DISMISSED' ? undefined : state }); } } - return new Map([...latest.entries()].map(([author, value]) => [author, value.state])); + return new Map( + [...latest.entries()] + .filter(([, value]) => value.state !== undefined) + .map(([author, value]) => [author, value.state!]), + ); } function analyzeBotSignals(reviews: GhReview[], comments: GhComment[], checks: GhCheckRun[]): { diff --git a/tests/mcp.test.ts b/tests/mcp.test.ts index 6e2bd2b..4e15a3d 100644 --- a/tests/mcp.test.ts +++ b/tests/mcp.test.ts @@ -25,7 +25,11 @@ function createTempRoot(): string { return root; } -function createCallToolHarness(options: { reviewStateQuery?: () => Promise } = {}) { +function createCallToolHarness( + options: { + reviewStateQuery?: (options: { cwd: string; pr?: number; currentBranch?: boolean }) => Promise; + } = {}, +) { let callToolHandler: any; const mockServer = { setRequestHandler: vi.fn((schema, handler) => { @@ -92,20 +96,21 @@ describe('MCP Server', () => { it('Does `method_review_state` return the shared review-state result under structuredContent.result?', async () => { const root = createTempRoot(); initWorkspace(root); + const reviewStateQuery = vi.fn().mockResolvedValue({ + status: 'ready', + pr_number: 18, + pr_url: 'https://example.test/pr/18', + review_decision: 'APPROVED', + unresolved_thread_count: 0, + checks: { passing: [], pending: [], failing: [] }, + bot_review_state: 'approved', + approval_count: 2, + changes_requested_count: 0, + merge_ready: true, + blockers: [], + }); const callToolHandler = createCallToolHarness({ - reviewStateQuery: vi.fn().mockResolvedValue({ - status: 'ready', - pr_number: 18, - pr_url: 'https://example.test/pr/18', - review_decision: 'APPROVED', - unresolved_thread_count: 0, - checks: { passing: [], pending: [], failing: [] }, - bot_review_state: 'approved', - approval_count: 2, - changes_requested_count: 0, - merge_ready: true, - blockers: [], - }), + reviewStateQuery, }); const result = await callToolHandler({ @@ -122,6 +127,11 @@ describe('MCP Server', () => { expect(result.structuredContent.tool).toBe('method_review_state'); expect(result.structuredContent.result.pr_number).toBe(18); expect(result.structuredContent.result.merge_ready).toBe(true); + expect(reviewStateQuery).toHaveBeenCalledWith({ + cwd: root, + pr: 18, + currentBranch: undefined, + }); expect(result.content[0].text).toContain('Review state: ready'); vi.restoreAllMocks(); }); @@ -129,7 +139,8 @@ describe('MCP Server', () => { it('Does `method_review_state` reject invalid selector types before querying GitHub?', async () => { const root = createTempRoot(); initWorkspace(root); - const callToolHandler = createCallToolHarness(); + const reviewStateQuery = vi.fn(); + const callToolHandler = createCallToolHarness({ reviewStateQuery }); const badPr = await callToolHandler({ params: { @@ -143,6 +154,7 @@ describe('MCP Server', () => { expect(badPr.isError).toBe(true); expect(badPr.structuredContent.error.message).toContain('pr must be a positive integer'); + expect(reviewStateQuery).not.toHaveBeenCalled(); const badSelector = await callToolHandler({ params: { @@ -157,6 +169,7 @@ describe('MCP Server', () => { expect(badSelector.isError).toBe(true); expect(badSelector.structuredContent.error.message).toContain('either pr or currentBranch'); + expect(reviewStateQuery).not.toHaveBeenCalled(); vi.restoreAllMocks(); }); diff --git a/tests/review-state.test.ts b/tests/review-state.test.ts index 26a2a92..8fcdccb 100644 --- a/tests/review-state.test.ts +++ b/tests/review-state.test.ts @@ -44,6 +44,16 @@ describe('review-state engine', () => { }); }); + it('rejects currentBranch:false when no explicit PR is provided', async () => { + const client = createClient({}); + + await expect(queryReviewState({ + cwd: '/tmp/method', + currentBranch: false, + client, + })).rejects.toThrow('review-state requires either --pr or --current-branch'); + }); + it('treats unresolved threads as the live blockers and suppresses stale CHANGES_REQUESTED duplication', async () => { const client = createClient({ gitBranch: 'feature/review-state', @@ -213,6 +223,32 @@ describe('review-state engine', () => { expect(result.approval_count).toBe(1); expect(result.blockers).toEqual([]); }); + + it('clears an earlier decisive vote when the same reviewer later dismisses it', async () => { + const client = createClient({ + prView: { + number: 24, + url: 'https://example.test/pr/24', + reviewDecision: 'NONE', + statusCheckRollup: [], + reviews: [ + { author: { login: 'reviewer-one' }, state: 'APPROVED', submittedAt: '2026-04-09T07:10:00Z' }, + { author: { login: 'reviewer-one' }, state: 'DISMISSED', submittedAt: '2026-04-09T07:20:00Z' }, + ], + comments: [], + }, + unresolvedThreads: [], + }); + + const result = await queryReviewState({ + cwd: '/tmp/method', + pr: 24, + client, + }); + + expect(result.approval_count).toBe(0); + expect(result.changes_requested_count).toBe(0); + }); }); function createClient(options: { From d96845ebdd2533fbbade98c5f34a4d5f920e82cd Mon Sep 17 00:00:00 2001 From: James Ross Date: Thu, 9 Apr 2026 05:12:30 -0700 Subject: [PATCH 3/4] Fix review-state follow-up issues --- src/mcp.ts | 3 +- src/review-state.ts | 10 +++- tests/mcp.test.ts | 38 ++++++++++++++ tests/review-state-exec.test.ts | 91 +++++++++++++++++++++++++++++++++ tests/review-state.test.ts | 46 +++++++++++++++++ 5 files changed, 186 insertions(+), 2 deletions(-) create mode 100644 tests/review-state-exec.test.ts diff --git a/src/mcp.ts b/src/mcp.ts index 74ddbee..42e3128 100644 --- a/src/mcp.ts +++ b/src/mcp.ts @@ -137,7 +137,8 @@ export function createMcpServer(options: CreateMcpServerOptions = {}) { if (request.params.name === 'method_review_state') { const pr = validateOptionalInteger(args.pr, 'pr'); - const currentBranch = validateOptionalBoolean(args.currentBranch, 'currentBranch'); + const currentBranchArg = validateOptionalBoolean(args.currentBranch, 'currentBranch'); + const currentBranch = currentBranchArg === true ? true : undefined; if (pr !== undefined && currentBranch === true) { throw new Error('method_review_state accepts either pr or currentBranch, not both.'); } diff --git a/src/review-state.ts b/src/review-state.ts index 394e837..e6c3a42 100644 --- a/src/review-state.ts +++ b/src/review-state.ts @@ -2,6 +2,7 @@ import { execFile } from 'node:child_process'; import { promisify } from 'node:util'; const execFileAsync = promisify(execFile); +const COMMAND_MAX_BUFFER = 10 * 1024 * 1024; const REVIEW_STATE_STATUSES = ['ready', 'blocked', 'no-pr', 'ambiguous-pr'] as const; const REVIEW_DECISIONS = ['APPROVED', 'CHANGES_REQUESTED', 'REVIEW_REQUIRED', 'NONE'] as const; @@ -248,6 +249,7 @@ async function runCommand(command: string, args: readonly string[], cwd: string) cwd, encoding: 'utf8', timeout: 30_000, + maxBuffer: COMMAND_MAX_BUFFER, env: { ...process.env, GH_PAGER: 'cat' }, }); return stdout; @@ -461,9 +463,11 @@ function classifyChecks(items: GhCheckRun[]): ReviewStateResult['checks'] { failing.push(check); continue; } - if (check.status.length > 0) { + if (isPassingCheck(check.status)) { passing.push(check); + continue; } + pending.push(check); } return { passing, pending, failing }; @@ -506,6 +510,10 @@ function isFailingCheck(item: GhCheckRun): boolean { return ['FAILURE', 'FAILED', 'ERROR', 'CANCELLED', 'ACTION_REQUIRED', 'TIMED_OUT', 'STALE', 'STARTUP_FAILURE'].includes(status); } +function isPassingCheck(status: string): boolean { + return ['SUCCESS', 'NEUTRAL', 'SKIPPED'].includes(status); +} + function latestDecisiveStates(reviews: GhReview[]): Map { const latest = new Map(); for (const review of reviews) { diff --git a/tests/mcp.test.ts b/tests/mcp.test.ts index 4e15a3d..4c885f4 100644 --- a/tests/mcp.test.ts +++ b/tests/mcp.test.ts @@ -173,6 +173,44 @@ describe('MCP Server', () => { vi.restoreAllMocks(); }); + it('Does `method_review_state` normalize currentBranch:false to the default current-branch query path?', async () => { + const root = createTempRoot(); + initWorkspace(root); + const reviewStateQuery = vi.fn().mockResolvedValue({ + status: 'no-pr', + pr_number: null, + pr_url: null, + review_decision: 'NONE', + unresolved_thread_count: 0, + checks: { passing: [], pending: [], failing: [] }, + bot_review_state: 'none', + approval_count: 0, + changes_requested_count: 0, + merge_ready: false, + blockers: [{ type: 'selection', message: 'No PR found for current branch.', source: 'github' }], + }); + const callToolHandler = createCallToolHarness({ reviewStateQuery }); + + const result = await callToolHandler({ + params: { + name: 'method_review_state', + arguments: { + workspace: root, + currentBranch: false, + }, + }, + }); + + expect(result.isError).toBe(false); + expect(reviewStateQuery).toHaveBeenCalledWith({ + cwd: root, + pr: undefined, + currentBranch: undefined, + }); + expect(result.structuredContent.result.status).toBe('no-pr'); + vi.restoreAllMocks(); + }); + it('Does `method_status` preserve the legacy full-status default when the caller omits the new summary flag?', async () => { const root = createTempRoot(); initWorkspace(root); diff --git a/tests/review-state-exec.test.ts b/tests/review-state-exec.test.ts new file mode 100644 index 0000000..cde9125 --- /dev/null +++ b/tests/review-state-exec.test.ts @@ -0,0 +1,91 @@ +import { promisify } from 'node:util'; +import { afterEach, describe, expect, it, vi } from 'vitest'; + +describe('review-state exec client', () => { + afterEach(() => { + vi.doUnmock('node:child_process'); + vi.resetModules(); + vi.restoreAllMocks(); + }); + + it('passes a larger maxBuffer to gh and git subprocesses used by the default query path', async () => { + vi.resetModules(); + const calls: Array<{ command: string; args: string[]; options: Record }> = []; + const resolveCommand = (command: string, args: string[], options: Record): { stdout: string; stderr: string } => { + calls.push({ command, args: [...args], options }); + + if (command === 'git' && args.join(' ') === 'rev-parse --abbrev-ref HEAD') { + return { stdout: 'cycles/0034-review-state-query\n', stderr: '' }; + } + + if (command === 'gh' && args[0] === 'pr' && args[1] === 'list') { + return { stdout: JSON.stringify([{ number: 19, url: 'https://example.test/pr/19' }]), stderr: '' }; + } + + if (command === 'gh' && args[0] === 'repo' && args[1] === 'view') { + return { stdout: JSON.stringify({ owner: { login: 'flyingrobots' }, name: 'method' }), stderr: '' }; + } + + if (command === 'gh' && args[0] === 'pr' && args[1] === 'view') { + return { + stdout: JSON.stringify({ + number: 19, + url: 'https://example.test/pr/19', + reviewDecision: 'APPROVED', + statusCheckRollup: [], + reviews: [], + comments: [], + }), + stderr: '', + }; + } + + if (command === 'gh' && args[0] === 'api' && args[1] === 'graphql') { + return { + stdout: JSON.stringify({ + data: { + repository: { + pullRequest: { + reviewThreads: { + pageInfo: { + hasNextPage: false, + endCursor: null, + }, + nodes: [], + }, + }, + }, + }, + }), + stderr: '', + }; + } + + throw new Error(`Unexpected command: ${command} ${args.join(' ')}`); + }; + const execFileMock = vi.fn((command: string, args: string[], options: Record, callback: (error: Error | null, stdout?: string, stderr?: string) => void) => { + try { + const result = resolveCommand(command, args, options); + callback(null, result.stdout, result.stderr); + } catch (error) { + callback(error as Error); + } + }); + execFileMock[promisify.custom] = (command: string, args: string[], options: Record) => Promise.resolve(resolveCommand(command, args, options)); + + vi.doMock('node:child_process', () => ({ + execFile: execFileMock, + })); + + const { queryReviewState } = await import('../src/review-state.js'); + + const result = await queryReviewState({ cwd: '/tmp/method' }); + + expect(result.status).toBe('ready'); + expect(calls.length).toBeGreaterThan(0); + for (const call of calls) { + expect(call.options.maxBuffer).toBe(10 * 1024 * 1024); + expect(call.options.timeout).toBe(30_000); + } + }); +}); diff --git a/tests/review-state.test.ts b/tests/review-state.test.ts index 8fcdccb..236afa0 100644 --- a/tests/review-state.test.ts +++ b/tests/review-state.test.ts @@ -224,6 +224,52 @@ describe('review-state engine', () => { expect(result.blockers).toEqual([]); }); + it('treats unknown check states as blocking instead of silently counting them as passing', async () => { + const client = createClient({ + prView: { + number: 25, + url: 'https://example.test/pr/25', + reviewDecision: 'APPROVED', + statusCheckRollup: [ + { + __typename: 'StatusContext', + context: 'mystery-check', + state: 'UNKNOWN', + targetUrl: 'https://example.test/checks/mystery', + startedAt: '2026-04-09T07:33:00Z', + }, + ], + reviews: [ + { author: { login: 'reviewer-one' }, state: 'APPROVED', submittedAt: '2026-04-09T07:20:00Z' }, + ], + comments: [], + }, + unresolvedThreads: [], + }); + + const result = await queryReviewState({ + cwd: '/tmp/method', + pr: 25, + client, + }); + + expect(result.status).toBe('blocked'); + expect(result.merge_ready).toBe(false); + expect(result.checks.passing).toEqual([]); + expect(result.checks.pending).toEqual([ + { + name: 'mystery-check', + status: 'UNKNOWN', + url: 'https://example.test/checks/mystery', + }, + ]); + expect(result.blockers).toContainEqual({ + type: 'pending_checks', + message: '1 pending check.', + source: 'github', + }); + }); + it('clears an earlier decisive vote when the same reviewer later dismisses it', async () => { const client = createClient({ prView: { From 6e193698df7540c319c1c893858f92cc15ba91ac Mon Sep 17 00:00:00 2001 From: James Ross Date: Thu, 9 Apr 2026 05:25:16 -0700 Subject: [PATCH 4/4] Rename bot activity timestamp in review-state --- src/review-state.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/review-state.ts b/src/review-state.ts index e6c3a42..054e868 100644 --- a/src/review-state.ts +++ b/src/review-state.ts @@ -588,7 +588,7 @@ function analyzeBotSignals(reviews: GhReview[], comments: GhComment[], checks: G const latestBotCheck = latestTimedSignal(botChecks); const latestSignal = latestTimedSignal([...botComments, ...botReviews, ...botChecks]); const cooldownMessage = latestSignal?.body === undefined ? undefined : extractCooldownMessage(latestSignal.body); - const latestHumanMeaningfulTimestamp = Math.max(latestBotReview?.timestamp ?? 0, latestBotComment?.timestamp ?? 0); + const latestBotMeaningfulTimestamp = Math.max(latestBotReview?.timestamp ?? 0, latestBotComment?.timestamp ?? 0); if (cooldownMessage !== undefined) { return { @@ -598,7 +598,7 @@ function analyzeBotSignals(reviews: GhReview[], comments: GhComment[], checks: G }; } - if (latestBotCheck !== undefined && isPendingBotStatus(latestBotCheck.status) && latestBotCheck.timestamp >= latestHumanMeaningfulTimestamp) { + if (latestBotCheck !== undefined && isPendingBotStatus(latestBotCheck.status) && latestBotCheck.timestamp >= latestBotMeaningfulTimestamp) { return { state: 'pending', source: latestBotCheck.source }; } @@ -608,7 +608,7 @@ function analyzeBotSignals(reviews: GhReview[], comments: GhComment[], checks: G if (latestBotReview?.state === 'CHANGES_REQUESTED') { return { state: 'changes_requested', source: latestBotReview.source }; } - if (latestBotCheck !== undefined && isSuccessfulBotStatus(latestBotCheck.status) && latestBotCheck.timestamp >= latestHumanMeaningfulTimestamp) { + if (latestBotCheck !== undefined && isSuccessfulBotStatus(latestBotCheck.status) && latestBotCheck.timestamp >= latestBotMeaningfulTimestamp) { return { state: 'approved', source: latestBotCheck.source }; } if (latestSignal !== undefined) {