Skip to content

Commit 5b82e75

Browse files
Krishcalinclaude
andcommitted
"9 systems" while naming eight
Found by driving an eight-system estate — 3,624 findings over 73 pages — which is the first time anything here has run against more than one SID. The top-risks page rendered `{instances} systems`, and `instances` counts FINDINGS. Two ways those diverge, and only one is exotic: * one system with several clients: a check firing in 100 and 200 is two findings on one SID, and that is the ordinary case * two systems sharing a SID across landscapes, which is what this estate has So the row read "9 instances across 8 systems" and the screen said "9 systems" above a list of eight names, with a "+5" that made 3 + 5 = 8 sit beside a stated 9. Every number was right about something and none was right about systems. THE COUNT DID NOT EXIST TO BE DISPLAYED, which is why this took three changes rather than a relabel. `findings_for_domains` did not select `f.system_id`, so `_distinct_risks` had only the SID string to group on — a display label, not an identity. It now counts systems on the system, keeps `systems` as the list of names a reader recognises, and the page shows that count with the finding count beside it only when the two differ. The unnamed remainder counts off the authoritative total too, so the arithmetic reconciles. THE EXISTING TEST ASSERTED THE BUG. Its fixture was `instances: 6` with four SIDs, expecting "6 systems" — so this was not merely untested, it was pinned in place, and any correct implementation would have failed the suite. Replaced, and three cases added including the two-clients-one-system one. A green suite says a test agrees with the code, not that either is right. WHAT THE SAME DRIVE FOUND NOTHING WRONG WITH, worth recording because it is the first evidence at this scale: paging serves every one of 3,624 findings exactly once across 73 pages, with no duplicates — the paging fix was made at 8 pages and had never been seen an order of magnitude up. Dashboard, domains and findings totals all reconcile. The "one problem on many systems is one risk" grouping, which the whole page rests on, ran on real data for the first time and collapsed 50 risks correctly. A COVERAGE GAIN FALLS OUT OF IT. Suite skips drop from 3 to 1: the scope test that has been reporting "only one system in this database" now runs, as does the cross-system finding-detail check. They were never broken, only unreachable. AND ONE PRODUCT CHARACTERISTIC WORTH KNOWING, measured on the way: scaling the row counts of an estate 20x left the finding count identical at 405 and grew the member lists instead. Findings are driven by distinct PROBLEMS, not by estate size — 297 of 405 are aggregates. The route to a large finding count is more systems, not bigger files, which is why this estate is eight systems rather than one enormous one. 5,512 pass with a database; 157 frontend tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent cfc6a5e commit 5b82e75

4 files changed

Lines changed: 98 additions & 7 deletions

File tree

frontend/src/api/types.ts

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1431,10 +1431,17 @@ export interface TopRisk {
14311431
title: string
14321432
sid: string | null
14331433
system_client: string | null
1434-
/** How many systems this same check fires on, and which. Five rows of the
1435-
* same risk are not five risks. */
1434+
/** How many FINDINGS this same check produced. Five rows of the same risk
1435+
* are not five risks — but this is not a system count either: one system
1436+
* with two clients gives two findings on one SID. */
14361437
instances: number
1438+
/** The SIDs, for display. Two systems in different landscapes can share one,
1439+
* so the length of this is not a system count either. */
14371440
systems: string[]
1441+
/** How many systems, counted on the system rather than on its name. Optional:
1442+
* an older server does not send it and the caller falls back to
1443+
* `systems.length`. */
1444+
system_count?: number
14381445
}
14391446

