feat: enhance prompts and validation for tool-call safety; enforce error handling and technical debt awareness

src/app/harness.rs

@@ -1,5 +1,7 @@
 //! Tool-call gating: decides whether a risky tool call is allowed to run
-//! before it executes.
+//! before it executes. Implements hooks-style pre-checks for write/edit/delete
+//! and bash tools so the agent cannot silently introduce stubs, denial
+//! patterns, assumption language, or destructive commands.
 
 /// Outcome of gating a tool call: whether it's allowed to run.
 #[derive(Debug, Clone, PartialEq)]
@@ -11,12 +13,111 @@ pub enum Verdict {
 /// Gatekeeper that decides whether a tool call may proceed before execution.
 pub struct Harness;
 
+/// Stub / placeholder / denial / assumption patterns that should never reach
+/// a file in real code. Detected in write/edit content and bash heredocs.
+const STUB_PATTERNS: &[&str] = &[
+    "todo!()",
+    "todo!(",
+    "unimplemented!()",
+    "unimplemented!(",
+    "todo_macro",
+    "FIXME",
+    "fixme:",
+    "XXX:",
+    "PLACEHOLDER",
+    "REPLACE_ME",
+    "stub_value",
+    "stub_function",
+    "fake_response",
+    "fake_data",
+    "not implemented",
+    "not yet implemented",
+    "to be implemented",
+    "to be done",
+];
+
+/// Language patterns indicating the AI is denying responsibility or
+/// punting the work ("I'll skip this", "for now just", etc).
+const DENIAL_PATTERNS: &[&str] = &[
+    "// skip",
+    "// skipping",
+    "// skipping for now",
+    "// for now just",
+    "// punt",
+    "// punted",
+    "// hack:",
+    "// hacky",
+    "// hack workaround",
+    "// workaround:",
+    "// cba",
+    "// later",
+    "// do later",
+    "// ignore for now",
+    "// disable",
+    "// disabled",
+    "// bypass",
+    "// quick fix",
+    "// temp fix",
+    "// temporary fix",
+    "// temp:",
+    "// temporary:",
+    "// noop",
+];
+
+/// Assumption-language patterns: words/phrases that indicate the code is
+/// reasoning based on guesswork rather than data.
+const ASSUMPTION_PATTERNS: &[&str] = &[
+    "// assume",
+    "// assuming",
+    "// probably",
+    "// maybe",
+    "// might",
+    "// should work",
+    "// hopefully",
+    "// guess",
+    "// i think",
+    "// should be fine",
+    "// should be",
+    "// likely",
+    "// ought to",
+];
+
+/// Network-exfiltration and credential-disclosure patterns for bash.
+const EXFIL_PATTERNS: &[&str] = &[
+    "curl ", "wget ", "nc -e ", "ncat ", "/dev/tcp/",
+    "base64 -d |", "base64 --decode |",
+    "openssl s_client", "ssh -R ",
+    "scp /", "rsync /",
+];
+
+/// Substrings of well-known credential / secret files that bash must not read.
+const SENSITIVE_PATH_PATTERNS: &[&str] = &[
+    ".ssh/id_rsa",
+    ".ssh/id_ed25519",
+    ".ssh/authorized_keys",
+    ".aws/credentials",
+    ".aws/config",
+    ".netrc",
+    ".pypirc",
+    ".npmrc",
+    ".kube/config",
+    ".docker/config.json",
+    ".gnupg/",
+    "/etc/shadow",
+    "/etc/passwd",
+    "/proc/self/environ",
+];
+
+/// Minimum character length of a `reason` argument to be considered meaningful.
+const MIN_REASON_LEN: usize = 8;
+
 impl Harness {
     /// Decide whether a tool call is allowed to execute.
     ///
-    /// Flow: if the tool isn't flagged risky, allow immediately → basic
-    /// content checks (path traversal in paths AND command args) →
-    /// workspace-root validation for output paths → classify.
+    /// Flow: if the tool isn't flagged risky, allow immediately → file-tool
+    /// reason & path checks → content stub / denial / assumption scan →
+    /// bash destructive-pattern & exfiltration scan → workspace-root
+    /// validation for output paths.
     ///
     /// Return: `Verdict::Allow` or `Verdict::Block(reason)`.
     pub fn gate_tool_call(
@@ -28,31 +129,137 @@ impl Harness {
         if !crate::tool::tool_is_risky(tool_name) {
             return Verdict::Allow;
         }
-        // Path traversal check for file-mutating tools.
+
+        // File-mutating tools: write / edit / delete
         if matches!(tool_name, "write" | "edit" | "delete") {
             if let Some(path) = args.get("path").and_then(|v| v.as_str()) {
                 if path.contains("..") {
-                    return Verdict::Block("path traversal detected in 'path' argument".to_string());
+                    return Verdict::Block(
+                        "path traversal detected in 'path' argument".to_string(),
+                    );
+                }
+                if !workspace_roots.is_empty() {
+                    let abs_check = std::path::PathBuf::from(path);
+                    if abs_check.is_absolute()
+                        && !workspace_roots.iter().any(|r| abs_check.starts_with(r))
+                    {
+                        return Verdict::Block(format!(
+                            "absolute path '{path}' is outside all workspace roots"
+                        ));
+                    }
                 }
             }
         }
-        // Path traversal and dangerous content check for bash commands.
+
+        // write / edit require a non-trivial `reason` argument (hooks-style
+        // discipline: every mutation must explain itself).
+        if matches!(tool_name, "write" | "edit" | "delete") {
+            match Self::validate_reason(tool_name, args) {
+                Ok(()) => {}
+                Err(msg) => return Verdict::Block(msg),
+            }
+        }
+
+        // write / edit content must not contain stubs, denial language, or
+        // assumption language.
+        if matches!(tool_name, "write" | "edit") {
+            if let Some(content) = Self::extract_content(tool_name, args) {
+                if let Some(pat) = Self::first_match(&content, STUB_PATTERNS) {
+                    return Verdict::Block(format!(
+                        "content contains stub/placeholder pattern '{pat}'; \
+                         production code must be fully implemented — \
+                         replace the stub with a real implementation"
+                    ));
+                }
+                if let Some(pat) = Self::first_match(&content, DENIAL_PATTERNS) {
+                    return Verdict::Block(format!(
+                        "content contains denial/punt pattern '{pat}'; \
+                         implement the change properly instead of skipping"
+                    ));
+                }
+                if let Some(pat) = Self::first_match(&content, ASSUMPTION_PATTERNS) {
+                    return Verdict::Block(format!(
+                        "content contains assumption pattern '{pat}'; \
+                         verify against data/tests instead of guessing"
+                    ));
+                }
+            }
+        }
+
+        // Bash: destructive patterns, exfiltration, sensitive-path reads.
         if tool_name == "bash" {
             let cmd = args.get("command").and_then(|v| v.as_str()).unwrap_or("");
             if cmd.contains("..") {
-                return Verdict::Block("path traversal detected in bash command".to_string());
+                return Verdict::Block(
+                    "path traversal detected in bash command".to_string(),
+                );
+            }
+            if !cmd.trim_start().starts_with("cargo")
+                && !cmd.trim_start().starts_with("rustc")
+                && !cmd.trim_start().starts_with("git ")
+                && !cmd.trim_start().starts_with("ls")
+                && !cmd.trim_start().starts_with("pwd")
+                && !cmd.trim_start().starts_with("echo")
+                && !cmd.trim_start().starts_with("cat")
+                && !cmd.trim_start().starts_with("find")
+                && !cmd.trim_start().starts_with("grep")
+                && !cmd.trim_start().starts_with("test")
+            {
+                for pat in EXFIL_PATTERNS {
+                    if cmd.contains(pat) {
+                        return Verdict::Block(format!(
+                            "potential data-exfiltration command blocked (matched '{pat}')"
+                        ));
+                    }
+                }
+            }
+            for pat in SENSITIVE_PATH_PATTERNS {
+                if cmd.contains(pat) {
+                    return Verdict::Block(format!(
+                        "refused to read/write sensitive path '{pat}'"
+                    ));
+                }
             }
             let dangerous_patterns = [
                 "rm -rf /", "rm -rf --no-preserve-root",
                 "rm -rf ~", "rm -fr /", "mkfs.", "dd if=",
                 ":(){", "> /dev/sda", "chmod -R 000 /",
+                "shutdown ", "poweroff ", "reboot ", "halt ",
             ];
             for pat in &dangerous_patterns {
                 if cmd.contains(pat) {
-                    return Verdict::Block(format!("destructive command pattern blocked: {}", pat));
+                    return Verdict::Block(format!(
+                        "destructive command pattern blocked: {pat}"
+                    ));
                 }
             }
+            // Also scan heredocs / -c / inline content for stub/denial
+            // language (e.g. `bash -c 'echo todo!()'`)
+            if let Some(pat) = Self::first_match(cmd, STUB_PATTERNS) {
+                return Verdict::Block(format!(
+                    "bash command contains stub pattern '{pat}'"
+                ));
+            }
         }
+
+        // git_operator: require a non-trivial reason as well.
+        if tool_name == "git_operator" {
+            if let Some(reason) = args.get("reason").and_then(|v| v.as_str()) {
+                if reason.trim().len() < MIN_REASON_LEN {
+                    return Verdict::Block(format!(
+                        "git_operator requires a non-trivial 'reason' \
+                         (>= {MIN_REASON_LEN} chars) explaining the operation"
+                    ));
+                }
+            } else {
+                return Verdict::Block(
+                    "git_operator requires a 'reason' argument explaining the operation"
+                        .to_string(),
+                );
+            }
+        }
+
+        // Workspace-root validation for the resolved output path.
         if let Some(out_path) = Self::find_output_path(tool_name, args) {
             if !workspace_roots.is_empty()
                 && !out_path.starts_with("/tmp")
@@ -61,7 +268,8 @@ impl Harness {
                 let allowed = workspace_roots.iter().any(|r| out_path.starts_with(r));
                 if !allowed {
                     return Verdict::Block(format!(
-                        "output path '{:?}' is outside all workspace roots", out_path
+                        "output path '{:?}' is outside all workspace roots",
+                        out_path
                     ));
                 }
             }
@@ -69,9 +277,83 @@ impl Harness {
         Self::classify(tool_name)
     }
 
-    /// Extract a candidate output path from a tool call, if one exists.
+    /// Validate the `reason` argument for a mutating tool.
     ///
-    /// Used to verify that writes and file mutations stay inside workspace roots.
+    /// Flow: require the field to exist and be a non-empty string ≥
+    /// `MIN_REASON_LEN` chars after trimming.
+    ///
+    /// Why: hook-style gates force the agent to articulate the *why* of
+    /// every change, which both deters lazy writes and produces a useful
+    /// audit trail in the edit log.
+    fn validate_reason(tool_name: &str, args: &serde_json::Value) -> Result<(), String> {
+        let reason = match args.get("reason") {
+            None => {
+                return Err(format!(
+                    "{tool_name} requires a non-empty 'reason' argument \
+                     explaining why the change is being made"
+                ));
+            }
+            Some(v) => match v.as_str() {
+                Some(s) => s,
+                None => {
+                    return Err(format!(
+                        "{tool_name} 'reason' must be a string"
+                    ));
+                }
+            },
+        };
+        let trimmed = reason.trim();
+        if trimmed.is_empty() {
+            return Err(format!(
+                "{tool_name} 'reason' must not be empty"
+            ));
+        }
+        if trimmed.len() < MIN_REASON_LEN {
+            return Err(format!(
+                "{tool_name} 'reason' must be at least {MIN_REASON_LEN} chars \
+                 (got {}) — explain WHY, not just WHAT",
+                trimmed.len()
+            ));
+        }
+        // Reject generic non-answers
+        let lower = trimmed.to_lowercase();
+        let non_answers = [
+            "fix", "update", "change", "edit", "modify",
+            "implement", "add", "remove", "delete",
+            "make it work", "make work", "test", "wip", "tbd",
+        ];
+        if non_answers.iter().any(|n| lower == *n) {
+            return Err(format!(
+                "{tool_name} 'reason' '{trimmed}' is too generic — \
+                 describe what changes and why (e.g. 'switch to Result<T> for \
+                 safer error propagation per user request')"
+            ));
+        }
+        Ok(())
+    }
+
+    /// Extract the textual content of a write/edit call, if any.
+    fn extract_content(tool_name: &str, args: &serde_json::Value) -> Option<String> {
+        match tool_name {
+            "write" => args.get("content").and_then(|v| v.as_str()).map(String::from),
+            "edit" => {
+                let old = args.get("old").and_then(|v| v.as_str()).unwrap_or("");
+                let new = args.get("new").and_then(|v| v.as_str()).unwrap_or("");
+                Some(format!("{old}\n{new}"))
+            }
+            _ => None,
+        }
+    }
+
+    /// Return the first pattern (case-insensitive substring) that matches
+    /// `text`, or `None` if no pattern matched.
+    fn first_match(text: &str, patterns: &'static [&'static str]) -> Option<&'static str> {
+        let lower = text.to_lowercase();
+        let iter: std::slice::Iter<'static, &'static str> = patterns.iter();
+        iter.copied().find(|p| lower.contains(&p.to_lowercase()))
+    }
+
+    /// Extract a candidate output path from a tool call, if one exists.
     fn find_output_path(tool_name: &str, args: &serde_json::Value) -> Option<std::path::PathBuf> {
         match tool_name {
             "write" | "edit" | "delete" | "read" => {

src/app/runtime/actions/mod.rs

@@ -307,7 +307,7 @@ pub fn apply_action(state: &mut AppStateRest, action: Action) {
             // every ~1s while disconnected, every ~30s while connected, so the
             // status bar reflects real API availability without user input.
             let check_interval = if state.misc.api_connected { 600 } else { 20 };
-            if state.misc.tick_count % check_interval == 0 {
+            if state.misc.tick_count.is_multiple_of(check_interval) {
                 spawn_api_connectivity_check(state);
             }
     crate::app::review::maybe_run_staleness_sweep(state);