Skip to content

chore: organize dirty work from codex/comparison-alias-distinctness - #1326

Closed
BigSimmo wants to merge 1 commit into
mainfrom
codex/comparison-alias-distinctness
Closed

chore: organize dirty work from codex/comparison-alias-distinctness#1326
BigSimmo wants to merge 1 commit into
mainfrom
codex/comparison-alias-distinctness

Conversation

@BigSimmo

@BigSimmoBigSimmo commented Jul 28, 2026

Copy link
Copy Markdown
Owner

Organized dirty work from codex/comparison-alias-distinctness

RAG impact: no retrieval behaviour change — branch cleanup and organisation only.

Summary by CodeRabbit

  • Bug Fixes

    • Improved document matching accuracy by ensuring each retrieved source can satisfy only one expected file.
    • Correctly distinguishes documents with different identities, even when their display names match.
    • Prevents repeated document chunks or duplicate expectations from inflating coverage results.
  • Tests

    • Added coverage for overlapping aliases, repeated entries, and document identity matching.

@supabase

supabaseBot commented Jul 28, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project sjrfecxgysukkwxsowpy because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@coderabbitai

coderabbitaiBot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

expectedFileCoverage now deduplicates retrieved source documents by identity, combines repeated chunks, and uses recursive one-to-one alias matching. Tests cover overlapping aliases, repeated entries, document_id identity selection, and distinct documents with identical display names.

Changes

Document coverage matching

Layer / File(s)Summary
Distinct-source assignment matcher
src/lib/eval-document-matching.ts
Source chunks are deduplicated by document_id, file_name, or title, their text is combined, and expectations are matched through recursive reassignment so each source is used once.
One-to-one matching test coverage
tests/eval-utils.test.ts
Tests cover overlapping aliases, complete assignments, repeated expectations, document_id identity selection, and distinct IDs sharing display names.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
participant expectedFileCoverage
participant SourceDocuments
participant AssignmentMatcher
expectedFileCoverage->>SourceDocuments: Deduplicate sources and combine repeated chunks
SourceDocuments->>AssignmentMatcher: Provide distinct documents and expected aliases
AssignmentMatcher->>AssignmentMatcher: Reassign matches recursively
AssignmentMatcher-->>expectedFileCoverage: Return matched file count
Loading

Possibly related PRs

  • BigSimmo/Database#564: Uses expectedFileCoverage(...).anyHit, so its relevance behavior depends on this matcher.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check nameStatusExplanationResolution
Description check⚠️ WarningThe description omits required template sections like Verification, Risk and rollout, and Notes, so it is largely incomplete.Fill in the full template with Summary, Verification, Risk and rollout, Clinical Governance Preflight if applicable, and Notes.
✅ Passed checks (4 passed)
Check nameStatusExplanation
Title check✅ PassedThe title matches the PR's branch-cleanup focus, though it is not very specific about the code changes.
Docstring Coverage✅ PassedNo functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check✅ PassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes check✅ PassedCheck skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

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

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (2)
src/lib/eval-document-matching.ts (2)

116-170: 🧹 Nitpick | 🔵 Trivial

Consider running the domain/production-readiness check for this eval-matching change.

This changes how document-coverage evaluation scores retrieval quality; per repo guidelines, search/ranking-adjacent changes should be paired with the smallest relevant domain check plus npm run check:production-readiness before handoff.

As per coding guidelines, "After authentication, privacy, ingestion, answer-generation, search/ranking, clinical-output, or source-governance changes, run the smallest relevant domain check plus npm run check:production-readiness."

🤖 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 `@src/lib/eval-document-matching.ts` around lines 116 - 170, Run the smallest
relevant domain check for the eval document-matching/search-ranking change, then
run npm run check:production-readiness before handoff. Verify both checks pass
for the updated distinctSourceDocuments, matchExpectedFilesToDistinctSources,
and expectedFileCoverage flow.

Source: Coding guidelines


114-163: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Bipartite-matching logic verified correct against all provided test scenarios.

Traced the augmenting-path (assign) reassignment through each test case (overlapping-alias distinct-counterpart, repeated-chunk dedup, document_id identity split/merge) — the one-to-one assignment behaves as intended in every case, including the reassignment branch that frees a source for a narrower expectation.

One gap: the recursive reassignment branch (lines 148-149) is only exercised by a single 2×2 case in the test suite. A 3-expectation/3-source scenario forcing a two-level reassignment chain would give more confidence in this non-trivial DFS.

🤖 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 `@src/lib/eval-document-matching.ts` around lines 114 - 163, Add a focused test
for matchExpectedFilesToDistinctSources covering three expectations and three
sources where satisfying the final expectation requires a two-level
augmenting-path reassignment. Assert that all three expectations are matched,
while preserving the existing tests and matching behavior.
🤖 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 `@src/lib/eval-document-matching.ts`:
- Around line 116-170: Run the smallest relevant domain check for the eval
document-matching/search-ranking change, then run npm run
check:production-readiness before handoff. Verify both checks pass for the
updated distinctSourceDocuments, matchExpectedFilesToDistinctSources, and
expectedFileCoverage flow.
- Around line 114-163: Add a focused test for
matchExpectedFilesToDistinctSources covering three expectations and three
sources where satisfying the final expectation requires a two-level
augmenting-path reassignment. Assert that all three expectations are matched,
while preserving the existing tests and matching behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 8ce9da1b-5d0e-443c-bb55-e430ff149355

📥 Commits

Reviewing files that changed from the base of the PR and between 1fd25f7 and 9f93b1a.

📒 Files selected for processing (2)
  • src/lib/eval-document-matching.ts
  • tests/eval-utils.test.ts

@cursor

cursorBot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Closing as superseded — no unique product value remains.

Comparison-alias distinctness already evolved on main (document_id-first identity + Kuhn matching + #80 dual aliases). This tip’s older matcher would conflict in eval-document-matching.ts and is not a retrieval behaviour change worth resurrecting.

Consolidation of the six-PR batch → #1335 and #1336.

@BigSimmo
BigSimmo deleted the codex/comparison-alias-distinctness branch July 30, 2026 09:10
Sign up for freeto 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

@BigSimmo