perf(scan): scan once and write both artifacts - #50
joeymussalli wants to merge 1 commit into
Conversation
The script ran two full scans of the target: one redirected to trustabl.sarif, one to trustabl.json. That is not needed. trustabl has --json-out and --sarif-out, which write their documents independently of --format, so a single run produces both. Cost aside, the two-scan arrangement had a correctness edge. Rules are fetched at scan time, so the two runs could resolve different rulesets -- the SARIF that gets uploaded and shown in the CodeCatalyst Reports tab and the JSON that decides the gate were describing two separate scans, with nothing checking that they agreed. On a large repository it also simply doubled the wall-clock and the compute bill of every build. There is a side benefit. Both runs previously redirected stdout to a file, so trustabl's own output never reached the build log at all -- everything a user saw came from this script's report box. With the machine documents going to --json-out/--sarif-out, --format human puts the scanner's own summary in the log where it belongs. - One invocation: --format human --json-out "$JSON_FILE" --sarif-out "$SARIF_FILE". - NATIVE_CODE still comes from that single run, so the gate is unaffected. Verified with a stub scanner that records its invocations: two full scans before (`--format sarif`, then `--format json`), one after, with both output files written and valid. Exit codes 0, 1 and 2 propagate to NATIVE_CODE exactly as before. bash -n passes. KNOWN CONFLICT. This touches the two scan invocations, and chore/remove-dead-code deletes the SCAN_END line immediately after them, so those two branches collide on one line. It is the only collision in the series and the resolution is to take both changes: no second scan, no SCAN_END. I tried stacking this on that branch instead, which removes the collision here but creates two larger ones against the gate and severity branches -- strictly worse, so this is left standalone. One compatibility note for reviewers: this requires a trustabl release that has --json-out/--sarif-out. VERSION defaults to "latest" so most users are covered, but anyone pinning an older tag would have the scan fail on the unknown flag. That fails closed -- a non-zero exit the gate reports -- rather than silently mis-reporting, which is the right direction, but it is a real consideration if you support older pinned versions and may warrant a documented minimum.
9705439 to
6865e94
Compare
|
Thanks @joeymussalli. 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 Reading against #10, which has the same goal. #10 feature-detects the 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. |
|
Thanks @joeymussalli — closing this one as a duplicate, and I want to be clear it is not a judgement on the change. Scanning twice to produce two formats was wasteful and you were right to collapse it. Going with #10 instead: #10 feature-detects Nothing here was wrong. Two people found the same problem within days of each other, which is a good sign for the repo and a bad sign for how long that bug had been sitting there. Sorry for the wasted effort, and thanks for taking the time. |
2 participants
The script ran two full scans of the target: one redirected to trustabl.sarif, one to trustabl.json. That is not needed. trustabl has --json-out and --sarif-out, which write their documents independently of --format, so a single run produces both.
Cost aside, the two-scan arrangement had a correctness edge. Rules are fetched at scan time, so the two runs could resolve different rulesets -- the SARIF that gets uploaded and shown in the CodeCatalyst Reports tab and the JSON that decides the gate were describing two separate scans, with nothing checking that they agreed. On a large repository it also simply doubled the wall-clock and the compute bill of every build.
There is a side benefit. Both runs previously redirected stdout to a file, so trustabl's own output never reached the build log at all -- everything a user saw came from this script's report box. With the machine documents going to --json-out/--sarif-out, --format human puts the scanner's own summary in the log where it belongs.
Verified with a stub scanner that records its invocations: two full scans before (
--format sarif, then--format json), one after, with both output files written and valid. Exit codes 0, 1 and 2 propagate to NATIVE_CODE exactly as before. bash -n passes.One compatibility note for reviewers: this requires a trustabl release that has --json-out/--sarif-out. VERSION defaults to "latest" so most users are covered, but anyone pinning an older tag would have the scan fail on the unknown flag. That fails closed -- a non-zero exit the gate reports -- rather than silently mis-reporting, which is the right direction, but it is a real consideration if you support older pinned versions and may warrant a documented minimum.