Enforce .agent.md tools: policy at runtime for copilot-sdk execution

[Copilot]

Summary

Enforce the selected custom agent's .agent.md tools: declaration for Copilot SDK evaluations. Omitted tools record unrestricted, an empty list means deny_all, and populated lists mean allow_list. Denied attempts fail the run even when the final answer is otherwise correct or an engine reports no error text.

Implementation

• Share selected-agent discovery with prompt injection: match the eval's agent name, use task-level effective skill paths, support nested/multiple agents (including a nested target beside an unrelated root agent), honor SKILL.md precedence, and skip policy/implicit grading when skills are fully disabled. Malformed definitions fail closed.
• Apply native SDK AvailableTools filtering, pre-tool-use checks, and fail-closed permission handling to initial/resumed sessions and static/responder follow-ups. Preserve the caller's permission handler for allowed requests.
• Share built-in aliases with the implicit allow_only grader; translate read/readFile/fileRead/view to builtin:view and fetch/web_fetch to builtin:web_fetch. Bare names refer to built-ins; MCP and custom tools require explicit mcp:<server>-<tool> and custom:<tool> declarations. Permission keys preserve these namespaces.
• Publish additive schema 1.3 policy mode/denial fields through results, NDJSON run_complete events, dashboard API, and trajectory digest. Align the embedded eval schema default and test it against the runtime version. Avoid shared mutable policy/grader state across tasks or baseline passes.

Validation

• go vet -p 2 ./..., full go test -p 2 ./..., short suite, and golangci-lint (0 issues), repeated after post-push feedback fixes.
• Opt-in real SDK read-only/current-information regression passed for both initial and resumed sessions, repeated after source-matching changes.
• Dashboard build/lint and all 58 Chromium tests passed, including policy denial display; screenshot suite exercised.
• Documentation site build passed; README, guide, PRD, custom-agent documentation, and schema changelog updated.
• Two independent review passes completed; findings reconciled with all prior feedback without duplicate review comments. Subsequent automated-review findings have focused regressions and original-thread responses.
• Integrated main through aa763a4 without force-pushing.

Boundaries

This restricts tool capabilities, not what an allowed tool can access. Hooks/transcripts may omit source metadata: explicit custom/MCP declarations match those literal names, with source isolation supplied by SDK native filtering. Explicitly supplied source metadata is preserved. A permitted subagent launcher is not a separate subagent sandbox. Source-aware identifiers retain SDK spelling; exact-name checks do not grant wildcard patterns.

Closes #585

[Copilot]

Copilot wasn't able to review any files in this pull request.

[Copilot]

🟡 Changes recommended

Unresolved critical policy-resolution and canonical tool-name findings, plus moderate state-handling and coverage issues, block approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

internal/execution/copilot.go:417

• The policy is wired into ResumeSessionConfig here, but the new tests only assert the create-session path. A regression in resumed turns would re-expose tools despite the acceptance requirement for resumed sessions. Add a test that resumes with a policy and checks AvailableTools plus rejection of an undeclared permission request.

			OnPermissionRequest: permRequestCallback,
			AvailableTools:      availableTools,

internal/orchestration/agent_graders.go:76

• When fm.Tools is nil because a valid agent omitted tools:, this branch returns nil. CopilotEngine only serializes ToolPolicyMode for a non-nil policy, so the session digest and results.json omit unrestricted, despite the documented effective-policy recording and the tri-state's third state. Return NewToolPolicy(nil) for this valid case while reserving nil for no resolved agent or a resolution failure.

// resolveAgentPath finds the first .agent.md file in the given skill directories.
// Returns empty string if no agent file is found.

• Files reviewed: 10/11 changed files
• Comments generated: 4
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Unresolved critical and moderate findings remain across asset references, policy matching, propagation, and result surfacing.

Review details

Suppressed comments (11)

Previously missed (1) — in code that hasn't changed since the last review.

internal/orchestration/runner.go:342

• This lookup can leave r.toolPolicy stale: on the second pass of a baseline run SkillPaths is emptied, so this branch is skipped but buildExecutionRequest still reuses the policy resolved during the skills-enabled pass. It also scans unfiltered paths even though requests use FilteredSkillPaths, so --no-skills can enforce a disabled agent. Clear the field before resolving and use the filtered paths here.

This issue also appears on line 351 of the same file.

internal/execution/toolpolicy.go:129

