pfsense: one bad rule no longer sinks the whole file (#78 item 4)

CLAUDE.md

@@ -236,6 +236,85 @@ Batfish runs in Docker container `batfish` (image `batfish/allinone`), exposing
   > missing `<type>`/`<protocol>` question (#80) are unaffected by this and
   > remain open.
 
+  > **UPDATE, 5 September.** The rest of #78 was picked up without waiting
+  > for Senaka's answers to the four questions now written out in Discussion
+  > #294, on the reasoning that a decision genuinely blocked on him should
+  > stay blocked, but nothing else should sit idle in the meantime. That
+  > split the remaining work into two very different piles.
+  >
+  > **The blast radius, fixed.** A single rule failing to convert for ANY
+  > reason (missing or unsupported `<type>`, an unsupported `<protocol>`
+  > value, an unresolved alias, a bad port, an unresolvable network
+  > reference) used to abort the entire file, not just the interface or rule
+  > it was on. At the client's real scale — 1,998 XML elements against this
+  > project's 54-element fixture — one anomaly anywhere meant zero analysis
+  > of anything. Re-scoped the same way the DHCP-WAN/undeclared-VPN case was:
+  > the one failing rule is excluded and named, everything else converts
+  > exactly as if it were never there. This required no guess about what any
+  > field means — every refusal still names the same cause it always did,
+  > only its blast radius shrank from "the whole file" to "this one rule".
+  >
+  > **`<protocol>`'s semantic question, still untouched.** #80 itself says
+  > "do not fix this before the client answers" — that instruction stands,
+  > unconditionally. What DID move: missing `<type>` now gets its own precise
+  > message, split from present-but-invalid the same way the two interface
+  > causes were split earlier, because there is no safe default for
+  > pass/block/reject the way "any" is one for protocol. `<protocol>`'s
+  > existing default-to-`"any"` behaviour was not touched at all.
+  >
+  > **NAT, reaffirmed rather than left stale.** Checked against the same
+  > "no guessing" bar the blast-radius fix cleared: it does not clear it.
+  > There is no version of "convert the NAT rule" that does not invent
+  > translation semantics this module has never observed. Detection and
+  > exclusion (#264) is unchanged, and `docs/design/pfsense-nat-support.md`
+  > records that this was revisited today, not simply never revisited.
+  >
+  > A separate idea — disclosing an inferred `<protocol>` with a comment,
+  > #80's own preferred Option C, which changes no behaviour, only makes the
+  > existing silent default visible — was drafted as a question for the team
+  > rather than built solo, precisely because it touches the one issue
+  > marked blocked on the client. Not yet decided.
+
+  > **UPDATE, 6 September — the blast-radius fix has a direction, and one
+  > direction is dangerous.** @SamikaPerera's review on #302 found what the
+  > entry above did not name: excluding a rule changes what the emitted ACL
+  > decides, and the two directions are not equally safe.
+  >
+  > **Skipping a `block` rule makes the emitted ACL MORE permissive than the
+  > real device.** Netwise then reports traffic as reachable that the real
+  > firewall denies — noisy, but safe, because the false finding gets
+  > investigated and dismissed.
+  >
+  > **Skipping a `pass` rule makes the emitted ACL STRICTER than the real
+  > device**, and this is the direction that matters. A policy rule asserting
+  > *"this traffic must be denied"* can then report `none` (clean) against
+  > the emitted ACL, while the real firewall actually permits that traffic —
+  > a real policy violation, reported as a pass. That is F-4's exact failure,
+  > reached through the converter instead of through a check: the report says
+  > "we checked and it is fine" about something nobody actually verified.
+  >
+  > Not new IN KIND — #104's interface-level skip made the identical trade,
+  > accepted at the time. What changed is conspicuousness: a whole excluded
+  > interface is visible in the skip list as a named gap; a single dropped
+  > `pass` rule inside an otherwise-normal ACL is not, unless someone reads
+  > the skip notes and reasons through what they mean for a specific policy
+  > rule.
+  >
+  > **Fixed, not just documented**: `analysis/coverage.py` and
+  > `analysis/report.py` (PR #302) now carry the skip list through to the
+  > downloaded report as its own "excluded during conversion" section, and
+  > the report's coverage statement stops claiming "nothing was skipped"
+  > when a conversion excluded a rule, even when every check ran clean. That
+  > closes the disclosure gap Samika also found — before this, the skip list
+  > reached the user exactly once, at upload, and was gone by the time a
+  > report was generated. It does not close the underlying modelling gap
+  > above: a skipped `pass` rule is still excluded, not modelled, and a
+  > policy check can still report `none` where the real firewall would not.
+  > Carrying skips into F-1 findings directly (an `error` card per skip,
+  > visible on the dashboard itself, not only in a downloaded report) was
+  > raised as the more complete fix and deliberately left for a separate,
+  > properly scoped change rather than folded into this one -- see #306.
+
 ## 7a. The finding format (F-1) — the one contract
 
 **`docs/finding-format.md` is authoritative.** It was agreed by all four team

analysis/coverage.py

@@ -11,11 +11,46 @@
 * status="error" means Netwise could not check something. That row must remain
   visible even when the main finding sections are skimmed, exported, or sorted
   elsewhere.
+
+CONVERSION GAPS ARE A SEPARATE THING FROM A CHECK GAP, AND THEY ARE PASSED IN
+SEPARATELY RATHER THAN INFERRED (#302 review, @SamikaPerera)
+    A finding-based gap means a CHECK could not run. A conversion gap means
+    part of the SOURCE FILE was never turned into anything a check could see
+    at all -- today, a pfSense rule `analysis/pfsense_convert.py` could not
+    convert (#78). Neither this module nor its caller can derive that from
+    the findings list, because a converter-excluded rule leaves no finding
+    behind to derive it from -- that is exactly the gap.
+
+    Before this parameter existed, `summarise()` had no way to know a
+    conversion had dropped anything, so a config with every CHECK running
+    cleanly reported "Every reported check ran. Nothing was skipped." even
+    when real rules from the uploaded file were never analysed. Not false
+    about the checks -- every one of them did run -- but read by a person as
+    a single claim, and the wrong one: skipping a `pass` rule makes the
+    emitted config STRICTER than the real device, which can make a policy
+    check that should report a violation report `none` instead. See
+    `CLAUDE.md` section 7 for the direction analysis. A reader who trusted
+    "nothing was skipped" would have no way to know that risk exists for
+    this specific report.
+
+`None` MEANS SOMETHING DIFFERENT FROM `()`, AND FROM A NON-EMPTY LIST
+(#302 review, @ARSH871-bot)
+    `web.main._staged_pfsense_skips()` returns three distinct things: `[]`
+    when it VERIFIED nothing was excluded, a non-empty list when it read
+    real skip notes, and `None` when a skip record exists but could not be
+    read -- truncated, wrong type, unparseable. Before this distinction
+    existed, an unreadable record and a verified-empty one both collapsed to
+    `[]` here, so a corrupted skip file produced the exact same "nothing was
+    skipped" claim as a config that genuinely excluded nothing. Passing
+    `None` through lets this function tell a reader the true thing: not
+    "nothing was skipped", but "whether anything was skipped could not be
+    determined" -- and `complete` is `False` in that case for the same
+    reason it is `False` when something genuinely was skipped.
 """
 
 from __future__ import annotations
 
-from typing import Any, Dict, Iterable, List, Tuple
+from typing import Any, Dict, Iterable, List, Optional, Sequence, Tuple
 
 
 def _label(finding: Dict[str, Any]) -> Tuple[str, str]:
@@ -25,9 +60,24 @@ def _label(finding: Dict[str, Any]) -> Tuple[str, str]:
     )
 
 
-def summarise(findings: Iterable[Dict[str, Any]]) -> Dict[str, Any]:
+def summarise(
+    findings: Iterable[Dict[str, Any]],
+    conversion_gaps: Optional[Sequence[str]] = (),
+) -> Dict[str, Any]:
     """Return the coverage/certainty view implied by a findings list.
 
+    `conversion_gaps` is optional and additive -- omitting it reproduces this
+    function's exact prior behaviour, so every existing caller keeps working
+    unchanged. Pass it when the findings came from a converted config and the
+    converter excluded anything, so the statement below can stop claiming
+    nothing was skipped when something genuinely was -- just not something a
+    check could have reported on.
+
+    Pass `None` rather than `()` when the CALLER genuinely does not know
+    whether anything was excluded -- see this module's own docstring for why
+    that is a third, distinct state rather than the same thing as "verified
+    empty".
+
     The result is intentionally plain dicts, matching the rest of the analysis
     layer's F-1 data. It is derived, not cached; if the findings change, the
     coverage statement changes with them.
@@ -59,6 +109,10 @@ def summarise(findings: Iterable[Dict[str, Any]]) -> Dict[str, Any]:
         {"check": check, "device": device}
         for check, device in sorted(checked)
     ]
+    conversion_gaps_unreadable = conversion_gaps is None
+    conversion_gap_rows = (
+        [] if conversion_gaps is None else [str(note) for note in conversion_gaps]
+    )
 
     if not checked and not gaps:
         # THE THIRD STATE, AND IT IS NOT COMPLETENESS.
@@ -80,6 +134,36 @@ def summarise(findings: Iterable[Dict[str, Any]]) -> Dict[str, Any]:
             f"Findings may be incomplete."
         )
         complete = False
+    elif conversion_gaps_unreadable:
+        # THE CASE #302's REVIEW (ROUND TWO, @ARSH871-bot) FOUND. Every
+        # check ran and none of them reported a blind spot, but the record
+        # of what a converter may have excluded before any check ran could
+        # not be read -- truncated, wrong type, unparseable. Complete is
+        # False here for the same reason it is False when the record names
+        # a real exclusion below: this report cannot say analysis was
+        # exhaustive when it does not know whether part of the source file
+        # was dropped before any check ever saw it.
+        statement = (
+            "Every reported check ran, but the record of what a converter "
+            "may have excluded before any check ran could not be read. "
+            "Whether anything was excluded is unknown, so this report "
+            "cannot claim the analysis is complete."
+        )
+        complete = False
+    elif conversion_gap_rows:
+        # THE CASE #302's REVIEW FOUND. Every check ran and none of them
+        # reported a blind spot -- but that is a claim about the checks,
+        # not about the file. Complete is False here on purpose: this
+        # report cannot say analysis was exhaustive when part of the
+        # source file was excluded before any check ever saw it.
+        one = len(conversion_gap_rows) == 1
+        statement = (
+            f"Every reported check ran, but {len(conversion_gap_rows)} part"
+            f"{'' if one else 's'} of the uploaded configuration could not "
+            f"be converted and {'was' if one else 'were'} not included in "
+            f"this analysis."
+        )
+        complete = False
     else:
         statement = "Every reported check ran. Nothing was skipped."
         complete = True
@@ -89,4 +173,6 @@ def summarise(findings: Iterable[Dict[str, Any]]) -> Dict[str, Any]:
         "statement": statement,
         "checked": checked_rows,
         "gaps": gaps,
+        "conversion_gaps": conversion_gap_rows,
+        "conversion_gaps_unreadable": conversion_gaps_unreadable,
     }

analysis/pfsense_convert.py

@@ -195,6 +195,11 @@
         "likely a named alias group"
     ),
     "unsupported_rule_type": "a rule's <type> is not pass, block or reject",
+    "missing_rule_type": (
+        "a rule's <type> element is absent -- cannot tell whether it is "
+        "meant to pass, block or reject, and guessing either direction "
+        "would misstate what the rule does"
+    ),
     "unsupported_protocol": (
         "a rule's <protocol> is not tcp, udp, icmp or any -- combined forms "
         "like 'tcp/udp' need two Cisco ACL lines and are not handled"
@@ -730,7 +735,13 @@ def _check_rule_order_is_unambiguous(
 def _rule_to_acl_line(rule_el: ET.Element, interfaces: Dict[str, Dict[str, str]]) -> str:
     """Convert one PF Sense <rule> element into one Cisco extended-ACL line."""
     pf_type = _text(rule_el, "type")
-    action = _PFSENSE_TO_CISCO_ACTION.get(pf_type or "")
+    # Absent gets its own message, distinct from present-but-invalid (#78
+    # item 4) -- "<type> is None" was the same confusing string either way,
+    # and the two are different questions: one is a value this module does
+    # not recognise, the other is a value that was never written at all.
+    if pf_type is None:
+        raise PfSenseConversionError(REFUSALS["missing_rule_type"])
+    action = _PFSENSE_TO_CISCO_ACTION.get(pf_type, None)
     if action is None:
         raise PfSenseConversionError(
             f"{REFUSALS['unsupported_rule_type']}: <type> is {pf_type!r}"
@@ -974,15 +985,52 @@ def convert(xml_path: Union[str, Path]) -> ConversionResult:
     #   module still refuses rather than guesses, because the general case
     #   -- some rules quick, some not, in no particular pattern -- does not
     #   reduce to a single reordering the way the all-or-nothing cases do.
+    # SKIPPED, NOT REFUSED (#78 item 4, re-scoped the same way item 1/2 were
+    # in #104): a rule that _rule_to_acl_line() cannot convert -- missing or
+    # unsupported <type>, an unsupported <protocol> value, a named alias
+    # where an address or port is expected, an out-of-range port, an
+    # unresolvable network reference -- used to abort THIS ENTIRE FILE, not
+    # just the interface it was on or the rule itself. At the client's real
+    # scale (1,998 XML elements against this project's 54-element fixture,
+    # per CLAUDE.md section 7), one anomaly anywhere meant zero analysis of
+    # anything. That is the same shape #104 fixed for an unmodellable
+    # interface, just one level deeper -- so it gets the same fix: exclude
+    # the one rule, name the real reason in `skipped`, convert everything
+    # else exactly as if it were never there.
+    #
+    # This changes NOTHING about what any field is interpreted to mean --
+    # every REFUSALS message below still names the same cause it always did.
+    # It only shrinks the blast radius of that cause from "the whole file"
+    # to "this one rule". In particular this does NOT touch <protocol>'s
+    # existing default-to-"any" behaviour, which is issue #80's own open
+    # question (blocked on the client) and is not this change's to answer.
     acl_lines_by_role: Dict[str, List[str]] = {}
     for role, rules in rules_by_role.items():
-        if not any(_is_quick(r) for r in rules):
-            rules = list(reversed(rules))
-            acl_lines_by_role[role] = [_rule_to_acl_line(r, interfaces) for r in rules]
-            continue
-        lines_for_role = [_rule_to_acl_line(r, interfaces) for r in rules]
-        _check_rule_order_is_unambiguous(rules, interfaces, lines_for_role)
-        acl_lines_by_role[role] = lines_for_role
+        no_quick_rules_at_all = not any(_is_quick(r) for r in rules)
+        # Position in a skip note is counted from ORIGINAL document order,
+        # before any reversal -- so the number matches what a human reading
+        # the XML top to bottom would count, not this function's internal
+        # processing order.
+        original_order = rules
+        ordered = list(reversed(rules)) if no_quick_rules_at_all else rules
+
+        kept_rules: List[ET.Element] = []
+        kept_lines: List[str] = []
+        for r in ordered:
+            try:
+                kept_lines.append(_rule_to_acl_line(r, interfaces))
+            except PfSenseConversionError as error:
+                position = original_order.index(r) + 1
+                skipped.append(
+                    f"rule {position} of {len(original_order)} on {role!r} "
+                    f"could not be converted: {error} -- skipped, not converted"
+                )
+                continue
+            kept_rules.append(r)
+
+        if not no_quick_rules_at_all:
+            _check_rule_order_is_unambiguous(kept_rules, interfaces, kept_lines)
+        acl_lines_by_role[role] = kept_lines
 
     lines: List[str] = [f"hostname {hostname}", "!"]
     for role, info in interfaces.items():
@@ -1004,7 +1052,15 @@ def convert(xml_path: Union[str, Path]) -> ConversionResult:
     for role in interfaces:
         acl_name = _acl_name(role, rule_bearing_roles=modelled_roles)
         lines.append(f"ip access-list extended {acl_name}")
-        if role in acl_lines_by_role:
+        # TRUTHINESS, NOT MEMBERSHIP (#78 item 4). A role can now be a key in
+        # acl_lines_by_role with an EMPTY list -- every one of its rules
+        # failed to convert and was skipped -- which used to be impossible:
+        # before the per-rule skip above, a role either had every rule
+        # convert or the whole file aborted, so a present key always meant
+        # real content. `role in acl_lines_by_role` alone would then emit an
+        # ACL with no lines in it at all, not even a fail-closed deny -- the
+        # exact fail-OPEN inversion the deny-all fallback exists to prevent.
+        if acl_lines_by_role.get(role):
             lines.extend(f" {line}" for line in acl_lines_by_role[role])
         else:
             lines.append(" deny ip any any")