feat(console): handle huh.ErrUserAborted as graceful exit (exit code 130)#44698
Conversation
…acefully
- Add console.IsCancelled(err) helper to pkg/console/prompt_form.go that
wraps errors.Is(err, huh.ErrUserAborted) for detecting deliberate user
cancellation (Ctrl-C / Esc before form submission).
- Update the 3 secret-entry call sites in pkg/cli/engine_secrets.go to
check IsCancelled: on user abort, print "Cancelled." and return
ExitCodeError{Code: 130} instead of a hard failure message.
- Update pkg/cli/run_interactive.go confirmExecution to log "User aborted
confirmation" vs "Confirmation failed" to distinguish the two paths.
- Add TestIsCancelled unit tests covering ErrUserAborted, wrapped errors,
other errors, and nil.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…ngine_secrets.go Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🤖 PR Triage
Handles
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #44698 does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (51 additions detected, threshold is 100). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
This PR improves interactive UX by treating huh.ErrUserAborted (e.g., Ctrl-C/Esc during huh prompts) as an intentional cancellation, enabling graceful handling (including exit code 130 for secret-entry prompts) rather than surfacing it as a hard failure.
Changes:
- Added
console.IsCancelled(err)to centralize detection ofhuh.ErrUserAborted. - Updated interactive secret-entry prompts to print
Cancelled.and returnExitCodeError{Code: 130}when the user aborts. - Updated interactive run confirmation logging to distinguish user-abort from genuine confirmation failures.
- Added unit tests covering
IsCancelledfor direct, wrapped, unrelated, and nil errors.
Show a summary per file
| File | Description |
|---|---|
| pkg/console/prompt_form.go | Adds IsCancelled helper wrapping errors.Is(err, huh.ErrUserAborted) for centralized cancellation detection. |
| pkg/console/prompt_form_test.go | Adds unit tests validating IsCancelled behavior across common error shapes (direct, wrapped, nil, unrelated). |
| pkg/cli/engine_secrets.go | Handles prompt aborts by printing Cancelled. and returning ExitCodeError{130} to produce a clean cancellation path. |
| pkg/cli/run_interactive.go | Improves logging by separating “user aborted confirmation” from other confirmation errors. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Low
There was a problem hiding this comment.
The changes look correct and well-structured.\n\n- IsCancelled correctly uses errors.Is to handle wrapped errors.\n- promptCancelled() follows Unix convention (exit 130 for Ctrl-C).\n- All three secret-entry call sites are updated consistently.\n- Tests cover direct, wrapped, unrelated, and nil cases.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 12.6 AIC · ⌖ 4.94 AIC · ⊞ 4.8K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — one observation worth addressing, rest is solid.
📋 Key Themes & Highlights
Key Themes
- Inconsistent exit-code propagation in
confirmExecution: The three secret-prompt call sites now returnExitCodeError{130}on Ctrl-C, butconfirmExecutionstill returns(false, nil). A caller cannot distinguish "user said no" from "user pressed Ctrl-C", so the process will exit 1 instead of 130 when aborting a workflow confirmation.
Positive Highlights
- ✅ Centralised
IsCancelled()helper with correcterrors.Iswrapping — easy to reuse. - ✅ Comprehensive test suite covers wrapped, unrelated, and nil error cases.
- ✅ Consistent application across all three secret-entry prompts.
- ✅ Exit code 130 matches the Unix convention for Ctrl-C terminated processes.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 21.6 AIC · ⌖ 4.46 AIC · ⊞ 6.6K
Comment /matt to run again
| } | ||
| return false | ||
| } | ||
|
|
There was a problem hiding this comment.
[/diagnosing-bugs] confirmExecution still returns false on Ctrl-C — the exit-code-130 signal isn't propagated here, unlike the three secret-prompt call sites. A caller that treats false as 'user said no' will exit 1, not 130.
💡 Suggested fix
Consider changing the signature to confirmExecution(...) (bool, error) and returning an error on abort so the caller can propagate ExitCodeError{130}:
if console.IsCancelled(err) {
runInteractiveLog.Print("User aborted confirmation")
return false, promptCancelled()
}This makes cancellation handling consistent across all interactive prompts.
@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — The cancellation-handling intent is correct, but the implementation has gaps that make the exit-130 contract incomplete.
Blocking issues (3)
confirmExecution doesn't propagate exit-130
The bool return type loses the cancellation signal — callers exit 0 on Ctrl-C during confirmation. The secret-entry paths exit 130; the confirmation path silently exits 0. This is the PR's stated goal not fully achieved.
promptCancelled() is untested
The function that defines the observable contract (exit code 130 + "Cancelled." stderr) has zero test coverage. A refactor can silently break the exit code.
Possible double output from huh + promptCancelled()
Needs empirical verification — if huh already renders a cancellation message before returning ErrUserAborted, the extra fmt.Fprintln produces confusing double output.
🔎 Code quality review by PR Code Quality Reviewer · 49.6 AIC · ⌖ 4.77 AIC · ⊞ 5.4K
Comment /review to run again
Comments that could not be inline-anchored
pkg/cli/run_interactive.go:63
confirmExecution cannot propagate exit-130 on Ctrl-C: the bool return collapses three outcomes (confirmed, denied, cancelled) into two, so callers cannot distinguish cancellation from a deliberate "No".
<details>
<summary>💡 Suggested fix</summary>
Change the signature to (bool, error) and return the ExitCodeError{130} on cancellation:
func confirmExecution(ctx context.Context, wf *WorkflowOption, inputs []string) (bool, error) {
...
if err := form.RunWithContext(ct…
</details>
<details><summary>pkg/cli/engine_secrets.go:27</summary>
**Possible double "Cancelled." output**: `promptCancelled()` unconditionally prints to stderr, but the `huh` library itself may already render a cancellation indicator in the terminal when `ErrUserAborted` is returned.
<details>
<summary>💡 Details</summary>
Depending on the `huh` theme and version in use, pressing Ctrl-C may already display something like `^C` or a styled cancellation message in the TUI before returning `ErrUserAborted`. Printing an additional `"Cancelled."` line on top of t…
</details>
<details><summary>pkg/cli/engine_secrets.go:24</summary>
**`promptCancelled()` is untested**: the most behavior-critical helper in this PR — it writes to stderr and returns `ExitCodeError{Code: 130}` — has no unit test.
<details>
<summary>💡 What to test</summary>
Add a test alongside `TestIsCancelled` (in `prompt_form_test.go`, or a new file if `promptCancelled` stays in `engine_secrets.go`) that verifies:
1. The returned error is an `*ExitCodeError` with `Code == 130`.
2. `"Cancelled.\n"` is written to stderr.
```go
func TestPromptCancelled(t *…
</details>
<details><summary>pkg/cli/run_interactive.go:61</summary>
**UX inconsistency: Ctrl-C during confirmation is silent**: secret-entry prompts print `"Cancelled."` to stderr via `promptCancelled()`, but aborting the confirmation step only logs internally — the user sees nothing.
<details>
<summary>💡 Details</summary>
In `engine_secrets.go`, every `IsCancelled` branch calls `promptCancelled()`, which writes `"Cancelled."` to stderr. In `run_interactive.go`, the cancellation branch only calls `runInteractiveLog.Print(...)`, which goes to the debug log — …
</details>
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (4 tests)
📝 AnalysisTest Inflation Note (2.33:1 ratio)
Assertion Quality
Build Tag
Verdict✅ Approved. 0% implementation tests (threshold: 30%). All tests verify behavioral contracts; zero mock violations; 100% edge-case coverage for the new Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
|
@copilot please run the Unresolved review feedback (newest first):
|
|
🎉 This pull request is included in a new release. Release: |
No call site in the codebase checked
huh.ErrUserAborted, so Ctrl-C during secret entry surfaced as a hard failure (e.g.failed to get Copilot token: user aborted) instead of a clean cancellation.Changes
pkg/console/prompt_form.go— addsIsCancelled(err error) boolwrappingerrors.Is(err, huh.ErrUserAborted), centralising the check alongside the other form helpers.pkg/cli/engine_secrets.go— adds a privatepromptCancelled()helper (prints"Cancelled."to stderr, returnsExitCodeError{130}); all three secret-entry call sites (promptForCopilotTokenUnified,promptForSystemTokenUnified,promptForGenericAPIKeyUnified) now call it instead of wrapping the error as a failure.pkg/cli/run_interactive.go—confirmExecutionnow logs"User aborted confirmation"vs"Confirmation failed: <err>"to distinguish the two paths.pkg/console/prompt_form_test.go— unit tests forIsCancelledcovering direct, wrapped, unrelated, and nil errors.