mirror of
https://github.com/Twigpine/zero.git
synced 2026-11-11 05:01:36 -10:00
* fix(acp): interrupt idle reads on cancellation * fix(acp): make idle-read interruption per-Read, not per-Serve-call Addresses the P2 on #782. The previous design raced a single, whole-connection Once (endRead) between the read loop's own "I have a terminal result" claim and a watcher goroutine's "ctx is done, treat this as interrupted" claim. This has a real, deterministic failure mode that has nothing to do with goroutine-scheduling luck: bufio.Reader can obtain data AND a terminal error from ONE underlying Read call (io.Reader explicitly permits returning (n>0, err) together). If a line delimiter is found within that data, ReadBytes returns the line with a nil error for that call, but bufio caches the error internally and returns it — without ever touching the underlying reader again — on the very next ReadBytes call. If cancellation happens in the gap between that first, successful read and the next one (e.g. while a synchronously-dispatched handler is still writing its response), the watcher's ctx.Done() case fires and claims the Once BEFORE the read loop ever gets back to a ReadBytes call — so when the cached error then surfaces (with zero contact with the underlying reader, let alone anything closing it), it is misattributed as a cancellation-driven interruption and silently swallowed as a clean shutdown, hiding a genuine broken transport. The fix moves interruption-detection down to the Read call itself instead of bracketing the whole Serve loop. interruptibleReader wraps the connection's reader: each Read runs the underlying call on its own goroutine and races it against ctx.Done(). If the underlying call finishes first, its result is returned untouched — cancellation a moment later has no bearing on a call that already had its answer. If ctx.Done() fires first, the reader is closed (when closable) to nudge a genuinely blocked read, a generation counter is bumped, and the call still waits for and returns the underlying call's own result rather than a synthesized one. Serve then brackets each ReadBytes call (which may invoke the wrapped Read zero or more times — zero when bufio answers from its own buffer, including a previously-cached error) with a snapshot of that counter before and after. A read whose bracket shows no change was never raced against cancellation — either answered from the buffer or genuinely completed before ctx.Done() could matter — so its outcome is reported truthfully regardless of the ctx state by the time it surfaces. Only a bracket that changed proves this specific call actually took the cancellation branch. Verified the fix has teeth: reverted to the original implementation and confirmed the new regression test (which forces the exact bufio-caching sequence deterministically, with a panic guard proving the underlying reader is never called a second time) fails 100% of the time against it, then restored the fix and confirmed it passes. Also applied the two open review threads on the existing tests: readStarted and writeStarted are now buffered channels with non-blocking sends rather than close(), so a future change causing an extra read or write can't panic on a double-close. Validated with go build/vet/test on Windows, plus go vet/build and `go test -race ./internal/acp/... -count=50` (and count=100 focused on the cancellation/error tests) under WSL/Linux — this session's Windows host has no C toolchain for -race directly. * fix(acp): preserve decided read results and require explicit reader ownership Addresses jatmn's two #782 review findings: - interruptibleReader.Read's select between resultCh and ctx.Done() chooses pseudo-randomly when both are ready, so a real terminal read error decided just before cancellation could be misattributed to shutdown and silently swallowed. Re-check resultCh non-blockingly before claiming a call for cancellation. - NewConn inferred reader ownership from an io.Closer assertion and closed any reader that happened to implement it, including one a caller merely borrowed. Split into NewConn (never closes r) and NewOwnedConn (explicit ownership, closes r on cancellation), and switch the sole production call site (process stdin) to the latter. Also applies CodeRabbit's atomic.Int64 and leading-ctx-parameter nits. Co-Authored-By: Claude Sonnet 5 <[email protected]> * fix(acp): synchronize read completion with cancellation Amp-Thread-ID: https://ampcode.com/threads/T-019fa018-9e4e-7506-85ab-e151888bfd71 Co-authored-by: Pierre Bruno <[email protected]> * fix(acp): preserve read completion across cancellation Amp-Thread-ID: https://ampcode.com/threads/T-019fa55a-483a-7119-92aa-4953a72f2a76 Co-authored-by: Pierre Bruno <[email protected]> * test(acp): bound buffered error synchronization Amp-Thread-ID: https://ampcode.com/threads/T-019fa55a-483a-7119-92aa-4953a72f2a76 Co-authored-by: Pierre Bruno <[email protected]> * fix(acp): report shutdown transport errors Amp-Thread-ID: https://ampcode.com/threads/T-019fa7a0-1223-701d-9529-48ba5d7cf8c8 Co-authored-by: Pierre Bruno <[email protected]> * test(acp): inject cancellation context Amp-Thread-ID: https://ampcode.com/threads/T-019fa7a0-1223-701d-9529-48ba5d7cf8c8 Co-authored-by: Pierre Bruno <[email protected]> --------- Co-authored-by: Claude Sonnet 5 <[email protected]> Co-authored-by: Amp <[email protected]>
107 lines
4.0 KiB
Go
107 lines
4.0 KiB
Go
package cli
|
|
|
|
import (
|
|
"context"
|
|
"fmt"
|
|
"io"
|
|
"os"
|
|
"path/filepath"
|
|
|
|
"github.com/Gitlawb/zero/internal/acp"
|
|
"github.com/Gitlawb/zero/internal/agent"
|
|
"github.com/Gitlawb/zero/internal/config"
|
|
"github.com/Gitlawb/zero/internal/providermodeldiscovery"
|
|
"github.com/Gitlawb/zero/internal/sandbox"
|
|
"github.com/Gitlawb/zero/internal/tools"
|
|
)
|
|
|
|
const acpUsage = `zero acp — serve the Agent Client Protocol (ACP) over stdio
|
|
|
|
Editors that speak ACP (Zed, JetBrains, Neovim, ...) spawn this command and drive
|
|
ZERO as a backend over JSON-RPC 2.0 on stdin/stdout. ZERO keeps your provider,
|
|
model, and API keys (BYOK); the editor only hosts the conversation thread.
|
|
|
|
Usage:
|
|
zero acp
|
|
|
|
Not meant to be run interactively — point your editor's ACP / external-agent
|
|
setting at "zero acp".`
|
|
|
|
var acpSignalContext = signalContext
|
|
|
|
// runACP serves ACP over stdio so an editor can drive ZERO's agent core. It
|
|
// speaks JSON-RPC 2.0 (newline-delimited JSON) on stdin/stdout; stderr stays free
|
|
// for human-readable diagnostics. The session lifecycle maps onto ZERO's own
|
|
// session store, and provider/model/keys remain owned by ZERO.
|
|
func runACP(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) int {
|
|
for _, arg := range args {
|
|
switch arg {
|
|
case "-h", "--help", "help":
|
|
fmt.Fprintln(stdout, acpUsage)
|
|
return exitSuccess
|
|
default:
|
|
return writeExecUsageError(stderr, fmt.Sprintf("unknown acp flag %q", arg))
|
|
}
|
|
}
|
|
|
|
// The process's own stdin belongs exclusively to this Conn for the life of
|
|
// the command, so cancellation may close it to interrupt an idle read.
|
|
conn := acp.NewOwnedConn(deps.stdin, stdout)
|
|
acp.NewAgent(conn, acp.Deps{
|
|
ResolveConfig: deps.resolveConfig,
|
|
DiscoverModels: func(ctx context.Context, profile config.ProviderProfile) ([]providermodeldiscovery.Model, error) {
|
|
return defaultDiscoverProviderModels(ctx, discoveryCredentialProfile(profile))
|
|
},
|
|
// deps.newProvider is wrapped in fillAppDeps to apply the stored API key,
|
|
// so ACP is authenticated for apiKeyStored profiles like every other
|
|
// surface — no ACP-specific credential handling needed.
|
|
NewProvider: deps.newProvider,
|
|
RunAgent: agent.Run,
|
|
// Build the SCOPED registry + sandbox engine per workspace, exactly like the
|
|
// exec surface, so ACP shell/file tools are confined — never run unconfined.
|
|
BuildWorkspace: func(workspaceRoot string, resolved config.ResolvedConfig) (*tools.Registry, *sandbox.Engine, error) {
|
|
scope, err := sandbox.NewScope(workspaceRoot, resolved.Sandbox.AdditionalWriteRoots)
|
|
if err != nil {
|
|
return nil, nil, err
|
|
}
|
|
engine, err := buildExecSandboxEngine(workspaceRoot, resolved, deps, scope)
|
|
if err != nil {
|
|
return nil, nil, err
|
|
}
|
|
registry := newCoreRegistryScoped(workspaceRoot, scope)
|
|
registerLocalControlTools(registry, workspaceRoot, resolved.LocalControl)
|
|
return registry, engine, nil
|
|
},
|
|
ResolveWorkspaceRoot: acpWorkspaceRootResolver(deps),
|
|
Store: deps.newSessionStore(),
|
|
AgentInfo: acp.Implementation{Name: "zero", Version: version},
|
|
})
|
|
|
|
ctx, stop := acpSignalContext()
|
|
defer stop()
|
|
if err := conn.Serve(ctx); err != nil {
|
|
return writeAppError(stderr, "acp: "+err.Error(), exitCrash)
|
|
}
|
|
return exitSuccess
|
|
}
|
|
|
|
// acpWorkspaceRootResolver validates a client-supplied cwd into a confinement
|
|
// root. It reuses exec's resolveWorkspaceRoot (abs+clean, must be an existing
|
|
// dir) and additionally rejects the filesystem root and the home directory — an
|
|
// editor must not be able to point ZERO's file/shell tools at the whole disk.
|
|
func acpWorkspaceRootResolver(deps appDeps) func(string) (string, error) {
|
|
return func(cwd string) (string, error) {
|
|
root, err := resolveWorkspaceRoot(cwd, deps)
|
|
if err != nil {
|
|
return "", err
|
|
}
|
|
if root == filepath.Dir(root) {
|
|
return "", fmt.Errorf("cwd must not be the filesystem root: %s", root)
|
|
}
|
|
if home, herr := os.UserHomeDir(); herr == nil && home != "" && filepath.Clean(home) == root {
|
|
return "", fmt.Errorf("cwd must not be the home directory: %s", root)
|
|
}
|
|
return root, nil
|
|
}
|
|
}
|