diff --git a/scripts/cli-args.mjs b/scripts/cli-args.mjs index 87b1e97..4c9420d 100644 --- a/scripts/cli-args.mjs +++ b/scripts/cli-args.mjs @@ -18,6 +18,7 @@ const valueOptions = new Set([ '--batch-size', '--jobs', '--port', + '--host', ]); const flagOptions = new Set([ '--help', @@ -26,6 +27,7 @@ const flagOptions = new Set([ '--no-agent', '--force', '--worktree', + '--no-browser', ]); const pathOptions = new Set([ '--summaries', @@ -61,6 +63,8 @@ Options: --force Regenerate all agent notes --remote NAME|URL Git remote (default: origin) --port NUMBER Local page port (default: 2299) + --host ADDRESS Page bind address (default: localhost) + --no-browser Do not open the page in a browser -h, --help Show this help -v, --version Show the installed version @@ -322,6 +326,8 @@ export function parseCliArgs( agentArgs, port: Number(portValue), portWasPassed: options.has('--port'), + host: options.get('--host') || 'localhost', + browserEnabled: !options.has('--no-browser'), forceSummaryRegeneration: options.has('--force'), }; } diff --git a/scripts/present.mjs b/scripts/present.mjs index a532e56..e7562c0 100755 --- a/scripts/present.mjs +++ b/scripts/present.mjs @@ -48,7 +48,7 @@ if (cli.doctor) { process.exit(report.ready ? 0 : 1); } -const { agentEnabled, port } = cli; +const { agentEnabled, browserEnabled, host, port } = cli; const feedArgs = [...cli.feedArgs]; const agentArgs = [...cli.agentArgs]; if (agentEnabled) { @@ -168,6 +168,8 @@ function startSite() { outputPath, '--port', String(port), + '--host', + host, '--project', projectKey, ...(!cli.portWasPassed ? ['--increment-port'] : []), @@ -190,7 +192,7 @@ function startSite() { } console.log(line); const match = line.match(/^Diffsplain: (http:\/\/\S+)$/); - if (!browserOpened && !browserOpenTimer && match) { + if (browserEnabled && !browserOpened && !browserOpenTimer && match) { browserOpenTimer = setTimeout(() => { browserOpenTimer = undefined; browserOpened = true; diff --git a/scripts/serve-built.mjs b/scripts/serve-built.mjs index a19ae3d..beaae49 100644 --- a/scripts/serve-built.mjs +++ b/scripts/serve-built.mjs @@ -24,6 +24,7 @@ function option(name, fallback) { const output = resolve(option('--output', resolve(root, '.cache/diff-data.json'))); const project = option('--project', ''); const portValue = option('--port', '2299'); +const host = option('--host', 'localhost'); if (!/^\d+$/.test(portValue) || Number(portValue) > 65_535) { throw new Error('--port must be a number from 0 to 65535'); } @@ -86,6 +87,15 @@ async function fetchAsset(request) { return fileResponse(file); } +function jsonResponse(value) { + return new Response(JSON.stringify(value), { + headers: { + 'cache-control': 'no-store', + 'content-type': 'application/json; charset=utf-8', + }, + }); +} + function nodeRequest(request) { const host = request.headers.host || 'localhost'; const init = { @@ -111,7 +121,12 @@ async function send(nodeResponse, response) { const server = createServer(async (request, response) => { try { const webRequest = nodeRequest(request); - if (new URL(webRequest.url).pathname === '/events') { + const url = new URL(webRequest.url); + if (url.pathname === '/health') { + await send(response, jsonResponse(readyState)); + return; + } + if (url.pathname === '/events') { response.writeHead(200, { 'content-type': 'text/event-stream', 'cache-control': 'no-store', @@ -119,7 +134,7 @@ const server = createServer(async (request, response) => { }); response.write('retry: 250\nevent: ready\ndata: {}\n\n'); eventClients.add(response); - const requestProject = new URL(webRequest.url).searchParams.get('project'); + const requestProject = url.searchParams.get('project'); if (project && requestProject === project) { console.log('Diffsplain tab: connected'); } @@ -148,19 +163,53 @@ watchFile(output, { interval: 100 }, (current, previous) => { }); let selectedPort = Number(portValue); +let readyState; +let closing = false; + +function urlFor(address, port) { + const formattedAddress = address.includes(':') ? `[${address}]` : address; + return `http://${formattedAddress}:${port}`; +} + +function isLoopback(address) { + return ( + address === 'localhost' || + address === '::1' || + address === '::ffff:127.0.0.1' || + /^127(?:\.\d{1,3}){3}$/.test(address) + ); +} function listen() { - server.listen(selectedPort, 'localhost'); + server.listen(selectedPort, host); } server.on('listening', () => { + if (closing) { + server.close(); + return; + } const address = server.address(); + const readyAddress = + address && typeof address === 'object' ? address.address : host; const readyPort = address && typeof address === 'object' ? address.port : selectedPort; + const url = urlFor(host, readyPort); const projectHash = project ? `#project=${encodeURIComponent(project)}` : ''; - console.log(`Diffsplain: http://localhost:${readyPort}${projectHash}`); + readyState = { + status: 'ok', + address: readyAddress, + port: readyPort, + }; + if (!isLoopback(readyAddress)) { + console.warn( + `Warning: Diffsplain is listening on ${readyAddress}. Anyone who can reach this address can view this review.`, + ); + } + console.log(`Diffsplain: ${url}${projectHash}`); + console.log(JSON.stringify({ event: 'ready', ...readyState, url })); }); server.on('error', (error) => { @@ -175,22 +224,20 @@ server.on('error', (error) => { return; } console.error(`Could not start Diffsplain: ${error.message}`); - process.exitCode = 1; + close(1); }); listen(); -let closing = false; -function close() { +function close(exitCode = 0) { if (closing) return; closing = true; + process.exitCode = exitCode; unwatchFile(output); for (const client of eventClients) client.end(); eventClients.clear(); - server.close(() => { - process.exitCode = 0; - }); + if (server.listening) server.close(); } -process.on('SIGINT', close); -process.on('SIGTERM', close); +process.on('SIGINT', () => close()); +process.on('SIGTERM', () => close()); diff --git a/tests/cli-args.test.mjs b/tests/cli-args.test.mjs index 68d3d65..d405e65 100644 --- a/tests/cli-args.test.mjs +++ b/tests/cli-args.test.mjs @@ -17,10 +17,22 @@ test('leaves agent selection open when no agent is passed', () => { assert.equal(parsed.agent, undefined); assert.equal(parsed.port, 2299); assert.equal(parsed.portWasPassed, false); + assert.equal(parsed.host, 'localhost'); + assert.equal(parsed.browserEnabled, true); assert.deepEqual(parsed.feedArgs, ['--repo', cwd, '--checkout']); assert.deepEqual(parsed.agentArgs, ['--repo', cwd, '--checkout']); }); +test('accepts headless browser and explicit bind options', () => { + const parsed = parseCliArgs(['--no-browser', '--host', '0.0.0.0'], { + callerDirectory: cwd, + pathExists: missing, + }); + + assert.equal(parsed.browserEnabled, false); + assert.equal(parsed.host, '0.0.0.0'); +}); + test('accepts a GitHub owner/name repo and a branch', () => { const parsed = parseCliArgs(['acme/widgets', '--branch', 'feature/search'], { callerDirectory: cwd, diff --git a/tests/present-instances.test.mjs b/tests/present-instances.test.mjs index 3a64306..027a085 100644 --- a/tests/present-instances.test.mjs +++ b/tests/present-instances.test.mjs @@ -60,6 +60,22 @@ function waitForUrl(child) { }); } +function waitForText(stream, pattern) { + return new Promise((resolve, reject) => { + let output = ''; + const timer = setTimeout(() => { + reject(new Error(`Did not find ${pattern}: ${output}`)); + }, 12_000); + stream.on('data', (chunk) => { + output += chunk; + if (pattern.test(output)) { + clearTimeout(timer); + resolve(output); + } + }); + }); +} + async function waitFor(read, timeout = 8_000) { const deadline = Date.now() + timeout; let lastError; @@ -308,3 +324,73 @@ test('reuses a matching project tab when it reconnects', async () => { await rm(root, { recursive: true, force: true }); } }); + +test('stays available when browser launch fails and skips it when asked', async () => { + const root = await mkdtemp(join(tmpdir(), 'diffsplain-headless-')); + const browserLog = join(root, 'browser.log'); + const browser = join(root, 'browser'); + let failingPresenter; + let headlessPresenter; + + try { + const repo = await makeRepo(root, 'repo', 'file.txt'); + failingPresenter = spawn( + process.execPath, + [ + script, + '--repo', + repo, + '--worktree', + '--no-agent', + '--port', + '0', + ], + { + cwd: root, + env: { ...process.env, BROWSER: join(root, 'missing-browser') }, + stdio: ['ignore', 'pipe', 'pipe'], + }, + ); + const browserFailure = waitForText( + failingPresenter.stderr, + /Could not open the browser:/, + ); + const failingUrl = await waitForUrl(failingPresenter); + await browserFailure; + assert.equal((await fetch(new URL('health', failingUrl))).status, 200); + assert.equal(failingPresenter.exitCode, null); + + await writeFile(browser, '#!/bin/sh\nprintf opened > "$BROWSER_LOG"\n'); + await chmod(browser, 0o755); + headlessPresenter = spawn( + process.execPath, + [ + script, + '--repo', + repo, + '--worktree', + '--no-agent', + '--no-browser', + '--port', + '0', + ], + { + cwd: root, + env: { ...process.env, BROWSER: browser, BROWSER_LOG: browserLog }, + stdio: ['ignore', 'pipe', 'pipe'], + }, + ); + const headlessUrl = await waitForUrl(headlessPresenter); + assert.equal((await fetch(new URL('health', headlessUrl))).status, 200); + await new Promise((resolve) => setTimeout(resolve, 900)); + await assert.rejects(readFile(browserLog, 'utf8')); + } finally { + if (failingPresenter && failingPresenter.exitCode === null) { + await stop(failingPresenter); + } + if (headlessPresenter && headlessPresenter.exitCode === null) { + await stop(headlessPresenter); + } + await rm(root, { recursive: true, force: true }); + } +}); diff --git a/tests/serve-built.test.mjs b/tests/serve-built.test.mjs index 7420e6d..689e62e 100644 --- a/tests/serve-built.test.mjs +++ b/tests/serve-built.test.mjs @@ -30,6 +30,51 @@ function waitForUrl(child) { }); } +function waitForReady(child) { + return new Promise((resolve, reject) => { + let output = ''; + const timer = setTimeout(() => { + reject(new Error(`Built server did not report readiness: ${output}`)); + }, 10_000); + child.stdout.on('data', (chunk) => { + output += chunk; + for (const line of output.split('\n')) { + try { + const event = JSON.parse(line); + if (event.event === 'ready') { + clearTimeout(timer); + resolve(event); + return; + } + } catch { + // The server also writes human-readable status lines. + } + } + }); + child.once('error', reject); + child.once('exit', (code) => { + clearTimeout(timer); + reject(new Error(`Built server exited with ${code}: ${output}`)); + }); + }); +} + +function waitForText(stream, pattern) { + return new Promise((resolve, reject) => { + let output = ''; + const timer = setTimeout(() => { + reject(new Error(`Did not find ${pattern}: ${output}`)); + }, 10_000); + stream.on('data', (chunk) => { + output += chunk; + if (pattern.test(output)) { + clearTimeout(timer); + resolve(output); + } + }); + }); +} + function stop(child) { return new Promise((resolve, reject) => { child.once('exit', resolve); @@ -38,6 +83,20 @@ function stop(child) { }); } +function waitForExit(child) { + return new Promise((resolve, reject) => { + const timer = setTimeout(() => { + child.kill('SIGKILL'); + reject(new Error('Built server did not exit after its bind error')); + }, 10_000); + child.once('error', reject); + child.once('exit', (code, signal) => { + clearTimeout(timer); + resolve({ code, signal }); + }); + }); +} + test('serves the built review page with live diff data', async () => { const directory = await mkdtemp(join(tmpdir(), 'diffsplain-server-')); const output = join(directory, 'diff-data.json'); @@ -119,6 +178,103 @@ test('reports a matching project tab connection', async () => { } }); +test('reports machine-readable readiness and closes its health endpoint', async () => { + const directory = await mkdtemp(join(tmpdir(), 'diffsplain-health-')); + const output = join(directory, 'diff-data.json'); + let child; + + try { + await writeFile(output, '{}'); + child = spawn( + process.execPath, + [script, '--output', output, '--port', '0'], + { stdio: ['ignore', 'pipe', 'pipe'] }, + ); + const ready = await waitForReady(child); + assert.ok(['127.0.0.1', '::1'].includes(ready.address)); + assert.ok(ready.port > 0); + assert.equal(ready.url, `http://localhost:${ready.port}`); + + const health = await fetch(`${ready.url}/health`); + assert.equal(health.status, 200); + assert.deepEqual(await health.json(), { + status: 'ok', + address: ready.address, + port: ready.port, + }); + + assert.equal(await stop(child), 0); + await assert.rejects(fetch(`${ready.url}/health`)); + } finally { + if (child && child.exitCode === null) await stop(child); + await rm(directory, { recursive: true, force: true }); + } +}); + +test('warns before binding the review to a remote address', async () => { + const directory = await mkdtemp(join(tmpdir(), 'diffsplain-remote-')); + const output = join(directory, 'diff-data.json'); + let child; + + try { + await writeFile(output, '{}'); + child = spawn( + process.execPath, + [script, '--output', output, '--port', '0', '--host', '0.0.0.0'], + { stdio: ['ignore', 'pipe', 'pipe'] }, + ); + const warning = waitForText(child.stderr, /anyone who can reach/i); + const ready = await waitForReady(child); + await warning; + assert.equal(ready.address, '0.0.0.0'); + assert.ok(ready.port > 0); + + const health = await fetch(`http://127.0.0.1:${ready.port}/health`); + assert.equal(health.status, 200); + } finally { + if (child && child.exitCode === null) await stop(child); + await rm(directory, { recursive: true, force: true }); + } +}); + +test('exits with an error when the requested host cannot bind', async () => { + const directory = await mkdtemp(join(tmpdir(), 'diffsplain-bind-error-')); + const output = join(directory, 'diff-data.json'); + let child; + let stdout = ''; + let stderr = ''; + + try { + await writeFile(output, '{}'); + child = spawn( + process.execPath, + [ + script, + '--output', + output, + '--port', + '0', + '--host', + '192.0.2.1', + ], + { stdio: ['ignore', 'pipe', 'pipe'] }, + ); + child.stdout.on('data', (chunk) => { + stdout += chunk; + }); + child.stderr.on('data', (chunk) => { + stderr += chunk; + }); + + assert.deepEqual(await waitForExit(child), { code: 1, signal: null }); + assert.match(stderr, /Could not start Diffsplain/); + assert.doesNotMatch(stdout, /"event":"ready"/); + } finally { + if (child && child.exitCode === null) child.kill('SIGKILL'); + await rm(directory, { recursive: true, force: true }); + } +}); + test('pushes an event soon after live diff data changes', async () => { const directory = await mkdtemp(join(tmpdir(), 'diffsplain-events-')); const output = join(directory, 'diff-data.json');