Add branded core and Alliance ServerConfig types#179
Merged
wpak-ai merged 5 commits intoJun 25, 2026
Conversation
📝 WalkthroughWalkthroughAdds branded core and Alliance config types, threads them through server context and setup APIs, updates tests for type misuse, and switches Alliance examples and docs to the Alliance resolver entrypoint. ChangesConfig Type Branding and Enforcement
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #179 +/- ##
=======================================
Coverage ? 87.88%
=======================================
Files ? 41
Lines ? 1758
Branches ? 559
=======================================
Hits ? 1545
Misses ? 213
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/alliance/setup.ts`:
- Around line 74-84: The setupAllianceServer path currently lets an incoming
context with no Alliance config fall back to core defaults when getConfig() is
later called. Update the setup logic in setupAllianceServer and its
ServerContext usage so it can distinguish “config already set” from “unset”
using a non-initializing check like hasConfig/getConfigIfSet, or reject
unconfigured contexts outright, and ensure Alliance always installs
resolveAllianceConfig({}) instead of materializing resolveConfig({}) from core
defaults.
- Around line 76-79: The context mutation in setupAllianceServer is bypassing
the injected-client validation in setupCoreServer, so restore the guard before
calling ServerContext.setConfig() and delegation. In setupAllianceServer, check
the combination of opts.config, resolvedCtx, and any injected client on the
context before mutating it, and throw the same conflict path that
setupCoreServer uses if both config and an injected client are present. Keep the
existing setupCoreServer call unchanged after the validation so
ServerContext.setConfig() does not silently clear the client.
In `@src/core/server/server-context.ts`:
- Around line 74-79: `setupCoreServer` is still accepting an unparameterized
`ServerContext`, which lets Alliance-specific config flow into the core path
through `createServer` and `ServerContext.setConfig`. Make
`SetupCoreServerOptions.context` use the core-specific generic/context type in
`server-context.ts`, or move the Alliance-to-core conversion behind an internal
helper so `setupCoreServer` stays core-only. Use the existing `setupCoreServer`,
`ServerContext`, and `setConfig` symbols to update the type boundary without
exposing `AnyServerConfig` to the core API.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6ec97d17-bec5-49e2-9cd3-1e56259c6a5d
📒 Files selected for processing (17)
.npmignoreexamples/alliance/custom-url-generator.tsexamples/alliance/guided-query-demo.tsexamples/alliance/suggest-flow-demo.tssrc/__tests__/config-branding.compile-test.tssrc/alliance/config.tssrc/alliance/index.tssrc/alliance/setup.tssrc/core/config.tssrc/core/index.tssrc/core/server/config-context.tssrc/core/server/server-context.tssrc/core/server/tools/test-helpers.tssrc/core/setup-guards.test.tssrc/core/setup.tssrc/index.tssrc/types.ts
added 2 commits
June 25, 2026 02:44
- In the context branch, when no opts.config and !ctx.hasConfig(), call resolveAllianceConfig({}) before tool registration
- Added test: unconfigured context + PINECONE_API_KEY env → Alliance gate defaults applied
- ServerContext<T> generic + CoreServerContext = ServerContext<CoreServerConfig>
- SetupCoreServerOptions.context is now CoreServerContext only
- createServer / createIsolatedContext overloaded by config brand
- Alliance uses internal setupCoreServerOnContext(ctx) (not exported from the public barrel) instead of setupCoreServer({ context })
…nd exported it from core/alliance barrels - Narrowed SetupAllianceServerOptions.context to AllianceServerContext - Extended config-branding.compile-test.ts with context-path @ts-expect-error cases (both directions)
Collaborator
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/server/server-context.ts`:
- Around line 145-150: The fallback in ServerContext.getConfig() is using
resolveConfig({}) for a lazily constructed Alliance context, which returns core
defaults instead of Alliance defaults. Update getConfig() so it does not
silently resolve the core config when the context is unconfigured: either make
the lazy fallback Alliance-aware by using the Alliance config path/type, or
explicitly reject direct ServerContext<AllianceServerConfig> construction
without prior seeding. Use the existing ServerContext and setupAllianceServer
flow as the reference points when adjusting the fallback behavior.
In `@src/core/setup.ts`:
- Around line 167-174: `setupCoreServer` still lets branded config objects slip
through the `SetupCoreServerOptions` branch because the options type is
structurally compatible with `AllianceServerConfig`, and
`normalizeSetupCoreServerArgs`/`isServerConfig` only key off `apiKey` and
`indexName`. Make the options path incompatible with config-shaped objects (for
example by adding a discriminant or required non-config field) or split
`setupCoreServer` into separate entry points so branded configs cannot be
accepted through the legacy/options overload.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bb050aba-629c-40b6-adca-8f4d053dbe4f
📒 Files selected for processing (21)
.npmignoreCHANGELOG.mddocs/MIGRATION.mdexamples/alliance/custom-url-generator.tsexamples/alliance/guided-query-demo.tsexamples/alliance/suggest-flow-demo.tssrc/__tests__/config-branding.compile-test.tssrc/alliance/config.tssrc/alliance/index.tssrc/alliance/setup.tssrc/core/config.tssrc/core/index.tssrc/core/server/config-context.tssrc/core/server/server-context.legacy-facade.test.tssrc/core/server/server-context.tssrc/core/server/tools/test-helpers.tssrc/core/setup-guards.test.tssrc/core/setup-multi-instance.test.tssrc/core/setup.tssrc/index.tssrc/types.ts
Collaborator
|
Please check the PR title again. |
jonathanMLDev
changed the title
Updated to #unreleased-branded-serverconfig-types (matches GitHub’s anchor for that heading, same pattern as other unreleased sections). setup.ts — when opts.context is passed without opts.config, validate getConfigIfSet() via assertCoreServerConfig. alliance/setup.ts — symmetric check with assertAllianceServerConfig when the context already has config.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/config.ts (1)
88-97: 🩺 Stability & Availability | 🔵 TrivialDefine the lineage marker as a non-enumerable property to avoid leaking through spreads.
brandCoreConfigandbrandAllianceConfigattachSERVER_CONFIG_LINEAGEvia direct assignment, creating an enumerable own symbol property. The JavaScript spread operator ({...obj}) andObject.assigncopy enumerable symbol-keyed properties. Downstream code that clones a resolved config before passing it to setup guards could inadvertently retain the lineage marker, potentially skipping resolution logic or bypassing type distinctions at runtime. Defining the property as non-enumerable prevents propagation while maintaining runtime access.♻️ Proposed change
/** Attach the core brand after {`@link` resolveConfig} resolution. */ export function brandCoreConfig(config: ServerConfigBase): CoreServerConfig { - (config as ServerConfigLineageHost)[SERVER_CONFIG_LINEAGE] = 'core'; + Object.defineProperty(config, SERVER_CONFIG_LINEAGE, { + value: 'core', + enumerable: false, + configurable: true, + writable: true, + }); return config as CoreServerConfig; } /** Attach the Alliance brand after {`@link` resolveAllianceConfig} resolution. */ export function brandAllianceConfig(config: ServerConfigBase): AllianceServerConfig { - (config as ServerConfigLineageHost)[SERVER_CONFIG_LINEAGE] = 'alliance'; + Object.defineProperty(config, SERVER_CONFIG_LINEAGE, { + value: 'alliance', + enumerable: false, + configurable: true, + writable: true, + }); return config as AllianceServerConfig; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/config.ts` around lines 88 - 97, The lineage marker on the config branding helpers is currently added as an enumerable symbol property, which can leak through object spreads and Object.assign. Update brandCoreConfig and brandAllianceConfig in config.ts to attach SERVER_CONFIG_LINEAGE via a non-enumerable property definition while preserving the existing returned CoreServerConfig and AllianceServerConfig types. Keep the change localized to these branding helpers so downstream clones do not retain the marker unexpectedly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/core/config.ts`:
- Around line 88-97: The lineage marker on the config branding helpers is
currently added as an enumerable symbol property, which can leak through object
spreads and Object.assign. Update brandCoreConfig and brandAllianceConfig in
config.ts to attach SERVER_CONFIG_LINEAGE via a non-enumerable property
definition while preserving the existing returned CoreServerConfig and
AllianceServerConfig types. Keep the change localized to these branding helpers
so downstream clones do not retain the marker unexpectedly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eecbb98e-0f98-49ec-8cb0-556dddb55052
📒 Files selected for processing (7)
CHANGELOG.mdsrc/alliance/setup.tssrc/core/config.tssrc/core/index.tssrc/core/server/server-context.tssrc/core/setup-guards.test.tssrc/core/setup.ts
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (2)
- src/alliance/setup.ts
- src/core/setup.ts
AuraMindNest
approved these changes
Jun 25, 2026
wpak-ai
approved these changes
Jun 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ServerConfigtype intoServerConfigBaseplus brandedCoreServerConfigandAllianceServerConfigusingunique symbolbrandsresolveConfig()/setupCoreServer()to core config andresolveAllianceConfig()/setupAllianceServer()to Alliance config so passing Alliance config into core setup is a compile-time errorsetupAllianceServerto hand config to core setup via context only (setConfig/createServer), avoiding branded-type conflicts at the delegation boundarysrc/__tests__/config-branding.compile-test.tswith@ts-expect-errorguards; excludedist/__tests__/from npm via.npmignoreresolveAllianceConfigMotivation
resolveConfig()andresolveAllianceConfig()returned structurally identical configs with different defaults (suggest-flow gate, index, rerank). Passing an Alliance config tosetupCoreServer()compiled cleanly but changed runtime behavior silently.Test plan
npm run typecheck(includes compile-time branding misuse test)npm run test(294 tests)npm run ciRelated issue
Summary by CodeRabbit