Production readiness: streaming console, correctness fixes, tests, docs - #1
Merged
Merged
Conversation
Every package was behind, and the console rebuild is about to be written against Spectre. Bumping first keeps a library surface change from being mistaken for a UI regression. Spectre.Console 0.49.1 -> 0.57.2 Microsoft.Extensions.DI 9.0.0 -> 10.0.10 System.Security.Crypto.Protect 9.0.0 -> 10.0.10 Markdig 1.2.0 -> 1.3.2 Microsoft.NET.Test.Sdk 17.11.1 -> 18.8.1 xunit / runner.visualstudio 2.9.2 -> 2.9.3 / 3.1.5 The Spectre 0.55 breaking changes (Style became a struct, Capabilities .IsTerminal removed, Table/Grid.Alignment removed) turned out not to touch any existing call site — the upgrade compiled clean. Move the publish flags out of README prose and into OpenKey.csproj. They only existed as documentation, so any `dotnet publish` that did not paste the exact command — including CI, once it exists — silently produced a different artifact than the one that got smoke-tested. A bare `dotnet publish -c Release -r win-x64` now yields the single-file exe. Convert System.Text.Json to source-generated contexts. Anonymous request bodies cannot be source-generated, so the OpenRouter chat request and the OAuth key exchange become named records; this also drops the `"temperature": null` that was serialized on every request. Enabling IsAotCompatible across all three projects is what surfaced these, and the tree is now warning-clean with the trim/AOT analyzers on — which retires the "Spectre reflection blocks AOT" claim in docs/06 as stale and shows Markdig is clean at our call sites too. Also: pin the SDK via global.json so CI cannot resolve a different one, and add win-arm64 alongside win-x64. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WGcarVYpt1mfjk6iFkcDpZ
Fourteen defects found by an interaction review, none of them marked in the
code — a grep for TODO/FIXME/NotImplemented returns nothing, so every gap
here was structural rather than known-and-deferred.
The two that mattered most were invisible from a desk:
HttpClient.Timeout was 60s, and that budget covers reading the response
body even under ResponseHeadersRead. A slow free model generating a long
reply would blow through it, get aborted, be misclassified as NetworkDown,
rotate — and die the same way on the next model. On a slow model the app
could never succeed. Timeout is now infinite and the provider applies
per-read deadlines instead, which is what actually distinguishes a slow
model from a dead connection.
Captive-portal Wi-Fi (hotels, airports) answers any request with a login
page and HTTP 200. JsonDocument.ParseAsync threw, nothing caught it, and
with no top-level handler the window closed on the same frame as the stack
trace. For a product distributed on a USB stick that is the likeliest
first-run experience in the wild.
Also fixed:
- One Ctrl+C poisoned the session. A single process-lifetime CTS was
cancelled forever, so every later turn was born cancelled. Now one CTS
per turn; Ctrl+C mid-reply cancels that reply, at the prompt it exits.
- A mid-reply rotation rendered the answer twice concatenated while
session.json stored it once, leaving screen and history permanently
disagreeing. ChatChunk now carries an attempt-restart signal.
- /reset that ended without a key dropped the user into a keyless REPL:
every message failed auth, the error said "run /reset", and /reset
landed back in the same place. Inescapable without killing the window.
- An empty free-model list was cached with a fresh 24h TTL, then crashed
on a bare InvalidOperationException every launch — unrecoverable
without deleting %APPDATA% by hand.
- Failed and cancelled turns left their user message in history to be
persisted on the next success.
- Streams ending with [DONE] and no prior finish_reason had their
complete reply discarded as malformed and retried elsewhere.
- Free-model detection string-matched "0"/"0.0"/"0.00", so any other
zero formatting silently marked a free model as paid.
- Multi-line paste submitted the first line and let the next prompt eat
the rest, so a pasted transcript containing /reset could open the wipe
confirm with the following line answering it.
- Every disk write was unguarded, so a full disk or read-only roaming
profile crashed after a reply was generated but before it was shown.
Now best-effort, except the key: failing to persist that silently
means re-authenticating every launch, so it still reports.
Contract changes, approved beforehand: ChatErrorKind gains InvalidRequest
(400/404/422 are unretryable — the request is at fault, not the model);
NetworkDown no longer rotates or counts against a model; ChatChunk gains
IsAttemptRestart; writes are guarded per docs/05, which already specified
this behaviour the code never implemented.
The RollingWindow test asserting NetworkDown was retryable encoded the old
rule and now encodes the new one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WGcarVYpt1mfjk6iFkcDpZ
The old console showed nothing at all until a reply was complete, printed raw ChatErrorKind enum names at users, and wrote its goodbye to a window that closes instantly on double-click — so nobody in the target audience had ever seen it. Colour and glyph literals were scattered across three files with three different cases and four border styles, sometimes inside a single method. Replies now stream. Raw text appears as it arrives and each completed markdown block is erased and repainted styled, so a fence renders as a panel and prose renders with emphasis, while the still-arriving tail stays raw — styled means settled, raw means still coming. LiveDisplay was the obvious tool for this and is the wrong one. Reading Spectre 0.57.2: it clamps the region to the viewport and discards overflow lines rather than scrolling them, and on shrink it issues EraseInDisplay(2) followed by ClearScrollback() — which would wipe the chat transcript. So TranscriptWriter drives the cursor directly, gated on caps.Ansi, using EraseInDisplay(0) and relative CursorUp. It refuses to rewind in the three cases where erasing would be wrong rather than merely ugly: past the viewport (those rows are in scrollback and unreachable), after a resize (the row count is stale and would eat unrelated transcript), and when output is redirected. Row counting is width-aware because CJK, emoji and combining marks all break a naive character count, and wrapping is figured at width-1 since conhost and Windows Terminal disagree about a glyph landing exactly on the last column. Chat input moves from Spectre's TextPrompt to Console.ReadLine. Spectre's reader handles only Enter, Tab, Backspace and printable characters — it silently drops arrow keys — whereas the Windows console gives line editing and history for free. It also cannot throw when output is redirected, which Spectre prompts now do, straight into a bare catch that exited the REPL. Glyphs are tiered. Spinner.Known.Dots is Braille U+28xx, absent from Consolas, and it ran on every single turn: the most likely visible breakage in the app. The caret U+276F is in neither CP437 nor Consolas. Both, plus rounded borders and the bullet, now degrade to ASCII outside Windows Terminal. Errors became cards that say what happened and what to do next; a card with no next step is a bug. Rotation is one grey line, not a yellow warning per attempt — rotation working correctly is not a warning. Inline code lost its "white on grey23" background, which downsampled to white-on-black and was invisible on light schemes and identical to body text on dark ones. The markdown renderer now puts one blank line between blocks; it previously emitted none, so headings and lists butted together. /models shows the model name and context size instead of a raw id, finally consuming the DisplayName that was fetched and never displayed. Voice is sentence case throughout, and no screen says DPAPI, OAuth, 429 or ChatErrorKind. Three defects found by running it rather than reading it: the version banner carried the SDK's "+<sha>" suffix; the banner printed twice whenever the screen could not be cleared; and Cursor.Show() in the turn's finally threw "The handle is invalid" under redirection, killing the app right after a successful reply. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WGcarVYpt1mfjk6iFkcDpZ
Grows the suite from 11 tests to 65. The gap was structural: there was no IChatProvider fake anywhere in the repo, so ChatEngine — the entire retry and rotation state machine — had never been executed by a test at all. FakeChatProvider makes attempts scriptable, which is what the engine tests needed to exist. Writing them found two real bugs that no amount of reading had surfaced. The first: replies were never persisted. The success bookkeeping in SendAsync — MarkSuccess, appending the assistant turn, saving the session — sat *after* `yield return` of the final chunk. A consumer that stops as soon as it sees IsFinal, which is the natural way to consume the stream and exactly what the console host does, disposes the iterator at that yield, so none of it ever ran. session.json was never written and no model success was ever recorded. Committing now happens before the final chunk is yielded, so it cannot depend on how the consumer behaves. Draining to completion hides this entirely, so the regression test deliberately breaks early the way the host does. The second: ChatMessage had two constructors, and System.Text.Json refuses to choose between them, so deserializing a session threw NotSupportedException — a type JsonSessionStore does not catch. Restoring a saved conversation had therefore never worked in any build. Marking the intended constructor fixes it. Resume now genuinely round-trips, verified against a real session file. TranscriptWriter also gained a design fix. Its rewind decision came from global ConsoleLayout state rather than the console it was handed, and the legacy-conhost fallback branch called Cursor.Move, which throws without a real console handle. That branch was unreachable in production anyway — the Rich guard already excludes every case that reached it — so it is gone, and the writer now judges from its own console's capabilities. Coverage: engine success, rotation, restart signalling, the NetworkDown and InvalidRequest no-rotate rules, user-turn removal on failure and on cancel, and empty-catalog handling; SSE framing including [DONE] without a finish_reason and in-stream error objects; the captive-portal HTML-with-200 case; HTTP status mapping across nine codes; price-parsing for free-model detection including the "0.000000" form the old string match got wrong; session round-trip, corruption quarantine, and best-effort writes; block splitting across delta boundaries with fences spanning chunks; and width measurement for CJK, emoji, surrogate pairs and combining marks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WGcarVYpt1mfjk6iFkcDpZ
The doc set had 16 internal contradictions and no entry point for either a
user or a contributor. Following docs/02 verbatim produced a project that
did not compile, because Step 2's package list omitted Markdig and included
Microsoft.Extensions.Hosting, which the code correctly never used.
New:
docs/architecture/ layers, request lifecycle, rotation, storage,
provider, console host, error taxonomy, decisions
docs/08-user-guide every feature and every failure, for a non-technical
reader; no acronyms
docs/09-testing test layout, the two helpers, and the two
conventions that catch real bugs
BACKLOG.md execution state and order
LICENSE MIT — the repo was all-rights-reserved by default,
which blocked the F-Droid plan in the roadmap
CHANGELOG, CONTRIBUTING, SECURITY
.github/workflows CI on push, both architectures on a v* tag
08-decisions.md is the one worth reading. It records what was rejected and
why, with evidence — LiveDisplay destroying scrollback, Polly corrupting
rotation's failure accounting, IChatClient breaking the cross-UI contract —
so that ideas which look obviously right do not get re-proposed annually.
Contradictions resolved, code treated as truth:
- docs/02 Step 2 omitted Markdig and prescribed Hosting
- docs/02 and docs/05 spelled out divergent /reset sequences; 05 is
normative and 02 now links to it
- docs/02's OAuth sequence used an ephemeral port, contradicting docs/03,
which explains why the port must be fixed
- docs/04's backoff prose was off by a factor of two and never reconciled
its 1 hour cooldown against the 5 minute cap
- docs/04 prescribed per-attempt console output from Core, which both
violates the layering and corrupts the streamed reply
- docs/00 promised silent rotation; it now shows one quiet line
- docs/00 and docs/02 described the buffered spinner the rebuild replaced
- docs/01 said "three surfaces" and listed four
- docs/03 had two headings named "Error mapping"
- docs/06 rejected AOT for a reason that expired when Spectre 0.55 shipped
trim annotations; the section now states measured status
- docs/06's smoke checklist tested a UI that no longer exists
- /help, /about and /models sat in the Phase 1.1 section while also being
Phase 1 acceptance items, violating the ladder's own lowest-tier rule
- the tier ladder was duplicated in CLAUDE.md, breaking the no-duplication
rule it defines
- <user> placeholders, a placeholder README link, and a 1.0.0-vs-0.1.0
version claim
- roadmap Phase 5 marked Claude models IsFree: true, which would surface
paid models in the free picker
Both acceptance checklists now reflect reality rather than sitting entirely
unchecked while the README claimed the app shipped.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WGcarVYpt1mfjk6iFkcDpZ
Every remaining item below Tier 4. The commands are the visible part; the
prerequisite was that OpenKey had nowhere to remember anything.
/new start fresh, keeping the key
/retry resend the last message
/history the conversation so far
/copy last reply to the clipboard, via clip.exe
/export markdown, defaulting to a timestamped file on the Desktop
/theme default | dark | light | mono, remembered
/new was the most conspicuous missing verb in the app: clearing history
previously required /reset, which also deleted the key and forced a new
sign-in. /retry routes back through the host rather than sending from the
router, so a resent message takes exactly the same path as a typed one.
IAppPaths.ConfigFile had been declared and never read or written since the
first commit, which is why a pinned model could only last until the app
closed. IConfigStore/JsonConfigStore fill it, matching the shape docs/05
already specified — preferredModels doubles as the pin, so no contract
change was needed. Users are told they may edit this file, so it is treated
as untrusted input: blank ids dropped, unknown themes reverted, silly token
limits reset, corrupt files quarantined like a corrupt session.
Token counting moves off chars/4 to a real tokenizer. It goes behind
ITokenCounter because OpenKey.Core deliberately has zero package
references — that absence is what keeps host concerns out of the shared
layer — so Core defines the interface and keeps the heuristic as its
fallback, while the host supplies a cl100k-backed one. The vocabulary is
embedded rather than downloaded: OpenKey has to work on first run behind a
captive portal and makes no network call except to OpenRouter.
Robustness:
- Browser sign-in tries four known callback ports instead of only 3000.
They stay fixed rather than random, because OpenRouter keys its app
record on the callback URL and 409s if it varies.
- A whole turn is now bounded, not just each attempt. Checked between
attempts only — a reply that is actively arriving is working, however
long it has taken, and cutting it off would waste it.
- Link URLs are escaped instead of bracket-filtered. The old filter
silently dropped the target of any URL containing a bracket.
Two backlog items were resolved differently from how they were written:
/stop was not added. A command cannot work while a reply streams — the app
is not reading a prompt — and Ctrl+C already cancels correctly. The real
gap was that nothing said so, so the fix is a one-off "(Ctrl+C to stop)"
hint. A key-watcher was considered and rejected: it would swallow
type-ahead, and people routinely start composing the next message while a
reply arrives.
Richer banner artwork was not added. The console design principle is that
calm beats decorative, and the banner is the first thing a non-technical
user sees. Art would contradict the design it is meant to serve.
The accessibility quick win is now enforced rather than asserted: mono is a
real palette, and a test proves no hue survives it, so anything that starts
carrying meaning in colour alone fails the build.
Also caught en route: Microsoft.ML.Tokenizers 2.0.0 pulls in
Microsoft.Bcl.Memory 9.0.4, which carries a known high-severity advisory
(GHSA-73j8-2gch-69rq). NuGet audit failed the build; pinned forward to
10.0.10 rather than shipped.
84 tests, up from 65.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WGcarVYpt1mfjk6iFkcDpZ
Version had stayed at 0.1.0 through streaming replies, six new commands, preference persistence and a dozen correctness fixes. Bumped and the changelog entry dated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WGcarVYpt1mfjk6iFkcDpZ
CI caught this on its first run; it passed on my machine. Two defects, one visible and one that hid it. The visible one: a single delta can carry both the opening and closing fence marker, and the fence check asked "are we inside a fence" rather than "does the pending text contain a fence at all". Both markers net to not-inside, so an entire code block reached the screen as raw backticks before the block flush replaced it. A model emitting a short fence in one chunk would show it. The one that hid it: TranscriptWriter decided whether it may stream raw text from `console.Profile.Capabilities.Ansi && ConsoleLayout.Rich`. That second term is global mutable state no caller controls, so the same code streamed raw on one machine and not on another, and the tests could not pin it down. caps.Ansi is already false whenever output is redirected, which is the only thing the global contributed — so the writer now judges solely from the console it was handed. The regression test runs with ANSI both enabled and disabled, since a bug that only appears on one capability profile is exactly what got through. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WGcarVYpt1mfjk6iFkcDpZ
ResetDiscardsEverythingFromAnAbandonedAttempt counted occurrences of the answer, which only means anything when the console forbids raw streaming. With ANSI available the writer streams raw and then issues an erase — but a StringWriter has no cursor, so nothing is removed and the count is three. That is correct behaviour being measured by the wrong instrument, and it is why the test passed locally and failed in CI. It now asserts what is actually true in each case: an erase was issued when raw streaming happened, exact occurrences when it did not. Adds the first test that reaches the rewind path at all, since it only runs on an ANSI console. It pins the sequence: never EraseInDisplay(2), never ClearScrollback. Those are precisely what LiveDisplay does on shrink and precisely why it was rejected, so a future change reaching for them fails the build rather than silently eating the conversation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WGcarVYpt1mfjk6iFkcDpZ
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.
Takes OpenKey from a working prototype to something shippable. Seven commits, staged so each is reviewable on its own and leaves a runnable exe.
Why
A scan of the code and docs found three classes of problem: defects that made the app feel broken in normal use, a console that read as a developer tool, and a doc set with 16 internal contradictions — following
docs/02verbatim produced a project that did not compile.The bugs that mattered
Three were invisible from a desk and only surfaced by running and testing the code:
yield returnof the final chunk. The console stops reading there, which disposes the iterator, so the code never executed. No conversation had ever been written to disk in any build. Draining the stream to completion hides this entirely — the regression test deliberately breaks early the way the host does.ChatMessagehad two constructors, soSystem.Text.JsonthrewNotSupportedException— a type the session store does not catch.HttpClient.Timeoutcovers reading the body even underResponseHeadersRead, so a healthy long reply was aborted, misread as a network fault, and retried onto a model that died identically.Plus: one Ctrl+C permanently disabled the session; captive-portal Wi-Fi crashed first run with no visible error; a mid-reply model switch showed the answer twice;
/resetcould strand you in an inescapable loop; an empty model list bricked the app for 24 hours.The console
Replies now stream. Each completed markdown block is repainted styled while the arriving tail stays raw — styled means settled, raw means still coming.
LiveDisplaywas the obvious tool and is the wrong one: reading Spectre 0.57.2's source, it clamps to the viewport, discards overflow lines rather than scrolling them, and on shrink issuesEraseInDisplay(2)+ClearScrollback()— which would wipe the transcript.TranscriptWriterdrives the cursor directly and refuses to rewind in the three cases where erasing would be wrong rather than merely ugly.Also: error cards that say what to do next, ASCII glyph fallback (the spinner was Braille and ran on every turn), sentence-case voice with no
DPAPI/OAuth/429/ChatErrorKindon screen, and an exit hold so a double-clicked exe stops vanishing before you can read anything.Contract changes
Approved beforehand:
InvalidRequestadded toChatErrorKind;NetworkDownno longer rotates or counts against a model;ChatChunkgainsIsAttemptRestart; writes guarded perdocs/05, which already specified behaviour the code never implemented.Also
/new,/retry,/history,/copy,/export,/theme, plusconfig.jsonpersistence and a real tokenizer.Microsoft.ML.Tokenizerspulls inMicrosoft.Bcl.Memory9.0.4, which carries a known high-severity advisory (GHSA-73j8-2gch-69rq). NuGet audit failed the build; pinned forward rather than shipped.docs/architecture/added —08-decisions.mdrecords what was rejected and why.Verified
Build clean with trim/AOT analyzers on, 84/84 tests, live chat against a real model, resume restoring a real session, redirected output staying plain, bare
dotnet publishproducing the exe, and the streaming rewind confirmed non-destructive in a real terminal.🤖 Generated with Claude Code
https://claude.ai/code/session_01WGcarVYpt1mfjk6iFkcDpZ