fix(cli): propagate signal cancellation, expose post-push warnings in JSON - #19
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 56 minutes and 42 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughNew tests validate Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
… JSON - Wire SIGINT/SIGTERM cancellation through fang.WithNotifySignal so Ctrl+C during a non-interactive bump unwinds the in-flight git push and hooks (executor.Execute already plumbs ctx into PushTag and RunHooks). - Add PostPushWarnings to JSONOutput; outputJSON was silently dropping result.PostPushWarnings, leaving --json users blind to fail-open hook failures that the text and TUI paths already surface. - Consolidate the per-hook CancelFunc cleanup in the TUI into clearHookCancel(), and call it in HookCompleteMsg so the final hook's context is released promptly instead of lingering until the next phase starts (or never, for the last phase). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
9fbed8e to
7c44f78
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances signal handling and output consistency by integrating signal notifications into the root command and including post-push warnings in the JSON output. It also refactors TUI rendering to use fmt.Fprintf and introduces better hook lifecycle management. A review comment points out that TUI hooks still use context.Background(), suggesting they should instead use the command context to ensure system signals are correctly propagated and handled during TUI execution.
| func (m *Model) clearHookCancel() { | ||
| if m.hookCancelFunc != nil { | ||
| m.hookCancelFunc() | ||
| m.hookCancelFunc = nil | ||
| } | ||
| } |
There was a problem hiding this comment.
The addition of clearHookCancel is a good improvement for managing hook lifecycles. However, the hooks are still being started with context.Background() in startNextHook (line 882). To fully achieve the PR's goal of propagating signal cancellation, the TUI should ideally receive and use the command context (from cobra.Command) so that signals handled by fang in the root command can also trigger cancellation of in-flight TUI hooks.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
internal/cli/output_test.go (1)
15-58: Test isolation: package-levelflagDryRunleaks intooutputJSON.
outputJSONreads the package-levelflagDryRun(and other flags exist at package scope). These tests don't reset it, so a future test that flipsflagDryRun = truewithout restoring could cause cross-test contamination. The current assertions don't checkdry_run, so it isn't a failure today, but a smallt.Cleanup(or saving/restoring the value) would harden this.♻️ Suggested guard
func TestOutputJSON_IncludesPostPushWarnings(t *testing.T) { + prev := flagDryRun + t.Cleanup(func() { flagDryRun = prev }) + flagDryRun = false cmd := &cobra.Command{}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/output_test.go` around lines 15 - 58, The tests call outputJSON which reads the package-level flagDryRun (and other package flags), so add isolation: in each test (e.g., TestOutputJSON_IncludesPostPushWarnings and TestOutputJSON_OmitsEmptyPostPushWarnings) save the current value of flagDryRun, set flagDryRun to the desired value for the test (likely false), and register a t.Cleanup to restore the original value; alternatively use explicit save/restore around the assertion rather than relying on global state—this ensures outputJSON is exercised with a known dry-run value and prevents cross-test leakage.internal/tui/model.go (1)
1058-1070: Cancellation gap: TUI push still usescontext.Background().The PR description states "Ctrl+C unwinds an in-flight push" — that holds for the non-interactive CLI (executor →
exec.CommandContext), but the TUI path here detaches the push from any cancellable context. Combined withhandleCtrlCblocking quit duringStateExecutingandhookCancelFuncbeing nil during the actual push, a TUI user cannot interrupt a slow/hunggit push.Consider threading a cancellable context (and storing its
CancelFuncsimilarly tohookCancelFunc) so the push respects the same double-ctrl+c cancellation UX as hooks. Pre-existing, but in-theme with this PR.Sketch
// In Model fields: // pushCancelFunc context.CancelFunc func (m Model) doPushAndPostPush() tea.Msg { if m.config.DryRun { return PushCompleteMsg{} } ctx, cancel := context.WithCancel(context.Background()) m.pushCancelFunc = cancel // note: requires pointer receiver to persist defer cancel() if err := m.config.Repository.PushTag(ctx, m.newVersion, m.config.Remote); err != nil { return ErrorMsg{Err: fmt.Errorf("failed to push tag: %w", err)} } return PushCompleteMsg{} }(Persisting the cancel func through
Modelrequires moving this onto a pointer receiver and routing it through a tea.Cmd that captures*Model— same pattern asstartNextHookalready uses.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tui/model.go` around lines 1058 - 1070, The TUI push uses context.Background() in doPushAndPostPush which prevents Ctrl+C cancellation; change Model.doPushAndPostPush to a pointer receiver, create a cancellable context via context.WithCancel, store the cancel function on a new Model field (e.g. pushCancelFunc) alongside the existing hookCancelFunc, defer cancel locally and use the cancellable ctx when calling m.config.Repository.PushTag, and ensure the tea.Cmd that invokes doPushAndPostPush captures *Model (same pattern as startNextHook) so the cancel func persists and Ctrl+C can cancel an in-flight push.internal/tui/confirm.go (1)
67-67: Refactor missed twosb.WriteString(fmt.Sprintf(...))sites.The PR normalizes this pattern across
confirm.go, but lines 67 and 99 still use the old form. Worth folding into the same cleanup for consistency.♻️ Proposed diff
- sb.WriteString(SuccessStyle.Render(fmt.Sprintf("%s Success!", IconCheck))) + fmt.Fprintf(&sb, "%s\n\n", SuccessStyle.Render(fmt.Sprintf("%s Success!", IconCheck)))Or, more cleanly, keep
WriteStringbut skip the innerSprintf:- sb.WriteString(SuccessStyle.Render(fmt.Sprintf("%s Success!", IconCheck))) + sb.WriteString(SuccessStyle.Render(IconCheck + " Success!")) ... - sb.WriteString(ErrorStyle.Render(fmt.Sprintf("%s Error", IconCross))) + sb.WriteString(ErrorStyle.Render(IconCross + " Error"))Also applies to: 99-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tui/confirm.go` at line 67, Two spots still wrap a literal in fmt.Sprintf inside SuccessStyle.Render when calling sb.WriteString; replace sb.WriteString(SuccessStyle.Render(fmt.Sprintf("%s Success!", IconCheck))) (and the similar occurrence around IconCheck on the other line) by removing the inner fmt.Sprintf and passing a plain string (e.g. sb.WriteString(SuccessStyle.Render(IconCheck+" Success!"))), keeping the same sb.WriteString + SuccessStyle.Render pattern and updating both occurrences referencing sb.WriteString, SuccessStyle.Render, and IconCheck.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/cli/output_test.go`:
- Around line 15-58: The tests call outputJSON which reads the package-level
flagDryRun (and other package flags), so add isolation: in each test (e.g.,
TestOutputJSON_IncludesPostPushWarnings and
TestOutputJSON_OmitsEmptyPostPushWarnings) save the current value of flagDryRun,
set flagDryRun to the desired value for the test (likely false), and register a
t.Cleanup to restore the original value; alternatively use explicit save/restore
around the assertion rather than relying on global state—this ensures outputJSON
is exercised with a known dry-run value and prevents cross-test leakage.
In `@internal/tui/confirm.go`:
- Line 67: Two spots still wrap a literal in fmt.Sprintf inside
SuccessStyle.Render when calling sb.WriteString; replace
sb.WriteString(SuccessStyle.Render(fmt.Sprintf("%s Success!", IconCheck))) (and
the similar occurrence around IconCheck on the other line) by removing the inner
fmt.Sprintf and passing a plain string (e.g.
sb.WriteString(SuccessStyle.Render(IconCheck+" Success!"))), keeping the same
sb.WriteString + SuccessStyle.Render pattern and updating both occurrences
referencing sb.WriteString, SuccessStyle.Render, and IconCheck.
In `@internal/tui/model.go`:
- Around line 1058-1070: The TUI push uses context.Background() in
doPushAndPostPush which prevents Ctrl+C cancellation; change
Model.doPushAndPostPush to a pointer receiver, create a cancellable context via
context.WithCancel, store the cancel function on a new Model field (e.g.
pushCancelFunc) alongside the existing hookCancelFunc, defer cancel locally and
use the cancellable ctx when calling m.config.Repository.PushTag, and ensure the
tea.Cmd that invokes doPushAndPostPush captures *Model (same pattern as
startNextHook) so the cancel func persists and Ctrl+C can cancel an in-flight
push.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f24e2c4d-fd50-43ec-9bda-f0c7314b2138
📒 Files selected for processing (4)
internal/cli/output_test.gointernal/cli/root.gointernal/tui/confirm.gointernal/tui/model.go
1 participant
Summary
Three small correctness fixes from a codebase audit:
cli.Executeregisters SIGINT/SIGTERM viafang.WithNotifySignal, andrunNonInteractivepassescmd.Context()toexecutor.Executeinstead ofcontext.Background(). The executor already threads ctx intoPushTag(exec.CommandContext) andRunHooks, so Ctrl+C now actually unwinds an in-flight push.--jsonnow reports post-push hook warnings.JSONOutputwas missing apost_push_warningsfield andoutputJSONsilently droppedresult.PostPushWarnings— automation users were blind to fail-open hook failures that the text and TUI paths already surface.CancelFunccleanup intoclearHookCancel()and call it inHookCompleteMsg, so the final hook's context is released as soon as it completes instead of lingering until the next phase starts (or never, for the last phase).Test plan
just check— all packages pass, golangci-lint cleaninternal/cli/output_test.go— covers populated and emptyPostPushWarningsJSON cases (verifiesomitempty)--json— verify warning appears in output🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
post_push_warningsfield that displays any warnings generated during push operations.Improvements