Skip to content

Warn when CLI is older than API minimum version#26

Open
andreakiro wants to merge 2 commits intomainfrom
andreakiro/api-version-guard
Open

Warn when CLI is older than API minimum version#26
andreakiro wants to merge 2 commits intomainfrom
andreakiro/api-version-guard

Conversation

@andreakiro
Copy link
Copy Markdown
Member

@andreakiro andreakiro commented Apr 15, 2026

Summary

  • Reads x-notte-min-cli-version response header from the API on every request
  • If the running CLI version is below the minimum, prints a one-time stderr warning with upgrade instructions
  • Prevents the silent data-loss bug where schema mismatches cause empty/null results (e.g. notte sessions network returning 0 batches on v0.0.10 after the S3 migration)

Companion API change: the backend sets x-notte-min-cli-version: 0.0.12 on all responses (separate PR in the monorepo).

Test plan

  • TestCheckMinVersion_WarnsWhenOutdated — warning printed when CLI < min
  • TestCheckMinVersion_SilentWhenCurrent — no output when CLI >= min
  • TestCheckMinVersion_SilentWhenNoHeader — no output when header absent
  • TestCheckMinVersion_SkipsDevVersion — no output for dev builds
  • Full test suite passes (go test ./internal/...)

Greptile Summary

Adds a x-notte-min-cli-version response-header check to every API request and surfaces a one-time upgrade prompt when the running CLI falls below the API-advertised minimum. The approach — sync.Once + package-level globals written in the HTTP transport and read after rootCmd.Execute() returns — is correct for single-goroutine CLI invocations and is well-covered by tests.

Confidence Score: 5/5

Safe to merge; both findings are P2 edge cases that do not affect the primary upgrade-warning path

No P0/P1 defects. The detectOnce consumed-early concern is a theoretical edge case (requires different API endpoints to return different minimums mid-session) and the changelog-URL issue is a minor UX trade-off explicitly acknowledged in the PR description. All four new tests pass and cover the main behavioural branches.

internal/api/client.go — sync.Once consumption order; internal/cmd/root.go — changelog URL target version

Important Files Changed

Filename Overview
internal/api/client.go Adds package-level sync.Once / detectedMinVersion globals plus checkMinVersion on every RoundTrip; logic and concurrency are sound for single-goroutine CLI usage, but once detectOnce fires "not outdated" it is permanently consumed for the session
internal/api/client_test.go Adds four unit tests with proper resetDetectOnce guards; covers outdated, current, no-header, and dev-version cases
internal/cmd/root.go Routes API-triggered warning before GitHub update check; LatestVersion is set to the min-required version rather than the actual latest release, so changelog URL points to the minimum tag

Fix All in Claude Code

Prompt To Fix All With AI
This is a comment left during a code review.
Path: internal/api/client.go
Line: 176-182

Comment:
**`detectOnce` consumed on "not outdated" check**

`detectOnce.Do` is called the first time any response carries the header — including when the CLI is already current. The closure returns early without setting `detectedMinVersion`, but `detectOnce` is permanently spent for the session. If a subsequent request returns a *higher* minimum version (e.g. the API is mid-deploy and different endpoints transiently return different minimums), the warning will be silently skipped.

Consider moving the `IsNewer` call outside `Do` so the once-guard is only consumed when the CLI is genuinely outdated:

```go
outdated, err := update.IsNewer(t.version, minVersion)
if err != nil || !outdated {
    return
}
detectOnce.Do(func() {
    detectedMinVersion = minVersion
})
```

This keeps `detectOnce` unspent until a genuinely outdated response is seen, so a later higher-minimum response can still trigger the warning.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: internal/cmd/root.go
Line: 58-68

Comment:
**Changelog URL points to minimum version, not latest release**

When the API-triggered path fires, `LatestVersion` is set to `minVer` (the API-required minimum). `PrintUpdateNotification` constructs the changelog URL as `https://github.com/nottelabs/notte-cli/releases/tag/v<minVer>`. If the actual latest release is, say, `v0.0.15` while `minVer` is `v0.0.12`, the user is directed to install `v0.0.12` and the GitHub update check is fully bypassed — so they never learn about `v0.0.15`.

