From 11f57886c5a725d03a95a2a7414865c6b1b481b9 Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Tue, 7 Jul 2026 21:49:06 -0400 Subject: [PATCH] fix: address post-merge review findings across PRs 54 and 58 1. up/down/restart --json route compose subprocess stdout to stderr so stdout stays a single parseable envelope (shell.run gains fd-2 routing; threaded through the compose backend and all four lifecycle call sites). 2. Auto branch slugs are collision-resistant: when two linked worktrees' branches sanitize to the same slug (feature/api vs feature-api), both deterministically get a short raw-name hash suffix instead of silently sharing one branch instance. 3. Per-package dedupe groups by (package, script suffix): distinct dev:web/dev:api scripts in a single-package repo are distinct services and all survive; only same-service variants collapse. Co-Authored-By: Claude Fable 5 --- src/backends/runtime-backend.ts | 22 ++++++++++-- src/commands/project.ts | 8 +++++ src/init/validation.ts | Bin 28496 -> 28962 bytes src/lib/branches.ts | 42 ++++++++++++++++++++++- src/lib/shell.ts | 8 ++++- tests/branches-effective-branch.test.ts | 43 ++++++++++++++++++++++++ tests/init-discovery-validation.test.ts | 10 ++++++ tests/runtime-backend.test.ts | 42 ++++++++++++++++++++++- 8 files changed, 169 insertions(+), 6 deletions(-) diff --git a/src/backends/runtime-backend.ts b/src/backends/runtime-backend.ts index f1e4b445..7d0075d5 100644 --- a/src/backends/runtime-backend.ts +++ b/src/backends/runtime-backend.ts @@ -20,6 +20,11 @@ export interface RuntimeBaseOptions { readonly profiles?: readonly string[]; readonly cwd: string; readonly env?: Record; + /** + * Route subprocess stdout to stderr — required by `--json` callers whose + * stdout must remain a single parseable envelope. + */ + readonly routeStdoutToStderr?: boolean; } export interface RuntimeUpOptions extends RuntimeBaseOptions { @@ -68,7 +73,11 @@ export const composeRuntimeBackend: RuntimeBackend = { ...(opts.detach ? ["-d"] : []), ]; if (opts.detach) { - return await run(cmd, { cwd: opts.cwd, env: opts.env }); + return await run(cmd, { + cwd: opts.cwd, + env: opts.env, + stdout: opts.routeStdoutToStderr ? "stderr" : "inherit", + }); } const env = mergeSpawnEnv(opts.env); @@ -80,8 +89,11 @@ export const composeRuntimeBackend: RuntimeBackend = { stderr: "pipe", }); + const stdoutTarget = opts.routeStdoutToStderr + ? process.stderr + : process.stdout; const stdoutGrouper = createStructuredLogGrouper({ - write: (text) => process.stdout.write(text), + write: (text) => stdoutTarget.write(text), }); const stderrGrouper = createStructuredLogGrouper({ write: (text) => process.stderr.write(text), @@ -107,7 +119,11 @@ export const composeRuntimeBackend: RuntimeBackend = { }, async down(opts) { const cmd = [...buildComposeArgs(opts), "down"]; - return await run(cmd, { cwd: opts.cwd, env: opts.env }); + return await run(cmd, { + cwd: opts.cwd, + env: opts.env, + stdout: opts.routeStdoutToStderr ? "stderr" : "inherit", + }); }, async psJson(opts) { const cmd = [...buildComposeArgs(opts), "ps", "--format", "json"]; diff --git a/src/commands/project.ts b/src/commands/project.ts index b207a1ad..d4f2761a 100644 --- a/src/commands/project.ts +++ b/src/commands/project.ts @@ -5431,6 +5431,7 @@ async function runUpCommand({ detach, cwd: dirname(project.composeFile), env: envOverrides.env, + routeStdoutToStderr: json, }); if (upCode !== 0) { await removeProjectRuntimeStateEntry({ @@ -5673,6 +5674,7 @@ async function runDownCommand({ composeProject: composeProjectName, profiles, cwd: dirname(project.composeFile), + routeStdoutToStderr: json, }); try { @@ -5756,6 +5758,7 @@ async function runRestartDownPhase(opts: { readonly lifecycleComposeProject: string; readonly profiles: readonly string[]; readonly branch: string | null; + readonly routeStdoutToStderr?: boolean; readonly envForCompose: Readonly>; }): Promise { const downBefore = await runLifecycleCommands({ @@ -5775,6 +5778,7 @@ async function runRestartDownPhase(opts: { composeProject: opts.composeProjectName, profiles: opts.profiles, cwd: dirname(opts.project.composeFile), + routeStdoutToStderr: opts.routeStdoutToStderr === true, }); try { await stopLifecycleProcesses({ @@ -5821,6 +5825,7 @@ async function runRestartUpPhase(opts: { readonly branch: string | null; readonly envName?: string | null; readonly detach?: boolean; + readonly routeStdoutToStderr?: boolean; }): Promise { await maybeSyncOauthAliasesInCompose({ project: opts.project }); @@ -5906,6 +5911,7 @@ async function runRestartUpPhase(opts: { detach: opts.detach === true, cwd: dirname(opts.project.composeFile), env: envOverrides.env, + routeStdoutToStderr: opts.routeStdoutToStderr === true, }); if (upCode !== 0) { await removeProjectRuntimeStateEntry({ @@ -6043,6 +6049,7 @@ async function runRestartCommand({ profiles, branch, envForCompose: lifecycleEnv, + routeStdoutToStderr: json, }); if (downCode !== 0) { if (json) { @@ -6074,6 +6081,7 @@ async function runRestartCommand({ envName: effectiveEnvName, // `--json` implies detach so the command terminates with a result. detach: json, + routeStdoutToStderr: json, }); if (!json) { diff --git a/src/init/validation.ts b/src/init/validation.ts index 2f53430924427dc456e1eefe816424f59c6674d5..61af48892224d5580c2c2cb19746f535233daa4e 100644 GIT binary patch delta 518 zcmYk3y-or_6on;rBqj)IXFS&~!32*GD?eF6(h zV?yI&_yiXAzJY}^EJml9%>DV!y=UG(a!;SR*XzQ%?lKLZlo#_$g63h48{4 z2&EVx?nwis%MBKBB%EccCg5&O^-b8#P|~+BCIrHcbs|8}Q+69;MM8D7rC1w4nQ=F> z4Uw>zGpUtP6M{vG(wFU($e zD`A|k`~m7*`qXcCDo(?xVguXNDG6x6asE^7f9%3757Snuv@n`053@N(ZZmx=I`bx< OeihH{JvlFZU;F^ZhO!L+ delta 120 zcmZ4Vi1ETb#tm5l8s(WK8JT$sB^jv-#fiD83I&PD*@@|?IttmTsRfyN>0An03i)|C zm6N9kESub7EVMaSsFHE=8xPLO`$YJJlN0k&GE)*uQuUzvJX0p07YV^8zFAH5Pc#6@ C?Jaiz diff --git a/src/lib/branches.ts b/src/lib/branches.ts index 1d2b6d24..4b4a5b62 100644 --- a/src/lib/branches.ts +++ b/src/lib/branches.ts @@ -6,6 +6,7 @@ import { import { ensureDir, readTextFile, writeTextFileIfChanged } from "./fs.ts"; import { isLinkedGitWorktree, + listGitWorktrees, resolveGitCurrentBranch, } from "./git-worktree.ts"; import { getString, isRecord } from "./guards.ts"; @@ -213,7 +214,46 @@ export async function resolveEffectiveBranch(opts: { return { branch: null, source: "none", gitBranch }; } - return { branch: slug, source: "worktree", gitBranch }; + const branch = await resolveCollisionFreeBranchSlug({ + slug, + gitBranch, + projectRoot: opts.projectRoot, + }); + return { branch, source: "worktree", gitBranch }; +} + +/** + * Guards the auto-derived branch slug against sanitization collisions: + * `feature/api` and `feature-api` both sanitize to `feature-api`, which + * would silently share one branch instance between two worktrees (and let + * either stop the other's containers). When another linked worktree's + * branch sanitizes to the same slug from a DIFFERENT raw name, both sides + * deterministically get a short raw-name hash suffix, keeping every + * worktree's derived instance distinct and stable across invocations. + */ +async function resolveCollisionFreeBranchSlug(opts: { + readonly slug: string; + readonly gitBranch: string; + readonly projectRoot: string; +}): Promise { + const worktrees = await listGitWorktrees({ repoRoot: opts.projectRoot }); + if (worktrees === null) { + return opts.slug; + } + const colliding = worktrees.some( + (entry) => + entry.branch !== null && + entry.branch !== opts.gitBranch && + sanitizeBranchSlug(entry.branch) === opts.slug + ); + if (!colliding) { + return opts.slug; + } + const hash = new Bun.CryptoHasher("sha1") + .update(opts.gitBranch) + .digest("hex") + .slice(0, 4); + return `${opts.slug}-${hash}`; } function defaultBranchesFile(): BranchesFile { diff --git a/src/lib/shell.ts b/src/lib/shell.ts index d1a8822d..9b54aaed 100644 --- a/src/lib/shell.ts +++ b/src/lib/shell.ts @@ -58,6 +58,12 @@ export interface RunOptions { readonly cwd?: string; readonly env?: Record; readonly stdin?: "inherit" | "pipe" | "ignore"; + /** + * Route the child's stdout to THIS process's stderr (fd 2). Used by + * `--json` code paths where stdout must stay a single parseable + * envelope while subprocess output remains visible to humans. + */ + readonly stdout?: "inherit" | "stderr"; } export async function run( @@ -68,7 +74,7 @@ export async function run( cwd: opts.cwd, env: buildSpawnEnv(opts.env), stdin: opts.stdin ?? "inherit", - stdout: "inherit", + stdout: opts.stdout === "stderr" ? 2 : "inherit", stderr: "inherit", }); diff --git a/tests/branches-effective-branch.test.ts b/tests/branches-effective-branch.test.ts index a3de6c7e..1c7b3ccb 100644 --- a/tests/branches-effective-branch.test.ts +++ b/tests/branches-effective-branch.test.ts @@ -134,3 +134,46 @@ test("non-git directories resolve to the base instance", async () => { expect(resolved.source).toBe("none"); expect(resolved.branch).toBeNull(); }); + +test("colliding sanitized slugs across worktrees get deterministic distinct suffixes", async () => { + const fixture = await createFixture({ branch: "feature/api" }); + const secondRoot = resolve(fixture.primaryRoot, "..", "linked-2"); + await runGit( + ["worktree", "add", "-b", "feature-api", secondRoot], + fixture.primaryRoot + ); + + const first = await resolveEffectiveBranch({ + explicitBranch: null, + projectRoot: fixture.linkedRoot, + autoBranchEnabled: true, + }); + const second = await resolveEffectiveBranch({ + explicitBranch: null, + projectRoot: secondRoot, + autoBranchEnabled: true, + }); + + expect(first.branch).not.toBe(second.branch); + expect(first.branch).toStartWith("feature-api-"); + expect(second.branch).toStartWith("feature-api-"); + + const firstAgain = await resolveEffectiveBranch({ + explicitBranch: null, + projectRoot: fixture.linkedRoot, + autoBranchEnabled: true, + }); + expect(firstAgain.branch).toBe(first.branch); +}); + +test("non-colliding worktree slugs stay unsuffixed", async () => { + const fixture = await createFixture({ branch: "feature/solo" }); + + const resolved = await resolveEffectiveBranch({ + explicitBranch: null, + projectRoot: fixture.linkedRoot, + autoBranchEnabled: true, + }); + + expect(resolved.branch).toBe("feature-solo"); +}); diff --git a/tests/init-discovery-validation.test.ts b/tests/init-discovery-validation.test.ts index 7b59166e..47314aa6 100644 --- a/tests/init-discovery-validation.test.ts +++ b/tests/init-discovery-validation.test.ts @@ -125,6 +125,16 @@ test("dedupeCandidatesByPackage keeps the highest-scored script per package", () expect(result.dropped).toEqual([{ candidate: start, keptScriptName: "dev" }]); }); +test("dedupeCandidatesByPackage preserves distinct dev:* services in a single package", () => { + const web = candidate({ id: "root:dev:web", scriptName: "dev:web" }); + const api = candidate({ id: "root:dev:api", scriptName: "dev:api" }); + + const result = dedupeCandidatesByPackage({ candidates: [web, api] }); + + expect(result.selected).toEqual([web, api]); + expect(result.dropped).toEqual([]); +}); + test("dedupeAggregatorCandidates drops a root --filter aggregator in favor of the package's own script", () => { const webDev = candidate({ id: "apps/web/package.json:dev", diff --git a/tests/runtime-backend.test.ts b/tests/runtime-backend.test.ts index 3aad8ffe..2fd25d18 100644 --- a/tests/runtime-backend.test.ts +++ b/tests/runtime-backend.test.ts @@ -3,6 +3,7 @@ import { afterAll, beforeAll, beforeEach, expect, test } from "bun:test"; import { registerScopedModuleMock } from "./helpers/scoped-module-mock.ts"; const runCalls: string[][] = []; +const runOpts: { stdout?: string }[] = []; const execCalls: string[][] = []; const shellMock = await registerScopedModuleMock({ @@ -17,8 +18,12 @@ const shellMock = await registerScopedModuleMock({ execCalls.push([...cmd]); return { exitCode: 0, stdout: "", stderr: "" }; }, - run: async (cmd: readonly string[]) => { + run: async ( + cmd: readonly string[], + opts: { readonly stdout?: string } = {} + ) => { runCalls.push([...cmd]); + runOpts.push({ stdout: opts.stdout }); return 0; }, findExecutableInPath: () => "/usr/bin/docker", @@ -56,6 +61,7 @@ beforeAll(() => { beforeEach(() => { runCalls.length = 0; + runOpts.length = 0; execCalls.length = 0; }); @@ -287,3 +293,37 @@ test("composeRuntimeBackend.exec disables TTY for non-interactive sessions", asy "dev", ]); }); + +test("detached up routes stdout to stderr when requested (--json purity)", async () => { + const composeRuntimeBackend = await loadComposeRuntimeBackend(); + await composeRuntimeBackend.up({ + composeFiles: ["/tmp/compose.yml"], + composeProject: "demo", + detach: true, + cwd: "/tmp", + routeStdoutToStderr: true, + }); + expect(runOpts[0]?.stdout).toBe("stderr"); +}); + +test("detached up inherits stdout by default", async () => { + const composeRuntimeBackend = await loadComposeRuntimeBackend(); + await composeRuntimeBackend.up({ + composeFiles: ["/tmp/compose.yml"], + composeProject: "demo", + detach: true, + cwd: "/tmp", + }); + expect(runOpts[0]?.stdout).toBe("inherit"); +}); + +test("down routes stdout to stderr when requested (--json purity)", async () => { + const composeRuntimeBackend = await loadComposeRuntimeBackend(); + await composeRuntimeBackend.down({ + composeFiles: ["/tmp/compose.yml"], + composeProject: "demo", + cwd: "/tmp", + routeStdoutToStderr: true, + }); + expect(runOpts[0]?.stdout).toBe("stderr"); +});