Open
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Reviewer's GuideIntroduce a background update notifier that asynchronously checks GitHub releases for interactive CLI sessions, persists state to avoid frequent checks, and emits Homebrew-aware upgrade hints after command execution, while extending the registry API, semver parsing, tests, and documentation. Sequence diagram for background update notifier during CLI commandsequenceDiagram
actor User
participant CLI as internal_cli_Main
participant Cmd as cobra_Command
participant Notifier as upgrade_UpdateNotifier
participant Registry as upgrade_registry_GithubRegistry
participant GH as GitHub_API
User->>CLI: start lets (version)
CLI->>CLI: maybeStartUpdateCheck(ctx, version, rootCmd)
CLI->>CLI: shouldCheckForUpdate(commandName, isInteractiveStderr)
alt skip_update_check
CLI->>Cmd: ExecuteContext(ctx)
Cmd-->>CLI: result
CLI->>CLI: printUpdateNotice(nil)
CLI-->>User: command output only
else start_background_check
CLI->>Registry: NewGithubRegistry(ctx)
CLI->>Notifier: NewUpdateNotifier(Registry)
CLI->>Notifier: Check(checkCtx, version) async
activate Notifier
Notifier->>Registry: GetLatestReleaseInfo(checkCtx)
Registry->>GH: GET /repos/lets-cli/lets/releases/latest
GH-->>Registry: ReleaseInfo(tag_name, published_at)
Registry-->>Notifier: ReleaseInfo
Notifier->>Notifier: readState()
Notifier->>Notifier: noticeFromState(...)
Notifier-->>CLI: updateCheckResult(notice)
deactivate Notifier
CLI->>Cmd: ExecuteContext(ctx)
Cmd-->>CLI: result
CLI->>CLI: printUpdateNotice(updateCh)
alt notice_available
CLI->>User: stderr: notice.Message()
CLI->>Notifier: MarkNotified(notice)
Notifier->>Notifier: writeState(updated_state)
else no_notice_or_timeout
CLI->>User: no update message
end
end
Updated class diagram for upgrade notifier and registry typesclassDiagram
class upgrade_UpdateNotice {
+string CurrentVersion
+string LatestVersion
-string command
+Message() string
}
class upgrade_notifierState {
+time_Time CheckedAt
+string LatestVersion
+time_Time LatestPublishedAt
+time_Time NotifiedAt
}
class upgrade_UpdateNotifier {
-registry_RepoRegistry registry
-string statePath
-string executablePath
-func() time_Time now
+Check(ctx context_Context, currentVersion string) (*upgrade_UpdateNotice, error)
+MarkNotified(notice *upgrade_UpdateNotice) error
-noticeFromState(state upgrade_notifierState, currentVersion string, current *semver_Version, now time_Time) *upgrade_UpdateNotice
-readState() (upgrade_notifierState, error)
-writeState(state upgrade_notifierState) error
}
class upgrade_registry_RepoRegistry {
<<interface>>
+GetLatestReleaseInfo(ctx context_Context) (*upgrade_registry_ReleaseInfo, error)
+GetLatestRelease() (string, error)
+DownloadReleaseBinary(packageName string, version string, dstPath string) error
+GetPackageName(os string, arch string) (string, error)
}
class upgrade_registry_GithubRegistry {
-http_Client *http_Client
-context_Context ctx
-string repoURI
-string apiURI
-string downloadURL
-time_Duration downloadPackageTimeout
-time_Duration latestReleaseTimeout
+NewGithubRegistry(ctx context_Context) *upgrade_registry_GithubRegistry
+GetLatestReleaseInfo(ctx context_Context) (*upgrade_registry_ReleaseInfo, error)
+GetLatestRelease() (string, error)
+DownloadReleaseBinary(packageName string, version string, dstPath string) error
+GetPackageName(os string, arch string) (string, error)
}
class upgrade_registry_ReleaseInfo {
+string TagName
+time_Time PublishedAt
}
class util_VersionHelpers {
+ParseVersion(version string) (*semver_Version, error)
}
upgrade_UpdateNotifier --> upgrade_notifierState : uses
upgrade_UpdateNotifier --> upgrade_UpdateNotice : creates
upgrade_UpdateNotifier --> upgrade_registry_RepoRegistry : depends_on
upgrade_registry_GithubRegistry ..|> upgrade_registry_RepoRegistry : implements
upgrade_registry_GithubRegistry --> upgrade_registry_ReleaseInfo : returns
upgrade_UpdateNotifier --> upgrade_registry_ReleaseInfo : reads
upgrade_UpdateNotifier --> util_VersionHelpers : uses
Flow diagram for deciding when to show an update noticeflowchart TD
A[start Check called with currentVersion] --> B[parseStableVersion currentVersion]
B -->|invalid_or_prerelease| Z1[return nil notice]
B -->|stable| C[readState from YAML]
C --> D[now minus state.CheckedAt < updateCheckInterval?]
D -->|yes| E[noticeFromState using cached LatestVersion]
D -->|no| F[GetLatestReleaseInfo from registry]
F -->|error| E
F -->|success| G[update state.CheckedAt, LatestVersion, LatestPublishedAt]
G --> H[writeState]
H --> E
E --> I[parseStableVersion state.LatestVersion]
I -->|invalid| Z2[return nil notice]
I -->|valid| J[current < latest?]
J -->|no| Z3[return nil notice]
J -->|yes| K[now minus state.NotifiedAt < updateNotifyInterval?]
K -->|yes| Z4[return nil notice]
K -->|no| L[isHomebrewInstall executablePath?]
L -->|no| M[command = lets --upgrade]
L -->|yes| N[now minus state.LatestPublishedAt < homebrewNoticeDelay?]
N -->|yes| Z5[return nil notice]
N -->|no| O[command = brew upgrade lets-cli/tap/lets]
M --> P[create UpdateNotice]
O --> P
P --> Z6[return notice]
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
UpdateNotifier.readState, decoding an empty or partially written state file will return an error and disable notifications until the file is manually fixed; consider treatingio.EOF(or a zero-length file) as an empty state instead of an error to make the state format more robust across versions. - The Homebrew detection in
isHomebrewInstallrelies on a hard-coded"/Cellar/lets/"path fragment; if the formula name or layout changes (e.g., different tap/name), this will silently fall back to the generic upgrade command—consider either making this check more flexible or scoping the string match to the tap path you expect (e.g.,lets-cli/tap).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `UpdateNotifier.readState`, decoding an empty or partially written state file will return an error and disable notifications until the file is manually fixed; consider treating `io.EOF` (or a zero-length file) as an empty state instead of an error to make the state format more robust across versions.
- The Homebrew detection in `isHomebrewInstall` relies on a hard-coded `"/Cellar/lets/"` path fragment; if the formula name or layout changes (e.g., different tap/name), this will silently fall back to the generic upgrade command—consider either making this check more flexible or scoping the string match to the tap path you expect (e.g., `lets-cli/tap`).
## Individual Comments
### Comment 1
<location path="internal/cli/cli.go" line_range="218-227" />
<code_context>
+ return ch, cancel
+}
+
+func printUpdateNotice(updateCh <-chan updateCheckResult) {
+ if updateCh == nil {
+ return
+ }
+
+ select {
+ case result := <-updateCh:
+ if result.notice == nil {
+ return
+ }
+
+ if _, err := fmt.Fprintln(os.Stderr, result.notice.Message()); err != nil {
+ log.Debugf("lets: update notifier print failed: %s", err)
+ return
+ }
+
+ if err := result.notifier.MarkNotified(result.notice); err != nil {
+ upgrade.LogUpdateCheckError(err)
+ }
+ default:
+ }
+}
</code_context>
<issue_to_address>
**issue (bug_risk):** Non-blocking receive can easily skip update notices and leaves the goroutine potentially blocked on send.
Because of the `select { case <-updateCh: ... default: }`, if `printUpdateNotice` runs before the goroutine sends to `updateCh`, the default branch is taken and the notice is never printed, even if the check succeeds shortly after. If the send happens after `printUpdateNotice` returns, the goroutine will block forever on `ch <- updateCheckResult`. Consider either using a blocking receive here (the check already has a timeout) or making the goroutine’s send non-blocking and treating the update as best-effort so neither side can block indefinitely and the notice is printed once the check completes.
</issue_to_address>
### Comment 2
<location path="internal/upgrade/notifier.go" line_range="181" />
<code_context>
+
+ defer file.Close()
+
+ if err := yaml.NewDecoder(file).Decode(&state); err != nil {
+ return notifierState{}, fmt.Errorf("failed to decode update state file: %w", err)
+ }
</code_context>
<issue_to_address>
**suggestion:** YAML decode errors on the state file permanently break update checks instead of treating the state as empty.
If the state file is corrupted or partially written, `readState` will keep returning an error and `Check` will surface it on every run. Consider treating decode errors the same as a missing/empty state (return a zero `notifierState` and proceed, with a debug log) so a bad file doesn’t permanently disable update checks.
Suggested implementation:
```golang
if err := yaml.NewDecoder(file).Decode(&state); err != nil {
// Treat a corrupted/partial state file as empty state so update checks can continue.
// This avoids permanently breaking update checks due to a bad state file.
log.Printf("debug: failed to decode update state file, ignoring and treating as empty: %v", err)
return notifierState{}, nil
}
```
1. Ensure `internal/upgrade/notifier.go` imports the standard library `log` package:
- Add `import "log"` to the existing import block if it is not already present.
2. If the project uses a structured logging abstraction instead of the standard library logger, replace the `log.Printf(...)` call with the appropriate debug-level logging call that matches the rest of the codebase (for example, `logger.Debug(...)` or similar).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+218
to
+227
| func printUpdateNotice(updateCh <-chan updateCheckResult) { | ||
| if updateCh == nil { | ||
| return | ||
| } | ||
|
|
||
| select { | ||
| case result := <-updateCh: | ||
| if result.notice == nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
issue (bug_risk): Non-blocking receive can easily skip update notices and leaves the goroutine potentially blocked on send.
Because of the select { case <-updateCh: ... default: }, if printUpdateNotice runs before the goroutine sends to updateCh, the default branch is taken and the notice is never printed, even if the check succeeds shortly after. If the send happens after printUpdateNotice returns, the goroutine will block forever on ch <- updateCheckResult. Consider either using a blocking receive here (the check already has a timeout) or making the goroutine’s send non-blocking and treating the update as best-effort so neither side can block indefinitely and the notice is printed once the check completes.
|
|
||
| defer file.Close() | ||
|
|
||
| if err := yaml.NewDecoder(file).Decode(&state); err != nil { |
There was a problem hiding this comment.
suggestion: YAML decode errors on the state file permanently break update checks instead of treating the state as empty.
If the state file is corrupted or partially written, readState will keep returning an error and Check will surface it on every run. Consider treating decode errors the same as a missing/empty state (return a zero notifierState and proceed, with a debug log) so a bad file doesn’t permanently disable update checks.
Suggested implementation:
if err := yaml.NewDecoder(file).Decode(&state); err != nil {
// Treat a corrupted/partial state file as empty state so update checks can continue.
// This avoids permanently breaking update checks due to a bad state file.
log.Printf("debug: failed to decode update state file, ignoring and treating as empty: %v", err)
return notifierState{}, nil
}- Ensure
internal/upgrade/notifier.goimports the standard librarylogpackage:- Add
import "log"to the existing import block if it is not already present.
- Add
- If the project uses a structured logging abstraction instead of the standard library logger, replace the
log.Printf(...)call with the appropriate debug-level logging call that matches the rest of the codebase (for example,logger.Debug(...)or similar).
kindermax
force-pushed
the
codex/adapt-cli-update-plan-to
branch
from
02c5d6b to
1877c75
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
internal/clito run in the background and print upgrade hints after commands finishLETS_NO_UPDATE_NOTIFIERvariable in docsSummary by Sourcery
Add an opt-outable background update notification system backed by GitHub release metadata and cached state, integrating it into the CLI for interactive sessions while updating docs and tests.
New Features:
Enhancements:
Documentation:
Tests: