[pull] main from danny-avila:main#108
Merged
pull[bot] merged 7 commits intoinnFactory:mainfrom Apr 13, 2026
Merged
Conversation
… Commands (#12623) Three related changes that tighten the GitNexus CI/CD loop. Serialized deploys - Previous concurrency group was keyed by head ref with cancel-in-progress, which let deploys targeting different refs (e.g. main push + PR command) run in parallel. That's a data race: the prune-stale-indexes step computes active_names up front, so deploy A rsyncing /opt/gitnexus/indexes/LibreChat-pr-12580 can collide with deploy B pruning the same folder based on a pre-rsync view of the active set. - Collapse to a single global group gitnexus-deploy with cancel-in-progress: false. All deploys queue behind one another. A rsync/docker-compose restart is never killed mid-operation. The 20-minute job timeout bounds queue depth. PR completion feedback - Add a "index complete" comment step in gitnexus-index.yml that fires only when inputs.pr_number is set (i.e. the run came via the /gitnexus command). Posts success or failure with a link to the run and whether embeddings were generated. - Add a "deploy complete" comment step in gitnexus-deploy.yml that handles both trigger paths: workflow_run from a native PR auto-index (PR number recovered from the matrix entry whose runId matches the trigger run), and workflow_dispatch from the index workflow's bot- fallback path (PR number passed through as a new inputs.pr_number). - Plumb inputs.pr_number through the bot-fallback dispatch in gitnexus-index.yml so the deploy workflow knows where to comment for command-triggered runs. - Only comments on the PR that asked for the index, never broadcasts. Workflow rename - Drop the "DigitalOcean" suffix from the deploy workflow's display name and filename. The platform is still DO (.do/gitnexus/ still holds the compose + caddy config) but the workflow itself is platform-agnostic in form and the suffix was visual noise. - File renamed gitnexus-deploy-do.yml -> gitnexus-deploy.yml. - Concurrency group and all cross-references updated in lock-step. - permissions at deploy job level now includes pull-requests: write so the completion comment can post.
* 🔬 fix: Scope Web Search Results to Own Turn in WebSearch Component Fix duplicated search results when multiple web_search tool calls occur in a single response. Each WebSearch instance now displays only its own turn's results instead of aggregating all turns. * test: Add WebSearch turn-scoping tests Cover the regression-prone turn isolation logic: verify each WebSearch instance renders only its own turn's sources when sharing a SearchContext, test the attachments-first priority path and the searchResults fallback, and assert component states (cancelled, error, streaming, complete). * fix: Use getAllByText for duplicate sr-only + visible text in tests WebSearch renders status text in both an sr-only aria-live region and a visible span, causing getByText to fail with multiple matches. * refactor: Make ownTurn a string to avoid repeated conversions
* feat: implement optimized Entra group sync with auto-creation
## Changes
### MUST FIX (Critical Issues) - RESOLVED
1. **BUG FIX: Prevent unintended user removal from existing groups**
- ISSUE: db.syncUserEntraGroups() was called with only missing groups, causing removal
from all existing Entra groups (full bidirectional sync behavior)
- SOLUTION: Replaced with db.upsertGroupByExternalId() for each missing group followed
by single bulkUpdateGroups() to add memberships (race-safe, idempotent)
- BENEFIT: User memberships correctly maintained for mix of existing + new groups
2. **JSDoc @throws contradiction**
- ISSUE: JSDoc declared function throws, but implementation catches all errors
- SOLUTION: Removed @throws from JSDoc - function is best-effort
- BENEFIT: Prevents unnecessary try/catch in caller code
3. **Missing test for group creation flow**
- ISSUE: Auto-creating missing Entra groups had no test coverage
- SOLUTION: Added regression test for mix of existing + new groups scenario
- BENEFIT: Prevents future regressions on critical path
### SHOULD FIX (Important Improvements) - RESOLVED
4. **E11000 race condition handling**
- SOLUTION: Upserts are idempotent and race-safe by design
- BENEFIT: Concurrent logins no longer race each other
5. **Direct Mongoose access instead of db layer**
- SOLUTION: Added findGroupsByExternalIds() helper to userGroup.ts
- BENEFIT: Centralized data access, easier to add tenant scoping
6. **Serial DB round-trips on login path**
- ISSUE: 40+ queries for user with 20 new groups
- SOLUTION: Promise.all() for parallel upserts + single bulkUpdate
- BENEFIT: ~10x performance improvement
7. **Graph API 429/503 throttling unhandled**
- SOLUTION: Retry logic with exponential backoff (1s, 2s delays)
- BENEFIT: Temporary API issues no longer cause permanent membership loss
8. **Sequential batch requests slow**
- ISSUE: 200 groups = 10 batches × 200ms = ~2s sequential
- SOLUTION: Promise.all() with concurrency limit (5 parallel batches)
- BENEFIT: ~400ms total time
## Minor Fixes
- Removed dead code check
- PII removal: user._id instead of user.email in logs
- ES6 shorthand fixes
- Style consistency (blank lines)
- Projection optimization
## Verification
✅ npm run build - success
✅ npm run test:api - 61/61 passing (+ new regression test)
✅ npm run lint - no errors
✅ All feedback from danny-avila resolved
* docs: better JSDoc for the syncUserEntraGroupMemberships method
---------
Co-authored-by: Airam Hernández Hernández <[email protected]>
…ain Collision (#12594) * 🐛 fix: resolve Action tools by exact tool name to prevent multi-action collision When two OpenAPI Actions on the same Agent share a hostname, the second action's entry overwrote the first in the encoded-domain Map and one action's tools silently disappeared from the LLM payload. The buggy resolution loop also used substring matching, which caused similar shadowing for any encoded-domain prefix overlap. This change builds a Map keyed on the full tool name (`<operationId>_action_<encoded-domain>`) directly, mirroring the exact lookup pattern that getActionToolDefinitions already uses. Each function in an action's spec gets its own slot, so two actions sharing a hostname no longer collide. Both the new and the legacy domain encodings are registered for each function so agents whose stored tool names predate the current encoding still resolve. Applied at all three call sites that had the buggy pattern: - processRequiredActions (assistants/threads path) - loadAgentTools (agent build path) - loadActionToolsForExecution (agent execution path) Adds three regression tests covering both ordering directions and the execution path. Tests fail without the fix and pass with it. * 🐛 fix: Normalize action tool name at lookup + cover assistants path Follow-up to the multi-action domain collision fix. Addresses PR #12594 review feedback: **Must-fix #1 — short-hostname lookup mismatch.** The toolToAction map is keyed on the `_`-collapsed domain, but `agent.tools` and `currentAction.tool` persist the raw `domainParser(..., true)` output, which for hostnames ≤ ENCODED_DOMAIN_LENGTH is a `---`-separated string (e.g. `medium---com`). Exact-match `Map.get()` missed those keys and silently dropped the tool. Fix: normalize every incoming tool name through a new `normalizeActionToolName` helper before the lookup in `loadAgentTools`, `processRequiredActions`, and `loadActionToolsForExecution`. **Must-fix #2 — assistants path coverage.** `processRequiredActions` received the same structural rewrite but had zero tests. Added a regression test under `multi-action domain collision regression` that drives two shared-hostname actions through the assistants path and asserts each tool reaches its own request builder. **Must-fix #3 — legacy encoding branch coverage.** The `if (legacyNormalized !== normalizedDomain)` registration was never exercised by any test. Added a test where `agent.tools` stores the legacy-format name and asserts it still resolves. **Should-fix #4 — DRY the registration loop.** Extracted `registerActionTools({ toolToAction, functionSignatures, normalizedDomain, legacyNormalized, makeEntry })`. All three call sites now share the same key-building logic; the key template lives in one place. **Should-fix #5 — remove stale optional chaining.** In `loadActionToolsForExecution`, `functionSignature?.description ?? ''` became `functionSignature.description` — `sig` is always defined by the iterator, matching the style of `loadAgentTools`. **Should-fix #6 — drop unreachable `!requestBuilder` guard.** Entries in `processRequiredActions` are now pre-built with `requestBuilder: requestBuilders[sig.name]`, which `openapiToFunction` always produces alongside the signature, so the guard is dead. **Should-fix #7 — unwrap `actionSetsData`.** It now holds a bare `Map` instead of `{ toolToAction }`; the sentinel `!actionSetsData` check still works because `new Map()` is truthy. Also added a short-hostname regression test (`loadAgentTools resolves raw ---separated tool names`) that reproduces Must-fix #1: it fails against the previous commit (0 create calls) and passes with the normalization in place. 41 tests, all passing. The 3 new regression tests are under `multi-action domain collision regression` and cover the assistants path, the legacy encoding branch, and the short-hostname lookup path. * 🐛 fix: Tighten registerActionTools key handling and assistants test Follow-up to d643444 addressing the second review pass on PR #12594. **ESLint** — Two prettier errors in the spec file (multi-line arrow function bodies that should fit on one line). Auto-fixed. **[MINOR] operationId containing `---` → key mismatch.** The lookup path collapsed every `actionDomainSeparator` sequence in the full tool name, but the registration path passed `sig.name` through unchanged. A `---` that survived into an operationId would shift the underscore boundary at lookup and miss its own key. Fix in `registerActionTools`: normalize `sig.name` with the same helper so registration and lookup always agree on the canonical form. `sanitizeOperationId` strips the characters that produce `---` in practice, so this is theoretical hardening, not a fix for a known reproducer. **[MINOR] Same-operationId + same-hostname silent overwrite.** Two actions sharing both an operationId and a hostname still produced a silent `Map.set()` overwrite (the new key is identical, so neither the operationId nor the domain disambiguates). Added a `setKey` helper inside `registerActionTools` that logs a `[Actions] operationId collision: ...` warning whenever a key is already present, naming the overwriting action_id. The silent-overwrite mode from the original bug cannot reappear under a different disguise without surfacing in the logs. **[NIT] processRequiredActions test simulated a runtime crash.** `mockCreateActionTool` returned a tool with `_call: jest.fn()`, which resolves to `undefined`. `processRequiredActions` chains `.then(handleToolOutput).catch(handleToolError)` directly onto that return, so `undefined.then(...)` threw synchronously and the outer try/catch funneled the error into `handleToolError`. Creation count assertions still passed because `createActionTool` runs before the crash, but the test was silently exercising the failure path. Updated the global mock to `_call: jest.fn().mockResolvedValue('{"status":"ok"}')` so the success path runs end-to-end. The assistants regression test now executes in ~5ms instead of ~90ms, which corroborates that it's no longer hitting the synchronous throw. **[NIT] Duplicated rationale comments.** All three call sites carried multi-line comment blocks restating why we key on the full tool name. That rationale now lives canonically in `registerActionTools`'s JSDoc; the inline blocks collapsed to `// See registerActionTools for the key-shape rationale.` Net -22 lines of comments. 41/41 tests still pass; lint is clean. * 🐛 fix: Scope tool-name normalization to the encoded-domain suffix Follow-up to f22228e addressing the Codex P1 on PR #12594. **Regression.** The previous commit normalized the entire tool name (`normalizeActionToolName(sig.name)` at registration, full-name `.replace()` at lookup) to handle operationIds that theoretically contained `---`. But `openapiToFunction` uses user-supplied operationIds verbatim and the fallback `sanitizeOperationId` only strips characters outside `[a-zA-Z0-9_-]`, so specs can legitimately produce operationIds like `get_foo---bar` and `get_foo_bar` side by side. Collapsing `---` to `_` across the entire key merged those two into a single map slot — one silently overwrote the other, and both tool requests routed to the surviving entry's request builder. **Fix.** Limit normalization to the encoded-domain portion of the full tool name, i.e. the substring after the last `actionDelimiter`. The operationId half is left verbatim, so hyphens-vs-underscores remain disambiguating. The short-hostname bug (Must-fix #1 from the original review) is still covered because the `---` → `_` collapse still happens on the domain suffix where it matters: ```js const normalizeActionToolName = (toolName) => { const delimiterIndex = toolName.lastIndexOf(actionDelimiter); if (delimiterIndex === -1) return toolName; const prefixEnd = delimiterIndex + actionDelimiter.length; const encodedDomain = toolName.slice(prefixEnd); return toolName.slice(0, prefixEnd) + encodedDomain.replace(domainSeparatorRegex, '_'); }; ``` `registerActionTools` reverts to `sig.name` verbatim — no more `normalizeActionToolName(sig.name)` ahead of the key build. **Regression test.** Added `loadAgentTools distinguishes operationIds that differ only by ---` vs `_`` under `multi-action domain collision regression`. It loads two actions sharing a hostname whose operationIds are `get_foo---bar` and `get_foo_bar` respectively, each pointing at a different path (`/foo-bar`, `/foo_bar`), and asserts that each tool resolves to its own request builder. Verified the test fails against f22228e (`hyphenTool` resolves to `/foo_bar` — the sibling's builder) and passes with this commit. 42/42 tests pass; lint clean.
* 🗂️ feat: Sidebar Icon Toggle & New Chat History Switch Add collapse-on-active-click for sidebar icons (VSCode-style) and optionally switch to Chat History panel when creating a new chat. * fix: Address review findings — extract DEFAULT_PANEL constant, add tests Export DEFAULT_PANEL from ActivePanelContext and use it in ExpandedPanel instead of hardcoding 'conversations'. Add ExpandedPanel tests covering NavIconButton collapse toggle and NewChatButton panel switch behaviors. * fix: Address review — prop-drill setActive, test disabled setting, strengthen assertions Pass setActive as a prop to NewChatButton instead of subscribing to ActivePanelContext, avoiding wasted re-renders on every panel switch. Add negative-path test for switchToHistory=false. Add positive panel assertions to inactive-icon click tests. Fix import order.
…th Token Refresh (#12643) * fix: clear stale client registration on invalid_client during token refresh When a token refresh fails with `invalid_client`, the stored DCR client registration is no longer valid on the authorization server. The existing error handler only checked for `unauthorized_client` and returned null, leaving the stale client_id cached in the database permanently. Every subsequent token refresh attempt would fail with the same error. Now when `invalid_client` is detected during refresh: 1. The stale client registration is deleted from the database 2. A `ReauthenticationRequiredError` is thrown to trigger a fresh OAuth flow with new dynamic client registration Also passes `deleteTokens` from MCPConnectionFactory to getTokens() so the cleanup has access to the token deletion method. * fix: address review findings for stale client cleanup on token refresh - Delete stale refresh token alongside client registration on invalid_client (Finding 1) - Add tests for all new code paths: cleanup, warning, case-insensitivity, cleanup failure (Finding 2) - Detect all vendor-specific client rejection patterns (client_id mismatch, client not found, unknown client) with case-insensitive matching (Finding 3) - Use else-if for mutually exclusive error branches (Finding 4) - Log warning when deleteTokens is not available on client rejection (Finding 6) - Fix log message to say "attempting to clear" before async cleanup (Finding 7) - Extract isClientRejectionMessage to shared utility, refactor MCPConnectionFactory.isClientRejection to use it * fix: address followup review findings - Extract isInvalidClientMessage (4 stale-client patterns) from isClientRejectionMessage to eliminate pattern duplication between utils.ts and tokens.ts (Finding 1) - Remove redundant staleIdentifier variable, reuse identifier already in scope (Finding 2) - Separate await from .then() on Promise.allSettled for readability (Finding 3) - Add dedicated unit tests for isInvalidClientMessage and isClientRejectionMessage (Finding 4) - Assert error message content in primary invalid_client test (Finding 5) - Add JSDoc on deleteTokens in GetTokensParams (Finding 6) --------- Co-authored-by: Mani Japra <[email protected]>
* fix: preserve selected artifact when clicking artifact button * fix: preserve artifact selection on click and during streaming * fix: remove broken streaming guard, add JSDoc and test - Remove `userHasManualSelection` guard from effect #3: it cannot distinguish manual clicks from system auto-selection, blocking auto-advancement to new artifacts during streaming. - Add JSDoc on `currentArtifactIdRef` explaining why it must not be added to effect deps (toggle-close regression). - Add test verifying auto-advancement during streaming. * style: use standard multi-line JSDoc format for ref comment --------- Co-authored-by: Danny Avila <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )