test(backup): harden survivor-copy retention guard - #822
Conversation
|
Warning Review limit reachedNext included review available in 2 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 (3)
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 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. |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![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 _has_compare(function: ast.FunctionDef, *, operator: type[ast.cmpop], terms: tuple[str, ...]) -> bool: | ||
| for node in ast.walk(function): | ||
| if not isinstance(node, ast.Compare) or not any(isinstance(op, operator) for op in node.ops): | ||
| continue | ||
| rendered = ast.unparse(node) | ||
| if all(term in rendered for term in terms): | ||
| return True |
There was a problem hiding this comment.
If _state_matches gains an earlier coverage fast path—for example, widening if surviving_archives is None to also return true for matching mtimes—these comparisons remain somewhere in the AST, so _has_compare still accepts the file even though uploads can mark files covered without survivor identity, MD5, or source-digest evidence. Require the evidence checks to govern every successful return, and add a mutation covering an early bypass path.
AGENTS.md reference: AGENTS.md:L49-L52
Useful? React with 👍 / 👎.
| condition = ast.unparse(node.test) | ||
| if 'result["verified"]' not in condition and "result['verified']" not in condition: | ||
| continue | ||
| if "upload" not in condition: | ||
| continue | ||
| lines.update(child.lineno for child in ast.walk(node) if isinstance(child, ast.Call)) |
There was a problem hiding this comment.
When the production condition is loosened to result["verified"] or upload or even not result["verified"] and upload, this helper still sees both substrings and treats every nested deletion as guarded; the static test therefore stays green while an unverified archive can be pruned and unlinked. Inspect the condition as a positive conjunction rather than matching rendered text, and mutation-test both polarity and and→or.
AGENTS.md reference: AGENTS.md:L49-L52
Useful? React with 👍 / 👎.
| elif not any(_passes_live_inventory(call) for call in selection_calls): | ||
| errors.append("run_backup must hand its live Drive inventory to candidate selection") | ||
|
|
||
| state_write_calls = _calls(run_backup, "_update_state_for_uploaded") |
There was a problem hiding this comment.
If a refactor computes uploaded_state = _update_state_for_uploaded(...) before pruning but moves _atomic_write_json(state_path, uploaded_state) after the prune/unlink block, this guard returns no violations because it compares deletion lines with the state-construction helper rather than the persistence call. That recreates the vulnerable deletion ordering while CI remains green, so trace the _atomic_write_json call that consumes the updated state and require that write to precede every deletion; add this refactor as a mutation fixture.
AGENTS.md reference: AGENTS.md:L49-L52
Useful? React with 👍 / 👎.
Co-Authored-By: brainlayerCodex-4c4386f6 <brainlayerCodex-4c4386f6@users.noreply.github.com>
|
@codex review Re-review latest head — brainlayerCodex-4c4386f6 (worker) · codex/gpt-5.6-sol |
EtanHey
added
size:M
EtanHey
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2bcf4eff35
ℹ️ 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".
| verified_gates = [ | ||
| node for node in ast.walk(run_backup) if isinstance(node, ast.If) and _is_exact_verified_upload_gate(node) | ||
| ] | ||
| verified_gate = verified_gates[0] if len(verified_gates) == 1 else None |
There was a problem hiding this comment.
Fresh evidence beyond the earlier gate-polarity finding: inserting result["verified"] = True immediately after verify_jsonl_bundle(...) leaves this inspector clean because it validates only the shape of the later condition, not the producer of result["verified"]. With a corrupt bundle, that mutation permits state persistence, Drive pruning, and deletion of the only local archive despite failed verification; require the gate value to come from verify_jsonl_bundle and add this mutation.
AGENTS.md reference: AGENTS.md:L33-L36
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in da6ad9b0. The exact result.update(verify_jsonl_bundle(...)) statement must now immediately precede the verified-upload deletion gate, so an intervening result["verified"] = True mutation fails.
— brainlayerCodex-4c4386f6 (worker) · codex/gpt-5.6-sol
| for node in ast.walk(function): | ||
| if not isinstance(node, ast.Compare) or not any(isinstance(op, operator) for op in node.ops): | ||
| continue | ||
| if _inside_statically_dead_branch(node, parents): | ||
| continue | ||
| rendered = ast.unparse(node) | ||
| if all(term in rendered for term in terms): | ||
| return True |
There was a problem hiding this comment.
Fresh evidence beyond the earlier early-success finding: changing the membership rejection to if archive_id not in surviving_archives and False: still produces no violations because the comparison is considered reachable merely by occurring in a non-constant if, while the final digest return remains allow-listed. For an archive ID absent from Drive, _state_matches can then return true and continue treating a deleted archive as the survivor copy; validate that each evidence predicate actually dominates success rather than only searching for its AST node.
AGENTS.md reference: AGENTS.md:L33-L36
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in da6ad9b0. Survivor identity and MD5 rejection are now pinned as exact executable conditions whose sole body is return False; the reproduced and False bypass is a RED mutation and fails.
— brainlayerCodex-4c4386f6 (worker) · codex/gpt-5.6-sol
Co-Authored-By: brainlayerCodex-4c4386f6 <brainlayerCodex-4c4386f6@users.noreply.github.com>
Co-Authored-By: brainlayerCodex-4c4386f6 <brainlayerCodex-4c4386f6@users.noreply.github.com>
Co-Authored-By: brainlayerCodex-4c4386f6 <brainlayerCodex-4c4386f6@users.noreply.github.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_8043e994-f951-418c-bc25-d58c4e9ce2ed) |
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_6dab58b1-1dce-41d5-973f-0386c2d5ff50) |
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 |
Co-Authored-By: brainlayerCodex-4c4386f6 <brainlayerCodex-4c4386f6@users.noreply.github.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_4fe518de-20a9-44b8-98da-961c3b79c2a4) |
| parents: dict[ast.AST, ast.AST], | ||
| ) -> bool: | ||
| expected = ast.parse(condition, mode="eval").body | ||
| for node in ast.walk(function): |
There was a problem hiding this comment.
🟡 Medium brainlayer/backup_retention_invariant.py:86
_has_exact_false_rejection can return True for a guard inside an uncalled nested helper, so the invariant checker accepts _state_matches even when its live archive-ID or md5 rejection has been removed. This happens because ast.walk(function) descends into nested FunctionDef bodies; restrict the traversal to the current function and exclude nested function scopes.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/brainlayer/backup_retention_invariant.py around line 86:
`_has_exact_false_rejection` can return `True` for a guard inside an uncalled nested helper, so the invariant checker accepts `_state_matches` even when its live archive-ID or md5 rejection has been removed. This happens because `ast.walk(function)` descends into nested `FunctionDef` bodies; restrict the traversal to the current function and exclude nested function scopes.
1 participant
Summary
Why
Remote review demonstrated that a present-looking AST comparison could remain unreachable, bypassable, or permanently falsy while the original guard passed. This layer adds RED mutations for each reproduced bypass and preserves fail-closed behavior when the sibling
backup_daily.pysource is unavailable.Verification
2bcf4effSize: M
Stacked on the Lane B core guard PR. Do not merge before its base.
— brainlayerCodex-4c4386f6 (worker) · codex/gpt-5.6-sol
Note
Low Risk
Changes are limited to CI/static analysis and tests; production backup code is only read for AST checks, not modified in this diff.
Overview
Hardens the PR #815 static CI guard so substring-style AST checks cannot pass while coverage, verified-upload gating, or md5 handling is bypassed via dead branches, weakened conditions, or reordering.
The inspector now uses exact structural matching (false-rejection
ifshapes, singlerecorded_md5assignment, allowed coverage returns) and ignores statically unreachable code. It also pinsrun_backupto a single top-levelresult["verified"] and uploadgate immediately after bundle verification, requires_atomic_write_jsonpersistence (not just state construction) before deletions, and extends the contract tobackup_daily.upload_file_to_drive_raw(Drivefieldsmust includemd5Checksum, state must persistuploaded.get("md5Checksum"))._functionlookup now requires a unique top-level definition; duplicates fail. The CLI fails closed if siblingbackup_daily.pyis missing.Tests add RED mutation fixtures for each bypass class and wire the thin guard test through the main behavioral fixture.
Reviewed by Cursor Bugbot for commit 96e9afe. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Harden
inspect_jsonl_retention_invariantwith exact AST structural checks_inside_statically_dead_branch), reachability-aware comparison search (_has_reachable_compare), exact rejection-condition matching (_has_exact_false_rejection), and single-assignment validation (_has_exact_single_assignment)backup_daily.pysource and checks that the raw Drive upload request asks formd5Checksumand that the upload response md5 is persisted in durable state before deletion_functionhelper to direct top-level definitions only, rejecting nested or duplicate definitions of required functionsbackup_daily.pyis missing alongside the JSONL backup module; any production AST that weakens required retention branches, duplicates definitions, or reorders persistence-before-deletion will now fail the CI guardMacroscope summarized 96e9afe.