From d5f1b78ec1a0f1ba2654969f1d01a8e08fe59d9b Mon Sep 17 00:00:00 2001 From: hbrooks Date: Tue, 11 Aug 2026 00:08:47 -0400 Subject: [PATCH] api: catch the CLI up to the current routes, and stop failing silently Most commands were 404ing against prod: the backend renamed routes under /integrations, /agents, /secrets, and /files, and every CLI path was a hand-maintained copy that drifted. Worse, the failures were invisible in the sessions UI, because the composer's pickers swallowed their errors and rendered an empty list. Repoints every drifted path and bumps @ellipsis-dev/sdk to 0.6.0. Error output now reads the server's {error: {code, message}} envelope, so a failure shows what went wrong instead of a bare status line; FastAPI's own {detail} rejections are still read, since validation and auth answer in that shape. Renames the asset command tree to file, following the server. The old spelling stays as a hidden alias. Drops the review surface the server deleted: pull_request_number is required now, so reviewing your working tree cannot work, and the /reviews/defaults ladder was replaced by resolving a pipeline from its committed location. That also fixes review init, which defaulted to agents/code_review.yaml and so wrote a file the sync rejects outright; the only paths that run are code_review.yaml and .ellipsis/code_review.yaml. --- README.md | 16 +- bun.lock | 4 +- package.json | 2 +- src/cli.tsx | 4 +- src/commands/config.ts | 16 +- src/commands/{asset.ts => file.ts} | 118 +++++---- src/commands/github.ts | 4 +- src/commands/linear.ts | 2 +- src/commands/review.ts | 345 ++++++--------------------- src/commands/sentry.ts | 2 +- src/commands/session.tsx | 4 +- src/commands/slack.ts | 4 +- src/commands/template.ts | 2 +- src/commands/variable.ts | 6 +- src/lib/api.ts | 139 +++++------ src/lib/help.ts | 2 +- src/lib/laptop.ts | 25 -- src/lib/output.ts | 2 +- src/lib/types.ts | 60 ++--- src/ui/SessionsApp.tsx | 58 +++-- test/api.test.ts | 107 ++------- test/discovery.test.ts | 20 +- test/{asset.test.ts => file.test.ts} | 10 +- test/output.test.ts | 2 +- test/review.test.ts | 113 ++------- 25 files changed, 349 insertions(+), 718 deletions(-) rename src/commands/{asset.ts => file.ts} (68%) rename test/{asset.test.ts => file.test.ts} (92%) diff --git a/README.md b/README.md index ac92c91..3a83ef9 100644 --- a/README.md +++ b/README.md @@ -68,11 +68,9 @@ agent session connect # inside an Ellipsis sandbox: connects to agent session stop # stop an in-flight session agent review 123 # review a pull request now, instead of waiting for a push -agent review # review the work in your tree; findings print here +agent review get # a review's findings, scope, and whether it posted agent review list --repo api # list a repository's reviews, newest first -agent review init [path] # scaffold a starter review pipeline (default: agents/code_review.yaml) -agent review default # the effective review pipeline for the repo you are standing in -agent review default set # set the account default pipeline (--repo [owner/name] for one repo) +agent review init # scaffold a starter review pipeline (code_review.yaml) agent config list # list saved agent configs agent config get # show one config as YAML (--json for JSON) @@ -92,10 +90,10 @@ agent slack members # workspace members, with linked GitHub identi agent linear teams # teams in the connected Linear organization agent sentry orgs # connected Sentry organizations -agent asset upload shot.png # store a PNG; prints an org-gated link to paste into a PR comment -agent asset list # list stored assets (--session scopes to one run's uploads) -agent asset get -o shot.png # show one asset, or download its bytes with -o -agent asset delete # delete an asset (it disappears from list/get and its link stops resolving) +agent file upload shot.png # store a PNG; prints an org-gated link to paste into a PR comment +agent file list # list stored files (--session scopes to one run's uploads) +agent file get -o shot.png # show one file, or download its bytes with -o +agent file delete # delete a file (it disappears from list/get and its link stops resolving) agent variable list # list sandbox env variable names (values are write-only) agent variable set A=1 B=2 # create/update variables (or --from-file .env/.json) @@ -110,7 +108,7 @@ agent analytics review --repo my-service # review totals + top reviewers agent ping # check authenticated API connectivity ``` -Every command shown is singular. The plural spelling of each (`agent assets`, +Every command shown is singular. The plural spelling of each (`agent files`, `agent sessions`, `agent analytics prs`) is a hidden alias that works but is left out of `--help`. See [`skills/cli-conventions`](skills/cli-conventions/SKILL.md) for the full diff --git a/bun.lock b/bun.lock index daaa242..480cb52 100644 --- a/bun.lock +++ b/bun.lock @@ -5,7 +5,7 @@ "": { "name": "@ellipsis/cli", "dependencies": { - "@ellipsis-dev/sdk": "^0.5.0", + "@ellipsis-dev/sdk": "^0.6.0", "chalk": "^5.6.2", "cli-table3": "^0.6.5", "commander": "^12.1.0", @@ -35,7 +35,7 @@ "@colors/colors": ["@colors/colors@1.5.0", "", {}, "sha512-ooWCrlZP11i8GImSjTHYHLkvFDP48nS4+204nGb1RiX/WXYHmJA2III9/e2DWVabCESdW7hBAEzHRqUn9OUVvQ=="], - "@ellipsis-dev/sdk": ["@ellipsis-dev/sdk@0.5.0", "", {}, "sha512-14g9g9Mrt10hfoPpaqeIqZsr8vsDYkBtIwdkd6Rz/3Izs5pvtVm4zys5znRL9UH8dC+R02B1Ljz2AGKGrtEEsQ=="], + "@ellipsis-dev/sdk": ["@ellipsis-dev/sdk@0.6.0", "", {}, "sha512-FEZVXzM+TJ7YrDnJaQu4G+cxpX/49wFbtQelldB/FowX7uwi9X13rY0Crv6Oxox+uJiise7TeK9AIAyfZbYv0Q=="], "@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 1984a10..ddece9f 100644 --- a/package.json +++ b/package.json @@ -20,7 +20,7 @@ "test:watch": "vitest" }, "dependencies": { - "@ellipsis-dev/sdk": "^0.5.0", + "@ellipsis-dev/sdk": "^0.6.0", "chalk": "^5.6.2", "cli-table3": "^0.6.5", "commander": "^12.1.0", diff --git a/src/cli.tsx b/src/cli.tsx index 7b4200e..be79329 100644 --- a/src/cli.tsx +++ b/src/cli.tsx @@ -7,7 +7,7 @@ import { registerSession } from './commands/session' import { registerReview } from './commands/review' import { registerConfig } from './commands/config' import { registerVariable } from './commands/variable' -import { registerAsset } from './commands/asset' +import { registerFile } from './commands/file' import { registerHook } from './commands/hooks' import { registerTemplate } from './commands/template' import { registerModel } from './commands/model' @@ -44,7 +44,7 @@ registerSession(program) registerReview(program) registerConfig(program) registerVariable(program) -registerAsset(program) +registerFile(program) registerHook(program) registerTemplate(program) registerModel(program) diff --git a/src/commands/config.ts b/src/commands/config.ts index 01f2529..dadd318 100644 --- a/src/commands/config.ts +++ b/src/commands/config.ts @@ -29,7 +29,7 @@ export function registerConfig(program: Command): void { config.command('list').description('List your saved agent configs'), 'ls', ), - 'GET /configs', + 'GET /agents/configs', ) .option('--json', 'output raw JSON') .action(async (opts: { json?: boolean }) => { @@ -59,7 +59,7 @@ export function registerConfig(program: Command): void { config .command('get ') .description('Print one agent config as YAML, or as JSON with --json'), - 'GET /configs/{id}', + 'GET /agents/configs/{id}', ) .option('--json', 'output raw JSON') .action(async (configId: string, opts: { json?: boolean }) => { @@ -85,7 +85,7 @@ export function registerConfig(program: Command): void { config .command('create') .description('Create an agent config by opening a pull request that adds it to a repo'), - 'POST /configs', + 'POST /agents/configs', ) .requiredOption( '-r, --repo ', @@ -148,7 +148,7 @@ export function registerConfig(program: Command): void { .description('Show or set which agent config runs when a session names none'), 'defaults', ), - 'GET /defaults', + 'GET /agents/defaults', ) .option('--json', 'output raw JSON') // Bare `agent config default`: the effective default for the repo you're @@ -189,7 +189,7 @@ export function registerConfig(program: Command): void { .description('List every default that is set, account rung and per-repo rungs'), 'ls', ), - 'GET /defaults', + 'GET /agents/defaults', ) .option('--json', 'output raw JSON') // The group also defines --json (for the bare view), and commander parses @@ -223,7 +223,7 @@ export function registerConfig(program: Command): void { defaults .command('set ') .description('Set the account default agent config, or a repo default with --repo'), - 'PUT /defaults', + 'PUT /agents/defaults', ) .option( '-r, --repo [repository]', @@ -256,7 +256,7 @@ export function registerConfig(program: Command): void { 'rm', 'delete', ), - 'DELETE /defaults', + 'DELETE /agents/defaults', ) .option( '-r, --repo [repository]', @@ -278,7 +278,7 @@ export function registerConfig(program: Command): void { .description( `Scaffold a starter agent config YAML locally (default: ${DEFAULT_CONFIG_PATH})`, ), - 'POST /configs with --template', + 'POST /agents/configs with --template', ) // No `-f` short: CLI-wide, `-f` means an input file (see `config create`). .option('--force', 'overwrite the file if it already exists') diff --git a/src/commands/asset.ts b/src/commands/file.ts similarity index 68% rename from src/commands/asset.ts rename to src/commands/file.ts index 2da9ff2..fa16300 100644 --- a/src/commands/asset.ts +++ b/src/commands/file.ts @@ -4,20 +4,19 @@ import { basename } from 'node:path' import { ApiClient, ApiError } from '../lib/api' import { alsoKnownAs, apiRoutes } from '../lib/help' import { formatTs, printJson, printTable, runAction } from '../lib/output' -import type { AssetView, CreateAssetRequest, GetAssetResponse } from '../lib/types' +import type { CreateFileRequest, FileView, GetFileResponse } from '../lib/types' -// `agent asset `: persist files to Ellipsis platform storage and get -// back an org-membership-gated link (documents/eng/AGENT_ASSET_STORAGE.md in -// the ellipsis repo). The primary caller is an agent inside a sandbox that -// took a screenshot of a UI change and wants a link to paste into a PR -// comment — the injected sandbox token authenticates it with zero setup, and -// the same commands work on a laptop with a device-login token. +// `agent file `: persist files to Ellipsis platform storage and get back +// an org-membership-gated link. The primary caller is an agent inside a sandbox +// that took a screenshot of a UI change and wants a link to paste into a PR +// comment: the injected sandbox token authenticates it with zero setup, and the +// same commands work on a laptop with a device-login token. -// Client-side mirrors of the server limits (assets_service.py), so an +// Client-side mirrors of the server limits (files_service.py), so an // oversized or non-PNG file fails fast with a clear message instead of a // base64-inflated round trip to a 400. The server re-validates; these are // UX, not enforcement. -export const MAX_ASSET_SIZE_BYTES = 10 * 1024 * 1024 +export const MAX_FILE_SIZE_BYTES = 10 * 1024 * 1024 const PNG_MAGIC = Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]) // Well-known magic bytes we can name in the "not a PNG" error, so an agent @@ -45,12 +44,12 @@ function sniffFormat(bytes: Buffer): string | null { // Build the upload request from a file's bytes, throwing the fast client-side // errors (empty, oversized, not a PNG). Exported for tests. -export function buildUploadRequest(path: string, bytes: Buffer): CreateAssetRequest { +export function buildUploadRequest(path: string, bytes: Buffer): CreateFileRequest { if (bytes.length === 0) throw new Error(`${path} is empty`) - if (bytes.length > MAX_ASSET_SIZE_BYTES) { + if (bytes.length > MAX_FILE_SIZE_BYTES) { throw new Error( `${path} is ${formatSize(bytes.length)}; the limit is ` + - `${formatSize(MAX_ASSET_SIZE_BYTES)} per asset`, + `${formatSize(MAX_FILE_SIZE_BYTES)} per file`, ) } if (!bytes.subarray(0, PNG_MAGIC.length).equals(PNG_MAGIC)) { @@ -75,25 +74,27 @@ export function formatSize(bytes: number): string { return `${(bytes / (1024 * 1024)).toFixed(1)} MiB` } -export function registerAsset(program: Command): void { - const asset = alsoKnownAs( +export function registerFile(program: Command): void { + const file = alsoKnownAs( program - .command('asset') + .command('file') .description('Store files on the platform and share them as org-gated links'), + 'files', + 'asset', 'assets', ) apiRoutes( - asset + file .command('upload ') .description('Upload a PNG and print its org-gated URL, ready to paste into a PR comment'), - 'POST /assets', + 'POST /files', ) .option('--json', 'output raw JSON') .action(async (path: string, opts: { json?: boolean }) => { await runAction(async () => { const req = buildUploadRequest(path, readFileSync(path)) - const res = await new ApiClient().uploadAsset(req) + const res = await new ApiClient().uploadFile(req) // The URL is the whole point — keep it the bare primary output so an // agent (or $(...) in a script) can capture it directly. if (opts.json) printJson(res) @@ -102,87 +103,82 @@ export function registerAsset(program: Command): void { }) apiRoutes( - alsoKnownAs( - asset.command('list').description('List your stored assets, newest first'), - 'ls', - ), - 'GET /assets', + alsoKnownAs(file.command('list').description('List your stored files, newest first'), 'ls'), + 'GET /files', ) - .option('--session ', 'only assets uploaded by this agent session') + .option('--session ', 'only files uploaded by this agent session') .option('-l, --limit ', 'max results (server cap: 250)', parsePositiveInt) .option('--json', 'output raw JSON') .action(async (opts: { session?: string; limit?: number; json?: boolean }) => { await runAction(async () => { - const assets = await new ApiClient().listAssets({ + const files = await new ApiClient().listFiles({ agent_session_id: opts.session, limit: opts.limit, }) if (opts.json) { - printJson(assets) + printJson(files) return } - if (assets.length === 0) { - console.log('No assets.') + if (files.length === 0) { + console.log('No files.') return } printTable( ['ID', 'FILENAME', 'SIZE', 'CREATED', 'SESSION'], - assets.map((a) => [ - a.id, - a.filename, - formatSize(a.size_bytes), - formatTs(a.created_at), - a.agent_session_id ?? '-', + files.map((f) => [ + f.id, + f.filename, + formatSize(f.size_bytes), + formatTs(f.created_at), + f.agent_session_id ?? '-', ]), ) }) }) apiRoutes( - asset - .command('get ') - .description("Print one asset's metadata, or download its bytes with -o"), - 'GET /assets/{id}', + file + .command('get ') + .description("Print one file's metadata, or download its bytes with -o"), + 'GET /files/{id}', 'presigned S3 GET', ) .option('-o, --output ', 'write the file contents to this path') .option('--json', 'output raw JSON (includes the short-lived download_url)') - .action(async (assetId: string, opts: { output?: string; json?: boolean }) => { + .action(async (fileId: string, opts: { output?: string; json?: boolean }) => { await runAction(async () => { - const res = await new ApiClient().getAsset(assetId) + const res = await new ApiClient().getFile(fileId) if (opts.output) { // download_url is a ~60s presigned S3 GET — fetch it immediately, // while it's fresh. The JSON API never carries the bytes itself. await downloadTo(res.download_url, opts.output) if (!opts.json) { - console.log(`✓ wrote ${opts.output} (${formatSize(res.asset.size_bytes)})`) + console.log(`✓ wrote ${opts.output} (${formatSize(res.file.size_bytes)})`) } } if (opts.json) printJson(res) - else if (!opts.output) renderAsset(res) + else if (!opts.output) renderFile(res) }) }) apiRoutes( alsoKnownAs( - asset - .command('delete ') - .description('Delete an asset, so its link stops resolving'), + file.command('delete ').description('Delete a file, so its link stops resolving'), 'rm', ), - 'DELETE /assets/{id}', + 'DELETE /files/{id}', ) .option('--json', 'output raw JSON') - .action(async (assetId: string, opts: { json?: boolean }) => { + .action(async (fileId: string, opts: { json?: boolean }) => { await runAction(async () => { try { - await new ApiClient().deleteAsset(assetId) + await new ApiClient().deleteFile(fileId) } catch (err) { // A 404 covers "never existed", "someone else's", and "already // deleted" — all the same "there's nothing here to delete" to the // caller, so give one clear message instead of the raw HTTP error. if (err instanceof ApiError && err.status === 404) { - throw new Error(`asset not found: ${assetId}`) + throw new Error(`file not found: ${fileId}`) } // A 403 is a real policy decision (e.g. sandbox tokens can't delete); // surface the server's own explanation rather than masking it. @@ -192,26 +188,26 @@ export function registerAsset(program: Command): void { throw err } // 204 No Content — nothing to echo, so confirm with the id. - if (opts.json) printJson({ id: assetId, deleted: true }) - else console.log(`✓ deleted ${assetId}`) + if (opts.json) printJson({ id: fileId, deleted: true }) + else console.log(`✓ deleted ${fileId}`) }) }) } -function renderAsset(res: GetAssetResponse): void { - const a: AssetView = res.asset - console.log(`id: ${a.id}`) - console.log(`filename: ${a.filename}`) - console.log(`type: ${a.content_type}`) - console.log(`size: ${formatSize(a.size_bytes)}`) - console.log(`created: ${formatTs(a.created_at)}`) - if (a.agent_session_id) console.log(`session: ${a.agent_session_id}`) +function renderFile(res: GetFileResponse): void { + const f: FileView = res.file + console.log(`id: ${f.id}`) + console.log(`filename: ${f.filename}`) + console.log(`type: ${f.content_type}`) + console.log(`size: ${formatSize(f.size_bytes)}`) + console.log(`created: ${formatTs(f.created_at)}`) + if (f.agent_session_id) console.log(`session: ${f.agent_session_id}`) console.log(`url: ${res.url}`) - console.log(`\ndownload the file with: agent asset get ${a.id} -o ${a.filename}`) + console.log(`\ndownload the file with: agent file get ${f.id} -o ${f.filename}`) } // Pull the bytes from the presigned S3 URL. Deliberately bare fetch (no -// bearer header — the signature in the URL is the credential). Assets are +// bearer header — the signature in the URL is the credential). Files are // ≤10 MiB, so buffering in memory is fine. async function downloadTo(url: string, path: string): Promise { const res = await fetch(url) diff --git a/src/commands/github.ts b/src/commands/github.ts index d121378..3536427 100644 --- a/src/commands/github.ts +++ b/src/commands/github.ts @@ -18,7 +18,7 @@ export function registerGithub(program: Command): void { .description('List the repositories the GitHub installation can reach'), 'repo', ), - 'GET /github/repos', + 'GET /integrations/github/repos', ) .option('--json', 'output raw JSON') .action(async (opts: { json?: boolean }) => { @@ -53,7 +53,7 @@ export function registerGithub(program: Command): void { ), 'member', ), - 'GET /github/members', + 'GET /integrations/github/members', ) .option('--json', 'output raw JSON') .action(async (opts: { json?: boolean }) => { diff --git a/src/commands/linear.ts b/src/commands/linear.ts index 842cf15..49af586 100644 --- a/src/commands/linear.ts +++ b/src/commands/linear.ts @@ -15,7 +15,7 @@ export function registerLinear(program: Command): void { .description('List the Linear teams, marking which have Ellipsis enabled'), 'team', ), - 'GET /linear/teams', + 'GET /integrations/linear/teams', ) .option('--json', 'output raw JSON') .action(async (opts: { json?: boolean }) => { diff --git a/src/commands/review.ts b/src/commands/review.ts index bd314e1..d62c415 100644 --- a/src/commands/review.ts +++ b/src/commands/review.ts @@ -3,44 +3,38 @@ import { existsSync, mkdirSync, writeFileSync } from 'node:fs' import { basename, dirname, extname } from 'node:path' import { ApiClient, ApiError } from '../lib/api' import { alsoKnownAs, apiRoutes } from '../lib/help' -import { createWipCommit, currentBranch, pushReviewBranch, repoFromCwd } from '../lib/laptop' +import { repoFromCwd } from '../lib/laptop' import { formatTs, printJson, printTable, relativeAge, runAction, usdFromMillicents } from '../lib/output' -import { resolveRepoFlag } from './config' import { watchSessionStreaming } from './session' -import type { - CodeReviewDefaultView, - CreateReviewRequest, - Finding, - Review, - ReviewScope, -} from '../lib/types' +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 — +// trigger one. // -// agent review 5975 an existing pull request -// agent review the work in your tree, right now +// agent review 5975 review that pull request // -// 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 always of an existing pull request: the range, the checkout, and +// the delivery all read PR state, so there is nothing to review without one. // -// 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. +// Which pipeline runs is not a parameter. It is resolved from the repository's +// committed `code_review.yaml` (see `agent review init`), the same way an +// automatic review resolves it. +// +// A review is a pipeline of stage sessions, not a single session: its id is a +// `crun_…`, and each stage's session id lives in `stages[]`. // How long to wait between REST polls when the stream isn't available. const FALLBACK_POLL_INTERVAL_SECONDS = 3 -// The conventional path for a pipeline file. Convention only: `kind:` decides. -const DEFAULT_PIPELINE_PATH = 'agents/code_review.yaml' +// The only paths a committed pipeline may live at, in the precedence order the +// server resolves them (CODE_REVIEW_CONFIG_PATHS). A file anywhere else is a +// hard sync error, never a silently-unused config. +const DEFAULT_PIPELINE_PATH = 'code_review.yaml' +const NESTED_PIPELINE_PATH = '.ellipsis/code_review.yaml' 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'), + program.command('review').description('Review a pull request on demand'), 'reviews', 'code-review', 'cr', @@ -48,35 +42,24 @@ export function registerReview(program: Command): void { apiRoutes( review - .command('start [pull-request]', { isDefault: true }) - .description('Review a pull request by number, or your working tree if you omit one'), + .command('start ', { isDefault: true }) + .description('Review a pull request by number'), 'POST /reviews', 'WS /sessions/{id}/stream', 'GET /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) => { + .action(async (pullRequest: string, 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 @@ -103,7 +86,7 @@ export function registerReview(program: Command): void { // 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. + // Same two-step `agent file get` uses. if (!opts.json) { console.log( `✓ reviewing ${request.owner}/${request.repo}#${started.pull_request.number} ` + @@ -182,7 +165,6 @@ export function registerReview(program: Command): void { }) registerReviewInit(review) - registerReviewDefaults(review) } // `agent review init`: the code review twin of `agent config init`. Scaffolds a @@ -199,14 +181,23 @@ function registerReviewInit(review: Command): void { .option('--force', 'overwrite the file if it already exists') .action((path: string | undefined, opts: { force?: boolean }) => { const target = path ?? DEFAULT_PIPELINE_PATH + // Refuse a path the server would reject at sync: writing a file that can + // never run is worse than not writing one, because it looks like it works. + if (target !== DEFAULT_PIPELINE_PATH && target !== NESTED_PIPELINE_PATH) { + console.error( + `error: a code review pipeline must live at '${DEFAULT_PIPELINE_PATH}' or ` + + `'${NESTED_PIPELINE_PATH}'; '${target}' is never used`, + ) + process.exitCode = 1 + return + } if (existsSync(target) && !opts.force) { console.error(`error: ${target} already exists (use --force to overwrite)`) process.exitCode = 1 return } - const name = basename(target, extname(target)) mkdirSync(dirname(target), { recursive: true }) - writeFileSync(target, starterPipeline(name, repoNameFromCwd())) + writeFileSync(target, starterPipeline(pipelineName())) console.log(`✓ wrote ${target}`) console.log( 'Commit it to your default branch. Ellipsis syncs code review pipelines from GitHub.', @@ -214,40 +205,42 @@ function registerReviewInit(review: Command): void { }) } -// The repository this pipeline should watch, as the bare name the schema takes -// (`repositories:` is scoped to your account, so it never carries the owner). -function repoNameFromCwd(): string | undefined { +// Both legal paths share one filename, so the file can't name the pipeline. +// Use the repository instead, falling back to the filename outside a checkout. +function pipelineName(): string { const repo = repoFromCwd(process.cwd()) - return repo ? repo.split('/')[1] : undefined + return repo ? `${repo.split('/')[1]} code review` : 'code review' } // A minimal valid pipeline. `ellipsis.kind` is the only field the schema // requires; every stage left unset runs the platform's default reviewers. -// Exported for tests. -export function starterPipeline(name: string, repository: string | undefined): string { +// +// Deliberately omits `pull_requests.repositories`: a file's location IS its +// scope now, so naming repositories is a sync error everywhere except the +// org-wide copy in the `.ellipsis` repository. Exported for tests. +export function starterPipeline(name: string): string { return `# Ellipsis code review pipeline. Commit this to your default branch; Ellipsis -# syncs it from GitHub. Valid locations: agents/, .agents/, ellipsis/, .ellipsis/ -# (any depth). The kind line is what makes this a review pipeline, not an agent. +# syncs it from GitHub. It must live at '${DEFAULT_PIPELINE_PATH}' or +# '${NESTED_PIPELINE_PATH}'; a pipeline anywhere else is never used. +# +# Where it sits decides what it reviews: in a normal repository it reviews that +# repository, and in your organization's '.ellipsis' repository it reviews every +# repository. The kind line is what makes this a review pipeline, not an agent. ellipsis: version: v1 kind: code_review name: ${name} description: What this pipeline reviews. -# Which pull requests this pipeline watches. Only one enabled pipeline may watch -# a given pull request, so name your repositories here. Leaving this out watches -# every repository in the account. -pull_requests: - repositories: - - ${repository ?? 'my-repo'} - # base: [main] - # draft: false - # paths: ["src/**"] - # for: { bots: false } +# Which pull requests to review. Omit this to review every pull request. +# pull_requests: +# base: [main] +# draft: false +# paths: ["src/**"] +# for: { bots: false } # Every stage is optional. With all of them unset, reviews run the platform -# default pipeline: three reviewer lenses (correctness, security, regression) -# and a gatekeeper that judges what they found. +# default pipeline: a pull request description writer plus one bug reviewer. # # review: # - name: migration-safety @@ -257,7 +250,8 @@ pull_requests: # pull_requests: # paths: ["sql/migrations/**"] # -# include_default_reviewers: true # run the built-in lenses as well +# Declaring a filter stage adds a gatekeeper that judges what the reviewers +# found. There is no gatekeeper unless you declare one. # # filter: # name: gatekeeper @@ -272,165 +266,11 @@ budget: ` } -// `agent review default`: which code review pipeline runs when an explicit -// review names none — a two-rung ladder (account default + per-repo defaults, -// repo wins) mirroring `agent config default`, with the same --repo -// semantics. Only explicit reviews read it: webhook reviews keep matching the -// pipelines' own `pull_requests:` filters. -function registerReviewDefaults(review: Command): void { - const defaults = apiRoutes( - alsoKnownAs( - review - .command('default') - .description('Show or set which code review pipeline runs when a review names none'), - 'defaults', - ), - 'GET /reviews/defaults', - ) - .option('--json', 'output raw JSON') - // Bare `agent review default`: the effective default for the repo you're - // standing in, computed locally from GET /reviews/defaults + the origin - // remote (the same ladder the server resolves at review start). - .action(async (opts: { json?: boolean }) => { - await runAction(async () => { - const rungs = await new ApiClient().listReviewDefaults() - const repo = repoFromCwd(process.cwd()) - const repoRung = repo - ? rungs.find((d) => d.repository?.toLowerCase() === repo.toLowerCase()) - : undefined - const accountRung = rungs.find((d) => d.repository === null) - const effective = repoRung ?? accountRung - if (opts.json) { - printJson({ repository: repo ?? null, effective: effective ?? null }) - return - } - if (!effective) { - console.log( - repo - ? `no default set for ${repo} or the account (reviews run the synced pipeline, or the platform defaults)` - : 'no account default set (reviews run the synced pipeline, or the platform defaults)', - ) - return - } - const rung = effective.repository - ? `repo default for ${effective.repository}` - : 'account default' - console.log( - `using pipeline "${defaultName(effective)}" (${rung})${brokenSuffix(effective)}`, - ) - }) - }) - - apiRoutes( - alsoKnownAs( - defaults - .command('list') - .description('List every default that is set, account rung and per-repo rungs'), - 'ls', - ), - 'GET /reviews/defaults', - ) - .option('--json', 'output raw JSON') - // The group also defines --json (for the bare view), and commander parses - // parent options even when they follow the subcommand name — so read the - // merged view, not just this command's own opts. - .action(async (_opts: { json?: boolean }, cmd: Command) => { - await runAction(async () => { - const rungs = await new ApiClient().listReviewDefaults() - if (cmd.optsWithGlobals().json) { - printJson(rungs) - return - } - if (rungs.length === 0) { - console.log( - 'No defaults set. Reviews run the synced pipeline, or the platform defaults.', - ) - return - } - printTable( - ['RUNG', 'PIPELINE', 'CONFIG ID', 'STATUS', 'UPDATED'], - rungs.map((d) => [ - d.repository ?? 'account', - d.config_name ?? '—', - d.config_id, - d.broken ? `broken: ${d.broken}` : 'ok', - formatTs(d.updated_at), - ]), - ) - }) - }) - - apiRoutes( - defaults - .command('set ') - .description('Set the account default code review pipeline, or a repo default with --repo'), - 'PUT /reviews/defaults', - ) - .option( - '-r, --repo [repository]', - 'target a repo rung: "owner/name", or no value for the repo you are standing in', - ) - .option('--json', 'output raw JSON') - .action( - async (configId: string, opts: { repo?: string | boolean; json?: boolean }, cmd: Command) => { - await runAction(async () => { - const repository = resolveRepoFlag(opts.repo) - const set = await new ApiClient().putReviewDefault({ - config_id: configId, - ...(repository ? { repository } : {}), - }) - if (cmd.optsWithGlobals().json) { - printJson(set) - return - } - const rung = set.repository ? `default for ${set.repository}` : 'account default' - console.log(`✓ set ${rung} to "${defaultName(set)}" (${set.config_id})`) - }) - }, - ) - - apiRoutes( - alsoKnownAs( - defaults - .command('clear') - .description('Clear the account default code review pipeline, or a repo default with --repo'), - 'rm', - 'delete', - ), - 'DELETE /reviews/defaults', - ) - .option( - '-r, --repo [repository]', - 'target a repo rung: "owner/name", or no value for the repo you are standing in', - ) - .action(async (opts: { repo?: string | boolean }) => { - await runAction(async () => { - const repository = resolveRepoFlag(opts.repo) - await new ApiClient().deleteReviewDefault(repository) - console.log(`✓ cleared ${repository ? `default for ${repository}` : 'account default'}`) - }) - }) -} - -function defaultName(d: CodeReviewDefaultView): string { - return d.config_name ?? d.config_id -} - -// A set-but-broken rung fails explicit reviews closed (never a silent run of -// a different pipeline), so surface it wherever the rung is shown. -function brokenSuffix(d: CodeReviewDefaultView): string { - return d.broken ? ` (broken: ${d.broken})` : '' -} - 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 @@ -446,41 +286,20 @@ interface ListOptions { json?: boolean } -// Assemble the request, doing the local path's git work when no pull request -// was named. Exported for tests. +// Assemble the request. Exported for tests. export function buildCreateRequest( - pullRequest: string | undefined, + pullRequest: string, opts: StartOptions, ): CreateReviewRequest { const cwd = opts.cwd ?? process.cwd() const repo = splitRepo(opts.repo ?? repoFromCwdOrThrow(cwd)) - const scope = buildScope(opts) - - const common = { + return { 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 } : {}), + scope: buildScope(opts), + pull_request_number: parsePullRequest(pullRequest), + post: opts.post, } - - 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: @@ -493,35 +312,15 @@ function buildScope(opts: StartOptions): ReviewScope { } } -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 + // The likeliest mistake is handing this a stage session id (or any other + // session id) instead of the review's own — 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 new Error(`no review with id ${reviewId} (a review id looks like crun_…)`) } throw err } @@ -617,9 +416,9 @@ export function parsePullRequest(raw: string): number { // `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} …"`, + `'${raw}' is not a pull request number. Pass a number (agent review 123). ` + + 'To run an agent with a prompt that starts with "review", quote it: ' + + `agent "review ${raw} …"`, ) } return Number.parseInt(match[1], 10) @@ -630,9 +429,3 @@ function parsePositiveInt(raw: string): number { 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/commands/sentry.ts b/src/commands/sentry.ts index 31af405..f182269 100644 --- a/src/commands/sentry.ts +++ b/src/commands/sentry.ts @@ -14,7 +14,7 @@ export function registerSentry(program: Command): void { 'org', 'organizations', ), - 'GET /sentry/organizations', + 'GET /integrations/sentry/organizations', ) .option('--json', 'output raw JSON') .action(async (opts: { json?: boolean }) => { diff --git a/src/commands/session.tsx b/src/commands/session.tsx index c863e29..c187bbe 100644 --- a/src/commands/session.tsx +++ b/src/commands/session.tsx @@ -326,7 +326,7 @@ export function registerSession(program: Command): void { '--since/--until accept ISO 8601 or "today", "yesterday", "N days ago".', ), 'GET /sessions', - 'GET /github/members to resolve --author', + 'GET /integrations/github/members to resolve --author', ) .option('-c, --config ', 'only sessions run by this saved agent config') .option( @@ -402,7 +402,7 @@ export function registerSession(program: Command): void { '--since/--until accept ISO 8601 or "today", "yesterday", "N days ago".', ), 'GET /sessions/search', - 'GET /github/members to resolve --author', + 'GET /integrations/github/members to resolve --author', ) .option( '-a, --author ', diff --git a/src/commands/slack.ts b/src/commands/slack.ts index 4d289ff..6eb0526 100644 --- a/src/commands/slack.ts +++ b/src/commands/slack.ts @@ -13,7 +13,7 @@ export function registerSlack(program: Command): void { slack.command('channels').description('List the channels in the Slack workspace'), 'channel', ), - 'GET /slack/channels', + 'GET /integrations/slack/channels', ) .option('--json', 'output raw JSON') .action(async (opts: { json?: boolean }) => { @@ -46,7 +46,7 @@ export function registerSlack(program: Command): void { .description('List the workspace members, with linked GitHub identities'), 'member', ), - 'GET /slack/members', + 'GET /integrations/slack/members', ) .option('--json', 'output raw JSON') .action(async (opts: { json?: boolean }) => { diff --git a/src/commands/template.ts b/src/commands/template.ts index 65b7748..b541eb9 100644 --- a/src/commands/template.ts +++ b/src/commands/template.ts @@ -16,7 +16,7 @@ export function registerTemplate(program: Command): void { .description('List the built-in agent templates and the slugs --template takes'), 'ls', ), - 'GET /templates', + 'GET /agents/templates', ) .option('--json', 'output raw JSON') .action(async (opts: { json?: boolean }) => { diff --git a/src/commands/variable.ts b/src/commands/variable.ts index 4d65aca..09de7ee 100644 --- a/src/commands/variable.ts +++ b/src/commands/variable.ts @@ -20,7 +20,7 @@ export function registerVariable(program: Command): void { variable.command('list').description('List the variable names (never their values)'), 'ls', ), - 'GET /variables', + 'GET /secrets', ) .option('--json', 'output raw JSON') .action(async (opts: { json?: boolean }) => { @@ -34,7 +34,7 @@ export function registerVariable(program: Command): void { variable .command('set [assignments...]') .description('Create or update variables, e.g. `set A=1 B=2`'), - 'PUT /variables', + 'PUT /secrets', ) .option('-f, --from-file ', 'load variables from a .env or .json file') .option('--json', 'output raw JSON') @@ -52,7 +52,7 @@ export function registerVariable(program: Command): void { apiRoutes( alsoKnownAs(variable.command('delete ').description('Delete a variable'), 'rm'), - 'DELETE /variables/{name}', + 'DELETE /secrets/{name}', ) .option('--json', 'output raw JSON') .action(async (name: string, opts: { json?: boolean }) => { diff --git a/src/lib/api.ts b/src/lib/api.ts index 3657065..9cb3e67 100644 --- a/src/lib/api.ts +++ b/src/lib/api.ts @@ -7,17 +7,16 @@ import type { AnalyticsMetricsQuery, AnalyticsPullRequestsQuery, AnalyticsReviewsQuery, - AssetView, BudgetSummary, CliAuthPoll, CliAuthStart, - CodeReviewDefaultView, CreateAgentConfigRequest, - CreateAssetRequest, - CreateAssetResponse, + CreateFileRequest, + CreateFileResponse, CreateReviewRequest, CreatedAgentConfig, - GetAssetResponse, + FileView, + GetFileResponse, GetAnalyticsMetricsResponse, GetAnalyticsPullRequestsResponse, GetAnalyticsReviewsResponse, @@ -31,9 +30,8 @@ import type { ListAgentSessionsQuery, ListAgentSessionsResponse, ListAgentTemplatesResponse, - ListAssetsQuery, - ListAssetsResponse, - ListCodeReviewDefaultsResponse, + ListFilesQuery, + ListFilesResponse, ListGithubMembersResponse, ListGithubRepositoriesResponse, ListLinearTeamsResponse, @@ -45,7 +43,6 @@ import type { ListSlackChannelsResponse, ListSlackMembersResponse, PutAgentDefaultRequest, - PutCodeReviewDefaultRequest, ReplayAgentSessionRequest, Review, SendSessionMessageRequest, @@ -298,42 +295,42 @@ export class ApiClient { ) } - // --------------------------------- assets -------------------------------- - // Agent asset storage: persist a file to the platform and get back an - // org-membership-gated link (documents/eng/AGENT_ASSET_STORAGE.md in the - // ellipsis repo). v1 is PNG-only with a 10 MiB cap, enforced server-side. + // --------------------------------- files --------------------------------- + // Agent file storage: persist a file to the platform and get back an + // org-membership-gated link. v1 is PNG-only with a 10 MiB cap, enforced + // server-side. - uploadAsset(req: CreateAssetRequest): Promise { - return this.request('POST', '/assets', req) + uploadFile(req: CreateFileRequest): Promise { + return this.request('POST', '/files', req) } - // Newest-first metadata for the credential's customer's assets. Metadata - // only — presigned download URLs are minted per explicit getAsset. - async listAssets(query?: ListAssetsQuery): Promise { - const res = await this.request( + // Newest-first metadata for the credential's customer's files. Metadata + // only — presigned download URLs are minted per explicit getFile. + async listFiles(query?: ListFilesQuery): Promise { + const res = await this.request( 'GET', - '/assets', + '/files', undefined, query as Record | undefined, ) - return res.assets + return res.files } // Metadata + the gated URL + a short-lived presigned `download_url`. To pull // the bytes locally, GET download_url immediately (it expires in ~60s; if it // lapses, just call this again for a fresh one). - getAsset(assetId: string): Promise { - return this.request('GET', `/assets/${encodeURIComponent(assetId)}`) + getFile(fileId: string): Promise { + return this.request('GET', `/files/${encodeURIComponent(fileId)}`) } - // Delete an asset: it disappears from every read path and its gated link + // Delete a file: it disappears from every read path and its gated link // stops resolving (the server soft-deletes; storage accounting keeps // charging for everything ever written). The // server returns 204 with an empty body on success; 404 when the id is // unknown to the credential's customer, 403 when the token isn't allowed to // delete (e.g. a sandbox token). - deleteAsset(assetId: string): Promise { - return this.request('DELETE', `/assets/${encodeURIComponent(assetId)}`) + deleteFile(fileId: string): Promise { + return this.request('DELETE', `/files/${encodeURIComponent(fileId)}`) } // -------------------------------- reviews -------------------------------- @@ -365,47 +362,21 @@ export class ApiClient { return res.reviews } - // --------------------------- review defaults ----------------------------- - // The default code review pipeline ladder (repo default -> account default), - // the code_review twin of the /defaults methods below and addressed the - // same way: `repository` is "owner/name" for a repo rung and null/omitted - // for the account rung — never a row id. Read by POST /reviews when no - // config is named; webhook reviews are unaffected. Mutations are refused - // for sandbox tokens (403). - - async listReviewDefaults(): Promise { - const res = await this.request( - 'GET', - '/reviews/defaults', - ) - return res.defaults - } - - putReviewDefault(req: PutCodeReviewDefaultRequest): Promise { - return this.request('PUT', '/reviews/defaults', req) - } - - // Clears a rung: the account default when `repository` is omitted, that - // repo's default otherwise. 404 when the rung isn't set. - deleteReviewDefault(repository?: string): Promise { - return this.request('DELETE', '/reviews/defaults', undefined, { repository }) - } - // ----------------------------- agent configs ---------------------------- async listAgentConfigs(): Promise { - const res = await this.request('GET', '/configs') + const res = await this.request('GET', '/agents/configs') return res.configs } // Opens a pull request that adds the config's YAML to the repo's agents/ // directory; the agent goes live once it merges and syncs. createAgentConfig(req: CreateAgentConfigRequest): Promise { - return this.request('POST', '/configs', req) + return this.request('POST', '/agents/configs', req) } getAgentConfig(configId: string): Promise { - return this.request('GET', `/configs/${encodeURIComponent(configId)}`) + return this.request('GET', `/agents/configs/${encodeURIComponent(configId)}`) } // ------------------------------ defaults -------------------------------- @@ -415,18 +386,18 @@ export class ApiClient { // refused for sandbox tokens (403). async listAgentDefaults(): Promise { - const res = await this.request('GET', '/defaults') + const res = await this.request('GET', '/agents/defaults') return res.defaults } putAgentDefault(req: PutAgentDefaultRequest): Promise { - return this.request('PUT', '/defaults', req) + return this.request('PUT', '/agents/defaults', req) } // Clears a rung: the account default when `repository` is omitted, that // repo's default otherwise. 404 when the rung isn't set. deleteAgentDefault(repository?: string): Promise { - return this.request('DELETE', '/defaults', undefined, { repository }) + return this.request('DELETE', '/agents/defaults', undefined, { repository }) } // ------------------------------- variables -------------------------------- @@ -434,14 +405,14 @@ export class ApiClient { // mutation), so callers can render the resulting state. async listSandboxVariables(): Promise { - const res = await this.request('GET', '/variables') + const res = await this.request('GET', '/secrets') return res.variables } async putSandboxVariables( variables: SandboxVariableInput[], ): Promise { - const res = await this.request('PUT', '/variables', { + const res = await this.request('PUT', '/secrets', { variables, }) return res.variables @@ -450,7 +421,7 @@ export class ApiClient { async deleteSandboxVariable(name: string): Promise { const res = await this.request( 'DELETE', - `/variables/${encodeURIComponent(name)}`, + `/secrets/${encodeURIComponent(name)}`, ) return res.variables } @@ -467,12 +438,12 @@ export class ApiClient { // ---------------------------- agent templates --------------------------- async listAgentTemplates(): Promise { - const res = await this.request('GET', '/templates') + const res = await this.request('GET', '/agents/templates') return res.templates } getAgentTemplate(slug: string): Promise { - return this.request('GET', `/templates/${encodeURIComponent(slug)}`) + return this.request('GET', `/agents/templates/${encodeURIComponent(slug)}`) } // ------------------------ integration discovery ------------------------- @@ -485,38 +456,38 @@ export class ApiClient { } listGithubRepositories(): Promise { - return this.request('GET', '/github/repos') + return this.request('GET', '/integrations/github/repos') } listGithubMembers(): Promise { - return this.request('GET', '/github/members') + return this.request('GET', '/integrations/github/members') } listSlackChannels(): Promise { - return this.request('GET', '/slack/channels') + return this.request('GET', '/integrations/slack/channels') } listSlackMembers(): Promise { - return this.request('GET', '/slack/members') + return this.request('GET', '/integrations/slack/members') } listLinearTeams(): Promise { - return this.request('GET', '/linear/teams') + return this.request('GET', '/integrations/linear/teams') } listSentryOrganizations(): Promise { - return this.request('GET', '/sentry/organizations') + return this.request('GET', '/integrations/sentry/organizations') } // --------------------------- device-code auth --------------------------- // Unauthenticated: the CLI has no credential yet — that's what it's obtaining. startCliAuth(): Promise { - return this.request('POST', '/cli-auth/start') + return this.request('POST', '/auth/cli/start') } pollCliAuth(deviceCode: string): Promise { - return this.request('POST', '/cli-auth/poll', { device_code: deviceCode }) + return this.request('POST', '/auth/cli/poll', { device_code: deviceCode }) } } @@ -552,20 +523,30 @@ export function buildQuery(query?: Record): string { return qs ? `?${qs}` : '' } -// Pull the server's `{"detail": ...}` message and request id off a non-2xx -// response. The detail keeps `agent` error output actionable instead of bare -// codes; the id comes from the `X-Request-ID` header (set on every API response) -// and falls back to the `request_id` field the server's 500 handler includes in -// its body, so an error carries something we can grep our logs for. +// Pull the server's message and request id off a non-2xx response. The message +// keeps `agent` error output actionable instead of bare codes; the id comes from +// the `X-Request-ID` header (set on every API response) and falls back to the +// body, so an error carries something we can grep our logs for. +// +// The public API answers with `{"error": {code, message, request_id}}`. FastAPI's +// own validation and auth rejections still use `{"detail": ...}`, so both shapes +// are read: an unrecognized body would otherwise degrade to a bare status line. export async function parseErrorResponse( res: Response, ): Promise<{ detail: string; requestId?: string }> { const headerRequestId = res.headers.get('x-request-id') ?? undefined try { - const body = (await res.json()) as { detail?: unknown; request_id?: unknown } + const body = (await res.json()) as { + error?: { code?: unknown; message?: unknown; request_id?: unknown } + detail?: unknown + request_id?: unknown + } + const bodyRequestId = body.error?.request_id ?? body.request_id const requestId = - headerRequestId ?? - (typeof body.request_id === 'string' ? body.request_id : undefined) + headerRequestId ?? (typeof bodyRequestId === 'string' ? bodyRequestId : undefined) + if (typeof body.error?.message === 'string') { + return { detail: body.error.message, requestId } + } if (typeof body.detail === 'string') return { detail: body.detail, requestId } if (body.detail) return { detail: JSON.stringify(body.detail), requestId } return { detail: res.statusText, requestId } diff --git a/src/lib/help.ts b/src/lib/help.ts index 6c9ed27..7c07203 100644 --- a/src/lib/help.ts +++ b/src/lib/help.ts @@ -20,7 +20,7 @@ function withoutAliases(term: string, cmd: Command): string { const TOP_LEVEL_GROUPS: ReadonlyArray<{ title: string; commands: readonly string[] }> = [ { title: 'Sessions', commands: ['session', 'review'] }, { title: 'Agents', commands: ['config', 'model', 'template'] }, - { title: 'Platform', commands: ['variable', 'asset', 'hook'] }, + { title: 'Platform', commands: ['variable', 'file', 'hook'] }, { title: 'Integrations', commands: ['integration', 'github', 'slack', 'linear', 'sentry'] }, { title: 'Spend', commands: ['budget', 'usage', 'analytics'] }, { title: 'Account', commands: ['install', 'login', 'logout', 'me', 'host', 'ping'] }, diff --git a/src/lib/laptop.ts b/src/lib/laptop.ts index 907a7dd..2e9a3d0 100644 --- a/src/lib/laptop.ts +++ b/src/lib/laptop.ts @@ -443,28 +443,3 @@ export function pushHandoffRef(cwd: string, sha: string): string { 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/output.ts b/src/lib/output.ts index 8230214..c6e4631 100644 --- a/src/lib/output.ts +++ b/src/lib/output.ts @@ -106,7 +106,7 @@ const UPGRADE_HINT_EXEMPT = new Set([ 404, // missing resource / integration not connected (mapped per-command) 408, // transient timeout 409, // documented conflicts (closed session, sandbox not running) - 413, // payload over a server-enforced cap (e.g. the 10 MiB asset limit) + 413, // payload over a server-enforced cap (e.g. the 10 MiB file limit) 429, // handled above: rate limit detail printed bare 502, // gateway trouble 503, // API down or overloaded diff --git a/src/lib/types.ts b/src/lib/types.ts index da884d0..c8a4b01 100644 --- a/src/lib/types.ts +++ b/src/lib/types.ts @@ -306,34 +306,6 @@ export interface PutAgentDefaultRequest { config_id: string } -// One rung of the default code review pipeline ladder (GET -// /reviews/defaults) — the code_review twin of AgentDefaultView, pointing -// at a synced `kind: code_review` pipeline (crcfg_…) instead of an agent -// config. Read by POST /reviews when no config is named: repo default -> -// account default -> the oldest synced pipeline -> the platform defaults. -export interface CodeReviewDefaultView { - id: string - repository: string | null - config_id: string - // The pointed-at pipeline's name; null when the pipeline is gone (see broken). - config_name: string | null - // Why this rung can't serve reviews (config_deleted | config_disabled | - // config_sync_error | repo_inaccessible); null when healthy. - broken: string | null - updated_at: string -} - -export interface ListCodeReviewDefaultsResponse { - defaults: CodeReviewDefaultView[] -} - -// Body of PUT /reviews/defaults: point a rung at a pipeline. `repository` -// omitted sets the account default; "owner/name" sets that repo's default. -export interface PutCodeReviewDefaultRequest { - repository?: string - config_id: string -} - // Create-config payload for POST /configs. Exactly one of `config` (inline) // or `template_id` (a gallery template slug). `repository` is a bare repo name // in the caller's account — the owner is always the account. @@ -856,13 +828,13 @@ export interface GetAnalyticsReviewsResponse { } } -// --------------------------------- assets -------------------------------- -// Agent asset storage (ellipsis: documents/eng/AGENT_ASSET_STORAGE.md): files -// an agent persists beyond its sandbox's lifetime — v1 is PNG screenshots -// posted as org-gated links on PRs. Mirrors assets_service.py. +// --------------------------------- files --------------------------------- +// Agent file storage: files an agent persists beyond its sandbox's lifetime — +// v1 is PNG screenshots posted as org-gated links on PRs. Mirrors +// files_service.py. -// Caller-facing asset metadata — no storage internals (S3 key, sha, owner). -export interface AssetView { +// Caller-facing file metadata — no storage internals (S3 key, sha, owner). +export interface FileView { id: string filename: string content_type: string @@ -872,9 +844,9 @@ export interface AssetView { agent_session_id: string | null } -export interface CreateAssetRequest { +export interface CreateFileRequest { // Original basename, display only (the S3 key derives from the server-side - // asset id, never from this). + // file id, never from this). filename: string // v1: must be image/png; the server magic-byte-checks the decoded bytes. content_type: string @@ -883,25 +855,25 @@ export interface CreateAssetRequest { data_b64: string } -export interface CreateAssetResponse { - asset: AssetView - // The fully-formed org-gated dashboard URL (app.ellipsis.dev/assets/{id}) — +export interface CreateFileResponse { + file: FileView + // The fully-formed org-gated dashboard URL (app.ellipsis.dev/files/{id}) — // the link an agent pastes into a PR comment. url: string } -export interface ListAssetsQuery { +export interface ListFilesQuery { // Scope to one run's uploads. agent_session_id?: string limit?: number } -export interface ListAssetsResponse { - assets: AssetView[] +export interface ListFilesResponse { + files: FileView[] } -export interface GetAssetResponse { - asset: AssetView +export interface GetFileResponse { + file: FileView // The gated dashboard URL (same link the upload returned). url: string // Short-lived (60s) presigned S3 GET for the actual bytes — fetch it diff --git a/src/ui/SessionsApp.tsx b/src/ui/SessionsApp.tsx index b7c2ef1..6cd321a 100644 --- a/src/ui/SessionsApp.tsx +++ b/src/ui/SessionsApp.tsx @@ -174,6 +174,15 @@ export function SessionsApp(props: SessionsAppProps): React.ReactElement { // lag, or attributed differently); merged into the list until it does. const [localSessions, setLocalSessions] = useState([]) + // The last API failure from any background call (the poll, the composer's + // pickers). Those calls have no output of their own, so without this a broken + // route or a dead token just shows an empty list. Rendered in the nav hint + // row, which is the one line always on screen. + const [apiError, setApiError] = useState(null) + const reportApiError = useCallback((label: string, err: unknown): void => { + setApiError(`${label}: ${err instanceof ApiError ? err.detail : (err as Error).message}`) + }, []) + const poll = useCallback(async (): Promise => { try { const listed = await api.listAgentSessions({ @@ -192,10 +201,13 @@ export function SessionsApp(props: SessionsAppProps): React.ReactElement { setSessions(listed) setLocalSessions((prev) => prev.filter((l) => !listed.some((s) => s.id === l.id))) setPolledOnce(true) - } catch { - // Transient poll failure — keep the previous list; the next tick retries. + setApiError(null) + } catch (err) { + // Keep the previous list (the next tick retries), but say so: a poll that + // fails every tick is a broken session list, not a blip. + reportApiError('sessions', err) } - }, [api, authorId]) + }, [api, authorId, reportApiError]) // The poll only feeds the nav's rows and attention dots; with the bar // hidden there is nothing on screen it could update. @@ -315,16 +327,25 @@ export function SessionsApp(props: SessionsAppProps): React.ReactElement { void api .listAgentConfigs() .then((rows) => setConfigs(rows.filter((c) => !c.deleted))) - .catch(() => setConfigs([])) + .catch((err) => { + setConfigs([]) + reportApiError('agent configs', err) + }) void api .listGithubRepositories() .then((r) => setRepos(r.repositories.map((repo) => repo.full_name))) - .catch(() => setRepos([])) + .catch((err) => { + setRepos([]) + reportApiError('repositories', err) + }) void api .listSupportedModels() .then(setModels) - .catch(() => setModels([])) - }, [mainPane.type, api]) + .catch((err) => { + setModels([]) + reportApiError('models', err) + }) + }, [mainPane.type, api, reportApiError]) const startSession = useCallback( async (prompt: string, choices: ComposerChoices): Promise => { @@ -643,13 +664,22 @@ export function SessionsApp(props: SessionsAppProps): React.ReactElement { {/* Absorbs the rows a short list leaves empty, keeping the hint on the band's bottom edge. */} - - {navFocused - ? `↑↓ move · enter open · n new · esc chat · q quit${ - win.end < rows.length ? ` · ${rows.length - win.end} more below` : '' - }` - : '↓/esc: sessions'} - + {/* An API failure replaces the key hints rather than sharing the line: + the hints are always recoverable from muscle memory, a swallowed error + is not. */} + {apiError ? ( + + {`✗ ${apiError}`} + + ) : ( + + {navFocused + ? `↑↓ move · enter open · n new · esc chat · q quit${ + win.end < rows.length ? ` · ${rows.length - win.end} more below` : '' + }` + : '↓/esc: sessions'} + + )} ) diff --git a/test/api.test.ts b/test/api.test.ts index 9f63806..bf511f1 100644 --- a/test/api.test.ts +++ b/test/api.test.ts @@ -160,7 +160,7 @@ describe('ApiClient sandbox variables', () => { const out = await new ApiClient('http://api.test', 't').listSandboxVariables() expect(out).toEqual([{ name: 'A', created_at: '', updated_at: '' }]) - expect(fetchMock.mock.calls[0][0]).toBe('http://api.test/variables') + expect(fetchMock.mock.calls[0][0]).toBe('http://api.test/secrets') expect((fetchMock.mock.calls[0][1] as RequestInit).method).toBe('GET') }) @@ -172,7 +172,7 @@ describe('ApiClient sandbox variables', () => { await new ApiClient('http://api.test', 't').putSandboxVariables([{ name: 'TOKEN', value: 'x' }]) const [url, init] = fetchMock.mock.calls[0] - expect(url).toBe('http://api.test/variables') + expect(url).toBe('http://api.test/secrets') expect((init as RequestInit).method).toBe('PUT') expect(JSON.parse((init as RequestInit).body as string)).toEqual({ variables: [{ name: 'TOKEN', value: 'x' }], @@ -187,72 +187,11 @@ describe('ApiClient sandbox variables', () => { await new ApiClient('http://api.test', 't').deleteSandboxVariable('MY/VAR') const [url, init] = fetchMock.mock.calls[0] - expect(url).toBe('http://api.test/variables/MY%2FVAR') + expect(url).toBe('http://api.test/secrets/MY%2FVAR') expect((init as RequestInit).method).toBe('DELETE') }) }) -describe('ApiClient review defaults', () => { - afterEach(() => vi.unstubAllGlobals()) - - const rung = { - id: 'crdef_1', - repository: null, - config_id: 'crcfg_1', - config_name: 'Team reviewer', - broken: null, - updated_at: '', - } - - it('lists rungs and unwraps the response envelope', async () => { - const fetchMock = vi.fn( - async () => new Response(JSON.stringify({ defaults: [rung] }), { status: 200 }), - ) - vi.stubGlobal('fetch', fetchMock) - - const out = await new ApiClient('http://api.test', 't').listReviewDefaults() - expect(out).toEqual([rung]) - expect(fetchMock.mock.calls[0][0]).toBe('http://api.test/reviews/defaults') - expect((fetchMock.mock.calls[0][1] as RequestInit).method).toBe('GET') - }) - - it('PUTs the rung: account when repository is omitted, repo when named', async () => { - const fetchMock = vi.fn(async () => new Response(JSON.stringify(rung), { status: 200 })) - vi.stubGlobal('fetch', fetchMock) - - await new ApiClient('http://api.test', 't').putReviewDefault({ config_id: 'crcfg_1' }) - let [url, init] = fetchMock.mock.calls[0] - expect(url).toBe('http://api.test/reviews/defaults') - expect((init as RequestInit).method).toBe('PUT') - expect(JSON.parse((init as RequestInit).body as string)).toEqual({ config_id: 'crcfg_1' }) - - await new ApiClient('http://api.test', 't').putReviewDefault({ - config_id: 'crcfg_1', - repository: 'acme/api', - }) - ;[url, init] = fetchMock.mock.calls[1] - expect(JSON.parse((init as RequestInit).body as string)).toEqual({ - config_id: 'crcfg_1', - repository: 'acme/api', - }) - }) - - it('clears a rung via query param, omitted for the account rung', async () => { - const fetchMock = vi.fn(async () => new Response(null, { status: 204 })) - vi.stubGlobal('fetch', fetchMock) - - const api = new ApiClient('http://api.test', 't') - await api.deleteReviewDefault() - expect(fetchMock.mock.calls[0][0]).toBe('http://api.test/reviews/defaults') - expect((fetchMock.mock.calls[0][1] as RequestInit).method).toBe('DELETE') - - await api.deleteReviewDefault('acme/api') - expect(fetchMock.mock.calls[1][0]).toBe( - 'http://api.test/reviews/defaults?repository=acme%2Fapi', - ) - }) -}) - describe('getSessionLog', () => { afterEach(() => vi.unstubAllGlobals()) @@ -328,7 +267,7 @@ describe('agent templates', () => { const out = await new ApiClient('http://api.test', 't').listAgentTemplates() expect(out.map((t) => t.slug)).toEqual(['a', 'b']) - expect(fetchMock.mock.calls[0][0]).toBe('http://api.test/templates') + expect(fetchMock.mock.calls[0][0]).toBe('http://api.test/agents/templates') }) it('fetches a single template by slug (encoded)', async () => { @@ -340,7 +279,7 @@ describe('agent templates', () => { const out = await new ApiClient('http://api.test', 't').getAgentTemplate('ci-failure-triager') expect(out.yaml).toBe('x') - expect(fetchMock.mock.calls[0][0]).toBe('http://api.test/templates/ci-failure-triager') + expect(fetchMock.mock.calls[0][0]).toBe('http://api.test/agents/templates/ci-failure-triager') }) }) @@ -390,7 +329,7 @@ describe('createAgentConfig', () => { }) expect(out.pull_request_url).toBe('https://github.com/octocat/api/pull/7') const [url, init] = fetchMock.mock.calls[0] - expect(url).toBe('http://api.test/configs') + expect(url).toBe('http://api.test/agents/configs') expect((init as RequestInit).method).toBe('POST') expect(JSON.parse((init as RequestInit).body as string)).toEqual({ template_id: 'ci-failure-triager', @@ -399,7 +338,7 @@ describe('createAgentConfig', () => { }) }) -describe('ApiClient assets', () => { +describe('ApiClient files', () => { afterEach(() => vi.unstubAllGlobals()) it('POSTs the upload payload and returns the gated URL', async () => { @@ -407,22 +346,22 @@ describe('ApiClient assets', () => { async () => new Response( JSON.stringify({ - asset: { id: 'a1', filename: 'shot.png' }, - url: 'https://app.ellipsis.dev/assets/a1', + file: { id: 'a1', filename: 'shot.png' }, + url: 'https://app.ellipsis.dev/files/a1', }), { status: 201 }, ), ) vi.stubGlobal('fetch', fetchMock) - const out = await new ApiClient('http://api.test', 't').uploadAsset({ + const out = await new ApiClient('http://api.test', 't').uploadFile({ filename: 'shot.png', content_type: 'image/png', data_b64: 'aGk=', }) - expect(out.url).toBe('https://app.ellipsis.dev/assets/a1') + expect(out.url).toBe('https://app.ellipsis.dev/files/a1') const [url, init] = fetchMock.mock.calls[0] - expect(url).toBe('http://api.test/assets') + expect(url).toBe('http://api.test/files') expect((init as RequestInit).method).toBe('POST') expect(JSON.parse((init as RequestInit).body as string)).toEqual({ filename: 'shot.png', @@ -431,45 +370,45 @@ describe('ApiClient assets', () => { }) }) - it('lists assets, unwrapping the envelope and passing filters as query', async () => { + it('lists files, unwrapping the envelope and passing filters as query', async () => { const fetchMock = vi.fn( - async () => new Response(JSON.stringify({ assets: [{ id: 'a1' }] }), { status: 200 }), + async () => new Response(JSON.stringify({ files: [{ id: 'a1' }] }), { status: 200 }), ) vi.stubGlobal('fetch', fetchMock) - const out = await new ApiClient('http://api.test', 't').listAssets({ + const out = await new ApiClient('http://api.test', 't').listFiles({ agent_session_id: 'session_1', limit: 5, }) expect(out).toEqual([{ id: 'a1' }]) expect(fetchMock.mock.calls[0][0]).toBe( - 'http://api.test/assets?agent_session_id=session_1&limit=5', + 'http://api.test/files?agent_session_id=session_1&limit=5', ) }) - it('URL-encodes the asset id on get', async () => { + it('URL-encodes the file id on get', async () => { const fetchMock = vi.fn( async () => new Response( - JSON.stringify({ asset: { id: 'a/1' }, url: 'u', download_url: 'd' }), + JSON.stringify({ file: { id: 'a/1' }, url: 'u', download_url: 'd' }), { status: 200 }, ), ) vi.stubGlobal('fetch', fetchMock) - const out = await new ApiClient('http://api.test', 't').getAsset('a/1') + const out = await new ApiClient('http://api.test', 't').getFile('a/1') expect(out.download_url).toBe('d') - expect(fetchMock.mock.calls[0][0]).toBe('http://api.test/assets/a%2F1') + expect(fetchMock.mock.calls[0][0]).toBe('http://api.test/files/a%2F1') }) - it('DELETEs the asset id (encoded) and tolerates a 204 empty body', async () => { + it('DELETEs the file id (encoded) and tolerates a 204 empty body', async () => { const fetchMock = vi.fn(async () => new Response(null, { status: 204 })) vi.stubGlobal('fetch', fetchMock) - const out = await new ApiClient('http://api.test', 't').deleteAsset('a/1') + const out = await new ApiClient('http://api.test', 't').deleteFile('a/1') expect(out).toBeUndefined() const [url, init] = fetchMock.mock.calls[0] - expect(url).toBe('http://api.test/assets/a%2F1') + expect(url).toBe('http://api.test/files/a%2F1') expect((init as RequestInit).method).toBe('DELETE') }) }) diff --git a/test/discovery.test.ts b/test/discovery.test.ts index 94428df..4b7f6d2 100644 --- a/test/discovery.test.ts +++ b/test/discovery.test.ts @@ -15,12 +15,20 @@ describe('integration discovery endpoints', () => { it('hits the provider-namespaced paths', async () => { const cases: Array<[(api: ApiClient) => Promise, string, unknown]> = [ [(api) => api.getIntegrations(), '/integrations', { sentry: [] }], - [(api) => api.listGithubRepositories(), '/github/repos', { repositories: [] }], - [(api) => api.listGithubMembers(), '/github/members', { members: [] }], - [(api) => api.listSlackChannels(), '/slack/channels', { channels: [] }], - [(api) => api.listSlackMembers(), '/slack/members', { members: [] }], - [(api) => api.listLinearTeams(), '/linear/teams', { teams: [] }], - [(api) => api.listSentryOrganizations(), '/sentry/organizations', { organizations: [] }], + [ + (api) => api.listGithubRepositories(), + '/integrations/github/repos', + { repositories: [] }, + ], + [(api) => api.listGithubMembers(), '/integrations/github/members', { members: [] }], + [(api) => api.listSlackChannels(), '/integrations/slack/channels', { channels: [] }], + [(api) => api.listSlackMembers(), '/integrations/slack/members', { members: [] }], + [(api) => api.listLinearTeams(), '/integrations/linear/teams', { teams: [] }], + [ + (api) => api.listSentryOrganizations(), + '/integrations/sentry/organizations', + { organizations: [] }, + ], ] for (const [call, path, body] of cases) { const fetchMock = stub(body) diff --git a/test/asset.test.ts b/test/file.test.ts similarity index 92% rename from test/asset.test.ts rename to test/file.test.ts index c44b8fb..a7761e9 100644 --- a/test/asset.test.ts +++ b/test/file.test.ts @@ -1,9 +1,9 @@ import { describe, expect, it } from 'vitest' import { - MAX_ASSET_SIZE_BYTES, + MAX_FILE_SIZE_BYTES, buildUploadRequest, formatSize, -} from '../src/commands/asset' +} from '../src/commands/file' const PNG_MAGIC = Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]) @@ -31,14 +31,14 @@ describe('buildUploadRequest', () => { }) it('rejects files over the 10 MiB cap, with sizes in the message', () => { - const big = Buffer.concat([PNG_MAGIC, Buffer.alloc(MAX_ASSET_SIZE_BYTES)]) - expect(() => buildUploadRequest('big.png', big)).toThrow(/10\.0 MiB per asset/) + const big = Buffer.concat([PNG_MAGIC, Buffer.alloc(MAX_FILE_SIZE_BYTES)]) + expect(() => buildUploadRequest('big.png', big)).toThrow(/10\.0 MiB per file/) }) it('accepts a PNG exactly at the cap', () => { const atCap = Buffer.concat([ PNG_MAGIC, - Buffer.alloc(MAX_ASSET_SIZE_BYTES - PNG_MAGIC.length), + Buffer.alloc(MAX_FILE_SIZE_BYTES - PNG_MAGIC.length), ]) expect(buildUploadRequest('cap.png', atCap).content_type).toBe('image/png') }) diff --git a/test/output.test.ts b/test/output.test.ts index d8f31c2..a5b14a0 100644 --- a/test/output.test.ts +++ b/test/output.test.ts @@ -74,7 +74,7 @@ describe('friendlyErrorMessage', () => { const err = new ApiError( 429, 'POST', - '/assets', + '/files', 'Asset limit reached: your organization is storing 50 of 50 assets. ' + 'Delete assets you no longer need, or email team@ellipsis.dev to raise the limit.', ) diff --git a/test/review.test.ts b/test/review.test.ts index 9ce8669..fa6ae96 100644 --- a/test/review.test.ts +++ b/test/review.test.ts @@ -11,7 +11,6 @@ import { splitRepo, starterPipeline, } 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 @@ -72,19 +71,6 @@ describe('buildCreateRequest — an existing pull request', () => { 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( @@ -93,76 +79,14 @@ describe('buildCreateRequest — an existing pull request', () => { }) }) -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/, - ) - }) - +describe('buildCreateRequest — repository resolution', () => { it('needs a repo when there is no git remote to infer one from', () => { expect(() => - buildCreateRequest(undefined, { ...START_DEFAULTS, cwd: mkdtempSync(join(tmpdir(), 'bare-')) }), + buildCreateRequest('123', { ...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) @@ -229,22 +153,37 @@ describe('formatFinding', () => { describe('starterPipeline', () => { it('marks the file as a pipeline, not an agent', () => { - expect(starterPipeline('code_review', 'cli')).toContain('kind: code_review') + expect(starterPipeline('cli code review')).toContain('kind: code_review') }) it('parses as YAML and only sets keys the schema allows', () => { - const parsed = parse(starterPipeline('code_review', 'cli')) as Record - expect(Object.keys(parsed).sort()).toEqual(['budget', 'ellipsis', 'pull_requests']) + const parsed = parse(starterPipeline('cli code review')) as Record + expect(Object.keys(parsed).sort()).toEqual(['budget', 'ellipsis']) expect(parsed.ellipsis).toMatchObject({ version: 'v1', kind: 'code_review' }) - expect(parsed.pull_requests).toEqual({ repositories: ['cli'] }) }) - it('names the pipeline after the file, so a second file does not collide', () => { - const parsed = parse(starterPipeline('backend', 'cli')) as { ellipsis: { name: string } } - expect(parsed.ellipsis.name).toBe('backend') + // Location is the scope now, so naming repositories is a sync error anywhere + // but the org-wide copy — the scaffold must never emit the key. + it('omits pull_requests.repositories, which would be a sync error', () => { + expect(starterPipeline('cli code review')).not.toContain('repositories:') + }) + + // Deleted from the schema, which forbids unknown keys. + it('omits include_default_reviewers', () => { + expect(starterPipeline('cli code review')).not.toContain('include_default_reviewers') + }) + + it('names the pipeline so a reader knows what it covers', () => { + const parsed = parse(starterPipeline('backend code review')) as { + ellipsis: { name: string } + } + expect(parsed.ellipsis.name).toBe('backend code review') }) - it('falls back to a placeholder repository outside a git checkout', () => { - expect(starterPipeline('code_review', undefined)).toContain('- my-repo') + it('documents both legal paths and no others', () => { + const text = starterPipeline('cli code review') + expect(text).toContain('code_review.yaml') + expect(text).toContain('.ellipsis/code_review.yaml') + expect(text).not.toContain('agents/') }) })