fix(shell): address controller runtime review comments

Author: skulidropek

packages/app/src/docker-git/controller-compose-runtime.ts

@@ -26,10 +26,19 @@ export const loadControllerDockerRuntime = (): Effect.Effect<ControllerDockerRun
   )
 }
 
-const isolatedOverlayFileName = (composeFileName: string): string =>
-  composeFileName.endsWith(".yaml")
-    ? `${composeFileName.slice(0, -".yaml".length)}.isolated.yaml`
-    : `${composeFileName.slice(0, -".yml".length)}.isolated.yml`
+const isolatedOverlayFileName = (composeFileName: string): Effect.Effect<string, ControllerBootstrapError> => {
+  if (composeFileName.endsWith(".yaml")) {
+    return Effect.succeed(`${composeFileName.slice(0, -".yaml".length)}.isolated.yaml`)
+  }
+  if (composeFileName.endsWith(".yml")) {
+    return Effect.succeed(`${composeFileName.slice(0, -".yml".length)}.isolated.yml`)
+  }
+  return Effect.fail(
+    controllerBootstrapError(
+      `${controllerDockerRuntimeEnvKey}=isolated requires a .yml or .yaml compose file. Received: ${composeFileName}`
+    )
+  )
+}
 
 export const resolveControllerRuntimeOverlayPath = (
   composePath: string,
@@ -40,9 +49,10 @@ export const resolveControllerRuntimeOverlayPath = (
     : Effect.gen(function*(_) {
       const fs = yield* _(FileSystem.FileSystem)
       const path = yield* _(Path.Path)
+      const overlayFileName = yield* _(isolatedOverlayFileName(path.basename(composePath)))
       const runtimeOverlayPath = path.join(
         path.dirname(composePath),
-        isolatedOverlayFileName(path.basename(composePath))
+        overlayFileName
       )
       const exists = yield* _(fs.exists(runtimeOverlayPath).pipe(Effect.mapError(mapComposePathError)))
       return exists

packages/app/src/docker-git/controller-runtime.ts

@@ -4,6 +4,30 @@ export const defaultIsolatedProjectDockerHost = "tcp://host.docker.internal:2375
 
 export type ControllerDockerRuntime = "host" | "isolated"
 
+// CHANGE: parse and normalize the controller Docker runtime mode.
+// WHY: controller startup must distinguish host-backed Docker from the isolated embedded daemon fallback.
+// QUOTE(ТЗ): "Host-Docker-backed runtime is the intended default; isolated is opt-in fallback"
+// REF: packages/api/README.md
+// SOURCE: n/a
+// FORMAT THEOREM: forall raw: empty(trim(raw)) or trim(raw)=host -> host; trim(raw)=isolated -> isolated; otherwise -> null
+// PURITY: CORE
+// EFFECT: n/a
+// INVARIANT: parseControllerDockerRuntime is deterministic over trimmed input.
+// COMPLEXITY: O(1) time, O(1) space.
+/**
+ * Parses the controller Docker runtime mode from an environment value.
+ *
+ * @param raw - Raw `DOCKER_GIT_DOCKER_RUNTIME` value.
+ * @returns `"host"` for empty or host mode, `"isolated"` for isolated mode, or `null` for invalid input.
+ *
+ * @pure true
+ * @effect n/a
+ * @invariant Empty input and `"host"` normalize to `"host"`; `"isolated"` normalizes to `"isolated"`; all other values return `null`.
+ * @precondition `raw` is a finite string or `undefined`.
+ * @postcondition The result is exactly `"host"`, `"isolated"`, or `null`.
+ * @complexity O(1) time and O(1) space.
+ * @throws Never
+ */
 export const parseControllerDockerRuntime = (raw?: string): ControllerDockerRuntime | null => {
   const trimmed = raw?.trim() ?? ""
   if (trimmed.length === 0 || trimmed === "host") {
@@ -12,6 +36,31 @@ export const parseControllerDockerRuntime = (raw?: string): ControllerDockerRunt
   return trimmed === "isolated" ? "isolated" : null
 }
 
+// CHANGE: resolve the project-container Docker endpoint from controller runtime mode.
+// WHY: isolated controllers must inject a reachable embedded daemon endpoint, while host-backed mode keeps the host-socket default.
+// QUOTE(ТЗ): "when isolated, project containers default to tcp://host.docker.internal:2375"
+// REF: packages/api/README.md
+// SOURCE: n/a
+// FORMAT THEOREM: forall runtime, raw: runtime=isolated -> nonempty(trim(raw) or defaultIsolatedProjectDockerHost); runtime=host -> trim(raw) or empty
+// PURITY: CORE
+// EFFECT: n/a
+// INVARIANT: isolated runtime always returns a non-empty Docker host string.
+// COMPLEXITY: O(1) time, O(1) space.
+/**
+ * Resolves the Docker host URL passed to project containers.
+ *
+ * @param runtime - Normalized controller runtime mode.
+ * @param rawProjectDockerHost - Raw `DOCKER_GIT_PROJECT_DOCKER_HOST` override.
+ * @returns The trimmed project Docker host, the isolated default endpoint, or an empty host-mode value.
+ *
+ * @pure true
+ * @effect n/a
+ * @invariant Isolated runtime returns a non-empty endpoint; host runtime returns the explicit endpoint or an empty string.
+ * @precondition `runtime` is a valid `ControllerDockerRuntime`; `rawProjectDockerHost` is a finite string or `undefined`.
+ * @postcondition Result is non-empty in isolated mode and preserves host-mode emptiness when no override exists.
+ * @complexity O(1) time and O(1) space.
+ * @throws Never
+ */
 export const resolveProjectDockerHostForRuntime = (
   runtime: ControllerDockerRuntime,
   rawProjectDockerHost?: string

packages/app/tests/docker-git/controller-compose.test.ts

@@ -5,6 +5,7 @@ import { describe, expect, it } from "@effect/vitest"
 import { Effect } from "effect"
 import * as fc from "fast-check"
 
+import { resolveControllerRuntimeOverlayPath } from "../../src/docker-git/controller-compose-runtime.js"
 import {
   controllerBuildSkillerEnvKey,
   controllerGpuModeEnvKey,
@@ -107,6 +108,7 @@ type PreparedRevision = {
 }
 
 type ControllerBuildSkillerFixtureMode = "0" | "1" | undefined
+type ControllerDockerRuntimeEnvFixtureMode = "host" | "isolated" | undefined
 
 type PrepareRevisionFixture = {
   readonly buildSkillerMode: ControllerBuildSkillerFixtureMode
@@ -118,6 +120,11 @@ const controllerBuildSkillerFixtureModeArbitrary = fc.constantFrom<ControllerBui
   "0",
   "1"
 )
+const controllerDockerRuntimeEnvFixtureModeArbitrary = fc.constantFrom<ControllerDockerRuntimeEnvFixtureMode>(
+  undefined,
+  "host",
+  "isolated"
+)
 const prepareRevisionFixtureArbitrary: fc.Arbitrary<PrepareRevisionFixture> = fc
   .record({
     buildSkillerMode: controllerBuildSkillerFixtureModeArbitrary,
@@ -126,18 +133,27 @@ const prepareRevisionFixtureArbitrary: fc.Arbitrary<PrepareRevisionFixture> = fc
   .filter(({ buildSkillerMode, includeSkillerPackage }) => buildSkillerMode === "0" || includeSkillerPackage)
 const controllerRevisionPattern = /^[a-f0-9]{16}-host-none-skiller[01]$/u
 
+const withMinimalControllerRoot = <A, E, R>(
+  effect: (rootDir: string) => Effect.Effect<A, E, R>
+) =>
+  Effect.scoped(
+    Effect.gen(function*(_) {
+      const rootDir = yield* _(temporaryControllerRoot)
+      yield* _(writeMinimalCompose(rootDir))
+      yield* _(withWorkingDirectory(rootDir))
+      return yield* _(effect(rootDir))
+    })
+  )
+
 const prepareRevisionInTemporaryRoot = ({
   buildSkillerMode,
   includeSkillerPackage
 }: PrepareRevisionFixture) =>
-  Effect.scoped(
+  withMinimalControllerRoot((rootDir) =>
     Effect.gen(function*(_) {
-      const rootDir = yield* _(temporaryControllerRoot)
-      yield* _(writeMinimalCompose(rootDir))
       if (includeSkillerPackage) {
         yield* _(writeSkillerPackage(rootDir))
       }
-      yield* _(withWorkingDirectory(rootDir))
       yield* _(
         withControllerEnv([
           [controllerBuildSkillerEnvKey, buildSkillerMode],
@@ -165,6 +181,23 @@ const expectPreparedRevisionInvariants = (fixture: PrepareRevisionFixture, prepa
   expect(prepared.revision.endsWith(expectedSkillerSuffixForMode(fixture.buildSkillerMode))).toBe(true)
 }
 
+const resolveComposeFilesInTemporaryRoot = (
+  dockerRuntimeMode: ControllerDockerRuntimeEnvFixtureMode
+) =>
+  withMinimalControllerRoot((rootDir) =>
+    Effect.gen(function*(_) {
+      yield* _(writeMinimalIsolatedCompose(rootDir))
+      yield* _(
+        withControllerEnv([
+          [controllerBuildSkillerEnvKey, "0"],
+          [controllerDockerRuntimeEnvKey, dockerRuntimeMode],
+          [controllerGpuModeEnvKey, undefined]
+        ])
+      )
+      return yield* _(resolveControllerComposeFiles())
+    })
+  ).pipe(Effect.provide(NodeContext.layer))
+
 const assertControllerComposeProperty = <PropertyArgs>(property: fc.IAsyncProperty<PropertyArgs>) =>
   Effect.tryPromise({
     catch: (cause) => cause,
@@ -217,32 +250,38 @@ describe("controller compose preparation", () => {
       })
     ).pipe(Effect.provide(NodeContext.layer)))
 
-  /* jscpd:ignore-start */
   it.effect("adds the isolated runtime overlay only for isolated controller mode", () =>
+    assertControllerComposeProperty(
+      fc.asyncProperty(controllerDockerRuntimeEnvFixtureModeArbitrary, (dockerRuntimeMode) =>
+        Effect.runPromise(
+          resolveComposeFilesInTemporaryRoot(dockerRuntimeMode).pipe(
+            Effect.tap((files) =>
+              Effect.sync(() => {
+                if (dockerRuntimeMode === "isolated") {
+                  expect(files.runtimeOverlayPath).toBeDefined()
+                  expect(files.runtimeOverlayPath?.endsWith("docker-compose.isolated.yml")).toBe(true)
+                  return
+                }
+                expect(files.runtimeOverlayPath).toBeNull()
+              })
+            ),
+            Effect.asVoid
+          )
+        ))
+    ))
+
+  it.effect("rejects unsupported compose filename extensions for isolated controller mode", () =>
     Effect.scoped(
       Effect.gen(function*(_) {
+        const path = yield* _(Path.Path)
         const rootDir = yield* _(temporaryControllerRoot)
-        yield* _(writeMinimalCompose(rootDir))
-        yield* _(writeMinimalIsolatedCompose(rootDir))
-        yield* _(withWorkingDirectory(rootDir))
-
-        yield* _(
-          withControllerEnv([
-            [controllerBuildSkillerEnvKey, "0"],
-            [controllerDockerRuntimeEnvKey, "host"],
-            [controllerGpuModeEnvKey, undefined]
-          ])
+        const error = yield* _(
+          resolveControllerRuntimeOverlayPath(path.join(rootDir, "docker-compose.json"), "isolated").pipe(Effect.flip)
         )
-        const hostFiles = yield* _(resolveControllerComposeFiles())
-        expect(hostFiles.runtimeOverlayPath).toBeNull()
-
-        yield* _(withControllerEnv([[controllerDockerRuntimeEnvKey, "isolated"]]))
-        const isolatedFiles = yield* _(resolveControllerComposeFiles())
-        expect(isolatedFiles.runtimeOverlayPath).toBeDefined()
-        expect(isolatedFiles.runtimeOverlayPath?.endsWith("docker-compose.isolated.yml")).toBe(true)
+        expect(error._tag).toBe("ControllerBootstrapError")
+        expect(error.message).toContain(".yml or .yaml")
       })
     ).pipe(Effect.provide(NodeContext.layer)))
-  /* jscpd:ignore-end */
 
   it.effect("prepares and persists host controller revisions for Skiller build modes", () =>
     assertControllerComposeProperty(

packages/app/tests/docker-git/controller.test.ts

@@ -58,6 +58,26 @@ const controllerSourceRevisionArbitrary = fc
 const controllerGpuModeArbitrary = fc.constantFrom<"none" | "all">("none", "all")
 const controllerBuildSkillerModeArbitrary = fc.constantFrom<"0" | "1">("0", "1")
 const controllerDockerRuntimeArbitrary = fc.constantFrom<"host" | "isolated">("host", "isolated")
+const dockerHostArbitrary = fc.constantFrom<string | undefined>(
+  undefined,
+  "",
+  " unix:///var/run/docker.sock ",
+  "tcp://docker.example.test:2376",
+  " ssh://docker@example.test "
+)
+const explicitApiBaseUrlArbitrary = fc.option(fc.webUrl(), { nil: undefined })
+const dockerNetworkNameArbitrary = fc
+  .string({ maxLength: 16, minLength: 1 })
+  .filter((value) => value.trim().length > 0)
+const dockerNetworkIpArbitrary = fc
+  .tuple(
+    fc.integer({ max: 255, min: 1 }),
+    fc.integer({ max: 255, min: 0 }),
+    fc.integer({ max: 255, min: 0 }),
+    fc.integer({ max: 254, min: 1 })
+  )
+  .map((octets) => joinIp(...octets.map(String)))
+const dockerNetworkIpsArbitrary = fc.dictionary(dockerNetworkNameArbitrary, dockerNetworkIpArbitrary)
 
 describe("controller reachability", () => {
   it.effect("builds direct API candidates without Docker inspection", () =>
@@ -128,33 +148,37 @@ describe("controller reachability", () => {
 
   it.effect("requires an explicit API URL only for non-inspectable remote Docker hosts", () =>
     Effect.sync(() => {
-      expect(
-        shouldRequireExplicitApiUrlForRemoteDocker("tcp://docker.example.test:2376", undefined, {})
-      ).toBe(true)
-      expect(
-        shouldRequireExplicitApiUrlForRemoteDocker(
-          "tcp://docker.example.test:2376",
-          makeHttpUrl("api.example.test", "3334"),
-          {}
-        )
-      ).toBe(false)
-      expect(
-        shouldRequireExplicitApiUrlForRemoteDocker(
-          "tcp://host.docker.internal:2375",
-          undefined,
-          { bridge: joinIp("172", "17", "0", "2") }
+      fc.assert(
+        fc.property(
+          dockerHostArbitrary,
+          explicitApiBaseUrlArbitrary,
+          dockerNetworkIpsArbitrary,
+          (dockerHost, explicitApiBaseUrl, currentContainerNetworks) => {
+            const expected = isRemoteDockerHost(dockerHost) &&
+              explicitApiBaseUrl === undefined &&
+              Object.keys(currentContainerNetworks).length === 0
+
+            expect(
+              shouldRequireExplicitApiUrlForRemoteDocker(dockerHost, explicitApiBaseUrl, currentContainerNetworks)
+            ).toBe(expected)
+          }
         )
-      ).toBe(false)
-      expect(
-        shouldRequireExplicitApiUrlForRemoteDocker("unix:///var/run/docker.sock", undefined, {})
-      ).toBe(false)
+      )
     }))
 
   it.effect("resolves the current container name from HOSTNAME or OS hostname", () =>
     Effect.sync(() => {
-      expect(resolveCurrentContainerName(" env-container ", "os-container")).toBe("env-container")
-      expect(resolveCurrentContainerName("", " os-container ")).toBe("os-container")
-      expect(resolveCurrentContainerName(undefined, " os-container ")).toBe("os-container")
+      fc.assert(
+        fc.property(
+          fc.option(fc.string(), { nil: undefined }),
+          fc.string(),
+          (envHostname, systemHostname) => {
+            expect(resolveCurrentContainerName(envHostname, systemHostname)).toBe(
+              envHostname?.trim() || systemHostname.trim()
+            )
+          }
+        )
+      )
     }))
 
   it.effect("parses controller revision from container env output", () =>

scripts/e2e/browser-command.sh

@@ -160,7 +160,7 @@ wait_for_controller_health() {
 
   for _ in $(seq 1 "$attempts"); do
     logged_url="$(read_logged_api_base_url || true)"
-    for candidate in "$local_url" "$logged_url"; do
+    for candidate in "$logged_url" "$local_url"; do
       if [[ -z "$candidate" ]]; then
         continue
       fi