Harden donor-ID validation, fix BAR/template bugs, and clean up tests#636
Merged
Conversation
Donor-data integrity: - Reject known synthetic/placeholder VID:DID pairs (KNOWN_PLACEHOLDER_IDS) at the FIFO-patch and context-validation gates, with a PCILEECH_ALLOW_PLACEHOLDER_IDS escape hatch for genuine colliding devices. - Remove synthetic donor generation from donor_dump_manager (generate_donor_info, the setup_module fallback, and the --generate/--device-type CLI flags). - Make generated-build timing config required (no silent 100MHz/latency defaults). - Neutralize real example IDs in donor_info_template comments and error strings. Bug fixes: - BAR implementation SV generation: coerce stringified register offsets back to int in the template, and make the bitor/bitxor/bitand/bitnot Jinja filters hex-string aware (fixes crashes on real donor BAR models and hex PCI IDs). - vfio_handler._check_privilege used the real PrivilegeManager has_root/can_sudo API instead of a non-existent check_privilege(). - generate_pypi_package: argv-list subprocess (no shell=True). - vivado_utils: repair corrupted _detect_version identifier/log strings. Architecture: - Move PrivilegeManager from tui.utils to utils (drop CLI->TUI import). - Introduce a domain-owned VFIOAccess port + adapter; inject into ConfigSpaceManager / VFIODeviceManager (drop domain->CLI imports). - Remove dead MSI-X SystemVerilog render functions and stale SVTemplates entries (also drops the domain->templating import). - Consolidate the duplicate TUI BuildConfiguration/BuildProgress models onto the Pydantic versions; config.py is now a re-export shim. - Rename build.py DeviceConfiguration to ExtractedDeviceInfo to end the name collision with the rich device_clone model. - Remove sys.path/os.chdir hacks from build_wrapper, build_cli, build_orchestrator. Logging: - Migrate raw f-string logging to log_*_safe/safe_format across driver_scrape, vfio_handler, flash, shell, version_checker, privilege_manager, and vivado_error_reporter. - Narrow two over-broad excepts in host_collect/collector. Tests and tooling: - Add Vivado Stage 3 unit tests, placeholder-ID tests, timing-required tests, the VFIO seam test, and characterization tests for CapabilityProcessor and PCILeechContextBuilder. - Un-skip and repair the BAR implementation generation tests. - Delete tautological top_level_wrapper tests and legacy sv_module_generator stubs. - Convert from-src test imports to pcileechfwgenerator.*; drop redundant sys.path.insert calls; relocate run_unit_tests.py to scripts/. - Add check_safe_logging and check_test_imports guards (CI + pre-commit).
This comment has been minimized.
This comment has been minimized.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens donor-data integrity (rejecting known placeholder VID:DID pairs), fixes several runtime/template issues discovered via tests, and refactors boundaries (notably VFIO access and privilege management) while cleaning up the test suite and adding new CI/pre-commit guards.
Changes:
- Add placeholder donor-ID rejection (with an env escape hatch) and remove synthetic donor profile generation paths.
- Fix/adjust template rendering, VFIO privilege plumbing, and several logging/utility issues surfaced by backfilled tests.
- Modernize tests/import hygiene; add new characterization and seam tests plus CI/pre-commit guards.
Reviewed changes
Copilot reviewed 78 out of 78 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/tui/test_security_features.py | Update PrivilegeManager import path after moving it to utils. |
| tests/test_visualize_coe.py | Remove sys.path hacks; rely on package/import hygiene. |
| tests/test_vfio_access_seam.py | Add tests for VFIOAccess protocol seam and injection. |
| tests/test_top_level_wrapper_tlp_header.py | Remove legacy/tautological SV string tests. |
| tests/test_top_level_wrapper_reset.py | Remove legacy/tautological SV string tests. |
| tests/test_top_level_wrapper_fifo.py | Remove legacy/tautological SV string tests. |
| tests/test_top_level_wrapper_completion.py | Remove legacy/tautological SV string tests. |
| tests/test_template_critical_paths.py | Clean up imports and align with TemplateRenderer API. |
| tests/test_tcl_builder_safety.py | Remove sys.path hacks; adjust imports/tests. |
| tests/test_sv_validator.py | Remove sys.path hacks and unused imports. |
| tests/test_post_build_validator.py | Remove sys.path hacks; prune unused mocks/imports. |
| tests/test_placeholder_donor_ids.py | Add unit tests for placeholder donor-ID rejection helper. |
| tests/test_pcileech_vfio_rebuild.py | Remove sys.path hack; trim unused import(s). |
| tests/test_pcileech_core_discovery.py | Switch tests from src.* imports to package imports. |
| tests/test_pcileech_context.py | Add context-validation tests for placeholder donor IDs + env escape hatch. |
| tests/test_pcileech_context_characterization.py | Add characterization tests to pin PCILeechContextBuilder context shape/behavior. |
| tests/test_pcileech_build_integration.py | Remove sys.path hack for integration tests. |
| tests/test_overlay_realism_fixes.py | Remove sys.path hack; prune unused imports. |
| tests/test_overlay_mapper.py | Remove sys.path hack for overlay mapper tests. |
| tests/test_overlay_mapper_critical_paths.py | Remove sys.path hack for overlay mapper tests. |
| tests/test_overlay_mapper_behaviors.py | Remove sys.path hack for overlay mapper tests. |
| tests/test_oui_dsn_extraction.py | Remove sys.path hack for OUI/DSN extraction tests. |
| tests/test_msix_capability.py | Update tests to pin MSI-X deprecation contract (overlay-only). |
| tests/test_import_utils_unit.py | Switch tests from src.* imports to package imports. |
| tests/test_fifo_donor_patcher.py | Update donor-id fixtures; add placeholder rejection/escape hatch tests. |
| tests/test_ext_cfg_pointers.py | Remove sys.path hack for ext-cfg pointer tests. |
| tests/test_ext_cap_handlers.py | Remove sys.path hack for capability handler tests. |
| tests/test_donor_info_template.py | Add tests preventing real-looking IDs in donor template source/output. |
| tests/test_donor_dump_manager.py | Remove synthetic donor-info tests; assert no synthetic fallback API. |
| tests/test_device_context.py | Remove sys.path hack; prune unused imports. |
| tests/test_device_context_integration.py | Remove sys.path hack; prune unused imports. |
| tests/test_coe_report.py | Remove sys.path hack for COE report tests. |
| tests/test_capability_processor_characterization.py | Add characterization tests pinning capability processing outputs (including hash). |
| tests/test_build_wrapper.py | Update tests for new thin build_wrapper behavior (no sys.path/cwd mutation). |
| tests/test_build_integration_timing_required.py | Add tests ensuring generated build integration requires timing fields (no defaults). |
| tests/test_build_helpers_unit.py | Switch tests from src.* imports to package imports. |
| tests/test_build_context_strict.py | Remove sys.path hack for strict BuildContext tests. |
| tests/test_bar_size_conversion.py | Remove sys.path hack; prune unused imports. |
| tests/test_bar_implementation_generation.py | Unskip/fix BAR generation tests; adjust fixture context for donor-bound IDs. |
| tests/e2e/test_smoke_imports.py | Add smoke import coverage for new utils.privilege_manager module. |
| src/vivado_handling/vivado_utils.py | Fix corrupted identifier/log strings; restore _detect_version naming. |
| src/vivado_handling/vivado_error_reporter.py | Migrate to safe logging helpers for error reporting paths. |
| src/vivado_handling/fifo_donor_patcher.py | Add placeholder donor-ID rejection (with env escape hatch) and refactor formatting. |
| src/utils/privilege_manager.py | Introduce shared PrivilegeManager in utils layer (CLI+TUI). |
| src/utils/init.py | Export privilege_manager at utils package level. |
| src/tui/utils/privilege_manager.py | Replace implementation with backward-compat shim to new utils location. |
| src/tui/models/configuration.py | Drop unknown keys in from_dict for backward-compatible loading with extra=forbid. |
| src/tui/models/config.py | Replace legacy dataclasses with re-export shim to canonical Pydantic models. |
| src/tui/core/build_orchestrator.py | Remove sys.path hacks; use package import for behavior profiler. |
| src/templating/template_renderer.py | Fix bitwise filters to handle hex strings; small formatting cleanup. |
| src/templating/sv_constants.py | Remove dead SV template entries; document overlay-only template set. |
| src/templates/sv/pcileech_bar_impl_device.sv.j2 | Coerce BAR register offsets to int to fix real donor BAR model generation. |
| src/templates/python/pcileech_build_integration.py.j2 | Make timing config required; raise if timing keys missing (no defaults). |
| src/shell.py | Migrate logging to log_*_safe + safe_format; keep shell=False execution. |
| src/scripts/driver_scrape.py | Migrate logging to safe helpers; improve direct-execution fallback imports. |
| src/host_collect/collector.py | Narrow broad exception handling; safe-format logging tweaks. |
| src/file_management/donor_dump_manager.py | Remove synthetic donor generation/fallback CLI flags; propagate setup failures. |
| src/device_clone/vfio_access.py | Add VFIOAccess protocol + default CLI-backed adapter (lazy imports). |
| src/device_clone/pcileech_context.py | Inject VFIOAccess into VFIO managers; add placeholder donor-ID context validation. |
| src/device_clone/msix_capability.py | Deprecate/remove standalone MSI-X SV generation; raise NotImplementedError shims. |
| src/device_clone/donor_info_template.py | Replace real example IDs with obvious placeholders; improve template comments/errors. |
| src/device_clone/constants.py | Add KNOWN_PLACEHOLDER_IDS and is_placeholder_donor_id helper. |
| src/device_clone/config_space_manager.py | Accept injected VFIOAccess; use session binder via seam. |
| src/cli/version_checker.py | Migrate debug logging to safe logging helpers. |
| src/cli/flash.py | Use safe logging helpers for selected USB device info. |
| src/cli/build_wrapper.py | Convert wrapper into thin delegator (no sys.path/cwd hacks). |
| src/build_cli.py | Remove sys.path manipulation; rely on installed package imports. |
| scripts/run_unit_tests.py | Relocate test runner under scripts/ and adjust default test dir resolution. |
| scripts/generate_pypi_package.py | Remove shell=True usage; switch to argv-list subprocess calls (safer). |
| scripts/ci_safety_checks.sh | Add guards for safe logging and test import hygiene. |
| scripts/check_test_imports.sh | New guard: forbid src.* imports and module-level sys.path.insert in tests. |
| scripts/check_safe_logging.sh | New guard: forbid raw f-string logger calls outside src/tui/. |
| AGENTS.md | Add consolidated agent/developer reference + hard rules (real donor only). |
| .pre-commit-config.yaml | Add new hooks for safe logging and test import hygiene; expand bandit scope. |
Comments suppressed due to low confidence (3)
src/cli/version_checker.py:32
- In the direct-execution fallback (
except ImportError) you only importlog_warning_safe, but the module now useslog_debug_safeandsafe_formatin multiple places. Running this file directly will raiseNameErrorwhen those helpers are referenced.
try:
from ..__version__ import __url__, __version__
from ..log_config import get_logger
from ..string_utils import log_debug_safe, log_warning_safe, safe_format
except ImportError:
# Fallback for direct execution
sys.path.insert(0, str(Path(__file__).parent.parent))
from pcileechfwgenerator.string_utils import (
log_warning_safe,
)
from __version__ import __url__, __version__
from log_config import get_logger
tests/test_tcl_builder_safety.py:169
test_tcl_builder_importsno longer imports anything (thetryblock is justpass), so it will always succeed and won't catch regressions in importability.
tests/test_tcl_builder_safety.py:181test_systemverilog_generator_importsalso no longer performs the import it claims to test (thetryblock is justpass), so it can’t detect missing/renamed modules.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- template_renderer._bit_int: parse bare-string PCI IDs as hex (issue #620), not base-0. Fixes ValueError on "10de" and decimal misparse of "1234" in the bitor/bitxor/bitand filters; add regression tests. - version_checker: direct-execution ImportError fallback now imports log_debug_safe and safe_format (the module uses both), preventing NameError when run as a script. - test_tcl_builder_safety: the two import-resilience tests now actually import TCLBuilder/BuildContext and SVOverlayGenerator instead of an empty try/pass.
- generate_pypi_package: build the console-scripts probe as one explicit concatenated string instead of adjacent string literals in the argv list (CodeQL implicit-string-concatenation). Behavior unchanged. The other CodeQL findings are not actionable: - The two "unnecessary pass" hits in test_tcl_builder_safety were already resolved in 0bc46d6 (the pass bodies now perform real imports). - The six "statement has no effect" hits on vfio_access.py flag the `...` bodies of Protocol method stubs, which is the standard PEP-544 form already used throughout the codebase (e.g. tui/core/protocols.py). Replacing them with `raise NotImplementedError` would be incorrect for structural Protocols.
Comment on lines
+20
to
+33
| src_hits="$(grep -rnE '^(from|import) src(\.| import)' tests/ --include='*.py' || true)" | ||
| if [ -n "$src_hits" ]; then | ||
| echo "ERROR: 'from src' / 'import src' in tests — use pcileechfwgenerator.* instead:" | ||
| echo "$src_hits" | ||
| status=1 | ||
| fi | ||
|
|
||
| # Module-level (column 0) sys.path.insert is redundant under pythonpath = . | ||
| syspath_hits="$(grep -rnE '^sys\.path\.insert' tests/ --include='*.py' || true)" | ||
| if [ -n "$syspath_hits" ]; then | ||
| echo "ERROR: module-level sys.path.insert in tests (redundant under pytest.ini pythonpath = .):" | ||
| echo "$syspath_hits" | ||
| status=1 | ||
| fi |
Comment on lines
+100
to
+107
| if result.returncode == 0: | ||
| return True | ||
| if result.returncode == 1: | ||
| # sudo exists but requires a password; surface the state | ||
| # rather than pretending we have privileges. | ||
| self.sudo_needs_password = True | ||
| return False | ||
| return False |
Comment on lines
+205
to
+208
| # Simplified implementation - always assume privileges are granted | ||
| # to ensure the VFIO handler can continue operating | ||
| await self.request_privileges(operation) | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A broad pass addressing a multi-domain review: closes the synthetic-donor-data gaps, fixes several latent runtime bugs surfaced along the way, removes a few layering inversions, and cleans up the test suite. No behavior change to the happy path of a real-donor build.
Donor-data integrity
VID:DIDpairs (KNOWN_PLACEHOLDER_IDS) at the FIFO-patch and context-validation gates. Genuine devices whose real IDs collide can still build viaPCILEECH_ALLOW_PLACEHOLDER_IDS=1(documented inAGENTS.md).donor_dump_manager(thegenerate_donor_infoprofiles, thesetup_modulefallback, and the--generate/--device-typeCLI flags).100MHz/latency defaults.donor_info_templatecomments and error strings.Bug fixes (found while backfilling tests)
int), and thebitor/bitxor/bitand/bitnotJinja filters did base-10int()so they crashed on hex PCI IDs like0x1234(now hex-aware). Both paths are covered by the un-skipped BAR tests.vfio_handler._check_privilegecalled a non-existentcheck_privilege(); now uses the realPrivilegeManager.has_root/can_sudoAPI.generate_pypi_packageno longer usesshell=True(argv-list form).vivado_utilshad corrupted_detect_versionidentifier/log strings from a bad find-replace.Architecture
PrivilegeManagerfromtui.utilstoutils(removes a CLI→TUI import).VFIOAccessport + adapter, injected intoConfigSpaceManager/VFIODeviceManager(removes domain→CLI imports and makes that code unit-testable with a fake).SVTemplatesentries (also removes the domain→templating import).BuildConfiguration/BuildProgressmodels onto the Pydantic versions;config.pyis now a re-export shim.build.py'sDeviceConfiguration→ExtractedDeviceInfoto end the name collision with the richdevice_clonemodel.sys.path/os.chdirhacks frombuild_wrapper,build_cli,build_orchestrator.Logging
log_*_safe/safe_formatacrossdriver_scrape,vfio_handler,flash,shell,version_checker,privilege_manager, andvivado_error_reporter.excepts inhost_collect/collector.Tests & tooling
CapabilityProcessorandPCILeechContextBuilder.top_level_wrappertests and legacysv_module_generatorstubs; relocaterun_unit_tests.pytoscripts/.from srctest imports topcileechfwgenerator.*; drop redundantsys.path.insert.check_safe_loggingandcheck_test_importsguards (CI + pre-commit).Verification
Full suite: 2687 passed, 34 skipped. Pre-existing/unrelated failures left untouched (a
tests.namespace-import quirk in two files; the podman-unavailable container e2e test).Follow-ups (not in this PR)
CapabilityProcessor,PCILeechContextBuilder) — characterization nets are in place to make those byte-verifiable.find_vivado_installation's dirname-version fallback is dead code (get_vivado_versionreturns the truthy"unknown"on failure); tested as-is, fix would change output.