mirror of
https://github.com/Twigpine/zero.git
synced 2026-11-09 18:08:31 -10:00
fix/opencode-session-header
238
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Anandan
|
d73ec81e2d |
feat(tui): interactive terminal attach for tty exec sessions (#1055)
* feat(execution): add a stdin-only write for interactive processes * feat(tui): attach overlay for interactive terminal sessions * feat(tui): add /attach for interactive terminal sessions * feat(tools): point the model at tty sessions for interactive prompts * feat(tui): open the terminal automatically for tty exec sessions * feat(tools): tell the model to keep polling while the user types * feat(tui): give attached terminal sessions the full viewport * feat(tools): route sudo through an escalated tty session * fix(tui): route the auto-attach tick to its run * fix(tui): defer auto-attach while another view owns the screen * fix(tui): keep bracketed-paste framing for attached sessions * fix(tools): let tty sessions run interactive commands * fix(tui): capture the auto-attach baseline before the exec starts |
||
|
Anandan
|
2f531178f6 |
feat(tui): richer /resume session browser (#1047)
* feat(tui): support an optional detail line on picker rows
Add a faint second line under picker items so richer surfaces can show
secondary metadata (project, model, size) without crowding the label.
The line renders on the selection band, joins the search haystack, and
only appears at medium width and above; detail rows halve the visible
window so the overlay keeps its height, and mouse hit-testing maps a
click on either line to the same item.
* feat(tui): show project, model, and size under /resume rows
Each session row now carries a detail line with the project directory
(~-contracted), the model that ran it, and its event count, plus a
status chip for forks and tagged sessions. The line also joins the
filter haystack, so searching by project path or model name works.
Narrow terminals keep the existing single-line list.
* feat(tui): show relative last-activity age in /resume rows
Replace the absolute timestamp column with relative age ("2h ago",
"12d ago") so recency reads at a glance; distant history still shows
the calendar date. Also adds coverage for the detail line: rendering
at wide vs narrow tiers, status chips, and filtering by model.
* refactor(tui): drop the event count from /resume detail rows
Event count is an internal metric — the detail line now carries just
project, model, and the status chip, which are the fields that
actually discriminate sessions.
* fix(tui): sanitize session metadata before it reaches picker rows
Persisted titles (--session-title, /rename) and other metadata can
legally contain newlines; a stray line would break the picker's fixed
row geometry and misalign mouse hit-testing. Reuse sanitizeCardField
for the label and detail line, and cover the detail-line click path.
* fix(tui): strip carriage returns from rendered session metadata
A \r in a persisted title or model id rewinds the cursor to column 0
and overwrites the row. Fold it into sanitizeCardField alongside the
existing newline handling, and cover it in the picker test.
* fix(tui): pad picker detail lines on the selection band
The detail line only painted its text, so the selection highlight
stopped at the end of the text instead of spanning the row like the
label line above it. Pad the remainder with the row surface.
* test(tui): cover carriage returns in sanitizeCardField
* fix(tui): drop all control runes from rendered session metadata
The picker detail line newly renders cwd, model id, and tag, and any
of them can legally carry ESC/CSI/OSC/BEL/BS/VT/NEL bytes — a project
directory name containing \x1b[2J would repaint the terminal when the
picker opens. sanitizeCardField now maps line separators and tabs to
spaces and drops every remaining control rune, matching the package's
other sanitizers; this also closes the same hole for titles, which
predates the detail line.
* fix(tui): include exactly-30-day sessions in the relative-age branch
|
||
|
Vasanth T
|
91d921f4c5 |
feat(tui): offer mid-run model escalation behind --allow-escalation (#1019)
* feat(tui): offer mid-run model escalation behind --allow-escalation Mid-run escalation existed but only exec offered it. The TUI already handled every consequence of a switch, re-deriving the compaction threshold "after a mid-run escalate_model switch" and resolving the summarizer against the active profile, while nothing on that surface could cause one: escalate_model was registered by exec alone. This closes that, opt-in, the same conservative way exec answers the question. Opt-in rather than default because escalation moves a run onto a different model, which changes what it costs and which provider sees the conversation. That is the operator's call, and the exec flag already answers it. The switchers are built per turn from m.providerProfile rather than once at launch. A TUI session can change models with /model, so a closure capturing the startup profile would escalate from whatever the session began with instead of what is in force now, and would carry that stale profile's base URL and credential with it. exec has no such problem because its profile cannot change mid-run, which is why the wiring lives in different places on the two surfaces. The nil-contract logic is now shared rather than copied. providers. EscalationSwitchers is the single implementation both surfaces call: the loop swaps only on a non-nil provider, so a (nil, nil) return has to leave everything untouched including the caller's usage attribution, and the session switcher is installed only when the run STARTED optimized so escalation cannot change the transport underneath a session. A second copy of that would only ever have been exercised on one surface. exec's behaviour is unchanged: its six existing escalation tests pass against the shared builder. The tool and the switchers ride on one flag and a test asserts they cannot be separated. Registering escalate_model without wiring a switcher ships a tool the loop silently ignores, so the model reports an escalation that never happened; that is the specific failure worth a guard rather than a comment. Note this does not answer all of #554. Assigning models per phase, plan with one and execute with another, is still not built. This makes the mechanism reachable from the interactive surface, which is the part that was one flag away. Refs #554 * fix(tui): attribute usage after an escalation to the model in force exec reassigns currentModel when the escalation switcher fires; the TUI captured usageModelID once per run and handed the switcher no callback, so every usage event after a mid-run escalation was billed to the model the run started on. The switcher now updates usageModelID, and the final response carries the model per usage event so the batch fallback does not swing the other way and bill the events before the switch to the escalated model. Covered by a run through a scripted escalation that checks both the live and the batch attribution. * fix(cli): accept the root flags in any order Each leading-flag splitter stops at the first token it does not own, so running them once in a fixed sequence made the order load-bearing: `zero --allow-escalation --theme auto` stranded the theme as an unknown command and exited with an argument error instead of launching, and the same happened after --skip-permissions-unsafe. The three splitters now run until they make no progress, at the root and after the unsafe flag, so every ordering reaches the TUI with every flag applied. A --theme written before --skip-permissions-unsafe was dropped on that path; it is kept now, with a later one winning as the last occurrence does. * fix(cli): forward a root --allow-escalation into exec, reject it elsewhere The root flag was stripped before dispatch, so `zero --allow-escalation exec ...` and `zero --allow-escalation -p ...` ran without the opt-in the help text promises, and `zero --allow-escalation version` accepted a flag it could only discard. It now follows --add-dir: re-synthesised into the exec argument list for the exec and -p shapes, so exec parses it as its own flag, and rejected loudly for every other command. The ordering test now generates every permutation of the three root flags, with --skip-permissions-unsafe absent or at any position. * fix(tui): record the model on an escalated run persisted usage events The live usage record carried the model in force; the persisted session payload did not. The usage report rebuilds cost from that payload and falls back to the session-wide model when an event does not name one, so an escalated interactive run was priced end to end at the model it started on. Written only under escalation, matching what exec records under the same flag, so an ordinary run persists the same compact payload as before. * test(cli): isolate the new launch helpers from real user state A temporary working directory isolates project files and nothing else. runWithDeps fills the dependencies a test leaves out with production ones, and the interactive launch path reads user config, opens stores, refreshes the models.dev cache and migrates any inline plaintext API key into the credential store before it reaches an injected runTUI callback. Running these tests on a developer machine therefore rewrote that developer's config.json and wrote a credential file beside it, and the results depended on whatever providers, MCP servers and plugins the machine had. Both new helpers now point every per-user base directory at a throwaway root, pin the models.dev cache path, disable its background fetch, and force the file credential backend so nothing reaches the host keyring. A new test seeds a config with an inline key outside those fixture roots and fails if either helper rewrites it or leaves a file beside it; without the isolation it reports the rewritten config and the credentials.enc files. * test(cli): check each launch helper isolation on its own t.Setenv restores at the end of the test, not when the helper returns, so one test exercising both helpers let the first isolation cover for the second. Each helper now seeds its own emulated user config in a subtest, and removing the isolation from either one fails that subtest. * test(cli): pin the isolation itself, not only what this platform writes The exec helper writes its models.dev cache from a background goroutine, so a footprint check could not prove its isolation deterministically. Each subtest now also asserts the helper moved the per-user paths off the seeded root and disabled the fetch and the keyring backend, which fails for either helper when the isolation call is removed. |
||
|
|
b04fe96e9e |
fix(config): lock config read-modify-write across processes (#960)
* fix(config): lock config read-modify-write across processes Every config mutator loads the whole document, edits independent fields, and publishes a complete replacement by rename. The rename is atomic, so a reader never sees partial JSON — but two processes that loaded the same revision each write a full document, and the second rename silently discards the first one's acknowledged update. The result is valid JSON with one update missing and no error anywhere. lockConfigFile takes a cross-process advisory lock through lockutil, using the retry-with-deadline idiom cron, hooks and oauth already share (10s timeout, 20ms retry). Callers acquire BEFORE their first read, so the lock spans load, mutation, validation and publication and the read inside it is authoritative — holding it only around the write would still let both processes start from the same stale revision. The lock file is a sibling (config.json.lock), never the config itself: an advisory lock is held against an inode, and publishing by rename installs a new one, so locking the config directly would leave each process holding a different inode. Covered: all 14 mutators in writer.go, plus ClearProviderKeyStored and MigratePlaintextProviderKeys in credentials.go. The migration matters most — it rewrites the config on every startup, so it is the likeliest writer to collide with an interactive mutation in another Zero. The SetProviderDescription test seam locks too, so it cannot stand in as the one unsynchronized writer. Two shapes needed care: - The lock is not reentrant. EnsureCatalogProvider scans for an existing profile and then upserts, so UpsertProvider is split into a locking wrapper and upsertProviderLocked; one lock now spans the scan and the upsert, which also closes the window where two processes could both create the same catalog profile. - SetPet edits raw bytes to preserve unknown members and formatting rather than round-tripping the struct. It takes the same lock, so it neither clobbers nor is clobbered by the struct writers. Regression tests, each verified to FAIL with the lock disabled: - TestConcurrentMutationsDoNotLoseUpdates — theme, pet, recaps, favorites and a provider mutated at once; all five are independent fields, so a lost update shows up as a zero value in exactly one of them. - TestConcurrentProviderUpsertsAllSurvive — 16 distinct providers added concurrently, all must be present. - TestConcurrentSameFieldMutationsSerialize — 24 writers contending on one field; every call succeeds and the document stays readable. - TestCrossProcessMutationExcludesAndPreserves — the coordinated two-process case. Goroutines share this process's descriptors, so only a second OS process shows the lock is kernel-held. The child announces itself, then the parent asserts its write cannot land while the lock is held elsewhere, does its own mutation, releases, and requires both updates to survive. Fixes #832 Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01U389qmQUhoZB3YtXkFmiTq * fix(cli): take the config lock for MCP config edits The previous commit locked internal/config's mutators, but that is not every writer of the user config document. internal/cli's MCP editor reads the SAME file, edits it, and republishes it with the identical temp-file+rename shape, at three sites (add/update, remove, disable/enable). Locking only one package left `zero mcp add` free to clobber a concurrent provider or preference write — and to be clobbered by one — with the file still valid JSON afterwards, which is the same silent lost update issue #832 reports. config.LockFile exports the existing helper so the lock is one authority across packages rather than a private detail of internal/config. A second, adjacent implementation would drift from the first exactly the way this writer already drifted from the mutators. TestRunMCPAddParticipatesInConfigLock is the regression. Racing the two writers and waiting to observe a lost update is NOT reliable — the losing interleaving is narrow, and a straightforward concurrent version of this test passed five consecutive runs against the unlocked code, so it would have shipped as reassurance that proved nothing. It instead asserts the deterministic property: while the config lock is held elsewhere, the MCP writer's update cannot land, and it completes once the lock is released. That fails immediately against the unlocked code and passes with it. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01U389qmQUhoZB3YtXkFmiTq * fix(config): report config unlock failures instead of discarding them AGENTS.md requires that a mutator never report success when unlock failed, and the release error was being dropped on the floor. A failed Release can leave the lock held for the rest of the process, so returning (cfg, nil) after one claims a state the next mutation cannot reproduce — it will block for the full lock timeout and then fail. lockConfigFile now returns lock.Release directly, and every caller joins it into its own result with the idiom credstore already uses: defer func() { err = errors.Join(err, unlock()) }() Joined, not chosen between: a release failure annotates the result rather than masking the mutation error that explains what actually went wrong. That covers all 16 mutators in writer.go, both in credentials.go, and the test seam. The three MCP sites return an exit code rather than an error, so they convert a release failure into a crash exit and a stderr message, without overwriting a failure the command had already reported. A release failure cannot be provoked through the public API — lockutil.Release is idempotent and reports nil once released — so the mutators call through a lockConfigFileFn seam that a test can substitute. My first attempt at this test could only ever t.Skip, which asserts nothing; the seam is what turns it into a real assertion. - TestMutationReportsUnlockFailure: SetTheme surfaces the release error AND the mutation is still published, since a release failure annotates the result rather than undoing the write. - TestMutationErrorSurvivesUnlockFailure: an unknown-provider error and the release failure are both present in the joined error. Both verified to fail when the defer discards the release error. Note for review: cron, hooks, oauth and swarm all still discard their lockutil.Release error, so this makes internal/config stricter than its siblings. Worth deciding whether the guideline should be applied to them too — out of scope here. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01U389qmQUhoZB3YtXkFmiTq * fix(cli): complete MCP config lock handling Amp-Thread-ID: https://ampcode.com/threads/T-01a043da-1703-70c5-9d8d-904cd8fd964b Co-authored-by: Pierre Bruno <[email protected]> * test(cli): synchronize MCP config lock assertion Amp-Thread-ID: https://ampcode.com/threads/T-01a0448f-5860-721c-8a47-5119fc57f685 Co-authored-by: Amp <[email protected]> * fix(tui): persist provider model selection atomically Amp-Thread-ID: https://ampcode.com/threads/T-01a04c92-2d1d-7508-91bc-416341b7e8b0 Co-authored-by: Amp <[email protected]> * fix(tui): surface unsaved model selections on every switch path choosePicker replaces handleModelCommand's status text with its own applied-notice, so the "· not saved (…)" qualifier that text carried was dropped and the selection reported an unqualified success. The user only found out at the next launch, when the old persisted model came back. dispatchCommand drops the same text on a committed switch, so typed /model had the identical hole — the cross-provider path already returned the outcome as a value precisely because display copy is not a contract. handleModelCommand now returns that outcome too, and one modelAppliedNoticeFor decides how a committed switch is announced for both entry points, so neither can regain the gap independently. Recent-model history is written under the same lock, so a selection that already lost the lock would pay a SECOND full timeout for one keystroke and add a redundant save error. It now falls back to the in-session updateRecentModels for exactly that case, keyed on a new config.ErrLockUnavailable rather than on any persistence failure: a missing config file also fails the lookup, and that write still has its own reason to be attempted. Regressions cover both entry points. The picker test holds the config lock — the case the review asked for — and bounds elapsed time, which is what proves one keystroke costs one wait; it fails at 20s against the previous code. The typed test uses an unwritable config path so it proves the same plumbing without a second ten-second timeout. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01REzorhNj3F1DGPXyn5Uq7j --------- Co-authored-by: Claude Opus 5 (1M context) <[email protected]> Co-authored-by: Amp <[email protected]> |
||
|
|
79fa1422ef |
fix(providers): register Atomic Chat as local OpenAI-compatible runtime (#550)
* fix(providers): register Atomic Chat as local OpenAI-compatible runtime Point atomic-chat at the default local API (http://127.0.0.1:1337/v1), mark it Local with no API key, and auto-detect it alongside Ollama and LM Studio. Keep opencode-go-anthropic-compatible in the provider catalog and curated model list after rebasing onto latest main. Co-authored-by: Cursor <[email protected]> * docs(zh): sync Atomic Chat local runtime into README_ZH The English README lists Atomic Chat in the provider list and tells local-model users to run its desktop app on http://127.0.0.1:1337/v1, but the Chinese README still mentioned only Ollama and LM Studio. Co-Authored-By: Claude Opus 4.8 <[email protected]> * fix(providers): give the local Atomic Chat runtime its own catalog ID Repurposing the existing atomic-chat ID from the hosted, key-gated preset to a keyless local one silently downgraded credential checks for profiles already saved against the remote endpoint: MissingCredentialEnv resolves RequiresAuth from the catalog descriptor, so those profiles were reported as ready and only failed later with an unauthenticated call to api.atomic.chat. Keep the hosted atomic-chat descriptor as it was and register the local runtime under a separate atomic-chat-local ID, mirroring the existing ollama-cloud/ollama split. Stored profiles keep their identity and no migration is needed. Co-Authored-By: Claude Opus 4.8 <[email protected]> * test(onboarding): assert the full Atomic Chat local endpoint and default model Co-Authored-By: Claude Opus 4.8 <[email protected]> * fix(models): drop the placeholder curated list for atomic-chat-local Atomic Chat's model catalog is a Hugging Face scrape refreshed every 12h plus arbitrary owner/repo lookups, so the served model ID is whichever model the user loaded and no hardcoded list stays accurate. The single local-model entry short-circuited Models() without adding anything: the descriptor is openai-compatible, so modelMatchesProvider never matches a registry entry and the fallback already yields the descriptor's DefaultModel. Output is unchanged; the difference is that local-model is no longer presented as a curated contract. Co-Authored-By: Claude Opus 4.8 <[email protected]> * fix(onboarding): pin the probed model in the local-runtime adopt command probeLocalRuntime already reads the runtime's /v1/models, but SetupAction dropped the result and emitted 'zero providers add <id> --set-active' with no --model. The add path then persisted the catalog DefaultModel, which for a local runtime is the placeholder 'local-model', and the first completion failed with an unknown-model response. SetupAction now pins the probed id via SetupCommandWithModel. The catalog default wins when the server actually serves it, so an existing Ollama setup does not switch models; otherwise the first advertised id is used. An empty probe result leaves the command on the catalog default rather than inventing an id. Covers a server whose returned id is not 'local-model'. Co-Authored-By: Claude Opus 4.8 <[email protected]> * fix(onboarding): shell-safe adopt command + reject atomic-chat-local placeholder model commandArg now POSIX single-quotes rendered args instead of strconv.Quote, so a model id from an untrusted /v1/models response cannot expand when the adopt command is pasted into a shell. providerProfileForAdd now errors when atomic-chat-local is added without --model instead of persisting the local-model placeholder that fails the first completion. Adds regression tests for command substitution, backticks, and the require-model guard. * fix(onboarding): finish Atomic Chat setup across supported shells Rebase on current main and keep the hosted Atomic Chat preset intact. Only emit setup arguments that are portable across POSIX, cmd.exe and PowerShell; show interactive guidance for other model IDs. Reject the Atomic Chat placeholder before both CLI and TUI persistence, and stop advertising adoption when discovery finds no usable model. Validation: full Go test suite, focused race suite, vet, release build, smoke, formatting, advisory lint and vulncheck pass with isolated config and inherited Anthropic overrides removed. Onboarding, CLI and TUI test packages cross-compile for Windows and Linux. Regression tests fail on the rebased pre-fix code and pass with these changes. Refs Gitlawb/zero#550 and Gitlawb/zero#592. * fix(onboarding): preserve option-looking model IDs in adopt commands Use the existing --model=value grammar when a discovered model begins with a hyphen. Validate the model separately from the fixed separator so shell safety remains unchanged and '=' is not added to the allowlist. Exercise generated detection actions through shell tokenization, the actual add parser, and profile construction for Atomic Chat, LM Studio and Ollama. Cover leading hyphens, flag-looking IDs, ordinary IDs, spaces, and unsafe inline values. Before the fix, the new regression failed with: generated adoption command rejected by add parser: --model requires a value Validation: full Go suite, focused race tests, formatting, vet, release build, smoke, lint-static and vulncheck passed. Affected test packages cross-compiled for Windows and Linux. Refs Gitlawb/zero#550. * fix(onboarding): select eligible chat models and preserve Ollama defaults P2: apply the existing known-noncoding exclusion before default preference or first-model fallback. When a nonempty list has no eligible model, offer commandless guidance instead of persisting the rejected/default model. P3: recognize an Ollama untagged default's :latest spelling, preserving the advertised ID. Prefer exact matches and leave other tags/providers alone. Add selector tests plus a mock-server integration test through discovery, the generated action, the real add parser, profile persistence, a fresh process, and the chat completion request. Before this fix, that test failed: fresh process sent model "text-embedding-local", want eligible model "-loaded chat model" The Ollama alias case also failed before the fix: AdoptModel() = "qwen3:8b", want "llama3.1:latest" Explicit model choices, shell checks, hosted/local provider identities and Atomic placeholder guards retain their existing behavior. Validation: full Go suite, focused race tests, formatting, vet, release build, smoke, lint-static and vulncheck passed. Affected test packages cross-compiled for Windows and Linux. Integration evidence uses a mock server; native Windows shell and actual runtime behavior are not claimed. Refs Gitlawb/zero#550 and Gitlawb/zero#592. --------- Co-authored-by: Cursor <[email protected]> Co-authored-by: Claude Opus 4.8 <[email protected]> Co-authored-by: alex <[email protected]> |
||
|
euxaristiaandeuxaristia
|
a3e7446681 |
chore(tui): remove unused test exports GetLocalDiffStats and WatchPRState (#975)
Refs #904 Co-authored-by: euxaristia <[email protected]> |
||
|
Anandan
|
eeea3308fe |
fix(tui): move voice capture to Ctrl+Space (#968)
* fix(tui): keep voice mode from swallowing spaces * fix(tui): clarify voice capture controls * fix(tui): align active voice status with capture key |
||
|
Anandan
|
1fde941803 |
perf: reduce startup and turn overhead (#932)
* perf(tui): disable duplicate composer cursor blink * perf(tui): stop composer wakeups after idle * refactor(tools): publish immutable registry snapshots * perf(startup): defer optional tool discovery * perf(provider): reuse compatible response sessions * feat(provider): add native patch tool input * fix(provider): recognize saved chat sessions * perf(tui): render only on screen changes * fix(runtime): address performance review findings * fix(usage): expose cache efficiency details * perf(agent): reduce redundant bounded-task turns * fix: harden runtime review edge cases * fix: address runtime review findings * build: restore official Bubble Tea dependency * fix(cli): redact MCP registration errors * fix: harden turn session regressions * fix: keep run tools and patch roots consistent * fix: bind freeform patch writes to scoped roots * test: keep patch fixtures out of worktree * fix(tools): reject mixed-root freeform patches |
||
|
Kevin CodexandClaude Fable 5
|
6645f9744b |
perf(agent): cheap compaction — dedicated summarizer model, input cap… (#532)
* perf(agent): cheap compaction — dedicated summarizer model, input caps, free reactive prune, window re-derive
Compaction summarization was the single most expensive recurring event
in a long run: the full elided middle — verbatim tool bodies included —
went to the session's main model at full price (a ~100k-token call near
a 200k window, recursively multiplied on overflow).
Four changes, each independently safe:
- Dedicated summarizer model. Compaction summaries route to a cheap
model resolved as ZERO_COMPACTION_MODEL > preferences.compactionModel
> a curated default on official endpoints (Haiku 4.5 / Gemini
Flash-Lite / gpt-4.1-mini; custom-endpoint catalogs are unknowable so
they get none). Built lazily on the first paid compaction; ANY
failure permanently falls back to the main provider for the run, so a
misconfigured summarizer can never break compaction. "main" at any
level forces the old behavior.
- Summarizer input caps. renderTranscript clamps tool-result bodies to
2,000 chars and tool-call arguments to 500 (head+tail, rune-safe) —
the summary needs what was done and decided, not raw payloads. Cuts
summarizer input 5-10x on tool-heavy sessions.
- Free reactive prune. recover() now runs the same prune-first stage as
the proactive path: a context-limit error is first answered by
pruning stale tool bodies at zero cost, without consuming the
one-shot reactive budget; the paid summarizer only runs when a later
error finds nothing left to prune.
- Window re-derivation on model switch. Options.ContextWindowFor
(wired to the model registry in exec and the TUI) lets the loop
re-derive the compaction threshold after escalate_model, closing the
documented KNOWN LIMITATION where a switch to a smaller-window model
could overflow and a larger one over-compacted.
## Rebase notes (2026-08-25, onto
|
||
|
Kevin CodexandClaude Fable 5
|
2749447cdc |
fix(tools): make apply_patch tolerant and re-expose edit_file (#956)
* fix(tools): make apply_patch tolerant and re-expose edit_file
Head-to-head benchmarking against Grok Build on grok-4.6 (3 tasks × 3 runs)
showed Zero solving every task but spending 2.7× the tokens and 2.3× the
time on the multi-file feature task. Every one of the 20 apply_patch calls
in those runs failed, after which the model degraded to whole-file
write_file rewrites and read→write verification loops.
Root causes and fixes:
- The sandbox and the tool both rejected an absolute path that lies inside
the workspace ("must stay inside the workspace"). Models routinely echo
the absolute path read_file showed them. Absolute paths now flow through
the normal workspace-scope validation (inside → allowed, outside → still
denied); unified-diff headers are rewritten to a/ b/ relative form before
git apply, and platform symlink prefixes (/var → /private/var) are
normalised the same way the other file tools do.
- The structured parser demanded a byte-exact "*** Begin Patch" /
"*** End Patch"; grok-4.6 writes "*** Begin Patch ***". Decorated marker
spellings are accepted, and a unified range after "@@" ("-12,4 +12,6",
optionally with a heading) no longer becomes a literal context search.
- Parse errors now carry a one-line format reminder so a failed call is
recoverable in one retry instead of a format-guessing loop.
- edit_file (exact search/replace with uniqueness guard, CRLF and fuzzy
fallbacks) is advertised to the model again alongside apply_patch, and
the apply_patch description shows the structured format.
- Writers (edit_file, write_file, apply_patch) recorded a file's line total
with lineCount, which is one higher than read_file's count for a file
ending in a newline. RecordSeenRange resets every observation when the
total changes, so a partial read_file after a successful edit silently
discarded whole-file knowledge and the next edit was refused with
"content that has not been read exactly in this session". All writers
now record the same total read_file reports (trackedLineTotal).
- read_file's padded " N | " line prefix was ~19% of every read (about
1.5k tokens per 800-line file) and was re-sent on every later call. It is
now the compact "N→" prefix models already know; nothing parsed the old
form.
- The editing-discipline prompt steers: edit_file for a targeted change,
apply_patch for multi-hunk/multi-file changes, write_file only for new
files or full rewrites; a successful edit result is confirmation, so the
file is not re-read to verify it.
Tests: regression tests for decorated markers, writer/reader line-total
agreement (edit → partial read → edit far away still succeeds), range hunk headers,
absolute in-workspace paths (structured + unified), outside-workspace
rejection, header-line-only relativisation, and tool visibility; the
prompt and tool-budget tests updated to the new contract.
Benchmark (grok-4.6, 3 tasks x 3 runs, harness in demo/bench-zero-vs-grok):
Zero 0.8.0 617 s / 1,527k tokens -> with this change 346 s / 493k tokens;
Grok Build 407 s / 713k. Every run of every configuration solved its task.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* fix(tools): keep CRLF line endings when relativizing patch headers
relativizeUnifiedPatchPaths split on a normalized "\n" and rejoined with
"\n", so any CRLF patch that needed a header rewrite lost its line endings
everywhere; git apply could then fail to match a CRLF file. Split on "\n"
only and carry each line's trailing "\r" through, including on rewritten
headers.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* fix(sandbox): share the structured-patch classifier with apply_patch
Review follow-ups for #956:
- The tool accepted decorated structured markers ("***Begin Patch") that
the sandbox's applyPatchPaths did not recognise, so for that spelling the
boundary scanned the patch as a unified diff, extracted no targets and
validated nothing. The marker classifier now lives in the sandbox package
(StructuredPatchMarker / IsStructuredPatch) and the tool delegates to it,
so both sides accept exactly the same set; tests cover inside, traversal
and outside targets for the no-space spelling at the boundary.
- Document the containment model at the apply site: structured patches are
applied through an opened os.Root (descriptor-relative, no-follow);
unified diffs keep the pre-existing git-apply pathname flow, which the
absolute-path rewrite does not widen (they become the same root-relative
form relative paths always used).
- OpenAI addendum: the one-file-per-call rule names read_file, edit_file and
write_file explicitly and allows a single multi-file apply_patch.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* test(sandbox): compare slash-normalised patch paths so the check passes on Windows
structuredPatchHeaderPaths normalises separators to "/", so the absolute
outside-workspace path in the boundary test must be compared in its slash
form; on Windows the backslash original never matched and the Smoke job
failed.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* test(sandbox): validate the parsed patch path at the scope boundary
For each accepted marker spelling, pass the exact path applyPatchRequestPaths
produced to scope.validate: an outside path must be denied and an inside
path accepted, so a separator-handling regression between the parser and the
scope cannot slip past the test.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* fix(tools): apply unified diffs in-process through the workspace root
Review follow-up for #956. Unified diffs were validated by pathname and then
handed to git apply, which reopened the targets by name — a check-to-use
window a symlink swap could exploit — and the absolute-path rewrite added in
this PR normalised `diff --git` headers with strings.Fields, which splits a
git C-quoted path containing whitespace.
Both go away by not shelling out at all: parseUnifiedPatch translates a
unified diff into the same operations a structured patch produces, and both
formats are applied by applyPatchOperations through os.OpenRoot — every
stat, read, create and write is descriptor-relative and refuses to follow a
link out of the root. git is no longer required by apply_patch.
Unified-diff support: modifications, creations (--- /dev/null), deletions
(+++ /dev/null), multiple hunks per file, git "rename from/to" (a move,
hunks optional) and "copy from/to" (a new copy operation whose destination
inherits only the tracker credit the source had), "\ No newline at end of
file" on either side, a/ b/ prefixes, C-quoted paths, CRLF patches, and
git's diff/index/mode lines. Hunks are placed at their "@@ -a,b" range when
the expected lines match there and located by context otherwise (so a
stale range still applies and duplicate blocks are disambiguated by the
range). Binary patches are rejected.
Removed: the git apply flow, relativizeUnifiedPatchPaths,
recheckPatchWriteTargets, missingPatchTargets, completeCreatedPatchTargets
and recordCreatedPatchTargets. validatePatchPaths/changedFilesFromPatch stay
for mutation-target listing.
Tests: the existing unified-diff suite now runs through the in-process
applier unchanged; new coverage for quoted whitespace paths (inside applies,
outside is refused untouched), a workspace path swapped for a symlink that
escapes the root (both formats refused, outside file unmodified), create +
delete + no-newline markers, range hint vs context fallback, rename with a
hunk, and binary/empty rejection.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* fix(tools): harden unified-diff parsing and cover the copy operation
Review follow-ups for #956 (in-process unified applier):
- A hunk that ends before its declared line counts is reported as
malformed instead of silently absorbing the next file's ---/+++ header
pair (which then failed later against the wrong file). Both shapes are
caught: leftover counts at a header or at end of input, and a --- line
directly followed by +++ inside a hunk. The element after a patch's final
newline is no longer counted as an empty context line, so a truncated
hunk cannot look complete.
- "\ No newline at end of file" may only follow a file's final hunk; a
marker before another hunk is rejected rather than leaking into that
hunk's result.
- Zero-context insertions (git diff -U0) are documented as
position-trusting by design, matching git apply without fuzz; hunks with
context are verified at the range before it is used.
Tests: copy from/to (source kept, destination created with the hunk
applied and tracked with the source's whole-file state; existing
destination refused untouched; copy onto itself refused), over-declared and
truncated hunk counts, and the no-newline marker before vs after the final
hunk.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* fix(tools): require an @@ line before treating ---/+++ inside a hunk as a file boundary
A removed "-- x" line directly followed by an added "++ y" line is valid
hunk content that also matches the ---/+++ header shape; the boundary check
now additionally requires the following line to be a hunk header, and a
regression test keeps such a pair intact. End-of-input strictness for a hunk
cut short is documented as deliberate: applying a truncated hunk would make
a partial change (removals without their replacements).
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* refactor(tools): share one line-count implementation between reader and writer
sourceLineCount and trackedLineTotal computed the same count; keeping two
copies invites exactly the reader/writer drift this PR fixed. The reader now
delegates to trackedLineTotal, which carries the tracker contract.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* fix(tools): verify a unified deletion's expected content before removing the file
A unified diff's "+++ /dev/null" hunks state the content being removed, but
the in-process applier dropped them and deleted unconditionally, so a stale
deletion removed a file whose current content the patch never described.
Unified deletions now keep their hunks, require at least one, and run them
through the same verifier as updates: the hunks must match the current file
and leave nothing behind, otherwise the deletion is refused and the file is
untouched. A structured "*** Delete File" keeps its unconditional meaning.
Hardening from the same review: every non-create change re-reads its source
through the opened root immediately before commit and refuses to proceed if
the bytes differ from what was planned.
Tests: matching deletion, stale deletion (file byte-for-byte unchanged),
partial deletion, header-only deletion, multi-hunk deletion with one stale
hunk (nothing removed) and all-good hunks, structured delete unchanged, and
a source changed between planning and commit for update and delete.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* test(tools): a move with a missing source is refused before publishing
The pre-commit recheck now refuses a move whose source vanished after
planning instead of publishing the destination and then failing; update the
test to the safer contract (error names the recheck, nothing is created).
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* fix(tools): byte-exact unified deletions and git's header-only empty-file forms
Review follow-ups for #956:
- A unified deletion is verified byte for byte: the removed lines (with the
removed side's "\ No newline at end of file" honoured) must equal the
current file exactly, with only the file's own line-ending style
normalised. None of the whitespace tolerance used to locate update hunks
applies, and a whitespace-only remainder no longer passes; a deletion is
not recoverable, so it fails closed.
- git describes an empty file's deletion or creation with header lines only
("diff --git", "deleted file mode"/"new file mode", "index") and no
hunks. Both forms are recognised: the deletion is applied only when the
current file is empty, the creation writes an empty file, and they compose
with hunk-carrying files in the same patch.
- The pre-commit recheck test also covers the copy operation and asserts no
destination is created on refusal.
Tests: whitespace/indentation/blank-line/space-only/missing-newline
mismatches all refused with the file unchanged, matching no-newline marker
and CRLF file accepted, header-only deletion of a non-empty file refused,
multi-file patch with header-only delete + create + a normal hunk.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* test(tools): remove the move source after planning via a deterministic hook
The refused-move test now creates source.txt, plans the move, and removes
the file through a test-only pre-commit hook (structuredPatchBeforeCommit)
so it exercises the post-planning disappearance path rather than a source
that was never there. Assertions unchanged: refused before publishing, no
destination created.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* test(tui): read_file fixtures use the compact N→ prefix
Three TUI fixtures still built fake read_file output in the legacy
" N | text" shape; they pass either way but pinned an outdated contract.
They now mirror what appendReadFileRange emits, and the shape comment and
the expanded-card assertion are updated.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* fix(tools): name the committed files when a patch is partially applied
Review follow-up for #956. When a later change in a multi-file patch fails
(for example the pre-commit recheck refuses a file that changed after
planning), the error now lists exactly which files were already committed
and states that the rest are unchanged, instead of a generic "partially
applied" warning. The model can then re-read only what actually changed.
Test: a three-file structured patch whose second file is altered after
planning fails with "already committed: first.txt", first.txt holds the
patched content, and the other two files are untouched.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
---------
Co-authored-by: Claude Fable 5 <[email protected]>
|
||
|
Anandan
|
d7ac85cbf9 |
Polish Zero's TUI and harden patch, model, and background workflows (#916)
* Checkpoint UI polish * Fix structured apply patch handling * Restore tool previews when resuming sessions * Improve theme-aware code rendering * Replace persistent sidebar with run details * Remove inert transcript interactions * Cover resumed edit previews * Smooth active run status feedback * Preserve semantic colors behind overlays * Normalize ChatGPT model aliases * Render staged image attachments in terminal * Polish transcript and tool feedback * Resolve rebased diff renderer conflict * Address UI polish review findings * Fix smoke test regressions * Harden patch and transcript edge cases * Bind structured patches to workspace root * Use unique structured patch temp files * Retry structured patch temp allocation * Reuse rooted patch temp allocator * Fix Windows CI process checks * Fix structured patch safety boundaries Preserve read-before-write credit only when the model already knew the full source, reject ambiguous hunk matches, and retain CRLF line endings. Add regression coverage for unsafe overwrites, positive tracking cases, duplicate matches, and Windows-style files. * Resolve plan test seam after rebase Drop the obsolete sticky-plan height helper while retaining the PR status test seams brought forward by the UI changes. * Address final review correctness gaps Bound narrow waiting labels, reject malformed plan records, preserve missing final newlines, reject out-of-order EOF hunks, clear stale tracking after failed rollback, and keep deferred tool loading aligned with model visibility. * Harden patch transactions and review edges Publish new patch targets without overwriting raced destinations, journal only completed mutations, and create replacement temps beside their targets. Reject pre-plan diagnostics, cover tool-search transcript suppression, and leave sufficient cleanup time in the Windows process fixture. * Bind patch rollback to committed versions Capture file identity and content before publication, quarantine rollback targets atomically, and refuse destructive undo when a path was replaced or modified in place. Add deterministic coverage for publication and rollback races. * Align structured patch failure semantics Use prevalidated sequential application with explicit partial-success errors instead of unsafe multi-file rollback. Retain atomic per-file publication, no-overwrite creates, unsupported-hardlink fallback, and conservative tracker invalidation. * Preserve patch context bytes Track source offsets for structured-patch context lines and reuse the exact matched source text during replacement. This keeps tolerant matching from changing indentation or mixed line endings outside the requested edit. |
||
|
|
d66ad71532 |
chore: remove dead wrapper functions and unused test seams (#905)
* chore: remove dead wrapper functions and unused test seams These five functions are unreachable from any cmd/* main even when tests are included (deadcode -test ./...), so they are genuinely dead on every platform: - TraceEventKeys: one-line alias of ParseTraceEventKeys, no callers - ValidateToolCapabilities: one-line alias of ValidateCapabilities, no callers - GetLocalDiffStats and WatchPRState: test seams in internal/tui/export_test.go that no test calls - ScanSSEData: non-context SSE scanner in internal/providers/providerio, fully superseded by ScanSSEDataWithContext The five remaining -test findings are //go:build !linux stubs (landlock/seccomp/linux_helper) that are live on Linux, so they are left in place. Co-Authored-By: cairn-code <[email protected]> * Bump Go to 1.26.6 so govulncheck and Windows smoke pass. The 1.26.5 standard library still reports GO-2026-6218 and the Punycode label check that fail required CI; 1.26.6 matches main and clears them. --------- Co-authored-by: euxaristia <[email protected]> Co-authored-by: cairn-code <[email protected]> |
||
|
|
64a783b08c |
fix(dictation): redact API keys from streaming transcriber errors (#852)
* fix(dictation): redact API keys from streaming transcriber errors Streaming dictation (Deepgram, OpenAI Realtime) injected API keys into websocket URLs/headers and echoed unredacted transport errors and server-side error payloads. Wrap those error endpoints with providerio.Redact and switch the connect-error format specifier from %w to %s so the raw error cannot leak via errors.Unwrap(). Adds regression tests asserting the API key is not present in the returned stream error for both transcribers. Co-authored-by: euxaristia <[email protected]> * fixup(dictation): address review: redact session/write errors, move redaction tests - Redact API key in OpenAI Realtime session configuration error. - Return redacted OpenAI Realtime write error from writeErrCh instead of discarding. - Move TestOpenAIRealtimeStreamTranscribeErrorRedaction and TestDeepgramStreamTranscribeErrorRedaction next to their source files. Refs #710 * fixup(dictation): sync Deepgram redaction test on CloseStream before close Address CodeRabbit nitpick: the server now drains the client's audio frames and waits for CloseStream before rejecting the connection with an API-key-bearing policy-violation close reason. This ensures StreamTranscribe observes the close reason (and redacts it) instead of failing earlier on a write error. Refs #710 * fix(dictation): preserve context.Canceled through streaming error redaction Formatting the read error with %s dropped the unwrap chain, so an Esc abort of a Deepgram or OpenAI Realtime dictation no longer matched errors.Is(msg.err, context.Canceled) in the TUI and produced a spurious error toast on top of the cancellation notice. Return the cancellation sentinel itself before redacting — it carries no key — and keep the redacted flat string for genuine failures. Adds regression tests that cancel a live stream and assert the sentinel survives. Co-Authored-By: Claude Fable 5 <[email protected]> * fix(dictation): close remaining cancellation and redaction gaps Key cancellation detection on ctx.Err() instead of errors.Is(err, context.Canceled), which was racing against writer-goroutine transport errors that could overwrite the error value right before the check. Applied consistently across both transcriber implementations. Fix goroutine leaks and a possible hang in the cancellation regression tests. * test(dictation): cover dial and startup cancel identity Add regression tests for Esc-abort during WebSocket dial and right after accept (session update / first write) so those redaction sites keep returning context.Canceled for the TUI errors.Is check. * fix(dictation): prefer cancel over a racing OpenAI realtime error Esc can cancel after conn.Read already has an error frame. Check ctx.Err() after partial callbacks and on the realtimeError path so the stream still returns context.Canceled instead of a redacted failure notice. Refs Gitlawb/zero#710 * fix(dictation): prevent auto-submit on cancelled realtime race handleDictationTranscribed suppressed context.Canceled but then fell through to the msg.submit branch. A delta -> Esc -> already-buffered realtime error race returns a nonempty transcript alongside context.Canceled, so with stt.autoSubmit on, Esc could submit the composer's restored pre-existing text instead of doing nothing. Treat a canceled result as terminal before the auto-submit path is reached, and add a TUI-level test covering the race. * test(dictation): exercise writeErrCh cancellation branch in realtime transcriber * fix(dictation): add custom key header redaction and single-pass redaction tests * fix(dictation): track dictation sessionID to drop stale completions after Esc and synchronize write cancellation test * fix(dictation): initialize non-zero session ID and filter stale session 0 messages Ensure the first dictation session in a process gets stale-message protection by initializing sessionID to 1 in newDictationController and removing msg.sessionID != 0 exemptions in handleDictationTranscribed and handleDictationPartial. Clean up unreachable redaction guards in providerio. Refs #710 * fix(dictation): redact API keys from streaming transcriber errors Streaming dictation (Deepgram, OpenAI Realtime) injected API keys into websocket URLs/headers and echoed unredacted transport errors and server-side error payloads. Wrap those error endpoints with providerio.Redact and switch the connect-error format specifier from %w to %s so the raw error cannot leak via errors.Unwrap(). Adds regression tests asserting the API key is not present in the returned stream error for both transcribers. Co-authored-by: euxaristia <[email protected]> * fixup(dictation): address review: redact session/write errors, move redaction tests - Redact API key in OpenAI Realtime session configuration error. - Return redacted OpenAI Realtime write error from writeErrCh instead of discarding. - Move TestOpenAIRealtimeStreamTranscribeErrorRedaction and TestDeepgramStreamTranscribeErrorRedaction next to their source files. Refs #710 * fixup(dictation): sync Deepgram redaction test on CloseStream before close Address CodeRabbit nitpick: the server now drains the client's audio frames and waits for CloseStream before rejecting the connection with an API-key-bearing policy-violation close reason. This ensures StreamTranscribe observes the close reason (and redacts it) instead of failing earlier on a write error. Refs #710 * fix(dictation): preserve context.Canceled through streaming error redaction Formatting the read error with %s dropped the unwrap chain, so an Esc abort of a Deepgram or OpenAI Realtime dictation no longer matched errors.Is(msg.err, context.Canceled) in the TUI and produced a spurious error toast on top of the cancellation notice. Return the cancellation sentinel itself before redacting — it carries no key — and keep the redacted flat string for genuine failures. Adds regression tests that cancel a live stream and assert the sentinel survives. Co-Authored-By: Claude Fable 5 <[email protected]> * fix(dictation): close remaining cancellation and redaction gaps Key cancellation detection on ctx.Err() instead of errors.Is(err, context.Canceled), which was racing against writer-goroutine transport errors that could overwrite the error value right before the check. Applied consistently across both transcriber implementations. Fix goroutine leaks and a possible hang in the cancellation regression tests. * test(dictation): cover dial and startup cancel identity Add regression tests for Esc-abort during WebSocket dial and right after accept (session update / first write) so those redaction sites keep returning context.Canceled for the TUI errors.Is check. * fix(dictation): prefer cancel over a racing OpenAI realtime error Esc can cancel after conn.Read already has an error frame. Check ctx.Err() after partial callbacks and on the realtimeError path so the stream still returns context.Canceled instead of a redacted failure notice. Refs Gitlawb/zero#710 * fix(dictation): prevent auto-submit on cancelled realtime race handleDictationTranscribed suppressed context.Canceled but then fell through to the msg.submit branch. A delta -> Esc -> already-buffered realtime error race returns a nonempty transcript alongside context.Canceled, so with stt.autoSubmit on, Esc could submit the composer's restored pre-existing text instead of doing nothing. Treat a canceled result as terminal before the auto-submit path is reached, and add a TUI-level test covering the race. * test(dictation): exercise writeErrCh cancellation branch in realtime transcriber * fix(dictation): add custom key header redaction and single-pass redaction tests * fix(dictation): track dictation sessionID to drop stale completions after Esc and synchronize write cancellation test * fix(dictation): initialize non-zero session ID and filter stale session 0 messages Ensure the first dictation session in a process gets stale-message protection by initializing sessionID to 1 in newDictationController and removing msg.sessionID != 0 exemptions in handleDictationTranscribed and handleDictationPartial. Clean up unreachable redaction guards in providerio. Refs #710 * fix(dictation): drop providerio widen; pin session and redaction tests Revert the Token/X-Api-Key providerio.Redact expansion (belongs in its own PR with coverage). Make Deepgram single-pass, cancel-race, and live-session partial tests pin the branches they name, and restore the ST1005 nolint on the user-facing Deepgram error string. * fix(dictation): ignore stale batch startup by session ID Gate dictationStartedMsg on session ID so a late Start result after cancel cannot reset a newer recording. Rename and comment cleanups from review; race suite recorded on the PR body. * fix(dictation): exit writers on cancel and pin dial/write redaction paths Select on ctx.Done while waiting for audio chunks so cancelled streaming sessions do not leave writer goroutines blocked on an open idle channel. Add non-cancellation redaction tests for Deepgram dial failure and OpenAI Realtime dial, session-configuration, and async writer failures. Refs #852 * fix(dictation): close the realtime socket on writer failure so a silent server cannot strand the read loop CodeRabbit review 4885680006 flagged that the OpenAI Realtime writer goroutine pushed write errors to writeErrCh and returned without closing the connection, so conn.Read stays parked forever when the peer goes silent after session setup and the error is never drained. Close the socket on both the append and commit write failure paths (CloseNow, since the close handshake itself would block on a silent peer), and add a silent-server regression test. Refs #852 Co-Authored-By: cairn-code <[email protected]> * test(dictation): cancel the silent-server writer failure test during teardown CodeRabbit review 4909044011 identified that the silent-server regression test used context.Background(), allowing its goroutine to remain blocked in conn.Read after a timeout. Give the test a cancellable context and defer cancellation so teardown always releases the blocked read. Refs #852 Co-Authored-By: cairn-code <[email protected]> --------- Co-authored-by: Claude Fable 5 <[email protected]> Co-authored-by: euxaristia <[email protected]> Co-authored-by: cairn-code <[email protected]> |
||
|
Anandan
|
04fd3c01a7 |
Improve diff viewer rendering (#902)
* Improve diff viewer rendering * Address diff viewer review findings * Fix diff viewer interaction regressions * Preserve header-like diff content * Fix diff hunk parsing and image vulnerability * Strengthen diff blank-context coverage * Fix diff rendering edge cases |
||
|
Anandan
|
2d2450e9a7 |
fix(providers): discover ChatGPT capabilities (#890)
* fix(providers): discover ChatGPT capabilities * fix(providers): preserve discovered capabilities * test(agent): cover service tier switching * fix(tui): restrict fast mode to ChatGPT |
||
|
AnandanandVasanthdev2004
|
0c1de25a04 |
Add interactive terminal companions (#887)
* Add terminal companions * Polish interactive terminal companions * Fix terminal companion edge cases * Strengthen terminal companion regression tests * Preserve config layout when selecting pets * Handle sparse pet preference layouts * fix(terminalpet): reset SGR before erasing a sixel companion On Windows Terminal the companion draws a filled black rectangle over the interface, and dragging it mangles text under and around it. It looks like a platform quirk. It is an untested code path that only Windows reaches. Sixel is the only protocol here that erases by PAINTING CHARACTERS. Kitty hands the terminal an image id and the placement goes away. A sixel is already in the cell grid, so clearRenderedImage writes literal spaces over the cells it occupied. Those spaces are ordinary characters and carry whatever SGR the TUI last set, and nothing normalised it, so every erase stamped a filled block in the ambient background. A drag erases and repaints on every pixel, so the block was continuously refreshed over the UI. It stayed invisible everywhere else because ghostty, kitty, WezTerm and iTerm all resolve to the Kitty protocol and delete by id, never reaching this code. Windows Terminal is the only mainstream terminal that lands on the sixel branch, which is why testing on Linux and macOS could not have caught it. The erase now resets SGR before painting, and uses DECSC/DECRC rather than CSI s/u because only DECSC saves the attributes this clobbers. Restoring them matters: the erase is emitted mid-frame, between styled writes the TUI has already begun. Measured before and after rather than reasoned about. Before: \x1b[s\x1b[3;41H \x1b[4;41H \x1b[5;41H \x1b[u no reset anywhere. After, the reset leads and DECRC closes. Tests assert the bytes, since bytes are what the terminal sees: SGR is reset BEFORE the first row is painted rather than merely present somewhere, the erase still covers every claimed cell so the fix cannot be satisfied by erasing nothing, and the Kitty path still deletes by id rather than acquiring the character-painting erase. Verified by mutation: restoring the old sequence fails the first of those with the exact reported symptom. Deliberately not fixed here, since both are design calls rather than defects: the erase rectangle is computed from the app's own cell maths rather than the terminal's real footprint, and the sixel path has no in-place move, so every drag pixel is a full erase and repaint where Kitty gets a cheap reposition. * fix(tui): ask sixel terminals for their cell size and erase what was painted The companion blanks a row and a column of live interface on Windows Terminal. The cause is one predicate. petPixelProtocolSupported gates two unrelated questions that only looked like one: whether pixel-precise dragging is available, and whether to ask the terminal for its cell size with CSI 16 t. It answers true for Kitty only, so on a sixel terminal the request was never sent, the reply never arrived, and petCellPixelWidth/Height stayed zero for the whole session. Everything downstream then took its fallback. petImageHeightPixels returned its blind preferredHeight of 75 pixels instead of the cell-aware branch below it, and the erase in clearRenderedImage used petImageColumns and petImageRows. Those constants describe the RESERVED area, not what was painted. Windows Terminal reports a 20x10 cell, so a 75-pixel sprite covers 4 rows and about 8 columns while the erase blanked 5 by 9, taking a row and a column of interface with it on every move. A drag repaints per pixel, so it did that continuously. Windows Terminal answers CSI 16 t, confirmed by hand: it replies ESC [6;20;10t. This was never a question of terminal support. Nothing asked. So the request now goes out for every image protocol, via a predicate separate from the dragging one, and the sixel footprint is computed from the rendered pixel height and the reported cell size rather than from the constants. It is still clamped to the reserved area, because the layout keeps that many cells clear and the drag clamps to it, so a larger computed footprint would mean erasing into live interface. Deliberately NOT widening petPixelProtocolSupported itself: that would enable pixel-precise dragging on sixel as a side effect, which is a separate feature decision rather than part of this defect. Kitty is untouched and a test pins that. There Columns and Rows are the placement REQUEST handed to the terminal, which owns the region and scales into it, so recomputing them would resize the image rather than fix an erase. Tests use the real 20x10 measurement: the sixel footprint is 4 rows not 5, Kitty keeps its requested placement, the fallback without metrics stays the reserved area rather than collapsing to zero and erasing nothing, and every image protocol asks for the measurement. * Keep Sixel companions in the reserved dock --------- Co-authored-by: Vasanthdev2004 <[email protected]> |
||
|
Vasanth T
|
2ec5c3ae6b |
fix(agent): tell the user a prefix approval leaves the sandbox (#885)
Approving a command prefix does not only stop the prompt from reappearing. shellExecutionArgsForApproval rewrites the call to require_escalated, which resolves to a nil sandbox engine, so the command runs unsandboxed on the host. The engine's escalation gate is then satisfied by the PermissionGranted flag from the approval the user just gave for the sandboxed form. The escalation itself is deliberate and guarded: proposedCommandPrefix refuses to offer a prefix when any other segment of the command is not known-safe, precisely because approval covers the whole command. The defect is that the prompt never said so. The user chose "allow command prefix for session" and got "and also run it outside the sandbox". So this discloses rather than removes. PermissionRequest gains PrefixApprovalEscalates, computed by shellPrefixApprovalEscalates from the same guards the rewrite consults, and the two prefix options append "(runs outside the sandbox)" when it is set. Both the TUI and the ACP path carry it, since an editor user decides on the same terms as a terminal user. The main test asserts the disclosure and the rewrite agree on every input rather than checking each against a fixed expectation, so the two cannot drift apart later. Verified by mutation: reverting the helper to always-false fails it, as does making the label unconditional. |
||
|
|
ff62f73b7f |
fix(secrets,worktrees): fix secret redaction leakage and prune stale worktrees (#855)
* fix(secrets,worktrees): fix secret redaction leakage and prune stale worktrees 1. Prevent trailing redaction leaks in github_token, aws_access_key_id, and google_api_key by adding trailing word boundaries and allowing variable lengths. Refine the openai_key pattern to cleanly distinguish legacy keys and modern prefixed keys (sk-proj-, sk-svcacct-) from ordinary kebab-case phrases. 2. Implement auto-pruning of zero-owned git worktrees older than 24 hours at the start of worktrees.Prepare to prevent indefinite disk space leaks. * fix(secrets,worktrees): close tail-leak edge case and worktree data-loss risk Drop the trailing \b anchor on the four secret patterns whose body class allows "-" (slack_token, google_api_key, the modern openai_key branch, jwt). \b requires a word/non-word transition, so a secret ending in "-" right before a delimiter has none, and the engine backtracked the greedy quantifier to drop that last character instead of failing the match, leaking it. The body character class already provides the real stopping boundary, so the anchor was unnecessary. Fix two issues in worktree Clean flagged in review: - Staleness was decided by the worktree directory's own mtime, which only changes when an entry is added/removed/renamed directly inside it, not when a long-running task edits existing files deeper in the tree. Clean now walks the tree and treats any recently modified entry as live, and also skips any worktree a caller has explicitly locked via git worktree lock. - baseDir ownership used a raw strings.HasPrefix, so a sibling like "<baseDir>-other" would false-match. Replaced with a filepath.Rel path-boundary check. * fix(worktrees): check exit codes on removal, fail closed on inspection errors defaultRunGit deliberately returns a nil error alongside a nonzero CommandResult.ExitCode for a failed git invocation, so the worktree remove call must check ExitCode itself instead of trusting a nil error to mean success. Route it through gitOutput, which already does that. worktreeIsStale treated an inspection failure (an unreadable file, a WalkDir error) the same as "keep walking," which can let an incompletely-inspected worktree be judged stale. Any inspection error now makes it ineligible for removal instead. * fix(secrets,worktrees): catch appended-suffix keys and scope pruning to owned worktrees The scanner's trailing \b anchors made a credential vanish entirely when followed by a word character outside its body class (an appended suffix like AKIA...EXTRA, or ghp_..._suffix): the fixed or unbounded-greedy quantifier had no valid word boundary to land on and the whole match failed, so the real secret reached the redaction output unredacted. Dropping the trailing anchors lets the body class itself stop the match, so the credential prefix still gets redacted even when noise follows it. Also recognize sk-admin- alongside sk-proj-/sk-svcacct- so OpenAI admin keys aren't skipped by the narrowed modern-key branch. Clean pruned any worktree under the caller-supplied BaseDir, but Prepare only ever creates worktrees under a per-repository zero-worktree-<repoKey> subtree of it. Scope pruning to that subtree so a worktree a user manages by hand elsewhere under a shared BaseDir is never force-removed. Also refuse to force-remove a worktree whose mtime looks stale but that still has uncommitted or untracked changes: a task can hold live work while waiting on a model, network, or user for longer than the staleness window without writing to the tree again. * fix(worktrees): make nested-activity test exercise the deep walk it claims to activePath/internal was created by the same MkdirAll as the nested pkg dir but never backdated, so it kept a fresh mtime and worktreeIsStale's walk reported "not stale" as soon as it hit that directory, before ever reaching the freshly-written file two levels deeper. The test passed without actually exercising recursion past the first directory. * fix(worktrees): lock zero-created worktrees and treat ignored files as dirty Prepare never called git worktree lock, so the entry.locked skip in Clean only ever protected worktrees a human locked by hand, never zero's own; a worktree that finished committing and sat idle (e.g. waiting on a slow model or network retry) for more than 24h looked clean-and-stale and got force-removed by the mtime+dirty heuristic alone. Lock every worktree Prepare creates so it gets the same protection. worktreeIsDirty also used git status --porcelain with no --ignored, so a worktree holding only .gitignore-matched task data (credentials, generated drafts, artifacts) reported as clean and got force-removed with --force, silently discarding it. Add --ignored so those files count as dirty too. * fix(worktrees): release Zero's Prepare lock so Clean can reclaim finished worktrees Prepare locks every worktree it creates so Clean's mtime+dirty staleness heuristic never force-removes one Zero is still using, but nothing ever unlocked it, making the automatic disk-space cleanup permanently inert. Add Release (git worktree unlock) and wire it in two ways: zero exec --worktree defers a release once its own run finishes, since that flow's use of the worktree is bound to its own process. zero worktrees prepare hands the path to a longer-lived external caller with no defined end-of-life, so a new zero worktrees release <path> subcommand lets that caller release it explicitly when done. * fix(worktrees): normalize release path, aggregate Clean errors, unlock deleted worktrees Address CodeRabbit's review on the lock-release fix: - zero worktrees release now resolves its path argument to absolute before calling Release, since git worktree unlock matches against the path git recorded at creation, not whatever directory the caller happens to be running from. - Clean now aggregates removal failures with errors.Join instead of overwriting lastErr, so multiple stale worktrees failing removal in the same pass are all reported, not just the last one. - Release falls back to options.Cwd as the git working directory when the worktree path itself no longer exists (e.g. a caller deleted a locked worktree by hand instead of releasing it first), so the orphaned lock can still be cleared. Added regression coverage for all three. * style(worktrees): align aggregation test comments to gofmt output * fix(worktrees,cli): restore reuse lease and scope unlock to owned locks - Prepare re-locks a reused worktree so Clean's staleness heuristic cannot force-remove it while the new caller is still using it; a lock already held by a live external caller is kept in place and reported through the new Result.LockAcquired field. - exec --worktree only releases the lock its own Prepare call acquired and surfaces a failed release on stderr with the affected path instead of discarding the error. - worktrees release wires the resolved workspace root into Options.Cwd so the deleted-path recovery works outside the worktree directory. - The relative-path release test derives its expected value via filepath.Abs, matching the resolution the CLI uses, so macOS /var vs /private/var spellings no longer break it. * fix(worktrees,cli): reject in-use leases, validate before cleanup, add release -C - Prepare rejects a worktree whose lock another run still holds, on both the reuse and the create-race paths, instead of handing a second live caller an unprotected shared checkout whose sole Git lock the first caller's exit would release. - The automatic stale-worktree pruning runs only after the request itself validates, so a rejected command (an invalid --name) has no destructive cleanup side effect; covered end to end with real git in both directions. - worktrees release accepts -C/--cwd naming the source repository, which the deleted-path recovery needs when launched outside the repo (the deleted worktree path is a one-way hash with no way back to its source). * test(worktrees): canonicalize test roots to physical spelling git records worktree paths in physical form, so the CI runners' symlinked (/var -> /private/var) and 8.3-short (RUNNER~1) temp spellings made Clean's containment check skip the test's stale entry and the pruning assertion fail on macOS and Windows. * fix(worktrees): preserve orphaned commits, verify release ownership, canonicalize base dir Four fixes from the latest review round: - Clean now creates a durable ref (refs/zero/orphaned-worktree/<sha>) for a detached worktree's HEAD before force-removing it, when that commit isn't already reachable from any other ref. Prepare always creates worktrees with `worktree add --detach`, so a commit made there had no ref pointing at it once the worktree was deleted, making it immediately eligible for git gc despite never having been merged/pushed elsewhere. - Clean resolves its configured base directory through EvalSymlinks before comparing it against git's reported worktree paths: `git worktree list --porcelain` reports each worktree's PHYSICAL location (resolving symlink components), so a symlinked --worktree-dir made every worktree created under it permanently unprunable. - Release now verifies path has a zero-worktree-<repoKey> ancestor directory component before running `git worktree unlock`, so it can't be used to clear the lock on a worktree a user or another tool manages by hand. The check doesn't need to know which --dir a given Prepare call used (nothing records that against a specific worktree, and the CLI never threads BaseDir through to Release) — the repoKey component is Prepare's actual ownership signature regardless of which directory it was created under. - Route the release command's printed path and exec's release-failure diagnostic through the existing CLI redaction helper, and split the worktrees help text into prepare-specific and release-specific flag sections (release only ever supported -C/--cwd, not the --name/--dir/ --json the shared block advertised). All four have regression tests confirmed to fail without their fix (two using real git worktrees, not just fakeRunner sequences). Build, vet, and gofmt clean on linux/windows/darwin. Co-Authored-By: Claude Sonnet 5 <[email protected]> * fix(worktrees): recoverable PID leases and reclaimable released worktrees Two review findings on the cleanup lifecycle: - A lock left by an abnormal exit (SIGKILL, crash, power loss) was skipped by Clean forever, recreating the permanent disk leak this PR set out to fix. exec --worktree now records its PID in the lease reason; Clean expires a lease whose recorded owner is provably dead, unlocking only after the staleness, dirty, and HEAD-preservation guards all pass. Human locks and PID-less leases (external `worktrees prepare` owners) remain permanent until explicit release, and any ambiguity in the liveness probe counts as alive. - An explicitly released worktree holding only gitignored residue (node_modules, build output) was skipped at every age. Release is the owner's completion signal, so unlocked entries now block removal only on tracked/untracked changes; expired crashed leases keep the conservative --ignored probe since they never signaled completion. Co-Authored-By: Claude Fable 5 <[email protected]> * fix(worktrees): address review feedback on lease detection and release safety Split dead-lease PID checking into posix/windows implementations so Windows can reliably tell a dead process from a live one. Fix a path canonicalization mismatch in two release tests. Derive release ownership from git worktree list instead of the git-dir parent, which was wrong for repos with a separate git-dir. Refuse to clear a lock that was not taken by Zero in the first place. * fix(worktrees): canonicalize paths for clean/release ownership checks Compare Clean containment and Release ownership against physical path spellings so macOS /var vs /private/var and symlink TMPDIR layouts match git worktree list. Require a registered porcelain entry and a Zero lease reason before unlock; treat an already-unlocked Zero worktree as a no-op. * fix(worktrees,secrets): address jatmn review findings on #632 - Prepare and Release now agree on repoKey regardless of which worktree (main or linked) Prepare runs from, by keying off git worktree list's first entry (always the main worktree) instead of --show-toplevel. A worktree prepared from a linked checkout previously failed its own ownership check on release and its lease could never be cleared. - osProcessAlive on Windows no longer treats every OpenProcess failure as "process is dead": only ERROR_ACCESS_DENIED (a live process this caller lacks rights to query) is now distinguished from a genuinely missing PID, so Clean can no longer force-remove an active worktree whose owning process it simply couldn't query. - The openai_key redaction pattern now recognizes sk-or-v1- (OpenRouter) alongside the existing sk-proj-/sk-svcacct-/sk-admin- prefixes, so hyphenated OpenAI-compatible provider keys are redacted again without reopening the sk-<kebab-case-phrase> false-positive this pattern was narrowed to avoid. - Release/exec --worktree error text is redacted before reaching stderr, matching the already-redacted success path; ownership errors interpolate the caller-supplied path, which could carry a key-shaped segment. - canonicalizePath resolves symlinks through the nearest existing ancestor when the target itself no longer exists, so the documented `release -C` recovery path works again for a worktree deleted by hand under a symlinked --worktree-dir. - Prepare rolls back the worktree `git worktree add` just created if the subsequent lock call fails for a reason other than a concurrent racer, instead of leaking an unleased checkout until Clean's 24h staleness window reclaims it. Not addressed here: the P2 finding that worktree ownership is provable only by a directory-name convention plus a lock-reason prefix, both of which a user can reproduce by hand. A durable per-worktree ownership marker would close that gap, but internal/worktrees has been on main since #70, so a marker requirement could reject worktrees an already-installed zero created before this change existed. Needs a decision on migration before implementing. * fix(worktrees): prove Prepare ownership with a git-admin marker Address review findings that path convention plus lease-reason prefix are forgeable by hand. Prepare now writes a zero-owner marker into the worktree admin dir; Release and Clean require it before force-touching a path. Clean also keys its owned subtree off the main worktree root so linked-checkout calls still prune the same bucket Prepare uses. Tests plant the marker and list the main worktree first so fixtures match production. * fix(cli): complete worktrees release in shell completions * test(worktrees,cli): add coverage for linked-worktree Clean, forged lease rejection, and completions - TestCleanFromLinkedWorktreePrunesStaleWorktree: pins Clean deriving its owned-subtree key from the main worktree root (not the invoking linked checkout's --show-toplevel) so Prepare/Clean run from a linked worktree actually reclaim the worktrees Prepare created there. - TestReleaseRejectsForgedZeroLeaseWithoutOwnershipMarker: pins the ownership-marker requirement against the exact forgery jatmn described - a worktree under the predictable zero-worktree-<repoKey> path, manually locked with a reason that merely starts with the zero lease prefix. - Fix TestPrepareCreatesDetachedGitWorktree: its fake git runner ran out of canned results at Prepare's post-lock ownership-marker write, so writeOwnershipMarker resolved gitDir to "" and os.WriteFile wrote "zero-owner" as a relative path into the test process's real working directory instead of failing loudly. Give it autoAbsoluteGitDir like the other Prepare-exercising tests use and assert on the marker-write call. - completions_test.go: assert `worktrees`/`worktree` completions include `release` alongside `prepare`. * fix(redaction,worktrees): sort extra secret values by length descending and canonicalize worktree paths * fix(secrets,worktrees): restore Anthropic key redaction and handle legacy worktree cleanup * fix(secrets): add bash output redaction regression test for Anthropic API keys * fix(worktrees): touch worktree mtime on reuse to prevent stale pruning When Prepare reuses an existing worktree, it now touches the directory to refresh its mtime. This prevents Clean from force-removing a long-running but idle worktree (e.g. waiting on a model) that has no recent file changes but is still actively in use. Addresses the data-loss risk flagged in the PR review where mtime-only staleness + --force removal could discard live worktrees. Co-authored-by: cairn-code Co-Authored-By: cairn-code <[email protected]> * fix(worktrees): properly handle os.Chtimes error on reused worktree path Co-Authored-By: cairn-code <[email protected]> * fix(worktrees,secrets): unlock using porcelain entry path, set RepoRoot to primaryRoot, and add anthropic_key pattern Unlock using matched porcelain entry path, set Result.RepoRoot to primaryRoot in Prepare, add dedicated anthropic_key secret pattern, and canonicalize fake-runner test paths. Refs #632 * fix(tools): correct redaction placeholder assertion in Anthropic key test TestFormatBashOutputRedactsAnthropicKey checked for the openai_key placeholder instead of anthropic_key, a copy-paste leftover. The redaction itself was already correct; only the assertion was wrong. * test(worktrees): physicalize test paths for macOS tempdir symlink resolution * fix(secrets,worktrees): address CodeRabbit review findings Align RedactString token boundaries with secrets.Scan, strengthen the Anthropic bash redaction assertion, fix Clean fixtures so they reach the guards under test, and fail closed on ambiguous processAlive probes. * fix(secrets,worktrees): restore broad key redaction and legacy Clean safety Address human review on #855: keep sk- bodies with a digit filter instead of enumerated vendor prefixes, add a looser JWT form, restore the sk-test fixture, probe legacy ownership before dirty, treat non-INVALID Windows OpenProcess errors as alive, redact Abs/cwd release errors, and reclaim dead-owner leases on Prepare reuse. * fix(secrets,redaction): always redact known OpenAI key prefixes Alphabet-only sk-proj-/sk-svcacct-/sk-admin- tokens are still credentials; keep the digit filter only for unknown sk- vendor forms so kebab phrases like sk-learn-… stay un-redacted. * Preserve digit-free legacy keys during redaction. Refs #855 * fix(secrets,worktrees): address CodeRabbit findings on PR #855 Redact full compact JWE tokens, pin git locale for lock parsing, write the ownership marker atomically, surface missing-dir unlock failures, and fail closed when HEAD probes are indeterminate. Align Clean fixtures and digit-free known-prefix redaction tests with the shipped behavior. Refs #855 --------- Co-authored-by: Claude Sonnet 5 <[email protected]> Co-authored-by: cairn-code <[email protected]> Co-authored-by: euxaristia <[email protected]> |
||
|
|
0176ee49ff |
feat(agent): add PermissionModePlan for interactive read-only planning (#853)
* feat(agent): add PermissionModePlan for interactive read-only planning * fix(agent): drop redundant modeName branch, add plan mode regression tests string(permissionMode) already yields "plan" / "spec-draft", so the if/else recomputing modeName in the denial message was dead branching on the same values. Also add plan-mode coverage mirroring three of the four existing spec-draft regression tests: advertised tool set, and denied write_file/bash calls. The fourth (submit-and-stop review control) has no plan-mode analog, since plan mode has no submit tool. * fix(agent): deny request_permissions in plan/spec-draft even when the registry omits it request_permissions is dispatched by name in executeToolCall before the registry-based ToolAdvertised gate runs, so that gate only helps when the tool happens to be present in the caller's registry. A plan- or spec-draft-mode registry that simply omits the tool (rather than registering it as denied) let the call fall through to a real turn/session-scoped permission grant, defeating the read-only boundary. Deny it unconditionally at the top of executeRequestPermissions for both read-only modes instead. Co-Authored-By: Claude Sonnet 5 <[email protected]> * fix(agent): suppress executable hooks while plan/spec-draft mode is active Plan mode promises a read-only turn, but sessionStart/sessionEnd fire on every run and beforeTool/afterTool fire around allowed read calls, and all four execute configured host commands outside the advertised-tool and sandbox gates — so a project hook could mutate the workspace or spawn a process from a session that advertises it cannot. Gate all four dispatch points on the run's permission mode, with a regression test asserting no hook command launches during a plan-mode run. Co-Authored-By: Claude Fable 5 <[email protected]> * fix(agent): close plan-mode tool advertisement bypass toolAdvertisedInPlan whitelisted ask_user and update_plan by name alone, so a caller could register a mutating tool under either name and have it advertised and executed in plan mode. Validate every tool against its Safety() instead. Also exclude lsp_navigate, which is marked SideEffectRead but lazily spawns a real language-server process, contradicting plan mode's read-only guarantee. Add regression tests for both. * fix(agent): keep trust-gated hooks in spec-draft mode Plan mode still suppresses executable hooks so a read-only planning turn cannot spawn host processes via session or tool hooks. Spec-draft keeps the existing trust model: project hooks fire when the workspace (or its worktree trust root) is trusted. Unconditionally suppressing hooks in spec-draft broke TestExecSpecWorktreeInheritsTrustEndToEnd for trusted worktrees under --use-spec --worktree. * fix(agent): cover plan-mode spoof and lsp_navigate denials Expand the name-only spoof regression to both update_plan and ask_user, and add an execution-path denial for lsp_navigate so plan mode cannot spawn language servers even when a call still arrives. * fix(agent): filter tool_search deferred candidates by plan/spec-draft visibility tool_search resolved and ranked deferred tools by EnabledTools/DisabledTools only, never by the run's permission-mode visibility. tool_search itself is already denied at dispatch in plan/spec-draft (its Safety carries no side effect, so it fails the same SideEffect==Read advertisement gate direct calls use), so this was not reachable through the normal Run() path today. But it is a real landmine: if that outer gate is ever loosened independently (e.g. tool_search's no-side-effect Safety is judged advertisable), the loader had no gate of its own and would hand a deferred write/mutator tool's name, description, and full schema straight to a plan/spec-draft model. Mirror agent.ToolAdvertised's plan/spec-draft branches inside the tools package (toolAdvertisedForPermissionMode, next to the existing toolAllowedByFilters mirror that avoids the same import cycle) and apply it alongside the operator filters in visibleDeferredTools and visibleEagerToolNames. Added unit tests in tool_search_test.go for both modes, and an end-to-end agent test that force-calls tool_search in plan mode and asserts no schema leaks. Verified by reverting tool_search.go and confirming the new tests fail (one shows load_tools resolving to the mutator's name); restored and confirmed they pass. Also confirmed via a temporary probe that if plan mode's outer advertisement gate is loosened, this filter is what actually stops the leak. * fix(agent): require Safety for spec-draft ask_user/submit_spec Do not advertise or load re-registered control tools by name alone in spec-draft mode. ask_user must be SideEffectRead+Allow and submit_spec must be SideEffectWrite+Allow, matching the real tools. Apply the same filter in tool_search and add spoof regression tests. Refs Gitlawb/zero#642 * fix(agent): wire plan mode into TUI, CLI, and ACP entry points Address the P1 finding that PermissionModePlan was documented but never selected by /plan, zero exec, or ACP mode selectors. /plan on|off now toggles the session permission mode (restoring the prior mode on off), zero exec --plan selects plan for a run, and ACP advertises plan as a client-selectable mode. Integration coverage for each entry path. * fix(cli): reject --plan combined with --worktree Worktree preparation runs in runExec before the plan permission mode is assigned, so `zero exec --plan --worktree` could still trigger workspace mutation ahead of the read-only gate. Reject the combination during option validation, alongside the existing --use-spec/--skip-permissions- unsafe conflict checks, so no worktree prep can occur. Addresses a coderabbitai finding on PR #642. * test(tools): assert spoofed tool schema doesn't leak via tool_search The spoofed-control-tool regression only asserted on the description string; Parameters() exposed no distinctive schema marker, so a regression that leaked the schema without the description would still have passed. Add a spoofed_secret property to the test tool's schema and assert it's absent from result.Output alongside the description. Addresses a coderabbitai finding on PR #642. * fix(tui): gate local mutating commands behind plan mode /plan on only flips the agent permission mode, which gates agent tool calls. Local TUI commands that run entirely inside the TUI process bypass that gate: /rewind restores workspace files from a checkpoint, /export writes a transcript to disk, and /sandbox-setup spawns a native host process. Add a shared plan-mode guard at the start of dispatchCommand (mirroring the existing BTW-unavailable guard) that rejects these three commands while permissionMode is agent.PermissionModePlan, with regression coverage proving each is blocked with no mutation/process spawn in plan mode and unaffected outside it. Addresses a coderabbitai finding on PR #642. * fix(agent,specialist): layer plan mode system prompt and enforce read-only subagent mode * Fix jatmn review findings for PR 642 * fix(agent,cli,tui): resolve CodeRabbit review comments on active turn model field and --plan permission mode conflict * fix(agent): propagate permission mode to exec options and serialize ACP mode changes Parse permissionMode in resolveExecPermissionMode, acquire turnMu.Lock in ACP handleSetMode to serialize mode changes with active turns, and block MCP subcommands in TUI plan mode. Refs #642 * fix(agent,tui): update tests off removed tools.CoreTools/NewWriteFileTool/NewLSPNavigateTool wrappers Those were thin unscoped wrappers around the Scoped variants, deleted upstream in #706 since nothing else called them directly. Only these tests still did; switch to the Scoped calls main's own tests already use. * fix(agent): fail-closed beforeTool vetoes and permission-mode plan guards Keep beforeTool deny gates active under plan mode so hooksSuppressed no longer fails open, and stop propagating --permission-mode for auto/ask/member children so swarm members keep write tools. Apply --plan combination rejects to --permission-mode plan as well. Refs #853 * fix(agent): address CodeRabbit findings for plan-mode advertisement and entry paths Unify plan/spec-draft tool advertisement in tools.ToolAdvertisedForPermissionMode (with PermissionDeny short-circuit), cover ACP config and --permission-mode plan list-tools paths, and allow bare /mcp while blocking mutating MCP subcommands. Refs #853 --------- Co-authored-by: Claude Sonnet 5 <[email protected]> Co-authored-by: euxaristia <[email protected]> |
||
|
Anandan
|
160e3bed1a |
Unify tool output views (#880)
* Unify tool output views * Preserve finalized tool outcomes * Make outcome tests portable |
||
|
Vasanth T
|
7fbd864e91 |
fix(tui): fall back to CellMotion where AllMotion is unreliable (#872)
* fix(tui): fall back to CellMotion where AllMotion is unreliable AllMotion (1003) buys exactly one thing over CellMotion (1002): hover highlighting. Wheel, click and drag all arrive under either. But a terminal that does not implement 1003 does not degrade to 1002, it sends nothing at all, and because the TUI holds the alternate screen the terminal's own scrollback and wheel are gone too. The user is left unable to scroll by any means, and a long answer then reads as a hang rather than as a missing highlight. That is how #870 arrived: a completed turn with "59 more" lines below the fold, reported as a freeze. The existing code already conceded 1003 is fragile by carving out PRoot. The legacy Windows console is the same case and was not covered, so a bare zero.exe outside Windows Terminal asked for a mode its terminal does not answer. Move the decision into mouseModeFor, which trusts terminals that identify themselves (WT_SESSION for Windows Terminal, TERM_PROGRAM for hosts like VS Code and mintty) and assumes an unidentified Windows terminal is the legacy console. Guessing wrong that way costs a hover highlight; guessing wrong the other way costs every mouse event. ZERO_MOUSE_MODE=all|cell|off overrides it, so a user on a terminal we guessed wrong about can fix it without waiting for a release. An unrecognised value is ignored rather than treated as "off", so a typo cannot silently kill the mouse. Refs #870. * test(tui): pin the inherited-environment limit of the mouse-mode guess WT_SESSION and TERM_PROGRAM are inherited, so they answer for whichever process set them rather than for the console host attached now. A shell started from Windows Terminal or Git Bash that later runs zero.exe against the legacy console still carries them, and mouseModeFor then asks for AllMotion on a host that may drop every mouse event: the #870 failure class through a narrower door. Not fixed, and deliberately so. Telling an inherited variable from a live one needs the console host rather than the environment, and erring the other way costs every Windows Terminal user their hover highlighting for a case that takes an unusual launch path to reach. So the limit is written down in the code and pinned by a test that also asserts ZERO_MOUSE_MODE=cell still overrides it, since that override is the only recourse an affected user has. If real host detection lands later the test fails by design, and its message says to delete it. Raised by @jatmn on #872. |
||
|
euxaristia
|
020c28eaf9 |
fix(tui): fill alt-screen frame with theme panel background (#850)
* fix(tui): fill alt-screen frame with theme panel background Blank and cleared cells fell back to the terminal's own default background instead of the theme's panel color, flashing white on light themes during resize or when ClearScreen erases the frame. Wrap the rendered content in zeroTheme.panel sized to the full frame so every cell is explicitly painted. * fix(tui): apply theme panel content to tea.View after render tea.NewView stores the original string; reassigning the local content variable left the view unchanged. Call SetContent after the panel render so the alt-screen frame uses the themed output. |
||
|
Anandan
|
f922cb378e |
Add cross-session messaging (#884)
* Add cross-session messaging * Harden cross-session messaging Authenticate sender metadata against live registry records, bound peer lifecycle state, preserve records across transient discovery failures, and make receipt delivery direct and bounded. Also serialize TUI peer approvals, harden platform runtime paths and transport deadlines, widen session references, and add regression coverage for identity spoofing, queue limits, path boundaries, and tool failures. * Support canonical macOS socket paths Resolve Unix transport directories to their physical path before applying strict no-symlink traversal, allowing the standard /var to /private/var macOS alias without weakening runtime-directory validation. * Fix macOS socket path regression test Create the canonicalization fixture beneath a guaranteed short Unix path so it exercises the primary endpoint on macOS, and report temporary-directory cleanup failures. * Harden peer messaging lifecycle * Fix Windows peer directory traversal * Handle Windows token-owned peer directories * Accept equivalent Windows ACL masks * Build Windows peer ACL explicitly * Pin Windows ACL trustee SIDs * Validate Windows peer ACL semantics * Propagate Windows peer handle cleanup errors * Use null handle for Windows close failure test * Test held-message eviction callback |
||
|
Anandan
|
ff608c7834 |
Reduce compaction context while preserving task state (#873)
* Reduce compaction context while preserving task state * Fix durable compaction evidence identity * Preserve structured tool errors during compaction * Unify manual and automatic compaction * Report compaction truncation structurally * Preserve automatic compaction metadata |
||
|
KRATOS
|
cae0269e77 |
fix(tui): a copied NUL byte no longer panics the TUI on Windows (#876)
syscall.StringToUTF16 PANICS rather than returning an error when the string contains a NUL — documented standard-library behaviour — and github.com/atotto/clipboard's Windows path calls it directly. So one NUL anywhere in a copied transcript selection killed the whole program: "zero: tui error: program was killed: program experienced a panic", reported from a real Windows machine. The NUL reaches the clipboard because ansi.Strip removes escape SEQUENCES and leaves C0 bytes untouched, and the transcript carries them from two independent directions: tool output (a binary read, git's -z NUL-separated listings — which this repo itself parses in files_git_sweep.go) and Zero's own card protocol, whose row prefixes are literally "\x00command-card\x00" and "\x00plan-card\x00". Sanitised at the WRITE rather than at each source, because the sources are unbounded: any tool result a user can select from is one, and a fix per source is a fix the next tool re-breaks. The same pass protects the OSC52 fallback, where a stray control byte terminates the escape sequence early and silently corrupts the copy instead of crashing — which is likely why this went unnoticed off Windows, where the identical NUL produces a bad copy rather than a dead process. \n and \t survive: a multi-line selection is the normal case and both are legal clipboard content. Fixes #875 |
||
|
|
8e266797a8 |
perf: improve harness context efficiency (#838)
* Improve harness context efficiency
* Address harness efficiency review findings
* Make recaps idle and cancellable
Delay recap generation until the session has been inactive, orient it around the active goal and next task, cancel stale work on activity, and render the result ephemerally above the composer.
Also strengthen the formatter safety regression to exercise formatter-modified edit output.
* Track every replace-all edit span
Translate each replacement into its post-edit line range, including cumulative shifts from earlier replacements, so follow-up edits remain authorized for every model-supplied change.
* Make review regressions cross-platform
Verify replace-all tracking through the canonical edit tool path instead of a platform-sensitive temp path, and cover cancellation when recaps are disabled during generation.
* fix(tools): derive SeenWhole from the recorded ranges
SeenWhole was a flag set only by a SINGLE read covering the file, while
SeenRange judged coverage from the accumulated ranges. The two then
disagreed about identical reads: after reading lines 1-50 and 51-100 of a
100-line file, SeenRange(1,100) was true and SeenWhole was false.
That matters because write_file gates overwrites on SeenWhole. A file past
the read byte budget cannot be read whole in one call — the read is
truncated and RecordSeenRange is correctly skipped, since the model did not
see those lines — so no sequence of reads could ever set the flag, and the
overwrite was refused permanently with:
the intended change depends on content that has not been read exactly
in this session. Read the affected lines with read_file, then retry.
Advice that could not change the answer. edit_file was unaffected because it
gates on SeenRange, so the escape was to edit rather than rewrite, but a
model following the error text would loop.
SeenWhole is now computed from the ranges against the file's recorded line
count, so scoped reads that together cover a file make it writable and the
error text becomes actionable. The observation carries that line count, and
a read reporting a different one clears the accumulated ranges: they
describe a shape the file no longer has.
Coverage is judged by merging sorted ranges rather than walking every line
against every range, which the previous SeenRange did — same answer, and it
no longer scans line-by-line over a large file.
Co-Authored-By: Claude Opus 5 <[email protected]>
* test(tools): key the large-file test on the resolved path
read_file resolves the workspace root through EvalSymlinks before recording
anything, so the tracker is keyed on the resolved path. The test asserted
against the raw t.TempDir() path, which is the same string on Windows and a
different one on macOS, where the temp dir sits under /var — a symlink to
/private/var. The SeenWhole assertions would have looked up a key that never
existed: passing here, failing on macOS CI.
Also switches the line generator to fmt.Fprintf (staticcheck QF1012).
Both from CodeRabbit on #842.
Co-Authored-By: Claude Opus 5 <[email protected]>
* test(tools): actually switch the line generator to fmt.Fprintf
The previous commit's message claimed this was done. It was not: the edit
silently did not apply and I reported it fixed without re-reading the file,
so builder.WriteString(fmt.Sprintf(...)) survived at the head — caught by
@anandh8x, not by me.
Verified this time by running the linter rather than by reading the diff:
golangci-lint --enable-only unused,ineffassign,staticcheck ./internal/tools/
reports 0 issues.
* fix: keep exact-read workflows recoverable
* test: cover permission normalization wiring
* fix: commit file observations after output budgeting
* fix: defer file observations across retries
* test: normalize patch line endings
---------
Co-authored-by: Vasanthdev2004 <[email protected]>
Co-authored-by: Claude Opus 5 <[email protected]>
|
||
|
Anandan
|
588635487f |
feat(tui): replace /retitle with local /rename (#826)
* Improve session naming and resume picker * Address session rename review feedback |
||
|
Anandan
|
845df6c7d2 |
fix(tui): hide unavailable sidebar shortcut (#821)
* fix(tui): hide unavailable sidebar shortcut * test(tui): cover medium sidebar footer hint |
||
|
Anandan
|
15eb16f736 |
fix(tui): exclude permission waits from turn time (#820)
* fix(tui): exclude permission waits from turn time * fix(tui): exclude permission waits from TTFT |
||
|
Vasanth T
|
ec3b822505 |
fix(tui): paste clipboard images with Ctrl+V (#534) (#807)
* fix(tui): paste clipboard images with Ctrl+V Ctrl+V did nothing when the clipboard held a screenshot, while right-click paste attached it. The image plumbing was already there: routePaste probes the OS clipboard for an image whenever a paste arrives with empty text. The gap is that a clipboard holding only an image produces no bracketed paste at all, since the terminal has no text to send, so routePaste never runs and its probe never fires. Right-click reached the probe only because that path always delivers a clipboardReadMsg, empty or not. Bubble's own Ctrl+V binding stays disabled, so nothing else was listening for the key. Handle Ctrl+V by issuing the image probe directly. It reads image content only, never text, so bracketed paste remains the single text path and a Ctrl+V with text on the clipboard is unaffected. The probe yields no message when there is no image, keeping it a silent no-op, and it is gated on noBlockingModal so a permission prompt or picker still swallows the key. readClipboardImage becomes a var so the tests stub it instead of depending on the machine's real clipboard. Fixes #534 * fix(tui): also probe the clipboard on Cmd+V, and test the real key route Two things raised in review, both fair. The handler matched keyCtrl only, which is ModCtrl. macOS reports Command as ModSuper, as the rest of the TUI already models it, so Cmd+V never reached the image probe even though the change claimed to cover it. It now matches both, via a keySuper helper alongside the existing modifier helpers. Worth being precise about the limit: this only helps where the terminal delivers the key to the application. A terminal that handles Cmd+V itself pastes the clipboard TEXT and sends no key event at all, so an image-only clipboard still produces nothing for this to react to. That is a separate problem from the one being fixed here. The tests also did not exercise the route they were named for. One asserted that Update returned some non-nil command and then ran a freshly built probe; the other never called Update at all. Both now stub the reader BEFORE Update and run the command Update actually returned. That matters more than it sounds. Making the branch return a different non-nil command leaves the old assertion passing and fails the new one, which is exactly the hole: the old test proved a command was issued, not that it was the image probe. Removing the route entirely, or reverting to ModCtrl only, also fails now. * fix(imageinput): report an unreadable clipboard image instead of silence On macOS the clipboard reader shells to pngpaste or a PyObjC one-liner, and neither ships with the OS: pngpaste is Homebrew-only and Apple dropped the bundled PyObjC. A stock Mac therefore fails every time, and the failure was returned as (nil, nil), which the caller reads as "no image on the clipboard" and treats as a silent no-op. Pressing Ctrl+V produced nothing at all: no image, no error, no hint that anything was missing. The darwin path already knows the difference. It probes with `clipboard info` first and only reaches the extraction step once the clipboard is confirmed to hold an image, so a failure there is not absence, it is "there is an image and nothing here can read it". That now returns ErrClipboardImageUnreadable, which the existing error path turns into a transcript notice naming what to install. The defect predates this branch, but this branch is what makes the path reachable from Ctrl+V, so shipping without it would close #534 with a feature that is inert on one of the three platforms. Raised by gnanam, who measured it on a real Mac rather than inferring it. Deliberately narrow. The Linux arm cannot tell "no tool" from "no image" without probing separately, and Windows uses PowerShell, which is always present, so neither is touched. The test drives the whole path: stubbed reader returns the error, Ctrl+V issues the probe, and the assertion is that the failure reaches the transcript as an error row rather than stopping earlier. Swallowing the error again makes it fail. * test(imageinput): cover the darwin unreadable-clipboard decision itself The test added with the fix asserted around the change rather than on it. It substituted ReadClipboardImage with a stub returning the error, which proves the TUI surfaces an error handed to it, but never reaches readClipboardImageDarwin, where the decision lives. Reverting both production return sites to nil,nil left every package green, which is the definition of a test that is not testing the change. Caught by gnanam. readClipboardImageDarwin shells out twice and had no seam, so the two steps are now package-level vars, the same shape as the readClipboardImage var this change already relied on. That makes the case that matters reachable: the info probe succeeds and reports an image, and the extractor then fails. It cannot be arranged from outside the function, because from there both steps are one opaque call. Five cases, and the negative ones carry as much weight as the positive. An image with no working extractor, and an extractor that exits 0 with no bytes, must both be actionable. A clipboard offering no image, and a failing info probe, must both stay silent, because turning those into a notice would make every stray Ctrl+V nag the user. The extractor must not run at all in either silent case. Repeating gnanam's experiment now fails: reverting both return sites to nil,nil fails the two actionable cases and leaves the silent ones passing, which is exactly the discrimination the fix is supposed to add. |
||
|
Anandan
|
5d1869e7bc |
feat: add persistent session goals (#803)
* feat: add persistent session goals * Fix goal run isolation and guards * Strengthen loop goal isolation test * Cover goal edit state transitions * fix(goal): bound automatic continuations Persist a provider-independent continuation limit, require approval for model-created goals, and keep resume, BTW, and loop runs from starting or charging unrelated goal work. Also reject trailing lifecycle arguments and bound persisted objective length. * fix(goal): resume continuations after btw |
||
|
Anandan
|
518d2e53ba |
fix(windows): use PowerShell for command execution (#804)
* fix(windows): prefer PowerShell for command execution Detect PowerShell 7 or Windows PowerShell before falling back to cmd.exe, and share the selected host shell across agent tools and the interactive shell escape. Configure UTF-8 output, preserve restricted-sandbox MSYS safeguards, and add Windows coverage for pipelines, workspace writes, and interactive stdin sessions. * test(windows): align shell coverage with PowerShell Make Windows command fixtures and guidance assertions follow the selected host shell, preserve cmd fallback coverage, and use a genuine MSYS executable when PowerShell aliases are available. Remove the unsupported non-TTY PowerShell stdin assertion; Windows interactive stdin requires a separate PTY transport. * fix(sandbox): classify Windows executable suffixes Normalize .exe, .cmd, .bat, and .com command names before safety and network lookup so PowerShell invocations such as curl.cmd receive the same network permission handling as curl. Add analyzer and engine-level regressions for the Windows PowerShell command shape observed in CI. * fix(sandbox): normalize absolute Windows command paths Extract drive-letter and UNC basenames before removing shell escape characters so fully qualified Windows executables continue matching canonical safety and network classifications. Cover both drive and UNC curl paths with analyzer regressions. * fix(sandbox): normalize relative Windows paths Recognize drive-relative, explicit relative, and suffixed backslash executable paths before generic shell escape removal so Windows commands continue to match canonical sandbox risk entries. * fix(windows): harden PowerShell execution * fix(sandbox): classify PowerShell removal aliases |
||
|
|
583e6530bd |
fix(lint): resolve static analysis baseline (#769)
* fix(lint): resolve static analysis baseline * fix(lint): clear linux staticcheck findings Amp-Thread-ID: https://ampcode.com/threads/T-019fa018-b2ee-7442-8a31-80b2cf9dd270 Co-authored-by: Pierre Bruno <[email protected]> --------- Co-authored-by: Amp <[email protected]> |
||
|
Anandan
|
a4a805a6c8 |
fix(tui): reset quiet clock for new runs (#799)
New turns inherited lastStreamActivity from the prior turn, so a fresh request could immediately look stalled. Initialize stream activity from the same timestamp as turnStartedAt and cover the stale-clock case with a deterministic regression test. |
||
|
Anandan
|
9fe02ae52e | fix(sandbox): clarify scoped permission prompts (#798) | ||
|
euxaristiaandClaude Sonnet 5
|
97ef90d27a |
chore: remove code unreachable on every supported platform (#706)
* chore: remove code unreachable on every supported platform The Security and code health job's advisory deadcode step reports 152 unreachable functions on main. Running the same check under GOOS=linux, windows, and darwin shows 13 of those are false positives whose callers live in windows build-tagged files, so only functions unreachable on all three platforms are touched. Functions with no references anywhere are deleted, including the internal/reasoning package, which nothing imports. Functions that only tests reference move into per-package export_test.go files so tests keep their seams while the production binary sheds the code. Test call sites of deleted convenience wrappers are ported to the live APIs (scoped tool constructors, tools.DeferredSource, FinishReason checks). Kept despite the report: cron.Store.Runs (only reader of runs.jsonl, which production writes), and agenteval.TraceEventKeys and MissingTraceEvents (used across packages by the trace contract test). * docs(config): correct ValidateFile's errors.As claim Message is a flattened string (.Error() already called on the parse error), not a wrapped error, so callers can't recover the concrete json error type via errors.As. Fix the comment to match. Co-Authored-By: Claude Sonnet 5 <[email protected]> * chore(trace,skills): drop obsolete Sink adapters and fix stale doc references The dead-code pass removed the Emit methods but left Sink, NDJSONSink, and TextSink exported with a contract nothing implements; remove the types with their contract. Also remove the stray blank line at EOF of trace.go flagged by gofmt, and update skills package docs and HOW_ZERO_WORKS.md to stop directing callers to DiscoveryRoots/Duplicates, which are now test seams. Co-Authored-By: Claude Fable 5 <[email protected]> * chore: clean up remaining review nits on dead code removal Replace a pointless fmt.Errorf(...%w...).Error() with fmt.Sprintf using %v, since the wrapped error was discarded immediately anyway. Remove a comment block that was duplicated verbatim. * docs(skills): drop remaining references to test-only List/Get Load and MergedSkills comments still named List/Get as production entry points after they moved to export_test.go. Point callers at Load and ListFromRoots instead. * chore: remove now-unreferenced export_test.go seams execSessionManager.len() and likelySandboxDenied/sandboxDenialKind had no remaining callers after the sandbox unify-command-execution rebase; the production code they wrapped for tests no longer exists. --------- Co-authored-by: Claude Sonnet 5 <[email protected]> |
||
|
PierrunoYTandClaude Sonnet 5
|
5c58256abc |
fix(config): kill provider-command process tree via job object on Windows (#690)
* fix(config): kill provider-command process tree via job object on Windows taskkill /T walks the process tree by parent PID in user space and can miss descendants, leaving the sleeping child holding the inherited stdout/stderr pipes so cmd.Wait() blocked until natural completion (~10s instead of the 5s timeout). Assign the started command to a job object with JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE and terminate the job on timeout so the kernel kills the whole tree atomically; taskkill remains as a fallback. Also set cmd.WaitDelay so Wait can never hang on pipes held by an escaped orphan, and add a regression assertion that the sleeping child is actually gone after the timeout returns. Fixes #683 Co-Authored-By: Claude Fable 5 <[email protected]> * fix(config): harden provider-command tree termination Address code review on PR #690: terminate the process tree when cmd.Wait() returns exec.ErrWaitDelay (a background descendant kept inherited pipes open, previously left running); start the Windows process suspended and assign it to the job object before resuming its main thread, closing the window where a fast child could escape the job; drop JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE so a successful command's detached helpers survive Close() instead of being reaped; escape single quotes in generated PID-file paths; and correct a misnamed Windows exit-code constant in a comment. Co-Authored-By: Claude Sonnet 5 <[email protected]> * fix(config): avoid gofmt smart-quote rewrite in doc comment gofmt's doc-comment formatter rewrites a trailing '' into a curly closing quote, which CI's gofmt -l check flags as unformatted. Reword to avoid the pattern instead of embedding a raw quote pair. * fix(config): terminate leaked descendants on nonzero exit, avoid PID reuse Terminate the process tree on any error returned from cmd.Wait, not only exec.ErrWaitDelay. Go only returns ErrWaitDelay when the shell itself exited successfully; a nonzero exit (e.g. sleep 600 & exit 7) yields the bare *ExitError instead, so a detached background child that held the inherited pipes open was never terminated on that path. Also stop falling through to the taskkill/PID fallback once a job handle successfully contained the tree: by the time Terminate runs on the ErrWaitDelay path, cmd.Wait has already reaped the root process, so its PID may have been reused by an unrelated process, and taskkill /T /PID against a reused PID would force-kill the wrong tree. Addresses PR #690 review feedback. * fix(config): surface suspended-thread resume failures instead of hanging resumeMainThread now reports whether it actually resumed at least one thread. If Toolhelp/OpenThread/ResumeThread all fail, the launched process would otherwise sit suspended forever, never exit on its own, and get misreported as a plain 5s provider-command timeout instead of the real resume failure. attachCommandProcess now terminates the process immediately in that case. Also documents the known, non-regressive limitation that the taskkill fallback (used only when job creation/assignment itself fails) can still miss a descendant that has reparented away from the tree it walks by PID. Addresses remaining PR #690 review feedback; PR description corrected separately to drop the outdated JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE claim. * fix(config): guard taskkill with retained process identity * fix(config): retain POSIX process group identity * test(tui): match provider wizard shortcut * test(tui): pin credential-step env for deterministic footer assertion The credential step's footer only shows the /→ shortcut once a key is present, whether typed or inherited from the provider's AuthEnvVars. The previous commit changed the assertion to "Enter/→ continue" without accounting for that, so the test only passed by coincidence when ANTHROPIC_API_KEY happened to be set in the environment — failing deterministically on a clean CI runner (as smoke reported on all three OSes). Pin the env var empty and assert the no-key footer text so the test is correct regardless of ambient environment. Co-Authored-By: Claude Sonnet 5 <[email protected]> * fix(config): preserve provider process attributes * test(config): keep SysProcAttr coverage portable --------- Co-authored-by: Claude Fable 5 <[email protected]> |
||
|
euxaristiaandClaude Fable 5
|
5bb1bc11c3 |
fix(tui): fix rendering corruption over multipass + Windows Terminal (#709)
* fix(tui): disable bubbletea hard scroll optimization unconditionally Vendors a patched charm.land/bubbletea/v2 (via a local go.mod replace) with hard scroll optimization (using terminal scroll-region sequences to shift existing lines instead of repainting) disabled unconditionally, instead of only on GOOS=="windows". That GOOS check only catches a Windows terminal driving the program directly. It misses a Linux process running inside a multipass VM whose output is ultimately displayed by Windows Terminal over a remote shell, which corrupts rendering badly (garbled/overlapping text, worse on resize) the same way the check exists to prevent. There's no reliable way to detect that remote hop from inside the VM, so scroll optimization is now off in all cases. * fix(tui): force redraw on streamed newlines to clear stale caret The streaming caret (appendStreamingCursor) is appended to whichever visual line is currently last. Over a remote terminal chain (multipass VM output displayed by Windows Terminal), the renderer's incremental cell diff sometimes fails to clear the caret's old cell when a newline moves it to a new line, leaving ghost carets behind. Fire tea.ClearScreen when a streamed delta contains a newline: that's exactly the moment the caret moves and the risk exists, so it forces a full redraw right then instead of leaving it to the incremental diff, without doing so on every animation tick. * chore(tui): trim upstream repo metadata from vendored bubbletea fork The vendored bubbletea-v2-patched module is ~99% byte-identical to upstream charm.land/bubbletea/v2 v2.0.8; only cursed_renderer.go differs (one line disabling hard scroll optimization). The .github workflows, README.md, UPGRADE_GUIDE_V2.md, Taskfile.yaml and CI config files are upstream repo metadata that serve no purpose inside this repo, so remove them to slim the PR. The Go source and testdata golden files are kept because Go's replace directive requires the full module to be present and buildable. Refs #709 * fix(tui): address review round on the rendering workaround and vendored fork Fork correctness fixes (all flagged by review): - key.go: KeyMediaRecord repeated the previous const expression and decoded record presses as previous-track; map it to uv.KeyMediaRecord. - mouse.go: MouseButton11 likewise collapsed onto MouseButton10. - termios_unix.go: BSDLY/BS0 are output-delay bits; read them from Oflag instead of Lflag so useBackspace is set correctly. - cursed_renderer.go: count wrapped rows with (lineWidth-1)/w so an exact terminal-width multiple no longer inserts a blank line; compare colors by normalized RGBA via a nil-safe helper instead of interface equality (which panics on non-comparable implementations); and make clearScreen invalidate lastView so the queued erase flushes with the next frame instead of deferring until an unrelated view change — the reordering that could leave the ghost caret in place. - exec_test.go: implement reset() on the spy renderer and inject it before Run; assert exec preserves renderer state (releaseTerminal with reset=false). - logging_test.go: restore the default logger's output/prefix/flags and fail fast on setup errors. - tea_test.go: tie the Quit-pumping goroutine to a channel closed when Run returns instead of leaking it until process exit. TUI scoping (review P2): the forced redraw on streamed newlines is now rate-limited to ~10/s so heavy streaming output no longer turns every newline into a full-screen repaint, and ZERO_NO_STREAM_CLEAR=1 opts a correctly-rendering terminal back into the fast incremental path. Co-Authored-By: Claude Fable 5 <[email protected]> * fix(tui): guarantee deferred stream-clear redraw and other review fixes A throttled stream-clear could drop the redraw entirely when a newline arrived in the last delta of a turn, leaving a ghost caret on screen. Add a one-shot deferred flush timer plus a flush on stream end. Fix a stale comment and a test that leaked global logger state in the vendored bubbletea fork. * fix(tui): use m.now for stream-clear throttle and harden tests Drive stream-clear rate limiting through the mockable clock and a shared streamClearThrottle constant so both the agentTextMsg and flush paths stay in sync. Add an explicit two-newline-within-window regression and detect ClearScreen by function pointer so tests do not evaluate Tick side effects. * fix(tui): relocate bubbletea fork out of third_party Move the maintained bubbletea/v2 hard-scroll patch from third_party/bubbletea-v2-patched to patches/bubbletea-v2 and update the root go.mod replace. third_party/ is immutable vendored content per AGENTS.md; a maintained fork belongs outside that tree. * fix(ci,docs): run vendored bubbletea fork tests and document unsupported go install path patches/bubbletea-v2 is its own Go module so `go test ./...` from the repo root never touched its 7 test files. Add a CI step (and a make test-patches target for local use) that runs the fork's own suite across the smoke matrix. The root go.mod's local `replace charm.land/bubbletea/v2 => ./patches/bubbletea-v2` also breaks `go install github.com/Gitlawb/zero/cmd/zero@<version>`, since a versioned install can't resolve a replace pointing at a filesystem path. Document that path as unsupported in README.md and docs/INSTALL.md and point at git clone + go build instead, per jatmn's review findings on PR #709. * ci: run vendored bubbletea fork tests with -race CodeRabbit review on this PR. The fork exists specifically to patch a concurrency bug (charmbracelet/bubbletea#1690), so its regression test needs the race detector to actually catch a reintroduced race instead of just passing on a benign-looking run. * fix(tui): drop vendored bubbletea fork; keep stream-clear workaround Maintainers asked not to ship an in-tree bubbletea replace: it breaks versioned go install and is a large maintenance surface for a narrow multipass/Windows Terminal scroll bug. Remove patches/bubbletea-v2 and the replace directive; keep the model.go full-redraw-on-streamed-newline workaround (ZERO_NO_STREAM_CLEAR to opt out). Restore normal install docs. Refs Gitlawb/zero#709 --------- Co-authored-by: Claude Fable 5 <[email protected]> |
||
|
Anandan
|
96859c9bd1 |
feat(sandbox): unify command execution and enforcement (#781)
* feat(sandbox): unify command execution and enforcement * test(sandbox): make platform contracts portable * fix(sandbox): close execution lifecycle races Bound retained interactive output, preserve interruption state, and start workspace observation before process launch. Protect active runtime roots with cross-platform leases, capture transient protected-path creation events, and restore process attributes after PTY fallback. * fix(sandbox): restore strict macOS isolation Remove Seatbelt host-local network allowances after native validation showed localhost filters can reach host interfaces. Track process reap completion so retained sessions cannot signal a stale numeric PID. |
||
|
KRATOSandKRATOS
|
3967d49d64 |
feat(tui): permission prompt takes free-text feedback inline (#780)
* feat(tui): let the permission prompt take free-text feedback inline "No, and tell Zero what to do differently" now opens an inline input on the permission card instead of silently cancelling: type an instruction, Enter sends it, Esc returns to the option list. The text is delivered as a Deny decision whose Reason is the typed instruction, so the agent surfaces it via deniedPermissionResult as the tool result and the model adjusts in the SAME turn — where the old cancel returned an error and aborted the run. Empty input + Enter falls back to a plain cancel (the prior meaning). No agent-side change: PermissionDecision.Reason already carries free text to the model. The UI mirrors the ask_user "type your own answer" pattern — the feedback field reuses the shared composer input and the card renders it in the transcript body. Also: a focused permission prompt now suppresses the composer (and idle hints/plan panel), keeping only the status line, exactly like the ask_user modal. Its keys are inert anyway, so a "describe a task for zero…" box below the card was misleading — and in feedback mode the shared input would otherwise echo in two places. Tests cover open / submit-as-Deny-with-text / empty-cancels / Esc-returns, the composer suppression in both modes, and that the feedback text renders exactly once. * fix(tui): preserve the composer draft across permission feedback (#780 review) anand + CodeRabbit (Major): entering feedback mode cleared the shared composer input via SetValue(""), and neither submit nor cancel restored it — so a user with a half-typed or queued next-turn message who chose "tell Zero what to do differently" lost that draft. Snapshot the composer value into pendingPermissionPrompt.savedDraft when feedback mode opens, and restore it on BOTH exits (submit and Esc-cancel). Only the feedback text is consumed, delivered via the decision Reason. Adds regression tests for both paths; mutation-verified against the SetValue("") regression. * fix(tui): guard permission-feedback backspace and clicks (#780 review) Vasanthdev2004 review, all on the permission path: 1. Real bug: Backspace on an empty feedback field dropped a staged attachment. The keyBackspace case runs before the typing branch and, on an empty composer (feedback mode clears it), fell to removeLastAttachment — silently dropping a staged image/doc that savedDraft does not restore. Route Backspace to the shared input while typing so it is a plain edit of the feedback text. 2. Defence-in-depth: the mouse click resolver in handleTranscriptSelectionMouse resolved a clicked permission option (Allow included) with no typing check. It is safe today only because renderFocusedPermissionPrompt returns nil clickable offsets in feedback mode — a single early-return in a function other PRs also edit. Add an explicit !typing guard on the click path so the safety is stated, not emergent. 3. Test hardening (also raised by CodeRabbit): assert len(got) == 1 before indexing got[0] in the draft-preservation test, so a regression fails cleanly instead of panicking. Tests: TestPermissionFeedbackBackspaceKeepsStagedAttachment (mutation-verified against the missing guard) and TestPermissionFeedbackRendersNoClickableOptionsWhileTyping. --------- Co-authored-by: KRATOS <[email protected]> |
||
|
KRATOSandKRATOS
|
722bb31216 |
fix(tui): stop the permission card clashing on cool themes (#778)
The focused permission prompt was the only prompt card in the TUI that used
brand/warning colours for its interior, and both read badly on cool themes such
as dracula:
1. The selected option ("Yes, proceed") was filled with zeroTheme.badge — the
brand chip meant for short labels (" 0 ", " ASK ", " SPEC REVIEW "), a
full-brightness accent fill. Every other selectable list in the TUI (model
picker, provider list, / palette) highlights its selected row with onSel/selBg
instead. The permission card was the outlier, painting a saturated accent slab
across the row and skipping the card tint every other line composed onto.
2. The card BODY was tinted with permBg, an amber-family wash intended to signal
a warning surface. It works on the warm default palette but renders as a
brown-yellow slab over cool themes, and it made the permission card the only
prompt whose body carries a fill — ask_user, spec review and plan all use a
transparent body (lipgloss.NewStyle()).
Fix both to match the rest of the TUI: the selected row uses onSel(ink) like
every other picker, and the body drops its permBg fill for a transparent one.
Warning identity is unchanged — the amber PERMISSION badge and the amber-mixed
border still mark it as a permission gate; only the clashing interior fills go.
TestFocusedPermissionSelectedRowUsesSelectionTintNotBrandChip asserts against
palette entries (not hardcoded hex, so it survives a theme retune): the selected
row carries selBg and not the brand accent, the body carries no permBg wash, and
the PERMISSION badge keeps its amber fill so the card still reads as a warning.
Co-authored-by: KRATOS <[email protected]>
|
||
|
KRATOSandKRATOS
|
b30c3971b4 |
fix(tui): model rows labelled by id when the description is prose; keep the sidebar under the / palette (#775)
* fix(tui): label model rows by id when the description is prose
The model picker (provider wizard AND onboarding, which share displayLabel)
labelled each row with the provider's Description, falling back to the ID only
when the description was "generic". That is right when Description holds a short
display name -- "Grok 4.3" for x-ai/grok-4.3, which is what the field is for and
why the row prefers it over an ugly raw ID.
Providers do not all honour that. Aggregators commonly return a prose blurb
instead ("Fast DeepSeek model for efficient chat, coding help, and agent
loops"), which fails as a label twice: it overflows the row, and because such
blurbs are written per model FAMILY rather than per model, several DIFFERENT
models collapse to byte-identical rows. Observed in a live picker with two
identical "Open-weight GPT model for self-hosted reasoning..." rows and two
identical "Kimi multimodal agent model..." rows -- the distinguishing ID was
visible only in the detail line of whichever row happened to be highlighted, so
the list could not be used to pick between them.
providerWizardGenericModelDescription cannot catch these: it rejects vague
descriptions ("live model", "catalog default"), whereas these are specific, just
shared and sentence-shaped. So gate on shape instead -- a display name is short
and few-worded; anything longer is prose and the ID (unique by construction)
makes the better label. Friendly names are still preferred where providers
supply them, so the existing behaviour is unchanged for well-behaved catalogs.
* fix(tui): keep the sidebar open under the / command palette
sidebarAvailable grouped suggestionsActive() with the full-screen overlays
(setup, wizards, pickers), whose stated rationale is that they "take over the
chat column and render at full width". That is not true of the `/` palette:
suggestionOverlay draws it through centerRenderedBlock at
minInt(width, suggestionPaletteMaxWidth), a centred box capped at 76 cells
floating over the chat column.
Treating it as full-screen collapsed the whole layout on every `/`: the sidebar
disappeared, the plan panel dropped out of it and re-rendered inline above the
composer, and the transcript re-wrapped to full width -- a jarring reflow for a
popup, and worst mid-run with a live plan on screen.
It cannot overlap the sidebar either: twoColumnTranscriptView renders the
overlay with width = chatColumnWidth, so the palette is capped at
min(chatColumn, 76) and centred inside the chat column. Mouse hit-testing is
unaffected because sidebarLineAtMouse carries its own suggestionsActive() guard,
so clicks still reach the palette rather than the sidebar rows beneath it.
The genuinely full-width overlays keep suppressing the sidebar, pinned by the
new test alongside the palette case.
---------
Co-authored-by: KRATOS <[email protected]>
|
||
|
KRATOSandKRATOS
|
8c6d3022bd |
feat(tui): add /undo as an alias for /rewind (#698) (#747)
* feat(tui): add /undo as an alias for /rewind (#698) /undo was unregistered and fell through to commandUnknown. Add it to the /rewind command's aliases (the same mechanism /quit, /find, /mcp-status use), so it dispatches to the existing rewind handler with args intact. * test(tui): cover the /undo numeric-sequence form (#698 review) Add a parseCommand("/undo 123") assertion so both the latest and numeric <sequence> forms of the /rewind contract are exercised through the alias. --------- Co-authored-by: KRATOS <[email protected]> |
||
|
PierrunoYT
|
4cbd144d11 |
fix(providers): stop "provider not found" for env-derived profiles (#716)
* fix(providers): stop "provider not found" for env-derived profiles * fix(providers): address env profile review feedback * fix(providers): keep env JSON response schemas stable |
||
|
PierrunoYT
|
d74ceb1127 |
fix(tui): cache settled alt-screen transcript (#647)
* fix(tui): cache settled alt-screen transcript * fix(tui): invalidate settled alt-screen cache on in-place status row updates setDoctorStatusRow, setSandboxSetupStatusRow, and setCompactStatusRow all mutate an already-flushed transcript row in place (m.transcript[i] = row) without invalidating the alt-screen settled cache. Since none of frontier, width, or row count change on such an update, settleTranscript's rebuild guard never fires and the View fast path serves the stale cached snapshot - e.g. the doctor connectivity spinner freezes on its first frame. Zero altScreenSettledWidth on the in-place path, mirroring the existing pattern in toggleTranscriptRow, so the next settle rebuilds the cache. Addresses PR #647 review feedback from Vasanthdev2004. * fix(tui): invalidate settled transcript cache * fix(tui): invalidate settled file selection cards |
||
|
Anandan
|
2e267bdbee |
feat(tui): add isolated /btw conversations (#748)
* feat(tui): add isolated btw conversations Add /btw with inline-question support, isolated non-resumable session forks, background main-run routing, and safe return behavior. Refs #637 * test(tui): cover btw return controls Preserve drafts when Ctrl+C is pressed during an active BTW run and cover returning to the parent through the documented /btw toggle. * Address BTW isolation review feedback * Reject explicit resumes of side sessions * Harden BTW isolation lifecycle |
||
|
Vasanth T
|
378d538e24 |
perf(agent): execution profiles with one-shot posture escalation (PR10b+PR10c) (#740)
* perf(agent): execution profiles, --exec-profile, bench passthrough, TUI /profile (PR10b+10c) * fix(execprofile): harden profiles against review findings - fast escalates on critical-risk mutations, not high: the sandbox marks every shell command high before command analysis, so a high trigger spent the one-shot on the first go test of any task - escalation can now restore the provider-default effort a profile displaced (PostureEscalation.RestoreDefaultEffort; a plain empty target means leave untouched) - TUI /turns while a profile is active pins the budget: disarms the escalation turn target (mirroring exec's explicit --max-turns rule) and profile reverts use touched bits instead of value equality, so explicit choices survive even when they coincide with profile values - spec-draft runs get the same escalation policy as the main run instead of the displaced budget with no safety net - bench validates and canonicalizes --exec-profile before spawning: an unknown name previously produced an exit-0 report full of millisecond usage-error walls - turn summary prints exec-profile alongside mode; zero exec rejects an inline-empty --exec-profile=; balanced trace label documented and pinned as deliberate attribution * fix(execprofile): explicit effort choices disarm the escalation effort restore An explicit effort set AFTER profile selection could be cleared by a mid-run escalation (the effort analog of the /turns pin, introduced by RestoreDefaultEffort): TUI /effort and Ctrl+T now disarm the restore on a policy clone, and the spec-draft path only arms it when no explicit --spec-reasoning-effort governs the draft. * fix(execprofile): address review findings on the profile surfaces - --use-spec projects away a profile's self-correct knob instead of silently arming an option the draft runner ignores (the explicit --self-correct --use-spec combination is already a parse-time error) - RunTurnBench canonicalizes and validates ExecProfile at the library boundary, so a direct caller's casing/whitespace cannot fork report labels and an unknown name fails before anything runs - reverting a profile whose displaced budget was zero clears ZERO_MAX_TURNS (SetMaxTurnsEnv ignores zero, so sub-agents would have kept the removed profile's budget) - the TUI effort-disarm test fails instead of skipping when the catalog stops filling low effort * fix(tui): reconcile an active profile's effort fill on model switch The fill is per-model, not a session preference: fast selected on a model without low fills nothing, and switching to a model that supports it should behave like selecting the profile there; the reverse switch must not keep sending a level the destination does not support. Both switch paths (the /model command and the cross-provider picker) now re-derive the fill, leaving explicitly touched efforts alone and keeping the escalation's effort restore in step. Also: the revert-env test asserts key removal via LookupEnv, and applyProfileTurnBudget's pinned-budget parameter is named and documented as covering mode presets, not just the flag. * fix(tui): void profile effort bookkeeping when the model-switch drop erases it The /model path's pre-existing unsupported-effort drop ran before the profile reconciliation and left contradictory state behind: a dropped touched override kept blocking the profile from governing, and a dropped profile fill left a stale applied value with the effort restore still armed. The drop itself is load-bearing (the TUI run path forwards the effort to the provider unfiltered), so instead of preserving a known-unsupported level, the voided choice now resets the profile's effort bookkeeping and the reconcile re-derives from scratch. The whole sequence lives in one ring-parameterized helper driven by the switch target's authoritative ring, unit-tested against synthetic rings for both the void-then-refill arc and the supported-override survival. * fix(tui): only drop an explicit effort when the target ring is authoritative A nil ring meant both a catalog model with no effort controls and a live-discovered custom model the catalog cannot vouch for. The drop now requires an authoritative ring (a resolved catalog entry, whose ring may legitimately be empty); an unknown ring preserves an explicit preference, matching the cross-provider picker's behavior, while the profile's own fill still applies only where support is known. |
||
|
|
6b9c2f0c08 |
feat(aimlapi): AI/ML API provider with guided onboarding (top-up + key issuance) (#655)
* feat(aimlapi): AI/ML API guided onboarding (top-up + key issuance) Add the AI/ML API provider integration to the zero CLI: a guided TUI onboarding sub-flow that takes an existing key or an email top-up and writes the issued key into the provider profile, attributed to the Gitlawb rebate partner. - internal/aimlapi: OpenAI-compatible client, partner-checkout top-up, key validation, streaming, and endpoint config (Paths A/B/C). - providercatalog / providermodelcatalog: register the aimlapi provider with default headers and models. - tui: aimlapi onboarding screen wired into the provider wizard and onboarding flow. Default endpoints point at production (auth/app/api/pay/aimlapi.com); each is overridable via AIMLAPI_*_URL env vars. Co-Authored-By: Claude Opus 4.8 <[email protected]> * fix(aimlapi): address review findings in onboarding flow - topup: use errors.As for the 5xx retry check so a wrapped APIError still retries instead of aborting a paid checkout. - onboarding: a Path-A top-up on an email with no account now lands on a single coherent "key saved, balance not topped up" screen instead of a green "ready" plus a red error. - onboarding: an empty amount field is rejected instead of silently falling through to the $25 default. - collapse the two identical top-up prompt constants into MsgTopUpPrompt and drop the dead amountPrompt() branch. - reuse stdlib maps.Clone instead of a third bespoke header-copy helper. Co-Authored-By: Claude Opus 4.8 <[email protected]> * fix(aimlapi): retry transient poll errors and sharpen amount validation * fix(aimlapi): retry transient poll errors and surface real amount errors * docs(aimlapi): document exported client API; drop out-of-scope gitignore entry Addresses the two CodeRabbit PR checks on #655: - Add doc comments to the exported aimlapi client API (Client, APIError, the session/checkout/onboarding types and their methods) and ParseAmountUSD, to satisfy the docstring-coverage check. - Remove the unrelated /zero.exe~ .gitignore entry that an earlier review-fix commit added; it is out of scope for the provider work, so the PR no longer touches .gitignore. Co-Authored-By: Claude Opus 4.8 <[email protected]> * fix(aimlapi): render aimlapi row like other recommended providers * fix(aimlapi): resolve onboarding review findings (Path A funding, base URL, request cancellation) * fix(aimlapi): validate pasted API keys * test(aimlapi): clarify pasted key validation * test(aimlapi): cover key-balance low-balance transition * fix(aimlapi): reject non-finite top-up amounts * fix(cli): scope and canonicalize provider headers * fix(tui): distinguish invalid verification codes * feat(aimlapi): resume account-bound top-ups safely * feat(tui): offer top-up for existing AIMLAPI keys * feat(tui): reuse AIMLAPI env key on first run * feat(tui): reuse configured AIMLAPI profiles * test(aimlapi): trim redundant flow coverage * fix(tui): preserve existing AIMLAPI profile metadata * fix(tui): clone AIMLAPI headers in env fallback * fix(aimlapi): address remaining review findings * fix(aimlapi): harden onboarding lifecycle * fix(aimlapi): preserve validated checkout context * fix(aimlapi): harden checkout retry state * fix(tui): clarify AIMLAPI configured choices * fix(aimlapi): make checkout retries idempotent * fix(aimlapi): validate checkout responses * fix(providers): reconcile aimlapi catalog after rebase --------- Co-authored-by: Lookoff123 <[email protected]> Co-authored-by: Claude Opus 4.8 <[email protected]> |
||
|
Leonardo Faoro
|
7f669f4550 |
feat(tui): Ctrl+X leader chords and emacs menu navigation (#699)
* feat(tui): Ctrl+X leader chords and emacs menu navigation Add Ctrl+X leader shortcuts for common slash commands (with Ctrl+X ? chord map), and Ctrl+P/N previous/next selection in modals and pickers. * fix(tui): drop Ctrl+X e /edit leader chord /edit replaces the composer and would discard an in-progress draft; leave it unbound so leader shortcuts stay draft-preserving. * fix(tui): consume idle Ctrl+N as a no-op Reserve Ctrl+N for emacs menu navigation so it never falls through to remapped global bindings when no selection surface is open. * docs: drop manual CHANGELOG entries for TUI shortcuts |
||
|
euxaristia
|
2b42cd567b |
fix(tui): stop the composer cursor blinking while typing or unfocused (#672)
* fix(tui): stop the composer cursor blinking while typing or unfocused The composer's cursor blinked on a fixed timer unconditionally, including mid-keystroke, which made typing feel janky since the cursor's job is to mark the current position. Gate the existing blink tick on two states instead: force solid while a keystroke landed within the last 500ms (reusing the existing lastCharTime tracker), and hide it entirely while the terminal window is unfocused (new terminalFocused field, driven by tea.FocusMsg/ BlurMsg). Idle and focused keeps the prior blink behavior unchanged. Also decouples view.ReportFocus from the notifier (it was previously gated on m.notifier != nil, which is effectively always true in production but left cursor-focus behavior fragile if that ever changes) so focus reporting is requested unconditionally. * fix(tui): apply caret visibility synchronously and in the argument-hint line - A keypress, focus, or blur now updates composerCursorVisible in its own message handler instead of waiting for the next 530ms composerBlinkMsg tick, which could render a typed character caret-less, leave a visible caret in an unfocused terminal, or keep it hidden after a refocus. - commandArgumentHintComposerLine takes the caret visibility state instead of painting its cursor cell unconditionally, so the required-argument hint path follows the same focus and blink contract as the main composer renderer. - New tests assert the state immediately after each message, with no blink tick in between, and cover the argument-hint render in both states. * fix(tui): apply the caret state on composer paste A paste is the same immediate-input transition as a keypress: routePaste now refreshes lastCharTime and sets the caret visible for both the bracketed tea.PasteMsg and right-click clipboardReadMsg paths, so a paste right after the blink phase hid the caret renders solid immediately instead of waiting for a tick that would toggle off a stale idle state. |