Skip to content

perf: scan once and write both formats - #10

Open
bradAGI wants to merge 2 commits into
trustabl:mainfrom
bradAGI:perf/single-scan-dual-output
Open

bradAGI wants to merge 2 commits into
trustabl:mainfrom
bradAGI:perf/single-scan-dual-output

Conversation

@bradAGI

@bradAGI bradAGI commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Depends on #1 (the test suite) — it is the first commit here so CI can run. Review the second commit.

Every build scans the source twice

# Run 1: SARIF (file emit).
trustabl "${BASE_ARGS[@]}" --format sarif > "$SARIF_FILE"
# Run 2: JSON (drives thresholds, log summary, dotenv).
trustabl "${BASE_ARGS[@]}" --format json > "$JSON_FILE" || true

Same target, same rules, walked and analyzed twice. Measured on openai/openai-agents-python with trustabl v0.1.7:

wall clock
two runs (today) 89.4s
one run (this PR) 43.8s

There is a correctness angle too

NATIVE_CODE comes from the SARIF run. Every gate input — readiness, findings count, max severity — comes from the JSON run. Two independent scans, so the code deciding whether to fail the build and the data it decides on come from different invocations. One scan removes the possibility of them disagreeing.

The fix

The engine has written both formats from a single scan since v0.1.3 — --json-out and --sarif-out persist each format to a file independently of --format:

      --json-out string        also write the JSON ScanResult to this file (independent of --format)
      --sarif-out string       also write the SARIF report to this file (independent of --format)

Used when the binary advertises them, with the two-run path kept for older engines — the same capability probe trustabl-action does in install.ts.

Output is unchanged

Verified against the real engine, not the stub:

$ cmp a.sarif b.sarif && echo "sarif: byte-identical"
sarif: byte-identical
$ diff <(jq -S 'del(.scan_id)' a.json) <(jq -S 'del(.scan_id)' b.json) && echo same
same
$ echo "$(jq -r .scan_id a.json) vs $(jq -r .scan_id b.json)"
scan_fbc725528ed93760 vs scan_fbc725528ed93760

And end to end through the actual scanner — real release tarball, real sha256 verification, real extraction, real engine:

checksum verified: trustabl_9.9.9_darwin_arm64.tar.gz
sarif results: 3
json findings: 3
TRUSTABL_EXIT_CODE=1
TRUSTABL_READINESS_SCORE=96
TRUSTABL_RISK_SCORE=4
TRUSTABL_MAX_SEVERITY=medium
TRUSTABL_FINDINGS_COUNT=3

Tests

The stub engine now models --json-out/--sarif-out and a scan --help that advertises them, with STUB_NO_FILE_OUT to present an engine older than v0.1.3. Three tests: the modern path invokes scan exactly once, the fallback path exactly twice, and both produce the same artifacts and the same gate inputs.

Before: not ok — scan invocations: expected '1', got '2'. After: all 18 test(s) passed.

The scanner is the whole product here — it resolves a release, verifies it,
runs the engine, scales the score, and decides whether the build fails — and
until now nothing checked any of it. A regression in the gate logic or in the
jq that reads the engine's ScanResult would ship silently.

The suite drives the real scan/trustabl-scan.sh. Each test builds an actual
gzipped release tarball containing a stub engine and a real checksums.txt over
it, then puts a stub curl on PATH that serves that directory by URL basename.
The download, sha256 verification, extraction and invocation therefore all run
unmodified; only the network and the engine binary are substituted.

The fixtures are unmodified output from a real `trustabl scan` (engine v0.1.7),
so the assertions pin the scanner against the ScanResult shape the engine
actually emits rather than a hand-written approximation of it.
The scanner ran the engine twice over the same source — once for SARIF, once
for JSON — so every build paid the parse and analysis cost twice. On
openai/openai-agents-python that is 89.4s against 43.8s for a single run.

The engine has written both formats from one scan since v0.1.3: --json-out and
--sarif-out persist each format to a file independently of --format. Use them
when the binary advertises them, and keep the two-run path for older engines.

Beyond the time, the two runs were two independent scans. NATIVE_CODE came from
the SARIF run while every gate input — score, findings count, max severity —
came from the JSON run, so the code that decided whether to fail the build and
the data it decided on came from different invocations. One scan removes that.

Output is unchanged: on the real engine the SARIF file is byte-identical and
the JSON differs in nothing, scan_id included.
@sairenchristianbuerano

Copy link
Copy Markdown
Collaborator

Thanks @bradAGI. Flagging that this is being read rather than sitting unlooked-at.

We are reviewing all 52 open PRs together instead of one at a time. 31 of them edit scan/trustabl-scan.sh, so merge order decides as much as any individual verdict — GitHub reports almost all of these as mergeable, but that is each PR against main in isolation, not against each other. The first merge makes most of the rest conflict.

Reading against #50, which has the same goal. The feature detection for --json-out/--sarif-out here is the difference, since VERSION is pinnable and an older pin would not have those flags.

The order we are working to: the test harness in #1 first, since twelve PRs depend on it and nothing is verifiable without it, then the fixes that close fail-open paths, then behaviour changes, then docs. A verdict on this one follows once its cluster is read.

Apologies for the wait, and thanks for the contribution.

@sairenchristianbuerano sairenchristianbuerano 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.

Thanks @bradAGI — the change looks good, but it needs a rebase.

Several PRs touching scan/trustabl-scan.sh have landed on main since you
opened this, so it no longer merges cleanly. Could you rebase and resolve?

bash test/run-tests.sh will confirm it — 19 passing on main right now.

Ping me when it's up.

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