From f77ccdd52082ff5626b649a2a7a4510292f8df61 Mon Sep 17 00:00:00 2001 From: Nicholas Zolton Date: Mon, 12 Oct 2026 16:24:03 -0500 Subject: [PATCH] fix(crit): forward remote reviews over Tailscale --- apps/server/src/process/CritManager.test.ts | 258 +++++++++++++++++- apps/server/src/process/CritManager.ts | 190 ++++++++++++- .../web/src/hooks/useOpenInCrit.logic.test.ts | 116 ++++++++ apps/web/src/hooks/useOpenInCrit.logic.ts | 41 +++ apps/web/src/hooks/useOpenInCrit.ts | 36 +-- packages/contracts/src/crit.ts | 1 + wiki/log.md | 2 + wiki/upstream-integration.md | 27 +- 8 files changed, 648 insertions(+), 23 deletions(-) create mode 100644 apps/web/src/hooks/useOpenInCrit.logic.test.ts create mode 100644 apps/web/src/hooks/useOpenInCrit.logic.ts diff --git a/apps/server/src/process/CritManager.test.ts b/apps/server/src/process/CritManager.test.ts index 6e4c7a82388a..f94f2a69dfed 100644 --- a/apps/server/src/process/CritManager.test.ts +++ b/apps/server/src/process/CritManager.test.ts @@ -6,9 +6,11 @@ import * as NodeFSP from "node:fs/promises"; import * as NodeHttp from "node:http"; import * as NodeOS from "node:os"; import * as NodePath from "node:path"; -import { CheckpointRef, ThreadId } from "@t3tools/contracts"; +import { CheckpointRef, ThreadId, type CritOpenInput } from "@t3tools/contracts"; import * as Effect from "effect/Effect"; import * as Schema from "effect/Schema"; +import * as HttpClient from "effect/http/HttpClient"; +import * as FetchHttpClient from "effect/http/FetchHttpClient"; import { afterEach, it } from "@effect/vitest"; import { createCritManager } from "./CritManager.ts"; @@ -16,6 +18,9 @@ const decodeArguments = Schema.decodeUnknownSync( Schema.fromJsonString(Schema.Array(Schema.String)), ); const encodeJsonString = Schema.encodeSync(Schema.fromJsonString(Schema.String)); +const decodeForwardRoute = Schema.decodeUnknownSync( + Schema.fromJsonString(Schema.Struct({ port: Schema.Number })), +); const decodeAlias = Schema.decodeSync( Schema.fromJsonString(Schema.Struct({ args: Schema.Array(Schema.String), port: Schema.String })), ); @@ -28,12 +33,159 @@ const decodeEnvironment = Schema.decodeUnknownSync( ), ); const fixtures: Array<() => Promise> = []; + +async function tailnetFixture( + options: { + readonly failForward?: boolean; + readonly requireSudo?: boolean; + } = {}, +): Promise<{ + readonly directory: string; + readonly manager: ReturnType; + readonly forwardClosed: Promise; + readonly browserConnected: Promise; + readonly existingConfig: string; +}> { + const directory = await NodeFSP.mkdtemp(NodePath.join(NodeOS.tmpdir(), "t3-crit-tailnet-test-")); + fixtures.push(() => NodeFSP.rm(directory, { recursive: true, force: true })); + let notifyClosed: () => void = () => undefined; + const forwardClosed = new Promise((resolve) => { + notifyClosed = resolve; + }); + let notifyConnected: () => void = () => undefined; + const browserConnected = new Promise((resolve) => { + notifyConnected = resolve; + }); + const control = NodeHttp.createServer((request, response) => { + if (request.url === "/connected") notifyConnected(); + else notifyClosed(); + response.end("ok"); + }); + await new Promise((resolve) => control.listen(0, "127.0.0.1", resolve)); + fixtures.push(() => new Promise((resolve) => control.close(() => resolve()))); + const address = control.address(); + NodeAssert.ok(address && typeof address !== "string"); + const closedUrl = `http://127.0.0.1:${address.port}/closed`; + const command = NodePath.join(directory, "crit"); + const tailscaleCommand = NodePath.join(directory, "tailscale"); + const sudoCommand = NodePath.join(directory, "sudo"); + const configPath = NodePath.join(directory, "serve.json"); + const existingConfig = JSON.stringify({ + TCP: { "443": { HTTPS: true } }, + Foreground: { existing: { TCP: { "8443": { HTTPS: true } } } }, + }); + await NodeFSP.writeFile(configPath, existingConfig); + await Promise.all([ + NodeFSP.writeFile( + command, + `#!${process.execPath} +const fs = require('node:fs'); +const http = require('node:http'); +const path = require('node:path'); +const args = process.argv.slice(2); +const port = Number(args[args.indexOf('--port') + 1]); +if (args.includes('stop')) process.exit(0); +if (args.includes('status')) { + process.stdout.write(fs.readFileSync(path.join(process.env.HOME, 'status.json'))); + process.exit(0); +} +fs.writeFileSync(path.join(process.env.HOME, 'status.json'), JSON.stringify({ sessions: [{ port }] })); +fs.writeFileSync(${encodeJsonString(NodePath.join(directory, "review-home"))}, process.env.HOME); +let connected = false; +let reportedConnection = false; +const server = http.createServer((req, res) => { + if (req.url === '/api/health') { + res.setHeader('content-type', 'application/json'); + res.end(JSON.stringify({ status: 'ok', browser_clients: connected })); + if (connected && !reportedConnection) { + reportedConnection = true; + fetch(${encodeJsonString(`http://127.0.0.1:${address.port}/connected`)}, { method: 'POST' }); + } + return; + } + connected = req.url !== '/close'; + res.end('Crit review'); +}); +server.listen(port, '127.0.0.1', () => { + process.stderr.write('Started crit daemon at ' + args[args.indexOf('--public-url') + 1] + ' (session test)\\n'); +}); +process.on('SIGTERM', () => server.close(() => process.exit(0))); +`, + { mode: 0o755 }, + ), + NodeFSP.writeFile( + tailscaleCommand, + `#!${process.execPath} +const fs = require('node:fs'); +const http = require('node:http'); +const args = process.argv.slice(2); +if (args[0] === 'status') { + process.stdout.write(JSON.stringify({ BackendState: 'Running', Self: { DNSName: 'dev.tail123.ts.net.' } })); + process.exit(0); +} +if (args[1] === 'status') { + process.stdout.write(fs.readFileSync(${encodeJsonString(configPath)})); + process.exit(0); +} +if (${String(options.requireSudo ?? false)} && !process.env.T3_CRIT_TEST_ELEVATED) { + process.stderr.write('sending serve config: 401 Unauthorized: must be root\\n'); + process.exit(1); +} +if (${String(options.failForward ?? false)}) process.exit(1); +if (args.includes('--bg') || args.includes('off') || args.includes('reset') || args[0] !== 'serve') process.exit(2); +const port = Number(args.find(arg => arg.startsWith('--https=')).split('=')[1]); +const target = args.at(-1); +const route = ${encodeJsonString(directory)} + '/route-' + port; +const server = http.createServer((req, res) => { + const upstream = http.request(target + req.url, { method: req.method }, response => { + res.writeHead(response.statusCode, response.headers); + response.pipe(res); + }); + upstream.on('error', () => { res.statusCode = 502; res.end(); }); + req.pipe(upstream); +}); +server.listen(0, '127.0.0.1', () => { + fs.writeFileSync(route, JSON.stringify({ port: server.address().port })); + process.stdout.write('Available within your tailnet:\\n\\nhttps://dev.tail123.ts.net:' + port + '/\\n'); +}); +process.on('SIGTERM', () => server.close(() => { + fs.unlinkSync(route); + fetch(${encodeJsonString(closedUrl)}, { method: 'POST' }).then(() => process.exit(0)); +})); +`, + { mode: 0o755 }, + ), + NodeFSP.writeFile( + sudoCommand, + `#!${process.execPath} +const { spawn } = require('node:child_process'); +const args = process.argv.slice(2); +if (args[0] !== '-n' || args[1] !== '--') process.exit(2); +const child = spawn(args[2], args.slice(3), { stdio: 'inherit', env: { ...process.env, T3_CRIT_TEST_ELEVATED: '1' } }); +process.on('SIGTERM', () => child.kill('SIGTERM')); +child.on('exit', code => process.exit(code ?? 0)); +`, + { mode: 0o755 }, + ), + ]); + const manager = createCritManager({ + command, + tailscaleCommand, + sudoCommand, + outputDir: directory, + browserCheckIntervalMs: 10, + browserDisconnectGraceMs: 0, + }); + fixtures.push(() => manager.dispose()); + return { directory, manager, forwardClosed, browserConnected, existingConfig }; +} afterEach(async () => { for (const cleanup of fixtures.splice(0).toReversed()) await cleanup(); }); -for (const stopReason of ["browser disconnect", "one-hour lifetime"] as const) - it.effect(`opens the selected scope and stops only its CLI after ${stopReason}`, () => +it.effect.each(["browser disconnect", "one-hour lifetime"] as const)( + "opens the selected scope and stops only its CLI after %s", + (stopReason) => Effect.gen(function* () { const directory = yield* Effect.promise(() => NodeFSP.mkdtemp(NodePath.join(NodeOS.tmpdir(), "t3-crit-test-")), @@ -118,7 +270,7 @@ for (const stopReason of ["browser disconnect", "one-hour lifetime"] as const) NodeAssert.equal(NodeFS.existsSync(stopHome), false); NodeAssert.equal(NodeFS.existsSync(outputDir), true); }), - ); +); it.effect("maps branch, turn, pull request, and commit selections to Crit scopes", () => Effect.gen(function* () { @@ -350,3 +502,101 @@ it.effect( ]); }), ); + +it.effect.each([false, true])( + "forwards concurrent Crit reviews without changing existing endpoints (sudo required: %s)", + (requireSudo) => + Effect.gen(function* () { + const fixture = yield* Effect.promise(() => tailnetFixture({ requireSudo })); + const input: CritOpenInput = { + cwd: process.cwd(), + tailscale: true, + scope: { kind: "working-tree" }, + }; + const urls = yield* Effect.all([fixture.manager.open(input), fixture.manager.open(input)], { + concurrency: "unbounded", + }); + NodeAssert.ok(urls[0] && urls[1]); + NodeAssert.notEqual(urls[0], urls[1]); + const ports = urls.map((url) => { + NodeAssert.ok(url); + const parsed = new URL(url); + NodeAssert.equal(parsed.protocol, "https:"); + NodeAssert.equal(parsed.hostname, "dev.tail123.ts.net"); + NodeAssert.ok(!["443", "8443"].includes(parsed.port)); + return parsed.port; + }); + const origins = yield* Effect.forEach(ports, (port) => + Effect.promise(async () => { + const route = decodeForwardRoute( + await NodeFSP.readFile(NodePath.join(fixture.directory, `route-${port}`), "utf8"), + ); + return `http://127.0.0.1:${route.port}`; + }), + ); + const first = origins[0]; + const second = origins[1]; + NodeAssert.ok(first && second); + const response = yield* HttpClient.get(first); + NodeAssert.equal(yield* response.text, "Crit review"); + yield* Effect.promise(() => fixture.browserConnected); + const closing = yield* HttpClient.get(`${first}/close`); + yield* closing.text; + yield* Effect.promise(() => fixture.forwardClosed); + NodeAssert.equal( + NodeFS.existsSync(NodePath.join(fixture.directory, `route-${ports[0]}`)), + false, + ); + const remaining = yield* HttpClient.get(second); + NodeAssert.equal(yield* remaining.text, "Crit review"); + yield* Effect.promise(() => fixture.manager.dispose()); + NodeAssert.equal( + NodeFS.existsSync(NodePath.join(fixture.directory, `route-${ports[1]}`)), + false, + ); + NodeAssert.equal( + yield* Effect.promise(() => + NodeFSP.readFile(NodePath.join(fixture.directory, "serve.json"), "utf8"), + ), + fixture.existingConfig, + ); + }).pipe(Effect.provide(FetchHttpClient.layer), Effect.scoped), +); + +it.effect.each([ + { requireSudo: false, message: /Tailscale Serve exited before forwarding Crit/ }, + { requireSudo: true, message: /Allow non-interactive sudo for tailscale/ }, +])( + "cleans up a Crit launch when Tailscale cannot publish the review ($requireSudo)", + ({ requireSudo, message }) => + Effect.gen(function* () { + const fixture = yield* Effect.promise(() => + tailnetFixture({ failForward: true, requireSudo }), + ); + const error = yield* fixture.manager + .open({ + cwd: process.cwd(), + tailscale: true, + scope: { kind: "working-tree" }, + }) + .pipe(Effect.flip); + NodeAssert.match(error.message, message); + const reviewHome = yield* Effect.promise(() => + NodeFSP.readFile(NodePath.join(fixture.directory, "review-home"), "utf8"), + ); + NodeAssert.equal(NodeFS.existsSync(reviewHome), false); + yield* Effect.promise(() => fixture.manager.dispose()); + NodeAssert.equal( + yield* Effect.promise(() => + NodeFSP.readFile(NodePath.join(fixture.directory, "serve.json"), "utf8"), + ), + fixture.existingConfig, + ); + NodeAssert.equal( + (yield* Effect.promise(() => NodeFSP.readdir(fixture.directory))).some((name) => + name.startsWith("route-"), + ), + false, + ); + }), +); diff --git a/apps/server/src/process/CritManager.ts b/apps/server/src/process/CritManager.ts index e545049dbc55..2cde39296e81 100644 --- a/apps/server/src/process/CritManager.ts +++ b/apps/server/src/process/CritManager.ts @@ -2,6 +2,7 @@ import * as NodeChildProcess from "node:child_process"; import * as NodeCrypto from "node:crypto"; import * as NodeFSP from "node:fs/promises"; +import * as NodeNet from "node:net"; import * as NodeOS from "node:os"; import * as NodePath from "node:path"; import * as NodeUtil from "node:util"; @@ -21,11 +22,129 @@ const decodeCritStatus = Schema.decodeUnknownSync( Schema.Struct({ sessions: Schema.Array(Schema.Struct({ port: Schema.Number })) }), ), ); +const decodeTailscaleStatus = Schema.decodeUnknownSync( + Schema.fromJsonString( + Schema.Struct({ + BackendState: Schema.String, + Self: Schema.optional( + Schema.NullOr(Schema.Struct({ DNSName: Schema.optional(Schema.String) })), + ), + }), + ), +); +const TailscaleListeners = Schema.Struct({ + TCP: Schema.optional(Schema.Record(Schema.String, Schema.Struct({}))), +}); +const decodeTailscaleServe = Schema.decodeUnknownSync( + Schema.fromJsonString( + Schema.Struct({ + ...TailscaleListeners.fields, + Foreground: Schema.optional(Schema.Record(Schema.String, TailscaleListeners)), + }), + ), +); +const FORWARD_START_TIMEOUT_MS = 10_000; const BROWSER_START_TIMEOUT_MS = 60_000; const BROWSER_CHECK_INTERVAL_MS = 2_000; const MAX_REVIEW_LIFETIME_MS = 60 * 60 * 1_000; const shellQuote = (value: string): string => `'${value.replaceAll("'", "'\\''")}'`; +class TailscaleServePermissionError extends Error { + constructor() { + super("Tailscale needs permission to publish the Crit review."); + } +} + +async function tailscaleTarget(command: string): Promise<{ + readonly hostname: string; + readonly occupiedPorts: ReadonlySet; +}> { + const status = decodeTailscaleStatus( + (await execFile(command, ["status", "--json"], { timeout: FORWARD_START_TIMEOUT_MS })).stdout, + ); + const hostname = status.Self?.DNSName?.replace(/\.$/, "") ?? ""; + if (status.BackendState !== "Running" || !hostname.endsWith(".ts.net")) { + throw new Error("Tailscale must be connected to open this Crit review remotely."); + } + const config = decodeTailscaleServe( + (await execFile(command, ["serve", "status", "--json"], { timeout: FORWARD_START_TIMEOUT_MS })) + .stdout, + ); + return { + hostname, + occupiedPorts: new Set( + [config, ...Object.values(config.Foreground ?? {})].flatMap((entry) => + Object.keys(entry.TCP ?? {}).map(Number), + ), + ), + }; +} + +async function unusedTailnetPort( + occupiedPorts: ReadonlySet, + allocatedPorts: ReadonlySet, +): Promise { + for (let attempt = 0; attempt < 10; attempt += 1) { + const server = NodeNet.createServer(); + const port = await new Promise((resolve, reject) => { + server.once("error", reject); + server.listen(0, "127.0.0.1", () => { + const address = server.address(); + if (address === null || typeof address === "string") { + reject(new Error("Could not reserve a port for the Crit review.")); + return; + } + resolve(address.port); + }); + }); + await new Promise((resolve, reject) => + server.close((error) => (error ? reject(error) : resolve())), + ); + if (!occupiedPorts.has(port) && !allocatedPorts.has(port)) return port; + } + throw new Error("Could not find an unused Tailscale port for the Crit review."); +} + +function waitForTailscaleForward(child: NodeChildProcess.ChildProcess, url: string): Promise { + return new Promise((resolve, reject) => { + let output = ""; + let stderr = ""; + const timeout = setTimeout( + () => finish(new Error("Tailscale did not forward the Crit review in time.")), + FORWARD_START_TIMEOUT_MS, + ); + const finish = (error?: Error): void => { + clearTimeout(timeout); + child.stdout?.off("data", onData); + child.stderr?.off("data", onStderr); + child.off("error", onError); + child.off("close", onClose); + if (error) reject(error); + else resolve(); + }; + const onData = (chunk: Buffer): void => { + output = (output + chunk.toString()).slice(-4_096); + if (output.includes("Available within your tailnet:") && output.includes(url)) finish(); + }; + const onStderr = (chunk: Buffer): void => { + stderr = (stderr + chunk.toString()).slice(-4_096); + }; + const onError = (): void => finish(new Error("Could not start Tailscale Serve for Crit.")); + const onClose = (): void => + finish( + /permission denied|access denied|must be root|operation not permitted/i.test(stderr) + ? new TailscaleServePermissionError() + : new Error( + "Tailscale Serve exited before forwarding Crit. Check Tailscale permissions.", + ), + ); + child.stdout?.on("data", onData); + child.stderr?.on("data", onStderr); + child.once("error", onError); + child.once("close", onClose); + }); +} + async function portlessAlias(command: string, args: string[], port: number): Promise { const env = { ...process.env, PORTLESS_PORT: String(port), PORTLESS_SYNC_HOSTS: "0" }; try { @@ -100,6 +219,8 @@ export function createCritManager( options: { readonly command?: string; readonly portlessCommand?: string; + readonly tailscaleCommand?: string; + readonly sudoCommand?: string; readonly outputDir?: string; readonly browserCheckIntervalMs?: number; readonly browserDisconnectGraceMs?: number; @@ -108,11 +229,18 @@ export function createCritManager( ) { const command = options.command ?? "crit"; const portlessCommand = options.portlessCommand ?? "portless"; + const tailscaleCommand = options.tailscaleCommand ?? "tailscale"; + const sudoCommand = options.sudoCommand ?? "sudo"; const outputDir = options.outputDir ?? NodePath.join(NodeOS.homedir(), ".crit"); const isWindows = HostProcess.Platform.defaultValue() === "win32"; const reviews = new Set(); + const allocatedTailnetPorts = new Set(); const open = async (input: CritOpenInput): Promise => { + if (input.tailscale && input.portlessPort) { + throw new Error("Choose either Tailscale or SSH forwarding for the Crit review."); + } + const tailnet = input.tailscale ? await tailscaleTarget(tailscaleCommand) : null; const args = await critArgs(input); let githubRepository: string | undefined; if (input.scope.kind === "pull-request" && input.scope.provider === "github") { @@ -129,7 +257,6 @@ export function createCritManager( } const root = await NodeFSP.mkdtemp(NodePath.join(NodeOS.tmpdir(), "t3-crit-")); const alias = input.portlessPort ? `t3-crit-${NodeCrypto.randomUUID()}` : null; - const reviewUrl = alias ? `https://${alias}.localhost:${input.portlessPort}` : null; const providerBin = NodePath.join(root, "bin"); if (input.scope.kind === "pull-request" && !isWindows) { try { @@ -147,6 +274,22 @@ export function createCritManager( throw error; } } + let tailnetPort: number | null = null; + try { + if (tailnet) { + tailnetPort = await unusedTailnetPort(tailnet.occupiedPorts, allocatedTailnetPorts); + allocatedTailnetPorts.add(tailnetPort); + } + } catch (error) { + await NodeFSP.rm(root, { recursive: true, force: true }); + throw error; + } + const reviewUrl = + tailnet && tailnetPort + ? `https://${tailnet.hostname}:${tailnetPort}` + : alias + ? `https://${alias}.localhost:${input.portlessPort}` + : null; const env = { ...process.env, HOME: root, @@ -165,6 +308,7 @@ export function createCritManager( "127.0.0.1", "--output", outputDir, + ...(tailnetPort === null ? [] : ["--port", String(tailnetPort)]), ...(reviewUrl ? ["--no-open", "--public-url", reviewUrl, "--allow-unauthenticated-network"] : []), @@ -188,6 +332,8 @@ export function createCritManager( child.once("error", resolveExit); let stopPromise: Promise | undefined; let aliased = false; + let tailnetChild: NodeChildProcess.ChildProcess | undefined; + let tailnetExited: Promise | undefined; const review: ManagedReview = { stop: () => { stopPromise ??= (async () => { @@ -195,6 +341,13 @@ export function createCritManager( if (timer) clearInterval(timer); if (startupTimeout) clearTimeout(startupTimeout); if (lifetimeTimeout) clearTimeout(lifetimeTimeout); + if (tailnetChild) { + if (tailnetChild.exitCode === null && tailnetChild.signalCode === null) { + tailnetChild.kill("SIGTERM"); + } + await tailnetExited; + } + if (tailnetPort !== null) allocatedTailnetPorts.delete(tailnetPort); if (aliased && alias && input.portlessPort) { await portlessAlias( portlessCommand, @@ -258,6 +411,41 @@ export function createCritManager( await portlessAlias(portlessCommand, ["alias", alias, String(port)], input.portlessPort); aliased = true; } + if (tailnetPort !== null && reviewUrl !== null) { + // Foreground Serve owns a separate session; exiting removes only this review's endpoint. + const forwardArgs: string[] = [ + "serve", + `--https=${tailnetPort}`, + `http://127.0.0.1:${port}`, + ]; + const startForward = async (elevated: boolean): Promise => { + const forward: NodeChildProcess.ChildProcess = NodeChildProcess.spawn( + elevated ? sudoCommand : tailscaleCommand, + elevated ? ["-n", "--", tailscaleCommand, ...forwardArgs] : forwardArgs, + { stdio: ["ignore", "pipe", "pipe"] }, + ); + tailnetChild = forward; + tailnetExited = new Promise((resolve) => { + forward.once("exit", resolve); + forward.once("error", resolve); + }); + await waitForTailscaleForward(forward, reviewUrl); + }; + try { + await startForward(false); + } catch (error) { + if (!(error instanceof TailscaleServePermissionError) || isWindows) throw error; + // Existing file shares make even an operator's proxy-only config require sudo. + try { + await startForward(true); + } catch { + throw new Error( + "Tailscale needs permission to forward Crit. Allow non-interactive sudo for tailscale or adjust the server's Tailscale permissions.", + ); + } + } + void tailnetExited?.then(() => review.stop()).catch(() => undefined); + } const started = Date.now(); lifetimeTimeout = setTimeout( () => void review.stop().catch(() => undefined), diff --git a/apps/web/src/hooks/useOpenInCrit.logic.test.ts b/apps/web/src/hooks/useOpenInCrit.logic.test.ts new file mode 100644 index 000000000000..579eb548d400 --- /dev/null +++ b/apps/web/src/hooks/useOpenInCrit.logic.test.ts @@ -0,0 +1,116 @@ +import { + BearerConnectionProfile, + BearerConnectionTarget, + PrimaryConnectionTarget, + SshConnectionProfile, + SshConnectionTarget, + type ConnectionCatalogEntry, + type ConnectionRoute, + type PreparedConnection, +} from "@t3tools/client-runtime/connection"; +import { EnvironmentId, type DesktopSshEnvironmentTarget } from "@t3tools/contracts"; +import * as Option from "effect/Option"; +import { describe, expect, it } from "vite-plus/test"; + +import { resolveCritForwarding } from "./useOpenInCrit.logic"; + +const environmentId = EnvironmentId.make("crit-environment"); +const base = { environmentId, label: "Crit environment" }; +const sshTarget: DesktopSshEnvironmentTarget = { + alias: "dev", + hostname: "dev.example", + username: "user", + port: 22, +}; +const ssh: ConnectionRoute = { + target: new SshConnectionTarget({ ...base, connectionId: "ssh" }), + profile: Option.some( + new SshConnectionProfile({ + ...base, + connectionId: "ssh", + target: sshTarget, + }), + ), +}; + +function bearer(httpBaseUrl: string, network?: "tailscale"): ConnectionRoute { + return { + target: new BearerConnectionTarget({ ...base, connectionId: "direct" }), + profile: Option.some( + new BearerConnectionProfile({ + ...base, + connectionId: "direct", + httpBaseUrl, + wsBaseUrl: httpBaseUrl.replace(/^http/, "ws"), + ...(network ? { network } : {}), + }), + ), + }; +} + +function entry(preferred: ConnectionRoute, alternate: ConnectionRoute): ConnectionCatalogEntry { + return { ...preferred, enabled: true, alternateRoutes: [alternate] }; +} + +function prepared(route: ConnectionRoute, httpBaseUrl: string): PreparedConnection { + return { + ...base, + target: route.target, + httpBaseUrl, + socketUrl: httpBaseUrl.replace(/^http/, "ws"), + httpAuthorization: null, + }; +} + +describe("Crit forwarding", () => { + it("follows a reconnected SSH fallback rather than the preferred direct route", () => { + const direct = bearer("https://dev.tail123.ts.net/"); + const catalog = entry(direct, ssh); + expect(resolveCritForwarding(catalog, prepared(direct, "https://dev.tail123.ts.net/"))).toEqual( + { kind: "tailscale" }, + ); + expect(resolveCritForwarding(catalog, prepared(ssh, "http://127.0.0.1:4444/"))).toEqual({ + kind: "ssh", + target: sshTarget, + }); + }); + + it.each([ + ["https://dev.tail123.ts.net/", undefined], + ["http://[fd7a:115c:a1e0::1]:3773/", undefined], + ["http://100.64.1.2:3773/", "tailscale"], + ] as const)("forwards the connected Tailscale route at %s", (url, network) => { + const direct = bearer(url, network); + expect(resolveCritForwarding(entry(ssh, direct), prepared(direct, url))).toEqual({ + kind: "tailscale", + }); + }); + + it("also forwards a remote web client's primary tailnet origin", () => { + const httpBaseUrl = "https://dev.tail123.ts.net/"; + const primary: ConnectionRoute = { + target: new PrimaryConnectionTarget({ + ...base, + httpBaseUrl, + wsBaseUrl: httpBaseUrl.replace(/^http/, "ws"), + }), + profile: Option.none(), + }; + expect( + resolveCritForwarding({ ...primary, enabled: true }, prepared(primary, httpBaseUrl)), + ).toEqual({ kind: "tailscale" }); + }); + + it("keeps local reviews local and rejects unsupported or unprepared remote connections", () => { + const local = bearer("http://127.0.0.1:3773/"); + const catalog = entry(local, ssh); + expect(resolveCritForwarding(catalog, prepared(local, "http://127.0.0.1:3773/"))).toEqual({ + kind: "local", + }); + expect(() => resolveCritForwarding(catalog, null)).toThrow("Wait for it to reconnect"); + const remote = bearer("https://dev.example/"); + expect(() => + resolveCritForwarding(entry(remote, ssh), prepared(remote, "https://dev.example/")), + ).toThrow("Tailscale or SSH"); + }); +}); diff --git a/apps/web/src/hooks/useOpenInCrit.logic.ts b/apps/web/src/hooks/useOpenInCrit.logic.ts new file mode 100644 index 000000000000..0f616c72f440 --- /dev/null +++ b/apps/web/src/hooks/useOpenInCrit.logic.ts @@ -0,0 +1,41 @@ +import { + connectionRouteId, + connectionRouteKind, + connectionRoutes, + type ConnectionCatalogEntry, + type PreparedConnection, +} from "@t3tools/client-runtime/connection"; +import type { DesktopSshEnvironmentTarget } from "@t3tools/contracts"; +import { isLocalLoopbackHost, isTailnetHost } from "@t3tools/shared/hostClassification"; +import * as Option from "effect/Option"; + +type CritForwarding = + | { readonly kind: "local" } + | { readonly kind: "ssh"; readonly target: DesktopSshEnvironmentTarget } + | { readonly kind: "tailscale" }; + +/** Forward through the connected route, which can change after reconnecting. */ +export function resolveCritForwarding( + entry: ConnectionCatalogEntry | null, + connection: PreparedConnection | null, +): CritForwarding { + if (entry === null || connection === null) { + throw new Error("The environment is not ready to open Crit. Wait for it to reconnect."); + } + const route = connectionRoutes(entry).find( + (candidate) => connectionRouteId(candidate.target) === connectionRouteId(connection.target), + ); + if (connection.target._tag === "SshConnectionTarget") { + const profile = route ? Option.getOrNull(route.profile) : null; + if (profile?._tag !== "SshConnectionProfile") { + throw new Error("The SSH connection is not ready to forward Crit."); + } + return { kind: "ssh", target: profile.target }; + } + const hostname = new URL(connection.httpBaseUrl).hostname; + if ((route && connectionRouteKind(route) === "tailnet") || isTailnetHost(hostname)) { + return { kind: "tailscale" }; + } + if (isLocalLoopbackHost(hostname)) return { kind: "local" }; + throw new Error("Remote Crit reviews require a Tailscale or SSH connection."); +} diff --git a/apps/web/src/hooks/useOpenInCrit.ts b/apps/web/src/hooks/useOpenInCrit.ts index a5bb5e9801b3..a9faa2ab6f10 100644 --- a/apps/web/src/hooks/useOpenInCrit.ts +++ b/apps/web/src/hooks/useOpenInCrit.ts @@ -10,6 +10,9 @@ import { toastManager } from "../components/ui/toast"; import { shellEnvironment } from "../state/shell"; import { useEnvironment } from "../state/environments"; import { useAtomCommand } from "../state/use-atom-command"; +import { usePreparedConnection } from "../state/session"; +import { ensureLocalApi } from "../localApi"; +import { resolveCritForwarding } from "./useOpenInCrit.logic"; export function useOpenInCrit(environmentId: EnvironmentId | null): { open: (input: CritOpenInput) => Promise; @@ -17,39 +20,38 @@ export function useOpenInCrit(environmentId: EnvironmentId | null): { } { const command = useAtomCommand(shellEnvironment.openInCrit, { reportFailure: false }); const environment = useEnvironment(environmentId); + const prepared = usePreparedConnection(environmentId); const [opening, setOpening] = useState(false); const open = async (input: CritOpenInput): Promise => { if (environmentId === null || opening) return; setOpening(true); try { - const profile = environment?.entry.profile; - const sshTarget = - environment?.entry.target._tag === "SshConnectionTarget" && - profile && - Option.isSome(profile) && - profile.value._tag === "SshConnectionProfile" - ? profile.value.target - : null; - if (environment?.entry.target._tag === "SshConnectionTarget" && !sshTarget) { - throw new Error("The SSH connection is not ready to forward Crit."); - } + const forwarding = resolveCritForwarding( + environment?.entry ?? null, + Option.getOrNull(prepared), + ); let portlessPort: number | null = null; - if (sshTarget) { + if (forwarding.kind === "ssh") { const sync = window.desktopBridge?.syncSshPortlessForward; if (!sync) throw new Error("SSH Portless forwarding requires the desktop app."); - portlessPort = await sync({ target: sshTarget, cwd: input.cwd }); + portlessPort = await sync({ target: forwarding.target, cwd: input.cwd }); if (portlessPort === null) throw new Error("No SSH Portless forward is available for this project."); } const result = await command({ environmentId, - input: portlessPort === null ? input : { ...input, portlessPort }, + input: { + ...input, + ...(portlessPort === null ? {} : { portlessPort }), + ...(forwarding.kind === "tailscale" ? { tailscale: true } : {}), + }, }); + if (result._tag === "Success" && result.value === null && forwarding.kind !== "local") { + throw new Error("The remote server did not forward Crit. Update the server and try again."); + } if (result._tag === "Success" && result.value !== null) { - if (!(await window.desktopBridge?.openExternal(result.value))) { - throw new Error("Could not open the forwarded Crit review in the browser."); - } + await ensureLocalApi().shell.openExternal(result.value); } if (result._tag === "Failure" && !isAtomCommandInterrupted(result)) { const error = squashAtomCommandFailure(result); diff --git a/packages/contracts/src/crit.ts b/packages/contracts/src/crit.ts index 20178be6bade..e9d0f54f652a 100644 --- a/packages/contracts/src/crit.ts +++ b/packages/contracts/src/crit.ts @@ -10,6 +10,7 @@ import { export const CritOpenInput = Schema.Struct({ cwd: TrimmedNonEmptyString, portlessPort: Schema.optional(PositiveInt), + tailscale: Schema.optional(Schema.Boolean), scope: Schema.Union([ Schema.Struct({ kind: Schema.Literal("working-tree") }), Schema.Struct({ kind: Schema.Literal("branch"), baseRef: TrimmedNonEmptyString }), diff --git a/wiki/log.md b/wiki/log.md index 36cd173f7f9d..ad0e2c77b488 100644 --- a/wiki/log.md +++ b/wiki/log.md @@ -2,6 +2,8 @@ ## 2026-10-12 +- **Compatibility decision**: Recorded the [Crit remote review boundary](upstream-integration.md#crit-remote-review-boundary): review-owned, tailnet-only HTTPS endpoints stay separate from the environment's persistent publication. Implemented on this branch; live deployment remains separate. + - **Integration**: [PR #44](https://github.com/NicholasZolton/t3code/pull/44) reached fork `main` at `f4a2d2472c`. Refreshed [upstream integration](upstream-integration.md): all 374 newer commits through `e688034b5f` are incorporated with upstream ancestry preserved and zero remaining gap. Remote deployment remains separate. - **Branch integration**: [Upstream integration](upstream-integration.md) records local merge `eec69e1928` through upstream `e688034b5f`, with all 374 missing commits incorporated on the branch. Fork `main` remains at `e373457c74` with that gap still pending. Adopted upstream package boundaries and Git-only cleanup; Jujutsu workspaces remain retained and ambiguous reused-child OpenCode reports remain guarded. diff --git a/wiki/upstream-integration.md b/wiki/upstream-integration.md index a2fd60f37988..e547edfd5da9 100644 --- a/wiki/upstream-integration.md +++ b/wiki/upstream-integration.md @@ -6,7 +6,7 @@ tags: [upstream, fork, integration] status: stable generated: by: opencode/gpt-6.1-sol - at: 2026-10-12T06:04:06Z + at: 2026-10-12T21:22:55Z verified: - by: opencode/gpt-6.1-sol at: 2026-10-12T06:04:06Z @@ -18,6 +18,12 @@ snapshot: last_main_integration: eec69e1928b4aadf52e78fd2b14f9df6aa1f5f44 missing_upstream_on_main: 0 sources: + - id: crit-tailnet-forwarding + resource: ../apps/server/src/process/CritManager.ts + title: Review-owned Crit forwarding, implemented on this branch; deployment is separate + - id: tailscale-foreground-serve + resource: https://github.com/tailscale/tailscale/blob/3caf7d9e7dcaba589cfc58beda596929733e4fea/cmd/tailscale/cli/serve_v2.go + title: Tailscale foreground Serve sessions and their connection-owned lifetime - id: october-12-main-integration resource: https://github.com/NicholasZolton/t3code/pull/44 title: Completed October 12 full upstream integration into fork main @@ -383,6 +389,21 @@ At the October 12 review, upstream `e688034b5f` still has both destructive recovery paths reported in issue #15608. Preserve this boundary during later syncs unless upstream supplies equivalent recovery behavior.[^upstream-ssh-recovery][^ssh-recovery-issue] +## Crit remote review boundary + +Crit's frontend assumes its own origin root, so tailnet reviews use a separate, +temporary HTTPS endpoint rather than a subpath of the T3 server. Preserve +foreground Serve ownership: closing a review must remove only its endpoint, +never the environment's persistent publication or another review. This branch +supports Tailscale connections alongside SSH/Portless; T3 Connect-only forwarding +is outside that boundary. Crit has no application authentication, so these endpoints +must remain tailnet-only, not Funnel publications.[^crit-tailnet-forwarding][^tailscale-foreground-serve] + +Existing file shares can require elevated Serve access even for a proxy-only +change. On Unix, retry permission failures with non-interactive `sudo -n`; +never change the host's operator settings or persistent routes to open a +review.[^crit-tailnet-forwarding] + ## Private Android signing identity Installable private fork APKs use one persistent signing key, stored age-encrypted @@ -436,6 +457,10 @@ git log --reverse --oneline origin/main..upstream/main [^ssh-recovery]: Fork-side SSH implementation and its focused readiness-recovery regressions. +[^crit-tailnet-forwarding]: The review manager keeps Crit on loopback and owns each temporary forward's lifetime. + +[^tailscale-foreground-serve]: Foreground Serve uses a nested configuration tied to its IPN watcher; closing that session removes its publication. + [^upstream-ssh-recovery]: Exact upstream source reviewed on October 12. [^ssh-recovery-issue]: Open upstream issue and its stale-tunnel reconnect reproduction.