fix(effect): address PR review compliance findings

- Validate clonedOnHostname at API/config boundaries and keep command builders pure by passing host identity explicitly.
- Serialize terminal session state mutations per project to prevent stale read/write races.
- Harden session-sync GitHub decoders, readiness polling, PR comment logging, and regression/property tests.

Proof of fix:
- Cause: review found weak schema contracts, unsynchronized durable terminal writes, silent best-effort failures, and incomplete regression tests.
- Solution: moved shared Effect lint rules into one module, strengthened decoders and hostname schemas, added project-scoped semaphores around state writers, and added focused unit/property tests.
- Verification: api test/typecheck/lint, root typecheck/test/lint, session-sync typecheck/test, lint:effect, and git diff --check passed locally.

Author: skulidropek

bun.lock

@@ -0,0 +1,37 @@
+export const effectMigrationWarnings = [
+  {
+    selector: "SwitchStatement",
+    message: "Effect migration blocker: use Match.exhaustive instead of switch."
+  },
+  {
+    selector: "TryStatement",
+    message: "Effect migration blocker: use Effect.try / Effect.catch* instead of try/catch."
+  },
+  {
+    selector: "AwaitExpression",
+    message: "Effect migration blocker: use Effect.gen / Effect.flatMap instead of await."
+  },
+  {
+    selector: "FunctionDeclaration[async=true], FunctionExpression[async=true], ArrowFunctionExpression[async=true]",
+    message: "Effect migration blocker: use Effect.gen / Effect.tryPromise instead of async functions."
+  },
+  {
+    selector: "NewExpression[callee.name='Promise']",
+    message: "Effect migration blocker: use Effect.async / Effect.tryPromise instead of new Promise."
+  },
+  {
+    selector: "CallExpression[callee.object.name='Promise']",
+    message: "Effect migration blocker: use Effect combinators instead of Promise.*."
+  }
+]
+
+export const effectPromiseRestrictedTypes = {
+  types: {
+    Promise: {
+      message: "Effect migration blocker: avoid Promise in public types. Use Effect.Effect<A, E, R>."
+    },
+    "Promise<*>": {
+      message: "Effect migration blocker: avoid Promise<T>. Use Effect.Effect<T, E, R>."
+    }
+  }
+}

eslint.effect-ts-shared.mjs

@@ -12,32 +12,7 @@ import eslintComments from "@eslint-community/eslint-plugin-eslint-comments"
 import globals from "globals"
 import tseslint from "typescript-eslint"
 
-const migrationBlockers = [
-  {
-    selector: "SwitchStatement",
-    message: "Effect migration blocker: use Match.exhaustive instead of switch."
-  },
-  {
-    selector: "TryStatement",
-    message: "Effect migration blocker: use Effect.try / Effect.catch* instead of try/catch."
-  },
-  {
-    selector: "AwaitExpression",
-    message: "Effect migration blocker: use Effect.gen / Effect.flatMap instead of await."
-  },
-  {
-    selector: "FunctionDeclaration[async=true], FunctionExpression[async=true], ArrowFunctionExpression[async=true]",
-    message: "Effect migration blocker: use Effect.gen / Effect.tryPromise instead of async functions."
-  },
-  {
-    selector: "NewExpression[callee.name='Promise']",
-    message: "Effect migration blocker: use Effect.async / Effect.tryPromise instead of new Promise."
-  },
-  {
-    selector: "CallExpression[callee.object.name='Promise']",
-    message: "Effect migration blocker: use Effect combinators instead of Promise.*."
-  }
-]
+import { effectMigrationWarnings, effectPromiseRestrictedTypes } from "../../eslint.effect-ts-shared.mjs"
 
 export default tseslint.config({
   name: "api-effect-ts-compliance",
@@ -58,21 +33,12 @@ export default tseslint.config({
       "ts-nocheck": true
     }],
     "@typescript-eslint/no-explicit-any": "error",
-    "@typescript-eslint/no-restricted-types": ["warn", {
-      types: {
-        Promise: {
-          message: "Avoid Promise in public types. Use Effect.Effect<A, E, R>."
-        },
-        "Promise<*>": {
-          message: "Avoid Promise<T>. Use Effect.Effect<T, E, R>."
-        }
-      }
-    }],
+    "@typescript-eslint/no-restricted-types": ["warn", effectPromiseRestrictedTypes],
     "eslint-comments/disable-enable-pair": "error",
     "eslint-comments/no-unlimited-disable": "error",
     "eslint-comments/no-unused-disable": "error",
     "eslint-comments/no-use": "error",
     "no-console": "error",
-    "no-restricted-syntax": ["warn", ...migrationBlockers]
+    "no-restricted-syntax": ["warn", ...effectMigrationWarnings]
   }
 })

