diff --git a/.changelog/unreleased/fixed-593-create-remedy-unknown-row.md b/.changelog/unreleased/fixed-593-create-remedy-unknown-row.md new file mode 100644 index 00000000..d7e5c1a9 --- /dev/null +++ b/.changelog/unreleased/fixed-593-create-remedy-unknown-row.md @@ -0,0 +1,5 @@ +- **`tps agent create` names both remedy branches when a failed key read-back leaves the Agent row's existence unknown (Refs #593).** + + With the row's existence unknown, the remedy now names `flair agent remove ` + (which also deletes that agent's Memory and Soul rows) as well as + `flair agent add --keys-dir `; the definite no-row remedy is unchanged. diff --git a/packages/cli/src/commands/agent.ts b/packages/cli/src/commands/agent.ts index ecf94a0a..526c473d 100644 --- a/packages/cli/src/commands/agent.ts +++ b/packages/cli/src/commands/agent.ts @@ -251,9 +251,16 @@ function storedKeyAsHex(stored: string): string | null { return bytes.length === 32 ? bytes.toString("hex") : null; } +/** Whether an Agent row was confirmed to exist when the registration is refused. */ +type RowState = "exists" | "absent" | "unknown"; + /** * cli#512 — when Flair is reachable, `create` exits non-zero here unless the * stored key reads back equal to the generated key. + * + * cli#593 — `rowState` is three-valued. A failed read-back leaves the row's + * existence unknown, and the remedy then names both branches rather than the + * no-row one. */ function refuseRegistration( id: string, @@ -261,7 +268,7 @@ function refuseRegistration( identityDir: string, detail: string, writeError: string | null, - rowExists: boolean, + rowState: RowState, ): never { const cause = writeError ? `${detail}; the registration write failed: ${writeError}` : detail; console.error(`❌ Agent '${id}' is not registered in Flair at ${flairUrl} — ${cause}.`); @@ -269,13 +276,18 @@ function refuseRegistration( ` Flair cannot store a key generated here yet (it drops publicKey on Agent PUT/PATCH; a supported operation is flair#2266).`, ); const copy = `copy ${join(identityDir, `${id}.key`)} and ${join(identityDir, `${id}.pub`)} into the Flair host's keys dir, run \`flair agent add ${id} --keys-dir \` there, then re-run \`tps agent create\``; - if (rowExists) { + if (rowState === "exists") { console.error( ` Until then: run \`flair agent remove ${id}\` on the Flair host, then ${copy}.`, ); console.error( ` ⚠️ \`flair agent remove\` deletes the agent's Agent row, its Memory and Soul rows, and its key files in the keys dir (unless --keep-keys); registering a key on an existing pending row without deleting is flair#2266, not yet available.`, ); + } else if (rowState === "unknown") { + console.error( + ` Until then: if \`flair agent show ${id}\` finds the agent, it already exists: run \`flair agent remove ${id}\` on the Flair host first (this also deletes that agent's Memory and Soul rows), then ${copy}.`, + ); + console.error(` Otherwise: ${copy}.`); } else { console.error(` Until then: ${copy}.`); } @@ -374,14 +386,16 @@ async function createAgent(args: AgentArgs): Promise { try { stored = await flair.readStoredPublicKey(id); } catch (e) { - refuseRegistration(id, flairUrl, identityDir, `the read-back failed (${e instanceof Error ? e.message : String(e)})`, writeError, false); + // The read failed with a non-404 error: whether a row exists is unknown, so + // the remedy must name the remove branch as well as the add branch (cli#593). + refuseRegistration(id, flairUrl, identityDir, `the read-back failed (${e instanceof Error ? e.message : String(e)})`, writeError, "unknown"); } if (!stored.found) { - refuseRegistration(id, flairUrl, identityDir, "no Agent row exists", writeError, false); + refuseRegistration(id, flairUrl, identityDir, "no Agent row exists", writeError, "absent"); } else if (stored.publicKey === null || storedKeyAsHex(stored.publicKey) !== pubKeyHex) { const found = stored.publicKey === null ? "the row has no public key" : `the stored public key is '${stored.publicKey}'`; - refuseRegistration(id, flairUrl, identityDir, `${found}, not the generated key`, writeError, true); + refuseRegistration(id, flairUrl, identityDir, `${found}, not the generated key`, writeError, "exists"); } console.log(successLine ?? ` Agent '${id}' registered in Flair.`); } diff --git a/packages/cli/test/agent-create-registration.test.ts b/packages/cli/test/agent-create-registration.test.ts index 76cf6788..da2d2278 100644 --- a/packages/cli/test/agent-create-registration.test.ts +++ b/packages/cli/test/agent-create-registration.test.ts @@ -2,20 +2,18 @@ import { createPatchShared } from "./helpers/patch-shared.js"; const patchShared = createPatchShared(); /** * cli#512 — `tps agent create` reports registration only when Flair reads the - * generated key back equal. These tests drive the create path against a fake + * local key back equal. These tests drive the create path against a fake * Flair: signed calls reach the shared verifying stub (cli#554), and the * operator-credentialed calls the create flow makes — the Agent PUT and the - * read-back GET, both Basic — reach a route this file controls. Each failure - * branch (refused write, no row, `pending` stored, mismatch, failed read) and - * the success branch is covered; the real-Harper case runs under - * TPS_TEST_REAL_FLAIR=1. + * read-back GET, both Basic — reach a route this file controls. */ import { afterEach, expect, mock, spyOn, test } from "bun:test"; import { randomUUID } from "node:crypto"; -import { mkdtempSync, rmSync } from "node:fs"; -import { tmpdir } from "node:os"; +import { homedir } from "node:os"; import { join } from "node:path"; +import { format } from "node:util"; import { runAgent } from "../src/commands/agent.js"; +import { generateKeyPair, saveKeyPair } from "../src/utils/identity.js"; import { stubFlairHandler } from "./helpers/stub-flair.js"; const AGENT = "create-registration-agent"; @@ -29,28 +27,34 @@ afterEach(() => { interface FakeFlair { url: string; stop(): void; + requests: Array<{ method: string; path: string; auth: string }>; } /** * A fake Flair. A request carrying a TPS-Ed25519 header reaches the shared - * verifying stub with no registered signers — so a fresh agent's own signed + * verifying stub — so a fresh agent's own signed * read (getAgent) and seed (AgentSeed) are refused 401 `unknown_agent`, exactly * as a real Flair refuses an unregistered caller. A request carrying Basic * reaches `basic` (the Agent PUT and the read-back GET). */ -function startFakeFlair(basic: (req: Request, url: URL) => Promise): FakeFlair { - const signed = stubFlairHandler({}); +function startFakeFlair( + basic: (req: Request, url: URL) => Promise, + signers: Record = {}, +): FakeFlair { + const signed = stubFlairHandler(signers, basic); + const requests: FakeFlair["requests"] = []; const server = Bun.serve({ port: 0, async fetch(req) { const url = new URL(req.url); if (url.pathname === "/Health") return new Response("ok"); const auth = req.headers.get("Authorization") ?? ""; + requests.push({ method: req.method, path: url.pathname, auth }); if (auth.startsWith("Basic ")) return (await basic(req, url)) ?? new Response("not found", { status: 404 }); return signed(req); }, }); - return { url: `http://127.0.0.1:${server.port}`, stop: () => server.stop(true) }; + return { url: `http://127.0.0.1:${server.port}`, stop: () => server.stop(true), requests }; } /** A Basic `PUT /Agent/` that Flair refuses (it drops `publicKey`, so the write cannot complete). */ @@ -63,32 +67,56 @@ function refusePut(url: URL, id: string): Response | undefined { async function runCreate( url: string, id: string, - flags: { noSeed?: boolean } = {}, -): Promise<{ exitCode: number | undefined; stdout: string; stderr: string }> { - const home = mkdtempSync(join(tmpdir(), "create-registration-")); - const savedHome = process.env.HOME; - process.env.HOME = home; + flags: { noSeed?: boolean; credential?: string; inspectError?: boolean; keyPair?: ReturnType } = {}, + create: typeof runAgent = runAgent, +): Promise<{ exitCode: number | undefined; stdout: string; stderr: string; error: unknown; hasError: boolean }> { + const home = homedir(); + const savedCredential = process.env.FLAIR_ADMIN_AUTH; + if (flags.credential !== undefined) process.env.FLAIR_ADMIN_AUTH = flags.credential; const stdout: string[] = []; const stderr: string[] = []; - const logSpy = spyOn(console, "log").mockImplementation((...args: unknown[]) => { stdout.push(args.join(" ")); }); - const errSpy = spyOn(console, "error").mockImplementation((...args: unknown[]) => { stderr.push(args.join(" ")); }); + const consoleSpies = (["log", "info", "warn", "error"] as const).map((method) => + spyOn(console, method).mockImplementation((...args: unknown[]) => { + (method === "log" || method === "info" ? stdout : stderr).push(`${format(...args)}\n`); + }), + ); + const captureWrite = (output: string[]): typeof process.stdout.write => ( + chunk: string | Uint8Array, + encodingOrCallback?: BufferEncoding | ((error?: Error | null) => void), + callback?: (error?: Error | null) => void, + ) => { + output.push(typeof chunk === "string" ? chunk : Buffer.from(chunk).toString()); + const done = typeof encodingOrCallback === "function" ? encodingOrCallback : callback; + if (done) queueMicrotask(() => done()); + return true; + }; + const stdoutSpy = spyOn(process.stdout, "write").mockImplementation(captureWrite(stdout)); + const stderrSpy = spyOn(process.stderr, "write").mockImplementation(captureWrite(stderr)); + const syntheticExit = Symbol("runCreate exit"); let exitCode: number | undefined; const exitSpy = spyOn(process, "exit").mockImplementation(((code?: number): never => { exitCode = code; - throw new Error(`exit:${code}`); + throw syntheticExit; }) as never); + let error: unknown = null; + let hasError = false; try { - await runAgent({ action: "create", id, name: id, flairUrl: url, noSeed: flags.noSeed }); + if (flags.keyPair) saveKeyPair(flags.keyPair, join(home, ".tps", "identity"), id); + await create({ action: "create", id, name: id, flairUrl: url, noSeed: flags.noSeed }); } catch (err) { - if (!String(err).includes("exit:")) throw err; + if (err !== syntheticExit) { + if (!flags.inspectError) throw err; + error = err; + hasError = true; + } } finally { exitSpy.mockRestore(); - logSpy.mockRestore(); - errSpy.mockRestore(); - if (savedHome === undefined) delete process.env.HOME; else process.env.HOME = savedHome; - rmSync(home, { recursive: true, force: true }); + for (const spy of consoleSpies) spy.mockRestore(); + stdoutSpy.mockRestore(); + stderrSpy.mockRestore(); + if (savedCredential === undefined) delete process.env.FLAIR_ADMIN_AUTH; else process.env.FLAIR_ADMIN_AUTH = savedCredential; } - return { exitCode, stdout: stdout.join("\n"), stderr: stderr.join("\n") }; + return { exitCode, stdout: stdout.join(""), stderr: stderr.join(""), error, hasError }; } test("refused write and no Agent row: exits non-zero, prints no success, names agent, URL and remedy", async () => { @@ -152,10 +180,10 @@ test("the stored key differs from the generated one: exits non-zero", async () = } }); -test("the read-back fails: exits non-zero, never treating a failed read as absent", async () => { +test("the read-back fails: exits non-zero, never treating a failed read as absent, and names both remedy branches", async () => { const id = `${AGENT}-read-fail`; const fake = startFakeFlair(async (req, url) => { - if (req.method === "PUT") return Response.json({}, { status: 200 }); + if (req.method === "PUT") return refusePut(url, id); if (url.pathname === `/Agent/${id}`) return new Response("boom", { status: 500 }); return undefined; }); @@ -164,51 +192,59 @@ test("the read-back fails: exits non-zero, never treating a failed read as absen expect(res.exitCode).toBe(1); expect(res.stdout).not.toContain("registered in Flair"); expect(res.stderr).toContain("the read-back failed"); + // cli#593: a non-404 read failure leaves the row's existence unknown, so the + // remedy names the remove branch as well as the add branch. + expect(res.stderr).toContain(`flair agent show ${id}`); + expect(res.stderr).toContain(`flair agent remove ${id}`); + expect(res.stderr).toContain(`flair agent add ${id} --keys-dir`); + expect(res.stderr).toContain("Memory and Soul rows"); } finally { fake.stop(); } }); -test("the key reads back equal: prints success and exits zero", async () => { +test("an existing Agent with the local key: prints success without a seed or write", async () => { const id = `${AGENT}-ok`; - let stored: string | null = null; + const keyPair = generateKeyPair(); + const publicKey = Buffer.from(keyPair.signing.publicKey).toString("base64url"); + const row = { id, name: id, publicKey, createdAt: keyPair.createdAt }; const fake = startFakeFlair(async (req, url) => { - if (url.pathname === `/Agent/${id}` && req.method === "PUT") { - const body = (await req.json()) as { publicKey?: string }; - stored = body.publicKey ?? null; - return Response.json({}, { status: 200 }); - } - if (url.pathname === `/Agent/${id}` && stored !== null) return Response.json({ id, name: id, publicKey: stored }); + if (req.method === "PUT" || req.method === "PATCH") return refusePut(url, id); + if (req.method === "GET" && url.pathname === `/Agent/${id}`) return Response.json(row); return undefined; - }); + }, { [id]: publicKey }); try { - const res = await runCreate(fake.url, id); + const res = await runCreate(fake.url, id, { keyPair, inspectError: true }); expect(res.exitCode).toBeUndefined(); - expect(res.stdout).toContain("registered in Flair (no seed"); + expect(res.hasError).toBe(false); + expect(res.error).toBeNull(); + expect(res.stdout).toContain("already registered in Flair"); expect(res.stderr).toBe(""); - expect(stored).toMatch(/^[a-f0-9]{64}$/); + expect(fake.requests.map(({ method, path }) => ({ method, path }))).toEqual([ + { method: "GET", path: `/Agent/${id}` }, + { method: "GET", path: `/Agent/${id}` }, + ]); + expect(fake.requests[0]!.auth).toStartWith(`TPS-Ed25519 ${id}:`); + expect(row.publicKey).toBe(publicKey); } finally { fake.stop(); } }); -/** A fake whose Agent row holds `encode()` */ async function runWithStoredEncoding( id: string, encode: (hexKey: string) => string, ): Promise<{ exitCode: number | undefined; stdout: string; stderr: string }> { - let stored: string | null = null; + const keyPair = generateKeyPair(); + const publicKey = encode(Buffer.from(keyPair.signing.publicKey).toString("hex")); + const row = { id, name: id, publicKey, createdAt: keyPair.createdAt }; const fake = startFakeFlair(async (req, url) => { - if (url.pathname === `/Agent/${id}` && req.method === "PUT") { - const body = (await req.json()) as { publicKey?: string }; - stored = body.publicKey ? encode(body.publicKey) : null; - return Response.json({}, { status: 200 }); - } - if (url.pathname === `/Agent/${id}` && stored !== null) return Response.json({ id, name: id, publicKey: stored }); + if (req.method === "PUT" || req.method === "PATCH") return refusePut(url, id); + if (req.method === "GET" && url.pathname === `/Agent/${id}`) return Response.json(row); return undefined; - }); + }, { [id]: /^[a-f0-9]{64}$/.test(publicKey) ? Buffer.from(publicKey, "hex").toString("base64") : publicKey }); try { - return await runCreate(fake.url, id); + return await runCreate(fake.url, id, { keyPair }); } finally { fake.stop(); } @@ -216,7 +252,7 @@ async function runWithStoredEncoding( const b64url = (hex: string) => Buffer.from(hex, "hex").toString("base64url"); -test("the row holds the base64url of the generated key (what flair stores): exits zero", async () => { +test("the row holds the base64url of the local key: exits zero", async () => { const res = await runWithStoredEncoding(`${AGENT}-b64-equal`, b64url); expect(res.exitCode).toBeUndefined(); expect(res.stdout).toContain("registered in Flair"); @@ -229,7 +265,7 @@ test("the row holds the base64url of a different key: exits non-zero", async () expect(res.stderr).toContain("not the generated key"); }); -test("the row holds the hex of the generated key (flair accepts either): exits zero", async () => { +test("the row holds the hex of the local key: exits zero", async () => { const res = await runWithStoredEncoding(`${AGENT}-hex-equal`, (hex) => hex); expect(res.exitCode).toBeUndefined(); }); @@ -275,3 +311,176 @@ test.skipIf(process.env.TPS_TEST_REAL_FLAIR !== "1")( }, 30_000, ); + +test.skipIf(process.env.TPS_TEST_REAL_FLAIR !== "1")( + "against a real Flair, create succeeds with an existing Agent and matching local key", + async () => { + const baseUrl = process.env.TPS_TEST_FLAIR_URL ?? "http://127.0.0.1:9926"; + const opsUrl = process.env.TPS_TEST_FLAIR_OPS_URL ?? "http://127.0.0.1:9925"; + const admin = process.env.TPS_TEST_FLAIR_ADMIN ?? "admin:test123"; + const id = `create-existing-${randomUUID()}`; + const keyPair = generateKeyPair(); + const publicKey = Buffer.from(keyPair.signing.publicKey).toString("base64url"); + const wire = Buffer.from(admin).toString("base64"); + const headers = { Authorization: `Basic ${wire}`, "Content-Type": "application/json" }; + const inserted = await fetch(opsUrl, { + method: "POST", headers, + body: JSON.stringify({ + operation: "insert", database: "flair", table: "Agent", + records: [{ id, name: id, publicKey, createdAt: keyPair.createdAt }], + }), + }); + try { + expect(inserted.ok).toBe(true); + const res = await runCreate(baseUrl, id, { keyPair, credential: admin, inspectError: true }); + expect(res.exitCode).toBeUndefined(); + expect(res.hasError).toBe(false); + expect(res.error).toBeNull(); + expect(res.stdout).toContain("already registered in Flair"); + expect(res.stderr).toBe(""); + for (const stream of [res.stdout, res.stderr, format(res.error)]) { + expect(stream).not.toContain(admin); + expect(stream).not.toContain(wire); + } + const record = await fetch(`${baseUrl}/Agent/${id}`, { headers }); + expect(record.ok).toBe(true); + expect((await record.json() as { publicKey: string }).publicKey).toBe(publicKey); + } finally { + const deleted = await fetch(opsUrl, { + method: "POST", headers, + body: JSON.stringify({ operation: "delete", database: "flair", table: "Agent", hash_values: [id] }), + }); + expect(deleted.ok).toBe(true); + } + }, + 30_000, +); + +test("selected create cases omit the credential marker and Basic encoding from captured output and errors", async () => { + const marker = `credential-593-${randomUUID()}-${"z".repeat(40)}`; + const credential = `operator:${marker}`; + const wire = Buffer.from(credential).toString("base64"); + + const fakes: FakeFlair[] = []; + const make = (basic: (req: Request, url: URL) => Promise): FakeFlair => { + const fake = startFakeFlair(basic); + fakes.push(fake); + return fake; + }; + + const noRowId = `${AGENT}-cred-no-row`; + const noRow = make(async (req, url) => (req.method === "PUT" ? refusePut(url, noRowId) : undefined)); + + const noSeedId = `${AGENT}-cred-no-seed`; + const noSeed = make(async (req, url) => (req.method === "PUT" ? refusePut(url, noSeedId) : undefined)); + + const readFailId = `${AGENT}-cred-read-fail`; + const readFail = make(async (req, url) => { + if (req.method === "PUT") return refusePut(url, readFailId); + if (url.pathname === `/Agent/${readFailId}`) return new Response("boom", { status: 500 }); + return undefined; + }); + + const noKeyId = `${AGENT}-cred-no-key`; + const noKey = make(async (req, url) => { + if (req.method === "PUT") return refusePut(url, noKeyId); + if (url.pathname === `/Agent/${noKeyId}`) return Response.json({ id: noKeyId, name: noKeyId }); + return undefined; + }); + + const differId = `${AGENT}-cred-differ`; + const differ = make(async (req, url) => { + if (req.method === "PUT") return refusePut(url, differId); + if (url.pathname === `/Agent/${differId}`) return Response.json({ id: differId, name: differId, publicKey: "de".repeat(32) }); + return undefined; + }); + + const okId = `${AGENT}-cred-ok`; + const okKeyPair = generateKeyPair(); + const okPublicKey = Buffer.from(okKeyPair.signing.publicKey).toString("base64url"); + const okRow = { id: okId, name: okId, publicKey: okPublicKey, createdAt: okKeyPair.createdAt }; + const ok = startFakeFlair(async (req, url) => { + if (req.method === "PUT" || req.method === "PATCH") return refusePut(url, okId); + if (req.method === "GET" && url.pathname === `/Agent/${okId}`) return Response.json(okRow); + return undefined; + }, { [okId]: okPublicKey }); + fakes.push(ok); + + const paths = [ + { name: "missing id", url: "http://127.0.0.1:1", id: "", exitCode: 1, message: "Usage: tps agent create" }, + { name: "refused write over no row", fake: noRow, id: noRowId, exitCode: 1, message: "no Agent row exists", seed: true }, + { name: "refused write, --no-seed", fake: noSeed, id: noSeedId, noSeed: true, exitCode: 1, message: "no Agent row exists", seed: false }, + { name: "failed read-back", fake: readFail, id: readFailId, exitCode: 1, message: "the read-back failed", seed: true }, + { name: "row holds no key", fake: noKey, id: noKeyId, exitCode: 1, message: "the row has no public key", seed: true }, + { name: "row holds a different key", fake: differ, id: differId, exitCode: 1, message: "not the generated key", seed: true }, + { name: "success", fake: ok, id: okId, keyPair: okKeyPair, exitCode: undefined, message: "already registered in Flair", seed: false }, + ]; + + try { + for (const path of paths) { + const res = await runCreate(path.fake?.url ?? path.url!, path.id, { credential, noSeed: path.noSeed, inspectError: true, keyPair: path.keyPair }); + for (const stream of [res.stdout, res.stderr, format(res.error)]) { + expect(`${path.name}: ${stream}`).not.toContain(marker); + expect(`${path.name}: ${stream}`).not.toContain(wire); + } + expect(res.hasError).toBe(false); + expect(res.error).toBeNull(); + expect(res.exitCode).toBe(path.exitCode); + expect(path.exitCode === 1 ? res.stderr : res.stdout).toContain(path.message); + if (path.exitCode === 1) expect(res.stdout).not.toContain("registered in Flair"); + else { + expect(res.stderr).toBe(""); + expect(res.stdout).toContain("Config written to"); + expect(okRow.publicKey).toBe(okPublicKey); + expect(ok.requests.map(({ method, path }) => ({ method, path }))).toEqual([ + { method: "GET", path: `/Agent/${okId}` }, + { method: "GET", path: `/Agent/${okId}` }, + ]); + expect(ok.requests[0]!.auth).toStartWith(`TPS-Ed25519 ${okId}:`); + } + if (path.fake) { + const basic = path.fake.requests.filter((req) => req.auth.startsWith("Basic ")); + expect(basic).toEqual([ + ...(path.keyPair ? [] : [{ method: "PUT", path: `/Agent/${path.id}`, auth: `Basic ${wire}` }]), + { method: "GET", path: `/Agent/${path.id}`, auth: `Basic ${wire}` }, + ]); + expect(path.fake.requests.some((req) => req.method === "POST" && req.path === "/AgentSeed/")).toBe(path.seed); + } + } + } finally { + for (const fake of fakes) fake.stop(); + } +}); + +test("runCreate captures console methods and direct stream writes", async () => { + const res = await runCreate("", "", {}, async () => { + console.log("log"); + console.info("info"); + console.warn("warn"); + console.error("error"); + await new Promise((resolve) => process.stdout.write(Buffer.from("stdout"), resolve)); + await new Promise((resolve) => process.stderr.write("stderr", "utf8", resolve)); + }); + expect(res.stdout).toBe("log\ninfo\nstdout"); + expect(res.stderr).toBe("warn\nerror\nstderr"); + expect(res.exitCode).toBeUndefined(); + expect(res.error).toBeNull(); +}); + +test("runCreate retains non-exit thrown values by identity when inspected", async () => { + for (const error of [new Error("credential exit:1"), new Error("early failure"), null, undefined]) { + const res = await runCreate("", "", { inspectError: true }, async () => { throw error; }); + expect(res.hasError).toBe(true); + expect(res.error).toBe(error); + expect(res.exitCode).toBeUndefined(); + } +}); + +test("runCreate rejects unexpected errors for callers without inspection", async () => { + for (const error of [new Error("credential exit:1"), new Error("early failure"), null, undefined]) { + await expect(runCreate("", "", {}, async () => { throw error; })).rejects.toBe(error); + } + const res = await runCreate("", "", {}, async () => { process.exit(1); }); + expect(res.exitCode).toBe(1); + expect(res.error).toBeNull(); +});