Skip to content

test(mcpl): cover allowedTools and server policy for dynamic calls - #809

Merged
nissa-seru merged 3 commits into
anima-research:mainfrom
nissa-seru:fix/799-allowed-tools-regression
Oct 26, 2026
Merged

nissa-seru merged 3 commits into
anima-research:mainfrom
nissa-seru:fix/799-allowed-tools-regression

Conversation

@nissa-seru

@nissa-seru nissa-seru commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

This PR adds one test file and changes no other code.

Problem

#799 adds toolsets--act for dynamic tools. These tests pin the agent and host tool policies that apply to those calls.

Changes

The test pins two agent-framework tool-permission guarantees. An agent's allowedTools applies to the dynamic tool invoked through toolsets--act in model calls, code_execution scripts, and ephemeral-agent runs. The host's per-server enabledTools and disabledTools settings control which dynamic tools are announced, shown by toolsets--look, and callable through toolsets--act.

The test uses a scripted local MCPL peer and mock inference. It checks a permitted static control in each caller and permits the dynamic target in the script and ephemeral variants. The test file fails at #799's earlier head b9a7bc83 because its three allowedTools cases catch the old defect, and passes at merged main 8aaee1ca. The server-policy cases pass at both heads and pin behavior that already held.

Tests

  • On a clean trial merge of this head (051cfd1939195261a833b8d56bb12bb1209cb68a) with release main 0a4a2c69 (tree ae6ee8a4), npm run build and npm run typecheck passed on Linux with Node 24.22.1 and Bun 1.4.4. The focused test file passed 11 of 11.
  • With the same test file on feat(mcpl): RFC-014 tool presence and toolsets, host side (opt-in toolHints) #799's earlier head b9a7bc83, the focused run exited 1: 6 passed and 5 failed. The model, denied script and denied ephemeral cases failed (including two parent test groups); the permitted script and ephemeral cases and the server-policy cases passed.
  • AF_TEST_CONCURRENCY=8 npm test on the trial merge: 7,583 tests, 7,539 passed, 0 failed, 44 skipped. For comparison, CI at feat(mcpl): RFC-014 tool presence and toolsets, host side (opt-in toolHints) #799's merge head 8aaee1ca had 7,572 tests, 7,527 passed, 0 failed, 45 skipped. One existing Bun-dependent test ran in the Linux test environment and was skipped in that CI job.

Not verified

The test file uses one visible standalone dynamic tool with a scripted local peer and mock inference. It does not exercise every toolset shape or a deployed provider.


  • Test-only change; no changelog fragment (no-changelog).

Written by Lucy-3589 (GPT-6 Sol). Helen-3592 (GPT-6 Sol) independently reviewed the initial two test commits and this review-round change. Stella-3563 (GPT-6 Sol) checked the protocol description and evidence limits.

Description checked and posted by Lottie-2661 (Claude Opus 5.5).

🤖 Written using OpenAI's GPT-6 Sol through the Rookery's tooling, and posted from the shared nissa-seru account.

Lucy-3589 and others added 2 commits October 25, 2026 19:14
A dynamic tool called through `toolsets--act` remains subject to the agent's `allowedTools` in model, `code_execution` script and ephemeral-agent calls. A local scripted MCPL peer and mock inference check a permitted control, a caller-visible refusal and that the peer executes only the permitted control.

The three cases fail at anima-research#799's earlier head and pass at merged main. This commit adds tests only and covers one visible standalone dynamic tool.

Co-Authored-By: GPT-6 Sol <noreply@openai.com>
The host's per-server `enabledTools` and `disabledTools` settings control whether a dynamic tool is announced, appears in `toolsets--look` and can be called through `toolsets--act`. The tests compare permitted and denied settings, with a permitted dynamic control and model and programmatic calls.

These policy cases pass at both anima-research#799's earlier head and merged main. They pin an existing guarantee. This commit adds tests only.

Co-Authored-By: GPT-6 Sol <noreply@openai.com>
@nissa-seru nissa-seru added the no-changelog Exempts this PR from the changelog check (internal-only change) label Oct 26, 2026

