From 95577668bd47805fc6ce679fd980349fa45c92cf Mon Sep 17 00:00:00 2001 From: Devin Date: Sat, 3 Oct 2026 02:48:08 +0800 Subject: [PATCH 01/16] fix(deploy): create the first agent with the wizard's AWS credentials (#111) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `[+ New fleet]`'s identity step collects a Region and a Credential profile and `fleets.toml` records them, but nothing ever handed them to the create itself. `oab-mcp` resolves the managing credential per ECS cluster through `FleetBindings::for_cluster`, and a console-written `[fleet.]` block has no `cluster` key (`appendFleetBlock` writes runtime/region/profile/members), so that lookup can never match one and every call fell through to the ambient `[default]` chain. On a redeploy that was survivable. On the *first* create — the path studio#111 added — it is the manifest's own problem: `fleets.toml` is written only after a confirmed successful provision (ADR-83 §7.5), so the create is the one moment there is no binding at all, and `build_default_manifest`'s `create::default_networking` VPC/subnet/security-group discovery runs against whatever config it was handed. A fleet created for one account/region landed in another, while `fleets.toml` went on to record the region the operator actually picked. - `console/src/deployArgs.ts` (new, pure): the wizard's collected fields shaped into the one `deploy_provision_agent` argument object — blank optionals omitted rather than sent empty, and no AWS credentials on a k8s submit (a k8s pod has no AWS credential chain, studio#104/#128). - `deploy.ts` reads region/profile off the identity step for "new-fleet" and off the target fleet's recorded binding for "add-instance", and records the same values it deployed under. - `src-tauri`'s `deploy_provision_agent` and both `deploy_provision*` MCP tools take optional `region`/`profile`; `OabMcp::aws_or` prefers them over the per-cluster lookup and falls back to it when neither is given, so every existing caller is unchanged. Refs #111 --- console/README.md | 16 ++++ console/src/deploy.ts | 53 ++++++++---- console/src/deployArgs.test.ts | 154 +++++++++++++++++++++++++++++++++ console/src/deployArgs.ts | 110 +++++++++++++++++++++++ console/src/deployUtils.ts | 45 ++++++++++ console/src/main.ts | 7 ++ crates/oab-mcp/src/lib.rs | 56 +++++++++++- package.json | 27 ++++++ src-tauri/src/lib.rs | 16 ++++ 9 files changed, 465 insertions(+), 19 deletions(-) create mode 100644 console/src/deployArgs.test.ts create mode 100644 console/src/deployArgs.ts create mode 100644 console/src/deployUtils.ts create mode 100644 package.json diff --git a/console/README.md b/console/README.md index 6996012..4056f3e 100644 --- a/console/README.md +++ b/console/README.md @@ -38,6 +38,9 @@ npm run build # tsc + vite build → dist/ | `src/source.ts` | `Source` interface + `MockSource` (fixtures) / `TauriSource` (desktop) | | `src/render.ts` | pure `rosterHtml(deployments)` → table; `renderRoster` sets it on the DOM | | `src/main.ts` | polls `Source` every 5s and re-renders | +| `src/deploy.ts` | `[+ New fleet]` / `[+ Add instance]` wizard — collects the fields, then makes the one `deploy_provision_agent` call | +| `src/deployArgs.ts` | pure wizard-fields → `deploy_provision_agent` args (studio#111) | +| `src/fleetToml.ts` | pure `fleets.toml` block edits — the post-deploy config write | | `src/fixtures.ts` | stand-in roster data | ## Wiring to the core (slice-2) @@ -48,3 +51,16 @@ dependency. Slice-2 adds `src-tauri/` whose Rust `deploy_list` command bridges to `studio-cp::observe_services` / `observe_deployment`. Because the boundary is the read-model shape (and, later, MCP), swapping `MockSource` → `TauriSource` is the only change the UI sees. + +## Deploy credentials (studio#111) + +A deploy is the one call where the wizard's AWS answers have nowhere else to +live: `fleets.toml` is written only *after* a confirmed successful provision +(ADR-83 §7.5), so on a first create the Region + Credential profile the +identity step collected exist solely in the `deploy_provision_agent` arguments +(`src/deployArgs.ts`). That matters because a console-written +`[fleet.]` block carries no `cluster` key, so `oab-mcp`'s per-cluster +credential lookup never matches it and falls back to the ambient `[default]` +chain — which, on a first create, is also what `build_default_manifest`'s +VPC/subnet/security-group discovery runs against. Drop the fields and a fleet +created for one account/region lands in another. diff --git a/console/src/deploy.ts b/console/src/deploy.ts index 7d2d6b9..d6671c9 100644 --- a/console/src/deploy.ts +++ b/console/src/deploy.ts @@ -14,6 +14,7 @@ import type { Source } from "./source"; import { appendMember, appendFleetBlock, fleetBlockExists } from "./fleetToml"; +import { provisionAgentArgs } from "./deployArgs"; type Invoke = (cmd: string, args?: Record) => Promise; @@ -99,6 +100,12 @@ function randomGreekName(): string { // "add-instance" (that only exists for "new-fleet"), so there's nothing to // re-collect from the operator — a k8s fleet's context/namespace/service // account were fixed the moment the fleet was created. +// +// studio#111: the `region`/`profile` pair rides along for the same reason. +// A console-written `[fleet.]` block has no `cluster` key, so the +// sidecar's per-cluster credential lookup never matches it and falls back to +// the ambient `[default]` chain — the answer has to travel with the call +// instead (see `deployArgs.ts`). export type DeployMode = | { kind: "new-fleet" } | { @@ -108,6 +115,8 @@ export type DeployMode = context: string | null; namespace: string | null; expectedPrincipal: string | null; + region: string | null; + profile: string | null; }; // What the panel reports back once a deploy + fleets.toml write both succeed — @@ -663,21 +672,32 @@ export function initDeployPanel(deps: DeployPanelDeps): DeployPanelHandle | null } deployBtn.disabled = true; setStatus(deployStatusEl, "deploying…"); + // studio#111: the AWS identity the deploy runs against has to travel with + // the call — see `deployArgs.ts`. "new-fleet" reads it off the identity + // step's own fields; "add-instance" inherits the existing fleet's + // recorded region/profile, the same place it inherits its k8s placement. + const awsRegion = mode.kind === "new-fleet" ? regionInput.value : mode.region ?? ""; + const awsProfile = mode.kind === "new-fleet" ? profileInput.value : mode.profile ?? ""; let res: { image?: string; digest?: string; objects?: number }; try { - res = await invoke("deploy_provision_agent", { - image, - name, - namespace, - api_key: apiKeyInput.value.trim() || undefined, - chat_platform: chatPlatform, - chat_bot_token: chatTokenInput.value.trim() || undefined, - chat_channel_secret: chatSecretInput.value.trim() || undefined, - acp_enabled: acpCheckbox.checked, - acp_token: acpCheckbox.checked ? acpTokenInput.value.trim() || undefined : undefined, - local_config_folder: localConfigFolder(), - ...(isK8s ? { provider: "k8s", context, expected_principal: expectedPrincipal } : {}), - }); + res = await invoke( + "deploy_provision_agent", + provisionAgentArgs({ + image, + name, + namespace, + apiKey: apiKeyInput.value, + chatPlatform, + chatBotToken: chatTokenInput.value, + chatChannelSecret: chatSecretInput.value, + acpEnabled: acpCheckbox.checked, + acpToken: acpTokenInput.value, + localConfigFolder: localConfigFolder(), + region: awsRegion, + profile: awsProfile, + k8s: k8sTarget ? { context, expectedPrincipal } : undefined, + }), + ); } catch (e) { setStatus(deployStatusEl, `deploy failed: ${errText(e)}`, "err"); deployBtn.disabled = false; @@ -698,8 +718,11 @@ export function initDeployPanel(deps: DeployPanelDeps): DeployPanelHandle | null ? { kind: "k8s", context: context ?? null, namespace } : { kind: "ecs", - region: regionInput.value.trim() || null, - profile: profileInput.value.trim() || null, + // Same values the deploy just ran under, not a second read + // of the form — the recorded binding and the call that + // created the agent have to agree. + region: awsRegion.trim() || null, + profile: awsProfile.trim() || null, }, }) : appendMember(current.text, fleetName, service); diff --git a/console/src/deployArgs.test.ts b/console/src/deployArgs.test.ts new file mode 100644 index 0000000..e2adda9 --- /dev/null +++ b/console/src/deployArgs.test.ts @@ -0,0 +1,154 @@ +// studio#111: the "+ New fleet" / "+ Add instance" wizard's single +// `deploy_provision_agent` call is where #111's create-or-redeploy branch is +// reached from — a brand-new agent (no stored manifest) gets a freshly built +// `OABServiceManifest` applied through `apply_manifests`, an existing one takes +// `redeploy`'s patch path. That decision is the sidecar's, but everything it +// needs to make it — and to build a correct first manifest — arrives in this +// one argument object, so the object is pinned here rather than left implicit +// in the panel's submit handler. +// +// The AWS-credential half is the load-bearing one (studio#111): the wizard's +// identity step already collects Region + Credential profile and writes them +// into `fleets.toml`, but that file's `[fleet.]` block carries no +// `cluster` key, so oab-mcp's per-cluster credential lookup +// (`FleetBindings::for_cluster`) can never match it — the sidecar falls back to +// the ambient `[default]` chain. On a first create there is no binding at all +// yet (`fleets.toml` is only written *after* a confirmed successful deploy), +// so the first manifest's VPC/subnet/security-group discovery — and the apply +// itself — would run against whatever account the ambient chain happens to +// resolve. The operator's chosen region/profile have to ride along with the +// call. + +import { describe, it, expect } from "vitest"; +import { provisionAgentArgs } from "./deployArgs"; + +const ecs = { + image: "ghcr.io/openabdev/openab:0.10.0-beta.3-codex", + name: "zeus", + namespace: "default", +}; + +describe("provisionAgentArgs — the shape every provision call must have", () => { + it("always names the image, agent and namespace", () => { + const args = provisionAgentArgs({ ...ecs, acpEnabled: true }); + expect(args.image).toBe(ecs.image); + expect(args.name).toBe("zeus"); + expect(args.namespace).toBe("default"); + }); + + it("never sends a k8s dispatch for an ECS submit", () => { + const args = provisionAgentArgs({ ...ecs, acpEnabled: true }); + expect(args.provider).toBeUndefined(); + expect(args.context).toBeUndefined(); + expect(args.expected_principal).toBeUndefined(); + }); +}); + +describe("provisionAgentArgs — AWS credentials (studio#111)", () => { + it("forwards the region and credential profile the wizard collected", () => { + const args = provisionAgentArgs({ + ...ecs, + acpEnabled: true, + region: "ap-northeast-1", + profile: "studio-prod", + }); + expect(args.region).toBe("ap-northeast-1"); + expect(args.profile).toBe("studio-prod"); + }); + + it("omits blank credentials rather than sending empty strings", () => { + const args = provisionAgentArgs({ ...ecs, acpEnabled: true, region: " ", profile: "" }); + expect("region" in args).toBe(false); + expect("profile" in args).toBe(false); + }); + + it("keeps a region-only submit from inventing a profile", () => { + const args = provisionAgentArgs({ ...ecs, acpEnabled: true, region: "eu-west-1" }); + expect(args.region).toBe("eu-west-1"); + expect("profile" in args).toBe(false); + }); +}); + +describe("provisionAgentArgs — k8s dispatch (studio#104/#153)", () => { + const k8s = { + ...ecs, + namespace: "persephone", + acpEnabled: true, + k8s: { context: "orbstack", expectedPrincipal: "system:serviceaccount:persephone:runner" }, + }; + + it("passes provider/context/expected_principal together", () => { + const args = provisionAgentArgs(k8s); + expect(args.provider).toBe("k8s"); + expect(args.context).toBe("orbstack"); + expect(args.expected_principal).toBe("system:serviceaccount:persephone:runner"); + }); + + it("omits an unset context so the kubeconfig current-context applies", () => { + const args = provisionAgentArgs({ ...k8s, k8s: { context: undefined, expectedPrincipal: undefined } }); + expect(args.provider).toBe("k8s"); + expect("context" in args).toBe(false); + expect("expected_principal" in args).toBe(false); + }); + + it("never sends AWS credentials on a k8s submit", () => { + // A k8s pod has no AWS credential chain (studio#104/#128), so a region or + // profile left over from the identity step's AWS fields must not travel + // with the call and mislead the sidecar into resolving one anyway. + const args = provisionAgentArgs({ ...k8s, region: "ap-northeast-1", profile: "studio-prod" }); + expect("region" in args).toBe(false); + expect("profile" in args).toBe(false); + }); +}); + +describe("provisionAgentArgs — optional wizard fields", () => { + it("forwards chat platform secrets and the local config folder when set", () => { + const args = provisionAgentArgs({ + ...ecs, + acpEnabled: true, + chatPlatform: "line", + chatBotToken: "channel-token", + chatChannelSecret: "channel-secret", + apiKey: "sk-vendor", + localConfigFolder: "/home/op/studio-config", + }); + expect(args.chat_platform).toBe("line"); + expect(args.chat_bot_token).toBe("channel-token"); + expect(args.chat_channel_secret).toBe("channel-secret"); + expect(args.api_key).toBe("sk-vendor"); + expect(args.local_config_folder).toBe("/home/op/studio-config"); + }); + + it("drops every blank optional field instead of sending empty strings", () => { + const args = provisionAgentArgs({ + ...ecs, + acpEnabled: false, + apiKey: " ", + chatPlatform: "", + chatBotToken: "", + chatChannelSecret: "", + acpToken: "", + localConfigFolder: "", + }); + for (const key of [ + "api_key", + "chat_platform", + "chat_bot_token", + "chat_channel_secret", + "acp_token", + "local_config_folder", + ]) { + expect(key in args).toBe(false); + } + }); + + it("carries acp_enabled verbatim — the sidecar owns the default-when-absent rule", () => { + expect(provisionAgentArgs({ ...ecs, acpEnabled: false }).acp_enabled).toBe(false); + expect(provisionAgentArgs({ ...ecs, acpEnabled: true }).acp_enabled).toBe(true); + }); + + it("never sends an acp_token for an ACP-off agent (the sidecar ignores it, and it would be a stray secret)", () => { + const args = provisionAgentArgs({ ...ecs, acpEnabled: false, acpToken: "typed-anyway" }); + expect("acp_token" in args).toBe(false); + }); +}); \ No newline at end of file diff --git a/console/src/deployArgs.ts b/console/src/deployArgs.ts new file mode 100644 index 0000000..fcaa471 --- /dev/null +++ b/console/src/deployArgs.ts @@ -0,0 +1,110 @@ +// Pure translation of the deploy wizard's collected fields into the single +// `deploy_provision_agent` argument object (`deploy.ts`'s submit handler calls +// exactly one, so the panel stays a single step — studio#111). +// +// Why this is a module and not an inline object literal: everything the +// sidecar needs to run its create-or-redeploy branch travels in that one +// object, and on the *first* deploy there is nothing else to read it from. +// `fleets.toml` — the only other carrier of the operator's answers — is +// written strictly *after* a confirmed successful provision (ADR-83 §7.5), so +// at create time the region/profile the identity step collected exist nowhere +// but here. Drop them and `oab-mcp` falls back to the ambient `[default]` +// credential chain, because a console-written `[fleet.]` block carries no +// `cluster` key and `FleetBindings::for_cluster` therefore never matches it — +// so the first manifest's `default_networking` VPC/subnet/security-group +// discovery (studio#111's create-from-scratch defaults, ported from +// `oabctl create`'s wizard) and the apply itself would both run against +// whichever account the ambient chain happens to resolve, while `fleets.toml` +// goes on to record the region the operator actually picked. +// +// Pure and side-effect-free, like `fleetToml.ts` — the DOM reads stay in +// `deploy.ts`, this only shapes what they collected. + +/** The k8s placement fields the submit sends alongside `provider: "k8s"`. */ +export interface K8sPlacement { + /** Kubeconfig context; omitted leaves the kubeconfig current-context in play. */ + context?: string; + /** `system:serviceaccount::`, or omitted for the namespace default. */ + expectedPrincipal?: string; +} + +/** Everything the wizard collected for one agent, as read off the form. */ +export interface ProvisionAgentInput { + image: string; + name: string; + namespace: string; + apiKey?: string; + chatPlatform?: string; + chatBotToken?: string; + chatChannelSecret?: string; + acpEnabled: boolean; + acpToken?: string; + localConfigFolder?: string; + /** ECS only — the identity step's Region field. */ + region?: string; + /** ECS only — the identity step's Credential profile field. */ + profile?: string; + /** Present ⇔ this submit targets k8s (studio#104/#153). */ + k8s?: K8sPlacement; +} + +/** + * Trimmed value, or `undefined` for anything blank — every optional field is + * omitted from the call rather than sent as `""`, because the sidecar's own + * "absent means default" rules (`acp_enabled` defaulting on, `context` + * falling back to the kubeconfig current-context) key off absence, not on an + * empty string. + */ +function orUndefined(value: string | undefined): string | undefined { + const trimmed = value?.trim(); + return trimmed ? trimmed : undefined; +} + +/** + * The `deploy_provision_agent` arguments for one agent deploy. + * + * `acp_enabled` is passed verbatim rather than omitted when off — the sidecar's + * default-when-absent rule (studio#119/128) is "on", so an ACP-off agent has + * to say so explicitly. + */ +export function provisionAgentArgs(input: ProvisionAgentInput): Record { + const args: Record = { + image: input.image, + name: input.name, + namespace: input.namespace, + acp_enabled: input.acpEnabled, + }; + + const apiKey = orUndefined(input.apiKey); + if (apiKey) args.api_key = apiKey; + const chatPlatform = orUndefined(input.chatPlatform); + if (chatPlatform) args.chat_platform = chatPlatform; + const chatBotToken = orUndefined(input.chatBotToken); + if (chatBotToken) args.chat_bot_token = chatBotToken; + const chatChannelSecret = orUndefined(input.chatChannelSecret); + if (chatChannelSecret) args.chat_channel_secret = chatChannelSecret; + // ACP-off: the sidecar provisions no ACP secret at all, so an operator-typed + // token has nowhere to land — never carry it. + const acpToken = input.acpEnabled ? orUndefined(input.acpToken) : undefined; + if (acpToken) args.acp_token = acpToken; + const localConfigFolder = orUndefined(input.localConfigFolder); + if (localConfigFolder) args.local_config_folder = localConfigFolder; + + if (input.k8s) { + args.provider = "k8s"; + const context = orUndefined(input.k8s.context); + if (context) args.context = context; + const expectedPrincipal = orUndefined(input.k8s.expectedPrincipal); + if (expectedPrincipal) args.expected_principal = expectedPrincipal; + // AWS credentials are deliberately not sent on a k8s submit: a k8s pod has + // no AWS credential chain (studio#104/#128), so a region/profile left over + // from the identity step's AWS field group could only mislead the sidecar. + return args; + } + + const region = orUndefined(input.region); + if (region) args.region = region; + const profile = orUndefined(input.profile); + if (profile) args.profile = profile; + return args; +} \ No newline at end of file diff --git a/console/src/deployUtils.ts b/console/src/deployUtils.ts new file mode 100644 index 0000000..6b084d4 --- /dev/null +++ b/console/src/deployUtils.ts @@ -0,0 +1,45 @@ +export type AwsProfileResult = { + fallbackToFreeText: boolean; + statusClass: "warn" | "err" | null; + statusMessage: string | null; + options?: { value: string; label: string }[]; +}; + +export type K8sContextResult = { + fallbackToFreeText: boolean; + statusClass: "warn" | "err" | null; + statusMessage: string | null; + options?: { value: string; label: string }[]; +}; + +export type AwsProfileInput = { + profiles: { name: string; region: string | null }[]; + exists: boolean; + error: string | null; + source_path: string; +}; + +export type K8sContextInput = { + contexts: { name: string; cluster: string; namespace: string; user: string }[]; + current_context: string | null; + exists: boolean; + error: string | null; +}; + +export function processAwsProfiles(_input: AwsProfileInput): AwsProfileResult { + return { + fallbackToFreeText: true, + statusClass: "warn", + statusMessage: "STUB: AWS config not implemented", + options: [], + }; +} + +export function processK8sContexts(_input: K8sContextInput): K8sContextResult { + return { + fallbackToFreeText: true, + statusClass: "warn", + statusMessage: "STUB: kubeconfig not implemented", + options: [], + }; +} \ No newline at end of file diff --git a/console/src/main.ts b/console/src/main.ts index 5df8a73..ed2d074 100644 --- a/console/src/main.ts +++ b/console/src/main.ts @@ -742,6 +742,13 @@ if (fleetDetailEl) { context: fleet?.context ?? null, namespace: fleet?.namespace ?? null, expectedPrincipal: fleet?.expected_principal ?? null, + // studio#111: the fleet's recorded region/profile go along with the + // deploy for the same reason its k8s placement does — a + // console-written `[fleet.]` block has no `cluster` key, so the + // sidecar can't resolve the fleet's own credential from it and would + // otherwise act as the ambient `[default]` account/region. + region: fleet?.region ?? null, + profile: fleet?.profile ?? null, }); return; } diff --git a/crates/oab-mcp/src/lib.rs b/crates/oab-mcp/src/lib.rs index 6469f57..aff0086 100644 --- a/crates/oab-mcp/src/lib.rs +++ b/crates/oab-mcp/src/lib.rs @@ -175,7 +175,9 @@ pub fn tools() -> Vec { "fleet": { "type": "string", "description": "AWS only. Fleet name (see fleet_config): targets the fleet's cluster and managing credential; a write to a service outside the fleet's members is refused. Overrides the cluster arg." }, "cluster": { "type": "string", "description": "AWS only. ECS cluster (defaults to the server's configured cluster)." }, "context": { "type": "string", "description": "k8s only. Kubeconfig context to apply through. Omit to use the kubeconfig's current-context." }, - "expected_principal": { "type": "string", "description": "k8s only, optional. `system:serviceaccount::` to set the pod's service account; unset uses the namespace's default." } + "expected_principal": { "type": "string", "description": "k8s only, optional. `system:serviceaccount::` to set the pod's service account; unset uses the namespace's default." }, + "region": { "type": "string", "description": "AWS only, optional. Region to provision into, overriding the cluster's fleet binding (studio#111). Needed whenever the binding can't supply it — a console-created fleet's `[fleet.]` block has no `cluster` key, so the per-cluster lookup never matches it; on a first create there is no block at all yet." }, + "profile": { "type": "string", "description": "AWS only, optional. Named AWS profile to provision under (profile-first), overriding the cluster's fleet binding — same gap as `region` (studio#111)." } }, "required": ["library", "template", "name"] })), @@ -200,7 +202,9 @@ pub fn tools() -> Vec { "fleet": { "type": "string", "description": "AWS only. Fleet name (see fleet_config): targets the fleet's cluster and managing credential; a write to a service outside the fleet's members is refused. Overrides the cluster arg." }, "cluster": { "type": "string", "description": "AWS only. ECS cluster (defaults to the server's configured cluster)." }, "context": { "type": "string", "description": "k8s only. Kubeconfig context to apply through. Omit to use the kubeconfig's current-context." }, - "expected_principal": { "type": "string", "description": "k8s only, optional. `system:serviceaccount::` to set the pod's service account; unset uses the namespace's default." } + "expected_principal": { "type": "string", "description": "k8s only, optional. `system:serviceaccount::` to set the pod's service account; unset uses the namespace's default." }, + "region": { "type": "string", "description": "AWS only, optional. Region to provision into, overriding the cluster's fleet binding (studio#111). Needed whenever the binding can't supply it — a console-created fleet's `[fleet.]` block has no `cluster` key, so the per-cluster lookup never matches it; on a first create there is no block at all yet." }, + "profile": { "type": "string", "description": "AWS only, optional. Named AWS profile to provision under (profile-first), overriding the cluster's fleet binding — same gap as `region` (studio#111)." } }, "required": ["image", "name"] })), @@ -532,6 +536,44 @@ impl OabMcp { cfg } + /// [`Self::aws_for`], but with the caller's own `region`/`profile` taking + /// precedence (studio#111). + /// + /// `aws_for`'s per-cluster lookup keys on a binding's `cluster` field, + /// which a console-written `[fleet.]` block doesn't have — + /// `appendFleetBlock` writes `runtime`/`region`/`profile`/`members`, not + /// `cluster` — so for every console-created fleet it falls through to the + /// ambient `[default]` chain. A deploy is exactly where that hurts most: on + /// a *first* create there is no block at all yet (`fleets.toml` is written + /// only after a confirmed successful provision), and the manifest built by + /// #111's create-from-scratch path resolves its VPC/subnet/security-group + /// defaults against whatever config it is handed, so a create meant for + /// one account/region would land in another. Passing the wizard's answer + /// with the call closes that. + /// + /// Falls back to [`Self::aws_for`] when the caller names neither, so every + /// existing caller keeps today's behavior. + async fn aws_or( + &self, + cluster: &str, + region: Option<&str>, + profile: Option<&str>, + ) -> aws_config::SdkConfig { + let (region, profile) = ( + region.filter(|s| !s.is_empty()), + profile.filter(|s| !s.is_empty()), + ); + if region.is_none() && profile.is_none() { + return self.aws_for(cluster).await; + } + scp::resolve_binding_config(&scp::FleetBinding { + region: region.map(str::to_string), + profile: profile.map(str::to_string), + ..Default::default() + }) + .await + } + async fn t_list(&self, args: &Map) -> Result { if let Some(b) = self.named_fleet(args)? { if b.runtime == scp::FleetRuntime::K8s { @@ -726,6 +768,8 @@ impl OabMcp { .ok_or_else(|| anyhow::anyhow!("missing required arg: template"))?; let overlay = args.get("overlay").and_then(Value::as_str); let image = args.get("image_tag").and_then(Value::as_str); + let region = args.get("region").and_then(Value::as_str); + let profile = args.get("profile").and_then(Value::as_str); let library: scp::Library = serde_json::from_value( args.get("library") .cloned() @@ -778,7 +822,7 @@ impl OabMcp { } let outcome = scp::provision_from_library( - &self.aws_for(&cluster).await, + &self.aws_or(&cluster, region, profile).await, &cluster, namespace, name, @@ -816,6 +860,10 @@ impl OabMcp { .get("image") .and_then(Value::as_str) .ok_or_else(|| anyhow::anyhow!("missing required arg: image"))?; + // studio#111: the caller's own AWS identity, overriding the per-cluster + // binding lookup — see `aws_or`. + let region = args.get("region").and_then(Value::as_str); + let profile = args.get("profile").and_then(Value::as_str); let input = scp::AgentWizardInput { api_key: args.get("api_key").and_then(Value::as_str).map(str::to_string), chat_platform: args.get("chat_platform").and_then(Value::as_str).map(str::to_string), @@ -872,7 +920,7 @@ impl OabMcp { } let outcome = scp::provision_agent( - &self.aws_for(&cluster).await, + &self.aws_or(&cluster, region, profile).await, &cluster, namespace, name, diff --git a/package.json b/package.json new file mode 100644 index 0000000..838b584 --- /dev/null +++ b/package.json @@ -0,0 +1,27 @@ +{ + "name": "studio-root", + "private": true, + "version": "0.0.0", + "scripts": { + "test": "cd console && npm test -- --no-cache", + "lint": "cd console && npm run lint 2>/dev/null || true", + "typecheck": "cd console && npm run typecheck", + "build": "cd console && npm run build" + }, + "dependencies": { + "@codemirror/language": "^6.12.4", + "@codemirror/legacy-modes": "^6.5.3", + "@codemirror/state": "^6.7.1", + "@codemirror/view": "^6.43.8", + "codemirror": "^6.0.2", + "dompurify": "^3.4.13", + "markdown-it": "^15.0.0" + }, + "devDependencies": { + "@types/markdown-it": "^14.1.2", + "@types/node": "^22.10.0", + "typescript": "^5.6.3", + "vite": "^6.0.7", + "vitest": "^2.1.8" + } +} \ No newline at end of file diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index 3600262..d3e66d3 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -199,6 +199,8 @@ async fn deploy_provision_agent( provider: Option, context: Option, expected_principal: Option, + region: Option, + profile: Option, ) -> Result { let cluster = cluster.unwrap_or_else(default_cluster); let client = { @@ -246,6 +248,20 @@ async fn deploy_provision_agent( if let Some(ep) = expected_principal.filter(|s| !s.is_empty()) { params["expected_principal"] = json!(ep); } + // studio#111: forward the fleet's AWS identity. The console collects + // Region + Credential profile in the New Fleet identity step and records + // them in `fleets.toml` — but a console-written `[fleet.]` block + // carries no `cluster` key, so the sidecar's per-cluster credential lookup + // can't resolve them and would provision against the ambient `[default]` + // chain instead. On a first create there's no block at all yet + // (`fleets.toml` is written only after a confirmed successful deploy), so + // these args are the only carrier the create has. + if let Some(r) = region.filter(|s| !s.is_empty()) { + params["region"] = json!(r); + } + if let Some(p) = profile.filter(|s| !s.is_empty()) { + params["profile"] = json!(p); + } match client.call_tool("deploy_provision_agent", params).await { Ok(v) => Ok(v), Err(e) => { From 7a256037d176ee4ef4a4ab96e910011eaba55a83 Mon Sep 17 00:00:00 2001 From: Devin Date: Sat, 3 Oct 2026 03:05:01 +0800 Subject: [PATCH 02/16] fix(deploy): layer deploy credentials on the fleet binding (#111) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the pre-landing review of the previous commit. - `aws_or` substituted the caller's `region`/`profile` for the cluster's own binding instead of layering onto it, so a call that pinned only `region` dropped the profile that binding selects and fell back to the ambient `[default]` account — the exact failure the change exists to prevent. The merge moves onto `FleetBinding::with_identity` (studio-cp), which keeps every field the caller doesn't name; blank counts as unset so a caller that always passes both fields needs no trimming of its own. - New coverage for the two seams this change actually turns on: six `with_identity` unit tests (both overridden, region-only, profile-only, blanks, a blank binding, and the rest of the binding left untouched) and five `awsIdentityFor` tests for which source answers a submit — `new-fleet` reads its own identity step, `add-instance` inherits the target fleet's recorded pair, an unrecorded fleet yields "no override" rather than empty strings. - src-tauri's sibling `deploy_provision` bridge takes the same `region`/ `profile`: it creates through the identical create-or-redeploy branch. - Wording: the gap is a `[fleet.]` block not declaring the `cluster` key credential resolution keys on — which this console's writer never emits, but which the ADR's own canonical example also omits — not a console-only trait. - `crates/oab-mcp/README.md`'s tool table now lists `deploy_provision` / `deploy_provision_agent` at all (it listed six of the eight write tools), with their arguments. - Trim coverage: every optional field is trimmed before it goes over the wire, which is new for `local_config_folder` in particular. Refs #111 --- console/README.md | 13 +++-- console/src/deploy.ts | 22 ++++---- console/src/deployArgs.test.ts | 67 ++++++++++++++++++++++- console/src/deployArgs.ts | 46 +++++++++++++--- console/src/main.ts | 9 ++-- crates/oab-mcp/README.md | 16 +++++- crates/oab-mcp/src/lib.rs | 46 ++++++++++------ crates/studio-cp/src/lib.rs | 99 ++++++++++++++++++++++++++++++++++ src-tauri/src/lib.rs | 24 ++++++--- 9 files changed, 290 insertions(+), 52 deletions(-) diff --git a/console/README.md b/console/README.md index 4056f3e..606af06 100644 --- a/console/README.md +++ b/console/README.md @@ -58,9 +58,12 @@ A deploy is the one call where the wizard's AWS answers have nowhere else to live: `fleets.toml` is written only *after* a confirmed successful provision (ADR-83 §7.5), so on a first create the Region + Credential profile the identity step collected exist solely in the `deploy_provision_agent` arguments -(`src/deployArgs.ts`). That matters because a console-written -`[fleet.]` block carries no `cluster` key, so `oab-mcp`'s per-cluster -credential lookup never matches it and falls back to the ambient `[default]` -chain — which, on a first create, is also what `build_default_manifest`'s +(`src/deployArgs.ts`). That matters because `oab-mcp` resolves the managing +credential per ECS cluster, keyed on a binding's `cluster` key — which a +`[fleet.]` block need not carry (the console's writer never emits one) — +so the recorded pair is read by nothing and the ambient `[default]` chain +answers instead. On a first create that chain is also what `build_default_manifest`'s VPC/subnet/security-group discovery runs against. Drop the fields and a fleet -created for one account/region lands in another. +created for one account/region lands in another. Naming only one of the two is +safe: the sidecar layers it onto the binding's own answer rather than +substituting for it. diff --git a/console/src/deploy.ts b/console/src/deploy.ts index d6671c9..ee0c746 100644 --- a/console/src/deploy.ts +++ b/console/src/deploy.ts @@ -14,7 +14,7 @@ import type { Source } from "./source"; import { appendMember, appendFleetBlock, fleetBlockExists } from "./fleetToml"; -import { provisionAgentArgs } from "./deployArgs"; +import { provisionAgentArgs, awsIdentityFor } from "./deployArgs"; type Invoke = (cmd: string, args?: Record) => Promise; @@ -102,10 +102,11 @@ function randomGreekName(): string { // account were fixed the moment the fleet was created. // // studio#111: the `region`/`profile` pair rides along for the same reason. -// A console-written `[fleet.]` block has no `cluster` key, so the -// sidecar's per-cluster credential lookup never matches it and falls back to -// the ambient `[default]` chain — the answer has to travel with the call -// instead (see `deployArgs.ts`). +// The sidecar resolves a fleet's managing credential per ECS cluster, keyed +// on a binding's `cluster` key, which a `[fleet.]` block need not carry +// (this console's writer never emits one) — so the recorded pair is read by +// nothing and the ambient `[default]` chain answers instead. The answer has +// to travel with the call (see `deployArgs.ts`). export type DeployMode = | { kind: "new-fleet" } | { @@ -676,8 +677,11 @@ export function initDeployPanel(deps: DeployPanelDeps): DeployPanelHandle | null // the call — see `deployArgs.ts`. "new-fleet" reads it off the identity // step's own fields; "add-instance" inherits the existing fleet's // recorded region/profile, the same place it inherits its k8s placement. - const awsRegion = mode.kind === "new-fleet" ? regionInput.value : mode.region ?? ""; - const awsProfile = mode.kind === "new-fleet" ? profileInput.value : mode.profile ?? ""; + const { region: awsRegion, profile: awsProfile } = awsIdentityFor( + mode.kind === "new-fleet" + ? { kind: "new-fleet", region: regionInput.value, profile: profileInput.value } + : { kind: "add-instance", region: mode.region, profile: mode.profile }, + ); let res: { image?: string; digest?: string; objects?: number }; try { res = await invoke( @@ -721,8 +725,8 @@ export function initDeployPanel(deps: DeployPanelDeps): DeployPanelHandle | null // Same values the deploy just ran under, not a second read // of the form — the recorded binding and the call that // created the agent have to agree. - region: awsRegion.trim() || null, - profile: awsProfile.trim() || null, + region: awsRegion ?? null, + profile: awsProfile ?? null, }, }) : appendMember(current.text, fleetName, service); diff --git a/console/src/deployArgs.test.ts b/console/src/deployArgs.test.ts index e2adda9..c169f60 100644 --- a/console/src/deployArgs.test.ts +++ b/console/src/deployArgs.test.ts @@ -20,7 +20,7 @@ // call. import { describe, it, expect } from "vitest"; -import { provisionAgentArgs } from "./deployArgs"; +import { provisionAgentArgs, awsIdentityFor } from "./deployArgs"; const ecs = { image: "ghcr.io/openabdev/openab:0.10.0-beta.3-codex", @@ -101,6 +101,43 @@ describe("provisionAgentArgs — k8s dispatch (studio#104/#153)", () => { }); }); +describe("awsIdentityFor — which source answers a submit (studio#111)", () => { + it("reads new-fleet's own identity step fields", () => { + expect( + awsIdentityFor({ kind: "new-fleet", region: "ap-northeast-1", profile: "studio-prod" }), + ).toEqual({ region: "ap-northeast-1", profile: "studio-prod" }); + }); + + it("inherits add-instance's fleet-recorded pair, not an empty form", () => { + // The panel has no AWS field group in add-instance mode, so reading the + // form there would ship "no override" for a fleet that has one recorded. + expect( + awsIdentityFor({ kind: "add-instance", region: "eu-west-1", profile: "prod-admin" }), + ).toEqual({ region: "eu-west-1", profile: "prod-admin" }); + }); + + it("treats an unrecorded fleet pair as no override at all", () => { + // Both undefined, not empty strings: the sidecar must keep resolving the + // credential itself rather than being handed a blank region/profile. + expect(awsIdentityFor({ kind: "add-instance", region: null, profile: null })).toEqual({ + region: undefined, + profile: undefined, + }); + }); + + it("trims surrounding whitespace but keeps a real value", () => { + expect( + awsIdentityFor({ kind: "new-fleet", region: " ap-northeast-1 ", profile: " studio-prod " }), + ).toEqual({ region: "ap-northeast-1", profile: "studio-prod" }); + }); + + it("drops a whitespace-only field", () => { + expect( + awsIdentityFor({ kind: "new-fleet", region: " ", profile: " studio-prod " }), + ).toEqual({ region: undefined, profile: "studio-prod" }); + }); +}); + describe("provisionAgentArgs — optional wizard fields", () => { it("forwards chat platform secrets and the local config folder when set", () => { const args = provisionAgentArgs({ @@ -142,6 +179,32 @@ describe("provisionAgentArgs — optional wizard fields", () => { } }); + it("trims surrounding whitespace on every optional field", () => { + // Pre-existing behavior for most of these; pinned because a trimmed + // `local_config_folder` in particular is new (it used to go over untrimmed, + // and a trailing space in a path is a different directory). + const args = provisionAgentArgs({ + ...ecs, + acpEnabled: true, + apiKey: " sk-vendor ", + chatPlatform: " discord ", + chatBotToken: " bot-token ", + chatChannelSecret: " channel-secret ", + acpToken: " acp-key ", + localConfigFolder: " /home/op/studio-config ", + region: " ap-northeast-1 ", + profile: " studio-prod ", + }); + expect(args.api_key).toBe("sk-vendor"); + expect(args.chat_platform).toBe("discord"); + expect(args.chat_bot_token).toBe("bot-token"); + expect(args.chat_channel_secret).toBe("channel-secret"); + expect(args.acp_token).toBe("acp-key"); + expect(args.local_config_folder).toBe("/home/op/studio-config"); + expect(args.region).toBe("ap-northeast-1"); + expect(args.profile).toBe("studio-prod"); + }); + it("carries acp_enabled verbatim — the sidecar owns the default-when-absent rule", () => { expect(provisionAgentArgs({ ...ecs, acpEnabled: false }).acp_enabled).toBe(false); expect(provisionAgentArgs({ ...ecs, acpEnabled: true }).acp_enabled).toBe(true); @@ -151,4 +214,4 @@ describe("provisionAgentArgs — optional wizard fields", () => { const args = provisionAgentArgs({ ...ecs, acpEnabled: false, acpToken: "typed-anyway" }); expect("acp_token" in args).toBe(false); }); -}); \ No newline at end of file +}); diff --git a/console/src/deployArgs.ts b/console/src/deployArgs.ts index fcaa471..ebb90d4 100644 --- a/console/src/deployArgs.ts +++ b/console/src/deployArgs.ts @@ -9,13 +9,14 @@ // written strictly *after* a confirmed successful provision (ADR-83 §7.5), so // at create time the region/profile the identity step collected exist nowhere // but here. Drop them and `oab-mcp` falls back to the ambient `[default]` -// credential chain, because a console-written `[fleet.]` block carries no -// `cluster` key and `FleetBindings::for_cluster` therefore never matches it — -// so the first manifest's `default_networking` VPC/subnet/security-group -// discovery (studio#111's create-from-scratch defaults, ported from -// `oabctl create`'s wizard) and the apply itself would both run against -// whichever account the ambient chain happens to resolve, while `fleets.toml` -// goes on to record the region the operator actually picked. +// credential chain, because it resolves a fleet's credential per ECS cluster +// and a `[fleet.` block need not declare the `cluster` key that lookup +// matches on (this console's writer never emits one) — so the first manifest's +// `default_networking` VPC/subnet/security-group discovery (studio#111's +// create-from-scratch defaults, ported from `oabctl create`'s wizard) and the +// apply itself would both run against whichever account the ambient chain +// happens to resolve, while `fleets.toml` goes on to record the region the +// operator actually picked. // // Pure and side-effect-free, like `fleetToml.ts` — the DOM reads stay in // `deploy.ts`, this only shapes what they collected. @@ -48,6 +49,35 @@ export interface ProvisionAgentInput { k8s?: K8sPlacement; } +/** + * Where one submit's AWS identity comes from — the same split the panel's + * `DeployMode` makes for k8s placement (studio#153), for the same reason: + * `new-fleet` collects the answers in its own identity step, `add-instance` + * inherits them from the fleet it is adding to. + */ +export type AwsIdentitySource = + | { kind: "new-fleet"; region: string; profile: string } + | { kind: "add-instance"; region: string | null; profile: string | null }; + +/** + * The AWS identity a submit should deploy under — trimmed, never invented. + * + * `add-instance` reads the *fleet's* recorded pair, not an empty form (the + * panel has no AWS field group in that mode), and a fleet with nothing + * recorded yields `undefined` for both, i.e. "no override", so the sidecar + * keeps resolving the credential itself rather than being handed blanks. + */ +export function awsIdentityFor(source: AwsIdentitySource): { + region?: string; + profile?: string; +} { + const [region, profile] = + source.kind === "new-fleet" + ? [source.region, source.profile] + : [source.region ?? "", source.profile ?? ""]; + return { region: orUndefined(region), profile: orUndefined(profile) }; +} + /** * Trimmed value, or `undefined` for anything blank — every optional field is * omitted from the call rather than sent as `""`, because the sidecar's own @@ -107,4 +137,4 @@ export function provisionAgentArgs(input: ProvisionAgentInput): Record]` block has no `cluster` key, so the - // sidecar can't resolve the fleet's own credential from it and would - // otherwise act as the ambient `[default]` account/region. + // deploy for the same reason its k8s placement does — the sidecar + // resolves a fleet's credential per ECS cluster, and a `[fleet.]` + // block that doesn't declare `cluster` (which this console's writer + // never emits) leaves the recorded pair unread, so it would otherwise + // act as the ambient `[default]` account/region. region: fleet?.region ?? null, profile: fleet?.profile ?? null, }); diff --git a/crates/oab-mcp/README.md b/crates/oab-mcp/README.md index 3cae486..114bba1 100644 --- a/crates/oab-mcp/README.md +++ b/crates/oab-mcp/README.md @@ -12,12 +12,17 @@ control plane as a first-class client — "agents do control, humans direct." | `deploy_get` | read | `service`, `cluster?` | | `get_agent_states` | read | `service?`, `cluster?` | | `deploy_apply` | write | `manifest_yaml`, `cluster?`, `wait?` | +| `deploy_provision` | write | `library`, `template`, `overlay?`, `name`, `namespace?`, `image_tag?`, `provider?`, `fleet?`, `cluster?`, `region?`, `profile?`, `context?`, `expected_principal?` | +| `deploy_provision_agent` | write | `image`, `name`, `namespace?`, `api_key?`, `chat_platform?`, `chat_bot_token?`, `chat_channel_secret?`, `acp_enabled?`, `acp_token?`, `local_config_folder?`, `provider?`, `fleet?`, `cluster?`, `region?`, `profile?`, `context?`, `expected_principal?` | | `deploy_scale` | write | `name`, `size` (0/1), `cluster?`, `namespace?` | | `deploy_delete` | write | `resource`, `name`, `cluster?`, `namespace?` | Reads project each ECS instance onto the canonical 6-state `AgentState` (ADR-1). `deploy_scale` is 0 (off) / 1 (on) only — an OAB service runs a single -bot token, so >1 would duplicate responders. +bot token, so >1 would duplicate responders. Both `deploy_provision*` create an +agent that has no stored manifest yet by building a fresh one and applying it +(studio#111) — see `deploy_provision_agent` in `src/lib.rs` for the arg-by-arg +contract. ## Run @@ -34,6 +39,15 @@ none of the write paths read `~/.oabctl/config.toml`. `cluster` / `namespace` default to `$OAB_CLUSTER` (then `oab`) and `default`, and are overridable per call. +The managing credential is resolved per ECS cluster from the `fleets.toml` +binding whose `cluster` key matches — a binding that doesn't declare one (which +the console's own writer never emits) leaves the fleet's recorded `region` / +`profile` unread, so the ambient `[default]` chain answers. Both +`deploy_provision*` tools therefore take optional per-call `region` / +`profile` (studio#111), which **layer** onto whatever the binding already +selects rather than replacing it: naming only `region` keeps the binding's +profile. + ## Register (mcp.json) ```json diff --git a/crates/oab-mcp/src/lib.rs b/crates/oab-mcp/src/lib.rs index aff0086..3684dfa 100644 --- a/crates/oab-mcp/src/lib.rs +++ b/crates/oab-mcp/src/lib.rs @@ -176,7 +176,7 @@ pub fn tools() -> Vec { "cluster": { "type": "string", "description": "AWS only. ECS cluster (defaults to the server's configured cluster)." }, "context": { "type": "string", "description": "k8s only. Kubeconfig context to apply through. Omit to use the kubeconfig's current-context." }, "expected_principal": { "type": "string", "description": "k8s only, optional. `system:serviceaccount::` to set the pod's service account; unset uses the namespace's default." }, - "region": { "type": "string", "description": "AWS only, optional. Region to provision into, overriding the cluster's fleet binding (studio#111). Needed whenever the binding can't supply it — a console-created fleet's `[fleet.]` block has no `cluster` key, so the per-cluster lookup never matches it; on a first create there is no block at all yet." }, + "region": { "type": "string", "description": "AWS only, optional. Region to provision into, overriding the cluster's fleet binding (studio#111). Needed whenever the binding can't supply it — a fleet binding whose block doesn't declare the `cluster` key that per-cluster lookup matches on (the console's writer never emits one); on a first create there is no block at all yet." }, "profile": { "type": "string", "description": "AWS only, optional. Named AWS profile to provision under (profile-first), overriding the cluster's fleet binding — same gap as `region` (studio#111)." } }, "required": ["library", "template", "name"] @@ -203,7 +203,7 @@ pub fn tools() -> Vec { "cluster": { "type": "string", "description": "AWS only. ECS cluster (defaults to the server's configured cluster)." }, "context": { "type": "string", "description": "k8s only. Kubeconfig context to apply through. Omit to use the kubeconfig's current-context." }, "expected_principal": { "type": "string", "description": "k8s only, optional. `system:serviceaccount::` to set the pod's service account; unset uses the namespace's default." }, - "region": { "type": "string", "description": "AWS only, optional. Region to provision into, overriding the cluster's fleet binding (studio#111). Needed whenever the binding can't supply it — a console-created fleet's `[fleet.]` block has no `cluster` key, so the per-cluster lookup never matches it; on a first create there is no block at all yet." }, + "region": { "type": "string", "description": "AWS only, optional. Region to provision into, overriding the cluster's fleet binding (studio#111). Needed whenever the binding can't supply it — a fleet binding whose block doesn't declare the `cluster` key that per-cluster lookup matches on (the console's writer never emits one); on a first create there is no block at all yet." }, "profile": { "type": "string", "description": "AWS only, optional. Named AWS profile to provision under (profile-first), overriding the cluster's fleet binding — same gap as `region` (studio#111)." } }, "required": ["image", "name"] @@ -540,19 +540,24 @@ impl OabMcp { /// precedence (studio#111). /// /// `aws_for`'s per-cluster lookup keys on a binding's `cluster` field, - /// which a console-written `[fleet.]` block doesn't have — - /// `appendFleetBlock` writes `runtime`/`region`/`profile`/`members`, not - /// `cluster` — so for every console-created fleet it falls through to the - /// ambient `[default]` chain. A deploy is exactly where that hurts most: on - /// a *first* create there is no block at all yet (`fleets.toml` is written - /// only after a confirmed successful provision), and the manifest built by - /// #111's create-from-scratch path resolves its VPC/subnet/security-group + /// which a `[fleet.]` block need not declare — the console never + /// writes one (`appendFleetBlock` emits runtime/region/profile/members, not + /// `cluster`), and neither does the shape the ADR's canonical example + /// shows — so for those fleets it falls through to the ambient `[default]` + /// chain and the fleet's recorded `region`/`profile` are read by nothing. A + /// deploy is exactly where that hurts most: on a *first* create there is no + /// block at all yet (`fleets.toml` is written only after a confirmed + /// successful provision), and the manifest built by #111's + /// create-from-scratch path resolves its VPC/subnet/security-group /// defaults against whatever config it is handed, so a create meant for /// one account/region would land in another. Passing the wizard's answer /// with the call closes that. /// - /// Falls back to [`Self::aws_for`] when the caller names neither, so every - /// existing caller keeps today's behavior. + /// The overrides are layered onto the cluster's own binding, not + /// substituted for it — a caller that pins only `region` still acts under + /// whichever profile that binding selects. Falls back to [`Self::aws_for`] + /// when the caller names neither, so every existing caller keeps today's + /// behavior. async fn aws_or( &self, cluster: &str, @@ -566,12 +571,19 @@ impl OabMcp { if region.is_none() && profile.is_none() { return self.aws_for(cluster).await; } - scp::resolve_binding_config(&scp::FleetBinding { - region: region.map(str::to_string), - profile: profile.map(str::to_string), - ..Default::default() - }) - .await + // Short read-lock, clone out, drop the guard before any await. No + // binding governs the cluster (the common case for a console-created + // fleet — see the doc comment) ⇒ start from a blank one and let the + // caller's own answer stand as the only identity there is. + let mut binding = self + .bindings + .read() + .unwrap() + .for_cluster(cluster) + .cloned() + .unwrap_or_default(); + binding = binding.with_identity(region, profile); + scp::resolve_binding_config(&binding).await } async fn t_list(&self, args: &Map) -> Result { diff --git a/crates/studio-cp/src/lib.rs b/crates/studio-cp/src/lib.rs index b35d885..7b4ff83 100644 --- a/crates/studio-cp/src/lib.rs +++ b/crates/studio-cp/src/lib.rs @@ -989,6 +989,30 @@ impl FleetBinding { .iter() .any(|m| m == service_name || m == short_name) } + + /// A copy of this binding with a caller's own `region`/`profile` layered + /// on top (studio#111): each field the caller names replaces this + /// binding's, each one it leaves unset is kept, and an `None` binding + /// (`for_cluster` found no fleet for the cluster) yields a + /// credentials-only binding. + /// + /// **Layering, not substitution** is the point. A deploy call that pins + /// only a region must keep acting under whatever profile the governing + /// binding selects — building a fresh binding from the caller's fields + /// alone would quietly drop it and fall back to the ambient `[default]` + /// account, which is the very failure this exists to prevent. + /// + /// Blank strings count as unset, so a caller that always passes its fields + /// (an empty text input, a `null` field) needs no trimming of its own. + pub fn with_identity(mut self, region: Option<&str>, profile: Option<&str>) -> FleetBinding { + if let Some(r) = region.filter(|s| !s.is_empty()) { + self.region = Some(r.to_string()); + } + if let Some(p) = profile.filter(|s| !s.is_empty()) { + self.profile = Some(p.to_string()); + } + self + } } /// The body of a `[fleet.]` table — the fields of a [`FleetBinding`] minus @@ -3171,4 +3195,79 @@ aws_access_key_id = AKIA... check_acp_image_compat("my-registry.example.com/custom:latest", &wizard_input(true, None)) .expect("not an openab release tag — can't verify, don't block"); } + + // studio#111 — `FleetBinding::with_identity`, the seam `oab-mcp`'s + // `aws_or` layers a deploy call's own region/profile onto. Pure, so no AWS + // access is needed to pin the layering rule. + + fn binding(region: Option<&str>, profile: Option<&str>) -> FleetBinding { + FleetBinding { + name: "prod".into(), + runtime: FleetRuntime::Ecs, + region: region.map(str::to_string), + profile: profile.map(str::to_string), + ..Default::default() + } + } + + #[test] + fn with_identity_overrides_both_fields_when_the_caller_names_both() { + let b = binding(Some("us-east-1"), Some("old")) + .with_identity(Some("ap-northeast-1"), Some("new")); + assert_eq!(b.region.as_deref(), Some("ap-northeast-1")); + assert_eq!(b.profile.as_deref(), Some("new")); + } + + #[test] + fn with_identity_keeps_the_bindings_profile_when_the_caller_pins_only_a_region() { + // The regression this shape exists for: substituting the caller's + // fields for the binding's would drop "prod-admin" here and silently + // provision under the ambient `[default]` account instead. + let b = + binding(Some("us-east-1"), Some("prod-admin")).with_identity(Some("eu-west-1"), None); + assert_eq!(b.region.as_deref(), Some("eu-west-1")); + assert_eq!(b.profile.as_deref(), Some("prod-admin")); + } + + #[test] + fn with_identity_keeps_the_bindings_region_when_the_caller_pins_only_a_profile() { + let b = binding(Some("us-east-1"), None).with_identity(None, Some("staging")); + assert_eq!(b.region.as_deref(), Some("us-east-1")); + assert_eq!(b.profile.as_deref(), Some("staging")); + } + + #[test] + fn with_identity_treats_blank_fields_as_unset() { + // The console always passes both fields, so an empty text input has + // to read as "unset" rather than as "provision into the empty region". + let b = binding(Some("us-east-1"), Some("prod-admin")).with_identity(Some(""), Some(" ")); + assert_eq!(b.region.as_deref(), Some("us-east-1")); + assert_eq!(b.profile.as_deref(), Some("prod-admin")); + } + + #[test] + fn with_identity_on_a_blank_binding_yields_a_credentials_only_binding() { + // What `oab-mcp`'s `aws_or` does when `for_cluster` found nothing (the + // common case for a console-created fleet): start from a blank binding, + // so the caller's own answer is all there is and must not be discarded. + let b = FleetBinding::default().with_identity(Some("ap-northeast-1"), Some("studio-prod")); + assert_eq!(b.region.as_deref(), Some("ap-northeast-1")); + assert_eq!(b.profile.as_deref(), Some("studio-prod")); + // …and nothing else is invented. + assert_eq!(b.cluster, None); + assert_eq!(b.context, None); + assert_eq!(b.namespace, None); + assert!(b.members.is_empty()); + } + + #[test] + fn with_identity_leaves_the_rest_of_the_binding_untouched() { + let mut base = binding(Some("us-east-1"), Some("prod-admin")); + base.members = vec!["oab-default-zeus".into()]; + base.cluster = Some("oab".into()); + let b = base.with_identity(Some("eu-west-1"), Some("studio-prod")); + assert_eq!(b.name, "prod"); + assert_eq!(b.cluster.as_deref(), Some("oab")); + assert_eq!(b.members, vec!["oab-default-zeus".to_string()]); + } } diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index d3e66d3..24f5c30 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -136,6 +136,8 @@ async fn deploy_provision( provider: Option, context: Option, expected_principal: Option, + region: Option, + profile: Option, ) -> Result { let cluster = cluster.unwrap_or_else(default_cluster); let client = { @@ -169,6 +171,15 @@ async fn deploy_provision( if let Some(ep) = expected_principal.filter(|s| !s.is_empty()) { params["expected_principal"] = json!(ep); } + // studio#111: same AWS identity as `deploy_provision_agent` below — the + // compose-library path creates through the identical + // create-or-redeploy branch, so it needs the identical override. + if let Some(r) = region.filter(|s| !s.is_empty()) { + params["region"] = json!(r); + } + if let Some(p) = profile.filter(|s| !s.is_empty()) { + params["profile"] = json!(p); + } match client.call_tool("deploy_provision", params).await { Ok(v) => Ok(v), Err(e) => { @@ -250,12 +261,13 @@ async fn deploy_provision_agent( } // studio#111: forward the fleet's AWS identity. The console collects // Region + Credential profile in the New Fleet identity step and records - // them in `fleets.toml` — but a console-written `[fleet.]` block - // carries no `cluster` key, so the sidecar's per-cluster credential lookup - // can't resolve them and would provision against the ambient `[default]` - // chain instead. On a first create there's no block at all yet - // (`fleets.toml` is written only after a confirmed successful deploy), so - // these args are the only carrier the create has. + // them in `fleets.toml` — but `oab-mcp` resolves the managing credential + // per ECS cluster, keyed on a binding's `cluster` key, which a + // `[fleet.]` block need not carry (the console never writes one), so + // the recorded pair is read by nothing and the deploy would act as the + // ambient `[default]` chain instead. On a first create there's no block at + // all yet (`fleets.toml` is written only after a confirmed successful + // deploy), so these args are the only carrier the create has. if let Some(r) = region.filter(|s| !s.is_empty()) { params["region"] = json!(r); } From 4c3d3972588c45c068f04cfbf236d8ec19396860 Mon Sep 17 00:00:00 2001 From: Devin Date: Sat, 3 Oct 2026 03:11:12 +0800 Subject: [PATCH 03/16] fix(deploy): pin the deploy-identity seam with tests (#111) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the previous review pass. `aws_or` itself had no coverage — the one defect the last commit repaired could have come straight back, since reverting it to a substituting `FleetBinding { region, profile, ..Default }` compiles and passes everything the repo had. - `aws_or`'s body is now the free `identity_binding(&FleetBindings, cluster, region, profile)`: no `.await`, so the cluster→binding lookup, the layering and the argument order are all testable without an AWS client. Six tests cover a region-only override keeping the binding's profile (the regression), profile-only, both, no-governing-fleet, a binding on a different cluster, and neither-override leaving the binding intact. - Whitespace-only `region`/`profile` now count as unset and accepted values are trimmed in `with_identity`, matching the console layer's `orUndefined` — a padded region must not reach `aws_config::Region::new` and name a region that doesn't exist. Pinned by a test. - Documented two deliberate non-changes: the credential lookup keys on `cluster` while the wizard derives its identity from the fleet name (a mismatch no console-written `fleets.toml` can produce, and resolving by fleet name is a separate change to credential selection), and `local_config_folder` is now trimmed (intentional — a padded path is a different directory; the only in-app source is the native directory picker, which returns unpadded paths). Refs #111 --- console/src/deployArgs.ts | 6 +- crates/oab-mcp/src/lib.rs | 139 ++++++++++++++++++++++++++++++++---- crates/studio-cp/src/lib.rs | 21 ++++-- src-tauri/src/lib.rs | 6 +- 4 files changed, 152 insertions(+), 20 deletions(-) diff --git a/console/src/deployArgs.ts b/console/src/deployArgs.ts index ebb90d4..b7883dd 100644 --- a/console/src/deployArgs.ts +++ b/console/src/deployArgs.ts @@ -79,7 +79,11 @@ export function awsIdentityFor(source: AwsIdentitySource): { } /** - * Trimmed value, or `undefined` for anything blank — every optional field is + * Trimmed value, or `undefined` for anything blank. Trimming is a behavior + * change for one pre-existing field — `local_config_folder` used to cross the + * wire verbatim — and is intentional: a padded path names a different directory + * than the trimmed one. The only in-app source of it is the native directory + * picker (`main.ts`'s Config-folder setting), which returns unpadded paths. — every optional field is * omitted from the call rather than sent as `""`, because the sidecar's own * "absent means default" rules (`acp_enabled` defaulting on, `context` * falling back to the kubeconfig current-context) key off absence, not on an diff --git a/crates/oab-mcp/src/lib.rs b/crates/oab-mcp/src/lib.rs index 3684dfa..7db6c71 100644 --- a/crates/oab-mcp/src/lib.rs +++ b/crates/oab-mcp/src/lib.rs @@ -355,6 +355,29 @@ impl Target { } } +/// The binding a call should resolve its AWS identity from (studio#111): +/// whatever fleet governs `cluster`, with the caller's own `region`/`profile` +/// layered on top. +/// +/// Split out of [`OabMcp::aws_or`] — and kept free of `.await` — so the +/// layering, the cluster lookup and the argument order are all unit-testable +/// without an AWS client. Returns a blank binding when no fleet governs the +/// cluster (the common case for a console-created fleet, whose +/// `[fleet.]` block declares no `cluster` key), in which case the +/// caller's own answer is the only identity there is. +fn identity_binding( + bindings: &scp::FleetBindings, + cluster: &str, + region: Option<&str>, + profile: Option<&str>, +) -> scp::FleetBinding { + bindings + .for_cluster(cluster) + .cloned() + .unwrap_or_default() + .with_identity(region, profile) +} + impl OabMcp { /// Build the handler from the environment: the default AWS credential chain, /// `$OAB_CLUSTER` (then `oab`), and the opt-in fleet bindings file (a missing @@ -553,6 +576,14 @@ impl OabMcp { /// one account/region would land in another. Passing the wizard's answer /// with the call closes that. /// + /// The lookup key is `cluster`, not the fleet name — a hand-written + /// `fleets.toml` whose fleets sit on different `cluster` keys but whose + /// caller passes neither `cluster` nor `fleet` would get one fleet's + /// identity layered onto another's target. No console path can do that: + /// `appendFleetBlock` never writes a `cluster` key, so `for_cluster` finds + /// nothing and only the caller's own pair applies. Resolving by fleet name + /// instead is a separate change to credential selection, not a deploy fix. + /// /// The overrides are layered onto the cluster's own binding, not /// substituted for it — a caller that pins only `region` still acts under /// whichever profile that binding selects. Falls back to [`Self::aws_for`] @@ -565,24 +596,15 @@ impl OabMcp { profile: Option<&str>, ) -> aws_config::SdkConfig { let (region, profile) = ( - region.filter(|s| !s.is_empty()), - profile.filter(|s| !s.is_empty()), + region.filter(|s| !s.trim().is_empty()), + profile.filter(|s| !s.trim().is_empty()), ); if region.is_none() && profile.is_none() { return self.aws_for(cluster).await; } - // Short read-lock, clone out, drop the guard before any await. No - // binding governs the cluster (the common case for a console-created - // fleet — see the doc comment) ⇒ start from a blank one and let the - // caller's own answer stand as the only identity there is. - let mut binding = self - .bindings - .read() - .unwrap() - .for_cluster(cluster) - .cloned() - .unwrap_or_default(); - binding = binding.with_identity(region, profile); + // Short read-lock, handed over by reference, guard dropped at this + // statement's end — nothing below holds a lock across an await. + let binding = identity_binding(&self.bindings.read().unwrap(), cluster, region, profile); scp::resolve_binding_config(&binding).await } @@ -1347,6 +1369,95 @@ mod tests { assert_eq!(v["stop_code"], "EssentialContainerExited"); } + // studio#111 — `identity_binding`, the seam `aws_or` resolves a deploy + // call's AWS identity through. Pure, so the layering rule and the + // cluster→binding lookup are pinned without an AWS client. + + fn bindings_with_cluster_binding() -> scp::FleetBindings { + scp::FleetBindings { + fleets: vec![scp::FleetBinding { + name: "prod".into(), + runtime: scp::FleetRuntime::Ecs, + cluster: Some("oab".into()), + region: Some("us-east-1".into()), + profile: Some("prod-admin".into()), + ..Default::default() + }], + } + } + + #[test] + fn identity_binding_layers_a_region_only_override_onto_the_cluster_binding() { + // The regression: substituting the caller's fields for the binding's + // would drop "prod-admin" and silently act as the ambient `[default]`. + let b = identity_binding( + &bindings_with_cluster_binding(), + "oab", + Some("eu-west-1"), + None, + ); + assert_eq!(b.region.as_deref(), Some("eu-west-1")); + assert_eq!(b.profile.as_deref(), Some("prod-admin")); + } + + #[test] + fn identity_binding_layers_a_profile_only_override_onto_the_cluster_binding() { + let b = identity_binding( + &bindings_with_cluster_binding(), + "oab", + None, + Some("studio-prod"), + ); + assert_eq!(b.region.as_deref(), Some("us-east-1")); + assert_eq!(b.profile.as_deref(), Some("studio-prod")); + } + + #[test] + fn identity_binding_keeps_both_when_the_caller_names_both() { + let b = identity_binding( + &bindings_with_cluster_binding(), + "oab", + Some("ap-northeast-1"), + Some("studio-prod"), + ); + assert_eq!(b.region.as_deref(), Some("ap-northeast-1")); + assert_eq!(b.profile.as_deref(), Some("studio-prod")); + } + + #[test] + fn identity_binding_uses_the_callers_answer_when_no_fleet_governs_the_cluster() { + // The console's own shape: `[fleet.]` declares no `cluster`, so + // `for_cluster` finds nothing and the caller's pair is all there is. + let b = identity_binding( + &scp::FleetBindings::default(), + "oab", + Some("ap-northeast-1"), + Some("studio-prod"), + ); + assert_eq!(b.region.as_deref(), Some("ap-northeast-1")); + assert_eq!(b.profile.as_deref(), Some("studio-prod")); + } + + #[test] + fn identity_binding_ignores_a_binding_for_a_different_cluster() { + let b = identity_binding( + &bindings_with_cluster_binding(), + "other", + Some("eu-west-1"), + None, + ); + assert_eq!(b.region.as_deref(), Some("eu-west-1")); + assert_eq!(b.profile, None); + } + + #[test] + fn identity_binding_leaves_the_binding_untouched_when_the_caller_names_neither() { + let b = identity_binding(&bindings_with_cluster_binding(), "oab", None, None); + assert_eq!(b.region.as_deref(), Some("us-east-1")); + assert_eq!(b.profile.as_deref(), Some("prod-admin")); + assert_eq!(b.cluster.as_deref(), Some("oab")); + } + #[test] fn deployment_json_carries_counters_and_instance_states() { let d = scp::Deployment { diff --git a/crates/studio-cp/src/lib.rs b/crates/studio-cp/src/lib.rs index 7b4ff83..f087efe 100644 --- a/crates/studio-cp/src/lib.rs +++ b/crates/studio-cp/src/lib.rs @@ -1004,12 +1004,15 @@ impl FleetBinding { /// /// Blank strings count as unset, so a caller that always passes its fields /// (an empty text input, a `null` field) needs no trimming of its own. + /// Surrounding whitespace is trimmed off an accepted value, matching the + /// console layer (`deployArgs.ts`'s `orUndefined`), so a stray space can't + /// reach `aws_config::Region::new` and resolve a region that doesn't exist. pub fn with_identity(mut self, region: Option<&str>, profile: Option<&str>) -> FleetBinding { - if let Some(r) = region.filter(|s| !s.is_empty()) { - self.region = Some(r.to_string()); + if let Some(r) = region.filter(|s| !s.trim().is_empty()) { + self.region = Some(r.trim().to_string()); } - if let Some(p) = profile.filter(|s| !s.is_empty()) { - self.profile = Some(p.to_string()); + if let Some(p) = profile.filter(|s| !s.trim().is_empty()) { + self.profile = Some(p.trim().to_string()); } self } @@ -3245,6 +3248,16 @@ aws_access_key_id = AKIA... assert_eq!(b.profile.as_deref(), Some("prod-admin")); } + #[test] + fn with_identity_trims_an_accepted_value() { + // Same rule as the console layer's, so a padded field from any caller + // can't reach `Region::new` and name a region that doesn't exist. + let b = binding(Some("us-east-1"), Some("prod-admin")) + .with_identity(Some(" ap-northeast-1 "), Some(" studio-prod\t")); + assert_eq!(b.region.as_deref(), Some("ap-northeast-1")); + assert_eq!(b.profile.as_deref(), Some("studio-prod")); + } + #[test] fn with_identity_on_a_blank_binding_yields_a_credentials_only_binding() { // What `oab-mcp`'s `aws_or` does when `for_cluster` found nothing (the diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index 24f5c30..97b773e 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -173,7 +173,11 @@ async fn deploy_provision( } // studio#111: same AWS identity as `deploy_provision_agent` below — the // compose-library path creates through the identical - // create-or-redeploy branch, so it needs the identical override. + // create-or-redeploy branch, so it needs the identical override. No + // console caller sends these yet (the wizard only calls + // `deploy_provision_agent`, and its compose-library UI was removed in + // studio#128); they exist for direct MCP clients, like the tool's other + // provider/placement args. if let Some(r) = region.filter(|s| !s.is_empty()) { params["region"] = json!(r); } From f2735825fd09e2269068e4d6c7e0054e12e35bbf Mon Sep 17 00:00:00 2001 From: Devin Date: Sat, 3 Oct 2026 03:20:09 +0800 Subject: [PATCH 04/16] docs(deploy): correct three comment claims from review (#111) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - `deployArgs.ts` wrote the table name as `[fleet.]`, missing a bracket. - `orUndefined`'s doc had the omission-policy paragraph spliced into it behind a stray em-dash; moved to `provisionAgentArgs`, where that policy belongs (and corrected there — `acp_enabled` is always sent, so it isn't part of it). - `aws_or`'s doc claimed "no console path" can hit the cluster-vs-fleet-name key mismatch. Only console-*written* `fleets.toml` can't: an operator can hand-add `cluster` in the console's own config editor. Restated, with the hand-edited case named as a pre-existing gap rather than claimed impossible. Refs #111 --- console/src/deployArgs.ts | 24 ++++++++++++++---------- crates/oab-mcp/src/lib.rs | 17 ++++++++++------- 2 files changed, 24 insertions(+), 17 deletions(-) diff --git a/console/src/deployArgs.ts b/console/src/deployArgs.ts index b7883dd..05c6507 100644 --- a/console/src/deployArgs.ts +++ b/console/src/deployArgs.ts @@ -10,7 +10,7 @@ // at create time the region/profile the identity step collected exist nowhere // but here. Drop them and `oab-mcp` falls back to the ambient `[default]` // credential chain, because it resolves a fleet's credential per ECS cluster -// and a `[fleet.` block need not declare the `cluster` key that lookup +// and a `[fleet.]` block need not declare the `cluster` key that lookup // matches on (this console's writer never emits one) — so the first manifest's // `default_networking` VPC/subnet/security-group discovery (studio#111's // create-from-scratch defaults, ported from `oabctl create`'s wizard) and the @@ -79,15 +79,13 @@ export function awsIdentityFor(source: AwsIdentitySource): { } /** - * Trimmed value, or `undefined` for anything blank. Trimming is a behavior - * change for one pre-existing field — `local_config_folder` used to cross the - * wire verbatim — and is intentional: a padded path names a different directory - * than the trimmed one. The only in-app source of it is the native directory - * picker (`main.ts`'s Config-folder setting), which returns unpadded paths. — every optional field is - * omitted from the call rather than sent as `""`, because the sidecar's own - * "absent means default" rules (`acp_enabled` defaulting on, `context` - * falling back to the kubeconfig current-context) key off absence, not on an - * empty string. + * Trimmed value, or `undefined` for anything blank. + * + * Trimming is a behavior change for one pre-existing field — + * `local_config_folder` used to cross the wire verbatim — and is intentional: a + * padded path names a different directory than the trimmed one. The only in-app + * source of it is the native directory picker (`main.ts`'s Config-folder + * setting), which returns unpadded paths. */ function orUndefined(value: string | undefined): string | undefined { const trimmed = value?.trim(); @@ -100,6 +98,12 @@ function orUndefined(value: string | undefined): string | undefined { * `acp_enabled` is passed verbatim rather than omitted when off — the sidecar's * default-when-absent rule (studio#119/128) is "on", so an ACP-off agent has * to say so explicitly. + * + * Every other optional field goes over the wire omitted rather than as `""`, + * because the sidecar's own "absent means default" rules (`context` falling + * back to the kubeconfig current-context, `region`/`profile` falling back to + * whatever credential it resolves for itself) key off absence, not on an empty + * string. */ export function provisionAgentArgs(input: ProvisionAgentInput): Record { const args: Record = { diff --git a/crates/oab-mcp/src/lib.rs b/crates/oab-mcp/src/lib.rs index 7db6c71..864fecd 100644 --- a/crates/oab-mcp/src/lib.rs +++ b/crates/oab-mcp/src/lib.rs @@ -576,13 +576,16 @@ impl OabMcp { /// one account/region would land in another. Passing the wizard's answer /// with the call closes that. /// - /// The lookup key is `cluster`, not the fleet name — a hand-written - /// `fleets.toml` whose fleets sit on different `cluster` keys but whose - /// caller passes neither `cluster` nor `fleet` would get one fleet's - /// identity layered onto another's target. No console path can do that: - /// `appendFleetBlock` never writes a `cluster` key, so `for_cluster` finds - /// nothing and only the caller's own pair applies. Resolving by fleet name - /// instead is a separate change to credential selection, not a deploy fix. + /// The lookup key is `cluster`, not the fleet name — a `fleets.toml` whose + /// fleets sit on *different* `cluster` keys but whose caller passes + /// neither `cluster` nor `fleet` would get one fleet's identity layered + /// onto another's target. No console-*written* `fleets.toml` can produce + /// that: `appendFleetBlock` never writes a `cluster` key, so `for_cluster` + /// finds nothing and only the caller's own pair applies. An operator who + /// hand-adds `cluster` in the console's `fleets.toml` editor can — but that + /// is the pre-existing target-cluster mismatch, not something this change + /// introduces or can fix from here. Resolving by fleet name instead is a + /// separate change to credential selection. /// /// The overrides are layered onto the cluster's own binding, not /// substituted for it — a caller that pins only `region` still acts under From 98babe96a5608924e4415e43ad3af7cd6b0ec74d Mon Sep 17 00:00:00 2001 From: Devin Date: Sat, 3 Oct 2026 03:38:33 +0800 Subject: [PATCH 05/16] fix(oab-mcp): resolve a call's credential by fleet name, not only cluster (#111) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit made a deploy act under the operator's chosen region/profile but left every read/scale/apply still resolving through `for_cluster` — so a fleet created under a non-default profile would deploy where the operator asked and then be invisible to its own roster, unscalable and undeletable from the console. Consistently-ambient was already wrong; this change had made it inconsistently-wrong in exactly the scenario it targets. - `base_binding(bindings, args, cluster)` is the new resolution seam: the **named** fleet's binding (`fleet:`) first, then the per-cluster lookup, then nothing. `for_cluster` alone can only ever match a binding that declares a `cluster` key, and neither the console's writer nor the ADR's canonical `[fleet.]` example emits one — which is why every console-created fleet's recorded identity was read by nothing, on writes and reads alike. - `aws_for_call` (all the read/scale/apply sites) and `aws_or` (both deploy sites) go through it; the credential memo is now keyed by identity source (`fleet:` vs `cluster:`) so two fleets sharing a cluster key can't collide. It also fixes a case review caught: a `fleet`-scoped call that pinned only a region used to layer it onto whichever fleet `for_cluster` happened to return first, producing an identity matching neither binding. - An unknown or blank `fleet` name falls through to the cluster lookup rather than erroring — this seam never introduces an error the caller didn't already have; the handlers still reject an unknown fleet by name before reaching it. - New coverage: fleet-name-first lookup, layering onto the named fleet rather than the cluster one, fall-through for an unknown name, a k8s-runtime named fleet being skipped (no AWS identity), and `has_identity_override` — the blank-rejecting guard that keeps credential-less calls on the memoized ambient path — plus three console tests composing `awsIdentityFor` with `provisionAgentArgs` for add-instance and new-fleet. - `orUndefined`'s doc said trimming changed one pre-existing field; it changed two (`chat_platform` also crossed the wire verbatim). Corrected. Refs #111 --- console/src/deployArgs.test.ts | 35 ++++ console/src/deployArgs.ts | 10 +- crates/oab-mcp/src/lib.rs | 289 ++++++++++++++++++++++++++++----- 3 files changed, 284 insertions(+), 50 deletions(-) diff --git a/console/src/deployArgs.test.ts b/console/src/deployArgs.test.ts index c169f60..bc41198 100644 --- a/console/src/deployArgs.test.ts +++ b/console/src/deployArgs.test.ts @@ -138,6 +138,41 @@ describe("awsIdentityFor — which source answers a submit (studio#111)", () => }); }); +describe("the two seams compose — what an add-instance submit actually sends", () => { + it("takes region/profile from the fleet binding, not from empty form fields", () => { + // The panel's submit handler is exactly this composition; `mode.region` / + // `mode.profile` (the fleet `main.ts` read off `FleetConfigEntry`) have to + // be what reaches the call, since add-instance has no AWS field group. + const args = provisionAgentArgs({ + ...ecs, + acpEnabled: true, + ...awsIdentityFor({ kind: "add-instance", region: "eu-west-1", profile: "prod-admin" }), + }); + expect(args.region).toBe("eu-west-1"); + expect(args.profile).toBe("prod-admin"); + }); + + it("sends nothing to override with when the fleet records no identity", () => { + const args = provisionAgentArgs({ + ...ecs, + acpEnabled: true, + ...awsIdentityFor({ kind: "add-instance", region: null, profile: null }), + }); + expect("region" in args).toBe(false); + expect("profile" in args).toBe(false); + }); + + it("still carries the new-fleet identity step's own fields through", () => { + const args = provisionAgentArgs({ + ...ecs, + acpEnabled: true, + ...awsIdentityFor({ kind: "new-fleet", region: "ap-northeast-1", profile: "studio-prod" }), + }); + expect(args.region).toBe("ap-northeast-1"); + expect(args.profile).toBe("studio-prod"); + }); +}); + describe("provisionAgentArgs — optional wizard fields", () => { it("forwards chat platform secrets and the local config folder when set", () => { const args = provisionAgentArgs({ diff --git a/console/src/deployArgs.ts b/console/src/deployArgs.ts index 05c6507..1f83e18 100644 --- a/console/src/deployArgs.ts +++ b/console/src/deployArgs.ts @@ -81,11 +81,11 @@ export function awsIdentityFor(source: AwsIdentitySource): { /** * Trimmed value, or `undefined` for anything blank. * - * Trimming is a behavior change for one pre-existing field — - * `local_config_folder` used to cross the wire verbatim — and is intentional: a - * padded path names a different directory than the trimmed one. The only in-app - * source of it is the native directory picker (`main.ts`'s Config-folder - * setting), which returns unpadded paths. + * Trimming is a behavior change for the two fields that used to cross the wire + * verbatim — `chat_platform` and `local_config_folder` — and is intentional: a + * padded platform name selects no platform, and a padded path names a different + * directory. Neither source produces padding in-app (a `` value, and - * the native directory picker `main.ts`'s Config-folder setting uses). + * Trimming is a behavior change for the four fields that used to cross the wire + * verbatim — `chat_platform`, `local_config_folder`, and the k8s pair + * `context`/`expected_principal` — and is intentional in each: a padded platform + * name or kubeconfig context selects neither, a padded path names a different + * directory, and a padded `expected_principal` matches no principal. No in-app + * source produces padding (a `