From 10cca1166b57168e41628f242d86dbc6d906e920 Mon Sep 17 00:00:00 2001 From: Austin Kurpuis Date: Sat, 25 Jul 2026 17:09:06 -0700 Subject: [PATCH 1/6] Let a linked Claude credential be moved between subjects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-keying where a credential is READ from does not move the credential. Records written before principals existed (ADR 0029/0030 §6) sit under the entry point's own subject, so converging the two entry points would still have charged every existing user a fresh login to reproduce a credential the gateway is holding the whole time. Adds the capability that makes that unnecessary: `ClaudeTokenStore.rekey` and a bearer-gated `POST /claude-auth/api/rekey`, plus `IdentityLinkPort.rekey` and both claude clients' side of it. No caller yet -- the pre-flight change that uses it is the next commit. Deliberate properties, each with a test: - MOVE, not copy. The claude-remote write-back only ever writes the new key, so a leftover copy would rot and then fail whichever flow still read it with "Login expired" -- worse than either alternative. - Never overwrites the destination: a record already there is by definition at least as current as the one being moved. - Deletes the source only after reading the destination back. `set` swallows its own Redis errors by design, so an unverified delete could drop a human's only credential on a transient failure. - A self-move is a no-op rather than a delete-what-was-just-written. - Bearer-gated like `token` and `invalidate`, which already hand out and destroy any subject's credential -- it grants no new authority over the keyspace. Authorizing that both subjects are the same human is the caller's job, and the route says so. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01W7XfNb5HsmaZH7QyxUrbyB --- .../claude-auth-gateway-client.ts | 27 ++++++ .../claude-remote-gateway-client.ts | 27 ++++++ .../src/identity-link/gateway-client.ts | 19 +++++ .../src/claude-auth/api.test.ts | 83 +++++++++++++++++++ .../src/claude-auth/api.ts | 46 ++++++++++ .../src/claude-auth/store.test.ts | 63 ++++++++++++++ .../src/claude-auth/store.ts | 47 +++++++++++ 7 files changed, 312 insertions(+) diff --git a/apps/agent-orchestrator/src/identity-link/claude-auth-gateway-client.ts b/apps/agent-orchestrator/src/identity-link/claude-auth-gateway-client.ts index f42027c..a5ee68c 100644 --- a/apps/agent-orchestrator/src/identity-link/claude-auth-gateway-client.ts +++ b/apps/agent-orchestrator/src/identity-link/claude-auth-gateway-client.ts @@ -83,4 +83,31 @@ export class ClaudeAuthGatewayClient implements IdentityLinkPort { throw new Error(`claude-auth invalidate failed: ${res.status} ${await res.text()}`); } } + + /** + * Best-effort by design: a rekey that fails leaves the credential where it + * was, which costs the caller a re-link at worst. Throwing instead would turn + * a missed OPTIMIZATION into a failed turn. + */ + async rekey(_provider: string, fromSubject: string, toSubject: string): Promise { + try { + const res = await this.fetchImpl(`${this.baseUrl}/claude-auth/api/rekey`, { + method: "POST", + headers: { "content-type": "application/json", authorization: `Bearer ${this.options.token}` }, + body: JSON.stringify({ from: fromSubject, to: toSubject }), + }); + if (!res.ok) { + console.error(`claude-auth rekey failed (leaving the credential where it is): ${res.status}`); + return false; + } + const body = (await res.json()) as { status?: string }; + return body.status === "moved"; + } catch (err) { + console.error( + "claude-auth rekey threw (leaving the credential where it is):", + err instanceof Error ? err.message : String(err), + ); + return false; + } + } } diff --git a/apps/agent-orchestrator/src/identity-link/claude-remote-gateway-client.ts b/apps/agent-orchestrator/src/identity-link/claude-remote-gateway-client.ts index ceb9f30..b4f792f 100644 --- a/apps/agent-orchestrator/src/identity-link/claude-remote-gateway-client.ts +++ b/apps/agent-orchestrator/src/identity-link/claude-remote-gateway-client.ts @@ -127,4 +127,31 @@ export class ClaudeRemoteGatewayClient implements IdentityLinkPort { throw new Error(`claude-auth invalidate (login) failed: ${res.status} ${await res.text()}`); } } + + /** + * Best-effort by design: a rekey that fails leaves the credential where it + * was, which costs the caller a re-link at worst. Throwing instead would turn + * a missed OPTIMIZATION into a failed turn. + */ + async rekey(_provider: string, fromSubject: string, toSubject: string): Promise { + try { + const res = await this.fetchImpl(`${this.baseUrl}/claude-auth/api/rekey`, { + method: "POST", + headers: { "content-type": "application/json", authorization: `Bearer ${this.options.token}` }, + body: JSON.stringify({ from: fromSubject, to: toSubject, mode: "login" }), + }); + if (!res.ok) { + console.error(`claude-auth rekey (login) failed (leaving the credential where it is): ${res.status}`); + return false; + } + const body = (await res.json()) as { status?: string }; + return body.status === "moved"; + } catch (err) { + console.error( + "claude-auth rekey (login) threw (leaving the credential where it is):", + err instanceof Error ? err.message : String(err), + ); + return false; + } + } } diff --git a/apps/agent-orchestrator/src/identity-link/gateway-client.ts b/apps/agent-orchestrator/src/identity-link/gateway-client.ts index fc2ed14..3417251 100644 --- a/apps/agent-orchestrator/src/identity-link/gateway-client.ts +++ b/apps/agent-orchestrator/src/identity-link/gateway-client.ts @@ -77,6 +77,25 @@ export interface IdentityLinkPort { * `IdentityLinkGatewayClient` doesn't need to implement it. */ invalidate?(provider: string, subject: string): Promise; + /** + * Moves an already-authorized credential from one subject to another, + * returning whether anything moved (docs/adr/0031). + * + * The pre-flight keys these records by the caller's principal, but records + * written before principals existed sit under the entry point's own subject. + * Both flows now read the same key; this is what puts the existing credential + * AT that key, instead of asking a human to re-authorize something the + * gateway is already holding. + * + * Optional, and only meaningful for a principal-keyed provider: the `github` + * link stays on the raw subject by design (it is what produces the mapping), + * so `IdentityLinkGatewayClient` deliberately does not implement this. + * + * The caller MUST have established that both subjects are the same human -- + * see the gateway route's doc. Never call it with a subject that several + * people can resolve to. + */ + rekey?(provider: string, fromSubject: string, toSubject: string): Promise; } export interface IdentityLinkGatewayClientOptions { diff --git a/apps/integration-gateway/src/claude-auth/api.test.ts b/apps/integration-gateway/src/claude-auth/api.test.ts index 2a94c60..2234bfe 100644 --- a/apps/integration-gateway/src/claude-auth/api.test.ts +++ b/apps/integration-gateway/src/claude-auth/api.test.ts @@ -54,6 +54,15 @@ function makeFakeStore(): ClaudeTokenStore & { async waitForCompletion(subject, _timeoutMs, kind = "setup-token") { return records.get(keyFor(subject, kind)); }, + async rekey(from, to, kind = "setup-token") { + if (from === to) return "occupied"; + const record = records.get(keyFor(from, kind)); + if (!record) return "not-found"; + if (records.get(keyFor(to, kind))) return "occupied"; + records.set(keyFor(to, kind), record); + records.delete(keyFor(from, kind)); + return "moved"; + }, }; } @@ -224,6 +233,80 @@ describe("ClaudeAuthApi", () => { }); }); + describe("rekey", () => { + it("moves a credential to a new subject and reports it moved", async () => { + // ADR 0031's whole point: the orchestrator re-keyed these records onto the + // caller's principal, and this is what spares an existing user a login for + // a credential the gateway already holds. + const record = { kind: "setup-token" as const, token: "sk-ant-oat01-x", createdAt: "2026-01-01T00:00:00Z" }; + await store.set("openwebui:42", record); + + const res = await fetch(`http://localhost:${port}/claude-auth/api/rekey`, { + method: "POST", + headers: { authorization: `Bearer ${BEARER}`, "content-type": "application/json" }, + body: JSON.stringify({ from: "openwebui:42", to: "github:alice" }), + }); + + expect(res.status).toBe(200); + expect(await res.json()).toEqual({ status: "moved" }); + expect(await store.get("github:alice")).toEqual(record); + expect(await store.get("openwebui:42")).toBeUndefined(); + }); + + it("routes mode=login to the login record", async () => { + const setupRecord = { kind: "setup-token" as const, token: "sk-ant-oat01-keep", createdAt: "2026-01-01T00:00:00Z" }; + await store.set("openwebui:43", setupRecord); + await store.set("openwebui:43", { kind: "login", credentialsJson: "{}", createdAt: "2026-01-01T00:00:00Z" }); + + const res = await fetch(`http://localhost:${port}/claude-auth/api/rekey`, { + method: "POST", + headers: { authorization: `Bearer ${BEARER}`, "content-type": "application/json" }, + body: JSON.stringify({ from: "openwebui:43", to: "github:bob", mode: "login" }), + }); + + expect(await res.json()).toEqual({ status: "moved" }); + expect(await store.get("github:bob", "login")).toBeDefined(); + expect(await store.get("openwebui:43")).toEqual(setupRecord); + expect(await store.get("github:bob")).toBeUndefined(); + }); + + it("reports the outcome rather than pretending, when there is nothing to move", async () => { + const res = await fetch(`http://localhost:${port}/claude-auth/api/rekey`, { + method: "POST", + headers: { authorization: `Bearer ${BEARER}`, "content-type": "application/json" }, + body: JSON.stringify({ from: "openwebui:nobody", to: "github:alice" }), + }); + + expect(res.status).toBe(200); + expect(await res.json()).toEqual({ status: "not-found" }); + }); + + it("400s on a missing or non-string subject rather than moving something unintended", async () => { + for (const body of [{ from: "openwebui:42" }, { to: "github:alice" }, { from: "", to: "github:alice" }, { from: 1, to: 2 }]) { + const res = await fetch(`http://localhost:${port}/claude-auth/api/rekey`, { + method: "POST", + headers: { authorization: `Bearer ${BEARER}`, "content-type": "application/json" }, + body: JSON.stringify(body), + }); + expect(res.status).toBe(400); + } + }); + + it("401s without the master bearer token -- it moves a credential between identities", async () => { + await store.set("openwebui:42", { kind: "setup-token", token: "sk-ant-oat01-x", createdAt: "2026-01-01T00:00:00Z" }); + + const res = await fetch(`http://localhost:${port}/claude-auth/api/rekey`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ from: "openwebui:42", to: "github:attacker" }), + }); + + expect(res.status).toBe(401); + expect(await store.get("github:attacker")).toBeUndefined(); + expect(await store.get("openwebui:42")).toBeDefined(); + }); + }); + describe("mode=login without loginFlows configured", () => { it("501s on start rather than silently falling back to setup-token", async () => { const unconfiguredApi = new ClaudeAuthApi(setupFlows as unknown as ConstructorParameters[0], store, BEARER, PUBLIC_BASE_URL); diff --git a/apps/integration-gateway/src/claude-auth/api.ts b/apps/integration-gateway/src/claude-auth/api.ts index 87ec7e3..da11ea5 100644 --- a/apps/integration-gateway/src/claude-auth/api.ts +++ b/apps/integration-gateway/src/claude-auth/api.ts @@ -221,6 +221,10 @@ export class ClaudeAuthApi { await this.handleInvalidate(req, res); return true; } + if (req.method === "POST" && action === "rekey") { + await this.handleRekey(req, res); + return true; + } if (req.method === "POST" && action === "writeback-token") { await this.handleWritebackToken(req, res); return true; @@ -306,6 +310,48 @@ export class ClaudeAuthApi { sendJson(res, 200, { status: "ok" }); } + /** + * Moves one subject's stored credential to another subject (bearer-gated -- + * called by agent-orchestrator's authorization pre-flight, never by a run). + * + * Why the gateway exposes this at all: agent-orchestrator changed which + * subject it keys these records by, from the entry point's own subject to the + * caller's principal (docs/adr/0029, 0031). Both flows now READ the same key; + * this is what makes the credential a human already authorized actually BE at + * that key, instead of every existing user paying a fresh login to reproduce + * a credential the gateway is already holding. + * + * ## Who is allowed to ask + * + * The orchestrator, and only because it is the component that established + * both subjects belong to the same human -- `from` is the authenticated + * caller's own subject for the turn, and `to` is the principal derived from a + * GitHub account that same caller proved control of. This route cannot + * re-derive that, so it does not pretend to: it is gated on the master bearer + * token, exactly like `token` (which already hands out any subject's + * credential) and `invalidate` (which already destroys any subject's). It + * grants no authority over the keyspace that those two do not. + * + * What it will NOT do is overwrite a record at the destination, so a stale + * source can never displace a current credential. + */ + private async handleRekey(req: IncomingMessage, res: ServerResponse): Promise { + const body = await parseJsonBody(req); + const from = (body as { from?: unknown } | undefined)?.from; + const to = (body as { to?: unknown } | undefined)?.to; + if (typeof from !== "string" || !from.trim() || typeof to !== "string" || !to.trim()) { + sendJson(res, 400, { error: "Request body must be JSON with non-empty string `from` and `to` fields" }); + return; + } + const mode = normalizeMode((body as { mode?: unknown }).mode); + try { + const status = await this.store.rekey(from, to, kindForMode(mode)); + sendJson(res, 200, { status }); + } catch (err) { + sendJson(res, 502, { error: err instanceof Error ? err.message : String(err) }); + } + } + private async handleInvalidate(req: IncomingMessage, res: ServerResponse): Promise { const body = await parseJsonBody(req); if (!body || typeof (body as { subject?: unknown }).subject !== "string") { diff --git a/apps/integration-gateway/src/claude-auth/store.test.ts b/apps/integration-gateway/src/claude-auth/store.test.ts index d6eb947..3a7f9ea 100644 --- a/apps/integration-gateway/src/claude-auth/store.test.ts +++ b/apps/integration-gateway/src/claude-auth/store.test.ts @@ -133,6 +133,69 @@ describe("RedisClaudeTokenStore", () => { expect(await store.get("user-999")).toEqual(setupRecord); }); + it("moves an authorized credential to a new subject, leaving nothing behind", async () => { + // ADR 0031: the orchestrator re-keyed these records onto the caller's + // principal. Moving the existing one is what spares every current user a + // login for a credential the gateway is already holding. + const store = new RedisClaudeTokenStore("redis://fake", KEY); + const record = { kind: "login" as const, credentialsJson: '{"accessToken":"supersecret"}', createdAt: "2026-07-22T00:00:00.000Z" }; + await store.set("openwebui:42", record); + + expect(await store.rekey("openwebui:42", "github:alice", "login")).toBe("moved"); + + expect(await store.get("github:alice", "login")).toEqual(record); + // Moved, not copied: the write-back that keeps a login credential alive + // only ever writes the new key, so a leftover copy would rot and then fail + // whichever flow still read it. + expect(await store.get("openwebui:42", "login")).toBeUndefined(); + }); + + it("moves only the requested kind", async () => { + const store = new RedisClaudeTokenStore("redis://fake", KEY); + const setupRecord = { kind: "setup-token" as const, token: "sk-ant-oat01-x", createdAt: "2026-07-22T00:00:00.000Z" }; + await store.set("openwebui:42", setupRecord); + await store.set("openwebui:42", { kind: "login", credentialsJson: "{}", createdAt: "2026-07-22T00:00:00.000Z" }); + + await store.rekey("openwebui:42", "github:alice", "login"); + + expect(await store.get("openwebui:42")).toEqual(setupRecord); + expect(await store.get("github:alice")).toBeUndefined(); + }); + + it("refuses to overwrite a record already at the destination", async () => { + // The destination's record is by definition at least as current as the one + // being moved -- clobbering it would replace a live credential with an older + // one and cause the "Login expired" failure this is meant to prevent. + const store = new RedisClaudeTokenStore("redis://fake", KEY); + const current = { kind: "setup-token" as const, token: "sk-ant-oat01-current", createdAt: "2026-07-25T00:00:00.000Z" }; + const stale = { kind: "setup-token" as const, token: "sk-ant-oat01-stale", createdAt: "2026-07-01T00:00:00.000Z" }; + await store.set("github:alice", current); + await store.set("openwebui:42", stale); + + expect(await store.rekey("openwebui:42", "github:alice")).toBe("occupied"); + + expect(await store.get("github:alice")).toEqual(current); + // The source is left intact too: nothing was moved, so nothing is deleted. + expect(await store.get("openwebui:42")).toEqual(stale); + }); + + it("reports not-found rather than creating an empty record", async () => { + const store = new RedisClaudeTokenStore("redis://fake", KEY); + expect(await store.rekey("openwebui:nobody", "github:alice")).toBe("not-found"); + expect(await store.get("github:alice")).toBeUndefined(); + }); + + it("treats a self-move as a no-op instead of deleting the record", async () => { + // Guards the degenerate case where a caller's principal IS its subject: the + // naive move (set then delete) would delete what it just wrote. + const store = new RedisClaudeTokenStore("redis://fake", KEY); + const record = { kind: "setup-token" as const, token: "sk-ant-oat01-x", createdAt: "2026-07-22T00:00:00.000Z" }; + await store.set("github:alice", record); + + expect(await store.rekey("github:alice", "github:alice")).toBe("occupied"); + expect(await store.get("github:alice")).toEqual(record); + }); + it("round-trips a credential write-back grant back to its subject", async () => { const store = new RedisClaudeTokenStore("redis://fake", KEY); const token = await store.createWritebackToken("user-wb", 900); diff --git a/apps/integration-gateway/src/claude-auth/store.ts b/apps/integration-gateway/src/claude-auth/store.ts index f0fb323..d52fb5d 100644 --- a/apps/integration-gateway/src/claude-auth/store.ts +++ b/apps/integration-gateway/src/claude-auth/store.ts @@ -53,6 +53,28 @@ export interface ClaudeTokenStore { * credential forever. */ delete(subject: string, kind?: ClaudeAuthKind): Promise; + /** + * Moves an existing record from one subject to another, so a credential the + * human already authorized keeps working under a new key instead of costing + * them a fresh flow (docs/adr/0031). + * + * Exists because agent-orchestrator changed WHICH subject it keys these + * records by -- from the entry point's own subject to the caller's principal + * (ADR 0029/0030 §6) -- and a re-key without a move means every existing user + * re-authorizes for no reason they can perceive. The alternative, leaving the + * old record in place as a copy, is worse than either: the write-back that + * keeps a `login` credential alive only ever writes the NEW key, so the copy + * silently rots and whichever flow still reads it fails with "Login expired". + * + * Deliberately non-destructive at the destination: an existing record there + * is never clobbered (`"occupied"`), because it is by definition at least as + * current as the one being moved. + * + * Authorization is the CALLER's responsibility, and it is not nothing: this + * moves a credential between identities, so the caller must have established + * that both subjects are the same human. See the `rekey` route's doc. + */ + rekey(fromSubject: string, toSubject: string, kind?: ClaudeAuthKind): Promise<"moved" | "not-found" | "occupied">; /** * Mints a single-purpose, expiring bearer token that authorizes ONE thing: * replacing `subject`'s stored `login` record with a refreshed @@ -175,6 +197,31 @@ export class RedisClaudeTokenStore implements ClaudeTokenStore { } } + async rekey( + fromSubject: string, + toSubject: string, + kind: ClaudeAuthKind = "setup-token", + ): Promise<"moved" | "not-found" | "occupied"> { + if (fromSubject === toSubject) return "occupied"; + // Read through `get`/`set` rather than RENAME-ing the Redis key: the record + // is field-encrypted, and a decrypt/re-encrypt round trip is the same work + // either way -- while `set` also publishes on the destination's completion + // channel, so a turn already blocked in `waitForCompletion` for the new + // subject resolves immediately instead of waiting out its timeout. + const record = await this.get(fromSubject, kind); + if (!record) return "not-found"; + if (await this.get(toSubject, kind)) return "occupied"; + await this.set(toSubject, record); + // Read back before deleting the source. `set` swallows its own Redis errors + // by design, so an unverified delete here could drop the human's only + // credential on a transient write failure -- the one outcome strictly worse + // than the extra login this whole method exists to avoid. + const landed = await this.get(toSubject, kind); + if (!landed) return "not-found"; + await this.delete(fromSubject, kind); + return "moved"; + } + /** * Only the token's SHA-256 is stored, so the value in Redis can't be * replayed as a bearer token by anything that can read the keyspace -- the From 8f8ba31651a93fc979a67b155bb0171dc6ad6227 Mon Sep 17 00:00:00 2001 From: Austin Kurpuis Date: Sat, 25 Jul 2026 17:09:26 -0700 Subject: [PATCH 2/6] Converge chat and triage on one Claude credential, both directions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ADR 0029's cross-entry-point sharing worked in one direction only, which left the reported bug fully intact for the flow it was reported from: 15:57 [authorization] {"verdict":"authorized", "actorLogin":null} 16:12 [authorization] {"verdict":"link-required","pending":["claude@github:imaustink"]} Same human, fifteen minutes apart, chat working and ai-triage prompting. A webhook turn always carries a verified `senderLogin`, so it always resolved a principal; chat could only read a login off an existing `github` link, and one of those was only ever created as a side effect of an Agent's `identityProviders` -- which ADR 0030 §5 removed `github` from to fix a production 401. So the fix for the 401 silently switched off sharing for every chat caller. Neither flow was broken alone; they simply never met. Establishing a principal becomes its own pre-flight step (ADR 0031), independent of what the Agent declares: an ordinary `github` link that is LINK-ONLY -- its token is never injected, so no GITHUB_TOKEN reaches the run and the agent's delegated-write path stays unreachable. Mapping and credential provisioning are now separately reachable, which is what §5 asked for and had no mechanism for. Safety and ordering, each pinned by a test: - Gated on `Identity.perUser`, newly asserted by the one resolver that knows it structurally (`OpenWebUiForwardedUserResolver`). A login filed under the gateway's shared service subject would be inherited by every later senderLogin-less webhook turn, handing one person's credentials to everyone. A live channel (`progressListener`) was tried as the signal and rejected: a shared subject on a streaming caller passes it, so it is unsound in the one direction that leaks. - The principal step runs first, so CRD provider order stays irrelevant (§4). - A pending principal link stops the turn rather than filing the credentials the user is about to create under a subject they are one link away from abandoning. A deliberate exception to §4's batching, which assumes the providers are independent. - Every failure degrades to the raw subject rather than blocking: sharing is an improvement, not a precondition. Existing credentials are ADOPTED rather than re-authorized, using the rekey added in the previous commit -- lazily, on the turn that needs it, because the (subject -> principal) mapping only exists on a caller's own authenticated turn. Same `perUser` gate, for the same reason. The `authorized` verdict now carries the `principal` its credentials were keyed by and `delegateToAgent` adopts it: otherwise the expired-credential path re-derives the pre-upgrade key, invalidates a record that was never written, and tells the user to retry -- the infinite loop that path exists to prevent. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01W7XfNb5HsmaZH7QyxUrbyB --- .../src/agent/authorization-service.test.ts | 261 +++++++++++++++++- .../src/agent/authorization-service.ts | 158 ++++++++++- .../src/agent/graph.test.ts | 116 ++++++++ apps/agent-orchestrator/src/agent/graph.ts | 15 +- .../identity-link/credential-subject.test.ts | 14 +- .../src/identity-link/credential-subject.ts | 26 +- .../openwebui-forwarded-user-resolver.test.ts | 12 +- .../rbac/openwebui-forwarded-user-resolver.ts | 5 +- apps/agent-orchestrator/src/rbac/types.ts | 17 ++ ...authorization-preflight-outside-the-llm.md | 6 + ...031-principal-establishing-account-link.md | 174 ++++++++++++ docs/adr/README.md | 1 + 12 files changed, 789 insertions(+), 16 deletions(-) create mode 100644 docs/adr/0031-principal-establishing-account-link.md diff --git a/apps/agent-orchestrator/src/agent/authorization-service.test.ts b/apps/agent-orchestrator/src/agent/authorization-service.test.ts index 2488fd1..1f37659 100644 --- a/apps/agent-orchestrator/src/agent/authorization-service.test.ts +++ b/apps/agent-orchestrator/src/agent/authorization-service.test.ts @@ -25,11 +25,21 @@ function gateway( token?: IdentityLinkToken; start?: IdentityLinkStartResult | "throw"; waitResolvesTo?: IdentityLinkToken | "throw"; + /** A record sitting under the caller's raw subject, adoptable by `rekey` (docs/adr/0031). */ + prePrincipalToken?: IdentityLinkToken; } >, ): IdentityLinkPort { + /** Subjects a `rekey` has moved a pre-principal record onto, so `getToken` starts finding it there. */ + const adopted = new Map(); return { - getToken: vi.fn(async (provider: string) => behaviour[provider]?.token), + getToken: vi.fn(async (provider: string, subject: string) => adopted.get(`${provider}@${subject}`) ?? behaviour[provider]?.token), + rekey: vi.fn(async (provider: string, from: string, to: string) => { + const record = behaviour[provider]?.prePrincipalToken; + if (!record || from === to) return false; + adopted.set(`${provider}@${to}`, record); + return true; + }), start: vi.fn(async (provider: string) => { const s = behaviour[provider]?.start; if (s === "throw") throw new Error(`start failed for ${provider}`); @@ -68,7 +78,9 @@ describe("AuthorizationService.authorize", () => { it("clears a launch with no secretEnv when the agent declares no providers", async () => { const svc = new AuthorizationService({}); const verdict = await svc.authorize({ agent: { id: "plain" }, identity: identity(), request: "hi" }); - expect(verdict).toEqual({ kind: "authorized" }); + // `principal` rides on every authorized verdict (docs/adr/0031) -- the raw + // subject standing in for itself here, since nothing established a mapping. + expect(verdict).toEqual({ kind: "authorized", principal: "openwebui:alice" }); }); it("keys cross-entry-point providers by principal and github by raw subject (§6)", async () => { @@ -426,3 +438,248 @@ describe("AuthorizationService.resolveLinkedCredentials", () => { expect(await svc.resolveLinkedCredentials({ identity: identity() })).toEqual({ kind: "resolved" }); }); }); + +/** + * The principal pre-flight (docs/adr/0031). + * + * These are the tests for the defect observed in production: chat kept writing + * Claude credentials under `openwebui:` while GitHub triage read + * `github:`, because only the webhook path could name a login. Sharing + * is the property under test, so each case asserts on the SUBJECT a credential + * was keyed by rather than on the message the user saw. + */ +describe("AuthorizationService.authorize principal pre-flight", () => { + it("establishes a principal before keying any cross-entry-point credential", async () => { + // A chat caller with no GitHub mapping yet. The claude gateway must not be + // touched: starting its flow now would file the credential the user is + // about to create under the raw subject -- the split this closes. + const github = gateway({ github: {} }); + const claude = gateway({ claude: {} }); + const svc = new AuthorizationService({ identityLinkGateway: github, claudeAuthGateway: claude }); + + const verdict = await svc.authorize({ + agent: { id: "swe", identityProviders: ["claude"] }, + identity: identity({ perUser: true }), + request: "fix the bug", + progressListener: vi.fn(), + }); + + expect(verdict.kind).toBe("link-required"); + if (verdict.kind !== "link-required") return; + expect(github.start).toHaveBeenCalledWith("github", "openwebui:alice", "authcode"); + expect(claude.start).not.toHaveBeenCalled(); + // The resume anchor is the GitHub link, recorded against the subject it was + // actually started with (the PR #144 store-vs-wait rule). + expect(verdict.pending).toMatchObject({ provider: "github", subject: "openwebui:alice", agentId: "swe" }); + // The original goal rides along, so the resume re-delegates THIS request. + expect(verdict.pending?.request).toBe("fix the bug"); + }); + + it("keys the credential canonically in the SAME turn once the link lands, without injecting GITHUB_TOKEN", async () => { + // The whole point: after linking, this chat turn reads the same + // `claude@github:alice` record a triage turn reads -- and the link that + // produced the mapping stays a mapping, never a credential for the run. + const github = gateway({ github: { waitResolvesTo: { token: "gho_x", githubLogin: "Alice" } } }); + const claude = gateway({ claude: { token: { token: "sk-ant-oat01-x" } } }); + const svc = new AuthorizationService({ identityLinkGateway: github, claudeAuthGateway: claude }); + + const verdict = await svc.authorize({ + agent: { id: "swe", identityProviders: ["claude"] }, + identity: identity({ perUser: true }), + request: "r", + progressListener: vi.fn(), + }); + + expect(verdict.kind).toBe("authorized"); + if (verdict.kind !== "authorized") return; + // Lower-cased: a webhook echoes "Alice", the OAuth API normalizes it, and + // two casings would key two records. + expect(claude.getToken).toHaveBeenCalledWith("claude", "github:alice"); + expect(verdict.principal).toBe("github:alice"); + expect(verdict.secretEnv?.map((e) => e.name)).toEqual(["CLAUDE_CODE_OAUTH_TOKEN", ACTOR_LOGIN_ENV]); + expect(verdict.actorLogin).toBe("Alice"); + }); + + it("never starts a principal link for a caller with no live channel", async () => { + // Security, not ergonomics: a webhook relay authenticates as its own + // service account, so its subject is shared by every sender. A link filed + // under it would hand that one person's credentials to every later + // senderLogin-less turn. Such a turn degrades to the raw subject instead. + const github = gateway({ github: {} }); + const claude = gateway({ claude: { token: { token: "sk-ant-oat01-x" } } }); + const svc = new AuthorizationService({ identityLinkGateway: github, claudeAuthGateway: claude }); + + const verdict = await svc.authorize({ + agent: { id: "swe", identityProviders: ["claude"] }, + identity: identity({ subject: "client-integration-gateway" }), + request: "r", + }); + + expect(verdict.kind).toBe("authorized"); + expect(github.start).not.toHaveBeenCalled(); + expect(claude.getToken).toHaveBeenCalledWith("claude", "client-integration-gateway"); + }); + + it("does nothing when the caller already has a canonical principal", async () => { + // The webhook path with a verified senderLogin, and every chat turn after + // the first: resolveIdentity already resolved it, so there is nothing to + // establish and no extra round trip to pay for. + const github = gateway({ github: {} }); + const claude = gateway({ claude: { token: { token: "sk-ant-oat01-x" } } }); + const svc = new AuthorizationService({ identityLinkGateway: github, claudeAuthGateway: claude }); + + await svc.authorize({ + agent: { id: "swe", identityProviders: ["claude"] }, + identity: identity({ principal: "github:alice", perUser: true }), + request: "r", + progressListener: vi.fn(), + }); + + // No link flow, and no `github` token read for the MAPPING (the one lookup + // that remains is §5's actor-login read, which is not a principal step). + expect(github.start).not.toHaveBeenCalled(); + expect(github.getToken).toHaveBeenCalledTimes(1); + }); + + it("degrades to the raw subject when the principal link cannot be started", async () => { + // Sharing is an improvement, not a precondition. A GitHub OAuth hiccup must + // not deny a run whose own credentials are already linked. + const github = gateway({ github: { start: "throw" } }); + const claude = gateway({ claude: { token: { token: "sk-ant-oat01-x" } } }); + const svc = new AuthorizationService({ identityLinkGateway: github, claudeAuthGateway: claude }); + + const verdict = await svc.authorize({ + agent: { id: "swe", identityProviders: ["claude"] }, + identity: identity({ perUser: true }), + request: "r", + progressListener: vi.fn(), + }); + + expect(verdict.kind).toBe("authorized"); + if (verdict.kind !== "authorized") return; + expect(verdict.principal).toBe("openwebui:alice"); + expect(claude.getToken).toHaveBeenCalledWith("claude", "openwebui:alice"); + // Not reported to the user, and NOT in `failedToStart`: nothing the caller + // did is wrong and nothing they can do fixes it. + expect(verdict.kind).toBe("authorized"); + }); + + it("keeps CRD provider ORDER irrelevant, including when github is declared too", async () => { + // `[claude, github]` used to key the claude credential before any login was + // known. The principal step runs first regardless, and the declared github + // provider still gets its token injected -- mapping and credential are + // separate concerns, not alternatives. + const github = gateway({ github: { token: { token: "gho_x", githubLogin: "alice" } } }); + const claude = gateway({ claude: { token: { token: "sk-ant-oat01-x" } } }); + const svc = new AuthorizationService({ identityLinkGateway: github, claudeAuthGateway: claude }); + + const verdict = await svc.authorize({ + agent: { id: "swe", identityProviders: ["claude", "github"] }, + identity: identity({ perUser: true }), + request: "r", + progressListener: vi.fn(), + }); + + expect(verdict.kind).toBe("authorized"); + if (verdict.kind !== "authorized") return; + expect(claude.getToken).toHaveBeenCalledWith("claude", "github:alice"); + expect(verdict.secretEnv?.map((e) => e.name)).toEqual([ + "CLAUDE_CODE_OAUTH_TOKEN", + "GITHUB_TOKEN", + ACTOR_LOGIN_ENV, + ]); + }); +}); + +/** + * Adopting a pre-principal credential (docs/adr/0031). + * + * Both flows now read the same key, but records written before principals + * existed sit under the entry point's own subject. Charging a human a fresh + * login to reproduce a credential the gateway is still holding is not a fix, so + * the pre-flight moves it. + */ +describe("AuthorizationService.authorize pre-principal adoption", () => { + it("adopts the caller's existing credential onto their principal instead of prompting", async () => { + const claude = gateway({ claude: { prePrincipalToken: { token: "sk-ant-oat01-authorized-in-chat" } } }); + const svc = new AuthorizationService({ claudeAuthGateway: claude }); + + const verdict = await svc.authorize({ + agent: { id: "swe", identityProviders: ["claude"] }, + identity: identity({ principal: "github:alice", perUser: true }), + request: "r", + }); + + expect(verdict.kind).toBe("authorized"); + if (verdict.kind !== "authorized") return; + expect(claude.rekey).toHaveBeenCalledWith("claude", "openwebui:alice", "github:alice"); + expect(verdict.secretEnv).toEqual([{ name: "CLAUDE_CODE_OAUTH_TOKEN", value: "sk-ant-oat01-authorized-in-chat" }]); + }); + + it("never adopts from a subject that is not per-user", async () => { + // The webhook relay's subject is its own service account, shared by every + // sender: moving a credential off it would hand whoever authorized first to + // whoever triggered next. This turn must park instead. + const claude = gateway({ claude: { prePrincipalToken: { token: "sk-ant-oat01-someone-elses" } } }); + const svc = new AuthorizationService({ claudeAuthGateway: claude }); + + const verdict = await svc.authorize({ + agent: { id: "swe", identityProviders: ["claude"] }, + identity: identity({ subject: "client-integration-gateway", principal: "github:sender" }), + request: "r", + senderLogin: "sender", + }); + + expect(claude.rekey).not.toHaveBeenCalled(); + expect(verdict.kind).toBe("link-required"); + }); + + it("does not attempt a move when the principal IS the subject", async () => { + // Nothing to converge, and a self-move is the degenerate case that would + // delete the record it just wrote if the store took it literally. + const claude = gateway({ claude: { prePrincipalToken: { token: "sk-ant-oat01-x" } } }); + const svc = new AuthorizationService({ claudeAuthGateway: claude }); + + await svc.authorize({ + agent: { id: "swe", identityProviders: ["claude"] }, + identity: identity({ perUser: true }), + request: "r", + }); + + expect(claude.rekey).not.toHaveBeenCalled(); + }); + + it("falls back to the ordinary link prompt when there is nothing to adopt", async () => { + const claude = gateway({ claude: {} }); + const svc = new AuthorizationService({ claudeAuthGateway: claude }); + + const verdict = await svc.authorize({ + agent: { id: "swe", identityProviders: ["claude"] }, + identity: identity({ principal: "github:alice", perUser: true }), + request: "r", + }); + + expect(claude.rekey).toHaveBeenCalled(); + expect(verdict.kind).toBe("link-required"); + if (verdict.kind !== "link-required") return; + // Started against the principal, as always -- adoption changes nothing + // about which subject a NEW credential is filed under. + expect(verdict.pending?.subject).toBe("github:alice"); + }); + + it("prompts rather than failing the turn when a gateway has no rekey at all", async () => { + // `IdentityLinkPort.rekey` is optional (the github client doesn't implement + // it), so absence must read as "nothing to adopt", not as a crash. + const claude = gateway({ claude: {} }); + delete (claude as { rekey?: unknown }).rekey; + const svc = new AuthorizationService({ claudeAuthGateway: claude }); + + const verdict = await svc.authorize({ + agent: { id: "swe", identityProviders: ["claude"] }, + identity: identity({ principal: "github:alice", perUser: true }), + request: "r", + }); + + expect(verdict.kind).toBe("link-required"); + }); +}); diff --git a/apps/agent-orchestrator/src/agent/authorization-service.ts b/apps/agent-orchestrator/src/agent/authorization-service.ts index 93615d1..0138da8 100644 --- a/apps/agent-orchestrator/src/agent/authorization-service.ts +++ b/apps/agent-orchestrator/src/agent/authorization-service.ts @@ -1,5 +1,5 @@ import type { IdentityLinkPort, IdentityLinkStartResult } from "../identity-link/gateway-client.js"; -import { resolveActorLogin } from "../identity-link/credential-subject.js"; +import { canonicalSubjectForLogin, isCanonicalPrincipal, resolveActorLogin } from "../identity-link/credential-subject.js"; import type { Identity } from "../rbac/types.js"; /** @@ -42,6 +42,17 @@ export const ACTOR_LOGIN_ENV = "AGENT_ACTOR_LOGIN"; */ export const CROSS_ENTRY_POINT_PROVIDERS: ReadonlySet = new Set(["claude", "claude-remote"]); +/** + * The provider whose link ESTABLISHES a principal (docs/adr/0031). + * + * GitHub, because it is the one identity both entry points can reach: a webhook + * vouches for the sender, and a chat caller can prove control of the account. + * Nothing else about the pre-flight is GitHub-specific -- when principals become + * first-class (ADR 0030 §6's alias table), this is the constant that stops + * meaning "GitHub" and starts meaning "whatever establishes the alias". + */ +export const PRINCIPAL_PROVIDER = "github"; + /** * Maps an identity-linked provider (Agent.identityProviders, e.g. "github") to * the env var name its linked token is injected as (AgentLaunchOptions' @@ -124,8 +135,19 @@ export interface AuthorizationRequest { * "launch anyway", which is the failure direction that matters here. */ export type AuthorizationOutcome = - /** Cleared to launch. `secretEnv` is the credentials + actor context the run receives. */ - | { kind: "authorized"; secretEnv?: CredentialEnvEntry[]; actorLogin?: string } + /** + * Cleared to launch. `secretEnv` is the credentials + actor context the run + * receives. + * + * `principal` is the one the credentials were actually keyed by, which the + * pre-flight may have UPGRADED this turn by establishing a mapping + * (docs/adr/0031). The caller must persist it onto the turn's identity: + * anything that later re-derives the credential's key -- notably the + * expired-credential invalidate path -- would otherwise clear a record that + * was never written and leave the caller re-reading a dead credential + * forever. + */ + | { kind: "authorized"; secretEnv?: CredentialEnvEntry[]; actorLogin?: string; principal?: string } /** * Not cleared: one or more links are outstanding, or a link flow could not be * started. `message` is the complete user-facing text for the turn, including @@ -218,7 +240,55 @@ export class AuthorizationService { */ let actorLoginFromLoop: string | undefined; - for (const provider of agent.identityProviders ?? []) { + // ── Principal pre-flight (docs/adr/0031) ─────────────────────────────── + // A cross-entry-point credential can only be SHARED if this caller has a + // principal to key it by. The webhook path always does (the verified + // `senderLogin`); the chat path only does once a `github` link exists to + // read a login off -- and links were only ever created as a side effect of + // an Agent's `identityProviders`, which ADR 0030 §5 removed `github` from. + // So chat kept writing credentials under its raw `openwebui:` subject + // while triage read `github:`, and the two never converged. + // + // Fix: when a run needs a cross-entry-point credential and no principal is + // established, establish one FIRST, with an ordinary `github` link that is + // deliberately LINK-ONLY -- its token is never injected into the run. That + // keeps the mapping separable from credential provisioning, which is the + // conflation ADR 0030 §5 identified as the cause of the production 401: + // declaring `github` to obtain a login also handed the agent a + // `GITHUB_TOKEN` and activated its delegated-write path. + // + // Deliberately first in the plan so provider ORDER in the CRD stays + // irrelevant (ADR 0030 §4) -- a `[claude, github]` Agent must not key its + // claude credential before the principal is known. + let principal = identity.principal ?? identity.subject; + let principalLogin: string | undefined; + const providerPlan: { name: string; principalOnly?: boolean }[] = (agent.identityProviders ?? []).map((name) => ({ + name, + })); + if ( + providerPlan.some((p) => CROSS_ENTRY_POINT_PROVIDERS.has(p.name)) && + !isCanonicalPrincipal(principal) && + this.deps.identityLinkGateway && + // PER-USER subjects only, and this guard is load-bearing security rather + // than ergonomics. A webhook relay authenticates as the gateway's own + // service account, so its subject is SHARED by every sender: filing a + // login under it would make every later senderLogin-less webhook turn + // inherit that one person's Claude credentials. A webhook turn with a + // real human behind it already carries `senderLogin` and never needs this + // path -- one without is exactly the case that must not take it. + // + // Asserted by the resolver that knows (`Identity.perUser`) rather than + // inferred here from a proxy like "has a live channel": a shared subject + // arriving on a streaming caller would pass that proxy, which is unsound + // in the one direction that leaks. Absent the assertion this degrades to + // the pre-principal behaviour (no sharing), never to the wrong principal. + identity.perUser === true + ) { + providerPlan.unshift({ name: PRINCIPAL_PROVIDER, principalOnly: true }); + } + + for (const entry of providerPlan) { + const provider = entry.name; const gateway = this.gatewayFor(provider); if (!gateway) { this.logVerdict("misconfigured", agent.id, { provider, reason: "no gateway configured" }); @@ -235,12 +305,40 @@ export class AuthorizationService { // property of the specific account that established it, and it is the very // thing principal resolution reads, so keying it by principal would be // circular. - const credentialSubject = CROSS_ENTRY_POINT_PROVIDERS.has(provider) - ? (identity.principal ?? identity.subject) - : identity.subject; + const credentialSubject = CROSS_ENTRY_POINT_PROVIDERS.has(provider) ? principal : identity.subject; let existing = await gateway.getToken(provider, credentialSubject); + // ── Adopt a pre-principal credential (docs/adr/0031) ───────────────── + // Nothing at the principal, but this caller may well have authorized + // already -- under their entry point's own subject, which is where these + // records were keyed before principals existed. Both flows now READ the + // principal; moving the record is what makes the credential the human + // already created actually BE there, instead of charging them a fresh + // login to reproduce something the gateway is still holding. + // + // Lazily, on the turn that needs it, rather than as a migration job: the + // set of (subject, principal) pairs is only knowable from a caller's own + // authenticated turn, and a batch job would have to invent that mapping. + // + // Gated on `perUser` for the same reason establishing a principal is: a + // shared subject's credential belongs to whoever authorized first, and + // moving it under a sender's principal would hand it to them outright. + // The webhook path's subject IS shared, so it never adopts -- it reads + // only what its own principal already has. + if ( + !existing && + CROSS_ENTRY_POINT_PROVIDERS.has(provider) && + identity.perUser === true && + credentialSubject !== identity.subject && + (await gateway.rekey?.(provider, identity.subject, credentialSubject)) + ) { + existing = await gateway.getToken(provider, credentialSubject); + console.log( + `[authorization] adopted this caller's pre-principal ${provider} credential onto their principal; no re-authorization needed`, + ); + } + if (!existing) { // Signal "this turn needs a link" NOW, before the (possibly slow) // start() below -- a fire-and-forget caller uses this to avoid @@ -271,6 +369,16 @@ export class AuthorizationService { return null; }); if (!started) { + // A principal link that won't start must DEGRADE, not block: sharing + // is an improvement over per-entry-point keying, and refusing the + // turn over it would make a GitHub OAuth hiccup deny a run whose own + // credentials are already linked -- the coupling ADR 0030 §4 removed. + if (entry.principalOnly) { + console.error( + `[authorization] could not start the principal-establishing ${PRINCIPAL_PROVIDER} link; continuing keyed by the raw subject, so this run's credentials will not be shared across entry points`, + ); + continue; + } failedToStart.push(PROVIDER_LABEL[provider] ?? provider); continue; } @@ -340,10 +448,34 @@ export class AuthorizationService { request: req.request, }, }); + // Assess nothing further when it is the PRINCIPAL that is pending: the + // remaining providers would have to be keyed by a subject this turn is + // about to abandon, so starting their flows would file the credentials + // the user is about to create under the raw subject -- creating + // exactly the split this change exists to close. The resume turn + // re-enters with a canonical principal and assesses them all then. + if (entry.principalOnly) break; continue; } } + // A link-only principal step: it contributes the mapping and nothing else. + // No `secretEnv` entry, so no `GITHUB_TOKEN` reaches the run and the + // agent's delegated-write path stays unreachable (docs/adr/0030 §5). + if (entry.principalOnly) { + if (existing.githubLogin) { + principalLogin = existing.githubLogin; + principal = canonicalSubjectForLogin(existing.githubLogin); + } else { + // A link with no login on it can't produce a mapping. Degrade to the + // raw subject rather than failing a turn over a missing nicety. + console.error( + `[authorization] the ${PRINCIPAL_PROVIDER} link for this caller carries no login; continuing keyed by the raw subject, without cross-entry-point sharing`, + ); + } + continue; + } + // Capture the login off whichever way this credential arrived. On a // streaming turn it lands via waitForCompletion, so the standalone lookup // below would miss it -- but that lookup is still needed for Agents that @@ -415,7 +547,9 @@ export class AuthorizationService { // exists to eliminate, and the co-author trailer degrades to the login-only // form without it. const actorLogin = - actorLoginFromLoop ?? (await resolveActorLogin(identity.subject, req.senderLogin, this.deps.identityLinkGateway)); + actorLoginFromLoop ?? + principalLogin ?? + (await resolveActorLogin(identity.subject, req.senderLogin, this.deps.identityLinkGateway)); if (actorLogin) { secretEnv = [...(secretEnv ?? []), { name: ACTOR_LOGIN_ENV, value: actorLogin }]; } @@ -426,8 +560,14 @@ export class AuthorizationService { // them at once. injecting: (secretEnv ?? []).map((e) => e.name), actorLogin: actorLogin ?? null, + principal, }); - return { kind: "authorized", ...(secretEnv ? { secretEnv } : {}), ...(actorLogin ? { actorLogin } : {}) }; + return { + kind: "authorized", + ...(secretEnv ? { secretEnv } : {}), + ...(actorLogin ? { actorLogin } : {}), + principal, + }; } /** diff --git a/apps/agent-orchestrator/src/agent/graph.test.ts b/apps/agent-orchestrator/src/agent/graph.test.ts index f6207e4..4b9cd8a 100644 --- a/apps/agent-orchestrator/src/agent/graph.test.ts +++ b/apps/agent-orchestrator/src/agent/graph.test.ts @@ -2883,6 +2883,16 @@ describe("buildAgentGraph canonical credential subject (cross-flow Claude creden }); } + /** + * A chat caller as `OpenWebUiForwardedUserResolver` resolves one: a per-user + * subject, asserted as such. `perUser` is what lets the pre-flight establish a + * principal against it (docs/adr/0031) -- the default double here omits it, + * standing in for a subject that may be shared. + */ + const chatCaller = (): IdentityResolver => ({ + resolve: vi.fn().mockResolvedValue({ subject: "alice", roles: ["reader"], perUser: true }), + }); + it("keys a chat caller's claude-remote credential by github: from their GitHub link, not the raw openwebui subject", async () => { const deps = canonicalDeps(); const graph = buildAgentGraph(deps); @@ -2958,6 +2968,112 @@ describe("buildAgentGraph canonical credential subject (cross-flow Claude creden expect(deps.identityLinkGateway!.getToken).not.toHaveBeenCalledWith("github", "github:imaustink"); }); + it("adopts a chat caller's existing credential onto their principal, so converging costs them no re-auth (docs/adr/0031)", async () => { + // The reported scenario, end to end: this human has been running agents from + // chat for weeks -- their credential sits at the raw `alice` subject, written + // before principals existed -- and triage was prompting them because it reads + // `github:imaustink`. After this, neither flow prompts. + const deps = canonicalDeps({ identityResolver: chatCaller() }, null); + (deps.identityLinkGateway!.start as ReturnType).mockResolvedValue({ + flow: "authcode" as const, + authorizeUrl: "https://gateway.example/identity-link/github/authorize", + expiresInSeconds: 600, + }); + deps.identityLinkGateway!.waitForCompletion = vi + .fn() + .mockResolvedValue({ token: "gh-token", githubLogin: "Imaustink" }); + // A credential that exists ONLY under the pre-principal subject. + const stored = new Map([["alice", { token: "creds-json-from-chat" }]]); + (deps.claudeRemoteGateway!.getToken as ReturnType).mockImplementation( + async (_provider: string, subject: string) => stored.get(subject), + ); + deps.claudeRemoteGateway!.rekey = vi.fn(async (_provider: string, from: string, to: string) => { + const record = stored.get(from); + if (!record) return false; + stored.set(to, record); + stored.delete(from); + return true; + }); + const graph = buildAgentGraph(deps); + + const final = await graph.invoke({ + request: "fix the failing test", + authToken: "tok", + progressListener: vi.fn(), + }); + + expect(final.error).toBeUndefined(); + // No Claude flow was started: the credential they already authorized was + // moved, not re-created. + expect(deps.claudeRemoteGateway!.start).not.toHaveBeenCalled(); + expect(deps.claudeRemoteGateway!.rekey).toHaveBeenCalledWith("claude-remote", "alice", "github:imaustink"); + const launched = (deps.agentRunLauncher!.launch as ReturnType).mock.calls[0]![2] as { + secretEnv?: { name: string; value: string }[]; + }; + expect(launched.secretEnv).toContainEqual({ name: "CLAUDE_LOGIN_CREDENTIALS_JSON", value: "creds-json-from-chat" }); + }); + + it("offers a chat caller with no GitHub mapping the principal link FIRST, before any claude-remote flow (docs/adr/0031)", async () => { + // The production defect, end to end: this caller could authorize Claude in + // chat all day and a triage turn would still prompt, because chat wrote + // `claude-remote@alice` and triage reads `claude-remote@github:`. + // Establishing the mapping first is what makes the two the same record. + const deps = canonicalDeps({ identityResolver: chatCaller() }, null); + (deps.identityLinkGateway!.start as ReturnType).mockResolvedValue({ + flow: "authcode" as const, + authorizeUrl: "https://gateway.example/identity-link/github/authorize", + expiresInSeconds: 600, + }); + const graph = buildAgentGraph(deps); + + const final = await graph.invoke({ + request: "fix the failing test", + authToken: "tok", + progressListener: vi.fn(), + }); + + expect(deps.identityLinkGateway!.start).toHaveBeenCalledWith("github", "alice", "authcode"); + expect(final.pendingIdentityLink).toMatchObject({ provider: "github", subject: "alice" }); + // Nothing was keyed to the subject this caller is one link away from + // abandoning -- that is what would have split the records again. + expect(deps.claudeRemoteGateway!.start).not.toHaveBeenCalled(); + }); + + it("converges the SAME turn once the principal link lands mid-wait", async () => { + const deps = canonicalDeps({ identityResolver: chatCaller() }, null); + (deps.identityLinkGateway!.start as ReturnType).mockResolvedValue({ + flow: "authcode" as const, + authorizeUrl: "https://gateway.example/identity-link/github/authorize", + expiresInSeconds: 600, + }); + // The caller completes the GitHub link in their browser while the turn + // holds open, exactly as the claude flows already do. + deps.identityLinkGateway!.waitForCompletion = vi + .fn() + .mockResolvedValue({ token: "gh-token", githubLogin: "Imaustink" }); + (deps.claudeRemoteGateway!.getToken as ReturnType).mockResolvedValue({ token: "creds-json" }); + const graph = buildAgentGraph(deps); + + const final = await graph.invoke({ + request: "fix the failing test", + authToken: "tok", + progressListener: vi.fn(), + }); + + expect(final.error).toBeUndefined(); + // The key a triage turn reads. Same human, same record, either entry point. + expect(deps.claudeRemoteGateway!.getToken).toHaveBeenCalledWith("claude-remote", "github:imaustink"); + // The mapping stayed a mapping: no GITHUB_TOKEN went to the run, so the + // agent's delegated-write path (the observed 401) stays unreachable. + const launched = (deps.agentRunLauncher!.launch as ReturnType).mock.calls[0]![2] as { + secretEnv?: { name: string }[]; + }; + expect(launched.secretEnv?.map((e) => e.name)).not.toContain("GITHUB_TOKEN"); + // And the upgraded principal is carried on the turn's identity, so a later + // expired-credential invalidate clears the record that was actually read. + expect(final.identity?.principal).toBe("github:imaustink"); + }); + it("starts the link against the canonical subject AND records it on pendingIdentityLink, so store and wait cannot drift (the PR #144 regression)", async () => { const deps = canonicalDeps(); const graph = buildAgentGraph(deps); diff --git a/apps/agent-orchestrator/src/agent/graph.ts b/apps/agent-orchestrator/src/agent/graph.ts index 635cdec..148efc8 100644 --- a/apps/agent-orchestrator/src/agent/graph.ts +++ b/apps/agent-orchestrator/src/agent/graph.ts @@ -1296,6 +1296,18 @@ export function buildAgentGraph(deps: AgentGraphDeps) { } const identitySecretEnv = verdict.secretEnv; + // The pre-flight may have ESTABLISHED the caller's principal on this very + // turn (docs/adr/0031), in which case the credentials it resolved are keyed + // by a principal `state.identity` does not carry yet. Adopt it for the rest + // of this turn: `handleAgentTurnFailure` below re-derives that same key to + // invalidate an expired credential, and deriving the pre-upgrade one would + // delete nothing while telling the user to retry -- the infinite "expired + // credential" loop that path exists to prevent. + const identity = + verdict.principal && verdict.principal !== state.identity.principal + ? { ...state.identity, principal: verdict.principal } + : state.identity; + const runId = randomUUID(); const jobId = randomUUID(); const callbackUrl = `${deps.callbackBaseUrl}/callback/${jobId}`; @@ -1336,6 +1348,7 @@ export function buildAgentGraph(deps: AgentGraphDeps) { const message = composeAgentTurnMessage(state, reply); return { agentRunId: runId, + identity, agentAwaitingReply: !reply.final, result: message, // Only a FINAL reply concludes an episode, so only then is @@ -1347,7 +1360,7 @@ export function buildAgentGraph(deps: AgentGraphDeps) { : {}), }; } catch (err) { - return { agentRunId: runId, ...(await handleAgentTurnFailure(err, deps, state, agent)) }; + return { agentRunId: runId, identity, ...(await handleAgentTurnFailure(err, deps, { ...state, identity }, agent)) }; } }) .addNode("loadSkillTools", async (state) => { diff --git a/apps/agent-orchestrator/src/identity-link/credential-subject.test.ts b/apps/agent-orchestrator/src/identity-link/credential-subject.test.ts index 08927f7..578a326 100644 --- a/apps/agent-orchestrator/src/identity-link/credential-subject.test.ts +++ b/apps/agent-orchestrator/src/identity-link/credential-subject.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it, vi } from "vitest"; -import { canonicalSubjectForLogin, resolveActorLogin, resolvePrincipal } from "./credential-subject.js"; +import { canonicalSubjectForLogin, isCanonicalPrincipal, resolveActorLogin, resolvePrincipal } from "./credential-subject.js"; function githubGateway(githubLogin?: string) { return { getToken: vi.fn().mockResolvedValue(githubLogin ? { token: "gh", githubLogin } : undefined) }; @@ -66,3 +66,15 @@ describe("resolveActorLogin", () => { expect(await resolveActorLogin("openwebui:42", undefined, githubGateway("imaustink"))).toBe("imaustink"); }); }); + +describe("isCanonicalPrincipal", () => { + it("tells a canonical principal apart from a raw entry-point subject", async () => { + // What the authorization pre-flight branches on: only the second shape can + // be shared across entry points, so only the first needs establishing. + expect(isCanonicalPrincipal("github:imaustink")).toBe(true); + expect(isCanonicalPrincipal("openwebui:42")).toBe(false); + expect(isCanonicalPrincipal("client-integration-gateway")).toBe(false); + // The fallback resolvePrincipal returns is, by construction, not canonical. + expect(isCanonicalPrincipal(await resolvePrincipal("openwebui:42", undefined, undefined))).toBe(false); + }); +}); diff --git a/apps/agent-orchestrator/src/identity-link/credential-subject.ts b/apps/agent-orchestrator/src/identity-link/credential-subject.ts index 71ee1dd..a858ac1 100644 --- a/apps/agent-orchestrator/src/identity-link/credential-subject.ts +++ b/apps/agent-orchestrator/src/identity-link/credential-subject.ts @@ -13,7 +13,31 @@ import type { IdentityLinkPort } from "./gateway-client.js"; * subject, the same namespacing discipline `openwebui:` already uses. */ export function canonicalSubjectForLogin(login: string): string { - return `github:${login.toLowerCase()}`; + return `${CANONICAL_PRINCIPAL_PREFIX}${login.toLowerCase()}`; +} + +/** + * Namespace marking a principal as CANONICAL -- resolved from a verified GitHub + * identity -- as opposed to the raw-subject fallback {@link resolvePrincipal} + * returns when no login could be established. + */ +export const CANONICAL_PRINCIPAL_PREFIX = "github:"; + +/** + * Whether a principal is the canonical, cross-entry-point one or merely a raw + * entry-point subject standing in for itself. + * + * The distinction is what the authorization pre-flight acts on: a caller whose + * principal is still their raw subject gets no credential sharing with their + * other entry points, so a turn that needs a cross-entry-point credential can + * offer to establish the mapping first (docs/adr/0031). + * + * A prefix test is sound because every entry-point subject is either namespaced + * by its own resolver (`openwebui:`) or an IdP `sub`, and none of them can + * be `github:` -- that namespace exists solely for principals. + */ +export function isCanonicalPrincipal(principal: string): boolean { + return principal.startsWith(CANONICAL_PRINCIPAL_PREFIX); } /** diff --git a/apps/agent-orchestrator/src/rbac/openwebui-forwarded-user-resolver.test.ts b/apps/agent-orchestrator/src/rbac/openwebui-forwarded-user-resolver.test.ts index c277ec0..4650f15 100644 --- a/apps/agent-orchestrator/src/rbac/openwebui-forwarded-user-resolver.test.ts +++ b/apps/agent-orchestrator/src/rbac/openwebui-forwarded-user-resolver.test.ts @@ -15,13 +15,18 @@ describe("OpenWebUiForwardedUserResolver", () => { const aliceToken = await sign({ id: "alice-id", role: "user" }); const bobToken = await sign({ id: "bob-id", role: "admin" }); + // `perUser` is asserted, not incidental: it is what permits the + // authorization pre-flight to establish a principal against this subject + // (docs/adr/0031), which would be a credential leak on a shared one. await expect(resolver.resolve(aliceToken)).resolves.toEqual({ subject: "openwebui:alice-id", roles: ["reader", "writer"], + perUser: true, }); await expect(resolver.resolve(bobToken)).resolves.toEqual({ subject: "openwebui:bob-id", roles: ["reader", "writer"], + perUser: true, }); }); @@ -45,10 +50,15 @@ describe("OpenWebUiForwardedUserResolver", () => { const subToken = await sign({ sub: "sub-id" }); const emailToken = await sign({ email: "alice@example.com" }); - await expect(resolver.resolve(subToken)).resolves.toEqual({ subject: "openwebui:sub-id", roles: ["reader", "writer"] }); + await expect(resolver.resolve(subToken)).resolves.toEqual({ + subject: "openwebui:sub-id", + roles: ["reader", "writer"], + perUser: true, + }); await expect(resolver.resolve(emailToken)).resolves.toEqual({ subject: "openwebui:alice@example.com", roles: ["reader", "writer"], + perUser: true, }); }); diff --git a/apps/agent-orchestrator/src/rbac/openwebui-forwarded-user-resolver.ts b/apps/agent-orchestrator/src/rbac/openwebui-forwarded-user-resolver.ts index 512830b..8b85265 100644 --- a/apps/agent-orchestrator/src/rbac/openwebui-forwarded-user-resolver.ts +++ b/apps/agent-orchestrator/src/rbac/openwebui-forwarded-user-resolver.ts @@ -63,6 +63,9 @@ export class OpenWebUiForwardedUserResolver implements IdentityResolver { const rawId = payload.id ?? payload.sub ?? payload.email; if (typeof rawId !== "string" || rawId.length === 0) return undefined; - return { subject: `openwebui:${rawId}`, roles: this.roles }; + // `perUser`: this subject came from a per-request JWT Open WebUI mints for + // one signed-in human, so it identifies exactly them -- which is what lets + // the authorization pre-flight establish a principal against it (ADR 0031). + return { subject: `openwebui:${rawId}`, roles: this.roles, perUser: true }; } } diff --git a/apps/agent-orchestrator/src/rbac/types.ts b/apps/agent-orchestrator/src/rbac/types.ts index 89635f6..161fa8f 100644 --- a/apps/agent-orchestrator/src/rbac/types.ts +++ b/apps/agent-orchestrator/src/rbac/types.ts @@ -30,6 +30,23 @@ export interface Identity { * test) still type-checks; consumers fall back to `subject`. */ principal?: string; + /** + * `true` only when this `subject` provably identifies ONE human. + * + * Set by the resolvers that know it structurally -- today + * `OpenWebUiForwardedUserResolver`, which exists precisely to give each Open + * WebUI user their own subject. Absent everywhere else, including the OIDC + * resolver, which cannot tell a human's token from integration-gateway's own + * service-account token: both are validly-signed JWTs with a `sub`. + * + * The authorization pre-flight gates principal ESTABLISHMENT on it + * (docs/adr/0031): filing a GitHub login under a subject that several people + * resolve to would hand that person's Claude credentials to everyone else who + * arrives that way. Fail-closed by omission -- a resolver that says nothing + * gets no linking, which costs cross-entry-point sharing rather than leaking + * a credential. + */ + perUser?: true; } /** diff --git a/docs/adr/0030-authorization-preflight-outside-the-llm.md b/docs/adr/0030-authorization-preflight-outside-the-llm.md index f885aed..4da875c 100644 --- a/docs/adr/0030-authorization-preflight-outside-the-llm.md +++ b/docs/adr/0030-authorization-preflight-outside-the-llm.md @@ -12,6 +12,12 @@ subject) and [0022](0022-per-user-github-device-flow-identity.md) / and Claude delegation). Nothing here is retracted; 0029's canonical subject remains the interim keying until the principal model below lands. +Amended by [0031](0031-principal-establishing-account-link.md): §5's removal of +the `github` provider from `claude-code-swe-agent` fixed the 401 but left chat +callers with no way to learn their own login, so §6's convergence held on the +webhook path only. 0031 makes establishing a principal its own pre-flight step +rather than a side effect of `identityProviders`. + ## Context Authorization currently lives in three places, with overlap: diff --git a/docs/adr/0031-principal-establishing-account-link.md b/docs/adr/0031-principal-establishing-account-link.md new file mode 100644 index 0000000..f213113 --- /dev/null +++ b/docs/adr/0031-principal-establishing-account-link.md @@ -0,0 +1,174 @@ +# 0031. Establishing a principal is its own pre-flight step, not a side effect of `identityProviders` + +Date: 2026-07-25 + +## Status + +Accepted + +Amends [0029](0029-canonical-github-credential-subject.md) (canonical credential +subject) and [0030](0030-authorization-preflight-outside-the-llm.md) §5/§6. + +## Context + +ADR 0029 converged Claude credentials on a canonical `github:` principal +so that authorizing once during GitHub triage is honored in Open WebUI chat and +vice versa. In production, it converged in one direction only, and the +asymmetry left the original bug fully intact for the flow it was reported from: + +``` +15:57:10 [authorization] {"verdict":"authorized", "agentId":"claude-code-swe-agent", "actorLogin":null} +16:12:33 [authorization] {"verdict":"link-required","agentId":"claude-code-swe-agent", "pending":["claude@github:imaustink"]} +``` + +Same human, fifteen minutes apart, one working and one prompting: + +- **The webhook path always has a login.** `senderLogin` rides on `/invoke`'s + event descriptor, so `resolvePrincipal` always returns `github:` and + the turn reads `claude@github:imaustink`. +- **The chat path can only read one off an existing `github` link.** There was + none, so `resolveActorLogin` returned `undefined` (`actorLogin:null` above), + the principal fell back to the raw `openwebui:` subject, and chat happily + kept reading and writing `claude@openwebui:`. + +Neither flow is broken on its own. They simply never meet, permanently, and no +amount of authorizing in one fixes the other. + +What made the chat side unable to learn a login is that a `github` link only +ever came into existence as a **side effect of an Agent's +`identityProviders`** — and ADR 0030 §5 deliberately removed `github` from +`claude-code-swe-agent`'s list, because declaring it to obtain a *mapping* also +provisioned a `GITHUB_TOKEN` into the run and activated the agent's +delegated-write path, which is what produced the observed +`401 Bad credentials`. ADR 0030 named the conflation ("obtaining an identity +mapping and provisioning a credential are different concerns") and removed the +symptom, but the only mechanism for obtaining a mapping was still the one that +provisions a credential. So the fix for the 401 silently switched off cross-flow +sharing for every chat caller. + +## Decision + +Establishing a principal becomes its own step in the authorization pre-flight, +independent of what the Agent declares. + +When a run needs a credential that is keyed by principal +(`CROSS_ENTRY_POINT_PROVIDERS` — `claude`, `claude-remote`) and the caller has +no canonical principal yet, the pre-flight first establishes one with an +ordinary `github` link that is **link-only**: it contributes the login and +nothing else, and its token is never added to `secretEnv`. Mapping and +credential are now separately reachable, which is what ADR 0030 §5 asked for. + +Four properties make this safe, and each is pinned by a test: + +1. **Per-user subjects only.** `Identity` gains `perUser?: true`, asserted by + the resolver that structurally knows it (`OpenWebUiForwardedUserResolver`) + and absent everywhere else — including the OIDC resolver, which cannot tell a + human's token from integration-gateway's own service-account token. Without + it, no principal is established. This is the security core of the change: a + login filed under the shared service subject would be inherited by every + later `senderLogin`-less webhook turn, handing one person's Claude + credentials to everyone — the shape of the leak + [0022](0022-per-user-github-device-flow-identity.md)'s successor fix and the + PR #144/#145 revert both landed on. A live channel (`progressListener`) was + considered as the signal and rejected: a shared subject arriving on a + streaming caller would pass it, so it is unsound in the one direction that + leaks. +2. **The principal step runs first, so CRD provider order stays irrelevant** + (ADR 0030 §4). A `[claude, github]` Agent must not key its Claude credential + before the login is known. +3. **A pending principal link stops the turn there.** The remaining providers + would have to be keyed by a subject the caller is one link away from + abandoning, and starting their flows would file the credentials the user is + about to create under the raw subject — re-creating the very split this + closes. The resume turn re-enters with a canonical principal and assesses + everything then. This is a deliberate exception to §4's batching: batching + assumes the providers are independent, and these are not. +4. **Every failure degrades rather than blocks.** A link that won't start, or + one carrying no login, logs and continues on the raw subject — sharing is an + improvement, not a precondition, and a GitHub OAuth hiccup must not deny a + run whose own credentials are already linked. + +### Existing credentials are moved, not re-authorized + +Both flows reading one key does not by itself put anything at that key. Records +written before principals existed sit under the entry point's own subject, so a +caller who has been running agents from chat for weeks would still have met a +login prompt on the turn after this shipped — for a credential the gateway is +holding the whole time. "Authorize once more, this last time" is the kind of +cost that reads as the bug not being fixed, and it is avoidable. + +The gateway gains `ClaudeTokenStore.rekey` and a bearer-gated +`POST /claude-auth/api/rekey`, and the pre-flight calls it lazily: when nothing +is stored at the principal, it moves the caller's pre-principal record onto it +and re-reads. Design points worth stating: + +- **Lazily, on the turn that needs it, not as a migration job.** The + (subject → principal) mapping is only derivable from a caller's own + authenticated turn; a batch job would have to invent it. +- **Gated on `perUser`, exactly like establishing a principal.** A shared + subject's credential belongs to whoever authorized first, so moving it onto a + sender's principal would hand it to them outright. The webhook path never + adopts — it reads only what its own principal already holds. This is also what + keeps `identity-keying.e2e.ts`'s negative controls meaningful. +- **Moved, not copied.** The `claude-remote` write-back only ever writes the new + key, so a leftover copy would silently rot and then fail whichever flow still + read it with "Login expired" — worse than either alternative. +- **Never overwrites the destination.** A record already at the principal is by + definition at least as current as the one being moved. +- **The source is deleted only after the destination read back.** `set` swallows + its own Redis errors by design, so an unverified delete could drop a human's + only credential on a transient failure — the one outcome strictly worse than + the extra login this avoids. +- **Best-effort throughout.** A failed rekey leaves the credential where it is + and the turn falls back to the ordinary link prompt; it never fails a turn. + +The `authorized` verdict now carries the `principal` the credentials were +actually keyed by, and `delegateToAgent` adopts it for the rest of the turn. +Without that, the expired-credential path would re-derive the pre-upgrade key, +invalidate a record that was never written, and tell the user to retry — the +infinite "expired credential" loop that path exists to prevent. + +## Consequences + +Chat and triage converge on one record for the same human, which is the whole +point. The first chat turn after this change costs that caller exactly one +one-time prompt — link GitHub — and their existing Claude credential is carried +onto the principal underneath it. ADR 0030's "at most one extra prompt per user, +once" migration cost is retired rather than paid: the gateway holds both records +in one store, so moving one is a store operation, not a re-authorization. + +`identityProviders` semantics are unchanged from ADR 0030 and now actually hold: +declaring a provider means "this run needs this credential," never "this run +needs to know who the caller is." + +Costs and limits, stated plainly: + +- **A caller whose subject is not asserted per-user gets no sharing.** Today + that means anyone reaching chat other than through Open WebUI's forwarded-user + JWT. They keep working, on their own per-entry-point keys. +- **A caller who never links GitHub keeps their credential where it is.** No + principal, no adoption, no sharing — and nothing lost. +- **The read side of the mapping still trusts a link under whatever subject it + finds**, including a shared one, if a leftover record exists there + (`resolveActorLogin`). This change never *creates* one there, but it does not + clean up a pre-existing one; the identity-link store has no TTL. Removing that + hazard belongs with ADR 0030 §6's alias table, where a principal stops being + "whatever GitHub link happens to be present." +- **`resolveLinkedCredentials` (the agent-backed-tool path) cannot establish a + principal**, because it must not start a link flow — a paused *tool* call has + no resume slot. It reads whatever principal the turn already has. Same + documented v1 scope cut as before. +- **e2e now drives both entry points.** The webhook-only suite is how a + one-directional convergence shipped in the first place, so `e2e/support/chat.ts` + was added: it mints the per-user `X-OpenWebUI-User-Jwt` Open WebUI signs and + posts a streaming `/v1/chat/completions`. `identity-keying.e2e.ts` gained the + chat-side cases — resolves from the principal, adopts a pre-principal record + without re-prompting, refuses another human's credential, and still works with + no GitHub link at all — plus `chat-harness.e2e.ts`, which checks the harness's + own signing against the real resolver and needs no cluster. The standing rule: + when behaviour differs per entry point, cover it from each of them. +- **The GitHub link is seeded in e2e, not established.** A real device/authcode + round trip needs github.com and a human with a browser. The orchestrator reads + only `githubLogin` off that record, so the path under test is identical; what + is NOT covered end-to-end is the OAuth exchange itself, which was already true + before this change. diff --git a/docs/adr/README.md b/docs/adr/README.md index 887c536..f61398f 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -36,5 +36,6 @@ See [../orchestrator.md](../orchestrator.md) for how these fit together. | [0028](0028-agent-tool-refs-sub-agent-tool-calls.md) | `Agent.spec.toolRefs` lets a sub-agent's own internal loop call Tool CRs directly, via a new `tool_call`/`tool_result` protocol pair and `AgentSession.callTool()` SDK method — no orchestrator Skill/planner involvement | | [0029](0029-canonical-github-credential-subject.md) | Claude credentials (`claude`/`claude-remote`) are keyed by a canonical, lower-cased `github:` subject resolved by one shared helper at every call site — so an authorization done during GitHub-webhook triage is reused in Open WebUI chat and vice versa, instead of each entry point's differing `identity.subject` prompting separately | | [0030](0030-authorization-preflight-outside-the-llm.md) | Authorization becomes a single deterministic pre-flight owned by agent-orchestrator — batch-resolving every provider an Agent CRD declares before launch, handing the run a sealed actor context so agents stop re-deriving identity, with the LLM able only to ask a human to link an account and never to decide authorization or see a credential | +| [0031](0031-principal-establishing-account-link.md) | Establishing the caller's principal is its own authorization pre-flight step — a link-only `github` link, gated on a resolver-asserted per-user subject and independent of `Agent.identityProviders` — so Open WebUI chat can learn the login that keys its Claude credentials and actually converge with GitHub-webhook triage, instead of the two flows sharing in one direction only; a pre-principal credential is MOVED onto the principal (new gateway `rekey`) so converging costs no re-authorization | Status values: `proposed` | `accepted` | `superseded by NNNN`. From 2a718636eddccd64f80527311bfca804096337a5 Mon Sep 17 00:00:00 2001 From: Austin Kurpuis Date: Sat, 25 Jul 2026 17:09:49 -0700 Subject: [PATCH 3/6] e2e: drive the chat entry point, not just webhooks The suite existed specifically to catch credential-keying bugs and could not see this one, for a structural reason: every spec drove GitHub webhooks. The webhook path always resolves a principal, so a convergence that worked in only that direction passed every spec while the chat path -- the one that could not resolve a principal -- was unreachable from a test. `support/chat.ts` closes that: it mints the per-request JWT Open WebUI signs (`X-OpenWebUI-User-Jwt`) and posts a STREAMING `/v1/chat/completions`, which is what makes the caller per-user and gives the turn the live channel the real chat surface has. Hand-rolled HS256 rather than adding `jose` -- e2e/ has kept its dependencies to vitest + typescript, same precedent as sender-assertion.ts. `chat-harness.e2e.ts` checks the harness against the REAL resolver and needs no cluster, so it runs anywhere. Signing is the part of a harness most likely to be quietly wrong, and when it is, every cluster spec built on it fails with "could not resolve caller identity" -- which reads as a product defect and costs a full stack cycle to disprove. Four chat-side cases in identity-keying.e2e.ts: resolves from the principal (and asserts no GITHUB_TOKEN reaches the run), adopts a pre-principal record without re-prompting (asserting the old key is GONE, not copied), refuses another human's credential, and still works with no GitHub link at all. The GitHub link is seeded (`seedGithubLink`), not established: a real device/authcode round trip needs github.com and a human with a browser. The orchestrator reads only `githubLogin` off that record, so the path under test is identical -- what stays uncovered is the OAuth exchange, as before. values-e2e.yaml now configures the forwarded-JWT secret, without which every e2e chat caller collapses onto the shared service subject and the specs would silently exercise the degraded path. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01W7XfNb5HsmaZH7QyxUrbyB --- charts/agent-controller/values-e2e.yaml | 9 ++ e2e/README.md | 49 ++++++++-- e2e/scripts/bootstrap-secrets.sh | 10 +- e2e/specs/chat-harness.e2e.ts | 59 +++++++++++ e2e/specs/identity-keying.e2e.ts | 124 +++++++++++++++++++++++- e2e/support/chat.ts | 104 ++++++++++++++++++++ e2e/support/openwebui-jwt.ts | 55 +++++++++++ e2e/support/redis.ts | 43 +++++++- 8 files changed, 441 insertions(+), 12 deletions(-) create mode 100644 e2e/specs/chat-harness.e2e.ts create mode 100644 e2e/support/chat.ts create mode 100644 e2e/support/openwebui-jwt.ts diff --git a/charts/agent-controller/values-e2e.yaml b/charts/agent-controller/values-e2e.yaml index a5bdceb..b6c1e45 100644 --- a/charts/agent-controller/values-e2e.yaml +++ b/charts/agent-controller/values-e2e.yaml @@ -160,6 +160,15 @@ agent-orchestrator: # identity-keying spec asserts that no Claude credential is ever keyed # under this subject. staticIdentities: '{"e2e-gateway-token":{"subject":"client-integration-gateway","roles":["reader","writer"]}}' + # Makes the CHAT entry point per-user, which is what the suite could not + # drive before (docs/adr/0031): with only the static map above, every chat + # caller collapses onto the shared `client-integration-gateway` subject, so + # the flow whose principal could not be resolved -- the entire bug ADR 0031 + # fixes -- was unreachable from a test. With this set, a chat turn carrying + # `X-OpenWebUI-User-Jwt` resolves to `openwebui:` and is asserted + # `perUser`, exactly as production's Open WebUI does. + openWebUiUserJwtSecretExistingSecret: e2e-openwebui-forward-jwt + openWebUiUserJwtSecretExistingSecretKey: AGENT_OPENWEBUI_USER_JWT_SECRET # Shrunk from the 10-minute default so resilience.e2e.ts can wait out a # silent agent inside a test's budget. It bounds SILENCE, not duration, so a # narrating turn is still unaffected however long it runs -- which is itself diff --git a/e2e/README.md b/e2e/README.md index 97d8078..68de250 100644 --- a/e2e/README.md +++ b/e2e/README.md @@ -15,8 +15,8 @@ stored. Every one of those has shipped broken at least once (PRs #144/#145, ## Safety: these only ever run against minikube -`support/guard.ts` aborts the whole suite unless `kubectl config -current-context` is exactly `minikube`. This is not a convenience check — the +`support/guard.ts` aborts any spec that touches the cluster unless `kubectl +config current-context` is exactly `minikube`. This is not a convenience check — the default context on the maintainer's machine is a **live cluster**, the tests create and delete namespaced objects, and a suite that silently ran there would be destructive. The guard runs before any fixture, and there is @@ -37,6 +37,36 @@ The line is: **stub what we don't own and can't make deterministic; run everything we do own for real.** A happy-path test that stubbed the orchestrator would not have caught any bug from this month. +Two fixtures are *seeded* rather than stubbed, both for the same reason — the +real thing needs a human and a paid third party, and the code path under test +only ever reads back the part we seed: + +| Fixture | Seeded by | What stays real | +| --- | --- | --- | +| A linked Claude credential | `support/redis.ts`'s `seedClaudeCredential` | Everything downstream: which subject the gate reads, whether it launches, what it injects. Asserting "a credential appears" is impossible hermetically, so the assertion is inverted — seed at the subject the gate is believed to use, and require a LAUNCH. A gate looking elsewhere finds nothing and parks. | +| A linked GitHub account | `support/redis.ts`'s `seedGithubLink` | The orchestrator reads only `githubLogin` off that record to resolve a principal (ADR 0031), so a seeded link drives the identical path a real OAuth round trip would produce. | + +## Both entry points, not just webhooks + +The suite drove only GitHub webhooks until ADR 0031, and that gap has a +scalp: credential convergence (ADR 0029) shipped working in **one direction**. +The webhook path always carries a verified `senderLogin` and so always resolved +a principal; chat had no way to learn a login and silently kept keying by its +own `openwebui:` subject. Every unit test passed, the webhook specs passed, +and the bug was reported from chat. + +`support/chat.ts` closes it: it mints the per-request JWT Open WebUI signs +(`X-OpenWebUI-User-Jwt`) and posts a **streaming** `/v1/chat/completions`, which +is what makes the caller per-user and gives the turn the live channel the real +chat surface has. `chat-harness.e2e.ts` checks that minting against the +orchestrator's own resolver and needs no cluster, so a harness/product drift +fails fast instead of surfacing as "could not resolve caller identity" inside +every chat spec. + +A rule worth keeping: **when a behaviour differs per entry point, cover it from +each of them.** Every keying bug in this repo's history has been an asymmetry +between the two. + ## Running ```bash @@ -53,15 +83,18 @@ lists) that concurrent runs would race on. ``` support/ - guard.ts context safety check — imported by every spec - k8s.ts kubectl wrappers, waitFor helpers - redis.ts reads credential/session keys out of the orchestrator's Redis - webhook.ts HMAC-signs and posts GitHub webhook payloads - fixtures.ts per-test namespace-scoped setup/teardown - resilience.ts paces the stub's turn, and DISRUPTS the cluster (NATS, rollouts) + guard.ts context safety check — imported by every CLUSTER spec + k8s.ts kubectl wrappers, waitFor helpers + redis.ts reads credential/session keys out of the orchestrator's Redis + webhook.ts HMAC-signs and posts GitHub webhook payloads + chat.ts drives the CHAT entry point (per-user JWT + streaming /v1/chat/completions) + openwebui-jwt.ts chat.ts's cluster-free half: JWT minting, SSE assembly + fixtures.ts per-test namespace-scoped setup/teardown + resilience.ts paces the stub's turn, and DISRUPTS the cluster (NATS, rollouts) specs/ happy-path.e2e.ts webhook -> triage -> AgentRun -> comment posted identity-keying.e2e.ts which subject each entry point keys credentials under + chat-harness.e2e.ts the harness's own signing, vs. the real resolver (no cluster) resilience.e2e.ts what survives NATS/orchestrator moving mid-turn manifests/ fake-github.yaml in-cluster GitHub API stub (Deployment + Service + script) diff --git a/e2e/scripts/bootstrap-secrets.sh b/e2e/scripts/bootstrap-secrets.sh index ea9df1c..36a4044 100755 --- a/e2e/scripts/bootstrap-secrets.sh +++ b/e2e/scripts/bootstrap-secrets.sh @@ -82,6 +82,14 @@ upsert e2e-integration-gateway-secrets \ --from-literal=GATEWAY_SENDER_ASSERTION_SECRET="$SENDER_ASSERTION_SECRET" \ --from-literal=GITHUB_TOKEN="e2e-not-a-real-token" +# The HS256 secret a chat turn's forwarded-user JWT is signed with. Fixed, and +# read back out of the cluster by e2e/support/chat.ts rather than duplicated +# there: the suite has to sign with whatever the orchestrator verifies against, +# or identity resolution fails closed and the turn 401s in a way that reads as a +# product bug rather than a config mismatch. +upsert e2e-openwebui-forward-jwt \ + --from-literal=AGENT_OPENWEBUI_USER_JWT_SECRET="e2e-openwebui-forward-jwt-secret" + # claude-code-swe-agent's static secret. The GITHUB_APP_* values are # placeholders: no e2e run mints a real installation token. upsert claude-code-swe-secrets \ @@ -96,7 +104,7 @@ if kubectl -n "$NS" get secret agent-orchestrator-secrets >/dev/null 2>&1; then # script must never overwrite. Only the identity-link token is added. kubectl -n "$NS" patch secret agent-orchestrator-secrets \ -p "{\"stringData\":{\"IDENTITY_LINK_GATEWAY_TOKEN\":\"$IDENTITY_LINK_TOKEN\",\"AGENT_SENDER_ASSERTION_SECRET\":\"$SENDER_ASSERTION_SECRET\"}}" >/dev/null - echo " ✓ agent-orchestrator-secrets exists (OPENAI_API_KEY untouched; added IDENTITY_LINK_GATEWAY_TOKEN)" + echo " ✓ agent-orchestrator-secrets exists (OPENAI_API_KEY untouched; added IDENTITY_LINK_GATEWAY_TOKEN + AGENT_SENDER_ASSERTION_SECRET)" else cat >&2 < { + const SECRET = "e2e-openwebui-forward-jwt-secret"; + const resolver = new OpenWebUiForwardedUserResolver({ secret: SECRET, roles: ["reader", "writer"] }); + + it("mints a JWT the resolver accepts, resolving the subject the specs assert on", async () => { + const identity = await resolver.resolve(mintForwardedUserJwt(SECRET, "e2e-chat-user")); + + expect(identity).toEqual({ + subject: "openwebui:e2e-chat-user", + roles: ["reader", "writer"], + // The assertion that makes the chat specs meaningful: without `perUser` + // the pre-flight refuses to establish or adopt anything (docs/adr/0031), + // so a harness that lost this would silently test the degraded path while + // appearing to test convergence. + perUser: true, + }); + }); + + it("is rejected when signed with the wrong secret", async () => { + // Proves the resolver is actually verifying, so the test above is evidence + // of agreement rather than of a resolver that accepts anything. + await expect(resolver.resolve(mintForwardedUserJwt("not-the-deployments-secret", "e2e-chat-user"))).resolves.toBeUndefined(); + }); + + it("assembles the assistant's text out of an OpenAI-style stream", async () => { + const body = [ + 'data: {"choices":[{"delta":{"role":"assistant"}}]}', + 'data: {"choices":[{"delta":{"content":"To continue, please "}}]}', + ": keep-alive", + 'data: {"choices":[{"delta":{"content":"link your GitHub account"}}]}', + 'data: {"choices":[{"delta":{},"finish_reason":"stop"}]}', + "data: [DONE]", + "", + ].join("\n"); + + // Frames with no content delta, comments and the terminator are skipped + // rather than parsed strictly -- a spec asserting on what the human saw + // should not fail over a frame shape it doesn't care about. + expect(assembleSseContent(body)).toBe("To continue, please link your GitHub account"); + }); +}); diff --git a/e2e/specs/identity-keying.e2e.ts b/e2e/specs/identity-keying.e2e.ts index c56fe17..050617a 100644 --- a/e2e/specs/identity-keying.e2e.ts +++ b/e2e/specs/identity-keying.e2e.ts @@ -1,7 +1,8 @@ import { afterAll, beforeAll, beforeEach, describe, expect, it } from "vitest"; import { requireMinikubeContext } from "../support/guard.js"; import { agentRunSecretEnvNames, agentRunsSince, cleanupAgentRunsSince, waitFor, withPortForward } from "../support/k8s.js"; -import { deleteCredentialKeys, seedAllClaudeCredentials } from "../support/redis.js"; +import { chatSubject, chatTurn } from "../support/chat.js"; +import { claudeCredentialSubjects, deleteCredentialKeys, seedAllClaudeCredentials, seedGithubLink } from "../support/redis.js"; import { issueLabeledPayload, postGithubWebhook } from "../support/webhook.js"; import { resetFakeGithub, webhookSecret } from "../support/fixtures.js"; @@ -167,3 +168,124 @@ describe("credential keying converges across entry points (ADR 0029/0030)", () = ); }); }); + +/** + * The CHAT side of the same convergence (docs/adr/0031). + * + * The suite above drives webhooks only, and that is exactly how the one-way + * convergence shipped: the webhook path always carries a verified `senderLogin`, + * so it always resolved a principal, while chat -- which had no way to learn a + * login -- kept keying by its own `openwebui:` subject. Neither flow was + * broken alone, and no webhook-only suite could see it. + * + * ## What is seeded, and why that is still a real test + * + * The GitHub link is seeded rather than established through OAuth: a real + * device/authcode round trip needs github.com and a human with a browser. The + * orchestrator only ever reads `githubLogin` off that record to resolve the + * principal, so a seeded link drives the identical code path -- and everything + * downstream of it (which subject the credential is read from, whether the run + * launches, whether the record moved) is genuine. + */ +describe("chat and triage converge on one credential (ADR 0031)", () => { + const CHAT_USER = "e2e-chat-user"; + const CHAT_SUBJECT = chatSubject(CHAT_USER); + let suiteStartedAt: Date; + + beforeAll(() => { + suiteStartedAt = new Date(); + }); + + afterAll(async () => { + await cleanupAgentRunsSince(suiteStartedAt); + }); + + beforeEach(async () => { + await resetFakeGithub(); + await deleteCredentialKeys("claudeAuth:*"); + await deleteCredentialKeys("claudeAuthLogin:*"); + await deleteCredentialKeys("identityLink:*"); + await deleteCredentialKeys("sess:*"); + }); + + it("resolves a chat turn's credentials from the principal, not the openwebui subject", async () => { + // Seeded ONLY at the canonical subject: if the chat path still keyed by its + // own subject it would find nothing and park, so a launch is the proof. + await seedGithubLink(CHAT_SUBJECT, SENDER); + await seedAllClaudeCredentials(CANONICAL); + const startedAt = new Date(); + + await chatTurn(CHAT_USER, "use the swe agent to fix the failing test"); + + const run = await waitFor( + "a chat-driven AgentRun to launch using the canonically-keyed credentials", + async () => (await agentRunsSince(startedAt))[0], + { timeoutMs: 420_000 }, + ); + const envNames = await agentRunSecretEnvNames(run.name); + expect(envNames).toContain("CLAUDE_CODE_OAUTH_TOKEN"); + expect(envNames).toContain("CLAUDE_LOGIN_CREDENTIALS_JSON"); + // The mapping stayed a mapping: the principal step is link-only, so no + // GITHUB_TOKEN reaches the run and the agent's delegated-write path (the + // observed production 401) stays unreachable. + expect(envNames).not.toContain("GITHUB_TOKEN"); + }); + + it("adopts a pre-principal credential instead of asking the human to authorize again", async () => { + // The reported complaint, as a test: this human authorized from chat before + // principals existed, so their credential sits under `openwebui:`. + // Converging must MOVE it, not re-prompt for it. + await seedGithubLink(CHAT_SUBJECT, SENDER); + await seedAllClaudeCredentials(CHAT_SUBJECT); + const startedAt = new Date(); + + await chatTurn(CHAT_USER, "use the swe agent to fix the failing test"); + + await waitFor( + "the adopted credential to carry a launch with no re-authorization", + async () => (await agentRunsSince(startedAt))[0], + { timeoutMs: 420_000 }, + ); + + // Moved, not copied. A leftover copy is the failure mode that matters: the + // claude-remote write-back only ever writes the new key, so the old one + // would rot and then fail whichever flow still read it. + for (const kind of ["setup-token", "login"] as const) { + const subjects = await claudeCredentialSubjects(kind); + expect(subjects).toContain(CANONICAL); + expect(subjects).not.toContain(CHAT_SUBJECT); + } + }); + + it("does not let a chat caller's principal serve another human's credential", async () => { + // The negative control for adoption. `perUser` permits moving THIS caller's + // record; nothing may pull in one keyed to anyone else. + await seedGithubLink(CHAT_SUBJECT, SENDER); + await seedAllClaudeCredentials("github:someone-else"); + const startedAt = new Date(); + + await chatTurn(CHAT_USER, "use the swe agent to fix the failing test"); + + await new Promise((r) => setTimeout(r, 60_000)); + expect(await agentRunsSince(startedAt)).toHaveLength(0); + // And the other human's credential is untouched -- not moved, not deleted. + expect(await claudeCredentialSubjects("login")).toContain("github:someone-else"); + }); + + it("keys a chat caller with no GitHub link by their own subject, sharing with nobody", async () => { + // The degraded path, asserted rather than assumed: no link means no + // principal, which must still WORK -- just without cross-flow sharing. + await seedAllClaudeCredentials(CHAT_SUBJECT); + const startedAt = new Date(); + + await chatTurn(CHAT_USER, "use the swe agent to fix the failing test"); + + await waitFor( + "an AgentRun keyed by the caller's own subject", + async () => (await agentRunsSince(startedAt))[0], + { timeoutMs: 420_000 }, + ); + // Nothing was moved anywhere: with no principal there is nothing to move to. + expect(await claudeCredentialSubjects("login")).toEqual([CHAT_SUBJECT]); + }); +}); diff --git a/e2e/support/chat.ts b/e2e/support/chat.ts new file mode 100644 index 0000000..d6a6a32 --- /dev/null +++ b/e2e/support/chat.ts @@ -0,0 +1,104 @@ +import { kubectl, withPortForward } from "./k8s.js"; +import { assembleSseContent, mintForwardedUserJwt } from "./openwebui-jwt.js"; + +// Re-exported so a spec has one import for "the chat entry point", while the +// cluster-free helpers stay independently testable (specs/chat-harness.e2e.ts). +export { assembleSseContent, mintForwardedUserJwt } from "./openwebui-jwt.js"; + +/** + * The CHAT entry point, as a test can drive it. + * + * Everything else in `e2e/` drives the webhook path, which is why the one-way + * credential convergence of docs/adr/0031 survived a suite built specifically + * to catch keying bugs: with no way to make a per-user chat call, the flow that + * could not resolve a principal was never exercised. This module is that way. + * + * Two things make a chat turn different from a relayed webhook, and both matter + * to the identity it resolves: + * + * - **Identity comes from a per-request signed JWT**, not the bearer token. Open + * WebUI's `Authorization` value is one static string shared by every one of + * its users, so the orchestrator resolves the caller from the + * `X-OpenWebUI-User-Jwt` header instead (`OpenWebUiForwardedUserResolver`), + * which is what makes the subject per-user and, since ADR 0031, what lets a + * principal be established against it. + * - **It is an OpenAI-shaped `/v1/chat/completions` call**, streaming, so the + * turn holds a live channel the way the real chat surface does. + */ + +const ORCHESTRATOR_SERVICE = "agent-orchestrator-invoke"; +const ORCHESTRATOR_PORT = 8081; +const CHAT_PORT = 18095; +const FORWARDED_USER_JWT_HEADER = "x-openwebui-user-jwt"; + +/** + * Open WebUI's forwarded-user JWT secret, read from the cluster for the same + * reason `webhookSecret()` is: the test has to sign with whatever the + * deployment verifies against, or identity resolution fails closed and the turn + * 401s in a way that looks like a product bug. + */ +async function forwardedUserJwtSecret(): Promise { + const b64 = ( + await kubectl(["get", "secret", "e2e-openwebui-forward-jwt", "-o", "jsonpath={.data.AGENT_OPENWEBUI_USER_JWT_SECRET}"]) + ).trim(); + if (!b64) { + throw new Error( + "e2e: AGENT_OPENWEBUI_USER_JWT_SECRET not found on e2e-openwebui-forward-jwt (run e2e/scripts/bootstrap-secrets.sh)", + ); + } + return Buffer.from(b64, "base64").toString("utf8"); +} + +/** The subject `OpenWebUiForwardedUserResolver` resolves for a given Open WebUI user id. */ +export function chatSubject(userId: string): string { + return `openwebui:${userId}`; +} + +/** + * Sends one streaming chat turn as `userId` and returns the assembled assistant + * text. + * + * Streaming (`stream: true`) on purpose: it is the shape the real Open WebUI + * surface uses, and the only one that gives the turn a live progress channel -- + * so a link prompt surfaces mid-turn exactly as a human would see it, rather + * than only in the final message. + * + * The `sessionId` is a real chat id, defaulted per call rather than fixed: it + * keys session state (active agent run, pending identity link), and a shared + * constant would let one spec's parked link resume inside another's turn. + */ +export async function chatTurn( + userId: string, + request: string, + opts: { sessionId?: string; timeoutMs?: number } = {}, +): Promise { + const secret = await forwardedUserJwtSecret(); + const jwt = mintForwardedUserJwt(secret, userId); + const sessionId = opts.sessionId ?? `e2e-chat-${userId}-${process.pid}`; + + return withPortForward(ORCHESTRATOR_SERVICE, ORCHESTRATOR_PORT, CHAT_PORT, async (baseUrl) => { + const res = await fetch(`${baseUrl}/v1/chat/completions`, { + method: "POST", + headers: { + "content-type": "application/json", + // The shared Open WebUI bearer. Identity does NOT come from this -- the + // JWT header below is what makes the caller per-user -- but the request + // still has to authenticate, and this is the value values-e2e.yaml's + // staticIdentities map registers. + authorization: "Bearer e2e-gateway-token", + "x-chat-id": sessionId, + [FORWARDED_USER_JWT_HEADER]: jwt, + }, + body: JSON.stringify({ model: "agent-orchestrator", stream: true, messages: [{ role: "user", content: request }] }), + // A chat turn that needs a link holds its connection open for the whole + // flow expiry, and a delegated turn waits on a real agent run. Generous, + // but bounded: an unbounded fetch turns a hung turn into a hung suite. + signal: AbortSignal.timeout(opts.timeoutMs ?? 420_000), + }); + if (!res.ok) { + throw new Error(`e2e: chat completion failed: ${res.status} ${await res.text()}`); + } + return assembleSseContent(await res.text()); + }); +} + diff --git a/e2e/support/openwebui-jwt.ts b/e2e/support/openwebui-jwt.ts new file mode 100644 index 0000000..a1f3ba2 --- /dev/null +++ b/e2e/support/openwebui-jwt.ts @@ -0,0 +1,55 @@ +import { createHmac } from "node:crypto"; + +/** + * The parts of the chat harness that touch nothing outside this file. + * + * Split from `chat.ts` (which shells out to kubectl and port-forwards) so they + * can be verified WITHOUT a cluster -- see specs/chat-harness.e2e.ts. Signing is + * the part of a harness most likely to be quietly wrong: a malformed JWT makes + * identity resolution fail closed, and every spec built on it then fails looking + * like a product bug. + */ + +const b64url = (input: Buffer | string): string => Buffer.from(input).toString("base64url"); + +/** + * Mints the HS256 JWT Open WebUI would send for a signed-in user. + * + * Hand-rolled rather than pulling in `jose`: the payload is one claim, the + * algorithm is fixed on both sides, and `e2e/` has deliberately kept its + * dependency list to vitest + typescript. Same no-new-dependency precedent as + * the orchestrator's own `sender-assertion.ts`. + */ +export function mintForwardedUserJwt(secret: string, userId: string): string { + const header = b64url(JSON.stringify({ alg: "HS256", typ: "JWT" })); + // `id` is the claim the resolver prefers; `role` is Open WebUI's own + // vocabulary and is deliberately ignored by the resolver (this system's RBAC + // roles come from configuration, not from the header). + const payload = b64url(JSON.stringify({ id: userId, role: "user" })); + const signature = createHmac("sha256", secret).update(`${header}.${payload}`).digest("base64url"); + return `${header}.${payload}.${signature}`; +} + +/** + * Concatenates the `content` deltas out of an OpenAI-style SSE stream. + * + * Tolerant by design: `[DONE]`, keep-alives and any chunk without a content + * delta are skipped rather than parsed strictly. A test asserting on what the + * human saw should not fail over a frame shape it doesn't care about. + */ +export function assembleSseContent(body: string): string { + let text = ""; + for (const line of body.split("\n")) { + if (!line.startsWith("data:")) continue; + const data = line.slice("data:".length).trim(); + if (!data || data === "[DONE]") continue; + try { + const chunk = JSON.parse(data) as { choices?: { delta?: { content?: string } }[] }; + const delta = chunk.choices?.[0]?.delta?.content; + if (typeof delta === "string") text += delta; + } catch { + // Not a JSON frame -- nothing to assemble from it. + } + } + return text; +} diff --git a/e2e/support/redis.ts b/e2e/support/redis.ts index 4a26fd5..303185f 100644 --- a/e2e/support/redis.ts +++ b/e2e/support/redis.ts @@ -80,7 +80,15 @@ export async function claudeCredentialSubjects(kind: "setup-token" | "login"): P * LAUNCHES. If the gate looked anywhere else it would find nothing and park. * That tests the behaviour rather than a log line. */ -export async function seedClaudeCredential(subject: string, kind: "setup-token" | "login"): Promise { +/** + * The field encryptor both stores share: AES-256-GCM under the deployment's own + * `IDENTITY_LINK_ENCRYPTION_KEY`, packed `iv:authTag:ciphertext` in base64. + * + * Read from the cluster rather than generated, because these records are read + * back by the gateway -- a locally-invented key produces blobs it can only fail + * to decrypt, which surfaces as "no credential linked" rather than as a test bug. + */ +async function fieldEncrypter(): Promise<(plaintext: string) => string> { const keyB64 = ( await kubectl(["get", "secret", "e2e-integration-gateway-secrets", "-o", "jsonpath={.data.IDENTITY_LINK_ENCRYPTION_KEY}"]) ).trim(); @@ -89,12 +97,16 @@ export async function seedClaudeCredential(subject: string, kind: "setup-token" if (key.length !== 32) throw new Error(`e2e: encryption key decoded to ${key.length} bytes, expected 32`); const { createCipheriv, randomBytes } = await import("node:crypto"); - const encrypt = (plaintext: string): string => { + return (plaintext: string): string => { const iv = randomBytes(12); const cipher = createCipheriv("aes-256-gcm", key, iv); const ct = Buffer.concat([cipher.update(plaintext, "utf8"), cipher.final()]); return `${iv.toString("base64")}:${cipher.getAuthTag().toString("base64")}:${ct.toString("base64")}`; }; +} + +export async function seedClaudeCredential(subject: string, kind: "setup-token" | "login"): Promise { + const encrypt = await fieldEncrypter(); const record = kind === "login" @@ -105,6 +117,33 @@ export async function seedClaudeCredential(subject: string, kind: "setup-token" await redisSet(`${prefix}${subject}`, JSON.stringify(record)); } +/** + * Seeds a `github` identity link for a subject, carrying `githubLogin`. + * + * This is what gives a CHAT caller a resolvable principal (docs/adr/0031) + * without a real GitHub OAuth round trip -- which no hermetic test can perform. + * The orchestrator only ever reads `githubLogin` off this record to establish + * the mapping, so a seeded link exercises exactly the same code path a linked + * one would; the token itself is never used by the paths under test (the + * principal step is deliberately link-only, and it injects nothing). + * + * Written in `RedisIdentityLinkStore`'s own format: `identityLink::`, + * with the token fields AES-256-GCM field-encrypted the way that store writes them. + */ +export async function seedGithubLink(subject: string, githubLogin: string): Promise { + const encrypt = await fieldEncrypter(); + await redisSet( + `identityLink:github:${subject}`, + JSON.stringify({ + githubLogin, + // Far future: an expired link is treated as no link, which would make the + // spec fail for a reason that has nothing to do with what it tests. + expiresAt: new Date(Date.now() + 365 * 24 * 60 * 60 * 1000).toISOString(), + token: encrypt("gho_e2e-seeded"), + }), + ); +} + /** Seeds BOTH Claude record kinds for one subject -- what claude-code-swe-agent declares. */ export async function seedAllClaudeCredentials(subject: string): Promise { await seedClaudeCredential(subject, "setup-token"); From 036f3d2d09fb187effe1c0991912a78540d1e4b0 Mon Sep 17 00:00:00 2001 From: Austin Kurpuis Date: Sat, 25 Jul 2026 17:09:49 -0700 Subject: [PATCH 4/6] Make the sender-assertion secret part of the documented bootstrap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Observed on the live deployment: neither AGENT_SENDER_ASSERTION_SECRET nor GATEWAY_SENDER_ASSERTION_SECRET is present, so both processes log their startup warning and the orchestrator trusts the sender login from the /invoke request BODY, unsigned. That field selects the caller's principal and therefore which stored Claude credentials a run receives (ADR 0030 §6, 0031) -- so anything holding the gateway's /invoke token can name a login and be handed that person's credentials. The mechanism has existed since ADR 0030; what was missing is that these Secrets are hand-created and nothing told anyone to include the key. e2e's bootstrap already sets it on both sides, so minikube was exercising the signed path while production ran the unsigned one. Adds it to values-production.yaml and README's bootstrap (with why, and the matching-value requirement), plus a script that performs the change on a live deployment: one generated value, both Secrets, digest-compared before anything rolls, then gateway first and orchestrator second. That order is load-bearing. Gateway-set/orchestrator-unset is exactly today's behaviour (the gateway signs, the orchestrator ignores the header and keeps reading the body field). The reverse makes the orchestrator require a signature nobody is producing, so every webhook turn loses its sender login, falls back to the shared service subject, and re-prompts for authorization. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01W7XfNb5HsmaZH7QyxUrbyB --- README.md | 8 +- .../agent-controller/values-production.yaml | 29 ++++- scripts/provision-sender-assertion-secret.sh | 123 ++++++++++++++++++ 3 files changed, 157 insertions(+), 3 deletions(-) create mode 100755 scripts/provision-sender-assertion-secret.sh diff --git a/README.md b/README.md index ffdf52e..8c56775 100644 --- a/README.md +++ b/README.md @@ -194,13 +194,19 @@ cluster, in your own terminal (never paste real secrets into chat or files): ```bash # Generate a random callback HMAC secret CALLBACK_SECRET=$(openssl rand -hex 32) +# ...and the secret the gateway signs a webhook's sender login with, which the +# orchestrator verifies (docs/adr/0030 §6). ONE value, set on both sides: it +# decides which human a webhook turn acts as, and hence whose stored Claude +# credentials the run gets. Left unset, that login is trusted unsigned. +SENDER_ASSERTION_SECRET=$(openssl rand -hex 32) kubectl create namespace controller-agent # OpenAI key + callback HMAC secret (used by the orchestrator) kubectl -n controller-agent create secret generic agent-orchestrator-secrets \ --from-literal=OPENAI_API_KEY= \ - --from-literal=AGENT_CALLBACK_SECRET="$CALLBACK_SECRET" + --from-literal=AGENT_CALLBACK_SECRET="$CALLBACK_SECRET" \ + --from-literal=AGENT_SENDER_ASSERTION_SECRET="$SENDER_ASSERTION_SECRET" # Mealie long-lived API token (create at /user/profile/api-tokens in your Mealie instance) kubectl -n controller-agent create secret generic recipe-publisher-secrets \ diff --git a/charts/agent-controller/values-production.yaml b/charts/agent-controller/values-production.yaml index 5115408..20d6f0e 100644 --- a/charts/agent-controller/values-production.yaml +++ b/charts/agent-controller/values-production.yaml @@ -35,12 +35,28 @@ agent-orchestrator: # Bring-your-own secret: OPENAI_API_KEY, AGENT_CALLBACK_SECRET (required), # AGENT_QDRANT_API_KEY (optional), IDENTITY_LINK_GATEWAY_TOKEN (required # once identityLink.enabled below is true -- see that block's comment). + # + # AGENT_SENDER_ASSERTION_SECRET is required in practice too, and its absence + # is a real weakness rather than a missing feature: without it the orchestrator + # trusts the sender login from the /invoke request BODY, unsigned. That field + # selects the caller's principal and therefore which stored Claude credentials + # a run receives (docs/adr/0030 §6, 0031), so anything holding the gateway's + # /invoke token could name a login and be handed that person's credentials. + # Both processes log a startup WARNING while it is unset. Must match + # integration-gateway-secrets' GATEWAY_SENDER_ASSERTION_SECRET below exactly -- + # the gateway signs with it and the orchestrator verifies with it, so two + # different values present as "the sender login vanished" (every webhook turn + # falls back to the shared service subject and re-prompts) rather than as a + # signature error. + # # Create/update with: # kubectl create secret generic agent-orchestrator-secrets -n controller-agent \ # --from-literal=OPENAI_API_KEY=sk-... \ # --from-literal=AGENT_CALLBACK_SECRET= \ # --from-literal=IDENTITY_LINK_GATEWAY_TOKEN= + # integration-gateway-secrets' GATEWAY_IDENTITY_LINK_TOKEN below> \ + # --from-literal=AGENT_SENDER_ASSERTION_SECRET= # # (or `kubectl edit secret agent-orchestrator-secrets -n controller-agent` # # / patch it if the Secret already exists) secrets: @@ -293,6 +309,14 @@ integration-gateway: # token; the gateway exchanges this for a fresh access token itself as # needed, so this key does not go stale on its own. # + # GATEWAY_SENDER_ASSERTION_SECRET signs the gateway's claim about WHICH HUMAN + # triggered a webhook turn (docs/adr/0030 §6). Any random string; must match + # agent-orchestrator-secrets' AGENT_SENDER_ASSERTION_SECRET above exactly. + # While it is unset the login rides to /invoke unsigned and the orchestrator + # trusts it from the request body -- see that Secret's comment for why that + # matters. Safe to set here FIRST: the orchestrator ignores the header until + # its own key exists, and the gateway keeps sending the body field either way. + # # Per-user GitHub identity linking (docs/adr/0022, identityLink.enabled # below) needs FOUR more keys in this same Secret: GATEWAY_IDENTITY_LINK_TOKEN # (any random string -- must match agent-orchestrator-secrets' @@ -316,7 +340,8 @@ integration-gateway: # --from-literal=GATEWAY_IDENTITY_LINK_TOKEN="$(openssl rand -hex 32)" \ # --from-literal=IDENTITY_LINK_ENCRYPTION_KEY="$(openssl rand -base64 32)" \ # --from-literal=IDENTITY_LINK_STATE_SECRET="$(openssl rand -base64 32)" \ - # --from-literal=GITHUB_APP_CLIENT_SECRET= + # --from-literal=GITHUB_APP_CLIENT_SECRET= \ + # --from-literal=GATEWAY_SENDER_ASSERTION_SECRET="$(openssl rand -hex 32)" # # (or `kubectl edit secret integration-gateway-secrets -n controller-agent` # # if it already exists -- just add/replace the relevant keys) secrets: diff --git a/scripts/provision-sender-assertion-secret.sh b/scripts/provision-sender-assertion-secret.sh new file mode 100755 index 0000000..af1a220 --- /dev/null +++ b/scripts/provision-sender-assertion-secret.sh @@ -0,0 +1,123 @@ +#!/usr/bin/env bash +# provision-sender-assertion-secret.sh — close the unsigned-sender-login gap on a +# live deployment (docs/adr/0030 §6). +# +# Usage: ./scripts/provision-sender-assertion-secret.sh [namespace] +# +# ## What it fixes +# +# integration-gateway authenticates to agent-orchestrator's /invoke with its own +# service token, so that token says "the gateway is calling" and nothing about +# the human who applied the label. The sender login therefore travels separately. +# With no shared secret configured, it travels in the request BODY, unsigned -- +# and that field selects the caller's principal, hence which stored Claude +# credentials the run receives (docs/adr/0031). Anything holding the gateway's +# /invoke token can name a login and be handed that person's credentials. Both +# processes log a startup WARNING while it is unset. +# +# Setting one shared secret on both sides makes the orchestrator require a +# signed assertion and ignore the body field entirely. +# +# ## Why the gateway is patched first +# +# The two are only meaningful together, and the intermediate states are not +# symmetric: +# +# gateway set, orchestrator unset -> gateway signs, orchestrator ignores the +# header and keeps trusting the body field. +# Harmless: exactly today's behaviour. +# gateway unset, orchestrator set -> orchestrator requires a signature nobody +# is producing, so every webhook turn loses +# its sender login, falls back to the shared +# service subject, and re-prompts for auth. +# +# So: gateway first, orchestrator second. Both are patched here before either +# rolls, which keeps that window to the rollout itself. +# +# Idempotent: re-running rotates the secret (both sides at once, so they never +# disagree). Existing keys in both Secrets are left untouched -- this PATCHES, +# it never recreates. + +set -euo pipefail + +NS="${1:-controller-agent}" +ORCH_SECRET="agent-orchestrator-secrets" +GW_SECRET="integration-gateway-secrets" +ORCH_DEPLOY="agent-orchestrator" +GW_DEPLOY="agent-controller-integration-gateway" + +CTX="$(kubectl config current-context 2>/dev/null || true)" +echo "Context: ${CTX:-}" +echo "Namespace: $NS" +echo "" +read -r -p "Patch the sender-assertion secret into both Secrets and roll both deployments? [y/N] " reply +[[ "$reply" == "y" || "$reply" == "Y" ]] || { echo "Aborted."; exit 1; } + +for s in "$GW_SECRET" "$ORCH_SECRET"; do + kubectl -n "$NS" get secret "$s" >/dev/null 2>&1 || { + echo "✗ Secret $s not found in $NS. Nothing patched." >&2 + echo " These Secrets are hand-created -- see charts/agent-controller/values-production.yaml." >&2 + exit 1 + } +done + +# ONE value, generated once, written to both. Generating per-secret is the +# failure this whole script exists to avoid: the gateway would sign with one key +# and the orchestrator verify with another, which presents as "the sender login +# vanished" (every webhook turn re-prompts) rather than as a signature error. +SECRET="$(openssl rand -hex 32)" + +kubectl -n "$NS" patch secret "$GW_SECRET" \ + -p "{\"stringData\":{\"GATEWAY_SENDER_ASSERTION_SECRET\":\"$SECRET\"}}" >/dev/null +echo " ✓ $GW_SECRET" +kubectl -n "$NS" patch secret "$ORCH_SECRET" \ + -p "{\"stringData\":{\"AGENT_SENDER_ASSERTION_SECRET\":\"$SECRET\"}}" >/dev/null +echo " ✓ $ORCH_SECRET" +unset SECRET + +# Compare what actually landed, by digest, so a typo/encoding slip is caught +# here rather than as re-prompting webhook turns later. Never prints the value. +gw_digest="$(kubectl -n "$NS" get secret "$GW_SECRET" -o jsonpath='{.data.GATEWAY_SENDER_ASSERTION_SECRET}' | shasum | cut -c1-12)" +orch_digest="$(kubectl -n "$NS" get secret "$ORCH_SECRET" -o jsonpath='{.data.AGENT_SENDER_ASSERTION_SECRET}' | shasum | cut -c1-12)" +if [[ "$gw_digest" != "$orch_digest" ]]; then + echo "✗ The two Secrets hold DIFFERENT values ($gw_digest vs $orch_digest). Not rolling anything." >&2 + echo " Fix the Secrets first: rolling now would break every webhook turn's sender login." >&2 + exit 1 +fi +echo " ✓ both sides match (digest $gw_digest)" + +# The env vars are `secretKeyRef` with `optional: true`, so a running pod does +# not pick up a newly-added key -- it has to restart. +echo "" +echo "Rolling $GW_DEPLOY (starts signing; orchestrator still accepts the body field)..." +kubectl -n "$NS" rollout restart "deployment/$GW_DEPLOY" +kubectl -n "$NS" rollout status "deployment/$GW_DEPLOY" --timeout=180s + +echo "" +echo "Rolling $ORCH_DEPLOY (starts REQUIRING the signature)..." +kubectl -n "$NS" rollout restart "deployment/$ORCH_DEPLOY" +kubectl -n "$NS" rollout status "deployment/$ORCH_DEPLOY" --timeout=300s + +echo "" +echo "Verifying the startup warnings are gone..." +if kubectl -n "$NS" logs "deployment/$ORCH_DEPLOY" -c agent-orchestrator --since=5m 2>/dev/null \ + | grep -q "AGENT_SENDER_ASSERTION_SECRET is not set"; then + echo " ✗ agent-orchestrator still reports the secret as unset" >&2 + exit 1 +fi +echo " ✓ agent-orchestrator no longer warns" +if kubectl -n "$NS" logs "deployment/$GW_DEPLOY" --since=5m 2>/dev/null \ + | grep -q "GATEWAY_SENDER_ASSERTION_SECRET is not set"; then + echo " ✗ integration-gateway still reports the secret as unset" >&2 + exit 1 +fi +echo " ✓ integration-gateway no longer warns" + +cat < -- now from a verified assertion rather than an +unsigned body field. Watch it with: + + kubectl -n $NS logs deployment/$ORCH_DEPLOY -c agent-orchestrator -f | grep authorization +EOF From f9f6177704790b203215f4c3655c3519d2ac44bc Mon Sep 17 00:00:00 2001 From: Austin Kurpuis Date: Sat, 25 Jul 2026 17:22:53 -0700 Subject: [PATCH 5/6] e2e: make the chat specs' agent selection legible, not lucky A webhook turn reaches its agent through an IntegrationRoute; chat has no such thing. `/v1/chat/completions` carries no event descriptor, and a session's `activeAgentId` only CONTINUES an existing run rather than launching one, so selection goes through the planner -- which the rest of the suite deliberately never depends on. Two fixes, since the surface offers no deterministic hook: - The request names the target agent outright, which is as close to deterministic as chat gets. - Every launch assertion goes through `expectCredentialAgent`, which asserts the launched `spec.agentRef` is one that actually declares the credentials under test, and FAILS NAMING what the planner picked instead. The e2e catalog holds three agents; two declare `claude`/`claude-remote` and either is a valid outcome for what these specs test (which SUBJECT a credential came from), while `opencode-swe-agent` declares none -- picking it previously surfaced as a confusing complaint about missing env vars, or as a 420s timeout with nothing saying which agent ran. The non-determinism is now stated in the spec's own doc rather than left for the next person to discover from a flake. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01W7XfNb5HsmaZH7QyxUrbyB --- e2e/specs/identity-keying.e2e.ts | 59 +++++++++++++++++++++----------- e2e/support/k8s.ts | 13 +++++++ 2 files changed, 52 insertions(+), 20 deletions(-) diff --git a/e2e/specs/identity-keying.e2e.ts b/e2e/specs/identity-keying.e2e.ts index 050617a..6acc694 100644 --- a/e2e/specs/identity-keying.e2e.ts +++ b/e2e/specs/identity-keying.e2e.ts @@ -1,6 +1,6 @@ import { afterAll, beforeAll, beforeEach, describe, expect, it } from "vitest"; import { requireMinikubeContext } from "../support/guard.js"; -import { agentRunSecretEnvNames, agentRunsSince, cleanupAgentRunsSince, waitFor, withPortForward } from "../support/k8s.js"; +import { agentRunAgentRef, agentRunSecretEnvNames, agentRunsSince, cleanupAgentRunsSince, waitFor, withPortForward } from "../support/k8s.js"; import { chatSubject, chatTurn } from "../support/chat.js"; import { claudeCredentialSubjects, deleteCredentialKeys, seedAllClaudeCredentials, seedGithubLink } from "../support/redis.js"; import { issueLabeledPayload, postGithubWebhook } from "../support/webhook.js"; @@ -186,12 +186,43 @@ describe("credential keying converges across entry points (ADR 0029/0030)", () = * principal, so a seeded link drives the identical code path -- and everything * downstream of it (which subject the credential is read from, whether the run * launches, whether the record moved) is genuine. + * + * ## The one thing here that is NOT deterministic + * + * A webhook turn reaches its agent through an `IntegrationRoute`, so the specs + * above never depend on a model choosing correctly. Chat has no such thing -- + * `/v1/chat/completions` carries no event descriptor, and a session's + * `activeAgentId` only CONTINUES an existing run rather than launching one -- + * so selection goes through the planner. `REQUEST` below names the target agent + * outright to get as close to deterministic as the surface allows, and every + * launch assertion goes through `expectCredentialAgent`, which fails naming the + * agent that actually ran. Two of the three agents in the e2e catalog declare + * the cross-entry-point providers and either is a valid outcome for what these + * specs test (WHICH SUBJECT a credential came from); `opencode-swe-agent` + * declares none, and picking it would otherwise fail as a confusing complaint + * about missing env vars. */ describe("chat and triage converge on one credential (ADR 0031)", () => { const CHAT_USER = "e2e-chat-user"; const CHAT_SUBJECT = chatSubject(CHAT_USER); + /** Names the agent outright -- see this describe's doc on planner-driven selection. */ + const REQUEST = "Delegate this to stub-agent: fix the failing test in e2e-org/e2e-repo"; + /** Agents whose launch proves a principal-keyed credential resolved: the ones declaring `claude`/`claude-remote`. */ + const CREDENTIAL_AGENTS = ["stub-agent", "claude-code-swe-agent"]; let suiteStartedAt: Date; + /** + * Waits for a launch and asserts it was an agent that actually needs the + * credentials under test, so a planner mis-pick names itself rather than + * surfacing as a missing-env-var failure. + */ + async function expectCredentialAgent(startedAt: Date, what: string): Promise<{ name: string }> { + const run = await waitFor(what, async () => (await agentRunsSince(startedAt))[0], { timeoutMs: 420_000 }); + const ref = await agentRunAgentRef(run.name); + expect(CREDENTIAL_AGENTS, `planner selected ${ref}`).toContain(ref); + return run; + } + beforeAll(() => { suiteStartedAt = new Date(); }); @@ -215,13 +246,9 @@ describe("chat and triage converge on one credential (ADR 0031)", () => { await seedAllClaudeCredentials(CANONICAL); const startedAt = new Date(); - await chatTurn(CHAT_USER, "use the swe agent to fix the failing test"); + await chatTurn(CHAT_USER, REQUEST); - const run = await waitFor( - "a chat-driven AgentRun to launch using the canonically-keyed credentials", - async () => (await agentRunsSince(startedAt))[0], - { timeoutMs: 420_000 }, - ); + const run = await expectCredentialAgent(startedAt, "a chat-driven AgentRun to launch using the canonically-keyed credentials"); const envNames = await agentRunSecretEnvNames(run.name); expect(envNames).toContain("CLAUDE_CODE_OAUTH_TOKEN"); expect(envNames).toContain("CLAUDE_LOGIN_CREDENTIALS_JSON"); @@ -239,13 +266,9 @@ describe("chat and triage converge on one credential (ADR 0031)", () => { await seedAllClaudeCredentials(CHAT_SUBJECT); const startedAt = new Date(); - await chatTurn(CHAT_USER, "use the swe agent to fix the failing test"); + await chatTurn(CHAT_USER, REQUEST); - await waitFor( - "the adopted credential to carry a launch with no re-authorization", - async () => (await agentRunsSince(startedAt))[0], - { timeoutMs: 420_000 }, - ); + await expectCredentialAgent(startedAt, "the adopted credential to carry a launch with no re-authorization"); // Moved, not copied. A leftover copy is the failure mode that matters: the // claude-remote write-back only ever writes the new key, so the old one @@ -264,7 +287,7 @@ describe("chat and triage converge on one credential (ADR 0031)", () => { await seedAllClaudeCredentials("github:someone-else"); const startedAt = new Date(); - await chatTurn(CHAT_USER, "use the swe agent to fix the failing test"); + await chatTurn(CHAT_USER, REQUEST); await new Promise((r) => setTimeout(r, 60_000)); expect(await agentRunsSince(startedAt)).toHaveLength(0); @@ -278,13 +301,9 @@ describe("chat and triage converge on one credential (ADR 0031)", () => { await seedAllClaudeCredentials(CHAT_SUBJECT); const startedAt = new Date(); - await chatTurn(CHAT_USER, "use the swe agent to fix the failing test"); + await chatTurn(CHAT_USER, REQUEST); - await waitFor( - "an AgentRun keyed by the caller's own subject", - async () => (await agentRunsSince(startedAt))[0], - { timeoutMs: 420_000 }, - ); + await expectCredentialAgent(startedAt, "an AgentRun keyed by the caller's own subject"); // Nothing was moved anywhere: with no principal there is nothing to move to. expect(await claudeCredentialSubjects("login")).toEqual([CHAT_SUBJECT]); }); diff --git a/e2e/support/k8s.ts b/e2e/support/k8s.ts index 460ccad..570f7fe 100644 --- a/e2e/support/k8s.ts +++ b/e2e/support/k8s.ts @@ -135,6 +135,19 @@ export async function withPortForward( } } +/** + * The Agent a launched AgentRun is for (`spec.agentRef`). + * + * Needed by any spec whose turn reaches an agent through the PLANNER rather + * than an IntegrationRoute: a wrong selection otherwise fails as a bare + * assertion about missing env vars, or as a wait that times out, with nothing + * saying which agent actually ran. + */ +export async function agentRunAgentRef(agentRunName: string): Promise { + const cr = await kubectlJson<{ spec?: { agentRef?: string } }>(["get", "agentrun", agentRunName]); + return cr.spec?.agentRef; +} + /** * `spec.secretEnv` entry NAMES on an AgentRun CR. * From 2303fdedf365572d220dbcdadb30f63ce520a586 Mon Sep 17 00:00:00 2001 From: Austin Kurpuis Date: Sat, 25 Jul 2026 18:01:13 -0700 Subject: [PATCH 6/6] e2e: fix two harness defects the first minikube run exposed Neither was visible to typecheck, and both made a chat spec fail (or worse, pass) for reasons that had nothing to do with the code under test. 1. `opencode-swe-agent` was in the e2e catalog, and the planner preferred it over the agent the request named -- its description is the closest match for any "fix the failing test in a repo" text. It declares no identityProviders, so the pre-flight iterates an empty list and launches with no credential resolved at all. Pointing the triage ROUTE away from it was enough while every spec arrived through an IntegrationRoute; a chat turn has no route. That is not a weaker version of the test but a different, silently-passing one, so the agent is now disabled in the e2e overlay entirely: an agent that cannot exercise the behaviour under test does not belong in the catalog the tests select from. 2. `chatTurn` treated a parked turn as a failure. A turn that needs an account link and has a live channel deliberately holds its connection open for the whole flow expiry waiting for the human -- so for the negative control, whose expected outcome is "no launch", parking IS the product working. It now returns `undefined` under an explicit `allowPark`, leaving a genuine hang in any other spec still failing loudly. Getting that right took two goes, both worth recording: the abort fires on the BODY READ, not on `fetch` (the orchestrator flushes SSE headers immediately, so `fetch` resolves as soon as they land), and `AbortSignal.timeout` rejects with a DOMException that fails `instanceof Error`. Either alone made `allowPark` a silent no-op. The park window also doubles as the "give it time to launch if it were going to" wait, which cut that spec from 845s to 93s. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01W7XfNb5HsmaZH7QyxUrbyB --- charts/community-components/values-e2e.yaml | 17 +++++ e2e/specs/identity-keying.e2e.ts | 8 ++- e2e/support/chat.ts | 79 +++++++++++++++------ 3 files changed, 79 insertions(+), 25 deletions(-) diff --git a/charts/community-components/values-e2e.yaml b/charts/community-components/values-e2e.yaml index 2bd16f2..8b32edd 100644 --- a/charts/community-components/values-e2e.yaml +++ b/charts/community-components/values-e2e.yaml @@ -40,6 +40,23 @@ stubAgent: - claude - claude-remote +# Removed from the e2e catalog entirely, not merely bypassed by the triage route. +# +# The comment at the top of this file explains why it is useless for the +# identity-keying spec: it declares no identityProviders, so the pre-flight +# iterates an empty list and launches with nothing to assert a subject against. +# Re-pointing the route at stub-agent was enough while every spec arrived through +# an IntegrationRoute -- but a CHAT turn has no route, so its agent is chosen by +# the planner, and observed runs picked THIS agent over the named stub (its +# description is the closest match for any "fix the failing repo" request). +# A turn that lands here produces no credential flow at all, which is not a +# weaker version of the test -- it is a different, silently-passing one. +# +# An agent that cannot exercise the behaviour under test does not belong in the +# catalog the tests select from. +opencodeSweAgent: + enabled: false + claudeCodeSweAgent: # Still enabled, though the triage route points at stub-agent above: this # Agent CR is the production-shaped reference the stub's providers are kept in diff --git a/e2e/specs/identity-keying.e2e.ts b/e2e/specs/identity-keying.e2e.ts index 6acc694..9257074 100644 --- a/e2e/specs/identity-keying.e2e.ts +++ b/e2e/specs/identity-keying.e2e.ts @@ -287,9 +287,13 @@ describe("chat and triage converge on one credential (ADR 0031)", () => { await seedAllClaudeCredentials("github:someone-else"); const startedAt = new Date(); - await chatTurn(CHAT_USER, REQUEST); + // The turn is EXPECTED to park: with nothing at this caller's principal and + // nothing adoptable, the pre-flight starts a link flow and holds open for the + // human to finish it. That window doubles as the "give it real time to launch + // if it were going to" wait -- absence is the assertion, so it must not be a + // race, but it also needn't cost the flow's full 10-minute expiry. + expect(await chatTurn(CHAT_USER, REQUEST, { allowPark: true, timeoutMs: 90_000 })).toBeUndefined(); - await new Promise((r) => setTimeout(r, 60_000)); expect(await agentRunsSince(startedAt)).toHaveLength(0); // And the other human's credential is untouched -- not moved, not deleted. expect(await claudeCredentialSubjects("login")).toContain("github:someone-else"); diff --git a/e2e/support/chat.ts b/e2e/support/chat.ts index d6a6a32..ebd4479 100644 --- a/e2e/support/chat.ts +++ b/e2e/support/chat.ts @@ -66,39 +66,72 @@ export function chatSubject(userId: string): string { * The `sessionId` is a real chat id, defaulted per call rather than fixed: it * keys session state (active agent run, pending identity link), and a shared * constant would let one spec's parked link resume inside another's turn. + * + * Resolves `undefined` when the turn PARKS -- only if `allowPark` is set. A turn + * that needs an account link and has a live channel deliberately holds open for + * the whole flow expiry waiting for the human to finish, so for any spec whose + * expected outcome is "no launch", parking is the product working and there is + * no completed reply to return. Without `allowPark` that same timeout rethrows, + * so a positive spec that hangs still fails loudly instead of silently reading + * as "no reply". */ export async function chatTurn( userId: string, request: string, - opts: { sessionId?: string; timeoutMs?: number } = {}, -): Promise { + opts: { sessionId?: string; timeoutMs?: number; allowPark?: boolean } = {}, +): Promise { const secret = await forwardedUserJwtSecret(); const jwt = mintForwardedUserJwt(secret, userId); const sessionId = opts.sessionId ?? `e2e-chat-${userId}-${process.pid}`; return withPortForward(ORCHESTRATOR_SERVICE, ORCHESTRATOR_PORT, CHAT_PORT, async (baseUrl) => { - const res = await fetch(`${baseUrl}/v1/chat/completions`, { - method: "POST", - headers: { - "content-type": "application/json", - // The shared Open WebUI bearer. Identity does NOT come from this -- the - // JWT header below is what makes the caller per-user -- but the request - // still has to authenticate, and this is the value values-e2e.yaml's - // staticIdentities map registers. - authorization: "Bearer e2e-gateway-token", - "x-chat-id": sessionId, - [FORWARDED_USER_JWT_HEADER]: jwt, - }, - body: JSON.stringify({ model: "agent-orchestrator", stream: true, messages: [{ role: "user", content: request }] }), - // A chat turn that needs a link holds its connection open for the whole - // flow expiry, and a delegated turn waits on a real agent run. Generous, - // but bounded: an unbounded fetch turns a hung turn into a hung suite. - signal: AbortSignal.timeout(opts.timeoutMs ?? 420_000), - }); - if (!res.ok) { - throw new Error(`e2e: chat completion failed: ${res.status} ${await res.text()}`); + try { + const res = await fetch(`${baseUrl}/v1/chat/completions`, { + method: "POST", + headers: { + "content-type": "application/json", + // The shared Open WebUI bearer. Identity does NOT come from this -- the + // JWT header below is what makes the caller per-user -- but the request + // still has to authenticate, and this is the value values-e2e.yaml's + // staticIdentities map registers. + authorization: "Bearer e2e-gateway-token", + "x-chat-id": sessionId, + [FORWARDED_USER_JWT_HEADER]: jwt, + }, + body: JSON.stringify({ model: "agent-orchestrator", stream: true, messages: [{ role: "user", content: request }] }), + // A chat turn that needs a link holds its connection open for the whole + // flow expiry, and a delegated turn waits on a real agent run. Generous, + // but bounded: an unbounded fetch turns a hung turn into a hung suite. + signal: AbortSignal.timeout(opts.timeoutMs ?? 420_000), + }); + if (!res.ok) { + throw new Error(`e2e: chat completion failed: ${res.status} ${await res.text()}`); + } + // The body read has to be INSIDE the try: the orchestrator flushes SSE + // headers immediately, so `fetch` resolves as soon as they land and a + // parked turn times out here, not above. Catching only around `fetch` made + // `allowPark` silently ineffective. + // + // A park discards whatever had streamed so far, including any link prompt. + // Fine for asserting "no launch"; a spec that wants to assert on the + // prompt text would need to read the stream incrementally instead. + return assembleSseContent(await res.text()); + } catch (err) { + // Distinguish "the turn parked" from every other failure. Only a caller + // that expects a park may swallow it; anything else is a real fault and + // must surface, including a genuine hang. + // + // Checked by NAME rather than `instanceof Error`: `AbortSignal.timeout` + // rejects with a DOMException, and undici may surface it wrapped, so the + // reason can sit on `cause`. + if (opts.allowPark && isAbort(err)) return undefined; + throw err; } - return assembleSseContent(await res.text()); }); } +/** Whether a rejection is an abort/timeout, however undici chose to wrap it. */ +function isAbort(err: unknown): boolean { + const names = [(err as { name?: string } | undefined)?.name, (err as { cause?: { name?: string } } | undefined)?.cause?.name]; + return names.some((name) => name === "TimeoutError" || name === "AbortError"); +}