Merge pull request #59 from hack-dance/fix/review-comments-round2

fix: address post-merge review findings (JSON stdout purity, branch slug collisions, dedupe over-collapse)

Author: roodboi

src/backends/runtime-backend.ts

@@ -20,6 +20,11 @@ export interface RuntimeBaseOptions {
   readonly profiles?: readonly string[];
   readonly cwd: string;
   readonly env?: Record<string, string>;
+  /**
+   * 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"];

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<Record<string, string>>;
 }): Promise<number> {
   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<number> {
   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) {

src/init/validation.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<string> {
+  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 {

src/lib/branches.ts

@@ -58,6 +58,12 @@ export interface RunOptions {
   readonly cwd?: string;
   readonly env?: Record<string, string>;
   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",
   });
 

src/lib/shell.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");
+});

tests/branches-effective-branch.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",

tests/init-discovery-validation.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");
+});