From f9c84c864b0e31bab0adbd4fb7f5c9dd54cca047 Mon Sep 17 00:00:00 2001 From: Dominikus Nold Date: Fri, 27 Mar 2026 00:18:20 +0100 Subject: [PATCH 1/2] fix: satisfy review gates for docs scripts and module_lifecycle typing - Replace print() with Rich Console in docs validation scripts (semgrep) - Split HTTP URL checks and doc scans to reduce cyclomatic complexity (radon) - Add icontract require/ensure on public helpers; use CliRunner() without mix_stderr - Cast questionary API for basedpyright reportUnknownMemberType Made-with: Cursor --- scripts/check-cross-site-links.py | 96 +++++++++++++------ scripts/check-docs-commands.py | 96 +++++++++++++------ src/specfact_cli/registry/module_lifecycle.py | 8 +- 3 files changed, 141 insertions(+), 59 deletions(-) diff --git a/scripts/check-cross-site-links.py b/scripts/check-cross-site-links.py index 9b371fb7..001f1482 100644 --- a/scripts/check-cross-site-links.py +++ b/scripts/check-cross-site-links.py @@ -4,18 +4,23 @@ from __future__ import annotations import argparse -import sys from pathlib import Path from urllib.error import HTTPError, URLError from urllib.parse import urlparse from urllib.request import Request, urlopen from beartype import beartype +from icontract import ensure +from rich.console import Console _REPO_ROOT = Path(__file__).resolve().parents[1] +_ERR = Console(stderr=True) +_OUT = Console() + _PREFIX = "https://modules.specfact.io" +_REDIRECT_CODES: frozenset[int] = frozenset({301, 302, 303, 307, 308}) @beartype @@ -56,34 +61,47 @@ def _collect_urls_from_markdown(text: str) -> list[str]: return cleaned +def _http_success_code(code: int | None) -> bool: + if code is None: + return False + c = int(code) + return 200 <= c < 400 + + +def _response_status(resp: object) -> int | None: + return getattr(resp, "status", None) or resp.getcode() # type: ignore[union-attr] + + @beartype -def _check_url(url: str, timeout_s: float) -> tuple[bool, str]: - parsed = urlparse(url) - if parsed.scheme != "https" or parsed.netloc != "modules.specfact.io": - return True, "skipped non-modules URL" +def _try_head_modules_url(url: str, timeout_s: float) -> tuple[bool, str] | None: + """Return a terminal result, or ``None`` to fall back to GET (e.g. HEAD 405).""" req = Request(url, method="HEAD", headers={"User-Agent": "specfact-docs-link-check/1.0"}) try: with urlopen(req, timeout=timeout_s) as resp: - code = getattr(resp, "status", None) or resp.getcode() - if code is not None and 200 <= int(code) < 400: + code = _response_status(resp) + if _http_success_code(code): return True, str(code) except HTTPError as exc: - if exc.code in {301, 302, 303, 307, 308}: + if exc.code in _REDIRECT_CODES: return True, str(exc.code) if exc.code != 405: return False, f"HTTP {exc.code}" except (URLError, OSError) as exc: return False, str(exc) + return None + +@beartype +def _try_get_modules_url(url: str, timeout_s: float) -> tuple[bool, str]: get_req = Request(url, headers={"User-Agent": "specfact-docs-link-check/1.0"}) try: with urlopen(get_req, timeout=timeout_s) as resp: - code = getattr(resp, "status", None) or resp.getcode() - if code is not None and 200 <= int(code) < 400: + code = _response_status(resp) + if _http_success_code(code): return True, str(code) return False, f"GET {code}" except HTTPError as exc: - if exc.code in {301, 302, 303, 307, 308}: + if exc.code in _REDIRECT_CODES: return True, str(exc.code) return False, f"HTTP {exc.code}" except (URLError, OSError) as exc: @@ -91,24 +109,20 @@ def _check_url(url: str, timeout_s: float) -> tuple[bool, str]: @beartype -def main() -> int: - parser = argparse.ArgumentParser(description=__doc__) - parser.add_argument( - "--warn-only", - action="store_true", - help="Print failures but exit 0 (for optional CI steps).", - ) - parser.add_argument("--timeout", type=float, default=25.0, help="HTTP timeout in seconds.") - args = parser.parse_args() +def _check_url(url: str, timeout_s: float) -> tuple[bool, str]: + parsed = urlparse(url) + if parsed.scheme != "https" or parsed.netloc != "modules.specfact.io": + return True, "skipped non-modules URL" + head = _try_head_modules_url(url, timeout_s) + if head is not None: + return head + return _try_get_modules_url(url, timeout_s) - docs_root = _REPO_ROOT / "docs" - if not docs_root.is_dir(): - print("check-cross-site-links: no docs/ directory", file=sys.stderr) - return 1 +@beartype +def _scan_cross_site_links(docs_root: Path, timeout: float) -> tuple[set[str], list[str]]: seen: set[str] = set() failures: list[str] = [] - for md_path in sorted(docs_root.rglob("*.md")): if "_site" in md_path.parts or "vendor" in md_path.parts: continue @@ -122,16 +136,40 @@ def main() -> int: if url in seen: continue seen.add(url) - ok, detail = _check_url(url, args.timeout) + ok, detail = _check_url(url, timeout) if not ok: failures.append(f"{rel}: {url} — {detail}") + return seen, failures + + +@beartype +@ensure(lambda result: result in (0, 1), "exit code must be 0 or 1") +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument( + "--warn-only", + action="store_true", + help="Print failures but exit 0 (for optional CI steps).", + ) + parser.add_argument("--timeout", type=float, default=25.0, help="HTTP timeout in seconds.") + args = parser.parse_args() + + docs_root = _REPO_ROOT / "docs" + if not docs_root.is_dir(): + _ERR.print("check-cross-site-links: no docs/ directory", markup=False) + return 1 + + seen, failures = _scan_cross_site_links(docs_root, args.timeout) if failures: - print("Cross-site link validation failed:", file=sys.stderr) + _ERR.print("Cross-site link validation failed:", markup=False) for line in failures: - print(line, file=sys.stderr) + _ERR.print(line, markup=False) return 0 if args.warn_only else 1 - print(f"check-cross-site-links: OK ({len(seen)} unique modules.specfact.io URL(s) checked)") + _OUT.print( + f"check-cross-site-links: OK ({len(seen)} unique modules.specfact.io URL(s) checked)", + markup=False, + ) return 0 diff --git a/scripts/check-docs-commands.py b/scripts/check-docs-commands.py index da29a7b0..e48620a6 100644 --- a/scripts/check-docs-commands.py +++ b/scripts/check-docs-commands.py @@ -9,9 +9,17 @@ import sys from pathlib import Path +from beartype import beartype +from icontract import ensure, require +from rich.console import Console +from typer.testing import CliRunner + _REPO_ROOT = Path(__file__).resolve().parents[1] +_ERR = Console(stderr=True) +_OUT = Console() + # Historical / illustrative pages: command lines are not guaranteed to match the current CLI. _EXCLUDED_DOC_PATHS: frozenset[str] = frozenset( { @@ -40,9 +48,6 @@ def _ensure_repo_path() -> None: _ensure_repo_path() -from beartype import beartype # noqa: E402 -from typer.testing import CliRunner # noqa: E402 - from specfact_cli.cli import app # noqa: E402 @@ -125,7 +130,10 @@ def _sanitize_command_tokens(tokens: list[str]) -> list[str]: @beartype +@require(lambda text: isinstance(text, str), "text must be a string") +@ensure(lambda result: isinstance(result, list), "must return a list") def collect_specfact_commands_from_text(text: str) -> list[list[str]]: + """Collect ``specfact …`` command token lists from Markdown *text*.""" commands: list[list[str]] = [] for body in _extract_code_block_bodies(text): for raw_line in body.splitlines(): @@ -137,48 +145,67 @@ def collect_specfact_commands_from_text(text: str) -> list[list[str]]: @beartype +def _eval_prefix_help(runner: CliRunner, prefix: list[str]) -> tuple[bool, str]: + """Return ``(True, "")`` if ``--help`` succeeds or the CLI is not installed; else ``(False, err)``.""" + result = runner.invoke(app, [*prefix, "--help"], catch_exceptions=True) + exc = getattr(result, "exception", None) + if result.exit_code == 0 and exc is None: + return True, "" + err = (getattr(result, "stdout", None) or "").strip() + if exc is not None: + last_err = f"{type(exc).__name__}: {exc!s}"[:800] + else: + last_err = err[:800] if err else f"exit {result.exit_code}" + combined = (err or last_err or "").lower() + if "not installed" in combined and "install" in combined: + return True, "" + return False, last_err + + +@beartype +@require( + lambda tokens: isinstance(tokens, list) and all(isinstance(t, str) for t in tokens), + "tokens must be a list of strings", +) +@ensure( + lambda result: ( + isinstance(result, tuple) and len(result) == 2 and isinstance(result[0], bool) and isinstance(result[1], str) + ), + "must return (bool, str)", +) def validate_command_tokens(tokens: list[str]) -> tuple[bool, str]: """True if some prefix of *tokens* is a valid CLI path (``… --help`` exits 0).""" tokens = _sanitize_command_tokens(tokens) if not tokens: return True, "" - runner = CliRunner(mix_stderr=False) + runner = CliRunner() last_err = "" for k in range(len(tokens), 0, -1): prefix = tokens[:k] - result = runner.invoke(app, [*prefix, "--help"], catch_exceptions=True) - exc = getattr(result, "exception", None) - if result.exit_code == 0 and exc is None: - return True, "" - err = (result.stderr or result.stdout or getattr(result, "output", None) or "").strip() - if exc is not None: - last_err = f"{type(exc).__name__}: {exc!s}"[:800] - else: - last_err = err[:800] if err else f"exit {result.exit_code}" - combined = (err or last_err or "").lower() - if "not installed" in combined and "install" in combined: + ok, msg = _eval_prefix_help(runner, prefix) + if ok: return True, "" + last_err = msg return False, last_err @beartype -def main() -> int: - docs_root = _REPO_ROOT / "docs" - if not docs_root.is_dir(): - print("check-docs-commands: no docs/ directory", file=sys.stderr) - return 1 +def _should_skip_markdown_path(rel: Path, rel_posix: str) -> bool: + if "_site" in rel.parts or "vendor" in rel.parts: + return True + return rel_posix.startswith("docs/migration/") or rel_posix in _EXCLUDED_DOC_PATHS + +@beartype +def _scan_docs_for_command_validation(docs_root: Path) -> tuple[set[tuple[str, ...]], list[str]]: seen: set[tuple[str, ...]] = set() failures: list[str] = [] - for md_path in sorted(docs_root.rglob("*.md")): - if "_site" in md_path.parts or "vendor" in md_path.parts: - continue rel = md_path.relative_to(_REPO_ROOT) rel_posix = rel.as_posix() - if rel_posix.startswith("docs/migration/") or rel_posix in _EXCLUDED_DOC_PATHS: + if _should_skip_markdown_path(rel, rel_posix): continue try: text = md_path.read_text(encoding="utf-8") @@ -193,13 +220,28 @@ def main() -> int: ok, msg = validate_command_tokens(tokens) if not ok: failures.append(f"{rel}: specfact {' '.join(tokens)} — {msg}") + return seen, failures + + +@beartype +@ensure(lambda result: result in (0, 1), "exit code must be 0 or 1") +def main() -> int: + docs_root = _REPO_ROOT / "docs" + if not docs_root.is_dir(): + _ERR.print("check-docs-commands: no docs/ directory", markup=False) + return 1 + + seen, failures = _scan_docs_for_command_validation(docs_root) if failures: - print("Docs command validation failed:", file=sys.stderr) + _ERR.print("Docs command validation failed:", markup=False) for line in failures: - print(line, file=sys.stderr) + _ERR.print(line, markup=False) return 1 - print(f"check-docs-commands: OK ({len(seen)} unique command prefix(es) checked)") + _OUT.print( + f"check-docs-commands: OK ({len(seen)} unique command prefix(es) checked)", + markup=False, + ) return 0 diff --git a/src/specfact_cli/registry/module_lifecycle.py b/src/specfact_cli/registry/module_lifecycle.py index aab85fef..b064af64 100644 --- a/src/specfact_cli/registry/module_lifecycle.py +++ b/src/specfact_cli/registry/module_lifecycle.py @@ -2,7 +2,7 @@ from __future__ import annotations -from typing import Any +from typing import Any, cast from beartype import beartype from icontract import ensure, require @@ -120,7 +120,8 @@ def _questionary_style() -> Any: import questionary # type: ignore[reportMissingImports] except ImportError: return None - return questionary.Style( + q = cast(Any, questionary) + return q.Style( [ ("qmark", "fg:#00af87 bold"), ("question", "bold"), @@ -209,7 +210,8 @@ def select_module_ids_interactive(action: str, modules_list: list[dict[str, Any] console.print(f"[cyan]{action_title} Modules[/cyan] (currently {current_state})") console.print("[dim]Controls: arrows navigate, space toggle, enter confirm[/dim]") display_to_id, choices = _checkbox_choices_for_modules(candidates) - selected: list[str] | None = questionary.checkbox( + q = cast(Any, questionary) + selected: list[str] | None = q.checkbox( f"{action_title} module(s):", choices=choices, instruction="(multi-select)", From d17869181adb9fbb70a868ce60839e93fdef3cba Mon Sep 17 00:00:00 2001 From: Dominikus Nold Date: Fri, 27 Mar 2026 00:26:23 +0100 Subject: [PATCH 2/2] fix(scripts): address #452 review (HTTP helpers, icontract, CLI streams) - _http_success_code: use int directly after None guard - _response_status: safe getcode via getattr/callable - check-docs: drop @require preconditions duplicated by beartype - _cli_invoke_streams_text: merge stdout + stderr for not-installed detection Made-with: Cursor --- scripts/check-cross-site-links.py | 11 ++++++++--- scripts/check-docs-commands.py | 30 +++++++++++++++++++++--------- 2 files changed, 29 insertions(+), 12 deletions(-) diff --git a/scripts/check-cross-site-links.py b/scripts/check-cross-site-links.py index 001f1482..7b5fb816 100644 --- a/scripts/check-cross-site-links.py +++ b/scripts/check-cross-site-links.py @@ -64,12 +64,17 @@ def _collect_urls_from_markdown(text: str) -> list[str]: def _http_success_code(code: int | None) -> bool: if code is None: return False - c = int(code) - return 200 <= c < 400 + return 200 <= code < 400 def _response_status(resp: object) -> int | None: - return getattr(resp, "status", None) or resp.getcode() # type: ignore[union-attr] + status = getattr(resp, "status", None) + if status is not None: + return status # type: ignore[no-any-return] + getcode = getattr(resp, "getcode", None) + if callable(getcode): + return getcode() # type: ignore[no-any-return] + return None @beartype diff --git a/scripts/check-docs-commands.py b/scripts/check-docs-commands.py index e48620a6..23bfd79a 100644 --- a/scripts/check-docs-commands.py +++ b/scripts/check-docs-commands.py @@ -10,7 +10,7 @@ from pathlib import Path from beartype import beartype -from icontract import ensure, require +from icontract import ensure from rich.console import Console from typer.testing import CliRunner @@ -130,7 +130,6 @@ def _sanitize_command_tokens(tokens: list[str]) -> list[str]: @beartype -@require(lambda text: isinstance(text, str), "text must be a string") @ensure(lambda result: isinstance(result, list), "must return a list") def collect_specfact_commands_from_text(text: str) -> list[list[str]]: """Collect ``specfact …`` command token lists from Markdown *text*.""" @@ -144,6 +143,23 @@ def collect_specfact_commands_from_text(text: str) -> list[list[str]]: return commands +def _cli_invoke_streams_text(result: object) -> str: + """Stdout + stderr text for a CliRunner ``Result`` (stderr via bytes when split, else safe).""" + out = (getattr(result, "stdout", None) or "").strip() + err = "" + stderr_bytes = getattr(result, "stderr_bytes", None) + if stderr_bytes is not None: + runner_obj = getattr(result, "runner", None) + charset = getattr(runner_obj, "charset", "utf-8") if runner_obj else "utf-8" + err = stderr_bytes.decode(charset, "replace").replace("\r\n", "\n").strip() + else: + try: + err = (getattr(result, "stderr", None) or "").strip() + except ValueError: + err = "" + return f"{out}\n{err}".strip() + + @beartype def _eval_prefix_help(runner: CliRunner, prefix: list[str]) -> tuple[bool, str]: """Return ``(True, "")`` if ``--help`` succeeds or the CLI is not installed; else ``(False, err)``.""" @@ -151,22 +167,18 @@ def _eval_prefix_help(runner: CliRunner, prefix: list[str]) -> tuple[bool, str]: exc = getattr(result, "exception", None) if result.exit_code == 0 and exc is None: return True, "" - err = (getattr(result, "stdout", None) or "").strip() + streams = _cli_invoke_streams_text(result) if exc is not None: last_err = f"{type(exc).__name__}: {exc!s}"[:800] else: - last_err = err[:800] if err else f"exit {result.exit_code}" - combined = (err or last_err or "").lower() + last_err = streams[:800] if streams else f"exit {result.exit_code}" + combined = (streams or last_err or "").lower() if "not installed" in combined and "install" in combined: return True, "" return False, last_err @beartype -@require( - lambda tokens: isinstance(tokens, list) and all(isinstance(t, str) for t in tokens), - "tokens must be a list of strings", -) @ensure( lambda result: ( isinstance(result, tuple) and len(result) == 2 and isinstance(result[0], bool) and isinstance(result[1], str)