diff --git a/docs/src/pages/agents/approvals.mdx b/docs/src/pages/agents/approvals.mdx index 0c24dc24..cdf53a56 100644 --- a/docs/src/pages/agents/approvals.mdx +++ b/docs/src/pages/agents/approvals.mdx @@ -82,4 +82,6 @@ db.CancelPendingTurnRequests(ctx, sessionID, promptRunID, "run cancelled") `api.RequireToolPolicySupport` is checked before a run's first model call. A per-tool policy of `deny` or `allow` is refused outright on a runtime that cannot carry an allow/deny list at all — running without it would grant the agent more than the spec allows. A per-tool policy of `ask` is refused unconditionally: no transport exposes a per-tool prompt to the *runtime's own* declarative tool filter, so it would resolve to "allowed" on every runtime that does support allow/deny lists. +One entry is exempt: an `allow` that names another agent's built-in (Claude's `Read: allow` on a codex run) is skipped, because an allow only pre-approves a tool the agent already has and constrains nothing on an agent without it. That lets one spec carry a Claude-style allowlist across backends. The exemption is deliberately narrow — a `deny` or `ask` on a foreign name is still refused, since Claude's `Bash` is codex's `shell` under another name and dropping the deny would hand the agent the very tool the spec forbade; and a name no agent declares is never treated as foreign, so a stale vocabulary table fails loud instead of waving a new built-in through. + That check is about the runtime's declarative filter, not about whether a broker exists — an `ask` decision made per call, live, is a different mechanism: it is what `approval.Broker.CanUseTool` implements as the `PermissionFunc` itself, and it is how the caller-tool path (aichat) honours a per-tool `ask` policy today. A streaming provider run's declared `permissions.tools` still cannot say `ask` — only `allow`/`deny` — regardless of whether a broker is attached. diff --git a/pkg/api/permissions.go b/pkg/api/permissions.go index bbb08751..e8be2990 100644 --- a/pkg/api/permissions.go +++ b/pkg/api/permissions.go @@ -92,16 +92,26 @@ func (t Tools) toolsWithPolicy(want ToolPolicy) []string { // proceeding as if the policy had been applied. // // Allow-lists are checked too: on a runtime with no tool filter, an allowlist is -// equally unenforced. `ask` is refused everywhere — no transport has a per-tool -// prompt, so it would resolve to "allowed" on the runtimes that advertise tool -// policy support. `auto` constrains nothing, so it needs no runtime support. +// equally unenforced. The one entry that may be dropped is an allow naming +// another agent's built-in (a Claude `Read: allow` on a codex run): an allow only +// pre-approves a tool the agent already has, so on an agent without that tool it +// constrains nothing, and a portable spec can carry both vocabularies. A deny or +// ask is never dropped on that basis — `Bash` is codex's `shell` under another +// name, and dropping the deny would hand the agent the very tool the spec +// forbade. `ask` is refused everywhere: no transport has a per-tool prompt, so it +// would resolve to "allowed" on the runtimes that advertise tool policy support. +// `auto` constrains nothing, so it needs no runtime support. func RequireToolPolicySupport(p *ModelProvider, mode RuntimeMode, permissions Permissions) error { if asked := permissions.Tools.toolsWithPolicy(ToolPolicyAsk); len(asked) > 0 { return fmt.Errorf( "per-tool policy \"ask\" (%s) is not enforceable on any runtime: transports carry allow/deny tool lists only, so the tool would run unprompted; use allow or deny", strings.Join(asked, ", ")) } - enforced := append(permissions.Tools.AllowList(), permissions.Tools.DenyList()...) + vocabulary := PermissionCapabilitiesFor(RuntimeOf(p, mode)).Tools + allowed := slices.DeleteFunc(permissions.Tools.AllowList(), func(tool string) bool { + return isForeignBuiltin(vocabulary, tool) + }) + enforced := append(allowed, permissions.Tools.DenyList()...) if len(enforced) == 0 || registry.SupportsToolPolicy(p, mode) { return nil } @@ -111,6 +121,27 @@ func RequireToolPolicySupport(p *ModelProvider, mode RuntimeMode, permissions Pe registry.RuntimeOf(p, mode), strings.Join(enforced, ", "), registry.RuntimesList(registry.ToolPolicyRuntimes())) } +// isForeignBuiltin reports whether tool is positively identified as another +// agent's built-in and absent from the selected runtime's vocabulary. It is the +// only basis on which an allow entry may be skipped; see RequireToolPolicySupport. +// +// A name no agent declares stays enforceable: the vocabularies are hand-kept, +// and a stale table must fail loud on a newly added built-in rather than wave it +// through. A runtime with no vocabulary at all (the API modes) owns every name. +func isForeignBuiltin(vocabulary []string, tool string) bool { + if len(vocabulary) == 0 || slices.Contains(vocabulary, tool) { + return false + } + for _, tools := range agentTools { + if slices.ContainsFunc(tools, func(candidate AgentTool) bool { + return candidate.Name == tool + }) { + return true + } + } + return false +} + // Validate checks the mode, presets, tool policies, and resource modes are // recognised. func (p Permissions) Validate() error { diff --git a/pkg/api/runtime_profiles.go b/pkg/api/runtime_profiles.go index 10bff344..6688ade6 100644 --- a/pkg/api/runtime_profiles.go +++ b/pkg/api/runtime_profiles.go @@ -259,7 +259,12 @@ func validateResolvedPermissions(spec Spec) error { if posture := spec.Permissions.Mode; posture != "" && !caps.ModeSupport(posture).Honoured() { return fmt.Errorf("permissions.mode %q is not available for %s", posture, runtime) } - for _, policy := range spec.Permissions.Tools { + for tool, policy := range spec.Permissions.Tools { + // Same rule as RequireToolPolicySupport: only an allow for another + // agent's built-in is inert here; a deny or ask must still be honoured. + if policy == ToolPolicyAllow && isForeignBuiltin(caps.Tools, tool) { + continue + } if err := requireResolvedToolPolicy(caps, runtime, ProvenanceAgent, policy); err != nil { return err } diff --git a/pkg/api/runtime_profiles_ginkgo_test.go b/pkg/api/runtime_profiles_ginkgo_test.go index d077e427..fceb3c52 100644 --- a/pkg/api/runtime_profiles_ginkgo_test.go +++ b/pkg/api/runtime_profiles_ginkgo_test.go @@ -124,6 +124,36 @@ var _ = Describe("Runtime profiles", func() { Expect(err).To(MatchError(ContainSubstring("is not available for openai agent"))) }) + // Profile validation shares RequireToolPolicySupport's one relaxation: an + // allow for a tool the runtime does not have (Claude's Read on codex) is + // inert, but a deny on a foreign name still describes a codex capability and + // must be refused like any other agent-tool policy codex cannot carry. + It("skips a foreign allow but refuses a foreign deny for the resolved runtime", func() { + profile := func(tools api.Tools) api.RuntimeProfile { + return api.RuntimeProfile{ + ID: "codex", Name: "Codex", Spec: api.Spec{ + Model: api.Model{Name: "gpt-5", Mode: api.ModeAgent}, + Permissions: api.Permissions{Tools: tools}, + }, + } + } + + _, err := api.ResolveRuntimeProfile(api.RuntimeProfileResolveRequest{ + Profile: profile(api.Tools{"Read": api.ToolPolicyAllow, "Edit": api.ToolPolicyAllow}), + }) + Expect(err).NotTo(HaveOccurred()) + + _, err = api.ResolveRuntimeProfile(api.RuntimeProfileResolveRequest{ + Profile: profile(api.Tools{"Bash": api.ToolPolicyDeny}), + }) + Expect(err).To(MatchError(ContainSubstring(`agent-tool policy "deny" is not available for openai agent`))) + + _, err = api.ResolveRuntimeProfile(api.RuntimeProfileResolveRequest{ + Profile: profile(api.Tools{"shell": api.ToolPolicyAllow}), + }) + Expect(err).To(MatchError(ContainSubstring(`agent-tool policy "allow" is not available for openai agent`))) + }) + // The posture is independent of isolation: a run with no sandbox at all must // still carry the mode it asked for, which folding it into SandboxRef broke. It("keeps a permission mode when no sandbox is configured", func() { diff --git a/pkg/api/tool_policy_support_test.go b/pkg/api/tool_policy_support_test.go index eab93621..99d04ebf 100644 --- a/pkg/api/tool_policy_support_test.go +++ b/pkg/api/tool_policy_support_test.go @@ -139,11 +139,74 @@ func TestToolsAllowDenyLists(t *testing.T) { // the same backends: where there is no tool filter, an allowlist is equally // unenforced, and silently ignoring it grants more than the spec allowed. func TestRequireToolPolicySupport_AllowListToo(t *testing.T) { - policy := Permissions{Tools: Tools{"Read": ToolPolicyAllow}} - if err := RequireToolPolicySupport(OpenAI, ModeCLI, policy); err == nil { + codexPolicy := Permissions{Tools: Tools{"shell": ToolPolicyAllow}} + err := RequireToolPolicySupport(OpenAI, ModeCLI, codexPolicy) + if err == nil { t.Fatal("codex-cli silently drops an allow-list; want a loud refusal") } - if err := RequireToolPolicySupport(Anthropic, ModeCLI, policy); err != nil { + if !strings.Contains(err.Error(), "shell") { + t.Errorf("error %q does not name the offending tool", err) + } + claudePolicy := Permissions{Tools: Tools{"Read": ToolPolicyAllow}} + if err := RequireToolPolicySupport(Anthropic, ModeCLI, claudePolicy); err != nil { t.Fatalf("claude-cli must carry an allow-list, got %v", err) } } + +// TestRequireToolPolicySupport_ForeignAllowIsInert pins the one relaxation of the +// guard: an allow naming another agent's built-in constrains nothing on a runtime +// that has no such tool, so a portable Claude-style allowlist (issue #110) must +// not abort a codex run. The check is per entry, so a mixed map keeps failing on +// the entries the runtime does own. +func TestRequireToolPolicySupport_ForeignAllowIsInert(t *testing.T) { + claudeAllow := Tools{} + for _, tool := range []string{"Bash", "Edit", "Glob", "Grep", "Read", "Write"} { + claudeAllow[tool] = ToolPolicyAllow + } + for _, runtime := range []Runtime{RuntimeOf(OpenAI, ModeAgent), RuntimeOf(OpenAI, ModeCLI), RuntimeOf(Google, ModeCLI)} { + p, _ := runtime.ModelProvider() + if err := RequireToolPolicySupport(p, runtime.Mode, Permissions{Tools: claudeAllow}); err != nil { + t.Errorf("%s refused an allowlist of tools it does not have: %v", runtime, err) + } + } + + mixed := Permissions{Tools: Tools{"Read": ToolPolicyAllow, "shell": ToolPolicyAllow}} + err := RequireToolPolicySupport(OpenAI, ModeAgent, mixed) + if err == nil { + t.Fatal("codex-agent dropped an allow for its own shell alongside a foreign one") + } + if !strings.Contains(err.Error(), "shell") || strings.Contains(err.Error(), "Read") { + t.Errorf("error %q should name shell and only shell", err) + } +} + +// TestRequireToolPolicySupport_ForeignDenyStaysLoud pins the boundary of that +// relaxation. Claude's Bash is codex's shell under another name, so a deny or ask +// on a foreign name still describes a capability the runtime has; dropping it +// would hand the agent the very tool the spec forbade. Both must keep failing. +func TestRequireToolPolicySupport_ForeignDenyStaysLoud(t *testing.T) { + for _, policy := range []ToolPolicy{ToolPolicyDeny, ToolPolicyAsk} { + err := RequireToolPolicySupport(OpenAI, ModeAgent, Permissions{Tools: Tools{"Bash": policy}}) + if err == nil { + t.Errorf("codex-agent silently dropped a foreign %s; want a loud refusal", policy) + continue + } + if !strings.Contains(err.Error(), "Bash") { + t.Errorf("%s: error %q does not name the offending tool", policy, err) + } + } +} + +// TestRequireToolPolicySupport_UnknownAllowStaysLoud pins that only a name some +// agent positively declares is treated as foreign. The vocabularies are hand-kept, +// so a built-in they have not caught up with must fail closed, not be waved +// through as if it belonged to some other agent. A runtime with no vocabulary at +// all (the API modes) owns every name for the same reason. +func TestRequireToolPolicySupport_UnknownAllowStaysLoud(t *testing.T) { + if err := RequireToolPolicySupport(OpenAI, ModeAgent, Permissions{Tools: Tools{"NotATool": ToolPolicyAllow}}); err == nil { + t.Error("codex-agent dropped an allow for a name no agent declares") + } + if err := RequireToolPolicySupport(OpenAI, ModeAPI, Permissions{Tools: Tools{"Read": ToolPolicyAllow}}); err == nil { + t.Error("openai api has no vocabulary and cannot call any name foreign") + } +}