diff --git a/server/pkg/agent/hermes.go b/server/pkg/agent/hermes.go index 826ca5a84b..0995ddeb33 100644 --- a/server/pkg/agent/hermes.go +++ b/server/pkg/agent/hermes.go @@ -594,8 +594,11 @@ func (c *hermesClient) handleLine(line string) { // anything but exactly "allow_once" (acp_adapter/edit_approval.py), so // the previous hardcoded "approve_for_session" silently blocked every // file write on the Hermes ACP runtime (GitHub multica#5300). -// selectACPApprovalOptionID echoes a granting option instead, so this -// works across Hermes' edit and command paths and other ACP backends. +// selectACPApprovalOptionID picks a *safe* granting option the agent +// offered (a known session-scoped id or any single-use allow_once). When +// none is offered it fails closed with a protocol-correct "cancelled" +// outcome rather than fabricate a selection or auto-grant a permanent +// "allow_always" that would persist past the task. func (c *hermesClient) handleAgentRequest(raw map[string]json.RawMessage) { var method string _ = json.Unmarshal(raw["method"], &method) @@ -608,18 +611,35 @@ func (c *hermesClient) handleAgentRequest(raw map[string]json.RawMessage) { var resp map[string]any switch method { case "session/request_permission": - optionID := selectACPApprovalOptionID(raw["params"]) - resp = map[string]any{ - "jsonrpc": "2.0", - "id": json.RawMessage(rawID), - "result": map[string]any{ - "outcome": map[string]any{ - "outcome": "selected", - "optionId": optionID, + if optionID, ok := selectACPApprovalOptionID(raw["params"]); ok { + resp = map[string]any{ + "jsonrpc": "2.0", + "id": json.RawMessage(rawID), + "result": map[string]any{ + "outcome": map[string]any{ + "outcome": "selected", + "optionId": optionID, + }, }, - }, + } + c.cfg.Logger.Debug("auto-approved agent permission request", "method", method, "optionId", optionID) + } else { + // No safe auto-approvable option was offered (empty, reject-only, + // unknown kinds, or only a permanent "allow_always" we refuse to + // auto-select because it persists past this task). Fail closed + // with ACP's "cancelled" outcome instead of fabricating an + // optionId the agent never offered. + resp = map[string]any{ + "jsonrpc": "2.0", + "id": json.RawMessage(rawID), + "result": map[string]any{ + "outcome": map[string]any{ + "outcome": "cancelled", + }, + }, + } + c.cfg.Logger.Warn("no safe auto-approvable permission option offered; cancelling", "method", method) } - c.cfg.Logger.Debug("auto-approved agent permission request", "method", method, "optionId", optionID) default: // Unknown agent→client method — reply with standard "method // not found" so the agent doesn't block waiting for us. Better @@ -649,86 +669,80 @@ func (c *hermesClient) handleAgentRequest(raw map[string]json.RawMessage) { // acpPermissionOption is one entry in a session/request_permission // request's `options` array. `kind` is the ACP-level classification // (allow_once / allow_always / reject_once / reject_always); `optionId` -// is the agent-defined string we echo back to select that option. +// is the agent-defined opaque string we echo back to select that option. type acpPermissionOption struct { OptionID string `json:"optionId"` Kind string `json:"kind"` } -// legacyACPApprovalOptionID is returned only when a request_permission -// carries no usable options at all (a non-conforming or older ACP -// agent). It preserves the id ACP backends relied on before we began -// echoing offered options. -const legacyACPApprovalOptionID = "approve_for_session" +// ACP v1 PermissionOptionKind grant values. Only these two mean "allow"; +// any other or unknown kind is treated as non-granting so a future or +// abnormal kind can never be auto-approved. +// https://agentclientprotocol.com/protocol/v1/schema#permissionoptionkind +const ( + acpKindAllowOnce = "allow_once" + acpKindAllowAlways = "allow_always" +) -// acpApprovalOptionPreference ranks known granting optionIds; lower -// index = preferred. Session scope avoids a per-action round-trip -// without the permanent on-disk allowlist write a permanent grant -// ("allow_always") triggers on Hermes (tools/approval.py's -// save_permanent_allowlist), which would outlive the task and change -// the runtime owner's local Hermes config. Single-use is the safe -// universal fallback. -var acpApprovalOptionPreference = []string{ - "allow_session", // Hermes command approval: session-scoped, not persisted - "approve_for_session", // other ACP agents' session-scoped id - "allow_once", // Hermes edit + command approval: single action - "approve", // other ACP agents' single-action id -} +// acpSessionScopedOptionIDs are optionIds known to grant for the current +// session only, without persisting a decision. Both Hermes' "allow_session" +// and its permanent "allow_always" option carry ACP kind "allow_always" +// (ACP has no session-scoped kind), so kind alone cannot tell them apart — +// we recognise the session-scoped ones by id. "approve_for_session" is the +// equivalent id other ACP backends use. +var acpSessionScopedOptionIDs = []string{"allow_session", "approve_for_session"} -// selectACPApprovalOptionID picks which optionId to return for an -// auto-approved session/request_permission. It only ever returns an id -// the agent actually offered, restricted to granting options (never a -// reject), and prefers session-scoped > single-use > any other grant so -// a permanent "allow_always" is chosen only when it is the sole grant -// offered. Falls back to legacyACPApprovalOptionID when the request -// carries no options. -func selectACPApprovalOptionID(params json.RawMessage) string { +// selectACPApprovalOptionID picks an optionId to auto-approve a +// session/request_permission with, returning ok=false when no *safe* +// granting option was offered. It only ever returns an id the agent +// actually offered and, per review of GitHub multica#5300, deliberately +// refuses to auto-select a permanent "allow_always" grant: on Hermes that +// persists to the runtime owner's on-disk allowlist and would outlive the +// task (ACP v1 allow_always "remembers the choice"). Grant nature is +// decided purely by the explicit ACP kind — never by the opaque optionId — +// and unknown kinds fail closed. Preference: a known session-scoped id, +// then any single-use (kind=allow_once) grant; anything else → ok=false. +func selectACPApprovalOptionID(params json.RawMessage) (string, bool) { var p struct { Options []acpPermissionOption `json:"options"` } if len(params) > 0 { - _ = json.Unmarshal(params, &p) - } - - grants := make([]acpPermissionOption, 0, len(p.Options)) - for _, opt := range p.Options { - if opt.OptionID == "" || isACPRejectOption(opt) { - continue + if err := json.Unmarshal(params, &p); err != nil { + return "", false } - grants = append(grants, opt) - } - if len(grants) == 0 { - return legacyACPApprovalOptionID } - // 1. Preferred known ids, in order. - for _, want := range acpApprovalOptionPreference { - for _, opt := range grants { - if opt.OptionID == want { - return opt.OptionID + // 1. A known session-scoped id, if actually offered with a grant kind. + for _, want := range acpSessionScopedOptionIDs { + for _, opt := range p.Options { + if opt.OptionID == want && isACPGrantKind(opt.Kind) { + return opt.OptionID, true } } } - // 2. Any explicitly single-use grant, to avoid a permanent allowlist write. - for _, opt := range grants { - if strings.EqualFold(strings.TrimSpace(opt.Kind), "allow_once") { - return opt.OptionID + // 2. Any single-use grant. kind=allow_once is inherently scoped to this + // one action, so it is safe regardless of the (opaque) optionId — this + // also covers agents that use non-standard option ids. + for _, opt := range p.Options { + if opt.OptionID != "" && strings.EqualFold(strings.TrimSpace(opt.Kind), acpKindAllowOnce) { + return opt.OptionID, true } } - // 3. First remaining grant (last resort; may be a permanent grant). - return grants[0].OptionID + // 3. No safe grant offered (empty, reject-only, unknown kinds, or only a + // permanent allow_always). Fail closed. + return "", false } -// isACPRejectOption reports whether an option denies rather than grants. -// ACP reject options carry a reject_* kind; we also treat a deny/reject -// optionId as a denial so an agent that omits kind can't trick us into -// selecting a rejection. -func isACPRejectOption(opt acpPermissionOption) bool { - if strings.HasPrefix(strings.ToLower(strings.TrimSpace(opt.Kind)), "reject") { +// isACPGrantKind reports whether an ACP PermissionOptionKind grants the +// action. Only the two current allow kinds qualify; every other or unknown +// value is non-granting, so grant detection fails closed. +func isACPGrantKind(kind string) bool { + switch strings.ToLower(strings.TrimSpace(kind)) { + case acpKindAllowOnce, acpKindAllowAlways: return true + default: + return false } - lowerID := strings.ToLower(opt.OptionID) - return strings.Contains(lowerID, "deny") || strings.Contains(lowerID, "reject") } // acpRPCError is a JSON-RPC error frame returned by the agent process. diff --git a/server/pkg/agent/hermes_test.go b/server/pkg/agent/hermes_test.go index 73a0120d92..8c7b443bd2 100644 --- a/server/pkg/agent/hermes_test.go +++ b/server/pkg/agent/hermes_test.go @@ -578,54 +578,85 @@ func (b *bufferWriter) String() string { return b.buf.String() } -// TestHermesClientAutoApprovesPermissionRequest asserts that when an -// ACP agent sends us `session/request_permission`, the client replies by -// selecting one of the optionIds the agent *actually offered* — an id the -// agent never offered is treated as a denial and, on Hermes' edit path, -// silently blocks every file write (GitHub multica#5300). The reply must -// prefer session-scoped over single-use over a permanent grant, and echo -// the agent's request id so its in-flight future resolves. +// TestHermesClientAutoApprovesPermissionRequest asserts how the client +// answers an ACP agent's `session/request_permission`. It must select an +// optionId the agent *actually offered* — an id the agent never offered is +// treated as a denial and, on Hermes' edit path, silently blocks every file +// write (GitHub multica#5300). It prefers a session-scoped grant over a +// single-use one, refuses to auto-select a permanent "allow_always" (which +// persists to the runtime owner's on-disk allowlist), and fails closed with +// a "cancelled" outcome when no safe grant is offered rather than fabricate +// a selection. The reply always echoes the agent's request id. func TestHermesClientAutoApprovesPermissionRequest(t *testing.T) { t.Parallel() cases := []struct { - name string - options string - wantID string + name string + options string + wantOutcome string // "selected" or "cancelled" + wantID string // expected optionId when wantOutcome == "selected" }{ { // Hermes edit-approval offers only these two and requires // exactly "allow_once"; this is the multica#5300 regression // the old hardcoded "approve_for_session" reply broke. - name: "hermes edit approval", - options: `[{"optionId":"allow_once","name":"Allow edit","kind":"allow_once"},{"optionId":"deny","name":"Deny","kind":"reject_once"}]`, - wantID: "allow_once", + name: "hermes edit approval selects allow_once", + options: `[{"optionId":"allow_once","name":"Allow edit","kind":"allow_once"},{"optionId":"deny","name":"Deny","kind":"reject_once"}]`, + wantOutcome: "selected", + wantID: "allow_once", }, { - // Hermes command approval offers a session option; prefer it - // over the permanent "allow_always" (which persists an on-disk - // allowlist) and over single-use "allow_once". - name: "command approval prefers session over permanent", - options: `[{"optionId":"allow_once","kind":"allow_once"},{"optionId":"allow_session","kind":"allow_always"},{"optionId":"allow_always","kind":"allow_always"},{"optionId":"deny","kind":"reject_once"},{"optionId":"deny_always","kind":"reject_always"}]`, - wantID: "allow_session", - }, - { - // A permanent grant is selected only when it is the sole grant. - name: "permanent grant only as last resort", - options: `[{"optionId":"allow_always","kind":"allow_always"},{"optionId":"deny","kind":"reject_once"}]`, - wantID: "allow_always", + // Hermes command approval offers a session option; prefer it over + // both the permanent "allow_always" and single-use "allow_once". + name: "command approval prefers session over permanent", + options: `[{"optionId":"allow_once","kind":"allow_once"},{"optionId":"allow_session","kind":"allow_always"},{"optionId":"allow_always","kind":"allow_always"},{"optionId":"deny","kind":"reject_once"},{"optionId":"deny_always","kind":"reject_always"}]`, + wantOutcome: "selected", + wantID: "allow_session", }, { // Other ACP agents' session-scoped id is honoured when offered. - name: "session-scoped id honoured", - options: `[{"optionId":"approve","kind":"allow_once"},{"optionId":"approve_for_session","kind":"allow_always"},{"optionId":"reject","kind":"reject_once"}]`, - wantID: "approve_for_session", + name: "session-scoped id honoured", + options: `[{"optionId":"approve","kind":"allow_once"},{"optionId":"approve_for_session","kind":"allow_always"},{"optionId":"reject","kind":"reject_once"}]`, + wantOutcome: "selected", + wantID: "approve_for_session", }, { - // No offered options: fall back to the legacy id rather than hang. - name: "no options falls back to legacy id", - options: `[]`, - wantID: "approve_for_session", + // Grant nature comes from the ACP kind, not the opaque optionId: + // a single-use option with a non-standard id is still selected. + name: "non-standard single-use id selected by kind", + options: `[{"optionId":"yolo-42","kind":"allow_once"},{"optionId":"nope","kind":"reject_once"}]`, + wantOutcome: "selected", + wantID: "yolo-42", + }, + { + // Only a permanent grant offered: fail closed, never auto-persist. + name: "permanent-only grant fails closed", + options: `[{"optionId":"allow_always","kind":"allow_always"},{"optionId":"deny","kind":"reject_once"}]`, + wantOutcome: "cancelled", + }, + { + // Reject-only options: fail closed. + name: "reject-only fails closed", + options: `[{"optionId":"deny","kind":"reject_once"},{"optionId":"deny_always","kind":"reject_always"}]`, + wantOutcome: "cancelled", + }, + { + // Unknown / future kind must not be auto-approved by name. + name: "unknown kind fails closed", + options: `[{"optionId":"allow_forever","kind":"allow_super"},{"optionId":"deny","kind":"reject_once"}]`, + wantOutcome: "cancelled", + }, + { + // No options at all: fail closed rather than fabricate a selection. + name: "empty options fails closed", + options: `[]`, + wantOutcome: "cancelled", + }, + { + // Malformed options payload (not an array): fail closed. + name: "malformed options fails closed", + options: `"not-an-array"`, + wantOutcome: "cancelled", }, } @@ -663,11 +694,18 @@ func TestHermesClientAutoApprovesPermissionRequest(t *testing.T) { if resp.ID != 42 { t.Errorf("id: got %d, want 42 (must echo agent's request id)", resp.ID) } - if resp.Result.Outcome.Outcome != "selected" { - t.Errorf("outcome.outcome: got %q, want %q", resp.Result.Outcome.Outcome, "selected") + if resp.Result.Outcome.Outcome != tc.wantOutcome { + t.Errorf("outcome.outcome: got %q, want %q", resp.Result.Outcome.Outcome, tc.wantOutcome) } - if resp.Result.Outcome.OptionID != tc.wantID { - t.Errorf("outcome.optionId: got %q, want %q", resp.Result.Outcome.OptionID, tc.wantID) + switch tc.wantOutcome { + case "selected": + if resp.Result.Outcome.OptionID != tc.wantID { + t.Errorf("outcome.optionId: got %q, want %q", resp.Result.Outcome.OptionID, tc.wantID) + } + case "cancelled": + if resp.Result.Outcome.OptionID != "" { + t.Errorf("cancelled outcome must not carry an optionId, got %q", resp.Result.Outcome.OptionID) + } } }) } diff --git a/server/pkg/agent/kimi.go b/server/pkg/agent/kimi.go index 3ccc476d35..0402e7605f 100644 --- a/server/pkg/agent/kimi.go +++ b/server/pkg/agent/kimi.go @@ -53,8 +53,9 @@ func (b *kimiBackend) Execute(ctx context.Context, prompt string, opts ExecOptio // `kimi acp` ignores --yolo / --auto-approve (they're flags on the // root `kimi` command, not on the `acp` subcommand). Instead, the - // daemon auto-approves in hermesClient.handleAgentRequest by replying - // "approve_for_session" to every session/request_permission request. + // daemon auto-approves in hermesClient.handleAgentRequest by selecting + // a safe granting option the agent offered (see + // selectACPApprovalOptionID) for each session/request_permission request. kimiArgs := append([]string{"acp"}, filterCustomArgs(opts.CustomArgs, kimiBlockedArgs, b.cfg.Logger)...) cmd := exec.CommandContext(runCtx, execPath, kimiArgs...) hideAgentWindow(cmd)