fix(audit): stop under-reporting suppressed_count as a maximum

README.md

@@ -619,7 +619,7 @@ consult the guard — a `guard` object:
 | `verdict` | `served`, `served_filtered`, `denied` or `refused`; the worst verdict of the call wins |
 | `rule` | What decided a per-bug assessment. Alongside the policy's own rule names the guard reports its own: `default` when no rule matched — a default-decided call records that literal, never an absent field — `min_bug_age_days` for the age quarantine, `<name>:unreadable-metadata` for a *granting* rule whose verdict hinged on metadata that could not be read (an undecidable deny rule keeps its plain name, having denied for its own reason), `unavailable` for a bug the classification fetch could not reach. Nothing stops you naming one of your own rules `default`, so this field says what decided, not what kind of thing it was. Absent only where no single rule decided: a refusal, the pre-dispatch gate, a search, either arm of the create gate, an id the guard could not assess, and a withheld attachment. A tool removed from the router (read-only mode, `disabled_tools`, discovery off) records no `guard` object at all |
 | `policy_hash` | `sha256:` over the raw policy file bytes, so a record says which policy text judged the call. Absent when no policy file is loaded |
-| `suppressed_count` | How much the response withheld: bug ids on a search or a multi-bug read, but also private comments and attachment metadata the private-content gate removed. It is the larger of the two tallies, not their sum, and it is the authoritative number — never infer a count from the id list |
+| `suppressed_count` | How much the response withheld, in total: bug ids on a search or a multi-bug read, plus the private comments and attachment metadata the private-content gate removed. The two never overlap — a call reads its bug ids off the content that survived filtering — so the field is their sum, and it is the authoritative number: never infer a count from the id list, which names bugs only and can be switched off entirely. Two ids under a count of five means three withheld items had no bug id of their own |
 | `suppressed_ids` | The withheld bug ids, subject to the `suppressed_ids` switch |
 | `redacted_fields` | Names of what a response redacted, never values — `summary_view` for a bug served through the redacted summary view |
 | `scan` | Present on every served search, carrying how many rows the window scanned and how many it dropped, so `dropped: 0` is a statement and not an omission; absent on a search that failed and on tools that scan nothing. The counts are recorded whatever the `suppressed_ids` switch says: counts are not ids |

crates/bugwarden/src/audit.rs

@@ -75,6 +75,14 @@ use serde::{Deserialize, Serialize};
 use sha2::{Digest as _, Sha256};
 
 /// The audit record schema version stamped into every event (`v`).
+///
+/// It tracks the STRUCTURE a reader must handle — fields, their names,
+/// their types — and not the meaning of what those fields hold. A field
+/// whose definition is corrected without changing the wire shape keeps
+/// `v`, so a corpus at one version can span both readings and the
+/// deployed build, not this number, is the boundary between them (see
+/// `guard.suppressed_count` in DESIGN.md, issue #68). Re-encoding the
+/// stream so that meaning changes ARE visible is #34's business.
 pub const SCHEMA_VERSION: u32 = 1;
 
 /// How often a persistently failing sink repeats its `tracing` diagnostic.
