Compare commits

...
Author SHA1 Message Date
Vasanthdev2004 73d83c469d fix(acp): validate a permission reply against the options that were sent
CodeRabbit caught a hole on the same seam this PR already fixes, in the
other direction.

buildPermissionOptions drops PermissionDecisionCancel, because ACP
expresses cancellation through the outcome rather than an option. But
ZERO enumerates cancel in AvailableDecisions for shell commands and
apply_patch — the two commonest prompts — so validating against the
decisions accepted {"outcome":"selected","optionId":"cancel"}: an
identifier no client was ever shown, aborting the whole turn.

Rather than reject cancel by name, decisionFromOutcome now takes the
[]PermissionOption that were actually sent, and requestPermission builds
that list once and uses it for both halves. "What we offered" and "what we
accept" become the same slice instead of two things kept in step, so
anything optionKindFor declines to render is excluded structurally — the
next such action is covered the day it is added.

The regression test is written over every option-less decision rather
than over cancel specifically, for the same reason. Cancelling through
the outcome still works, which is asserted separately: closing the
selected-id route must not close the real one.

Also adds the cancellation coverage CodeRabbit asked for on the two
sandbox-retry paths. Both wrap ErrPermissionApprovalCanceled and each is
reached by a different denial, and the ACP mapping in this PR depends on
them producing something recognisable. They also assert the escalated
retry does not run: a cancelled prompt that still ran it would run the
command the user had just declined.
2026-08-17 15:01:48 +05:30
Vasanthdev2004 854064face refactor(acp): drop the session/load replay, superseded by #914
gnanam's #914 lands the same fix and does more with it: the replayed
messages carry stable ids derived from the store's event ids, session/load
replays while session/resume deliberately does not, and both are
capability-gated. Keeping a second, weaker replay here would have meant a
conflict in handleSessionLoad and translate.go for no gain.

translate.go is back to its state on main. What remains are the three
defects #914 does not touch: the permission option that could not be
accepted, the cancel that arrived as an internal error, and the two
options labelled the same.
2026-08-17 14:38:02 +05:30
Vasanthdev2004 de624f8fe8 fix(acp): four defects a desktop ACP client hits
Found by driving `zero acp` from ZeroApp and comparing both sides of the
wire. None of them fail loudly — they produce a blank transcript, a
silent denial, a spurious crash, or two buttons nobody can tell apart.

session/load restored the conversation into the agent's own memory and
sent nothing. The turns were read from the store and used to build every
later prompt, but not one notification went out, so a client that resumed
opened an empty transcript against a model that remembered everything.
Ask a follow-up and ZERO answers correctly, about a question no longer on
screen. It now replays the loaded turns as user/agent message chunks
before returning, so a client rendering on the reply already has them.
Whole messages rather than per-token deltas: this is a transcript, not a
stream.

A permission option we offered could not be accepted. The fallback list
for a request with no AvailableDecisions lived inside
buildPermissionOptions, while requestPermission validated the client's
answer against the raw field — so whenever ZERO did not enumerate, the
client was sent Allow and Reject and its reply was checked against an
empty slice. Every button it could possibly show failed closed to deny
with "permission option was not offered". The user clicked Allow and ZERO
recorded a denial, and nothing surfaced it, because a denial is a
legitimate answer. One resolver now feeds both, so they agree by
construction. The narrowing this validation exists for is unchanged: an
unoffered broader grant is still refused.

Cancelling a permission prompt came back as an internal error. Only
context.Canceled was recognised, so errPermissionApprovalCanceled fell
through to -32603 carrying the sentinel text, and clients render that as
a failed turn — declining a tool looked like ZERO falling over. For
apply_patch, dismissing the dialog is the only refusal a client is
offered. It maps to StopCancelled now. The sentinel is exported so the
surfaces can recognise it; a genuine failure is still an error.

request_permissions labelled two different decisions "Allow". The label
is the only thing an ACP client shows, and plain allow and strict-review
allow are offered together, so the panel had two identical buttons where
one silently enabled strict auto-review of what was granted. The
distinction cannot live in the option id, which is opaque to the client,
so it is in the name.