14401447
export interface TopRisksView {

frontend/src/routes/TopRisks.test.tsx

Lines changed: 41 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -127,13 +127,52 @@ describe('the five worst in each domain', () => {
127127
// One problem on six systems is one risk — and the reader still has to
128128
// know it is six.
129129
topRisks.mockResolvedValue(view([
130-
domain({ shown: [risk({ instances: 6, systems: ['PRD', 'D01', 'T01', 'Q01'] })] }),
130+
domain({ shown: [risk({ instances: 6, system_count: 6,
131+
systems: ['PRD', 'D01', 'T01', 'Q01', 'S01', 'X01'] })] }),
131132
]))
132133
draw()
133134
expect(await screen.findByText(/6 systems: PRD, D01, T01/)).toBeInTheDocument()
134-
expect(screen.getByText(/\+1/)).toBeInTheDocument()
135+
expect(screen.getByText(/\+3/)).toBeInTheDocument()
135136
})
136137

138+
it('counts systems, not findings', async () => {
139+
// THIS TEST USED TO ASSERT THE BUG. Its fixture said instances: 6 with four
140+
// SIDs and expected "6 systems", because `instances` counts FINDINGS: one
141+
// system with two clients gives two findings on one SID, and two systems in
142+
// different landscapes can share a SID. Measured on an eight-system estate,
143+
// the screen read "9 systems" while naming eight.
144+
topRisks.mockResolvedValue(view([
145+
domain({ shown: [risk({ instances: 6, system_count: 4,
146+
systems: ['PRD', 'D01', 'T01', 'Q01'] })] }),
147+
]))
148+
draw()
149+
expect(await screen.findByText(/4 systems \(6 findings\)/)).toBeInTheDocument()
150+
expect(screen.queryByText(/6 systems/)).not.toBeInTheDocument()
151+
})
152+
153+
it('counts the unnamed remainder off the system total, not the name list',
154+
async () => {
155+
// Two systems sharing a SID contribute one name, so "+N" taken from the
156+
// name list left the reader adding 3 and 5 and getting 8 beside a stated 9.
157+
topRisks.mockResolvedValue(view([
158+
domain({ shown: [risk({ instances: 9, system_count: 9,
159+
systems: ['PRD', 'D01', 'T01', 'Q01', 'S01',
160+
'X01', 'Y01', 'Z01'] })] }),
161+
]))
162+
draw()
163+
await screen.findByText(/9 systems/)
164+
expect(screen.getByText(/\+6/)).toBeInTheDocument()
165+
})
166+
167+
it('falls back to the name list when an older server sends no count',
168+
async () => {
169+
topRisks.mockResolvedValue(view([
170+
domain({ shown: [risk({ instances: 2, systems: ['PRD', 'D01'] })] }),
171+
]))
172+
draw()
173+
expect(await screen.findByText(/2 systems: PRD, D01/)).toBeInTheDocument()
174+
})
175+
137176
it('says nothing of the sort when every domain was assessed', async () => {
138177
// A caveat that is always on is a caveat nobody reads.
139178
topRisks.mockResolvedValue(view([domain()]))

frontend/src/routes/TopRisks.tsx

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -64,10 +64,29 @@ function Row({ risk }: { risk: TopRiskDomain['shown'][number] }) {
6464
<span className="block text-[11px] text-ink3 font-mono truncate">
6565
{risk.check_id}
6666
{/* The systems it lands on, because one problem on six systems is
67-
one risk — and the reader still has to know it is six. */}
67+
one risk — and the reader still has to know it is six.
68+
69+
SIX SYSTEMS, NOT SIX FINDINGS. This said `{instances} systems`,
70+
and `instances` counts findings: a check firing in two clients of
71+
one system, or on two systems that share a SID across landscapes,
72+
made the label overstate. Measured on an eight-system estate it
73+
read "9 systems" while naming eight. `system_count` counts the
74+
systems; `instances` is shown beside it only when the two differ,
75+
because that difference is itself worth seeing. */}
6876
{risk.instances > 1
69-
? <> · {risk.instances} systems: {risk.systems.slice(0, 3).join(', ')}
70-
{risk.systems.length > 3 && ` +${risk.systems.length - 3}`}</>
77+
? <> · {risk.system_count ?? risk.systems.length} system
78+
{(risk.system_count ?? risk.systems.length) === 1 ? '' : 's'}
79+
{risk.instances !== (risk.system_count ?? risk.systems.length)
80+
&& ` (${risk.instances} findings)`}
81+
: {risk.systems.slice(0, 3).join(', ')}
82+
{/* "and N more" counted off the authoritative total, not off
83+
the name list. Two systems sharing a SID contribute one
84+
name, so "DEV, DV2, PR2 +5" beside "9 systems" left the
85+
reader adding 3 and 5 and getting 8. */}
86+
{(risk.system_count ?? risk.systems.length)
87+
> Math.min(3, risk.systems.length)
88+
&& ` +${(risk.system_count ?? risk.systems.length)
89+
- Math.min(3, risk.systems.length)}`}</>
7190
: risk.sid && <> · {risk.sid}{risk.system_client ? `/${risk.system_client}` : ''}</>}
7291
</span>
7392
</span>

server/queries.py

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -969,9 +969,26 @@ def _distinct_risks(rows: List[Dict[str, Any]]) -> List[Dict[str, Any]]:
969969
"""
970970
out: List[Dict[str, Any]] = []
971971
seen: Dict[str, Dict[str, Any]] = {}
972+
ids: Dict[str, set] = {}
972973
for row in rows:
973974
check = str(row.get("check_id") or "")
974975
system = row.get("sid")
976+
# HOW MANY SYSTEMS, COUNTED ON THE SYSTEM AND NOT ON ITS NAME.
977+
#
978+
# `systems` is the DISPLAY list and is keyed by SID, which is what a
979+
# reader recognises. It is not a count: two systems can share a SID —
980+
# the same SID recorded in two landscapes — and one system carries
981+
# several clients, so a check firing in client 100 and 200 is two
982+
# findings on one SID. Counting the display list therefore understates,
983+
# and counting `instances` (which counts FINDINGS) overstates.
984+
#
985+
# Found on an eight-system estate, where AUTH-001 read "9 instances
986+
# across 8 systems" and the screen rendered "9 systems" while naming
987+
# eight. Both numbers were right about something and neither was right
988+
# about systems.
989+
holder = ids.setdefault(check, set())
990+
if row.get("system_id") is not None:
991+
holder.add(row["system_id"])
975992
if check in seen:
976993
entry = seen[check]
977994
entry["instances"] += 1
@@ -983,6 +1000,11 @@ def _distinct_risks(rows: List[Dict[str, Any]]) -> List[Dict[str, Any]]:
9831000
entry["systems"] = [system] if system else []
9841001
seen[check] = entry
9851002
out.append(entry)
1003+
for entry in out:
1004+
# Falls back to the display list where the rows carry no system id, so
1005+
# a caller that does not select it is no worse off than before.
1006+
entry["system_count"] = (len(ids.get(entry["check_id"]) or ())
1007+
or len(entry["systems"]))
9861008
return out
9871009

9881010

@@ -1085,6 +1107,10 @@ def findings_for_domains(scope: Optional[Sequence[int]]) -> List[Dict[str, Any]]
10851107
# above it.
10861108
"SELECT f.id, f.check_id, f.severity, f.priority_tier, "
10871109
" f.priority_score, f.state, cd.category, cd.title, "
1110+
# The system ITSELF, not only the name it goes by. `_distinct_risks`
1111+
# counts systems on this: a SID is a display label that two systems can
1112+
# share across landscapes, and one system carries several clients.
1113+
" f.system_id, "
10881114
" s.sid, s.client AS system_client "
10891115
"FROM finding f "
10901116
"JOIN check_definition cd ON cd.check_id = f.check_id "

0 commit comments

Comments
 (0)