feat(agent): allow variable tool permission modes - #7538
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
@cubic-dev-ai review this PR |
@j15z I have started the AI code review. It will take a few minutes to complete. |
Greptile SummaryThis PR adds variable-backed Agent tool permission modes, resolves them before provider execution, preserves them across workflow editing and serialization, and introduces a global feature flag intended to gate authoring and persistence.
Confidence Score: 4/5The PR is not safe to merge until variable permission modes are rejected at the realtime persistence boundary and the explicit component rules are satisfied. Application-level saves enforce the new global flag, but realtime state replacement independently writes the gated fields without validation, allowing disabled configurations to persist; the new picker also violates two mandatory repository UI-structure requirements. Files Needing Attention: apps/sim/lib/workflows/tool-input/usage-control.server.ts, apps/realtime/src/database/operations.ts, apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/tool-input/components/tools/usage-control.tsx
|
| Filename | Overview |
|---|---|
| apps/sim/lib/workflows/tool-input/usage-control.server.ts | Adds the global variable-mode write assertion, but the assertion is not shared with realtime workflow persistence. |
| apps/realtime/src/database/operations.ts | The existing realtime replacement sink remains able to persist mode-bearing block state without the newly introduced feature gate. |
| apps/sim/executor/handlers/agent/agent-handler.ts | Resolves active tool permission expressions before filtering and provider request construction. |
| apps/sim/lib/workflows/tool-input/usage-control.ts | Defines indexed canonical-mode keys and normalizes active permission values. |
| apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/tool-input/components/tools/usage-control.tsx | Adds the fixed/variable permission-mode editor but violates component-configuration and canonical picker rules. |
| apps/sim/lib/workflows/editing/builders.ts | Preserves expressions and reconstructs indexed canonical modes while normalizing workflow edits. |
| apps/sim/lib/api/contracts/v2/workflows.ts | Extends all Agent tool contract variants with a bounded permission-mode expression. |
| apps/sim/lib/workflows/search-replace/indexer.ts | Indexes the permission expression only when its advanced canonical mode is active. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Editor[Workflow editor] --> Queue[Realtime operation queue]
Queue --> Realtime[Realtime replace-state handler]
Realtime -->|Direct write lacks new gate| DB[(Workflow blocks)]
API[API / CLI / Copilot edits] --> AppGate[Agent tool permission-mode flag assertion]
Import[Workflow import] --> AppGate
AppGate --> Save[Normalized workflow save]
Save --> DB
DB --> Executor[Agent executor]
Executor --> Resolve[Resolve variable permission mode]
Resolve --> Filter[Remove tools resolving to none]
Filter --> Provider[Provider request]
Reviews (1): Last reviewed commit: "fix(agent): gate variable permission mod..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| export async function assertAgentToolPermissionModeEnabled( | ||
| blocks: Iterable<Pick<BlockState, 'type' | 'subBlocks' | 'data'>> | ||
| ): Promise<void> { | ||
| for (const block of blocks) { | ||
| if (block.type !== 'agent') continue | ||
| const tools = block.subBlocks?.tools?.value | ||
| if (!Array.isArray(tools)) continue | ||
|
|
||
| const hasVariableMode = tools.some( | ||
| (tool, index) => | ||
| getAgentToolUsageControlMode(index, block.data?.canonicalModes) === 'advanced' || | ||
| (isRecordLike(tool) && tool.usageControlExpression !== undefined) | ||
| ) | ||
| if (!hasVariableMode) continue | ||
|
|
||
| if (!(await isFeatureEnabled('agent-tool-permission-mode'))) { | ||
| throw new OrchestrationError( | ||
| 'validation', | ||
| 'Variable agent tool permission modes are disabled' | ||
| ) | ||
| } | ||
| return |
There was a problem hiding this comment.
This assertion protects the Sim application's workflow-save wrappers, but realtime replace-state writes the same block data and subBlocks directly without applying it. A client can therefore persist usageControlExpression and advanced canonical modes through collaboration while the global flag is disabled. Enforce the same validation at the realtime persistence boundary or move it into a shared boundary used by both services.
Knowledge Base Used:
| const MODE_OPTIONS = [ | ||
| { | ||
| value: 'auto', | ||
| label: 'Auto', | ||
| suffixElement: <span className='text-[var(--text-tertiary)]'>(model decides)</span>, | ||
| }, | ||
| { | ||
| value: 'force', | ||
| label: 'Force', | ||
| suffixElement: <span className='text-[var(--text-tertiary)]'>(always use)</span>, | ||
| }, | ||
| { | ||
| value: 'none', | ||
| label: 'None', | ||
| suffixElement: <span className='text-[var(--text-tertiary)]'>(disable tool)</span>, | ||
| }, | ||
| ] as const |
There was a problem hiding this comment.
MODE_OPTIONS defines component properties directly in the rendered component module. This violates the repository directive that component properties must live in a dedicated .ts configuration file while rendered components remain in their component files. This repository requirement must be satisfied before merging.
Rule Used: When defining properties for components, use a ded... (source)
Learned From
simstudioai/sim#367
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| <Combobox | ||
| options={MODE_OPTIONS.map((option) => ({ | ||
| ...option, | ||
| disabled: option.value === 'force' && !supportsForce, | ||
| suffixElement: | ||
| option.value === 'force' && !supportsForce ? ( | ||
| <span className='text-[var(--text-tertiary)]'>(not supported by model)</span> | ||
| ) : ( | ||
| option.suffixElement | ||
| ), | ||
| onSelect: () => onFixedChange(option.value), | ||
| }))} | ||
| value={tool.usageControl ?? 'auto'} | ||
| disabled={disabled} | ||
| aria-label='Permission Mode' | ||
| /> |
There was a problem hiding this comment.
The new fixed permission-mode picker uses the legacy Combobox. This violates the repository directive to use the chip-family replacement—ChipCombobox, ChipSelect, or ChipDropdown—for new pickers. This repository requirement must be satisfied before merging.
Context Used: EMCN component library patterns with CVA (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
11c3d58 to
818dca4
Compare
818dca4 to
41af1d3
Compare
41af1d3 to
661f655
Compare
github-actions
Bot
added
the
requires-mothership-merge
|
1 participant
Summary
Let each agent tool's Permission Mode use a fixed selector or a variable such as
<start.toolMode>. Resolve variables before tool filtering and fail invalid modes before calling the provider.Put Permission Mode in the expanded tool parameters with the canonical arrow toggle. Preserve the selected mode and both inputs through collaborative editing, API/CLI edits, export, and workflow search.
Gate the editor and workflow writes behind a flag that defaults off, including imports and dry runs. Execution continues honoring previously configured variable permissions if the flag is turned off.
Preserve permission modes when API tool arrays change, compare search-replacement values independent of object key order, and report interrupted CLI responses with guidance for uncertain writes.
Show existing variable settings read-only when the flag is off and preflight browser imports before creating workflows or folders.
Companion: simstudioai/mothership#488
Type of Change
Testing
Review focus: retaining each tool's permission after array edits, keeping disabled tools out of discovery/execution, and blocking new variable authoring while honoring existing configurations. Browser import preflight prevents known disabled-feature imports from creating resources; the existing multi-request importer is not transactional for arbitrary network failures.
Checklist
Screenshots/Videos
No screenshots or videos attached.