Repository navigation
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00c27f7484
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| func (c *cachedEmbedder) key(ctx context.Context, text string) string { | ||
| tenantID, _ := types.TenantIDFromContext(ctx) | ||
| digest := sha256.Sum256([]byte(strconv.FormatUint(tenantID, 10) + "\x00" + c.namespace + "\x00" + text)) | ||
| return hex.EncodeToString(digest[:]) |
There was a problem hiding this comment.
When an NVIDIA embedder is used, BatchEmbed selects input_type from types.EmbedQueryContextKey (internal/models/embedding/nvidia.go:157-160), meaning identical text can legitimately produce different query and passage vectors. This cache key ignores that mode, so after a string is cached during document ingestion, embedding the same string as a query can return the passage vector and corrupt retrieval and evaluation results; include the effective embedding mode in the key.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 437dff4: the embedding cache key now includes the effective query/passage mode, so identical text cannot reuse vectors across query and passage contexts. Added a regression test covering the mode boundary.
| if vector, ok := vectors[key]; ok { | ||
| c.store.put(key, vector, c.now().Add(c.options.ttl), c.options.maxEntries) | ||
| c.observeCache(ctx, 1, 1, 0, 0) |
There was a problem hiding this comment.
When a durable entry is loaded shortly before its database expires_at, this resets its in-memory expiry to now + configured TTL rather than retaining the remaining lifetime. For example, an entry with one minute left can remain usable for another 24 hours after a process restart, violating the configured TTL and serving stale vectors; the persistent cache result needs to carry the stored expiry into the LRU.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 437dff4: persistent cache reads now return the stored expires_at value, and L1 warming preserves that deadline instead of extending it by a fresh TTL. Added an expiry regression test.
| ) { | ||
| extraConfig.thinking_control = modelData.thinkingControl | ||
| } | ||
| if (saveType === 'chat' && modelData.pricingEnabled) { |
There was a problem hiding this comment.
When pricing is switched off for a local chat model, this branch emits no pricing fields and the otherwise empty extraConfig is omitted. UpdateModel then preserves the stored map when ExtraConfig is nil (internal/handler/model.go:635-637), so the old pricing_enabled=true and prices remain active despite the UI showing the switch as disabled; serialize pricing_enabled=false or otherwise explicitly clear the saved pricing keys.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 437dff4: chat model updates now always serialize pricing_enabled, including false, so disabling pricing clears the prior enabled state. Added a frontend regression test.
| // Start evaluation in background goroutine | ||
| logger.Info(ctx, "Starting evaluation in background") | ||
| go func() { |
There was a problem hiding this comment.
If the server exits after the task is registered and before this goroutine performs its terminal update, the work is lost while the durable row remains pending or running. There is no startup replay or stale-run reconciliation elsewhere in the repository, so these runs remain permanently in progress in history and can never produce final evidence; persisted nonterminal runs need to be resumed or marked failed during startup.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 437dff4: service startup now reconciles durable pending/running evaluations whose workers were lost, marking them failed with a finished timestamp, duration, and explicit restart message. The test covers pending/running records and confirms terminal records remain unchanged.
| config := struct { | ||
| Name string `json:"name"` | ||
| Type types.ModelType `json:"type"` | ||
| Source types.ModelSource `json:"source"` | ||
| Provider string `json:"provider"` |
There was a problem hiding this comment.
This fingerprint whitelist omits behavior-affecting model parameters such as BaseURL and ExtraConfig (including provider request controls such as thinking_control). Editing a model in place to point at another deployment or change its request mapping can therefore retain the same config_fingerprint, causing aggregateRepeatedRuns and comparison checks to group materially different runs as equivalent; hash all non-secret effective settings rather than this subset.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 437dff4: model fingerprints now include BaseURL and non-secret behavior/runtime settings (including ExtraConfig), while credential-like keys remain excluded. Regression tests cover behavior changes and API key/token variants.
0e20978 to
437dff4
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 437dff4872
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) interfaces.EvaluationService { | ||
| evaluationMemoryStorage := newEvaluationMemoryStorage() | ||
| storage := newEvaluationStorage(db) | ||
| if recovered, err := storage.reconcileInterrupted(context.Background(), time.Now().UTC()); err != nil { |
There was a problem hiding this comment.
In a multi-replica deployment sharing the database, constructing the service on any new or restarted replica invokes this global reconciliation and marks every pending/running evaluation as failed, including work still executing normally on another replica. A scale-up or rolling deployment can therefore publish false failures until the original worker writes again; reconciliation needs worker ownership/leases or a stale-heartbeat predicate rather than closing all nonterminal rows.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6cb578c. Evaluation rows now carry an owner ID and heartbeat; startup reconciliation only transitions stale leases (with a conservative legacy-row window), and the conditional update rechecks staleness to avoid racing an active replica. Covered by TestEvaluationStorageReconcilesWorkersLostOnRestart.
| config.BaseURL, | ||
| strconv.Itoa(config.Dimensions), | ||
| strconv.Itoa(config.TruncatePromptTokens), | ||
| strconv.FormatBool(config.SupportsDimensionOverride), | ||
| }, "\x00") |
There was a problem hiding this comment.
When an embedding model is edited without changing these whitelisted fields, behavior-affecting settings such as ExtraConfig["remote_model_name"], Azure api_version, and custom routing headers leave the namespace unchanged. Both the LRU and durable tier can then return vectors produced by the old deployment while uncached inputs use the new one, mixing incompatible embedding spaces and corrupting retrieval; hash all non-secret provider-relevant settings into the namespace.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6cb578c. The embedding-cache namespace now deterministically includes MaxConcurrency plus all non-secret ExtraConfig and custom-header controls, while filtering credential-bearing keys. Added a regression test for routing changes and secret rotation.
| extraConfig.embedding_cache_enabled = String(modelData.embeddingCacheEnabled !== false) | ||
| extraConfig.embedding_cache_ttl_seconds = String(Math.max(1, Number(modelData.embeddingCacheTTLSeconds) || 86400)) | ||
| extraConfig.embedding_cache_max_entries = String(Math.max(1, Number(modelData.embeddingCacheMaxEntries) || 10000)) |
There was a problem hiding this comment.
When editing an embedding model that already has provider-specific extra_config such as api_version or remote_model_name, this creates a non-nil map containing only the three cache fields. UpdateModel replaces the stored map whenever ExtraConfig is non-nil (internal/handler/model.go:635-638), so an otherwise unrelated edit silently deletes provider routing settings and can break or redirect subsequent embedding calls; merge these fields into the existing configuration instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6cb578c. Model edits now start from the persisted extra_config map and overlay only fields owned by the form, preserving provider-specific routing and API-version settings. The pricing regression test asserts this merge.
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
If model snapshotting fails here—for example because a submitted model ID was deleted—or the subsequent task registration fails, the evaluation knowledge base created earlier in this method is never handed to EvalDataset, where cleanup is installed. Repeated invalid or stale requests therefore accumulate orphaned evaluation knowledge bases and their backing resources; delete the newly created KB on every pre-worker error path or install cleanup immediately after creation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6cb578c. Temporary evaluation-KB ownership is now explicit: the synchronous caller cleans up every pre-worker error path, and the background worker cleans up on every exit, including status-update failures.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6cb578c1d5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const extraConfig: Record<string, string> = { | ||
| ...(editingModel.value?.parameters?.extra_config || {}), | ||
| } |
There was a problem hiding this comment.
When editing a model with provider-specific settings such as api_version or remote_model_name, convertToLegacyFormat does not copy model.parameters onto the editor row, so editingModel.value?.parameters?.extra_config is always undefined and this starts from an empty map. Saving then replaces the backend map with only the form-owned fields, potentially breaking or redirecting subsequent requests. Fresh evidence after the claimed fix is that the new merge reads a property which the current conversion function never returns.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 41fddde. The legacy editor model now retains the complete non-secret parameters payload, so update saves merge from the persisted extra_config before overlaying form-owned fields. Added a regression assertion in ModelSettings.pricing.test.mjs.
| BaseURL string `json:"base_url"` | ||
| ParameterSize string `json:"parameter_size"` | ||
| ExtraConfig map[string]string `json:"extra_config,omitempty"` | ||
| SupportsVision bool `json:"supports_vision"` |
There was a problem hiding this comment.
When a gateway uses a custom header to select a deployment, model version, or routing policy, changing that header leaves this fingerprint unchanged because the whitelist includes filtered ExtraConfig but omits model.Parameters.CustomHeaders. Runs against materially different backends can consequently be grouped as repeated or controlled comparisons. Fresh evidence after the earlier fingerprint fix is that the expanded current whitelist still has no filtered custom-header field.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 41fddde. Evaluation model fingerprints now include CustomHeaders after applying the same secret-key filter used for extra_config. The regression test proves a non-secret routing header changes the fingerprint while Authorization changes do not.
| // Pass the raw inner model to avoid observing provider-internal sub-batch | ||
| // callbacks twice. | ||
| vectors, err := o.inner.BatchEmbedWithPool(ctx, o.inner, texts) | ||
| o.observe(ctx, startedAt, err) |
There was a problem hiding this comment.
When BatchEmbedWithPool splits a large input according to BATCH_EMBED_SIZE, the pool invokes BatchEmbed once per chunk, but passing o.inner here bypasses this observer for all of those sub-batch calls and emits only one aggregate observation afterward. As a result, evaluation_model_calls and cachebench undercount actual provider requests and report one combined latency instead of the provider-facing calls the telemetry claims to measure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 41fddde. BatchEmbedWithPool now threads the observable wrapper into provider sub-batch callbacks and emits no aggregate duplicate. A new test asserts two provider sub-batches produce exactly two observations.
6cb578c to
41fddde
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41fddde438
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| if kind.BenchmarkID != "" { | ||
| var report types.WikiCacheBenchmarkEvidence | ||
| if err := json.Unmarshal(data, &report); err != nil { |
There was a problem hiding this comment.
Decode evidence with unknown-field rejection before verifying it. Both report paths use json.Unmarshal, which silently discards unrecognized fields, so adding an arbitrary top-level or nested field such as an approval annotation leaves the canonical typed structure unchanged and evidenceverify still reports that the modified file's checksum is valid.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d5e0419: evidenceverify now decodes the selected typed report with DisallowUnknownFields, rejects trailing JSON values, and includes a regression test proving an unchecksummed top-level field is rejected.
| continue | ||
| } | ||
| c.store.put(item.key, entry.Vector, entry.ExpiresAt, c.options.maxEntries) | ||
| hitCount += len(item.indices) |
There was a problem hiding this comment.
When duplicate inputs are absent from L1 but present in the persistent cache, the duplicates have already incremented deduplicatedCount, and this line then counts every duplicate index again as a hit. Consequently AvoidedComputations = hits + deduplicated can exceed LookupCount and the displayed avoided rate can exceed 100%; classify the duplicate positions as either persistent hits or deduplications, not both.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d5e0419: persistent-cache hits are counted once per unique key; duplicate positions remain classified as in-batch deduplication, so avoided computations cannot exceed lookup count. Added a persistent duplicate regression test.
| }{ | ||
| SchemaVersion: config.SchemaVersion, DatasetFingerprint: config.DatasetFingerprint, | ||
| DatasetSamples: config.DatasetSamples, SourceKnowledgeBaseID: config.SourceKnowledgeBaseID, | ||
| Chunking: config.Chunking, Pipeline: pipeline, Models: models, CodeVersion: config.CodeVersion, |
There was a problem hiding this comment.
Hash only behavior-affecting model data here rather than the complete EvaluationModelSnapshot. The snapshots include DisplayName and UpdatedAt, so merely renaming an embedding/rerank model or saving an unchanged row changes controlled_fingerprint, causing aggregateRepeatedRuns to split otherwise identical runs even though the nested model config_fingerprint remains unchanged.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d5e0419: controlled comparison fingerprints now ignore display_name and updated_at while retaining the non-secret model config fingerprint. Added a rename/timestamp regression test.
| for s.lru.Len() > maxEntries { | ||
| s.remove(s.lru.Back()) |
There was a problem hiding this comment.
The store is process-global across every model and tenant, but each insertion trims it using the current model's embedding_cache_max_entries. In a deployment with two embedding models configured for 10,000 and 10 entries, one insertion through the latter evicts the former model's cache down to 10, while a later insertion through the former permits growth again; partition the store by namespace or apply a single process-wide bound.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d5e0419: the shared embedding cache is partitioned into independently bounded namespace LRUs, so a small-cache model cannot evict another model namespace. Added an independent-bound regression test.
41fddde to
b143b8f
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5e0419133
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| storage := newEvaluationStorage(db) | ||
| if recovered, err := storage.reconcileInterrupted(context.Background(), time.Now().UTC()); err != nil { |
There was a problem hiding this comment.
When a worker crashes and its replacement starts within the two-minute heartbeat timeout, this one-time constructor call correctly leaves the recent lease untouched, but no later code calls reconcileInterrupted; after the timeout expires, the evaluation therefore remains pending or running forever. Fresh evidence beyond the earlier reconciliation fix is that a repo-wide search shows this is still the only production invocation, so reconciliation needs a periodic sweep or a delayed follow-up rather than running only at startup.
Useful? React with 👍 / 👎.
| if (base.run_config.dataset_fingerprint !== candidate.run_config.dataset_fingerprint) warnings.add(t('evaluationSettings.datasetDiffers')) | ||
| if (base.run_config.source_knowledge_base_id !== candidate.run_config.source_knowledge_base_id) warnings.add(t('evaluationSettings.knowledgeBaseDiffers')) | ||
| if (stableStringify(base.run_config.chunking) !== stableStringify(candidate.run_config.chunking)) warnings.add(t('evaluationSettings.chunkingDiffers')) | ||
| if (controlledPipeline(base.run_config.pipeline) !== controlledPipeline(candidate.run_config.pipeline)) warnings.add(t('evaluationSettings.pipelineDiffers')) | ||
| if (controlledModelFingerprints(base) !== controlledModelFingerprints(candidate)) warnings.add(t('evaluationSettings.modelDependencyDiffers')) |
There was a problem hiding this comment.
When selected runs come from different code versions but otherwise have the same dataset and settings, these manual checks produce no warning and the UI labels the runs comparable. The backend's controlled_fingerprint already includes code_version and the other controlled inputs, so use that fingerprint (or explicitly compare the version) to avoid presenting results produced by different retrieval or metric implementations as a controlled model comparison.
Useful? React with 👍 / 👎.
| const currencies = new Set(selectedRuns.value.flatMap(run => Object.keys(run.usage?.cost_by_currency || {}))) | ||
| for (const currency of [...currencies].sort()) { | ||
| rows.push({ | ||
| key: `cost.${currency}`, | ||
| label: `${t('evaluationSettings.cost')} (${currency})`, | ||
| values: selectedRuns.value.map(run => { | ||
| const costs = run.usage?.cost_by_currency | ||
| return costs && Object.prototype.hasOwnProperty.call(costs, currency) ? costs[currency] : undefined |
There was a problem hiding this comment.
When a run has both priced and unpriced calls, cost_by_currency contains only the priced subtotal, but this code renders it as the run's cost without checking unpriced_calls. This commonly occurs when chat pricing is configured while embedding or rerank calls remain unpriced, so comparisons can make an incomplete run look cheaper; treat such values as unavailable or visibly partial, as the model-usage and Wiki benchmark views already do.
Useful? React with 👍 / 👎.
Description
This PR turns the existing RAG evaluation endpoint into an auditable quality-and-cost workflow for Topic 3. It adds persistent evaluation history, controlled multi-model comparisons, model-call observability, deterministic cache gates, persistent embedding caching, and a strict Wiki prompt-cache A/B benchmark.
Key changes:
000093-000102after upstream MCP migration000092, with equivalent SQLite migrations000014-000023.The final acceptance report is available at
evaluation/evidence/final-acceptance-2026-09-09.md. It records the frozen implementation commit, non-sensitive fingerprints, exported-report checksums, limitations, and results from real Qwen and Wiki-cache experiments. Raw exports remain local because they contain resource UUIDs; prompts, answers, retrieved content, tenant IDs, and resource UUIDs are not published.Type of Change
Related Issue
Topic 3: quality evaluation baseline and cost observability. No issue is automatically closed by this PR.
Testing
Validated after rebasing the implementation onto upstream commit
fbaff900a09b2543c291fb0545ecfaf361bfaafc:Results:
-lc++linker warnings were emitted.go run ./cmd/evidenceverify -report <exported-report.json>.Real final-version acceptance on implementation commit
5fb873a7660c4ff730493b9655f73bd7edc7f965:qwen3.7-plusand localqwen2:7bused the same dataset, knowledge base, retrieval pipeline, dependency models, and controlled fingerprint; both completed 4/4 samples with 100% retrieval Recall;Upstream advanced by two unrelated Sandbox/chat commits after the evidence was frozen. The only shared paths are locale dictionaries; no Topic 3 keys were changed upstream. The frozen commit is intentionally preserved so exported evidence remains traceable to the exact tested code.
Checklist
git diff --check upstream/main...HEADpasses for the tested baseScreenshots / Recordings
Matched repeated-run summary
Controlled multi-model comparison
Strict Wiki cache A/B
The Wiki screenshot is a separate UI revalidation on the same frozen implementation commit. It again passed byte-identical cold/warm validation and reported a 99.62% warm cache hit rate and 68.29% estimated-cost reduction. Its warm P95 was higher because of remote-service variance, so it is not used to replace the frozen latency result in the acceptance report.