[shubhamkataria2005]

First step of #238, not closing it. On #300 I said this change to analysis/findings.py would land on its own, ahead of the chaining work, so it could be reviewed on its own terms rather than arriving inside a feature PR. This is that.

@ARSH871-bot — analysis/findings.py is yours, so this is yours to say no to.

What it does

make_finding() accepts A-3's optional subject and validates it. Nothing populates it. A-3's order is the field first and a consumer next; the chaining in #238 is the consumer.

make_finding(..., subject={"dst": "10.20.0.5", "dstPort": 443, "protocol": "tcp"})  # new
make_finding(...)                                                                  # unchanged

How each of A-3's conditions is held

A-3 says	here
optional	subject=None default
a check that cannot fill it leaves it out	absent means no key — every existing finding keeps exactly its seven F-1 keys, never "subject": None
every key inside is optional	yes
no new vocabulary	a closed key set: src, dst, srcPort, dstPort, protocol
does not replace evidence.detail	untouched

The decision I would most like checked

The closed key set. A-3 shows an example but never enumerates keys. A free-form dict would let "dest" and "dst" both appear, and two findings about the same address would fail to join — silently. That is the exact failure A-3 was raised to remove from regex parsing, coming back through a typo. So unknown keys raise, and adding one should come with a consumer that needs it.

Values fail loudly, like every other field: address/CIDR for src/dst, integer 0–65535 for ports (a bool is refused), non-empty name for protocol, lower-cased so TCP and tcp join. An empty subject is refused.

Tests

tests/test_finding_subject.py, including a guard that fails if any check starts populating subject before its consumer exists, so the field is never half-used.

absent becomes subject=None      2 failed
key set opened                   1 failed
protocol not normalised          1 failed

1865 passed, 13 skipped with Batfish running.

Corrected after review. This line used to say test_analysis_cache.py's exact-seven-keys assertion was what held "absent is absent". It isn't: that assertion runs on hand-built dicts and never calls make_finding(), and it stays green with make_finding mutated to always add "subject": None (@ARSH871-bot measured 18/18). The property is held by test_a_finding_without_subject_is_unchanged here, and by test_cve_mapping.py:106. Recorded so nobody leans on the wrong test later.

[shubhamkataria2005]

@ARSH871-bot — this is the analysis/findings.py change I said on #300 would come to you on its own. It is small (an optional field, populated nowhere), but #238's chaining can't start until you have said yes or no to the closed key set (src, dst, srcPort, dstPort, protocol). If you would rather the set were different, I would rather know before building on it.

[ARSH871-bot]