• This canonicalizer strips builtin:, mcp:, and custom: prefixes, but augmentGradersFromAgent passes the raw declaration to allow_only, whose matcher compares it directly with the raw ToolCall.Name. For example, tools: [builtin:bash] is allowed by the runtime as bash but an ordinary bash event is rejected by the implicit grader. Either remove this prefix normalization or apply the same canonicalization to grader declarations and recorded calls.

func canonicalToolName(name string) string {
	name = strings.ToLower(strings.TrimSpace(name))
	if idx := strings.Index(name, ":"); idx > 0 {
		switch name[:idx] {
		case "builtin", "mcp", "custom":
			name = name[idx+1:]
		}
	}

internal/execution/toolpolicy.go:215

• The custom-agent guide defines fileRead, fileWrite, and runCommand as valid tools: entries, but these native permission variants are canonicalized here as read, write, and bash. An agent using the documented names will therefore have its otherwise allowed file/shell requests rejected before execution, so runtime enforcement does not match the existing frontmatter/grader contract. Define the canonical aliases consistently (and test these variants) or update the documented contract before enabling this policy.

	case *copilot.PermissionRequestRead:
		return "read", true
	case *copilot.PermissionRequestWrite:
		return "write", true
	case *copilot.PermissionRequestShell:

internal/execution/toolpolicy.go:106

• This list is passed directly to the SDK as AvailableTools, but the policy map has already lowercased and stripped prefixes from every declaration. That turns documented identifiers such as codeSearch, fileRead, and runCommand into different names (codesearch, fileread, runcommand) before the SDK filters its tools, which can hide an allowed tool even though the permission comparison is case-insensitive. Keep the original declared identifiers for AvailableTools and use canonicalized values only for comparisons.

	names := make([]string, 0, len(p.allowed))
	for n := range p.allowed {
		names = append(names, n)
	}
	sort.Strings(names)

internal/execution/toolpolicy.go:200

• The SDK MCP permission request carries the server separately from the tool name, while the SDK exposes MCP tools as <server-key>-<tool-name> (and recommends mcp:<server-key>-<tool-name> for AvailableTools). Dropping req.ServerName means a declaration such as tools: [github-list_issues] is reduced to list_issues and rejected, while the SDK filter also receives an unqualified name. Compose the server and tool using one canonical contract before matching and populating the allow-list.

	case *copilot.PermissionRequestMCP:
		return canonicalToolName(req.ToolName), true
	case *copilot.PermissionRequestHook:

internal/models/outcome.go:277

• These fields reach results.json through SessionDigest, but the existing --session-log path still writes only aggregate SessionCompleteData totals and never emits the per-run policy mode or denials. The issue and PR description require the effective policy and every denial in session logs as well; add those fields to the session events or log the run digest during execution.

	// ToolPolicyMode is the effective .agent.md `tools:` runtime policy
	// applied to this session: "unrestricted", "deny_all", or "allow_list".
	// Empty when no .agent.md tool policy was resolved for the run.
	ToolPolicyMode string `json:"tool_policy_mode,omitempty"`
	// ToolPolicyDenials records every tool-execution attempt denied by an
	// active tool policy across all turns of this session (initial,
	// follow-up, and responder-driven alike).
	ToolPolicyDenials []ToolPolicyDenial `json:"tool_policy_denials,omitempty"`

internal/models/outcome.go:277

• These fields are added to the canonical session digest, but internal/webapi.mapSessionDigest still constructs SessionDigestResponse with only the legacy fields, and the web client type is unchanged. Dashboard/API consumers therefore drop tool_policy_mode and tool_policy_denials, so the new policy result is unavailable outside raw results.json; add the fields to that DTO/mapping and client representation.

	ToolPolicyMode string `json:"tool_policy_mode,omitempty"`
	// ToolPolicyDenials records every tool-execution attempt denied by an
	// active tool policy across all turns of this session (initial,
	// follow-up, and responder-driven alike).
	ToolPolicyDenials []ToolPolicyDenial `json:"tool_policy_denials,omitempty"`

internal/orchestration/agent_graders.go:72

• An agent with a valid .agent.md but no tools: key is treated as nil here, so ExecutionResponse.ToolPolicyMode stays empty and the session digest cannot report the required unrestricted state; it is indistinguishable from a run with no agent policy. Keep nil only for fm == nil and return execution.NewToolPolicy(fm.Tools) for the omitted-key case (Active() already makes it a no-op).

	if fm == nil || fm.Tools == nil {
		return nil
	}
	return execution.NewToolPolicy(fm.Tools)

internal/orchestration/runner.go:351

• resolveAgentPath returns the first .agent.md it encounters and this call does not use spec.SkillName, whereas buildSkillSystemMessage selects the definition by name. With multiple configured agent directories (or multiple agent files), the runtime policy can therefore come from a different agent and allow or deny the wrong tools. Resolve the path for the selected agent before assigning its policy.

		spec.Graders = augmentGradersFromAgent(spec.Graders, agentPath, fm)
		r.toolPolicy = resolveToolPolicy(fm)

internal/orchestration/runner.go:1530

• The new field is exercised only by direct CopilotEngine tests; there is no orchestration test that sends a static follow-up or responder reply and verifies the policy is carried into that request and that a later-turn denial is merged into SessionDigest. This is the path required to enforce and surface violations beyond the initial turn, so add regression coverage for it.

		ToolPolicy:        r.toolPolicy,

internal/orchestration/runner.go:351

• This resolution scans the raw spec.Config.SkillPaths, while buildExecutionRequest later uses FilteredSkillPaths and can set NoSkills when the agent directory is disabled (for example via disabled_skills: ["*"]). In that case the .agent.md is not exposed to the SDK, but its tools: policy is still attached to every request and can deny the fallback agent's tools. Resolve from the same effective skill paths, or skip the policy when the target agent is disabled.

		r.toolPolicy = resolveToolPolicy(fm)

• Files reviewed: 10/11 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Shayne Boyer (spboyer)]

