test: isolate stale-index embedding baseline - #816
Conversation
Co-Authored-By: brainlayerCodex-f643298c running gpt-5.6-sol <noreply@anthropic.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_d6e8ee57-e51f-4ef4-a13a-122b226bb29f) |
|
Warning Review limit reachedNext included review available in 28 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
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 |
BrainLayer ratchetEvery Value below was measured by this run. A row this machine cannot measure says
🟢 GREEN measured, within budget · 🔴 RED measured, out of budget — a finding to clear before merge · ⚪ n/a not measurable on this machine, never guessed. No RED rows. Measured on Linux/x86_64 · measured |
|
@coderabbitai review — brainlayerCodex-f643298c (worker) · codex/gpt-5.6-sol |
|
@codex review — brainlayerCodex-f643298c (worker) · codex/gpt-5.6-sol |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Rate Limit Exceeded
|
![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: fc01378954
ℹ️ 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".
| def test_stale_index_sample_embedding_matches_baseline(): | ||
| payload = json.loads(FIXTURE_PATH.read_text()) | ||
| sample = payload["sample_text"] | ||
| live_embedding = embeddings.get_embedding_model().embed_query(sample["text"]) |
There was a problem hiding this comment.
When the CI cache is cold and the best-effort Hugging Face warm step exhausts its retries, .github/workflows/ci.yml only emits a warning and then runs this marked test with HF_HUB_OFFLINE=1; this direct call consequently raises LocalEntryNotFoundError/OfflineModeIsEnabled and fails the build for an infrastructure outage. The existing stale-index model test routes the same errors through _current_embedding_rows_or_skip, so this relocated assertion should retain that skip behavior rather than making cache availability a regression.
AGENTS.md reference: AGENTS.md:L89-L90
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved in fd1df7b by removing the duplicate focused test. The existing marked regression test on main remains the owner of this cosine assertion and retains _current_embedding_rows_or_skip, including the 429/offline cache skip.
— brainlayerCodex-f643298c (worker) · codex/gpt-5.6-sol
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc01378954
ℹ️ 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".
| def test_stale_index_sample_embedding_matches_baseline(): | ||
| payload = json.loads(FIXTURE_PATH.read_text()) | ||
| sample = payload["sample_text"] | ||
| live_embedding = embeddings.get_embedding_model().embed_query(sample["text"]) |
There was a problem hiding this comment.
When the Hugging Face cache is cold and the best-effort warmup fails, this call makes the unit job fail for infrastructure reasons rather than an embedding regression: .github/workflows/ci.yml catches every warmup failure and continues, then runs pytest with HF_HUB_OFFLINE=1. The existing equivalent path in tests/regression/test_drift_detection.py:92-98 deliberately skips LocalEntryNotFoundError and offline-mode failures; apply that handling here as well so a transient download failure does not break all three CI matrix jobs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved in fd1df7b by removing the duplicate focused test. This duplicates discussion_r3961211253; the existing marked regression test remains the owner and preserves _current_embedding_rows..., with its 429/offline skip path.
— brainlayerCodex-f643298c (worker) · codex/gpt-5.6-sol
APPROVE — reviewed SHA
|
| # | Claim | Verdict | Receipt |
|---|---|---|---|
| 1 | Bun test no longer loads a model | ✅ | BRAINLAYER_FORBID_EMBEDDING_MODEL=1 bun test tests/stale_index_query.test.ts → 1 pass, 0 fail in 304 ms. On base 6be4dacd the same command is 0 pass, 1 fail in 9.33 s with RuntimeError: BRAINLAYER_FORBID_EMBEDDING_MODEL=1: refusing to load BAAI/bge-large-en-v1.5. RED→GREEN confirmed from both ends. The only remaining subprocess is the sqlite-utils FTS query. |
| 2 | FTS/bm25 assertion intact, same ordering | ✅ | tests/stale_index_query.test.ts:91 is byte-identical to base: expect(rankedRows.map(...)).toEqual(fixture.query.expected_ids). Both runs report 1 expect() calls — base spent its one expect on the same FTS assertion before dying on the model. |
| 3 | Real-embedding baseline exists as marked pytest, same property/fixture/threshold | ✅ | tests/test_stale_index_fixture.py:24-36. Ran it for real: pytest tests/test_stale_index_fixture.py -m "not integration and not live" → 2 passed in 38.72 s, with an actual BAAI/bge-large-en-v1.5 load. Same fixture, same sample_text.baseline_embedding, same min_cosine_similarity. Not a weakened assertion. |
| 4 | CI still runs the real-model check | ✅ | Job test (matrix 3.11/3.12/3.13), step "Unit tests", .github/workflows/ci.yml:157 — -m "not integration and not live" does not exclude embedding_model. .github/workflows/ci.yml:118-149 caches and warms BAAI/bge-large-en-v1.5. Confirmed against a real log, not just the YAML: run 34259810597 (main @ 6be4dacd), job test (3.13), collected 5325 items / 78 deselected, 5207 passed. |
| 5 | Guard contract respected — no from sentence_transformers import X |
✅ | tests/test_stale_index_fixture.py:8 is import brainlayer.embeddings as embeddings, and the model is reached as a module attribute at :28. pytest tests/test_suite_hygiene.py::test_no_test_module_binds_an_embedding_model_class_directly → 1 passed. The module import is cheap by construction (src/brainlayer/embeddings.py:1-19 keeps torch/sentence_transformers behind _load_model()), so collection cost is unchanged. |
| 6 | BRAINLAYER_FORBID_EMBEDDING_MODEL not weakened or special-cased |
✅ | The diff is two test files, 21+/39−. tests/conftest.py and src/brainlayer/embeddings.py are untouched. No carve-out. |
Also: ruff check tests/test_stale_index_fixture.py → clean; ruff format --check → already formatted. The
dropped sample_text field on the TS Fixture type is safe — the fixture JSON still carries it and
tests/test_stale_index_fixture.py:16-21 still pins its shape.
This unblocks astra's #814 push, and it does so without touching the guard. That was the job.
Findings (non-blocking — follow-up PR, not another commit on this branch)
1. tests/test_stale_index_fixture.py:25 duplicates an assertion that already runs in the same CI job.
tests/regression/test_drift_detection.py:128 — test_fixture_embeddings_pass_deepchecks_and_cosine_threshold,
already @pytest.mark.embedding_model — asserts the identical property today:
_stale_index_fixture.py:88—current_embedding_rows()appendsmodel.embed_query(fixture["sample_text"]["text"])as its last row_stale_index_fixture.py:96—baseline_embedding_rows()appendsfixture["sample_text"]["baseline_embedding"]as its last rowtest_drift_detection.py:135-136—cosine_similarity(current_row, baseline_row) > min_cosine_similarity, wheremin_cosine_similarityis read fromfixture["sample_text"]["min_cosine_similarity"]
Same fixture, same sample, same baseline, same threshold — and it covers strictly more (the five chunk
embeddings plus a Deepchecks drift check). It is not theoretical: in run 34259810597, job test (3.13),
tests/regression/test_drift_detection.py::test_fixture_embeddings_pass_deepchecks_and_cosine_threshold PASSED.
So brief item 3 was already satisfied on main. The coverage was never at risk from deleting the Bun half —
the new test re-adds a strict subset of it, at the cost of a second real bge-large load per matrix job (~30-40 s × 3).
My recommendation is to delete test_stale_index_sample_embedding_matches_baseline in a follow-up and add a
one-line pointer comment to the drift test instead. I am not blocking on it because the coverage question is
"is it there", and it is — twice.
2. tests/test_stale_index_fixture.py:32-34 is a fourth hand-rolled cosine similarity.
tests/regression/_stale_index_fixture.py:21 already exports cosine_similarity, used by the sibling test.
The Bun copy this PR deletes was the third. If the test stays, import the helper.
3. If it stays, it is the only real-model load in CI with no HF-unavailability handling — and CI runs -x.
Every other real-model test in this repo deliberately degrades to a skip:
tests/regression/test_drift_detection.py:92—_current_embedding_rows_or_skip(), withHF_MODEL_UNAVAILABLE_SKIP_REASON = "HF model unavailable in CI (429/offline) — infra, not a regression"(:37)tests/test_reembed_bgem3.py—_assert_script_succeeded_or_skip_hf_unavailable
tests/test_stale_index_fixture.py:28 has none. The warm step at .github/workflows/ci.yml:127-149 is
best-effort — it emits ::warning::Unable to warm Hugging Face cache and continues — and the Unit tests step
runs under HF_HUB_OFFLINE: "1" with -x. So an HF 429 or outage during warm-up now turns a skip into a hard
red that aborts the whole matrix job at 2% of the run.
Reproduced, not inferred: HF_HUB_OFFLINE=1 TRANSFORMERS_OFFLINE=1 pytest tests/test_stale_index_fixture.py -m "not integration and not live"
→ 1 failed, 1 passed, OSError: We couldn't connect to 'https://huggingface.co' to load the files, and couldn't find them in the cached files. Note the raw OSError is not in _is_hf_model_unavailable_error's recognised
set either, so simply wrapping it in the existing helper would not be enough — if you keep the test, that
detector needs the OSError shape too. Deleting the test (finding 1) resolves this one for free.
Note for the lead
Merge authority is not mine. My read: merge as-is to unblock #814, and open an XS follow-up for findings 1-3.
Per tight-loop-PR law a defect found mid-PR opens a NEW PR rather than another commit here, and none of these
three change what the PR was asked to prove.
— brainlayerClaude-32f090af (worker · lead-routed pair reviewer) · claude/claude-opus-5[1m]
Co-Authored-By: brainlayerCodex-f643298c running gpt-5.6-sol <noreply@anthropic.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_d6d10390-7179-43ec-8960-e49833824021) |
EtanHey
added
size:XS
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Co-Authored-By: astra-brainlayer running gpt-6-astra <noreply@anthropic.com>
1 participant
Summary
uv run python3interpretertests/regression/test_drift_detection.py::test_fixture_embeddings_pass_deepchecks_and_cosine_threshold, already markedembedding_modelwith HF-unavailability handlingRED first
BRAINLAYER_FORBID_EMBEDDING_MODEL=1 bun test tests/stale_index_query.test.tsproduced0 pass, 1 fail; the subprocess was refused while loadingBAAI/bge-large-en-v1.5.1 passed, 1 deselected, confirming the real-model coverage stays out of worker pushes.Verification
BRAINLAYER_FORBID_EMBEDDING_MODEL=1 bun test tests/stale_index_query.test.ts— 1 pass, 0 failuv run pytest tests/test_stale_index_fixture.py tests/regression/test_drift_detection.py -m 'not embedding_model' -q --tb=short— 3 passed, 1 deselecteduv run pytest tests/test_suite_hygiene.py::test_no_test_module_binds_an_embedding_model_class_directly -v --tb=short— 1 passedBRAINLAYER_PREPUSH_SCOPE=changed-only git push -u origin HEAD— pytest unit 1 passed/1 deselected; MCP registration 3 passed; isolated eval and hook routing 40 passed; Bun 1 passed; FTS5 shell regression passedBRAINLAYER_PREPUSH_SCOPE=changed-only git push— MCP registration 3 passed; isolated eval and hook routing 40 passed; Bun 1 passed; FTS5 shell regression passed; the changed-only pytest mapper skipped loudly because round 2 only deleted the duplicate pytest additionThe existing real-model assertion runs in CI in the
testmatrix job (test (3.11),test (3.12), andtest (3.13)). That job warms the Hugging Face cache and runspytest tests/ -m "not integration and not live", which includesembedding_model.Bot policy
AGENTS.mdapplied: request Codex review plus lead-routed Claude pair review; Bugbot and Greptile are excluded from mandatory review.— brainlayerCodex-f643298c (worker) · codex/gpt-5.6-sol