Governance first. A-3 is ratified on main, four of four, each row ticked by its own signer: you (#269, 125936d), Samika (#287, c327fa0), Ankeet (#299, addddd6), me (#300, 542d864). So subject existing is not a repeat of A-2, and your commit message is right about that.

What A-3 did not ratify is the key set. SUBJECT_KEYS (analysis/findings.py:133) is field vocabulary: five names, each with a value format. A-3's text (docs/finding-format.md:316-327) shows one example with dst, dstPort, protocol; src and srcPort appear nowhere in it, and your description says A-3 "never enumerates keys". CLAUDE.md §7a puts the field vocabulary in findings.py under all four, and findings.py:4-5 says the doc is the agreement and the module the enforcement. Here the module would enforce something the doc doesn't say. Samika signed #269 on "doesn't add ... new vocabulary", and on #300 you described this PR as only the optional keyword.

You asked me for a yes or no on the set. I think closed is the right design, for exactly your reason. But it isn't mine alone to agree. Please:

1. Write the five keys and their value forms into A-3's section of docs/finding-format.md in this PR.
2. Ask Ankeet and Samika for a yes here, and record the four answers in the doc.

Code:

3. protocol is still open vocabulary (findings.py:164-167). subject={"dst":"10.20.0.5","protocol":"!tcp","dstPort":443} is accepted. !tcp is policy_compliance's own syntax (analysis/checks/policy_compliance.py:219), and that is A-3's named first producer. "6", "any" and "banana" pass too. Your argument for closing the keys applies one level down: tcp and 6 never join, and !tcp means the opposite. Close the value set.
4. Addresses aren't canonical (findings.py:154-159). "10.20.0.5" and "10.20.0.5/32" don't join; neither do "2001:DB8::1" and "2001:db8::1". "10.20.0.1/16" is accepted because of strict=False. You lower-case protocol so TCP and tcp join; addresses need the same: store a canonical form, or refuse non-canonical input.
5. 6 of 13 mutants survive. Nothing tests src or srcPort: deleting "src" from the address branch, or "srcPort" from the port branch, stays green. Accepting 65536 or -1 stays green, because the only port case is 70000. Deleting the dict check stays green, because a string fails on the unknown-key check anyway; a list like ["dst"] would separate them. Please add src/srcPort cases, 0 and 65535 accepted, -1 and 65536 refused, and an ipaddress object stored as a string.
6. The tripwire (tests/test_finding_subject.py:75-82). Path("analysis") is relative to the cwd. With a real subject= planted in a check, it fails from the repo root and passes from any other directory, because rglob on a missing path yields nothing. It also misses finding["subject"] = {...} (planted in access_control.py: still green) and anything in web/ or ai/. Please anchor it on Path(__file__).resolve().parents[1] and assert the file list is non-empty. Better still, walk the AST for a subject keyword or a ["subject"] assignment.
7. Description. The test_analysis_cache.py:362 assertion runs on hand-built dicts (:70-109) and never calls make_finding. With make_finding mutated to always add "subject": None it stays 18/18 green. The property is held by your test_a_finding_without_subject_is_unchanged and test_cve_mapping.py:106 (4 failures under that mutant). Please correct the line so nobody leans on the wrong test later.

For #238, not this PR, but please put these on its checklist before the first producer lands:

• ai/explain.py:671 puts the whole finding, subject included, into the prompt. _names_an_invented_fact (:754-775) checks only summary, device, detail and source. A model repeating a subject address gets rejected as inventing it, and the explanation falls back. That's the safe direction, but it's Ankeet's layer.
• Saved scans keep only _F1_FIELDS (web/main.py:1771, :1792), and tests/test_history_api.py:149 pins that, so subject will be silently dropped from history.
• make_finding accepts subject on none/error findings. A chain must never run through a "could not check" card.

Nits: findings.py:172 still says "eight fields". findings.py:157-158 should raise ... from None.

What I ran:

• Your branch: 1864 passed, 1 failed, 13 skipped. The failure is a live-Batfish test that passes alone.
• Merged onto main dc93ccb: 1901 passed, 1 failed (same flake), 13 skipped; +17 is your 15 plus 2 hygiene cases for the new file.
• Without subject, findings are byte-identical to main across 164 inputs, explicit None included.
• Merges cleanly with #387 on top of main.

Requesting changes for 1–6. The "absent is absent" property is solid.

[shubhamkataria2005]

@ARSH871-bot — all six items are addressed in f88e7ee. Your review is void under M-2 rule 1, so this needs a fresh look.

#		now
1	key set unwritten	written into A-3's section of docs/finding-format.md: every key, its value form, its stored form, and why it is closed at both levels
2	four answers	sign-off table added, one of four (mine). The note says this PR should not merge until it is full
3	protocol values open	closed: tcp/udp/icmp or 6/17/1, stored as the name. !tcp, any, banana and "" are refused
4	addresses not canonical	stored as the canonical network: 10.20.0.5 and 10.20.0.5/32 match, so do 2001:DB8::1 and 2001:db8::1. 10.20.0.1/16 is refused, not masked
5	6 of 13 mutants survive	all six killed, plus three more: addresses stored as given, host bits masked, protocol set reopened
6	tripwire blind from the wrong cwd	anchored on __file__, asserts it scanned more than 20 files, walks the AST of analysis/, web/ and ai/ for both subject= and ["subject"] =. A second test plants both forms to prove it can see them
7	wrong test credited	the description now carries a visible correction

Nits: "eight fields" → "nine", and raise … from None. Your three #238 items are on #238's checklist.

1929 passed, 13 skipped with Batfish running, on top of current main.

---

@patelankeet2 @SamikaPerera — this needs a yes or no from each of you on the key set. It's the table under "The key set — NOT covered by the signatures below" in A-3's section. Five keys (src, dst, srcPort, dstPort, protocol), each with one allowed value form. A-3's signatures covered that subject exists, not which keys it holds. Tick your own row on this branch, or say no here — a no now is free, and a no after the first producer lands is a migration.

[patelankeet2]

Yes from me on the key set, ticked in d01ca23.

What I checked before signing

Read SUBJECT_KEYS, SUBJECT_PROTOCOLS and _canonical_address() in analysis/findings.py directly against the table in docs/finding-format.md rather than taking the table's word for the code - they match exactly: the five keys, protocol closed to tcp/udp/icmp or 6/17/1 stored as the name, addresses stored as the canonical network with host-bits-set CIDRs refused rather than masked.

Ran tests/test_finding_subject.py on f88e7ee: 42 passed.

Mutation-tested three of the guards myself rather than only trusting the PR's own table:

removed strict=True from address canonicalisation      host-bits test FAILED (right reason)
removed address canonicalisation entirely               6 tests FAILED, including the two-spellings-join test
removed the bool guard on srcPort/dstPort                FAILED (right reason - bool is a subclass of int,
                                                          so True would otherwise be silently accepted as port 1)

Full suite on f88e7ee: 1929 passed, 13 skipped, matching the number you posted.

One non-blocking note

The equivalent bool guard on protocol (line checking isinstance(value, bool) before the map lookup) is defensive redundancy, not load-bearing - I removed it and all 42 tests still passed, because no string form of True/False collides with anything in SUBJECT_PROTOCOLS. Same shape as the destination digit-guard finding on #361. Worth keeping for what it says at the point of decision, not a gap.

Agreed the key set is the right design, for the reason already written into the doc: closing one level (keys) without closing the next (protocol values, address spelling) would have left the exact join-failure A-3 exists to remove.

[patelankeet2]

APPROVE on the code, as a fresh look since Arsh's CHANGES_REQUESTED is void under M-2 (two commits landed after it, including my own).

Went through all seven items from Shubham's "all six items are addressed in f88e7ee" table plus the nits, not just the key-set column I signed off on three days ago.

Verified today, independently

• Tripwire (item 6). Confirmed REPO = Path(__file__).resolve().parents[1] is genuinely cwd-independent, not just correct-looking - ran test_no_product_code_populates_subject_yet from a different working directory (docs/) and it still scanned the right files. Then mutation-tested it against real product code, not just the synthetic planted file the second test uses: added a real f["subject"] = {...} assignment to analysis/snapshot.py, confirmed the tripwire caught it by name and line number, reverted.
• Protocol set reopened (item 5's third extra mutant). Hadn't checked this specific one three days ago - replaced the whole SUBJECT_PROTOCOLS lookup with str(value) and a never-true guard. 10 tests failed correctly, confirming the closed set is genuinely load-bearing everywhere, not just against the bool case I'd already tested.
• Item 7 (wrong test credited) and the nits - confirmed present and correct: the PR body carries the "Corrected after review" note crediting the right test, findings.py:199 says "nine fields", and _canonical_address()'s raise carries from None.
• Items 1-4 and the rest of item 5 (address canonicalisation, the port bool guard) were already independently verified and mutation-tested in my sign-off comment on the 18th - re-ran tests/test_finding_subject.py fresh today, still 42 passed.

Full suite on d01ca23: 1929 passed, 13 skipped - matches the number posted.

Not blocking this review, but blocking merge

The key-set sign-off table is 2 of 4 (Shubham, and my own tick from the 18th). Per the note in docs/finding-format.md, this PR should not merge until Arsh and Samika have answered - that gate is real and separate from whether the code itself is correct, which is what this review is approving.