chore(hub,sidecar,tools,workflows): comments say why, never what; long ones move to docs/ (CL-8377)

apps/hub/src/mailbox-persist.ts

@@ -1,11 +1,6 @@
-// Wires `@corbits/mailbox`'s `createMailboxPersist` onto the hub's own
-// `persistMail` lookup (`vendor/intx/hub-sessions/src/hub-session-lookups.ts`)
-// so every outbound agent frame also lands a durable `principal_mail` row in
-// each addressed human participant's mailbox, not just `session_mail`.
-//
-// One seam lives here, host-owned by the package's own contract
-// (`persist.ts` in `@corbits/mailbox`): `authorizeSender` decides which
-// sender addresses may write at all.
+// Wires @corbits/mailbox's createMailboxPersist onto the hub's own
+// persistMail lookup so every outbound frame also lands a durable
+// principal_mail row, not just session_mail.
 
 import { eq } from "drizzle-orm";
 import type { DB } from "@intx/db";
@@ -19,46 +14,10 @@ import { getLogger } from "@intx/log";
 const logger = getLogger(["hub", "mailbox-persist"]);
 
 /**
- * Resolve a `mail.outbound` frame's sender run address to the mailbox
- * authorization the package needs: the tenant the resulting rows belong to,
- * and the mail domain that scopes which recipients are even addressable.
- *
- * Mirrors exactly what `persistMail` itself already does to resolve a sender
- * (`resolveRoutableAddress`, the same routable-address resolver) rather than
- * re-deriving liveness some other way: a sender that is not a live run
- * resolves to `undefined` there and to `null` here, which skips the mailbox
- * write while the frame still goes upstream unchanged.
- *
- * This is a genuine double resolve of the same address across one frame --
- * `baseLookups.persistMail` (vendor-owned, `hub-session-lookups.ts`) resolves
- * `senderAddress` for its own `session_mail` write, and this seam resolves it
- * again for mailbox authorization. `createMailboxPersist` calls `upstream`
- * and `authorizeSender` as two independent stages and hands neither's result
- * to the other, so there is no seam to thread one resolution through without
- * changing the vendor-owned `persistMail` signature itself, which the
- * ground rules rule out. Accepted as the cost of two owners agreeing on one
- * fact from two directions: one extra indexed lookup by address per frame,
- * not per recipient.
- */
-/**
- * Wraps the hub's own `persistMail` (vendor's `baseLookups.persistMail`)
- * so a run's `agent_session` and event collector exist before that write
- * runs: the first inbound mail on a freshly triggered run is often the
- * earliest point the run is mail-routable at all, since
- * `workflow_run.principal_id` only reconciles onto the trigger that just
- * fired. Resolves the frame's own sender address to its run
- * (`resolveRoutableAddress`, the same resolver `persistMail` itself
- * uses) rather than assuming the caller already knows it; a sender that
- * is not a live run resolves to `undefined` and this is a no-op past
- * that point. Best-effort: `ensureRunSession` failing must not swallow
- * the mail upstream is about to persist regardless, so this only
- * degrades to "no session yet" rather than ever throwing past itself.
- *
- * `eventCollectorsRef` is a forward reference (the same pattern this
- * file's other composition-order refs use, e.g. `apps/hub/src/index.ts`'s
- * `grantAllowanceGateRef`): the wrapped `eventCollectors` registry isn't
- * constructed until after `lookups` is, so this only reads `.current`
- * inside the returned closure, never at wiring time.
+ * Ensures a run's agent_session/event collector exist before the first
+ * mail-triggered write reaches it, since workflow_run.principal_id only
+ * reconciles onto the trigger that just fired. Best-effort: a failure
+ * here must not block the mail upstream is about to persist regardless.
  */
 export function createHubPersistMailWithSessionEnsure<R extends readonly unknown[]>(
   db: DB["db"],
@@ -78,22 +37,15 @@ export function createHubPersistMailWithSessionEnsure<R extends readonly unknown
         extra: { senderAddress: args.senderAddress },
       });
     }
