Pplx embed context - #132
Conversation
![hyperspace-pr-bot[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR introduces a new embedding model (pplx-embed), a disk-backed embedding cache, and a full deterministic RAG evaluation harness with HTML/Markdown reporting. The implementation is solid and well-tested overall, but there are several correctness issues to address: a Float32Array alignment bug in the embedding cache that can make it permanently unreadable, HTML injection risks from unescaped &/>/" characters and unescaped doc ids in the compare report, a misdiagnosis in diagnose() that falsely reports a ranking regression whenever recall drops, and a persistent typo ("anker" → "anchor") in the synthetic chunk id scheme that would be hard to migrate once baked into stored run data.
PR Bot Information
Version: 1.29.18
- Correlation ID:
11c401d0-92ff-11f1-87b1-6502bb781a84 - LLM:
anthropic--claude-4.6-sonnet - File Content Strategy: Full file content
- Event Trigger:
pull_request.opened
|
|
||
| const causes = [] | ||
| if (down(recall)) causes.push('recall_down → chunking/embedding regression') | ||
| if (down(mrrD) || down(ndcg)) causes.push('recall_stable_mrr_down → ranking/scoring regression') |
There was a problem hiding this comment.
Logic Error: The diagnose function conflates "recall stable + MRR down" with "recall down + MRR down" for the ranking-regression cause. When recall drops (which implies MRR also drops), the label recall_stable_mrr_down is incorrectly included even though recall is not stable. This produces misleading combined diagnoses like "recall_down → chunking/embedding regression; recall_stable_mrr_down → ranking/scoring regression" for a pure recall regression.
The ranking cause should only fire when recall has not dropped past the dead-band:
if (!down(recall) && (down(mrrD) || down(ndcg))) causes.push('recall_stable_mrr_down → ranking/scoring regression')| if (down(mrrD) || down(ndcg)) causes.push('recall_stable_mrr_down → ranking/scoring regression') | |
| if (!down(recall) && (down(mrrD) || down(ndcg))) causes.push('recall_stable_mrr_down → ranking/scoring regression') |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| function escHtml(text) { | ||
| return (text || '').replace(/</g, '<') | ||
| } |
There was a problem hiding this comment.
Bug: escHtml only escapes < but leaves >, ", and & unescaped. In renderRunDetails, values such as r.config.label, r.run_id, and r.diagnosis are interpolated directly into HTML attributes and text content without going through escHtml. For example, a run label containing & produces invalid HTML, and one containing " can break out of a title="" attribute.
At minimum & must be escaped first (before <), and > and " should also be handled:
function escHtml(text) {
return (text || '')
.replace(/&/g, '&')
.replace(/</g, '<')
.replace(/>/g, '>')
.replace(/"/g, '"')
}| function escHtml(text) { | |
| return (text || '').replace(/</g, '<') | |
| } | |
| function escHtml(text) { | |
| return (text || '') | |
| .replace(/&/g, '&') | |
| .replace(/</g, '<') | |
| .replace(/>/g, '>') | |
| .replace(/"/g, '"') | |
| } |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| ${(r.per_question).map(q => { | ||
| const relevant = new Set(q.relevant_doc_ids || []) | ||
| const question = escHtml(q.question) | ||
| const relList = (q.relevant_doc_ids || []).map(id => `<li class="mono">${id}</li>`).join('') |
There was a problem hiding this comment.
Bug: relList renders each relevant doc id directly into HTML (<li class="mono">${id}</li>) without escaping it. Although current golden-set ids are plain URLs, any future id containing < (e.g. a generated capire:// id from a chunk whose breadcrumb text includes angle brackets) would inject raw HTML into the report.
Should use escHtml:
const relList = (q.relevant_doc_ids || []).map(id => `<li class="mono">${escHtml(id)}</li>`).join('')| const relList = (q.relevant_doc_ids || []).map(id => `<li class="mono">${id}</li>`).join('') | |
| const relList = (q.relevant_doc_ids || []).map(id => `<li class="mono">${escHtml(id)}</li>`).join('') |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| if (text) { | ||
| return `<li class="${hit ? 'hit' : 'miss'}"><details class="chunk"><summary class="mono">${marker}</summary><pre class="chunk-text">${escHtml(text)}</pre></details></li>` | ||
| } | ||
| return `<li class="mono ${hit ? 'hit' : 'miss'}">${marker} <span class="muted">(text unavailable)</span></li>` |
There was a problem hiding this comment.
Bug: The retrieved doc id (and the composed marker string containing it) is interpolated into HTML without escaping. A doc id that contains < or > — possible with synthesized capire://generated/ ids whose slug comes from arbitrary breadcrumb text — would produce broken or injectable HTML in both the <summary> and <li> branches.
Should escape marker (and id separately for the non-expandable branch):
const markerEsc = escHtml(marker)
if (text) {
return `<li class="${hit ? 'hit' : 'miss'}"><details class="chunk"><summary class="mono">${markerEsc}</summary><pre class="chunk-text">${escHtml(text)}</pre></details></li>`
}
return `<li class="mono ${hit ? 'hit' : 'miss'}">${markerEsc} <span class="muted">(text unavailable)</span></li>`| if (text) { | |
| return `<li class="${hit ? 'hit' : 'miss'}"><details class="chunk"><summary class="mono">${marker}</summary><pre class="chunk-text">${escHtml(text)}</pre></details></li>` | |
| } | |
| return `<li class="mono ${hit ? 'hit' : 'miss'}">${marker} <span class="muted">(text unavailable)</span></li>` | |
| const markerEsc = escHtml(marker) | |
| if (text) { | |
| return `<li class="${hit ? 'hit' : 'miss'}"><details class="chunk"><summary class="mono">${markerEsc}</summary><pre class="chunk-text">${escHtml(text)}</pre></details></li>` | |
| } | |
| return `<li class="mono ${hit ? 'hit' : 'miss'}">${markerEsc} <span class="muted">(text unavailable)</span></li>` |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| assert.ok(r.aggregate.recall_at_k.delta < 0) | ||
| assert.match(r.diagnosis, /chunking\/embedding regression/) | ||
| assert.deepEqual(r.gated_failures.sort(), ['hit_rate_at_k', 'mrr', 'recall_at_k']) |
There was a problem hiding this comment.
Logic Error: The test asserts that recall_stable_mrr_down is not diagnosed when both recall and MRR drop (a pure recall regression), which is the intended behaviour. However, with the current implementation in runner.js, diagnose fires the ranking cause whenever MRR drops regardless of whether recall also dropped. This means the test at line 153 currently passes the chunking/embedding assertion but would also see recall_stable_mrr_down in the output — the test does not assert its absence, so the misdiagnosis goes undetected.
Consider adding an explicit assertion that the ranking cause is absent for a pure recall regression:
assert.ok(!r.diagnosis.includes('recall_stable_mrr_down'), 'should not report ranking regression when recall also drops')| assert.ok(r.aggregate.recall_at_k.delta < 0) | |
| assert.match(r.diagnosis, /chunking\/embedding regression/) | |
| assert.deepEqual(r.gated_failures.sort(), ['hit_rate_at_k', 'mrr', 'recall_at_k']) | |
| assert.ok(r.aggregate.recall_at_k.delta < 0) | |
| assert.match(r.diagnosis, /chunking\/embedding regression/) | |
| assert.ok(!r.diagnosis.includes('recall_stable_mrr_down'), 'should not report ranking regression when recall also drops') | |
| assert.deepEqual(r.gated_failures.sort(), ['hit_rate_at_k', 'mrr', 'recall_at_k']) |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| yield [url, text] | ||
| } else if (prevUrl) { | ||
| contN += 1 | ||
| yield [`${prevUrl.split('#')[0]}#generated-anker-${contN}`, text] |
There was a problem hiding this comment.
Typo: #generated-anker-N is a misspelling of "anchor".
This synthetic id is written to result.jsonl and used as a ground-truth key, so the typo becomes part of the persistent file format. Fixing it later would invalidate all stored run data that used these generated ids.
Should be #generated-anchor-N.
| yield [`${prevUrl.split('#')[0]}#generated-anker-${contN}`, text] | |
| yield [`${prevUrl.split('#')[0]}#generated-anchor-${contN}`, text] |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| const header = JSON.parse(buf.toString('utf-8', 0, nl)) | ||
| const { dim, keys } = header | ||
| let offset = nl + 1 | ||
| const floats = new Float32Array(buf.buffer, buf.byteOffset + offset, (buf.length - offset) / 4) |
There was a problem hiding this comment.
Bug: loadEmbeddingCache builds a Float32Array typed-array view over the raw Buffer without guaranteeing 4-byte alignment. Float32Array requires that byteOffset be a multiple of 4; if the JSON header + newline length is not divisible by 4, the constructor throws RangeError: start offset of Float32Array should be a multiple of 4, making the entire cache permanently unreadable.
Use buf.buffer.slice(...) to create a fresh, correctly-aligned ArrayBuffer:
const floats = new Float32Array(buf.buffer.slice(buf.byteOffset + offset))| const floats = new Float32Array(buf.buffer, buf.byteOffset + offset, (buf.length - offset) / 4) | |
| const floats = new Float32Array(buf.buffer.slice(buf.byteOffset + offset)) |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
0818296 to
4751107
Compare
= evals-mcp-server + only the model change: - calculateEmbeddings.js: default MODEL_NAME to perplexity-ai/pplx-embed-context-v1-0.6b, per-model MODEL_DIR, download the external-data ONNX sidecars, and load via onnxruntime-node (onnxruntime-web can't resolve sidecar files in Node.js). - index.js: reflect the pplx default in help text. - package.json: declare onnxruntime-node. Golden set and eval harness are identical to the base branch.
afee9c5 to
c4fefe4
Compare
1 participant
Add Perplexity Embed Context Model Support with Retrieval Eval Harness
New Features
✨ This PR introduces two major additions:
Switch the default embedding model from
Xenova/all-MiniLM-L6-v2toperplexity-ai/pplx-embed-context-v1-0.6b, with full support for models that use external ONNX weight sidecar files (model.onnx_data). The model is now configurable viaCDS_MCP_MODEL.A complete deterministic RAG retrieval eval harness (
evals/) that scores thesearch_docstool against a frozen human-authored golden set, computes standard IR metrics, and renders HTML/Markdown comparison dashboards.Changes
lib/calculateEmbeddings.js: Changed default model toperplexity-ai/pplx-embed-context-v1-0.6bviaCDS_MCP_MODELenv var. Each model now gets its own subdirectory undermodels/. Added support for downloading and loading ONNX external data sidecar files (model.onnx_data), usingonnxruntime-nodefor models with external data files.lib/embeddings.js: Added a disk-backed embedding cache keyed by(model, chunk-text SHA-256 hash), so re-runs only recompute embeddings for changed chunks. Added progress logging with elapsed time and ETA (configurable viaEMBEDDINGS_LOG_EVERY; suppress with=0).index.js: AddedCDS_MCP_MODELto the help text.evals/config.json: Central configuration for the eval harness (K, gates, paths, output format).evals/data/golden-set.json: Frozen golden set of 10 CAP-domain questions with human-authored relevance labels.evals/lib/metrics.js: Pure-arithmetic implementations of Recall@K, Precision@K, MRR, Hit-Rate@K, and nDCG@K.evals/lib/runner.js: Core eval logic — builds reports, computes deltas vs. baseline, diagnoses regressions (chunking/embedding vs. ranking), validates the golden set.evals/lib/cli.js: Orchestration entry point (run()andrunAll()), wires retriever, golden set, baseline, and store together.evals/lib/compare.js: Generates a self-contained HTML dashboard (line charts, leaderboard, per-question searchable table with lazy charts) or a Markdown report.evals/lib/store.js: Append-only JSONL result store with run capping and chronological sorting.evals/lib/config.js: Config loader withEVAL_*env overrides and programmatic override support.evals/lib/ids.js: Pure string parsing ofSource:URLs from chunks for stable doc identity; fallback tocapire://generated/slugs.evals/lib/retriever.js: Wrapssearch_docsand resolves returned chunk text to corpus-consistent doc IDs.evals/docs/README.md/evals/docs/METRICS.md: Full documentation for running the harness and understanding each metric.evals/bin/eval.js/evals/bin/compare.js: Thin CLI entry points fornpm run evalsandnpm run evals:compare.package.json: Addedevals,evals:compare, andevals:testnpm scripts.tests/embedding-cache.test.js/tests/embedding-progress.test.js: Tests for the new embedding cache and progress logging.evals/tests/unit/*.test.js: Comprehensive unit tests for all eval harness components (metrics, runner, CLI, store, compare, config, IDs, retriever)..gitignore: Addedevals/runs/to ignore transient eval output.PR Bot Information
Version:
1.29.18anthropic--claude-4.6-sonnetpull_request.opened11c401d0-92ff-11f1-87b1-6502bb781a84