Skip to content
Merged
5 changes: 4 additions & 1 deletion crates/sharecli-core/src/speculation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
//! - A sliding-window counter prevents stale高频 commands from being
//! speculated on indefinitely.

use std::cmp::Reverse;
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
use std::collections::HashMap;
use std::sync::Arc;
use std::time::{Duration, Instant};
Expand Down Expand Up @@ -57,13 +58,15 @@ pub struct SpeculationCandidate {
///
/// Wrapped in `Arc<Mutex<…>>` so the background task can drain candidates
/// without blocking the hot `Hypervisor::run` path.
#[derive(Default)]
struct Inner {
/// CommandKey → (hit count, first-seen instant).
hits: HashMap<String, (u32, Instant)>,
/// CommandKey → request details needed for re-execution.
requests: HashMap<String, SpeculationCandidate>,
}

#[derive(Default)]
Comment thread
coderabbitai[bot] marked this conversation as resolved.
pub struct SpeculationTracker {
inner: Mutex<Inner>,
}
Expand Down Expand Up @@ -122,7 +125,7 @@ impl SpeculationTracker {
.collect();

// Highest frequency first.
scored.sort_by(|a, b| b.0.cmp(&a.0));
scored.sort_by_key(|entry| Reverse(entry.0));
scored.truncate(SPECULATION_MAX_CANDIDATES);

let mut candidates = Vec::new();
Expand Down
53 changes: 46 additions & 7 deletions crates/sharecli-mesh/src/worktree_pool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,29 @@ use std::path::{Path, PathBuf};
use std::process::Command;
use std::sync::Mutex;

/// Git variables that bind a subprocess to the caller's repository.
///
/// Git exports these to hooks. Every command in this module intentionally
/// operates on an explicit foreign checkout, so inheriting them would let a
/// hook test inspect or mutate the hook's repository instead.
const GIT_LOCAL_ENV_VARS: &[&str] = &[
"GIT_ALTERNATE_OBJECT_DIRECTORIES",
"GIT_CONFIG",
"GIT_CONFIG_PARAMETERS",
"GIT_CONFIG_COUNT",
"GIT_OBJECT_DIRECTORY",
"GIT_DIR",
"GIT_WORK_TREE",
"GIT_IMPLICIT_WORK_TREE",
"GIT_GRAFT_FILE",
"GIT_INDEX_FILE",
"GIT_NO_REPLACE_OBJECTS",
"GIT_REPLACE_REF_BASE",
"GIT_PREFIX",
"GIT_SHALLOW_FILE",
"GIT_COMMON_DIR",
];
Comment on lines +17 to +36

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: GIT_NAMESPACE is omitted from the sanitized environment, even though Git uses it to redirect ref lookups and updates into a namespace. When WorktreePool runs from a namespaced hook, branch listing, branch creation, and worktree operations can therefore target the caller's namespace instead of the explicit foreign repository context. Add GIT_NAMESPACE to the variables removed by git_command. [api mismatch]

Severity Level: Major ⚠️
- ⚠️ Namespaced hook worktree allocation may select wrong refs.
- ❌ Agent checkout creation can fail in namespaced repositories.
- ⚠️ Branch cleanup may affect an unintended ref namespace.

Fix in Cursor Fix in VSCode Claude

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** crates/sharecli-mesh/src/worktree_pool.rs
**Line:** 17:33
**Comment:**
	*Api Mismatch: `GIT_NAMESPACE` is omitted from the sanitized environment, even though Git uses it to redirect ref lookups and updates into a namespace. When WorktreePool runs from a namespaced hook, branch listing, branch creation, and worktree operations can therefore target the caller's namespace instead of the explicit foreign repository context. Add `GIT_NAMESPACE` to the variables removed by `git_command`.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 348e7c4: foreign Git subprocesses now remove GIT_NAMESPACE as well as Git local-context variables.

Comment thread
coderabbitai[bot] marked this conversation as resolved.

/// Error from worktree pool operations.
#[derive(Debug, thiserror::Error)]
pub enum WorktreePoolError {
Expand Down Expand Up @@ -150,9 +173,8 @@ impl WorktreePool {
}

fn ensure_git_repo(path: &Path) -> Result<(), WorktreePoolError> {
let status = Command::new("git")
let status = git_command(path)
.args(["rev-parse", "--git-dir"])
.current_dir(path)
.output()
.map_err(|e| WorktreePoolError::Git(e.to_string()))?;
if status.status.success() {
Expand All @@ -163,11 +185,8 @@ fn ensure_git_repo(path: &Path) -> Result<(), WorktreePoolError> {
}

fn git(cwd: &Path, args: &[&str]) -> Result<String, WorktreePoolError> {
let out = Command::new("git")
.args(args)
.current_dir(cwd)
.output()
.map_err(|e| WorktreePoolError::Git(e.to_string()))?;
let out =
git_command(cwd).args(args).output().map_err(|e| WorktreePoolError::Git(e.to_string()))?;
if out.status.success() {
Ok(String::from_utf8_lossy(&out.stdout).into_owned())
} else {
Expand All @@ -181,6 +200,15 @@ fn git(cwd: &Path, args: &[&str]) -> Result<String, WorktreePoolError> {
}
}

fn git_command(cwd: &Path) -> Command {
let mut command = Command::new("git");
command.current_dir(cwd);
for variable in GIT_LOCAL_ENV_VARS {
command.env_remove(variable);
}
command
}

#[cfg(test)]
mod tests {
use super::*;
Expand Down Expand Up @@ -221,4 +249,15 @@ mod tests {
let err = WorktreePool::open(dir.path(), pool_dir.path()).expect_err("must fail");
assert!(matches!(err, WorktreePoolError::NotGitRepo(_)));
}

#[test]
fn git_command_clears_hook_repository_context() {
let command = git_command(Path::new("/tmp"));
for variable in GIT_LOCAL_ENV_VARS {
assert!(
command.get_envs().any(|(key, value)| key == *variable && value.is_none()),
"{variable} must not leak into a foreign git command"
);
}
}
}
1 change: 1 addition & 0 deletions src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1512,6 +1512,7 @@ fn cli_version() -> Result<()> {
println!("{splash}");
println!("sharecli {version}");
println!("shared CLI process manager");
println!("Backbone-2 family (ASCII palette disabled)");
println!("(NO_COLOR set — ASCII palette disabled)");
} else {
let splash = r#"
Expand Down
2 changes: 1 addition & 1 deletion src/runtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -848,7 +848,7 @@ mod tests {
);

let info = pool.spawn(cmd, &args, None, None, None).await;
assert!(info.is_ok());
assert!(info.is_ok(), "process-pool spawn failed: {info:?}");

let list = pool.list().await;
assert!(!list.is_empty());
Expand Down
115 changes: 80 additions & 35 deletions tests/fr006_proc_ndjson.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,20 +5,86 @@
//! AC-006.37 NDJSON agent rows include `state` (parity with flat `--json`, AC-006.32)

use std::io::Read;
use std::process::{Command, Stdio};
use std::process::{Child, Command, Stdio};
use std::sync::{Arc, Mutex};
use std::thread;
use std::time::Duration;
use std::time::{Duration, Instant};

fn watch_grace(normal: Duration, profiled: Duration) -> Duration {
matches!(std::env::var("SHARECLI_DHAT_PROFILE"), Ok(value) if value == "1")
.then_some(profiled)
.unwrap_or(normal)
}
const WATCH_DEADLINE: Duration = Duration::from_secs(45);

fn bin() -> Command {
Command::new(env!("CARGO_BIN_EXE_sharecli"))
}

fn complete_ndjson_line_count(output: &str) -> usize {
output
.split_inclusive('\n')
.filter(|line| line.ends_with('\n') && !line.trim().is_empty())
.count()
}

fn drain_watch_until(
child: &mut Child,
mut ready: impl FnMut(&str, &str) -> bool,
) -> (String, String) {
let stdout = child.stdout.take().expect("piped stdout");
let stderr = child.stderr.take().expect("piped stderr");
let stdout_buf = Arc::new(Mutex::new(String::new()));
let stderr_buf = Arc::new(Mutex::new(String::new()));
let out_arc = Arc::clone(&stdout_buf);
let err_arc = Arc::clone(&stderr_buf);
let stdout_reader = thread::spawn(move || {
let mut chunk = [0u8; 16_384];
let mut out = stdout;
loop {
match out.read(&mut chunk) {
Ok(0) | Err(_) => break,
Ok(n) => out_arc
.lock()
.expect("stdout lock")
.push_str(&String::from_utf8_lossy(&chunk[..n])),
}
}
});
let stderr_reader = thread::spawn(move || {
let mut chunk = [0u8; 4096];
let mut err = stderr;
loop {
match err.read(&mut chunk) {
Ok(0) | Err(_) => break,
Ok(n) => err_arc
.lock()
.expect("stderr lock")
.push_str(&String::from_utf8_lossy(&chunk[..n])),
}
}
});

let deadline = Instant::now() + WATCH_DEADLINE;
while Instant::now() < deadline {
let stdout = stdout_buf.lock().expect("stdout lock").clone();
let stderr = stderr_buf.lock().expect("stderr lock").clone();
if ready(&stdout, &stderr) {
break;
}
thread::sleep(Duration::from_millis(100));
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

let _ = child.kill();
let _ = child.wait();
let _ = stdout_reader.join();
let _ = stderr_reader.join();
let stdout = stdout_buf.lock().expect("stdout lock").clone();
let stderr = stderr_buf.lock().expect("stderr lock").clone();
(stdout, stderr)
}

#[test]
fn complete_ndjson_line_count_requires_newline_delimited_objects() {
assert_eq!(complete_ndjson_line_count("{\"ts\":1}\n{\"ts\":2}\n"), 2);
assert_eq!(complete_ndjson_line_count("{\"ts\":1}\n{\"ts\":2}"), 1);
}

/// FR-006 / AC-006.18 — each watch refresh is a single parseable NDJSON line with `ts`.
#[test]
#[serial_test::serial]
Expand All @@ -30,14 +96,8 @@ fn fr006_proc_watch_ndjson_one_line_per_refresh() {
.spawn()
.expect("spawn sharecli proc --json --watch 1");

thread::sleep(watch_grace(Duration::from_secs(8), Duration::from_secs(25)));
let _ = child.kill();

let mut stdout = String::new();
if let Some(mut out) = child.stdout.take() {
let _ = out.read_to_string(&mut stdout);
}
let _ = child.wait();
let (stdout, _) =
drain_watch_until(&mut child, |stdout, _| complete_ndjson_line_count(stdout) >= 2);

let lines: Vec<&str> = stdout.lines().filter(|l| !l.is_empty()).collect();
assert!(
Expand All @@ -63,18 +123,9 @@ fn fr006_proc_watch_ndjson_stdout_is_pipe_clean() {
.spawn()
.expect("spawn sharecli proc --json --watch 1");

thread::sleep(watch_grace(Duration::from_secs(5), Duration::from_secs(12)));
let _ = child.kill();

let mut stdout = String::new();
let mut stderr = String::new();
if let Some(mut out) = child.stdout.take() {
let _ = out.read_to_string(&mut stdout);
}
if let Some(mut err) = child.stderr.take() {
let _ = err.read_to_string(&mut stderr);
}
let _ = child.wait();
let (stdout, stderr) = drain_watch_until(&mut child, |stdout, stderr| {
complete_ndjson_line_count(stdout) >= 1 && stderr.contains("[watch]")
});

assert!(
!stdout.contains("[watch]"),
Expand All @@ -98,14 +149,8 @@ fn fr006_proc_watch_ndjson_agent_rows_include_state_key() {
.spawn()
.expect("spawn sharecli proc --json --watch 1");

thread::sleep(watch_grace(Duration::from_millis(2_500), Duration::from_secs(12)));
let _ = child.kill();

let mut stdout = String::new();
if let Some(mut out) = child.stdout.take() {
let _ = out.read_to_string(&mut stdout);
}
let _ = child.wait();
let (stdout, _) =
drain_watch_until(&mut child, |stdout, _| complete_ndjson_line_count(stdout) >= 1);

let lines: Vec<&str> = stdout.lines().filter(|l| !l.is_empty()).collect();
let line = lines.first().copied().unwrap_or_else(|| {
Expand Down
2 changes: 1 addition & 1 deletion tests/fr007_health_pool_status_csv.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ fn fr007_render_health_csv_body() {
healthy: true,
issues: vec![],
gate: gate.clone(),
host_watch: host_watch.clone(),
host_watch,
status: None,
};
let health = HealthJson {
Expand Down
4 changes: 2 additions & 2 deletions tests/fr007_ipc_health_pool_status.rs
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ fn fr007_ipc_health_snapshot_pool_status_order() {
total_memory_mb: 16384,
healthy: true,
gate: gate.clone(),
host_watch: host_watch.clone(),
host_watch,
pool: PoolSnapshot {
node_total: 2,
node_idle: 1,
Expand All @@ -168,7 +168,7 @@ fn fr007_ipc_health_snapshot_pool_status_order() {
healthy: true,
issues: vec![],
gate: gate.clone(),
host_watch: host_watch.clone(),
host_watch,
status: None,
},
status: StatusSnapshot {
Expand Down
33 changes: 16 additions & 17 deletions tests/fr007_ipc_health_status_gate_host_watch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,24 +139,23 @@ fn fr007_ipc_health_snapshot_gate_before_host_watch() {
fn fr007_ipc_health_snapshot_wire_roundtrip() {
use sharecli_ipc::handler::HealthSnapshot;

let raw = format!(
r#"{{"managed_processes":3,"used_memory_mb":2048,"total_memory_mb":16384,
"healthy":true,"gate":{{"thermal_pressure":"GREEN","detected_agents":0,
"agent_total_rss_bytes":0,"agent_contention":"OK","gate_decision":"ADMIT"}},
"host_watch":{{"fd_count":1,"net_rx_bytes":2,"net_tx_bytes":3,
"mem_rss_bytes":4,"load_1m":0.5}},
"pool":{{"node_total":0,"node_idle":0,"bun_total":0,"bun_idle":0,"max_per_type":4,
let raw = r#"{"managed_processes":3,"used_memory_mb":2048,"total_memory_mb":16384,
"healthy":true,"gate":{"thermal_pressure":"GREEN","detected_agents":0,
"agent_total_rss_bytes":0,"agent_contention":"OK","gate_decision":"ADMIT"},
"host_watch":{"fd_count":1,"net_rx_bytes":2,"net_tx_bytes":3,
"mem_rss_bytes":4,"load_1m":0.5},
"pool":{"node_total":0,"node_idle":0,"bun_total":0,"bun_idle":0,"max_per_type":4,
"healthy":true,"issues":[],
"gate":{{"thermal_pressure":"GREEN","detected_agents":0,
"agent_total_rss_bytes":0,"agent_contention":"OK","gate_decision":"ADMIT"}},
"host_watch":{{"fd_count":0,"net_rx_bytes":0,"net_tx_bytes":0,
"mem_rss_bytes":0,"load_1m":0.0}}}},
"status":{{"total_processes":0,"agents":[],"scanned":0,"watched":0,
"gate":{{"thermal_pressure":"GREEN","detected_agents":0,
"agent_total_rss_bytes":0,"agent_contention":"OK","gate_decision":"ADMIT"}},
"host_watch":{{"fd_count":0,"net_rx_bytes":0,"net_tx_bytes":0,
"mem_rss_bytes":0,"load_1m":0.0}}}}}}"#
);
"gate":{"thermal_pressure":"GREEN","detected_agents":0,
"agent_total_rss_bytes":0,"agent_contention":"OK","gate_decision":"ADMIT"},
"host_watch":{"fd_count":0,"net_rx_bytes":0,"net_tx_bytes":0,
"mem_rss_bytes":0,"load_1m":0.0}},
"status":{"total_processes":0,"agents":[],"scanned":0,"watched":0,
"gate":{"thermal_pressure":"GREEN","detected_agents":0,
"agent_total_rss_bytes":0,"agent_contention":"OK","gate_decision":"ADMIT"},
"host_watch":{"fd_count":0,"net_rx_bytes":0,"net_tx_bytes":0,
"mem_rss_bytes":0,"load_1m":0.0}}}"#
.to_string();
let h: HealthSnapshot = serde_json::from_str(&raw).expect("decode HealthSnapshot wire JSON");
assert_eq!(h.managed_processes, 3);
assert_eq!(h.used_memory_mb, 2048);
Expand Down
4 changes: 2 additions & 2 deletions tests/fr007_ipc_monitoring_report_gate_host_watch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ fn fr007_ipc_monitoring_report_snapshot_gate_before_host_watch() {
thread_count: None,
}],
gate: gate.clone(),
host_watch: host_watch.clone(),
host_watch,
pool: PoolSnapshot {
node_total: 2,
node_idle: 1,
Expand All @@ -120,7 +120,7 @@ fn fr007_ipc_monitoring_report_snapshot_gate_before_host_watch() {
healthy: true,
issues: vec![],
gate: gate.clone(),
host_watch: host_watch.clone(),
host_watch,
status: None,
},
status: StatusSnapshot {
Expand Down
4 changes: 2 additions & 2 deletions tests/fr007_ipc_monitoring_report_pool_status.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ fn fr007_ipc_monitoring_report_snapshot_pool_status_order() {
thread_count: None,
}],
gate: gate.clone(),
host_watch: host_watch.clone(),
host_watch,
pool: PoolSnapshot {
node_total: 2,
node_idle: 1,
Expand All @@ -120,7 +120,7 @@ fn fr007_ipc_monitoring_report_snapshot_pool_status_order() {
healthy: true,
issues: vec![],
gate: gate.clone(),
host_watch: host_watch.clone(),
host_watch,
status: None,
},
status: StatusSnapshot {
Expand Down
Loading
Loading