perf(prompt): deliver the contract to fable by path, not inlined - #83
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
Inlining exists for ONE reason (issue #22): gemini-cli refuses any read outside its workspace root, and the protocol lives under the plugin cache — always outside the reviewed repo. That constraint belongs to dispatch-kind `shell`. It has never applied to `fable`, which runs IN-HARNESS as a subagent and can Read an absolute path anywhere. Applying gemini's sandbox to fable cost 14 KB on the most-dispatched prompt in the system: fable is the guaranteed floor reviewer, so it is the one provider present in every round of every run. fable 17,008 B -> 3,056 B (82% of it was the inlined contract) gemini 17,008 B unchanged (the #22 constraint is real) codex 2,702 B unchanged (byte-frozen golden still matches) prompt_head now has THREE shapes — skill / inline / path — keyed on dispatch-kind rather than a provider id, so a future shell-kind provider inherits the right behaviour without touching this. WHY THIS IS SOUND AND NOT MERELY CHEAPER: the prompt tail states the operative grammar in full regardless of shape, so a fable turn that never opens the file still emits findings `merge` can read. The file adds marker/scope detail, not the enforceable shape. The suite asserts each grammar token — the finding form, [no-findings], and the via/risk/evidence lines — survives WITHOUT the inline, and the prompt now tells the reviewer to stop and say so if the read fails rather than guess. Deliberate test change (§4): the inline assertions ran over `fable gemini` and pinned "no filesystem path may stand in for it". That encoded gemini's constraint as a universal, which is the bug. The loop is narrowed to gemini — where it is still exactly right and still asserted — and fable gets its own block asserting the path shape plus the grammar-survives-without-the-file property. Nothing was weakened: the gemini path keeps every assertion it had. Also drops the "Cost is ~7KB of prompt per dispatch" comment, which had drifted to 14 KB as the protocol grew. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Nm3hNXA4WHbgKjFbxX7VUE
Both branches edited reviewer.sh; git auto-merged them and BOTH behaviours were re-verified rather than assumed — G2's session-root basis for the gemini check, and this branch's three-shape contract delivery. The table conflict needed care in one direction the usual keep-both does not cover: this branch predates #82's removal of the now-stale reviewer/check-doc-gemini-basis entry, so a blind keep-both would have RESURRECTED it. Confirmed absent (0 occurrences) and both live entries present. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Nm3hNXA4WHbgKjFbxX7VUE
Narrowing the inline assertions from `fable gemini` to `gemini` left a one-element loop, which shellcheck flags as probably-a-mistake. It is not: the loop is over a POPULATION — every provider whose contract must be inlined — which has exactly one member today because `shell` is the only dispatch-kind that cannot be sent to a path. Kept as a loop so a second shell-kind provider joins by being added to the list and inherits every assertion, with the reason recorded next to the disable. Caught by CI, not locally: shellcheck is not installed on this machine, so the gate's shellcheck step is the only place this could surface. 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: The reviewer that runs on every round of every review was being handed a 17 KB prompt, 82% of which was a copy of the protocol it can simply open itself. That copy exists to work around a limitation only the Gemini CLI has. Fable now gets a file path instead — 17,008 B → 3,056 B.
Why the contract was inlined at all
Exactly one reason, from #22: gemini-cli refuses any read outside its workspace root, the protocol lives under the plugin cache (always outside the reviewed repo), and the in-repo copy is git-ignored twice over by
ensure-skill— which gemini-cli also honors. Inlining removes the filesystem from the path entirely.That constraint belongs to dispatch-kind
shell. It has never applied tofable, which runs in-harness as a subagent and can Read an absolute path anywhere. Applying gemini's sandbox to fable was a generalization nobody checked.And it lands on the worst possible provider: fable is the guaranteed floor reviewer, so it is the one present in every round of every run.
Keyed on dispatch-kind, not provider id
prompt_headnow has three shapes — skill / inline / path — selected bykind.shellmeans "an external CLI with its own workspace sandbox", which is precisely the population that cannot be sent to a path. A future shell-kind provider inherits the right behaviour without touching this.Why this is sound, not merely cheaper
The prompt tail states the operative grammar in full regardless of shape. A fable turn that never opens the file still emits findings
mergecan read — the file adds marker/scope detail, not the enforceable shape.The suite asserts that explicitly: each grammar token (
[finding:<id>|<sev>],[no-findings],— via,— risk:,— evidence:) is present without the inline. And the prompt now tells the reviewer to stop and say so if it cannot read the file, rather than guess.So the failure mode of a missed read is a slightly less informed reviewer, not an unparseable turn.
Deliberate test change (§4)
The inline assertions ran over
fable geminiand pinned "no filesystem path may stand in for it". That encoded gemini's constraint as a universal — the bug itself, written as a test. The loop is narrowed togemini, where it remains exactly right and fully asserted; fable gets its own block asserting the path shape plus the grammar-survives-without-the-file property.Nothing was weakened — gemini keeps every assertion it had, including the two that prove the inline is complete and not truncated.
Also drops the
Cost is ~7KB of prompt per dispatchcomment, which had drifted to 14 KB as the protocol grew.Verification
scripts/*.test.sh(15 suites)reviewer/fable-contract-by-pathmutationmulti-review-version-check.sh1.22.1 → 1.22.2 (ok)This branch and #82 both edited
reviewer.shand git auto-merged them; both behaviours were re-verified rather than assumed. The table conflict also needed care in a direction the usual keep-both doesn't cover — this branch predates #82's removal of a stale entry, so a blind keep-both would have resurrected it. Confirmed absent.Not run locally:
shellcheck; CI covers it plus the bash 3.2 leg and the full sweep.🤖 Generated with Claude Code
https://claude.ai/code/session_01Nm3hNXA4WHbgKjFbxX7VUE