.gitignore (2)

6-8: Track Cargo.lock for all CLI binaries

To ensure reproducible builds across the workspace’s binaries (e.g., crates/fluent-cli, crates/fluent-lambda, minesweeper_solitaire_game), remove Cargo.lock from .gitignore and commit it to version control. Verified that:

• The repository defines multiple binaries via async fn main() in:
  • src/main.rs
  • crates/fluent-cli/src/main.rs
  • crates/fluent-lambda/src/main.rs
  • minesweeper_solitaire_game/src/main.rs
• Cargo.lock is currently not tracked (absent from git index)

Locations needing updates:

• .gitignore (lines 6–8)

Suggested diff:

-.gitignore:6,7,8
-# Remove Cargo.lock from gitignore if creating an executable, leave it for libraries
-# More information here https://doc.rust-lang.org/cargo/guide/cargo-toml-vs-cargo-lock.html
-Cargo.lock
+.gitignore:6,7,8
+# For libraries, ignoring Cargo.lock is allowed. For workspace applications/CLIs, commit Cargo.lock for reproducible builds.
+# See: https://doc.rust-lang.org/cargo/guide/cargo-toml-vs-cargo-lock.html
+# (Remove the `Cargo.lock` ignore entry below)

Action items:

• Remove the above lines from .gitignore.
• Add and commit Cargo.lock at the workspace root.

---

101-105: Enhance .gitignore for secrets and verify secret hygiene

I ran the provided checks:

• key_safe* is not currently tracked in Git history.
• A scan for common secret patterns surfaced only placeholders (e.g., api_key: "test-key"), but no real private keys or tokens.

Please apply the following mandatory updates to harden our secret handling:

• In .gitignore (around lines 101–105), group and broaden the ignore rules under a dedicated “Secrets” section:

 # Large generated files
 enhanced_reflection_profiling_report.txt
 reasoning_engine_profiling_report.txt

+# Secrets (never commit)
+key_safe*

• Eliminate any plaintext secret storage. Instead, load credentials at runtime from an OS keychain or an encrypted vault (e.g., age/sops).
• Install a pre-commit secret scanner (for example, gitleaks or trufflehog) if not already configured in this PR’s tooling.

These changes will ensure continued protection against accidental exposure of sensitive material.

amber.yaml (1)

112-115: Enforce AMBER_ naming for GROK_API_KEY*

No occurrences of GROK_API_KEY were found outside of its definition in amber.yaml (line 112), and it breaks the established AMBER_* prefix convention that your secret loaders and provider integrations rely on. To ensure this key is picked up correctly, please:

• Rename the secret in amber.yaml to follow the prefix convention (for example, AMBER_GROK_API_KEY or AMBER_FLUENT_GROK_API_KEY_01).
• Update any consumer code or configuration to reference the new name.

This will align with the rest of your secrets (e.g., AMBER_REPO_CLOUD_FLUENT_DEMO_KEY) and prevent silent misconfigurations.

crates/fluent-agent/src/goal.rs (1)

268-279: Templates overwrite success criteria; base criteria get lost when you call success_criteria twice.

The second call to success_criteria replaces the first one in code_review, testing, and documentation templates, dropping the default criteria. This changes validation, complexity, and estimated duration heuristics.

Apply this refactor to add an appending builder method and use it at call sites:

