Repository navigation
test(mcpl): cover allowedTools and server policy for dynamic calls - #809
Conversation
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>
There was a problem hiding this comment.
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 saystoolsets--actcalls a dynamic tool "down the same path as a direct call of it (the agent'sallowedTools, 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
allowedToolsis 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:
- Medium severity, test coverage. Ephemeral case can't tell whether the ephemeral agent's own allowedTools is applied.
- Medium severity, test coverage. code_execution and ephemeral paths are only tested for denial, never for a permitted dynamic call.
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.
| 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()); | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
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.
| }); | ||
|
|
||
| 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) => { |
There was a problem hiding this comment.
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>
This PR adds one test file and changes no other code.
Problem
#799 adds
toolsets--actfor 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
allowedToolsapplies to the dynamic tool invoked throughtoolsets--actin model calls,code_executionscripts, and ephemeral-agent runs. The host's per-serverenabledToolsanddisabledToolssettings control which dynamic tools are announced, shown bytoolsets--look, and callable throughtoolsets--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
b9a7bc83because its threeallowedToolscases catch the old defect, and passes at mergedmain8aaee1ca. The server-policy cases pass at both heads and pin behavior that already held.Tests
051cfd1939195261a833b8d56bb12bb1209cb68a) with releasemain0a4a2c69(treeae6ee8a4),npm run buildandnpm run typecheckpassed on Linux with Node 24.22.1 and Bun 1.4.4. The focused test file passed 11 of 11.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 teston 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 head8aaee1cahad 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.
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-seruaccount.