Conversation
…openabdev#111) `[+ 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.<name>]` 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/openabdev#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 openabdev#111
…#111) 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.<name>]` 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 openabdev#111
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 openabdev#111
- `deployArgs.ts` wrote the table name as `[fleet.<name>]`, 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 openabdev#111
…ster (openabdev#111) 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.<name>]` 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:<name>` vs `cluster:<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 openabdev#111
…abdev#111) The last commit's own docs contradicted its code, and one comment claimed a consequence the console can't actually reach. All corrected, not restated: - `aws_for_call`'s doc had the deleted `aws_for`'s paragraph glued to it with no blank line, and `aws_or` linked `[`Self::aws_for`]` twice to a function that no longer exists. Replaced with the actual contract; links repointed. - `aws_or`'s doc opened by saying "the lookup key is `cluster`, not the fleet name" — the bug the previous commit fixed, stated as the design. Now describes the fleet-name-first base, and records the asymmetry that remains: `target()` rejects a `fleet`-scoped ECS call whose binding omits `cluster`, so for a console-written fleet the reads never reach this seam at all. Closing that gate is pre-existing work, not this change's. - `aws_for_binding`'s named branch lacked the `profile.is_some() || region.is_some()` guard its cluster branch had, contradicting its own doc: a credential-less named binding did a redundant config load and cached a duplicate ambient config under that fleet's key. Mirrored. - The memo key is now `memo_key(fleet, cluster)`, free of `.await` and unit- tested for the two namespaces staying disjoint (including a fleet named `oab` and one named `cluster:oab`). - crates/oab-mcp/README.md and both tool schemas said the override applies to "the cluster's fleet binding", which stopped being true once fleet-name-first landed. Restated as the `fleet` binding, else the `cluster` binding, with the no-memoization rule for overrides spelled out. Refs openabdev#111
…ev#111) Last review's remaining items, all in prose or in coverage of a decision that had none: - Three surviving "per-cluster" descriptions of the memo, which is keyed by identity source (`fleet:<name>` / `cluster:<cluster>`) since the previous commit — corrected at the struct field, at `t_fleet_write`, and at the test comment that explains `has_identity_override`. - The inline comment above `t_provision_agent`'s `region`/`profile` read still said it overrides "the per-cluster binding lookup"; the base is fleet-name first now. - `with_identity`'s doc described a `None` binding and blamed `for_cluster`, but the method takes an owned `FleetBinding` and the `unwrap_or_default()` step lives in `identity_binding`. Restated. - `aws_or`'s closing note now says plainly what the previous commit left standing: the console writes no `cluster` key and sends no `fleet`, so a console-created fleet's base is the caller's own pair (the point), the reads of such a fleet never reach the seam because `target()` rejects a cluster-less `fleet` scope (pre-existing), and an operator who hand-adds a `cluster` key can reach the cluster base, where a region-only override pairs with that cluster's first fleet's profile (also pre-existing, and needing credential selection keyed on the governing fleet rather than the cluster). - `binding_and_key(bindings, fleet, cluster)` is the whole `aws_for_binding` decision, free of `.await`, so the ambient short-circuit — a binding naming no profile *or* region — is covered by five new tests instead of being asserted only by a doc comment. `base_binding` takes the fleet name directly now rather than re-reading it out of the args map. - Correction to `7a25603`'s message, which said crates/oab-mcp/README.md "listed six of the eight write tools": it listed three of the six. Refs openabdev#111
…rule (openabdev#111) Last review's items 1-5. - `binding_and_key` now takes the `args` map and extracts `fleet` itself, so the one line that was untested — `aws_for_call` forwarding `args["fleet"]` as the memo namespace selector — lives in the tested pure seam. Passing `None` there instead compiled and passed every test in the repo while reintroducing the exact bug two commits ago fixed. - `aws_for_binding`'s doc claimed `named` was a `(fleet name, binding)` pair; it is the caller's fleet name, and the binding comes back from `binding_and_key`. - The trim-count claim was an undercount twice over: four fields crossed the wire verbatim before, not two — `context`/`expected_principal` did too, and are reachable with padding from a hand-edited `fleets.toml` via add-instance's inherited placement. Corrected, and now covered by a test (the k8s pair's trim was previously untested). - Four places still described credential resolution as cluster-keyed only, which stopped being true when fleet-name-first landed and contradicted crates/oab-mcp/README.md. Restated as the two-step rule everywhere, including the fact that the console's own calls match neither key — which is exactly why the wizard has to send its answer. - Two new tests pin the composition `deploy.ts` performs: the identity fed to `provisionAgentArgs` and the one recorded into `fleets.toml` are the same values, and a blank identity is recorded as absent rather than as `""` (which would read as a pinned-but-blank region to every later consumer). These are the pure halves of the submit handler; the DOM reads themselves remain untested, as they were before this change. Refs openabdev#111
…ment (openabdev#111) Follows 21fc52e/f575f5c, which restated the rule in the console and in crates/oab-mcp/README.md but missed this comment. The console names no fleet and its writer emits no `cluster` key, so neither of oab-mcp's two lookup keys matches — which is the whole reason the wizard has to send its answer. Refs openabdev#111
… is described (openabdev#111) The last review's remaining items. Eight places still described the credential lookup as cluster-only — three of them in files this change edits — which stopped being true when base_binding became fleet-name-first, and two of them (console/src/source.ts, console/src/render.ts) had never been touched at all: - console/src/deployArgs.ts + its test header: a [fleet.<name>] block *is* reachable by name now; the conclusion held, the stated reason did not. - console/src/source.ts (scaleDeployment) and render.ts (the roster's scale rows): both scale through aws_for_call now. - src-tauri's deploy_scale bridge: disagreed with its own sibling bridges, which c9662e2 already restated. - studio-cp's FleetBinding::cluster and FleetBindings::for_cluster docs: the cluster key is the *fallback* for a call that names no fleet, and for_cluster's first match is exactly why fleet-name-first resolution exists. Also renamed a test that exercised identity_binding while promising to cover base_binding. Refs openabdev#111
…abdev#111) Last review's items 1-4. - The new `region`/`profile` properties on both provision tools were the one part of this change asserted nowhere — a caller learns they exist only by reading the catalog, so silently dropping either property from either schema would leave every other test in the repo green while the argument quietly did nothing. A test now asserts both tools advertise both, and that neither became required. - Removed a lock comment from `aws_for_call`, which takes no lock: it only delegates to `aws_for_binding`, which carries its own. - `aws_or`'s doc still called `aws_for_call`'s lookup "per-cluster"; it is the cluster *fallback* inside it now. - `provisionAgentArgs`'s doc justified omitting `""` by saying the sidecar's region/profile rules "key off absence, not on an empty string" — they treat a blank as unset, which is not the same thing. Restated to match `with_identity`/`has_identity_override`. Refs openabdev#111
…abdev#111) Last review's items 1-4. - `binding_and_key` now states the one behavior change it makes visible: a `fleet`-scoped call naming a credential-less binding answers ambient, where `for_cluster`'s first match used to lend it a sibling's credential. Shadowing a sibling by accident is what per-fleet scoping exists to prevent, and `FleetBinding::profile`'s documented semantics are "ambient default, explicit override" — but an operator can observe it, so it is documented rather than buried. - Two `#[tokio::test]`s pin the memo boundary from both sides: an override resolves its own identity and leaves `self.resolved` empty (memoizing one would serve that caller's profile to every later call on the fleet), while a credential-free call does populate `cluster:oab` and is served from it on repeat. This is the invariant crates/oab-mcp/README.md states in prose. - Both tool schemas now say the override LAYERS — "a field you omit still comes from that binding" — and that it is not memoized. The catalog is the only contract an external MCP client reads, and the substitution-vs-layering distinction is exactly what a caller gets wrong. - The "identity cannot drift" test asserted against a hand-rolled entry stand-in, so it would have stayed green if deploy.ts went back to reading the form twice. It now asserts against the real `appendFleetBlock` output — the TOML that actually lands in `fleets.toml` — and its mutation (recording nothing while still deploying with a region) was confirmed to go red. Refs openabdev#111
…enabdev#111) Last review's items 1-4. - `handler_for_test` pinned a region on its loader. `ConfigLoader::load()` eagerly awaits the default region chain (env → profile file → IMDS), and with no `AWS_REGION` and no `~/.aws/config` that chain reaches for 169.254.169.254 — which in a sandbox times out rather than failing fast, so both `#[tokio::test]`s were spending seconds there. The comment claiming otherwise was wrong. - `identity_args(args)` extracts `(region, profile)` beside `base_binding`, and both provision handlers call it. That read was the one seam with no coverage: a handler that stopped forwarding, or swapped the halves, would no-op with every test green. Two tests, including the non-string-means-absent case that routes to the ambient path. - `binding_and_key`'s justification cited `FleetBinding::profile`'s docs for the phrase "ambient default, explicit override" — that wording belongs to `context`, not `profile`. It now cites what actually decides it: `resolve_binding_config` pins only what a binding names. - docs/adr/reverse-mcp-client.md still called the credential "cluster/account-granular" — the last doc the restatement pass missed. Refs openabdev#111
…v#111) Last review's items 1-4. - The 'no ambient work left to do / region is pinned' note was sitting on an unrelated `identity_args` test instead of `handler_for_test`, which it describes; a previous commit had inserted two tests between them. Moved. - `binding_and_key`'s `None` arm still carried a 'short read-lock … drop the guard before any await' comment and a `let guard = bindings;` alias from when it took the guard itself. It takes a plain borrow now — the caller's `read()` guard died at the end of its own statement — and the function has no `.await`. - console/README.md said 'a console call names no fleet', which is true of the wizard but false of the console's read/scale paths, where `main.ts` passes `activeFleet` and `target()` then rejects a cluster-less binding outright. Scoped to the deploy call, with that pre-existing gap named. - crates/oab-mcp/README.md pointed at `t_provision_agent` for an 'arg-by-arg contract' that is a two-line comment. Now points at what actually holds the contract: the table above plus each tool's description in `tools()`. Also corrects `9595b61`'s message, which claimed swapping the hand-rolled entry stand-in for `appendFleetBlock`'s real output fixed that test's blindness to `deploy.ts` reading the form twice. It did not, and does not — that test never executes `deploy.ts`. What the change actually adds is pinning that `appendFleetBlock` emits no `region`/`profile` line at all for a null pair. Refs openabdev#111
…ead it (openabdev#111) Last review's items 1-2. - `FleetBinding`'s doc (studio-cp) and the console's view-model comment still said two fleets sharing a `cluster` share "one credential" — which fleet-name-first resolution stopped implying, and which is exactly the case that motivated it. Both now say a shared `cluster` can carry different `region`/`profile`. - crates/oab-mcp/README.md disclosed the one operator-visible behavior change only in an internal function doc, so an operator whose scale or delete stops resolving after upgrading had no way to learn why from the README. The resolution paragraph now states it: a named fleet declaring no `profile`/`region` answers ambient rather than borrowing a same-cluster sibling's credential, and what to do about it. Refs openabdev#111
…ed rule (openabdev#111) Last review's items 1-4. - crates/oab-mcp/README.md said both `deploy_provision*` "create an agent that has no stored manifest yet", which contradicted the same tools' own descriptions: they are create-or-update, and an agent that already has a stored manifest takes `redeploy`'s patch path. - docs/adr/fleet-grouping-and-connection-model.md — the ADR whose own §4 already says `oab-mcp` keys credential selection on the governing fleet, not the cluster — still opened with "credential is a *consequence* of where the members physically live" and said orca/mira "share the `oab` cluster and one credential". That premise is exactly what this change stops relying on, and §4 contradicted it. Both passages restated: `region`/`profile` are per fleet, a shared `cluster` may carry different identities, and a fleet declaring neither gets the ambient chain. - `identity_args`'s doc claimed extracting it protects against a handler that "stopped forwarding" these — no test covers that, since driving `t_provision_agent` far enough to observe the config it builds needs AWS. Narrowed to the failure that *is* covered: swapping the two halves. - `binding_and_key`'s two `match` arms were provably the same expression (with no fleet named, `base_binding` reduces to `for_cluster`), so the rule was written twice and could drift, and the lock comment sat in only one arm. Collapsed to one expression; the comment moved to where it applies. Refs openabdev#111
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #111
Status of #111 itself
#111's core fix is already on
main(it landed with #116): whendeploy_provision/deploy_provision_agentfinds no stored manifest,build_default_manifestassembles a freshOABServiceManifestfromcreate.rs'swizard defaults and persists+applies it through the same
apply_manifestspathdeploy_applyuses; an existing manifest still takesredeploy's patch path,and the k8s counterpart landed with it. Scope items 1–3 are done.
This PR closes the part of #111 that was still broken on the create path
itself, found while verifying the above: the wizard's Region and Credential
profile were collected and recorded in
fleets.toml, but never reached thecreate.
The bug
oab-mcpresolves the managing credential fromfleets.tomlby the fleet acall named (
fleet:), else by the binding whoseclusterkey matches, elsethe ambient
[default]chain. The console matches neither key:fleet, andappendFleetBlockwritesruntime/region/profile/members— nevercluster(nor does the fleet ADR's canonical[fleet.<name>]example).So a console-created fleet's recorded
region/profilewere read by nothing,on writes and reads alike, and the ambient chain answered.
That is survivable on a redeploy. On the first create — the path #111 added —
it is the manifest's own problem:
fleets.tomlis written only after aconfirmed successful provision (ADR-83 §7.5), so the create is the one moment
there is no binding at all, and
build_default_manifest'screate::default_networkingVPC/subnet/security-group discovery runs againstwhatever config it was handed. A fleet created for one account/region landed in
another, while
fleets.tomlwent on to record the region the operator picked.The fix
console/src/deployArgs.ts(new, pure) — the wizard's collected fieldsshaped into the one
deploy_provision_agentargument object. Blank optionalsare omitted rather than sent as
""; no AWS credentials ride along on a k8ssubmit (a k8s pod has no AWS credential chain);
acp_enabledis always sent(the sidecar defaults it on when absent) and
acp_tokenis never sent for anACP-off agent.
awsIdentityFor— which source answers a submit:new-fleetreads its ownidentity step,
add-instanceinherits the target fleet's recorded pair, anunrecorded fleet yields "no override" rather than empty strings.
deploy.tsreads the identity once and feeds the same values to the call and to the
fleets.tomlblock, so the recorded binding cannot drift from what deployed.deploy_provision*MCP tools take optionalregion/profile,forwarded by both
src-tauribridges.OabMcp::aws_orlayers them ontothe base binding rather than substituting: naming only
regionkeeps thebinding's profile.
base_binding), so a fleet'sown recorded identity is honored by observation as well as creation. Without
this the change would have been a new half-working state: deploys into the
operator's account, roster blind to them. The memo is keyed per identity
source (
fleet:<name>/cluster:<cluster>), and per-call overrides are nevermemoized — caching one would serve that caller's profile to later calls on the
fleet.
Behavior change worth reading
A
fleet-scoped call naming a binding that declares noprofile/regionnow answers the ambient chain, where the per-cluster lookup used to lend it a
same-cluster sibling's credential. Shadowing a sibling by accident is what
per-fleet scoping exists to prevent, and a binding naming no credential is
asking for the ambient one — but it is operator-visible, and documented in
crates/oab-mcp/README.mdand inbinding_and_key.Known gaps this deliberately leaves
target()still rejects afleet-scoped ECS call whose binding omitscluster, so the console's read and scale paths never reach the newcredential seam. Pre-existing and unchanged by this PR (the deploy call avoids
it by passing
cluster); closing it is separate work.crates/studio-cp's first-provision manifest still has no test. That is test(studio-cp): pin the first-provision manifest contract (#111) #165'ssubject and deliberately untouched here; note its red proof cannot be replayed
by the fleet verifier, which computes the
nodeprofile from the rootpackage.jsonand therefore never runscargo.Test plan
npm test— 141 passed (24 new inconsole/src/deployArgs.test.ts); redat base (exit 1, the module did not exist), reproduced by the fleet
verifier's red replay
npm run typecheck,npm run build,npm run lint(a pre-existingsilent no-op —
consolehas no lint script)cargo check --workspace --all-targets— clean, zero warnings, includingthe
#[cfg(test)]targetsverify_issue.py— all five gates pass (baseline green, red replayexits 1, candidate + clean detached replay green)
cargo test --workspace— cannot run on the gate host: compilingaws-sdk-ec2needs >6.7 GB and the kernel OOM-killsrustc(
dmesg:oom-kill … task=rustc … anon-rss:6767156kB). The Rust testsadded here (20 in
oab-mcp, 7 instudio-cp) are thereforecompile-verified only; they avoid AWS entirely (pure helpers plus two
#[tokio::test]s with the region pinned soConfigLoader::load()cannotreach IMDS).
src-tauri— cannot be compiled here either (nopkg-config/glibfortauri-build). Its change is two additiveOption<String>params plus twoif letblocks, mirroring the sibling params exactly.+ New fleetcreate in a non-default profile/region,then roster, scale and delete against that account.