-    // The vendored `session_mail` ledger (`upstream`, `baseLookups.persistMail`)
-    // keys on the sender run's principal, which does not exist until the
-    // run's first trigger anchors one — a run's own first outbound mail can
-    // reach this wrapper before that happens. `@corbits/mailbox`'s own write
-    // (this function's caller, `createMailboxPersist`) is keyed on the run
-    // address instead and runs regardless, so it is already the durable
-    // record for this frame; skip the vendored delegate rather than let it
-    // throw "no session for address" past the caller.
+    // The vendored session_mail write keys on the sender's principal, which
+    // doesn't exist yet on a run's first outbound mail; skip it rather than
+    // let it throw, since @corbits/mailbox's own write already persists this
+    // frame.
     if (sender !== undefined && sender.sessionId === null) {
       logger.debug("skipping vendored persistMail for run {runId}: no session yet", {
         runId: sender.id,
         senderAddress: args.senderAddress,
       });
-      // No `session_mail` rows were written for this frame: an empty
-      // result, not a thrown error, so a caller iterating this like
-      // `handleMailPersist` does simply emits no `mail.persisted` events.
       return [] as unknown as R;
     }
     return upstream(args);

apps/hub/src/provisioners/process.ts

@@ -67,13 +67,7 @@ export type ReadProcessProvisionerConfigArgs = {
   readonly hubWebSocketUrl: string;
 };
 
