feat: add automatic CLI update detection and notification#144
Merged
MaxHuang22 merged 1 commit intomainfrom Mar 31, 2026
Merged
feat: add automatic CLI update detection and notification#144MaxHuang22 merged 1 commit intomainfrom
MaxHuang22 merged 1 commit intomainfrom
Conversation
|
|
📝 WalkthroughWalkthroughAdds an update-notifier: new internal/update package with caching and network fetch, CLI wiring to set a pending notice (sync cached check + async refresh), injection of an optional Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Execute()
participant Root as cmd/root
participant Cache as Local Cache
participant Registry as npm Registry
participant Output as Output Envelope
participant User as User
CLI->>Root: Execute()
Root->>Cache: CheckCached(current)
Cache-->>Root: cached UpdateInfo?
Root->>Root: SetPending(if info)
Root->>Root: spawn goroutine RefreshCache(current)
par Async Refresh
Root->>Cache: RefreshCache(current)
Cache->>Registry: FetchLatest()
Registry-->>Cache: latest version
Cache->>Cache: persist update-state
Cache-->>Root: SetPending(UpdateInfo)
and Main Flow
Root->>Output: run command logic
Output->>Output: GetNotice()
Output-->>User: JSON envelope (+ _notice if set)
end
sequenceDiagram
participant Doctor as doctor command
participant Build as build.Version
participant Update as update package
participant Registry as npm Registry
participant Result as checkResult
Doctor->>Build: read current version
Doctor->>Result: add cli_version entry
Doctor->>Update: checkCLIUpdate() (sync if online)
Update->>Registry: FetchLatest()
alt Fetch succeeds
Update->>Update: IsNewer(latest, current)?
alt Newer
Update-->>Doctor: warn (with hint)
else
Update-->>Doctor: pass
end
else Fetch fails
Update-->>Doctor: warn (fetch error)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR introduces a non-blocking automatic update detection system for Key concerns found during review:
Confidence Score: 3/5Not safe to merge as-is — the Raw-error handling change silently removes the stderr error envelope for The core update-check machinery ( cmd/root.go requires the most attention: the Raw-error early-return logic and the Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant CLI as cmd/root.go
participant Cache as update cache (disk)
participant npm as npm registry
participant Output as output package
User->>CLI: run any command
CLI->>Cache: CheckCached(version) [sync, no network]
alt cache hit & newer version
Cache-->>CLI: UpdateInfo
CLI->>Output: SetPending(info)
else cache miss / up-to-date
Cache-->>CLI: nil
end
CLI->>Output: PendingNotice = func(){GetPending()}
CLI-)npm: RefreshCache() [async goroutine]
CLI->>CLI: rootCmd.Execute()
CLI->>Output: PrintJson / WriteErrorEnvelope
Output->>Output: injectNotice() → adds _notice.update if pending
CLI-->>User: JSON with optional _notice.update
npm-->>Cache: save latest version [async, future runs]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
internal/output/print.go (1)
17-44: Avoid mutating caller-owned maps during_noticeinjection.Line 17 + Lines 28-44 currently mutate the input map in place and can overwrite an existing
"_notice"key. That side effect is surprising and can leak outsidePrintJson.Proposed refactor (non-mutating injection + preserve existing
_notice)func PrintJson(w io.Writer, data interface{}) { - injectNotice(data) - b, err := json.MarshalIndent(data, "", " ") + payload := injectNotice(data) + b, err := json.MarshalIndent(payload, "", " ") if err != nil { fmt.Fprintf(os.Stderr, "json marshal error: %v\n", err) return } fmt.Fprintln(w, string(b)) } // injectNotice adds a "_notice" field into CLI envelope maps. // Only modifies map[string]interface{} values that have an "ok" key. -func injectNotice(data interface{}) { +func injectNotice(data interface{}) interface{} { if PendingNotice == nil { - return + return data } m, ok := data.(map[string]interface{}) if !ok { - return + return data } if _, isEnvelope := m["ok"]; !isEnvelope { - return + return data + } + if _, exists := m["_notice"]; exists { + return data } notice := PendingNotice() if notice == nil { - return + return data } - m["_notice"] = notice + cloned := make(map[string]interface{}, len(m)+1) + for k, v := range m { + cloned[k] = v + } + cloned["_notice"] = notice + return cloned }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/output/print.go` around lines 17 - 44, The current injectNotice(data) mutates caller-owned maps and can overwrite an existing "_notice"; change injectNotice so it does not mutate inputs and preserves any pre-existing "_notice": have injectNotice accept interface{} and return interface{} (or a new value) and, when data is a map[string]interface{} with an "ok" key and no "_notice", create a shallow copy of the map, set "_notice" on the copy from PendingNotice(), and return the copy; otherwise return the original data unchanged. Update the PrintJson caller (the spot that currently calls injectNotice(data) before json.MarshalIndent) to use the returned value from injectNotice when marshalling so the original map is never modified and existing "_notice" is preserved.internal/update/update_test.go (1)
225-241: Test isolation: Consider clearingpendingat test start as well as end.
SetPending(nil)at line 240 cleans up for subsequent tests, but if a previous test failed mid-execution,pendingcould be non-nil at the start. Adding a reset at the beginning would make this test more robust.♻️ Suggested improvement
func TestPendingAtomicAccess(t *testing.T) { + // Reset at start in case a prior test left state + SetPending(nil) + // Initially nil if got := GetPending(); got != nil { t.Errorf("expected nil, got %+v", got) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/update/update_test.go` around lines 225 - 241, The test TestPendingAtomicAccess should reset shared state at the start as well as the end to ensure isolation; add a call to SetPending(nil) at the beginning of TestPendingAtomicAccess (before the initial GetPending check) so that the GetPending/SetPending/UpdateInfo behavior is verified from a clean slate and then continue to call SetPending(nil) at the end for cleanup.cmd/root.go (1)
163-170: Consider restricting the completion check to early arguments.The current loop scans all arguments, but "completion" or "__complete" appearing as a value to another flag (e.g.,
--name completion) would incorrectly suppress update notices. Checking only the first 2-3 positional arguments would be more precise.♻️ Optional: Restrict check to early arguments
func isCompletionCommand(args []string) bool { - for _, arg := range args { + // Check only first few args; completion subcommands appear early + limit := 3 + if len(args) < limit { + limit = len(args) + } + for _, arg := range args[:limit] { if arg == "completion" || arg == "__complete" { return true } } return false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/root.go` around lines 163 - 170, The isCompletionCommand function currently scans all args and can misidentify flags' values as the completion command; modify isCompletionCommand to only inspect the first few positional arguments (e.g., check args[0:3] or up to len(args) if smaller) for "completion" or "__complete" so occurrences later (like as a --name value) don't trigger it; update the function isCompletionCommand to iterate only over that limited slice and return true if any of those early entries match.internal/update/update.go (1)
94-97: Silent failure on fetch error may obscure transient issues.When
fetchLatestVersionfails, the error is silently discarded. While this is acceptable for a non-critical background check, consider logging at debug level if there's a debug logging facility available, to aid troubleshooting connectivity issues.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/update/update.go` around lines 94 - 97, When fetchLatestVersion() returns an error we currently drop err and return silently; change this to log the failure at debug/trace level before returning so transient connectivity issues are visible during troubleshooting—capture err from fetchLatestVersion(), emit a debug-level message that includes the error and context (e.g., "fetchLatestVersion failed"), using the repository's existing debug logger (e.g., debugLogger.Debugf, logger.Debug, or similar), and then keep the same early return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/update/update_test.go`:
- Around line 192-223: TestRefreshCache is skipping network work because
CI-related env vars cause shouldSkip to return true; update the test
(TestRefreshCache) to unset CI environment flags at the start (e.g., clear "CI"
and other CI indicators your shouldSkip checks such as "GITHUB_ACTIONS") so that
RefreshCache("1.0.0") runs normally and writes the cache; ensure you remove or
restore any env changes after the test to avoid side effects.
- Around line 161-190: TestCheckCached fails because CheckCached calls
shouldSkip which returns true when CI env vars are present; clear those before
exercising cache logic. In TestCheckCached (the test function), unset the
CI-related env variables (at least "CI", optionally "GITHUB_ACTIONS" etc.)
before calling CheckCached — e.g., call os.Unsetenv("CI") (and any other CI var)
or use t.Setenv to restore state — so shouldSkip returns false and the cached
updateState file is actually read; keep the rest of the test logic and
references to CheckCached and updateState unchanged.
- Around line 102-118: The tests fail because CI-related env vars (CI,
BUILD_NUMBER, RUN_ID) set in the CI environment cause shouldSkip to return true;
update the subtests in update_test.go so that before calling shouldSkip you
unset those variables and restore them after the test (capture original values
with os.LookupEnv, call os.Unsetenv for each, and register a t.Cleanup to
restore the original values) so shouldSkip behaves deterministically for each
case; locate the test loop that calls shouldSkip and add this unset/restore
logic around the call.
---
Nitpick comments:
In `@cmd/root.go`:
- Around line 163-170: The isCompletionCommand function currently scans all args
and can misidentify flags' values as the completion command; modify
isCompletionCommand to only inspect the first few positional arguments (e.g.,
check args[0:3] or up to len(args) if smaller) for "completion" or "__complete"
so occurrences later (like as a --name value) don't trigger it; update the
function isCompletionCommand to iterate only over that limited slice and return
true if any of those early entries match.
In `@internal/output/print.go`:
- Around line 17-44: The current injectNotice(data) mutates caller-owned maps
and can overwrite an existing "_notice"; change injectNotice so it does not
mutate inputs and preserves any pre-existing "_notice": have injectNotice accept
interface{} and return interface{} (or a new value) and, when data is a
map[string]interface{} with an "ok" key and no "_notice", create a shallow copy
of the map, set "_notice" on the copy from PendingNotice(), and return the copy;
otherwise return the original data unchanged. Update the PrintJson caller (the
spot that currently calls injectNotice(data) before json.MarshalIndent) to use
the returned value from injectNotice when marshalling so the original map is
never modified and existing "_notice" is preserved.
In `@internal/update/update_test.go`:
- Around line 225-241: The test TestPendingAtomicAccess should reset shared
state at the start as well as the end to ensure isolation; add a call to
SetPending(nil) at the beginning of TestPendingAtomicAccess (before the initial
GetPending check) so that the GetPending/SetPending/UpdateInfo behavior is
verified from a clean slate and then continue to call SetPending(nil) at the end
for cleanup.
In `@internal/update/update.go`:
- Around line 94-97: When fetchLatestVersion() returns an error we currently
drop err and return silently; change this to log the failure at debug/trace
level before returning so transient connectivity issues are visible during
troubleshooting—capture err from fetchLatestVersion(), emit a debug-level
message that includes the error and context (e.g., "fetchLatestVersion failed"),
using the repository's existing debug logger (e.g., debugLogger.Debugf,
logger.Debug, or similar), and then keep the same early return behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f052c032-704b-4772-8622-c8b438bf6314
📒 Files selected for processing (11)
cmd/doctor/doctor.gocmd/root.gointernal/output/envelope.gointernal/output/errors.gointernal/output/errors_test.gointernal/output/print.gointernal/output/print_test.gointernal/update/update.gointernal/update/update_test.goshortcuts/common/runner.goskills/lark-shared/SKILL.md
MaxHuang22
force-pushed
the
feat/auto_upgrade_detect
branch
3 times, most recently
from
3cdfb60 to
73733f5
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/root.go`:
- Around line 186-194: The current early return when exitErr.Raw is true
prevents the structured stderr envelope from being written; update the
asExitError handling so that for Raw errors you skip enrichPermissionError(f,
exitErr) but still call output.WriteErrorEnvelope(errOut, exitErr,
string(f.ResolvedIdentity)) before returning exitErr.Code (i.e., remove the
immediate return in the exitErr.Raw branch or add the WriteErrorEnvelope call
there) so callers receive the error payload and the _notice field while
preserving the skip of enrichPermissionError.
- Around line 163-169: isCompletionCommand currently only checks for
"completion" and "__complete", so Cobra's completion path using
"__completeNoDesc" isn't recognized and still triggers setupUpdateNotice; update
the isCompletionCommand function to also consider "__completeNoDesc" (add that
string to the check) so calls like "__completeNoDesc" return true and skip
setupUpdateNotice, ensuring completion paths are properly exempted.
In `@internal/update/update.go`:
- Around line 215-232: IsNewer currently uses ParseVersion which strips
prerelease metadata, so prerelease ordering is lost; update ParseVersion (or
create a new parse that returns major, minor, patch plus a prerelease identifier
slice/string) and change IsNewer to compare semver correctly: compare
major/minor/patch first, then handle prerelease per semver rules (no prerelease
> any prerelease; if both have prerelease, split by '.' and compare identifiers
numerically when numeric and lexically otherwise, shorter equal-prefix
prerelease has lower precedence). Apply the same change to the related
comparison logic around the other block flagged (lines ~235-254) and add tests
for IsNewer("1.2.3","1.2.3-rc.1"), IsNewer("1.2.3-rc.2","1.2.3-rc.1"), and GA vs
prerelease cases to cover the new behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f910021e-64f0-4da9-8e2e-9130af5268ce
📒 Files selected for processing (11)
cmd/doctor/doctor.gocmd/root.gointernal/output/envelope.gointernal/output/errors.gointernal/output/errors_test.gointernal/output/print.gointernal/output/print_test.gointernal/update/update.gointernal/update/update_test.goshortcuts/common/runner.goskills/lark-shared/SKILL.md
✅ Files skipped from review due to trivial changes (2)
- skills/lark-shared/SKILL.md
- internal/output/print_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- shortcuts/common/runner.go
- internal/output/print.go
- internal/output/errors.go
- internal/output/errors_test.go
- internal/update/update_test.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cmd/root.go (1)
163-170:⚠️ Potential issue | 🟡 MinorHandle Cobra's
__completeNoDescpath too.Cobra's shell completion can also go through
__completeNoDesc(completions without descriptions). These invocations currently still runsetupUpdateNotice(), which may start the refresh goroutine and potentially corrupt machine-parseable completion output.🐛 Proposed fix
func isCompletionCommand(args []string) bool { for _, arg := range args { - if arg == "completion" || arg == "__complete" { + if arg == "completion" || arg == "__complete" || arg == "__completeNoDesc" { return true } } return false }In spf13/cobra v1.10.x, does shell completion use a hidden command called `__completeNoDesc` in addition to `__complete`?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/root.go` around lines 163 - 170, The completion check in isCompletionCommand currently only looks for "completion" and "__complete", so add "__completeNoDesc" to that check to detect Cobra's no-description completion path and prevent setupUpdateNotice() from running; update the function isCompletionCommand(args []string) to return true for the "__completeNoDesc" token (alongside "completion" and "__complete") so the CLI will skip starting the refresh goroutine during that invocation.
🧹 Nitpick comments (4)
internal/output/errors_test.go (1)
126-150: UsedeferforPendingNoticerestoration for consistency and safety.Unlike the other tests in this file,
TestGetNoticerestoresPendingNoticevia direct assignment at line 149 rather thandefer. If a future edit adds at.Fatalor causes a panic, the global won't be restored.♻️ Proposed fix for consistent cleanup
func TestGetNotice(t *testing.T) { // Nil PendingNotice → nil origNotice := PendingNotice + defer func() { PendingNotice = origNotice }() PendingNotice = nil if got := GetNotice(); got != nil { t.Errorf("expected nil, got %v", got) } // With PendingNotice → returns value PendingNotice = func() map[string]interface{} { return map[string]interface{}{"update": "test"} } got := GetNotice() if got == nil || got["update"] != "test" { t.Errorf("expected {update: test}, got %v", got) } // PendingNotice returns nil → nil PendingNotice = func() map[string]interface{} { return nil } if got := GetNotice(); got != nil { t.Errorf("expected nil, got %v", got) } - - PendingNotice = origNotice }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/output/errors_test.go` around lines 126 - 150, TestGetNotice mutates the global PendingNotice and restores it only at the end, which is unsafe; capture the original PendingNotice into a local (origNotice) then immediately call defer to restore it (defer func() { PendingNotice = origNotice }()) so the global is always reset even if the test fails or panics; locate this in TestGetNotice where PendingNotice is assigned and replace the final direct assignment with a deferred restore.internal/update/update_test.go (3)
23-31: Add a brief comment explaining the dual-call pattern.The
t.Setenv(key, "")followed byos.Unsetenv(key)is intentional:t.Setenvregisters cleanup to restore original values, whileos.Unsetenvactually removes the variable (sinceshouldSkipchecks!= ""which would be true for empty strings). A brief comment would help future maintainers understand this pattern.📝 Suggested clarification
// clearSkipEnv unsets all env vars that shouldSkip checks, // preventing the host environment (e.g. CI=true) from polluting test results. func clearSkipEnv(t *testing.T) { t.Helper() for _, key := range []string{"LARKSUITE_CLI_NO_UPDATE_NOTIFIER", "CI", "BUILD_NUMBER", "RUN_ID"} { + // t.Setenv registers cleanup to restore original value after test. + // os.Unsetenv actually removes the var (empty string != unset for shouldSkip). t.Setenv(key, "") os.Unsetenv(key) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/update/update_test.go` around lines 23 - 31, Add a short clarifying comment above the dual-call pattern in clearSkipEnv explaining why both t.Setenv(key, "") and os.Unsetenv(key) are used: t.Setenv registers a test cleanup to restore the original environment value, while os.Unsetenv actually removes the variable so shouldSkip (which checks for non-empty values) sees it as unset; reference the function name clearSkipEnv and the calls t.Setenv and os.Unsetenv in the comment so future maintainers understand the intentional pattern.
203-235: GlobalDefaultClientmutation is acceptable but document the isolation assumption.The test directly mutates the package-level
DefaultClientvariable. This is safe because:
- Go test functions run sequentially by default within a package
- The
deferproperly restoresnilafter the testHowever, if
t.Parallel()is ever added to these tests, this would cause race conditions. Consider adding a brief comment noting this constraint.📝 Optional: Add isolation note
func TestRefreshCache(t *testing.T) { clearSkipEnv(t) tmp := t.TempDir() t.Setenv("LARKSUITE_CLI_CONFIG_DIR", tmp) - // Set up mock npm registry via DefaultClient + // Set up mock npm registry via DefaultClient. + // Note: This test mutates a package-level var; do NOT use t.Parallel(). srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/update/update_test.go` around lines 203 - 235, Add a brief comment in TestRefreshCache (near where DefaultClient is mutated and restored) noting that the test mutates the package-level DefaultClient and therefore relies on tests running sequentially (i.e., must not call t.Parallel()), and that the defer restoring DefaultClient to nil is used to avoid cross-test leakage; reference the unique symbols DefaultClient, roundTripFunc, and RefreshCache so reviewers can find the mutation site quickly. Ensure the comment explicitly warns future contributors about potential race conditions if t.Parallel() is added and suggests using isolated clients or t.Cleanup for parallel-safe tests.
237-253: Test depends on initial nil state from test execution order.Line 239 asserts
GetPending()is initially nil, which assumes no prior test has set it. While the cleanup at line 252 is good practice, the initial assertion is fragile. Consider resetting state at the start:💡 Optional: Reset state at start for robustness
func TestPendingAtomicAccess(t *testing.T) { + // Reset to known state in case prior test left pending set + SetPending(nil) + // Initially nil if got := GetPending(); got != nil { t.Errorf("expected nil, got %+v", got) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/update/update_test.go` around lines 237 - 253, The test TestPendingAtomicAccess assumes global state is nil at start; make it robust by explicitly resetting the shared state before assertions — call SetPending(nil) at the beginning of TestPendingAtomicAccess (before the initial GetPending check) so GetPending() is guaranteed to be nil, then proceed with creating info and calling SetPending(info) and the existing cleanup SetPending(nil) at the end; reference functions GetPending and SetPending in the test to locate where to add the initial reset.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmd/root.go`:
- Around line 163-170: The completion check in isCompletionCommand currently
only looks for "completion" and "__complete", so add "__completeNoDesc" to that
check to detect Cobra's no-description completion path and prevent
setupUpdateNotice() from running; update the function isCompletionCommand(args
[]string) to return true for the "__completeNoDesc" token (alongside
"completion" and "__complete") so the CLI will skip starting the refresh
goroutine during that invocation.
---
Nitpick comments:
In `@internal/output/errors_test.go`:
- Around line 126-150: TestGetNotice mutates the global PendingNotice and
restores it only at the end, which is unsafe; capture the original PendingNotice
into a local (origNotice) then immediately call defer to restore it (defer
func() { PendingNotice = origNotice }()) so the global is always reset even if
the test fails or panics; locate this in TestGetNotice where PendingNotice is
assigned and replace the final direct assignment with a deferred restore.
In `@internal/update/update_test.go`:
- Around line 23-31: Add a short clarifying comment above the dual-call pattern
in clearSkipEnv explaining why both t.Setenv(key, "") and os.Unsetenv(key) are
used: t.Setenv registers a test cleanup to restore the original environment
value, while os.Unsetenv actually removes the variable so shouldSkip (which
checks for non-empty values) sees it as unset; reference the function name
clearSkipEnv and the calls t.Setenv and os.Unsetenv in the comment so future
maintainers understand the intentional pattern.
- Around line 203-235: Add a brief comment in TestRefreshCache (near where
DefaultClient is mutated and restored) noting that the test mutates the
package-level DefaultClient and therefore relies on tests running sequentially
(i.e., must not call t.Parallel()), and that the defer restoring DefaultClient
to nil is used to avoid cross-test leakage; reference the unique symbols
DefaultClient, roundTripFunc, and RefreshCache so reviewers can find the
mutation site quickly. Ensure the comment explicitly warns future contributors
about potential race conditions if t.Parallel() is added and suggests using
isolated clients or t.Cleanup for parallel-safe tests.
- Around line 237-253: The test TestPendingAtomicAccess assumes global state is
nil at start; make it robust by explicitly resetting the shared state before
assertions — call SetPending(nil) at the beginning of TestPendingAtomicAccess
(before the initial GetPending check) so GetPending() is guaranteed to be nil,
then proceed with creating info and calling SetPending(info) and the existing
cleanup SetPending(nil) at the end; reference functions GetPending and
SetPending in the test to locate where to add the initial reset.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ffca7142-1add-44e1-9f11-e5e2a89186b9
📒 Files selected for processing (11)
cmd/doctor/doctor.gocmd/root.gointernal/output/envelope.gointernal/output/errors.gointernal/output/errors_test.gointernal/output/print.gointernal/output/print_test.gointernal/update/update.gointernal/update/update_test.goshortcuts/common/runner.goskills/lark-shared/SKILL.md
✅ Files skipped from review due to trivial changes (1)
- skills/lark-shared/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (6)
- shortcuts/common/runner.go
- internal/output/errors.go
- internal/output/envelope.go
- internal/output/print.go
- cmd/doctor/doctor.go
- internal/update/update.go
Add non-blocking update check that queries the npm registry for the latest @larksuite/cli version. Results are cached locally (24h TTL) to avoid repeated network requests. When a newer version is detected, a `_notice.update` field is injected into all JSON output envelopes (success, error, and shortcut responses), enabling AI agents and scripts to surface upgrade prompts. Key changes: - New `internal/update` package: registry fetch, semver compare, cache - Async check in root command (cache-first, then background refresh) - `_notice` field added to Envelope/ErrorEnvelope structs - `PrintJson` injects notice into map-based envelopes with "ok" key - `doctor` command gains cli_version and cli_update checks - Suppressed for CI, DEV builds, shell completion, and git-describe versions Change-Id: Ifab77f5eae4539ae07a1e31e6baf8d83ee574bed Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
MaxHuang22
force-pushed
the
feat/auto_upgrade_detect
branch
from
73733f5 to
c85530c
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
cmd/root.go (1)
163-166:⚠️ Potential issue | 🟡 MinorInclude
__completeNoDescin completion detection.Completion requests can still pass through
__completeNoDesc, so update setup is not fully suppressed on completion paths.🔧 Minimal fix
- if arg == "completion" || arg == "__complete" { + if arg == "completion" || arg == "__complete" || arg == "__completeNoDesc" { return true }In spf13/cobra v1.10.2, does shell completion invoke "__completeNoDesc" in addition to "__complete"?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/root.go` around lines 163 - 166, The isCompletionCommand function currently checks for "completion" and "__complete" but misses "__completeNoDesc", allowing completion flows to bypass update suppression; update the check in isCompletionCommand to also treat "__completeNoDesc" as a completion trigger (e.g., add arg == "__completeNoDesc" to the condition) so all completion invocations (including "__completeNoDesc") are detected and handled the same way.internal/update/update.go (1)
215-233:⚠️ Potential issue | 🟠 MajorPrerelease precedence is still dropped in version comparison.
ParseVersionremoves prerelease identifiers, soIsNewercannot correctly handle cases like GA vs RC of the same core version or RC-to-RC ordering. This can suppress or misreport update notices.Also applies to: 237-255
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/update/update.go` around lines 215 - 233, IsNewer currently relies on ParseVersion which strips prerelease identifiers, so implement semver-complete comparison: extend ParseVersion (or add ParseFullVersion) to return the prerelease string (or nil) alongside the [major,minor,patch] ints, then update IsNewer (and the other comparison block at the 237-255 region) to, after numeric equality, handle prerelease precedence per semver—treat absence of a prerelease (GA) as higher precedence than any prerelease, and when both have prerelease strings compare their dot-separated identifiers with numeric parts compared numerically and non-numeric lexicographically to determine ordering. Use the existing function names ParseVersion/IsNewer to locate and replace logic.
🧹 Nitpick comments (1)
cmd/root.go (1)
137-141: Refresh path may keep stale update notice for this run.If cached notice is already set, post-refresh data is ignored even when a newer latest version is fetched. Consider always re-reading cache after
RefreshCacheand updating pending accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/root.go` around lines 137 - 141, The current logic may leave a stale pending update because it only sets pending from cache once; after RefreshCache completes you should re-read the cache and update pending accordingly. Modify the flow around update.GetPending / update.SetPending so that after calling RefreshCache you call update.CheckCached(build.Version) again (or otherwise re-fetch cached info) and call update.SetPending(info) if the returned info is non-nil and represents a newer/latest release; this ensures any post-refresh data replaces an older pending notice even if update.GetPending() was already non-nil earlier.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmd/root.go`:
- Around line 163-166: The isCompletionCommand function currently checks for
"completion" and "__complete" but misses "__completeNoDesc", allowing completion
flows to bypass update suppression; update the check in isCompletionCommand to
also treat "__completeNoDesc" as a completion trigger (e.g., add arg ==
"__completeNoDesc" to the condition) so all completion invocations (including
"__completeNoDesc") are detected and handled the same way.
In `@internal/update/update.go`:
- Around line 215-233: IsNewer currently relies on ParseVersion which strips
prerelease identifiers, so implement semver-complete comparison: extend
ParseVersion (or add ParseFullVersion) to return the prerelease string (or nil)
alongside the [major,minor,patch] ints, then update IsNewer (and the other
comparison block at the 237-255 region) to, after numeric equality, handle
prerelease precedence per semver—treat absence of a prerelease (GA) as higher
precedence than any prerelease, and when both have prerelease strings compare
their dot-separated identifiers with numeric parts compared numerically and
non-numeric lexicographically to determine ordering. Use the existing function
names ParseVersion/IsNewer to locate and replace logic.
---
Nitpick comments:
In `@cmd/root.go`:
- Around line 137-141: The current logic may leave a stale pending update
because it only sets pending from cache once; after RefreshCache completes you
should re-read the cache and update pending accordingly. Modify the flow around
update.GetPending / update.SetPending so that after calling RefreshCache you
call update.CheckCached(build.Version) again (or otherwise re-fetch cached info)
and call update.SetPending(info) if the returned info is non-nil and represents
a newer/latest release; this ensures any post-refresh data replaces an older
pending notice even if update.GetPending() was already non-nil earlier.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 396d67cd-ac36-4215-9860-e12c1a86139b
📒 Files selected for processing (11)
cmd/doctor/doctor.gocmd/root.gointernal/output/envelope.gointernal/output/errors.gointernal/output/errors_test.gointernal/output/print.gointernal/output/print_test.gointernal/update/update.gointernal/update/update_test.goshortcuts/common/runner.goskills/lark-shared/SKILL.md
✅ Files skipped from review due to trivial changes (3)
- skills/lark-shared/SKILL.md
- internal/output/print_test.go
- internal/output/errors_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- shortcuts/common/runner.go
- internal/output/print.go
- cmd/doctor/doctor.go
- internal/output/envelope.go
liangshuo-1
approved these changes
Mar 31, 2026
tuxedomm
pushed a commit
that referenced
this pull request
Apr 3, 2026
Add non-blocking update check that queries the npm registry for the latest @larksuite/cli version. Results are cached locally (24h TTL) to avoid repeated network requests. When a newer version is detected, a `_notice.update` field is injected into all JSON output envelopes (success, error, and shortcut responses), enabling AI agents and scripts to surface upgrade prompts. Key changes: - New `internal/update` package: registry fetch, semver compare, cache - Async check in root command (cache-first, then background refresh) - `_notice` field added to Envelope/ErrorEnvelope structs - `PrintJson` injects notice into map-based envelopes with "ok" key - `doctor` command gains cli_version and cli_update checks - Suppressed for CI, DEV builds, shell completion, and git-describe versions
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
3 participants
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.
Summary
Add non-blocking update check that queries the npm registry for the latest
@larksuite/cli version, caches results locally (24h TTL), and injects a
_notice.updatefield into JSON output envelopes when a newer version is available.Changes
internal/updatepackage: npm registry fetch, semver compare, local cache_noticefield added toEnvelope/ErrorEnvelopestructs (omitempty)PrintJsoninjects notice into map-based envelopes with"ok"keyErrorEnvelope(previously early-return) to carry noticedoctorcommand gainscli_versionandcli_updatecheckslark-shared/SKILL.mdto surface update promptsTest Plan
internal/update: IsNewer, ParseVersion, shouldSkip, isRelease, CheckCached, RefreshCache, Pending atomic accessinternal/output: PrintJson notice injection (map envelope, non-envelope, struct, nil notice), WriteErrorEnvelope with/without notice, GetNoticego test ./internal/update/ ./internal/output/ ./cmd/all passgolangci-lint run --new-from-rev=origin/main— 0 issuesSummary by CodeRabbit
New Features
Tests
Documentation