[patelankeet2]

Closes item 2 on #402 (Arsh's post-merge review of #389), assigned to me and Samika. The other items on that checklist (device-presence filtering, coverage wording, differential searchFilters, docs) are Shubham's or joint follow-ups and are not touched here.

What was wrong

1. Propose-a-change never used the staged policy. propose_change() had no policy parameter at all, so /api/propose always compared a proposed change against Netwise's built-in example rules -- even when the user had uploaded their own via /api/policy. Worse, the finding's own evidence then said "no policy file was supplied", which was true of that specific call and false about the app as a whole.

2. The warning banner could claim the wrong direction. ai/propose.py's warning text was keyed on severity == "high" and unconditionally said "newly opens access". For a policy finding that assumption breaks: severity there tracks whether a breach was newly created, and a created breach can be a requirement violated -- which CLOSES access that should stay open, the opposite of what the text claimed.

Live reproduction, before touching anything

Staged a policy with a high-severity requirement rule (DNS to the approved resolver must work), then proposed blocking that exact flow:

BEFORE this PR, direct call:
  source: "Netwise's built-in example policy (analysis/checks/policy_compliance.py)
           -- no policy file was supplied; rule 4"
  severity: medium   (the BUILT-IN rule's own violation_severity, so the wording bug
                       doesn't even surface with it -- only with a user's OWN high-severity
                       requirement, exactly Arsh's scenario)

Constructed a high-severity version directly against change_impact.analyse_change(policy=...):
  CH-002  high  "This change BREAKS policy rule 4"  Direction: tightening
  current propose.py logic: warning=True, wording would say "newly opens access"
  ACTUAL effect: the change BLOCKS required DNS traffic -- it closes access, not opens it

What changed

1. analysis/change_impact.py -- every finding now carries a structured direction key (opening/closing/changed), attached after make_finding() returns (same place explanation/explanation_source are attached -- F-1's frozen schema is untouched, no four-way amendment needed). For the filter and reachability producers this was already implicit in severity; for the policy producer it was not, which is the actual bug.
2. ai/propose.py -- propose_change() accepts policy=None, threaded into the one analyse_change() call it already makes. The warning's trigger stays severity == "high" unchanged (under-warning a serious change is the dangerous direction, same reasoning as the verified/warning split from #13/#14: propose a config change from plain English, and simulate its impact #183) -- only the wording changes, checking the real direction field instead of assuming severity implies it.
3. web/main.py -- /api/propose now passes policy=_staged_policy(), the exact same accessor /api/findings already uses.

Verified end to end, not just unit tests

After the fix, same live sequence through the real running server (real session, real uploaded sample, real staged policy file):

"source": "the policy file you supplied; rule 4",
"direction": "closing",
"warning": true,
"answer": "...Warning: simulating this change has a serious effect that is not simply
            opening access -- see the impact list for exactly what changed."

Regression check: a request with no policy staged produces byte-identical output to before this PR.

A real mutation-testing gap, found and closed in its own commit

First version of the ai/propose.py commit shipped with every existing test accepting a policy keyword in its stub but none of them checking its value -- reverting the policy=policy line back to dropping it entirely left the whole suite green. Caught by mutation-testing my own commit, fixed in a follow-up commit (d005831) that captures the real call and pins both the supplied-policy and omitted-policy cases.

Acceptance criteria (against #402 item 2)

• A proposal checked against a staged policy actually uses it, not the built-in example
• The finding's evidence names the real policy source
• The warning banner never claims "opens access" for a change that closes required access
• The warning still fires for every high-severity finding (no narrowing of the safety trigger)
• No-policy behaviour is unchanged (regression-tested)

Testing

python -m pytest tests/test_change_impact.py tests/test_change_against_policy.py tests/test_propose.py tests/test_web_propose_endpoint.py -v
  166 passed

python -m pytest tests/ -q
  1896 passed, 13 skipped   (main: 1885 passed, 13 skipped -- +11 accounted for by new tests)

Mutation-tested each commit independently (direction field, policy threading at both the ai/propose.py and web/main.py layers) -- each targeted revert killed exactly the test(s) it should have, nothing else.

@SamikaPerera -- requesting your review since these are files #402 names as ours jointly. @ARSH871-bot -- flagging you too since you raised both findings on #389's review; happy to take a second look from you as well if you have time.

[shubhamkataria2005]

Verified rather than read, merged onto current main (dc93ccb): 1896 passed, 13 skipped with Batfish running.

Both bugs from #402 item 2 are fixed. I staged a user policy with one high-severity requirement (DNS to 218.8.104.58 must work), then proposed blocking exactly that flow:

verified: True   warning: True
CH-001  found  medium  direction=closing  A rule change blocks traffic that acl_in used to allow
CH-002  found  high    direction=closing  This change BREAKS policy rule 4 (DNS to the approved resolver must work)
answer: ... Warning: simulating this change has a serious effect that is not simply opening access

The user's own rule is the one evaluated, it keeps its own severity, and the banner no longer says "opens access" for a change that closes it. That was the scenario Arsh described, and it's gone.

One question, and it's for @ARSH871-bot, not a code problem

I'm leaving this as a comment rather than an approval because of one claim in the code comment:

attached after make_finding() returns (same place explanation/explanation_source are attached -- F-1's frozen schema is untouched, no four-way amendment needed)

That comparison doesn't hold. explanation is attached in web/main.py:1012, at the response layer, downstream of analysis. direction is attached inside analysis/change_impact.py, so the analysis producer itself now returns findings with an eighth key.

Whether that needs an amendment is genuinely open. But it's the same question Arsh pressed on my #390 last week: there the extra key was optional and populated nowhere, and it still got a sign-off table because §7a puts field vocabulary under all four. This one is populated on every change_impact finding. I'd rather the team decide it once than have #390 and #405 answer it in opposite ways a week apart.

Two cheap ways through, if you'd rather not amend:

• analyse_change() returns the direction beside the findings, as a mapping from id to direction, instead of inside them. That's my function, and I'm happy to change its signature with you.
• Or ratify direction the same way as #390's key set: one table, values opening / closing / changed.

Heads-up on merge order

I'm about to fix #403 and #402 items 1, 3, 5 and 6, all in _policy_change_findings, which this PR also restructures. I'll build on main and rebase onto yours once it lands, so you're not blocked on me. Whatever lands second absorbs the conflict, and that will be me.

[patelankeet2]

You're right, and I should have caught it myself rather than reaching for a comparison that doesn't hold. explanation/explanation_source are attached in web/main.py, at the response layer, strictly downstream of analyse_change() returning. direction is attached inside analysis/change_impact.py itself, on every finding the producer makes -- that's a different claim than "a key some caller adds afterward", and I wrote the comment as if it were the same one.

Agreed this is the same question as #390's, not a smaller version of it. If anything it's a stronger case for a table: A-3's subject was optional and populated nowhere when it needed four signatures; direction is unconditional and populated on every change_impact finding already on main (once this merges). I'd rather not have #390 and #405 answer "does an unconditional producer-side key need ratification" two different ways a week apart either.

Of your two options, I'd lean toward ratifying it the way #390 did -- a table in docs/finding-format.md, values opening/closing/changed -- over moving it outside the finding, mostly because a caller reading impact for any OTHER reason (a future dashboard view, say) would have to know to go look in a second mapping rather than finding it on the finding itself. But that's a preference, not a unilateral call, and I'll go whichever way the four of us land on it.

@ARSH871-bot -- happy to write the table's wording if you'd rather not, same way I did for #390's.