test_apply_pack_oserror_integrity.py
python
sha256:b636f72dcba9e190afb980bece906fa5b717fbde014b76ef023df8cb96e01eb9
docs: expand cache plan with all seven testing tiers and do…
Sonnet 4.6
131 days ago
| 1 | """Tests for Bug 13: apply_pack propagates unhandled OSError from write_commit. |
| 2 | |
| 3 | Root cause: apply_pack's commit loop catches (KeyError, ValueError, TypeError) |
| 4 | but NOT OSError. write_commit raises OSError("Store integrity violation") when |
| 5 | an existing commit file contains a DIFFERENT commit_id than the incoming one — |
| 6 | i.e. an impostor file. When this condition exists in the local store, apply_pack |
| 7 | propagates the unhandled OSError and the entire pull/push/clone call stack crashes |
| 8 | with an unhandled exception rather than logging CRITICAL and continuing. |
| 9 | |
| 10 | Concrete attack / failure scenario: |
| 11 | 1. Local store has commit file abc.msgpack containing impostor bytes |
| 12 | (different commit_id inside the file than the filename implies). |
| 13 | 2. A bundle arrives containing the legitimate abc commit. |
| 14 | 3. write_commit reads the existing file, detects commit_id mismatch, raises |
| 15 | OSError("Store integrity violation"). |
| 16 | 4. apply_pack (before fix) propagates the OSError → muse pull / muse clone |
| 17 | crashes with an unhandled exception. |
| 18 | |
| 19 | Scope of tests |
| 20 | -------------- |
| 21 | - apply_pack with impostor commit file does not crash (no uncaught OSError) |
| 22 | - apply_pack logs CRITICAL when OSError is raised by write_commit |
| 23 | - apply_pack continues processing remaining commits after OSError on one |
| 24 | - apply_pack correctly reports commits_written for non-affected commits |
| 25 | - The impostor file is NOT overwritten by the bundle commit (safety) |
| 26 | - apply_pack raises ValueError (not OSError) for commits with wrong hash |
| 27 | """ |
| 28 | from __future__ import annotations |
| 29 | |
| 30 | import datetime |
| 31 | import logging |
| 32 | import pathlib |
| 33 | |
| 34 | import msgpack |
| 35 | import pytest |
| 36 | |
| 37 | from muse.core._types import fake_id |
| 38 | from muse.core.pack import MPackBundle as PackBundle, apply_mpack as apply_pack |
| 39 | from muse.core.snapshot import compute_commit_id, compute_snapshot_id |
| 40 | from muse.core.store import ( |
| 41 | CommitDict, |
| 42 | CommitRecord, |
| 43 | SnapshotRecord, |
| 44 | commit_path, |
| 45 | read_commit, |
| 46 | write_commit, |
| 47 | write_snapshot, |
| 48 | ) |
| 49 | |
| 50 | _TS = datetime.datetime(2024, 6, 15, 10, 0, 0, tzinfo=datetime.timezone.utc) |
| 51 | |
| 52 | |
| 53 | def _make_repo(tmp_path: pathlib.Path) -> pathlib.Path: |
| 54 | repo = tmp_path / "repo" |
| 55 | repo.mkdir() |
| 56 | (repo / ".muse").mkdir() |
| 57 | (repo / ".muse" / "commits").mkdir() |
| 58 | (repo / ".muse" / "snapshots").mkdir() |
| 59 | return repo |
| 60 | |
| 61 | |
| 62 | def _make_commit_record(message: str, parent: str | None = None) -> CommitRecord: |
| 63 | manifest = {f"{message}.py": fake_id("obj")} |
| 64 | snap_id = compute_snapshot_id(manifest) |
| 65 | parent_ids = [parent] if parent else [] |
| 66 | cid = compute_commit_id( |
| 67 | repo_id="test-repo", |
| 68 | parent_ids=parent_ids, |
| 69 | snapshot_id=snap_id, |
| 70 | message=message, |
| 71 | committed_at_iso=_TS.isoformat(), |
| 72 | author="tester", |
| 73 | ) |
| 74 | return CommitRecord( |
| 75 | commit_id=cid, |
| 76 | repo_id="test-repo", |
| 77 | created_on_branch="main", |
| 78 | snapshot_id=snap_id, |
| 79 | message=message, |
| 80 | committed_at=_TS, |
| 81 | author="tester", |
| 82 | parent_commit_id=parent, |
| 83 | parent2_commit_id=None, |
| 84 | ) |
| 85 | |
| 86 | |
| 87 | def _bundle_with_commits(commits: list[CommitRecord]) -> PackBundle: |
| 88 | return PackBundle(objects=[], snapshots=[], commits=[c.to_dict() for c in commits], tags=[]) |
| 89 | |
| 90 | |
| 91 | def _write_impostor_file(repo: pathlib.Path, path_commit_id: str, content_commit_id: str) -> None: |
| 92 | """Write a commit file at path_commit_id.msgpack that contains content_commit_id inside.""" |
| 93 | impostor_data = { |
| 94 | "commit_id": content_commit_id, |
| 95 | "repo_id": "impostor", |
| 96 | "branch": "main", |
| 97 | "snapshot_id": "0" * 64, |
| 98 | "message": "impostor", |
| 99 | "committed_at": _TS.isoformat(), |
| 100 | "parent_commit_id": None, |
| 101 | "parent2_commit_id": None, |
| 102 | "author": "attacker", |
| 103 | } |
| 104 | path = commit_path(repo, path_commit_id) |
| 105 | path.parent.mkdir(parents=True, exist_ok=True) |
| 106 | path.write_bytes(msgpack.packb(impostor_data, use_bin_type=True)) |
| 107 | |
| 108 | |
| 109 | # ────────────────────────────────────────────────────────────────────────────── |
| 110 | # Bug 13: unhandled OSError from write_commit |
| 111 | # ────────────────────────────────────────────────────────────────────────────── |
| 112 | |
| 113 | class TestApplyPackOsErrorIntegrity: |
| 114 | |
| 115 | def test_apply_pack_does_not_crash_on_store_integrity_violation(self, tmp_path: pathlib.Path) -> None: |
| 116 | """Bug 13: apply_pack must not propagate OSError from write_commit.""" |
| 117 | repo = _make_repo(tmp_path) |
| 118 | c = _make_commit_record("good-commit") |
| 119 | |
| 120 | # Poison the store: write an impostor at c.commit_id's path |
| 121 | _write_impostor_file(repo, c.commit_id, "f" * 64) |
| 122 | |
| 123 | bundle = _bundle_with_commits([c]) |
| 124 | # Before the fix: apply_pack would raise OSError("Store integrity violation") |
| 125 | # After the fix: apply_pack must return normally (logs CRITICAL, skips commit) |
| 126 | result = apply_pack(repo, bundle) # must not raise |
| 127 | assert result is not None |
| 128 | |
| 129 | def test_apply_pack_logs_critical_on_store_integrity_violation( |
| 130 | self, tmp_path: pathlib.Path, caplog: pytest.LogCaptureFixture |
| 131 | ) -> None: |
| 132 | """apply_pack must log CRITICAL (not swallow silently) when OSError is raised.""" |
| 133 | repo = _make_repo(tmp_path) |
| 134 | c = _make_commit_record("critical-log-commit") |
| 135 | _write_impostor_file(repo, c.commit_id, "e" * 64) |
| 136 | |
| 137 | bundle = _bundle_with_commits([c]) |
| 138 | with caplog.at_level(logging.CRITICAL, logger="muse.core.pack"): |
| 139 | apply_pack(repo, bundle) |
| 140 | |
| 141 | crits = [r for r in caplog.records if r.levelno >= logging.CRITICAL] |
| 142 | assert crits, "apply_pack must log CRITICAL when write_commit raises OSError" |
| 143 | assert any("integrity" in r.message.lower() or "violation" in r.message.lower() |
| 144 | for r in crits) |
| 145 | |
| 146 | def test_apply_pack_continues_after_integrity_violation(self, tmp_path: pathlib.Path) -> None: |
| 147 | """Remaining commits must be processed after an integrity-violation skip.""" |
| 148 | repo = _make_repo(tmp_path) |
| 149 | bad = _make_commit_record("bad-commit") |
| 150 | good = _make_commit_record("good-commit-after") |
| 151 | |
| 152 | # Poison only the bad commit's file |
| 153 | _write_impostor_file(repo, bad.commit_id, "d" * 64) |
| 154 | |
| 155 | bundle = _bundle_with_commits([bad, good]) |
| 156 | result = apply_pack(repo, bundle) |
| 157 | |
| 158 | # good commit must be written despite the preceding integrity violation |
| 159 | assert read_commit(repo, good.commit_id) is not None, ( |
| 160 | "apply_pack must continue processing commits after an integrity violation" |
| 161 | ) |
| 162 | |
| 163 | def test_apply_pack_commits_written_excludes_integrity_violation(self, tmp_path: pathlib.Path) -> None: |
| 164 | """commits_written must not include commits that triggered OSError.""" |
| 165 | repo = _make_repo(tmp_path) |
| 166 | bad = _make_commit_record("integrity-violation") |
| 167 | good = _make_commit_record("normal-commit") |
| 168 | |
| 169 | _write_impostor_file(repo, bad.commit_id, "c" * 64) |
| 170 | |
| 171 | bundle = _bundle_with_commits([bad, good]) |
| 172 | result = apply_pack(repo, bundle) |
| 173 | |
| 174 | # Only the good commit should be counted as written |
| 175 | assert result["commits_written"] == 1, ( |
| 176 | f"commits_written should be 1 (only good), got {result['commits_written']}" |
| 177 | ) |
| 178 | |
| 179 | def test_impostor_file_not_overwritten_by_apply_pack(self, tmp_path: pathlib.Path) -> None: |
| 180 | """apply_pack must NOT replace the impostor file — that would hide the violation.""" |
| 181 | repo = _make_repo(tmp_path) |
| 182 | c = _make_commit_record("legit-commit") |
| 183 | impostor_id = "b" * 64 |
| 184 | _write_impostor_file(repo, c.commit_id, impostor_id) |
| 185 | |
| 186 | bundle = _bundle_with_commits([c]) |
| 187 | apply_pack(repo, bundle) |
| 188 | |
| 189 | # The file still contains the impostor — write_commit's OSError branch |
| 190 | # does NOT overwrite (only write_commit with a valid incoming record |
| 191 | # and a corrupt EXISTING file overwrites via the except Exception branch). |
| 192 | # apply_pack catches the OSError and skips — no write happens. |
| 193 | path = commit_path(repo, c.commit_id) |
| 194 | raw = msgpack.unpackb(path.read_bytes(), raw=False) |
| 195 | assert raw["commit_id"] == impostor_id, ( |
| 196 | "apply_pack must not overwrite an impostor file — that would hide the " |
| 197 | "integrity violation from the user" |
| 198 | ) |
| 199 | |
| 200 | def test_apply_pack_valid_commit_no_integrity_violation(self, tmp_path: pathlib.Path) -> None: |
| 201 | """Regression: valid commits must still be written normally.""" |
| 202 | repo = _make_repo(tmp_path) |
| 203 | c = _make_commit_record("clean-commit") |
| 204 | bundle = _bundle_with_commits([c]) |
| 205 | result = apply_pack(repo, bundle) |
| 206 | |
| 207 | assert result["commits_written"] == 1 |
| 208 | assert read_commit(repo, c.commit_id) is not None |
| 209 | |
| 210 | def test_apply_pack_wrong_hash_raises_valuerror_not_oserror(self, tmp_path: pathlib.Path) -> None: |
| 211 | """Commits with mismatched commit_id hash raise ValueError (caught separately).""" |
| 212 | repo = _make_repo(tmp_path) |
| 213 | # Bad record: commit_id doesn't match content hash |
| 214 | bad_dict = CommitDict( |
| 215 | commit_id="f" * 64, # wrong hash |
| 216 | repo_id="test-repo", |
| 217 | created_on_branch="main", |
| 218 | snapshot_id="a" * 64, |
| 219 | message="bad", |
| 220 | committed_at=_TS.isoformat(), |
| 221 | parent_commit_id=None, |
| 222 | parent2_commit_id=None, |
| 223 | author="tester", |
| 224 | ) |
| 225 | bundle = PackBundle(objects=[], snapshots=[], commits=[bad_dict], tags=[]) |
| 226 | result = apply_pack(repo, bundle) # must not raise |
| 227 | assert result["commits_written"] == 0 |
File History
3 commits
sha256:b636f72dcba9e190afb980bece906fa5b717fbde014b76ef023df8cb96e01eb9
docs: expand cache plan with all seven testing tiers and do…
Sonnet 4.6
131 days ago
sha256:7f9e2ef5286aedad9c1e6011b4c46ca27f39dbdad6e3409357e36b26e46b3b7c
docs: docstring sprint for-each-ref→hotspots — idiomatic ru…
Sonnet 4.6
patch
137 days ago
sha256:a09b1b4f6838754495547f200aa0ce88e2f56ffc5b20b900f6f0cff2c3cdede9
fix(cursorignore): remove git-ism (.git/worktrees)
Human
minor
⚠
140 days ago