From 13b73e9af1056991cc0a17da622e4a5d980fc458 Mon Sep 17 00:00:00 2001 From: Brett Chien Date: Sat, 22 Aug 2026 11:19:11 +0800 Subject: [PATCH 1/2] fix(bundle): actually restore a deployed agent's bundle at boot, via pre_seed (slice 3c) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root-cause fix for the gap studio#97's slice-3c investigation found: nothing in openab downloads bundleFrom's loose-file S3 prefix back into ~ at boot. push_bundle (slice 2, already merged) only ever built the upload half; no restore mechanism was ever wired to the other end, on ECS or anywhere else. openab's only "S3 -> ~" feature is hooks.pre_seed, and it consumes zip archives, not a prefix of individual objects -- a different shape nothing bridged. Fix: reuse pre_seed instead of building a bespoke restore mechanism per platform. - studio_compose::Bundle::zip_bytes() -- a deterministic zip of the bundle's files (still pure/no-I/O: ZipWriter writes to an in-memory Cursor>). - oabctl::studio_api::bundle_zip_uri/inject_pre_seed_hook -- the zip's S3 URI, and appending a [hooks.pre_seed] section to a config.toml's bytes pointing at it. Appends rather than re-parsing+re-serializing, so an operator's existing comments/formatting survive; a no-op if hooks.pre_seed is already present (never overrides an operator's own hook). - control_plane::resolve_bucket exposed pub, so a caller can resolve the bucket before constructing objects (needed to compute the zip URI ahead of upload) instead of after, avoiding a second implicit resolution. - provision_from_library (studio-cp): resolves the bucket once, injects the hook into the composed config.toml object, uploads the zip alongside the existing per-file objects (kept for now -- harmless, and removing them is a separable decision). - K8sDriver: removed the bundle_from bail entirely. This is the actual payoff -- pre_seed is orchestrator-agnostic (S3 GetObject + extract, same binary/feature regardless of what started the process), and build_deployment already points the container's command at configFrom exactly like EcsDriver does, so a k8s pod restores its bundle through the identical mechanism with zero k8s-specific carrier code. No ConfigMap, no volume, no init-container -- slice 3c's original scope turned out not to exist once the actual restore path was fixed. Fixes both the ECS gap and the k8s driver in one change; the two platforms now share one restore mechanism instead of doubling the maintenance surface with a bespoke k8s carrier. 11 new tests across oabctl/studio-compose/studio-cp (145/145 total across the three crates), clippy clean (no new warnings; 3 pre-existing ones elsewhere untouched). Stacked on #102 (3f) -- needs K8sDriver + the driver-side test fixtures from the earlier branches in the chain. Ref: studio#97 (K8s driver — ADR #63 slice 3, sub-slice tracking) --- crates/oabctl/src/control_plane.rs | 9 ++- crates/oabctl/src/k8s_driver.rs | 50 +++++---------- crates/oabctl/src/lib.rs | 1 + crates/oabctl/src/studio_api.rs | 100 +++++++++++++++++++++++++++-- crates/studio-compose/Cargo.toml | 3 + crates/studio-compose/src/lib.rs | 59 +++++++++++++++++ crates/studio-cp/src/lib.rs | 33 +++++++++- 7 files changed, 214 insertions(+), 41 deletions(-) diff --git a/crates/oabctl/src/control_plane.rs b/crates/oabctl/src/control_plane.rs index 16f926b..4524bc8 100644 --- a/crates/oabctl/src/control_plane.rs +++ b/crates/oabctl/src/control_plane.rs @@ -8,7 +8,14 @@ pub(crate) fn select_bucket(configured: Option<&str>, env: Option<&str>) -> Opti .map(str::to_owned) } -pub(crate) async fn resolve_bucket( +/// Resolve the control-plane bucket: `configured`, else `$OAB_CONTROL_PLANE_BUCKET`, +/// else `oab-control-plane-{account}` derived from the caller's AWS identity +/// (an STS call — only made when neither of the first two is set). `provision`/ +/// `redeploy`/`push_bundle`/`delete` all resolve it internally via this same +/// function; exposed `pub` so a caller that needs the resolved value *before* +/// calling one of those (e.g. to compute a bundle's zip URI ahead of upload) +/// doesn't have to reimplement or guess at the resolution order. +pub async fn resolve_bucket( aws_config: &aws_config::SdkConfig, configured: Option<&str>, ) -> Result { diff --git a/crates/oabctl/src/k8s_driver.rs b/crates/oabctl/src/k8s_driver.rs index 371a94b..d56e105 100644 --- a/crates/oabctl/src/k8s_driver.rs +++ b/crates/oabctl/src/k8s_driver.rs @@ -11,10 +11,16 @@ //! `aws-sm://`/raw-ARN values in a k8s-runtime manifest fail loudly at apply //! time — a manifest error, not a silent no-op. //! -//! `spec.bundleFrom` (the composed persona/skills bundle) is explicitly -//! **not yet supported** and fails loudly rather than silently -//! mis-deploying — ECS gets this for free via its S3 file carrier, k8s needs -//! a ConfigMap/volume carrier, tracked as sub-slice 3c. +//! `spec.bundleFrom` needs no k8s-specific handling at all (sub-slice 3c +//! turned out to be a non-issue): the actual bundle restore happens via +//! `openab`'s own `hooks.pre_seed` feature, wired into the composed +//! `config.toml`'s content at provisioning time +//! (`oabctl::studio_api::inject_pre_seed_hook`) — orchestrator-agnostic by +//! construction, since `pre_seed` is just "S3 GetObject + extract," it +//! doesn't know or care whether the booting process is an ECS task or a k8s +//! pod. `build_deployment` already points the container's command at +//! `configFrom`, same as ECS, so this Just Works without any driver code +//! here reading `bundleFrom` at all. //! //! Observing k8s state into the canonical 6-state (the `apply`/`scale` //! counterpart to `status.rs`'s ECS `service_status`/`instance_status`) is @@ -88,19 +94,6 @@ fn require_kubernetes_runtime(m: &OABServiceManifest) -> Result<&crate::manifest } } -/// Reject the still-not-yet-supported manifest feature explicitly (see -/// module docs) instead of silently dropping it. -fn reject_unsupported(m: &OABServiceManifest) -> Result<()> { - if m.spec.bundle_from.is_some() { - anyhow::bail!( - "k8s bundle carrier not implemented yet (studio#97 sub-slice 3c) — '{}/{}' has spec.bundleFrom set", - m.metadata.namespace, - m.metadata.name - ); - } - Ok(()) -} - /// Build the `env[]` entries for `spec.secrets`: each value must be a /// `k8s-secret://#` ref, which becomes a `secretKeyRef` — /// kubelet resolves it at pod-start time, no API call needed here (unlike @@ -249,7 +242,6 @@ impl ProvisionDriver for K8sDriver { async fn apply(&self, manifests: &[OABServiceManifest], _opts: &ProvisionOptions) -> Result { let mut services = Vec::with_capacity(manifests.len()); for m in manifests { - reject_unsupported(m)?; let deployment = build_deployment(m)?; let name = k8s_deployment_name(&m.metadata.name); let api: Api = Api::namespaced(self.client.clone(), &m.metadata.namespace); @@ -359,22 +351,12 @@ mod tests { } #[test] - fn reject_unsupported_passes_a_plain_manifest() { - let m = k8s_manifest(None, &[]); - reject_unsupported(&m).unwrap(); - } - - #[test] - fn reject_unsupported_bails_on_bundle_from() { - let m = k8s_manifest(Some("s3://bucket/artifacts/prod/orca/"), &[]); - let err = reject_unsupported(&m).unwrap_err(); - assert!(err.to_string().contains("3c")); - } - - #[test] - fn reject_unsupported_passes_manifests_with_k8s_secrets() { - let m = k8s_manifest(None, &[("DISCORD_BOT_TOKEN", "k8s-secret://oab-orca#DISCORD_BOT_TOKEN")]); - reject_unsupported(&m).unwrap(); + fn build_deployment_ignores_bundle_from_no_special_handling_needed() { + // bundleFrom isn't consumed by the driver at all (see module docs) — + // a manifest carrying it builds identically to one without. + let with = k8s_manifest(Some("s3://bucket/artifacts/prod/orca/"), &[]); + let without = k8s_manifest(None, &[]); + assert_eq!(build_deployment(&with).unwrap(), build_deployment(&without).unwrap()); } #[test] diff --git a/crates/oabctl/src/lib.rs b/crates/oabctl/src/lib.rs index 8bad400..ef552c3 100644 --- a/crates/oabctl/src/lib.rs +++ b/crates/oabctl/src/lib.rs @@ -74,6 +74,7 @@ pub use manifest::{ Runtime, Spec, }; +pub use control_plane::resolve_bucket; pub use driver::{EcsDriver, ProvisionDriver, ProvisionOptions}; pub use events::{fetch_ecs_events, EcsEvent, DEFAULT_EVENTS_LOG_GROUP}; pub use k8s_driver::{k8s_deployment_name, K8sDriver}; diff --git a/crates/oabctl/src/studio_api.rs b/crates/oabctl/src/studio_api.rs index e73ec04..f775931 100644 --- a/crates/oabctl/src/studio_api.rs +++ b/crates/oabctl/src/studio_api.rs @@ -40,14 +40,66 @@ pub fn parse_manifests(yaml: &str) -> Result> { } } -/// The `bundleFrom` S3 **prefix** URI an agent's composed bundle is uploaded to -/// and restored from at boot: `s3://{bucket}/artifacts/{namespace}/{name}/` -/// (trailing slash). Pairs with `studio_compose::Bundle::artifact_objects`, whose -/// keys are exactly `artifacts/{namespace}/{name}/{path}` under the same bucket. +/// The `bundleFrom` S3 **prefix** URI an agent's composed bundle is uploaded to: +/// `s3://{bucket}/artifacts/{namespace}/{name}/` (trailing slash). Pairs with +/// `studio_compose::Bundle::artifact_objects`, whose keys are exactly +/// `artifacts/{namespace}/{name}/{path}` under the same bucket. +/// +/// Bookkeeping only — nothing downloads this prefix back (see +/// [`bundle_zip_uri`]'s doc for the mechanism that actually restores a +/// bundle). Kept for now as an informational record of where the loose files +/// landed; no manifest field or driver reads it. pub fn bundle_from_uri(bucket: &str, namespace: &str, name: &str) -> String { format!("s3://{bucket}/artifacts/{namespace}/{name}/") } +/// Filename (relative to the artifacts prefix) a bundle's zip archive uploads +/// to. Must match `studio_compose::Bundle::ZIP_FILENAME` — the two crates +/// don't share a dependency edge to enforce this with a shared constant, so +/// `studio-cp` (which depends on both) tests the two stay in sync. +pub const BUNDLE_ZIP_FILENAME: &str = "bundle.zip"; + +/// The S3 URI a bundle's zip archive (`studio_compose::Bundle::zip_bytes`) +/// uploads to: `s3://{bucket}/artifacts/{namespace}/{name}/bundle.zip`. +/// +/// This — not [`bundle_from_uri`]'s loose-file prefix — is what actually gets +/// restored at boot: [`inject_pre_seed_hook`] wires this URI into the +/// deployed agent's own `config.toml` as a `[hooks.pre_seed]` source, and +/// `openab`'s `pre_seed` feature (already the mechanism that restores an +/// agent's own persistent state across restarts) downloads + extracts it into +/// `~` on every boot. Platform-agnostic by construction — `pre_seed` is pure +/// "S3 GetObject + extract," it doesn't know or care whether the process +/// booting is an ECS task or a k8s pod, so no k8s-specific bundle carrier is +/// needed (see studio#97's slice-3c investigation: `bundle_from_uri`'s prefix +/// was never actually consumed by anything on the ECS path either). +pub fn bundle_zip_uri(bucket: &str, namespace: &str, name: &str) -> String { + format!("s3://{bucket}/artifacts/{namespace}/{name}/{BUNDLE_ZIP_FILENAME}") +} + +/// Append a `[hooks.pre_seed]` section to `config_toml`'s bytes, pointing at +/// `zip_uri` — see [`bundle_zip_uri`] for why this is the actual bundle-restore +/// mechanism. A no-op (returns `config_toml` unchanged) if the config already +/// declares `[hooks.pre_seed]`: an operator who hand-authored one keeps +/// control, this never silently overrides it. Appends rather than +/// re-serializing the whole document, so existing comments/formatting in the +/// operator's authored `config.toml` survive untouched — same "verbatim, +/// don't round-trip through a parser" principle `save_bindings_text` already +/// applies to `fleets.toml`. +pub fn inject_pre_seed_hook(config_toml: &[u8], zip_uri: &str) -> Result> { + let text = std::str::from_utf8(config_toml).context("config.toml is not valid UTF-8")?; + let parsed: toml::Value = text.parse().context("config.toml is not valid TOML")?; + if parsed.get("hooks").and_then(|h| h.get("pre_seed")).is_some() { + return Ok(config_toml.to_vec()); + } + let quoted_uri = toml::Value::String(zip_uri.to_string()); + let mut out = text.to_string(); + if !out.is_empty() && !out.ends_with('\n') { + out.push('\n'); + } + out.push_str(&format!("\n[hooks.pre_seed]\nsources = [{quoted_uri}]\n")); + Ok(out.into_bytes()) +} + /// Outcome of pushing a bundle: which bucket it landed in and how many objects. #[derive(Debug, Clone, PartialEq, Eq)] pub struct PushBundleReport { @@ -237,6 +289,46 @@ mod tests { ); } + #[test] + fn bundle_zip_uri_is_the_bundle_from_prefix_plus_zip_filename() { + assert_eq!( + bundle_zip_uri("oab-control-plane-123", "prod", "orca"), + format!( + "{}{BUNDLE_ZIP_FILENAME}", + bundle_from_uri("oab-control-plane-123", "prod", "orca") + ) + ); + assert_eq!( + bundle_zip_uri("b", "prod", "orca"), + "s3://b/artifacts/prod/orca/bundle.zip" + ); + } + + #[test] + fn inject_pre_seed_hook_appends_to_existing_config() { + let config = b"[agent]\nname = \"orca\"\n"; + let out = inject_pre_seed_hook(config, "s3://bucket/artifacts/prod/orca/bundle.zip").unwrap(); + let text = String::from_utf8(out).unwrap(); + assert!(text.starts_with("[agent]\nname = \"orca\"\n")); + assert!(text.contains("[hooks.pre_seed]")); + assert!(text.contains("s3://bucket/artifacts/prod/orca/bundle.zip")); + // still valid TOML after injection + text.parse::().expect("valid toml"); + } + + #[test] + fn inject_pre_seed_hook_is_a_noop_when_already_present() { + let config = b"[hooks.pre_seed]\nsources = [\"s3://other/bucket.zip\"]\n"; + let out = inject_pre_seed_hook(config, "s3://bucket/artifacts/prod/orca/bundle.zip").unwrap(); + // unchanged, byte-for-byte — an operator's own hook is never overridden + assert_eq!(out, config); + } + + #[test] + fn inject_pre_seed_hook_rejects_invalid_toml() { + assert!(inject_pre_seed_hook(b"not = [valid", "s3://x/bundle.zip").is_err()); + } + #[test] fn manifest_round_trips_bundle_from() { // A manifest carrying bundleFrom parses into spec.bundle_from; one without diff --git a/crates/studio-compose/Cargo.toml b/crates/studio-compose/Cargo.toml index 74d1c80..3782a9b 100644 --- a/crates/studio-compose/Cargo.toml +++ b/crates/studio-compose/Cargo.toml @@ -13,3 +13,6 @@ license = "MIT" serde = { version = "1.0", features = ["derive"] } serde_json = "1" sha2 = "0.10" +# Still pure/no-I/O: ZipWriter writes to an in-memory Cursor>, no +# filesystem touched — see Bundle::zip_bytes. +zip = { version = "2", default-features = false, features = ["deflate"] } diff --git a/crates/studio-compose/src/lib.rs b/crates/studio-compose/src/lib.rs index d50cf5d..ef8f7b0 100644 --- a/crates/studio-compose/src/lib.rs +++ b/crates/studio-compose/src/lib.rs @@ -165,6 +165,38 @@ impl Bundle { .map(|(path, bytes)| (format!("{prefix}/{path}"), bytes.clone())) .collect() } + + /// The filename (relative to [`artifacts_prefix`]) the bundle's zip archive + /// uploads to: `bundle.zip`. + pub const ZIP_FILENAME: &'static str = "bundle.zip"; + + /// A zip archive of every bundle file, deterministic (files are a + /// `BTreeMap`, so entry order is stable across composes of equal inputs — + /// same guarantee [`Bundle::digest`] has). This is the artifact + /// `hooks.pre_seed` actually restores at boot — unlike + /// [`Bundle::artifact_objects`]'s loose per-file uploads, which nothing + /// downloads back (see the K8s driver ADR's slice-3c investigation: + /// `bundleFrom`'s doc comment claimed "the runtime restores this prefix + /// into ~ at first boot," but no such restore code exists anywhere in + /// `openab` — `pre_seed` only ever consumed zip sources). Still a pure, + /// no-I/O transform: `ZipWriter` writes to an in-memory buffer. + pub fn zip_bytes(&self) -> Vec { + let mut buf = std::io::Cursor::new(Vec::new()); + { + let mut writer = zip::ZipWriter::new(&mut buf); + let options = zip::write::SimpleFileOptions::default() + .compression_method(zip::CompressionMethod::Deflated); + for (path, bytes) in &self.files { + // start_file/write on an in-memory Cursor> cannot fail + // (no OS I/O involved) — unwrap keeps this fn infallible, matching + // every other pure Bundle method (digest, preview, artifact_objects). + writer.start_file(path, options).expect("zip: in-memory write"); + std::io::Write::write_all(&mut writer, bytes).expect("zip: in-memory write"); + } + writer.finish().expect("zip: in-memory write"); + } + buf.into_inner() + } } /// A UTF-8-lossy, serde-friendly view of one bundle file for the preview UI. @@ -679,6 +711,33 @@ mod tests { assert_eq!(artifacts_prefix("ns", "a"), "artifacts/ns/a"); } + #[test] + fn zip_bytes_round_trips_every_file_unmodified() { + let lib = SkillsLibrary::from_iter([("s", skill_with(&[("SKILL.md", "hi\n")]))]); + let mut t = tmpl(); + t.skills = vec!["s".into()]; + let bundle = compose(&t, &Overlay::default(), &lib).unwrap(); + + let zip_bytes = bundle.zip_bytes(); + let mut archive = zip::ZipArchive::new(std::io::Cursor::new(zip_bytes)).expect("valid zip"); + let mut by_name: BTreeMap> = BTreeMap::new(); + for i in 0..archive.len() { + let mut entry = archive.by_index(i).unwrap(); + let mut bytes = Vec::new(); + std::io::Read::read_to_end(&mut entry, &mut bytes).unwrap(); + by_name.insert(entry.name().to_string(), bytes); + } + assert_eq!(by_name, bundle.files); + } + + #[test] + fn zip_bytes_is_deterministic_across_equal_composes() { + let lib = SkillsLibrary::default(); + let bundle_a = compose(&tmpl(), &Overlay::default(), &lib).unwrap(); + let bundle_b = compose(&tmpl(), &Overlay::default(), &lib).unwrap(); + assert_eq!(bundle_a.zip_bytes(), bundle_b.zip_bytes()); + } + #[test] fn round_trips_through_json() { // The Tauri boundary shuttles these as JSON; make sure serde is wired. diff --git a/crates/studio-cp/src/lib.rs b/crates/studio-cp/src/lib.rs index 4678795..b07aff2 100644 --- a/crates/studio-cp/src/lib.rs +++ b/crates/studio-cp/src/lib.rs @@ -725,7 +725,27 @@ pub async fn provision_from_library( .filter(|s| !s.is_empty()) .map(str::to_string) .unwrap_or_else(|| bundle.image_tag.clone()); - let objects = bundle.artifact_objects(namespace, name); + + // Resolve the bucket once here (rather than letting `redeploy` resolve it + // internally, as before) so the zip's S3 URI can be computed and wired + // into config.toml *before* upload. See oabctl::studio_api::bundle_zip_uri + // for why this — not the loose-file artifact_objects prefix — is what + // actually gets restored on the deployed agent at boot. + let bucket = oabctl::resolve_bucket(aws_config, None).await?; + let mut objects = bundle.artifact_objects(namespace, name); + let zip_uri = oabctl::studio_api::bundle_zip_uri(&bucket, namespace, name); + let config_key = format!("{}/config.toml", studio_compose::artifacts_prefix(namespace, name)); + match objects.iter_mut().find(|(key, _)| *key == config_key) { + Some((_, bytes)) => *bytes = oabctl::studio_api::inject_pre_seed_hook(bytes, &zip_uri)?, + None => anyhow::bail!("composed bundle for {namespace}/{name} has no config.toml — cannot wire hooks.pre_seed"), + } + let zip_key = format!( + "{}/{}", + studio_compose::artifacts_prefix(namespace, name), + oabctl::studio_api::BUNDLE_ZIP_FILENAME + ); + objects.push((zip_key, bundle.zip_bytes())); + let digest = bundle.digest(); let report = oabctl::studio_api::redeploy( @@ -735,7 +755,7 @@ pub async fn provision_from_library( name, Some(&image), &objects, - None, + Some(&bucket), ) .await?; @@ -1165,4 +1185,13 @@ namespace = "prod" // exact match assert!(principal_matches(brett, brett)); } + + #[test] + fn bundle_zip_filename_constants_stay_in_sync() { + // oabctl::studio_api::BUNDLE_ZIP_FILENAME and + // studio_compose::Bundle::ZIP_FILENAME can't share a dependency edge + // to enforce this with one constant (see provision_from_library) — + // this is the cross-crate seam that catches drift instead. + assert_eq!(oabctl::studio_api::BUNDLE_ZIP_FILENAME, studio_compose::Bundle::ZIP_FILENAME); + } } From 4b2550b438a955beebdcaf48672b96a904bfaedb Mon Sep 17 00:00:00 2001 From: Brett Chien Date: Sun, 23 Aug 2026 15:55:48 +0800 Subject: [PATCH 2/2] fix(oabctl): address code-review findings on the bundle-restore fix Follow-up commit on #103, from a multi-angle review pass before merge: - inject_pre_seed_hook: rewritten on toml_edit (format-preserving) instead of raw string append. Previously, *any* existing [hooks.pre_seed] made injection a full no-op -- an operator's own hooks.pre_seed (e.g. this agent's own persistent-state restore, unrelated to the deploy bundle) meant the new bundle.zip was uploaded but never actually wired in, and provision_from_library still reported success. Now merges the zip URI into the existing `sources` array (true no-op only when that exact URI is already present), mirrors pre_seed's own 5-source cap, and bails instead of silently producing invalid TOML when `hooks`/`hooks.pre_seed` is already an inline table (which can't be reopened with a `[table]` header). - provision_from_library: mutates `bundle.files` in place before deriving artifact_objects/zip_bytes/digest from it, instead of deriving them from the pre-injection Bundle. Previously the uploaded zip's own config.toml never got the pre_seed hook, and ProvisionOutcome.digest didn't match what was actually uploaded. - secret_env_vars: attaches the per-secret error context (env var name, namespace/name) to both the "wrong scheme" and "right scheme, malformed body" cases. anyhow's Context impl for Option only fires on None, so the previous `.with_context()??` silently dropped context on the latter. - EcsDriver::apply: preserves the structured ApplyError in the anyhow error's source chain (via .context() on Error::new(e)) instead of flattening it into anyhow::anyhow!(...). A caller can again `.chain().find_map(downcast_ref::)` to recover `.completed`/`.failed_service` for a partial fleet-apply failure. ApplyError::reconciliation made pub(crate) so driver.rs's test can construct one without a live ECS call. 24 new/changed tests, 131/131 total across oabctl+studio-cp, clippy clean (same 3 pre-existing warnings elsewhere, untouched). --- crates/oabctl/Cargo.toml | 1 + crates/oabctl/src/apply.rs | 4 +- crates/oabctl/src/driver.rs | 53 +++++++++++- crates/oabctl/src/k8s_driver.rs | 33 ++++++-- crates/oabctl/src/studio_api.rs | 145 +++++++++++++++++++++++++++----- crates/studio-cp/src/lib.rs | 17 ++-- 6 files changed, 219 insertions(+), 34 deletions(-) diff --git a/crates/oabctl/Cargo.toml b/crates/oabctl/Cargo.toml index 143a5dd..18ea47f 100644 --- a/crates/oabctl/Cargo.toml +++ b/crates/oabctl/Cargo.toml @@ -40,6 +40,7 @@ serde_json = "1" serde_yaml = "0.9" tokio = { version = "1.40", features = ["full"] } toml = "0.8" +toml_edit = "0.22" anyhow = "1.0" dirs = "6" rpassword = "7" diff --git a/crates/oabctl/src/apply.rs b/crates/oabctl/src/apply.rs index a7edf20..97db0df 100644 --- a/crates/oabctl/src/apply.rs +++ b/crates/oabctl/src/apply.rs @@ -111,7 +111,9 @@ impl ApplyError { } } - fn reconciliation( + // pub(crate): driver.rs's tests construct one to verify the + // ApplyError -> anyhow::Error conversion preserves it in the source chain. + pub(crate) fn reconciliation( failed_service: ServiceTarget, completed: ApplyReport, source: impl Into, diff --git a/crates/oabctl/src/driver.rs b/crates/oabctl/src/driver.rs index 3d716b9..01bed27 100644 --- a/crates/oabctl/src/driver.rs +++ b/crates/oabctl/src/driver.rs @@ -66,7 +66,7 @@ impl<'a> ProvisionDriver for EcsDriver<'a> { } crate::apply::apply_manifests(self.aws_config, manifests, &ecs_opts) .await - .map_err(|e| anyhow::anyhow!("apply failed [{:?}]: {e}", e.kind)) + .map_err(ecs_apply_error_to_anyhow) } async fn scale(&self, namespace: &str, name: &str, size: i32) -> Result<()> { @@ -93,3 +93,54 @@ impl<'a> ProvisionDriver for EcsDriver<'a> { .await } } + +/// Convert `apply_manifests`'s structured error into an `anyhow::Error` +/// while keeping the `ApplyError` itself in the source chain — a caller can +/// still `err.chain().find_map(anyhow::Error::downcast_ref::)` +/// to recover `.completed`/`.failed_service` for a partial fleet-apply +/// failure, same as callers could before `ProvisionDriver` existed (when +/// `studio_api::provision` used `.context(..)` directly on `apply_manifests`'s +/// result). A prior version of this flattened `e` into `anyhow::anyhow!(...)`, +/// which silently dropped that recovery path — extracted as its own function +/// so the conversion is unit-testable without a live ECS call. +fn ecs_apply_error_to_anyhow(e: crate::apply::ApplyError) -> anyhow::Error { + let kind = e.kind; + anyhow::Error::new(e).context(format!("apply failed [{kind:?}]")) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::apply::{ApplyAction, AppliedService, ApplyError, ApplyErrorKind, ApplyReport, ServiceTarget}; + + #[test] + fn ecs_apply_error_to_anyhow_preserves_downcast_and_partial_progress() { + let completed = ApplyReport { + services: vec![AppliedService { + namespace: "prod".to_string(), + name: "orca".to_string(), + resource_name: "oab-prod-orca".to_string(), + action: ApplyAction::Updated, + webhook_urls: vec![], + warnings: vec![], + }], + }; + let failed = ServiceTarget { + namespace: "prod".to_string(), + name: "mira".to_string(), + ecs_service_name: "oab-prod-mira".to_string(), + }; + let source = ApplyError::reconciliation(failed.clone(), completed.clone(), anyhow::anyhow!("boom")); + + let err = ecs_apply_error_to_anyhow(source); + + assert!(err.to_string().contains("Reconciliation")); + let recovered = err + .chain() + .find_map(|e| e.downcast_ref::()) + .expect("ApplyError must survive in the source chain"); + assert_eq!(recovered.kind, ApplyErrorKind::Reconciliation); + assert_eq!(recovered.failed_service, Some(failed)); + assert_eq!(recovered.completed, completed); + } +} diff --git a/crates/oabctl/src/k8s_driver.rs b/crates/oabctl/src/k8s_driver.rs index d56e105..efcfa0e 100644 --- a/crates/oabctl/src/k8s_driver.rs +++ b/crates/oabctl/src/k8s_driver.rs @@ -105,14 +105,22 @@ fn secret_env_vars(m: &OABServiceManifest) -> Result> { .secrets .iter() .map(|(env_name, value)| { + // parse_k8s_secret_uri returns Option>: None for the + // wrong scheme, Some(Err(..)) for the right scheme but a + // malformed body (e.g. missing '#'). anyhow's Context impl for + // Option only fires on None, so a chained `.with_context()??` + // here would silently drop this context on the Some(Err(..)) + // path — attach it explicitly to both instead. + let context = || { + format!( + "spec.secrets['{env_name}'] for k8s runtime must use \ + k8s-secret://# (got '{value}') — '{}/{}'", + m.metadata.namespace, m.metadata.name + ) + }; let (secret_name, key) = crate::secrets::parse_k8s_secret_uri(value) - .with_context(|| { - format!( - "spec.secrets['{env_name}'] for k8s runtime must use \ - k8s-secret://# (got '{value}') — '{}/{}'", - m.metadata.namespace, m.metadata.name - ) - })??; + .ok_or_else(|| anyhow::anyhow!(context()))? + .with_context(context)?; Ok(EnvVar { name: env_name.clone(), value_from: Some(EnvVarSource { @@ -384,6 +392,17 @@ mod tests { assert!(err.to_string().contains("k8s-secret://")); } + #[test] + fn build_deployment_attributes_malformed_k8s_secret_ref_to_its_env_var() { + // Right scheme, malformed body (missing #key) — must still name the + // offending spec.secrets entry, not just the bare parser error. + let m = k8s_manifest(None, &[("DISCORD_BOT_TOKEN", "k8s-secret://oab-orca")]); + let err = build_deployment(&m).unwrap_err(); + let msg = err.to_string(); + assert!(msg.contains("DISCORD_BOT_TOKEN"), "must name the env var: {msg}"); + assert!(msg.contains("prod/orca"), "must name the agent: {msg}"); + } + #[test] fn build_deployment_sets_image_command_and_env() { let m = k8s_manifest(None, &[]); diff --git a/crates/oabctl/src/studio_api.rs b/crates/oabctl/src/studio_api.rs index f775931..a73679a 100644 --- a/crates/oabctl/src/studio_api.rs +++ b/crates/oabctl/src/studio_api.rs @@ -76,28 +76,85 @@ pub fn bundle_zip_uri(bucket: &str, namespace: &str, name: &str) -> String { format!("s3://{bucket}/artifacts/{namespace}/{name}/{BUNDLE_ZIP_FILENAME}") } -/// Append a `[hooks.pre_seed]` section to `config_toml`'s bytes, pointing at -/// `zip_uri` — see [`bundle_zip_uri`] for why this is the actual bundle-restore -/// mechanism. A no-op (returns `config_toml` unchanged) if the config already -/// declares `[hooks.pre_seed]`: an operator who hand-authored one keeps -/// control, this never silently overrides it. Appends rather than -/// re-serializing the whole document, so existing comments/formatting in the -/// operator's authored `config.toml` survive untouched — same "verbatim, -/// don't round-trip through a parser" principle `save_bindings_text` already -/// applies to `fleets.toml`. +/// `pre_seed`'s own cap on `hooks.pre_seed.sources` length +/// (`openab_core::pre_seed::MAX_SOURCES`) — mirrored here so a full sources +/// list fails loudly at inject time instead of silently at the deployed +/// agent's next boot. +const PRE_SEED_MAX_SOURCES: usize = 5; + +/// Add `zip_uri` to `config_toml`'s `hooks.pre_seed.sources`, creating +/// `[hooks.pre_seed]` if absent — see [`bundle_zip_uri`] for why this is the +/// actual bundle-restore mechanism. Uses `toml_edit` (format-preserving), so +/// an operator's existing comments/layout survive; only the `sources` array +/// (and, if needed, the `[hooks]`/`[hooks.pre_seed]` headers) are touched. +/// +/// Idempotent — a true no-op (byte-for-byte unchanged) only when `zip_uri` +/// is *already in* the sources list. Critically, this is **not** the same as +/// "a `[hooks.pre_seed]` section already exists": an operator's own +/// unrelated `hooks.pre_seed` (e.g. modeled on this fleet's own persistent +/// state restore) gets `zip_uri` appended to its sources, not silently left +/// alone — the earlier version of this function treated *any* existing +/// `hooks.pre_seed` as "already handled" and skipped injection entirely, +/// which meant a template that already used pre_seed for something else +/// silently never got the new bundle wired in at all. +/// +/// Bails (doesn't guess) when `hooks` or `hooks.pre_seed` already exists but +/// isn't a table (e.g. an inline table `hooks = { restart = "always" }` — +/// TOML forbids reopening a key already closed by inline-table syntax with a +/// `[hooks.pre_seed]` header, so blindly appending would silently produce +/// invalid TOML that only fails at the deployed agent's boot), or when +/// `sources` is already at `pre_seed`'s own 5-entry cap. pub fn inject_pre_seed_hook(config_toml: &[u8], zip_uri: &str) -> Result> { let text = std::str::from_utf8(config_toml).context("config.toml is not valid UTF-8")?; - let parsed: toml::Value = text.parse().context("config.toml is not valid TOML")?; - if parsed.get("hooks").and_then(|h| h.get("pre_seed")).is_some() { + let mut doc: toml_edit::DocumentMut = text.parse().context("config.toml is not valid TOML")?; + + let hooks: &mut dyn toml_edit::TableLike = table_like_entry(doc.as_table_mut(), "hooks")?; + let pre_seed = table_like_entry(hooks, "pre_seed")?; + + let sources_item = pre_seed + .entry("sources") + .or_insert(toml_edit::value(toml_edit::Array::new())); + let sources = sources_item + .as_array_mut() + .with_context(|| "config.toml's hooks.pre_seed.sources is not an array".to_string())?; + + if sources.iter().any(|v| v.as_str() == Some(zip_uri)) { return Ok(config_toml.to_vec()); } - let quoted_uri = toml::Value::String(zip_uri.to_string()); - let mut out = text.to_string(); - if !out.is_empty() && !out.ends_with('\n') { - out.push('\n'); + if sources.len() >= PRE_SEED_MAX_SOURCES { + anyhow::bail!( + "config.toml's hooks.pre_seed.sources already has {PRE_SEED_MAX_SOURCES} entries \ + (openab's own limit) — cannot add the bundle zip source '{zip_uri}' without \ + exceeding it; trim an existing source first" + ); + } + sources.push(zip_uri); + + Ok(doc.to_string().into_bytes()) +} + +/// Get-or-create `parent[key]` as a table-like entry (regular `[table]` or +/// inline `{ ... }` both qualify structurally, but only a regular table can +/// safely receive a further nested `[table.child]` header — see +/// [`inject_pre_seed_hook`]'s doc for why an inline table bails instead of +/// silently producing invalid TOML). +fn table_like_entry<'a>( + parent: &'a mut dyn toml_edit::TableLike, + key: &str, +) -> Result<&'a mut dyn toml_edit::TableLike> { + if let Some(existing) = parent.get(key) { + if existing.is_inline_table() { + anyhow::bail!( + "config.toml's `{key}` is an inline table — cannot add a nested `[{key}.*]` \ + section to it; use a regular `[{key}]` table instead" + ); + } } - out.push_str(&format!("\n[hooks.pre_seed]\nsources = [{quoted_uri}]\n")); - Ok(out.into_bytes()) + parent + .entry(key) + .or_insert(toml_edit::Item::Table(toml_edit::Table::new())) + .as_table_like_mut() + .with_context(|| format!("config.toml's `{key}` key is not a table")) } /// Outcome of pushing a bundle: which bucket it landed in and how many objects. @@ -317,18 +374,66 @@ mod tests { } #[test] - fn inject_pre_seed_hook_is_a_noop_when_already_present() { - let config = b"[hooks.pre_seed]\nsources = [\"s3://other/bucket.zip\"]\n"; + fn inject_pre_seed_hook_is_a_true_noop_only_when_source_already_present() { + let config = b"[hooks.pre_seed]\nsources = [\"s3://bucket/artifacts/prod/orca/bundle.zip\"]\n"; let out = inject_pre_seed_hook(config, "s3://bucket/artifacts/prod/orca/bundle.zip").unwrap(); - // unchanged, byte-for-byte — an operator's own hook is never overridden + // unchanged, byte-for-byte — this exact source is already wired, a + // second redeploy of the same agent shouldn't touch the file at all assert_eq!(out, config); } + #[test] + fn inject_pre_seed_hook_appends_to_an_operators_own_unrelated_pre_seed() { + // The bug this replaced: an operator's own hooks.pre_seed (e.g. for + // this agent's own persistent-state restore, unrelated to the deploy + // bundle) used to make injection silently no-op entirely, so the new + // bundle was never wired in at all. It must be added alongside. + let config = b"[hooks.pre_seed]\nsources = [\"s3://other/state.zip\"]\n"; + let out = inject_pre_seed_hook(config, "s3://bucket/artifacts/prod/orca/bundle.zip").unwrap(); + let text = String::from_utf8(out).unwrap(); + assert!(text.contains("s3://other/state.zip"), "operator's own source must survive: {text}"); + assert!(text.contains("s3://bucket/artifacts/prod/orca/bundle.zip"), "new source must be added: {text}"); + let reparsed: toml::Value = text.parse().expect("valid toml"); + let sources = reparsed["hooks"]["pre_seed"]["sources"].as_array().unwrap(); + assert_eq!(sources.len(), 2); + } + + #[test] + fn inject_pre_seed_hook_rejects_more_than_five_sources() { + let config = br#"[hooks.pre_seed] +sources = ["s3://a", "s3://b", "s3://c", "s3://d", "s3://e"] +"#; + let err = inject_pre_seed_hook(config, "s3://new").unwrap_err(); + assert!(err.to_string().contains('5')); + } + #[test] fn inject_pre_seed_hook_rejects_invalid_toml() { assert!(inject_pre_seed_hook(b"not = [valid", "s3://x/bundle.zip").is_err()); } + #[test] + fn inject_pre_seed_hook_rejects_inline_table_hooks() { + let config = b"hooks = { restart = \"always\" }\n"; + let err = inject_pre_seed_hook(config, "s3://x/bundle.zip").unwrap_err(); + assert!(err.to_string().contains("inline table")); + } + + #[test] + fn inject_pre_seed_hook_rejects_inline_table_pre_seed() { + let config = b"[hooks]\npre_seed = { target = \"~\" }\n"; + let err = inject_pre_seed_hook(config, "s3://x/bundle.zip").unwrap_err(); + assert!(err.to_string().contains("inline table")); + } + + #[test] + fn inject_pre_seed_hook_preserves_comments_and_layout() { + let config = b"# a comment worth keeping\n[agent]\nname = \"orca\" # inline comment\n"; + let out = inject_pre_seed_hook(config, "s3://x/bundle.zip").unwrap(); + let text = String::from_utf8(out).unwrap(); + assert!(text.starts_with("# a comment worth keeping\n[agent]\nname = \"orca\" # inline comment\n")); + } + #[test] fn manifest_round_trips_bundle_from() { // A manifest carrying bundleFrom parses into spec.bundle_from; one without diff --git a/crates/studio-cp/src/lib.rs b/crates/studio-cp/src/lib.rs index b07aff2..57d0242 100644 --- a/crates/studio-cp/src/lib.rs +++ b/crates/studio-cp/src/lib.rs @@ -719,7 +719,7 @@ pub async fn provision_from_library( overlay: Option<&str>, image_override: Option<&str>, ) -> anyhow::Result { - let bundle = studio_compose::compose_named(library, template, overlay) + let mut bundle = studio_compose::compose_named(library, template, overlay) .map_err(|e| anyhow::anyhow!("compose failed: {e}"))?; let image = image_override .filter(|s| !s.is_empty()) @@ -731,14 +731,21 @@ pub async fn provision_from_library( // into config.toml *before* upload. See oabctl::studio_api::bundle_zip_uri // for why this — not the loose-file artifact_objects prefix — is what // actually gets restored on the deployed agent at boot. + // + // Patch `bundle.files` itself (not a derived copy) *before* deriving + // artifact_objects/zip_bytes/digest from it, so all three agree on the + // same, actually-uploaded content — deriving them from the bundle + // pre-patch (as an earlier version of this function did) meant the + // uploaded zip's own config.toml lacked the hook, and the reported + // digest didn't match what was actually deployed. let bucket = oabctl::resolve_bucket(aws_config, None).await?; - let mut objects = bundle.artifact_objects(namespace, name); let zip_uri = oabctl::studio_api::bundle_zip_uri(&bucket, namespace, name); - let config_key = format!("{}/config.toml", studio_compose::artifacts_prefix(namespace, name)); - match objects.iter_mut().find(|(key, _)| *key == config_key) { - Some((_, bytes)) => *bytes = oabctl::studio_api::inject_pre_seed_hook(bytes, &zip_uri)?, + match bundle.files.get_mut("config.toml") { + Some(bytes) => *bytes = oabctl::studio_api::inject_pre_seed_hook(bytes, &zip_uri)?, None => anyhow::bail!("composed bundle for {namespace}/{name} has no config.toml — cannot wire hooks.pre_seed"), } + + let mut objects = bundle.artifact_objects(namespace, name); let zip_key = format!( "{}/{}", studio_compose::artifacts_prefix(namespace, name),