Skip to content

fix: Two pipeline bugs affecting rules loading and acceptance rate#369

Merged
justn-hyeok merged 1 commit intobssm-oss:mainfrom
HuiNeng6:fix/pipeline-quality-bugs
Mar 28, 2026
Merged

fix: Two pipeline bugs affecting rules loading and acceptance rate#369
justn-hyeok merged 1 commit intobssm-oss:mainfrom
HuiNeng6:fix/pipeline-quality-bugs

Conversation

@HuiNeng6
Copy link
Copy Markdown
Contributor

@HuiNeng6 HuiNeng6 commented Mar 27, 2026

Fix two high-priority pipeline bugs:

1. orchestrator.ts: Rules not found in CI/MCP (Issue #302)

  • Was using \process.cwd()\ for \loadReviewRules\ and \loadLearnedPatterns\
  • This fails when running in GitHub Actions or MCP with different working directory
  • Now uses \input.repoPath ?? process.cwd()\ to respect the repository path

2. quality-tracker.ts: headAcceptanceRate penalizes thorough reviewers (Issue #303)

  • Was dividing \headAccepted\ by \issuesRaised\
  • \headAccepted\ only counts discussed issues
  • \issuesRaised\ includes issues below threshold (never discussed)
  • Result: thorough reviewers get lower scores than lazy ones
  • Now divides by \ otalInDiscussion\ (the correct denominator)

Fixes: #302
Fixes: #303

Summary by CodeRabbit

Bug Fixes

  • Fixed quality metrics calculation to count only issues that participated in discussions when measuring acceptance rates.
  • Fixed the pipeline to load custom review rules and learned patterns from the correct repository path when specified.

1. orchestrator.ts: Use input.repoPath for loadReviewRules/loadLearnedPatterns
   - Was using process.cwd() which fails in CI/MCP with different cwd
   - Now falls back to process.cwd() only if repoPath is undefined

2. quality-tracker.ts: Fix headAcceptanceRate denominator
   - Was dividing by issuesRaised (includes non-discussed issues)
   - Now divides by totalInDiscussion (only discussed issues)
   - Prevents penalizing thorough reviewers who raise issues below threshold

Fixes: #302
Fixes: #303
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 27, 2026

📝 Walkthrough

Walkthrough

Two bug fixes applied: corrected headAcceptanceRate calculation in quality tracking to use only discussed issues as denominator instead of all raised issues, and fixed runPipeline to respect provided repository path when loading custom review rules and learned patterns instead of always using current working directory.

Changes

Cohort / File(s) Summary
Quality Tracker Acceptance Rate Calculation
packages/core/src/l0/quality-tracker.ts
Modified headAcceptanceRate denominator from issuesRaised to totalInDiscussion, with guard clause returning 1.0 when no discussions occurred. Prevents underrating reviewers who raise issues below discussion threshold.
Pipeline Repository Path Resolution
packages/core/src/pipeline/orchestrator.ts
Updated loadReviewRules and loadLearnedPatterns calls to use input.repoPath ?? process.cwd() instead of hardcoded process.cwd(), ensuring rules and patterns load from the specified repository root in CI/containerized environments.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 Two bugs hopped away today,
Rules now find their rightful way,
Fair reviewers get their due,
Math and paths both fixed anew! ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the two main bug fixes in the changeset: correcting rules loading with repoPath and fixing the acceptance rate calculation.
Linked Issues check ✅ Passed The changes fully address both linked issues: orchestrator.ts now uses input.repoPath ?? process.cwd() for rule loading [#302], and quality-tracker.ts now divides by totalInDiscussion instead of issuesRaised [#303].
Out of Scope Changes check ✅ Passed All changes are directly scoped to the two linked issues with no unrelated modifications to the codebase.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/core/src/pipeline/orchestrator.ts (1)

530-540: ⚠️ Potential issue | 🟡 Minor

The orchestrator changes are correct, but callers need updating for full feature support.

The fix correctly respects input.repoPath when provided and falls back to process.cwd(). However, not all callers pass it:

  • CLI (packages/cli/src/index.ts): ✓ Detects and passes repoPath via git root detection
  • GitHub Action (packages/github/src/action.ts:88): ✗ Only passes { diffPath: inputs.diff }
  • MCP (packages/mcp/src/helpers.ts:26): ✗ Only passes diffPath and options
  • TUI (packages/tui/src/screens/PipelineScreen.tsx:32): ✗ Only passes diffPath

The orchestrator change works for CLI users, but GitHub Action and MCP—the environments mentioned in issue #302—will continue to use process.cwd() unless updated to detect and pass repoPath.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/pipeline/orchestrator.ts` around lines 530 - 540, Callers
of the orchestrator must pass the repoPath detected from the git root (as the
CLI already does) so the orchestrator's use of input.repoPath ?? process.cwd()
works correctly; update the GitHub Action, MCP helpers, and TUI entry points
that currently call the pipeline/orchestrator with only { diffPath } to detect
the repository root (use the same git root detection logic used in the CLI) and
include repoPath in the input object passed to the orchestrator (the same input
shape that loadReviewRules and loadLearnedPatterns expect), ensuring every
invocation provides repoPath when available.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@packages/core/src/pipeline/orchestrator.ts`:
- Around line 530-540: Callers of the orchestrator must pass the repoPath
detected from the git root (as the CLI already does) so the orchestrator's use
of input.repoPath ?? process.cwd() works correctly; update the GitHub Action,
MCP helpers, and TUI entry points that currently call the pipeline/orchestrator
with only { diffPath } to detect the repository root (use the same git root
detection logic used in the CLI) and include repoPath in the input object passed
to the orchestrator (the same input shape that loadReviewRules and
loadLearnedPatterns expect), ensuring every invocation provides repoPath when
available.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 857a6ed4-ff94-41ce-9f99-06031c7e1f01

📥 Commits

Reviewing files that changed from the base of the PR and between 1b61dac and 15f875e.

📒 Files selected for processing (2)
  • packages/core/src/l0/quality-tracker.ts
  • packages/core/src/pipeline/orchestrator.ts

Copy link
Copy Markdown
Collaborator

@justn-hyeok justn-hyeok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! headAcceptanceRate denominator fix correct. repoPath fallback preserves compat. Follow-up: update Action/MCP callers.

@justn-hyeok justn-hyeok merged commit a69e5e4 into bssm-oss:main Mar 28, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants