mirror of
https://github.com/Twigpine/zero.git
synced 2026-11-09 18:08:31 -10:00
fix/opencode-session-header
13
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 KRATOS
|
faec8ee201 |
feat(acp): emit structured file changes in tool updates (#999)
* feat(tools): retain bounded structured file diffs * feat(acp): emit structured file changes in tool updates * fix(acp): harden structured file diff transport * fix(tools): reject unsafe rich diff previews * fix(acp): preserve safe rich diff semantics * fix(tools): bind structured diffs to committed changes * fix(acp): preserve structured diff identity * fix(tools): close rich diff race gaps * fix(tools): report partial patch paths from workspace * fix(tools): finalize structured diff evidence * fix(tools): separate unverified patch writes * test(tools): cover failed formatter recovery * fix(acp): serialize file deletions as nullable diffs * fix(tools): recreate files after formatter failure * test(acp): require nullable diff fields on wire |
||
|
KRATOS
|
c1937dfac7 |
feat(acp): add standard session list and resume (#914)
* feat(acp): add standard session list and resume
Origin-Session: local-abff1c | Claude Code | 7 prompts
Origin-Snapshot: b7d0806d49f9
Origin-Session: local-13d543 | Claude Code | 5 prompts
Origin-Snapshot: 6b5eb8ba4e5b
Origin-Session: local-c962d7 | Claude Code | 3 prompts
Origin-Snapshot: d2b8f44a9abc
* fix(acp): preserve notification wire order
Origin-Session: local-abff1c | Claude Code | 7 prompts
Origin-Snapshot: b7d0806d49f9
Origin-Session: local-13d543 | Claude Code | 5 prompts
Origin-Snapshot: 6b5eb8ba4e5b
Origin-Session: local-c962d7 | Claude Code | 3 prompts
Origin-Snapshot: d2b8f44a9abc
* fix(acp): bind sessions to persisted workspaces
Origin-Session: local-abff1c | Claude Code | 7 prompts
Origin-Snapshot: b7d0806d49f9
Origin-Session: local-13d543 | Claude Code | 5 prompts
Origin-Snapshot: 6b5eb8ba4e5b
Origin-Session: local-c962d7 | Claude Code | 3 prompts
Origin-Snapshot: d2b8f44a9abc
* fix(acp): compare workspaces by filesystem identity, not by spelling
@Vasanthdev2004's three findings, all reproduced before changing anything.
TWO SPELLINGS OF ONE DIRECTORY WERE TWO WORKSPACES. Both comparisons were string
equality on ResolveWorkspaceRoot output, and that resolver is abs plus
filepath.Clean plus a stat — it does not fold case and does not resolve
junctions. A session persisted from the TUI was unresumable from an editor
holding a different spelling of the same project folder, and session/list
filtered by the other spelling returned nothing, which makes it an invisible
failure rather than a reported one. It lands on exactly the case this feature
exists for, and on the two processes most likely to disagree about spelling.
os.SameFile asks the filesystem which directories these are, which is the
question. filepath.EvalSymlinks is NOT the fix on Windows — it normalises a
drive letter and returns a junction path unchanged, so the alias survives it,
and junctions need no privilege. String equality stays as the fast path, and a
stat failure falls back to it rather than widening the match: this gate refuses
access to another workspace's files and configuration, so an unanswerable
comparison denies.
THE TEST COULD NOT SEE ANY OF IT. testDeps resolves with the identity function,
so the guard degenerated to "are these two strings different" fed two unrelated
temp directories — it could only ever answer yes. The rejection direction was
pinned and the acceptance direction was asserted nowhere. The new test uses a
resolver reproducing the production normalisation and drives the ACCEPTANCE
direction through an alias, skipping if the filesystem folds the alias away so
it never passes vacuously. Reverting to string equality fails it three ways:
load, resume, and a list that returns zero.
THE WIRE KEYS ARE AN EXTERNAL CONTRACT. Nothing pinned sessionId, cwd, title,
updatedAt, _meta, modelId, createdAt, sessions, nextCursor, cursor, loadSession,
promptCapabilities, sessionCapabilities, list or resume, so renaming a Go field
would break every client and leave the suite green. Renaming modelId to model_id
now fails.
Origin-Session: local-79d7a0 | Claude Code | 5 prompts
Origin-Snapshot: c175cabb9d50
Origin-Session: local-13d543 | Claude Code | 5 prompts
Origin-Snapshot: 6b5eb8ba4e5b
Origin-Session: local-c962d7 | Claude Code | 3 prompts
Origin-Snapshot: d2b8f44a9abc
* fix(acp): test the alias guard where the bug lives, and stop listing unusable sessions
THE TEST COULD NOT RUN ON THE PLATFORM THE BUG IS FROM. @Vasanthdev2004's point,
and he is right that it is not a nit. The alias test built its second name with
os.Symlink, which needs a privilege an ordinary Windows session does not hold, so
it SKIPPED there — and Windows is where junctions exist and where this defect came
from. The identity guard was verified by CI on the two platforms that never had
the problem. It now builds the alias with mklink /J on Windows, which needs no
privilege and is how he found the defect in the first place, and keeps the symlink
arm elsewhere.
This is the second time in this series a correct fix shipped with a test that
could not exercise it: the same helper shape was added to internal/memory for the
same reason a day earlier.
A SESSION WITH NO PERSISTED WORKSPACE IS NOT RESUMABLE, SO IT IS NOT LISTED.
CodeRabbit's finding. activatePersistedSession refuses an empty Cwd, but the
listing advertised it anyway — a menu entry that only fails when taken. The test
asserts both halves, because the listing is only correct relative to what resume
will accept: the omitted session is checked to really fail on resume, and a usable
session is checked to survive the filter.
Mutation-checked: removing the filter advertises the unusable session again.
Origin-Session: local-13d543 | Claude Code | 5 prompts
Origin-Snapshot: 6b5eb8ba4e5b
Origin-Session: local-c962d7 | Claude Code | 3 prompts
Origin-Snapshot: d2b8f44a9abc
* fix(acp): resolve every listed workspace, not only when a filter was supplied
@anandh8x's P1, both halves reproduced before changing anything.
The loop resolved item.Cwd only when a cwd filter was present, and skipped only a
blank one. Two shapes stayed on the menu that session/resume then refuses:
- a session whose persisted workspace has since been deleted, advertised as
resumable
- a legacy entry holding a relative path, reported as cwd "." although ACP
requires SessionInfo.cwd to be absolute
Every entry is now resolved unconditionally, anything that cannot resolve is
omitted, and the RESOLVED root is what SessionInfo carries — absolute as the
contract requires, and the same value the client hands back on resume. The
optional identity filter then applies to the resolved roots, which is also where
it belonged.
Listing is a menu: activatePersistedSession resolves and refuses what it cannot
reach, so anything this loop cannot resolve is something a client would be
offered and then denied.
Mutation-checked: restoring the resolve-only-when-filtered shape re-advertises
the deleted workspace and re-emits the relative cwd.
Origin-Session: local-76c8d7 | Claude Code | 6 prompts
Origin-Snapshot: 259b715cf0fd
Origin-Session: local-13d543 | Claude Code | 5 prompts
Origin-Snapshot: 6b5eb8ba4e5b
Origin-Session: local-c962d7 | Claude Code | 3 prompts
Origin-Snapshot: d2b8f44a9abc
* test(acp): assert the relative entry is retained, not merely absolute
CodeRabbit's catch, and the test was genuinely weaker than it looked. It checked
that the deleted workspace was gone, the live one kept, and every listed cwd
absolute — all of which a "fix" that simply DISCARDED any non-absolute entry
would satisfy, while losing a resumable session.
Presence is now asserted separately from spelling: the relative entry must still
be listed, and listed with an absolute path.
Mutation-checked: skipping non-absolute entries instead of resolving them now
fails with "a session with a resolvable relative workspace was dropped rather
than normalised". The first attempt at that mutation did not compile, so it
proved nothing until it was rewritten — worth saying, because a mutation that
fails to build looks exactly like a test that passes.
Origin-Session: local-13d543 | Claude Code | 5 prompts
Origin-Snapshot: 6b5eb8ba4e5b
Origin-Session: local-c962d7 | Claude Code | 3 prompts
Origin-Snapshot: d2b8f44a9abc
* fix(acp): a relative persisted workspace is omitted, not rebased
Reported by @anandh8x. Resolving a stored relative cwd does not recover the
session's workspace, it invents one: ResolveWorkspaceRoot joins it against
whatever directory the ACP server happens to be running in, and that invented
absolute path was then advertised as the session's workspace and accepted as
its home on resume.
Reproduced on f2c6fc9a, a session persisted with cwd ".":
LISTED legacy-rel as cwd="/Users/kratos/dev/f914/internal/acp"
resume with an UNRELATED workspace -> err=... cwd does not match its persisted workspace
resume with NO cwd (falls back to ".") -> err=<nil>
The mismatch check does its job when the client names a workspace, so the only
opening was the fallback path, where the rebased value was compared against
itself and always agreed. A conversation created for one project could be
resumed against another project's files, configuration and tools.
Both doors now take the same guard: handleSessionList omits an entry whose
persisted cwd is not absolute, and activatePersistedSession refuses one rather
than resolving it. The original base is not knowable from the metadata, so
guessing at it is not an option a fix can take.
This reverses an earlier assertion in TestSessionListResolvesEveryWorkspace,
which expected the relative entry to be normalised and retained. That was
requested in review on the grounds that dropping it loses a resumable session.
It does, but the entry was never resumable into its own workspace, only into
this process's. The test now asserts it is dropped, and a new
TestResumeRefusesARelativePersistedWorkspace covers the fallback path that the
listing filter alone leaves open.
Both guards mutation-checked: removing either one fails its test.
Pre-existing on this branch and on its merge-base, unrelated to this change:
TestRunDoctorFormatsRedactedProviderDiagnostics and
TestRunDoctorConnectivityProbesProvider both exit 3 in this environment.
Origin-Session: local-8cd239 | Claude Code | 11 prompts
Origin-Snapshot: 365efe3045f2
Origin-Session: local-13d543 | Claude Code | 5 prompts
Origin-Snapshot: 6b5eb8ba4e5b
Origin-Session: local-c962d7 | Claude Code | 3 prompts
Origin-Snapshot: d2b8f44a9abc
* test(acp): the resume guard is asserted through both activating methods
Raised by CodeRabbit: the test is named for resume and called session/load.
session/load and session/resume are separate entry points that today share
activatePersistedSession, so an assertion through either one passes while the
guard holds — but the name promised a surface it was not touching. Both are now
named explicitly, which keeps that true: if resume is ever given its own path,
this fails rather than quietly covering half of what it claims to.
With the guard removed, both methods accept a relative persisted workspace on
the no-cwd fallback. The named-workspace case was already refused by the
existing mismatch check; the fallback was the only door open, and it is open on
both.
Not taken in this PR, from the same review: threading a rooted directory handle
through ResolveWorkspaceRoot and workspace construction so a root rename or link
swap cannot redirect later file operations. That is a real question and a
pre-existing one — this change adds a refusal and no path handling — but it is a
capability refactor across workspace and tool access with its own race test, not
something to fold into a session-list fix. Worth its own issue.
Origin-Session: local-8cd239 | Claude Code | 11 prompts
Origin-Snapshot: 365efe3045f2
Origin-Session: local-13d543 | Claude Code | 5 prompts
Origin-Snapshot: 6b5eb8ba4e5b
Origin-Session: local-c962d7 | Claude Code | 3 prompts
Origin-Snapshot: d2b8f44a9abc
* fix(acp): activation requires an absolute cwd in the REQUEST, not just the record
Reported by @jatmn.
ResumeSessionParams is a type alias for LoadSessionParams, so JSON decoding turns
an OMITTED resume cwd into an empty string. That blank reached the shared
activation path, whose blank-cwd fallback substitutes meta.Cwd — so
{"sessionId":"known"} activated a persisted session, even though ACP v1 requires
session/resume to carry an absolute working directory.
This is a different hole from the persisted-cwd one fixed earlier on this branch.
That guard asks whether the STORED workspace is identifiable; this asks whether
the CALLER named one at all. The earlier fix does not cover it, because a stored
absolute cwd passes that check and the blank request then silently inherits it.
requestedWorkspace validates the request's own cwd before anything reaches the
fallback: absent, empty and whitespace-only are all invalid params, and so is a
relative path. Applied to both activating methods rather than to resume alone —
load's omitted-cwd fallback was inheriting the same way, and leaving one door
open is how this class survived the last fix.
Mutation-checked at the wire, which is where the defect lives: making the blank
case return no error compiles and fails the regression on four separate requests
— session/load and session/resume, each with cwd omitted and with cwd blank. A
Go-level test would not have caught it, since the defect is in decoding an absent
field.
Two notes from a verification pass, neither a defect:
Error precedence changed: a blank cwd with an UNKNOWN session id now reports the
cwd problem instead of "session not found". Kept deliberately and pinned — it
stops the server confirming whether a session exists to a request that named no
workspace.
AdditionalDirectories is declared on two params structs and consumed nowhere in
the repo. Not a hole today, but it is the same shape — client-supplied paths with
no absoluteness rule — so the field now carries a note saying it must go through
requestedWorkspace when wired up.
Rebased onto
|
||
|
KRATOS
|
7f5e5af7bb |
fix(acp): make configured MCP tools callable (#1003)
* fix(acp): register configured MCP tools per turn * fix(acp): address MCP review findings |
||
|
KRATOS
|
a6ec0435e3 |
feat(acp): surface safe browser tool metadata (#1000)
* feat(acp): surface safe browser tool metadata * fix(acp): align browser permission titles * fix(acp): namespace browser metadata * fix(acp): reject unsafe browser title text |
||
|
Vasanth T
|
a78c6c357e |
fix(acp): three defects a desktop ACP client hits (#915)
* 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. * 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. * 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. |
||
|
Vasanth T
|
2ec5c3ae6b |
fix(agent): tell the user a prefix approval leaves the sandbox (#885)
Approving a command prefix does not only stop the prompt from reappearing. shellExecutionArgsForApproval rewrites the call to require_escalated, which resolves to a nil sandbox engine, so the command runs unsandboxed on the host. The engine's escalation gate is then satisfied by the PermissionGranted flag from the approval the user just gave for the sandboxed form. The escalation itself is deliberate and guarded: proposedCommandPrefix refuses to offer a prefix when any other segment of the command is not known-safe, precisely because approval covers the whole command. The defect is that the prompt never said so. The user chose "allow command prefix for session" and got "and also run it outside the sandbox". So this discloses rather than removes. PermissionRequest gains PrefixApprovalEscalates, computed by shellPrefixApprovalEscalates from the same guards the rewrite consults, and the two prefix options append "(runs outside the sandbox)" when it is set. Both the TUI and the ACP path carry it, since an editor user decides on the same terms as a terminal user. The main test asserts the disclosure and the rewrite agree on every input rather than checking each against a fixed expectation, so the two cannot drift apart later. Verified by mutation: reverting the helper to always-false fails it, as does making the label unconditional. |
||
|
|
0176ee49ff |
feat(agent): add PermissionModePlan for interactive read-only planning (#853)
* feat(agent): add PermissionModePlan for interactive read-only planning * fix(agent): drop redundant modeName branch, add plan mode regression tests string(permissionMode) already yields "plan" / "spec-draft", so the if/else recomputing modeName in the denial message was dead branching on the same values. Also add plan-mode coverage mirroring three of the four existing spec-draft regression tests: advertised tool set, and denied write_file/bash calls. The fourth (submit-and-stop review control) has no plan-mode analog, since plan mode has no submit tool. * fix(agent): deny request_permissions in plan/spec-draft even when the registry omits it request_permissions is dispatched by name in executeToolCall before the registry-based ToolAdvertised gate runs, so that gate only helps when the tool happens to be present in the caller's registry. A plan- or spec-draft-mode registry that simply omits the tool (rather than registering it as denied) let the call fall through to a real turn/session-scoped permission grant, defeating the read-only boundary. Deny it unconditionally at the top of executeRequestPermissions for both read-only modes instead. Co-Authored-By: Claude Sonnet 5 <[email protected]> * fix(agent): suppress executable hooks while plan/spec-draft mode is active Plan mode promises a read-only turn, but sessionStart/sessionEnd fire on every run and beforeTool/afterTool fire around allowed read calls, and all four execute configured host commands outside the advertised-tool and sandbox gates — so a project hook could mutate the workspace or spawn a process from a session that advertises it cannot. Gate all four dispatch points on the run's permission mode, with a regression test asserting no hook command launches during a plan-mode run. Co-Authored-By: Claude Fable 5 <[email protected]> * fix(agent): close plan-mode tool advertisement bypass toolAdvertisedInPlan whitelisted ask_user and update_plan by name alone, so a caller could register a mutating tool under either name and have it advertised and executed in plan mode. Validate every tool against its Safety() instead. Also exclude lsp_navigate, which is marked SideEffectRead but lazily spawns a real language-server process, contradicting plan mode's read-only guarantee. Add regression tests for both. * fix(agent): keep trust-gated hooks in spec-draft mode Plan mode still suppresses executable hooks so a read-only planning turn cannot spawn host processes via session or tool hooks. Spec-draft keeps the existing trust model: project hooks fire when the workspace (or its worktree trust root) is trusted. Unconditionally suppressing hooks in spec-draft broke TestExecSpecWorktreeInheritsTrustEndToEnd for trusted worktrees under --use-spec --worktree. * fix(agent): cover plan-mode spoof and lsp_navigate denials Expand the name-only spoof regression to both update_plan and ask_user, and add an execution-path denial for lsp_navigate so plan mode cannot spawn language servers even when a call still arrives. * fix(agent): filter tool_search deferred candidates by plan/spec-draft visibility tool_search resolved and ranked deferred tools by EnabledTools/DisabledTools only, never by the run's permission-mode visibility. tool_search itself is already denied at dispatch in plan/spec-draft (its Safety carries no side effect, so it fails the same SideEffect==Read advertisement gate direct calls use), so this was not reachable through the normal Run() path today. But it is a real landmine: if that outer gate is ever loosened independently (e.g. tool_search's no-side-effect Safety is judged advertisable), the loader had no gate of its own and would hand a deferred write/mutator tool's name, description, and full schema straight to a plan/spec-draft model. Mirror agent.ToolAdvertised's plan/spec-draft branches inside the tools package (toolAdvertisedForPermissionMode, next to the existing toolAllowedByFilters mirror that avoids the same import cycle) and apply it alongside the operator filters in visibleDeferredTools and visibleEagerToolNames. Added unit tests in tool_search_test.go for both modes, and an end-to-end agent test that force-calls tool_search in plan mode and asserts no schema leaks. Verified by reverting tool_search.go and confirming the new tests fail (one shows load_tools resolving to the mutator's name); restored and confirmed they pass. Also confirmed via a temporary probe that if plan mode's outer advertisement gate is loosened, this filter is what actually stops the leak. * fix(agent): require Safety for spec-draft ask_user/submit_spec Do not advertise or load re-registered control tools by name alone in spec-draft mode. ask_user must be SideEffectRead+Allow and submit_spec must be SideEffectWrite+Allow, matching the real tools. Apply the same filter in tool_search and add spoof regression tests. Refs Gitlawb/zero#642 * fix(agent): wire plan mode into TUI, CLI, and ACP entry points Address the P1 finding that PermissionModePlan was documented but never selected by /plan, zero exec, or ACP mode selectors. /plan on|off now toggles the session permission mode (restoring the prior mode on off), zero exec --plan selects plan for a run, and ACP advertises plan as a client-selectable mode. Integration coverage for each entry path. * fix(cli): reject --plan combined with --worktree Worktree preparation runs in runExec before the plan permission mode is assigned, so `zero exec --plan --worktree` could still trigger workspace mutation ahead of the read-only gate. Reject the combination during option validation, alongside the existing --use-spec/--skip-permissions- unsafe conflict checks, so no worktree prep can occur. Addresses a coderabbitai finding on PR #642. * test(tools): assert spoofed tool schema doesn't leak via tool_search The spoofed-control-tool regression only asserted on the description string; Parameters() exposed no distinctive schema marker, so a regression that leaked the schema without the description would still have passed. Add a spoofed_secret property to the test tool's schema and assert it's absent from result.Output alongside the description. Addresses a coderabbitai finding on PR #642. * fix(tui): gate local mutating commands behind plan mode /plan on only flips the agent permission mode, which gates agent tool calls. Local TUI commands that run entirely inside the TUI process bypass that gate: /rewind restores workspace files from a checkpoint, /export writes a transcript to disk, and /sandbox-setup spawns a native host process. Add a shared plan-mode guard at the start of dispatchCommand (mirroring the existing BTW-unavailable guard) that rejects these three commands while permissionMode is agent.PermissionModePlan, with regression coverage proving each is blocked with no mutation/process spawn in plan mode and unaffected outside it. Addresses a coderabbitai finding on PR #642. * fix(agent,specialist): layer plan mode system prompt and enforce read-only subagent mode * Fix jatmn review findings for PR 642 * fix(agent,cli,tui): resolve CodeRabbit review comments on active turn model field and --plan permission mode conflict * fix(agent): propagate permission mode to exec options and serialize ACP mode changes Parse permissionMode in resolveExecPermissionMode, acquire turnMu.Lock in ACP handleSetMode to serialize mode changes with active turns, and block MCP subcommands in TUI plan mode. Refs #642 * fix(agent,tui): update tests off removed tools.CoreTools/NewWriteFileTool/NewLSPNavigateTool wrappers Those were thin unscoped wrappers around the Scoped variants, deleted upstream in #706 since nothing else called them directly. Only these tests still did; switch to the Scoped calls main's own tests already use. * fix(agent): fail-closed beforeTool vetoes and permission-mode plan guards Keep beforeTool deny gates active under plan mode so hooksSuppressed no longer fails open, and stop propagating --permission-mode for auto/ask/member children so swarm members keep write tools. Apply --plan combination rejects to --permission-mode plan as well. Refs #853 * fix(agent): address CodeRabbit findings for plan-mode advertisement and entry paths Unify plan/spec-draft tool advertisement in tools.ToolAdvertisedForPermissionMode (with PermissionDeny short-circuit), cover ACP config and --permission-mode plan list-tools paths, and allow bare /mcp while blocking mutating MCP subcommands. Refs #853 --------- Co-authored-by: Claude Sonnet 5 <[email protected]> Co-authored-by: euxaristia <[email protected]> |
||
|
|
15b7a5475c |
fix(acp): interrupt idle reads on cancellation (#782)
* 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]> |
||
|
euxaristiaandClaude Sonnet 5
|
97ef90d27a |
chore: remove code unreachable on every supported platform (#706)
* chore: remove code unreachable on every supported platform The Security and code health job's advisory deadcode step reports 152 unreachable functions on main. Running the same check under GOOS=linux, windows, and darwin shows 13 of those are false positives whose callers live in windows build-tagged files, so only functions unreachable on all three platforms are touched. Functions with no references anywhere are deleted, including the internal/reasoning package, which nothing imports. Functions that only tests reference move into per-package export_test.go files so tests keep their seams while the production binary sheds the code. Test call sites of deleted convenience wrappers are ported to the live APIs (scoped tool constructors, tools.DeferredSource, FinishReason checks). Kept despite the report: cron.Store.Runs (only reader of runs.jsonl, which production writes), and agenteval.TraceEventKeys and MissingTraceEvents (used across packages by the trace contract test). * docs(config): correct ValidateFile's errors.As claim Message is a flattened string (.Error() already called on the parse error), not a wrapped error, so callers can't recover the concrete json error type via errors.As. Fix the comment to match. Co-Authored-By: Claude Sonnet 5 <[email protected]> * chore(trace,skills): drop obsolete Sink adapters and fix stale doc references The dead-code pass removed the Emit methods but left Sink, NDJSONSink, and TextSink exported with a contract nothing implements; remove the types with their contract. Also remove the stray blank line at EOF of trace.go flagged by gofmt, and update skills package docs and HOW_ZERO_WORKS.md to stop directing callers to DiscoveryRoots/Duplicates, which are now test seams. Co-Authored-By: Claude Fable 5 <[email protected]> * chore: clean up remaining review nits on dead code removal Replace a pointless fmt.Errorf(...%w...).Error() with fmt.Sprintf using %v, since the wrapped error was discarded immediately anyway. Remove a comment block that was duplicated verbatim. * docs(skills): drop remaining references to test-only List/Get Load and MergedSkills comments still named List/Get as production entry points after they moved to export_test.go. Point callers at Load and ListFromRoots instead. * chore: remove now-unreferenced export_test.go seams execSessionManager.len() and likelySandboxDenied/sandboxDenialKind had no remaining callers after the sandbox unify-command-execution rebase; the production code they wrapped for tests no longer exists. --------- Co-authored-by: Claude Sonnet 5 <[email protected]> |
||
|
|
ddfdf2878e |
Fix ChatGPT OAuth model discovery and ACP model selection (#724)
* fix: expose authenticated ChatGPT models to ACP Co-authored-by: Pierre Bruno <[email protected]> * fix(acp): persist discovered model selections * fix(acp): normalize model selections --------- Co-authored-by: Amp <[email protected]> |
||
|
euxaristia
|
ddc4927aac |
fix(acp): make truncateHint rune-safe (#614)
truncateHint slices string hints using a raw byte offset (s[:max]), which can split multi-byte UTF-8 characters (like CJK characters, emojis, or accents) in half and produce invalid UTF-8 sequences. Update it to count and slice by runes to guarantee valid UTF-8, and add a unit test TestToolTitleTruncateHintRuneSafe to verify. |
||
|
冯基魁
|
f951aee65d | surface ACP session persistence failures (#535) | ||
|
KRATOS
|
5b348056a4 |
feat(acp): Agent Client Protocol surface — drive ZERO as an editor backend (#288)
* feat(acp): JSON-RPC 2.0 ndjson transport for the ACP surface
Bidirectional JSON-RPC 2.0 peer over stdio (newline-delimited JSON): serves
inbound requests/notifications via registered handlers and issues outbound
requests/notifications. Handlers run on their own goroutines so a long-running
request (session/prompt) never blocks delivery of session/cancel or a permission
response. Stdlib-only, no new dependencies.
Derived solely from the public JSON-RPC 2.0 + ACP specs.
* feat(acp): ACP wire types derived from the public spec
Protocol message types for initialize, session/{new,load,prompt,cancel,update,
set_mode,set_config_option}, request_permission, content blocks, tool-call
updates, plan entries, permission options, and session modes — method names and
field shapes taken from the public ACP spec (agentclientprotocol.com schema/v1).
Model selection is exposed both via the spec's session/set_config_option and a
vendor _zero/set_model alias for clients that prefer it.
* feat(acp): translate ZERO agent events to ACP session/update payloads
Pure mappers (unit-tested) from ZERO's agent callbacks to ACP updates: text->
agent_message_chunk, reasoning->agent_thought_chunk, tool call->tool_call(in_progress),
tool result->tool_call_update(completed/failed) with content + changed-file
locations, and plan items->plan entries. A notifier wires the mappers to a live
connection per session.
* feat(acp): map ZERO permission prompts to ACP session/request_permission
Translate ZERO's permission decisions into ACP PermissionOptions (optionId carries
the ZERO action verbatim for a lossless round trip) and map the client's outcome
back to a ZERO PermissionDecision: cancelled->cancel, selected->the chosen action,
anything unrecognized fails closed to deny. Builds the embedded toolCall too.
* feat(acp): ACP Agent server driving ZERO's core
Implements the ACP methods over the JSON-RPC peer: initialize (capability +
version negotiation), session/new, session/load (with history replay for resume),
session/prompt (drives the real agent.Run with streaming callbacks -> session/update,
and an OnPermissionRequest that round-trips through session/request_permission),
session/cancel, session/set_mode (permission/autonomy mode), and model selection
via session/set_config_option plus the vendor _zero/set_model alias. Dependencies
are injected so the editor only hosts the thread while ZERO keeps auth/model/keys
(BYOK). Includes an end-to-end test (initialize+new+prompt over stdio pipes through
agent.Run with a fake provider) asserting the streamed updates.
* feat(acp): wire the 'zero acp' subcommand
Add 'zero acp', which serves ACP over stdio so editors (Zed, JetBrains, Neovim,
...) can drive ZERO as a backend. Reuses ZERO's real deps — config resolution,
provider construction, the core tool registry, and the session store — so
provider/model/keys stay owned by ZERO (BYOK). Registered in the command
dispatch and the help listing.
* fix(acp): drain in-flight handlers on stream close
On stdin EOF the read loop returned before the dispatch goroutines flushed their
responses, so piped/finite ndjson input (and editors that close the stream right
after a request) dropped the reply. Serve now cancels in-flight handlers (so any
blocked outbound Call unblocks via ctx) and waits for them before returning.
* fix(acp): address review — sandbox confinement, mode/cwd/transport hardening
Blockers:
- Confine shell/file tools: runTurn now builds a SCOPED registry + sandbox engine
per session (BuildWorkspace mirrors exec's buildExecSandboxEngine + NewScope) and
passes Options.Sandbox — ACP no longer runs bash/exec_command unconfined.
- Reject PermissionModeUnsafe over ACP (set_mode + advertised modes are auto/ask
only); Unsafe stays gated to the operator, so a client can't self-grant no-prompt
host access.
- Fix data race on acpSession.history: registerSession sets history before
publishing the session and reuses an already-live session instead of overwriting.
- deliver() now deletes the pending entry and sends non-blocking, so a duplicate/
late response frame can't wedge the read loop.
Should-fix:
- Serialize prompt turns per session (turnMu) so concurrent prompts can't interleave
history or clobber the cancel slot.
- Validate client cwd (ResolveWorkspaceRoot mirrors exec; rejects filesystem root +
home) before it becomes the file-tool confinement root.
- Malformed JSON no longer tears down the connection: framing is per-line; a bad
line replies -32700 (id null) and the loop continues. Also validate jsonrpc:"2.0".
- Permission outcome binds to the offered set only (dropped the isKnownDecision
fallback) — a client can't return a broader grant than was presented; fail closed.
- Drop the non-conformant SessionCapabilities advertisement (no resume handler) and
the non-conformant SessionConfigOption model selector; model switching stays
available via the vendor _zero/set_model.
Adds regression tests: sandbox+scoped-registry wiring, invalid-cwd rejection,
Unsafe-mode rejection, and malformed-line resilience. Full go test ./... -race green.
|