Add fuzzing #928
Add fuzzing #928
Conversation
WalkthroughAdds Hypothesis-based fuzz tests for manifest handling, introduces a SAFE_STR regex to forbid NULL/control characters in manifest string fields, adds Hypothesis as a dev dependency, updates documentation and changelog, and adds Changes
Sequence DiagramsequenceDiagram
actor Hypothesis
participant Strategy as Manifest Strategy
participant Validator as StrictYAML Validator
participant Manifest as Manifest Model
participant CLI as dfetch Commands
Hypothesis->>Strategy: generate manifest-like data
Strategy-->>Hypothesis: return candidate data
Hypothesis->>Validator: serialize/validate against schema
alt Valid
Validator-->>Hypothesis: validation passed
Hypothesis->>Manifest: instantiate Manifest (may raise KeyError)
alt Instantiation success
Manifest-->>Hypothesis: instance created
Hypothesis->>CLI: run check/update (in temp dir)
CLI-->>Hypothesis: success or DfetchFatalException (caught)
else KeyError
Hypothesis-->>Hypothesis: tolerate/ignore KeyError
end
else Invalid
Validator-->>Hypothesis: fail -> trigger shrinking
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
🔇 Additional comments (12)
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 |
fix typo
ignore list should never be empty
![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
🤖 Fix all issues with AI agents
In @dfetch/manifest/schema.py:
- Around line 7-8: The SAFE_STR regex currently only blocks NUL but the comment
claims it blocks all control chars; update the SAFE_STR pattern so it also
rejects ASCII control ranges 0x00–0x1F and 0x7F–0x9F (i.e., disallow bytes with
hex values 00..1F and 7F..9F) and adjust the comment to match, or if the
intended behavior is to only block NUL, change the comment to state that only
NUL bytes are disallowed.
In @tests/test_fuzzing.py:
- Line 79: The generated ignore_list is always present because it's defined as
st.lists(..., min_size=1); make it optional by allowing None like the other
optional fields: replace the current ignore_list assignment with
st.one_of(st.none(), st.lists(SAFE_TEXT, min_size=1, max_size=5)) so the dict
comprehension can filter out None and omit the ignore key; apply the same change
for the other occurrence of ignore_list referenced in the comment.
🧹 Nitpick comments (1)
tests/test_fuzzing.py (1)
146-152: Consider broadening exception handling for robustness.The test only catches
KeyError, but other exceptions likeValueError,TypeError, orstrictyamlvalidation errors may be raised duringManifestconstruction. If these are expected failures for invalid fuzzed input, they should also be suppressed.Suggested improvement
@given(manifest_strategy) def test_manifest_can_be_created(data): """Validate by attempting to construct a Manifest.""" - try: - Manifest(data) - except KeyError: - pass + with suppress(KeyError, ValueError, TypeError): + Manifest(data)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.gitignoreCHANGELOG.rstdfetch/manifest/schema.pydoc/manifest.rstpyproject.tomltests/test_fuzzing.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_fuzzing.py (5)
dfetch/__main__.py (1)
DfetchFatalException(25-26)dfetch/manifest/manifest.py (3)
Manifest(103-356)remotes(281-283)projects(260-262)dfetch/util/util.py (1)
in_directory(66-75)dfetch/manifest/remote.py (1)
url(78-80)dfetch/manifest/project.py (1)
vcs(432-434)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build / build (windows-latest)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (12)
.gitignore (1)
17-17: LGTM!The
.hypothesisdirectory is correctly ignored. This is a standard practice for projects using Hypothesis, as it stores test data and examples that should not be committed to version control.CHANGELOG.rst (1)
7-8: LGTM!The changelog entries clearly document the new fuzzing capability and the stricter manifest validation. The issue references (#819, #114) provide good traceability.
dfetch/manifest/schema.py (2)
10-33: Systematic replacement approach is good.The replacement of
Str()withSAFE_STRacross all string fields in bothREMOTE_SCHEMAandPROJECT_SCHEMAis thorough and consistent. This provides centralized validation once the regex pattern is corrected.
3-3: LGTM!The import changes correctly reflect the shift from
Str()toRegex-based validation.doc/manifest.rst (1)
20-22: Documentation is correct, but implementation is incomplete.The documentation correctly states that strings should not contain NULL or control characters. However, the current
SAFE_STRregex indfetch/manifest/schema.pyonly blocks NULL bytes, not control characters. The implementation must be updated to match this documentation (see comment on schema.py).pyproject.toml (1)
89-89: No action required—Hypothesis 6.150.0 is the current stable release and has no known security vulnerabilities.Version 6.150.0 is the latest release on PyPI (released January 6, 2026) with no public security advisories or high-severity CVEs. The pinned version is current and secure.
tests/test_fuzzing.py (6)
1-18: LGTM!The imports are well-organized and appropriate for the fuzzing test suite.
20-39: LGTM!The Hypothesis profile configuration is well-structured for different environments. The
deadline=Nonesetting is appropriate given the filesystem and subprocess operations in these tests.
117-128: LGTM!The manifest strategy structure aligns well with the expected manifest schema format.
131-136: LGTM!Clean helper function that leverages StrictYAML's validation.
155-174: LGTM!Both test functions follow a consistent and appropriate pattern for fuzzing CLI commands with generated manifests. The use of
suppress(DfetchFatalException)correctly handles expected failures from invalid manifest configurations.
177-194: LGTM!The main block provides useful utilities for manual testing and debugging. The round-trip demonstration validates the strategy produces valid data.
One minor note:
manifest_strategy.example()(line 181) bypasses Hypothesis's shrinking and database, but this is acceptable here for demonstration purposes.
Summary by CodeRabbit
Bug Fixes
Documentation
Tests
Chores
Changelog
✏️ Tip: You can customize this high-level summary in your review settings.