SARIF evidence chains + external-findings fusion (ingest-sarif, overlap gate, MCP tool)#83
Merged
Conversation
Introduce `codelore_lib::external` with a SARIF 2.1.0 subset parser (`parse_sarif`) that normalises findings from Semgrep, Clippy, and CodeQL dialects into a uniform `ExternalFinding` struct. Key design points: - Rule-id: `result.ruleId` else `rules[ruleIndex].id` (CodeQL pattern) - Level fallback: result → rule defaultConfiguration → "warning" - Fingerprint chain: partialFingerprints.primaryLocationLineHash → any partialFingerprints value → any fingerprints value → sha256(engine|rule_id|path|start_line) self-hash - Path normalisation: strip `file://` scheme, strip leading `./` - Unparseable individual results are skipped with tracing::warn; non-SARIF documents return CodeLoreError::Analysis Three handwritten fixtures (semgrep / clippy / CodeQL) exercise each dialect quirk; 7 table-driven tests all pass.
Introduce `quality_gates::evidence` with `EvidenceCommit` + `evidence_for_path`. The function returns the top-N commits (newest-first, date DESC / rev DESC for tie-breaking) that touched a given path, joining `changes`/`changes_lineage` with `commits` for author, churn (loc_added + loc_deleted), and a SQL-truncated first-line message (DuckDB substr counts code points, not bytes — no Rust slicing needed). A nonexistent path returns an empty Vec, not an error. Four tests on `biomarker_repo`: newest-first ordering, churn > 0, n-limiting, and deterministic ordering across identical calls. All 4 pass.
Add `codelore ingest-sarif --repo . file1.sarif [file2…]` that parses each SARIF file via the B1 parser, groups findings by engine, and calls `ExternalStore::replace_engine` per engine so re-ingest is idempotent. Prints `ingested N finding(s) from M engine(s) → <path>` on success. The sidecar lives at `<cache_root>/codelore/<repo_hash_8>/external-findings.duckdb-ext`. The `.duckdb-ext` extension was chosen after verifying in cache.rs that the LRU pruner filters on `.extension() == "duckdb"` — `.duckdb-ext` returns `"duckdb-ext"` from that call and is skipped (see B2 report). Also extracts `pub fn repo_cache_dir(cache_root, repo_path)` from `cache.rs` so the SHA-256 repo-dir derivation is defined once and called from both `quality_gates::ledger::ledger_dir` and the new `ExternalStore`. Ledger tests remain green after the extraction. 5 store tests: fixture ingestion counts, re-ingest idempotency, two engines coexist.
Add `codelore check --format sarif` — a SARIF 2.1.0 document for all gate
violations, with evidence chains for per-file breaches.
Key design:
- `write_check_sarif(violations, evidence, repo_root, head_sha, w)` in
output/sarif.rs. One ReportingDescriptor per distinct gate (deduped via
BTreeSet for stable ordering). One result per GateViolation.
- Dual partialFingerprints per result: `gateFinding/v1`
(sha256(gate|path|head_sha) — versioned per SARIF spec SHALL) and
`primaryLocationLineHash` (sha256(repo_root|path) — mirrors the hotspots
emitter convention GitHub deduplicates on).
- Per-file violations get `relatedLocations` + one `codeFlow`/`threadFlow`
with ≤5 evidence commits (newest-first from evidence_for_path). Repo-wide
paths ("(repo-wide)", "(degraded)") emit uri "." with no evidence chain.
- Verdict lines (FAIL / WARNING / PASS) always go to stderr; exit codes
unchanged. GHA annotations suppressed in sarif mode (stdout is JSON only).
Evidence collection: after evaluate_all_gates, for each violated per-file
path (deduped) call evidence_for_path(db, opts, path, 5) via unwrap_or_default
(graceful empty on DB errors — evidence is best-effort context, not gating).
Tests:
- `check_sarif_structure_and_fingerprints` — 2 violations + 1 evidence
commit; asserts version, rules dedup, both fingerprint keys, codeFlows
nesting, repo-wide result has no codeFlow.
- `check_sarif_fingerprint_stability` — pinned sha256 regression guard for
gateFinding/v1; any accidental hash-input change fails loudly.
Pinned: sha256:88a397f9502c73ea613a9cabc2d9d20fb6ecfa80c1acd93e00438669156a1d0b
- CLI smoke: `check --format sarif` on biomarker_repo with code_health_min=100.0
→ exit 1 AND stdout parses as SARIF with ≥1 result AND "FAIL" on stderr.
… test Two B1-review folds into the B2 commit: 1. sarif_parse.rs::normalize_path docstring was wrong: claimed file:///abs/path → src/main.rs (stripped to repo-relative). The function only strips the scheme; absolute paths arrive with their leading slash intact (e.g. /home/runner/work/repo/src/Foo.java). Docstring now states the real contract; store.rs gains a module-level note explaining that absolute paths are stored as-is and will intentionally not match repo-relative hotspot paths in B3. 2. Added multi_run_sarif_produces_findings_from_both_engines: a single SARIF document with two runs (engine-a: 1 finding, engine-b: 2 findings) parses to 3 total; grouping by engine and calling replace_engine per engine stores all 3. Exercises the multi-run loop that was previously untested.
Add two tests required by the A1 review verdict: 1. evidence_message_head_truncated_to_80_ascii — builds a one-commit fixture repo with a 95-char ASCII message, ingests it, and asserts message_head.chars().count() == 80. Verifies the SQL substr(split_part(..., chr(10), 1), 1, 80) cap is enforced. 2. evidence_message_head_multibyte_safe — 79 ASCII chars + 5 CJK ideographs (3 UTF-8 bytes each, 84 code points total). Asserts message_head has ≤ 80 code points, starts with the 79 ASCII chars, and does not panic. Confirms DuckDB's code-point-aware substr never splits at a byte boundary mid-ideograph.
…ral signal
Adds the `finding-hotspot-overlap` analysis (B3): joins the external
SARIF sidecar (populated by `codelore ingest-sarif`) with hotspot and
code-health behavioral signals to produce a per-path priority triage.
Each output row carries: path, findings count, comma-joined engines,
worst severity level, hotspot_score, revs_percentile (PERCENT_RANK of
revision count computed in Rust), health_band, and priority label
("act-now" | "plan" | "note"). Sort: priority asc, findings desc,
path asc.
Join strategy: the external sidecar and FactsDb each own a separate
DuckDB connection (both are !Send + !Sync); no ATTACH is performed —
all joining happens in Rust via HashMaps.
Touches (7-touch §TEMPLATE):
- analysis.rs: FindingHotspotOverlap variant + as_str() + registry!
- analyses/mod.rs: pub mod finding_hotspot_overlap
- analyses/finding_hotspot_overlap.rs: full implementation + 8 unit tests
- external/store.rs: findings_by_path() + PathFindings struct
- output/csv.rs: write_finding_hotspot_overlap_csv()
- output/markdown.rs: write_finding_hotspot_overlap_markdown()
- cli_api.rs: re-export `external` module
- main.rs: FindingHotspotOverlap dispatch arm + dispatch fn
- cli_test.rs: "finding-hotspot-overlap" added to EXPLAIN_UNCOVERED
- Cargo.toml: [[test]] finding_hotspot_overlap_test
- tests/finding_hotspot_overlap_test.rs: 4 integration tests
Fix 1 — replace_engine atomicity: The DELETE + INSERT loop in replace_engine was non-transactional: a process kill after DELETE but before all INSERTs completed left the engine's findings permanently deleted. Wrapped the entire operation in conn.unchecked_transaction() (takes &self, rolls back on drop) so any mid-loop failure is automatically rolled back. Fix 2 — multi-file same-engine grouping test: Adds multi_file_same_engine_combined_count_is_sum to external_store_test: two batches from the same engine (simulating two SARIF files) are combined before calling replace_engine once. Asserts count = 3 (sum of both files), not 2 (file2-overwrites-file1). This pins the sharpest correctness property of the per-engine replace design.
- Promote `SARIF_SCHEMA` / `CODELORE_HOMEPAGE` in output/sarif.rs to `pub const SARIF_SCHEMA_URL` / `pub const TOOL_INFO_URI` so all four in-crate emitters and the diff emitter share one source of truth. - diff_output::emit_sarif gains `db_opts: Option<(&FactsDb, &Options)>`; when `Some`, evidence_for_path(n=3) is called per CODELORE-HOTSPOT, CODELORE-CLONE and CODELORE-DELTA-HEALTH finding and the commits are serialised as SARIF codeFlows → threadFlows → locations chains. - Degrading delta-health functions emit one CODELORE-DELTA-HEALTH result per unique path (Outcome::Bad, deduplicated); rule declaration is conditional on delta_health being present. - diff.rs: new analyze_head_at_rev returns (RevAnalyses, FactsDb, Options) keeping the in-memory db alive after the worktree is dropped; run_diff returns Result<(DiffOutput, FactsDb, Options)>; run_diff_cmd destructures and passes Some((&head_db, &head_opts)) to emit. - Stale URLs fixed: $schema was schemastore.azurewebsites.net, informationUri was github.com/emre/codescene — both now reference the constants. - Test: diff_sarif_schema_url_and_info_uri_use_canonical_constants asserts $schema == SARIF_SCHEMA_URL, informationUri == TOOL_INFO_URI, and the degrading CODELORE-DELTA-HEALTH result for the monster function carries ≥1 codeFlow with a populated threadFlow. - finding-hotspot-overlap added to EXPLAIN_UNCOVERED allowlist in cli_test.rs (no explain topic wired yet; decision recorded in-list).
Adds the `max_findings_in_hot_files` optional gate in `[gates]`: [gates] max_findings_in_hot_files = 0 # zero act-now overlap rows allowed Fails when the number of "act-now" rows from `finding-hotspot-overlap` (external findings that sit in a high-percentile hotspot with a red code-health band) exceeds the threshold. **Skip semantics**: the gate is skipped — not failed — when the external findings sidecar is absent or empty (the user has not yet run `codelore ingest-sarif`). A skip prints a distinct warning line and records verdict="skipped" in the ledger; it does not affect the exit code. Mirrors the degraded print style. Touches: - quality_gates/mod.rs: Gates.max_findings_in_hot_files + is_empty() + evaluate_finding_overlap_rows() pure fn + 6 unit tests - quality_gates/ledger.rs: extend verdict rustdoc to include "skipped" - main.rs: wire gate inline in run_check_cmd after evaluate_all_gates; violations is now mut to allow extending from the overlap gate - docs/advanced-usage.md: one-sentence §11.8 entry
…e path Extracts the per-engine grouping logic from run_ingest_sarif_cmd into a pure lib fn external::group_findings_by_engine(Vec<ExternalFinding>) -> HashMap<String, Vec<ExternalFinding>>. The command now builds a flat Vec across all input files then calls the lib fn — making the grouping step independently testable. Adds group_findings_by_engine_combines_two_fixture_files to external_store_test: parses the semgrep + clippy fixtures into a flat vec, calls group_findings_by_engine, ingests per engine, asserts count=4 across 2 engine keys. This pins the real ingest code path, not just the store contract.
Convert CheckArgs.format from String to a CheckFormat ValueEnum (Text | Sarif) matching the existing DiffFormat pattern. Unknown values like --format sariff now fail at parse time with a clap error rather than silently falling back to text. Also drop the needless evidence_locs.clone() in sarif.rs (build code_flow_locs via .iter() before consuming evidence_locs into relatedLocations) and fix two B-lane clippy regressions in finding_hotspot_overlap.rs (doc backticks for PERCENT_RANK, #[allow(dead_code)] on compute_percent_ranks). New test: check_default_format_is_text_not_json — verifies that omitting --format yields text output, not a JSON document. Note: --format sarif on a PASS emits a zero-result SARIF document (valid; the caller decides whether an empty result set matters).
Splits the condition introduced in the overlap gate commit so it passes `cargo fmt --all --check` at the CI-exact level.
Document check --format sarif (stdout SARIF, stderr verdicts, exit codes unchanged; zero-result document on PASS), the commit evidence chain (lineage-aware, newest-first), fingerprint keys (gateFinding/v1 for identity, primaryLocationLineHash for GitHub dedup), codeFlows + relatedLocations GitHub rendering, and the GitHub Actions upload-sarif snippet with continue-on-error note. Document diff --format sarif evidence chains (up to 3 commits per file). README: one-sentence add on --format sarif with chain callout. CHANGELOG [Unreleased]: - Added: check --format sarif with evidence chains - Added: diff --format sarif evidence chains - Added: ingest-sarif subcommand and external findings sidecar - Added: finding-hotspot-overlap analysis (behavioral x static) - Added: max_findings_in_hot_files gate - Fixed: stale schema URL and informationUri in diff SARIF emitter
Add relatedLocations alongside codeFlows on all evidence-bearing diff
SARIF results (CODELORE-HOTSPOT, CODELORE-CLONE, CODELORE-DELTA-HEALTH).
GitHub's inline annotation panel reads relatedLocations; codeFlows feeds
the "Show paths" flows tab — both are required for full GitHub rendering.
Refactor code_flow_for closure into evidence_for returning
Option<(codeFlows, relatedLocations)>: relatedLocations entries are plain
location objects (no wrapper); codeFlows threadFlowLocations wrap each in
{"location": …} as required by SARIF §3.36. Remove the "module" field from
threadFlowLocations — it means code module/namespace in SARIF, not commit
message; message_head is already in location.message.text.
Tests:
- Extend diff_sarif_schema_url_and_info_uri_use_canonical_constants to
also assert relatedLocations on the delta-health result (non-empty;
plain physicalLocation without "location" wrapper).
- Add diff_sarif_hotspot_rank_entrant_carries_code_flows_and_related_locations:
fixture with no Rust files at base and src/hot.rs changed twice at head,
guaranteeing a CODELORE-HOTSPOT rank-entrant; asserts both codeFlows and
relatedLocations are present and non-empty, and no stray "module" key.
Add finding_hotspot_overlap MCP tool (8th tool) to the stdio server.
The tool opens the sidecar only when it exists; an absent or empty
sidecar returns {"findings":[], "note":"run codelore ingest-sarif first"}
rather than an error, so agents calling it on a fresh repo get a
structured hint instead of a crash.
mcp_test: count 7→8, finding_hotspot_overlap in names list, new
mcp_finding_hotspot_overlap_returns_note_when_sidecar_absent test
(tiny_repo has no sidecar → note path exercised).
docs/advanced-usage.md: replaces the one-line overlap-gate note with
a full behavioral×static fusion section covering ingest-sarif usage,
dialect notes (Semgrep/clippy/CodeQL variance from §VALIDATED),
finding-hotspot-overlap column reference + priority rules, gate config
example, and finding_hotspot_overlap MCP tool entry.
README: Behavioral×static fusion paragraph with the ingest-sarif →
overlap → gate → MCP tool pipeline.
CHANGELOG: MCP entry updated from seven to eight tools.
Removes .superpowers/sdd2/task-B4-report.md from tracking (file stays on disk) and git-ignores the .superpowers/ scratch directory so session ledgers and task reports can no longer ride into commits.
… emit vacuous SARIF Four external-findings correctness fixes in the check/analyze/ingest paths: - Empty-but-present sidecar now takes the same skip path as an absent one in the max_findings_in_hot_files gate. Previously a present-but-empty store made run_finding_hotspot_overlap_with return Err, aborting the whole check with exit 4 — contradicting the documented skip contract. The Some(store) arm now checks the row count and joins the None case when zero, mirroring the MCP overlap tool. - analyze --analysis finding-hotspot-overlap now opens the sidecar under --cache-dir instead of the hardcoded default XDG root, matching where the FactsDb and ingest-sarif read/write. - Re-ingesting a clean (zero-result) SARIF scan now clears that engine's stale rows. parse_sarif discards engine names of zero-result runs, so the grouping step never saw them; a new parse_sarif_engines companion surfaces every engine present, and ingest seeds empty batches for them so replace_engine drops the prior run's findings. Restores the IngestSarif contract that the stored count is always the current run. - check --format sarif on a repo with no thresholds now emits a valid zero-result SARIF document instead of printing nothing, so the documented upload-sarif pipeline no longer breaks on a vacuous pass.
Doc- and comment-only truth fixes, no behavior change: - Reword task/plan-ID references to state the current contract: drop the §VALIDATED, per-R7, B3, B1, and A3 tags in favor of the fact each was standing in for (SARIF 2.1.0 verified, connection-isolation rationale, the overlap join, the dialect fixtures, the schema-constant check). - advanced-usage.md §11.8: the SARIF evidence entry shows date, author, message head, and churn — not the full commit SHA — and the chain is ordered by recency (commit date, then revision), not by how heavily a file was touched. Correct the description to match the emitter. - advanced-usage.md + mcp.rs check_gates: the MCP tool evaluates a subset of codelore check (no max_findings_in_hot_files, no degraded/ratchet), so its verdict can diverge from a CI run. State that exception instead of the old never-contradicts claim; name codelore check authoritative. - CHANGELOG [Unreleased]: add the check --cache-dir flag line.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This branch ships two SARIF features.
Evidence chains:
codelore check --format sarifandcodelore diff --format sarifnow attach per-finding commit lineage (top-5/check, top-3/diff, newest-first, rename-aware via the canonical-lineage fact store) as SARIFcodeFlows+relatedLocations, with versionedpartialFingerprints(gateFinding/v1,primaryLocationLineHash) for stable GitHub Code Scanning identity;--formatis a typed clapValueEnumand verdict lines stay on stderr with exit codes unchanged.Ingest-SARIF fusion:
codelore ingest-sarifparses external scanner SARIF (CodeQL/Semgrep/clippy dialect variance handled via documented fallback chains) into a prune-safe per-repo sidecar (external-findings.duckdb-ext,(engine,fingerprint)PK, transactional per-engine replace); the newfinding-hotspot-overlapanalysis joins findings × revision-percentile (SQL-equivalent tie ranks) × code-health band intoact-now/plan/notepriorities; themax_findings_in_hot_filesgate enforces anact-nowceiling incheck(row-reuse, skip-with-ledger-record when the sidecar is absent or empty, never creates the sidecar on read); an eighth MCP tool exposes the same table.~1,900 lines of new tests cover dialect parsing, store lifecycle, percentile semantics, fingerprint stability, and end-to-end CLI/MCP contracts.
Verification
just ci(fmt + clippy-D warnings+ cargo-deny + full workspace tests) green at HEADKnown debt (deliberately shipped, tracked)
check --ratchet --format sarifcombination emits no SARIF (ratchet paths return before emission)tracing::warn!would help)partialFingerprints(check results carry two) — asymmetric GitHub dedupanalyze_head_at_revduplicates ~32 lines ofanalyze_at_rev(refactor candidate)