Skip to content

Add simple filesystem converter using OFRAK#735

Open
rbs-jacob wants to merge 10 commits into
redballoonsecurity:masterfrom
rbs-jacob:feature/filesystem-converter
Open

Add simple filesystem converter using OFRAK#735
rbs-jacob wants to merge 10 commits into
redballoonsecurity:masterfrom
rbs-jacob:feature/filesystem-converter

Conversation

@rbs-jacob
Copy link
Copy Markdown
Member

  • I have reviewed the OFRAK contributor guide and attest that this pull request is in accordance with it.
  • I have made or updated a changelog entry for the changes in this pull request.

One sentence summary of this PR (This should go in the CHANGELOG!)

Add simple CLI filesystem converter using OFRAK.

Anyone you think should look at this, specifically?

@whyitfor

@rbs-jacob rbs-jacob requested a review from whyitfor April 10, 2026 00:59
@rbs-jacob rbs-jacob self-assigned this Apr 10, 2026
@whyitfor
Copy link
Copy Markdown
Contributor

@claude perform an initial review based on the projects coding style. What issues do you see? list them here for me.

@claude
Copy link
Copy Markdown

claude Bot commented Apr 30, 2026

Claude finished @whyitfor's task in 3m 8s —— View job


Code Review: feature/filesystem-converter

  • Read changed files
  • Review against project coding standards
  • Post findings

Issues Found

🔴 Bug: import tempfile312 as tempfile — typo causes ImportError

File: examples/test_fs_convert.py:2

import tempfile312 as tempfile  # ← should be: import tempfile

tempfile312 is not a standard library module or known package. This will cause an ImportError when tests are run. Fix this →


🔴 CHANGELOG regression: removes an existing entry

File: ofrak_core/CHANGELOG.md

The PR diff removes this line from master's CHANGELOG:

- Fix typo in `find` command used by CPIO packer (#740)

This is almost certainly a bad merge conflict resolution when master was merged into the feature branch. If this PR lands as-is, it will drop a released changelog entry. The PR also does not add a new entry for itself, which is required per CLAUDE.md and CONTRIBUTING.md. Fix this →%20restore%20the%20removed%20line%20%22Fix%20typo%20in%20find%20command%20used%20by%20CPIO%20packer%20(%23740)%22%20in%20the%20Unreleased%20section%2C%20and%202)%20add%20a%20new%20Added%20entry%20for%20%22Add%20simple%20CLI%20filesystem%20converter%20(fs_convert.py)%20to%20examples%22.&repo=redballoonsecurity/ofrak)


🔴 # pragma: no cover violates 100% coverage requirement

File: examples/fs_convert.py:50

def detect_format_from_extension(path: str) -> str:  # pragma: no cover

CLAUDE.md mandates 100% coverage enforced by fun-coverage. Using pragma: no cover is a workaround, not a fix. The root cause is that the function calls sys.exit() directly, which is hard to test without mocking (and mocking is prohibited per CLAUDE.md).

Recommendation: Raise a ValueError instead of calling sys.exit(). Move the sys.exit call to the __main__ block:

def detect_format_from_extension(path: str) -> str:
    ext = os.path.splitext(path)[1].lower()
    fmt = EXTENSION_MAP.get(ext)
    if fmt is None:
        raise ValueError(
            f"Cannot detect format from extension '{ext}'. "
            f"Use --to with one of: {', '.join(FORMATS.keys())}"
        )
    return fmt

Then in __main__, wrap in a try/except ValueError and call sys.exit. This makes the function fully testable. Fix this →


🟡 Temp directory leak in test helper

File: examples/test_fs_convert.py:55

async def create_source_archive(...) -> str:
    temp_dir = tempfile.mkdtemp()  # never cleaned up
    ...

mkdtemp() creates a directory that is never deleted. The function returns a path, so a context manager can't be used in-place — but the caller test_fs_convert already has access to tmp_path (a pytest fixture). Pass tmp_path into create_source_archive instead of creating a new temp dir:

async def create_source_archive(ofrak_context, source_tag, source_attrs, tmp_path) -> str:
    tree_dir = tmp_path / "tree"
    tree_dir.mkdir()
    ...
    archive_path = str(tmp_path / "source_archive")

Fix this →


🟡 Module-level mutable instance shared across test runs

File: examples/fs_convert.py:45-47

FORMAT_DEFAULT_VIEWS = {
    "cpio": CpioFilesystem(CpioArchiveType.NEW_ASCII),
}

A single CpioFilesystem instance is created at import time and reused by all callers of convert_filesystem. If add_view or any downstream code mutates this object, it will silently corrupt later calls. Consider making this a factory (dict of callables) or constructing the instance inside convert_filesystem. The inconsistency with the test is also worth noting: the test builds sources using AttributesType[CpioFilesystem](CpioArchiveType.NEW_ASCII) (via add_attributes), while the converter uses add_view(CpioFilesystem(...)). These are equivalent per the add_view implementation, but the asymmetry makes the code harder to follow.


🟢 Minor: sys imported but only used in the pragma: no cover path

File: examples/fs_convert.py:5

If detect_format_from_extension is refactored to raise ValueError (as suggested above), the import sys and sys.exit call become unnecessary in the library portion of the file. sys would only be needed in the __main__ block.


Summary

Severity Issue
🔴 Bug import tempfile312 — ImportError typo
🔴 Standards CHANGELOG entry removed + no new entry added
🔴 Standards # pragma: no cover bypasses required 100% coverage
🟡 Quality Temp directory leak in create_source_archive
🟡 Quality Module-level shared mutable view instance
🟢 Minor import sys unused after refactor

@rbs-jacob
Copy link
Copy Markdown
Member Author

rbs-jacob commented Apr 30, 2026

Wow that's mostly slop. Too bad. I'll fix the ~1 actually applicable comment in there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants