Skip to content

fix(cli): give both multi-skill discovery modes a report on stdout - #139

Merged
rodrigorjsf merged 9 commits into
mainfrom
fix/multi-skill-report-emission
Aug 22, 2026
Merged

rodrigorjsf merged 9 commits into
mainfrom
fix/multi-skill-report-emission

Conversation

@rodrigorjsf

Copy link
Copy Markdown
Owner

Closes #114. Closes #115. Closes #116.

Three issues, one user-visible shape reached three different ways: a --format is accepted, a
report is promised, and stdout gets nothing usable. They are one pull request because they touch the
same code and because #115's empty body has to be the empty case of the shape #116 builds — split
across three branches they would have raced each other.

What lands, per issue

#116 — --repo-scan --format json was a concatenation, not a document. It now emits one object:
multi_skill, skill_count, max_risk_score, execution_successful, skills — the same object
--recursive builds, asserted as a key-and-shape equivalence rather than against a literal, so the
two discovery modes answer in one vocabulary. markdown is deliberately left concatenated and
pinned as such; merging Markdown has no shape to reuse and is not this issue's scope. sarif and
terminal are byte-identical to before.

#115 — a discovery miss returned before it built anything. The early return is now a
fall-through, so the requested format is honoured. The exit code stays 0.

#114 — --recursive with no --output wrote no report. -f json and -f terminal now print
the body --output would have received. sarif and markdown stay file-only, which is the brief's
scope; see the deferral note below.

All --recursive claims are qualified once the flag engages — below two immediate child skills it
falls through to an ordinary scan, and test_below_the_threshold_… passes untouched.

The measurement #115 was blocked on: one run, not zero

#115's brief made the option choice conditional on whether a zero-run SARIF log is acceptable to the
consumers this project targets, and required the branch taken to be recorded. Branch 4 was
taken.
SARIF 2.1.0 allows runs to be empty ("minItems": 0), but GitHub code scanning documents
"an array of one or more runs" — not acceptable to both — so a discovery miss emits one run
holding no result
. This project's own SarifLog.runs_non_empty validator refuses a zero-run log
too, which corroborates the choice rather than having driven it.

That decision has a consequence the issue did not anticipate, and this pull request documents it
rather than hiding it:
a one-run-no-result log is byte-indistinguishable from a clean single-skill
scan, so SARIF carries no discovery-miss signal at all. The recipe first shipped here
(jq -e '[.runs[].results[]] | length > 0') was therefore wrong twice over — it fails on a healthy
repository that simply found nothing wrong, and it still cannot see the case it was written for. The
README now says plainly that --format json's skill_count is the machine-readable miss signal and
that SARIF has none.

A defect introduced and then closed inside the same change

_merge_repository_json first derived execution_successful from the exception list. A child that
reports execution_successful: false without raising then exited 2 while the merged body
claimed execution_successful: true / SAFE — measured, not theorised. Both that and
partial-inspection counting are now read off the children. The old concatenated body made no
aggregate claim; #116 created the claim and therefore owed it. The fix is folded into the commit
that created the exposure, not bolted on after it.

#114's stated reason for excluding -f sarif is stale — filed, not silently fixed

The issue argues sarif must wait because --recursive -f sarif --output is a --- path ---
concatenation. Measured on this branch: it is already one merged, validated log. Upstream's
2.9.6 merge replaced the concatenation the issue was written against. Scope was not widened —
sarif stays file-only here — but the false premise was corrected in three contract documents so it
is not repeated, and the measurement is filed as #136.

Review

Three independent critics graded this branch — acceptance-criteria conformance, scope and contract,
adversarial correctness — over two rounds. Every blocking finding is closed on the branch. Four
commits came out of the first round (c90aca0, 0295644, 601e4f2, f21efdd); the last blocking
finding and two README contract errors the critics raised as scope notes were closed in b2b7c14
and 77b0165.

The last blocking finding is worth naming because the behaviour was already right: the guard that
skips the per-skill column heading on a discovery miss had no test. Replacing it with an
unconditional print left the entire unit selection green, because the miss tests assert only that
the warning is present in stderr, and a bare heading beneath it satisfies a containment check.
Proven by mutating that one site — the new assertion goes red, and git diff --exit-code on the
source file is clean after restoring by hand.

Deliberate deferrals

Verification

Run in the main thread on this branch, at 77b0165, after the rebase onto b306027:

make test-unit      4412 passed, 14 skipped, 38 deselected, 4 xfailed
make lint           All checks passed!
make format-check   258 files already formatted
make update-snapshots + git status --short   -> clean, no churn

The Behavior Snapshot gate cannot see any of this — the projection scans through the graph and never
imports the CLI — so a clean snapshot run is evidence that nothing else moved, not that this works.
The evidence that it works is tests/unit/test_cli_streams.py, which asserts result.stdout and
result.stderr apart throughout.

`--repo-scan` merged only for SARIF. Every other format fell through to the
per-Skill bodies concatenated behind `--- <path> ---` separators, so
`skillspector scan . --repo-scan -f json | jq .` failed with a parse error on
every repository and the mode had no machine-readable format but SARIF.

