Skip to content

fix(abctl): correct pipeline divider row cell count (fixes startup panic)#469

Closed
huang195 wants to merge 1 commit into
rossoctl:mainfrom
huang195:fix/abctl-pipeline-divider-panic
Closed

fix(abctl): correct pipeline divider row cell count (fixes startup panic)#469
huang195 wants to merge 1 commit into
rossoctl:mainfrom
huang195:fix/abctl-pipeline-divider-panic

Conversation

@huang195

@huang195 huang195 commented Jun 3, 2026

Copy link
Copy Markdown
Member

Summary

abctl panics on startup the moment it renders a pipeline:

Caught panic:
runtime error: index out of range [6] with length 6
  .../bubbles/table.(*Model).renderRow
  .../bubbles/table.(*Model).UpdateViewport
  .../bubbles/table.(*Model).SetRows
  .../abctl/tui.(*model).rebuildPipelineTable  pipeline_pane.go:48

Root cause

The pipeline table defines 6 columns (#, DIRECTION, PLUGIN, DEPS, BODY, EVENTS), and pipelineRow returns 6 cells — but the inbound/outbound ── (app) ── divider row still had 7 cells:

rows = append(rows, table.Row{"", "", "── (app) ──", "", "", "", ""}) // 7 cells

It's a leftover from when the table had a 7th WRITES column (removed in the capability-simplification change). bubbles' table.renderRow indexes the column slice by cell position, so the 7th cell on a 6-column table panics with index out of range [6] with length 6.

Fix

  • Trim the divider row to 6 cells (matching the columns).
  • Add pipeline_pane_test.go — a regression test asserting every row built by rebuildPipelineTable (including the divider) has exactly the column count, so this can't silently recur.

go test ./cmd/abctl/tui/ passes; abctl no longer panics rendering a pipeline.

Attribution

Assisted-By: Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Fixed a potential crash when rendering the pipeline table with multiple plugin sections.
  • Tests

    • Added regression test to validate pipeline table row structure integrity.

…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>
@coderabbitai

coderabbitai Bot commented Jun 3, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This PR documents and tests a cell-count constraint in the pipeline table divider row. Comments clarify that the divider must have exactly 6 cells to match the table's column structure, preventing bubbles table renderer panics. A new regression test verifies all rows conform to this 6-cell requirement.

Changes

Pipeline Table Divider Row Cell-Count Constraint

Layer / File(s) Summary
Divider row column-cell constraint documentation
authbridge/cmd/abctl/tui/pipeline_pane.go
Comments document that the (app) divider row must have exactly 6 cells matching the table columns, and note that bubbles' table.renderRow panics on cell count mismatches.
Row cell-count regression test
authbridge/cmd/abctl/tui/pipeline_pane_test.go
New test TestRebuildPipelineTable_AllRowsMatchColumnCount builds a pipeline with inbound and outbound plugins, calls rebuildPipelineTable(), and asserts both row count (3: inbound + divider + outbound) and that each row has exactly 6 cells.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

🐰 A table once panicked with rows out of line,
Six cells per divider—a structure so fine!
Comments and tests now keep structure aligned,
No more mismatches—constraints well-defined! ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title clearly and specifically describes the main fix: correcting the pipeline divider row cell count to resolve a startup panic, which is the core change across both modified files.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"


Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
authbridge/cmd/abctl/tui/pipeline_pane_test.go (1)

16-22: 💤 Low value

Consider initializing m.events for test completeness.

While rebuildPipelineTable() handles a nil events map safely (via countEventsPerPlugin() ranging over nil), explicitly initializing it would make the test's model setup more complete and self-documenting.

Optional: initialize events map
 	m := &model{
 		pipeline: &apiclient.PipelineView{
 			Inbound:  []apiclient.PipelinePlugin{{Name: "jwt-validation", Direction: "inbound", Position: 1}},
 			Outbound: []apiclient.PipelinePlugin{{Name: "token-exchange", Direction: "outbound", Position: 1}},
 		},
 		pipelineTbl: newPipelineTable(),
+		events:      map[string][]apiclient.Event{},
 	}
🤖 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/pipeline_pane_test.go` around lines 16 - 22,
Initialize the model's events map in the test setup: when creating the model
instance (m := &model{...}) add an explicit m.events = make(map[string]int) so
the test model is fully initialized; this is a simple change near the model
literal used in pipeline_pane_test.go and keeps rebuildPipelineTable() and
countEventsPerPlugin() behavior unchanged while making the test
self-documenting.
🤖 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/pipeline_pane_test.go`:
- Around line 16-22: Initialize the model's events map in the test setup: when
creating the model instance (m := &model{...}) add an explicit m.events =
make(map[string]int) so the test model is fully initialized; this is a simple
change near the model literal used in pipeline_pane_test.go and keeps
rebuildPipelineTable() and countEventsPerPlugin() behavior unchanged while
making the test self-documenting.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 5ff6831a-effb-46c8-9bb5-34730e9d05d0

📥 Commits

Reviewing files that changed from the base of the PR and between 191b2cb and 4399434.

📒 Files selected for processing (2)
  • authbridge/cmd/abctl/tui/pipeline_pane.go
  • authbridge/cmd/abctl/tui/pipeline_pane_test.go

@huang195

huang195 commented Jun 3, 2026

Copy link
Copy Markdown
Member Author

Superseded by #470 (combined with the echo-demo preflight fix).

@huang195 huang195 closed this Jun 3, 2026
@github-project-automation github-project-automation Bot moved this from New /:ToDo to Done in Rossoctl Issue Prioritization Jun 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants