Skip to content

fix(cli,supply-chain): parse package.json as JSON, and send fatal errors to stderr - #323

Open
Mark2Mac wants to merge 2 commits into
NVIDIA:mainfrom
Mark2Mac:fix/package-json-parser
Open

fix(cli,supply-chain): parse package.json as JSON, and send fatal errors to stderr#323
Mark2Mac wants to merge 2 commits into
NVIDIA:mainfrom
Mark2Mac:fix/package-json-parser

Conversation

@Mark2Mac

Copy link
Copy Markdown
Contributor

Two correctness fixes, both surfaced while driving the CLI from automation. They are independent of each other but small enough to review together; happy to split if you prefer.

1. package.json was scanned line by line

_extract_packages_from_package_json walked the file looking for a line containing "dependencies", then read entries until a line starting with }. A manifest written on a single line — valid JSON, and what several generators emit — never enters the section:

$ cat package.json
{"name":"x","dependencies":{"express":"^4.18.0","lodash":"4.17.21"}}

$ skillspector scan .   # before
# no dependencies extracted, no supply-chain findings, no warning

That is not a noisy result, it is a blind one: the output is indistinguishable from a clean manifest.

It is now parsed with json.loads. Specifically:

  • Version extraction is unchanged, including the caret handling — that is a separate question (fix(supply-chain): treat only == / <= as version pins in requirements.txt (#294) #302). Only the parsing of the file changes.
  • Line numbers survive. JSON parsing loses positions, so the entry is located in the raw text starting from the section header, which also means a package name that appears in "scripts" does not steal the line.
  • A manifest that does not parse falls back to the previous line scan, so a truncated or templated file keeps today's behaviour instead of going silent.

2. Fatal errors were written to stdout

console = Console() writes to stdout, and every Error: goes through it. Any caller that separates the two streams — which is what automation does — throws the diagnosis away:

$ skillspector scan . --baseline old.yaml 2>err.log >out.json
$ cat err.log        # before: empty
$ head -c 60 out.json
Error: unsupported baseline version 1; expected 2. Version 1 ...

The message lands in the file that was supposed to hold the report, and the error log is empty. Error: unsupported baseline version 1 is precisely the message a user needs after upgrading, and it was the one hardest to find.

Errors now print to a Console(stderr=True).

Tests

Seven cases for the parser (one-line manifest, compact manifest, line numbers preserved, name shadowed by scripts, invalid JSON falling back, non-object manifest, non-string specs ignored).

Full suite: 1567 passed, 14 skipped, 6 xfailed.

…ors to stderr

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 (NVIDIA#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 <Mark2Mac@users.noreply.github.com>
@Mark2Mac

Copy link
Copy Markdown
Contributor Author

Cross-PR note: this PR conflicts with #302, which also touches _extract_packages_from_package_json (it replaces the caret-stripping with a _pinned_npm_version predicate).

The reconciliation is one line — the JSON parser here calls that predicate instead of its own _npm_version_or_none — and with both applied the full suite is green (1585 passed, 14 skipped, 6 xfailed). I kept version extraction untouched in this PR precisely so the two stay orthogonal; happy to rebase onto #302 or the other way round, whichever you prefer to land first.

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Automated SkillSpector Review]

Requesting changes. Parsing package.json as JSON fixes the compact-manifest blind spot and the fallback preserves best-effort handling of malformed manifests. The stderr change is incomplete, however: both generic --verbose exception branches still call console.print_exception(), so a fatal traceback is written to stdout. I reproduced this at the current head with a forced graph exception: exit 2, full traceback in stdout, empty stderr. Please route those tracebacks through err_console and add a stream-separation regression for the verbose path.

Comment thread src/skillspector/cli.py
# 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)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: the generic --verbose handlers in both scan() and baseline() still call console.print_exception(), which writes the fatal traceback to stdout. I reproduced exit 2 with the full traceback in stdout and empty stderr. Please use err_console.print_exception() for those branches and add a regression that asserts stdout/stderr separation under --verbose.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in f617861 — the objection was correct, and it was the same defect this PR set out to fix, left in the one branch where it is hardest to notice.

Both generic handlers now print through err_console, so scan --verbose and baseline --verbose behave like the one-line error paths. Reproduced your case first: with graph.invoke raising, the traceback was in stdout and result.stderr was empty.

Regression covers both commands (tests/unit/test_cli.py): exit code 2, RuntimeError present in stderr and absent from stdout. Verified it fails on the previous commit.

grep -rn print_exception src/ now returns only those two lines, both on err_console. Full suite: 1572 passed, 12 skipped, 6 xfailed.

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 <Mark2Mac@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants