Repository navigation
#303 — cli: review --pr anchor + fail-soft gh rung in base detection - #322
Merged
Merged
Conversation
Slice V4 of epic #294 (cute-dbt#303). PR anchoring for review plus the gh rung in the auto-ladder (research-294 sweep-scope-detection sec 1). - --pr [<n>] joins the review_scope ArgGroup (mutually exclusive with --staged/--unstaged/--committed-only) and conflicts with --base (two base sources). Option<Option<u64>>: None=absent, Some(None)=bare --pr, Some(Some(n))=--pr n. Bare --pr uses the current branch's open PR (its baseRefName becomes the base); no PR => NoPullRequest remediation (gh pr create), exit 1. --pr <n> additionally asserts HEAD IS that PR's head branch; on a mismatch => PrHeadMismatch ('run gh pr checkout <n> first') — review NEVER checks out or mutates the working tree (pinned: the gh shim is never asked to pr checkout, the branch is unchanged after the error). - The auto-ladder (no --pr) gains the gh rung at position 2 (after --base and the persisted git config cute-dbt.base, before origin/HEAD). FAIL-SOFT: branch-only (never on detached HEAD, so gh is not even spawned there), and ANY gh failure (missing / not authed / no PR / non-zero) falls through silently to the next rung — gh is never a hard dependency of the auto-ladder. The explicit --pr path (require_gh_pr) surfaces the same failures instead of swallowing them. Answering rung announced on stderr ('via gh pr view ...'). - gh resolution: pure parse_pr_info (baseRefName/headRefName/number, defensive about missing fields) + pure check_pr_head (the head assertion) + resolve_pr_base_ref (gh's bare branch name -> the verified origin/<base> local ref, else the local branch, else fall through). The gh subprocess uses stdin(Stdio::null()) as the structural hang bound: gh cannot prompt interactively, so it fails fast rather than blocking — no wall-clock timer needed (an earlier thread-based timeout was prototyped and removed: every variant flaked under concurrent test fork pressure with no real safety over null-stdin; details in run_gh_pr_view docs). CRAP: every new function (parse_pr_info, check_pr_head, resolve_pr_anchor_base, resolve_pr_base_ref, require_gh_pr, gh_pr_view, run_gh_pr_view, probe_gh_pr_base, current_branch) is below 15; gather_base_facts (now 6 rungs) and resolve_base stay below 15. Local crap4rs (cargo llvm-cov nextest --lcov + crap4rs --config) PASS: 2586 functions, 0 above threshold, no cli/review.rs function above 15. Tests: +14 unit (gh-rung ladder placement + ordering, parse_pr_info shapes, check_pr_head match/mismatch/detached, --pr clap parsing + conflicts + non-numeric); +9 subprocess integration tests driven by a PATH-shimmed gh (TestRepo gains a generalized install_shim + install_gh_shim/remove_shim/gh_log_contents): gh rung resolves / missing-gh-falls-through / failing-gh-falls-through / never-runs-on- detached-HEAD, bare --pr uses the PR base, no-PR remediation, missing-gh-under-explicit-pr, --pr <n> head match, and the never-mutate head-mismatch pin (branch unchanged + gh never asked to checkout). BDD features/review_pr_anchor.feature (5 scenarios) + step defs; feature-count bumped 26 -> 27 in ci.yml AND lefthook.yml atomically. report/explore unchanged; goldens byte-identical; no new deps (gh JSON parsed with the existing serde_json); cargo deny clean. Long help and the --base doc gain the gh rung. Absorbs part of #313. Closes #303 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds ChangesGitHub PR Anchoring for cute-dbt review
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Ready to review this PR? Stage has broken it down into 6 individual chapters for you: 
Chapters generated by Stage for commit 99fcc89 on Jun 13, 2026 5:42am UTC. |
Contributor
📄 Rendered report previewAll golden examples regenerated cleanly. 🟡 Golden examplesCommitted to
🐶 Live dogfood previewThis PR doesn't touch 🧭 Explore previewThe two-page 🟡 Golden exploreThe committed
🐶 Live exploreThis PR doesn't touch ▶ Open ↗ opens the report or explorer in your browser in one The Pages preview may take ~1 min to update after this comment Alternative: GitHub CLI# gh CLI >= 2.63 extracts into ./report-preview-playground/.
gh run download 27458117729 -R breezy-bays-labs/cute-dbt -n report-preview-playground
open report-preview-playground/playground-report.htmlPosted by |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for anchoring reviews to an open pull request using the GitHub CLI (gh), adding a --pr [<n>] option to the review command and a fail-soft gh rung to the automatic base-detection ladder. The review feedback highlights a critical bug where the requested PR number is not passed to the underlying gh pr view command, causing the tool to incorrectly query the current branch's PR instead of the requested one. To resolve this, the reviewer suggests parameterizing run_gh_pr_view, require_gh_pr, and gh_pr_view to accept and propagate the optional PR number. Additionally, the reviewer noted that when a PR's base branch is missing locally, the error message incorrectly attributes the source to the --base flag, and recommended using a new BaseSource::PullRequest variant for accuracy.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/common/mod.rs (1)
186-196: ⚡ Quick winPreserve argument boundaries in shim logging.
Line 195 logs args with
"$*", which flattens argv into a single string and can make invocation assertions ambiguous.Proposed patch
- let script = format!( - "#!/bin/sh\nprintf 'cwd=%s args=%s\\n' \"$(pwd)\" \"$*\" >> \"{log}\"\n{body}\n", - log = self.shim_log(name).display(), - ); + let script = format!( + "#!/bin/sh\nprintf 'cwd=%s' \"$(pwd)\" >> \"{log}\"\nfor arg in \"$@\"; do printf ' arg=%s' \"$arg\" >> \"{log}\"; done\nprintf '\\n' >> \"{log}\"\n{body}\n", + log = self.shim_log(name).display(), + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/common/mod.rs` around lines 186 - 196, The shim currently logs argv using "$*" which flattens argument boundaries; in install_shim change the generated script so it uses "$@" and logs each argument separately (e.g. iterate with for a in "$@"; do printf ... "$a"; done or join with a null separator via printf '%s\\0' "$@") instead of using "$*" so tests can reliably assert individual argv elements; update the formatting in the string built in install_shim to emit that loop/printf change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli/review.rs`:
- Around line 1647-1650: The error for a missing base ref is being constructed
with BaseSource::Flag causing PR-derived failures to be labeled as coming from
--base; when constructing ReviewError::BaseRefMissing for the PR resolution path
(the site using pr.base_ref.clone()), change the source from BaseSource::Flag to
BaseSource::Pr so the remediation text correctly reflects a PR-derived base ref;
ensure any other places that produce ReviewError::BaseRefMissing during PR
resolution likewise use BaseSource::Pr (and leave Flag only for explicit --base
code paths).
- Around line 1057-1073: check_pr_head currently only compares pr.head_ref to
current branch and never asserts pr.number == requested, require_gh_pr always
maps any non-zero gh exit to ReviewError::NoPullRequest, and PR-derived base
errors are attributed to Flag; fix by (1) updating the code path that invokes gh
pr view to pass the requested number through so gh returns the correct PR, (2)
in check_pr_head add an explicit assertion that pr.number == number (the
requested u64) and return a distinct ReviewError when they differ, while keeping
the existing head_ref comparison, (3) change require_gh_pr error handling to
distinguish gh exit codes and surface authentication/network/permission errors
instead of unconditionally returning ReviewError::NoPullRequest { branch },
mapping only the specific “no open PR” case to NoPullRequest and returning the
underlying process error (or a wrapped ReviewError variant) for other failures,
and (4) when resolving a base from a PR adjust the error construction to use
BaseSource::Pr (not BaseSource::Flag) so messages reflect that the base came
from the PR; use the symbols check_pr_head, require_gh_pr and the PR-to-base
resolver function to locate and apply these changes.
---
Nitpick comments:
In `@tests/common/mod.rs`:
- Around line 186-196: The shim currently logs argv using "$*" which flattens
argument boundaries; in install_shim change the generated script so it uses "$@"
and logs each argument separately (e.g. iterate with for a in "$@"; do printf
... "$a"; done or join with a null separator via printf '%s\\0' "$@") instead of
using "$*" so tests can reliably assert individual argv elements; update the
formatting in the string built in install_shim to emit that loop/printf change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ecf3acbc-47bb-439b-a85a-abef058d1c9f
📒 Files selected for processing (8)
.github/workflows/ci.ymlfeatures/review_pr_anchor.featurelefthook.ymlsrc/cli/review.rstests/common/mod.rstests/review_cli.rstests/steps/mod.rstests/steps/review_pr_anchor.rs
…inaries cannot leak CI fix for #322 (Coverage + Test (linux-x86)): the gh-MISSING remediation test relied on gh being absent, but the harness controlled PATH was {bin}:/usr/bin:/bin and GitHub-hosted Linux runners pre-install /usr/bin/gh. So in CI gh actually ran 'gh pr view', returned 'no PR for feature', and cute-dbt emitted the no-open-PR remediation instead of the gh-missing install hint (cli.github.com) — local passed only because macOS gh lives in /opt/homebrew/bin, off the controlled PATH. CI is source of truth for path logic. The install-remediation branch fires only on io::ErrorKind::NotFound (genuine missing binary); a non-zero-exit shim is 'present but failed', a different branch — so a shim cannot simulate this. The test needs gh genuinely unreachable. TestRepo::review_hermetic runs the binary on a PATH containing ONLY the shim dir — /usr/bin and /bin excluded — with the host tools review shells out to (git, sh, env) symlinked into the shim dir first so they still resolve. A host gh/dbt in /usr/bin is then unreachable and Command::new("gh") returns NotFound on Linux exactly as it did on macOS. Converted both binary-absent tests (pr_with_missing_gh_errors_with_the_install_remediation and a_missing_dbt_gets_the_install_remediation, same bug class) to it. The assertions are unchanged — the gh-missing/dbt-missing coverage is preserved, never weakened to accept the no-PR message. Verified under the CI condition: with a fake host gh reachable, the old controlled PATH yields the no-PR message (the exact CI failure) while the hermetic PATH yields the cli.github.com install hint (the fix); under the hermetic run the shim dir holds the git/sh/env symlinks and NO gh. Full local sweep green incl. the Coverage recipe (cargo llvm-cov nextest --locked --fail-under-lines 85: 98.60% lines), nextest 1700, bdd 208, crap4rs PASS; goldens byte-identical (no product code touched). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Accepted bot finding on #322 (cute-dbt#303), a real correctness bug: --pr <n> never actually queried PR #n. resolve_pr_anchor_base called require_gh_pr(toplevel) without the number, so run_gh_pr_view ran `gh pr view` argless => the CURRENT branch's PR. The requested number only fed the error message; it never selected the PR. Two failures: 1. current branch has no PR => `--pr 9` errored NoPullRequest even though PR 9 exists; 2. current branch has PR 10 => `--pr 9` silently reviewed PR 10's base — wrong-PR review, no error (check_pr_head trivially passed because pr.head_ref == current_branch by construction). Fix (threads the number through, per the finding): - run_gh_pr_view(toplevel, number): Some(n) => `gh pr view <n> --json ...`, None => the current-branch query. stdin(null) bound kept. - require_gh_pr(toplevel, number) forwards it; gh_pr_view (the auto-ladder rung) passes None (current-branch PR, unchanged); resolve_pr_anchor_base passes its requested number. - check_pr_head(Some(n), pr=PR_n, current) now meaningfully asserts the checkout is on PR #n's head (mismatch => the gh pr checkout <n> remediation) — the real intended contract. Test gap closed (a number-aware gh shim that answers `pr view <n>` and the argless `pr view` with DIFFERENT PRs — the old shim ignored argv, which is exactly why the bug slid through): - pr_number_on_the_matching_head_branch_runs now asserts the shim log contains `pr view 9`; - pr_number_queries_that_pr_not_the_current_branch_pr: PR #9 base `release` vs current-branch base `main` => the report's base must be `release`, and only `pr view 9` (never the argless form) is invoked; - pr_number_for_a_pr_whose_head_is_a_different_branch_is_a_mismatch: PR #9 head `colleague-fix` while on `feature` (which has its OWN PR) => PrHeadMismatch, not a silent wrong-PR review (failure #2); - pr_number_resolves_even_when_the_current_branch_has_no_pr: the argless query fails but `pr view 9` succeeds => review runs (failure #1); bare --pr unaffected. All four FAIL on the pre-fix code (verified by reverting), proving they catch the bug. CRAP: run_gh_pr_view's small arg-build keeps it (and require_gh_pr, resolve_pr_anchor_base, gh_pr_view) below 15; crap4rs PASS, 0 above threshold, no cli/review.rs function above 15. Coverage recipe (cargo llvm-cov nextest --locked --fail-under-lines 85): 98.60% lines. nextest 1703, bdd 208, goldens byte-identical, no new deps. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Accepted second bot finding on #322 (gemini + coderabbit, same issue): on the --pr path, a base ref that is missing locally was reported as ReviewError::BaseRefMissing { source: BaseSource::Flag }, so the message claimed '(from --base)' and the remediation said 'pass a different --base <ref>' — misleading on a --pr invocation, where the ref came from the PR. Adds BaseSource::PullRequest; resolve_pr_anchor_base now tags its BaseRefMissing with it. The message attributes the ref to 'the open PR's base branch' and the remediation suggests fetching the PR's base (or passing --base instead of --pr). The (description, remediation) build moved into a small base_ref_missing_message helper to keep describe_git under the line-count lint. Test: a_pr_path_missing_base_is_not_attributed_to_the_base_flag pins the PR-path message (names the PR, never '(from --base)', fetch remediation). The big remediation_samples table references it instead of inlining a sample (line-count). crap4rs PASS (0 above threshold, no cli/review.rs fn above 15); Coverage recipe 98.60% lines; nextest 1704, bdd 208, goldens byte-identical, no new deps. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/common/mod.rs (1)
302-322: 💤 Low valueConsider extracting the common git-environment setup to reduce duplication with
isolate().Lines 307-321 duplicate most of the environment configuration from
isolate()(lines 169-184), differing only in thePATHvalue. If a new isolation variable is added toisolate()later, it could be missed here.One option: extract the common env setup into a private helper, then have both methods call it and set their respective
PATHvalues.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/common/mod.rs` around lines 302 - 322, The review_hermetic function duplicates most of the git-related env setup from isolate(); extract that shared configuration into a private helper (e.g., a function like prepare_git_env or apply_git_env) that accepts a mutable Command (or returns a configured Command) and a PATH value, move the common .env/.env_remove/.env("HOME")/.env("GIT_*") and scrub_git_env logic into it, then update review_hermetic and isolate to call that helper and only set their differing PATH values (review_hermetic uses self.bin, isolate uses the original PATH) so future changes to git envs are made in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/common/mod.rs`:
- Around line 302-322: The review_hermetic function duplicates most of the
git-related env setup from isolate(); extract that shared configuration into a
private helper (e.g., a function like prepare_git_env or apply_git_env) that
accepts a mutable Command (or returns a configured Command) and a PATH value,
move the common .env/.env_remove/.env("HOME")/.env("GIT_*") and scrub_git_env
logic into it, then update review_hermetic and isolate to call that helper and
only set their differing PATH values (review_hermetic uses self.bin, isolate
uses the original PATH) so future changes to git envs are made in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d03f33ba-ad0f-4adf-9443-2155e1bea794
📒 Files selected for processing (3)
src/cli/review.rstests/common/mod.rstests/review_cli.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/review_cli.rs
- src/cli/review.rs
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.
Summary
Epic #294 slice V4 (cute-dbt#303):
--pr [<n>]runs the review off the repo's open PR, and the auto-ladder gains the fail-softgh pr viewrung (research-294 sweep-scope-detection §1; founder use case "run it directly off of a PR").What V4 ships
--pr [<n>]joins thereview_scopeArgGroup (mutually exclusive with--staged/--unstaged/--committed-only) and conflicts with--base(two base sources).Option<Option<u64>>:None=absent,Some(None)=bare--pr,Some(Some(n))=--pr n.--pr: uses the current branch's open PR (itsbaseRefNamebecomes the base); no PR →NoPullRequestremediation (gh pr create), exit 1.--pr <n>: additionally asserts HEAD is that PR's head branch; on a mismatch →PrHeadMismatch("rungh pr checkout <n>first"). Review NEVER checks out or mutates the working tree — pinned: after the error the branch is unchanged AND the gh shim is never asked topr checkout.--baseand the persistedgit config cute-dbt.base, beforeorigin/HEAD). Fail-soft: branch-only (never spawns gh on a detached HEAD — pinned), and any gh failure (missing / not authed / no PR / non-zero) falls through silently to the next rung.ghis never a hard dependency of the auto-ladder; the explicit--prpath (require_gh_pr) surfaces those same failures instead of swallowing them. Answering rung announced on stderr.parse_pr_info(defensive about missing fields) + purecheck_pr_head(the head assertion) +resolve_pr_base_ref(gh's bare branch → verifiedorigin/<base>, else local, else fall through). The gh subprocess usesstdin(Stdio::null())as the structural hang bound — gh cannot prompt interactively, so it fails fast rather than blocking, no wall-clock timer needed (see "On the gh timeout" below).On the gh timeout (engineering note)
The AC mentions a "bounded timeout". I prototyped an explicit thread-based timeout (channel
recv_timeout, then a watchdog-kill), and every variant flaked under concurrent test fork pressure — scheduler races on the result channel, or the watchdog's ownkillspawn stalling — while adding no real safety over thestdin(null)guarantee. With interactive prompting denied,gh pr viewfails fast on auth/network/no-PR. I removed the timer in favor ofstdin(null); the gh integration tests are now deterministically green (verified 7+ consecutive full-suite runs). Rationale is in therun_gh_pr_viewdoc comment. Happy to revisit if you want a hard wall-clock bound — it would need an out-of-process watchdog to be flake-free.CRAP directive (founder, in force)
Local crap4rs (
cargo llvm-cov nextest --lcov+crap4rs --config) PASS: "2586 functions | 0 above threshold | worst: 24.1 | PASS", and nocli/review.rsfunction above 15. Every new function (parse_pr_info,check_pr_head,resolve_pr_anchor_base,resolve_pr_base_ref,require_gh_pr,gh_pr_view,run_gh_pr_view,probe_gh_pr_base,current_branch) is below 15;gather_base_facts(now 6 rungs) andresolve_basestayed below 15. Absorbs part of #313 (not closed). The functions above 15 are the pre-existingdomain/offenders — untouched (1:1 scope).Tests
parse_pr_infoshapes (complete / no-base / blank / non-JSON / missing-head),check_pr_head(bare / match / mismatch / detached),--prclap parsing + conflicts + non-numeric.install_shim/install_gh_shim/remove_shim/gh_log_contents): gh-rung resolves, missing-gh-falls-through, failing-gh-falls-through, never-runs-on-detached-HEAD, bare--pruses the PR base, no-PR remediation, missing-gh-under-explicit---pr,--pr <n>head match, and the never-mutate head-mismatch pin (branch unchanged + gh never asked to checkout).features/review_pr_anchor.feature(5 scenarios) + step defs; feature-count bumped 26 → 27 in ci.yml AND lefthook.yml atomically (208 scenarios / 1384 steps green).Gate evidence
--all-targets --locked -D warningsexit 0 (judged byrtk proxy ...; $?); nextest 1700 passed; bdd 27 features / 208 scenarios passed (verified stable across repeated runs)git status examples/→ 0)cargo doc -D warnings(no-deps and --document-private-items) ✓;cargo deny check✓ — no new deps (gh JSON via the existing serde_json)origin/cli-303-pr-anchor== local HEADdbfe5c5Note:
.github/workflows/ci.ymlhere changes only the feature-count line (different region from builder-308's Windows job — git auto-merges).Closes #303
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Chores