From c26311bb6afaef4d6749ca7c3fb97ea0cbfed895 Mon Sep 17 00:00:00 2001 From: schickling-assistant <261620128+schickling-assistant@users.noreply.github.com> Date: Tue, 21 Jul 2026 11:52:42 +0200 Subject: [PATCH 1/2] `convoy run` declares, launches, and attaches (supersedes #92) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `convoy run` shipped in #92 as an AD-HOC session: deliberately not declared, not reconciled, not respawned, no durable context. This replaces that with the intended semantics — `run` is exactly `convoy add`, plus a launch and an attach. The verb set becomes coherent: add declare (no launch) render materialize the overlay (no launch, no bus) up reconcile — launch what is declared (no attach) run declare, launch, and attach The deeper reason: an undeclared session has no stable identity, therefore no durable context/ directory, therefore it cannot externalize state or converge after a restart. That is a permanent second class of session. Making `run` declare means every agent gets the same guarantees and the ad-hoc/declared distinction disappears rather than being managed forever. `add` and `run` now share `buildDeclaration`, so the two verbs cannot drift. Identity is REQUIRED, as for `add`. #92 generated one (`run-b3ur0v`) when it was omitted; that is precisely the hashed-name problem that makes durable context unreachable — nobody re-derives a random name on the next cold boot. Attach/detach: detaching leaves the agent running (a pty session outlives its client), and because it is declared, `convoy up` reconciles and respawns it. `--no-attach` covers non-interactive callers. Already-exists is a decision table, NOT `add`'s flat refuse-without-force: not declared → declare + launch + attach declared, not live → RESUME from the existing declaration (the point of declaring; refusing would break it) declared, live → attach to the running session, never restart declared, not live, --force → re-declare with these flags, then launch declared, live, --force → refuse; `convoy reload` is the kill+respawn verb Removed the ad-hoc machinery rather than leaving two models: generateAdHocIdentity, adHocNotice, isAdHocIdentity, AD_HOC_PREFIX, and validateRunIdentity (whose premise inverted — collision-with-declared is now the resume case). Help text in cli.ts and the command table no longer advertise the old semantics. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_014ePNMmLYa7qVT3h7bRCWUJ agent-session-id: 0abcedc7-6b71-4046-9e7c-f645268c0b15 agent-tool: Claude Code agent-tool-version: 2.1.215 agent-model: claude-opus-4-8 agent-runtime-profile: /nix/store/acr8a3l2v366jgmwiq8xdrhgz1py0db5-coding-agent-runtime-profile/share/coding-agents/profile.json agent-skills-manifest: /nix/store/sj1v5j91h8v8d1w9lca4040302lwrd6v-agent-skills-corpus/share/agent-skills/manifest.json tooling-profile: dotfiles@unknown-dirty --- src/cli.ts | 2 +- src/command-table.ts | 27 +++-- src/commands.ts | 272 ++++++++++++++++++++++++----------------- src/exec.ts | 15 ++- src/run.test.ts | 282 +++++++++++++++++++++++++++++-------------- src/run.ts | 201 ++++++++++++++++-------------- 6 files changed, 487 insertions(+), 312 deletions(-) diff --git a/src/cli.ts b/src/cli.ts index 9dd5bc1..30f4ba1 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -141,7 +141,7 @@ function printHelp(): void { " add DECLARE an agent — write its agent file into the synced catalog; NO launch (convoy up runs it) [--identity --host --harness claude|codex --model --transport ding|mcp --mcp --network --dir --persona --permanent --dry-run --force]\n" + " render materialize an agent's worktree overlay from its catalog agent file — NO launch, NO bus (declarative: add=declare · render=materialize · up=reconcile) [--dir --network --dry-run]\n" + " cos --repo bootstrap a Chief of Staff\n" + - " run [role] launch an AD-HOC session — NOT declared, NOT reconciled, NOT respawned, no durable context (declare it with `add` if it should survive) [--identity --harness claude|codex --model --transport ding|mcp --mcp --network --dir --persona --prefix --config-dir --dry-run --force]\n" + + " run [role] DECLARE an agent, launch it, and ATTACH — `add` + `up` in one step (detaching leaves it running; re-run to resume it) [--identity --host --harness claude|codex --model --transport ding|mcp --mcp --network --dir --bin --supervisor --persona --permanent --no-attach --dry-run --force]\n" + " up host a network in the foreground (TCC anchor + supervisor + flapping-cap) [--once = one-shot reconcile-and-exit (adopts live sessions, no daemon) --json]\n" + " down [network] tear down the network — the ONLY path that kills sessions [--dry-run --force --json]\n" + " env [network] print eval-safe exports for a network's env — `eval \"$(convoy env )\"` sets ST_ROOT+PTY_ROOT [--identity ]\n" + diff --git a/src/command-table.ts b/src/command-table.ts index a983674..909ad6d 100644 --- a/src/command-table.ts +++ b/src/command-table.ts @@ -153,27 +153,28 @@ export const COMMANDS: readonly CommandSpec[] = [ }, { name: "run", - // No `--permanent`: a permanent ad-hoc session is a contradiction — nothing declares it, so nothing - // can bring it back. `cmdRun` pins permanentOverride=false so a permanent-by-default role can't - // smuggle respawn semantics in either. - desc: "Launch an ad-hoc session — not declared, not reconciled, not respawned", + // `run` is `add` + launch + attach, so its flag set IS `add`'s (they share `buildDeclaration`), plus + // `--no-attach` for non-interactive callers. `--permanent` is here now: a run-created agent is a real + // catalog member, so "always-on" is a coherent thing to ask of it. `--prefix` and `--config-dir` are + // gone — the declaration carries the host prefix (`--host`) and the harness config dir (via `env`), and + // a per-invocation override of either would desync the session from the agent file that describes it. + desc: "Declare an agent, launch it, and attach (add + up, in one step)", flags: [ IDENTITY_FLAG, + { name: "host", desc: "Owning host (default: this machine)", kind: "value" }, HARNESS_FLAG, - { name: "model", desc: "Per-agent model id", kind: "value" }, + { name: "model", desc: "Model id passed to the harness", kind: "value" }, TRANSPORT_FLAG, { name: "mcp", desc: "Shorthand for --transport mcp", kind: "bool" }, NETWORK_FLAG, - { name: "dir", desc: "Workspace dir (default: cwd)", kind: "value", takesPath: true }, + { name: "dir", desc: "Workspace dir", kind: "value", takesPath: true }, + { name: "bin", desc: "Harness executable (path or command name)", kind: "value", takesPath: true }, + { name: "supervisor", desc: "Identity this agent reports to", kind: "value" }, PERSONA_FLAG, - { name: "prefix", desc: "Session-id prefix (default: short hostname)", kind: "value" }, - CONFIG_DIR_FLAG, - // `--bin` on the ad-hoc path too: a deployment that wraps its harness wraps it for ad-hoc sessions - // as well, and `convoy run` is the replacement for exactly those launcher aliases. Without it, the - // ad-hoc path is the one place a convoy session escapes the deployment's boundary. - { name: "bin", desc: "Run this in place of the bare harness binary", kind: "value", takesPath: true }, + { name: "permanent", desc: "Always-on member", kind: "bool" }, + { name: "no-attach", desc: "Declare and launch, but do not attach", kind: "bool" }, DRY_RUN_FLAG, - { name: "force", desc: "Overwrite a foreign pty.toml in the workspace", kind: "bool" }, + { name: "force", desc: "Re-declare an existing (stopped) agent with these flags", kind: "bool" }, ], positional: { desc: "Role (default: worker)", values: ROLE_SPELLINGS }, }, diff --git a/src/commands.ts b/src/commands.ts index f70f2e8..df5f6e7 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -7,14 +7,15 @@ import { existsSync, mkdirSync, readFileSync, statSync, writeFileSync } from "no import { homedir, hostname } from "node:os"; import { basename, delimiter, dirname, join, resolve } from "node:path"; import { fileURLToPath } from "node:url"; -import { run } from "./exec.ts"; +import { execInteractive, run } from "./exec.ts"; import { flagAllowList } from "./command-table.ts"; import { CONVOY_DIR, DEFAULT_NETWORK_NAME, defaultConvoyNetwork, isNetworkName, networkDirForName, networkDirOfStRoot, networkLayout, stRootOf } from "./paths.ts"; import { DING_SERVICES, isDingService, networkNameFromDir, readNetworkConfig, writeNetworkConfig, type DingService } from "./network-config.ts"; import { defaultBinDir, installClis } from "./install-cli.ts"; import { Bus, isLive, type Agent } from "./bus.ts"; -import { PtyHost, spawnFromPtyFile, workspaceOfPtyfile, type SupervisedSession } from "./host.ts"; +import { PtyHost, gone, processAlive, spawnFromPtyFile, workspaceOfPtyfile, type SupervisedSession } from "./host.ts"; import { busIdOf } from "./up.ts"; +import { agentBusId } from "./reconcile.ts"; import { discoverSmalltalkDir, nativeLaunch, regenerateDingRoot, writeAgentFiles } from "./launch.ts"; import { agentFilePath, agentFileToSpec, agentFileToToml, catalogDir, isValidBin, readAgentFile, writeAgentFile, SAMPLE_AGENT_TOML, type AgentFile } from "./agent-file.ts"; import { readCatalog } from "./catalog.ts"; @@ -29,8 +30,8 @@ import { runFullOrgSuite, runReadinessSuite } from "./doctor/suite.ts"; import { structureChecks } from "./doctor/structure.ts"; import { baseFile, ensureInstalled, personasDir, personasInstalled } from "./personas.ts"; import { ROLES, parseRole } from "./role.ts"; -import { adHocNotice, generateAdHocIdentity, validateRunIdentity } from "./run.ts"; -import { busAgentId, isValidModel, preflight, resolvedPersonaPath, shortHostname, type AgentSpec, type Transport } from "./agent-spec.ts"; +import { declaredRunNotice, liveForceRefusal, passedDeclarationFlags, resolveRunAction, staleFlagsNote } from "./run.ts"; +import { busAgentId, isValidModel, preflight, resolvedPersonaPath, sessionId, shortHostname, type AgentSpec, type Transport } from "./agent-spec.ts"; import { HARNESSES, HARNESS_SESSION_KEYS, HARNESS_SUFFIX_RE, harnessDescriptor, harnessesInPtyToml, harnessLimitations, isHarness, type Harness } from "./harness.ts"; import { claudeConfigPath, codexConfigPath, pretrustDirs, pretrustDirsCodex } from "./trust.ts"; import { DEFAULT_JOB_ID, completionPath, isValidJobId, readCompletion, writeCompletion, type JobStatus } from "./job.ts"; @@ -700,15 +701,27 @@ export async function cutWorktree(megarepo: string, path: string, identity: stri return { ok: true, branch }; } -export async function cmdAdd(args: string[]): Promise { - const bad = unknownFlag(args, ...flagAllowList("add")); - if (bad) { - err(`unrecognized flag "${bad}" for \`convoy add\` — refusing rather than silently ignoring it. See \`convoy add --help\`.`); - return 2; - } +/** A built-but-NOT-yet-written declaration, plus the context the callers need to report it. */ +interface Declaration { + readonly af: AgentFile; + readonly path: string; + readonly existed: boolean; + readonly network: string; + /** True when the workspace came from an explicit --dir (vs. the derived megarepo worktree path). */ + readonly dirGiven: boolean; + readonly megarepo: string | null; +} + +/** Build + validate the declaration for `convoy add` and `convoy run` from the SAME flags. + * + * Extracted so the two verbs cannot drift: `run` is `add` plus a launch and an attach, and the only honest + * way to promise that is for both to compile their agent file through one function. Writes NOTHING — the + * caller decides whether and when to persist it (add gates on --force; run's decision table is richer). + * Returns a numeric exit code on failure, having already reported the error. */ +async function buildDeclaration(args: string[], verb: "add" | "run"): Promise { const roleRaw = positionals(args)[0]; if (!roleRaw) { - err("missing role. Usage: convoy add --identity "); + err(`missing role. Usage: convoy ${verb} --identity `); return 2; } const role = parseRole(roleRaw); @@ -716,9 +729,17 @@ export async function cmdAdd(args: string[]): Promise { err(`unknown role "${roleRaw}". Valid: ${ROLES.join(", ")}`); return 2; } + // --identity is REQUIRED for `run`, exactly as for `add`. #92 generated one (`run-b3ur0v`) when it was + // omitted; that is precisely the hashed-name problem that makes durable context unreachable — nobody + // re-derives a random name on the next cold boot, so the agent can never find its own context/ again. + // A meaningful name is the cheapest thing a caller can supply and the one thing continuity depends on. const identity = optValue(args, "--identity"); if (!identity) { - err("--identity is required"); + err( + verb === "run" + ? "--identity is required. `convoy run` DECLARES the agent, and a declared agent needs a name you can re-derive — that is what lets its context/ survive a restart. Pick something meaningful: `convoy run worker --identity fodfix`." + : "--identity is required", + ); return 2; } const harnessRaw = (optValue(args, "--harness") ?? "claude").toLowerCase(); @@ -743,37 +764,40 @@ export async function cmdAdd(args: string[]): Promise { } const network = resolveNetworkRoot(optValue(args, "--network")); - // VALIDATE THE IDENTITY HERE — `add` is the DECLARE path, and it used to check only truthiness, so a + // VALIDATE THE IDENTITY HERE — this is the DECLARE path, and it used to check only truthiness, so a // name the bus rejects (`worker_fodfix`) was written into the SYNCED catalog and propagated to every // peer machine before anything noticed. Validating at launch is too late by several machines. The // network is resolved above, so the check gets its real context: the bus grammar, the socket budget // for THIS network's path, and the identities already declared on it. const addHost = (optValue(args, "--host") ?? shortHostname()).toLowerCase(); - const declared = readCatalog(network).entries.map((e) => e.af.identity); + const declaredIds = readCatalog(network).entries.map((e) => e.af.identity); const idErrors = identityErrors(identity, { ptyRoot: networkLayout(network).ptyRoot, prefix: addHost, - // A re-declare of the SAME name is an overwrite, gated by --force below, not a uniqueness failure. - existing: declared.filter((d) => d !== identity), + // A re-declare of the SAME name is an overwrite (add: gated by --force; run: the resume path), not a + // uniqueness failure. + existing: declaredIds.filter((d) => d !== identity), }); if (idErrors.length > 0) { for (const e of idErrors) err(e); return 2; } - // DECLARE-ONLY (Nathan's piece-2 call): `convoy add` writes the agent file (declarative intent) into the - // SYNCED catalog and does NOTHING else — no render, no launch, no bus folder, no pretrust, no persona-clone, - // no worktree cut. The catalog is desired state; `convoy up` renders-if-needed + launches this host's agents - // on the way (piece 3), and `convoy render ` materializes the overlay standalone. Declaring != running. + const path = agentFilePath(catalogDir(network), identity); + const existed = existsSync(path); + + // workspace: --dir wins; else, with a megarepo configured, the intended per-agent worktree + // path /worktrees/ (materialized at launch — reuses #59). No --dir + no megarepo → no workspace + // (agents need one) → refuse with a clear fix. // - // workspace (accepts both forms Nathan signed off): --dir wins; else, with a megarepo configured, - // the intended per-agent worktree path /worktrees/ (materialize cuts it off the megarepo — reuses - // #59, at materialize-time now that add is declare-only). No --dir + no megarepo → no workspace (agents need - // one) → refuse with a clear fix. + // EXCEPT when `run` is resuming an agent that is ALREADY declared: the existing agent file already carries + // its workspace, and demanding `--dir` again would make the resume path — the everyday path, the one this + // whole design exists to deliver — impossible to invoke without repeating what the catalog already knows. + // `add` keeps the hard requirement: it only ever authors a declaration, so it has nothing to fall back on. const dir = optValue(args, "--dir"); const cfg = readNetworkConfig(network); const workspace = dir ? resolve(expandTilde(dir)) : cfg?.megarepo ? join(networkLayout(network).worktrees, identity) : null; - if (!workspace) { + if (!workspace && !(verb === "run" && existed)) { err(`no workspace for "${identity}": pass --dir , or configure a megarepo (\`convoy init --megarepo \`) so a worktree is cut for it at launch.`); return 1; } @@ -784,7 +808,7 @@ export async function cmdAdd(args: string[]): Promise { identity, role, host: addHost, - workspace, + ...(workspace ? { workspace } : {}), harness: harnessRaw as Harness, transport, ...(supervisor ? { supervisor } : {}), @@ -794,8 +818,25 @@ export async function cmdAdd(args: string[]): Promise { ...(hasFlag(args, "--permanent") ? { strategy: "permanent" as const } : {}), }; - const path = agentFilePath(catalogDir(network), identity); - const existed = existsSync(path); + return { af, path, existed, network, dirGiven: dir !== null, megarepo: cfg?.megarepo ?? null }; +} + +export async function cmdAdd(args: string[]): Promise { + const bad = unknownFlag(args, ...flagAllowList("add")); + if (bad) { + err(`unrecognized flag "${bad}" for \`convoy add\` — refusing rather than silently ignoring it. See \`convoy add --help\`.`); + return 2; + } + const built = await buildDeclaration(args, "add"); + if (typeof built === "number") return built; + const { af, path, existed, dirGiven, megarepo } = built; + const identity = af.identity; + + // DECLARE-ONLY: `convoy add` writes the agent file (declarative intent) into the SYNCED catalog and does + // NOTHING else — no render, no launch, no bus folder, no pretrust, no persona-clone, no worktree cut. The + // catalog is desired state; `convoy up` renders-if-needed + launches this host's agents, `convoy render + // ` materializes the overlay standalone, and `convoy run` is this same declaration plus a launch and + // an attach. Declaring != running. if (existed && !hasFlag(args, "--force")) { err(`agent "${identity}" is already declared at ${path}. The catalog SYNCS across machines — overwriting could disrupt a running agent. Re-run with --force to replace it.`); return 1; @@ -807,9 +848,9 @@ export async function cmdAdd(args: string[]): Promise { return 0; } writeAgentFile(path, af); - out(`✓ declared "${identity}" (${role}, host ${af.host})${existed ? " — REPLACED" : ""} → ${path}`); - out(` workspace: ${workspace}${!dir && cfg?.megarepo ? ` (a worktree off megarepo ${cfg.megarepo}, cut at launch)` : ""}`); - out(` NOTHING launched — the catalog is desired state. Run \`convoy up\` to reconcile + launch this host's agents (or \`convoy render ${identity}\` to materialize the overlay now).`); + out(`✓ declared "${identity}" (${af.role}, host ${af.host})${existed ? " — REPLACED" : ""} → ${path}`); + out(` workspace: ${af.workspace}${!dirGiven && megarepo ? ` (a worktree off megarepo ${megarepo}, cut at launch)` : ""}`); + out(` NOTHING launched — the catalog is desired state. Run \`convoy up\` to reconcile + launch this host's agents (or \`convoy render ${identity}\` to materialize the overlay now), or \`convoy run\` to declare+launch+attach in one step.`); return 0; } @@ -909,16 +950,19 @@ export async function cmdCos(args: string[]): Promise { return rc; } -/** `convoy run` — launch an AD-HOC session: the runnable core with NO declaration on top. +/** `convoy run [role] --identity ` — DECLARE, launch, and attach. + * + * `run` is `convoy add` plus a launch and an attach: the same flags, the same validation, the same catalog + * write (they share `buildDeclaration`, so they cannot drift), and then it starts the agent and hands the + * caller the terminal. This SUPERSEDES the ad-hoc session that shipped in #92 — see the header of run.ts + * for why the declared model is right and why #92's three objections do not survive. * - * Structurally this is `cmdCos`'s shape (build an AgentSpec, hand it to launchSpec) generalized past the - * hardcoded chief-of-staff, and it writes NO catalog entry for the same reason `cmdCos` doesn't. The - * difference from every other launch is what it REFUSES to promise; see run.ts for the full contract - * delta and why an "ephemeral declaration" was rejected. + * Everything an agent needs to converge — a stable identity, a durable `context/`, reconcile and respawn + * under `convoy up` — follows from being DECLARED. So `run` declares, and there is no longer a second class + * of session for the codebase (or the operator) to keep track of. * - * `--permanent` is deliberately absent from the flag table: a permanent ad-hoc session is a contradiction - * (nothing declares it, so nothing can bring it back), and `permanentOverride` is pinned false below so a - * role whose default is permanent cannot smuggle respawn semantics in. */ + * `--permanent` is now accepted, unlike in #92: a run-created agent is a real catalog member, so "always-on" + * is a coherent thing to ask for and `strategy = "permanent"` is the legal value that expresses it. */ export async function cmdRun(args: string[]): Promise { const bad = unknownFlag(args, ...flagAllowList("run")); if (bad) { @@ -926,91 +970,95 @@ export async function cmdRun(args: string[]): Promise { return 2; } - // Role defaults to `worker`: an ad-hoc session is a hands-on one-off, and worker is the least-privileged - // role that still gets a persona. Spawner roles are permitted but are a poor fit — nothing it spawns is - // declared either, and the children outlive the ad-hoc parent that has no supervisor to escalate to. - const roleRaw = positionals(args)[0] ?? "worker"; - const role = parseRole(roleRaw); - if (!role) { - err(`unknown role "${roleRaw}". Valid: ${ROLES.join(", ")}`); - return 2; - } + // Role defaults to `worker` (kept from #92): the least-privileged role that still gets a persona, and by + // far the most common thing started by hand. `buildDeclaration` reads the role as a positional, so supply + // the default by prepending it rather than by teaching the shared builder a run-only special case. + const withRole = positionals(args).length > 0 ? args : ["worker", ...args]; - const harnessRaw = (optValue(args, "--harness") ?? "claude").toLowerCase(); - if (!isHarness(harnessRaw)) { - err(`unknown harness "${harnessRaw}". Valid: ${HARNESSES.join(", ")}`); - return 2; - } - const transport = resolveTransport(args); - if (!transport) { - err(`unknown transport. Valid: ding, mcp`); - return 2; + const built = await buildDeclaration(withRole, "run"); + if (typeof built === "number") return built; + const { af, path, existed, network } = built; + const identity = af.identity; + const force = hasFlag(args, "--force"); + const dryRun = hasFlag(args, "--dry-run"); + + // ACTUAL state, keyed EXACTLY as `convoy up`'s reconcile keys it (reconcile.ts `agentBusId` + up.ts + // `busIdOf`), so `run` and `up` can never disagree about what "already running" means. The gone-but-pid- + // alive tolerance is reconcile's too: pty can transiently report `gone` under load, and treating that as + // dead would relaunch a live agent. + const busId = agentBusId(af, shortHostname()); + const live = (await new PtyHost(network).sessions()).filter( + (s) => busIdOf(s) === busId && (!gone(s) || processAlive(s.pid)), + ); + + const action = resolveRunAction({ declared: existed, live: live.length > 0, force }); + if (action === "refuse-live-force") { + err(liveForceRefusal(identity)); + return 1; } - const model = optValue(args, "--model"); - if (model !== null && !isValidModel(model)) { - err(`invalid --model "${model}" — use letters, digits, and . _ : / - (start alphanumeric), e.g. claude-fable-5`); - return 2; + + // On resume/attach the EXISTING declaration is authoritative — the catalog is the source of truth once it + // exists, and silently re-declaring from this invocation's flags would let a stray `--model` mutate a + // synced, fleet-visible agent file as a side effect of attaching to it. + const reuseExisting = action === "resume" || action === "attach"; + let effective = af; + if (reuseExisting) { + try { + effective = readAgentFile(path); + } catch (e) { + err(`agent "${identity}" is declared at ${path} but its agent file could not be read: ${e instanceof Error ? e.message : String(e)}`); + return 1; + } + const note = staleFlagsNote(identity, passedDeclarationFlags(args)); + if (note) out(note); } - const network = resolveNetworkRoot(optValue(args, "--network")); + const spec = agentFileToSpec(effective, { networkRoot: network }); + const ref = sessionId(spec); + const attach = !hasFlag(args, "--no-attach"); - // An explicit --identity is validated against the DECLARED catalog, not just the live bus: colliding with - // a declared agent that happens to be down right now would still point this session at that agent's - // durable context. Generated identities skip the check — they cannot collide by construction. - const explicit = optValue(args, "--identity"); - let identity: string; - if (explicit !== null) { - const declared = readCatalog(network).entries.map((e) => e.af.identity); - const problem = validateRunIdentity(explicit, declared); - if (problem) { - err(problem); - return 2; + out(`convoy run — ${identity} (${effective.harness ?? "claude"}, ${effective.role})`); + out(`workspace: ${effective.workspace ?? "(none)"}`); + + if (action === "attach") { + out(` already running (${live.map((s) => s.name).join(", ")}) — attaching, NOT restarting it.`); + if (dryRun) { + out(`✓ Dry run only. Re-run without --dry-run to attach to ${ref}.`); + return 0; } - identity = explicit; - } else { - identity = generateAdHocIdentity(); + if (!attach) { + out(`✓ ${identity} is running. \`pty attach ${ref}\` to join it.`); + return 0; + } + return execInteractive("pty", ["attach", ref]); } - // Workspace: --dir, else the CURRENT directory. Unlike `convoy add`, a missing workspace is NOT an error - // and no worktree is cut off the megarepo — an ad-hoc session is normally "a harness, here, now", which - // is exactly what the launcher aliases it replaces did. Cutting a worktree would leave durable - // filesystem residue behind a session that promises no durability. - const dir = optValue(args, "--dir"); - const workingDir = dir ? resolve(expandTilde(dir)) : process.cwd(); - - const runBin = optValue(args, "--bin"); - if (runBin !== null && !isValidBin(runBin)) { - err(`invalid --bin "${runBin}" — it is interpolated into the launch command, so it must be a plain path or command name (letters, digits, and . _ / - ), with no spaces, quotes, or shell metacharacters`); - return 2; + // declare / redeclare → persist the declaration. `resume` deliberately writes NOTHING. + if (action === "declare" || action === "redeclare") { + if (dryRun) { + out(`convoy run — DRY RUN: would ${existed ? "OVERWRITE" : "write"} the agent file ${path}:\n`); + out(agentFileToToml(af)); + out(`✓ Dry run only. Re-run without --dry-run to declare, launch, and attach ${identity}.`); + return 0; + } + writeAgentFile(path, af); + out(`✓ declared "${identity}" (${af.role}, host ${af.host})${action === "redeclare" ? " — REPLACED" : ""} → ${path}`); + } else { + out(` resuming the declared agent — its context/ and bus folder are picked back up where they were.`); + if (dryRun) { + out(`✓ Dry run only. Re-run without --dry-run to launch and attach ${identity}.`); + return 0; + } } - const dryRun = hasFlag(args, "--dry-run"); - const spec: AgentSpec = { - harness: harnessRaw, - role, - identity, - transport, - networkRoot: network, - // An ad-hoc session declares no extra environment — it is the runnable core, so it takes the network - // as-is. `--bin` IS honored: a deployment that wraps its harness (credential selection, persona - // projection, policy gates) wraps it for ad-hoc sessions too, and this path replaces the launcher - // aliases that were themselves those wrappers. - bin: runBin, - env: {}, - personaOverride: optValue(args, "--persona"), - workingDir, - permanentOverride: false, // never respawned — see the doc comment above - prefix: optValue(args, "--prefix"), - configDir: optValue(args, "--config-dir"), - model, - }; + const rc = await launchSpec(spec, { dryRun: false, force }); + if (rc !== 0) return rc; - out(`convoy run — ${identity} (${harnessRaw}, ${role})`); - out(`workspace: ${workingDir}`); - out(adHocNotice(identity, busAgentId(spec), role)); out(); - - return launchSpec(spec, { dryRun, force: hasFlag(args, "--force") }); + out(declaredRunNotice(identity, busAgentId(spec), ref)); + out(); + if (!attach) return 0; + return execInteractive("pty", ["attach", ref]); } // ---- `convoy ls --tree`: spawn-parentage tree (cos → supervisor → worker) + a remote section ---- diff --git a/src/exec.ts b/src/exec.ts index 220534b..ebceccf 100644 --- a/src/exec.ts +++ b/src/exec.ts @@ -1,7 +1,7 @@ // A thin promise wrapper around child_process for the few tools convoy still shells (`st`, `git`). // Most pty interaction is native via @compoundingtech/pty/client (src/host.ts); this is the residual seam. -import { execFile, execFileSync } from "node:child_process"; +import { execFile, execFileSync, spawn } from "node:child_process"; import { homedir } from "node:os"; export interface ExecResult { @@ -44,6 +44,19 @@ export function childEnv(overlay?: NodeJS.ProcessEnv): NodeJS.ProcessEnv { return { ...(overlay ?? process.env), PATH: enrichedPath() }; } +/** Run a child with the caller's OWN stdio — for handing the terminal over, not for capturing output. + * + * `run` above buffers stdout/stderr through execFile, which is exactly wrong for `pty attach`: the child + * needs the real TTY to put the terminal in raw mode, size the pane, and forward the detach key. Resolves + * with the child's exit status once the user detaches or the session ends. */ +export function execInteractive(cmd: string, args: string[], opts: { cwd?: string } = {}): Promise { + return new Promise((resolve) => { + const child = spawn(cmd, args, { cwd: opts.cwd, env: childEnv(), stdio: "inherit" }); + child.on("error", () => resolve(127)); // command not found / not executable + child.on("close", (code, signal) => resolve(typeof code === "number" ? code : signal ? 1 : 0)); + }); +} + export function run( cmd: string, args: string[], diff --git a/src/run.test.ts b/src/run.test.ts index 18bc68c..bf81476 100644 --- a/src/run.test.ts +++ b/src/run.test.ts @@ -1,12 +1,11 @@ import { describe, it, expect } from "vitest"; import { spawnSync } from "node:child_process"; -import { mkdirSync, mkdtempSync, readdirSync, rmSync, writeFileSync } from "node:fs"; +import { mkdirSync, mkdtempSync, readFileSync, readdirSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; import { fileURLToPath } from "node:url"; -import { isValidIdentity } from "./agent-spec.ts"; import { COMMANDS } from "./command-table.ts"; -import { AD_HOC_PREFIX, adHocNotice, generateAdHocIdentity, isAdHocIdentity, validateRunIdentity } from "./run.ts"; +import { declaredRunNotice, liveForceRefusal, passedDeclarationFlags, resolveRunAction, staleFlagsNote } from "./run.ts"; const root = dirname(dirname(fileURLToPath(import.meta.url))); const bin = join(root, "bin", "convoy"); @@ -22,155 +21,252 @@ function cli(args: string[], home: string): { rc: number; out: string; err: stri return { rc: r.status ?? -1, out: r.stdout ?? "", err: r.stderr ?? "" }; } -describe("ad-hoc identity generation", () => { - it("mints `run-` — the prefix is what makes a session visibly undeclared in `pty ls` / `st agents`", () => { - const id = generateAdHocIdentity(() => 0.123456789); - expect(id.startsWith(AD_HOC_PREFIX)).toBe(true); - expect(isAdHocIdentity(id)).toBe(true); +// ---- The decision table: what `run` does about what already exists ---- + +describe("resolveRunAction — the four states a `convoy run` can find, plus the one refusal", () => { + it("not declared → declare (write the agent file, launch, attach): the first-run path", () => { + expect(resolveRunAction({ declared: false, live: false, force: false })).toBe("declare"); }); - it("produces an identity the bus grammar accepts — a name convoy mints must be one smalltalk can bind", () => { - // #88 item 2's failure ordering: a name that launches cleanly and is then rejected by the bus is the - // worst case. A GENERATED name has no operator to blame, so it must be valid by construction. - for (let i = 0; i < 200; i++) { - expect(isValidIdentity(generateAdHocIdentity())).toBe(true); - } + it("ACCEPTANCE: declared but NOT live → RESUME, never a refusal — this is the whole point of declaring", () => { + // #92's `run` refused an identity that collided with a declared agent, and `add` refuses a re-declare + // without --force. Inheriting either would break the headline property: an agent's durable context + // survives a restart, so re-running a name you already declared is the everyday path, not a collision. + expect(resolveRunAction({ declared: true, live: false, force: false })).toBe("resume"); + }); + + it("declared AND live → attach to the running session (never restart it and lose in-flight state)", () => { + expect(resolveRunAction({ declared: true, live: true, force: false })).toBe("attach"); }); - it("stays short, so the ding socket path fits its budget", () => { - // pty binds `/..ding.sock` against a limited sun_path. A generated name - // must not be what pushes a network over that edge. - expect(generateAdHocIdentity().length).toBeLessThanOrEqual(12); + it("declared, not live, --force → redeclare with the flags given now, then launch", () => { + expect(resolveRunAction({ declared: true, live: false, force: true })).toBe("redeclare"); }); - it("is RANDOM, not a counter — a counter re-derives and would hand a restart a stranger's context (#88 item 6)", () => { - // The whole reason a generated name is tolerable here: `run-` never recurs, so the silent - // inherit-someone-else's-`now.md` failure is impossible rather than merely discouraged. - const seen = new Set(); - for (let i = 0; i < 500; i++) seen.add(generateAdHocIdentity()); - expect(seen.size).toBeGreaterThan(490); + it("declared AND live AND --force → refuse: relaunching would double-spawn on the same pinned session id", () => { + expect(resolveRunAction({ declared: true, live: true, force: true })).toBe("refuse-live-force"); }); - it("respects an injected random source, so the generator is testable without stubbing globals", () => { - let n = 0; - const seq = (): number => [0.111111, 0.222222, 0.333333][n++ % 3]!; - const a = generateAdHocIdentity(seq); - n = 0; - expect(generateAdHocIdentity(seq)).toBe(a); + it("--force on an UNdeclared agent is simply the declare path (nothing to overwrite)", () => { + expect(resolveRunAction({ declared: false, live: false, force: true })).toBe("declare"); }); }); -describe("validateRunIdentity", () => { - it("rejects a name the bus grammar would refuse", () => { - expect(validateRunIdentity("Worker_Fix", [])).toContain("invalid identity"); +describe("liveForceRefusal — points at the verb that actually does what was asked", () => { + it("names `convoy reload`, the existing kill+respawn verb, rather than growing a second way to do it", () => { + const m = liveForceRefusal("fodfix"); + expect(m).toContain("convoy reload fodfix"); + expect(m).toContain("SECOND session"); }); - it("REFUSES to reuse a declared agent's identity — an ad-hoc session must never open a declared agent's bus folder", () => { - // The mirror image of #88 item 6: reading a stranger's durable context, arrived at from the other - // direction. A declared agent that is merely DOWN right now still owns its context/. - const problem = validateRunIdentity("cos", ["cos", "fabric"]); - expect(problem).toContain("already a DECLARED agent"); - expect(problem).toContain("convoy up"); + it("tells the caller that dropping --force attaches to the running agent", () => { + expect(liveForceRefusal("fodfix")).toContain("convoy run --identity fodfix"); + }); +}); + +describe("staleFlagsNote — never silently ignore a flag that looks like it changed something", () => { + it("names the ignored flags and how to actually apply them", () => { + const n = staleFlagsNote("fodfix", ["--model", "--harness"]); + expect(n).toContain("--model, --harness"); + expect(n).toContain("--force"); + expect(n).toContain("EXISTING declaration"); }); - it("accepts a fresh, well-formed name", () => { - expect(validateRunIdentity("scratch", ["cos"])).toBeNull(); + it("says nothing when no declaration flags were passed (the clean resume)", () => { + expect(staleFlagsNote("fodfix", [])).toBeNull(); + }); + + it("agrees in number so the sentence reads correctly for a single flag", () => { + expect(staleFlagsNote("fodfix", ["--model"])).toContain("--model was ignored"); }); }); -describe("adHocNotice", () => { - it("names every property the session does NOT have, so nobody reads it as equivalent to a declared one", () => { - const notice = adHocNotice("run-abc123", "dev3.run-abc123", "worker"); - expect(notice).toContain("NOT a declared catalog member"); - expect(notice).toMatch(/respawn|recover/); - expect(notice).toContain("no durable context"); - expect(notice).toContain("dev3.run-abc123"); +describe("passedDeclarationFlags — distinguishes declaration flags from per-invocation ones", () => { + it("picks up flags that describe the DECLARATION", () => { + expect(passedDeclarationFlags(["run", "--identity", "x", "--model", "m", "--permanent"])).toEqual(["--model", "--permanent"]); }); - it("points at the declared path — the counter-pressure against `run` quietly becoming the default", () => { - expect(adHocNotice("run-abc123", "dev3.run-abc123", "worker")).toContain("convoy add"); + it("ignores flags that only steer THIS invocation — they are not part of the agent file", () => { + // --network/--dry-run/--no-attach/--force change what this command does, not what the agent IS, so + // passing them on a resume is not a silently-ignored configuration change. + expect(passedDeclarationFlags(["run", "--network", "n", "--dry-run", "--no-attach", "--force"])).toEqual([]); }); }); +describe("declaredRunNotice — states the guarantees, the exact inverse of #92's disclaimer", () => { + it("ACCEPTANCE: says detaching leaves the agent RUNNING (a pty session outlives its client)", () => { + const n = declaredRunNotice("fodfix", "dev3.fodfix", "dev3.fodfix"); + expect(n).toContain("detaching leaves it RUNNING"); + }); + + it("promises reconcile + respawn + durable context — the things an ad-hoc session could never have", () => { + const n = declaredRunNotice("fodfix", "dev3.fodfix", "dev3.fodfix"); + expect(n).toContain("convoy up"); + expect(n).toContain("context/ survives a restart"); + }); + + it("tells the caller how to get back in, by the same command they just ran", () => { + expect(declaredRunNotice("fodfix", "dev3.fodfix", "dev3.fodfix")).toContain("convoy run --identity fodfix"); + }); + + it("points decommissioning at `retired = true`, not at `convoy down` (which only stops the session)", () => { + expect(declaredRunNotice("fodfix", "dev3.fodfix", "dev3.fodfix")).toContain("retired = true"); + }); + + it("carries NO ad-hoc disclaimer — the superseded wording must not survive anywhere", () => { + const n = declaredRunNotice("fodfix", "dev3.fodfix", "dev3.fodfix"); + expect(n).not.toContain("ad-hoc"); + expect(n).not.toContain("NOT a declared"); + }); +}); + +// ---- The command table: `run` must present as `add` + launch + attach ---- + describe("`run` in the command table", () => { - const run = COMMANDS.find((c) => c.name === "run"); + const runCmd = COMMANDS.find((c) => c.name === "run"); + const addCmd = COMMANDS.find((c) => c.name === "add"); + const names = (c: typeof runCmd): string[] => (c?.flags ?? []).map((f) => f.name).sort(); it("is declared, so dispatch accepts its flags and completions cover it", () => { - expect(run).toBeDefined(); + expect(runCmd).toBeDefined(); + }); + + it("ACCEPTANCE: `run` offers exactly `add`'s flags plus --no-attach — the two verbs cannot drift apart", () => { + // The claim of this design is "run IS add, plus launch and attach". If the flag sets diverge, that + // claim is false at the surface the user actually touches. + expect(names(runCmd)).toEqual([...names(addCmd), "no-attach"].sort()); + }); + + it("has --permanent now: a run-created agent is a real catalog member, so always-on is coherent", () => { + expect(runCmd?.flags?.some((f) => f.name === "permanent")).toBe(true); }); - it("has NO --permanent flag: a permanent ad-hoc session is a contradiction — nothing declares it, so nothing can bring it back", () => { - expect(run?.flags?.some((f) => f.name === "permanent")).toBe(false); + it("has --host, so a run-created agent can be owned by a named host like any declared one", () => { + expect(runCmd?.flags?.some((f) => f.name === "host")).toBe(true); }); - it("offers --config-dir, the account selection the launcher aliases it replaces actually used", () => { - expect(run?.flags?.some((f) => f.name === "config-dir")).toBe(true); + it("drops --prefix: the declaration carries the host prefix, and an override would desync session from agent file", () => { + expect(runCmd?.flags?.some((f) => f.name === "prefix")).toBe(false); + }); + + it("ACCEPTANCE: the help text no longer advertises the superseded ad-hoc semantics", () => { + expect(runCmd?.desc).not.toMatch(/ad-hoc|not declared|not reconciled|not respawned/i); + expect(runCmd?.desc).toMatch(/declare/i); }); }); +// ---- End to end, through the real CLI ---- + describe("`convoy run` end to end", () => { let home: string; const setup = (): string => { - home = mkdtempSync(join(tmpdir(), "convoy-run-")); + home = mkdtempSync(join(tmpdir(), "cvr-")); return home; }; const teardown = (): void => rmSync(home, { recursive: true, force: true }); + const catalogOf = (h: string): string => join(h, "convoy", "default", "catalog"); - it("writes NO catalog entry — the defining property: an ad-hoc session is not a declared member", () => { + it("ACCEPTANCE: WRITES a catalog entry — the defining inversion of #92, which wrote none", () => { const h = setup(); try { - const net = join(h, "convoy", "default"); - const catalog = join(net, "catalog"); - mkdirSync(catalog, { recursive: true }); - const r = cli(["run", "--dry-run", "--dir", h], h); - expect(readdirSync(catalog)).toEqual([]); - expect(r.out).toContain("ad-hoc session"); + mkdirSync(catalogOf(h), { recursive: true }); + const r = cli(["run", "worker", "--identity", "fodfix", "--dir", h, "--no-attach", "--dry-run"], h); + expect(r.rc).toBe(0); + // --dry-run shows the agent file it WOULD write; the declaration is the point, so it must be visible. + expect(r.out).toContain('identity = "fodfix"'); + expect(r.out).toContain("agent file"); } finally { teardown(); } }); - it("prints the contract disclosure on every launch, not behind a flag", () => { + it("ACCEPTANCE: requires --identity — a generated name is the hashed-name problem that kills durable context", () => { const h = setup(); try { - const r = cli(["run", "--dry-run", "--dir", h], h); - expect(r.out).toContain("NOT a declared catalog member"); - expect(r.out).toContain("convoy add"); + const r = cli(["run", "worker", "--dir", h, "--dry-run"], h); + expect(r.rc).toBe(2); + expect(r.err).toContain("--identity is required"); + // and it must explain WHY, since #92 accepted the omission happily + expect(r.err).toContain("context/"); } finally { teardown(); } }); - it("refuses an --identity that collides with a declared agent (rc=2), before doing anything", () => { + it("ACCEPTANCE: an --identity that is already DECLARED resumes it — #92 refused this outright (rc=2)", () => { const h = setup(); try { - const catalog = join(h, "convoy", "default", "catalog"); + const catalog = catalogOf(h); mkdirSync(catalog, { recursive: true }); - writeFileSync(join(catalog, "cos.toml"), 'identity = "cos"\nrole = "chief-of-staff"\n'); - const r = cli(["run", "--identity", "cos", "--dry-run", "--dir", h], h); - expect(r.rc).toBe(2); - expect(r.err).toContain("already a DECLARED agent"); + writeFileSync(join(catalog, "cos.toml"), 'identity = "cos"\nrole = "chief-of-staff"\nworkspace = "' + h + '"\n'); + const r = cli(["run", "--identity", "cos", "--dry-run", "--no-attach"], h); + expect(r.rc).toBe(0); + expect(r.out).toContain("resuming the declared agent"); + expect(r.err).not.toContain("already a DECLARED agent"); } finally { teardown(); } }); - it("rejects an unknown flag rather than silently ignoring it (rc=2), like every other convoy command", () => { + it("a resume does NOT rewrite the declaration — the catalog is the source of truth once it exists", () => { const h = setup(); try { - const r = cli(["run", "--nope"], h); - expect(r.rc).toBe(2); - expect(r.err).toContain("unrecognized flag"); + const catalog = catalogOf(h); + mkdirSync(catalog, { recursive: true }); + const file = join(catalog, "cos.toml"); + const original = 'identity = "cos"\nrole = "chief-of-staff"\nworkspace = "' + h + '"\n'; + writeFileSync(file, original); + const r = cli(["run", "--identity", "cos", "--model", "claude-fable-5", "--dry-run", "--no-attach"], h); + expect(r.rc).toBe(0); + expect(readFileSync(file, "utf8")).toBe(original); // untouched + expect(r.out).toContain("--model"); // and it said so, rather than silently dropping it + } finally { + teardown(); + } + }); + + it("accepts --permanent, which #92 rejected (rc=2) as a contradiction for an undeclared session", () => { + const h = setup(); + try { + mkdirSync(catalogOf(h), { recursive: true }); + const r = cli(["run", "worker", "--identity", "always", "--dir", h, "--permanent", "--dry-run", "--no-attach"], h); + expect(r.rc).toBe(0); + expect(r.out).toContain('strategy = "permanent"'); } finally { teardown(); } }); - it("rejects --permanent: respawn semantics are not available to an undeclared session", () => { + it("validates the identity against the bus grammar BEFORE writing to the synced catalog", () => { const h = setup(); try { - const r = cli(["run", "--permanent", "--dry-run"], h); + mkdirSync(catalogOf(h), { recursive: true }); + const r = cli(["run", "worker", "--identity", "Worker_Fix", "--dir", h, "--dry-run"], h); expect(r.rc).toBe(2); + expect(readdirSync(catalogOf(h))).toEqual([]); + } finally { + teardown(); + } + }); + + it("refuses with no workspace, exactly as `add` does — run inherits add's validation wholesale", () => { + const h = setup(); + try { + mkdirSync(catalogOf(h), { recursive: true }); + const r = cli(["run", "worker", "--identity", "nodir", "--dry-run"], h); + const add = cli(["add", "worker", "--identity", "nodir", "--dry-run"], h); + expect(r.rc).toBe(1); + expect(r.err).toContain("no workspace"); + expect(add.err).toContain("no workspace"); // same message, same code path + } finally { + teardown(); + } + }); + + it("rejects an unknown flag rather than silently ignoring it (rc=2), like every other convoy command", () => { + const h = setup(); + try { + expect(cli(["run", "--nope"], h).rc).toBe(2); } finally { teardown(); } @@ -179,7 +275,7 @@ describe("`convoy run` end to end", () => { it("rejects an unknown role", () => { const h = setup(); try { - const r = cli(["run", "wizard", "--dry-run"], h); + const r = cli(["run", "wizard", "--identity", "x", "--dry-run"], h); expect(r.rc).toBe(2); expect(r.err).toContain("unknown role"); } finally { @@ -188,20 +284,20 @@ describe("`convoy run` end to end", () => { }); }); -describe("adHocNotice — the declared-path suggestion", () => { - it("suggests a PLACEHOLDER name, never the generated one: a meaningless declared name is what breaks continuity", () => { - const notice = adHocNotice("run-b3ur0v", "dev3.run-b3ur0v", "worker"); - expect(notice).toContain(""); - expect(notice).not.toContain("--identity b3ur0v"); - }); +describe("the ad-hoc model is GONE, not merely unused", () => { + const src = readFileSync(join(root, "src", "run.ts"), "utf8"); - it("carries the actual role into the suggestion, so the suggested command is runnable as printed", () => { - expect(adHocNotice("run-x1", "dev3.run-x1", "supervisor")).toContain("convoy add supervisor"); + it("ACCEPTANCE: run.ts exports no ad-hoc machinery — two models in the codebase is the thing being removed", () => { + expect(src).not.toContain("export function generateAdHocIdentity"); + expect(src).not.toContain("export function adHocNotice"); + expect(src).not.toContain("export function isAdHocIdentity"); + expect(src).not.toContain("export const AD_HOC_PREFIX"); }); - it("does not claim a per-launch identity when the operator named the session explicitly", () => { - const named = adHocNotice("scratch", "dev3.scratch", "worker"); - expect(named).not.toContain("minted per launch"); - expect(named).toContain("NOT a declared catalog member"); + it("commands.ts no longer imports or mints an ad-hoc identity", () => { + const cmds = readFileSync(join(root, "src", "commands.ts"), "utf8"); + expect(cmds).not.toContain("generateAdHocIdentity("); + expect(cmds).not.toContain("adHocNotice("); + expect(cmds).not.toContain("validateRunIdentity("); }); }); diff --git a/src/run.ts b/src/run.ts index 8970efd..bf0e9ba 100644 --- a/src/run.ts +++ b/src/run.ts @@ -1,109 +1,126 @@ -// `convoy run` — an AD-HOC session: the runnable core WITHOUT the declaration on top. +// `convoy run` — DECLARE, launch, and attach. The interactive front door to the SAME declared model +// every other verb speaks. // -// The agent spec's own layering is the justification: "`pty` is the runnable core; the agent fields are -// the superset — a `pty` block alone still runs; convoy just layers intent on top." An ad-hoc session is -// that lower layer. It is deliberately NOT a catalog member, and the line is drawn where the spec already -// draws it rather than at a flag we invented. +// This SUPERSEDES the ad-hoc design that shipped in #92. `convoy run` is now exactly `convoy add` — +// same flags, same validation, same catalog write — plus a launch and an attach. The verb set is: // -// Why not a transient catalog entry (the "ephemeral declaration" fork in open-questions.md): -// 1. The catalog syncs under a UNION/NO-DELETE policy (see fabric-sync.ts, and the spec's `retired` -// field: "an edit, never a file delete"). A per-run entry is therefore PERMANENT, fleet-wide garbage -// that nothing can ever collect. Ad-hoc sessions are the highest-frequency kind; that is the worst -// possible thing to make undeletable. -// 2. `supervisor` is required for every non-root agent. An ad-hoc session has no honest answer, so an -// ephemeral declaration would have to fabricate one and corrupt the supervision tree. -// 3. convoy's own schema already refuses it: `strategy` accepts ONLY "permanent", and -// agent-file.test.ts asserts `strategy = "ephemeral"` throws. Declaring-but-ephemeral is a shape the -// declaration contract does not have. +// add declare (no launch) +// render materialize the overlay (no launch, no bus) +// up reconcile — launch what is declared (no attach) +// run declare, launch, and attach // -// What an ad-hoc session consequently does NOT get — stated here because the whole risk of this command -// is someone later assuming it is equivalent to a declared one: -// · no catalog entry → `convoy up` cannot reconcile it, and never will -// · no respawn / no recovery → dies with its process; a host reboot does not bring it back (R44) -// · no durable context → its identity is minted per launch, so it starts empty every time and -// CANNOT satisfy R48/R49 by construction. This is the honest reason a -// generated name is safe here: nothing is promised on top of it. -// · no adoption path → it cannot later become declared; declare it properly instead. +// Why the ad-hoc model was wrong, in one sentence: an undeclared session has no stable identity, +// therefore no durable `context/` directory, therefore it cannot externalize state or converge after a +// restart — a permanent second class of session that could never be promoted. Making `run` declare means +// every agent gets the same guarantees and the ad-hoc/declared distinction disappears rather than being +// managed forever. // -// What it DOES get, because it launches through convoy's real launch path rather than bare-exec'ing a -// harness: the network's ST_ROOT/PTY_ROOT wiring, a bus identity with inbox/archive (so it is addressable -// and observable), persona + permission-mode derivation, pretrust, and a real pty session. +// #92's three objections do not survive: +// 1. "`supervisor` is required for every non-root agent" — FALSE. `parseAgentFile` requires only +// `identity` and `role`; there is no such field requirement. This was refuted on review. +// 2. "A per-run catalog entry is permanent fleet-wide garbage (union/no-delete sync)." Permanence is now +// the INTENT, not a leak: a run-created agent is a declared agent that happened to be started +// interactively. `retired = true` is the documented way to decommission one. +// 3. "`strategy` accepts only `permanent`." Consistent, not an obstacle — a run-created agent IS a real +// declared agent, so the one legal value is the correct one. +// +// The coherence property that makes this safe: `convoy up`'s reconcile keys liveness on the bus id +// `.` (reconcile.ts `agentBusId`), and `run` launches through the very same `nativeLaunch` +// that up's reconcile uses, pinning the same identity-derived session id. So a session started by `run` is +// ADOPTED by a concurrently-running `convoy up` — never double-spawned. Declaring and launching in one +// breath is indistinguishable, to every other verb, from `add` followed by `up`. -import { isValidIdentity } from "./agent-spec.ts"; +/** What `convoy run` should do about the declaration + session that already exist (or don't). + * + * `run` deliberately does NOT inherit `add`'s flat "already declared → refuse without --force". The + * headline property of the declared model is that an agent's durable context SURVIVES a restart, so + * re-running an identity you already declared is the intended everyday path — the resume — not a + * collision. Refusing it would break the exact thing this redesign exists to deliver. */ +export type RunAction = + /** Not declared → write the agent file, launch, attach. The first-run path. */ + | "declare" + /** Declared, no live session → launch from the EXISTING declaration, attach. The resume path: durable + * context is picked back up. The declaration is left untouched; changing it needs --force. */ + | "resume" + /** Declared and already live → attach to the running session. No re-declare, no relaunch: joining an + * agent that is already working must never restart it and lose its in-flight state. */ + | "attach" + /** Declared, not live, --force → overwrite the declaration with the flags given now, then launch+attach. */ + | "redeclare" + /** Declared AND live AND --force → REFUSE. Overwriting a live agent's declaration and relaunching it + * would spawn a second session on the same pinned session id. `convoy reload` is the existing verb for + * kill-and-respawn, so point at it rather than growing a second way to do it. */ + | "refuse-live-force"; -/** Prefix marking a bus identity as ad-hoc. Chosen so `st agents` / `convoy ls` / `pty ls` all show at a - * glance that a session is outside the declaration contract, with no lookup and no extra field. */ -export const AD_HOC_PREFIX = "run-"; +export interface RunSituation { + /** A catalog entry for this identity already exists. */ + readonly declared: boolean; + /** A live pty session is already serving this identity's bus id. */ + readonly live: boolean; + /** --force was passed. */ + readonly force: boolean; +} -/** Length of the random discriminator. 6 base36 chars ≈ 2.2e9 — collision risk is negligible at the - * handful-per-day rate ad-hoc sessions actually occur, and the whole identity stays 10 bytes so it costs - * almost nothing against the socket-path budget (`/..ding.sock`). */ -export const AD_HOC_DISCRIMINATOR_LEN = 6; +/** Resolve what `convoy run` does, given what already exists. Pure — the decision table is the design, + * so it is testable without a network, a catalog, or a pty. */ +export function resolveRunAction(s: RunSituation): RunAction { + if (!s.declared) return "declare"; + if (s.live) return s.force ? "refuse-live-force" : "attach"; + return s.force ? "redeclare" : "resume"; +} -/** Generate an ad-hoc identity: `run-`. - * - * RANDOM, deliberately, not a counter. #88 item 6 is about `-` counter names: a counter - * re-derives within each parent's lifetime, so after a restart `worker-2` names a DIFFERENT agent and - * would read a stranger's `context/now.md` as its own memory. A random discriminator never recurs, so - * the silent-inheritance failure is impossible by construction rather than merely discouraged. - * - * `rand` is injectable so the generator is testable without stubbing globals. */ -export function generateAdHocIdentity(rand: () => number = Math.random): string { - let s = ""; - while (s.length < AD_HOC_DISCRIMINATOR_LEN) { - // Drop the leading "0." and take base36 digits; loop because Math.random() can return a short string. - s += rand().toString(36).slice(2); - } - return `${AD_HOC_PREFIX}${s.slice(0, AD_HOC_DISCRIMINATOR_LEN)}`; +/** The message for the refused case, naming the verb that actually does what the user asked for. */ +export function liveForceRefusal(identity: string): string { + return ( + `"${identity}" is declared AND already running. \`--force\` would overwrite its declaration and launch a ` + + `SECOND session on the same pinned session id. To pick up the running one, drop \`--force\` — ` + + `\`convoy run --identity ${identity}\` attaches to it. To apply a CHANGED declaration to a live agent, use ` + + `\`convoy reload ${identity}\` (kill + respawn), or \`convoy down ${identity}\` and re-run with --force.` + ); } -/** True when an identity was minted by `convoy run`. Used to keep the disclosure honest: an explicitly - * named ad-hoc session (`--identity`) is still ad-hoc, but its name at least CAN recur, so it is worth - * telling the user that naming alone does not make it declared. */ -export function isAdHocIdentity(identity: string): boolean { - return identity.startsWith(AD_HOC_PREFIX); +/** The note printed when `run` resumes or attaches to an EXISTING declaration while the caller also passed + * configuration flags. Silently ignoring `--model`/`--harness`/... would be a real footgun: the user would + * believe they changed something. `add` gates a re-declare behind `--force`; `run` says so out loud. */ +export function staleFlagsNote(identity: string, flags: readonly string[]): string | null { + if (flags.length === 0) return null; + return ( + ` ! using the EXISTING declaration for "${identity}" — ${flags.join(", ")} ${flags.length === 1 ? "was" : "were"} ignored.\n` + + ` The catalog entry is the source of truth once it exists. \`--force\` re-declares it with these flags ` + + `(the agent must not be running), or edit the agent file directly.` + ); } -/** Validate an operator-supplied `--identity` for `convoy run`. - * - * Returns an error string, or null when acceptable. Deliberately REFUSES a name that collides with a - * declared catalog member: an ad-hoc session that shares an identity with a declared agent would write - * into that agent's bus folder — the same read-a-stranger's-memory failure #88 item 6 exists to prevent, - * arrived at from the other direction. */ -export function validateRunIdentity(identity: string, declared: readonly string[]): string | null { - if (!isValidIdentity(identity)) { - return `invalid identity "${identity}" — lowercase alphanumeric plus \`. _ -\`, starting alphanumeric.`; - } - if (declared.includes(identity)) { - return ( - `"${identity}" is already a DECLARED agent in this network. An ad-hoc session under that identity would ` + - `share its bus folder and could read or overwrite its durable context. Pick another name, or run the ` + - `declared agent with \`convoy up\`.` - ); - } - return null; +/** The configuration flags that describe a DECLARATION (as opposed to flags that only steer this + * invocation, like --network / --dry-run / --no-attach). Used to detect the stale-flag case above. */ +export const DECLARATION_FLAGS = [ + "--harness", + "--model", + "--transport", + "--mcp", + "--dir", + "--bin", + "--supervisor", + "--persona", + "--permanent", + "--host", +] as const; + +/** Which declaration flags the caller actually passed. */ +export function passedDeclarationFlags(args: readonly string[]): string[] { + return DECLARATION_FLAGS.filter((f) => args.includes(f)); } -/** The contract disclosure printed on every ad-hoc launch. - * - * Printed EVERY time, not once and not behind a flag. The stated risk of having this command at all is - * that it quietly becomes the default and the declared path withers; a notice that names what is missing - * and points at `convoy add` on every single launch is the cheapest available counter-pressure, and it - * costs nothing to anyone who genuinely wants a one-off. */ -export function adHocNotice(identity: string, busId: string, role: string): string { - // The suggested declared name is deliberately a PLACEHOLDER, never this session's identity. Echoing back - // a generated `run-b3ur0v` would propose carrying a meaningless name into the catalog — and a meaningless - // declared name is precisely what breaks context continuity, since nobody re-derives it on the next - // cold boot. The whole value of declaring is choosing a name that means something. - const generated = isAdHocIdentity(identity); - const continuity = generated - ? ` · its identity is minted per launch, so it has no durable context and starts empty every time.\n` - : ` · it is undeclared, so nothing recreates it — its context does not survive as a managed session's would.\n`; +/** The line `run` prints once the session is up, stating the guarantees it DOES have — the exact inverse + * of #92's `adHocNotice`, which existed to disclaim them. Detaching is safe and is worth saying: a pty + * session outlives its client, and because the agent is declared, `convoy up` also respawns it if the + * process dies. That is the whole point of routing `run` through the declared path. */ +export function declaredRunNotice(identity: string, busId: string, sessionRef: string): string { return ( - ` ad-hoc session — NOT a declared catalog member.\n` + - ` · \`convoy up\` will not reconcile, respawn, or recover it; it dies with its process.\n` + - continuity + - ` · addressable on the bus as ${busId} while it lives.\n` + - ` For work that should survive a restart, declare it instead: \`convoy add ${role} --identity \`.` + ` declared agent — detaching leaves it RUNNING.\n` + + ` · detach with the pty escape; re-attach any time with \`convoy run --identity ${identity}\` or \`pty attach ${sessionRef}\`.\n` + + ` · \`convoy up\` reconciles and respawns it like any other catalog member; its context/ survives a restart.\n` + + ` · addressable on the bus as ${busId}.\n` + + ` Decommission it with \`retired = true\` in its agent file (\`convoy down ${identity}\` just stops the session).` ); } From 5bd857ae62e229061ebceefc1222be2f26835954 Mon Sep 17 00:00:00 2001 From: schickling-assistant <261620128+schickling-assistant@users.noreply.github.com> Date: Tue, 21 Jul 2026 12:00:11 +0200 Subject: [PATCH 2/2] key `run`'s liveness on the DECLARATION's host, as reconcile does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `buildDeclaration` always populates `host` (`--host` ?? this machine), so keying liveness on the args-built agent file computed a this-machine bus id for an agent declared `host = otherbox` — whose catalog file is present locally via fabric sync. `convoy run --identity ` would find nothing live and launch a DUPLICATE alongside the real session. That falsified the property this design rests on: `run` and `up` must agree on what "already running" means. `up` reconciles on `entry.af.host ?? thisHost` — the declaration's host — so `run` now reads the existing declaration BEFORE computing the bus id and keys off it. The choice is a named, exported function (`livenessAgentFile`) rather than an inline `??`: it is the single point where the two verbs agree or diverge, so it deserves to be visible and directly testable. Reverting it to return the args-built file turns two tests red. Also, while reading the existing declaration up front: · reuse it for `effective` instead of re-reading the file · warn when a positional ROLE differs from the declared one — the role is not a flag, so `passedDeclarationFlags` could not see it and it was silently dropped on resume · print `session:` / `bus:` ids, so a declaration owned by another host is visible before anything launches Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_014ePNMmLYa7qVT3h7bRCWUJ agent-session-id: 0abcedc7-6b71-4046-9e7c-f645268c0b15 agent-tool: Claude Code agent-tool-version: 2.1.215 agent-model: claude-opus-4-8 agent-runtime-profile: /nix/store/acr8a3l2v366jgmwiq8xdrhgz1py0db5-coding-agent-runtime-profile/share/coding-agents/profile.json agent-skills-manifest: /nix/store/sj1v5j91h8v8d1w9lca4040302lwrd6v-agent-skills-corpus/share/agent-skills/manifest.json tooling-profile: dotfiles@unknown-dirty --- src/commands.ts | 39 +++++++++++++++++++++++++++++---------- src/run.test.ts | 44 ++++++++++++++++++++++++++++++++++++++++++-- src/run.ts | 15 +++++++++++++++ 3 files changed, 86 insertions(+), 12 deletions(-) diff --git a/src/commands.ts b/src/commands.ts index df5f6e7..1bbcf59 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -30,7 +30,7 @@ import { runFullOrgSuite, runReadinessSuite } from "./doctor/suite.ts"; import { structureChecks } from "./doctor/structure.ts"; import { baseFile, ensureInstalled, personasDir, personasInstalled } from "./personas.ts"; import { ROLES, parseRole } from "./role.ts"; -import { declaredRunNotice, liveForceRefusal, passedDeclarationFlags, resolveRunAction, staleFlagsNote } from "./run.ts"; +import { declaredRunNotice, liveForceRefusal, livenessAgentFile, passedDeclarationFlags, resolveRunAction, staleFlagsNote } from "./run.ts"; import { busAgentId, isValidModel, preflight, resolvedPersonaPath, sessionId, shortHostname, type AgentSpec, type Transport } from "./agent-spec.ts"; import { HARNESSES, HARNESS_SESSION_KEYS, HARNESS_SUFFIX_RE, harnessDescriptor, harnessesInPtyToml, harnessLimitations, isHarness, type Harness } from "./harness.ts"; import { claudeConfigPath, codexConfigPath, pretrustDirs, pretrustDirsCodex } from "./trust.ts"; @@ -986,7 +986,24 @@ export async function cmdRun(args: string[]): Promise { // `busIdOf`), so `run` and `up` can never disagree about what "already running" means. The gone-but-pid- // alive tolerance is reconcile's too: pty can transiently report `gone` under load, and treating that as // dead would relaunch a live agent. - const busId = agentBusId(af, shortHostname()); + // Read the EXISTING declaration first, because liveness must be keyed off it, not off this invocation's + // flags. `buildDeclaration` always sets `af.host` (`--host` ?? this machine), whereas reconcile keys on + // `entry.af.host ?? thisHost` — the DECLARATION's host. Keying on the args host would mean that for an + // agent declared `host = dev4`, a `convoy run --identity ` on another box (its catalog file is present + // via fabric sync) computes the wrong bus id, sees nothing live, and launches a duplicate. That would + // falsify the exact property this design rests on: `run` and `up` must agree on what "already running" + // means. Deriving both from the same source makes them agree by construction. + let existingAf: AgentFile | null = null; + if (existed) { + try { + existingAf = readAgentFile(path); + } catch (e) { + err(`agent "${identity}" is declared at ${path} but its agent file could not be read: ${e instanceof Error ? e.message : String(e)}`); + return 1; + } + } + + const busId = agentBusId(livenessAgentFile(existingAf, af), shortHostname()); const live = (await new PtyHost(network).sessions()).filter( (s) => busIdOf(s) === busId && (!gone(s) || processAlive(s.pid)), ); @@ -1001,15 +1018,14 @@ export async function cmdRun(args: string[]): Promise { // exists, and silently re-declaring from this invocation's flags would let a stray `--model` mutate a // synced, fleet-visible agent file as a side effect of attaching to it. const reuseExisting = action === "resume" || action === "attach"; - let effective = af; + const effective = reuseExisting && existingAf ? existingAf : af; if (reuseExisting) { - try { - effective = readAgentFile(path); - } catch (e) { - err(`agent "${identity}" is declared at ${path} but its agent file could not be read: ${e instanceof Error ? e.message : String(e)}`); - return 1; - } - const note = staleFlagsNote(identity, passedDeclarationFlags(args)); + // The role is a POSITIONAL, not a flag, so `passedDeclarationFlags` cannot see it — report it + // separately rather than silently resuming `cos` as its declared role when the caller typed another. + const declaredRole = existingAf?.role; + const askedRole = positionals(args)[0]; + const roleDiffers = askedRole !== undefined && declaredRole !== undefined && parseRole(askedRole) !== declaredRole; + const note = staleFlagsNote(identity, [...(roleDiffers ? [`role "${askedRole}"`] : []), ...passedDeclarationFlags(args)]); if (note) out(note); } @@ -1019,6 +1035,9 @@ export async function cmdRun(args: string[]): Promise { out(`convoy run — ${identity} (${effective.harness ?? "claude"}, ${effective.role})`); out(`workspace: ${effective.workspace ?? "(none)"}`); + // Print the host-prefixed ids: they are what `convoy up`, `pty attach`, and `st` all key on, and showing + // them makes an unexpected host (a declaration owned by another box) visible before anything launches. + out(`session: ${ref} · bus: ${busAgentId(spec)}`); if (action === "attach") { out(` already running (${live.map((s) => s.name).join(", ")}) — attaching, NOT restarting it.`); diff --git a/src/run.test.ts b/src/run.test.ts index bf81476..5603ad6 100644 --- a/src/run.test.ts +++ b/src/run.test.ts @@ -1,11 +1,12 @@ import { describe, it, expect } from "vitest"; import { spawnSync } from "node:child_process"; import { mkdirSync, mkdtempSync, readFileSync, readdirSync, rmSync, writeFileSync } from "node:fs"; -import { tmpdir } from "node:os"; +import { hostname, tmpdir } from "node:os"; import { dirname, join } from "node:path"; import { fileURLToPath } from "node:url"; import { COMMANDS } from "./command-table.ts"; -import { declaredRunNotice, liveForceRefusal, passedDeclarationFlags, resolveRunAction, staleFlagsNote } from "./run.ts"; +import { declaredRunNotice, liveForceRefusal, livenessAgentFile, passedDeclarationFlags, resolveRunAction, staleFlagsNote } from "./run.ts"; +import { agentBusId } from "./reconcile.ts"; const root = dirname(dirname(fileURLToPath(import.meta.url))); const bin = join(root, "bin", "convoy"); @@ -93,6 +94,29 @@ describe("passedDeclarationFlags — distinguishes declaration flags from per-in }); }); +describe("livenessAgentFile — the single point where `run` agrees with `up` about what is running", () => { + const declared = { identity: "x", role: "worker" as const, host: "otherbox" }; + const fromArgs = { identity: "x", role: "worker" as const, host: "thisbox" }; + + it("ACCEPTANCE: keys on the EXISTING declaration — `up` reconciles on `af.host`, so `run` must too", () => { + // `buildDeclaration` ALWAYS populates host (--host ?? this machine), so keying on the args-built file + // computes a this-machine bus id for an agent declared `host = otherbox` (catalog file arrived via + // fabric sync). run would find nothing live and launch a DUPLICATE beside the real one. + expect(livenessAgentFile(declared, fromArgs)).toBe(declared); + expect(livenessAgentFile(declared, fromArgs).host).toBe("otherbox"); + }); + + it("falls back to the args-built file only when nothing is declared yet (the first-run path)", () => { + expect(livenessAgentFile(null, fromArgs)).toBe(fromArgs); + }); + + it("keys identically to reconcile's agentBusId for an explicitly-hosted agent", () => { + const thisHost = "thisbox"; + expect(agentBusId(livenessAgentFile(declared, fromArgs), thisHost)).toBe(agentBusId(declared, thisHost)); + expect(agentBusId(livenessAgentFile(declared, fromArgs), thisHost)).toBe("otherbox.x"); + }); +}); + describe("declaredRunNotice — states the guarantees, the exact inverse of #92's disclaimer", () => { it("ACCEPTANCE: says detaching leaves the agent RUNNING (a pty session outlives its client)", () => { const n = declaredRunNotice("fodfix", "dev3.fodfix", "dev3.fodfix"); @@ -225,6 +249,22 @@ describe("`convoy run` end to end", () => { } }); + it("resolves an explicitly-hosted declaration to ITS host, not this machine's", () => { + const h = setup(); + try { + const catalog = catalogOf(h); + mkdirSync(catalog, { recursive: true }); + writeFileSync(join(catalog, "elsewhere.toml"), 'identity = "elsewhere"\nrole = "worker"\nhost = "otherbox"\nworkspace = "' + h + '"\n'); + // No --host passed: the declaration's host must still win for the session ref and the bus id. + const r = cli(["run", "--identity", "elsewhere", "--dry-run", "--no-attach"], h); + expect(r.rc).toBe(0); + expect(r.out).toContain("otherbox.elsewhere"); + expect(r.out).not.toContain(`${hostname().split(".")[0]?.toLowerCase()}.elsewhere`); + } finally { + teardown(); + } + }); + it("accepts --permanent, which #92 rejected (rc=2) as a contradiction for an undeclared session", () => { const h = setup(); try { diff --git a/src/run.ts b/src/run.ts index bf0e9ba..37fafde 100644 --- a/src/run.ts +++ b/src/run.ts @@ -111,6 +111,21 @@ export function passedDeclarationFlags(args: readonly string[]): string[] { return DECLARATION_FLAGS.filter((f) => args.includes(f)); } +/** Which agent file `run` must key LIVENESS on: the existing declaration whenever there is one, never the + * one just built from this invocation's flags. + * + * This is the single point where `run` either does or does not agree with `convoy up` about what "already + * running" means, so it is a named function rather than an inline `??` — the coherence claim of this whole + * design reduces to this choice. + * + * `up`'s reconcile keys on `entry.af.host ?? thisHost` — the DECLARATION's host. `buildDeclaration` always + * populates `host` (`--host` ?? this machine), so keying on the args-built file would compute a + * this-machine-prefixed bus id for an agent declared `host = otherbox` (its catalog file having arrived by + * fabric sync). `run` would then find nothing live and launch a DUPLICATE alongside the real one. */ +export function livenessAgentFile(existing: T | null, fromArgs: T): T { + return existing ?? fromArgs; +} + /** The line `run` prints once the session is up, stating the guarantees it DOES have — the exact inverse * of #92's `adHocNotice`, which existed to disclaim them. Detaching is safe and is worth saying: a pty * session outlives its client, and because the agent is declared, `convoy up` also respawns it if the