diff --git a/README.md b/README.md index 944b5639..374b5e36 100644 --- a/README.md +++ b/README.md @@ -188,10 +188,15 @@ commands — the server renews its own session's lease on a timer, so an agent never has to ask it to. Start it with `simlock mcp` — it reserves stdout for MCP JSON-RPC, so lease results never mix with protocol framing. -`SIMLOCK_AGENT_ID` sets the server's stable requester identity. Simlock -allows at most one active lease per identity, so give each agent session a -distinct, stable id — run one MCP server process per agent session, each -with its own id. +Simlock allows at most one active lease per requester identity, so each agent +session needs a distinct, stable id. Under Claude Code the server takes it +from the agent session (`claude-code:`), the same id the CLI uses there, +with no setup. Codex does not pass its session id to MCP servers, so under +Codex the server uses a pid-derived id that differs from the CLI's `codex:` +in the same session; use one interface per session, or set the same +`SIMLOCK_AGENT_ID` for both. Under other clients, set +`SIMLOCK_AGENT_ID` as in the config above and run one MCP server process per +agent session, each with its own id. `SIMLOCK_AGENT_ID` always wins when set. The server exposes exactly four tools: `list_devices` (read-only catalog of what can be leased), `lease_simulator`, `release_simulator`, and `lease_status` diff --git a/docs/CLI.md b/docs/CLI.md index b2046b95..f4cd00a6 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -96,22 +96,33 @@ implement them: ## Agent identity Leases are keyed by requester: at most one active lease per agent id, -enforced by the daemon. Give each agent session a **stable** -id so this actually constrains anything — a fresh id on every CLI invocation -(the default) makes the constraint a no-op, since every invocation looks -like a different requester. +enforced by the daemon. Each agent session needs a **stable** id for this to +constrain anything — a fresh id on every CLI invocation makes the constraint a +no-op, since every invocation looks like a different requester. Resolution order, first match wins: 1. `--agent-id ` on `simlock lease`. 2. the `SIMLOCK_AGENT_ID` environment variable. -3. a pid-derived value (today's behavior; not stable across invocations). - -Reuse the same id across an agent's own invocations (e.g. export -`SIMLOCK_AGENT_ID` once per agent session) and use a distinct id per agent so -they don't collide with each other. The id shows up as the requester in -`simlock status` and `simlock list --leases`, so an operator can tell which -agent holds what. +3. the session id of the agent tool Simlock runs under, as `:`: + + | Tool | Variable | Id | + | ----------- | ------------------------ | ------------------ | + | Claude Code | `CLAUDE_CODE_SESSION_ID` | `claude-code:` | + | Codex | `CODEX_SESSION_ID` | `codex:` | + + The first variable in the table that is set and not empty wins. +4. a pid-derived value (not stable across invocations). + +Under Claude Code or Codex nothing needs setting for the CLI: every `simlock` +command in one agent session, including its sub-agents and parallel commands, +is the same requester and shares that session's one lease. (`simlock mcp` gets +the session id under Claude Code only; see [`simlock mcp`](#simlock-mcp).) +Elsewhere, export +`SIMLOCK_AGENT_ID` once per agent session, with a distinct id per agent so +they don't collide. An explicit id always wins over the session id. The id +shows up as the requester in `simlock status` and `simlock list --leases`, so +an operator can tell which agent holds what. ## `simlock lease` @@ -134,8 +145,8 @@ granted. - `--platform`, `--device` — required. `--os` defaults to the newest runtime already installed for that platform. - `--agent-id` — this invocation's requester identity; see - [Agent identity](#agent-identity). Defaults to `SIMLOCK_AGENT_ID`, then a - pid-derived value. + [Agent identity](#agent-identity). Defaults to `SIMLOCK_AGENT_ID`, then the + agent tool's session id, then a pid-derived value. - `--timeout` — max time to wait in the queue (exit 10 on expiry). - `--no-wait` — fail immediately with exit 11 instead of queueing. - `--allow-download` — permit downloading a missing runtime / system image @@ -805,10 +816,17 @@ relayed as MCP `notifications/progress` for that request. See [../README.md](../README.md#mcp-integration-optional) for details. The requester identity for leases made through this server is -`SIMLOCK_AGENT_ID`, falling back to a pid-derived value — see -[Agent identity](#agent-identity). Set a distinct `SIMLOCK_AGENT_ID` per MCP -server process (one per agent session) so the one-lease-per-agent rule is -meaningful. +`SIMLOCK_AGENT_ID`, then the agent tool's session id, then a pid-derived +value — see [Agent identity](#agent-identity). Under Claude Code the server +gets the session's id with no setup, the same id the CLI resolves in that +session. Codex starts MCP servers without its own environment variables, so +under Codex the server cannot see the session id: it falls back to a +pid-derived requester, which differs from the id the CLI resolves in the same +Codex session. An agent that leases through both the CLI and MCP in one Codex +session can therefore hold two leases. To avoid that, use one interface per +session, or set the same `SIMLOCK_AGENT_ID` for both. Under any other client, +set a distinct `SIMLOCK_AGENT_ID` per MCP server process (one per agent +session) so the one-lease-per-agent rule is meaningful. ### Breaking in 0.3.0: tool schemas are now the contract's own field names @@ -1157,7 +1175,7 @@ connection's resolved `role` so a caller can tell which one it got. This is also why `simlock lease --detach` followed later by `simlock lease renew ` or `simlock release ` from a -different invocation works even though each CLI process has a different +different invocation works even when each CLI process has a different pid-derived identity: all of them connect as admin (when the local file is readable), and admin bypasses the per-connection ownership check that would otherwise apply. diff --git a/docs/internal/KNOWN-PITFALLS.md b/docs/internal/KNOWN-PITFALLS.md index cf35fff4..8048503f 100644 --- a/docs/internal/KNOWN-PITFALLS.md +++ b/docs/internal/KNOWN-PITFALLS.md @@ -1,5 +1,30 @@ # Known pitfalls +## `simlock mcp` under Codex cannot see the session id + +The requester id comes from the agent tool's session variable +(`CLAUDE_CODE_SESSION_ID`, `CODEX_SESSION_ID`). The CLI runs in the agent's +shell, so it sees them. + +**The pitfall:** Codex starts MCP servers with its own `CODEX_*` variables +stripped (verified against Codex 0.159.0 with a stub server that printed its +`CODEX_*` environment: empty). Claude Code passes its variable through. MCP +stdio servers inherit only a limited environment by contract, and MCP has no +session id for stdio, so this is client behaviour we cannot rely on. Under +Codex, `simlock mcp` falls back to `mcp:`, so an agent using both the CLI +(`codex:`) and MCP in one session is two requesters and can hold two +leases. The daemon still enforces one lease per requester on each side. + +**Why it is accepted:** mixing the CLI and MCP in one agent session is +uncommon, and the id is only a per-requester key, so nothing collides across +agents. Removing MCP would cost a supported interface for a narrow gap. + +**The fix, if needed:** an optional `agentId` argument on `lease_simulator`, +which the agent fills from its shell's session variable. This matches MCP's +direction that cross-request identity be an explicit identifier the client +passes. Until then, use one interface per session or set the same +`SIMLOCK_AGENT_ID` for both. + ## A SIGKILLed lease holder keeps its device until the TTL expires A lease ends in exactly one of two ways: somebody releases it, or its TTL diff --git a/src/agent-identity/index.test.ts b/src/agent-identity/index.test.ts new file mode 100644 index 00000000..a965c5b1 --- /dev/null +++ b/src/agent-identity/index.test.ts @@ -0,0 +1,40 @@ +import { describe, expect, it } from "vitest"; + +import { resolveRequesterId } from "./index.js"; + +describe("resolveRequesterId", () => { + it("returns SIMLOCK_AGENT_ID unchanged when it is defined, even with a Claude Code session id also set", () => { + expect( + resolveRequesterId( + { SIMLOCK_AGENT_ID: "agent-7", CLAUDE_CODE_SESSION_ID: "abc" }, + "fallback", + ), + ).toBe("agent-7"); + }); + + it("returns claude-code: from CLAUDE_CODE_SESSION_ID when SIMLOCK_AGENT_ID is unset", () => { + expect(resolveRequesterId({ CLAUDE_CODE_SESSION_ID: "abc" }, "fallback")).toBe( + "claude-code:abc", + ); + }); + + it("returns codex: from CODEX_SESSION_ID when SIMLOCK_AGENT_ID is unset", () => { + expect(resolveRequesterId({ CODEX_SESSION_ID: "xyz" }, "fallback")).toBe("codex:xyz"); + }); + + it("takes CLAUDE_CODE_SESSION_ID over CODEX_SESSION_ID when both are set", () => { + expect( + resolveRequesterId({ CLAUDE_CODE_SESSION_ID: "abc", CODEX_SESSION_ID: "xyz" }, "fallback"), + ).toBe("claude-code:abc"); + }); + + it("skips a session variable set to the empty string and takes the next row", () => { + expect( + resolveRequesterId({ CLAUDE_CODE_SESSION_ID: "", CODEX_SESSION_ID: "xyz" }, "fallback"), + ).toBe("codex:xyz"); + }); + + it("returns the caller's fallback when no variable is set", () => { + expect(resolveRequesterId({}, "fallback")).toBe("fallback"); + }); +}); diff --git a/src/agent-identity/index.ts b/src/agent-identity/index.ts new file mode 100644 index 00000000..43fdd9b0 --- /dev/null +++ b/src/agent-identity/index.ts @@ -0,0 +1,36 @@ +/** + * The default requester id a frontend declares when the caller names none. Both the CLI and + * `simlock mcp` call this one function, so the two cannot drift apart (architecture rule 10). + * Resolution happens in the frontend; the daemon only sees the id it is sent. + * + * Order, first match wins: + * + * 1. `SIMLOCK_AGENT_ID`, when defined -- unchanged from before session detection existed. + * 2. The first row of `AGENT_SESSION_VARIABLES` whose variable is set and not empty, as + * `:`. Every command and sub-agent in one agent session shares that session's + * id, and so its one lease. + * 3. `fallback`, the frontend's own pid-derived value. + * + * The session value gets no more checking than `SIMLOCK_AGENT_ID` does: it is passed through + * as-is, and only the empty string is skipped. + */ + +/** The agent tools whose session id Simlock reads, in the order they are tried. Adding a tool + * is one row here and one test. */ +// fallow-ignore-next-line unused-export -- the spec'd public table; exported so the supported tools are readable in one place. +export const AGENT_SESSION_VARIABLES: readonly { + readonly tool: string; + readonly variable: string; +}[] = [ + { tool: "claude-code", variable: "CLAUDE_CODE_SESSION_ID" }, + { tool: "codex", variable: "CODEX_SESSION_ID" }, +]; + +export function resolveRequesterId(env: NodeJS.ProcessEnv, fallback: string): string { + if (env.SIMLOCK_AGENT_ID !== undefined) return env.SIMLOCK_AGENT_ID; + for (const { tool, variable } of AGENT_SESSION_VARIABLES) { + const sessionId = env[variable]; + if (sessionId !== undefined && sessionId !== "") return `${tool}:${sessionId}`; + } + return fallback; +} diff --git a/src/cli/index.test.ts b/src/cli/index.test.ts index 61a76d22..b7ddaed2 100644 --- a/src/cli/index.test.ts +++ b/src/cli/index.test.ts @@ -47,7 +47,6 @@ import { RELEASE_TIMEOUT_MS } from "../lease-policy/index.js"; import { buildCliEnvironment, errorExitCode, - fallbackRequesterId, parseDuration, readLogFile, readPipedStdin, @@ -2863,12 +2862,68 @@ describe("CLI: a SIMLOCK_HOME the kernel could not bind", () => { }); }); -describe("CLI: pure helpers", () => { - it("fallbackRequesterId prefers SIMLOCK_AGENT_ID over a pid-derived default", () => { - expect(fallbackRequesterId({ SIMLOCK_AGENT_ID: "agent-7" })).toBe("agent-7"); - expect(fallbackRequesterId({})).toBe(String(process.pid)); +describe("CLI: requester id from the agent session", () => { + const sessionEnv = { CLAUDE_CODE_SESSION_ID: "abc" }; + + it("buildCliEnvironment resolves requesterId to the session-derived id when SIMLOCK_AGENT_ID is unset", () => { + expect(buildCliEnvironment(realCliEnvironmentPorts(), sessionEnv).requesterId).toBe( + "claude-code:abc", + ); }); + it("buildCliEnvironment prefers SIMLOCK_AGENT_ID over a session-derived id", () => { + expect( + buildCliEnvironment(realCliEnvironmentPorts(), { ...sessionEnv, SIMLOCK_AGENT_ID: "agent-7" }) + .requesterId, + ).toBe("agent-7"); + }); + + it("buildCliEnvironment falls back to the pid when no agent id or session id is set", () => { + expect(buildCliEnvironment(realCliEnvironmentPorts(), {}).requesterId).toBe( + String(process.pid), + ); + }); + + it("simlock lease --agent-id overrides a session-derived id", async () => { + const requested: string[] = []; + const client = fakeClient({ + requestLease: (input, _options) => { + requested.push(input.requesterId ?? ""); + return fakeClient().requestLease(input); + }, + }); + const output = outputCapture(); + const environment = { + ...buildCliEnvironment(realCliEnvironmentPorts(), sessionEnv), + connectAdmin: async () => client, + stderr: { write: (value: string) => (output.stderr += value) }, + stdout: { write: (value: string) => (output.stdout += value) }, + }; + + await expect( + runCli(["lease", "--platform", "ios", "--device", "iPhone 17 Pro", "--detach"], environment), + ).resolves.toBe(0); + await expect( + runCli( + [ + "lease", + "--platform", + "ios", + "--device", + "iPhone 17 Pro", + "--detach", + "--agent-id", + "explicit-agent", + ], + environment, + ), + ).resolves.toBe(0); + + expect(requested).toEqual(["claude-code:abc", "explicit-agent"]); + }); +}); + +describe("CLI: pure helpers", () => { it("parseDuration parses units and rejects garbage", () => { expect(parseDuration("500")).toBe(500); expect(parseDuration("500ms")).toBe(500); diff --git a/src/cli/index.ts b/src/cli/index.ts index f2dff4e9..1ea88ae0 100644 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -24,6 +24,7 @@ import { type ParentWatchHandle, type SystemStats, } from "../ports/index.js"; +import { resolveRequesterId } from "../agent-identity/index.js"; import { connectSimlockAdmin } from "../admin/index.js"; import { isSimlockError, @@ -124,7 +125,8 @@ export interface CliEnvironment { */ readonly clock: Clock; readonly configPath: string; - /** ADR §4's requester default and this connection's fixed principal -- see §9's + /** ADR §4's requester default and this connection's fixed principal: `SIMLOCK_AGENT_ID`, else + * the agent tool's session id, else the pid (`resolveRequesterId`). See §9's * "SIMLOCK_AGENT_ID and --agent-id still set the requester id ... they are not the * principal": `--agent-id` overrides `lease.request`'s `requesterId` field, never this. */ readonly requesterId: string; @@ -186,16 +188,6 @@ export interface CliEnvironment { readonly readStdin?: () => Promise; } -/** - * Resolves the fallback requester identity from the environment: `SIMLOCK_AGENT_ID` - * when set, else a pid-derived value so callers that never configure a stable id - * keep today's behavior. The per-invocation `--agent-id` flag on `lease` (parsed at - * that command's own boundary) takes precedence over this default. - */ -export function fallbackRequesterId(env: NodeJS.ProcessEnv): string { - return env.SIMLOCK_AGENT_ID ?? String(process.pid); -} - /** * ADR §5: "a daemon still writing the file" is a real race between the daemon claiming its * socket (reachable) and `admin.token` landing on disk (`DaemonServer#start` awaits the socket @@ -377,7 +369,10 @@ export function buildCliEnvironment( const configPath = join(dataDirectory, "config.json"); const logPath = join(dataDirectory, "daemon.log"); const adminTokenPath = join(dataDirectory, "admin.token"); - const requesterId = fallbackRequesterId(env); + // `SIMLOCK_AGENT_ID`, else the agent tool's session id, else this process's pid. The + // per-invocation `--agent-id` flag on `lease` (parsed at that command's own boundary) takes + // precedence over this default. + const requesterId = resolveRequesterId(env, String(process.pid)); const autoLaunchIpc = new AutoLaunchIpcConnector(ipc, clock, launcher); // ADR §5 / B2: the raw connection (and, for `connectAdmin`, any auto-launch it triggers) is @@ -823,8 +818,8 @@ function extractLeaseFlag(args: readonly string[]): { * whenever the requester id differs from the principal (an `--agent-id` used on the lease but * not on this invocation): a refusal with no safety behind it. * - * Identity rather than a flag, in either case: `--agent-id`/`SIMLOCK_AGENT_ID` already names - * who is asking, `simlock lease` attributes a lease to it, and one requester holds at most one + * Identity rather than a flag, in either case: `--agent-id`, `SIMLOCK_AGENT_ID` or the agent + * session's id already names who is asking, `simlock lease` attributes a lease to it, and one requester holds at most one * lease -- so the usual invocation needs nothing, and `--lease` is there for the rest. */ async function resolveRemoteLeaseId( diff --git a/src/mcp/main.test.ts b/src/mcp/main.test.ts index 929c395b..e5dd8294 100644 --- a/src/mcp/main.test.ts +++ b/src/mcp/main.test.ts @@ -4,7 +4,14 @@ import { InMemoryTransport } from "@modelcontextprotocol/sdk/inMemory.js"; import { CallToolResultSchema } from "@modelcontextprotocol/sdk/types.js"; import { describe, expect, it, vi } from "vitest"; -import { FakeClock } from "../ports/index.js"; +import { buildCliEnvironment } from "../cli/index.js"; +import { + FakeClock, + FakeDaemonLauncher, + FakeSystemStats, + MemoryFilesystem, + MemoryIpcTransport, +} from "../ports/index.js"; import { FakeSimlockClient, sampleGrant } from "./test-support.js"; import { startMcpStdio, type McpTransport } from "./main.js"; @@ -197,6 +204,43 @@ describe("MCP stdio lifecycle", () => { await runner.shutdown(); }); + it("sources the connection principal as claude-code: from CLAUDE_CODE_SESSION_ID when neither requesterId nor SIMLOCK_AGENT_ID is given", async () => { + await expect(principalFor({ env: { CLAUDE_CODE_SESSION_ID: "abc" } })).resolves.toBe( + "claude-code:abc", + ); + }); + + it("falls back to mcp: when no requesterId, SIMLOCK_AGENT_ID or session id is given", async () => { + await expect(principalFor({ env: {} })).resolves.toBe(`mcp:${process.pid}`); + }); + + it("prefers an explicit requesterId over a session-derived id", async () => { + await expect( + principalFor({ env: { CLAUDE_CODE_SESSION_ID: "abc" }, requesterId: "explicit-agent" }), + ).resolves.toBe("explicit-agent"); + }); + + it.each([{ CLAUDE_CODE_SESSION_ID: "abc" }, { CODEX_SESSION_ID: "xyz" }])( + "the CLI and simlock mcp resolve the same session-derived requester id from the same environment (%o)", + async (env) => { + const cli = buildCliEnvironment( + { + clock: new FakeClock(0), + dataDirectory: "/simlock", + filesystem: new MemoryFilesystem(), + ipc: new MemoryIpcTransport(), + launcher: new FakeDaemonLauncher(), + systemStats: new FakeSystemStats({ cpuCount: 1, freeRamBytes: 1, totalRamBytes: 1 }), + }, + env, + ); + const expected = "CLAUDE_CODE_SESSION_ID" in env ? "claude-code:abc" : "codex:xyz"; + + expect(cli.requesterId).toBe(expected); + await expect(principalFor({ env })).resolves.toBe(cli.requesterId); + }, + ); + it("closes once when stdin reaches EOF", async () => { const transport = new FakeTransport(); const server = new FakeServer(); @@ -244,6 +288,32 @@ describe("MCP stdio lifecycle", () => { }); }); +/** The principal `startMcpStdio` connects under, read off the first daemon connection it makes. */ +async function principalFor(options: { + readonly env: NodeJS.ProcessEnv; + readonly requesterId?: string; +}): Promise { + connectWithAutoLaunch.mockReset().mockResolvedValue(new FakeSimlockClient()); + const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); + const runner = await startMcpStdio({ + createTransport: () => serverTransport, + signals: new FakeSignals(), + ...options, + }); + const mcpClient = new Client({ name: "test", version: "1.0.0" }); + await mcpClient.connect(clientTransport); + await mcpClient + .request( + { method: "tools/call", params: { arguments: {}, name: "lease_status" } }, + CallToolResultSchema, + ) + .catch(() => undefined); + await mcpClient.close(); + await runner.shutdown(); + const [connectOptions] = connectWithAutoLaunch.mock.calls[0] ?? []; + return (connectOptions as { readonly principal?: unknown } | undefined)?.principal; +} + class FakeServer { closeCalls = 0; connectCalls = 0; diff --git a/src/mcp/main.ts b/src/mcp/main.ts index dc4b4993..4a3c23fd 100644 --- a/src/mcp/main.ts +++ b/src/mcp/main.ts @@ -3,6 +3,7 @@ import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { dirname, join } from "node:path"; import { fileURLToPath } from "node:url"; +import { resolveRequesterId } from "../agent-identity/index.js"; import type { SimlockClient } from "../client/index.js"; import { NodeDaemonLauncher, @@ -43,7 +44,8 @@ export interface McpStdioEnvironment { readonly connectForRenew?: () => Promise; readonly createServer?: (session: McpSession) => McpServer; readonly createTransport?: () => McpTransport; - /** Source for `SIMLOCK_AGENT_ID` when `requesterId` is not given explicitly. */ + /** Source for `SIMLOCK_AGENT_ID` and the agent tool's session id (`resolveRequesterId`) when + * `requesterId` is not given explicitly. */ readonly env?: NodeJS.ProcessEnv; readonly requesterId?: string; readonly signals?: Signals; @@ -71,7 +73,7 @@ export async function startMcpStdio( environment: McpStdioEnvironment = {}, ): Promise { const env = environment.env ?? process.env; - const requesterId = environment.requesterId ?? env.SIMLOCK_AGENT_ID ?? `mcp:${process.pid}`; + const requesterId = environment.requesterId ?? resolveRequesterId(env, `mcp:${process.pid}`); // One `Clock` for the whole frontend: the session's renew timer and the auto-launch retry // loop must not be able to disagree about what time it is (architecture rule 9). const clock = environment.clock ?? new SystemClock(); diff --git a/src/mcp/session.ts b/src/mcp/session.ts index c07fc140..e83de20e 100644 --- a/src/mcp/session.ts +++ b/src/mcp/session.ts @@ -262,7 +262,7 @@ export class McpSession { * call (ADR 0003 §9), never a cache of lease state. `lease.list` filters by **owner principal * only** (see `src/daemon/dispatcher.ts`'s `lease.list` handler), so it can return leases * this connection never requested: one a `simlock lease --detach` left behind under the same - * `SIMLOCK_AGENT_ID` principal, or one left over from an earlier session under that + * principal (`SIMLOCK_AGENT_ID` or the agent session's id), or one left over from an earlier session under that * principal. Taking `leases[0]` unconditionally would report a lease this session neither * renews nor will release on close. Matching on `id === #heldLeaseId` scopes the answer to * the one lease this session's own `lease()` call actually obtained -- the only filter left,