whitesmith / whitesmith/rubycritic
Compare mode (-b/--mode-ci) silently succeeds when the internal checkout to the base branch fails
Nobody has claimed this yet.
- Dominant language
- Ruby
- Stars
- 3.5k
- Forks
- 234
- PR merge metrics
- No merged PRs in 30d
Description
Summary
In compare/CI mode (-b/--mode-ci), if the internal git checkout used to
switch to the base branch fails for any reason, RubyCritic doesn't notice: it
silently stays on the current branch, analyses it twice, and still labels one
copy "Base branch (X)" — reporting success with a comparison that never
actually happened.
Root cause
SourceControlSystem::Git.switch_branch runs git checkout via backticks
and never checks the result:
https://github.com/whitesmith/rubycritic/blob/main/lib/rubycritic/source_control_systems/git.rb
def self.switch_branch(branch)
dirty = !uncommitted_changes.empty?
abort("Uncommitted changes are present: #{uncommitted_changes}") if dirty
git("checkout #{branch}")
end
If git checkout #{branch} fails (e.g. the ref doesn't exist), $CHILD_STATUS
is never inspected, so execution continues as if the checkout had succeeded.
Command::Compare#analyse_branch then runs the full analysis against
whatever is actually checked out — which, since the checkout failed, is still
the other branch:
def analyse_branch(branch)
SourceControlSystem::Git.switch_branch(Config.send(branch))
critic = critique(branch)
Config.send(:"#{branch}_score=", critic.score)
...
end
build_details.txt is written from Config.base_branch/Config.feature_branch
— the option strings, not anything derived from the actual checked-out
state — so it still prints the branch name you asked for, even though that
branch's content was never analysed.
This is a different case from #377/#471: that fix raises when
base_branch == feature_branch (same name passed twice). Here the names
are different; the checkout to the base name simply fails silently.
Minimal reproduction
In any git repo with RubyCritic configured, with a clean working tree on any
branch:
bundle exec rubycritic --mode-ci this-branch-does-not-exist --maximum-decrease 0 <a small path>
Output (trimmed):
fatal: ambiguous argument 'this-branch-does-not-exist': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
...
Score: 73.59
Exit code: 0. compare/build_details.txt:
Base branch (this-branch-does-not-exist) score: 73.59
Feature branch (<current-branch>) score: 73.59
Both scores are identical because both "branches" analysed are the same,
actually-checked-out content. The fatal: ambiguous argument line (from the
git diff --name-status call used for modified_files) is the only visible
symptom, and it's easy to miss among the analyser progress dots — nothing
about the run's exit code or the generated report flags it.
I hit this in a CI setup where the base ref is created by a separate step
before invoking RubyCritic; if that step ever fails to produce the expected
ref (a race condition, a scripting bug, anything), --maximum-decrease 0
silently stops comparing anything and always reports "no regression" instead
of failing the build. That's the opposite of what a CI quality gate should do
when it can't actually check what it's supposed to check.
Suggested fix
Check the result of the checkout in switch_branch, e.g.:
def self.switch_branch(branch)
dirty = !uncommitted_changes.empty?
abort("Uncommitted changes are present: #{uncommitted_changes}") if dirty
git("checkout #{branch}")
abort("Failed to check out branch/ref '#{branch}'.") unless $CHILD_STATUS.success?
end
I'm happy to open a PR with this (plus a test) if that's welcome — let me
know if you'd want it shaped differently, e.g. raising instead of abort, or
verifying git rev-parse HEAD against the resolved ref instead of trusting
the checkout's exit status alone.
Environment
- RubyCritic 5.0.0 (also confirmed present on
mainas of the file's most
recent commit, 2025-07-30, #527 — not fixed by any change since) - Ruby 4.0.5
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in lib/rubycritic/source_control_systems/git.rb at SourceControlSystem::Git.switch_branch, then trace its use from Command::Compare#analyse_branch. Reproduce the missing-ref command from the issue and add a regression test for a failed checkout. Done means the compare run reports failure instead of analysing the current branch twice and exiting successfully.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- git, ruby
- Domain
- cli, tooling
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 78/100