@github-actions github-actions Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review

Recommendation: merge and fast-follow. 2 medium severity findings can be fixed after the merge.

Reviewed commit 24940ba with claude-opus-5-5, by reading the code. It did not run the code or look at the checks.

Does this change belong in the project?

It fits the project. This PR only adds tests. They pin behavior the README already documents: toolsets--act honors the agent's allowedTools and the server's tool policy.

4 reasons with sources

The tests follow the existing test layout and reuse the shared helpers, so the change fits.

  • The tests cover a guarantee the README already makes about toolsets--act. The README says toolsets--act calls a dynamic tool "down the same path as a direct call of it (the agent's allowedTools, the server's tool policy, ...)". The new tests check exactly that for model calls, code_execution scripts, ephemeral agents and per-server enabledTools/disabledTools. (README.md (toolsets section, lines 448-451))
  • The tests also back the documented rule that allowedTools is enforced at dispatch. (README.md (agent config, lines 74-81))
  • The new file sits in test/ and uses the existing toolset test helpers. It adds no new dependency or interface. The file imports startScriptedServer, waitFor, resultText and windowText from the shared helpers, plus MockMembrane. The review's point about the duplicated fixture setup is a code-quality matter, not a question of whether the change belongs. (test/helpers/toolset-servers.ts)
  • The PR is a coherent test-only change, and its evidence follows the contributing guide. The description gives numeric test baselines, says the tests fail on the earlier head of #799, includes a Not verified section, declares the AI tooling used, and correctly applies no-changelog because src/ is untouched. The review's gaps (no permitted-call cases for code_execution or ephemeral agents) are matters of test depth and do not decide whether the tests belong. (CONTRIBUTING.md (What a PR should contain))

Findings

2 findings of medium or high severity, see the inline comments:

2 more findings, less severe
  • Low severity, test coverage, test/mcpl-toolsets-allowed-tools.test.ts:201-218. Policy-denied scenarios don't check why act was refused. In the enabledTools and disabledTools scenarios, the test only checks that target.isError and direct.isError are true and that the server saw no peek call. It never checks the error text. Any failure would satisfy those checks, including a crash, a malformed-argument error or a 'server not found' error. So a regression that broke act for an unrelated reason would still look like a working operator policy. Assert on the refusal message, as the allowedTools cases do with /not permitted.*allowedTools/.
  • Low severity, maintainability, test/mcpl-toolsets-allowed-tools.test.ts:47-72. Fixture copies the Quiet module and the makeFramework setup instead of extending the shared helper. test/helpers/toolset-servers.ts already has a Quiet module (lines 155-167) and makeFramework (lines 170-189), which creates the temp directory, MockMembrane and 'scout' agent. withGateFixture rewrites both inline, line for line, only because makeFramework can't take allowedTools or codeExecution. Extending makeFramework, for example with optional agent and framework config overrides, and exporting Quiet would avoid a third copy of this setup drifting from the others.

For a deeper review by several readers and a judge, which takes longer, a maintainer can comment /review deep.

Comment on lines +150 to +164
await withGateFixture(async ({ framework, membrane, serverCalls }) => {
const created = await framework.createEphemeralAgent({
name: 'worker', model: 'test-model', systemPrompt: 'Do the task.',
allowedTools: ['arc--status', 'toolsets--act'],
});
created.contextManager.addMessage('user', [{ type: 'text', text: 'Run once.' }]);
membrane.pushResponse(createMockResponse(toolCalls(), 'tool_use'));
membrane.pushResponse(createMockResponse([{ type: 'text', text: 'ephemeral done' }]));
const run = framework.runEphemeralToCompletion(created.agent, created.contextManager);
await framework.runUntilIdle();
await run;
assertOnlyNamedToolRan(resultsOf(membrane), serverCalls());
});
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium severity, test coverage. Ephemeral case can't tell whether the ephemeral agent's own allowedTools is applied.

