Skip to content

Recover from completion panics#81

Closed
M09Ic wants to merge 1 commit into
reeflective:mainfrom
M09Ic:pr/console-completion-panic-recover
Closed

Recover from completion panics#81
M09Ic wants to merge 1 commit into
reeflective:mainfrom
M09Ic:pr/console-completion-panic-recover

Conversation

@M09Ic

@M09Ic M09Ic commented Jun 27, 2026

Copy link
Copy Markdown

Background

A panic during completion currently escapes the completion path and can bring down an interactive console. Completion is usually triggered while typing, so a single bad completer or transient command-tree state should degrade into a visible completion error instead of crashing the process.

Approach

  • Add a deferred recover around Console.complete.
  • Return a readline completion message containing the panic value.
  • Reset the menu command state after a recovered panic so the next completion starts from a clean command tree.

Tests

  • Added a regression test that forces a nil command-tree panic and verifies complete does not panic.
  • Ran go test ./...

@codecov

codecov Bot commented Jun 28, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 23.18%. Comparing base (7002774) to head (c47f10f).

Files with missing lines Patch % Lines
completer.go 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #81      +/-   ##
==========================================
+ Coverage   22.54%   23.18%   +0.63%     
==========================================
  Files          27       27              
  Lines        2045     2049       +4     
==========================================
+ Hits          461      475      +14     
+ Misses       1554     1544      -10     
  Partials       30       30              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

maxlandon added a commit to reeflective/readline that referenced this pull request Jul 5, 2026
readline calls the application-provided Completer on nearly every keystroke,
for both menu completion and as-you-type autocomplete (both funnel through
commandCompletion). A panic in that user code — a bad completer or transient
command-tree state — propagated out of the completion path and crashed the
whole interactive shell.

Recover around the completer invocation and surface the panic value as a
completion message instead, so a single faulty completion degrades into a
visible error rather than taking down the process. Because both completion
paths share commandCompletion, this one choke point covers them all.

This is the readline-side fix for reeflective/console#81, which worked around
the crash by recovering in its own completer; downstream callers no longer
need to guard their completers against panics.
@maxlandon

Copy link
Copy Markdown
Member

The crash this guards against is better fixed upstream in readline: readline invokes the application Completer on nearly every keystroke, so it — not each downstream caller — should recover from a panicking completer. Opened reeflective/readline#116 to add a recover() around the completer invocation in commandCompletion, which protects every readline consumer.

Once that lands, this PR can be dropped, or trimmed down to just the menu.resetPreRun() command-tree reset (which is console-specific and not something readline can do). Thanks for surfacing it!

@maxlandon

Copy link
Copy Markdown
Member

Closing: this is now fixed upstream in readline (reeflective/readline#116, released in readline v1.3.0). readline recovers from panics in the application completer at its own boundary, so console no longer needs to guard its completer. Once console bumps its readline dependency to v1.3.0 the crash is handled for every consumer.

If you still want the menu.resetPreRun() command-tree reset that this PR also included (console-specific, not something readline can do), feel free to open a focused PR for just that piece. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants