refactor(dedup): added batchrewrite helper to remove duplications#223
Merged
SantiagoDePolonia merged 2 commits intomainfrom Apr 11, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughConsolidates batch-rewrite bookkeeping, file cleanup, and endpoint-hint merging into a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors native batch request rewrite handling by consolidating repeated logic (rewrite metadata recording, endpoint-hint merging, and temporary rewritten input file cleanup) into a shared internal/batchrewrite helper package.
Changes:
- Introduced
internal/batchrewritehelpers for recording batch rewrite metadata, cleaning up rewritten input files, and merging endpoint hints. - Updated native batch server flow and provider decorators to use the shared helpers instead of local duplicated implementations.
- Added unit tests covering the new helper behaviors.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/server/native_batch_support.go | Uses shared cleanup helper for rewritten batch input file deletion and removes local hint-merge helper. |
| internal/server/native_batch_service.go | Uses shared helpers to record rewrite results and merge endpoint hints before persisting batches. |
| internal/server/batch_request_preparer.go | Replaces local cleanup + hint merge logic with shared helper calls during preparer chaining. |
| internal/guardrails/provider.go | Replaces duplicated rewrite-recording, cleanup, and hint-merging helpers with shared batchrewrite utilities. |
| internal/aliases/provider.go | Replaces duplicated rewrite-recording, cleanup, and hint-merging helpers with shared batchrewrite utilities. |
| internal/batchrewrite/helpers.go | New shared helper implementations for rewrite metadata recording, cleanup, and hint merging. |
| internal/batchrewrite/helpers_test.go | New tests validating the helper functions’ behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.pre-commit-config.yaml:
- Around line 23-28: The pre-commit hook with id "go-unit-tests" currently runs
"make test-race", which invokes the Go race detector and slows commits; update
the hook to run "make test" instead (change the entry value from "make
test-race" to "make test" for the go-unit-tests hook) so pre-commit uses the
faster test target while keeping "make test-race" reserved for CI or explicit
use.
In `@DEVELOPMENT.md`:
- Around line 25-29: DEVELOPMENT.md is missing documentation for the Makefile
targets test-integration and test-contract; update the test target list (the
same section that currently lists make test, make test-race, make
test-dashboard, make test-e2e, make test-all) to include brief entries for make
test-integration and make test-contract, describing what each runs (integration
tests and contract tests respectively) and any special flags or prerequisites
(e.g., tags, environment variables, or services required) so readers can run
them consistently.
In `@docs/adr/0006-semantic-response-cache.md`:
- Line 118: Replace the weak phrase "very large" in the sentence "Pinecone
metadata size caps can block caching very large JSON bodies — mitigated by clear
errors / operational choice of backend" with stronger wording such as "oversized
JSON bodies" or "large JSON payloads" so the ADR reads e.g. "Pinecone metadata
size caps can block caching oversized JSON bodies — mitigated by clear errors /
operational choice of backend"; update that exact sentence in
docs/adr/0006-semantic-response-cache.md.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 575fbbe6-50e9-4f9c-bf6b-66a34bec4dd4
📒 Files selected for processing (6)
.github/workflows/test.yml.pre-commit-config.yamlDEVELOPMENT.mdMakefilecmd/gomodel/main_test.godocs/adr/0006-semantic-response-cache.md
Comment on lines
+23
to
+28
| - id: go-unit-tests | ||
| name: make test-race | ||
| entry: make test-race | ||
| language: system | ||
| pass_filenames: false | ||
| files: '^((cmd|config|internal)/.*\.go|go\.(mod|sum)|Makefile|\.github/workflows/test\.yml)$' |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using make test instead of make test-race for pre-commit.
Running the race detector on every commit can significantly slow down the development workflow. The race detector adds substantial overhead (typically 2-10x slower execution). Consider:
- Using
make testfor pre-commit (fast feedback) - Reserving
make test-racefor CI where thoroughness is prioritized over speed
If the race detector is intentionally required at commit time, this is fine as-is.
♻️ Suggested change for faster pre-commit
- id: go-unit-tests
- name: make test-race
- entry: make test-race
+ name: make test
+ entry: make test
language: system
pass_filenames: false
files: '^((cmd|config|internal)/.*\.go|go\.(mod|sum)|Makefile|\.github/workflows/test\.yml)$'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - id: go-unit-tests | |
| name: make test-race | |
| entry: make test-race | |
| language: system | |
| pass_filenames: false | |
| files: '^((cmd|config|internal)/.*\.go|go\.(mod|sum)|Makefile|\.github/workflows/test\.yml)$' | |
| - id: go-unit-tests | |
| name: make test | |
| entry: make test | |
| language: system | |
| pass_filenames: false | |
| files: '^((cmd|config|internal)/.*\.go|go\.(mod|sum)|Makefile|\.github/workflows/test\.yml)$' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.pre-commit-config.yaml around lines 23 - 28, The pre-commit hook with id
"go-unit-tests" currently runs "make test-race", which invokes the Go race
detector and slows commits; update the hook to run "make test" instead (change
the entry value from "make test-race" to "make test" for the go-unit-tests hook)
so pre-commit uses the faster test target while keeping "make test-race"
reserved for CI or explicit use.
Comment on lines
+25
to
+29
| make test # Go unit tests | ||
| make test-race # Go unit tests with race detection and coverage | ||
| make test-dashboard # Dashboard JavaScript unit tests | ||
| make test-e2e # End-to-end tests (requires -tags=e2e; uses in-process mock servers, no Docker) | ||
| make test-all # All tests |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Documentation is missing test-integration and test-contract targets.
The Makefile defines additional test targets that aren't documented here. Consider adding them for completeness:
📝 Suggested addition
make test # Go unit tests
make test-race # Go unit tests with race detection and coverage
make test-dashboard # Dashboard JavaScript unit tests
make test-e2e # End-to-end tests (requires -tags=e2e; uses in-process mock servers, no Docker)
+make test-integration # Integration tests (requires Docker; -tags=integration)
+make test-contract # Contract replay tests (requires -tags=contract)
make test-all # All tests📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| make test # Go unit tests | |
| make test-race # Go unit tests with race detection and coverage | |
| make test-dashboard # Dashboard JavaScript unit tests | |
| make test-e2e # End-to-end tests (requires -tags=e2e; uses in-process mock servers, no Docker) | |
| make test-all # All tests | |
| make test # Go unit tests | |
| make test-race # Go unit tests with race detection and coverage | |
| make test-dashboard # Dashboard JavaScript unit tests | |
| make test-e2e # End-to-end tests (requires -tags=e2e; uses in-process mock servers, no Docker) | |
| make test-integration # Integration tests (requires Docker; -tags=integration) | |
| make test-contract # Contract replay tests (requires -tags=contract) | |
| make test-all # All tests |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@DEVELOPMENT.md` around lines 25 - 29, DEVELOPMENT.md is missing documentation
for the Makefile targets test-integration and test-contract; update the test
target list (the same section that currently lists make test, make test-race,
make test-dashboard, make test-e2e, make test-all) to include brief entries for
make test-integration and make test-contract, describing what each runs
(integration tests and contract tests respectively) and any special flags or
prerequisites (e.g., tags, environment variables, or services required) so
readers can run them consistently.
| - ~50–100 ms added latency on semantic miss (acceptable vs LLM latency) | ||
| - Requires a provider with a working OpenAI-compatible embeddings endpoint and credentials | ||
| - Semantic layer **requires external vector infrastructure** (no embedded sqlite-vec path) | ||
| - Pinecone metadata size caps can block caching very large JSON bodies — mitigated by clear errors / operational choice of backend |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer stronger wording over “very large.”
Line 118 can be tightened to avoid weak intensifiers (e.g., “oversized JSON bodies” or “large JSON payloads”).
🧰 Tools
🪛 LanguageTool
[style] ~118-~118: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...ne metadata size caps can block caching very large JSON bodies — mitigated by clear errors...
(EN_WEAK_ADJECTIVE)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/adr/0006-semantic-response-cache.md` at line 118, Replace the weak
phrase "very large" in the sentence "Pinecone metadata size caps can block
caching very large JSON bodies — mitigated by clear errors / operational choice
of backend" with stronger wording such as "oversized JSON bodies" or "large JSON
payloads" so the ADR reads e.g. "Pinecone metadata size caps can block caching
oversized JSON bodies — mitigated by clear errors / operational choice of
backend"; update that exact sentence in
docs/adr/0006-semantic-response-cache.md.
SantiagoDePolonia
deleted the
refactor/create-a-batchrewrite-module-to-dedup
branch
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
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