Skip to content

fix(rollback): capture and report silent filesystem restoration failures #535

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
DebasmitaBose0 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(rollback): capture and report silent filesystem restoration failures #535

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 57 additions & 11 deletions agentwatch/rollback/engine.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,8 @@ class RollbackResult:
error: str | None = None
duration_seconds: float | None = None
completed_at: datetime | None = None
warnings: list[str] = field(default_factory=list)
partial_restoration: bool = False


class FilesystemSnapshot:
Expand Down Expand Up @@ -118,17 +120,53 @@ def filter_fn(member: tarfile.TarInfo) -> tarfile.TarInfo | None:
return out_path

@staticmethod
async def restore(snapshot_path: Path, target_path: Path) -> list[str]:
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)

Comment on lines +123 to +169

@coderabbitai coderabbitai Bot Jul 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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:

🏁 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]])
PY

Repository: 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]])
PY

Repository: 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)
PY

Repository: 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()
PY

Repository: 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 || true

Repository: 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 || true

Repository: 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.

return restored


Expand Down Expand Up @@ -321,13 +359,21 @@ async def rollback(
logger.info("Rolled back git to %s", cp.git_commit_ref[:8])

# Filesystem rollback
if restore_filesystem and cp.snapshot_path and cp.snapshot_path.exists():
restored = await FilesystemSnapshot.restore(
snapshot_path=cp.snapshot_path,
target_path=cwd,
)
result.rolled_back_files = restored
logger.info("Restored %d files from snapshot", len(restored))
if restore_filesystem:
if cp.snapshot_path and cp.snapshot_path.exists():
restored = await FilesystemSnapshot.restore(
snapshot_path=cp.snapshot_path,
target_path=cwd,
)
result.rolled_back_files = restored
logger.info("Restored %d files from snapshot", len(restored))
else:
msg = (
f"Filesystem snapshot for checkpoint {checkpoint_id} "
f"is missing or was never created"
)
result.warnings.append(msg)
logger.warning(msg)

result.status = RollbackStatus.COMPLETED

Expand Down
55 changes: 54 additions & 1 deletion tests/test_rollback.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,13 @@

import pytest

from agentwatch.rollback.engine import FilesystemSnapshot
from agentwatch.rollback.engine import (
Checkpoint,
CheckpointType,
FilesystemSnapshot,
RollbackEngine,
RollbackStatus,
)


def test_valid_archive_restore(tmp_path: Path):
Expand Down Expand Up @@ -60,3 +66,50 @@ def test_malicious_archive_rejected(tmp_path: Path):

# Ensure nothing was extracted to our target path unexpectedly
assert not (target_path / "etc" / "passwd").exists()


def test_rollback_missing_snapshot_adds_warning(tmp_path: Path):
"""Test that rollback with missing snapshot returns a warning."""
engine = RollbackEngine(checkpoints_dir=tmp_path / ".agentwatch" / "checkpoints")
cp = Checkpoint(
checkpoint_id="ckpt-missing",
session_id="session-1",
step_number=1,
checkpoint_type=CheckpointType.FILESYSTEM,
snapshot_path=tmp_path / "nonexistent" / "snapshot.tar.gz",
working_dir=str(tmp_path),
)
engine._checkpoints["ckpt-missing"] = cp
engine._session_checkpoints["session-1"] = ["ckpt-missing"]

result = asyncio.run(engine.rollback("ckpt-missing", restore_git=False))
assert result.status == RollbackStatus.COMPLETED
assert len(result.warnings) == 1
assert "missing" in result.warnings[0].lower()


def test_rollback_missing_checkpoint_returns_failed():
"""Test that rolling back a non-existent checkpoint returns FAILED."""
engine = RollbackEngine()
result = asyncio.run(engine.rollback("ckpt-nonexistent"))
assert result.status == RollbackStatus.FAILED
assert result.error is not None


def test_rollback_result_tracks_partial_restoration():
"""Test RollbackResult partial_restoration flag and warnings list."""
result = asyncio.run(_simulate_partial_restoration())
assert result.status == RollbackStatus.COMPLETED
assert result.partial_restoration is True
assert len(result.warnings) > 0


async def _simulate_partial_restoration() -> RollbackResult:
"""Helper to verify RollbackResult tracks partial state."""
result = RollbackResult(
checkpoint_id="ckpt-p",
status=RollbackStatus.COMPLETED,
warnings=["Some files could not be restored"],
partial_restoration=True,
)
return result
Loading