Plan: Collect brain trust diagnostics into a SARIF emitter (6.5.1) - #354
Draft
leynos wants to merge 3 commits into
Draft
Plan: Collect brain trust diagnostics into a SARIF emitter (6.5.1)#354leynos wants to merge 3 commits into
leynos wants to merge 3 commits into
Conversation
Draft the execution plan for roadmap item 6.5.1: an opt-in, English-only SARIF 2.1.0 emitter for brain trust findings. The plan records the reconnaissance findings that shape it: the brain_type and brain_trait Dylint crates do not yet exist, so the emitter consumes the existing `BrainTypeDiagnostic` and `BrainTraitDiagnostic` values from `whitaker-common`; nothing in the tree currently writes a SARIF file, so the sink and opt-in mechanism are designed from first principles; and `SarifResult::partial_fingerprints` is a `HashMap`, which makes byte-identical output impossible until it becomes a `BTreeMap`. It proposes a new `whitaker_brain_sarif` crate with a hexagonal split between the pure finding/ordering/mapping domain and the settings and filesystem adapters, and plans verification jointly with implementation: Verus proofs for fingerprint-encoding injectivity and the result ordering, a bounded Kani harness for the collector, and property, snapshot, and behavioural tests for emission. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Revise the 6.5.1 execution plan after a six-lens design review. The plan now mirrors the clone detector's architecture instead of inventing a parallel one, and reuses the shared SARIF infrastructure rather than duplicating it. The emitter becomes a pure function returning an in-memory `whitaker_sarif::Run`, exactly as `emit_run0` does, with file writing left to whichever CLI or driver item takes it on. That removes the new crate, the `cap_std` sink, the ports-and-adapters layer, the invented `dylint.toml` configuration channel, and with them every operational hazard the review identified in running a writer inside rustc: incremental compilation replaying cached lint results, stale output files, compilation units colliding on a filename, and ambient filesystem authority in a crate the capability policy governs. Four helpers move from `whitaker_clones_core` into `whitaker_sarif` so both producers share one implementation, and each promotion fixes a real defect: fingerprint hashing gains an injective length-prefixed encoding in place of zero-delimited concatenation; file URIs gain a validated newtype; region conversion and its UTF-16 column convention stop being clone-local; and the existing `properties.whitaker` extension point is generalized into a discriminated family instead of gaining a rival namespace. `all_rules` is renamed to `clone_detection_rules`, whose contents it actually describes. Verification is rescoped to what can fail when the implementation is wrong. The Verus injectivity proof is kept and now protects both producers; the total-order proof is cut because `#[derive(Ord)]` on a tuple already guarantees it; and the Kani budget moves from a collector harness that would have verified a `BTreeMap` guarantee to span-to-region conversion, where `SourceLocation::new(0, 0)` is constructible but SARIF requires one-based positions. The cuts and their rationale are recorded as the execplans skill requires. Configuration is deferred to roadmap 6.6.1, which owns it and requires 3.6.3 for `ortho_config` and `whitaker.toml`. Also fixes a `Cargo.toml` that would not have parsed, the SARIF schema URL, and the spelling-gate failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Your free Security trial is over. An organization admin can activate billing to continue. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Reviewer's GuideAdds an execution plan document describing how to implement an opt-in, deterministic SARIF 2.1.0 emitter for brain trust diagnostics, reusing and refactoring existing SARIF and clone-detector infrastructure without adding any code changes yet. Sequence diagram for planned brain trust SARIF emission flowsequenceDiagram
participant LintCaller as BrainTrustCaller
participant BTS as brain_trust_sarif
participant WS as whitaker_sarif
LintCaller->>BTS: emit_brain_trust_run(mode, findings, tool_name, tool_version)
alt mode is Disabled
BTS-->>LintCaller: Ok(None)
else mode is Enabled
loop for each BrainTrustFinding
BTS->>WS: span_to_region(SourceSpan)
BTS->>WS: fingerprint_hex(components)
BTS->>WS: brain_trust_rules()
BTS->>WS: WhitakerProperties::BrainTrust
end
BTS->>WS: RunBuilder::build()
WS-->>BTS: Run
BTS-->>LintCaller: Ok(Some(Run))
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
The design review found that nothing creates `crates/brain_type/` or `crates/brain_trait/`. Roadmap items 6.2.2 and 6.3.2 are ticked but delivered only the evaluation layer in `whitaker-common`, while 6.6.3 already presupposes `ui/` directories under both crates. The SARIF emitter was therefore being designed without its caller in the room, leaving the span-to-URI seam as a guess that snapshots would have frozen. Close the gap in the roadmap rather than routing around it: - 6.1.3 requires an ADR formalizing the brain trust lint driver interfaces — span and `TyCtxt` to file identifier and `SourceSpan`, HIR traversal into the metric builders, how decomposition suggestions reach rendering, the lint-pass lifecycle for collecting findings, and the boundary between English SARIF text and localized diagnostics. - 6.2.4 and 6.3.3 create the two Dylint lint crates. - 6.5.1 now requires 6.1.3, so the emitter consumes those interfaces instead of inventing them. - 6.6.1, 6.6.2, and 6.6.3 move from requiring 6.2.2/6.3.2 to requiring 6.2.4/6.3.3, because configuration, localization, and UI tests genuinely cannot proceed until the crates exist. The execplan gains the ADR as a hard prerequisite: Stage A now ends only when both the plan is approved and 6.1.3's ADR is accepted, and the ADR is authoritative where it contradicts any signature the plan sketches. The plan also stops claiming an ADR number, since 6.1.3's will take the next free one. The edits were made directly rather than through `mapsplice`, which reindents every continuation line in the roadmap from two spaces to four — the broad spacing-only churn its own guidance says to stop on. No renumbering was needed, as all three tasks append within existing steps. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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
Draft execution plan for roadmap item 6.5.1 — collect brain trust
diagnostics into an opt-in, English-only SARIF 2.1.0 emitter — plus the roadmap
changes needed to make that item well-founded.
Plan document:
docs/execplans/6-5-1-collect-brain-trust-diagnostics-into-sarif-emitter.mdThis PR contains no implementation — only the plan and the roadmap edits.
The plan needs approval before any code is written.
Roadmap changes
Review found that nothing creates
crates/brain_type/orcrates/brain_trait/: 6.2.2 and 6.3.2 are ticked but delivered only theevaluation layer in
whitaker-common, while 6.6.3 already presupposesui/directories under both. Three tasks close the gap:
TyCtxtto file identifier andSourceSpan, HIR traversal into the metricbuilders, how decomposition suggestions reach rendering, the lint-pass
lifecycle, and the English-SARIF/localized-diagnostic boundary.
6.5.1now requires6.1.3, so the emitter consumes those interfaces ratherthan inventing them.
6.6.1–6.6.3move from requiring 6.2.2/6.3.2 torequiring 6.2.4/6.3.3, since configuration, localization, and UI tests cannot
proceed until the crates exist.
All three tasks append within existing steps, so nothing renumbers.
Approach
The plan mirrors the clone detector rather than inventing a parallel
architecture. The emitter is a pure function returning an in-memory
whitaker_sarif::Run, exactly asemit_run0does, with file writing left towhichever CLI item takes it on.
Four helpers currently in
whitaker_clones_coremove intowhitaker_sarifsoboth producers share one implementation. Each promotion also fixes a real
defect:
Stringvaries by working directory and platformproperties.whitakerSarifResult::partial_fingerprintsalso becomes aBTreeMap, without whichbyte-stable output is impossible.
Design review
A six-lens panel reviewed the first draft; findings are folded in and attributed
in the plan's decision log. Two were verified empirically: the originally
proposed
Cargo.tomldid not parse, and the spelling gate rejectedlocalisation(Oxford spelling takes-ize).The review also rescoped verification to obligations that can fail when the
implementation is wrong:
replaces a shipped encoding that lacks the property.
#[derive(Ord)]on a tuple alreadyguarantees it.
a
BTreeMapguarantee against a handwritten model) to span-to-regionconversion, where
SourceLocation::new(0, 0)is constructible but SARIFrequires one-based positions.
Cuts and rationale are recorded in the plan, as the execplans skill requires.
Validation
make markdownlintandmake nixieboth pass. No code changed.References
🤖 Generated with Claude Code