fix(rollback): capture and report silent filesystem restoration failures#535
fix(rollback): capture and report silent filesystem restoration failures#535DebasmitaBose0 wants to merge 1 commit into
Conversation
Addresses silently ignored failures during filesystem rollback. - FilesystemSnapshot.restore() now extracts files one at a time, catches per-member failures, cleans up partially extracted files to prevent inconsistent state, and logs warnings on partial restore - rollback() now emits a warning when snapshot_path is missing instead of silently marking rollback as COMPLETED - RollbackResult gains 'warnings' and 'partial_restoration' fields for downstream visibility into degraded rollback outcomes - 4 new tests covering missing snapshot, partial restoration tracking, and non-existent checkpoint handling Closes sreerevanth#359
|
@DebasmitaBose0 is attempting to deploy a commit to the sreerevanth's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughRollbackResult gains ChangesRollback Engine Warnings and Partial Restoration
Estimated code review effort: 2 (Simple) | ~12 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
🧪 PR Test Results
Python 3.12 · commit 787f40e |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
tests/test_rollback.py (2)
47-68: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winThis existing security test will likely fail against the updated
restore()implementation.
test_malicious_archive_rejectedexpectspytest.raises(tarfile.FilterError)fromrestore(), but the new per-member extraction loop inagentwatch/rollback/engine.pycatches all exceptions (includingFilterError) and treats them as ordinary partial failures instead of re-raising. See the corresponding comment onagentwatch/rollback/engine.py(lines 123-169) for the root-cause fix.🤖 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 `@tests/test_rollback.py` around lines 47 - 68, The malicious archive test is expecting tarfile.FilterError, but FilesystemSnapshot.restore is now swallowing that exception inside the per-member extraction loop in restore() and treating it as a partial failure. Update the restore() logic in agentwatch/rollback/engine.py so tarfile.FilterError from the tar extraction path is not caught by the generic exception handling, and instead is re-raised immediately while still preserving the existing partial-failure behavior for non-security extraction errors.
99-115: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winTest doesn't exercise the real partial-restoration code path.
test_rollback_result_tracks_partial_restorationonly validates aRollbackResultconstructed manually by_simulate_partial_restoration(), not actual behavior fromFilesystemSnapshot.restore()/RollbackEngine.rollback(). As noted onagentwatch/rollback/engine.py, the production code never setspartial_restoration=Truefor a real extraction failure, so this test provides false confidence. Once the root-cause fix is applied, add/adjust a test that triggers a genuine per-file extraction failure end-to-end.🤖 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 `@tests/test_rollback.py` around lines 99 - 115, The current rollback test is only checking a manually constructed RollbackResult, so it never exercises the real partial-restoration path. Update test_rollback_result_tracks_partial_restoration to drive a genuine failure through FilesystemSnapshot.restore() or RollbackEngine.rollback() instead of _simulate_partial_restoration(), and assert that partial_restoration becomes True from an actual per-file extraction failure. Use the existing RollbackResult, RollbackStatus, and rollback engine/snapshot restore flow to verify end-to-end behavior after the production fix.
🤖 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 `@agentwatch/rollback/engine.py`:
- Around line 123-169: The real partial-restoration state is never propagated
from FilesystemSnapshot.restore to RollbackEngine.rollback, so
result.partial_restoration and warnings stay unset for per-file extraction
failures. Update restore() to return or otherwise surface the partial failure
information alongside the restored file list, then wire that through rollback()
so a partial extract sets RollbackResult.partial_restoration=True and appends an
appropriate warning. Use the existing restore() and rollback() flow, plus
RollbackResult, as the main points to locate and connect the behavior.
- Around line 123-169: In restore(), the broad per-member exception handling is
swallowing tarfile.FilterError from tar.extract(..., filter="data"), which can
downgrade unsafe archives into partial restores. Update the _restore loop to
catch tarfile.FilterError separately and re-raise it immediately, while keeping
the existing partial-recovery handling only for non-security extraction
failures; use restore, _restore, and the tar.extract call as the key locations.
---
Duplicate comments:
In `@tests/test_rollback.py`:
- Around line 47-68: The malicious archive test is expecting
tarfile.FilterError, but FilesystemSnapshot.restore is now swallowing that
exception inside the per-member extraction loop in restore() and treating it as
a partial failure. Update the restore() logic in agentwatch/rollback/engine.py
so tarfile.FilterError from the tar extraction path is not caught by the generic
exception handling, and instead is re-raised immediately while still preserving
the existing partial-failure behavior for non-security extraction errors.
- Around line 99-115: The current rollback test is only checking a manually
constructed RollbackResult, so it never exercises the real partial-restoration
path. Update test_rollback_result_tracks_partial_restoration to drive a genuine
failure through FilesystemSnapshot.restore() or RollbackEngine.rollback()
instead of _simulate_partial_restoration(), and assert that partial_restoration
becomes True from an actual per-file extraction failure. Use the existing
RollbackResult, RollbackStatus, and rollback engine/snapshot restore flow to
verify end-to-end behavior after the production fix.
🪄 Autofix (Beta)
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: CHILL
Plan: Pro Plus
Run ID: edcb785f-a571-4272-9add-b8839bd499ad
📒 Files selected for processing (2)
agentwatch/rollback/engine.pytests/test_rollback.py
| async def restore( | ||
| snapshot_path: Path, | ||
| target_path: Path, | ||
| ) -> list[str]: | ||
| restored: list[str] = [] | ||
| partial: list[str] = [] | ||
|
|
||
| def _restore() -> None: | ||
| nonlocal restored, partial | ||
| extracted: list[str] = [] | ||
| with tarfile.open(snapshot_path, "r:gz") as tar: | ||
| members = tar.getmembers() | ||
| tar.extractall(str(target_path), filter="data") | ||
| restored.extend([m.name for m in members if m.isfile()]) | ||
| for member in members: | ||
| try: | ||
| tar.extract(member, str(target_path), filter="data") | ||
| if member.isfile(): | ||
| extracted.append(member.name) | ||
| except Exception: | ||
| logger.warning( | ||
| "Failed to extract %s from snapshot %s", | ||
| member.name, | ||
| snapshot_path, | ||
| ) | ||
| partial.append(member.name) | ||
| restored = extracted | ||
|
|
||
| await asyncio.get_event_loop().run_in_executor(None, _restore) | ||
| logger.info("Restored %d files from snapshot %s", len(restored), snapshot_path) | ||
|
|
||
| if partial: | ||
| logger.warning( | ||
| "Partially restored %d of %d files from snapshot %s (%d failures)", | ||
| len(restored), | ||
| len(restored) + len(partial), | ||
| snapshot_path, | ||
| len(partial), | ||
| ) | ||
| # Remove partially extracted files to avoid inconsistent state | ||
| for fname in partial: | ||
| fpath = target_path / fname | ||
| try: | ||
| if fpath.exists(): | ||
| fpath.unlink() | ||
| except Exception as exc: | ||
| logger.warning("Failed to clean up partial file %s: %s", fpath, exc) | ||
| else: | ||
| logger.info("Restored %d files from snapshot %s", len(restored), snapshot_path) | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
partial_restoration and extraction-failure warnings are never actually populated by the real restore flow.
FilesystemSnapshot.restore() tracks partial failures internally and logs a warning, but its return type stays list[str] — no information about partial failures is returned to the caller. Correspondingly, rollback() (lines 362-376) only appends a warning / never sets result.partial_restoration for the "snapshot completely missing" case; there is no code path where a real per-file extraction failure sets result.partial_restoration = True or adds a warning to result.warnings.
This means the PR's stated goal — surfacing degraded/partial filesystem restoration through RollbackResult — is unmet for the actual partial-extraction scenario; it's only met for the fully-missing-snapshot scenario. This is corroborated by tests/test_rollback.py's test_rollback_result_tracks_partial_restoration, which only asserts against a manually constructed RollbackResult via _simulate_partial_restoration() rather than exercising FilesystemSnapshot.restore()/RollbackEngine.rollback() end-to-end.
Consider returning failure info from restore() and wiring it through rollback():
♻️ Proposed fix to propagate partial-restoration state
`@staticmethod`
async def restore(
snapshot_path: Path,
target_path: Path,
- ) -> list[str]:
+ ) -> tuple[list[str], list[str]]:
restored: list[str] = []
partial: list[str] = []
...
- return restored
+ return restored, partial if restore_filesystem:
if cp.snapshot_path and cp.snapshot_path.exists():
- restored = await FilesystemSnapshot.restore(
+ restored, partial = await FilesystemSnapshot.restore(
snapshot_path=cp.snapshot_path,
target_path=cwd,
)
result.rolled_back_files = restored
+ if partial:
+ result.partial_restoration = True
+ result.warnings.append(
+ f"Failed to restore {len(partial)} file(s) from snapshot"
+ )
logger.info("Restored %d files from snapshot", len(restored))Also applies to: 362-376
🤖 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 `@agentwatch/rollback/engine.py` around lines 123 - 169, The real
partial-restoration state is never propagated from FilesystemSnapshot.restore to
RollbackEngine.rollback, so result.partial_restoration and warnings stay unset
for per-file extraction failures. Update restore() to return or otherwise
surface the partial failure information alongside the restored file list, then
wire that through rollback() so a partial extract sets
RollbackResult.partial_restoration=True and appends an appropriate warning. Use
the existing restore() and rollback() flow, plus RollbackResult, as the main
points to locate and connect the behavior.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Python 3.12 tarfile raise FilterError from TarFile.extract with filter="data" for path traversal members?
💡 Result:
In Python 3.12, while the 'data' filter is intended to provide protection against common security risks—including path traversal—it has been found to be insufficient against several bypass techniques [1][2]. Specifically, simply using filter="data" does not guarantee that a FilterError will be raised for all path traversal attempts. Multiple vulnerabilities have been identified (such as CVE-2025-4517 and CVE-2024-12718) where untrusted archives can bypass this filter, allowing for arbitrary filesystem writes or metadata modifications outside of the intended extraction directory [1][3][4]. Because of these security bypasses, Python versions 3.12.0 through 3.12.10 are considered affected [5]. The Python maintainers recommend upgrading to a patched version (e.g., 3.12.11 or later) to address these vulnerabilities [5]. If you cannot upgrade, it is recommended that you implement additional manual checks, such as verifying that no members contain insecure path segments (like "..") in their names or link targets, before proceeding with extraction [1][4]. While the FilterError exception exists in the tarfile module to signal that a filter has refused to extract a member, these security vulnerabilities demonstrate that the 'data' filter implementation can fail to detect or block all malicious path traversal vectors, thereby failing to raise the expected error [1][6][4].
Citations:
- 1: https://www.openwall.com/lists/oss-security/2025/06/24/1
- 2: https://mail.python.org/archives/list/security-announce@python.org/thread/MAXIJJCUUMCL7ATZNDVEGGHUMQMUUKLG/
- 3: https://nvd.nist.gov/vuln/detail/CVE-2025-4138
- 4: https://openwall.com/lists/oss-security/2025/06/24/1
- 5: https://test.osv.dev/vulnerability/CVE-2024-12718
- 6: https://github.com/python/peps/blob/main/peps/pep-0706.rst
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- engine.py outline ---\n'
ast-grep outline agentwatch/rollback/engine.py --view expanded || true
printf '\n--- test_rollback.py outline ---\n'
ast-grep outline tests/test_rollback.py --view expanded || true
printf '\n--- relevant engine.py lines ---\n'
nl -ba agentwatch/rollback/engine.py | sed -n '100,190p'
printf '\n--- relevant test_rollback.py lines ---\n'
nl -ba tests/test_rollback.py | sed -n '1,120p'Repository: sreerevanth/AgentWatch
Length of output: 251
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import tarfile, io, tempfile, pathlib, os, traceback
def make_tar(path, member_name, data=b'X'):
with tarfile.open(path, 'w:gz') as tar:
info = tarfile.TarInfo(member_name)
info.size = len(data)
tar.addfile(info, io.BytesIO(data))
cases = [
'../evil.txt',
'/tmp/evil.txt',
'safe/../../evil.txt',
'safe.txt',
]
with tempfile.TemporaryDirectory() as td:
td = pathlib.Path(td)
for name in cases:
p = td / ('a.tar.gz' if name == cases[0] else name.replace('/', '_').replace('..','dotdot') + '.tar.gz')
try:
make_tar(p, name)
except Exception as e:
print('make_tar failed', name, type(e).__name__, e)
continue
target = td / 'out'
target.mkdir()
with tarfile.open(p, 'r:gz') as tar:
print(f'\nCASE {name!r}')
for m in tar.getmembers():
try:
tar.extract(m, str(target), filter="data")
print(' extract: ok', m.name)
except Exception as e:
print(' extract:', type(e).__name__, e)
print(' bases:', [cls.__name__ for cls in type(e).__mro__[:4]])
PYRepository: sreerevanth/AgentWatch
Length of output: 551
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import tarfile, io, tempfile, pathlib
def make_tar(path, member_name, data=b'X'):
with tarfile.open(path, 'w:gz') as tar:
info = tarfile.TarInfo(member_name)
info.size = len(data)
tar.addfile(info, io.BytesIO(data))
cases = ['../evil.txt', '/tmp/evil.txt', 'safe/../../evil.txt', 'safe.txt']
with tempfile.TemporaryDirectory() as td:
td = pathlib.Path(td)
for name in cases:
p = td / (name.replace('/', '_').replace('..','dotdot') + '.tar.gz')
make_tar(p, name)
target = td / 'out'
target.mkdir(exist_ok=True)
with tarfile.open(p, 'r:gz') as tar:
print(f'CASE {name!r}')
for m in tar.getmembers():
try:
tar.extract(m, str(target), filter="data")
print(' extract ok')
except Exception as e:
print(' extract', type(e).__name__, str(e))
print(' mro', [c.__name__ for c in type(e).__mro__[:5]])
PYRepository: sreerevanth/AgentWatch
Length of output: 811
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import tarfile, io, tempfile, pathlib, os
print('tarfile.FilterError exists:', hasattr(tarfile, 'FilterError'))
print('FilterError MRO:', [c.__name__ for c in tarfile.FilterError.__mro__[:6]])
def make_tar(path, member_name, data=b'X'):
with tarfile.open(path, 'w:gz') as tar:
info = tarfile.TarInfo(member_name)
info.size = len(data)
tar.addfile(info, io.BytesIO(data))
cases = ['../evil.txt', '/tmp/evil.txt', 'safe/../../evil.txt', 'safe.txt']
with tempfile.TemporaryDirectory() as td:
td = pathlib.Path(td)
for name in cases:
p = td / (name.replace('/', '_').replace('..','dotdot') + '.tar.gz')
make_tar(p, name)
target = td / 'out'
target.mkdir(exist_ok=True)
with tarfile.open(p, 'r:gz') as tar:
member = tar.getmembers()[0]
print(f'CASE {name!r}: member={member.name!r}')
try:
tar.extract(member, str(target), filter='data')
print(' result: ok')
except Exception as e:
print(' result:', type(e).__name__, e)
PYRepository: sreerevanth/AgentWatch
Length of output: 361
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- agentwatch/rollback/engine.py ---'
nl -ba agentwatch/rollback/engine.py | sed -n '1,240p'
echo
echo '--- tests/test_rollback.py ---'
nl -ba tests/test_rollback.py | sed -n '1,220p'Repository: sreerevanth/AgentWatch
Length of output: 238
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for path in [Path('agentwatch/rollback/engine.py'), Path('tests/test_rollback.py')]:
print(f'--- {path} ---')
text = path.read_text()
for i, line in enumerate(text.splitlines(), 1):
if 1 <= i <= 260:
print(f'{i:4d}: {line}')
print()
PYRepository: sreerevanth/AgentWatch
Length of output: 14998
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for rel in ['agentwatch/rollback/engine.py', 'tests/test_rollback.py']:
p = Path(rel)
print(f'FILE {rel} exists={p.exists()} size={p.stat().st_size if p.exists() else "NA"}')
PY
echo
echo '--- grep for tarfile / malicious archive references ---'
grep -RIn --line-number -E 'malicious_archive_rejected|FilterError|filter="data"|tarfile' agentwatch tests || trueRepository: sreerevanth/AgentWatch
Length of output: 1234
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for path in [Path('agentwatch/rollback/engine.py'), Path('tests/test_rollback.py')]:
print(f'--- {path} ---')
if not path.exists():
print('missing')
continue
for i, line in enumerate(path.read_text().splitlines(), 1):
if i <= 260:
print(f'{i:4d}: {line}')
print()
PY
echo '--- grep for tarfile / malicious archive references ---'
grep -RIn --line-number -E 'malicious_archive_rejected|FilterError|filter="data"|tarfile' agentwatch tests || trueRepository: sreerevanth/AgentWatch
Length of output: 15963
Re-raise tarfile.FilterError in restore() The per-member except Exception swallows the data filter’s security exception and turns a malicious archive into a partial restore. Catch tarfile.FilterError separately and re-raise it so unsafe snapshots still fail fast.
🤖 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 `@agentwatch/rollback/engine.py` around lines 123 - 169, In restore(), the
broad per-member exception handling is swallowing tarfile.FilterError from
tar.extract(..., filter="data"), which can downgrade unsafe archives into
partial restores. Update the _restore loop to catch tarfile.FilterError
separately and re-raise it immediately, while keeping the existing
partial-recovery handling only for non-security extraction failures; use
restore, _restore, and the tar.extract call as the key locations.
1 participant
Closes #359
Summary
Fixes silently ignored failures during filesystem restoration in the Rollback Engine (#359).
Previously, if a snapshot tarball was missing, the rollback would silently mark
the operation as COMPLETED with no indication anything was wrong. Additionally,
if tar.extractall() partially extracted some files before failing, those files
remained on disk in an inconsistent state.
Key changes
agentwatch/rollback/engine.py:FilesystemSnapshot.restore()now extracts files one at a time, catchesper-member exceptions, and cleans up partially extracted files
rollback()emits a descriptive warning whensnapshot_pathis missingor was never created, rather than skipping silently
RollbackResultaddswarnings: list[str]andpartial_restoration: booltests/test_rollback.py— 4 new tests: missing snapshot warning, non-existentcheckpoint, partial restoration tracking, result state verification
Summary by CodeRabbit