Skip to content

Change impact reports what it did not look at (closes #403, most of #402) - #406

Open
shubhamkataria2005 wants to merge 1 commit into
mainfrom
fix/403-402-change-impact-coverage
Open

shubhamkataria2005 wants to merge 1 commit into
mainfrom
fix/403-402-change-impact-coverage

Conversation

@shubhamkataria2005

Copy link
Copy Markdown
Collaborator

Closes #403. Addresses #402 items 1, 3, 4, 5, 6 and 7 — every item except 2, which is #405.

Every item below was reproduced first, mostly with @ARSH871-bot's own repro inputs from the #389 post-merge review.

#403 — a deleted router was "No change detected"

analyse_change() now compares the two snapshots' device lists. Each device added or removed is a found card; a side whose devices can't be listed is an error (CH-052), never "no change". Live, on #403's exact repro:

before = multi-device-10, after = the same minus rtr-dev009
before this PR:  CH-000 none  "No change detected"
after:           CH-00n found "Device rtr-dev009 is removed by this change"

#402

item what was wrong now
1 on a network without rtr-us5, five built-in rules → CH-051 ×5 → every proposal verified=False a rule about a device in neither config is not applicable, named in the result, not an error. Device on one side only → a single CH-051. Live repro with rtr-us5 renamed edge-fw1: no errors
3 "every policy rule" was every policy_compliance rule, silently a sentence starting "NOT compared:" names the access_control/routing rules it skipped; a policy with no policy_compliance rules says no rule of yours was checked
4 "still breached" sat in the green card; "neither causes nor fixes" overclaimed breached before and after is found. A differential searchFilters separates same traffic (low, not caused by this change) from different traffic (the rule's own severity, direction changed). "Unchanged" is scoped to the rule's filter
5 four mutants only the live test caught, and it skips on CI _rule_breached is tested against a fake session. Policy replaced by None, first arm only, snapshot not switched, kind ignored — all four now fail without Batfish
6 a failed reachability stage silently skipped the policy stage; CH-051 could collide with a 51st filter finding every stage runs whatever the others did. Ordinary findings stop at 48; 49+ are reserved (CH-049 summarises overflow, CH-050…054 are this module's own error cards). CLI prints renamed/assigned
7 stale docs module docstring lists what's reported and in what order; CLAUDE.md row current; the policy-rules section moved out from above §7's sign-off ticks into its own §8

Merge order with #405

#405 restructures _policy_change_findings too (_attach_direction around every make_finding). Whichever lands second takes the conflict, and if it's this one, that's me. If #405 lands first I'll rebase onto it and attach direction to the new card kinds here (altered → changed, device removed → closing, added → opening), assuming the open question on #405 about the extra key resolves that way.

Mutation-tested

device add/remove not reported              3 failed
absent-device rules made errors again       3 failed
set_active_policy(None) instead of (policy) 1 failed
only the first query arm asked              1 failed   (also without Batfish)
reachability failure skips policy stage     1 failed
no cap, ids can collide                     1 failed
still-breached back in the green card       2 failed
snapshot not switched                       2 failed   (also without Batfish)
kind ignored                                1 failed   (without Batfish)

1907 passed, 13 skipped with Batfish running. ruff clean on touched files.

)

#403. Deleting a whole router reported "No change detected" with a green
tick: neither compareFilters nor differentialReachability describes a device
that exists on one side only. analyse_change() now compares the device lists
of both snapshots and reports each added or removed device as `found`. A side
whose devices cannot be listed is an error (CH-052), never "no change".
Reproduced live on multi-device-10 minus rtr-dev009.

#402, from @ARSH871-bot's post-merge review of #389:

1. A rule about a device in NEITHER config is not applicable -- this change
   cannot affect it -- and is named as such, not reported as an error. Every
   built-in rule used to be CH-051 on a network without rtr-us5, so every
   proposal there was "could not be fully verified". A rule about a device on
   ONE side only is one CH-051. Reproduced live with rtr-us5 renamed edge-fw1.

3. Only policy_compliance rules are compared, and the result now says so: a
   sentence beginning "NOT compared:" names the access_control and routing
   rules it skipped, and a policy with no policy_compliance rules says no
   rule of the user's was checked.

4. A rule breached before AND after is `found`, never inside the green card.
   A differential searchFilters tells "breached the same way" (low, not
   caused by this change) from "breached by different traffic" (the rule's
   own severity, direction: changed). "Unchanged" is scoped to the rule's
   filter, because unbinding a filter changes the network without changing
   that answer.

5. _rule_breached is tested against a fake session, so CI now catches the
   four mutants only the skipped live test could: the policy passed in
   replaced by None, only the first query arm asked, the snapshot not
   switched, and the rule's kind ignored.

6. Every stage runs whatever the others did, so a failed reachability stage
   no longer silently skips the policy stage. Ordinary findings stop at 48;
   49 and up are reserved (CH-049 summarises any overflow, CH-050..054 are
   this module's own error cards), so a long diff cannot collide with them.
   The CLI prints renamed and assigned notes.

7. Docs: the module docstring lists what is reported and in what order,
   CLAUDE.md's change_impact row is current, and the policy-rules section
   moved out from above section 7's sign-off into its own section 8.

Not here: item 2 is #405 (Ankeet's). Shubham's rebase onto it, whichever
lands second, absorbs the conflict in _policy_change_findings.

Mutation-tested: device changes dropped, absent-device rules made errors,
set_active_policy(None), first arm only, reachability failure skipping the
policy stage, no cap, still-breached back in the green card, snapshot not
switched -- each fails the new tests; the arm, snapshot and kind mutants fail
WITHOUT Batfish.

1907 passed, 13 skipped with Batfish running. ruff clean on touched files.

@patelankeet2 patelankeet2 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVE. Tested, not just read.

Independently reproduced both headline fixes, live

#403's own repro (multi-device-10 minus rtr-dev009):
  CH-001 found medium rtr-dev009 | Device rtr-dev009 is removed by this change
  "No change detected"? absent, confirmed

#402 item 1's repro (rtr-us5 renamed edge-fw1, both sides):
  CH-000 none low | No change detected between the two configurations
  any status=="error"? False, confirmed

Both match the PR body's own tables exactly.

Read the full diff, not just the summary

Traced the F-4-sensitive part carefully: the old code returned immediately on a filter- or reachability-stage exception, abandoning every later stage. Now each stage appends a reserved-id error card (blind()) and continues, so a failed reachability check can no longer silently skip the policy stage -- and confirmed results = _cap(regular) + reserved means the "no change detected" sentinel is only reachable when BOTH are empty, so an error card can never collapse into a false all-clear. That's the right direction for F-4 and I checked it holds rather than assumed it from the description.

Mutation-tested one of your own claims rather than only trusting the table

Reverted _rule_breached to check only rule["queries"][0] instead of looping every arm -- test_rule_breached_asks_every_arm_not_just_the_first failed for exactly the right reason, nothing else did. Reverted.

Verified independently

python -m pytest tests/ -q                                             1907 passed, 13 skipped (matches)
python -m ruff check analysis/change_impact.py tests/test_change_impact_coverage.py
                     tests/test_change_against_policy.py tests/test_change_impact.py   all clean
git log -1 --format=%ad --date=short dc93ccb                           2026-10-14 -- matches the
                                                                        CLAUDE.md claim "#323 merged 14 October"

On the #405 merge-order note

Agreed with your plan -- whichever lands second absorbs the conflict. I've also replied on #405's own thread to your direction-ratification question (you're right that my comparison to explanation/explanation_source didn't hold; it's a producer-side key, not a response-layer one, so it's the same class of question as #390's). Not blocking this PR either way, just flagging that I've seen it from both sides so nobody has to repeat the finding a third time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

change_impact says "No change detected" when a whole device is removed

2 participants