mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-04 14:57:42 +00:00
4860 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Danny Avila
|
104bbc8633 |
🔓 fix: Don't Lock Skills Out of Saving Over Pre-Existing Malformed Flags
Three findings from the self-review's completeness pass. A skill whose STORED SKILL.md already carried a malformed flag became unsavable. `validateBodyDerivedColumns` ran on every body-carrying update, and the skill editor resubmits the whole file on every save, so a body containing `user-invocable: yes` rejected edits to unrelated fields — the user could not even fix the description. Those documents exist precisely because pre-fix import returned 201 for that value, and `yes`/`no`/`on`/`off` read as booleans in YAML 1.1, so it is an ordinary authoring shape. The stored-body scan is now taken before validation and a flag that was already malformed no longer blocks the save; a value this edit introduces is still rejected. The end-to-end spec never ran in CI: `test:ci` ignores `\.*integration\.`, and the named integration scripts are scoped to cache, s3 and agents, so the only test asserting the issue's reproduction against a real `createSkill` was skipped everywhere. Renamed to `import.db.spec.ts`, matching the several packages/api specs that already boot MongoMemoryServer in the default run. Parity between the two frontmatter readers was asserted only in a commit message. `parity.db.spec.ts` now feeds 23 shapes through both the upload path and the inline-body path and requires identical columns, with the two known asymmetries pinned as their own cases: tab indentation, which only the YAML parser rejects, and duplicate keys differing in case, which the file leaves ambiguous. Neither can release a restriction. Skill validation failures now carry `message`, as the import handler already did. The client falls back to a generic string when it is absent, so a rejected flag line was previously undiagnosable from the UI. |
||
|
Danny Avila
|
1d139666ae |
🧯 fix: Let Only the Body's Own Prior Declaration Release a Restriction
Self-review found the previous guard was the wrong shape. Gating on "the new body has a frontmatter block" still released a bag-only restriction whenever the edited body happened to carry a block, and any key the body reader cannot see (a quoted `"user-invocable":`) looked like a removal too. Both are instances of one class: treating the reader's silence as a declaration. A body edit may now only remove a flag that the STORED body declared, read back under the same version guard that protects the write. An edit can therefore release what the author wrote into the file, a skill whose flags were never in the text keeps them, and a key the reader misses is invisible on both sides of the comparison — so its blind spots degrade to no-ops instead of silent releases. The structured-bag contract is untouched: a bag that omits a key still removes it, which stays the escape hatch for flags the body never had. Two reader divergences fixed with it, both confirmed against the real modules: the body scanner now unquotes keys, so `"user-invocable": false` is honored the way the importer already honored it; and an empty flag value is a placeholder rather than a malformed boolean even when the line scan finds nothing, which an indented mapping or a quoted key can cause. A corpus of 26 frontmatter shapes now runs through both the import path and the inline-body path with identical columns in 25 — the residual is duplicate keys differing only in case, where the file is ambiguous by construction (js-yaml takes the last, the line reader the first) and neither answer can release a restriction. |
||
|
Danny Avila
|
0c5cd2ed84 |
🛡️ fix: Require a Frontmatter Block Before a Body Edit Releases a Restriction
Self-review caught a data-loss regression in the body cascade. A skill whose invocation flags live only in the frontmatter bag — the pre-Phase-6 shape `backfillDerivedFromFrontmatter` exists for, and what a caller setting flags through the API alone produces, since the bag need not be repeated in the SKILL.md text — had its restriction silently lifted by any body edit. Verified against the real methods: a `disable-model-invocation: true` skill came back `undefined` on both columns after an unrelated body rewrite, quietly exposing it to the model. The body now only counts as declaring these flags when it actually carries a YAML frontmatter block. A block that omits the key is still a declaration, so the release path this PR added keeps working for imported skills; a body with no block at all declares nothing and leaves the columns and the bag alone. Losing a restriction silently is worse than keeping one an edit longer. `alwaysApply` keeps its existing "no frontmatter block means opt out" contract — it is opt-in and defaults to false, so absence there cannot lose a restriction. |
||
|
Danny Avila
|
286758449d |
🩹 fix: Keep Skill Parser Mock-Safe and Read Continued Body Flags
Three follow-ups on the invocation-mode work. CI: parse.ts built its key lookup at module scope from a `SKILL_BOOLEAN_FLAGS` value imported out of data-schemas. Suites that replace that module with a partial mock (agents/openai/service.spec.ts mocks it as `logger` alone) left the import undefined, so the map construction threw before any test ran and took six `api` suites plus one `@librechat/api` shard down with it. The table is declared locally again — this module is pure text parsing and must load without the DB package initialized — and parse.test.ts asserts it still matches data-schemas. Codex P1: a body-only edit unset the derived column but left the stored frontmatter bag's copy in place, so `backfillDerivedFromFrontmatter` read the restriction back on the next `getSkillByName` and the release undid itself. A body-driven update now clears the bag's flag keys, handing authority to the columns; the SKILL.md body still carries the declarations. Codex P2 / Copilot: the body scanner treated `user-invocable:` with its value on the following line as an unwritten placeholder, and skipping every indented line also blinded it to a frontmatter block indented as a whole. It now reads keys at the mapping's own indentation and follows a lone indented scalar as a continuation value, matching what the import/sync parser already accepted. |
||
|
Danny Avila
|
1e1f751e92 |
🎛️ fix: Honor All Invocation-Mode Frontmatter Fields on Skill Import and Inline Edits
`POST /api/skills/import` silently discarded `user-invocable` and `disable-model-invocation`, returning 201 with both columns at their schema defaults. Those columns derive only from the structured `frontmatter` bag, and import never passed one, so the flags had no channel to reach the document. `always-apply` survived because it also travels an explicit column param and a body-parse fallback. Import now passes a sanitized bag, and the body-level cascade that previously served only `always-apply` covers all three flags, so a flag declared inline is honored on `POST`/`PATCH /api/skills` too — the create/edit forms send `body` with no `frontmatter`, so that was the same defect on another endpoint, and without it an imported restriction could never be released from the UI. - parse.ts: one shared flag table drives parsing and the new `toCleanFrontmatter`, which rewrites each flag from its resolved value under the canonical key - data-schemas: `checkFrontmatterEntry` shared by `validateSkillFrontmatter` and the new `pickValidFrontmatter`; body scanner generalized to all three flags - sync/github.ts: local cleaner replaced by the shared one - both frontmatter readers stop matching indented lines, and trust a resolved boolean when the key's line carries no inline text to contradict it `allowedTools` stays bag-only: the body scan reads booleans, not YAML sequences, so a body-only edit must not drop a list it cannot re-read. |
||
|
Danny Avila
|
120ee2afa6
|
🚦 fix: Bound, Single-Flight, and Retry Skill File Priming Uploads (#14611)
Some checks are pending
Docker Dev Branch Images Build / build (Dockerfile, lc-dev, node) (push) Waiting to run
Docker Dev Branch Images Build / build (Dockerfile.multi, lc-dev-api, api-build) (push) Waiting to run
GitNexus Index / index (push) Waiting to run
GitNexus Index / post-index (push) Blocked by required conditions
Docker Dev Images Build / build (Dockerfile, librechat-dev, node) (push) Waiting to run
Docker Dev Images Build / build (Dockerfile.multi, librechat-dev-api, api-build) (push) Waiting to run
Sync Locize Translations & Create Translation PR / Sync Translation Keys with Locize (push) Waiting to run
Sync Locize Translations & Create Translation PR / Create Translation PR on Version Published (push) Blocked by required conditions
Sync Helm Chart Tags / Ignore non-main push (push) Waiting to run
Sync Helm Chart Tags / Sync chart tags (push) Waiting to run
Skill priming fanned out one unbounded batch upload per cold skill, bursting through codeapi's per-user upload limiter (30 per 5 min). Failures degraded silently: nothing persisted, every turn re-burned budget, and handle_skill reported success with no files mounted. - Bound batch uploads to 3 process-wide slots across both prime paths - Single-flight primeSkillFiles per (skill id, version) - Retry a 429 once per Retry-After, capped at 15s, fresh streams - handle_skill now tells the model when bundled files are unavailable - Warn on fulfilled-null primes in primeInvokedSkills |
||
|
Danny Avila
|
5029dd467e
|
✂️ fix: Truncate Overflowing Activity and Intent Labels in Chat UI (#14607)
* ✂️ fix: Truncate Overflowing Activity and Intent Labels in Chat UI * 🖍️ style: Use Middle Dot Separator in Tool Label Chrome * ✂️ fix: Truncate Completed Web Search Label |
||
|
Danny Avila
|
f738810c11
|
🧬 fix: Resolve URL-Named Model Spec to Its Full Preset on Cold Load (#14610)
* 🧬 fix: Resolve URL-Named Model Spec to Its Full Preset on Cold Load * 🧬 fix: Preserve Hidden Spec Names for Server-Side Resolution |
||
|
Danny Avila
|
6bbbee7a78
|
📏 fix: Scope Skill Command Query to Text Before the Caret (#14604) | ||
|
Danny Avila
|
664290c653
|
🌍 i18n: Update translation.json with latest translations (#14598)
Some checks are pending
Docker Dev Branch Images Build / build (Dockerfile, lc-dev, node) (push) Waiting to run
Docker Dev Branch Images Build / build (Dockerfile.multi, lc-dev-api, api-build) (push) Waiting to run
GitNexus Index / index (push) Waiting to run
GitNexus Index / post-index (push) Blocked by required conditions
Docker Dev Images Build / build (Dockerfile, librechat-dev, node) (push) Waiting to run
Docker Dev Images Build / build (Dockerfile.multi, librechat-dev-api, api-build) (push) Waiting to run
Sync Locize Translations & Create Translation PR / Sync Translation Keys with Locize (push) Waiting to run
Sync Locize Translations & Create Translation PR / Create Translation PR on Version Published (push) Blocked by required conditions
Sync Helm Chart Tags / Ignore non-main push (push) Waiting to run
Sync Helm Chart Tags / Sync chart tags (push) Waiting to run
|
||
|
Danny Avila
|
b11978017d
|
🧱 fix: Enforce Agent Runtime File Trust Boundaries (#14577)
* fix: secure agent runtime file metadata * chore: sort agent resource test imports * fix: Align Agent Tool Resource Types * fix: Rehydrate Agent Image Resources * fix: preserve remote agent file authorization |
||
|
Danny Avila
|
178e61b763
|
⚓ fix: Bind Action Servers to Metadata Ports (#14575)
* fix: bind action server ports * style: sort action imports * fix: normalize action port input * fix: parse action ports consistently |
||
|
Danny Avila
|
db6ba5392a
|
🪢 fix: Bind MCP OAuth Secrets to Trusted Endpoints (#14578)
* fix: bind MCP OAuth secrets to trusted endpoints * fix: bind stored MCP OAuth clients during refresh * fix: address MCP OAuth review findings * fix: bind stored MCP OAuth credentials * fix: make MCP OAuth credentials generation-safe * test: update MCP OAuth uninstall binding fixtures * fix: harden MCP OAuth credential persistence * fix: scope MCP OAuth refresh single-flight * style: sort MCP OAuth token imports |
||
|
Danny Avila
|
cdb60e74c2
|
⌨️ fix: Honor defaultPrevented in Global Shortcut Dispatch (#14570)
Some checks are pending
Docker Dev Branch Images Build / build (Dockerfile, lc-dev, node) (push) Waiting to run
Docker Dev Branch Images Build / build (Dockerfile.multi, lc-dev-api, api-build) (push) Waiting to run
GitNexus Index / index (push) Waiting to run
GitNexus Index / post-index (push) Blocked by required conditions
* ⌨️ fix: Honor defaultPrevented in Global Shortcut Dispatch * ⌨️ fix: Order-Independent Shortcut Yield via Window Listener * 📝 fix: Align Remaining Shortcut Contract Docs with Window Listener * 🧪 test: e2e Yield Contract Coverage for Global Shortcut Dispatch * 🧪 fix: Match Real Generation POST Path in Shortcut e2e |
||
|
Danny Avila
|
cdf437dc5b
|
🪺 fix: Keep Preempt-Abandoned Siblings Nested, Make Message Tree Order-Robust (#14587)
* 🪺 fix: Keep Preempt-Abandoned Siblings Nested, Make Message Tree Order-Robust * 🔗 fix: Sever Cycle Back-Edges So the Returned Message Tree Is Acyclic * 🧪 fix: Satisfy TFile in buildTree Spec fileMap Fixture * 🌲 fix: Assert Repaired Trees in convoStructure Specs, Uncharge Self-Parent Edges * 📌 feat: Identity-Stable Sibling Selection Across Background Tree Churn * 🎭 test: E2E Coverage for Thread Fold and Sibling Selection Invariants * 🔑 fix: Treat Newest-Sibling Re-Key as Hydration, Not a New Branch * 🧭 fix: Rebind Sibling Selection Per Parent, Detect Appends by Membership |
||
|
Danny Avila
|
2d606a9783
|
🧹 chore: Migrate Legacy Duplicate Code Files Blocking Dedupe Index (#14593)
Atomic file claiming (#11675) added a unique partial index on (filename, conversationId, context, tenantId) for execute_code outputs. Records written before it inserted a new document per regeneration, so any deployment that re-ran a cell producing the same filename carries duplicates the index cannot span: Mongo aborts the build with E11000 and the constraint is silently absent — the claim path still works, but without its database-level guard against concurrent inserts. Adds config/migrate-code-file-duplicates.js to normalize that legacy data, following the existing migration conventions (dry-run default, --batch-size, runAsSystem for cross-tenant scans). Renames rather than deletes: each duplicate is a distinct stored object, typically still referenced by a message attachment, so removing one would strip a real artifact from a user's history. The newest record keeps the canonical name — matching the claim path's latest-write-wins behavior — and older copies gain a ' (n)' suffix that skips names already taken in the conversation. Attachments embed their own filename, so rendered history is unchanged. After a successful apply the script builds the index directly (targeted createIndex, not syncIndexes) so the operator learns immediately whether the constraint is now in place. |
||
|
Danny Avila
|
928b14f5bc
|
🔒 fix: Single-Flight MCP OAuth Token Refresh per User/Server (#14596)
* 🔒 fix: Single-Flight MCP OAuth Token Refresh per User/Server Concurrent refresh-token redemptions (tool-call 401, ping, reconnect retries, expired-token reads) each replayed the same stored refresh token at the OAuth token endpoint. RFC 9700 reuse detection treats the replay as theft and revokes the entire grant family, forcing manual re-consent every access-token expiry. MCPTokenStorage.forceRefreshTokens is the choke point every refresh path converges on; it now single-flights redemptions per (tenantId, userId, serverName) so concurrent callers share one wire call and receive the same rotated result. The refresh token is re-read from storage inside the locked execution — never from a caller snapshot — so a redemption starting after another refresh completed uses the rotated token instead of replaying the consumed one. Fixes #14583 * 🧪 test: Isolate Single-Flight Keys per Test via Unique Server Names * 🔒 fix: Evict Stalled Refresh Slots, Decouple Waiter Aborts from Shared Redemption Codex review round 1: - A redemption that never settles no longer wedges the single-flight slot until process restart: a stale-entry timer evicts the map entry so later refreshes start fresh, while existing waiters keep their promise. - Caller AbortSignals no longer thread into the shared redemption. An impatient waiter (silent refresh's short timeout) resolves its own wait with null via a per-waiter race; the shared wire call proceeds for everyone else, bounded by transport timeouts plus eviction. * 🔒 fix: Abort Stalled Refreshes Before Slot Release, Hook Cache Invalidation to Redemption Codex review round 2: - The stale timer now aborts the wedged execution instead of deleting its slot; the slot frees only once the execution has settled, and an abort guard before the token-endpoint call stops a woken pre-wire stall from replaying a refresh token a successor already rotated. - New onRefreshSuccess hook runs inside the shared redemption after rotated tokens persist, so the silent-refresh path's mcp_get_tokens cache invalidation fires even when the initiating waiter timed out before the redemption completed. * 📝 docs: Record Post-Dispatch Abort Recovery Rationale on Stale-Refresh Valve |
||
|
Danny Avila
|
7e74f8eb8c
|
🪪 fix: Strip Unresolved Header Placeholders at Final Resolution (#14595)
Unresolved {{LIBRECHAT_USER_*}} header templates leaked literally to
upstream providers when user context was missing at resolution time
(e.g. async title generation racing client disposal), letting a gateway
trust LibreChat's own template syntax as an account identity.
resolveHeaders now takes an opt-in stripUnresolved flag that blanks any
resolvable-but-unresolved LIBRECHAT_USER/BODY/OPENID placeholder, enabled
at every final resolution boundary (resolveConfigHeaders, model fetches,
Google init, summarization overrides, azureAssistants init). Staged
passes that resolve again later with more context are left untouched, as
is the async-resolved {{LIBRECHAT_GRAPH_ACCESS_TOKEN}} and unknown names.
titleConvo now resolves headers from the req captured at entry instead of
re-reading this.options.req, which disposeClient nulls concurrently.
Fixes #14580
|
||
|
Danny Avila
|
ed25ae5b59
|
🧪 ci: Settle to render-idle before ConversationsSection memo baselines (#14590)
The lazy BookmarkNav's Suspense resolution commits during waitFor's polling, outside any act scope, so its follow-up render work lands in React's real scheduler as a macrotask. The single empty async act added in #14071 only drains microtasks and the act queue, so on slow Windows shards that work can still be pending when baselines are captured. The next act flushes pending root work wholesale, so the first stream tick carries the leftover pass and inflates the tag counter (Expected: 1, Received: 2). Flush full event-loop turns inside act until two consecutive turns add no renders, then capture baselines. |
||
|
Danny Avila
|
105f0c6236
|
🧭 fix: Drop v6-Only MemoryRouter future Prop from Skill Markdown Spec (#14588)
Some checks are pending
Docker Dev Branch Images Build / build (Dockerfile, lc-dev, node) (push) Waiting to run
Docker Dev Branch Images Build / build (Dockerfile.multi, lc-dev-api, api-build) (push) Waiting to run
GitNexus Index / index (push) Waiting to run
GitNexus Index / post-index (push) Blocked by required conditions
|
||
|
Danny Avila
|
2fb03118bb
|
💬 feat: Interim Progress Card for Streaming Q&A Calls (#14576)
Some checks are pending
Docker Dev Branch Images Build / build (Dockerfile, lc-dev, node) (push) Waiting to run
Docker Dev Branch Images Build / build (Dockerfile.multi, lc-dev-api, api-build) (push) Waiting to run
GitNexus Index / index (push) Waiting to run
GitNexus Index / post-index (push) Blocked by required conditions
* 💬 feat: Interim Progress Card for Streaming ask_user_question Calls * 🔍 fix: Match Progress Card Against Every Live Ask Pause, Not Newest Only * ⏳ feat: Hold Streaming Cursor Under Answered Question While Resume Is In Flight |
||
|
Danny Avila
|
59395a6bf0
|
🪢 refactor: Move Agent Execution Seam Before Initialization (#14581)
* refactor: move agent execution seam before initialization * refactor: type librechat agent request extensions * refactor: read envelope values from descriptors * fix: preserve envelope types and validation errors * fix: bound agent envelope traversal |
||
Marco Beretta
|
c2d8252b4f
|
🔗 fix: Resolve Relative Skill Markdown Links (#14586) | ||
|
Danny Avila
|
96499f0765
|
🔒 chore: Upgrade react-router-dom to v7.18.2 (security) (#14582)
* 📦 chore: Upgrade react-router-dom to v7.18.2 (security) Fixes GHSA-wrjc-x8rr-h8h6 (open redirect via backslash in Link/useNavigate, CVE-2025-68470 bypass) and GHSA-337j-9hxr-rhxg (deserializeErrors constructor injection). Neither has a 6.x patch; v7's react-router-dom is a shim re-exporting react-router, so all existing imports work unchanged. - vite manualChunks: match react-router so the routing chunk still captures the router (v7 moves all code out of the react-router-dom package) - jest: add test/polyfills.js (TextEncoder/TextDecoder + minimal Request); v7's CJS bundle constructs TextEncoder at module scope and builds a Request per navigation, neither exists in jsdom - auth specs: v7 types drop the synthetic default export; use a namespace import and mark the mock factory __esModule so the useOutletContext spy patches the object components actually read - isSafeRedirect: reject backslashes as defense in depth for the same open-redirect class the router patch addresses * 📦 chore: Regenerate stale bun.lock bun.lock predated months of package.json drift and still pinned react-router 6.30.3. Regenerated with bun install --lockfile-only so bun installs match current manifests, including react-router 7.18.2. * 🗂️ fix: Commit project-chip URL updates synchronously under router v7 v7 wraps router state updates in React.startTransition unconditionally, so the chip's paired updates tear: the conversation draft (Recoil) commits synchronously while the ?projectId removal defers. ChatRoute's draftProjectMismatch re-init sees draft != URL in that window and restores the removed project. The flushSync navigate option commits both in one pass, matching v6 ordering. Caught by the projects e2e specs. * 🧹 chore: Drop unused banner-query spy variable in Registration spec Pre-existing warning, but the changed-files eslint gate runs with --max-warnings=0 so it blocks this PR. The spy call stays; only the never-read variable goes. |
||
Rayan Salhab
|
3551c1ba8e
|
🪧 fix: Guard Admin OAuth Routes When Providers Are Not Configured (#14507)
* fix: guard admin OpenID routes without config * fix: guard remaining admin SSO routes without registered strategy --------- Co-authored-by: cyphercodes <cyphercodes@users.noreply.github.com> Co-authored-by: Danny Avila <danny@librechat.ai> |
||
|
Danny Avila
|
ad0f72dede
|
🌀 ci: Deterministic Circular Dependency Checks (#14579)
* 🌀 ci: Deterministic Circular Dependency Checks * 🌀 ci: Enforce Type-Level Edges in Circular Dependency Scan * 🌀 ci: Materialize Import-Type Expression Edges in Cycle Scan * 🌀 ci: Collect Inline Type-Only Specifier Edges in Cycle Scan |
||
|
Danny Avila
|
b253b623fe
|
📦 chore: update sanitize-html to latest (#14573)
Some checks are pending
Docker Dev Branch Images Build / build (Dockerfile, lc-dev, node) (push) Waiting to run
Docker Dev Branch Images Build / build (Dockerfile.multi, lc-dev-api, api-build) (push) Waiting to run
GitNexus Index / index (push) Waiting to run
GitNexus Index / post-index (push) Blocked by required conditions
* 📦 chore: update `sanitize-html` to latest
* chore: add additional modules to esModules for Jest configuration
|
||
|
Danny Avila
|
3191f6975a
|
🏷️ fix: Skip Title Generation for Preempt-Incomplete Turns (#14571)
Some checks failed
Publish `@librechat/client` to NPM / pack (push) Has been cancelled
Publish `librechat-data-provider` to NPM / pack (push) Has been cancelled
Publish `@librechat/data-schemas` to NPM / pack (push) Has been cancelled
Docker Dev Images Build / build (Dockerfile, librechat-dev, node) (push) Has been cancelled
Docker Dev Images Build / build (Dockerfile.multi, librechat-dev-api, api-build) (push) Has been cancelled
GitNexus Index / index (push) Has been cancelled
Sync Locize Translations & Create Translation PR / Sync Translation Keys with Locize (push) Has been cancelled
Sync Helm Chart Tags / Ignore non-main push (push) Has been cancelled
Sync Helm Chart Tags / Sync chart tags (push) Has been cancelled
Publish `@librechat/client` to NPM / publish-npm (push) Has been cancelled
Publish `librechat-data-provider` to NPM / publish-npm (push) Has been cancelled
Publish `@librechat/data-schemas` to NPM / publish-npm (push) Has been cancelled
GitNexus Index / post-index (push) Has been cancelled
Sync Locize Translations & Create Translation PR / Create Translation PR on Version Published (push) Has been cancelled
|
||
|
Danny Avila
|
9fbea04d46
|
🦗 fix: Deliver Abort Acknowledgements on Zero-Subscriber Replicas (#14569) | ||
|
Danny Avila
|
3dc5532111
|
🏢 fix: Preserve Tenant Context for Partial Response Saves on Disconnect (#14567) | ||
|
Danny Avila
|
6f45a9e32e
|
🔗 fix: Normalize MCP Tool Keys at Every Producer, Resolve Raw Names via Aliases (#14553)
* 🔗 fix: Normalize MCP Tool Keys at Every Producer, Resolve Raw Names via Aliases Tool keys had two spellings that could diverge for any server whose name contains characters outside [a-zA-Z0-9_.-]: the tool cache (and registry inspector) built keys with the RAW server name, while runtime instances are named with normalizeServerName(serverName). Three code comments already asserted "tool keys embed the normalized server name" - no producer honored it. For a special-character server that meant: - definitions-only mode shipped raw def names the model echoed back, but the executor's tool map held the normalized instance name, so every call failed with "Tool not found"; - per-tool tool_options (defer_loading / allowed_callers / run_in_background / describe_intent) were persisted under raw keys that never matched the definition names the option passes resolve against, so builder settings were silently inert; - tool-key parsing against normalized candidate lists silently fell back to last-delimiter splitting, which mis-parses delimiter-bearing tool names. The reconciliation is one contract enforced in three moves: 1. PRODUCERS NORMALIZE. The tool cache (packages/api/src/mcp/tools.ts) and the registry inspector build keys with the normalized server name, matching the instance names MCP.js has always assigned. The builder's tool ids, agent.tools entries, tool_options keys, and definition names all flow from these keys, so every model-facing name now agrees. The cache STORE stays keyed by the raw config name. 2. CONFIG LOOKUPS RESOLVE ALIASES. New shared helpers in data-provider (buildServerNameAliases, normalizeMCPToolKey) map a parsed normalized name back to the raw config name that the registry, config maps, tool cache, and plugin-auth rows are keyed by. Applied in the definitions loader closure, handleTools grouping, createMCPTool's parsing fallback, getUserMCPAuthMap, and the MCP tools endpoint - matching both spellings so legacy raw keys keep resolving. 3. LEGACY DATA HEALS AT ONE BOUNDARY. initializeAgent rewrites raw-keyed agent.tools entries and tool_options keys to the normalized form (normalizeAgentToolKeys) before anything consumes them, so agents persisted under the old convention load their tools AND have all four per-tool options honored. Placeholder and server-pin tokens stay raw - they are config-identity references, not model-facing names. Servers whose names are already in the safe character set (the common case) produce byte-identical keys before and after; the fast path allocates nothing. Stale Redis-cached raw keys self-heal via the existing reconnect-on-missing path within one cache cycle. * 🧯 fix: Deterministic Alias Collisions + Raw Names in Definition Metadata Two review findings on the normalization contract: - Two configured server names that normalize to the same segment (e.g. 'Sales Force' and 'Sales:Force' -> 'Sales_Force') produce inherently ambiguous tool keys; the alias map silently resolved last-wins, so a tool selected from one server could execute against the other's config. buildServerNameAliases now resolves collisions to the FIRST configured name deterministically, and resolveMCPServerContext warns once per colliding pair per process so the operator can rename one server. A collision-resistant identifier would change every existing tool key, so detection + stable routing is the right treatment here; startup-time config validation can follow separately. - The definitions loader resolved parsed (normalized) server names to raw only inside the ToolService closure, while the definition metadata (serverName -> mcpRawServerName) kept the normalized value. Server instructions are keyed by raw config names, so a special-character server's instructions were silently omitted in definitions-only mode. loadToolDefinitions now takes rawServerNames, resolves the boundary against both spellings, and stores the RAW name in definition metadata - consistent with the instance path. * 🧯 fix: Heal Stale Caches, Skill Allowed-Tools, and Builder Selectors Three review findings on the normalization rollout, all in the transition class: - Stale cache entries (P1): the definitions-only loader treats the per-server tool map as authoritative and never reconnects on a per-key miss, so a pre-change raw-keyed Redis entry would make a special-character server's tools vanish for up to the cache TTL. getMCPServerTools now heals legacy raw-keyed entries to the normalized format at read time (keys and function names), covering every consumer with no coordinated invalidation; safe names return the map untouched. - Skill allowed-tools: a skill declaring a raw MCP key in allowed-tools bypassed the initialize-boundary heal (the union runs after it) and would neither dedupe against healed agent tools nor match the normalized tool map. The primes' allowedTools now pass through the same normalizeAgentToolKeys heal before unioning. - Builder selectors: matchesMcpServer and useVisibleTools parsed tool ids against raw server names only, so an attached special-character server rendered as an unselected orphan card. Both now accept the normalized spelling and resolve it back to the raw map key, keeping legacy raw ids working. * 🧯 fix: Fail Closed on Normalized Server-Name Collisions Escalation of the collision finding: a deterministic first-wins alias plus a warning still let the tools listing publish BOTH colliding servers, so a tool selected under the shadowed second server would silently execute against the first server's configuration (their model-facing keys are identical, so routing cannot ever distinguish them). - findShadowedServerNames identifies later-configured names whose normalized form an earlier different name claimed. - getMCPTools excludes shadowed servers from the published listing entirely (with a warn naming the collision), so their tools are never selectable - nothing ambiguous can be picked. - Server creation reserves both spellings: a generated slug may not collide with a raw config name OR the normalized form its tool keys would carry. Collision-resistant model-facing IDs remain out of scope: changing normalizeServerName's output would rewrite every existing tool key (agent documents, caches, instance names) for ALL servers to handle a misconfiguration that is now blocked from exposure instead. * ✅ fix: Dedupe Reserved Server-Name Spellings at Creation The reservation list appended normalized forms unconditionally, which duplicated every safe name (raw === normalized) and broke the route-level contract test pinning the exact list. Dedupe via a Set so safe names contribute one entry, while special-character names still reserve both spellings; adds the special-character reservation case. * 🧯 fix: Never Heal a Shadowed Server's Keys; Align Authorization Tie-Break Persisted references were the remaining collision vector: an agent or skill saved with the shadowed later server's raw key was HEALED into the shared normalized key, authorized through a last-wins map, and routed first-wins - authorized as one server, executed as another. - normalizeAgentToolKeys now refuses to rewrite keys of shadowed servers (findShadowedServerNames): rewriting would produce exactly the first server's key. Left raw, the key cannot match the normalized-keyed tool map and the tool fails visibly - broken beats misrouted. Covers agent.tools, tool_options, and skill allowed-tools through the shared heal. - filterAuthorizedTools (agents/v1.js) builds its normalized-to-raw map via the shared buildServerNameAliases instead of a last-wins Map constructor, so authorization resolves a colliding key to the SAME first server execution routes to. * 🧯 fix: Direct Identity Wins Over Aliases; Heal Client Forms and Degraded Contexts Four review findings on the normalization edges: - Alias hijack (P1): a user-DB server named exactly like an operator server's normalized form ('foo' vs YAML 'foo!') had its tools rerouted to the operator server by unconditional alias resolution. Resolution is now DIRECT-FIRST everywhere: the parsed name is tried as-is, and only when nothing resolves is it treated as a normalized spelling (definitions loader, handleTools grouping, createMCPTool fallback). buildServerNameAliases seats identity entries before derived ones so a literal name owns its slot regardless of config order, findShadowedServerNames and the collision warning derive from the same construction, and getUserMCPAuthMap fetches auth under both spellings so either owner finds its rows. - Builder double-match: a normalized name containing the delimiter ('foo mcp bar' -> 'foo_mcp_bar') also suffix-matched a server named 'bar', selecting both cards and making removal strip the wrong tool. matchesMcpServer now resolves the token ONCE against the full configured list (longest boundary, both spellings) when the caller supplies it; selection and removal share the resolution. - Builder legacy ids: an agent saved with raw-keyed ids showed its tools unchecked while the runtime heal kept them active, and selection updates never replaced the legacy entries. McpSection maps legacy raw ids to their current normalized ids when deriving and rewriting this server's selection. - Degraded context: a transient ensureConfigServers failure returned an entirely empty context, leaving normalized keys unresolvable for the request. resolveMCPServerContext now keeps the name lists (they derive from the config snapshot alone) and degrades only the lazy-init configs. * 🧯 fix: Collision Detection Sees Accessible Servers; Shadowed Refs Fail Closed End to End Round follow-ups on the collision design, all in the DB-server-visibility class: - The legacy-key heal detected collisions against operator-config names only, so healing could still produce a key that direct-first resolution routes to an invisible user-DB server. initializeAgent gains an optional getAccessibleMcpServerNames dep (wired through ToolService for controllers that mock it, directly elsewhere), consulted ONLY when a configured name needs normalization - zero cost for safe-name deployments. The heal then sees the full accessible set and skips shadowed servers' keys. - Wildcard and legacy raw tokens bypassed catalog filtering, letting a shadowed server's instances join a run under the same normalized names as the winner's. filterAuthorizedTools rejects tools of shadowed servers at authorization (its merged map sees DB + config), and handleTools skips them at execution. - The builder migrated only tool selection, not tool_options: legacy raw option keys showed disabled while the runtime honored them, and toggles could not clear them. McpSection now migrates option keys to the current normalized ids (existing normalized entries win). - A transient ensureConfigServers failure degraded to an EMPTY server context, leaving normalized keys unresolvable for the request. resolveMCPServerContext keeps the name lists (derived from the config snapshot alone) and degrades only the lazy-init configs. * 🧯 fix: Complete the Collision Audit at Every Gate; Safer Heal Semantics Round follow-ups hardening the collision audit: - Execution guards now consult the FULL accessible set: the caller's heal threads its already-fetched names through loadTools, and handleTools fetches them itself when a configured name needs normalization (never for safe-name deployments) - so a cross-tier collision (user-DB 'foo' vs operator 'foo!') fails closed at eager execution instead of joining the run under one normalized name. - Healing is SKIPPED when the collision audit cannot complete (transient lookup failure, or no dep): un-healed raw keys still resolve through the direct-first candidates, so skipping is safe while rewriting against an incomplete audit is not. - The audit lookup is gated on the agent actually carrying delimiter-bearing keys (tools, tool_options, or skill allowed-tools), so non-MCP agents never pay a registry round-trip even on specially named deployments. - normalizeAgentToolKeys gives the CURRENT (normalized) entry precedence when both spellings carry options, matching the builder's migration semantics instead of letting insertion order decide. - The builder's toCurrentToolId resolves entries boundary-exactly against every configured server (longest match, both spellings), so a raw suffix shared with a LONGER server name can no longer reassign that server's selection or options while another dialog is open. * 🧯 fix: Shared Collision Audit for Definitions Loading; Fail Closed on Audit Failure Round follow-ups closing the remaining audit gaps: - The definitions-only loader now consumes the same collision audit as eager loading: shadowed servers' entries (wildcards included) are dropped before definitions are emitted, so the default execution path can never resolve a shadowed server's normalized function name to another server. The audit names thread from initializeAgent's heal; the loader self-fetches only when a configured name needs normalization. - resolveCollisionAuditNames centralizes the audit-resolution policy (threaded set > self-fetch when needed > incomplete on failure), and BOTH loaders now fail closed under an incomplete audit: any normalization-sensitive reference (its own name needs normalizing, or it equals the normalized form of a configured special-character name) is skipped with a warning instead of being audited against operator names alone. isNormalizationSensitiveName lives in packages/api as a pure helper so test mocks use the real predicate. - normalizeAgentToolKeys collapses duplicate ids after healing (order-preserving): a document carrying both spellings converges on one key, never two instances with the same function name. * 🧯 fix: Thread the Audit Everywhere; Identity-Aware Alias Fallback Round follow-ups on audit plumbing: - The OpenAI-compatible and Responses tool loaders now forward the already-resolved accessibleMcpServerNames instead of discarding it, so the definitions loader neither repeats the registry lookup nor fails closed on a transient second lookup after the first succeeded. - The skill-only path threads its audit: when the baseline agent has no MCP keys but a primed skill's allowed-tools fetched the complete set, that set (not the operator-only list) reaches the loader, so the collision remains visible and the shadowed reference stays rejected end to end. - OAuth discovery iterates the collision-FILTERED tool list, so a request can no longer emit an OAuth prompt, wait out the connection timeout, and reconnect a server whose definitions were deliberately rejected. - The definitions loader's alias fallback is identity-aware: when the parsed name IS a known accessible server, a null tool fetch means temporarily unavailable (OAuth pending, missing user variables, disconnected) and no longer reroutes to the raw alias - previously the aliased operator server's definitions could be emitted under the unavailable DB server's names. * 🧯 fix: Legacy-Key Definition Lookup; Retain Audit for Deferred Execution - createMCPTool resolves tool definitions by BOTH spellings: the key as persisted plus the canonical normalized key built from the resolved server name. Assistants and direct tool calls persisted before the rollout bypass the agent-boundary heal and arrive with raw keys, while availableTools is now indexed canonically - previously every such call missed the index, burned a reconnect, and returned the unavailable stub permanently via the negative cache. - The initialized agent retains accessibleMcpServerNames (the COMPLETE collision audit this initialization resolved), buildAgentToolContext copies it into every per-agent tool context, and loadToolsForExecution threads it into the eager loader as bare options. Deferred/event-driven execution therefore reuses the snapshot instead of repeating the merged registry read - a transient failure there could fail-closed a tool the same turn already advertised from the successful first audit. - MCP.spec.js keeps @librechat/api pure helpers REAL (requireActual spread) so normalization paths are exercised rather than mirrored. * 🧯 fix: Parse Legacy Keys Against Both Server-Name Spellings createMCPTool's boundary candidates were normalized-only, so a legacy raw key whose server name contains the delimiter (foo_mcp_bar!) missed the suffix match and fell to the generic last-delimiter split - the canonical rebuild then produced a key that could never hit the index and the persisted call stubbed out. The candidate list now carries the RAW resolved name (and raw config names on the parse-only path) next to the normalized spellings. * 🧯 fix: Honest Audit Completeness; Shadowed-Server Form-Key Guard - resolveAllMcpConfigs tolerates ensureConfigServers failures, so the merged registry read can silently omit config-only servers while the audit still reported complete: true - a foo/foo! collision would go unseen and a persisted key could route to the wrong server. Both audit consumers now union the snapshot-derived raw config names back in (resolveCollisionAuditNames unions the caller's rawServerNames; the initializeAgent heal unions configRawServerNames), keeping the completeness label honest without an extra read: operator names come from the registry-independent config snapshot, user-DB names from the merged read that fails loudly into the existing incomplete path. - The client tool_options migration now mirrors the runtime heal's fail-closed rule for SHADOWED servers: when the dialog's server has lost its normalized slot to another catalog name, legacy raw keys stay raw instead of being rewritten onto the winning server's key, where a later save would apply the wrong server's per-tool settings. The dialog's own server joins the alias construction so a stale catalog map can't misread as a collision. * 🧯 fix: Heal Legacy Assistant MCP Tool Names on Save The assistants create/update controllers look tools up in the cached definitions by exact key, and the cache is now normalized-keyed - an assistant saved before the convention resubmits its raw-suffixed MCP name on every edit, so any save silently removed the tool. healMcpToolNames pre-heals the payload's tool list: a delimiter-bearing string that misses the cache resolves through the configured raw names (longest-suffix, boundary-exact) and rewrites to the normalized key only when that key actually exists in the cache. SHADOWED raw names stay raw and fail closed, mirroring the runtime heal; the config read happens only when a delimiter-bearing name actually misses, and read failures propagate (write path) rather than silently dropping tools. v2's update loop also stops re-reading the tool cache per iteration. * 🧯 fix: Full-Audit Shadow Set + Dedupe in the Assistant Key Heal - The assistant-save heal built its shadow set from operator config names alone, so a cross-tier collision (user-DB `foo` owning the normalized slot of operator `foo!`) looked unshadowed and the legacy key healed into the shared normalized name - which direct-first execution then binds to the DB server. The shadow set now comes from resolveCollisionAuditNames' full accessible audit, and an incomplete audit skips healing outright (every rewrite candidate is normalization-sensitive by construction, so raw-and-fail-closed is the only safe answer). - Healed string entries dedupe order-preserving: a payload carrying both spellings of the same tool collapses to one entry instead of expanding into duplicate function definitions the provider rejects. |
||
|
Danny Avila
|
de033b7dbd
|
🦗 fix: Ignore Sequenced Redis Events Without SSE Subscribers (#14557) | ||
|
Danny Avila
|
b9ca391b84
|
📦 chore: bump @librechat/agents to v3.3.11 (#14562)
|
||
|
Danny Avila
|
e7f1838515
|
⚡ feat: Reliable Interrupt & Steer Escalation and Recovery (#14558)
Some checks are pending
Docker Dev Branch Images Build / build (Dockerfile, lc-dev, node) (push) Waiting to run
Docker Dev Branch Images Build / build (Dockerfile.multi, lc-dev-api, api-build) (push) Waiting to run
GitNexus Index / index (push) Waiting to run
GitNexus Index / post-index (push) Blocked by required conditions
* feat: surface interrupt-steer escalation on waiting messages The interrupt & steer feature shipped reachable only through the composer chord, the send-button hovercard, and the composer button; a message already waiting (queued for after the run, or steered and parked at the next tool boundary) had no path to it. Both waiting surfaces now carry one: - Queued rows get an icon-only ZapOff escalation button beside the existing Steer primary. It routes through sendQueuedNow, which now takes a preempt option on its live-run path. The tooltip teaches the composer chord, derived through resolveComposerKeyDown so a rebound or yielded chord is never advertised. - In-flight steer bubbles get an "Interrupt now" overflow entry with the same race rules as Edit: reclaim first, and only a `reclaimed` outcome resubmits (via retrySteer with preempt, swapping the chip for an interrupting one). `applied` and run-ended-mid-reclaim outcomes stop at the existing informational toasts, so the words can never land twice. Not offered on a steer already preempting. - Every during-run overflow menu gains an "Always interrupt instead" toggle for steerInterruptsByDefault, next to the existing steer/queue default toggle. MenuEntry supports disabled for the new entries. Only one interrupt can be unresolved at a time: while one preempt is pending (or the run is paused on approval, where the server 409s), every escalation control disables instead of racing the same seal. Ten new tests across both surfaces; 381 green in the affected suites. * fix: lock escalation across its reclaim window, keep the paused control visible, label as steer Codex round 1, all three findings. P2, escalation race. The single-interrupt invariant had a window between clicking "Interrupt now" and the reclaim resolving, where no preempt chip existed for the chip-derived gate to see: two bubbles escalated back-to-back could both resubmit. A shared escalating flag (Jotai, per-conversation) now covers the window and disables every escalation control on both surfaces, and a fresh recheck before resubmitting catches an interrupt armed elsewhere meanwhile (composer chord, queued row); those words re-home to the queue with an informational toast instead of breaking the invariant. P2, unreachable paused state. canSteer is defined as hasRealConvoId && !pausedOnApproval, so gating the button on canSteer removed it exactly when it was meant to render disabled; the test only passed on an impossible stub combination. The render gate is now duringRunActive && (canSteer || pausedOnApproval), and the test uses the real invariant. P2, label semantics. "Interrupt & send now" borrowed the name of the hard-abort action; this one preserves the partial answer and steers. Renamed to "Interrupt & steer now" (com_ui_interrupt_steer_now). Both behavior fixes counterfactually verified; 384 tests green across the affected suites. * fix: disable bubble escalation while the run cannot accept a steer Codex round 2, one P2. Answer mode (ask_user_question) sets duringRunActive false while pausedOnApproval stays false, since that flag only detects approval-bearing tool calls. The bubble's escalation entry stayed enabled there, so clicking it cancelled a healthy waiting steer and the preempt resubmission bounced off RUN_PAUSED, degrading the words to the queue. The entry now also disables on !duringRunActive, matching the queued-row control's gate. Counterfactually verified: reverting the gate fails the new answer-mode test. * fix: recheck live run state after the reclaim, not just at the click Codex round 3, one P2, and it is the round-1 recheck principle applied one level deeper: the entry-time disable cannot see a run that pauses (tool approval, answer mode) while the reclaim round-trip is in flight, and the .then closure held the render's stale steering controls, so the resubmit would fire into a RUN_PAUSED rejection after the reclaim had already surrendered the steer's boundary slot. The escalation continuation now reads the LIVE controls through a latest-ref: if the run can no longer accept a steer, the words re-home to the queue with an informational toast instead of resubmitting, and the resubmit itself also goes through the live controls. Counterfactually verified: reading the stale closure instead of the ref fails the new mid-reclaim pause test. * refactor: make escalation one atomic server-side arm, in place Codex round 4: four P2s, every one an interleaving of the same window — escalation as reclaim-then-repost is a compound, non-atomic operation whose continuation must revalidate the world (FIFO position lost, ref assigned too late, no run fence, competing bubble actions). Rounds 1-3 patched that window with a lock and rechecks; round 4 shows the window itself is the defect, so this removes it instead of guarding it again. Escalation is now POST /chat/steer/arm: the server flips preempt on the EXISTING queued item in one atomic store op (new IJobStore.armSteer; a decode-patch-encode LSET Lua on Redis, an in-place mutation in memory), fenced to the validated generation and refused once the queue closes. The handler mirrors the steer POST's preempt contract exactly: durable flag gated on the owner's recorded capability, volatile requestPreempt fire-and-forget because the durable flag is the truth resume/handover re-arm from. By construction this resolves all four findings: FIFO survives (the item never moves; the whole queue still drains in instruction order at the seal), there is no continuation to hold stale controls, the store op is fenced to the original run, and a competing Edit/Queue/Cancel either beats the arm (armed:false, chip untouched) or operates on the armed item, whose cancel already disarms. The client escalation entry becomes one mutation: armed:true relabels the chip in place (same steerId, same position), PREEMPT_UNSUPPORTED and lost races toast honestly, and the round 1-3 machinery — the escalating lock atom, the latest-ref, the post-reclaim rechecks and their two toast strings — is deleted rather than extended. Verified: 7 new handler tests on the real in-memory manager (including FIFO preservation and the stale-generation fence), 2 Redis integration tests against real Redis (in-place arm keeps order and every field; missing/stale/closed all refuse), client suites 396 green. * fix: decide capability inside the atomic arm, neutralize the lost-race toast Codex round 5, both findings, both edges of the new arm design rather than its mechanism. P2, capability TOCTOU. A HITL resume on a rolling deploy rewrites preemptCapable for the SAME generation, so the handler's read could go stale between validation and the flag flip, arming a steer the live owner cannot seal. armSteer now returns armed | missing | incapable, with the owner's live capability part of the same atomic predicate as the generation fence (HGET preemptCapable inside the Lua; the flat job field, not a metadata blob — the in-memory store reads the same field). The handler's pre-check is deleted rather than kept alongside; the store predicate is the single source. New handler test rewrites the capability after queueing and expects PREEMPT_UNSUPPORTED with the item left unflagged; the Redis guards test now asserts the incapable refusal against real Redis. P2, ambiguous toast. armed:false covers injected, cancelled, re-homed, and run-over alike, so telling the user the message "already reached the agent" claimed one specific outcome. The lost-race branch now uses a neutral message (com_ui_steer_arm_lost_race) and defers to the events for what actually happened. * fix: flip the escalation lock synchronously before the arm request Codex round 6, one P2. Round 4 deleted the escalating flag along with the reclaim continuation it guarded, but that left the one-interrupt gate blind during the arm request's own round trip: the chip-derived check cannot see an arm until its response relabels the chip, so on a slow connection two bubbles could both arm before either response landed. Double-arm is harmless server-side now (the run seals once and drains the whole queue in order), but every escalation control advertises "one interrupt at a time" by disabling, and the controls must tell the truth. The per-conversation escalating flag returns as a pure UX gate: set synchronously at click, before the mutation, cleared on settlement, and folded into interruptPending on both surfaces. Unlike its round 1-3 ancestor there is no continuation behind it to guard and no recheck to pair with it. Counterfactually verified: without the synchronous set, the two-bubble race test arms twice. 207 tests green across the Chat Input suites. * test(e2e): cover escalation of waiting messages through the real seal Three mock-harness tests on E2E_SLOW_REPLY, a 160-chunk stream with no tool boundary, so an in-thread steer part can ONLY come from a genuine mid-stream seal — which makes each test a behavioral proof rather than a UI check: - Queued row escalation: the ZapOff button turns a waiting queued message into a preempt-armed steer (202 echoes preempt: true) that seals and injects, where the sibling steering.spec test proves the unescalated path waits for run end instead. - Bubble in-place arm: an ordinary steer (202 with no preempt echo) waits as a bubble, POST /chat/steer/arm answers armed: true, the bubble relabels in place (same single bubble, same text, escalation no longer offered on reopen), and the armed steer seals mid-stream. - Always-interrupt toggle: flipped from a waiting row's overflow menu, plain Enter now produces a preempt: true steer that seals in the SAME run, and the menu offers the way back. An afterEach clears the localStorage preference so a mid-test failure cannot leak preempt-by-default into the rest of the serial suite. All three verified locally through the full harness (real backend, mock LLM, seeded DB): 3 passed in 27s. * feat: dedicated escalation arrow + shortcut, menu split into actions and preferences The escalation was still half-hidden: the bubble only offered it inside the overflow menu, and the tooltip taught the composer chord, which does a different thing (interrupts with typed text, not this chip). Three changes make it a first-class command: - A shared EscalateNowButton (circular arrow, ghost-bordered like the composer's interrupt control) is always visible on BOTH surfaces: beside each queued row's Steer primary and on every waiting steer bubble next to its menu. It disappears once a steer is interrupting. - A dedicated registry shortcut, escalateSteer (Cmd/Ctrl+Shift+.), editing-allowed and rebindable like every other action. Deliberately NOT an Enter chord: the composer owns every Enter chord, and the yield design rests on no default binding using Enter besides submit. Its handler clicks the newest enabled arrow control (bubbles beat queued rows), so the shortcut can never diverge from the button, and the arrow's tooltip teaches THIS command via the registry display. - The overflow menus separate one-off actions from sticky behavior changes: Edit, Cancel, Queue, then a smaller "Preferences" section holding the queueing and always-interrupt toggles, each with the standard InfoHoverCard reusing the Settings panel's descriptions. "Interrupt & steer now" leaves the menu entirely. 386 client tests green, including a menu-structure test locking the order and the absence of the escalation entry; bubble escalation tests drive the visible arrow. The e2e spec's bubble test now clicks the arrow, and a fourth test drives the dedicated shortcut end to end through a real mid-stream seal. * style: bind the escalation arrow to its message (variant A anatomy) Two same-weight circles in a row read as one control group, leaving the arrow's ownership ambiguous, and a floating arrow stops meaning anything once several messages stack. The shared control now carries variant A's anatomy: a thin divider binds a small SOLID arrow (filled, inverted) to the message region on its left, and the menu ellipsis stays a bare glyph, so the two affordances can no longer blur together — and the divider+arrow pairing repeats cleanly per chip at N messages. * chore: drop the unused within import CI lint caught * fix: advertise the escalation shortcut only while the control is live Codex on the e2e head, one P2: the tooltip appended the chord hint even while the button was disabled, advertising a shortcut that does nothing during an approval pause. The flagged control (InterruptNowButton) was since replaced by the shared EscalateNowButton, which inherited the pattern; the successor now omits the chord whenever the control is disabled, matching the rule the during-run hovercard already follows. * fix: harden steer escalation lifecycle and recovery * test(e2e): disambiguate accessible steer preferences * test: align abort persistence coverage with prerequisites * chore(i18n): remove obsolete steer race message * chore: normalize imports across steering changes * test: exercise stream integration on Redis Cluster * test: scope HITL checkpoints to generation * test: fix cluster cleanup and locale policy * fix: keep escalation visible during ask pauses * fix: fence recovery downgrade and stale predecessors * fix: require generation owner abort acknowledgement * fix: validate delayed preempt arms * test: align final escalation fixtures * fix: preserve in-memory predecessor abort handoff * fix: restore controls for recovered queued messages * test: cover recovered queue controls * fix: close final steering review gaps |
||
|
Danny Avila
|
60ca751a7f
|
🧠 fix: Preserve Deferred Tool Schemas Across HITL Resume (#14552)
Some checks are pending
Docker Dev Branch Images Build / build (Dockerfile, lc-dev, node) (push) Waiting to run
Docker Dev Branch Images Build / build (Dockerfile.multi, lc-dev-api, api-build) (push) Waiting to run
GitNexus Index / index (push) Waiting to run
GitNexus Index / post-index (push) Blocked by required conditions
* 🧠 fix: Preserve deferred tool schemas across HITL resume * 🧪 test: Harden deferred tool resume regression * 📦 chore: bump @librechat/agents to v3.3.10 |
||
|
Danny Avila
|
78ec1940a2
|
🧹 fix: Clean Up MCP OAuth State Mappings on Uninstall + Reject Superseded Callbacks (#14549)
* 🧹 fix: Clean Up MCP OAuth State Mappings on Uninstall + Reject Superseded Callbacks Disconnecting an OAuth MCP server deleted its mcp_oauth flows but left the mcp_oauth_state:{state} mappings behind for the full TTL. Because flow ids are deterministic (userId:serverName) and the CSRF token is HMAC(flowId), a stale browser tab's callback could resolve its orphaned state to the NEXT flow for the same server, pass CSRF, burn the fresh flow's one-shot CSRF cookie, and fail the PKCE exchange, sabotaging the legitimate retry. - Add MCPOAuthHandler.deleteFlowAndStateMapping: reads the flow's stored state and deletes the mapping before the flow (mapping-first so a crash between deletes fails closed instead of recreating the orphan) - Route mcp_oauth deletions in clearStoredMCPOAuthState through the helper for both tenant-scoped and legacy flow ids - Reject callbacks whose state does not match the resolved flow's stored state: the only control distinguishing a superseded attempt from the current one on a deterministic flow id Fixes #14534 * fix: gate failFlow on state match in the OAuth error branch (Codex P1) The provider-error branch failed the resolved flow on CSRF/session alone, so a superseded error callback resolved through an orphaned mapping could mark the current flow FAILED. Apply the same stored-state equality gate before failFlow. * fix: leave the flow in place when the state-mapping delete fails (Codex P2) deleteFlow swallows storage errors and returns false, and deleteStateMapping discarded that result, so a failed mapping delete followed by a successful flow delete would silently recreate the orphan. Surface the boolean from deleteStateMapping and throw from deleteFlowAndStateMapping before touching the flow, so the caller's allSettled warn branch fires and the next replacement retries both. * fix: restore the state mapping when the flow delete fails (Codex P2) The inverse partial failure of the round-3 fix: a successful mapping delete followed by a silently failed flow delete left a PENDING flow whose reused authorization URL could never resolve, dead-ending every callback in invalid_state until the flow went stale. Check deleteFlow's result, re-store the mapping on failure, and throw so the caller's allSettled warn branch fires. * fix: never leave a callback-capable flow behind on uninstall (Codex round 6) Teardown runs after the server's tokens are deleted, so a preserved flow+mapping pair (the round-3 early-throw path) let a lingering consent tab complete the callback and recreate credentials post-uninstall. Now that both callback branches gate on stored-state equality, an orphaned mapping is the benign failure mode, so invert the order: delete the flow first, attempt the mapping delete regardless, and reject when either reports a storage failure. This supersedes the round-4 mapping restore, which also preserved a callback-capable pair. * fix: delete the flow even when its metadata read fails (Codex round 7) A storage error on the initial getFlowState aborted teardown before any delete ran, preserving the callback-capable flow after token deletion. Tolerate the read failure, delete the flow blindly, skip the mapping it could not identify (the callback gates neutralize the possible orphan), and reject so the caller's warn branch fires. |
||
|
Danny Avila
|
a67b0c1da8
|
🎯 feat: Per-Tool Intent Label Toggles in the Agent Builder (#14550)
* 🎯 feat: Per-Tool Intent Label Toggles in the Agent Builder Saved agents have had per-tool intent control on the backend since the capability landed (tool_options[name].describe_intent, consumed by applyIntentLabels), but the builder offered no way to set it - the capability was invisible to saved agents on MCP tools, which default off. This is the deferred UI slice. The MCP tools panel gains a fourth per-tool option toggle (Captions icon, teal) next to defer / programmatic / background, plus the matching section-header bulk toggle, gated on the tool_intents capability. The toggle writes describe_intent: true through the same withBooleanOption path the sibling flags use, so an opt-in composes with existing entries and clearing the last flag drops the tool's entry entirely. No backend changes: the agent CRUD schema already validates describe_intent and initialization already consumes it. * 🧯 fix: Keep the Intent Toggle Truthful for Programmatic-Only Tools A tool marked Programmatic in the builder gets allowed_callers: ['code_execution'], and the backend's canInjectIntentParam deliberately skips non-direct tools (no card renders for calls made from code), so an intent opt-in on such a tool is guaranteed inert. The UI could nevertheless show both settings active. The intent toggle now mirrors the runtime gate: isToolProgrammaticOnly (allowed_callers set and missing 'direct', the exact backend predicate) renders the per-row toggle inert with a tooltip explaining why, shows it unpressed regardless of any stored flag, and the bulk toggle and its all-state consider only tools the label can actually reach. The stored describe_intent value is preserved, so unmarking Programmatic restores the user's earlier choice instead of destroying it. OptionToggle gains a disabled state (dimmed, non-interactive, tooltip kept) shared by the row and bulk variants. |
||
|
Danny Avila
|
52b2ebf948
|
🧪 test: Run mock E2E against Redis in shards (#14551)
* 🧪 test: Run mock E2E against Redis in shards * 🧪 test: Isolate local Redis E2E data |
||
|
Danny Avila
|
f5e8feba80
|
📦 chore: bump @librechat/agents to v3.3.9 (#14548)
|
||
|
Danny Avila
|
1e1de6eff9
|
🎯 fix: Exact Ask-Question Attribution via Interrupt tool_call_id (#14539)
The ask_user_question pause/answer stamps (server pause-time args stamp, resume-time answer stamp, and the client mirror) targeted the newest unanswered ask part by position. When a model emits several ask calls in one turn, the interrupt's question and the user's answer land on the wrong card. @librechat/agents > 3.3.8 surfaces the interrupting call's tool_call_id on the ask interrupt payload. All three stamps now target that id exactly when present, keeping the positional fallback for older payloads. The tool body passes config.toolCall.id through to askUserQuestion via a typed alias that is a no-op on the pinned SDK and lights up on the next dependency bump. Companion to danny-avila/agents#366, which also fixes the underlying dangling tool_use 400 (INVALID_TOOL_RESULTS) when one of the parallel ask calls streams malformed args. |
||
|
Danny Avila
|
ad4ed67070
|
🟦 chore: Convert Activity-Label Eval Harness to TypeScript (#14530) | ||
|
Danny Avila
|
f0d3bcb622
|
📄 fix: Filter Non-PDF Documents on the Anthropic Encode Path (#14535)
Anthropic's Messages API only accepts application/pdf for base64 document sources, but encodeAndFormatDocuments sent every allowlisted file (docx, xlsx, csv, html) through the base64 branch unfiltered. The provider 400 recurs on every retry because attachments are re-encoded each request, permanently breaking the conversation. - Add isAnthropicDocumentType / isAnthropicTextDocumentType to data-provider, mirroring isBedrockDocumentType - Filter unsupported types before encoding (matching Bedrock semantics) and log the skipped attachments - Send textual types as plain-text document sources (source.type 'text'), which Anthropic accepts and supports citations for, instead of invalid base64 blocks Fixes #14485 |
||
|
Danny Avila
|
8e165eb451
|
🔒 fix: Remove Owner Email from Agent owner_contact Fallback (#14541)
Some checks are pending
Docker Dev Branch Images Build / build (Dockerfile, lc-dev, node) (push) Waiting to run
Docker Dev Branch Images Build / build (Dockerfile.multi, lc-dev-api, api-build) (push) Waiting to run
GitNexus Index / index (push) Waiting to run
GitNexus Index / post-index (push) Blocked by required conditions
* 🔒 fix: Remove Owner Email from Agent `owner_contact` Fallback The owner-contact fallback for agents without an explicit support_contact exposed the owner's private account email to any VIEW-level caller via GET /agents/:id and GET /agents. The fallback now resolves a display name only (name/username/authorName): the User query no longer projects email, the resolver never returns one, and the shared AgentOwnerContact type drops the field. Emails are only served when the owner opts in via support_contact. * 🔒 fix: Reject Email-Shaped Owner Display Names in Contact Fallback OpenID and SAML strategies fall back to the account email for the user's name and username when no display-name claims exist, so the name-only owner fallback could still surface the email through those fields. The resolver now rejects email-shaped display-name candidates entirely. * 🔒 fix: Treat Any @-Containing Display Name as Email-Derived RFC-5321 quoted local parts may contain whitespace and the User schema email validator is an unanchored substring match, so such addresses can reach the name/username fields via SSO fallbacks. Rejecting on '@' presence covers every legal email form without re-fetching the account email. |
||
Jens Schumann
|
ad74a282d1
|
🪃 fix: Resolve User Vars Before the First Post-OAuth Reconnect (#14538)
* fix: resolve customUserVars before first post-OAuth-callback MCP reconnect
The OAuth callback route reconnects the user's MCP connection immediately
after storing new tokens, but never resolves customUserVars before doing
so - unlike the /reinitialize route a few hundred lines below, which does.
As a result, headers/oauth_headers templates like `{{MY_KEY}}` are sent
to the MCP server literally, unsubstituted, on this first connection
attempt, even though the user's value is already saved. The upstream
server rejects it as an invalid credential.
Fixes #14537
* refactor: share getServerCustomUserVars reader from @librechat/api
The mcp_-prefixed key shape was built by getUserMCPAuthMap but re-derived
by hand at each read site (a private helper in services/MCP.js, and the
new callback-route extraction). Export a reader from the same module that
owns the writer and reuse it at both sites, so the key shape has a single
source of truth.
* chore: sort destructured require members in routes/mcp.js
---------
Co-authored-by: Danny Avila <danny@librechat.ai>
|
||
|
Danny Avila
|
ff9d89540c
|
🎯 feat: Render Tool Intent as the Live Tool-Call Label (#14536)
* 🎯 feat: Render Tool Intent as the Live Tool-Call Label The tool_intents capability injects a model-authored `intent` sentence as the FIRST key of a tool call's args, and the SDK's coding tools carry it natively — but no client component ever read it, so cards kept showing their generic labels ("Running command") while the intent streamed by unused. A shared useToolCallIntent hook extracts the intent from streaming args via parseJsonField's partial-JSON fallback, so the label renders from the first delta — before any other arg exists — and keeps updating as it streams. When present, the intent replaces the generic in-progress label and persists as the settled label (completion is a UI state, not a tense change, matching the SDK's applyOutcome design). Cancelled, error, and background states keep their existing precedence. Wired into BashCall, ExecuteCode, the generic ToolCall (MCP, actions, plugin tools), ReadFileCall, SkillCall, and FileAuthoringCall. Non-string `intent` business params are ignored. SubagentCall keeps its verb+name header design for a follow-up. * 🎯 fix: Harden Intent Label Extraction per Review Gate the label on intent being the FIRST args key (the label contract's first-position rule), so a tool's own business param named intent — e.g. a CRM's {"q":"acme","intent":"billing_inquiry"} — no longer renders as the status label. Bound the label to a single 256-char line before it reaches ProgressText's nowrap layout, mirroring the SDK's outcome-label cap. Decode the full JSON escape set in parseJsonField's streaming fallback (\t \r \b \f \/ and \uXXXX with surrogate pairs) so a partial label renders exactly as its settled JSON.parse form; stream-edge incompletions (dangling escape, partial \uXX, split surrogate) are held back rather than shown. Wire web_search into the intent label: Part.tsx now passes toolCall.args and the WebSearch card prefers the intent for its progress and completed texts — it carries intent natively but never received args at all. * 🎯 fix: Round-2 Review — Stable Live Region, Split Low Surrogates, Specialized Cards Keep the aria-live region on its stable generic value while the intent streams: an atomic polite region re-announces the whole growing sentence on every delta otherwise. The settled intent is still announced once via the finished text. Hold back a decoded high surrogate while its low-surrogate escape is still streaming (\ud83d\u, \ud83d\ude0), not only when the high half ends the value exactly; a complete following escape composes the pair on the next iteration, and a lone surrogate followed by ordinary text stays emitted, matching JSON.parse. Thread args into the specialized cards for explicitly opted-in tools: RetrievalCall (file_search) and the image-gen cards (image_gen_oai, image_edit_oai, gemini_image_gen) now resolve the intent for their progress and settled labels, with the image phase texts as fallback. * 🎯 fix: Round-3 Review — Live-Region Settled Announcements & Remaining Cards Announce the settled intent once through the aria-live regions of RetrievalCall, the image-gen card, and WebSearch, while each region keeps a stable generic value during streaming (WebSearch was still piping the growing intent into its atomic region on every delta). Pass object-valued args through Part.tsx to the image-gen card instead of coercing them to '' — persisted/completed calls carry object args, so the first-key intent was invisible on reload. Guard complete serialized args against non-string intents: parseJsonField's JSON branch would coerce {"intent":{...}} into "[object Object]"; the hook now type-checks the parsed field, matching the object-args path. Wire the subagent card: the SDK-native subagent intent now leads its header, without overriding error or cancellation framing. * 🎯 fix: Round-4 Review — Constant-Cost Extraction, Final-Search Settling, Safe Truncation Replace the hook's JSON.parse-per-delta with a single anchored regex over a bounded 2 KB head window: the first-position contract lets one match do the business-param gating and the value capture (complete or streaming), so per-delta cost stays constant while a large code/content argument streams behind the label. A non-string first-key intent never matches the opening quote, keeping the round-3 guard without parsing. Settle a web search that is the message's final part once submission ends: `complete` previously required !isLast permanently, so the last-part case shimmered forever and never announced its settled intent. Back the truncation cut off a high surrogate so a bounded multilingual label never ends in a replacement glyph before the ellipsis. * 🎯 fix: Round-5 Review — Keep Terminal Lone Surrogates in Settled Values Thread value completeness from the extractor into the escape decoder: a captured closing quote means the value is settled, so the stream-edge hold-backs (partial \uXX, high-surrogate deferral) no longer apply and a value genuinely ending in a lone high surrogate keeps its final code unit, matching JSON.parse and the object-args rendering. Streaming callers keep the hold-back behavior unchanged. |
||
|
Danny Avila
|
d6c2dc5d8e
|
🧵 feat: Background-Native Code Execution Tools (#14532)
The code-execution pair (execute_code/bash_tool) now defaults INTO background dispatch whenever the run_in_background capability is enabled, the same way the SDK's coding tools carry `intent` natively: enabling the capability is enough, with no per-tool or per-spec flag required. An explicit run_in_background: false opts the pair out (by definition name, marker projection, or a narrowing spec selection's wildcard), and the builder Code toggle flips to opt-out semantics: absent reads as on, and turning it off persists an explicit false. A spec's runInBackground: false now synthesizes an explicit wildcard opt-out instead of staying a silent no-op. Pre-native, false and absent were behaviorally identical, so a config that wrote false must not silently flip to backgrounding code. The ephemeral toggle's false stays no-policy: it is a badge default, not a decision. |
||
|
Danny Avila
|
3f02efdef9
|
⚡ feat: Interrupt & Steer (Initial UI) (#14528)
Some checks are pending
Docker Dev Branch Images Build / build (Dockerfile, lc-dev, node) (push) Waiting to run
Docker Dev Branch Images Build / build (Dockerfile.multi, lc-dev-api, api-build) (push) Waiting to run
GitNexus Index / index (push) Waiting to run
GitNexus Index / post-index (push) Blocked by required conditions
* 🛑 feat: Preemptive Steer — server half (Interrupt & Steer, PR 2 of 3)
Lets the steer route ask the generating replica to seal its live model
stream at the next provider-safe boundary instead of waiting for a tool
step. The run is never aborted, job status never changes, the partial
answer is kept, and generation resumes in the same assistant message
after the injected steer. Consumes the SDK seam in @librechat/agents
(danny-avila/agents#335, #346).
Transport: IEventTransport gains a fenced emitPreempt/onPreempt pair
beside abort. RedisEventTransport fans PREEMPT out on the SAME events
channel and subscription (no new connection, key, or subscribe call);
onPreempt returns a registration-scoped unsubscribe with the same
replacement-safe state-identity guard onAbort uses. InMemory implements
neither — single-process preempt lives entirely in the runtime set.
Runtime state: RuntimeJobState carries the per-generation request set,
createdAt-fenced and capped at STEER_QUEUE_MAX_DEPTH, plus a bounded
`cleared` tombstone so a late cross-replica arm cannot resurrect a
request whose steer already drained. registerPreemptSubscription
mirrors the abort registration's double fence (runtime identity +
generation createdAt); releaseAbortSubscription retires BOTH listeners
and the armed set, so every terminal path drops preempt state for free.
Public surface: requestPreempt (arm + fenced publish, never a rejection
surface, never touches job status), isPreemptRequested (O(1)
level-triggered poll), noteSteersRemoved (drain/cancel bookkeeping +
fenced clear), clearPreemptRequests (empty-boundary disarm).
One drain body, two boundaries: createSteerDrainHook (PostToolBatch)
and createSteerPreemptBoundaryHook (PreemptBoundary) share
drainAndBuildInjections, so the two injection sites cannot drift — the
SDK's provider-safety argument rests on identical HumanMessage shapes.
The shared body builds injections incrementally under a swallow-all
catch (a mid-loop throw still injects what was applied — those parts
are already persisted), clears preempt requests in finally, and
disarms the generation when a boundary drains nothing.
Request path: POST /chat/steer accepts preempt: true. The guard ladder
is unchanged in order and in every status code. A preempt request is
NEVER a rejection reason — without the capability the steer still
enqueues and the 202 echoes preempt: false. Armed strictly after a
successful enqueue; cancel disarms. The capability is read from the
OWNING replica's recorded `preemptCapable` rather than the route
replica's own SDK probe, so a rolling deploy cannot label a steer
"interrupting" that the older owner will only inject at a tool step.
Durable label: SteerQueueItem.preempt → TPendingSteer.preempt, so a
parked/claimed/replayed chip keeps its wording.
Run wiring: createRun registers the PreemptBoundary hook and threads
RunConfig.preemption, both gated on isSteerPreemptSupported() — a
separate probe from isSteeringSupported(), so the client affordance can
never arm against an SDK that only injects at tool boundaries.
buildSteerWiring builds both hooks from one shared closures object, so
preemption survives HITL pause/resume for free.
Honest finalization: an empty preempt boundary persists and emits with
unfinished: true — the same contract an abort gets — re-marked
explicitly because BaseClient has already saved the row as
unfinished: false by that point.
Not changed: no new job status, store method, Lua, SSE event type,
endpoint, or authorization surface. abortJob, completeJob,
transitionStatus, closeAndDrainSteers, getResumeState, emitChunk,
applySteerPart and the whole abort path are untouched.
Tests: 120 packages/api steering specs (preempt lifecycle, tombstone,
fences, caps, terminal release, both-boundary drain parity,
level-triggered poll, request/cancel arming, owner-capability
degradation) plus 5 in api for buildSteerWiring gating, and 2
Redis-gated cross-replica transport specs.
* 🔒 fix: Codex round 2 — evict tombstones, scope the empty-boundary disarm, honest resumes
All four server findings were fresh consequences of the round-1 fixes,
which is the review doing exactly what it should.
- Tombstone cap refused new entries instead of evicting. Every drained
or cancelled steer is tombstoned, not just preempting ones, so a
generation that processed 20 steers exhausted the set and the
late-arm race resurfaced silently. Now evicts oldest-first (Set
iteration is insertion-ordered), with the budget named
PREEMPT_TOMBSTONE_MAX rather than an inline expression.
- The empty-boundary disarm I added in round 1 wiped the generation's
ENTIRE armed set. A second steer can enqueue and arm between the
atomic drain returning empty and the disarm running — that arm is
backed by a live, uninjected queue item and must survive. The drain
now snapshots the armed ids BEFORE draining
(getArmedPreemptIds) and clearPreemptRequests takes an explicit id
list instead of clearing everything.
- HITL resume finalized with a hardcoded unfinished: false. The
boundary hook is re-registered on resume via buildSteerWiring, so a
resumed segment can end on an empty preempt boundary exactly like a
fresh one; finalizeResumedTurn now reads getPreemptStats() and the
halt reason, matching the normal request path.
- Ownership moves on resume, so the job's recorded preemptCapable must
describe the replica that will actually generate. Refreshed before
resumeCompletion; a job created on a capable replica that resumes on
an older one during a rolling deploy no longer acknowledges steers as
interrupting.
Tests: +3 (scoped disarm sparing a post-snapshot arm, oldest-first
tombstone eviction, id-list disarm). 122 packages/api steering specs
green.
* 🚨 fix: Codex round 3 — deserialize preemptCapable from Redis (feature was dead under Redis)
The P1 here is the most consequential defect in the whole feature, and
it was introduced by round 1's own capability fix.
- `RedisJobStore.serializeJob` writes booleans generically, so
`preemptCapable` reached Redis — but `deserializeJob` is an EXPLICIT
field map and had no line for it. Every `getJob()` therefore dropped
the flag, `job.metadata.preemptCapable` was always undefined, and
`handleSteerRequest` computed `preemptArmed: false` unconditionally.
Interrupt & steer would have silently degraded to ordinary
tool-boundary steering in EVERY Redis deployment — i.e. the feature
shipping as a no-op in production while passing every in-memory test.
Now deserialized, with a round-trip assertion in the metadata spec
that fails (`Received: undefined`) against the unfixed store.
- The resume capability refresh moved from just-before
`resumeCompletion` to immediately after `approvals.resolve` claims
the run. That call already flips the job back to `running`, so the
steer route accepts requests from that instant; leaving the refresh
135 lines later (across the whole client reconstruction) left a real
window where a steer read the PREVIOUS owner's capability. Not the
fully atomic transition Codex suggested — that reaches into the
approvals Lua — but it shrinks the window from seconds to one await,
which is proportionate for a label-accuracy issue.
Refuted: "avoid triggering preemption inside subagents". The premise —
that the run-wide poll can seal a subagent stream — does not hold
against the shipped SDK. Child graphs are constructed with
`subagentScope: true` (SubagentExecutor) and `preemption` is NOT
propagated into child inputs, while `canClaimPreemptSeal()` requires
`!subagentScope && preemption != null`. Both conditions fail
independently, so a subagent can never claim a seal and the boundary
cannot fire with `agentId` set. The `input.agentId != null` guard in
the hook is defensive depth, not the thing standing between us and the
described failure.
140 packages/api specs green.
* 🔁 fix: Codex round 4 — re-arm durable interrupt steers when resume moves owners
- An arm lives only in the owning replica's runtime plus a transient
pub/sub message, while the steer's `preempt` flag is durable on the
queue item. A HITL resume landing on a different replica therefore
started with an empty armed set and a poll stuck false, so an
interrupt the user had already been ACKed for silently waited for an
ordinary tool boundary. New `GenerationJobManager.rearmQueuedPreempts`
rebuilds the armed set by peeking the durable queue (fenced on the
generation) and re-arming every item flagged `preempt`; resume calls
it right after claiming. Safe by construction: every item peeked is
still queued, so no drained steer can be resurrected.
- Capability-refresh failure now logs at error rather than warn, but
deliberately does NOT fail the resume — see the reply on that thread.
Tests: +2 (rebuild from queue arms only the flagged item and reports
the count; a stale generation arms nothing). 124 packages/api steering
specs green.
* 📡 fix: Codex round 5 — acknowledge only what was actually armed
- A cross-replica arm was fire-and-forget: `emitPreempt` logged its own
publish failure and `requestPreempt` returned void, so the route
answered `preempt: true` even when the owner never armed a poll. The
steer still injected at the next tool boundary, but the chip claimed
an interrupt that could not happen — and unlike HITL resume, an
ordinary running generation had no durable reconciliation to recover
it.
`emitPreempt` now resolves to the subscriber count and rejects on
failure; `requestPreempt` is async and returns whether the arm truly
landed (owned locally, or delivered to at least one subscriber). The
202 reports THAT rather than what was asked for, so the chip relabels
to ordinary steering exactly as it does for a capability-degraded
deployment. Errors are swallowed into `false` — an unarmed interrupt
is a downgrade, never a failed steer.
- The owner capability is re-read immediately before enqueue rather
than reused from the top of the guard ladder. `checkAgentAccess` and
file resolution are awaits, so a request can span an entire HITL
pause/resume that moves ownership to a replica with different
capability and rewrites that very flag. Only paid for by requests
that actually asked to interrupt.
Tests: +3 (not-armed when the publish reaches nobody; armed when this
replica owns the generation; a throwing publish downgrades instead of
propagating). 127 packages/api steering specs green.
* 🎯 fix: Codex round 6 — real ownership, confirmed disarms, and a CI regression of my own
Three review findings plus three CI failures the round-5 commit caused.
Review:
- Ownership came from `runtimeState`, which a cross-replica `getJob`
populates with a FACADE runtime on any replica that merely read the
job. Matching `createdAt` therefore proved only "we looked at this
job", so a non-owner could arm nothing and report success. Ownership
now comes from `ownedJobs`, the actual owner map.
- `armPreemptIds` returns how many ids it accepted, and a local arm is
only reported as armed when one was. A tombstoned id (its steer
drained at an ordinary boundary mid-request) no longer answers
`preempt: true` for an interrupt that cannot happen.
- The cancel disarm is awaited. A dropped clear is worse than a dropped
arm: the owner keeps a level-triggered request for a steer that no
longer exists, seals its next chunk and truncates an unrelated
answer. The boundary drain's own call stays non-blocking — there the
owner is local, so the disarm is already effective and awaiting the
informational publish would only delay injection.
- Subscriber count is NOT read as proof of owner receipt: the count
includes this replica's own facade subscription. A successful publish
reports armed, a rejected one does not. Documented rather than
papered over — see the acknowledgement-semantics note on the PR.
CI regressions from round 5, all mine:
- `registerPreemptSubscription` was AWAITED at both runtime-init sites,
so job creation blocked on a second Redis channel subscription and
hung when that subscribe was slow. Abort is awaited because a missed
abort strands a run; a missed preempt only degrades that steer to the
next tool boundary, so it now registers without gating createJob.
- Two api specs mocked `@librechat/api` without the newly imported
`isSteerPreemptSupported`, so the call threw before createJob; and one
exact-match assertion needed the new `preemptCapable` metadata field.
- My own Redis integration spec asserted arm-before-clear ordering,
which two publishes carry no guarantee of — the receiving tombstone
exists precisely because of that. Now asserts delivery and payload
fidelity, order-independent.
158 packages/api specs, 27 api specs green.
* 🧭 fix: Codex round 7 — settle the acknowledgement semantics (Option A)
Round 7's second finding is the incoherence I flagged on the PR: the
route persisted `preempt: true` on the durable queue item while
returning `preempt: false` when delivery could not be confirmed. Those
two then disagreed, and `rearmQueuedPreempts` trusts the DURABLE one —
so a resumed owner would honour an interrupt the client had explicitly
been told degraded to ordinary steering.
Rather than patch the disagreement, this settles the meaning:
`preempt` in the 202 means "queued as an interrupt request", NOT "a
seal is guaranteed". It mirrors `SteerQueueItem.preempt` exactly, so
the response, the durable record, and the resume-time re-arm can never
disagree. The gates that ARE knowable stay — the owner's recorded
capability and a successful enqueue. Everything past that degrades to
the documented fallback of injecting at the next tool boundary.
A route cannot synchronously know whether another replica will seal:
proving it needs a correlated request/response over pub-sub, and even
that only proves the owner heard, not that it is still streaming when
the arm lands. Four rounds of tightening this boolean each surfaced a
narrower case; the sequence does not converge, so the invariant is now
"the flag describes the durable decision" and an unconfirmed arm logs a
warning instead of rewriting the answer.
Also from this round: a failed disarm publish is retried once and its
outcome reported. `handleSteerCancel` keeps `removed: true` — the steer
really did leave the queue, and saying otherwise would make the client
re-show a chip for a steer that can never arrive — and adds
`disarmed: false` so the residual risk is visible rather than swallowed.
Damage stays bounded regardless: the empty-boundary self-clear disarms
the generation after a single seal.
Tests: +1 pinning the response/durable-flag invariant. 159
packages/api specs green.
* 🧹 fix: Codex round 8 — remove the unverifiable disarm signal
Round 8 found the same over-promise on the disarm side that round 7
corrected on the arm side, so this applies the same answer rather than
patching around it.
The `disarmed: false` field added in round 7 was both unreliable and
unused: a resolved publish is not proof the owner heard it (the
delivery count includes this replica's own facade subscription), and it
was never threaded into `CancelSteerResponse` or read by any client. A
signal that claims a certainty the transport cannot provide is worse
than no signal — it invites callers to trust it.
Removed from the response. The retry stays, because it genuinely
reduces the failure rate, and `noteSteersRemoved` still returns whether
the publish succeeded FOR LOGGING, now documented explicitly as
"published without error", not "the owner disarmed".
Disarm is best effort with a bounded, self-healing failure: if the
clear is lost the owner seals once, the empty-boundary self-clear
disarms the generation, and the turn is persisted `unfinished: true`
rather than silently truncated. Tightening that further needs a
correlated request/response over pub-sub with a timeout — noted on the
PR as the deliberate boundary of this design rather than an oversight.
130 packages/api steering specs green.
* 🧽 fix: Codex round 9 — spend snapshot arms on nonempty drains too
The round-6 scoping fix only cleared the pre-drain snapshot when the
drain came back EMPTY. On a nonempty drain the `finally` cleared just
the drained ids, so a stale arm — typically a cancel whose
cross-replica clear was lost — survived the boundary. It would then
immediately seal the continuation meant to answer the steer that had
just been injected, and land on an empty boundary as
`preempt_incomplete`: the interrupt appears to work, and the answer to
it is truncated.
A boundary that runs has spent its seal, so everything armed at
snapshot time is spent whether or not it came back from the drain. The
`finally` now clears the union of the snapshot and the drained ids.
Arms that land AFTER the snapshot are still spared — their queue items
are live and uninjected, which is the property round 6 added.
Also fixes an api-workspace CI failure of mine: `resume.spec.js` stubs
`GenerationJobManager` wholesale, and the round-3/4 resume work added
two calls (`updateMetadata`, `rearmQueuedPreempts`) the stub did not
define, so 34 specs threw. Stub extended.
Tests: +2 (a nonempty drain clears a stale snapshot arm; a nonempty
drain spares an arm that landed mid-drain). Counterfactually verified —
the stale-arm spec fails against the unfixed drain. 132 packages/api
specs, 60 resume specs green.
* fix: never let a failed preempt subscription reject into the void
registerPreemptSubscription is called detached at both sites, so a
rejected Redis SUBSCRIBE became an unhandled rejection — process-fatal
under Node's default --unhandled-rejections=throw. The comment already
promised this path merely degrades steering; it now does.
Swallowed and logged inside the registration rather than at each call
site, so a future third caller cannot reintroduce the trap. Losing the
channel costs this generation's cross-replica preempts, not the server:
same-replica arming is runtime state and still works, and remote arms
fall back to the next tool boundary.
Verified counterfactually — the new spec surfaces SUBSCRIBE failed as an
unhandled rejection against the unfixed registration.
* docs: state the real blast radius of a failed preempt subscription
LibreChat's own entrypoints install a global unhandledRejection handler
that logs and keeps serving, so the escaping rejection this guards was
never fatal to this server — only to another consumer of @librechat/api
that installs no handler. The fix stands either way; the comment just
should not overstate what it prevents.
* test: cover the cross-replica preempt hop with two manager instances
Every other preempt test runs against a single manager, so the hop that
actually carries an interrupt in production had no coverage: the steer POST
lands on whichever replica the balancer picks, which is usually not the one
generating. Non-owner publishes, owner arms, owner's level-triggered poll
flips — none of that was exercised end to end.
Two GenerationJobManagerClass instances are a faithful replica pair here.
runtimeState and ownedJobs are private instance fields, there is no
module-level mutable state between them, and createStreamServices duplicates
a dedicated subscriber connection per call, so separate OS processes would
exercise the same objects over the same Redis.
Both assertions verified counterfactually against real Redis:
- Deleting the preemptCapable deserialization in RedisJobStore fails this
with 'Expected: true, Received: undefined' — the exact P1 that shipped past
every in-memory test and would have made the feature a silent no-op on
every Redis deployment.
- Dropping the non-owner arm publish fails it with 'Received: false'.
* test: remove the fixed sleeps and vacuity from the cross-replica preempt test
Codex round 11, both findings, both on the test I added last commit.
P2 — the 300ms waits were load-bearing. Redis pub/sub never replays and the
owner's SUBSCRIBE is detached, so on a slow CI worker the publish could land
before anyone was listening and the test would fail against correct code.
Now it republishes until the owner's state converges, which is safe because
arms and clears are idempotent set writes keyed by steerId. Side effect: the
tests got ~10x faster (85ms/57ms vs 929ms/606ms) since they finish on
delivery rather than on a timer.
P3 — afterEach destroyed only the transports, leaving each manager alive in
its own cleanup-interval closure, still working against a dead transport.
Now tracks the managers and awaits destroy(), which disposes the job store
and its timer too. Matches how the rest of this file cleans up.
Fixing the sleeps exposed a third problem codex did not flag: the stale-arm
test could pass vacuously, because an undelivered arm and a fenced one look
identical. It now brackets the stale publish between two control arms — the
first proves the owner is listening before the stale one is sent, the second
proves it has had its chance to arrive.
Verified counterfactually against real Redis, and stable over 5 runs:
- dropping the preemptCapable deserialization fails with 'Received: undefined'
- dropping the non-owner arm publish times out both tests
- removing the generation fence fails the stale test with
["control-before", "steer-stale", "control-after"] — which also confirms
the bracketing orders as intended rather than by luck
* fix: gate interrupt on the OWNER's capability alone, not the route's
Codex round 12. The comment above this gate already said 'the OWNER's
recorded capability, not this replica's probe' — and then the code ANDed in
isSteerPreemptSupported(), which is exactly this replica's probe. The
contradiction dates to the original commit; round 6 made the gate
owner-scoped and wrote that comment without removing the local conjunct.
The route never seals. It enqueues and publishes an arm, neither of which
touches the SDK, so during a rolling deploy a steer landing on an
un-upgraded replica silently lost its interrupt even though the owner could
seal. When the route IS the owner the probe is redundant anyway: the flag it
would consult is the one this process wrote at createJob.
The real degradation path is unchanged and still tested — an owner that
recorded no capability relabels to an ordinary steer. The test that pinned
the local probe asserted an impossible same-replica state (capable metadata
plus an incapable local SDK, when the metadata is written from that probe);
it now pins the mixed-SDK direction instead, and fails with
'Expected: true, Received: false' if the probe is put back.
* fix: reconcile arms at handover, and stop holding the 202 on a publish
Codex round 13, two of three findings.
P2 — rearmQueuedPreempts only ever ADDED. A replica that merely read the job
still installs a facade runtime and subscribes, so it can accept an arm and
then miss the best-effort clear that follows the drain. HITL resume promotes
that facade to owner, the union keeps the orphan, and the first resumed
stream seals on a steer no longer in the queue, drains nothing, and
truncates the resumed answer as preempt_incomplete. acquireResumedJobOwnership
only sets ownedJobs, so nothing else was clearing it. The durable queue is
the sole authority at a handover: arms it does not back are now disarmed and
tombstoned, so an in-flight publish cannot revive them either.
Worth recording that my own independent review raised this and my verifier
refuted it. Codex found it separately; two reviewers converging should have
outweighed one refutation.
P2 — the route awaited the arm publish before answering. The 202 reports
capability, not delivery, so the await could not change the response; it only
exposed the caller to Redis latency after the queue item was already durable.
A client that times out and retries mints a second steer while the first
stays queued, injecting the same instruction twice, whereas a lost publish
merely takes the tool-boundary fallback. Detached, with both outcomes logged.
All three tests verified counterfactually: union-only rearm fails the two new
handover specs, and re-awaiting the publish hangs the stalled-publish spec
until jest kills it.
* fix: snapshot arms before reading the queue at handover
Codex round 14 — a regression from my own round-13 fix, and a worse failure
than the one it corrected.
Round 13 read the durable queue first, then tombstoned any armed id the
snapshot did not back. But approvals.resolve reopens steering before
reconciliation runs, so another replica can commit a preempt steer and
publish its arm while the peek is in flight. That arm is then present locally
but absent from a snapshot taken before the steer existed, so a LIVE
interrupt the route already acknowledged got dropped — and tombstoned, which
blocks the re-arm, making it unrecoverable rather than merely late.
Fixed by inverting the two reads rather than by locking or paying a second
round trip. A steer is durably enqueued BEFORE its arm is published, so any
id in an arms-first snapshot was already queued when it was armed, and the
later peek must observe it unless it has since drained — which is exactly the
orphan this reconciliation exists to drop. Arms landing after the snapshot
are simply not candidates.
Also re-checks runtime identity across the await, since the generation can be
replaced while the queue read is in flight.
New spec injects a steer + arm during the peek and verifies it survives;
against the round-13 ordering it fails with Received array: [].
* fix: bound the cancel disarm wait and fence enqueue to its generation
Codex round 15.
P2 — the cancel awaited its disarm publish unbounded. ioredis queues
commands during an outage rather than rejecting, so that await could hang for
the length of the outage with the steer ALREADY durably cancelled; a client
that gives up then treats the cancel as failed and restores a chip for a
steer that can never produce an applied event. Every successful cancel
publishes, so ordinary steers were exposed too, not only preemptive ones.
Now bounded at 1s, with the publish continuing behind it — its retry and
logging are unchanged, it is just no longer in front of the response. This is
the sibling of round 13's arm-publish finding; I fixed one path and left this
one.
P3 — enqueue was not fenced to the generation the capability decision was
made against. The access checks, file resolution and owner re-read are all
awaits, so the run can be replaced before the enqueue: the item then lands on
the REPLACEMENT queue while the durable preempt flag and the arm still name
the previous epoch, the arm is fenced out at the owner, and the 202 promises
an interrupt that cannot happen. enqueueSteer now takes an expected
generation, mirroring drain/peek, and the Redis path enforces it inside
STEER_ENQUEUE_LUA so the check is atomic with the push rather than racing it.
All three new specs verified counterfactually, including the Lua guard
against real Redis (removing it returns 1 where -1 is required).
* ⚡ feat: Interrupt & Steer — client half (PR 3 of 3)
Makes preemptive steering reachable. Consumes the server contract from
PR 2 (POST /chat/steer `preempt`, echoed on the 202) and the SDK seam
in @librechat/agents 3.3.5.
Settings shape follows the agreed correction, NOT the earlier plan
draft: `steerInterruptsByDefault` is a boolean ORTHOGONAL to
`duringRunDefaultAction` — that enum still chooses steer-vs-queue, the
new boolean chooses how soon a steer lands. This deliberately avoids
widening the enum to three values, which would have silently broken two
hard-coded binary TOGGLES (`DuringRunAction.tsx`'s setter and
`SteerMenu`'s `useDefaultToggleEntry`, both `prev === 'steer' ? … : …`)
where a third value collapses to the wrong branch and one click erases
the setting.
- useSteering: `submitSteer` takes an opts bag and threads `preempt`
into the POST, the optimistic chip, and the failure chip. The ACK
relabels from the SERVER's echo, so a deployment that cannot seal
mid-stream downgrades the chip's wording instead of erroring — the
entire UX surface of capability degradation. New `interruptSteer`
reuses the whole chip lifecycle and degradation ladder, and falls
back to `interruptAndSend` when `!canSteer`, because steering needs a
server-side job and an always-visible button would otherwise be dead
for the whole first turn. `steerFromComposer` honours the new
preference.
- Composer: always-visible `InterruptSteerButton` with one fixed
meaning (stop now, keep what's written), disabled on a paused run to
pre-empt the server's 409, `type="button"` so it never steals the
form's Enter submit, RTL-correct margins. A fourth hovercard row on
the during-run send button, and ⌘/Ctrl+Shift+Enter routed AHEAD of
the bare ⌘/Ctrl+Enter branch that would otherwise swallow it.
- Chips: an in-flight preempt chip reads "Interrupting" with a ZapOff
glyph; `preempt` survives reconnect through `seedSteerChips`.
- `RunEnd.interruptArmed`, `drainAfterAbortByIndex`, `useQueueDrain`,
`stopGenerating` and `interruptAndSend` are untouched — the preempt
path deliberately shares none of the abort machinery.
Tests: 7 new specs (posts preempt, turn-1 fallback, empty-text refusal,
default route with and without the preference, server-echo relabel,
double-click). 66 useSteering specs green; tsc and lint clean.
Round-1 review fixes folded in:
- P1: interrupt & steer no longer hard-aborts a run paused on tool
approval. `canSteer` is false there, so the fallback was routing the
keyboard and hovercard paths into `interruptAndSend` — discarding the
partial answer, the exact opposite of what the action promises. The
fallback is now scoped to the missing-conversation case only, and a
paused run refuses outright (the standalone button was already
disabled; the guard now lives where all three paths reach it).
- The preference no longer leaks into the explicit Steer action.
`steerFromComposer` backs both the default Enter route AND the
explicit hovercard row / Ctrl+Enter alternate; applying
`steerInterruptsByDefault` inside it made ordinary Steer interrupt and
the two rows indistinguishable. It now takes an explicit argument that
only `submitDuringRun`'s default route sets.
- Retry preserves preemption: a failed interrupt-steer chip keeps
`preempt: true`, and `retrySteer` now forwards it rather than silently
resubmitting as an ordinary tool-boundary steer.
- ⌘/Ctrl+Shift+Enter defers to a rebound submit shortcut, mirroring the
bare ⌘/Ctrl+Enter branch — a user who bound submit to that chord keeps
getting submit.
Round-2 fix: the preempt label now survives the page-reload resume path
too. `seedSteerChips` (useResumableSSE) and `restoreSteerChips`
(useResumeOnLoad) are two independent TPendingSteer→PendingSteer
mappers with near-identical bodies; the first carried the flag and the
second silently dropped it, so an armed interrupt reverted to plain
"Steering" after a reload. Swept: those are the only two in production
code. The reclaim/convert paths deliberately omit it — a queued
follow-up starts its own turn, so there is nothing to interrupt.
* fix: yield the interrupt-steer chord only to a submit shortcut bound to it
The previous guard skipped the Ctrl/Cmd+Shift+Enter branch whenever ANY
submitMessage override existed. Rebinding submit to something unrelated
(Ctrl+J) or unbinding it entirely then fell through to the override
resolver, which returns 'none' for shifted Enter — silently removing the
shortcut the hovercard still advertises.
Compare the pressed chord against the configured one instead. The
adjacent bare Ctrl/Cmd+Enter branch keeps its any-override guard on
purpose: that chord IS the default submit chord, so once submit moves
the resolver should own it.
The predicate already existed inside resolveSubmitOverrideAction; pulled
it out as bindingsMatch so both sites compare chords the same way. That
call is behavior-preserving — eventBinding.key is 'Enter' by the early
return, and equal hashes imply equal keys, so the dropped explicit key
check was redundant.
* fix: disable the Interrupt & steer menu row while paused on approval
interruptSteer hard-refuses when the run is paused for tool approval, but
the hovercard row was never gated, so it rendered enabled and clicking it
did nothing at all — no chip, no queue entry, no toast — at exactly the
moment a user is trying to say "stop, don't run that command". The
standalone button already gates on pausedOnApproval; the row contradicted
it.
Gated on pausedOnApproval rather than !canSteer like the steer row above,
because canSteer is also false before a conversation exists, where
interruptSteer deliberately falls back to interruptAndSend and the row
must stay live for the whole first turn.
Tests pin both directions and were verified counterfactually: removing the
gate fails the paused case, and using !canSteer fails the first-turn case.
* test: render the during-run hovercard eagerly instead of driving Ariakit
The new spec passed locally and failed all four cases on CI's Ubuntu and
Windows shards: Ariakit's show path keys off pointer geometry, which jsdom
reports as zeros, so whether a synthetic mouseEnter opens the hovercard is
environment-dependent. Driving it was testing Ariakit's hover behavior, not
which rows this component disables.
Mocking the three Ariakit primitives renders the rows unconditionally and
drops the fake timers. Both counterfactuals still fail as they should:
removing the gate fails the paused case, !canSteer fails the first-turn case.
* test(e2e): cover interrupt & steer sealing mid-stream
The mock Playwright suite covered every sibling during-run action — steer at
a tool boundary, steer degrading to a queued follow-up, queue, and interrupt
& send — but not interrupt & steer, the one this stack adds.
Uses E2E_SLOW_REPLY, which streams pure text with no tools, so the scenario
is the same one where an ordinary steer provably degrades to a queued
follow-up turn. Injecting in-thread there is something only a mid-stream
seal can do, which makes the assertion discriminating rather than incidental:
the steer part lands in the response, the final chunk never arrives, the text
written before the seal survives, and no follow-up turn pair is created.
* test(e2e): assert the run resumes after the seal, not just that it sealed
The other four assertions are all satisfied by a seal that killed the run:
the steer part is persisted by applySteer during the drain, before the
continuation starts, so 'sealed and resumed' and 'sealed and died' were
indistinguishable — and resuming is the whole difference from interrupt &
send.
The continuation answers the injected steer, whose text carries no
fake-model marker, so getLatestUserText falls through to the default reply.
That string ('E2E mock reply') is distinct from the setup turn's
('E2E reply <label>'), so seeing it proves generation restarted rather than
matching text that was already on screen.
The test itself is confirmed working: it ran as 104/121 in the Playwright
job on
|
||
|
Danny Avila
|
d5819becf2
|
🎯 feat: Per-Tool Intent Labels for Model Specs (#14526)
* 🎯 feat: Per-Tool Intent Labels for Model Specs A model spec could only turn intent labels on for ALL of its eligible tools. Saved agents have had per-tool control since the capability landed (`tool_options[id].describe_intent`, with the builder toggle following in the UI slice), but a model spec is admin YAML that produces an ephemeral agent — there is no agent document to hold per-tool options, so `describeIntent: true` synthesized an entry for every eligible tool. `describeIntent` now accepts a string array alongside the boolean, matching the `skills` field already in the same schema: describeIntent: true # every eligible tool describeIntent: ['web_search', 'search_code_mcp_github'] This matters because the label costs schema tokens on every request, so an admin may want it on a handful of illegible calls rather than the whole toolset. - Named tools still pass eligibility, so an excluded tool cannot be forced on by listing it. - An empty array reads as disabled. - Names that are not eligible or not equipped on the spec are logged rather than silently skipped — a typo in a spec would otherwise be undiagnosable. - The ephemeral toggle stays boolean and stays global even when a spec list is present: it has no per-tool UI to drive it, so narrowing it would silently cover fewer tools than the user asked for. `runInBackground` has the same all-or-nothing limitation and could take the same shape; left alone here to keep this change reviewable. * 🎯 feat: Per-Tool Background Dispatch for Model Specs Gives `runInBackground` the same `boolean | string[]` shape as `describeIntent`, so the two per-tool capabilities are configured identically from a model spec: runInBackground: true # every eligible tool runInBackground: ['slow_report_mcp_analytics'] # only this one Selectivity matters more here than for intent labels. An intent label is inert — it costs tokens and nothing else. Backgrounding changes execution semantics: the model gets a synthetic handle and must poll. Letting an admin detach one slow MCP call without making every other tool in the spec detachable is the difference between a usable setting and an all-or-nothing one. Same guarantees as the intent equivalent: - Named tools still pass eligibility, so the exclusion list still holds — a list cannot force on web_search, file_search, image gen, the HITL tool, or anything whose attachments/artifact continuity would break. - Empty array reads as disabled. - Unmatched names are logged rather than silently skipped. - The ephemeral toggle stays boolean and global; it has no per-tool UI. Also fixes a latent no-op: this function did not skip the lazily-expanded `mcp_all` placeholder, so a spec with an overlay MCP server recorded an option under a name `applyBackgroundToolCalls` can never match. The intent equivalent already skipped it; now both do. * ♻️ refactor: One Definition of the mcp_all Placeholder Guard Fixing the background no-op left the placeholder prefix declared twice — once per capability synthesizer — which is the same duplicated-literal shape that made the intent label marker fragile: two copies that must agree, with drift producing a silent no-op rather than an error. `MCP_ALL_PLACEHOLDER_PREFIX` and `isMCPAllPlaceholder` now live beside `mcpToolPattern` in mcp/utils, so both synthesizers cannot disagree about which tool entries to ignore, and anything added later that keys per-tool config by exact name has an obvious guard to reach for. Audited the rest of the capability family while here: only background and intent synthesize per-tool options from a model spec. `defer_loading` and `allowed_callers` have no model-spec path at all, so neither can carry this bug. Both synthesizers now have an explicit regression test naming the placeholder. * 🧯 fix: Treat a describeIntent List as a Selection Policy, Not a Filter Two build/behavior defects from the previous commits. **Narrowing did not actually narrow.** Omitting a tool from the synthesized options is not the same as opting it out, because intent has two default-on paths background does not: `isIntentOptedIn` treats every NATIVE_INTENT_TOOL_NAMES member as enabled when it finds no entry, and `sanitizeIntentLabels` keeps an SDK-native label unless it sees an explicit `describe_intent: false`. So `describeIntent: ['web_search']` still labelled `set_memory`, and an empty list — the most explicit way to say "none" — disabled nothing at all. A list is now a selection policy: selected eligible tools get true, unselected get an explicit false. Ineligible tools still get no entry at all. Background needs no equivalent change: it opts in on `run_in_background === true` only, with no default-on set, so omission there genuinely means off. **Fixed the CI build break.** `MCP_ALL_PLACEHOLDER_PREFIX` was exported without a type annotation, which `tsc --noEmit` accepts but the package build rejects under `--isolatedDeclarations` (TS9010) — the same reason `mcpToolPattern` beside it is annotated `: RegExp`. Verified with an actual `npm run build` this time, not just a typecheck. * 🧯 fix: Propagate Intent Selection Through Capability Marker Expansion A model spec's `tools` carries capability MARKERS, not the definition names initialization actually registers, so an option recorded under a marker never matches the tool it becomes — the same silent no-op as an `mcp_all` placeholder entry. Harmless for an opt-IN (the tool keeps its default) but not for the opt-OUTs the previous commit introduced: `memory` becomes `set_memory` + `delete_memory`, both default-on natives, and `execute_code` becomes `bash_tool`, which carries an SDK-native label that survives unless sanitize sees an explicit false. So `describeIntent: ['web_search']` on a spec with `memory: true` still labelled both memory tools, and `[]` disabled neither. `expandIntentToolOptions` propagates a marker's value to the names it expands into, mirroring `expandCodeToolOptions` in background.ts which solves the same problem for the code marker. Applied at APPLY time rather than synthesis, so hand-edited saved agents that key options by marker benefit too, not just synthesized specs. An explicit per-tool entry always wins — expansion only fills names the caller did not already decide — and it runs in both `applyIntentLabels` and `sanitizeIntentLabels`, since the SDK-native strip reads the same options. Verified with an actual package build, not just a typecheck. * ♻️ refactor: Resolve Spec Tool Selections Against Final Definitions Three review rounds hit the same root cause from three directions: a per-tool option synthesized at load time is keyed by names that may not exist at injection time. Spec tools carry capability markers (execute_code, memory), skills never reach the tools array at all, and lazy MCP servers expand after synthesis - every mismatch was a silent no-op, and each fix added another hand-maintained marker map that the next case leaked past. This removes the name-space gap instead of bridging it per case: - Synthesis records the selection as a policy: a wildcard '*' entry carries the default (true for "every tool", false for "only the named ones") and listed names are recorded verbatim. No load-time enumeration, eligibility checks, or placeholder special-casing. - Resolution happens at injection time, per final definition: explicit name -> capability marker projection -> wildcard. The projection maps a marker onto the names its registration actually produced this run - the registrars report their own tool names and initializeAgent accumulates them - so the mapping cannot drift from what gets registered. - The unmatched-name diagnosis moves to the apply passes, the one place the real definitions are known, so a selection naming a marker whose runtime expansion is entirely ineligible (runInBackground: ['memory']) is now warned about instead of recorded as a dead success. Covers all four round-3 findings: code opt-outs now reach create_file/edit_file/read_file, skill definitions are governed by narrowing selections, the memory marker is rejected and diagnosed for backgrounding, and the mcp_all placeholder predicate is no longer consulted for selections at all (its one remaining consumer is the definitions loader that defines the convention). * 🧯 fix: Reject Dead Ask-Tool Selections and the Reserved Wildcard Two review findings on the selection policy, both fixed at the point where they are knowably invalid: - ask_user_question joins EXCLUDED_INTENT_TOOL_NAMES: createRun strips its provisional definition and rebuilds the graph tool from its own Zod schema, so definition-level injection never reaches the model. A describeIntent selection naming it now warns as ineligible instead of crediting a label that gets discarded. Real intent support for the ask tool lands with the HITL slice via the interrupt payload. - A literal '*' in a describeIntent/runInBackground list is dropped with a warning at synthesis: it would overwrite the wildcard opt-out default and silently enable the capability for every eligible tool instead of selecting one named tool. The wildcard is reserved for the internal policy; boolean true is the supported way to cover everything. |
||
|
Danny Avila
|
8af6414e13
|
🪟 fix: Surface MCP Initialization Errors (#14529) | ||
|
Danny Avila
|
7bb6651883
|
🛑 feat: Preemptive Steer - Backend Interrupt & Steer (#14518)
* 🛑 feat: Preemptive Steer — server half (Interrupt & Steer, PR 2 of 3) Lets the steer route ask the generating replica to seal its live model stream at the next provider-safe boundary instead of waiting for a tool step. The run is never aborted, job status never changes, the partial answer is kept, and generation resumes in the same assistant message after the injected steer. Consumes the SDK seam in @librechat/agents (danny-avila/agents#335, #346). Transport: IEventTransport gains a fenced emitPreempt/onPreempt pair beside abort. RedisEventTransport fans PREEMPT out on the SAME events channel and subscription (no new connection, key, or subscribe call); onPreempt returns a registration-scoped unsubscribe with the same replacement-safe state-identity guard onAbort uses. InMemory implements neither — single-process preempt lives entirely in the runtime set. Runtime state: RuntimeJobState carries the per-generation request set, createdAt-fenced and capped at STEER_QUEUE_MAX_DEPTH, plus a bounded `cleared` tombstone so a late cross-replica arm cannot resurrect a request whose steer already drained. registerPreemptSubscription mirrors the abort registration's double fence (runtime identity + generation createdAt); releaseAbortSubscription retires BOTH listeners and the armed set, so every terminal path drops preempt state for free. Public surface: requestPreempt (arm + fenced publish, never a rejection surface, never touches job status), isPreemptRequested (O(1) level-triggered poll), noteSteersRemoved (drain/cancel bookkeeping + fenced clear), clearPreemptRequests (empty-boundary disarm). One drain body, two boundaries: createSteerDrainHook (PostToolBatch) and createSteerPreemptBoundaryHook (PreemptBoundary) share drainAndBuildInjections, so the two injection sites cannot drift — the SDK's provider-safety argument rests on identical HumanMessage shapes. The shared body builds injections incrementally under a swallow-all catch (a mid-loop throw still injects what was applied — those parts are already persisted), clears preempt requests in finally, and disarms the generation when a boundary drains nothing. Request path: POST /chat/steer accepts preempt: true. The guard ladder is unchanged in order and in every status code. A preempt request is NEVER a rejection reason — without the capability the steer still enqueues and the 202 echoes preempt: false. Armed strictly after a successful enqueue; cancel disarms. The capability is read from the OWNING replica's recorded `preemptCapable` rather than the route replica's own SDK probe, so a rolling deploy cannot label a steer "interrupting" that the older owner will only inject at a tool step. Durable label: SteerQueueItem.preempt → TPendingSteer.preempt, so a parked/claimed/replayed chip keeps its wording. Run wiring: createRun registers the PreemptBoundary hook and threads RunConfig.preemption, both gated on isSteerPreemptSupported() — a separate probe from isSteeringSupported(), so the client affordance can never arm against an SDK that only injects at tool boundaries. buildSteerWiring builds both hooks from one shared closures object, so preemption survives HITL pause/resume for free. Honest finalization: an empty preempt boundary persists and emits with unfinished: true — the same contract an abort gets — re-marked explicitly because BaseClient has already saved the row as unfinished: false by that point. Not changed: no new job status, store method, Lua, SSE event type, endpoint, or authorization surface. abortJob, completeJob, transitionStatus, closeAndDrainSteers, getResumeState, emitChunk, applySteerPart and the whole abort path are untouched. Tests: 120 packages/api steering specs (preempt lifecycle, tombstone, fences, caps, terminal release, both-boundary drain parity, level-triggered poll, request/cancel arming, owner-capability degradation) plus 5 in api for buildSteerWiring gating, and 2 Redis-gated cross-replica transport specs. * 🔒 fix: Codex round 2 — evict tombstones, scope the empty-boundary disarm, honest resumes All four server findings were fresh consequences of the round-1 fixes, which is the review doing exactly what it should. - Tombstone cap refused new entries instead of evicting. Every drained or cancelled steer is tombstoned, not just preempting ones, so a generation that processed 20 steers exhausted the set and the late-arm race resurfaced silently. Now evicts oldest-first (Set iteration is insertion-ordered), with the budget named PREEMPT_TOMBSTONE_MAX rather than an inline expression. - The empty-boundary disarm I added in round 1 wiped the generation's ENTIRE armed set. A second steer can enqueue and arm between the atomic drain returning empty and the disarm running — that arm is backed by a live, uninjected queue item and must survive. The drain now snapshots the armed ids BEFORE draining (getArmedPreemptIds) and clearPreemptRequests takes an explicit id list instead of clearing everything. - HITL resume finalized with a hardcoded unfinished: false. The boundary hook is re-registered on resume via buildSteerWiring, so a resumed segment can end on an empty preempt boundary exactly like a fresh one; finalizeResumedTurn now reads getPreemptStats() and the halt reason, matching the normal request path. - Ownership moves on resume, so the job's recorded preemptCapable must describe the replica that will actually generate. Refreshed before resumeCompletion; a job created on a capable replica that resumes on an older one during a rolling deploy no longer acknowledges steers as interrupting. Tests: +3 (scoped disarm sparing a post-snapshot arm, oldest-first tombstone eviction, id-list disarm). 122 packages/api steering specs green. * 🚨 fix: Codex round 3 — deserialize preemptCapable from Redis (feature was dead under Redis) The P1 here is the most consequential defect in the whole feature, and it was introduced by round 1's own capability fix. - `RedisJobStore.serializeJob` writes booleans generically, so `preemptCapable` reached Redis — but `deserializeJob` is an EXPLICIT field map and had no line for it. Every `getJob()` therefore dropped the flag, `job.metadata.preemptCapable` was always undefined, and `handleSteerRequest` computed `preemptArmed: false` unconditionally. Interrupt & steer would have silently degraded to ordinary tool-boundary steering in EVERY Redis deployment — i.e. the feature shipping as a no-op in production while passing every in-memory test. Now deserialized, with a round-trip assertion in the metadata spec that fails (`Received: undefined`) against the unfixed store. - The resume capability refresh moved from just-before `resumeCompletion` to immediately after `approvals.resolve` claims the run. That call already flips the job back to `running`, so the steer route accepts requests from that instant; leaving the refresh 135 lines later (across the whole client reconstruction) left a real window where a steer read the PREVIOUS owner's capability. Not the fully atomic transition Codex suggested — that reaches into the approvals Lua — but it shrinks the window from seconds to one await, which is proportionate for a label-accuracy issue. Refuted: "avoid triggering preemption inside subagents". The premise — that the run-wide poll can seal a subagent stream — does not hold against the shipped SDK. Child graphs are constructed with `subagentScope: true` (SubagentExecutor) and `preemption` is NOT propagated into child inputs, while `canClaimPreemptSeal()` requires `!subagentScope && preemption != null`. Both conditions fail independently, so a subagent can never claim a seal and the boundary cannot fire with `agentId` set. The `input.agentId != null` guard in the hook is defensive depth, not the thing standing between us and the described failure. 140 packages/api specs green. * 🔁 fix: Codex round 4 — re-arm durable interrupt steers when resume moves owners - An arm lives only in the owning replica's runtime plus a transient pub/sub message, while the steer's `preempt` flag is durable on the queue item. A HITL resume landing on a different replica therefore started with an empty armed set and a poll stuck false, so an interrupt the user had already been ACKed for silently waited for an ordinary tool boundary. New `GenerationJobManager.rearmQueuedPreempts` rebuilds the armed set by peeking the durable queue (fenced on the generation) and re-arming every item flagged `preempt`; resume calls it right after claiming. Safe by construction: every item peeked is still queued, so no drained steer can be resurrected. - Capability-refresh failure now logs at error rather than warn, but deliberately does NOT fail the resume — see the reply on that thread. Tests: +2 (rebuild from queue arms only the flagged item and reports the count; a stale generation arms nothing). 124 packages/api steering specs green. * 📡 fix: Codex round 5 — acknowledge only what was actually armed - A cross-replica arm was fire-and-forget: `emitPreempt` logged its own publish failure and `requestPreempt` returned void, so the route answered `preempt: true` even when the owner never armed a poll. The steer still injected at the next tool boundary, but the chip claimed an interrupt that could not happen — and unlike HITL resume, an ordinary running generation had no durable reconciliation to recover it. `emitPreempt` now resolves to the subscriber count and rejects on failure; `requestPreempt` is async and returns whether the arm truly landed (owned locally, or delivered to at least one subscriber). The 202 reports THAT rather than what was asked for, so the chip relabels to ordinary steering exactly as it does for a capability-degraded deployment. Errors are swallowed into `false` — an unarmed interrupt is a downgrade, never a failed steer. - The owner capability is re-read immediately before enqueue rather than reused from the top of the guard ladder. `checkAgentAccess` and file resolution are awaits, so a request can span an entire HITL pause/resume that moves ownership to a replica with different capability and rewrites that very flag. Only paid for by requests that actually asked to interrupt. Tests: +3 (not-armed when the publish reaches nobody; armed when this replica owns the generation; a throwing publish downgrades instead of propagating). 127 packages/api steering specs green. * 🎯 fix: Codex round 6 — real ownership, confirmed disarms, and a CI regression of my own Three review findings plus three CI failures the round-5 commit caused. Review: - Ownership came from `runtimeState`, which a cross-replica `getJob` populates with a FACADE runtime on any replica that merely read the job. Matching `createdAt` therefore proved only "we looked at this job", so a non-owner could arm nothing and report success. Ownership now comes from `ownedJobs`, the actual owner map. - `armPreemptIds` returns how many ids it accepted, and a local arm is only reported as armed when one was. A tombstoned id (its steer drained at an ordinary boundary mid-request) no longer answers `preempt: true` for an interrupt that cannot happen. - The cancel disarm is awaited. A dropped clear is worse than a dropped arm: the owner keeps a level-triggered request for a steer that no longer exists, seals its next chunk and truncates an unrelated answer. The boundary drain's own call stays non-blocking — there the owner is local, so the disarm is already effective and awaiting the informational publish would only delay injection. - Subscriber count is NOT read as proof of owner receipt: the count includes this replica's own facade subscription. A successful publish reports armed, a rejected one does not. Documented rather than papered over — see the acknowledgement-semantics note on the PR. CI regressions from round 5, all mine: - `registerPreemptSubscription` was AWAITED at both runtime-init sites, so job creation blocked on a second Redis channel subscription and hung when that subscribe was slow. Abort is awaited because a missed abort strands a run; a missed preempt only degrades that steer to the next tool boundary, so it now registers without gating createJob. - Two api specs mocked `@librechat/api` without the newly imported `isSteerPreemptSupported`, so the call threw before createJob; and one exact-match assertion needed the new `preemptCapable` metadata field. - My own Redis integration spec asserted arm-before-clear ordering, which two publishes carry no guarantee of — the receiving tombstone exists precisely because of that. Now asserts delivery and payload fidelity, order-independent. 158 packages/api specs, 27 api specs green. * 🧭 fix: Codex round 7 — settle the acknowledgement semantics (Option A) Round 7's second finding is the incoherence I flagged on the PR: the route persisted `preempt: true` on the durable queue item while returning `preempt: false` when delivery could not be confirmed. Those two then disagreed, and `rearmQueuedPreempts` trusts the DURABLE one — so a resumed owner would honour an interrupt the client had explicitly been told degraded to ordinary steering. Rather than patch the disagreement, this settles the meaning: `preempt` in the 202 means "queued as an interrupt request", NOT "a seal is guaranteed". It mirrors `SteerQueueItem.preempt` exactly, so the response, the durable record, and the resume-time re-arm can never disagree. The gates that ARE knowable stay — the owner's recorded capability and a successful enqueue. Everything past that degrades to the documented fallback of injecting at the next tool boundary. A route cannot synchronously know whether another replica will seal: proving it needs a correlated request/response over pub-sub, and even that only proves the owner heard, not that it is still streaming when the arm lands. Four rounds of tightening this boolean each surfaced a narrower case; the sequence does not converge, so the invariant is now "the flag describes the durable decision" and an unconfirmed arm logs a warning instead of rewriting the answer. Also from this round: a failed disarm publish is retried once and its outcome reported. `handleSteerCancel` keeps `removed: true` — the steer really did leave the queue, and saying otherwise would make the client re-show a chip for a steer that can never arrive — and adds `disarmed: false` so the residual risk is visible rather than swallowed. Damage stays bounded regardless: the empty-boundary self-clear disarms the generation after a single seal. Tests: +1 pinning the response/durable-flag invariant. 159 packages/api specs green. * 🧹 fix: Codex round 8 — remove the unverifiable disarm signal Round 8 found the same over-promise on the disarm side that round 7 corrected on the arm side, so this applies the same answer rather than patching around it. The `disarmed: false` field added in round 7 was both unreliable and unused: a resolved publish is not proof the owner heard it (the delivery count includes this replica's own facade subscription), and it was never threaded into `CancelSteerResponse` or read by any client. A signal that claims a certainty the transport cannot provide is worse than no signal — it invites callers to trust it. Removed from the response. The retry stays, because it genuinely reduces the failure rate, and `noteSteersRemoved` still returns whether the publish succeeded FOR LOGGING, now documented explicitly as "published without error", not "the owner disarmed". Disarm is best effort with a bounded, self-healing failure: if the clear is lost the owner seals once, the empty-boundary self-clear disarms the generation, and the turn is persisted `unfinished: true` rather than silently truncated. Tightening that further needs a correlated request/response over pub-sub with a timeout — noted on the PR as the deliberate boundary of this design rather than an oversight. 130 packages/api steering specs green. * 🧽 fix: Codex round 9 — spend snapshot arms on nonempty drains too The round-6 scoping fix only cleared the pre-drain snapshot when the drain came back EMPTY. On a nonempty drain the `finally` cleared just the drained ids, so a stale arm — typically a cancel whose cross-replica clear was lost — survived the boundary. It would then immediately seal the continuation meant to answer the steer that had just been injected, and land on an empty boundary as `preempt_incomplete`: the interrupt appears to work, and the answer to it is truncated. A boundary that runs has spent its seal, so everything armed at snapshot time is spent whether or not it came back from the drain. The `finally` now clears the union of the snapshot and the drained ids. Arms that land AFTER the snapshot are still spared — their queue items are live and uninjected, which is the property round 6 added. Also fixes an api-workspace CI failure of mine: `resume.spec.js` stubs `GenerationJobManager` wholesale, and the round-3/4 resume work added two calls (`updateMetadata`, `rearmQueuedPreempts`) the stub did not define, so 34 specs threw. Stub extended. Tests: +2 (a nonempty drain clears a stale snapshot arm; a nonempty drain spares an arm that landed mid-drain). Counterfactually verified — the stale-arm spec fails against the unfixed drain. 132 packages/api specs, 60 resume specs green. * fix: never let a failed preempt subscription reject into the void registerPreemptSubscription is called detached at both sites, so a rejected Redis SUBSCRIBE became an unhandled rejection — process-fatal under Node's default --unhandled-rejections=throw. The comment already promised this path merely degrades steering; it now does. Swallowed and logged inside the registration rather than at each call site, so a future third caller cannot reintroduce the trap. Losing the channel costs this generation's cross-replica preempts, not the server: same-replica arming is runtime state and still works, and remote arms fall back to the next tool boundary. Verified counterfactually — the new spec surfaces SUBSCRIBE failed as an unhandled rejection against the unfixed registration. * docs: state the real blast radius of a failed preempt subscription LibreChat's own entrypoints install a global unhandledRejection handler that logs and keeps serving, so the escaping rejection this guards was never fatal to this server — only to another consumer of @librechat/api that installs no handler. The fix stands either way; the comment just should not overstate what it prevents. * test: cover the cross-replica preempt hop with two manager instances Every other preempt test runs against a single manager, so the hop that actually carries an interrupt in production had no coverage: the steer POST lands on whichever replica the balancer picks, which is usually not the one generating. Non-owner publishes, owner arms, owner's level-triggered poll flips — none of that was exercised end to end. Two GenerationJobManagerClass instances are a faithful replica pair here. runtimeState and ownedJobs are private instance fields, there is no module-level mutable state between them, and createStreamServices duplicates a dedicated subscriber connection per call, so separate OS processes would exercise the same objects over the same Redis. Both assertions verified counterfactually against real Redis: - Deleting the preemptCapable deserialization in RedisJobStore fails this with 'Expected: true, Received: undefined' — the exact P1 that shipped past every in-memory test and would have made the feature a silent no-op on every Redis deployment. - Dropping the non-owner arm publish fails it with 'Received: false'. * test: remove the fixed sleeps and vacuity from the cross-replica preempt test Codex round 11, both findings, both on the test I added last commit. P2 — the 300ms waits were load-bearing. Redis pub/sub never replays and the owner's SUBSCRIBE is detached, so on a slow CI worker the publish could land before anyone was listening and the test would fail against correct code. Now it republishes until the owner's state converges, which is safe because arms and clears are idempotent set writes keyed by steerId. Side effect: the tests got ~10x faster (85ms/57ms vs 929ms/606ms) since they finish on delivery rather than on a timer. P3 — afterEach destroyed only the transports, leaving each manager alive in its own cleanup-interval closure, still working against a dead transport. Now tracks the managers and awaits destroy(), which disposes the job store and its timer too. Matches how the rest of this file cleans up. Fixing the sleeps exposed a third problem codex did not flag: the stale-arm test could pass vacuously, because an undelivered arm and a fenced one look identical. It now brackets the stale publish between two control arms — the first proves the owner is listening before the stale one is sent, the second proves it has had its chance to arrive. Verified counterfactually against real Redis, and stable over 5 runs: - dropping the preemptCapable deserialization fails with 'Received: undefined' - dropping the non-owner arm publish times out both tests - removing the generation fence fails the stale test with ["control-before", "steer-stale", "control-after"] — which also confirms the bracketing orders as intended rather than by luck * fix: gate interrupt on the OWNER's capability alone, not the route's Codex round 12. The comment above this gate already said 'the OWNER's recorded capability, not this replica's probe' — and then the code ANDed in isSteerPreemptSupported(), which is exactly this replica's probe. The contradiction dates to the original commit; round 6 made the gate owner-scoped and wrote that comment without removing the local conjunct. The route never seals. It enqueues and publishes an arm, neither of which touches the SDK, so during a rolling deploy a steer landing on an un-upgraded replica silently lost its interrupt even though the owner could seal. When the route IS the owner the probe is redundant anyway: the flag it would consult is the one this process wrote at createJob. The real degradation path is unchanged and still tested — an owner that recorded no capability relabels to an ordinary steer. The test that pinned the local probe asserted an impossible same-replica state (capable metadata plus an incapable local SDK, when the metadata is written from that probe); it now pins the mixed-SDK direction instead, and fails with 'Expected: true, Received: false' if the probe is put back. * fix: reconcile arms at handover, and stop holding the 202 on a publish Codex round 13, two of three findings. P2 — rearmQueuedPreempts only ever ADDED. A replica that merely read the job still installs a facade runtime and subscribes, so it can accept an arm and then miss the best-effort clear that follows the drain. HITL resume promotes that facade to owner, the union keeps the orphan, and the first resumed stream seals on a steer no longer in the queue, drains nothing, and truncates the resumed answer as preempt_incomplete. acquireResumedJobOwnership only sets ownedJobs, so nothing else was clearing it. The durable queue is the sole authority at a handover: arms it does not back are now disarmed and tombstoned, so an in-flight publish cannot revive them either. Worth recording that my own independent review raised this and my verifier refuted it. Codex found it separately; two reviewers converging should have outweighed one refutation. P2 — the route awaited the arm publish before answering. The 202 reports capability, not delivery, so the await could not change the response; it only exposed the caller to Redis latency after the queue item was already durable. A client that times out and retries mints a second steer while the first stays queued, injecting the same instruction twice, whereas a lost publish merely takes the tool-boundary fallback. Detached, with both outcomes logged. All three tests verified counterfactually: union-only rearm fails the two new handover specs, and re-awaiting the publish hangs the stalled-publish spec until jest kills it. * fix: snapshot arms before reading the queue at handover Codex round 14 — a regression from my own round-13 fix, and a worse failure than the one it corrected. Round 13 read the durable queue first, then tombstoned any armed id the snapshot did not back. But approvals.resolve reopens steering before reconciliation runs, so another replica can commit a preempt steer and publish its arm while the peek is in flight. That arm is then present locally but absent from a snapshot taken before the steer existed, so a LIVE interrupt the route already acknowledged got dropped — and tombstoned, which blocks the re-arm, making it unrecoverable rather than merely late. Fixed by inverting the two reads rather than by locking or paying a second round trip. A steer is durably enqueued BEFORE its arm is published, so any id in an arms-first snapshot was already queued when it was armed, and the later peek must observe it unless it has since drained — which is exactly the orphan this reconciliation exists to drop. Arms landing after the snapshot are simply not candidates. Also re-checks runtime identity across the await, since the generation can be replaced while the queue read is in flight. New spec injects a steer + arm during the peek and verifies it survives; against the round-13 ordering it fails with Received array: []. * fix: bound the cancel disarm wait and fence enqueue to its generation Codex round 15. P2 — the cancel awaited its disarm publish unbounded. ioredis queues commands during an outage rather than rejecting, so that await could hang for the length of the outage with the steer ALREADY durably cancelled; a client that gives up then treats the cancel as failed and restores a chip for a steer that can never produce an applied event. Every successful cancel publishes, so ordinary steers were exposed too, not only preemptive ones. Now bounded at 1s, with the publish continuing behind it — its retry and logging are unchanged, it is just no longer in front of the response. This is the sibling of round 13's arm-publish finding; I fixed one path and left this one. P3 — enqueue was not fenced to the generation the capability decision was made against. The access checks, file resolution and owner re-read are all awaits, so the run can be replaced before the enqueue: the item then lands on the REPLACEMENT queue while the durable preempt flag and the arm still name the previous epoch, the arm is fenced out at the owner, and the 202 promises an interrupt that cannot happen. enqueueSteer now takes an expected generation, mirroring drain/peek, and the Redis path enforces it inside STEER_ENQUEUE_LUA so the check is atomic with the push rather than racing it. All three new specs verified counterfactually, including the Lua guard against real Redis (removing it returns 1 where -1 is required). * fix: fence the steer to its authorized generation, bound resume setup, keep preempt when Redis parks Codex round 16, all three findings. P2 — round 15 fenced the enqueue to owner.createdAt, the RE-READ job. Every guard above it (ownership, tenant, paused-state, agent ACL) ran against the job read at the top, so if the run was replaced during those awaits the fence happily accepted the steer into a generation the request was never authorized against, carrying the wrong agent's metadata. Now rejects on any mismatch between the validated job and the re-read. P2 — resume awaited its steering bookkeeping unbounded, after approvals.resolve had consumed the action and flipped the job to running, and outside the resume lifecycle's own try/finally. .catch does not fire on a promise that never settles, which is what ioredis produces during an outage, so the client times out, its retry gets a 409 for a spent action, and no cleanup runs. Bounded at 1s with the writes finishing in the background. P3 — Redis parks leftover steers inside its terminal-transition Lua, which projects item fields one by one, so preempt was silently dropped and a steer recovered from /chat/status lost its interrupting label. Added to both projections. All three verified counterfactually, two against real Redis. Worth recording that my first version of the generation-mismatch test was VACUOUS — it faked a createdAt matching no live job, so the round-15 enqueue fence rejected it for the wrong reason and the test passed with the guard removed. Rewritten to replace the run for real; it now fails with 'Expected 404, Received 202'. |