Eleven tests, each confirmed to fail without its fix.
2026-08-17 14:32:15 +05:30
d065467cd3 feat(zerogit): auto-create a conventional branch before push/pr on default branch (#671)
* feat(zerogit): auto-create a conventional branch before push/pr on default branch

zero changes push/pr refused to push straight to the default branch but
never offered an alternative, unlike other agent tooling that names and
checks out a feature branch automatically. Add CreateBranch, IsDefaultBranch,
CurrentGitUser, and branch-name slugify/build helpers to zerogit, and wire
push/pr to auto-create a "<user>/<slug>" branch (slug from an LLM when a
provider is configured, otherwise a deterministic fallback from the diff)
whenever the current branch is the default one and --yes/--dry-run weren't
passed.

* fix(zerogit): normalize LLM branch slugs, bound remote lookup, handle existing branches

Address CodeRabbit's review on the auto-branch-naming PR:

- generateAutoBranchSlug now takes the first non-empty, quote-trimmed
  line of the model's response before slugifying, instead of folding
  any preamble or wrapping quotes into the branch name.
- isDefaultBranch's ls-remote lookup is now bounded by a 5s timeout so
  a slow or unreachable remote can't stall push/pr; falls back to the
  local main/master heuristic as before.
- CreateBranch checks whether the target branch already exists locally
  and checks it out instead of failing checkout -b on the collision.

Also add regression coverage: CurrentGitUser's OS-username fallback
tier, CreateBranch's existing-branch and generic checkout-failure
paths, a CLI-level test for changes pr (previously untested) mirroring
the existing changes push coverage, and a messy-LLM-response case for
the slug normalization fix.

* fix(zerogit,cli): make auto-branching collision-safe, remote-aware, fail-closed, and opt-in for LLM naming

- CreateBranch never checks out an existing branch on a name collision: it
  picks a unique suffixed name off the current HEAD (name-2 .. name-9) and
  fails visibly when the namespace is exhausted, so a stale branch with
  unrelated history can no longer be published while the new commit stays
  behind on the default branch.
- ensureFeatureBranch resolves the remote once (explicit --remote, then the
  original branch's configured upstream, then origin) via IsDefaultBranch,
  which now reports the resolved remote, and push/pr thread that remote into
  Push, so a freshly created branch without tracking configuration no longer
  falls back to origin in fork setups.
- The default-branch check fails closed: main and master count as default
  with no network, an unreachable remote falls back to the local
  refs/remotes/<remote>/HEAD record, and when neither answers the push is
  refused with guidance instead of silently dropping the confirmation guard
  for repositories whose default is trunk or develop.
- LLM branch naming is now opt-in via --auto on push/pr, matching commit
  --auto: a configured provider alone no longer causes the change diff to be
  uploaded during ordinary git-only pushes.
- After the ordinary commit-then-push sequence the tree is clean, so the
  fallback name comes from the HEAD commit subject instead of the constant
  user/changes.

* style(zerogit): align fake-runner result comments to gofmt output

* test(zerogit): cover Push's fail-closed path when the default branch is unknown

* fix(zerogit): keep a stale cached remote HEAD from clearing the push guard

When the live ls-remote default-branch lookup fails, isDefaultBranch fell
back to the local refs/remotes/<remote>/HEAD cache and returned
branch == cachedName. That cache is only a hint: if the server renamed its
default (main -> trunk) the record still names main, so checking whether
pushing trunk is safe returned false and the newly protected branch was
pushed without --yes.

Trust the cache only to block a push (a positive match proves the branch is
the recorded default); on a mismatch fall through to the existing
fail-closed unknown-default error instead of treating the branch as
unprotected.

* fix(cli): honor --diff-bytes when inspecting the diff for auto-branch naming

changes push/pr accept and document --diff-bytes, but ensureFeatureBranch
inspected the working tree with only Cwd set. With --auto the resulting
unbounded diff was embedded in the branch-name request to the provider, so a
user who capped the diff to limit how much proprietary source is uploaded
for naming still sent the complete diff.

Thread options.maxDiffBytes through ensureFeatureBranch into InspectOptions,
matching how the commit path already bounds the diff.

* fix(cli): refuse an auto-branch push when HEAD has nothing to publish

The auto-branch path decided to branch solely from working-tree status and
never checked that HEAD was ahead of the default branch. On a clean,
up-to-date default branch it would create and push user/<subject> at the
exact default tip; with only uncommitted edits it named a branch from those
edits but pushed the unchanged HEAD, leaving the edits local, and changes pr
then abandoned the empty branch before the host rejected the comparison.

Add zerogit.CommitsAhead and consult it before branching: when HEAD is
provably not ahead of <remote>/<default>, report "no changes to publish"
instead of creating the branch. An indeterminate count (for example a
remote-tracking ref that was never fetched) is treated as "cannot tell" so a
legitimate first push is not blocked.

* fix(cli): recover the branch slug from preamble or fenced LLM replies

The slug helper returned the first non-empty line verbatim, so a reply like
"Here is a suggested branch name:\nadd-login-page" produced
user/here-is-a-suggested-branch-name, and a reply wrapped in a code fence
slugified the ``` line to nothing and silently fell back to the local name.

Drop Markdown code-fence lines and prefer a line that already reads as a
kebab-case slug, falling back to the first quoted/plain line only when no
slug-shaped line exists. Cover preamble and fenced responses with tests.

* fix(zerogit): harden auto-branch remote handling

Three review findings:

- isDefaultBranch passed the remote to ls-remote before the HEAD
  positional with no --, so a remote value shaped like an option
  (--upload-pack=/bin/echo) was parsed as one. Terminate options with --
  and cover a dash-prefixed remote.

- An unborn remote (freshly created, zero refs) answered ls-remote with
  empty output, fell through to the fail-closed unknown-default error,
  and made the very first feature-branch push a --yes dead end that
  git remote set-head --auto cannot repair. ls-remote succeeding with no
  refs now counts as proof there is no protected default; main/master
  stay guarded by the name heuristic and every failure path stays
  fail-closed.

- CreateBranch probed only local refs/heads for collisions, so a branch
  existing only on the target remote (an old merged-PR branch) was
  silently fast-forwarded by the later push -u. The target remote's
  heads are now probed once (bounded, with --) and count as taken;
  an unreachable remote fails visibly before anything is created.

Co-Authored-By: Claude Fable 5 <[email protected]>

* fix(zerogit): close branch-naming and push safety gaps from review

Require a confirmed unborn HEAD, not just an empty ls-remote --symref,
before granting the first-push exception, since a dangling remote
HEAD looked the same. Resolve branch naming and ahead-count checks
against the remote branch instead of a working-tree snapshot, which
could describe uncommitted edits a commit-only push would never
publish. Guard concurrent branch creation with a force-with-lease
push. Scope the ls-remote timeout to the default-branch preflight
only, so it no longer caps Push's own unrelated check.

* fix(cli): require a clean tree before auto-branch push/pr

Refuse ensureFeatureBranch when the working tree is dirty or the
ahead count against the remote default cannot be determined, so a
default-branch push cannot leave uncommitted edits behind or publish
an empty comparison under a guessed name.

* fix(zerogit): drop fixed five-second remote lookup timeout

IsDefaultBranch and CreateBranch applied a hard 5s bound on ls-remote,
so ordinary feature-branch pushes and the collision probe failed on slow
but reachable remotes (SSH/VPN) before Push could run. Honor the
caller's context instead; callers that need a bound pass a deadline.

* fix(cli): bypass the ahead-count and diff-base checks on a confirmed-unborn remote

jatmn flagged that the confirmed-unborn-remote exception this PR added to
IsDefaultBranch never actually helps the main/master case it was written
for: when the local branch is named main, isDefaultBranch returns true from
the name heuristic before it ever consults the remote, so
ensureFeatureBranch still unconditionally calls commitsAhead against
<remote>/<branch>. On a genuinely empty remote that ref cannot exist, so
`zero changes push`/`pr` from a local default branch against a brand-new
empty remote still dead-ended with "cannot determine whether HEAD is
ahead of origin/main" - the exact bug this PR set out to fix.

Tracing the flow further, the same problem also hit the diff-base Inspect
call right after: it uses the same nonexistent <remote>/<branch> ref, so
bypassing only commitsAhead would have left the dead end one line later.

ensureFeatureBranch now probes the new zerogit.IsUnbornRemote (ls-remote
--heads on the remote) whenever commitsAhead fails, and only bypasses both
checks when that probe positively confirms the remote has no refs at all.
An unreachable remote, or one that answers with refs, still fails closed
exactly as before - the fail-closed "unknown default branch" behavior is
unchanged and covered by TestEnsureFeatureBranchFailsWhenAheadCountUnknown
and the new TestEnsureFeatureBranchFailsWhenUnbornCheckErrors.

TestEnsureFeatureBranchCreatesBranchOnConfirmedUnbornRemote is the
regression test: confirmed failing (with the exact dead-end error) against
the pre-fix logic, passing after.

* fix(zerogit): resolve main/master against the live remote default first

isDefaultBranch short-circuited on the literal branch names main/master
before ever consulting the remote. A repository whose remote default is
genuinely something else (e.g. trunk) could still have a local main branch
tracking that remote, and the shortcut wrongly treated it as protected
instead of trusting the live symref result. The conventional-name check
now only applies as a last-resort fallback, after both the live ls-remote
symref lookup and the local remotes/<remote>/HEAD cache fail to answer.

* fix(cli): preserve push lease on retry and check the live default ref

Two related gaps in ensureFeatureBranch's auto-branch preflight:

- When an auto-branch push loses a force-with-lease race against a
  concurrent creator, the generated branch is left checked out locally
  with no successful push behind it. A retry took the "already off the
  default branch" early return and dropped RequireNewRemoteBranch,
  letting the next push silently fast-forward whatever the concurrent
  creator published. The lease now stays required unless the branch
  already has a configured upstream (proof a push already landed).

- The ahead-of-default check compared HEAD against the local
  remote-tracking ref, which is only refreshed at clone or the last
  fetch. A stale ref can under-report how far ahead HEAD is, or say
  zero when the remote has actually moved past it, so ensureFeatureBranch
  now refreshes that tracking ref before trusting it, and fails closed
  if the refresh itself cannot be confirmed.

Adds regression coverage for both: a retry after a lease collision keeps
requiring the lease (and an already-published branch does not), and a
stale tracking ref is refreshed before the ahead check runs (with a
fail-closed case when that refresh cannot be confirmed).

* fix(cli): track generated branches, reject PR on unborn remote, classify main on unborn

* fix(zerogit,cli): preserve explicit nonexistence lease on force, propagate marker write errors, check unborn remote before branch switch

* fix(zerogit): preserve non-existence lease on target remote and roll back failed branch marking

Compare target remote against configured upstream remote to retain non-existence lease on target, roll back created branch if marking fails, and allow --yes to bypass PR default-branch preflight.

Refs #671

* fix(cli): address review findings for auto feature branch creation

* fix(cli): refine LLM preamble candidate slug extraction and enforce unborn-remote guard

* fix(cli): restore default branch after auto-branch and resolve --yes upstream

Do not leave auto-branch commits on the protected local default branch,
and resolve the real upstream for the unborn-remote guard under --yes
instead of always probing origin.

* test(cli): assert auto-branch passes capped diff into the provider

Thread --auto and a mock provider through DiffBytes coverage so the
cap is checked on the real LLM request path, not only Inspect.

* fix(cli): block auto-branch on unborn remotes and fix lease/restore

Refuse auto-creating a feature branch when the target remote has no refs yet.
Publishing only user/slug leaves remote HEAD dangling, so the next changes pr
fails closed and a later main push has no PR diff. Require push --yes first.

Keep the nonexistence lease unless branch@{upstream} is exactly the target
remote/generated-branch ref from a successful push -u. Inherited origin/main
from branch.autoSetupMerge=inherit must not drop the lease.

Restore the original default branch against its own upstream tip, not the
push destination, so --remote upstream does not rewrite a fork main that
tracks origin/main.

* fix(cli): recover push -u upstream and filter LLM slug preambles

Push -u can publish a remote branch then fail to write local upstream
config. Treat that as incomplete, recover with set-upstream-to, and probe
the remote before reasserting the nonexistence lease on retry. Also skip
one-word LLM acknowledgements before accepting a slug-shaped line.

Refs #671

* fix(cli): decide the branch-collision lease with one live remote check

Replace the local-state inference chain (upstream config, a
zeroAutoBranch marker, per-remote provenance tracking) that kept
producing new collision-safety edge cases across many review rounds
with a single live remoteHasBranch check against the destination
remote: missing there gets the nonexistence lease, already there gets
a plain push, and git's own fast-forward check is the safety net if
that existing branch isn't actually ours.

Removes MarkGeneratedBranch/IsGeneratedBranch, now dead, along with
the tests that pinned the specific local-state heuristics rather than
the collision-safety property itself.

* fix(cli,zerogit): address CodeRabbit findings on auto-branch naming

Recover trailing-punctuation LLM slugs, notice deterministic fallback,
report dual restore/rollback failures, and skip Push upstream repair on
dry-run. Align comments with fail-closed and DryRun behavior.

Refs #671

* fix(tools): set explicit stdin reader and use resolved binary for format-on-write

Use the binary path resolved by exec.LookPath and set formatter.Stdin
to an empty reader so format-on-write does not hang on inherited stdin
handles on Windows.

Refs #671

* fix(cli,zerogit,tools): address CodeRabbit review findings on PR 671

Remove redundant command validation, eliminate duplicate remote lookup in PR
workflow, harden ref options and branch switching, isolate test Git
environment, and add formatter lookup test.

Refs #671

* fix(cli,zerogit): address new CodeRabbit review findings on PR 671

Harden CreateBranch name validation, remove unreachable workflow error check,
complete doc comment, isolate test Git environment, and clean up unused
appDeps field.

Refs #671

* fix(cli,zerogit,tools): address third round of CodeRabbit review findings on PR 671

Use featureBranchOptions struct for ensureFeatureBranch, bound preflight
contexts, add upstream repair mismatch subtest, and centralize test Git
config isolation.

Refs #671

* fix(cli,zerogit): finish ensureFeatureBranch doc contract and close review gaps

The ensureFeatureBranch doc comment ended mid-sentence and sat on the featureBranchOptions type, so go doc attached function prose to the struct. Move it above the function and state which deps are required non-nil (inspectChanges, commitsAhead, headCommitSubject, currentGitUser, createBranch) versus optional and nil-guarded (refreshTrackingRef, isUnbornRemote, branchUpstreamRef, resetBranchRef, deleteBranch). Also extract the branch slug filename with path.Base because git paths are always slash-separated, gate two tests on the git version their fixtures need (autoSetupMerge=inherit requires 2.35, init --bare -b requires 2.28), and cover the literal "user" CurrentGitUser fallback via a test seam.

Refs #671

Co-Authored-By: cairn-code <[email protected]>

* Protect auto-branch publication and default ref restoration

Co-Authored-By: cairn-code <[email protected]>

* fix(cli): wire expected-old restore through the dependency seam

The P1 fix added expectedOld to resetBranchRef but left the appDeps field and the workflows.go import behind, so the CLI package did not compile. Update the field type to the 5-argument signature and restore the filepath import used by worktree release resolution.

Co-Authored-By: cairn-code <[email protected]>

* fix(cli,zerogit): wire CAS tip and close review test gaps

Addresses the CodeRabbit round on 93e3448e: fillAppDeps now defaults currentBranchTip so the compare-and-swap old value actually reaches ResetBranchRef in production; TestEnsureFeatureBranchRestoresDefaultBranchAfterCreate asserts the CAS value; the real-git auto-branch test asserts local main is restored to its upstream tip; docs explain the deliberate context.Background push, CurrentBranchTip semantics, the RemoteHasBranch tip-equality ownership check, and the Git 2.24 requirement for CommitsAhead; CreateBranch gains name-validation subtests.

Co-Authored-By: cairn-code <[email protected]>

* fix(zerogit): make RemoteHasBranch report existence only, not tip equality

RemoteHasBranch's tip-equality check made ensureFeatureBranch reassert the
nonexistence lease (--force-with-lease=<branch>:) on every push after the
first: a second local commit puts HEAD ahead of the just-published remote
tip, so the check reported false for a branch that plainly exists, and Git
rejected the follow-up push with a stale-info error instead of a normal
fast-forward.

The concurrent-creator race the tip check was meant to guard was already
handled one layer up: ensureFeatureBranch drops to a plain push when the
branch exists and relies on Git's own fast-forward check as the safety net
(see TestRunChangesPushDropsLeaseWhenBranchExistsOnRemote), so a race
survivor with unrelated history is rejected there regardless of tip
equality here.

Add TestRemoteHasBranchReportsTrueForDivergedTip, which fails under the old
tip-equality check and passes now.

Refs #671

* fix(cli,zerogit): preserve the generated branch on a compare-and-swap conflict

After auto-branching, ensureFeatureBranch restores the default branch to its
own upstream tip using the compare-and-swap form of update-ref. When that
refused because the default branch had moved concurrently, the failure was
indistinguishable from an ordinary update-ref write error, so the rollback
path ran and deleted the freshly created feature branch. That branch
exclusively owns the user's commits at that point, so a lost race destroyed
work in order to "recover" from it.

Add ErrCompareAndSwapConflict, returned by ResetBranchRef only when an
expected-old value was supplied (the sole way git refuses is a tip mismatch).
ensureFeatureBranch now recognizes it and returns without rolling back,
reporting that the branch was preserved so the user can reconcile the default
branch by hand.

Extend TestResetBranchRefRefusesConcurrentDefaultAdvance, which already drove
a real concurrent advance, to require the sentinel. Add
TestEnsureFeatureBranchPreservesBranchOnCompareAndSwapConflict, which fails
with "must not delete the generated branch" when the new branch is removed.

Refs #671

* Address review feedback for auto-branch workflow.

Refresh the exact configured upstream ref before branching and restoring, guard direct URL and path remote destinations from tracking-ref assertion, and remove undeclared --end-of-options from CommitsAhead.

Refs #671

* fix(zerogit,cli): resolve reviewer feedback on branch restoration and remote handling

Update CommitsAhead and ResetBranchRef to safely terminate option parsing with --, inspect all linked worktrees before ref updates, resolve advertised branches on direct path and URL remotes, and query git configuration directly for tracking remote and merge branch without splitting on slashes.

Refs #671

* fix(zerogit): invoke rev-list without positional delimiter to treat revs correctly

Remove premature option terminator from rev-list command invocation while validating base ref against option-like prefixes.

Refs #671

* fix(cli,zerogit): resolve restore tip commit and enforce push remote unborn policy

Resolve restore tip commits via ResolveRemoteBranchTip for named and direct URL/path upstreams to eliminate bare branch fallback, enforce unborn destination policy on push remotes before branch creation, and extract resolveBranchPushPlan to isolate decision logic from repository mutation.

Refs #671

* fix(cli,zerogit): address review feedback on destination remote and upstream matching

Forward resolved pushRemote to branchPushPlan, update HasUpstream to support multi-segment remote upstream references, clarify IsNamedRemote comment, and extend CLI argument parsing tests.

Refs #671

* docs(cli): complete ensureFeatureBranch documentation and wire branchUpstreamRemoteAndMerge

Refs #671

---------

Co-authored-by: Claude Fable 5 <[email protected]>
Co-authored-by: euxaristia <[email protected]>
Co-authored-by: cairn-code <[email protected]>
2026-08-16 16:01:40 +08:00
Vasanth T 0eab63c908 docs(agents): record the normalize-before-match blocker (#900)
Three separate changes hit this in a fortnight, each written by someone
being careful about exactly the thing that got them.

A transform that removes bytes without leaving a gap is also a
reassembler. Redaction that matches by shape, run before a sanitizer that
strips control bytes, lets a credential split by a NUL or an ESC pass the
patterns as two fragments and be rejoined on the way out: the MCP failure
reason in #835, and the imported-transcript chokepoint in #878. The path
form of the same mistake is comparing where a handle landed against a
value produced by the same resolver the kernel just used, so a redirect
agrees with itself: the ACL guard in #808, where junctions were caught
only by an accident of Go's mode bits and directory symlinks were not
caught at all.

The unsplit value passing is what makes it survive review, so the note
says the test needs a split case.
2026-08-15 20:32:33 +08:00
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]>
2026-08-15 20:31:52 +08:00
9c292092a2 fix(daemon): clean up child after startup timeout (#774)
* fix(daemon): clean up child after startup timeout

* fix(daemon): terminate startup process tree

* test(background): accept reaped-later zombie children

* fix(background): retain configured process group target

* fix(background): address termination review feedback

* fix(daemon): use launch-time group identity, fix zombie/reap classification

Addresses jatmn's three #774 review findings:

- TerminateCommand rediscovered the process group via
  execution.TerminateProcessTree's Getpgid(pid) lookup. On Darwin,
  that lookup can return ESRCH once an unreaped group leader has
  already exited, even though live descendants remain in the group
  it configured at launch — silently falling back to signalling only
  the (already dead) leader PID and leaving descendants running (the
  required macOS smoke job's failure). Add
  execution.TerminateProcessGroup, which signals the negative PID
  directly using the launch-time Setpgid invariant instead of
  rediscovering it, and route TerminateCommand through it.

- signalTargetRunning's zombie check matched the individual-PID
  fallback target (a process that is not its own group leader)
  against the process table by PGID. That target's actual PGID
  differs from its own PID by definition, so no row ever matched,
  unknown resulted every time, and a zombie individual-PID target
  was always conservatively reported as still running. Branch on the
  sign of the target: group members by PGID, individual PIDs by PID.

- TerminateCommand returned the termination error even when the
  subsequent reap succeeded — a successful cmd.Wait is authoritative
  proof the leader is gone and collected, regardless of how the kill
  attempt itself went (e.g. taskkill /T racing an already-exited PID
  on Windows). Classify a successful reap as success.

All three regression tests are verified to fail against the pre-fix
code (confirmed by temporarily reverting each fix and rerunning).

Co-Authored-By: Claude Sonnet 5 <[email protected]>

* fix(daemon): harden command termination inputs

Amp-Thread-ID: https://ampcode.com/threads/T-019fa018-abb8-73de-9489-51e9e6514c6b

Co-authored-by: Pierre Bruno <[email protected]>

* fix(background): preserve process tree failures

Amp-Thread-ID: https://ampcode.com/threads/T-019fa7a0-1223-701d-9529-48ba5d7cf8c8

Co-authored-by: Pierre Bruno <[email protected]>

* docs(daemon): clarify boundary-probe and Setsid-guard comments per PR #774 review

Co-authored-by: Cursor <[email protected]>

---------

Co-authored-by: Claude Sonnet 5 <[email protected]>
Co-authored-by: Amp <[email protected]>
Co-authored-by: Cursor <[email protected]>
2026-08-15 20:31:10 +08:00
KRATOSandClaude Opus 4.8 060d38ce71 feat(security): path-jail primitive, credential-store locking, worktree git hardening (#891)
Split 1/3 of #829, carved out as the independently-reversible hardening layer
with no dependency on the orchestration work that stacks on top of it.

- internal/pathjail: os.Root handle-relative path confinement — ancestor
  traversal is confined, the final component is checked for reparse points
  (including a name written with a trailing separator), unprivileged Windows
  junctions are covered, temp-name fragments are rejected before they can
  escape the directory, whitespace-bearing paths are confined as spelled, and
  temp files stay unpredictable and exclusive.
- internal/credstore: file locking around the credential read-modify-write —
  exclusive for writers, shared for readers, so a reader cannot race a publish.
- internal/worktrees: git invocation hardening.

pathjail has no importers in this PR by design; internal/memory and
internal/specialist adopt it in the stacked PRs (2/3 and 3/3).


Origin-Session: local-de382f | Claude Code | 4 prompts
Origin-Snapshot: 52db9b74dad5

Co-authored-by: Claude Opus 4.8 <[email protected]>
2026-08-15 20:24:08 +08:00
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
2026-08-15 17:54:38 +08:00
Vasanth T dc15e82298 fix: bump Go to 1.26.6 for stdlib vulnerability fixes (#903) 2026-08-14 18:58:16 +05:30
54 changed files with 9314 additions and 268 deletions

No files matched your search

+8
View File
@@ -91,6 +91,14 @@ These classes drive multi-round reviews. Fix them before requesting review:
final-component-only no-follow is insufficient. On multi-step setup, roll back
only what this run created; never destroy pre-existing resources you did
not create; never report success when cleanup or unlock failed.
- **Normalize before matching:** Any transform that REMOVES bytes without
leaving a gap is also a reassembler, so it has to run before whatever matches
on the result. Redacting by shape and then stripping control bytes lets a
credential split by a NUL, an ESC or a C1 byte pass the patterns as two
fragments and be rejoined on the way out; comparing a path against a form
produced by the same resolver the kernel just used makes a redirect agree with
itself. Strip, decode, or canonicalize first, then match. The unsplit value
passing is not evidence, so the test needs a split case.
- **Atomic shared state:** Write a complete temporary file, then atomically
replace the destination so concurrent readers never see a partial write.
Exclusive create or a write lock alone is not enough for readers. Serialize
+2 -2
View File
@@ -1,6 +1,6 @@
module github.com/Gitlawb/zero
go 1.26.5
go 1.26.6
require (
charm.land/bubbles/v2 v2.1.1
@@ -16,7 +16,7 @@ require (
github.com/charmbracelet/x/term v0.2.2
github.com/coder/websocket v1.8.15
github.com/ledongthuc/pdf v0.0.0-20250511090121-5959a4027728
golang.org/x/image v0.44.0
golang.org/x/image v0.45.0
golang.org/x/sys v0.47.0
mvdan.cc/sh/v3 v3.13.1
)
+2
View File
@@ -66,6 +66,8 @@ golang.org/x/exp v0.0.0-20260611194520-c48552f49976 h1:X8Hz2ImujgbmetVuW+w2YkyZC
golang.org/x/exp v0.0.0-20260611194520-c48552f49976/go.mod h1:vnf4pv9iKZXY58sQE1L86zmNWJ4159e1RkcWiLCkeEY=
golang.org/x/image v0.44.0 h1:+tDekMZED9+LrtB3G5xzRggpVh9CARjZqROla3R3R+I=
golang.org/x/image v0.44.0/go.mod h1:V8K3KE9KKKE+pLpQDOeN18w9oacNSvy1tDOirTu4xtY=
golang.org/x/image v0.45.0 h1:FMb1nTbH5H9vF55SriQHgFw5GnNL9Jg6L25BwXKzhB0=
golang.org/x/image v0.45.0/go.mod h1:n62x/7RqlwXDvGsSU4u6IUTUf6KghUZ9Bt7cG/T9Fx4=
golang.org/x/sync v0.22.0 h1:SZjpbeLmrCk4xhRSZFNZW5gFUeCeFgjekvI/+gfScek=
golang.org/x/sync v0.22.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0=
golang.org/x/sys v0.47.0 h1:o7XGOvZQCADBQQ4Y7VNq2dRWQR7JmOUW8Kxx4ZsNgWs=
+16 -2
View File
@@ -296,6 +296,16 @@ func stopReasonFor(result agent.Result, err error) (string, error) {
if errors.Is(err, context.Canceled) {
return StopCancelled, nil
}
// A CLIENT THAT CANCELLED A PERMISSION PROMPT DID NOT FAIL. Only
// context.Canceled was matched here, so dismissing a permission dialog —
// a deliberate action, and for apply_patch the only refusal a client is
// offered — came back as JSON-RPC -32603 carrying the internal sentinel
// text. Editors and the desktop app both render that as a crashed turn,
// so declining a tool looked like ZERO falling over. It is a
// cancellation, and StopCancelled is what ACP has for saying so.
if errors.Is(err, agent.ErrPermissionApprovalCanceled) {
return StopCancelled, nil
}
return "", err
}
if result.FinishReason == "length" {
@@ -311,10 +321,14 @@ func stopReasonFor(result agent.Result, err error) (string, error) {
// session/request_permission request and maps the outcome back. Failure to reach
// the client fails closed to deny.
func (a *Agent) requestPermission(ctx context.Context, sessionID string, req agent.PermissionRequest) (agent.PermissionDecision, error) {
// Built ONCE and used for both halves. Sending one list and validating the
// reply against another is the defect this whole path had; see
// decisionFromOutcome.
options := buildPermissionOptions(req)
params := RequestPermissionParams{
SessionID: sessionID,
ToolCall: permissionToolCall(req),
Options: buildPermissionOptions(req),
Options: options,
}
var result RequestPermissionResult
if err := a.conn.Call(ctx, MethodSessionRequestPerm, params, &result); err != nil {
@@ -323,7 +337,7 @@ func (a *Agent) requestPermission(ctx context.Context, sessionID string, req age
}
return agent.PermissionDecision{Action: agent.PermissionDecisionDeny, Reason: "permission request failed: " + err.Error()}, nil
}
return decisionFromOutcome(result.Outcome, req.AvailableDecisions), nil
return decisionFromOutcome(result.Outcome, options), nil
}
func (a *Agent) emitPlan(registry *tools.Registry, note *notifier) {
+217
View File
@@ -0,0 +1,217 @@
package acp
// desktopinterop_test.go covers three defects found by driving the real `zero
// acp` binary from a desktop ACP client. Each is a case where ZERO and a
// conforming client disagreed about the wire, and none of them failed loudly —
// they produced a silent denial, a spurious crash, or two buttons the user
// could not tell apart.
//
// A fourth, session/load restoring history without replaying it, is fixed by
// #914 rather than here: that one also gives the replayed messages stable ids
// and keeps session/resume replay-free, which this did not.
import (
"errors"
"fmt"
"testing"
"github.com/Gitlawb/zero/internal/agent"
)
// ---- an offered option is an acceptable answer ----
// The list the options were BUILT from and the list the answer was CHECKED
// against were different whenever ZERO did not enumerate: the client was sent
// Allow and Reject, and its reply was validated against an empty slice, so
// every button it could possibly show failed closed to deny. The user clicked
// Allow and ZERO recorded a denial, with nothing on screen to say so.
func TestAnOfferedOptionIsAccepted(t *testing.T) {
// No AvailableDecisions: the fallback path, which is every permission event
// that is not a prompt.
req := agent.PermissionRequest{ToolName: "bash"}
options := buildPermissionOptions(req)
if len(options) == 0 {
t.Fatal("no options were offered")
}
for _, option := range options {
decision := decisionFromOutcome(
RequestPermissionOutcome{Outcome: OutcomeSelected, OptionID: option.OptionID},
buildPermissionOptions(req),
)
if string(decision.Action) != option.OptionID {
t.Fatalf("option %q was answered with %q (%s) — an offered option must be accepted",
option.OptionID, decision.Action, decision.Reason)
}
}
}
// The narrowing this validation exists for still holds: a client must not be
// able to return a broader grant than it was shown.
func TestAnOptionThatWasNotOfferedIsStillRefused(t *testing.T) {
req := agent.PermissionRequest{ToolName: "bash"}
decision := decisionFromOutcome(
RequestPermissionOutcome{Outcome: OutcomeSelected, OptionID: string(agent.PermissionDecisionAlwaysAllow)},
buildPermissionOptions(req),
)
if decision.Action != agent.PermissionDecisionDeny {
t.Fatalf("an unoffered broader grant was accepted as %q", decision.Action)
}
}
// The resolver is the single source of truth for both sides, which is what
// makes the two agree by construction rather than by being kept in step.
func TestOfferedDecisionsMatchWhatWasSent(t *testing.T) {
for _, req := range []agent.PermissionRequest{
{ToolName: "bash"},
{ToolName: "bash", AvailableDecisions: []agent.PermissionDecisionAction{
agent.PermissionDecisionAllow,
agent.PermissionDecisionAllowForSession,
agent.PermissionDecisionDeny,
}},
} {
offered := buildPermissionOptions(req)
for _, option := range buildPermissionOptions(req) {
if !actionOffered(option.OptionID, offered) {
t.Fatalf("option %q was sent but is not in the offered set %v", option.OptionID, offered)
}
}
}
}
// ---- two options the user can tell apart ----
// request_permissions offers plain allow and strict-review allow together, and
// both were labelled "Allow". The label is the only thing an ACP client shows,
// so the panel presented two identical buttons where one silently enabled
// strict auto-review of what was granted.
func TestEveryOfferedOptionHasADistinctLabel(t *testing.T) {
req := agent.PermissionRequest{
ToolName: "request_permissions",
AvailableDecisions: []agent.PermissionDecisionAction{
agent.PermissionDecisionAllow,
agent.PermissionDecisionAllowStrict,
agent.PermissionDecisionAllowForSession,
agent.PermissionDecisionDeny,
},
}
seen := map[string]string{}
for _, option := range buildPermissionOptions(req) {
if previous, clash := seen[option.Name]; clash {
t.Fatalf("options %q and %q share the label %q — a user cannot tell them apart",
previous, option.OptionID, option.Name)
}
seen[option.Name] = option.OptionID
}
}
// The two remain distinct decisions: the label changed, the round trip did not.
func TestStrictAllowStillRoundTrips(t *testing.T) {
req := agent.PermissionRequest{
ToolName: "request_permissions",
AvailableDecisions: []agent.PermissionDecisionAction{
agent.PermissionDecisionAllow,
agent.PermissionDecisionAllowStrict,
},
}
decision := decisionFromOutcome(
RequestPermissionOutcome{Outcome: OutcomeSelected, OptionID: string(agent.PermissionDecisionAllowStrict)},
buildPermissionOptions(req),
)
if decision.Action != agent.PermissionDecisionAllowStrict {
t.Fatalf("strict allow round-tripped as %q", decision.Action)
}
}
// ---- cancelling a permission is a cancellation, not a crash ----
// Only context.Canceled was recognised, so dismissing a permission dialog came
// back as JSON-RPC -32603 carrying the internal sentinel text. Clients render
// that as a failed turn, so declining a tool looked like ZERO falling over —
// and for apply_patch, dismissing is the ONLY refusal a client is offered.
func TestCancellingAPermissionEndsTheTurnAsCancelled(t *testing.T) {
canceled := fmt.Errorf("%w for apply_patch", agent.ErrPermissionApprovalCanceled)
reason, err := stopReasonFor(agent.Result{}, canceled)
if err != nil {
t.Fatalf("stopReasonFor returned an error for a cancellation: %v", err)
}
if reason != StopCancelled {
t.Fatalf("stop reason = %q, want %q", reason, StopCancelled)
}
}
// A genuine failure must still be an error. Mapping too much to cancelled would
// hide real faults behind a clean ending, which is the opposite mistake.
func TestARealFailureIsStillAnError(t *testing.T) {
failure := errors.New("provider exploded")
if _, err := stopReasonFor(agent.Result{}, failure); !errors.Is(err, failure) {
t.Fatalf("stopReasonFor(%v) = %v, want the failure preserved", failure, err)
}
}
// The sentinel is matched through errors.Is, so it survives the wrapping every
// return site applies — each one adds the tool name.
func TestPermissionCancellationSurvivesWrapping(t *testing.T) {
if !errors.Is(fmt.Errorf("%w for bash", agent.ErrPermissionApprovalCanceled), agent.ErrPermissionApprovalCanceled) {
t.Fatal("a wrapped permission cancellation was not recognised")
}
if errors.Is(errors.New("something else"), agent.ErrPermissionApprovalCanceled) {
t.Fatal("an unrelated error was reported as a permission cancellation")
}
}
// A decision that never became an option cannot be selected.
//
// PermissionDecisionCancel is the case that exists today: optionKindFor drops
// it because ACP expresses cancellation through the outcome, yet ZERO
// enumerates it in AvailableDecisions for shell commands and apply_patch. While
// the reply was validated against the DECISIONS rather than the sent options, a
// client could return {"outcome":"selected","optionId":"cancel"} — an id it was
// never shown — and abort the whole turn.
//
// Written over every option-less decision rather than over "cancel", so a
// future action that optionKindFor declines to render is covered the day it is
// added rather than the day someone remembers this test.
func TestADecisionThatIsNotAnOptionCannotBeSelected(t *testing.T) {
for _, toolName := range []string{"bash", "apply_patch"} {
req := agent.PermissionRequest{
ToolName: toolName,
AvailableDecisions: []agent.PermissionDecisionAction{
agent.PermissionDecisionAllow,
agent.PermissionDecisionDeny,
agent.PermissionDecisionCancel,
},
}
options := buildPermissionOptions(req)
for _, action := range req.AvailableDecisions {
if actionOffered(string(action), options) {
continue // it was sent; selecting it is legitimate
}
decision := decisionFromOutcome(
RequestPermissionOutcome{Outcome: OutcomeSelected, OptionID: string(action)},
options,
)
if decision.Action != agent.PermissionDecisionDeny {
t.Fatalf("%s: %q was never sent as an option but was accepted as %q",
toolName, action, decision.Action)
}
}
}
}
// Cancelling is still reachable — through the outcome, which is where ACP puts
// it. Closing the selected-id route must not close the real one.
func TestCancellingThroughTheOutcomeStillWorks(t *testing.T) {
options := buildPermissionOptions(agent.PermissionRequest{ToolName: "apply_patch"})
decision := decisionFromOutcome(RequestPermissionOutcome{Outcome: OutcomeCancelled}, options)
if decision.Action != agent.PermissionDecisionCancel {
t.Fatalf("outcome=cancelled gave %q, want cancel", decision.Action)
}
}
+57 -18
View File
@@ -15,14 +15,7 @@ import (
// PermissionOptions. Only the actions ZERO actually presented (AvailableDecisions)
// are surfaced; the optionId is the ZERO action string for a clean round-trip.
func buildPermissionOptions(req agent.PermissionRequest) []PermissionOption {
actions := req.AvailableDecisions
if len(actions) == 0 {
// Sensible default if ZERO didn't enumerate: allow once / reject.
actions = []agent.PermissionDecisionAction{
agent.PermissionDecisionAllow,
agent.PermissionDecisionDeny,
}
}
actions := offeredDecisions(req)
options := make([]PermissionOption, 0, len(actions))
for _, action := range actions {
kind, name := optionKindFor(action, req.PrefixApprovalEscalates)
@@ -38,6 +31,32 @@ func buildPermissionOptions(req agent.PermissionRequest) []PermissionOption {
return options
}
// offeredDecisions resolves what this request actually offers the client.
//
// ONE RESOLVER, USED BY BOTH SIDES, and that is the entire point of it existing.
// The fallback below used to live inside buildPermissionOptions while
// requestPermission validated the client's answer against the raw
// req.AvailableDecisions. When ZERO did not enumerate — which is every
// permission event that is not a prompt, and includes a shell call whose sandbox
// decision came back deny — the two disagreed completely: the client was sent
// "Allow" and "Reject", and validation was performed against an EMPTY list, so
// every option the client could possibly click failed closed to deny with
// "permission option was not offered".
//
// The user clicked Allow and ZERO recorded a denial. Nothing surfaced it,
// because a denial is a legitimate answer — the tool was simply refused and the
// turn carried on as though the user had rejected it.
func offeredDecisions(req agent.PermissionRequest) []agent.PermissionDecisionAction {
if len(req.AvailableDecisions) > 0 {
return req.AvailableDecisions
}
// Sensible default if ZERO didn't enumerate: allow once / reject.
return []agent.PermissionDecisionAction{
agent.PermissionDecisionAllow,
agent.PermissionDecisionDeny,
}
}
// optionKindFor maps a ZERO decision action to an ACP PermissionOptionKind and a
// human label. Returns an empty kind for actions that ACP expresses through the
// outcome rather than an option (cancel).
@@ -49,8 +68,16 @@ func buildPermissionOptions(req agent.PermissionRequest) []PermissionOption {
// disclosure cannot be TUI-only.
func optionKindFor(action agent.PermissionDecisionAction, escalates bool) (kind, name string) {
switch action {
case agent.PermissionDecisionAllow, agent.PermissionDecisionAllowStrict:
case agent.PermissionDecisionAllow:
return PermAllowOnce, "Allow"
case agent.PermissionDecisionAllowStrict:
// SPLIT FROM PLAIN ALLOW because the label is the only thing an ACP
// client shows. Both were labelled "Allow", and request_permissions
// offers them together — so the panel presented two identical buttons,
// and whichever the user picked looked the same while one of them
// silently turned on strict auto-review of what was granted. The option
// id carries no room for the distinction, so it has to be in the name.
return PermAllowOnce, "Allow with strict review"
case agent.PermissionDecisionAllowForSession:
return PermAllowAlways, "Allow for this session"
case agent.PermissionDecisionAllowPrefix:
@@ -70,19 +97,31 @@ func optionKindFor(action agent.PermissionDecisionAction, escalates bool) (kind,
// decisionFromOutcome maps the client's permission outcome back to a ZERO
// decision. A cancelled outcome cancels the run; a selected option id is the ZERO
// action verbatim (validated against what was offered); anything unrecognized
// fails closed to deny.
func decisionFromOutcome(outcome RequestPermissionOutcome, offered []agent.PermissionDecisionAction) agent.PermissionDecision {
// action verbatim (validated against what was actually sent); anything
// unrecognized fails closed to deny.
//
// IT TAKES THE OPTIONS THAT WERE SENT, not the decisions the request offered,
// and the two are not the same list. Some actions never become options —
// PermissionDecisionCancel is dropped by optionKindFor because ACP expresses
// cancellation through the outcome instead — yet it IS enumerated in
// AvailableDecisions for shell commands and apply_patch, the two commonest
// prompts. Validating against the decisions therefore accepted
// {"outcome":"selected","optionId":"cancel"}: an identifier no client was ever
// shown, aborting the whole turn.
//
// Passing the sent options makes "what we offered" and "what we accept" the
// same slice rather than two things kept in step, so anything optionKindFor
// declines to render is excluded structurally instead of by name.
func decisionFromOutcome(outcome RequestPermissionOutcome, offered []PermissionOption) agent.PermissionDecision {
switch outcome.Outcome {
case OutcomeCancelled:
return agent.PermissionDecision{Action: agent.PermissionDecisionCancel, Reason: "client cancelled"}
case OutcomeSelected:
action := agent.PermissionDecisionAction(outcome.OptionID)
// Bind to what was actually offered for THIS call: a client must not be able
// to return a broader grant (always_allow / allow_for_session) that wasn't
// presented. Anything not offered fails closed to deny.
if actionOffered(action, offered) {
return agent.PermissionDecision{Action: action}
if actionOffered(outcome.OptionID, offered) {
return agent.PermissionDecision{Action: agent.PermissionDecisionAction(outcome.OptionID)}
}
return agent.PermissionDecision{Action: agent.PermissionDecisionDeny, Reason: "permission option was not offered"}
default:
@@ -90,9 +129,9 @@ func decisionFromOutcome(outcome RequestPermissionOutcome, offered []agent.Permi
}
}
func actionOffered(action agent.PermissionDecisionAction, offered []agent.PermissionDecisionAction) bool {
for _, a := range offered {
if a == action {
func actionOffered(optionID string, offered []PermissionOption) bool {
for _, option := range offered {
if option.OptionID == optionID {
return true
}
}
+3 -2
View File
@@ -56,10 +56,11 @@ func TestACPPrefixOptionsStaySilentWithoutEscalation(t *testing.T) {
// what was offered, so decorating it would fail every prefix approval closed.
func TestACPDisclosureDoesNotDisturbTheOptionIDRoundTrip(t *testing.T) {
request := escalationRequest(true)
for _, option := range buildPermissionOptions(request) {
options := buildPermissionOptions(request)
for _, option := range options {
decision := decisionFromOutcome(
RequestPermissionOutcome{Outcome: OutcomeSelected, OptionID: option.OptionID},
request.AvailableDecisions,
options,
)
if string(decision.Action) != option.OptionID {
t.Errorf("option id %q round-tripped to %q (%s)", option.OptionID, decision.Action, decision.Reason)
+8 -5
View File
@@ -39,11 +39,14 @@ func TestBuildPermissionOptionsDefault(t *testing.T) {
}
func TestDecisionFromOutcome(t *testing.T) {
offered := []agent.PermissionDecisionAction{
agent.PermissionDecisionAllow,
agent.PermissionDecisionAlwaysAllow,
agent.PermissionDecisionDeny,
}
offered := buildPermissionOptions(agent.PermissionRequest{
ToolName: "bash",
AvailableDecisions: []agent.PermissionDecisionAction{
agent.PermissionDecisionAllow,
agent.PermissionDecisionAlwaysAllow,
agent.PermissionDecisionDeny,
},
})
if d := decisionFromOutcome(RequestPermissionOutcome{Outcome: OutcomeCancelled}, offered); d.Action != agent.PermissionDecisionCancel {
t.Errorf("cancelled -> %q, want cancel", d.Action)
}
+12 -4
View File
@@ -40,7 +40,15 @@ const (
toolResultControlSpecReview = "spec_review_required"
)
var errPermissionApprovalCanceled = errors.New("permission approval cancelled")
// ErrPermissionApprovalCanceled ends a run because the caller cancelled a
// permission prompt, rather than because anything failed.
//
// EXPORTED FOR THE SURFACES. A cancel is a normal ending and each surface says
// so its own way — the TUI unwinds, and ACP has a "cancelled" stop reason for
// exactly this. While it was unexported ACP could not distinguish it from a
// fault and reported it as an internal error, so declining a tool looked like
// the agent crashing.
var ErrPermissionApprovalCanceled = errors.New("permission approval cancelled")
// isImageRejectionError reports whether err is a provider 400 that rejects
// image/multimodal content. This is checked BEFORE the compaction-retry path
@@ -1364,7 +1372,7 @@ func executeToolCall(ctx context.Context, registry *tools.Registry, call ToolCal
}
case PermissionDecisionCancel:
emitCanceledPermission(options, call, requestEvent, decisionReason)
return canceledPermissionResult(call, decisionReason, requestEvent), fmt.Errorf("%w for %s", errPermissionApprovalCanceled, call.Name)
return canceledPermissionResult(call, decisionReason, requestEvent), fmt.Errorf("%w for %s", ErrPermissionApprovalCanceled, call.Name)
default:
emitDeniedPermission(options, call, requestEvent, decisionReason)
return deniedPermissionResult(call, decisionReason, requestEvent), nil
@@ -1548,7 +1556,7 @@ func maybeRetryUnsandboxedAfterSandboxRestriction(ctx context.Context, registry
case PermissionDecisionCancel:
emitCanceledPermission(options, call, requestEvent, reason)
canceled := canceledPermissionResult(call, reason, requestEvent)
return result, &canceled, true, decision.Action, reason, nil, fmt.Errorf("%w for %s", errPermissionApprovalCanceled, call.Name)
return result, &canceled, true, decision.Action, reason, nil, fmt.Errorf("%w for %s", ErrPermissionApprovalCanceled, call.Name)
default:
emitDeniedPermission(options, call, requestEvent, reason)
denied := deniedPermissionResult(call, reason, requestEvent)
@@ -1596,7 +1604,7 @@ func maybeRetryWithNetworkAfterSandboxDenial(ctx context.Context, registry *tool
case PermissionDecisionCancel:
emitCanceledPermission(options, call, requestEvent, reason)
canceled := canceledPermissionResult(call, reason, requestEvent)
return result, &canceled, true, decision.Action, reason, fmt.Errorf("%w for %s", errPermissionApprovalCanceled, call.Name)
return result, &canceled, true, decision.Action, reason, fmt.Errorf("%w for %s", ErrPermissionApprovalCanceled, call.Name)
default:
emitDeniedPermission(options, call, requestEvent, reason)
denied := deniedPermissionResult(call, reason, requestEvent)
+93 -1
View File
@@ -1853,7 +1853,7 @@ func TestRunAbortsWhenPermissionRequestCanceled(t *testing.T) {
},
})
if !errors.Is(err, errPermissionApprovalCanceled) {
if !errors.Is(err, ErrPermissionApprovalCanceled) {
t.Fatalf("expected permission approval cancel error, got %v", err)
}
if result.FinalAnswer != "" {
@@ -4340,3 +4340,95 @@ func TestPlanModeHonorsBeforeToolVeto(t *testing.T) {
t.Fatalf("expected tool result to mention the beforeTool veto, got %q", combined)
}
}
// A cancelled permission on either sandbox-retry path must abort the run with a
// recognisable cancellation, and must not run the escalated retry.
//
// BOTH PATHS, because both wrap ErrPermissionApprovalCanceled and each is
// reached by a different kind of denial: one by an unrestricted platform denial
// asking for approval, the other by an external-network denial. Surfaces map
// this sentinel onto their own idea of a cancel — ACP returns stopReason
// "cancelled" for it — so a path that produced a bare error instead would show
// the user a crash where they had simply declined.
func TestCancellingASandboxRetryAbortsWithoutRetrying(t *testing.T) {
denial := func(capability execution.CapabilityKind, scope string) tools.Result {
return tools.Result{
Status: tools.StatusError,
ExecutionOutcome: &execution.Outcome{
State: execution.StateDenied,
Kind: execution.OutcomeEnforcementDenied,
Denial: &execution.Denial{
Capability: execution.Capability{Kind: capability, Scope: scope},
Source: execution.DenialSourcePlatformSandbox,
Reason: "denied by the sandbox",
Recoverable: true,
NextAction: execution.DenialNextActionRequestApproval,
},
},
}
}
for _, testCase := range []struct {
name string
result tools.Result
run func(context.Context, *tools.Registry, ToolCall, tools.Tool, map[string]any, tools.Result, Options) error
}{
{
name: "unsandboxed retry",
result: denial(execution.CapabilityUnrestricted, "host"),
run: func(ctx context.Context, registry *tools.Registry, call ToolCall, tool tools.Tool, args map[string]any, result tools.Result, options Options) error {
_, _, _, _, _, _, abortErr := maybeRetryUnsandboxedAfterSandboxRestriction(
ctx, registry, call, tool, args, result, PermissionModeAsk, options, nil)
return abortErr
},
},
{
name: "network retry",
result: denial(execution.CapabilityExternalNetwork, ""),
run: func(ctx context.Context, registry *tools.Registry, call ToolCall, tool tools.Tool, args map[string]any, result tools.Result, options Options) error {
_, _, _, _, _, abortErr := maybeRetryWithNetworkAfterSandboxDenial(
ctx, registry, call, tool, args, result, PermissionModeAsk, options, nil)
return abortErr
},
},
} {
t.Run(testCase.name, func(t *testing.T) {
retryTool := &sandboxNamespaceLimitedRetryTool{}
registry := tools.NewRegistry()
registry.Register(retryTool)
asked := 0
options := Options{
Registry: registry,
PermissionMode: PermissionModeAsk,
Autonomy: "medium",
Sandbox: sandbox.NewEngine(sandbox.EngineOptions{
WorkspaceRoot: t.TempDir(),
Policy: sandbox.DefaultPolicy(),
}),
OnPermissionRequest: func(context.Context, PermissionRequest) (PermissionDecision, error) {
asked++
return PermissionDecision{Action: PermissionDecisionCancel, Reason: "client cancelled"}, nil
},
}
call := ToolCall{ID: "call-1", Name: "bash", Arguments: `{"command":"curl example.com"}`}
args := map[string]any{"command": "curl example.com"}
abortErr := testCase.run(context.Background(), registry, call, retryTool, args, testCase.result, options)
if asked != 1 {
t.Fatalf("permission was requested %d times, want exactly 1", asked)
}
if !errors.Is(abortErr, ErrPermissionApprovalCanceled) {
t.Fatalf("abort error = %v, want it to match ErrPermissionApprovalCanceled", abortErr)
}
// The escalated retry is the thing the permission was gating. A
// cancelled prompt that still ran it would have run the command the
// user just declined.
if len(retryTool.calls) != 0 {
t.Fatalf("the escalated retry ran %d times after a cancel: %+v", len(retryTool.calls), retryTool.calls)
}
})
}
}
+18
View File
@@ -39,3 +39,21 @@ func ConfigureChildProcessGroup(cmd *exec.Cmd) {
func terminateProcess(pid int) error {
return execution.TerminateProcessTree(pid, terminationGracePeriod, terminationPollInterval)
}
// terminateOwnedProcess terminates cmd's process group directly when its launch
// configuration proves it was made a group leader. This avoids fragile Getpgid
// rediscovery after an owned leader exits. Ordinary commands fall back to the
// safe PID/tree path rather than assuming their PID is also a process-group ID.
//
// The Setpgid && Pgid == 0 guard only recognizes ConfigureChildProcessGroup's
// own convention. A command made its own session leader via Setsid (as opposed
// to Setpgid) is also its own process-group leader in practice, but takes the
// slower rediscovery path here since Setsid isn't checked. Harmless today
// because TerminateCommand has exactly one caller in this codebase; worth
// covering explicitly if Setsid-configured commands start using this path too.
func terminateOwnedProcess(cmd *exec.Cmd) (bool, error) {
if cmd.SysProcAttr != nil && cmd.SysProcAttr.Setpgid && cmd.SysProcAttr.Pgid == 0 {
return false, execution.TerminateProcessGroup(cmd.Process.Pid, terminationGracePeriod, terminationPollInterval)
}
return false, terminateProcess(cmd.Process.Pid)
}
+75 -4
View File
@@ -147,11 +147,12 @@ func TestTerminateProcessKillsForkedChildren(t *testing.T) {
t.Fatalf("terminateProcess: %v", err)
}
// The forked child must be gone too (poll until reaped by init).
// The forked child must no longer be running. An orphaned zombie is already
// dead but may remain visible briefly until the platform's init reaps it.
deadline := time.Now().Add(2 * time.Second)
// Only ESRCH proves the child is gone; any other error (e.g. EPERM) would
// wrongly pass the test, so treat it as still-present and keep polling.
for !errors.Is(syscall.Kill(childPID, syscall.Signal(0)), syscall.ESRCH) {
// processStopped treats only ESRCH or an observed zombie state as stopped;
// other errors (for example EPERM) remain live and keep polling.
for !processStopped(childPID) {
if time.Now().After(deadline) {
_ = syscall.Kill(childPID, syscall.SIGKILL)
t.Fatalf("forked child %d survived terminateProcess — group kill failed", childPID)
@@ -159,3 +160,73 @@ func TestTerminateProcessKillsForkedChildren(t *testing.T) {
time.Sleep(20 * time.Millisecond)
}
}
func TestTerminateCommandStopsUnconfiguredCommand(t *testing.T) {
cmd := exec.Command("sleep", "30")
if err := cmd.Start(); err != nil {
t.Fatalf("start: %v", err)
}
t.Cleanup(func() {
if cmd.ProcessState == nil {
_ = cmd.Process.Kill()
}
})
if err := TerminateCommand(cmd); err != nil {
t.Fatalf("TerminateCommand: %v", err)
}
if cmd.ProcessState == nil {
t.Fatal("unconfigured command was not reaped")
}
}
func TestTerminateCommandKillsChildAfterLeaderExits(t *testing.T) {
grace, poll := terminationGracePeriod, terminationPollInterval
terminationGracePeriod, terminationPollInterval = 2*time.Second, 20*time.Millisecond
t.Cleanup(func() { terminationGracePeriod, terminationPollInterval = grace, poll })
// The leader exits immediately after launching the child. TerminateCommand
// must capture and signal the process group before Wait reaps the leader and
// makes its group identity unavailable.
cmd := exec.Command("sh", "-c", "sleep 300 & echo $!; exit 0")
ConfigureChildProcessGroup(cmd)
stdout, err := cmd.StdoutPipe()
if err != nil {
t.Fatalf("stdout pipe: %v", err)
}
if err := cmd.Start(); err != nil {
t.Fatalf("start: %v", err)
}
line, err := bufio.NewReader(stdout).ReadString('\n')
if err != nil {
t.Fatalf("read forked child pid: %v", err)
}
childPID, err := strconv.Atoi(strings.TrimSpace(line))
if err != nil {
t.Fatalf("parse forked child pid %q: %v", line, err)
}
if err := TerminateCommand(cmd); err != nil {
t.Fatalf("TerminateCommand: %v", err)
}
deadline := time.Now().Add(2 * time.Second)
for !processStopped(childPID) {
if time.Now().After(deadline) {
_ = syscall.Kill(childPID, syscall.SIGKILL)
t.Fatalf("forked child %d survived TerminateCommand", childPID)
}
time.Sleep(20 * time.Millisecond)
}
}
func processStopped(pid int) bool {
if errors.Is(syscall.Kill(pid, syscall.Signal(0)), syscall.ESRCH) {
return true
}
state, err := exec.Command("ps", "-o", "stat=", "-p", strconv.Itoa(pid)).Output()
if err != nil {
return errors.Is(syscall.Kill(pid, syscall.Signal(0)), syscall.ESRCH)
}
return strings.HasPrefix(strings.TrimSpace(string(state)), "Z")
}
+34
View File
@@ -3,11 +3,20 @@
package background
import (
"fmt"
"os/exec"
"github.com/Gitlawb/zero/internal/execution"
"golang.org/x/sys/windows"
)
// stillActiveExitCode is the documented GetExitCodeProcess value for a process
// that has not terminated. x/sys/windows does not expose Windows' STILL_ACTIVE
// constant.
const stillActiveExitCode uint32 = 259
var terminateProcessForTest = terminateProcess
// ConfigureChildProcessGroup is a no-op on Windows: process-tree termination is
// delegated to execution.TerminateProcessTree, so no launch-time process-group
// setup is required (the POSIX build sets Setpgid here instead).
@@ -16,3 +25,28 @@ func ConfigureChildProcessGroup(cmd *exec.Cmd) { execution.ConfigureProcessGroup
func terminateProcess(pid int) error {
return execution.TerminateProcessTree(pid, 0, 0)
}
// terminateOwnedProcess asks taskkill /T to terminate the tree rooted at cmd,
// with KillProcessTree's direct Process.Kill fallback if taskkill fails. taskkill
// can discover descendants only while the root PID still exists; unlike a POSIX
// process group, a Windows tree has no independently addressable identity after
// its root exits.
func terminateOwnedProcess(cmd *exec.Cmd) (bool, error) {
var (
alreadyExited bool
terminateErr error
)
err := cmd.Process.WithHandle(func(handle uintptr) {
var exitCode uint32
if windows.GetExitCodeProcess(windows.Handle(handle), &exitCode) == nil {
alreadyExited = exitCode != stillActiveExitCode
}
// Keep the exact process identity pinned while the PID-based taskkill
// operation runs, preventing the PID from being recycled underneath it.
terminateErr = terminateProcessForTest(cmd.Process.Pid)
})
if err != nil {
return false, fmt.Errorf("pin process %d for termination: %w", cmd.Process.Pid, err)
}
return alreadyExited, terminateErr
}
+106
View File
@@ -0,0 +1,106 @@
//go:build windows
package background
import (
"bufio"
"errors"
"os/exec"
"strconv"
"strings"
"testing"
"time"
)
func TestTerminateCommandReturnsTreeFailureAfterRootFallbackReaps(t *testing.T) {
cmd := exec.Command("powershell.exe", "-NoProfile", "-NonInteractive", "-Command", "Start-Sleep -Seconds 300")
if err := cmd.Start(); err != nil {
t.Fatalf("start: %v", err)
}
treeErr := errors.New("taskkill /T failed")
originalTerminateProcess := terminateProcessForTest
terminateProcessForTest = func(pid int) error {
if err := cmd.Process.Kill(); err != nil {
return errors.Join(treeErr, err)
}
return treeErr
}
t.Cleanup(func() { terminateProcessForTest = originalTerminateProcess })
err := TerminateCommand(cmd)
if !errors.Is(err, treeErr) {
t.Fatalf("TerminateCommand error = %v, want tree failure", err)
}
if cmd.ProcessState == nil {
t.Fatal("root process was not reaped")
}
}
func TestTerminateCommandReapsExitedLeader(t *testing.T) {
cmd := exec.Command("cmd.exe", "/c", "exit", "0")
if err := cmd.Start(); err != nil {
t.Fatalf("start: %v", err)
}
// Let the child exit without calling Wait so cleanup exercises the Windows
// taskkill/TerminateProcess race while it still owns the process handle.
time.Sleep(500 * time.Millisecond)
// jatmn's #774 finding: taskkill /T racing an already-exited PID (and the
// Process.Kill fallback) both fail here, but the reap below still succeeds —
// the process-handle exit-code check proves the leader was already gone, so
// TerminateCommand must not surface the expected kill-attempt error.
if err := TerminateCommand(cmd); err != nil {
t.Fatalf("TerminateCommand: %v, want nil (leader was already gone and got reaped)", err)
}
if cmd.ProcessState == nil {
t.Fatal("exited process was not reaped")
}
}
func TestTerminateCommandDeadLeaderCannotIdentifyDescendantTree(t *testing.T) {
cmd := exec.Command("powershell.exe", "-NoProfile", "-NonInteractive", "-Command",
`$p = Start-Process powershell.exe -ArgumentList '-NoProfile','-Command','Start-Sleep -Seconds 300' -PassThru; $p.Id`)
stdout, err := cmd.StdoutPipe()
if err != nil {
t.Fatalf("stdout pipe: %v", err)
}
if err := cmd.Start(); err != nil {
t.Fatalf("start: %v", err)
}
line, err := bufio.NewReader(stdout).ReadString('\n')
if err != nil {
t.Fatalf("read descendant pid: %v", err)
}
descendantPID, err := strconv.Atoi(strings.TrimSpace(line))
if err != nil {
t.Fatalf("parse descendant pid %q: %v", line, err)
}
t.Cleanup(func() {
_ = exec.Command("taskkill.exe", "/F", "/PID", strconv.Itoa(descendantPID)).Run()
})
// Wait until the leader is observably exited but deliberately leave it
// unreaped, matching the POSIX regression's ordering.
deadline := time.Now().Add(5 * time.Second)
for processExistsOnWindows(cmd.Process.Pid) {
if time.Now().After(deadline) {
t.Fatal("leader did not exit")
}
time.Sleep(20 * time.Millisecond)
}
if err := TerminateCommand(cmd); err != nil {
t.Fatalf("TerminateCommand: %v", err)
}
// Windows has no persistent group ID analogous to POSIX: once the leader is
// gone, taskkill /T cannot rediscover this descendant. Assert that platform
// distinction rather than claiming TerminateCommand can provide it here.
if !processExistsOnWindows(descendantPID) {
t.Fatal("descendant unexpectedly stopped after its root exited")
}
}
func processExistsOnWindows(pid int) bool {
return exec.Command("powershell.exe", "-NoProfile", "-NonInteractive", "-Command",
"Get-Process -Id "+strconv.Itoa(pid)+" -ErrorAction Stop | Out-Null").Run() == nil
}
+75
View File
@@ -1,5 +1,14 @@
package background
import (
"errors"
"fmt"
"os/exec"
"time"
)
const commandReapTimeout = 3 * time.Second
// TerminateProcess stops a background process by PID — on Windows its process
// tree; on POSIX its whole process group when the PID leads its own group (the
// invariant ConfigureChildProcessGroup establishes for processes started through
@@ -12,3 +21,69 @@ package background
func TerminateProcess(pid int) error {
return terminateProcess(pid)
}
// TerminateCommand stops a started command and reaps its leader. The caller must
// have exclusive ownership of cmd: it must not have previously called Wait or
// Process.Release, and no goroutine may call either concurrently. On POSIX it
// stops the whole group when the command was configured as its leader; ordinary
// commands safely fall back to PID/tree discovery. On Windows, success for a
// leader that was already dead confirms only that the leader was reaped;
// descendants may survive because Windows cannot rediscover a tree from a dead
// root. `zero daemon start` needs this operation when readiness times out: it
// launched the child, so it must both stop the tree and collect the leader.
//
// The order matters: the tree is signalled first, because Wait releases the
// leader's PID and a later group lookup could then resolve to nothing (or, worse,
// to a recycled PID). Termination stays in the shared execution lifecycle
// primitives rather than introducing a second platform-specific killer.
//
// If the bounded reap times out, the Wait call already started by this function
// continues in its goroutine. From that point onward the caller must not access
// cmd at all, including ProcessState, because that would race with Wait.
func TerminateCommand(cmd *exec.Cmd) error {
if cmd == nil || cmd.Process == nil {
return errors.New("terminate command: process was never started")
}
leaderAlreadyExited, terminateErr := terminateOwnedProcess(cmd)
reapErr := waitForTerminatedCommandWithin(cmd, commandReapTimeout)
if reapErr != nil {
if terminateErr != nil {
return fmt.Errorf("%v (reap failed: %w)", terminateErr, reapErr)
}
return reapErr
}
if terminateErr != nil && !leaderAlreadyExited {
return terminateErr
}
// Only discard a termination error when the platform demonstrated before
// attempting termination that the leader had already exited. A successful
// reap alone says nothing about whether a live descendant tree was stopped.
return nil
}
// classifyWaitError treats a non-zero exit status as success: a process being
// terminated is expected to report one (or a signal), so the only interesting
// failure is Wait itself not working.
func classifyWaitError(waitErr error) error {
var exitErr *exec.ExitError
if waitErr != nil && !errors.As(waitErr, &exitErr) {
return fmt.Errorf("reap process: %w", waitErr)
}
return nil
}
// waitForTerminatedCommandWithin bounds the reap so a child that somehow survives
// termination cannot hang the caller — the daemon-start cleanup path runs while a
// user waits at the CLI. On timeout its Wait goroutine is intentionally left to
// finish: exec.Cmd permits only one Wait call, and abandoning it without a reaper
// would leak the child once it eventually exits.
func waitForTerminatedCommandWithin(cmd *exec.Cmd, timeout time.Duration) error {
waitDone := make(chan error, 1)
go func() { waitDone <- cmd.Wait() }()
select {
case waitErr := <-waitDone:
return classifyWaitError(waitErr)
case <-time.After(timeout):
return fmt.Errorf("process %d did not reap after termination", cmd.Process.Pid)
}
}
+132 -37
View File
@@ -64,38 +64,53 @@ type appDeps struct {
// getenv reads a process environment variable (production: os.Getenv, set in
// defaultAppDeps — deliberately NOT filled by fillAppDeps, so tests are hermetic
// against ambient vars like ZERO_PROVIDER unless they inject it). nil ⇒ empty.
getenv func(string) string
probeProviderHealth func(context.Context, providerhealth.Options) providerhealth.Result
discoverProviderModels func(context.Context, config.ProviderProfile) ([]providermodeldiscovery.Model, error)
detectLocalRuntimes func(context.Context, provideronboarding.LocalDetectOptions) []provideronboarding.DetectedLocalRuntime
openRouterLogin func(context.Context, provideroauth.OpenRouterOptions) (string, error)
newSessionStore func() *sessions.Store
loadPlugins func(plugins.LoadOptions) (plugins.LoadResult, error)
loadHooks func(hooks.LoadOptions) (hooks.LoadResult, error)
skillsDir func() string
pluginsDir func() string
toolsDir func() string
newMCPStore func() (*mcp.PermissionStore, error)
newMCPTokenStore func() (*mcp.TokenStore, error)
newSandboxStore func() (*sandbox.GrantStore, error)
selectSandboxBackend func(sandbox.BackendOptions) sandbox.Backend
runSandboxSetupHelper func(path string, args []string, stdout io.Writer, stderr io.Writer) error
registerMCPTools func(context.Context, *tools.Registry, config.MCPConfig, mcp.RegisterOptions) (mcpToolRuntime, error)
prepareWorktree func(context.Context, worktrees.Options) (worktrees.Result, error)
releaseWorktree func(context.Context, worktrees.Options, string) error
detectVerifyPlan func(string) (verify.Plan, error)
runVerify func(context.Context, verify.Plan, verify.RunOptions) verify.Report
runSelfVerify func(context.Context, verify.Plan, selfverify.Options) selfverify.Report
runAgentEval func(context.Context, agentEvalOptions) (agentEvalReport, error)
inspectChanges func(context.Context, zerogit.InspectOptions) (zerogit.ChangeSummary, error)
commitChanges func(context.Context, zerogit.CommitOptions) (zerogit.CommitResult, error)
pushChanges func(context.Context, zerogit.PushOptions) (zerogit.PushResult, error)
createPR func(context.Context, zerogit.PROptions) (zerogit.PRResult, error)
runTUI func(context.Context, tui.Options) int
runEditor func(string) error
checkUpdate func(context.Context, update.Options) (update.Result, error)
applyUpdate func(context.Context, update.Options) (update.ApplyResult, error)
now func() time.Time
getenv func(string) string
probeProviderHealth func(context.Context, providerhealth.Options) providerhealth.Result
discoverProviderModels func(context.Context, config.ProviderProfile) ([]providermodeldiscovery.Model, error)
detectLocalRuntimes func(context.Context, provideronboarding.LocalDetectOptions) []provideronboarding.DetectedLocalRuntime
openRouterLogin func(context.Context, provideroauth.OpenRouterOptions) (string, error)
newSessionStore func() *sessions.Store
loadPlugins func(plugins.LoadOptions) (plugins.LoadResult, error)
loadHooks func(hooks.LoadOptions) (hooks.LoadResult, error)
skillsDir func() string
pluginsDir func() string
toolsDir func() string
newMCPStore func() (*mcp.PermissionStore, error)
newMCPTokenStore func() (*mcp.TokenStore, error)
newSandboxStore func() (*sandbox.GrantStore, error)
selectSandboxBackend func(sandbox.BackendOptions) sandbox.Backend
runSandboxSetupHelper func(path string, args []string, stdout io.Writer, stderr io.Writer) error
registerMCPTools func(context.Context, *tools.Registry, config.MCPConfig, mcp.RegisterOptions) (mcpToolRuntime, error)
prepareWorktree func(context.Context, worktrees.Options) (worktrees.Result, error)
releaseWorktree func(context.Context, worktrees.Options, string) error
detectVerifyPlan func(string) (verify.Plan, error)
runVerify func(context.Context, verify.Plan, verify.RunOptions) verify.Report
runSelfVerify func(context.Context, verify.Plan, selfverify.Options) selfverify.Report
runAgentEval func(context.Context, agentEvalOptions) (agentEvalReport, error)
inspectChanges func(context.Context, zerogit.InspectOptions) (zerogit.ChangeSummary, error)
commitChanges func(context.Context, zerogit.CommitOptions) (zerogit.CommitResult, error)
pushChanges func(context.Context, zerogit.PushOptions) (zerogit.PushResult, error)
createPR func(context.Context, zerogit.PROptions) (zerogit.PRResult, error)
createBranch func(context.Context, zerogit.BranchOptions) (zerogit.BranchResult, error)
isDefaultBranch func(context.Context, zerogit.DefaultBranchOptions) (bool, string, string, error)
currentGitUser func(context.Context, string) string
headCommitSubject func(context.Context, string) string
commitsAhead func(context.Context, string, string, string) (int, error)
isUnbornRemote func(context.Context, string, string) (bool, error)
refreshTrackingRef func(context.Context, string, string, string) error
branchUpstreamRemote func(context.Context, string, string) string
branchUpstreamRemoteAndMerge func(context.Context, string, string) (string, string)
resolveRemoteBranchTip func(context.Context, string, string, string) (string, error)
remoteHasBranch func(context.Context, string, string, string) (bool, error)
currentGitBranch func(context.Context, string) string
currentBranchTip func(context.Context, string) string
deleteBranch func(context.Context, string, string, string) error
resetBranchRef func(context.Context, string, string, string, string) error
runTUI func(context.Context, tui.Options) int
runEditor func(string) error
checkUpdate func(context.Context, update.Options) (update.Result, error)
applyUpdate func(context.Context, update.Options) (update.ApplyResult, error)
now func() time.Time
}
type mcpToolRuntime interface {
@@ -198,11 +213,52 @@ func defaultAppDeps() appDeps {
commitChanges: zerogit.Commit,
pushChanges: zerogit.Push,
createPR: zerogit.CreatePR,
runTUI: tui.Run,
runEditor: openEditor,
checkUpdate: update.Check,
applyUpdate: update.Apply,
now: time.Now,
createBranch: zerogit.CreateBranch,
isDefaultBranch: zerogit.IsDefaultBranch,
currentGitUser: func(ctx context.Context, cwd string) string {
return zerogit.CurrentGitUser(ctx, cwd, nil)
},
headCommitSubject: func(ctx context.Context, cwd string) string {
return zerogit.HeadCommitSubject(ctx, cwd, nil)
},
commitsAhead: func(ctx context.Context, cwd, remote, branch string) (int, error) {
return zerogit.CommitsAhead(ctx, cwd, remote, branch, nil)
},
isUnbornRemote: func(ctx context.Context, cwd, remote string) (bool, error) {
return zerogit.IsUnbornRemote(ctx, cwd, remote, nil)
},
refreshTrackingRef: func(ctx context.Context, cwd, remote, branch string) error {
return zerogit.RefreshTrackingRef(ctx, cwd, remote, branch, nil)
},
branchUpstreamRemote: func(ctx context.Context, cwd, branch string) string {
return zerogit.UpstreamRemote(ctx, cwd, branch, nil)
},
branchUpstreamRemoteAndMerge: func(ctx context.Context, cwd, branch string) (string, string) {
return zerogit.UpstreamRemoteAndMergeBranch(ctx, cwd, branch, nil)
},
resolveRemoteBranchTip: func(ctx context.Context, cwd, remote, branch string) (string, error) {
return zerogit.ResolveRemoteBranchTip(ctx, cwd, remote, branch, nil)
},
remoteHasBranch: func(ctx context.Context, cwd, remote, branch string) (bool, error) {
return zerogit.RemoteHasBranch(ctx, cwd, remote, branch, nil)
},
currentGitBranch: func(ctx context.Context, cwd string) string {
return zerogit.CurrentBranch(ctx, cwd, nil)
},
currentBranchTip: func(ctx context.Context, cwd string) string {
return zerogit.CurrentBranchTip(ctx, cwd, nil)
},
deleteBranch: func(ctx context.Context, cwd, fallbackBranch, branchToDelete string) error {
return zerogit.DeleteBranch(ctx, cwd, fallbackBranch, branchToDelete, nil)
},
resetBranchRef: func(ctx context.Context, cwd, branch, newTip, expectedOld string) error {
return zerogit.ResetBranchRef(ctx, cwd, branch, newTip, nil, expectedOld)
},
runTUI: tui.Run,
runEditor: openEditor,
checkUpdate: update.Check,
applyUpdate: update.Apply,
now: time.Now,
}
}
@@ -562,6 +618,45 @@ func fillAppDeps(deps appDeps) appDeps {
if deps.createPR == nil {
deps.createPR = defaults.createPR
}
if deps.createBranch == nil {
deps.createBranch = defaults.createBranch
}
if deps.isDefaultBranch == nil {
deps.isDefaultBranch = defaults.isDefaultBranch
}
if deps.currentGitUser == nil {
deps.currentGitUser = defaults.currentGitUser
}
if deps.headCommitSubject == nil {
deps.headCommitSubject = defaults.headCommitSubject
}
if deps.commitsAhead == nil {
deps.commitsAhead = defaults.commitsAhead
}
if deps.isUnbornRemote == nil {
deps.isUnbornRemote = defaults.isUnbornRemote
}
if deps.refreshTrackingRef == nil {
deps.refreshTrackingRef = defaults.refreshTrackingRef
}
if deps.branchUpstreamRemote == nil {
deps.branchUpstreamRemote = defaults.branchUpstreamRemote
}
if deps.branchUpstreamRemoteAndMerge == nil {
deps.branchUpstreamRemoteAndMerge = defaults.branchUpstreamRemoteAndMerge
}
if deps.remoteHasBranch == nil {
deps.remoteHasBranch = defaults.remoteHasBranch
}
if deps.currentGitBranch == nil {
deps.currentGitBranch = defaults.currentGitBranch
}
if deps.currentBranchTip == nil {
deps.currentBranchTip = defaults.currentBranchTip
}
// resolveRemoteBranchTip, deleteBranch, and resetBranchRef stay nil when
// unset so unit tests that mock createBranch without a real git tree do not
// hit real git restore/delete commands.
if deps.runTUI == nil {
deps.runTUI = defaults.runTUI
}
+52 -10
View File
@@ -1,6 +1,7 @@
package cli
import (
"errors"
"fmt"
"io"
"os"
@@ -160,20 +161,61 @@ func runDaemonStartDetached(paths daemon.Paths, stdout io.Writer, stderr io.Writ
cmd.Stdout = logFile
cmd.Stderr = logFile
background.ConfigureChildProcessGroup(cmd) // own process group: outlives this shell
if err := cmd.Start(); err != nil {
if err := startAndAwaitDaemonProcess(cmd, func() bool { return daemonReachable(paths) }, 5*time.Second, 25*time.Millisecond); err != nil {
if errors.Is(err, errDaemonStartTimeout) {
return writeAppError(stderr, err.Error()+"; see "+logPath, exitCrash)
}
return writeAppError(stderr, err.Error(), exitCrash)
}
_ = cmd.Process.Release()
fmt.Fprintf(stdout, "zero daemon started (socket %s)\n", paths.Socket)
return exitSuccess
}
deadline := time.Now().Add(5 * time.Second)
for time.Now().Before(deadline) {
if daemonReachable(paths) {
fmt.Fprintf(stdout, "zero daemon started (socket %s)\n", paths.Socket)
return exitSuccess
}
time.Sleep(25 * time.Millisecond)
var errDaemonStartTimeout = errors.New("daemon did not come up within timeout")
func startAndAwaitDaemonProcess(cmd *exec.Cmd, reachable func() bool, timeout time.Duration, pollInterval time.Duration) error {
if err := cmd.Start(); err != nil {
return err
}
return writeAppError(stderr, "daemon did not come up within timeout; see "+logPath, exitCrash)
if waitForDaemonReadiness(reachable, timeout, pollInterval) {
if err := cmd.Process.Release(); err != nil {
cleanupErr := terminateAndReapDaemonProcess(cmd)
if cleanupErr != nil {
return fmt.Errorf("release daemon process: %w (cleanup failed: %v)", err, cleanupErr)
}
return fmt.Errorf("release daemon process: %w", err)
}
return nil
}
if err := terminateAndReapDaemonProcess(cmd); err != nil {
return fmt.Errorf("%w; cleanup failed: %v", errDaemonStartTimeout, err)
}
return errDaemonStartTimeout
}
func waitForDaemonReadiness(reachable func() bool, timeout time.Duration, pollInterval time.Duration) bool {
deadline := time.Now().Add(timeout)
for {
if reachable() {
return true
}
remaining := time.Until(deadline)
if remaining <= 0 {
break
}
time.Sleep(min(pollInterval, remaining))
}
// The loop's last reachable() call already ran immediately before this one
// (right after the final Sleep, with the same deadline check separating
// them), so this adds no extra waiting time and is not a grace window for a
// slow-starting daemon — it only re-checks the same instant in case
// reachable() itself is non-deterministic. To tolerate a genuinely slow
// start, raise the timeout passed to startAndAwaitDaemonProcess instead.
return reachable()
}
func terminateAndReapDaemonProcess(cmd *exec.Cmd) error {
return background.TerminateCommand(cmd)
}
func runDaemonStop(args []string, stdout io.Writer, stderr io.Writer) int {
+88
View File
@@ -2,8 +2,15 @@ package cli
import (
"bytes"
"errors"
"os"
"os/exec"
"path/filepath"
"strings"
"testing"
"time"
"github.com/Gitlawb/zero/internal/background"
)
// isolateDaemonPaths points DefaultPaths at a temp dir so the test never touches
@@ -121,3 +128,84 @@ func TestDaemonSubcommandsRejectExtraArgs(t *testing.T) {
}
}
}
func TestWaitForDaemonReadinessChecksTimeoutBoundary(t *testing.T) {
checks := 0
ready := waitForDaemonReadiness(func() bool {
checks++
return checks == 2
}, 0, time.Millisecond)
if !ready {
t.Fatal("daemon that became reachable at the timeout boundary was not detected")
}
if checks != 2 {
t.Fatalf("reachability checks = %d, want initial and final checks", checks)
}
}
func TestTerminateAndReapDaemonProcess(t *testing.T) {
cmd := daemonDetachedChildCommand("hang")
if err := cmd.Start(); err != nil {
t.Fatalf("start helper process: %v", err)
}
if err := terminateAndReapDaemonProcess(cmd); err != nil {
t.Fatalf("terminate and reap helper process: %v", err)
}
if cmd.ProcessState == nil {
t.Fatalf("helper process was not reaped: state=%v", cmd.ProcessState)
}
if cmd.ProcessState.Success() {
t.Fatalf("helper process unexpectedly exited successfully: %v", cmd.ProcessState)
}
}
func TestStartAndAwaitDaemonProcessTimeoutTerminatesAndReaps(t *testing.T) {
cmd := daemonDetachedChildCommand("hang")
if err := startAndAwaitDaemonProcess(cmd, func() bool { return false }, 0, time.Millisecond); !errors.Is(err, errDaemonStartTimeout) {
t.Fatalf("start and await error = %v, want timeout", err)
}
if cmd.ProcessState == nil {
t.Fatal("timed-out helper process was not reaped")
}
}
func TestStartAndAwaitDaemonProcessReleasesReadyChild(t *testing.T) {
marker := filepath.Join(t.TempDir(), "child-finished")
cmd := daemonDetachedChildCommand("mark", "ZERO_TEST_DAEMON_CHILD_MARKER="+marker)
if err := startAndAwaitDaemonProcess(cmd, func() bool { return true }, time.Second, time.Millisecond); err != nil {
t.Fatalf("start and await ready helper: %v", err)
}
deadline := time.Now().Add(5 * time.Second)
for time.Now().Before(deadline) {
if _, err := os.Stat(marker); err == nil {
return
}
time.Sleep(10 * time.Millisecond)
}
t.Fatal("released helper process did not remain alive to write its marker")
}
func daemonDetachedChildCommand(mode string, extraEnv ...string) *exec.Cmd {
cmd := exec.Command(os.Args[0], "-test.run=TestDaemonDetachedChildProcess")
cmd.Env = append(os.Environ(), "ZERO_TEST_DAEMON_DETACHED_CHILD="+mode)
cmd.Env = append(cmd.Env, extraEnv...)
background.ConfigureChildProcessGroup(cmd)
return cmd
}
func TestDaemonDetachedChildProcess(t *testing.T) {
switch os.Getenv("ZERO_TEST_DAEMON_DETACHED_CHILD") {
case "":
return
case "mark":
if err := os.WriteFile(os.Getenv("ZERO_TEST_DAEMON_CHILD_MARKER"), []byte("ready"), 0o600); err != nil {
os.Exit(2)
}
os.Exit(0)
}
for {
time.Sleep(time.Hour)
}
}
File diff suppressed because it is too large. Load diff
+538 -8
View File
@@ -2,9 +2,12 @@ package cli
import (
"context"
"errors"
"fmt"
"io"
"path"
"path/filepath"
"regexp"
"strconv"
"strings"
"time"
@@ -602,18 +605,20 @@ func parseChangesArgs(args []string, command string) (changesCommandOptions, boo
return options, false, execUsageError{fmt.Sprintf("unexpected changes argument %q", arg)}
}
}
if command != "commit" && options.message != "" {
if command != "commit" && options.hasMessage {
return options, false, execUsageError{"--message is only valid with `zero changes commit`"}
}
if command != "commit" && (options.hasMessage || options.dryRun || options.auto) {
return options, false, execUsageError{"--message, --dry-run, and --auto are only valid with `zero changes commit`"}
}
if command == "commit" && options.hasMessage && options.auto {
return options, false, execUsageError{"cannot specify both --message and --auto"}
}
if command != "commit" && command != "push" && options.dryRun {
return options, false, execUsageError{"--dry-run is only valid with commit or push"}
}
// --auto on push/pr is the explicit opt-in for LLM branch naming (see
// ensureFeatureBranch); on commit it opts into the LLM commit message.
if command != "commit" && command != "push" && command != "pr" && options.auto {
return options, false, execUsageError{"--auto is only valid with commit, push, or pr"}
}
if command != "inspect" && options.baseRef != "" {
return options, false, execUsageError{"--base is only valid with `zero changes inspect`"}
}
@@ -934,8 +939,7 @@ Flags:
--fill Automatically populate PR title and body from commits
--draft Create PR as a draft
--yes Confirm pushing to a default/protected branch
-a, --auto Auto-generate commit message using LLM (use --dry-run to preview)
--dry-run Preview commit metadata without mutating git state
-a, --auto Use the LLM: commit generates the message, push/pr name the auto-created branch (sends the diff to the provider)
--json Print JSON output
-h, --help Show this help
`)
@@ -958,11 +962,31 @@ func runChangesPush(args []string, stdout io.Writer, stderr io.Writer, deps appD
return writeExecUsageError(stderr, err.Error())
}
ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute)
defer cancel()
branch, remote, created, err := ensureFeatureBranch(ctx, stdout, workspaceRoot, options.remote, featureBranchOptions{
JSONMode: options.json,
AllowDefaultBranch: options.yes,
DryRun: options.dryRun,
AutoNaming: options.auto,
MaxDiffBytes: options.maxDiffBytes,
}, deps)
if err != nil {
return writeExecUsageError(stderr, err.Error())
}
// context.Background() is deliberate: the 2-minute ctx above is the
// feature-branch preflight (inspection, remote probes, branch creation).
// The push itself must not be cancelled mid-transfer just because the
// preflight budget elapsed; a large push can legitimately take longer.
result, err := deps.pushChanges(context.Background(), zerogit.PushOptions{
Cwd: workspaceRoot,
Remote: options.remote,
Remote: firstNonEmptyString(options.remote, remote),
Branch: branch,
Force: options.force,
DryRun: options.dryRun,
RequireNewRemoteBranch: created,
AllowPushDefaultBranch: options.yes,
})
if err != nil {
@@ -1010,15 +1034,62 @@ func runChangesPR(args []string, stdout io.Writer, stderr io.Writer, deps appDep
return writeExecUsageError(stderr, err.Error())
}
ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute)
defer cancel()
branch, remote, created, err := ensureFeatureBranch(ctx, stdout, workspaceRoot, options.remote, featureBranchOptions{
JSONMode: options.json,
AllowDefaultBranch: options.yes,
DryRun: false,
AutoNaming: options.auto,
MaxDiffBytes: options.maxDiffBytes,
}, deps)
if err != nil {
return writeExecUsageError(stderr, err.Error())
}
// Resolve the target remote for the unborn-remote check. Explicit --remote wins,
// followed by the remote resolved by ensureFeatureBranch. When --yes is set,
// fall back to the branch upstream or "origin".
targetRemote := strings.TrimSpace(options.remote)
if targetRemote == "" {
targetRemote = remote
}
if targetRemote == "" {
currentBranch := ""
if deps.currentGitBranch != nil {
currentBranch = deps.currentGitBranch(context.Background(), workspaceRoot)
}
if deps.branchUpstreamRemote != nil && currentBranch != "" {
targetRemote = deps.branchUpstreamRemote(context.Background(), workspaceRoot, currentBranch)
}
if targetRemote == "" {
targetRemote = "origin"
}
}
// Probe errors are intentionally ignored so the flow can continue; if the
// probe fails, the subsequent push or PR operation provides fail-closed behavior.
if deps.isUnbornRemote != nil {
if unborn, unbornErr := deps.isUnbornRemote(context.Background(), workspaceRoot, targetRemote); unbornErr == nil && unborn {
return writeExecUsageError(stderr, fmt.Sprintf("cannot create pull request on unborn remote %s: push the initial default branch first", targetRemote))
}
}
if !options.json {
if _, err := fmt.Fprintln(stdout, "Pushing current branch to set upstream..."); err != nil {
return exitCrash
}
}
// context.Background() is deliberate here too: runChangesPR reuses the
// 2-minute ctx for preflight (branch creation and remote probes), but the
// push must not be cancelled mid-transfer if that budget elapsed.
pushResult, err := deps.pushChanges(context.Background(), zerogit.PushOptions{
Cwd: workspaceRoot,
Remote: options.remote,
Remote: firstNonEmptyString(options.remote, remote),
Branch: branch,
Force: options.force,
RequireNewRemoteBranch: created,
AllowPushDefaultBranch: options.yes,
})
if err != nil {
@@ -1100,3 +1171,462 @@ func generateAutoCommitMessage(ctx context.Context, provider zeroruntime.Provide
}
return msg, nil
}
type featureBranchOptions struct {
JSONMode bool
AllowDefaultBranch bool
DryRun bool
AutoNaming bool
MaxDiffBytes int
}
// branchPushPlan holds the resolved parameters for a push or PR workflow
// before any branches are created or refs mutated. Separating plan resolution
// from mutation allows tests to assert the resolution matrix directly without
// touching a git repository.
type branchPushPlan struct {
ShouldBranch bool
CurrentBranch string
PushRemote string
RequireNewRemoteBranch bool
TrackingRemote string
TrackingBranch string
RestoreTip string
PublishabilityBase string
Summary zerogit.ChangeSummary
Slug string
}
// resolveBranchPushPlan resolves the branch, remotes, restore tip, and naming
// slug for changes push / pr workflows. It performs all checks (clean working
// tree, unborn remote detection, ahead count, restore tip resolution) but
// executes no mutation (no branch creation, ref resets, or deletions).
func resolveBranchPushPlan(ctx context.Context, stdout io.Writer, workspaceRoot string, requestedRemote string, opts featureBranchOptions, deps appDeps) (branchPushPlan, error) {
if opts.AllowDefaultBranch || opts.DryRun {
return branchPushPlan{
ShouldBranch: false,
PushRemote: strings.TrimSpace(requestedRemote),
RequireNewRemoteBranch: false,
}, nil
}
if deps.isDefaultBranch == nil {
return branchPushPlan{}, fmt.Errorf("isDefaultBranch dependency missing")
}
isDefault, currentBranch, remote, err := deps.isDefaultBranch(ctx, zerogit.DefaultBranchOptions{Cwd: workspaceRoot, Remote: requestedRemote})
if err != nil {
return branchPushPlan{}, err
}
if !isDefault {
// Whether this push needs the nonexistence lease is decided by one
// live check against the destination remote, not by inference from
// local git state (upstream config, a generated-branch marker):
// every local signal that stood in for that check turned out to be
// wrong in some ordering (inherited config from checkout -b, a
// config write that failed after a successful push, a push aimed at
// a different remote than the one local config describes). A branch
// missing on the remote gets the nonexistence lease, protecting a
// concurrent creator of the same name; a branch already there gets a
// plain push, and git's own fast-forward check is the safety net —
// if the remote history isn't ours, the push fails with a clear
// non-fast-forward error instead of an attempted silent recovery.
if deps.remoteHasBranch == nil {
return branchPushPlan{
ShouldBranch: false,
CurrentBranch: currentBranch,
PushRemote: remote,
RequireNewRemoteBranch: true,
}, nil
}
targetRemote := firstNonEmptyString(requestedRemote, remote)
exists, existsErr := deps.remoteHasBranch(ctx, workspaceRoot, targetRemote, currentBranch)
if existsErr != nil {
return branchPushPlan{}, fmt.Errorf("cannot check whether %s already exists on remote %s: %w", currentBranch, targetRemote, existsErr)
}
return branchPushPlan{
ShouldBranch: false,
CurrentBranch: currentBranch,
PushRemote: remote,
RequireNewRemoteBranch: !exists,
}, nil
}
// CreateBranch and Push publish commits only. A dirty working tree would
// leave uncommitted edits behind under a branch/PR that does not include
// them, so refuse until the tree is clean (commit or stash first).
workingTree, err := deps.inspectChanges(ctx, zerogit.InspectOptions{Cwd: workspaceRoot})
if err != nil {
return branchPushPlan{}, fmt.Errorf("failed to inspect working tree: %w", err)
}
if !workingTree.Clean {
return branchPushPlan{}, fmt.Errorf("working tree has uncommitted changes; commit or stash them before pushing from the default branch")
}
// Capture the source default branch's own upstream before refreshing or
// leaving it. --remote selects the push destination; restore must not
// rewrite local main to upstream/main when main still tracks origin/main
// (fork setups), and refreshing must update the exact tracking ref that
// will supply both the ahead count and the restore target.
trackingRemote := remote
trackingBranch := currentBranch
if deps.branchUpstreamRemoteAndMerge != nil {
if r, m := deps.branchUpstreamRemoteAndMerge(ctx, workspaceRoot, currentBranch); r != "" && m != "" {
trackingRemote = r
trackingBranch = m
}
}
// Refresh the local remote-tracking ref before trusting it: IsDefaultBranch
// already contacted the remote for its symref check, but the tracking ref
// commitsAhead reads from is only a local cache (written at clone or the
// last fetch) and can sit behind the remote's live tip. Left stale, a real
// publishable range could look like zero commits ahead, and the diff
// derived below (which reuses this same ref as its base) would be stale
// too. A failure here (offline, or a genuinely unborn remote with no such
// ref to fetch yet) is folded into the same fail-closed path as an
// unresolvable ahead count below.
var fetchErr error
if deps.refreshTrackingRef != nil {
fetchErr = deps.refreshTrackingRef(ctx, workspaceRoot, trackingRemote, trackingBranch)
}
// The ahead check gates on the source upstream (trackingRemote), but the
// feature branch is pushed to `pushRemote`, which can be a different
// destination (--remote on a fork). Refuse to auto-branch onto an unborn
// destination: without an initial default branch the new feature branch
// becomes the destination's first ref and its HEAD is left pointing at it.
// This is checked once, here, against the push destination, before resolving
// commit tips or creating any branches.
pushRemote := firstNonEmptyString(requestedRemote, remote)
if deps.isUnbornRemote != nil {
if unbornDest, unbornDestErr := deps.isUnbornRemote(ctx, workspaceRoot, pushRemote); unbornDestErr == nil && unbornDest {
return branchPushPlan{}, fmt.Errorf("remote %s has no branches yet; push the initial default branch first with --yes (`zero changes push --yes`), then use auto-branching for subsequent work", pushRemote)
}
}
// Branching off the default branch only makes sense when HEAD carries a
// commit that is not already on the remote default branch. A clean,
// up-to-date default branch would otherwise publish a feature branch at the
// exact default tip. If the ahead count cannot be determined (for example
// the remote-tracking ref was never fetched), fail rather than guess. When
// the remote is confirmed unborn, refuse auto-branching entirely: the
// initial default branch must be established first (changes push --yes)
// so the remote has a real HEAD before any feature branch is published.
ahead, aheadErr := deps.commitsAhead(ctx, workspaceRoot, trackingRemote, trackingBranch)
if fetchErr != nil || aheadErr != nil {
var unbornRemote bool
var unbornErr error
if deps.isUnbornRemote != nil {
unbornRemote, unbornErr = deps.isUnbornRemote(ctx, workspaceRoot, trackingRemote)
}
if unbornErr == nil && unbornRemote {
return branchPushPlan{}, fmt.Errorf("remote %s has no branches yet; push the initial default branch first with --yes (`zero changes push --yes`), then use auto-branching for subsequent work", trackingRemote)
}
cause := aheadErr
if cause == nil {
cause = fetchErr
}
return branchPushPlan{}, fmt.Errorf("cannot determine whether HEAD is ahead of %s/%s: %w; fetch the remote tracking branch first", trackingRemote, trackingBranch, cause)
}
if ahead == 0 {
return branchPushPlan{}, fmt.Errorf("no changes to publish: HEAD is not ahead of %s/%s; commit your work before pushing", trackingRemote, trackingBranch)
}
// Resolve the restore tip to a concrete commit before creating any branch.
// It must never be a bare local branch name: resolving "main" yields the
// very commit the feature branch is about to own, so restore becomes a
// silent no-op and the publishable commits stay on the default branch. A
// named remote resolves through its just-refreshed remote-tracking ref; a
// direct URL or path asks the remote itself. Fail closed rather than
// substitute a local branch name.
restoreTip := ""
if deps.resolveRemoteBranchTip != nil {
tip, tipErr := deps.resolveRemoteBranchTip(ctx, workspaceRoot, trackingRemote, trackingBranch)
if tipErr != nil {
return branchPushPlan{}, fmt.Errorf("cannot resolve restore tip for %s/%s: %w", trackingRemote, trackingBranch, tipErr)
}
restoreTip = tip
}
if restoreTip == "" {
restoreTip = trackingRemote + "/" + trackingBranch
}
// Name the branch (and, with --auto, send the provider) from what HEAD is
// actually ahead of the resolved remote branch by, using the same ref
// commitsAhead just checked. A working-tree snapshot can describe edits a
// commit-only push will never include.
baseRef := restoreTip
summary, err := deps.inspectChanges(ctx, zerogit.InspectOptions{Cwd: workspaceRoot, BaseRef: baseRef, MaxDiffBytes: opts.MaxDiffBytes})
if err != nil {
return branchPushPlan{}, fmt.Errorf("failed to inspect changes: %w", err)
}
slug := fallbackBranchSlug(summary)
if len(summary.Files) == 0 {
// An empty commit (or one whose only content the diff omits) leaves
// nothing to derive a slug from; name the branch from the commit
// subject instead.
if subject := deps.headCommitSubject(ctx, workspaceRoot); subject != "" {
slug = zerogit.SlugifyBranchComponent(subject)
}
}
if opts.AutoNaming && strings.TrimSpace(summary.Diff) != "" {
llmSlug := ""
if resolved, cfgErr := deps.resolveConfig(workspaceRoot, config.Overrides{}); cfgErr == nil && config.HasProviderProfile(resolved.Provider) {
if provider, provErr := deps.newProvider(resolved.Provider); provErr == nil {
if !opts.JSONMode {
fmt.Fprintln(stdout, "Generating branch name using LLM...")
}
genCtx, cancel := context.WithTimeout(ctx, 60*time.Second)
generated, genErr := generateAutoBranchSlug(genCtx, provider, resolved.Provider.Model, redactChangeSummary(summary))
cancel()
if genErr == nil && generated != "" {
llmSlug = generated
}
}
}
if llmSlug != "" {
slug = llmSlug
} else if !opts.JSONMode {
// Cover resolveConfig failures, missing provider profiles,
// newProvider errors, generation errors, and empty results so the
// user is not left guessing after "Generating..." or silence.
fmt.Fprintln(stdout, "LLM branch naming unavailable; using deterministic name.")
}
}
return branchPushPlan{
ShouldBranch: true,
CurrentBranch: currentBranch,
PushRemote: pushRemote,
RequireNewRemoteBranch: true,
TrackingRemote: trackingRemote,
TrackingBranch: trackingBranch,
RestoreTip: restoreTip,
PublishabilityBase: baseRef,
Summary: summary,
Slug: slug,
}, nil
}
// ensureFeatureBranch is the branch-naming step `zero changes push`/`pr` run
// before pushing: pushing straight to the default branch is refused deeper in
// zerogit.Push, so rather than surface that as a dead end, create and switch
// to a conventionally named "<user>/<slug>" branch first. It returns the
// branch push/pr should target, or "" to mean "current HEAD branch, unchanged"
// (zerogit.Push already treats an empty Branch that way), plus the remote the
// preflight resolved (requestedRemote, then the original branch's configured
// upstream, then "origin"), plus whether Push should require that branch not
// already exist on that remote. Callers must pass the remote to Push: a
// freshly created branch has no tracking configuration, so Push's own
// fallback would silently retarget "origin" even when the work came from a
// branch tracking a different remote. Callers must also pass that third value
// through as Push's RequireNewRemoteBranch: CreateBranch's own
// remote-collision probe runs before this returns, and closing that race
// requires Push's push itself to assert the destination is still new. That
// requirement also carries across a retry: if the current branch is already
// non-default (this call didn't create it) but has not been successfully
// published to the exact target remote under the same branch name, keep the
// nonexistence lease. branch.<name>.remote alone is not enough: with
// branch.autoSetupMerge=inherit, checkout -b copies remote=origin from the
// source branch before any push. After a successful create, the original
// default branch ref is moved back to its own upstream tip (not necessarily
// the push destination) so the feature branch exclusively owns the
// publishable commits; a failed restore rolls the feature branch back.
// allowDefaultBranch (the --yes flag) and dryRun both opt out via the ""
// branch / false return, leaving Push's own guard/preview behavior on the
// default branch unaffected.
//
// autoNaming gates the LLM naming path (--auto): these commands were
// git-only, and sending the change diff to a configured provider on every
// default-branch push would silently export source code nobody asked to
// share. Without the opt-in the name comes from deterministic local
// information only. maxDiffBytes caps the committed-range diff Inspect
// returns, so a user who passed --diff-bytes to bound the proprietary source
// sent for LLM naming has that cap honored here just as the commit path does.
// The working tree must be clean and HEAD must be ahead of the resolved
// remote branch; both are verified before any branch is created. The
// inspectChanges, commitsAhead, headCommitSubject, currentGitUser, and
// createBranch dependencies are required on the default-branch path and are
// called without nil guards; fillAppDeps populates all of them. The
// refreshTrackingRef, resolveRemoteBranchTip, isUnbornRemote, resetBranchRef,
// and deleteBranch dependencies are optional and are skipped when nil.
func ensureFeatureBranch(ctx context.Context, stdout io.Writer, workspaceRoot string, requestedRemote string, opts featureBranchOptions, deps appDeps) (string, string, bool, error) {
plan, err := resolveBranchPushPlan(ctx, stdout, workspaceRoot, requestedRemote, opts, deps)
if err != nil {
return "", "", false, err
}
if !plan.ShouldBranch {
return plan.CurrentBranch, plan.PushRemote, plan.RequireNewRemoteBranch, nil
}
expectedRestoreTip := ""
if deps.currentBranchTip != nil {
expectedRestoreTip = deps.currentBranchTip(ctx, workspaceRoot)
}
name := zerogit.BuildBranchName(deps.currentGitUser(ctx, workspaceRoot), plan.Slug)
result, err := deps.createBranch(ctx, zerogit.BranchOptions{Cwd: workspaceRoot, Name: name, Remote: plan.PushRemote})
if err != nil {
return "", "", false, fmt.Errorf("failed to create branch: %w", err)
}
// The normal flow commits on the default branch first, then creates the
// feature branch at the same HEAD. Without moving the original default
// ref back, local main keeps the pre-squash commits and diverges from
// its upstream after a squash-merge (and a later pull/push can re-publish
// them). Once the feature branch owns those commits, restore the default
// branch to the source branch's own upstream tip captured above.
// Failure-safe: if the restore fails, delete the feature branch and
// check the default branch back out so the tree is not left half-moved.
if deps.resetBranchRef != nil {
if err := deps.resetBranchRef(ctx, workspaceRoot, plan.CurrentBranch, plan.RestoreTip, expectedRestoreTip); err != nil {
// A compare-and-swap conflict means the default branch moved
// while we were branching, so restoreTip is no longer the tip we
// meant to restore. The rollback below deletes the branch that
// now exclusively owns the user's commits, which would destroy
// work to recover from a race we can simply report. Keep the
// branch and let the user reconcile the default branch.
if errors.Is(err, zerogit.ErrCompareAndSwapConflict) {
return "", "", false, fmt.Errorf("failed to restore default branch %s to %s after auto-branching: %w; generated branch %s was preserved because the default branch changed concurrently", plan.CurrentBranch, plan.RestoreTip, err, result.Branch)
}
var rollbackErr error
if deps.deleteBranch != nil {
rollbackErr = deps.deleteBranch(ctx, workspaceRoot, plan.CurrentBranch, result.Branch)
}
if rollbackErr != nil {
return "", "", false, fmt.Errorf("failed to restore default branch %s to %s after auto-branching: %w; rollback of %s also failed: %v", plan.CurrentBranch, plan.RestoreTip, err, result.Branch, rollbackErr)
}
return "", "", false, fmt.Errorf("failed to restore default branch %s to %s after auto-branching: %w", plan.CurrentBranch, plan.RestoreTip, err)
}
}
if !opts.JSONMode {
fmt.Fprintf(stdout, "Created branch %s (was on %s)\n", result.Branch, plan.CurrentBranch)
}
return result.Branch, plan.PushRemote, true, nil
}
// fallbackBranchSlug derives a deterministic branch-name slug from a change
// summary without calling an LLM, so ensureFeatureBranch still works when no
// provider is configured.
func fallbackBranchSlug(summary zerogit.ChangeSummary) string {
switch len(summary.Files) {
case 0:
return "changes"
case 1:
// Git paths are always slash-separated regardless of platform, so
// path.Base (not filepath.Base) extracts the filename portably.
return zerogit.SlugifyBranchComponent(path.Base(summary.Files[0].Path))
default:
return fmt.Sprintf("update-%d-files", len(summary.Files))
}
}
// generateAutoBranchSlug asks the model for a short kebab-case slug
// describing the diff, mirroring generateAutoCommitMessage's prompt shape.
func generateAutoBranchSlug(ctx context.Context, provider zeroruntime.Provider, model string, summary zerogit.ChangeSummary) (string, error) {
var promptBuilder strings.Builder
promptBuilder.WriteString("Analyze the following git diff and generate a short git branch name slug for it.\n")
promptBuilder.WriteString("The slug must be 2 to 5 lowercase words separated by hyphens (kebab-case), using only letters, digits, and hyphens, with no prefix like \"feature/\" or \"fix/\" and no surrounding quotes.\n")
promptBuilder.WriteString("Output ONLY the raw slug text, nothing else.\n\n")
promptBuilder.WriteString("Git Diff:\n")
promptBuilder.WriteString(summary.Diff)
request := zeroruntime.CompletionRequest{
Messages: []zeroruntime.Message{
{Role: zeroruntime.MessageRoleUser, Content: promptBuilder.String()},
},
}
stream, err := provider.StreamCompletion(ctx, request)
if err != nil {
return "", err
}
collected := zeroruntime.CollectStream(ctx, stream)
if collected.Error != "" {
return "", fmt.Errorf("%s", collected.Error)
}
slug := zerogit.SlugifyBranchComponent(extractBranchSlug(collected.Text))
if slug == "" {
return "", fmt.Errorf("provider returned empty branch slug")
}
return slug, nil
}
// slugLineRe matches a line that already reads as a kebab-case slug (letters,
// digits, and internal single hyphens only). extractBranchSlug prefers such a
// line so a preamble sentence is never mistaken for the slug.
var slugLineRe = regexp.MustCompile(`^[a-zA-Z0-9]+(-[a-zA-Z0-9]+)*$`)
func isPreambleText(s string) bool {
lower := strings.ToLower(strings.TrimSpace(s))
if strings.HasSuffix(lower, ":") || strings.HasSuffix(lower, ".") || strings.HasSuffix(lower, "!") || strings.HasSuffix(lower, "?") {
return true
}
preambles := []string{
"here is", "here's", "below is", "below are", "sure", "certainly",
"i suggest", "suggested branch", "branch name", "recommended branch",
"how about", "you could use",
}
for _, p := range preambles {
if strings.HasPrefix(lower, p) {
return true
}
}
return false
}
// extractBranchSlug pulls the intended slug out of a model response that didn't
// follow the "output only the raw slug" instruction exactly. It drops Markdown
// code-fence lines, then prefers a line that already looks like a kebab-case
// slug: that skips a leading preamble such as "Here is a suggested branch
// name:" in favor of the "add-login-page" line that follows it. One-word
// acknowledgements that happen to match the slug regex ("Sure", "Certainly")
// are filtered as preamble before the early return, so "Sure\nadd login page"
// yields the real suggestion. Inline labeled forms ("Branch name: add-login-page")
// still work: the preamble check runs on the extracted value after the label
// is stripped. If no line is strictly kebab-cased, it prefers a plausible
// non-preamble line so plain multi-word replies still slugify correctly.
func extractBranchSlug(text string) string {
var firstLine string
for _, line := range strings.Split(text, "\n") {
line = strings.TrimSpace(line)
if strings.HasPrefix(line, "```") {
continue
}
line = strings.TrimSpace(strings.Trim(line, `"'`))
if line == "" {
continue
}
candidate := line
if idx := strings.Index(line, ":"); idx != -1 {
after := strings.TrimSpace(strings.Trim(line[idx+1:], `"'`))
if after != "" {
candidate = after
}
}
// Models often terminate a suggestion with sentence punctuation;
// strip it so "add login page." is recovered as the slug rather than
// rejected as preamble (isPreambleText treats a trailing '.' as a
// full sentence). True preamble lines still match the prefix list.
candidate = strings.TrimSpace(strings.TrimRight(candidate, ".!?"))
if candidate == "" {
continue
}
// Preamble classification runs before accepting a slug-shaped line so
// "Sure" does not win over a later "add-login-page". The check is on
// candidate (post label-strip) so "Branch name: add-login-page" still
// returns the extracted slug.
if slugLineRe.MatchString(candidate) && !isPreambleText(candidate) {
return candidate
}
if firstLine == "" && !isPreambleText(candidate) {
firstLine = candidate
}
}
return firstLine
}
+246
View File
@@ -0,0 +1,246 @@
package credstore
import (
"fmt"
"os"
"os/exec"
"path/filepath"
"strings"
"sync"
"testing"
)
func fileStore(t *testing.T, dir string) *Store {
t.Helper()
store, err := New(Options{Dir: dir, Storage: "file"})
if err != nil {
t.Fatalf("New: %v", err)
}
return store
}
// THE REPRODUCTION, kept as the regression. Before the lock this reliably kept
// 1 of 100: every writer read the same map, added its own provider, and the
// last rename published a file missing all the others.
func TestConcurrentSetKeepsEveryKey(t *testing.T) {
dir := t.TempDir()
store := fileStore(t, dir)
const n = 100
var wg sync.WaitGroup
errs := make(chan error, n)
for i := 0; i < n; i++ {
wg.Add(1)
go func(i int) {
defer wg.Done()
if err := store.Set(fmt.Sprintf("p%03d", i), fmt.Sprintf("k%03d", i)); err != nil {
errs <- err
}
}(i)
}
wg.Wait()
close(errs)
for err := range errs {
t.Fatalf("Set: %v", err)
}
got, err := store.Providers()
if err != nil {
t.Fatalf("Providers: %v", err)
}
if len(got) != n {
t.Fatalf("kept %d of %d concurrent writes", len(got), n)
}
// ...and every value is the one its own writer wrote, not a neighbour's.
for i := 0; i < n; i++ {
key, ok, err := store.Get(fmt.Sprintf("p%03d", i))
if err != nil {
t.Fatalf("Get: %v", err)
}
if !ok || key != fmt.Sprintf("k%03d", i) {
t.Fatalf("p%03d = %q (present=%v); a writer's own value must survive", i, key, ok)
}
}
}
// DELETE RACES SET, and an unlocked delete loses the other writer's key.
//
// A first version of this only checked that keys nobody deleted survived — and
// every reader sees those, so it passed with the lock removed from Delete. The
// case that actually breaks is a Delete whose stale read predates a concurrent
// Set: it writes back a map that never contained the new key, and the Set is
// gone. So the assertion is on the SETS surviving, not on the bystanders.
func TestADeleteCannotClobberAConcurrentSet(t *testing.T) {
dir := t.TempDir()
store := fileStore(t, dir)
// Throwaway keys for the deleters to remove, written first so a delete
// always has something to do.
const churn = 40
for i := 0; i < churn; i++ {
if err := store.Set(fmt.Sprintf("churn%02d", i), "v"); err != nil {
t.Fatal(err)
}
}
const adds = 60
var wg sync.WaitGroup
for i := 0; i < adds; i++ {
wg.Add(1)
go func(i int) {
defer wg.Done()
_ = store.Set(fmt.Sprintf("added%02d", i), "v")
}(i)
}
for i := 0; i < churn; i++ {
wg.Add(1)
go func(i int) {
defer wg.Done()
_, _ = store.Delete(fmt.Sprintf("churn%02d", i))
}(i)
}
wg.Wait()
got, err := store.Providers()
if err != nil {
t.Fatal(err)
}
survivors := map[string]bool{}
for _, name := range got {
survivors[name] = true
}
missing := []string{}
for i := 0; i < adds; i++ {
name := fmt.Sprintf("added%02d", i)
if !survivors[name] {
missing = append(missing, name)
}
}
if len(missing) > 0 {
t.Fatalf("%d of %d concurrent Set calls were clobbered by a racing Delete: %v",
len(missing), adds, missing)
}
// ...and every churn key really was removed, so the deletes were not
// no-ops that would make the check above vacuous.
for i := 0; i < churn; i++ {
if survivors[fmt.Sprintf("churn%02d", i)] {
t.Fatalf("churn%02d survived; the deletes did nothing and this test proves nothing", i)
}
}
}
// ACROSS PROCESSES, which is the case that matters: a plan running with
// max_workers > 1 spawns child PROCESSES, and an in-process mutex would not
// have helped any of them. The race detector cannot see this one either — it is
// two OS processes — so it is driven for real.
func TestConcurrentSetAcrossProcesses(t *testing.T) {
if testing.Short() {
t.Skip("spawns processes")
}
helper := os.Getenv("ZERO_CREDSTORE_HELPER_DIR")
if helper != "" {
// Child mode: write our slice of the keys and exit.
store, err := New(Options{Dir: helper, Storage: "file"})
if err != nil {
os.Exit(3)
}
prefix := os.Getenv("ZERO_CREDSTORE_HELPER_PREFIX")
for i := 0; i < 25; i++ {
if err := store.Set(fmt.Sprintf("%s%02d", prefix, i), "v"); err != nil {
os.Exit(4)
}
}
os.Exit(0)
}
dir := t.TempDir()
const children = 4
var wg sync.WaitGroup
failures := make(chan string, children)
for c := 0; c < children; c++ {
wg.Add(1)
go func(c int) {
defer wg.Done()
cmd := exec.Command(os.Args[0], "-test.run=TestConcurrentSetAcrossProcesses", "-test.v")
cmd.Env = append(os.Environ(),
"ZERO_CREDSTORE_HELPER_DIR="+dir,
fmt.Sprintf("ZERO_CREDSTORE_HELPER_PREFIX=c%d_", c),
)
if out, err := cmd.CombinedOutput(); err != nil {
failures <- fmt.Sprintf("child %d: %v\n%s", c, err, out)
}
}(c)
}
wg.Wait()
close(failures)
for failure := range failures {
t.Fatal(failure)
}
store := fileStore(t, dir)
got, err := store.Providers()
if err != nil {
t.Fatal(err)
}
if len(got) != children*25 {
t.Fatalf("kept %d of %d keys written by %d concurrent processes", len(got), children*25, children)
}
}
// The lock lives BESIDE the data file, never on it. write publishes by rename,
// so a lock taken on the data file would be attached to an inode the next
// writer has already replaced — every writer would appear to hold it.
func TestTheLockIsNotTheDataFile(t *testing.T) {
dir := t.TempDir()
store := fileStore(t, dir)
if store.lockPath() == store.file {
t.Fatal("the lock is the data file; rename would carry it away")
}
if !strings.HasPrefix(filepath.Base(store.lockPath()), filepath.Base(store.file)) {
t.Fatalf("lock %q is not beside the data file %q", store.lockPath(), store.file)
}
if err := store.Set("p", "k"); err != nil {
t.Fatal(err)
}
if _, err := os.Stat(store.lockPath()); err != nil {
t.Fatalf("the lock file was not created: %v", err)
}
// ...and it survives a write, since the rename replaces only the data file.
if err := store.Set("q", "k"); err != nil {
t.Fatal(err)
}
if _, err := os.Stat(store.lockPath()); err != nil {
t.Fatalf("the lock file did not survive a write: %v", err)
}
}
// The lock is a security boundary, so the path where it CANNOT be taken needs
// coverage too: an operation that could not lock must report that, never proceed
// unserialized and never report success. Replacing the credential directory with
// a regular file makes the lock's MkdirAll fail on every platform.
func TestOperationsFailWhenTheLockCannotBeAcquired(t *testing.T) {
base := t.TempDir()
dir := filepath.Join(base, "creds")
store := fileStore(t, dir)
// Where the store expects its directory, put a regular file.
if err := os.WriteFile(dir, []byte("not a directory"), 0o600); err != nil {
t.Fatal(err)
}
if err := store.Set("openai", "sk-test"); err == nil {
t.Error("Set reported success although the lock could not be acquired")
}
if _, err := store.Delete("openai"); err == nil {
t.Error("Delete reported success although the lock could not be acquired")
}
if _, _, err := store.Get("openai"); err == nil {
t.Error("Get reported success although the lock could not be acquired")
}
if _, err := store.Providers(); err == nil {
t.Error("Providers reported success although the lock could not be acquired")
}
// And nothing may have been written through the failed path.
if raw, err := os.ReadFile(dir); err != nil || string(raw) != "not a directory" {
t.Errorf("the blocking file was modified: %q (err %v)", raw, err)
}
}
+49 -5
View File
@@ -9,6 +9,7 @@ package credstore
import (
"encoding/json"
"errors"
"fmt"
"os"
"path/filepath"
@@ -125,14 +126,27 @@ func (s *Store) Backend() string { return s.backend }
func (s *Store) Encrypted() bool { return s.backend == "keyring" || s.backend == "encrypted-file" }
// Set stores (or overwrites) the API key for provider.
func (s *Store) Set(provider, key string) error {
func (s *Store) Set(provider, key string) (err error) {
provider = normalizeProvider(provider)
if provider == "" {
return fmt.Errorf("credstore: provider is required")
}
if s.backend == "keyring" {
// The OS keyring serializes its own writes; the file lock guards the
// file backends only.
return s.kr.Set(keyringService, keyringPrefix+provider, key)
}
// READ AND WRITE UNDER ONE LOCK. Separately locking each half would still
// lose keys: two writers would each read the same map, each add their own
// provider, and the second rename would publish a file missing the first.
// Measured before this existed: 1 of 100 concurrent Set calls survived.
release, err := s.acquireFileLock(true)
if err != nil {
return err
}
// Joined, not chosen between: a release failure annotates the result instead
// of masking the write error that actually explains what went wrong.
defer func() { err = errors.Join(err, release()) }()
data, err := s.read()
if err != nil {
return err
@@ -142,7 +156,7 @@ func (s *Store) Set(provider, key string) error {
}
// Get returns the API key for provider and whether one is stored.
func (s *Store) Get(provider string) (string, bool, error) {
func (s *Store) Get(provider string) (value string, found bool, err error) {
provider = normalizeProvider(provider)
if provider == "" {
return "", false, nil
@@ -150,6 +164,15 @@ func (s *Store) Get(provider string) (string, bool, error) {
if s.backend == "keyring" {
return s.kr.Get(keyringService, keyringPrefix+provider)
}
// A shared lock, so a read serializes against a writer's publish. On unix
// the rename is atomic and this only prevents reading an empty file mid-swap;
// on Windows an unsynchronized reader holding the file open would block the
// writer's rename outright, so readers must coordinate through the same lock.
release, err := s.acquireFileLock(false)
if err != nil {
return "", false, err
}
defer func() { err = errors.Join(err, release()) }()
data, err := s.read()
if err != nil {
return "", false, err
@@ -159,7 +182,7 @@ func (s *Store) Get(provider string) (string, bool, error) {
}
// Delete removes the API key for provider, reporting whether one existed.
func (s *Store) Delete(provider string) (bool, error) {
func (s *Store) Delete(provider string) (existed bool, err error) {
provider = normalizeProvider(provider)
if provider == "" {
return false, nil
@@ -167,6 +190,13 @@ func (s *Store) Delete(provider string) (bool, error) {
if s.backend == "keyring" {
return s.kr.Delete(keyringService, keyringPrefix+provider)
}
// The same lock as Set: a delete is a read-modify-write too, and one racing
// a set would otherwise resurrect or erase an unrelated provider.
release, err := s.acquireFileLock(true)
if err != nil {
return false, err
}
defer func() { err = errors.Join(err, release()) }()
data, err := s.read()
if err != nil {
return false, err
@@ -179,17 +209,22 @@ func (s *Store) Delete(provider string) (bool, error) {
}
// Providers lists providers with a stored key (sorted), for display/audit.
func (s *Store) Providers() ([]string, error) {
func (s *Store) Providers() (names []string, err error) {
if s.backend == "keyring" {
// The OS keyring has no enumerate-by-prefix in our minimal surface; callers
// that need a list pass known provider names to Get. Return empty here.
return nil, nil
}
release, err := s.acquireFileLock(false)
if err != nil {
return nil, err
}
defer func() { err = errors.Join(err, release()) }()
data, err := s.read()
if err != nil {
return nil, err
}
names := make([]string, 0, len(data))
names = make([]string, 0, len(data))
for name := range data {
names = append(names, name)
}
@@ -264,6 +299,15 @@ func (s *Store) write(data map[string]string) error {
return nil
}
// lockPath is the advisory lock guarding a read-modify-write of the credential
// file. Beside the data file, never the data file itself — write publishes by
// rename and would carry the lock away with the old inode.
func (s *Store) lockPath() string { return s.file + ".lock" }
// filepathDir is filepath.Dir, named locally so the two platform lock files can
// share it without either importing path/filepath for one call.
func filepathDir(path string) string { return filepath.Dir(path) }
func normalizeProvider(provider string) string {
return strings.ToLower(strings.TrimSpace(provider))
}
+55
View File
@@ -0,0 +1,55 @@
//go:build !windows
package credstore
import (
"errors"
"fmt"
"os"
"golang.org/x/sys/unix"
)
// acquireFileLock takes an exclusive advisory lock (flock) so a read-modify-write
// of the credential file is serialized against every other one — across
// processes AND across goroutines, since flock is held per open file
// description and two opens in one process contend exactly as two processes do.
//
// THE LOCK FILE IS SEPARATE FROM THE DATA FILE, and that is not tidiness. write
// publishes by os.Rename, which replaces the inode; a lock taken on the data
// file would be attached to an inode that the next writer has already replaced,
// so every writer would appear to hold it. The lock lives on a file nothing
// renames.
func (s *Store) acquireFileLock(exclusive bool) (func() error, error) {
path := s.lockPath()
if err := os.MkdirAll(filepathDir(path), 0o700); err != nil {
return nil, fmt.Errorf("credstore: lock dir: %w", err)
}
file, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE, 0o600)
if err != nil {
return nil, fmt.Errorf("credstore: open lock: %w", err)
}
// Writers take LOCK_EX; readers take LOCK_SH so they run concurrently with
// each other but still serialize against a writer's publish (see Get).
how := unix.LOCK_SH
if exclusive {
how = unix.LOCK_EX
}
if err := unix.Flock(int(file.Fd()), how); err != nil {
_ = file.Close()
return nil, fmt.Errorf("credstore: lock: %w", err)
}
return func() error {
// Close alone drops the flock, so the explicit unlock is belt-and-braces —
// but its failure is reported rather than swallowed, because a cleanup that
// did not complete must not be indistinguishable from one that did. The
// callers join this into their result, so it annotates a successful write
// instead of masking it.
unlockErr := unix.Flock(int(file.Fd()), unix.LOCK_UN)
closeErr := file.Close()
if err := errors.Join(unlockErr, closeErr); err != nil {
return fmt.Errorf("credstore: release lock: %w", err)
}
return nil
}, nil
}
+55
View File
@@ -0,0 +1,55 @@
//go:build windows
package credstore
import (
"errors"
"fmt"
"os"
"golang.org/x/sys/windows"
)
// acquireFileLock takes an exclusive OS lock (LockFileEx) so a read-modify-write
// of the credential file is serialized, matching the flock behaviour on unix.
//
// The lock file is SEPARATE from the data file for the same reason: write
// publishes by rename, and a lock on the renamed file would be attached to
// something the next writer has already replaced.
func (s *Store) acquireFileLock(exclusive bool) (func() error, error) {
path := s.lockPath()
if err := os.MkdirAll(filepathDir(path), 0o700); err != nil {
return nil, fmt.Errorf("credstore: lock dir: %w", err)
}
file, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE, 0o600)
if err != nil {
return nil, fmt.Errorf("credstore: open lock: %w", err)
}
handle := windows.Handle(file.Fd())
overlapped := new(windows.Overlapped)
// A fixed 1-byte region, blocking (no LOCKFILE_FAIL_IMMEDIATELY) so a
// waiter queues rather than failing the operation. Writers pass
// LOCKFILE_EXCLUSIVE_LOCK; readers omit it (a shared lock) so they still
// serialize against a writer's publish — which matters more on Windows,
// where an unsynchronized reader holding the file open blocks the rename.
flags := uint32(0)
if exclusive {
flags = windows.LOCKFILE_EXCLUSIVE_LOCK
}
if err := windows.LockFileEx(handle, flags, 0, 1, 0, overlapped); err != nil {
_ = file.Close()
return nil, fmt.Errorf("credstore: lock: %w", err)
}
return func() error {
// Reported rather than swallowed, for the same reason as the unix side: a
// cleanup that did not complete must not look identical to one that did.
// It matters more here — on Windows the handle staying open is what blocks
// the next writer's rename, so a failed release has a visible consequence.
unlockErr := windows.UnlockFileEx(handle, 0, 1, 0, overlapped)
closeErr := file.Close()
if err := errors.Join(unlockErr, closeErr); err != nil {
return fmt.Errorf("credstore: release lock: %w", err)
}
return nil
}, nil
}
+39 -8
View File
@@ -9,6 +9,7 @@ import (
"strconv"
"strings"
"github.com/Gitlawb/zero/internal/providers/providerio"
"github.com/coder/websocket"
)
@@ -60,7 +61,15 @@ func (d *deepgramTranscriber) StreamTranscribe(ctx context.Context, chunks <-cha
HTTPHeader: http.Header{"Authorization": {"Token " + d.cfg.APIKey}},
})
if err != nil {
return "", fmt.Errorf("connecting to Deepgram: %w", err)
// Preserve the context.Canceled sentinel (it carries no key) so an
// Esc-abort during dial still matches the UI's errors.Is check. Key
// on ctx.Err() itself rather than unwrapping err: a cancellation can
// surface as a plain transport error (e.g. "closed network
// connection") rather than context.Canceled directly.
if ctx.Err() != nil {
return "", ctx.Err()
}
return "", fmt.Errorf("connecting to Deepgram: %s", providerio.Redact(err.Error(), d.cfg.APIKey))
}
defer conn.CloseNow()
@@ -68,15 +77,26 @@ func (d *deepgramTranscriber) StreamTranscribe(ctx context.Context, chunks <-cha
// so no conversion (unlike sherpa-onnx, which wants float32).
writeErrCh := make(chan error, 1)
go func() {
for chunk := range chunks {
if err := conn.Write(ctx, websocket.MessageBinary, chunk); err != nil {
writeErrCh <- err
// Select on ctx.Done() while waiting for the next chunk so a cancelled
// session does not leave this goroutine blocked on an open idle channel.
for {
select {
case <-ctx.Done():
writeErrCh <- ctx.Err()
return
case chunk, ok := <-chunks:
if !ok {
// CloseStream flushes any buffered audio and returns final
// results before Deepgram closes the socket.
writeErrCh <- conn.Write(ctx, websocket.MessageText, []byte(`{"type":"CloseStream"}`))
return
}
if err := conn.Write(ctx, websocket.MessageBinary, chunk); err != nil {
writeErrCh <- err
return
}
}
}
// CloseStream flushes any buffered audio and returns final results before
// Deepgram closes the socket.
writeErrCh <- conn.Write(ctx, websocket.MessageText, []byte(`{"type":"CloseStream"}`))
}()
// Deepgram results are per-utterance segments, not cumulative: accumulate the
@@ -100,7 +120,18 @@ func (d *deepgramTranscriber) StreamTranscribe(ctx context.Context, chunks <-cha
}
default:
}
return compose(), fmt.Errorf("Deepgram stream error: %w", err) //nolint:staticcheck // Preserve established user-facing error text.
// A user abort cancels the streaming context; the UI matches it
// with errors.Is(err, context.Canceled), so return the sentinel
// itself (it carries no key) instead of a flat redacted string.
// Key on ctx.Err() rather than unwrapping err: the writeErrCh
// swap above can replace err with a plain transport error (e.g.
// "closed network connection") that doesn't itself unwrap to
// context.Canceled even though the cancellation is what caused it.
if ctx.Err() != nil {
return compose(), ctx.Err()
}
//nolint:staticcheck // Preserve established user-facing error text.
return compose(), fmt.Errorf("Deepgram stream error: %s", providerio.Redact(err.Error(), d.cfg.APIKey))
}
if typ != websocket.MessageText {
continue
@@ -0,0 +1,278 @@
package dictation
import (
"context"
"errors"
"net/http"
"net/http/httptest"
"strings"
"sync"
"testing"
"github.com/coder/websocket"
)
// Non-cancellation dial failure: handshake returns 101 with an Upgrade header
// equal to the API key so the dial error embeds the key and redaction must strip it.
func TestDeepgramDialFailureRedactsKey(t *testing.T) {
const key = "sk-test-dial-key-abcdef"
baseURL := dialFailServerWithUpgrade(t, key)
tr, err := NewDeepgramTranscriber(DeepgramConfig{APIKey: key, BaseURL: baseURL})
if err != nil {
t.Fatal(err)
}
chunks := make(chan []byte)
close(chunks)
_, ferr := tr.StreamTranscribe(context.Background(), chunks, nil)
if ferr == nil {
t.Fatal("expected dial failure")
}
if errors.Is(ferr, context.Canceled) {
t.Fatalf("dial failure should not be context.Canceled: %v", ferr)
}
if strings.Contains(ferr.Error(), key) {
t.Errorf("API key leaked from dial failure: %v", ferr)
}
if !strings.Contains(ferr.Error(), "connecting to Deepgram") {
t.Errorf("expected dial-path error prefix, got: %v", ferr)
}
}
// dialFailServerWithUpgrade returns a ws URL whose handshake responds 101 with
// Upgrade set to upgradeValue (not "websocket"), so websocket.Dial fails with
// that value in the error string. Hijacks and closes immediately to avoid the
// library's 3s body-drain timeout on failed dials.
func dialFailServerWithUpgrade(t *testing.T, upgradeValue string) string {
t.Helper()
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
hj, ok := w.(http.Hijacker)
if !ok {
http.Error(w, "hijack not supported", http.StatusInternalServerError)
return
}
conn, bufrw, err := hj.Hijack()
if err != nil {
return
}
defer conn.Close()
_, _ = bufrw.WriteString("HTTP/1.1 101 Switching Protocols\r\n")
_, _ = bufrw.WriteString("Connection: Upgrade\r\n")
_, _ = bufrw.WriteString("Upgrade: " + upgradeValue + "\r\n\r\n")
_ = bufrw.Flush()
}))
t.Cleanup(srv.Close)
return "ws" + strings.TrimPrefix(srv.URL, "http")
}
func TestDeepgramStreamTranscribeErrorRedaction(t *testing.T) {
url := wsTestServer(t, func(ctx context.Context, c *websocket.Conn) {
// Drain the client's audio frames, then wait for CloseStream so the
// client has flushed its writes before we reject the connection. Closing
// immediately (before CloseStream) risks the client failing on a write
// error and never observing this API-key-bearing close reason.
for {
typ, data, err := c.Read(ctx)
if err != nil {
return
}
if typ == websocket.MessageText && strings.Contains(string(data), "CloseStream") {
break
}
}
c.Close(websocket.StatusPolicyViolation, "invalid key sk-test-key")
})
tr, err := NewDeepgramTranscriber(DeepgramConfig{APIKey: "sk-test-key", BaseURL: url})
if err != nil {
t.Fatal(err)
}
chunks := make(chan []byte, 1)
chunks <- make([]byte, 320)
close(chunks)
_, ferr := tr.StreamTranscribe(context.Background(), chunks, func(string, bool) {})
if ferr == nil {
t.Fatal("expected error")
}
if strings.Contains(ferr.Error(), "sk-test-key") {
t.Errorf("API key leaked: %v", ferr)
}
}
func TestDeepgramStreamTranscribeCancelKeepsSentinel(t *testing.T) {
firstFrame := make(chan struct{})
url := wsTestServer(t, func(ctx context.Context, c *websocket.Conn) {
// Hold the connection open, never answering, so the client blocks in
// Read until its context is cancelled (the Esc-abort path).
var once sync.Once
for {
if _, _, err := c.Read(ctx); err != nil {
return
}
once.Do(func() { close(firstFrame) })
}
})
tr, err := NewDeepgramTranscriber(DeepgramConfig{APIKey: "sk-test-key", BaseURL: url})
if err != nil {
t.Fatal(err)
}
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
chunks := make(chan []byte, 1)
defer close(chunks)
chunks <- make([]byte, 320)
// The channel stays open: the session is live when the user aborts.
errCh := make(chan error, 1)
go func() {
_, ferr := tr.StreamTranscribe(ctx, chunks, nil)
errCh <- ferr
}()
select {
case <-firstFrame:
cancel()
ferr := <-errCh
if !errors.Is(ferr, context.Canceled) {
t.Fatalf("cancelled stream error lost the context.Canceled sentinel: %v", ferr)
}
case ferr := <-errCh:
t.Fatalf("StreamTranscribe failed early instead of blocking: %v", ferr)
}
}
// Esc can cancel while the WebSocket dial is still pending. The dial error is
// redacted with %s, so we must return ctx.Err() rather than a flat string.
func TestDeepgramStreamTranscribeDialCancelKeepsSentinel(t *testing.T) {
tr, err := NewDeepgramTranscriber(DeepgramConfig{
APIKey: "sk-test-key",
BaseURL: "ws://127.0.0.1:1", // never reached; ctx is already cancelled
})
if err != nil {
t.Fatal(err)
}
ctx, cancel := context.WithCancel(context.Background())
cancel()
chunks := make(chan []byte)
defer close(chunks)
_, ferr := tr.StreamTranscribe(ctx, chunks, nil)
if !errors.Is(ferr, context.Canceled) {
t.Fatalf("dial-cancel lost the context.Canceled sentinel: %v", ferr)
}
}
// Esc right after accept can race the first write or the first Read; both
// setup/write redaction sites must still yield context.Canceled.
func TestDeepgramStreamTranscribeStartupCancelKeepsSentinel(t *testing.T) {
accepted := make(chan struct{})
url := wsTestServer(t, func(ctx context.Context, c *websocket.Conn) {
close(accepted)
for {
if _, _, err := c.Read(ctx); err != nil {
return
}
}
})
tr, err := NewDeepgramTranscriber(DeepgramConfig{APIKey: "sk-test-key", BaseURL: url})
if err != nil {
t.Fatal(err)
}
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
chunks := make(chan []byte, 1)
defer close(chunks)
// No frames yet: the writer blocks on chunks or the reader blocks on Read.
errCh := make(chan error, 1)
go func() {
_, ferr := tr.StreamTranscribe(ctx, chunks, nil)
errCh <- ferr
}()
select {
case <-accepted:
cancel()
ferr := <-errCh
if !errors.Is(ferr, context.Canceled) {
t.Fatalf("startup-cancel lost the context.Canceled sentinel: %v", ferr)
}
case ferr := <-errCh:
t.Fatalf("StreamTranscribe failed before accept: %v", ferr)
}
}
// TestDeepgramStreamErrorRedactsHeaderShapedKey exercises the same stream-error
// redaction path as TestDeepgramStreamTranscribeErrorRedaction, with a close
// reason that embeds the key next to an X-Api-Key-shaped header label.
func TestDeepgramStreamErrorRedactsHeaderShapedKey(t *testing.T) {
url := wsTestServer(t, func(ctx context.Context, c *websocket.Conn) {
for {
typ, data, err := c.Read(ctx)
if err != nil {
return
}
if typ == websocket.MessageText && strings.Contains(string(data), "CloseStream") {
break
}
}
c.Close(websocket.StatusPolicyViolation, "X-Api-Key: sk-custom-secret-key-1234567890 failed")
})
tr, err := NewDeepgramTranscriber(DeepgramConfig{APIKey: "sk-custom-secret-key-1234567890", BaseURL: url})
if err != nil {
t.Fatal(err)
}
chunks := make(chan []byte, 1)
chunks <- make([]byte, 320)
close(chunks)
_, ferr := tr.StreamTranscribe(context.Background(), chunks, func(string, bool) {})
if ferr == nil {
t.Fatal("expected error")
}
if strings.Contains(ferr.Error(), "sk-custom-secret-key-1234567890") {
t.Errorf("Custom header API key leaked: %v", ferr)
}
}
func TestDeepgramSinglePassRedaction(t *testing.T) {
url := wsTestServer(t, func(ctx context.Context, c *websocket.Conn) {
for {
typ, data, err := c.Read(ctx)
if err != nil {
return
}
if typ == websocket.MessageText && strings.Contains(string(data), "CloseStream") {
break
}
}
c.Close(websocket.StatusPolicyViolation, "error with sk-key1234567890abcdef1234")
})
tr, err := NewDeepgramTranscriber(DeepgramConfig{APIKey: "sk-key1234567890abcdef1234", BaseURL: url})
if err != nil {
t.Fatal(err)
}
chunks := make(chan []byte, 1)
chunks <- make([]byte, 320)
close(chunks)
_, ferr := tr.StreamTranscribe(context.Background(), chunks, func(string, bool) {})
if ferr == nil {
t.Fatal("expected error")
}
got := ferr.Error()
if strings.Contains(got, "sk-key1234567890abcdef1234") {
t.Fatalf("API key leaked into stream error: %q", got)
}
// One key occurrence must produce exactly one redaction marker (no double wrap).
if strings.Count(got, "[REDACTED]") != 1 {
t.Fatalf("redaction markers in %q: want exactly 1 [REDACTED]", got)
}
}
@@ -8,6 +8,7 @@ import (
"net/http"
"strings"
"github.com/Gitlawb/zero/internal/providers/providerio"
"github.com/coder/websocket"
)
@@ -22,6 +23,10 @@ type OpenAIRealtimeConfig struct {
Model string // default "gpt-4o-transcribe"
// BaseURL overrides the wss endpoint (tests point it at a fake server).
BaseURL string
// writeErrInjector, when set, is consulted before each JSON websocket write.
// Used by same-package tests to force controllable write failures that embed
// secrets; production callers leave it nil.
writeErrInjector func() error
}
type openAIRealtimeTranscriber struct {
@@ -59,7 +64,15 @@ func (o *openAIRealtimeTranscriber) StreamTranscribe(ctx context.Context, chunks
},
})
if err != nil {
return "", fmt.Errorf("connecting to OpenAI Realtime: %w", err)
// Preserve the context.Canceled sentinel (it carries no key) so an
// Esc-abort during dial still matches the UI's errors.Is check. Key
// on ctx.Err() itself rather than unwrapping err: a cancellation can
// surface as a plain transport error (e.g. "closed network
// connection") rather than context.Canceled directly.
if ctx.Err() != nil {
return "", ctx.Err()
}
return "", fmt.Errorf("connecting to OpenAI Realtime: %s", providerio.Redact(err.Error(), o.cfg.APIKey))
}
defer conn.CloseNow()
@@ -73,24 +86,53 @@ func (o *openAIRealtimeTranscriber) StreamTranscribe(ctx context.Context, chunks
},
},
}
if err := writeJSON(ctx, conn, sessionUpdate); err != nil {
return "", fmt.Errorf("configuring OpenAI Realtime session: %w", err)
if err := o.writeJSON(ctx, conn, sessionUpdate); err != nil {
// Same cancellation short-circuit as the dial above: an Esc-abort
// while the session update is in flight must stay context.Canceled.
if ctx.Err() != nil {
return "", ctx.Err()
}
return "", fmt.Errorf("configuring OpenAI Realtime session: %s", providerio.Redact(err.Error(), o.cfg.APIKey))
}
writeErrCh := make(chan error, 1)
go func() {
for chunk := range chunks {
appendMsg := map[string]any{
"type": "input_audio_buffer.append",
"audio": base64.StdEncoding.EncodeToString(chunk),
}
if err := writeJSON(ctx, conn, appendMsg); err != nil {
writeErrCh <- err
// Select on ctx.Done() while waiting for the next chunk so a cancelled
// session does not leave this goroutine blocked on an open idle channel.
for {
select {
case <-ctx.Done():
writeErrCh <- ctx.Err()
return
case chunk, ok := <-chunks:
if !ok {
// Commit the buffered audio to force final transcription.
if err := o.writeJSON(ctx, conn, map[string]any{"type": "input_audio_buffer.commit"}); err != nil {
writeErrCh <- err
// Unblock the reader: without a close, conn.Read stays
// parked on a silent peer and the error is never drained
// from writeErrCh. CloseNow aborts instead of waiting on
// the close handshake, which a silent peer never answers.
conn.CloseNow()
return
}
writeErrCh <- nil
return
}
appendMsg := map[string]any{
"type": "input_audio_buffer.append",
"audio": base64.StdEncoding.EncodeToString(chunk),
}
if err := o.writeJSON(ctx, conn, appendMsg); err != nil {
writeErrCh <- err
// Same unblocking close as the commit path: a server that
// goes silent after session setup would otherwise leave the
// reader blocked in conn.Read with the error undrained.
conn.CloseNow()
return
}
}
}
// Commit the buffered audio to force final transcription of the utterance.
writeErrCh <- writeJSON(ctx, conn, map[string]any{"type": "input_audio_buffer.commit"})
}()
// OpenAI deltas are incremental (append), unlike Deepgram/sherpa's cumulative
@@ -113,7 +155,17 @@ func (o *openAIRealtimeTranscriber) StreamTranscribe(ctx context.Context, chunks
}
default:
}
return compose(), fmt.Errorf("OpenAI Realtime stream error: %w", err)
// A user abort cancels the streaming context; the UI matches it
// with errors.Is(err, context.Canceled), so return the sentinel
// itself (it carries no key) instead of a flat redacted string.
// Key on ctx.Err() rather than unwrapping err: the writeErrCh
// swap above can replace err with a plain transport error (e.g.
// "closed network connection") that doesn't itself unwrap to
// context.Canceled even though the cancellation is what caused it.
if ctx.Err() != nil {
return compose(), ctx.Err()
}
return compose(), fmt.Errorf("OpenAI Realtime stream error: %s", providerio.Redact(err.Error(), o.cfg.APIKey))
}
if typ != websocket.MessageText {
continue
@@ -125,6 +177,12 @@ func (o *openAIRealtimeTranscriber) StreamTranscribe(ctx context.Context, chunks
if onPartial != nil {
onPartial(compose(), false)
}
// onPartial may deliver the partial that makes the TUI cancel
// (Esc). Observe that before the next Read, which can already
// have a racing OpenAI error frame buffered.
if ctx.Err() != nil {
return compose(), ctx.Err()
}
case realtimeCompleted:
// A completed item replaces the in-progress delta buffer with the
// server's authoritative transcript for that segment.
@@ -138,6 +196,9 @@ func (o *openAIRealtimeTranscriber) StreamTranscribe(ctx context.Context, chunks
if onPartial != nil {
onPartial(compose(), true)
}
if ctx.Err() != nil {
return compose(), ctx.Err()
}
// Once we've committed the buffer (user stopped) and the server has
// returned a completed transcription, the utterance is done.
if committed {
@@ -147,7 +208,15 @@ func (o *openAIRealtimeTranscriber) StreamTranscribe(ctx context.Context, chunks
case realtimeCommitted:
committed = true
case realtimeError:
return compose(), fmt.Errorf("OpenAI Realtime error: %s", evt.text)
// Esc can race an incoming error event: conn.Read may already have
// the OpenAI error in hand by the time cancellation lands. Key on
// ctx.Err() first, same as every other return point in this loop,
// so a cancel that wins the race still surfaces as context.Canceled
// instead of a spurious redacted failure alongside it.
if ctx.Err() != nil {
return compose(), ctx.Err()
}
return compose(), fmt.Errorf("OpenAI Realtime error: %s", providerio.Redact(evt.text, o.cfg.APIKey))
}
// The writer signals commit completion out of band; observe it so a
// stop with no further audio still flips `committed`.
@@ -155,6 +224,10 @@ func (o *openAIRealtimeTranscriber) StreamTranscribe(ctx context.Context, chunks
case werr := <-writeErrCh:
if werr == nil {
committed = true
} else if ctx.Err() != nil {
return compose(), ctx.Err()
} else {
return compose(), fmt.Errorf("OpenAI Realtime stream error: %s", providerio.Redact(werr.Error(), o.cfg.APIKey))
}
default:
}
@@ -201,6 +274,15 @@ func parseRealtimeEvent(data []byte) realtimeEvent {
return realtimeEvent{kind: realtimeOther}
}
func (o *openAIRealtimeTranscriber) writeJSON(ctx context.Context, conn *websocket.Conn, v any) error {
if o.cfg.writeErrInjector != nil {
if err := o.cfg.writeErrInjector(); err != nil {
return err
}
}
return writeJSON(ctx, conn, v)
}
func writeJSON(ctx context.Context, conn *websocket.Conn, v any) error {
data, err := json.Marshal(v)
if err != nil {
@@ -0,0 +1,437 @@
package dictation
import (
"context"
"errors"
"strings"
"sync"
"sync/atomic"
"testing"
"time"
"github.com/coder/websocket"
)
// Non-cancellation dial failure: handshake returns 101 with an Upgrade header
// equal to the API key so the dial error embeds the key and redaction must strip it.
func TestOpenAIRealtimeDialFailureRedactsKey(t *testing.T) {
const key = "sk-test-dial-key-abcdef"
baseURL := dialFailServerWithUpgrade(t, key)
tr, err := NewOpenAIRealtimeTranscriber(OpenAIRealtimeConfig{APIKey: key, BaseURL: baseURL})
if err != nil {
t.Fatal(err)
}
chunks := make(chan []byte)
close(chunks)
_, ferr := tr.StreamTranscribe(context.Background(), chunks, nil)
if ferr == nil {
t.Fatal("expected dial failure")
}
if errors.Is(ferr, context.Canceled) {
t.Fatalf("dial failure should not be context.Canceled: %v", ferr)
}
if strings.Contains(ferr.Error(), key) {
t.Errorf("API key leaked from dial failure: %v", ferr)
}
if !strings.Contains(ferr.Error(), "connecting to OpenAI Realtime") {
t.Errorf("expected dial-path error prefix, got: %v", ferr)
}
}
// Session-configuration write fails after dial: inject a write error that embeds
// the API key (peer close cannot reach this path reliably — the session write
// always lands in the kernel buffer first) and assert redaction.
func TestOpenAIRealtimeSessionConfigFailureRedactsKey(t *testing.T) {
const key = "sk-test-session-key-abcdef"
url := wsTestServer(t, func(ctx context.Context, c *websocket.Conn) {
// Hold open; the client fails on the injected session write before reading.
<-ctx.Done()
})
tr, err := NewOpenAIRealtimeTranscriber(OpenAIRealtimeConfig{
APIKey: key,
BaseURL: url,
writeErrInjector: func() error {
return errors.New("invalid API key " + key)
},
})
if err != nil {
t.Fatal(err)
}
chunks := make(chan []byte)
close(chunks)
_, ferr := tr.StreamTranscribe(context.Background(), chunks, nil)
if ferr == nil {
t.Fatal("expected session-config failure")
}
if errors.Is(ferr, context.Canceled) {
t.Fatalf("session-config failure should not be context.Canceled: %v", ferr)
}
if strings.Contains(ferr.Error(), key) {
t.Errorf("API key leaked from session-config failure: %v", ferr)
}
if !strings.Contains(ferr.Error(), "configuring OpenAI Realtime session") {
t.Errorf("expected session-config error prefix, got: %v", ferr)
}
}
// Asynchronous writer failure: session update succeeds, then the audio writer
// injects a key-bearing write error. The server keeps the connection open and
// emits a delta so the reader loop observes writeErrCh (not a peer close).
func TestOpenAIRealtimeWriterFailureRedactsKey(t *testing.T) {
const key = "sk-test-writer-key-abcdef"
sessionDone := make(chan struct{})
url := wsTestServer(t, func(ctx context.Context, c *websocket.Conn) {
if _, _, err := c.Read(ctx); err != nil {
return
}
close(sessionDone)
// Keep reading and emit a delta so the client processes an event and
// drains writeErrCh while the connection is still open.
_ = c.Write(ctx, websocket.MessageText, []byte(
`{"type":"conversation.item.input_audio_transcription.delta","delta":"hi"}`,
))
for {
if _, _, err := c.Read(ctx); err != nil {
return
}
}
})
var writes atomic.Int32
tr, err := NewOpenAIRealtimeTranscriber(OpenAIRealtimeConfig{
APIKey: key,
BaseURL: url,
writeErrInjector: func() error {
// First write is session.update; fail subsequent audio/commit writes.
if writes.Add(1) == 1 {
return nil
}
return errors.New("invalid API key " + key)
},
})
if err != nil {
t.Fatal(err)
}
chunks := make(chan []byte, 1)
chunks <- make([]byte, 480)
// Leave open after the first frame so the writer hits the injected error
// on that append rather than a clean commit.
done := make(chan error, 1)
go func() {
_, ferr := tr.StreamTranscribe(context.Background(), chunks, func(string, bool) {})
done <- ferr
}()
var ferr error
select {
case ferr = <-done:
case <-time.After(5 * time.Second):
close(chunks)
t.Fatal("StreamTranscribe did not complete after writer failure")
}
close(chunks)
if ferr == nil {
t.Fatal("expected writer failure")
}
if errors.Is(ferr, context.Canceled) {
t.Fatalf("writer failure should not be context.Canceled: %v", ferr)
}
if strings.Contains(ferr.Error(), key) {
t.Errorf("API key leaked from writer failure: %v", ferr)
}
if !strings.Contains(ferr.Error(), "OpenAI Realtime stream error") {
t.Errorf("expected stream/writer error prefix, got: %v", ferr)
}
select {
case <-sessionDone:
default:
// Session may race the injected audio write; prefix assertion above is enough.
}
}
// Writer failure with a silent server: the peer never sends a frame after
// session setup, so the reader is parked in conn.Read. The writer must close
// the connection on failure or the writeErrCh error is never observed and
// StreamTranscribe blocks forever.
func TestOpenAIRealtimeWriterFailureUnblocksSilentServer(t *testing.T) {
const key = "sk-test-silent-writer-key-abcdef"
url := wsTestServer(t, func(ctx context.Context, c *websocket.Conn) {
// Hold the connection open without ever writing a frame.
<-ctx.Done()
})
var writes atomic.Int32
tr, err := NewOpenAIRealtimeTranscriber(OpenAIRealtimeConfig{
APIKey: key,
BaseURL: url,
writeErrInjector: func() error {
// First write is session.update; fail the subsequent append write.
if writes.Add(1) == 1 {
return nil
}
return errors.New("invalid API key " + key)
},
})
if err != nil {
t.Fatal(err)
}
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
chunks := make(chan []byte, 1)
chunks <- make([]byte, 480)
// Leave the channel open: the writer must fail on the append, not the commit.
done := make(chan error, 1)
go func() {
_, ferr := tr.StreamTranscribe(ctx, chunks, func(string, bool) {})
done <- ferr
}()
select {
case ferr := <-done:
if ferr == nil {
t.Fatal("expected writer failure")
}
if errors.Is(ferr, context.Canceled) {
t.Fatalf("writer failure should not be context.Canceled: %v", ferr)
}
if strings.Contains(ferr.Error(), key) {
t.Errorf("API key leaked from writer failure: %v", ferr)
}
if !strings.Contains(ferr.Error(), "OpenAI Realtime stream error") {
t.Errorf("expected stream/writer error prefix, got: %v", ferr)
}
case <-time.After(5 * time.Second):
close(chunks)
t.Fatal("StreamTranscribe did not unblock after writer failure with a silent server")
}
}
func TestOpenAIRealtimeStreamTranscribeErrorRedaction(t *testing.T) {
url := wsTestServer(t, func(ctx context.Context, c *websocket.Conn) {
defer c.Close(websocket.StatusNormalClosure, "")
for {
typ, _, err := c.Read(ctx)
if err != nil {
return
}
if typ != websocket.MessageText {
continue
}
_ = c.Write(ctx, websocket.MessageText, []byte(`{"type":"error","error":{"message":"invalid API key sk-test-key"}}`))
}
})
tr, err := NewOpenAIRealtimeTranscriber(OpenAIRealtimeConfig{APIKey: "sk-test-key", BaseURL: url})
if err != nil {
t.Fatal(err)
}
chunks := make(chan []byte, 1)
chunks <- make([]byte, 480)
close(chunks)
_, ferr := tr.StreamTranscribe(context.Background(), chunks, func(string, bool) {})
if ferr == nil {
t.Fatal("expected error")
}
if strings.Contains(ferr.Error(), "sk-test-key") {
t.Errorf("API key leaked: %v", ferr)
}
}
func TestOpenAIRealtimeStreamTranscribeCancelKeepsSentinel(t *testing.T) {
firstFrame := make(chan struct{})
url := wsTestServer(t, func(ctx context.Context, c *websocket.Conn) {
// Hold the connection open, never answering, so the client blocks in
// Read until its context is cancelled (the Esc-abort path).
var once sync.Once
for {
if _, _, err := c.Read(ctx); err != nil {
return
}
once.Do(func() { close(firstFrame) })
}
})
tr, err := NewOpenAIRealtimeTranscriber(OpenAIRealtimeConfig{APIKey: "sk-test-key", BaseURL: url})
if err != nil {
t.Fatal(err)
}
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
chunks := make(chan []byte, 1)
defer close(chunks)
chunks <- make([]byte, 480)
// The channel stays open: the session is live when the user aborts.
errCh := make(chan error, 1)
go func() {
_, ferr := tr.StreamTranscribe(ctx, chunks, nil)
errCh <- ferr
}()
select {
case <-firstFrame:
cancel()
ferr := <-errCh
if !errors.Is(ferr, context.Canceled) {
t.Fatalf("cancelled stream error lost the context.Canceled sentinel: %v", ferr)
}
case ferr := <-errCh:
t.Fatalf("StreamTranscribe failed early instead of blocking: %v", ferr)
}
}
// Esc can cancel while the WebSocket dial is still pending. The dial error is
// redacted with %s, so we must return ctx.Err() rather than a flat string.
func TestOpenAIRealtimeStreamTranscribeDialCancelKeepsSentinel(t *testing.T) {
tr, err := NewOpenAIRealtimeTranscriber(OpenAIRealtimeConfig{
APIKey: "sk-test-key",
BaseURL: "ws://127.0.0.1:1", // never reached; ctx is already cancelled
})
if err != nil {
t.Fatal(err)
}
ctx, cancel := context.WithCancel(context.Background())
cancel()
chunks := make(chan []byte)
defer close(chunks)
_, ferr := tr.StreamTranscribe(ctx, chunks, nil)
if !errors.Is(ferr, context.Canceled) {
t.Fatalf("dial-cancel lost the context.Canceled sentinel: %v", ferr)
}
}
// Esc right after accept can hit the session-update write or the first Read;
// both redaction sites must still yield context.Canceled.
func TestOpenAIRealtimeStreamTranscribeStartupCancelKeepsSentinel(t *testing.T) {
accepted := make(chan struct{})
url := wsTestServer(t, func(ctx context.Context, c *websocket.Conn) {
close(accepted)
// Do not read: leave the client in session-update write or first Read.
<-ctx.Done()
})
tr, err := NewOpenAIRealtimeTranscriber(OpenAIRealtimeConfig{APIKey: "sk-test-key", BaseURL: url})
if err != nil {
t.Fatal(err)
}
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
chunks := make(chan []byte, 1)
defer close(chunks)
errCh := make(chan error, 1)
go func() {
_, ferr := tr.StreamTranscribe(ctx, chunks, nil)
errCh <- ferr
}()
select {
case <-accepted:
cancel()
ferr := <-errCh
if !errors.Is(ferr, context.Canceled) {
t.Fatalf("startup-cancel lost the context.Canceled sentinel: %v", ferr)
}
case ferr := <-errCh:
t.Fatalf("StreamTranscribe failed before accept: %v", ferr)
}
}
// Cancel while the writer is blocked on more chunks. Whichever path observes
// the cancellation first (the blocked conn.Read or the writeErrCh drain) must
// return context.Canceled rather than a redacted flat string.
func TestOpenAIRealtimeStreamTranscribeWriteCancelKeepsSentinel(t *testing.T) {
sessionReceived := make(chan struct{})
url := wsTestServer(t, func(ctx context.Context, c *websocket.Conn) {
if _, _, err := c.Read(ctx); err != nil {
return
}
close(sessionReceived)
for {
if _, _, err := c.Read(ctx); err != nil {
return
}
}
})
tr, err := NewOpenAIRealtimeTranscriber(OpenAIRealtimeConfig{APIKey: "sk-test-key", BaseURL: url})
if err != nil {
t.Fatal(err)
}
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
chunks := make(chan []byte, 1)
chunks <- make([]byte, 480)
defer close(chunks)
errCh := make(chan error, 1)
go func() {
_, ferr := tr.StreamTranscribe(ctx, chunks, nil)
errCh <- ferr
}()
select {
case <-sessionReceived:
cancel()
ferr := <-errCh
if !errors.Is(ferr, context.Canceled) {
t.Fatalf("write-path cancel lost the context.Canceled sentinel: %v", ferr)
}
case ferr := <-errCh:
t.Fatalf("StreamTranscribe failed early instead of blocking: %v", ferr)
}
}
// Esc can race an incoming OpenAI error event. conn.Read may already have
// the error frame in hand by the time the cancellation lands. The server
// fires a delta immediately followed by the error, back to back. Cancel from
// inside the delta callback (same path the TUI uses when a partial triggers
// Esc handling) so the cancel is observed before the next event is processed.
// The returned error must still be context.Canceled, not the redacted OpenAI
// error.
func TestOpenAIRealtimeStreamTranscribeErrorRaceKeepsSentinel(t *testing.T) {
url := wsTestServer(t, func(ctx context.Context, c *websocket.Conn) {
if _, _, err := c.Read(ctx); err != nil {
return
}
_ = c.Write(ctx, websocket.MessageText, []byte(
`{"type":"conversation.item.input_audio_transcription.delta","delta":"hi"}`,
))
_ = c.Write(ctx, websocket.MessageText, []byte(
`{"type":"error","error":{"message":"invalid API key sk-test-key"}}`,
))
<-ctx.Done()
})
tr, err := NewOpenAIRealtimeTranscriber(OpenAIRealtimeConfig{APIKey: "sk-test-key", BaseURL: url})
if err != nil {
t.Fatal(err)
}
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
chunks := make(chan []byte, 1)
defer close(chunks)
chunks <- make([]byte, 480)
var once sync.Once
_, ferr := tr.StreamTranscribe(ctx, chunks, func(string, bool) {
// Cancel synchronously from the partial callback, before the
// stream loop continues to the already-buffered error frame.
once.Do(cancel)
})
if !errors.Is(ferr, context.Canceled) {
t.Fatalf("StreamTranscribe error = %v, want context.Canceled (cancel must win over a racing OpenAI error event)", ferr)
}
}
+125 -6
View File
@@ -6,6 +6,8 @@ import (
"errors"
"fmt"
"os/exec"
"strconv"
"strings"
"syscall"
"time"
)
@@ -38,12 +40,45 @@ func KillProcessTree(pid int) error {
// TerminateProcessTree requests graceful termination, then force-kills the
// process tree after grace. Callers retain their distinct persistence models;
// this function owns only the OS lifecycle primitive.
//
// The signal target is REDISCOVERED here via processSignalTarget(pid), which
// asks the OS for pid's current group. A caller that already knows pid was
// configured as its own group leader at launch (e.g. via ConfigureProcessGroup)
// should use TerminateProcessGroup instead — see its doc comment for why this
// rediscovery is fragile once the leader may have already exited.
func TerminateProcessTree(pid int, grace, poll time.Duration) error {
target, err := processSignalTarget(pid)
if err != nil {
return err
}
alive := func() bool { return syscall.Kill(target, syscall.Signal(0)) == nil }
return terminateTarget(pid, target, grace, poll)
}
// TerminateProcessGroup is like TerminateProcessTree, but for a caller that
// already knows pid is (or was, at launch) its own process-group leader — e.g.
// a command started after ConfigureProcessGroup(cmd), which sets Setpgid so
// cmd.Process.Pid always leads its own group at the moment it is signalled.
//
// TerminateProcessTree instead rediscovers the group via syscall.Getpgid(pid)
// at call time. On Darwin, that lookup can return ESRCH once an unreaped group
// leader has already exited, even though live descendants remain in the group
// it configured — TerminateProcessTree would then silently fall back to
// signalling only the (already dead) leader PID and leave those descendants
// running. Skipping rediscovery in favor of the launch-time invariant avoids
// that: the negative PID is used directly, regardless of whether the leader is
// still alive to be looked up.
func TerminateProcessGroup(pid int, grace, poll time.Duration) error {
if pid <= 1 {
return fmt.Errorf("refusing to signal invalid pid %d", pid)
}
return terminateTarget(pid, -pid, grace, poll)
}
// terminateTarget signals target (an individual PID or, negative, a process
// group) with SIGTERM then, after grace, SIGKILL. reportPid identifies the
// original process in error messages regardless of which form target takes.
func terminateTarget(reportPid, target int, grace, poll time.Duration) error {
alive := func() bool { return signalTargetRunning(target) }
if err := syscall.Kill(target, syscall.SIGTERM); err != nil {
if errors.Is(err, syscall.ESRCH) {
return nil
@@ -77,11 +112,91 @@ func TerminateProcessTree(pid int, grace, poll time.Duration) error {
time.Sleep(poll)
}
if alive() {
return fmt.Errorf("process %d did not exit after SIGKILL", pid)
return fmt.Errorf("process %d did not exit after SIGKILL", reportPid)
}
return nil
}
// signalTargetRunning reports whether the signal target still has a process that
// is actually RUNNING, as opposed to one waiting to be reaped.
//
// kill(2) with signal 0 succeeds for a zombie: the PID still exists until its
// parent collects it. That matters because the target here is usually a process
// GROUP, and a terminated leader's child is briefly a zombie before init/launchd
// reaps it. Treating that window as "still alive" made termination sit through
// both grace periods, SIGKILL an already-dead group, and then report
// "did not exit after SIGKILL" for a tree that had in fact stopped — turning a
// successful cleanup into a spurious failure (and a flaky one, since it depends on
// reap timing).
//
// Zombie detection goes through ps, which is available on every Unix we target and
// avoids a /proc dependency Darwin does not have. It is only consulted when the
// cheap kill check says something is still there, so the common path costs nothing
// extra. If ps cannot be run or its output cannot be parsed, the conservative
// pre-existing answer (the kill result) stands.
func signalTargetRunning(target int) bool {
if syscall.Kill(target, syscall.Signal(0)) != nil {
return false
}
// A negative target is a process GROUP: check whether any member is
// non-zombie. A positive target is an individual PID that is NOT its own
// group leader (processSignalTarget's fallback) — its actual PGID differs
// from its own PID, so matching rows by PGID here would look for a group
// that doesn't exist and always report "unknown" (conservatively alive).
// Check its own PID's state instead.
var running, ok bool
if target < 0 {
running, ok = processGroupHasRunningMember(-target)
} else {
running, ok = processIsRunning(target)
}
if !ok {
return true
}
return running
}
// processGroupHasRunningMember reports whether any member of pgid is in a state
// other than zombie. ok is false when the group's states could not be determined.
func processGroupHasRunningMember(pgid int) (running bool, ok bool) {
return processTableStateMatches(func(_, rowPgid int) bool { return rowPgid == pgid })
}
// processIsRunning reports whether pid itself is in a state other than zombie.
// ok is false when pid's state could not be determined.
func processIsRunning(pid int) (running bool, ok bool) {
return processTableStateMatches(func(rowPid, _ int) bool { return rowPid == pid })
}
// processTableStateMatches scans the process table once, via ps, and reports
// whether any row satisfying match is non-zombie, and whether at least one
// matching row was found at all (ok). No rows found means either the process
// (or group) genuinely doesn't exist (a race with exit) or ps's output was
// unparseable/restricted; either way, the caller should not claim knowledge.
func processTableStateMatches(match func(pid, pgid int) bool) (running bool, ok bool) {
output, err := exec.Command("ps", "-A", "-o", "pid=,pgid=,stat=").Output()
if err != nil {
return false, false
}
found := false
for line := range strings.SplitSeq(string(output), "\n") {
fields := strings.Fields(line)
if len(fields) < 3 {
continue
}
rowPid, errPid := strconv.Atoi(fields[0])
rowPgid, errPgid := strconv.Atoi(fields[1])
if errPid != nil || errPgid != nil || !match(rowPid, rowPgid) {
continue
}
found = true
if !strings.HasPrefix(fields[2], "Z") {
return true, true
}
}
return false, found
}
func processSignalTarget(pid int) (int, error) {
if pid <= 1 {
return 0, fmt.Errorf("refusing to signal invalid pid %d", pid)
@@ -91,10 +206,14 @@ func processSignalTarget(pid int) (int, error) {
if pgid == pid {
target = -pid
}
} else if errors.Is(err, syscall.ESRCH) {
// Preserve the individual target; the signal call below treats ESRCH as
// already gone, which is a successful lifecycle outcome.
return pid, nil
} else {
if errors.Is(err, syscall.ESRCH) {
// Preserve the individual target; the signal call below treats ESRCH as
// already gone, which is a successful lifecycle outcome.
return pid, nil
}
// Conservatively retain the individual PID after other lookup failures;
// guessing and signalling a process group could affect unrelated processes.
}
return target, nil
}
+133
View File
@@ -0,0 +1,133 @@
//go:build !windows
package execution
import (
"os/exec"
"strconv"
"syscall"
"testing"
"time"
)
// TestTerminateProcessTreeTreatsZombieGroupAsExited is the regression test for
// counting unreaped processes as alive: kill(2) with signal 0 succeeds for a
// zombie, so a group whose only remaining member is waiting to be reaped used to
// sit through both grace periods, take a pointless SIGKILL, and then be reported
// as "did not exit after SIGKILL" — a spurious cleanup failure whose timing
// depended on when the reaper ran.
func TestTerminateProcessTreeTreatsZombieGroupAsExited(t *testing.T) {
cmd := exec.Command("sh", "-c", "exit 0")
ConfigureProcessGroup(cmd)
if err := cmd.Start(); err != nil {
t.Fatalf("start: %v", err)
}
pid := cmd.Process.Pid
// Deliberately do NOT Wait: the child stays a zombie in its own group, which
// is exactly the state a terminated tree passes through.
t.Cleanup(func() { _ = cmd.Wait() })
deadline := time.Now().Add(5 * time.Second)
for {
if zombie, ok := processIsZombie(pid); ok && zombie {
break
}
if time.Now().After(deadline) {
t.Skip("child did not reach the zombie state; ps output is unavailable in this environment")
}
time.Sleep(10 * time.Millisecond)
}
if syscall.Kill(-pid, syscall.Signal(0)) != nil {
t.Skip("the zombie group is no longer signalable; cannot exercise the case")
}
if signalTargetRunning(-pid) {
t.Fatal("a group holding only a zombie must not count as running")
}
if err := TerminateProcessTree(pid, 50*time.Millisecond, 10*time.Millisecond); err != nil {
t.Fatalf("TerminateProcessTree on an already-exited tree: %v", err)
}
}
// TestTerminateProcessTreeStopsRunningGroup is the positive control: a group with
// a live member must still be seen as running and then actually stopped, so the
// zombie tolerance above cannot degrade into ignoring real processes.
func TestTerminateProcessTreeStopsRunningGroup(t *testing.T) {
cmd := exec.Command("sh", "-c", "sleep 30")
ConfigureProcessGroup(cmd)
if err := cmd.Start(); err != nil {
t.Fatalf("start: %v", err)
}
pid := cmd.Process.Pid
defer func() { _ = cmd.Wait() }()
if !signalTargetRunning(-pid) {
t.Fatal("a group with a live member must count as running")
}
if err := TerminateProcessTree(pid, time.Second, 10*time.Millisecond); err != nil {
t.Fatalf("TerminateProcessTree: %v", err)
}
if running, ok := processGroupHasRunningMember(pid); ok && running {
t.Fatal("the group still has a running member after termination")
}
}
// TestSignalTargetRunningTreatsZombieIndividualPIDAsExited is the regression
// test for jatmn's second #774 finding: signalTargetRunning's zombie check
// matched the individual-PID fallback target (a process that is NOT its own
// group leader — processSignalTarget's positive-PID case) against process-table
// rows by PGID. That target's actual PGID differs from its own PID by
// definition (that's exactly why processSignalTarget fell back to the
// individual PID instead of a negative group target), so no row ever matched,
// "unknown" resulted every time, and signalTargetRunning conservatively (and
// incorrectly) treated a zombie individual-PID target as still running.
func TestSignalTargetRunningTreatsZombieIndividualPIDAsExited(t *testing.T) {
cmd := exec.Command("sh", "-c", "exit 0")
// Deliberately do NOT call ConfigureProcessGroup: the child inherits this
// test process's group, so it is not its own leader and processSignalTarget
// falls back to the individual (positive) PID.
if err := cmd.Start(); err != nil {
t.Fatalf("start: %v", err)
}
pid := cmd.Process.Pid
t.Cleanup(func() { _ = cmd.Wait() })
deadline := time.Now().Add(5 * time.Second)
for {
if zombie, ok := processIsZombie(pid); ok && zombie {
break
}
if time.Now().After(deadline) {
t.Skip("child did not reach the zombie state; ps output is unavailable in this environment")
}
time.Sleep(10 * time.Millisecond)
}
target, err := processSignalTarget(pid)
if err != nil {
t.Fatalf("processSignalTarget: %v", err)
}
if target != pid {
t.Skip("child unexpectedly became its own group leader; cannot exercise the individual-PID fallback")
}
if signalTargetRunning(target) {
t.Fatal("a zombie individual-PID target must not count as running")
}
}
// processIsZombie reports a process's zombie state via ps. ok is false when the
// state could not be read.
func processIsZombie(pid int) (zombie bool, ok bool) {
output, err := exec.Command("ps", "-o", "stat=", "-p", strconv.Itoa(pid)).Output()
if err != nil {
return false, false
}
state := string(output)
for len(state) > 0 && (state[0] == ' ' || state[0] == '\n' || state[0] == '\t') {
state = state[1:]
}
if state == "" {
return false, false
}
return state[0] == 'Z', true
}
+27 -1
View File
@@ -3,6 +3,8 @@
package execution
import (
"errors"
"fmt"
"os"
"os/exec"
"path/filepath"
@@ -12,10 +14,34 @@ import (
func ConfigureProcessGroup(cmd *exec.Cmd) {}
// KillProcessTree asks taskkill /T to kill pid and descendants it can still
// discover from that live root. If taskkill fails, it falls back to killing pid
// alone but still returns the tree error because descendants may have survived.
// Neither path can recover descendants after the root has exited.
func KillProcessTree(pid int) error {
if err := exec.Command(taskkillPath(), "/T", "/F", "/PID", strconv.Itoa(pid)).Run(); err == nil {
return killProcessTree(pid, runTaskkill, killRootProcess)
}
func killProcessTree(pid int, killTree, killRoot func(int) error) error {
treeErr := killTree(pid)
if treeErr == nil {
return nil
}
rootErr := killRoot(pid)
if rootErr != nil {
return errors.Join(
fmt.Errorf("kill process tree rooted at %d: %w", pid, treeErr),
fmt.Errorf("kill root process %d: %w", pid, rootErr),
)
}
return fmt.Errorf("kill process tree rooted at %d (root process killed directly): %w", pid, treeErr)
}
func runTaskkill(pid int) error {
return exec.Command(taskkillPath(), "/T", "/F", "/PID", strconv.Itoa(pid)).Run()
}
func killRootProcess(pid int) error {
process, err := os.FindProcess(pid)
if err != nil {
return err
@@ -0,0 +1,29 @@
//go:build windows
package execution
import (
"errors"
"strings"
"testing"
)
func TestKillProcessTreeRetainsTreeErrorWhenRootFallbackSucceeds(t *testing.T) {
treeErr := errors.New("taskkill failed")
err := killProcessTree(42, func(int) error { return treeErr }, func(int) error { return nil })
if !errors.Is(err, treeErr) {
t.Fatalf("killProcessTree error = %v, want tree error", err)
}
if !strings.Contains(err.Error(), "root process killed directly") {
t.Fatalf("killProcessTree error = %q, want fallback context", err)
}
}
func TestKillProcessTreeJoinsTreeAndRootErrors(t *testing.T) {
treeErr := errors.New("taskkill failed")
rootErr := errors.New("root kill failed")
err := killProcessTree(42, func(int) error { return treeErr }, func(int) error { return rootErr })
if !errors.Is(err, treeErr) || !errors.Is(err, rootErr) {
t.Fatalf("killProcessTree error = %v, want both failures", err)
}
}
+177
View File
@@ -0,0 +1,177 @@
// Package pathjail confines file operations to a directory tree.
//
// It exists because the obvious guard does not work. Checking a path with
// Lstat and then handing the same string to MkdirAll, CreateTemp, Rename or
// Remove is two independent resolutions with a gap between them, and every
// operation after the check re-resolves the ANCESTORS as well. A store that
// verified only its own directory and file was therefore writable outside its
// tree whenever any component above them was a link: the check passed, and the
// syscall that followed went somewhere else.
//
// On Windows the same guard failed for a second, independent reason. A junction
// is a reparse point but NOT a symlink, so os.Lstat reports it as ModeIrregular
// with ModeSymlink clear, and a guard testing only ModeSymlink returns "not a
// link" when pointed straight at one. Junctions also need no privilege to
// create, unlike Windows symlinks, so this is reachable by exactly the
// unprivileged user a workspace guard is meant to contain.
//
// The fix for both is to stop naming things by path. Open a handle on the root
// once and address everything below it relative to that handle, so the kernel
// resolves each component against an object that cannot be swapped underneath.
// os.Root is that handle: it refuses any component that would escape, including
// a Windows reparse point, and it is the same idea the sandbox uses for ACL
// materialization.
package pathjail
import (
"crypto/rand"
"encoding/hex"
"errors"
"fmt"
"io/fs"
"os"
"path/filepath"
"strings"
)
// ErrEscapes reports a target that is not inside the root it was confined to.
// Returned before anything is opened, so a caller cannot act on the path first.
var ErrEscapes = errors.New("path escapes its root")
// ErrReparse reports a link or reparse point where a real file or directory was
// required.
var ErrReparse = errors.New("refusing to write through a link")
// Open confines dir to root and returns a handle on root plus dir expressed
// relative to it. The caller closes the handle.
//
// root is resolved normally, because it is the boundary the caller chose and
// trusts (a workspace the operator opened, say, which may legitimately sit under
// a link). Everything BELOW it is resolved against the returned handle and
// cannot escape.
func Open(root, dir string) (*os.Root, string, error) {
// Only a genuinely EMPTY value is treated as missing; every other spelling is
// used verbatim. Testing TrimSpace(...) == "" instead would retarget the very
// paths this guards: " " is a legal directory name on unix, so trimming would
// reject it as "no root" or silently swap dir for root, which is the same
// retargeting bug as trimming "root " down to "root".
if root == "" {
return nil, "", fmt.Errorf("%w: no root to confine %q to", ErrEscapes, dir)
}
if dir == "" {
dir = root
}
// Rel before Open, so an escaping target is refused without opening
// anything. filepath.Rel is lexical, which is the right test here: it says
// what the caller ASKED for, and the handle enforces what actually happens.
relative, err := filepath.Rel(root, dir)
if err != nil {
return nil, "", fmt.Errorf("%w: %q against %q: %v", ErrEscapes, dir, root, err)
}
if relative == ".." || strings.HasPrefix(relative, ".."+string(filepath.Separator)) {
return nil, "", fmt.Errorf("%w: %q is outside %q", ErrEscapes, dir, root)
}
// The root is created if it is missing, by ordinary pathname, because it is
// the boundary the caller declared rather than anything being confined. The
// stores this replaced called MkdirAll on the whole tree, so this creates
// strictly less by pathname than before, and everything BELOW root is opened
// relative to the handle.
if err := os.MkdirAll(root, 0o700); err != nil {
return nil, "", fmt.Errorf("create %s: %w", root, err)
}
handle, err := os.OpenRoot(root)
if err != nil {
return nil, "", fmt.Errorf("open %s: %w", root, err)
}
return handle, relative, nil
}
// RefuseReparse fails when relative names a link or reparse point.
//
// Absent is fine: a name that does not exist yet is what a create is for.
//
// This is belt to os.Root's braces. The handle already refuses to traverse a
// reparse point, so this exists to give the FINAL component a clear error
// instead of an "escapes from parent" one, and to keep the refusal explicit at
// the point a reader looks for it.
//
// ModeIrregular is tested alongside ModeSymlink deliberately: that is what a
// Windows junction reports, and a guard checking only ModeSymlink lets one
// straight through.
func RefuseReparse(handle *os.Root, relative string) error {
// A trailing separator makes os.Root.Lstat resolve the terminal component on
// Windows (doInRootAlwaysResolveTerminalSlash), so a junction named with a
// trailing slash would be stat'd as its target and wave through as "not a
// link". Strip it so the final component itself is the thing inspected.
//
// os.IsPathSeparator rather than a literal `/\` set, because backslash is a
// separator only on Windows: on unix it is an ordinary filename character, and
// trimming it would inspect "linked" when asked about a symlink genuinely
// named `linked\` — reporting "not a link" for one that is.
if trimmed := trimTrailingSeparators(relative); trimmed != "" {
relative = trimmed
}
info, err := handle.Lstat(relative)
if err != nil {
if errors.Is(err, fs.ErrNotExist) {
return nil
}
return fmt.Errorf("inspect %s: %w", relative, err)
}
if info.Mode()&(os.ModeSymlink|os.ModeIrregular) != 0 {
return fmt.Errorf("%w: %s", ErrReparse, relative)
}
return nil
}
// trimTrailingSeparators drops the path separators at the end of a name, using
// the ACTIVE platform's definition of one: `/` and `\` on Windows, `/` only on
// unix, where a backslash is a legal character in a filename.
func trimTrailingSeparators(path string) string {
end := len(path)
for end > 0 && os.IsPathSeparator(path[end-1]) {
end--
}
return path[:end]
}
// randomBytes fills b with cryptographic randomness. A variable rather than a
// direct crypto/rand call so a test can force the name collision that exercises
// the O_EXCL retry below — the branch that matters, and one a real random source
// will not reach on demand.
var randomBytes = rand.Read
// CreateTemp makes an O_EXCL temp file named prefix.<random>suffix inside dir,
// relative to handle, and returns it with its relative path.
//
// O_EXCL rather than a fixed name: a link planted at a predictable temp path
// between the check and the create is the same race this package exists to
// close, and O_EXCL makes the kernel refuse it rather than us.
func CreateTemp(handle *os.Root, dir, prefix, suffix string) (*os.File, string, error) {
// prefix and suffix are NAME fragments, not path fragments. A separator or a
// "."/".." in either would relocate the file out of dir — a prefix of
// "../escaped", or a suffix of ".tmp/../evil" that also erases the random
// component and with it the O_EXCL unpredictability the file relies on.
// Reject them up front so the file lands where dir promises.
for _, fragment := range []string{prefix, suffix} {
if strings.ContainsAny(fragment, `/\`) || fragment == "." || fragment == ".." {
return nil, "", fmt.Errorf("%w: temporary name fragment %q must be a single path component", ErrEscapes, fragment)
}
}
for attempt := 0; attempt < 10; attempt++ {
noise := make([]byte, 8)
if _, err := randomBytes(noise); err != nil {
return nil, "", fmt.Errorf("generate a temporary name: %w", err)
}
relative := filepath.Join(dir, prefix+"."+hex.EncodeToString(noise)+suffix)
file, err := handle.OpenFile(relative, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0o600)
if err == nil {
return file, relative, nil
}
if errors.Is(err, fs.ErrExist) {
continue
}
return nil, "", err
}
return nil, "", errors.New("could not create a temporary file with an unused name")
}
+360
View File
@@ -0,0 +1,360 @@
package pathjail
import (
"encoding/hex"
"errors"
"os"
"os/exec"
"path/filepath"
"runtime"
"testing"
)
// linkDir points link at target, using whatever kind of link this platform lets
// an unprivileged process create.
//
// On Windows that is a JUNCTION, not a symlink, and the difference is the whole
// reason this package exists twice over. A symlink needs
// SeCreateSymbolicLinkPrivilege, which an ordinary account does not hold, so a
// test written with os.Symlink silently skips on Windows and proves nothing
// there. A junction needs no privilege at all, so it is both the reachable
// attack and the only one that can be tested.
func linkDir(t *testing.T, target, link string) {
t.Helper()
if runtime.GOOS == "windows" {
if out, err := exec.Command("cmd", "/c", "mklink", "/J", link, target).CombinedOutput(); err != nil {
t.Skipf("cannot create a junction: %v %s", err, out)
}
return
}
if err := os.Symlink(target, link); err != nil {
t.Skipf("cannot create a symlink: %v", err)
}
}
// The reported defect: a link in an ANCESTOR of the target, not at the target
// itself. Every guard that checked only the final directory and file passed
// this, because the components it checked were genuinely not links; the ones
// the syscall then traversed were.
func TestAnAncestorLinkCannotRedirectAWrite(t *testing.T) {
base := t.TempDir()
outside := filepath.Join(base, "outside")
workspace := filepath.Join(base, "workspace")
for _, dir := range []string{outside, workspace} {
if err := os.MkdirAll(dir, 0o700); err != nil {
t.Fatal(err)
}
}
linkDir(t, outside, filepath.Join(workspace, ".zero"))
handle, relative, err := Open(workspace, filepath.Join(workspace, ".zero", "store"))
if err != nil {
t.Fatalf("Open should confine, not refuse outright: %v", err)
}
defer handle.Close()
if err := handle.MkdirAll(relative, 0o700); err == nil {
t.Fatal("created a directory through a linked ancestor, so a write can still leave the workspace")
}
if _, err := os.Stat(filepath.Join(outside, "store")); !os.IsNotExist(err) {
t.Errorf("something was created outside the workspace, stat error = %v", err)
}
}
// RefuseReparse has to catch a junction, which is the case os.ModeSymlink alone
// misses. Asserted through the exported helper rather than by reading the mode,
// so the test fails if the predicate is ever narrowed back.
func TestRefuseReparseCatchesAJunctionNotJustASymlink(t *testing.T) {
base := t.TempDir()
target := filepath.Join(base, "target")
inside := filepath.Join(base, "inside")
for _, dir := range []string{target, inside} {
if err := os.MkdirAll(dir, 0o700); err != nil {
t.Fatal(err)
}
}
linkDir(t, target, filepath.Join(inside, "linked"))
handle, relative, err := Open(inside, inside)
if err != nil {
t.Fatal(err)
}
defer handle.Close()
err = RefuseReparse(handle, filepath.Join(relative, "linked"))
if !errors.Is(err, ErrReparse) {
t.Fatalf("RefuseReparse(link) = %v, want ErrReparse; on Windows a junction reports ModeIrregular, so a ModeSymlink-only check returns nil here", err)
}
}
// A name that does not exist yet is what a create is for, so it must not be
// refused. Without this the guard above would be satisfied by refusing
// everything.
func TestRefuseReparseAllowsAnAbsentName(t *testing.T) {
base := t.TempDir()
handle, relative, err := Open(base, base)
if err != nil {
t.Fatal(err)
}
defer handle.Close()
if err := RefuseReparse(handle, filepath.Join(relative, "not-there.md")); err != nil {
t.Errorf("an absent name was refused: %v", err)
}
if err := RefuseReparse(handle, relative); err != nil {
t.Errorf("an ordinary directory was refused: %v", err)
}
}
// Open refuses a target outside its root before opening anything, so a caller
// cannot act on the path first.
func TestOpenRefusesATargetOutsideTheRoot(t *testing.T) {
base := t.TempDir()
inside := filepath.Join(base, "inside")
if err := os.MkdirAll(inside, 0o700); err != nil {
t.Fatal(err)
}
for _, target := range []string{
filepath.Join(base, "sibling"),
filepath.Join(inside, "..", "..", "escape"),
} {
if _, _, err := Open(inside, target); !errors.Is(err, ErrEscapes) {
t.Errorf("Open(%q, %q) = %v, want ErrEscapes", inside, target, err)
}
}
if _, _, err := Open("", filepath.Join(base, "anything")); !errors.Is(err, ErrEscapes) {
t.Error("an empty root was accepted; a boundary is not optional")
}
}
// CreateTemp must use O_EXCL and an unpredictable name, so a link planted at a
// guessable temp path is refused by the kernel rather than followed.
func TestCreateTempIsExclusiveAndUnpredictable(t *testing.T) {
base := t.TempDir()
handle, relative, err := Open(base, base)
if err != nil {
t.Fatal(err)
}
defer handle.Close()
seen := map[string]bool{}
for i := 0; i < 8; i++ {
file, name, err := CreateTemp(handle, relative, "note", ".tmp")
if err != nil {
t.Fatal(err)
}
file.Close()
if seen[name] {
t.Fatalf("CreateTemp reused the name %q, so two concurrent writes would stomp each other", name)
}
seen[name] = true
if filepath.Ext(name) != ".tmp" {
t.Errorf("temp name %q lost its suffix, so a listing may pick it up as real content", name)
}
}
}
// A legitimate directory whose name carries a space must be confined as spelled.
// Trimming it would open a different filesystem object.
//
// A LEADING space, deliberately: Win32 normalizes a component's trailing spaces
// and dots away, so "root " and "root" are one object there and a trailing-space
// case cannot express the bug on Windows. A leading space survives on every
// platform, so this runs everywhere rather than being gated to unix.
// The path is RELATIVE, under t.Chdir, so the leading space sits at the start of
// the whole string. An absolute filepath.Join(base, " root") would put the space
// in the middle, where TrimSpace cannot reach it — the test would then pass
// against the very guard it exists to catch, on every platform.
func TestOpenPreservesSpaceInPaths(t *testing.T) {
base := t.TempDir()
t.Chdir(base)
const spaced = " root" // the leading space is part of the name
handle, relative, err := Open(spaced, spaced)
if err != nil {
t.Fatal(err)
}
defer handle.Close()
// Write through the handle, then confirm the bytes landed under the spelling
// the caller asked for — the property that actually matters, and one
// handle.Name() alone would not prove.
file, _, err := CreateTemp(handle, relative, "probe", ".tmp")
if err != nil {
t.Fatal(err)
}
name := file.Name()
if err := file.Close(); err != nil {
t.Fatal(err)
}
if _, err := os.Stat(filepath.Join(base, spaced, filepath.Base(name))); err != nil {
t.Errorf("a file created through the handle is not under %q: %v — the leading space was trimmed away", spaced, err)
}
if _, err := os.Stat(filepath.Join(base, "root")); err == nil {
t.Error(`a trimmed "root" was created instead of the requested " root"`)
}
}
// A whitespace-only name is a legal directory on unix, so it must be confined as
// given rather than treated as "no root" or silently swapped for the root.
func TestOpenAcceptsAWhitespaceOnlyComponent(t *testing.T) {
if runtime.GOOS == "windows" {
t.Skip("Win32 normalizes a whitespace-only component away; the name is not expressible there")
}
// The path must be whitespace-only IN FULL for this to bite, so it has to be
// relative: an absolute path ending in " " still has a non-empty TrimSpace and
// would sail past the old guard, making the test prove nothing.
t.Chdir(t.TempDir())
handle, _, err := Open(" ", " ")
if err != nil {
t.Fatalf("a whitespace-only path was rejected as missing: %v", err)
}
defer handle.Close()
if _, err := os.Stat(" "); err != nil {
t.Errorf("the whitespace-only directory was not created as spelled: %v", err)
}
}
// prefix and suffix are name fragments. A separator or traversal in either must
// be refused, or the file lands outside dir and (for a suffix erasing the random
// component) loses the O_EXCL unpredictability it depends on.
func TestCreateTempRejectsPathFragmentsThatEscapeTheDir(t *testing.T) {
base := t.TempDir()
inside := filepath.Join(base, "inside")
if err := os.MkdirAll(inside, 0o700); err != nil {
t.Fatal(err)
}
// root is base, dir is the subdirectory: a "../escaped" fragment lands in
// base — still inside the os.Root, so os.Root does NOT catch it — but outside
// the dir CreateTemp promised. This is the case the reviewers reproduced.
handle, relative, err := Open(base, inside)
if err != nil {
t.Fatal(err)
}
defer handle.Close()
cases := []struct{ prefix, suffix string }{
{"../escaped", ".tmp"},
{`..\sibling`, ".tmp"},
{"note", ".tmp/../evil"},
{"note", ".."},
{"..", ".tmp"},
}
for _, c := range cases {
if _, _, err := CreateTemp(handle, relative, c.prefix, c.suffix); !errors.Is(err, ErrEscapes) {
t.Errorf("CreateTemp(prefix=%q, suffix=%q) = %v, want ErrEscapes", c.prefix, c.suffix, err)
}
}
// Nothing may have leaked into base above the promised dir.
entries, _ := os.ReadDir(base)
for _, entry := range entries {
if entry.Name() != "inside" {
t.Errorf("a temp fragment escaped into the parent: %q", entry.Name())
}
}
}
// A junction named with a trailing separator must still be refused. On Windows
// os.Root.Lstat resolves the terminal component when the name ends in a
// separator, so without trimming it the link is stat'd as its target and waved
// through.
func TestRefuseReparseCatchesALinkNamedWithATrailingSeparator(t *testing.T) {
base := t.TempDir()
target := filepath.Join(base, "target")
inside := filepath.Join(base, "inside")
for _, dir := range []string{target, inside} {
if err := os.MkdirAll(dir, 0o700); err != nil {
t.Fatal(err)
}
}
linkDir(t, target, filepath.Join(inside, "linked"))
handle, relative, err := Open(inside, inside)
if err != nil {
t.Fatal(err)
}
defer handle.Close()
name := filepath.Join(relative, "linked") + string(filepath.Separator)
if err := RefuseReparse(handle, name); !errors.Is(err, ErrReparse) {
t.Fatalf("RefuseReparse(%q) = %v, want ErrReparse", name, err)
}
}
// The O_EXCL retry is the branch that protects an existing file, and a real
// random source will not collide on demand — so force it. The first two draws
// return the same bytes: the name is pre-created, CreateTemp must refuse to
// reuse it, leave its contents alone, and come back with a different name.
func TestCreateTempRetriesWithoutClobberingAnExistingName(t *testing.T) {
base := t.TempDir()
handle, relative, err := Open(base, base)
if err != nil {
t.Fatal(err)
}
defer handle.Close()
draws := [][]byte{
{1, 1, 1, 1, 1, 1, 1, 1}, // collides with the pre-created file
{1, 1, 1, 1, 1, 1, 1, 1}, // and again, so the retry must keep trying
{2, 2, 2, 2, 2, 2, 2, 2}, // finally a free name
}
previous := randomBytes
t.Cleanup(func() { randomBytes = previous })
call := 0
randomBytes = func(b []byte) (int, error) {
copy(b, draws[min(call, len(draws)-1)])
call++
return len(b), nil
}
taken := filepath.Join(base, "note."+hex.EncodeToString(draws[0])+".tmp")
if err := os.WriteFile(taken, []byte("do not clobber me"), 0o600); err != nil {
t.Fatal(err)
}
file, name, err := CreateTemp(handle, relative, "note", ".tmp")
if err != nil {
t.Fatal(err)
}
defer file.Close()
if filepath.Base(name) == filepath.Base(taken) {
t.Fatalf("CreateTemp returned the name that was already taken (%q), so O_EXCL did not protect it", name)
}
kept, err := os.ReadFile(taken)
if err != nil {
t.Fatal(err)
}
if string(kept) != "do not clobber me" {
t.Errorf("the pre-existing file was overwritten: %q", kept)
}
if call < 2 {
t.Errorf("the random source was drawn %d time(s); the collision path never ran", call)
}
}
// On unix a backslash is an ordinary filename character, not a separator, so a
// link genuinely named `linked\` must still be refused. Trimming backslashes
// unconditionally would inspect "linked" instead and report "not a link" for one
// that is.
func TestRefuseReparseKeepsABackslashInAUnixName(t *testing.T) {
if runtime.GOOS == "windows" {
t.Skip(`backslash is a separator on Windows, so this name is not expressible there`)
}
base := t.TempDir()
target := filepath.Join(base, "target")
inside := filepath.Join(base, "inside")
for _, dir := range []string{target, inside} {
if err := os.MkdirAll(dir, 0o700); err != nil {
t.Fatal(err)
}
}
linkDir(t, target, filepath.Join(inside, `linked\`))
handle, relative, err := Open(inside, inside)
if err != nil {
t.Fatal(err)
}
defer handle.Close()
name := filepath.Join(relative, `linked\`)
if err := RefuseReparse(handle, name); !errors.Is(err, ErrReparse) {
t.Fatalf("RefuseReparse(%q) = %v, want ErrReparse — the backslash is part of the name here, not a separator", name, err)
}
}
+4 -2
View File
@@ -78,14 +78,16 @@ func maybeFormatWrittenFile(ctx context.Context, absolutePath string, writtenCon
if !ok {
return writtenContent
}
if _, err := exec.LookPath(command[0]); err != nil {
binaryPath, err := exec.LookPath(command[0])
if err != nil {
return writtenContent
}
formatCtx, cancel := context.WithTimeout(ctx, formatOnWriteTimeout)
defer cancel()
arguments := append(append([]string(nil), command[1:]...), absolutePath)
formatter := exec.CommandContext(formatCtx, command[0], arguments...)
formatter := exec.CommandContext(formatCtx, binaryPath, arguments...)
formatter.Dir = filepath.Dir(absolutePath)
formatter.Stdin = strings.NewReader("")
if err := formatter.Run(); err != nil {
return writtenContent
}
+14
View File
@@ -102,3 +102,17 @@ func TestFormatOnWriteSkipsUnknownExtensions(t *testing.T) {
t.Fatalf("unknown extension must pass through: %q", content)
}
}
func TestFormatOnWriteFormatterLookupFailure(t *testing.T) {
t.Setenv("ZERO_FORMAT_ON_WRITE", "1")
t.Setenv("PATH", t.TempDir())
targetPath := filepath.Join(t.TempDir(), "a.go")
uglyContent := "package a\n\nfunc A( ) { }\n"
if err := os.WriteFile(targetPath, []byte(uglyContent), 0o644); err != nil {
t.Fatal(err)
}
content := maybeFormatWrittenFile(context.Background(), targetPath, uglyContent)
if content != uglyContent {
t.Fatalf("missing formatter must return written content, got %q", content)
}
}
+56
View File
@@ -121,3 +121,59 @@ func TestToolResultRowExposesClickToggle(t *testing.T) {
t.Fatalf("tool result visible body/footer must stay selectable, got %#v", selectable)
}
}
func TestAlwaysExpandedToolResultHeaderIsNotToggleable(t *testing.T) {
m := transcriptViewTestModel()
for _, tool := range []string{"edit_file", "apply_patch", "write_file"} {
row := transcriptRow{
kind: rowToolResult,
id: "diff",
tool: tool,
status: tools.StatusOK,
detail: "--- a/example.go\n+++ b/example.go\n@@ -1 +1 @@\n-old\n+new",
}
_, selectable := m.renderSelectableToolResultRow(0, row, m.width, buildRowContext(nil), 0)
if len(selectable) == 0 {
t.Fatalf("%s: expected a visible diff result row", tool)
}
if selectable[0].toggle {
t.Fatalf("%s header must not advertise a collapse toggle: %#v", tool, selectable[0])
}
}
}
func TestToolResultOutputCannotCreateToggle(t *testing.T) {
m := transcriptViewTestModel()
row := transcriptRow{
kind: rowToolResult,
id: "custom",
tool: "custom_tool",
status: tools.StatusOK,
detail: "the file literally says: click to expand",
}
_, selectable := m.renderSelectableToolResultRow(0, row, m.width, buildRowContext(nil), 0)
if len(selectable) == 0 {
t.Fatal("expected a visible tool result row")
}
if selectable[0].toggle {
t.Fatalf("tool-output text must not create a header toggle: %#v", selectable[0])
}
}
func TestCollapsedExploreToolResultHeaderIsToggleable(t *testing.T) {
m := transcriptViewTestModel()
row := transcriptRow{
kind: rowToolResult,
id: "read",
tool: "read_file",
status: tools.StatusOK,
detail: numberedLines(cardBodyMaxLines + 5),
}
_, selectable := m.renderSelectableToolResultRow(0, row, m.width, buildRowContext(nil), 0)
if len(selectable) == 0 {
t.Fatal("expected a visible explore result row")
}
if !selectable[0].toggle {
t.Fatalf("collapsed explore card header must remain toggleable: %#v", selectable[0])
}
}
+34 -9
View File
@@ -61,6 +61,7 @@ type dictationController struct {
browseVariants []dictation.ModelVariant
// in-flight session state
sessionID int64
streaming bool
recorder Recorder
transcriber Transcriber
@@ -101,13 +102,17 @@ type dictationController struct {
}
// dictationStartedMsg reports the outcome of Recorder.Start().
// sessionID must match the dictation controller's current session so a late
// start result after cancel/reset cannot reset a newer recording.
type dictationStartedMsg struct {
err error
sessionID int64
err error
}
// dictationTranscribedMsg carries the final transcript of a batch (or a
// completed streaming) recording.
type dictationTranscribedMsg struct {
sessionID int64
text string
err error
submit bool
@@ -125,6 +130,7 @@ func newDictationController(opts Options) dictationController {
platform: dictation.DetectPlatform(),
downloadRoot: opts.STTDownloadRoot,
userConfigPath: opts.UserConfigPath,
sessionID: 1,
}
}
@@ -262,7 +268,7 @@ func (m model) startDictation() (model, tea.Cmd) {
if streaming {
return m.startStreamingDictation()
}
return m, startBatchRecordingCmd(rec)
return m, startBatchRecordingCmd(m.dictation.sessionID, rec)
}
// stopDictation ends a recording. For batch it triggers Stop()+Transcribe(); for
@@ -276,7 +282,7 @@ func (m model) stopDictation() (model, tea.Cmd) {
}
return m, nil // final text arrives via the streaming command already running
}
return m, transcribeBatchCmd(m.dictation.ctx, m.dictation.recorder, m.dictation.transcriber, m.dictation.cfg.AutoSubmitEnabled())
return m, transcribeBatchCmd(m.dictation.sessionID, m.dictation.ctx, m.dictation.recorder, m.dictation.transcriber, m.dictation.cfg.AutoSubmitEnabled())
}
// cancelDictation aborts an in-flight recording without transcribing. Bound to
@@ -311,6 +317,7 @@ func (d dictationController) maxDuration() time.Duration {
}
func (d *dictationController) reset() {
d.sessionID++
d.phase = dictIdle
d.recorder = nil
d.transcriber = nil
@@ -326,31 +333,36 @@ func (d *dictationController) reset() {
// startBatchRecordingCmd starts the record-to-file capture off the UI goroutine
// (Start spawns a subprocess and may briefly block or fail on a missing tool).
func startBatchRecordingCmd(rec Recorder) tea.Cmd {
func startBatchRecordingCmd(sessionID int64, rec Recorder) tea.Cmd {
return func() tea.Msg {
return dictationStartedMsg{err: rec.Start()}
return dictationStartedMsg{sessionID: sessionID, err: rec.Start()}
}
}
// transcribeBatchCmd stops the recording, transcribes the audio, and reports the
// final text. Runs off the UI goroutine — Stop waits on the capture tool and
// Transcribe does a network round-trip or a local exec.
func transcribeBatchCmd(ctx context.Context, rec Recorder, transcriber Transcriber, submit bool) tea.Cmd {
func transcribeBatchCmd(sessionID int64, ctx context.Context, rec Recorder, transcriber Transcriber, submit bool) tea.Cmd {
if ctx == nil {
ctx = context.Background()
}
return func() tea.Msg {
audio, err := rec.Stop()
if err != nil {
return dictationTranscribedMsg{err: err}
return dictationTranscribedMsg{sessionID: sessionID, err: err}
}
text, err := transcriber.Transcribe(ctx, audio)
return dictationTranscribedMsg{text: text, err: err, submit: submit}
return dictationTranscribedMsg{sessionID: sessionID, text: text, err: err, submit: submit}
}
}
// handleDictationStarted transitions to recording (or reports a start failure).
func (m model) handleDictationStarted(msg dictationStartedMsg) (model, tea.Cmd) {
if msg.sessionID != m.dictation.sessionID {
// Stale startup from a cancelled session must not reset or re-arm a
// newer recording that already owns the controller.
return m, nil
}
if msg.err != nil {
m = m.discardDictationRegion()
m.dictation.reset()
@@ -375,6 +387,9 @@ func (m model) handleDictationStarted(msg dictationStartedMsg) (model, tea.Cmd)
// handleDictationTranscribed inserts the final transcript into the composer (or
// submits it when stt.autoSubmit is on), then returns to idle.
func (m model) handleDictationTranscribed(msg dictationTranscribedMsg) (tea.Model, tea.Cmd) {
if msg.sessionID != m.dictation.sessionID {
return m, nil
}
streaming := m.dictation.streaming || msg.streaming
m = m.commitDictationRegion()
// A streaming session can end via a transcriber error (not just a user stop), so
@@ -392,7 +407,17 @@ func (m model) handleDictationTranscribed(msg dictationTranscribedMsg) (tea.Mode
}
m.dictation.reset()
if msg.err != nil && !errors.Is(msg.err, context.Canceled) {
if errors.Is(msg.err, context.Canceled) {
// cancelDictation already discarded the live region, reset the
// session, and posted "Dictation cancelled." A streaming transcriber
// can still race a buffered event past that cancel and report back a
// nonempty compose()+context.Canceled; treat that as terminal here,
// before the auto-submit branch below, so Esc can never fall through
// to msg.submit and fire the composer's restored pre-existing text.
return m, nil
}
if msg.err != nil {
// A cloud auth failure (missing/invalid key) is fixable in place: reopen the
// API-key prompt for the current provider so the user can paste a key and
// retry, instead of hitting a dead-end "run zero auth" line.
+9 -4
View File
@@ -14,8 +14,9 @@ import (
// mechanism agent text deltas use (§6). text is the cumulative best transcript
// so far (not a delta); final marks a settled segment.
type sttPartialMsg struct {
text string
final bool
sessionID int64
text string
final bool
}
// startStreamingDictation begins continuous capture and launches the streaming
@@ -38,6 +39,7 @@ func (m model) startStreamingDictation() (model, tea.Cmd) {
}
transcriber := m.dictation.transcriber
submit := m.dictation.cfg.AutoSubmitEnabled()
sessionID := m.dictation.sessionID
// Tap the audio: compute a mic level per chunk for the live waveform, then
// forward the chunk to the transcriber. A small buffer keeps the tap from
@@ -63,11 +65,11 @@ func (m model) startStreamingDictation() (model, tea.Cmd) {
streamCmd := func() tea.Msg {
onPartial := func(text string, final bool) {
if sink != nil {
sink(sttPartialMsg{text: text, final: final})
sink(sttPartialMsg{sessionID: sessionID, text: text, final: final})
}
}
text, err := transcriber.StreamTranscribe(ctx, tapped, onPartial)
return dictationTranscribedMsg{text: text, err: err, submit: submit, streaming: true}
return dictationTranscribedMsg{sessionID: sessionID, text: text, err: err, submit: submit, streaming: true}
}
// Streaming drives the waveform from real levels (no synthetic tick needed).
return m, streamCmd
@@ -77,6 +79,9 @@ func (m model) startStreamingDictation() (model, tea.Cmd) {
// composer, replacing the previously-rendered live region wholesale so the text
// builds up in place as the user keeps talking.
func (m model) handleDictationPartial(msg sttPartialMsg) model {
if msg.sessionID != m.dictation.sessionID {
return m
}
// Ignore stragglers that arrive after the session ended (cancel/final).
if m.dictation.phase != dictRecording && m.dictation.phase != dictTranscribing {
return m
+181 -2
View File
@@ -86,9 +86,10 @@ func TestDictationNonAuthErrorDoesNotPrompt(t *testing.T) {
func TestDictationStartedFailureResets(t *testing.T) {
m := model{}
m.dictation.sessionID = 1
m.dictation.phase = dictStarting
m.dictation.streaming = false
got, _ := m.handleDictationStarted(dictationStartedMsg{err: errors.New("mic busy")})
got, _ := m.handleDictationStarted(dictationStartedMsg{sessionID: 1, err: errors.New("mic busy")})
if got.dictation.active() {
t.Error("a start failure should reset to idle")
}
@@ -99,13 +100,48 @@ func TestDictationStartedFailureResets(t *testing.T) {
func TestDictationStartedArmsRecording(t *testing.T) {
m := model{}
m.dictation.sessionID = 1
m.dictation.phase = dictStarting
got, _ := m.handleDictationStarted(dictationStartedMsg{})
got, _ := m.handleDictationStarted(dictationStartedMsg{sessionID: 1})
if got.dictation.phase != dictRecording {
t.Error("a successful start should advance to recording")
}
}
// TestStaleDictationStartedIgnoredWhileLive pins the session gate on batch
// startup messages. A late Start() result after cancel must not reset() a
// newer recording (success would leave dictStarting with no recorder; failure
// would kill the live session).
func TestStaleDictationStartedIgnoredWhileLive(t *testing.T) {
// Active recording on session 2.
m := model{}
m.dictation.sessionID = 2
m.dictation.phase = dictRecording
m.dictation.streaming = true
m.setComposerState(composerState{text: "live session text", cursor: len("live session text")})
// Stale failure from session 1 must not reset the live controller.
afterFail, _ := m.handleDictationStarted(dictationStartedMsg{
sessionID: 1,
err: errors.New("mic busy from previous start"),
})
if afterFail.dictation.phase != dictRecording || afterFail.dictation.sessionID != 2 {
t.Fatalf("stale start failure: phase=%v session=%d, want recording/2", afterFail.dictation.phase, afterFail.dictation.sessionID)
}
if transcriptHasText(afterFail, "mic busy from previous start") {
t.Fatal("stale start failure must not post an error notice")
}
// Stale success from session 1 must not leave the live session stuck mid-start.
m2 := model{}
m2.dictation.sessionID = 2
m2.dictation.phase = dictRecording
afterOK, _ := m2.handleDictationStarted(dictationStartedMsg{sessionID: 1})
if afterOK.dictation.phase != dictRecording || afterOK.dictation.sessionID != 2 {
t.Fatalf("stale start success: phase=%v session=%d, want recording/2", afterOK.dictation.phase, afterOK.dictation.sessionID)
}
}
func TestDictationUnavailableShowsHint(t *testing.T) {
m := model{} // no build factory
next, _ := m.toggleDictation()
@@ -136,6 +172,69 @@ func TestDictationStreamingPartialReplacesRegion(t *testing.T) {
}
}
func TestDictationCanceledStreamRaceDoesNotAutoSubmit(t *testing.T) {
// Esc can race an already-buffered realtime event: cancelDictation discards
// the live region and resets state synchronously, but the streaming
// goroutine's dictationTranscribedMsg (a nonempty compose() alongside
// context.Canceled) can still arrive afterward. With stt.autoSubmit on,
// that must never fall through to msg.submit and fire the composer's
// restored pre-existing text.
// Matching sessionID is required so this hits the context.Canceled branch
// rather than only the stale-session gate (session 0 after cancel would be
// dropped before that branch runs).
m := model{}
m.dictation.sessionID = 1
m.setComposerState(composerState{text: "existing prompt", cursor: len("existing prompt")})
m.dictation.phase = dictRecording
m.dictation.streaming = true
m = m.handleDictationPartial(sttPartialMsg{sessionID: 1, text: "half-formed transcript"})
if m.composer.text != "existing prompt half-formed transcript" {
t.Fatalf("partial did not render into composer: %q", m.composer.text)
}
m, _ = m.cancelDictation()
if m.composer.text != "existing prompt" {
t.Fatalf("cancel should restore the pre-existing composer text, got %q", m.composer.text)
}
// The already-buffered event's message arrives after the cancel above.
next, cmd := m.handleDictationTranscribed(dictationTranscribedMsg{
sessionID: m.dictation.sessionID,
text: "half-formed transcript",
err: context.Canceled,
submit: true,
streaming: true,
})
got := next.(model)
if got.composer.text != "existing prompt" {
t.Errorf("a raced cancellation must not auto-submit the restored composer text, got %q", got.composer.text)
}
if cmd != nil {
t.Error("a raced cancellation must not return a submit command")
}
}
// TestStaleDictationPartialIgnoredWhileLive pins the session-ID gate on
// handleDictationPartial while a new session is still recording. Cancel-then-
// stale cases are already covered by the phase guard; this case needs a live
// phase so only the session check drops the ghost partial.
func TestStaleDictationPartialIgnoredWhileLive(t *testing.T) {
m := model{}
m.dictation.sessionID = 2
m.dictation.phase = dictRecording
m.dictation.streaming = true
m.setComposerState(composerState{text: "user typed this", cursor: len("user typed this")})
got := m.handleDictationPartial(sttPartialMsg{
sessionID: 1,
text: "ghost text from session 1",
})
if got.composer.text != "user typed this" {
t.Fatalf("stale partial while live: got %q, want %q", got.composer.text, "user typed this")
}
}
func TestDictationCommitKeepsStreamedText(t *testing.T) {
m := model{}
m.setComposerState(composerState{text: "", cursor: 0})
@@ -302,3 +401,83 @@ func TestCurrentModelLabel(t *testing.T) {
t.Errorf("no-model label = %q", got)
}
}
func TestStaleDictationCompletionIgnoredAfterCancel(t *testing.T) {
m := model{}
m.setComposerState(composerState{text: "original composer text", cursor: 22})
m.dictation.sessionID = 1
m.dictation.phase = dictRecording
m, _ = m.cancelDictation()
if m.composer.text != "original composer text" {
t.Fatalf("composer text = %q, want 'original composer text'", m.composer.text)
}
// Stale partial message from session 1 must be ignored
stalePartial := sttPartialMsg{
sessionID: 1,
text: "stale partial that should be ignored",
}
mPartial := m.handleDictationPartial(stalePartial)
if mPartial.composer.text != "original composer text" {
t.Fatalf("composer text changed to %q after stale partial", mPartial.composer.text)
}
staleMsg := dictationTranscribedMsg{
sessionID: 1,
text: "stale text that should be ignored",
submit: true,
streaming: true,
}
afterStale, cmd := m.handleDictationTranscribed(staleMsg)
got := afterStale.(model)
if got.composer.text != "original composer text" {
t.Fatalf("composer text changed to %q after stale completion", got.composer.text)
}
if cmd != nil {
t.Fatalf("expected no command for stale completion, got %v", cmd)
}
}
func TestStaleDictationSessionZeroIgnoredAfterCancel(t *testing.T) {
m := model{}
m.setComposerState(composerState{text: "initial composer text", cursor: 21})
m.dictation.sessionID = 0
m.dictation.phase = dictRecording
m, _ = m.cancelDictation()
// Session 0 was reset to session 1
if m.dictation.sessionID != 1 {
t.Fatalf("dictation.sessionID = %d after cancel, want 1", m.dictation.sessionID)
}
// Stale partial from session 0 must be ignored
stalePartial := sttPartialMsg{
sessionID: 0,
text: "stale partial from session 0",
}
mPartial := m.handleDictationPartial(stalePartial)
if mPartial.composer.text != "initial composer text" {
t.Fatalf("composer text changed to %q after stale session 0 partial", mPartial.composer.text)
}
// Stale completion from session 0 must be ignored
staleMsg := dictationTranscribedMsg{
sessionID: 0,
text: "stale completion from session 0",
submit: true,
streaming: true,
}
afterStale, cmd := m.handleDictationTranscribed(staleMsg)
got := afterStale.(model)
if got.composer.text != "initial composer text" {
t.Fatalf("composer text changed to %q after stale session 0 completion", got.composer.text)
}
if cmd != nil {
t.Fatalf("expected no command for stale session 0 completion, got %v", cmd)
}
}
+64
View File
@@ -0,0 +1,64 @@
package tui
import "strings"
// Visible unchanged lines kept at each edge of a collapsed diff section.
const diffViewerContextLines = 3
// diffViewerLine is a source diff line or a synthetic collapsed-context row.
// rawIndex keeps rendering anchored to the original diff so line-pair and
// syntax-highlighting logic remain correct after context compaction.
type diffViewerLine struct {
text string
rawIndex int
hiddenContext int
}
// compactDiffViewerContext returns the display sequence for a unified diff.
// Long contiguous unchanged runs keep their leading and trailing context while
// one synthetic line accounts for the elided middle. Structural and changed
// rows always remain intact.
func compactDiffViewerContext(raw []string) []diffViewerLine {
lines := make([]diffViewerLine, 0, len(raw))
hunk := diffHunkState{}
for index := 0; index < len(raw); {
if parsed, ok := parseDiffHunkHeader(raw[index]); ok {
lines = append(lines, diffViewerLine{text: raw[index], rawIndex: index})
hunk = parsed
index++
continue
}
if !hunk.active() || !strings.HasPrefix(raw[index], " ") {
lines = append(lines, diffViewerLine{text: raw[index], rawIndex: index})
if hunk.active() && isDiffHunkBodyLine(raw[index]) {
hunk.consume(raw[index])
}
index++
continue
}
start := index
for index < len(raw) && hunk.active() && strings.HasPrefix(raw[index], " ") {
hunk.consume(raw[index])
index++
}
count := index - start
// Collapsing seven lines would replace one readable line with one marker,
// leaving the display equally tall. Only collapse when it removes space.
if count <= diffViewerContextLines*2+1 {
for i := start; i < index; i++ {
lines = append(lines, diffViewerLine{text: raw[i], rawIndex: i})
}
continue
}
for i := start; i < start+diffViewerContextLines; i++ {
lines = append(lines, diffViewerLine{text: raw[i], rawIndex: i})
}
lines = append(lines, diffViewerLine{rawIndex: -1, hiddenContext: count - diffViewerContextLines*2})
for i := index - diffViewerContextLines; i < index; i++ {
lines = append(lines, diffViewerLine{text: raw[i], rawIndex: i})
}
}
return lines
}
+402
View File
@@ -0,0 +1,402 @@
package tui
import (
"fmt"
"strings"
"testing"
"charm.land/lipgloss/v2"
"github.com/charmbracelet/colorprofile"
)
func requireDiffViewerTrueColor(t *testing.T) {
t.Helper()
oldProfile := lipgloss.Writer.Profile
lipgloss.Writer.Profile = colorprofile.TrueColor
t.Cleanup(func() {
lipgloss.Writer.Profile = oldProfile
})
}
func TestDiffViewerUsesHunkHeaderForLineNumbers(t *testing.T) {
diff := strings.Join([]string{
"--- a/internal/tui/view.go",
"+++ b/internal/tui/view.go",
"@@ -40,3 +40,4 @@ func renderView() {",
" context",
"-old line",
"+new line",
}, "\n")
body := diffCardBody(diff, 90, cardRenderOptions{bodyCap: 0})
got := plainRender(t, strings.Join(body.lines, "\n"))
if strings.Contains(got, "@@") || !strings.Contains(got, " 41 − old line") || !strings.Contains(got, " 41 + new line") {
t.Fatalf("diff viewer did not apply the hunk range correctly:\n%s", got)
}
}
func TestDiffViewerCollapsesLongUnchangedContext(t *testing.T) {
lines := []string{
"--- a/x.go",
"+++ b/x.go",
"@@ -1,12 +1,12 @@",
}
for i := 1; i <= 12; i++ {
lines = append(lines, " context "+string(rune('a'+i-1)))
}
diff := strings.Join(lines, "\n")
body := diffCardBody(diff, 80, cardRenderOptions{bodyCap: 0})
got := plainRender(t, strings.Join(body.lines, "\n"))
if !strings.Contains(got, "… 6 unchanged lines") {
t.Fatalf("diff viewer did not collapse long unchanged context:\n%s", got)
}
for _, want := range []string{"context a", "context b", "context c", "context j", "context k", "context l"} {
if !strings.Contains(got, want) {
t.Errorf("diff viewer should preserve contextual edge %q:\n%s", want, got)
}
}
}
func TestDiffViewerKeepsSevenUnchangedContextLines(t *testing.T) {
lines := []string{
"--- a/x.go",
"+++ b/x.go",
"@@ -1,7 +1,7 @@",
}
for i := 1; i <= diffViewerContextLines*2+1; i++ {
lines = append(lines, " context "+string(rune('a'+i-1)))
}
body := diffCardBody(strings.Join(lines, "\n"), 80, cardRenderOptions{bodyCap: 0})
got := plainRender(t, strings.Join(body.lines, "\n"))
if strings.Contains(got, "unchanged lines") {
t.Fatalf("seven unchanged context lines should remain visible:\n%s", got)
}
for _, want := range []string{"context a", "context d", "context g"} {
if !strings.Contains(got, want) {
t.Errorf("diff viewer omitted %q:\n%s", want, got)
}
}
}
func TestDiffViewerKeepsLineNumbersAfterCollapsedContext(t *testing.T) {
lines := []string{
"--- a/x.go",
"+++ b/x.go",
"@@ -1,13 +1,13 @@",
}
for i := 1; i <= 12; i++ {
lines = append(lines, " context "+string(rune('a'+i-1)))
}
lines = append(lines, "-old value", "+new value")
body := diffCardBody(strings.Join(lines, "\n"), 80, cardRenderOptions{bodyCap: 0})
got := plainRender(t, strings.Join(body.lines, "\n"))
if !strings.Contains(got, " 13 − old value") || !strings.Contains(got, " 13 + new value") {
t.Fatalf("line numbers drifted after collapsed context:\n%s", got)
}
}
func TestDiffViewerSyntheticContextHasNoSourceIndex(t *testing.T) {
raw := make([]string, 0, diffViewerContextLines*2+2)
for index := 0; index < diffViewerContextLines*2+2; index++ {
raw = append(raw, " context")
}
for _, line := range compactDiffViewerContext(raw) {
if line.hiddenContext > 0 && line.rawIndex != -1 {
t.Fatalf("synthetic collapsed context rawIndex = %d, want -1", line.rawIndex)
}
}
}
func TestDiffViewerNearRewriteDeletionSurvives(t *testing.T) {
diff := strings.Join([]string{
"--- a/notes.log",
"+++ b/notes.log",
"@@ -1 +1 @@",
"-alpha bravo charlie delta",
"+zulu yankee xray whiskey",
}, "\n")
body := diffCardBody(diff, 100, cardRenderOptions{bodyCap: 0})
got := plainRender(t, strings.Join(body.lines, "\n"))
if !strings.Contains(got, "alpha bravo charlie delta") {
t.Fatalf("near-rewrite deletion disappeared:\n%s", got)
}
}
func TestDiffViewerRendersStyledDeletionOnce(t *testing.T) {
requireDiffViewerTrueColor(t)
diff := strings.Join([]string{
"--- a/example.go",
"+++ b/example.go",
"@@ -1 +1 @@",
"-func value() string { return \"old\" }",
"+func value() string { return \"new\" }",
}, "\n")
body := diffCardBody(diff, 100, cardRenderOptions{bodyCap: 0})
got := plainRender(t, strings.Join(body.lines, "\n"))
if count := strings.Count(got, "func value() string { return \"old\" }"); count != 1 {
t.Fatalf("styled deleted row count = %d, want 1:\n%s", count, got)
}
}
func TestDiffViewerDoesNotFoldPreambleContext(t *testing.T) {
raw := []string{
"commit deadbeef",
"Author: Example",
"",
}
for index := 1; index <= 10; index++ {
raw = append(raw, fmt.Sprintf(" commit message line %d", index))
}
raw = append(raw,
"--- a/example.go",
"+++ b/example.go",
"@@ -1,1 +1,1 @@",
"-old value",
"+new value",
)
compacted := compactDiffViewerContext(raw)
for _, line := range compacted {
if line.hiddenContext > 0 {
t.Fatalf("preamble context was folded: %#v", compacted)
}
}
if len(compacted) != len(raw) {
t.Fatalf("compacted preamble length = %d, want %d", len(compacted), len(raw))
}
}
func TestDiffViewerKeepsHeaderLikeHunkContent(t *testing.T) {
diff := strings.Join([]string{
"--- a/example.go",
"+++ b/example.go",
"@@ -1 +1 @@",
"--- removed",
"+++ added",
}, "\n")
body := diffCardBody(diff, 100, cardRenderOptions{bodyCap: 0})
got := plainRender(t, strings.Join(body.lines, "\n"))
for _, want := range []string{"-- removed", "++ added"} {
if !strings.Contains(got, want) {
t.Fatalf("diff viewer lost header-like hunk content %q:\n%s", want, got)
}
}
meta := diffCardMetadata(diff)
if meta.adds != 1 || meta.dels != 1 {
t.Fatalf("diff metadata counts = +%d -%d, want +1 -1", meta.adds, meta.dels)
}
if _, ok := highlightedDiffLines(strings.Split(diff, "\n"), meta)[3]; !ok {
t.Fatal("deleted header-like hunk content was not syntax highlighted")
}
}
func TestDiffViewerSourcePathStripsOnlyOneDiffPrefix(t *testing.T) {
if got, want := diffViewerSourcePath("--- a/b/c.go"), "b/c.go"; got != want {
t.Fatalf("diffViewerSourcePath = %q, want %q", got, want)
}
}
func TestDiffViewerSyntaxHighlightsBothChangedSides(t *testing.T) {
requireDiffViewerTrueColor(t)
diff := strings.Join([]string{
"--- a/example.go",
"+++ b/example.go",
"@@ -1 +1 @@",
"-func greet(name string) string { return \"old\" }",
"+func greet(name string) string { return \"new\" }",
}, "\n")
body := diffCardBody(diff, 100, cardRenderOptions{bodyCap: 0})
joined := strings.Join(body.lines, "\n")
// `func` uses Zero's accent and string literals use its green token style.
if !strings.Contains(joined, "202;255;63") || !strings.Contains(joined, "93;209;164") {
t.Fatalf("diff viewer did not retain Go syntax colors in changed rows:\n%s", joined)
}
// The exact changed string literal keeps the brighter word-diff background.
if !strings.Contains(joined, "46;101;77") || !strings.Contains(joined, "80;45;48") {
t.Fatalf("diff viewer lost word-level changed-span emphasis:\n%s", joined)
}
}
func TestDiffViewerSyntaxHighlightsUnchangedHunkContext(t *testing.T) {
requireDiffViewerTrueColor(t)
diff := strings.Join([]string{
"--- a/example.go",
"+++ b/example.go",
"@@ -1,3 +1,3 @@",
" func greet() string {",
"-\treturn \"old\"",
"+\treturn \"new\"",
" }",
}, "\n")
body := diffCardBody(diff, 100, cardRenderOptions{bodyCap: 0})
for _, line := range body.lines {
if strings.Contains(plainRender(t, line), "func greet() string {") {
if !strings.Contains(line, "202;255;63") {
t.Fatalf("unchanged source context was not syntax-highlighted: %q", line)
}
return
}
}
t.Fatal("diff viewer omitted unchanged source context")
}
func TestDiffViewerPreservesSyntaxStateAcrossHunkLines(t *testing.T) {
requireDiffViewerTrueColor(t)
diff := strings.Join([]string{
"--- a/example.py",
"+++ b/example.py",
"@@ -1,3 +1,4 @@",
" def message():",
"- return \"\"\"old\"\"\"",
"+ return \"\"\"new",
"+continued\"\"\"",
}, "\n")
body := diffCardBody(diff, 100, cardRenderOptions{bodyCap: 0})
for _, line := range body.lines {
if strings.Contains(plainRender(t, line), "continued") {
if !strings.Contains(line, "93;209;164") {
t.Fatalf("multiline string lost syntax state across hunk rows: %q", line)
}
return
}
}
t.Fatal("diff viewer omitted multiline source row")
}
func TestDiffViewerKeepsNonHunkDiffFallback(t *testing.T) {
diff := strings.Join([]string{
"--- a/example.go",
"+++ b/example.go",
"+func greet() string { return \"new\" }",
}, "\n")
body := diffCardBody(diff, 100, cardRenderOptions{bodyCap: 0})
got := plainRender(t, strings.Join(body.lines, "\n"))
if !strings.Contains(got, "func greet() string { return \"new\" }") {
t.Fatalf("diff viewer lost content without a hunk header:\n%s", got)
}
}
func TestDiffViewerKeepsRenameMetadataWithoutHunks(t *testing.T) {
diff := strings.Join([]string{
"diff --git a/old-name.go b/new-name.go",
"similarity index 100%",
"rename from old-name.go",
"rename to new-name.go",
}, "\n")
body := diffCardBody(diff, 100, cardRenderOptions{bodyCap: 0})
got := plainRender(t, strings.Join(body.lines, "\n"))
if !strings.Contains(got, "rename from old-name.go") || !strings.Contains(got, "rename to new-name.go") {
t.Fatalf("rename-only diff lost its meaningful metadata:\n%s", got)
}
}
func TestDiffViewerKeepsModeMetadataWithoutHunks(t *testing.T) {
diff := strings.Join([]string{
"diff --git a/tool.sh b/tool.sh",
"old mode 100644",
"new mode 100755",
}, "\n")
body := diffCardBody(diff, 100, cardRenderOptions{bodyCap: 0})
got := plainRender(t, strings.Join(body.lines, "\n"))
if !strings.Contains(got, "old mode 100644") || !strings.Contains(got, "new mode 100755") {
t.Fatalf("mode-only diff lost its meaningful metadata:\n%s", got)
}
}
func TestDiffViewerFallsBackForOversizedSourceLine(t *testing.T) {
requireDiffViewerTrueColor(t)
longLine := strings.Repeat("x", diffViewerHighlightMaxLineBytes)
diff := strings.Join([]string{
"--- a/example.go",
"+++ b/example.go",
"@@ -1 +1 @@",
"+" + longLine,
}, "\n")
body := diffCardBody(diff, 100, cardRenderOptions{bodyCap: 0})
joined := strings.Join(body.lines, "\n")
if strings.Contains(joined, "202;255;63") {
t.Fatalf("oversized diff line should use the plain diff fallback: %q", joined)
}
if !strings.Contains(plainRender(t, joined), strings.Repeat("x", 10)) {
t.Fatalf("plain fallback lost oversized source line: %q", joined)
}
}
func TestDiffViewerSyntaxHighlighterKeepsSpans(t *testing.T) {
requireDiffViewerTrueColor(t)
styled, ok := highlightCodeForPathWithSpans(
[]string{"func greet(name string) string { return \"new\" }"},
"example.go",
200,
zeroTheme.addLine.GetBackground(),
[]highlightSpan{{line: 0, start: 39, end: 44, background: zeroTheme.addLineWord.GetBackground()}},
)
if !ok || len(styled) != 1 {
t.Fatalf("highlightCodeForPathWithSpans = %#v, %v", styled, ok)
}
if !strings.Contains(styled[0], "46;101;77") {
t.Fatalf("highlighted span lost its word-diff background: %q", styled[0])
}
}
func TestDiffViewerHighlightsEachFileWithItsOwnPath(t *testing.T) {
requireDiffViewerTrueColor(t)
rawLines := []string{
"diff --git a/example.go b/example.go",
"--- a/example.go",
"+++ b/example.go",
"@@ -1 +1 @@",
"-func old() {}",
"+func new() {}",
"diff --git a/example.py b/example.py",
"--- a/example.py",
"+++ b/example.py",
"@@ -1 +1 @@",
"-def old(): pass",
"+def new(): pass",
}
highlighted := highlightedDiffLines(rawLines, diffCardMetadata(strings.Join(rawLines, "\n")))
if len(highlighted) != 4 {
t.Fatalf("highlighted line count = %d, want 4: %#v", len(highlighted), highlighted)
}
for _, index := range []int{4, 5, 10, 11} {
if !strings.Contains(highlighted[index], "202;255;63") {
t.Fatalf("source row %d lost syntax highlighting: %q", index, highlighted[index])
}
}
for _, header := range []int{2, 3, 8, 9} {
if _, ok := highlighted[header]; ok {
t.Fatalf("file header at row %d entered syntax highlighter: %q", header, highlighted[header])
}
}
}
func TestDiffViewerHighlightBudgetSpansHunks(t *testing.T) {
hunkLines := diffViewerHighlightMaxLines/2 + 1
rawLines := []string{
"--- a/example.go",
"+++ b/example.go",
fmt.Sprintf("@@ -1,%d +1,%d @@", hunkLines, hunkLines),
}
for i := 0; i < hunkLines; i++ {
rawLines = append(rawLines, " context")
}
rawLines = append(rawLines, fmt.Sprintf("@@ -6000,%d +6000,%d @@", hunkLines, hunkLines))
for i := 0; i < hunkLines; i++ {
rawLines = append(rawLines, " context")
}
if highlighted := highlightedDiffLines(rawLines, diffCardMetadata(strings.Join(rawLines, "\n"))); highlighted != nil {
t.Fatalf("diff exceeding the aggregate highlight budget should use the plain fallback: %d highlighted rows", len(highlighted))
}
}
+2 -1
View File
@@ -244,7 +244,8 @@ func (m model) renderSelectableSpecialistRow(rowIndex int, row transcriptRow, wi
}
func (m model) renderSelectableToolResultRow(rowIndex int, row transcriptRow, width int, rc rowContext, startBodyY int) (string, []transcriptSelectableLine) {
return m.renderSelectableToolResultRowFn(rowIndex, row, width, rc, startBodyY, m.renderRow)
opts := cardRenderOptions{bodyCap: cardBodyMaxLines, cwd: m.cwd}
return m.renderSelectableToolResultRowFn(rowIndex, row, width, rc, startBodyY, m.renderRow, opts)
}
func (m model) transcriptBody(width int, emptyOverlay string) (string, []transcriptSelectableLine) {
+339 -70
View File
@@ -2,6 +2,7 @@ package tui
import (
"fmt"
"image/color"
"net/url"
"os"
"path/filepath"
@@ -1439,12 +1440,14 @@ func renderAskUserQuestionnaire(prompt pendingAskUserPrompt, input string, width
const cardBodyMaxLines = 16
// cardBody is what a result-shape renderer hands back: body lines, an
// optional footer embedded in the bottom border, and optional extra head
// metadata (e.g. a read's line range).
// optional footer embedded in the bottom border, optional extra head metadata
// (e.g. a read's line range), and whether the rendered body exposes an
// expand/collapse interaction.
type cardBody struct {
lines []string
footer string
headTag string
lines []string
footer string
headTag string
canToggle bool
}
// renderRunningToolCard draws the head-only card for a tool call that has no
@@ -1994,9 +1997,75 @@ func genericCardBody(detail string, opts cardRenderOptions) cardBody {
return cardBody{lines: capCardLines(lines, opts.bodyCap)}
}
// hunkHeaderPattern extracts the old/new start lines from a unified-diff hunk
// header so the gutter can show real line numbers.
var hunkHeaderPattern = regexp.MustCompile(`^@@ -(\d+)(?:,\d+)? \+(\d+)(?:,\d+)? @@`)
// hunkHeaderPattern extracts the old/new ranges from a unified-diff hunk
// header so the gutter can show real line numbers and distinguish hunk content
// that happens to resemble a file header.
var hunkHeaderPattern = regexp.MustCompile(`^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@`)
type diffHunkState struct {
oldLine int
newLine int
oldRemaining int
newRemaining int
}
func parseDiffHunkHeader(line string) (diffHunkState, bool) {
match := hunkHeaderPattern.FindStringSubmatch(line)
if match == nil {
return diffHunkState{}, false
}
oldLine, oldErr := strconv.Atoi(match[1])
newLine, newErr := strconv.Atoi(match[3])
if oldErr != nil || newErr != nil {
return diffHunkState{}, false
}
oldRemaining, newRemaining := 1, 1
if match[2] != "" {
oldRemaining, oldErr = strconv.Atoi(match[2])
}
if match[4] != "" {
newRemaining, newErr = strconv.Atoi(match[4])
}
if oldErr != nil || newErr != nil || oldRemaining < 0 || newRemaining < 0 {
return diffHunkState{}, false
}
return diffHunkState{oldLine: oldLine, newLine: newLine, oldRemaining: oldRemaining, newRemaining: newRemaining}, true
}
func (state diffHunkState) active() bool {
return state.oldRemaining > 0 || state.newRemaining > 0
}
func (state *diffHunkState) consume(line string) {
if len(line) == 0 {
// Some producers omit the leading space on a blank context line. Treat
// that non-standard but harmless form exactly like normal context so the
// parser stays aligned with the hunk ranges.
state.oldRemaining--
state.newRemaining--
return
}
switch line[0] {
case ' ':
state.oldRemaining--
state.newRemaining--
case '-':
state.oldRemaining--
case '+':
state.newRemaining--
}
}
func (state *diffHunkState) consumeContext(count int) {
state.oldRemaining -= count
state.newRemaining -= count
state.oldLine += count
state.newLine += count
}
func isDiffFileHeader(line string) bool {
return strings.HasPrefix(line, "--- ") || strings.HasPrefix(line, "+++ ")
}
type diffMetadata struct {
path string
@@ -2007,18 +2076,31 @@ type diffMetadata struct {
func diffCardMetadata(detail string) diffMetadata {
meta := diffMetadata{}
hunk := diffHunkState{}
for _, line := range strings.Split(detail, "\n") {
if parsed, ok := parseDiffHunkHeader(line); ok {
hunk = parsed
continue
}
if hunk.active() && isDiffHunkBodyLine(line) {
if len(line) > 0 {
switch line[0] {
case '+':
meta.adds++
case '-':
meta.dels++
}
}
hunk.consume(line)
continue
}
switch {
case strings.HasPrefix(line, "+++ "):
meta.path = strings.TrimPrefix(strings.TrimSpace(strings.TrimPrefix(line, "+++ ")), "b/")
meta.path = diffViewerSourcePath(line)
case strings.HasPrefix(line, "--- "):
if strings.TrimSpace(strings.TrimPrefix(line, "--- ")) == "/dev/null" {
meta.newFile = true
}
case strings.HasPrefix(line, "+"):
meta.adds++
case strings.HasPrefix(line, "-"):
meta.dels++
}
}
return meta
@@ -2039,12 +2121,9 @@ func diffCountTag(adds int, dels int) string {
zeroTheme.faint.Render(")")
}
func (meta diffMetadata) addOnly() bool {
return meta.adds > 0 && meta.dels == 0
}
func diffCardBody(detail string, width int, opts cardRenderOptions) cardBody {
rawLines := strings.Split(detail, "\n")
displayLines := compactDiffViewerContext(rawLines)
meta := diffCardMetadata(detail)
innerWidth := width
lines := []string{}
@@ -2057,88 +2136,263 @@ func diffCardBody(detail string, width int, opts cardRenderOptions) cardBody {
gutterWidth = 4
}
textBudget := maxInt(8, innerWidth-3-gutterWidth)
highlightedAdds := highlightedAddedDiffLines(rawLines, meta, textBudget)
highlightAddIndex := 0
oldLine, newLine := 0, 0
inHunk := false
for i := 0; i < len(rawLines); i++ {
line := rawLines[i]
highlightedLines := highlightedDiffLines(rawLines, meta)
hunk := diffHunkState{}
for i := 0; i < len(displayLines); i++ {
displayLine := displayLines[i]
line := displayLine.text
switch {
case strings.HasPrefix(line, "+++ "), strings.HasPrefix(line, "--- "):
case displayLine.hiddenContext > 0:
lines = append(lines, zeroTheme.diffMeta.Render(fmt.Sprintf("… %d unchanged lines", displayLine.hiddenContext)))
hunk.consumeContext(displayLine.hiddenContext)
case isDiffFileHeader(line) && !hunk.active():
// Path and counts live in the tool head row.
case strings.HasPrefix(line, "diff --git ") && !hunk.active():
// A subsequent file section is metadata, not context from the preceding
// hunk. Reset so its headers remain hidden until its next hunk starts.
hunk = diffHunkState{}
case strings.HasPrefix(line, "@@"):
if match := hunkHeaderPattern.FindStringSubmatch(line); match != nil {
oldLine, _ = strconv.Atoi(match[1])
newLine, _ = strconv.Atoi(match[2])
inHunk = true
if parsed, ok := parseDiffHunkHeader(line); ok {
hunk = parsed
}
// The raw hunk header is implementation metadata. Use it for line
// numbers, but keep the visible diff focused on file content.
continue
case !inHunk, strings.HasPrefix(line, `\`):
// Preamble ("diff --git", "index …", a stray "stdout:") and the
// "\ No newline at end of file" marker are not content lines: no
// gutter number, and the hunk counters must not advance.
// The line ranges drive the gutters below but do not compete with source
// content in a compact tool card.
case !hunk.active():
// The card head already carries the target path and change counts. Hide
// Git transport metadata so the viewer opens on the first useful hunk.
// Preserve unusual pre-hunk output as muted context rather than losing
// diagnostics from nonstandard diff-producing commands.
if !isDiffViewerPreamble(line) {
lines = append(lines, zeroTheme.diffMeta.Render(truncateRunes(line, innerWidth)))
}
case strings.HasPrefix(line, `\`):
// "\ No newline at end of file" has no source-line position.
lines = append(lines, zeroTheme.diffMeta.Render(truncateRunes(line, innerWidth)))
case strings.HasPrefix(line, "+"):
text := truncateRunes(strings.TrimPrefix(line, "+"), textBudget)
if len(highlightedAdds) == meta.adds && highlightAddIndex < len(highlightedAdds) {
lines = append(lines, diffBodyStyledLine(newLine, "+", highlightedAdds[highlightAddIndex], true, textBudget, gutter))
highlightAddIndex++
if styled, ok := highlightedLines[displayLine.rawIndex]; ok {
lines = append(lines, diffBodyStyledLine(hunk.newLine, "+", styled, true, textBudget, gutter))
} else {
lines = append(lines, diffBodyLine(newLine, "+", text, true, textBudget, gutter))
text := truncateRunes(strings.TrimPrefix(line, "+"), textBudget)
lines = append(lines, diffBodyLine(hunk.newLine, "+", text, true, textBudget, gutter))
}
newLine++
hunk.newLine++
hunk.consume(line)
case strings.HasPrefix(line, "-"):
// Isolated 1:1 replacement (one "-" immediately followed by one "+"):
// highlight only the changed span on each side so a one-token edit reads
// instantly. Block changes and near-rewrites fall back to whole-line tint.
if isIsolatedReplacement(rawLines, i) {
// instantly. When a lexer is available the syntax-highlighter carries
// that same word span; unknown paths retain the established plain-text
// word-diff fallback.
if styled, ok := highlightedLines[displayLine.rawIndex]; ok {
lines = append(lines, diffBodyStyledLine(hunk.oldLine, "−", styled, false, textBudget, gutter))
hunk.oldLine++
hunk.consume(line)
continue
} else if isIsolatedReplacement(rawLines, displayLine.rawIndex) {
delText := truncateRunes(strings.TrimPrefix(line, "-"), textBudget)
addText := truncateRunes(strings.TrimPrefix(rawLines[i+1], "+"), textBudget)
if delRow, addRow, ok := renderWordDiffPair(oldLine, newLine, delText, addText, textBudget, gutter); ok {
addText := truncateRunes(strings.TrimPrefix(rawLines[displayLine.rawIndex+1], "+"), textBudget)
if delRow, addRow, ok := renderWordDiffPair(hunk.oldLine, hunk.newLine, delText, addText, textBudget, gutter); ok {
lines = append(lines, delRow, addRow)
oldLine++
newLine++
hunk.oldLine++
hunk.newLine++
hunk.consume(line)
hunk.consume(rawLines[displayLine.rawIndex+1])
i++ // consume the paired "+"
continue
}
}
text := truncateRunes(strings.TrimPrefix(line, "-"), textBudget)
lines = append(lines, diffBodyLine(oldLine, "−", text, false, textBudget, gutter))
oldLine++
lines = append(lines, diffBodyLine(hunk.oldLine, "−", text, false, textBudget, gutter))
hunk.oldLine++
hunk.consume(line)
default:
text := truncateRunes(strings.TrimPrefix(line, " "), textBudget)
row := " " + zeroTheme.muted.Render(text)
if gutter {
row = zeroTheme.faintest.Render(fmt.Sprintf("%4d", newLine)) + row
if styled, ok := highlightedLines[displayLine.rawIndex]; ok {
lines = append(lines, diffContextStyledLine(hunk.newLine, styled, textBudget, gutter))
} else {
text := truncateRunes(strings.TrimPrefix(line, " "), textBudget)
row := " " + zeroTheme.muted.Render(text)
if gutter {
row = zeroTheme.faintest.Render(fmt.Sprintf("%4d", hunk.newLine)) + row
}
lines = append(lines, row)
}
lines = append(lines, row)
oldLine++
newLine++
hunk.oldLine++
hunk.newLine++
hunk.consume(line)
}
}
return cardBody{lines: capCardLines(lines, opts.bodyCap), headTag: diffHeadTag(meta)}
}
func highlightedAddedDiffLines(rawLines []string, meta diffMetadata, textBudget int) []string {
if !meta.addOnly() || meta.path == "" {
return nil
func isDiffViewerPreamble(line string) bool {
for _, prefix := range []string{
"diff --git ", "index ", "similarity index ", "dissimilarity index ",
} {
if strings.HasPrefix(line, prefix) {
return true
}
}
content := make([]string, 0, meta.adds)
for _, line := range rawLines {
if strings.HasPrefix(line, "+") && !strings.HasPrefix(line, "+++ ") {
content = append(content, strings.TrimPrefix(line, "+"))
return false
}
const (
diffViewerHighlightMaxLines = 10_000
diffViewerHighlightMaxBytes = 512 * 1024
diffViewerHighlightMaxLineBytes = 4 * 1024
)
type diffHighlightBudget struct {
lines int
bytes int
}
func (budget *diffHighlightBudget) reserve(lines int, bytes int) bool {
if lines > diffViewerHighlightMaxLines || bytes > diffViewerHighlightMaxBytes ||
budget.lines+lines > diffViewerHighlightMaxLines || budget.bytes+bytes > diffViewerHighlightMaxBytes {
return false
}
budget.lines += lines
budget.bytes += bytes
return true
}
// highlightedDiffLines lexes each unified-diff hunk as one source unit. This
// preserves lexer state through multiline comments, strings, and declarations,
// while still returning independently renderable rows. Large diffs fall back to
// the regular viewer so expanding a tool card remains responsive.
func highlightedDiffLines(rawLines []string, meta diffMetadata) map[int]string {
highlighted := make(map[int]string)
budget := diffHighlightBudget{}
path := ""
for index := 0; index < len(rawLines); {
line := rawLines[index]
switch {
case isDiffFileHeader(line):
if candidate := diffViewerSourcePath(line); candidate != "" {
path = candidate
}
index++
continue
case !strings.HasPrefix(line, "@@"):
index++
continue
}
hunk, ok := parseDiffHunkHeader(line)
if !ok {
index++
continue
}
start := index + 1
index = start
for index < len(rawLines) && hunk.active() && isDiffHunkBodyLine(rawLines[index]) {
hunk.consume(rawLines[index])
index++
}
hunkPath := path
if hunkPath == "" {
hunkPath = meta.path
}
if hunkPath == "" || !highlightDiffHunk(rawLines[start:index], start, hunkPath, highlighted, &budget) {
return nil
}
}
return highlighted
}
func diffViewerSourcePath(line string) string {
path := strings.TrimSpace(strings.TrimPrefix(strings.TrimPrefix(line, "--- "), "+++ "))
if tab := strings.IndexByte(path, '\t'); tab >= 0 {
path = path[:tab]
}
if path == "" || path == "/dev/null" {
return ""
}
if strings.HasPrefix(path, "a/") {
return strings.TrimPrefix(path, "a/")
}
return strings.TrimPrefix(path, "b/")
}
func isDiffHunkBodyLine(line string) bool {
return line == "" || line[0] == ' ' || line[0] == '+' || line[0] == '-' || line[0] == '\\'
}
func highlightDiffHunk(rawLines []string, rawStart int, path string, highlighted map[int]string, budget *diffHighlightBudget) bool {
content := make([]string, 0, len(rawLines))
backgrounds := make([]color.Color, 0, len(rawLines))
rawIndexes := make([]int, 0, len(rawLines))
contentIndexes := make(map[int]int, len(rawLines))
bytes := 0
for offset, line := range rawLines {
if len(line) == 0 || line[0] == '\\' {
continue
}
prefix := line[0]
if prefix != ' ' && prefix != '+' && prefix != '-' {
continue
}
contentIndexes[rawStart+offset] = len(content)
content = append(content, line[1:])
rawIndexes = append(rawIndexes, rawStart+offset)
bytes += len(line)
if len(line) > diffViewerHighlightMaxLineBytes {
return false
}
switch prefix {
case '+':
backgrounds = append(backgrounds, zeroTheme.addLine.GetBackground())
case '-':
backgrounds = append(backgrounds, zeroTheme.delLine.GetBackground())
default:
backgrounds = append(backgrounds, nil)
}
}
if len(content) == 0 {
return nil
return true
}
highlighted, ok := highlightCodeForPath(content, meta.path, textBudget, zeroTheme.addLine.GetBackground())
if !ok || len(highlighted) != len(content) {
return nil
if !budget.reserve(len(content), bytes) {
return false
}
return highlighted
spans := diffHunkWordSpans(rawLines, rawStart, contentIndexes)
styled, ok := highlightCodeForPathWithLineBackgrounds(content, path, 1<<20, backgrounds, spans)
if !ok || len(styled) != len(content) {
return false
}
for contentIndex, text := range styled {
highlighted[rawIndexes[contentIndex]] = text
}
return true
}
func diffHunkWordSpans(rawLines []string, rawStart int, contentIndexes map[int]int) []highlightSpan {
var spans []highlightSpan
for offset, line := range rawLines {
rawIndex := rawStart + offset
if !isDiffDelContent(line) || offset+1 >= len(rawLines) || !isDiffAddContent(rawLines[offset+1]) {
continue
}
if offset > 0 && isDiffDelContent(rawLines[offset-1]) {
continue
}
if offset+2 < len(rawLines) && isDiffAddContent(rawLines[offset+2]) {
continue
}
before := []rune(line[1:])
after := []rune(rawLines[offset+1][1:])
prefix, beforeEnd, afterEnd := changedSpan(before, after)
changed := maxInt(beforeEnd-prefix, afterEnd-prefix)
if changed == 0 || float64(changed)/float64(maxInt(len(before), len(after))) > 0.6 {
continue
}
if lineIndex, ok := contentIndexes[rawIndex]; ok {
spans = append(spans, highlightSpan{line: lineIndex, start: prefix, end: beforeEnd, background: zeroTheme.delLineWord.GetBackground()})
}
if lineIndex, ok := contentIndexes[rawIndex+1]; ok {
spans = append(spans, highlightSpan{line: lineIndex, start: prefix, end: afterEnd, background: zeroTheme.addLineWord.GetBackground()})
}
}
return spans
}
// diffBodyLine paints one changed row: optional gutter number, sign column,
@@ -2168,6 +2422,7 @@ func diffBodyStyledLine(number int, sign string, styledText string, added bool,
if added {
lineStyle, signStyle, numStyle = zeroTheme.addLine, zeroTheme.addSign, zeroTheme.addLineNum
}
styledText = fitStyledLine(styledText, textBudget)
if pad := textBudget - lipgloss.Width(styledText); pad > 0 {
styledText += lineStyle.Render(strings.Repeat(" ", pad))
}
@@ -2178,6 +2433,18 @@ func diffBodyStyledLine(number int, sign string, styledText string, added bool,
return numCol + signStyle.Render(" "+sign+" ") + styledText
}
func diffContextStyledLine(number int, styledText string, textBudget int, gutter bool) string {
styledText = fitStyledLine(styledText, textBudget)
if pad := textBudget - lipgloss.Width(styledText); pad > 0 {
styledText += zeroTheme.muted.Render(strings.Repeat(" ", pad))
}
row := " " + styledText
if gutter {
row = zeroTheme.faintest.Render(fmt.Sprintf("%4d", number)) + row
}
return row
}
func isDiffAddContent(s string) bool {
return strings.HasPrefix(s, "+") && !strings.HasPrefix(s, "+++ ")
}
@@ -2275,15 +2542,17 @@ func exploreCardBody(name string, hint string, arg string, detail string, width
if strings.TrimSpace(detail) != "" {
footer = zeroTheme.faint.Render("▸ details")
}
return cardBody{lines: []string{summary}, footer: footer}
return cardBody{lines: []string{summary}, footer: footer, canToggle: footer != ""}
}
body := exploreDetailCardBody(name, detail, width, opts)
lines := append([]string{summary}, body.lines...)
footer := body.footer
canToggle := body.canToggle
if opts.expanded && opts.bodyCap > 0 && footer == "" {
footer = zeroTheme.faint.Render("▾ collapse")
canToggle = true
}
return cardBody{lines: lines, footer: footer}
return cardBody{lines: lines, footer: footer, canToggle: canToggle}
}
func exploreDetailCardBody(name string, detail string, width int, opts cardRenderOptions) cardBody {
+26
View File
@@ -975,6 +975,32 @@ func TestEditedDiffCardHidesHunkHeader(t *testing.T) {
}
}
func TestEditedDiffCardHandlesBareBlankHunkContext(t *testing.T) {
m := limeTestModel()
diff := strings.Join([]string{
"--- a/time_test.py",
"+++ b/time_test.py",
"@@ -1,3 +1,3 @@",
" from datetime import datetime",
"",
"-print(datetime.now())",
"+print(f\"Current time: {datetime.now()}\")",
}, "\n")
row := transcriptRow{kind: rowToolResult, id: "call_1", tool: "edit_file", status: tools.StatusOK, detail: diff}
got := plainRender(t, m.renderRow(row, 96, buildRowContext(nil)))
for _, want := range []string{"Edited", "time_test.py", "(+1 -1)", "from datetime import datetime", " 3 −", " 3 +"} {
if !strings.Contains(got, want) {
t.Fatalf("edited diff card = %q, missing %q", got, want)
}
}
for _, line := range strings.Split(got, "\n") {
if strings.TrimSpace(line) == "2" {
return
}
}
t.Fatalf("edited diff card = %q, missing blank context line 2", got)
}
func TestReadCardBodyShowsExploredSummary(t *testing.T) {
m := limeTestModel()
// Mirrors the real read_file output shape: "<right-aligned N> | <text>".
+91 -7
View File
@@ -101,6 +101,27 @@ func highlightCodeForPath(code []string, path string, measure int, bg color.Colo
return highlightCodeWithLexer(cachedLexerForPath(path), code, measure, bg)
}
// highlightSpan overlays a background on a rune range in one source line while
// retaining the lexer-selected foreground. Diff rendering uses it to keep the
// precise changed word visible inside a syntax-highlighted add/delete row.
type highlightSpan struct {
line int
start, end int
background color.Color
}
func highlightCodeForPathWithSpans(code []string, path string, measure int, bg color.Color, spans []highlightSpan) ([]string, bool) {
return highlightCodeWithLexerAndSpans(cachedLexerForPath(path), code, measure, bg, spans)
}
// highlightCodeForPathWithLineBackgrounds applies syntax highlighting to a
// single source unit while preserving a distinct background for every source
// line. The diff viewer uses one lexer pass per hunk so multiline syntax keeps
// its state across changed and unchanged lines.
func highlightCodeForPathWithLineBackgrounds(code []string, path string, measure int, backgrounds []color.Color, spans []highlightSpan) ([]string, bool) {
return highlightCodeWithLexerAndLineBackgrounds(cachedLexerForPath(path), code, measure, backgrounds, spans)
}
func inferCodeLanguage(code []string) string {
for _, line := range code {
trimmed := strings.TrimSpace(line)
@@ -151,6 +172,18 @@ func inferCodeLanguage(code []string) string {
}
func highlightCodeWithLexer(lexer chroma.Lexer, code []string, measure int, bg color.Color) ([]string, bool) {
return highlightCodeWithLexerAndSpans(lexer, code, measure, bg, nil)
}
func highlightCodeWithLexerAndSpans(lexer chroma.Lexer, code []string, measure int, bg color.Color, spans []highlightSpan) ([]string, bool) {
backgrounds := make([]color.Color, len(code))
for index := range backgrounds {
backgrounds[index] = bg
}
return highlightCodeWithLexerAndLineBackgrounds(lexer, code, measure, backgrounds, spans)
}
func highlightCodeWithLexerAndLineBackgrounds(lexer chroma.Lexer, code []string, measure int, backgrounds []color.Color, spans []highlightSpan) ([]string, bool) {
if measure < 4 {
return nil, false
}
@@ -163,6 +196,10 @@ func highlightCodeWithLexer(lexer chroma.Lexer, code []string, measure int, bg c
}
lines := []string{}
spansByLine := make(map[int][]highlightSpan, len(spans))
for _, span := range spans {
spansByLine[span.line] = append(spansByLine[span.line], span)
}
var cur strings.Builder
curWidth := 0
flushLine := func() {
@@ -170,18 +207,52 @@ func highlightCodeWithLexer(lexer chroma.Lexer, code []string, measure int, bg c
cur.Reset()
curWidth = 0
}
emit := func(style lipgloss.Style, s string) {
if bg != nil {
style = style.Background(bg)
}
emit := func(style lipgloss.Style, s string, line, column int) {
var chunk strings.Builder
var chunkStyle lipgloss.Style
var chunkBackground color.Color
var haveChunkStyle bool
var styledBackground color.Color
var styleWithBackground lipgloss.Style
var haveStyledBackground bool
flushChunk := func() {
if chunk.Len() > 0 {
cur.WriteString(style.Render(chunk.String()))
cur.WriteString(chunkStyle.Render(chunk.String()))
chunk.Reset()
haveChunkStyle = false
chunkBackground = nil
}
}
for _, r := range s {
for offset, r := range []rune(s) {
var background color.Color
if line < len(backgrounds) {
background = backgrounds[line]
}
for _, span := range spansByLine[line] {
if column+offset >= span.start && column+offset < span.end {
background = span.background
break
}
}
if !haveStyledBackground || !sameHighlightColor(background, styledBackground) {
styleWithBackground = style
if background != nil {
styleWithBackground = styleWithBackground.Background(background)
}
styledBackground = background
haveStyledBackground = true
}
runeStyle := styleWithBackground
if !haveChunkStyle {
chunkStyle = runeStyle
chunkBackground = background
haveChunkStyle = true
} else if !sameHighlightColor(background, chunkBackground) {
flushChunk()
chunkStyle = runeStyle
chunkBackground = background
haveChunkStyle = true
}
rw := lipgloss.Width(string(r))
if curWidth+rw > measure {
flushChunk()
@@ -193,13 +264,17 @@ func highlightCodeWithLexer(lexer chroma.Lexer, code []string, measure int, bg c
flushChunk()
}
line, column := 0, 0
for _, token := range iterator.Tokens() {
style := tokenStyle(token.Type)
for index, part := range strings.Split(token.Value, "\n") {
if index > 0 {
flushLine()
line++
column = 0
}
emit(style, part)
emit(style, part, line, column)
column += len([]rune(part))
}
}
flushLine()
@@ -209,3 +284,12 @@ func highlightCodeWithLexer(lexer chroma.Lexer, code []string, measure int, bg c
}
return lines, true
}
func sameHighlightColor(a, b color.Color) bool {
if a == nil || b == nil {
return a == nil && b == nil
}
ar, ag, ab, aa := a.RGBA()
br, bg, bb, ba := b.RGBA()
return ar == br && ag == bg && ab == bb && aa == ba
}
+46 -12
View File
@@ -716,17 +716,19 @@ func (m model) transcriptRowBodyHeightCacheKeyOpts(row transcriptRow, width int,
}
func (m model) renderTranscriptRow(rowIndex int, row transcriptRow, width int, rc rowContext, startBodyY int) (string, []transcriptSelectableLine) {
return m.renderTranscriptRowFn(rowIndex, row, width, rc, startBodyY, m.renderRow)
opts := cardRenderOptions{bodyCap: cardBodyMaxLines, cwd: m.cwd}
return m.renderTranscriptRowFn(rowIndex, row, width, rc, startBodyY, m.renderRow, opts)
}
// renderTranscriptDetailedRow routes through renderTranscriptRowFn with
// renderRowDetailed (bodyCap: 0) so tool output appears uncapped.
func (m model) renderTranscriptDetailedRow(rowIndex int, row transcriptRow, width int, rc rowContext, startBodyY int) (string, []transcriptSelectableLine) {
return m.renderTranscriptRowFn(rowIndex, row, width, rc, startBodyY, m.renderRowDetailed)
opts := cardRenderOptions{bodyCap: 0, cwd: m.cwd}
return m.renderTranscriptRowFn(rowIndex, row, width, rc, startBodyY, m.renderRowDetailed, opts)
}
// renderTranscriptRowFn dispatches row-kind rendering using the provided renderFn.
func (m model) renderTranscriptRowFn(rowIndex int, row transcriptRow, width int, rc rowContext, startBodyY int, renderFn rowRenderFn) (string, []transcriptSelectableLine) {
func (m model) renderTranscriptRowFn(rowIndex int, row transcriptRow, width int, rc rowContext, startBodyY int, renderFn rowRenderFn, toolOpts cardRenderOptions) (string, []transcriptSelectableLine) {
switch row.kind {
case rowUser:
return m.renderSelectableUserRow(rowIndex, row, width, startBodyY)
@@ -737,7 +739,7 @@ func (m model) renderTranscriptRowFn(rowIndex int, row transcriptRow, width int,
case rowSystem, rowError, rowToolCall, rowPermission, rowAskUser:
return m.renderSelectableRenderedRowFn(rowIndex, row, width, rc, startBodyY, renderFn)
case rowToolResult:
return m.renderSelectableToolResultRowFn(rowIndex, row, width, rc, startBodyY, renderFn)
return m.renderSelectableToolResultRowFn(rowIndex, row, width, rc, startBodyY, renderFn, toolOpts)
case rowSpecialist:
return m.renderSelectableSpecialistRowFn(rowIndex, row, width, rc, startBodyY, renderFn)
default:
@@ -750,19 +752,19 @@ func (m model) renderTranscriptRowFn(rowIndex int, row transcriptRow, width int,
}
}
// renderSelectableToolResultRow renders the tool result card and marks its head
// (first line) as a clickable collapse/expand toggle. Body/footer text remains
// selectable so copying a visible transcript range includes command output.
func (m model) renderSelectableToolResultRowFn(rowIndex int, row transcriptRow, width int, rc rowContext, startBodyY int, renderFn rowRenderFn) (string, []transcriptSelectableLine) {
// renderSelectableToolResultRow renders the tool result card. Only cards that
// can actually collapse expose a clickable header; always-visible diff cards
// retain normal text selection and never imply an unavailable action.
func (m model) renderSelectableToolResultRowFn(rowIndex int, row transcriptRow, width int, rc rowContext, startBodyY int, renderFn rowRenderFn, opts cardRenderOptions) (string, []transcriptSelectableLine) {
rendered := renderFn(row, width, rc)
if rendered == "" {
return "", nil
}
// The first rendered line is the clickable toggle header; carry its text so a
// selection dragged through it copies the label too (the toggle flag still
// expands/collapses on a direct click, resolved on press before selection).
// Carry the first line's text so a selection dragged through the card copies
// its label too. It receives a toggle only when this card exposes a collapse
// affordance in the current transcript mode.
allLines := viewLines(rendered)
header := transcriptSelectableLine{bodyY: startBodyY, rowIndex: rowIndex, toggle: true}
header := transcriptSelectableLine{bodyY: startBodyY, rowIndex: rowIndex, toggle: toolResultCanToggle(row, width, rc, opts)}
if len(allLines) > 0 {
if meta, ok := selectableLineFromRenderedLine(rowIndex, startBodyY, allLines[0], false); ok {
header.text = meta.text
@@ -778,6 +780,38 @@ func (m model) renderSelectableToolResultRowFn(rowIndex int, row transcriptRow,
return rendered, selectable
}
func toolResultCanToggle(row transcriptRow, width int, rc rowContext, opts cardRenderOptions) bool {
name := toolRowName(row)
if opts.bodyCap <= 0 || toolCardAlwaysExpands(name) {
return false
}
failed := row.status == tools.StatusError
if !failed && looksLikeRedundantConfirmation(row.detail) {
return false
}
collapsedFooter := ""
if failed || (!isExploreTool(name) && !isLocalControlTool(name)) {
collapsedFooter = collapsedToolFooter(row.detail)
}
if collapsedFooter != "" && !row.expanded {
return true
}
// Explore cards deliberately use their own compact "details" affordance
// instead of the generic long-output footer. Ask the body renderer whether
// it exposed that affordance so selection behavior stays coupled to the
// rendered card rather than matching display text.
if collapsedFooter == "" && !isExploreTool(name) {
return false
}
bodyOpts := opts
bodyOpts.expanded = row.expanded
body := toolCardBody(name, rc.hints[rcKey(row.runID, row.id)], rc.args[rcKey(row.runID, row.id)], row.detail, width, bodyOpts, failed)
if collapsedFooter != "" && row.expanded && body.footer == "" {
return true
}
return body.canToggle
}
func (m model) renderSelectableRenderedRowFn(rowIndex int, row transcriptRow, width int, rc rowContext, startBodyY int, renderFn rowRenderFn) (string, []transcriptSelectableLine) {
rendered := renderFn(row, width, rc)
if rendered == "" {
+97
View File
@@ -0,0 +1,97 @@
package worktrees
import (
"context"
"fmt"
"os"
"os/exec"
"runtime"
"strings"
"testing"
)
type discardWriter struct{}
func (discardWriter) Write(p []byte) (int, error) { return len(p), nil }
// The constructor every git call goes through must carry the hardening, so a
// future call site cannot get an unhardened command by using it.
func TestTheGitConstructorHardens(t *testing.T) {
command := newHardenedCommand(context.Background(), t.TempDir(), "git", "status")
if command.WaitDelay == 0 {
t.Fatal("WaitDelay is unset; a cancelled git can block Wait indefinitely")
}
if command.Dir == "" {
t.Fatal("the working directory was dropped")
}
// The process-group half is POSIX-only and is asserted in
// run_git_unix_test.go, which can name Setpgid at all — this file must
// compile on Windows, where it does not exist.
if runtime.GOOS != "windows" && command.Cancel == nil {
t.Fatal("Cancel is unset; the process group is never signalled")
}
}
// ...and the git runner itself still works, so the hardening did not break the
// ordinary path.
func TestHardenedGitStillRuns(t *testing.T) {
if _, err := exec.LookPath("git"); err != nil {
t.Skipf("no git: %v", err)
}
result, err := defaultRunGit(context.Background(), t.TempDir(), "--version")
if err != nil {
t.Fatalf("git --version: %v", err)
}
if result.ExitCode != 0 || result.Stdout == "" {
t.Fatalf("result = %+v", result)
}
}
// EXACTLY ONE PLACE BUILDS A SUBPROCESS IN THIS PACKAGE.
//
// Every behavioural test above goes through newHardenedCommand, so all of them
// pass against a defaultRunGit that quietly builds its own unhardened command —
// which is the wiring gap this feature has produced five times, and the only one
// no runtime assertion here can reach: defaultRunGit returns a result, not the
// command it used.
//
// So the invariant is enforced at the SOURCE. This is the small, local form of
// the AST enforcement the repo-wide hardening sweep would need, applied to the
// one package a plan reaches.
func TestOnlyTheHardenedConstructorBuildsSubprocesses(t *testing.T) {
entries, err := os.ReadDir(".")
if err != nil {
t.Fatal(err)
}
offenders := []string{}
checked := 0
for _, entry := range entries {
name := entry.Name()
if entry.IsDir() || !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") {
continue
}
raw, err := os.ReadFile(name)
if err != nil {
t.Fatal(err)
}
checked++
for index, line := range strings.Split(string(raw), "\n") {
if !strings.Contains(line, "exec.Command") {
continue
}
// The one permitted site is inside newHardenedCommand, which
// hardens what it builds.
if name == "worktrees.go" && strings.Contains(line, "exec.CommandContext(ctx, name, args...)") {
continue
}
offenders = append(offenders, fmt.Sprintf("%s:%d: %s", name, index+1, strings.TrimSpace(line)))
}
}
if checked == 0 {
t.Fatal("no source files were scanned; this test checked nothing")
}
if len(offenders) > 0 {
t.Fatalf("a subprocess is built outside newHardenedCommand, so it is not hardened:\n %s",
strings.Join(offenders, "\n "))
}
}
+39
View File
@@ -0,0 +1,39 @@
//go:build !windows
package worktrees
import (
"errors"
"os/exec"
"syscall"
"time"
)
// worktreeWaitDelay bounds how long Wait blocks for the child's stdout/stderr
// pipes to drain after the process exits or its context is cancelled, so a
// leaked grandchild cannot hang the parent past cancel/timeout. Var (not const)
// so tests can shorten it.
var worktreeWaitDelay = 2 * time.Second
// hardenWorktreeGit makes a git subprocess killable as a single unit. Setpgid
// puts the child into its own process group, so on cancel/timeout we signal the
// whole group (negative pid) — any long-running git subprocess dies with it
// instead of being orphaned. WaitDelay is the backstop if a grandchild still
// holds a pipe after the group is killed. Must be called before command.Run.
func hardenWorktreeGit(command *exec.Cmd) {
if command.SysProcAttr == nil {
command.SysProcAttr = &syscall.SysProcAttr{}
}
command.SysProcAttr.Setpgid = true
command.WaitDelay = worktreeWaitDelay
command.Cancel = func() error {
if command.Process == nil {
return nil
}
// Negative pid targets the whole process group led by the child.
if err := syscall.Kill(-command.Process.Pid, syscall.SIGKILL); err != nil && !errors.Is(err, syscall.ESRCH) {
return err
}
return nil
}
}
+120
View File
@@ -0,0 +1,120 @@
//go:build !windows
package worktrees
import (
"context"
"os"
"os/exec"
"path/filepath"
"strconv"
"strings"
"syscall"
"testing"
"time"
)
// THE HARDENING HAS TO WORK, not merely be configured.
//
// A leaked grandchild holding the pipes is what makes an unhardened cancel hang:
// CommandContext signals the direct child, Wait blocks until the pipes reach
// EOF, and the grandchild holds them open. So this launches exactly that shape —
// a shell that spawns a long sleeper inheriting stdout — cancels, and requires
// the call to return.
//
// It goes through newHardenedCommand, the constructor defaultRunGit uses. A test
// calling hardenWorktreeGit directly would prove three fields are set and prove
// nothing about whether anything calls it.
// TestTheGitConstructorSetsAProcessGroup is the POSIX half of the constructor
// assertion; the portable half lives in run_git_test.go.
func TestTheGitConstructorSetsAProcessGroup(t *testing.T) {
command := newHardenedCommand(context.Background(), t.TempDir(), "git", "status")
if command.SysProcAttr == nil || !command.SysProcAttr.Setpgid {
t.Fatal("Setpgid is unset; a cancel signals only the direct child")
}
if command.Cancel == nil {
t.Fatal("Cancel is unset; the process group is never signalled")
}
}
func TestACancelledCommandKillsTheWholeProcessGroup(t *testing.T) {
if _, err := exec.LookPath("sh"); err != nil {
t.Skipf("no shell: %v", err)
}
previous := worktreeWaitDelay
worktreeWaitDelay = 500 * time.Millisecond
t.Cleanup(func() { worktreeWaitDelay = previous })
dir := t.TempDir()
pidFile := filepath.Join(dir, "grandchild.pid")
ctx, cancel := context.WithCancel(context.Background())
// A grandchild that outlives its parent shell and inherits stdout — the
// exact shape that hangs an unhardened Wait AND survives a kill aimed at
// the direct child only.
// pidFile is passed as a positional argument ($1), not interpolated into the
// script, so a temp path containing a space or shell metacharacter cannot
// break or reshape the command.
script := `sleep 60 & echo $! > "$1"; sleep 60`
command := newHardenedCommand(ctx, dir, "sh", "-c", script, "sh", pidFile)
var sink discardWriter
command.Stdout = &sink
command.Stderr = &sink
if err := command.Start(); err != nil {
t.Fatalf("start: %v", err)
}
// Wait for the grandchild to exist before cancelling, or the test proves
// nothing about what happens to it.
var grandchild int
deadline := time.Now().Add(10 * time.Second)
for time.Now().Before(deadline) {
raw, err := os.ReadFile(pidFile)
if err == nil {
if pid, convErr := strconv.Atoi(strings.TrimSpace(string(raw))); convErr == nil && pid > 0 {
grandchild = pid
break
}
}
time.Sleep(20 * time.Millisecond)
}
if grandchild == 0 {
// The command is already running; clean it up before failing rather than
// leaking the shell and its grandchild. A missed pid is a failure of the
// setup this test depends on, not a reason to silently skip the assertion.
cancel()
_ = command.Wait()
t.Fatal("the grandchild never reported its pid")
}
cancel()
// FIRST: the call must return. An unhardened Wait blocks on the pipes the
// grandchild holds open.
done := make(chan error, 1)
go func() { done <- command.Wait() }()
select {
case <-done:
case <-time.After(15 * time.Second):
t.Fatal("Wait did not return after cancel; a leaked grandchild is holding the pipes open")
}
// SECOND, and this is the property WaitDelay alone does not give: the
// grandchild must be DEAD. Returning promptly while leaving a process
// running is the orphan this hardening exists to prevent, and a test that
// only checked the return would pass against a cancel aimed at the direct
// child alone.
alive := false
for attempt := 0; attempt < 50; attempt++ {
if err := syscall.Kill(grandchild, 0); err != nil {
alive = false
break
}
alive = true
time.Sleep(20 * time.Millisecond)
}
if alive {
_ = syscall.Kill(grandchild, syscall.SIGKILL)
t.Fatalf("grandchild %d survived the cancel; the signal did not reach the process group", grandchild)
}
}
+21
View File
@@ -0,0 +1,21 @@
//go:build windows
package worktrees
import (
"os/exec"
"time"
)
// worktreeWaitDelay bounds how long Wait blocks for the child's stdout/stderr
// pipes to drain after the process exits or its context is cancelled, so a
// leaked grandchild cannot hang the parent past cancel/timeout. Var (not const)
// so tests can shorten it.
var worktreeWaitDelay = 2 * time.Second
// hardenWorktreeGit sets WaitDelay so a leaked grandchild cannot block Wait
// indefinitely. Windows lacks POSIX process groups; tree-killing is not wired
// for this path, so the default Cancel (Process.Kill) plus WaitDelay is used.
func hardenWorktreeGit(command *exec.Cmd) {
command.WaitDelay = worktreeWaitDelay
}
+23 -2
View File
@@ -740,12 +740,33 @@ func gitCommonDir(ctx context.Context, runGit GitRunner, dir string) (string, er
return filepath.Clean(resolved), nil
}
func defaultRunGit(ctx context.Context, dir string, args ...string) (CommandResult, error) {
command := exec.CommandContext(ctx, "git", args...)
// newHardenedCommand builds a subprocess that dies as one unit on cancel.
//
// Extracted so a test can exercise the SAME constructor defaultRunGit uses. A
// test that only called hardenWorktreeGit would prove the helper sets three
// fields and prove nothing about whether anything calls it — which is the
// wiring gap this feature has produced four times.
func newHardenedCommand(ctx context.Context, dir string, name string, args ...string) *exec.Cmd {
command := exec.CommandContext(ctx, name, args...)
command.Dir = dir
// Pin C locale so English phrases callers match on (e.g. "already locked"
// in lockWorktree) stay parseable under localized git installs.
//
// Set HERE rather than in defaultRunGit, so every command built through this
// constructor is parseable, not just the one path that happened to need it
// first.
command.Env = append(os.Environ(), "LC_ALL=C", "LANG=C")
// Without this a cancelled git is signalled but its Wait can still block on
// pipes a grandchild holds open. git is short-lived and rarely forks, so
// this is a small risk — but the worktree path is now reached by a plan,
// and a plan can be cancelled at any moment by /plans stop or by the run
// ending.
hardenWorktreeGit(command)
return command
}
func defaultRunGit(ctx context.Context, dir string, args ...string) (CommandResult, error) {
command := newHardenedCommand(ctx, dir, "git", args...)
// Capture stdout and stderr separately: callers parse Stdout for values
// (rev-parse output) and prefer Stderr for error messages. CombinedOutput
// merged the two, letting git's stderr warnings pollute parsed output and
+739 -11
View File
@@ -3,11 +3,16 @@ package zerogit
import (
"bytes"
"context"
"errors"
"fmt"
"os"
"os/exec"
"os/user"
"path/filepath"
"regexp"
"strconv"
"strings"
"unicode"
"unicode/utf8"
"github.com/Gitlawb/zero/internal/redaction"
@@ -528,11 +533,18 @@ func firstNonEmpty(values ...string) string {
}
type PushOptions struct {
Cwd string
Remote string
Branch string
Force bool
DryRun bool
Cwd string
Remote string
Branch string
Force bool
DryRun bool
// RequireNewRemoteBranch guards this push with a zero-value
// --force-with-lease, so it is rejected instead of fast-forwarding if
// Branch already exists on Remote. CreateBranch's collision probe reads
// the remote's branches before this push runs, leaving a window in which
// a concurrent creator can publish the same generated name; this closes
// it at the one point that actually talks to the remote atomically.
RequireNewRemoteBranch bool
AllowPushDefaultBranch bool
RunGit Runner
RunGitEnv EnvRunner
@@ -578,7 +590,11 @@ func Push(ctx context.Context, options PushOptions) (PushResult, error) {
}
if !options.AllowPushDefaultBranch {
if isDefaultBranch(ctx, runGit, root, remote, branch) {
isDefault, err := isDefaultBranch(ctx, runGit, root, remote, branch)
if err != nil {
return PushResult{}, fmt.Errorf("cannot verify %q is not the default/protected branch: %w; use --yes to override", branch, err)
}
if isDefault {
return PushResult{}, fmt.Errorf("refusing to push to %q (default/protected branch); use --yes to override", branch)
}
}
@@ -587,7 +603,14 @@ func Push(ctx context.Context, options PushOptions) (PushResult, error) {
if options.DryRun {
args = append(args, "--dry-run")
}
if options.Force {
switch {
case options.RequireNewRemoteBranch:
// An empty expected value means the ref must not currently exist on
// the remote: Git rejects the push if another client created
// <branch> after CreateBranch's own remote probe ran, instead of
// silently fast-forwarding it with this work.
args = append(args, "--force-with-lease="+branch+":")
case options.Force:
args = append(args, "--force-with-lease")
}
args = append(args, "-u", "--", remote, branch)
@@ -597,6 +620,28 @@ func Push(ctx context.Context, options PushOptions) (PushResult, error) {
return PushResult{}, fmt.Errorf("push: %w", err)
}
// git push -u can create the remote branch and still exit 0 when it cannot
// write branch.<name>.remote/merge (for example .git/config.lock held by
// another process). Zero must not report that as a full success: without
// the local upstream, a later ensureFeatureBranch retry would reassert
// --force-with-lease=<branch>: against a branch that already exists.
// Dry-run never publishes and never writes branch.<name>.remote/merge, so
// skip verification/repair: set-upstream-to would mutate the repo and fail
// for unpublished branches with a misleading "push published" error.
expectedUpstream := remote + "/" + branch
if !options.DryRun && IsNamedRemote(ctx, root, remote, runGit) && UpstreamRef(ctx, root, branch, runGit) != expectedUpstream {
if _, setErr := gitOutput(ctx, runGit, root, "branch", "--set-upstream-to="+expectedUpstream, branch); setErr != nil {
return PushResult{Remote: remote, Branch: branch, Output: output}, fmt.Errorf(
"push published %s but failed to configure local upstream %s: %w; run `git branch --set-upstream-to=%s %s` then retry",
expectedUpstream, expectedUpstream, setErr, expectedUpstream, branch)
}
if UpstreamRef(ctx, root, branch, runGit) != expectedUpstream {
return PushResult{Remote: remote, Branch: branch, Output: output}, fmt.Errorf(
"push published %s but local upstream is still not %s; run `git branch --set-upstream-to=%s %s` then retry",
expectedUpstream, expectedUpstream, expectedUpstream, branch)
}
}
return PushResult{
Remote: remote,
Branch: branch,
@@ -604,18 +649,701 @@ func Push(ctx context.Context, options PushOptions) (PushResult, error) {
}, nil
}
func isDefaultBranch(ctx context.Context, runGit Runner, dir, remote, branch string) bool {
if out, err := gitOutput(ctx, runGit, dir, "ls-remote", "--symref", remote, "HEAD"); err == nil {
func isDefaultBranch(ctx context.Context, runGit Runner, dir, remote, branch string) (bool, error) {
// "--" terminates option parsing: remote comes from --remote/branch config,
// and a value like "--upload-pack=/bin/echo" must reach Git as a positional
// argument, never as an option.
if out, err := gitOutput(ctx, runGit, dir, "ls-remote", "--symref", "--", remote, "HEAD"); err == nil {
for _, line := range strings.Split(out, "\n") {
line = strings.TrimSpace(line)
if strings.HasPrefix(line, "ref: refs/heads/") && strings.HasSuffix(line, "\tHEAD") {
symref := strings.TrimPrefix(line, "ref: refs/heads/")
symref = strings.TrimSuffix(symref, "\tHEAD")
return branch == symref
return branch == symref, nil
}
}
if strings.TrimSpace(out) == "" {
// HEAD didn't resolve to anything. A genuinely unborn remote (no
// refs at all) answers this way, and it cannot have a protected
// default branch yet, so the fail-closed error below would make
// the very first feature-branch push a dead end. But a non-empty
// remote whose HEAD symref is dangling or missing produces the
// same empty output while still possibly having a protected
// default under a name this couldn't identify, so confirm the
// remote truly has no branches before granting the unborn
// exception; a non-default first push is safe.
if heads, headsErr := gitOutput(ctx, runGit, dir, "ls-remote", "--heads", "--", remote); headsErr == nil && strings.TrimSpace(heads) == "" {
if branch == "main" || branch == "master" {
return true, nil
}
return false, nil
}
}
}
return branch == "main" || branch == "master"
// The remote lookup failed (unreachable, slow, or gave no symref): the
// local refs/remotes/<remote>/HEAD record, written by clone or `git remote
// set-head`, is only a cache. Trust it to *block* a push (a positive match
// proves branch is the recorded default) but never to *clear* the guard. If
// the server renamed its default (main -> trunk) the stale record still
// names main, so a mismatch here is not evidence that pushing trunk is safe;
// fall through instead of returning false.
if out, err := gitOutput(ctx, runGit, dir, "symbolic-ref", "--quiet", "refs/remotes/"+remote+"/HEAD"); err == nil {
if name, ok := strings.CutPrefix(strings.TrimSpace(out), "refs/remotes/"+remote+"/"); ok && name == branch {
return true, nil
}
}
// The conventional default names are only a fallback for when the remote's
// actual default genuinely could not be determined above (live or cached).
// Applying this before consulting the remote at all would misidentify a
// repository whose real default is e.g. "trunk" but that also happens to
// have a local/tracked "main": the live symref result must win whenever
// it's available. This is still safe-direction only (it can block a push,
// never permit one) since it's the last resort before failing closed.
if branch == "main" || branch == "master" {
return true, nil
}
// Fail closed: before this, a lookup timeout silently downgraded the
// check to the main/master name heuristic, so a repository whose default
// is trunk/develop lost the confirmation guard exactly when the remote
// was slow.
return false, fmt.Errorf("default branch for remote %q is unknown (remote lookup failed and no local refs/remotes/%s/HEAD record exists; run `git remote set-head %s --auto` to record it)", remote, remote, remote)
}
// DefaultBranchOptions resolves whether a branch is the repository's
// default/protected branch.
type DefaultBranchOptions struct {
Cwd string
Remote string
Branch string // empty resolves the current branch
RunGit Runner
}
// IsDefaultBranch reports whether options.Branch (or, if empty, the current
// branch) is the repository's default/protected branch, using the same check
// Push already applies before refusing to push straight to it. It returns the
// resolved branch name and remote alongside the bool so callers that left
// them empty don't need a second lookup: the remote is resolved exactly the
// way Push resolves it (explicit option, then the branch's configured
// upstream, then "origin"), so a caller can thread the same remote through a
// later Push instead of letting a freshly created branch with no tracking
// configuration silently fall back to "origin".
func IsDefaultBranch(ctx context.Context, options DefaultBranchOptions) (bool, string, string, error) {
cwd, err := resolveCwd(options.Cwd)
if err != nil {
return false, "", "", err
}
runGit, _ := resolveRunners(options.RunGit, nil)
root, err := gitOutput(ctx, runGit, cwd, "rev-parse", "--show-toplevel")
if err != nil {
return false, "", "", fmt.Errorf("not a git repository: %w", err)
}
root = filepath.Clean(root)
branch := strings.TrimSpace(options.Branch)
if branch == "" {
branch, err = gitOutput(ctx, runGit, root, "rev-parse", "--abbrev-ref", "HEAD")
if err != nil {
return false, "", "", fmt.Errorf("resolve current branch: %w", err)
}
}
remote := strings.TrimSpace(options.Remote)
if remote == "" {
if upstream, err := gitOutput(ctx, runGit, root, "config", "branch."+branch+".remote"); err == nil && upstream != "" {
remote = upstream
} else {
remote = "origin"
}
}
// Honor the caller's deadline (or lack of one). A fixed short timeout here
// turned slow but reachable remotes (SSH/VPN handshakes) into fail-closed
// "use --yes" errors for ordinary feature-branch pushes, because
// ensureFeatureBranch always consults this before Push. Callers that need
// a bound should pass a context with a deadline.
isDefault, err := isDefaultBranch(ctx, runGit, root, remote, branch)
if err != nil {
return false, branch, remote, err
}
return isDefault, branch, remote, nil
}
// BranchOptions configures creating and checking out a new local branch.
type BranchOptions struct {
Cwd string
Name string // full branch name, e.g. "alice/fix-typo"
DryRun bool
RunGit Runner
// Remote, when non-empty, is the remote the new branch will be pushed
// to; its branch names are treated as taken when resolving collisions,
// so a remote-only stale branch (e.g. an old merged PR) is never
// fast-forwarded with unrelated new work.
Remote string
}
// BranchResult reports the branch that was (or, in dry-run, would be) created.
type BranchResult struct {
Branch string `json:"branch"`
}
// CreateBranch checks out a new local branch named options.Name off the
// current HEAD. DryRun returns the requested trimmed name without collision
// resolution or repository mutation; a real run may append a numeric suffix
// when the name is already taken locally or on the remote.
func CreateBranch(ctx context.Context, options BranchOptions) (BranchResult, error) {
cwd, err := resolveCwd(options.Cwd)
if err != nil {
return BranchResult{}, err
}
runGit, _ := resolveRunners(options.RunGit, nil)
root, err := gitOutput(ctx, runGit, cwd, "rev-parse", "--show-toplevel")
if err != nil {
return BranchResult{}, fmt.Errorf("not a git repository: %w", err)
}
root = filepath.Clean(root)
name := strings.TrimSpace(options.Name)
if name == "" {
return BranchResult{}, fmt.Errorf("branch name required")
}
if strings.HasPrefix(name, "-") || strings.HasPrefix(name, "/") ||
strings.Contains(name, "..") || strings.ContainsAny(name, "\\ \t\n") {
return BranchResult{}, fmt.Errorf("invalid branch name %q", name)
}
if options.DryRun {
return BranchResult{Branch: name}, nil
}
// A repeated run (the same diff producing the same slug, or a low-entropy
// fallback name) can collide with a branch that already exists locally.
// Never check that existing ref out: its history may be entirely
// unrelated to the current work (an old push under the same name), and
// switching to it would publish the stale branch while leaving the new
// commit behind on the default branch. Pick a unique suffixed name off
// the current HEAD instead, and fail visibly when the namespace is
// exhausted rather than guess.
//
// Local refs are not enough: a branch that exists only on the target
// remote (an old merged-PR branch, or one pruned locally) would be
// silently fast-forwarded by the later `push -u`, appending the new work
// to an unrelated remote branch. Probe the remote's heads once under the
// caller's context (same connectivity the later push needs) and fail
// visibly when the remote cannot be consulted.
remoteTaken := map[string]bool{}
if remote := strings.TrimSpace(options.Remote); remote != "" {
out, err := gitOutput(ctx, runGit, root, "ls-remote", "--heads", "--", remote)
if err != nil {
return BranchResult{}, fmt.Errorf("cannot check branch names against remote %q: %w", remote, err)
}
for _, line := range strings.Split(out, "\n") {
if _, ref, ok := strings.Cut(strings.TrimSpace(line), "\t"); ok {
remoteTaken[strings.TrimPrefix(strings.TrimSpace(ref), "refs/heads/")] = true
}
}
}
base := name
for suffix := 2; ; suffix++ {
_, localErr := gitOutput(ctx, runGit, root, "rev-parse", "--verify", "--quiet", "refs/heads/"+name)
if localErr != nil && !remoteTaken[name] {
break
}
if suffix > 9 {
return BranchResult{}, fmt.Errorf("branch %q already exists locally or on the remote (as do %s-2 through %s-9); delete the stale branches or create one explicitly with `git checkout -b`", base, base, base)
}
name = fmt.Sprintf("%s-%d", base, suffix)
}
if _, err := gitOutput(ctx, runGit, root, "checkout", "-b", name, "--"); err != nil {
return BranchResult{}, fmt.Errorf("create branch %q: %w", name, err)
}
return BranchResult{Branch: name}, nil
}
// HeadCommitSubject returns the subject line of the HEAD commit, or "" when
// it cannot be resolved (empty repository, not a git directory). Callers use
// it to name the branch for a push that follows a commit, where the working
// tree is already clean and a diff-based name would be empty.
func HeadCommitSubject(ctx context.Context, cwd string, runGit Runner) string {
runGit, _ = resolveRunners(runGit, nil)
if subject, err := gitOutput(ctx, runGit, cwd, "log", "-1", "--format=%s"); err == nil {
return strings.TrimSpace(subject)
}
return ""
}
// CommitsAhead reports how many commits HEAD is ahead of the remote-tracking
// ref <remote>/<branch> or the advertised branch on a direct remote. Auto-branching
// runs this before creating and pushing a feature branch off the default branch: a
// clean, up-to-date default branch has nothing to publish, so the caller can refuse
// rather than push an empty comparison. It returns an error when the count cannot be
// determined (for example the remote-tracking ref was never fetched); callers treat
// that as a hard failure rather than guessing that there is something to publish.
func CommitsAhead(ctx context.Context, cwd, remote, branch string, runGit Runner) (int, error) {
runGit, _ = resolveRunners(runGit, nil)
var baseRef string
if IsNamedRemote(ctx, cwd, remote, runGit) {
baseRef = remote + "/" + branch
} else {
// Non-named remote (direct path or URL). Resolve the advertised remote branch tip via ls-remote.
out, err := gitOutput(ctx, runGit, cwd, "ls-remote", "--heads", "--", remote, "refs/heads/"+branch)
if err != nil {
return 0, fmt.Errorf("cannot check remote branch %s on %s: %w", branch, remote, err)
}
var remoteSHA string
for _, line := range strings.Split(out, "\n") {
if sha, ref, ok := strings.Cut(strings.TrimSpace(line), "\t"); ok {
if strings.TrimSpace(ref) == "refs/heads/"+branch {
remoteSHA = strings.TrimSpace(sha)
break
}
}
}
if remoteSHA == "" {
// Remote has no such branch yet; count all commits on HEAD.
out, err := gitOutput(ctx, runGit, cwd, "rev-list", "--count", "HEAD")
if err != nil {
return 0, err
}
count, err := strconv.Atoi(strings.TrimSpace(out))
if err != nil {
return 0, fmt.Errorf("parse commit count %q: %w", out, err)
}
return count, nil
}
baseRef = remoteSHA
}
if strings.HasPrefix(baseRef, "-") {
return 0, fmt.Errorf("invalid base ref %q", baseRef)
}
out, err := gitOutput(ctx, runGit, cwd, "rev-list", "--count", baseRef+"..HEAD")
if err != nil {
return 0, err
}
count, err := strconv.Atoi(strings.TrimSpace(out))
if err != nil {
return 0, fmt.Errorf("parse commit count %q: %w", out, err)
}
return count, nil
}
// RefreshTrackingRef updates the local remote-tracking ref for <remote>/<branch>
// from the remote's current advertised tip. ensureFeatureBranch calls this
// before CommitsAhead: IsDefaultBranch already contacted the remote for its
// symref check, but a merely-cached origin/main (last written at clone or a
// previous fetch) can sit behind the remote's live tip, making a local-only
// rev-list comparison report nothing to publish when the remote has actually
// advanced. When remote is a raw URL or path, no remote-tracking ref is
// maintained and this call is a no-op.
func RefreshTrackingRef(ctx context.Context, cwd, remote, branch string, runGit Runner) error {
runGit, _ = resolveRunners(runGit, nil)
if !IsNamedRemote(ctx, cwd, remote, runGit) {
return nil
}
refspec := fmt.Sprintf("+refs/heads/%s:refs/remotes/%s/%s", branch, remote, branch)
// "--" terminates option parsing so a remote value shaped like an option
// (--upload-pack=/bin/echo) reaches Git as a positional argument.
_, err := gitOutput(ctx, runGit, cwd, "fetch", "--", remote, refspec)
return err
}
// ResolveRemoteBranchTip returns the commit SHA that branch points to on
// remote. ensureFeatureBranch uses this as the restore target after
// auto-branching, and that target must never be a bare local branch name:
// resolving "main" yields the very commit the just-created feature branch
// owns, which makes the restore a silent no-op and leaves the publishable
// commits on the default branch. For a named remote the SHA comes from the
// local remote-tracking ref (which RefreshTrackingRef has just updated); for a
// direct URL or path it is resolved from the remote's advertised heads. It
// fails closed when no such commit exists rather than letting the caller
// substitute a local branch name.
func ResolveRemoteBranchTip(ctx context.Context, cwd, remote, branch string, runGit Runner) (string, error) {
runGit, _ = resolveRunners(runGit, nil)
if IsNamedRemote(ctx, cwd, remote, runGit) {
out, err := gitOutput(ctx, runGit, cwd, "rev-parse", "--verify", "refs/remotes/"+remote+"/"+branch+"^{commit}")
if err != nil {
return "", fmt.Errorf("resolve restore tip refs/remotes/%s/%s: %w", remote, branch, err)
}
return strings.TrimSpace(out), nil
}
// Direct URL or path: resolve the advertised branch tip without creating
// or updating a remote-tracking ref.
out, err := gitOutput(ctx, runGit, cwd, "ls-remote", "--heads", "--", remote, "refs/heads/"+branch)
if err != nil {
return "", fmt.Errorf("cannot resolve branch %s on %s: %w", branch, remote, err)
}
for _, line := range strings.Split(out, "\n") {
if sha, ref, ok := strings.Cut(strings.TrimSpace(line), "\t"); ok {
if strings.TrimSpace(ref) == "refs/heads/"+branch && strings.TrimSpace(sha) != "" {
return strings.TrimSpace(sha), nil
}
}
}
return "", fmt.Errorf("remote %s has no branch %s to restore to", remote, branch)
}
// isRemoteURLOrPath reports whether remote is formatted as a direct URL
// (https://..., git@..., ssh://...) or filesystem path (/path/to/repo,
// C:\path\to\repo, .\repo), as opposed to a configured named remote (origin, upstream, team/upstream).
func isRemoteURLOrPath(remote string) bool {
remote = strings.TrimSpace(remote)
if remote == "" {
return false
}
if strings.Contains(remote, "://") ||
strings.HasPrefix(remote, "/") ||
strings.HasPrefix(remote, "\\") ||
strings.HasPrefix(remote, ".") ||
strings.HasPrefix(remote, "~") {
return true
}
if len(remote) >= 3 && unicode.IsLetter(rune(remote[0])) && remote[1] == ':' && (remote[2] == '/' || remote[2] == '\\') {
return true
}
if strings.Contains(remote, "@") && strings.Contains(remote, ":") {
return true
}
return false
}
// IsNamedRemote reports whether remote is shaped like a remote NAME (such as
// "origin", "upstream", or "team/upstream") rather than a direct URL
// (https://..., git@...) or a file path (/path/to/repo, C:\path\to\repo).
// It does not consult git config, so a name that is not configured (a typo)
// still reports true; the subsequent git command surfaces that failure.
// ctx, cwd, and runGit are accepted for signature stability with the other
// remote helpers and are unused.
func IsNamedRemote(ctx context.Context, cwd, remote string, runGit Runner) bool {
remote = strings.TrimSpace(remote)
if remote == "" || isRemoteURLOrPath(remote) {
return false
}
return true
}
// UpstreamRemoteAndMergeBranch returns the configured upstream remote and merge branch
// for branch from git config branch.<name>.remote and branch.<name>.merge.
func UpstreamRemoteAndMergeBranch(ctx context.Context, cwd, branch string, runGit Runner) (remote string, mergeBranch string) {
runGit, _ = resolveRunners(runGit, nil)
branch = strings.TrimSpace(branch)
if branch == "" {
return "", ""
}
r, _ := gitOutput(ctx, runGit, cwd, "config", "branch."+branch+".remote")
m, _ := gitOutput(ctx, runGit, cwd, "config", "branch."+branch+".merge")
remote = strings.TrimSpace(r)
mergeBranch = strings.TrimPrefix(strings.TrimSpace(m), "refs/heads/")
return remote, mergeBranch
}
// RemoteHasBranch reports whether remote already has refs/heads/<branch>.
// ensureFeatureBranch uses this on every non-default-branch push (not just
// when local upstream config is missing or wrong): a prior push may have
// published the branch even when push -u failed to write .git/config, and
// the nonexistence lease must not be reasserted against a ref that already
// exists on the remote.
//
// This is existence-only, not tip equality. ensureFeatureBranch already
// covers the concurrent-creator race this could be mistaken for protecting:
// when the branch exists, it drops to a plain push and relies on Git's own
// fast-forward check as the safety net (see
// TestRunChangesPushDropsLeaseWhenBranchExistsOnRemote), so a survivor of a
// branch-name race with unrelated history is rejected there, not here. A
// tip-equality check here instead breaks the ordinary case: after publishing
// once, a second local commit makes HEAD outrun the remote tip, so this
// would report false, Push would reassert the empty-value
// force-with-lease=<branch>: (branch must not exist), and Git would reject
// the plain follow-up push with a stale-info error against a ref that
// plainly exists.
func RemoteHasBranch(ctx context.Context, cwd, remote, branch string, runGit Runner) (bool, error) {
runGit, _ = resolveRunners(runGit, nil)
remote = strings.TrimSpace(remote)
branch = strings.TrimSpace(branch)
if remote == "" || branch == "" {
return false, nil
}
// Ask for the exact head ref rather than listing every branch. "--"
// terminates option parsing so a remote shaped like an option stays
// positional.
ref := "refs/heads/" + branch
out, err := gitOutput(ctx, runGit, cwd, "ls-remote", "--heads", "--", remote, ref)
if err != nil {
return false, err
}
for _, line := range strings.Split(out, "\n") {
line = strings.TrimSpace(line)
if line == "" {
continue
}
if _, got, ok := strings.Cut(line, "\t"); ok && strings.TrimSpace(got) == ref {
return true, nil
}
}
return false, nil
}
// UpstreamRef returns the full upstream reference name for branch (e.g.
// "origin/feature-1" or "upstream/main"), or the empty string if no upstream or
// the ref cannot be resolved. Callers that need to know whether Zero's own
// `push -u` published exactly <remote>/<branch> compare this string rather than
// reading branch.<name>.remote alone: with branch.autoSetupMerge=inherit,
// `git checkout -b` copies remote=origin and merge=refs/heads/main from the
// source branch before any push, so a remote config field is not publication
// state.
func UpstreamRef(ctx context.Context, cwd, branch string, runGit Runner) string {
runGit, _ = resolveRunners(runGit, nil)
branch = strings.TrimSpace(branch)
if branch == "" {
return ""
}
out, err := gitOutput(ctx, runGit, cwd, "rev-parse", "--abbrev-ref", branch+"@{upstream}")
if err != nil {
return ""
}
return strings.TrimSpace(out)
}
// HasUpstream reports whether branch has a published upstream tracking the same
// branch name on a remote (the relationship `git push -u <remote> <branch>`
// records). ensureFeatureBranch consults this when it is called again with a
// non-default current branch: a generated branch that just lost a
// force-with-lease race against a concurrent creator is left checked out
// locally without that relationship, so a retry must not treat it the same as
// an ordinary, already-published feature branch and drop the nonexistence
// lease.
//
// An inherited upstream to a different branch (branch.autoSetupMerge=inherit
// copies origin/main onto a new user/slug) is not publication state and reports
// false. Any failure to resolve the upstream (including "no upstream
// configured") also reports false so the caller keeps requiring the lease.
func HasUpstream(ctx context.Context, cwd, branch string, runGit Runner) (bool, error) {
branch = strings.TrimSpace(branch)
if branch == "" {
return false, nil
}
ref := UpstreamRef(ctx, cwd, branch, runGit)
if ref == "" {
return false, nil
}
// rev-parse --abbrev-ref prints "<remote>/<branch>". Both remote names
// (e.g. team/upstream) and branch names (e.g. user/slug) may contain
// slashes, so verify that the upstream ref ends with "/" + branch and
// has a non-empty remote prefix.
suffix := "/" + branch
if strings.HasSuffix(ref, suffix) && len(ref) > len(suffix) {
return true, nil
}
return false, nil
}
// UpstreamRemote returns the configured upstream remote name for branch (e.g. "origin"),
// or "" if no upstream is configured. This alone is not proof of publication:
// branch.autoSetupMerge=inherit copies branch.<name>.remote from the source
// branch before any push. Prefer UpstreamRef when checking that <remote>/<branch>
// was actually published.
func UpstreamRemote(ctx context.Context, cwd, branch string, runGit Runner) string {
runGit, _ = resolveRunners(runGit, nil)
out, err := gitOutput(ctx, runGit, cwd, "config", "branch."+branch+".remote")
if err != nil {
return ""
}
return strings.TrimSpace(out)
}
// DeleteBranch switches to fallbackBranch and deletes branchToDelete.
func DeleteBranch(ctx context.Context, cwd, fallbackBranch, branchToDelete string, runGit Runner) error {
runGit, _ = resolveRunners(runGit, nil)
if _, err := gitOutput(ctx, runGit, cwd, "switch", "--", fallbackBranch); err != nil {
return err
}
_, err := gitOutput(ctx, runGit, cwd, "branch", "-D", "--", branchToDelete)
return err
}
// CurrentBranchTip returns the SHA of the currently checked-out commit
// (HEAD^{commit}), or "" when HEAD is detached or unresolvable.
// ensureFeatureBranch uses this as the compare-and-swap old value when it
// restores the original default branch after creating a feature branch, so a
// concurrent advance of the default ref is refused instead of rewound.
func CurrentBranchTip(ctx context.Context, cwd string, runGit Runner) string {
runGit, _ = resolveRunners(runGit, nil)
out, err := gitOutput(ctx, runGit, cwd, "rev-parse", "--verify", "HEAD^{commit}")
if err != nil {
return ""
}
return strings.TrimSpace(out)
}
// CurrentBranch returns the short name of the currently checked-out branch, or
// "" when HEAD is detached / unresolvable. Callers that need the branch's
// configured upstream (for example the --yes unborn-remote preflight) use this
// instead of IsDefaultBranch so a remote-HEAD lookup failure does not block
// remote resolution.
func CurrentBranch(ctx context.Context, cwd string, runGit Runner) string {
runGit, _ = resolveRunners(runGit, nil)
out, err := gitOutput(ctx, runGit, cwd, "rev-parse", "--abbrev-ref", "HEAD")
if err != nil {
return ""
}
branch := strings.TrimSpace(out)
if branch == "" || branch == "HEAD" {
return ""
}
return branch
}
// ErrCompareAndSwapConflict reports that a ref changed since the expected tip
// was captured, so the compare-and-swap form of update-ref refused to move it.
// Callers distinguish this from an ordinary update-ref failure because it means
// someone else advanced the ref concurrently rather than that the write itself
// is broken, and destroying work to "recover" from it would be wrong.
var ErrCompareAndSwapConflict = errors.New("compare-and-swap conflict")
// checkedOutInWorktrees reports whether branch is currently checked out in any
// worktree across the repository, returning the path of the worktree if found.
func checkedOutInWorktrees(ctx context.Context, cwd, branch string, runGit Runner) (bool, string, error) {
out, err := gitOutput(ctx, runGit, cwd, "worktree", "list", "--porcelain")
if err != nil {
return false, "", err
}
var currentWorktree string
targetRef := "refs/heads/" + branch
for _, line := range strings.Split(out, "\n") {
line = strings.TrimSpace(line)
if strings.HasPrefix(line, "worktree ") {
currentWorktree = strings.TrimPrefix(line, "worktree ")
} else if strings.HasPrefix(line, "branch ") {
b := strings.TrimPrefix(line, "branch ")
if b == targetRef || b == branch {
return true, currentWorktree, nil
}
}
}
return false, "", nil
}
// ResetBranchRef points the local branch ref at newTip without checking the
// branch out. ensureFeatureBranch uses this after creating a feature branch
// that owns the publishable commits: the normal flow commits on the default
// branch first, then creates user/slug at the same HEAD, and without this
// restore the local default keeps those commits and diverges from the remote
// after a squash-merge. newTip is typically the remote-tracking ref
// (origin/main) that CommitsAhead already used. Refuses to move the currently
// checked-out branch so callers must have already switched to the feature
// branch.
func ResetBranchRef(ctx context.Context, cwd, branch, newTip string, runGit Runner, expectedOld ...string) error {
runGit, _ = resolveRunners(runGit, nil)
branch = strings.TrimSpace(branch)
newTip = strings.TrimSpace(newTip)
if branch == "" {
return fmt.Errorf("branch name required")
}
if newTip == "" {
return fmt.Errorf("new tip required")
}
// Branch becomes refs/heads/<branch>; reject traversal / absolute forms so
// a hostile branch name cannot escape the heads namespace.
if strings.Contains(branch, "..") || strings.HasPrefix(branch, "/") || strings.ContainsAny(branch, "\\ \t\n") {
return fmt.Errorf("invalid branch name %q", branch)
}
if inUse, wtPath, wtErr := checkedOutInWorktrees(ctx, cwd, branch, runGit); wtErr == nil && inUse {
return fmt.Errorf("refusing to move currently checked-out branch %q (checked out in worktree %s)", branch, wtPath)
} else if wtErr != nil {
headBranch, err := gitOutput(ctx, runGit, cwd, "rev-parse", "--abbrev-ref", "HEAD")
if err == nil && strings.TrimSpace(headBranch) == branch {
return fmt.Errorf("refusing to move currently checked-out branch %q", branch)
}
}
tipSHA, err := gitOutput(ctx, runGit, cwd, "rev-parse", "--verify", newTip+"^{commit}")
if err != nil {
return fmt.Errorf("resolve tip %q: %w", newTip, err)
}
args := []string{"update-ref", "refs/heads/" + branch, strings.TrimSpace(tipSHA)}
if len(expectedOld) > 0 && strings.TrimSpace(expectedOld[0]) != "" {
args = append(args, strings.TrimSpace(expectedOld[0]))
}
if _, err := gitOutput(ctx, runGit, cwd, args...); err != nil {
// With an expected-old value this is a compare-and-swap: the only way
// git refuses is that the ref no longer matches, so report it as a
// conflict the caller can recognize rather than a generic write error.
if len(expectedOld) > 0 && strings.TrimSpace(expectedOld[0]) != "" {
return fmt.Errorf("%w: update-ref refs/heads/%s: %v", ErrCompareAndSwapConflict, branch, err)
}
return fmt.Errorf("update-ref refs/heads/%s: %w", branch, err)
}
return nil
}
// IsUnbornRemote reports whether remote is a freshly created repository with
// no refs at all (no branches, no HEAD). ensureFeatureBranch consults this
// when CommitsAhead fails to determine why: a genuinely empty remote has no
// <remote>/<branch> tracking ref for CommitsAhead to diff against, and that
// is proof there is nothing published yet, not an unknown state to fail
// closed on. An error here (unreachable remote, timeout) leaves the state
// unconfirmed, so callers must treat that the same as a non-empty remote.
func IsUnbornRemote(ctx context.Context, cwd, remote string, runGit Runner) (bool, error) {
runGit, _ = resolveRunners(runGit, nil)
// "--" terminates option parsing so a remote value shaped like an option
// (--upload-pack=/bin/echo) reaches Git as a positional argument.
out, err := gitOutput(ctx, runGit, cwd, "ls-remote", "--heads", "--", remote)
if err != nil {
return false, err
}
return strings.TrimSpace(out) == "", nil
}
// currentUser is an indirection over os/user.Current so tests can force the
// final fallback tier of CurrentGitUser without depending on the host account.
var currentUser = user.Current
// CurrentGitUser resolves an identity to prefix generated branch names with:
// git config user.name, falling back to the OS account username, falling
// back to the literal "user" so BuildBranchName always gets a non-empty
// input.
func CurrentGitUser(ctx context.Context, cwd string, runGit Runner) string {
runGit, _ = resolveRunners(runGit, nil)
if name, err := gitOutput(ctx, runGit, cwd, "config", "user.name"); err == nil && name != "" {
return name
}
if u, err := currentUser(); err == nil && u.Username != "" {
return u.Username
}
return "user"
}
// slugComponentRe matches runs of characters not allowed in a branch-name
// component, so SlugifyBranchComponent can collapse them to a single hyphen.
var slugComponentRe = regexp.MustCompile(`[^a-z0-9]+`)
// maxSlugComponentLen caps a single branch-name component so generated names
// stay short and readable, matching the "username/feature-name" convention
// rather than sprawling into a full sentence.
const maxSlugComponentLen = 40
// SlugifyBranchComponent lowercases s and collapses any run of non
// alphanumeric characters into a single hyphen, trimming leading/trailing
// hyphens and capping length so the result is a safe, short branch-name
// component.
func SlugifyBranchComponent(s string) string {
slug := slugComponentRe.ReplaceAllString(strings.ToLower(strings.TrimSpace(s)), "-")
slug = strings.Trim(slug, "-")
if len(slug) > maxSlugComponentLen {
slug = strings.Trim(slug[:maxSlugComponentLen], "-")
}
return slug
}
// BuildBranchName composes a "<user>/<slug>" branch name from a git identity
// and a short feature slug (the convention used across Gitlawb tooling).
// Empty or unsafe inputs fall back to "user" and "changes" respectively so
// the result is always a valid, non-empty branch name.
func BuildBranchName(gitUser, slug string) string {
userSlug := SlugifyBranchComponent(gitUser)
if userSlug == "" {
userSlug = "user"
}
featureSlug := SlugifyBranchComponent(slug)
if featureSlug == "" {
featureSlug = "changes"
}
return userSlug + "/" + featureSlug
}
type PROptions struct {
File diff suppressed because it is too large. Load diff