fix(maintenance): refuse to run stale code — merging is not deploying - #651
Conversation
com.brainlayer.maintenance-nightly runs `python -m brainlayer.maintenance` from an EDITABLE install, so `import brainlayer` resolves to the WORKING TREE, not a built artifact. On 2026-08-05 that tree was 8 commits behind origin/main: #650's pause- sentinel fix was merged at 05:09Z and simply was not on disk. The job would have run stale code and reported ok — a third 04:00 enrichment restart, with the board saying the fix had shipped. It reopened within 30 minutes of a manual pull when #630 merged. Whether the service runs correct code cannot depend on someone remembering to `git pull`. Guard asserts, it does not auto-pull: a half-succeeded pull in an unattended 04:00 job is worse than a clean refusal. On drift it aborts naming both SHAs and the remedy. 'Unverifiable' is never treated as 'fresh' — an unreachable origin warns (strict mode aborts) rather than silently proceeding. Same defect class as the bug #650 fixed, one level up: inferring a state instead of checking it. Escape hatch: BRAINLAYER_MAINTENANCE_ALLOW_STALE=1, logged to stderr.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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_915e70c2-72bb-4c8d-a943-e2adc612c222) |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
📝 WalkthroughWalkthroughMaintenance now verifies that the editable working tree matches the merged reference before execution. It supports strict and non-strict verification, explicit stale-code overrides, and pytest bypasses. ChangesMaintenance freshness validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant run_maintenance
participant _assert_running_merged_code
participant GitRepository
participant MaintenanceAbort
run_maintenance->>_assert_running_merged_code: Check repository freshness
_assert_running_merged_code->>GitRepository: Fetch and read origin/main SHA
GitRepository-->>_assert_running_merged_code: Return working-tree and merged-reference SHAs
_assert_running_merged_code-->>run_maintenance: Continue when SHAs match
_assert_running_merged_code->>MaintenanceAbort: Abort on stale or unverifiable code
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| ) | ||
| return | ||
|
|
||
| if head != merged: |
There was a problem hiding this comment.
🟠 High brainlayer/maintenance.py:484
_assert_running_merged_code only compares HEAD to origin/main, so uncommitted edits to tracked files pass the check. When HEAD == origin/main but maintenance.py or another imported module has local modifications, the editable install imports the modified working tree — not the merged code — yet the guard proceeds as if the code is fresh, silently defeating the freshness guarantee. Consider checking for a dirty working tree (e.g. git status --porcelain or git diff-index) and aborting when tracked files have local changes, or document why uncommitted edits are acceptable.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/brainlayer/maintenance.py around line 484:
`_assert_running_merged_code` only compares `HEAD` to `origin/main`, so uncommitted edits to tracked files pass the check. When `HEAD == origin/main` but `maintenance.py` or another imported module has local modifications, the editable install imports the modified working tree — not the merged code — yet the guard proceeds as if the code is fresh, silently defeating the freshness guarantee. Consider checking for a dirty working tree (e.g. `git status --porcelain` or `git diff-index`) and aborting when tracked files have local changes, or document why uncommitted edits are acceptable.
| return out.stdout.strip() or None | ||
|
|
||
|
|
||
| def _git_merged_head_sha(repo_root: Path) -> str | None: |
There was a problem hiding this comment.
🟠 High brainlayer/maintenance.py:425
_git_merged_head_sha ignores the exit status of git fetch and then resolves the cached origin/main ref. When the remote is unreachable, an old cached origin/main can match local HEAD, so _assert_running_merged_code treats the working tree as fresh and proceeds with maintenance instead of warning or aborting about an unverifiable origin. The intended unverifiable-origin path in _assert_running_merged_code is never reached as long as any cached origin/main ref exists. Consider checking the fetch result before resolving origin/main, or resolving the remote ref directly (e.g. git rev-parse origin/main after confirming the fetch succeeded).
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/brainlayer/maintenance.py around line 425:
`_git_merged_head_sha` ignores the exit status of `git fetch` and then resolves the cached `origin/main` ref. When the remote is unreachable, an old cached `origin/main` can match local `HEAD`, so `_assert_running_merged_code` treats the working tree as fresh and proceeds with maintenance instead of warning or aborting about an unverifiable origin. The intended unverifiable-origin path in `_assert_running_merged_code` is never reached as long as any cached `origin/main` ref exists. Consider checking the fetch result before resolving `origin/main`, or resolving the remote ref directly (e.g. `git rev-parse origin/main` after confirming the fetch succeeded).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/brainlayer/maintenance.py`:
- Around line 428-444: Update the maintenance helper containing the git fetch
and origin/main rev-parse flow to capture the fetch result and return None when
its returncode is nonzero, preventing use of a cached ref; preserve the existing
successful fetch and rev-parse behavior. Add a regression test covering a failed
fetch while a cached origin/main ref exists and assert that the helper returns
None.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 85ea94ab-7f91-4142-8f4f-b6b7b4f3e78a
📒 Files selected for processing (2)
src/brainlayer/maintenance.pytests/test_maintenance_code_freshness.py
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
- GitHub Check: test (3.13)
🧰 Additional context used
📓 Path-based instructions (2)
src/brainlayer/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/brainlayer/**/*.py: Treat retrieval correctness, write safety, and MCP stability as critical-path correctness concerns; prioritize regressions in search quality, lock handling, and tool contracts.
Implement database concurrency with one writer at a time, allow concurrent reads, use per-worker connections, and retry SQLITE_BUSY errors.
Resolve the database path throughpaths.py:get_db_path()rather than hard-coding the canonical path, while supporting the environment-variable override.
Preserveai_code,stack_trace, anduser_messageverbatim; skip noise, summarize build logs, retain only structure for directory listings, use AST-aware tree-sitter chunking, never split stack traces, and mask large tool output.
Exclude only brain-worker and session-miner/weave sessions from indexing, never delete their source JSONL, and keepBRAINLAYER_INGEST_DENYLISTas an explicit deployment override.
Default searches must exclude lifecycle-managed chunks;include_archived=Truemust expose history.brain_archiveis a soft delete, andbrain_supersedemust retain its personal-data safety gate.
brain_storesupersession must use the atomicsupersedesstore-and-replace behavior.
Do not runbrain_digestor other write-heavy MCP work in parallel with other MCP writes; never run bulk database operations while enrichment writes.
The canonical database path is~/.local/share/brainlayer/brainlayer.db; offsets, locks, sockets, logs, and session-dedup files must use their documented BrainLayer locations.
Always runbrain_searchbefore answering questions about project history, architecture, or past decisions, and runbrain_storeafter decisions, bugs, or corrections.
Files:
src/brainlayer/maintenance.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.py: Runpytestbefore claiming behavior changes are safe.
Tests must not refresh the production backup heartbeat log; useBRAINLAYER_BACKUP_LOG_PATHandBRAINLAYER_BACKUP_LOG_PROVENANCE=pytest.
Files:
tests/test_maintenance_code_freshness.py
🪛 ast-grep (0.45.0)
src/brainlayer/maintenance.py
[error] 412-418: Command coming from incoming request
Context: subprocess.run(
["git", "-C", str(repo_root), "rev-parse", "HEAD"],
capture_output=True,
text=True,
timeout=30,
check=True,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 427-433: Command coming from incoming request
Context: subprocess.run(
["git", "-C", str(repo_root), "fetch", "--quiet", "origin", "main"],
capture_output=True,
text=True,
timeout=60,
check=False,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 434-440: Command coming from incoming request
Context: subprocess.run(
["git", "-C", str(repo_root), "rev-parse", "origin/main"],
capture_output=True,
text=True,
timeout=30,
check=True,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
| subprocess.run( | ||
| ["git", "-C", str(repo_root), "fetch", "--quiet", "origin", "main"], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=60, | ||
| check=False, | ||
| ) | ||
| out = subprocess.run( | ||
| ["git", "-C", str(repo_root), "rev-parse", "origin/main"], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=30, | ||
| check=True, | ||
| ) | ||
| except Exception: | ||
| return None | ||
| return out.stdout.strip() or None |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail closed when git fetch fails.
check=False discards the fetch result. If origin is unreachable and a cached origin/main exists, rev-parse succeeds against that stale ref. The guard can then allow maintenance after main has advanced remotely.
Return None when the fetch exits nonzero. Add a regression test for a failed fetch with a cached origin/main ref.
Proposed fix
- subprocess.run(
+ fetch = subprocess.run(
["git", "-C", str(repo_root), "fetch", "--quiet", "origin", "main"],
capture_output=True,
text=True,
timeout=60,
check=False,
)
+ if fetch.returncode != 0:
+ return None
out = subprocess.run(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| subprocess.run( | |
| ["git", "-C", str(repo_root), "fetch", "--quiet", "origin", "main"], | |
| capture_output=True, | |
| text=True, | |
| timeout=60, | |
| check=False, | |
| ) | |
| out = subprocess.run( | |
| ["git", "-C", str(repo_root), "rev-parse", "origin/main"], | |
| capture_output=True, | |
| text=True, | |
| timeout=30, | |
| check=True, | |
| ) | |
| except Exception: | |
| return None | |
| return out.stdout.strip() or None | |
| fetch = subprocess.run( | |
| ["git", "-C", str(repo_root), "fetch", "--quiet", "origin", "main"], | |
| capture_output=True, | |
| text=True, | |
| timeout=60, | |
| check=False, | |
| ) | |
| if fetch.returncode != 0: | |
| return None | |
| out = subprocess.run( | |
| ["git", "-C", str(repo_root), "rev-parse", "origin/main"], | |
| capture_output=True, | |
| text=True, | |
| timeout=30, | |
| check=True, | |
| ) | |
| except Exception: | |
| return None | |
| return out.stdout.strip() or None |
🧰 Tools
🪛 ast-grep (0.45.0)
[error] 434-440: Command coming from incoming request
Context: subprocess.run(
["git", "-C", str(repo_root), "rev-parse", "origin/main"],
capture_output=True,
text=True,
timeout=30,
check=True,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/brainlayer/maintenance.py` around lines 428 - 444, Update the maintenance
helper containing the git fetch and origin/main rev-parse flow to capture the
fetch result and return None when its returncode is nonzero, preventing use of a
cached ref; preserve the existing successful fetch and rev-parse behavior. Add a
regression test covering a failed fetch while a cached origin/main ref exists
and assert that the helper returns None.
Source: Path instructions
1 participant
#650 shipped and would not have run. This closes the gap that made that possible.
The gap
com.brainlayer.maintenance-nightlyexecutes~/Gits/brainlayer/.venv/bin/python -m brainlayer.maintenance. That venv is an editable install —import brainlayerresolves to/Users/etanheyman/Gits/brainlayer/src/brainlayer, the working tree, not a built artifact.On 2026-08-05 the tree sat 8 commits behind
origin/main. #650's pause-sentinel fix merged at 05:09Z and was simply not on disk —grep -c sentinel src/brainlayer/maintenance.py→0. The job would have run stale code, resumed enrichment, and reportedstatus: ok, producing a third consecutive 04:00 restart while the board said the fix had shipped.A manual
git pullfixed it. It reopened 30 minutes later when #630 merged. Whether the service runs correct code cannot depend on someone remembering to pull.Design: assert, don't auto-pull
Auto-pulling adds failure modes — dirty tree, conflicts, network — to an unattended 04:00 job, and a half-succeeded pull is worse than a clean refusal. The required property is runs the merged code, or fails loudly; refusing satisfies it with far less that can go wrong.
originunreachablestrict=Trueaborts — "unverifiable" is never treated as "fresh"BRAINLAYER_MAINTENANCE_ALLOW_STALE=1The unreachable-origin case matters: an absent negative is not a positive. That is the same trap as
PRAGMA quick_check: okon a zero-page database — a check that passes because there is nothing to check.Same defect class as #650, one level up
#650 fixed
_resume_servicesinferring "I am the reason it is stopped" from "it is in my list." The deploy path had the identical shape: inferring "merged" means "running." Both are a proxy standing in for a property.Verification
test_maintenance_routinetests, because a feature branch legitimately differs from main. Scoped rather than weakened.PROCEED — tree matches merged code.3720 passed, 9 skipped, 61 deselected, 1 xfailed in 1016.68s—BrainLayer test gate passed.Still open in this class (not fixed here)
brainlayer enrichand the CLI run from the brew Cellar venv, where main was hand-installed. The nextbrew upgradereverts it silently and this guard does not cover that path — only a v1.5.3 cut + formula bump closes it.Note
Medium Risk
A nightly job can now fail on drift or network/git issues until the tree is pulled, which is intentional but operationally sensitive for unattended maintenance.
Overview
Nightly maintenance now refuses to start unless the repo working tree matches
origin/main, because the launchd job uses an editable install that imports the tree on disk—not whatever was merged on GitHub.run_maintenancecalls_assert_running_merged_codeup front: compare localHEADto a best-effortgit fetch+origin/main. On mismatch it raisesMaintenanceAbortwith both SHAs and agit pullhint.BRAINLAYER_MAINTENANCE_ALLOW_STALE=1opts out (logged); under pytest the guard is skipped so feature-branch tests keep working. Iforigin/maincan’t be verified, default behavior is a stderr warning only;strict=Trueaborts instead of treating “unknown” as fresh.New
tests/test_maintenance_code_freshness.pylocks in match, drift, strict unverifiable remote, override, and pytest vs production behavior.Reviewed by Cursor Bugbot for commit 7e41f97. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Refuse to run stale code in
run_maintenanceby checking HEAD againstorigin/main_assert_running_merged_codeto maintenance.py, which compares the local HEAD SHA againstorigin/mainand raisesMaintenanceAbortif they differ.run_maintenancenow calls this guard before any other logic, so stale working trees are rejected at entry.PYTEST_CURRENT_TESTis set orBRAINLAYER_MAINTENANCE_ALLOW_STALE=1is exported.origin/mainis unreachable, the guard raises ifstrict=Trueor logs a warning and continues ifstrict=False.run_maintenancefrom an unmerged or out-of-date checkout will now get a hard failure unless the override env var is set.📊 Macroscope summarized 7e41f97. 1 file reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.
Summary by CodeRabbit