feat: manage the transcript, its settings and its extensions end to end - #64
Merged
Merged
Conversation
suiflex-bot
Bot
added
commit: docs
commit: feat
commit: fix
commit: refactor
commit: test
maintainer
area: cli
ForgeGuard
🔴 blocks the merge · 🟡 advisory |
mulhamna
changed the title
macOS binds a keychain item to the code identity of the binary that stored it, and a debug build's identity changes on every rebuild, so every rebuild produced an unlock prompt. The prompt arrives while the interactive session is painting its own frame, which is a worse failure than the local-account read the keyring guards against on a machine the operator already controls. The documentation already conceded the point by recommending the file store for exactly this case, and the TUI carried a special case to skip preloading OS handles for the same reason. A 0600 file beside the user configuration answers the same threat on a single-operator machine, so `file` becomes the only store and the keyring dependency leaves the workspace with its three platform backends. `os` stays a recognised store id, answered by a store that resolves nothing and refuses with the command that moves the credential: a handle written by an older build must not read as a typo. Signing in again re-points an endpoint still configured for the keyring, since that is the act that moves such a credential. Nothing migrates automatically — a build without the dependency cannot read what it is being asked to move — and an entry already in the keyring is left where it is, so removing such a handle drops the catalog record without claiming otherwise.
Every stdio server got a thread that wrote its stderr straight to this process's stderr, unconditionally and with no idea that anything was drawing. An interactive session redraws its composer in place, so those writes landed wherever the cursor happened to be, tearing the frame and duplicating the prompt line. A server that prints a deprecation notice on every start could bury a turn. The caller now says where those lines go. A scripted run keeps stderr, which is where a pipeline can capture them; an interactive session collects them in a bounded buffer and drains it at a turn boundary, where nothing else is painting. They stay scrubbed of the values the definition handed the server, and stay untrusted text: they are shown as the server's own output, never as a diagnostic about ARSY. `ui.mcp_log` says how much survives — `hidden`, `summary` (one row per server with a count, the default), or `full`. A server that fails to connect is still reported at every level, because that is a diagnostic rather than logging, and a server that said something is never silently dropped: at `summary` it still gets a row saying how much there was.
The numbered set describes the target design, so it has to stop promising a store that no longer exists and stop saying a server's stderr is written as it arrives. ADR-0013 carries the decision itself, with the threat argument and the alternatives that were weighed, so a later reader finds the reasoning rather than only the result.
`ARSY_CONFIG_HOME` is process-global, so a test running beside this one resolves its configuration from the very file this one rewrites. The rewrite used a truncating write, which is visible while the file is still empty, and an empty `arsy.json` is a fatal parse error rather than a missing layer — so the neighbour failed on this test's write rather than on anything of its own. `bootstrap_user_config` already stages and links for exactly this reason. The test now does the same, which is also the honest thing for it to assert against: production never leaves that window open, and a test that does is testing a situation the code does not create.
Signing in re-points an endpoint away from the withdrawn keyring, but `write_config` owns the user file and only that one. An endpoint a repository or an enterprise layer defined was left pointing at the keyring and nothing said so, which would have met the operator with the same refusal on the next turn, telling them to run the command they had just run. It now reports what it could not edit and where to edit it. The held log buffer drops its bound from the front, so a server in a log loop no longer shifts the whole buffer for every line it writes. The command table still promised `auth set` would use the OS credential store, and neither the drain boundary nor the server-name prefix the scripted route now writes was written down. A turn's worth of logging appears at the next turn boundary rather than as it arrives, and that is a thing an operator should read rather than discover.
The TUI is about to be split into its own presentation crate and given a second look-and-feel. The existing one has to come through that unchanged — it is what every operator's muscle memory is trained on, and "it looks about the same" is not something a reviewer can check. So this records the real output of every widget at a fixed width, with colour on and off, into a file beside the test. Escapes are written as `\e` so the file can be read and reviewed rather than only diffed, and a failure names the first line that moved rather than dumping the lot. Captured before the extraction starts, so it is the current renderer that is being preserved rather than whatever the move happens to produce.
The renderer built rows as strings that were already full of escape codes. A row like that is a dead end: its width can only be recovered by parsing the escapes back out, its colours cannot be recovered at all, and nothing downstream can wrap it, re-theme it, or hand it to a second renderer. Every remaining piece of this work — word wrapping, markdown answers, a selectable look — was blocked on that one representation. So a row is now a sequence of spans that each know their role, and the escapes appear only when it is serialised. Width is a sum rather than a parse. The palette moves here with it, unchanged: the escape codes are the exact ones every built-in theme has always painted, so an operator who picked `ocean` two releases ago still gets the same `ocean`. The crate depends on nothing else in the workspace and does no I/O, so every widget renders the same string twice for the same input — which is what makes the golden test of the interface possible at all. Terminal lifecycle stays in the CLI, where it belongs. Word wrapping arrives with it. There has never been any: a row too wide for the terminal was cut and its end thrown away, which is tolerable for a file path and wrong for the answer an operator is reading. The thinking block is the first of the five hand-rolled card borders to move onto the shared widget, and the golden test holds its output to the byte.
…widget Three of the five hand-rolled card borders now use the shared widget, so the border arithmetic they each kept a copy of exists once. The approval card's rows name a palette role instead of carrying an escape string, which is what let its frame move at all. The visible output does not change. Where a refactor writes one escape run in place of two of the same colour — `╭` and `──` were painted separately — the bytes differ and the picture does not, so the golden records both halves and the `COLOUR OFF` half is byte-identical before and after. Seven lines moved, none of them visibly: checked by stripping the escapes from the pre- and post-conversion captures and comparing. The tool cards keep their own borders. Their accent and border are six literal hues that no theme owns, so their frames cannot be expressed in the model without either bending it or changing the colours — which is the renderer split's job, not this one's. The launch banner keeps its own body rows for the same kind of reason: they arrive already painted, and on a Kitty terminal one of them has an image spliced into it, which the span model would strip as untrusted control text. Its two rules have moved. A theme now says whether it has hue rather than being recognised by its colours: the tool cards used to detect `mono` by comparing the palette's border and accent against `mono`'s literal escape codes, which would equally have fired on any theme that happened to share those two values.
The interactive half of this test ran in an isolated home; the non-interactive half did not, so `mcp list` read whatever Claude and Codex configuration the machine running it happened to have. On a developer's own machine the first declaration was never the fixture's, and the test failed before it reached anything it was written to check. The two halves now share one environment, and the variables that override the home-derived paths are removed rather than pointed at an empty directory — redirecting `CLAUDE_CONFIG_DIR` stops the workspace's own hooks being found at all, and redirecting `ARSY_CONFIG_HOME` moves the `.arsy/` the test later asserts was written. Two expectations had also gone stale against the product. A bare `/mcp` opens the toggle dialog now, so the inspection assertions are addressed to `/mcp show docs`, which is the surface they were written for and still prints exactly what they look for.
runtime and registry grew a skills parameter; the debug.run tests were the three call sites the change missed, so the test target stopped compiling everywhere.
Both tests compared a path they built with a slash against one the code built with the platform separator, so on Windows the disabled-hook key never matched and the golden's fixture root was never scrubbed. Neither ran there before — the target did not compile.
Replacing every backslash in the serialised document turned each escaped separator into two slashes — and would have eaten a quote or a newline escape with it. The pair is what a separator looks like there.
badrus123
reviewed
Sep 20, 2026
badrus123
reviewed
Sep 20, 2026
badrus123
reviewed
Sep 20, 2026
badrus123
reviewed
Sep 20, 2026
badrus123
reviewed
Sep 20, 2026
badrus123
reviewed
Sep 20, 2026
badrus123
reviewed
Sep 20, 2026
badrus123
reviewed
Sep 20, 2026
The key is registered and Config::mcp_log reads it, but setting_value had no arm for it, so every reader saw an empty string: the settings row read blank, and the dialog submitted "" — which validation then refused.
Splitting the table dropped the five claude-*-thinking arms, so a Claude model at medium or high effort quietly stopped asking to think. Pro also gained a -medium arm naming an id the API was never told about; medium and an unset effort ask for the plain id again, as before the split.
take_recorded ran only on the success path, so a turn that approved a rule and then failed left the grant in the cell: its evidence was never written, and the next turn filed it under its own id — attribution for a mutation that belonged to another turn (invariant 7).
The composer edits a key before it finishes one, so Char('e') was taken
as text and Action::Expand was never returned: every finished card stayed
clipped and the hint named a key that did nothing. A control byte owns
the action instead, leaving a prompt that starts with `e` as typing.
Ctrl-E was not free — it is End, the pair of Ctrl-A.
The home skills claimed their names first, so the workspace copy was the one dropped — the opposite of what the comment promised and of what a repository shipping its own skill means by shipping it. The workspace is listed first and the home yields to the names it claims.
The listing printed each skill's path and told the model to read it, but a home skill's path is absolute and resolve_file refuses to leave the workspace, so those reads could only fail. Workspace skills are named by the skill:// reference that resolves them — the scheme the prompt never mentioned — and the rest carry their description alone.
An unsupported event, or a handler type this build never runs, was still offered as a row to switch off. Toggling one wrote a key that matches nothing and then read back as "off", which reads as a change that took effect.
main gained three features after this branch was cut, all of them inside functions this refactor relocated out of lib.rs: - OAuth token mid-session refresh (PR #66): dispatch_with_refresh, native_status_with_refresh, is_stale_oauth_token, and the &mut Resolved threading through run_turn/native_turn/resolve_route/ take_turn now live in run.rs, turn.rs, and picker/prompt.rs. TaskRun no longer holds &Invocation here, so the refresh re-resolves by resolved.endpoint.id directly rather than the CLI's --provider flag — the same endpoint either way, one field access instead of a stored reference. - Durable task attempts and evidence (PR #61, feat/phase0-runtime-truth): TaskNode.runtime and the agent_runtime lineage parameter, both auto-merged into orchestration.rs and most call sites; two call sites this refactor's new files reintroduced (run.rs enqueue/execute, turn.rs record_turn/run_turn) needed the same wiring by hand. - content_block_start index desync fix (PR #69): auto-merged clean, kernel-only, no overlap with this refactor. Verified: cargo build/clippy(-D warnings)/fmt/test --workspace --all-features all clean, 680 tests passing, including the token- refresh, tool-block-order, and flaky-follow-up regression tests.
Contributor
|
This branch predates #66, #61 ( Opened #70 against this branch with the merge done and the two features (OAuth mid-session refresh, task-attempt lineage) re-wired into their new locations in |
badrus123
reviewed
Sep 20, 2026
badrus123
reviewed
Sep 20, 2026
badrus123
reviewed
Sep 20, 2026
Six findings from the inline review of this branch, each with the check that fails without the fix. `skill://` resolved a home-declared skill to the absolute path its own listing named, and the workspace refused it for being absolute — so every skill the operator's home declares was listed in the prompt and unreadable. Those files are read from where they live now, through a reader that only a path the harness itself resolved reaches; an absolute path the model types is still confined. The hook listing read only the workspace's `.claude/settings*.json` while the engine also loads `~/.claude/settings.json`, so `/hooks` named a subset of what runs and could not switch a home hook off at all. Both are listed, and the declaration key a row carries is the one the engine builds. `file` is the only credential store now, so its guards carry the whole weight: the traversal check `set` had is applied to `remove` and `resolve` too, and a write forces `0600` on a file that already exists rather than leaving a secret in whatever mode was there. `Line::fit(0)` returned a row one column wide, overrunning a panel narrower than its own lead. Front matter that never closes is no longer read to the end of the file, where any line starting `name:` became the skill's name.
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
3 participants
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.
Summary
arsy-tuicrate, with a modern default projection and the classic one held to the byte./hooks,/skill,/settings,/session— now manage what they show, and skills reach the model at all.Changes
TUI & transcripts
arsy-tuicrate: cell model, palette, wrapping, panels; modern projection as default, classic held byte-for-byte by a golden test.eon an empty line expands the last tool call's whole output, while a call is running and after it finishes.Commands that manage
/hooks: every declaration is keyed the way the engine keys it, so toggling one in the dialog really stops it (hook.disabledin the operator'sarsy.json, never another tool's file)./skill: lists workspace and user skills (from~/.claude/skillsand~/.codex/skills, the directories the ecosystems themselves read), shows a skill's body, and switches it off from the prompt./settings: an in-dialog editor over a kernel-side registry of the writable keys — arrows cycle, Enter applies, theme and transcript style take effect immediately. No typing blind./session: the running session is a real row, so rename and delete act on it; a failed write is reported as a failure.Kernel & compat
credentials.store,ui.style,ui.mcp_log,execution.max_parallel,compat.*.enabled,theme.base) with validation before the write.skill://<name>resolves infs.read, matching the ecosystem family's convention.Refactor
lib.rs13,087 → 5,880 lines: pickers, turn engine, task runner, config loader and catalog extracted into modules. Pure moves, behaviour held by the test suites.Test plan
cargo test --workspace --all-features— cli-lib 175, code-lib 164+21, compat 16, all integration targets greencargo clippy --workspace --all-targets -- -D warnings(the CI lint step)cargo fmt --all -- --checktui_smoke.py): dialogs, plan mode, inspection, filtering, paste, terminal restorationtui_smoke.pyasserts no MODE rows and no recorded events from a bare inspection