fix(quality): eliminate silent failures in cli_documentation scripts (#878 #879 #880)#886
fix(quality): eliminate silent failures in cli_documentation scripts (#878 #879 #880)#886
Conversation
Resolves #878 by propagating errors instead of swallowing them Resolves #879 by enforcing UTF-8 encoding in all file I/O Resolves #880 by validating required 'command' field in examples Changes: - Add DocumentationError exception class to models.py - example_manager.py: propagate ValueError, use encoding='utf-8', validate required fields - extractor.py: raise DocumentationError instead of printing warnings - hasher.py: use encoding='utf-8', raise on failure - sync_manager.py: use encoding='utf-8', narrow except clauses - scripts/__init__.py: new empty file making scripts/ a Python package - pyproject.toml: add pythonpath and dev dependencies for tests - Add comprehensive test suite in tests/unit/test_cli_documentation.py Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📊 Test Coverage Report — PR #886PR: Coverage Impact AssessmentThis PR adds 535 lines of new tests (41 test methods across 13 test classes) targeting the
Estimated coverage change: 44% → ~47–49% (+3–5%) ✅ This is a meaningful positive contribution toward the 80% target. Modules Covered by New Tests ✅
Modules Still Uncovered — High Priority for Next PR 🎯
What This PR Does Well ✅
Suggested Next Steps (Toward 80%)
These three additions could push coverage to ~55–58%, making solid progress toward the monthly goal of 52%. Progress Toward 80% Goal 📈Month 1 goal (44% → 52%): On track — this PR contributes ~3–5 points. One more focused test PR for Thank you for adding these regression tests alongside the bug fixes — this is exactly the kind of test-with-your-fix discipline that builds toward 80% coverage! 🎯
|
Dependency Review
New Dev Dependencies Added
Priority: Low — all changes are dev-only dependencies with no impact on production runtime. Key Changes
Risk Assessment
Action Items
|
Per TDD methodology (Step 7), write tests that:
- FAIL on main (DocumentationError does not yet exist in models.py)
- PASS once the implementation in this PR is merged
Test coverage (41 tests across 5 groups):
TestDocumentationError (3):
- DocumentationError is an Exception subclass
- Can be raised with a message
- Exported in models.__all__
TestCLIHasherErrorHandling (9):
- Corrupt JSON raises DocumentationError (not silent {})
- Missing file returns empty dict (first-run behaviour)
- JSONDecodeError is chained via 'from e' (SEC-R-14)
- save_hashes() uses encoding='utf-8' (SEC-R-10)
- Unicode hash round-trip
- OSError on save raises DocumentationError
TestExampleManagerValidation (16):
- Missing/empty/None 'command' field raises DocumentationError (SEC-R-11)
- Valid 'command' field returns examples
- load/save use encoding='utf-8' (SEC-R-10)
- Unicode content survives round-trip
- ValueError from _sanitize_command_name propagates (SEC-R-08)
- yaml.safe_load rejects !!python/object (SEC-R-09)
- Corrupt YAML raises DocumentationError
TestDocSyncManagerExceptionNarrowing (8):
- DocumentationError is caught; AttributeError/TypeError propagate (SEC-R-12)
- write_text() uses encoding='utf-8' (SEC-R-10)
- Path traversal rejected in _get_output_path
TestCLIExtractorSecurity (5):
- ALLOWED_MODULES whitelist unchanged (SEC-R-13)
- Unlisted module rejected
- Parse failure raises DocumentationError (not silent None)
- Missing command returns None (not found != error)
- yaml.load() absence verified in source
Also adds tests/conftest.py with repo-root sys.path setup so both
azlin and scripts.cli_documentation are importable during tests.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| import re | ||
|
|
||
| # Match 'yaml.load(' but not 'yaml.safe_load(' | ||
| forbidden = re.findall(r"\byaml\.load\s*\(", source) |
Check failure
Code scanning / CodeQL
Potentially uninitialized local variable Error test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 11 days ago
In general, to fix “potentially uninitialized local variable” issues, ensure the variable is definitely assigned along all code paths before it is used. This can be done by initializing it with a safe default before any conditional logic, or by restructuring the code so that any use of the variable is confined to blocks that only execute when assignment has succeeded.
For this specific test, the cleanest fix without changing behavior is to initialize source to None before the try block, and then only run the re.findall and assertion if source is not None. If inspect.getsource raises OSError, we already pytest.skip the test; if it raises any other exception, source will remain None and we should let that exception propagate (so the test errors rather than silently doing nothing). To preserve that behavior while satisfying the analyzer, we can add an else clause to the try/except and move the re import and subsequent logic into that else, which only executes if the try block succeeded and source is definitely initialized.
Concretely:
- In
tests/unit/test_cli_documentation.py, inTestCLIExtractorSecurity.test_yaml_load_not_used_in_source, wrap thereimport, theforbiddencomputation, and theassertinside atry/exceptelseblock, so they only run wheninspect.getsourcesucceeds. - No new imports or helper functions are required; we only restructure existing lines.
| @@ -797,13 +797,13 @@ | ||
| source = inspect.getsource(extractor_module) | ||
| except OSError: | ||
| pytest.skip("Source not available for inspection") | ||
| else: | ||
| # Bare yaml.load( calls (not yaml.safe_load) are forbidden | ||
| import re | ||
|
|
||
| # Bare yaml.load( calls (not yaml.safe_load) are forbidden | ||
| import re | ||
|
|
||
| # Match 'yaml.load(' but not 'yaml.safe_load(' | ||
| forbidden = re.findall(r"\byaml\.load\s*\(", source) | ||
| assert not forbidden, ( | ||
| "extractor.py must not call yaml.load() — use yaml.safe_load() only. " | ||
| f"Found: {forbidden}" | ||
| ) | ||
| # Match 'yaml.load(' but not 'yaml.safe_load(' | ||
| forbidden = re.findall(r"\byaml\.load\s*\(", source) | ||
| assert not forbidden, ( | ||
| "extractor.py must not call yaml.load() — use yaml.safe_load() only. " | ||
| f"Found: {forbidden}" | ||
| ) |
| msg = "corrupt JSON in .cli_doc_hashes.json" | ||
| with pytest.raises(DocumentationError) as exc_info: | ||
| raise DocumentationError(msg) | ||
| assert msg in str(exc_info.value) |
Check warning
Code scanning / CodeQL
Unreachable code Warning test
Copilot Autofix
AI 11 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| def test_exported_in_models_all(self) -> None: | ||
| """DocumentationError must be listed in models.__all__ so callers can | ||
| do 'from scripts.cli_documentation.models import DocumentationError'.""" | ||
| import scripts.cli_documentation.models as models_module |
Check notice
Code scanning / CodeQL
Module is imported with 'import' and 'import from' Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 11 days ago
General fix: avoid importing the same module both with from module import ... and import module. Keep one style. Since the file already uses from scripts.cli_documentation.models import (...) for all the needed classes, the best fix is to remove the second plain import and adjust the test_exported_in_models_all test to access __all__ without re-importing the module.
Concrete change in tests/unit/test_cli_documentation.py:
- Remove the line
import scripts.cli_documentation.models as models_moduleinsidetest_exported_in_models_all. - Replace uses of
models_modulein that test by something that doesn’t require a second import. The simplest approach that doesn’t change functionality is:- Build the expected
__all__as a list of names currently imported from the models module (e.g."CLIArgument","CLIMetadata","CLIOption","CommandExample","DocumentationError"). - Assert that
__all__exists on that module by importing it once at module level or, to avoid another import form, by importing it locally with the samefrompattern. However, CodeQL’s complaint is about mixingimportandfrom import; adding anotherfromis fine. But we can also avoid any additional imports by turning the test into a check thatDocumentationErrorcan be imported via thefromform, which is already implied by the test docstring. That’s even cleaner:- Use
import importlib(new standard-library import at top) andimportlib.import_module("scripts.cli_documentation.models")locally to obtain the module object in a way that doesn’t use a second import form for the same module.
- Use
- Build the expected
- This preserves behavior (we still verify that
models.__all__exists and contains"DocumentationError") while removing the duplicateimport/fromusage.
Specifically, adjust the body of test_exported_in_models_all:
-
Add
import importlibat the top of the file (new dependency, standard library). -
Replace:
import scripts.cli_documentation.models as models_module assert hasattr(models_module, "__all__"), "models.py must define __all__" assert "DocumentationError" in models_module.__all__, ( "DocumentationError must be in models.__all__" )
with:
import importlib models_module = importlib.import_module("scripts.cli_documentation.models") assert hasattr(models_module, "__all__"), "models.py must define __all__" assert "DocumentationError" in models_module.__all__, ( "DocumentationError must be in models.__all__" )
This way we only use from scripts.cli_documentation.models import ... and import the module object via importlib without a direct second import of the same module.
| @@ -30,6 +30,7 @@ | ||
| from pathlib import Path | ||
| from unittest.mock import MagicMock, PropertyMock, patch | ||
|
|
||
| import importlib | ||
| import pytest | ||
| import yaml | ||
|
|
||
| @@ -99,7 +100,7 @@ | ||
| def test_exported_in_models_all(self) -> None: | ||
| """DocumentationError must be listed in models.__all__ so callers can | ||
| do 'from scripts.cli_documentation.models import DocumentationError'.""" | ||
| import scripts.cli_documentation.models as models_module | ||
| models_module = importlib.import_module("scripts.cli_documentation.models") | ||
|
|
||
| assert hasattr(models_module, "__all__"), "models.py must define __all__" | ||
| assert "DocumentationError" in models_module.__all__, ( |
| the same guarantee within the pytest suite. | ||
| """ | ||
| import inspect | ||
| import scripts.cli_documentation.extractor as extractor_module |
Check notice
Code scanning / CodeQL
Module is imported with 'import' and 'import from' Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 11 days ago
In general, to fix this issue you remove either the from module import Name or the plain import module so that each module is only imported in one style. If you still need the imported name, you can reference it via the remaining module import (e.g., module.Name), or add a simple alias (Name = module.Name) right after the import.
Here, the cleanest fix is to stop using from scripts.cli_documentation.extractor import CLIExtractor and instead import only the module once, then reference CLIExtractor via extractor_module.CLIExtractor everywhere. Since the bottom of the file already uses a module import (import scripts.cli_documentation.extractor as extractor_module), we can move that import to the top and reuse it, eliminating the from ... import CLIExtractor statement. Concretely:
- At the top of
tests/unit/test_cli_documentation.py, remove thefrom scripts.cli_documentation.extractor import CLIExtractorline. - Add a single module-level import, e.g.
import scripts.cli_documentation.extractor as extractor_module, alongside the otherscripts.cli_documentation.*imports. - Replace all uses of
CLIExtractorin the file withextractor_module.CLIExtractor. - Remove the bottom local import in
test_yaml_load_not_used_in_sourceand use the already-importedextractor_modulethere (inspect.getsource(extractor_module)).
This keeps functionality identical (same class and module used) while satisfying the CodeQL rule and simplifying imports.
| @@ -49,7 +49,7 @@ | ||
| from scripts.cli_documentation.example_manager import ExampleManager | ||
| from scripts.cli_documentation.hasher import CLIHasher | ||
| from scripts.cli_documentation.sync_manager import DocSyncManager | ||
| from scripts.cli_documentation.extractor import CLIExtractor | ||
| import scripts.cli_documentation.extractor as extractor_module | ||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| @@ -715,7 +715,7 @@ | ||
| SEC-R-13: new modules must be explicitly reviewed before addition. | ||
| """ | ||
| expected = {"azlin.cli", "azlin.storage", "azlin.context"} | ||
| actual = set(CLIExtractor.ALLOWED_MODULES) | ||
| actual = set(extractor_module.CLIExtractor.ALLOWED_MODULES) | ||
| assert actual == expected, ( | ||
| f"ALLOWED_MODULES changed unexpectedly.\n" | ||
| f" Expected: {sorted(expected)}\n" | ||
| @@ -725,7 +725,7 @@ | ||
| def test_unlisted_module_rejected(self) -> None: | ||
| """extract_command() must return None (with a warning) when the module | ||
| path is not in ALLOWED_MODULES — this prevents arbitrary code execution.""" | ||
| extractor = CLIExtractor() | ||
| extractor = extractor_module.CLIExtractor() | ||
| result = extractor.extract_command("os.path", "join") | ||
| assert result is None, ( | ||
| "Modules not in ALLOWED_MODULES must be rejected (return None)" | ||
| @@ -768,7 +768,7 @@ | ||
| error (raises DocumentationError) — these two cases are distinct. | ||
| """ | ||
|
|
||
| extractor = CLIExtractor() | ||
| extractor = extractor_module.CLIExtractor() | ||
|
|
||
| # Mock the module so importlib.import_module succeeds, but the attribute | ||
| # is not a Click.Command instance (simulating a missing command). | ||
| @@ -791,7 +791,6 @@ | ||
| the same guarantee within the pytest suite. | ||
| """ | ||
| import inspect | ||
| import scripts.cli_documentation.extractor as extractor_module | ||
|
|
||
| try: | ||
| source = inspect.getsource(extractor_module) |
Dependency ReviewNote: This PR is a code quality fix, not a Dependabot dependency update. The dependency changes are minor dev-only additions introduced to support the new test suite. Dependency Changes
Priority: Low — all additions are dev-only dependencies ( Risk Assessment
Code Quality Review (Primary Purpose of PR)Issues resolved (#878, #879, #880):
Additional improvements noted:
One observation: RecommendationMerge — the changes are well-scoped, the test plan is solid (809 lines of new tests), and the dependency additions are minimal and appropriate. No blocking concerns.
|
📊 Test Coverage ReportPR: fix(quality): eliminate silent failures in cli_documentation scripts (#878 #879 #880) Coverage Impact Assessment
Estimated Coverage Change: 44% → ~47–49% (+3–5%) ✅ This PR adds 41 new tests across 809 lines of test code, targeting the Newly Covered Areas
Still Uncovered (Priority for Next PR)
Highest-impact next steps:
Quality Highlights ✅This PR goes beyond simple coverage — it adds security-validating tests:
Progress Toward 80% GoalMonth 1 Goal: 44% → 52% — this PR contributes meaningfully toward that target. Recommendation✅ No blocking coverage concerns. Coverage is moving in the right direction. For the next PR focused on coverage, consider targeting Coverage estimated via static diff analysis. For precise numbers, ensure
|
… in cli_documentation
_extract_from_click_command now raises DocumentationError instead of
returning None, so the `if metadata:` and `if sub_metadata:` guards in
the callers were unreachable dead code. Remove them and update the return
type annotation from `CLIMetadata | None` to `CLIMetadata`.
Also remove the redundant `{e}` interpolation in save_examples()
DocumentationError — the cause is already chained via `from e`.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ator hasher.py: replace bare `except Exception` with specific handlers for json.JSONDecodeError and OSError, each carrying a file-path-aware message — satisfies SEC-R-14 (exception chaining). validator.py: pass encoding='utf-8' to path.read_text() — closes the last remaining instance of the locale-dependent I/O pattern from #879. Resolves #878 (error-swallowing detail) Closes #879 (encoding='utf-8' — final file) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Dependency & PR Review
Dependency ChangesNew dev dependencies added to
Priority: Low — all additions are dev-only dependencies (not shipped in production), well-established packages, and required for the new test suite introduced by this PR. Risk Assessment:
Code Scanning Findings (GitHub Advanced Security)Four issues flagged in
Action Items:
Overall RecommendationThe production code changes (error propagation, UTF-8 encoding, required field validation) are solid quality improvements with good test coverage. The four code scanning findings are all confined to test code and are straightforward to fix before merging.
|
📊 Test Coverage Report — PR #88641/41 tests passing ✅ Coverage Summary (scripts/ package)
Overall scripts/ coverage: 22% (277/1240 lines) What This PR AddsThis PR introduces 41 new tests targeting the three quality issues (#878, #879, #880), with strong coverage on the modules it directly changes:
The uncovered lines in each of these modules are mostly secondary branches (e.g. Still Uncovered — High Priority for Follow-on Work
Progress Toward 80% GoalThis PR makes a significant positive contribution to the coverage baseline for the Recommended Next Steps (to reach 80%)
Great work on this PR! The TDD approach (writing failing tests first, then implementing) is exactly the right pattern. The 41 tests provide solid regression coverage for all three security/quality issues, and the coverage on the directly-modified modules is strong.
|
1 participant
Summary
Fixes three quality issues in the
scripts/cli_documentation/package:exceptblocks that previously swallowed errors now raiseDocumentationError, andValueErrorfrom_sanitize_command_nameis no longer caught silently.open()calls inhasher.py,example_manager.py,sync_manager.py, andvalidator.pynow explicitly useencoding="utf-8", ensuring consistent round-trip behaviour across platforms.example_manager.pynow validates that the requiredcommandfield is present in example data and raisesDocumentationErrorif it is missing.Additional supporting changes:
models.py: AddsDocumentationErrorexception class (exported via__all__)scripts/__init__.py: New empty file makingscripts/a proper Python packagepyproject.toml: Adds[tool.pytest.ini_options] pythonpath = ["."]and dev dependenciesuv.lock: Updated to reflect dependency changesRelated Issues
Closes #878
Closes #879
Closes #880
Step 16b: Outside-In Testing Results
Scenario 1 — Corrupt JSON raises DocumentationError (fix for #879)
Command:
python -c "from scripts.cli_documentation.hasher import CLIHasher; CLIHasher('/tmp/corrupt.json')"Result: PASS
Output:
DocumentationError: Hash file /tmp/corrupt.json is corrupt: Expecting property name...Scenario 2 — Missing hash file returns empty dict without error
Command:
python -c "CLIHasher('/tmp/nonexistent.json')"(file does not exist)Result: PASS
Output: No exception raised; hashes initialized to empty dict
Scenario 3 — Missing 'command' field raises DocumentationError (fix for #880)
Command:
ExampleManager.load_examples('vm')with YAML lacking 'command' fieldResult: PASS
Output:
DocumentationError: Example entry in 'vm.yaml' is missing required field 'command'Scenario 4 — Path traversal in command name raises ValueError (fix for #878)
Command:
ExampleManager.load_examples('../etc/passwd')Result: PASS
Output:
ValueError: Invalid command name: ../etc/passwdScenario 5 — UTF-8 unicode roundtrip works end-to-end (fix for #879)
Command: Write unicode YAML (日本語テスト héllo wörld), load via ExampleManager
Result: PASS
Output: Description preserved exactly after save/load roundtrip
All 41 unit tests pass. Fix iterations: 0 (no fixes required during outside-in testing).