Fix/delta 1n split body collision#8489
Open
wdower wants to merge 4 commits into
Open
Conversation
When several new controls resolve to one old control (a 1:N split), the
requirement-first matcher mapped every sibling (primary AND related) into
controlMappings, so all of them were set up to inherit the same old InSpec
body. The mapped-control file is named by old id, so siblings then collided
on ${oldId}.rb and only the last-written one kept the body — arbitrarily,
and frequently not the primary.
Restore the original matcher intent: only the primary (best match) is added
to controlMappings and inherits the body. Related links are emitted as new
controls without a body for the author to fill, while remaining in
delta.json links[] and the dupMatch counter. This also makes each old id
appear at most once in controlMappings, so the file write can no longer
collide.
Report the related (bodiless) controls so reviewers can find close matches:
the markdown report gains a "Related Controls (no body copied)" section
listing each related control (sorted by confidence, highest first) with its
best-match old control, and the per-link log distinguishes related from
mapped primaries.
Update statistics validation to the primary-only model: mapped ==
match + posMisMatch; full accounting == mapped + related + noMatch.
Validated on the real 214-control SLES15 derivation: 193 primaries get a
body, 13 related are bodiless stubs, invariants hold. Adds mapControls and
report-formatter unit tests.
Signed-off-by: Will <will@dower.dev>
Refine the "Related Controls (no body copied)" report section: instead of a flat confidence-sorted list, group the related links by the old control they matched, name the primary new control that kept that old control's body, and label each entry `related -> <newId>`. Groups are ordered by their strongest related match (closest first). This makes the 1:N split shape explicit (which new controls share an old control, and which one received the body) and removes the old-vs-new ambiguity of the flat list without introducing a markdown table — the delta report uses no tables or column headers elsewhere, so a labeled grouped layout stays consistent with its house style. Signed-off-by: Will <will@dower.dev>
There was a problem hiding this comment.
Pull request overview
Fixes saf generate delta -M behavior for 1:N control splits by ensuring only the selected primary match inherits the old control’s Ruby body, and adds a dedicated report section to surface related (non-primary) matches for manual review.
Changes:
- Change
mapControlsso onlyprimarylinks are added tocontrolMappings(only primaries inherit bodies);relatedlinks remain visible indelta.jsonand logs. - Add a “Related Controls (no body copied)” section to the generated delta markdown report, grouped by old control and sorted by confidence.
- Add unit tests validating the new 1:N body-copy policy and the related-controls report formatting.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/commands/generate/delta.test.ts | Adds unit tests for 1:N primary-only body-copy behavior and related-controls report output. |
| src/commands/generate/delta.ts | Enforces primary-only body copying in mapControls, updates stats validation, and appends a related-controls report section to delta artifacts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // controlMappings so they are emitted as new controls without a body. This | ||
| // also guarantees no two new controls share an old id in controlMappings, so | ||
| // the mapped-control file write cannot collide. | ||
| describe('Test generate delta mapControls 1:N body-copy policy', () => { |
Comment on lines
+702
to
+717
| // Only the PRIMARY (best) match to an old control inherits its Ruby | ||
| // body. A `related` link is a weaker/secondary new control whose | ||
| // primary already claimed that old body; copying it here would | ||
| // duplicate the same test code across distinct controls (and, for | ||
| // spurious SRG/CCI co-bucketing, graft outright wrong code). Related | ||
| // controls are instead emitted as new controls WITHOUT a body for the | ||
| // author to fill — matching the original matcher's "best match wins the | ||
| // body, duplicates are skipped" behavior. They remain in | ||
| // delta.json links[] (and the dupMatch counter) for triage visibility. | ||
| // | ||
| // This also guarantees each old id appears at most once in | ||
| // controlMappings, so the mapped-control file write (keyed by old id) | ||
| // cannot collide between siblings of a 1:N split. | ||
| if (link.relationship === 'primary') { | ||
| controlMappings[newId] = link.oldId; | ||
| } |
Comment on lines
+815
to
+817
| const conf = `${(l.confidence * 100).toFixed(0)}%`; | ||
| const flag = l.potentialMismatch ? ' ** potential mismatch **' : ''; | ||
| return ` related -> ${basename(l.newId)} [${l.matchMethod}, confidence=${conf}]${flag}`; |
Rework how the requirement-first matcher designates the `primary` link in a 1:N control split (the new control that inherits the old InSpec body). Old behavior labeled the first-claimed candidate `primary` regardless of match quality, then nothing corrected it — so the body could go to a far weaker match while a stronger sibling was left `related`. Candidate-first model: the tier functions now build every match as a `related` candidate (`makeLink`) and only register the claim for the prefer-unclaimed spreading heuristic (`registerClaim`). `electPrimaries` is the single authority that designates primaries — for each old control it elects one primary and recomputes `potentialMismatch` from the final relationship. Election is tier-first (`matchMethodRank`: deterministic > cci-tiebreak > fuse-fallback) and only breaks within-tier ties by confidence, because confidence is not comparable across tiers (tier 1 = 1.0, tier 2 = CCI Jaccard, tier 3 = 1 - Fuse score) — so a cross-SRG fuzzy match cannot outrank a same-SRG match on the same old. Scope: this elects the best recipient among the controls that landed on a given old; it does not make the greedy assignment globally optimal (a control can still be routed off its strongest-but-claimed old). That is the requirement-text matcher rework's concern, documented on `electPrimaries`. Signed-off-by: Will <will@dower.dev>
… + stats coverage - Drop the dead `(potential mismatch)` branch from the related-controls report: `related` links can never carry the flag (it is primary-only), so the branch was unreachable. Confidence is the per-row risk signal. - Reset GenerateDelta static state via `beforeEach` instead of a per-test first line, so isolation no longer depends on author discipline. - Add the missing coverage the review flagged: a multi-old (tier-2) block where the last-processed, highest-confidence sibling correctly inherits the body (the core fix, previously only exercised by tied tier-1 blocks); a low-confidence elected primary incrementing posMisMatch; and the primary-only statistics invariants. Signed-off-by: Will <will@dower.dev>
|
Contributor
Author
|
After copilot review, realized that SAF CLI presently just picks as primary whatever the first match it makes is regardless of whether it finds a stronger match later. Adjusted logic to just consider all matches a "candidate" until all controls have been mapped, and then pick the strongest as the 'primary' and the rest of the candidates become 'related.' |
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
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.
Resolves #8488. In short -- in cases where an 'old' control in a source profile had several reasonable matches in the 'new' baseline, SAF CLI would pick whichever the last reasonable match it saw to copy the InSpec body to, instead of the best match (which we were already calculating, and referring to as the primary match).
This PR a) makes SAF CLI only map an old control's InSpec body to its primary match in the output, and b) adds a report section to the delta artifacts that specifically calls out old controls with extra 'related' matches for further review after delta run. Delta now says "we copied the code block into the best candidate, but this other set of candidates was close enough that you should probably check."
Tried to match existing report style for the new related-controls report section. Looks like this:
Since this one is a bugfix and I had a discussion with @aaronlippold to validate that we do in fact only want to copy code to the primary match, I didn't ADR this one before putting up the full PR with code changes.