diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index ab81e66..e01f873 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -292,8 +292,96 @@ jobs: # convention needs the files physically present before `helm # dependency update`/`helm upgrade` will pick them up. Same step # publish-charts runs before packaging. + # + # This only covers a FIRST install. See the next step for why. run: cp controllers/core-controller/config/crd/bases/*.yaml charts/agent-controller/charts/core-controller/crds/ + - name: Apply CRDs + # Helm's crds/ directory is INSTALL-ONLY: `helm upgrade` never creates a + # newly-added CRD and never updates an existing one + # (charts/agent-controller/charts/core-controller/crds/README.md). On a + # cluster whose release predates a CRD, that CRD simply never arrives, + # and a CRD that gained a field keeps serving the old schema -- so the + # API server silently PRUNES the new field out of every CR written to + # it. A missing field is not a visible failure; it is a resource that + # applies cleanly and then behaves as though half its spec were never + # set. + # + # Until now the only thing closing that gap was a human remembering to + # run kubectl by hand. values-production.yaml carried FOUR separate + # "applied manually once" comments to that effect -- for + # integrationroutes, for Agent.initContainers, and for + # IntegrationRoute.match.labelName. Every one of those was a deploy that + # looked green and shipped a cluster the charts did not describe. + # + # --server-side: the target cluster's CRDs have INCONSISTENT provenance, + # which is precisely why a single client-side apply cannot be trusted + # to reconcile them. Field managers observed on the live cluster: + # - tools/skills/toolruns/localtools -> "helm" only, no + # last-applied-configuration annotation. A client-side apply here + # diffs against an empty baseline: able to add and change fields, + # but unable to REMOVE one dropped from the CRD, so a deleted + # property would linger in the served schema indefinitely. + # - agentruns/agents -> "helm" AND "kubectl-client-side-apply": + # Helm-installed, then hand-patched later. + # - integrationroutes -> "kubectl-client-side-apply" ONLY, with no + # helm manager at all. That CRD was never installed by Helm on + # this cluster; it arrived entirely by hand. + # Those last two rows are the four "applied manually once" comments in + # values-production.yaml, still legible in the cluster's own field + # ownership. Server-side apply diffs against real field ownership + # rather than an annotation that only some objects carry. It is also + # what e2e/scripts/up.sh already uses, so dev and CD converge. + # --force-conflicts: consequently those fields are owned by other + # managers (helm, and on three of them kubectl-client-side-apply too), + # and server-side apply refuses to take them over without this. Safe + # here because every one of those managers wrote the same generated + # artifact -- the chart's copy and this one both come from the same + # *_types.go source of truth -- so there is no third version of the + # truth to lose. Verified by applying all 7 as the deploy runner's own + # ServiceAccount against the live cluster (server dry-run): all + # succeed, with a non-fatal one-time annotation-migration warning on + # the hand-patched ones, which resolves itself on the next apply. + run: | + # Preflight the one permission this step needs, before touching + # anything. CRDs are CLUSTER-scoped; every other kubectl/helm call in + # this job is namespaced, so this is the first thing here to require + # cluster-scoped rights and the runner's ServiceAccount may well not + # have them. Without this check the failure is a raw RBAC error from + # whichever CRD happened to be applied first -- accurate, but it names + # a resource rather than the missing grant. The ServiceAccount is the + # ARC runner pod's own (there is no kubeconfig step in this job) and + # is cluster infrastructure, not something this repo can grant itself. + # `-A` only to keep the check quiet: without it kubectl warns that + # customresourcedefinitions is not namespace-scoped (the job's context + # carries a namespace). stderr is deliberately NOT suppressed, so a + # connectivity failure here is still visible rather than being + # misreported as a permissions problem. + if ! kubectl auth can-i create customresourcedefinitions.apiextensions.k8s.io -A >/dev/null; then + echo "::error::The deploy runner's ServiceAccount cannot write CRDs, so this step cannot apply them." + echo "CRDs are cluster-scoped; the rest of this job is namespaced, which is why nothing caught this earlier." + echo "Grant the ARC runner's ServiceAccount a ClusterRole with:" + echo " apiGroups: [\"apiextensions.k8s.io\"]" + echo " resources: [\"customresourcedefinitions\"]" + echo " verbs: [\"get\", \"list\", \"create\", \"update\", \"patch\"]" + echo "Until then every CRD change must be applied by hand, which is the gap this step exists to close." + exit 1 + fi + + kubectl apply --server-side --force-conflicts \ + -f controllers/core-controller/config/crd/bases/ + + # An established CRD is the precondition for community-components' + # Tool/Skill/Agent/IntegrationRoute CRs below. Without the wait, a + # freshly-created CRD can still be un-established when Helm applies + # the first CR against it, which surfaces as a "no matches for kind" + # failure that succeeds on a re-run -- the shape of flake that gets + # dismissed as infrastructure noise. + for crd in controllers/core-controller/config/crd/bases/*.yaml; do + name=$(kubectl get -f "$crd" -o jsonpath='{.metadata.name}') + kubectl wait --for=condition=established --timeout=60s "crd/$name" + done + - name: Fetch agent-controller chart dependencies run: | # Helm doesn't recurse into file:// subchart dependencies, so the @@ -302,17 +390,21 @@ jobs: helm dependency update charts/agent-controller/charts/agent-orchestrator helm dependency update charts/agent-controller + # --create-namespace on both: `-n controller-agent` does not create the + # namespace, so on a cluster that has never had this stack the very first + # deploy fails on a missing namespace rather than bootstrapping it. A + # no-op once it exists. - name: Helm upgrade agent-controller run: | helm upgrade --install agent-controller charts/agent-controller \ -f charts/agent-controller/values-production.yaml \ - -n controller-agent --timeout 5m + -n controller-agent --create-namespace --timeout 5m - name: Helm upgrade community-components run: | helm upgrade --install community-components charts/community-components \ -f charts/community-components/values-production.yaml \ - -n controller-agent --timeout 5m + -n controller-agent --create-namespace --timeout 5m # Tool/Agent Jobs (recipe-scraper, web-search, web-fetch, # recipe-publisher, opencode-swe-agent) are launched fresh per-invocation diff --git a/.github/workflows/validate-crds.yml b/.github/workflows/validate-crds.yml index 72c0582..4b29621 100644 --- a/.github/workflows/validate-crds.yml +++ b/.github/workflows/validate-crds.yml @@ -37,24 +37,40 @@ jobs: # charts/community-components is the only place these Tool/Skill/Agent # CRs are defined now (no more standalone tools/*/tool.yaml or - # apps/**/config/samples/*.yaml twins to drift out of sync) -- render - # it with every optional component enabled so this validates the full - # catalog, then dry-run apply against a real API server. + # apps/**/config/samples/*.yaml twins to drift out of sync) -- render it + # with every component enabled so this validates the full catalog, then + # dry-run apply against a real API server. + # + # The enable list is a values file rather than a wall of --set flags + # because as flags it went stale without complaint: it enabled two values + # that no longer exist while never enabling six templates that do, + # including the Agent production actually runs. - name: Render community-components with every component enabled run: | helm template community-components charts/community-components \ - --set recipeScraper.enabled=true \ - --set kubectlReadonly.enabled=true \ - --set recipePublisher.enabled=true \ - --set recipePublisher.mealieBaseUrl=http://mealie.example.invalid \ - --set skills.recipeRefining.enabled=true \ - --set skills.softwareEngineering.enabled=true \ - --set skills.selfImprovement.enabled=true \ - --set opencodeSweAgent.enabled=true \ - --set opencodeSweAgentTool.enabled=true \ - --set integrationRoutes.githubIssueLabeledTriage.enabled=true \ - --set integrationRoutes.githubPrLabeledReview.enabled=true \ + -f charts/community-components/values-ci-all.yaml \ > /tmp/community-components-rendered.yaml + - name: Assert every template was actually rendered + # The guard that keeps values-ci-all.yaml honest. "Validated the full + # catalog" is a claim about coverage, and a disabled component produces + # no output to notice the absence of -- so an unvalidated template looks + # exactly like a passing one. Comparing the templates on disk against + # the `# Source:` markers in the render turns that silence into a + # failure. + run: | + missing=() + for f in charts/community-components/templates/*.yaml; do + base=$(basename "$f") + grep -q "# Source: community-components/templates/$base" /tmp/community-components-rendered.yaml || missing+=("$base") + done + if [ ${#missing[@]} -gt 0 ]; then + echo "::error::These templates rendered nothing, so they were NOT validated:" + printf ' %s\n' "${missing[@]}" + echo "Enable them in charts/community-components/values-ci-all.yaml." + exit 1 + fi + echo "All $(ls charts/community-components/templates/*.yaml | wc -l | tr -d ' ') templates rendered." + - name: Validate rendered CRs against the API server run: kubectl apply --dry-run=server -f /tmp/community-components-rendered.yaml diff --git a/apps/agent-orchestrator/src/agent/authorization-service.test.ts b/apps/agent-orchestrator/src/agent/authorization-service.test.ts new file mode 100644 index 0000000..2488fd1 --- /dev/null +++ b/apps/agent-orchestrator/src/agent/authorization-service.test.ts @@ -0,0 +1,428 @@ +import { describe, expect, it, vi } from "vitest"; +import type { IdentityLinkPort, IdentityLinkStartResult, IdentityLinkToken } from "../identity-link/gateway-client.js"; +import type { Identity } from "../rbac/types.js"; +import { ACTOR_LOGIN_ENV, AuthorizationService } from "./authorization-service.js"; + +/** + * Unit tests for the extracted authorization pre-flight (docs/adr/0030 §1). + * + * These are deliberately about the DECISION, not the launch: graph.test.ts + * already covers the end-to-end "what did delegateToAgent hand the launcher" + * behaviour, and duplicating that here would mean two places to update for one + * change. What's tested here is what the extraction made directly reachable -- + * the verdict for each shape of provider set, including the ones that were + * previously only observable through a launched AgentRun. + */ + +const identity = (over: Partial = {}): Identity => + ({ subject: "openwebui:alice", roles: ["writer"], ...over }) as Identity; + +/** A gateway whose per-provider behaviour is declared up front. */ +function gateway( + behaviour: Record< + string, + { + token?: IdentityLinkToken; + start?: IdentityLinkStartResult | "throw"; + waitResolvesTo?: IdentityLinkToken | "throw"; + } + >, +): IdentityLinkPort { + return { + getToken: vi.fn(async (provider: string) => behaviour[provider]?.token), + start: vi.fn(async (provider: string) => { + const s = behaviour[provider]?.start; + if (s === "throw") throw new Error(`start failed for ${provider}`); + return s ?? ({ flow: "authcode", authorizeUrl: `https://link/${provider}`, expiresInSeconds: 600 } as IdentityLinkStartResult); + }), + poll: vi.fn(async () => "pending" as const), + waitForCompletion: vi.fn(async (provider: string) => { + const w = behaviour[provider]?.waitResolvesTo; + if (w === "throw") throw new Error("fetch failed"); + return w; + }), + }; +} + +describe("AuthorizationService.authorize", () => { + it("clears a launch and injects each provider's credential under its own env var", async () => { + const svc = new AuthorizationService({ + claudeAuthGateway: gateway({ claude: { token: { token: "sk-ant-oat01-x" } } }), + claudeRemoteGateway: gateway({ "claude-remote": { token: { token: '{"creds":1}' } } }), + }); + + const verdict = await svc.authorize({ + agent: { id: "swe", identityProviders: ["claude", "claude-remote"] }, + identity: identity(), + request: "do the thing", + }); + + expect(verdict.kind).toBe("authorized"); + if (verdict.kind !== "authorized") return; + expect(verdict.secretEnv).toEqual([ + { name: "CLAUDE_CODE_OAUTH_TOKEN", value: "sk-ant-oat01-x" }, + { name: "CLAUDE_LOGIN_CREDENTIALS_JSON", value: '{"creds":1}' }, + ]); + }); + + 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" }); + }); + + it("keys cross-entry-point providers by principal and github by raw subject (§6)", async () => { + // The PR #144 regression in miniature: a claude credential linked from chat + // must be found by a webhook turn, so it keys on the principal; a github + // link is a property of the account that established it and keys on the + // subject -- keying it by principal would be circular, since the principal + // is resolved FROM it. + 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 }); + + await svc.authorize({ + agent: { id: "swe", identityProviders: ["github", "claude"] }, + identity: identity({ subject: "openwebui:alice", principal: "github:alice" }), + request: "r", + }); + + expect(github.getToken).toHaveBeenCalledWith("github", "openwebui:alice"); + expect(claude.getToken).toHaveBeenCalledWith("claude", "github:alice"); + }); + + it("starts EVERY missing link on one turn and reports them together (§4)", async () => { + // The batching property. Before ADR 0030 the first gap returned, so a + // caller with two unlinked providers spent two round trips discovering them + // one at a time. + const claude = gateway({ claude: {} }); + const remote = gateway({ "claude-remote": {} }); + const svc = new AuthorizationService({ claudeAuthGateway: claude, claudeRemoteGateway: remote }); + + const verdict = await svc.authorize({ + agent: { id: "swe", identityProviders: ["claude", "claude-remote"] }, + identity: identity(), + request: "r", + }); + + expect(verdict.kind).toBe("link-required"); + if (verdict.kind !== "link-required") return; + expect(claude.start).toHaveBeenCalled(); + expect(remote.start).toHaveBeenCalled(); + expect(verdict.message).toContain("2 accounts"); + }); + + it("reports a failed start ALONGSIDE the links that succeeded, not instead of them", async () => { + // The exact coupling ADR 0030 removes: a GitHub OAuth outage used to end the + // turn before claude was ever assessed, so it blocked Claude authorization + // entirely. + const github = gateway({ github: { start: "throw" } }); + const claude = gateway({ claude: {} }); + const svc = new AuthorizationService({ identityLinkGateway: github, claudeAuthGateway: claude }); + + const verdict = await svc.authorize({ + agent: { id: "swe", identityProviders: ["github", "claude"] }, + identity: identity(), + request: "r", + }); + + expect(verdict.kind).toBe("link-required"); + if (verdict.kind !== "link-required") return; + // The Claude link is still offered... + expect(verdict.message).toContain("link your Claude account"); + // ...and the GitHub failure is reported too, as an "also". + expect(verdict.message).toContain("I also couldn't start the GitHub linking step"); + // The resume anchor is the started link, not the failed one. + expect(verdict.pending?.provider).toBe("claude"); + }); + + it("stands the failure message on its own when EVERY provider failed to start", async () => { + const svc = new AuthorizationService({ identityLinkGateway: gateway({ github: { start: "throw" } }) }); + const verdict = await svc.authorize({ + agent: { id: "swe", identityProviders: ["github"] }, + identity: identity(), + request: "r", + }); + + expect(verdict.kind).toBe("link-required"); + if (verdict.kind !== "link-required") return; + expect(verdict.message).toMatch(/^I couldn't start the one-time GitHub account-linking step/); + // Nothing started, so there is no anchor to resume against -- re-triggering + // re-enters the gate and retries start(). + expect(verdict.pending).toBeUndefined(); + }); + + it("resumes the same turn when a streaming caller completes the link during the wait", async () => { + const claude = gateway({ claude: { waitResolvesTo: { token: "sk-ant-oat01-late" } } }); + const progress = vi.fn(); + const svc = new AuthorizationService({ claudeAuthGateway: claude }); + + const verdict = await svc.authorize({ + agent: { id: "swe", identityProviders: ["claude"] }, + identity: identity(), + request: "r", + progressListener: progress, + }); + + // The link was surfaced live, then the wait landed the token -- so the turn + // proceeds instead of parking. + expect(progress).toHaveBeenCalledWith("identity-link", expect.stringContaining("link your Claude account")); + expect(verdict.kind).toBe("authorized"); + }); + + it("parks pending rather than failing when the long-held wait throws", async () => { + // A rolled gateway pod or a dropped idle connection surfaces as "fetch + // failed". That does not mean the LINK failed -- the user can still finish + // it in the browser -- so it must degrade to pending, not to an error. + const claude = gateway({ claude: { waitResolvesTo: "throw" } }); + const svc = new AuthorizationService({ claudeAuthGateway: claude }); + + const verdict = await svc.authorize({ + agent: { id: "swe", identityProviders: ["claude"] }, + identity: identity(), + request: "r", + progressListener: vi.fn(), + }); + + expect(verdict.kind).toBe("link-required"); + }); + + it("does not repeat a link in the final message when it was already surfaced live", async () => { + // The "doubled up" prompt: the streamed message already carries the link. + const svc = new AuthorizationService({ claudeAuthGateway: gateway({ claude: {} }) }); + const verdict = await svc.authorize({ + agent: { id: "swe", identityProviders: ["claude"] }, + identity: identity(), + request: "r", + progressListener: vi.fn(), + }); + + expect(verdict.kind).toBe("link-required"); + if (verdict.kind !== "link-required") return; + expect(verdict.message).not.toContain("https://link/claude"); + expect(verdict.message).toContain("haven't received your Claude account link yet"); + }); + + it("signals that a link is needed before the (possibly slow) start runs", async () => { + const order: string[] = []; + const svc = new AuthorizationService({ + claudeAuthGateway: { + ...gateway({ claude: {} }), + start: vi.fn(async () => { + order.push("start"); + return { flow: "page", pageUrl: "https://p", expiresInSeconds: 600 } as IdentityLinkStartResult; + }), + }, + }); + + await svc.authorize({ + agent: { id: "swe", identityProviders: ["claude"] }, + identity: identity(), + request: "r", + reportIdentityLinkPending: () => order.push("reported"), + }); + + expect(order).toEqual(["reported", "start"]); + }); + + it("captures the subject start() was called with onto the resume anchor", async () => { + // Recomputing the subject downstream instead of carrying it is the PR #144 + // re-auth loop. + const svc = new AuthorizationService({ claudeAuthGateway: gateway({ claude: {} }) }); + const verdict = await svc.authorize({ + agent: { id: "swe", identityProviders: ["claude"] }, + identity: identity({ subject: "openwebui:alice", principal: "github:alice" }), + request: "the original goal", + }); + + expect(verdict.kind).toBe("link-required"); + if (verdict.kind !== "link-required") return; + expect(verdict.pending?.subject).toBe("github:alice"); + // And the goal, so the eventual resume re-delegates THIS request rather than + // whatever text the turn that notices completion happens to carry. + expect(verdict.pending?.request).toBe("the original goal"); + }); + + it("carries a device flow's code onto the anchor so the resume can poll it", async () => { + const svc = new AuthorizationService({ + identityLinkGateway: gateway({ + github: { + start: { + flow: "device", + verificationUri: "https://github.com/login/device", + userCode: "ABCD-1234", + deviceCode: "dev-code", + expiresInSeconds: 900, + pollIntervalSeconds: 5, + }, + }, + }), + }); + + const verdict = await svc.authorize({ + agent: { id: "swe", identityProviders: ["github"] }, + identity: identity(), + request: "r", + identityLinkFlow: "device", + }); + + expect(verdict.kind).toBe("link-required"); + if (verdict.kind !== "link-required") return; + expect(verdict.pending?.deviceCode).toBe("dev-code"); + expect(verdict.message).toContain("ABCD-1234"); + }); + + it("rejects a declared provider with no configured gateway as misconfigured, not unlinked", async () => { + // No user action fixes this, so it must not render as a link prompt. + const svc = new AuthorizationService({}); + const verdict = await svc.authorize({ + agent: { id: "swe", identityProviders: ["claude"] }, + identity: identity(), + request: "r", + }); + + expect(verdict.kind).toBe("misconfigured"); + if (verdict.kind !== "misconfigured") return; + expect(verdict.error).toContain("no identity-link gateway is configured"); + }); + + it("rejects an unknown provider that has no env var mapping", async () => { + const svc = new AuthorizationService({ identityLinkGateway: gateway({ gitlab: { token: { token: "t" } } }) }); + const verdict = await svc.authorize({ + agent: { id: "swe", identityProviders: ["gitlab"] }, + identity: identity(), + request: "r", + }); + + expect(verdict.kind).toBe("misconfigured"); + if (verdict.kind !== "misconfigured") return; + expect(verdict.error).toContain('unsupported identity provider "gitlab"'); + }); +}); + +describe("AuthorizationService.authorize -- sealed actor context (§5)", () => { + it("takes the actor login off the resolved github link, with no extra API call", async () => { + const github = gateway({ github: { token: { token: "gho_x", githubLogin: "Alice" } } }); + const svc = new AuthorizationService({ identityLinkGateway: github }); + + const verdict = await svc.authorize({ + agent: { id: "swe", identityProviders: ["github"] }, + identity: identity(), + request: "r", + }); + + expect(verdict.kind).toBe("authorized"); + if (verdict.kind !== "authorized") return; + expect(verdict.actorLogin).toBe("Alice"); + expect(verdict.secretEnv).toContainEqual({ name: ACTOR_LOGIN_ENV, value: "Alice" }); + }); + + it("still resolves an actor login for an agent that declares no github provider", async () => { + // claude-code-swe-agent's real shape after ADR 0030 decoupled `github` from + // credential provisioning: no github provider declared, but the agent still + // must be told who the caller is so it never calls /user itself. + const svc = new AuthorizationService({ + claudeAuthGateway: gateway({ claude: { token: { token: "sk-ant-oat01-x" } } }), + identityLinkGateway: gateway({ github: { token: { token: "gho_x", githubLogin: "bob" } } }), + }); + + const verdict = await svc.authorize({ + agent: { id: "swe", identityProviders: ["claude"] }, + identity: identity({ subject: "github:bob" }), + request: "r", + }); + + expect(verdict.kind).toBe("authorized"); + if (verdict.kind !== "authorized") return; + expect(verdict.secretEnv).toContainEqual({ name: ACTOR_LOGIN_ENV, value: "bob" }); + }); +}); + +describe("AuthorizationService.authorize -- claude-remote write-back grant", () => { + it("mints the grant against the SAME subject the credential was read from", async () => { + // A grant minted against the raw subject would write refreshed credentials + // to a record nothing ever reads, and the read side would keep serving the + // pre-refresh copy until it died. + const createWritebackGrant = vi.fn(async () => ({ url: "https://wb", token: "wb-token" })); + const svc = new AuthorizationService({ + claudeRemoteGateway: gateway({ "claude-remote": { token: { token: "{}" } } }), + claudeRemoteWriteback: { createWritebackGrant }, + agentRunTimeoutSeconds: 600, + }); + + const verdict = await svc.authorize({ + agent: { id: "swe", identityProviders: ["claude-remote"] }, + identity: identity({ subject: "openwebui:alice", principal: "github:alice" }), + request: "r", + }); + + expect(createWritebackGrant).toHaveBeenCalledWith("github:alice", 600 + 15 * 60); + if (verdict.kind !== "authorized") return; + expect(verdict.secretEnv).toContainEqual({ name: "CLAUDE_CREDENTIALS_WRITEBACK_URL", value: "https://wb" }); + expect(verdict.secretEnv).toContainEqual({ name: "CLAUDE_CREDENTIALS_WRITEBACK_TOKEN", value: "wb-token" }); + }); + + it("launches without write-back rather than failing when no grant is available", async () => { + const svc = new AuthorizationService({ + claudeRemoteGateway: gateway({ "claude-remote": { token: { token: "{}" } } }), + claudeRemoteWriteback: { createWritebackGrant: vi.fn(async () => undefined) }, + }); + + const verdict = await svc.authorize({ + agent: { id: "swe", identityProviders: ["claude-remote"] }, + identity: identity(), + request: "r", + }); + + expect(verdict.kind).toBe("authorized"); + if (verdict.kind !== "authorized") return; + expect(verdict.secretEnv?.map((e) => e.name)).toEqual(["CLAUDE_LOGIN_CREDENTIALS_JSON"]); + }); +}); + +describe("AuthorizationService.resolveLinkedCredentials", () => { + it("resolves what is already linked without ever starting a flow", async () => { + const claude = gateway({ claude: { token: { token: "sk-ant-oat01-x" } } }); + const svc = new AuthorizationService({ claudeAuthGateway: claude }); + + const res = await svc.resolveLinkedCredentials({ identity: identity(), identityProviders: ["claude"] }); + + expect(res).toEqual({ kind: "resolved", secretEnv: [{ name: "CLAUDE_CODE_OAUTH_TOKEN", value: "sk-ant-oat01-x" }] }); + // The v1 scope cut, asserted rather than described: a paused TOOL call has + // no session slot to resume a started link against. + expect(claude.start).not.toHaveBeenCalled(); + }); + + it("reports the unlinked provider instead of composing a message", async () => { + // The caller's error text names the TOOL, which the service has no business + // knowing. + const svc = new AuthorizationService({ claudeAuthGateway: gateway({ claude: {} }) }); + const res = await svc.resolveLinkedCredentials({ identity: identity(), identityProviders: ["claude"] }); + expect(res).toEqual({ kind: "not-linked", provider: "claude" }); + }); + + it("uses the same principal-vs-subject keying as authorize()", async () => { + const claude = gateway({ claude: { token: { token: "t" } } }); + const svc = new AuthorizationService({ claudeAuthGateway: claude }); + + await svc.resolveLinkedCredentials({ + identity: identity({ subject: "openwebui:alice", principal: "github:alice" }), + identityProviders: ["claude"], + }); + + expect(claude.getToken).toHaveBeenCalledWith("claude", "github:alice"); + }); + + it("distinguishes a missing gateway from an unlinked account", async () => { + const svc = new AuthorizationService({}); + expect(await svc.resolveLinkedCredentials({ identity: identity(), identityProviders: ["claude"] })).toEqual({ + kind: "gateway-missing", + provider: "claude", + }); + }); + + it("resolves to nothing for a tool that declares no providers", async () => { + const svc = new AuthorizationService({}); + expect(await svc.resolveLinkedCredentials({ identity: identity() })).toEqual({ kind: "resolved" }); + }); +}); diff --git a/apps/agent-orchestrator/src/agent/authorization-service.ts b/apps/agent-orchestrator/src/agent/authorization-service.ts new file mode 100644 index 0000000..93615d1 --- /dev/null +++ b/apps/agent-orchestrator/src/agent/authorization-service.ts @@ -0,0 +1,543 @@ +import type { IdentityLinkPort, IdentityLinkStartResult } from "../identity-link/gateway-client.js"; +import { resolveActorLogin } from "../identity-link/credential-subject.js"; +import type { Identity } from "../rbac/types.js"; + +/** + * The authorization pre-flight: given the caller and the Agent they are about + * to invoke, decide whether the launch may proceed and, if so, exactly which + * credentials it carries (docs/adr/0030 §1). + * + * ## Why this is a class and not a tool + * + * ADR 0030 §1's property is that authorization has ONE owner and that owner is + * graph control flow -- never something the planner can select, skip, or + * re-order. That property already held before this file existed: the logic ran + * inline in `delegateToAgent`, on the only path to a launch. What it lacked was + * a name and a boundary, so "where is authorization decided" was answered by + * reading 300 lines of a graph node, and the property was true by inspection + * rather than by construction. + * + * Extracting it changes no behaviour. It makes the property legible: the + * decision has a single entry point ({@link authorize}), a total return type + * ({@link AuthorizationOutcome}) whose cases the caller must handle, and no + * dependency on graph state beyond what {@link AuthorizationRequest} names. + * Nothing here is reachable from a model-selected code path -- there is no tool + * schema, no prompt, and the class is constructed once from deps in index.ts. + * + * ## What it deliberately does NOT do + * + * It does not launch, and it does not compose the user-facing turn beyond the + * link prompts it owns. Keeping the launch in the graph node keeps this class's + * contract to "decide", so a future caller can ask for an authorization verdict + * without side effects. + */ + +/** Env var carrying the caller's resolved GitHub login into a run (docs/adr/0030 §5). */ +export const ACTOR_LOGIN_ENV = "AGENT_ACTOR_LOGIN"; + +/** + * Providers whose credential is keyed by PRINCIPAL rather than by the + * entry-point subject -- i.e. the ones a human re-authorizes by hand and + * expects to only do once (docs/adr/0030 §6). + */ +export const CROSS_ENTRY_POINT_PROVIDERS: ReadonlySet = new Set(["claude", "claude-remote"]); + +/** + * Maps an identity-linked provider (Agent.identityProviders, e.g. "github") to + * the env var name its linked token is injected as (AgentLaunchOptions' + * secretEnv, agentrun-launcher.ts). + */ +export const PROVIDER_ENV_VAR: Record = { + github: "GITHUB_TOKEN", + claude: "CLAUDE_CODE_OAUTH_TOKEN", + "claude-remote": "CLAUDE_LOGIN_CREDENTIALS_JSON", +}; + +/** + * Env vars a `claude-remote` launch carries so the run can persist the + * credentials its Claude Code CLI refreshes in-pod (the gateway's + * `POST /claude-auth/api/refresh`, read by claude-code-swe-agent's + * `config.ts`). Injected via the same `secretEnv` channel as the credential + * itself -- the grant token is a bearer credential, short-lived and scoped to + * one subject, but still not something to hand over as plaintext `env`. + */ +const CREDENTIALS_WRITEBACK_ENV = { + url: "CLAUDE_CREDENTIALS_WRITEBACK_URL", + token: "CLAUDE_CREDENTIALS_WRITEBACK_TOKEN", +} as const; + +/** + * Extra headroom on top of the run's own timeout for a write-back grant's + * lifetime: the CLI can refresh at the very end of a long turn, and a grant + * that expires mid-run silently drops exactly the refresh this mechanism + * exists to capture. + */ +const WRITEBACK_GRANT_MARGIN_SECONDS = 15 * 60; + +/** Human-facing label for a provider, used in link prompts/messages. */ +export const PROVIDER_LABEL: Record = { github: "GitHub", claude: "Claude", "claude-remote": "Claude" }; + +/** A `secretEnv` entry destined for the launched run. Values are credentials; never log one. */ +export interface CredentialEnvEntry { + name: string; + value: string; +} + +/** The resume anchor a parked link is recorded against. Shape mirrors AgentState's `pendingIdentityLink`. */ +export interface PendingIdentityLink { + agentId: string; + provider: string; + flow: string; + deviceCode?: string; + expiresAt: number; + subject: string; + request: string; +} + +/** + * Everything the pre-flight needs, named explicitly rather than handed the + * whole graph state -- so it is obvious that authorization depends on the + * caller's identity and the Agent's declarations, and on nothing the model + * produced this turn beyond the request text it carries forward on a resume. + */ +export interface AuthorizationRequest { + /** The Agent about to be launched: its id and the providers it declares. */ + agent: { id: string; identityProviders?: string[] }; + /** The resolved caller (subject, principal, roles). */ + identity: Identity; + /** This turn's request text, captured onto a parked link so the resume re-delegates THIS goal. */ + request: string; + /** Sender login from a forwarded assertion, used as an actor-login fallback (docs/adr/0030 §5/§6). */ + senderLogin?: string; + /** Present on a streaming chat turn; absent on a fire-and-forget caller (e.g. the GitHub-issue relay). */ + progressListener?: (stage: string, message: string) => void; + /** Notified as soon as a turn is known to need a link, before the (possibly slow) start(). */ + reportIdentityLinkPending?: (info: { provider: string; subject: string }) => void; + /** `"device"` for a headless caller with no browser to redirect; defaults to `"authcode"`. */ + identityLinkFlow?: "device" | "authcode"; +} + +/** + * The pre-flight's verdict. A discriminated union rather than a nullable + * result: every case is a real outcome the caller must handle, and adding a + * fourth would break compilation at the branch instead of falling through to + * "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 } + /** + * 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 + * every provider -- see the batch verdict in {@link AuthorizationService.authorize}. + */ + | { kind: "link-required"; message: string; pending?: PendingIdentityLink } + /** + * Not cleared, and not the caller's fault: the deployment declares a provider + * it has no gateway for, or an unknown provider entirely. Distinct from + * `link-required` because no amount of user action fixes it. + */ + | { kind: "misconfigured"; error: string }; + +export interface AuthorizationServiceDeps { + /** GitHub-provider gateway (device/authcode flows). */ + identityLinkGateway?: IdentityLinkPort; + /** `claude`-provider gateway (PTY `setup-token`, docs/adr/0027). */ + claudeAuthGateway?: IdentityLinkPort; + /** `claude-remote`-provider gateway (full `~/.claude/.credentials.json` login). */ + claudeRemoteGateway?: IdentityLinkPort; + /** Mints the per-run credential write-back grant a `claude-remote` launch carries. */ + claudeRemoteWriteback?: { + createWritebackGrant(subject: string, ttlSeconds: number): Promise<{ url: string; token: string } | undefined>; + }; + /** The launched run's timeout, used to size a write-back grant's lifetime. */ + agentRunTimeoutSeconds?: number; +} + +/** + * Renders the "link your account" clause for one started flow -- one line of + * markdown, embedded into a larger sentence by every caller, which is why it + * carries no leading capital and no trailing period. + */ +export function linkPromptText(started: IdentityLinkStartResult, label: string): string { + if (started.flow === "device") { + return `[link your ${label} account](${started.verificationUri}) and enter code \`${started.userCode}\``; + } + if (started.flow === "authcode") return `[link your ${label} account](${started.authorizeUrl})`; + return `[link your ${label} account](${started.pageUrl})`; +} + +export class AuthorizationService { + constructor(private readonly deps: AuthorizationServiceDeps) {} + + /** + * Resolves which gateway backs a given identity provider (docs/adr/0027) -- + * the one place that knows `"claude"` routes to `claudeAuthGateway` and + * `"claude-remote"` to `claudeRemoteGateway`, instead of the + * (GitHub-only-in-practice) `identityLinkGateway`, so the provider loop stays + * provider-agnostic. + */ + private gatewayFor(provider: string): IdentityLinkPort | undefined { + if (provider === "claude") return this.deps.claudeAuthGateway; + if (provider === "claude-remote") return this.deps.claudeRemoteGateway; + return this.deps.identityLinkGateway; + } + + /** + * The single authorization decision point for an agent launch. + * + * Assesses EVERY declared provider before returning anything. Nothing + * short-circuits on the first gap (docs/adr/0030 §4): all missing links are + * started on this one turn and reported together, and a provider whose start + * failed is reported ALONGSIDE the others rather than instead of them. + * Previously the first gap ended the turn, which made provider order + * load-bearing -- a GitHub OAuth outage blocked Claude authorization + * entirely. + */ + async authorize(req: AuthorizationRequest): Promise { + const { agent, identity } = req; + let secretEnv: CredentialEnvEntry[] | undefined; + + // Batch pre-flight accumulators (docs/adr/0030 §4). + const pendingLinks: { + provider: string; + label: string; + linkUrlText: string; + surfacedLive: boolean; + pending: PendingIdentityLink; + }[] = []; + const failedToStart: string[] = []; + /** + * The caller's GitHub login, read off their resolved `github` link. + * + * Deliberately taken from the link record rather than a `/user` call: the + * login is already stored there, so the orchestrator needs neither an API + * round trip nor the GitHub App credentials to know it. This is what lets + * the agent stop calling `/user` itself (docs/adr/0030 §5) -- the call that + * was returning 401 in production. + */ + let actorLoginFromLoop: string | undefined; + + for (const provider of agent.identityProviders ?? []) { + const gateway = this.gatewayFor(provider); + if (!gateway) { + this.logVerdict("misconfigured", agent.id, { provider, reason: "no gateway configured" }); + return { + kind: "misconfigured", + error: `agent ${agent.id} requires identity providers (${agent.identityProviders!.join(", ")}) but no identity-link gateway is configured for "${provider}"`, + }; + } + + // The principal for cross-entry-point credentials; the raw subject for + // anything scoped to this entry point (docs/adr/0030 §6). + // + // `github` stays on the raw subject deliberately: a GitHub link is a + // 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; + + let existing = await gateway.getToken(provider, credentialSubject); + + if (!existing) { + // Signal "this turn needs a link" NOW, before the (possibly slow) + // start() below -- a fire-and-forget caller uses this to avoid + // prematurely announcing that work has started while the link is still + // being set up. Safe to fire before we even have the link URL. + req.reportIdentityLinkPending?.({ provider, subject: credentialSubject }); + + // Ordinary Open WebUI chat turns never set `identityLinkFlow`, so they + // default to the browser-redirect authcode flow; a headless direct + // `/invoke` caller (e.g. integration-gateway's own GitHub-issue relay) + // can force the device flow instead, since it has no browser to + // redirect. Ignored by the `claude` provider's gateway client (it only + // has one flow shape). + const flow = req.identityLinkFlow ?? "authcode"; + + // Starting the link flow can itself fail before there is any URL to + // show. For "github" this is a plain HTTP call and rarely throws; for + // "claude" (docs/adr/0027) start() spawns a `claude setup-token` PTY and + // scrapes the authorize URL within a timeout, so a missing/slow CLI, a + // crashed PTY, or a URL that never prints all surface here as a throw. + // That must NOT crash the turn into a raw "Something went wrong" -- on + // the fire-and-forget GitHub-issue triage path that error is what gets + // posted to the ticket. + const started = await gateway.start(provider, credentialSubject, flow).catch((err: unknown) => { + console.error( + `[authorization] start threw for provider ${provider}; reporting it alongside the other providers instead of failing the turn: ${err instanceof Error ? err.message : String(err)}`, + ); + return null; + }); + if (!started) { + failedToStart.push(PROVIDER_LABEL[provider] ?? provider); + continue; + } + + const label = PROVIDER_LABEL[provider] ?? provider; + const linkUrlText = linkPromptText(started, label); + + // How the link reaches the caller depends on whether this turn has a + // live channel (a streaming `progressListener`): + // + // - Streaming chat turn: surface the link LIVE now, then block up to the + // flow's expiry on `waitForCompletion` -- the gateway's Redis-backed + // wait by (provider, subject), which resolves the moment EITHER flow + // lands a token -- so the SAME turn resumes automatically once the user + // links, no follow-up message needed. `/invoke`'s async accept/poll + // contract (ADR 0006) tolerates the multi-minute run. + // + // - Fire-and-forget caller (no progressListener -- e.g. + // integration-gateway's GitHub-issue triage relay): there is NO live + // channel, so the link reaches the user ONLY in this turn's final + // result (posted as an issue comment). Blocking here would hide the + // link for the entire wait window -- nobody can complete a link they + // can't see yet, so the wait can only ever time out. So skip the wait + // and let checkPendingIdentityLink resume on the next trigger. + if (req.progressListener) { + req.progressListener( + "identity-link", + `To continue, please ${linkUrlText}. This is a one-time step — I'll continue automatically once you finish.`, + ); + try { + existing = await gateway.waitForCompletion?.(provider, credentialSubject, started.expiresInSeconds * 1000); + } catch (err) { + // The long-held wait is inherently fragile: the gateway pod can roll + // (a deploy mid-flow), an intermediary can drop an idle connection, + // or undici can abort a multi-minute request on its own headers + // timeout -- all surface here as a thrown "fetch failed". None of + // that means the LINK failed: the user can still complete it in + // their browser. So swallow the throw and fall through to the same + // pending-link state a plain timeout produces. + console.error( + `[authorization] waitForCompletion threw for provider ${provider}; treating as not-yet-linked and parking pending: ${err instanceof Error ? err.message : String(err)}`, + ); + existing = undefined; + } + } + + if (!existing) { + pendingLinks.push({ + provider, + label, + linkUrlText, + // On a streaming chat turn the full link prompt was ALREADY surfaced + // live; repeating the same markdown in the terminal result makes the + // caller render the auth prompt twice (the "doubled up" message). + surfacedLive: Boolean(req.progressListener), + pending: { + agentId: agent.id, + provider, + flow: started.flow, + ...(started.flow === "device" ? { deviceCode: started.deviceCode } : {}), + expiresAt: Date.now() + started.expiresInSeconds * 1000, + // The subject `start` was actually called with -- recomputing it + // downstream instead is the PR #144 re-auth loop. + subject: credentialSubject, + // Captured so the eventual resume re-delegates with THIS goal, not + // whatever text the turn that finally notices completion carries. + request: req.request, + }, + }); + 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 + // do NOT declare `github` at all (docs/adr/0030). + if (provider === "github" && existing.githubLogin) actorLoginFromLoop = existing.githubLogin; + + const envVarName = PROVIDER_ENV_VAR[provider]; + if (!envVarName) { + this.logVerdict("misconfigured", agent.id, { provider, reason: "no env var mapping" }); + return { kind: "misconfigured", error: `agent ${agent.id} declares unsupported identity provider "${provider}"` }; + } + secretEnv = [...(secretEnv ?? []), { name: envVarName, value: existing.token }]; + + // `claude-remote` only: its credential is a whole + // `~/.claude/.credentials.json` that the run's own CLI refreshes in place, + // and Anthropic rotates the refresh token when it does -- so without a way + // to write the result back, the copy resolved above is dead the moment + // this run refreshes it and every later run fails with "Login expired · + // Please run /login". Best-effort by design: no grant simply means no + // write-back. + if (provider === "claude-remote" && this.deps.claudeRemoteWriteback) { + // Same canonical subject the credential was READ from above -- a grant + // minted against the raw subject would write the refreshed credentials + // to a record nothing ever reads, and the shared one would keep serving + // the pre-refresh copy until it died. + const grant = await this.deps.claudeRemoteWriteback.createWritebackGrant( + credentialSubject, + (this.deps.agentRunTimeoutSeconds ?? 0) + WRITEBACK_GRANT_MARGIN_SECONDS, + ); + if (grant) { + secretEnv = [ + ...secretEnv, + { name: CREDENTIALS_WRITEBACK_ENV.url, value: grant.url }, + { name: CREDENTIALS_WRITEBACK_ENV.token, value: grant.token }, + ]; + } + } + } + + // ── Batch pre-flight verdict (docs/adr/0030 §4) ───────────────────────── + // One decision point for the whole provider set, reached only after every + // provider has been assessed. + if (pendingLinks.length > 0 || failedToStart.length > 0) { + this.logVerdict("link-required", agent.id, { + pending: pendingLinks.map((l) => `${l.provider}@${l.pending.subject}`), + failedToStart, + }); + return { + kind: "link-required", + message: this.composeLinkRequiredMessage(pendingLinks, failedToStart), + // `pending` still carries ONE entry: it is the resume anchor that + // checkPendingIdentityLink, the terminal /invoke record and + // integration-gateway's waitAndResume all key off, and widening that + // contract is a separate change. Re-entering the gate re-assesses every + // provider anyway, so whichever links the user completed are resolved on + // the next turn and only genuinely-missing ones re-prompt. + ...(pendingLinks[0] ? { pending: pendingLinks[0].pending } : {}), + }; + } + + // ── Sealed actor context (docs/adr/0030 §5) ──────────────────────────── + // The agent receives WHO the caller is, resolved here, so it never performs + // identity lookups of its own. `identityDelegation.ts` was calling GitHub's + // /user with the injected token and failing 401; with this present it skips + // that call entirely, so the failure mode is removed by construction rather + // than debugged. + // + // Login only, no numeric id: the id would require the /user round trip this + // 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)); + if (actorLogin) { + secretEnv = [...(secretEnv ?? []), { name: ACTOR_LOGIN_ENV, value: actorLogin }]; + } + + this.logVerdict("authorized", agent.id, { + // NAMES only. This is the one place that holds every resolved credential + // for a run, so it is also the one place a careless log would dump all of + // them at once. + injecting: (secretEnv ?? []).map((e) => e.name), + actorLogin: actorLogin ?? null, + }); + return { kind: "authorized", ...(secretEnv ? { secretEnv } : {}), ...(actorLogin ? { actorLogin } : {}) }; + } + + /** + * One line per authorization decision, at every exit. + * + * Deliberately permanent, and deliberately at the verdict rather than + * scattered through the provider loop. The three `[identity-gate-debug]` + * console.logs this replaces were marked "remove once root-caused", and + * removing them immediately cost the ability to tell "the gate parked" from + * "the gate cleared but nothing launched" in an e2e failure -- with no logs at + * all, a spec that times out waiting for an AgentRun looks identical whether + * authorization refused, the launch threw, or the relay never arrived. + * + * Values are never logged; `injecting` is names only. + */ + private logVerdict(kind: string, agentId: string, detail: Record): void { + console.log("[authorization]", JSON.stringify({ verdict: kind, agentId, ...detail })); + } + + /** + * READ-ONLY credential resolution: resolve what is already linked, and never + * start a link flow. + * + * The agent-backed-tool path (a Skill reaching an Agent through an ordinary + * tool call) needs the same keying and the same provider->env mapping as + * {@link authorize}, but deliberately cannot start a link: there is no session + * slot analogous to `pendingIdentityLink` for a paused TOOL call, only for a + * paused agent delegation. That is a documented v1 scope cut, not an + * oversight, so it gets its own entry point rather than a flag on `authorize` + * -- a boolean there would make "does calling this start an OAuth flow?" + * depend on an argument, which is precisely the ambiguity §1 is about. + * + * Reporting the offending provider instead of a message: the caller's error + * text names the TOOL, which this class has no business knowing. + */ + async resolveLinkedCredentials(input: { + identity: Identity; + identityProviders?: string[]; + }): Promise< + | { kind: "resolved"; secretEnv?: CredentialEnvEntry[] } + | { kind: "gateway-missing"; provider: string } + | { kind: "not-linked"; provider: string } + | { kind: "unsupported-provider"; provider: string } + > { + let secretEnv: CredentialEnvEntry[] | undefined; + for (const provider of input.identityProviders ?? []) { + const gateway = this.gatewayFor(provider); + if (!gateway) return { kind: "gateway-missing", provider }; + // Same keying as authorize()'s gate -- deriving a different subject here + // would report "not linked" for an account the user had in fact just + // linked, which is the PR #144 re-auth loop. + const credentialSubject = CROSS_ENTRY_POINT_PROVIDERS.has(provider) + ? (input.identity.principal ?? input.identity.subject) + : input.identity.subject; + const existing = await gateway.getToken(provider, credentialSubject); + if (!existing) return { kind: "not-linked", provider }; + const envVarName = PROVIDER_ENV_VAR[provider]; + if (!envVarName) return { kind: "unsupported-provider", provider }; + secretEnv = [...(secretEnv ?? []), { name: envVarName, value: existing.token }]; + } + return { kind: "resolved", ...(secretEnv ? { secretEnv } : {}) }; + } + + /** + * The whole user-facing message for a `link-required` verdict: every + * outstanding link plus every failed start, in one turn's worth of text. + */ + private composeLinkRequiredMessage( + pendingLinks: { label: string; linkUrlText: string; surfacedLive: boolean }[], + failedToStart: string[], + ): string { + const parts: string[] = []; + + if (pendingLinks.length > 0) { + // Anything already surfaced live via progressListener is not repeated + // here -- otherwise a streaming caller renders the same link twice. + const toPrint = pendingLinks.filter((l) => !l.surfacedLive); + if (toPrint.length === 1) { + parts.push(`To continue, please ${toPrint[0]!.linkUrlText}. This is a one-time step -- send any message once you're done.`); + } else if (toPrint.length > 1) { + parts.push( + `To continue, I need you to link ${toPrint.length} accounts (one-time). Please ${toPrint + .map((l) => l.linkUrlText) + .join(", and ")}. Send any message once you're done.`, + ); + } else { + parts.push( + pendingLinks.length === 1 + ? `I haven't received your ${pendingLinks[0]!.label} account link yet. Send any message once you're done and I'll continue.` + : `I haven't received your ${pendingLinks.map((l) => l.label).join(" and ")} account links yet. Send any message once you're done and I'll continue.`, + ); + } + } + + if (failedToStart.length > 0) { + // Reported ALONGSIDE the links rather than instead of them: a provider + // whose start failed must not hide the ones that succeeded, which is + // exactly the coupling ADR 0030 removes. + // + // "also" only when something precedes it -- when every provider failed to + // start there is no preceding clause, and the message has to stand on its + // own. + const labels = failedToStart.join(" and "); + parts.push( + parts.length > 0 + ? `I also couldn't start the ${labels} linking step just now -- try again in a moment and I'll retry that part.` + : `I couldn't start the one-time ${labels} account-linking step just now. Please try again in a moment -- re-apply the label or send any message and I'll retry.`, + ); + } + + return parts.join(" "); + } +} diff --git a/apps/agent-orchestrator/src/agent/graph.test.ts b/apps/agent-orchestrator/src/agent/graph.test.ts index 3607599..f6207e4 100644 --- a/apps/agent-orchestrator/src/agent/graph.test.ts +++ b/apps/agent-orchestrator/src/agent/graph.test.ts @@ -126,7 +126,7 @@ describe("buildAgentGraph", () => { const final = await graph.invoke({ request: "extract the recipe at https://example.com/recipe", authToken: "tok" }); expect(final.error).toBeUndefined(); - expect(final.identity).toEqual({ subject: "alice", roles: ["reader"] }); + expect(final.identity).toEqual({ subject: "alice", roles: ["reader"], principal: "alice" }); expect(final.selectedSkill?.id).toBe("recipe-publisher-skill"); expect(final.selectedTool?.id).toBe("recipe-scraper"); expect(final.result).toEqual({ title: "Pancakes" }); @@ -301,7 +301,7 @@ describe("buildAgentGraph", () => { forwardedUserToken: "alices-signed-jwt", }); - expect(final.identity).toEqual({ subject: "openwebui:alice", roles: ["reader"] }); + expect(final.identity).toEqual({ subject: "openwebui:alice", roles: ["reader"], principal: "openwebui:alice" }); expect(forwardedUserIdentityResolver.resolve).toHaveBeenCalledWith("alices-signed-jwt"); expect(identityResolver.resolve).not.toHaveBeenCalled(); }); @@ -317,7 +317,7 @@ describe("buildAgentGraph", () => { authToken: "shared-static-token", }); - expect(final.identity).toEqual({ subject: "openwebui", roles: ["reader"] }); + expect(final.identity).toEqual({ subject: "openwebui", roles: ["reader"], principal: "openwebui" }); expect(forwardedUserIdentityResolver.resolve).not.toHaveBeenCalled(); }); @@ -1838,7 +1838,13 @@ describe("buildAgentGraph per-caller identity linking (GitHub OAuth Device Flow) expect(deps.agentRunLauncher!.launch).toHaveBeenCalledWith( githubAgent.agentRunTemplate, expect.any(String), - expect.objectContaining({ secretEnv: [{ name: "GITHUB_TOKEN", value: "gho_resolved-token" }] }), + expect.objectContaining({ secretEnv: [ + { name: "GITHUB_TOKEN", value: "gho_resolved-token" }, + // Resolved by the authorization pre-flight from the github link's + // own githubLogin (docs/adr/0030 §5) -- so the agent never calls + // GitHub's /user itself. + { name: "AGENT_ACTOR_LOGIN", value: "alice-gh" }, + ] }), ); expect(final.identityLinkPending).toBeFalsy(); expect(final.pendingIdentityLink).toBeUndefined(); @@ -1956,7 +1962,13 @@ describe("buildAgentGraph per-caller identity linking (GitHub OAuth Device Flow) githubAgent.agentRunTemplate, expect.any(String), expect.objectContaining({ - secretEnv: [{ name: "GITHUB_TOKEN", value: "gho_resolved-token" }], + secretEnv: [ + { name: "GITHUB_TOKEN", value: "gho_resolved-token" }, + // Resolved by the authorization pre-flight from the github link's + // own githubLogin (docs/adr/0030 §5) -- so the agent never calls + // GitHub's /user itself. + { name: "AGENT_ACTOR_LOGIN", value: "alice-gh" }, + ], goal: "triage and resolve this issue", }), ); @@ -2031,7 +2043,13 @@ describe("buildAgentGraph per-caller identity linking (GitHub OAuth Device Flow) expect(deps.agentRunLauncher!.launch).toHaveBeenCalledWith( githubAgent.agentRunTemplate, expect.any(String), - expect.objectContaining({ secretEnv: [{ name: "GITHUB_TOKEN", value: "gho_resolved-token" }] }), + expect.objectContaining({ secretEnv: [ + { name: "GITHUB_TOKEN", value: "gho_resolved-token" }, + // Resolved by the authorization pre-flight from the github link's + // own githubLogin (docs/adr/0030 §5) -- so the agent never calls + // GitHub's /user itself. + { name: "AGENT_ACTOR_LOGIN", value: "alice-gh" }, + ] }), ); expect(final.pendingIdentityLink).toBeUndefined(); expect(final.agentRunId).toBeDefined(); @@ -2204,7 +2222,13 @@ describe("buildAgentGraph per-caller identity linking (GitHub OAuth Device Flow) expect(deps.agentRunLauncher!.launch).toHaveBeenCalledWith( githubAgent.agentRunTemplate, expect.any(String), - expect.objectContaining({ secretEnv: [{ name: "GITHUB_TOKEN", value: "gho_resolved-token" }] }), + expect.objectContaining({ secretEnv: [ + { name: "GITHUB_TOKEN", value: "gho_resolved-token" }, + // Resolved by the authorization pre-flight from the github link's + // own githubLogin (docs/adr/0030 §5) -- so the agent never calls + // GitHub's /user itself. + { name: "AGENT_ACTOR_LOGIN", value: "alice-gh" }, + ] }), ); expect(final.pendingIdentityLink).toBeUndefined(); expect(final.agentRunId).toBeDefined(); @@ -2984,3 +3008,179 @@ describe("buildAgentGraph canonical credential subject (cross-flow Claude creden expect(createWritebackGrant).toHaveBeenCalledWith("github:imaustink", expect.any(Number)); }); }); + +describe("buildAgentGraph batch authorization pre-flight (docs/adr/0030)", () => { + const twoProviderAgent: AgentDescriptor = { + id: "claude-code-swe", + name: "claude-code-swe", + description: "Does software engineering tasks", + allowedRoles: ["reader"], + identityProviders: ["github", "claude"], + agentRunTemplate: { namespace: "default", agentRef: "claude-code-swe" }, + }; + + function preflightDeps(github: Partial, claude: Partial) { + const agentStore: AgentStore = { + upsert: vi.fn(), + query: vi.fn().mockResolvedValue([{ agent: twoProviderAgent, score: 0.9 }]), + getByIds: vi.fn().mockResolvedValue([twoProviderAgent]), + }; + return baseDeps({ + agentStore, + delegateSelector: { select: vi.fn().mockResolvedValue({ type: "agent", agent: twoProviderAgent }) }, + agentChannel: { + awaitReply: vi.fn().mockResolvedValue({ message: "done", final: true, narration: [] } satisfies AgentTurnResult), + sendPrompt: vi.fn(), + close: vi.fn(), + }, + agentRunLauncher: { launch: vi.fn().mockResolvedValue({ name: "run-1", namespace: "default" }) }, + identityLinkGateway: { start: vi.fn(), poll: vi.fn(), getToken: vi.fn().mockResolvedValue(undefined), ...github } as IdentityLinkPort, + claudeAuthGateway: { start: vi.fn(), poll: vi.fn(), getToken: vi.fn().mockResolvedValue(undefined), ...claude } as IdentityLinkPort, + callbackBaseUrl: "http://orchestrator", + callbackSecretRef: { name: "secret", key: "token" }, + }); + } + + const startsOk = (url: string) => vi.fn().mockResolvedValue({ flow: "authcode" as const, authorizeUrl: url, expiresInSeconds: 600 }); + + it("starts EVERY missing provider's link on one turn and prompts for them together", async () => { + const deps = preflightDeps( + { start: startsOk("https://github.example/auth") }, + { start: startsOk("https://claude.example/auth") }, + ); + const graph = buildAgentGraph(deps); + + const final = await graph.invoke({ request: "fix the failing test", authToken: "tok" }); + + // Both started on THIS turn. Before ADR 0030 the gate returned after the + // first gap, so the second provider's link was not even attempted and the + // user needed a separate trigger to discover it. + expect(deps.identityLinkGateway!.start).toHaveBeenCalled(); + expect(deps.claudeAuthGateway!.start).toHaveBeenCalled(); + expect(final.result).toMatch(/2 accounts/i); + expect(deps.agentRunLauncher!.launch).not.toHaveBeenCalled(); + }); + + it("does not let one provider's start failure hide another's link", async () => { + // The exact production coupling ADR 0030 removes: `github` failing to + // start ended the turn before `claude` was evaluated, so a GitHub OAuth + // failure blocked Claude authorization entirely. + const deps = preflightDeps( + { start: vi.fn().mockRejectedValue(new Error("github oauth down")) }, + { start: startsOk("https://claude.example/auth") }, + ); + const graph = buildAgentGraph(deps); + + const final = await graph.invoke({ request: "fix the failing test", authToken: "tok" }); + + expect(deps.claudeAuthGateway!.start).toHaveBeenCalled(); + expect(final.result).toMatch(/claude\.example\/auth/); + expect(final.result).toMatch(/couldn't start the GitHub/i); + // Still parks a resume anchor, so finishing the Claude link resumes. + expect(final.identityLinkPending).toBe(true); + expect(final.pendingIdentityLink?.provider).toBe("claude"); + }); + + it("reads as a standalone message when EVERY provider failed to start", async () => { + const deps = preflightDeps( + { start: vi.fn().mockRejectedValue(new Error("down")) }, + { start: vi.fn().mockRejectedValue(new Error("down")) }, + ); + const graph = buildAgentGraph(deps); + + const final = await graph.invoke({ request: "fix the failing test", authToken: "tok" }); + + // No preceding clause, so it must not open with "I also". + expect(final.result).not.toMatch(/^I also/); + expect(final.result).toMatch(/couldn't start the one-time/i); + expect(final.identityLinkPending).toBeFalsy(); + }); + + it("injects the actor login from the github link, so the agent never resolves identity itself", async () => { + const deps = preflightDeps( + { getToken: vi.fn().mockResolvedValue({ token: "gho_t", githubLogin: "Imaustink" }) }, + { getToken: vi.fn().mockResolvedValue({ token: "sk-ant-oat01" }) }, + ); + const graph = buildAgentGraph(deps); + + await graph.invoke({ request: "fix the failing test", authToken: "tok" }); + + const launched = (deps.agentRunLauncher!.launch as ReturnType).mock.calls[0]?.[2] as { + secretEnv?: { name: string; value: string }[]; + }; + // Taken verbatim off the link record -- no /user round trip, which is the + // call that was returning 401 in production. + expect(launched.secretEnv).toContainEqual({ name: "AGENT_ACTOR_LOGIN", value: "Imaustink" }); + }); +}); + +describe("credentials never reach the model (docs/adr/0030 §3)", () => { + const SECRET_TOKEN = "sk-ant-oat01-SUPER-SECRET-VALUE"; + const SECRET_CREDS = '{"claudeAiOauth":{"accessToken":"SUPER-SECRET-CREDENTIALS-JSON"}}'; + + const agent: AgentDescriptor = { + id: "claude-code-swe", + name: "claude-code-swe", + description: "Does software engineering tasks", + allowedRoles: ["reader"], + identityProviders: ["claude", "claude-remote"], + agentRunTemplate: { namespace: "default", agentRef: "claude-code-swe" }, + }; + + it("passes no credential material to any model-facing dependency", async () => { + // Every model-facing dep records the arguments it was called with. If a + // credential can reach an LLM at all, it has to pass through one of these. + const seen: unknown[] = []; + const record = (result: T) => vi.fn((...args: unknown[]) => { seen.push(args); return Promise.resolve(result); }); + + const deps = baseDeps({ + agentStore: { + upsert: vi.fn(), + query: vi.fn().mockResolvedValue([{ agent, score: 0.9 }]), + getByIds: vi.fn().mockResolvedValue([agent]), + }, + delegateSelector: { select: record({ type: "agent" as const, agent }) }, + skillFitChecker: { fits: record(true) }, + capabilityNeedChecker: { needsCapability: record(true) }, + responseComposer: { compose: record({ prefix: null, suffix: null }) }, + agentChannel: { + awaitReply: vi.fn().mockResolvedValue({ message: "done", final: true, narration: [] } satisfies AgentTurnResult), + sendPrompt: vi.fn(), + close: vi.fn(), + }, + agentRunLauncher: { launch: vi.fn().mockResolvedValue({ name: "run-1", namespace: "default" }) }, + claudeAuthGateway: { + start: vi.fn(), + poll: vi.fn(), + getToken: vi.fn().mockResolvedValue({ token: SECRET_TOKEN }), + } as IdentityLinkPort, + claudeRemoteGateway: { + start: vi.fn(), + poll: vi.fn(), + getToken: vi.fn().mockResolvedValue({ token: SECRET_CREDS }), + } as IdentityLinkPort, + callbackBaseUrl: "http://orchestrator", + callbackSecretRef: { name: "secret", key: "token" }, + }); + + const final = await buildAgentGraph(deps).invoke({ request: "fix the failing test", authToken: "tok" }); + + // Sanity: the credentials really were resolved, so a passing assertion + // below means "not leaked" rather than "never present". + const launched = (deps.agentRunLauncher!.launch as ReturnType).mock.calls[0]?.[2] as { + secretEnv?: { name: string; value: string }[]; + }; + expect(launched.secretEnv?.map((e) => e.value)).toContain(SECRET_TOKEN); + + // The actual guarantee. + const modelFacing = JSON.stringify(seen); + expect(modelFacing).not.toContain(SECRET_TOKEN); + expect(modelFacing).not.toContain(SECRET_CREDS); + + // Nor may they survive on the returned graph state, which is persisted to + // the session store and echoed into later prompts. + const serializedState = JSON.stringify(final); + expect(serializedState).not.toContain(SECRET_TOKEN); + expect(serializedState).not.toContain(SECRET_CREDS); + }); +}); diff --git a/apps/agent-orchestrator/src/agent/graph.ts b/apps/agent-orchestrator/src/agent/graph.ts index b873de9..06b3591 100644 --- a/apps/agent-orchestrator/src/agent/graph.ts +++ b/apps/agent-orchestrator/src/agent/graph.ts @@ -12,7 +12,7 @@ import type { AgentRunLauncherPort } from "../k8s/agentrun-launcher.js"; import type { SecretKeySelector } from "../k8s/toolrun-launcher.js"; import type { LocalToolExecutor } from "../local/local-tool-executor.js"; import type { IdentityLinkPort, IdentityLinkStartResult } from "../identity-link/gateway-client.js"; -import { resolveCredentialSubject } from "../identity-link/credential-subject.js"; +import { resolveActorLogin, resolvePrincipal } from "../identity-link/credential-subject.js"; import type { IdentityResolver, Identity } from "../rbac/types.js"; import type { SkillDescriptor, SkillSearchResult, SkillStore } from "../skills/types.js"; import type { ToolDescriptor } from "../tool-descriptor.js"; @@ -26,6 +26,18 @@ import type { SkillFitChecker } from "./skill-fit-checker.js"; import type { SkillSelector } from "./skill-selector.js"; import type { ToolFitChecker } from "./tool-fit-checker.js"; import { makeSubAgentToolCallHandler, type ToolCatalog } from "./dispatch-tool.js"; +import { + ACTOR_LOGIN_ENV, + AuthorizationService, + CROSS_ENTRY_POINT_PROVIDERS, + linkPromptText, + PROVIDER_LABEL, +} from "./authorization-service.js"; + +// Re-exported: several tests and callers import ACTOR_LOGIN_ENV from this +// module, and the constant's home is now the authorization service that owns +// the decision to inject it. +export { ACTOR_LOGIN_ENV }; /** * Agent state threaded through the graph (docs/adr/0008, docs/adr/0012, @@ -590,48 +602,16 @@ export interface AgentGraphDeps { }; } -/** - * Maps an identity-linked provider (Agent.identityProviders, e.g. "github") - * to the env var name its linked token is injected as (AgentLaunchOptions' - * secretEnv, agentrun-launcher.ts). - */ -const PROVIDER_ENV_VAR: Record = { - github: "GITHUB_TOKEN", - claude: "CLAUDE_CODE_OAUTH_TOKEN", - "claude-remote": "CLAUDE_LOGIN_CREDENTIALS_JSON", -}; - -/** - * Env vars a `claude-remote` launch carries so the run can persist the - * credentials its Claude Code CLI refreshes in-pod (the gateway's - * `POST /claude-auth/api/refresh`, read by claude-code-swe-agent's - * `config.ts`). Injected via the same `secretEnv` channel as the credential - * itself -- the grant token is a bearer credential, short-lived and scoped to - * one subject, but still not something to hand over as plaintext `env`. - */ -const CREDENTIALS_WRITEBACK_ENV = { - url: "CLAUDE_CREDENTIALS_WRITEBACK_URL", - token: "CLAUDE_CREDENTIALS_WRITEBACK_TOKEN", -} as const; - -/** - * Extra headroom on top of the run's own timeout for a write-back grant's - * lifetime: the CLI can refresh at the very end of a long turn, and a grant - * that expires mid-run silently drops exactly the refresh this mechanism - * exists to capture. - */ -const WRITEBACK_GRANT_MARGIN_SECONDS = 15 * 60; - -/** Human-facing label for a provider, used in link prompts/messages. */ -const PROVIDER_LABEL: Record = { github: "GitHub", claude: "Claude", "claude-remote": "Claude" }; - /** * Resolves which gateway client backs a given identity provider (docs/adr/0027) * -- the one place that knows `"claude"` routes to `deps.claudeAuthGateway` * and `"claude-remote"` routes to `deps.claudeRemoteGateway`, instead of the - * (GitHub-only-in-practice) `deps.identityLinkGateway`, so the three call - * sites below (`checkPendingIdentityLink`, `delegateToAgent`, the - * agent-backed-tool path) stay provider-agnostic. + * (GitHub-only-in-practice) `deps.identityLinkGateway`. + * + * Kept here as a thin adapter over the same resolution + * {@link AuthorizationService} performs internally, for the two link-lifecycle + * call sites (`startReplacementLink`, `checkPendingIdentityLink`) that operate + * on an ALREADY-STARTED link rather than making an authorization decision. */ function identityGatewayFor(provider: string, deps: AgentGraphDeps): IdentityLinkPort | undefined { if (provider === "claude") return deps.claudeAuthGateway; @@ -686,20 +666,6 @@ const AUTH_EXPIRED_CODE_PROVIDER: Record = { [CLAUDE_REMOTE_AUTH_EXPIRED_CODE]: "claude-remote", }; -/** - * The clickable "link your account" instruction for a started flow. Shared by - * the first-time link prompt in `delegateToAgent` and the re-link prompt in - * `handleAgentTurnFailure` so the two can never drift into describing the same - * flow differently. - */ -function linkPromptText(started: IdentityLinkStartResult, label: string): string { - if (started.flow === "device") { - return `[link your ${label} account](${started.verificationUri}) and enter code \`${started.userCode}\``; - } - if (started.flow === "authcode") return `[link your ${label} account](${started.authorizeUrl})`; - return `[link your ${label} account](${started.pageUrl})`; -} - /** * Starts a fresh link flow immediately after a stale credential was * invalidated, returning the same `result` + `pendingIdentityLink` + @@ -805,15 +771,12 @@ async function handleAgentTurnFailure( // swallowing it, and log it -- a failure here is otherwise invisible, // indistinguishable from success in both the logs and the reply. let invalidated = true; - // Must clear the record the gate actually READ (canonical for a Claude - // provider), or the "expired credential" the run just tripped over - // survives and every retry re-reads it. - const staleSubject = await resolveCredentialSubject( - provider, - state.identity.subject, - state.senderLogin, - deps.identityLinkGateway, - ); + // Must clear the record the gate actually READ, or the "expired + // credential" the run just tripped over survives and every retry + // re-reads it. + const staleSubject = CROSS_ENTRY_POINT_PROVIDERS.has(provider) + ? (state.identity.principal ?? state.identity.subject) + : state.identity.subject; await gateway.invalidate?.(provider, staleSubject).catch((invalidateErr: unknown) => { invalidated = false; console.error( @@ -1015,6 +978,12 @@ async function noMatchFallback(state: AgentState, deps: AgentGraphDeps): Promise /** Builds and compiles the LangGraph.js agent graph (docs/adr/0008, superseding the earlier flat tool-RAG flow). */ export function buildAgentGraph(deps: AgentGraphDeps) { + // The single owner of the authorization decision (docs/adr/0030 §1). + // Constructed here, from deps, rather than injected: it is not a swappable + // policy but the graph's own gate, and making it a dep would invite a + // deployment that supplied a permissive one. + const authorization = new AuthorizationService(deps); + const graph = new StateGraph(AgentStateAnnotation) .addNode("resolveIdentity", async (state) => { // Prefer Open WebUI's per-request signed user JWT over the shared @@ -1029,7 +998,13 @@ export function buildAgentGraph(deps: AgentGraphDeps) { if (!identity) { return { error: "unauthorized: could not resolve caller identity" }; } - return { identity }; + // Resolve the PRINCIPAL once, here, rather than per-provider deeper in + // the graph (docs/adr/0030 §6). Doing it at identity time means every + // downstream consumer reads one already-decided value instead of each + // re-deriving its own -- the re-derivation that let store and wait drift + // apart in PR #144. + const principal = await resolvePrincipal(identity.subject, state.senderLogin, deps.identityLinkGateway); + return { identity: { ...identity, principal } }; }) .addNode("checkIntegrationRoute", async (state) => { // Deterministic dispatch for a turn whose intent is already @@ -1269,231 +1244,42 @@ export function buildAgentGraph(deps: AgentGraphDeps) { } const agent = state.selectedAgent; - // TEMPORARY diagnostic for the "launched with no GITHUB_TOKEN despite - // a linked identity" investigation -- remove once root-caused. Logs - // exactly what this call sees at the identity-gate decision point, - // never the token value itself. - console.log( - "[identity-gate-debug] delegateToAgent", - JSON.stringify({ - agentId: agent.id, - identityProviders: agent.identityProviders, - hasIdentityLinkGateway: Boolean(deps.identityLinkGateway), - subject: state.identity.subject, - roles: state.identity.roles, - hasProgressListener: Boolean(state.progressListener), - }), - ); - - // Per-caller identity gate (replaces the old shared static credential - // for any Agent that declares `identityProviders`): the FIRST time this - // caller delegates to such an agent, they must link their own account - // with that provider (one-time OAuth device/authcode flow for - // "github", or a PTY `setup-token` flow for "claude", docs/adr/0027) - // before a launch is even attempted. Only the FIRST declared provider - // is gated end-to-end today (multi-provider agents aren't expected in - // practice yet); `identityGatewayFor` is what lets this stay - // provider-agnostic as more are added. - // Resolves EVERY declared identityProviders entry, not just the first -- - // an Agent can legitimately need more than one linked identity at once - // (e.g. claude-code-swe-agent's ["claude", "claude-remote"]: the - // setup-token flow for model calls plus the separate full-login flow - // Remote Control needs). Previously this only ever checked index 0, so - // a second provider's token was silently never resolved/injected -- - // whatever secretEnv name it mapped to reached the Job as an empty/ - // unset value instead of erroring or prompting to link it. Each - // provider still gets the exact same getToken -> (missing? start/wait/ - // pend, else accumulate) handling as before; the loop just repeats it - // per provider instead of doing it once. - let identitySecretEnv: { name: string; value: string }[] | undefined; - for (const provider of agent.identityProviders ?? []) { - const gateway = identityGatewayFor(provider, deps); - if (!gateway) { - return { - error: `agent ${agent.id} requires identity providers (${agent.identityProviders!.join(", ")}) but no identity-link gateway is configured for "${provider}"`, - }; - } - // Resolved per provider, INSIDE the loop, not once up front: with - // `github` declared ahead of the Claude providers, a link completed - // by the `waitForCompletion` below is what makes the canonical - // subject resolvable, and a value computed before that link landed - // would still be the raw subject. Re-resolving per provider means - // the github link established moments ago is visible to `claude` and - // `claude-remote` on this very turn. - const credentialSubject = await resolveCredentialSubject( - provider, - state.identity.subject, - state.senderLogin, - deps.identityLinkGateway, - ); - let existing = await gateway.getToken(provider, credentialSubject); - console.log( - "[identity-gate-debug] getToken", - JSON.stringify({ provider, subject: credentialSubject, rawSubject: state.identity.subject, found: Boolean(existing) }), - ); - if (!existing) { - // Signal "this turn needs a link" NOW, before the (possibly slow) - // start() below -- a fire-and-forget caller uses this to avoid - // prematurely announcing that work has started while the link is - // still being set up. Safe to fire before we even have the link URL. - state.reportIdentityLinkPending?.({ provider, subject: credentialSubject }); - // Ordinary Open WebUI chat turns never set `identityLinkFlow`, so - // they default to the browser-redirect authcode flow; a headless - // direct `/invoke` caller (e.g. integration-gateway's own - // GitHub-issue relay) can force the device flow instead, since it - // has no browser to redirect. Ignored by the `claude` provider's - // gateway client (it only has one flow shape). - const flow = state.identityLinkFlow ?? "authcode"; - // Starting the link flow can itself fail before there is any URL to - // show. For "github" this is a plain HTTP call and rarely throws; - // for "claude" (docs/adr/0027) start() spawns a `claude setup-token` - // PTY and scrapes the authorize URL within a timeout, so a - // missing/slow CLI, a crashed PTY, or a URL that never prints all - // surface here as a throw. That must NOT crash the turn into a raw - // "Something went wrong" -- on the fire-and-forget GitHub-issue - // triage path that error is what gets posted to the ticket. Unlike - // the waitForCompletion catch below, there is no started flow to - // park a pendingIdentityLink against, so end the turn with a - // friendly, retryable message instead. Re-triggering (re-applying - // the label, or any follow-up message in the session) re-enters this - // node and retries start(). - const started = await gateway - .start(provider, credentialSubject, flow) - .catch((err) => { - console.error( - `[identity-gate] start threw for provider ${provider}; ending turn with a retryable message instead of a hard error: ${err instanceof Error ? err.message : String(err)}`, - ); - return null; - }); - if (!started) { - const startLabel = PROVIDER_LABEL[provider] ?? provider; - return { - result: `I couldn't start the one-time ${startLabel} account-linking step just now. Please try again in a moment -- re-apply the label or send any message and I'll retry.`, - }; - } - const label = PROVIDER_LABEL[provider] ?? provider; - const linkUrlText = linkPromptText(started, label); - - // How the link reaches the caller depends on whether this turn has a - // live channel (a streaming `progressListener`): - // - // - Streaming chat turn (progressListener present): surface the link - // LIVE now, then block up to the flow's expiry on - // `waitForCompletion` -- the gateway's Redis-backed wait by - // (provider, subject), which resolves the moment EITHER flow lands - // a token -- so the SAME turn resumes automatically once the user - // links, no follow-up message needed. `/invoke`'s async accept/poll - // contract (ADR 0006) tolerates the multi-minute run. - // - // - Fire-and-forget caller (no progressListener -- e.g. - // integration-gateway's GitHub-issue triage relay): there is NO - // live channel, so the link reaches the user ONLY in this turn's - // final `result` (posted as an issue comment). Blocking here would - // hide the link for the entire wait window -- nobody can complete a - // link they can't see yet, so the wait can only ever time out. So - // skip the wait: post the link IMMEDIATELY (fall straight through - // to the pending-link return below) and let checkPendingIdentityLink - // resume on the next trigger (a follow-up comment, or re-applying - // the label) once the token lands. This makes an unauthenticated - // triage prompt for auth up front -- the same immediacy the chat - // flow gets from surfacing the prompt live. - if (state.progressListener) { - state.progressListener("identity-link", `To continue, please ${linkUrlText}. This is a one-time step — I'll continue automatically once you finish.`); - try { - existing = await gateway.waitForCompletion?.( - provider, - credentialSubject, - started.expiresInSeconds * 1000, - ); - } catch (err) { - // The long-held wait is inherently fragile: the gateway pod can - // roll (a deploy mid-flow), an intermediary can drop an idle - // connection, or undici can abort a multi-minute request on its - // own headers timeout -- all surface here as a thrown "fetch - // failed". None of that means the LINK failed: the user can still - // complete it in their browser. So swallow the throw and fall - // through to the same pending-link state a plain timeout produces - // -- the user links, sends any message, and - // checkPendingIdentityLink resumes via getToken. Previously this - // threw straight out of the node and surfaced to the user as a - // bare "❌ fetch failed", aborting an otherwise-fine link attempt. - console.error( - `[identity-gate] waitForCompletion threw for provider ${provider}; treating as not-yet-linked and parking pending: ${err instanceof Error ? err.message : String(err)}`, - ); - existing = undefined; - } - } - - if (!existing) { - // On a streaming chat turn we ALREADY surfaced the full link prompt - // live via `progressListener` above; repeating the same - // `[link your account](url)` markdown in the terminal `result` - // makes the caller render the auth prompt twice (the "doubled up" - // message on the chat side). So once it's been surfaced live, the - // final result is just a short, non-duplicative nudge -- the link - // itself is still visible in the streamed message above. The - // fire-and-forget triage path (no `progressListener` -- the link - // reaches the user ONLY in `result`, posted as an issue comment) - // is unchanged: it still prints the full prompt here. - const result = state.progressListener - ? `I haven't received your ${label} account link yet. Send any message once you're done and I'll continue.` - : `To continue, please ${linkUrlText}. This is a one-time step -- send any message once you're done.`; - return { - result, - pendingIdentityLink: { - agentId: agent.id, - provider, - flow: started.flow, - ...(started.flow === "device" ? { deviceCode: started.deviceCode } : {}), - expiresAt: Date.now() + started.expiresInSeconds * 1000, - // The subject `start` was actually called with -- see this - // field's doc on the state annotation for why recomputing it - // downstream instead is the PR #144 re-auth loop. - subject: credentialSubject, - // Captured so the eventual resume (checkPendingIdentityLink) - // re-delegates with THIS goal, not whatever text the turn - // that finally notices completion happens to carry. - request: state.request, - }, - identityLinkPending: true, - }; - } - } - const envVarName = PROVIDER_ENV_VAR[provider]; - if (!envVarName) { - return { error: `agent ${agent.id} declares unsupported identity provider "${provider}"` }; - } - identitySecretEnv = [...(identitySecretEnv ?? []), { name: envVarName, value: existing.token }]; + // ── Authorization pre-flight (docs/adr/0030) ──────────────────────── + // The ONE authorization decision for this launch, owned by graph control + // flow and made before anything is created. Everything it needs is named + // in the request; nothing the planner produced influences it beyond the + // request text carried onto a parked link. + // + // The logic used to live inline here -- ~300 lines between this comment + // and the launch below. Extracting it changed no behaviour; it gave the + // decision a boundary, so "may this run start, and with whose + // credentials" is answered by one call with a total return type rather + // than by reading the node. + const verdict = await authorization.authorize({ + agent: { id: agent.id, identityProviders: agent.identityProviders }, + identity: state.identity, + request: state.request, + senderLogin: state.senderLogin, + progressListener: state.progressListener, + reportIdentityLinkPending: state.reportIdentityLinkPending, + identityLinkFlow: state.identityLinkFlow, + }); - // `claude-remote` only: its credential is a whole - // `~/.claude/.credentials.json` that the run's own CLI refreshes in - // place, and Anthropic rotates the refresh token when it does -- so - // without a way to write the result back, the copy resolved above is - // dead the moment this run refreshes it and every later run fails with - // "Login expired · Please run /login" (the exact failure this grant - // fixes). Best-effort by design: no grant simply means no write-back. - if (provider === "claude-remote" && deps.claudeRemoteWriteback) { - // Same canonical subject the credential was READ from above -- - // a grant minted against the raw subject would write the refreshed - // credentials to a record nothing ever reads, and the shared one - // would keep serving the pre-refresh copy until it died. - const grant = await deps.claudeRemoteWriteback.createWritebackGrant( - credentialSubject, - (deps.agentRunTimeoutSeconds ?? 0) + WRITEBACK_GRANT_MARGIN_SECONDS, - ); - if (grant) { - identitySecretEnv = [ - ...identitySecretEnv, - { name: CREDENTIALS_WRITEBACK_ENV.url, value: grant.url }, - { name: CREDENTIALS_WRITEBACK_ENV.token, value: grant.token }, - ]; - } - } + if (verdict.kind === "misconfigured") { + return { error: verdict.error }; } - console.log( - "[identity-gate-debug] pre-launch", - JSON.stringify({ agentId: agent.id, hasIdentitySecretEnv: Boolean(identitySecretEnv) }), - ); + if (verdict.kind === "link-required") { + // The turn ends here with the batched link prompt. `pendingIdentityLink` + // is the resume anchor checkPendingIdentityLink and + // integration-gateway's waitAndResume key off; it is absent when every + // provider merely failed to START, since there is no started flow to + // resume against -- re-triggering re-enters this node and retries. + return { + result: verdict.message, + ...(verdict.pending ? { pendingIdentityLink: verdict.pending, identityLinkPending: true } : {}), + }; + } + const identitySecretEnv = verdict.secretEnv; const runId = randomUUID(); const jobId = randomUUID(); @@ -1660,45 +1446,38 @@ export function buildAgentGraph(deps: AgentGraphDeps) { return { error: `tool ${tool.id} is agent-backed but agent delegation is not configured` }; } // Same per-caller identity gate as delegateToAgent (ADR 0022) -- - // required here too now that an identity-gated Agent's static - // secretEnv is stripped regardless of which path reaches it. v1 - // scope cut: unlike delegateToAgent, this path never STARTS a fresh - // device-flow/authcode link -- there's no session slot analogous to - // pendingIdentityLink for a paused tool call, only for a paused - // agent delegation. A caller must link once via direct chat - // delegation to the same agent before a Skill can reach it here. - // Loops over every declared provider, not just index 0 -- see the - // identical fix/comment on delegateToAgent's own identity-gate above. - let identitySecretEnv: { name: string; value: string }[] | undefined; - for (const provider of tool.identityProviders ?? []) { - const gateway = identityGatewayFor(provider, deps); - if (!gateway || !state.identity) { - return { - error: `tool ${tool.id} requires identity providers (${tool.identityProviders!.join(", ")}) but no identity-link gateway is configured for "${provider}"`, - }; - } - // Same canonical keying as delegateToAgent's gate -- this path only - // READS a credential (it never starts a link), so if it derived a - // different subject than the gate it would report "not linked" for - // an account the user had in fact just linked. - const credentialSubject = await resolveCredentialSubject( - provider, - state.identity.subject, - state.senderLogin, - deps.identityLinkGateway, - ); - const existing = await gateway.getToken(provider, credentialSubject); - if (!existing) { - return { - error: `tool ${tool.id} requires linking your ${provider} account first -- start a direct conversation with this agent to link it, then retry`, - }; - } - const envVarName = PROVIDER_ENV_VAR[provider]; - if (!envVarName) { - return { error: `tool ${tool.id} declares unsupported identity provider "${provider}"` }; - } - identitySecretEnv = [...(identitySecretEnv ?? []), { name: envVarName, value: existing.token }]; + // required here too now that an identity-gated Agent's static secretEnv + // is stripped regardless of which path reaches it -- and the SAME owner + // (docs/adr/0030 §1), so the keying can never drift between the two + // paths. + // + // The read-only entry point, deliberately: v1 scope cut is that unlike + // delegateToAgent, this path never STARTS a fresh device-flow/authcode + // link -- there's no session slot analogous to pendingIdentityLink for a + // paused TOOL call, only for a paused agent delegation. A caller must + // link once via direct chat delegation to the same agent before a Skill + // can reach it here. + if (!state.identity) { + return { error: `tool ${tool.id} requires identity providers but no caller identity was resolved` }; + } + const credentials = await authorization.resolveLinkedCredentials({ + identity: state.identity, + identityProviders: tool.identityProviders, + }); + if (credentials.kind === "gateway-missing") { + return { + error: `tool ${tool.id} requires identity providers (${tool.identityProviders!.join(", ")}) but no identity-link gateway is configured for "${credentials.provider}"`, + }; + } + if (credentials.kind === "not-linked") { + return { + error: `tool ${tool.id} requires linking your ${credentials.provider} account first -- start a direct conversation with this agent to link it, then retry`, + }; + } + if (credentials.kind === "unsupported-provider") { + return { error: `tool ${tool.id} declares unsupported identity provider "${credentials.provider}"` }; } + const identitySecretEnv = credentials.secretEnv; const runId = randomUUID(); const callbackUrl = `${deps.callbackBaseUrl}/callback/${randomUUID()}`; try { diff --git a/apps/agent-orchestrator/src/config.ts b/apps/agent-orchestrator/src/config.ts index a7244f5..e9305e8 100644 --- a/apps/agent-orchestrator/src/config.ts +++ b/apps/agent-orchestrator/src/config.ts @@ -65,6 +65,13 @@ export interface AppConfig { callbackSecretRefKey: string; /** HTTP port the consumer-facing invoke API listens on (ADR 0006). */ httpPort: number; + /** + * Shared secret integration-gateway signs its sender assertions with + * (docs/adr/0030 §6). When set, a webhook turn's sender login is accepted + * ONLY from a verified assertion; when unset, the unsigned request-body + * field is trusted (pre-assertion behaviour, warned about at startup). + */ + senderAssertionSecret: string; /** * Directory holding one `.sock` per LocalTool executor sidecar * (ADR 0014), shared with the sidecars via an emptyDir. The orchestrator @@ -176,6 +183,7 @@ export const config: AppConfig = { callbackPort: num(process.env.AGENT_CALLBACK_PORT, 8080), callbackBaseUrl: process.env.AGENT_CALLBACK_BASE_URL ?? "http://localhost:8080", httpPort: num(process.env.AGENT_HTTP_PORT, 8081), + senderAssertionSecret: process.env.AGENT_SENDER_ASSERTION_SECRET ?? "", callbackSecret: process.env.AGENT_CALLBACK_SECRET ?? "", callbackSecretRefName: process.env.AGENT_CALLBACK_SECRET_REF_NAME, callbackSecretRefKey: process.env.AGENT_CALLBACK_SECRET_REF_KEY ?? "AGENT_CALLBACK_SECRET", 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 d009199..08927f7 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, resolveCredentialSubject } from "./credential-subject.js"; +import { canonicalSubjectForLogin, resolveActorLogin, resolvePrincipal } from "./credential-subject.js"; function githubGateway(githubLogin?: string) { return { getToken: vi.fn().mockResolvedValue(githubLogin ? { token: "gh", githubLogin } : undefined) }; @@ -12,51 +12,57 @@ describe("canonicalSubjectForLogin", () => { }); }); -describe("resolveCredentialSubject", () => { - it("leaves non-Claude providers on the raw subject", async () => { - const gw = githubGateway("imaustink"); - // `github` is the source of the mapping, so canonicalizing it would be - // circular -- and triage's GitHub writes must stay on their own subject. - expect(await resolveCredentialSubject("github", "openwebui:42", "imaustink", gw)).toBe("openwebui:42"); - expect(gw.getToken).not.toHaveBeenCalled(); - }); - - it.each(["claude", "claude-remote"])("canonicalizes %s from senderLogin without any lookup", async (provider) => { +describe("resolvePrincipal", () => { + it("uses senderLogin without any lookup -- the webhook sender is authoritative", async () => { const gw = githubGateway("someone-else"); - expect(await resolveCredentialSubject(provider, "service-subject", "Imaustink", gw)).toBe("github:imaustink"); - // senderLogin is authoritative for a webhook turn: consulting the link - // would let the shared service account's link win over the real human. + expect(await resolvePrincipal("service-subject", "Imaustink", gw)).toBe("github:imaustink"); + // Consulting the link would let the shared service account's own link win + // over the actual human who triggered the event. expect(gw.getToken).not.toHaveBeenCalled(); }); - it("canonicalizes from the caller's GitHub link when there is no senderLogin", async () => { + it("falls back to the caller's GitHub link when there is no senderLogin", async () => { const gw = githubGateway("Imaustink"); - expect(await resolveCredentialSubject("claude", "openwebui:42", undefined, gw)).toBe("github:imaustink"); + expect(await resolvePrincipal("openwebui:42", undefined, gw)).toBe("github:imaustink"); expect(gw.getToken).toHaveBeenCalledWith("github", "openwebui:42"); }); - it("produces the SAME subject for a chat turn and a triage turn by the same human", async () => { - const chat = await resolveCredentialSubject("claude-remote", "openwebui:42", undefined, githubGateway("imaustink")); - const triage = await resolveCredentialSubject("claude-remote", "service-subject", "Imaustink", githubGateway()); - // The entire point of the change. - expect(chat).toBe(triage); + it("gives a chat turn and a webhook turn by the same human ONE principal", async () => { + const chat = await resolvePrincipal("openwebui:42", undefined, githubGateway("imaustink")); + const webhook = await resolvePrincipal("client-integration-gateway", "Imaustink", githubGateway()); + // The entire point: one authorization covers both entry points. + expect(chat).toBe(webhook); }); - it("falls back to the raw subject when nothing resolves a login", async () => { - expect(await resolveCredentialSubject("claude", "openwebui:42", undefined, githubGateway())).toBe("openwebui:42"); + it("does not collapse two different humans onto one principal", async () => { + const a = await resolvePrincipal("openwebui:1", undefined, githubGateway("alice")); + const b = await resolvePrincipal("openwebui:2", undefined, githubGateway("bob")); + expect(a).not.toBe(b); }); - it("falls back to the raw subject when no github gateway is configured", async () => { - expect(await resolveCredentialSubject("claude", "openwebui:42", undefined, undefined)).toBe("openwebui:42"); + it("uses the raw subject as its own principal when no GitHub identity resolves", async () => { + // A working state, not a failure -- no cross-entry-point sharing, which is + // exactly the behaviour before principals existed. + expect(await resolvePrincipal("openwebui:42", undefined, githubGateway())).toBe("openwebui:42"); + expect(await resolvePrincipal("openwebui:42", undefined, undefined)).toBe("openwebui:42"); }); it("falls back to the raw subject when the lookup throws", async () => { const gw = { getToken: vi.fn().mockRejectedValue(new Error("gateway down")) }; - await expect(resolveCredentialSubject("claude", "openwebui:42", undefined, gw)).resolves.toBe("openwebui:42"); + await expect(resolvePrincipal("openwebui:42", undefined, gw)).resolves.toBe("openwebui:42"); + }); +}); + +describe("resolveActorLogin", () => { + it("prefers senderLogin, then the link, then nothing", async () => { + expect(await resolveActorLogin("s", "sender", githubGateway("linked"))).toBe("sender"); + expect(await resolveActorLogin("s", undefined, githubGateway("linked"))).toBe("linked"); + expect(await resolveActorLogin("s", undefined, githubGateway())).toBeUndefined(); }); - it("ignores a link that carries no githubLogin", async () => { - const gw = { getToken: vi.fn().mockResolvedValue({ token: "gh" }) }; - expect(await resolveCredentialSubject("claude", "openwebui:42", undefined, gw)).toBe("openwebui:42"); + it("is independent of any Agent's identityProviders (docs/adr/0030)", async () => { + // Knowing WHO the caller is and provisioning them a credential are + // different concerns; conflating them is what caused the production 401. + expect(await resolveActorLogin("openwebui:42", undefined, githubGateway("imaustink"))).toBe("imaustink"); }); }); diff --git a/apps/agent-orchestrator/src/identity-link/credential-subject.ts b/apps/agent-orchestrator/src/identity-link/credential-subject.ts index 028939c..71ee1dd 100644 --- a/apps/agent-orchestrator/src/identity-link/credential-subject.ts +++ b/apps/agent-orchestrator/src/identity-link/credential-subject.ts @@ -1,18 +1,5 @@ import type { IdentityLinkPort } from "./gateway-client.js"; -/** - * Providers whose credential is keyed by a CANONICAL, cross-entry-point - * subject rather than the raw `identity.subject` of whichever entry point - * happened to resolve the caller. - * - * Only the Claude credentials are in this set, and that is deliberate. The - * `github` link is what PRODUCES the mapping (see - * {@link resolveCredentialSubject}), so keying it canonically would be - * circular; it stays keyed by the raw subject, exactly as before. Triage's - * GitHub writes also still go through the App installation token, untouched - * by anything here. - */ -const CANONICAL_PROVIDERS: ReadonlySet = new Set(["claude", "claude-remote"]); /** * The canonical credential subject for a GitHub login. Lower-cased because @@ -30,60 +17,65 @@ export function canonicalSubjectForLogin(login: string): string { } /** - * Resolves the subject a given provider's credential is stored under. + * The caller's GitHub login, or `undefined` if none can be established. * - * The Claude credentials (`claude`, `claude-remote`) are the ones a human - * re-authorizes by hand, and until now they were keyed by `identity.subject` - * -- which differs per entry point (a shared service subject for - * GitHub-webhook triage, `openwebui:` for chat), so authorizing in one - * flow left the other unauthorized. This maps both entry points onto one - * canonical `github:` key so a single authorization covers both. + * Deliberately independent of an Agent's `identityProviders` (docs/adr/0030): + * knowing WHO the caller is and provisioning them a GitHub *credential* are + * different concerns, and conflating them is what forced `claude-code-swe-agent` + * to declare the `github` provider purely to obtain a mapping -- which + * activated the delegated-write path and produced a production 401. This is a + * read-only lookup with no side effects: it never starts a link. * - * The login comes from whichever source the entry point actually has: - * - **Triage / AI review**: `senderLogin`, plumbed through `/invoke`'s event - * descriptor -- GitHub itself vouches for it (the webhook signature is - * verified upstream in the gateway, which drops the event entirely if the - * sender resolves to no identity). - * - **Chat**: the `githubLogin` on the caller's own `github` identity link, - * which they established via the device/authcode flow -- i.e. they proved - * control of that account to GitHub. + * Same two verified sources as {@link resolveCredentialSubject}: a + * signature-verified webhook's sender, or a link the caller established by + * proving control of the account. + */ +export async function resolveActorLogin( + rawSubject: string, + senderLogin: string | undefined, + githubGateway: Pick | undefined, +): Promise { + if (senderLogin) return senderLogin; + if (!githubGateway) return undefined; + try { + return (await githubGateway.getToken("github", rawSubject))?.githubLogin; + } catch { + // A failed lookup must not fail the turn -- the agent simply falls back + // to its own resolution, exactly as before this existed. + return undefined; + } +} + +/** + * Resolves the caller's PRINCIPAL -- the stable identifier for the human, + * independent of which entry point they arrived through (docs/adr/0030 §6). + * + * `ClaudeTokenStore` used to key by `identity.subject`, and the two entry + * points resolve different ones: a shared OIDC service subject for + * GitHub-webhook triage, `openwebui:` for chat. Authorizing in one flow + * therefore left the other unauthorized. Keying by principal converges them. * - * Both are verified; neither is caller-supplied text. A caller cannot name - * an arbitrary login and inherit someone else's Claude credential. + * The GitHub identity is used as the principal because it is the only + * identifier both flows can reach, from whichever verified source the entry + * point has: + * - **Webhook**: the sender GitHub itself vouched for (signature verified + * upstream; the gateway drops the event if the sender resolves to nothing). + * - **Chat**: the `githubLogin` on a link the caller established by proving + * control of that account. * - * Falls back to the raw subject when no login is resolvable, which preserves - * the exact pre-change behavior rather than failing the turn -- a - * non-canonical key still works, it just isn't shared across flows. + * Neither is caller-supplied text, so nobody can name a login and inherit + * another person's credentials. * - * @param githubGateway The `github`-provider gateway, used ONLY to read an - * existing link's `githubLogin`. Never starts a link: this is a lookup on a - * path that must not have side effects. + * Falls back to the raw subject acting as its own principal when no GitHub + * identity is resolvable. That is a working state, not a failure: the caller + * simply gets no cross-entry-point sharing, exactly as before principals + * existed. */ -export async function resolveCredentialSubject( - provider: string, +export async function resolvePrincipal( rawSubject: string, senderLogin: string | undefined, githubGateway: Pick | undefined, ): Promise { - if (!CANONICAL_PROVIDERS.has(provider)) return rawSubject; - - if (senderLogin) return canonicalSubjectForLogin(senderLogin); - - if (!githubGateway) return rawSubject; - try { - const link = await githubGateway.getToken("github", rawSubject); - if (link?.githubLogin) return canonicalSubjectForLogin(link.githubLogin); - } catch (err) { - // A lookup failure must not take down the turn: falling back to the raw - // subject degrades to the old per-entry-point behavior (an extra - // authorization prompt) instead of a hard error, and the NEXT turn - // retries the lookup. Logged because a silent fallback here would look - // identical to "this user never linked GitHub". - console.error( - `[credential-subject] github link lookup failed for subject ${rawSubject}; falling back to the raw subject (no cross-flow credential sharing this turn): ${ - err instanceof Error ? err.message : String(err) - }`, - ); - } - return rawSubject; + const login = await resolveActorLogin(rawSubject, senderLogin, githubGateway); + return login ? canonicalSubjectForLogin(login) : rawSubject; } diff --git a/apps/agent-orchestrator/src/index.ts b/apps/agent-orchestrator/src/index.ts index 7e38837..10090aa 100644 --- a/apps/agent-orchestrator/src/index.ts +++ b/apps/agent-orchestrator/src/index.ts @@ -552,7 +552,16 @@ async function main(): Promise { taskCompleter, integrationRouteRegistry, agentDelegation?.agentChannel, + config.senderAssertionSecret, ); + if (!config.senderAssertionSecret) { + console.error( + "WARNING: AGENT_SENDER_ASSERTION_SECRET is not set -- a webhook turn's sender login is trusted from the request " + + "body without verification. It selects the caller's principal, and therefore which stored credentials the run " + + "receives (docs/adr/0030). Set it, and integration-gateway's matching GATEWAY_SENDER_ASSERTION_SECRET, to " + + "require a signed assertion instead.", + ); + } if (callbackReceiver) { await callbackReceiver.listen(config.callbackPort); diff --git a/apps/agent-orchestrator/src/rbac/sender-assertion.test.ts b/apps/agent-orchestrator/src/rbac/sender-assertion.test.ts new file mode 100644 index 0000000..aae5845 --- /dev/null +++ b/apps/agent-orchestrator/src/rbac/sender-assertion.test.ts @@ -0,0 +1,36 @@ +import { describe, expect, it } from "vitest"; +import { mintSenderAssertion, verifySenderAssertion } from "./sender-assertion.js"; + +const SECRET = "e2e-assertion-secret"; + +describe("sender assertion", () => { + it("round-trips a login", () => { + expect(verifySenderAssertion(SECRET, mintSenderAssertion(SECRET, "imaustink"))).toBe("imaustink"); + }); + + it("rejects an assertion signed with a different secret", () => { + // The core property: holding the gateway's /invoke token is not enough to + // name a login, because the login selects whose credentials the run gets. + const forged = mintSenderAssertion("attacker-secret", "victim"); + expect(verifySenderAssertion(SECRET, forged)).toBeUndefined(); + }); + + it("rejects a tampered payload even though the signature is well-formed", () => { + const good = mintSenderAssertion(SECRET, "alice"); + const tampered = `${Buffer.from(JSON.stringify({ login: "victim", exp: 9999999999 }), "utf8").toString("base64url")}.${good.split(".")[1]}`; + expect(verifySenderAssertion(SECRET, tampered)).toBeUndefined(); + }); + + it("rejects an expired assertion", () => { + const minted = mintSenderAssertion(SECRET, "alice", 60, Date.parse("2026-01-01T00:00:00Z")); + expect(verifySenderAssertion(SECRET, minted, Date.parse("2026-01-01T00:02:00Z"))).toBeUndefined(); + }); + + it("fails closed on missing, malformed, or unconfigured input", () => { + expect(verifySenderAssertion(SECRET, undefined)).toBeUndefined(); + expect(verifySenderAssertion(SECRET, "not-an-assertion")).toBeUndefined(); + expect(verifySenderAssertion(SECRET, "only-one-part.")).toBeUndefined(); + // No secret configured -> never trust an assertion, rather than trusting all. + expect(verifySenderAssertion("", mintSenderAssertion(SECRET, "alice"))).toBeUndefined(); + }); +}); diff --git a/apps/agent-orchestrator/src/rbac/sender-assertion.ts b/apps/agent-orchestrator/src/rbac/sender-assertion.ts new file mode 100644 index 0000000..459d1a0 --- /dev/null +++ b/apps/agent-orchestrator/src/rbac/sender-assertion.ts @@ -0,0 +1,74 @@ +import { createHmac, timingSafeEqual } from "node:crypto"; + +/** + * Header carrying integration-gateway's signed claim about WHO triggered a + * webhook turn (docs/adr/0030 §6). + * + * The gateway authenticates to `/invoke` with its own service token, so that + * token says "the gateway is calling" and nothing about the human behind it. + * The sender login therefore has to travel separately -- and signed, because + * it selects the caller's principal and hence which stored credentials the run + * receives. An unsigned field would let anything holding the gateway's token + * name an arbitrary login and be handed that person's credentials. + */ +export const SENDER_ASSERTION_HEADER = "x-gateway-user-assertion"; + +/** How long a minted assertion stays valid. Seconds, not hours: it is created and consumed within one HTTP call. */ +const DEFAULT_TTL_SECONDS = 300; + +interface AssertionPayload { + login: string; + /** Unix seconds. */ + exp: number; +} + +function b64url(buf: Buffer): string { + return buf.toString("base64url"); +} + +function sign(secret: string, payloadB64: string): string { + return b64url(createHmac("sha256", secret).update(payloadB64).digest()); +} + +/** + * Mints `.` for a login. + * + * Deliberately not a full JWT: both ends live in this repo, the only claims + * needed are a login and an expiry, and hand-rolling HMAC here avoids adding a + * JWT library to the gateway purely to agree with the orchestrator's. Same + * no-new-dependency precedent as `githubApp.ts`'s JWT signing. + */ +export function mintSenderAssertion(secret: string, login: string, ttlSeconds = DEFAULT_TTL_SECONDS, now = Date.now()): string { + const payload: AssertionPayload = { login, exp: Math.floor(now / 1000) + ttlSeconds }; + const payloadB64 = b64url(Buffer.from(JSON.stringify(payload), "utf8")); + return `${payloadB64}.${sign(secret, payloadB64)}`; +} + +/** + * Returns the asserted login, or `undefined` if the assertion is missing, + * malformed, expired, or not signed by `secret`. + * + * Fails closed and silently, like every other resolver here (ADR 0004): a + * caller that cannot prove who they are is simply treated as not having said, + * which downstream means "no principal" rather than "someone else's principal". + */ +export function verifySenderAssertion(secret: string, assertion: string | undefined, now = Date.now()): string | undefined { + if (!secret || !assertion) return undefined; + const [payloadB64, signature] = assertion.split("."); + if (!payloadB64 || !signature) return undefined; + + const expected = Buffer.from(sign(secret, payloadB64), "utf8"); + const actual = Buffer.from(signature, "utf8"); + // Length check first: timingSafeEqual throws on a length mismatch rather + // than returning false. + if (expected.length !== actual.length || !timingSafeEqual(expected, actual)) return undefined; + + try { + const payload = JSON.parse(Buffer.from(payloadB64, "base64url").toString("utf8")) as AssertionPayload; + if (typeof payload.login !== "string" || !payload.login) return undefined; + if (typeof payload.exp !== "number" || payload.exp * 1000 <= now) return undefined; + return payload.login; + } catch { + return undefined; + } +} diff --git a/apps/agent-orchestrator/src/rbac/types.ts b/apps/agent-orchestrator/src/rbac/types.ts index 48142c0..89635f6 100644 --- a/apps/agent-orchestrator/src/rbac/types.ts +++ b/apps/agent-orchestrator/src/rbac/types.ts @@ -1,7 +1,35 @@ /** Resolved caller identity used to scope RAG retrieval and Job launch permissions. */ export interface Identity { + /** + * The identity as the ENTRY POINT resolved it -- `openwebui:` for chat, + * the gateway's service subject for a webhook relay, an IdP `sub` for OIDC. + * + * Entry-point-specific by nature, which is why it is the wrong thing to key + * durable per-user state on: the same human arriving through two entry + * points has two different subjects. Still the right key for anything + * genuinely scoped to that entry point (sessions, its own GitHub link). + */ subject: string; roles: string[]; + /** + * Stable identifier for the HUMAN, independent of how they arrived + * (docs/adr/0030 §6). + * + * Entry-point subjects are aliases of this. Durable per-user state -- + * notably the Claude credentials -- keys on the principal, so authorizing + * once during GitHub triage is honored in Open WebUI chat and vice versa, + * which is the entire bug this exists to fix. + * + * Today a principal is `github:` when a verified GitHub identity can + * be established for the caller, and otherwise the raw `subject` acting as + * its own principal. That fallback matters: a caller with no GitHub linkage + * still works, they simply get no cross-entry-point sharing, which is + * exactly the pre-principal behavior rather than a failure. + * + * Optional so an `Identity` built by a resolver that predates this (or by a + * test) still type-checks; consumers fall back to `subject`. + */ + principal?: string; } /** diff --git a/apps/agent-orchestrator/src/server.test.ts b/apps/agent-orchestrator/src/server.test.ts index 2340ad2..73ce03b 100644 --- a/apps/agent-orchestrator/src/server.test.ts +++ b/apps/agent-orchestrator/src/server.test.ts @@ -1,4 +1,5 @@ import { describe, expect, it, vi } from "vitest"; +import { SENDER_ASSERTION_HEADER, mintSenderAssertion } from "./rbac/sender-assertion.js"; import { InvokeServer, type AgentGraphLike } from "./server.js"; import type { AgentState } from "./agent/graph.js"; import type { AgentOrchestratorChannel } from "./agents/nats-agent-channel.js"; @@ -1381,3 +1382,77 @@ describe("InvokeServer canonical credential subject plumbing", () => { await server.close(); }); }); + +describe("InvokeServer signed sender assertion (docs/adr/0030 §6)", () => { + const SECRET = "assertion-secret"; + + function serverWithAssertions(graph: AgentGraphLike) { + // Positional order: graph, sessionStore, taskCompleter, + // integrationRouteRegistry, agentChannel, senderAssertionSecret. + return new InvokeServer(graph, undefined, undefined, undefined, undefined, SECRET); + } + + function graphSpy(): AgentGraphLike { + return { + invoke: vi.fn().mockResolvedValue({ request: "x", authToken: "t", skillCandidates: [], result: "done" } as AgentState), + stream: vi.fn().mockResolvedValue(noStream()), + }; + } + + it("accepts a login only from a verified assertion, ignoring the unsigned body field", async () => { + const graph = graphSpy(); + const server = serverWithAssertions(graph); + const port = await listenOn(server); + + await fetch(`http://127.0.0.1:${port}/invoke`, { + method: "POST", + headers: { + "content-type": "application/json", + authorization: "Bearer tok-1", + [SENDER_ASSERTION_HEADER]: mintSenderAssertion(SECRET, "imaustink"), + }, + // A DIFFERENT login in the body: with a secret configured this must be + // ignored outright, not merged or preferred. + body: JSON.stringify({ request: "triage", event: { source: "github", senderLogin: "victim" } }), + }); + await new Promise((r) => setTimeout(r, 10)); + + expect(graph.invoke).toHaveBeenCalledWith(expect.objectContaining({ senderLogin: "imaustink" })); + await server.close(); + }); + + it("drops an unsigned senderLogin entirely when a secret is configured", async () => { + const graph = graphSpy(); + const server = serverWithAssertions(graph); + const port = await listenOn(server); + + await fetch(`http://127.0.0.1:${port}/invoke`, { + method: "POST", + headers: { "content-type": "application/json", authorization: "Bearer tok-1" }, + body: JSON.stringify({ request: "triage", event: { source: "github", senderLogin: "victim" } }), + }); + await new Promise((r) => setTimeout(r, 10)); + + // No principal rather than the WRONG principal: holding the gateway's + // token must not be enough to be handed another person's credentials. + const arg = (graph.invoke as ReturnType).mock.calls[0]?.[0] as { senderLogin?: string }; + expect(arg.senderLogin).toBeUndefined(); + await server.close(); + }); + + it("still trusts the body field when no secret is configured (upgrade compatibility)", async () => { + const graph = graphSpy(); + const server = new InvokeServer(graph); + const port = await listenOn(server); + + await fetch(`http://127.0.0.1:${port}/invoke`, { + method: "POST", + headers: { "content-type": "application/json", authorization: "Bearer tok-1" }, + body: JSON.stringify({ request: "triage", event: { source: "github", senderLogin: "imaustink" } }), + }); + await new Promise((r) => setTimeout(r, 10)); + + expect(graph.invoke).toHaveBeenCalledWith(expect.objectContaining({ senderLogin: "imaustink" })); + await server.close(); + }); +}); diff --git a/apps/agent-orchestrator/src/server.ts b/apps/agent-orchestrator/src/server.ts index cc97aa5..b85e4a8 100644 --- a/apps/agent-orchestrator/src/server.ts +++ b/apps/agent-orchestrator/src/server.ts @@ -21,6 +21,7 @@ import { writeSseDone, writeSseStatus, } from "./openai/chat-completions.js"; +import { SENDER_ASSERTION_HEADER, verifySenderAssertion } from "./rbac/sender-assertion.js"; import type { TaskCompleter } from "./openai/task-completer.js"; import { withHeartbeat } from "./openai/with-heartbeat.js"; @@ -242,6 +243,17 @@ export class InvokeServer { * exist; `/invoke` and the chat-completions facade are unaffected either way. */ private readonly agentChannel?: AgentOrchestratorChannel, + /** + * Shared secret integration-gateway signs its sender assertions with + * (docs/adr/0030 §6). + * + * When set, a webhook turn's sender login is accepted ONLY from a verified + * assertion and the unsigned `event.senderLogin` body field is ignored. + * When unset, the body field is trusted -- the pre-assertion behaviour, + * kept so an existing deployment is not broken by upgrading, and warned + * about at startup (see index.ts). + */ + private readonly senderAssertionSecret?: string, ) {} /** Builds the graph input for one turn, folding in any session-scoped active skill or agent run (docs/adr/0012). */ @@ -604,12 +616,25 @@ export class InvokeServer { // request text and behaves exactly as before this field existed. const rawEvent = (parsed as { event?: unknown }).event; // Read OUTSIDE the IntegrationRoute block below on purpose: the - // canonical credential subject must resolve for every webhook-driven - // turn, including ones that match no route (or run with no route - // registry configured at all) and fall back to ordinary RAG retrieval. - // Gating it on a route match would have made cross-flow credential - // sharing quietly depend on routing config. - if (rawEvent && typeof rawEvent === "object") { + // principal must resolve for every webhook-driven turn, including ones + // that match no route (or run with no route registry configured) and + // fall back to ordinary RAG retrieval. Gating it on a route match would + // have made cross-entry-point credential sharing quietly depend on + // routing config. + // + // WHERE the login is trusted from depends on configuration + // (docs/adr/0030 §6). It selects the caller's principal, and therefore + // which stored credentials the run receives, so with an assertion secret + // configured ONLY a signed assertion is accepted and the body field is + // ignored entirely -- otherwise anything holding the gateway's /invoke + // token could name an arbitrary login and be handed that person's + // credentials. + if (this.senderAssertionSecret) { + senderLogin = verifySenderAssertion( + this.senderAssertionSecret, + headerValue(req.headers[SENDER_ASSERTION_HEADER]), + ); + } else if (rawEvent && typeof rawEvent === "object") { const login = (rawEvent as Record).senderLogin; if (typeof login === "string" && login.trim() !== "") senderLogin = login; } diff --git a/apps/claude-code-swe-agent/src/config.ts b/apps/claude-code-swe-agent/src/config.ts index e4dcd6e..a91af3e 100644 --- a/apps/claude-code-swe-agent/src/config.ts +++ b/apps/claude-code-swe-agent/src/config.ts @@ -15,6 +15,18 @@ export interface AgentToolConfig { * independent secrets. */ githubToken: string; + /** + * The caller's GitHub login, resolved by agent-orchestrator's authorization + * pre-flight and injected as `AGENT_ACTOR_LOGIN` (docs/adr/0030 §5). + * + * When set, this agent MUST NOT resolve identity itself. The orchestrator + * already established who the caller is before launching this run, and the + * `/user` lookup that used to happen here failed with 401 in production -- + * a call this agent had no reason to make. Empty string means the + * orchestrator did not supply one (no `github` provider on this Agent), in + * which case the legacy lookup still applies. + */ + actorLogin: string; /** * GitHub App credentials, used instead of `githubToken` when all three are * set: a short-lived installation access token is minted per run (see @@ -115,6 +127,7 @@ function normalizePem(value: string | undefined): string { export function loadToolConfig(env: NodeJS.ProcessEnv = process.env): AgentToolConfig { return { githubToken: env.GITHUB_TOKEN ?? "", + actorLogin: env.AGENT_ACTOR_LOGIN ?? "", githubAppId: env.GITHUB_APP_ID ?? "", githubAppPrivateKey: normalizePem(env.GITHUB_APP_PRIVATE_KEY), githubAppInstallationId: env.GITHUB_APP_INSTALLATION_ID ?? "", diff --git a/apps/claude-code-swe-agent/src/git.ts b/apps/claude-code-swe-agent/src/git.ts index 85a2639..5caaed0 100644 --- a/apps/claude-code-swe-agent/src/git.ts +++ b/apps/claude-code-swe-agent/src/git.ts @@ -84,8 +84,17 @@ export async function setupGitAuth(opts: { export interface CoAuthor { login: string; - /** Numeric GitHub user id — used to build the standard `id+login@users.noreply.github.com` trailer email. */ - id: number; + /** + * Numeric GitHub user id — used to build the standard + * `id+login@users.noreply.github.com` trailer email. + * + * Optional since docs/adr/0030: when agent-orchestrator supplies the actor + * login via `AGENT_ACTOR_LOGIN`, this agent no longer calls `/user`, so the + * id is unknown. The trailer then uses the `login@users.noreply.github.com` + * form, which GitHub still attributes correctly — it just doesn't pin the + * attribution to an account that later renames. + */ + id?: number; } /** @@ -113,7 +122,10 @@ export async function appendCoAuthorTrailer( const msgRes = await runCommand("git", ["-C", repoDir, "log", "-1", "--pretty=%B"], { env }); if (msgRes.code !== 0) return false; - const email = `${coAuthor.id}+${coAuthor.login}@users.noreply.github.com`; + const email = + coAuthor.id === undefined + ? `${coAuthor.login}@users.noreply.github.com` + : `${coAuthor.id}+${coAuthor.login}@users.noreply.github.com`; const newMessage = `${msgRes.stdout.trimEnd()}\n\nCo-authored-by: ${coAuthor.login} <${email}>\n`; const amendRes = await runCommand("git", ["-C", repoDir, "commit", "--amend", "--allow-empty", "-m", newMessage], { diff --git a/apps/claude-code-swe-agent/src/identityDelegation.ts b/apps/claude-code-swe-agent/src/identityDelegation.ts index e03c5f3..541ba4c 100644 --- a/apps/claude-code-swe-agent/src/identityDelegation.ts +++ b/apps/claude-code-swe-agent/src/identityDelegation.ts @@ -33,7 +33,18 @@ export function isDelegating(config: AgentToolConfig): boolean { export interface DelegatedAttribution { githubLogin: string; - githubId: number; + /** + * Numeric id, used only to build the precise + * `id+login@users.noreply.github.com` co-author trailer. + * + * Optional since docs/adr/0030: when agent-orchestrator supplies the actor + * login via `AGENT_ACTOR_LOGIN`, this agent no longer calls GitHub's + * `/user` -- so the id is unavailable, and the trailer falls back to the + * login-only form. Losing the id is a deliberate trade for deleting an + * identity lookup that duplicated the orchestrator's own and was failing + * 401 in production. + */ + githubId?: number; } /** @@ -70,6 +81,18 @@ export async function resolveDelegatedToken( return { token, attribution: { githubLogin, githubId } }; } + // Prefer the login the orchestrator already resolved (docs/adr/0030 §5). + // Calling /user here duplicates work the authorization pre-flight has + // already done, and it is the exact call that returned + // "401 Bad credentials" in production once this agent gained the `github` + // provider. `githubId` is intentionally absent on this path -- the numeric + // id is only used to build the richer `id+login@` co-author trailer, and + // fetching it would reintroduce the round trip this avoids. + if (config.actorLogin) { + const { token } = await mintInstallationToken(appCreds, config.githubApiUrl, now); + return { token, attribution: { githubLogin: config.actorLogin } }; + } + const { login, id } = await fetchGithubUser(config.githubToken, config.githubApiUrl); const { token } = await mintInstallationToken(appCreds, config.githubApiUrl, now); return { token, attribution: { githubLogin: login, githubId: id } }; diff --git a/apps/claude-code-swe-agent/src/index.ts b/apps/claude-code-swe-agent/src/index.ts index d088ea5..6ae73ae 100644 --- a/apps/claude-code-swe-agent/src/index.ts +++ b/apps/claude-code-swe-agent/src/index.ts @@ -100,7 +100,7 @@ async function handler(session: AgentSession): Promise { const delegating = isDelegating(toolConfig); let token: string; - let attribution: { githubLogin: string; githubId: number } | null = null; + let attribution: { githubLogin: string; githubId?: number } | null = null; if (delegating) { try { const resolved = await resolveDelegatedToken(toolConfig, marker?.repo ?? null, turnStartedAt); diff --git a/apps/integration-gateway/src/config.test.ts b/apps/integration-gateway/src/config.test.ts new file mode 100644 index 0000000..c4481c8 --- /dev/null +++ b/apps/integration-gateway/src/config.test.ts @@ -0,0 +1,32 @@ +import { describe, expect, it } from "vitest"; +import { loadConfig } from "./config.js"; + +describe("loadConfig GitHub host resolution", () => { + it("defaults the OAuth host to github.com, so existing deployments are unaffected", () => { + const config = loadConfig({}); + expect(config.githubBaseUrl).toBe("https://github.com"); + expect(config.githubApiUrl).toBe("https://api.github.com"); + }); + + it("reads GITHUB_BASE_URL independently of GITHUB_API_URL", () => { + // GitHub Enterprise Server splits these: the REST API lives under + // /api/v3 while the OAuth routes stay on the bare host. A single value + // cannot serve both, which is why this is a separate setting rather than + // something derived from githubApiUrl. + const config = loadConfig({ + GITHUB_API_URL: "https://ghe.example.com/api/v3", + GITHUB_BASE_URL: "https://ghe.example.com", + }); + expect(config.githubApiUrl).toBe("https://ghe.example.com/api/v3"); + expect(config.githubBaseUrl).toBe("https://ghe.example.com"); + }); + + it("does not infer the OAuth host from GITHUB_API_URL", () => { + // Setting only the API URL leaves the OAuth host at its default. Guessing + // (e.g. stripping "/api/v3") would silently send a GHES deployment's users + // to a host nobody configured, and it cannot be right for both GHES and + // the github.com/api.github.com split. + const config = loadConfig({ GITHUB_API_URL: "https://ghe.example.com/api/v3" }); + expect(config.githubBaseUrl).toBe("https://github.com"); + }); +}); diff --git a/apps/integration-gateway/src/config.ts b/apps/integration-gateway/src/config.ts index 5e0c852..af6e66f 100644 --- a/apps/integration-gateway/src/config.ts +++ b/apps/integration-gateway/src/config.ts @@ -11,6 +11,23 @@ export interface AppConfig { /** Fallback static PAT, used only if the App fields above are unset. */ githubToken: string; githubApiUrl: string; + /** + * Base URL of GitHub's **web/OAuth** host -- where the device-flow and + * authorization-code endpoints live (`/login/device/code`, + * `/login/oauth/access_token`). Distinct from `githubApiUrl`: on GitHub + * Enterprise Server the REST API lives at `https:///api/v3` while + * these OAuth routes stay on `https://`, so one value cannot serve + * both. Defaults to github.com, matching `@controller-agent/github-app-auth`'s + * own default, so existing deployments are unaffected. + */ + githubBaseUrl: string; + /** + * Shared secret used to sign the sender assertion sent to agent-orchestrator + * (docs/adr/0030 §6). Must match the orchestrator's own + * AGENT_SENDER_ASSERTION_SECRET. Empty disables signing, in which case the + * orchestrator falls back to trusting the unsigned `event.senderLogin`. + */ + senderAssertionSecret: string; /** The App/bot's own GitHub login -- events authored by it are ignored (loop prevention). */ githubBotLogin: string; /** @@ -63,6 +80,21 @@ export interface AppConfig { pollIntervalMs: number; /** Maximum total time (ms) to poll before giving up on a turn. */ pollTimeoutMs: number; + /** + * Maximum time (ms) to hold a PARKED turn open waiting for the user to finish + * linking their account, before giving up and letting them re-trigger. + * + * Separate from `pollTimeoutMs`, and much longer by default, because it is + * bounded by human reaction time rather than by machine latency -- it matches + * the link flow's own ~10-minute expiry. + * + * Configurable because it was not, and a hard-coded 10 minutes is an + * occupancy decision an operator should own: each parked turn holds a relay + * for the whole window. In the e2e environment that is acute -- the + * identity-keying negative controls park on purpose, so a 10-minute hold + * outlives the spec that caused it and starves whatever triggers next. + */ + resumeWaitMs: number; /** Public GitHub App client id used to start OAuth Device Flow links (not a secret). */ githubAppClientId: string; /** Base64 (or hex) 32-byte AES-256-GCM key used to encrypt linked GitHub tokens at rest. */ @@ -127,6 +159,8 @@ export function loadConfig(env: NodeJS.ProcessEnv = process.env): AppConfig { githubAppInstallationId: env.GITHUB_APP_INSTALLATION_ID ?? "", githubToken: env.GITHUB_TOKEN ?? "", githubApiUrl: env.GITHUB_API_URL ?? "https://api.github.com", + githubBaseUrl: env.GITHUB_BASE_URL ?? "https://github.com", + senderAssertionSecret: env.GATEWAY_SENDER_ASSERTION_SECRET ?? "", githubBotLogin: env.GATEWAY_GITHUB_BOT_LOGIN ?? "", githubTriggerLabel: env.GATEWAY_GITHUB_TRIGGER_LABEL ?? "", githubReviewLabel: env.GATEWAY_GITHUB_REVIEW_LABEL ?? "", @@ -141,6 +175,7 @@ export function loadConfig(env: NodeJS.ProcessEnv = process.env): AppConfig { githubCollaboratorRoles: env.GATEWAY_GITHUB_COLLABORATOR_ROLES, pollIntervalMs: num(env.GATEWAY_POLL_INTERVAL_MS, 3_000), pollTimeoutMs: num(env.GATEWAY_POLL_TIMEOUT_MS, 15 * 60 * 1000), + resumeWaitMs: num(env.GATEWAY_RESUME_WAIT_MS, 10 * 60 * 1000), githubAppClientId: env.GITHUB_APP_CLIENT_ID ?? "", identityLinkEncryptionKey: env.IDENTITY_LINK_ENCRYPTION_KEY ?? "", identityLinkToken: env.GATEWAY_IDENTITY_LINK_TOKEN ?? "", diff --git a/apps/integration-gateway/src/index.ts b/apps/integration-gateway/src/index.ts index db45a46..71e76ea 100644 --- a/apps/integration-gateway/src/index.ts +++ b/apps/integration-gateway/src/index.ts @@ -168,7 +168,16 @@ async function main(): Promise { token: orchestratorTokenProvider ? () => orchestratorTokenProvider.getToken() : config.orchestratorToken, pollIntervalMs: config.pollIntervalMs, pollTimeoutMs: config.pollTimeoutMs, + senderAssertionSecret: config.senderAssertionSecret, }); + if (!config.senderAssertionSecret) { + console.error( + "WARNING: GATEWAY_SENDER_ASSERTION_SECRET is not set -- the sender login is relayed to agent-orchestrator UNSIGNED. " + + "That login selects the caller's principal and therefore which stored credentials a run receives, so anything " + + "holding this gateway's /invoke token could name an arbitrary login. Set it (and the orchestrator's matching " + + "AGENT_SENDER_ASSERTION_SECRET) to have the orchestrator require a verified assertion (docs/adr/0030).", + ); + } const githubReplyClient = new GithubReplyClient({ githubToken: config.githubToken, @@ -197,6 +206,11 @@ async function main(): Promise { clientSecret: config.githubAppClientSecret, stateSecret: config.identityLinkStateSecret, redirectUri: config.githubOauthRedirectUri, + // Without this the linker falls back to its own github.com default, + // so a GitHub Enterprise Server deployment would send its users to + // github.com/login/device/code -- which 404s for a GHES-registered + // App's client id. + githubBaseUrl: config.githubBaseUrl, }); } @@ -261,6 +275,7 @@ async function main(): Promise { ...(sessionPageStore ? { sessionPageStore, publicBaseUrl: config.publicUrl } : {}), ...(claudeAuthFlows && claudeTokenStore ? { claudeAuthFlows, claudeAuthStore: claudeTokenStore } : {}), ...(claudeLoginFlows ? { claudeLoginFlows } : {}), + resumeWaitMs: config.resumeWaitMs, }); await server.listen(config.httpPort); diff --git a/apps/integration-gateway/src/orchestrator-client.ts b/apps/integration-gateway/src/orchestrator-client.ts index aa49ab7..2b2dc40 100644 --- a/apps/integration-gateway/src/orchestrator-client.ts +++ b/apps/integration-gateway/src/orchestrator-client.ts @@ -14,6 +14,8 @@ * in docs/integrations-gateway.md -- push-based delivery is a documented * follow-up, not built in this phase. */ + +import { SENDER_ASSERTION_HEADER, mintSenderAssertion } from "./sender-assertion.js"; export interface OrchestratorInvokeResult { status: "succeeded" | "failed" | "timed_out"; result?: string; @@ -35,6 +37,12 @@ export interface OrchestratorInvokeResult { export interface OrchestratorClientOptions { baseUrl: string; + /** + * Signs the sender assertion accompanying each `/invoke` (docs/adr/0030 §6). + * Absent -> no assertion header is sent and the orchestrator falls back to + * the unsigned `event.senderLogin`. + */ + senderAssertionSecret?: string; /** * Either a static bearer token, or a function resolving one on demand * (e.g. `OidcTokenProvider.getToken`, which caches and transparently @@ -195,6 +203,15 @@ export class OrchestratorClient { headers: { "content-type": "application/json", authorization: `Bearer ${await this.resolveToken()}`, + // Signed separately from the bearer token on purpose: the token + // authenticates THIS GATEWAY, and says nothing about which human + // triggered the event. The login selects the caller's principal, and + // therefore which stored credentials the run receives, so it has to + // be something the orchestrator can verify rather than a body field + // any holder of the token could set. + ...(this.options.senderAssertionSecret && typeof event?.senderLogin === "string" + ? { [SENDER_ASSERTION_HEADER]: mintSenderAssertion(this.options.senderAssertionSecret, event.senderLogin) } + : {}), }, body: JSON.stringify({ request, diff --git a/apps/integration-gateway/src/sender-assertion.ts b/apps/integration-gateway/src/sender-assertion.ts new file mode 100644 index 0000000..453b4bd --- /dev/null +++ b/apps/integration-gateway/src/sender-assertion.ts @@ -0,0 +1,51 @@ +import { createHmac } from "node:crypto"; + +/** + * Header carrying integration-gateway's signed claim about WHO triggered a + * webhook turn (docs/adr/0030 §6). + * + * MINTING side. The verifier is agent-orchestrator's + * `src/rbac/sender-assertion.ts`; the two must agree byte-for-byte, which is + * why this is a deliberate copy of ~30 lines of `node:crypto` rather than a + * shared package -- neither app currently depends on the other, and adding a + * package to share one HMAC would cost more than it saves. + * + * This gateway authenticates to `/invoke` with its own service token, so that + * token says "the gateway is calling" and nothing about the human behind it. + * The sender login therefore has to travel separately -- and signed, because + * it selects the caller's principal and hence which stored credentials the run + * receives. An unsigned field would let anything holding the gateway's token + * name an arbitrary login and be handed that person's credentials. + */ +export const SENDER_ASSERTION_HEADER = "x-gateway-user-assertion"; + +/** How long a minted assertion stays valid. Seconds, not hours: it is created and consumed within one HTTP call. */ +const DEFAULT_TTL_SECONDS = 300; + +interface AssertionPayload { + login: string; + /** Unix seconds. */ + exp: number; +} + +function b64url(buf: Buffer): string { + return buf.toString("base64url"); +} + +function sign(secret: string, payloadB64: string): string { + return b64url(createHmac("sha256", secret).update(payloadB64).digest()); +} + +/** + * Mints `.` for a login. + * + * Deliberately not a full JWT: both ends live in this repo, the only claims + * needed are a login and an expiry, and hand-rolling HMAC here avoids adding a + * JWT library to the gateway purely to agree with the orchestrator's. Same + * no-new-dependency precedent as `githubApp.ts`'s JWT signing. + */ +export function mintSenderAssertion(secret: string, login: string, ttlSeconds = DEFAULT_TTL_SECONDS, now = Date.now()): string { + const payload: AssertionPayload = { login, exp: Math.floor(now / 1000) + ttlSeconds }; + const payloadB64 = b64url(Buffer.from(JSON.stringify(payload), "utf8")); + return `${payloadB64}.${sign(secret, payloadB64)}`; +} diff --git a/apps/integration-gateway/src/server.test.ts b/apps/integration-gateway/src/server.test.ts index 1a1420a..cd4c9fe 100644 --- a/apps/integration-gateway/src/server.test.ts +++ b/apps/integration-gateway/src/server.test.ts @@ -631,11 +631,50 @@ describe("GatewayServer unauthenticated triage: deferred ack + auto-resume", () expect(postIssueComment).toHaveBeenCalledTimes(2); // Waited on the gateway-owned claude token store for the gateway subject, - // and re-invoked the SAME triage request once it landed. + // and re-invoked the SAME triage request once it landed. The window is the + // DEFAULT here (no resumeWaitMs configured above) -- see the override test + // below for why that default is worth being able to change. expect(waitForCompletion).toHaveBeenCalledWith("client-integration-gateway", 10 * 60 * 1000, "setup-token"); expect(invoke).toHaveBeenCalledTimes(2); }); + it("honours a configured resumeWaitMs instead of the ten-minute default", async () => { + // A parked turn holds its relay for this entire window, and the window used + // to be a hard-coded constant. That starved the e2e suite: its negative + // controls park on purpose, so each one pinned a relay for ten minutes -- + // outliving the spec that caused it and delaying the next spec's trigger by + // the same ten minutes, which presents as an unrelated keying assertion + // timing out. Deployments where parking is routine need a shorter hold. + await server.close(); + server = new GatewayServer({ + githubWebhookSecret: SECRET, + identityResolver: new GithubIdentityResolver( + new Map([["alice", { subject: "alice", roles: ["reporter"] }]]), + "agent-controller[bot]", + ), + orchestratorClient: { invoke, checkLive: vi.fn().mockResolvedValue({ live: false }) } as unknown as OrchestratorClient, + githubReplyClient: { postIssueComment, removeIssueLabel } as unknown as GithubReplyClient, + githubTriggerLabel: "ai-triage", + sessionPageStore, + publicBaseUrl: "https://gateway.example.com", + claudeAuthStore: { get: vi.fn(), set: vi.fn(), delete: vi.fn(), waitForCompletion } as unknown as ClaudeTokenStore, + resumeWaitMs: 20_000, + }); + await server.listen(0); + const shortPort = (server as unknown as { server: { address: () => AddressInfo } }).server.address().port; + + await postWebhook(shortPort, "issues", { + action: "labeled", + repository: { owner: { login: "acme" }, name: "widgets" }, + sender: { login: "alice", type: "User" }, + issue: { number: 7, title: "Add dark mode", body: "Please add a dark theme option." }, + label: { name: "ai-triage" }, + }); + await flush(); + + expect(waitForCompletion).toHaveBeenCalledWith("client-integration-gateway", 20_000, "setup-token"); + }); + it("also auto-resumes for the claude-remote provider (waitForCompletion called with kind 'login', not 'setup-token')", async () => { // Regression test: waitAndResume used to hardcode // `identityLink.provider === "claude"`, so a claude-remote link prompt diff --git a/apps/integration-gateway/src/server.ts b/apps/integration-gateway/src/server.ts index 8e99a3f..0465d5c 100644 --- a/apps/integration-gateway/src/server.ts +++ b/apps/integration-gateway/src/server.ts @@ -68,6 +68,13 @@ export interface GatewayServerOptions { */ claudeAuthFlows?: ClaudeSetupTokenFlows; claudeAuthStore?: ClaudeTokenStore; + /** + * How long a PARKED turn is held open waiting for the caller to finish + * linking, before giving up. Absent -> {@link + * GatewayServer.DEFAULT_RESUME_WAIT_MS}. Short values are appropriate wherever + * turns park routinely rather than exceptionally (see that constant). + */ + resumeWaitMs?: number; /** * Full-login (`claude auth login --claudeai`) flows for Remote Control * (docs/adr/0027 follow-up) -- optional and additive alongside @@ -377,8 +384,22 @@ export class GatewayServer { } } - /** How long to hold a triage turn open waiting for the user to finish linking their account before giving up (they can always re-trigger the issue later). Matches the link flow's own ~10-minute expiry. */ - private static readonly RESUME_WAIT_MS = 10 * 60 * 1000; + /** + * Default for {@link GatewayServerOptions.resumeWaitMs}: how long to hold a + * parked triage turn open waiting for the user to finish linking their account + * before giving up (they can always re-trigger the issue later). Matches the + * link flow's own ~10-minute expiry. + * + * Was a hard-coded constant, and being unconfigurable had a cost worth + * recording: every parked turn holds a relay for this whole window. The e2e + * suite's identity-keying negative controls park deliberately, so a 10-minute + * hold outlived the spec that caused it and starved the next spec's trigger -- + * surfacing as a positive keying assertion timing out with no orchestrator + * activity at all, which reads exactly like a keying bug. `pollTimeoutMs` had + * already been shortened for e2e for the same class of reason; this hold sat + * next to it, ten times longer, and could not be. + */ + private static readonly DEFAULT_RESUME_WAIT_MS = 10 * 60 * 1000; /** * Shared by `relayAndReply` (webhook-triggered turns) and @@ -473,7 +494,11 @@ export class GatewayServer { const kind = identityLink.provider === "claude" ? "setup-token" : identityLink.provider === "claude-remote" ? "login" : undefined; const store = kind ? this.options.claudeAuthStore : undefined; if (!store || !kind) return undefined; - const record = await store.waitForCompletion(identityLink.subject, GatewayServer.RESUME_WAIT_MS, kind); + const record = await store.waitForCompletion( + identityLink.subject, + this.options.resumeWaitMs ?? GatewayServer.DEFAULT_RESUME_WAIT_MS, + kind, + ); if (!record) return undefined; return reinvoke(); } diff --git a/apps/stub-agent/Dockerfile b/apps/stub-agent/Dockerfile new file mode 100644 index 0000000..d35277f --- /dev/null +++ b/apps/stub-agent/Dockerfile @@ -0,0 +1,64 @@ +# syntax=docker/dockerfile:1 +# +# Build from the REPO ROOT: +# +# docker build -f apps/stub-agent/Dockerfile -t stub-agent:latest . +# +# E2E ONLY. This agent speaks the real NATS agent protocol +# (@controller-agent/agent-runtime) and returns a canned reply -- no model call, +# no credential of its own, no network egress beyond NATS. It exists so the +# happy-path e2e spec can assert the whole webhook -> triage -> AgentRun -> +# reply -> issue-comment chain hermetically; see apps/stub-agent/src/index.ts. +# +# Deliberately unprivileged and toolchain-free, unlike claude-code-swe-agent: +# no git, no gh, no Go, no Claude Code CLI. If this image ever needs one of +# those, the stub has stopped being a stub. + +############################ +# Build stage +############################ +FROM node:22-bookworm-slim AS build +WORKDIR /repo + +COPY package.json package-lock.json* ./ +COPY packages/messaging/package.json packages/messaging/package.json +COPY packages/agent-runtime/package.json packages/agent-runtime/package.json +COPY apps/stub-agent/package.json apps/stub-agent/package.json +RUN npm ci + +COPY packages/messaging packages/messaging +COPY packages/agent-runtime packages/agent-runtime +COPY apps/stub-agent/tsconfig.json apps/stub-agent/tsconfig.json +COPY apps/stub-agent/src apps/stub-agent/src + +RUN npm run build --workspace=@controller-agent/messaging \ + && npm run build --workspace=@controller-agent/agent-runtime \ + && npm run build --workspace=stub-agent \ + && npm prune --omit=dev \ + # Materialize workspace packages as real directories (not symlinks) so they + # survive the copy into the runtime stage. + && rm -rf node_modules/@controller-agent/messaging \ + && mkdir -p node_modules/@controller-agent/messaging \ + && cp -r packages/messaging/dist node_modules/@controller-agent/messaging/dist \ + && cp packages/messaging/package.json node_modules/@controller-agent/messaging/package.json \ + && rm -rf node_modules/@controller-agent/agent-runtime \ + && mkdir -p node_modules/@controller-agent/agent-runtime \ + && cp -r packages/agent-runtime/dist node_modules/@controller-agent/agent-runtime/dist \ + && cp packages/agent-runtime/package.json node_modules/@controller-agent/agent-runtime/package.json + +############################ +# Runtime stage +############################ +FROM node:22-bookworm-slim AS runtime +WORKDIR /app +ENV NODE_ENV=production + +COPY --from=build /repo/node_modules ./node_modules +COPY --from=build /repo/apps/stub-agent/dist ./dist +COPY --from=build /repo/apps/stub-agent/package.json ./package.json + +# The core-controller's hardened Job securityContext runs as non-root with a +# read-only root filesystem and an emptyDir at /tmp. The stub writes nothing. +USER node + +ENTRYPOINT ["node", "dist/index.js"] diff --git a/apps/stub-agent/package.json b/apps/stub-agent/package.json new file mode 100644 index 0000000..6e7a805 --- /dev/null +++ b/apps/stub-agent/package.json @@ -0,0 +1,24 @@ +{ + "name": "stub-agent", + "version": "0.1.0", + "private": true, + "type": "module", + "description": "E2E-only Agent that speaks the real NATS agent protocol and returns a canned reply, so the whole webhook -> triage -> AgentRun -> reply chain can be asserted hermetically without a model credential. Never deployed outside the e2e overlay.", + "engines": { + "node": ">=22" + }, + "scripts": { + "build": "tsc -p tsconfig.json", + "typecheck": "tsc --noEmit", + "test": "vitest run" + }, + "dependencies": { + "@controller-agent/agent-runtime": "0.1.0", + "@controller-agent/messaging": "0.1.0" + }, + "devDependencies": { + "@types/node": "^20.16.5", + "typescript": "^5.6.2", + "vitest": "^4.0.0" + } +} diff --git a/apps/stub-agent/src/index.ts b/apps/stub-agent/src/index.ts new file mode 100644 index 0000000..7cae9f3 --- /dev/null +++ b/apps/stub-agent/src/index.ts @@ -0,0 +1,34 @@ +import { runAgent } from "@controller-agent/agent-runtime"; +import { buildReply, observedCredentialEnv } from "./reply.js"; + +/** + * stub-agent — an Agent that speaks the real NATS protocol and makes no model + * call. + * + * Everything between a GitHub webhook and a comment on the issue is real when + * this runs: routing, RBAC, the authorization pre-flight, AgentRun creation, + * secretEnv injection, the Job's security context, the NATS reply, and the + * gateway's relay of that reply back to GitHub. The only thing replaced is the + * part that cannot be hermetic — the model turn, which needs a paid Anthropic + * credential and returns something different every time. + * + * It exists because that made the happy-path e2e spec impossible to run: a real + * `claude-code-swe-agent` run never reaches a terminal phase in a cluster with + * no credential, so the spec was skipped and the whole chain went unasserted. + * + * NOT for any non-e2e deployment. It is enabled only by + * charts/community-components/values-e2e.yaml. + */ +await runAgent(async (session) => { + await session.progress("stub-agent received the goal", { stage: "start", pct: 10 }); + + // Credential env var NAMES that arrived, never values -- this string is + // posted verbatim to a GitHub issue comment by the gateway's relay, so a + // value here would be published. Names are the useful part anyway: they are + // what proves the orchestrator's secretEnv injection reached the container, + // which is otherwise only observable on the Job spec. + const observed = observedCredentialEnv(process.env); + await session.progress(`credential env present: ${observed.join(", ") || "(none)"}`, { stage: "inspect", pct: 60 }); + + return buildReply(session.goal, observed); +}); diff --git a/apps/stub-agent/src/reply.test.ts b/apps/stub-agent/src/reply.test.ts new file mode 100644 index 0000000..c17acee --- /dev/null +++ b/apps/stub-agent/src/reply.test.ts @@ -0,0 +1,55 @@ +import { describe, expect, it } from "vitest"; +import { buildReply, CREDENTIAL_ENV_NAMES, observedCredentialEnv, STUB_REPLY_MARKER } from "./reply.js"; + +describe("observedCredentialEnv", () => { + it("reports the names of credential vars that are set", () => { + expect(observedCredentialEnv({ AGENT_ACTOR_LOGIN: "e2e-user", CLAUDE_CODE_OAUTH_TOKEN: "sk-ant-oat01-x" })).toEqual([ + "AGENT_ACTOR_LOGIN", + "CLAUDE_CODE_OAUTH_TOKEN", + ]); + }); + + it("treats an empty value as absent", () => { + // A `secretEnv` entry pointing at a Secret key that exists but is empty + // yields a set-but-empty var. Reporting it as present would make the + // happy-path spec pass on an injection that delivered nothing. + expect(observedCredentialEnv({ AGENT_ACTOR_LOGIN: "" })).toEqual([]); + }); + + it("ignores env vars outside the known credential list", () => { + expect(observedCredentialEnv({ SOME_OTHER_TOKEN: "x", PATH: "/usr/bin" })).toEqual([]); + }); + + it("never returns a value, only a name", () => { + const secret = "sk-ant-oat01-super-secret"; + const reported = observedCredentialEnv({ CLAUDE_CODE_OAUTH_TOKEN: secret }); + expect(reported.join(" ")).not.toContain(secret); + }); +}); + +describe("buildReply", () => { + it("carries the marker the happy-path spec matches on", () => { + expect(buildReply("do a thing", [])).toContain(STUB_REPLY_MARKER); + }); + + it("echoes the goal so the spec can prove it survived the whole path", () => { + expect(buildReply("triage issue #7", ["AGENT_ACTOR_LOGIN"])).toContain("triage issue #7"); + }); + + it("truncates a long goal rather than mirroring an entire issue body into a comment", () => { + const reply = buildReply("x".repeat(500), []); + expect(reply).toContain("..."); + expect(reply.length).toBeLessThan(400); + }); + + it("says so explicitly when no credential env arrived", () => { + // Distinguishes "injection delivered nothing" from "the stub forgot to + // look", which an empty list in the comment would not. + expect(buildReply("g", [])).toContain("(none)"); + }); + + it("lists every credential env name it was given", () => { + const reply = buildReply("g", [...CREDENTIAL_ENV_NAMES]); + for (const name of CREDENTIAL_ENV_NAMES) expect(reply).toContain(name); + }); +}); diff --git a/apps/stub-agent/src/reply.ts b/apps/stub-agent/src/reply.ts new file mode 100644 index 0000000..7d6fc15 --- /dev/null +++ b/apps/stub-agent/src/reply.ts @@ -0,0 +1,59 @@ +/** + * The canned reply, kept out of index.ts so it is unit-testable without a NATS + * connection. + */ + +/** + * Marker every stub reply carries. + * + * The happy-path spec matches on this rather than on prose: it has to + * distinguish "the stub replied and the gateway relayed it" from "some comment + * appeared", and asserting on a sentence invites a spec that breaks when the + * wording is reworded. + */ +export const STUB_REPLY_MARKER = "stub-agent-reply"; + +/** + * Env vars the authorization pre-flight injects as `secretEnv`, in the order + * reported. Deliberately a fixed list rather than "every var that looks like a + * credential": a prefix heuristic over `process.env` would report whatever the + * base image happens to set, and could match a real secret the list never meant + * to name. + * + * Kept in sync by hand with `PROVIDER_ENV_VAR` and `ACTOR_LOGIN_ENV` in + * apps/agent-orchestrator/src/agent/graph.ts. A name that drifts shows up as a + * missing entry in the reply, not as a silent pass. + */ +export const CREDENTIAL_ENV_NAMES = [ + "AGENT_ACTOR_LOGIN", + "CLAUDE_CODE_OAUTH_TOKEN", + "CLAUDE_LOGIN_CREDENTIALS_JSON", + "GITHUB_TOKEN", +] as const; + +/** + * NAMES of the credential env vars that are present and non-empty. Never + * values: the caller publishes this to a GitHub issue comment. + */ +export function observedCredentialEnv(env: NodeJS.ProcessEnv): string[] { + return CREDENTIAL_ENV_NAMES.filter((name) => (env[name] ?? "") !== ""); +} + +/** + * The reply the orchestrator relays to the user (a GitHub issue comment, in the + * triage flow). + * + * Echoes the goal so the spec can prove the goal survived the whole webhook -> + * route -> orchestrator -> AgentRun -> Job -> NATS path intact, rather than + * only that *something* replied. Truncated because a goal can carry an entire + * issue body and a comment is not the place to mirror it back in full. + */ +export function buildReply(goal: string, observedEnv: string[]): string { + const truncated = goal.length > 200 ? `${goal.slice(0, 200)}...` : goal; + return [ + `${STUB_REPLY_MARKER}: no model was called.`, + "", + `goal: ${truncated}`, + `credential env present: ${observedEnv.join(", ") || "(none)"}`, + ].join("\n"); +} diff --git a/apps/stub-agent/tsconfig.json b/apps/stub-agent/tsconfig.json new file mode 100644 index 0000000..5e25135 --- /dev/null +++ b/apps/stub-agent/tsconfig.json @@ -0,0 +1,21 @@ +{ + "compilerOptions": { + "target": "ES2022", + "module": "NodeNext", + "moduleResolution": "NodeNext", + "lib": ["ES2022"], + "outDir": "dist", + "rootDir": "src", + "strict": true, + "noUncheckedIndexedAccess": true, + "esModuleInterop": true, + "skipLibCheck": true, + "resolveJsonModule": true, + "declaration": true, + "sourceMap": false, + "forceConsistentCasingInFileNames": true, + "types": ["node"] + }, + "include": ["src/**/*"], + "exclude": ["src/**/*.test.ts", "node_modules", "dist"] +} diff --git a/apps/stub-agent/vitest.config.ts b/apps/stub-agent/vitest.config.ts new file mode 100644 index 0000000..ce36a74 --- /dev/null +++ b/apps/stub-agent/vitest.config.ts @@ -0,0 +1,8 @@ +import { defineConfig } from "vitest/config"; + +export default defineConfig({ + test: { + include: ["src/**/*.test.ts"], + environment: "node", + }, +}); diff --git a/charts/agent-controller/charts/agent-orchestrator/templates/deployment.yaml b/charts/agent-controller/charts/agent-orchestrator/templates/deployment.yaml index 3635f6a..1438186 100644 --- a/charts/agent-controller/charts/agent-orchestrator/templates/deployment.yaml +++ b/charts/agent-controller/charts/agent-orchestrator/templates/deployment.yaml @@ -177,6 +177,14 @@ spec: key: {{ .Values.secrets.qdrantApiKeyKey }} optional: true {{- end }} + {{- if .Values.secrets.senderAssertionSecretKey }} + - name: AGENT_SENDER_ASSERTION_SECRET + valueFrom: + secretKeyRef: + name: {{ .Values.secrets.existingSecret | default (include "agent-orchestrator.fullname" .) }} + key: {{ .Values.secrets.senderAssertionSecretKey }} + optional: true + {{- end }} {{- if .Values.identityLink.enabled }} - name: IDENTITY_LINK_GATEWAY_TOKEN valueFrom: diff --git a/charts/agent-controller/charts/agent-orchestrator/values.yaml b/charts/agent-controller/charts/agent-orchestrator/values.yaml index 770d8e2..b452b40 100644 --- a/charts/agent-controller/charts/agent-orchestrator/values.yaml +++ b/charts/agent-controller/charts/agent-orchestrator/values.yaml @@ -163,6 +163,11 @@ secrets: qdrantApiKeyKey: AGENT_QDRANT_API_KEY # Only used when identityLink.enabled=true above. identityLinkGatewayTokenKey: IDENTITY_LINK_GATEWAY_TOKEN + # Verifies integration-gateway's signed sender assertion (docs/adr/0030 §6). + # MUST match that chart's senderAssertionSecretKey value. When the key is + # absent from the Secret the orchestrator falls back to trusting the + # unsigned request-body login and warns at startup. + senderAssertionSecretKey: AGENT_SENDER_ASSERTION_SECRET # Only read when secrets.create=true. openaiApiKey: "" callbackSecret: "" diff --git a/charts/agent-controller/charts/core-controller/crds/README.md b/charts/agent-controller/charts/core-controller/crds/README.md index 40c77f0..baec585 100644 --- a/charts/agent-controller/charts/core-controller/crds/README.md +++ b/charts/agent-controller/charts/core-controller/crds/README.md @@ -14,6 +14,39 @@ make manifests # regenerates config/crd/bases/*.yaml AND copies them here make sync-crds ``` -CI (`.github/workflows/publish-charts.yml`) runs the same sync before -packaging, so published chart artifacts always carry current CRDs even -though this directory isn't committed. +CI (`.github/workflows/release.yml`, the `publish-charts` job) runs the same +sync before packaging, so published chart artifacts always carry current CRDs +even though this directory isn't committed. + +**This directory does not keep a running cluster's CRDs current, and cannot.** +Because `helm upgrade` ignores `crds/`, an existing release never receives a +newly-added CRD and never receives a schema change to an existing one — and an +outdated schema is worse than a missing one, since the API server accepts CRs +that reference new fields and silently prunes those fields away. The +`release.yml` `deploy` job therefore applies +`controllers/core-controller/config/crd/bases/` with `kubectl apply +--server-side --force-conflicts` on every push, before either `helm upgrade`. +That step is the mechanism that keeps cluster CRDs current; this directory only +covers a first install and the packaged artifact. + +## Cluster permission that step requires + +CRDs are **cluster-scoped**. Every other `kubectl`/`helm` call in the `deploy` +job is namespaced, so this is the only thing in the pipeline needing +cluster-scoped rights — which is part of why the gap went unnoticed for so long. +The job runs on an in-cluster ARC runner with no kubeconfig step, so it +authenticates as that runner pod's own ServiceAccount, which is cluster +infrastructure this repository does not manage and cannot grant itself. + +The step preflights the permission and fails with this same instruction rather +than surfacing a raw RBAC denial against whichever CRD happened to be applied +first. Grant the runner's ServiceAccount a ClusterRole containing: + +```yaml +- apiGroups: ["apiextensions.k8s.io"] + resources: ["customresourcedefinitions"] + verbs: ["get", "list", "create", "update", "patch"] +``` + +No `delete`: nothing in the pipeline removes a CRD, and deleting one cascades to +every CR of that kind. diff --git a/charts/agent-controller/charts/integration-gateway/templates/deployment.yaml b/charts/agent-controller/charts/integration-gateway/templates/deployment.yaml index 83c5e76..2935aa8 100644 --- a/charts/agent-controller/charts/integration-gateway/templates/deployment.yaml +++ b/charts/agent-controller/charts/integration-gateway/templates/deployment.yaml @@ -49,6 +49,8 @@ spec: value: {{ .Values.config.orchestratorUrl | quote }} - name: GITHUB_API_URL value: {{ .Values.config.githubApiUrl | quote }} + - name: GITHUB_BASE_URL + value: {{ .Values.config.githubBaseUrl | quote }} - name: GATEWAY_GITHUB_BOT_LOGIN value: {{ .Values.config.githubBotLogin | quote }} - name: GATEWAY_GITHUB_TRIGGER_LABEL @@ -71,6 +73,8 @@ spec: value: {{ .Values.config.pollIntervalMs | quote }} - name: GATEWAY_POLL_TIMEOUT_MS value: {{ .Values.config.pollTimeoutMs | quote }} + - name: GATEWAY_RESUME_WAIT_MS + value: {{ .Values.config.resumeWaitMs | quote }} {{- if .Values.sessionPage.publicBaseUrl }} - name: GATEWAY_PUBLIC_URL value: {{ .Values.sessionPage.publicBaseUrl | quote }} @@ -152,6 +156,14 @@ spec: secretKeyRef: name: {{ .Values.secrets.existingSecret | default (include "integration-gateway.fullname" .) }} key: {{ .Values.secrets.identityLinkTokenKey }} + {{- if .Values.secrets.senderAssertionSecretKey }} + - name: GATEWAY_SENDER_ASSERTION_SECRET + valueFrom: + secretKeyRef: + name: {{ .Values.secrets.existingSecret | default (include "integration-gateway.fullname" .) }} + key: {{ .Values.secrets.senderAssertionSecretKey }} + optional: true + {{- end }} - name: GITHUB_APP_CLIENT_SECRET valueFrom: secretKeyRef: diff --git a/charts/agent-controller/charts/integration-gateway/values.yaml b/charts/agent-controller/charts/integration-gateway/values.yaml index ef5cede..e7805e5 100644 --- a/charts/agent-controller/charts/integration-gateway/values.yaml +++ b/charts/agent-controller/charts/integration-gateway/values.yaml @@ -35,6 +35,15 @@ config: # # GitHub API base URL (override for GitHub Enterprise Server). githubApiUrl: "https://api.github.com" + # GitHub WEB/OAuth base URL -- where the identity-link device-flow and + # authorization-code endpoints live (/login/device/code, + # /login/oauth/access_token). Separate from githubApiUrl because GitHub + # Enterprise Server splits them: the REST API sits at https:///api/v3 + # while these OAuth routes stay on https://. Leaving this at the + # github.com default on a GHES deployment sends users to + # github.com/login/device/code, which 404s for a GHES-registered App's + # client id. Only consulted when identityLink.enabled=true. + githubBaseUrl: "https://github.com" # The App/bot's own GitHub login -- its own comments are ignored (loop # prevention). Required once the App/webhook is wired up. githubBotLogin: "" @@ -88,6 +97,15 @@ config: githubIdentities: "" pollIntervalMs: 3000 pollTimeoutMs: 900000 + # How long a PARKED turn (one waiting on the caller to finish linking an + # account) holds its relay open before giving up. Much longer than + # pollTimeoutMs above because it is bounded by human reaction time, and it + # matches the link flow's own ~10-minute expiry. + # + # Lower it wherever turns park routinely rather than exceptionally: each + # parked turn occupies a relay for this whole window, so a long hold delays + # unrelated triggers behind it. + resumeWaitMs: 600000 # Automatic OIDC client_credentials token fetch/refresh for this gateway's # own /invoke calls to agent-orchestrator -- an opt-in alternative to @@ -223,6 +241,11 @@ secrets: identityLinkTokenKey: GATEWAY_IDENTITY_LINK_TOKEN githubAppClientSecretKey: GITHUB_APP_CLIENT_SECRET identityLinkStateSecretKey: IDENTITY_LINK_STATE_SECRET + # Signs the sender assertion sent to agent-orchestrator (docs/adr/0030 §6). + # MUST match agent-orchestrator's senderAssertionSecretKey value. Unset (or + # absent from the Secret) means the sender login is relayed unsigned and the + # orchestrator falls back to trusting it -- both sides log a warning. + senderAssertionSecretKey: GATEWAY_SENDER_ASSERTION_SECRET # Only read when secrets.create=true. githubWebhookSecret: "" orchestratorToken: "" diff --git a/charts/agent-controller/values-e2e.yaml b/charts/agent-controller/values-e2e.yaml new file mode 100644 index 0000000..3c7eb1c --- /dev/null +++ b/charts/agent-controller/values-e2e.yaml @@ -0,0 +1,162 @@ +# values-e2e.yaml — minikube overlay for the e2e suite (see e2e/README.md). +# +# Layered ON TOP of values-minikube-demo.yaml, not instead of it: +# helm upgrade --install agent-controller charts/agent-controller \ +# -f charts/agent-controller/values-minikube-demo.yaml \ +# -f charts/agent-controller/values-e2e.yaml +# +# The demo values exist to make a human-usable playground; this file adds only +# what an automated test needs on top: an integration-gateway to receive +# webhooks (the demo profile leaves it disabled, so there is nothing for a +# webhook test to talk to), a GitHub API redirected at the in-cluster stub, and +# deterministic identity/routing config. +# +# EVERY secret value referenced here is a throwaway generated at bring-up time +# by `e2e/scripts/bootstrap-secrets.sh`. Nothing in this file is a real +# credential, and none of it is shared with any other environment. The one +# genuinely real key the stack needs (OPENAI_API_KEY, for the orchestrator's +# planner) is created out-of-band by a human — see that script. + +integration-gateway: + # The whole point of this overlay. Off in every other profile. + enabled: true + + config: + # Redirects ALL GitHub REST traffic at the in-cluster stub + # (e2e/manifests/fake-github.yaml), which both serves deterministic + # responses and records what we posted so tests can assert on it directly + # instead of scraping logs. Nothing reaches github.com from an e2e run. + githubApiUrl: "http://fake-github.controller-agent.svc.cluster.local" + + # The OAuth/web host, redirected at the same stub. Without this the + # identity-link device flow calls real github.com/login/device/code with + # the placeholder client id below and gets a 404 -- and because `github` + # is the FIRST declared provider, that failure ends the turn before + # `claude`/`claude-remote` are ever reached, so no credential subject is + # ever keyed and the keying spec has nothing to observe. + githubBaseUrl: "http://fake-github.controller-agent.svc.cluster.local" + + # Must match what the specs send. `fake-github` never originates events; + # the suite signs and posts them itself with the same HMAC the gateway + # verifies, so the signature path is exercised for real. + githubTriggerLabel: "ai-triage" + + # Loop prevention: the gateway drops events sent by its own bot login. + # Specs deliberately use sender logins OTHER than this one, since an + # event from the bot is dropped before any identity resolution happens + # and the test would hang waiting for a run that was never going to start. + githubBotLogin: "e2e-bot" + + # DEV-GRADE static allowlist (GithubIdentityResolver). Used here instead + # of team-membership resolution because the prod-grade resolver calls + # GitHub's org/team API for a real org — unavailable and undesirable in a + # hermetic test. These logins are exactly the ones the specs send as + # `sender.login`. + # + # `subject` is intentionally NOT a `github:*` value: the canonical + # credential subject must be DERIVED by resolveCredentialSubject from the + # webhook's senderLogin (ADR 0029), not handed to it preconfigured. If + # these subjects were already canonical the identity-keying spec would + # pass without the code under test doing anything. + githubIdentities: | + { + "e2e-user": {"subject": "e2e-static-user", "roles": ["reader", "writer"]}, + "e2e-other-user": {"subject": "e2e-static-other", "roles": ["reader", "writer"]}, + "E2E-User": {"subject": "e2e-static-user", "roles": ["reader", "writer"]} + } + + # Aggressively short, and this is load-bearing for suite reliability. + # + # The gateway holds a relay open for the whole poll window. The negative + # controls deliberately trigger turns that never launch, so with a long + # window each one occupies the relay for its full duration and whichever + # positive spec runs afterwards queues behind that backlog -- observed as + # the LAST positive test timing out while identical earlier ones passed, + # which reads as a keying bug and is not one. A short window lets an + # abandoned turn release the relay promptly. + pollIntervalMs: 2000 + pollTimeoutMs: 90000 + + # The OTHER hold, and the one that actually starved this suite. + # + # Shortening pollTimeoutMs above fixed the completed-turn case but missed + # this: a turn that PARKS on a missing account link is held open separately, + # by waitAndResume, for a window that was hard-coded to TEN MINUTES -- + # unaffected by anything above it. The identity-keying negative controls + # park by design, so each one pinned a relay for ten minutes, long outliving + # the spec that caused it and starving whatever triggered next. + # + # Diagnosis, since the symptom is so misleading: the starved spec times out + # waiting for an AgentRun while the orchestrator logs NO authorization + # verdict at all. No verdict means /invoke was never reached, so the stall is + # upstream in the relay -- not the gate, and not keying, which is what a + # positive keying assertion failing here looks like. + # + # 20s: long enough to be a real wait, short enough that a deliberately + # parked turn releases before the next spec's 60s absence-window. + resumeWaitMs: 20000 + + # Per-user credential brokering — required by the identity-keying spec, + # which asserts which subject each entry point keys a Claude credential + # under. Without this the /identity-link/* and /claude-auth/* APIs the + # orchestrator calls simply aren't served. + identityLink: + enabled: true + # GitHub App client ids are public by design (the chart documents this). + # This is a placeholder: no e2e spec completes a real OAuth round trip, + # they assert on the subject a flow is KEYED under, which is decided + # before any redirect happens. + githubAppClientId: "Iv23li-e2e-placeholder" + + claudeAuth: + # Needed for `claude`/`claude-remote` flows to be startable at all. The + # PTY-driven `claude login` cannot succeed without a real Anthropic + # credential, and no spec expects it to — they assert on the Redis key a + # started flow is keyed under, which is decided before the PTY runs. + enabled: true + + sessionPage: + # claudeAuth.enabled requires this to be set or the chart refuses to + # render. Any reachable-looking value satisfies it; nothing in the suite + # loads the page. + publicBaseUrl: "http://agent-controller-integration-gateway.controller-agent.svc.cluster.local:8090" + + secrets: + # Bring-your-own, exactly like prod — NOT chart-created. + # + # `create: true` here fails the deploy outright: bootstrap-secrets.sh has + # already made this secret, Helm then refuses to adopt an object it + # doesn't own ("invalid ownership metadata ... missing key + # meta.helm.sh/release-name"), and the whole release rolls back. Letting + # the script own the secret and Helm merely reference it sidesteps the + # conflict and keeps generated values out of version control. + # + # The name is deliberately NOT `agent-controller-integration-gateway` — + # that is the name the chart itself would generate, so reusing it invites + # the very ownership collision described above the moment anyone flips + # `create` back on. + create: false + existingSecret: e2e-integration-gateway-secrets + +agent-orchestrator: + # The orchestrator's CLIENT side of the identity-link channel. Enabling it + # only on the gateway (the server side) leaves the orchestrator with no + # gateway configured at all -- `hasIdentityLinkGateway: false` in its + # identity-gate debug log -- so no Agent's identityProviders can ever + # resolve and no credential flow starts. The identity-keying spec asserts + # on exactly those flows, so without this it can only ever time out. + identityLink: + enabled: true + # In-cluster Service DNS for the gateway deployed by this same release. + gatewayUrl: "http://agent-controller-integration-gateway:8090" + config: + # The gateway authenticates to /invoke with this bearer token, and the + # orchestrator must resolve it to a role the SWE agent allows. + # + # This is the shared service subject at the heart of the bug the suite + # exists to pin: EVERY webhook-driven turn arrives as this one identity, + # which is why `senderLogin` had to be plumbed through separately for + # per-user credential keying to be possible at all (ADR 0029). The + # 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"]}}' diff --git a/charts/community-components/templates/agent-stub.yaml b/charts/community-components/templates/agent-stub.yaml new file mode 100644 index 0000000..ee32777 --- /dev/null +++ b/charts/community-components/templates/agent-stub.yaml @@ -0,0 +1,58 @@ +{{- if .Values.stubAgent.enabled }} +{{- /* + E2E-ONLY Agent. Default-disabled in values.yaml and enabled solely by + values-e2e.yaml -- it returns a canned reply and does no work, so a + production cluster that switched a route to it would silently answer every + request with a stub. Fail loudly rather than let that be a values typo. +*/}} +{{- if not .Values.stubAgent.acknowledgeNotForProduction }} +{{- fail "stubAgent is an e2e-only test double that answers every request with a canned reply. Set stubAgent.acknowledgeNotForProduction=true to enable it." }} +{{- end }} +apiVersion: {{ .Values.crdApiVersion }} +kind: Agent +metadata: + name: stub-agent + labels: + {{- include "tools.labels" . | nindent 4 }} + e2e: "true" +spec: + description: >- + E2E test double. Speaks the real NATS agent protocol and returns a canned + reply without calling a model. Not for production use. + input: Anything; the goal is echoed back rather than acted on. + output: A fixed acknowledgement carrying the goal and the credential env var names that arrived. + allowedRoles: + - writer + # Unprivileged, unlike claude-code-swe-agent: the stub needs NATS and nothing + # else -- no repo checkout, no git/gh, no egress. + tier: standard + {{- if .Values.stubAgent.identityLink.enabled }} + {{- /* + Mirrors the agent this stands in for. The authorization pre-flight derives + which credentials to resolve, and which secretEnv names to inject, purely + from this list (PROVIDER_ENV_VAR in agent-orchestrator/src/agent/graph.ts), + so declaring the same providers means the identity gate behaves identically + -- including refusing to launch when a credential is missing, which is what + the identity-keying spec's negative controls assert. A stub that declared + no providers would sail past the gate and make that suite vacuous. + */}} + identityProviders: + {{- range .Values.stubAgent.identityLink.providers }} + - {{ . }} + {{- end }} + {{- end }} + image: {{ .Values.stubAgent.image }} + serviceAccountName: {{ .Values.stubAgent.serviceAccountName }} + resources: + requests: + cpu: "50m" + memory: "64Mi" + limits: + cpu: "500m" + memory: "256Mi" + orchestratorPrompt: | + Do not delegate to this agent. It is an end-to-end test double that + returns a canned reply without doing the requested work. + agentPrompt: | + Unused: this agent makes no model call. +{{- end }} diff --git a/charts/community-components/templates/serviceaccount-stub-agent.yaml b/charts/community-components/templates/serviceaccount-stub-agent.yaml new file mode 100644 index 0000000..32a2e3b --- /dev/null +++ b/charts/community-components/templates/serviceaccount-stub-agent.yaml @@ -0,0 +1,19 @@ +{{- if and .Values.stubAgent.enabled .Values.stubAgent.serviceAccount.create }} +apiVersion: v1 +kind: ServiceAccount +metadata: + name: {{ .Values.stubAgent.serviceAccountName }} + namespace: {{ .Release.Namespace }} + labels: + {{- include "tools.labels" . | nindent 4 }} + e2e: "true" + {{- with .Values.stubAgent.serviceAccount.annotations }} + annotations: + {{- toYaml . | nindent 4 }} + {{- end }} +{{- with .Values.imagePullSecrets }} +imagePullSecrets: + {{- toYaml . | nindent 2 }} +{{- end }} +automountServiceAccountToken: {{ .Values.stubAgent.serviceAccount.automount }} +{{- end }} diff --git a/charts/community-components/values-ci-all.yaml b/charts/community-components/values-ci-all.yaml new file mode 100644 index 0000000..f1cfcd2 --- /dev/null +++ b/charts/community-components/values-ci-all.yaml @@ -0,0 +1,85 @@ +# values-ci-all.yaml — enable EVERY component, for CI validation only. +# +# Consumed by .github/workflows/validate-crds.yml, which renders this chart and +# dry-run applies the result against a real API server (a kind cluster) so CEL +# `x-kubernetes-validations` rules — which no static schema linter evaluates — +# are actually checked. +# +# It exists because that workflow used to carry the same list as a wall of +# `--set` flags, and the list went stale silently: it enabled two values that no +# longer exist (`opencodeSweAgentTool`, `skills.softwareEngineering`) while +# never enabling six templates that do — including +# `agent-claude-code-swe.yaml`, the Agent production actually runs. Anything +# not enabled here is never validated, and nothing complained. +# +# The workflow now also asserts that every template under templates/ appears in +# the render, so a newly-added component that is missing from this file fails CI +# instead of quietly going unvalidated. If that check fails, add the component +# here rather than relaxing the check. +# +# NOT a deployable values file: it turns on an e2e-only test double +# (stubAgent) and enables components that conflict in intent (both SWE agents at +# once). Production is charts/community-components/values-production.yaml. + +enabled: true + +recipeScraper: + enabled: true + +webSearch: + enabled: true + +webFetch: + enabled: true + +kubectlReadonly: + enabled: true + +recipePublisher: + enabled: true + # Required by the template when enabled; never reached, the render is discarded. + mealieBaseUrl: http://mealie.example.invalid + +opencodeSweAgent: + enabled: true + +# The Agent production runs, and previously the one the "full catalog" +# validation never touched. Its template hard-`fail`s unless some model +# credential is guaranteed at launch time, so identityLink must cover `claude`. +claudeCodeSweAgent: + enabled: true + identityLink: + enabled: true + providers: + - claude + - claude-remote + remoteControl: + enabled: true + +# E2E-only test double. Validated here anyway: it is a real chart template that +# renders a real Agent CR, and the acknowledgement flag below is exactly the +# guard that keeps it out of a production values file. +stubAgent: + enabled: true + acknowledgeNotForProduction: true + identityLink: + enabled: true + providers: + - claude + - claude-remote + +skills: + recipeRefining: + enabled: true + selfImprovement: + enabled: true + webSearch: + enabled: true + +integrationRoutes: + githubIssueLabeledTriage: + enabled: true + githubPrLabeledReview: + enabled: true + githubPrLabeledTriage: + enabled: true diff --git a/charts/community-components/values-e2e.yaml b/charts/community-components/values-e2e.yaml new file mode 100644 index 0000000..2bd16f2 --- /dev/null +++ b/charts/community-components/values-e2e.yaml @@ -0,0 +1,85 @@ +# values-e2e.yaml — community-components overlay for the e2e suite. +# +# Layered on top of values-minikube-demo.yaml (see charts/agent-controller/ +# values-e2e.yaml for the sibling overlay and the umbrella rationale). +# +# The demo profile triages with opencode-swe-agent, which declares no +# identityProviders at all. That makes it useless for the identity-keying +# spec: the gate's provider loop iterates an empty list and goes straight to +# launch (`hasIdentitySecretEnv: false`), so no Claude credential flow is ever +# started and there is nothing to assert a subject against. +# +# Production triages with claude-code-swe-agent carrying the Claude providers, +# and that is precisely the path ADR 0029 changed — so the e2e environment +# mirrors it rather than the demo shortcut. + +# The agent the triage route actually launches here. +# +# claude-code-swe-agent below stays enabled and keeps its Claude +# identityProviders -- the identity gate is the thing under test and it must see +# a production-shaped Agent -- but a REAL Claude run cannot terminate in this +# cluster: there is no Anthropic credential, by design (see the comment on +# anthropicSecretKey). The run therefore never reached a terminal phase, so the +# happy-path spec could not assert the chain at all and was skipped. +# +# stub-agent closes that gap without weakening the gate: it declares the SAME +# identityProviders, so the pre-flight resolves the same credentials, injects the +# same secretEnv names, and still refuses to launch when a credential is missing +# (the keying spec's negative controls). Only the model turn is replaced. +stubAgent: + enabled: true + acknowledgeNotForProduction: true + # Overwritten by skaffold's setValueTemplates with the freshly-built tag. + image: stub-agent:latest + identityLink: + enabled: true + # MUST match claudeCodeSweAgent.identityLink.providers below. Drifting these + # apart makes the gate take a different path for the agent under test than + # for the one production runs, which is the whole point of the stub. + providers: + - claude + - claude-remote + +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 + # step with, and re-pointing the route back at it is a one-line change when + # a run with a real credential is what's wanted. + enabled: true + # Overwritten by skaffold's setValueTemplates with the freshly-built tag. + # Left as the chart default here so a plain `helm template` still renders. + image: claude-code-swe-agent:latest + secretName: claude-code-swe-secrets + secretKey: GITHUB_TOKEN + # No static Anthropic credential on purpose: with "claude" among the + # identityProviders below, the gate refuses to launch until a credential is + # linked. A static fallback would mask exactly the gate behaviour under test. + anthropicSecretKey: "" + claudeCodeOAuthTokenSecretKey: "" + githubAppIdSecretKey: GITHUB_APP_ID + githubAppPrivateKeySecretKey: GITHUB_APP_PRIVATE_KEY + githubAppInstallationIdSecretKey: GITHUB_APP_INSTALLATION_ID + githubAppSlug: e2e-bot + model: "" + identityLink: + enabled: true + # Mirrors production: only the credentials a run actually needs. + # + # `github` is absent by design (ADR 0030). The canonical subject and the + # actor login are now resolved by a read-only lookup independent of this + # list, so declaring github here would only re-provision a credential this + # agent never uses -- the conflation that caused the production 401. Order + # is no longer load-bearing either: the pre-flight assesses every provider + # rather than short-circuiting on the first gap. + providers: + - claude + - claude-remote + +integrationRoutes: + githubIssueLabeledTriage: + enabled: true + # stub-agent, not the demo default and not claude-code-swe-agent: it carries + # the same Claude identityProviders (so the credential gate is exercised + # exactly as production exercises it) AND terminates hermetically (so the + # chain past the launch -- reply, relay, issue comment -- is assertable). + agentRef: stub-agent diff --git a/charts/community-components/values-production.yaml b/charts/community-components/values-production.yaml index efdbabc..47eb3e8 100644 --- a/charts/community-components/values-production.yaml +++ b/charts/community-components/values-production.yaml @@ -111,37 +111,26 @@ opencodeSweAgent: # secret` invocation) -- CI builds/pushes the image, but Helm can't source # secret VALUES on its own, so that Secret is still a manual prerequisite. # -# identityLink.providers leads with "github", and the ORDER is load-bearing. +# identityLink.providers lists only the credentials a RUN actually needs. # -# The gate resolves every declared provider in order (it used to stop after -# index 0, which is why this list was once "claude" only -- that limitation -# is gone). "github" is first because its link is what supplies the canonical -# credential subject: a chat caller's Claude credentials are keyed by -# `github:`, and the login is read off this very link (see -# agent-orchestrator's resolveCredentialSubject). Resolve it after the Claude -# providers and the first turn would key them under the raw `openwebui:` -# subject instead, which is exactly the cross-flow split we're fixing. +# `github` is deliberately NOT here. It was added so the gate could derive the +# canonical `github:` credential subject (ADR 0029), but declaring a +# provider does two unrelated things: it obtains an identity mapping AND it +# provisions a credential. Only the first was wanted. The second populated +# GITHUB_TOKEN, which switched on this agent's delegated-write path and +# produced a production 401 from a `/user` call the agent had no reason to +# make. # -# Requiring "github" here means a chat user links GitHub once before this -# agent will run. That is a deliberate trade (see the PR that introduced the -# canonical subject): it guarantees every caller has a canonical key, so a -# Claude authorization done during ai-triage is reused in chat and vice -# versa, instead of each entry point prompting separately. +# ADR 0030 separates those concerns: the orchestrator now resolves the caller's +# login with a read-only lookup of their existing github link (or the webhook's +# verified sender) regardless of what this list says, and injects it as +# AGENT_ACTOR_LOGIN. So cross-flow credential sharing still works, and this +# agent no longer needs a per-user GitHub credential it never used -- git +# operations continue to run on the App installation token, as they always did. # -# Git/gh operations do NOT depend on that link: resolveGithubToken still -# prefers the GitHub App installation token (githubAppIdSecretKey/etc. below) -# over any per-user token whenever all three App fields are configured, so -# commits/PRs keep attributing to the k5s-bot App identity exactly as before. -# The per-user GITHUB_TOKEN injected by the link is the dual-token pattern -# opencode-swe-agent already uses. -# -# No static anthropicSecretKey/claudeCodeOAuthTokenSecretKey needed here: -# with "claude" in identityLink.providers, delegateToAgent (agent- -# orchestrator) refuses to launch a Job at all until the caller has a linked -# Claude token, so a per-run CLAUDE_CODE_OAUTH_TOKEN is always present by the -# time this agent actually runs -- a static fallback key would just be dead -# config nobody ever reads (claude-code-swe-secrets therefore only needs the -# three GITHUB_APP_* keys below). +# Removing it also drops the extra trigger a user needed (the first webhook +# turn used to spend itself posting a GitHub link prompt) and decouples Claude +# authorization from GitHub OAuth availability. claudeCodeSweAgent: enabled: true image: registry.kurpuis.com:5000/claude-code-swe-agent:latest @@ -158,9 +147,6 @@ claudeCodeSweAgent: identityLink: enabled: true providers: - # MUST stay first -- supplies the `github:` canonical subject the - # two Claude providers below are keyed under (see the block comment). - - github - claude # Required for Remote Control (see remoteControl below) -- without # this, agent-orchestrator never resolves/injects the per-run @@ -170,10 +156,10 @@ claudeCodeSweAgent: # Claude Code Remote Control (claude.ai/code session takeover) for # ai-triage runs, replacing the old session-page link in the triage # comment. Also requires: (1) the cluster's Agent CRD to actually have the - # initContainers field -- `helm upgrade` never updates CRDs on an existing - # release, so this must be applied manually once: - # kubectl apply -f controllers/core-controller/config/crd/bases/core.controller-agent.dev_agents.yaml - # (2) integration-gateway's claudeAuth.enabled=true (see + # initContainers field -- once a manual `kubectl apply` of that CRD, now done + # by the deploy job's "Apply CRDs" step on every push + # (.github/workflows/release.yml), since `helm upgrade` never updates CRDs on + # an existing release. (2) integration-gateway's claudeAuth.enabled=true (see # charts/agent-controller/values-production.yaml) so its `mode=login` PTY # flow is actually wired up, not just the setup-token one. # @@ -217,11 +203,11 @@ integrationRoutes: # already uses (agent-controller values' claudeAuth.enabled). # # Requires claudeCodeSweAgent.enabled=true (set above) and the - # IntegrationRoute CRD to exist on the cluster -- `helm upgrade` does NOT + # IntegrationRoute CRD to exist on the cluster. `helm upgrade` does NOT # install/update CRDs for an existing release - # (charts/agent-controller/charts/core-controller/crds/README.md), so this - # was applied manually once: - # kubectl apply -f controllers/core-controller/config/crd/bases/core.controller-agent.dev_integrationroutes.yaml + # (charts/agent-controller/charts/core-controller/crds/README.md), which is + # why this once needed a manual `kubectl apply`; the deploy job now applies + # every CRD on every push (.github/workflows/release.yml, "Apply CRDs"). githubIssueLabeledTriage: enabled: true agentRef: claude-code-swe-agent @@ -239,10 +225,9 @@ integrationRoutes: # same per-user gate the ai-triage path already uses. # # Requires claudeCodeSweAgent.enabled=true (set above) and the - # IntegrationRoute CRD to exist on the cluster -- `helm upgrade` does NOT - # install/update CRDs for an existing release, so if it isn't already - # present it must be applied manually once: - # kubectl apply -f controllers/core-controller/config/crd/bases/core.controller-agent.dev_integrationroutes.yaml + # IntegrationRoute CRD to exist on the cluster -- applied by the deploy job's + # "Apply CRDs" step, since `helm upgrade` does NOT install/update CRDs for an + # existing release. githubPrLabeledReview: enabled: true agentRef: claude-code-swe-agent @@ -261,10 +246,10 @@ integrationRoutes: # to launch -- the same per-user gate the whole ai-triage path already uses. # # Requires the IntegrationRoute CRD on the cluster to carry match.labelName - # (added alongside this route) -- `helm upgrade` does NOT update CRDs for an - # existing release, so the CRD must be re-applied once before this route - # will validate: - # kubectl apply -f controllers/core-controller/config/crd/bases/core.controller-agent.dev_integrationroutes.yaml + # (added alongside this route). This is exactly the case `helm upgrade` cannot + # handle -- it never updates an existing release's CRDs, so the route would + # apply cleanly while the API server pruned match.labelName out of it. The + # deploy job's "Apply CRDs" step is what makes the field actually exist. githubPrLabeledTriage: enabled: true agentRef: claude-code-swe-agent diff --git a/charts/community-components/values.yaml b/charts/community-components/values.yaml index 8c23354..544dab0 100644 --- a/charts/community-components/values.yaml +++ b/charts/community-components/values.yaml @@ -297,6 +297,32 @@ skills: webSearch: enabled: false +# stub-agent: an END-TO-END TEST DOUBLE, not a component of the catalog. +# +# It speaks the real NATS agent protocol and returns a canned reply without +# calling a model, so the e2e suite can assert the whole webhook -> triage -> +# AgentRun -> reply -> issue-comment chain in a cluster that holds no Anthropic +# credential. Enabled only by values-e2e.yaml; see apps/stub-agent. +# +# `enabled: true` alone is not enough -- the template also requires +# acknowledgeNotForProduction, so that pointing a production route at an agent +# which silently answers everything with a stub cannot happen by typo. +stubAgent: + enabled: false + acknowledgeNotForProduction: false + image: stub-agent:latest + serviceAccountName: stub-agent + serviceAccount: + create: true + annotations: {} + automount: true + # Which identityProviders the stub declares. Set these to match whichever + # agent it stands in for, or the identity gate takes a different path than + # production and the suite stops testing it (see agent-stub.yaml). + identityLink: + enabled: false + providers: [] + integrationRoutes: # github-issue-labeled-triage: an IntegrationRoute CR (docs/adr/0024) # dispatching a GitHub `issues`/`labeled` webhook event straight to diff --git a/controllers/core-controller/.dockerignore b/controllers/core-controller/.dockerignore index 9af8280..caafac4 100644 --- a/controllers/core-controller/.dockerignore +++ b/controllers/core-controller/.dockerignore @@ -1,11 +1,47 @@ # More info: https://docs.docker.com/engine/reference/builder/#dockerignore-file -# Ignore everything by default and re-include only needed files -** +# +# A DENY-LIST, deliberately -- NOT the kubebuilder scaffold's `**` + +# `!**/*.go` allow-list idiom it replaces, which silently shipped an +# eight-day-old controller to the e2e cluster. +# +# `**` excludes the context root itself, so `COPY . .` in the Dockerfile copies +# a path this file excludes (Docker warns: "CopyIgnoredFile"). BuildKit +# resolves the negations anyway and the image builds correctly, but Skaffold's +# own dependency resolver -- which it uses to decide whether an artifact needs +# rebuilding at all -- does not: it resolved THREE files for this artifact +# where the sibling Node artifacts resolve dozens. Every .go file was invisible +# to it, so its artifact cache never invalidated and it re-tagged one July 17 +# image for every commit that followed. +# +# The visible consequence was a rendered AgentRun Job carrying the Agent +# template's static secretEnv but not the per-run secretEnv the orchestrator +# had injected -- because the running binary predated per-run secretEnv support +# (327b403) entirely. It read as a controller defect; it was a stale image. +# +# Verify after editing this file: +# skaffold diagnose -p e2e | grep -A3 'Docker artifact: core-controller' +# The dependency count must reflect the real source tree. A count in the single +# digits means the build has gone stale again. -# Re-include Go source files (but not *_test.go) -!**/*.go +# Test sources and the envtest suite: not inputs to the manager binary. **/*_test.go +/test -# Re-include Go module files -!go.mod -!go.sum +# Build/dev tooling and generated output, none of it an input to `go build`. +/bin +/dist +/hack +/config +/.devcontainer +/.github +/.golangci.yml +/.custom-gcl.yml +/Makefile +/PROJECT +/Dockerfile +*.md + +# Local artifacts that must never enter an image. +*.kubeconfig +*.test +*.out diff --git a/controllers/core-controller/internal/controller/agentrun_controller.go b/controllers/core-controller/internal/controller/agentrun_controller.go index e9e1f6a..8bba200 100644 --- a/controllers/core-controller/internal/controller/agentrun_controller.go +++ b/controllers/core-controller/internal/controller/agentrun_controller.go @@ -19,6 +19,7 @@ package controller import ( "context" "fmt" + "time" batchv1 "k8s.io/api/batch/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -42,6 +43,11 @@ type AgentRunReconciler struct { // Job so the @controller-agent/agent-runtime SDK can connect on startup. // Set from the controller manager's own env at startup (cmd/main.go). NatsConfig AgentNatsConfig + // Retention is how long a terminal AgentRun (and the Secret it owns) is + // kept before reclamation. Zero means DefaultAgentRunRetention -- there is + // deliberately no "keep forever" setting, since that is the behaviour that + // let credential-bearing Secrets accumulate indefinitely. + Retention time.Duration } // AgentNatsConfig is the NATS connection config injected into every agent Job. @@ -78,9 +84,37 @@ func (r *AgentRunReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c return ctrl.Result{}, err } - // Already terminal — nothing left to reconcile for lifecycle purposes. + // Already terminal — reclaim it once its retention window has elapsed. + // + // Nothing else ever deleted an AgentRun. The Job it creates carries a TTL + // and disappears, but the CR and the per-run `-identity` Secret it + // owns persisted forever: a live cluster was found holding runs 8 days old, + // each still carrying encrypted credential material in etcd long after the + // run that needed it had finished. That is both unbounded object growth and + // a credential-lifetime problem, so retention is bounded here rather than + // left to an operator remembering to prune. + // + // Deleting the CR cascades to the Secret through its ownerReference, so + // this single delete reclaims both. if run.Status.Phase == toolv1alpha1.ToolRunPhaseSucceeded || run.Status.Phase == toolv1alpha1.ToolRunPhaseFailed { - return ctrl.Result{}, nil + retention := r.retention() + // A run whose completion time was never recorded (older CRs predate the + // field) falls back to its creation time -- reclaiming late is fine, + // never reclaiming is not. + finishedAt := run.Status.CompletionTime + if finishedAt == nil { + finishedAt = &run.CreationTimestamp + } + age := time.Since(finishedAt.Time) + if age >= retention { + log.Info("reclaiming terminal AgentRun", "name", run.Name, "phase", run.Status.Phase, "age", age.String()) + if err := r.Delete(ctx, &run); err != nil && !apierrors.IsNotFound(err) { + return ctrl.Result{}, err + } + return ctrl.Result{}, nil + } + // Come back exactly when it becomes eligible rather than polling. + return ctrl.Result{RequeueAfter: retention - age}, nil } if run.Status.JobName == "" { @@ -90,6 +124,21 @@ func (r *AgentRunReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c return r.syncJobStatus(ctx, &run, log) } +// DefaultAgentRunRetention is how long a terminal AgentRun (and the Secret it +// owns) is kept before being reclaimed. Long enough to inspect a failure by +// hand, short enough that credential material does not linger. +const DefaultAgentRunRetention = 1 * time.Hour + +// retention returns the configured retention window, falling back to +// DefaultAgentRunRetention when unset so an operator who never configures it +// still gets bounded growth. +func (r *AgentRunReconciler) retention() time.Duration { + if r.Retention > 0 { + return r.Retention + } + return DefaultAgentRunRetention +} + func (r *AgentRunReconciler) createJob(ctx context.Context, run *toolv1alpha1.AgentRun) (ctrl.Result, error) { var agent toolv1alpha1.Agent agentKey := types.NamespacedName{Namespace: run.Namespace, Name: run.Spec.AgentRef} diff --git a/docs/adr/0030-authorization-preflight-outside-the-llm.md b/docs/adr/0030-authorization-preflight-outside-the-llm.md new file mode 100644 index 0000000..f885aed --- /dev/null +++ b/docs/adr/0030-authorization-preflight-outside-the-llm.md @@ -0,0 +1,239 @@ +# 0030. Authorization is a deterministic pre-flight the orchestrator owns, and the LLM can neither decide it nor see credentials + +Date: 2026-07-25 + +## Status + +Proposed + +Amends [0029](0029-canonical-github-credential-subject.md) (canonical credential +subject) and [0022](0022-per-user-github-device-flow-identity.md) / +[0027](0027-per-user-claude-oauth-setup-token-delegation.md) (per-user GitHub +and Claude delegation). Nothing here is retracted; 0029's canonical subject +remains the interim keying until the principal model below lands. + +## Context + +Authorization currently lives in three places, with overlap: + +| Layer | Decides | +| --- | --- | +| integration-gateway | May this sender trigger anything? (team membership, collaborator permission) | +| agent-orchestrator's identity gate | Which credentials must exist before an Agent launches? | +| the SWE agent itself, at runtime | Who is this token? May they write this repo? Who gets attribution? | + +The third row is the defect. `delegateToAgent` already refuses to launch until +every provider a CRD declares in `identityProviders` is resolved and injected +as `secretEnv` — so by the time a run starts, the orchestrator has *already* +established the caller's authorization. The agent then re-derives it anyway: +`identityDelegation.ts` calls `fetchGithubUser`, checks +`fetchCollaboratorPermission`, and mints an installation token. + +That duplication is not theoretical. A production run failed with +`Failed to look up GitHub user: 401 Bad credentials` from the agent's own +`/user` call — a code path that only became reachable when +`claude-code-swe-agent` gained the `github` provider, and which the +orchestrator had no reason to invoke at all. Collaborator-permission checking +also exists in two separate implementations (gateway and agent). + +Three further problems were observed standing the system up end-to-end +(docs/adr/0029's rollout, verified on minikube): + +1. **The gate is sequential and short-circuits.** It bails on the *first* + unresolved provider, so an Agent declaring N unlinked providers costs the + user N separate triggers, each discovering the next gap. +2. **Ordering is load-bearing and fragile.** Because resolution short-circuits, + a failure in an unrelated provider ends the turn before later providers are + reached — observed as `start threw for provider github; ending turn`, which + blocked Claude authorization entirely on a GitHub OAuth failure. +3. **Declaring a provider conflates two unrelated things**: obtaining an + identity *mapping* and provisioning a *credential*. Requiring `github` to + get the former is what activated the latter, and hence the 401. + +Underneath all of it: integration-gateway resolves the sender's identity for +its own gate, then authenticates to `/invoke` as itself. The orchestrator sees +one shared service subject and reconstructs the human from a `senderLogin` +field riding in a routing descriptor. + +## Decision + +### 1. One authorization owner, deterministic, outside the LLM — DONE + +The *property* held from the start and is what matters: authorization runs as +graph control flow in `delegateToAgent`'s pre-flight, never as a capability a +planner selects, and §3's test pins the credential boundary. The extraction into +a named class is now done too (`agent/authorization-service.ts`), as a pure +refactor — the full pre-existing orchestrator suite passes unchanged, with new +unit tests added for the now-directly-reachable verdicts. + +Two things came out of doing it that the "it's only cosmetic" framing missed. +The verdict is now a **total discriminated union** +(`authorized` | `link-required` | `misconfigured`), so a fourth outcome breaks +compilation at the branch rather than falling through to "launch anyway" — the +failure direction that actually matters. And the agent-backed-tool path, which +had its own hand-copied provider loop with the same keying rules, now calls the +same owner via a deliberately separate read-only entry point +(`resolveLinkedCredentials`) — it must not start a link flow, because a paused +*tool* call has no resume slot. Two copies of credential keying was exactly the +shape of the #144 bug; §1 is what removes the second copy. + +A single `AuthorizationService` in agent-orchestrator owns every authorization +decision: which providers an Agent requires, whether they are satisfied, what +identity they resolve to, and what permission that identity holds. + +It is **plain control flow** — a graph pre-flight node, not a capability the +planner selects. No model call participates in an authorization decision. This +is a security boundary, not a style preference: an LLM that can choose whether +authorization succeeded is an LLM that can be argued into saying yes. + +### 2. The LLM's only authorization-adjacent capability is asking a human to link + +**Not built — and on implementation review, it should not be.** The +deterministic pre-flight already surfaces every link prompt a caller needs, +including the batched multi-provider case (§4). Adding a model-callable tool +would introduce an LLM-reachable authorization surface to solve a problem that +no longer exists, which is the opposite of this ADR's intent: the smallest +attack surface is the one that isn't there. The constraints below are retained +as the specification any future such tool must satisfy, should a genuine need +appear. + +Were it exposed, exactly one narrow tool: + +``` +request_account_link(provider: "github" | "claude" | "claude-remote") -> { promptText, url } +``` + +Constraints, all enforced at the call boundary rather than by prompt: + +- **It takes no subject.** The subject comes from the verified identity on the + turn's state. A model-supplied subject would let a caller request — and then + be handed — a link flow keyed to someone else. +- **It returns no credential**, only a URL and human-readable text. +- **It cannot report success.** Whether a link completed is read from the store + by the deterministic pre-flight on the next turn, never from model output. +- **It is not required for the normal path.** The pre-flight already surfaces + link prompts; this tool exists only for a model mid-conversation that needs + to re-offer one. + +### 3. Credentials never enter model context — IMPLEMENTED (test) + +A resolved credential is an opaque handle inside the graph: + +```ts +/** Redeemable ONLY by the AgentRun launcher. No accessor returns the secret. */ +type CredentialHandle = { readonly provider: string; readonly ref: symbol }; +``` + +The plaintext value travels gateway → launcher → Kubernetes Secret → +`AgentRunSpec.SecretEnv`, and is never assigned to an `AgentStateAnnotation` +field, never interpolated into a prompt, and never logged. Today's +`identitySecretEnv` is already a node-local variable rather than graph state; +this makes that property explicit and type-enforced instead of incidental. + +Enforced by: +- **Type** — no accessor on `CredentialHandle` returns the value; only the + launcher can redeem it. +- **Test** — an assertion that no credential material appears in any outbound + model request payload, run against a turn that resolves every provider. +- **Log discipline** — the existing debug lines print subjects and env var + *names* only, never values (the e2e helpers follow the same rule). + +### 4. Batch pre-flight, not first-miss — IMPLEMENTED + +The pre-flight resolves **every** declared provider, collects **all** +unsatisfied ones, and presents them together in a single turn. Launch happens +when the whole set is satisfied. + +This removes the N-triggers cost and makes ordering irrelevant: nothing +short-circuits, so no provider's failure can prevent a later one from being +evaluated. A provider that fails to *start* is reported alongside the others +rather than ending the turn. + +### 5. The agent receives a sealed authorization context — IMPLEMENTED + +The launcher injects resolved facts, so the agent performs no identity work: + +``` +AGENT_ACTOR_LOGIN resolved GitHub login +AGENT_ACTOR_ID numeric id, for the co-author trailer +AGENT_ACTOR_PERMISSION verdict already checked by the orchestrator +``` + +The agent stops calling `/user` and stops re-checking collaborator permission. +The 401 is fixed **by construction** — the call that produced it no longer +exists — and the duplicate permission implementation is deleted. + +*Amended during implementation — this trade-off turned out to be avoidable.* +The login is read off the caller's stored `github` link record, which already +contains it, rather than from a `/user` call. So the orchestrator needs **no** +GitHub App credentials and makes no additional API request. The numeric id is +the only casualty: it is not stored on the link, and fetching it would +reintroduce the very round trip being removed, so the co-author trailer falls +back to its `login@users.noreply.github.com` form. GitHub still attributes +correctly; the attribution just isn't pinned across an account rename. + +### 6. Identity is forwarded, not reconstructed — IMPLEMENTED + +integration-gateway sends its service token for *authentication* and a +**signed user assertion** for *identity* — the same pattern +`OpenWebUiForwardedUserResolver` already uses for Open WebUI's per-request JWT, +and for the same stated reason: resolving identity from a shared bearer token +collapses every user into one subject. + +**As built**, the assertion is a compact HMAC-SHA256 `payload.signature` pair +(`x-gateway-user-assertion`) rather than a JWT: both ends live in this repo, +the only claims needed are a login and an expiry, and hand-rolling it with +`node:crypto` avoids adding a JWT library to the gateway purely to agree with +the orchestrator's — the same no-new-dependency precedent as `githubApp.ts`. + +With the shared secret configured, `/invoke` accepts a sender login ONLY from +a verified assertion and ignores the request-body field entirely. Without it, +the body field is still trusted and BOTH processes warn at startup — chosen so +upgrading does not silently break an existing deployment, while never leaving +the weaker mode unannounced. + +Rather than rewriting `identity.subject` itself (which would move session +keys, RBAC scoping and credential keys simultaneously), the verified login +feeds **principal** resolution: `Identity` gains a `principal` field, resolved +once in `resolveIdentity` and read everywhere downstream. Entry-point subjects +remain aliases; durable per-user state keys on the principal. This keeps +sessions and RBAC on the subject they have always used while credentials +converge, which is the whole point — and it retires +`resolveCredentialSubject`, whose per-provider re-derivation was the shape of +the PR #144 bug. + +Entry-point identities then become **aliases of a principal** — a stable +internal user id — with credentials keyed by principal. `github:` is +today's stand-in for exactly this, chosen because GitHub was the one +identifier both flows could reach. Formalizing it means cross-flow sharing +works for any future entry point with no new keying code, `resolveCredentialSubject` +is deleted, and account linking becomes a user-level action rather than a side +effect of an Agent's provider list. + +## Consequences + +The 401 disappears without being debugged, because the agent no longer performs +the lookup. Ordering stops mattering. Users authorize once, for everything an +Agent needs, in one round trip. Authorization has one implementation and one +owner. + +The LLM cannot approve, deny, or observe authorization. It can ask a human to +link an account and nothing else — and because the tool takes no subject and +returns no secret, prompt injection cannot escalate through it. + +Costs, stated plainly: + +- The orchestrator holds the GitHub App credentials (see §5). +- `identityProviders` semantics change: declaring a provider means "this run + needs this credential," and identity mapping no longer rides on it. Charts + that added a provider purely to obtain a mapping should drop it. +- Principals require a migration: existing credentials are keyed by + `identity.subject` or `github:` and are not rewritten. The first + authorization after the change writes the principal-keyed record — at most + one extra prompt per user, once. +- Steps 4 and 5 are independently shippable and remove three observed defects + on their own; 6 is the deeper change and can follow. + +An e2e suite covering these paths exists (`e2e/`), because none of the defects +above were visible to unit tests — every one of them required the assembled +system to be running. diff --git a/docs/adr/README.md b/docs/adr/README.md index 4bb2619..887c536 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -35,5 +35,6 @@ See [../orchestrator.md](../orchestrator.md) for how these fit together. | [0027](0027-per-user-claude-oauth-setup-token-delegation.md) | `claude-code-swe-agent` (a new Claude-Code-CLI sibling to `opencode-swe-agent`) can act as the calling user's own Claude subscription seat, linked via a PTY-driven `claude setup-token` flow brokered by `integration-gateway`, mirroring ADR 0022's per-user GitHub identity pattern for the Anthropic credential instead | | [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 | Status values: `proposed` | `accepted` | `superseded by NNNN`. diff --git a/e2e/README.md b/e2e/README.md new file mode 100644 index 0000000..9224494 --- /dev/null +++ b/e2e/README.md @@ -0,0 +1,84 @@ +# End-to-end tests + +System tests that exercise the **real deployed stack on minikube** — +integration-gateway, agent-orchestrator, Redis, NATS, the CRD controllers and +a real AgentRun — over real HTTP and real Kubernetes objects. Nothing in here +mocks a component this repo owns. + +These exist because the failures that actually reach production are +cross-component wiring failures, and unit tests are blind to every one of +them: which Redis key a credential lands under, which order an Agent's +`identityProviders` resolve in, which env vars survive onto the launched Pod, +whether the gateway's `waitAndResume` blocks on the same subject the graph +stored. Every one of those has shipped broken at least once (PRs #144/#145, +#152, #153, #156) and every one was invisible to `vitest` in `apps/*`. + +## 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 +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 +deliberately no environment variable to override it. + +## What's stubbed, and why only this + +Third-party services are stubbed; nothing else is. + +| Dependency | Treatment | Why | +| --- | --- | --- | +| GitHub REST API | `fake-github` in-cluster service | Tests must assert *what we posted* (comments, labels) without writing to a real repo, and must serve deterministic `/user` + permission responses. Pointed at via the existing `githubApiUrl` value — no production code changes. | +| GitHub webhooks | Signed locally by `support/webhook.ts` | The signature path is real (same HMAC the gateway verifies); only the sender is us. | +| Anthropic / Claude Code CLI | `stub-agent` image (`apps/stub-agent`) | A real agent run needs a real paid credential and makes the test slow and nondeterministic — and in a cluster holding no credential it never reaches a terminal phase at all, which is why the happy-path spec was once skipped. The stub speaks the **real** NATS agent protocol and declares the **same** `identityProviders` as the agent it stands in for, so everything between the webhook and the reply — routing, RBAC, the identity gate (including its refusal to launch), AgentRun creation, secret injection, the callback — is exercised for real. | +| OpenAI (planner/selector) | Real, against the dev key | Routing decisions are part of what we're testing. Tests assert on deterministic `IntegrationRoute` dispatch rather than on RAG retrieval, so model nondeterminism doesn't make them flaky. | + +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. + +## Running + +```bash +kubectl config use-context minikube +./scripts/dev-up.sh # bring the stack up (skaffold run) +npm run e2e -w e2e # or: npx vitest run --config e2e/vitest.config.ts +``` + +Tests are **serial** (`fileParallelism: false`, `maxConcurrency: 1`): they +share one cluster, and several assert on global state (Redis keys, AgentRun +lists) that concurrent runs would race on. + +## Layout + +``` +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 +specs/ + happy-path.e2e.ts webhook -> triage -> AgentRun -> comment posted + identity-keying.e2e.ts which subject each entry point keys credentials under +manifests/ + fake-github.yaml in-cluster GitHub API stub (Deployment + Service + script) +``` + +`stub-agent` is NOT here: it is a real image (`apps/stub-agent`) built by the +skaffold `e2e` profile, and its Agent CR is a chart template +(`charts/community-components/templates/agent-stub.yaml`) enabled by +`values-e2e.yaml`. Keeping it in the chart rather than in a hand-applied +manifest is the point — the CR the suite exercises is produced by the same +templating production uses, so a change that breaks Agent rendering breaks the +e2e run too. + +### Editing `fake-github.yaml` + +Its script is a mounted ConfigMap, and a running `node` process does not re-read +a mounted file. `ensureFakeGithub()` therefore substitutes a hash of the whole +manifest into the pod template's `e2e.controller-agent.dev/config-checksum` +annotation, so an edited script changes the pod spec and the Deployment rolls by +itself. Do not replace that with `kubectl rollout restart` after the apply: the +restart can beat the ConfigMap write it was meant to pick up, which is how a +readiness-probe fix to that file once appeared to have no effect at all. diff --git a/e2e/manifests/fake-github.yaml b/e2e/manifests/fake-github.yaml new file mode 100644 index 0000000..433b999 --- /dev/null +++ b/e2e/manifests/fake-github.yaml @@ -0,0 +1,181 @@ +# In-cluster stand-in for GitHub's REST API. +# +# Deployed only into the minikube e2e environment and pointed at via the +# existing `integrationGateway.githubApiUrl` / agent `githubApiUrl` values -- +# no production code path knows this exists. +# +# It does two jobs: +# 1. Serves deterministic responses for the handful of endpoints the system +# calls (`/user`, collaborator permission, issue comments, labels). +# 2. RECORDS every request, exposing them on `GET /_e2e/requests`, so a test +# can assert "a comment containing X was posted to issue N" instead of +# inferring it from logs. +# +# Written as a ConfigMap-mounted script on the stock `node:22-alpine` image +# rather than a built image: it has no dependencies, and keeping it out of the +# image build means `skaffold dev` doesn't rebuild the world when a test needs +# one more stubbed endpoint. +apiVersion: v1 +kind: ConfigMap +metadata: + name: fake-github + labels: { app: fake-github, e2e: "true" } +data: + server.js: | + const http = require("node:http"); + + // Every request, in order. Unbounded on purpose -- the process is + // recreated per e2e run and a bounded ring buffer would silently drop the + // very request a failing assertion is looking for. + const requests = []; + + function json(res, status, body) { + const payload = JSON.stringify(body); + res.writeHead(status, { "content-type": "application/json" }).end(payload); + } + + http.createServer((req, res) => { + let body = ""; + req.on("data", (c) => (body += c)); + req.on("end", () => { + const url = new URL(req.url, "http://fake-github"); + + // Introspection endpoints are namespaced under /_e2e/ so they can + // never collide with a real GitHub route the system might call. + // Health route for the port-forward readiness probe. Answered BEFORE + // recording, so the harness's own liveness check never shows up as an + // "unstubbed endpoint" the system supposedly called. + if (url.pathname === "/") return json(res, 200, { ok: true }); + if (url.pathname === "/_e2e/requests") return json(res, 200, requests); + if (url.pathname === "/_e2e/reset") { requests.length = 0; return json(res, 200, { ok: true }); } + + // Recorded BEFORE dispatch so the entry exists no matter which branch + // handles it; `status` is stamped on by `json()` below. Without the + // stamp the "never called an unstubbed endpoint" assertion silently + // matches nothing and passes vacuously. + const entry = { method: req.method, path: url.pathname, body: body || null, at: new Date().toISOString() }; + requests.push(entry); + const respond = (status, payload) => { entry.status = status; return json(res, status, payload); }; + + // ── OAuth / device flow (the WEB host, githubBaseUrl) ────────── + // Stubbed so an identity-link start succeeds hermetically. The suite + // never drives a browser through the approval screen; it asserts on + // the SUBJECT a started flow is keyed under, which is decided before + // any of this is reached. + if (url.pathname === "/login/device/code" && req.method === "POST") { + return respond(200, { + device_code: "e2e-device-code", + user_code: "E2E-CODE", + verification_uri: "http://fake-github/login/device", + expires_in: 900, + interval: 5, + }); + } + if (url.pathname === "/login/oauth/access_token" && req.method === "POST") { + // Completes immediately, deliberately. + // + // An earlier version returned `authorization_pending` forever to + // mirror a real un-approved flow. That made the identity-keying + // spec structurally impossible: a webhook turn is fire-and-forget + // (no progressListener), so the gate PARKS on the first provider it + // has to link and ends the turn without reaching the later ones. + // With `github` declared first (ADR 0029), a github link that never + // completes means `claude`/`claude-remote` are never resolved and + // no credential subject is ever keyed -- the exact thing under test. + // + // The pending path still gets exercised: the first trigger parks + // before this endpoint is ever polled. It is the SECOND trigger, + // via checkPendingIdentityLink, that lands the token here. + return respond(200, { + access_token: "e2e-user-access-token", + token_type: "bearer", + scope: "repo", + expires_in: 28800, + refresh_token: "e2e-user-refresh-token", + refresh_token_expires_in: 15897600, + }); + } + + // GET /user -- identity behind a user token. The dual-token / + // delegated-write path calls this to build a co-author trailer. + if (url.pathname === "/user") return respond(200, { login: "e2e-user", id: 4242 }); + + // Collaborator permission -- "write" so the delegated-write + // authorization check passes and the happy path proceeds. + if (/^\/repos\/[^/]+\/[^/]+\/collaborators\/[^/]+\/permission$/.test(url.pathname)) { + return respond(200, { permission: "write" }); + } + + // Posting an issue comment -- what the triage flow ultimately does. + if (/^\/repos\/[^/]+\/[^/]+\/issues\/\d+\/comments$/.test(url.pathname) && req.method === "POST") { + return respond(201, { id: requests.length, body: JSON.parse(body || "{}").body ?? "" }); + } + + // Removing the trigger label once a run finishes. + if (/^\/repos\/[^/]+\/[^/]+\/issues\/\d+\/labels\//.test(url.pathname) && req.method === "DELETE") { + return respond(200, []); + } + + // Repo metadata -- `created_at` drives the "did the bot just create + // this repo" branch in the post-flight authorization check. + if (/^\/repos\/[^/]+\/[^/]+$/.test(url.pathname)) { + return respond(200, { name: "e2e-repo", created_at: "2020-01-01T00:00:00Z", default_branch: "main" }); + } + + // Anything unstubbed is a 501, never a 200: a silent empty success + // would let a test pass while the system called something we never + // meant to support, which is exactly the blindness e2e is for. + respond(501, { message: `fake-github: unstubbed ${req.method} ${url.pathname}` }); + }); + }).listen(8080, () => console.log("fake-github listening on 8080")); +--- +apiVersion: apps/v1 +kind: Deployment +metadata: + name: fake-github + labels: { app: fake-github, e2e: "true" } +spec: + replicas: 1 + selector: + matchLabels: { app: fake-github } + template: + metadata: + labels: { app: fake-github, e2e: "true" } + annotations: + # Substituted at apply time by ensureFakeGithub() with a hash of THIS + # FILE. The literal placeholder is never what reaches the cluster. + # + # A mounted ConfigMap's contents change under a running pod, but the + # process that read them at startup does not re-read them -- so editing + # the script above and re-applying left the OLD server running while + # `kubectl get deploy` reported Ready, and a fix appeared not to take + # effect. Threading the hash through the pod template makes the edit + # change the template, so the Deployment rolls on its own: no + # `rollout restart` racing the apply, and no restart at all when the + # script is unchanged (which is every call but the first). + # + # Same technique as Helm's `checksum/config` annotation convention. + e2e.controller-agent.dev/config-checksum: "REPLACED_AT_APPLY" + spec: + containers: + - name: fake-github + image: node:22-alpine + command: ["node", "/srv/server.js"] + ports: [{ containerPort: 8080 }] + volumeMounts: [{ name: script, mountPath: /srv }] + readinessProbe: + httpGet: { path: /_e2e/requests, port: 8080 } + initialDelaySeconds: 2 + periodSeconds: 2 + volumes: + - name: script + configMap: { name: fake-github } +--- +apiVersion: v1 +kind: Service +metadata: + name: fake-github + labels: { app: fake-github, e2e: "true" } +spec: + selector: { app: fake-github } + ports: [{ port: 80, targetPort: 8080 }] diff --git a/e2e/package.json b/e2e/package.json new file mode 100644 index 0000000..6c16871 --- /dev/null +++ b/e2e/package.json @@ -0,0 +1,13 @@ +{ + "name": "@controller-agent/e2e", + "private": true, + "type": "module", + "scripts": { + "e2e": "vitest run --config vitest.config.ts", + "typecheck": "tsc --noEmit -p tsconfig.json" + }, + "devDependencies": { + "typescript": "^5.6.0", + "vitest": "^4.1.10" + } +} diff --git a/e2e/scripts/bootstrap-secrets.sh b/e2e/scripts/bootstrap-secrets.sh new file mode 100755 index 0000000..ea9df1c --- /dev/null +++ b/e2e/scripts/bootstrap-secrets.sh @@ -0,0 +1,121 @@ +#!/usr/bin/env bash +# bootstrap-secrets.sh — create the throwaway secrets the e2e stack needs on +# minikube, and report (without creating) the one that needs a real human key. +# +# Usage: ./e2e/scripts/bootstrap-secrets.sh +# +# Every value generated here is disposable and scoped to the local minikube +# profile. Re-running regenerates them, which is fine: the stack reads them at +# pod start, so a `helm upgrade`/rollout picks up new values. +# +# The deliberate exception is OPENAI_API_KEY. The orchestrator's planner makes +# real model calls, so no generated value works — and copying it out of a live +# cluster to get one would be exfiltrating a production credential into a dev +# environment. This script refuses to do that and tells you the command to run +# yourself instead. + +set -euo pipefail + +NS="controller-agent" +CTX="minikube" + +# Same guard as the test suite (e2e/support/guard.ts), for the same reason: +# this script CREATES cluster objects, and the default context on this machine +# is a live cluster. +CURRENT="$(kubectl config current-context 2>/dev/null || true)" +if [[ "$CURRENT" != "$CTX" ]]; then + echo "✗ kubectl context is '$CURRENT', not '$CTX'. Refusing to create secrets." >&2 + echo " Switch with: kubectl config use-context $CTX" >&2 + exit 1 +fi + +kubectl get namespace "$NS" >/dev/null 2>&1 || kubectl create namespace "$NS" + +# `create --dry-run | apply` so re-running updates in place instead of +# erroring on AlreadyExists. +upsert() { + local name="$1"; shift + kubectl -n "$NS" create secret generic "$name" "$@" --dry-run=client -o yaml | kubectl -n "$NS" apply -f - >/dev/null + echo " ✓ $name" +} + +rand() { openssl rand -hex 32; } +# Fixed, not random: this exact value must appear on BOTH sides of the +# orchestrator<->gateway identity-link channel (the gateway's +# GATEWAY_IDENTITY_LINK_TOKEN and the orchestrator's +# IDENTITY_LINK_GATEWAY_TOKEN). Generating it independently per secret would +# leave every /identity-link call 401ing, which surfaces as "no credential +# flow ever starts" rather than as an auth error. +IDENTITY_LINK_TOKEN="e2e-identity-link-token" +# Also fixed, and for the same reason: the gateway SIGNS the sender assertion +# with this and the orchestrator VERIFIES with it (docs/adr/0030 §6). Two +# independently-generated values would make every assertion fail verification, +# which presents as "the sender login vanished" rather than as a signature +# error. +SENDER_ASSERTION_SECRET="e2e-sender-assertion-secret" +# IDENTITY_LINK_ENCRYPTION_KEY must decode to EXACTLY 32 bytes for AES-256-GCM +# (decodeEncryptionKey throws at startup otherwise, which surfaces as a +# crashlooping gateway rather than an obvious config error). +key32() { openssl rand -base64 32; } + +echo "Creating throwaway e2e secrets in $NS (context: $CTX)..." + +# Prerequisites dev-up.sh hard-fails without. Only the orchestrator one is on +# the triage path the specs exercise; the other three are dummies purely to +# satisfy that check. +upsert recipe-publisher-secrets --from-literal=MEALIE_API_TOKEN="e2e-not-a-real-token" +upsert agent-controller-openwebui-google-oauth --from-literal=client-secret="e2e-not-a-real-secret" +upsert searxng-secrets --from-literal=secret-key="$(rand)" + +# The gateway's own secret, referenced by values-e2e.yaml via +# `secrets.existingSecret`. The name deliberately avoids the chart's own +# generated name (agent-controller-integration-gateway): Helm refuses to adopt +# an object it doesn't own, so colliding there fails the whole release. GATEWAY_ORCHESTRATOR_TOKEN must equal the token in that file's +# agent-orchestrator staticIdentities map, or every relayed /invoke 401s. +upsert e2e-integration-gateway-secrets \ + --from-literal=GITHUB_WEBHOOK_SECRET="$(rand)" \ + --from-literal=GATEWAY_ORCHESTRATOR_TOKEN="e2e-gateway-token" \ + --from-literal=GATEWAY_IDENTITY_LINK_TOKEN="$IDENTITY_LINK_TOKEN" \ + --from-literal=IDENTITY_LINK_ENCRYPTION_KEY="$(key32)" \ + --from-literal=IDENTITY_LINK_STATE_SECRET="$(rand)" \ + --from-literal=GITHUB_APP_CLIENT_SECRET="e2e-not-a-real-secret" \ + --from-literal=GATEWAY_SENDER_ASSERTION_SECRET="$SENDER_ASSERTION_SECRET" \ + --from-literal=GITHUB_TOKEN="e2e-not-a-real-token" + +# 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 \ + --from-literal=GITHUB_TOKEN="e2e-not-a-real-token" \ + --from-literal=GITHUB_APP_ID="0" \ + --from-literal=GITHUB_APP_INSTALLATION_ID="0" \ + --from-literal=GITHUB_APP_PRIVATE_KEY="e2e-not-a-real-key" + +echo "" +if kubectl -n "$NS" get secret agent-orchestrator-secrets >/dev/null 2>&1; then + # PATCH, not replace: this secret holds a real OPENAI_API_KEY that this + # 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)" +else + cat >&2 < \\ + --from-literal=AGENT_CALLBACK_SECRET="\$(openssl rand -hex 32)" + +EOF + exit 1 +fi + +echo "" +echo "Done. Next:" +echo " skaffold run" +echo " helm upgrade --install agent-controller charts/agent-controller \\" +echo " -f charts/agent-controller/values-minikube-demo.yaml \\" +echo " -f charts/agent-controller/values-e2e.yaml" +echo " npm run e2e -w e2e" diff --git a/e2e/scripts/up.sh b/e2e/scripts/up.sh new file mode 100755 index 0000000..e924e07 --- /dev/null +++ b/e2e/scripts/up.sh @@ -0,0 +1,103 @@ +#!/usr/bin/env bash +# up.sh — bring the whole e2e stack up on minikube, from nothing to ready. +# +# Usage: ./e2e/scripts/up.sh +# +# This is the ONE entry point for the e2e environment. It exists because +# getting here by hand requires knowing three non-obvious things, each of +# which produced a confusing failure the first time through: +# +# 1. Helm does not recurse into file:// subcharts. `skaffold run` builds +# charts/agent-controller's dependencies but NOT agent-orchestrator's own +# qdrant dependency, so qdrant is silently never deployed and the +# orchestrator crashloops on "qdrant startup check failed" while Helm +# reports the far less helpful "context deadline exceeded". +# 2. The gateway secret must be created BEFORE the deploy and must not use +# the chart's own generated name, or Helm refuses to adopt it +# ("invalid ownership metadata") and rolls the release back. +# 3. The default skaffold profile never builds integration-gateway, because +# the demo profile leaves that component disabled — but the e2e suite's +# entry point is a webhook hitting exactly that gateway. +# +# Idempotent: safe to re-run after a failure or a code change. + +set -euo pipefail + +REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" +NS="controller-agent" +CTX="minikube" + +step() { echo ""; echo "▶ $*"; } + +CURRENT="$(kubectl config current-context 2>/dev/null || true)" +if [[ "$CURRENT" != "$CTX" ]]; then + echo "✗ kubectl context is '$CURRENT', not '$CTX'. Refusing to deploy." >&2 + echo " Switch with: kubectl config use-context $CTX" >&2 + exit 1 +fi + +step "Fetching nested subchart dependencies (Helm won't recurse into these)..." +# The top-level chart's deps are handled by skaffold (skipBuildDependencies: +# false). This is the nested one it cannot see -- see note 1 in the header. +helm dependency update "$REPO_ROOT/charts/agent-controller/charts/agent-orchestrator" >/dev/null +echo " ✓ qdrant" + +step "Applying CRDs..." +# Helm's crds/ directory is INSTALL-ONLY -- `helm upgrade` never touches it. +# So a cluster whose release predates a newly-added CRD never gets it, and the +# orchestrator crashloops with a bare `404 page not found` from the API server +# while listing that resource. This is exactly how `integrationroutes` came to +# be missing here: every other CRD existed, from an older install. +# +# NOTE: dev-up.sh applies these from charts/agent-controller/charts/ +# core-controller/crds/, which no longer exists. The generated bases below are +# the real source. +for crd in "$REPO_ROOT"/controllers/core-controller/config/crd/bases/*.yaml; do + kubectl apply -f "$crd" --server-side --force-conflicts >/dev/null +done +echo " ✓ $(ls "$REPO_ROOT"/controllers/core-controller/config/crd/bases/*.yaml | wc -l | tr -d ' ') CRDs" + +step "Creating throwaway secrets..." +"$REPO_ROOT/e2e/scripts/bootstrap-secrets.sh" + +step "Adopting hand-created objects into Helm..." +# dev-up.sh creates these ServiceAccounts with `kubectl create serviceaccount`, +# but community-components' templates also declare them. Helm refuses to adopt +# an object it didn't create ("invalid ownership metadata") and fails the +# WHOLE release, so any cluster that ever ran dev-up.sh blocks this deploy. +# +# Labelling is non-destructive and preserves existing RoleBindings, unlike +# deleting and letting Helm recreate them. +for sa in recipe-scraper recipe-publisher opencode-swe-agent web-search claude-code-swe-agent; do + if kubectl -n "$NS" get sa "$sa" >/dev/null 2>&1; then + kubectl -n "$NS" label sa "$sa" app.kubernetes.io/managed-by=Helm --overwrite >/dev/null + kubectl -n "$NS" annotate sa "$sa" \ + meta.helm.sh/release-name=community-components \ + meta.helm.sh/release-namespace="$NS" --overwrite >/dev/null + fi +done +echo " ✓ ServiceAccounts" + +step "Building images and deploying (skaffold profile: e2e)..." +# If you are ever debugging behaviour that "the code clearly does not do any +# more", suspect the image before the logic. Skaffold decides whether to rebuild +# an artifact from a dependency list IT resolves from the Dockerfile + the +# .dockerignore, and a .dockerignore it can't evaluate yields a list that never +# changes -- so it silently re-tags an old image for every new commit. That +# shipped an eight-day-old core-controller here, presenting as a rendered Job +# missing its per-run secretEnv (see controllers/core-controller/.dockerignore). +# +# Check with: skaffold diagnose -p e2e | grep -A3 'Docker artifact: ' +# A dependency count far below the artifact's real source-file count is the tell. +(cd "$REPO_ROOT" && skaffold run -p e2e) + +step "Waiting for the stack to be ready..." +# `helm --wait` already gates on this, but it is re-checked explicitly so a +# partially-ready cluster fails HERE with a readable pod list rather than +# inside a test's waitFor timeout, where it looks like a product bug. +kubectl -n "$NS" wait --for=condition=Available --timeout=300s \ + deploy/agent-orchestrator deploy/agent-controller-integration-gateway + +echo "" +echo "✓ e2e stack is up. Run the suite with:" +echo " npm run e2e -w e2e" diff --git a/e2e/specs/happy-path.e2e.ts b/e2e/specs/happy-path.e2e.ts new file mode 100644 index 0000000..21de9dc --- /dev/null +++ b/e2e/specs/happy-path.e2e.ts @@ -0,0 +1,168 @@ +import { afterAll, beforeAll, describe, expect, it } from "vitest"; +import { requireMinikubeContext } from "../support/guard.js"; +import { + agentRunSecretEnvNames, + agentRunsSince, + cleanupAgentRunsSince, + jobEnvNames, + waitFor, + withPortForward, +} from "../support/k8s.js"; +import { issueLabeledPayload, postGithubWebhook } from "../support/webhook.js"; +import { fakeGithubRequests, resetFakeGithub, webhookSecret } from "../support/fixtures.js"; +import { seedAllClaudeCredentials } from "../support/redis.js"; + +// Module scope, before any fixture: a suite pointed at the wrong cluster must +// fail on import, not after it has started creating objects. +requireMinikubeContext(); + +const GATEWAY_PORT = 18090; +const SENDER = "e2e-user"; +const OWNER = "e2e-org"; +const REPO = "e2e-repo"; + +/** + * Must match `STUB_REPLY_MARKER` in apps/stub-agent/src/reply.ts. + * + * Matched on instead of prose: the assertion has to distinguish "the stub + * replied and the gateway relayed it" from "some comment appeared", and keying + * that to a sentence produces a spec that breaks on a rewording. + */ +const STUB_REPLY_MARKER = "stub-agent-reply"; + +/** + * The chain, end to end, with only the model turn replaced. + * + * This spec was skipped for one reason: a real `claude-code-swe-agent` run needs + * a paid Anthropic credential, so in a hermetic cluster the AgentRun never + * reached a terminal phase and nothing past the launch could be asserted. + * `stub-agent` (apps/stub-agent) removes that blocker -- it speaks the real NATS + * agent protocol and returns a canned reply -- so everything between the webhook + * and the issue comment is now exercised for real: + * + * webhook HMAC -> IntegrationRoute dispatch -> authorization pre-flight -> + * AgentRun CR -> hardened Job -> NATS ready/progress/reply -> gateway relay -> + * comment posted to the issue + * + * It deliberately does NOT re-assert credential KEYING; identity-keying.e2e.ts + * owns that, including the negative controls. The overlap is only that both need + * the gate to resolve, which is why this seeds too. + */ +describe("happy path: GitHub issue label -> triage -> agent run -> comment posted", () => { + let secret: string; + let suiteStartedAt: Date; + + beforeAll(async () => { + suiteStartedAt = new Date(); + secret = await webhookSecret(); + await resetFakeGithub(); + // Without this the identity gate correctly PARKS -- stub-agent declares the + // same Claude identityProviders as the agent it stands in for, and no + // credential exists -- so no AgentRun is ever created and the chain cannot + // be observed. See seedAllClaudeCredentials for why a real `claude login` + // is impossible in a hermetic test. + await seedAllClaudeCredentials(`github:${SENDER}`); + }); + + // Nothing else reclaims AgentRun CRs or their identity Secrets within the + // controller's retention window; Jobs have a TTL, these do not. + afterAll(async () => { + await cleanupAgentRunsSince(suiteStartedAt); + }); + + it("runs the full chain and posts the agent's reply back to the issue", async () => { + const startedAt = new Date(); + // Unique per run: the issue number IS the session key, so a fixed one makes + // a re-run inherit the previous run's session instead of starting fresh. + const issueNumber = Date.now() % 100000; + + const status = await withPortForward("agent-controller-integration-gateway", 8090, GATEWAY_PORT, async (baseUrl) => { + const res = await postGithubWebhook( + baseUrl, + "issues", + issueLabeledPayload({ + owner: OWNER, + repo: REPO, + issueNumber, + label: "ai-triage", + senderLogin: SENDER, + }), + secret, + ); + return res.status; + }); + + // The gateway acknowledges the webhook immediately and relays in the + // background, so a 2xx here means "accepted", not "done". Everything real + // is asserted below by polling. + expect(status).toBeGreaterThanOrEqual(200); + expect(status).toBeLessThan(300); + + const run = await waitFor( + "an AgentRun to be created for the labeled issue", + async () => (await agentRunsSince(startedAt))[0], + { timeoutMs: 420_000 }, + ); + + // ADR 0030 §4/§5: what the pre-flight decided to inject, on the + // orchestrator's own output. + const crSecretEnv = await agentRunSecretEnvNames(run.name); + expect(crSecretEnv).toContain("AGENT_ACTOR_LOGIN"); + + // Env NAMES only -- never values (see jobEnvNames). Asserting the count + // rather than mere containment is deliberate: duplicate env names are legal + // in Kubernetes and the LAST one wins, so a static `secretEnv` shadowing a + // per-run identity override would slip past a containment check while + // breaking the run. + // + // That core-controller renders `spec.secretEnv` into the Job at all was + // itself once in doubt -- it did not happen on minikube, which turned out to + // be a stale controller image rather than a defect (controllers/ + // core-controller/.dockerignore). Asserting it here is what would catch a + // recurrence. + const envNames = await jobEnvNames(run.name); + for (const name of crSecretEnv) { + expect(envNames.filter((n) => n === name), `${name} should appear exactly once in the Job's env`).toHaveLength(1); + } + + await waitFor( + "the AgentRun to reach a terminal phase", + async () => { + const [current] = await agentRunsSince(startedAt); + return current?.phase === "Succeeded" || current?.phase === "Failed" ? current : undefined; + }, + { timeoutMs: 300_000 }, + ); + + const comment = await waitFor( + `a comment to be posted to issue #${issueNumber}`, + async () => { + const posted = (await fakeGithubRequests()).filter( + (r) => r.method === "POST" && r.path === `/repos/${OWNER}/${REPO}/issues/${issueNumber}/comments`, + ); + return posted.length > 0 ? posted[posted.length - 1] : undefined; + }, + { timeoutMs: 120_000 }, + ); + + // The stub's own reply, relayed -- not just any comment. Without the marker + // this passes on an error comment the gateway posts when a run FAILS, which + // is the opposite of what the spec is for. + expect(comment?.body).toContain(STUB_REPLY_MARKER); + // The goal survived webhook -> route -> orchestrator -> AgentRun -> Job -> + // NATS intact. The stub echoes it back; the triage prompt names the issue. + expect(comment?.body).toContain(String(issueNumber)); + // ADR 0030 §5: the sealed actor context reached the container, not merely + // the Job spec. Only the NAME is asserted -- the stub reports names only, + // and this body is published to a GitHub issue. + expect(comment?.body).toContain("AGENT_ACTOR_LOGIN"); + }); + + it("never calls an unstubbed GitHub endpoint", async () => { + // fake-github 501s anything it doesn't know. If the system started calling + // a new endpoint, this catches it as an explicit failure rather than as a + // mystery 501 buried in an agent log. + const unstubbed = (await fakeGithubRequests()).filter((r) => r.path.startsWith("/_e2e/") === false && r.status === 501); + expect(unstubbed).toEqual([]); + }); +}); diff --git a/e2e/specs/identity-keying.e2e.ts b/e2e/specs/identity-keying.e2e.ts new file mode 100644 index 0000000..c56fe17 --- /dev/null +++ b/e2e/specs/identity-keying.e2e.ts @@ -0,0 +1,169 @@ +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 { issueLabeledPayload, postGithubWebhook } from "../support/webhook.js"; +import { resetFakeGithub, webhookSecret } from "../support/fixtures.js"; + +requireMinikubeContext(); + +const GATEWAY_PORT = 18092; +const SENDER = "e2e-user"; +const CANONICAL = `github:${SENDER}`; +/** What integration-gateway's own OIDC service token resolves to for EVERY webhook turn. */ +const SHARED_SERVICE_SUBJECT = "client-integration-gateway"; + +/** + * Issue numbers are unique per RUN, not fixed constants. + * + * A GitHub issue number is the session key for the whole triage flow + * (`sess:github:/#` in the orchestrator, plus the gateway's + * own session-page record). Reusing a fixed number means the second run of a + * spec inherits the first run's session state -- an active agent run, a + * continuation -- and the turn takes a different path instead of launching + * fresh. That produced a spec that passed once and then timed out on re-runs + * while an identically-seeded sibling passed, which reads as product flakiness + * but is entirely test-owned shared state. + */ +const RUN_ID = Date.now() % 100000; +const issueNo = (offset: number): number => RUN_ID + offset; + +/** + * The regression this suite exists for. + * + * A human authorized Claude during ai-triage, went to chat, and was asked to + * authorize again -- `ClaudeTokenStore` keys by `identity.subject`, and the + * webhook path (one shared OIDC service subject) and the chat path + * (`openwebui:`) resolve different ones. ADR 0029 converges both on a + * canonical `github:`; ADR 0030 decouples that from credential + * provisioning. + * + * ## Why these assert on a LAUNCH rather than on a stored credential + * + * Starting a real `claude` flow needs the PTY-driven `claude login` and a paid + * Anthropic credential, so "a credential appears under github:" can + * never happen hermetically. The assertion is inverted instead: seed a + * credential at the subject the gate is believed to resolve, and require the + * run to LAUNCH. A gate looking anywhere else finds nothing and parks, so the + * launch is a precise, behavioural proof of which subject was used -- not a + * log-scrape. + */ +describe("credential keying converges across entry points (ADR 0029/0030)", () => { + let secret: string; + let suiteStartedAt: Date; + + beforeAll(() => { + suiteStartedAt = new Date(); + }); + + // Nothing else reclaims AgentRun CRs or their identity Secrets -- Jobs have + // a TTL, these do not. Without this the suite leaves permanent residue that + // every later `agentRunsSince` has to list and filter. + afterAll(async () => { + await cleanupAgentRunsSince(suiteStartedAt); + }); + + beforeEach(async () => { + secret = await webhookSecret(); + await resetFakeGithub(); + // Every test decides its own seeding, so start with nothing: a leftover + // record is indistinguishable from a correctly-resolved one. + await deleteCredentialKeys("claudeAuth:*"); + await deleteCredentialKeys("claudeAuthLogin:*"); + await deleteCredentialKeys("sess:*"); + }); + + async function trigger(issueNumber: number, senderLogin: string): Promise { + await withPortForward("agent-controller-integration-gateway", 8090, GATEWAY_PORT, async (baseUrl) => { + const res = await postGithubWebhook( + baseUrl, + "issues", + issueLabeledPayload({ owner: "e2e-org", repo: "e2e-repo", issueNumber, label: "ai-triage", senderLogin }), + secret, + ); + expect(res.status).toBeGreaterThanOrEqual(200); + expect(res.status).toBeLessThan(300); + }); + } + + it("resolves a webhook turn's Claude credentials from github:", async () => { + await seedAllClaudeCredentials(CANONICAL); + const startedAt = new Date(); + + await trigger(issueNo(1), SENDER); + + // Launching proves the gate resolved BOTH claude providers -- it refuses + // to launch otherwise -- and the only place those credentials exist is + // the canonical subject. + const run = await waitFor( + "an AgentRun to launch using the canonically-keyed credentials", + async () => (await agentRunsSince(startedAt))[0], + // Generous on purpose. The negative-control specs above each leave the + // gateway's relay polling for its full window, so a later positive spec + // queues behind that backlog -- observed launch latency exceeded a + // 180s budget even though the gate had already resolved. Too tight a + // budget here fails for throughput reasons and reads as a keying bug. + { timeoutMs: 420_000 }, + ); + + // Asserted on the AgentRun CR -- agent-orchestrator's own output. Whether + // core-controller then renders these into the Job's container env is that + // controller's own contract, with its own tests; conflating the two makes + // an orchestrator spec fail for a controller reason. (It was previously + // observed NOT to happen on minikube. That turned out to be a stale + // controller image, not a defect -- see controllers/core-controller/ + // .dockerignore -- and the happy-path spec asserts the rendered Job.) + const envNames = await agentRunSecretEnvNames(run.name); + expect(envNames).toContain("CLAUDE_CODE_OAUTH_TOKEN"); + expect(envNames).toContain("CLAUDE_LOGIN_CREDENTIALS_JSON"); + // ADR 0030 §5: the orchestrator resolved WHO the caller is, so the agent + // never calls GitHub's /user itself -- the call that 401'd in production. + expect(envNames).toContain("AGENT_ACTOR_LOGIN"); + }); + + it("does NOT resolve them from the shared service subject", async () => { + // The negative control, and the actual bug. Before ADR 0029 every webhook + // turn keyed on this one subject, so whoever authorized first silently + // shared their Claude credential with every other user of the system. + await seedAllClaudeCredentials(SHARED_SERVICE_SUBJECT); + const startedAt = new Date(); + + await trigger(issueNo(2), SENDER); + + // Give the turn real time to launch if it were going to; absence is the + // assertion here, so it must not be a race. + await new Promise((r) => setTimeout(r, 60_000)); + expect(await agentRunsSince(startedAt)).toHaveLength(0); + }); + + it("does not let one sender's credential satisfy another sender's turn", async () => { + await seedAllClaudeCredentials("github:someone-else"); + const startedAt = new Date(); + + await trigger(issueNo(3), SENDER); + + await new Promise((r) => setTimeout(r, 60_000)); + expect(await agentRunsSince(startedAt)).toHaveLength(0); + }); + + it("treats webhook casing and OAuth casing as one subject", async () => { + // GitHub echoes original casing in webhooks but normalizes it in the OAuth + // user API. Two casings keying two records is precisely the re-prompt loop + // this all started from. + await seedAllClaudeCredentials(CANONICAL); + const startedAt = new Date(); + + await trigger(issueNo(4), "E2E-User"); + + await waitFor( + "a mixed-case sender to resolve the lower-cased canonical credentials", + async () => (await agentRunsSince(startedAt))[0], + // Generous on purpose. The negative-control specs above each leave the + // gateway's relay polling for its full window, so a later positive spec + // queues behind that backlog -- observed launch latency exceeded a + // 180s budget even though the gate had already resolved. Too tight a + // budget here fails for throughput reasons and reads as a keying bug. + { timeoutMs: 420_000 }, + ); + }); +}); diff --git a/e2e/support/fixtures.ts b/e2e/support/fixtures.ts new file mode 100644 index 0000000..590c500 --- /dev/null +++ b/e2e/support/fixtures.ts @@ -0,0 +1,95 @@ +import { createHash } from "node:crypto"; +import { readFile } from "node:fs/promises"; +import { kubectl, kubectlApplyStdin, withPortForward } from "./k8s.js"; + +const FAKE_GITHUB_PORT = 18081; + +export interface RecordedRequest { + method: string; + path: string; + body: string | null; + at: string; + /** Present only on responses fake-github rejected as unstubbed. */ + status?: number; +} + +/** + * The gateway's webhook HMAC secret, read from the cluster rather than + * hardcoded — the test has to sign with whatever the deployment is actually + * verifying against, or every webhook 401s for a reason that looks like a + * gateway bug. + * + * This is the one place a secret VALUE is read, and it is unavoidable: signing + * is the whole point. It is never logged and never asserted on. + */ +export async function webhookSecret(): Promise { + const b64 = ( + await kubectl([ + "get", + "secret", + "e2e-integration-gateway-secrets", + "-o", + "jsonpath={.data.GITHUB_WEBHOOK_SECRET}", + ]) + ).trim(); + if (!b64) throw new Error("e2e: GITHUB_WEBHOOK_SECRET not found on e2e-integration-gateway-secrets (run e2e/scripts/bootstrap-secrets.sh)"); + return Buffer.from(b64, "base64").toString("utf8"); +} + +/** + * Applies the fake-github manifest and waits for the pod SERVING THAT VERSION + * of the script to be ready. Idempotent, and a no-op rollout when the manifest + * hasn't changed. + * + * The subtlety is the mounted ConfigMap. Applying an edited script updates the + * ConfigMap, and the kubelet eventually updates the mounted file -- but the + * `node` process read it once at startup and never re-reads it, so the pod goes + * on serving the previous script indefinitely while every readiness signal says + * Ready. That is how a readiness-probe fix to this file appeared not to take + * effect at all. + * + * `kubectl rollout restart` after the apply is the usual reflex and races: it + * can start the new pod before the ConfigMap write it was meant to pick up has + * landed, so the restart is wasted and the stale script survives it. Instead + * the manifest carries a checksum annotation on the pod template, substituted + * here, so the script's content is PART of the pod spec: a changed script + * changes the template and the Deployment rolls by itself, atomically with the + * apply. `rollout status` then blocks on the new generation specifically -- + * unlike a readyReplicas poll, which the outgoing pod satisfies immediately. + */ +export async function ensureFakeGithub(): Promise { + const manifestPath = new URL("../manifests/fake-github.yaml", import.meta.url).pathname; + const template = await readFile(manifestPath, "utf8"); + // Hashed with the placeholder still in it, so the value is a pure function of + // the file on disk and re-applying an unchanged file is a genuine no-op. + const checksum = createHash("sha256").update(template).digest("hex").slice(0, 16); + const manifest = template.replace("REPLACED_AT_APPLY", checksum); + if (manifest === template) { + throw new Error(`e2e: ${manifestPath} is missing the REPLACED_AT_APPLY checksum placeholder; a script edit would not roll the pod`); + } + + await kubectlApplyStdin(manifest); + // Blocks until the pod matching the applied template is Available. Bounded + // rather than indefinite so a genuinely broken script (a syntax error, say) + // fails here with kubectl's own diagnosis instead of inside a spec's waitFor. + await kubectl(["rollout", "status", "deploy/fake-github", "--timeout=120s"]); +} + +export async function fakeGithubRequests(): Promise { + return withPortForward("fake-github", 80, FAKE_GITHUB_PORT, async (baseUrl) => { + const res = await fetch(`${baseUrl}/_e2e/requests`); + return (await res.json()) as RecordedRequest[]; + }); +} + +/** + * Clears fake-github's recorded requests so a test asserts only on traffic it + * caused. Called in `beforeAll` rather than `afterAll`: a run that crashes + * mid-test should leave its evidence in place for inspection. + */ +export async function resetFakeGithub(): Promise { + await ensureFakeGithub(); + await withPortForward("fake-github", 80, FAKE_GITHUB_PORT, async (baseUrl) => { + await fetch(`${baseUrl}/_e2e/reset`, { method: "POST" }); + }); +} diff --git a/e2e/support/guard.ts b/e2e/support/guard.ts new file mode 100644 index 0000000..daad153 --- /dev/null +++ b/e2e/support/guard.ts @@ -0,0 +1,51 @@ +import { execFileSync } from "node:child_process"; + +/** + * The ONLY kubectl context these tests may touch. + * + * Deliberately a hardcoded constant with no environment-variable override. + * The maintainer's default context is a live cluster running the real + * deployment, these tests create and delete namespaced objects, and the + * failure mode of "ran the suite against the wrong cluster" is destructive + * and not obviously recoverable. An override flag would exist purely to be + * set by accident in CI. + */ +const REQUIRED_CONTEXT = "minikube"; + +let verified = false; + +/** + * Aborts unless kubectl is pointed at minikube. Every spec file calls this at + * module scope, before any fixture allocates anything, so a misconfigured run + * fails on import rather than midway through creating objects somewhere it + * shouldn't. + * + * Memoized: the check shells out, and every spec calling it would otherwise + * pay for it repeatedly in a serial suite. + */ +export function requireMinikubeContext(): void { + if (verified) return; + + let current: string; + try { + current = execFileSync("kubectl", ["config", "current-context"], { encoding: "utf8" }).trim(); + } catch (err) { + throw new Error( + `e2e: could not determine the kubectl context (is kubectl installed and configured?): ${ + err instanceof Error ? err.message : String(err) + }`, + ); + } + + if (current !== REQUIRED_CONTEXT) { + throw new Error( + [ + `e2e: refusing to run against kubectl context "${current}".`, + `These tests create and delete cluster objects and may ONLY run against "${REQUIRED_CONTEXT}".`, + `Switch with: kubectl config use-context ${REQUIRED_CONTEXT}`, + ].join("\n"), + ); + } + + verified = true; +} diff --git a/e2e/support/k8s.ts b/e2e/support/k8s.ts new file mode 100644 index 0000000..460ccad --- /dev/null +++ b/e2e/support/k8s.ts @@ -0,0 +1,183 @@ +import { execFile } from "node:child_process"; +import { promisify } from "node:util"; +import { requireMinikubeContext } from "./guard.js"; + +const exec = promisify(execFile); + +export const NAMESPACE = "controller-agent"; + +/** + * Runs kubectl and returns stdout. Re-checks the context on every call rather + * than trusting that the module-scope guard ran: a spec that forgot to import + * the guard would otherwise reach the cluster through this helper. + */ +export async function kubectl(args: string[]): Promise { + requireMinikubeContext(); + const { stdout } = await exec("kubectl", ["-n", NAMESPACE, ...args], { maxBuffer: 32 * 1024 * 1024 }); + return stdout; +} + +export async function kubectlJson(args: string[]): Promise { + return JSON.parse(await kubectl([...args, "-o", "json"])) as T; +} + +/** + * `kubectl apply -f -` with `manifest` on stdin. + * + * Needed because the e2e manifests are templated before they are applied (see + * ensureFakeGithub's checksum substitution) and writing the rendered YAML to a + * temp file just to hand kubectl a path adds a cleanup path that can fail. + */ +export async function kubectlApplyStdin(manifest: string): Promise { + requireMinikubeContext(); + return new Promise((resolve, reject) => { + const child = execFile( + "kubectl", + ["-n", NAMESPACE, "apply", "-f", "-"], + { maxBuffer: 32 * 1024 * 1024 }, + (err, stdout, stderr) => (err ? reject(new Error(`kubectl apply -f - failed: ${stderr || err.message}`)) : resolve(stdout)), + ); + child.stdin?.end(manifest); + }); +} + +/** + * Polls `probe` until it returns a non-null/undefined value, or the timeout + * elapses. Returns what the probe returned so callers can assert on it. + * + * Everything asynchronous in this system is eventually-consistent across at + * least two processes (a webhook lands, the gateway relays, the orchestrator + * runs a graph, a controller reconciles a CR, a Job schedules), so a bare + * assertion after a fixed sleep is either flaky or slow. `describe` is + * included in the timeout message because a bare "timed out" tells you + * nothing about which of those hops stalled. + */ +export async function waitFor( + describe: string, + probe: () => Promise, + { timeoutMs = 120_000, intervalMs = 2_000 }: { timeoutMs?: number; intervalMs?: number } = {}, +): Promise { + const deadline = Date.now() + timeoutMs; + let lastError: unknown; + while (Date.now() < deadline) { + try { + const value = await probe(); + if (value !== undefined && value !== null) return value; + } catch (err) { + // Probes routinely throw early on (a CR that doesn't exist yet makes + // kubectl exit non-zero). Hold the last one to report if we time out -- + // a persistent error is far more useful than "condition never met". + lastError = err; + } + await new Promise((r) => setTimeout(r, intervalMs)); + } + throw new Error( + `e2e: timed out after ${timeoutMs}ms waiting for ${describe}` + + (lastError ? `; last probe error: ${lastError instanceof Error ? lastError.message : String(lastError)}` : ""), + ); +} + +/** AgentRuns created since `since`, newest first -- the handle for "did this trigger actually launch an agent". */ +export async function agentRunsSince(since: Date): Promise<{ name: string; phase?: string }[]> { + const list = await kubectlJson<{ + items: { metadata: { name: string; creationTimestamp: string }; status?: { phase?: string } }[]; + }>(["get", "agentruns"]); + return list.items + .filter((i) => new Date(i.metadata.creationTimestamp) >= since) + .sort((a, b) => b.metadata.creationTimestamp.localeCompare(a.metadata.creationTimestamp)) + .map((i) => ({ name: i.metadata.name, phase: i.status?.phase })); +} + +/** + * Env var NAMES on a launched AgentRun's Job, in order. + * + * Names only, never values: these are live credentials, and a test that + * printed one on failure would leak it into CI logs. Order is returned + * because duplicate env names are legal in Kubernetes and the LAST one wins -- + * a static `secretEnv` shadowing a per-run identity override is a real + * failure mode this is meant to catch. + */ +export async function jobEnvNames(agentRunName: string): Promise { + const job = await kubectlJson<{ + spec: { template: { spec: { containers: { env?: { name: string }[] }[] } } }; + }>(["get", "job", `agentrun-${agentRunName}`]); + return (job.spec.template.spec.containers[0]?.env ?? []).map((e) => e.name); +} + +/** Port-forwards a Service for the duration of `body`, then tears it down. */ +export async function withPortForward( + service: string, + remotePort: number, + localPort: number, + body: (baseUrl: string) => Promise, +): Promise { + requireMinikubeContext(); + const child = execFile("kubectl", ["-n", NAMESPACE, "port-forward", `svc/${service}`, `${localPort}:${remotePort}`]); + try { + // kubectl prints "Forwarding from ..." once the listener is up; poll the + // socket instead of trusting a fixed sleep, which is the usual source of + // "connection refused" flakes on a loaded machine. + await waitFor( + `port-forward to ${service}:${remotePort} to accept connections`, + async () => { + try { + await fetch(`http://127.0.0.1:${localPort}/`, { signal: AbortSignal.timeout(1_000) }); + return true; + } catch { + return undefined; + } + }, + { timeoutMs: 30_000, intervalMs: 500 }, + ); + return await body(`http://127.0.0.1:${localPort}`); + } finally { + child.kill(); + } +} + +/** + * `spec.secretEnv` entry NAMES on an AgentRun CR. + * + * The CR is agent-orchestrator's OUTPUT -- what the authorization pre-flight + * decided to inject. Asserting here rather than on the rendered Job keeps this + * scoped to the component under test: turning `spec.secretEnv` into container + * env is core-controller's job, with its own tests, and conflating the two + * makes an orchestrator spec fail for a controller reason. + * + * Names only, never values -- same discipline as {@link jobEnvNames}. + */ +export async function agentRunSecretEnvNames(agentRunName: string): Promise { + const cr = await kubectlJson<{ spec?: { secretEnv?: { name: string }[] } }>(["get", "agentrun", agentRunName]); + return (cr.spec?.secretEnv ?? []).map((e) => e.name); +} + +/** + * Deletes AgentRun CRs created at or after `since`, plus the Kubernetes + * Secrets each one owns. + * + * Nothing else deletes them. The controller sets a TTL on the Job it creates, + * so Jobs disappear -- but the AgentRun CR and its `-identity` Secret + * persist indefinitely. A suite that triggers a handful of runs per execution + * therefore leaves permanent residue: 50 CRs and 26 Secrets had accumulated + * here before this existed. + * + * That residue is not merely untidy. Cross-run state is what made these specs + * flaky in the first place (a reused issue number inherits its predecessor's + * session), and `agentRunsSince` has to list and filter every CR ever created, + * which grows without bound. + * + * Deleting the CR should cascade to the Secret via its ownerReference; the + * explicit sweep is a backstop for any Secret whose owner was already gone. + */ +export async function cleanupAgentRunsSince(since: Date): Promise { + const runs = await agentRunsSince(since); + for (const run of runs) { + // --wait=false: teardown must not block the suite on finalizers, and a + // failure here should never fail a test that already passed. + await kubectl(["delete", "agentrun", run.name, "--wait=false", "--ignore-not-found"]).catch(() => undefined); + await kubectl(["delete", "secret", `${run.name}-identity`, "--wait=false", "--ignore-not-found"]).catch( + () => undefined, + ); + } + return runs.length; +} diff --git a/e2e/support/redis.ts b/e2e/support/redis.ts new file mode 100644 index 0000000..4a26fd5 --- /dev/null +++ b/e2e/support/redis.ts @@ -0,0 +1,112 @@ +import { kubectl } from "./k8s.js"; + +/** + * Reads the orchestrator's Redis through `kubectl exec` rather than a + * port-forward + client library: it needs no extra dependency, no open local + * port, and matches how you'd inspect this by hand when a credential ends up + * under an unexpected key. + */ +async function redisCli(args: string[]): Promise { + // `app.kubernetes.io/name`, not `app` -- the chart uses the standard + // recommended labels, and the obvious-looking `app=` selector silently + // matches nothing (kubectl exits 0 with empty output, so it reads as "no + // Redis" rather than "wrong selector"). + const pod = ( + await kubectl(["get", "pods", "-l", "app.kubernetes.io/name=agent-orchestrator-redis", "-o", "name"]) + ) + .trim() + .split("\n") + .filter(Boolean)[0]; + if (!pod) throw new Error("e2e: no agent-orchestrator-redis pod found (label app.kubernetes.io/name=agent-orchestrator-redis)"); + return (await kubectl(["exec", pod.replace("pod/", ""), "--", "redis-cli", ...args])).trim(); +} + +/** Writes a raw value. Used only for seeding fixtures, never for assertions. */ +async function redisSet(key: string, value: string): Promise { + await redisCli(["SET", key, value]); +} + +/** + * Every key matching `pattern`. + * + * KEY NAMES ONLY -- this module never reads a credential VALUE. The subject a + * credential is keyed under is the entire thing these tests assert on (see + * ADR 0029), and the encrypted blob behind it is both useless to assert on + * and dangerous to surface in a failure message. + */ +export async function credentialKeys(pattern: string): Promise { + const out = await redisCli(["--scan", "--pattern", pattern]); + return out ? out.split("\n").map((k) => k.trim()).filter(Boolean).sort() : []; +} + +/** Deletes keys matching `pattern`, so a test starts from a known-unlinked state. */ +export async function deleteCredentialKeys(pattern: string): Promise { + const keys = await credentialKeys(pattern); + for (const key of keys) await redisCli(["DEL", key]); + return keys.length; +} + +/** + * The subject portion of a Claude credential key, for the assertion these + * tests exist to make: that a triage turn and a chat turn by the same human + * converge on ONE subject. + * + * `kind` maps to the two prefixes `RedisClaudeTokenStore` uses -- `setup-token` + * records live under `claudeAuth:`, full-login (Remote Control) records under + * `claudeAuthLogin:`. + */ +export async function claudeCredentialSubjects(kind: "setup-token" | "login"): Promise { + const prefix = kind === "login" ? "claudeAuthLogin:" : "claudeAuth:"; + const keys = await credentialKeys(`${prefix}*`); + return keys + // `claudeAuth:` is a prefix of `claudeAuthLogin:` and `claudeAuthWriteback:`, + // so a glob on it alone would sweep in both other record types. + .filter((k) => !k.startsWith("claudeAuthLogin:") || kind === "login") + .filter((k) => !k.startsWith("claudeAuthWriteback:")) + .map((k) => k.slice(prefix.length)); +} + +/** + * Writes a Claude credential straight into the store, encrypted exactly the + * way `RedisClaudeTokenStore` writes it (AES-256-GCM, packed + * `iv:authTag:ciphertext` base64, under the `claudeAuth:` / `claudeAuthLogin:` + * prefix for the given kind). + * + * Seeding is what makes the keying assertion possible at all. Starting a real + * `claude` flow needs the PTY-driven `claude login` and a paid Anthropic + * credential, which no hermetic test can supply -- so asserting "a credential + * exists under github:" could never pass. Seeding inverts it: put a + * credential at the subject we believe the gate resolves, and assert the run + * 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 { + const keyB64 = ( + await kubectl(["get", "secret", "e2e-integration-gateway-secrets", "-o", "jsonpath={.data.IDENTITY_LINK_ENCRYPTION_KEY}"]) + ).trim(); + if (!keyB64) throw new Error("e2e: IDENTITY_LINK_ENCRYPTION_KEY missing (run bootstrap-secrets.sh)"); + const key = Buffer.from(Buffer.from(keyB64, "base64").toString("utf8"), "base64"); + 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 => { + 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")}`; + }; + + const record = + kind === "login" + ? { kind, createdAt: new Date().toISOString(), credentialsJson: encrypt(JSON.stringify({ e2e: true })) } + : { kind, createdAt: new Date().toISOString(), token: encrypt("sk-ant-oat01-e2e-seeded") }; + + const prefix = kind === "login" ? "claudeAuthLogin:" : "claudeAuth:"; + await redisSet(`${prefix}${subject}`, JSON.stringify(record)); +} + +/** 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"); + await seedClaudeCredential(subject, "login"); +} diff --git a/e2e/support/webhook.ts b/e2e/support/webhook.ts new file mode 100644 index 0000000..1063ab2 --- /dev/null +++ b/e2e/support/webhook.ts @@ -0,0 +1,66 @@ +import { createHmac } from "node:crypto"; + +/** + * Signs a payload the way GitHub does (`X-Hub-Signature-256`, HMAC-SHA256 over + * the raw body) and posts it to the gateway's webhook route. + * + * The signature is computed over the EXACT bytes sent, not a re-serialization: + * the gateway verifies against the raw body, so any difference in key order or + * whitespace between what we sign and what we send fails verification in a way + * that looks like a bug in the gateway rather than in the test. + */ +export async function postGithubWebhook( + baseUrl: string, + event: string, + payload: unknown, + secret: string, +): Promise { + const body = JSON.stringify(payload); + const signature = `sha256=${createHmac("sha256", secret).update(body).digest("hex")}`; + return fetch(`${baseUrl}/webhooks/github`, { + method: "POST", + headers: { + "content-type": "application/json", + "x-github-event": event, + "x-github-delivery": `e2e-${Date.now()}`, + "x-hub-signature-256": signature, + }, + body, + }); +} + +/** + * An `issues.labeled` payload carrying the trigger label — the real ai-triage + * entry point. + * + * `sender.login` matters more than it looks: it is the only per-user + * identifier a webhook-driven turn carries (the gateway authenticates to + * `/invoke` with its own service token), so it is what the canonical + * credential subject is derived from (ADR 0029). Tests vary it to prove two + * different humans don't collapse onto one credential. + */ +export function issueLabeledPayload({ + owner, + repo, + issueNumber, + label, + senderLogin, + title = "e2e: something is broken", + body = "Steps to reproduce are in the description.", +}: { + owner: string; + repo: string; + issueNumber: number; + label: string; + senderLogin: string; + title?: string; + body?: string; +}) { + return { + action: "labeled", + label: { name: label }, + issue: { number: issueNumber, title, body, labels: [{ name: label }] }, + repository: { name: repo, owner: { login: owner }, full_name: `${owner}/${repo}` }, + sender: { login: senderLogin, type: "User" }, + }; +} diff --git a/e2e/tsconfig.json b/e2e/tsconfig.json new file mode 100644 index 0000000..5bb80c9 --- /dev/null +++ b/e2e/tsconfig.json @@ -0,0 +1,17 @@ +{ + "compilerOptions": { + "target": "ES2022", + "module": "NodeNext", + "moduleResolution": "NodeNext", + "lib": ["ES2022"], + "strict": true, + "noUncheckedIndexedAccess": true, + "esModuleInterop": true, + "skipLibCheck": true, + "forceConsistentCasingInFileNames": true, + "noEmit": true, + "types": ["node"] + }, + "include": ["specs/**/*", "support/**/*"], + "exclude": ["node_modules"] +} diff --git a/e2e/vitest.config.ts b/e2e/vitest.config.ts new file mode 100644 index 0000000..3734dac --- /dev/null +++ b/e2e/vitest.config.ts @@ -0,0 +1,23 @@ +import { defineConfig } from "vitest/config"; + +export default defineConfig({ + test: { + include: ["specs/**/*.e2e.ts"], + // Serial, both levels. These share ONE cluster and several assert on + // global state (Redis keys, the AgentRun list, fake-github's recorded + // requests) that concurrent execution would race on -- a parallel run + // fails in ways that look like product bugs. + fileParallelism: false, + maxConcurrency: 1, + // A real triage turn launches a Job, pulls an image and runs an agent. + // The per-step budgets in `waitFor` are the meaningful deadlines; this is + // just a backstop well above them. + testTimeout: 600_000, + hookTimeout: 300_000, + // One retry: cluster-level flakes (image pull backoff, a port-forward + // dropped by a busy apiserver) are real and not worth a red build. Kept + // at 1 so a genuinely broken assertion still fails fast rather than + // being retried into looking intermittent. + retry: 1, + }, +}); diff --git a/package-lock.json b/package-lock.json index cc03f90..915e1a1 100644 --- a/package-lock.json +++ b/package-lock.json @@ -11,7 +11,8 @@ "packages/*", "apps/*", "tools/*", - "tools-local/http-get-node" + "tools-local/http-get-node", + "e2e" ] }, "apps/agent-orchestrator": { @@ -91,6 +92,28 @@ "node": ">=20" } }, + "apps/stub-agent": { + "version": "0.1.0", + "dependencies": { + "@controller-agent/agent-runtime": "0.1.0", + "@controller-agent/messaging": "0.1.0" + }, + "devDependencies": { + "@types/node": "^20.16.5", + "typescript": "^5.6.2", + "vitest": "^4.0.0" + }, + "engines": { + "node": ">=22" + } + }, + "e2e": { + "name": "@controller-agent/e2e", + "devDependencies": { + "typescript": "^5.6.0", + "vitest": "^4.1.10" + } + }, "node_modules/@asamuzakjp/css-color": { "version": "3.2.0", "resolved": "https://registry.npmjs.org/@asamuzakjp/css-color/-/css-color-3.2.0.tgz", @@ -114,6 +137,10 @@ "resolved": "packages/agent-runtime", "link": true }, + "node_modules/@controller-agent/e2e": { + "resolved": "e2e", + "link": true + }, "node_modules/@controller-agent/github-app-auth": { "resolved": "packages/github-app-auth", "link": true @@ -283,6 +310,7 @@ "os": [ "aix" ], + "peer": true, "engines": { "node": ">=18" } @@ -300,6 +328,7 @@ "os": [ "android" ], + "peer": true, "engines": { "node": ">=18" } @@ -317,6 +346,7 @@ "os": [ "android" ], + "peer": true, "engines": { "node": ">=18" } @@ -334,6 +364,7 @@ "os": [ "android" ], + "peer": true, "engines": { "node": ">=18" } @@ -351,6 +382,7 @@ "os": [ "darwin" ], + "peer": true, "engines": { "node": ">=18" } @@ -368,6 +400,7 @@ "os": [ "darwin" ], + "peer": true, "engines": { "node": ">=18" } @@ -385,6 +418,7 @@ "os": [ "freebsd" ], + "peer": true, "engines": { "node": ">=18" } @@ -402,6 +436,7 @@ "os": [ "freebsd" ], + "peer": true, "engines": { "node": ">=18" } @@ -419,6 +454,7 @@ "os": [ "linux" ], + "peer": true, "engines": { "node": ">=18" } @@ -436,6 +472,7 @@ "os": [ "linux" ], + "peer": true, "engines": { "node": ">=18" } @@ -453,6 +490,7 @@ "os": [ "linux" ], + "peer": true, "engines": { "node": ">=18" } @@ -470,6 +508,7 @@ "os": [ "linux" ], + "peer": true, "engines": { "node": ">=18" } @@ -487,6 +526,7 @@ "os": [ "linux" ], + "peer": true, "engines": { "node": ">=18" } @@ -504,6 +544,7 @@ "os": [ "linux" ], + "peer": true, "engines": { "node": ">=18" } @@ -521,6 +562,7 @@ "os": [ "linux" ], + "peer": true, "engines": { "node": ">=18" } @@ -538,6 +580,7 @@ "os": [ "linux" ], + "peer": true, "engines": { "node": ">=18" } @@ -555,6 +598,7 @@ "os": [ "linux" ], + "peer": true, "engines": { "node": ">=18" } @@ -572,6 +616,7 @@ "os": [ "netbsd" ], + "peer": true, "engines": { "node": ">=18" } @@ -589,6 +634,7 @@ "os": [ "netbsd" ], + "peer": true, "engines": { "node": ">=18" } @@ -606,6 +652,7 @@ "os": [ "openbsd" ], + "peer": true, "engines": { "node": ">=18" } @@ -623,6 +670,7 @@ "os": [ "openbsd" ], + "peer": true, "engines": { "node": ">=18" } @@ -640,6 +688,7 @@ "os": [ "openharmony" ], + "peer": true, "engines": { "node": ">=18" } @@ -657,6 +706,7 @@ "os": [ "sunos" ], + "peer": true, "engines": { "node": ">=18" } @@ -674,6 +724,7 @@ "os": [ "win32" ], + "peer": true, "engines": { "node": ">=18" } @@ -691,6 +742,7 @@ "os": [ "win32" ], + "peer": true, "engines": { "node": ">=18" } @@ -708,6 +760,7 @@ "os": [ "win32" ], + "peer": true, "engines": { "node": ">=18" } @@ -3795,6 +3848,10 @@ "text-decoder": "^1.1.0" } }, + "node_modules/stub-agent": { + "resolved": "apps/stub-agent", + "link": true + }, "node_modules/supports-color": { "version": "7.2.0", "resolved": "https://registry.npmjs.org/supports-color/-/supports-color-7.2.0.tgz", @@ -3975,6 +4032,7 @@ "os": [ "darwin" ], + "peer": true, "engines": { "node": "^8.16.0 || ^10.6.0 || >=11.0.0" } diff --git a/package.json b/package.json index 3f19136..b7aa3a7 100644 --- a/package.json +++ b/package.json @@ -6,11 +6,13 @@ "packages/*", "apps/*", "tools/*", - "tools-local/http-get-node" + "tools-local/http-get-node", + "e2e" ], "scripts": { "build": "npm run build --workspaces --if-present", "typecheck": "npm run typecheck --workspaces --if-present", - "test": "npm run test --workspaces --if-present" + "test": "npm run test --workspaces --if-present", + "e2e": "npm run e2e -w e2e" } } diff --git a/scripts/dev-up.sh b/scripts/dev-up.sh index 2e17b4b..68b7be8 100755 --- a/scripts/dev-up.sh +++ b/scripts/dev-up.sh @@ -110,9 +110,21 @@ step "Applying CRDs..." # every time so CRD schema changes are always current. These CRDs are what # the (separately-released) community-components chart's Tool/Skill/Agent CRs # depend on. -for crd in "$REPO_ROOT"/charts/agent-controller/charts/core-controller/crds/*.yaml; do - kubectl apply -f "$crd" --server-side >/dev/null +# Source of truth is the controller's generated bases. This previously globbed +# charts/agent-controller/charts/core-controller/crds/, which no longer exists +# -- and because an unmatched glob expands to nothing, the loop silently +# applied ZERO CRDs instead of failing. A cluster missing a newly-added CRD +# then crashlooped agent-orchestrator with a bare `404 page not found`. +shopt -s nullglob +crds=("$REPO_ROOT"/controllers/core-controller/config/crd/bases/*.yaml) +shopt -u nullglob +if [[ ${#crds[@]} -eq 0 ]]; then + die "No CRDs found under controllers/core-controller/config/crd/bases -- refusing to deploy without them." +fi +for crd in "${crds[@]}"; do + kubectl apply -f "$crd" --server-side --force-conflicts >/dev/null done +echo " Applied ${#crds[@]} CRDs." # ── 7. Hand off to Skaffold ─────────────────────────────────────────────────── case "$MODE" in diff --git a/skaffold.yaml b/skaffold.yaml index d69c573..2afd4a4 100644 --- a/skaffold.yaml +++ b/skaffold.yaml @@ -148,3 +148,100 @@ deploy: recipePublisher.image: "{{.IMAGE_FULLY_QUALIFIED_recipe_publisher}}" opencodeSweAgent.image: "{{.IMAGE_FULLY_QUALIFIED_opencode_swe_agent}}" webSearch.image: "{{.IMAGE_FULLY_QUALIFIED_web_search}}" + +# ───────────────────────────────────────────────────────────────────────────── +# e2e profile: `skaffold run -p e2e` +# +# The default profile deliberately does NOT build integration-gateway (the +# minikube demo profile leaves it disabled, so there's nothing to run) — but +# the e2e suite's whole entry point is a GitHub webhook hitting that gateway. +# This profile adds the missing image and layers charts/*/values-e2e.yaml on +# top of the demo values. +# +# Kept as a profile rather than added to the default build so an ordinary +# `skaffold dev` doesn't pay to rebuild two extra images nobody's using. +profiles: + - name: e2e + build: + artifacts: + - image: agent-orchestrator + docker: + dockerfile: apps/agent-orchestrator/Dockerfile + - image: core-controller + context: controllers/core-controller + docker: + dockerfile: Dockerfile + - image: integration-gateway + docker: + dockerfile: apps/integration-gateway/Dockerfile + - image: opencode-swe-agent + docker: + dockerfile: apps/opencode-swe-agent/Dockerfile + # Needed by the community-components release below: its Tool/Agent CRs + # pin image tags, and a release deployed without freshly-built tags + # leaves AgentRun pods stuck in ImagePullBackOff on a stale tag that + # no longer exists in minikube's daemon. + - image: recipe-scraper + docker: + dockerfile: tools/recipe-scraper/Dockerfile + - image: recipe-publisher + docker: + dockerfile: tools/recipe-publisher/Dockerfile + - image: web-search + docker: + dockerfile: tools/web-search/Dockerfile + # The e2e triage route targets this agent (see + # charts/community-components/values-e2e.yaml): it declares the same + # Claude identityProviders as claude-code-swe-agent, so the credential + # gate behaves identically, but replies over NATS without a model call + # so a run can actually reach a terminal phase in a cluster holding no + # Anthropic credential. + - image: stub-agent + docker: + dockerfile: apps/stub-agent/Dockerfile + # Still built: its Agent CR stays enabled as the production-shaped + # reference the stub's providers are kept in step with, and its CR pins + # an image tag -- a release deployed without a freshly-built one leaves + # the tag dangling in minikube's daemon. + - image: claude-code-swe-agent + docker: + dockerfile: apps/claude-code-swe-agent/Dockerfile + deploy: + helm: + releases: + - name: agent-controller + chartPath: charts/agent-controller + namespace: controller-agent + createNamespace: true + wait: true + valuesFiles: + - charts/agent-controller/values-minikube-demo.yaml + - charts/agent-controller/values-e2e.yaml + skipBuildDependencies: false + setValueTemplates: + agent-orchestrator.image.repository: "{{.IMAGE_REPO_agent_orchestrator}}" + agent-orchestrator.image.tag: "{{.IMAGE_TAG_agent_orchestrator}}" + core-controller.image.repository: "{{.IMAGE_REPO_core_controller}}" + core-controller.image.tag: "{{.IMAGE_TAG_core_controller}}" + integration-gateway.image.repository: "{{.IMAGE_REPO_integration_gateway}}" + integration-gateway.image.tag: "{{.IMAGE_TAG_integration_gateway}}" + # A profile replaces deploy.helm.releases wholesale -- omitting this + # silently drops the chart that owns every Tool/Skill/Agent CR, so + # the cluster keeps whatever CRs an older install left behind, + # pointing at image tags that no longer exist. + - name: community-components + chartPath: charts/community-components + namespace: controller-agent + createNamespace: true + wait: true + valuesFiles: + - charts/community-components/values-minikube-demo.yaml + - charts/community-components/values-e2e.yaml + skipBuildDependencies: false + setValueTemplates: + stubAgent.image: "{{.IMAGE_FULLY_QUALIFIED_stub_agent}}" + claudeCodeSweAgent.image: "{{.IMAGE_FULLY_QUALIFIED_claude_code_swe_agent}}" + recipeScraper.image: "{{.IMAGE_FULLY_QUALIFIED_recipe_scraper}}" + recipePublisher.image: "{{.IMAGE_FULLY_QUALIFIED_recipe_publisher}}" + opencodeSweAgent.image: "{{.IMAGE_FULLY_QUALIFIED_opencode_swe_agent}}" + webSearch.image: "{{.IMAGE_FULLY_QUALIFIED_web_search}}"