FSM: union allow_always grants into action toolset (fixes #280)#302
Open
odk- wants to merge 7 commits into
Open
FSM: union allow_always grants into action toolset (fixes #280)#302odk- wants to merge 7 commits into
odk- wants to merge 7 commits into
Conversation
`request_tool_access` advertised itself as a permission gate, but the toolset for an LLM action was snapshotted by `buildTools` and never reconsulted ToolAccessGrants. Authors granting `allow_always` for a workspace MCP tool got `granted: true`, then watched the next action fail to call the tool — the grant existed in storage but no production code path read it. Fixes the gap for future actions: - New `ToolAccessGrants.listForWorkspace` enumerates persistent grants by workspace via a KV prefix scan. - `buildTools` queries it once per action and unions any granted tool already present in `filtered` (post-PLATFORM_TOOL_ALLOWLIST) into `scoped`. Bypass mode skips the query — it already widens past per-agent narrowing. - vitest.setup bootstraps + binds the tool-access-grant storage so every suite that builds tools through the FSM engine has the adapter available. The `allow_once` case still cannot widen the current action (mid-stream toolset rebuild is a separate workstream, tracked in the PR). SKILL.md is rewritten to stop claiming "the same action continues" and to point at delegation / failStep as the route-around path.
odk-
force-pushed
the
fix/280-union-allow-always-grants
branch
from
a17931b to
6a4c62c
Compare
…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.
5 tasks
…tics PR #302 puts the burden of "approval signal vs. permission grant" on SKILL.md and the tool description — language. Unit tests prove the backend wiring; they can't prove the LLM actually follows the framing. Two new first-principles scenarios cover the model behavior: - `allow-once-route-around-check` — action calls `request_tool_access` for `fake_inbox/modify_message_labels`, harness answers `allow_once`, scenario asserts the model emits `persistent: false` in the structured reply AND does not call `modify_message_labels` in the same action (route-around path). - `allow-always-grant-check` + `allow-always-future-action-check` — step 1 calls `request_tool_access`, harness answers `allow_always`, asserts `persistent: true` + no in-action call to the tool; step 2 triggers a separate action in the same workspace and asserts the LLM DOES call `modify_message_labels` directly (no second `request_tool_access`), proving the grant union widens future actions end-to-end through the live daemon. Modeled on the existing `blocking-elicitation-check` pattern in the same fixture; no new MCP server / harness scaffolding. Workspace.yml gains three signals + three jobs; scenario file gains two runners wired into the main dispatch list between the blocking-elicitation and request-user-decision scenarios.
Collaborator
|
@odk- could the mechanism actually be for |
#297's refactor in main changed materializeFixture to take fridayHome as the first argument so the harness can place fixtures under the daemon's own home. The new allow_once / allow_always scenarios from PR #302 were still calling the old 2-arg form; merging main surfaced two TS2554 errors at typecheck. Pass `d.fridayHome` through, matching every other call site in the file. Pure mechanical fix — no behavior change.
Contributor
Author
I don't think so. If you allow it, then export the workspace and I import it does it mean I get it auto allowed? If it should ask on import then we can also have it without storing other ppl decisions in the workspace.yml. |
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
2 participants
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
request_tool_accessadvertised itself as a permission gate but never affected what tools the LLM could actually call.buildTools(packages/fsm-engine/fsm-engine.ts) snapshots the toolset once per action and ignoredToolAccessGrants; the grant store was only consulted byrequest_tool_accessitself to short-circuit duplicate elicitations. A user whoallow_always'dworkspace-mcp/create_taskwould seegranted: true, then watch the next action's tool call fail because nothing in production code read that grant.This PR closes the gap end-to-end across four commits.
What changes
1.
ToolAccessGrants.listForWorkspace+ storage server-awarenesslistForWorkspacemethod on the storage adapter + facade. KV prefix scan overkeySegment(workspaceId).*, drains keys before fetching values (matching thejetstream-adapterpattern to dodge the nats.js v2.29 iterator-termination bug).ToolAccessGrantSchemagains an optionalserverIdfield. Additive; legacy grants keep deserializing.grantAlwaysderivesserverIdfrom a qualifiedtoolNamewhen the caller doesn't pass one explicitly. KV key derivation is unchanged (still based on the fulltoolName), sohasGrantlookups by the original LLM-facing string keep resolving both legacy and new grants.listForWorkspacereturnsListedGrant[]with{ toolName, bareToolName, serverId? }.bareToolNameis whatmcpResult.toolsis keyed by;serverIdis the source MCP server when known.2.
buildToolsunion + eager-loadeffectiveConfigschoice so the engine can react to grants before MCP servers are loaded.effectiveConfigsbeforecreateMCPTools. Closes the "strictly-qualified action + grant for a different server's tool" failure mode — a grant forgmail/send_emailnow works even when the action declares onlygoogle-calendar/list_events.scopedwith any granted tool the engine actually loaded. serverId-aware attribution: a grant withserverId: "gmail"only widens whenmcpResult.toolsByServer.gmailactually 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-filteredcheck.dangerouslySkipAllowlist) skips the query — it already widens past per-agent narrowing, so the union would be redundant. Failure to read grants → warn-log + continue with the un-widened toolset; no crash, no escalation.3.
request_tool_accessdistinguishesallow_oncevsallow_alwaysfor the LLMpersistent: booleanis now first-class on every answered/bypass/persistent_allow response shape — previously present only whentrue. The LLM branches on a single field instead of parsinganswer.grantTypelands in the operator info-log (allow_once/allow_always/bypass/persistent_allow/deny) so audits don't need to join against the elicitation store.packages/system/agents/workspace-chat/tools/request-tool-access.ts).persistentand behave accordingly.4. SKILL.md rewrite
request_tool_accessas an approval signal:allow_oncerequires routing around in-action;allow_alwaysmakes the tool available from the next action onward.persistent+ the operatorgrantTypelog field.fs_write_file(ambient platform tool, no narrowing applies) to a workspace MCP tool, which is where the narrowing actually applies.5.
vitest.setup.tsBootstraps + binds the tool-access-grant adapter so every suite that exercises
buildToolsend-to-end has the storage facade available.Test plan
deno task typecheck— greendeno task test— 6495 / 6514 pass, no regressions; the 19 skipped are pre-existingdeno run -A npm:@biomejs/biome check— 0 errorstoolName, and explicit-serverIdoverride.grant-union.test.tscovers (10 cases):tools:array (hasNameAllowlist === false) → no-op unionCompat
serverIdfrom a qualifiedtoolNamewhen feasible.hasGrantkey derivation unchanged — back-compat with both legacy and new grants.request_tool_accessresponse now always carriespersistent: boolean. Previously present only whentrue. Existing exact-match tests updated; no behavioural regression.Scope notes
allow_oncestill can't widen the current action. Mid-stream toolset rebuild against the AI SDK is meaningfully larger work; out of scope. SKILL.md tells authors to delegate / failStep onallow_once.apps/atlasd/routes/agents/run.tsandpackages/workspace/src/runtime.ts— neither has per-actiontools:narrowing, so grants have nothing to widen there. The fsm-enginebuildToolspath is the sole narrowing surface.Closes #280.