Survive an expired GitHub token instead of going blank - #60
Merged
Conversation
An expired GitHub token 401s every request and keepkit shows nothing: the error is dropped at four layers, so a stale card renders as healthy and a manual refresh writes nothing. The plan names every failure class, retries once without the rejected token so a blackout becomes a 60/h session, and gives the three states a surface each. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A token that goes bad after it was stored turns keepkit into a silent liar. The reported symptom was a card showing installed v2.1.221 against latest v2.1.220 with [r] changing nothing. The token in ~/.config/keepkit/token had expired, so every request answered 401 Bad credentials — one session log holds 33 × /releases/latest http=401 and 32 × /repos/* http=401, every request it made, all 34 tracked tools, and none of it reached the screen. An expired token is strictly worse than no token: the same URLs answer 200 with no Authorization header at all. So doGH now retries. On a 401 to a request that carried a token it drains the body, marks the credential rejected and reissues the same request once without the header; every later request skips straight to the anonymous form. A blackout becomes a working 60 req/h session. The retry's rate headers go through the same accounting, so the gauge moves honestly from 5000 to 60. The rejection is stored as a value (rejectedToken string), not a bool, which is what makes it need no lifecycle: a newly entered token differs from it and resolves immediately, a cleared one 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 splits accordingly: effectiveToken() is the raw core behind Token()/TokenSource()/TokenRejected(), and only resolveToken() — whose sole caller is doGH — applies the suppression. The overlay has to keep printing the source and the mask; blanking them in the very 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 we may destroy the user's data. rejectToken logs on the transition only — a cold start rejects once per tracked tool in parallel. FetchRateWithToken keeps bypassing doGH, and that is now a pinned invariant rather than an incidental detail: validation is the one caller that must observe a 401 instead of surviving it, or the retry would answer an anonymous 200 and a dead token would be persisted. Two swallowing layers under that: - classifyStatus gives 401 a name (ErrTokenInvalid) instead of an anonymous "HTTP %d". Once the retry lands this is a classification rather than a hot path — doGH consumes the 401 and GitHub does not 401 a credential-less request — so what is left is the log line and a proxy that 401s anonymously. - getRepoData propagated only rate limiting. `d.Err = rlErr` dropped every other error on the floor, which is the line the 401 died at: the model never learned anything had gone wrong and a stale card rendered as perfectly healthy. It now carries pickFetchErr(relErr, infoErr) — ErrRateLimited outranks transient because it is the one class with an answer the user can act on, errNoReleases never participates because a repo without releases is a conclusive negative, and among transient failures the choice is arbitrary since the UI treats them identically. Conclusive is untouched and still not derivable from Err, now in both directions: a repo with no releases settles a tool with a nil error, and a rate-limited pass that served a stale card carries one while settling nothing. resetTokenState learns rejectedToken in both its setup and its cleanup. Go runs a package's tests in one process, so a test leaving a rejection standing would strip Authorization from every later test in the binary — deterministic contamination, not a flake. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Nothing on screen said the data had stopped being refreshed. With a dead token the only trace was a log file the user has to know to open, and [r] answered 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. Three surfaces, one per lifetime of fact. The status-bar gauge marks the session: api✕, the ✕ in Danger, in every form. 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. U+2715 measures one cell under both runewidth conditions (U+00D7 would be two under RUNEWIDTH_EASTASIAN=1), which the bar's right-edge arithmetic requires. That one column has a consequence the plan did not anticipate: the marker does not merely shed with the gauge, it makes the gauge shed earlier. Measured against the six-hint bar with a 60-limit snapshot, the gauge survived down to ≤78 columns before and only to 81 after — so at the 80x24 baseline arming the degraded state removed the announcement entirely, and the session that most needed announcing was the one showing nothing. Hence renderRateMarker(), a third and narrowest form: the mark with no numbers, tried last. What survives follows the bar's own shed rule — the numbers are a measurement the [a] overlay repeats, the ✕ is the only sign anywhere that requests stopped carrying the token. It returns "" without a rejection, so a healthy bar gains no form it did not have. The [a] overlay explains it, and is the surface that always has the answer since the gauge is droppable and invisible before the first rate snapshot: the token line becomes "token config (ghp_••••••••3f2a) — rejected (HTTP 401)" in Danger with "requests run unauthenticated" under it. The mask is load-bearing — it is how the user tells which credential to replace — and it survives only because Token() reads the raw core rather than the suppressed resolveToken. The nudge gets its own wording for this state: "add a github token" reads as advice to someone who has already done it, so a rejected token is told to replace one. [r] now answers whenever the version layer calls the pass inconclusive: ErrRateLimited → "refresh failed: rate limited — press [a]", everything else → "refresh failed: network error". A conclusive pass stays silent because the repainted card is the answer. Two tiers and 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. 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 report a failure for a gesture nobody made. Two more, without which the above reports a state the app then handles wrongly: - The remoteMsg data path is gated on hasData || err == nil. The predicate is about data, not about the error class; now that every failure reaches the model named, gating the stale-data branch on ErrRateLimited alone would blank a card the pass had carried up from the cache — a tool going empty for a failure it survived. - An accepted token recovers the whole tracker. tokenValidatedMsg clears m.tokenRejected explicitly (it returns straight to modeAPIStatus, so the overlay would otherwise redraw "rejected" against the token that just validated), clears m.remoteAnswered wholesale and fans out fetchRemoteCmd for every tool with a repo — Init's predicate, deliberately not needsRemote, which answers false as soon as a card exists with a non-empty Latest: exactly the tools that rendered stale-but-present data through the degraded window. Cost is a goroutine and a cache.json read per tool, not API quota. The rejection rides on remoteMsg/rateMsg, snapshotted in the command goroutine like rate, rather than being read from the version global in View(): a package global cannot be set in a model test without the network, and a frame must not depend on when a goroutine happened to write. Every new assertion was mutation-checked — removing the label marker, dropping the marker-only form, restyling the mark Dim, removing the rejected overlay line and its nudge, removing [r]'s answer, collapsing its two tiers, and swapping the recovery predicate for needsRemote each turn the new tests red. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CLAUDE.md, ARCHITECTURE.md and README.md all carried the claim the change falsifies — "Err holds ErrRateLimited or nil, so an offline start reaches the model as a nil error" — in three separate places. Fixed there, plus the doGH retry, the value-not-bool rejection state, the effectiveToken/resolveToken split, the FetchRateWithToken bypass as a pinned invariant, the api✕ gauge and its third form, the overlay's rejected line, [r]'s answer and the token-accepted fan-out. README gets the user-facing version, including the honest caveat that on a cold cache the recovery is partial: a large list needs more than the anonymous 60 req/h. Both files also gain a note that version.rejectedToken is process-global state a test can leak, with resetTokenState as its seam — the same hazard the config seams already document. Verified unchanged while checking: 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 against README Stack, the storage-path table, every timeout and limit, and all three docs/design files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| - | - | Generic High Entropy Secret | d73fd20 | internal/model/render_test.go | View secret |
| 35771884 | Triggered | Generic High Entropy Secret | 177ab84 | internal/model/render_test.go | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
A /review:pr pass with an independent analysis agent and mutation checks found five real defects in the work this branch delivered. The local suite was green through all of them, which is itself part of the finding. 1. The token-accept fan-out dispatched the selected tool twice. The argument for preferring Init's predicate over needsRemote covered only one of the two degraded shapes: 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 for the same entry. The loop now skips whatever the backfill already queued, decided against the same cleared marker state it reads. The existing test missed it by pre-populating cards, which is the other shape. 2. The comment on that fan-out, and the matching CLAUDE.md sentence, claimed it costs "not API quota" — while README.md described the opposite two paragraphs later. The refetch does spend quota: an inconclusive pass never stamps CheckedAt, so the degraded window left every entry stale and every tool makes a real three-request pass. That is the point of it, and the new token is what pays for it. 3. rejectToken fired before the retry's verdict was known. A 401 is evidence about the credential only when dropping the header changes the answer; a host that 401s an anonymous request too — the proxy/enterprise case ErrTokenInvalid's own doc comment names — is refusing the resource. Marking the token there stripped Authorization for the rest of the session and put "rejected (HTTP 401)" beside a credential that was fine. It now runs after the retry. 4. [r]'s !msg.conclusive predicate was broader than what it meant to report. It said "refresh failed: network error" 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. msg.err != nil is the right predicate now that every real failure carries a name. The table's "inconclusive pass carrying no error" row had enshrined the wrong message as correct; it is now two rows asserting silence. 5. Mirroring the rejection into a Model field was the wrong design, not just a buggy one. remoteCmd/fetchRateCmd snapshot version.TokenRejected() inside the goroutine after the fetch, so a reply can be in flight across the very keystroke that fixes the credential: press [r] in the [a] overlay, replace the token while FetchRate is still blocked, and the reply carries a value observed under the old token — re-arming the flag after tokenValidatedMsg cleared it, so the overlay redrew "rejected (HTTP 401)" beside the mask of the token that had just validated. The cache bought nothing: renderAPIStatus already read version.TokenSource() and version.Token() straight from the package on the adjacent lines, so the three facts on that line could disagree. The field, the two message fields, the two handler writes and the explicit clear are gone — both renderers ask version.TokenRejected() at paint time. Arming it in a test is SetTokenRejectedForTesting, the same Set…ForTesting(x) (restore func()) shape as the config-dir seams, because internal/model can reach neither the unexported rejectToken nor the network. Two test-quality findings from the same pass. The glyph-width assertion measured through lipgloss.Width, which follows the ambient RUNEWIDTH_EASTASIAN, so swapping in the two-cell U+00D7 that its own comment names as the hazard stayed green under CI's plain `go test`; it now checks both runewidth conditions like TestLanguageBandGlyphWidth and TestMarkerGlyphWidth. And a fixture token tripped GitGuardian — renamed to an obviously synthetic value, which is what every other fake token in the suite already looks like. TokenRejected also folds its two reads into one lock depth rather than nesting them, since it is now on a render path. Every fix carries a test that fails without it: the cold-cache fan-out shape, a retry that also 401s, a refused ref and a partial pass staying silent, and that the surfaces keep no cached copy. All five verified by mutation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The degradation work this branch delivers made the [a] overlay tell a user with an expired GITHUB_TOKEN to press [e] and replace it. That advice cannot work, and following it made the session worse than ignoring it. [e] runs SetToken, which writes ~/.config/keepkit/token. effectiveToken reads that file only when GITHUB_TOKEN is empty, so under env precedence the accepted credential is saved and still is not what goes on the wire — FetchRateWithToken sends an explicit header, so its 200 proves the token works and says nothing about whether the session will use it. TokenRejected stayed true, the overlay kept printing "rejected (HTTP 401)" against the env token, and resolveToken kept suppressing it. The expensive half was the recovery fan-out. tokenValidatedMsg clears remoteAnswered and dispatches fetchRemoteCmd for every tool with a repo — three requests each, on a cache the degraded window left entirely stale. Dispatched while the env token is still the effective one, those go out unauthenticated: a 34-tool tracker spends ~102 requests against a 60/h ceiling. The one gesture the overlay offers as the way out of a degraded session burnt the rest of the hour. Two changes, and both are needed — [e] is unconditional, so wording alone leaves the burst reachable, and a silent guard leaves the overlay still recommending it. The nudge gains a first arm: a rejected env token reads "GITHUB_TOKEN was refused — replace it in your shell" and offers no key, because the variable belongs to the shell that launched keepkit and no key here can reach it. The arm must stay ahead of the config one or that wording swallows it. [d] two blocks down has gated on TokenSource() == "config" for this reason all along; [e] never learned it. The handler returns early on TokenSource() == "env" after a successful save, reporting "saved — GITHUB_TOKEN still takes precedence" and stopping before the rate write, the README-negative drop, the remoteAnswered clear and the fan-out. Each is wrong there in its own way — msg.rate is the candidate's snapshot, so the gauge would claim a 5000 limit the session does not have — but the fan-out is the one that does damage. Failing to recover is survivable; making it worse is not. The predicate is TokenSource() == "env", 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 env overlay is 55 cells framed, narrower than both siblings, so the modal did not grow; read at 80 and 120 columns. Six mutations, all killed: dropping the env arm, giving it an [e] offer, ordering it after the config arm, dropping the guard, keeping the guard without the return, and keeping the return without the message. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
b4e86c7 changed [r]'s answer from !msg.conclusive to msg.err != nil and gave the reason in a comment above the branch: !conclusive is the broader thing, false also for a ref the version layer refused outright — a bare RepoData with a nil Err and no request made — so it reported a network failure that never happened, and it contradicted a card the user had just watched update on a partial pass. That commit updated ARCHITECTURE.md and README.md and left three places still describing the predicate it replaced. CLAUDE.md's Refresh bullet is the one worth fixing on its own: the file is loaded whole into every session as the record of what is invariant here, so a sentence that names the wrong predicate does not merely read oddly, it is what the next change gets designed against. It now states msg.err != nil and keeps the rejected alternative with its reason, which is the part that stops the broader predicate looking like a simplification worth making. README said "a refresh that settled nothing says so" — a refused ref and a partial-but-successful pass both settle nothing and both stay silent, so the user-facing sentence promised more than the app does. It says "failed" now. The doc comment on TestRefreshAnswersEveryPress still claimed the predicate is conclusive, directly above the two rows the same commit added to assert silence for exactly the cases conclusive would have spoken for. And refreshFailedStatus was introduced as naming "why an [r] settled nothing", which is the same slip one layer down. No behavior change; comments and prose only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
An expired GitHub token turned keepkit into a silent liar. Reported symptom: a card showing
installed v2.1.221againstlatest v2.1.220, and[r]changing nothing.The cause was not release resolution.
~/.config/keepkit/tokenhad expired, so every request answered401 Bad credentials— while the same URLs answer200with noAuthorizationheader at all. One session log holds 33 ×/releases/latest http=401and 32 ×/repos/* http=401— every request of the session, all 34 tracked tools — and none of it reached the screen.The error was swallowed at four layers. The core defect was
d.Err = rlErringetRepoData: only rate limiting was propagated, everything else dropped on the floor, so the model never learned a 401 happened and a stale card rendered as perfectly healthy.What changed
Degradation.
doGHretries. On a 401 to a request that carried a token it drains the body, marks the credential rejected and reissues the same request once without the header; later requests skip straight to the anonymous form. A blackout becomes a working 60 req/h session. The retry lives there and nowhere else — it is the only place that decides to send a token, and a startup pre-check cannot help sinceInitfires the rate seed and every repo pass in one batch.The rejection is a value (
rejectedToken string), not a bool, so it needs no lifecycle: a new token differs from it, a cleared one is empty, and a badGITHUB_TOKENthat keepkit cannot unset is suppressed by the same comparison.token.gosplits intoeffectiveToken()(raw core, feedsToken/TokenSource/TokenRejected) andresolveToken()(suppressing, only caller isdoGH). The token file is never deleted.FetchRateWithTokenkeeps bypassingdoGH— now a pinned invariant, since it is the one caller that must observe a 401 rather than survive it, or a dead token would be persisted.Taxonomy.
classifyStatusnames a 401ErrTokenInvalid;pickFetchErrcarries the more actionable of the two core fetches' errors out ofgetRepoData(ErrRateLimited> transient,errNoReleasesnever participates).Conclusiveis untouched and still not derivable fromErr, in both directions.Visibility. Three surfaces:
api✕[a]overlay[r]statusMsg[r]now answers whenever the pass was inconclusive —refresh failed: rate limited — press [a]orrefresh failed: network error; a conclusive pass stays silent because the repainted card is the answer. An accepted token clearsremoteAnsweredand refetches every tool with a repo (Init's predicate, notneedsRemote, which answers false for exactly the tools that rendered stale-but-present data).Two supporting fixes: the
remoteMsgdata path is gated onhasData || err == nil— on data, not on the error class, or naming every failure would blank cards on a 5xx; andtokenValidatedMsgclearsm.tokenRejectedexplicitly, since it returns straight tomodeAPIStatus.Deviation from the plan
The plan said the
✕"costs one column, sheds with the gauge". Measured, that was one step short: the marker makes the gauge shed earlier. With the six-hint bar and a 60-limit snapshot the gauge survived to ≤78 columns before and only to 81 after — so at the 80×24 baseline arming the degraded state removed the announcement entirely.Fixed with a third, narrowest gauge form (
renderRateMarker): the mark with no numbers, tried last. What 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.Testing
go build/go vet/go test -race ./.../golangci-lint run(0 issues), plus CI's cross-compile step. Every new assertion was mutation-checked per thetui-renderskill — 8 mutations, all killed, including removing the label marker, restyling itDim, revertingToken()to the suppressingresolveToken, and swapping the recovery predicate forneedsRemote.Known and out of scope
[r]— pre-existing, and the retry makes the new error class unreachable in practice.SelfLatesthas no force variant, so a self-check that failed while degraded has no banner until the next launch. It does not poison the cache.Not covered by tests
The manual verification against the live API — a deliberately invalid token with warm and cold caches, replacing it via
[a] → [e],GITHUB_TOKEN=<invalid>, and a run with no token — cannot be automated, since no test may hold a real credential.