Skip to content

Fix CodeRabbit findings from PR #34's full-diff review - #36

Merged
hardcoreerik merged 1 commit into
masterfrom
fix/coderabbit-exhaustive-answer-findings
Jul 4, 2026
Merged

Fix CodeRabbit findings from PR #34's full-diff review#36
hardcoreerik merged 1 commit into
masterfrom
fix/coderabbit-exhaustive-answer-findings

Conversation

@hardcoreerik

@hardcoreerik hardcoreerik commented Jul 4, 2026

Copy link
Copy Markdown
Owner

Summary

  • BuildExhaustiveAnswer used FirstOrDefault() per card, silently dropping additional genuinely-matching claims when a card lists multiple distinct entries — now keeps every matching claim (ordered by segment then score, IncludedSegmentIds deduped).
  • Added "under" to the scoring stopword list — without it, an incidental single mention of "under" in an unrelated card could look rarer than the real distinguishing identifier and wrongly become the sole distinctive term, excluding the actual target cards (same failure class as the earlier "every" bug).
  • Tightened ContextFabricExhaustiveAnswerTests and ContextFabricB2TopKRagTests — both had weak assertions that didn't actually exercise the scenarios their comments claimed.

All three were flagged by CodeRabbit's full-diff review of PR #34, posted before merge but not addressed in time.

Test plan

  • Full suite: 471 passed / 0 failed / 4 skipped
  • Build clean

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Exhaustive answers now include all matching claims from the same evidence card, rather than only the top-ranked one.
    • Matching segment IDs are now returned without duplicates.
    • Improved term selection so more generic words are ignored, helping the right content rank higher.
  • Tests

    • Updated and expanded test coverage for ranking and exhaustive-answer behavior, including cases with multiple matches in one entry.

Three real bugs in BuildExhaustiveAnswer/tests that were posted by
CodeRabbit before merge but not addressed in time:

- FirstOrDefault() kept only the single highest-scoring claim per
  card, silently dropping other genuinely matching claims when a card
  lists multiple distinct entries. Now keeps every matching claim,
  ordered by segment then score; IncludedSegmentIds deduped since a
  card can contribute more than one claim.
- "under" wasn't a scoring stopword, so "list every X under ledger Y"
  could let an incidental single mention of "under" in an unrelated
  card look rarer than the actual distinguishing identifier and wrongly
  become the sole distinctive term, excluding the real target cards.
  Same failure class as the earlier "every" bug.
- ContextFabricExhaustiveAnswerTests' 5-entry test only asserted a
  count, not that all 5 case IDs were actually present -- wouldn't
  have caught the FirstOrDefault() bug above. Tightened, plus added a
  dedicated multi-claim-per-card regression test.
- ContextFabricB2TopKRagTests' big-vs-small budget test repeated the
  same term three times expecting it to outscore a single mention, but
  Tokenize() returns a per-segment HashSet so repetition doesn't
  increase score -- the test passed without exercising the ranking
  behavior it claimed to. Rewritten so "big" genuinely outranks "small"
  via distinct rare terms ("batch", "inspector") "small" lacks.

Full suite: 471/0/4 (was 470/0/4; +1 new regression test).
@coderabbitai

coderabbitai Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 8cb0d4e9-08c1-46fc-af11-4ed9af5d79c7

📥 Commits

Reviewing files that changed from the base of the PR and between 4a745ce and 2b75207.

📒 Files selected for processing (3)
  • OrchestratorIDE.UnitTests/ContextFabricB2TopKRagTests.cs
  • OrchestratorIDE.UnitTests/ContextFabricExhaustiveAnswerTests.cs
  • OrchestratorIDE/Services/ContextFabric/ContextFabricFeasibilityRunner.cs

📝 Walkthrough

Walkthrough

Modifies ContextFabricFeasibilityRunner.BuildExhaustiveAnswer to select all matching claims per evidence card (instead of only the top-scoring one), deduplicate IncludedSegmentIds, and add "under" to scoring stopwords. Updates corresponding unit tests and revises a Top-K RAG test fixture to use distinct rare terms for IDF scoring.

Changes

Exhaustive Answer Multi-Claim Selection

Layer / File(s) Summary
Core selection logic and stopword changes
OrchestratorIDE/Services/ContextFabric/ContextFabricFeasibilityRunner.cs
Selection changed from single highest-scoring claim per card to all overlapping claims via SelectMany ordered by IDF score; IncludedSegmentIds deduplicated with Distinct; "under" added to _scoringStopwords.
Exhaustive answer test updates
OrchestratorIDE.UnitTests/ContextFabricExhaustiveAnswerTests.cs
Updates existing test to assert all five CASE-01-* matches and adds a new test verifying multiple claims from a single evidence card are all included.

Top-K RAG Test Fixture Correction

Layer / File(s) Summary
IDF-based fixture rewrite
OrchestratorIDE.UnitTests/ContextFabricB2TopKRagTests.cs
Replaces repeated CK-777 mentions with distinct rare terms (batch, inspector) in the big segment text and benchmark question, keeping expected assertions and comments updated.

Estimated code review effort: 2 (Simple) | ~12 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Test
  participant BuildExhaustiveAnswer
  participant EvidenceCard
  Test->>BuildExhaustiveAnswer: run question with evidence cards
  BuildExhaustiveAnswer->>EvidenceCard: filter claims overlapping distinctive terms
  EvidenceCard-->>BuildExhaustiveAnswer: all matching claims per card
  BuildExhaustiveAnswer->>BuildExhaustiveAnswer: order by IDF score, dedupe IncludedSegmentIds
  BuildExhaustiveAnswer-->>Test: answer text with all matching claims
Loading

Possibly related PRs

  • hardcoreerik/TheOrc#32: Both PRs modify ContextFabricFeasibilityRunner.BuildExhaustiveAnswer logic, touching selection/deduping and related answer-building behavior.
🚥 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 is related to the patch, though it describes the source of the fix rather than the actual code changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/coderabbit-exhaustive-answer-findings

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

@hardcoreerik
hardcoreerik merged commit a5f2eae into master Jul 4, 2026
2 checks passed
@hardcoreerik
hardcoreerik deleted the fix/coderabbit-exhaustive-answer-findings branch July 18, 2026 02:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant