test(version-check): scrub BRAINLAYER_VERSION_CHECK_* from the subprocess env, and pin the mirror sibling case (XS) - #772
Conversation
…cess env, and pin the mirror sibling case tests/test_version_consistency.py ran scripts/brainlayer-version-check.sh with the INHERITED environment. A release operator has BRAINLAYER_VERSION_CHECK_CASK_LAG_REASON exported -- they need it for the real check -- so pre-push -> pytest -> the script saw the reason, allowed the lag, and returned 0. The two tests that assert the script FAILS on a lagging/drifting cask without a stated reason went RED for a change with nothing to do with them. The env is now built from a scrubbed copy in one helper (_script_env, mirroring the sibling tests/test_run_tests_script.py::_script_env) and the scrub is by PREFIX: REPO_ROOT, TAP_ROOT and GIT_TAG would steer the fixtures elsewhere exactly the same way. Tests that WANT a knob still pass it explicitly through extra_env/git_tag, which override the base. Also folds in the mirror case the #762 round-1 review left uncovered: test_build_sha.py present, test_version_consistency.py absent -> escalate. The script was already correct in both directions; only one direction was pinned. Verified by mutation -- narrowing the mapping to build-sha alone turns the new test RED while the existing build-sha-sibling test stays green. And adds the missing `import pytest`: pytest.MonkeyPatch was already referenced in an annotation, surviving only because `from __future__ import annotations` never evaluates it. Evidence: - RED first: with BRAINLAYER_VERSION_CHECK_CASK_LAG_REASON exported, 4 failed / 12 passed (the two pre-existing tests + both new ones). After: 16 passed, with and without the export. - tests/test_version_consistency.py + tests/test_run_tests_script.py: 39 passed. - ruff check + ruff format --check clean. Co-Authored-By: Claude Opus 5 (1M context) <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_acfa6ed6-1f91-48f6-a69c-aca688864c3a) |
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Python |  |
Sep 5, 2026 3:12p.m. | Review ↗ |
| Swift | |
Sep 5, 2026 3:12p.m. | Review ↗ |
| JavaScript | |
Sep 5, 2026 3:12p.m. | Review ↗ |
| Shell | |
Sep 5, 2026 3:12p.m. | Review ↗ |
| Secrets | |
Sep 5, 2026 3:12p.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
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. 1 RED row(s) to clear: Measured on Linux/x86_64 · measured |
|
Warning Review limit reachedNext included review available in 23 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: Team Run ID: 📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (3)
🧰 Additional context used🪛 ast-grep (0.45.2)tests/test_run_tests_script.py[error] 758-760: Command coming from incoming request (subprocess-from-request) tests/test_version_consistency.py[error] 93-97: Avoid command injection (command-injection-python) [error] 93-97: Command coming from incoming request (subprocess-from-request) [error] 112-112: Command coming from incoming request (subprocess-from-request) [error] 323-327: Avoid command injection (command-injection-python) [error] 323-327: Command coming from incoming request (subprocess-from-request) 🔇 Additional comments (2)
📝 WalkthroughWalkthroughThe changes add regression coverage for changed-only test selection and isolate version-check fixture subprocesses from inherited environment overrides. ChangesTest coverage updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to Version-check tests now run with release and Git overrides removed from their subprocess environments, and changed-only test selection covers the missing sibling case. No current merge-blocking risk remains. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
…, without losing its teeth
CI caught this, not the local gate: `tests/test_kg_judge.py::test_git_shellout_tests_scrub_inherited_git_env`
is a meta-guard that reads every git-shelling test file and demanded the LITERAL
`if not key.startswith("GIT_")` plus `env=_clean_git_env()`. This branch replaced both in
tests/test_version_consistency.py with a named prefix tuple and `_script_env()`, so the guard went
red on 3.11/3.12/3.13 while the scrub it guards got STRONGER.
The contract is "GIT_* is scrubbed", not one exact line of code. The guard now accepts either the
inline literal or `startswith(_SCRUBBED_ENV_PREFIXES)` -- but only when the tuple demonstrably
leads with "GIT_" -- and `_script_env` joins the accepted helper names. The failing file is named
in the assertion message, which it was not before.
Verified by mutation: dropping "GIT_" from the tuple in tests/test_version_consistency.py fails the
guard with `AssertionError: test_version_consistency.py`. Restored; 54 passed across
test_kg_judge.py, test_version_consistency.py and test_run_tests_script.py.
Why the local gate missed it: test_kg_judge.py is not a changed-only target for a change to
test_version_consistency.py, so the scoped pre-push never ran the guard. That mapping gap is a
separate defect and is reported to the lead rather than fixed here.
Co-Authored-By: Claude Opus 5 (1M context) <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_ab323f99-9263-4265-9f1c-bf8ca0445dea) |
…again, and now says so in a test
Round-1 review, medium — confirmed, my own regression. Relaxing the guard to accept the prefix-tuple
spelling dropped the `if not` from both accepted forms, so a substring match on
`startswith("GIT_")` would also pass a file carrying an inverted KEEP-only filter
(`if key.startswith("GIT_")`) — which names the same prefix and hands the fixture repos the parent
checkout's GIT_DIR. That is a fail-open weakening of the cross-file contract this guard exists to
enforce.
The predicate is now a named helper, `_drops_inherited_git_env`, and both accepted spellings carry
`if not`:
- `if not key.startswith("GIT_")` (seven of the eight guarded files), or
- `if not key.startswith(_SCRUBBED_ENV_PREFIXES)` where the tuple demonstrably leads with `"GIT_"`
(tests/test_version_consistency.py, which must also scrub BRAINLAYER_VERSION_CHECK_*).
Test-first: extracting the predicate is what makes the polarity testable at all, so it is covered
directly — `test_git_env_scrub_predicate_requires_the_dropping_polarity` parametrizes six shapes,
including both inversions and a tuple that no longer leads with "GIT_" (a tuple scrubbing something
else under a name that reads right). It fails on the pre-fix predicate, which accepted the two
inverted cases.
Evidence:
- 7 passed on the predicate + guard; tests/test_kg_judge.py + tests/test_version_consistency.py:
37 passed.
- Mutation on a REAL guarded file: inverting `if not key.startswith(_SCRUBBED_ENV_PREFIXES)` in
tests/test_version_consistency.py fails the guard with
`AssertionError: test_version_consistency.py`. Restored.
- Full CI matrix on the previous head (742e55d) was green on 3.11/3.12/3.13; this commit only
tightens the guard.
- ruff check + ruff format clean.
Co-Authored-By: Claude Opus 5 (1M context) <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_a43fceee-4afe-4194-9f41-32a3836b7119) |
Round 1 — the medium is real, and it was my own regressionmedium Relaxing the guard to accept the prefix-tuple spelling dropped Both accepted spellings now carry
Test-first, and the extraction is what makes it testable. The predicate is now Mutation on a real guarded file: inverting 7 passed on the predicate + guard; Nothing refuted. New head: brainlayerClaude-19a28f09 running claude-opus-5. |
All six sites: pyproject.toml, src/brainlayer/__init__.py, server.json (root + packages[0]), brain-bar/bundle/Info.plist (short, bundle, release). Casks/brainbar.rb stays 1.5.9: no Swift change this release, declared to scripts/brainlayer-version-check.sh via BRAINLAYER_VERSION_CHECK_CASK_LAG_REASON="no BrainBar release for 1.5.15". Why a release. The installed keg on the M4 is 1.5.14 / build_sha 3bbe19f. Verified: `git merge-base --is-ancestor 7369f80 3bbe19f` -> false, so #781 -- the watcher per-poll burst fix, 14.97% -> 2.39% under the LaunchAgent's background QoS -- is NOT in any installed keg. Enabling com.brainlayer.watch today would start the burst-y watcher. 1.5.15 is what carries #781 (7369f80) onto the Mac. Also in: #772 #773 #775 #776. Co-authored-by: brainlayerClaude-ebe88b04 running claude-opus-5 <noreply@anthropic.com>
1 participant
What forced this PR
tests/test_version_consistency.pyranscripts/brainlayer-version-check.shin a subprocess withthe inherited environment. A release operator has
BRAINLAYER_VERSION_CHECK_CASK_LAG_REASONexported — they need it for the real release check — so pre-push → pytest → the script saw that
reason, allowed the cask lag, and returned 0. The two tests that assert the script fails on a
lagging or drifting cask without a stated reason then went RED for a change that had nothing to
do with them:
test_version_check_fails_loudly_when_cask_version_drifts(cask0.0.0)test_version_check_still_fails_for_a_lagging_cask_without_a_reason(cask1.5.9)Loud, not silent — but it is the same non-hermetic-env class the sibling
tests/test_run_tests_script.py::_script_env()already solves forBRAINLAYER_CHANGED_FILES.The change
_script_env(), builds the subprocess env from a scrubbed copy ofos.environ, andevery subprocess call in the file goes through it (it replaces
_clean_git_env()).BRAINLAYER_VERSION_CHECK_*alongside the existing
GIT_*.REPO_ROOT,TAP_ROOTandGIT_TAGare the script's otherknobs and would steer these fixtures elsewhere in exactly the same way — scrubbing only the lag
reason just queues up the next instance. Tests that want a knob still pass it explicitly via
extra_env/git_tag, which override the scrubbed base.import pytest.pytest.MonkeyPatchwas already referenced in an annotationand survived only because
from __future__ import annotationsnever evaluates it.No change to
scripts/brainlayer-version-check.sh.Folded in: the mirror sibling case from the #762 round-1 review
The round-1 review on #762 asked for the missing-sibling regression and got the
test_build_sha.pyhalf. Nothing pinned the other direction, so a mapping that kept
mapped=1when onlytest_build_sha.pyexisted would still have looked covered — import path gated, the six versionsites and the cask-lag reason not gated at all.
test_changed_only_scope_escalates_when_the_version_consistency_sibling_is_missingpins it.The script is already correct in both directions; only one direction was tested. Verified by
mutation rather than asserted: narrowing the
src/brainlayer/__init__.pycase to build-sha aloneturns the new test RED (
1 failed, 1 passed) while the existing build-sha-sibling test staysgreen — they cover different directions. Script restored,
git diff scripts/run_tests.shempty.Evidence
BRAINLAYER_VERSION_CHECK_CASK_LAG_REASON="operator export" pytest tests/test_version_consistency.py→ 4 failed / 12 passed (the two pre-existing testsreproducing the reported defect, plus both new ones). After the fix: 16 passed, with the var
exported and without it.
tests/test_version_consistency.py+tests/test_run_tests_script.py→ 39 passed.ruff checkclean;ruff format --check tests/ src/→ 482 files already formatted.BRAINLAYER_PREPUSH_SCOPE=changed-onlywithBRAINLAYER_CHANGED_FILESnaming both touched files — 39 passed, registration, isolated 40passed, bun 1 pass, fts5 determinism shell → "BrainLayer test gate passed."
Size
XS — tests only: 2 test files, 1 helper rename + prefix scrub, 3 new tests.
@coderabbitai review
Opened by brainlayerClaude-19a28f09 running claude-opus-5.
Note
Low Risk
Tests-only changes that improve subprocess env isolation and pre-push mapping coverage; no production script or runtime behavior changes.
Overview
Fixes non-hermetic version-check tests that inherited a release operator’s
BRAINLAYER_VERSION_CHECK_*exports (especially cask-lag reason), which could make “must fail without a reason” cases pass spuriously.In
test_version_consistency.py,_clean_git_env()becomes_script_env(), strippingGIT_*andBRAINLAYER_VERSION_CHECK_*by prefix before every subprocess; fixtures still pass knobs viaextra_env/git_tag. Adds regressions for inherited lag reason and full prefix scrub, plusimport pytestforMonkeyPatch.test_kg_judge.pyreplaces a brittle substring guard with_drops_inherited_git_env(parametrized) so git-shellout tests must dropGIT_*(including tuple-based scrubbers that still lead withGIT_) and may use_script_envalongside the existing clean-env helpers.test_run_tests_script.pyadds the mirror of the missingtest_build_sha.pycase: changed-only scope onsrc/brainlayer/__init__.pymust escalate to the full suite whentest_version_consistency.pyis absent.Production
brainlayer-version-check.shis unchanged; only test isolation and coverage.Reviewed by Cursor Bugbot for commit 260d431. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Scrub
BRAINLAYER_VERSION_CHECK_*from test subprocess env and pin version-consistency sibling escalation case_script_envin test_version_consistency.py and broadens it to strip bothGIT_- andBRAINLAYER_VERSION_CHECK_-prefixed variables from all fixture subprocess environments, including version-check invocations via_run, repo git lookups, and repo init fixtures._drops_inherited_git_envsource-text predicate and guard tests in test_kg_judge.py to ensure git-shellout fixtures actually drop inheritedGIT_variables in either direct or tuple-based filter form, and to recognize the_script_envhelper name.test_version_consistency.pyis absent even thoughtest_build_sha.pyexists.BRAINLAYER_VERSION_CHECK_overrides are removed prefix-wide._git_envhelper name or expectedBRAINLAYER_VERSION_CHECK_*variables to pass through to subprocesses will break; the helper rename in_script_envrequires all call sites intest_version_consistency.pyto be updated (done in-tree).Macroscope summarized 260d431.
Summary by CodeRabbit