fix(review): address PR review findings

- main.rs: Replace unwrap_or_default() with .context() for proper error
  propagation (malformed config.toml now fails early instead of silently
  falling through to production defaults)
- apply.rs: Extract DRAIN_RETRY_ATTEMPTS/DRAIN_RETRY_INTERVAL constants;
  remove dead post-loop code by returning error on last attempt directly
- delete.rs: Extract DRAIN_POLL_ATTEMPTS/DRAIN_POLL_INTERVAL constants

Resolves merge conflicts by rebasing onto current main (which already has
typed error codes and proper transient error handling).

Author: chaodu-agent

operator/src/apply.rs

@@ -543,14 +543,15 @@ async fn apply_ecs(
         // Match on the typed error code (InvalidParameterException) rather than
         // raw message text to be resilient to SDK/API wording changes.
         use aws_sdk_ecs::error::ProvideErrorMetadata;
-        let mut last_err = None;
-        for attempt in 0..12 {
+        const DRAIN_RETRY_ATTEMPTS: u32 = 12;
+        const DRAIN_RETRY_INTERVAL: std::time::Duration = std::time::Duration::from_secs(5);
+
+        for attempt in 0..DRAIN_RETRY_ATTEMPTS {
             match create_req.clone().send().await {
                 Ok(_) => {
                     if attempt > 0 {
                         eprintln!(" ok");
                     }
-                    last_err = None;
                     break;
                 }
                 Err(e) => {
@@ -559,29 +560,28 @@ async fn apply_ecs(
                             .unwrap_or_default()
                             .to_lowercase()
                             .contains("draining");
-                    if is_draining {
+                    let is_last = attempt == DRAIN_RETRY_ATTEMPTS - 1;
+                    if is_draining && !is_last {
                         if attempt == 0 {
                             eprint!("  ⏳ Service still draining, retrying...");
                         } else {
                             eprint!(".");
                         }
-                        last_err = Some(e);
-                        if attempt < 11 {
-                            tokio::time::sleep(std::time::Duration::from_secs(5)).await;
-                        }
+                        tokio::time::sleep(DRAIN_RETRY_INTERVAL).await;
                     } else {
                         if attempt > 0 {
                             eprintln!(" failed");
                         }
-                        return Err(e).context("failed to create ECS service");
+                        let ctx = if is_last && is_draining {
+                            "failed to create ECS service after retries (service still draining)"
+                        } else {
+                            "failed to create ECS service"
+                        };
+                        return Err(e).context(ctx);
                     }
                 }
             }
         }
-        if let Some(e) = last_err {
-            eprintln!(" timed out");
-            return Err(anyhow::anyhow!(e)).context("failed to create ECS service after retries");
-        }
         println!(
             "  ✓ {} created ({}, {}cpu/{}mem{})",
             m.metadata.name,

operator/src/delete.rs

@@ -91,8 +91,11 @@ pub async fn run(
     // 2a. Wait for the service to fully drain (INACTIVE status) so that
     // a subsequent `apply` doesn't hit "Unable to Start a service that
     // is still Draining".
+    const DRAIN_POLL_ATTEMPTS: u32 = 12;
+    const DRAIN_POLL_INTERVAL: std::time::Duration = std::time::Duration::from_secs(5);
+
     eprint!("  ⏳ Waiting for drain to complete...");
-    for i in 0..12 {
+    for i in 0..DRAIN_POLL_ATTEMPTS {
         // Check status first, then sleep — avoids an unnecessary initial delay
         // when the service transitions quickly.
         let resp = ecs
@@ -116,16 +119,16 @@ pub async fn run(
             if i == 0 {
                 eprintln!(" done (immediate)");
             } else {
-                let elapsed = i * 5;
+                let elapsed = u64::from(i) * DRAIN_POLL_INTERVAL.as_secs();
                 eprintln!(" done ({elapsed}s)");
             }
             break;
         }
-        if i == 11 {
+        if i == DRAIN_POLL_ATTEMPTS - 1 {
             eprintln!(" timed out (service may still be draining)");
         } else {
             eprint!(".");
-            tokio::time::sleep(std::time::Duration::from_secs(5)).await;
+            tokio::time::sleep(DRAIN_POLL_INTERVAL).await;
         }
     }
 

operator/src/main.rs

@@ -49,9 +49,9 @@ enum Commands {
         resource: String,
         /// Optional resource name
         name: Option<String>,
-        /// ECS cluster name
-        #[arg(long, default_value = "default")]
-        cluster: String,
+        /// ECS cluster name (default: from ~/.oabctl/config.toml)
+        #[arg(long)]
+        cluster: Option<String>,
     },
     /// Delete an OAB service
     Delete {
@@ -63,13 +63,12 @@ enum Commands {
         /// instead of specifying <resource> <name> directly (mirrors `apply -f`)
         #[arg(short, long, conflicts_with_all = ["resource", "name"])]
         file: Option<String>,
-        /// ECS cluster name (ignored when using -f — manifests are always
-        /// deployed to the "oab" cluster, same as `apply`)
-        #[arg(long, default_value = "default")]
-        cluster: String,
+        /// ECS cluster name (default: from ~/.oabctl/config.toml; ignored when using -f)
+        #[arg(long)]
+        cluster: Option<String>,
         /// Namespace (ignored when using -f — read from each manifest instead)
-        #[arg(long, default_value = "prod")]
-        namespace: String,
+        #[arg(long)]
+        namespace: Option<String>,
     },
     /// Execute a command in an agent container (via ecsctl)
     Exec {
@@ -133,13 +132,20 @@ async fn main() -> anyhow::Result<()> {
     match cli.command {
         Commands::Apply { file, no_sync, wait } => apply::run(&config, &file, !no_sync, wait).await,
         Commands::Create { name, namespace, auto_apply } => create::run(&config, &name, &namespace, auto_apply).await,
-        Commands::Get { resource, name, cluster } => get::run(&config, &resource, name.as_deref(), &cluster).await,
+        Commands::Get { resource, name, cluster } => {
+            let oab_cfg = config::OabConfig::load().context("failed to load ~/.oabctl/config.toml")?;
+            let cluster = cluster.unwrap_or(oab_cfg.defaults.cluster);
+            get::run(&config, &resource, name.as_deref(), &cluster).await
+        }
         Commands::Delete { resource, name, file, cluster, namespace } => {
             if let Some(file) = file {
                 delete::run_from_file(&config, &file).await
             } else {
                 let resource = resource.context("<RESOURCE> is required when not using -f")?;
                 let name = name.context("<NAME> is required when not using -f")?;
+                let oab_cfg = config::OabConfig::load().context("failed to load ~/.oabctl/config.toml")?;
+                let cluster = cluster.unwrap_or(oab_cfg.defaults.cluster);
+                let namespace = namespace.unwrap_or(oab_cfg.defaults.namespace);
                 delete::run(&config, &resource, &name, &cluster, &namespace).await
             }
         }