fix(gemini): judge and dispatch against the same root (#66's other half) - #82
Merged
Conversation
The command file documented this gap rather than closing it: "gemini remains exposed to the same check-cwd-vs-dispatch-cwd drift this fixed for codex". gemini-cli's workspace is the cwd of the process at LAUNCH, and it refuses to read or write outside it. Two things followed from never pinning that: THE CHECK JUDGED BY ITS OWN CWD. The basis was `repo_root()` — the git toplevel of the CHECK invocation — on the stated reasoning that gemini "runs as a shell command in the CURRENT cwd" so that was the right basis. The premise holds only if check and dispatch stand in the same place, and nothing enforced it. The arm was therefore wrong in BOTH directions, and the suite now asserts both: it MISSED a copy outside the root dispatch would inherit (the #66 failure), and it FALSELY hinted on a copy that was inside that root but outside whatever repo the check happened to be standing in. The second was the surprise — a spurious hint teaches the reader to ignore the real one. THE DISPATCH PINNED NOTHING. The shell branch set no directory at all, so the workspace was whatever cwd that Bash call inherited. Now `cd "<session-root>"` inside a subshell, scoped to the dispatch, leaving the primary's own cwd alone. Both halves are needed: pinning only the check would validate a root the dispatch does not use, which is exactly the shape of #66. `fable` needs no basis — it runs in-harness with no workspace of its own — so `--session-root` is now consumed by both arms that have one, and the scope note saying otherwise is removed. A stale claim like that is how the gap survived: written down instead of fixed. Guards: three entries — the check's basis, the dispatch's cwd, and the scope claim itself, so the documentation cannot quietly revert to describing the bug. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Nm3hNXA4WHbgKjFbxX7VUE
CI caught what my local verification structurally could not. I verified the three NEW entries with `--only`, which cannot detect that an EXISTING entry went stale — and rewriting the gemini branch deleted the line `reviewer/check-doc-gemini-basis` targets, so the sweep failed with "the target line is not present (stale mutation table?)". The entry is superseded rather than re-pointed. It asserted the gemini arm must judge against `repo_root()` and not codex's sandbox — half right, and the wrong half is exactly the bug this PR fixes: the basis is neither, it is the SESSION ROOT the dispatch will inherit. Re-pointing it would re-assert the behaviour just removed. The property it protected still holds and is still covered: the replacement entry pins the basis to `$session_root` with `$rr` as fallback, and neither is `codex_workspace_root`. Lesson worth recording: `--only` is enough to prove a NEW guard bites, but only the full sweep proves the table is still consistent with the code. Any change that edits a line another entry targets needs the sweep, not the subset. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Nm3hNXA4WHbgKjFbxX7VUE
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.
TL;DR: Gemini reviews could quietly land in the wrong directory — the tool checked one folder and then launched the reviewer in another. This closes the half of #66 that was fixed for codex but only documented for gemini.
Completes #66.
The gap the file described instead of closing
commands/multi-review.mdsaid, in writing:gemini-cli's workspace is the cwd of the process at launch, and it refuses to read or write outside it. Two things followed from never pinning that.
1. The check judged by its own cwd
The basis was
repo_root()— the git toplevel of the check invocation — on the stated reasoning that gemini "runs as a shell command in the CURRENT cwd", so that was correct. The premise holds only if check and dispatch stand in the same place, and nothing enforced that.So the arm was wrong in both directions, and the suite now asserts both:
The second was the surprise. A spurious hint is not harmless here — it's how the real one gets ignored, which is precisely the #22 lesson about a warning that fires when it shouldn't.
2. The dispatch pinned nothing at all
The shell branch set no directory, so gemini's workspace was whatever cwd that Bash call happened to inherit:
The subshell scopes it to the dispatch and leaves the primary's own cwd alone.
Both halves are needed. Pinning only the check would validate a root the dispatch doesn't use — which is the exact shape of #66, just relocated.
fableneeds no basis (in-harness, no workspace of its own), so--session-rootis now consumed by both arms that have one, and the scope note claiming otherwise is gone.Guards
reviewer/gemini-session-root-basis$rrcommand/gemini-dispatch-cwd-pinnedcdcommand/session-root-scope-claimAll three verified caught. The third is deliberate: a stale claim is how this gap survived — it was written down instead of fixed — so the sentence itself is now guarded.
Verification
scripts/*.test.sh(15 suites)multi-review-version-check.sh1.22.0 → 1.22.1 (ok)New assertions: hint fires when the copy is outside the dispatch root; stays advisory (exit 0); stays silent when the copy is inside that root even though the check runs from elsewhere.
Not run locally:
shellcheck(not installed here); CI covers it plus the bash 3.2 leg and the full sweep.Flagged, not fixed
The empty-argv branch two lines above still reads
: quarantine <id> "…"—:is the null builtin, so a primary transcribing it literally does nothing and the failure surfaces later as a wait-bound timeout under the wrong reason. I rewrote the lines around it and deliberately left it alone per §1.3; it wants its own change.🤖 Generated with Claude Code
https://claude.ai/code/session_01Nm3hNXA4WHbgKjFbxX7VUE