If informing users about the true latest release matters here, consider combining both: use `minVer` for the warning message but still check `checker.GetResult()` and surface the actual latest in the changelog URL.

How can I resolve this? If you propose a fix, please make it concise.

Reviews (4): Last reviewed commit: "prompt upgrade instead of just warning o..." | Re-trigger Greptile

Check x-notte-min-cli-version response header and print a
one-time stderr warning when the running CLI is outdated.
Prevents silent data loss from schema mismatches.
Reuse the existing update.PrintUpdateNotification flow so the
user gets an interactive Y/n upgrade prompt (or a non-blocking
notice in non-TTY/agent contexts) when the API returns a
minimum CLI version higher than the running one.
@andreakiro
Copy link
Copy Markdown
Member Author

@greptile

@giordano-lucas
Copy link
Copy Markdown
Member

@greptile I already added something similar in this PR #24. Can you check whether #26 is still needed after that ?

@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Apr 15, 2026

#26 is still needed — the two PRs solve different problems:

The critical distinction is the data-loss scenario in the PR description: when the API makes a schema-breaking change (e.g. the S3 migration), #24 won't help — it only nudges users toward newer versions on a 24-hour cadence and doesn't convey urgency. #26 fires on the specific request that's affected and explicitly signals "your version is too old to work correctly."

The root.go diff in #26 integrates cleanly with #24's PrintUpdateNotification infrastructure and takes priority over the GitHub update check (if minVer := api.DetectedMinVersion(); minVer != "" { ... } else if checker != nil { ... }), so there's no duplication — they're complementary layers.

Tip: You can customize Greptile's behavior for this repo with .greptile/rules and .greptile/config.

@giordano-lucas
Copy link
Copy Markdown
Member

@

@greptile I already added something similar in this PR #24. Can you check whether #26 is still needed after that ?

@greptile we release a new version of the CLI at each notte-api upgrade so the goal of #26 was that it would warn users that a new version is available

@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Apr 15, 2026

#26 is still needed — the two PRs solve different problems:

#24 (merged): Proactive update check via GitHub releases API, cached for 24 hours. Tells users "a newer version exists." May be delayed by the cache TTL.

#26 (this PR): Reactive compatibility enforcement via x-notte-min-cli-version response header. Triggers immediately when the API signals incompatibility — no cache, no GitHub API call. This is what prevents the silent data-loss bug (e.g. notte sessions network returning 0 batches on v0.0.10 after the S3 migration).

The key gap #24 leaves: a user within the 24h cache window on an incompatible version will get no warning from #24, but will silently get broken results. #26 fires the moment any API response carries the header — no cache involved.

The diff in #26 also integrates cleanly with #24's infrastructure — it reuses update.PrintUpdateNotification and takes priority over the GitHub check in root.go, so there's no duplication in the final user-facing output.

Short answer: merge both. #24 = "upgrade available," #26 = "upgrade required now."

@giordano-lucas
Copy link
Copy Markdown
Member

#26 is still needed — the two PRs solve different problems:

#24 (merged): Proactive update check via GitHub releases API, cached for 24 hours. Tells users "a newer version exists." May be delayed by the cache TTL.

#26 (this PR): Reactive compatibility enforcement via x-notte-min-cli-version response header. Triggers immediately when the API signals incompatibility — no cache, no GitHub API call. This is what prevents the silent data-loss bug (e.g. notte sessions network returning 0 batches on v0.0.10 after the S3 migration).

The key gap #24 leaves: a user within the 24h cache window on an incompatible version will get no warning from #24, but will silently get broken results. #26 fires the moment any API response carries the header — no cache involved.

The diff in #26 also integrates cleanly with #24's infrastructure — it reuses update.PrintUpdateNotification and takes priority over the GitHub check in root.go, so there's no duplication in the final user-facing output.

Short answer: merge both. #24 = "upgrade available," #26 = "upgrade required now."

ok but this assumes that we correctly override x-notte-cli-min-version when we do the releases but this will never happen in practice

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants