diff --git a/AGENTS.md b/AGENTS.md index b23920650..559b4baa3 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -15,6 +15,22 @@ make unit-test # Required before PR (runs with -race) make test # Full: vet + unit + integration ``` +## Notification Opt-Outs + +`lark-cli` emits two notice types into JSON envelope `_notice` to nudge AI agents toward fixes: + +- `_notice.update` — a newer binary is available on npm +- `_notice.skills` — locally installed skills are out of sync with the running binary + +To suppress them in non-CI scripts (CI envs are auto-skipped): + +| Env var | Effect | +|---------|--------| +| `LARKSUITE_CLI_NO_UPDATE_NOTIFIER=1` | Suppress `_notice.update` | +| `LARKSUITE_CLI_NO_SKILLS_NOTIFIER=1` | Suppress `_notice.skills` | + +Both notices recommend the same fix command: `lark-cli update`. The skills notice's `current` field is `""` when skills have never been synced (cold start) and a version string when synced for an older binary (drift). + ## Pre-PR Checks (match CI gates) 1. `make unit-test` diff --git a/cmd/root.go b/cmd/root.go index 06d3f7eed..a65e5f737 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -20,6 +20,7 @@ import ( "github.com/larksuite/cli/internal/core" "github.com/larksuite/cli/internal/output" "github.com/larksuite/cli/internal/registry" + "github.com/larksuite/cli/internal/skillscheck" "github.com/larksuite/cli/internal/update" "github.com/spf13/cobra" ) @@ -93,9 +94,9 @@ func Execute() int { HideProfile(isSingleAppMode()), ) - // --- Update check (non-blocking) --- + // --- Notices (non-blocking) --- if !isCompletionCommand(os.Args) { - setupUpdateNotice() + setupNotices() } if err := rootCmd.Execute(); err != nil { @@ -104,42 +105,54 @@ func Execute() int { return 0 } -// setupUpdateNotice starts an async update check and wires the output decorator. -func setupUpdateNotice() { - // Sync: check cache immediately (no network, fast). +// setupNotices wires both the binary update notice and the skills +// staleness notice into output.PendingNotice as a composed function. +// Each provider populates an independent key under _notice; either +// or both may be present in any given envelope. +func setupNotices() { + // Binary update — synchronous cache check + async refresh if info := update.CheckCached(build.Version); info != nil { update.SetPending(info) } - - // Async: refresh cache for this run (and future runs). + ver := build.Version go func() { defer func() { if r := recover(); r != nil { fmt.Fprintf(os.Stderr, "update check panic: %v\n", r) } }() - update.RefreshCache(build.Version) - // If cache was just populated for the first time, set pending now. + update.RefreshCache(ver) if update.GetPending() == nil { - if info := update.CheckCached(build.Version); info != nil { + if info := update.CheckCached(ver); info != nil { update.SetPending(info) } } }() - // Wire the output decorator so JSON envelopes include "_notice". + // Skills check — synchronous, local-only (no network, no goroutine). + skillscheck.Init(build.Version) + + // Composed notice provider — emits keys only when each pending is set. output.PendingNotice = func() map[string]interface{} { - info := update.GetPending() - if info == nil { - return nil - } - return map[string]interface{}{ - "update": map[string]interface{}{ + notice := map[string]interface{}{} + if info := update.GetPending(); info != nil { + notice["update"] = map[string]interface{}{ "current": info.Current, "latest": info.Latest, "message": info.Message(), - }, + } + } + if stale := skillscheck.GetPending(); stale != nil { + notice["skills"] = map[string]interface{}{ + "current": stale.Current, + "target": stale.Target, + "message": stale.Message(), + } + } + if len(notice) == 0 { + return nil } + return notice } } diff --git a/cmd/root_integration_test.go b/cmd/root_integration_test.go index c5c6a1d75..7e8119a08 100644 --- a/cmd/root_integration_test.go +++ b/cmd/root_integration_test.go @@ -7,6 +7,7 @@ import ( "bytes" "context" "encoding/json" + "os" "reflect" "strings" "testing" @@ -14,11 +15,14 @@ import ( "github.com/larksuite/cli/cmd/api" "github.com/larksuite/cli/cmd/auth" "github.com/larksuite/cli/cmd/service" + "github.com/larksuite/cli/internal/build" "github.com/larksuite/cli/internal/cmdutil" "github.com/larksuite/cli/internal/core" "github.com/larksuite/cli/internal/envvars" "github.com/larksuite/cli/internal/httpmock" "github.com/larksuite/cli/internal/output" + "github.com/larksuite/cli/internal/skillscheck" + "github.com/larksuite/cli/internal/update" "github.com/larksuite/cli/shortcuts" "github.com/spf13/cobra" ) @@ -490,3 +494,181 @@ func TestIntegration_Shortcut_BusinessError_OutputsEnvelope(t *testing.T) { }, }) } + +// TestSetupNotices_ColdStart verifies that when no skills stamp exists, +// the composed PendingNotice provider includes a "skills" key with an +// empty Current and the cold-start message. +func TestSetupNotices_ColdStart(t *testing.T) { + clearNoticeEnv(t) + dir := t.TempDir() + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", dir) + + origVersion := build.Version + build.Version = "1.0.21" + t.Cleanup(func() { build.Version = origVersion }) + + // Reset pending state to ensure a clean test. + skillscheck.SetPending(nil) + update.SetPending(nil) + output.PendingNotice = nil + t.Cleanup(func() { + skillscheck.SetPending(nil) + update.SetPending(nil) + output.PendingNotice = nil + }) + + setupNotices() + + notice := output.GetNotice() + if notice == nil { + t.Fatal("GetNotice() = nil, want non-nil for cold start") + } + skills, ok := notice["skills"].(map[string]interface{}) + if !ok { + t.Fatalf("notice.skills missing, got %+v", notice) + } + if skills["current"] != "" || skills["target"] != "1.0.21" { + t.Errorf("notice.skills = %+v, want {current:\"\", target:\"1.0.21\"}", skills) + } + if msg, _ := skills["message"].(string); msg != "lark-cli skills not installed, run: lark-cli update" { + t.Errorf("notice.skills.message = %q, want cold-start message", msg) + } +} + +// TestSetupNotices_InSync verifies that a matching stamp produces no +// skills key in the composed notice. +func TestSetupNotices_InSync(t *testing.T) { + clearNoticeEnv(t) + dir := t.TempDir() + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", dir) + if err := skillscheck.WriteStamp("1.0.21"); err != nil { + t.Fatal(err) + } + + origVersion := build.Version + build.Version = "1.0.21" + t.Cleanup(func() { build.Version = origVersion }) + + skillscheck.SetPending(nil) + update.SetPending(nil) + output.PendingNotice = nil + t.Cleanup(func() { + skillscheck.SetPending(nil) + update.SetPending(nil) + output.PendingNotice = nil + }) + + setupNotices() + + notice := output.GetNotice() + if notice != nil { + if _, ok := notice["skills"]; ok { + t.Errorf("notice.skills present in in-sync state: %+v", notice) + } + } +} + +// TestSetupNotices_Drift verifies a mismatching stamp produces the +// drift message with both current and target populated. +func TestSetupNotices_Drift(t *testing.T) { + clearNoticeEnv(t) + dir := t.TempDir() + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", dir) + if err := skillscheck.WriteStamp("1.0.20"); err != nil { + t.Fatal(err) + } + + origVersion := build.Version + build.Version = "1.0.21" + t.Cleanup(func() { build.Version = origVersion }) + + skillscheck.SetPending(nil) + update.SetPending(nil) + output.PendingNotice = nil + t.Cleanup(func() { + skillscheck.SetPending(nil) + update.SetPending(nil) + output.PendingNotice = nil + }) + + setupNotices() + + notice := output.GetNotice() + if notice == nil { + t.Fatal("GetNotice() = nil, want non-nil for drift") + } + skills, ok := notice["skills"].(map[string]interface{}) + if !ok { + t.Fatalf("notice.skills missing, got %+v", notice) + } + if skills["current"] != "1.0.20" || skills["target"] != "1.0.21" { + t.Errorf("notice.skills = %+v, want {current:\"1.0.20\", target:\"1.0.21\"}", skills) + } + want := "lark-cli skills 1.0.20 out of sync with binary 1.0.21, run: lark-cli update" + if msg, _ := skills["message"].(string); msg != want { + t.Errorf("notice.skills.message = %q, want %q", msg, want) + } +} + +// TestSetupNotices_BothUpdateAndSkills verifies the composed envelope +// emits BOTH "_notice.update" and "_notice.skills" keys when each +// pending value is set. Drives the skills key via setupNotices() (drift +// state) and manually populates the update pending afterwards, since +// clearNoticeEnv suppresses the update goroutine to avoid network +// flakiness. +func TestSetupNotices_BothUpdateAndSkills(t *testing.T) { + clearNoticeEnv(t) + dir := t.TempDir() + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", dir) + if err := skillscheck.WriteStamp("1.0.20"); err != nil { + t.Fatal(err) + } + + origVersion := build.Version + build.Version = "1.0.21" + t.Cleanup(func() { build.Version = origVersion }) + + skillscheck.SetPending(nil) + update.SetPending(nil) + output.PendingNotice = nil + t.Cleanup(func() { + skillscheck.SetPending(nil) + update.SetPending(nil) + output.PendingNotice = nil + }) + + setupNotices() + + // After setupNotices, skills pending is set (drift). Manually populate + // the update side so the composed envelope has both keys — the update + // goroutine is suppressed by clearNoticeEnv. + update.SetPending(&update.UpdateInfo{Current: "1.0.21", Latest: "1.0.22"}) + + notice := output.GetNotice() + if notice == nil { + t.Fatal("GetNotice() = nil, want both keys") + } + if _, ok := notice["update"].(map[string]interface{}); !ok { + t.Errorf("missing 'update' key: %+v", notice) + } + if _, ok := notice["skills"].(map[string]interface{}); !ok { + t.Errorf("missing 'skills' key: %+v", notice) + } +} + +// clearNoticeEnv unsets the env vars that affect either notice. We +// proactively SUPPRESS the update notifier (LARKSUITE_CLI_NO_UPDATE_NOTIFIER=1) +// because setupNotices spawns a goroutine that hits the npm registry — +// tests focused on the skills check should not depend on network state. +func clearNoticeEnv(t *testing.T) { + t.Helper() + for _, key := range []string{ + "LARKSUITE_CLI_NO_SKILLS_NOTIFIER", + "CI", "BUILD_NUMBER", "RUN_ID", + } { + t.Setenv(key, "") + os.Unsetenv(key) + } + // Suppress the update goroutine's network call deterministically. + t.Setenv("LARKSUITE_CLI_NO_UPDATE_NOTIFIER", "1") +} diff --git a/cmd/update/update.go b/cmd/update/update.go index 182ef57db..0a4679797 100644 --- a/cmd/update/update.go +++ b/cmd/update/update.go @@ -14,13 +14,15 @@ import ( "github.com/larksuite/cli/internal/cmdutil" "github.com/larksuite/cli/internal/output" "github.com/larksuite/cli/internal/selfupdate" + "github.com/larksuite/cli/internal/skillscheck" "github.com/larksuite/cli/internal/update" ) const ( - repoURL = "https://github.com/larksuite/cli" - maxNpmOutput = 2000 - osWindows = "windows" + repoURL = "https://github.com/larksuite/cli" + maxNpmOutput = 2000 + maxStderrDetail = 500 + osWindows = "windows" ) // Overridable for testing. @@ -33,6 +35,13 @@ var ( func isWindows() bool { return currentOS == osWindows } +// normalizeVersion canonicalizes a version string for stamp comparison. +// Strips a leading "v" so versions written from Makefile (git describe → +// "v1.0.0") and npm (no prefix → "1.0.0") compare equal. +func normalizeVersion(s string) string { + return strings.TrimPrefix(strings.TrimSpace(s), "v") +} + func releaseURL(version string) string { return repoURL + "/releases/tag/v" + strings.TrimPrefix(version, "v") } @@ -127,16 +136,15 @@ func updateRun(opts *UpdateOptions) error { // 3. Compare versions if !opts.Force && !update.IsNewer(latest, cur) { - if opts.JSON { - output.PrintJson(io.Out, map[string]interface{}{ - "ok": true, "previous_version": cur, "current_version": cur, - "latest_version": latest, "action": "already_up_to_date", - "message": fmt.Sprintf("lark-cli %s is already up to date", cur), - }) - return nil + // Run skills sync before returning — covers the case where the + // binary is already current but skills were never synced. + // Stamp dedup makes this a no-op if skills are already in sync. + // Skip side-effects under --check (pure report path per spec §3.6). + var skillsResult *selfupdate.NpmResult + if !opts.Check { + skillsResult = runSkillsAndStamp(updater, io, cur, opts.Force) } - fmt.Fprintf(io.ErrOut, "%s lark-cli %s is already up to date\n", symOK(), cur) - return nil + return reportAlreadyUpToDate(opts, io, cur, latest, skillsResult, opts.Check) } // 4. Detect installation method @@ -149,7 +157,7 @@ func updateRun(opts *UpdateOptions) error { // 6. Execute update if !detect.CanAutoUpdate() { - return doManualUpdate(opts, io, cur, latest, detect) + return doManualUpdate(opts, io, cur, latest, detect, updater) } return doNpmUpdate(opts, io, cur, latest, updater) } @@ -169,13 +177,24 @@ func reportError(opts *UpdateOptions, io *cmdutil.IOStreams, exitCode int, errTy func reportCheckResult(opts *UpdateOptions, io *cmdutil.IOStreams, cur, latest string, canAutoUpdate bool) error { if opts.JSON { - output.PrintJson(io.Out, map[string]interface{}{ + out := map[string]interface{}{ "ok": true, "previous_version": cur, "current_version": cur, "latest_version": latest, "action": "update_available", "auto_update": canAutoUpdate, "message": fmt.Sprintf("lark-cli %s %s %s available", cur, symArrow(), latest), "url": releaseURL(latest), "changelog": changelogURL(), - }) + } + // skills_status: pure report, no side effect, no stamp write. + // ReadStamp errors are silently swallowed — if we can't read the + // stamp we just omit the block rather than fail the --check. + if stamp, err := skillscheck.ReadStamp(); err == nil { + out["skills_status"] = map[string]interface{}{ + "current": stamp, + "target": cur, + "in_sync": stamp == cur, + } + } + output.PrintJson(io.Out, out) return nil } fmt.Fprintf(io.ErrOut, "Update available: %s %s %s\n", cur, symArrow(), latest) @@ -189,15 +208,19 @@ func reportCheckResult(opts *UpdateOptions, io *cmdutil.IOStreams, cur, latest s return nil } -func doManualUpdate(opts *UpdateOptions, io *cmdutil.IOStreams, cur, latest string, detect selfupdate.DetectResult) error { +func doManualUpdate(opts *UpdateOptions, io *cmdutil.IOStreams, cur, latest string, detect selfupdate.DetectResult, updater *selfupdate.Updater) error { + skillsResult := runSkillsAndStamp(updater, io, cur, opts.Force) + reason := detect.ManualReason() if opts.JSON { - output.PrintJson(io.Out, map[string]interface{}{ + out := map[string]interface{}{ "ok": true, "previous_version": cur, "latest_version": latest, "action": "manual_required", "message": fmt.Sprintf("Automatic update unavailable: %s (path: %s)", reason, detect.ResolvedPath), "url": releaseURL(latest), "changelog": changelogURL(), - }) + } + applySkillsResult(out, skillsResult) + output.PrintJson(io.Out, out) return nil } fmt.Fprintf(io.ErrOut, "Automatic update unavailable: %s (path: %s).\n\n", reason, detect.ResolvedPath) @@ -205,7 +228,7 @@ func doManualUpdate(opts *UpdateOptions, io *cmdutil.IOStreams, cur, latest stri fmt.Fprintf(io.ErrOut, " Release: %s\n", releaseURL(latest)) fmt.Fprintf(io.ErrOut, " Changelog: %s\n", changelogURL()) fmt.Fprintf(io.ErrOut, "\nOr install via npm:\n npm install -g %s@%s\n", selfupdate.NpmPackage, latest) - fmt.Fprintf(io.ErrOut, "\nAfter updating, also update skills:\n npx -y skills add larksuite/cli -g -y\n") + emitSkillsTextHints(io, skillsResult) return nil } @@ -264,8 +287,10 @@ func doNpmUpdate(opts *UpdateOptions, io *cmdutil.IOStreams, cur, latest string, return output.ErrBare(output.ExitAPI) } - // Skills update (best-effort). - skillsResult := updater.RunSkillsUpdate() + // Skills update (best-effort) — uses runSkillsAndStamp so the + // stamp gets persisted on success and dedup applies if a previous + // run already stamped this version. + skillsResult := runSkillsAndStamp(updater, io, latest, opts.Force) if opts.JSON { result := map[string]interface{}{ @@ -274,28 +299,17 @@ func doNpmUpdate(opts *UpdateOptions, io *cmdutil.IOStreams, cur, latest string, "message": fmt.Sprintf("lark-cli updated from %s to %s", cur, latest), "url": releaseURL(latest), "changelog": changelogURL(), } - if skillsResult.Err != nil { - result["skills_warning"] = fmt.Sprintf("skills update failed: %s", skillsResult.Err) - if detail := strings.TrimSpace(skillsResult.Stderr.String()); detail != "" { - result["skills_detail"] = selfupdate.Truncate(detail, maxNpmOutput) - } - } + applySkillsResult(result, skillsResult) output.PrintJson(io.Out, result) return nil } fmt.Fprintf(io.ErrOut, "\n%s Successfully updated lark-cli from %s to %s\n", symOK(), cur, latest) fmt.Fprintf(io.ErrOut, " Changelog: %s\n", changelogURL()) - fmt.Fprintf(io.ErrOut, "\nUpdating skills ...\n") - if skillsResult.Err != nil { - fmt.Fprintf(io.ErrOut, "%s Skills update failed: %s\n", symWarn(), skillsResult.Err) - if detail := strings.TrimSpace(skillsResult.Stderr.String()); detail != "" { - fmt.Fprintf(io.ErrOut, " %s\n", selfupdate.Truncate(detail, 500)) - } - fmt.Fprintf(io.ErrOut, " Run manually: npx -y skills add larksuite/cli -g -y\n") - } else { - fmt.Fprintf(io.ErrOut, "%s Skills updated\n", symOK()) + if skillsResult != nil { + fmt.Fprintf(io.ErrOut, "\nUpdating skills ...\n") } + emitSkillsTextHints(io, skillsResult) return nil } @@ -312,3 +326,96 @@ func verificationFailureHint(updater *selfupdate.Updater, latest string) string } return fmt.Sprintf("automatic rollback is unavailable on this platform; reinstall manually: npm install -g %s@%s, or download %s", selfupdate.NpmPackage, latest, releaseURL(latest)) } + +// runSkillsAndStamp triggers updater.RunSkillsUpdate and persists the +// stamp on success. Skips the npx invocation when the stamp already +// matches stampVersion (unless force is true). The stamp write failure +// emits a warning to io.ErrOut but does NOT fail the update command — +// best-effort. ReadStamp errors are swallowed (fail-closed: treated as +// out-of-sync, so npx re-runs). Returns nil iff skipped due to stamp +// dedup; otherwise returns the underlying *NpmResult with Err semantics +// from RunSkillsUpdate. +func runSkillsAndStamp(updater *selfupdate.Updater, io *cmdutil.IOStreams, stampVersion string, force bool) *selfupdate.NpmResult { + if !force { + if existing, _ := skillscheck.ReadStamp(); normalizeVersion(existing) == normalizeVersion(stampVersion) { + return nil + } + } + r := updater.RunSkillsUpdate() + if r.Err == nil { + if err := skillscheck.WriteStamp(stampVersion); err != nil { + fmt.Fprintf(io.ErrOut, "warning: skills synced but stamp not written: %v\n", err) + } + } + return r +} + +// reportAlreadyUpToDate emits the JSON / pretty output for the +// already-up-to-date branch, including any skills_action / skills_warning +// fields derived from skillsResult. When check is true, this is the pure +// report path (spec §3.6): no side-effects, JSON envelope uses +// skills_status (spec §4.2) instead of skills_action. +func reportAlreadyUpToDate(opts *UpdateOptions, io *cmdutil.IOStreams, cur, latest string, skillsResult *selfupdate.NpmResult, check bool) error { + if opts.JSON { + out := map[string]interface{}{ + "ok": true, "previous_version": cur, "current_version": cur, + "latest_version": latest, "action": "already_up_to_date", + "message": fmt.Sprintf("lark-cli %s is already up to date", cur), + } + if check { + // Pure report — read stamp directly, emit skills_status block. + // ReadStamp errors are silently swallowed — if we can't read + // the stamp we just omit the block rather than fail the --check. + if stamp, err := skillscheck.ReadStamp(); err == nil { + out["skills_status"] = map[string]interface{}{ + "current": stamp, + "target": cur, + "in_sync": stamp == cur, + } + } + } else { + applySkillsResult(out, skillsResult) + } + output.PrintJson(io.Out, out) + return nil + } + fmt.Fprintf(io.ErrOut, "%s lark-cli %s is already up to date\n", symOK(), cur) + if !check { + emitSkillsTextHints(io, skillsResult) + } + return nil +} + +// applySkillsResult mutates the JSON envelope to include skills_action +// (and skills_warning when failed). nil result = "in_sync" (dedup hit). +func applySkillsResult(env map[string]interface{}, r *selfupdate.NpmResult) { + switch { + case r == nil: + env["skills_action"] = "in_sync" + case r.Err != nil: + env["skills_action"] = "failed" + env["skills_warning"] = fmt.Sprintf("skills update failed: %s", r.Err) + if detail := strings.TrimSpace(r.Stderr.String()); detail != "" { + env["skills_detail"] = selfupdate.Truncate(detail, maxNpmOutput) + } + default: + env["skills_action"] = "synced" + } +} + +// emitSkillsTextHints prints human-readable feedback about the skills +// sync result for non-JSON output. +func emitSkillsTextHints(io *cmdutil.IOStreams, r *selfupdate.NpmResult) { + switch { + case r == nil: + // dedup hit — silent (already up to date) + case r.Err != nil: + fmt.Fprintf(io.ErrOut, "%s Skills update failed: %v\n", symWarn(), r.Err) + if detail := strings.TrimSpace(r.Stderr.String()); detail != "" { + fmt.Fprintf(io.ErrOut, " %s\n", selfupdate.Truncate(detail, maxStderrDetail)) + } + fmt.Fprintf(io.ErrOut, " Run manually: npx -y skills add larksuite/cli -g -y\n") + default: + fmt.Fprintf(io.ErrOut, "%s Skills updated\n", symOK()) + } +} diff --git a/cmd/update/update_test.go b/cmd/update/update_test.go index 41abe2961..ae1a35524 100644 --- a/cmd/update/update_test.go +++ b/cmd/update/update_test.go @@ -5,8 +5,11 @@ package cmdupdate import ( "bytes" + "encoding/json" "errors" "fmt" + "os" + "path/filepath" "strings" "testing" @@ -14,6 +17,7 @@ import ( "github.com/larksuite/cli/internal/core" "github.com/larksuite/cli/internal/output" "github.com/larksuite/cli/internal/selfupdate" + "github.com/larksuite/cli/internal/skillscheck" ) // newTestFactory creates a test factory with minimal config. @@ -709,6 +713,7 @@ func TestUpdateWindows_Symbols(t *testing.T) { } func TestUpdateNpm_SkillsSuccess_JSON(t *testing.T) { + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) f, stdout, _ := newTestFactory(t) cmd := NewCmdUpdate(f) cmd.SetArgs([]string{"--json"}) @@ -737,6 +742,7 @@ func TestUpdateNpm_SkillsSuccess_JSON(t *testing.T) { } func TestUpdateNpm_SkillsFail_JSON(t *testing.T) { + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) f, stdout, _ := newTestFactory(t) cmd := NewCmdUpdate(f) cmd.SetArgs([]string{"--json"}) @@ -789,6 +795,7 @@ func TestUpdateNpm_SkillsFail_JSON(t *testing.T) { } func TestUpdateNpm_SkillsFail_Human(t *testing.T) { + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) f, _, stderr := newTestFactory(t) cmd := NewCmdUpdate(f) cmd.SetArgs([]string{}) @@ -836,6 +843,98 @@ func TestUpdateNpm_SkillsFail_Human(t *testing.T) { } } +// newTestIO returns a cmdutil.IOStreams backed by bytes.Buffers, suitable +// for direct calls to internals like runSkillsAndStamp that write to +// io.ErrOut. +func newTestIO() *cmdutil.IOStreams { + return cmdutil.NewIOStreams(&bytes.Buffer{}, &bytes.Buffer{}, &bytes.Buffer{}) +} + +func TestRunSkillsAndStamp_DedupHit(t *testing.T) { + dir := t.TempDir() + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", dir) + if err := skillscheck.WriteStamp("1.0.21"); err != nil { + t.Fatal(err) + } + called := false + updater := &selfupdate.Updater{ + SkillsUpdateOverride: func() *selfupdate.NpmResult { + called = true + return &selfupdate.NpmResult{} + }, + } + got := runSkillsAndStamp(updater, newTestIO(), "1.0.21", false) + if got != nil { + t.Errorf("runSkillsAndStamp() = %+v, want nil for dedup hit", got) + } + if called { + t.Error("SkillsUpdateOverride called, want skipped due to dedup") + } +} + +func TestRunSkillsAndStamp_DedupForceBypass(t *testing.T) { + dir := t.TempDir() + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", dir) + if err := skillscheck.WriteStamp("1.0.21"); err != nil { + t.Fatal(err) + } + called := false + updater := &selfupdate.Updater{ + SkillsUpdateOverride: func() *selfupdate.NpmResult { + called = true + return &selfupdate.NpmResult{} + }, + } + got := runSkillsAndStamp(updater, newTestIO(), "1.0.21", true) + if got == nil { + t.Fatal("runSkillsAndStamp(force=true) = nil, want non-nil") + } + if !called { + t.Error("SkillsUpdateOverride not called with force=true") + } +} + +func TestRunSkillsAndStamp_SuccessWritesStamp(t *testing.T) { + dir := t.TempDir() + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", dir) + updater := &selfupdate.Updater{ + SkillsUpdateOverride: func() *selfupdate.NpmResult { + return &selfupdate.NpmResult{} + }, + } + got := runSkillsAndStamp(updater, newTestIO(), "1.0.21", false) + if got == nil || got.Err != nil { + t.Fatalf("runSkillsAndStamp() = %+v, want non-nil with nil Err", got) + } + stamp, _ := skillscheck.ReadStamp() + if stamp != "1.0.21" { + t.Errorf("stamp = %q, want \"1.0.21\"", stamp) + } +} + +func TestRunSkillsAndStamp_FailureKeepsOldStamp(t *testing.T) { + dir := t.TempDir() + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", dir) + if err := skillscheck.WriteStamp("1.0.20"); err != nil { + t.Fatal(err) + } + updater := &selfupdate.Updater{ + SkillsUpdateOverride: func() *selfupdate.NpmResult { + r := &selfupdate.NpmResult{} + r.Err = fmt.Errorf("npx failed") + return r + }, + } + got := runSkillsAndStamp(updater, newTestIO(), "1.0.21", false) + if got == nil || got.Err == nil { + t.Fatalf("runSkillsAndStamp() = %+v, want non-nil with non-nil Err", got) + } + stamp, _ := skillscheck.ReadStamp() + if stamp != "1.0.20" { + t.Errorf("stamp = %q, want \"1.0.20\" (failure must not overwrite)", stamp) + } +} + func TestTruncate(t *testing.T) { long := strings.Repeat("x", 3000) got := selfupdate.Truncate(long, 2000) @@ -849,3 +948,272 @@ func TestTruncate(t *testing.T) { t.Errorf("expected 'hello', got %q", got2) } } + +func TestUpdateRun_AlreadyLatest_RunsSkillsSync(t *testing.T) { + dir := t.TempDir() + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", dir) + + origFetch := fetchLatest + origCur := currentVersion + t.Cleanup(func() { fetchLatest = origFetch; currentVersion = origCur }) + fetchLatest = func() (string, error) { return "1.0.21", nil } + currentVersion = func() string { return "1.0.21" } + + skillsCalled := false + origNew := newUpdater + t.Cleanup(func() { newUpdater = origNew }) + newUpdater = func() *selfupdate.Updater { + return &selfupdate.Updater{ + SkillsUpdateOverride: func() *selfupdate.NpmResult { + skillsCalled = true + return &selfupdate.NpmResult{} + }, + } + } + + f, _, _ := newTestFactory(t) + opts := &UpdateOptions{Factory: f, JSON: true} + if err := updateRun(opts); err != nil { + t.Fatalf("updateRun() err = %v, want nil", err) + } + if !skillsCalled { + t.Error("RunSkillsUpdate not called in already-up-to-date branch (cold stamp), want called") + } + stamp, _ := skillscheck.ReadStamp() + if stamp != "1.0.21" { + t.Errorf("stamp = %q, want \"1.0.21\"", stamp) + } +} + +func TestUpdateRun_Manual_RunsSkillsSync(t *testing.T) { + dir := t.TempDir() + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", dir) + + origFetch := fetchLatest + origCur := currentVersion + t.Cleanup(func() { fetchLatest = origFetch; currentVersion = origCur }) + fetchLatest = func() (string, error) { return "1.0.22", nil } + currentVersion = func() string { return "1.0.21" } + + skillsCalled := false + origNew := newUpdater + t.Cleanup(func() { newUpdater = origNew }) + newUpdater = func() *selfupdate.Updater { + return &selfupdate.Updater{ + DetectOverride: func() selfupdate.DetectResult { + return selfupdate.DetectResult{ + Method: selfupdate.InstallManual, + ResolvedPath: "/usr/local/bin/lark-cli", + } + }, + SkillsUpdateOverride: func() *selfupdate.NpmResult { + skillsCalled = true + return &selfupdate.NpmResult{} + }, + } + } + + f, _, _ := newTestFactory(t) + opts := &UpdateOptions{Factory: f, JSON: true} + if err := updateRun(opts); err != nil { + t.Fatalf("updateRun() err = %v, want nil", err) + } + if !skillsCalled { + t.Error("RunSkillsUpdate not called in manual branch, want called") + } + stamp, _ := skillscheck.ReadStamp() + if stamp != "1.0.21" { + t.Errorf("stamp = %q, want \"1.0.21\" (manual path stamps cur)", stamp) + } +} + +func TestUpdateRun_Npm_RunsSkillsSync_StampsLatest(t *testing.T) { + dir := t.TempDir() + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", dir) + + origFetch := fetchLatest + origCur := currentVersion + t.Cleanup(func() { fetchLatest = origFetch; currentVersion = origCur }) + fetchLatest = func() (string, error) { return "1.0.22", nil } + currentVersion = func() string { return "1.0.21" } + + skillsCalled := false + origNew := newUpdater + t.Cleanup(func() { newUpdater = origNew }) + newUpdater = func() *selfupdate.Updater { + return &selfupdate.Updater{ + DetectOverride: func() selfupdate.DetectResult { + return selfupdate.DetectResult{ + Method: selfupdate.InstallNpm, NpmAvailable: true, + ResolvedPath: "/usr/local/bin/lark-cli", + } + }, + NpmInstallOverride: func(version string) *selfupdate.NpmResult { + return &selfupdate.NpmResult{} + }, + VerifyOverride: func(expectedVersion string) error { return nil }, + SkillsUpdateOverride: func() *selfupdate.NpmResult { + skillsCalled = true + return &selfupdate.NpmResult{} + }, + } + } + + f, _, _ := newTestFactory(t) + opts := &UpdateOptions{Factory: f, JSON: true} + if err := updateRun(opts); err != nil { + t.Fatalf("updateRun() err = %v, want nil", err) + } + if !skillsCalled { + t.Error("RunSkillsUpdate not called in npm branch") + } + stamp, _ := skillscheck.ReadStamp() + if stamp != "1.0.22" { + t.Errorf("stamp = %q, want \"1.0.22\" (npm path stamps latest)", stamp) + } +} + +func TestUpdateRun_CheckIncludesSkillsStatus(t *testing.T) { + dir := t.TempDir() + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", dir) + if err := skillscheck.WriteStamp("1.0.20"); err != nil { + t.Fatal(err) + } + + origFetch := fetchLatest + origCur := currentVersion + t.Cleanup(func() { fetchLatest = origFetch; currentVersion = origCur }) + fetchLatest = func() (string, error) { return "1.0.22", nil } + currentVersion = func() string { return "1.0.21" } + + origNew := newUpdater + t.Cleanup(func() { newUpdater = origNew }) + skillsCalled := false + newUpdater = func() *selfupdate.Updater { + return &selfupdate.Updater{ + DetectOverride: func() selfupdate.DetectResult { + return selfupdate.DetectResult{Method: selfupdate.InstallNpm, NpmAvailable: true} + }, + SkillsUpdateOverride: func() *selfupdate.NpmResult { + skillsCalled = true + return &selfupdate.NpmResult{} + }, + } + } + + f, stdout, _ := newTestFactory(t) + opts := &UpdateOptions{Factory: f, JSON: true, Check: true} + if err := updateRun(opts); err != nil { + t.Fatalf("updateRun(--check) err = %v, want nil", err) + } + if skillsCalled { + t.Error("RunSkillsUpdate called under --check, want skipped (pure report)") + } + + var env map[string]interface{} + if err := json.Unmarshal(stdout.Bytes(), &env); err != nil { + t.Fatalf("json.Unmarshal stdout: %v\nstdout: %s", err, stdout.String()) + } + status, ok := env["skills_status"].(map[string]interface{}) + if !ok { + t.Fatalf("skills_status missing or wrong type in --check JSON: %s", stdout.String()) + } + if status["current"] != "1.0.20" || status["target"] != "1.0.21" || status["in_sync"] != false { + t.Errorf("skills_status = %+v, want {current:\"1.0.20\", target:\"1.0.21\", in_sync:false}", status) + } +} + +func TestUpdateRun_CheckAlreadyLatest_NoSideEffect(t *testing.T) { + dir := t.TempDir() + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", dir) + if err := skillscheck.WriteStamp("1.0.20"); err != nil { + t.Fatal(err) + } + + origFetch := fetchLatest + origCur := currentVersion + t.Cleanup(func() { fetchLatest = origFetch; currentVersion = origCur }) + fetchLatest = func() (string, error) { return "1.0.21", nil } + currentVersion = func() string { return "1.0.21" } + + skillsCalled := false + origNew := newUpdater + t.Cleanup(func() { newUpdater = origNew }) + newUpdater = func() *selfupdate.Updater { + return &selfupdate.Updater{ + SkillsUpdateOverride: func() *selfupdate.NpmResult { + skillsCalled = true + return &selfupdate.NpmResult{} + }, + } + } + + f, stdout, _ := newTestFactory(t) + opts := &UpdateOptions{Factory: f, JSON: true, Check: true} + if err := updateRun(opts); err != nil { + t.Fatalf("updateRun(--check, already-latest) err = %v, want nil", err) + } + if skillsCalled { + t.Error("RunSkillsUpdate called under --check (already-latest), want skipped (pure report)") + } + + stamp, _ := skillscheck.ReadStamp() + if stamp != "1.0.20" { + t.Errorf("stamp mutated to %q under --check, want \"1.0.20\" (pure report must not write stamp)", stamp) + } + + var env map[string]interface{} + if err := json.Unmarshal(stdout.Bytes(), &env); err != nil { + t.Fatalf("json.Unmarshal stdout: %v\n%s", err, stdout.String()) + } + if env["action"] != "already_up_to_date" { + t.Errorf("action = %v, want \"already_up_to_date\"", env["action"]) + } + if _, has := env["skills_action"]; has { + t.Errorf("skills_action present under --check, want absent: %+v", env) + } + status, ok := env["skills_status"].(map[string]interface{}) + if !ok { + t.Fatalf("skills_status missing under --check + already-latest: %s", stdout.String()) + } + if status["current"] != "1.0.20" || status["target"] != "1.0.21" || status["in_sync"] != false { + t.Errorf("skills_status = %+v, want {current:\"1.0.20\", target:\"1.0.21\", in_sync:false}", status) + } +} + +// TestRunSkillsAndStamp_StampWriteFailureWarns verifies the stderr warning +// emission when RunSkillsUpdate succeeds but WriteStamp fails. +func TestRunSkillsAndStamp_StampWriteFailureWarns(t *testing.T) { + // Force WriteStamp to fail by pointing config dir at a path that exists + // as a regular file (so MkdirAll fails). + tmp := t.TempDir() + badPath := filepath.Join(tmp, "blocker") + if err := os.WriteFile(badPath, []byte("not-a-dir"), 0o644); err != nil { + t.Fatal(err) + } + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", badPath) + + f, _, stderr := newTestFactory(t) + updater := &selfupdate.Updater{ + SkillsUpdateOverride: func() *selfupdate.NpmResult { + return &selfupdate.NpmResult{} // success + }, + } + got := runSkillsAndStamp(updater, f.IOStreams, "1.0.21", false) + if got == nil || got.Err != nil { + t.Fatalf("runSkillsAndStamp() = %+v, want non-nil with nil Err", got) + } + if !strings.Contains(stderr.String(), "warning: skills synced but stamp not written") { + t.Errorf("stderr does not contain warning: %q", stderr.String()) + } +} + +// TestEmitSkillsTextHints_Success verifies the "Skills updated" success +// message is printed to ErrOut on a successful (Err == nil) result. +func TestEmitSkillsTextHints_Success(t *testing.T) { + f, _, stderr := newTestFactory(t) + emitSkillsTextHints(f.IOStreams, &selfupdate.NpmResult{}) // Err==nil → success + if !strings.Contains(stderr.String(), "Skills updated") { + t.Errorf("stderr does not contain 'Skills updated': %q", stderr.String()) + } +} diff --git a/internal/binding/types.go b/internal/binding/types.go index e45de9ed4..8bf83813c 100644 --- a/internal/binding/types.go +++ b/internal/binding/types.go @@ -169,7 +169,7 @@ type ProviderConfig struct { const ( DefaultFileTimeoutMs = 5000 DefaultFileMaxBytes = 1024 * 1024 // 1 MiB - DefaultExecTimeoutMs = 5000 + DefaultExecTimeoutMs = 10000 DefaultExecMaxOutputBytes = 1024 * 1024 // 1 MiB ) diff --git a/internal/skillscheck/check.go b/internal/skillscheck/check.go new file mode 100644 index 000000000..c88a89450 --- /dev/null +++ b/internal/skillscheck/check.go @@ -0,0 +1,38 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package skillscheck + +// Init runs the synchronous skills version check. Stores a StaleNotice +// when the local stamp does not match currentVersion. Safe to call +// from cmd/root.go before rootCmd.Execute(); zero network, zero +// subprocess — only a local stamp file read. +// +// Skip rules: see shouldSkip (CI envs, DEV builds, non-release semver, +// LARKSUITE_CLI_NO_SKILLS_NOTIFIER opt-out). +// +// Failure modes (all → no notice, no nag): +// - shouldSkip rule met +// - ReadStamp returns an I/O error other than ENOENT +// - Stamp matches currentVersion (in-sync) +func Init(currentVersion string) { + // Clear any stale notice from a prior call so early returns below + // (skip rules / read errors / in-sync) leave pending == nil instead + // of preserving a stale value from a previous Init invocation. + SetPending(nil) + if shouldSkip(currentVersion) { + return + } + stamp, err := ReadStamp() + if err != nil { + // Fail closed — don't nag for a transient FS problem. + return + } + if stamp == currentVersion { + return + } + SetPending(&StaleNotice{ + Current: stamp, // "" when never synced + Target: currentVersion, + }) +} diff --git a/internal/skillscheck/check_test.go b/internal/skillscheck/check_test.go new file mode 100644 index 000000000..f2443c462 --- /dev/null +++ b/internal/skillscheck/check_test.go @@ -0,0 +1,90 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package skillscheck + +import ( + "os" + "path/filepath" + "testing" +) + +func resetPending(t *testing.T) { + t.Helper() + SetPending(nil) + t.Cleanup(func() { SetPending(nil) }) +} + +func TestInit_InSync_NoNotice(t *testing.T) { + clearSkillsSkipEnv(t) + resetPending(t) + dir := t.TempDir() + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", dir) + if err := WriteStamp("1.0.21"); err != nil { + t.Fatal(err) + } + Init("1.0.21") + if got := GetPending(); got != nil { + t.Errorf("GetPending() = %+v, want nil (in-sync)", got) + } +} + +func TestInit_ColdStart_NoticeWithEmptyCurrent(t *testing.T) { + clearSkillsSkipEnv(t) + resetPending(t) + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) + Init("1.0.21") + got := GetPending() + if got == nil { + t.Fatal("GetPending() = nil, want non-nil for cold start") + } + if got.Current != "" || got.Target != "1.0.21" { + t.Errorf("notice = %+v, want {Current:\"\", Target:\"1.0.21\"}", got) + } +} + +func TestInit_Drift_NoticeWithStampVersion(t *testing.T) { + clearSkillsSkipEnv(t) + resetPending(t) + dir := t.TempDir() + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", dir) + if err := WriteStamp("1.0.20"); err != nil { + t.Fatal(err) + } + Init("1.0.21") + got := GetPending() + if got == nil { + t.Fatal("GetPending() = nil, want non-nil for drift") + } + if got.Current != "1.0.20" || got.Target != "1.0.21" { + t.Errorf("notice = %+v, want {Current:\"1.0.20\", Target:\"1.0.21\"}", got) + } +} + +func TestInit_Skipped_NoNotice(t *testing.T) { + clearSkillsSkipEnv(t) + resetPending(t) + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) + // Even with an empty config dir (no stamp), DEV version should skip + // the check entirely and never emit a notice. + Init("DEV") + if got := GetPending(); got != nil { + t.Errorf("GetPending() = %+v, want nil (skip rules met)", got) + } +} + +func TestInit_ReadStampError_FailsClosed(t *testing.T) { + clearSkillsSkipEnv(t) + resetPending(t) + dir := t.TempDir() + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", dir) + // Make the stamp path a directory so vfs.ReadFile returns a + // non-ENOENT I/O error. + if err := os.MkdirAll(filepath.Join(dir, "skills.stamp"), 0o755); err != nil { + t.Fatal(err) + } + Init("1.0.21") + if got := GetPending(); got != nil { + t.Errorf("GetPending() = %+v, want nil (fail closed on I/O error)", got) + } +} diff --git a/internal/skillscheck/notice.go b/internal/skillscheck/notice.go new file mode 100644 index 000000000..93b1d9dfb --- /dev/null +++ b/internal/skillscheck/notice.go @@ -0,0 +1,46 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +// Package skillscheck verifies that the locally installed lark-cli +// skills are in sync with the running binary version, by comparing +// the current binary version against a stamp file written when skills +// are last synced (by `lark-cli update`). On mismatch it stores a +// notice for injection into JSON envelopes via output.PendingNotice. +package skillscheck + +import ( + "fmt" + "sync/atomic" +) + +// StaleNotice signals that the locally synced skills version does not +// match the running binary. Current is the last successfully synced +// version (or "" when never synced); Target is the running binary +// version. Mirrors internal/update.UpdateInfo's pending-notice pattern. +type StaleNotice struct { + Current string `json:"current"` + Target string `json:"target"` +} + +// Message returns a single-line, AI-agent-parseable description of the +// gap plus the canonical fix command. Mirrors internal/update.UpdateInfo.Message +// in style ("..., run: lark-cli update" suffix). +func (s *StaleNotice) Message() string { + if s.Current == "" { + return "lark-cli skills not installed, run: lark-cli update" + } + return fmt.Sprintf( + "lark-cli skills %s out of sync with binary %s, run: lark-cli update", + s.Current, s.Target, + ) +} + +// pending stores the latest stale notice for the current process. +var pending atomic.Pointer[StaleNotice] + +// SetPending stores the stale notice for consumption by output decorators. +// Pass nil to clear. +func SetPending(n *StaleNotice) { pending.Store(n) } + +// GetPending returns the pending stale notice, or nil. +func GetPending() *StaleNotice { return pending.Load() } diff --git a/internal/skillscheck/notice_test.go b/internal/skillscheck/notice_test.go new file mode 100644 index 000000000..688ed57c1 --- /dev/null +++ b/internal/skillscheck/notice_test.go @@ -0,0 +1,71 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package skillscheck + +import ( + "sync" + "testing" +) + +func TestStaleNotice_Message(t *testing.T) { + tests := []struct { + name string + n StaleNotice + want string + }{ + { + "cold_start", + StaleNotice{Current: "", Target: "1.0.21"}, + "lark-cli skills not installed, run: lark-cli update", + }, + { + "drift", + StaleNotice{Current: "1.0.20", Target: "1.0.21"}, + "lark-cli skills 1.0.20 out of sync with binary 1.0.21, run: lark-cli update", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := tt.n.Message(); got != tt.want { + t.Errorf("Message() = %q, want %q", got, tt.want) + } + }) + } +} + +func TestSetGetPending(t *testing.T) { + SetPending(nil) + t.Cleanup(func() { SetPending(nil) }) + + if got := GetPending(); got != nil { + t.Fatalf("initial GetPending() = %+v, want nil", got) + } + + want := &StaleNotice{Current: "1.0.20", Target: "1.0.21"} + SetPending(want) + got := GetPending() + if got == nil || got.Current != "1.0.20" || got.Target != "1.0.21" { + t.Errorf("GetPending() = %+v, want %+v", got, want) + } +} + +func TestSetGetPending_Concurrent(t *testing.T) { + SetPending(nil) + t.Cleanup(func() { SetPending(nil) }) + + var wg sync.WaitGroup + for i := 0; i < 100; i++ { + wg.Add(2) + go func() { + defer wg.Done() + SetPending(&StaleNotice{Current: "a", Target: "b"}) + }() + go func() { + defer wg.Done() + _ = GetPending() + }() + } + wg.Wait() + // Just verifying no race; -race flag enforces. +} diff --git a/internal/skillscheck/skip.go b/internal/skillscheck/skip.go new file mode 100644 index 000000000..b4da13d4b --- /dev/null +++ b/internal/skillscheck/skip.go @@ -0,0 +1,27 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package skillscheck + +import ( + "os" + + "github.com/larksuite/cli/internal/update" +) + +// shouldSkip returns true when the skills check should be silently +// suppressed. Mirrors internal/update.shouldSkip semantics but uses +// a dedicated opt-out env var so users can disable the skills nag +// without also disabling the binary update nag. +func shouldSkip(version string) bool { + if os.Getenv("LARKSUITE_CLI_NO_SKILLS_NOTIFIER") != "" { + return true + } + if update.IsCIEnv() { + return true + } + if version == "DEV" || version == "dev" || version == "" { + return true + } + return !update.IsRelease(version) +} diff --git a/internal/skillscheck/skip_test.go b/internal/skillscheck/skip_test.go new file mode 100644 index 000000000..0d9b21655 --- /dev/null +++ b/internal/skillscheck/skip_test.go @@ -0,0 +1,68 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package skillscheck + +import ( + "os" + "testing" +) + +// clearSkillsSkipEnv unsets the env vars shouldSkip checks so the +// host environment cannot pollute test results. +func clearSkillsSkipEnv(t *testing.T) { + t.Helper() + for _, key := range []string{"LARKSUITE_CLI_NO_SKILLS_NOTIFIER", "CI", "BUILD_NUMBER", "RUN_ID"} { + t.Setenv(key, "") + os.Unsetenv(key) + } +} + +func TestShouldSkip(t *testing.T) { + tests := []struct { + name string + setup func(t *testing.T) + version string + want bool + }{ + {"release_no_skip", clearSkillsSkipEnv, "1.0.21", false}, + {"dev_uppercase", clearSkillsSkipEnv, "DEV", true}, + {"dev_lowercase", clearSkillsSkipEnv, "dev", true}, + {"empty_version", clearSkillsSkipEnv, "", true}, + {"git_describe", clearSkillsSkipEnv, "1.0.0-12-g9b933f1-dirty", true}, + {"opt_out", func(t *testing.T) { + clearSkillsSkipEnv(t) + t.Setenv("LARKSUITE_CLI_NO_SKILLS_NOTIFIER", "1") + }, "1.0.21", true}, + {"ci_env", func(t *testing.T) { + clearSkillsSkipEnv(t) + t.Setenv("CI", "true") + }, "1.0.21", true}, + {"build_number_env", func(t *testing.T) { + clearSkillsSkipEnv(t) + t.Setenv("BUILD_NUMBER", "42") + }, "1.0.21", true}, + {"run_id_env", func(t *testing.T) { + clearSkillsSkipEnv(t) + t.Setenv("RUN_ID", "abc") + }, "1.0.21", true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tt.setup(t) + if got := shouldSkip(tt.version); got != tt.want { + t.Errorf("shouldSkip(%q) = %v, want %v", tt.version, got, tt.want) + } + }) + } +} + +// Independent opt-out: LARKSUITE_CLI_NO_SKILLS_NOTIFIER must NOT be +// affected by LARKSUITE_CLI_NO_UPDATE_NOTIFIER (different env vars). +func TestShouldSkip_OptOutIsIndependent(t *testing.T) { + clearSkillsSkipEnv(t) + t.Setenv("LARKSUITE_CLI_NO_UPDATE_NOTIFIER", "1") // update opt-out, not us + if shouldSkip("1.0.21") { + t.Error("shouldSkip(release) = true with only LARKSUITE_CLI_NO_UPDATE_NOTIFIER set, want false") + } +} diff --git a/internal/skillscheck/stamp.go b/internal/skillscheck/stamp.go new file mode 100644 index 000000000..052e331c9 --- /dev/null +++ b/internal/skillscheck/stamp.go @@ -0,0 +1,49 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package skillscheck + +import ( + "errors" + "io/fs" + "path/filepath" + "strings" + + "github.com/larksuite/cli/internal/core" + "github.com/larksuite/cli/internal/validate" + "github.com/larksuite/cli/internal/vfs" +) + +const stampFile = "skills.stamp" + +// stampPath returns ~/.lark-cli/skills.stamp. +// Uses the BASE config dir (not workspace-aware) because skills install +// globally via `npx -g`; per-workspace tracking would produce false +// drift signals when switching workspaces. +func stampPath() string { + return filepath.Join(core.GetBaseConfigDir(), stampFile) +} + +// ReadStamp returns the version recorded in the stamp file. Returns +// ("", nil) when the file does not exist (interpreted as "never synced"). +// Other I/O errors are returned as-is so callers can fail closed. +func ReadStamp() (string, error) { + data, err := vfs.ReadFile(stampPath()) + if err != nil { + if errors.Is(err, fs.ErrNotExist) { + return "", nil + } + return "", err + } + return strings.TrimSpace(string(data)), nil +} + +// WriteStamp records `version` as the last successfully synced skills +// version. Atomic via tmp + rename (validate.AtomicWrite). Creates +// the base config directory if it does not exist. +func WriteStamp(version string) error { + if err := vfs.MkdirAll(core.GetBaseConfigDir(), 0o700); err != nil { + return err + } + return validate.AtomicWrite(stampPath(), []byte(version), 0o644) +} diff --git a/internal/skillscheck/stamp_test.go b/internal/skillscheck/stamp_test.go new file mode 100644 index 000000000..8e60dfbb4 --- /dev/null +++ b/internal/skillscheck/stamp_test.go @@ -0,0 +1,113 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package skillscheck + +import ( + "os" + "path/filepath" + "testing" +) + +func TestReadStamp_Missing(t *testing.T) { + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) + got, err := ReadStamp() + if err != nil { + t.Fatalf("ReadStamp() err = %v, want nil for ENOENT", err) + } + if got != "" { + t.Errorf("ReadStamp() = %q, want \"\" for missing file", got) + } +} + +func TestReadStamp_Normal(t *testing.T) { + dir := t.TempDir() + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", dir) + if err := os.WriteFile(filepath.Join(dir, "skills.stamp"), []byte("1.0.21"), 0o644); err != nil { + t.Fatal(err) + } + got, err := ReadStamp() + if err != nil || got != "1.0.21" { + t.Errorf("ReadStamp() = (%q, %v), want (\"1.0.21\", nil)", got, err) + } +} + +func TestReadStamp_TrailingNewlineTolerated(t *testing.T) { + dir := t.TempDir() + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", dir) + if err := os.WriteFile(filepath.Join(dir, "skills.stamp"), []byte("1.0.21\n"), 0o644); err != nil { + t.Fatal(err) + } + got, _ := ReadStamp() + if got != "1.0.21" { + t.Errorf("ReadStamp() = %q, want \"1.0.21\" (newline trimmed)", got) + } +} + +func TestReadStamp_EmptyFile(t *testing.T) { + dir := t.TempDir() + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", dir) + if err := os.WriteFile(filepath.Join(dir, "skills.stamp"), []byte(""), 0o644); err != nil { + t.Fatal(err) + } + got, err := ReadStamp() + if err != nil || got != "" { + t.Errorf("ReadStamp() = (%q, %v), want (\"\", nil)", got, err) + } +} + +func TestWriteStamp_CreatesDir(t *testing.T) { + dir := filepath.Join(t.TempDir(), "nested") + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", dir) + if err := WriteStamp("1.0.21"); err != nil { + t.Fatalf("WriteStamp() = %v, want nil", err) + } + got, _ := os.ReadFile(filepath.Join(dir, "skills.stamp")) + if string(got) != "1.0.21" { + t.Errorf("file content = %q, want \"1.0.21\"", string(got)) + } +} + +func TestWriteStamp_OverwritesExisting(t *testing.T) { + dir := t.TempDir() + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", dir) + if err := WriteStamp("1.0.20"); err != nil { + t.Fatal(err) + } + if err := WriteStamp("1.0.21"); err != nil { + t.Fatal(err) + } + got, _ := ReadStamp() + if got != "1.0.21" { + t.Errorf("ReadStamp() after overwrite = %q, want \"1.0.21\"", got) + } +} + +func TestWriteStamp_NoTrailingNewline(t *testing.T) { + dir := t.TempDir() + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", dir) + if err := WriteStamp("1.0.21"); err != nil { + t.Fatal(err) + } + raw, _ := os.ReadFile(filepath.Join(dir, "skills.stamp")) + if string(raw) != "1.0.21" { + t.Errorf("raw file = %q, want exactly \"1.0.21\" (no newline)", string(raw)) + } +} + +// TestWriteStamp_MkdirAllFailure verifies WriteStamp returns the mkdir error +// when the base config dir cannot be created (parent path is a regular file). +func TestWriteStamp_MkdirAllFailure(t *testing.T) { + tmp := t.TempDir() + blocker := filepath.Join(tmp, "blocker") + // Create a regular file where MkdirAll wants to create a directory. + if err := os.WriteFile(blocker, []byte("not-a-dir"), 0o644); err != nil { + t.Fatal(err) + } + // Point the config dir at a path UNDER the regular file — MkdirAll must fail. + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", filepath.Join(blocker, "child")) + + if err := WriteStamp("1.0.21"); err == nil { + t.Fatal("WriteStamp() = nil, want non-nil error from MkdirAll failure") + } +} diff --git a/internal/update/update.go b/internal/update/update.go index 39052d9ef..c3509b3d6 100644 --- a/internal/update/update.go +++ b/internal/update/update.go @@ -37,9 +37,12 @@ type UpdateInfo struct { Latest string `json:"latest"` } -// Message returns a concise update notification. +// Message returns a concise update notification including the canonical +// fix command. Aligned with skillscheck.StaleNotice.Message style so +// AI agents can parse a unified "run: lark-cli update" hint across +// both notice types. func (u *UpdateInfo) Message() string { - return fmt.Sprintf("lark-cli %s available, current %s", u.Latest, u.Current) + return fmt.Sprintf("lark-cli %s available, current %s, run: lark-cli update", u.Latest, u.Current) } // pending stores the latest update info for the current process. @@ -111,10 +114,8 @@ func shouldSkip(version string) bool { return true } // Suppress in CI environments. - for _, key := range []string{"CI", "BUILD_NUMBER", "RUN_ID"} { - if os.Getenv(key) != "" { - return true - } + if IsCIEnv() { + return true } // No version info at all — can't compare. if version == "DEV" || version == "dev" || version == "" { @@ -141,6 +142,24 @@ func isRelease(version string) bool { return !gitDescribePattern.MatchString(v) } +// IsRelease reports whether version looks like a clean published release +// (semver "1.0.0", or npm prerelease "1.0.0-beta.1") and not a git-describe +// dev build like "1.0.0-12-g9b933f1-dirty". Exported so internal/skillscheck +// can apply the same release-only gating without duplicating the regex. +func IsRelease(version string) bool { return isRelease(version) } + +// IsCIEnv returns true when any of the standard CI environment variables +// is set. Exported for internal/skillscheck so its skip rules track the +// same CI-suppression behavior as the update notifier. +func IsCIEnv() bool { + for _, key := range []string{"CI", "BUILD_NUMBER", "RUN_ID"} { + if os.Getenv(key) != "" { + return true + } + } + return false +} + // --- state file I/O --- func statePath() string { diff --git a/internal/update/update_test.go b/internal/update/update_test.go index 5ff356225..d19619cf3 100644 --- a/internal/update/update_test.go +++ b/internal/update/update_test.go @@ -10,7 +10,6 @@ import ( "net/url" "os" "path/filepath" - "strings" "testing" "time" ) @@ -143,28 +142,27 @@ func TestShouldSkip(t *testing.T) { func TestIsRelease(t *testing.T) { tests := []struct { - version string - want bool + name string + ver string + want bool }{ - {"1.0.0", true}, - {"v1.0.0", true}, - {"0.1.0", true}, - {"1.0.0-beta.1", true}, - {"1.0.0-rc.1", true}, - {"2.0.0-alpha.0", true}, - {"v1.0.0-12-g9b933f1", false}, // git describe - {"v1.0.0-12-g9b933f1-dirty", false}, // git describe dirty - {"v2.1.0-3-gabcdef0", false}, // git describe short - {"9b933f1", false}, // bare commit hash - {"DEV", false}, // dev marker - {"", false}, // empty - {"1.0", false}, // incomplete semver + {"clean_semver", "1.0.0", true}, + {"v_prefix", "v1.0.0", true}, + {"prerelease", "1.0.0-beta.1", true}, + {"rc", "1.0.0-rc.1", true}, + {"alpha_prerelease", "2.0.0-alpha.0", true}, + {"git_describe_dirty", "1.0.0-12-g9b933f1-dirty", false}, + {"git_describe_clean", "1.0.0-12-g9b933f1", false}, + {"bare_commit_hash", "9b933f1", false}, + {"dev_marker", "DEV", false}, + {"incomplete_semver", "1.0", false}, + {"empty", "", false}, + {"invalid", "not-a-version", false}, } for _, tt := range tests { - t.Run(tt.version, func(t *testing.T) { - got := isRelease(tt.version) - if got != tt.want { - t.Errorf("isRelease(%q) = %v, want %v", tt.version, got, tt.want) + t.Run(tt.name, func(t *testing.T) { + if got := IsRelease(tt.ver); got != tt.want { + t.Errorf("IsRelease(%q) = %v, want %v", tt.ver, got, tt.want) } }) } @@ -172,13 +170,10 @@ func TestIsRelease(t *testing.T) { func TestUpdateInfoMethods(t *testing.T) { info := &UpdateInfo{Current: "1.0.0", Latest: "2.0.0"} - - msg := info.Message() - if !strings.Contains(msg, "2.0.0") { - t.Errorf("Message() missing latest version: %s", msg) - } - if !strings.Contains(msg, "1.0.0") { - t.Errorf("Message() missing current version: %s", msg) + got := info.Message() + want := "lark-cli 2.0.0 available, current 1.0.0, run: lark-cli update" + if got != want { + t.Errorf("Message() = %q, want %q", got, want) } } @@ -264,3 +259,19 @@ func TestPendingAtomicAccess(t *testing.T) { // Clean up for other tests SetPending(nil) } + +func TestIsCIEnv(t *testing.T) { + clearSkipEnv(t) + if IsCIEnv() { + t.Fatal("IsCIEnv() = true after clearSkipEnv, want false") + } + for _, key := range []string{"CI", "BUILD_NUMBER", "RUN_ID"} { + t.Run(key, func(t *testing.T) { + clearSkipEnv(t) + t.Setenv(key, "1") + if !IsCIEnv() { + t.Errorf("IsCIEnv() = false with %s=1, want true", key) + } + }) + } +}