Skip to content

Conversation

@gagik
Copy link
Collaborator

@gagik gagik commented Nov 24, 2025

This adds header and query param-based overrides.

@gagik gagik requested a review from a team as a code owner November 24, 2025 14:15
Copilot AI review requested due to automatic review settings November 24, 2025 14:15
protected async setupServer(): Promise<Server> {
protected async setupServer(request?: RequestContext): Promise<Server> {
// Apply config overrides from request context (headers and query parameters)
let userConfig = applyConfigOverrides({ baseConfig: this.userConfig, request });
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we make this config override opt-in?
Can it be problematic that people who update the MCP server don't release that others can override some other settings now?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah the override should be opt-in using the --allowRequestOverrides flag. Just realised that I did not write this info in the ticket but its further explained in Goal#3 in the scope.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds the ability to override configuration parameters via HTTP headers and query parameters when using the HTTP transport. Overrides are applied automatically from request context before the createSessionConfig hook runs, with query parameters taking precedence over headers. Each config field defines its override behavior (not-allowed, override, merge, or custom logic).

Key changes:

  • New config override system with three behaviors: "not-allowed" (security-sensitive fields), "override" (simple replacement), and "merge" (arrays)
  • Conditional overrides for security flags (readOnly, indexCheck, disableEmbeddingsValidation) using custom logic
  • Request context (headers/query) passed through transport layer to config application

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/common/config/configOverrides.ts New module implementing config override extraction and application logic
src/common/config/configUtils.ts Added override behavior types, parseBoolean preprocessor, and oneWayOverride helper
src/common/config/userConfig.ts Registered override behaviors for all config fields and added boolean preprocessors
src/transports/base.ts Modified setupServer to accept RequestContext and apply overrides before createSessionConfig
src/transports/streamableHttp.ts Extracts headers/query into RequestContext and passes to setupServer
tests/unit/common/config/configOverrides.test.ts Comprehensive unit tests for override behaviors and edge cases
tests/integration/transports/configOverrides.test.ts Integration tests verifying overrides work end-to-end via HTTP
tests/integration/transports/createSessionConfig.test.ts Refactored to use helpers and verify createSessionConfig receives request context
tests/integration/server.test.ts Updated test to check for insert-many instead of insert-one
package.json Added --run flag to test script

Co-authored-by: Copilot <[email protected]>
@coveralls
Copy link
Collaborator

coveralls commented Nov 24, 2025

Pull Request Test Coverage Report for Build 19703979104

Details

  • 242 of 251 (96.41%) changed or added relevant lines in 7 files are covered.
  • 36 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.2%) to 80.415%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/lib.ts 0 1 0.0%
src/common/config/configUtils.ts 48 50 96.0%
src/common/config/configOverrides.ts 112 118 94.92%
Files with Coverage Reduction New Missed Lines %
src/tools/mongodb/mongodbTool.ts 5 84.09%
src/common/session.ts 6 85.08%
src/tools/atlas/connect/connectCluster.ts 25 71.56%
Totals Coverage Status
Change from base Build 19701626410: 0.2%
Covered Lines: 6665
Relevant Lines: 8203

💛 - Coveralls

protected async setupServer(): Promise<Server> {
protected async setupServer(request?: RequestContext): Promise<Server> {
// Apply config overrides from request context (headers and query parameters)
let userConfig = applyConfigOverrides({ baseConfig: this.userConfig, request });
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah the override should be opt-in using the --allowRequestOverrides flag. Just realised that I did not write this info in the ticket but its further explained in Goal#3 in the scope.

Copy link
Collaborator

@nirinchev nirinchev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't looked at the tests yet, but I figured it's better to leave the comments I have so far so you don't wait for the full review.

* Preprocessor for boolean values that handles string "true"/"false" correctly.
* Zod's coerce.boolean() treats any non-empty string as true, which is not what we want.
*/
export function parseBoolean(val: unknown): unknown {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function appears to treat any non-empty string as true too (except false and 0) - is this intentional?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, this was a good balance between making sure old behaviors will work similar way (so if someone has --readonly=yes before, it'd still be true and not suddenly become false. while also making sure intentional setting of the value works too.
This helps us have the same logic for CLI and request parsing/validation. Though open to adjusting if there's concerns here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should just error out if the string value is different from true/false/1/0? Because based on the same logic, if we have --readonly=no, that would now make the server readonly.

Copy link
Collaborator Author

@gagik gagik Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm open to this but it does mean this is a breaking change.
in latest v1.2.0 --readOnly no would make the server readOnly
--readOnly false would make the server write-able

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, the question is if this is intentional - I would say this is more of a bugfix than a breaking change. Additionally, if we throw, it won't be a silent breaking change - we'll give users feedback about what to fix, which I think is a reasonable compromise.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay apparently this preprocessing only really applies to these headers.
everything else is configured by yargs-parser... so it's about changing settings there

Copy link
Collaborator Author

@gagik gagik Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed logic to be:

  • if "true" or "false" -> parse into boolean
  • any other value error with invalid boolean

note: this doesn't affect our CLI passing, this only affects query params. CLI stuff is handled by yargs-parser

.default(300_000)
.describe("Time in milliseconds after which an export is considered expired and eligible for cleanup."),
.describe("Time in milliseconds after which an export is considered expired and eligible for cleanup.")
.register(configRegistry, { overrideBehavior: "override" }),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds sketchy - can I setup my exports to timeout after a year, then flood the server storage with exports?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. But what if the client wanted a smaller timeout of their export? We can cap the overridable max timeout to the server configured timeout? But yea if it becomes too complex, I am fine leaving it as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setting this to onlyLowerThanBaseValueOverride

.describe("When set to true, disables validation of embeddings dimensions."),
.describe("When set to true, disables validation of embeddings dimensions.")
.register(configRegistry, {
overrideBehavior: oneWayOverride(false),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonder why this is oneWayOverride?

Copy link
Collaborator Author

@gagik gagik Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if validation is enabled (i.e. disableValidation is false) then we would not want a client to disable validation and let the MCP server insert arbitrary data in a vector search field (which apparently breaks vector search)

By the way, I find the double negative to be confusing so it'd be great to rename this in general

.default([])
.describe("An array of preview features that are enabled."),
.describe("An array of preview features that are enabled.")
.register(configRegistry, { overrideBehavior: "merge" }),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we merging these arrays?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just following the scope document. Agree some can be adjusted cc: @himanshusinghs

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is based on understanding that individual clients might want to enable specific preview features for their session.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should either be override (so you can remove features if needed) or not-allowed (so no experimental features can be enabled)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed this to be only subset of base value (so you can disable preview features but not add any new ones)

@gagik gagik force-pushed the gagik/query-header-override branch from baf9bbe to d2731d8 Compare November 25, 2025 18:11
"generate:arguments": "tsx scripts/generateArguments.ts",
"pretest": "pnpm run build",
"test": "vitest --project eslint-rules --project unit-and-integration --coverage",
"test": "vitest --project eslint-rules --project unit-and-integration --coverage --run",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

drive-by: makes it easier to for LLMs to run this... we can have test:watch if needed

@gagik gagik merged commit 3a21cd8 into main Nov 26, 2025
31 of 39 checks passed
@gagik gagik deleted the gagik/query-header-override branch November 26, 2025 15:24
Copy link
Collaborator

@himanshusinghs himanshusinghs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought I submitted my review but I forgot to click the button. Overall its perfect 🚀 I left only one small thing to consider.

throw new Error(`Invalid config key: ${key}`);
}

return fieldSchema.safeParse(value).data;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gagik the safeparse would silently insert undefined when value is incorrect here

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that seems fine right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, right we could error in way which is more useful

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea I think we should error out instead because otherwise we might be overriding with potentially wrong values.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants