Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion packages/cli/src/annotation/runAnnotation.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import type { AnnotationSubmission } from '@contextbridge/shared/annotationSchema';
import { createDeferred } from '@contextbridge/shared/testHelpers';
import { describe, expect, it } from 'bun:test';
import { annotationArgs } from '#src/testFactories.ts';
import { annotationArgs, environment } from '#src/testFactories.ts';
import { createAnnotationDependencies, createStubContext } from '#src/testHelpers/index.ts';
import { runAnnotation } from './runAnnotation.ts';

Expand All @@ -18,10 +18,29 @@ describe('runAnnotation', () => {
expect(deps.payloads).toEqual([
{ content: '# Plan', title: 'Plan', contentKind: 'plan', metadata: { entrypoint: 'plan_command' } },
]);
expect(deps.port).toBeUndefined();
expect(deps.closeCount).toBe(1);
expect(deps.sigintHandlerRemoved).toBe(true);
});

it('uses CONTEXTBRIDGE_PORT when no explicit port is supplied', async () => {
const { context } = createStubContext({ env: environment.build({ CONTEXTBRIDGE_PORT: 3456 }) });
const deps = createAnnotationDependencies();

await runAnnotation(context, annotationArgs.build(), deps);

expect(deps.port).toBe(3456);
});

it('uses the explicit port before CONTEXTBRIDGE_PORT', async () => {
const { context } = createStubContext({ env: environment.build({ CONTEXTBRIDGE_PORT: 3456 }) });
const deps = createAnnotationDependencies();

await runAnnotation(context, annotationArgs.build({ port: 3000 }), deps);

expect(deps.port).toBe(3000);
});

it('captures plan-review lifecycle analytics around a successful review', async () => {
const { context, analytics } = createStubContext();
const deps = createAnnotationDependencies();
Expand Down
10 changes: 7 additions & 3 deletions packages/cli/src/annotation/runAnnotation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ export interface RunAnnotationArgs {
content: string;
contentKind: ContentKind;
entrypoint: AnnotationEntrypoint;
port?: number;
sourcePath?: string;
}

Expand All @@ -35,6 +36,7 @@ export interface AnnotationDependencies {
html: Promise<string>;
payload: AnnotationPayload;
config: FrontendConfig;
port?: number;
checkForUpdate?: () => Promise<UpdateNotice | null>;
},
): RunningServer;
Expand All @@ -46,7 +48,8 @@ export async function runAnnotation(
args: RunAnnotationArgs,
deps: AnnotationDependencies = defaultAnnotationDependencies,
): Promise<AnnotationSubmission> {
const { analytics, logger, openUrl, frontendConfig, updater } = ctx;
const { analytics, logger, openUrl, frontendConfig, updater, env } = ctx;
const { port = env.CONTEXTBRIDGE_PORT } = args;
const startedAt = nowInstant();

const payload: AnnotationPayload = {
Expand Down Expand Up @@ -81,6 +84,7 @@ export async function runAnnotation(
html: htmlPromise,
payload,
config: frontendConfig,
port,
checkForUpdate: () => updater.checkForUpdate().catch(() => null),
});
removeSigintHandler = deps.registerSigintHandler(() => {
Expand Down Expand Up @@ -120,8 +124,8 @@ export async function runAnnotation(

const defaultAnnotationDependencies: AnnotationDependencies = {
loadHtml: () => import('./bundledAnnotationHtml.ts').then((m) => m.bundledAnnotationHtml),
startReviewServer: (ctx, { html, payload, config, checkForUpdate }) =>
startServer(ctx, { html, payload, config, checkForUpdate }),
startReviewServer: (ctx, { html, payload, config, port, checkForUpdate }) =>
startServer(ctx, { html, payload, config, port, checkForUpdate }),
registerSigintHandler: (handler) => {
process.on('SIGINT', handler);
return () => {
Expand Down
7 changes: 7 additions & 0 deletions packages/cli/src/cli.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,13 @@ describe('runCli', () => {
expect(io.stderr.text()).toContain('--bogus');
});

it.each(['abc', '3000.5', '0', '-1', '65536'])('rejects invalid plan --port value %s', async (port) => {
const { context, io } = createStubContext();
const exitCode = await runCli(context, ['plan', '--port', port]);
expect(exitCode).not.toBe(0);
expect(io.stderr.text()).toContain('port must be an integer between 1 and 65535');
});

// Open-handler behavior lives in open.test.ts. Here we only verify argv
// routes into the open subcommand — an unknown flag on `open` surfaces as
// a CommanderError with a non-zero exit.
Expand Down
23 changes: 23 additions & 0 deletions packages/cli/src/commands/plan.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { afterEach, beforeEach, describe, expect, it } from 'bun:test';
import { CommanderError } from 'commander';
import { formatAgentResponse } from '#src/formatters/annotation/markdown.ts';
import { PLAN_TEMPLATES } from '#src/formatters/plan/templates.ts';
import { environment } from '#src/testFactories.ts';
import {
createAnnotationDependencies,
createStubContext,
Expand Down Expand Up @@ -148,6 +149,28 @@ describe('plan handler', () => {
expect(typeof submitted?.properties?.['duration_ms']).toBe('number');
});

it('passes an explicit port to the review server', async () => {
const { context, io } = createStubContext();
const deps = createAnnotationDependencies();
io.stdin.write('# My plan\n');
io.stdin.end();

await runPlan(context, { port: 3000 }, deps);

expect(deps.port).toBe(3000);
});

it('lets an explicit port override CONTEXTBRIDGE_PORT', async () => {
const { context, io } = createStubContext({ env: environment.build({ CONTEXTBRIDGE_PORT: 3456 }) });
const deps = createAnnotationDependencies();
io.stdin.write('# My plan\n');
io.stdin.end();

await runPlan(context, { port: 3000 }, deps);

expect(deps.port).toBe(3000);
});

it('does not report SIGINT to telemetry', async () => {
const { context, analytics, telemetry, io } = createStubContext();
const deps = createAnnotationDependencies({ result: createDeferred<AnnotationSubmission>().promise });
Expand Down
25 changes: 20 additions & 5 deletions packages/cli/src/commands/plan.ts
Original file line number Diff line number Diff line change
@@ -1,22 +1,24 @@
import { getErrorMessage } from '@contextbridge/shared/errors';
import { type Command, CommanderError } from 'commander';
import { type Command, CommanderError, InvalidArgumentError } from 'commander';
import {
type AnnotationDependencies,
AnnotationInterruptedError,
runAnnotation,
} from '#src/annotation/runAnnotation.ts';
import type { CliContext } from '#src/context.ts';
import { parsePort } from '#src/environment.ts';
import { formatAgentResponse } from '#src/formatters/annotation/markdown.ts';
import { PLAN_TEMPLATES } from '#src/formatters/plan/templates.ts';
import { abort } from './abort.ts';

export interface PlanArgs {
path?: string;
port?: number;
}

export async function runPlan(ctx: CliContext, args: PlanArgs, deps?: AnnotationDependencies): Promise<void> {
const { io, logger } = ctx;
const { path } = args;
const { path, port } = args;

if (!path && io.stdinIsTTY === true) {
abort(
Expand All @@ -42,7 +44,11 @@ export async function runPlan(ctx: CliContext, args: PlanArgs, deps?: Annotation
logger.info({ source, bytes: Buffer.byteLength(content, 'utf8') }, 'plan received');

try {
const submission = await runAnnotation(ctx, { content, contentKind: 'plan', entrypoint: 'plan_command' }, deps);
const submission = await runAnnotation(
ctx,
{ content, contentKind: 'plan', entrypoint: 'plan_command', port },
deps,
);
io.writeStdout(formatAgentResponse(PLAN_TEMPLATES, submission, content));
} catch (err) {
if (err instanceof AnnotationInterruptedError) {
Expand All @@ -60,7 +66,16 @@ export function registerPlan(ctx: CliContext, program: Command): void {
'Run a PlanBridge plan review: reads the plan from stdin or [path], opens a local browser UI for a human to approve or annotate, and writes the markdown result to stdout.',
)
.argument('[path]', 'path to a file containing the plan (alternative to stdin)')
.action(async (path: string | undefined) => {
await runPlan(ctx, { path });
.option('--port <number>', 'serve the plan review browser UI on a specific port', parsePortOption)
.action(async (path: string | undefined, opts: { port?: number }) => {
await runPlan(ctx, { path, port: opts.port });
});
}

function parsePortOption(value: string): number {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is duplicating logic that zod already has encoded. Can we lean on that instead?

try {
return parsePort(value);
} catch {
throw new InvalidArgumentError('port must be an integer between 1 and 65535');
}
}
14 changes: 14 additions & 0 deletions packages/cli/src/environment.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,17 @@ describe('boolean env coercion', () => {
});
});
});

describe('CONTEXTBRIDGE_PORT', () => {
it('coerces a configured port to a number', () => {
expect(getEnvironment({ CONTEXTBRIDGE_PORT: '3000' }).CONTEXTBRIDGE_PORT).toBe(3000);
});

it('is unset by default', () => {
expect(getEnvironment({}).CONTEXTBRIDGE_PORT).toBeUndefined();
});

it.each(['abc', '3000.5', '0', '-1', '65536'])('rejects invalid value %s', (value) => {
expect(() => getEnvironment({ CONTEXTBRIDGE_PORT: value })).toThrow();
});
});
7 changes: 7 additions & 0 deletions packages/cli/src/environment.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
import { z } from 'zod';

const booleanEnv = z.stringbool({ truthy: ['1', 'true', 'yes'], falsy: ['', '0', 'false', 'no'] }).default(false);
export const PortSchema = z.coerce.number().int().min(1).max(65535);
const portEnv = PortSchema.optional();

const Environment = z.object({
LOG_LEVEL: z.enum(['trace', 'debug', 'info', 'warn', 'error', 'fatal', 'silent']).default('info'),
DO_NOT_TRACK: booleanEnv,
CONTEXTBRIDGE_TELEMETRY_DISABLED: booleanEnv,
CI: booleanEnv,
CONTEXTBRIDGE_UPDATE_CHECK_DISABLED: booleanEnv,
CONTEXTBRIDGE_PORT: portEnv,
CONTEXTBRIDGE_DB_PATH: z.string().trim().nonempty().optional(),
XDG_CONFIG_HOME: z.string().optional(),
XDG_DATA_HOME: z.string().optional(),
Expand All @@ -19,3 +22,7 @@ export type Environment = z.infer<typeof Environment>;
export function getEnvironment(env: NodeJS.ProcessEnv = process.env): Environment {
return Environment.parse(env);
}

export function parsePort(value: unknown): number {
return PortSchema.parse(value);
}
1 change: 1 addition & 0 deletions packages/cli/src/testFactories.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ export const environment = Factory.define<Environment>(() => ({
CONTEXTBRIDGE_TELEMETRY_DISABLED: false,
CI: false,
CONTEXTBRIDGE_UPDATE_CHECK_DISABLED: false,
CONTEXTBRIDGE_PORT: undefined,
}));

export const harnessDescriptor = Factory.define<HarnessDescriptor>(() => ({
Expand Down
9 changes: 8 additions & 1 deletion packages/cli/src/testHelpers/annotationFakes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ export interface TrackedAnnotationDependencies extends AnnotationDependencies {
payloads: AnnotationPayload[];
/** Submission the fake server resolves with (unless `result` overrides it). */
submission: AnnotationSubmission;
/** Port passed to startReviewServer, if any. */
port: number | undefined;
/** Number of times the server's close() has been invoked. */
closeCount: number;
/** Convenience: true iff closeCount > 0. */
Expand Down Expand Up @@ -35,13 +37,17 @@ export function createAnnotationDependencies(
const submission = options.submission ?? annotationSubmission.build();
const sigintRegistration = createDeferred<void>();
let closeCount = 0;
let observedPort: number | undefined;
let sigintHandler: (() => void) | null = null;
let sigintHandlerRemoved = false;

return {
payloads,
submission,
sigintHandlerRegistered: sigintRegistration.promise,
get port() {
return observedPort;
},
get closeCount() {
return closeCount;
},
Expand All @@ -59,8 +65,9 @@ export function createAnnotationDependencies(
sigintHandler();
},
loadHtml: () => Promise.resolve('<html><body>annotation</body></html>'),
startReviewServer: (_ctx, { payload }) => {
startReviewServer: (_ctx, { payload, port }) => {
payloads.push(payload);
observedPort = port;
return {
port: 4312,
url: 'http://localhost:4312',
Expand Down
10 changes: 9 additions & 1 deletion packages/server/src/annotation.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import type { FrontendConfig } from '@contextbridge/shared/frontendConfigSchema'
import { annotationThread, globalThread } from '@contextbridge/shared/testFactories';
import type { UpdateNotice } from '@contextbridge/shared/updateNoticeSchema';
import { describe, expect, it } from 'bun:test';
import { createAnnotationServerApp, startServer } from './annotation.ts';
import { createAnnotationServerApp, resolveListenPort, startServer } from './annotation.ts';

describe('createAnnotationServerApp', () => {
const payload: AnnotationPayload = {
Expand Down Expand Up @@ -197,6 +197,14 @@ describe('startServer', () => {
const config: FrontendConfig = { distinctId: 'test-distinct-id', telemetryDisabled: false };
const ctx = fakeBaseContext();

it('uses port 0 by default so the OS chooses an available port', () => {
expect(resolveListenPort({})).toBe(0);
});

it('uses a configured port when supplied', () => {
expect(resolveListenPort({ port: 3456 })).toBe(3456);
});

// Regression test for the submit→close race: the /submit response must be
// fully delivered to the client even though the CLI tears the server down
// immediately after `running.result` resolves. Pre-fix, this failed with a
Expand Down
7 changes: 6 additions & 1 deletion packages/server/src/annotation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ export function startServer(ctx: ServerContext, opts: StartServerOptions): Runni
const { logger } = ctx;
const app = createAnnotationServerApp(ctx, opts);
const server = Bun.serve({
port: opts.port ?? 0,
port: resolveListenPort(opts),
fetch: app.fetch,
});

Expand Down Expand Up @@ -109,6 +109,11 @@ export function createAnnotationServerApp(ctx: ServerContext, opts: StartServerO
};
}

export function resolveListenPort(opts: Pick<StartServerOptions, 'port'>): number {
const { port = 0 } = opts;
return port;
}

async function resolveUpdateNotice(checkForUpdate: CheckForUpdate | undefined): Promise<UpdateNotice | null> {
if (!checkForUpdate) return null;
const timeout = new Promise<null>((resolve) => {
Expand Down
2 changes: 1 addition & 1 deletion tools
Submodule tools updated from 3cc696 to cc8956