docs: planning out the auto review#99
Open
nikilok wants to merge 3 commits into
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
📝 WalkthroughWalkthroughThis documentation redesigns Phase 5 of the sweep algorithm to eliminate the asynchronous review queue workflow. Same-rank company mapping ambiguities are now resolved inline during sweep execution using a configurable scoreCandidate function with a MARGIN-based promotion threshold. The decision model, operational reporting, and test expectations are updated accordingly. ChangesPhase 5 Sweep: Inline Resolution Replaces Review Queue
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@docs/review-queue-resolver.md`:
- Around line 96-103: Wrap the per-row work so a single failure doesn't abort
the loop: around the existing process(row) call in the loop that iterates over
rows, add per-row try/except handling to catch/log the exception (including
which row/id failed) and continue to the next row; ensure sleep(550ms) still
runs after each row and that print summary(verified, declined, already_applied,
superseded, lock_missed, errored) is unchanged so totals include errored rows.
- Around line 90-99: The current loop uses SELECT ... FOR UPDATE SKIP LOCKED on
hmrc_company_mapping_review_queue and holds the transaction open while calling
process(row) and sleep(550ms), which keeps row locks until commit; instead
implement a two-step "claim then process" pattern: in a short transaction run a
SELECT ... FOR UPDATE SKIP LOCKED LIMIT max_rows to identify rows where
resolved_at IS NULL, update those rows to set claimed_at and claimed_by (or
similar marker) and commit immediately, then outside any transaction iterate
over the claimed rows calling process(row) and sleeping as needed; update code
paths that reference detected_at, resolved_at, process(row), and any
LIMIT/max_rows logic to use claimed_at/claimed_by so locks are not held during
CH/model I/O.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d924de4d-7a24-45ea-bc2c-7aadc762c379
📒 Files selected for processing (1)
docs/review-queue-resolver.md
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@docs/review-queue-resolver.md`:
- Line 197: Two fenced code blocks in docs/review-queue-resolver.md (the one
showing the field list starting with "query, query_slug, query_normalised, ..."
and the project tree starting with "apps/ml-reranker/") are missing language
identifiers and trigger MD040; update both opening fences to include an explicit
language (e.g., change ``` to ```text) so the blocks are fenced as ```text;
ensure you update the fences around the blocks that contain the field list and
the apps/ml-reranker tree respectively.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7914ccbb-1b53-4296-9967-c0bd33885a3a
📒 Files selected for processing (1)
docs/review-queue-resolver.md
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@docs/phase5-sweep-algorithm.md`:
- Around line 251-252: The scorer is being fed a self-comparison:
scoreCandidate(existing_profile, sponsor, existing_profile) passes the same
object as both candidate and existing, biasing company_type and succession
checks; change the calls to use a true baseline (e.g., the original HMRC mapping
record or the common baseline object used for comparison) so the two
scoreCandidate invocations compare different candidate objects against the same
existing baseline (or, if the third parameter is redundant, remove it and
refactor scoreCandidate signature to accept only (candidate, sponsor) and update
its internal logic); also apply the same fix to the repeated calls in the
migration script (the pair at lines ~313-314) to eliminate self-comparison.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6aa6c5be-0499-4236-aa95-e37b92e6b2f1
📒 Files selected for processing (1)
docs/phase5-sweep-algorithm.md
Comment on lines
+251
to
+252
| s_e = scoreCandidate(existing_profile, sponsor, existing_profile) | ||
| s_p = scoreCandidate(proposed_profile, sponsor, existing_profile) |
There was a problem hiding this comment.
Logic error in scoreCandidate invocations.
When scoring the existing profile on line 251, you pass existing_profile as both the candidate and existing parameters:
s_e = scoreCandidate(existing_profile, sponsor, existing_profile)
This creates a self-comparison where:
- Line 235's check
candidate.company_type == existing.company_typealways evaluates to true → always awards +1 - Line 223's succession check compares the profile against itself
This pattern repeats on lines 313-314 in the migration script.
The scorer should evaluate both candidates against the same consistent baseline. Consider whether:
- The third
existingparameter is actually needed, or - Both calls should pass a different reference object (e.g., the original HMRC mapping record rather than a CH profile), or
- The scoring model needs revision to avoid self-comparison bias
🤖 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 `@docs/phase5-sweep-algorithm.md` around lines 251 - 252, The scorer is being
fed a self-comparison: scoreCandidate(existing_profile, sponsor,
existing_profile) passes the same object as both candidate and existing, biasing
company_type and succession checks; change the calls to use a true baseline
(e.g., the original HMRC mapping record or the common baseline object used for
comparison) so the two scoreCandidate invocations compare different candidate
objects against the same existing baseline (or, if the third parameter is
redundant, remove it and refactor scoreCandidate signature to accept only
(candidate, sponsor) and update its internal logic); also apply the same fix to
the repeated calls in the migration script (the pair at lines ~313-314) to
eliminate self-comparison.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit