From bd57e80a8ab0bfc04a9b99819bfd599ad7872f87 Mon Sep 17 00:00:00 2001 From: hbrooks Date: Sat, 25 Jul 2026 15:37:05 -0400 Subject: [PATCH] review: ask for a code review now, on a PR or on your working tree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviews could only be triggered by a push. `agent review 5975` reviews a pull request on demand; a bare `agent review` reviews the work in your tree right now, before there is a PR to review it against. The local path snapshots your working tree, force-pushes it to a sidecar branch (never the branch you are on), and the platform finds-or-creates a draft PR to review it against — a code review is structurally a PR review, so it needs one. Those reviews are terminal-only: nothing is posted to GitHub, findings print in the terminal. A review id IS a session id, so `agent session get `, records, and stop all work on it — which is why there is no review-specific status or stream plumbing here. Needs @ellipsis-dev/sdk 0.3.0 for the generated types. --- bun.lock | 4 +- package.json | 2 +- src/cli.tsx | 2 + src/commands/review.ts | 386 +++++++++++++++++++++++++++++++++++++++++ src/lib/api.ts | 33 ++++ src/lib/help.ts | 2 +- src/lib/laptop.ts | 26 +++ src/lib/types.ts | 25 +++ test/review.test.ts | 226 ++++++++++++++++++++++++ 9 files changed, 702 insertions(+), 4 deletions(-) create mode 100644 src/commands/review.ts create mode 100644 test/review.test.ts diff --git a/bun.lock b/bun.lock index a634fb0..9db0533 100644 --- a/bun.lock +++ b/bun.lock @@ -5,7 +5,7 @@ "": { "name": "@ellipsis/cli", "dependencies": { - "@ellipsis-dev/sdk": "^0.2.3", + "@ellipsis-dev/sdk": "^0.3.0", "commander": "^12.1.0", "ink": "^5.0.1", "ink-spinner": "^5.0.0", @@ -28,7 +28,7 @@ "packages": { "@alcalzone/ansi-tokenize": ["@alcalzone/ansi-tokenize@0.1.3", "", { "dependencies": { "ansi-styles": "^6.2.1", "is-fullwidth-code-point": "^4.0.0" } }, "sha512-3yWxPTq3UQ/FY9p1ErPxIyfT64elWaMvM9lIHnaqpyft63tkxodF5aUElYHrdisWve5cETkh1+KBw1yJuW0aRw=="], - "@ellipsis-dev/sdk": ["@ellipsis-dev/sdk@0.2.3", "", {}, "sha512-VETp7aLWJe1q6vXHBZaCFh/RCYn59t7zQ8kWDvoT1zBFjfHLsQ4aL2usTnjBTTfblOTq2H+iiXrsHXfw0Gg56A=="], + "@ellipsis-dev/sdk": ["@ellipsis-dev/sdk@0.3.0", "", {}, "sha512-b7Q5Vzkny/bHYtmwVrLyNt8LxwOVHCg2riawYT6l+FmH+IywMPmnQciuF3uTELlSZ71WfE2nmTgkk8Sex+em5A=="], "@esbuild/aix-ppc64": ["@esbuild/aix-ppc64@0.27.7", "", { "os": "aix", "cpu": "ppc64" }, "sha512-EKX3Qwmhz1eMdEJokhALr0YiD0lhQNwDqkPYyPhiSwKrh7/4KRjQc04sZ8db+5DVVnZ1LmbNDI1uAMPEUBnQPg=="], diff --git a/package.json b/package.json index 3b0f13a..7baaeee 100644 --- a/package.json +++ b/package.json @@ -20,7 +20,7 @@ "test:watch": "vitest" }, "dependencies": { - "@ellipsis-dev/sdk": "^0.2.3", + "@ellipsis-dev/sdk": "^0.3.0", "commander": "^12.1.0", "ink": "^5.0.1", "ink-spinner": "^5.0.0", diff --git a/src/cli.tsx b/src/cli.tsx index 749a978..ef67cc9 100644 --- a/src/cli.tsx +++ b/src/cli.tsx @@ -3,6 +3,7 @@ import { registerLogin } from './commands/login' import { registerHost } from './commands/host' import { registerMe } from './commands/me' import { registerSession } from './commands/session' +import { registerReview } from './commands/review' import { registerConfig } from './commands/config' import { registerSandbox } from './commands/sandbox' import { registerAsset } from './commands/asset' @@ -36,6 +37,7 @@ registerLogin(program) registerHost(program) registerMe(program) registerSession(program) +registerReview(program) registerConfig(program) registerSandbox(program) registerAsset(program) diff --git a/src/commands/review.ts b/src/commands/review.ts new file mode 100644 index 0000000..a50fb11 --- /dev/null +++ b/src/commands/review.ts @@ -0,0 +1,386 @@ +import { type Command } from 'commander' +import { ApiClient, ApiError } from '../lib/api' +import { alsoKnownAs, apiRoutes } from '../lib/help' +import { createWipCommit, currentBranch, pushReviewBranch, repoFromCwd } from '../lib/laptop' +import { formatTs, printJson, printTable, relativeAge, runAction, usdFromMillicents } from '../lib/output' +import { watchSessionStreaming } from './session' +import type { CreateReviewRequest, Finding, Review, ReviewScope } from '../lib/types' + +// `agent review`: ask for a code review now, instead of waiting for a push to +// trigger one. Two targets — +// +// agent review 5975 an existing pull request +// agent review the work in your tree, right now +// +// The second is the interesting one: it snapshots your working tree, pushes it +// to a sidecar branch (never your own), and the platform finds-or-creates a +// draft PR to review it against — because a code review is structurally a PR +// review (the range, the checkout, and the delivery all read PR state). Those +// reviews are terminal-only: nothing is posted to GitHub, findings print here. +// +// A review IS a session under the hood, and a review id IS a session id — so +// `agent session get `, `--watch`, records, and stop all work on it. + +// How long to wait between REST polls when the stream isn't available. +const FALLBACK_POLL_INTERVAL_SECONDS = 3 + +export function registerReview(program: Command): void { + const review = alsoKnownAs( + program + .command('review') + .description('Review a pull request, or the work in your tree, on demand'), + 'reviews', + 'code-review', + 'cr', + ) + + apiRoutes( + review + .command('start [pull-request]', { isDefault: true }) + .description('Review a pull request by number, or your working tree if you omit one'), + 'POST /v1/reviews', + 'WS /v1/sessions/{id}/stream', + 'GET /v1/reviews/{id}', + ) + .option('--repo ', 'repository to review (default: this git remote)') + .option( + '--branch ', + 'review an already-pushed branch instead of snapshotting your tree', + ) + .option('--full', 're-review the whole pull request, not just the new commits') + .option('--watermark ', 'pin the range start (a commit SHA)') + .option('--head ', 'pin the range end (a commit SHA)') + .option('-c, --config ', 'run a saved agent config instead of the built-in reviewer') + .option('-m, --model ', 'override the reviewer model (see `agent model list`)') + .option('-b, --budget ', 'override the budget for this review', parseUsd) + .option('--no-post', 'do not post to GitHub; print the findings here instead') + .option('--no-wait', 'print the review id and exit instead of waiting for findings') + .option('--cwd ', 'repository directory (default: current directory)') + .option('--json', 'output raw JSON') + .action(async (pullRequest: string | undefined, opts: StartOptions) => { + await runAction(async () => { + const api = new ApiClient() + const request = buildCreateRequest(pullRequest, opts) + const local = request.branch !== undefined + if (!opts.json && local) { + console.log(`✓ pushed ${request.sha?.slice(0, 12)} to ${request.branch}`) + } + const started = await api.createReview(request) + + // Nothing new since the last review of this PR. Not an error — you + // asked, and the honest answer is "already covered". + if (started.status === 'skipped') { + if (opts.json) printJson(started) + else { + console.log( + `already reviewed at ${(started.scope.watermark ?? '').slice(0, 7)} — ` + + 'nothing new to review (use --full to re-review the whole PR)', + ) + } + return + } + + if (!opts.wait) { + if (opts.json) printJson(started) + else { + console.log(`✓ started review ${started.id}`) + console.log(` follow with: agent review get ${started.id}`) + } + return + } + + // Block-and-stream, then re-read: the findings are collected from the + // sandbox at teardown, so they only exist once the review finalizes. + // Same two-step `agent asset get` uses. + if (!opts.json) { + console.log( + `✓ reviewing ${request.owner}/${request.repo}#${started.pull_request.number} ` + + `(${started.id})`, + ) + } + await watchSessionStreaming(api, started.id, FALLBACK_POLL_INTERVAL_SECONDS, false) + const finished = await api.getReview(started.id) + if (opts.json) printJson(finished) + else renderReview(finished) + }) + }) + + apiRoutes( + review + .command('get ') + .description("Print a review's findings, scope, and whether it posted"), + 'GET /v1/reviews/{id}', + ) + .option('--json', 'output raw JSON') + .action(async (reviewId: string, opts: { json?: boolean }) => { + await runAction(async () => { + const found = await getReviewOrExplain(new ApiClient(), reviewId) + if (opts.json) printJson(found) + else renderReview(found) + }) + }) + + apiRoutes( + alsoKnownAs( + review.command('list').description("List a pull request's reviews, newest first"), + 'ls', + ), + 'GET /v1/reviews', + ) + .option('--repo ', 'only reviews of this repository') + .option('--pr ', 'only reviews of this pull request', parsePositiveInt) + .option('-s, --status ', 'only reviews in this session status') + .option('-l, --limit ', 'max results (server cap: 200)', parsePositiveInt) + .option('--json', 'output raw JSON') + .action(async (opts: ListOptions) => { + await runAction(async () => { + const repo = opts.repo ? splitRepo(opts.repo) : undefined + if (opts.pr !== undefined && repo === undefined) { + throw new Error('--pr needs --repo to say which repository') + } + const reviews = await new ApiClient().listReviews({ + owner: repo?.owner, + repo: repo?.name, + pull_request_number: opts.pr, + status: opts.status, + limit: opts.limit, + }) + if (opts.json) { + printJson(reviews) + return + } + if (reviews.length === 0) { + console.log('No reviews.') + return + } + printTable( + ['ID', 'PR', 'STATUS', 'SCOPE', 'FINDINGS', 'POSTED', 'COST', 'AGE'], + reviews.map((r) => [ + r.id, + `${r.repository.owner}/${r.repository.name}#${r.pull_request.number}`, + r.status, + scopeWord(r), + r.counters ? String(r.counters.n_parsed) : '-', + r.posted_review_id ? String(r.posted_review_id) : r.post_error ? 'failed' : '-', + usdFromMillicents(r.cost_millicents), + r.created_at ? relativeAge(r.created_at) : '-', + ]), + ) + }) + }) +} + +interface StartOptions { + repo?: string + branch?: string + full?: boolean + watermark?: string + head?: string + config?: string + model?: string + budget?: number + // commander sets these false for --no-* flags. + post: boolean + wait: boolean + cwd?: string + json?: boolean +} + +interface ListOptions { + repo?: string + pr?: number + status?: string + limit?: number + json?: boolean +} + +// Assemble the request, doing the local path's git work when no pull request +// was named. Exported for tests. +export function buildCreateRequest( + pullRequest: string | undefined, + opts: StartOptions, +): CreateReviewRequest { + const cwd = opts.cwd ?? process.cwd() + const repo = splitRepo(opts.repo ?? repoFromCwdOrThrow(cwd)) + const scope = buildScope(opts) + + const common = { + owner: repo.owner, + repo: repo.name, + scope, + // The generated type requires every field the server defaults, so send the + // empty default rather than omitting it. + metadata: {}, + ...(opts.config ? { config_id: opts.config } : {}), + ...(opts.model ? { model: opts.model } : {}), + ...(opts.budget !== undefined ? { budget: opts.budget } : {}), + } + + if (pullRequest !== undefined) { + return { ...common, pull_request_number: parsePullRequest(pullRequest), post: opts.post } + } + + // The local path. Snapshot the tree and push it to a sidecar branch, so the + // branch you are working on is never touched. ALWAYS terminal-only — the + // pull request being reviewed is one the platform manufactured for the + // purpose, so commenting on it would be talking to itself. Findings print + // here instead. (`post` has no opt-in flag; --no-post only turns it off for + // a real pull request.) + const branch = opts.branch ?? sidecarFor(cwd) + const sha = opts.branch ? undefined : pushSnapshot(cwd, branch) + return { ...common, branch, ...(sha ? { sha } : {}), post: false } +} + +// `--full`/`--watermark`/`--head` → the scope model. Default is incremental: +// only the commits since the last review, which is what the webhook does. +function buildScope(opts: StartOptions): ReviewScope { + return { + kind: opts.full ? 'full' : 'incremental', + ...(opts.watermark ? { watermark: opts.watermark } : {}), + ...(opts.head ? { head: opts.head } : {}), + } +} + +function sidecarFor(cwd: string): string { + const branch = currentBranch(cwd) + if (!branch) { + throw new Error( + 'detached HEAD: check out a branch, or name a pull request (`agent review 123`)', + ) + } + return `ellipsis/review/${branch}` +} + +// Snapshot the working tree and force-push it. Returns the pushed SHA, which +// pins the review's range end — GitHub's reported PR head lags a force-push. +function pushSnapshot(cwd: string, branch: string): string { + const { sha } = createWipCommit(cwd) + const realBranch = currentBranch(cwd) + if (!realBranch) throw new Error('detached HEAD: check out a branch first') + pushReviewBranch(cwd, sha, realBranch) + return sha +} + +async function getReviewOrExplain(api: ApiClient, reviewId: string): Promise { + try { + return await api.getReview(reviewId) + } catch (err) { + // A review id IS a session id, so the most likely mistake is handing this + // the id of a session that isn't a review — indistinguishable from an + // unknown id server-side, on purpose. + if (err instanceof ApiError && err.status === 404) { + throw new Error(`no review with id ${reviewId} (a review id looks like session_…)`) + } + throw err + } +} + +function renderReview(review: Review): void { + console.log(`review: ${review.id}`) + console.log( + `pr: ${review.repository.owner}/${review.repository.name}` + + `#${review.pull_request.number} ${review.pull_request.url}`, + ) + console.log(`status: ${review.status}`) + console.log(`scope: ${scopeWord(review)}`) + if (review.posted_review_id) console.log(`posted: ${review.posted_review_id}`) + if (review.post_error) console.log(`post: failed — ${review.post_error}`) + console.log(`cost: ${usdFromMillicents(review.cost_millicents)}`) + if (review.completed_at) console.log(`completed: ${formatTs(review.completed_at)}`) + + if (review.review_body) console.log(`\n${review.review_body.trim()}`) + + const findings = review.findings ?? [] + if (findings.length === 0) { + // Distinguish "clean" from "not collected yet": the outbox row only exists + // once the review finalizes. + console.log(review.counters ? '\nNo findings.' : '\nStill running — no findings yet.') + return + } + console.log('') + // Highest severity first, the order the platform posts them in. + for (const finding of [...findings].sort((a, b) => b.severity - a.severity)) { + console.log(formatFinding(finding)) + } + const counters = review.counters + if (counters && counters.n_dropped > 0) { + console.log(`(${counters.n_dropped} finding(s) could not be parsed)`) + } +} + +// One finding as a `path:line severity category` header plus its claim, so the +// output greps like a compiler's. Exported for tests. +export function formatFinding(finding: Finding): string { + const lines = + finding.end_line > finding.start_line + ? `${finding.start_line}-${finding.end_line}` + : String(finding.start_line) + const head = `${finding.path}:${lines} [${finding.severity}/5 ${finding.category}]` + const body = [finding.claim, finding.evidence, finding.suggested_fix] + .filter((part): part is string => Boolean(part && part.trim())) + .map((part) => indent(part.trim())) + .join('\n') + // Anchored off the diff means it was recorded but never posted inline. + const note = + finding.anchor === 'not_commentable' + ? indent('(outside the diff — recorded, not posted)') + : '' + return [head, body, note].filter(Boolean).join('\n') + '\n' +} + +function indent(text: string): string { + return text + .split('\n') + .map((line) => ` ${line}`) + .join('\n') +} + +function scopeWord(review: Review): string { + const { watermark, head } = review.scope + return `${(watermark ?? 'base').slice(0, 7)}...${(head ?? '').slice(0, 7)}` +} + +function repoFromCwdOrThrow(cwd: string): string { + const repo = repoFromCwd(cwd) + if (!repo) { + throw new Error( + 'not inside a git repository with an origin remote — pass --repo ', + ) + } + return repo +} + +export function splitRepo(value: string): { owner: string; name: string } { + const [owner, name, ...rest] = value.split('/') + if (!owner || !name || rest.length > 0) { + throw new Error(`--repo must be owner/name (got '${value}')`) + } + return { owner, name } +} + +// Accept `123` and `#123`, and a full PR URL, since all three get pasted. +export function parsePullRequest(raw: string): number { + const match = /^#?(\d+)$/.exec(raw.trim()) ?? /\/pull\/(\d+)/.exec(raw.trim()) + if (!match) { + // `review` reserves the word, so `agent review the auth changes` lands + // here rather than starting a session with that prompt. Name the fix. + throw new Error( + `'${raw}' is not a pull request number. Pass a number (agent review 123), ` + + 'or omit it to review your working tree. To run an agent with a prompt ' + + `that starts with "review", quote it: agent "review ${raw} …"`, + ) + } + return Number.parseInt(match[1], 10) +} + +function parsePositiveInt(raw: string): number { + const n = Number.parseInt(raw, 10) + if (!Number.isFinite(n) || n <= 0) throw new Error(`invalid count '${raw}'`) + return n +} + +function parseUsd(raw: string): number { + const n = Number.parseFloat(raw) + if (!Number.isFinite(n) || n < 0) throw new Error(`invalid budget '${raw}'`) + return n +} diff --git a/src/lib/api.ts b/src/lib/api.ts index 610b9f1..4ae1582 100644 --- a/src/lib/api.ts +++ b/src/lib/api.ts @@ -14,6 +14,7 @@ import type { CreateAgentConfigRequest, CreateAssetRequest, CreateAssetResponse, + CreateReviewRequest, CreatedAgentConfig, GetAssetResponse, GetAnalyticsMetricsResponse, @@ -34,6 +35,8 @@ import type { ListGithubMembersResponse, ListGithubRepositoriesResponse, ListLinearTeamsResponse, + ListReviewsQuery, + ListReviewsResponse, ListSentryOrganizationsResponse, ListSessionRecordsResponse, ListSessionTurnsResponse, @@ -41,6 +44,7 @@ import type { ListSlackMembersResponse, PutAgentDefaultRequest, ReplayAgentSessionRequest, + Review, SendSessionMessageRequest, SandboxVariableInput, SandboxVariableSummary, @@ -329,6 +333,35 @@ export class ApiClient { return this.request('DELETE', `/v1/assets/${encodeURIComponent(assetId)}`) } + // -------------------------------- reviews -------------------------------- + // A review IS a code_review agent session over one commit range, and its id + // IS the session id — so the session methods above (get, records, stream, + // stop) all work on a review id unchanged, and only the review-shaped parts + // (scope, findings, posting outcome) need endpoints of their own. + + createReview(request: CreateReviewRequest): Promise { + return this.request('POST', '/v1/reviews', request) + } + + // The findings only exist once the review finalizes (they're collected from + // the sandbox at teardown), so a running review returns findings: [] — hence + // the stream-then-re-GET two-step the command uses. + getReview(reviewId: string): Promise { + return this.request('GET', `/v1/reviews/${encodeURIComponent(reviewId)}`) + } + + // Newest first, findings omitted (counters only). Includes webhook-triggered + // reviews, so this is a PR's whole review history. + async listReviews(query: ListReviewsQuery = {}): Promise { + const res = await this.request( + 'GET', + '/v1/reviews', + undefined, + query, + ) + return res.reviews + } + // ----------------------------- agent configs ---------------------------- async listAgentConfigs(): Promise { diff --git a/src/lib/help.ts b/src/lib/help.ts index 7a69b71..6373bc9 100644 --- a/src/lib/help.ts +++ b/src/lib/help.ts @@ -18,7 +18,7 @@ function withoutAliases(term: string, cmd: Command): string { // missing from every group still renders (under "Other") rather than silently // vanishing from help. const TOP_LEVEL_GROUPS: ReadonlyArray<{ title: string; commands: readonly string[] }> = [ - { title: 'Sessions', commands: ['session'] }, + { title: 'Sessions', commands: ['session', 'review'] }, { title: 'Agents', commands: ['config', 'model', 'template'] }, { title: 'Platform', commands: ['sandbox', 'asset', 'hook'] }, { title: 'Integrations', commands: ['integration', 'github', 'slack', 'linear', 'sentry'] }, diff --git a/src/lib/laptop.ts b/src/lib/laptop.ts index 6fe5c2c..69edb2f 100644 --- a/src/lib/laptop.ts +++ b/src/lib/laptop.ts @@ -442,3 +442,29 @@ export function pushHandoffRef(cwd: string, sha: string): string { gitOrThrow(cwd, 'push', 'origin', `${sha}:${ref}`) return ref } + +// --------------------------------------------------------------------------- +// Local review: the same snapshot trick, but pushed to a real BRANCH. +// --------------------------------------------------------------------------- + +export function currentBranch(cwd: string): string | null { + const branch = gitOrThrow(cwd, 'rev-parse', '--abbrev-ref', 'HEAD') + // Detached HEAD has no branch to derive a sidecar name from. + return branch && branch !== 'HEAD' ? branch : null +} + +// The sidecar branch a local review is pushed to. Your real branch is never +// touched, so a stash-quality WIP commit never lands on the work you're doing. +export function reviewBranchName(branch: string): string { + return `ellipsis/review/${branch}` +} + +// Force-push the WIP snapshot to the sidecar branch. A BRANCH, not the hidden +// ref handoff uses: GitHub cannot open a pull request from a non-branch ref, +// and the platform needs a PR to review (it finds-or-creates a draft one). +// Force because each review replaces the previous snapshot of the same work. +export function pushReviewBranch(cwd: string, sha: string, branch: string): string { + const target = reviewBranchName(branch) + gitOrThrow(cwd, 'push', '--force', 'origin', `${sha}:refs/heads/${target}`) + return target +} diff --git a/src/lib/types.ts b/src/lib/types.ts index 17ff2c5..5c5a548 100644 --- a/src/lib/types.ts +++ b/src/lib/types.ts @@ -24,8 +24,18 @@ export type { AgentSessionSource, AgentSessionStatus, AgentSessionWire, + // The reviews surface. A review's `id` IS a session id, so everything above + // applies to it unchanged — which is why there is no review-specific + // status, stream, or cost type. + CreateReviewRequest, + Finding, + ListReviewsResponse, ListSessionRecordsResponse, ListSessionTurnsResponse, + ResolvedReviewScope, + Review, + ReviewCounters, + ReviewScope, SendSessionMessageRequest, SessionPrompting, SessionState, @@ -852,6 +862,21 @@ export interface GetAssetResponse { download_url: string } +// ------------------------------- reviews -------------------------------- +// The Review/CreateReviewRequest shapes come from the SDK (re-exported at the +// top of this file); only the list query is hand-rolled, since query params +// aren't part of the generated response types. + +export interface ListReviewsQuery { + owner?: string + repo?: string + pull_request_number?: number + status?: string + limit?: number + // The client's query builder takes a plain record. + [key: string]: unknown +} + // ------------------------------ cli auth -------------------------------- export interface CliAuthStart { diff --git a/test/review.test.ts b/test/review.test.ts new file mode 100644 index 0000000..e17fec9 --- /dev/null +++ b/test/review.test.ts @@ -0,0 +1,226 @@ +import { execFileSync } from 'node:child_process' +import { mkdtempSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { describe, expect, it } from 'vitest' +import { + buildCreateRequest, + formatFinding, + parsePullRequest, + splitRepo, +} from '../src/commands/review' +import { currentBranch, reviewBranchName } from '../src/lib/laptop' +import type { Finding } from '../src/lib/types' + +// A throwaway repo with an origin remote, so the local path's git work runs for +// real instead of being mocked. The "remote" is a bare repo on disk — good +// enough for `push` and for repoFromCwd's remote parsing. +function scratchRepo(branch = 'feature/thing'): { work: string; remote: string } { + const root = mkdtempSync(join(tmpdir(), 'agent-review-')) + const remote = join(root, 'remote.git') + const work = join(root, 'work') + execFileSync('git', ['init', '--bare', '-b', 'main', remote]) + execFileSync('git', ['init', '-b', 'main', work]) + const git = (...args: string[]) => execFileSync('git', ['-C', work, ...args]) + git('config', 'user.email', 'ci@example.com') + git('config', 'user.name', 'ci') + // An https URL so repoFromCwd resolves owner/name; the push target is set + // separately below, since that URL isn't reachable. + git('remote', 'add', 'origin', 'https://github.com/ellipsis-dev/scratch.git') + git('remote', 'set-url', '--push', 'origin', remote) + writeFileSync(join(work, 'a.txt'), 'one\n') + git('add', '.') + git('commit', '-m', 'first') + git('checkout', '-b', branch) + return { work, remote } +} + +const START_DEFAULTS = { post: true, wait: true, json: false } + +describe('buildCreateRequest — an existing pull request', () => { + it('sends owner and repo as separate fields, never owner/name', () => { + const req = buildCreateRequest('5975', { + ...START_DEFAULTS, + repo: 'ellipsis-dev/ellipsis', + }) + expect(req.owner).toBe('ellipsis-dev') + expect(req.repo).toBe('ellipsis') + expect(req.pull_request_number).toBe(5975) + expect(req.branch).toBeUndefined() + }) + + it('defaults to incremental — only the commits since the last review', () => { + const req = buildCreateRequest('1', { ...START_DEFAULTS, repo: 'o/r' }) + expect(req.scope.kind).toBe('incremental') + }) + + it('--full re-reviews the whole pull request', () => { + const req = buildCreateRequest('1', { ...START_DEFAULTS, repo: 'o/r', full: true }) + expect(req.scope.kind).toBe('full') + }) + + it('passes a pinned range through as SHAs', () => { + const req = buildCreateRequest('1', { + ...START_DEFAULTS, + repo: 'o/r', + watermark: 'aaaa111', + head: 'bbbb222', + }) + expect(req.scope.watermark).toBe('aaaa111') + expect(req.scope.head).toBe('bbbb222') + }) + + it('forwards the config, model, and budget overrides', () => { + const req = buildCreateRequest('1', { + ...START_DEFAULTS, + repo: 'o/r', + config: 'agent_abc', + model: 'claude-opus-4-8', + budget: 5, + }) + expect(req.config_id).toBe('agent_abc') + expect(req.model).toBe('claude-opus-4-8') + expect(req.budget).toBe(5) + }) + + it('posts by default, and --no-post turns it off', () => { + expect(buildCreateRequest('1', { ...START_DEFAULTS, repo: 'o/r' }).post).toBe(true) + expect( + buildCreateRequest('1', { ...START_DEFAULTS, repo: 'o/r', post: false }).post, + ).toBe(false) + }) +}) + +describe('buildCreateRequest — the local path', () => { + it('pushes a sidecar branch and pins the range end to the pushed commit', () => { + const { work: cwd, remote } = scratchRepo('feature/thing') + writeFileSync(join(cwd, 'a.txt'), 'dirty\n') + + const req = buildCreateRequest(undefined, { ...START_DEFAULTS, cwd }) + + // The sidecar, never the branch you're working on. + expect(req.branch).toBe('ellipsis/review/feature/thing') + expect(currentBranch(cwd)).toBe('feature/thing') + // The pushed snapshot pins the head: GitHub's reported PR head lags a + // force-push to the sidecar. + expect(req.sha).toMatch(/^[0-9a-f]{40}$/) + expect(req.pull_request_number).toBeUndefined() + // The commit really landed on the remote (read the bare repo directly — + // the fetch URL is a real GitHub URL this test can't reach). + const pushed = execFileSync('git', ['-C', remote, 'show-ref'], { + encoding: 'utf8', + }) + expect(pushed).toContain('refs/heads/ellipsis/review/feature/thing') + }) + + it('never posts a local review, even without --no-post', () => { + const { work: cwd } = scratchRepo() + // Reviewing unfinished work must not leave comments on a pull request, so + // the terminal-only default does not depend on the caller remembering. + expect(buildCreateRequest(undefined, { ...START_DEFAULTS, cwd }).post).toBe(false) + }) + + it('snapshots a clean tree too (HEAD), so a review needs no dirty edit', () => { + const { work: cwd } = scratchRepo() + const head = execFileSync('git', ['-C', cwd, 'rev-parse', 'HEAD'], { + encoding: 'utf8', + }).trim() + expect(buildCreateRequest(undefined, { ...START_DEFAULTS, cwd }).sha).toBe(head) + }) + + it('reviews an already-pushed branch without touching git', () => { + const { work: cwd } = scratchRepo() + const req = buildCreateRequest(undefined, { + ...START_DEFAULTS, + cwd, + branch: 'someone/elses-branch', + }) + expect(req.branch).toBe('someone/elses-branch') + // Nothing was pushed, so there is no snapshot SHA to pin. + expect(req.sha).toBeUndefined() + }) + + it('explains itself on a detached HEAD instead of guessing a branch', () => { + const { work: cwd } = scratchRepo() + execFileSync('git', ['-C', cwd, 'checkout', '--detach'], { stdio: 'ignore' }) + expect(() => buildCreateRequest(undefined, { ...START_DEFAULTS, cwd })).toThrow( + /detached HEAD/, + ) + }) + + it('needs a repo when there is no git remote to infer one from', () => { + expect(() => + buildCreateRequest(undefined, { ...START_DEFAULTS, cwd: mkdtempSync(join(tmpdir(), 'bare-')) }), + ).toThrow(/--repo/) + }) +}) + +describe('reviewBranchName', () => { + it('prefixes the sidecar so it is obvious what it is in a branch list', () => { + expect(reviewBranchName('hunter/my-feature')).toBe('ellipsis/review/hunter/my-feature') + }) +}) + +describe('parsePullRequest', () => { + it('accepts the three spellings people paste', () => { + expect(parsePullRequest('5975')).toBe(5975) + expect(parsePullRequest('#5975')).toBe(5975) + expect(parsePullRequest('https://github.com/ellipsis-dev/ellipsis/pull/5975')).toBe(5975) + }) + + it('teaches the fix when `review` swallowed a bare prompt', () => { + // `agent review the auth changes` reaches here because the verb reserves + // the word — the error has to name the quoted form. + expect(() => parsePullRequest('the')).toThrow(/quote it/) + }) +}) + +describe('splitRepo', () => { + it('rejects anything that is not exactly owner/name', () => { + expect(splitRepo('o/r')).toEqual({ owner: 'o', name: 'r' }) + expect(() => splitRepo('just-a-name')).toThrow(/owner\/name/) + expect(() => splitRepo('a/b/c')).toThrow(/owner\/name/) + }) +}) + +describe('formatFinding', () => { + const base: Finding = { + path: 'src/auth.py', + start_line: 42, + end_line: 42, + side: 'RIGHT', + severity: 4, + category: 'security', + claim: 'Missing authz check.', + evidence: '', + suggested_fix: null, + confidence: null, + extra: {}, + anchor: 'valid', + snapped_from: null, + in_scope: true, + } + + it('leads with a greppable path:line and the severity', () => { + expect(formatFinding(base)).toContain('src/auth.py:42 [4/5 security]') + }) + + it('renders a multi-line anchor as a range', () => { + expect(formatFinding({ ...base, end_line: 48 })).toContain('src/auth.py:42-48') + }) + + it('includes the evidence and the suggested fix', () => { + const out = formatFinding({ + ...base, + evidence: 'no membership assert', + suggested_fix: 'assert_membership(user)', + }) + expect(out).toContain('no membership assert') + expect(out).toContain('assert_membership(user)') + }) + + it('flags a finding that was recorded but never posted inline', () => { + const out = formatFinding({ ...base, anchor: 'not_commentable' }) + expect(out).toContain('outside the diff') + }) +})