The merged object is the one `--recursive --format json` already builds, not a
second shape: both callers now go through `_combined_skill_entry` and
`_combined_json_report`, so the two discovery modes answer in one vocabulary and
a consumer tells them apart by the flag it ran rather than by the body it parses.
Each `skills` entry carries its repository-relative path.

`_multi_skill_analysis_completeness` takes a `scope`, defaulting to the recursive
value it already emitted; a Repository Scan passes `repository_skills`, because
`CONTEXT.md` lists "recursive scan" among the spellings a Repository Scan is
never called and a machine-readable field is prose too.

`-f sarif` and `-f terminal` are byte-identical to before, measured. `-f markdown`
keeps the concatenation: merging Markdown is a separate question with no shape in
this codebase to reuse, and a test pins that half so the README cannot rot.

Closes #116

Signed-off-by: Rodrigo França <rodrigo_rjsf@hotmail.com>
A `--repo-scan` that discovered nothing returned above every line that builds the
report body, so the requested `--format` was accepted and silently ignored,
stdout was empty and the exit code was `0`. Before the stream sweep the warning
was on stdout, so a gate piping into `jq` broke loudly on a discovery miss — an
accident, never a designed signal. On stderr, where a note belongs, the same gate
read an empty stream and passed vacuously.

A discovery miss is now the zero-Skill case of the same body-building path a hit
takes. `-f sarif` emits a valid SARIF 2.1.0 log, `-f json` the merged repository
object with the counts at zero and an empty skill list, `-f markdown` a document
naming the miss and the root patterns searched. `-f terminal` is unchanged: a
rendered report of no Skills is nothing, so the body builder answers `None` there
and neither stdout nor `--output` receives anything — distinct from an empty body,
which an empty file could not be told apart from a crashed run.

The SARIF log carries one run holding no result rather than no run at all,
following the decision rule on the issue: the SARIF 2.1.0 schema permits
`"minItems": 0` on `runs`, but GitHub code scanning documents "an array of one or
more runs", so a zero-run log is not acceptable to both. This project's own
`validate_sarif_report` refuses one as well, which corroborates rather than drives
the choice.

The exit code stays `0` in every case and no new code was added. A discovery hit
emits byte-for-byte what it emitted before, in every format, measured.

Closes #115

Signed-off-by: Rodrigo França <rodrigo_rjsf@hotmail.com>
Once `--recursive` engaged — two or more immediate child Skills — it wrote its
combined report only to `--output`. Every write of the body was guarded on
`--output`, so with `-f json` and none, every Skill was scanned, the exit code was
correct, and stdout was empty: the format flag was accepted and silently ignored,
and a CI step running `skillspector scan ./skills --recursive -f json | jq` read
an empty stream and passed vacuously. Below the engagement threshold the flag
never engages and an ordinary scan already printed its report, so this was only
ever true of the engaged flag.

The body is now built once and either written to `--output` or printed, through
`_emit_multi_skill_body`, exactly as every other report-producing path does. The
printed object is the one `--output` already received — measured identical field
for field — rather than a second shape. `-f terminal` prints its combined report
too.

Because stdout now carries a report, the Multi-Skill Summary table is a digest of
one rather than the whole product of the scan, so the `summary` console selector
that picked between stdout and stderr is deleted and the table's prints are plain
`advice` calls. Leaving it would have interleaved a Rich table into the JSON on
stdout, which is the defect #99 was filed about.

`-f sarif` and `-f markdown` stay `--output`-only, per the issue's scope. The
issue's stated reason no longer holds for SARIF: measured, its `--output` shape is
one merged, validated log — upstream's recursive merge arrived with the 2.9.6 sync
and replaced the concatenation the issue was written against — so only scope keeps
it off stdout. The docstrings, the README and the pinning test say so rather than
repeating the stale premise, and #136 tracks printing it. Markdown's exclusion
stands on its own: there is no merged Markdown document to print.

Exit codes are unchanged on every path.

Closes #114

Signed-off-by: Rodrigo França <rodrigo_rjsf@hotmail.com>
`_merge_repository_sarif`'s zero-run fall-back, added for #115, was keyed on
`not runs` — strictly wider than the discovery miss its docstring argued for. A
Skill only joins `scanned` when the graph *returns*, so a repository whose every
discovered Skill failed reaches the merge with nothing to merge as well, and the
fall-back fired there too. Measured against a two-Skill fixture, in both reachable
variants — the graph raising for every Skill, and every child returning
`execution_successful: false` with no `sarif_report` — `--repo-scan -f sarif`
emitted a run asserting `"invocations": [{"executionSuccessful": true}]` while the
process exited `2`. That is the field GitHub code scanning reads, so a SARIF-only
gate read a clean bill of health for a Scan that inspected nothing: the vacuous
pass #115 exists to remove, recreated on a different input.

