whitesmith / whitesmith/rubycritic

Compare mode (-b/--mode-ci) silently succeeds when the internal checkout to the base branch fails

Open Beginner friendly
#578 0 comments 0 reactions 0 assignees View on GitHub

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 main as of the file's most
    recent commit, 2025-07-30, #527 — not fixed by any change since)
  • Ruby 4.0.5

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.