mirror of
https://github.com/multica-ai/multica.git
synced 2026-08-02 01:45:52 +02:00
fix(agent): harden ACP permission auto-approve per review
Address review feedback on the Hermes permission fix: - Never auto-select a permanent "allow_always" grant. It persists to the runtime owner's on-disk allowlist (ACP v1 allow_always "remembers the choice") and would outlive the task; #5300 does not need it. - Fail closed when no safe grant is offered (empty, reject-only, unknown kinds, or permanent-only): reply with a protocol-correct "cancelled" outcome instead of fabricating an un-offered "approve_for_session" id, which a strict agent would treat as a denial anyway. - Decide grant nature purely from the explicit ACP kind (allow_once / allow_always); unknown kinds fail closed. Drop the optionId-substring heuristic so an opaque id can neither sneak a grant through nor get a legitimate allow rejected by name. selectACPApprovalOptionID now returns (id, ok). Preference: a known session-scoped id, then any single-use allow_once grant. Adds tests for permanent-only, reject-only, unknown kind, empty and malformed options, and a non-standard single-use id; refreshes the stale kimi.go comment. Co-authored-by: multica-agent <github@multica.ai>
This commit is contained in:
J
@@ -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.
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user