Compare commits

..
Author SHA1 Message Date
Vasanthdev2004 da050ef23a docs(contributing): add contribution volume and batching guidance
Documents expectations for keeping contribution flow sustainable during the
stabilization phase: one issue per bug (no bundled multi-finding audits), keep
only a few items open at a time, and hold AI-assisted contributions to the same
per-item verification bar. Complements the existing issue-approved gate, which
already covers PR-to-issue linkage and scope; the gap was submission volume.
2026-07-21 11:25:29 +05:30
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]>
2026-07-20 23:29:10 +05:30
KRATOSandKRATOS 89bdc6719a fix(tools): read_file recovers a backwards line range instead of erroring (#779)
* fix(tools): read_file recovers a backwards line range instead of erroring

renderReadFileRange hard-failed when end_line < start_line, costing the caller a
whole retry for an arithmetic slip. It was also inconsistent: start_line past EOF
returns a friendly note and end_line past EOF is auto-clamped to the last line —
only a backwards range hard-errored.

Recover it the same way: clamp end_line UP to start_line (read just that line) and
surface the adjustment in the output so it is never silent. Valid ranges
(end_line >= start_line) are byte-for-byte unchanged.

The change is in the argument-validation phase, before any file I/O, so there is
no runtime cost; if anything it saves the agent a wasted retry turn.

Tests: TestReadFileToolRecoversBackwardsRange (recovery + note + reads only
start_line, mutation-verified against the old hard error) and
TestReadFileToolValidRangeUnchangedByRecovery (valid ranges carry no note and read
the identical window).

* test(tools): assert full valid-range output in read_file recovery test (#779 review)

anand + CodeRabbit: TestReadFileToolValidRangeUnchangedByRecovery claimed
byte-for-byte preservation but only checked substrings, so an altered
header/separator or a stray out-of-range line would still pass. Compare the
whole result.Output against the exact expected rendering.

---------

Co-authored-by: KRATOS <[email protected]>
2026-07-20 22:57:25 +05:30
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]>
2026-07-20 22:39:50 +05:30
KRATOSandKRATOS a21a052a4a feat(sandbox): disable the sandbox via config (#687) (#746)
* feat(sandbox): disable the sandbox via config (#687)

Add a sandbox.enabled field (`"sandbox": {"enabled": false}`). ModeDisabled
already short-circuits the engine but had no config surface.

- SandboxConfig.Enabled *bool (pointer distinguishes an explicit false from an
  omitted key).
- mergeConfig honors it (global config / CLI); applyConfiguredSandboxPolicy maps
  enabled:false to ModeDisabled.
- mergeProjectConfig intentionally does NOT merge it: a cloned repo must not be
  able to disable the sandbox that constrains it — same posture as
  AdditionalWriteRoots and the Network tighten-only rule.

* fix(config): honor sandbox.enabled from the CLI override path (#687 review)

applyOverrides copied BlockUnixSockets/MonitorDenials but not the new Enabled
tri-state pointer, so a CLI/programmatic Overrides{Sandbox:{Enabled:&false}} was
dropped — contradicting the 'global config / CLI can disable' contract. Preserve
the pointer in applyOverrides (runs after all config sources, so an explicit
override wins) and add a resolver regression test: an explicit false override
disables, and an explicit true override wins over a lower-precedence config false.

* fix(config): keep sandbox.enabled out of the provider-command path (#687 review)

mergeConfig now copies Sandbox.Enabled, and it is shared by two callers: the
global-config merge (trusted) and the provider-command merge (not). A provider
command is an arbitrary executable named in config, and LoadProviderCommand
parses its stdout into a full FileConfig -- so one returning a valid provider
plus {"sandbox":{"enabled":false}} could disable the sandbox meant to constrain
it, despite the setting being documented as global-config/CLI only. Confirmed
reproducible in both directions before the fix.

Clear Sandbox.Enabled from the provider-command config before merging, mirroring
the carve-out mergeProjectConfig already applies for the same reason. Done at the
call site rather than inside mergeConfig because that helper must keep honouring
the setting for the trusted global-config merge.

Adds TestResolveSandboxEnabledIgnoredFromProviderCommand covering both
directions (cannot disable, cannot enable) and asserting the provider itself is
still applied, so the guard cannot be satisfied by dropping the command wholesale.

---------

Co-authored-by: KRATOS <[email protected]>
2026-07-20 21:07:04 +05:30
KRATOSandKRATOS f079b90f82 fix(sandbox): AST second opinion for interactive-command bypasses (#473) (#745)
* fix(sandbox): AST second opinion for interactive-command bypasses (#473)

The interactive-command guard split shell commands with a hand-written parser
(splitShellSegments), which mis-handles unusual quoting, command substitution,
subshells, and newline separators — an interactive program hidden by one slips
through and the agent hangs until the per-command timeout.

Add astCommandFields(): mvdan.cc/sh/v3/syntax (already used by analyzer.go) to
re-extract the real simple commands, then apply the SAME per-program checks the
regex path uses (interactivePrograms + hasNonInteractiveFlag). This only ADDS
detections and classifies every program exactly as before — ssh with a trailing
command, python -c, etc. stay allowed — while catching the splitter's bypasses.
Unparseable input (Windows cmd.exe, obfuscation) yields no commands and falls
through to the regex path; the guard never hard-blocks on a parse error.

* fix(sandbox): AST pass runs the full pipeline + guards dynamic programs (#473 review)

Address review on #745:
- The AST second opinion now applies the complete detection pipeline via a new
  inspectCommandFields helper (multi-word interactive segments, sh -c payload
  recursion, then the per-program lookup), so parser-hidden bypasses like
  { git rebase -i HEAD~1; } and { sh -c 'vim file.txt'; } are caught, not just
  bare interactive programs.
- astCommandFields now skips a CallExpr whose program word is not a static
  literal (isLiteralWord), so a substitution concatenated with a literal
  ($(printf foo)vim -> foovim) can't fabricate an interactive program name.

Regression tests added for both boundaries. (The un-braced $(printf foo)vim
false positive is a separate, pre-existing hand-parser behavior — present on
main before this PR — and is out of scope here.)

* fix(sandbox): require literal words for the whole AST call (#473 review)

astCommandFields guarded only the program word, then ran every argument
through wordText, which keeps literal/quoted parts and silently drops
CmdSubst/ParamExp. A dynamic ARGUMENT was therefore reconstructed lossily:
`git $(printf foo)rebase -i HEAD~1` runs as `git foorebase -i` (a
non-existent, non-interactive subcommand) but collapsed to `git rebase -i`
and was blocked -- a false positive on a command the user never wrote, and
new on this branch (the base correctly reports not-interactive).

Extend the literalness guard to every word in the call: if any word carries
an expansion, skip that AST command rather than classify a reconstruction
whose runtime value is unknowable. Skipping is the safe direction -- the
hand-written passes still run first, and a miss falls through to the normal
permission prompt instead of hard-blocking.

Genuine detection is unchanged: `git rebase -i HEAD~1` and the #473 bypass
cases still classify exactly as before.

---------

Co-authored-by: KRATOS <[email protected]>
2026-07-20 21:01:55 +05:30
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]>
2026-07-20 20:58:11 +05:30
17 changed files with 846 additions and 31 deletions

No files matched your search

+22
View File
@@ -67,6 +67,28 @@ An approved issue means only that the specific issue has been accepted for work.
It does not approve unrelated future work, future pull requests, or future issues
from the same author. Each issue is reviewed case by case.
## Contribution Volume and Batching
To keep triage and review sustainable while the project stabilizes, please keep
your contribution flow to a size the core team can realistically work through:
- One issue per bug. File each defect as its own issue with its own
reproduction. Please do not bundle several unrelated findings into a single
"audit" issue; separate issues are far easier to triage, approve, and close
individually.
- Keep only a few items open at a time. If you already have several issues or
pull requests awaiting review, let those move through the queue before opening
more. A large simultaneous burst from one contributor slows review for
everyone, even when each item is valid.
- AI-assisted contributions are welcome, held to the same bar. Automated audits
and generated patches are fine, but each issue and pull request must be
individually verified, scoped, and reproducible before you open it. Please do
not post bulk machine-generated findings unfiltered.
These apply to all contributors and are about review bandwidth, not the value of
any single report. Maintainers may ask you to consolidate, split, or hold
submissions to keep the queue manageable.
## Pull Request Requirements
Before opening a community pull request, make sure all of the following are true:
+6
View File
@@ -939,6 +939,12 @@ func providerSensitiveEnvKeys(resolved config.ResolvedConfig) []string {
// applyConfiguredSandboxPolicy overlays every config-sourced sandbox knob onto
// the default policy.
func applyConfiguredSandboxPolicy(policy sandbox.Policy, cfg config.SandboxConfig) sandbox.Policy {
// An explicit `"sandbox": {"enabled": false}` disables the sandbox: every
// request is allowed and commands run unwrapped. Only reachable from global
// config / CLI — project config never sets Enabled (see mergeProjectConfig).
if cfg.Enabled != nil && !*cfg.Enabled {
policy.Mode = sandbox.ModeDisabled
}
if network := strings.TrimSpace(cfg.Network); network != "" {
switch sandbox.NetworkMode(network) {
case sandbox.NetworkAllow, sandbox.NetworkDeny:
+15
View File
@@ -1001,3 +1001,18 @@ func sandboxPolicyCapabilityStatus(capabilities []sandbox.BackendCapability, key
}
return ""
}
func TestApplyConfiguredSandboxPolicyEnabledFalseDisables(t *testing.T) {
disabled := false
if got := applyConfiguredSandboxPolicy(sandbox.DefaultPolicy(), config.SandboxConfig{Enabled: &disabled}); got.Mode != sandbox.ModeDisabled {
t.Fatalf("enabled:false must yield ModeDisabled, got %q", got.Mode)
}
// Omitted (nil) and explicit true keep the default enforcing mode.
if got := applyConfiguredSandboxPolicy(sandbox.DefaultPolicy(), config.SandboxConfig{}); got.Mode == sandbox.ModeDisabled {
t.Fatal("omitted enabled must not disable the sandbox")
}
enabled := true
if got := applyConfiguredSandboxPolicy(sandbox.DefaultPolicy(), config.SandboxConfig{Enabled: &enabled}); got.Mode == sandbox.ModeDisabled {
t.Fatal("enabled:true must not disable the sandbox")
}
}
+20
View File
@@ -93,6 +93,16 @@ func Resolve(options ResolveOptions) (ResolvedConfig, error) {
if err != nil {
return ResolvedConfig{}, err
}
// Sandbox.Enabled is NOT accepted from a provider command, for the same
// reason project config cannot set it (see mergeProjectConfig): only
// global config and the CLI may turn the sandbox off. A provider command
// is an arbitrary executable named in config, and LoadProviderCommand
// parses its stdout into a full FileConfig — so without this, a command
// returning a valid provider plus {"sandbox":{"enabled":false}} would
// disable the very sandbox meant to constrain what it can do. Cleared
// here rather than in mergeConfig because that helper is shared with the
// trusted global-config merge, which must keep honouring the setting.
commandConfig.Sandbox.Enabled = nil
mergeConfig(&cfg, commandConfig)
}
@@ -216,6 +226,9 @@ func mergeConfig(dst *FileConfig, src FileConfig) {
mergeProvider(dst, provider)
}
mergeMCPConfig(&dst.MCP, src.MCP, true)
if src.Sandbox.Enabled != nil {
dst.Sandbox.Enabled = src.Sandbox.Enabled
}
if network := strings.TrimSpace(src.Sandbox.Network); network != "" {
dst.Sandbox.Network = network
}
@@ -273,6 +286,10 @@ func mergeProjectConfig(dst *FileConfig, src FileConfig) error {
if err := mergeProjectMCPConfig(&dst.MCP, src.MCP); err != nil {
return err
}
// Sandbox.Enabled is intentionally NOT merged from project config: a cloned
// repo's .zero/config.json must not be able to disable the sandbox that
// constrains it. Only global config and CLI can turn the sandbox off.
//
// Sandbox.AdditionalWriteRoots is intentionally NOT merged from project
// config: a cloned repo's .zero/config.json must not be able to grant
// itself write access outside the workspace. Global config and CLI flags
@@ -688,6 +705,9 @@ func applyOverrides(cfg *FileConfig, overrides Overrides) {
if overrides.MaxTurns > 0 {
cfg.MaxTurns = overrides.MaxTurns
}
if overrides.Sandbox.Enabled != nil {
cfg.Sandbox.Enabled = overrides.Sandbox.Enabled
}
if overrides.Sandbox.BlockUnixSockets {
cfg.Sandbox.BlockUnixSockets = true
}
+79
View File
@@ -1744,6 +1744,85 @@ func TestResolveSandboxNetworkProjectConfigCannotWeaken(t *testing.T) {
}
}
func TestResolveSandboxEnabledUserConfigDisables(t *testing.T) {
userPath := writeConfig(t, `{"sandbox": {"enabled": false}}`)
resolved, err := Resolve(ResolveOptions{UserConfigPath: userPath, Env: map[string]string{}})
if err != nil {
t.Fatalf("Resolve: %v", err)
}
if resolved.Sandbox.Enabled == nil || *resolved.Sandbox.Enabled {
t.Fatalf("user config enabled:false must resolve to a false pointer, got %v", resolved.Sandbox.Enabled)
}
}
func TestResolveSandboxDisableIgnoredFromProjectConfig(t *testing.T) {
// Security: a cloned repo's project config must NOT be able to disable the
// sandbox that constrains it. Only global config / CLI can turn it off.
userPath := writeConfig(t, `{}`)
projectPath := writeConfig(t, `{"sandbox": {"enabled": false}}`)
resolved, err := Resolve(ResolveOptions{UserConfigPath: userPath, ProjectConfigPath: projectPath, Env: map[string]string{}})
if err != nil {
t.Fatalf("Resolve: %v", err)
}
if resolved.Sandbox.Enabled != nil {
t.Fatalf("project config enabled:false must be ignored, got %v — a repo could disable the sandbox", *resolved.Sandbox.Enabled)
}
}
func TestResolveSandboxEnabledIgnoredFromProviderCommand(t *testing.T) {
// Security: a provider command is an arbitrary executable named in config, and
// its stdout is parsed into a full FileConfig — so it must NOT be able to reach
// Sandbox.Enabled, in either direction. Same rule as project config: only global
// config / CLI may turn the sandbox off.
for _, tc := range []struct {
name string
value string
}{
{name: "cannot disable", value: "false"},
{name: "cannot enable", value: "true"},
} {
t.Run(tc.name, func(t *testing.T) {
command := writeCommand(t, commandScript{
Stdout: `{"activeProvider":"cmd","providers":[{"name":"cmd","provider":"openai","apiKey":"sk-command","model":"gpt-command"}],"sandbox":{"enabled":` + tc.value + `}}`,
})
resolved, err := Resolve(ResolveOptions{ProviderCommand: command, Env: map[string]string{}})
if err != nil {
t.Fatalf("Resolve: %v", err)
}
// The provider itself must still be applied — only the sandbox toggle is dropped.
if resolved.ActiveProvider != "cmd" {
t.Fatalf("ActiveProvider = %q, want cmd (provider command must still apply)", resolved.ActiveProvider)
}
if resolved.Sandbox.Enabled != nil {
t.Fatalf("provider command set sandbox.enabled=%s, got %v — a provider command could toggle the sandbox", tc.value, *resolved.Sandbox.Enabled)
}
})
}
}
func TestResolveSandboxEnabledCLIOverride(t *testing.T) {
// A CLI/programmatic override (applied after every config source) must be able
// to disable the sandbox, even with no config file.
disabled := false
resolved, err := Resolve(ResolveOptions{Env: map[string]string{}, Overrides: Overrides{Sandbox: SandboxConfig{Enabled: &disabled}}})
if err != nil {
t.Fatalf("Resolve: %v", err)
}
if resolved.Sandbox.Enabled == nil || *resolved.Sandbox.Enabled {
t.Fatalf("CLI override enabled:false must disable the sandbox, got %v", resolved.Sandbox.Enabled)
}
// An explicit true override must win over a lower-precedence global-config false.
userPath := writeConfig(t, `{"sandbox": {"enabled": false}}`)
enabled := true
resolved2, err := Resolve(ResolveOptions{UserConfigPath: userPath, Env: map[string]string{}, Overrides: Overrides{Sandbox: SandboxConfig{Enabled: &enabled}}})
if err != nil {
t.Fatalf("Resolve: %v", err)
}
if resolved2.Sandbox.Enabled == nil || !*resolved2.Sandbox.Enabled {
t.Fatalf("CLI override enabled:true must win over config false, got %v", resolved2.Sandbox.Enabled)
}
}
func TestResolveNotifyValid(t *testing.T) {
path := writeConfig(t, `{"notify":{"mode":"both","focusMode":"always"}}`)
resolved, err := Resolve(ResolveOptions{UserConfigPath: path, Env: map[string]string{}})
+6
View File
@@ -62,6 +62,12 @@ func HasProviderProfile(profile ProviderProfile) bool {
}
type SandboxConfig struct {
// Enabled turns the sandbox off when set to false (`"sandbox": {"enabled":
// false}`). A pointer distinguishes an explicit false (disable) from an
// omitted key (keep the default: enabled). Honored from the GLOBAL user
// config and CLI only — deliberately NOT project config, so a cloned repo
// cannot disable the sandbox that constrains it. nil / true keep enforcement.
Enabled *bool `json:"enabled,omitempty"`
// Network controls whether shell commands classified as network-touching
// (curl, git push, package installs, …) are allowed: "allow" or "deny".
// Empty keeps the built-in default (deny). Without this knob the engine's
+70
View File
@@ -65,6 +65,76 @@ func AnalyzeCommand(script string) AnalysisResult {
return result
}
// astCommandFields parses command with the shell parser and returns each simple
// command as its literal field slice (program + args as text), resolving the
// real command positions across quoting, command substitution, subshells, and
// newline separators — the constructs the hand-written splitter in
// safe_command.go mis-handles (issue #473). It returns nil when the command
// cannot be parsed (e.g. a Windows cmd.exe string), so callers fall through to
// the regex path rather than hard-blocking.
func astCommandFields(command string) [][]string {
file, err := syntax.NewParser().Parse(strings.NewReader(command), "")
if err != nil {
return nil
}
var commands [][]string
syntax.Walk(file, func(node syntax.Node) bool {
call, ok := node.(*syntax.CallExpr)
if !ok || len(call.Args) == 0 {
return true
}
// EVERY word must be a static literal, not just the program name.
// wordText keeps only the literal/quoted parts of a word and silently
// drops expansions, so a dynamic word anywhere in the call reconstructs
// to something the shell will never run: `$(printf foo)vim` (runs as
// `foovim`) collapses to "vim", and `git $(printf foo)rebase -i` (runs
// as `git foorebase -i`, non-interactive) collapses to `git rebase -i`
// and fabricates an interactive match. Since the runtime value of an
// expansion is unknowable here, skip the whole call rather than classify
// a lossy reconstruction. Skipping is the safe direction: the
// hand-written passes above already ran, and a missed detection falls
// through to the normal permission prompt instead of hard-blocking a
// command the user never wrote.
for _, word := range call.Args {
if !isLiteralWord(word) {
return true
}
}
fields := make([]string, 0, len(call.Args))
for _, word := range call.Args {
fields = append(fields, wordText(word))
}
commands = append(commands, fields)
return true
})
return commands
}
// isLiteralWord reports whether every part of word is a static literal (bare or
// quoted). A word containing a command substitution, parameter/arithmetic
// expansion, process substitution, etc. is dynamic — its runtime value is
// unknown, so its wordText (a partial literal) must not be trusted as a program
// name.
func isLiteralWord(word *syntax.Word) bool {
if word == nil {
return false
}
for _, part := range word.Parts {
switch typed := part.(type) {
case *syntax.Lit, *syntax.SglQuoted:
case *syntax.DblQuoted:
for _, inner := range typed.Parts {
if _, ok := inner.(*syntax.Lit); !ok {
return false
}
}
default:
return false
}
}
return true
}
// analyzeInto parses script and folds its interactive/destructive/network usage
// into result, sharing seen so program names are de-duplicated across recursion.
func analyzeInto(script string, result *AnalysisResult, seen map[string]bool, depth int) {
+56
View File
@@ -219,9 +219,65 @@ func DetectInteractiveCommand(command string, goos string) InteractiveCommandRes
}
}
// AST second opinion (issue #473): the hand-written segment split above
// mis-handles interactive programs hidden by unusual quoting, command
// substitution, subshells, or newline separators. Re-extract the real simple
// commands from the parsed shell tree and apply the SAME full pipeline
// (interactive segments, sh -c payload recursion, per-program lookup), so a
// bypass is caught while every program stays classified exactly as before
// (ssh with a trailing command, python -c, etc. remain allowed). A command
// the parser cannot handle (Windows cmd.exe, obfuscation) yields no commands
// and falls through unchanged — the guard never hard-blocks on a parse error.
for _, fields := range astCommandFields(command) {
if result, ok := inspectCommandFields(fields, goos); ok {
return result
}
}
return InteractiveCommandResult{}
}
// inspectCommandFields applies the full interactive-detection pipeline to one
// already-split command's fields: the multi-word interactive segments, then the
// `sh -c <payload>` recursion, then the per-program lookup with its
// non-interactive suppressions (hasNonInteractiveFlag covers REPL flags and
// trailing-command clients like ssh). It mirrors the hand-written passes above
// so an AST-extracted command is classified identically to a plainly-split one.
func inspectCommandFields(fields []string, goos string) (InteractiveCommandResult, bool) {
body := strings.ToLower(commandBody(fields))
for _, seg := range interactiveSegments {
if body == seg.match || strings.HasPrefix(body, seg.match+" ") {
suggestion := seg.suggestion
if goos == "windows" && seg.windowsSuggestion != "" {
suggestion = seg.windowsSuggestion
}
return InteractiveCommandResult{Interactive: true, Command: seg.command, Reason: seg.reason, Suggestion: suggestion}, true
}
}
first := firstProgram(fields)
if first == "" {
return InteractiveCommandResult{}, false
}
if payload := shellDashCPayload(first, fields); payload != "" {
if inner := DetectInteractiveCommand(payload, goos); inner.Interactive {
return inner, true
}
return InteractiveCommandResult{}, false
}
program, ok := interactivePrograms[first]
if !ok || (program.windowsOnly && goos != "windows") {
return InteractiveCommandResult{}, false
}
if hasNonInteractiveFlag(first, fields) {
return InteractiveCommandResult{}, false
}
suggestion := program.suggestion
if goos == "windows" && program.windowsSuggestion != "" {
suggestion = program.windowsSuggestion
}
return InteractiveCommandResult{Interactive: true, Command: first, Reason: program.reason, Suggestion: suggestion}, true
}
// wrapperPrograms are launcher prefixes that precede the real program. After
// one of these we keep scanning for the actual executable.
var wrapperPrograms = map[string]bool{
+84
View File
@@ -406,3 +406,87 @@ func TestDetectInteractiveMongoEvalAndFullPaths(t *testing.T) {
}
}
}
// The hand-written segment splitter misses interactive programs hidden by
// constructs only a real shell parser resolves (a newline separator collapsed
// to a space; a brace group that shifts the real command position). The AST
// second opinion must catch them (issue #473).
func TestDetectInteractiveCommandCatchesParserBypasses(t *testing.T) {
for _, cmd := range []string{
"echo hi\nvim file.txt", // newline separator collapsed to a space by the regex path
"{ vim file.txt; }", // brace group hides the real program position
} {
got := DetectInteractiveCommand(cmd, "linux")
if !got.Interactive {
t.Errorf("DetectInteractiveCommand(%q) = not interactive, want caught (parser bypass)", cmd)
continue
}
if got.Command != "vim" {
t.Errorf("DetectInteractiveCommand(%q).Command = %q, want vim", cmd, got.Command)
}
}
}
// The AST second opinion must not flag an interactive program NAME that appears
// only inside a quoted argument (not a real command position) — the parser
// distinguishes program from argument, so this must stay non-interactive.
func TestDetectInteractiveCommandNoFalsePositiveOnQuotedArgument(t *testing.T) {
got := DetectInteractiveCommand(`echo "please run vim later"`, "linux")
if got.Interactive {
t.Fatalf("interactive name inside a quoted argument must not be flagged, got %#v", got)
}
}
// The AST second opinion must run the FULL pipeline — multi-word interactive
// segments and sh -c payload recursion, not just the per-program lookup — so a
// grouped/nested bypass is caught; and it must not fabricate a program name from
// a command substitution concatenated with a literal (issue #473, review).
func TestDetectInteractiveCommandASTPipelineBoundaries(t *testing.T) {
interactive := []struct{ cmd, wantCmd string }{
{"{ git rebase -i HEAD~1; }", "git rebase -i"}, // multi-word segment in a brace group
{"{ sh -c 'vim file.txt'; }", "vim"}, // sh -c payload in a brace group
}
for _, tc := range interactive {
got := DetectInteractiveCommand(tc.cmd, "linux")
if !got.Interactive || got.Command != tc.wantCmd {
t.Errorf("DetectInteractiveCommand(%q) = %+v, want interactive Command=%q", tc.cmd, got, tc.wantCmd)
}
}
}
// The AST pass must not fabricate a program name from a dynamic (non-literal)
// program word: `$(printf '%s' foo)vim` runs as `foovim`, not vim. Tested at the
// extractor so the hand-written pass (which pre-empts DetectInteractiveCommand
// with its own, separate handling of that construct) does not mask the guard.
func TestAstCommandFieldsSkipsDynamicProgram(t *testing.T) {
for _, fields := range astCommandFields("$(printf '%s' foo)vim file.txt") {
if firstProgram(fields) == "vim" {
t.Fatalf("astCommandFields fabricated a vim command from a substitution: %v", fields)
}
}
}
// The literalness guard covers every word, not just the program name. A dynamic
// ARGUMENT is dropped by wordText the same way a dynamic program word is, so
// `git $(printf foo)rebase -i HEAD~1` (which runs as `git foorebase -i`, a
// non-existent and non-interactive subcommand) would otherwise be reconstructed
// as `git rebase -i` and blocked — a false positive on a command the user never
// wrote. Asserted through DetectInteractiveCommand as well as the extractor,
// because the whole point is what the caller ends up blocking.
func TestAstCommandFieldsSkipsDynamicArgument(t *testing.T) {
const dynamic = "{ git $(printf foo)rebase -i HEAD~1 ; }"
for _, fields := range astCommandFields(dynamic) {
if firstProgram(fields) == "git" {
t.Fatalf("astCommandFields reconstructed a git command from a dynamic argument: %v", fields)
}
}
if got := DetectInteractiveCommand(dynamic, "linux"); got.Interactive {
t.Errorf("DetectInteractiveCommand(%q) = %+v, want not interactive", dynamic, got)
}
// The genuine form must still be detected — the guard skips lossy
// reconstructions, it does not weaken real detection.
const genuine = "{ git rebase -i HEAD~1 ; }"
if got := DetectInteractiveCommand(genuine, "linux"); !got.Interactive || got.Command != "git rebase -i" {
t.Errorf("DetectInteractiveCommand(%q) = %+v, want interactive Command=%q", genuine, got, "git rebase -i")
}
}
+58
View File
@@ -829,3 +829,61 @@ func TestGlobSkipsWorkspaceExcludedDirectoriesAndBinaryFiles(t *testing.T) {
t.Fatalf("expected only keep.txt, got:\n%s", res.Output)
}
}
// A backwards range (end_line < start_line) is a caller-arithmetic slip, not a
// fatal error: read_file recovers by reading just start_line and surfaces a note
// explaining the adjustment, matching how an end_line past EOF is auto-clamped.
// It must never hard-error the way it used to.
func TestReadFileToolRecoversBackwardsRange(t *testing.T) {
root := t.TempDir()
writeTestFile(t, filepath.Join(root, "notes.txt"), "alpha\nbeta\ngamma\ndelta\nepsilon")
result := NewReadFileTool(root).Run(context.Background(), map[string]any{
"path": "notes.txt",
"start_line": 3,
"end_line": 2, // backwards
})
if result.Status != StatusOK {
t.Fatalf("backwards range must recover, not error; got %s: %s", result.Status, result.Output)
}
if strings.Contains(result.Output, "must be greater than or equal to start_line") {
t.Fatalf("backwards range still hard-errors: %q", result.Output)
}
for _, want := range []string{
"end_line 2 was before start_line 3", // the note
"only line 3 was read",
"3 | gamma",
} {
if !strings.Contains(result.Output, want) {
t.Fatalf("expected %q in output, got %q", want, result.Output)
}
}
// It read exactly start_line, nothing adjacent.
if strings.Contains(result.Output, "beta") || strings.Contains(result.Output, "delta") {
t.Fatalf("recovery read the wrong lines: %q", result.Output)
}
}
// A valid range with end_line >= start_line is byte-for-byte unchanged: the
// recovery path must not touch normal reads (no stray note, exact same window).
func TestReadFileToolValidRangeUnchangedByRecovery(t *testing.T) {
root := t.TempDir()
writeTestFile(t, filepath.Join(root, "notes.txt"), "alpha\nbeta\ngamma\ndelta\nepsilon")
result := NewReadFileTool(root).Run(context.Background(), map[string]any{
"path": "notes.txt",
"start_line": 2,
"end_line": 4,
})
if result.Status != StatusOK {
t.Fatalf("valid range failed: %s: %s", result.Status, result.Output)
}
// Byte-for-byte: header, blank separator, the exact selected lines, and no
// recovery note or trailing content. An altered header/separator or a stray
// out-of-range line would fail this where a substring check would not.
const want = "File: notes.txt (lines 2-4 of 5)\n\n2 | beta\n3 | gamma\n4 | delta"
if result.Output != want {
t.Fatalf("valid-range output changed by the recovery path:\n got: %q\nwant: %q", result.Output, want)
}
}
+17 -2
View File
@@ -106,8 +106,18 @@ func renderReadFileRange(absolutePath string, relativePath string, total int, st
if endLine == 0 || endLine > total {
endLine = total
}
// A backwards range (end_line < start_line) is a caller slip, not a fatal
// error. Recover the way an end_line past EOF is already recovered a few lines
// up (clamped down to the last line): clamp end_line UP to start_line and read
// just that one line. Erroring here instead cost the caller a whole retry for
// an off-by-arithmetic mistake, and it was inconsistent — start_line past EOF
// and end_line past EOF both recover gracefully; only this case hard-failed.
// The recovery is surfaced in the output (rangeNote) so it is never silent: the
// caller sees it read a different range than it asked for and why.
rangeNote := ""
if endLine < startLine {
return errorResult("Error: Invalid arguments for read_file: end_line must be greater than or equal to start_line")
rangeNote = fmt.Sprintf("(note: end_line %d was before start_line %d, so only line %d was read — pass end_line >= start_line to read a wider range)", endLine, startLine, startLine)
endLine = startLine
}
truncated := false
@@ -135,7 +145,12 @@ func renderReadFileRange(absolutePath string, relativePath string, total int, st
budgetedOutput = newHeadTailOutputBudgetBuilder(readOutputBudgetBytes, "use start_line/end_line or max_lines to continue with a smaller range")
}
budgetedOutput.WriteString(header)
budgetedOutput.WriteString("\n\n")
budgetedOutput.WriteString("\n")
if rangeNote != "" {
budgetedOutput.WriteString(rangeNote)
budgetedOutput.WriteString("\n")
}
budgetedOutput.WriteString("\n")
if err := appendReadFileRange(budgetedOutput, absolutePath, startLine, selectedLines, width); err != nil {
return errorResult("Error reading file " + relativePath + ": " + err.Error())
}
+56 -2
View File
@@ -689,6 +689,17 @@ type pendingPermissionPrompt struct {
// resting approval choice. Moved by ↑/↓/Tab; confirmed by Enter or a click.
// Hotkeys resolve the matching request-provided option directly.
cursor int
// typing is true once the user chose "tell Zero what to do differently": the
// card replaces its option list with a free-text field (sharing the composer
// input, like the ask_user questionnaire). Submitting sends a Deny decision
// whose Reason is the typed text, so the model reads it as the tool result and
// adjusts course in the same turn instead of the run being cancelled.
typing bool
// savedDraft holds whatever was in the shared composer input when feedback
// mode was entered. The field is cleared for typing and restored on both
// submit and cancel, so a half-typed or queued next-turn message survives the
// detour (permissionRequestMsg, unlike ask_user, does not clear the composer).
savedDraft string
}
// askUserRequestMsg is the TUI-loop equivalent of permissionRequestMsg: the
@@ -1439,6 +1450,11 @@ func (m model) updateModel(msg tea.Msg) (tea.Model, tea.Cmd) {
if m.pendingAskUser != nil {
return m.escapeAskUser()
}
// Esc in the permission feedback field steps back to the option list
// rather than resolving, so a stray keystroke is recoverable.
if m.pendingPermission != nil && m.pendingPermission.typing {
return m.cancelPermissionTyping()
}
if m.pendingSpecReview != nil {
m.burstCount = 0
return m.cancelSpecReview()
@@ -1659,6 +1675,16 @@ func (m model) updateModel(msg tea.Msg) (tea.Model, tea.Cmd) {
return m, nil
}
case keyBackspace(msg):
// In permission feedback mode Backspace is a plain edit of the feedback
// text. This case runs before the typing branch below and, on an empty
// field (feedback mode clears the composer), would otherwise fall to the
// removeLastAttachment path and silently drop a staged image/doc that
// savedDraft does not restore. Route it to the shared input instead.
if m.pendingPermission != nil && m.pendingPermission.typing {
var cmd tea.Cmd
m.input, cmd = m.input.Update(msg)
return m, cmd
}
if m.picker != nil {
if m.modelPickerIsLoading() {
return m, nil
@@ -1854,6 +1880,15 @@ func (m model) updateModel(msg tea.Msg) (tea.Model, tea.Cmd) {
return m.handleSpecReviewKey(msg)
}
if m.pendingPermission != nil {
// Feedback mode: a printable keystroke (and editing keys like
// backspace) types into the shared composer input, mirroring the
// ask_user free-text path above. Enter/Esc/↑/↓ were already handled
// earlier in this switch; the remaining keys reach the input here.
if m.pendingPermission.typing {
var cmd tea.Cmd
m.input, cmd = m.input.Update(msg)
return m, cmd
}
m.burstCount = 0
return m.handlePermissionKey(msg)
}
@@ -2784,6 +2819,16 @@ func (m model) footerView(width int) string {
footer.WriteString(m.statusLine(width))
return footer.String()
}
// A focused permission prompt owns the keyboard: its options (and the feedback
// field) consume every key, so the composer is inert. Suppress it and the idle
// hints/plan panel like the ask_user modal above, keeping only the status line.
// The card itself renders in the transcript body. This also keeps the shared
// input from echoing in two places once "tell Zero what to do differently"
// opens the on-card feedback field.
if m.pendingPermission != nil {
footer.WriteString(m.statusLine(width))
return footer.String()
}
// Pinned plan panel: sits directly above the composer so it stays visible
// while the transcript scrolls underneath (a streaming turn no longer pushes
// the plan off-screen). Budgeted to at most a third of the screen height; a
@@ -3942,13 +3987,22 @@ func (m model) handlePermissionKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) {
key := strings.ToLower(msg.String())
for _, option := range permissionOptions(m.pendingPermission.request) {
if option.hotkey == key {
return m.resolvePermission(option.choice)
return m.choosePermissionOption(option.choice)
}
}
return m, nil
}
func (m model) resolvePermission(decision permissionDecision) (tea.Model, tea.Cmd) {
return m.resolvePermissionWithReason(decision, permissionDecisionReason(decision))
}
// resolvePermissionWithReason resolves the pending prompt with an explicit reason
// string. It backs both the fixed-label choices (reason = permissionDecisionReason)
// and the free-text "tell Zero what to do differently" path, where the reason is
// the user's typed instruction and the action is Deny so the agent surfaces it as
// the tool result and keeps going.
func (m model) resolvePermissionWithReason(decision permissionDecision, reason string) (tea.Model, tea.Cmd) {
pending := m.pendingPermission
if pending == nil {
return m, nil
@@ -3957,7 +4011,7 @@ func (m model) resolvePermission(decision permissionDecision) (tea.Model, tea.Cm
if pending.decide != nil {
pending.decide(agent.PermissionDecision{
Action: decision,
Reason: permissionDecisionReason(decision),
Reason: reason,
})
}
m.pendingPermission = nil
+63 -3
View File
@@ -1,6 +1,8 @@
package tui
import (
"strings"
tea "charm.land/bubbletea/v2"
"github.com/Gitlawb/zero/internal/agent"
@@ -74,7 +76,9 @@ func clampPermissionCursor(cursor int, request agent.PermissionRequest) int {
// the ends. A no-op when no permission prompt is pending. The cursor lives on the
// pending prompt (a pointer), mirroring how the picker's selection moves.
func (m model) movePermissionCursor(delta int) model {
if m.pendingPermission == nil {
if m.pendingPermission == nil || m.pendingPermission.typing {
// While typing feedback the arrow/Tab keys belong to the text field, not
// the option list.
return m
}
n := len(permissionOptions(m.pendingPermission.request))
@@ -87,11 +91,67 @@ func (m model) movePermissionCursor(delta int) model {
}
// confirmPermissionCursor resolves the currently highlighted option. It is the
// Enter-key counterpart to the a/y/d hotkeys and a mouse click.
// Enter-key counterpart to the a/y/d hotkeys and a mouse click. Confirming the
// "tell Zero what to do differently" choice opens the inline feedback field
// instead of resolving immediately.
func (m model) confirmPermissionCursor() (tea.Model, tea.Cmd) {
if m.pendingPermission == nil {
return m, nil
}
if m.pendingPermission.typing {
return m.submitPermissionFeedback()
}
option := permissionOptions(m.pendingPermission.request)[clampPermissionCursor(m.pendingPermission.cursor, m.pendingPermission.request)]
return m.resolvePermission(option.choice)
return m.choosePermissionOption(option.choice)
}
// choosePermissionOption applies a chosen decision. The cancel choice (the
// "tell Zero what to do differently" row and its [n] hotkey) opens the inline
// feedback field rather than aborting the run; every other choice resolves
// immediately as before.
func (m model) choosePermissionOption(choice permissionDecision) (tea.Model, tea.Cmd) {
if m.pendingPermission == nil {
return m, nil
}
if choice == permissionDecisionCancel {
m.pendingPermission.typing = true
// Preserve whatever the user had drafted/queued in the composer so it is
// restored when they leave feedback mode (submit or cancel).
m.pendingPermission.savedDraft = m.input.Value()
m.input.SetValue("")
return m, nil
}
return m.resolvePermission(choice)
}
// submitPermissionFeedback ends the feedback field. Non-empty text is sent as a
// Deny decision whose Reason is the text: the agent surfaces that as the tool
// result (deniedPermissionResult) so the model reads the instruction and adjusts
// in the same turn, rather than the run being cancelled. Empty text falls back to
// a plain cancel, matching the option's prior behaviour.
func (m model) submitPermissionFeedback() (tea.Model, tea.Cmd) {
if m.pendingPermission == nil {
return m, nil
}
feedback := strings.TrimSpace(m.input.Value())
// Restore the composer draft the user had before entering feedback mode; the
// feedback text itself is delivered via the decision Reason, not the composer.
m.input.SetValue(m.pendingPermission.savedDraft)
m.pendingPermission.typing = false
if feedback == "" {
return m.resolvePermission(permissionDecisionCancel)
}
return m.resolvePermissionWithReason(permissionDecisionDeny, feedback)
}
// cancelPermissionTyping returns from the feedback field to the option list
// without resolving, so Esc is a safe "I didn't mean to type" back-out.
func (m model) cancelPermissionTyping() (tea.Model, tea.Cmd) {
if m.pendingPermission == nil || !m.pendingPermission.typing {
return m, nil
}
m.pendingPermission.typing = false
m.input.SetValue(m.pendingPermission.savedDraft)
m.pendingPermission.savedDraft = ""
return m, nil
}
+254 -16
View File
@@ -2,6 +2,7 @@ package tui
import (
"context"
"fmt"
"strings"
"testing"
@@ -10,6 +11,7 @@ import (
"github.com/Gitlawb/zero/internal/agent"
"github.com/Gitlawb/zero/internal/sandbox"
"github.com/Gitlawb/zero/internal/tools"
"github.com/Gitlawb/zero/internal/zeroruntime"
)
func pendingPermissionModel(t *testing.T, decide func(agent.PermissionDecision)) model {
@@ -72,7 +74,7 @@ func TestPermissionOptionsExposeApprovalCancelWhenSupplied(t *testing.T) {
t.Fatalf("cancel option = %#v, want cancel on n", options[3])
}
card, _ := renderFocusedPermissionPrompt(request, 3, 80)
card, _ := renderFocusedPermissionPrompt(request, 3, false, "", 80)
got := plainRender(t, card)
for _, want := range []string{"continue without running it", "[d]", "tell Zero what to do differently", "[n]"} {
if !strings.Contains(got, want) {
@@ -95,7 +97,7 @@ func TestPermissionOptionsExposeCommandPrefixApproval(t *testing.T) {
if len(options) != 3 || options[1].choice != permissionDecisionAllowPrefix || options[1].hotkey != "p" {
t.Fatalf("prefix option = %#v, want p hotkey in supplied order", options)
}
card, _ := renderFocusedPermissionPrompt(request, 1, 100)
card, _ := renderFocusedPermissionPrompt(request, 1, false, "", 100)
got := plainRender(t, card)
for _, want := range []string{"allow `git status` in this session", "[p]"} {
if !strings.Contains(got, want) {
@@ -114,7 +116,7 @@ func TestPermissionPromptMapsEscalatedSandboxReason(t *testing.T) {
agent.PermissionDecisionDeny,
},
}
card, _ := renderFocusedPermissionPrompt(request, 0, 96)
card, _ := renderFocusedPermissionPrompt(request, 0, false, "", 96)
got := plainRender(t, card)
if !strings.Contains(got, "This command needs to run outside the sandbox.") {
t.Fatalf("permission card = %q, missing user-facing sandbox reason", got)
@@ -139,7 +141,7 @@ func TestPermissionOptionsExposePersistentCommandPrefixApproval(t *testing.T) {
if len(options) != 4 || options[2].choice != permissionDecisionAlwaysAllowPrefix || options[2].hotkey != "y" {
t.Fatalf("persistent prefix option = %#v, want y hotkey in supplied order", options)
}
card, _ := renderFocusedPermissionPrompt(request, 2, 100)
card, _ := renderFocusedPermissionPrompt(request, 2, false, "", 100)
got := plainRender(t, card)
for _, want := range []string{"always allow `git status`", "[y]"} {
if !strings.Contains(got, want) {
@@ -157,7 +159,7 @@ func TestPermissionOptionsCanExposePatchCancelWithoutRecoverableDeny(t *testing.
agent.PermissionDecisionCancel,
},
}
card, _ := renderFocusedPermissionPrompt(request, 2, 80)
card, _ := renderFocusedPermissionPrompt(request, 2, false, "", 80)
got := plainRender(t, card)
if !strings.Contains(got, "tell Zero what to do differently") || !strings.Contains(got, "[n]") {
t.Fatalf("permission card = %q, missing cancel option", got)
@@ -180,7 +182,7 @@ func TestRequestPermissionsPromptUsesGrantLabelsAndEscDenies(t *testing.T) {
agent.PermissionDecisionDeny,
},
}
card, _ := renderFocusedPermissionPrompt(request, 1, 96)
card, _ := renderFocusedPermissionPrompt(request, 1, false, "", 96)
got := plainRender(t, card)
for _, want := range []string{
"Grant requested permissions?",
@@ -255,7 +257,9 @@ func TestPermissionHotkeysStillResolveDirectly(t *testing.T) {
}
}
func TestPermissionCancelHotkeyResolvesDirectly(t *testing.T) {
// feedbackRequest is a bash prompt whose decision set includes Cancel, so the
// "tell Zero what to do differently" row (and its [n] hotkey) is present.
func feedbackRequest() agent.PermissionRequest {
request := testPromptPermissionRequest()
request.ToolName = "bash"
request.AvailableDecisions = []agent.PermissionDecisionAction{
@@ -263,21 +267,87 @@ func TestPermissionCancelHotkeyResolvesDirectly(t *testing.T) {
agent.PermissionDecisionDeny,
agent.PermissionDecisionCancel,
}
got := []permissionDecision{}
m := pendingPermissionModelWithRequest(t, request, func(d agent.PermissionDecision) {
got = append(got, permissionDecision(d.Action))
return request
}
// The [n] "tell Zero what to do differently" hotkey no longer resolves cancel
// immediately: it opens the inline feedback field and sends nothing yet.
func TestPermissionTellDifferentlyOpensFeedbackField(t *testing.T) {
var got []agent.PermissionDecision
m := pendingPermissionModelWithRequest(t, feedbackRequest(), func(d agent.PermissionDecision) {
got = append(got, d)
})
if _, cmd := m.Update(testKeyText("n")); cmd != nil {
t.Fatal("'n' should resolve synchronously")
next, _ := m.Update(testKeyText("n"))
nm := next.(model)
if nm.pendingPermission == nil || !nm.pendingPermission.typing {
t.Fatalf("'n' should open the feedback field, got pending=%#v", nm.pendingPermission)
}
if len(got) != 1 || got[0] != permissionDecisionCancel {
t.Fatalf("'n' should resolve cancel directly, got %#v", got)
if len(got) != 0 {
t.Fatalf("'n' must not resolve anything yet, got %#v", got)
}
}
// Typing an instruction and pressing Enter sends a Deny whose Reason is the
// text, so the agent surfaces it to the model as the tool result.
func TestPermissionFeedbackSubmitSendsDenyWithText(t *testing.T) {
var got []agent.PermissionDecision
m := pendingPermissionModelWithRequest(t, feedbackRequest(), func(d agent.PermissionDecision) {
got = append(got, d)
})
next, _ := m.Update(testKeyText("n"))
nm := typeRunes(t, next.(model), "use apply_patch instead")
after, _ := nm.Update(testKey(tea.KeyEnter))
am := after.(model)
if len(got) != 1 {
t.Fatalf("Enter should resolve exactly one decision, got %#v", got)
}
if got[0].Action != agent.PermissionDecisionDeny {
t.Fatalf("feedback should resolve as Deny, got %s", got[0].Action)
}
if got[0].Reason != "use apply_patch instead" {
t.Fatalf("Deny reason should carry the typed text, got %q", got[0].Reason)
}
if am.pendingPermission != nil {
t.Fatal("prompt should be dismissed after submitting feedback")
}
}
// Enter with an empty field falls back to a plain cancel (the option's prior
// meaning), so opening the field and changing your mind is safe.
func TestPermissionFeedbackEmptySubmitCancels(t *testing.T) {
var got []agent.PermissionDecision
m := pendingPermissionModelWithRequest(t, feedbackRequest(), func(d agent.PermissionDecision) {
got = append(got, d)
})
next, _ := m.Update(testKeyText("n"))
after, _ := next.(model).Update(testKey(tea.KeyEnter))
_ = after
if len(got) != 1 || got[0].Action != agent.PermissionDecisionCancel {
t.Fatalf("empty feedback should resolve Cancel, got %#v", got)
}
}
// Esc in the feedback field returns to the option list without resolving.
func TestPermissionFeedbackEscReturnsToOptions(t *testing.T) {
var got []agent.PermissionDecision
m := pendingPermissionModelWithRequest(t, feedbackRequest(), func(d agent.PermissionDecision) {
got = append(got, d)
})
next, _ := m.Update(testKeyText("n"))
after, _ := typeRunes(t, next.(model), "half a thought").Update(testKey(tea.KeyEsc))
am := after.(model)
if am.pendingPermission == nil || am.pendingPermission.typing {
t.Fatalf("Esc should return to the option list, got pending=%#v", am.pendingPermission)
}
if len(got) != 0 {
t.Fatalf("Esc must not resolve anything, got %#v", got)
}
}
func TestPermissionRenderEmitsHighlightedClickableOffsets(t *testing.T) {
request := agent.PermissionRequest{ToolName: "write_file", AvailableDecisions: testAllPermissionDecisions()}
card, offsets := renderFocusedPermissionPrompt(request, 2, 60) // cursor on future approval
card, offsets := renderFocusedPermissionPrompt(request, 2, false, "", 60) // cursor on future approval
if len(offsets) != len(permissionOptions(request)) {
t.Fatalf("offsets = %d, want %d", len(offsets), len(permissionOptions(request)))
}
@@ -298,7 +368,7 @@ func TestPermissionRenderShowsNetworkTargetAndHostScopedAlways(t *testing.T) {
Scope: "example.com",
AvailableDecisions: testAllPermissionDecisions(),
}
card, _ := renderFocusedPermissionPrompt(request, 1, 72)
card, _ := renderFocusedPermissionPrompt(request, 1, false, "", 72)
got := plainRender(t, card)
for _, want := range []string{"target: example.com", "allow this host for this conversation", "[s]", "allow this host in the future", "[y]"} {
if !strings.Contains(got, want) {
@@ -376,3 +446,171 @@ func TestShiftDownComposerGuard(t *testing.T) {
t.Fatalf("Shift+Down with non-empty composer scrolled offset to %d, want 3 (unchanged)", got)
}
}
// The highlighted permission option must use the selected-row tint, not the
// brand chip. zeroTheme.badge is the accent-filled chip for short labels
// (" 0 ", " ASK ", " SPEC REVIEW "); using it for a whole row painted a
// full-brightness accent slab across a card whose palette is deliberately amber
// (warning), and skipped the card tint every other line composes onto. selBg is
// the tint tuned for a highlighted row against a panel, and onSel is what every
// other selectable list in the TUI uses.
func TestFocusedPermissionSelectedRowUsesSelectionTintNotBrandChip(t *testing.T) {
request := agent.PermissionRequest{ToolName: "exec_command", SideEffect: "shell"}
card, _ := renderFocusedPermissionPrompt(request, 0, false, "", 70)
var selected string
for _, line := range strings.Split(card, "\n") {
if strings.Contains(ansiPattern.ReplaceAllString(line, ""), "▸ ") {
selected = line
break
}
}
if selected == "" {
t.Fatal("no highlighted option row rendered")
}
accentBg := backgroundCode(darkPalette.accent)
selBg := backgroundCode(darkPalette.selBg)
if strings.Contains(selected, accentBg) {
t.Errorf("selected row is filled with the brand accent %s (zeroTheme.badge); want the selection tint:\n%q", darkPalette.accent, selected)
}
if !strings.Contains(selected, selBg) {
t.Errorf("selected row should carry the selection tint %s:\n%q", darkPalette.selBg, selected)
}
// The PERMISSION chip keeps its amber fill — the card must still read as a
// warning surface, so this fix must not flatten it.
if !strings.Contains(card, backgroundCode(darkPalette.amber)) {
t.Errorf("PERMISSION badge lost its amber fill:\n%q", card)
}
// The card BODY carries no warm permBg wash any more: it matches the other
// prompt cards (ask_user, spec) whose bodies are transparent. Warning identity
// comes from the amber badge + border, not a full-body tint that clashes on
// cool themes.
if strings.Contains(card, backgroundCode(darkPalette.permBg)) {
t.Errorf("permission card body still tinted with permBg %s; want a transparent body:\n%q", darkPalette.permBg, card)
}
}
// backgroundCode renders the SGR truecolor background sequence for a #rrggbb
// palette entry, so assertions compare against the palette rather than
// hardcoded numbers that drift when a theme is retuned.
func backgroundCode(hex string) string {
var r, g, b int
fmt.Sscanf(hex, "#%02x%02x%02x", &r, &g, &b)
return fmt.Sprintf("48;2;%d;%d;%d", r, g, b)
}
// A focused permission prompt suppresses the composer: keys drive the card, so a
// "describe a task for zero…" box below it is inert and misleading — and once the
// feedback field is open, the shared input must not echo in both places.
func TestComposerSuppressedDuringPermissionPrompt(t *testing.T) {
m := pendingPermissionModelWithRequest(t, feedbackRequest(), func(agent.PermissionDecision) {})
m.width, m.height = 96, 30
view := plainRender(t, m.View())
if strings.Contains(view, "describe a task for zero") {
t.Errorf("composer must be hidden while a permission prompt is focused:\n%s", view)
}
// While typing feedback, the text lives on the card, not in a second box.
next, _ := m.Update(testKeyText("n"))
typed := typeRunes(t, next.(model), "use apply_patch")
tv := plainRender(t, typed.View())
if strings.Contains(tv, "describe a task for zero") {
t.Errorf("composer must stay hidden in feedback mode:\n%s", tv)
}
if strings.Count(tv, "use apply_patch") != 1 {
t.Errorf("feedback text should appear exactly once (on the card), got %d occurrences:\n%s", strings.Count(tv, "use apply_patch"), tv)
}
}
// Entering feedback mode must not lose a composer draft (a half-typed or queued
// next-turn message). The draft is restored whether feedback is submitted or
// cancelled; only the feedback text (delivered via the decision Reason) is
// consumed.
func TestPermissionFeedbackPreservesComposerDraftOnSubmit(t *testing.T) {
var got []agent.PermissionDecision
m := pendingPermissionModelWithRequest(t, feedbackRequest(), func(d agent.PermissionDecision) {
got = append(got, d)
})
m.input.SetValue("my queued next message")
next, _ := m.Update(testKeyText("n"))
nm := next.(model)
if nm.input.Value() != "" {
t.Fatalf("feedback field should start empty, got %q", nm.input.Value())
}
nm = typeRunes(t, nm, "use apply_patch")
after, _ := nm.Update(testKey(tea.KeyEnter))
am := after.(model)
if len(got) != 1 {
t.Fatalf("expected exactly one permission decision, got %#v", got)
}
if got[0].Reason != "use apply_patch" {
t.Fatalf("feedback text lost: %q", got[0].Reason)
}
if am.input.Value() != "my queued next message" {
t.Fatalf("composer draft not restored after submit, got %q", am.input.Value())
}
}
func TestPermissionFeedbackPreservesComposerDraftOnCancel(t *testing.T) {
m := pendingPermissionModelWithRequest(t, feedbackRequest(), func(agent.PermissionDecision) {})
m.input.SetValue("my queued next message")
next, _ := m.Update(testKeyText("n"))
nm := typeRunes(t, next.(model), "half a thought")
after, _ := nm.Update(testKey(tea.KeyEsc))
am := after.(model)
if am.pendingPermission == nil || am.pendingPermission.typing {
t.Fatal("Esc should return to the option list")
}
if am.input.Value() != "my queued next message" {
t.Fatalf("composer draft not restored after cancel, got %q", am.input.Value())
}
}
// Backspace on an empty permission feedback field must edit the feedback text,
// never drop a staged attachment. The keyBackspace case runs before the typing
// branch and, on an empty composer, would otherwise remove the last staged
// image/doc — which savedDraft does not restore.
func TestPermissionFeedbackBackspaceKeepsStagedAttachment(t *testing.T) {
m := pendingPermissionModelWithRequest(t, feedbackRequest(), func(agent.PermissionDecision) {})
m.pendingImages = []zeroruntime.ImageBlock{{MediaType: "image/png"}}
m.pendingImageLabels = []string{"diagram.png"}
next, _ := m.Update(testKeyText("n")) // open feedback field (clears composer text)
after, _ := next.(model).Update(testKey(tea.KeyBackspace))
am := after.(model)
if len(am.pendingImageLabels) != 1 || len(am.pendingImages) != 1 {
t.Fatalf("Backspace in feedback mode dropped the staged attachment: labels=%v imgs=%d", am.pendingImageLabels, len(am.pendingImages))
}
if !am.pendingPermission.typing {
t.Fatal("Backspace should stay in the feedback field, not exit it")
}
}
// No permission option row is clickable while the feedback field is open: the
// renderer registers zero clickable offsets in typing mode, so a stray click
// (Allow included) has no row to land on. This is the primary safety; the
// !typing guard on the click resolver in handleTranscriptSelectionMouse is the
// explicit second layer for if this early-return is ever refactored away.
func TestPermissionFeedbackRendersNoClickableOptionsWhileTyping(t *testing.T) {
request := feedbackRequest()
// Option mode: rows are clickable.
_, optOffsets := renderFocusedPermissionPrompt(request, 0, false, "", 80)
if len(optOffsets) == 0 {
t.Fatal("precondition: option rows should be clickable in option mode")
}
// Feedback mode: no clickable rows.
_, typingOffsets := renderFocusedPermissionPrompt(request, 0, true, "some feedback", 80)
if typingOffsets != nil {
t.Fatalf("feedback mode must register no clickable option offsets, got %#v", typingOffsets)
}
}
+30 -4
View File
@@ -1107,12 +1107,19 @@ func wrapDetailBlock(detail string, width int) string {
// renderFocusedPermissionPrompt draws the modal permission card and reports the
// card-relative Y offset of each option line (in permissionOptions order) so the
// caller can register those lines as clickable.
func renderFocusedPermissionPrompt(request agent.PermissionRequest, cursor int, width int) (string, []int) {
func renderFocusedPermissionPrompt(request agent.PermissionRequest, cursor int, typing bool, feedback string, width int) (string, []int) {
name := strings.TrimSpace(request.ToolName)
if name == "" {
name = "tool"
}
fill := zeroTheme.onPerm
// The card body carries no background fill, matching every other prompt card
// (ask_user, spec review, plan) — see the lipgloss.NewStyle() fills below. The
// permission card used to tint its whole body with permBg, an amber-family
// wash that reads as a warm slab on cool themes (e.g. a brown-yellow box over
// dracula's purples) and made it the one outlier. The amber PERMISSION badge
// and the amber-mixed border still carry the "this is a permission gate"
// signal; the body no longer clashes with the surrounding theme.
fill := func(style lipgloss.Style) lipgloss.Style { return style }
top := zeroTheme.permBadge.Render(" PERMISSION ")
@@ -1134,6 +1141,17 @@ func renderFocusedPermissionPrompt(request agent.PermissionRequest, cursor int,
lines = append(lines, "")
// Feedback mode: the option list is replaced by a free-text field, like the
// ask_user "type your own answer" surface. What is typed is sent to the model
// as the denial reason, so it reads the instruction and adjusts.
if typing {
lines = append(lines, fill(zeroTheme.muted).Render("Tell Zero what to do differently:"))
lines = append(lines, zeroTheme.userPrompt.Render("❯ ")+fill(zeroTheme.ink).Render(feedback)+fill(zeroTheme.accent).Render("▌"))
lines = append(lines, "")
lines = append(lines, fill(zeroTheme.faint).Render("enter · send to Zero esc · back to options"))
return styledBlockFill(width, lines, zeroTheme.permBorder, zeroTheme.permBg), nil
}
// Each option is its own line so a click anywhere on that row selects it (no
// per-column hit-testing). The highlighted row gets a ▸ marker and a reverse
// label; the rest stay quiet. styledBlockFill prepends exactly one top-border
@@ -1147,8 +1165,16 @@ func renderFocusedPermissionPrompt(request agent.PermissionRequest, cursor int,
hotkey := fill(zeroTheme.faint).Render(" [" + option.hotkey + "]")
optionLabel := permissionOptionLabel(option, request)
if index == cursor {
// onSel, not badge. zeroTheme.badge is the brand chip (" 0 ", " ASK ",
// " SPEC REVIEW ") — a full-brightness accent fill meant for short
// labels. Using it for a selected ROW painted a bright accent slab
// across the permission card, fighting the card's amber warning palette
// and ignoring the card tint every other line composes onto. selBg is
// the tint tuned for exactly this job ("separates from the panel while
// ink label contrast stays ~9.4:1"), and onSel is what every other
// selectable list in the TUI uses for its highlighted row.
marker := fill(zeroTheme.accent).Render("▸ ")
label := zeroTheme.badge.Render(" " + optionLabel + " ")
label := zeroTheme.onSel(zeroTheme.ink).Bold(true).Render(" " + optionLabel + " ")
lines = append(lines, marker+label+hotkey)
} else {
label := fill(zeroTheme.ink).Render(optionLabel)
@@ -1163,7 +1189,7 @@ func renderFocusedPermissionPrompt(request agent.PermissionRequest, cursor int,
}
lines = append(lines, fill(zeroTheme.faint).Render(footer))
return styledBlockFill(width, lines, zeroTheme.permBorder, zeroTheme.permBg), offsets
return styledBlockFill(width, lines, zeroTheme.permBorder, lipgloss.NewStyle()), offsets
}
func permissionScopeLine(request agent.PermissionRequest, scope string) string {
+1 -1
View File
@@ -1659,7 +1659,7 @@ func TestFocusedPermissionCardShowsBadgeAndKeys(t *testing.T) {
Risk: sandbox.Risk{Level: sandbox.RiskMedium},
AvailableDecisions: testAllPermissionDecisions(),
}
card, offsets := renderFocusedPermissionPrompt(request, 0, 80)
card, offsets := renderFocusedPermissionPrompt(request, 0, false, "", 80)
got := plainRender(t, card)
for _, want := range []string{"PERMISSION", "edit_file", "writes internal/agent/exec.go", "Yes, proceed", "[a]", "this session", "[s]", "don't ask again", "[y]", "continue without running it", "[d]", "[esc]"} {
if !strings.Contains(got, want) {
+9 -3
View File
@@ -454,7 +454,7 @@ func (m model) buildTranscriptBodyItems(width int, emptyOverlay string, detailed
rowIndex: -1,
heightCacheStable: false, // the highlight changes with the cursor
render: func(startBodyY int) transcriptBodyRenderedItem {
block, offsets := renderFocusedPermissionPrompt(perm.request, perm.cursor, width)
block, offsets := renderFocusedPermissionPrompt(perm.request, perm.cursor, perm.typing, m.input.Value(), width)
options := permissionOptions(perm.request)
selectable := make([]transcriptSelectableLine, 0, len(offsets))
for index, offset := range offsets {
@@ -1401,8 +1401,14 @@ func (m model) handleTranscriptSelectionMouse(msg tea.MouseMsg) (model, tea.Cmd,
}
return m, nil, false
}
if line.permOption {
// A left-click on a permission-popup option resolves it directly.
if line.permOption && !(m.pendingPermission != nil && m.pendingPermission.typing) {
// A left-click on a permission-popup option resolves it directly. The
// typing guard is defence-in-depth: renderFocusedPermissionPrompt already
// returns nil offsets in feedback mode, so no option row is registered as
// clickable then — but that single early-return is the only thing keeping
// a stray click (Allow included) off the decision path, and it lives in a
// function other PRs also edit. Guarding here makes the safety explicit
// rather than emergent.
next, cmd := m.resolvePermission(line.permChoice)
return next.(model), cmd, true
}