fix(review-pr): make Console-mode merge-base detection cross-shell compatible#162
Conversation
…detection Assisted-By: docker-agent
![docker-agent[bot]](https://avatars.githubusercontent.com/in/2787757?s=60&v=4){kind=small}
| step 4) AND echoes it so you can see what was captured. The fallback order | ||
| prefers `upstream` (forks) over `origin`: | ||
| prompt says. Try each candidate ref in its own `shell` tool call, stopping at | ||
| the first that returns exit 0 with non-empty output. Candidates in order: |
There was a problem hiding this comment.
[LIKELY LOW] No explicit instruction to continue when a git merge-base candidate returns non-zero exit
The instruction reads "stopping at the first that returns exit 0 with non-empty output" — describing the success condition but not the failure handling. The phrasing implicitly means "skip those that don't return exit 0", but it does not explicitly say "if a candidate fails (non-zero exit), that is expected — continue to the next one."
An LLM agent may interpret a non-zero exit from the shell tool as a fatal error and abort rather than advancing to the next candidate ref. Making the intent explicit would reduce ambiguity, e.g.:
"If the call returns a non-zero exit code or empty output, that ref does not exist — continue to the next candidate. Non-zero exit from
git merge-baseis expected and normal for missing refs."
There was a problem hiding this comment.
Clarified in dea7290. Added an explicit sentence after the cross-shell warning: "If a git merge-base call returns a non-zero exit code or empty output, that ref does not exist in this clone — that is expected; continue to the next candidate. Only the seventh candidate failing should be treated as an error."
There was a problem hiding this comment.
Perfect! The added instruction makes the failure handling explicit and removes ambiguity. The clarification that "only the seventh candidate failing should be treated as an error" also gives the agent a clear decision point. Thank you for addressing this!
…lity - Edit 1: replace bash-only `ls AGENTS.md CLAUDE.md 2>/dev/null` in step 2 with prose directing the agent to use `read_file` for AGENTS.md then CLAUDE.md, with no shell redirection - Edit 2: replace shell redirect `> ./pr-review.diff` with `git diff --output=./pr-review.diff` to avoid PowerShell 5.1 UTF-16 LE BOM encoding corruption; update step 4 sub-bullet accordingly - Edit 3: add explicit guidance that a non-zero exit code from `git merge-base` is expected and means skip to the next candidate; only the seventh failure is an error Assisted-By: docker-agent
|
/review |
|
|
||
| If all seven candidates fail, issue a plain `echo` explaining that no known | ||
| remote or local branch was found, then stop calling tools. | ||
| If `BASE` equals the output of `git rev-parse HEAD`, issue a plain `echo` |
There was a problem hiding this comment.
[MEDIUM] Missing instruction to run git rev-parse HEAD before the BASE == HEAD guard
The instruction tells the agent:
"If
BASEequals the output ofgit rev-parse HEAD, issue a plainecho… then stop calling tools."
But the agent has no way to perform this comparison without first obtaining HEAD's SHA via an explicit shell tool call (git rev-parse HEAD). That call is never instructed. Since every shell invocation is a fresh shell (stated explicitly in the file), the agent cannot carry BASE across calls — it must compare two values in its LLM reasoning, which means it needs the HEAD SHA explicitly surfaced.
Without the instruction, the agent is left with two bad options:
- Skip the guard silently, proceeding with a potentially empty diff.
- Attempt
[ "$BASE" = "$(git rev-parse HEAD)" ]— exactly the bash-only syntax this PR is trying to eliminate.
Suggested fix: Add an explicit step after the successful git merge-base call:
git rev-parse HEAD
Then tell the agent: "If this SHA matches BASE, issue a plain echo explaining the diff would be empty and stop."
There was a problem hiding this comment.
Fixed in e3e5e71. Added an explicit instruction after a git merge-base candidate succeeds: the agent now runs git rev-parse HEAD as a separate shell call, then compares it to BASE in its reasoning (not in shell). If equal, it echos a clear message ("you're on the default branch; check out a feature branch first") and stops. No bash-only test syntax involved.
…EAD guard Assisted-By: docker-agent
|
/review |
| Otherwise, proceed to the diff capture by running these two commands, each as | ||
| its own `shell` call: | ||
| ``` | ||
| git diff <BASE> HEAD --output=./pr-review.diff |
There was a problem hiding this comment.
[MEDIUM] git diff --output=<file> requires Git ≥ 2.25; older environments silently produce no diff file
The --output=<file> flag on git diff was introduced in Git 2.25 (January 2020). Several common environments ship older versions (e.g., Ubuntu 18.04 includes Git 2.17; older macOS Xcode CLT builds are also affected). If --output is not recognised, Git may silently ignore the flag or fail without writing ./pr-review.diff, while git diff --stat (the next step) succeeds anyway — so the agent sees a stat summary but delegates a non-existent file path to the drafter.
The cross-shell concern that motivated this choice does not actually block the simpler alternative: output redirection with > works in both bash and PowerShell 5.1, so:
git diff <BASE> HEAD > ./pr-review.diff
is both cross-shell compatible and universally supported across all Git versions.
Fix: Replace git diff <BASE> HEAD --output=./pr-review.diff with git diff <BASE> HEAD > ./pr-review.diff.
There was a problem hiding this comment.
Respectfully disagree on both points — keeping --output.
On the Git version requirement: I checked upstream sources and git diff --output=<file> has been parsed in diff.c since at least v2.0.0 (2014):
$ curl -sS https://raw.githubusercontent.com/git/git/v2.0.0/diff.c | grep -A2 '"output"'
else if ((argcount = parse_long_opt("output", av, &optarg))) {
options->file = fopen(optarg, "w");
It's also present in v2.10.0, v2.17.0, v2.20.0 — well before 2.25. Ubuntu 18.04's Git 2.17 supports it fine. The 2.25 release notes don't mention any change to this option. I think this finding is based on a misattribution.
On > being cross-shell safe: This contradicts the previous review (#162 (comment)) which we accepted as valid. PowerShell 5.1's > operator is a shorthand for Out-File, which defaults to Unicode (UTF-16 LE) with BOM — confirmed in Microsoft's docs. On a Windows host running pr-review.yaml in Console mode, git diff … > ./pr-review.diff would write a UTF-16-encoded diff that the drafter sub-agent's read_file would then mis-parse. That's the bug the switch to --output was meant to prevent.
Since --output (a) is universally available in the Git versions anyone runs this agent against, and (b) bypasses the host shell entirely so encoding can't be re-mangled, it's strictly safer than > here. Happy to revisit if you have a counter-example for the version concern.
There was a problem hiding this comment.
You're absolutely right on both counts — I apologize for the error.
I misattributed the Git 2.25 date based on faulty information. Your research shows --output has been in diff.c since at least Git 2.0.0 (2014), making it universally available in any environment that would run this agent. My claim about Ubuntu 18.04 compatibility was wrong.
The PowerShell 5.1 encoding issue is also spot-on. I should have read the earlier review thread more carefully before suggesting > as an alternative — the UTF-16 LE default encoding in PowerShell's Out-File is exactly the kind of subtle corruption that --output prevents by bypassing the shell entirely. Thank you for the detailed explanation and Microsoft docs reference.
Keeping --output is the correct choice here.
Assisted-By: docker-agent
|
/review |
|
⏭️ Review skipped — another review for this PR is already in progress (started 518s ago). Only one review runs at a time to avoid duplicate comments. The in-progress review will post its results when complete. |
|
/review |
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The cross-shell compatibility fix is well-structured and correctly removes all bash-only constructs from Console-mode step 1. The --output= flag is used instead of shell > redirection (avoiding PowerShell's UTF-16 LE encoding issue), step 4b is updated to match, the safety clause is correctly scoped to Console mode only, and all 7 candidate refs are listed in the right order. No real bugs found in the changed code.
2 participants
Problem
The Console-mode merge-base detection in
review-pr/agents/pr-review.yaml(step 1) used a bash$(...||...)chain with2>/dev/null,[ -z ... ]tests,exit 1, andtee. These constructs are bash-only. On Windows, cagent'sshelltool routes through PowerShell 5.1, which cannot parse any of them, causing local runs to fail immediately.This regression was introduced in PR #150.
Fixes #155
Changes
review-pr/agents/pr-review.yaml(1 file, 35 insertions / 28 deletions):Added a cross-shell safety clause to the root agent's
## Processpreamble (before step 0). It instructs the agent to keep everyshelltool call to a single simple command and avoid||,&&,2>/dev/null,[ ... ]tests, and heredocs.Replaced step 1's Console-mode bash chain with prose instructions that:
upstream/main,upstream/master,origin/HEAD,origin/main,origin/master,main,master) in its ownshelltool call, stopping at the first that returns exit 0 with non-empty output.git merge-base <ref> HEAD— no chaining, no2>/dev/null.git diff <BASE> HEAD > ./pr-review.diffthengit diff --stat <BASE> HEAD(each as its ownshellcall).BASEequals HEAD): issue a plainechoand stop — no[ -z ... ]test, noexit 1.Updated step 4's Console output mode sub-bullet to say the diff was written via
>instead oftee.Test Plan
docker agent runon macOS/Linux from a fork repo — confirm the agent correctly issues per-candidategit merge-basecalls and produces a diff file.docker agent runon Windows (PowerShell 5.1) — confirm the agent no longer fails to parse shell syntax and successfully detects the merge base.upstream, noorigin): confirm the agent echoes an error message and stops gracefully.origin/masterexists: confirm the agent tries candidates in order and stops at the correct one.