fix(report): address review findings on CSPT detection

Resolve the Greptile review on the CSPT PR:

- coverage: a bundled skill's sub-topic is only covered by a row from an
  agent that carried that skill, so an unrelated row (an open_redirect
  "meta refresh") can no longer mask an untested browser surface
- reporting: the CSPT discriminator now requires an endpoint or a
  code_location that survives _build_physical_locations (a repo-relative
  file with a start line), so a drive/absolute path SARIF drops no longer
  passes and leaves the alert locationless and collapsed
- sarif: drop the "client-side path traversal" title keyword so existing
  dynamic reports keep their title token and fingerprint; CSPT is still
  distinguished by the structured finding_class
- dedupe: exempt the default `dynamic` class so an older CSPT filed as
  dynamic can still dedupe against a client_side_path_traversal report
- tests: relabel the fixture regression as a source-to-sink primitive
  (reaches_sink / no_unguarded_sink), not an exploitability verdict;
  add a coverage regression for the sub-topic masking fix

strix/report/coverage.py

@@ -355,20 +355,35 @@ def skill_coverage_gaps(
         return []
 
     carriers: dict[str, list[str]] = {}
+    carrier_ids: dict[str, set[str]] = {}
     for agent in agents:
+        agent_id = str(agent.get("agent_id") or "")
         for skill in agent["skills"]:
             leaf = _skill_leaf(skill)
             if leaf in risk_skills:
                 carriers.setdefault(leaf, []).append(str(agent["agent_name"]))
+                if agent_id:
+                    carrier_ids.setdefault(leaf, set()).add(agent_id)
 
     gaps: list[dict[str, Any]] = []
     for skill, agent_names in sorted(carriers.items()):
         names = ", ".join(sorted(set(agent_names)))
         topics = _topics_for(skill)
+        # A bundled skill's surfaces are only covered by rows from an agent that
+        # actually carried that skill. Otherwise an unrelated row that merely
+        # shares a word — an open_redirect row mentioning "meta refresh" — would
+        # mask an untested browser surface. Single-topic skills keep the shared
+        # ledger semantics: any row naming the class counts, whoever wrote it.
+        vclass = VULN_CLASSES.get(skill)
+        if vclass is not None and vclass.sub_topics:
+            owner_ids = carrier_ids.get(skill, set())
+            relevant = [e for e in entries if str(e.get("agent_id") or "") in owner_ids]
+        else:
+            relevant = entries
         uncovered = [
             (label, wordlists)
             for label, wordlists in topics
-            if not any(_entry_is_about(entry, wordlists) for entry in entries)
+            if not any(_entry_is_about(entry, wordlists) for entry in relevant)
         ]
         if not uncovered:
             continue

strix/report/dedupe.py

@@ -94,9 +94,13 @@ CRITICAL DEDUPLICATION RULES:
    - Same CVE but different package/ecosystem is NOT a duplicate
    - Same package/ecosystem but different CVE is NOT a duplicate
 
-5. Different finding_class is NOT a duplicate even under the same CWE:
-   - client_side_path_traversal vs a server-side/dynamic path-traversal finding
-     both map to CWE-22, but they are distinct classes fixed by different code
+5. finding_class separates classes only when both are specific:
+   - two reports with different SPECIFIC (non-default) finding_class values are
+     distinct classes even under one shared CWE - fixed by different code, NOT duplicates
+   - but `dynamic` is the default catch-all, not a class: a `dynamic` report and a
+     `client_side_path_traversal` report can still be the SAME bug (e.g. an older CSPT
+     filed before the class existed). Do not treat dynamic-vs-specific as proof of
+     distinctness - compare the underlying root cause
 
 COMPARISON GUIDELINES:
 - Focus on the technical root cause, not surface-level similarities
@@ -107,8 +111,9 @@ COMPARISON GUIDELINES:
 FIELDS TO ANALYZE:
 - title, description: General vulnerability info
 - target, endpoint, method: Exact location of vulnerability
-- finding_class: Machine-readable class; a different finding_class means a
-  different class of bug even under a shared CWE - not a duplicate
+- finding_class: Machine-readable class. Two different SPECIFIC classes under one
+  CWE are different bugs (not duplicates); but `dynamic` is the default, not a
+  class, so a `dynamic` report may still duplicate a specific one - compare the bug
 - technical_analysis: Root cause details
 - poc_description: How it's exploited
 - impact: What damage it can cause

strix/report/sarif.py

@@ -920,7 +920,6 @@ _VULN_CLASS_KEYWORDS = (
     "default password",
     "session fixation",
     "open redirect",
-    "client-side path traversal",
     "path traversal",
     "directory traversal",
     "command injection",

strix/tools/reporting/tool.py

@@ -17,6 +17,7 @@ from typing import TYPE_CHECKING, Any
 from agents import RunContextWrapper, function_tool
 
 from strix.report.coverage import selectable_finding_classes
+from strix.report.sarif import _build_physical_locations
 from strix.tools.nullish import clean_optional
 from strix.tools.proxy.tools import existing_request_ids
 
@@ -858,18 +859,24 @@ async def _do_create(
             f"Invalid finding_class: {finding_class!r}. "
             f"Must be one of: {sorted(_VALID_FINDING_CLASSES)}"
         )
-    elif finding_class == "client_side_path_traversal" and not (
-        str(endpoint or "").strip() or parsed_locations
-    ):
+    elif finding_class == "client_side_path_traversal":
         # CSPT findings are typically locationless (the bug is in a JS bundle,
-        # not a server route), so without a discriminator multiple distinct
-        # CSPT findings collapse onto one synthetic SARIF fingerprint.
-        errors.append(
-            "finding_class 'client_side_path_traversal' needs a discriminator so "
-            "locationless CSPT findings do not collapse onto one SARIF fingerprint: "
-            "set endpoint (the traversed target path, e.g. '/admin/keys') and method, "
-            "or a code_location for the vulnerable client-side sink."
-        )
+        # not a server route), so without a discriminator multiple distinct CSPT
+        # findings collapse onto one synthetic SARIF fingerprint. The anchor has
+        # to be one SARIF can actually use: an endpoint (→ route), or a
+        # code_location that survives _build_physical_locations (a repo-relative
+        # file with a real start line — an absolute or drive path is dropped and
+        # would leave the finding synthetic and collapsed).
+        has_endpoint = bool(str(endpoint or "").strip())
+        physical_locations, _dropped = _build_physical_locations(parsed_locations)
+        if not (has_endpoint or physical_locations):
+            errors.append(
+                "finding_class 'client_side_path_traversal' needs a discriminator SARIF "
+                "can anchor, or distinct CSPT findings collapse onto one fingerprint: set "
+                "endpoint (the traversed target path, e.g. '/admin/keys') and method, or a "
+                "code_location with a repo-relative file and start line (not an absolute or "
+                "drive path, which SARIF drops)."
+            )
 
     if errors:
         return {"success": False, "error": "Validation failed", "errors": errors}

tests/fixtures/cspt/README.md

@@ -5,10 +5,13 @@ flows, used by `tests/test_cspt_fixtures.py` as a deterministic, browser-free
 regression for what CSPT looks like in client code. Unlike a Playwright
 capture test, these run in ordinary CI with no Chromium.
 
-Naming: `positive_*` is an exploitable flow — an attacker-controlled client
+Naming: `positive_*` has the CSPT **primitive** — an attacker-controlled client
 source (URL query, fragment, referrer, postMessage, storage) reaches a request
-sink's path unguarded. `negative_*` is a safe control — the flow is validated,
-or the user value never reaches the request path.
+sink's path unguarded. That is the source-to-sink reachability only; whether the
+redirected request is *exploitable* (a state change, a rendered response, an
+internal fetch, an authz bypass) is a separate step, per the exploitability bar
+in the `client_side_path_traversal` skill. `negative_*` is a control — the flow
+is validated, or the user value never reaches the request path.
 
 Convention the test relies on: the attacker-controlled value that reaches a
 sink is named `userPath`, so a sink-call line containing `userPath` marks a

tests/fixtures/cspt/positive_hash_route.html

@@ -1,6 +1,7 @@
 <!DOCTYPE html>
 <!--
-  CSPT fixture - Expected classification: positive (exploitable).
+  CSPT fixture - positive: the unguarded source-to-sink primitive is present
+  (impact is proven separately, per the client_side_path_traversal skill).
   Source: URL fragment (location.hash, a client hash router). Sink: fetch() path.
   The fragment is never sent to the server by the browser, so only a
   client-side trace shows the traversed request leaving the page.

tests/fixtures/cspt/positive_query_param.html

@@ -1,6 +1,7 @@
 <!DOCTYPE html>
 <!--
-  CSPT fixture - Expected classification: positive (exploitable).
+  CSPT fixture - positive: the unguarded source-to-sink primitive is present
+  (impact is proven separately, per the client_side_path_traversal skill).
   Source: URL query parameter (location.search). Sink: fetch() request path.
   The attacker-controlled value flows unmodified into the fetch path, so a
   value like ../../admin/keys redirects the victim's authenticated request.

tests/test_cspt_fixtures.py

@@ -1,11 +1,15 @@
-"""Deterministic, browser-free source-to-sink regression over CSPT fixtures.
+"""Deterministic, browser-free regression over the CSPT fixtures.
 
-PR #789 proves CSPT detection with a Playwright capture test that skips in any
-CI without Chromium. This instead pins the same source-to-sink intent with a
-tiny matcher over hand-authored fixtures, so it runs everywhere. It is a
-fixture/pattern regression — not a claim that Strix ships a static taint
-tracker — kept honest by a convention: the attacker-controlled value that
-reaches a sink is named ``userPath`` in every fixture.
+This checks one thing only: whether an attacker-controlled client source reaches
+a request sink's path **unguarded** — the CSPT *primitive*. It deliberately does
+NOT decide exploitability: a reached sink is not yet a finding. Whether the
+redirected request has security impact (a state change, a rendered response, an
+internal fetch, an authz bypass) is proven separately, per the exploitability bar
+in the `client_side_path_traversal` skill. So a passing test here means "the
+primitive is present/absent as labelled", not "exploitable".
+
+It is a fixture/pattern regression, not a static taint tracker, kept honest by a
+convention: the attacker value that reaches a sink is named ``userPath``.
 """
 
 from __future__ import annotations
@@ -24,40 +28,46 @@ _SOURCES = (
     "postMessage",
     "localStorage",
 )
-#: Request sinks whose path, if attacker-shaped, makes the flow a CSPT.
+#: Request sinks whose path, if attacker-shaped, carries the traversal.
 _SINKS = ("fetch(", "XMLHttpRequest", "axios", "EventSource", "WebSocket")
-#: Markers of a guard that neutralises the flow (so it is a safe control).
+#: Markers of a guard that neutralises the flow before the sink.
 _MITIGATIONS = ("/^", ".test(", "allowlist", "hardcoded")
 
 
-def _taints_sink(text: str) -> bool:
-    """True when the attacker value (``userPath``) appears on a sink-call line."""
+def _reaches_sink_unguarded(text: str) -> bool:
+    """True when the attacker value (``userPath``) reaches a sink-call line."""
     return any(
         any(sink in line for sink in _SINKS) and "userPath" in line
         for line in text.splitlines()
     )
 
 
-def _classify(text: str) -> str:
-    """Exploitable only when an unguarded attacker source reaches a sink path."""
+def _detect_primitive(text: str) -> str:
+    """"reaches_sink" when an unguarded attacker source reaches a sink path.
+
+    This is the CSPT primitive, not an exploitability verdict — impact is a
+    separate, out-of-band step the skill describes.
+    """
     has_source = any(token in text for token in _SOURCES)
     has_sink = any(token in text for token in _SINKS)
     guarded = any(token in text for token in _MITIGATIONS)
-    if has_source and has_sink and _taints_sink(text) and not guarded:
-        return "positive"
-    return "negative"
+    if has_source and has_sink and _reaches_sink_unguarded(text) and not guarded:
+        return "reaches_sink"
+    return "no_unguarded_sink"
 
 
-def test_cspt_fixtures_classify_by_their_label() -> None:
+def test_cspt_fixtures_match_their_labelled_primitive() -> None:
     fixtures = sorted(_FIXTURES.glob("*.html"))
     assert fixtures, "no CSPT fixtures found"
 
     positives = [f for f in fixtures if f.name.startswith("positive_")]
     negatives = [f for f in fixtures if f.name.startswith("negative_")]
-    assert len(positives) >= 2, "need at least two positive CSPT fixtures"
-    assert len(negatives) >= 2, "need at least two negative control fixtures"
+    assert len(positives) >= 2, "need at least two primitive-present fixtures"
+    assert len(negatives) >= 2, "need at least two guarded / no-flow controls"
 
     for fixture in fixtures:
-        expected = "positive" if fixture.name.startswith("positive_") else "negative"
-        actual = _classify(fixture.read_text(encoding="utf-8"))
+        # positive_* = the unguarded source-to-sink primitive is present
+        # (exploitability is proven separately); negative_* = guarded or no flow.
+        expected = "reaches_sink" if fixture.name.startswith("positive_") else "no_unguarded_sink"
+        actual = _detect_primitive(fixture.read_text(encoding="utf-8"))
         assert actual == expected, f"{fixture.name}: expected {expected}, matched {actual}"

tests/test_report_coverage.py

@@ -170,6 +170,39 @@ def test_bundled_skill_with_every_surface_covered_is_clean() -> None:
     ]
 
 
+def test_unrelated_agents_row_does_not_cover_a_browser_subtopic() -> None:
+    """An open_redirect agent's 'meta refresh' row must not mark browser_security's
+    navigation surface covered — only a browser_security carrier's row counts."""
+    doc = _document(
+        entries=[
+            _entry(
+                risk_area="postMessage origin validation",
+                surface="listener",
+                agent_id="agent-1",
+                agent_name="browser-tester",
+            ),
+            _entry(
+                risk_area="open redirect via meta refresh",
+                surface="/login?next=",
+                agent_id="agent-2",
+                agent_name="redirect-tester",
+            ),
+        ],
+        agent_graph=_graph(
+            statuses={"agent-1": "completed", "agent-2": "completed"},
+            names={"agent-1": "browser-tester", "agent-2": "redirect-tester"},
+            metadata={
+                "agent-1": {"skills": ["browser_security"]},
+                "agent-2": {"skills": ["open_redirect"]},
+            },
+        ),
+    )
+
+    sub = {g["risk_area"] for g in doc["gaps"] if g["kind"] == "unrecorded_sub_topic"}
+    assert "navigation and redirect control" in sub  # not masked by the open_redirect row
+    assert "postMessage" not in sub  # the browser agent's own row still counts
+
+
 def test_registry_backs_skill_phrasings_and_cwe() -> None:
     """_SKILL_PHRASINGS is derived from the registry, so they cannot drift."""
     assert _SKILL_PHRASINGS == {name: v.aliases for name, v in VULN_CLASSES.items()}