[ARSH871-bot]

#308's item 2, the test that can fail, plus the live instance it found on main.

The bug, measured on main (fa568f7) with real Batfish

A PF Sense upload whose DHCP WAN rule cannot be converted (the client's real shape):

dashboard     X-Netwise-Conversion-Gap-Count: 1                   says so
HTML report   "Excluded during conversion" ... ['wan'] ...         says so
CSV report    three could-not-check rows, no trace of the skip     SILENT

The CSV is the export most likely to be forwarded, and it read as a complete account of a firewall with a rule left out. This is #308's rule broken in the direction the issue calls dangerous: the silent surface is the one that looks complete.

The fix

render_csv() writes an excluded_during_conversion column on every row, taking its value from the same coverage.summarise() the HTML uses:

state	cell
something excluded	the skip notes, joined
record unreadable	UNKNOWN -- the record of what conversion excluded could not be read ...
nothing excluded	empty

It sits on every row for the reason report_subject already does (#347): a single labelled row does not survive sorting, filtering or pasting a few rows into an email.

The same upload on this branch:

id,...,report_subject,excluded_during_conversion
AC-001,...,device.cfg,"['wan'] have no static address configured (DHCP or unconfigured) -- ... 1 rule(s) skipped, not converted"
PC-050,...,device.cfg,"['wan'] have no static address configured ..."
RT-050,...,device.cfg,"['wan'] have no static address configured ..."

Kept out of CSV_COLUMNS on purpose. #298's comparison CSV writes its header from CSV_COLUMNS and builds its rows by hand. Adding the column there would leave that header one column wider than its rows the moment both land, and no merge conflict would show it. @patelankeet2, nothing for you to change; I wanted you to know why the constant is separate.

The test, #308 item 2

tests/test_coverage_on_every_surface.py: one upload through the real endpoint and the real PF Sense converter, three surfaces, three states.

a skipped rule          dashboard count 1      HTML names it      CSV names it on every row
nothing excluded        dashboard count 0      HTML silent        CSV empty
unreadable record       dashboard unreadable   HTML "could not    CSV UNKNOWN
                                                be read"

No Batfish: only pipeline._analyse is stubbed, with canned clean findings. That is the case where a silent surface does most harm, because nothing else on it hints at a gap.

As @SamikaPerera asked on #308, its docstring says the surfaces are enumerated, not discovered. A fourth surface added later will not be caught unless someone adds it to the test. The saved-scans comparison (#223) already records skips and is the obvious candidate to check next.

Mutation testing

7 mutants, each killed, with Batfish running and stopped:

endpoint does not give the CSV the skips     KILLED  only by the new test
CSV cell always empty                        KILLED  only by the new test
unreadable record read as nothing excluded   KILLED  only by the new test
cell written on the first row only           KILLED  only by the new test
endpoint does not give the HTML the skips    KILLED  new test + test_the_downloaded_report_names_a_skipped_rule
dashboard gap count always 0                 KILLED  new test + an existing header test
dashboard unreadable flag always false       KILLED  new test + an existing header test

The first four are the point: before this, nothing could object to the CSV going silent.

full suite   Batfish running   1820 passed, 10 skipped
             Batfish stopped   1793 passed, 37 skipped
ruff check --select E4,E7,E9,F (0.14.14)   clean

One existing test changed: test_no_dashboard_only_text_leaks_into_either_format pinned the CSV header to exactly CSV_COLUMNS. It now pins CSV_COLUMNS + CSV_REPORT_COLUMNS, so a dashboard-only key leaking into the export still fails it.

Where #308 stands with this merged

• Item 1, the rule: agreed by @patelankeet2 and @SamikaPerera. I take Samika's stricter wording ("a surface that cannot carry it must say so, not omit it"), and it is what this test enforces. @shubhamkataria2005 has not weighed in, so I have not marked the issue closed.
• Item 2, the test: this PR.
• Item 3, the two original instances: fixed on Strengthen propose-a-change: full diff, download, full scan, and a before/after comparison (#295) #298 (still open) and pfsense: one bad rule no longer sinks the whole file (#78 item 4) #302 (merged). This PR fixes a third instance, the CSV.

Reviewers: @SamikaPerera (/web/, and he offered to co-write this test), @patelankeet2 (#302's header half, and #298's CSV).

[patelankeet2]

Verified independently rather than trusted from the PR's own tables.

• Diffed against the real merge-base: exactly the 4 files, 179/-4, matching the stat claimed.
• Checked coverage.summarise()'s actual return dict directly in analysis/coverage.py -- conversion_gaps and conversion_gaps_unreadable are real keys, not assumed from the PR body. Confirmed render_html() calls the same function the same way (_coverage_html(), line 343), so the "same three states, from the same function, cannot disagree" claim is true by inspection, not just by argument.
• Ran tests/test_coverage_on_every_surface.py and tests/test_report_export.py on the branch: 41 passed.
• Independently mutation-tested one of the table's own claimed mutants rather than taking it on trust: reverted web/main.py's CSV call to the old render_csv(results, source=subject) with no conversion_gaps. Two tests failed for the right reason (test_a_skipped_rule_is_named_on_every_surface, test_an_unreadable_record_is_unknown_on_every_surface), both asserting the CSV column stayed empty/unaware instead of naming the skip. Reverted after.
• Full suite on the branch: 1817 passed, 13 skipped, 0 failed. ruff check --select E4,E7,E9,F on the four changed files: clean.

One non-blocking note, not a request for changes: the multiple-gaps case ("; ".join(...)) isn't exercised by a test with more than one skip -- the fixture only produces one. Low risk given each note is an internal structured message rather than arbitrary text, but worth a parametrized case if this file gets touched again.

Approving.

[ARSH871-bot]

@patelankeet2, thanks. On the multiple-gaps note, I measured it rather than pushing a test, because a push would void your approval on 0aeeaa9.

A PF Sense file with two different skip causes (a DHCP WAN carrying a rule, and a rule naming an openvpn interface that <interfaces> never declares), through the real endpoints on this branch, against real Batfish:

converter skipped 2 notes
dashboard   X-Netwise-Conversion-Gap-Count = 2
HTML        contains both notes (escaped)                     True
CSV         3 rows, 1 distinct value in the column            every row agrees
            cell == "; ".join(notes)                          True
            cell.split("; ") gives back the notes exactly     True

So the join holds today, and splitting on "; " recovers the notes because no converter message contains that separator. That property is not pinned, though: a future message containing "; " would make the cell ambiguous. I will add exactly this case as a parametrised test the next time this file is touched, as you suggested, and I have noted it on #308 so it is not lost.