feat: semantic search and similar feedback API#40
Merged
Conversation
✱ Stainless preview buildsThis PR will update the This comment is auto-generated by GitHub Actions and is automatically kept up to date as you push. |
mattinannt
reviewed
Feb 27, 2026
Member
mattinannt
left a comment
mattinannt
left a comment
There was a problem hiding this comment.
@xernobyl Thanks for the PR :-)
Looking good already; we just need some fine tuning :-)
I'll also test the functionality in more detail in the coming days and also build a small use-case to test this with real data.
Member
|
@coderabbitai please review |
|
✅ Actions performedReview triggered.
|
|
✅ Actions performedFull review triggered. |
mattinannt
approved these changes
Mar 4, 2026
Member
mattinannt
left a comment
mattinannt
left a comment
There was a problem hiding this comment.
Thanks for the changes; works great! :-)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Around line 53-54: Add and expose the SEARCH_RATE_LIMIT_REQUESTS_PER_MINUTE
setting in the runtime config by adding a SearchRateLimitRequestsPerMinute int
field to the Config struct in internal/config/config.go and loading it from the
environment in the config loader (e.g., where other env vars are parsed in
LoadConfig/NewConfig). Implement a rate-limiting middleware in
internal/api/middleware (or extend existing middleware) that reads the
configured limit and enforces per-API-key per-minute limits for the semantic
search and similar-feedback routes, then register/apply that middleware in
cmd/api/app.go for the relevant handlers (semantic-search and similar-feedback
endpoints) so the value from Config is actually enforced at runtime. Ensure the
middleware identifies requests by API key and returns the proper HTTP 429 when
the limit is exceeded.
In @.github/workflows/api-contract-tests.yml:
- Around line 122-128: The current CI workflow excludes the two search operation
IDs (semantic-search-feedback-records and similar-feedback-records) from the
generic Schemathesis step, dropping contract coverage; keep those
--exclude-operation-id entries if needed for the generic step but add a new
dedicated Schemathesis job/step that targets only these operations (use
--operation-id semantic-search-feedback-records --operation-id
similar-feedback-records or equivalent scoping) and do not pass the
not_a_server_error override so the documented 503 behavior is validated; ensure
the new step uses the same auth/header args as the others (e.g., -H
"Authorization: Bearer test-api-key-for-ci") and runs in CI alongside the
existing step.
In `@internal/api/handlers/search_handler.go`:
- Around line 253-269: The parseMinScore function currently treats a parse error
as 0, which silently disables the min_score filter; change parseMinScore so that
when s is empty or when strconv.ParseFloat(s, 64) returns an error it returns
defaultMinScore instead of 0, and keep the existing guards that clamp negative
values to 0 and cap the value at 1 (i.e., after successful parse, if val < 0
return 0, else return math.Min(val, 1)). This ensures malformed min_score falls
back to defaultMinScore while valid numeric inputs are still clamped to [0,1].
In `@internal/api/response/response.go`:
- Around line 89-95: The current logic in fieldNameForAPI that adds underscores
uses only the next-rune check, causing "TenantID" -> "tenanti_d"; update the
underscore condition to also detect a lower-to-upper transition by checking the
previous rune: insert '_' when i>0 && unicode.IsUpper(r) &&
(unicode.IsLower(rune(fieldPath[i-1])) || (i+1 < len(fieldPath) &&
unicode.IsLower(rune(fieldPath[i+1])))). Apply this change in the loop that
iterates over fieldPath so acronyms like "ID" become "id" and "TenantID" becomes
"tenant_id".
In `@internal/models/feedback_records.go`:
- Line 105: The comment on the SubmissionID field in the FeedbackRecord struct
contradicts the new mandatory/non-null invariant; update the comment on
SubmissionID (the struct tag `SubmissionID string
`json:"submission_id"``) to remove the parenthetical “DB NULL is exposed as ""”
or reword it to reflect that the field is non-null/mandatory (e.g., "mandatory;
never null"). Ensure the new comment accurately describes the enforced invariant
and remove any mention of DB NULL exposure.
In `@internal/service/search_service.go`:
- Around line 134-144: The pagination cursor is being suppressed by the
condition len(results) == limit even though the repo over-fetches and then
filters by minScore; update the logic that sets out.NextCursor (where
EncodeSearchCursor is called) to consider whether the repository fetched the
full fetchLimit (e.g., have the repo return a boolean like fetchedFullLimit or
similar alongside results) and set NextCursor when fetchedFullLimit is true or
when len(results) == limit (or alternatively set a cursor whenever len(results)
> 0); modify the service code that assigns out.Results and computes
out.NextCursor to use that new fetchedFullLimit flag instead of relying solely
on len(results) == limit so pages aren’t prematurely terminated.
In `@Makefile`:
- Line 1: The .PHONY line needs to include aliases "all" and "test" so
checkmake/lint won't flag missing required phony targets; update the .PHONY
declaration (which currently lists targets like help tests tests-coverage build
...) to also include the names all and test (as aliases that map to your
existing default targets) so CI/lint expectations are satisfied while preserving
current target behavior.
- Around line 66-69: The Makefile target run-backfill-embeddings should fail
fast if loading .env fails: update the recipe so that sourcing .env is required
(do not use a trailing semicolon that allows continuation), and ensure the shell
exits with a non-zero status when .env is missing or the source command fails
before invoking go run ./cmd/backfill-embeddings; modify the
run-backfill-embeddings recipe to check and source .env with a failing path (or
use &&/|| control flow) and only call go run when the .env load succeeded.
In `@migrations/005_embeddings_indexes_and_feedback_tenant_required.sql`:
- Line 4: The CREATE INDEX for idx_embeddings_model on the embeddings table is
not using CONCURRENTLY and will block writes on large tables; change the
statement that creates idx_embeddings_model to use "CREATE INDEX CONCURRENTLY
idx_embeddings_model ON embeddings (model)" so the B-tree build does not lock
writes, and ensure the migration runner supports concurrent index creation (or
run it outside a transaction if required).
- Line 25: The Down migration currently drops idx_embeddings_model with a
regular DROP INDEX; change it to use DROP INDEX CONCURRENTLY IF EXISTS
idx_embeddings_model to match the other index (idx_embeddings) and avoid
blocking writes during rollback, and ensure this CONCURRENTLY call is executed
outside of a transaction if your migration runner requires that.
In `@openapi.yaml`:
- Around line 576-580: The OpenAPI parameter schema for the offset parameter
currently documents a 1000 cap in the description but does not enforce it;
update the offset parameter schemas (the entries where description reads "Number
of results to skip (OFFSET-based paging). Ignored if cursor is set. Capped at
1000 for performance.") to include "maximum: 1000" under the schema object (and
leave minimum: 0 and default: 0 intact) so the cap is enforced in both
occurrences.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 00aa247e-4680-4f05-9e2e-fcbc1e87e5ab
📒 Files selected for processing (18)
.env.example.github/workflows/api-contract-tests.ymlMakefileinternal/api/handlers/feedback_records_handler.gointernal/api/handlers/search_handler.gointernal/api/handlers/search_handler_test.gointernal/api/handlers/webhooks_handler.gointernal/api/response/response.gointernal/api/response/response_test.gointernal/models/feedback_records.gointernal/repository/embeddings_repository.gointernal/service/feedback_records_service.gointernal/service/search_cursor.gointernal/service/search_service.gointernal/service/search_service_test.gomigrations/005_embeddings_indexes_and_feedback_tenant_required.sqlopenapi.yamltests/integration_test.go
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (6)
internal/api/response/response.go (1)
89-95:⚠️ Potential issue | 🟡 MinorAcronym conversion is still incorrect for API field names.
On Line 90, the underscore rule still splits
"TenantID"as"tenanti_d"instead of"tenant_id", which can mislead clients in type-error responses.💡 Suggested fix
- var buf strings.Builder - - for i, r := range fieldPath { - if i > 0 && unicode.IsUpper(r) && (i+1 >= len(fieldPath) || unicode.IsLower(rune(fieldPath[i+1]))) { - buf.WriteByte('_') - } - - buf.WriteRune(unicode.ToLower(r)) - } + var buf strings.Builder + runes := []rune(fieldPath) + + for i, r := range runes { + if i > 0 && unicode.IsUpper(r) { + prev := runes[i-1] + nextIsLower := i+1 < len(runes) && unicode.IsLower(runes[i+1]) + if unicode.IsLower(prev) || (unicode.IsUpper(prev) && nextIsLower) { + buf.WriteByte('_') + } + } + buf.WriteRune(unicode.ToLower(r)) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/response/response.go` around lines 89 - 95, The camelCase→snake_case conversion in the loop that builds buf mishandles acronyms (e.g., "TenantID"); change the underscore insertion condition in that loop to insert an underscore when the current rune is uppercase and either the previous rune is lowercase OR the next rune is lowercase (i.e., if i>0 && unicode.IsUpper(r) && (unicode.IsLower(rune(fieldPath[i-1])) || (i+1<len && unicode.IsLower(rune(fieldPath[i+1]))))). Update the logic around the loop that writes runes (the code handling fieldPath and buf) to use this condition so acronyms like "ID" become "id" and "TenantID" becomes "tenant_id".Makefile (2)
1-1:⚠️ Potential issue | 🟡 MinorAdd required phony aliases
allandtest.Line 1 still misses the aliases required by checkmake.
Suggested patch
-.PHONY: help tests tests-coverage build build-backfill-embeddings run run-backfill-embeddings init-db clean docker-up docker-down docker-clean deps install-tools fmt lint lint-new dev-setup test-all test-unit schemathesis install-hooks migrate-status migrate-validate river-migrate +.PHONY: all test help tests tests-coverage build build-backfill-embeddings run run-backfill-embeddings init-db clean docker-up docker-down docker-clean deps install-tools fmt lint lint-new dev-setup test-all test-unit schemathesis install-hooks migrate-status migrate-validate river-migrate + +all: build +test: test-all🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` at line 1, The .PHONY declaration is missing the required phony aliases "all" and "test" required by checkmake; update the Makefile by adding "all" and "test" to the .PHONY list (or create phony targets named all and test that delegate to existing targets like build and tests) so checkmake recognizes those aliases; modify the .PHONY line that currently lists targets to include all and test (or add standalone phony target definitions for all and test that run build/tests respectively).
66-69:⚠️ Potential issue | 🟠 Major
run-backfill-embeddingsshould fail fast when env sourcing fails.On Line 68,
go runis executed after;, so sourcing failures can be masked and produce misleading follow-on errors.Suggested patch
run-backfill-embeddings: - @(set -a && [ -f .env ] && . ./.env && set +a; go run ./cmd/backfill-embeddings) + `@set` -e; \ + if [ -f .env ]; then set -a; . ./.env; set +a; fi; \ + go run ./cmd/backfill-embeddings🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 66 - 69, The Makefile target run-backfill-embeddings can mask env sourcing failures because the go run is executed after a semicolon; change the command so go run only runs if .env is found and successfully sourced. Update the recipe for run-backfill-embeddings (the shell line that currently uses set -a && [ -f .env ] && . ./.env && set +a; go run ./cmd/backfill-embeddings) to use && before go run (e.g., wrap the sourcing in the same && chain or add an explicit failure when .env is missing) so sourcing failures cause the make target to fail fast and prevent misleading follow-on errors.openapi.yaml (1)
576-580:⚠️ Potential issue | 🟡 MinorEnforce the documented
offsetcap in both search endpoint schemas.Both descriptions say offset is capped at 1000, but the schema does not enforce it.
Proposed fix
- name: offset in: query description: Number of results to skip (OFFSET-based paging). Ignored if cursor is set. Capped at 1000 for performance. schema: type: integer minimum: 0 + maximum: 1000 default: 0 @@ - name: offset in: query description: Number of results to skip (OFFSET-based paging). Ignored if cursor is set. Capped at 1000 for performance. schema: type: integer minimum: 0 + maximum: 1000 default: 0Also applies to: 668-672
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openapi.yaml` around lines 576 - 580, The OpenAPI schemas declare that the query parameter named "offset" is capped at 1000 in the description but the schema lacks enforcement; update the two "offset" parameter schemas (the integer schema blocks for the search endpoints) to include "maximum: 1000" so the OpenAPI validation enforces the documented cap while keeping "minimum: 0" and "default: 0" unchanged.internal/models/feedback_records.go (1)
105-105:⚠️ Potential issue | 🟡 MinorFix
SubmissionIDcomment to match the non-null invariant.The inline note still mentions DB NULL exposure, which conflicts with the current mandatory/non-null contract.
Proposed fix
- SubmissionID string `json:"submission_id"` // mandatory; never null (DB NULL is exposed as "") + SubmissionID string `json:"submission_id"` // mandatory; never null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/models/feedback_records.go` at line 105, The comment for the SubmissionID field is outdated and mentions DB NULL exposure; update the inline comment on the SubmissionID struct field to state that SubmissionID is mandatory and never null (reflecting the non-null invariant) and remove the note about DB NULL being exposed as an empty string; target the SubmissionID field declaration in the feedback records struct (SubmissionID string `json:"submission_id"`) and make the comment concise and accurate about non-null/must be present.internal/api/handlers/search_handler.go (1)
253-266:⚠️ Potential issue | 🟡 MinorMalformed
min_scoreshould fall back to default, not0.Invalid numeric input currently drops filtering to
0, which broadens results unexpectedly.Proposed fix
func parseMinScore(s string) float64 { if s == "" { return defaultMinScore } val, err := strconv.ParseFloat(s, 64) if err != nil { - return 0 + return defaultMinScore } if val < 0 { return 0 } return math.Min(val, 1) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/handlers/search_handler.go` around lines 253 - 266, The parseMinScore function currently returns 0 on parse errors or out-of-range values; change it to return defaultMinScore instead when input is malformed. In parseMinScore, when strconv.ParseFloat returns an error or the parsed val is outside [0,1] (val < 0 or val > 1), return defaultMinScore; only return the parsed val when it is within the valid range. Ensure you reference the existing parseMinScore function and defaultMinScore constant when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/config/config.go`:
- Around line 99-100: The doc comment for the exported function GetEnvAsBool is
incorrect: it claims unrecognized values map to false but the implementation
returns the provided defaultValue; update the comment to accurately describe
behavior (accepted true values "true", "1", "yes" case-insensitive => true;
accepted false values "false", "0", "no" => false; any other or missing value =>
returns the supplied defaultValue) so callers are not misled; reference the
GetEnvAsBool function in the comment and keep wording concise.
In `@internal/service/embedding_provider_test.go`:
- Around line 196-211: Add tests to TestEmbeddingPrefixForProvider that assert
EmbeddingPrefixForProvider returns "passage: " for the full E5 model identifiers
"intfloat/multilingual-e5-large" and "intfloat/e5-large" (and their uppercase
variants if you want case-insensitivity coverage). Update the
TestEmbeddingPrefixForProvider function to include t.Run subtests calling
EmbeddingPrefixForProvider with those exact strings and using assert.Equal(t,
"passage: ", ...) to verify behavior alongside the existing "e5" case.
In `@internal/service/embedding_provider.go`:
- Around line 196-210: Update the EmbeddingPrefixForProvider function: change
the "mistral" case to return an empty string (remove "search_document: ") and
adjust the function comment to no longer list Mistral as requiring a prefix;
instead document that Mistral uses the API parameter input_type to distinguish
query vs document. Also explicitly list the supported E5 model variants
("intfloat/multilingual-e5-large" and "intfloat/e5-large") in the comment and
keep their case returning "passage: ". Reference: function
EmbeddingPrefixForProvider and the switch cases for "mistral" and the E5
variants.
In `@internal/service/search_service.go`:
- Around line 135-137: The code that generates a next-cursor by accessing last
:= results[len(results)-1] must be guarded against non-positive limits to avoid
a panic when limit == 0; update the logic around the next-cursor generation (the
blocks that check if len(results) == limit and then take
results[len(results)-1], including the second occurrence around lines 197-199)
to first ensure limit > 0 and len(results) > 0 before indexing, and only compute
the last element/next-cursor when those conditions hold.
- Around line 23-25: Update the sentinel error ErrMissingTenantID so its text
uses the service/API terminology "tenant_id" instead of "environment_id"; locate
the ErrMissingTenantID declaration in internal/service/search_service.go and
change the error message string to something like "tenant_id is required" to
keep naming consistent across the codebase.
In `@pkg/embeddings/normalize.go`:
- Around line 24-29: The normalization loop currently divides each element by
magnitude per iteration; precompute the inverse once and multiply instead to
improve performance: compute invMag := 1.0 / magnitude (as float64), then in the
loop replace vector[i] = float32(float64(vector[i]) / magnitude) with vector[i]
= float32(float64(vector[i]) * invMag). Update references in the normalization
function where magnitude and vector are used to use invMag in the for loop.
---
Duplicate comments:
In `@internal/api/handlers/search_handler.go`:
- Around line 253-266: The parseMinScore function currently returns 0 on parse
errors or out-of-range values; change it to return defaultMinScore instead when
input is malformed. In parseMinScore, when strconv.ParseFloat returns an error
or the parsed val is outside [0,1] (val < 0 or val > 1), return defaultMinScore;
only return the parsed val when it is within the valid range. Ensure you
reference the existing parseMinScore function and defaultMinScore constant when
making the change.
In `@internal/api/response/response.go`:
- Around line 89-95: The camelCase→snake_case conversion in the loop that builds
buf mishandles acronyms (e.g., "TenantID"); change the underscore insertion
condition in that loop to insert an underscore when the current rune is
uppercase and either the previous rune is lowercase OR the next rune is
lowercase (i.e., if i>0 && unicode.IsUpper(r) &&
(unicode.IsLower(rune(fieldPath[i-1])) || (i+1<len &&
unicode.IsLower(rune(fieldPath[i+1]))))). Update the logic around the loop that
writes runes (the code handling fieldPath and buf) to use this condition so
acronyms like "ID" become "id" and "TenantID" becomes "tenant_id".
In `@internal/models/feedback_records.go`:
- Line 105: The comment for the SubmissionID field is outdated and mentions DB
NULL exposure; update the inline comment on the SubmissionID struct field to
state that SubmissionID is mandatory and never null (reflecting the non-null
invariant) and remove the note about DB NULL being exposed as an empty string;
target the SubmissionID field declaration in the feedback records struct
(SubmissionID string `json:"submission_id"`) and make the comment concise and
accurate about non-null/must be present.
In `@Makefile`:
- Line 1: The .PHONY declaration is missing the required phony aliases "all" and
"test" required by checkmake; update the Makefile by adding "all" and "test" to
the .PHONY list (or create phony targets named all and test that delegate to
existing targets like build and tests) so checkmake recognizes those aliases;
modify the .PHONY line that currently lists targets to include all and test (or
add standalone phony target definitions for all and test that run build/tests
respectively).
- Around line 66-69: The Makefile target run-backfill-embeddings can mask env
sourcing failures because the go run is executed after a semicolon; change the
command so go run only runs if .env is found and successfully sourced. Update
the recipe for run-backfill-embeddings (the shell line that currently uses set
-a && [ -f .env ] && . ./.env && set +a; go run ./cmd/backfill-embeddings) to
use && before go run (e.g., wrap the sourcing in the same && chain or add an
explicit failure when .env is missing) so sourcing failures cause the make
target to fail fast and prevent misleading follow-on errors.
In `@openapi.yaml`:
- Around line 576-580: The OpenAPI schemas declare that the query parameter
named "offset" is capped at 1000 in the description but the schema lacks
enforcement; update the two "offset" parameter schemas (the integer schema
blocks for the search endpoints) to include "maximum: 1000" so the OpenAPI
validation enforces the documented cap while keeping "minimum: 0" and "default:
0" unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 57776643-c1e8-4690-8bc7-22ee679f7afb
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (34)
.env.example.github/workflows/api-contract-tests.ymlMakefilecmd/api/app.gocmd/backfill-embeddings/main.gogo.modinternal/api/handlers/feedback_records_handler.gointernal/api/handlers/search_handler.gointernal/api/handlers/search_handler_test.gointernal/api/handlers/webhooks_handler.gointernal/api/response/response.gointernal/api/response/response_test.gointernal/config/config.gointernal/googleai/client.gointernal/models/embeddings.gointernal/models/feedback_records.gointernal/observability/aggregate.gointernal/observability/cache.gointernal/observability/names.gointernal/openai/client.gointernal/repository/embeddings_repository.gointernal/service/embedding_client.gointernal/service/embedding_provider.gointernal/service/embedding_provider_test.gointernal/service/feedback_records_service.gointernal/service/search_cursor.gointernal/service/search_service.gointernal/service/search_service_test.gointernal/workers/feedback_embedding.gomigrations/005_embeddings_indexes_and_feedback_tenant_required.sqlopenapi.yamlpkg/embeddings/normalize.gopkg/embeddings/normalize_test.gotests/integration_test.go
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
3 participants
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.
Adds semantic search and similar feedback endpoints, and improves embedding configuration and robustness.
Semantic search & similar feedback
Both endpoints require embeddings to be configured (EMBEDDING_PROVIDER + EMBEDDING_MODEL). When disabled, they return 503 Service Unavailable. Features:
hasMoreflag so pagination works correctly when the repo over-fetches and filters byminScoreEmbedding configuration changes
EmbeddingPrefixForProviderfuture proofing support for other embedding providersBackfill-embeddings
FeedbackEmbeddingWorkerfor job processing.env; fails fast if missingmake run-backfill-embeddingstargetOther changes
response.JSONDecodeErrorDetailfor clearer 400 messages on invalid request bodiesmaximum: 1000for search offset; search endpoints documentedCREATE INDEX CONCURRENTLY/DROP INDEX CONCURRENTLYforidx_embeddings_model(no write blocking)invMagand multiply instead of divide in L2 normalization loopHow should this be tested?
Unit tests
Integration tests
make dev-setup # or: docker-up, init-db make testsSemantic search (requires embeddings)
.env:make runmake run-backfill-embeddingsAPI contract tests
make schemathesis # with API running; runs search endpoints when embeddings enabledChecklist
Required
make buildmake tests(integration tests)make fmtandmake lint; no new warningsmake migrate-validateAppreciated
make tests-coveragefor meaningful logic changes