Review fixes: install paths, spending overflow, approval prompt, session records

Author: chandan25809-lyzr

README.md

@@ -565,7 +565,7 @@ With `learning: online`, gitagent learns from each task it completes. The model
 
 ### Output, records and events
 
-- **Records:** every run writes `trajectory.json` (the ATIF trajectory format), `counters.json` and the full output of every command (`out/<n>.log`) to `--out-dir`. They are updated after every turn.
+- **Records:** every run writes `trajectory.json` (the ATIF trajectory format), `counters.json` and the full output of every command (`out/<n>.log`) to `--out-dir`. They are updated after every turn. The console gives each session its own folder, `console-<time>-<pid>/task-<n>/`, and prints its path at the start.
 - **Events:** `--events jsonl` prints one JSON object per line on stdout: `turn_start`, `model_text`, `tool_call`, `tool_result`, `stop_check`, `guard`, `turn_end`, `repo`, `run_end`.
 - **Telemetry:** set `OTEL_EXPORTER_OTLP_ENDPOINT` to export traces and metrics (OTLP over HTTP/JSON). `OTEL_EXPORTER_OTLP_HEADERS` and `OTEL_SERVICE_NAME` are honoured.
 

crates/cli/src/approval.rs

@@ -45,16 +45,14 @@ impl Approver for TtyApprover {
         while rx.try_recv().is_ok() {}
         let st = crate::console::Style::for_stderr();
         let question = format!(
-            "\n{} {}\n  {}\n  {} ",
+            "\n{} {}\n{}  {}\n  {} ",
             st.orange("!"),
             st.bold(&format!(
                 "approval needed · {}",
-                crate::console::tool_line(
-                    &request.tool_name,
-                    Some(&serde_json::Value::Object(request.arguments.clone()))
-                )
+                visible(&request.tool_name)
             )),
-            st.dim(&request.reason),
+            details(&request.arguments),
+            st.dim(&visible(&request.reason)),
             st.orange("approve? [y/N]")
         );
         let shown = std::fs::OpenOptions::new()
@@ -79,3 +77,96 @@ impl Approver for TtyApprover {
         }
     }
 }
+
+/// The longest argument shown whole; a longer one keeps its start and end.
+const SHOW_CHARS: usize = 4000;
+
+/// Every argument of the call, in full, one per block: what the person is
+/// approving, not a one-line summary of it.
+fn details(args: &serde_json::Map<String, serde_json::Value>) -> String {
+    let mut out = String::new();
+    for (key, value) in args {
+        let text = match value {
+            serde_json::Value::String(s) => s.clone(),
+            other => serde_json::to_string_pretty(other).unwrap_or_default(),
+        };
+        out.push_str(&format!("  {}:\n", visible(key)));
+        for line in shorten(&visible(&text)).lines() {
+            out.push_str(&format!("    {line}\n"));
+        }
+        if text.is_empty() {
+            out.push_str("    (empty)\n");
+        }
+    }
+    out
+}
+
+/// `text` whole, or its start and end with the number of characters left
+/// out said plainly, so nothing is hidden without the person knowing.
+fn shorten(text: &str) -> String {
+    let n = text.chars().count();
+    if n <= SHOW_CHARS {
+        return text.to_string();
+    }
+    let head: String = text.chars().take(SHOW_CHARS * 3 / 4).collect();
+    let tail: String = text.chars().skip(n - SHOW_CHARS / 4).collect();
+    format!(
+        "{head}\n[… {} characters not shown; answer n if unsure …]\n{tail}",
+        n - SHOW_CHARS
+    )
+}
+
+/// `text` with everything that could change how the terminal shows it made
+/// visible: escape sequences, carriage returns, other control characters
+/// and the Unicode marks that reorder text. Newlines and tabs stay.
+fn visible(text: &str) -> String {
+    text.chars()
+        .map(|c| match c {
+            '\n' | '\t' => c.to_string(),
+            '\x1b' => "␛".to_string(),
+            '\r' => "␍".to_string(),
+            c if c.is_control()
+                || matches!(c, '\u{200e}' | '\u{200f}' | '\u{202a}'..='\u{202e}' | '\u{2066}'..='\u{2069}') =>
+            {
+                format!("\\u{{{:04x}}}", c as u32)
+            }
+            c => c.to_string(),
+        })
+        .collect()
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn the_prompt_shows_every_argument_in_full_and_makes_controls_visible() {
+        let long_cmd = format!("echo {}; rm -rf ~/precious", "x".repeat(200));
+        let args = serde_json::json!({"cmd": long_cmd})
+            .as_object()
+            .unwrap()
+            .clone();
+        let shown = details(&args);
+        assert!(
+            shown.contains("rm -rf ~/precious"),
+            "nothing past 120 characters may be hidden"
+        );
+
+        let args = serde_json::json!({"path": "a.sh", "content": "echo hi\nrm -rf /"})
+            .as_object()
+            .unwrap()
+            .clone();
+        let shown = details(&args);
+        assert!(
+            shown.contains("content:") && shown.contains("rm -rf /"),
+            "{shown}"
+        );
+
+        assert_eq!(visible("ok\x1b[2K\rsafe"), "ok␛[2K␍safe");
+        assert_eq!(visible("a\u{202e}b"), "a\\u{202e}b");
+
+        let huge = "y".repeat(SHOW_CHARS * 2);
+        let cut = shorten(&huge);
+        assert!(cut.contains(&format!("{} characters not shown", SHOW_CHARS)));
+    }
+}

