Repository navigation
🔓 feat: Remember Tool Approvals Per Conversation - #16557
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7becdfdad0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /** Upper bound on remembered tools per conversation; later choices are not stored. */ | ||
| export const MAX_CONVERSATION_TOOL_ALLOWS = 64; | ||
| /** Longest tool name accepted as a remembered allow. */ | ||
| const MAX_ALLOWED_TOOL_NAME_LENGTH = 256; |
There was a problem hiding this comment.
Move these newly introduced bounds into toolApproval configuration with backward-compatible defaults. Once a conversation reaches 64 remembered tools—or a provider exposes a tool name longer than 256 characters—“Always allow” silently degrades to a one-time approval, and deployments with broader tool catalogs have no way to adjust that behavior.
AGENTS.md reference: AGENTS.md:L77-L79
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cc187a9. Added toolApproval.allowAlwaysMaxTools and allowAlwaysMaxToolNameLength (defaults 64 and 256, bounded 1 to 1024). Once a conversation is at the cap the server stops offering Always allow for new tools instead of silently downgrading; an already remembered tool is still offered. Tests in allow.spec.ts.
| ? markToolApprovalAllowAlways( | ||
| markNativeCodeToolApprovalRequests(interrupt.payload, reachableAgents), | ||
| appConfig?.endpoints?.[EModelEndpoint.agents]?.toolApproval, |
There was a problem hiding this comment.
When an admin ask pattern matches only an MCP tool's alternate spelling, createRun heals that pattern before evaluating the tool, so the call pauses, but this code checks the raw endpoint policy and incorrectly offers “Always allow.” Subsequent runs heal the same ask rule again, and because ask outranks the remembered allow, the user is prompted indefinitely despite selecting the option; marking and resume-time eligibility should use the same healed effective policy as the run.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cc187a9. buildEffectiveToolApprovalPolicy in allow.ts heals the policy against the reachable agents' MCP aliases (reusing healToolApprovalPolicy) and folds in remembered allows; marking and resume time eligibility both use it. Test: a call held by an ask rule written against the alternate spelling is not offered the choice and nothing is stored.
| toolApprovalAllows: resolveRunToolApprovalAllows( | ||
| agentsEConfig?.toolApproval, | ||
| this.options.req?.resolvedConversation, | ||
| this.conversationId, |
There was a problem hiding this comment.
In the AgentClient scheduled-fire path, canAgentGraphPause runs earlier against effectiveToolApprovalPolicy without these remembered allows, while only createRun receives them here. If the static policy would ask for all eagerly known tools but this conversation has remembered each of them and has no other pause source, the actual run policy would auto-approve them, yet a deployment without Redis or a durable checkpointer still rejects the schedule with SCHEDULED_HITL_REQUIRES_SHARED_STORE or SCHEDULED_HITL_REQUIRES_DURABLE_CHECKPOINT; fold the stored allows into the policy used for admission as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cc187a9. canAgentGraphPause now receives the remembered tools and evaluates the same effective policy as createRun, so a scheduled run whose known tools are all remembered is not rejected; an alias matched ask rule still makes it pausable. Covered in admission specs.
There was a problem hiding this comment.
Confirmed covered at cc187a9 and still at 881e833: client.js passes the remembered allows into canAgentGraphPause, which evaluates the same buildEffectiveToolApprovalPolicy as createRun. Tested by 'lets scheduled admission skip a run whose every known tool is remembered', and the new invariant spec asserts admission for every row where the offer is made.
| await recordToolApprovalAllows({ | ||
| userId, | ||
| conversationId, | ||
| policy: req.config?.endpoints?.[EModelEndpoint.agents]?.toolApproval, | ||
| pendingAction, | ||
| resolutions: req.body.decisions, | ||
| request: req, | ||
| addConvoToolApprovalAllows, | ||
| }); |
There was a problem hiding this comment.
Persist the remembered approval only after the schedule and project-context fences below have accepted the continuation. A schedule edit can make finalizeScheduleResumeClaim reject, and a project move can make rejectChangedResumeProjectContext return a 409, but this write has already committed in both cases; the submitted call never runs while a later turn silently auto-approves that tool despite the failed response.
AGENTS.md reference: AGENTS.md:L42-L44
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cc187a9. The store now runs after finalizeScheduleResumeClaim, rejectChangedResumeProjectContext, initializeClient and beginProviderExecution. resume.spec.js asserts nothing is stored on a schedule rejection or a 409 project context rejection, and that a storage failure still resumes once.
danny-avila
added
the
🗺️ Agent Runtime
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc187a9e13
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ( | ||
| !config.allowed_decisions.includes('approve') || | ||
| !isToolAllowAlwaysEligible(effective, name) |
There was a problem hiding this comment.
When a programmatic or plugin PreToolUse hook is what returned ask—for example, a per-argument payment check—while the static policy does not ask or deny this tool, this predicate still marks the card allow_always. Selecting it only adds the name to the static allow list; createRun continues to register the same hook with ask taking precedence, so the next matching call pauses again despite the UI promising it will run without asking. The current alias-healing change is fresh evidence for this separate path: the marker now sees healed static lists, but it is still called without the resolved or plugin hook information. Suppress this offer for hook-governed pauses or carry the actual pause provenance into the payload.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 881e833. The interrupt payload does not record which hook asked (the request only carries source for native code tools), so the offer is now suppressed whenever a registered programmatic or plugin PreToolUse hook could apply to any spelling of the tool (toolApprovalHookCanApply in allow.ts, built with the same request context as the run). client.js only passes hookContext and pluginHookSource. Covered by the new invariant spec's hook cases.
| aliases: readonly MCPToolAlias[], | ||
| allowedTools?: readonly string[], | ||
| ): TToolApprovalPolicy | undefined { | ||
| return applyConversationToolAllows(healToolApprovalPolicy(policy, aliases), allowedTools); |
There was a problem hiding this comment.
When an MCP tool switches between its legacy prefixed key and current stripped key—for example after an agent is resaved against the current catalog—the conversation retains the spelling that was approved. This function heals the endpoint policy before adding those stored entries, so the stored spelling is never expanded through aliases; the equivalent runtime name then falls through to ask and the supposedly remembered tool prompts again. Normalize remembered names through the alias map, or add them before healing, while retaining deny/ask precedence.
AGENTS.md reference: AGENTS.md:L42-L44
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 881e833. Remembered names now expand through the exact MCP alias pairs (getEquivalentToolNames) into allow entries for every spelling, and a tool is eligible only if every spelling is, so deny and ask on either spelling still win and similarly named tools are never included. Covered by legacy vs stripped cases in the invariant spec 'Always allow is offered only when the next identical call is auto-approved' (12 cases, each running offer, record and the real hook wiring through the SDK executeHooks).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 881e833fb4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eed374ac38
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { | ||
| policy: appConfig?.endpoints?.[EModelEndpoint.agents]?.toolApproval, | ||
| agents: reachableAgents, |
There was a problem hiding this comment.
When a lazy subagent resolves an MCP tool whose alternate spelling matches an admin ask rule or programmatic-hook matcher, createRun adds that alias only to its run-local mcpToolAliases, while reachableAgents here still contains the unresolved descriptor. The live run therefore pauses using the healed rule, but markToolApprovalAllowAlways does not see the alias and offers “Always allow”; the resume check has the same blind spot, stores the name, and the rebuilt run rediscovers the alias and prompts again. Unlike the earlier fixed eager-alias path, the fresh evidence is the separate registerResolvedMCPToolAliases path in createRun; propagate those runtime aliases or pause provenance into marking and resume validation.
AGENTS.md reference: AGENTS.md:L42-L44
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in dae5445. createRun now keeps its live alias list (including aliases reported by lazily resolved subagents through registerResolvedMCPToolAliases) per run, and the interrupt passes getRunMCPToolAliases(this.run) into markToolApprovalAllowAlways, so the offer is judged against the same healed rules the paused run used. Resume only stores a name the pause offered, so it inherits the narrower offer. allow.spec.ts: 'heals against aliases the run discovered after its agents were collected'.
| const stored = new Set(storedTools); | ||
| let room = getToolAllowAlwaysMaxTools(policy) - stored.size; |
There was a problem hiding this comment.
When an operator lowers allowAlwaysMaxTools below the number already stored, applyConversationToolAllows honors only the first max entries, but this code puts the entire stored array in stored. A tool beyond that active prefix therefore pauses yet is treated as already remembered and is still offered “Always allow” even with negative room; persistence then rejects the over-cap array and the next identical call prompts again. Limit the stored set to the same active prefix used by the run policy before deciding whether an offer will hold.
AGENTS.md reference: AGENTS.md:L42-L44
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in dae5445. markToolApprovalAllowAlways now counts only storedTools.slice(0, allowAlwaysMaxTools), the same prefix applyConversationToolAllows honors, so a tool past a lowered cap is neither treated as remembered nor offered. allow.spec.ts: 'counts only the stored prefix the run honors after the cap is lowered'.
eed374a to
1b8feed
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b8feed196
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const aliases = collectAgentAliases(agents); | ||
| const eligible = collectToolApprovalAllows( | ||
| payload, | ||
| resolutions as Agents.ToolApprovalResolution[], | ||
| buildEffectiveToolApprovalPolicy(policy, aliases), | ||
| aliases, |
There was a problem hiding this comment.
When a paused lazy-subagent tool was offered “Always allow” and an administrator adds an ask/deny rule for its alternate MCP spelling before resume, this recheck still sees only aliases on the rebuilt, unresolved reachableAgents; the original run’s aliases from registerResolvedMCPToolAliases are not persisted in the pending action. The approval is consequently stored, but the resumed run rediscovers the alias, heals the new rule, and prompts again. The fresh evidence beyond the earlier lazy-alias finding is that marking now receives getRunMCPToolAliases(this.run), while this resume path still derives aliases solely via collectAgentAliases(agents); carry the pause-time aliases into resume validation or resolve them before storing.
AGENTS.md reference: AGENTS.md:L44-L47
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bad8c44. The interrupt now stores the paused run's alias pairs for the tools it offered (collectAllowAlwaysAliases over getRunMCPToolAliases(run)) as a server-only pendingAction.toolApprovalAliases, stripped by toClientPendingAction, and recordToolApprovalAllows unions them with the rebuilt agents' aliases before rechecking, so an ask/deny rule added mid-pause on the lazy alternate spelling makes the approval one-time. allow.spec.ts 'rechecks against the alias pairs the paused run kept for its offers'; policy.spec.ts asserts the field never reaches the client.
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Persistent auto-approval crosses security-sensitive policy, storage, resume, and execution paths and warrants final human validation.
Review effort: Balanced
Findings: None
What changed in this PR
Adds conversation-scoped tool approval persistence across the data, agent runtime, resume, and client layers.
Changes:
- Stores bounded, owner-scoped remembered tool approvals.
- Enforces policy, alias, hook, and resume eligibility server-side.
- Adds the “Always allow” UI and comprehensive unit/E2E coverage.
| File | Description |
|---|---|
packages/data-schemas/src/types/convo.ts |
Adds the persisted approval list type. |
packages/data-schemas/src/schema/convo.ts |
Defines conversation storage. |
packages/data-schemas/src/methods/conversation.ts |
Adds bounded storage and write protection. |
packages/data-schemas/src/methods/conversation.spec.ts |
Tests storage invariants. |
packages/data-provider/src/types/agents.ts |
Extends approval wire types. |
packages/data-provider/src/config.ts |
Adds configuration options. |
packages/api/src/agents/run.ts |
Applies remembered approvals to runs. |
packages/api/src/agents/hitl/resume.ts |
Validates remembered scopes. |
packages/api/src/agents/hitl/policy.ts |
Persists and hides alias metadata. |
packages/api/src/agents/hitl/policy.spec.ts |
Tests client-safe projection. |
packages/api/src/agents/hitl/index.ts |
Exports approval helpers. |
packages/api/src/agents/hitl/allow.ts |
Implements eligibility and persistence policy. |
packages/api/src/agents/hitl/allow.spec.ts |
Tests policy invariants. |
packages/api/src/agents/hitl/admission.ts |
Includes remembered tools in pause admission. |
librechat.example.yaml |
Documents the feature toggle. |
e2e/specs/mock/scenarios/tool-allow-always.spec.ts |
Covers refusal scenarios end-to-end. |
e2e/config/librechat.e2e.yaml |
Enables the feature for E2E. |
client/src/utils/approval.ts |
Projects offer eligibility onto tool calls. |
client/src/locales/en/translation.json |
Adds localized UI copy. |
client/src/components/Chat/Messages/Content/ToolApproval.tsx |
Adds the selectable approval scope. |
client/src/components/Chat/Messages/Content/ApprovalContext.tsx |
Stores scope in decision drafts. |
client/src/components/Chat/Messages/Content/__tests__/ToolApproval.test.tsx |
Tests UI behavior and submission. |
client/src/components/Chat/approval/Review.tsx |
Passes eligibility into composer cards. |
api/server/controllers/agents/resume.js |
Records approvals after resume fences. |
api/server/controllers/agents/client.js |
Wires approvals through run lifecycle. |
api/server/controllers/agents/__tests__/resume.spec.js |
Tests route ordering and failures. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Adds an opt-in "Always allow" choice to the tool approval card. When endpoints.agents.toolApproval.allowAlways is true, the server marks eligible paused calls with allow_always; approving with scope "session" stores that exact tool name on the conversation, and later runs in the same conversation fold the stored names into the policy as exact-name allow entries. The server enforces every step: the resume route rejects a session scope the pause did not offer, eligibility is rechecked against the live policy before storing, the list is written only through an owner-scoped bounded method, and generic saves and imports cannot set or clear it. Admin deny and ask rules and programmatic hooks still win, stored names are ignored under dontAsk or when the flag is off, and native code tools stay under the code approval picker. The flag defaults to false, so existing deployments keep prompting.
…ume Fences Offering, resume re-check, scheduled admission and createRun now read one policy from buildEffectiveToolApprovalPolicy: endpoint rules healed against MCP alias spellings, then the conversation's remembered tools. A call held by a healed ask rule is no longer offered "Always allow", and a scheduled fire whose known tools are all remembered is no longer rejected as pause capable. The remembered-tools cap and name length cap move to toolApproval.allowAlwaysMaxTools and allowAlwaysMaxToolNameLength (defaults 64 and 256). The card stops offering the choice for new tools once the cap is reached instead of silently approving once. The resume route stores remembered tools only after the schedule and project context fences pass; a storage failure still approves the call once.
Offer allow_always only when storing the name would auto-approve the next identical call on the run's own path: every MCP spelling of the tool passes the healed static policy and no programmatic or plugin approval hook can apply to it (the pause payload does not record which hook asked). Remembered names now expand to every alias spelling of the same tool, and a deny or ask on any spelling keeps the tool ineligible. A table-driven spec walks the invariant through the real hook wiring.
The resume-time eligibility recheck now applies the same programmatic and plugin PreToolUse hook check as marking, with a hook context built from the resume request and the live plugin hook source. A hook registered after the pause turns the choice into a one-time approval: nothing is stored and the resume continues. Document that toolApproval.allowAlways should be enabled only once every replica supports it, since older replicas ignore the choice.
bad8c44 to
25e0c32
Compare
Summary
Every paused tool call asks for approval again, even when the user already approved the same tool several times in the same conversation. Admins can write allow rules in
librechat.yaml, but users have no way to say "stop asking for this tool here".With the new
endpoints.agents.toolApproval.allowAlwaysoption enabled, the approval card offers "Always allow" for eligible calls. Choosing it approves the call and stores that exact tool name on the conversation (MCP tool names include their server), and later runs in the same conversation add it to the policy as an exact-name allow entry. The server enforces each step: the resume route rejects an "always" scope the pause did not offer, eligibility is rechecked against the current policy before storing, and the list is written only through an owner-scoped, bounded method that generic saves and imports cannot touch. Admin deny and ask rules and programmatic hooks still win, stored names are ignored underdontAskor when the flag is off, and native code tools stay under the code approval picker. Offering and resume eligibility, scheduled pause admission and the run itself all read one effective policy (admin rules healed against MCP aliases, plus remembered tools), the choice is stored only after every resume fence passes, andallowAlwaysMaxTools/allowAlwaysMaxToolNameLength(defaults 64 and 256) bound the list; at the cap the choice is no longer offered for new tools. The choice is not offered when a programmatic or plugin approval hook could still ask for the tool, and remembered names cover every MCP alias spelling of the same tool. The flag defaults to false, so existing deployments keep prompting.Type of change
Testing
Exercised the real SDK policy hook with stored names against admin deny, ask and allow rules, MCP names from two servers, wildcard names,
dontAskand the flag turned off; exercised the resume route with offered and unoffered scopes and a storage failure.Tested environments/configuration:
endpoints.agents.toolApproval.allowAlways: trueand unsetAutomated tests:
packages/api:npx jest src/agents/hitl: 376 of 377 passed, including an invariant spec that runs offer, record and the real hook wiring for 12 cases; the one failure,hookLoader.spec.ts"continues past a bad entry to load the good ones", fails identically on an unmodified canary checkoutapi:resume.spec.js: 167 passed, including fence rejections that store nothingclient:ToolApproval.test.tsx: 10 passednpx tsc --noEmitclean inpackages/data-provider,packages/data-schemas,packages/apiandclientallow.spec.ts, resume route cases, approval card cases, andconversation.spec.tscases for the storage method; the latter could not run locally because MongoMemoryServer fails to start in this environment (including on untouched specs), so CI is their first runScreenshots / recordings
Captured from the running app with a seeded paused MCP call carrying
allow_always; before is canarybddb8f8ab, after is this branch.Dark mode, after:
Risk / compatibility
Adds optional
allowAlways,allowAlwaysMaxToolsandallowAlwaysMaxToolNameLengthfields to the tool approval config and an optional field on conversations; no migration. Security-sensitive: the server never trusts the client's scope without the pause having offered it, deny and ask rules always take precedence, and a rejected resume stores nothing.Checklist
toolApproval.allowAlwaysstill to be written