From 16d7d238ff94aca1bfd1a9977a962fede3c18fa0 Mon Sep 17 00:00:00 2001 From: bmorelli25 Date: Mon, 3 Aug 2026 14:40:13 -0700 Subject: [PATCH 1/4] Keep setup usable when an optional tool installer fails A failing Vale or docs-builder installer aborted install before skills and host adapters were configured, so a sudo prompt that could not be answered left the command with nothing done. Both tools are optional, so report every failure, finish the rest of the setup, and exit non-zero. Status output could not distinguish a tool the user never asked for from one that failed to install. Item hints already existed but only reached the session-start hook, so print them beneath any row that is not current, and point a component that was never requested at its install option. Also make version reporting honest: --yes was parsed and discarded, version lookups silently reported unknown when the shared 60-per-hour GitHub limit was reached, and a tool that logged before printing its version was read as not installed. Co-authored-by: Cursor --- README.md | 16 ++- cmd/elastic-docs-utils/main.go | 74 +++++++++--- cmd/elastic-docs-utils/main_test.go | 73 +++++++++++- docs/reference.md | 22 ++++ internal/bootstrap/bootstrap.go | 42 ++++--- internal/bootstrap/bootstrap_test.go | 23 ++++ internal/updates/updates.go | 162 ++++++++++++++++++++------- internal/updates/updates_test.go | 103 +++++++++++++++++ 8 files changed, 442 insertions(+), 73 deletions(-) diff --git a/README.md b/README.md index bc64c29..d23585f 100644 --- a/README.md +++ b/README.md @@ -53,7 +53,13 @@ installers. Every non-dry-run `install` and `update` also refreshes the status of docs-builder, the Vale binary, Elastic Vale rules, managed skills, and Elastic -Docs Utils. +Docs Utils. Each component that is not current prints its next step. A +component shown as `not installed` is usually one that was never requested; +install it with the matching `--with-...` option above. + +Version lookups use the unauthenticated GitHub API, which allows 60 requests +per hour per address. Set `GITHUB_TOKEN` or `GH_TOKEN` to avoid that shared +limit; without it, exhausted lookups report `unknown` and say so. For a complete first-time setup, use `--with-docs-tools`. It runs the maintained installers for Vale, Elastic Vale rules, and docs-builder: @@ -78,6 +84,14 @@ overwrites an existing docs-builder binary. Vale itself remains managed by its platform package manager when it is already installed; the Vale installer does not forcibly replace that executable. +Add `--yes` where nothing can answer a prompt, such as CI or a command with no +terminal attached. It closes the installers' input rather than accepting +replacement prompts, so existing configuration is left alone. + +Because these tools are optional, a failing installer does not abandon the +rest of the command. Skills and host adapters are still configured, every +failure is reported, and the command exits non-zero. + See the [command and managed-locations reference](docs/reference.md) for `--verbose`, configuration paths, and optional-tool locations. diff --git a/cmd/elastic-docs-utils/main.go b/cmd/elastic-docs-utils/main.go index 59680c2..86d5a24 100644 --- a/cmd/elastic-docs-utils/main.go +++ b/cmd/elastic-docs-utils/main.go @@ -132,7 +132,6 @@ func commandInstall(r *ui.Renderer, args []string) error { if err := fs.Parse(args); err != nil { return err } - _ = yes r.Header(Version) if *dryRun { r.DryRun() @@ -154,9 +153,12 @@ func commandInstall(r *ui.Renderer, args []string) error { rows = append(rows, []string{string(id), "selected"}) } r.Table([]string{"HOST", "STATUS"}, rows) - if err := installOptionalTools(r, *withVale, *withDocsBuilder, *dryRun, *force); err != nil { - return err - } + // Vale and docs-builder are optional, and their installers touch system + // locations that can fail for reasons unrelated to this setup. Report the + // failure but still configure skills and host adapters, which are the + // point of the command. + toolFailures := installOptionalTools(r, *withVale, *withDocsBuilder, *dryRun, *force, *yes) + reportToolFailures(r, toolFailures) if err := synchronize(ids, *internal, *dryRun, *force, r); err != nil { return err @@ -178,11 +180,17 @@ func commandInstall(r *ui.Renderer, args []string) error { if *force { r.Warn("Replacing conflicting managed MCP entries with the Elastic Docs Utils configuration.") } + if len(toolFailures) > 0 { + return toolFailureError(toolFailures) + } r.Success("Elastic Docs Utils is configured.") return nil } -func installOptionalTools(r *ui.Renderer, vale, docsBuilder, dryRun, force bool) error { +// installOptionalTools runs the selected upstream installers and returns every +// failure instead of stopping at the first one, so one broken installer cannot +// hide the state of the other. +func installOptionalTools(r *ui.Renderer, vale, docsBuilder, dryRun, force, assumeYes bool) []error { if !vale && !docsBuilder { return nil } @@ -196,21 +204,35 @@ func installOptionalTools(r *ui.Renderer, vale, docsBuilder, dryRun, force bool) } return nil } + var failures []error if vale { r.Info("Installing Vale and Elastic Vale rules%s.", forced(force)) r.Verbose("Runs the upstream Elastic Vale Rules installer; it reports the Vale binary, configuration, and rule paths it edits.") - if err := bootstrap.InstallVale(force); err != nil { - return fmt.Errorf("install Vale and Elastic Vale rules: %w", err) + if err := bootstrap.InstallVale(force, assumeYes); err != nil { + failures = append(failures, fmt.Errorf("install Vale and Elastic Vale rules: %w", err)) } } if docsBuilder { r.Info("Installing docs-builder%s.", forced(force)) r.Verbose("Runs the upstream docs-builder installer; it reports the binary path it edits.") - if err := bootstrap.InstallDocsBuilder(force); err != nil { - return fmt.Errorf("install docs-builder: %w", err) + if err := bootstrap.InstallDocsBuilder(force, assumeYes); err != nil { + failures = append(failures, fmt.Errorf("install docs-builder: %w", err)) } } - return nil + return failures +} + +func reportToolFailures(r *ui.Renderer, failures []error) { + for _, failure := range failures { + r.Warn("%v", failure) + } + if len(failures) > 0 { + r.Warn("Continuing with the rest of the setup. Re-run the installer for the tools above once the cause is resolved.") + } +} + +func toolFailureError(failures []error) error { + return fmt.Errorf("%d optional documentation tool installer(s) failed: %w", len(failures), errors.Join(failures...)) } func forced(force bool) string { @@ -291,6 +313,7 @@ func commandStatus(r *ui.Renderer, args []string) error { updateRows = append(updateRows, []string{item.Name, item.State}) } r.Table([]string{"COMPONENT", "STATUS"}, updateRows) + renderUpdateHints(r, cache.Items) } return nil } @@ -346,25 +369,31 @@ func commandUpdate(r *ui.Renderer, args []string) error { } else { r.Info("Skipping Elastic Docs skills.") } + // A failed component must not stop the remaining ones, and the refreshed + // status below is most useful precisely when something went wrong. + var toolFailures []error if selected.vale { - if err := installOptionalTools(r, true, false, *dryRun, *force); err != nil { - return err - } + toolFailures = append(toolFailures, installOptionalTools(r, true, false, *dryRun, *force, false)...) } else { r.Info("Skipping Vale and Elastic Vale rules.") } if selected.docsBuilder { - if err := installOptionalTools(r, false, true, *dryRun, *force); err != nil { - return err - } + toolFailures = append(toolFailures, installOptionalTools(r, false, true, *dryRun, *force, false)...) } else { r.Info("Skipping docs-builder.") } + reportToolFailures(r, toolFailures) if *dryRun { r.Info("Would refresh documentation tool status after updates.") return nil } - return refreshUpdates(r) + if err := refreshUpdates(r); err != nil { + return err + } + if len(toolFailures) > 0 { + return toolFailureError(toolFailures) + } + return nil } type updateComponents struct { @@ -411,6 +440,17 @@ func renderUpdateStatus(r *ui.Renderer, status updates.Status) { rows = append(rows, []string{item.Name, item.Installed, item.Latest, item.State}) } r.Table([]string{"COMPONENT", "INSTALLED", "LATEST", "STATUS"}, rows) + renderUpdateHints(r, status.Items) +} + +// renderUpdateHints prints the next step for each row that is not current. A +// status of "missing" or "unknown" is not actionable on its own. +func renderUpdateHints(r *ui.Renderer, items []updates.Item) { + for _, item := range items { + if item.Hint != "" && item.State != "current" && item.State != "local" { + r.Info("%s: %s", item.Name, item.Hint) + } + } } func commandDoctor(r *ui.Renderer, args []string) error { diff --git a/cmd/elastic-docs-utils/main_test.go b/cmd/elastic-docs-utils/main_test.go index 5df4c70..4388fef 100644 --- a/cmd/elastic-docs-utils/main_test.go +++ b/cmd/elastic-docs-utils/main_test.go @@ -1,6 +1,14 @@ package main -import "testing" +import ( + "bytes" + "errors" + "strings" + "testing" + + "github.com/elastic/docs-utils/internal/ui" + "github.com/elastic/docs-utils/internal/updates" +) func TestParseUpdateComponents(t *testing.T) { tests := []struct { @@ -42,3 +50,66 @@ func TestParseUpdateComponentsRejectsUnknownComponent(t *testing.T) { t.Fatal("unknown component was accepted") } } + +func TestInstallOptionalToolsSkippedWhenNoneSelected(t *testing.T) { + var out bytes.Buffer + r := ui.New(ui.ColorNever, &out, &out) + if failures := installOptionalTools(r, false, false, false, false, false); failures != nil { + t.Fatalf("failures = %v, want none", failures) + } + if out.Len() != 0 { + t.Fatalf("unselected tools produced output: %q", out.String()) + } +} + +func TestInstallOptionalToolsDryRunRunsNoInstaller(t *testing.T) { + var out bytes.Buffer + r := ui.New(ui.ColorNever, &out, &out) + if failures := installOptionalTools(r, true, true, true, false, false); failures != nil { + t.Fatalf("failures = %v, want none", failures) + } + for _, want := range []string{"Would run the supported Elastic Vale Rules installer", "Would run the supported docs-builder installer"} { + if !strings.Contains(out.String(), want) { + t.Fatalf("dry run output missing %q:\n%s", want, out.String()) + } + } +} + +func TestToolFailureErrorKeepsEveryCause(t *testing.T) { + vale := errors.New("install Vale and Elastic Vale rules: boom") + builder := errors.New("install docs-builder: boom") + err := toolFailureError([]error{vale, builder}) + if !errors.Is(err, vale) || !errors.Is(err, builder) { + t.Fatalf("aggregated error lost a cause: %v", err) + } + if !strings.Contains(err.Error(), "2 optional") { + t.Fatalf("error = %q, want the failure count", err.Error()) + } +} + +func TestReportToolFailuresWarnsAndContinues(t *testing.T) { + var out bytes.Buffer + r := ui.New(ui.ColorNever, &out, &out) + reportToolFailures(r, []error{errors.New("install docs-builder: boom")}) + if !strings.Contains(out.String(), "install docs-builder: boom") { + t.Fatalf("failure was not reported:\n%s", out.String()) + } + if !strings.Contains(out.String(), "Continuing with the rest of the setup") { + t.Fatalf("output does not say setup continues:\n%s", out.String()) + } +} + +func TestRenderUpdateStatusShowsHintsForActionableRows(t *testing.T) { + var out bytes.Buffer + r := ui.New(ui.ColorNever, &out, &out) + renderUpdateStatus(r, updates.Status{Items: []updates.Item{ + {Name: "docs-builder", Installed: "not installed", State: "missing", Hint: "Run `elastic-docs-utils install --with-docs-builder`"}, + {Name: "Vale", Installed: "3.17.0", Latest: "3.17.0", State: "current", Hint: "should not appear"}, + }}) + if !strings.Contains(out.String(), "Run `elastic-docs-utils install --with-docs-builder`") { + t.Fatalf("missing component has no next step:\n%s", out.String()) + } + if strings.Contains(out.String(), "should not appear") { + t.Fatalf("current component printed a hint:\n%s", out.String()) + } +} diff --git a/docs/reference.md b/docs/reference.md index fb6e852..8a34d0b 100644 --- a/docs/reference.md +++ b/docs/reference.md @@ -54,6 +54,16 @@ overwrites docs-builder when it already exists. The upstream Vale installer keeps an existing Vale executable package-managed; it refreshes the Elastic rules rather than forcibly replacing the executable. +Add `--yes` when no one is available to answer a prompt, such as in CI or when +a command runs without a terminal. Unlike `--force`, it does not accept +replacement prompts: it closes the installer's input so the installer takes its +own default and leaves existing configuration in place. + +These tools are optional, so a failing installer does not stop the rest of the +command. `install` still synchronizes skills and host adapters, `update` still +processes the remaining components and refreshes status, and both report every +failure and then exit non-zero. + `install --with-vale` runs the maintained Elastic Vale Rules platform installer. It may install the Vale binary and manages the following locations: @@ -85,3 +95,15 @@ Supported components are `skills`, `vale`, `vale-rules`, and `docs-builder`. The `vale` and `vale-rules` selections use the same upstream installer because it manages both components together. Add `--force` to accept replacement prompts from upstream installers. + +### Update status + +`status` and `check-updates` print a next step beneath any component that is +not current. A component reported as `not installed` is usually one that was +never requested, so its next step is the matching `install --with-...` option +rather than an update command. + +Version lookups use the GitHub API, which allows 60 unauthenticated requests +per hour per address. When that limit is reached, or GitHub is unreachable, the +affected rows report `unknown` and say why. Set `GITHUB_TOKEN` or `GH_TOKEN` to +authenticate the lookups and avoid the shared limit. diff --git a/internal/bootstrap/bootstrap.go b/internal/bootstrap/bootstrap.go index a4bca42..1e83788 100644 --- a/internal/bootstrap/bootstrap.go +++ b/internal/bootstrap/bootstrap.go @@ -139,24 +139,26 @@ func copyExecutable(source, target string) error { // InstallVale delegates to the official Elastic Vale Rules installer, which // installs the Vale binary when necessary and installs the Elastic rule bundle. // Force confirms replacement of an existing non-Elastic Vale configuration. -func InstallVale(force bool) error { +// AssumeYes keeps the installer from blocking on a prompt it cannot read. +func InstallVale(force, assumeYes bool) error { name, shell, err := valeScript() if err != nil { return err } - return downloadAndRun(valeRulesRaw+name, shell, force) + return downloadAndRun(valeRulesRaw+name, shell, force, assumeYes) } // InstallDocsBuilder delegates to the official Docs Builder installer. Force -// confirms replacement when the installer finds an existing binary. -func InstallDocsBuilder(force bool) error { +// confirms replacement when the installer finds an existing binary. AssumeYes +// keeps the installer from blocking on a prompt it cannot read. +func InstallDocsBuilder(force, assumeYes bool) error { if runtime.GOOS == "windows" { - return downloadAndRun(docsBuilderWindows, "powershell", force) + return downloadAndRun(docsBuilderWindows, "powershell", force, assumeYes) } if runtime.GOOS != "darwin" && runtime.GOOS != "linux" { return fmt.Errorf("docs-builder installation is not supported on %s", runtime.GOOS) } - return downloadAndRun(docsBuilderUnix, "sh", force) + return downloadAndRun(docsBuilderUnix, "sh", force, assumeYes) } func valeScript() (string, string, error) { @@ -172,7 +174,7 @@ func valeScript() (string, string, error) { } } -func downloadAndRun(url, shell string, force bool) error { +func downloadAndRun(url, shell string, force, assumeYes bool) error { path, err := download(url, extension(shell)) if err != nil { return err @@ -184,20 +186,30 @@ func downloadAndRun(url, shell string, force bool) error { } cmd := exec.Command(command, args...) cmd.Stdout, cmd.Stderr = os.Stdout, os.Stderr - if force { - // The maintained installers ask only before replacing existing local - // configuration or binaries. Supplying yes lets --force be safely - // non-interactive without exposing configuration contents. - cmd.Stdin = strings.NewReader("y\n") - } else { - cmd.Stdin = os.Stdin - } + cmd.Stdin = installerInput(force, assumeYes) if err := cmd.Run(); err != nil { return fmt.Errorf("run upstream installer: %w", err) } return nil } +// installerInput selects the stream the upstream installers read prompts from. +// The maintained installers ask only before replacing existing local +// configuration or binaries, so --force answers yes without exposing +// configuration contents. --yes only guarantees the installer never blocks on +// a read it cannot satisfy: closed input makes it take its own default, which +// leaves existing configuration in place. +func installerInput(force, assumeYes bool) io.Reader { + switch { + case force: + return strings.NewReader("y\n") + case assumeYes: + return strings.NewReader("") + default: + return os.Stdin + } +} + func download(url, suffix string) (string, error) { client := http.Client{Timeout: 2 * time.Minute} resp, err := client.Get(url) diff --git a/internal/bootstrap/bootstrap_test.go b/internal/bootstrap/bootstrap_test.go index 82e89ee..1fe48d9 100644 --- a/internal/bootstrap/bootstrap_test.go +++ b/internal/bootstrap/bootstrap_test.go @@ -14,6 +14,7 @@ package bootstrap import ( + "io" "os" "path/filepath" "testing" @@ -28,6 +29,28 @@ func TestExtension(t *testing.T) { } } +func TestInstallerInput(t *testing.T) { + if got := readAll(t, installerInput(true, false)); got != "y\n" { + t.Fatalf("force input = %q, want an accepted prompt", got) + } + // --yes must not answer a replacement prompt, only avoid blocking on it. + if got := readAll(t, installerInput(false, true)); got != "" { + t.Fatalf("assume-yes input = %q, want closed input", got) + } + if installerInput(false, false) != os.Stdin { + t.Fatal("interactive input did not use stdin") + } +} + +func readAll(t *testing.T, r io.Reader) string { + t.Helper() + data, err := io.ReadAll(r) + if err != nil { + t.Fatal(err) + } + return string(data) +} + func TestSamePath(t *testing.T) { path := filepath.Join("/tmp", "elastic-docs-utils") if !samePath(path, path) { diff --git a/internal/updates/updates.go b/internal/updates/updates.go index 99183c3..ca6e3c8 100644 --- a/internal/updates/updates.go +++ b/internal/updates/updates.go @@ -15,7 +15,9 @@ package updates import ( + "context" "encoding/json" + "errors" "fmt" "net/http" "os" @@ -31,6 +33,13 @@ import ( const CacheTTL = 24 * time.Hour +// versionProbeTimeout bounds a `--version` call. A tool that starts a server +// instead of reporting its version must not hang the whole command. +const versionProbeTimeout = 10 * time.Second + +// errRateLimited marks the one lookup failure a user can act on directly. +var errRateLimited = errors.New("GitHub API rate limit reached") + type Item struct { Name string `json:"name"` Installed string `json:"installed"` @@ -85,20 +94,29 @@ func Refresh(version string) (Status, error) { func checkDocsBuilder() Item { installed := binaryVersion("docs-builder", "--version") - latest := githubRelease("elastic", "docs-builder") - return compare("docs-builder", installed, latest, "Run `elastic-docs-utils update --component docs-builder`") + latest, err := githubRelease("elastic", "docs-builder") + return compare("docs-builder", installed, latest, err, hints{ + missing: "Not installed. Run `elastic-docs-utils install --with-docs-builder`", + update: "Run `elastic-docs-utils update --component docs-builder`", + }) } func checkVale() Item { installed := binaryVersion("vale", "--version") - latest := githubRelease("errata-ai", "vale") - return compare("Vale", installed, latest, "Update Vale with your package manager") + latest, err := githubRelease("errata-ai", "vale") + return compare("Vale", installed, latest, err, hints{ + missing: "Not installed. Run `elastic-docs-utils install --with-vale`", + update: "Update Vale with your package manager", + }) } func checkValeRules() Item { installed := valeRulesVersion() - latest := githubRelease("elastic", "vale-rules") - return compare("Elastic Vale rules", installed, latest, "Run `elastic-docs-utils update --component vale-rules`") + latest, err := githubRelease("elastic", "vale-rules") + return compare("Elastic Vale rules", installed, latest, err, hints{ + missing: "Not installed. Run `elastic-docs-utils install --with-vale`", + update: "Run `elastic-docs-utils update --component vale-rules`", + }) } func checkSkills() Item { @@ -106,16 +124,22 @@ func checkSkills() Item { if err != nil { return Item{Name: "Elastic Docs skills", Installed: "managed", State: "unknown", Hint: "Run `elastic-docs-utils sync` to refresh skills"} } - latest := githubCommit("elastic", "elastic-docs-skills") + latest, lookupErr := githubCommit("elastic", "elastic-docs-skills") + if lookupErr != nil { + return Item{Name: "Elastic Docs skills", Installed: "managed", State: "unknown", Hint: lookupHint(lookupErr)} + } return skillStatus(current.Skills, latest) } func checkElasticDocsUtils(version string) Item { - latest := githubRelease("elastic", "docs-utils") + latest, err := githubRelease("elastic", "docs-utils") if version == "dev" { return Item{Name: "Elastic Docs Utils", Installed: "local build", Latest: latest, State: "local", Hint: "Builds from a checkout are not compared to releases"} } - return compare("Elastic Docs Utils", version, latest, "Run the installer to update Elastic Docs Utils") + return compare("Elastic Docs Utils", version, latest, err, hints{ + missing: "Run the installer to install Elastic Docs Utils", + update: "Run the installer to update Elastic Docs Utils", + }) } func skillStatus(records map[string]state.SkillState, latest string) Item { @@ -148,69 +172,117 @@ func shortRevision(value string) string { return value } -func compare(name, installed, latest, hint string) Item { - item := Item{Name: name, Installed: installed, Latest: latest, Hint: hint} +// hints carry the next step for each actionable state. A component the user +// never asked for reports the install command rather than the update command, +// so "not installed" does not read as a failed install. +type hints struct { + missing string + update string +} + +func compare(name, installed, latest string, latestErr error, h hints) Item { + item := Item{Name: name, Installed: installed, Latest: latest} switch { case installed == "": - item.Installed, item.State = "not installed", "missing" + item.Installed, item.State, item.Hint = "not installed", "missing", h.missing + case latestErr != nil: + item.State, item.Hint = "unknown", lookupHint(latestErr) case latest == "": - item.State = "unknown" + item.State, item.Hint = "unknown", h.update case semverGT(latest, installed): - item.State = "update available" + item.State, item.Hint = "update available", h.update default: item.State = "current" } return item } +// lookupHint separates "we could not ask GitHub" from "the tool is missing", +// which otherwise both surface as an unexplained unknown. +func lookupHint(err error) string { + if errors.Is(err, errRateLimited) { + return "GitHub API rate limit reached; set GITHUB_TOKEN or retry later" + } + return "Could not reach GitHub to determine the latest version" +} + func binaryVersion(command string, args ...string) string { path, err := exec.LookPath(command) if err != nil { return "" } - out, err := exec.Command(path, args...).Output() - if err != nil { + ctx, cancel := context.WithTimeout(context.Background(), versionProbeTimeout) + defer cancel() + // Some tools report their version on stderr, and others interleave startup + // logging with it, so parse the combined stream and tolerate a non-zero + // exit as long as the tool printed something. + out, err := exec.CommandContext(ctx, path, args...).CombinedOutput() + if err != nil && len(out) == 0 { return "" } return parseVersion(string(out)) } -func githubRelease(owner, repo string) string { - client := http.Client{Timeout: 5 * time.Second} - resp, err := client.Get(fmt.Sprintf("https://api.github.com/repos/%s/%s/releases/latest", owner, repo)) - if err != nil { - return "" +func githubRelease(owner, repo string) (string, error) { + var payload struct { + TagName string `json:"tag_name"` } - defer resp.Body.Close() - if resp.StatusCode != http.StatusOK { - return "" + if err := githubJSON(fmt.Sprintf("https://api.github.com/repos/%s/%s/releases/latest", owner, repo), &payload); err != nil { + return "", err } + return parseVersion(payload.TagName), nil +} + +func githubCommit(owner, repo string) (string, error) { var payload struct { - TagName string `json:"tag_name"` + SHA string `json:"sha"` } - if json.NewDecoder(resp.Body).Decode(&payload) != nil { - return "" + if err := githubJSON(fmt.Sprintf("https://api.github.com/repos/%s/%s/commits/main", owner, repo), &payload); err != nil { + return "", err } - return parseVersion(payload.TagName) + return payload.SHA, nil } -func githubCommit(owner, repo string) string { +func githubJSON(url string, payload any) error { + req, err := http.NewRequest(http.MethodGet, url, nil) + if err != nil { + return err + } + req.Header.Set("Accept", "application/vnd.github+json") + // A token is optional, but the unauthenticated limit is 60 requests per + // hour for the whole machine, which a few checks a day can exhaust. + if token := githubToken(); token != "" { + req.Header.Set("Authorization", "Bearer "+token) + } client := http.Client{Timeout: 5 * time.Second} - resp, err := client.Get(fmt.Sprintf("https://api.github.com/repos/%s/%s/commits/main", owner, repo)) + resp, err := client.Do(req) if err != nil { - return "" + return err } defer resp.Body.Close() if resp.StatusCode != http.StatusOK { - return "" + if rateLimited(resp) { + return errRateLimited + } + return fmt.Errorf("GitHub API returned %s", resp.Status) } - var payload struct { - SHA string `json:"sha"` + return json.NewDecoder(resp.Body).Decode(payload) +} + +func githubToken() string { + for _, name := range []string{"GITHUB_TOKEN", "GH_TOKEN"} { + if value := strings.TrimSpace(os.Getenv(name)); value != "" { + return value + } } - if json.NewDecoder(resp.Body).Decode(&payload) != nil { - return "" + return "" +} + +func rateLimited(resp *http.Response) bool { + if resp.StatusCode != http.StatusForbidden && resp.StatusCode != http.StatusTooManyRequests { + return false } - return payload.SHA + return resp.Header.Get("X-RateLimit-Remaining") == "0" } func valeRulesVersion() string { @@ -236,9 +308,21 @@ func valeRulesVersion() string { return "" } -var versionRE = regexp.MustCompile(`\d+\.\d+\.\d+`) +var ( + versionRE = regexp.MustCompile(`\d+\.\d+\.\d+`) + // A line holding only a version, optionally with build metadata, is the + // reliable signal when a tool logs before reporting its version. + versionLineRE = regexp.MustCompile(`^v?(\d+\.\d+\.\d+)([+-].*)?$`) +) -func parseVersion(value string) string { return versionRE.FindString(value) } +func parseVersion(value string) string { + for _, line := range strings.Split(value, "\n") { + if match := versionLineRE.FindStringSubmatch(strings.TrimSpace(line)); match != nil { + return match[1] + } + } + return versionRE.FindString(value) +} func semverGT(a, b string) bool { if a == "" || b == "" { diff --git a/internal/updates/updates_test.go b/internal/updates/updates_test.go index 779d39c..3d29730 100644 --- a/internal/updates/updates_test.go +++ b/internal/updates/updates_test.go @@ -14,6 +14,10 @@ package updates import ( + "fmt" + "net/http" + "net/http/httptest" + "strings" "testing" "github.com/elastic/docs-utils/internal/state" @@ -54,3 +58,102 @@ func TestSkillStatus(t *testing.T) { t.Fatalf("outdated skill item = %#v", item) } } + +func TestParseVersionPrefersVersionOnlyLine(t *testing.T) { + // A tool that logs before reporting its version must not be misread as + // whichever version-shaped string appears first in the log. + out := "info ::config:: loaded schema 2.0.1 from cache\n1.32.0+b21afa632aad\n" + if got := parseVersion(out); got != "1.32.0" { + t.Fatalf("parseVersion = %q, want 1.32.0", got) + } +} + +func TestParseVersionFallsBackToInlineMatch(t *testing.T) { + if got := parseVersion("vale version 3.17.0\n"); got != "3.17.0" { + t.Fatalf("parseVersion = %q, want 3.17.0", got) + } + if got := parseVersion("v2.2.0"); got != "2.2.0" { + t.Fatalf("parseVersion = %q, want 2.2.0", got) + } +} + +func TestCompareReportsInstallHintWhenMissing(t *testing.T) { + item := compare("docs-builder", "", "1.32.0", nil, hints{missing: "install it", update: "update it"}) + if item.State != "missing" || item.Installed != "not installed" { + t.Fatalf("missing item = %#v", item) + } + if item.Hint != "install it" { + t.Fatalf("hint = %q, want the install hint", item.Hint) + } +} + +func TestCompareStates(t *testing.T) { + h := hints{missing: "install it", update: "update it"} + if item := compare("tool", "1.0.0", "2.0.0", nil, h); item.State != "update available" || item.Hint != "update it" { + t.Fatalf("outdated item = %#v", item) + } + if item := compare("tool", "2.0.0", "2.0.0", nil, h); item.State != "current" || item.Hint != "" { + t.Fatalf("current item = %#v", item) + } +} + +func TestCompareDistinguishesLookupFailureFromMissingTool(t *testing.T) { + h := hints{missing: "install it", update: "update it"} + item := compare("tool", "1.0.0", "", errRateLimited, h) + if item.State != "unknown" { + t.Fatalf("state = %q, want unknown", item.State) + } + if !strings.Contains(item.Hint, "rate limit") { + t.Fatalf("hint = %q, want a rate limit explanation", item.Hint) + } +} + +func TestRateLimited(t *testing.T) { + resp := &http.Response{StatusCode: http.StatusForbidden, Header: http.Header{}} + resp.Header.Set("X-RateLimit-Remaining", "0") + if !rateLimited(resp) { + t.Fatal("exhausted rate limit was not detected") + } + resp.Header.Set("X-RateLimit-Remaining", "12") + if rateLimited(resp) { + t.Fatal("forbidden response with quota left reported as rate limited") + } +} + +func TestGithubJSONSendsToken(t *testing.T) { + var authorization string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + authorization = r.Header.Get("Authorization") + fmt.Fprint(w, `{"tag_name":"v1.2.3"}`) + })) + defer server.Close() + t.Setenv("GITHUB_TOKEN", "secret") + + var payload struct { + TagName string `json:"tag_name"` + } + if err := githubJSON(server.URL, &payload); err != nil { + t.Fatal(err) + } + if authorization != "Bearer secret" { + t.Fatalf("Authorization = %q, want the token", authorization) + } + if payload.TagName != "v1.2.3" { + t.Fatalf("tag = %q", payload.TagName) + } +} + +func TestGithubJSONReportsRateLimit(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("X-RateLimit-Remaining", "0") + w.WriteHeader(http.StatusForbidden) + })) + defer server.Close() + t.Setenv("GITHUB_TOKEN", "") + t.Setenv("GH_TOKEN", "") + + var payload struct{} + if err := githubJSON(server.URL, &payload); err != errRateLimited { + t.Fatalf("err = %v, want errRateLimited", err) + } +} From fb7cb22a70c24eb3f9356173c401652e86ca07a4 Mon Sep 17 00:00:00 2001 From: Fabrizio Ferri-Benedetti Date: Tue, 4 Aug 2026 08:49:13 +0200 Subject: [PATCH 2/4] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- internal/updates/updates.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/internal/updates/updates.go b/internal/updates/updates.go index ca6e3c8..e891de0 100644 --- a/internal/updates/updates.go +++ b/internal/updates/updates.go @@ -203,8 +203,7 @@ func lookupHint(err error) string { if errors.Is(err, errRateLimited) { return "GitHub API rate limit reached; set GITHUB_TOKEN or retry later" } - return "Could not reach GitHub to determine the latest version" -} + return "Could not query GitHub to determine the latest version" func binaryVersion(command string, args ...string) string { path, err := exec.LookPath(command) From ffabb67f92b928b357ac8ff7dc83fbf84e37e5f7 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 4 Aug 2026 06:50:59 +0000 Subject: [PATCH 3/4] fix: avoid update hint when latest version is unknown Co-authored-by: theletterf <1773616+theletterf@users.noreply.github.com> --- internal/updates/updates.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/updates/updates.go b/internal/updates/updates.go index e891de0..681163e 100644 --- a/internal/updates/updates.go +++ b/internal/updates/updates.go @@ -188,7 +188,7 @@ func compare(name, installed, latest string, latestErr error, h hints) Item { case latestErr != nil: item.State, item.Hint = "unknown", lookupHint(latestErr) case latest == "": - item.State, item.Hint = "unknown", h.update + item.State = "unknown" case semverGT(latest, installed): item.State, item.Hint = "update available", h.update default: @@ -204,6 +204,7 @@ func lookupHint(err error) string { return "GitHub API rate limit reached; set GITHUB_TOKEN or retry later" } return "Could not query GitHub to determine the latest version" +} func binaryVersion(command string, args ...string) string { path, err := exec.LookPath(command) From 31fbf230d4418f48852724cec12dd8d011fb35e2 Mon Sep 17 00:00:00 2001 From: Fabrizio Ferri Benedetti Date: Tue, 4 Aug 2026 09:01:24 +0200 Subject: [PATCH 4/4] Fix secondary rate-limit detection, timeout guard, and unknown-version hint - rateLimited: treat any 429 as rate-limited; GitHub's secondary rate limit returns 429 without X-RateLimit-Remaining, so the previous check silently fell through to a generic error and showed a less actionable hint. - binaryVersion: discard output when the context deadline fires; a tool that wrote partial bytes before the 10 s timeout could yield a garbled version string. - compare: add a hint for the latest=="" / err==nil case so a non-semver tag or unexpected payload surfaces an explanation rather than a bare "unknown" with no next step. Addresses Copilot review comment. - checkElasticDocsUtils: use _ for the error in the dev path to make the intentional discard explicit. Co-Authored-By: Claude Sonnet 4.6 --- internal/updates/updates.go | 17 ++++++++++++----- internal/updates/updates_test.go | 19 +++++++++++++++++++ 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/internal/updates/updates.go b/internal/updates/updates.go index 681163e..2bfae5d 100644 --- a/internal/updates/updates.go +++ b/internal/updates/updates.go @@ -132,10 +132,11 @@ func checkSkills() Item { } func checkElasticDocsUtils(version string) Item { - latest, err := githubRelease("elastic", "docs-utils") if version == "dev" { + latest, _ := githubRelease("elastic", "docs-utils") return Item{Name: "Elastic Docs Utils", Installed: "local build", Latest: latest, State: "local", Hint: "Builds from a checkout are not compared to releases"} } + latest, err := githubRelease("elastic", "docs-utils") return compare("Elastic Docs Utils", version, latest, err, hints{ missing: "Run the installer to install Elastic Docs Utils", update: "Run the installer to update Elastic Docs Utils", @@ -188,7 +189,7 @@ func compare(name, installed, latest string, latestErr error, h hints) Item { case latestErr != nil: item.State, item.Hint = "unknown", lookupHint(latestErr) case latest == "": - item.State = "unknown" + item.State, item.Hint = "unknown", "Could not determine the latest version from GitHub" case semverGT(latest, installed): item.State, item.Hint = "update available", h.update default: @@ -217,8 +218,10 @@ func binaryVersion(command string, args ...string) string { // logging with it, so parse the combined stream and tolerate a non-zero // exit as long as the tool printed something. out, err := exec.CommandContext(ctx, path, args...).CombinedOutput() - if err != nil && len(out) == 0 { - return "" + if err != nil { + if len(out) == 0 || ctx.Err() != nil { + return "" + } } return parseVersion(string(out)) } @@ -279,7 +282,11 @@ func githubToken() string { } func rateLimited(resp *http.Response) bool { - if resp.StatusCode != http.StatusForbidden && resp.StatusCode != http.StatusTooManyRequests { + // 429 from the secondary rate limit may not carry X-RateLimit-Remaining. + if resp.StatusCode == http.StatusTooManyRequests { + return true + } + if resp.StatusCode != http.StatusForbidden { return false } return resp.Header.Get("X-RateLimit-Remaining") == "0" diff --git a/internal/updates/updates_test.go b/internal/updates/updates_test.go index 3d29730..2859d2a 100644 --- a/internal/updates/updates_test.go +++ b/internal/updates/updates_test.go @@ -120,6 +120,25 @@ func TestRateLimited(t *testing.T) { } } +func TestRateLimitedTreats429AsRateLimited(t *testing.T) { + // GitHub's secondary rate limit returns 429 without X-RateLimit-Remaining. + resp := &http.Response{StatusCode: http.StatusTooManyRequests, Header: http.Header{}} + if !rateLimited(resp) { + t.Fatal("429 without X-RateLimit-Remaining was not detected as rate limited") + } +} + +func TestCompareHintWhenLatestUnparseable(t *testing.T) { + h := hints{missing: "install it", update: "update it"} + item := compare("tool", "1.0.0", "", nil, h) + if item.State != "unknown" { + t.Fatalf("state = %q, want unknown", item.State) + } + if item.Hint == "" { + t.Fatalf("hint is empty; want an explanation for the unknown state") + } +} + func TestGithubJSONSendsToken(t *testing.T) { var authorization string server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {