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/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/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 371a94b..efcfa0e 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 @@ -112,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 { @@ -249,7 +250,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 +359,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] @@ -402,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/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..a73679a 100644 --- a/crates/oabctl/src/studio_api.rs +++ b/crates/oabctl/src/studio_api.rs @@ -40,14 +40,123 @@ 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}") +} + +/// `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 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()); + } + 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" + ); + } + } + 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. #[derive(Debug, Clone, PartialEq, Eq)] pub struct PushBundleReport { @@ -237,6 +346,94 @@ 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_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 — 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-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..57d0242 100644 --- a/crates/studio-cp/src/lib.rs +++ b/crates/studio-cp/src/lib.rs @@ -719,13 +719,40 @@ 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()) .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. + // + // 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 zip_uri = oabctl::studio_api::bundle_zip_uri(&bucket, namespace, name); + 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), + 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 +762,7 @@ pub async fn provision_from_library( name, Some(&image), &objects, - None, + Some(&bucket), ) .await?; @@ -1165,4 +1192,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); + } }