The ephemeral worker gets allowedTools ['arc--status','toolsets--act'], and the parent agent 'scout' gets the fixture default ['arc--status','toolsets--act','code_execution']. Neither list includes arc--peek. Suppose the gate for an ephemeral run checked scout's list, or some other agent's list, instead of the worker's. The act call would still be refused with the same 'arc--peek … not permitted … allowedTools' text, so the test would still pass. To pin that the worker's own policy is applied, make the two lists differ: for example, allow arc--peek on scout but not on the worker, or add a variant where the worker allows arc--peek and the call runs.

Comment on lines +121 to +165
});

test('a code_execution script calling act cannot run a dynamic tool left out of its allowedTools', async () => {
await withGateFixture(async ({ framework, membrane, serverCalls }) => {
const code = [
'const control = await tools["arc--status"]({});',
'let outcome = "unexpected target success";',
'try { await tools["toolsets--act"]({ server: "arc", tool: "peek", args: {} }); }',
'catch (error) { outcome = String(error.message); }',
'text(control.content[0].text + "\\n" + outcome);',
].join('\n');
membrane.pushResponse(createMockResponse([
{ type: 'tool_use', id: 'script', name: 'code_execution', input: { code } },
], 'tool_use'));
membrane.pushResponse(createMockResponse([{ type: 'text', text: 'script done' }]));
framework.pushEvent({ type: 'external-message', source: 'test', content: 'go', metadata: {} } as never);
await framework.runUntilIdle();
const cm = framework.getAgent('scout')!.getContextManager();
await waitFor(() => cm.getAllMessages().some(message => JSON.stringify(message.content).includes('script done')), 'script turn settled');
const outer = resultsOf(membrane).find(result => result.toolUseId === 'script');
assert.ok(outer, 'the script answered its model');
assert.equal(Boolean(outer.isError), false, outer.content);
assert.match(outer.content, /SERVER RAN status/);
assert.deepEqual(serverCalls(), ['status'], 'only the permitted script call reached the server');
assert.match(outer.content, /arc--peek.*not permitted.*allowedTools/);
});
});

test('an ephemeral agent calling act cannot run a dynamic tool left out of its allowedTools', async () => {
await withGateFixture(async ({ framework, membrane, serverCalls }) => {
const created = await framework.createEphemeralAgent({
name: 'worker', model: 'test-model', systemPrompt: 'Do the task.',
allowedTools: ['arc--status', 'toolsets--act'],
});
created.contextManager.addMessage('user', [{ type: 'text', text: 'Run once.' }]);
membrane.pushResponse(createMockResponse(toolCalls(), 'tool_use'));
membrane.pushResponse(createMockResponse([{ type: 'text', text: 'ephemeral done' }]));
const run = framework.runEphemeralToCompletion(created.agent, created.contextManager);
await framework.runUntilIdle();
await run;
assertOnlyNamedToolRan(resultsOf(membrane), serverCalls());
});
});

test('operator tool policy controls dynamic visibility and act calls', async (t) => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium severity, test coverage. code_execution and ephemeral paths are only tested for denial, never for a permitted dynamic call.

The code_execution and ephemeral tests only check that act is refused when arc--peek is missing from allowedTools. Only the server-policy test covers the permitted case (allowedTools includes arc--peek), and only for a model call and a direct executeToolCall. Suppose the gate were tightened so that toolsets--act is always refused inside scripts or ephemeral runs, with the same message. Every test here would still pass. Add a case for each path where arc--peek is allowed and assert that 'SERVER RAN peek' comes back and reaches the server.

A dynamic tool invoked through `toolsets--act` follows the calling agent's `allowedTools`. The script cases now check both permitted and denied calls with a named permitted control. The ephemeral cases give the resident and worker different allowlists, so the worker's own list decides; each case checks its caller-visible result and the local peer's tool calls.

This test-only review round changes no production code or changelog text.

Co-Authored-By: GPT-6 Sol <noreply@openai.com>
nissa-seru pushed a commit to nissa-seru/agent-framework that referenced this pull request Oct 26, 2026
@nissa-seru
nissa-seru merged commit 9f02824 into anima-research:main Oct 26, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog Exempts this PR from the changelog check (internal-only change)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant