Add OIDC authentication, fix tests, and update PyPI installation docs

[jonpspri]

Summary

This PR adds OIDC authentication support for HTTP mode deployments, fixes a skipped test in the discovery server, and updates all installation documentation to reflect PyPI as the primary installation source.

Changes

1. OIDC Authentication (HTTP Mode)

• Added OidcSettings class with DATABEAK_OIDC_* environment variables
• Integrated OIDCProxy from FastMCP for secure HTTP deployments
• Authentication only applies to HTTP transport mode (not stdio/MCP clients)
• Validates that all four OIDC variables are set; logs error if incomplete

Environment Variables:

• DATABEAK_OIDC_CONFIG_URL - OIDC discovery configuration URL
• DATABEAK_OIDC_CLIENT_ID - OAuth2 client ID
• DATABEAK_OIDC_CLIENT_SECRET - OAuth2 client secret
• DATABEAK_OIDC_BASE_URL - Application base URL for OAuth2

2. Fixed Skipped Test

• Fixed test_no_data_loaded in test_discovery_server_coverage.py:624
• Replaced mocking approach with real session management
• Now properly tests NoDataLoadedError exception handling
• Removed unused imports (Mock, patch, ToolError, databeak)

3. Documentation Updates

• Updated README.md and docs/installation.md for PyPI installation
• Simplified all client configurations from uvx --from git+... to uvx databeak
• Added OIDC authentication documentation for HTTP mode deployments
• Clarified OIDC is only for HTTP transport, not stdio mode
• Updated Python version requirement to 3.12+
• Fixed all example commands to use PyPI

Test Plan

• All tests pass: uv run pytest tests/unit/servers/test_discovery_server_coverage.py
• Fixed test now properly validates no-data error handling
• Code quality checks pass (ruff, mypy)
• Markdown formatting applied

Breaking Changes

None - this is additive functionality.

Notes

• OIDC authentication is optional and only activates when all four environment variables are set
• Stdio mode (default for MCP clients) is unaffected by OIDC configuration
• Documentation now prioritizes PyPI installation over GitHub

🤖 Generated with Claude Code

Co-Authored-By: Claude noreply@anthropic.com

[Copilot]

Pull Request Overview

This PR adds OIDC authentication support for HTTP mode deployments, fixes a previously skipped test, and updates all installation documentation to use PyPI instead of GitHub URLs.

• Adds optional OIDC authentication for HTTP transport mode with proper environment variable validation
• Fixes test_no_data_loaded test to properly validate NoDataLoadedError exception handling
• Updates all documentation and client configurations to use PyPI installation (uvx databeak)

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
src/databeak/server.py	Adds OIDC authentication settings and integration with validation logic
tests/unit/servers/test_discovery_server_coverage.py	Fixes skipped test by replacing mocks with real session management
docs/installation.md	Updates installation commands and adds OIDC authentication documentation
README.md	Updates quick start examples and adds OIDC configuration section

Comments suppressed due to low confidence (1)

src/databeak/server.py:1

• The import DatabeakSettings is removed but was previously used in the @smithery.server(config_schema=DatabeakSettings) decorator. Verify that removing the config schema doesn't break server configuration functionality.

"""Main FastMCP server for DataBeak."""

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[Copilot]

[nitpick] The import is placed inside the test function rather than at the module level. Consider moving this import to the top of the file with other imports for consistency and better readability.

[codecov]

Codecov Report

❌ Patch coverage is 75.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.68%. Comparing base (3c6ce7e) to head (38ba4ed).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/databeak/server.py	75.00%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #134      +/-   ##
==========================================
- Coverage   84.75%   84.68%   -0.07%     
==========================================
  Files          27       27              
  Lines        3076     3089      +13     
  Branches      458      460       +2     
==========================================
+ Hits         2607     2616       +9     
- Misses        355      357       +2     
- Partials      114      116       +2     

