ToolAccessGrants: server-aware grants + missing tests + LLM-readable response#304
Merged
Merged
Conversation
…t branches Adds three test cases to grant-union.test.ts that lock the contract: - Bypass mode skips the grant query entirely (spy assertion on listForWorkspace, mirroring the "redundant under bypass" claim from the originating PR). - Grant store read failure → warn-and-continue with un-widened toolset (no crash, no escalation). KV down / deserialize-edge-case path. - Action without a `tools:` array (hasNameAllowlist === false) → union is a no-op because every loaded tool already flowed through. Most common ad-hoc LLM action shape; previously untested. Refactors the mock for listForWorkspace from a closure-bound function into a vi.fn() so call-count assertions are possible. RunOptions grows two parameters (`bypass`, full `grants` envelope) without changing the existing happy-path test shapes.
… logs After #302 the two grant flavors have meaningfully different runtime semantics: `allow_always` widens future actions in the workspace via the grant union; `allow_once` is an approval signal only. The tool surface collapsed both into the same `granted: true` shape, leaving the LLM without a programmatic way to tell "this will keep working" from "this is a one-shot". - Always include `persistent: boolean` in answered / bypass / persistent responses. Previously present only when true; now first-class on every shape so the LLM branches on a single field instead of parsing `answer`. - Add `grantType` to the `info` log lines (`allow_once` / `allow_always` / `bypass` / `persistent_allow` / `deny`) so operators reading `~/.atlas/logs/global.log` can audit choices without joining against the elicitation store. - Mirror both changes in the chat-side parallel (`packages/system/agents/workspace-chat/tools/request-tool-access.ts`). - SKILL.md gains a "Reading the response" subsection that teaches the LLM to branch on `persistent` and explains the `grantType` log field. - Tool descriptions surface the `persistent` semantics in the prompt so the LLM sees the new behavior at registration time. Existing exact-match `toEqual` tests updated; no behavioural regression.
…P servers #302 left a known limitation: a grant for `gmail/send_email` only worked when the action that benefits from it happened to load gmail anyway — typically because it declared a bare tool name (forcing all workspace MCP servers to load). For strictly-qualified actions like `tools: [google-calendar/list_events]`, the gmail server stays unloaded and the grant union finds nothing to widen. This closes the gap end-to-end: - `ToolAccessGrantSchema` gains an optional `serverId` field. Additive schema change — legacy grants keep deserializing; the read path infers serverId from a qualified `toolName` when feasible. - `grantAlways` derives `serverId` from the LLM-facing tool name when the caller doesn't pass one explicitly. KV key derivation is unchanged (still based on `toolName`) so `hasGrant` lookups by the original LLM-facing string keep working for both old and new grants. - `listForWorkspace` returns `ListedGrant[]` with `{ toolName, bareToolName, serverId? }`. `bareToolName` is what `mcpResult.tools` is keyed by; `serverId` is the source MCP server when known. - `fsm-engine.buildTools` hoists the grant fetch above the `effectiveConfigs` choice. Any grant-referenced server that the workspace still declares is eagerly added to the MCP load. Stale grants (server dropped from workspace config) are silently skipped to avoid surfacing a confusing MCP error. - The union step now does serverId-aware attribution: a grant with `serverId: "gmail"` only widens when `mcpResult.toolsByServer.gmail` actually contains the bare tool name. Defense in depth against cross-server bare-name collisions. Legacy bare-name grants fall through to the existing bareToolName-in-filtered check. - `request_tool_access` (MCP-server side) passes the parsed serverId through to `grantAlways` so new grants land with the field set. Two new fsm-engine tests cover the eager-load happy path (grant-referenced server is loaded; the granted tool reaches the LLM) and the stale-grant defensive path (server not in workspace config → silently skipped). The earlier `serverId mismatch must not widen` test covers the cross-server attribution check.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stacked on #302. Three follow-ups identified during that PR's review:
hasNameAllowlist === falseshape (action without atools:array).allow_oncevsallow_alwaysis now distinguishable to the LLM —persistentis always present in the response envelope;grantTypelands in the operator log; SKILL.md teaches the LLM to read it.ToolAccessGrantSchemagainsserverId?; new grants record it;buildToolseagerly loads grant-referenced workspace MCP servers beforecreateMCPTools; the union now does serverId-aware attribution.Commits
879113e—fsm-engine: cover bypass-skip + grant-read-failure + no-narrowing test branchesfa97fc8—request_tool_access: distinguish allow_once vs allow_always for LLM + logs965c859—ToolAccessGrants: record source serverId + eagerly load referenced MCP serversEach is self-contained; can be cherry-picked independently if the stack splits.
Compat
serverIdfrom a qualifiedtoolNamewhen feasible.hasGrantkey derivation unchanged — lookups by the original LLM-facing string keep resolving both legacy and new grants.request_tool_accessresponse gainspersistent: booleanon every "answered / bypass / persistent_allow" shape. Previously present only whentrue; now first-class. ExistingtoEqual({...})tests updated; no production caller depends on the field's absence.Test plan
deno task typecheck— 0 errorsdeno task test— 6495 / 6514 pass, no regressionsdeno run -A npm:@biomejs/biome check— 0 errorsListedGrantprojectiontools:shape, serverId-aware widening, serverId-mismatch defense, eager-load happy path, eager-load skipped for stale grantKnown limitations now resolved
The "strictly-qualified action + grant for a different server's tool" gap called out in #302's PR description is closed: the action no longer needs to pull in the granted-tool's server via a bare-name declaration. Grants whose source server has been dropped from the workspace config are silently skipped to avoid surfacing a confusing MCP error — the action still loads its declared servers normally.
Base
Targets
fix/280-union-allow-always-grantsso the diff is just the follow-up delta. Once #302 lands, rebase onto main and re-target there.