packages/api/eslint.effect-ts-check.config.mjs

@@ -26,7 +26,7 @@ export type ProjectSummary = {
   readonly sshSessions: number
   readonly startedAtIso: string | null
   readonly startedAtEpochMs: number | null
-  readonly clonedOnHostname?: string | undefined
+  readonly clonedOnHostname?: string
 }
 
 export type ProjectDetails = ProjectSummary & {
@@ -480,7 +480,20 @@ export type CreateProjectRequest = {
   readonly forceEnv?: boolean | undefined
   readonly waitForClone?: boolean | undefined
   readonly async?: boolean | undefined
-  readonly clonedOnHostname?: string | undefined
+  /**
+   * Hostname of the machine where the project was cloned.
+   *
+   * CHANGE: add explicit clone-origin hostname to the create-project contract.
+   * WHY: keeps command builders pure by passing host identity as immutable input instead of reading OS state.
+   * QUOTE(ТЗ): "CORE: Исключительно чистые функции, неизменяемые данные, математические операции"
+   * REF: pr-420-coderabbit-review-4518791377
+   * SOURCE: n/a
+   * FORMAT THEOREM: ∀h ∈ Hostname: request(h) -> build(request).config.clonedOnHostname = h
+   * PURITY: CORE - immutable request data.
+   * INVARIANT: if present, value satisfies API HostnameSchema.
+   * COMPLEXITY: O(1)/O(1)
+   */
+  readonly clonedOnHostname?: string
 }
 
 export type AgentEnvVar = {

packages/api/src/api/contracts.ts

@@ -12,6 +12,11 @@ export {
 const OptionalString = Schema.optional(Schema.String)
 const OptionalBoolean = Schema.optional(Schema.Boolean)
 const OptionalNullableString = Schema.optional(Schema.NullOr(Schema.String))
+const HostnameSchema = Schema.String.pipe(
+  Schema.minLength(1),
+  Schema.maxLength(253),
+  Schema.pattern(/^(?:[A-Za-z0-9](?:[A-Za-z0-9-]{0,61}[A-Za-z0-9])?)(?:\.[A-Za-z0-9](?:[A-Za-z0-9-]{0,61}[A-Za-z0-9])?)*$/u)
+)
 
 export const CreateProjectRequestSchema = Schema.Struct({
   repoUrl: OptionalString,
@@ -52,7 +57,7 @@ export const CreateProjectRequestSchema = Schema.Struct({
   forceEnv: OptionalBoolean,
   waitForClone: OptionalBoolean,
   async: OptionalBoolean,
-  clonedOnHostname: OptionalString
+  clonedOnHostname: Schema.optional(HostnameSchema)
 })
 
 export const GithubAuthLoginRequestSchema = Schema.Struct({

packages/api/src/api/schema.ts

@@ -1426,7 +1426,11 @@ export const makeRouter = () => {
       "/projects",
       Effect.gen(function*(_) {
         const request = yield* _(readCreateProjectRequest())
-        const result = yield* _(createProjectFromRequest(request))
+        const { clonedOnHostname, ...requestWithoutCloneHost } = request
+        const result = yield* _(createProjectFromRequest({
+          ...requestWithoutCloneHost,
+          ...(clonedOnHostname === undefined ? {} : { clonedOnHostname })
+        }))
         return yield* _(
           "accepted" in result && result.accepted === true
             ? jsonResponse(result, 202)

packages/api/src/http.ts

@@ -35,7 +35,7 @@ type SnapshotFile = {
   readonly sessions: ReadonlyArray<AgentSession>
 }
 
-const SnapshotFileSchema = Schema.Struct({
+const SnapshotFileSchema: Schema.Schema<SnapshotFile> = Schema.Struct({
   sessions: Schema.Array(AgentSessionSchema)
 })
 
@@ -46,16 +46,43 @@ const projectIndex: Map<string, Set<string>> = new Map()
 const maxLogLines = 5000
 let initialized = false
 
-export const clearAgentRuntimeForTest = (): void => {
-  for (const record of records.values()) {
-    if (record.process !== null && !record.process.killed) {
-      record.process.kill("SIGTERM")
+const waitForProcessExit = (child: ChildProcess): Effect.Effect<void> =>
+  Effect.async((resume) => {
+    const finish = (): void => {
+      clearTimeout(timeout)
+      child.off("exit", finish)
+      child.off("close", finish)
+      resume(Effect.void)
     }
-  }
-  records.clear()
-  projectIndex.clear()
-  initialized = false
-}
+    const timeout = setTimeout(finish, 1_000)
+    if (child.exitCode !== null || child.signalCode !== null) {
+      finish()
+      return
+    }
+    child.once("exit", finish)
+    child.once("close", finish)
+  })
+
+export const clearAgentRuntimeForTest = (): Effect.Effect<void> =>
+  Effect.gen(function*(_) {
+    const children = [...records.values()]
+      .map((record) => record.process)
+      .filter((process): process is ChildProcess => process !== null)
+
+    for (const child of children) {
+      if (!child.killed) {
+        child.kill("SIGTERM")
+      }
+    }
+
+    yield* _(Effect.forEach(children, waitForProcessExit, {
+      concurrency: "unbounded",
+      discard: true
+    }))
+    records.clear()
+    projectIndex.clear()
+    initialized = false
+  })
 
 const nowIso = (): string => new Date().toISOString()
 
@@ -229,7 +256,9 @@ const persistSnapshot = (): Effect.Effect<void, PlatformError, FileSystem.FileSy
 const persistSnapshotBestEffort = (): void => {
   Effect.runFork(persistSnapshot().pipe(
     Effect.provide(NodeContext.layer),
-    Effect.catchAll(() => Effect.void)
+    Effect.catchAll((error) =>
+      Effect.logWarning(`[agents] Failed to persist snapshot: ${String(error)}`)
+    )
   ))
 }
 
@@ -359,19 +388,16 @@ const readSnapshotFile = (): Effect.Effect<SnapshotFile, never, FileSystem.FileS
   Effect.gen(function*(_) {
     const fs = yield* _(FileSystem.FileSystem)
     const filePath = stateFilePath()
-    const exists = yield* _(Effect.either(fs.exists(filePath)))
-    const fileExists = Either.match(exists, {
-      onLeft: () => false,
-      onRight: (value) => value
-    })
+    const fileExists = yield* _(fs.exists(filePath).pipe(
+      Effect.orElse(() => Effect.succeed(false))
+    ))
     if (!fileExists) {
       return emptySnapshotFile()
     }
-    const contents = yield* _(Effect.either(fs.readFileString(filePath)))
-    return Either.match(contents, {
-      onLeft: () => emptySnapshotFile(),
-      onRight: (value) => decodeSnapshotFile(value) ?? emptySnapshotFile()
-    })
+    const contents = yield* _(fs.readFileString(filePath).pipe(
+      Effect.orElse(() => Effect.succeed(""))
+    ))
+    return decodeSnapshotFile(contents) ?? emptySnapshotFile()
   }).pipe(Effect.catchAll(() => Effect.succeed(emptySnapshotFile())))
 
 const hydrateFromSnapshot = (): Effect.Effect<void, never, FileSystem.FileSystem> =>
@@ -386,6 +412,8 @@ const hydrateFromSnapshot = (): Effect.Effect<void, never, FileSystem.FileSystem
         updatedAt: nowIso()
       }
 
+      // Hydrated sessions are restart metadata only; projectDir is available
+      // only when new operations receive a live ProjectDetails argument.
       const record: AgentRecord = {
         session: restored,
         projectDir: "",

packages/api/src/services/agents.ts

@@ -160,7 +160,7 @@ const withProjectRuntime = (
       sshSessions: runtime.sshSessions,
       startedAtIso: runtime.startedAtIso,
       startedAtEpochMs: runtime.startedAtEpochMs,
-      clonedOnHostname: project.clonedOnHostname
+      ...(project.clonedOnHostname === undefined ? {} : { clonedOnHostname: project.clonedOnHostname })
     }))
   )
 
@@ -191,7 +191,7 @@ const dbProjectSummary = (
   sshSessions: 0,
   startedAtIso: project.lastStartedAtIso,
   startedAtEpochMs: project.lastStartedAtEpochMs,
-  clonedOnHostname: project.clonedOnHostname
+  ...(project.clonedOnHostname === undefined ? {} : { clonedOnHostname: project.clonedOnHostname })
 })
 
 const toProjectDetails = (