Present consumes adjudications — the frontend stops judging (#521, #499) - #530
Merged
Merged
Conversation
The frontend guesses a host's role from port numbers — "< 1024 means server" — and #496 is the bill. This slice records the truth instead. THE FINDING: direction was destroyed at parse time. createConversationKey normalises A<->B into one bucket by IP string comparison, so a conversation's srcIp is "whichever endpoint sorted first", not "who started it". The signal never reached the database at all, which is *why* the frontend resorted to guessing — there was nothing else to go on. Extract now asks tshark for tcp.flags.syn/ack and records the endpoint that sent SYN without ACK. Appended to the field list, not inserted: every field is read by positional index, so a field in the middle would silently shift them all. Grade: MEASURED. The traffic exhibited it — nobody asserted it, no tool judged it. That is precisely why it can replace a heuristic. Null means UNKNOWN, never "nobody initiated": UDP/ICMP/ARP have no handshake, and a capture can begin mid-flow. It must NOT be backfilled from ports — that is the bug, not the fallback. Deliberately NOT computing a "role" column. Role is a per-HOST conclusion (a host can initiate one flow and serve another); per-conversation we record the FACT and let Scan/Adjudicate judge over many flows. Extract records, Scan judges. Verified against tshark ground truth on discord.pcap: TracePcap : initiator_ip=10.0.2.15 initiator_port=42834 tshark : syn==1 && ack==0 -> 10.0.2.15:42834 (exact match) Null semantics verified per protocol on the same file: TCP 1 conv, 1 initiator <- handshake seen UDP 33 convs, 0 initiators <- no handshake exists RTCP 12 convs, 0 initiators TLSV1 1 conv, 0 initiators <- capture joined mid-flow, SYN missed The old rule would have called those TLS flows "server" because 443 < 1024. And the bug itself, on demo_all_rules.pcap: 192.168.100.10:55000 -> 203.0.113.10:4434 port rule says the responder is a CLIENT; it answered the SYN. 1 of 6 responders mislabelled by the port rule in one small capture. 153 tests, 152 pass (PipelineIntegrationTest needs a Docker socket locally). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0165VsVLYm3G8DnE5N9vXToS
The fact is useless to #521 if the frontend cannot see it. Both conversation DTOs — list and detail — now carry initiatorIp/initiatorPort, and the OpenAPI baseline is regenerated (CI fails without it). Verified the field survives the whole path, not just that it compiles: DB initiator_ip=10.0.2.15 initiator_port=42834 API GET /conversations/{id} -> "initiator=10.0.2.15:42834" tshark syn==1 && ack==0 -> 10.0.2.15:42834 1 of 48 conversations has one — the file's only TCP flow. The other 47 (UDP, RTCP, mid-flow TLS) correctly report null rather than a guess. 153 tests, 152 pass (PipelineIntegrationTest needs a Docker socket locally). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0165VsVLYm3G8DnE5N9vXToS
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe change records TCP connection initiators during packet parsing, persists and exposes initiator metadata, and updates the frontend to derive roles and node types from backend signals. Legacy host identities are lazily backfilled when requested. ChangesInitiator capture and API propagation
Backend-adjudicated graph metadata
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Tshark
participant PcapParserService
participant Database
participant ConversationQueryService
participant Frontend
Tshark->>PcapParserService: provide frame and TCP flag fields
PcapParserService->>Database: persist initiatorIp and initiatorPort
Frontend->>ConversationQueryService: request conversations
ConversationQueryService-->>Frontend: return initiator metadata
Frontend->>Frontend: derive roles from initiator and project identities
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces tracking of TCP connection initiators (using SYN without ACK flags) to replace port-based role guessing, updating the backend parser, database schema, and DTOs. It also refactors the frontend to render adjudicated host identities directly from the backend rather than performing client-side classification. Feedback highlights several critical issues: fragile absolute array indexing in PcapParserService that breaks existing frame number parsing, a logic bug in networkService.ts that incorrectly classifies most active nodes as having 'both' roles due to truthy 'unknown' values, a potential race condition in the lazy backfill endpoint, and a hardcoded connectionCount of 0 in the node type evidence.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Critical bug, caught by gemini, and one I warned about then walked into. Appending tcp.flags.syn/ack to the tshark field list moved frame.number off the last slot — but the parse loop read it as f[f.length - 1]. So every packet's stored number became the ACK bit (0 or 1). Confirmed in the DB before fixing: 406 distinct packet numbers across 411 packets, min 0 (frame numbers start at 1 and are unique). The irony: my own commit comment said "read by positional index, so a field in the middle would silently shift them all" — and I broke it at the *tail*, where the reader was already tail-relative for exactly this reason. The comment even claimed "frame.number is the last field", which the change made false. Fix reads all three trailing fields from the end, as gemini suggested: frame.number = f[f.length - 3] tcp.flags.syn = f[f.length - 2] tcp.flags.ack = f[f.length - 1] This fixes the corruption AND makes initiator detection robust against index shifting — an Info-column '|' can no longer break either. Re-analysed discord.pcap after the fix: packet numbers: 411 distinct = 411 total, min 1, max 411 (was 406, min 0) initiator : 10.0.2.15:42834 (still exact vs tshark) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0165VsVLYm3G8DnE5N9vXToS
The four functions #521 targeted are gone. The browser no longer decides what a host is; it renders what the backend adjudicated. determineRole(port) DELETED — "< 1024 means server". #496's bug. finalizeNodeRole(...) DELETED — ORed 'server' across BOTH endpoints, so a host talking from :51000 to a router's :80 was itself marked a server. classifyNodeType(...) DELETED — a Scan-stage classifier (nDPI app -> port -> peer fan-out) running in the browser over a TRUNCATED 50-node set, so a host could classify differently depending on what else fit on screen. getNodeColor precedence DELETED — adjudicated nodeType vs deviceType by display order, with no confidence, no contested. What replaces them: role <- conv.initiatorIp (#496). Who opened the connection is a MEASURED fact now, so role is read, not guessed. Accumulates across flows: a host that opens some and answers others is genuinely 'both' — a value the port heuristic could never produce, though the type always allowed it. Null initiator (UDP/ARP/mid-flow) => say nothing. nodeType <- the adjudicated host identity, projected through one map (IDENTITY_LABEL_TO_NODE_TYPE). This is #499's fix: WEB_SERVER and web-server were two taxonomies for one question, disagreeing on colour. Now one is a projection of the other — the adjudicator decides, the graph renders. No third taxonomy. colour <- follows nodeType, which already IS the one answer. Nothing left to resolve. The honest risk, handled: HostIdentity only exists for files analysed after the adjudicator shipped (slice 5). Measured it — 4 of 12 files had identities, though all 12 have the classifications to adjudicate FROM. So GET /host-identities now backfills lazily: empty + classifications present => adjudicate on read. Idempotent (@transactional, same inputs -> same winners), so fresh files are untouched and the cost is paid once per legacy file. Verified: a legacy file went 0 -> 180 identities on first read. Verified on screen, tracing colour to conclusion rather than eyeballing: 10.0.2.10 adjudicated ROUTER -> nodeType router (amber router icon) 203.0.113.1 adjudicated WEB_SERVER -> nodeType web-server (green, conf 100) The legend shows one reconciled taxonomy. backend 153 tests (152 pass, Docker-socket), frontend 57 pass, tsc clean. Closes #521. Part of #499, #496. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0165VsVLYm3G8DnE5N9vXToS
The known-divergence section listed "the frontend scans and adjudicates" as outstanding; that is now false. Records the four deleted functions, role from initiatorIp, nodeType as a projection of the adjudication (closing #499's two- taxonomies problem), and the lazy backfill for legacy files. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0165VsVLYm3G8DnE5N9vXToS
NotYuSheng
force-pushed
the
feature/521-present-consumes-adjudications
branch
from
July 17, 2026 13:42
440967b to
b300e3e
Compare
Three from gemini's review of #530. The first would have made #521 pointless. 1. CRITICAL — every node was becoming 'both'. role initialises to 'unknown', which is a truthy string, so `if (!node.data.role)` was false on the first flow and the code fell straight to the 'both' branch. So the moment a host had any flow with a known initiator, it was 'both' — the exact opposite of reading the fact. Guard 'unknown' explicitly. Proven on discord.pcap: 10.0.2.15 (initiator) -> client, 162.159.128.233 (responder) -> server. With the bug both would have been 'both'. 2. Lazy-backfill race. Two concurrent first-reads of a legacy file both find it empty, both run delete-and-regenerate, and the loser hits the unique (file_id, ip) constraint -> 500. Catch DataIntegrityViolationException and fall through to the read: the winner's rows are already what we wanted. 3. nodeTypeEvidence.connectionCount was hardcoded 0. It is the busiest server port's actual count, which dominantServerPort already located — populate it. frontend 57 pass, backend 153 (152, Docker-socket), tsc clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0165VsVLYm3G8DnE5N9vXToS
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/features/network/services/networkService.ts (1)
484-513: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not project generic
SERVERasdatabase-server.
SERVERcontains no database evidence, but this mapping assigns the database icon, colour, and node type. Map it tounknownor introduce a generic server node type rather than inventing a more specific adjudication.Proposed safe mapping
- SERVER: 'database-server', + SERVER: 'unknown',🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/features/network/services/networkService.ts` around lines 484 - 513, Update IDENTITY_LABEL_TO_NODE_TYPE so the generic SERVER identity maps to unknown rather than database-server; keep database-server reserved for identities with explicit database evidence and leave the other label mappings unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@backend/src/main/java/com/tracepcap/insights/controller/HostIdentitiesController.java`:
- Around line 38-47: Update the DataIntegrityViolationException catch in the
host identity read flow to re-query hostIdentityRepository.findByFileId(fileId)
after the failed adjudicateFile call. Swallow and log the exception only when
the re-query finds identities; otherwise rethrow the original exception so
genuine persistence failures are not returned as an empty successful response.
In `@docs/architecture/layers.rst`:
- Around line 419-420: Update the lazy backfill documentation in the
architecture layers text to reference GET /files/{fileId}/host-identities
instead of GET /host-identities, preserving the existing idempotent first-read
behavior.
In `@frontend/src/components/network/NetworkGraph/NetworkGraph.tsx`:
- Around line 250-261: Update the generic-node legend to derive its colors from
NODE_TYPE_CONFIG using the same nodeType-based authority as getNodeColor,
replacing deviceTypeColor(dt) while preserving the legend’s existing entries and
fallback behavior.
In `@frontend/src/features/network/services/networkService.ts`:
- Around line 319-326: The network service currently applies host roles only
while processing the rendering-limited conversation set. Update the conversation
aggregation flow around updateNodeStats and applyRoleFromInitiator so role
accumulation processes all conversations before limitedConversations is derived,
or consume an equivalent backend host-level result; retain the rendering cap for
graph presentation only. In docs/architecture/layers.rst lines 411-420, defer or
revise the “Present no longer adjudicates” claim until host roles are
independent of the truncated graph subset.
---
Outside diff comments:
In `@frontend/src/features/network/services/networkService.ts`:
- Around line 484-513: Update IDENTITY_LABEL_TO_NODE_TYPE so the generic SERVER
identity maps to unknown rather than database-server; keep database-server
reserved for identities with explicit database evidence and leave the other
label mappings unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c2fa533c-477e-4eab-9bd2-354d4152ac7b
📒 Files selected for processing (16)
backend/src/main/java/com/tracepcap/analysis/dto/ConversationDetailResponse.javabackend/src/main/java/com/tracepcap/analysis/dto/ConversationResponse.javabackend/src/main/java/com/tracepcap/analysis/entity/ConversationEntity.javabackend/src/main/java/com/tracepcap/analysis/service/AnalysisService.javabackend/src/main/java/com/tracepcap/analysis/service/ConversationLookupAdapter.javabackend/src/main/java/com/tracepcap/analysis/service/PcapParserService.javabackend/src/main/java/com/tracepcap/analysis/spi/ConversationLookup.javabackend/src/main/java/com/tracepcap/conversation/service/ConversationQueryService.javabackend/src/main/java/com/tracepcap/insights/controller/HostIdentitiesController.javabackend/src/main/resources/db/migration/V35__conversation_initiator.sqldocs/architecture/layers.rstfrontend/src/components/network/NetworkGraph/NetworkGraph.tsxfrontend/src/features/conversation/services/conversationService.tsfrontend/src/features/network/services/networkService.tsfrontend/src/types/common.types.tsopenapi/baseline.json
Four from CodeRabbit's review of #530. The first is a real hole in #521 itself. 1. ROLE STILL DEPENDED ON THE DISPLAY CUTOFF. applyRoleFromInitiator ran inside the limitedConversations loop — the top-N by packet count — so a host's client/server determination changed with which flows survived the cap. That is the exact truncation dependency #521 exists to remove, just relocated from classification to role. Moved role aggregation to a pass over ALL conversations, before the cap. It only reads a MEASURED fact (initiatorIp), so running it over the full set is cheap; nodes absent from the diagram (their flows capped out) are skipped since there is nothing to label. 2. Backfill catch was too broad. DataIntegrityViolationException can be a real persistence defect, not just the concurrent-backfill race — swallowing all of them could return 200-with-nothing. Now rethrows if the rows are still absent after the catch; only tolerates it when the winner's rows are actually there. 3. layers.rst said GET /host-identities; the path is GET /files/{fileId}/host-identities. 4. Generic-node legend used deviceTypeColor for its swatch, but generic nodes now colour from NODE_TYPE_CONFIG[nodeType] — so the swatch advertised a colour no node shows. Aligned it to the node's actual colour; the device icon still tells IoT from Mobile. frontend 57 pass, backend compiles, tsc + RST clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0165VsVLYm3G8DnE5N9vXToS
This was referenced Jul 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #528 (which adds the initiator fact this consumes). Review #528 first; this branch includes its commits.
The four functions #521 has tracked since the epic began are deleted. The browser no longer decides what a host is — it renders what the backend adjudicated.
determineRole(port)finalizeNodeRole(...)serveracross both endpoints, so a host talking from :51000 to a router's :80 was itself marked a serverclassifyNodeType(...)getNodeColorprecedenceWhat replaces them
role←conv.initiatorIp(#528). Who opened the connection is a MEASURED fact now, so role is read, not guessed. It accumulates across flows: a host that opens some and answers others is genuinely'both'— a value the port heuristic could never produce, though the type always allowed it. Null initiator (UDP/ARP/mid-flow) → say nothing.nodeType← the adjudicated host identity, projected through one map (IDENTITY_LABEL_TO_NODE_TYPE). This is #499's fix:WEB_SERVERandweb-serverwere two taxonomies for one question, disagreeing on colour (green vs indigo). Now one is a projection of the other — the adjudicator decides, the graph renders. No third taxonomy.Colour ←
nodeType, which already is the one answer. Nothing left to resolve.The honest risk, handled
HostIdentityonly exists for files analysed after the adjudicator shipped (slice 5). I measured before assuming: 4 of 12 files had identities — but all 12 have the classifications to adjudicate from. So deleting the client-side classifier would have rendered 8 files entirely grey.GET /host-identitiesnow backfills lazily: empty + classifications present → adjudicate on read. Idempotent (@Transactional, same inputs → same winners), so fresh files are untouched and the cost is paid once per legacy file.Verified: a legacy file went 0 → 180 identities on first read.
Verification — traced colour to conclusion, not eyeballed
Screenshotted the diagram: nodes coloured by identity, legend showing one reconciled taxonomy (Web Server / Database Server / Router / Mobile / IoT).
backend 153 tests (152 pass —
PipelineIntegrationTestneeds a Docker socket locally), frontend 57 pass,tscclean.layers.rstupdated: the known-divergence entry "the frontend scans and adjudicates" is now resolved.Closes #521. Part of #499, #496.
🤖 Generated with Claude Code
https://claude.ai/code/session_0165VsVLYm3G8DnE5N9vXToS
Summary by CodeRabbit
New Features
Bug Fixes