From 972dbe1078159025135108db9674278a0d0d527f Mon Sep 17 00:00:00 2001 From: Rex Lorenzo Date: Fri, 7 Aug 2026 23:05:29 -0700 Subject: [PATCH 1/2] feat(review-loop): log agent output so a failed run can be diagnosed A run that dies leaves nothing behind. Agent output went to the terminal and nowhere else, so a loop that stopped mid-refinement showed a bare "Execution error" with no way to tell a crash from a timeout from an external kill, and no record to read afterwards. A run started in the background does not even have the scrollback. run_agent mirrors the agent's combined output to AGENT_LOG when set, stamped with the agent, its tools, and the exit code. Dispatch moves to _dispatch_agent so the wrapper lives in one place. It returns PIPESTATUS[0] rather than the pipeline status, since tee succeeds even when the agent does not and the caller's `|| local_exit=$?` has to see the agent's own code. code-review-loop claims a directory per run and names a log per step, printed in the banner and again whenever an agent fails. The directory sits outside TMPDIR_REVIEW, which the EXIT trap wipes, and outside the project, where the staging and artifact-cleanup passes would otherwise pick the logs up as agent output. CODE_REVIEW_LOOP_LOG_DIR moves the root; each run still gets its own timestamped directory beneath it, claimed with mkdir so two loops starting in the same second cannot share one. is_inside_dir backs both exclusions. Resolving the paths rather than trimming a prefix is what makes a relative, trailing-slashed, or symlinked log directory compare correctly. --- README.md | 25 +++++++ bin/code-review-loop | 30 ++++++-- lib/lib-review-loop | 64 ++++++++++++++++- test/code-review-loop.bats | 73 +++++++++++++++++++ test/lib-review-loop.bats | 141 +++++++++++++++++++++++++++++++++++++ 5 files changed, 327 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 3652cb8..d7f5a68 100644 --- a/README.md +++ b/README.md @@ -190,6 +190,24 @@ code-review-loop --editor claude --reviewer codex **Outputs (project root):** `agent-code-review.md` (latest findings), `agent-review-summary.md` (narrative). +**When a run fails, read the logs.** Each agent's full output is written to a per-run directory, printed in the banner at startup and again whenever an agent exits non-zero: + +```text + Logs : ~/.cache/code-review-loop/20260807-142516 +``` + +One file per step, named for the step and the agent that ran it: + +```text +1-refinement.claude.log +3-review-initial.antigravity.log +4.1-response.claude.log +6.1-review.antigravity.log +final-summary.claude.log +``` + +Each records the agent, the tools it was allowed, its combined stdout and stderr, and its exit code. This is the difference between "it failed" and knowing why: a loop that stops with a bare `Execution error` on the terminal leaves nothing else behind, and a run started in the background does not even have the scrollback. Note that an agent failing does not stop the loop; it logs the failure and carries on, so the log is often the only sign a step went wrong. + ### plan-review-loop Iteratively improves a **plan document** through review feedback: @@ -223,6 +241,13 @@ Two caveats for `kimi`: it takes its prompt as a command-line argument (there is Both loops write their working files (`agent-code-review.md`, `agent-review-summary.md`, `feedback-plan.md`, `plan-review-summary.md`) to the target project's root. Consider adding those names to that project's `.gitignore` (or your global gitignore) so an agent never commits them by accident. +Environment variables: + +| Variable | Default | Effect | +| --- | --- | --- | +| `CODE_REVIEW_LOOP_LOG_DIR` | `~/.cache/code-review-loop` | The directory `code-review-loop` writes its per-run log directories under. | +| `AI_CODING_SETUP_PROMPTS_DIR` | `~/.local/share/ai-coding-setup/prompts` | Where the loops read their prompts from. | + ### Shared prompts Both loops are driven by agent-agnostic prompts in [prompts/](prompts/), not interactive commands. They're listed here so you can audit or tweak the behavior: diff --git a/bin/code-review-loop b/bin/code-review-loop index ea38a8a..d6c22d7 100755 --- a/bin/code-review-loop +++ b/bin/code-review-loop @@ -121,6 +121,8 @@ stage_review_changes() { if [[ -n "$new_untracked" ]]; then while IFS= read -r file; do [[ -z "$file" ]] && continue + # A log records the run; it is not part of the change. + is_inside_dir "$file" "${RUN_LOG_DIR:-}" && continue git add -- "$file" write_status "Staged new file: $file" "$DIM" done <<< "$new_untracked" @@ -166,6 +168,13 @@ fi setup_temp "code-review-loop" trap cleanup_temp EXIT +# ---- run logs ------------------------------------------------------------- +# Outside TMPDIR_REVIEW, which the EXIT trap wipes, and outside the project, +# where stage_review_changes would commit them. CODE_REVIEW_LOOP_LOG_DIR names +# the directory runs go under, not the run itself. +RUN_LOG_ROOT="${CODE_REVIEW_LOOP_LOG_DIR:-$HOME/.cache/code-review-loop}" +RUN_LOG_DIR=$(claim_run_log_dir "$RUN_LOG_ROOT") + echo "" echo -e "${MAGENTA}========================================${NC}" echo -e "${MAGENTA} Code Review Loop${NC}" @@ -174,6 +183,7 @@ echo " Max iterations : $MAX_ITERATIONS" echo " Skip refinement: $SKIP_REFINEMENT" echo " Editor : $EDITOR_AGENT" echo " Reviewer : $REVIEWER_AGENT" +echo " Logs : $RUN_LOG_DIR" start_time=$(date +%s) @@ -219,11 +229,13 @@ if [[ "$SKIP_REFINEMENT" == false ]]; then snapshot_untracked > "$pre_snapshot" local_exit=0 - run_agent "$EDITOR_AGENT" "$refinement_prompt" "$EDITOR_TOOLS" || local_exit=$? + AGENT_LOG="$RUN_LOG_DIR/1-refinement.$EDITOR_AGENT.log" \ + run_agent "$EDITOR_AGENT" "$refinement_prompt" "$EDITOR_TOOLS" || local_exit=$? cleanup_agent_artifacts "$EDITOR_AGENT" "$pre_snapshot" "editor" if [[ $local_exit -ne 0 ]]; then write_status "$EDITOR_AGENT code-refinement exited with code $local_exit" "$YELLOW" + write_status "Log: $RUN_LOG_DIR/1-refinement.$EDITOR_AGENT.log" "$YELLOW" else write_status "Code refinement complete" "$GREEN" fi @@ -262,15 +274,18 @@ pre_snapshot="$TMPDIR_REVIEW/pre-review-initial.txt" snapshot_untracked > "$pre_snapshot" local_exit=0 -run_agent "$REVIEWER_AGENT" "$review_task" "$REVIEWER_TOOLS" || local_exit=$? +AGENT_LOG="$RUN_LOG_DIR/3-review-initial.$REVIEWER_AGENT.log" \ + run_agent "$REVIEWER_AGENT" "$review_task" "$REVIEWER_TOOLS" || local_exit=$? cleanup_agent_artifacts "$REVIEWER_AGENT" "$pre_snapshot" "reviewer" if [[ $local_exit -ne 0 ]]; then write_status "$REVIEWER_AGENT code review exited with code $local_exit" "$YELLOW" + write_status "Log: $RUN_LOG_DIR/3-review-initial.$REVIEWER_AGENT.log" "$YELLOW" fi if [[ ! -f "$REVIEW_FILE" ]]; then write_status "No review file created; $REVIEWER_AGENT may have failed" "$RED" + write_status "Log: $RUN_LOG_DIR/3-review-initial.$REVIEWER_AGENT.log" "$RED" echo "" echo -e "${MAGENTA}========================================${NC}" echo -e "${MAGENTA} Code Review Loop Complete${NC}" @@ -315,7 +330,8 @@ Read it, evaluate each finding, implement valid fixes, and respond inline per th snapshot_untracked > "$pre_snapshot" local_exit=0 - run_agent "$EDITOR_AGENT" "$context_prompt" "$EDITOR_TOOLS" || local_exit=$? + AGENT_LOG="$RUN_LOG_DIR/4.$iteration-response.$EDITOR_AGENT.log" \ + run_agent "$EDITOR_AGENT" "$context_prompt" "$EDITOR_TOOLS" || local_exit=$? cleanup_agent_artifacts "$EDITOR_AGENT" "$pre_snapshot" "editor" if [[ $local_exit -ne 0 ]]; then @@ -343,7 +359,8 @@ IMPORTANT: You MUST overwrite agent-code-review.md with your updated findings." snapshot_untracked > "$pre_snapshot" local_exit=0 - run_agent "$REVIEWER_AGENT" "$followup_task" "$REVIEWER_TOOLS" || local_exit=$? + AGENT_LOG="$RUN_LOG_DIR/6.$iteration-review.$REVIEWER_AGENT.log" \ + run_agent "$REVIEWER_AGENT" "$followup_task" "$REVIEWER_TOOLS" || local_exit=$? cleanup_agent_artifacts "$REVIEWER_AGENT" "$pre_snapshot" "reviewer" if [[ $local_exit -ne 0 ]]; then @@ -437,7 +454,8 @@ Write agent-review-summary.md with this structure: Be concise and focus on the substance of review-driven improvements, not the original feature work or process details." local_exit=0 -run_agent "$EDITOR_AGENT" "$summary_prompt" "Read,Write,Grep,Glob" || local_exit=$? +AGENT_LOG="$RUN_LOG_DIR/final-summary.$EDITOR_AGENT.log" \ + run_agent "$EDITOR_AGENT" "$summary_prompt" "Read,Write,Grep,Glob" || local_exit=$? if [[ $local_exit -ne 0 ]]; then write_status "$EDITOR_AGENT summary generation exited with code $local_exit" "$YELLOW" @@ -487,4 +505,6 @@ if [[ -n "$STASH_REF" ]]; then echo "" fi fi +echo -e " Logs : $RUN_LOG_DIR" +echo "" echo -e "${CYAN}Waiting for manual review.${NC}" diff --git a/lib/lib-review-loop b/lib/lib-review-loop index 67f8bd7..a284e3f 100644 --- a/lib/lib-review-loop +++ b/lib/lib-review-loop @@ -227,7 +227,7 @@ agent_command() { # $1: agent name ("claude", "codex", "copilot", "antigravity", or "kimi") # $2: prompt text # $3: allowed tools (only used by Claude; ignored by others) -run_agent() { +_dispatch_agent() { local agent="$1" prompt="$2" tools="${3:-}" case "$agent" in claude) run_claude "$prompt" "$tools" ;; @@ -239,6 +239,30 @@ run_agent() { esac } +# Mirrors the agent's combined output to AGENT_LOG when set. Without it an agent +# that dies leaves only an exit code and whatever is still in scrollback. +run_agent() { + local agent="$1" prompt="$2" tools="${3:-}" + local rc=0 + + if [[ -z "${AGENT_LOG:-}" ]]; then + _dispatch_agent "$agent" "$prompt" "$tools" + return $? + fi + + mkdir -p "$(dirname "$AGENT_LOG")" + printf '=== %s agent=%s tools=%s ===\n' \ + "$(date '+%Y-%m-%d %H:%M:%S')" "$agent" "${tools:-default}" >> "$AGENT_LOG" + + # PIPESTATUS, not the pipeline's status: tee succeeds even when the agent + # does not, and the caller's `|| local_exit=$?` has to see the agent's code. + _dispatch_agent "$agent" "$prompt" "$tools" 2>&1 | tee -a "$AGENT_LOG" + rc=${PIPESTATUS[0]} + + printf '=== exit=%s ===\n\n' "$rc" >> "$AGENT_LOG" + return "$rc" +} + # ---- shared validation ---------------------------------------------------- # Validate that the configured editor/reviewer agents are installed. @@ -395,6 +419,41 @@ cleanup_temp() { [[ -n "$TMPDIR_REVIEW" ]] && rm -rf "$TMPDIR_REVIEW" } +# ---- paths ---------------------------------------------------------------- + +# True when $1 is a file inside directory $2. Both sides are resolved, so a +# relative, trailing-slashed, or symlinked argument compares correctly. +# $1: file path +# $2: directory path +is_inside_dir() { + local file="$1" dir="$2" file_abs dir_abs + [[ -n "$dir" && -d "$dir" ]] || return 1 + dir_abs=$(cd "$dir" && pwd -P) || return 1 + file_abs="$(cd "$(dirname "$file")" 2>/dev/null && pwd -P)/$(basename "$file")" + [[ "$file_abs" == "$dir_abs"/* ]] +} + +# ---- run log directories -------------------------------------------------- + +# Claim a run directory under $1 and print its path. +# Claimed by creating it: mkdir is atomic, so two loops starting in the same +# second cannot share one. The loser falls back to mktemp, also atomic (a pid is +# not enough: in a subshell $$ is the parent's, so every loser picks alike). +# $1: directory to create the run directory under +claim_run_log_dir() { + local root="$1" stamp dir + mkdir -p "$root" || return 1 + stamp=$(date '+%Y%m%d-%H%M%S') + dir="$root/$stamp" + if mkdir "$dir" 2>/dev/null; then + printf '%s\n' "$dir" + return 0 + fi + # Suffixed, so the name still starts with the run stamp. + dir=$(mktemp -d "$root/$stamp-XXXXXX") || return 1 + printf '%s\n' "$dir" +} + # ---- elapsed time -------------------------------------------------------- # Compute human-readable elapsed time from a start timestamp. @@ -447,6 +506,9 @@ cleanup_agent_artifacts() { if [[ " $KNOWN_REVIEW_FILES " == *" $base "* ]]; then continue fi + # A run log is not an agent artifact; it is only reachable here + # when the log dir sits inside the repo. + is_inside_dir "$file" "${RUN_LOG_DIR:-}" && continue rm -f "$file" 2>/dev/null || true write_status "Removed Codex artifact: $file" "$DIM" done <<< "$new_files" diff --git a/test/code-review-loop.bats b/test/code-review-loop.bats index 7d6c6ed..a4f809c 100644 --- a/test/code-review-loop.bats +++ b/test/code-review-loop.bats @@ -33,3 +33,76 @@ BIN="$PROJECT_ROOT/bin/code-review-loop" assert_failure assert_output --partial "Unknown agent" } + +# ========================================================================= +# Run log staging exclusion +# +# stage_review_changes must never offer a run log up as a review change. The +# log directory can sit inside the repo (CODE_REVIEW_LOOP_LOG_DIR allows it), +# and the value can arrive relative, with a trailing slash, or as the project +# root itself, each of which defeated an earlier string-prefix check. +# ========================================================================= + +# Runs the loop once against a throwaway repo with stub agents, and sets: +# log_root absolute path the logs were written under +# log_count how many .log files it produced +# staged_logs how many of them git ended up staging +run_loop_with_logs() { # run_loop_with_logs + cd "$BATS_TEST_TMPDIR" || return 1 + rm -rf repo && mkdir repo && cd repo || return 1 + git init -q . && git config user.email t@t && git config user.name t + echo tracked > f && git add . && git commit -qm init + echo changed > f && git add f + + mkdir -p stub + printf '#!/usr/bin/env bash\ncat >/dev/null\necho ran\nprintf "# R\\n\\nHigh: 0\\nMedium: 0\\nLow: 0\\n\\nVerdict: good to go\\n" > agent-code-review.md\n' > stub/claude + cp stub/claude stub/agy + chmod +x stub/claude stub/agy + PATH="$PWD/stub:$PATH" + # The suite sandboxes HOME, so the installed prompts are not reachable and + # the loop would exit at validate_prompts before writing a single log. + # Point at the checkout's own prompts: the test should not depend on + # whether ./setup has been run on this machine. + export AI_CODING_SETUP_PROMPTS_DIR="$BATS_TEST_DIRNAME/../prompts" + export CODE_REVIEW_LOOP_LOG_DIR="$1" + + # Agents named explicitly: without them the reviewer comes from + # ~/.ai-coding-setup.conf or the built-in default, so the test passes or + # fails on whether that machine happens to have codex installed. CI does + # not, and the loop exited at validate_tools before writing a log. + run "$BATS_TEST_DIRNAME/../bin/code-review-loop" -m 1 -e claude -r claude + log_root="$2" + log_count=$(find "$log_root" -name '*.log' -type f 2>/dev/null | wc -l | tr -d ' ') + staged_logs=$(git diff --staged --name-only | grep -cE '\.log$' || true) +} + +@test "run logs are not staged when the log dir is a relative in-repo path" { + run_loop_with_logs "mylogs" "$BATS_TEST_TMPDIR/repo/mylogs" + # Assert logs were actually produced, or "none staged" proves nothing. + [ "$log_count" -gt 0 ] + [ "$staged_logs" -eq 0 ] +} + +@test "run logs are not staged when the log dir has a trailing slash" { + run_loop_with_logs "$BATS_TEST_TMPDIR/repo/trailing/" "$BATS_TEST_TMPDIR/repo/trailing" + [ "$log_count" -gt 0 ] + [ "$staged_logs" -eq 0 ] +} + +@test "run logs are not staged when the log dir is the project root" { + run_loop_with_logs "$BATS_TEST_TMPDIR/repo" "$BATS_TEST_TMPDIR/repo" + [ "$log_count" -gt 0 ] + [ "$staged_logs" -eq 0 ] +} + +@test "each run gets its own log directory under a shared root" { + run_loop_with_logs "$BATS_TEST_TMPDIR/repo/shared" "$BATS_TEST_TMPDIR/repo/shared" + [ "$log_count" -gt 0 ] + git reset -q --hard HEAD + echo again > f && git add f + run "$BATS_TEST_DIRNAME/../bin/code-review-loop" -m 1 -e claude -r claude + # Two runs must not append into one set of step filenames. + local dirs + dirs=$(find "$BATS_TEST_TMPDIR/repo/shared" -mindepth 1 -maxdepth 1 -type d | wc -l | tr -d ' ') + [ "$dirs" -eq 2 ] +} diff --git a/test/lib-review-loop.bats b/test/lib-review-loop.bats index 66a67e1..13a4585 100644 --- a/test/lib-review-loop.bats +++ b/test/lib-review-loop.bats @@ -326,3 +326,144 @@ EOF # Allow for 1-second clock skew between date calls assert_output --regexp "^2m [56]s$" } + +# ========================================================================= +# Agent output logging +# +# run_agent dispatches to a real CLI, so these stub one onto PATH and assert +# only the wrapper's own behaviour: that it mirrors output, and that it hands +# back the agent's exit code rather than tee's. +# ========================================================================= + +stub_agent() { # stub_agent [stderr-line] + local dir="$BATS_TEST_TMPDIR/bin" + mkdir -p "$dir" + { + echo '#!/usr/bin/env bash' + echo 'cat >/dev/null' + printf 'echo %q\n' "$2" + [ -n "${3:-}" ] && printf 'echo %q >&2\n' "$3" + echo "exit $1" + } > "$dir/claude" + chmod +x "$dir/claude" + PATH="$dir:$PATH" +} + +@test "run_agent without AGENT_LOG passes output through and writes no file" { + source_lib + stub_agent 0 "refined the code" + unset AGENT_LOG + run run_agent claude "a prompt" "Read" + assert_success + assert_output --partial "refined the code" + [ -z "$(find "$BATS_TEST_TMPDIR" -name '*.log' -print -quit)" ] +} + +@test "run_agent mirrors output to AGENT_LOG and records the exit code" { + source_lib + stub_agent 0 "refined the code" + AGENT_LOG="$BATS_TEST_TMPDIR/logs/step.log" run run_agent claude "a prompt" "Read" + assert_success + assert_output --partial "refined the code" + run cat "$BATS_TEST_TMPDIR/logs/step.log" + assert_output --partial "agent=claude" + assert_output --partial "refined the code" + assert_output --partial "exit=0" +} + +@test "run_agent returns the agent's exit code, not tee's" { + source_lib + stub_agent 1 "partial work" "Execution error" + AGENT_LOG="$BATS_TEST_TMPDIR/logs/step.log" run run_agent claude "a prompt" "Read" + assert_failure + # The caller's `|| local_exit=$?` depends on this, and tee always succeeds. + [ "$status" -eq 1 ] +} + +@test "run_agent captures a failing agent's stderr in the log" { + source_lib + stub_agent 1 "partial work" "Execution error" + AGENT_LOG="$BATS_TEST_TMPDIR/logs/step.log" run run_agent claude "a prompt" "Read" + run cat "$BATS_TEST_TMPDIR/logs/step.log" + # The whole point: a run that dies leaves the reason on disk. + assert_output --partial "Execution error" + assert_output --partial "exit=1" +} + +@test "run_agent creates the log directory if it does not exist" { + source_lib + stub_agent 0 "ok" + AGENT_LOG="$BATS_TEST_TMPDIR/deep/nested/dir/step.log" run run_agent claude "p" "Read" + assert_success + [ -f "$BATS_TEST_TMPDIR/deep/nested/dir/step.log" ] +} + +@test "cleanup_agent_artifacts does not delete run logs inside the repo" { + source_lib + cd "$BATS_TEST_TMPDIR" + git init -q . && git config user.email t@t && git config user.name t + echo tracked > kept.txt && git add . && git commit -qm init + + RUN_LOG_DIR="$BATS_TEST_TMPDIR/logs/20260807-120000" + mkdir -p "$RUN_LOG_DIR" + + local before="$BATS_TEST_TMPDIR/before.txt" + snapshot_untracked > "$before" + + # Both appear during the run: one is a Codex artifact, one is our own log. + echo junk > artifact.txt + echo "agent output" > "$RUN_LOG_DIR/3-review.codex.log" + + run cleanup_agent_artifacts codex "$before" reviewer + assert_success + [ ! -f artifact.txt ] + [ -f "$RUN_LOG_DIR/3-review.codex.log" ] +} + +@test "claim_run_log_dir gives concurrent callers separate directories" { + source_lib + local root="$BATS_TEST_TMPDIR/logs" + local out="$BATS_TEST_TMPDIR/claimed.txt" + : > "$out" + + # Twenty at once, all inside the same second, which is exactly when a + # check-then-create claim hands two callers the same name. + local _ + for _ in $(seq 1 20); do + ( claim_run_log_dir "$root" >> "$out" ) & + done + wait + + local claimed unique + claimed=$(wc -l < "$out" | tr -d ' ') + unique=$(sort -u "$out" | wc -l | tr -d ' ') + [ "$claimed" -eq 20 ] + [ "$unique" -eq 20 ] +} + +@test "claim_run_log_dir creates the root when it is missing" { + source_lib + local dir + dir=$(claim_run_log_dir "$BATS_TEST_TMPDIR/deep/nested/root") + [ -d "$dir" ] + [[ "$dir" == "$BATS_TEST_TMPDIR/deep/nested/root/"* ]] +} + +@test "is_inside_dir resolves relative, trailing-slash and symlinked paths" { + source_lib + local root="$BATS_TEST_TMPDIR/root" + mkdir -p "$root/logs" "$root/other" + : > "$root/logs/a.log" + : > "$root/other/b.txt" + ln -s "$root/logs" "$root/linked" + cd "$root" || return 1 + + run is_inside_dir "logs/a.log" "logs"; assert_success + run is_inside_dir "logs/a.log" "logs/"; assert_success + run is_inside_dir "logs/a.log" "$root/logs"; assert_success + run is_inside_dir "logs/a.log" "linked"; assert_success + run is_inside_dir "other/b.txt" "logs"; assert_failure + # An empty or missing directory is never a container. + run is_inside_dir "logs/a.log" ""; assert_failure + run is_inside_dir "logs/a.log" "$root/nope"; assert_failure +} From 30612270321cf55554744c9cf7b56adeecf850fd Mon Sep 17 00:00:00 2001 From: Rex Lorenzo Date: Fri, 7 Aug 2026 23:06:13 -0700 Subject: [PATCH 2/2] feat(review-loop): keep one day of run logs Run logs outlive their run so a failure can still be read afterwards, which also meant nothing ever removed them: one directory per run, forever, in a cache directory. Old runs are pruned at startup, by age rather than by count. The loop gets run several times in a sitting and what you come back for is today's failure, so ten runs in an afternoon should not push out yesterday's, and a quiet week should not keep last month's. A day by default, REVIEW_LOOP_LOG_DAYS to change it. Two limits on what pruning touches. It only matches the YYYYmmdd-HHMMSS directories the loops create, so anything kept alongside them survives, and setting CODE_REVIEW_LOOP_LOG_DIR opts out entirely, since a directory the user named is theirs to manage. The README now says where the logs are, what each one holds, and that an agent failing does not stop the loop, so a log is often the only sign a step went wrong. --- README.md | 5 ++- bin/code-review-loop | 6 +++ lib/lib-review-loop | 31 ++++++++++++- test/lib-review-loop.bats | 91 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 131 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index d7f5a68..5ef9d33 100644 --- a/README.md +++ b/README.md @@ -208,6 +208,8 @@ final-summary.claude.log Each records the agent, the tools it was allowed, its combined stdout and stderr, and its exit code. This is the difference between "it failed" and knowing why: a loop that stops with a bare `Execution error` on the terminal leaves nothing else behind, and a run started in the background does not even have the scrollback. Note that an agent failing does not stop the loop; it logs the failure and carries on, so the log is often the only sign a step went wrong. +Logs are kept for **one day** and older runs are pruned at startup. Retention is by age rather than by count because the loop tends to be run several times in a sitting, and what you come back for is today's failure. Override with `REVIEW_LOOP_LOG_DAYS`, or set `CODE_REVIEW_LOOP_LOG_DIR` to keep logs somewhere of your own, which opts out of pruning entirely. + ### plan-review-loop Iteratively improves a **plan document** through review feedback: @@ -245,7 +247,8 @@ Environment variables: | Variable | Default | Effect | | --- | --- | --- | -| `CODE_REVIEW_LOOP_LOG_DIR` | `~/.cache/code-review-loop` | The directory `code-review-loop` writes its per-run log directories under. | +| `CODE_REVIEW_LOOP_LOG_DIR` | `~/.cache/code-review-loop/` | Where `code-review-loop` writes its run logs. Setting it also turns off log pruning, on the grounds that a directory you named is yours to manage. | +| `REVIEW_LOOP_LOG_DAYS` | `1` | Delete run logs older than this many days. Only applies to the default location. | | `AI_CODING_SETUP_PROMPTS_DIR` | `~/.local/share/ai-coding-setup/prompts` | Where the loops read their prompts from. | ### Shared prompts diff --git a/bin/code-review-loop b/bin/code-review-loop index d6c22d7..114ee49 100755 --- a/bin/code-review-loop +++ b/bin/code-review-loop @@ -175,6 +175,11 @@ trap cleanup_temp EXIT RUN_LOG_ROOT="${CODE_REVIEW_LOOP_LOG_DIR:-$HOME/.cache/code-review-loop}" RUN_LOG_DIR=$(claim_run_log_dir "$RUN_LOG_ROOT") +# Only the default location is pruned; a directory the user named is theirs. +if [[ -z "${CODE_REVIEW_LOOP_LOG_DIR:-}" ]]; then + pruned=$(prune_run_logs "$RUN_LOG_ROOT") || true +fi + echo "" echo -e "${MAGENTA}========================================${NC}" echo -e "${MAGENTA} Code Review Loop${NC}" @@ -184,6 +189,7 @@ echo " Skip refinement: $SKIP_REFINEMENT" echo " Editor : $EDITOR_AGENT" echo " Reviewer : $REVIEWER_AGENT" echo " Logs : $RUN_LOG_DIR" +[[ -n "${pruned:-}" ]] && echo " Pruned : $pruned run log(s) older than a day" start_time=$(date +%s) diff --git a/lib/lib-review-loop b/lib/lib-review-loop index a284e3f..94b49e0 100644 --- a/lib/lib-review-loop +++ b/lib/lib-review-loop @@ -449,11 +449,40 @@ claim_run_log_dir() { printf '%s\n' "$dir" return 0 fi - # Suffixed, so the name still starts with the run stamp. + # Suffixed, so it still starts with the stamp and prune_run_logs sees it. dir=$(mktemp -d "$root/$stamp-XXXXXX") || return 1 printf '%s\n' "$dir" } +# ---- run log retention ---------------------------------------------------- + +# Delete run directories under $1 older than $2 days. Prints how many. +# By age, not count: the loop is run repeatedly in a sitting, so ten runs in an +# afternoon should not push out yesterday's. Only matches the YYYYmmdd-HHMMSS +# directories the loops create, so anything kept alongside them survives, and +# -mtime +0 spares today's. +# $1: directory holding the run directories +# $2: delete runs older than this many days (default 1, or REVIEW_LOOP_LOG_DAYS) +prune_run_logs() { + local root="$1" days="${2:-${REVIEW_LOOP_LOG_DAYS:-1}}" + [[ -d "$root" ]] || return 0 + [[ "$days" =~ ^[0-9]+$ ]] && (( days >= 1 )) || return 0 + + local -a old=() + local d + while IFS= read -r d; do + [[ -n "$d" ]] && old+=("$d") + done < <(find "$root" -mindepth 1 -maxdepth 1 -type d \ + -name '[0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9]-[0-9][0-9][0-9][0-9][0-9][0-9]*' \ + -mtime "+$(( days - 1 ))" 2>/dev/null | sort) + + (( ${#old[@]} > 0 )) || return 0 + for d in "${old[@]}"; do + rm -rf "$d" + done + printf '%s\n' "${#old[@]}" +} + # ---- elapsed time -------------------------------------------------------- # Compute human-readable elapsed time from a start timestamp. diff --git a/test/lib-review-loop.bats b/test/lib-review-loop.bats index 13a4585..0f0987f 100644 --- a/test/lib-review-loop.bats +++ b/test/lib-review-loop.bats @@ -398,6 +398,97 @@ stub_agent() { # stub_agent [stderr-line] [ -f "$BATS_TEST_TMPDIR/deep/nested/dir/step.log" ] } +# ========================================================================= +# Run log retention +# ========================================================================= + +make_run_dir() { # make_run_dir + local dir="$1/$2" + mkdir -p "$dir" + echo "log" > "$dir/step.log" + if [ "$3" -gt 0 ]; then + # touch -A/-d differ across platforms; -t with a computed stamp is portable. + local stamp + stamp=$(date -v "-$3d" '+%Y%m%d%H%M' 2>/dev/null) \ + || stamp=$(date -d "$3 days ago" '+%Y%m%d%H%M') + touch -t "$stamp" "$dir" + fi +} + +@test "prune_run_logs also deletes a suffixed run directory" { + source_lib + local root="$BATS_TEST_TMPDIR/logs" + # A run that lost the claim race is named -XXXXXX and must not + # outlive the plain ones just because of its suffix. + make_run_dir "$root" 20260101-120000-a1b2c3 5 + make_run_dir "$root" 20260807-120000-d4e5f6 0 + run prune_run_logs "$root" + assert_output "1" + [ ! -d "$root/20260101-120000-a1b2c3" ] + [ -d "$root/20260807-120000-d4e5f6" ] +} + +@test "prune_run_logs deletes runs older than a day and keeps today's" { + source_lib + local root="$BATS_TEST_TMPDIR/logs" + make_run_dir "$root" 20260101-120000 5 + make_run_dir "$root" 20260102-120000 3 + make_run_dir "$root" 20260807-120000 0 + run prune_run_logs "$root" + assert_output "2" + [ ! -d "$root/20260101-120000" ] + [ ! -d "$root/20260102-120000" ] + [ -d "$root/20260807-120000" ] +} + +@test "prune_run_logs leaves everything when nothing is old enough" { + source_lib + local root="$BATS_TEST_TMPDIR/logs" + make_run_dir "$root" 20260807-120000 0 + make_run_dir "$root" 20260807-130000 0 + run prune_run_logs "$root" + assert_output "" + [ -d "$root/20260807-120000" ] + [ -d "$root/20260807-130000" ] +} + +@test "prune_run_logs ignores directories that are not run stamps" { + source_lib + local root="$BATS_TEST_TMPDIR/logs" + mkdir -p "$root/backups" "$root/notes" + echo keep > "$root/backups/original.sh" + touch -t 202001010000 "$root/backups" "$root/notes" + make_run_dir "$root" 20260101-120000 5 + run prune_run_logs "$root" + assert_output "1" + # Anything a user keeps alongside the run dirs is not ours to delete. + [ -f "$root/backups/original.sh" ] + [ -d "$root/notes" ] +} + +@test "prune_run_logs honours a custom retention in days" { + source_lib + local root="$BATS_TEST_TMPDIR/logs" + make_run_dir "$root" 20260101-120000 10 + make_run_dir "$root" 20260102-120000 3 + run prune_run_logs "$root" 7 + assert_output "1" + [ ! -d "$root/20260101-120000" ] + [ -d "$root/20260102-120000" ] +} + +@test "prune_run_logs is a no-op on a missing or invalid target" { + source_lib + run prune_run_logs "$BATS_TEST_TMPDIR/nope" + assert_success + assert_output "" + local root="$BATS_TEST_TMPDIR/logs" + make_run_dir "$root" 20260101-120000 5 + run prune_run_logs "$root" "not-a-number" + assert_output "" + [ -d "$root/20260101-120000" ] +} + @test "cleanup_agent_artifacts does not delete run logs inside the repo" { source_lib cd "$BATS_TEST_TMPDIR"