feat(drive-arch): corrupt-FS hardening + survey subcommand (DA-002/DA-003) - #46
Conversation
…-002/DA-003) Scanner gains the guards from the corrupt hp-v210w postmortem and the 2TB pre-scan audit: capacity-sanity gate (a direntry claiming more bytes than the filesystem can hold is classified Corrupt Direntry and never opened), mojibake/undecodable-name detection with backslashreplace-safe JSONL serialization, symlink record-don't-traverse (a drive symlink to /home no longer walks the host filesystem into the catalog), repeatable --exclude globs, nested-archive depth cap (--max-archive-depth, extraction skipped past it), an output clobber guard (--force to override), itemized hidden/system skip reporting, and hardlink/cross-link duplicate marking with single extraction per inode. New 'drive-arch survey' subcommand: one stats-only walk with the same classifier — no JSONL, no hashing, no archive extraction by default — prints a category/extension table and a wipe/keep verdict that discloses everything the walk did not cover (hidden skips, unopened archives, symlinks, read errors, corruption). Classifier: RINEX short-name regex fallback so obs/nav/Hatanaka/met files outside the hardcoded .15-.22 year-extension list still classify as GNSS. 68 tests (17 new).
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR adds RINEX filename fallback classification, hardens the drive scanner against filesystem corruption (suspect names, oversized entries, symlinks, hardlinks, archive depth limits), adds a stats-only ChangesDrive Archaeologist Hardening and Survey Mode
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant DeepScanner
participant survey_verdict
User->>CLI: survey path --options
CLI->>DeepScanner: scan(stats_only=True)
DeepScanner-->>CLI: ScanStats (categories, corruption, GNSS counts)
CLI->>survey_verdict: compute verdict(ScanStats)
survey_verdict-->>CLI: verdict text, warnings
CLI-->>User: print category table and verdict
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
3,665 Leica raw files surfaced Unclassified on the first real GNSS-bearing drive — the .m00/.m01/... extension family cannot live in the static profile map, so classify via regex fallback like the RINEX short names. Counted in the survey GNSS verdict.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
tools/drive-archaeologist/src/drive_archaeologist/classifier.py (1)
10-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNit: ambiguous EN DASH in comment.
Ruff flags the
–(EN DASH) as ambiguous; consider using a regular hyphen for consistency with lint output.Suggested fix
-# The profile extension list only enumerates years .15–.22; real archives span +# The profile extension list only enumerates years .15-.22; real archives span🤖 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 `@tools/drive-archaeologist/src/drive_archaeologist/classifier.py` around lines 10 - 15, The comment above _RINEX_SHORT_RE uses an ambiguous EN DASH that Ruff flags; update the explanatory comment to use a normal hyphen instead of “.15–.22” while keeping the meaning unchanged. Make the wording consistent with the surrounding notes in classifier.py and leave the regex definition itself untouched.Source: Linters/SAST tools
tools/drive-archaeologist/src/drive_archaeologist/cli.py (3)
156-164: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
ext_by_catis built but never used.
ext_by_cat[category] = extsis assigned each iteration but the dict is never read afterward — dead code, and it forces the nestedclassify_by_extscan over all extensions for no benefit beyond the localextslist already used intable.add_row.♻️ Proposed fix
- ext_by_cat: dict[str, list[str]] = {} for category, count in stats.categories.most_common(top): exts = [ e for e, _ in stats.extensions.most_common() if scanner.classifier.classify_by_ext(e) == category ][:4] - ext_by_cat[category] = exts table.add_row(category, f"{count:,}", " ".join(exts))🤖 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 `@tools/drive-archaeologist/src/drive_archaeologist/cli.py` around lines 156 - 164, The `ext_by_cat` mapping in `build_stats_table` is dead code because it is only written to and never read. Remove the unused `ext_by_cat` dict assignment and keep the existing local `exts` list for `table.add_row`, or alternatively reuse that structure only if it is needed elsewhere in the function.
60-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMissing explicit return type hints on
scan/survey.Parameters are fully typed, but neither command function declares a return annotation (
-> None). As per coding guidelines, "Add type hints to function signatures for all public functions" for files underdrive_archaeologist/.Also applies to: 121-127
🤖 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 `@tools/drive-archaeologist/src/drive_archaeologist/cli.py` around lines 60 - 70, The public command functions scan and survey are missing explicit return annotations even though all parameters are already typed. Update both function signatures to declare a None return type, using the existing scan and survey definitions in cli.py, so they comply with the drive_archaeologist public API typing guideline.Source: Coding guidelines
144-149: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueBlind
except Exceptionflagged by Ruff (BLE001).Mirrors the existing pattern in
scan(line 94), so likely intentional as a top-level CLI safety net rather than a new defect. Consider narrowing or at least logging the exception type if you want to silence the linter, but not blocking.🤖 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 `@tools/drive-archaeologist/src/drive_archaeologist/cli.py` around lines 144 - 149, The top-level CLI exception handler is using a broad `except Exception` and is tripping Ruff’s BLE001. In the command block that already handles `KeyboardInterrupt`, either narrow the exception to expected error types or, if you keep the safety net, update the `except Exception as e` path to log the exception type alongside the message so the intent is clearer. Keep the existing `console.print` and `raise click.Abort()` behavior intact.Source: Linters/SAST tools
🤖 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 `@tools/drive-archaeologist/src/drive_archaeologist/scanner.py`:
- Around line 440-448: The verdict logic in scanner.py’s scan result path should
treat corrupt entries as unsafe, not just metadata_inconsistent. Update the
branch that builds the final verdict so any corrupt_entries (including
undecodable names) downgrades to the unreliable/manual-inspect message instead
of falling through to safe-to-wipe, and keep the CLI color logic in cli.py
consistent by including corrupt_entries in the red condition.
In `@tools/drive-archaeologist/tests/test_hardening.py`:
- Around line 79-91: The test test_undecodable_filename_recorded_not_opened
relies on creating a raw byte filename via os.open with b"bad\xff.zip", which is
only valid on Linux-like filesystems. Update this test to be skipped or
conditionally executed on non-Linux platforms, or refactor the setup to use a
portable surrogate-path approach, while keeping the assertions around run_scan,
read_jsonl, and scanner.stats.archives_seen intact.
In `@tools/drive-archaeologist/tests/test_survey.py`:
- Around line 34-42: The test verdict assertion in
test_verdict_safe_on_pure_media unpacks warnings from
DeepScanner.survey_verdict() but never uses it. Update
test_verdict_safe_on_pure_media to avoid the unused warnings variable by either
asserting on its contents or replacing the unpacking with only the verdict if
warnings are not needed; keep the check around DeepScanner, scan(), and
survey_verdict() aligned with the intended behavior.
---
Nitpick comments:
In `@tools/drive-archaeologist/src/drive_archaeologist/classifier.py`:
- Around line 10-15: The comment above _RINEX_SHORT_RE uses an ambiguous EN DASH
that Ruff flags; update the explanatory comment to use a normal hyphen instead
of “.15–.22” while keeping the meaning unchanged. Make the wording consistent
with the surrounding notes in classifier.py and leave the regex definition
itself untouched.
In `@tools/drive-archaeologist/src/drive_archaeologist/cli.py`:
- Around line 156-164: The `ext_by_cat` mapping in `build_stats_table` is dead
code because it is only written to and never read. Remove the unused
`ext_by_cat` dict assignment and keep the existing local `exts` list for
`table.add_row`, or alternatively reuse that structure only if it is needed
elsewhere in the function.
- Around line 60-70: The public command functions scan and survey are missing
explicit return annotations even though all parameters are already typed. Update
both function signatures to declare a None return type, using the existing scan
and survey definitions in cli.py, so they comply with the drive_archaeologist
public API typing guideline.
- Around line 144-149: The top-level CLI exception handler is using a broad
`except Exception` and is tripping Ruff’s BLE001. In the command block that
already handles `KeyboardInterrupt`, either narrow the exception to expected
error types or, if you keep the safety net, update the `except Exception as e`
path to log the exception type alongside the message so the intent is clearer.
Keep the existing `console.print` and `raise click.Abort()` behavior intact.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b04ce084-01b8-4040-ab63-5429e68eb6bf
📒 Files selected for processing (6)
tools/drive-archaeologist/src/drive_archaeologist/classifier.pytools/drive-archaeologist/src/drive_archaeologist/cli.pytools/drive-archaeologist/src/drive_archaeologist/scanner.pytools/drive-archaeologist/src/drive_archaeologist/utils/paths.pytools/drive-archaeologist/tests/test_hardening.pytools/drive-archaeologist/tests/test_survey.py
| if self.stats.gnss_files: | ||
| verdict = ( | ||
| f"{self.stats.gnss_files} GNSS-classified files — DO NOT wipe; " | ||
| "run a full scan and excavate first" | ||
| ) | ||
| elif self.stats.metadata_inconsistent: | ||
| verdict = "corrupt filesystem — verdict unreliable, inspect manually before wiping" | ||
| else: | ||
| verdict = "no GNSS payload detected — safe-to-wipe candidate (human confirms)" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Corrupt direntries do not downgrade the "safe-to-wipe" verdict.
metadata_inconsistent is only set by the capacity-overflow path (Line 302); a suspect/undecodable filename sets corrupt_reason="undecodable_name" and increments corrupt_entries without ever flipping metadata_inconsistent. As a result, a drive containing mojibake/undecodable entries (which are unclassifiable and could themselves be GNSS payload) falls through to "safe-to-wipe candidate". Given the wipe decision is destructive, corrupt entries should make the verdict unreliable rather than green.
Proposed fix
- elif self.stats.metadata_inconsistent:
+ elif self.stats.metadata_inconsistent or self.stats.corrupt_entries:
verdict = "corrupt filesystem — verdict unreliable, inspect manually before wiping"Note: the CLI color at cli.py (color = "red" if stats.gnss_files or stats.metadata_inconsistent else "green") has the same blind spot and would need the corresponding or stats.corrupt_entries to stay consistent.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if self.stats.gnss_files: | |
| verdict = ( | |
| f"{self.stats.gnss_files} GNSS-classified files — DO NOT wipe; " | |
| "run a full scan and excavate first" | |
| ) | |
| elif self.stats.metadata_inconsistent: | |
| verdict = "corrupt filesystem — verdict unreliable, inspect manually before wiping" | |
| else: | |
| verdict = "no GNSS payload detected — safe-to-wipe candidate (human confirms)" | |
| if self.stats.gnss_files: | |
| verdict = ( | |
| f"{self.stats.gnss_files} GNSS-classified files — DO NOT wipe; " | |
| "run a full scan and excavate first" | |
| ) | |
| elif self.stats.metadata_inconsistent or self.stats.corrupt_entries: | |
| verdict = "corrupt filesystem — verdict unreliable, inspect manually before wiping" | |
| else: | |
| verdict = "no GNSS payload detected — safe-to-wipe candidate (human confirms)" |
🤖 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 `@tools/drive-archaeologist/src/drive_archaeologist/scanner.py` around lines
440 - 448, The verdict logic in scanner.py’s scan result path should treat
corrupt entries as unsafe, not just metadata_inconsistent. Update the branch
that builds the final verdict so any corrupt_entries (including undecodable
names) downgrades to the unreliable/manual-inspect message instead of falling
through to safe-to-wipe, and keep the CLI color logic in cli.py consistent by
including corrupt_entries in the red condition.
| def test_undecodable_filename_recorded_not_opened(tmp_path): | ||
| root = tmp_path / "drive" | ||
| root.mkdir() | ||
| fd = os.open(os.path.join(str(root).encode(), b"bad\xff.zip"), os.O_CREAT | os.O_WRONLY) | ||
| os.write(fd, b"not really a zip") | ||
| os.close(fd) | ||
| out = tmp_path / "out.jsonl" | ||
| scanner = run_scan(root, out) | ||
| records = read_jsonl(out) # must not crash on the surrogate path | ||
| assert len(records) == 1 | ||
| assert records[0]["category"] == CORRUPT_CATEGORY | ||
| assert records[0]["corrupt_reason"] == "undecodable_name" | ||
| assert scanner.stats.archives_seen == 0 # suspect entries are never opened |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does macOS APFS allow filenames containing invalid UTF-8 byte sequences created via os.open with bytes path in Python?
💡 Result:
No, macOS APFS does not allow filenames containing invalid UTF-8 byte sequences. APFS explicitly requires filenames to be valid UTF-8 [1][2]. When you attempt to create a file with an invalid UTF-8 byte sequence using os.open (or other file system APIs) on an APFS-formatted volume, the operating system will reject the operation, typically returning an OSError with an "Illegal byte sequence" (errno 92) or an "Invalid argument" (errno 22) error [1][3][4]. This is a strict requirement of the APFS file system, which differs from some other file systems (such as ext4 or older HFS+ implementations in certain contexts) that may treat filenames as arbitrary byte sequences [5][6]. Because APFS enforces this constraint at the kernel/file system level, passing a bytes-encoded path that contains invalid UTF-8 to Python's os.open will fail on macOS when the target path resides on an APFS volume [1][4].
Citations:
- 1: https://bugs.python.org/issue31380
- 2: https://developer.apple.com/library/archive/documentation/FileManagement/Conceptual/APFS_Guide/FAQ/FAQ.html
- 3: https://stackoverflow.com/questions/76978022/os-open-with-non-utf8-characters-in-file-name
- 4: test_undecodable_filename() in Lib/test/test_httpservers.py broken on APFS python/cpython#75561
- 5: https://eclecticlight.co/2021/05/08/explainer-unicode-normalization-and-apfs/
- 6: https://unicodefyi.com/guide/unicode-in-filenames/
🏁 Script executed:
git ls-files .github/workflows tools/drive-archaeologist/tests/test_hardening.py && echo '---' && rg -n "runs-on:|ubuntu|windows|macos|pytest|drive-archaeologist" .github/workflows tools/drive-archaeologist/tests/test_hardening.pyRepository: alfieprojectsdev/movefaults
Length of output: 479
🏁 Script executed:
rg -n "skipif|sys\.platform|platform\.system|linux-only|APFS|os\.link|bad\\xff|pytest.mark" .Repository: alfieprojectsdev/movefaults
Length of output: 8567
🏁 Script executed:
cat -n tools/drive-archaeologist/tests/test_paths.py | sed -n '1,120p'
printf '\n---\n'
cat -n tools/drive-archaeologist/tests/test_scanner.py | sed -n '1,140p'Repository: alfieprojectsdev/movefaults
Length of output: 7903
🏁 Script executed:
git ls-files | rg -n '(^|/)(pyproject\.toml|tox\.ini|noxfile\.py|pytest\.ini|conftest\.py|README\.md|docs/drive-archaeologist/.*\.md|\.github/workflows/.*\.yml)$' && echo '---' && rg -n "Windows|macOS|APFS|Linux-only|cross-platform|platform\.system|skipif" pyproject.toml tox.ini noxfile.py pytest.ini README.md docs tools/drive-archaeologistRepository: alfieprojectsdev/movefaults
Length of output: 50386
Guard this test on non-Linux platforms tools/drive-archaeologist/tests/test_hardening.py:79-91 — b"bad\xff.zip" relies on raw byte filenames, which APFS and Windows won’t accept. Skip it outside Linux or switch to a portable surrogate-path setup.
🤖 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 `@tools/drive-archaeologist/tests/test_hardening.py` around lines 79 - 91, The
test test_undecodable_filename_recorded_not_opened relies on creating a raw byte
filename via os.open with b"bad\xff.zip", which is only valid on Linux-like
filesystems. Update this test to be skipped or conditionally executed on
non-Linux platforms, or refactor the setup to use a portable surrogate-path
approach, while keeping the assertions around run_scan, read_jsonl, and
scanner.stats.archives_seen intact.
| def test_verdict_safe_on_pure_media(tmp_path): | ||
| root = tmp_path / "drive" | ||
| root.mkdir() | ||
| make_media_tree(root) | ||
| scanner = DeepScanner(root, stats_only=True, include_hidden=True) | ||
| scanner.scan() | ||
| verdict, warnings = scanner.survey_verdict() | ||
| assert "safe-to-wipe candidate" in verdict | ||
| assert scanner.stats.gnss_files == 0 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Unused warnings variable.
warnings is unpacked but never used in this test.
🧹 Proposed fix
- verdict, warnings = scanner.survey_verdict()
+ verdict, _ = scanner.survey_verdict()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_verdict_safe_on_pure_media(tmp_path): | |
| root = tmp_path / "drive" | |
| root.mkdir() | |
| make_media_tree(root) | |
| scanner = DeepScanner(root, stats_only=True, include_hidden=True) | |
| scanner.scan() | |
| verdict, warnings = scanner.survey_verdict() | |
| assert "safe-to-wipe candidate" in verdict | |
| assert scanner.stats.gnss_files == 0 | |
| def test_verdict_safe_on_pure_media(tmp_path): | |
| root = tmp_path / "drive" | |
| root.mkdir() | |
| make_media_tree(root) | |
| scanner = DeepScanner(root, stats_only=True, include_hidden=True) | |
| scanner.scan() | |
| verdict, _ = scanner.survey_verdict() | |
| assert "safe-to-wipe candidate" in verdict | |
| assert scanner.stats.gnss_files == 0 |
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 40-40: Unpacked variable warnings is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 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 `@tools/drive-archaeologist/tests/test_survey.py` around lines 34 - 42, The
test verdict assertion in test_verdict_safe_on_pure_media unpacks warnings from
DeepScanner.survey_verdict() but never uses it. Update
test_verdict_safe_on_pure_media to avoid the unused warnings variable by either
asserting on its contents or replacing the unpacking with only the verdict if
warnings are not needed; keep the check around DeepScanner, scan(), and
survey_verdict() aligned with the intended behavior.
Source: Linters/SAST tools
fix(drive-arch): code-review findings on PR #46 hardening
1 participant
What
Implements DA-002 (corrupt-filesystem hardening) and DA-003 (
surveytriage subcommand) in one PR — DA-003 depends on DA-002's gates so its verdict can't be fooled the wayduwas on the corrupt hp-v210w stick (1.1 TB claimed on a 7.5 GB device).DA-002 — scanner hardening
Corrupt Direntry, never opened/extracted; running Σ(claimed) > capacity sets a metadata-inconsistent warning./home/...no longer walks the host filesystem into the catalog (found live on DOSTB20150918:repos/Qt4.8.7/Qt4.8.7 -> /home/finch/Qt4.8.7).--excludeglobs (repeatable), archive depth cap (--max-archive-depth, 0 = never extract), clobber guard (--forcerequired to overwrite an existing output), itemized skip reporting, hardlink/cross-link dup marking (single extraction per inode).DA-003 —
drive-arch survey <path>Stats-only walk with the same classifier: no JSONL, no hashing, no extraction by default. Prints category/extension table + wipe/keep verdict with explicit disclosures (hidden entries skipped, archives unopened, symlinks, read errors, corruption).
Classifier
RINEX short-name regex fallback (
ssssdddh.yy[ondgm]) —.23o,.05detc. now classify as GNSS Data despite being absent from the hardcoded.15–.22extension list.Tests
68 pass (17 new): capacity gate, mojibake handling, symlink non-traversal + loop, exclude globs, hidden-skip itemization +
--include-hidden, depth cap, clobber guard, hardlink dedup, RINEX fallback, survey verdicts + CLI end-to-end.Tickets
ticket_backlog.md: DA-002 (incl. audit addendum #7–#10), DA-003.🤖 Generated with Claude Code
https://claude.ai/code/session_015x6qfdkePCwviCoAhmUcD2
Summary by CodeRabbit
New Features
Bug Fixes
Tests