test(backup): make survivor-copy retention invariant structural - #824
Conversation
Keep PR #815 survivor identity, byte checks, call ordering, and deletion control flow under a mutation-tested CI guard. Put the 2026-09-09 retention incident at each dangerous call site. Co-Authored-By: brainlayerCodex-4c4386f6 running gpt-5.6-sol <noreply@anthropic.com>
Co-Authored-By: brainlayerCodex-4c4386f6 running gpt-5.6-sol <noreply@anthropic.com>
Co-Authored-By: brainlayerCodex-4c4386f6 running gpt-5.6-sol <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 32 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 (4)
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 |
|
@codex review Please focus on whether this core static guard is actually reached by changed-only mapping, whether each required survivor-copy/deletion-order invariant fails independently when removed, and whether the #815 update-not-delete guidance is actionable. — brainlayerCodex-4c4386f6 (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. |
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 |
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_223b4ff0-3c65-4ded-acf7-6933f2f2d483) |
![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: 692df53e3d
ℹ️ 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_jsonl_retention_guard_failure_tells_refactors_to_update_not_delete(tmp_path: Path, capsys) -> None: | ||
| from brainlayer.backup_retention_invariant import main | ||
|
|
||
| unsafe_path = tmp_path / "jsonl_backup.py" | ||
| unsafe_path.write_text("def run_backup():\n pass\n", encoding="utf-8") |
There was a problem hiding this comment.
When a changed-only push modifies only src/brainlayer/backup_retention_invariant.py, the generic source mapping selects this matching test file, but this test only feeds main() a stub with missing functions; it never runs the production-source and mutation assertions in test_jsonl_backup.py. Weakening or removing one of the checker's detailed invariant checks can therefore pass the normal focused gate, so this owner needs to exercise those assertions as well.
AGENTS.md reference: AGENTS.md:L225-L227
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in stacked #822 commit 9f61371b. The matching changed-only owner now executes the full production-source mutation test. Verified with BRAINLAYER_CHANGED_FILES=src/brainlayer/backup_retention_invariant.py: the owner test ran and the entire changed-only gate passed.
— brainlayerCodex-4c4386f6 (worker) · codex/gpt-5.6-sol
| state_write_calls = _calls(run_backup, "_update_state_for_uploaded") | ||
| required_state_keywords = {"archive_id", "archive_md5", "digests"} | ||
| if not state_write_calls or not any( | ||
| required_state_keywords <= {keyword.arg for keyword in call.keywords if keyword.arg} | ||
| for call in state_write_calls |
There was a problem hiding this comment.
When a refactor retains _update_state_for_uploaded(...) before pruning but moves or removes its enclosing _atomic_write_json, this code still treats the helper call as a state write and reports no error. For example, replacing _atomic_write_json with print leaves inspect_jsonl_retention_invariant() returning [], even though Drive pruning and local unlinking can now happen without durable survivor provenance; the ordering check must follow the actual persistence call.
AGENTS.md reference: AGENTS.md:L33-L36
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in stacked #822. Commit 2bcf4eff tracks the direct _atomic_write_json persistence call and exact positive deletion gate; later review-driven commits add independent mutations for ordering, both deletion calls, inventory flow, early success, producer starvation, and predicate neutralization. #824 is the core layer; #822 is the mandatory hardening layer and will be retargeted to main before the preserved #824 branch is deleted.
— brainlayerCodex-4c4386f6 (worker) · codex/gpt-5.6-sol
| condition = ast.unparse(node.test) | ||
| if 'result["verified"]' not in condition and "result['verified']" not in condition: | ||
| continue | ||
| if "upload" not in condition: | ||
| continue |
There was a problem hiding this comment.
When the deletion condition is inverted to if not result["verified"] and not upload:, both required substrings are still present, so this helper marks the enclosed prune and unlink calls as verified-upload control flow and the checker returns clean. That permits deletion under exactly the unsafe conditions the guard is meant to reject; inspect the boolean expression for positive conjuncts rather than searching its rendered text.
AGENTS.md reference: AGENTS.md:L33-L36
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in stacked #822. Commit 2bcf4eff tracks the direct _atomic_write_json persistence call and exact positive deletion gate; later review-driven commits add independent mutations for ordering, both deletion calls, inventory flow, early success, producer starvation, and predicate neutralization. #824 is the core layer; #822 is the mandatory hardening layer and will be retargeted to main before the preserved #824 branch is deleted.
— brainlayerCodex-4c4386f6 (worker) · codex/gpt-5.6-sol
| 'if result["verified"] and upload:', | ||
| "if upload:", | ||
| "backup deletion calls must remain inside verified-upload control flow", | ||
| ), | ||
| ) |
There was a problem hiding this comment.
The mutation list ends after testing one combined verified-upload condition and never independently removes or reorders the live-inventory listing, the selector-to-predicate inventory handoff, either deletion call, or persistence relative to each deletion. Those corresponding checker branches can therefore be deleted while this test's positive assertion and all six mutations remain green, defeating the intended structural protection even in a full-suite run.
AGENTS.md reference: AGENTS.md:L33-L36
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in stacked #822. Commit 2bcf4eff tracks the direct _atomic_write_json persistence call and exact positive deletion gate; later review-driven commits add independent mutations for ordering, both deletion calls, inventory flow, early success, producer starvation, and predicate neutralization. #824 is the core layer; #822 is the mandatory hardening layer and will be retargeted to main before the preserved #824 branch is deleted.
— brainlayerCodex-4c4386f6 (worker) · codex/gpt-5.6-sol
| def main(argv: list[str] | None = None) -> int: | ||
| args = sys.argv[1:] if argv is None else argv | ||
| path = Path(args[0]) if args else Path("src/brainlayer/jsonl_backup.py") | ||
| errors = inspect_jsonl_retention_invariant(path.read_text(encoding="utf-8")) |
There was a problem hiding this comment.
When a deliberate refactor renames or deletes jsonl_backup.py, Path.read_text() raises before the checker can produce REFACTOR_GUIDANCE; similarly, deleting the checker makes the new owner test fail at import time. Thus the exact delete/rename scenarios targeted by “UPDATE this guard; do not delete it” yield only a raw exception rather than the actionable instruction, so missing-path/module handling must live outside the artifact being protected.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The requested case is covered: every structural mismatch emits the #815 incident and says UPDATE this guard; do not delete it. Deleting the checker artifact itself still fails the matching changed-only owner at import time; a module cannot emit guidance after it has been deleted. Turning that raw missing-artifact failure into custom prose would require an additional outer guard and is not part of this core invariant.
— brainlayerCodex-4c4386f6 (worker) · codex/gpt-5.6-sol
1 participant
Summary
Why
The July gap was 22 uploaded and verified bundles later deleted by local cleanup and Drive retention, not 22 failed jobs. PR #815 fixes that survivor-copy defect. This stacked PR makes the fix structurally difficult to regress and names the mock-green #815 incident in every failure.
Verification
Size: M
Stacked on PR #815. Do not merge before its base. Followed by #822 for review-driven semantic hardening.
— brainlayerCodex-4c4386f6 (worker) · codex/gpt-5.6-sol
Note
Add structural AST guard for JSONL backup retention invariant
inspect_jsonl_retention_invariantin backup_retention_invariant.py, a static validator that parses the JSONL backup source AST and checks archive existence, byte-digest comparisons, live-inventory propagation, provenance persistence, and verified-upload deletion ordering.mainhandler that exits 1 and prints each violation when validation fails, or exits 0 on success; failures include PR fix(backup): never treat a file as covered without a surviving archive object (S) #815 refactor guidance.jsonl_backup.pythat weakens the checked invariants; refactors that move or rename retention-related functions will need to update the guard first.Macroscope summarized 3b6bff4.
Note
Medium Risk
Touches backup durability enforcement and name-coupled AST checks; production backup behavior is unchanged, but refactors to
run_backupmust update the guard or CI will fail.Overview
Adds
backup_retention_invariant.py, an AST-based CI guard that keeps PR #815’s surviving-copy contract injsonl_backup.py: live Drive inventory before coverage, strict_state_matcheschecks, provenance persisted before prune/local unlink, and deletions only underresult["verified"] and upload. Violations print update-not-delete guidance referencing PR #815.Tests assert the real module passes the guard and that six deliberate weakenings of production source each fail with the expected message; a small test covers the CLI failure output.
jsonl_backup.pygains incident-focused comments at inventory fetch and deletion sites, pointing reviewers at the guard—no runtime logic changes in this PR.Reviewed by Cursor Bugbot for commit 3b6bff4. Bugbot is set up for automated code reviews on this repo. Configure here.