Skip to content

Commit 48d6ed0

Browse files
authored
Add per-tool call limits to tools.github.allowed and emit allow-only.tool-call-limits (#35376)
1 parent e2dda5d commit 48d6ed0

9 files changed

Lines changed: 397 additions & 12 deletions

.github/workflows/contribution-check.lock.yml

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

.github/workflows/pr-sous-chef.lock.yml

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.
Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
# ADR-35376: Per-Tool Call Limits in `tools.github.allowed`
2+
3+
**Date**: 2026-05-28
4+
**Status**: Draft
5+
**Deciders**: PR author (pelikhan), reviewers of PR #35376
6+
7+
---
8+
9+
## Part 1 — Narrative (Human-Friendly)
10+
11+
### Context
12+
13+
`tools.github.allowed` already restricts *which* GitHub MCP tools a workflow can call, but it does not bound *how many times* each tool runs in a single workflow execution. This leaves single-read workflows (issue triage, label classification, cross-reference checks) exposed to an agent that re-invokes an allowed read tool dozens of times to expand context or chase references — a small allow-list is not the same as a small blast radius. The MCP gateway already enforces other allow-only policy fields (`repos`, `min-integrity`), so a per-tool call limit is the missing dimension on the same seam. The constraint is that the compiler must stay stateless about runtime tool-call counts: enforcement has to live downstream.
14+
15+
### Decision
16+
17+
We will extend `tools.github.allowed` to accept structured entries that carry a per-tool call cap, in addition to the existing string entries. Two syntaxes are accepted for an entry: a plain string `"tool"` (unchanged, no cap) and an object `{name: "tool", max-calls: N}`. Caps are collected into a `tool-call-limits: { "<tool>": <n> }` map under the existing `allow-only` guard policy, and enforcement is delegated to the MCP gateway/firewall layer — the compiler emits policy only. When a workflow declares call limits but no `allowed-repos`/`repos` or `min-integrity`, the generated `allow-only` policy is still emitted with `repos: "all"` because the gateway requires `repos` to be present on any allow-only block.
18+
19+
### Alternatives Considered
20+
21+
#### Alternative 1: New top-level field `tools.github.tool-call-limits` as a separate map
22+
23+
Keep `allowed` as a plain string array and add a sibling map `tool-call-limits: { issue_read: 1, list_labels: 3 }` at the same level as `allowed`. This was rejected because it splits a tool's definition across two YAML locations: a reviewer scanning `allowed` would not see the limit, and a tool name could drift between the two fields (in `allowed` but not in `tool-call-limits`, or vice versa) with no schema-level signal that one referenced the other. Co-locating the cap with the tool name on the `allowed` entry makes the schema self-describing and matches the principle that one decision goes in one place.
24+
25+
#### Alternative 2: Enforce call limits in the compiler or agent loop instead of the gateway
26+
27+
Emit a wrapper around each MCP tool invocation in the generated workflow that counts calls and refuses past the cap, or have the agent harness track its own call counts. This was rejected because the gateway already evaluates `allow-only` policy (`repos`, `min-integrity`) and is the trust boundary the agent process cannot bypass. Putting enforcement in the agent loop would require the agent to honor a self-imposed limit, which is exactly the failure mode (runaway re-invocation) the cap exists to prevent. Reusing the gateway path also keeps the compiler stateless w.r.t. runtime counts.
28+
29+
#### Alternative 3: Colon-delimited shorthand for limits in string entries
30+
31+
Accept a shorthand string like `"tool:N"` and interpret it as a per-tool limit. This was rejected to keep `allowed` string entries unambiguous tool names and avoid overloading free-form strings with parser-specific syntax.
32+
33+
### Consequences
34+
35+
#### Positive
36+
37+
- Single-read workflows can now declare an explicit per-tool ceiling and rely on the gateway to enforce it, closing the cross-reference-expansion gap that `allowed` alone could not address.
38+
- Existing workflows with string-only `allowed` lists keep working unchanged; the new schema is strictly additive, so no migration is required.
39+
- Co-locating the cap with the tool name (object form) means a reviewer reading the YAML sees both pieces of information at once, and schema validation catches drift.
40+
- Enforcement lives at the same MCP gateway seam that already handles `repos` and `min-integrity`, so the firewall/gateway team owns one policy surface, not two.
41+
42+
#### Negative
43+
44+
- Only the object form can carry limits, so concise one-line string entries cannot express call caps.
45+
- The `allowed` array schema changes from a plain string array to a `oneOf` of string and object, which is harder for YAML-aware editors (and humans) to autocomplete than a flat string array.
46+
- Invalid limit values are silently dropped rather than rejected: `parseGitHubAllowedToolsAndLimits` skips `max-calls` values that fail `typeutil.ParseIntValue` or are `<= 0`. A typo (`max-calls: "one"`) becomes an unlimited tool, not a compile error.
47+
- The emitted `tool-call-limits` field only takes effect against an MCP gateway version that understands it; older gateways will ignore the cap and the workflow will run uncapped. There is no compile-time check that the targeted gateway version supports the field.
48+
49+
#### Neutral
50+
51+
- When a workflow declares call limits but neither `allowed-repos`/`repos` nor `min-integrity`, the compiler now emits `allow-only.repos: "all"` automatically to satisfy the gateway's requirement that `allow-only` always carry a `repos` scope. This is a new implicit policy emission case but it matches the existing fallback already used for `min-integrity`-only configurations.
52+
- The `GitHubAllowedTools` parser in `pkg/workflow/tools_parser.go` intentionally discards limit information at parse time; the limits travel exclusively through `getGitHubGuardPolicies`, so any downstream consumer of `config.Allowed` sees only tool names, as before.
53+
- The new helper `parseGitHubAllowedToolsAndLimits` is reused by both `getGitHubAllowedTools` and `getGitHubGuardPolicies`, so the two paths cannot disagree about which entries are well-formed.
54+
55+
---
56+
57+
## Part 2 — Normative Specification (RFC 2119)
58+
59+
> The key words **MUST**, **MUST NOT**, **REQUIRED**, **SHALL**, **SHALL NOT**, **SHOULD**, **SHOULD NOT**, **RECOMMENDED**, **MAY**, and **OPTIONAL** in this section are to be interpreted as described in [RFC 2119](https://www.rfc-editor.org/rfc/rfc2119).
60+
61+
### `tools.github.allowed` Entry Syntax
62+
63+
1. Each entry in `tools.github.allowed` **MUST** be one of: a plain string tool name, or an object with a required `name` field and an optional `max-calls` field.
64+
2. The schema for entries **MUST** be expressed as a `oneOf` of a `string` branch and an `object` branch with `additionalProperties: false` and `required: ["name"]`.
65+
3. The object branch **MUST** declare `max-calls` as `type: integer, minimum: 1`.
66+
4. Plain string entries **MUST** be treated as tool names with no call limit.
67+
5. Object entries **MUST** be skipped (neither the tool name nor a limit emitted) when `name` is missing, not a string, or empty after trimming whitespace.
68+
6. A `max-calls` value that is not a positive integer **MUST** result in the tool name being added to the allowed list without a call limit; the value **MUST NOT** raise a compile error.
69+
70+
### Parser Behavior
71+
72+
1. The function `parseGitHubAllowedToolsAndLimits` **MUST** be the single source of truth for parsing `tools.github.allowed` entries; both `getGitHubAllowedTools` and `getGitHubGuardPolicies` **MUST** call it rather than reimplement entry parsing.
73+
2. The `GitHubToolConfig.Allowed` field produced by `parseGitHubTool` **MUST** contain only tool names; per-tool call limits **MUST NOT** be stored on the parsed config struct.
74+
3. Tool names returned by the parser **MUST** preserve the order they appear in the source YAML.
75+
76+
### Guard Policy Emission
77+
78+
1. When at least one `allowed` entry carries a positive `max-calls`, `getGitHubGuardPolicies` **MUST** emit an `allow-only` block containing a `tool-call-limits` map keyed by tool name with integer values.
79+
2. The `allow-only` block emitted under condition (1) **MUST** also carry a `repos` field; when neither `allowed-repos` nor `repos` is configured on the tool, `repos` **MUST** default to `"all"`.
80+
3. The `tool-call-limits` map **MUST** only include tools whose limit successfully parsed as a positive integer; tools without limits **MUST NOT** appear as keys with value `0`.
81+
4. When no `allowed` entry carries a call limit and neither `allowed-repos`/`repos` nor `min-integrity` is configured, `getGitHubGuardPolicies` **MUST NOT** emit an `allow-only` block solely because `allowed` is non-empty.
82+
83+
### Conformance
84+
85+
An implementation is considered conformant with this ADR if it satisfies all **MUST** and **MUST NOT** requirements above. Failure to meet any **MUST** or **MUST NOT** requirement constitutes non-conformance.
86+
87+
---
88+
89+
*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/26555753359) workflow. The PR author must review, complete, and finalize this document before the PR can merge.*

pkg/cli/compile_guard_policy_test.go

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -470,3 +470,69 @@ This workflow sets trusted-users without min-integrity (should fail).
470470
require.Error(t, err, "Expected compilation to fail without min-integrity")
471471
assert.Contains(t, err.Error(), "min-integrity", "Error should mention min-integrity requirement")
472472
}
473+
474+
// TestGuardPolicyToolCallLimitsCompilation verifies that max-calls entries under
475+
// tools.github.allowed compile successfully in CLI workflow compilation.
476+
func TestGuardPolicyToolCallLimitsCompilation(t *testing.T) {
477+
workflowContent := `---
478+
on:
479+
workflow_dispatch:
480+
permissions:
481+
contents: read
482+
issues: read
483+
pull-requests: read
484+
engine: copilot
485+
tools:
486+
github:
487+
mode: local
488+
allowed:
489+
- name: issue_read
490+
max-calls: 1
491+
---
492+
493+
# Guard Policy Tool Limits Test
494+
`
495+
496+
tmpDir := t.TempDir()
497+
workflowPath := filepath.Join(tmpDir, "test-guard-policy-tool-limits.md")
498+
err := os.WriteFile(workflowPath, []byte(workflowContent), 0o644)
499+
require.NoError(t, err, "Failed to write workflow file")
500+
501+
compiler := workflow.NewCompiler()
502+
err = CompileWorkflowWithValidation(context.Background(), compiler, workflowPath, CompileValidationOptions{})
503+
require.NoError(t, err, "Expected compilation to succeed")
504+
505+
lockFilePath := filepath.Join(tmpDir, "test-guard-policy-tool-limits.lock.yml")
506+
_, err = os.Stat(lockFilePath)
507+
require.NoError(t, err, "Compiled lock file must be generated")
508+
}
509+
510+
// TestGuardPolicyAllowedColonStringDoesNotEmitToolCallLimits verifies that string
511+
// entries containing colons are not interpreted as per-tool call limits.
512+
func TestGuardPolicyAllowedColonStringDoesNotEmitToolCallLimits(t *testing.T) {
513+
workflowContent := `---
514+
on:
515+
workflow_dispatch:
516+
permissions:
517+
contents: read
518+
engine: copilot
519+
tools:
520+
github:
521+
allowed:
522+
- issue_read:1
523+
- list_labels
524+
---
525+
526+
# Guard Policy Colon Entry Test
527+
`
528+
529+
tmpDir := t.TempDir()
530+
workflowPath := filepath.Join(tmpDir, "test-guard-policy-colon-entry.md")
531+
err := os.WriteFile(workflowPath, []byte(workflowContent), 0o644)
532+
require.NoError(t, err, "Failed to write workflow file")
533+
534+
compiler := workflow.NewCompiler()
535+
err = CompileWorkflowWithValidation(context.Background(), compiler, workflowPath, CompileValidationOptions{})
536+
require.Error(t, err, "Expected compilation to fail for unknown tool name")
537+
assert.Contains(t, err.Error(), "Unknown GitHub tool(s): issue_read:1", "Compiler must treat colon entry as literal tool name")
538+
}

pkg/parser/schema_test.go

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1367,3 +1367,72 @@ func TestValidateWithSchema_YAMLIntegerTypes(t *testing.T) {
13671367
t.Errorf("validateWithSchema should accept YAML integer types, got: %v", err)
13681368
}
13691369
}
1370+
1371+
func TestMainWorkflowSchema_GitHubAllowedSupportsToolCallLimits(t *testing.T) {
1372+
t.Parallel()
1373+
1374+
schemaContent, err := os.ReadFile("schemas/main_workflow_schema.json")
1375+
if err != nil {
1376+
t.Fatalf("failed to read schema: %v", err)
1377+
}
1378+
1379+
var schema map[string]any
1380+
if err := json.Unmarshal(schemaContent, &schema); err != nil {
1381+
t.Fatalf("failed to parse schema json: %v", err)
1382+
}
1383+
1384+
properties := schema["properties"].(map[string]any)
1385+
tools := properties["tools"].(map[string]any)
1386+
toolsProps := tools["properties"].(map[string]any)
1387+
github := toolsProps["github"].(map[string]any)
1388+
githubOneOf := github["oneOf"].([]any)
1389+
1390+
var githubObjectSchema map[string]any
1391+
for _, item := range githubOneOf {
1392+
candidate, ok := item.(map[string]any)
1393+
if !ok {
1394+
continue
1395+
}
1396+
if candidate["type"] == "object" {
1397+
githubObjectSchema = candidate
1398+
break
1399+
}
1400+
}
1401+
if githubObjectSchema == nil {
1402+
t.Fatal("tools.github object schema not found")
1403+
}
1404+
1405+
allowed := githubObjectSchema["properties"].(map[string]any)["allowed"].(map[string]any)
1406+
items := allowed["items"].(map[string]any)
1407+
itemOneOf := items["oneOf"].([]any)
1408+
1409+
var objectBranch map[string]any
1410+
for _, branch := range itemOneOf {
1411+
candidate, ok := branch.(map[string]any)
1412+
if !ok {
1413+
continue
1414+
}
1415+
if candidate["type"] == "object" {
1416+
objectBranch = candidate
1417+
break
1418+
}
1419+
}
1420+
if objectBranch == nil {
1421+
t.Fatal("tools.github.allowed object entry schema not found")
1422+
}
1423+
1424+
entryProps := objectBranch["properties"].(map[string]any)
1425+
maxCalls, hasMaxCalls := entryProps["max-calls"].(map[string]any)
1426+
if !hasMaxCalls {
1427+
t.Fatal("tools.github.allowed[].max-calls schema not found")
1428+
}
1429+
if maxCalls["type"] != "integer" {
1430+
t.Fatalf("expected max-calls type integer, got: %v", maxCalls["type"])
1431+
}
1432+
if maxCalls["minimum"] != float64(1) {
1433+
t.Fatalf("expected max-calls minimum 1, got: %v", maxCalls["minimum"])
1434+
}
1435+
if _, hasMaxAlias := entryProps["max"]; hasMaxAlias {
1436+
t.Fatal("tools.github.allowed[].max alias should not be present")
1437+
}
1438+
}

pkg/parser/schemas/main_workflow_schema.json

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3807,9 +3807,30 @@
38073807
"properties": {
38083808
"allowed": {
38093809
"type": "array",
3810-
"description": "List of allowed GitHub API functions (e.g., 'create_issue', 'update_issue', 'add_comment')",
3810+
"description": "List of allowed GitHub API functions. Each entry can be a string tool name (e.g., 'issue_read') or an object with per-tool limits (e.g., {name: 'issue_read', max-calls: 1})",
38113811
"items": {
3812-
"type": "string"
3812+
"oneOf": [
3813+
{
3814+
"type": "string"
3815+
},
3816+
{
3817+
"type": "object",
3818+
"additionalProperties": false,
3819+
"required": ["name"],
3820+
"properties": {
3821+
"name": {
3822+
"type": "string",
3823+
"minLength": 1,
3824+
"description": "GitHub MCP tool name"
3825+
},
3826+
"max-calls": {
3827+
"type": "integer",
3828+
"minimum": 1,
3829+
"description": "Maximum number of calls allowed for this tool per run"
3830+
}
3831+
}
3832+
}
3833+
]
38133834
}
38143835
},
38153836
"mode": {

0 commit comments

Comments
 (0)