-/**
- * Parse the process provisioner's configuration. Both environment keys
- * are optional: an unconfigured install spawns this repository's own
- * `apps/sidecar` entry with the running `bun`, which is what makes
- * "no provisioner configured" work on a single server with no operator
- * setup at all.
- */
+/** Both env keys are optional: unconfigured, this spawns the repo's own apps/sidecar with the running bun. */
 export function readProcessProvisionerConfig(
   args: ReadProcessProvisionerConfigArgs,
 ): ProcessProvisionerConfig {
@@ -104,25 +98,17 @@ export type SpawnSidecarProcessArgs = {
   readonly env: Record<string, string>;
 };
 
-/**
- * The narrow OS port this backend needs: start a detached child, ask
- * whether a pid is still alive, and signal it. Injected so the unit
- * suite exercises the full ensure/destroy lifecycle — including
- * generation fencing and directory cleanup — without starting a real
- * sidecar.
- */
+/** Injected so tests exercise the full ensure/destroy lifecycle without starting a real sidecar. */
 export interface SidecarProcessRunner {
   spawn(args: SpawnSidecarProcessArgs): number;
   isAlive(pid: number): boolean;
   signal(pid: number, signal: "SIGTERM" | "SIGKILL"): void;
 }
 
 /**
- * Bun implementation. The child is spawned with its own stdio inherited
- * from the hub so a local operator sees sidecar logs in the same
- * terminal, and `unref`ed so a hub shutdown is not blocked waiting on
- * it — a still-running sidecar is reconciled (or destroyed) on the next
- * boot from its pid file.
+ * Stdio inherited so a local operator sees sidecar logs in the same
+ * terminal; unref()ed so a hub shutdown doesn't wait on it — a still-running
+ * sidecar is reconciled from its pid file on the next boot.
  */
 export function createBunSidecarProcessRunner(): SidecarProcessRunner {
   return {
@@ -169,24 +155,11 @@ const TERMINATION_GRACE_MS = 5_000;
 const TERMINATION_POLL_MS = 100;
 
 /**
- * Implements `SidecarBackend` (see `./sandbox-sidecar`) by running one
- * `apps/sidecar` process per allocation on the hub host itself, the only
- * sidecar backend, so one server hosts many chats and workflows with no
- * container runtime and no remote sandbox account.
- *
- * Layout under the hub's data dir, one directory per started unit:
- *
- *     <allocationsDir>/<allocationId>/gen-<generation>/sidecar.pid
- *     <allocationsDir>/<allocationId>/gen-<generation>/data/
- *
- * `data/` is that sidecar's own `SIDECAR_DATA_DIR`. The pid file is what
- * makes this backend survive a hub restart: the child handle is gone, but
- * `findUnitsByAllocation` still finds every live unit from disk, so the
- * core can sweep a superseded generation and destroy can still clean up
- * after a unit this process never started. Scoping a unit by generation
- * (rather than one pid file per allocation) is what keeps that sweep
- * honest — a new generation's pid never overwrites the record of the one
- * it is replacing.
+ * One apps/sidecar process per allocation on the hub host — the only
+ * backend. Scoping each unit's directory by generation (not one pid file
+ * per allocation) is what lets this backend survive a hub restart:
+ * findUnitsByAllocation recovers every live unit from disk by pid file,
+ * and a new generation's pid never overwrites the one it supersedes.
  */
 export function createProcessBackend(
   runner: SidecarProcessRunner,
@@ -319,13 +292,7 @@ export function createProcessBackend(
   };
 }
 
-/**
- * The sidecar's own boot config (`apps/sidecar/src/config.ts`) requires
- * `SIDECAR_DATA_DIR`, `HUB_WS_URL`, `SIDECAR_ID`, `SIDECAR_TOKEN` and
- * `PATH`, and forwards `HOME`/`TMPDIR` into each workflow-process child.
- * Nothing else of the hub's environment is inherited: a sidecar learns
- * everything else over the wire.
- */
+/** A sidecar learns everything else over the wire; nothing else of the hub's env is inherited. */
 function sidecarEnvFor(args: StartUnitArgs, sidecarDataDir: string): Record<string, string> {
   const path = process.env["PATH"];
   if (path === undefined || path === "") {
@@ -453,11 +420,10 @@ const PROVISIONER_API_VERSION = 1 as const;
 export const PROCESS_PROVISIONER_ID = "process";
 
 /**
- * Which allocations this instance owns. Interchange adopts a probe's
- * allocation for the deployment when the two provisioners share id, api
- * version and binding fingerprint; that adopt path does not deploy the
- * workflow after the sidecar reconnects at the current pin, so the hub
- * runs a separate probe instance whose fingerprint never matches.
+ * Interchange adopts a probe's allocation for the deployment when both
+ * provisioners share id/apiVersion/fingerprint, which skips redeploying at
+ * the current pin — so the hub runs a probe instance whose fingerprint
+ * never matches.
  */
 export type ProcessProvisionerRole = "deployment" | "probe";
 
@@ -469,15 +435,9 @@ export type CreateProcessSidecarProvisionerOpts = {
 };
 
 /**
- * The only sidecar backend: every allocation is a child process of the
- * hub on the same host. Idempotence, generation fencing, and destroy
- * tombstones come from `./sandbox-sidecar`'s shared core — this module
- * supplies only the OS-level unit.
- *
- * The binding fingerprint pins the two facts that decide what a
- * provisioned sidecar actually is: which entry point runs, and which hub
- * it dials. A change to either is a different backend binding, so
- * allocations bound to the old one are not silently treated as current.
+ * The binding fingerprint pins entry point and hub URL: a change to either
+ * is a different backend binding, so allocations bound to the old one
+ * aren't silently treated as current.
  */
 export function createProcessSidecarProvisioner(
   opts: CreateProcessSidecarProvisionerOpts,

apps/hub/src/provisioners/sandbox-sidecar.ts

@@ -1,8 +1,5 @@
-// Backend-agnostic sidecar allocation lifecycle: state store and
-// ensure/destroy convergence for a SidecarProvisioner. A backend supplies
-// only the SidecarBackend port (start/stop/find) and identity
-// (id/apiVersion/bindingFingerprint); this module owns everything else a
-// SidecarProvisioner needs to behave correctly under concurrent and
+// Backend-agnostic lifecycle: a backend supplies only start/stop/find and
+// identity; this module owns correctness under concurrent and
 // out-of-order ensure()/destroy() calls.
 import { randomBytes } from "node:crypto";
 import { mkdir, open, readFile, rename, unlink } from "node:fs/promises";
@@ -39,13 +36,7 @@ export interface SidecarBackend {
   findUnitsByAllocation(allocationId: string): Promise<readonly string[]>;
 }
 
-/**
- * A backend throws this from startUnit()/stopUnit() to report a
- * classified failure — its own error code and whether it is worth
- * retrying — rather than a generic Error. The core forwards code/message/
- * retryable straight into the rejected EnsureSidecarResult/
- * DestroySidecarResult.
- */
+/** Lets a backend report a classified, retryable-or-not failure instead of a generic Error. */
 export class BackendOperationError extends Error {
   readonly code: string;
   readonly retryable: boolean;
@@ -59,12 +50,10 @@ export class BackendOperationError extends Error {
 }
 
 /**
- * The isolation a sidecar backend gives the code it runs, as a ladder: a
- * container also isolates the process, and a VM also isolates the
- * container. A backend declares every rung it reaches as `available` and
- * every rung above it as `blocked`, so a deployment that requires
- * `isolation:vm` fails closed on the process backend instead of quietly
- * landing on a shared kernel.
+ * A ladder: a container also isolates the process, a VM also isolates the
+ * container. A backend declares every rung it reaches as available so a
+ * deployment requiring isolation:vm fails closed rather than landing quietly
+ * on a shared kernel.
  */
 export const SIDECAR_ISOLATION_LEVELS = ["process", "container", "vm"] as const;
 
@@ -372,10 +361,8 @@ export type CreateSidecarProvisionerOpts = {
   readonly apiVersion: 1;
   readonly bindingFingerprint: string;
   /**
-   * Capabilities this backend declares to the hub's capability policy.
-   * Empty declares nothing, which matches any deployment that states no
-   * capability requirement -- the behaviour every allocation had before
-   * Interchange replaced the placement model with capability selection.
+   * Empty matches any deployment stating no requirement — the behaviour
+   * every allocation had before capability selection replaced placement.
    */
   readonly capabilities: readonly SidecarCapabilityDeclaration[];
   readonly backend: SidecarBackend;
@@ -441,13 +428,9 @@ export function createSidecarProvisioner(opts: CreateSidecarProvisionerOpts): Si
         false,
       );
     }
-    // Removes any other unit still labeled for this allocation: an older
-    // generation's unit this ensure() is superseding, a duplicate left
-    // behind by a concurrent ensure() that lost the race to record its
-    // unit, or a unit from a prior process that crashed between starting
-    // the unit and recordUnit(). All three look identical from here — "a
-    // unit for this allocation that isn't the one we just recorded" — so
-    // one sweep by allocation covers all of them.
+    // Covers three cases indistinguishable from here (superseded generation,
+    // a losing concurrent ensure(), a crash between startUnit and
+    // recordUnit) with one sweep by allocation.
     await sweepObsoleteUnits(backend, request.allocationId, externalRef);
     return { kind: "accepted", externalRef };
   }
@@ -513,13 +496,7 @@ export function createSidecarProvisioner(opts: CreateSidecarProvisionerOpts): Si
   };
 }
 
-/**
- * Stops every unit labeled for this allocation other than the one just
- * confirmed current, best-effort: a sweep failure is logged to stderr
- * rather than failing the ensure() it runs alongside, since the
- * allocation itself is already correctly ensured by this point and an
- * orphaned unit is a leak, not a correctness bug.
- */
+/** Best-effort: a sweep failure only leaks a unit, so it's logged rather than failing ensure(). */
 async function sweepObsoleteUnits(
   backend: SidecarBackend,
   allocationId: string,

apps/hub/src/server.ts

@@ -48,13 +48,8 @@ import { upgradeWebSocket, websocket } from "hono/bun";
 import { setup, getLogger } from "@intx/log";
 import { Hono } from "hono";
 
-// ---------------------------------------------------------------------
-// Everything above this line is upstream Interchange's own
-// apps/hub/src/server.ts, verbatim. Corbits libraries are mounted in one
-// delimited block below, after createApp(...) and before the function's
-// return -- see AGENTS.md's "Workbench is a plain Interchange tenant"
-// ruling. Nothing else in this file is Workbench-specific.
-// ---------------------------------------------------------------------
+// Everything above this line is upstream Interchange's server.ts, verbatim;
+// see AGENTS.md's "plain Interchange tenant" ruling.
 import {
   buildMailFrame,
   createInMemoryMailboxEventBus,
@@ -464,14 +459,8 @@ export async function createHubServer({
     }),
   });
 
-  // ---------------------------------------------------------------------
   // Corbits mount block -- everything Workbench adds to the stock hub.
-  // Each library gets its own Hono<TenantEnv> routed under the tenant
-  // prefix (or "/" when the library's own routes already carry that
-  // prefix), reusing the app's own db/grantStore/credentialCipher/router.
-  // See AGENTS.md: "any other hub mount is cutover debt with a Linear
-  // issue, never a pattern to extend."
-  // ---------------------------------------------------------------------
+  // See AGENTS.md: any other hub mount is cutover debt, never a pattern to extend.
   const TENANT_PREFIX = "/api/tenants/:tenantId";
   const corbitsDatabaseUrl = `postgres://${encodeURIComponent(process.env["DB_USER"] ?? "postgres")}:${encodeURIComponent(process.env["DB_PASSWORD"] ?? "postgres")}@${process.env["DB_HOST"] ?? "localhost"}:${String(Number(process.env["DB_PORT"] ?? 5432))}/${process.env["DB_NAME"] ?? "interchange"}`;
   const { db: mailboxDb } = createMailboxDb(corbitsDatabaseUrl);
@@ -639,9 +628,7 @@ export async function createHubServer({
     router: webhookMailRouter,
   });
 
-  // ---------------------------------------------------------------------
   // End of Corbits mount block.
-  // ---------------------------------------------------------------------
 
   log.info("Starting server on port {port}", { port });