Feat(abctl): one row per network message, show every message#527
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 22 minutes and 36 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. 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, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR removes plugin-activity gating from session event recording across forward proxy, reverse proxy, and transparent proxy, so every network message is recorded regardless of plugin participation. It adds an explicit ChangesUnconditional Session Recording and Downstream Fixes
TUI Event-Row Model Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
The session timeline was confusing for two reasons: messages no plugin
processed were invisible, and a message processed/skipped by N plugins
showed as N rows (plus a separate row for a TLS-bridged call's CONNECT).
Server (authlib): relax the four-clause append gate to unconditional at
every accept-path emit site (forward-proxy request/response, transparent
tunnel-open, reverse-proxy request/response/streaming) so every message
the pipeline saw is recorded — including passthrough requests and generic
responses (e.g. a 404) no plugin touched. The !skipped guards stay, so
listener.skip_hosts traffic remains suppressed. Raise session.max_events
default 100 -> 500 in all three binaries to absorb the ~2x volume.
abctl: render one table.Row per SessionEvent. eventAction() folds a
message's per-plugin invocations into one ACTION + PLUGIN cell, headlining
the highest-ranked ENFORCED action — deny > modify > observe > allow >
skip. observe outranks allow so a parser (which supplies METHOD) headlines
over a gate that merely allowed; a skip-only or no-plugin message shows
passthrough markers ("—") rather than crediting a plugin that declined to
act. A shadow deny/modify never headlines over the action that really took
effect (the pipeline enforces deny only when !Shadow); it is surfaced with
a trailing "*" instead (e.g. "allow*"), or headlines alone ("deny*") when
nothing enforced acted. The detail pane shows the whole event; a bridged
row also folds the CONNECT tunnel's gate invocations into its ACTION and
inactive-filter view.
A TLS-bridge CONNECT tunnel folds into the decrypted inner request that
follows it, so a bridged call is one request row + one response row, with
a "tunnel:" summary in the detail pane. Two back-to-back passthrough
CONNECTs to the same host are NOT folded (each is its own message).
Event-level span glyphs in PHASE bracket each request/response exchange
and nest outbound calls under the inbound request that caused them. The
`s` key hides passthrough/skip-only messages (default off = show all).
Replaces the per-invocation row model: drops flattenInvocations and the
per-invocation pairing; the span-glyph rendering now operates on events,
so a multi-plugin message no longer duplicates into one row per plugin.
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
MCP tool responses (tools/list, tools/call) over the Streamable HTTP
transport are text/event-stream, dispatched to plugins via
StreamingResponder.OnResponseFrame. In abctl these calls showed the
request parsed (observe) but the response blank (no result, no
invocation) — the response was never parsed.
Root cause: plugins.Build wraps every Configurable plugin in
pipeline.configuredPlugin (to surface raw config on /v1/pipeline). That
wrapper embeds the Plugin INTERFACE, and Go does not promote method-set
membership through an embedded interface — so the wrapped plugin's
OnResponseFrame is not promoted, and the wrapper fails the
StreamingResponder type-assertion in Pipeline.RunResponseFrame /
HasStreamingResponders. mcp-parser implements Configure (Configurable),
so it gets wrapped and silently loses response-frame dispatch;
inference-parser has no Configure, isn't wrapped, and worked. The
listener then takes a path that records the response event with no
parser activity, leaving result/invocations empty.
Fix: WrapConfigured returns a StreamingResponder-preserving wrapper
(configuredStreamingPlugin) when, and only when, the inner plugin
implements StreamingResponder — forwarding OnResponseFrame to it. Using a
distinct type (rather than an unconditional no-op OnResponseFrame on
configuredPlugin) keeps HasStreamingResponders exact, so the listener's
streaming-vs-buffered path selection is unchanged for non-streaming
pipelines.
Also harden mcp-parser's OnResponseFrame to parse via the SSE-aware
parseMCPResponse instead of a bare json.Unmarshal, so the buffered
whole-body dispatch (which can hand it a raw "data: {...}" SSE blob)
records the result instead of silently dropping it.
Tests: pipeline regression (a Configurable+StreamingResponder plugin
stays a StreamingResponder after wrapping; a non-streaming Configurable
plugin does not become one); mcp-parser raw-SSE-blob frame records an
observe; forward-proxy integration test drives a tools/list SSE response
through the proxy with a WrapConfigured'd mcp-parser and asserts the
response is parsed + observed. Verified e2e on Kind: the tools/list
response now records result + mcp-parser observe.
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
huang195
force-pushed
the
feat/abctl-one-row-per-message
branch
from
June 19, 2026 20:45
e69d3c7 to
6cc58fe
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@authbridge/cmd/abctl/tui/events_pane.go`:
- Around line 212-241: The isTunnelOpen function currently infers CONNECT
tunnels using heuristics (checking Direction, Phase, protocol extension absence,
and hasPort), which can incorrectly match ordinary unparsed HTTP messages and
cause them to be misfolded with subsequent same-host requests. Replace the
heuristic checks in isTunnelOpen with an explicit tunnel-open marker from the
SessionEvent producer, such as a dedicated boolean field or enum value on the
SessionEvent that explicitly indicates tunnel-open status, to ensure accurate
detection and prevent unrelated events from being incorrectly folded in
isBridgedInner.
- Around line 506-507: The eventMethod function truncates method names to 22
characters for display purposes, but it is being used directly in comparison
logic at lines 506-507 which causes incorrect pairing of methods with identical
prefixes and breaks filtering on suffixes. Replace the eventMethod(*ri) and
eventMethod(*rj) comparisons with direct access to the raw untruncated method
values from the request objects (such as ri.Method and rj.Method). Keep
eventMethod calls only for rendering/display purposes, not for business logic.
Apply the same fix to the comparison logic at lines 690-692.
In `@authbridge/cmd/abctl/tui/keys.go`:
- Around line 413-417: The help text for the `s` key toggle in the skipHint
variable does not accurately describe what the hideInactive toggle actually
controls. Update the skipHint string on line 413 from "[s] hide skips" to use
terminology that reflects all the types of inactive messages being hidden,
including skips, passthrough, and no-plugin messages, so users understand the
complete effect of toggling this option.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5b36156b-8213-4459-a151-9e0464783e9f
📒 Files selected for processing (19)
authbridge/authlib/config/config.goauthbridge/authlib/listener/forwardproxy/mcp_sse_repro_test.goauthbridge/authlib/listener/forwardproxy/server.goauthbridge/authlib/listener/forwardproxy/server_test.goauthbridge/authlib/listener/forwardproxy/transparent.goauthbridge/authlib/listener/reverseproxy/server.goauthbridge/authlib/pipeline/configured.goauthbridge/authlib/pipeline/configured_test.goauthbridge/authlib/plugins/mcpparser/plugin.goauthbridge/authlib/plugins/mcpparser/streaming_test.goauthbridge/cmd/abctl/tui/app.goauthbridge/cmd/abctl/tui/detail_pane.goauthbridge/cmd/abctl/tui/detail_pane_test.goauthbridge/cmd/abctl/tui/events_pane.goauthbridge/cmd/abctl/tui/events_pane_test.goauthbridge/cmd/abctl/tui/keys.goauthbridge/cmd/authbridge-envoy/main.goauthbridge/cmd/authbridge-lite/main.goauthbridge/cmd/authbridge-proxy/main.go
Comment on lines
+413
to
+417
| skipHint := "[s] hide skips" | ||
| if m.hideInactive { | ||
| skipHint = "[s] show all" | ||
| } | ||
| base := "[↑↓] nav [↵] detail [esc] back [/] filter " + skipHint + " [p] pause [q] quit" |
There was a problem hiding this comment.
Make the s help text match hideInactive.
Line 413 says “hide skips”, but the toggle also hides passthrough/no-plugin messages. That can make hidden network messages look unexpected.
Proposed wording update
- skipHint := "[s] hide skips"
+ inactiveHint := "[s] hide passthru/skips"
if m.hideInactive {
- skipHint = "[s] show all"
+ inactiveHint = "[s] show all"
}
- base := "[↑↓] nav [↵] detail [esc] back [/] filter " + skipHint + " [p] pause [q] quit"
+ base := "[↑↓] nav [↵] detail [esc] back [/] filter " + inactiveHint + " [p] pause [q] quit"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| skipHint := "[s] hide skips" | |
| if m.hideInactive { | |
| skipHint = "[s] show all" | |
| } | |
| base := "[↑↓] nav [↵] detail [esc] back [/] filter " + skipHint + " [p] pause [q] quit" | |
| inactiveHint := "[s] hide passthru/skips" | |
| if m.hideInactive { | |
| inactiveHint = "[s] show all" | |
| } | |
| base := "[↑↓] nav [↵] detail [esc] back [/] filter " + inactiveHint + " [p] pause [q] quit" |
🤖 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 `@authbridge/cmd/abctl/tui/keys.go` around lines 413 - 417, The help text for
the `s` key toggle in the skipHint variable does not accurately describe what
the hideInactive toggle actually controls. Update the skipHint string on line
413 from "[s] hide skips" to use terminology that reflects all the types of
inactive messages being hidden, including skips, passthrough, and no-plugin
messages, so users understand the complete effect of toggling this option.
A request/response pair nested three or more levels deep — e.g. a tools/list pair inside an a2a message/stream span inside a long-lived $transport/stream span — rendered "││" on both rows instead of "│┌" / "│└", so the pair didn't read as connected. computeSpanGlyphs caps the PHASE prefix at two levels and was picking the two WIDEST enclosing spans, whose middle bars masked the row's own corner. Pick the inner glyph from the row's NARROWEST containing span (its own tightest exchange) instead of the second-widest, so an endpoint of a deeply-nested pair always shows its ┌/└ corner. The outer glyph still shows the broadest enclosing span for context. Adds a triple-nested test case covering exactly this shape. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
session.max_events is now 500, so one-row-at-a-time nav through a busy session is tedious. Bind PgUp/PgDn (and pgdn) to page the active pane by a near-full screen — events/sessions/pipeline/catalog tables move the cursor by (visible height − 1), one row of overlap for context, clamped to the row range; the detail viewport delegates to its built-in page scroll. Surface "[⇞⇟] page" in the events footer. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
Mac laptops have no dedicated Page Up/Down keys (they're fn+↑/fn+↓), so route the less/vim-style b (page up) and f (page down) through the same pager as PgUp/PgDn — uniform one-row overlap on every keyboard — and show "[b/f] page" in the events footer instead of the ⇞⇟ glyphs, which aren't obvious on a laptop. Extends the page-nav test to cover b/f. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
…oter text Three review findings on PR rossoctl#527: - (Major) Don't infer CONNECT tunnels from host/extension shape. With unconditional recording, an ordinary unparsed outbound request can look like a tunnel-open (no protocol ext + host:port) and get wrongly folded with a following same-host request. Add an explicit Tunnel marker to SessionEvent, set by recordTunnelOpened, and key abctl's isTunnelOpen on it instead of the heuristic. (Drops the now-unused hasPort helper.) - (Minor) eventMethod truncates to 22 chars for display but was used for pairing and search — long names sharing a 22-char prefix mis-paired, and truncated suffixes weren't searchable. Add eventMethodValue (raw) for logic; keep eventMethod (truncated) for rendering only. - (Minor) The `s` footer hint said "hide skips" but the toggle also hides passthrough/no-plugin messages — now "[s] hide passthru/skip". Tests: SessionEvent JSON round-trip covers Tunnel; recordTunnelOpened sets the marker; abctl tunnel tests set Tunnel explicitly. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
TestReverseProxy_Finisher_{Allow,Deny} read finisherStub.seen immediately
after http.Get returned. The reverse proxy sets FlushInterval=-1, so the
streamed response reaches the client before the handler's deferred
RunFinish (which dispatches OnFinish) executes — a pre-existing race that
flaked CI under -race (~1 in 20 locally).
Poll for the expected finishers with a short timeout (waitSeen) instead of
reading once, and store finisherStub.seen LAST in OnFinish (after outcome)
so observing seen==true guarantees outcome is visible. The negative check
(after-deny must NOT fire) runs after the positive waits, by which point
the single RunFinish dispatch is complete.
Verified: 50× -race runs of the finisher tests and the full authlib -race
suite are green.
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
cwiklik
approved these changes
Jun 22, 2026
cwiklik
left a comment
cwiklik
left a comment
Collaborator
There was a problem hiding this comment.
Clean, well-structured PR that fixes a real UX problem (N rows per message → 1 row per message) and a correctness bug (WrapConfigured losing StreamingResponder).
- Explicit
Tunnelmarker replaces heuristic-based CONNECT detection — eliminates a class of false-fold bugs. eventActionprecedence logic correctly separates enforced vs shadow actions with thorough test coverage.WrapConfiguredfix uses distinct types to preserveHasStreamingResponderssemantics without unconditional no-ops.- Finisher test race properly fixed with atomic store ordering + poll-wait.
- CONNECT→inner adjacency limitation documented with clear follow-up path.
Areas reviewed: Go (library/server), Go (CLI/TUI) — 22 files, ~2500 lines changed
Commits: 7/7 signed-off ✓
CI: 17/17 passing ✓
github-project-automation
Bot
moved this from New/ToDo
to Done
in Kagenti Issue Prioritization
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
3 participants
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.
Problem
The abctl session timeline was confusing for two reasons:
if ev.MCP != nil || ev.Inference != nil || ev.Invocations != nil || plugins != nil— so a request/response no plugin acted on (a genericexample.com404, a passthrough call) was never stored, and abctl couldn't show it.SessionEvent × Invocation), so a single message touched by N plugins appeared as N rows. A TLS-bridged call also showed twice (thehost:443CONNECT event + the decrypted inner-request event).Goal: a clean one-row-per-network-message timeline that shows every message, with per-plugin detail on drill-in.
Changes
Server — record every message (
authbridge/authlib)!skippedguards are untouched, solistener.skip_hoststraffic stays suppressed by design.session.max_eventsdefault 100 → 500 in all three binaries (proxy/envoy/lite) to absorb the ~2× event volume; still tunable.abctl — one row per message (
authbridge/cmd/abctl/tui)table.RowperSessionEvent.eventAction()folds an event's per-plugin invocations into one ACTION + PLUGIN cell, headlining the highest-ranked enforced action —deny > modify > observe > allow > skip.observeoutranksallowso a parser (which supplies the METHOD shown on the row) headlines over a gate that merely allowed — keeping PLUGIN consistent with METHOD.— —), not the name of a plugin that declined to act.!Shadow); it's surfaced with a trailing*(e.g.allow*), or headlines alone (deny*) when nothing enforced acted.tunnel:summary in the detail pane. Two back-to-back passthrough CONNECTs to the same host are not folded.┌/│/└) in PHASE bracket each request/response exchange and nest outbound calls under the inbound request that caused them. A pair nested 3+ deep keeps its own┌/└corners (the inner glyph tracks the row's narrowest containing span, not the second-widest), so deeply-nested exchanges still read as connected.stoggle hides passthrough/skip-only messages — default off = show everything.flattenInvocations, per-invocation pairing,eventScopedToPlugin; span rendering now operates on events).abctl — page navigation
b/f(less/vim style, work on any keyboard — Mac laptops have no dedicated Page keys) as well as PgUp/PgDn. Footer shows[b/f] page.Also included — fix MCP/SSE responses not being parsed
While e2e-testing the timeline, MCP tool calls (
tools/list,tools/call) showed the request parsed (observe / mcp-parser) but the response blank — no result, no invocation. Root cause:plugins.Buildwraps every Configurable plugin inpipeline.configuredPlugin(to surface raw config on/v1/pipeline), and that wrapper embeds thePlugininterface — Go doesn't promote method-set membership through an embedded interface, so the wrapped plugin'sOnResponseFrameis lost and it fails theStreamingResponderassertion inRunResponseFrame/HasStreamingResponders. mcp-parser hasConfigure(so it's wrapped and broke); inference-parser doesn't (so it worked).WrapConfiguredreturns aStreamingResponder-preserving wrapper only when the inner plugin implements it, forwardingOnResponseFrame. A distinct type (not an unconditional no-op) keepsHasStreamingRespondersexact, so the streaming-vs-buffered path selection is unchanged for non-streaming pipelines.OnResponseFramenow parses via the SSE-awareparseMCPResponse(not a barejson.Unmarshal), so the buffered whole-body dispatch handling a rawdata: {...}SSE blob records the result instead of dropping it.tools/listresponse now recordsresult+mcp-parser observe.Tests
WrapConfiguredpreservesStreamingResponderfor a Configurable+streaming plugin and does not promote a non-streaming one; mcp-parser parses a raw-SSE-blob frame; forward-proxy integration drives atools/listSSE response through aWrapConfigured'd mcp-parser and asserts it's parsed.eventActionprecedence (incl.observe > allow, shadow →*, skip → passthrough);eventInactive; multi-plugin → 1 row; empty-invocations response → 1 row with status; CONNECT+inner → 1 collapsed row; passthrough/two-CONNECT not folded; tunnel invocations folded into the row; event-level pairing + nested + triple-nested span glyphs;hideInactiveintegration; page-up/down (PgUp/PgDn + b/f).go test ./...green for both modules;go vet+gofmtclean; all three release binaries build underGOWORK=off.Notes / follow-ups
Assisted-By: Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Configuration