crates/cli/src/install_cmd.rs

@@ -66,9 +66,12 @@ pub(crate) fn run(args: &[std::ffi::OsString]) -> ExitCode {
         report("parent", install_parent(&root, src, cli.force));
     }
     for dep in manifest["dependencies"].as_array().into_iter().flatten() {
-        let Some((name, mount)) = dependency_mount(&root, dep) else {
-            report("dependency", Err("has no `name`".into()));
-            continue;
+        let (name, mount) = match dependency_mount(&root, dep) {
+            Ok(m) => m,
+            Err(e) => {
+                report("dependency", Err(e));
+                continue;
+            }
         };
         let source = dep["source"].as_str().map(str::trim).unwrap_or_default();
         let version = dep["version"].as_str().map(str::trim);
@@ -126,6 +129,13 @@ fn install_dependency(
             mount.display()
         ));
     }
+    // Something else is there: only `--force` may replace it.
+    if mount.exists() && !force {
+        return Err(format!(
+            "{} exists and is not an installed agent; move it away or use --force to replace it",
+            mount.display()
+        ));
+    }
     if mount.exists() {
         std::fs::remove_dir_all(mount).map_err(|e| e.to_string())?;
     }

crates/cli/src/plugin_cmd.rs

@@ -104,6 +104,13 @@ fn install(
         .trim_end_matches(".git")
         .to_string();
     let name = name.unwrap_or(&base);
+    for n in [name, base.as_str()] {
+        if !gitagent_loader::manifest::is_plain_name(n) {
+            return Err(format!(
+                "the plugin name {n:?} must not contain `/`, `\\` or be `.` or `..`; pass --name"
+            ));
+        }
+    }
     let mut fields = vec![("enabled", Value::Bool(enable))];
     let dest = if is_git_source(source) {
         let dest = root.join(".gitagent/plugins").join(&base);
@@ -200,6 +207,9 @@ fn list(root: &Path) -> Result<String, String> {
 }
 
 fn remove(root: &Path, name: &str) -> Result<String, String> {
+    if !gitagent_loader::manifest::is_plain_name(name) {
+        return Err(format!("{name:?} is not a plugin name"));
+    }
     let mut removed = None;
     for dir in [
         root.join("plugins").join(name),

crates/cli/src/repl.rs

@@ -48,6 +48,18 @@ pub(crate) async fn run(s: Setup, out_dir: &Path) -> Result<()> {
     let mut resume: Option<Resume> = None;
     let mut n = 0;
     let session_id = format!("gitagent-{}", std::process::id());
+    // Each console session keeps its records in a folder of its own, so a
+    // new session never overwrites an earlier one's.
+    let session_dir = out_dir.join(format!(
+        "console-{}-{}",
+        gitagent_record::utc_now_rfc3339()
+            .chars()
+            .take(19)
+            .collect::<String>()
+            .replace(':', ""),
+        std::process::id()
+    ));
+    eprintln!("{}", st.dim(&format!("records: {}", session_dir.display())));
     let mcp = crate::connect_mcp(&s.spec).await;
     loop {
         let line = if editing {
@@ -94,7 +106,7 @@ pub(crate) async fn run(s: Setup, out_dir: &Path) -> Result<()> {
             let opts = RunOptions {
                 instruction: String::new(),
                 work_dir: s.work_dir.clone(),
-                out_dir: out_dir.join(format!("task-{n}")),
+                out_dir: session_dir.join(format!("task-{n}")),
                 timeout: s.timeout_set,
                 session_id: Some(session_id.clone()),
                 events: s.events.clone(),
@@ -147,7 +159,7 @@ pub(crate) async fn run(s: Setup, out_dir: &Path) -> Result<()> {
         let opts = RunOptions {
             instruction: task,
             work_dir: s.work_dir.clone(),
-            out_dir: out_dir.join(format!("task-{n}")),
+            out_dir: session_dir.join(format!("task-{n}")),
             timeout: s.timeout_set,
             session_id: Some(session_id.clone()),
             events: events.clone(),
@@ -191,7 +203,7 @@ pub(crate) async fn run(s: Setup, out_dir: &Path) -> Result<()> {
         let opts = RunOptions {
             instruction: String::new(),
             work_dir: s.work_dir.clone(),
-            out_dir: out_dir.join("teardown"),
+            out_dir: session_dir.join("teardown"),
             timeout: s.timeout_set,
             session_id: Some(session_id.clone()),
             events: s.events.clone(),

crates/cli/tests/cli.rs

@@ -251,7 +251,18 @@ fn interactive_session_continues_between_tasks() {
         std::fs::read_to_string(dir.path().join("x.txt")).unwrap(),
         "7\n"
     );
-    assert!(dir.path().join("logs/task-2/trajectory.json").exists());
+    let session = std::fs::read_dir(dir.path().join("logs"))
+        .unwrap()
+        .flatten()
+        .map(|e| e.path())
+        .find(|p| {
+            p.file_name()
+                .unwrap()
+                .to_string_lossy()
+                .starts_with("console-")
+        })
+        .expect("a session folder");
+    assert!(session.join("task-2/trajectory.json").exists());
 }
 
 #[test]
@@ -1703,7 +1714,23 @@ fn interactive_commands() {
             && stderr.contains("● completed · 1 model call"),
         "{stderr}"
     );
-    let t = std::fs::read_to_string(dir.path().join("out/task-1/trajectory.json")).unwrap();
+    // Each console session writes under its own `console-…` folder.
+    let session = std::fs::read_dir(dir.path().join("out"))
+        .unwrap()
+        .flatten()
+        .map(|e| e.path())
+        .find(|p| {
+            p.file_name()
+                .unwrap()
+                .to_string_lossy()
+                .starts_with("console-")
+        })
+        .expect("a session folder");
+    assert!(
+        stderr.contains(&format!("records: {}", session.display())),
+        "{stderr}"
+    );
+    let t = std::fs::read_to_string(session.join("task-1/trajectory.json")).unwrap();
     assert!(
         t.contains("Use the skill `release-notes` for this request")
             && t.contains("Group the changes.")
@@ -2294,3 +2321,59 @@ fn schedules_are_listed_run_and_fired_when_due() {
     assert_eq!(code, Some(0));
     assert!(out.contains("removed") && !agent.join("schedules/ticker.yaml").exists());
 }
+
+/// `gitagent install` never deletes outside the agent's folder, and inside
+/// it replaces something that is not an installed agent only with --force.
+#[test]
+fn install_keeps_to_the_agent_folder() {
+    let dir = tempfile::tempdir().unwrap();
+    let precious = dir.path().join("precious");
+    std::fs::create_dir_all(&precious).unwrap();
+    std::fs::write(precious.join("data.txt"), "important").unwrap();
+    let agent = dir.path().join("agent");
+    std::fs::create_dir_all(agent.join("agents/local")).unwrap();
+    std::fs::write(agent.join("agents/local/notes.txt"), "mine").unwrap();
+    let absolute = precious.display().to_string();
+    for (mount, name) in [
+        ("../precious", "up"),
+        (absolute.as_str(), "abs"),
+        ("agents/local", "local"),
+    ] {
+        std::fs::write(
+            agent.join("agent.yaml"),
+            format!("name: kid\nversion: 1.0.0\ndescription: Kid.\ndependencies:\n  - {{name: {name}, source: ../nowhere, mount: \"{mount}\"}}\n"),
+        )
+        .unwrap();
+        let out = gitagent().arg("install").arg(&agent).output().unwrap();
+        let text = format!(
+            "{}{}",
+            String::from_utf8_lossy(&out.stdout),
+            String::from_utf8_lossy(&out.stderr)
+        );
+        assert!(!out.status.success(), "{mount}: {text}");
+        assert!(
+            text.contains("inside the agent's folder")
+                || text.contains("leads outside")
+                || text.contains("use --force"),
+            "{mount}: {text}"
+        );
+    }
+    assert!(
+        precious.join("data.txt").is_file(),
+        "outside the agent: untouched"
+    );
+    assert!(
+        agent.join("agents/local/notes.txt").is_file(),
+        "not an installed agent: kept without --force"
+    );
+
+    // A dependency name that is a path is refused too.
+    std::fs::write(
+        agent.join("agent.yaml"),
+        "name: kid\nversion: 1.0.0\ndescription: Kid.\ndependencies:\n  - {name: \"..\", source: ../nowhere}\n",
+    )
+    .unwrap();
+    let out = gitagent().arg("install").arg(&agent).output().unwrap();
+    assert!(!out.status.success());
+    assert!(String::from_utf8_lossy(&out.stderr).contains("must not contain"));
+}