fix(authbridge): post-#464 follow-ups — abctl pipeline panic + echo demo preflight#470
Conversation
make demo-echo previously failed at the late setup-keycloak step (after building images, overriding the operator, and deploying) when python-keycloak was missing. Move the check into preflight so it fails before any of that. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
…nic)
The inbound/outbound "(app)" divider row in the pipeline table carried 7 cells, but the table has only 6 columns (the WRITES column was removed earlier). bubbles' table.renderRow indexes columns by cell position, so it panicked ("index out of range [6] with length 6") whenever abctl rendered a pipeline. Trim the divider to 6 cells and add a regression test asserting every row matches the column count.
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds invocation-scoped event detail rendering and selection plumbing, fixes a pipeline table divider row to match the table’s 6 columns with a regression test, and adds a demo preflight check to verify python-keycloak is installed. ChangesTUI: invocation scoping and pipeline rendering
Demo preflight
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
The events list is one row per plugin invocation, but selecting a row rendered the whole SessionEvent (every plugin's invocation). Carry the selected invocation through to showDetail and render a copy scoped to that plugin's invocations and Plugins entry. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
authbridge/cmd/abctl/tui/detail_pane_test.go (1)
70-127: 💤 Low valueConsider adding edge case test coverage.
The existing tests cover the main scenarios well. For additional robustness, consider adding test cases for:
eventScopedToPluginwhen the plugin doesn't exist in the invocations or plugins mapeventScopedToPluginwhene.Invocationsis nileventScopedToPluginwhene.Pluginsis nilThese edge cases are handled correctly by the implementation, but explicit test coverage would improve confidence.
🤖 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/detail_pane_test.go` around lines 70 - 127, Add unit tests to cover edge cases for eventScopedToPlugin: (1) when the requested plugin name does not appear in either ev.Invocations or ev.Plugins — assert returned scoped event has empty Invocations.Inbound and Plugins map (and original ev unchanged); (2) when ev.Invocations is nil — call eventScopedToPlugin(ev, "some-plugin") and assert no panic and returned scoped.Invocations is nil or empty as appropriate and original ev.Invocations remains nil; (3) when ev.Plugins is nil — call eventScopedToPlugin(ev, "some-plugin") and assert no panic and returned scoped.Plugins is empty map or nil as per implementation and original ev.Plugins remains nil; reference the helper eventScopedToPlugin and the existing tests TestEventScopedToPlugin_FiltersToSelectedPlugin/TestEventScopedToPlugin_EmptyPluginReturnsUnchanged to mirror structure and assertions.
🤖 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.
Nitpick comments:
In `@authbridge/cmd/abctl/tui/detail_pane_test.go`:
- Around line 70-127: Add unit tests to cover edge cases for
eventScopedToPlugin: (1) when the requested plugin name does not appear in
either ev.Invocations or ev.Plugins — assert returned scoped event has empty
Invocations.Inbound and Plugins map (and original ev unchanged); (2) when
ev.Invocations is nil — call eventScopedToPlugin(ev, "some-plugin") and assert
no panic and returned scoped.Invocations is nil or empty as appropriate and
original ev.Invocations remains nil; (3) when ev.Plugins is nil — call
eventScopedToPlugin(ev, "some-plugin") and assert no panic and returned
scoped.Plugins is empty map or nil as per implementation and original ev.Plugins
remains nil; reference the helper eventScopedToPlugin and the existing tests
TestEventScopedToPlugin_FiltersToSelectedPlugin/TestEventScopedToPlugin_EmptyPluginReturnsUnchanged
to mirror structure and assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0d01abf2-6588-43b4-a0d5-0dbf140cfdef
📒 Files selected for processing (5)
authbridge/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/keys.go
Split the merged doc block so showDetail keeps its own godoc and eventScopedToPlugin gets a one-liner (the comment had drifted onto the helper). Use the PEP 668-friendly 'pip3 install --user' form for the python-keycloak hints, matching the adjacent PyYAML check. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
cwiklik
left a comment
There was a problem hiding this comment.
Clean follow-up fixing the pipeline table startup panic (divider row had 7 cells vs 6 columns) and scoping event detail to the selected plugin invocation. Both fixes have regression tests. Echo demo preflight now catches missing python-keycloak early.
Areas reviewed: Go (TUI pane logic, detail scoping, pipeline table), Makefile (echo demo), Tests
Commits: 4 commits, all signed-off
CI status: all passing
pdettori
left a comment
There was a problem hiding this comment.
Clean follow-up PR fixing two TUI bugs and improving demo UX.
Areas reviewed: Go (TUI logic + tests), Makefile, Security
Commits: 4/4 signed-off, conventional format
CI: All checks passing
No issues found — well-structured fixes with proper regression tests.
Summary
Small follow-ups in the authbridge abctl / echo-demo area (post-#464). Three independent fixes:
1. fix(abctl): pipeline divider row cell count — fixes startup panic
abctlpanicked the moment it rendered a pipeline:The pipeline table has 6 columns and
pipelineRowreturns 6 cells, but the inbound/outbound── (app) ──divider row still had 7 cells — a leftover from when there was a 7thWRITEScolumn. bubbles'renderRowindexes columns by cell position, so the extra cell panics. Trimmed the divider to 6 cells + added a regression test asserting every row matches the column count.2. fix(abctl): scope event detail to the selected plugin
The events list is one row per plugin invocation, but selecting a row rendered the whole
SessionEvent(every plugin's invocation + the full per-pluginPluginsmap). Now the selected invocation is carried through toshowDetail, which renders a copy of the event scoped to just that plugin's invocations andPluginsentry (event-level context like the protocol slot/identity is preserved). AddedeventScopedToPlugin+ unit tests.3. demo(authbridge): echo demo — fail fast in preflight on missing python-keycloak
make demo-echocheckedpython-keycloakonly in the latesetup-keycloakstep, so a missing package failed the run after building images, overriding the operator config, and deploying. Moved the check intopreflight(first step), matching the existing PyYAML check.Testing
go test ./cmd/abctl/tui/passes (incl. the two new regression tests); abctl no longer panics, and event detail is plugin-scoped.make demo-echonow errors in preflight with a clear install hint whenpython-keycloakis absent.abctl / demo / tooling only; no production authbridge runtime code touched.
Attribution
Assisted-By: Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores