diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 61769c2..00c597a 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -120,9 +120,21 @@ marks a tool in `m.remoteAnswered` when the version layer reports the pass as conclusive (`RepoData.Conclusive` — the entry is fresh), because an empty `latest` is itself the answer for a repo with neither releases nor tags and the missing-`Latest` clause would otherwise re-dispatch on every cursor visit. The flag has to come from -that layer: `Err` holds `ErrRateLimited` or nil, so an offline start reaches the model -as a nil error, and `m.repoStatus` is no better — a rate-limited pass carrying a stale -card writes a status there too. +that layer, and `Err` cannot stand in for it even now that every failure class arrives +named: a repo with no releases settles the tool with a nil `Err`, while a rate-limited +pass that served a stale card carries one while settling nothing. `m.repoStatus` is no +better — that same pass writes a status there too. A validated token clears the whole +marker set and refetches every tool with a repo, since nothing settled under the old +credential is settled under the new one — unless `GITHUB_TOKEN` is set, in which case +the handler stops right after saving. `SetToken` writes the config file that env +precedence shadows, so the accepted credential is not the one requests will carry, and +recovering on that basis would spend a three-request pass per tool at the anonymous +60/h ceiling. + +The handler's data path is gated on `hasData || err == nil` — on *data*, not on the +error class. A pass that failed but still carried values up from the cache renders +them; gating on `ErrRateLimited` alone meant a 401 or a 5xx blanked a card the pass +had in hand. Two commands in the `Init()` batch belong to no tool: `fetchRateCmd` (seeds the quota gauge — warm-cache starts make no other request) and, on release builds only, @@ -251,6 +263,14 @@ Key invariants: subject that names what is being updated. Under pressure the bar sheds in order of how actionable a thing is: trailing hints, then the gauge, then the version, then the self cell — and the leading hint is truncated rather than allowed to wrap. + A **rejected token** — read from `version.TokenRejected()` at paint time, never + cached on the model, so it cannot disagree with the source and mask the overlay + prints beside it — marks the gauge `api✕` (`Danger` `✕`, one cell under both + runewidth conditions) in every form, since that is where a degraded session is + announced for its whole life. The mark costs a column, so the gauge has a third, + narrowest form — `renderRateMarker`, the mark with no numbers — without which arming + the state at the 80-column baseline pushed the whole gauge off the bar and the + session that most needed announcing showed nothing. All three panels reserve the same `panelFooterRows` and keep the same `panelGutter` — one blank column between the frame and everything the panel draws, which in `[2]` and `[3]` is *all* content: `cardWidth()` and `helpWrapWidth()` are the single definitions @@ -461,13 +481,29 @@ makes the check free. Token: `GITHUB_TOKEN` from the environment always wins ove `/rate_limit` request before being written to disk. - **`doGH(req)`** — the single auth point: headers, the 5-second client, reading the - rate-limit headers of every response. + rate-limit headers of every response. **On a 401 to a request that carried a token + it marks the credential rejected and retries the same request once without it.** An + expired token is strictly worse than no token — the same URLs answer 200 + unauthenticated — so a blackout degrades into a working 60 req/h session instead. + `FetchRateWithToken` deliberately bypasses `doGH`: validation is the one caller that + must observe a 401 rather than survive it, or a dead token would be persisted. +- **The rejection is a value, not a bool** (`rejectedToken`), so it needs no lifecycle: + a new token differs from it, a cleared one is empty, and a bad `GITHUB_TOKEN` that + cannot be unset is suppressed by the same comparison. Only `resolveToken` (whose sole + caller is `doGH`) applies it; `Token`/`TokenSource` read the raw core, so the `[a]` + overlay keeps the source and the mask that identify which credential to replace. The + token file is never deleted. - **The rate-limit snapshot** is updated through `mergeRateObservation`: an "optimistic" observation from `/rate_limit` cannot roll back the per-request header readings within the same window. -- **`ErrRateLimited`** — a typed error for 403/429 with `X-RateLimit-Remaining: 0` - from the response's own headers; the card shows "rate limited — press [a]", - already-loaded data is not erased. +- **The failure taxonomy is closed.** `ErrRateLimited` for 403/429 with + `X-RateLimit-Remaining: 0` from the response's own headers, `ErrTokenInvalid` for a + 401, a generic `HTTP %d` for everything else — "transient" is the absence of a name, + since 5xx, timeout and DNS all mean "try later" and the UI cannot tell them apart. + `RepoData.Err` carries the more actionable of the two core fetches' errors + (`pickFetchErr`: rate limit outranks transient, `errNoReleases` never participates); + it used to carry `ErrRateLimited` or nil, which is how a dead token stayed invisible. + The card shows "rate limited — press [a]" and already-loaded data is not erased. - **The cache** (`cache.json`, 24h TTL): every read-modify-write goes through `updateCacheEntry(repo, mutate)` — under a mutex, re-read from disk, merge, write back; parallel startup goroutines never clobber each other's repositories. `mutate` @@ -547,8 +583,14 @@ instead of `keepkitStyle`; an unknown name forces the glamour construction failu which is how the plain-text fallback is covered). `testAPIBase` is private to `version`, so a `model` test cannot redirect a fetch at an httptest server — a network command is executed there only when the cache can answer it (`seedSelfReleaseCache` for the self-check); -otherwise `Init` batches are asserted by length, never run. The races are real (mutexes -in `version`, `logx`), so tests always run with `-race`: +otherwise `Init` batches are asserted by length, never run. + +Config paths are not the only leakable process-global state. `version.rejectedToken` +is the other: a test that leaves a rejection standing strips `Authorization` from every +later test in the same binary, which is deterministic contamination rather than a +flake. `resetTokenState` clears it on both ends and every test that provokes a 401 goes +through it. The races are real (mutexes in `version`, `logx`), so tests always run with +`-race`: ```bash go test -race ./... diff --git a/CLAUDE.md b/CLAUDE.md index 8ad9d02..0623d0a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -78,7 +78,7 @@ The `model` package is split by responsibility (one package, several files): **Tool probes are sandboxed on two layers** (a tracked tool that ignores its args and boots its own TUI — e.g. a ratatui app answering `--help` with the app itself — must not shred keys' screen): (1) every probe subprocess (`fetchHelpCmd`, `version.InstalledVersion`, `man`) runs through `proc.DetachTTY`, i.e. in its own session with no controlling terminal, so the child's attempt to open `/dev/tty` fails (ENXIO) instead of toggling raw mode/alt-screen on the live terminal; (2) captured output is sanitized before it can reach a viewport — `ui.StripANSI` delegates to `x/ansi.Strip` (full escape grammar: private-mode CSI like `ESC[?1049l`, OSC, DCS — not just SGR) and `cleanTerminalOutput` additionally drops stray control characters (keeps `\n`/`\t`), because anything left in viewport content is re-emitted verbatim by the renderer and flips the real terminal's state. On top of that, `fetchHelpCmd` discards a capture whose RAW bytes carry the alt-screen signature (`isTUITakeover`, `ESC[?1049`): that's a TUI boot plus crash trace, not help text, so the panel falls through to the friendly `No --help output for …` message. `version.InstalledVersion` applies the same guard to its own `--version`/`-V` captures — with its own copy of `isTUITakeover`, since `version` sits below `model` in the import graph — and there it also stops the probe loop and drops the accumulated failure reasons: parsing that capture would mis-read a dependency's version out of the panic trace (`ratatui-0.30.2`), and logging it would re-create a session log on every startup for a tool that is merely a TUI app. -`Init()` fires `fetchInstalledCmd` + (conditionally) `fetchRemoteCmd` for every tool, plus changelog/README for the selected one (the README is what panel `[3]` shows on the first screen, so without that seed the default mode would sit on a placeholder until the selection moved). Two commands in the batch are **not** per tool: `fetchRateCmd` (the gauge seed) and — when `selfCheckEnabled()` — `selfCheckCmd` (keepkit's own release, see **Self-update**), appended right after it and deliberately **not last**, because `TestInitFetchesReadmeForSelected` executes the batch's final element and it must stay the README seed. The `--help` seed is gated on `helpMode == helpModeHelp` — the probe spawns a subprocess, and in the default readme mode its capture would never be rendered (same reasoning as the readme case in `autoFetchCmdsForSelected`). `autoFetchCmdsForSelected()` runs after track/untrack/rename and selection changes (every selection move — `j`/`k`, arrows, pgup/pgdown, search commit/rollback, mouse click — funnels through the shared `selectMeta(idx)` helper in `model.go`, which refreshes the tools list, brief card and help viewport and fires the auto-fetch); it re-fetches the same sources for the selected tool, guarded by the pure predicates `needsInstalled(t)` / `needsRemote(t)` (skip if already cached; `needsRemote` also requires `t.GitHub != ""` and a missing `Latest` or card — **plus a session-scoped `m.remoteAnswered` marker**, written by the `remoteMsg` handler **only when `msg.conclusive`**, because an empty `Latest` can itself be the answer for a repo with neither releases nor tags and the missing-`Latest` clause otherwise re-dispatched the pass on every cursor visit. **`conclusive` comes from the version layer** (`RepoData.Conclusive`, mirrored onto `remoteMsg`) and is the only honest source for it: `Err` carries `ErrRateLimited` or nil, so an offline start and a 5xx both arrive with a **nil error**, and a marker written off `msg.err == nil` stopped the retry for the whole session on exactly the passes that fetched nothing. `m.repoStatus` is no better a proxy — a rate-limited pass carrying a stale card writes `active`/`archived` there. The repeat cost it saves is a goroutine plus a `cache.json` read, **not** API quota — `errNoReleases` is already conclusive on the version side. Rename deletes the old name from it alongside the other per-name maps; untrack deliberately does not, like every sibling map there). If a tool is added or renamed mid-session, this path populates its card without a restart. Rename also deletes the stale old-name entries from `m.repoCards` / `m.versions` / `m.repoStatus` / `m.changelogData` / `m.helpCache` / `m.readmeData` / `m.remoteAnswered` so the tool re-fetches under its new name. +`Init()` fires `fetchInstalledCmd` + (conditionally) `fetchRemoteCmd` for every tool, plus changelog/README for the selected one (the README is what panel `[3]` shows on the first screen, so without that seed the default mode would sit on a placeholder until the selection moved). Two commands in the batch are **not** per tool: `fetchRateCmd` (the gauge seed) and — when `selfCheckEnabled()` — `selfCheckCmd` (keepkit's own release, see **Self-update**), appended right after it and deliberately **not last**, because `TestInitFetchesReadmeForSelected` executes the batch's final element and it must stay the README seed. The `--help` seed is gated on `helpMode == helpModeHelp` — the probe spawns a subprocess, and in the default readme mode its capture would never be rendered (same reasoning as the readme case in `autoFetchCmdsForSelected`). `autoFetchCmdsForSelected()` runs after track/untrack/rename and selection changes (every selection move — `j`/`k`, arrows, pgup/pgdown, search commit/rollback, mouse click — funnels through the shared `selectMeta(idx)` helper in `model.go`, which refreshes the tools list, brief card and help viewport and fires the auto-fetch); it re-fetches the same sources for the selected tool, guarded by the pure predicates `needsInstalled(t)` / `needsRemote(t)` (skip if already cached; `needsRemote` also requires `t.GitHub != ""` and a missing `Latest` or card — **plus a session-scoped `m.remoteAnswered` marker**, written by the `remoteMsg` handler **only when `msg.conclusive`**, because an empty `Latest` can itself be the answer for a repo with neither releases nor tags and the missing-`Latest` clause otherwise re-dispatched the pass on every cursor visit. **`conclusive` comes from the version layer** (`RepoData.Conclusive`, mirrored onto `remoteMsg`) and is the only honest source for it. It was originally so because `Err` carried `ErrRateLimited` or nil, so an offline start and a 5xx both arrived with a **nil error** and a marker written off `msg.err == nil` stopped the retry for the whole session on exactly the passes that fetched nothing. `Err` now names every failure class and the split still holds, in both directions: a repo with no releases **settles** the tool while carrying a nil error, and a rate-limited pass that served a stale card **settles nothing** while carrying one. `m.repoStatus` is no better a proxy — a rate-limited pass carrying a stale card writes `active`/`archived` there. The repeat cost it saves is a goroutine plus a `cache.json` read, **not** API quota — `errNoReleases` is already conclusive on the version side. Rename deletes the old name from it alongside the other per-name maps; untrack deliberately does not, like every sibling map there). If a tool is added or renamed mid-session, this path populates its card without a restart. Rename also deletes the stale old-name entries from `m.repoCards` / `m.versions` / `m.repoStatus` / `m.changelogData` / `m.helpCache` / `m.readmeData` / `m.remoteAnswered` so the tool re-fetches under its new name. **Version comparison** (`version.IsNewer`) is semver via `golang.org/x/mod/semver` with a canonicalization layer (`canonSemver`) that also accepts what strict semver rejects but real tools emit: zero-padded CalVer segments (`2024.01.15`) and 4-segment versions (`1.2.3.4`, truncated to three). Pre-releases order below their release; either side failing to canonicalize means "not newer". @@ -107,16 +107,16 @@ The model is a three-panel layout with focus cycling via `→/←` between `focu - **Panel titles**: all three panels inset a title into their top border (`┌─ ▸ [1] tools 27 3↑ ─…─┐`) via the shared `insetPanelTitle` — an ANSI-safe splice over the already-rendered frame (`ui`'s `truncateVisible` is unexported; the helper repaints the border runs from their `stripANSI` text and drops the title in **already styled**). That is what lets one title carry several colors, which the `[1]` counts are the point of: the tracked count in `Dim`, the update count in `Signal` — the tracker's whole reason to exist, so it is the one thing in a panel title that gets the signal color, and it is absent when nothing is behind. A `panelTitle` carries **both** a plain and a styled form: the plain one is what the border arithmetic measures, because escape sequences are not cells, and a title that does not fit is dropped whole (a chopped title reads worse than none). **Focus is marked twice** — by the accent color and by a `▸` prefix — so it survives a monochrome terminal and a reader who cannot separate the two panel colors; `TestPanelTitleFollowsFocus` pins both signals. The titles are lowercase (`[1] tools`, `[2] brief`, `[3] readme` / `[3] help` / `[3] man`, overridden by `[3] update` while a live log shows and by `[3] update finished` / `[3] update failed` once it ended) and double as the documentation for the digit focus hotkeys, so the status bar carries no digit hints. `[3]` additionally names **the two sources it is not showing** (`· h help · m man`) in the border color: those keys switch what the panel *is*, which is a property of the panel rather than an action on its content — and they are dropped while the update log owns it, since none of the three modes is what is on screen. All title characters are single-width and non-East-Asian-Ambiguous except `▸` (U+25B8, Ambiguous like the list markers, and measured with the plain form either way), keeping the border width math stable. - **Panel footers**: each panel reserves the last `panelFooterRows` (2) of its content height for a **blank** spacer plus a footer line. `[1]` carried a border-colored rule there for a while and it only made the footer read as a fourth section of the list rather than as the frame's own caption. Cells are joined by a **dim** ` · ` — the same painted middot the card's language list uses, since an unpainted one renders at the terminal's default brightness, louder than the hint labels it is separating (`TestPanelFooterSeparatorIsDim`). `[1]` carries `/ filter · enter run · space group` — `enter` is a `[1]` action, not a global one (see **Status bar**), and the three are ordered most-important-first because cells drop from the right and on a narrow list "run" outranks "group"; a right cell that is absent now reserves **nothing**, gap included, which is what had been dropping `[1]`'s last cell one step earlier than the width required. `[2]` carries its own actions led by the contextual `enter update to ` when a release is pending (and deliberately **not** `e note` / `# tags`: both are already offered in the meta line beside the values they edit, and a footer repeating them spends the row on saying it twice), `[3]` the source name, the `page/pages` position, the entry-cursor hints while there is an index to walk, and `ctrl+d/u page` pinned right. Cells are dropped from the right until they fit, exactly like the status bar and for the same reason: a footer that wrapped would push the panel one row past its height, and lipgloss answers that by scrolling the top border off the alt screen. **`calcListHeight()` is the single definition** of the viewport height inside a panel (`calcVpHeight() - footerRows()`), used both by the `WindowSizeMsg` handler that sizes the viewports and by the renderers that stack viewport + footer back to the full height — two copies of that arithmetic would drift and cost a row. On a terminal too short to spare them (`calcVpHeight() < 6`) `footerRows()` is 0 and the panels are content only: two of six rows is a third of the panel, and a footer is a reminder while the content is the point. - **`panelGutter`**: the blank column a panel keeps between its frame and everything it draws — the `[1]` group headers, every footer, and in `[2]` and `[3]` **all** content, at **both** ends. Content that touches the border it lives in reads as having overflowed it, and on the right the gutter is also what keeps text off the scrollbar thumb. The two wide panels reach it from opposite directions and each through a **single definition**: `[2]` sizes itself to **`cardWidth()`** (`briefW - 1 - 2*panelGutter` — the viewport is one column narrower than the panel, `withScrollbar` keeps that one) and `buildCard` steps the finished card in with **`indentLines`** at the very end; `[3]` wraps to **`helpWrapWidth()`** (the same arithmetic) and `renderHelpContent` is a one-line wrapper applying `indentLines` over `helpContent()`, so the spotlight, the search highlight, the placeholders and the update log all land on the same point and no branch can render flush against the frame. The indent is applied **last, to whole finished lines**: it is plain spaces outside the styling, so it can never split an escape sequence, it lands in front of the metrics plate's own background segments rather than inside them, and it cannot shift a line index — which is why `buildCard`'s clickable-link map, built while writing, stays correct for free. `TestPanelsKeepTheirGutter` pins both panels on rendered output. `[1]` and `[2]` additionally open with a blank **row** for the same reason — their content started against the title spliced into the top border. In `[1]` that row is a real screen line, so it goes into the line maps as a non-selectable one and every tool below it shifts by one (which is what the maps exist for, and `syncToolsViewport` walks *up* over every non-selectable row above the selection so a header and the blank above it are revealed together). Tool rows keep the wider `toolRowIndent` (gutter + marker + blank), which puts the `⏺` in the same column as a section header's label and the names one step in from it. -- **Refresh (`r` in `focusBrief`)**: `refreshSelectedCmd(t)` force-refreshes the selected tool bypassing the 24h cache TTL — the repo pass (`refreshRemoteCmd` → `version.RefreshRepoData`) + changelog (`refreshChangelogCmd` → `version.RefreshChangelog`) + README (`refreshReadmeCmd` → `version.RefreshReadme`, preceded by a `delete(m.readmeData, name)` so a session-cached 404/rate-limit negative can recover, then a `markReadmeLoading(name)` — the deletion makes `needsReadme` true again for the whole in-flight window, so without the marker leaving and re-entering the tool would spend a second request; `refreshingFor` does *not* cover it, since `remoteMsg` clears that flag as soon as the repo pass lands, which can be well before the README does) + a local installed re-detect (`fetchInstalledCmd`). It emits the same `remoteMsg`/`changelogMsg` as the startup path, so the merge/re-render logic is reused. While the repo pass is in flight `m.refreshingFor` (the tool name) turns the card title into a status line — `refreshing data ` (`bubbles/spinner`, `MiniDot`; the about is hidden) — with no status-bar takeover; the `remoteMsg` handler clears `refreshingFor` on completion, which reverts the title to name+about and halts the `spinner.TickMsg` loop. `refreshingFor` doubles as the double-press guard; a tool with no `GitHub` only re-detects the installed version (`m.statusMsg = "no repo to refresh"`, no spinner). Note `case "r"` no longer branches at all: it is refresh in `focusBrief` and unbound everywhere else. Rename went global as `m` and the README source moved to `R`, so the key that once meant three things by focus now means one. +- **Refresh (`r` in `focusBrief`)**: `refreshSelectedCmd(t)` force-refreshes the selected tool bypassing the 24h cache TTL — the repo pass (`refreshRemoteCmd` → `version.RefreshRepoData`) + changelog (`refreshChangelogCmd` → `version.RefreshChangelog`) + README (`refreshReadmeCmd` → `version.RefreshReadme`, preceded by a `delete(m.readmeData, name)` so a session-cached 404/rate-limit negative can recover, then a `markReadmeLoading(name)` — the deletion makes `needsReadme` true again for the whole in-flight window, so without the marker leaving and re-entering the tool would spend a second request; `refreshingFor` does *not* cover it, since `remoteMsg` clears that flag as soon as the repo pass lands, which can be well before the README does) + a local installed re-detect (`fetchInstalledCmd`). It emits the same `remoteMsg`/`changelogMsg` as the startup path, so the merge/re-render logic is reused. While the repo pass is in flight `m.refreshingFor` (the tool name) turns the card title into a status line — `refreshing data ` (`bubbles/spinner`, `MiniDot`; the about is hidden) — with no status-bar takeover; the `remoteMsg` handler clears `refreshingFor` on completion, which reverts the title to name+about and halts the `spinner.TickMsg` loop. **`[r]` answers every press**: on completion the handler returns `setStatus(refreshFailedStatus(msg.err))` whenever **`msg.err != nil`** — every path that actually failed to fetch now carries a named error (see `version.pickFetchErr`), while a pass that fetched something stays silent because the repainted card *is* the answer. The predicate is deliberately **not `!msg.conclusive`**, which is a broader thing: it is also false when the version layer refused the ref outright (an unsupported or spoofed host — a bare `RepoData` with a nil `Err`, no request made), which would report a network failure that never happened, and on a partial pass that fetched a new tag and lost only the repo card, where the bar would contradict a card the user just watched update. Before that, success, a rate limit, a 401, a timeout and a dropped connection were one indistinguishable gesture: the spinner turns, the card does not change. The reason is two-tier — `ErrRateLimited` → `refresh failed: rate limited — press [a]` (the one class with an answer the user can act on), everything else → `refresh failed: network error`. There is deliberately **no token wording**: by the time a fetch fails, `doGH` has already retried a rejected token anonymously, so the refresh did not fail *because of* the token, and the gauge and `[a]` overlay report that state anyway. The write sits inside the `msg.toolName == m.refreshingFor` branch, so the background passes `Init` fires — inconclusive all the time on an offline start — never put a "refresh failed" on the bar for a gesture nobody made. `refreshingFor` doubles as the double-press guard; a tool with no `GitHub` only re-detects the installed version (`m.statusMsg = "no repo to refresh"`, no spinner). Note `case "r"` no longer branches at all: it is refresh in `focusBrief` and unbound everywhere else. Rename went global as `m` and the README source moved to `R`, so the key that once meant three things by focus now means one. - **Update (`enter` in `focusBrief`)** — full rationale in **[`docs/design/updating.md`](docs/design/updating.md)**. Installs a newer release from inside the TUI: `enter` is the card's primary action (in `focusTools` the same key runs the tool), requires `hasUpdate(name)`, and reports the shared `updateBusyStatus` while `updatingFor != ""` — one update at a time, no queue. The guard sequence lives in `startToolUpdate()` (**pointer receiver**, it sets a status message). Detection runs off `Update()` in `detectUpdateCmd(t, false)` because it spawns subprocesses. The invariants most often broken (the full list is in the design file): **`updater.Detect`'s chain order is load-bearing twice** — brew before go, pnpm/bun before npm (both layouts carry `node_modules` segments npm claims on sight, and the misdetection installs a duplicate the shadowed copy hides); **an empty manager root disables its own step, enforced inside `underDir`/`segmentUnder`**, never by a repeated `!= ""` guard (`filepath.Rel("", …)` makes a relative path read as living under every disabled root); all five roots come from `managerDirsFrom` and are **symlink-expanded** by the wrapper, because `Detect` compares them against an `EvalSymlinks`-resolved binary path; pnpm needs the cmd-shim's `# cmd-shim-target=` line and an over-cap file is **rejected whole**, never parsed truncated. `update_cmd` always wins; a `LookPath` miss **and** an exhausted chain both fall back to brew-by-name. `acceptsUpdateDetect(msg)` drops a stale result, and `m.updateTarget` is resolved from the *message*, not from the selection at keypress time. Streaming order is fixed by os/exec: scan the pipe to EOF → `cmd.Wait()` → final `updateLine{done:true, err, elapsed}` → `close(ch)`; a `\r` segment sets `replace`; the log caps at ~500 lines; the deadline path uses `proc.KillGroup` (negative pid); `elapsed` is stamped in `startUpdateCmd`, never in `Update()`, where `time.Now()` would make completion non-deterministic in tests. **`showsUpdateLog()` is the single predicate for who owns `[3]`**. A finished session leaves a **terminal block** under the log — `✓ finished · go · 12s` / `✕ failed · brew · 4s`, then the *verified* version once the post-update `installedMsg` lands (`⚠ fd still v10.2.0` is what catches a manager that exited zero having done nothing), then the way out — and the frame follows it (`[3] update` → `[3] update finished` / `[3] update failed`, words rather than glyphs because `insetPanelTitle` measures runes). It is **model state, not log lines**: the buffer is wrapped at render time and would shred styling. `recordUpdateOutcome` is the single writer, shared with the self path and called on **both** results, so neither the block nor the log format can drift between them. **The phase-2 write in the `installedMsg` handler sits after that handler's cursor remap**, never before: its repaint is gated on `showsUpdateLog()` → `selectedMeta()`, and the version merge above is what re-partitions the list the index reads against, so the pre-remap order both skips the repaint for the tool that just updated and paints its log over the tool sitting at its old row. The **buffer itself renders `Dim`** (`dimUpdateLog`) so the block is the only thing on the panel carrying a verdict; the style lands after the wrap, per whole line, and strips nothing — segments are sanitized at the `updateChunkMsg` boundary. - **Self-update (`U`/`X`)** — full rationale in **[`docs/design/self-update.md`](docs/design/self-update.md)**. keepkit watches its own releases and installs one through the very same pipeline as `enter`. **The feature's main case is a keepkit that is not tracked**, so nothing in this path may read `meta.yaml`, the selection or a card — every guard that normally leans on `selectedMeta()` has a self counterpart that does not. The invariants most often broken (the full list is in the design file): **`selfCheckEnabled()` rejects three shapes**, not one — `""` (no `WithAppVersion`), `"dev"`, and anything `isDevVersion` sees as a working copy (a Go pseudo-version tail or any `+` build metadata), which is what keeps `go build .` from offering to `go install …@latest` over itself; **`isSelfUpdate(name)` = the name *and* that gate**, both clauses load-bearing; the `selfCheckMsg` handler writes **only from `selfNone`**, in either direction, so a late message cannot walk back a state the user acted on; `selfState` has **no "updating" member** — that is derived by `selfUpdating()`; **six sites switch on `selfState`** and each enumerates every member and ends in a `default:`, with the `selfStateCount` sentinel driving `TestSelfStateSitesAreExhaustive`; the failure branch of `updateDoneMsg` writes **no `selfState` at all** (the banner returns by itself once `updatingFor` clears, and any write there could only walk a state back); `[U]` checks `selfNone` **before** the busy guard, or a dev build answers `another update is running` for a surface documented as absent. Restart happens strictly **after `p.Run()` returns** (Bubble Tea has restored the terminal by then), and `resolveSelfPath`'s order matters: an argv0 carrying a separator wins when it exists, a bare argv0 goes through `lookPath` **first** because Linux's `/proc/self/exe` can still name the old binary after an upgrade, and `sameProgram` rejects a `PATH` hit whose base differs. - **Panel `[3]` modes (`helpMode`)** — full rationale in **[`docs/design/readme-pipeline.md`](docs/design/readme-pipeline.md)**. Three sources: `helpModeReadme = 2` (the **default** set in `New()`), `helpModeHelp = 0`, `helpModeMan = 1`. `helpMode` is a sticky global field, not per tool; `[R]`/`[H]`/`[M]` switch it from `focusBrief || focusHelp` through the shared **`switchHelpMode(mode)`** (sets the mode, dismisses a *completed* update log, `setHelpContent()` + `GotoTop()`, returns the fetch command for the mode's missing source). The trio is **capitals as a set** so none collides with a lowercase verb (`r` is `[2]`'s refresh, `m` the global rename). The invariants most often broken (the full list is in the design file): **`m.helpCache` is a `map[string][2]string` indexed by `helpMode`, so mode 2 panics on every index site** — README content lives in `m.readmeData` and each index site (`rawHelpText`, `renderHelpContent`, `autoFetchCmdsForSelected`) carries a readme early-return *before* the array read; a **live** update log keeps `[3]` in every path (its branch sits ahead of both the readme branch and the `No tool selected` guard). Rendering is `cleanTerminalOutput` → **`cleanReadmeMarkdown`** (readme_clean.go) → glamour with `keepkitStyle`, and a glamour failure falls back to the **preprocessed** text, never to the raw one. In the preprocessor: **code is never rewritten** (fenced blocks segmented, inline spans NUL-masked), CRLF is normalized on entry (a `\r`-suffixed closing fence protects the rest of the file), images run before links, autolinks and bare URLs are left alone, both reference forms are gated on labels collected **document-wide after** the HTML rules, HTML tag names come from the fixed `rcHTMLNames` allowlist whose trailing `\b` is what makes the ~80-branch alternation order-independent, and **`rcLineContent` slices rather than `ReplaceAllString`** — the replace form froze the whole TUI for 8.4 s on a 512 KiB adversarial README. `keepkitStyle` **clones** the glamour globals and assigns a **fresh pointer** per override, because `styles.DefaultStyles` aliases the same structs and writing through a cloned one restyles glamour process-wide. Dark/light is resolved **once at construction** into `m.darkBG` — `glamour.WithAutoStyle()` probes the terminal with an OSC query that races Bubble Tea's input reader. - **Help navigation (`j`/`k` in `focusHelp`)**: `[3]` is navigable per *entry* — a flag or subcommand line plus its indented description block. `parseHelpEntries(raw, width)` (textutil.go) detects entries heuristically on the **pre-wrap source lines** (flag start = the `helpTokenRe` flag core at the trimmed line start; subcommand start = `helpEntrySubcmdRe`, an indented non-dash word + 2+ spaces + text — the word class excludes `.` so justified man prose like `tree. See also…` doesn't match; continuation = `continuesEntry`: any deeper-indented non-header line — including deeper lines that *begin* with a flag token (`…overridden with\n --no-ignore.`) — plus blank lines whose next non-blank line still continues, so multi-paragraph descriptions stay one entry; the entry ends at a section header or the next line at the entry's own indent or shallower) and maps the ranges to wrapped display-line indices via `wrapLine` — the same code `wrapText` uses, which is the point: `wrapText` rebuilds wrapped lines from `strings.Fields` (indentation is lost), so parsing wrapped output would break the indent heuristic, and sharing the wrap algorithm plus the single `helpWrapWidth()` (`max(helpW-1-2*panelGutter, 20)` — the viewport is a column narrower than the panel and a gutter is held at each end) keeps entry indices in lockstep with what the viewport shows. `isHelpSectionHeader` is the one definition of a header, used by both `colorizeHelp` and the parser. State is `m.helpEntries []entryRange` + `m.helpNavIdx` (−1 = off) + `m.helpBase` — the wrapped+colorized full-color content, cached because cursor moves repaint per keystroke and must not re-run the colorize regex over a whole man page (`helpContent`'s normal path serves `applySpotlight(helpBase)`, and `renderHelpContent` is the one-line wrapper that steps the result in by `panelGutter`; the base is built directly in `setHelpContent`, not via the renderer, which serves that very base back through `applySpotlight`). **`setHelpContent()` is the single recompute point** — every site where the *visible* text changes (selection via `autoFetchCmdsForSelected`, `[R]`/`[H]`/`[M]`, `helpOutputMsg` — gated on `msg.mode == m.helpMode`, a late fetch for the hidden mode must not reset the cursor, `readmeMsg` for the selected tool while in readme mode, resize — only when `helpWrapWidth()` actually changed, so a height-only resize keeps the cursor and a width change re-renders the README, update-log start) goes through it: recompute entries (empty for the update log, readme mode, `helpLoadingFor` and placeholders — `j`/`k` stay plain scroll there), reset the cursor, repaint, never scroll. Style-only repaints (per-chunk log appends, cursor moves) call `SetContent(renderHelpContent())` directly and must not reset the cursor. Interaction: **only the letter keys navigate — `↑`/`↓` keep their 3-line scroll** so prose between/after entries stays keyboard-reachable; the first `j`/`k` lands via `helpNavStart(delta)` on the first entry intersecting the window, or (none visible) the nearest entry in the movement direction; later presses step clamped without wrap; `applySpotlight` (render.go) dims every line outside the current entry (`Styles.Dim.Render(stripANSI(line))` — the `[a]` overlay's strip-then-repaint trick per whole line) while the entry keeps full `colorizeHelp` color; `scrollToNavEntry` keeps it in view with mutually exclusive branches and a `min(end-Height, start)` clamp so a taller-than-window entry pins its start to the top. `esc` is two-stage: cursor off first (scroll kept), focus walk second. `PgUp`/`PgDn`/`g`/`G`/wheel stay pure scroll and never touch the cursor. Every path that deactivates navigation (esc, any `setFocus` move) goes through `clearHelpNav()`, which pairs the reset with the repaint — clearing the index without repainting leaves stale dimming. The `focusHelp` bar shows `[j/k] navigate` alongside `[↑↓] scroll` when entries exist and prepends `[esc] exit nav` while the cursor is on. - **Tracking verbs are global**: `t` track (add by GitHub URL or plain name → `modeTrack`), `u` untrack (with confirmation → `modeConfirmUntrack`), `m` rename (fix the binary name when the repo name differs → `modeRename`). All three fire in **every focus** and are three of the six keys `globalHints` puts on the status bar. They used to be `[1]`-only because each collided with a `[2]` action (`t` tags, `u` update, `r` refresh); the redesign moved those onto keys of their own (`#` tags, `enter` update) and freed all three to mean one thing everywhere, which is what lets the bar carry a single focus-independent list instead of three per-focus ones. Rename is **`m`, not `R`**: `R` is panel `[3]`'s readme source now, and the tracker's verbs are the lowercase set. That leaves lowercase `r` meaning **refresh in `[2]` and nothing else** — it used to double as `[3]`'s readme switch, so one key meant "spend three requests" or "swap the panel's source" depending on a focus the `[3]` title did not mention. Each mode has a handler in `mode.go` and a matching branch in `renderStatusBar()`, mirroring the `modeEditNote`/`modeEditTags` input pattern. Mutations go through `loader.UpsertMeta`/`RemoveMeta`, persist via `loader.SaveMeta`, then rebuild `m.tools = loader.ToolsFromMeta(m.meta)` and refresh the viewport. - **Run (`enter` in `focusTools`)**: launches the selected tool without leaving keepkit. `enter` fires only in `modeNormal`+`focusTools` (empty list → no-op; in `modeSearch` enter stays the commit key) and opens `modeRunInput` — a one-line prompt (`m.runInput`, its own textinput like `m.search`, not shared with note/tags) prefilled with `m.lastRun[name]` else the tool name, cursor at end; the status bar echoes `run : [enter] run [esc] cancel`. `m.lastRun map[string]string` is session-only per-tool memory of the last dispatched command — rename's stale-state cleanup deletes the old-name entry alongside `helpCache` et al.; untrack deliberately leaves it (harmless, session-scoped). Enter with empty/whitespace input cancels like `esc`. On dispatch `launcher.Detect(command, name)` picks the path — env-only, so unlike every probe it is safe inside `Update()`: a tab plan runs its `Argv` via `startLaunchCmd` (`exec.Command` + `proc.DetachTTY`, `launchTimeout` — a 10s **var**, shrunk by the timeout/KillGroup test — with `proc.KillGroup` on expiry, `safeCmd`-wrapped) → `launchDoneMsg{toolName, command, err}`, with `m.launchingFor` (the launch twin of `updatingFor`) as the one-adapter-launch-at-a-time guard and a `launching in …` statusMsg as in-flight feedback (this is also `Plan.Terminal`'s consumer); a `Fallback` plan runs `execToolCmd` → `tea.ExecProcess` over `shellCommand(runtime.GOOS, cmd)` (`sh -c` / `cmd /c`; goos-parameterized and spawn-free like `browserCommand`, so both branches are table-testable) — keepkit suspends, Bubble Tea restores the terminal when the tool exits → `execDoneMsg{toolName, err}`. **Auto-fallback**: an adapter failure (kitty remote control off, Automation permission denied) must not strand the launch — the `launchDoneMsg` error handler sets `statusMsg` (`tab open failed — running here`) and returns `execToolCmd(msg.toolName, msg.command)`; `command` rides the msg so the handler never re-reads input state. The auto-fallback is **gated on `modeNormal`**: the result can arrive up to `launchTimeout` after enter (osascript blocked on the macOS Automation dialog), and `tea.ExecProcess` seizing the terminal under an open editor/overlay would route keystrokes to the spawned shell — under any other mode the fallback is **deferred**, not dropped: the gate stores `m.pendingLaunchName`/`Command` and `flushPendingLaunch` (mode.go) dispatches `execToolCmd` with the same statusMsg (single definition: `launchFallbackStatus`, shared with the ungated auto-fallback) on the keystroke that returns the mode to `modeNormal` (every modal return in `Update` funnels through it — the mode-dispatch switch plus the inline `modeSearch` exit; `modeTokenInput`'s esc lands on `modeAPIStatus`, so the flush waits for the overlay to actually close). A statusMsg set at gate time would be dead UI — every open mode's `renderStatusBar` branch outranks the statusMsg branch, and the blanket `statusMsg = ""` reset on `tea.KeyMsg` fires on the very keystroke that closes the mode; setting it in the flush (after both) is what makes the failure visible. The flush goes straight to `execToolCmd` — never back through `launcher.Detect` — so a known-failing adapter plan is never re-run; a new dispatch from `modeRunInput`'s enter drops a pending fallback first (flushing both on one keystroke would run two commands), and confirming untrack of the pending tool itself drops it too — the dialog-closing enter must not exec the now-untracked tool's command (a pending fallback for a *different* tool deliberately survives the untrack and flushes on that keystroke). The handler clears `launchingFor` first in all outcomes. Working directory differs by path: a tab opens in the new shell's default cwd, the ExecProcess fallback inherits keepkit's. The timeout carries one accepted race: an adapter killed after its tab command already executed (osascript stuck post-`write text`) still triggers the fallback, so the command can run twice — narrow, undetectable, and better than stranding genuine failures. This path also serves native Windows: `planFor` is env-only, so WezTerm there yields a doomed `sh -c` plan whose failure lands in the fallback (one noisy attempt accepted; a `GOOS` guard in `planFor` is deliberate YAGNI). Success wording is mode-neutral — `launched ` — because Terminal.app and tmux open a *window*, not a tab. **No `logx` anywhere in the flow**: a non-zero tool exit (`statusMsg " exited: "`) is the tool's business, not a keepkit anomaly, and an adapter error is a degraded path, not a malfunction (the auto-fallback still launches the tool). Launch during a running update is deliberately not blocked — independent concerns; ExecProcess pauses rendering of the live update log and the buffer catches up on resume. A not-installed tool launches anyway (no PATH pre-check): in a tab `sh` reports `command not found` inside that tab, while on the ExecProcess path the shell's not-found exit (`notFoundExit`: 127 sh / 9009 cmd.exe) maps to `statusMsg " not found — is it installed?"` instead of the cryptic raw exit status. The `[?]` overlay's tools group carries `enter — run in tab` (desc kept short — the overlay sits at the 76-col edge of its budget). -- **Status bar** (`renderStatusBar()`) is **global** — the same line in every focus, instead of the three per-focus lists it used to rewrite itself between as the user moved panels. A key belongs on it exactly when it does the same thing in **every** focus, and six do: `t track · u untrack · m rename · a api · ? keys · q quit` (`globalHints`). `enter` and `/` used to lead that list and are precisely the two that failed the definition — `enter` runs a tool in `[1]`, installs a release in `[2]` and is unbound in `[3]`; `/` filters the list in `[1]` and is unbound in `[2]`/`[3]` — so the bar either advertised a key that does nothing in two of the three panels, or (in `enter`'s case) one meaning while being wrong about the others. Both moved to `[1]`'s footer beside `space group`, next to the panel where their meaning is fixed, which is the same rule everything else panel-local follows (see **Panel footers**). Key hints lost their brackets — `m.key(k)` renders the bare key in the accent, and `m.hint(k, label)` pairs it with its word **in `Dim`**, a deliberate two-step contrast (the key is the only part of a hint the eye needs to find, so the word beside it steps back rather than competing at reading brightness), so the key/label contrast is defined once; the accent already says "this is a key", and at the bar's density a pair of brackets per hint cost more columns than the whole `[?]` overlay's third column. Literal brackets survive only where they name a *panel* (`[3] readme`), which is now the only bracketed thing on screen. In the three normal focus states `renderHintsBar` lays the line out as **three zones**: **`appVersionCell()`** pinned to the **left** edge, the hint cells **centered on the bar**, and the **API-usage gauge** pinned to the **right** edge. The two edges are the two facts that hold regardless of what the user is doing — which build this is, and how much quota is left — so they sit where the eye returns rather than inside a key list; centering the keys between them is only honest because those keys no longer change with focus. Centering is measured against the **whole bar**, not against the leftover band between the edges (the block must read as centered on screen, and the two edges are rarely the same width), then clamped into that band, so on a narrow terminal it is pushed up against the version cell rather than overlapping it. The collapsed **self-update cell** rides with the version on the left: it carries its action alone (`U update`) and `appVersionCell` is the subject naming what is being updated — split across the two edges, the verb would lose its noun. `appVersionCell` is keepkit's own identity line — `keepkit v0.1.0` in `Dim`, the one fact about the app that is true on every frame and the answer to "which build is this?" without opening `[?]`; when a newer release is known and still uninstalled (`selfUpdateAvailable()`, the sixth `selfState` site) the version takes `SignalBold` and the same ` ↑` an outdated tool row carries, so the app announces its own update in the vocabulary it announces everyone else's. The version shown stays the **running** one — the offer is that this build is behind, not a preview of what it would become — and once the update is installed the arrow goes (the pending action is a restart, which the self cell says). It **replaced the pending-updates count** that used to sit here: that count was the third surface for the same number (the `[1]` title carries it beside the tracked count, and every outdated row carries its own `↑`), while the running version had none outside an overlay; `updateCount()` still feeds the `[1]` title. Under width pressure the bar sheds in order of how *actionable* the thing dropped is: trailing hint cells first (they are ordered most-important-first, so `? keys` and `q quit` go before `t track`), then the gauge (read-only), then the version cell, then the self cell — and the leading hint cell is never dropped, only truncated. That order is spelled out step by step rather than computed, because "what goes next" is a judgement about meaning, not about width. `TestAppVersionCell` pins the cell across all five self states plus a tagless offer. **The gauge is on screen for the whole session** once the rate is known (`gaugeVisible`): it is also the only visible sign that keepkit has an API surface at all, so hiding it at rest hid the `[a]` overlay along with it — the numbers are the affordance, and `a api` sits in the global key list beside them. Its fill turns `Danger` under `min(gaugeDangerRemaining, limit/4)` remaining, read against the limit as well as absolutely, because an absolute-only bound would paint a token-less user's bar permanently red — the opposite of a signal. It renders as `api ▮▮▮▮░░░░░░░░ 45/60` in the corner, a fixed 12-cell bar of foreground-colored `▮` fill / `░` track glyphs (not painted backgrounds, so it survives a degraded color profile; both glyphs are deliberately non-East-Asian-Ambiguous, where a `█` would measure two cells and break the alignment math), degrading to `api 45/60` and then to nothing. `gaugeFilled` clamps the rounded ratio so any usage shows at least one `▮` and a full bar means exhaustion only. The bar spends **no columns advertising `[a]`** — the `[?]` overlay documents it. Input/modal states show no right group at all. **The bar must stay one line**: `Styles.StatusBar` is width-constrained, so a hint list wider than `m.width-2` wraps to a second row and `View()` returns `m.height+1` lines — one row past the terminal, which scrolls the top border off the alt screen. `renderHintsBar` therefore takes the hints as `[]string` cells ordered most-important-first and drops them from the right until they fit, reserving the two non-gauge right-group members first. The **leading** cell is never dropped, so one cell wider than the whole bar is possible — the fused self banner is 37 cells and overflows below ~39 columns — and that case is **truncated** (ANSI stripped first, since a cut inside an escape sequence would be re-emitted to the terminal verbatim) rather than allowed to wrap. `TestStatusBarNeverWraps` pins the invariant at the 80×24 baseline and, for the banner, down to 24 columns. Below 80 columns only the *bar* is checked: the three panels have their own minimums (15+30+30 plus borders), so the layout overflows there on its own — a separate, pre-existing limit. +- **Status bar** (`renderStatusBar()`) is **global** — the same line in every focus, instead of the three per-focus lists it used to rewrite itself between as the user moved panels. A key belongs on it exactly when it does the same thing in **every** focus, and six do: `t track · u untrack · m rename · a api · ? keys · q quit` (`globalHints`). `enter` and `/` used to lead that list and are precisely the two that failed the definition — `enter` runs a tool in `[1]`, installs a release in `[2]` and is unbound in `[3]`; `/` filters the list in `[1]` and is unbound in `[2]`/`[3]` — so the bar either advertised a key that does nothing in two of the three panels, or (in `enter`'s case) one meaning while being wrong about the others. Both moved to `[1]`'s footer beside `space group`, next to the panel where their meaning is fixed, which is the same rule everything else panel-local follows (see **Panel footers**). Key hints lost their brackets — `m.key(k)` renders the bare key in the accent, and `m.hint(k, label)` pairs it with its word **in `Dim`**, a deliberate two-step contrast (the key is the only part of a hint the eye needs to find, so the word beside it steps back rather than competing at reading brightness), so the key/label contrast is defined once; the accent already says "this is a key", and at the bar's density a pair of brackets per hint cost more columns than the whole `[?]` overlay's third column. Literal brackets survive only where they name a *panel* (`[3] readme`), which is now the only bracketed thing on screen. In the three normal focus states `renderHintsBar` lays the line out as **three zones**: **`appVersionCell()`** pinned to the **left** edge, the hint cells **centered on the bar**, and the **API-usage gauge** pinned to the **right** edge. The two edges are the two facts that hold regardless of what the user is doing — which build this is, and how much quota is left — so they sit where the eye returns rather than inside a key list; centering the keys between them is only honest because those keys no longer change with focus. Centering is measured against the **whole bar**, not against the leftover band between the edges (the block must read as centered on screen, and the two edges are rarely the same width), then clamped into that band, so on a narrow terminal it is pushed up against the version cell rather than overlapping it. The collapsed **self-update cell** rides with the version on the left: it carries its action alone (`U update`) and `appVersionCell` is the subject naming what is being updated — split across the two edges, the verb would lose its noun. `appVersionCell` is keepkit's own identity line — `keepkit v0.1.0` in `Dim`, the one fact about the app that is true on every frame and the answer to "which build is this?" without opening `[?]`; when a newer release is known and still uninstalled (`selfUpdateAvailable()`, the sixth `selfState` site) the version takes `SignalBold` and the same ` ↑` an outdated tool row carries, so the app announces its own update in the vocabulary it announces everyone else's. The version shown stays the **running** one — the offer is that this build is behind, not a preview of what it would become — and once the update is installed the arrow goes (the pending action is a restart, which the self cell says). It **replaced the pending-updates count** that used to sit here: that count was the third surface for the same number (the `[1]` title carries it beside the tracked count, and every outdated row carries its own `↑`), while the running version had none outside an overlay; `updateCount()` still feeds the `[1]` title. Under width pressure the bar sheds in order of how *actionable* the thing dropped is: trailing hint cells first (they are ordered most-important-first, so `? keys` and `q quit` go before `t track`), then the gauge (read-only), then the version cell, then the self cell — and the leading hint cell is never dropped, only truncated. That order is spelled out step by step rather than computed, because "what goes next" is a judgement about meaning, not about width. `TestAppVersionCell` pins the cell across all five self states plus a tagless offer. **The gauge is on screen for the whole session** once the rate is known (`gaugeVisible`): it is also the only visible sign that keepkit has an API surface at all, so hiding it at rest hid the `[a]` overlay along with it — the numbers are the affordance, and `a api` sits in the global key list beside them. Its fill turns `Danger` under `min(gaugeDangerRemaining, limit/4)` remaining, read against the limit as well as absolutely, because an absolute-only bound would paint a token-less user's bar permanently red — the opposite of a signal. It renders as `api ▮▮▮▮░░░░░░░░ 45/60` in the corner, a fixed 12-cell bar of foreground-colored `▮` fill / `░` track glyphs (not painted backgrounds, so it survives a degraded color profile; both glyphs are deliberately non-East-Asian-Ambiguous, where a `█` would measure two cells and break the alignment math), degrading to `api 45/60`, then to `api✕` when there is a rejection to report, and then to nothing. **A rejected token adds a `Danger` `✕` to the label** (`api✕ ▮▮▮…`) in every form — this is the primary place a degraded session is announced, and the compact form is the one a narrow bar keeps, which is exactly where bare numbers explain themselves least. It is a suffix rather than a relabel to `anon` because a user with no token is also anonymous and that is not degradation; the `✕` says a credential was **refused**, and the limit beside it says what is left. `rejectedTokenGlyph` is U+2715, one cell under **both** runewidth conditions (U+00D7 would be two under `RUNEWIDTH_EASTASIAN=1`), which the right-edge arithmetic requires. That one column is also why **`renderRateMarker()`** exists as a third, narrowest form: at the 80×24 baseline the marked gauge no longer fits beside the six hints, so without it arming the degraded state removed the announcement entirely — the session that most needs announcing was the one showing nothing. What survives follows the bar's own shed rule (drop the least actionable thing first): the numbers are a measurement the `[a]` overlay repeats, the `✕` is the only sign anywhere on screen that requests stopped carrying the user's token. It returns `""` when there is no rejection, so a healthy bar gains no form it did not have before. `gaugeFilled` clamps the rounded ratio so any usage shows at least one `▮` and a full bar means exhaustion only. The bar spends **no columns advertising `[a]`** — the `[?]` overlay documents it. Input/modal states show no right group at all. **The bar must stay one line**: `Styles.StatusBar` is width-constrained, so a hint list wider than `m.width-2` wraps to a second row and `View()` returns `m.height+1` lines — one row past the terminal, which scrolls the top border off the alt screen. `renderHintsBar` therefore takes the hints as `[]string` cells ordered most-important-first and drops them from the right until they fit, reserving the two non-gauge right-group members first. The **leading** cell is never dropped, so one cell wider than the whole bar is possible — the fused self banner is 37 cells and overflows below ~39 columns — and that case is **truncated** (ANSI stripped first, since a cut inside an escape sequence would be re-emitted to the terminal verbatim) rather than allowed to wrap. `TestStatusBarNeverWraps` pins the invariant at the 80×24 baseline and, for the banner, down to 24 columns. Below 80 columns only the *bar* is checked: the three panels have their own minimums (15+30+30 plus borders), so the layout overflows there on its own — a separate, pre-existing limit. - **Status-message lifecycle**: `m.statusMsg` (rendered by `renderStatusBar`, which outranks both the self-update banner and the hints bar whenever non-empty — the banner is not modal, and a transient status covering it for `statusMsgTTL` is intended) has two clears. (1) **Immediate**: the blanket `m.statusMsg = ""` on every `tea.KeyMsg` — any keypress wipes the current status at once. (2) **TTL auto-expiry**: every *transient* status is set via **`setStatus(s) tea.Cmd`** (model.go), which bumps a generation counter `m.statusSeq`, sets the message, and returns `tea.Tick(statusMsgTTL, …)` producing a `statusExpiredMsg{seq}` stamped with the current seq; the `Update` handler clears the message only when `msg.seq == m.statusSeq`, so a stale timer from a message already superseded by a newer one is a harmless no-op. `statusMsgTTL` is a **var** (`1 * time.Second`), shrunk by tests the same way as `launchTimeout` (the tick captures the value at construction, so the shrink must precede the `Update`). The returned `tea.Cmd` must be **batched** into whatever the caller already returns (`tea.Batch`); callers that only set a status return it directly. **In-flight** statuses (`launching in …`, `still launching `) are the deliberate exception and go through **`setStickyStatus(s)`** — no timer, but it **still bumps `statusSeq`**: they report work still in progress and are extinguished by `launchDoneMsg`, not the clock, so letting a timer expire one mid-flight would hide the only sign the adapter is busy for up to `launchTimeout`. The bump is the whole point of the helper rather than a plain assignment — a transient status set within the last `statusMsgTTL` (the group toggle, say) has a tick in flight whose seq would otherwise still match the sticky message and wipe it (`TestInFlightStatusSurvivesStaleExpiry`). `TestStatusExpired*`/`TestSetStatus*` pin the mechanism; `assertOnlyExpiryTick` is the test helper that asserts a site returns *only* the expiry tick (no fetch/exec rode along). -- **API-status overlay (`a`)**: opens a read-only view of the GitHub rate limit and token (source, masked value, used/limit with threshold icon, reset time) with token entry/removal/refresh. When no token is configured it leads with an `Add a GitHub token…` nudge (hidden once a token exists or while entering one). It is `modeAPIStatus` (token entry: `modeTokenInput`) with a matching `renderStatusBar()` branch; `a` fires only in `modeNormal`; `esc` **or `q`** closes it. See the GitHub API section for the data flow. +- **API-status overlay (`a`)**: opens a read-only view of the GitHub rate limit and token (source, masked value, used/limit with threshold icon, reset time) with token entry/removal/refresh. When no token is configured it leads with an `add a github token…` nudge; a **rejected** one gets `replace the token to restore the 5000/h limit` instead — "add" reads as advice to someone who has already done it, and the thing to do is swap the credential, not create one. A rejected **env** token gets a third wording, `GITHUB_TOKEN was refused — replace it in your shell`, and offers **no key at all**: `[e]` runs `SetToken`, which writes the config file, and `effectiveToken` reads that only when `GITHUB_TOKEN` is empty — so the key cannot fix this one, and the variable belongs to the shell that launched keepkit. The env arm must stay **first** in the switch or the config wording swallows it; `[d]` two blocks down has gated on the source all along for the same reason. All three are hidden while entering a token, when the open input is already the answer. A rejection also rewrites the token line itself to `token () — rejected (HTTP 401)` in `Danger`, with `requests run unauthenticated` under it: this is the surface that **always** has the answer, since the gauge's `✕` is droppable under width pressure and invisible before the first rate snapshot. **The mask is load-bearing** — it is how the user recognises which credential to replace — and it survives only because `version.Token()` reads the raw `effectiveToken()` core rather than the suppressed `resolveToken()`. It is `modeAPIStatus` (token entry: `modeTokenInput`) with a matching `renderStatusBar()` branch; `a` fires only in `modeNormal`; `esc` **or `q`** closes it. See the GitHub API section for the data flow. - **Overlay compositing**: two overlays composite over the layout via `ui.PlaceOverlay` (a centered fg-over-bg compositor), gated by the shared `overlayVisible()` predicate in `View()` — the `[a]` API-status overlay (`renderAPIStatus`) and the `[?]` hotkeys overlay (`renderHotkeys`), picked by `m.mode`. `PlaceOverlay` dims the whole visible background — original styling is stripped and repainted with `OverlayDimStyle` (`ColorDim`) — so the modal is the only full-color element. Covered rows get the dim inside `overlayLine` *after* `truncateVisible`/`dropVisible`, because those helpers `StripANSI` the bg and would erase a pre-applied dim from the modal's side margins. - **Mouse policy** (`handleMouse` in `render.go`, dispatched from `Update()` before the mode switch, gated inside the function): wheel scrolling works in every mode; while any overlay is visible (`overlayVisible()` — `[a]` API status or `[?]` hotkeys) all mouse input is a no-op, and before the first `WindowSizeMsg` (`!m.ready`) too. Clicks that change selection or focus fire only in `modeNormal` — otherwise a click would move `selectedMeta()` under an open note/tags/rename editor and retarget the commit. A click that changes the selected tool goes through the same `selectMeta` helper as the keyboard `j`/`k` path, including the auto-fetch; a click anywhere in the tools panel (row or empty area) focuses it via `setFocus`, matching brief/help — the same helper the keyboard uses, so a click cannot leave the list painted with stale focus styling. Both panels translate the click row the same way — **through `panelRow(msg.Y, vp.Height)`**, then `+ vp.YOffset`. X alone does not mean "inside a panel": the outer `Margin(1,0)` row, the two borders and the status/hints bars all share the panels' columns, and with a scrolled viewport an unbounded `msg.Y - 2` maps that chrome onto real content — a click on the blank top row would open a card link. `panelRow` returns `-1` outside the viewport's rows; inside, `[1]` goes through `toolAtLine()` (a group header maps to `-1` and selects nothing) and `[2]` through `buildCard()`'s link index — a click on the `repo:` line or the changelog release URL returns `openURLCmd(url)`, every other line only moves focus. @@ -144,6 +144,8 @@ The base dir is resolved by `configdir.Base()`: `~/.config/keepkit` on **macOS a The internal `testConfigDir`/`testCacheDir`/`testTokenDir`/`testAPIBase`/`testBrewPrefix`/`testHomeDir` vars still exist for per-test setup (`testBrewPrefix` exists **twice** — once in `version`, once in `updater` — each private to its package, so an override must name the right one; `version`'s `TestMain` additionally pins its copy to an empty throwaway prefix package-wide, which is why a test there sees no brew layout unless it sets one up); the exported seams exist because the package that can *reach* a file is usually not the one that owns it — a `model` test that drives the tags/track/rename/untrack/status handlers lands in `loader.SaveMeta`, which rewrites `meta.yaml` **wholesale**, and one that drives the `[a]` overlay lands in `version.SetToken`. Leaving that to each test to remember (with its own temp `HOME`) is not a safeguard: an ad-hoc probe test that forgot it destroyed a real user's tracker, which is why the isolation moved to `TestMain`. Each of those packages carries a `TestConfigDirIsolated` test whose only job is to fail loudly if the isolation is ever removed. Never write a test that reaches a config path without one of these seams active. +Config files are not the only process-global state a test can leak. `version.rejectedToken` is the other: Go runs a package's tests in one process, sequentially, so a test that leaves a rejection standing strips `Authorization` from every later test in the same binary — deterministic contamination, not a flake. `resetTokenState` (`token_test.go`) is the seam, and it clears the field in **both** its setup and its `t.Cleanup`; every test that provokes a 401 must go through it. From outside the package the state is armed through **`version.SetTokenRejectedForTesting(bool) (restore func())`** — `rejectToken` is unexported and reached only by a real 401, so a `model` test that renders the degraded surfaces has no other way to produce it without the network. + `testAPIBase` is **not** exported, so a `model` test cannot point a fetch at an httptest server — executing any network command there would hit the live API. For `selfCheckCmd` the way around it is a **warm cache**: `seedSelfReleaseCache` (selfupdate_test.go) adds a fresh `ReleaseCheckedAt` entry for `version.SelfRepo`, so `version.SelfLatest` answers from `cache.json` without a request. It **snapshots the whole cache and restores it in `t.Cleanup`** rather than writing a one-entry `version.Cache` over it: `cache.json` is shared package-wide by `TestMain`'s `version.SetConfigDirForTesting`, so a clobbering seed would leak into every later test and a case needing both a README and a release seed would silently lose one. Hermeticity is asserted, not assumed — `TestSelfCheckCmdServesCache` checks `ReleaseCheckedAt` is *unchanged* afterwards, and any real request would have re-stamped it. `Init`'s batch test executes only element 1 (the self-check, right after the rate seed) against that seed; the rest of the batch is judged by **length** (the `TestInitHelpProbeFollowsHelpMode` pattern), and the README seed must stay the batch's last element. ### Session error log (`internal/logx`) @@ -162,13 +164,25 @@ Unauthenticated the REST API allows **60 requests/hour** per IP; with a token it **`doGH(req)`** is the single auth point: it **defaults** `Accept` to `application/vnd.github.v3+json` **only when the caller set none** (`req.Header.Get("Accept") == ""`) and sets `Authorization: Bearer ` (only when non-empty), runs the request with the 5s-timeout client, and calls `updateRateFromHeaders(resp.Header)`. Every fetcher builds a request then calls `doGH` — no duplicated header/client code or `os.Getenv` copies. The Accept rule is what lets `fetchReadme` pre-set `application/vnd.github.raw+json` (so `GET /repos/{owner}/{repo}/readme` answers with raw markdown instead of base64 JSON) without a second HTTP path; a fetcher that sets no Accept is unaffected. +**`doGH` answers a 401 by retrying the request once without the token.** An expired token is strictly worse than no token: the very same URLs answer 200 unauthenticated at 60 req/h, while authenticated with a dead credential *every* request is rejected — one observed session logged 33 × `/releases/latest http=401` and 32 × `/repos/* http=401`, every request it made, and none of it reached the screen. So on a 401 to a request that carried `Authorization`, `doGH` drains and closes the body, calls `rejectToken(tok)`, clones the request minus the header (all calls are bodiless GETs) and retries once; the retry's response goes through the same `updateRateFromHeaders`, so the gauge moves honestly from 5000 to 60. The retry lives **here and nowhere else** — this is the only place that decides to send a token at all, and a startup pre-check cannot help since `Init` fires the rate seed and every repo pass in one batch. Cost: with the passes running in parallel a few requests pay their own retry before the mark lands, bounded by the tool count, and those requests would have failed anyway. **`FetchRateWithToken` must keep bypassing `doGH`** (`ghClient.Do` directly): it is the one caller that must *observe* a 401 rather than survive it — riding the shared path would answer an anonymous 200 and persist a token already known to be dead. That separation is now a pinned invariant, not an incidental detail. + +**The rejection is stored as a value, not a bool** (`rejectedToken string`, under `tokenMu`), which is what makes it need no lifecycle: a newly entered token differs from it and resolves immediately, a cleared token is empty, and a bad `GITHUB_TOKEN` — which keepkit cannot unset — is suppressed by the same comparison. A bool would have to be cleared from `SetToken`, `ClearToken` and the env path, and one missed site is a session that keeps sending a token it knows is dead or stops sending a good one. `token.go` therefore splits into a raw core and one suppressing wrapper: **`effectiveToken()`** (env, else `tokenMem`) feeds `Token()` and `TokenRejected()`, while **`resolveToken()` — whose only caller is `doGH`** — returns `""` while the effective token equals the rejected one. The suppression must not reach the accessors the `[a]` overlay reads: the mask is how the user recognises *which* credential to replace, so blanking it in exactly the state the overlay exists to describe would leave "token config — rejected" naming no token at all. **The token file is never deleted** — a rejection means the credential was refused, not that keepkit may destroy the user's data. `rejectToken` logs on the **transition only**: a cold start rejects once per tracked tool in parallel, and a line each would bury the session log under identical entries. The `rejectedToken != ""` guard in `TokenRejected` is load-bearing — without it a user with **no** token compares equal to the empty rejected value and gets the whole degraded UI for a state that is not degraded at all. + **Hybrid rate read with precedence:** response headers (`X-RateLimit-Limit`/`-Remaining`/`-Reset`) observe the real counter for free on every request; `FetchRate()` hits `GET /rate_limit` (decodes `resources.core`) **without spending core quota** — used on overlay open, refresh, and startup seeding. But `/rate_limit` is **advisory only**: observed live, with a token it can report a pristine counter (`used=0`, sliding reset) while the per-request headers count real usage. Every write to the shared `rl` snapshot therefore goes through `mergeRateObservation` → `shouldReplaceRate`: an observation wins only if the current one is unknown, the `Limit` changed (token added/removed), it reports the same or more usage (lower/equal `Remaining`), or the current window has expired (a legitimate hourly reset). A same-window snapshot claiming fewer used requests is dropped — this is what keeps the status-bar gauge from zeroing out when the `[a]` overlay fires `fetchRateCmd`. `Rate()` returns the snapshot. Warm-cache starts make no request, so `Init()` fires one `fetchRateCmd` to seed the signal; on the model side snapshots with `Known==false` never overwrite a known `m.rate` (non-clobber merge). -**`ErrRateLimited`** (typed) is returned by `classifyStatus(resp)` for 403/429 **only when the response's own `X-RateLimit-Remaining == 0`** (read from the per-response header, never global `rl`, since concurrent goroutines race on `rl`); a 403 with remaining>0 is a generic `HTTP %d`. `fetchRemoteCmd` maps `errors.Is(err, ErrRateLimited)` + no cached card to a `"rate-limited"` `repoStatus` so the card renders "rate limited — press [a]" instead of a bare error; known tags/cards survive a total failure. +**The failure taxonomy is closed and every class carries a name.** `classifyStatus(resp)` returns **`ErrRateLimited`** for 403/429 **only when the response's own `X-RateLimit-Remaining == 0`** (read from the per-response header, never global `rl`, since concurrent goroutines race on `rl`); **`ErrTokenInvalid`** for a 401; a generic `HTTP %d` for everything else, and a 404 alone goes unlogged (it is conclusive and would re-create a session log every launch). "Transient" is deliberately the *absence* of a name rather than a name: 5xx, timeout, DNS and a broken transport all get the same treatment and the UI has nothing to tell them apart with. `ErrTokenInvalid` is a **classification, not a hot path** — once `doGH` retries a rejected token anonymously, a 401 never reaches a caller as an error, and GitHub does not 401 a credential-less request; what is left is the log line plus the defensive case of a proxy or enterprise host. `fetchRemoteCmd` maps `errors.Is(err, ErrRateLimited)` + no cached card to a `"rate-limited"` `repoStatus` so the card renders "rate limited — press [a]" instead of a bare error; known tags/cards survive a total failure. + +**`RepoData.Err` carries the more actionable of the two core fetches' errors**, picked by **`pickFetchErr(relErr, infoErr)`**: `ErrRateLimited` outranks everything (it is the one class with an answer the user can act on), `errNoReleases` never participates (a repo without releases is a conclusive negative, not a failure), and among transient failures the choice is arbitrary because the UI treats them identically. It used to carry `ErrRateLimited` **or nil and nothing else**, which is how a dead token became invisible: a 401 to all 34 tools was dropped on the floor at this line, the model never learned anything had gone wrong, and a stale card rendered as perfectly healthy. Two consequences ride on the fix. The `remoteMsg` handler's data path is now gated on **`hasData || msg.err == nil`** — the predicate is about *data*, not about the error class, since gating the stale-data branch on `ErrRateLimited` alone meant a 401 or a 5xx blanked a card the pass had actually carried up from the cache. And `Conclusive` is still not derivable from `Err` and never will be: a repo with no releases settles the tool with a nil error, while a rate-limited pass that served a stale card carries one while settling nothing. **Token validation before persistence:** `FetchRateWithToken(token)` issues `/rate_limit` with an explicit `Authorization` header **without touching `tokenMem`/the file**; 401 → `ErrTokenInvalid`. `SetToken` runs only after a 200, so an invalid token is never written to disk. -**API-status overlay (`a`):** opens via `ui.PlaceOverlay`, shows an optional add-token nudge (only when `TokenSource()=="none"` and not entering one), token source (masked), a `Used: / ` line (used = `Limit-Remaining`, matching the status-bar gauge) with the shared `rateLowThreshold` icon (none / `⚠` / `✕`), and reset time; `[e]` enters a masked `textinput` to set a token (validated via `FetchRateWithToken`, then `SetToken` + `autoFetchCmdsForSelected()` backfill), `[d]` removes it (config source only), `[r]` refreshes, `[esc]`/`[q]` closes. `a` is guarded structurally — it only fires in `modeNormal`. +**API-status overlay (`a`):** opens via `ui.PlaceOverlay`, shows an optional nudge (replace-in-your-shell when `TokenRejected()` **and** `TokenSource()=="env"`, replace-token when `TokenRejected()` alone, add-token when `TokenSource()=="none"`, none while entering one — checked in that order), token source (masked, and marked `— rejected (HTTP 401)` in `Danger` with a `requests run unauthenticated` line under it when refused), a `Used: / ` line (used = `Limit-Remaining`, matching the status-bar gauge) with the shared `rateLowThreshold` icon (none / `⚠` / `✕`), and reset time; `[e]` enters a masked `textinput` to set a token, `[d]` removes it (config source only), `[r]` refreshes, `[esc]`/`[q]` closes. `a` is guarded structurally — it only fires in `modeNormal`. + +**The rejected state is never cached on the model.** `renderRateGauge`, `renderRateMarker` and `renderAPIStatus` call **`version.TokenRejected()` at paint time**, the same way the token line beside them already calls `version.TokenSource()` and `version.Token()` — the three facts on that line come from one place and cannot disagree. It rode in on `remoteMsg`/`rateMsg` for a while, snapshotted in the command goroutine like `rate`, and that had no upside and one sharp edge: a reply in flight across the keystroke that replaced the credential carried a value observed under the *old* token, so it re-armed the flag after the accepted `tokenValidatedMsg` had cleared it and the overlay redrew `rejected (HTTP 401)` beside the mask of a token that had just validated. A render should show what is true now, and the value is mutex-guarded and cheap. Arming it in a test — `internal/model` can neither reach the unexported `rejectToken` nor the network — is what **`version.SetTokenRejectedForTesting(bool) (restore func())`** exists for, the same shape as the config-dir seams; `TestRejectedTokenHasNoCachedCopy` is what stops the cache from coming back. + +**A validated token recovers the whole tracker, not just the selected tool.** The `tokenValidatedMsg` success path is the one proof a credential works — `FetchRateWithToken` bypasses `doGH` precisely so a 401 cannot be retried away there — so it drops the rate-limited README negatives, clears **`m.remoteAnswered` wholesale**, and fans out `fetchRemoteCmd` for **every tool with `t.GitHub != ""`** — `Init`'s predicate, deliberately **not** `needsRemote`, which returns false as soon as a card exists with a non-empty `Latest`: that is true for precisely the tools that rendered stale-but-present data through the degraded window, the ones this recovery exists for. This spends real quota — a three-request pass per tool, because the degraded window left every entry stale (an inconclusive pass never stamps `CheckedAt`), and that is exactly what makes the refetch worth doing; the new token is what pays for it. `autoFetchCmdsForSelected` dispatches the selected tool's repo pass too whenever `needsRemote` says so, which after the clear is true for a cold-cache tool, so the loop skips whatever it already queued: two passes for one repo spend twice the quota and race two `updateCacheEntry` writes for the same entry. + +**None of that runs when `GITHUB_TOKEN` shadows the save.** `SetToken` writes the config file, and `effectiveToken` reads it only when the env var is empty, so a candidate accepted under env precedence is saved and still is not what goes on the wire — `FetchRateWithToken`'s 200 says the *credential* works, never that the session will send it. So the handler returns early on `TokenSource() == "env"` with `m.tokenError = "saved — GITHUB_TOKEN still takes precedence"`, before the rate write, the README-negative drop, the `remoteAnswered` clear and the fan-out. Each of those is wrong there in its own way, but the fan-out is the expensive one: `resolveToken` still suppresses the rejected env credential, so the recovery would dispatch a three-request pass per tool **at the anonymous 60/h ceiling** — with a few dozen tools, the one gesture the overlay offers as the way out of a degraded session spends the rest of the hour and leaves it worse off than before. Failing to recover is survivable; making it worse is not. The predicate is deliberately `TokenSource() == "env"` and not `TokenRejected()`: it is also the honest answer when a *valid* env token shadows the save, where the fan-out would merely run on somebody else's credential while the saved one stayed unused. The `version` package caches responses in `cache.json`. URL→`owner/repo` normalization lives in `loader.NormalizeRepo`; `version.extractRepo` delegates to it (the `loader` package owns GitHub-ref parsing to avoid an import cycle). diff --git a/README.md b/README.md index a7942b5..1e2f580 100644 --- a/README.md +++ b/README.md @@ -324,6 +324,32 @@ with `0600` permissions; an environment token is never written to disk. When the quota is exhausted, already-loaded cards are not erased, and a card with no data shows the `rate limited — press a` hint. +**A token that expires does not black the app out.** GitHub answers `401` to every +request a dead credential carries, while the very same URLs work fine with no token at +all — so keepkit retries once without it and carries on unauthenticated at 60 +requests per hour. The token file is left alone; it is simply unused until you replace +it. The state is visible rather than silent: the gauge gains a `✕` (`api✕ ▮▮▮▮░░░░░░░░ +34/60`, or just `api✕` when the bar is narrow), and `a` explains it — +`token config (ghp_••••••••3f2a) — rejected (HTTP 401)`, with the mask kept so you can +tell which credential to swap. Enter a working one with `[e]` and every card refetches +at once, without a restart, at the restored 5000 per hour. + +One case `[e]` cannot fix: a token from `GITHUB_TOKEN`. The variable always wins over +the file, so a replacement entered in the TUI is saved and then shadowed by the dead +one — keepkit says `saved — GITHUB_TOKEN still takes precedence` and changes nothing +else, rather than refetching the whole list on a credential it will not be sending. +Replace or unset the variable in the shell instead; the overlay says so. + +The degraded session itself is where the ceiling bites: a large tool list costs more +than 60 requests, so on a cold cache expect a partial fill until the hour turns. Cards +that were already loaded keep showing what they had. + +`[r]` in the brief panel now answers every press. A refresh that failed says so — +`refresh failed: rate limited — press [a]` or `refresh failed: network error` — while +one that fetched something stays silent, because the repainted card is the answer. +Before, success and every kind of failure looked identical: the spinner turned and +nothing changed. + ## Data storage The tool list lives in `~/.config/keepkit/meta.yaml` — one entry per tool (`name`, diff --git a/docs/plans/completed/20260804-api-failure-degradation.md b/docs/plans/completed/20260804-api-failure-degradation.md new file mode 100644 index 0000000..fdf8086 --- /dev/null +++ b/docs/plans/completed/20260804-api-failure-degradation.md @@ -0,0 +1,733 @@ +# Make GitHub API failures visible and survivable + +## Overview + +A GitHub token that goes bad after it was stored turns keepkit into a silent +liar. The reported symptom: `claude` showed `installed v2.1.221` against +`latest v2.1.220` on the card, and a manual `[r]` refresh changed nothing. + +The cause is not release/tag resolution. `~/.config/keepkit/token` had expired, +so every request answered `401 Bad credentials` — the same URLs answer `200` +with no `Authorization` header at all. An expired token is therefore **strictly +worse than no token**: unauthenticated the app would have worked at 60 req/h. +One session log holds 33 × `/repos/*/releases/latest http=401` and 32 × +`/repos/* http=401` — every request of the session, all 34 tracked tools — and +none of it reached the screen. + +The failure is swallowed one layer at a time: + +1. `classifyStatus` (`internal/version/github.go:169`) — 401 is neither 403 nor + 429, so it is not `ErrRateLimited`; it becomes an anonymous + `fmt.Errorf("GitHub API: HTTP %d")`. +2. `getRepoData` — with both core fetches failing, the total-failure early + return serves the stale entry and leaves `CheckedAt` untouched. That is why + `cache.json` still held the previous day's timestamp: the forced refresh + physically wrote nothing. +3. **`d.Err = rlErr`** (`internal/version/github.go:547`) — the core defect. Only + rate-limiting is propagated; every other error is dropped on the floor, so + the model never learns a 401 happened. +4. `remoteCmd` (`internal/model/commands.go:195`) — with no error, the + `repoStatus = "rate-limited"` branch cannot fire and the status stays + whatever the cached entry said (`active`). +5. The `remoteMsg` handler (`internal/model/model.go:1018`) — `hasData` is true + (cached `latest`), so the stale card renders as perfectly healthy. + +Four consequences, in the order they hurt: + +1. **A dead token is indistinguishable from "no updates".** Nothing on screen + says the data is not being refreshed, and the only trace is a log file the + user has to know to open. +2. **`[r]` answers nothing, ever.** `refreshingFor` starts the card-title + spinner and `remoteMsg` clears it. Success, 401, 403, a timeout and a dropped + connection all look identical: the spinner turns, the card does not change. +3. **The one error class we do handle is nearly unreachable.** The + `rate limited — press [a]` hint is gated on + `errors.Is(d.Err, ErrRateLimited) && d.Latest == "" && d.About == ""`, so it + only ever reaches a tool with no cache at all — a freshly tracked one. For + every tool that has been fetched once, a rate-limited pass is as invisible as + a 401. +4. **Nothing ever re-checks the token.** It is validated exactly once, at entry + time, by `FetchRateWithToken`. That it later expired is unobservable. + +This plan makes every failure class carry a name, degrades a rejected token to +unauthenticated requests instead of a blackout, and gives each of the three +states a surface: a persistent indicator, an explanation, and an answer to +`[r]`. + +**What "degraded" honestly buys, and what it does not.** With a warm cache the +degraded session is a working session: the retried requests succeed, cards +update, and the only difference is the gauge. On a **cold** cache it is not — +34 tools × 3 requests is 102 against an anonymous 60/h, and `Init` fires them in +one batch. There the outcome is a partial fill plus a visible rate-limited +surface. That is still strictly better than today's blackout, but it is a +different claim and the manual-verification steps below are written for it. + +## Context (from discovery) + +- files/components involved: + - `internal/version/github.go` — `classifyStatus`, `doGH`, `getRepoData`, + `RepoData`, `FetchRate`, `FetchRateWithToken` + - `internal/version/token.go` — `resolveToken`, `Token`, `TokenSource`, + `SetToken`, `ClearToken` + - `internal/model/model.go` — `remoteMsg` / `rateMsg` structs, the `remoteMsg` + and `tokenValidatedMsg` handlers, `Init` + - `internal/model/commands.go` — `remoteCmd`, `fetchRateCmd`, `needsRemote` + - `internal/model/render.go` — `renderRateGauge`, `renderAPIStatus`, + `renderHintsBar` +- test files that actually hold the affected coverage (there is **no** + `internal/model/model_test.go` — the handler tests live elsewhere): + - `internal/model/render_test.go` — `remoteMsg`/`installedMsg` handler tests + (`TestUpdateInstalledAndRemoteMsgPopulateCaches`, :2460), the rate/gauge + tests, the `[a]` overlay tests + - `internal/model/status_test.go` — `assertOnlyExpiryTick` (:27) and the + `statusMsgTTL` helpers + - `internal/model/commands_test.go` — fetch predicates and command batches + - `internal/version/github_test.go`, `internal/version/token_test.go` +- related patterns found: + - `doGH` is already documented as the single auth + rate-accounting point, so + a retry there needs no second HTTP path + - `RepoData.Conclusive` already exists and is already honest: it means "this + pass settled the tool", which is exactly the predicate `[r]` needs + - `remoteMsg` already carries a `version.RateLimit` snapshot taken inside the + command goroutine — the precedent for carrying `tokenRejected` the same way + - `shouldReplaceRate` (`github.go:96-110`) already admits an observation whose + `Limit` changed, so the 5000 → 60 transition needs no new support +- dependencies identified: none new; no new package, no new import + +## Development Approach + +- **testing approach**: Regular (code first, then tests within the same task) +- complete each task fully before moving to the next +- make small, focused changes +- **CRITICAL: every task MUST include new/updated tests** for code changes in + that task + - write unit tests for new functions/methods + - write unit tests for modified functions/methods + - add new test cases for new code paths + - **update existing test cases whose stated premise the change falsifies** — + tasks 2 and 3 each carry one, named explicitly in their checklists + - tests cover both success and error scenarios +- **CRITICAL: all tests must pass before starting next task** — no exceptions +- **CRITICAL: update this plan file when scope changes during implementation** +- run `go test -race ./...` after each change +- maintain backward compatibility + +## Testing Strategy + +- **unit tests**: required for every task (see Development Approach above) +- **e2e tests**: none — this project has no UI-based e2e suite. The TUI is + covered by rendering tests over constructed models and messages +- `internal/version` has `testAPIBase`, so its tests drive a real `httptest` + server and can assert on the headers a request carried +- `internal/model` cannot reach the network (`testAPIBase` is unexported there), + so every model test constructs messages by hand and feeds them to `Update` +- **process-global state needs a reset seam.** Go runs a package's tests in one + process, sequentially, so a test that leaves `rejectedToken` set would strip + `Authorization` from every later test in the same binary — deterministic + contamination, not a flake. `resetTokenState` + (`internal/version/token_test.go:12-27`) is the existing seam and must learn + the new field, in both its setup and its `t.Cleanup` +- test isolation is per test binary via the `TestMain` seams + (`logx.SetDirForTesting`, `loader.SetConfigDirForTesting`, + `version.SetConfigDirForTesting`) — already in place, nothing new needed. **No + test may reach a real config path.** + +## Progress Tracking + +- mark completed items with `[x]` immediately when done +- add newly discovered tasks with ➕ prefix +- document issues/blockers with ⚠️ prefix +- update plan if implementation deviates from original scope +- keep plan in sync with actual work done + +## Solution Overview + +Three layers, in dependency order. + +**Taxonomy.** A closed set of failure reasons: `ErrRateLimited` (403/429 with +`X-RateLimit-Remaining: 0`), `errNoReleases` (a conclusive negative that is never +surfaced as an error), `ErrTokenInvalid` (401), and everything else = transient. +"Transient" is deliberately the *absence* of a name rather than a name: 5xx, +timeout, DNS and a broken transport all get the same treatment (retry later) and +the UI has nothing to tell them apart with. No new sentinels are introduced — +`ErrTokenInvalid` already exists and is currently produced only by +`FetchRateWithToken`. + +> **Why the priority pick is two-tier, not three.** Once the retry lands (below), +> a 401 on a request that carried a token never reaches the caller as an error: +> `doGH` consumes it and returns the anonymous retry's response. GitHub answers +> 401 only for bad credentials, so an anonymous request cannot produce one +> either. `ErrTokenInvalid` therefore survives as a **classification**, not as a +> hot path — it names the code in a log line and in the defensive case of a +> proxy or enterprise host that 401s an anonymous request. The `getRepoData` +> pick is `ErrRateLimited > transient`, and `[r]` has no token wording. Wiring a +> tier that nothing can reach would read as live logic to the next person. + +**Degradation.** On a 401 to a request that carried `Authorization`, `doGH` +marks the token rejected and retries the same request once without the header. +A blackout becomes a working 60 req/h session. The rejection is stored as a +*value* (`rejectedToken string`), not a bool, so re-entering a token, clearing +it, or an env token that cannot be unset are all handled by one comparison +rather than by a lifecycle that has to be maintained in four places. + +**Visibility.** Three surfaces, each for a different lifetime of fact: + +| fact | lifetime | surface | +|---|---|---| +| the token was rejected, we are anonymous | the session | the status-bar gauge: `api✕` | +| why, and how to fix it | on demand | the `[a]` overlay | +| this refresh settled nothing | one press | a `statusMsg` from `[r]` | + +The gauge is the *primary* place the state is announced, not a guaranteed one: +it renders nothing while `!gaugeVisible(m.rate)` and it is droppable under width +pressure. The `[a]` overlay is what always has the answer, and the `[?]` overlay +already documents the `a` key. + +Key design decisions and rationale: + +- **The retry lives in `doGH` and nowhere else.** It is already the single auth + point, so there is exactly one place that can produce a 401 for a token we + chose to send. A startup pre-check was rejected: `Init` fires the rate seed and + 34 repo passes in one batch, so a check cannot land before the passes it would + warn. +- **`FetchRateWithToken` must keep bypassing `doGH`** (`ghClient.Do` directly, + `github.go:262`). If it rode the shared path, the retry would answer an + anonymous `200` and we would persist a known-dead token. Today that separation + reads as an incidental detail; this plan turns it into a pinned invariant. +- **Suppression is a wrapper, not a rewrite of the token accessors.** Only the + request path may see a rejected token as absent. The overlay still has to + print its source and its mask, which is exactly what the rejected state is + about — so a raw core stays underneath (see Technical Details). +- **The token file is never deleted.** A 401 means the credential was rejected, + not that we may destroy the user's data. It stays on disk and is merely unused + for the session. +- **The rejected flag rides on messages, not read from `View`.** A package + global cannot be set in a model test without the network. `remoteMsg`/`rateMsg` + carry the snapshot taken inside the command goroutine — the same shape `rate` + already uses — and the model keeps `m.tokenRejected`. +- **`✕` on the gauge, not a relabel to `anon`.** A user with no token is also + anonymous, and that is not degradation. The suffix costs one column, sheds + with the gauge, and does not collide with the `⚠`/`✕` usage-threshold icons, + which live only in the `[a]` overlay. + +Explicitly **out of scope** (taken to personal notes as a separate option): +a full `apiHealth` state machine, data age on the card, and an "installed newer +than latest" anomaly marker. + +## Technical Details + +**`classifyStatus`** gains one branch before the generic return: +`resp.StatusCode == http.StatusUnauthorized` → `ErrTokenInvalid`. Logging stays +as it is (401 is not a 404 and is worth a line). + +**`getRepoData` error pick.** `var rlErr error` and the terminal +`d.Err = rlErr` (`github.go:547`, plus the early-return site at `:480`) are +replaced by a helper that picks the more actionable of `relErr` / `infoErr`: +`ErrRateLimited` (fixed by waiting) over transient (fixed by nothing). +`errNoReleases` never participates: it is a conclusive negative, not a failure. +`RepoData.Conclusive` is untouched. + +**Token accessors** (`token.go`, all under the existing `tokenMu`). The +suppression must not reach the accessors the overlay reads, or the mask would +vanish in exactly the state the overlay exists to describe (`Token()` is a +one-line delegate today, `token.go:102-104`, and `render.go:577` renders its +mask). So the file splits into a raw core and one suppressing wrapper: + +| function | reads | used by | +|---|---|---| +| `effectiveToken()` | env, else `tokenMem` | the three accessors below | +| `Token()` | `effectiveToken()` | the overlay's masked preview | +| `TokenSource()` | env / `tokenMem` (unchanged) | the overlay | +| `TokenRejected()` | `rejectedToken != "" && effectiveToken() == rejectedToken` | the commands | +| `resolveToken()` | `effectiveToken()`, `""` when rejected | **`doGH` only** | + +`rejectToken(tok string)` records the refused value and emits one `logx` line on +the transition only — 34 goroutines must not write 34 lines. No clearing code is +needed anywhere: a newly entered token differs from the rejected value and works +immediately; a cleared token is empty anyway; a bad `GITHUB_TOKEN` cannot be +unset from the environment but is suppressed by the same comparison. The +`rejectedToken != ""` guard in `TokenRejected` is load-bearing: without it a user +with **no** token compares equal to the empty rejected value and gets the whole +degraded UI for a state that is not degraded at all. + +**`doGH` retry.** After `ghClient.Do`, when the request carried `Authorization` +and the response is 401: drain and close the body, `rejectToken(tok)`, +`req.Clone(req.Context())` minus the `Authorization` header, one retry. All +requests are bodiless GETs, so cloning is safe. The retry's response goes +through the same `updateRateFromHeaders`, so the gauge moves honestly from 5000 +to 60. Cost: with 34 parallel goroutines a few pay their own retry before the +mark lands — bounded by the tool count, and those requests would have failed +anyway. Every `doGH` consumer inherits this: `fetchRelease` (`:825`, shared by +`getChangelog` and `SelfLatest`), `fetchRepoInfo` (`:715`), `fetchLanguages` +(`:747`), `fetchLatestTag` (`:781`), `fetchReadme` (`:676`) and `FetchRate` +(`:233`). + +**Model state and messages.** `remoteMsg` and `rateMsg` gain +`tokenRejected bool`, snapshotted in the command goroutine after the fetch. The +model keeps `m.tokenRejected`; `renderRateGauge` and `renderAPIStatus` read the +field. The `tokenValidatedMsg` success path clears it explicitly: that message +is the one proof a new token works, it returns the user straight to +`modeAPIStatus` (`model.go:1097-1099`), and without the clear the overlay would +keep reading `rejected (HTTP 401)` against the token that just validated. + +**Recovery.** `tokenValidatedMsg` currently backfills only the selected tool via +`autoFetchCmdsForSelected()`. On an accepted token it will additionally clear +`m.remoteAnswered` and fan out `fetchRemoteCmd` for **every tool with +`t.GitHub != ""`** — `Init`'s predicate (`model.go:882-887`), deliberately *not* +`needsRemote`. `needsRemote` (`commands.go:342-354`) returns false as soon as a +card exists with a non-empty `Latest`, which is true for precisely the tools that +rendered stale-but-present data during the degraded window — the ones this +recovery exists for. The cost is the documented one: a goroutine plus a +`cache.json` read per tool, not API quota. + +**Processing flow after the change**, with a dead token and a warm cache: + +``` +doGH → 401 → rejectToken → retry anonymous → 200 + → data fetched, cache written, card updated + → remoteMsg{tokenRejected: true, conclusive: true} + → gauge renders api✕, [a] explains, [r] stays silent (it worked) +``` + +and with an exhausted anonymous quota (the cold-cache case): + +``` +doGH → 403 remaining=0 → ErrRateLimited → getRepoData total-failure return + → remoteMsg{err: ErrRateLimited, conclusive: false, hasData: true} + → stale card still renders, [r] answers "refresh failed: rate limited — press [a]" +``` + +## What Goes Where + +- **Implementation Steps** (`[ ]` checkboxes): code changes, tests and + documentation updates inside this repository +- **Post-Completion** (no checkboxes): manual verification against the live + GitHub API, which no automated test can perform + +## Implementation Steps + +### Task 1: Classify 401 as ErrTokenInvalid + +**Files:** +- Modify: `internal/version/github.go` +- Modify: `internal/version/github_test.go` + +- [x] add a `http.StatusUnauthorized` branch to `classifyStatus` returning + `ErrTokenInvalid`, placed before the generic `HTTP %d` return +- [x] extend the doc comment on `ErrTokenInvalid`: it is no longer produced only + by `FetchRateWithToken`, and after task 5 it is a defensive classification + rather than a hot path +- [x] write a test: a 401 response classifies as `ErrTokenInvalid` + (`TestClassifyStatusTaxonomy`, `TestClassifyStatusUnauthorizedLogs`) +- [x] write tests for the neighbours that must not change: 403 with + `remaining=0` → `ErrRateLimited`, 403 with `remaining>0` → generic, 404 → + generic and unlogged (`TestClassifyStatusTaxonomy`, + `TestClassifyStatusNotFoundStaysSilent`) +- [x] run `go test -race ./internal/version/` — must pass before task 2 + +### Task 2: Render cached data on any error, not only on a nil one + +**Files:** +- Modify: `internal/model/model.go` +- Modify: `internal/model/render_test.go` + +- [x] rewrite the first `remoteMsg` case as `case hasData || msg.err == nil:` +- [x] update the branch comment to say the predicate is about data, not about + the error class +- [x] **amend the existing assertion this falsifies**: the "remoteMsg with err + set must not touch the caches" block in + `TestUpdateInstalledAndRemoteMsgPopulateCaches` + (`internal/model/render_test.go:2503-2512`) feeds `latest: "2.0"`, which + makes `hasData` true. Split it: an error with **no** data still populates + nothing; an error **with** data now populates +- [x] write a test: `remoteMsg` carrying a non-rate-limit error plus a populated + card merges `versions`, `repoCards` and `repoStatus` + (`TestRemoteMsgNonRateLimitErrorKeepsData`) +- [x] write a test: `remoteMsg` with no data and `repoStatus == "rate-limited"` + still reaches the second case + (`TestRemoteMsgRateLimitedWithNoDataStillFlags`) +- [x] run `go test -race ./internal/model/` — must pass before task 3 + +### Task 3: Propagate the classified error out of getRepoData + +**Files:** +- Modify: `internal/version/github.go` +- Modify: `internal/version/github_test.go` + +- [x] add a helper that picks the more actionable of two errors — + `ErrRateLimited` over transient — ignoring `errNoReleases` (`pickFetchErr`) +- [x] replace `var rlErr error` and both `d.Err = rlErr` sites (`github.go:480` + and `:547`) with the helper +- [x] update the `RepoData.Err` doc comment: it no longer carries only + `ErrRateLimited` (and the `Conclusive` comment, whose rationale rested on + the same claim) +- [x] **amend the existing assertion this falsifies**: + `TestRepoDataConclusive/total failure is not conclusive` + (`internal/version/github_test.go:1281-1284`) fatals on a non-nil `Err` + with the premise *"a 5xx reaches the caller as a nil error"*. Invert it and + rewrite the function's rationale comment (`:1252-1257`), which states the + same now-false claim — the test's real subject (`Conclusive`, not `Err`, is + the retry marker) survives +- [x] write tests for the pick order, driving an `httptest` server that answers + 403+remaining=0 / 500 per endpoint (`TestRepoDataErrPickOrder`, 7 rows) +- [x] write a test: a repo with no releases still reports `Err == nil` and + `Conclusive == true` (`TestRepoDataConclusive/no releases is conclusive + with no error`) +- [x] run `go test -race ./...` — must pass before task 4 + +### Task 4: Split the token accessors and record a rejected value + +**Files:** +- Modify: `internal/version/token.go` +- Modify: `internal/version/token_test.go` + +- [x] extract `effectiveToken()` (env, else `tokenMem`) as the raw core and + point `Token()` and `TokenSource()` at it — they must keep answering for a + rejected token, or the overlay loses the mask it is meant to show + (`TokenSource` reads the raw state directly: it must tell env from config, + which the core's single return value cannot, so it is pointed at the same + state rather than at the function, with a comment saying why) +- [x] add `rejectedToken string` under `tokenMu` and `rejectToken(tok string)` + writing it, with exactly one `logx` line on the transition +- [x] make `resolveToken()` the suppressing wrapper — `""` while the effective + token equals `rejectedToken` — and document that `doGH` is its only caller +- [x] add `TokenRejected() bool` as + `rejectedToken != "" && effectiveToken() == rejectedToken` +- [x] extend `resetTokenState` (`token_test.go:12-27`) to clear `rejectedToken` + in **both** its setup and its `t.Cleanup` +- [x] write tests: rejecting suppresses `resolveToken` but not `Token`, + `TokenSource` or the mask; a different token via `SetToken` resolves again; + `ClearToken` leaves nothing resolvable + (`TestRejectTokenSuppressesOnlyTheRequestPath`, + `TestRejectTokenNewTokenResolvesAgain`, + `TestRejectTokenClearLeavesNothingResolvable`) +- [x] write a test: with **no** token configured, `TokenRejected()` is false — + the empty-equals-empty degenerate case + (`TestTokenRejectedWithNoTokenConfigured`) +- [x] write a test: `rejectToken` called twice for the same value logs once + (`TestRejectTokenLogsOnceForTheSameValue`, which also pins that the token + never reaches the log) +- ➕ `TestTokenRejectedEnvToken` — a bad `GITHUB_TOKEN` cannot be unset, and the + same comparison is what suppresses it; worth its own case +- [x] run `go test -race ./internal/version/` — must pass before task 5 + +### Task 5: Retry once without the token on a 401 in doGH + +**Files:** +- Modify: `internal/version/github.go` +- Modify: `internal/version/github_test.go` + +- [x] in `doGH`, remember whether an `Authorization` header was set; on a 401 + response drain and close the body, call `rejectToken`, clone the request + without the header and retry exactly once +- [x] account the retry's response through `updateRateFromHeaders` like any + other response +- [x] document on `FetchRateWithToken` that bypassing `doGH` is load-bearing, + not incidental — the retry would otherwise answer an anonymous 200 and a + dead token would be persisted +- [x] route every new 401 test through `resetTokenState` so the rejection cannot + leak into later tests in the same binary +- [x] write a test: server answers 401 with `Authorization` and 200 without → + two requests, the second header-less, the call succeeds, + `TokenRejected()` is true (`TestDoGHRetriesUnauthenticatedOn401`, which + also asserts the retry's 60-limit headers are what the gauge ends up with) +- [x] write a test: after the rejection, later requests go out header-less on + the first attempt with no second retry (`TestDoGHSkipsTokenAfterRejection`) +- [x] write a test: `FetchRateWithToken` against an always-401 server returns + `ErrTokenInvalid` and never an anonymous 200 + (`TestFetchRateWithTokenNeverSeesTheRetry`, which also pins that + validating a candidate does not arm the session-wide suppression) +- [x] write a test: a 401 for a request that carried no token is classified, + not retried (`TestDoGHDoesNotRetryATokenlessRequest`) +- [x] run `go test -race ./...` — must pass before task 6 + +### Task 6: Carry the rejected state to the model on messages + +**Files:** +- Modify: `internal/model/model.go` +- Modify: `internal/model/commands.go` +- Modify: `internal/model/render_test.go` + +- [x] add `tokenRejected bool` to `remoteMsg` and `rateMsg`, documenting that it + is snapshotted in the command goroutine like `rate` +- [x] set it in `remoteCmd` and `fetchRateCmd` from `version.TokenRejected()` + after the fetch returns +- [x] add `m.tokenRejected` to `Model` and write it from both handlers + (unconditionally and in both directions — the flag is the state of the + session's credential, not a property of one pass, and a `rateMsg` whose + numbers the non-clobber merge drops must still carry the reason) +- [x] clear `m.tokenRejected` in the `tokenValidatedMsg` success path, before it + returns to `modeAPIStatus` +- [x] write tests: a `remoteMsg` and a `rateMsg` with `tokenRejected: true` each + set the field (`TestTokenRejectedRidesOnMessages`, 4 rows including the + clear direction and a failed `rateMsg`) +- [x] write a test: an accepted `tokenValidatedMsg` clears it, and a failed one + leaves it alone (`TestTokenValidatedMsgClearsRejection`) +- [x] run `go test -race ./internal/model/` — must pass before task 7 + +### Task 7: Mark the rejected token on the status-bar gauge + +**Files:** +- Modify: `internal/model/render.go` +- Modify: `internal/model/render_test.go` + +- [x] render the gauge label as `api✕ ` with the `✕` in `Danger` when + `m.tokenRejected`, in both the full and the `compact` form +- [x] extend the `renderRateGauge` doc comment with the new state and with why + the suffix is not a relabel to `anon` +- [x] write a test: the gauge carries `✕` when rejected and does not when not + (`TestRenderRateGaugeMarksARejectedToken`) +- [x] write a test: `TestStatusBarNeverWraps` stays green at the 80×24 baseline + with the rejected state on (three rows added: plain, in `focusBrief`, and + under a self banner) +- [x] write a test: the compact form keeps the marker (it is the form a narrow + bar keeps) — note the shed order is *version cell first, gauge after* + (`render.go:284-303`), so no test may claim the opposite +- ⚠️ **the plan's "the suffix costs one column, sheds with the gauge" was one + step short.** The marker does not merely shed *with* the gauge — it makes + the gauge shed *earlier*. Measured: with the six-hint bar and a 60-limit + snapshot the gauge survived down to ≤78 columns before, and only to 81 + after, so **at the 80×24 baseline arming the degraded state removed the + announcement entirely** — the session that most needs announcing was the + one showing nothing. Fixed inside this task with a third, narrowest form: + **`renderRateMarker()`** — `api✕` with no numbers, tried last in + `renderHintsBar`'s widest-form-first loop, `""` when there is no rejection + (so a healthy bar gains no form it did not have). Which half survives + follows the bar's own shed rule: the numbers are a measurement `[a]` + repeats, the `✕` is the only sign anywhere that requests stopped carrying + the token. Pinned by `TestRenderRateMarkerIsTheLastFormStanding` +- ➕ `TestRenderRateGaugeMarksARejectedToken/the mark costs exactly one cell` — + the gauge's width is what `renderHintsBar` reserves at the right edge, so a + two-cell glyph would push the bar past the terminal and scroll the top + border off the alt screen. U+2715 measures 1 under both runewidth + conditions; U+00D7 would have measured 2 under `RUNEWIDTH_EASTASIAN=1` +- [x] mutation-checked (per the `tui-render` skill): removing the label marker, + dropping the marker-only form from the fit loop, and restyling the mark + `Dim` each turn the new assertions red +- [x] run `go test -race ./internal/model/` — must pass before task 8 + +### Task 8: Explain the rejection in the [a] overlay + +**Files:** +- Modify: `internal/model/render.go` +- Modify: `internal/model/render_test.go` + +- [x] render the token line as `token () — rejected (HTTP + 401)` in `Danger` when `m.tokenRejected`, with + `requests run unauthenticated` under it +- [x] widen the `add a github token…` nudge condition from + `source == "none"` to also cover the rejected case, with wording about + replacing it (the two are a `switch`, not one widened condition: "add a + github token" reads as advice to someone who already has one, so the + rejected case gets `replace the token to restore the 5000/h limit`) +- [x] write a test: the rejected token line renders **with its mask intact** + (the task 4 split is what makes this possible) plus the unauthenticated + note (`TestRenderAPIStatusExplainsARejectedToken`). Note the mask + invariant is pinned **one layer down**, in + `TestRejectTokenSuppressesOnlyTheRequestPath`: `internal/model` cannot + reach the unexported `rejectToken`, so a model test can only prove the + overlay prints what `version.Token()` returns, not that `Token()` still + returns it under a rejection. Mutation-verified there — reverting `Token` + to delegate to `resolveToken` turns that test red +- [x] write a test: the nudge appears for a rejected token and stays hidden + while `modeTokenInput` is open (and the *line* stays under the input — it + is the state, not a prompt) +- [x] write a test: a healthy token renders neither +- [x] write a size-budget test for the overlay at 80×24 — this task adds its + longest line so far and nothing pins its width today + (`TestRenderAPIStatusSizeBudget`, both modes × both states, with a reset + time so the height is measured full; the widest framed form is 56 cells + against the 76 budget) +- [x] mutation-checked: removing the rejected line and removing the replace + nudge each turn the new assertions red +- [x] run `go test -race ./internal/model/` — must pass before task 9 + +### Task 9: Make [r] answer every press + +**Files:** +- Modify: `internal/model/model.go` +- Modify: `internal/model/status_test.go` + +- [x] in the `remoteMsg` handler, where `refreshingFor` is cleared, return + `setStatus` with a reason when `!msg.conclusive` (inside the + `msg.toolName == m.refreshingFor` branch, so the background passes `Init` + fires — inconclusive all the time on an offline start — never put a + "refresh failed" on the bar for a gesture nobody made) +- [x] map the reason from `msg.err`: `ErrRateLimited` → + `refresh failed: rate limited — press [a]`, anything else → + `refresh failed: network error` (no token wording — see the two-tier note + in Solution Overview), in the single `refreshFailedStatus(err)` helper +- [x] keep success silent — the card repaint is the answer +- [x] write a table test over the reasons asserting the status text, shrinking + `statusMsgTTL` first as the sibling helpers require + (`status_test.go:44-45`) so the tick does not add a real second per case + (`TestRefreshAnswersEveryPress`, 5 rows) +- [x] write a test using `assertOnlyExpiryTick` (`status_test.go:27`) that no + fetch command rode along (asserted on every failing row of the table) +- [x] write a test: a conclusive pass sets no status +- ➕ `TestRefreshStatusOnlyForTheRefreshedTool` — the ownership half: a failing + pass for a *different* tool leaves both the status and `refreshingFor` + alone +- [x] mutation-checked: removing the answer, and collapsing the rate-limit tier + into the generic one, each turn the table red +- [x] run `go test -race ./internal/model/` — must pass before task 10 + +### Task 10: Refetch everything when a good token is accepted + +**Files:** +- Modify: `internal/model/model.go` +- Modify: `internal/model/commands_test.go` + +- [x] in the `tokenValidatedMsg` success path, clear `m.remoteAnswered` before + building commands +- [x] fan out `fetchRemoteCmd` for every tool with `t.GitHub != ""` — `Init`'s + predicate, **not** `needsRemote`, which returns false for exactly the + tools that rendered stale-but-present data and therefore need it most +- [x] document the choice and its cost inline (a goroutine plus a `cache.json` + read per tool, not API quota) +- [x] write a test: an accepted token clears `remoteAnswered` and returns a + batch sized for every tool with a GitHub ref + (`TestTokenAcceptedRefetchesEveryRepo`; the fixture seeds the state a + degraded session ends in — every repo tool answered and carrying a card) +- [x] write a test: a tool whose card is already populated is still in the batch + (the regression `needsRemote` would cause) — mutation-verified: swapping + the predicate for `needsRemote` turns that subtest red +- [x] write a test: a rejected validation (`msg.err != nil`) changes neither +- [x] run `go test -race ./...` — must pass before task 11 + +### Task 11: Verify acceptance criteria + +- [x] verify all requirements from Overview are implemented: 401 named (task 1), + error propagated (task 3, plus task 2 so the propagation does not blank a + card), retry degrades to anonymous (task 5), three surfaces present + (tasks 7/8/9), `[r]` always answers (task 9) +- [x] verify edge cases: a 401 on a token-less request + (`TestDoGHDoesNotRetryATokenlessRequest`), a rejected env token + (`TestTokenRejectedEnvToken`), a re-entered identical bad token + (`TestRejectedTokenReenteredStaysSuppressed`), and **no** token at all + (`TestTokenRejectedWithNoTokenConfigured` — the `rejectedToken != ""` + guard, so an unauthenticated-by-choice session shows no `✕` and no + rejection text) +- [x] verify the two `doGH` consumers whose negatives outlive the degraded + window. **Both recorded as known and out of scope**, and neither is made + worse by this change: + - **README.** The `tokenValidatedMsg` cleanup drops only content-less + `ErrRateLimited` entries, so a README that failed with a *transient* error + stays a session-scoped dead end recoverable by `[r]` alone. That was already + true before this change and the retry makes the new class unreachable in + practice: a 401 is consumed by `doGH` and answered by the anonymous + response, and GitHub does not 401 a credential-less request — so no README + can be cached under `ErrTokenInvalid` outside a proxy/enterprise host that + 401s anonymously. Widening the cleanup to any content-less non-`ErrNoReadme` + entry is a behaviour change beyond this plan's scope. + - **`SelfLatest`.** Still no force variant, so a self-check that failed while + degraded has no banner until the next launch. It does **not** poison the + cache — a transient failure stamps no timestamp — so the next launch + retries, which is the documented design. Left as is. +- [x] run the full suite: `go test -race ./...` +- [x] run the `preflight` skill (build / vet / `go test -race` / golangci-lint — + all four green, lint reports 0 issues), plus CI's cross-compile step + (`GOOS=windows build`+`vet`, `GOOS=darwin build`) +- [x] verify no test reaches a real config path (the `TestConfigDirIsolated` + tests stay green in all four packages that carry one) + +### Task 12: [Final] Update documentation + +- [x] update the **GitHub API** section of `CLAUDE.md`: `doGH` now retries + without the token on a 401, and `RepoData.Err` no longer carries only + `ErrRateLimited` (plus the value-not-bool rejection state and the + `effectiveToken`/`resolveToken` split, and the `FetchRateWithToken` bypass + promoted from an incidental detail to a pinned invariant) +- [x] update the **Async fetch responsibility split** sentence in `CLAUDE.md` + that reads *"`Err` carries `ErrRateLimited` or nil, so an offline start and + a 5xx both arrive with a nil error"* — the second half stops being true +- [x] update the **status bar** and **API-status overlay** paragraphs of + `CLAUDE.md` with the `api✕` state and the rejected-token line (plus the + `renderRateMarker` third form, the replace-vs-add nudge, the recovery + fan-out, and `[r]`'s answer under **Refresh**) +- [x] update the two in-code comments that assert the same obsolete claim and + that `docs-sync` cannot see: the `remoteMsg.conclusive` field doc + (`internal/model/model.go:67-71`) and the `remoteAnswered` field doc + (`:399-408`) +- [x] run the `docs-sync` skill to catch anything else that drifted. Found and + fixed three more in **`ARCHITECTURE.md`** (the same now-false `Err` claim + in the async-fetch section, the `ErrRateLimited`/`doGH` bullets in the + GitHub API section, and the status-bar shed order), plus a new note in both + `CLAUDE.md` and `ARCHITECTURE.md` that **`version.rejectedToken` is + process-global state a test can leak** — `resetTokenState` is its seam. + Verified unchanged: the mermaid import graph (21 edges, no new package + edge — `version` already imported `logx`), the 12-value `inputMode` enum, + the README key tables, `go.mod` vs README **Stack**, the storage-path + table, every timeout/limit, and all three `docs/design/` files (none makes + a claim this change falsifies; `self-update.md`'s "no release published + arrives as a nil error" is still exactly true) +- [x] update `README.md` only if it documents the token flow — it does, so the + **GitHub API and token** section gained the expired-token degradation, the + `api✕`/overlay surfaces, the one-press recovery, the honest cold-cache + caveat, and `[r]`'s new answer +- [x] move this plan to `docs/plans/completed/` + +## Post-review corrections + +A full `/review:pr` pass on #60 (independent subagent + mutation checks) found five +defects in the delivered work. All fixed on the branch before merge: + +1. **The task-10 fan-out dispatched the selected tool twice.** The rationale for + preferring `Init`'s predicate over `needsRemote` covered only the + stale-but-present shape; on a **cold** cache `needsRemote` is *true*, so + `autoFetchCmdsForSelected` queued the selected tool's repo pass and the loop + queued it again — six requests for one repo and two racing `updateCacheEntry` + writes. The loop now skips whatever the backfill already queued, decided + against the same (cleared) marker state it reads. +2. **The inline cost comment and `CLAUDE.md` were wrong**, and `README.md` said the + opposite in the same change: the fan-out *does* spend quota, because the + degraded window left every entry stale. That is the point of it. +3. **`rejectToken` fired before the retry's verdict was known**, so a host that + 401s an anonymous request too — the proxy/enterprise case `ErrTokenInvalid`'s + own doc names — permanently disarmed a perfectly good token. It now runs after + the retry, and only when dropping the header changed the answer. +4. **Task 9's `!msg.conclusive` predicate was too broad.** It reported + `refresh failed: network error` for a ref the version layer refused outright + (a bare `RepoData`, nil `Err`) and contradicted a card the user had just + watched update on a partial pass. `msg.err != nil` is the right predicate now + that every real failure carries a name. The plan's own test row had enshrined + the wrong message as correct. +5. **Task 6's whole design was wrong.** Mirroring the rejection into a `Model` + field from a goroutine snapshot bought nothing and opened a window: a reply in + flight across the keystroke that replaced the credential re-armed the flag + after `tokenValidatedMsg` cleared it, and the overlay named the *new* token as + rejected. The field, the two message fields and the two handler writes are + gone; the renderers call `version.TokenRejected()` at paint time, exactly as + the token line beside them already calls `TokenSource()`/`Token()`. Arming it + in a test is `version.SetTokenRejectedForTesting`. + +Two test-quality findings from the same pass: the glyph-width assertion measured +through `lipgloss.Width` (ambient `RUNEWIDTH_EASTASIAN`), so substituting the +two-cell U+00D7 the comment itself names stayed green under CI's plain +`go test` — it now checks both runewidth conditions like its two siblings; and a +fixture token tripped GitGuardian, renamed to an obviously synthetic value. + +## Post-Completion + +*Items requiring manual intervention or external systems — no checkboxes, +informational only* + +**Manual verification** (cannot be covered by tests — no automated test may hold +a real credential): + +- with a **warm** cache, put a deliberately invalid token in + `~/.config/keepkit/token`, start keepkit, and confirm: cards still render, the + gauge reads `api✕` against a 60-limit, `[a]` explains the rejection with the + mask still visible, and `[r]` succeeds silently +- with a **cold** cache (`rm ~/.config/keepkit/cache.json`) and the same bad + token, confirm the honest degraded outcome: a partial fill, then + `refresh failed: rate limited — press [a]` — 34 tools × 3 requests exceeds the + anonymous 60/h, and this is the case the plan does *not* claim to make whole +- replace the token with a valid one via `[a] → [e]` and confirm every card + refetches without a restart and the `✕` clears immediately +- confirm `GITHUB_TOKEN= keepkit` degrades the same way and that the + overlay names `env` as the source +- confirm a run with **no** token configured shows no `✕` and no rejection text + +**External system updates**: none. No consuming projects, no deployment config, +no third-party integration is affected. diff --git a/internal/model/commands_test.go b/internal/model/commands_test.go index 6468100..ff7bc73 100644 --- a/internal/model/commands_test.go +++ b/internal/model/commands_test.go @@ -564,3 +564,141 @@ func TestInitHelpProbeFollowsHelpMode(t *testing.T) { t.Errorf("Init queued %d cmds in help mode and %d in readme mode, want exactly one more (the --help probe)", help, readme) } } + +// TestTokenAcceptedRefetchesEveryRepo pins the recovery a new token buys. The +// degraded window leaves every tool holding stale-but-present data, and +// needsRemote answers false for exactly that shape — a card exists, Latest is +// non-empty — so the predicate here is Init's (`t.GitHub != ""`), not +// needsRemote's. Backfilling only the selected tool left the other 33 frozen +// until a restart. +func TestTokenAcceptedRefetchesEveryRepo(t *testing.T) { + t.Setenv("GITHUB_TOKEN", "") + restore := version.SetConfigDirForTesting(t.TempDir()) + t.Cleanup(restore) + t.Cleanup(func() { _ = version.ClearToken() }) + + newModel := func() Model { + m := New([]loader.ToolMeta{ + {Name: "gh", GitHub: "cli/cli"}, + {Name: "rg", GitHub: "BurntSushi/ripgrep"}, + {Name: "fd", GitHub: "sharkdp/fd"}, + {Name: "localtool"}, // no repo: nothing to refetch + }) + m.width, m.height = 80, 24 + // The state a degraded session ends in: every repo tool answered and + // carrying a card, which is what makes needsRemote the wrong predicate. + for _, name := range []string{"gh", "rg", "fd"} { + m.remoteAnswered[name] = true + m.repoCards[name] = version.RepoCard{About: "stale", Latest: "v1.0.0"} + m.versions[name] = VersionInfo{Latest: "v1.0.0"} + } + return m + } + + t.Run("an accepted token clears remoteAnswered", func(t *testing.T) { + nm := mustModel(newModel().Update(tokenValidatedMsg{token: "ghp_goodtoken1234"})) + if len(nm.remoteAnswered) != 0 { + t.Errorf("remoteAnswered = %v, want cleared: nothing settled under the old credential is settled under the new one", nm.remoteAnswered) + } + }) + + t.Run("every repo tool is in the batch, populated card or not", func(t *testing.T) { + m := newModel() + _, cmd := m.Update(tokenValidatedMsg{token: "ghp_goodtoken1234"}) + if cmd == nil { + t.Fatal("an accepted token returned no commands") + } + batch, ok := cmd().(tea.BatchMsg) + if !ok { + t.Fatalf("cmd produced %T, want a tea.BatchMsg", cmd()) + } + // autoFetchCmdsForSelected's own batch plus one fetchRemoteCmd per repo + // tool. Asserting the count rather than executing the leaves keeps the + // test off the network: internal/model cannot redirect testAPIBase. + if len(batch) != 1+3 { + t.Errorf("batch has %d commands, want 1 backfill + 3 repo refetches", len(batch)) + } + }) + + t.Run("the selected tool is fetched once, not twice", func(t *testing.T) { + // The cold-cache shape, which is the harsher of the two degraded states + // and the one the fixture above deliberately does not cover: with no + // card at all, needsRemote answers TRUE once remoteAnswered is cleared, + // so autoFetchCmdsForSelected dispatches the selected tool's repo pass — + // and a loop that also dispatched it would spend six requests on one repo + // and race two updateCacheEntry writes for the same entry. + m := New([]loader.ToolMeta{ + {Name: "gh", GitHub: "cli/cli"}, + {Name: "rg", GitHub: "BurntSushi/ripgrep"}, + {Name: "fd", GitHub: "sharkdp/fd"}, + }) + m.width, m.height = 80, 24 + sel, ok := m.selectedTool() + if !ok || !m.needsRemote(sel) { + t.Fatal("fixture: the selected tool must need a remote pass, else this asserts nothing") + } + + _, cmd := m.Update(tokenValidatedMsg{token: "ghp_goodtoken1234"}) + if cmd == nil { + t.Fatal("an accepted token returned no commands") + } + batch, ok := cmd().(tea.BatchMsg) + if !ok { + t.Fatalf("cmd produced %T, want a tea.BatchMsg", cmd()) + } + // The backfill covers the selected tool, so the loop contributes the + // other two only. + if len(batch) != 1+2 { + t.Errorf("batch has %d commands, want 1 backfill + 2 refetches — "+ + "the selected tool must not be dispatched by both", len(batch)) + } + }) + + t.Run("a refused token changes neither", func(t *testing.T) { + m := newModel() + updated, cmd := m.Update(tokenValidatedMsg{token: "ghp_bad", err: version.ErrTokenInvalid}) + nm := updated.(Model) + if len(nm.remoteAnswered) != 3 { + t.Errorf("remoteAnswered = %v, want untouched by a refused token", nm.remoteAnswered) + } + if cmd != nil { + t.Errorf("a refused token returned a cmd (%T), want none", cmd()) + } + }) + + // A token GITHUB_TOKEN shadows validates and saves like any other — the + // candidate goes to /rate_limit under an explicit header, so the 200 says + // nothing about whether the session will use it. It will not: SetToken writes + // the config file and effectiveToken reads that only when the env var is + // empty. Recovering on that basis is the expensive half of the mistake — a + // three-request pass per tool, dispatched at the anonymous 60/h ceiling + // because resolveToken still suppresses the rejected env credential. With a + // few dozen tools the one gesture the overlay offers as the way out of a + // degraded session spends the rest of the hour and leaves it worse off. + t.Run("a token shadowed by GITHUB_TOKEN recovers nothing", func(t *testing.T) { + t.Setenv("GITHUB_TOKEN", "ghp_envtoken12345678") + m := newModel() + updated, cmd := m.Update(tokenValidatedMsg{ + token: "ghp_goodtoken1234", + rate: version.RateLimit{Known: true, Remaining: 5000, Limit: 5000}, + }) + nm := updated.(Model) + + if cmd != nil { + t.Errorf("a shadowed token dispatched %T, want no fan-out at the anonymous ceiling", cmd()) + } + if len(nm.remoteAnswered) != 3 { + t.Errorf("remoteAnswered = %v, want untouched: nothing was recovered", nm.remoteAnswered) + } + // The candidate's snapshot describes a limit the session does not have. + if nm.rate.Limit == 5000 { + t.Error("the gauge took the candidate's 5000 limit while requests keep running at 60") + } + if nm.tokenError == "" { + t.Error("the overlay reports nothing, so the save reads as having worked") + } + if !strings.Contains(nm.tokenError, "GITHUB_TOKEN") { + t.Errorf("tokenError = %q, want it to name what takes precedence", nm.tokenError) + } + }) +} diff --git a/internal/model/model.go b/internal/model/model.go index dab67f2..7d6c35a 100644 --- a/internal/model/model.go +++ b/internal/model/model.go @@ -66,8 +66,9 @@ type remoteMsg struct { err error // conclusive mirrors version.RepoData.Conclusive: the pass settled this tool // for the cache window. It is what remoteAnswered is written from, because - // err cannot answer that — it carries ErrRateLimited or nil, so an offline - // start and a 5xx both arrive here with a nil error. + // err cannot answer that even now that every failure class arrives named: + // a repo with no releases settles the tool with a nil err, and a pass that + // served a stale card under a rate limit settles nothing while carrying one. conclusive bool } @@ -136,6 +137,21 @@ const updateLogMaxLines = 500 // differently. const updateBusyStatus = "another update is running" +// refreshFailedStatus names why an [r] failed. The taxonomy is +// deliberately two-tier: a rate limit has an answer the user can act on — wait, +// or raise the ceiling from the [a] overlay — while a 401, a 5xx, a timeout and +// a dropped connection all mean "try later" and the UI has nothing to tell them +// apart with. In particular there is no token wording here: by the time a fetch +// fails, doGH has already retried a rejected token anonymously, so a refresh +// that failed did not fail *because of* the token — and the gauge and the [a] +// overlay are where that state is reported anyway. +func refreshFailedStatus(err error) string { + if errors.Is(err, version.ErrRateLimited) { + return "refresh failed: rate limited — press [a]" + } + return "refresh failed: network error" +} + // selfToolName is the name keepkit uses for itself inside the update pipeline: // the updater's detection target, the updatingFor/updateLogFor guard value and // the label the confirm bar and status messages show. A constant rather than a @@ -396,16 +412,21 @@ type Model struct { // two selection moves onto the same tool inside that window (j then k, a // click back) would each spend a GitHub request. Cleared by readmeMsg. readmeLoading map[string]bool - // remoteAnswered holds the tools whose network pass came back WITHOUT an - // error — a conclusive answer, even when it carried no Latest (a repo with - // neither releases nor tags). needsRemote's empty-Latest clause would - // otherwise re-dispatch the whole pass on every cursor visit. The honest - // cost of that is a goroutine and a cache.json read, not API quota: - // errNoReleases is already conclusive on the version side, so the repeat - // was served from cache. Session-scoped, and deliberately keyed off the - // error rather than off m.repoStatus — a rate-limited pass carrying a stale - // card writes "active"/"archived" there, so that marker would suppress a - // retry the predicate still wants. + // remoteAnswered holds the tools whose network pass the version layer called + // CONCLUSIVE — it settled the tool for the cache window, even when it + // carried no Latest (a repo with neither releases nor tags). needsRemote's + // empty-Latest clause would otherwise re-dispatch the whole pass on every + // cursor visit. The honest cost of that is a goroutine and a cache.json + // read, not API quota: errNoReleases is already conclusive on the version + // side, so the repeat was served from cache. + // + // Session-scoped, and written from msg.conclusive rather than from the error + // or from m.repoStatus. Neither of those can answer it: a repo with no + // releases settles the tool with a nil error, a rate-limited pass that + // served a stale card settles nothing while carrying one, and that same pass + // writes "active"/"archived" into repoStatus. Cleared wholesale when a new + // token validates — nothing settled under the old credential is settled + // under the new one. remoteAnswered map[string]bool // readmeRender memoizes the last glamour render (see readme.go). readmeRender readmeRenderCache @@ -997,12 +1018,10 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { if msg.rate.Known { m.rate = msg.rate } - // Data is displayable when the fetch succeeded, or when a rate-limit error - // still carried usable cache values: a fresh tag from a partial fetch, or - // the stale card kept on a total failure. In those cases render the data so - // known tags/cards survive the outage. Only a rate-limit failure with - // nothing to show falls back to the "rate limited — press a" hint. A - // generic error carries no data and must not touch the caches. + // Data is displayable when the fetch succeeded, or when it failed but still + // carried usable cache values: a fresh tag from a partial fetch, or the + // stale card kept on a total failure. In those cases render the data so + // known tags/cards survive the outage. hasData := msg.latest != "" || msg.card.About != "" // Only a pass the version layer calls conclusive is an answer: it stamped // (or found) a fresh cache entry. A rate-limited pass may still have @@ -1015,7 +1034,12 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.remoteAnswered[msg.toolName] = true } switch { - case msg.err == nil, errors.Is(msg.err, version.ErrRateLimited) && hasData: + // The predicate is about data, not about the error class: what decides + // whether the caches are written is whether the pass came back with + // something to write. Gating the stale-data path on ErrRateLimited alone + // meant a 401 or a 5xx dropped a card the pass had actually carried up + // from the cache — the tool went blank for a failure it survived. + case hasData || msg.err == nil: info := m.versions[msg.toolName] info.Latest = msg.latest m.versions[msg.toolName] = info @@ -1040,6 +1064,23 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { if msg.toolName == m.refreshingFor { m.refreshingFor = "" m.briefViewport.SetContent(m.renderCard()) + // [r] used to answer nothing at all: success, a rate limit, a 401, a + // timeout and a dropped connection were one gesture — the spinner + // turns, the card does not change. A failed pass says why; a pass + // that fetched something stays silent, because the repainted card IS + // the answer. + // + // The predicate is the error and deliberately not !conclusive, which + // is a broader thing: it is also false when the version layer refused + // the ref outright (an unsupported or spoofed host — a bare RepoData + // with a nil Err), which would report a network failure that never + // happened, and on a partial pass that fetched a new tag and lost + // only the repo card — where the bar would contradict a card the user + // just watched update. Every path that actually failed to fetch now + // carries a named error (see version.pickFetchErr). + if msg.err != nil { + return m, m.setStatus(refreshFailedStatus(msg.err)) + } } return m, nil @@ -1099,6 +1140,21 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { } m.tokenInput.Blur() m.tokenInput.SetValue("") + // The token validated and is on disk, but SetToken writes the config file + // and effectiveToken reads that only when GITHUB_TOKEN is empty — so under + // env precedence what was just accepted is not what goes on the wire. Say + // so, and stop here. + // + // Everything below assumes the opposite. msg.rate is the candidate's + // snapshot, so the gauge would claim a 5000 limit the session does not + // have; and the fan-out would spend a three-request pass per tool at the + // anonymous 60/h ceiling — with a few dozen tools that is the whole hour, + // burnt by the one gesture the overlay offers as the way out of a degraded + // session. Failing to recover is survivable; making it worse is not. + if version.TokenSource() == "env" { + m.tokenError = "saved — GITHUB_TOKEN still takes precedence" + return m, nil + } m.tokenError = "" if msg.rate.Known { m.rate = msg.rate @@ -1112,8 +1168,38 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { delete(m.readmeData, name) } } - // Backfill cards now that the higher limit is available. - return m, m.autoFetchCmdsForSelected() + // Refetch every tool with a repo, not just the selected one. The predicate + // is Init's — `t.GitHub != ""` — and deliberately NOT needsRemote, which + // returns false as soon as a card exists with a non-empty Latest: that is + // true for precisely the tools that rendered stale-but-present data + // through the degraded window, the ones this recovery exists for. Clearing + // remoteAnswered first is the other half; a tool the layer settled under + // the old credential is not settled under the new one. + // + // This does spend quota — a real three-request pass per tool, since the + // degraded window left every entry stale (an inconclusive pass never + // stamps CheckedAt) and that is exactly what makes the refetch worth + // doing. The new token is what pays for it: the limit is 5000/h from here + // on, not the 60 the session was running at. + clear(m.remoteAnswered) + // autoFetchCmdsForSelected refreshes the selected tool's other sources + // (changelog, installed, README) and dispatches its repo pass too, but + // only when needsRemote says so — which after the clear above is true for + // a cold-cache tool, the harsher of the two degraded shapes. So decide + // against the same marker state it will read, and let the loop skip + // whatever it already queued: two passes for one repo spend twice the + // quota on it and race two updateCacheEntry writes for the same entry. + queued := "" + if t, ok := m.selectedTool(); ok && m.needsRemote(t) { + queued = t.Name + } + cmds := []tea.Cmd{m.autoFetchCmdsForSelected()} + for _, t := range m.tools { + if t.GitHub != "" && t.Name != queued { + cmds = append(cmds, fetchRemoteCmd(t)) + } + } + return m, tea.Batch(cmds...) case openURLMsg: if msg.err != nil { diff --git a/internal/model/render.go b/internal/model/render.go index de2b5a5..820bd0b 100644 --- a/internal/model/render.go +++ b/internal/model/render.go @@ -293,9 +293,13 @@ func (m Model) renderHintsBar(style lipgloss.Style, cells []string) string { hints = truncateToWidth(stripANSI(hints), inner) } - // The gauge takes whatever is left over, widest form first. + // The gauge takes whatever is left over, widest form first. The marker-only + // form is last because it is the least informative — and it exists because + // the marker costs a column: without it, arming the degraded state at the + // 80-column baseline pushed the whole gauge off the bar, so the session that + // most needs announcing was the one that announced nothing. right := "" - for _, g := range []string{m.renderRateGauge(false), m.renderRateGauge(true)} { + for _, g := range []string{m.renderRateGauge(false), m.renderRateGauge(true), m.renderRateMarker()} { if g != "" && claim(left)+lipgloss.Width(hints)+claim(g) <= inner { right = g break @@ -432,6 +436,13 @@ const ( // a token, <15 without one. const gaugeDangerRemaining = 100 +// rejectedTokenGlyph marks a session whose stored credential GitHub refused. It +// is the same ✕ the [a] overlay's exhausted-quota icon uses, deliberately: both +// mean "this is broken and it is not going to fix itself", and the two never +// appear on the same surface. Width-stable (1 cell under both runewidth +// conditions), which the gauge's right-edge arithmetic requires. +const rejectedTokenGlyph = "✕" + // gaugeVisible reports whether there is a quota to show. The gauge is on screen // for the whole session once the rate is known: it is also the only place the // API surface is visible at all, so hiding it at rest hid the [a] overlay with @@ -449,6 +460,17 @@ func gaugeVisible(r version.RateLimit) bool { // itself still lives only in the [a] overlay, which the [?] overlay documents — // the bar spends no columns advertising a key. compact drops the bar, keeping // "api used/limit" for narrow terminals. +// +// A rejected token adds a ✕ to the label in both forms — this is the primary +// place the degraded session is announced, and the compact form is the one a +// narrow bar keeps, so dropping the marker there would hide the state exactly +// where the numbers alone are least explicable. It is a suffix rather than a +// relabel to "anon" because a user with no token is also anonymous and that is +// not degradation: the ✕ says a credential was refused, and the limit beside it +// says what is left. One column, sheds with the gauge, and no collision with the +// ⚠/✕ usage-threshold icons, which live only in the [a] overlay. U+2715 measures +// one cell under both runewidth conditions (unlike U+00D7, which is East-Asian +// Ambiguous), so the bar's width arithmetic is unaffected. func (m Model) renderRateGauge(compact bool) string { s := m.sty() r := m.rate @@ -456,7 +478,11 @@ func (m Model) renderRateGauge(compact bool) string { return "" } used := usedOf(r) - label := s.Dim.Render("api ") + label := s.Dim.Render("api") + if version.TokenRejected() { + label += s.Danger.Render(rejectedTokenGlyph) + } + label += s.Dim.Render(" ") nums := s.Dim.Render(fmt.Sprintf("%d/%d", used, r.Limit)) if compact { return label + nums @@ -471,6 +497,25 @@ func (m Model) renderRateGauge(compact bool) string { return label + bar + " " + nums } +// renderRateMarker is the gauge's narrowest form: the rejection alone, with no +// numbers. It exists for one width band — the marker costs a column, and at the +// 80-column baseline that column is what decided between "api 34/60" and no +// gauge at all, which would have hidden the degraded state at exactly the +// default terminal size. +// +// Which half survives follows the bar's own rule, that what sheds is what is +// least actionable: the numbers are a measurement the [a] overlay repeats, the +// ✕ is the only sign anywhere on the screen that requests stopped carrying the +// user's token. It returns "" when there is no rejection to report, so a healthy +// session never gains a form the gauge did not have before. +func (m Model) renderRateMarker() string { + if !version.TokenRejected() || !gaugeVisible(m.rate) { + return "" + } + s := m.sty() + return s.Dim.Render("api") + s.Danger.Render(rejectedTokenGlyph) +} + // usedOf returns consumed requests (Limit-Remaining) clamped to [0,Limit], the // single source of used/limit for both the status-bar gauge and the [a] overlay. // GitHub always reports Remaining in [0,Limit]; the clamp is defensive against a @@ -559,25 +604,57 @@ func maskToken(t string) string { } // renderAPIStatus builds the API-status overlay body: an optional add-token -// nudge (when none is configured), the token source (masked), used/limit with -// the shared icon, and the reset time. +// nudge (when none is configured or the configured one was refused), the token +// source (masked), used/limit with the shared icon, and the reset time. +// +// This is the surface that always has the answer. The gauge announces a rejected +// token but is droppable under width pressure and invisible before the first +// rate snapshot; here the state gets its reason (HTTP 401), its consequence +// (requests run unauthenticated) and the key that fixes it, on a modal nothing +// competes with for room. func (m Model) renderAPIStatus() string { s := m.sty() var b strings.Builder b.WriteString(s.EmphasisBold.Render("github api usage") + "\n\n") - source := version.TokenSource() - // Nudge the user to add a token when none is configured — it lifts the hourly - // limit from 60 to 5000. Hidden once a token exists or while entering one. - if source == "none" && m.mode != modeTokenInput { - b.WriteString(s.Signal.Render("add a github token to raise the limit (60 → 5000/h) ") + m.hint("e", "set") + "\n\n") + source, rejected := version.TokenSource(), version.TokenRejected() + // Nudge the user when there is a token to add or to replace — either way the + // hourly limit is 60 and a working token lifts it to 5000. A rejected token + // needs different wording: "add" reads as advice to someone who has already + // done it, and the thing to do is swap the credential, not create one. + // Hidden while entering one, when the input below is the whole answer. + // + // A rejected *env* token needs its own wording again, and this one is about + // what keepkit cannot do: [e] writes the config file, which effectiveToken + // reads only when GITHUB_TOKEN is empty, so offering the key here would send + // the user through a save that changes nothing on the wire. The variable + // belongs to the shell that launched us, so the shell is where it is named. + // [d] two blocks down already gates on "config" for the same reason. + if m.mode != modeTokenInput { + switch { + case rejected && source == "env": + b.WriteString(s.Signal.Render("GITHUB_TOKEN was refused — replace it in your shell") + "\n\n") + case rejected: + b.WriteString(s.Signal.Render("replace the token to restore the 5000/h limit ") + m.hint("e", "set") + "\n\n") + case source == "none": + b.WriteString(s.Signal.Render("add a github token to raise the limit (60 → 5000/h) ") + m.hint("e", "set") + "\n\n") + } } tokenLine := "token " + source if tok := version.Token(); tok != "" { tokenLine += " (" + maskToken(tok) + ")" } - b.WriteString(s.Text.Render(tokenLine) + "\n") + // The mask is what the user recognises the dead credential by, which is why + // version.Token() reads the raw core and not the suppressed resolveToken: + // blanking the value in exactly the state this line exists to describe would + // leave "token config — rejected" naming no token at all. + if rejected { + b.WriteString(s.Danger.Render(tokenLine+" — rejected (HTTP 401)") + "\n") + b.WriteString(s.Dim.Render("requests run unauthenticated") + "\n") + } else { + b.WriteString(s.Text.Render(tokenLine) + "\n") + } if m.rate.Known { icon := rateIcon(s, m.rate) diff --git a/internal/model/render_test.go b/internal/model/render_test.go index a1851bf..392f564 100644 --- a/internal/model/render_test.go +++ b/internal/model/render_test.go @@ -9,6 +9,7 @@ import ( "slices" "strings" "testing" + "time" "unicode/utf8" "github.com/charmbracelet/bubbles/spinner" @@ -1657,6 +1658,118 @@ func TestRenderRateGauge(t *testing.T) { }) } +// TestRenderRateGaugeMarksARejectedToken pins the session-long announcement: the +// ✕ has to reach BOTH forms, because the compact one is what a narrow bar keeps +// and is exactly where bare numbers explain themselves least. A healthy session +// must carry no mark at all — unauthenticated by choice is not degradation. +func TestRenderRateGaugeMarksARejectedToken(t *testing.T) { + rate := version.RateLimit{Known: true, Remaining: 15, Limit: 60} + + for _, compact := range []bool{false, true} { + name := "full" + if compact { + name = "compact" + } + t.Run(name+" form marks a rejection", func(t *testing.T) { + defer armRejectedToken(t)() + got := stripANSI(Model{rate: rate}.renderRateGauge(compact)) + if !strings.HasPrefix(got, "api"+rejectedTokenGlyph+" ") { + t.Errorf("%s gauge = %q, want the label to lead with api%s", name, got, rejectedTokenGlyph) + } + if !strings.Contains(got, "45/60") { + t.Errorf("%s gauge = %q, want the numbers kept beside the mark", name, got) + } + }) + + t.Run(name+" form is unmarked when healthy", func(t *testing.T) { + got := stripANSI(Model{rate: rate}.renderRateGauge(compact)) + if strings.Contains(got, rejectedTokenGlyph) { + t.Errorf("%s gauge = %q, want no mark without a rejection", name, got) + } + }) + } + + t.Run("the mark is styled Danger, not dim like the rest", func(t *testing.T) { + forceColorProfile(t) + defer armRejectedToken(t)() + got := Model{rate: rate, styles: ui.NewStyles(ui.Default)}.renderRateGauge(true) + danger := ui.NewStyles(ui.Default).Danger.Render(rejectedTokenGlyph) + if !strings.Contains(got, danger) { + t.Errorf("gauge = %q, want the mark rendered through Danger (%q)", got, danger) + } + }) + + t.Run("the mark costs exactly one cell", func(t *testing.T) { + // The gauge sits at the bar's right edge and its width is what + // renderHintsBar reserves, so a two-cell mark would push the bar past the + // terminal and scroll the top border off the alt screen. + // + // Measured under BOTH runewidth conditions, the way langBandGlyph and the + // list markers are: lipgloss.Width follows the ambient + // RUNEWIDTH_EASTASIAN, and CI runs a plain `go test`, so a check through + // it would pass here with U+00D7 — which is exactly the two-cell glyph + // this test exists to reject. + for _, cond := range []bool{false, true} { + c := runewidth.NewCondition() + c.EastAsianWidth = cond + if got := c.StringWidth(rejectedTokenGlyph); got != 1 { + t.Errorf("rejectedTokenGlyph width = %d with EastAsianWidth=%v, want 1", got, cond) + } + } + // And the gauge really spends that one cell, rather than the mark being + // width-stable but rendered somewhere the bar does not measure. + plain := lipgloss.Width(Model{rate: rate}.renderRateGauge(true)) + restore := armRejectedToken(t) + marked := lipgloss.Width(Model{rate: rate}.renderRateGauge(true)) + restore() + if marked-plain != 1 { + t.Errorf("marker costs %d cells, want 1", marked-plain) + } + }) +} + +// TestRenderRateMarkerIsTheLastFormStanding covers the width band the marker's +// own column created: at the 80-col baseline the marked gauge no longer fits, so +// without a marker-only form the degraded session would be the one showing +// nothing. What survives follows the bar's rule — the numbers are a measurement +// the [a] overlay repeats, the ✕ is the only sign anywhere that requests stopped +// carrying the token. +func TestRenderRateMarkerIsTheLastFormStanding(t *testing.T) { + rate := version.RateLimit{Known: true, Remaining: 26, Limit: 60} + + t.Run("nothing to report renders nothing", func(t *testing.T) { + healthy := Model{rate: rate} + if got := healthy.renderRateMarker(); got != "" { + t.Errorf("renderRateMarker() = %q, want empty for a healthy session", got) + } + defer armRejectedToken(t)() + noQuota := Model{} + if got := noQuota.renderRateMarker(); got != "" { + t.Errorf("renderRateMarker() = %q, want empty with no known quota", got) + } + }) + + t.Run("it is narrower than the compact form", func(t *testing.T) { + defer armRejectedToken(t)() + m := Model{rate: rate} + if lipgloss.Width(m.renderRateMarker()) >= lipgloss.Width(m.renderRateGauge(true)) { + t.Error("the marker-only form must be the narrowest, or it would never be reached") + } + }) + + t.Run("the bar keeps the mark at the 80-col baseline", func(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + m := New([]loader.ToolMeta{{Name: "rg", GitHub: "BurntSushi/ripgrep"}}).WithAppVersion("v0.1.0") + m = mustModel(m.Update(tea.WindowSizeMsg{Width: 80, Height: 24})) + m.rate = rate + defer armRejectedToken(t)() + bar := stripANSI(m.renderStatusBar()) + if !strings.Contains(bar, "api"+rejectedTokenGlyph) { + t.Errorf("status bar at 80 cols dropped the rejection mark:\n%s", bar) + } + }) +} + // TestGaugeVisible pins when the gauge is on screen at all: whenever there is a // known quota. It is also the only visible sign that keepkit has an API surface, // so hiding it at rest hid the [L] overlay along with it — the numbers are the @@ -2500,15 +2613,90 @@ func TestUpdateInstalledAndRemoteMsgPopulateCaches(t *testing.T) { t.Errorf("reversed order versions[gh] = %+v, want {Installed:1.0 Latest:2.0}", got) } - // remoteMsg with err set must not touch the caches. + // remoteMsg with an error and nothing to show must not touch the caches: + // there is no data, so writing would only blank what a previous pass left. m = newModel() - updated, _ = m.Update(remoteMsg{toolName: "gh", latest: "2.0", err: errBoom}) + updated, _ = m.Update(remoteMsg{toolName: "gh", err: errBoom}) nm = updated.(Model) if _, ok := nm.repoCards["gh"]; ok { - t.Errorf("repoCards populated despite remoteMsg error") + t.Errorf("repoCards populated by an empty failed remoteMsg") } if got := nm.versions["gh"]; got.Latest != "" { - t.Errorf("versions[gh].Latest = %q, want empty on remoteMsg error", got.Latest) + t.Errorf("versions[gh].Latest = %q, want empty on an empty failed remoteMsg", got.Latest) + } + + // remoteMsg with an error that still carried data DOES populate: the values + // came up from the cache and are what the tool showed a moment ago. Anything + // else blanks a tool for a failure it survived. + m = newModel() + updated, _ = m.Update(remoteMsg{toolName: "gh", latest: "2.0", err: errBoom}) + nm = updated.(Model) + if got := nm.versions["gh"]; got.Latest != "2.0" { + t.Errorf("versions[gh].Latest = %q, want 2.0 kept through a failed pass", got.Latest) + } +} + +// TestRemoteMsgNonRateLimitErrorKeepsData pins the rule the caches are written +// by: a pass that failed but still carried values up from the cache renders +// them. The error class is deliberately not ErrRateLimited — that was the one +// class the old predicate admitted, so a 401 or a 5xx blanked a tool whose data +// the pass had in hand. +func TestRemoteMsgNonRateLimitErrorKeepsData(t *testing.T) { + m := Model{ + meta: []loader.ToolMeta{{Name: "gh", GitHub: "cli/cli"}}, + versions: map[string]VersionInfo{}, + repoStatus: map[string]string{}, + repoCards: map[string]version.RepoCard{}, + changelogData: map[string]changelogMsg{}, + } + m.tools = loader.ToolsFromMeta(m.meta) + + updated, _ := m.Update(remoteMsg{ + toolName: "gh", + latest: "2.0", + repoStatus: "active", + card: version.RepoCard{About: "the github cli"}, + err: version.ErrTokenInvalid, + }) + nm := updated.(Model) + + if got := nm.versions["gh"].Latest; got != "2.0" { + t.Errorf("versions[gh].Latest = %q, want 2.0", got) + } + if got := nm.repoStatus["gh"]; got != "active" { + t.Errorf("repoStatus[gh] = %q, want active", got) + } + if got, ok := nm.repoCards["gh"]; !ok || got.About != "the github cli" { + t.Errorf("repoCards[gh] = %+v (ok=%v), want the cached card", got, ok) + } +} + +// TestRemoteMsgRateLimitedWithNoDataStillFlags verifies the second case is still +// reachable under the widened first one: a rate-limited pass with nothing to +// show must land on the "rate limited — press a" marker rather than fall through +// to the data path and write blanks. +func TestRemoteMsgRateLimitedWithNoDataStillFlags(t *testing.T) { + m := Model{ + meta: []loader.ToolMeta{{Name: "gh", GitHub: "cli/cli"}}, + versions: map[string]VersionInfo{}, + repoStatus: map[string]string{}, + repoCards: map[string]version.RepoCard{}, + changelogData: map[string]changelogMsg{}, + } + m.tools = loader.ToolsFromMeta(m.meta) + + updated, _ := m.Update(remoteMsg{ + toolName: "gh", + repoStatus: "rate-limited", + err: version.ErrRateLimited, + }) + nm := updated.(Model) + + if got := nm.repoStatus["gh"]; got != "rate-limited" { + t.Errorf("repoStatus[gh] = %q, want rate-limited", got) + } + if _, ok := nm.repoCards["gh"]; ok { + t.Error("repoCards written by a pass that carried no card") } } @@ -2694,6 +2882,213 @@ func TestRenderAPIStatusTokenHint(t *testing.T) { }) } +// rejectedOverlayModel returns an overlay-mode model with a real config token +// stored, so the token line has a source and a mask to print. The seam keeps it +// out of the user's config, and the token is long enough for maskToken to keep +// its first and last four characters. +func rejectedOverlayModel(t *testing.T, rejected bool, mode inputMode) Model { + t.Helper() + restore := version.SetConfigDirForTesting(t.TempDir()) + t.Cleanup(restore) + t.Setenv("GITHUB_TOKEN", "") + if err := version.SetToken("ghp_notarealtoken"); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = version.ClearToken() }) + t.Cleanup(version.SetTokenRejectedForTesting(rejected)) + + return Model{ + width: 80, height: 24, mode: mode, + tokenInput: textinput.New(), + rate: version.RateLimit{Known: true, Remaining: 26, Limit: 60}, + } +} + +// TestRenderAPIStatusExplainsARejectedToken pins the surface that always has the +// answer. The gauge's ✕ is droppable and says only "something is wrong"; this is +// where the state gets its reason, its consequence and the key that fixes it. +// +// The mask is the load-bearing part: it is how the user recognises WHICH +// credential to replace, and it survives only because version.Token() reads the +// raw core rather than the suppressed resolveToken (task 4's split). +func TestRenderAPIStatusExplainsARejectedToken(t *testing.T) { + t.Run("the rejected line keeps source and mask", func(t *testing.T) { + m := rejectedOverlayModel(t, true, modeAPIStatus) + got := stripANSI(m.renderAPIStatus()) + for _, want := range []string{"token config", "ghp_", "oken", "rejected (HTTP 401)"} { + if !strings.Contains(got, want) { + t.Errorf("overlay missing %q:\n%s", want, got) + } + } + if !strings.Contains(got, "requests run unauthenticated") { + t.Errorf("overlay does not say what the rejection means for the session:\n%s", got) + } + }) + + t.Run("the rejected line is styled Danger", func(t *testing.T) { + forceColorProfile(t) + m := rejectedOverlayModel(t, true, modeAPIStatus) + m.styles = ui.NewStyles(ui.Default) + got := m.renderAPIStatus() + if !strings.Contains(got, themeSeq(ui.Default.Danger)) { + t.Errorf("overlay carries no Danger run for the rejection:\n%q", got) + } + }) + + t.Run("a healthy token renders neither", func(t *testing.T) { + m := rejectedOverlayModel(t, false, modeAPIStatus) + got := stripANSI(m.renderAPIStatus()) + if !strings.Contains(got, "token config") { + t.Fatalf("fixture: the token line is missing:\n%s", got) + } + for _, unwanted := range []string{"rejected", "unauthenticated", "replace the token"} { + if strings.Contains(got, unwanted) { + t.Errorf("healthy overlay carries %q:\n%s", unwanted, got) + } + } + }) + + t.Run("the nudge tells a rejected user to replace, not to add", func(t *testing.T) { + m := rejectedOverlayModel(t, true, modeAPIStatus) + got := stripANSI(m.renderAPIStatus()) + if !strings.Contains(got, "replace the token") { + t.Errorf("overlay missing the replace nudge:\n%s", got) + } + // "add a github token" reads as advice to someone who already did. + if strings.Contains(got, "add a github token") { + t.Errorf("overlay tells a user who HAS a token to add one:\n%s", got) + } + }) + + t.Run("the nudge stays hidden while entering a token", func(t *testing.T) { + m := rejectedOverlayModel(t, true, modeTokenInput) + got := stripANSI(m.renderAPIStatus()) + if strings.Contains(got, "replace the token to restore") { + t.Errorf("nudge shown over the open input, which is already the answer:\n%s", got) + } + // The line itself stays: it is the state, not a prompt. + if !strings.Contains(got, "rejected (HTTP 401)") { + t.Errorf("the rejected line vanished under the input:\n%s", got) + } + }) +} + +// envRejectedOverlayModel returns an overlay-mode model whose rejected token +// came from GITHUB_TOKEN. The config-dir seam is still installed, because the +// [e] path this test is about writes there — the point being that the write +// changes nothing while the env var stands. +func envRejectedOverlayModel(t *testing.T) Model { + t.Helper() + restoreDir := version.SetConfigDirForTesting(t.TempDir()) + t.Cleanup(restoreDir) + t.Setenv("GITHUB_TOKEN", "ghp_E1n2V3t4O5f6G7h8") + restoreFlag := version.SetTokenRejectedForTesting(true) + t.Cleanup(restoreFlag) + + return Model{ + width: 80, height: 24, mode: modeAPIStatus, + tokenInput: textinput.New(), + rate: version.RateLimit{Known: true, Remaining: 26, Limit: 60}, + } +} + +// overlayNudgeRow returns the overlay's nudge line — the fourth rendered row, +// between the title and the token line — with the border and ANSI stripped. +// Asserted by position rather than by a substring search over the whole modal, +// because the footer hints carry "e set token" too: a nudge that wrongly +// offered the key would pass any test that only grepped the output. +func overlayNudgeRow(t *testing.T, m Model) string { + t.Helper() + rows := strings.Split(stripANSI(m.renderAPIStatus()), "\n") + if len(rows) < 4 { + t.Fatalf("overlay has %d rows, too few to hold a nudge:\n%s", len(rows), strings.Join(rows, "\n")) + } + // 0 top border, 1 title, 2 blank, 3 nudge. + return rows[3] +} + +// TestAPIStatusNudgeIsEnvAware pins the one rejected state keepkit cannot fix +// from inside. [e] runs SetToken, which writes the config file, and +// effectiveToken reads that only when GITHUB_TOKEN is empty — so under env +// precedence the key saves a credential that never goes on the wire. Offering +// it there sends the user through a save that changes nothing and, before the +// handler learned to stop, through a full recovery fan-out at the anonymous +// 60/h ceiling. [d] has gated on the source for this reason all along. +func TestAPIStatusNudgeIsEnvAware(t *testing.T) { + t.Run("an env rejection names the variable and offers no key", func(t *testing.T) { + nudge := overlayNudgeRow(t, envRejectedOverlayModel(t)) + if !strings.Contains(nudge, "GITHUB_TOKEN") { + t.Errorf("nudge row %q does not name the variable the user has to go edit", nudge) + } + if !strings.Contains(nudge, "shell") { + t.Errorf("nudge row %q does not say where the variable lives", nudge) + } + // The whole point: the key cannot fix this one. + if strings.Contains(nudge, "e set") { + t.Errorf("nudge row %q offers [e], which writes a config token that env shadows", nudge) + } + if strings.Contains(nudge, "replace the token to restore") { + t.Errorf("nudge row %q is the config wording, which points at [e]", nudge) + } + }) + + t.Run("a config rejection still offers the key", func(t *testing.T) { + // The env case is a new first arm of the same switch; it must not swallow + // the case that [e] genuinely answers. + nudge := overlayNudgeRow(t, rejectedOverlayModel(t, true, modeAPIStatus)) + if !strings.Contains(nudge, "replace the token to restore") { + t.Errorf("nudge row %q lost the config wording", nudge) + } + if !strings.Contains(nudge, "e set") { + t.Errorf("nudge row %q no longer offers [e], which is what fixes a config token", nudge) + } + if strings.Contains(nudge, "GITHUB_TOKEN") { + t.Errorf("nudge row %q names the env var for a config token", nudge) + } + }) + + t.Run("the env nudge is styled Signal like its siblings", func(t *testing.T) { + forceColorProfile(t) + m := envRejectedOverlayModel(t) + m.styles = ui.NewStyles(ui.Default) + if !strings.Contains(m.renderAPIStatus(), themeSeq(ui.Default.Signal)) { + t.Error("the env nudge carries no Signal run; it must read as the one thing to act on") + } + }) + + t.Run("the env wording stays inside the 80-col budget", func(t *testing.T) { + m := envRejectedOverlayModel(t) + m.rate.Reset = time.Now().Add(37 * time.Minute) + if w := lipgloss.Width(m.renderAPIStatus()); w > 76 { + t.Errorf("framed width = %d, want <= 76 (PlaceOverlay clips against an 80-col background)", w) + } + }) +} + +// TestRenderAPIStatusSizeBudget: the overlay composites over the 80x24 layout +// through PlaceOverlay, which CLIPS rather than wraps — a line past the +// background width loses its tail silently. The rejected state adds the widest +// line the overlay has ever carried and nothing pinned its width before. +func TestRenderAPIStatusSizeBudget(t *testing.T) { + for _, mode := range []inputMode{modeAPIStatus, modeTokenInput} { + for _, rejected := range []bool{false, true} { + m := rejectedOverlayModel(t, rejected, mode) + // A reset time is the one field the fixtures above leave out, and it + // adds a line — include it so the height budget is measured full. + m.rate.Reset = time.Now().Add(37 * time.Minute) + overlay := m.renderAPIStatus() + if w := lipgloss.Width(overlay); w > 76 { + t.Errorf("mode=%v rejected=%v: framed width = %d, want <= 76 (80-col background)", + mode, rejected, w) + } + if h := lipgloss.Height(overlay); h > 20 { + t.Errorf("mode=%v rejected=%v: framed height = %d, want <= 20 (24-row background)", + mode, rejected, h) + } + } + } +} + // sgrParamRe captures the parameter list of each SGR escape sequence. var sgrParamRe = regexp.MustCompile(`\x1b\[([0-9;]*)m`) @@ -2831,6 +3226,68 @@ func TestTokenValidatedMsgValid(t *testing.T) { } } +// armRejectedToken stores a token and marks it refused, returning the restore +// func. internal/model cannot reach version.rejectToken (unexported, and armed +// only by a real 401), so the exported seam is the only way to render the +// degraded state in a test that must not touch the network. +func armRejectedToken(t *testing.T) (restore func()) { + t.Helper() + t.Setenv("GITHUB_TOKEN", "") + restoreDir := version.SetConfigDirForTesting(t.TempDir()) + if err := version.SetToken("ghp_refusedtoken"); err != nil { + t.Fatal(err) + } + restoreFlag := version.SetTokenRejectedForTesting(true) + return func() { + restoreFlag() + _ = version.ClearToken() + restoreDir() + } +} + +// TestRejectedTokenHasNoCachedCopy pins the shape the surfaces read from. The +// rejection was briefly mirrored into a Model field, written from a snapshot +// remoteCmd/fetchRateCmd took inside the goroutine after the fetch — and that +// created a window with no upside: a reply in flight across the keystroke that +// replaced the credential carried a value observed under the OLD token, so it +// re-armed the flag after the accepted tokenValidatedMsg had cleared it and the +// overlay redrew "rejected (HTTP 401)" beside the mask of a token that had just +// validated. +// +// There is nothing to go stale now: the two renderers ask version.TokenRejected() +// at paint time, exactly as the token line beside them already asks +// version.TokenSource() and version.Token(). This test is what stops the cache +// from coming back. +func TestRejectedTokenHasNoCachedCopy(t *testing.T) { + restore := armRejectedToken(t) + m := Model{width: 80, height: 24, rate: version.RateLimit{Known: true, Limit: 60, Remaining: 26}} + + if !strings.Contains(stripANSI(m.renderStatusBar()), rejectedTokenGlyph) { + t.Fatal("fixture: the armed rejection does not reach the bar") + } + + // Messages carry no rejection field to go stale with — feeding the handlers + // the very messages that used to re-arm it changes nothing. + m.versions = map[string]VersionInfo{} + m.repoStatus = map[string]string{} + m.repoCards = map[string]version.RepoCard{} + m.changelogData = map[string]changelogMsg{} + m.meta = []loader.ToolMeta{{Name: "gh", GitHub: "cli/cli"}} + m.tools = loader.ToolsFromMeta(m.meta) + + restore() + + for _, msg := range []tea.Msg{ + rateMsg{rate: version.RateLimit{Known: true, Limit: 5000, Remaining: 4999}}, + remoteMsg{toolName: "gh", latest: "v1.0"}, + } { + nm := mustModel(m.Update(msg)) + if strings.Contains(stripANSI(nm.renderStatusBar()), rejectedTokenGlyph) { + t.Errorf("%T left the bar marked after the rejection ended", msg) + } + } +} + // TestUpdateAPIStatusRemoveToken verifies [d] clears a config-sourced token. func TestUpdateAPIStatusRemoveToken(t *testing.T) { t.Setenv("GITHUB_TOKEN", "") @@ -4317,8 +4774,15 @@ func TestStatusBarNeverWraps(t *testing.T) { selfTag string updating bool knownRate bool + rejected bool width int }{ + // The rejection marker costs the gauge a column, and the bar is measured + // against the whole width: at the baseline it is the last thing added to + // a line that already fits exactly. + {name: "rejected token", focus: focusTools, helpMode: helpModeHelp, knownRate: true, rejected: true}, + {name: "rejected token at 80 cols in brief", focus: focusBrief, helpMode: helpModeHelp, knownRate: true, rejected: true, width: 80}, + {name: "rejected token under a self banner", focus: focusTools, helpMode: helpModeHelp, self: selfOffered, knownRate: true, rejected: true}, {name: "tools", focus: focusTools, helpMode: helpModeHelp}, {name: "brief", focus: focusBrief, helpMode: helpModeHelp}, {name: "help mode", focus: focusHelp, helpMode: helpModeHelp}, @@ -4384,6 +4848,9 @@ func TestStatusBarNeverWraps(t *testing.T) { if tc.knownRate { m.rate = version.RateLimit{Known: true, Limit: 60, Remaining: 42} } + if tc.rejected { + defer armRejectedToken(t)() + } if got := lipgloss.Height(m.renderStatusBar()); got != 3 { t.Errorf("status bar height = %d, want 3 (border + one hint line)", got) diff --git a/internal/model/status_test.go b/internal/model/status_test.go index 728597d..328689c 100644 --- a/internal/model/status_test.go +++ b/internal/model/status_test.go @@ -7,6 +7,9 @@ import ( "time" tea "github.com/charmbracelet/bubbletea" + + "github.com/stanlyzoolo/keepkit/internal/loader" + "github.com/stanlyzoolo/keepkit/internal/version" ) // shrinkStatusTTL shortens statusMsgTTL so a test can invoke the tick Cmd that @@ -221,3 +224,121 @@ func TestStatusBarReturnsHintsAfterExpiry(t *testing.T) { t.Errorf("status bar = %q, want the global hints back", bar) } } + +// refreshingModel returns a model mid-[r] for the named tool: refreshingFor set, +// the maps the remoteMsg handler writes through initialised. +func refreshingModel(t *testing.T, name string) Model { + t.Helper() + m := New([]loader.ToolMeta{{Name: name, GitHub: "cli/cli"}}) + m.width, m.height = 80, 24 + m.focus = focusBrief + m.refreshingFor = name + return m +} + +// TestRefreshAnswersEveryPress pins the promise [r] makes. Before this, success, +// a rate limit, a 401, a timeout and a dropped connection were one gesture: the +// spinner turns and the card does not change, so a user could not tell a tool +// that is up to date from a tool whose data has not been fetched in a day. +// +// The predicate is err, not !conclusive. The broader one reported a network +// failure that never happened for a ref the version layer refused outright — an +// unsupported or spoofed host, answered with a bare RepoData and a nil error, +// no request made — and it contradicted a card the user had just watched update +// on a partial pass that fetched a new tag and lost only the repo info. Now that +// every real failure carries a name, the error is the honest question to ask; +// the two silent rows below are what pin the difference. +func TestRefreshAnswersEveryPress(t *testing.T) { + tests := []struct { + name string + msg remoteMsg + wantStatus string + }{ + { + name: "rate limited names the key that raises the ceiling", + msg: remoteMsg{toolName: "gh", err: version.ErrRateLimited}, + wantStatus: "refresh failed: rate limited — press [a]", + }, + { + name: "a rejected token is not called out separately", + msg: remoteMsg{toolName: "gh", err: version.ErrTokenInvalid}, + wantStatus: "refresh failed: network error", + }, + { + name: "a transient failure reads the same whatever it was", + msg: remoteMsg{toolName: "gh", err: errBoom}, + wantStatus: "refresh failed: network error", + }, + { + // The version layer refuses an unsupported or spoofed host before + // making any request and answers a bare RepoData — no error, not + // conclusive. Nothing failed to fetch, so claiming a network error + // would name a cause that never existed. + name: "a ref the version layer refused says nothing", + msg: remoteMsg{toolName: "gh"}, + wantStatus: "", + }, + { + // A partial pass: the release fetch landed a new tag, only the repo + // card was lost. Conclusive is false (CheckedAt stays stale for the + // refill) but the card visibly updated, so the bar must not + // contradict what the user just watched happen. + name: "a partial pass that fetched a tag says nothing", + msg: remoteMsg{toolName: "gh", latest: "v2.0.0"}, + wantStatus: "", + }, + { + name: "a conclusive pass stays silent — the repainted card is the answer", + msg: remoteMsg{toolName: "gh", latest: "v2.0.0", conclusive: true}, + wantStatus: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // The tick is constructed inside Update, so the TTL must shrink first. + shrinkStatusTTL(t) + m := refreshingModel(t, "gh") + updated, cmd := m.Update(tt.msg) + nm := updated.(Model) + + if nm.statusMsg != tt.wantStatus { + t.Errorf("statusMsg = %q, want %q", nm.statusMsg, tt.wantStatus) + } + if nm.refreshingFor != "" { + t.Errorf("refreshingFor = %q, want cleared whatever the outcome", nm.refreshingFor) + } + if tt.wantStatus == "" { + if cmd != nil { + t.Errorf("a silent success returned a cmd (%T), want none", cmd()) + } + return + } + // Nothing may ride along: [r] answering is a message, not a retry. + assertOnlyExpiryTick(t, cmd) + }) + } +} + +// TestRefreshStatusOnlyForTheRefreshedTool: the background passes Init fires are +// inconclusive all the time (offline start, rate limit) and must not put a +// "refresh failed" message on the bar for a gesture the user never made. +func TestRefreshStatusOnlyForTheRefreshedTool(t *testing.T) { + shrinkStatusTTL(t) + m := refreshingModel(t, "gh") + m.meta = append(m.meta, loader.ToolMeta{Name: "rg", GitHub: "BurntSushi/ripgrep"}) + m.tools = loader.ToolsFromMeta(m.meta) + + updated, cmd := m.Update(remoteMsg{toolName: "rg", err: version.ErrRateLimited}) + nm := updated.(Model) + + if nm.statusMsg != "" { + t.Errorf("statusMsg = %q, want silence for a tool nobody refreshed", nm.statusMsg) + } + if nm.refreshingFor != "gh" { + t.Errorf("refreshingFor = %q, want gh still in flight", nm.refreshingFor) + } + if cmd != nil { + t.Errorf("returned a cmd (%T), want none", cmd()) + } +} diff --git a/internal/version/github.go b/internal/version/github.go index 599ccb2..4c94c81 100644 --- a/internal/version/github.go +++ b/internal/version/github.go @@ -118,11 +118,24 @@ func shouldReplaceRate(cur, snap RateLimit, now time.Time) bool { // different media type (fetchReadme asks for raw markdown) can pre-set it and // still ride the shared auth + rate-accounting path instead of a second HTTP // code path. +// +// A 401 to a request that carried a token is answered by retrying the same +// request once without it. An expired token is strictly worse than no token — +// unauthenticated the very same URLs answer 200 at 60 requests/hour, while +// authenticated with a dead credential every single one is rejected — so a +// blackout degrades into a working, if smaller, session instead. The retry lives +// here because this is the only place that decides to send a token at all; a +// startup pre-check cannot help, since Init fires the rate seed and every repo +// pass in one batch and no check would land before the passes it was meant to +// warn. Cost: with the passes running in parallel a few requests pay their own +// retry before the mark lands — bounded by the tool count, and those requests +// would have failed outright anyway. func doGH(req *http.Request) (*http.Response, error) { if req.Header.Get("Accept") == "" { req.Header.Set("Accept", "application/vnd.github.v3+json") } - if token := resolveToken(); token != "" { + token := resolveToken() + if token != "" { req.Header.Set("Authorization", "Bearer "+token) } resp, err := ghClient.Do(req) @@ -131,7 +144,38 @@ func doGH(req *http.Request) (*http.Response, error) { return nil, err } updateRateFromHeaders(resp.Header) - return resp, nil + if token == "" || resp.StatusCode != http.StatusUnauthorized { + return resp, nil + } + + // Bad credentials. Drain and close before reusing the connection, then retry + // the same request without the header. All GitHub calls here are bodiless + // GETs, so the clone carries everything that matters. + _, _ = io.Copy(io.Discard, resp.Body) + _ = resp.Body.Close() + + retry := req.Clone(req.Context()) + retry.Header.Del("Authorization") + retryResp, retryErr := ghClient.Do(retry) + + // Mark the token only once the retry has answered, because a 401 is evidence + // about the *credential* only when dropping it changes the answer. A host + // that 401s an anonymous request too — a proxy, an enterprise instance — is + // refusing the resource, and marking the token there would strip + // Authorization for the rest of the session and put "rejected (HTTP 401)" + // beside a credential that is fine. A transport failure on the retry teaches + // nothing new, so the first response's 401 stands as the evidence. + if retryErr != nil || retryResp.StatusCode != http.StatusUnauthorized { + rejectToken(token) + } + if retryErr != nil { + logx.Errorf("version.doGH: %s %s (unauthenticated retry): %v", retry.Method, retry.URL.Path, retryErr) + return nil, retryErr + } + // The retry's headers are the honest ones: they carry the anonymous 60/h + // window, which is what the gauge must show from here on. + updateRateFromHeaders(retryResp.Header) + return retryResp, nil } // ErrRateLimited signals that a GitHub request was rejected because the API @@ -139,8 +183,17 @@ func doGH(req *http.Request) (*http.Response, error) { // use errors.Is to degrade gracefully instead of showing a raw HTTP error. var ErrRateLimited = errors.New("github api rate limit exceeded") -// ErrTokenInvalid signals that a candidate token failed validation against -// GET /rate_limit (HTTP 401). Used by FetchRateWithToken before persistence. +// ErrTokenInvalid signals that GitHub rejected the credentials a request +// carried (HTTP 401). Two producers: FetchRateWithToken, which validates a +// candidate token before persistence, and classifyStatus, which names the code +// wherever it surfaces. +// +// On the shared request path it is a classification rather than a hot path: +// doGH consumes a 401 that a token earned, retries the request anonymously and +// returns that response instead (see doGH), and GitHub does not answer 401 to a +// request carrying no credentials at all. What is left for classifyStatus to +// name is the defensive case — a proxy or an enterprise host that 401s an +// anonymous request — plus the log line either way. var ErrTokenInvalid = errors.New("github token invalid") // errNoReleases signals that a repo's /releases/latest returned 404 — the repo @@ -163,9 +216,11 @@ const readmeMaxBytes = 512 << 10 // classifyStatus maps a non-2xx GitHub response to an error. A 403 or 429 whose // own X-RateLimit-Remaining header reads 0 is rate-limit exhaustion and returns // ErrRateLimited; a 403 with remaining>0 is a genuine access denial and returns a -// generic HTTP error. Remaining is read from this response's own headers, never -// from the global rl snapshot, because a concurrent request may overwrite rl -// between this request's accounting and its classification. +// generic HTTP error. A 401 is bad credentials and returns ErrTokenInvalid, so +// the one class of failure the user can actually fix carries a name instead of +// an anonymous "HTTP 401". Remaining is read from this response's own headers, +// never from the global rl snapshot, because a concurrent request may overwrite +// rl between this request's accounting and its classification. func classifyStatus(resp *http.Response) error { remaining := resp.Header.Get("X-RateLimit-Remaining") path := "" @@ -179,6 +234,13 @@ func classifyStatus(resp *http.Response) error { return ErrRateLimited } } + // A 401 is worth a line: unlike a 404 it is neither conclusive nor normal, + // and it is the one status whose fix is in the user's hands. + if resp.StatusCode == http.StatusUnauthorized { + logx.Errorf("version.classifyStatus: %s http=%d remaining=%s: token rejected", + path, resp.StatusCode, remaining) + return ErrTokenInvalid + } // A 404 is a conclusive "not found" (a stale/private/renamed repo ref), not // a transient failure. It would recur on every startup and re-create the // session log each launch, defeating the "a log file means something went @@ -250,6 +312,12 @@ func FetchRate() (RateLimit, error) { // or the global rl snapshot, so an unpersisted token never leaks into shared // state. A 401 returns ErrTokenInvalid; callers persist via SetToken only after a // successful (200) result. +// +// Going straight to ghClient rather than through doGH is load-bearing, not an +// incidental detail: doGH answers a 401 by retrying the request without the +// token, so this function would see that anonymous 200 and report a dead +// credential as valid — and then persist it. Validation is the one caller that +// must observe the rejection instead of surviving it. func FetchRateWithToken(token string) (RateLimit, error) { req, err := http.NewRequest("GET", apiBase()+"/rate_limit", nil) if err != nil { @@ -376,10 +444,12 @@ type RepoData struct { Body string HtmlUrl string PublishedAt string - // Err carries a classified fetch error (currently only ErrRateLimited) so - // callers can degrade gracefully via errors.Is. It is set when a release or - // repo-info fetch was rejected for rate limiting; the returned data may still - // hold stale values from the cache. + // Err carries the more actionable of the two core fetches' errors (see + // pickFetchErr) so callers can name what went wrong instead of showing a + // silent no-op. ErrRateLimited is the one class with a specific answer; + // everything else is transient. A definitive "no releases" is not a failure + // and never lands here. The returned data may still hold stale values from + // the cache — the error is advisory, not a signal that the data is unusable. Err error // Conclusive reports that this answer settles the tool for the cache window: // the entry is fresh, either because a pass just stamped CheckedAt or because @@ -387,14 +457,37 @@ type RepoData struct { // entry stale for a retry — a total fetch failure, and a partial one whose // missing half must still be refilled. // - // It exists because Err cannot carry that: Err holds ErrRateLimited or nil, so - // an offline start and a 5xx both reach the caller as a nil error. A consumer - // that read "no error" as "answered" would stop retrying for the session on - // exactly the passes that fetched nothing at all (internal/model's - // remoteAnswered marker did). + // It exists because Err cannot carry that, and still cannot now that Err + // names every failure class: a definitive "no releases" is a nil Err on a + // pass that settled the tool, while a rate-limited pass that served a stale + // card is a non-nil Err on one that did not. The two questions — what went + // wrong, and is this tool done for the window — have different answers, and a + // consumer that read "no error" as "answered" stopped retrying on exactly the + // passes that fetched nothing (internal/model's remoteAnswered marker did). Conclusive bool } +// pickFetchErr chooses which of the two core fetches' errors reaches the caller. +// ErrRateLimited outranks everything else: it is the one class with an answer +// the user can act on (wait, or add a token), while every other failure — 401, +// 5xx, timeout, DNS, a broken transport — is transient and gets the same +// treatment, so the UI has nothing to tell them apart with and picking between +// them would be arbitrary. errNoReleases never participates: a repo without +// releases is a conclusive negative, not a failure, and surfacing it as one +// would put an error on the most ordinary card there is. +func pickFetchErr(relErr, infoErr error) error { + if errors.Is(relErr, ErrRateLimited) || errors.Is(infoErr, ErrRateLimited) { + return ErrRateLimited + } + if relErr != nil && !errors.Is(relErr, errNoReleases) { + return relErr + } + if infoErr != nil { + return infoErr + } + return nil +} + func repoDataFromEntry(e CacheEntry) RepoData { return RepoData{ Latest: e.Latest, @@ -462,13 +555,10 @@ func getRepoData(githubField string, force bool) RepoData { repoStatus, about, stars, infoErr := fetchRepoInfo(repo) langs, _ := fetchLanguages(repo) - // Surface a rate-limit classification so the UI can render "rate limited" - // instead of a bare failure. The data itself may still carry stale cache + // Surface the more actionable failure so the UI can name it instead of + // rendering a silent no-op. The data itself may still carry stale cache // values; the error is advisory. - var rlErr error - if errors.Is(relErr, ErrRateLimited) || errors.Is(infoErr, ErrRateLimited) { - rlErr = ErrRateLimited - } + fetchErr := pickFetchErr(relErr, infoErr) // Total fetch failure (offline / rate-limited): both the release and the repo // info endpoints failed. Do not write a fresh-but-empty entry — since freshness @@ -477,10 +567,11 @@ func getRepoData(githubField string, force bool) RepoData { // cache was cold) so the next start retries. if relErr != nil && infoErr != nil { d := repoDataFromEntry(entry) - d.Err = rlErr + d.Err = fetchErr // Conclusive stays false: nothing was fetched and nothing was written, so - // the caller must keep retrying. rlErr is nil for an offline or 5xx - // failure, which is precisely why the flag and not the error carries this. + // the caller must keep retrying. The flag and not the error carries that — + // a definitive "no releases" arrives with a nil error on a pass that DID + // settle the tool, so the two cannot be derived from one another. return d } @@ -544,7 +635,7 @@ func getRepoData(githubField string, force bool) RepoData { return e }) d := repoDataFromEntry(stored) - d.Err = rlErr + d.Err = fetchErr d.Conclusive = conclusive return d } diff --git a/internal/version/github_test.go b/internal/version/github_test.go index 477b020..16efa05 100644 --- a/internal/version/github_test.go +++ b/internal/version/github_test.go @@ -5,6 +5,7 @@ import ( "errors" "net/http" "net/http/httptest" + "strconv" "strings" "sync" "testing" @@ -682,6 +683,286 @@ func TestClassifyStatusRateLimited(t *testing.T) { } } +// TestClassifyStatusTaxonomy pins the closed set of named failure classes and, +// just as importantly, their neighbours: a 401 is bad credentials and must carry +// ErrTokenInvalid, while the two 403 forms and a 404 must keep answering exactly +// what they answered before. The 401 branch sits between them, so a misplaced +// return would show up here as a neighbour changing class. +func TestClassifyStatusTaxonomy(t *testing.T) { + tests := []struct { + name string + code int + remaining string + want error // nil means "generic, named neither sentinel" + }{ + {"401 is a rejected token", http.StatusUnauthorized, "", ErrTokenInvalid}, + {"401 with a remaining header is still a rejected token", http.StatusUnauthorized, "4999", ErrTokenInvalid}, + {"403 exhausted is rate limited", http.StatusForbidden, "0", ErrRateLimited}, + {"429 exhausted is rate limited", http.StatusTooManyRequests, "0", ErrRateLimited}, + {"403 with quota left is a plain denial", http.StatusForbidden, "37", nil}, + {"404 is generic", http.StatusNotFound, "4999", nil}, + {"500 is generic", http.StatusInternalServerError, "4999", nil}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := classifyStatus(respWith(t, tt.code, tt.remaining, "/repos/cli/cli")) + if err == nil { + t.Fatalf("classifyStatus(%d) = nil, want an error", tt.code) + } + if tt.want != nil { + if !errors.Is(err, tt.want) { + t.Fatalf("classifyStatus(%d) = %v, want %v", tt.code, err, tt.want) + } + return + } + if errors.Is(err, ErrTokenInvalid) || errors.Is(err, ErrRateLimited) { + t.Fatalf("classifyStatus(%d) = %v, want an unnamed generic error", tt.code, err) + } + }) + } +} + +// authProbeServer answers 401 to any request carrying an Authorization header +// and 200 to any request without one, recording every request's auth state in +// order. It is the shape of a GitHub that has expired the stored credential: the +// same URLs work perfectly well anonymously. +type authProbeServer struct { + *httptest.Server + mu sync.Mutex + auth []bool // one entry per request: did it carry Authorization? +} + +func newAuthProbeServer(t *testing.T, body string) *authProbeServer { + t.Helper() + p := &authProbeServer{} + p.Server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + authed := r.Header.Get("Authorization") != "" + p.mu.Lock() + p.auth = append(p.auth, authed) + p.mu.Unlock() + if authed { + w.WriteHeader(http.StatusUnauthorized) + return + } + // All three headers, since updateRateFromHeaders drops an observation + // missing any one of them. + w.Header().Set("X-RateLimit-Limit", "60") + w.Header().Set("X-RateLimit-Remaining", "59") + w.Header().Set("X-RateLimit-Reset", strconv.FormatInt(time.Now().Add(time.Hour).Unix(), 10)) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(body)) + })) + t.Cleanup(p.Close) + return p +} + +func (p *authProbeServer) requests() []bool { + p.mu.Lock() + defer p.mu.Unlock() + return append([]bool(nil), p.auth...) +} + +// TestDoGHRetriesUnauthenticatedOn401 is the degradation this whole change +// rests on: an expired token is strictly worse than no token, because the very +// same URLs answer 200 without one. A 401 must therefore cost one wasted request +// and then work, not blank the session. +func TestDoGHRetriesUnauthenticatedOn401(t *testing.T) { + dir := t.TempDir() + resetTokenState(t, dir) + t.Setenv("GITHUB_TOKEN", "") + resetRate(t) + if err := SetToken("ghp_expired"); err != nil { + t.Fatal(err) + } + + srv := newAuthProbeServer(t, `{"tag_name":"v1.0.0"}`) + + req, err := http.NewRequest("GET", srv.URL+"/repos/cli/cli/releases/latest", nil) + if err != nil { + t.Fatal(err) + } + resp, err := doGH(req) + if err != nil { + t.Fatalf("doGH: %v", err) + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusOK { + t.Errorf("status = %d, want 200 from the unauthenticated retry", resp.StatusCode) + } + if got := srv.requests(); len(got) != 2 || !got[0] || got[1] { + t.Errorf("requests (authed?) = %v, want [true false] — one rejected, one retried header-less", got) + } + if !TokenRejected() { + t.Error("TokenRejected() = false after a 401 to an authorized request") + } + // The retry's headers are the honest window and must be what the gauge reads. + if r := Rate(); r.Limit != 60 { + t.Errorf("Rate().Limit = %d, want 60 accounted from the anonymous retry", r.Limit) + } +} + +// TestDoGHSkipsTokenAfterRejection pins that the rejection is remembered: later +// requests go out header-less on the FIRST attempt. Without it every request in +// the session would pay a doomed round trip. +func TestDoGHSkipsTokenAfterRejection(t *testing.T) { + dir := t.TempDir() + resetTokenState(t, dir) + t.Setenv("GITHUB_TOKEN", "") + resetRate(t) + if err := SetToken("ghp_expired"); err != nil { + t.Fatal(err) + } + + srv := newAuthProbeServer(t, `{"tag_name":"v1.0.0"}`) + + for i := range 3 { + req, err := http.NewRequest("GET", srv.URL+"/repos/cli/cli/releases/latest", nil) + if err != nil { + t.Fatal(err) + } + resp, err := doGH(req) + if err != nil { + t.Fatalf("doGH #%d: %v", i, err) + } + _ = resp.Body.Close() + } + + // First call: rejected + retried. The two after it: one request each. + want := []bool{true, false, false, false} + got := srv.requests() + if len(got) != len(want) { + t.Fatalf("made %d requests %v, want %d %v", len(got), got, len(want), want) + } + for i := range want { + if got[i] != want[i] { + t.Fatalf("requests (authed?) = %v, want %v", got, want) + } + } +} + +// TestDoGHDoesNotRetryATokenlessRequest verifies the retry is gated on the +// request having carried credentials. A 401 to an anonymous request is a +// defensive case (a proxy, an enterprise host) and repeating it verbatim would +// double every request for nothing. +func TestDoGHDoesNotRetryATokenlessRequest(t *testing.T) { + dir := t.TempDir() + resetTokenState(t, dir) + t.Setenv("GITHUB_TOKEN", "") + resetRate(t) + + var requests int + var mu sync.Mutex + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + mu.Lock() + requests++ + mu.Unlock() + w.WriteHeader(http.StatusUnauthorized) + })) + defer srv.Close() + + req, err := http.NewRequest("GET", srv.URL+"/repos/cli/cli/releases/latest", nil) + if err != nil { + t.Fatal(err) + } + resp, err := doGH(req) + if err != nil { + t.Fatalf("doGH: %v", err) + } + defer func() { _ = resp.Body.Close() }() + + mu.Lock() + n := requests + mu.Unlock() + if n != 1 { + t.Errorf("made %d requests, want 1 — nothing to retry without", n) + } + if err := classifyStatus(resp); !errors.Is(err, ErrTokenInvalid) { + t.Errorf("classifyStatus = %v, want ErrTokenInvalid", err) + } + if TokenRejected() { + t.Error("TokenRejected() = true after a 401 to a request that carried no token") + } +} + +// TestDoGHKeepsTheTokenWhenAnonymousAlso401s pins what the 401 is evidence +// ABOUT. Dropping the header has to change the answer for the credential to be +// the thing at fault; a host that refuses an anonymous request too — a proxy, an +// enterprise instance — is refusing the resource. Marking the token there would +// strip Authorization for the rest of the session and put "rejected (HTTP 401)" +// in the [a] overlay beside a credential that is perfectly good. +func TestDoGHKeepsTheTokenWhenAnonymousAlso401s(t *testing.T) { + dir := t.TempDir() + resetTokenState(t, dir) + t.Setenv("GITHUB_TOKEN", "") + resetRate(t) + if err := SetToken("ghp_perfectly_fine"); err != nil { + t.Fatal(err) + } + + var requests int + var mu sync.Mutex + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + mu.Lock() + requests++ + mu.Unlock() + w.WriteHeader(http.StatusUnauthorized) + })) + defer srv.Close() + + req, err := http.NewRequest("GET", srv.URL+"/repos/cli/cli/releases/latest", nil) + if err != nil { + t.Fatal(err) + } + resp, err := doGH(req) + if err != nil { + t.Fatalf("doGH: %v", err) + } + defer func() { _ = resp.Body.Close() }() + + mu.Lock() + n := requests + mu.Unlock() + if n != 2 { + t.Errorf("made %d requests, want 2 — the retry still has to happen to learn this", n) + } + if TokenRejected() { + t.Error("TokenRejected() = true, but dropping the header changed nothing — " + + "the host refuses the resource, not the credential") + } + if got := resolveToken(); got != "ghp_perfectly_fine" { + t.Errorf("resolveToken() = %q, want the token still sent on later requests", got) + } +} + +// TestFetchRateWithTokenNeverSeesTheRetry pins the doGH bypass as an invariant. +// Validation is the one caller that must observe a 401 rather than survive it: +// riding the shared path would answer an anonymous 200 and persist a token +// already known to be dead. +func TestFetchRateWithTokenNeverSeesTheRetry(t *testing.T) { + dir := t.TempDir() + resetTokenState(t, dir) + t.Setenv("GITHUB_TOKEN", "") + resetRate(t) + + srv := newAuthProbeServer(t, `{"resources":{"core":{"limit":60,"remaining":59}}}`) + origAPIBase := testAPIBase + testAPIBase = srv.URL + t.Cleanup(func() { testAPIBase = origAPIBase }) + + if _, err := FetchRateWithToken("ghp_candidate"); !errors.Is(err, ErrTokenInvalid) { + t.Fatalf("FetchRateWithToken = %v, want ErrTokenInvalid", err) + } + if got := srv.requests(); len(got) != 1 || !got[0] { + t.Errorf("requests (authed?) = %v, want exactly one authorized attempt", got) + } + // Validation must not arm the session-wide suppression either: the candidate + // was never the token in effect. + if TokenRejected() { + t.Error("TokenRejected() = true after validating an unrelated candidate") + } +} + // TestFetchRateParsesCore verifies FetchRate decodes resources.core from the // /rate_limit endpoint and updates the shared snapshot. func TestFetchRateParsesCore(t *testing.T) { @@ -1252,9 +1533,13 @@ func TestGetRepoDataTagsFallbackKeepsPreservedTuple(t *testing.T) { // TestRepoDataConclusive pins the flag the model settles on. It is the version // layer's answer to "is this tool done for the window", and it must be false on // exactly the passes that left the entry stale on purpose — otherwise the model -// stops re-dispatching for a tool whose card was never filled. Err cannot carry -// that: it holds ErrRateLimited or nil, so an offline start reaches the model -// as a nil error. +// stops re-dispatching for a tool whose card was never filled. +// +// Err cannot carry that, and still cannot now that every failure class reaches +// the caller named: the two answer different questions and disagree in both +// directions. A repo with no releases settles the tool with a nil Err; a +// rate-limited pass that served a stale card is a non-nil Err on a pass that +// settled nothing. func TestRepoDataConclusive(t *testing.T) { t.Run("clean pass is conclusive", func(t *testing.T) { githubTestServer(t) @@ -1279,14 +1564,51 @@ func TestRepoDataConclusive(t *testing.T) { }) d := GetRepoData("github.com/owner/repo") - if d.Err != nil { - t.Fatalf("Err = %v — the premise of this test is that a 5xx reaches the caller as a nil error", d.Err) + if d.Err == nil { + t.Fatal("Err = nil after a 5xx — a transient failure now reaches the caller named") + } + if errors.Is(d.Err, ErrRateLimited) { + t.Fatalf("Err = %v, want the plain transient error, not a rate-limit claim", d.Err) } if d.Conclusive { t.Error("Conclusive = true after a total failure — the entry was deliberately left stale") } }) + t.Run("no releases is conclusive with no error", func(t *testing.T) { + // The definitive negative: /releases/latest 404s, the repo itself answers. + // The tool is settled for the window and nothing failed, so the card must + // not carry an error for the most ordinary state there is. + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case strings.HasSuffix(r.URL.Path, "/releases/latest"), strings.HasSuffix(r.URL.Path, "/tags"): + w.WriteHeader(http.StatusNotFound) + case strings.HasSuffix(r.URL.Path, "/languages"): + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]int{"Go": 1000}) + default: + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{ + "description": "a repo that never cut a release", "stargazers_count": 7, + }) + } + })) + origAPIBase, origCacheDir := testAPIBase, testCacheDir + testAPIBase, testCacheDir = srv.URL, t.TempDir() + t.Cleanup(func() { + srv.Close() + testAPIBase, testCacheDir = origAPIBase, origCacheDir + }) + + d := GetRepoData("github.com/owner/repo") + if d.Err != nil { + t.Errorf("Err = %v, want nil — errNoReleases is a negative, not a failure", d.Err) + } + if !d.Conclusive { + t.Error("Conclusive = false — a repo with no releases is settled for the window") + } + }) + t.Run("partial failure is not conclusive", func(t *testing.T) { // The release endpoint answers, repo info does not: getRepoData leaves // CheckedAt stale so the next pass refills About/stars/maintenance. @@ -1318,6 +1640,88 @@ func TestRepoDataConclusive(t *testing.T) { }) } +// TestRepoDataErrPickOrder pins which of the two core fetches' errors reaches +// the caller. ErrRateLimited outranks a transient failure whichever endpoint it +// came from, because it is the only class with an answer the user can act on; +// the rest are indistinguishable to the UI and it does not matter which one is +// picked, only that one is. Driven end to end through getRepoData rather than +// against pickFetchErr directly — the wiring is what regressed before. +func TestRepoDataErrPickOrder(t *testing.T) { + // status returns a server answering each endpoint with the given code, 200 + // meaning "answer normally". + serve := func(t *testing.T, relCode, infoCode int) { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + write := func(code int) bool { + if code == http.StatusOK { + return false + } + if code == http.StatusForbidden { + w.Header().Set("X-RateLimit-Remaining", "0") + } + w.WriteHeader(code) + return true + } + switch { + case strings.HasSuffix(r.URL.Path, "/languages"): + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]int{"Go": 1000}) + case strings.HasSuffix(r.URL.Path, "/releases/latest"): + if write(relCode) { + return + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]string{"tag_name": "v1.0.0"}) + default: + if write(infoCode) { + return + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{"description": "x", "stargazers_count": 1}) + } + })) + origAPIBase, origCacheDir := testAPIBase, testCacheDir + testAPIBase, testCacheDir = srv.URL, t.TempDir() + t.Cleanup(func() { + srv.Close() + testAPIBase, testCacheDir = origAPIBase, origCacheDir + }) + } + + tests := []struct { + name string + relCode, infoCode int + wantRateLimited bool + wantErr bool + }{ + {"release rate-limited outranks a repo-info 5xx", http.StatusForbidden, http.StatusInternalServerError, true, true}, + {"repo-info rate-limited outranks a release 5xx", http.StatusInternalServerError, http.StatusForbidden, true, true}, + {"both rate-limited", http.StatusForbidden, http.StatusForbidden, true, true}, + {"both transient", http.StatusInternalServerError, http.StatusInternalServerError, false, true}, + {"release fails alone", http.StatusInternalServerError, http.StatusOK, false, true}, + {"repo info fails alone", http.StatusOK, http.StatusInternalServerError, false, true}, + {"clean pass carries no error", http.StatusOK, http.StatusOK, false, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + serve(t, tt.relCode, tt.infoCode) + d := GetRepoData("github.com/owner/repo") + switch { + case !tt.wantErr: + if d.Err != nil { + t.Fatalf("Err = %v, want nil", d.Err) + } + case d.Err == nil: + t.Fatal("Err = nil, want a classified failure") + case tt.wantRateLimited && !errors.Is(d.Err, ErrRateLimited): + t.Fatalf("Err = %v, want ErrRateLimited", d.Err) + case !tt.wantRateLimited && errors.Is(d.Err, ErrRateLimited): + t.Fatalf("Err = %v, want a transient error, not a rate-limit claim", d.Err) + } + }) + } +} + // TestGetRepoDataTagsFallbackSkippedWhenRateLimited pins the placement of the // fallback call: it sits AFTER the total-failure early return, so a pass that // fetched nothing spends no request on tags whose result it would discard. The diff --git a/internal/version/logx_test.go b/internal/version/logx_test.go index fcd67b8..1b860d4 100644 --- a/internal/version/logx_test.go +++ b/internal/version/logx_test.go @@ -91,6 +91,42 @@ func TestClassifyStatusGenericForbiddenLogs(t *testing.T) { } } +// TestClassifyStatusUnauthorizedLogs verifies a 401 writes a line naming the +// code and the path — it is neither conclusive like a 404 nor normal, and it is +// the one status the user can fix — while a 404 stays silent, so a stale repo +// ref does not re-create a session log on every launch. +func TestClassifyStatusUnauthorizedLogs(t *testing.T) { + logDir := t.TempDir() + restore := logx.SetDirForTesting(logDir) + defer restore() + + err := classifyStatus(respWith(t, http.StatusUnauthorized, "4999", "/repos/cli/cli")) + if !errors.Is(err, ErrTokenInvalid) { + t.Fatalf("expected ErrTokenInvalid, got %v", err) + } + out := logx.ReadAllForTesting(logDir) + if !strings.Contains(out, "http=401") || !strings.Contains(out, "/repos/cli/cli") { + t.Errorf("log missing code/path, got:\n%s", out) + } + if strings.Contains(out, "rate limited") { + t.Errorf("a 401 must not be labelled rate limited, got:\n%s", out) + } +} + +func TestClassifyStatusNotFoundStaysSilent(t *testing.T) { + logDir := t.TempDir() + restore := logx.SetDirForTesting(logDir) + defer restore() + + err := classifyStatus(respWith(t, http.StatusNotFound, "4999", "/repos/cli/cli")) + if err == nil || errors.Is(err, ErrTokenInvalid) || errors.Is(err, ErrRateLimited) { + t.Fatalf("classifyStatus(404) = %v, want a generic error", err) + } + if out := logx.ReadAllForTesting(logDir); out != "" { + t.Errorf("a 404 must leave no log line, got:\n%s", out) + } +} + func TestDoGHNeverLogsToken(t *testing.T) { // Clear env precedence and route the config token through the seam. t.Setenv("GITHUB_TOKEN", "") diff --git a/internal/version/token.go b/internal/version/token.go index 82a0139..3a82162 100644 --- a/internal/version/token.go +++ b/internal/version/token.go @@ -7,15 +7,26 @@ import ( "sync" "github.com/stanlyzoolo/keepkit/internal/configdir" + "github.com/stanlyzoolo/keepkit/internal/logx" ) // testTokenDir overrides the token file directory in tests. var testTokenDir string // token state: value from the config file or TUI entry (never the env token). +// +// rejectedToken holds the credential GitHub answered 401 to, stored as the +// value rather than as a bool. That is what makes the state need no lifecycle: +// a newly entered token simply differs from it and resolves again, a cleared +// token is empty and compares against the "" guard, and a bad GITHUB_TOKEN — +// which keepkit cannot unset — is suppressed by the same comparison for as long +// as it is the effective token. A bool would have to be cleared from SetToken, +// ClearToken and the env path, and one missed site is a session that keeps +// sending a token it knows is dead or stops sending a good one. var ( tokenMu sync.RWMutex tokenMem string + rejectedToken string loadTokenOnce sync.Once ) @@ -48,9 +59,12 @@ func loadTokenFromFile() { tokenMu.Unlock() } -// resolveToken returns the effective GitHub token: the GITHUB_TOKEN env var -// takes precedence, otherwise the config-file token (lazily loaded once). -func resolveToken() string { +// effectiveToken returns the configured GitHub token: the GITHUB_TOKEN env var +// takes precedence, otherwise the config-file token (lazily loaded once). This +// is the raw core — it answers what the user configured, regardless of whether +// GitHub has since refused it, which is exactly what the [a] overlay has to +// print. Only resolveToken applies the suppression. +func effectiveToken() string { if env := os.Getenv("GITHUB_TOKEN"); env != "" { return env } @@ -60,6 +74,84 @@ func resolveToken() string { return tokenMem } +// resolveToken returns the token to send on a request, or "" when the effective +// one has been rejected. doGH is its only caller, and deliberately so: the +// suppression is about what we put on the wire, not about what the user +// configured. An accessor that hid a rejected token from the UI would blank the +// source and the mask in precisely the state the overlay exists to explain. +func resolveToken() string { + tok := effectiveToken() + if tok == "" { + return "" + } + tokenMu.RLock() + defer tokenMu.RUnlock() + if tok == rejectedToken { + return "" + } + return tok +} + +// rejectToken records the credential GitHub answered 401 to, so every later +// request goes out unauthenticated instead of repeating a call that cannot +// succeed. The token file is deliberately left alone: a rejection means the +// credential was refused, not that keepkit may destroy the user's data. +// +// The log line is written on the transition only. A cold start fires one pass +// per tracked tool in parallel, so a line per rejection would put a few dozen +// identical entries in a session log whose whole value is that a line means +// something happened. +func rejectToken(tok string) { + if tok == "" { + return + } + tokenMu.Lock() + first := rejectedToken != tok + rejectedToken = tok + tokenMu.Unlock() + if first { + logx.Errorf("version.rejectToken: github rejected the %s token (HTTP 401); "+ + "requests continue unauthenticated for this session", TokenSource()) + } +} + +// TokenRejected reports that the token currently in effect is the one GitHub +// refused. The rejectedToken != "" guard is load-bearing: without it a user with +// no token at all compares equal to the empty rejected value and gets the whole +// degraded UI for a state that is not degraded — unauthenticated by choice is +// not the same as unauthenticated by failure. +func TokenRejected() bool { + // effectiveToken first, so the two reads are never nested and the lock order + // is trivially one-deep. Both are cheap enough to sit on a render path, which + // is where this is read from — like Token() and TokenSource() beside it. + tok := effectiveToken() + tokenMu.RLock() + defer tokenMu.RUnlock() + return rejectedToken != "" && tok == rejectedToken +} + +// SetTokenRejectedForTesting arms or disarms the rejected state for the token +// currently in effect and returns a restore func, following the same shape as +// the config-dir seams. It exists because rejectToken is unexported and reached +// only through a real 401, so internal/model — which renders the degraded state +// and cannot reach the network in tests — has no other way to produce it. +func SetTokenRejectedForTesting(rejected bool) (restore func()) { + tok := effectiveToken() + tokenMu.Lock() + prev := rejectedToken + if rejected { + rejectedToken = tok + } else { + rejectedToken = "" + } + tokenMu.Unlock() + return func() { + tokenMu.Lock() + rejectedToken = prev + tokenMu.Unlock() + } +} + // SetToken stores the token in memory and persists it to a 0600 file. func SetToken(t string) error { path, err := tokenFilePath() @@ -97,14 +189,18 @@ func ClearToken() error { return nil } -// Token returns the effective GitHub token (env precedence, else config file), -// or "" when none is set. Used by the UI to render a masked preview. +// Token returns the configured GitHub token (env precedence, else config file), +// or "" when none is set. Used by the UI to render a masked preview, so it reads +// the raw core: a rejected token still has to show its source and its mask — +// that line is how the user recognises which credential to replace. func Token() string { - return resolveToken() + return effectiveToken() } -// TokenSource reports where the effective token comes from: "env", "config", -// or "none". +// TokenSource reports where the configured token comes from: "env", "config", +// or "none". Like Token it reads the raw state and never the suppression — a +// rejected token still came from somewhere, and naming that place is half of +// what the overlay tells the user to go fix. func TokenSource() string { if env := os.Getenv("GITHUB_TOKEN"); env != "" { return "env" diff --git a/internal/version/token_test.go b/internal/version/token_test.go index d20ed3c..ea0ccf5 100644 --- a/internal/version/token_test.go +++ b/internal/version/token_test.go @@ -3,24 +3,34 @@ package version import ( "os" "path/filepath" + "strings" "sync" "testing" + + "github.com/stanlyzoolo/keepkit/internal/logx" ) // resetTokenState clears the in-memory token and the sync.Once so each test // starts from a clean slate and re-reads the (test-overridden) token file. +// +// rejectedToken is cleared on both ends, and that is not symmetry for its own +// sake: Go runs a package's tests in one process, so a test that left a +// rejection standing would strip Authorization from every later test in the +// binary — deterministic contamination, not a flake. func resetTokenState(t *testing.T, dir string) { t.Helper() origDir := testTokenDir testTokenDir = dir tokenMu.Lock() tokenMem = "" + rejectedToken = "" tokenMu.Unlock() loadTokenOnce = sync.Once{} t.Cleanup(func() { testTokenDir = origDir tokenMu.Lock() tokenMem = "" + rejectedToken = "" tokenMu.Unlock() loadTokenOnce = sync.Once{} }) @@ -146,3 +156,176 @@ func TestClearTokenNoFile(t *testing.T) { t.Errorf("ClearToken on missing file should be nil, got %v", err) } } + +// TestRejectTokenSuppressesOnlyTheRequestPath pins the split the [a] overlay +// depends on: the suppression reaches what goes on the wire and nothing else. A +// rejected token must keep its source and its value, or the overlay loses the +// mask the user identifies the dead credential by — in exactly the state the +// overlay exists to describe. +func TestRejectTokenSuppressesOnlyTheRequestPath(t *testing.T) { + dir := t.TempDir() + resetTokenState(t, dir) + t.Setenv("GITHUB_TOKEN", "") + + if err := SetToken("ghp_deadbeef"); err != nil { + t.Fatal(err) + } + rejectToken("ghp_deadbeef") + + if got := resolveToken(); got != "" { + t.Errorf("resolveToken() = %q, want empty — a rejected token must not go on the wire", got) + } + if got := Token(); got != "ghp_deadbeef" { + t.Errorf("Token() = %q, want the value kept for the overlay's mask", got) + } + if got := TokenSource(); got != "config" { + t.Errorf("TokenSource() = %q, want config", got) + } + if !TokenRejected() { + t.Error("TokenRejected() = false after rejecting the token in effect") + } +} + +// TestRejectTokenNewTokenResolvesAgain verifies the value-not-bool choice pays +// off: entering a different token needs no clearing code anywhere, it simply +// stops matching the rejected value. +func TestRejectTokenNewTokenResolvesAgain(t *testing.T) { + dir := t.TempDir() + resetTokenState(t, dir) + t.Setenv("GITHUB_TOKEN", "") + + if err := SetToken("ghp_dead"); err != nil { + t.Fatal(err) + } + rejectToken("ghp_dead") + if err := SetToken("ghp_fresh"); err != nil { + t.Fatal(err) + } + + if got := resolveToken(); got != "ghp_fresh" { + t.Errorf("resolveToken() = %q, want ghp_fresh — a new token is not the rejected one", got) + } + if TokenRejected() { + t.Error("TokenRejected() = true after a different token was stored") + } +} + +// TestRejectTokenClearLeavesNothingResolvable covers the other exit: removing +// the token leaves nothing to send and nothing to call degraded, since an absent +// token is not a refused one. +func TestRejectTokenClearLeavesNothingResolvable(t *testing.T) { + dir := t.TempDir() + resetTokenState(t, dir) + t.Setenv("GITHUB_TOKEN", "") + + if err := SetToken("ghp_dead"); err != nil { + t.Fatal(err) + } + rejectToken("ghp_dead") + if err := ClearToken(); err != nil { + t.Fatal(err) + } + + if got := resolveToken(); got != "" { + t.Errorf("resolveToken() = %q, want empty after ClearToken", got) + } + if TokenRejected() { + t.Error("TokenRejected() = true with no token configured") + } +} + +// TestTokenRejectedWithNoTokenConfigured is the degenerate case the +// rejectedToken != "" guard exists for: without it "" == "" and a user who +// never set a token would be shown the whole degraded UI for a state that is +// not degraded at all. +func TestTokenRejectedWithNoTokenConfigured(t *testing.T) { + dir := t.TempDir() + resetTokenState(t, dir) + t.Setenv("GITHUB_TOKEN", "") + + if TokenRejected() { + t.Error("TokenRejected() = true with no token ever configured") + } + // rejectToken("") is a no-op, so an empty rejection cannot arm the state + // either — the guard is not the only thing holding this line. + rejectToken("") + if TokenRejected() { + t.Error("TokenRejected() = true after rejectToken(\"\")") + } +} + +// TestTokenRejectedEnvToken covers the case keepkit cannot fix by writing a +// file: a bad GITHUB_TOKEN cannot be unset from the environment, and the same +// value comparison is what suppresses it. +func TestTokenRejectedEnvToken(t *testing.T) { + dir := t.TempDir() + resetTokenState(t, dir) + t.Setenv("GITHUB_TOKEN", "ghp_env_dead") + + rejectToken("ghp_env_dead") + + if got := resolveToken(); got != "" { + t.Errorf("resolveToken() = %q, want empty for a rejected env token", got) + } + if got := TokenSource(); got != "env" { + t.Errorf("TokenSource() = %q, want env", got) + } + if !TokenRejected() { + t.Error("TokenRejected() = false for a rejected env token") + } +} + +// TestRejectTokenLogsOnceForTheSameValue pins the transition-only log line. A +// cold start rejects in parallel once per tracked tool, so a line per call would +// bury the session log under identical entries and destroy the "a log file means +// something went wrong" signal. +func TestRejectTokenLogsOnceForTheSameValue(t *testing.T) { + dir := t.TempDir() + resetTokenState(t, dir) + t.Setenv("GITHUB_TOKEN", "") + + logDir := t.TempDir() + restore := logx.SetDirForTesting(logDir) + defer restore() + + rejectToken("ghp_dead") + rejectToken("ghp_dead") + rejectToken("ghp_dead") + + out := logx.ReadAllForTesting(logDir) + if n := strings.Count(out, "version.rejectToken"); n != 1 { + t.Errorf("rejectToken logged %d times for one value, want 1:\n%s", n, out) + } + if strings.Contains(out, "ghp_dead") { + t.Errorf("the rejected token leaked into the log:\n%s", out) + } +} + +// TestRejectedTokenReenteredStaysSuppressed covers the loop a frustrated user +// closes: paste the same expired token again. Validation goes through +// FetchRateWithToken, which bypasses doGH precisely so it observes the 401 +// instead of surviving it, so nothing is persisted — and the value comparison +// keeps the session unauthenticated either way. +func TestRejectedTokenReenteredStaysSuppressed(t *testing.T) { + dir := t.TempDir() + resetTokenState(t, dir) + t.Setenv("GITHUB_TOKEN", "") + + const dead = "ghp_expired_value" + if err := SetToken(dead); err != nil { + t.Fatal(err) + } + rejectToken(dead) + + // Re-entering the same value: SetToken is what the accept path would call, + // and even reached directly it cannot un-reject the credential. + if err := SetToken(dead); err != nil { + t.Fatal(err) + } + if got := resolveToken(); got != "" { + t.Errorf("resolveToken() = %q, want empty — the same dead value is still dead", got) + } + if !TokenRejected() { + t.Error("TokenRejected() = false after re-entering the rejected value") + } +}