From 2d5136a8191cd8dd7c8c3017f8d1b0e4f83b2aa0 Mon Sep 17 00:00:00 2001 From: Mark2Mac Date: Fri, 31 Jul 2026 10:44:20 +0200 Subject: [PATCH 1/2] fix(cli,supply-chain): parse package.json as JSON, and send fatal errors to stderr MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two correctness fixes, both found while driving the CLI from automation. package.json was scanned line by line. A manifest written on a single line — valid JSON, and what several generators emit — never entered the dependency section, so it produced *no* dependencies at all and the file passed silently. That is not noise, it is blindness: the scanner reports nothing and the caller cannot tell the difference from a clean manifest. It is now parsed as JSON. Version extraction is unchanged, including the caret handling, which is a separate question (#302): only the parsing changes. Line numbers survive the switch — the entry is located from the section header onwards, so a name that also appears in "scripts" does not steal the position — and a manifest that does not parse still falls back to the previous scan rather than going blind. Fatal errors were printed with the default Rich console, which writes to stdout. Anything driving the CLI from a script separates the two streams, so the only diagnosis available was discarded: a scan that failed left an empty error log and nothing to act on. Concretely, "Error: unsupported baseline version 1" — which is exactly the message a user needs after upgrading — arrived on stdout. Errors now go to a stderr console. Tests: one-line manifest, compact manifest, line numbers preserved, a name shadowed by "scripts", invalid JSON falling back, a non-object manifest, and non-string specs ignored. Full suite: 1567 passed, 14 skipped, 6 xfailed. Signed-off-by: Mark2Mac --- src/skillspector/cli.py | 16 +++--- .../analyzers/static_patterns_supply_chain.py | 51 +++++++++++++++++-- tests/unit/test_patterns_new.py | 48 +++++++++++++++++ 3 files changed, 104 insertions(+), 11 deletions(-) diff --git a/src/skillspector/cli.py b/src/skillspector/cli.py index aa1ed6581..ca6dfb699 100644 --- a/src/skillspector/cli.py +++ b/src/skillspector/cli.py @@ -71,6 +71,10 @@ def _ensure_utf8_streams() -> None: ) console = Console() +# Fatal errors go to stderr. Anything driving the CLI from a script separates the two streams, +# and with the message on stdout the only diagnosis available was thrown away: a failed scan +# left an empty error log and the caller had nothing to act on. +err_console = Console(stderr=True) class FormatChoice(StrEnum): @@ -379,13 +383,13 @@ def scan( except typer.Exit: raise except (FileNotFoundError, ValueError) as e: - console.print(f"[red]Error:[/red] {e}") + err_console.print(f"[red]Error:[/red] {e}") raise typer.Exit(code=2) from e except Exception as e: if verbose: console.print_exception() else: - console.print(f"[red]Error:[/red] {e}") + err_console.print(f"[red]Error:[/red] {e}") raise typer.Exit(code=2) from e finally: if result is not None: @@ -445,7 +449,7 @@ def _scan_multi_skill( severity = result.get("risk_severity") or "LOW" console.print(f" Score: {score}/100 ({severity})\n") except Exception as e: - console.print(f" [red]Error:[/red] {e}\n") + err_console.print(f" [red]Error:[/red] {e}\n") execution_failed = True results.append({"skill_name": skill.name, "error": str(e)}) @@ -559,7 +563,7 @@ def mcp( run_mcp(transport=transport.value, host=host, port=port) except ModuleNotFoundError as e: - console.print(f"[red]Error:[/red] {e}") + err_console.print(f"[red]Error:[/red] {e}") raise typer.Exit(code=2) from e @@ -633,13 +637,13 @@ def baseline( except typer.Exit: raise except (FileNotFoundError, ValueError) as e: - console.print(f"[red]Error:[/red] {e}") + err_console.print(f"[red]Error:[/red] {e}") raise typer.Exit(code=2) from e except Exception as e: if verbose: console.print_exception() else: - console.print(f"[red]Error:[/red] {e}") + err_console.print(f"[red]Error:[/red] {e}") raise typer.Exit(code=2) from e finally: if result is not None: diff --git a/src/skillspector/nodes/analyzers/static_patterns_supply_chain.py b/src/skillspector/nodes/analyzers/static_patterns_supply_chain.py index 63a6b55e2..ece8391d7 100644 --- a/src/skillspector/nodes/analyzers/static_patterns_supply_chain.py +++ b/src/skillspector/nodes/analyzers/static_patterns_supply_chain.py @@ -28,6 +28,7 @@ from __future__ import annotations +import json import os import re import sys @@ -547,8 +548,23 @@ def _extract_packages_from_requirements(content: str) -> list[tuple[str, str | N return results -def _extract_packages_from_package_json(content: str) -> list[tuple[str, str | None, int]]: - """Extract (package_name, version_or_None, line_number) from package.json content.""" +_NPM_DEPENDENCY_SECTIONS = ("dependencies", "devDependencies", "peerDependencies") + + +def _package_json_line(content: str, section: str, name: str) -> int: + """Best-effort line for a dependency entry, so findings keep pointing somewhere useful. + + Parsing JSON loses positions, and the search starts at the section header so a name that + also appears in ``scripts`` does not win. + """ + header = re.search(rf'"{re.escape(section)}"\s*:', content) + start = header.end() if header else 0 + entry = re.compile(rf'"{re.escape(name)}"\s*:').search(content, start) + return get_line_number(content, entry.start()) if entry else 1 + + +def _extract_packages_from_package_json_scan(content: str) -> list[tuple[str, str | None, int]]: + """Line-oriented fallback, used only when the manifest is not valid JSON.""" results: list[tuple[str, str | None, int]] = [] in_deps = False for i, line in enumerate(content.splitlines(), 1): @@ -562,9 +578,34 @@ def _extract_packages_from_package_json(content: str) -> list[tuple[str, str | N if in_deps: m = re.match(r'"([^"]+)"\s*:\s*"([^"]*)"', stripped) if m: - name = m.group(1) - version = _pinned_npm_version(m.group(2)) - results.append((name, version, i)) + results.append((m.group(1), _pinned_npm_version(m.group(2)), i)) + return results + + +def _extract_packages_from_package_json(content: str) -> list[tuple[str, str | None, int]]: + """Extract (package_name, version_or_None, line_number) from package.json content. + + package.json is JSON, so it is parsed as JSON. Scanning it line by line made the result + depend on formatting: a manifest written on a single line — which is valid, and what many + generators emit — never entered the dependency section at all and yielded *no* dependencies, + silently. The line-oriented scan remains as a fallback for manifests that do not parse. + """ + try: + data = json.loads(content) + except (ValueError, TypeError): + return _extract_packages_from_package_json_scan(content) + if not isinstance(data, dict): + return [] + results: list[tuple[str, str | None, int]] = [] + for section in _NPM_DEPENDENCY_SECTIONS: + deps = data.get(section) + if not isinstance(deps, dict): + continue + for name, spec in deps.items(): + if not isinstance(name, str) or not isinstance(spec, str): + continue + line = _package_json_line(content, section, name) + results.append((name, _pinned_npm_version(spec), line)) return results diff --git a/tests/unit/test_patterns_new.py b/tests/unit/test_patterns_new.py index 029553df0..6b12122ea 100644 --- a/tests/unit/test_patterns_new.py +++ b/tests/unit/test_patterns_new.py @@ -1906,6 +1906,54 @@ def test_extract_packages_package_json_caret_is_not_a_pin(self) -> None: assert versions["semver"] is None assert versions["glob"] is None + def test_package_json_on_a_single_line_is_not_invisible(self) -> None: + # Regression: the line-oriented scan never entered the dependency section, so a valid + # one-line manifest yielded no dependencies at all — silently. + content = '{"name":"x","dependencies":{"express":"^4.18.0","lodash":"4.17.21"}}' + names = {p[0] for p in sc_mod._extract_packages_from_package_json(content)} + assert names == {"express", "lodash"} + + def test_package_json_compact_keeps_versions(self) -> None: + # Version resolution is not this PR's subject: it stays whatever the shared predicate + # decides (#319). Only the parsing of the manifest changes, and a compact manifest must + # resolve exactly like the indented one. + content = '{"dependencies":{"lodash":"4.17.21","semver":"^7.5.0"}}' + versions = {p[0]: p[1] for p in sc_mod._extract_packages_from_package_json(content)} + assert versions["lodash"] == "4.17.21" + assert versions["semver"] is None + + def test_package_json_line_numbers_survive_parsing(self) -> None: + content = '{\n "name": "x",\n "dependencies": {\n "express": "4.18.0"\n }\n}\n' + lines = {p[0]: p[2] for p in sc_mod._extract_packages_from_package_json(content)} + assert lines["express"] == 4 + + def test_package_json_line_prefers_the_dependency_over_a_script(self) -> None: + # A name that also appears in "scripts" must not steal the line number. + content = ( + "{\n" + ' "scripts": { "express": "node server.js" },\n' + ' "dependencies": {\n' + ' "express": "4.18.0"\n' + " }\n" + "}\n" + ) + lines = {p[0]: p[2] for p in sc_mod._extract_packages_from_package_json(content)} + assert lines["express"] == 4 + + def test_package_json_invalid_falls_back_to_the_scan(self) -> None: + # A manifest that does not parse keeps the previous behaviour instead of going blind. + content = '{\n "dependencies": {\n "express": "4.18.0",\n' # truncated + names = {p[0] for p in sc_mod._extract_packages_from_package_json(content)} + assert "express" in names + + def test_package_json_non_object_is_empty(self) -> None: + assert sc_mod._extract_packages_from_package_json("[1, 2, 3]") == [] + + def test_package_json_ignores_non_string_specs(self) -> None: + content = '{"dependencies":{"ok":"1.0.0","broken":{"version":"1.0.0"},"n":42}}' + names = {p[0] for p in sc_mod._extract_packages_from_package_json(content)} + assert names == {"ok"} + def test_extract_packages_package_json(self) -> None: content = ( '{\n "dependencies": {\n "express": "^4.18.0",\n "lodash": "4.17.21"\n }\n}' From bf5792e1fd5df62fc1ac08d0ba523e1dce406067 Mon Sep 17 00:00:00 2001 From: Mark2Mac Date: Fri, 31 Jul 2026 19:42:45 +0200 Subject: [PATCH 2/2] fix(cli): route --verbose tracebacks to stderr too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The generic --verbose handlers in scan() and baseline() still called console.print_exception(), so a fatal traceback landed on stdout while stderr stayed empty — the exact split the rest of this PR fixes for the one-line error messages. A caller redirecting stdout to a report file got the traceback inside the file and nothing in its error log. Both branches now print through err_console, and the regression asserts the separation on both commands: RuntimeError appears in stderr and not in stdout, exit code 2. Tests: tests/unit 735 passed, 12 skipped. Signed-off-by: Mark2Mac --- src/skillspector/cli.py | 4 ++-- tests/unit/test_cli.py | 27 +++++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/src/skillspector/cli.py b/src/skillspector/cli.py index ca6dfb699..841891fa1 100644 --- a/src/skillspector/cli.py +++ b/src/skillspector/cli.py @@ -387,7 +387,7 @@ def scan( raise typer.Exit(code=2) from e except Exception as e: if verbose: - console.print_exception() + err_console.print_exception() else: err_console.print(f"[red]Error:[/red] {e}") raise typer.Exit(code=2) from e @@ -641,7 +641,7 @@ def baseline( raise typer.Exit(code=2) from e except Exception as e: if verbose: - console.print_exception() + err_console.print_exception() else: err_console.print(f"[red]Error:[/red] {e}") raise typer.Exit(code=2) from e diff --git a/tests/unit/test_cli.py b/tests/unit/test_cli.py index fb7061f6c..38935d87d 100644 --- a/tests/unit/test_cli.py +++ b/tests/unit/test_cli.py @@ -885,3 +885,30 @@ def fake_invoke(state: dict[str, Any], config: Any = None) -> dict[str, Any]: assert payload["issues"] == [{"id": "X-1", "severity": "low"}] assert payload["suppressed_count"] == 0 assert payload["suppressed"] == [] + + +def test_scan_verbose_traceback_goes_to_stderr(tmp_path: Path) -> None: + """A fatal --verbose traceback belongs on stderr, so stdout stays parseable.""" + (tmp_path / "SKILL.md").write_text("# Boom", encoding="utf-8") + + with patch("skillspector.cli.graph.invoke", side_effect=RuntimeError("scan crashed")): + result = runner.invoke(app, ["scan", str(tmp_path), "--no-llm", "--verbose"]) + + assert result.exit_code == 2 + assert "RuntimeError" in result.stderr + assert "RuntimeError" not in result.stdout + + +def test_baseline_verbose_traceback_goes_to_stderr(tmp_path: Path) -> None: + """Same separation for `baseline`, which shares the generic --verbose handler.""" + (tmp_path / "SKILL.md").write_text("# Boom", encoding="utf-8") + + with patch("skillspector.cli.graph.invoke", side_effect=RuntimeError("baseline crashed")): + result = runner.invoke( + app, + ["baseline", str(tmp_path), "--no-llm", "--verbose", "-o", str(tmp_path / "b.yaml")], + ) + + assert result.exit_code == 2 + assert "RuntimeError" in result.stderr + assert "RuntimeError" not in result.stdout