Simplify instance lifecycle commands

2026-08-17 00:49:29 +08:00
parent f9fb4ef776
commit fe7bbd0631
13 changed files with 385 additions and 237 deletions
@@ -1,26 +1,59 @@
 import assert from "node:assert/strict";
 import { spawnSync } from "node:child_process";
+import fs from "node:fs";
+import os from "node:os";
 import path from "node:path";
 import test from "node:test";

-function runCli(args: string[]) {
+function runCli(args: string[], root = fs.mkdtempSync(path.join(os.tmpdir(), "gori-cli-"))) {
  return spawnSync(process.execPath, ["--import", "tsx", path.resolve("src/cli.ts"), ...args], {
    cwd: path.resolve("."),
    encoding: "utf8",
-    env: { ...process.env, GORI_GATEWAY_CONFIG: "" }
+    env: { ...process.env, GORI_AGENT_ROOT: root, GORI_GATEWAY_CONFIG: "" }
  });
 }

-test("CLI rejects unknown flags and command-specific extra arguments", () => {
-  const unknown = runCli(["status", "--bogus"]);
-  assert.equal(unknown.status, 1);
-  assert.match(unknown.stderr, /Unknown option: --bogus/);
-
-  const extra = runCli(["doctor", "extra"]);
-  assert.equal(extra.status, 1);
-  assert.match(extra.stderr, /doctor accepts only --config/);
-
-  const instanceFlag = runCli(["instance", "list", "--config", "config.json"]);
-  assert.equal(instanceFlag.status, 1);
-  assert.match(instanceFlag.stderr, /instance commands do not accept/);
+test("CLI help exposes only direct instance commands", () => {
+  const result = runCli(["--help"]);
+  assert.equal(result.status, 0);
+  for (const command of ["init", "setup", "start", "stop", "restart", "status", "logs", "doctor"]) {
+    assert.match(result.stdout, new RegExp(`gori-agent ${command} <bot-id>`));
+  }
+  assert.match(result.stdout, /gori-agent list/);
+  assert.doesNotMatch(result.stdout, /gori-agent instance|--config|--json|discover-backends|print feishu/);
+});
+
+test("CLI dispatches direct commands and rejects removed entry points", () => {
+  assert.equal(runCli(["list"]).status, 0);
+
+  for (const command of ["setup", "start", "stop", "restart", "status", "logs", "doctor"]) {
+    const result = runCli([command, "missing-bot"]);
+    assert.equal(result.status, 1);
+    assert.match(result.stderr, /Runtime config does not exist/);
+  }
+  const init = runCli(["init", "INVALID"]);
+  assert.equal(init.status, 1);
+  assert.match(init.stderr, /Invalid bot ID/);
+
+  for (const removed of [["instance", "list"], ["discover-backends"], ["discover-agents"], ["print", "feishu"]]) {
+    const result = runCli(removed);
+    assert.equal(result.status, 1);
+    assert.match(result.stderr, /Unknown command/);
+  }
+});
+
+test("CLI rejects options and invalid command argument counts", () => {
+  for (const args of [["status", "bot", "--config", "config.json"], ["list", "--json"], ["start", "--bogus"]]) {
+    const result = runCli(args);
+    assert.equal(result.status, 1);
+    assert.match(result.stderr, /Unknown option/);
+  }
+
+  const missing = runCli(["setup"]);
+  assert.equal(missing.status, 1);
+  assert.match(missing.stderr, /setup requires exactly one <bot-id>/);
+
+  const extra = runCli(["doctor", "one", "two"]);
+  assert.equal(extra.status, 1);
+  assert.match(extra.stderr, /doctor requires exactly one <bot-id>/);
 });
@@ -1,10 +1,12 @@
 import assert from "node:assert/strict";
 import fs from "node:fs";
+import { EventEmitter } from "node:events";
 import os from "node:os";
 import path from "node:path";
 import test from "node:test";
 import { writeConfigFile } from "../src/cli/config-file.js";
-import { instanceDirectory, runInstanceCommand } from "../src/cli/instance.js";
+import { assertSetupPidSafe, instanceDirectory, instanceRunnerPath, matchesInstanceRunner, runInstanceCommand } from "../src/cli/instance.js";
+import { runInstanceRunner, waitForShutdown } from "../src/cli/instance-runner.js";
 import { parseConfig } from "../src/config.js";

 test("instance paths reject traversal and config identity mismatch", async () => {
@@ -16,7 +18,7 @@ test("instance paths reject traversal and config identity mismatch", async () =>
    assert.throws(() => instanceDirectory("Uppercase"), /Invalid bot ID/);
    await assert.rejects(runInstanceCommand("list", ["extra"]), /does not accept/);
    await assert.rejects(runInstanceCommand("status", ["one", "two"]), /exactly one/);
-    await assert.rejects(runInstanceCommand("unknown", ["bot"]), /Unknown instance command/);
+    await assert.rejects(runInstanceCommand("unknown", ["bot"]), /Unknown command/);
    await assert.rejects(runInstanceCommand("status", ["missing-bot"]), /Runtime config does not exist/);

    const directory = instanceDirectory("directory-bot");
@@ -43,3 +45,65 @@ test("instance paths reject traversal and config identity mismatch", async () =>
    else process.env.GORI_AGENT_ROOT = previous;
  }
 });
+
+test("setup rejects both an owned runner PID and a foreign PID", () => {
+  assert.throws(() => assertSetupPidSafe("safe-bot", { kind: "running", pid: 123 }), /instance 'safe-bot' is running/);
+  assert.throws(() => assertSetupPidSafe("safe-bot", { kind: "foreign", pid: 456 }), /unrelated live process/);
+  assert.doesNotThrow(() => assertSetupPidSafe("safe-bot", { kind: "absent" }));
+  assert.doesNotThrow(() => assertSetupPidSafe("safe-bot", { kind: "stale" }));
+});
+
+test("runner identity requires exact executable, runner, and config arguments", () => {
+  const configFile = path.resolve("/tmp/test-instance-config.json");
+  const commandLine = [process.execPath, instanceRunnerPath(), configFile];
+  assert.equal(matchesInstanceRunner(commandLine, process.execPath, configFile), true);
+  assert.equal(matchesInstanceRunner([...commandLine, "extra"], process.execPath, configFile), false);
+  assert.equal(matchesInstanceRunner([process.execPath, path.resolve("dist/cli.js"), configFile], process.execPath, configFile), false);
+  assert.equal(matchesInstanceRunner(commandLine, "/bin/sh", configFile), false);
+  assert.equal(matchesInstanceRunner(commandLine, process.execPath, `${configFile}.other`), false);
+});
+
+test("instance runner rejects unsafe argument and config paths before starting a server", async () => {
+  await assert.rejects(runInstanceRunner([]), /exactly one config path/);
+  await assert.rejects(runInstanceRunner(["one", "two"]), /exactly one config path/);
+  await assert.rejects(runInstanceRunner([path.join(os.tmpdir(), `missing-runner-${Date.now()}.json`)]), /Runtime config does not exist/);
+});
+
+test("instance runner gracefully shuts down once on SIGTERM", async () => {
+  const signals = new EventEmitter();
+  let shutdowns = 0;
+  const result = waitForShutdown({ shutdown: async () => { shutdowns++; } }, signals);
+  signals.emit("SIGTERM");
+  signals.emit("SIGINT");
+  assert.equal(await result, 0);
+  assert.equal(shutdowns, 1);
+  assert.equal(signals.listenerCount("SIGINT"), 0);
+  assert.equal(signals.listenerCount("SIGTERM"), 0);
+});
+
+test("setup refuses a foreign live PID and unsafe PID file", async (t) => {
+  const root = fs.mkdtempSync(path.join(os.tmpdir(), "gori-setup-pid-"));
+  t.after(() => fs.rmSync(root, { recursive: true, force: true }));
+  const previous = process.env.GORI_AGENT_ROOT;
+  process.env.GORI_AGENT_ROOT = root;
+  t.after(() => {
+    if (previous === undefined) delete process.env.GORI_AGENT_ROOT;
+    else process.env.GORI_AGENT_ROOT = previous;
+  });
+
+  const directory = instanceDirectory("safe-bot");
+  writeConfigFile(path.join(directory, "config.json"), parseConfig({
+    configVersion: 3,
+    bot: { id: "safe-bot", workspace: os.tmpdir(), persona: "", agent: { id: "node", command: process.execPath, args: ["acp"] }, permissions: { mode: "deny" } },
+    gateway: { platform: { type: "webhook", secret: "test-secret" } },
+    runtime: {}
+  }));
+  fs.mkdirSync(path.join(directory, "state"), { mode: 0o700 });
+  fs.mkdirSync(path.join(directory, "logs"), { mode: 0o700 });
+  const pidFile = path.join(directory, "state", "gori-agent.pid");
+  fs.writeFileSync(pidFile, `${process.pid}\n`, { mode: 0o600 });
+  await assert.rejects(runInstanceCommand("setup", ["safe-bot"]), /unrelated live process/);
+
+  fs.chmodSync(pidFile, 0o644);
+  await assert.rejects(runInstanceCommand("setup", ["safe-bot"]), /PID file mode must be 0600/);
+});
@@ -70,16 +70,21 @@ function existingConfig(host = "0.0.0.0", port = 8787) {
      id: "existing-bot",
      workspace: os.tmpdir(),
      persona: "existing persona",
-      agent: { id: readyAgent.id, command: readyAgent.command, args: readyAgent.args, env: {} },
-      skills: [],
-      permissions: { mode: "deny", allowedTools: [], allowedCommandPatterns: [] }
+      agent: {
+        id: "custom-kimi",
+        command: readyAgent.command,
+        args: ["-m", "gori-gpt/gpt-5.5", "acp"],
+        env: { TEST_AGENT_SETTING: "preserved" }
+      },
+      skills: [{ id: "test-skill", file: path.join(os.tmpdir(), "test-skill.md"), maxBytes: 1234 }],
+      permissions: { mode: "allowlist", allowedTools: ["Read"], allowedCommandPatterns: ["^true$"] }
    },
    gateway: {
      server: { host, port, publicBaseUrl: "https://public.example.test" },
-      policy: { allowedUsers: [], allowedChats: [], requireMentionInGroup: true },
+      policy: { allowedUsers: ["allowed-user"], allowedChats: ["allowed-chat"], requireMentionInGroup: false },
      platform: { type: "webhook", secret: testSecret }
    },
-    runtime: {}
+    runtime: { acp: { promptTimeoutMs: 123456, maxProcesses: 3 } }
  });
 }

@@ -145,6 +150,17 @@ test("existing setup can change host and port while preserving public URL and se
  assert.equal(config.gateway.server.publicBaseUrl, "https://public.example.test");
  assert.equal(config.gateway.platform.type, "webhook");
  assert.equal(config.gateway.platform.secret, testSecret);
+  assert.deepEqual(config.bot.agent, {
+    id: "custom-kimi",
+    command: readyAgent.command,
+    args: ["-m", "gori-gpt/gpt-5.5", "acp"],
+    env: { TEST_AGENT_SETTING: "preserved" }
+  });
+  assert.deepEqual(config.bot.skills, [{ id: "test-skill", file: path.join(os.tmpdir(), "test-skill.md"), maxBytes: 1234 }]);
+  assert.deepEqual(config.bot.permissions, { mode: "allowlist", allowedTools: ["Read"], allowedCommandPatterns: ["^true$"] });
+  assert.deepEqual(config.gateway.policy, { allowedUsers: ["allowed-user"], allowedChats: ["allowed-chat"], requireMentionInGroup: false });
+  assert.equal(config.runtime.acp.promptTimeoutMs, 123456);
+  assert.equal(config.runtime.acp.maxProcesses, 3);
  assert.equal(prompt.closeCount, 1);
  assert.ok(messages.some((message) => message.startsWith("ERROR: server host")));
  assert.ok(messages.some((message) => message.startsWith("ERROR: server port")));
@@ -175,3 +191,51 @@ test("existing setup keeps host and port when defaults are accepted", async (t)
  assert.equal(config.gateway.server.port, 9123);
  assert.equal(prompt.closeCount, 1);
 });
+
+test("existing setup succeeds without a ready discovered agent and preserves its agent", async (t) => {
+  const root = fs.mkdtempSync(path.join(os.tmpdir(), "gori-setup-no-ready-"));
+  t.after(() => fs.rmSync(root, { recursive: true, force: true }));
+  const target = path.join(root, "existing-bot", "config.json");
+  writeConfigFile(target, existingConfig());
+
+  await runSetup(target, {
+    botId: "existing-bot",
+    writeWithoutConfirmation: true,
+    prompt: new FakePrompt({}, ""),
+    discoverAgents: async () => [{ id: "kimi", command: "kimi", args: ["acp"], status: "not-found" }],
+    instancesDirectory: root,
+    log: () => undefined
+  });
+
+  assert.deepEqual(loadConfigFile(target).config.bot.agent, existingConfig().bot.agent);
+});
+
+test("new setup still fails when no ACP agent is ready", async (t) => {
+  const root = fs.mkdtempSync(path.join(os.tmpdir(), "gori-setup-new-no-ready-"));
+  t.after(() => fs.rmSync(root, { recursive: true, force: true }));
+  const prompt = new FakePrompt();
+
+  await assert.rejects(runSetup(path.join(root, "new-bot", "config.json"), {
+    botId: "new-bot",
+    requireNew: true,
+    writeWithoutConfirmation: true,
+    prompt,
+    discoverAgents: async () => [],
+    instancesDirectory: root,
+    log: () => undefined
+  }), /No ACP agent is available/);
+  assert.equal(prompt.closeCount, 1);
+});
+
+test("existing setup rejects attempts to change bot identity", async (t) => {
+  const root = fs.mkdtempSync(path.join(os.tmpdir(), "gori-setup-identity-"));
+  t.after(() => fs.rmSync(root, { recursive: true, force: true }));
+  const target = path.join(root, "existing-bot", "config.json");
+  writeConfigFile(target, existingConfig());
+
+  await assert.rejects(runSetup(target, {
+    botId: "different-bot",
+    prompt: new FakePrompt(),
+    discoverAgents: async () => []
+  }), /Cannot change existing bot\.id/);
+});