It also made 9d6f6a4's "A discovery hit emits byte-for-byte what it emitted
before, in every format, measured" false. On that input `main` prints 101 bytes
of `{"runs": []}` and this branch printed 344. That sentence is true again now,
measured on the same input: 101 bytes, exit `2`, and
`_merge_repository_sarif`'s executable body compares equal to `main`'s.

The miss keeps its one-run log — it is #115's deliverable and correct there — as
`_empty_repository_sarif`, beside `_empty_repository_markdown`, reached from
`_repository_report_body` on the `not scanned and not failures` predicate that
function already used for Markdown. Its `$schema` is `SARIF_SCHEMA_URI`, the one
every other SARIF document this tool builds declares, rather than the unrelated
schemastore literal `_merge_repository_sarif` seeds when no child hands it one;
the miss log is a documented path now, so a consumer pinning the field must not
see the scanner describe itself two ways.

Three tests pin it, each mutated at its own site to prove it non-vacuous: both
all-fail variants assert `runs: []` and no `executionSuccessful` at exit `2`, and
the miss log's `$schema` is asserted equal to a hit's rather than to a constant,
so the two cannot drift apart.

The zero-run log a failed hit still emits is refused by this project's own
`validate_sarif_report`. It is what the mode has always emitted, no brief in this
wave covers it, and its answer is a run declaring `executionSuccessful` **false**
— a change to a committed output shape. #138 carries it, and the code and the
tests point at it.

Signed-off-by: Rodrigo França <rodrigo_rjsf@hotmail.com>
`_merge_repository_json` passed `skills_scanned=len(scanned)`, which drops every
Skill whose graph invocation raised, while `_scan_multi_skill` passes the length
of the list of Skills it *attempted*. With two children and one raising, the two
discovery modes printed `skills_scanned` 1 and 2 for the same situation — the one
key on which the object both modes are supposed to answer in disagreed with
itself.

It also broke an identity that holds by construction on the recursive path, where
`omitted_skill_count = len(skills) - len(processed_skills)` makes
`skills_scanned + skills_omitted == skill_count` true always. A Repository Scan
omits nothing, so hardcoding `skills_omitted=0` beside a successful-only count
answered `0 + 0` against a `skill_count` of two.

The merged repository object is new on this branch and has never been released,
so no consumer can have read the old value.

Signed-off-by: Rodrigo França <rodrigo_rjsf@hotmail.com>
`scope` is the deliberate difference between the two merged objects, so the
equivalence test that compares their shared keys cannot assert it — and nothing
else did: reverting the value to the helper's default left the whole unit
selection green.

It is the only field that lets a reader tell a Repository Scan's zeroed
`public_finding_records` and `report_characters`, budgets that mode does not
have, from a recursive Scan that consumed none of its own. It is also a
vocabulary contract: `CONTEXT.md` lists "recursive scan" among the spellings a
Repository Scan is never called, and a machine-readable field is prose too.

Signed-off-by: Rodrigo França <rodrigo_rjsf@hotmail.com>
Four sites still described the `summary = console if … else advice` selector this
branch deleted, and each therefore asserted the opposite of what the code does:
the module-level comment above the console definitions, `_advise_on_advisory_findings`'s
docstring, README's "Which stream carries what" lead sentence, and one test
docstring. The first is the block `docs/DEVELOPMENT.md`'s Logging bullet points a
contributor at when they add a print site, so it instructed the next reader to put
a Rich table back on stdout in front of the JSON — issue #99 exactly.

They are restated rather than replaced with a new universal: "the report reaches
stdout on every path" would be false for `--recursive -f sarif`, `--recursive
-f markdown` and a `--repo-scan -f terminal` discovery miss. The rule is that a
print site is classified by what it is, never by its neighbour; the Multi-Skill
Summary table is a digest like `--repo-scan`'s, and the history of why it used to
be argued is kept where it reads as history.

Signed-off-by: Rodrigo França <rodrigo_rjsf@hotmail.com>
The guard that skips the per-skill column heading when discovery matched
nothing had no test. Replacing it with an unconditional print left the whole
unit selection green, because the four miss tests assert only that the warning
is *present* in stderr and a bare heading printed beneath it satisfies a
containment check.

Assert the absence of the two heading columns that appear nowhere else, so the
guard is covered rather than merely correct.

Signed-off-by: Rodrigo França <rodrigo_rjsf@hotmail.com>
…-miss signal

The recipe this change shipped recommended gating on the SARIF result count.
It fails on a healthy repository that simply found nothing wrong, and after the
one-run-no-result decision a discovery miss is byte-indistinguishable in SARIF
from a clean single-skill scan -- so the expression rejects every clean scan and
still cannot see the case it was written for. Say plainly that SARIF carries no
miss signal and that `--format json`'s `skill_count` is the one that does.

The comparison table's no-`--output` row also claimed the report reaches stdout
in every format. Two of the four are the exception on a discovery miss:
`terminal` writes nothing at all, and `markdown` writes one document naming the
miss rather than a concatenation. The stream section already said so, and the
clause this pull request added forbids the two disagreeing.

Signed-off-by: Rodrigo França <rodrigo_rjsf@hotmail.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

1 participant