Flag	Coverage Δ
unittests	84.68% <75.00%> (-0.07%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
src/databeak/server.py	92.30% <75.00%> (-4.62%)	⬇️

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3c6ce7e...38ba4ed. Read the comment docs.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[claude]

Code Review: Add OIDC authentication, fix tests, and update PyPI installation docs

Overview

This PR adds OIDC authentication for HTTP mode, fixes a previously-skipped test, and updates documentation to reflect PyPI availability. The intent is sound, but the architectural shift in server.py introduces a critical bug and some maintainability concerns.

---

🚨 Critical Issues

1. Smithery entry point is broken (pyproject.toml:371)

The function create_server() is renamed to create_smithery_server(), but [tool.smithery] still references the old name:

[tool.smithery]
server = "databeak.server:create_server"  # ← will fail with AttributeError

This silently breaks the Smithery integration at startup. Either update the entry to create_smithery_server or keep the function name as create_server.

---

⚠️ Medium Issues

2. Module-level side effects in server.py

OidcSettings(), the auth conditional block, and mcp instantiation all execute at import time:

oidc_settings = OidcSettings()    # reads env vars on import
auth: OIDCProxy | None = None
if oidc_settings.config_url ...:
    auth = OIDCProxy(...)
mcp = FastMCP("DataBeak", auth=auth, ...)  # singleton at import

This has two practical consequences:

• Changing OIDC config mid-process requires reimporting the module, which isn't possible.
• Tests that import server.py will trigger environment reads and create a global mcp, making test isolation harder. The previous create_server() factory was better for this.

3. OIDC client secret should use SecretStr

client_secret: str | None = None stores a raw secret that will appear in repr() output and logging. Pydantic's SecretStr prevents this:

from pydantic import SecretStr

client_secret: SecretStr | None = None

Then pass oidc_settings.client_secret.get_secret_value() to OIDCProxy.

4. No session cleanup in test_no_data_loaded

The test creates a session but has no cleanup path:

_session = manager.get_or_create_session(no_data_session_id)
# ... test proceeds, but session leaks if assertions fail

The test_session fixture pattern from CLAUDE.md handles this correctly. Consider converting to a fixture or adding a try/finally with await manager.remove_session(no_data_session_id).

---

🔍 Minor Issues

5. # type: ignore[arg-type] on mcp.run() (server.py:198)

mcp.run(**run_args)  # type: ignore[arg-type]

Suppressing type errors silently masks mismatches between run_args and FastMCP.run()'s signature. Worth investigating the actual type mismatch, especially given the fastmcp version bump.

6. Log level set in __init__.py as side effect

logging.getLogger("databeak").setLevel(get_settings().log_level)
logging.getLogger("mcp").setLevel(get_settings().log_level)

This runs on every import databeak, which can interfere with application-level logging configuration that runs before the import. A more standard pattern is to configure logging only in main() or at server startup, not at module init time.

7. Removed valid settings from Claude Desktop example (docs/installation.md:78)

The example config removes DATABEAK_MAX_FILE_SIZE_MB and DATABEAK_CHUNK_SIZE. These are still valid settings users may want — removing them from the example reduces discoverability.

---

✅ What's Done Well

• OIDC all-or-nothing validation with logger.error for partial config is correct behavior.
• Replacing mock-based test with real session management is the right call — aligns with project testing philosophy.
• SettingsConfigDict(env_prefix="DATABEAK_") is cleaner than the dict literal.
• Adding log_level to DatabeakSettings follows the existing pattern well.
• PyPI documentation simplification is appropriate now that the package is published.

---

Summary

The Smithery entry point rename (item 1) will break the integration and should be fixed before merge. The OIDC secret type (item 3) and session cleanup (item 4) are straightforward fixes. The module-level side effects (item 2) are an architectural tradeoff worth discussing — the previous factory pattern was more testable.

[claude]

Code Review: PR #134 — OIDC Authentication, Test Fix, Docs Update

Overview

This PR adds optional OIDC authentication for HTTP mode, fixes a previously-skipped unit test, and updates installation docs to reference PyPI. The overall direction is good, but there are several correctness and security issues worth addressing.

---

Critical: Broken Smithery Entry Point

In pyproject.toml, the Smithery config still points to the old function name:

server = "databeak.server:create_server"

But server.py renames it to create_smithery_server(). This will break any Smithery integration that uses the entry point. The pyproject.toml must be updated to match, or the function kept at its original name.

---

Architecture: Module-Level Initialisation in server.py

Moving mcp, oidc_settings, and auth to module level has consequences:

# These all execute at import time, not at server startup:
oidc_settings = OidcSettings()
auth: OIDCProxy | None = None

if oidc_settings.config_url and ...:
    auth = OIDCProxy(...)
elif any([...]):
    logger.error("Incomplete OIDC configuration...")  # logged at import, not startup

• logger.error(...) fires during module import — before any logging handlers are configured. This message may be silently swallowed or go to stderr unexpectedly.
• OIDCProxy is instantiated eagerly regardless of whether HTTP transport is actually used. For stdio deployments, this is wasted initialisation (and a potential failure point if OIDC libs have side effects).
• Consider lazy initialisation: check OIDC config inside main() after the transport is known, only for --transport http.

---

Security: client_secret Not Using SecretStr

class OidcSettings(BaseSettings):
    client_secret: str | None = None

Pydantic provides SecretStr specifically for credentials. Using str means the secret can leak in repr output, logs, and debug traces. Recommend:

from pydantic import SecretStr

client_secret: SecretStr | None = None
# Then access via: oidc_settings.client_secret.get_secret_value()

---

Settings: Removed case_sensitive=False

# Before
model_config = {"env_prefix": "DATABEAK_", "case_sensitive": False}

# After  
model_config = SettingsConfigDict(env_prefix="DATABEAK_")

The SettingsConfigDict default for case_sensitive may differ from the explicit False that was set before. On case-sensitive systems (Linux), this is a behaviour change: DATABEAK_SESSION_TIMEOUT would work but databeak_session_timeout might not. Confirm this is intentional, or explicitly set case_sensitive=False.

---

log_level Field: No Validation

log_level: str = Field(default="INFO", description="Logging level")

This accepts any string. logging.getLogger("databeak").setLevel("NONSENSE") silently does nothing useful. Consider validating against the standard levels:

from typing import Literal
log_level: Literal["DEBUG", "INFO", "WARNING", "ERROR", "CRITICAL"] = "INFO"

---

Side Effect in __init__.py

# src/databeak/__init__.py
logging.getLogger("databeak").setLevel(get_settings().log_level)
logging.getLogger("mcp").setLevel(get_settings().log_level)

Setting log levels as a module-level side effect in __init__.py is surprising and can interfere with applications that import DataBeak as a library and manage their own logging. This is better placed in main() or a dedicated configure_logging() function called at startup.

---

mcp.run() Type Ignore

mcp.run(**run_args)  # type: ignore[arg-type]

The suppression suggests a type mismatch between what run_args contains and what FastMCP.run() expects. This should be resolved rather than suppressed — either by typing run_args correctly or by passing arguments explicitly.

---

Test Fix

The test_no_data_loaded fix is correct: replacing mock-based testing with a real session is strictly better and aligns with the project's testing philosophy. The removal of unused imports is clean. No issues here.

---

Documentation

The PyPI installation updates and OIDC documentation additions are accurate and well-structured. The note that OIDC only applies to HTTP transport is clearly communicated.

---

Summary

Area	Status
Smithery entry point (create_server → create_smithery_server)	❌ Must fix
client_secret using plain str	⚠️ Use SecretStr
case_sensitive=False dropped silently	⚠️ Confirm intent
Module-level OIDC init + logger.error at import time	⚠️ Move to main()
log_level no validation	⚠️ Use Literal type
Logging side effects in __init__.py	⚠️ Move to startup
type: ignore on mcp.run()	⚠️ Investigate root cause
Test fix	✅
Documentation	✅

The broken entry point is a blocker. The security and architectural concerns are significant enough to warrant a revision pass before merge.

[claude]

Code Review: PR #134 — OIDC Authentication, Test Fix, and PyPI Docs

Overview

This PR adds optional OIDC authentication for HTTP mode deployments, fixes one skipped test, and updates all installation docs to point at PyPI. The functionality is additive and the docs changes are straightforward, but there are a few structural concerns in the server refactor worth addressing.

---

Server Refactor — Key Structural Changes

🔴 pyproject.toml entry point is broken

The PR renames create_server() → create_smithery_server() in server.py, but pyproject.toml still has:

server = "databeak.server:create_server"

This will cause the Smithery entry point to fail at runtime. The [project.scripts] or [project.entry-points] section needs updating.

🟡 Module-level OIDC initialization

oidc_settings = OidcSettings()
auth: OIDCProxy | None = None

if oidc_settings.config_url and ...:
    auth = OIDCProxy(...)

OIDC is initialized at module import time, not lazily. This means:

• Environment variables must be set before the module is first imported
• Tests that import server.py will trigger OIDC initialization even when testing unrelated functionality
• No way to reconfigure OIDC without restarting the process (acceptable for a server, but worth documenting)

The pattern is workable for a production server, but it complicates testing. Consider moving initialization inside main() or using a factory function.

🟡 OidcSettings placed in server.py, not settings.py

Project convention (per CLAUDE.md and the existing DatabeakSettings) is to centralize all settings in src/databeak/core/settings.py. OidcSettings should live there alongside DatabeakSettings.

🟡 mcp.run(**run_args) # type: ignore[arg-type]

The type: ignore comment indicates a type mismatch between what run_args contains and what mcp.run() accepts. This should be resolved properly — either by typing run_args correctly or unpacking the known keys explicitly.

@smithery.server() — removed config_schema=DatabeakSettings ⚠️

The old code had @smithery.server(config_schema=DatabeakSettings). Removing config_schema may affect Smithery's ability to surface configuration options to users. Was this intentional?

---

OIDC Authentication

Design is sound ✅

The "all-or-nothing" approach (all four vars must be set, otherwise log error and skip) is a reasonable safety choice — partial OIDC config is worse than no OIDC config.

Partial config is silent-ish 🟡

The error log when OIDC is incomplete reads:

logger.error("Incomplete OIDC configuration...")

This logs at error level but doesn't raise. If a user misconfigures OIDC expecting authentication to be enabled, the server starts successfully with no auth. This could be a security footgun. Consider raising an exception (or at minimum a more prominent warning) when partial config is detected, so misconfiguration fails loudly.

DATABEAK_OIDC_CLIENT_SECRET in env ✅

Standard practice for secrets in server applications. Acceptable.

---

Test Fix — test_no_data_loaded

Session leak in the fixed test 🟡

_session = manager.get_or_create_session(no_data_session_id)
ctx = create_mock_context(no_data_session_id)
with pytest.raises(NoDataLoadedError):
    await detect_outliers(ctx)

The test creates a real session but never removes it. Following the project's session fixture pattern (per CLAUDE.md), a cleanup should be added:

try:
    ...test body...
finally:
    await manager.remove_session(no_data_session_id)

Exception type change: ToolError → NoDataLoadedError ✅

Using the more specific exception type is correct — it tests the actual contract of the function rather than the MCP wrapper.

---

Documentation

The docs changes (PyPI install, OIDC config table, Python 3.12+ requirement) are clear and accurate. No concerns.

The __init__.py logging setup is a minor quality-of-life improvement:

logging.getLogger("databeak").setLevel(get_settings().log_level)

The new log_level: str field in DatabeakSettings works, but there's no validation that the value is a valid Python log level. Consider using a Literal["DEBUG", "INFO", "WARNING", "ERROR", "CRITICAL"] type or a validator.

---

Summary

Item	Status
Entry point create_server → create_smithery_server mismatch	🔴 Breaking — fix before merge
OidcSettings in server.py vs settings.py	🟡 Convention violation
Silent partial OIDC config	🟡 Security UX risk
Module-level OIDC init	🟡 Testing complexity
Session leak in test	🟡 Test hygiene
type: ignore in mcp.run()	🟡 Unresolved type mismatch
log_level field without validation	🟡 Minor
Docs updates	✅ Clean
OIDC design intent	✅ Sound

The entry point breakage is the only blocker. The other items are improvement opportunities.

---

Review generated by Claude Code

[claude]

Code Review: PR #134 — OIDC Authentication, Test Fix, PyPI Docs

The intent here is solid — OIDC auth, a proper test fix, and a long-overdue PyPI simplification. But the server.py refactor has two issues that will prevent the server from loading correctly in some configurations, plus several structural concerns worth addressing.

---

🔴 Critical Issues

1. IndentationError in create_smithery_server()

The diff shows the function docstring has 5 spaces of indentation while return mcp has 4 spaces. Python will raise IndentationError on import:

@smithery.server()
def create_smithery_server() -> FastMCP:
     """Create and return the Smithery FastMCP server instance."""  # 5 spaces
    return mcp                                                        # 4 spaces — IndentationError

The same problem appears in the new create_server() body: """Create and return...""", # Initialize FastMCP server, and # Mount specialized servers all have 5 spaces while the actual code has 4. This is likely a stray editor artifact but it will break the module.

2. mcp is instantiated twice — create_server() is now dead code

The PR creates a module-level mcp and leaves create_server() creating a second, independent FastMCP instance with duplicate mounts and prompt registrations:

# Module level — the one main() now uses
mcp = FastMCP("DataBeak", auth=auth, ...)
mcp.mount(system_server)
...
@mcp.prompt
def analyze_csv_prompt(...): ...

# create_server() — completely separate instance, never used by main()
def create_server() -> FastMCP:
    mcp = FastMCP("DataBeak", auth=auth, ...)  # different object
    mcp.mount(system_server)                    # double-mount side effects?
    ...
    mcp.prompt()(analyze_csv_prompt)            # double-registers the prompt

main() now calls mcp.run() (module-level) and create_smithery_server() returns the module-level mcp, so create_server() is unreachable in normal operation. It should either be removed or become the single source of truth. Leaving two diverging construction paths is a maintenance hazard.

---

🟡 Moderate Issues

3. OidcSettings belongs in settings.py, not server.py

The project has a clear pattern: all DATABEAK_* settings live in DatabeakSettings in core/settings.py (and the CLAUDE.md explicitly references the centralized settings pattern). OidcSettings breaks this:

• It duplicates BaseSettings / SettingsConfigDict imports in server.py
• It's not accessible via get_settings() for other modules
• It's evaluated at module import time with no way to override for tests

Consider folding it into DatabeakSettings as optional fields (e.g. oidc_config_url: str | None = None).

4. Missing test session cleanup

test_no_data_loaded creates a real session but never removes it:

async def test_no_data_loaded(self) -> None:
    no_data_session_id = str(uuid.uuid4())
    manager = get_session_manager()
    _session = manager.get_or_create_session(no_data_session_id)
    # ... test assertions ...
    # ← no cleanup

The session leaks between runs. The project's standard pattern (from CLAUDE.md and tests/README.md) is a fixture with yield + teardown:

@pytest.fixture
async def no_data_session():
    session_id = str(uuid.uuid4())
    manager = get_session_manager()
    manager.get_or_create_session(session_id)
    yield session_id
    await manager.remove_session(session_id)

5. Silent OIDC auth failure is a potential security gap

When OIDC vars are partially configured, the PR logs an error and silently continues without authentication. If an operator sets three of the four variables thinking auth is active, the server will be unprotected with no visible indication at startup beyond a log line. Consider either raising at startup or at minimum printing a WARNING to stderr so it's visible outside log aggregation.

---

🟢 Minor Issues

6. log_level: str has no validation

log_level: str = Field(default="INFO", description="Logging level")

logging.setLevel() silently ignores invalid strings (it falls back to WARNING). A typo like DATABEAK_LOG_LEVEL=DEBU will silently misconfigure logging. Consider:

from typing import Literal
log_level: Literal["DEBUG", "INFO", "WARNING", "ERROR", "CRITICAL"] = Field(default="INFO", ...)

7. case_sensitive removed from DatabeakSettings

The old config had "case_sensitive": False. The new SettingsConfigDict(env_prefix="DATABEAK_") omits it. pydantic-settings v2 is case-insensitive by default, so this is likely fine, but worth a comment confirming it's intentional — it's the kind of silent behavior change that can surprise future maintainers.

8. Module-level get_settings() call in __init__.py

logging.getLogger("databeak").setLevel(get_settings().log_level)

This runs on every import databeak, initializes the settings singleton before any test can configure environment variables, and makes test isolation harder. Consider deferring this to main() or using logging.basicConfig with the standard DATABEAK_LOG_LEVEL handling.

9. Doc example uses DATABEAK_MAX_DOWNLOAD_SIZE_MB: 200

The original example had DATABEAK_MAX_FILE_SIZE_MB: 2048. Verify DATABEAK_MAX_DOWNLOAD_SIZE_MB is an actual setting — if not, this is a misleading example.

---

✅ What's Working Well

• The test fix replacing patch.object with real session management is the right approach and correctly tests NoDataLoadedError directly.
• Simplifying all uvx --from git+... to uvx databeak is a clean, necessary improvement.
• The SettingsConfigDict migration is a positive modernization.
• OIDC documentation is clear about the HTTP-only scope.
• Adding .envrc to .gitignore is a sensible quality-of-life addition.

---

Summary

The indentation error and the duplicated mcp construction are the blockers — the former is a crash, the latter is a correctness/maintenance risk. The other items are real but addressable in follow-on work. Happy to discuss the create_server() architecture question if there are Smithery-specific constraints driving it.

[claude]

Code Review — PR #134: OIDC Authentication, Test Fix, and PyPI Docs

Overview

This PR adds OIDC authentication for HTTP mode, fixes a skipped test in the discovery server, and updates all installation docs to reference PyPI. The intent is sound, but there are structural issues in server.py that introduce a regression and some design concerns worth addressing.

---

Critical: Health Check Endpoint Silently Drops in HTTP Mode

src/databeak/server.py

The refactor creates a module-level mcp instance and registers prompts/mounts on it. main() now calls mcp.run() using this module-level instance — but the /health custom route is still defined inside create_server() on a local mcp variable that shadows the module-level one:

# Module-level mcp — used by main()
mcp = FastMCP("DataBeak", auth=auth, ...)

# ...

def create_server() -> FastMCP:
    mcp = FastMCP(...)          # ← local variable, shadows module-level
    @mcp.custom_route("/health", ...)   # ← registered on local mcp, not the one main() runs
    ...

def main():
    mcp.run(...)  # ← module-level mcp has NO /health route

Any HTTP deployment relying on GET /health for container orchestration or load balancer probes will get 404s. The health check endpoint needs to be registered on the module-level mcp, not inside create_server().

---

Significant: Duplicate Server Instance / Dead Code

create_server() builds a second FastMCP instance with all the same mounts and prompts, but this instance is never used — main() bypasses it entirely. This creates two problems:

1. All nine sub-servers are mounted twice at import time (once on the module-level mcp, once inside create_server()). Depending on FastMCP's internals this may be harmless, but it's surprising state.
2. create_server() is dead code. If it's kept for backward compatibility (e.g. callers outside this repo), that should be documented; otherwise it should be removed.

Similarly, create_smithery_server() just returns the module-level mcp. If Smithery only needs the server instance, the @smithery.server() decorator on a thin wrapper is an odd pattern — worth a comment explaining why it can't decorate the module-level mcp directly.

---

Moderate: Module-Level OIDC Initialization Has Side Effects

# server.py top-level
oidc_settings = OidcSettings()  # reads env vars at import time
auth: OIDCProxy | None = None

if oidc_settings.config_url and ...:
    auth = OIDCProxy(...)       # makes network/DNS calls at import time?
elif any([...]):
    logger.error(...)           # logs at import time

Executing this at module load means any test that imports server triggers environment variable reads and potentially an OIDCProxy initialization (which may do OIDC discovery). This complicates test isolation. A lazy initialization pattern (e.g. a get_auth() function called from main()) would be safer and consistent with how DatabeakSettings is handled via get_settings().

---

Minor: Test Session Leak

tests/unit/servers/test_discovery_server_coverage.py:624

The new test_no_data_loaded creates a session but never tears it down:

_session = manager.get_or_create_session(no_data_session_id)
# ... test runs, session is never removed

Other tests in this file use fixture-based teardown with await manager.remove_session(...). This session will accumulate across runs and could affect session capacity warnings if tests run in a shared process. Add explicit cleanup (try/finally or a fixture).

---

Minor: log_level Not Validated

src/databeak/core/settings.py

log_level: str = Field(default="INFO", description="Logging level")

An invalid value like DATABEAK_LOG_LEVEL=DEBG will silently pass Pydantic validation and then fail at runtime when passed to logging.setLevel(). Consider using Literal["DEBUG", "INFO", "WARNING", "ERROR", "CRITICAL"] or a validator.

---

Minor: case_sensitive Removed Without Noting the Behavioral Change

The old config was {"env_prefix": "DATABEAK_", "case_sensitive": False}. The new SettingsConfigDict(env_prefix="DATABEAK_") drops case_sensitive. The pydantic-settings default for case_sensitive is False, so behavior should be unchanged — but this implicit reliance on a default is worth a comment, or restoring it explicitly to make the intent clear.

---

Positive Notes

• The documentation updates (PyPI over GitHub, Python 3.12+ requirement, OIDC table) are clean and well-structured.
• Replacing the mocked test_no_data_loaded with real session management is the right call — the old mock approach didn't actually test the code path.
• The OidcSettings / DatabeakSettings separation into distinct classes is a clean pattern.
• .envrc in .gitignore is a sensible addition.

---

Summary

The health check regression (HTTP /health 404) is the blocker — it's a behavioral change that will break any containerized HTTP deployment without any test to catch it. The dead create_server() and module-level initialization concerns are worth addressing in the same pass. Everything else is polish.

[claude]

Code Review: PR #134 — OIDC Authentication, Test Fix, PyPI Docs

Overview

This PR adds optional OIDC authentication for HTTP mode, fixes a previously skipped test in the discovery server, and updates documentation to reference PyPI installation. The direction is sound but there are several structural issues that need attention before merging.

---

🔴 Critical Issues

1. Health check endpoint is broken

main() now runs the module-level mcp directly (mcp.run(**run_args)), bypassing create_server() entirely. But the /health route is only registered inside create_server():

# In create_server() — never called by main()
@mcp.custom_route("/health", methods=["GET"])
async def health_check(_request: Request) -> PlainTextResponse:
    return PlainTextResponse("OK")

Any container orchestration that probes /health will get a 404. The health check is documented as an HTTP mode feature; silently breaking it is a regression.

2. create_server() is dead code with duplicate mounts

main() now runs the module-level mcp, rendering create_server() unused. However, create_server() still exists and:

• Creates a second FastMCP instance with the same name and mounts
• Attempts to re-register analyze_csv_prompt and data_cleaning_prompt via mcp.prompt()(...), but those are already bound to the module-level mcp via @mcp.prompt

This should either be removed entirely (and the health route moved to module level) or it needs to be the canonical factory used by both main() and create_smithery_server().

3. --log-level CLI flag is silently ignored

__init__.py sets log levels at import time:

logging.getLogger("databeak").setLevel(get_settings().log_level)
logging.getLogger("mcp").setLevel(get_settings().log_level)

main() calls setup_structured_logging(args.log_level) later, but the "databeak" and "mcp" loggers are already configured and won't respect the CLI argument. A user passing --log-level DEBUG will not see debug output from either logger.

---

🟡 Structural Issues

4. OidcSettings belongs in core/settings.py

The project consolidates all configuration in core/settings.py with the DatabeakSettings class. OidcSettings is defined in server.py, which violates this pattern and makes it harder to test in isolation or reference from other modules. Consider either adding OIDC fields to DatabeakSettings (with DATABEAK_OIDC_ prefix via Field(alias=...)) or moving the class to core/settings.py.

5. Module-level OIDC initialization runs at import time

oidc_settings = OidcSettings()
auth: OIDCProxy | None = None

if oidc_settings.config_url and ...:
    auth = OIDCProxy(...)
elif any([...]):
    logger.error("Incomplete OIDC configuration...")

This runs every time server.py is imported — including during tests. Partially-set OIDC env vars in a test environment will produce error-level log noise and could cause unexpected test failures if OIDCProxy.__init__ has side effects. Consider deferring this to main() or wrapping it in a factory function.

6. log_level field accepts any string

log_level: str = Field(default="INFO", description="Logging level")

logging.setLevel("VERBOSE") would silently fail or raise. This should be typed as Literal["DEBUG", "INFO", "WARNING", "ERROR", "CRITICAL"] to catch misconfiguration at settings load time.

---

🟡 Test Issues

7. Test session leak in test_no_data_loaded

The fixed test creates a real session but never cleans it up:

_session = manager.get_or_create_session(no_data_session_id)
# ... test runs ...
# no manager.remove_session() call

The project's own fixture pattern (from CLAUDE.md and tests/README.md) uses yield + explicit cleanup. This session will persist in the manager for the lifetime of the test process and could affect other tests.

8. Import inside test function

async def test_no_data_loaded(self) -> None:
    from databeak.exceptions import NoDataLoadedError

Move this to the module-level imports at the top of the file, consistent with all other imports in this test module.

---

🟢 Positive Changes

• SettingsConfigDict migration in settings.py is the right move — typed config over raw dicts.
• PyPI installation docs are a meaningful improvement for users; the simplification from uvx --from git+... to uvx databeak is correct now that the package is published.
• Fixing the skipped test by using real session management over mocks is the right call — consistent with the project's stated preference for integration-style tests.
• .envrc in .gitignore is a sensible addition for direnv users.
• fastmcp version bump to >=3.2.0 in pre-commit is necessary for OIDCProxy and is clearly intentional.
• Documentation corrections: DATABEAK_MAX_FILE_SIZE_MB → DATABEAK_MAX_DOWNLOAD_SIZE_MB and removing DATABEAK_CHUNK_SIZE align with the actual field names in settings.py.

---

Minor Notes

• The # type: ignore[arg-type] on mcp.run(**run_args) suppresses rather than fixes a typing issue — worth investigating if run_args can be typed more precisely.
• client_secret is stored as a plain str. For settings that hold secrets, pydantic.SecretStr prevents accidental logging of the value. Low priority but worth considering.
• Python version updated to 3.12+ in docs without a corresponding pyproject.toml change visible in the diff — verify this is consistent.

---

Summary

The OIDC feature concept is solid, but the refactor to use a module-level mcp instance introduced a broken health endpoint and dead create_server() code. The logging initialization order creates a real user-facing bug where --log-level stops working. These three issues should be resolved before merging.

[claude]

Code Review: PR #134 — Add OIDC authentication, fix tests, and update PyPI installation docs

Good overall direction — OIDC support is a useful addition and the PyPI migration simplifies install docs significantly. There are a few architectural issues that need attention before merge.

---

Critical Issues

1. Dual-instance mcp creates test/production divergence

server.py now has two separate FastMCP instances:

• A module-level mcp (line ~97) with @mcp.prompt decorators and sub-server mounts
• A local mcp inside create_server() that re-mounts all sub-servers and is returned to integration tests

main() now calls mcp.run() (the module-level instance), while integration tests use create_server()._fastmcp. These are different objects. Integration tests do not test the server that actually runs in production, and the @mcp.prompt registrations on the module-level instance are invisible to integration tests.

Either create_server() should return the module-level mcp, or the module-level mounting should be removed and deferred to create_server(). The current split is a latent bug waiting to cause confusion.

2. Module-level side effects run before logging is configured

In server.py, OidcSettings() instantiation and the elif any([...]) / logger.error(...) block execute at module import time. At that point, logging may not yet be configured (the __init__.py log-level setup itself calls get_settings() at import time, but the ordering between these two modules is not guaranteed). The error message could be silently swallowed or go to the root logger unexpectedly.

Move OIDC initialization into main() or a deferred factory function rather than at module scope.

---

Significant Issues

3. create_server() is now effectively dead code for the production path

main() was changed from create_server().run(**run_args) to mcp.run(**run_args). create_server() is still called from integration tests, but any changes to its internal mcp initialization (e.g. a future auth change) won't affect what main() runs. Either remove create_server() and expose the module-level mcp directly, or keep create_server() as the single source of truth.

4. create_smithery_server() and create_server() have overlapping purposes

Two functions now wrap FastMCP, with @smithery.server() moved to create_smithery_server(). This widens the surface area — any caller of create_server() (including the integration test conftest) gets a different object than Smithery uses. The intent should be clarified.

5. type: ignore[arg-type] added without explanation

mcp.run(**run_args)  # type: ignore[arg-type]

Suppressing a type error without a comment explaining why is against the project's zero-tolerance ruff/mypy standard. If run_args has a legitimate type mismatch, it should be typed correctly rather than silenced.

---

Minor Issues

6. log_level field accepts any string

log_level: str = Field(default="INFO", description="Logging level")

Should be typed as Literal["DEBUG", "INFO", "WARNING", "ERROR", "CRITICAL"] to catch invalid values at startup rather than silently passing a bad string to logging.setLevel().

7. Test session not cleaned up

test_no_data_loaded creates a session via manager.get_or_create_session(no_data_session_id) but never removes it. Other tests in the suite use explicit teardown fixtures for session cleanup — this test should follow the same pattern to avoid session manager state leaking between tests.

8. case_sensitive=False removed from DatabeakSettings without callout

The original config was {"env_prefix": "DATABEAK_", "case_sensitive": False}. The new SettingsConfigDict(env_prefix="DATABEAK_") drops case_sensitive. Pydantic-settings v2 defaults differ from v1 here. Verify this doesn't break users who use lowercase env var names (unlikely but worth confirming).

9. OIDCProxy imported from an internal module path

from fastmcp.server.auth.oidc_proxy import OIDCProxy

This is a private internal path that fastmcp could reorganize without a semver bump. Check whether OIDCProxy is exported from a public fastmcp or fastmcp.server.auth namespace instead.

10. OidcSettings fields have no Field descriptions

CLAUDE.md and the project conventions require Field(description=...) on settings fields. OidcSettings has bare = None assignments without descriptions.

11. Documentation example variable name change

In docs/installation.md, the example env block changed DATABEAK_MAX_FILE_SIZE_MB to DATABEAK_MAX_DOWNLOAD_SIZE_MB. If DATABEAK_MAX_FILE_SIZE_MB is still a valid setting in settings.py, this is a misleading docs change. If the variable was renamed, that rename should be reflected in settings.py and called out as a breaking change.

---

What's Good

• Removing uvx --from git+... in favor of uvx databeak is the right simplification now that the package is on PyPI.
• The test fix for test_no_data_loaded correctly replaces mocking with real session management, matching the project's integration testing philosophy.
• SettingsConfigDict migration is the right direction for explicit pydantic-settings v2 usage.
• The OIDC partial-config detection (the elif any(...) guard) with a clear error message is good defensive behavior — just needs to be moved out of module scope.
• .envrc gitignore addition is a sensible QoL addition for direnv users.

---

Summary

The feature is sound but the architectural split between the module-level mcp and create_server() is the blocking issue — it means integration tests silently diverge from the production code path. Recommend consolidating to a single instance before merge.

[claude]

Code Review: PR #134 — OIDC Authentication, Test Fix, PyPI Docs

Overview

This PR bundles three distinct concerns: OIDC auth support for HTTP mode, a fix for a previously-skipped test, and a documentation refresh for PyPI installation. The doc and test changes are generally clean; the server.py refactoring has a few structural issues worth addressing.

---

Critical Issues

1. Duplicate mcp instances — module-level vs. create_server()

server.py now has two separate FastMCP instances:

# Module level (new)
mcp = FastMCP("DataBeak", auth=auth, instructions=_load_instructions(), version=__version__)
mcp.mount(system_server)
...

# Inside create_server() (kept)
def create_server() -> FastMCP:
    mcp = FastMCP("DataBeak", auth=auth, instructions=_load_instructions(), version=__version__)
    mcp.mount(system_server)
    ...
    return mcp

main() runs the module-level instance, but integration tests use create_server(). These are different objects — @mcp.prompt decorators bind to the module-level instance only, so prompts registered there are absent from the create_server() instance used by integration tests.

Recommended fix: Have create_server() return the module-level singleton, or move all setup into create_server() and call it once at module level.

2. @mcp.prompt decorators registered on the wrong instance

@mcp.prompt
def analyze_csv_prompt(...): ...

@mcp.prompt
def data_cleaning_prompt(...): ...

These bind to the module-level mcp. The create_server() instance (used by integration tests and previously by main()) does not have these prompts. This is a regression — prompts that previously worked will silently disappear for consumers of create_server().

3. Missing session cleanup in the fixed test

async def test_no_data_loaded(self) -> None:
    no_data_session_id = str(uuid.uuid4())
    manager = get_session_manager()
    _session = manager.get_or_create_session(no_data_session_id)
    # ... test runs ...
    # No cleanup!

The session is never removed. Given the global SessionManager, this leaks state into subsequent tests. Add a try/finally or use the test_session fixture pattern from tests/README.md.

---

Design Concerns

4. Module-level side effects at import time

oidc_settings = OidcSettings()  # reads env at import
auth: OIDCProxy | None = None

if oidc_settings.config_url and ...:
    auth = OIDCProxy(...)       # network-capable object at import
elif any([...]):
    logger.error("Incomplete OIDC configuration...")  # logs at import time

This means any test or tool that imports databeak.server will:

• Instantiate OidcSettings (reads env vars)
• Potentially instantiate OIDCProxy
• Potentially emit a log error

This makes test isolation harder and can cause surprising behavior. Prefer lazy initialization inside main() or create_server().

5. log_level: str not validated

log_level: str = Field(default="INFO", description="Logging level")

An invalid value like DATABEAK_LOG_LEVEL=verbose would be accepted silently and then cause a confusing error when passed to logging.setLevel(). Consider using Literal["DEBUG", "INFO", "WARNING", "ERROR", "CRITICAL"] or a @field_validator.

6. case_sensitive removed from model_config

# Before
model_config = {"env_prefix": "DATABEAK_", "case_sensitive": False}

# After
model_config = SettingsConfigDict(env_prefix="DATABEAK_")

pydantic-settings defaults vary by platform (case-sensitive on Linux). If users relied on case-insensitive env var names, this could silently break them. Worth being explicit: SettingsConfigDict(env_prefix="DATABEAK_", case_sensitive=False).

---

Minor Issues

• fastmcp>=3.2.0 is a large version jump from >=0.1.0. If OIDCProxy is the only reason for this bump, it's worth verifying no other fastmcp APIs changed in ways that affect existing tools.
• DATABEAK_MAX_FILE_SIZE_MB and DATABEAK_CHUNK_SIZE removed from the docs example in installation.md — if these env vars still exist in settings, removing them from the example silently drops guidance users may rely on.
• # type: ignore[arg-type] on mcp.run(**run_args) — worth understanding why this is needed rather than suppressing it. If run_args keys don't match FastMCP.run()'s signature, this could fail at runtime.
• The create_smithery_server() function returns the module-level mcp while create_server() creates a new instance. These should be consistent.

---

What Works Well

• The OIDC configuration is clean and self-contained in OidcSettings
• All-or-nothing OIDC activation with an error log for partial config is the right UX
• The test fix correctly uses real session management instead of mocks — aligned with the project's testing philosophy
• Documentation improvements are accurate and clean
• .envrc gitignore addition is sensible
• Switching to SettingsConfigDict is the right modernization step

---

Summary

The main thing to resolve before merging is the duplicate mcp instance problem — the module-level instance and create_server() diverge silently, which will cause integration tests to miss prompts and any future divergence to be hard to diagnose. The session leak in the test is a straightforward fix. The remaining items are lower priority but worth addressing.

[claude]

Code Review

Overview

This PR covers three distinct areas: OIDC authentication for HTTP mode, a previously skipped test fix, and PyPI-based installation docs. The test fix and documentation changes are solid. The OIDC integration has structural issues that need attention before merge.

---

Critical Issues

1. Duplicate server instances (server.py)

The PR now has two parallel code paths that create and mount FastMCP instances:

# Path 1: module-level (used by main())
mcp = FastMCP("DataBeak", auth=auth, ...)
mcp.mount(system_server)
...

# Path 2: create_server() — independent instance (used by integration tests)
def create_server() -> FastMCP:
    mcp = FastMCP("DataBeak", auth=auth, ...)
    mcp.mount(system_server)
    ...

main() was changed to mcp.run() (module-level), but create_server() creates a separate instance and mounts all servers again. The integration tests in conftest.py call create_server(), so they exercise a different server than what main() runs. More directly: if a tool registers state on a mounted server object, the two instances will diverge. This needs a single authoritative server instance.

2. @smithery.server() decorator silently broken

Previously create_server() was decorated with @smithery.server(config_schema=DatabeakSettings). This PR introduces create_smithery_server() returning the module-level mcp, while create_server() is left undecorated and creates a new instance. Two consequences:

• The config_schema=DatabeakSettings argument was dropped with no explanation — Copilot flagged this too.
• If Smithery discovers the server by finding the decorated function, renaming it from create_server to create_smithery_server may break that discovery silently.

3. Prompt decorators only on the module-level mcp

@mcp.prompt
def analyze_csv_prompt(...): ...

@mcp.prompt
def data_cleaning_prompt(...): ...

These register prompts on the module-level mcp. The mcp created inside create_server() never receives these registrations, so integration tests (which use create_server()) run against a server with no prompts.

---

Significant Issues

4. OIDC initialized at import time

oidc_settings = OidcSettings()
auth: OIDCProxy | None = None

if oidc_settings.config_url and ...:
    auth = OIDCProxy(...)

This runs when server.py is first imported — before main() is called, before any argument parsing. Env vars must be set prior to the first import of this module. Any test that imports server without OIDC env vars baked in will get auth=None permanently, even if those vars are set later in the process. Moving OIDC initialization into main() (or a lazy-init function) would be more correct.

5. OidcSettings belongs in settings.py

CLAUDE.md is explicit: "Configuration centralized in csv_session.py" (and by extension, settings.py). Defining a second BaseSettings subclass in server.py fragments the configuration surface and makes DATABEAK_OIDC_* variables invisible to anyone inspecting settings.py.

6. log_level needs validation

log_level: str = Field(default="INFO", description="Logging level")

str accepts any value. DATABEAK_LOG_LEVEL=VERBOSE will silently set a nonsense level and logging.setLevel() will ignore unknown strings (they're treated as level 0). Use:

from typing import Literal
log_level: Literal["DEBUG", "INFO", "WARNING", "ERROR", "CRITICAL"] = Field(default="INFO", ...)

7. Test session leak in test_no_data_loaded

_session = manager.get_or_create_session(no_data_session_id)
ctx = create_mock_context(no_data_session_id)
with pytest.raises(NoDataLoadedError):
    await detect_outliers(ctx)

The session is created but never removed. Following the project's fixture pattern (manager.remove_session()), add cleanup — either wrap in a try/finally or convert to a fixture with yield.

---

Minor Issues

8. Python version requirement change is a breaking change

docs/installation.md and README.md now say 3.12+ (was 3.10+). This isn't in the "Breaking Changes" section of the PR description, and it's unclear whether pyproject.toml was updated to match. Users on 3.10/3.11 will hit confusing failures.

9. type: ignore without explanation

mcp.run(**run_args)  # type: ignore[arg-type]

Per project conventions, the comment should briefly explain why — what's the type mismatch and why isn't it fixable cleanly?

---

What Works Well

• Test fix: Replacing mocks with real session management is the right call. NoDataLoadedError being raised (rather than ToolError) matches the actual exception hierarchy and makes the test more meaningful.
• SettingsConfigDict upgrade: Switching from {"env_prefix": ...} dict to typed SettingsConfigDict in settings.py is a good cleanup.
• Partial OIDC warning: Logging an error when only some OIDC vars are set is the right behavior for catching misconfiguration.
• Documentation: The PyPI simplification is clean and the OIDC docs are clear about scope (HTTP-only).

---

Suggested Priority for Fixes

1. Resolve the duplicate server / single-instance problem — this is architecturally load-bearing.
2. Move OidcSettings to settings.py and defer OIDC init to main().
3. Fix the Smithery decorator / config_schema regression.
4. Add session cleanup to the test.
5. Add log_level validation.
6. Note the Python version bump in Breaking Changes and verify pyproject.toml alignment.

[claude]

Code Review: PR #134 — OIDC Authentication, Test Fix, PyPI Docs

Overall this is a useful addition, but there are several architectural and correctness issues worth addressing before merge.

---

Critical Issues

1. Duplicate server instantiation in server.py

The refactor creates two separate FastMCP("DataBeak") instances:

• A module-level mcp (line ~97) with servers mounted at module load time.
• A second instance created inside create_server() (still present after the refactor) that mounts all the same sub-servers again.

main() was changed to call mcp.run() (the global), but create_server() still builds and returns its own fresh instance. This means create_server() (used by integration tests) and main() (used in production) are running different server objects. At minimum this is confusing; at worst it causes integration tests to test a different code path than production.

Recommendation: remove the server-building logic from create_server() entirely and have it just return the module-level mcp.

2. Module-level side effects in server.py

oidc_settings = OidcSettings()   # runs at import time
auth: OIDCProxy | None = None
if oidc_settings.config_url and ...:
    auth = OIDCProxy(...)

This runs OIDC setup and potentially network I/O (OIDC provider discovery) at import time. This makes the module hard to test, sensitive to environment state at import, and breaks the lazy-singleton pattern used everywhere else in the codebase (get_settings()). Move OIDC setup inside a factory function, similar to how DatabeakSettings uses get_settings().

3. Silent auth bypass on partial OIDC config (security concern)

elif any([...]):
    logger.error("Incomplete OIDC configuration...")

If an operator intends to run with OIDC but makes a typo in one variable name, the server starts unauthenticated with only a log error. For a security feature, this should raise an exception and refuse to start — fail-closed, not fail-open.

---

Design/Convention Issues

4. OidcSettings belongs in settings.py

The OidcSettings class is defined in server.py, but all other settings classes live in core/settings.py. This breaks the centralized configuration pattern described in CLAUDE.md. Moving it there would also allow get_settings() (or a new get_oidc_settings()) to be used instead of a bare module-level instance.

5. client_secret should be SecretStr

client_secret: str | None = None

Pydantic provides SecretStr specifically to prevent secrets from being logged or serialised in plaintext. This is an easy, low-risk improvement:

from pydantic import SecretStr
client_secret: SecretStr | None = None

Then pass .get_secret_value() when constructing OIDCProxy.

6. log_level field is unvalidated

log_level: str = Field(default="INFO", description="Logging level")

An invalid value like DATABEAK_LOG_LEVEL=VERBOSE would be accepted but logging.getLevelName() would silently fall back to a numeric level. Use a Literal or add a @field_validator to catch this early.

---

Correctness / Test Issues

7. Test session not cleaned up in test_no_data_loaded

_session = manager.get_or_create_session(no_data_session_id)

The session is created but never removed. The existing pattern in the test suite uses a fixture that calls manager.remove_session(session_id) on teardown. Without cleanup, this leaks state across tests if the session manager is shared.

8. Private API usage in conftest.py

async with Client(mcp, auto_initialize=False) as client:
    await client._transport.connect()
    yield client
    await client._transport.disconnect()

Accessing client._transport couples the integration test setup to FastMCP internals. This will silently break on any FastMCP version that renames or restructures transport handling. Worth raising with the FastMCP maintainers to see if there is a supported API for in-process testing.

---

Minor Points

• fastmcp>=3.2.0 in .pre-commit-config.yaml is a major version constraint jump from >=0.1.0. If FastMCP 3.x has breaking changes, this should be reflected in pyproject.toml as well, not just pre-commit.
• # type: ignore[arg-type] on mcp.run(**run_args) was added without a comment explaining why. A short inline note (# run_args keys are not in the TypedDict but are valid kwargs) aids future maintainers.
• @mcp.prompt decorators were added to analyze_csv_prompt and data_cleaning_prompt. If these were previously unregistered, that looks like a pre-existing bug being fixed — worth calling out explicitly in the PR description so it is not lost as a side effect.
• The Python version bump in docs (3.10+ → 3.12+) should be verified against the requires-python field in pyproject.toml for consistency.

---

What is Working Well

• Documentation updates are clean and the PyPI migration is straightforward.
• The test fix for test_no_data_loaded correctly moves away from fragile mock patching toward real session management, which matches the project's testing philosophy.
• Using SettingsConfigDict instead of a bare dict in settings.py is the right direction.
• .envrc added to .gitignore is a sensible addition for direnv users.

[claude]

Code Review — PR #134: OIDC Auth, Test Fix, PyPI Docs

Good direction overall — OIDC support is a meaningful addition and the test fix removes a TODO. However, there are several architectural issues in server.py that need attention before merging.

---

Critical

Two divergent mcp instances (server.py)

The PR introduces a module-level mcp singleton and keeps create_server() creating a fresh local mcp (shadowing the global). These two instances are not equivalent:

• The global mcp is used by main() and create_smithery_server().
• create_server() (used by integration tests) builds a separate FastMCP with the same mounts — but it also registers health_check via @mcp.custom_route("/health", ...) on its own local mcp.
• The global mcp has no /health route, so the production path (main() → mcp.run()) silently drops the health check that existed before this PR.

This is a regression. Either collapse everything into create_server() and have main() call it, or move the health check registration to module level on the global mcp.

client_secret stored as plain str (OidcSettings)

client_secret: str | None = None

Use pydantic.SecretStr to prevent the secret from appearing in repr(), log output, or serialized model dumps:

from pydantic import SecretStr

client_secret: SecretStr | None = None

Then pass oidc_settings.client_secret.get_secret_value() to OIDCProxy.

---

Architecture / Conventions

OidcSettings belongs in settings.py

CLAUDE.md is explicit: "Configuration centralized in csv_session.py [settings]" and "All environment variables use DATABEAK_ prefix." Defining a second BaseSettings subclass inside server.py fragments that contract. Move OidcSettings into core/settings.py, or fold these four fields into DatabeakSettings with a dedicated group.

Module-level side effects fire before logging is configured

# server.py — runs at import time
oidc_settings = OidcSettings()
...
logger.error("Incomplete OIDC configuration. ...")  # before setup_structured_logging()

main() calls setup_structured_logging(args.log_level) after import, so this error line goes through an unconfigured handler. In __init__.py, get_settings() is called twice at import time:

logging.getLogger("databeak").setLevel(get_settings().log_level)
logging.getLogger("mcp").setLevel(get_settings().log_level)

Cache the result in one call. More broadly, configuring loggers as a module-level import side effect will affect any test that imports the package — consider moving this into main() or a dedicated configure_logging() called explicitly.

---

Testing

test_no_data_loaded leaks a session

The new test creates a session with get_or_create_session() but never removes it. Other tests in this file use the session fixture pattern with teardown. Add cleanup:

async def test_no_data_loaded(self) -> None:
    no_data_session_id = str(uuid.uuid4())
    manager = get_session_manager()
    manager.get_or_create_session(no_data_session_id)
    try:
        ctx = create_mock_context(no_data_session_id)
        with pytest.raises(NoDataLoadedError):
            await detect_outliers(ctx)

        ctx = create_mock_context(no_data_session_id)
        with pytest.raises(NoDataLoadedError):
            await profile_data(ctx)
    finally:
        await manager.remove_session(no_data_session_id)

The _session assignment is also unnecessary since the return value isn't used.

---

Minor

any([...]) with a list literal — pass a generator instead to avoid constructing the list:

elif any(
    v is not None for v in (
        oidc_settings.config_url,
        oidc_settings.client_id,
        oidc_settings.client_secret,
        oidc_settings.base_url,
    )
):

# type: ignore[arg-type] without explanation (mcp.run(**run_args)) — the comment form doesn't explain why the suppression is needed. If run_args is dict[str, str | int] but run() expects narrower types, consider a typed TypedDict or cast at the call site with a brief comment.

fastmcp>=3.2.0 in .pre-commit-config.yaml — no corresponding change is visible in pyproject.toml. Verify the runtime dependency floor is also updated there, otherwise CI can pass pre-commit with one version while a user installs an older one.

SettingsConfigDict without case_sensitive — the original DatabeakSettings explicitly set "case_sensitive": False. The new OidcSettings omits it. Pydantic-settings defaults to case-insensitive for env vars on Linux, but explicit is better than implicit, especially for cross-platform consistency.

---

Documentation

The docs changes are clean and welcome — the PyPI simplification is a clear improvement. The OIDC section in docs/installation.md correctly notes that all four variables are required. One small addition worth considering: a note that DATABEAK_OIDC_CLIENT_SECRET should come from a secrets manager or env file, not be hardcoded in shell scripts.

---

Summary

Severity	Issue
🔴 Critical	Health check endpoint missing from production path (global mcp regression)
🔴 Critical	client_secret should be SecretStr
🟡 Medium	OidcSettings should live in core/settings.py
🟡 Medium	Module-level logging side effects fire before logging is configured
🟡 Medium	Test session leak in test_no_data_loaded
🟢 Minor	any([...]) → generator expression
🟢 Minor	type: ignore missing rationale
🟢 Minor	fastmcp version floor not updated in pyproject.toml