Fix the internal/model review findings - #59
Merged
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ase shapes Second plan-review pass on the review-fixes plan: the tags fallback must not write a git tag over a deleted release's cached notes (the hybrid-card failure ReleaseMissing exists to prevent), the rate-limit trap case gets its exact shape, the Chroma repaint is stated dark-only, and the card assertion moves to the task whose test gate actually runs it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A repo that tags releases in git but never cuts a GitHub release showed no latest version at all, so keepkit could not tell an outdated install from an up-to-date one. On the definitive 404 from /releases/latest — and only after the total-failure return, so a rate-limited pass spends nothing — fetchLatestTag reads the first page of /tags and takes the semver maximum (that list is ordered by creation, so v1.9.0 routinely precedes v1.10.0). A tag is not a release, so the write is deliberately narrow: it fills Latest alone, sits beside applyReleaseOutcome rather than inside it, leaves ReleaseMissing true (the self-check must stay quiet about a version with no release page), and is skipped when the entry still carries a release tuple — a tag written over a deleted release's preserved notes would render a hybrid card that getChangelog's cached-body gate would then serve all window. A failed tags request has no vote in the conclusive decision, so it degrades to exactly the previous blank-Latest outcome instead of re-spending the card requests every launch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
openURLCmd started the opener and never waited on it, so each o, c or card-link click left a zombie for the rest of the session. Start plus a goroutine that Waits reaps the child without holding the command goroutine for the opener's whole lifetime — a shell-wrapper xdg-open can outlive the click by minutes — and keeps openURLMsg.err meaning "could not launch" rather than starting to report the opener's own exit code through the visible status path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
glamour sends a code fence through rules.Chroma whenever the color profile is not Ascii — every real session — and only falls back to the CodeBlock StyleBlock fields otherwise. The theme overrode the StyleBlock alone, so the plate showed up in NO_COLOR terminals and in this package's tests while a live session kept the stock charm palette on the terminal's own background. Both paths now carry it. The Chroma repaint is bounded to two entries: Background holds the plate and Text takes the same background because chroma's formatter emits one per token, so without it the plate is punched through wherever a token falls back to Text. Every other token entry is inherited unchanged, and the clone is a fresh pointer — writing through the cloned one would restyle glamour process-wide. Dark only, like every other CodeBlock override: the light palette deliberately stays stock. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Six defects found by a review of internal/model, plus one feature removal. Re-tracking a tool by its bare name wiped the note, tag, update_cmd, Added date and GitHub ref: UpsertMeta replaces the record wholesale and trackTool built a fresh one, behind an "already tracked" status that reads as a no-op. The merge now carries every user-authored field forward; the status reset to trying stays, since re-tracking is "I'm trying this again". The track and rename commits remapped the cursor through a raw m.meta index, while metaSelected is an index into filteredMeta() — with the update partition floating another tool to the top, the cursor landed on that tool. Both go through indexOfMeta now, like every other remap site. Release notes on the card lost what the author wrote as code: the HTML strip ate the argument out of `--output <path>`, emphasis fired across two adjacent spans, and prose carrying Vec<String> or <user@host> was cut as markup. mdInline masks inline code before any rule runs and strips HTML through the README preprocessor's allowlist. A fence also closes on its own marker only, so a ~~~ block wrapping ``` samples stays one block instead of ending at the inner fence. The changelog block sized itself one cell wider than the card, and since the code plate is padded to the block's full width it painted over the panel's right gutter. It uses cardWidth(), the card's single width definition. An in-flight "launching <name> in <terminal>…" status could be wiped by the expiry timer of a transient status set moments earlier — the direct assignment left statusSeq unbumped, so the stale timer's generation still matched. setStickyStatus bumps it without arming a timer of its own. needsRemote re-dispatched the network pass on every cursor visit for a repo that answers with no version at all; a session-scoped marker, written only when the pass returned no error, settles it. A rate-limited pass carrying a stale card stays retryable. Star counts read 46k rather than 46.0k, while a real fraction survives. Also removes the help and man page search (/ from [2]/[3], n/N, the match counter): the feature was not worth its surface. `/` is now the tool-list filter and nothing else, guarded on focusTools so a press elsewhere cannot fall through into it, and its row moved to the [1] group of the hotkeys overlay. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…lusive Review of #59 found that the remoteAnswered marker added earlier in the same branch reads a nil error as "the pass came back clean", which it is not. RepoData.Err carries ErrRateLimited or nil, so a total failure — offline, DNS, a 5xx on both endpoints — reaches the model with a nil error and used to mark the tool answered. needsRemote then stopped re-dispatching for the rest of the session, so a start without a network never filled a card until a manual refresh; the tokenValidatedMsg backfill goes through the same predicate and was suppressed too. A partial failure, where getRepoData deliberately leaves CheckedAt stale so the next pass refills the card, had the same outcome. RepoData now reports Conclusive — the entry is fresh, because this pass stamped CheckedAt or already found it stamped — and remoteMsg carries it to the marker. The model no longer guesses from an error that cannot answer the question, and the flag is false on exactly the passes that left the entry stale on purpose. Also from the same review: - rcFenceCloses guards an empty marker instead of indexing marker[0]. Both call sites keep an "inside a fence" flag in step with the marker, so this was latent, but the helper should not depend on that. - The tags-fallback tests are exact now: the tuple-collision case counts the tags request, so it can tell a gated write from a request that never happened, and a new case pins that a rate-limited pass spends nothing on the fallback. - markdownToLines' fence rows assert the mdCode kind, which mdDump collapses — the kind is what paints the card's plate, so a regression emitting fenced lines as body with the indent intact would have gone green. - TestTagDerivedLatestOnCard is renamed to what it actually pins: the card's rendering of a Latest with no release tuple. It never touched the fallback. 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.
Fixes the confirmed findings of the 2026-08-03 review of
internal/model/, per the plan indocs/plans/completed/20260803-model-review-fixes.md. Every fix landed test-first; each regression test fails onmain.Tracker correctness
UpsertMetareplaces a record wholesale andtrackToolbuilt a fresh one, so re-tracking by bare name dropped the note, tag,update_cmd,Addeddate and the GitHub ref — behind an "already tracked" status that reads as a no-op, and the nextSaveMetamade it permanent. The merge now carries every user-authored field forward; the reset totryingstays.m.metaindex whilemetaSelectedindexesfilteredMeta(); with the update partition active the two orders differ. Both useindexOfMetanow.Release notes and the card
`--output <path>`, emphasis fired across two adjacent spans, and prose carryingVec<String>or<user@host>was cut as markup.mdInlinemasks inline code before any rule runs (rcMaskSpans) and strips HTML through the README preprocessor'srcHTMLNamesallowlist.~~~block wrappingrcFenceCloses, the same helper the README pass uses.briefW-2, one cell wider than the card, and the code plate is padded to the block's full width. It usescardWidth().46kinstead of46.0k; a real fraction still survives (46.2k).Panel [3]
rules.Chromawhenever the color profile is notAscii, and through theCodeBlockStyleBlockfields otherwise — so the theme's plate only ever showed up underNO_COLORand in this package's TTY-less tests. Both paths carry it now, via a repaint bounded toChroma.BackgroundandChroma.Text(chroma emits a background per token, soTextneeds one too); every other token entry is inherited unchanged, and the clone is a fresh pointer so glamour is not restyled process-wide.Plumbing
launching <name> in <terminal>…was a direct assignment, so a transient status set within the laststatusMsgTTLhad a timer whose generation still matched and wiped it — hiding the only sign the adapter is busy for up tolaunchTimeout.setStickyStatusbumpsstatusSeqwithout arming a timer.o/c/link click.Start()plusgo cmd.Wait(), which does not hold the command goroutine for the opener's lifetime and keepsopenURLMsg.errmeaning "could not launch".needsRemotere-dispatched forever for a repo that answers with no version: an emptyLatestis itself the answer. A session-scoped marker written only onerr == nilsettles it; a rate-limited pass carrying a stale card stays retryable (the marker deliberately is notm.repoStatus, which such a pass does write).New: a latest version for tag-only repos
fetchLatestTagreads the first page of/tagson a definitive 404 from/releases/latestand takes the semver maximum — that list is ordered by creation, sov1.9.0routinely precedesv1.10.0. It costs one extra request per such repo per 24h window, spent only after the total-failure return so a rate-limited pass spends nothing.A tag is not a release, so the write is narrow:
Latestalone, besideapplyReleaseOutcomerather than inside it,ReleaseMissingleft true (the self-check stays quiet), skipped entirely when the entry still carries a release tuple — a tag over a deleted release's preserved notes would render a hybrid card thatgetChangelog's cached-body gate would serve all window — and with no vote in theconclusivedecision, so a failed tags request degrades to the previous blank-Latestoutcome instead of re-spending the card requests every launch.Removed: the help and man page search
/from[2]/[3],modeHelpSearch,n/Nand the match counter are gone — the feature was not worth its surface./is now the tool-list filter and nothing else, guarded onfocusToolsso a press elsewhere cannot fall through into the list search, and its row moved from theglobalgroup of the[?]overlay to[1] tools.Notes
CLAUDE.md,ARCHITECTURE.md,README.mdanddocs/design/readme-pipeline.md(including the stalerate limited — press Lhint and the retiredh/mkeys).go test -race ./..., golangci-lint v2 (0 issues)./indemo/update.tapeis the[1]list filter.