feat(review-loop): log agent output so a failed run can be diagnosed - #21
Open
rlorenzo wants to merge 5 commits into
Open
feat(review-loop): log agent output so a failed run can be diagnosed#21rlorenzo wants to merge 5 commits into
rlorenzo wants to merge 5 commits into
Conversation
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. Reading the code could not settle it either, because the evidence was gone. run_agent now mirrors the agent's combined output to AGENT_LOG when that is set, stamped with the agent, the tools it was given, and the exit code. The dispatch moves to _dispatch_agent so the wrapper stays one place rather than five. It returns PIPESTATUS[0] rather than the pipeline status: tee succeeds even when the agent does not, and the caller's `|| local_exit=$?` has to see the agent's code. code-review-loop sets a per-run directory under ~/.cache and names a log per step, printed in the banner and again on any agent failure. Deliberately outside both the temp dir, which the EXIT trap wipes and which takes the most interesting run with it, and the project, where stage_review_changes would offer the logs up for commit. That last part is also why staging now skips anything under the log directory: CODE_REVIEW_LOOP_LOG_DIR can point inside the repo, and without the skip the first run there staged its own logs. Five tests cover the wrapper with a stubbed agent, including that a failing agent's stderr reaches the log and that the exit code survives the pipe. Both were confirmed to fail against the pipeline status.
Greptile SummaryThe PR records each review agent’s combined output and exit status in persistent, per-run log directories.
|
| Filename | Overview |
|---|---|
| bin/code-review-loop | Integrates per-step logs, atomically claimed run directories, retention, failure-path log reporting, and resolved-path staging exclusion; the previously reported path and collision issues are fixed. |
| lib/lib-review-loop | Adds centralized output mirroring with correct agent exit propagation plus atomic run-directory claiming and suffix-aware pruning. |
| test/code-review-loop.bats | Exercises successful log production and non-staging for relative, trailing-slash, project-root, and shared-root configurations. |
| test/lib-review-loop.bats | Covers output capture, stderr capture, exit-code preservation, directory creation, concurrent uniqueness, and retention behavior. |
| README.md | Documents log locations, contents, retention, failure behavior, and configuration variables. |
Reviews (3): Last reviewed commit: "fix(review-loop): claim run directories ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
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 that is supposed to be safe to leave alone. Old runs are now pruned at startup. By age rather than by count, because the loop gets run several times in a sitting and what you come back for is today's failure: ten runs in an afternoon should not push out yesterday's, and a quiet week should not keep logs from last month. A day by default, REVIEW_LOOP_LOG_DAYS to change it. Two limits on what pruning will touch. 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. Also fixes a collision between the logs and Codex artifact cleanup. That sweep removes files that appeared during a reviewer run, and when the log directory is inside the repo, which CODE_REVIEW_LOOP_LOG_DIR permits, it was deleting the reviewer's own log seconds after it was written. A log is a record of the run, not something the agent made. The README now says where the logs are, what each one holds, and that an agent failing does not stop the loop, so the log is often the only sign a step went wrong. Environment variables are listed too.
Owner
Author
|
Pushed 8d25b89 covering two follow-ups. Retention. Logs outlived their run by design, which also meant nothing ever removed them: one directory per run, forever, in a cache directory. They are now pruned at startup by age rather than count, since the loop gets run several times in a sitting and what you come back for is today's failure. Ten runs in an afternoon should not push out yesterday's, and a quiet week should not keep last month's. One day by default, A bug the end-to-end test caught. Codex artifact cleanup sweeps files that appeared during a reviewer run, and when the log directory sits inside the repo, which A log is a record of the run, not something the agent made, so README now documents where the logs are, what each holds, and that an agent failing does not stop the loop, so the log is often the only sign a step went wrong. Environment variables are listed too. 64 tests pass. The six new ones cover retention and log preservation, and each was confirmed to fail against the previous behaviour. |
The exclusion that keeps run logs out of a review trimmed a string prefix off the project root, which only holds when the configured path is absolute, has no trailing slash, and is not the project root itself. A relative value like "mylogs", or a trailing slash, left the prefix unmatched and the logs were staged as though the agent had written them. It now compares resolved absolute paths, the same way cleanup_agent_artifacts already did, so a symlinked location works too. CODE_REVIEW_LOOP_LOG_DIR also named the run directory itself, so two runs sharing one value wrote the same step filenames and appended into each other. It now names the directory runs go under, matching how the default already behaved, and each run still gets its own timestamp. A second run inside the same second takes a pid suffix rather than adopting the first run's directory. That also covers the case where the configured path is the project root, since the run directory is then a level below it. The four new tests were written twice. The first versions asserted only that no logs were staged, which passed without the loop ever starting: the suite sandboxes HOME, so the installed prompts were unreachable and the run exited at validate_prompts. They now assert logs were produced before asserting none were staged, and read prompts from the checkout rather than from whatever ./setup left on the machine.
Owner
Author
|
@greptileai review |
…ly test failure Two loops starting in the same second both saw the timestamped name free and both created it, then appended their steps into one set of files. The name is now claimed by creating it: plain mkdir is atomic, so exactly one caller wins and the loser falls back to mktemp, which is atomic too. A pid suffix was the first attempt and was not enough. In a subshell $$ is still the parent's, so twenty concurrent callers picked two names between them, which the test caught. prune_run_logs also had to learn that a run directory can carry a suffix, or a collided run would never have been cleaned up. The claim moves to the library as claim_run_log_dir. It is a few lines of process coordination worth testing on its own, and driving four whole loops at one git index to reach it tested the index lock more than the claim. Also fixes the four log-staging tests, which passed here and failed in CI. They took the reviewer from ~/.ai-coding-setup.conf or the built-in default, so they depended on the machine having codex installed; CI does not, and the loop exited at validate_tools before writing a log. They now name both agents explicitly. Verified against a shell with the config hidden and the real agents off PATH.
Owner
Author
|
@greptileai review |
The "is this file inside that directory" check existed twice, once in stage_review_changes and once in cleanup_agent_artifacts, both resolving paths by hand. It is one idea, so it is now one function, is_inside_dir, with its own tests for the relative, trailing-slash and symlinked cases that motivated it. The comments were also carrying the history of how each fix was reached, which belongs in the commit log rather than in the source. Trimmed to the reason a reader needs: bin was at 37% comment lines against 19% for the rest of the file and had 9-line blocks where nothing else exceeds 4. Both files now sit at or under the surrounding density.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
A review loop that dies leaves nothing behind. Agent output went to the terminal and
nowhere else, so when a run stopped mid-refinement all it showed was:
No way to tell a crash from a timeout from an external kill, and no way to recover it
afterwards. A run started in the background does not even have the scrollback.
The change
run_agentmirrors the agent's combined output toAGENT_LOGwhen set, stamped with theagent, the tools it was given, and the exit code. Dispatch moves to
_dispatch_agentsothe wrapper lives in one place rather than being repeated per runner.
It returns
PIPESTATUS[0], not the pipeline status.teesucceeds even when the agentdoes not, and the caller's
|| local_exit=$?has to see the agent's real code, otherwiseevery failure silently reads as success.
code-review-loopcreates a per-run directory and names a log per step:printed in the banner and again on any agent failure. It sits outside two places on
purpose:
TMPDIR_REVIEW, which theEXITtrap wipes, taking the most interesting run with itstage_review_changeswould offer the logs up for commitThat second point is also why staging now skips anything under the log directory.
CODE_REVIEW_LOOP_LOG_DIRcan point inside a repo, and without the skip the first runthere staged its own logs. That was caught by testing the change rather than by reading it.
Tests
Five tests in
test/lib-review-loop.batscover the wrapper with a stubbed agent onPATH:AGENT_LOGis unsettee'sThe two that matter were confirmed to fail against
rc=$?before the fix, so they arenot vacuous:
58 tests pass, shellcheck clean,
pre-commit rungreen.Not included
No timeout.
claudehas no timeout flag andgtimeoutis not present on macOS bydefault, so anything here would have been invented.
run_antigravityalready carries--print-timeout 30m; the others do not.Worth noting the original failure turned out not to be a bug in these scripts. The
tolerated-failure path works: with a stubbed agent that exits 1, the loop logs the warning
and continues through every remaining step. The run had been killed externally. The defect
was that this was not knowable, which is what this PR fixes.