@@ -522,10 +530,26 @@ pub struct GuardInfo {
     /// the exact policy that produced it.
     #[serde(skip_serializing_if = "Option::is_none")]
     pub policy_hash: Option<String>,
-    /// How many bugs the guard removed from the response. This count is
-    /// authoritative: [`GuardInfo::suppressed_ids`] may be elided
-    /// (empty) or a subset by configuration, so consumers must never
-    /// infer the count from the id list.
+    /// How much the guard withheld from the response, in total: the
+    /// withheld bug ids PLUS the suppressed content that carries no bug
+    /// id of its own — private comments and private attachment metadata
+    /// the I5 gate removed. Not a count of bugs, then, and not a count of
+    /// either tally alone.
+    ///
+    /// The two populations are disjoint by construction, so the sum can
+    /// never double-count. The two tools that feed BOTH tallies —
+    /// `bug_comments` and `summarize_bug` — derive their duplicate-marker
+    /// ids from the comments that SURVIVED the private filter, so a
+    /// dropped comment contributes its id to nothing even when it carries
+    /// one; `list_attachments`, the third id-less site, names no ids at
+    /// all.
+    ///
+    /// This count is authoritative — it is the only tally that always
+    /// ships. [`GuardInfo::suppressed_ids`] carries bug ids alone, and
+    /// may be elided entirely by [`AuditConfig::suppressed_ids`], so
+    /// consumers must never infer the count from the id list: two ids
+    /// under a count of five says three withheld items had no bug id, not
+    /// that the record contradicts itself.
     pub suppressed_count: u64,
     /// The removed bug ids. Left empty by the recording call sites when
     /// [`AuditConfig::suppressed_ids`] is `false`.
@@ -1219,6 +1243,11 @@ impl AuditCell {
     /// Note `n` suppressions that carry no bug id (private comments or
     /// attachment metadata filtered out). Adds across notes; upgrades the
     /// verdict exactly as [`AuditCell::note_suppressed`] does.
+    ///
+    /// This tally and [`AuditCell::note_suppressed`]'s id set must stay
+    /// disjoint populations: [`GuardInfo::suppressed_count`] SUMS them,
+    /// so a call site that counted an item here and also named it there
+    /// would count it twice.
     pub fn note_suppressed_count(&self, n: u64) {
         if n == 0 {
             return;
@@ -1265,16 +1294,18 @@ impl AuditCell {
     /// verdict was ever noted — the tool performed no guard assessment.
     /// When `suppressed_ids_cfg` is `false` the ids are dropped and only
     /// the count ships ([`AuditConfig::suppressed_ids`]). The count is
-    /// the larger of the id set and the id-less counter, so it can only
-    /// ever under-name, never under-count.
+    /// the id set PLUS the id-less counter (issue #68): the two are
+    /// disjoint populations, so their sum is the total withheld and
+    /// nothing is counted twice.
     pub fn into_guard_info(
         &self,
         policy_hash: Option<&str>,
         suppressed_ids_cfg: bool,
     ) -> Option<GuardInfo> {
         let state = std::mem::take(&mut *self.lock());
         let verdict = state.verdict?;
-        let suppressed_count = (state.suppressed_ids.len() as u64).max(state.suppressed_extra);
+        let suppressed_count =
+            (state.suppressed_ids.len() as u64).saturating_add(state.suppressed_extra);
         Some(GuardInfo {
             verdict,
             rule: state.rule,
@@ -2264,23 +2295,33 @@ mod tests {
     }
 
     #[test]
-    fn cell_suppressed_count_is_max_of_ids_and_counter() {
+    fn cell_suppressed_count_sums_ids_and_counter() {
+        // Issue #68: the two tallies are disjoint populations, so the
+        // count is their TOTAL — not the larger of them, which would
+        // silently under-report whenever both are non-empty.
         let cell = AuditCell::default();
         cell.note_suppressed([7]);
         cell.note_suppressed_count(2);
         cell.note_suppressed_count(1);
         let info = cell.into_guard_info(None, true).unwrap();
-        assert_eq!(info.suppressed_count, 3, "id-less counter adds up");
+        assert_eq!(
+            info.suppressed_count, 4,
+            "one named id plus three id-less suppressions"
+        );
         assert_eq!(info.suppressed_ids, vec![7]);
     }
 
     #[test]
     fn cell_suppressed_ids_config_ships_the_count_only() {
         let cell = AuditCell::default();
         cell.note_suppressed([40, 41]);
+        cell.note_suppressed_count(3);
         let info = cell.into_guard_info(None, false).unwrap();
         assert!(info.suppressed_ids.is_empty(), "ids elided by config");
-        assert_eq!(info.suppressed_count, 2, "the count still ships");
+        assert_eq!(
+            info.suppressed_count, 5,
+            "the elided ids still count, and the id-less ones add to them"
+        );
     }
 
     #[test]

crates/bugwarden/src/server.rs

@@ -1878,6 +1878,10 @@ impl BugWarden {
                 // Bugzilla writes "*** Bug N has been marked as a duplicate
                 // of this bug ***" itself, so a hidden bug can name itself in
                 // the comments of one the client may read (I2).
+                // LOAD-BEARING: harvest from the FILTERED list, never the raw
+                // one. A dropped private comment is already counted id-less
+                // below; naming its marker id too would put one comment in
+                // both tallies, and guard.suppressed_count sums them (#68).
                 let named = Guard::duplicate_marker_ids(&filtered);
                 let disclosable = self
                     .guard
@@ -3108,6 +3112,10 @@ impl BugWarden {
         let comments = self.guard.filter_comments(comments, false);
         // Same scrub as bug_comments: otherwise a client that would be
         // scrubbed there just asks for a summary instead (I14).
+        // LOAD-BEARING, as in bug_comments: `comments` is already the
+        // FILTERED list here. Harvesting marker ids from the pre-filter one
+        // would count a dropped private comment in both tallies, and
+        // guard.suppressed_count sums them (#68).
         let named = Guard::duplicate_marker_ids(&comments);
         let disclosable = self
             .guard

crates/bugwarden/tests/audit_wiremock.rs

@@ -678,6 +678,192 @@ async fn suppressed_ids_off_records_the_count_never_the_ids() {
     );
 }
 
+/// Bug 7's comments mix BOTH suppression populations in one call (issue
+/// #68): three private comments the I5 gate drops, which carry no bug id
+/// of their own, and two duplicate markers naming the
+/// [`HIDE_SECRET_POLICY`]-hidden bugs 666 and 667, which do.
+///
+/// One of the three private comments is ITSELF a duplicate marker, naming
+/// a third hidden bug 668. That comment is what makes the fixture defend
+/// the disjointness the sum rests on rather than merely assume it: the
+/// marker ids are harvested from the comments that SURVIVED the private
+/// filter, so 668 is counted once as id-less content and never named —
+/// total 5. Harvest the ids from the pre-filter list instead and the same
+/// dropped comment is counted twice, once in each tally: 6, over a
+/// three-id list. Both classify fetches are mounted so that mutation
+/// fails on the NUMBER rather than on an unmatched mock.
+async fn mount_mixed_suppression(mock: &MockServer) {
+    Mock::given(method("GET"))
+        .and(path("/rest/bug"))
+        .and(query_param("id", "7"))
+        .respond_with(ResponseTemplate::new(200).set_body_json(json!({ "bugs": [world_bug(7)] })))
+        .mount(mock)
+        .await;
+    let secret = |id: u64| {
+        let mut bug = world_bug(id);
+        bug["product"] = json!("SecretSauce");
+        bug
+    };
+    // The link-disclosure fetch asks for the whole candidate set at once.
+    // Expected exactly once: without the expectation an id list the guard
+    // never asks for still 404s into disclosable's fail-closed arm, which
+    // withholds 666/667 anyway and leaves the fixture decorative.
+    Mock::given(method("GET"))
+        .and(path("/rest/bug"))
+        .and(query_param("id", "666,667"))
+        .respond_with(
+            ResponseTemplate::new(200).set_body_json(json!({ "bugs": [secret(666), secret(667)] })),
+        )
+        .expect(1)
+        .mount(mock)
+        .await;
+    // Never requested while the harvest stays post-filter — mounted so a
+    // pre-filter harvest gets a truthful answer and is caught by the
+    // count, not by a 404.
+    Mock::given(method("GET"))
+        .and(path("/rest/bug"))
+        .and(query_param("id", "666,667,668"))
+        .respond_with(
+            ResponseTemplate::new(200)
+                .set_body_json(json!({ "bugs": [secret(666), secret(667), secret(668)] })),
+        )
+        .mount(mock)
+        .await;
+    Mock::given(method("GET"))
+        .and(path("/rest/bug/7/comment"))
+        .respond_with(ResponseTemplate::new(200).set_body_json(json!({
+            "bugs": { "7": { "comments": [
+                { "id": 1, "bug_id": 7, "is_private": false, "text": "public comment" },
+                { "id": 2, "bug_id": 7, "is_private": true, "text": "canary-private-8a1c one" },
+                { "id": 3, "bug_id": 7, "is_private": true, "text": "canary-private-8a1c two" },
+                // Private AND a duplicate marker: id-less content by the
+                // only reading that keeps the two tallies disjoint.
+                { "id": 4, "bug_id": 7, "is_private": true,
+                  "text": "canary-private-8a1c *** Bug 668 has been marked as a duplicate of this bug ***" },
+                { "id": 5, "bug_id": 7, "is_private": false,
+                  "text": "*** Bug 666 has been marked as a duplicate of this bug ***" },
+                { "id": 6, "bug_id": 7, "is_private": false,
+                  "text": "*** Bug 667 has been marked as a duplicate of this bug ***" },
+            ] } }
+        })))
+        .mount(mock)
+        .await;
+}
+
+#[tokio::test]
+async fn suppressed_count_totals_both_populations_in_one_call() {
+    // Issue #68: one call suppresses three id-less private comments AND
+    // two duplicate-marker bug ids. The count is the TOTAL — five — not
+    // the larger tally, which would report three beside a two-id list and
+    // silently lose two withheld items.
+    let mock = MockServer::start().await;
+    mount_mixed_suppression(&mock).await;
+    let audited = audited_client_for(HIDE_SECRET_POLICY, &mock, "test-key").await;
+
+    let result = call(&audited.client, "bug_comments", json!({ "id": 7 })).await;
+    assert_ne!(result.is_error, Some(true), "bug 7's comments are served");
+    let envelope = serde_json::to_string(&result).unwrap();
+    for id in ["666", "667"] {
+        assert!(
+            !envelope.contains(id),
+            "the hidden id must be scrubbed from the served comments (I14): {envelope}"
+        );
+    }
+    assert!(
+        !envelope.contains("canary-private-8a1c"),
+        "the private comments must not reach the client (I5): {envelope}"
+    );
+
+    let events = read_events(&audited.audit_path);
+    let tc = last_tool_call(&events);
+    assert_eq!(tc.request.tool, "bug_comments");
+    let guard = tc.guard.as_ref().expect("guard info recorded");
+    assert_eq!(guard.verdict, Verdict::ServedFiltered);
+    assert_eq!(
+        guard.suppressed_ids,
+        vec![666, 667],
+        "the surviving comments' marker ids are named, and 668's — noted \
+         only inside a DROPPED private comment — is not: naming it would \
+         count that one comment in both tallies"
+    );
+    assert_eq!(
+        guard.suppressed_count, 5,
+        "three id-less comments plus two withheld ids: {guard:?}"
+    );
+    assert!(
+        guard.suppressed_ids.len() < guard.suppressed_count as usize,
+        "the count is authoritative and outruns the id list it may not be \
+         inferred from: {guard:?}"
+    );
+}
+
+#[tokio::test]
+async fn summarize_bug_records_the_same_total_as_bug_comments() {
+    // summarize_bug filters and scrubs the same comments as bug_comments
+    // and notes both tallies from its own call site (server.rs), so this
+    // diff changes the number it records too — it needs its own record,
+    // not bug_comments' by analogy.
+    let mock = MockServer::start().await;
+    mount_mixed_suppression(&mock).await;
+    let audited = audited_client_for(HIDE_SECRET_POLICY, &mock, "test-key").await;
+
+    let result = call(&audited.client, "summarize_bug", json!({ "id": 7 })).await;
+    assert_ne!(result.is_error, Some(true), "the summary prompt is served");
+    let envelope = serde_json::to_string(&result).unwrap();
+    for id in ["666", "667", "668"] {
+        assert!(
+            !envelope.contains(id),
+            "no hidden id may reach the summarization prompt (I14): {envelope}"
+        );
+    }
+    assert!(
+        !envelope.contains("canary-private-8a1c"),
+        "no private comment may reach the summarization prompt (I5): {envelope}"
+    );
+
+    let events = read_events(&audited.audit_path);
+    let tc = last_tool_call(&events);
+    assert_eq!(tc.request.tool, "summarize_bug");
+    let guard = tc.guard.as_ref().expect("guard info recorded");
+    assert_eq!(guard.verdict, Verdict::ServedFiltered);
+    assert_eq!(
+        guard.suppressed_ids,
+        vec![666, 667],
+        "the same two ids bug_comments names, and 668 no more than it does"
+    );
+    assert_eq!(
+        guard.suppressed_count, 5,
+        "three id-less comments plus two withheld ids: {guard:?}"
+    );
+}
+
+#[tokio::test]
+async fn suppressed_count_totals_both_populations_with_the_ids_elided() {
+    // The same call under `suppressed_ids = false`, which is the case the
+    // issue calls out: with no id list at all, the count is the operator's
+    // ONLY signal that anything was withheld (I3 keeps it from the
+    // client), so it must still total both populations.
+    let mock = MockServer::start().await;
+    mount_mixed_suppression(&mock).await;
+    let audited = audited_client_with(HIDE_SECRET_POLICY, &mock, "test-key", false).await;
+
+    let result = call(&audited.client, "bug_comments", json!({ "id": 7 })).await;
+    assert_ne!(result.is_error, Some(true), "bug 7's comments are served");
+
+    let events = read_events(&audited.audit_path);
+    let tc = last_tool_call(&events);
+    let guard = tc.guard.as_ref().expect("guard info recorded");
+    assert!(
+        guard.suppressed_ids.is_empty(),
+        "ids are elided when the knob is off: {:?}",
+        guard.suppressed_ids
+    );
+    assert_eq!(
+        guard.suppressed_count, 5,
+        "the elided ids still count, and the id-less comments add to them: {guard:?}"
+    );
+}
+
 /// Serve a quicksearch corpus (issue #29 tests): search requests are paged
 /// by limit/offset the way Bugzilla pages them, and the I14 link-disclosure
 /// classify fetch — which addresses bugs by id, not by query — gets an