chore(lint): clear pre-existing CI lint baseline - #18
Conversation
- Replace `sb.WriteString(fmt.Sprintf(...))` with `fmt.Fprintf(&sb, ...)` in `confirm.go` and `model.go` (staticcheck QF1012). - Cancel any previous hook context before starting a new one in `startNextHook`. The cancel func was being silently overwritten when hooks completed normally, leaking the prior context — calling it first also clears gosec G118. These were flagged by newer golangci-lint releases; the workflow uses `version: latest` so rule churn started failing CI on previously-passing code. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
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 18 minutes and 52 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 (2)
✨ 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 |
ReviewClean, focused PR — all four lint findings are resolved correctly without nolint suppressions or scope creep. QF1012 —
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // Tear down any previous hook's context before starting a new one — the | ||
| // cancel func is otherwise leaked when hooks complete normally and the | ||
| // field is overwritten on the next call. |
There was a problem hiding this comment.
Per the project's style guideline (CLAUDE.md: "one short line max"), this three-line comment could be condensed. The why is still captured:
| // Tear down any previous hook's context before starting a new one — the | |
| // cancel func is otherwise leaked when hooks complete normally and the | |
| // field is overwritten on the next call. | |
| // Call previous cancel before overwriting — hooks completing normally don't cancel their own context. |
There was a problem hiding this comment.
Pull request overview
Clears newly surfaced CI lint failures by refactoring string formatting in the TUI code and addressing a hook-context cancellation leak.
Changes:
- Replaced
sb.WriteString(fmt.Sprintf(...))withfmt.Fprintf(&sb, ...)ininternal/tui/model.goandinternal/tui/confirm.goto satisfystaticcheck QF1012. - Ensured any previously stored hook cancel func is invoked before being overwritten in
Model.startNextHookto addressgosec G118.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/tui/model.go | Uses fmt.Fprintf for builder output; cancels any prior hook context before creating a new one. |
| internal/tui/confirm.go | Uses fmt.Fprintf for builder output in confirmation/success/error renderers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Tear down any previous hook's context before starting a new one — the | ||
| // cancel func is otherwise leaked when hooks complete normally and the | ||
| // field is overwritten on the next call. | ||
| if m.hookCancelFunc != nil { | ||
| m.hookCancelFunc() | ||
| } |
There was a problem hiding this comment.
startNextHook cancels the previous hook context only when starting a subsequent hook. If the last hook in a phase completes and execution continues (or the program exits) without another startNextHook call, m.hookCancelFunc is never invoked and remains referenced. Consider calling the current cancel func (and setting m.hookCancelFunc = nil) when a hook completes / when leaving StateExecutingHooks, rather than only right before overwriting it here.
![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 optimizes string building by replacing sb.WriteString(fmt.Sprintf(...)) with fmt.Fprintf(&sb, ...) across several TUI components to reduce allocations. Additionally, it fixes a potential context leak in the hook execution logic by ensuring that any existing hookCancelFunc is called before a new context is created. I have no feedback to provide.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Clears the 4 lint findings that were failing CI (and pre-dated PR #17) so the lint job goes green again.
Why these are appearing now
The CI workflow uses
golangci-lint-action@v9withversion: latestandactions/setup-go@v6withgo-version: "stable". Both float, so newer rule versions of staticcheck/gosec started flagging code that was clean at the time it was written. Pinninglatestis the upstream cause; this PR just clears the resulting backlog.Changes
internal/tui/confirm.go,internal/tui/model.go): replacesb.WriteString(fmt.Sprintf(...))withfmt.Fprintf(&sb, ...). Pure mechanical, no behavior change. Includes some lines staticcheck didn't initially report — it has a per-file cap and re-rolls after each fix, so I converted allsb.WriteString(fmt.Sprintf(...))instances in the touched files for consistency.internal/tui/model.go:884):startNextHookwas silently overwritingm.hookCancelFuncwhen hooks completed normally, leaking the priorcontext.WithCancelcleanup. Call the previous cancel func first before assigning a new one — this fixes the (small) real leak and incidentally satisfies G118 without needing a//nolint.Test plan
just fmtjust buildjust test— all packages passjust lint—0 issueslocally🤖 Generated with Claude Code