@@
 impl GoalBuilder {
@@
     /// Add success criteria
     pub fn success_criteria(mut self, criteria: Vec<String>) -> Self {
         self.goal.success_criteria = criteria;
         self
     }
 
+    /// Append additional success criteria without replacing existing ones
+    pub fn append_success_criteria<I>(mut self, criteria: I) -> Self
+    where
+        I: IntoIterator<Item = String>,
+    {
+        self.goal.success_criteria.extend(criteria);
+        self
+    }
@@
     pub fn code_review(file_path: String, focus_areas: Vec<String>) -> Goal {
         Goal::builder(
             format!("Review code in {}", file_path),
             GoalType::CodeReview,
         )
         .priority(GoalPriority::Medium)
-        .success_criteria(vec![
+        .success_criteria(vec![
             "Identify potential issues".to_string(),
             "Suggest improvements".to_string(),
             "Check code quality".to_string(),
         ])
-        .success_criteria(
-            focus_areas
-                .iter()
-                .map(|area| format!("Review {}", area))
-                .collect(),
-        )
+        .append_success_criteria(focus_areas.into_iter().map(|area| format!("Review {}", area)))
         .max_iterations(15)
         .timeout(Duration::from_secs(300))
         .metadata("file_path".to_string(), serde_json::json!(file_path))
         .build_unchecked()
     }
@@
     pub fn testing(component: String, test_types: Vec<String>) -> Goal {
         Goal::builder(format!("Create tests for {}", component), GoalType::Testing)
             .priority(GoalPriority::Medium)
-            .success_criteria(vec![
+            .success_criteria(vec![
                 "Create comprehensive test suite".to_string(),
                 "Achieve good test coverage".to_string(),
                 "Tests pass successfully".to_string(),
             ])
-            .success_criteria(
-                test_types
-                    .iter()
-                    .map(|t| format!("Include {} tests", t))
-                    .collect(),
-            )
+            .append_success_criteria(test_types.into_iter().map(|t| format!("Include {} tests", t)))
             .max_iterations(20)
             .timeout(Duration::from_secs(480))
             .metadata("component".to_string(), serde_json::json!(component))
             .build_unchecked()
     }
@@
     pub fn documentation(scope: String, doc_types: Vec<String>) -> Goal {
         Goal::builder(
             format!("Create documentation for {}", scope),
             GoalType::Documentation,
         )
         .priority(GoalPriority::Low)
-        .success_criteria(vec![
+        .success_criteria(vec![
             "Create clear documentation".to_string(),
             "Include examples".to_string(),
             "Cover all features".to_string(),
         ])
-        .success_criteria(
-            doc_types
-                .iter()
-                .map(|t| format!("Include {} documentation", t))
-                .collect(),
-        )
+        .append_success_criteria(doc_types.into_iter().map(|t| format!("Include {} documentation", t)))
         .max_iterations(15)
         .timeout(Duration::from_secs(540))
         .metadata("scope".to_string(), serde_json::json!(scope))
         .build_unchecked()
     }

Also applies to: 336-358, 385-404, 423-444

Cargo.toml (1)

173-179: Align tempfile versions to avoid duplicate crates.

You pin tempfile = "3.13.0" in the workspace, but dev-dependencies here use "3.0". Prefer the workspace version to avoid two versions in the graph.

 [dev-dependencies]
-# Testing utilities
-tempfile = "3.0"
+# Testing utilities
+tempfile = { workspace = true }
 tokio-test = "0.4"
 assert_cmd = "2.0"
 predicates = "3.0"

Also applies to: 32-33

crates/fluent-agent/src/performance/cache.rs (4)

215-232: Fragile “availability” heuristic for Redis; misclassifies valid URLs.

Treating redis://localhost:6379 as “not available” is arbitrary and will surprise users—localhost is a perfectly valid deployment. Availability should reflect connectivity, not a hardcoded URL check.

Use “configured” semantics: consider the backend “available” when a non-empty URL is provided; log that it’s running in fallback (no real connection). Apply:

 pub fn new(url: String, ttl: Duration) -> Result<Self> {
-    // Check if Redis URL is provided and warn about fallback mode
-    let available = !url.is_empty() && url != "redis://localhost:6379";
+    // Consider non-empty URL as configured; real connectivity is not implemented yet.
+    let available = !url.is_empty();
 
-    if !available {
-        warn!("Redis cache initialized in fallback mode - Redis not available or not configured");
-        warn!("To enable Redis caching, add redis dependency and implement actual Redis connectivity");
-    } else {
-        debug!("Redis cache configured for URL: {} (fallback mode)", url);
-    }
+    if !available {
+        warn!("Redis cache initialized in fallback mode - Redis not configured (url: '{}')", url);
+        warn!("To enable Redis caching, add redis dependency and implement connectivity");
+    } else {
+        debug!("Redis cache configured for URL: {} (fallback mode)", url);
+    }
 
     Ok(Self {
         url,
         ttl,
         available,
         _phantom: std::marker::PhantomData,
     })
 }

---

294-313: SQLite in-memory detection is incorrect; mismatches the tests and common DSNs.

The code checks starts_with("sqlite://memory"), but tests use "sqlite::memory:". Common DSNs include sqlite::memory: and sqlite://:memory:. Current logic can incorrectly mark in-memory DBs as “available”.

Make the detection robust:

 pub fn new(url: String, ttl: Duration) -> Result<Self> {
-    // Check if database URL is provided and warn about fallback mode
-    let available = !url.is_empty() && !url.starts_with("sqlite://memory");
+    // Consider configured when non-empty; treat in-memory DSNs as "not available" for persistence
+    let available = !url.is_empty() && !url.contains(":memory:");
 
     if !available {
         warn!("Database cache initialized in fallback mode - Database caching not fully implemented");
         warn!("To enable database caching, implement actual database connectivity using sqlx");
     } else {
         debug!("Database cache configured for URL: {} (fallback mode)", url);
     }

---

321-360: Same fallback semantic issue for L3: return Ok on unavailability.

To keep behavior consistent and keep tests passing, mirror the Redis changes for the database cache.

 async fn get(&self, _key: &K) -> Result<Option<V>> {
-    if !self.available {
-        return Err(anyhow::anyhow!("Database cache not available: {}", self.url));
-    }
+    if !self.available {
+        warn!("Database cache not available: {}", self.url);
+        return Ok(None);
+    }
     ...
     Ok(None)
 }
 
 async fn set(&self, _key: &K, _value: &V, ttl: Duration) -> Result<()> {
-    if !self.available {
-        return Err(anyhow::anyhow!("Database cache not available: {}", self.url));
-    }
+    if !self.available {
+        warn!("Database cache not available: {}", self.url);
+        return Ok(());
+    }
     ...
     Ok(())
 }
 
 async fn remove(&self, _key: &K) -> Result<()> {
-    if !self.available {
-        return Err(anyhow::anyhow!("Database cache not available: {}", self.url));
-    }
+    if !self.available {
+        warn!("Database cache not available: {}", self.url);
+        return Ok(());
+    }
     ...
     Ok(())
 }
 
 async fn clear(&self) -> Result<()> {
-    if !self.available {
-        return Err(anyhow::anyhow!("Database cache not available: {}", self.url));
-    }
+    if !self.available {
+        warn!("Database cache not available: {}", self.url);
+        return Ok(());
+    }
     ...
     Ok(())
 }

---

241-280: Restore graceful fallback semantics for Redis cache methods

The get, set, remove, and clear methods in crates/fluent-agent/src/performance/cache.rs currently return Err when self.available is false, which contradicts both the module documentation’s promise of a “graceful fallback” and the existing tests asserting Ok(None) or Ok(()) in that scenario. This breaking change will cause test failures and unexpected behavior.

Affected locations:

• crates/fluent-agent/src/performance/cache.rs, methods get, set, remove, clear (around lines 241–280)

Proposed fix: replace Err(...) on unavailable cache with a warning log and an Ok return matching the original fallback contract.

 async fn get(&self, _key: &K) -> Result<Option<V>> {
-    if !self.available {
-        return Err(anyhow::anyhow!("Redis cache not available: {}", self.url));
-    }
+    if !self.available {
+        warn!("Redis cache not available: {}", self.url);
+        return Ok(None);
+    }
     // Redis implementation would go here …
     Ok(None)
 }

 async fn set(&self, _key: &K, _value: &V, ttl: Duration) -> Result<()> {
-    if !self.available {
-        return Err(anyhow::anyhow!("Redis cache not available: {}", self.url));
-    }
+    if !self.available {
+        warn!("Redis cache not available: {}", self.url);
+        return Ok(());
+    }
     // Redis implementation would go here …
     Ok(())
 }

 async fn remove(&self, _key: &K) -> Result<()> {
-    if !self.available {
-        return Err(anyhow::anyhow!("Redis cache not available: {}", self.url));
-    }
+    if !self.available {
+        warn!("Redis cache not available: {}", self.url);
+        return Ok(());
+    }
     // Redis implementation would go here …
     Ok(())
 }

 async fn clear(&self) -> Result<()> {
-    if !self.available {
-        return Err(anyhow::anyhow!("Redis cache not available: {}", self.url));
-    }
+    if !self.available {
+        warn!("Redis cache not available: {}", self.url);
+        return Ok(());
+    }
     // Redis implementation would go here …
     Ok(())
 }

crates/fluent-agent/src/orchestrator.rs (1)

730-816: Mock implementations at the bottom of production code.

Having test/mock implementations mixed with production code reduces maintainability. These should be in a separate test module or file.

Move mock implementations to a test module:

-/// Mock reasoning engine for testing and basic functionality
-struct MockReasoningEngine;
-
-#[async_trait::async_trait]
-impl ReasoningEngine for MockReasoningEngine {
-    // ... implementation
-}
-
-/// Mock engine for testing and configuration fallback
-struct MockEngine;
-
-impl fluent_core::traits::Engine for MockEngine {
-    // ... implementation
-}

+#[cfg(test)]
+mod mocks {
+    use super::*;
+    
+    /// Mock reasoning engine for testing and basic functionality
+    pub(super) struct MockReasoningEngine;
+    
+    #[async_trait::async_trait]
+    impl ReasoningEngine for MockReasoningEngine {
+        // ... implementation
+    }
+    
+    /// Mock engine for testing
+    pub(super) struct MockEngine;
+    
+    impl fluent_core::traits::Engine for MockEngine {
+        // ... implementation
+    }
+}
+
+#[cfg(test)]
+use mocks::{MockEngine, MockReasoningEngine};

crates/fluent-agent/src/mcp_resource_manager.rs (2)

417-432: Potential SSRF and unbounded fetch in read_http_resource; restrict hosts, add timeouts and size limits.

Allowing arbitrary http(s) fetches lets untrusted inputs hit internal networks (e.g., metadata IPs) and large bodies. Add allow/deny-lists, timeouts, and a max response size.

Example hardening:

- let client = reqwest::Client::new();
- let response = client.get(uri.as_str()).send().await?;
+ let client = reqwest::Client::builder()
+   .timeout(Duration::from_secs(10))
+   .build()?;
+ // Optional: enforce allowlist from config (e.g., self.config.allowed_http_hosts)
+ self.validate_http_target(uri)?;
+ let response = client.get(uri.as_str()).send().await?;

- let content = response.text().await?;
+ let bytes = response.bytes().await?;
+ let max = 1_048_576u64; // 1MB cap; consider self.config.max_http_bytes
+ if (bytes.len() as u64) > max {
+   return Err(anyhow!("HTTP response exceeds max size of {} bytes", max));
+ }
+ let content = String::from_utf8_lossy(&bytes).into_owned();

Also add:

fn validate_http_target(&self, uri: &Url) -> Result<()> {
  // Minimal example: block loopback and link-local
  if let Some(host) = uri.host_str() {
    if host == "localhost" || host == "127.0.0.1" {
      return Err(anyhow!("Access to localhost is not allowed"));
    }
  }
  Ok(())
}

Happy to wire this into ResourceManagerConfig as allow/deny lists.

---

434-445: file:// handling and path policy likely block legitimate absolute paths.

Url::parse("file:///crates/…") yields an absolute path starting with '/', but is_path_allowed() only allows relative prefixes. As written, most valid file URLs will be denied.

- fn is_path_allowed(&self, path: &str) -> bool {
-   // Basic security check - prevent path traversal
-   !path.contains("..") && (
-     path.starts_with("./") ||
-     path.starts_with("src/") ||
-     path.starts_with("crates/") ||
-     path.starts_with("examples/") ||
-     path == "README.md" ||
-     path == "Cargo.toml"
-   )
- }
+ fn is_path_allowed(&self, path: &str) -> bool {
+   if path.contains("..") { return false; }
+   // Normalize leading slash from file:// URLs
+   let p = path.strip_prefix('/').unwrap_or(path);
+   matches!(p,
+     _ if p.starts_with("src/") ||
+          p.starts_with("crates/") ||
+          p.starts_with("examples/") ||
+          p.starts_with("./") ||
+          p == "README.md" ||
+          p == "Cargo.toml"
+   )
+ }

Consider anchoring to a workspace root (canonicalize and ensure it’s under CARGO_WORKSPACE/).

Also applies to: 371-388

crates/fluent-agent/src/mcp_adapter.rs (1)

374-401: Path traversal and arbitrary write risks in read_file/write_file.

read_file/write_file accept arbitrary paths without normalization or sandboxing. An MCP client can read/write outside the workspace (e.g., “../../etc/shadow”).

Add canonicalization and workspace-root enforcement:

@@
-            "read_file" => {
-                if let Some(path) = tool_args.get("path") {
-                    match tokio::fs::read_to_string(path.as_str().unwrap_or("")).await {
+            "read_file" => {
+                if let Some(path) = tool_args.get("path") {
+                    let safe = validate_and_canonicalize_read(path.as_str().unwrap_or(""))?;
+                    match tokio::fs::read_to_string(&safe).await {
                         Ok(content) => format!("File content: {}", content),
                         Err(e) => format!("Error reading file: {}", e),
                     }
                 } else {
                     "Error: path parameter required".to_string()
                 }
             }
             "write_file" => {
-                if let Some(path) = tool_args.get("path") {
-                    if let Some(content) = tool_args.get("content") {
-                        match tokio::fs::write(path.as_str().unwrap_or(""), content.as_str().unwrap_or("")).await {
+                if let Some(path) = tool_args.get("path") {
+                    if let Some(content) = tool_args.get("content") {
+                        let safe = validate_and_canonicalize_write(path.as_str().unwrap_or(""))?;
+                        match tokio::fs::write(&safe, content.as_str().unwrap_or("")).await {
                             Ok(_) => "File written successfully".to_string(),
                             Err(e) => format!("Error writing file: {}", e),
                         }
                     } else {
                         "Error: content parameter required".to_string()
                     }
                 } else {
                     "Error: path parameter required".to_string()
                 }
             }

Add helpers (outside the shown range):

fn workspace_root() -> std::path::PathBuf {
    std::env::var_os("FLUENT_WORKSPACE_ROOT")
        .map(std::path::PathBuf::from)
        .unwrap_or_else(|| std::env::current_dir().unwrap_or_else(|_| std::path::PathBuf::from(".")))
}

fn validate_and_canonicalize_read(p: &str) -> anyhow::Result<std::path::PathBuf> {
    use std::path::Path;
    let root = workspace_root().canonicalize()?;
    let path = root.join(p);
    let canon = path.canonicalize()?;
    if !canon.starts_with(&root) {
        anyhow::bail!("path escapes workspace root");
    }
    Ok(canon)
}

fn validate_and_canonicalize_write(p: &str) -> anyhow::Result<std::path::PathBuf> {
    use std::path::Path;
    let root = workspace_root().canonicalize()?;
    let path = root.join(p);
    let parent = path.parent().unwrap_or(&root).to_path_buf().canonicalize().unwrap_or(root);
    if !parent.starts_with(workspace_root().canonicalize()?) {
        anyhow::bail!("path escapes workspace root");
    }
    Ok(path)
}

crates/fluent-agent/src/lib.rs (1)

331-342: Route git_commit through the same secure execution path (run_command).

git_commit bypasses security controls (env clear, timeouts, output caps). Keep policy consistent.

@@
     pub async fn git_commit(&self, message: &str) -> Result<()> {
-        self.run_command("git", &["add", "."]).await?;
-        let status = Command::new("git")
-            .args(["commit", "-m", message])
-            .status()
-            .await?;
-        if !status.success() {
-            return Err(anyhow!("git commit failed"));
-        }
-        Ok(())
+        // Use secure path for both commands
+        let _ = self.run_command("git", &["add", "."]).await?;
+        let output = self.run_command("git", &["commit", "-m", message]).await?;
+        // Heuristic: treat common failure outputs as errors
+        if output.to_lowercase().contains("error:") || output.to_lowercase().contains("fatal:") {
+            return Err(anyhow!("git commit failed: {}", output));
+        }
+        Ok(())
     }

Optionally, expose a run_command_status(cmd, args) helper that surfaces exit codes while keeping the same security envelope.

Also applies to: 144-234