From fc5921a52100f01730babd4227fb1ee25b41359f Mon Sep 17 00:00:00 2001 From: Affaan Mustafa Date: Sun, 12 Apr 2026 22:29:05 -0700 Subject: [PATCH] fix: detach ecc2 background session runners (#1387) * fix: detach ecc2 background session runners * fix: stabilize windows ci portability * fix: persist detached runner startup stderr * fix: prefer repo-relative hook file paths * fix: make npm pack test shell-safe on windows --- ecc2/src/session/manager.rs | 142 ++++++++++++++++++- scripts/hooks/session-activity-tracker.js | 104 +++++++++----- tests/hooks/session-activity-tracker.test.js | 52 +++++++ tests/scripts/build-opencode.test.js | 3 +- 4 files changed, 260 insertions(+), 41 deletions(-) diff --git a/ecc2/src/session/manager.rs b/ecc2/src/session/manager.rs index d8fd1e55..7be6f259 100644 --- a/ecc2/src/session/manager.rs +++ b/ecc2/src/session/manager.rs @@ -4,6 +4,7 @@ use cron::Schedule as CronSchedule; use serde::Serialize; use std::collections::{BTreeMap, HashMap, HashSet}; use std::fmt; +use std::fs::OpenOptions; use std::path::{Path, PathBuf}; use std::process::Stdio; use std::str::FromStr; @@ -2983,7 +2984,28 @@ async fn spawn_session_runner_for_program( working_dir: &Path, current_exe: &Path, ) -> Result<()> { - let child = Command::new(current_exe) + let stderr_log_path = background_runner_stderr_log_path(working_dir, session_id); + if let Some(parent) = stderr_log_path.parent() { + std::fs::create_dir_all(parent).with_context(|| { + format!( + "Failed to create ECC runner log directory {}", + parent.display() + ) + })?; + } + let stderr_log = OpenOptions::new() + .create(true) + .append(true) + .open(&stderr_log_path) + .with_context(|| { + format!( + "Failed to open ECC runner stderr log {}", + stderr_log_path.display() + ) + })?; + + let mut command = Command::new(current_exe); + command .arg("run-session") .arg("--session-id") .arg(session_id) @@ -2995,7 +3017,10 @@ async fn spawn_session_runner_for_program( .arg(working_dir) .stdin(Stdio::null()) .stdout(Stdio::null()) - .stderr(Stdio::null()) + .stderr(Stdio::from(stderr_log)); + configure_background_runner_command(&mut command); + + let child = command .spawn() .with_context(|| format!("Failed to spawn ECC runner from {}", current_exe.display()))?; @@ -3005,6 +3030,46 @@ async fn spawn_session_runner_for_program( Ok(()) } +fn background_runner_stderr_log_path(working_dir: &Path, session_id: &str) -> PathBuf { + working_dir + .join(".claude") + .join("ecc2") + .join("logs") + .join(format!("{session_id}.runner-stderr.log")) +} + +#[cfg(windows)] +fn detached_creation_flags() -> u32 { + const DETACHED_PROCESS: u32 = 0x0000_0008; + const CREATE_NEW_PROCESS_GROUP: u32 = 0x0000_0200; + DETACHED_PROCESS | CREATE_NEW_PROCESS_GROUP +} + +fn configure_background_runner_command(command: &mut Command) { + #[cfg(unix)] + { + use std::os::unix::process::CommandExt; + + // Detach the runner from the caller's shell/session so it keeps + // processing a live harness session after `ecc-tui start` returns. + unsafe { + command.as_std_mut().pre_exec(|| { + if libc::setsid() == -1 { + return Err(std::io::Error::last_os_error()); + } + Ok(()) + }); + } + } + + #[cfg(windows)] + { + use std::os::windows::process::CommandExt; + + command.as_std_mut().creation_flags(detached_creation_flags()); + } +} + fn build_agent_command( cfg: &Config, agent_type: &str, @@ -5032,6 +5097,22 @@ mod tests { anyhow::bail!("timed out waiting for {}", path.display()); } + fn wait_for_text(path: &Path, needle: &str) -> Result { + for _ in 0..200 { + if path.exists() { + let content = fs::read_to_string(path) + .with_context(|| format!("failed to read {}", path.display()))?; + if content.contains(needle) { + return Ok(content); + } + } + + thread::sleep(StdDuration::from_millis(20)); + } + + anyhow::bail!("timed out waiting for {}", path.display()); + } + fn command_env_map(command: &Command) -> BTreeMap { command .as_std() @@ -5047,6 +5128,63 @@ mod tests { .collect() } + #[cfg(unix)] + #[tokio::test(flavor = "current_thread")] + async fn background_runner_command_starts_new_session() -> Result<()> { + let tempdir = TestDir::new("manager-detached-runner")?; + let script_path = tempdir.path().join("detached-runner.py"); + let log_path = tempdir.path().join("detached-runner.log"); + let script = format!( + "#!/usr/bin/env python3\nimport os\nimport pathlib\nimport time\n\npath = pathlib.Path(r\"{}\")\npath.write_text(f\"pid={{os.getpid()}} sid={{os.getsid(0)}}\", encoding=\"utf-8\")\ntime.sleep(30)\n", + log_path.display() + ); + fs::write(&script_path, script)?; + let mut permissions = fs::metadata(&script_path)?.permissions(); + permissions.set_mode(0o755); + fs::set_permissions(&script_path, permissions)?; + + let mut command = Command::new(&script_path); + command + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()); + configure_background_runner_command(&mut command); + + let mut child = command.spawn()?; + let child_pid = child.id().context("detached child pid")? as i32; + let content = wait_for_text(&log_path, "sid=")?; + let sid = content + .split_whitespace() + .find_map(|part| part.strip_prefix("sid=")) + .context("session id should be logged")? + .parse::() + .context("session id should parse")?; + let parent_sid = unsafe { libc::getsid(0) }; + + assert_eq!(sid, child_pid); + assert_ne!(sid, parent_sid); + + let _ = child.kill().await; + let _ = child.wait().await; + Ok(()) + } + + #[test] + fn background_runner_stderr_log_path_is_session_scoped() { + let path = + background_runner_stderr_log_path(Path::new("/tmp/ecc-repo"), "session-123"); + assert_eq!( + path, + PathBuf::from("/tmp/ecc-repo/.claude/ecc2/logs/session-123.runner-stderr.log") + ); + } + + #[cfg(windows)] + #[test] + fn detached_creation_flags_include_detach_and_process_group() { + assert_eq!(detached_creation_flags(), 0x0000_0008 | 0x0000_0200); + } + fn write_package_manager_project_files( repo_root: &Path, package_manager_field: Option<&str>, diff --git a/scripts/hooks/session-activity-tracker.js b/scripts/hooks/session-activity-tracker.js index 9d4716a1..19f9d9a8 100644 --- a/scripts/hooks/session-activity-tracker.js +++ b/scripts/hooks/session-activity-tracker.js @@ -359,44 +359,72 @@ function gitRepoRoot(cwd) { return runGit(['rev-parse', '--show-toplevel'], cwd); } -function repoRelativePath(repoRoot, filePath) { - const absolute = path.isAbsolute(filePath) - ? path.resolve(filePath) - : path.resolve(process.cwd(), filePath); - const relative = path.relative(repoRoot, absolute); - if (!relative || relative.startsWith('..') || path.isAbsolute(relative)) { - return null; +const MAX_RELEVANT_PATCH_LINES = 6; + +function candidateGitPaths(repoRoot, filePath) { + const resolvedRepoRoot = path.resolve(repoRoot); + const candidates = []; + const pushCandidate = value => { + const candidate = String(value || '').trim(); + if (!candidate || candidates.includes(candidate)) { + return; + } + candidates.push(candidate); + }; + + const absoluteCandidates = path.isAbsolute(filePath) + ? [path.resolve(filePath)] + : [ + path.resolve(resolvedRepoRoot, filePath), + path.resolve(process.cwd(), filePath), + ]; + + for (const absolute of absoluteCandidates) { + const relative = path.relative(resolvedRepoRoot, absolute); + if (!relative || relative.startsWith('..') || path.isAbsolute(relative)) { + continue; + } + + pushCandidate(relative); + pushCandidate(relative.split(path.sep).join('/')); + pushCandidate(absolute); + pushCandidate(absolute.split(path.sep).join('/')); } - return relative.split(path.sep).join('/'); + + return candidates; } -function patchPreviewFromGitDiff(repoRoot, repoRelative) { - const patch = runGit( - ['diff', '--no-ext-diff', '--no-color', '--unified=1', '--', repoRelative], - repoRoot +function patchPreviewFromGitDiff(repoRoot, pathCandidates) { + for (const candidate of pathCandidates) { + const patch = runGit( + ['diff', '--no-ext-diff', '--no-color', '--unified=1', '--', candidate], + repoRoot + ); + if (!patch) { + continue; + } + + const relevant = patch + .split(/\r?\n/) + .filter(line => + line.startsWith('@@') + || (line.startsWith('+') && !line.startsWith('+++')) + || (line.startsWith('-') && !line.startsWith('---')) + ) + .slice(0, MAX_RELEVANT_PATCH_LINES); + + if (relevant.length > 0) { + return relevant.join('\n'); + } + } + + return undefined; +} + +function trackedInGit(repoRoot, pathCandidates) { + return pathCandidates.some(candidate => + runGit(['ls-files', '--error-unmatch', '--', candidate], repoRoot) !== null ); - if (!patch) { - return undefined; - } - - const relevant = patch - .split(/\r?\n/) - .filter(line => - line.startsWith('@@') - || (line.startsWith('+') && !line.startsWith('+++')) - || (line.startsWith('-') && !line.startsWith('---')) - ) - .slice(0, 6); - - if (relevant.length === 0) { - return undefined; - } - - return relevant.join('\n'); -} - -function trackedInGit(repoRoot, repoRelative) { - return runGit(['ls-files', '--error-unmatch', '--', repoRelative], repoRoot) !== null; } function enrichFileEventFromWorkingTree(toolName, event) { @@ -409,14 +437,14 @@ function enrichFileEventFromWorkingTree(toolName, event) { return event; } - const repoRelative = repoRelativePath(repoRoot, event.path); - if (!repoRelative) { + const pathCandidates = candidateGitPaths(repoRoot, event.path); + if (pathCandidates.length === 0) { return event; } const tool = String(toolName || '').trim().toLowerCase(); - const tracked = trackedInGit(repoRoot, repoRelative); - const patchPreview = patchPreviewFromGitDiff(repoRoot, repoRelative) || event.patch_preview; + const tracked = trackedInGit(repoRoot, pathCandidates); + const patchPreview = patchPreviewFromGitDiff(repoRoot, pathCandidates) || event.patch_preview; const diffPreview = buildDiffPreviewFromPatchPreview(patchPreview) || event.diff_preview; if (tool.includes('write')) { diff --git a/tests/hooks/session-activity-tracker.test.js b/tests/hooks/session-activity-tracker.test.js index 99eb7c6f..3eb95a02 100644 --- a/tests/hooks/session-activity-tracker.test.js +++ b/tests/hooks/session-activity-tracker.test.js @@ -309,6 +309,58 @@ function runTests() { fs.rmSync(repoDir, { recursive: true, force: true }); }) ? passed++ : failed++); + (test('resolves repo-relative paths even when the hook runs from a nested cwd', () => { + const tmpHome = makeTempDir(); + const repoDir = fs.mkdtempSync(path.join(os.tmpdir(), 'session-activity-tracker-nested-repo-')); + + spawnSync('git', ['init'], { cwd: repoDir, encoding: 'utf8' }); + spawnSync('git', ['config', 'user.email', 'ecc@example.com'], { cwd: repoDir, encoding: 'utf8' }); + spawnSync('git', ['config', 'user.name', 'ECC Tests'], { cwd: repoDir, encoding: 'utf8' }); + + const srcDir = path.join(repoDir, 'src'); + const nestedCwd = path.join(repoDir, 'subdir'); + fs.mkdirSync(srcDir, { recursive: true }); + fs.mkdirSync(nestedCwd, { recursive: true }); + + const trackedFile = path.join(srcDir, 'app.ts'); + fs.writeFileSync(trackedFile, 'const count = 1;\n', 'utf8'); + spawnSync('git', ['add', 'src/app.ts'], { cwd: repoDir, encoding: 'utf8' }); + spawnSync('git', ['commit', '-m', 'init'], { cwd: repoDir, encoding: 'utf8' }); + + fs.writeFileSync(trackedFile, 'const count = 2;\n', 'utf8'); + + const input = { + tool_name: 'Write', + tool_input: { + file_path: 'src/app.ts', + content: 'const count = 2;\n', + }, + tool_output: { output: 'updated src/app.ts' }, + }; + const result = runScript(input, { + ...withTempHome(tmpHome), + CLAUDE_HOOK_EVENT_NAME: 'PostToolUse', + ECC_SESSION_ID: 'ecc-session-nested-cwd', + }, { + cwd: nestedCwd, + }); + assert.strictEqual(result.code, 0); + + const metricsFile = path.join(tmpHome, '.claude', 'metrics', 'tool-usage.jsonl'); + const row = JSON.parse(fs.readFileSync(metricsFile, 'utf8').trim()); + assert.deepStrictEqual(row.file_events, [ + { + path: 'src/app.ts', + action: 'modify', + diff_preview: 'const count = 1; -> const count = 2;', + patch_preview: '@@ -1 +1 @@\n-const count = 1;\n+const count = 2;', + }, + ]); + + fs.rmSync(tmpHome, { recursive: true, force: true }); + fs.rmSync(repoDir, { recursive: true, force: true }); + }) ? passed++ : failed++); + (test('prefers ECC_SESSION_ID over CLAUDE_SESSION_ID and redacts bash summaries', () => { const tmpHome = makeTempDir(); const input = { diff --git a/tests/scripts/build-opencode.test.js b/tests/scripts/build-opencode.test.js index 56b30f43..253d16e7 100644 --- a/tests/scripts/build-opencode.test.js +++ b/tests/scripts/build-opencode.test.js @@ -49,8 +49,9 @@ function main() { const result = spawnSync("npm", ["pack", "--dry-run", "--json"], { cwd: repoRoot, encoding: "utf8", + shell: process.platform === "win32", }) - assert.strictEqual(result.status, 0, result.stderr) + assert.strictEqual(result.status, 0, result.error?.message || result.stderr) const packOutput = JSON.parse(result.stdout) const packagedPaths = new Set(packOutput[0]?.files?.map((file) => file.path) ?? [])