diff --git a/skills/continuous-learning-v2/agents/observer-loop.sh b/skills/continuous-learning-v2/agents/observer-loop.sh index 74b8f5110..bce0d2d7c 100755 --- a/skills/continuous-learning-v2/agents/observer-loop.sh +++ b/skills/continuous-learning-v2/agents/observer-loop.sh @@ -9,6 +9,13 @@ set +e unset CLAUDECODE SLEEP_PID="" +CLAUDE_PID="" +CLAUDE_PROCESS_GROUP=0 +WATCHDOG_PID="" +ACTIVE_ANALYSIS_FILE="" +ACTIVE_PROMPT_FILE="" +ACTIVE_RESULT_FILE="" +RESULT_FDS_OPEN=0 USR1_FIRED=0 PENDING_ANALYSIS=0 ANALYZING=0 @@ -25,7 +32,83 @@ ACTIVITY_FILE="${PROJECT_DIR}/.observer-last-activity" # ${BASH_SOURCE[0]}, which always points at this file (#2370). SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +claude_process_alive() { + local process_pid="$1" + + if [ -z "$process_pid" ]; then + return 1 + fi + + if [ "$CLAUDE_PROCESS_GROUP" -eq 1 ]; then + kill -0 -- "-$process_pid" 2>/dev/null + else + kill -0 "$process_pid" 2>/dev/null + fi +} + +signal_claude_process() { + local process_pid="$1" + local signal_name="$2" + + if [ "$CLAUDE_PROCESS_GROUP" -eq 1 ]; then + kill -"$signal_name" -- "-$process_pid" 2>/dev/null || true + else + kill -"$signal_name" "$process_pid" 2>/dev/null || true + fi +} + +stop_claude_process() { + local process_pid="$1" + local attempts=0 + + if [ -z "$process_pid" ]; then + return + fi + + if claude_process_alive "$process_pid"; then + signal_claude_process "$process_pid" TERM + while claude_process_alive "$process_pid" && [ "$attempts" -lt 20 ]; do + sleep 0.1 + attempts=$((attempts + 1)) + done + if claude_process_alive "$process_pid"; then + signal_claude_process "$process_pid" KILL + fi + fi + wait "$process_pid" 2>/dev/null || true + CLAUDE_PROCESS_GROUP=0 +} + +cleanup_analysis_resources() { + if [ -n "$WATCHDOG_PID" ]; then + kill "$WATCHDOG_PID" 2>/dev/null || true + wait "$WATCHDOG_PID" 2>/dev/null || true + WATCHDOG_PID="" + fi + if [ -n "$CLAUDE_PID" ]; then + stop_claude_process "$CLAUDE_PID" + CLAUDE_PID="" + fi + + if [ "$RESULT_FDS_OPEN" -eq 1 ]; then + { exec 8>&-; } 2>/dev/null || true + if [ -n "${LOG_FILE:-}" ]; then + cat <&9 >> "$LOG_FILE" 2>/dev/null || true + fi + { exec 7<&-; } 2>/dev/null || true + { exec 9<&-; } 2>/dev/null || true + RESULT_FDS_OPEN=0 + fi + [ -n "$ACTIVE_ANALYSIS_FILE" ] && rm -f "$ACTIVE_ANALYSIS_FILE" + [ -n "$ACTIVE_PROMPT_FILE" ] && rm -f "$ACTIVE_PROMPT_FILE" + [ -n "$ACTIVE_RESULT_FILE" ] && rm -f "$ACTIVE_RESULT_FILE" + ACTIVE_ANALYSIS_FILE="" + ACTIVE_PROMPT_FILE="" + ACTIVE_RESULT_FILE="" +} + cleanup() { + cleanup_analysis_resources [ -n "$SLEEP_PID" ] && kill "$SLEEP_PID" 2>/dev/null if [ -f "$PID_FILE" ] && [ "$(cat "$PID_FILE" 2>/dev/null)" = "$$" ]; then rm -f "$PID_FILE" @@ -149,7 +232,17 @@ analyze_observations() { # substitutes a trailing X run, so a suffix after it (e.g. `.jsonl`) produces a # literal, non-random name that wedges every later cycle with "File exists" (#2417). analysis_file="$(mktemp "${observer_tmp_dir}/ecc-observer-analysis.jsonl.XXXXXX")" - tail -n "$MAX_ANALYSIS_LINES" "$OBSERVATIONS_FILE" > "$analysis_file" + if [ -z "$analysis_file" ] || [ ! -f "$analysis_file" ]; then + echo "[$(date)] Failed to create observer analysis file; retaining observations for retry" >> "$LOG_FILE" + return + fi + ACTIVE_ANALYSIS_FILE="$analysis_file" + + if ! tail -n "$MAX_ANALYSIS_LINES" "$OBSERVATIONS_FILE" > "$analysis_file"; then + echo "[$(date)] Failed to snapshot observations; retaining them for retry" >> "$LOG_FILE" + cleanup_analysis_resources + return + fi analysis_count=$(wc -l < "$analysis_file" 2>/dev/null || echo 0) echo "[$(date)] Using last $analysis_count of $obs_count observations for analysis" >> "$LOG_FILE" @@ -166,6 +259,12 @@ analyze_observations() { fi prompt_file="$(mktemp "${observer_tmp_dir}/ecc-observer-prompt.XXXXXX")" + if [ -z "$prompt_file" ] || [ ! -f "$prompt_file" ]; then + echo "[$(date)] Failed to create observer prompt file; retaining observations for retry" >> "$LOG_FILE" + cleanup_analysis_resources + return + fi + ACTIVE_PROMPT_FILE="$prompt_file" cat > "$prompt_file" </dev/null || true)" rm -f "$prompt_file" + ACTIVE_PROMPT_FILE="" if [ -z "$prompt_content" ]; then echo "[$(date)] Failed to load observer prompt content, skipping analysis" >> "$LOG_FILE" - rm -f "$analysis_file" + cleanup_analysis_resources return fi @@ -249,7 +356,30 @@ PROMPT # Ensure CWD is PROJECT_DIR so the relative analysis_relpath resolves correctly # on all platforms, not just when the observer happens to be launched from the project root. - cd "$PROJECT_DIR" || { echo "[$(date)] Failed to cd to PROJECT_DIR ($PROJECT_DIR), skipping analysis" >> "$LOG_FILE"; rm -f "$analysis_file"; return; } + cd "$PROJECT_DIR" || { echo "[$(date)] Failed to cd to PROJECT_DIR ($PROJECT_DIR), skipping analysis" >> "$LOG_FILE"; cleanup_analysis_resources; return; } + + analysis_result_file="$(mktemp "${observer_tmp_dir}/ecc-observer-result.XXXXXX")" + if [ -z "$analysis_result_file" ] || [ ! -f "$analysis_result_file" ]; then + echo "[$(date)] Failed to create observer result file, skipping analysis" >> "$LOG_FILE" + cleanup_analysis_resources + return + fi + ACTIVE_RESULT_FILE="$analysis_result_file" + + # Keep validation bound to the inode created by mktemp. Removing the path + # after opening both descriptors prevents a workspace process from replacing + # it with a forged completion record while Claude is running. + RESULT_FDS_OPEN=1 + if ! { exec 7<"$analysis_result_file" && exec 9<"$analysis_result_file" && exec 8>"$analysis_result_file"; }; then + echo "[$(date)] Failed to open observer result descriptors, skipping analysis" >> "$LOG_FILE" + cleanup_analysis_resources + return + fi + if ! rm -f "$analysis_result_file" || [ -e "$analysis_result_file" ] || [ -L "$analysis_result_file" ]; then + echo "[$(date)] Failed to unlink observer result file, skipping analysis" >> "$LOG_FILE" + cleanup_analysis_resources + return + fi # Prevent observe.sh from recording this automated observer session as observations. # Pass prompt via -p flag instead of stdin redirect for Windows compatibility (#842). @@ -262,34 +392,77 @@ PROMPT # e.g. ECC_OBSERVER_MODEL=opus for higher-quality instinct extraction. Heavier models are # slower — consider raising ECC_OBSERVER_TIMEOUT_SECONDS (default 120s) so the watchdog # doesn't kill the analysis mid-run. + # Job control gives the background Claude command its own process group on + # Bash, including macOS's Bash 3.2 and Git Bash. That lets timeout/signal + # cleanup terminate tool subprocesses as well as the direct CLI process. + set -m ECC_SKIP_OBSERVE=1 ECC_HOOK_PROFILE=minimal claude --model "${ECC_OBSERVER_MODEL:-haiku}" --max-turns "$max_turns" --print \ --allowedTools "Read,Write" \ - -p "$prompt_content" < /dev/null >> "$LOG_FILE" 2>&1 & - claude_pid=$! + -p "$prompt_content" < /dev/null >&8 2>> "$LOG_FILE" & + CLAUDE_PID=$! + CLAUDE_PROCESS_GROUP=1 + set +m ( sleep "$timeout_seconds" - if kill -0 "$claude_pid" 2>/dev/null; then + if claude_process_alive "$CLAUDE_PID"; then echo "[$(date)] Claude analysis timed out after ${timeout_seconds}s; terminating process" >> "$LOG_FILE" - kill "$claude_pid" 2>/dev/null || true + signal_claude_process "$CLAUDE_PID" TERM + grace_attempts=0 + while claude_process_alive "$CLAUDE_PID" && [ "$grace_attempts" -lt 20 ]; do + sleep 0.1 + grace_attempts=$((grace_attempts + 1)) + done + if claude_process_alive "$CLAUDE_PID"; then + echo "[$(date)] Claude analysis ignored TERM; killing process" >> "$LOG_FILE" + signal_claude_process "$CLAUDE_PID" KILL + fi fi - ) & - watchdog_pid=$! + ) /dev/null 2>&1 7<&- 8>&- 9<&- & + WATCHDOG_PID=$! - wait_for_claude_analysis "$claude_pid" + wait_for_claude_analysis "$CLAUDE_PID" exit_code=$? - kill "$watchdog_pid" 2>/dev/null || true + completed_claude_pid="$CLAUDE_PID" + CLAUDE_PID="" + kill "$WATCHDOG_PID" 2>/dev/null || true + wait "$WATCHDOG_PID" 2>/dev/null || true + WATCHDOG_PID="" + # A successful CLI can still leave tool subprocesses behind. Terminate any + # remaining members before closing the inherited result descriptors. + if claude_process_alive "$completed_claude_pid"; then + stop_claude_process "$completed_claude_pid" + else + CLAUDE_PROCESS_GROUP=0 + fi + { exec 8>&-; } 2>/dev/null || true + + analysis_complete=0 + if awk '{ sub(/\r$/, "", $0); if ($0 == "{\"status\":\"analysis_complete\"}") count++; if (NF) last = $0 } END { exit !(count == 1 && last == "{\"status\":\"analysis_complete\"}") }' <&7; then + analysis_complete=1 + fi + cat <&9 >> "$LOG_FILE" 2>/dev/null || true + { exec 7<&-; } 2>/dev/null || true + { exec 9<&-; } 2>/dev/null || true + RESULT_FDS_OPEN=0 + rm -f "$analysis_result_file" rm -f "$analysis_file" + ACTIVE_RESULT_FILE="" + ACTIVE_ANALYSIS_FILE="" if [ "$exit_code" -ne 0 ]; then echo "[$(date)] Claude analysis failed (exit $exit_code); retaining observations for retry" >> "$LOG_FILE" return fi - # Archive observations only after a successful analysis. A transient - # failure (timeout, non-zero exit, rate limit) must not discard the batch - # before it has been turned into instincts, since the analyzer only ever - # reads the live observations file (#2370). + if [ "$analysis_complete" -ne 1 ]; then + echo "[$(date)] Claude analysis incomplete (completion record missing); retaining observations for retry" >> "$LOG_FILE" + return + fi + + # Archive observations only after process success and the current analysis + # result's exact completion record. A semantic failure can still exit zero, + # so exit status alone must not discard the only live copy (#2370, #2673). if [ -f "$OBSERVATIONS_FILE" ]; then archive_dir="${PROJECT_DIR}/observations.archive" mkdir -p "$archive_dir" diff --git a/tests/hooks/observer-loop-archive.test.js b/tests/hooks/observer-loop-archive.test.js index 56676e76b..f35089282 100644 --- a/tests/hooks/observer-loop-archive.test.js +++ b/tests/hooks/observer-loop-archive.test.js @@ -1,19 +1,10 @@ /** - * Tests for observer-loop archive-on-failure fix (#2370) + * Tests for observer-loop archive-on-failure fixes (#2370, #2673). * - * Bug: analyze_observations() in observer-loop.sh moved the live - * observations.jsonl into observations.archive/ unconditionally, even when - * the Claude analysis step failed (timeout, non-zero exit, rate limit). - * Because the analyzer only ever reads the live file, a failed batch could - * never be re-analyzed and its instincts were silently lost. - * - * Fix: archive only after a successful analysis; on failure log and return, - * retaining observations for the next cycle to retry. - * - * Strategy: source observer-loop.sh (a BASH_SOURCE guard stops the main - * loop from running when sourced) and drive analyze_observations directly - * with a stub `claude` (exit code controlled per case) and a stub sibling - * session-guardian.sh. Assert symmetric outcomes for failure vs success. + * A batch may be archived only when the Claude process exits successfully and + * its current stdout contains one exact completion record as the final + * non-empty line. Process failures, semantic failures, stderr/log markers, + * duplicate markers, and tampering with the result path must fail closed. * * Run with: node tests/hooks/observer-loop-archive.test.js */ @@ -26,6 +17,8 @@ const { spawnSync } = require('child_process'); let passed = 0; let failed = 0; +let skipped = 0; +const SKIP = Symbol('skip'); function test(name, fn) { try { @@ -33,12 +26,23 @@ function test(name, fn) { console.log(` ✓ ${name}`); passed++; } catch (err) { + if (err === SKIP) { + console.log(` - ${name} (skipped: requires bash fixture)`); + skipped++; + return; + } console.log(` ✗ ${name}`); console.log(` Error: ${err.message}`); failed++; } } +function skipOnWindows() { + if (process.platform === 'win32') { + throw SKIP; + } +} + function createTempDir() { return fs.mkdtempSync(path.join(os.tmpdir(), 'ecc-observer-archive-')); } @@ -47,7 +51,17 @@ function cleanupDir(dir) { try { fs.rmSync(dir, { recursive: true, force: true }); } catch { - // ignore cleanup errors + // Ignore cleanup errors in an already-isolated test directory. + } +} + +function processExists(pid) { + if (!Number.isInteger(pid) || pid <= 0) return false; + try { + process.kill(pid, 0); + return true; + } catch { + return false; } } @@ -55,148 +69,316 @@ const repoRoot = path.resolve(__dirname, '..', '..'); const observerLoopPath = path.join( repoRoot, 'skills', 'continuous-learning-v2', 'agents', 'observer-loop.sh' ); +const ANALYSIS_COMPLETE_RECORD = '{"status":"analysis_complete"}'; +const ORIGINAL_OBSERVATIONS = '{"a":1}\n{"a":2}\n{"a":3}\n'; /** - * Run analyze_observations once with the given stub claude exit code. - * Returns { liveExists, archivedCount, log } describing the resulting state. + * Source observer-loop.sh in a sandbox and invoke analyze_observations once. */ -function runAnalyzeOnce(claudeExitCode) { +function runAnalyzeOnce(options = {}) { + const { + claudeExitCode = 0, + claudeOutput = '', + claudeStderr = '', + claudeDelaySeconds = 0, + claudeIgnoreTerm = false, + claudeSpawnChild = false, + existingLog = '', + observerTimeoutSeconds = 10, + probeResultPath = false, + } = options; const sandbox = createTempDir(); + try { const binDir = path.join(sandbox, 'bin'); const projectDir = path.join(sandbox, 'project'); + const observerFixtureDir = path.join(sandbox, 'observer'); fs.mkdirSync(binDir, { recursive: true }); fs.mkdirSync(projectDir, { recursive: true }); + fs.mkdirSync(observerFixtureDir, { recursive: true }); - // Stub claude: exit with the requested code, ignoring all args. const claudeStub = path.join(binDir, 'claude'); - fs.writeFileSync(claudeStub, '#!/usr/bin/env bash\nexit ${CLAUDE_STUB_EXIT:-0}\n'); + fs.writeFileSync(claudeStub, [ + '#!/usr/bin/env bash', + "printf '%s\n' \"$$\" > \"${CLAUDE_STUB_PID_FILE}\"", + "if [ \"${CLAUDE_STUB_IGNORE_TERM:-false}\" = \"true\" ]; then trap '' TERM; fi", + 'if [ "${CLAUDE_STUB_SPAWN_CHILD:-false}" = "true" ]; then', + ' sleep "${CLAUDE_STUB_DELAY_SECONDS:-10}" &', + " printf '%s\n' \"$!\" > \"${CLAUDE_STUB_CHILD_PID_FILE}\"", + ' wait', + 'fi', + 'if [ "${CLAUDE_STUB_PROBE_RESULT_PATH:-false}" = "true" ]; then', + ' for candidate in "${PROJECT_DIR}"/.observer-tmp/ecc-observer-result.*; do', + ' [ -e "$candidate" ] || [ -L "$candidate" ] || continue', + " printf 'found\n' > \"${CLAUDE_STUB_ATTACK_FILE}\"", + ' rm -f "$candidate"', + ` printf '%s\n' '${ANALYSIS_COMPLETE_RECORD}' > "$candidate"`, + ' done', + 'fi', + 'if [ "${CLAUDE_STUB_DELAY_SECONDS:-0}" != "0" ]; then', + ' sleep "${CLAUDE_STUB_DELAY_SECONDS}"', + 'fi', + "printf '%s' \"${CLAUDE_STUB_OUTPUT:-}\"", + "printf '%s' \"${CLAUDE_STUB_STDERR:-}\" >&2", + 'exit "${CLAUDE_STUB_EXIT:-0}"', + '', + ].join('\n')); fs.chmodSync(claudeStub, 0o755); - // analyze_observations resolves the real session-guardian.sh via its own - // ${BASH_SOURCE[0]}-derived SCRIPT_DIR, so we drive the real guardian with - // all of its gates disabled/isolated (see env below) rather than stubbing it. + // Source a sandbox copy so the sibling guardian is deterministic. + const observerFixture = path.join(observerFixtureDir, 'observer-loop.sh'); + const guardianStub = path.join(observerFixtureDir, 'session-guardian.sh'); + fs.copyFileSync(observerLoopPath, observerFixture); + fs.writeFileSync(guardianStub, '#!/usr/bin/env bash\nexit 0\n'); + fs.chmodSync(guardianStub, 0o755); - // Driver sources observer-loop.sh (guard stops the main loop) then runs - // the single function under test. const driver = path.join(sandbox, 'driver.sh'); fs.writeFileSync( driver, - `#!/usr/bin/env bash\nsource ${JSON.stringify(observerLoopPath)}\nanalyze_observations\n` + `#!/usr/bin/env bash\nsource ${JSON.stringify(observerFixture)}\nanalyze_observations\n` ); fs.chmodSync(driver, 0o755); const observationsFile = path.join(projectDir, 'observations.jsonl'); - fs.writeFileSync(observationsFile, '{"a":1}\n{"a":2}\n{"a":3}\n'); + const logFile = path.join(projectDir, 'observer.log'); + const claudePidFile = path.join(projectDir, 'claude-stub.pid'); + const claudeChildPidFile = path.join(projectDir, 'claude-stub-child.pid'); + const attackFile = path.join(projectDir, 'result-path-attack-found'); + fs.writeFileSync(observationsFile, ORIGINAL_OBSERVATIONS); + if (existingLog) fs.writeFileSync(logFile, existingLog); - // Defensive: never leak CLAUDE_PLUGIN_ROOT into the ECC test shell (it - // contaminates this project's hook-root resolution). - const childEnv = Object.assign({}, process.env); - delete childEnv.CLAUDE_PLUGIN_ROOT; - childEnv.PATH = binDir + path.delimiter + process.env.PATH; - childEnv.CLAUDE_STUB_EXIT = String(claudeExitCode); - childEnv.OBSERVATIONS_FILE = observationsFile; - childEnv.MIN_OBSERVATIONS = '1'; - childEnv.PROJECT_DIR = projectDir; - childEnv.LOG_FILE = path.join(projectDir, 'observer.log'); - childEnv.PROJECT_NAME = 'test-project'; - childEnv.PROJECT_ID = 'test-project'; - childEnv.INSTINCTS_DIR = path.join(projectDir, 'instincts'); - childEnv.CONFIG_DIR = projectDir; - childEnv.CLV2_IS_WINDOWS = 'false'; - childEnv.ECC_OBSERVER_TIMEOUT_SECONDS = '2'; - // Make the real session-guardian.sh deterministically proceed (exit 0): - // disable the active-hours and idle gates, isolate the cooldown log, and - // zero the cooldown interval so a fresh project always passes. - childEnv.OBSERVER_ACTIVE_HOURS_START = '0'; - childEnv.OBSERVER_ACTIVE_HOURS_END = '0'; - childEnv.OBSERVER_MAX_IDLE_SECONDS = '0'; - childEnv.OBSERVER_INTERVAL_SECONDS = '0'; - childEnv.OBSERVER_LAST_RUN_LOG = path.join(projectDir, 'observer-last-run.log'); + const inheritedEnv = Object.fromEntries( + Object.entries(process.env).filter( + ([key]) => key !== 'CLAUDE_PLUGIN_ROOT' && !key.startsWith('ECC_OBSERVER_') + ) + ); + const childEnv = { + ...inheritedEnv, + PATH: binDir + path.delimiter + process.env.PATH, + CLAUDE_STUB_EXIT: String(claudeExitCode), + CLAUDE_STUB_OUTPUT: claudeOutput, + CLAUDE_STUB_STDERR: claudeStderr, + CLAUDE_STUB_DELAY_SECONDS: String(claudeDelaySeconds), + CLAUDE_STUB_IGNORE_TERM: String(claudeIgnoreTerm), + CLAUDE_STUB_SPAWN_CHILD: String(claudeSpawnChild), + CLAUDE_STUB_PROBE_RESULT_PATH: String(probeResultPath), + CLAUDE_STUB_PID_FILE: claudePidFile, + CLAUDE_STUB_CHILD_PID_FILE: claudeChildPidFile, + CLAUDE_STUB_ATTACK_FILE: attackFile, + OBSERVATIONS_FILE: observationsFile, + MIN_OBSERVATIONS: '1', + PROJECT_DIR: projectDir, + LOG_FILE: logFile, + PROJECT_NAME: 'test-project', + PROJECT_ID: 'test-project', + INSTINCTS_DIR: path.join(projectDir, 'instincts'), + CONFIG_DIR: projectDir, + CLV2_IS_WINDOWS: 'false', + ECC_OBSERVER_TIMEOUT_SECONDS: String(observerTimeoutSeconds), + ECC_OBSERVER_MAX_ANALYSIS_LINES: '500', + ECC_OBSERVER_MAX_TURNS: '20', + ECC_OBSERVER_MODEL: 'haiku', + ECC_OBSERVER_ALLOW_WINDOWS: 'false', + }; + const startedAt = Date.now(); const result = spawnSync('bash', [driver], { encoding: 'utf8', timeout: 15000, - env: childEnv + env: childEnv, }); + const durationMs = Date.now() - startedAt; + assert.ifError(result.error); assert.strictEqual( - result.status, 0, + result.status, + 0, `driver should exit 0, got ${result.status}; stderr: ${result.stderr}` ); const archiveDir = path.join(projectDir, 'observations.archive'); - let archivedCount = 0; - if (fs.existsSync(archiveDir)) { - archivedCount = fs.readdirSync(archiveDir) - .filter(f => /^processed-.*\.jsonl$/.test(f)).length; - } - let log = ''; - try { log = fs.readFileSync(childEnv.LOG_FILE, 'utf8'); } catch { /* none */ } + const archivedContents = fs.existsSync(archiveDir) + ? fs.readdirSync(archiveDir) + .filter(file => /^processed-.*\.jsonl$/.test(file)) + .sort() + .map(file => fs.readFileSync(path.join(archiveDir, file), 'utf8')) + : []; + const liveContent = fs.existsSync(observationsFile) + ? fs.readFileSync(observationsFile, 'utf8') + : null; + const log = fs.existsSync(logFile) ? fs.readFileSync(logFile, 'utf8') : ''; + const observerTempDir = path.join(projectDir, '.observer-tmp'); + const tempEntries = fs.existsSync(observerTempDir) + ? fs.readdirSync(observerTempDir) + : []; + const claudePid = fs.existsSync(claudePidFile) + ? Number(fs.readFileSync(claudePidFile, 'utf8').trim()) + : null; + const claudeChildPid = fs.existsSync(claudeChildPidFile) + ? Number(fs.readFileSync(claudeChildPidFile, 'utf8').trim()) + : null; - return { liveExists: fs.existsSync(observationsFile), archivedCount, log }; + return { + archivedContents, + attackFound: fs.existsSync(attackFile), + claudeStillRunning: processExists(claudePid), + claudeChildStillRunning: processExists(claudeChildPid), + durationMs, + liveContent, + log, + tempEntries, + }; } finally { cleanupDir(sandbox); } } -console.log('\n=== Observer-loop Archive-on-Failure Tests (#2370) ===\n'); +function assertOriginalBatchIsRetryable(state) { + assert.strictEqual( + state.liveContent, + ORIGINAL_OBSERVATIONS, + 'the live batch must remain byte-for-byte intact for retry' + ); + assert.deepStrictEqual(state.archivedContents, []); +} +console.log('\n=== Observer-loop Archive-on-Failure Tests (#2370, #2673) ===\n'); console.log('--- behavioral ---'); test('failed analysis retains observations and archives nothing', () => { - // Shell-driven behavioral check; skip on Windows where the bash driver's - // $0 path handling differs (matches observer-memory.test.js convention). - if (process.platform === 'win32') { - return; - } - const { liveExists, archivedCount, log } = runAnalyzeOnce(1); - assert.ok(liveExists, 'live observations.jsonl must be retained when analysis fails'); - assert.strictEqual(archivedCount, 0, 'nothing should be archived when analysis fails'); - assert.ok( - /retaining observations for retry/.test(log), - `failure log should note retention; got: ${log}` - ); + skipOnWindows(); + const state = runAnalyzeOnce({ + claudeExitCode: 1, + claudeOutput: `${ANALYSIS_COMPLETE_RECORD}\n`, + }); + assertOriginalBatchIsRetryable(state); + assert.match(state.log, /retaining observations for retry/); + assert.deepStrictEqual(state.tempEntries, []); }); -test('successful analysis archives the batch (happy path preserved)', () => { - // Shell-driven behavioral check; skip on Windows (see note above). - if (process.platform === 'win32') { - return; - } - const { liveExists, archivedCount } = runAnalyzeOnce(0); - assert.ok(!liveExists, 'live observations.jsonl should be moved after a successful analysis'); - assert.strictEqual(archivedCount, 1, 'exactly one processed-*.jsonl should be archived on success'); +test('zero-exit analysis without a completion record retains observations', () => { + skipOnWindows(); + const state = runAnalyzeOnce({ + claudeOutput: 'Analysis blocked because the sampled file was not found.\n', + }); + assertOriginalBatchIsRetryable(state); + assert.match(state.log, /completion record missing.*retaining observations for retry/i); + assert.deepStrictEqual(state.tempEntries, []); +}); + +test('mentioning the completion record in prose does not authorize archival', () => { + skipOnWindows(); + const state = runAnalyzeOnce({ + claudeOutput: `I would emit ${ANALYSIS_COMPLETE_RECORD} after analysis, but the read failed.\n`, + }); + assertOriginalBatchIsRetryable(state); +}); + +test('completion record followed by failure text does not authorize archival', () => { + skipOnWindows(); + const state = runAnalyzeOnce({ + claudeOutput: `${ANALYSIS_COMPLETE_RECORD}\nLater failure: instinct write did not complete.\n`, + }); + assertOriginalBatchIsRetryable(state); +}); + +test('a completion record from an older log entry cannot authorize this run', () => { + skipOnWindows(); + const state = runAnalyzeOnce({ + existingLog: `prior run\n${ANALYSIS_COMPLETE_RECORD}\n`, + claudeOutput: 'Current run could not read its analysis file.\n', + }); + assertOriginalBatchIsRetryable(state); +}); + +test('a completion record written only to stderr does not authorize archival', () => { + skipOnWindows(); + const state = runAnalyzeOnce({ + claudeOutput: 'Analysis did not complete.\n', + claudeStderr: `${ANALYSIS_COMPLETE_RECORD}\n`, + }); + assertOriginalBatchIsRetryable(state); + assert.ok(state.log.includes(ANALYSIS_COMPLETE_RECORD)); +}); + +test('duplicate exact completion records do not authorize archival', () => { + skipOnWindows(); + const state = runAnalyzeOnce({ + claudeOutput: `${ANALYSIS_COMPLETE_RECORD}\n${ANALYSIS_COMPLETE_RECORD}\n`, + }); + assertOriginalBatchIsRetryable(state); +}); + +test('replacing the result pathname cannot forge completion', () => { + skipOnWindows(); + const state = runAnalyzeOnce({ + claudeOutput: 'Analysis did not complete.\n', + probeResultPath: true, + }); + assertOriginalBatchIsRetryable(state); + assert.strictEqual(state.attackFound, false, 'the open result inode must not remain path-addressable'); +}); + +test('successful analysis archives the original batch byte-for-byte', () => { + skipOnWindows(); + const state = runAnalyzeOnce({ + claudeOutput: `Analysis finished.\n\n${ANALYSIS_COMPLETE_RECORD}\n\n`, + }); + assert.strictEqual(state.liveContent, null); + assert.deepStrictEqual(state.archivedContents, [ORIGINAL_OBSERVATIONS]); + assert.deepStrictEqual(state.tempEntries, []); +}); + +test('completion record accepts a CRLF line ending', () => { + skipOnWindows(); + const state = runAnalyzeOnce({ + claudeOutput: `Analysis complete.\r\n${ANALYSIS_COMPLETE_RECORD}\r\n\r\n`, + }); + assert.strictEqual(state.liveContent, null); + assert.deepStrictEqual(state.archivedContents, [ORIGINAL_OBSERVATIONS]); +}); + +test('watchdog force-stops a process that ignores TERM and retains observations', () => { + skipOnWindows(); + const state = runAnalyzeOnce({ + claudeDelaySeconds: 10, + claudeIgnoreTerm: true, + claudeSpawnChild: true, + observerTimeoutSeconds: 1, + }); + assertOriginalBatchIsRetryable(state); + assert.ok(state.durationMs < 8000, `watchdog should return promptly; took ${state.durationMs}ms`); + assert.strictEqual(state.claudeStillRunning, false, 'timed-out Claude process must be reaped'); + assert.strictEqual(state.claudeChildStillRunning, false, 'timed-out Claude descendants must stop'); + assert.match(state.log, /timed out after 1s/); + assert.deepStrictEqual(state.tempEntries, []); }); console.log('--- static guards ---'); -test('analyze_observations returns on failure before the archive mv', () => { +test('process and semantic failure guards run before archival', () => { const content = fs.readFileSync(observerLoopPath, 'utf8'); - // Operate on full file content with explicit anchors rather than a lazy - // function-body extraction (which could truncate on a future inner "\n}" - // and pass vacuously). These tokens each occur once, inside the function. - const failIdx = content.search(/exit_code"?\s+-ne\s+0/); - const returnIdx = content.indexOf('return', failIdx); + const processGuardIdx = content.search(/exit_code"?\s+-ne\s+0/); + const semanticGuardIdx = content.indexOf('if [ "$analysis_complete" -ne 1 ]'); const archiveIdx = content.indexOf('observations.archive'); - assert.ok(failIdx !== -1, 'should find the non-zero exit_code check'); - assert.ok(archiveIdx !== -1, 'should find the archive block'); - assert.ok(returnIdx !== -1, 'failure branch should contain a return'); - assert.ok(returnIdx < archiveIdx, - 'failure branch must return before reaching the archive block'); + assert.ok(processGuardIdx !== -1); + assert.ok(semanticGuardIdx !== -1); + assert.ok(archiveIdx !== -1); + assert.ok(processGuardIdx < archiveIdx); + assert.ok(semanticGuardIdx < archiveIdx); }); -test('observer-loop.sh has a source-guard so it can be sourced in tests', () => { +test('observer-loop.sh has a source guard', () => { const content = fs.readFileSync(observerLoopPath, 'utf8'); assert.ok( - content.includes('BASH_SOURCE[0]') && content.includes('return 0 2>/dev/null'), - 'observer-loop.sh should short-circuit when sourced rather than executed' + content.includes('BASH_SOURCE[0]') && content.includes('return 0 2>/dev/null') ); }); console.log('\n=== Test Results ==='); console.log(`Passed: ${passed}`); console.log(`Failed: ${failed}`); -console.log(`Total: ${passed + failed}\n`); +console.log(`Skipped: ${skipped}`); +console.log(`Total: ${passed + failed + skipped}\n`); process.exit(failed > 0 ? 1 : 0);