Reviewed runtime enforcement of custom-agent tool policies at bb42abaf with two independent review passes.

No new, non-duplicate findings. Previously reported blockers remain at this head, so changes are still requested. See the existing review; no duplicate inline comments are added.

[Copilot]

🟡 Changes recommended

Unresolved policy mismatches, resolution gaps, missing result/UI propagation, asset issues, and incomplete integration coverage block approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (4)

Previously missed (1) — in code that hasn't changed since the last review.

internal/orchestration/agent_graders.go:72

• For an existing .agent.md whose tools: key is omitted, this returns nil, so CopilotEngine leaves ExecutionResponse.ToolPolicyMode empty and SessionDigest omits the effective unrestricted mode. That loses the tri-state result and conflates it with a run with no agent; keep nil only when fm is nil and return NewToolPolicy(fm.Tools) for an agent frontmatter.

internal/execution/copilot.go:417

• The new tests only capture the initial SessionConfig on a mocked client and invoke the callback directly; they do not exercise the changed ResumeSessionConfig branch or a real Copilot session. That cannot catch an SDK regression where AvailableTools is ignored or a resumed session installs a different callback, while the issue's acceptance criteria explicitly require resumed-session and live bash/web-fetch coverage. Add a policy-specific resume test and a gated live integration test.

			OnPermissionRequest: permRequestCallback,
			AvailableTools:      availableTools,

internal/execution/copilot_test.go:287

• The new tests exercise the wrapper with mock permission requests, but none drives a real SDK session/model through AvailableTools and the built-in URL/shell paths. That leaves the actual tool-name contract (including web_fetch and the read aliases) unverified; the issue's acceptance criteria explicitly require a live read-only integration scenario. Add a gated integration test/CI job or document and track the missing acceptance criterion separately.

func TestCopilotExecute_ToolPolicyDenialFailsRun(t *testing.T) {
	ctrl := gomock.NewController(t)
	clientMock := newClientMock(ctrl)
	sessionMock := NewMockCopilotSession(ctrl)

	sourceDir := t.TempDir()

	declared := []string{"read"}
	policy := NewToolPolicy(&declared)

	var capturedConfig *copilot.SessionConfig
	clientMock.EXPECT().CreateSession(gomock.Any(), gomock.Any()).DoAndReturn(
		func(_ context.Context, cfg *copilot.SessionConfig) (CopilotSession, error) {
			capturedConfig = cfg
			return sessionMock, nil
		})
	sessionMock.EXPECT().Disconnect()
	clientMock.EXPECT().DeleteSession(gomock.Any(), "session-1")

	sessionMock.EXPECT().On(gomock.Any()).Times(3).Return(func() {})
	sessionMock.EXPECT().SendAndWait(gomock.Any(), gomock.Any()).DoAndReturn(
		func(_ context.Context, _ copilot.MessageOptions) (*copilot.SessionEvent, error) {
			// Simulate the model attempting an undeclared bash call mid-turn.
			_, err := capturedConfig.OnPermissionRequest(&copilot.PermissionRequestShell{FullCommandText: "curl example.com"}, copilot.PermissionInvocation{})
			require.NoError(t, err)
			return &copilot.SessionEvent{}, nil
		})

internal/models/outcome.go:277

• These fields are added to the results model, but internal/webapi.mapSessionDigest and web/src/api/client.ts/SessionDigestCard still drop and cannot display them. Thus policy mode and denials disappear from the dashboard even though this PR adds result metadata and updates the dashboard artifact. Update the API/UI mapping, or explicitly scope this feature to raw results and logs.

	// ToolPolicyMode is the effective .agent.md `tools:` runtime policy
	// applied to this session: "unrestricted", "deny_all", or "allow_list".
	// Empty when no .agent.md tool policy was resolved for the run.
	ToolPolicyMode string `json:"tool_policy_mode,omitempty"`
	// ToolPolicyDenials records every tool-execution attempt denied by an
	// active tool policy across all turns of this session (initial,
	// follow-up, and responder-driven alike).
	ToolPolicyDenials []ToolPolicyDenial `json:"tool_policy_denials,omitempty"`

• Files reviewed: 10/11 changed files
• Comments generated: 4
• Review effort level: Lite

[Shayne Boyer (spboyer)]

Reviewed ec4a1a7 against current main, including all historical feedback and two independent review passes. The policy, selected-agent resolution, canonical names/native filters, resumed and later-turn behavior, schema 1.3, session logs, and dashboard propagation are addressed. No remaining actionable findings after deduplication. Full Go tests/vet/short suite and lint pass; live SDK initial/resumed read-only regression passes; dashboard build/lint, 58 Chromium tests, and docs build pass. All original inline threads are resolved. This supersedes my earlier changes-requested review; it does not waive required CI, strict up-to-date main, or independent latest-push/code-owner approval.

[Copilot]

Copilot review overview

🟡 Changes recommended

Two critical enforcement issues and one agent-discovery defect remain unresolved; the schema default also needs alignment.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 · 1

Open (3)

• Unqualified tool names bypass MCP and custom namespaces · New
• Policy denials can be recorded as successful runs · New
• Top-level agent mismatch prevents nested skill resolution · New

Resolved since last review (5)

• This resolves the policy from the first .agent.md found in the eval-level skill paths, but…
• URL permission requests are always canonicalized to fetch. If an agent declares the…
• The public custom-agent guide uses tools: [codeSearch, fileRead, runCommand], but this alias…
• Both an omitted tools: key and a failed LoadAgentDefinition become nil here. A malformed or…
• The runtime policy canonicalizes readFile/writeFile to read/write, but…

[Shayne Boyer (spboyer)]

Re-reviewed the new head c53e73b after the post-push review. Source-qualified permission matching, fatal denial-only responses on all turn paths, nested target discovery beside unrelated root agents, and embedded schema default alignment are fixed with regressions. All 11 inline threads are resolved; no new actionable findings remain after deduplication. Full Go tests/vet/short suite and lint pass again; repeated live SDK initial/resumed read-only test and docs build pass. Previous dashboard build/lint and 58 Chromium tests remain applicable (no frontend changes in this follow-up). Required CI and independent latest-push/code-owner approval still apply; no bypass requested.

[Copilot]

Copilot review overview

🟡 Changes recommended

Critical findings remain: a nil permission request can panic, and the integration test file does not compile.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2

Open (2)

• Guard nil permission requests before calling Kind · New
• Fix invalid slice pointer construction in tests · New

Resolved since last review (3)

• Policy denials can be recorded as successful runs
• Unqualified tool names bypass MCP and custom namespaces
• Top-level agent mismatch prevents nested skill resolution

[Shayne Boyer (spboyer)]

All existing review findings are fixed in 0f3cd51. Nil and typed-nil requests now fail closed, the tri-state tests compile, and full Go tests and lint pass.

[Shayne Boyer (spboyer)]

Re-reviewed the current conflict-resolved head. The fail-closed tool-policy fixes are preserved, current main changes are integrated, all review threads are resolved, and full Go tests and lint pass.

[Copilot]

Copilot review overview

🟡 Changes recommended

Critical policy-enforcement gaps remain unresolved.

Review effort: Lite
Findings: 2

Open (2)

• Resolve agent policy once per task run · New
• Include agent policy contents in cache keys · New

Resolved since last review (2)

• Fix invalid slice pointer construction in tests
• Guard nil permission requests before calling Kind

[Copilot]

Copilot review overview

🟡 Changes recommended

Three moderate findings and one documentation nit remain unresolved.

Review effort: Lite
Findings: 2 · 1

Open (3)

• Include agent policy contents in cache keys
• Resolve agent policy once per task run
• Baseline still discovers cwd agents when skills are disabled · New