ci: fail closed when README command discovery is empty - #41
codeforester merged 2 commits into
Conversation
| if in_shell_block: | ||
| in_shell_block = False | ||
| else: | ||
| in_shell_block = language in {"bash", "sh", "shell", "console"} |
There was a problem hiding this comment.
Fence language allowlist silently narrows command discovery (removed-behavior regression risk)
The old parser toggled in_shell_block on any ``` fence, regardless of language tag. The new parser only turns it on when the language is exactly one of {"bash", "sh", "shell", "console"} (line 29). A future README edit that documents a real northstar command inside a fence tagged e.g. zsh, cmd, powershell, text, or left untagged will have that command silently dropped from discovery — no error is raised unless the total also happens to dip below the < 10 floor in readme_commands(). This is a real narrowing of what used to be scanned, with no compensating warning for the "in a fence, but wrong/missing language tag" case.
Severity: correctness/robustness gap (recall-verified: CONFIRMED)
|
|
||
| def readme_commands() -> list[list[str]]: | ||
| commands = parse_readme_commands(README_PATH.read_text(encoding="utf-8")) | ||
| if len(commands) < 10: |
There was a problem hiding this comment.
Hardcoded "< 10" threshold hardcodes today's exact command count
readme_commands() raises unless at least 10 commands are discovered. Today's README happens to yield exactly 10 (2 + 4 + 1 + 3 across the four bash fences), so this passes with zero slack. Any legitimate future edit that trims even one documented example will trip this assertion, with an error message that doesn't explain why 10 is the expected number. Conversely, a parsing regression that happens to still emit ≥10 commands (e.g. by misparsing something else and getting lucky on count) would go undetected. Consider anchoring the check to something more meaningful than a magic literal (e.g. comparing against the currently-documented command set, or asserting non-zero plus a comment explaining the number's provenance).
Severity: altitude/fragility (recall-verified: CONFIRMED)
| command_line = command_line[2:].lstrip() | ||
| if re.match(r"^northstar(?:-[a-z0-9-]+)?(?:\s|$)", command_line): | ||
| commands.append(shlex.split(command_line)) | ||
| return commands |
There was a problem hiding this comment.
Duplicates existing command-discovery logic in tests/test_documentation.py
tests/test_documentation.py already has an independent mechanism for extracting $ northstar ... example commands from README.md (and docs/learning-path.md) via COMMAND_PATTERN = re.compile(r"^\$\s+(northstar(?:\s+.*)?)$") and documented_commands(). This PR adds a second, more complex fence-tracking parser (parse_readme_commands) for the same underlying goal, with different matching rules (fence/language-context-aware vs. plain line-start matching). Two independently-maintained parsers over the same source document can silently diverge on what counts as a "documented command," since each test file only validates its own parsed set. Worth consolidating into one shared parser, or at least cross-referencing so they can't drift apart unnoticed.
Severity: reuse/duplication (recall-verified: CONFIRMED)
| "LOCALAPPDATA": str(home / "home" / "AppData" / "Local"), | ||
| } | ||
| ) | ||
| shutil.copytree(EXAMPLES_PATH, home / "examples") |
There was a problem hiding this comment.
Unconditional copytree adds redundant I/O to every test invocation
shutil.copytree(EXAMPLES_PATH, home / "examples") now runs on every call to run_installed_command, which is exercised by all 13 tests in this file. Only the two --config examples/northstar-commerce.json commands actually need the examples/ directory; the other ~11 invocations (--help, status, the JSON-output tests, etc.) copy the tree for no reason. The cost is small today (one file), but it's wasted work on a hot path that will grow if examples/ grows. Consider copying only for commands that reference examples/, or copying once per test session instead of per invocation.
Severity: efficiency (recall-verified: CONFIRMED)
| tmp_path: Path, | ||
| ) -> None: | ||
| markdown = README_PATH.read_text(encoding="utf-8").replace( | ||
| "$ northstar --help", "$ northstar --definitely-invalid" |
There was a problem hiding this comment.
Fail-closed test relies on an unanchored substring replace that can silently no-op
README_PATH.read_text(...).replace("$ northstar --help", "$ northstar --definitely-invalid") matches an exact literal substring. If a future README edit rewords or removes that exact line (e.g. -h instead of --help, or trailing text added), .replace() becomes a silent no-op, parse_readme_commands returns the unmodified (valid) command list, and next(args for args in commands if "--definitely-invalid" in args) on line 96 raises StopIteration instead of meaningfully exercising the fail-closed path this test is meant to cover. Anchoring to a more structural marker (e.g. injecting an invalid flag into the first parsed command rather than string-replacing README prose) would make this test resilient to routine doc edits.
Severity: test brittleness (recall-verified: CONFIRMED)
1 participant
Fixes #29