Fix audit findings: soundness, coverage reporting, docs, and review follow-ups #1

Merged
trevon merged 18 commits from fix/audit-findings into master 2026-09-25 06:48:08 +00:00
6 changed files with 88 additions and 12 deletions
Showing only changes of commit b8c997c732 - Show all commits

fix(scan): report quarantined TUs as coverage gaps

Problem: a TU skipped because a prior run quarantined it was not analysed,
but coverage did not say so. The serial path dropped it from the attempted
set, so a scan whose only TU was quarantined was complete and exited 0; the
fan-out path saw no file_finished record and called it frontend-failed.

Fix: the shard runner counts every requested TU and records skipped ones
with gap reason `quarantined`; a worker emits a file_finished record with
that reason so the parent classifies it the same way. USAGE lists the reason.

Test: test_subprocess_fanout "[quarantine]" covers serial (was 2 attempted,
no gap) and fan-out (was frontend-failed). CLI probe: serial and fan-out
both exit 2 with a `quarantined` gap.
trvon 2026-09-24 21:37:21 -06:00
Signed by: trevon
GPG key ID: 38686EA797D7A8A7

View file

@ -92,7 +92,8 @@ deduplicated finding set.
Both machine formats record which C/C++ translation units were analyzed: the Both machine formats record which C/C++ translation units were analyzed: the
native JSON gains a `coverage` object (`tus_attempted`, `tus_analyzed`, and one native JSON gains a `coverage` object (`tus_attempted`, `tus_analyzed`, and one
`gaps` entry per unanalyzed TU with a reason: `compile-errors`, `gaps` entry per unanalyzed TU with a reason: `compile-errors`,
`frontend-failed`, `analysis-failed`, `timeout`, `crash`, or `worker-exit`), `frontend-failed`, `analysis-failed`, `timeout`, `crash`, `worker-exit`, or
`quarantined` for a TU a prior run's quarantine skipped),
and SARIF carries the same data as an `invocations` entry with and SARIF carries the same data as an `invocations` entry with
`executionSuccessful` and one `toolExecutionNotifications` entry per gap. A `executionSuccessful` and one `toolExecutionNotifications` entry per gap. A
scan with any gap, or one that analyzed no TU, exits `2` after writing its scan with any gap, or one that analyzed no TU, exits `2` after writing its

View file

@ -50,6 +50,10 @@ struct ClangShardTuOutcome {
ClangShardTuOutcome runClangShardTranslationUnit(const ScanOptions& opts, clang::ASTContext& ctx, ClangShardTuOutcome runClangShardTranslationUnit(const ScanOptions& opts, clang::ASTContext& ctx,
ClangShardTuState state, ClangShardTuHooks hooks); ClangShardTuState state, ClangShardTuHooks hooks);
// Log TUs skipped because a prior run quarantined them; a worker also emits a
// `quarantined` file_finished record for each so its parent counts the gap.
void reportQuarantinedTus(const ScanOptions& opts, const std::vector<std::string>& skipped);
// Coverage of `requested` TUs given the outcomes recorded for them. A // Coverage of `requested` TUs given the outcomes recorded for them. A
// requested TU with no outcome never reached the detectors: the front end // requested TU with no outcome never reached the detectors: the front end
// produced no AST for it ("frontend-failed"). // produced no AST for it ("frontend-failed").

View file

@ -13,6 +13,7 @@ struct CoverageGap {
// compile-errors: the front end reported errors, so the AST is partial. // compile-errors: the front end reported errors, so the AST is partial.
// frontend-failed: no AST was produced (missing file, bad command line). // frontend-failed: no AST was produced (missing file, bad command line).
// analysis-failed: the detectors threw on this TU. // analysis-failed: the detectors threw on this TU.
// quarantined: a prior run quarantined this TU, so it was skipped.
// timeout / crash / worker-exit: the isolated worker died on this TU. // timeout / crash / worker-exit: the isolated worker died on this TU.
std::string reason; std::string reason;
}; };

View file

@ -129,6 +129,29 @@ ClangShardTuOutcome runClangShardTranslationUnit(const ScanOptions& opts, clang:
return outcome; return outcome;
} }
void reportQuarantinedTus(const ScanOptions& opts, const std::vector<std::string>& skipped) {
if (skipped.empty())
return;
spdlog::info("quarantine: skipping {} TU(s) from prior run in {}", skipped.size(),
opts.quarantine_dir);
for (std::size_t i = 0; i < skipped.size() && i < 8; ++i)
spdlog::info(" skipped: {}", skipped[i]);
if (skipped.size() > 8)
spdlog::info(" ... (+{} more)", skipped.size() - 8);
// A quarantined TU was not analysed, so it is a coverage gap. A worker
// tells its parent, which otherwise sees no file_finished record and
// calls the TU frontend-failed.
if (!opts.is_worker)
return;
for (const auto& file : skipped) {
std::cout << nlohmann::json{{"kind", "file_finished"},
{"data", {{"path", file}, {"gap", "quarantined"}}}}
.dump()
<< '\n';
}
std::cout.flush();
}
ScanCoverage tuCoverage(const std::vector<std::string>& requested, ScanCoverage tuCoverage(const std::vector<std::string>& requested,
const std::vector<ClangShardTuOutcome>& outcomes) { const std::vector<ClangShardTuOutcome>& outcomes) {
std::unordered_map<std::string, std::string> gap_by_path; std::unordered_map<std::string, std::string> gap_by_path;

View file

@ -351,16 +351,7 @@ ShardResult runClangDetectorsOnShard(clang::tooling::CompilationDatabase& db,
} }
files.push_back(f); files.push_back(f);
} }
if (!skipped.empty()) { reportQuarantinedTus(opts, skipped);
spdlog::info("quarantine: skipping {} TU(s) from prior run in {}", skipped.size(),
opts.quarantine_dir);
for (std::size_t i = 0; i < skipped.size() && i < 8; ++i) {
spdlog::info(" skipped: {}", skipped[i]);
}
if (skipped.size() > 8) {
spdlog::info(" ... (+{} more)", skipped.size() - 8);
}
}
const SourceDetectorPlan detector_plan = makeSourceDetectorPlan(opts.detectors); const SourceDetectorPlan detector_plan = makeSourceDetectorPlan(opts.detectors);
std::vector<DetectorProfile> detector_profiles = detector_plan.makeDetectorProfiles(); std::vector<DetectorProfile> detector_profiles = detector_plan.makeDetectorProfiles();
@ -584,7 +575,9 @@ ShardResult runClangDetectorsOnShard(clang::tooling::CompilationDatabase& db,
graph_security_candidates.empty() ? nullptr : &graph_security_candidates}; graph_security_candidates.empty() ? nullptr : &graph_security_candidates};
auto factory = makeSourceTuFrontendFactory(tu_config, tu_state); auto factory = makeSourceTuFrontendFactory(tu_config, tu_state);
(void)tool.run(factory.get()); // which TUs failed comes from tu_outcomes (void)tool.run(factory.get()); // which TUs failed comes from tu_outcomes
out.coverage = tuCoverage(files, tu_state.tu_outcomes); for (const auto& f : skipped)
tu_state.tu_outcomes.push_back({f, "quarantined"});
out.coverage = tuCoverage(files_in, tu_state.tu_outcomes);
} }
if (out.profile) { if (out.profile) {
out.profile->source_tus_completed = tu_state.source_tus_completed; out.profile->source_tus_completed = tu_state.source_tus_completed;

View file

@ -172,6 +172,60 @@ TEST_CASE("SubprocessFanout single worker == serial path", "[fanout][unit]") {
REQUIRE(parallel.size() == serial.findings.size()); REQUIRE(parallel.size() == serial.findings.size());
} }
TEST_CASE("quarantined TUs are coverage gaps in serial and fan-out scans",
"[fanout][quarantine][coverage]") {
// A TU skipped because a prior run quarantined it was not analysed. The
// serial path dropped it from the attempted set (a complete scan with it
// missing); the fan-out path reported it as frontend-failed.
auto fixtures = locateFixtures();
writeCompileDb(fixtures);
std::string err;
auto db = fold::loadCompilationDatabase(fixtures, err);
REQUIRE(db);
auto files = db->getAllFiles();
REQUIRE(files.size() == 3);
const auto unique = std::to_string(std::chrono::steady_clock::now().time_since_epoch().count());
const auto quarantine =
std::filesystem::temp_directory_path() / ("fold-quarantine-coverage-" + unique);
fold::cli::appendQuarantine(quarantine.string(), files[0], SIGSEGV);
fold::cli::ScanOptions opts;
opts.compile_db = fixtures;
opts.detectors = {"clone"};
opts.min_body_stmts = 1;
opts.quarantine_dir = quarantine.string();
auto quarantinedGap = [&](const fold::ScanCoverage& coverage) {
CHECK(coverage.tus_attempted == 3);
CHECK(coverage.tus_analyzed == 2);
REQUIRE(coverage.gaps.size() == 1);
CHECK(fold::cli::normalizeQuarantinePath(coverage.gaps[0].file) ==
fold::cli::normalizeQuarantinePath(files[0]));
CHECK(coverage.gaps[0].reason == "quarantined");
};
SECTION("serial") {
auto serial = fold::cli::runDetectorsOnShard(*db, opts, files,
/*include_clone_clustering=*/true);
quarantinedGap(serial.coverage);
}
SECTION("fan-out") {
std::string fold_bin = locateFoldBinary();
if (fold_bin.empty()) {
WARN("could not locate fold binary; skipping fan-out coverage");
return;
}
ScopedWorkerBinary worker(fold_bin);
fold::cli::SubprocessFanout fanout;
(void)fanout.runSync(opts, files, /*n_workers=*/2);
quarantinedGap(fanout.coverage());
}
std::error_code error;
std::filesystem::remove_all(quarantine, error);
}
TEST_CASE("SubprocessFanout isolates sequential frontend failures and preserves findings", TEST_CASE("SubprocessFanout isolates sequential frontend failures and preserves findings",
"[fanout][recovery]") { "[fanout][recovery]") {
const char* helper = std::getenv("FOLD_TEST_FANOUT_FAILURE_WORKER"); const char* helper = std::getenv("FOLD_TEST_FANOUT_FAILURE_WORKER");