Dependabot security fixes and integration test corrections

[jonpspri]

Summary

Addresses 36 Dependabot security vulnerabilities and fixes failing integration tests.

Security Fixes

Dependabot identified 36 vulnerabilities on the default branch:

• 2 critical
• 15 high
• 17 moderate
• 2 low

Review at: https://github.com/jonpspri/databeak/security/dependabot

Test Fixes

Integration Test Corrections (test_relaxed_integer_validation.py)

Fixed 3 failing tests caused by FastMCP's updated error message format. The regex patterns were matching against the old "Input validation error" string, but FastMCP now wraps Pydantic validation errors with "1 validation error for call[get_cell_value]".

Changes:

• test_invalid_string_integer_rejected: "Input validation error" → "validation error"
• test_fractional_float_rejected: "Input validation error" → "validation error"
• test_empty_string_rejected: "Input validation error" → "validation error"

Root cause: FastMCP wraps Pydantic ValidationError in ToolError with a different message format than previously expected.

Testing

• ✅ All integration tests pass (10/10 in test_relaxed_integer_validation.py)
• ✅ All unit tests pass
• ✅ Quality checks pass (ruff, mypy, MCP docs)
• ✅ Security tests pass

Checklist

• All CI checks passing
• Integration tests corrected
• No new dependencies added
• Backward compatible changes only

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.74%. Comparing base (395addc) to head (3b59efd).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #137   +/-   ##
=======================================
  Coverage   84.74%   84.74%           
=======================================
  Files          27       27           
  Lines        3080     3081    +1     
  Branches      458      458           
=======================================
+ Hits         2610     2611    +1     
  Misses        356      356           
  Partials      114      114           

Flag	Coverage Δ
unittests	84.74% <100.00%> (+<0.01%)	⬆️

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

Files with missing lines	Coverage Δ
src/databeak/servers/discovery_server.py	88.57% <100.00%> (ø)
src/databeak/utils/secure_evaluator.py	88.36% <100.00%> (+0.05%)	⬆️

---

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 395addc...3b59efd. 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: Dependabot security fixes

Overview

This PR addresses 36 security vulnerabilities (2 critical, 15 high, 17 moderate, 2 low) by updating pyproject.toml and uv.lock. The security motivation is clear and the approach is reasonable overall, but a few dependency choices warrant discussion.

---

⚠️ Issues Worth Discussing

1. ansible>=13.3.0 in dev dependencies

Ansible is an unusually heavy dev dependency for a Python CSV processing tool — the package is ~55MB and brings in ansible-core, jinja2, resolvelib, pyyaml, and others. It substantially increases dev environment setup time and expands the attack surface for dev tooling.

The PR description doesn't explain why Ansible is needed. If it's only for a one-off infrastructure task, it shouldn't be a permanent dev dependency. If it's needed for CI/CD automation, that's typically managed externally rather than in the project's dependency group.

2. fastmcp major version bump: 2.12.4 → 3.2.0

This is a major version jump that likely carries breaking API changes. The PR description says "all tests pass," which is reassuring, but a major bump on a core dependency deserves explicit callout of what changed and confirmation that the full surface area (not just unit tests) was exercised. This concern is somewhat shared with PR #134 which also targets fastmcp 3.x — the two PRs may need to be coordinated.

3. Transient dependency pinning in [dependencies] (production)

The # Transient pins block adds runtime dependencies that are actually transitive (brought in by fastmcp/authlib):

"python-multipart>=0.0.26",  # fastmcp
"authlib>=1.6.11",           # fastmcp
"urllib3>=2.6.3",
"cryptography>=46.0.7",
...

While pinning transitive deps is a valid security workaround when upstream hasn't updated their minimum constraints, these should be treated as temporary — tracked for removal once the upstream packages tighten their own constraints. A follow-up issue or TODO comment would help ensure they don't become permanent fixtures.

The same applies to the dev group pins:

"filelock>=3.20.3",    # pre-commit
"virtualenv>=20.36.1", # pre-commit

---

✅ What's Done Well

• Addressing 36 vulnerabilities is the right priority, especially the 2 critical ones.
• simpleeval>=1.0.5 addresses a known expression evaluation security issue — important given DataBeak evaluates user-provided expressions.
• cryptography>=47.0.0 and authlib>=1.7.0 are meaningful security upgrades.
• pipdeptree is a useful addition for understanding the dependency tree, which is directly relevant to managing transient pins.
• Lock file is fully updated, which is correct.

---

Summary

The security intent is correct and this PR should be merged. The main questions before doing so: (1) Why is ansible needed as a dev dependency? If there's no clear answer, it should be removed. (2) Confirm the fastmcp 3.x API surface is compatible beyond unit tests, especially given the overlap with PR #134. Transient pins are acceptable as a short-term measure but should be tracked for cleanup.

[claude]

Code Review: PR #137 — Dependabot Security Fixes & Integration Test Corrections

Overview

This PR bumps security-vulnerable dependencies and fixes three integration tests whose regex assertions broke against FastMCP's updated error message format. The security intent is sound, but the dependency management approach has some significant issues.

---

Security Fixes

FastMCP major version bump (2.12.4 → 3.2.0)

This is a major version change. The PR description mentions 36 Dependabot vulnerabilities but doesn't call out whether the fastmcp 2.x→3.x jump itself introduces breaking changes or whether the test fixes here are the full scope of adaptation needed. Recommend explicitly stating what changed in FastMCP 3.x and confirming all tool behaviours were validated, not just the integration tests.

---

Dependency Management Concerns

ansible>=13.3.0 in dev dependencies ⚠️

This is the most surprising line in the diff. ansible (55 MB wheel, pulls in ansible-core, jinja2, cryptography, pyyaml, resolvelib, …) has no conceivable role in a CSV MCP server's dev tooling. The only plausible explanation is that it was added to force-resolve cryptography to a patched version via Dependabot's transient fix.

This approach is fragile and adds enormous bloat to every developer's environment. Preferred alternatives:

# uv supports dependency overrides natively — pin the transient without polluting the graph
[tool.uv]
override-dependencies = [
    "cryptography>=47.0.0",
]

Or, if constraints are needed: a constraints.txt / uv.override pattern. Adding ansible to solve a cryptography CVE is a maintenance time-bomb.

Transient pins in [project.dependencies]

Pinning python-multipart, authlib, urllib3, cryptography, starlette, python-dotenv, requests, Pygments as direct project dependencies when they are transitive dependencies is a smell. These will appear in DataBeak's public dependency metadata and will confuse users and downstream consumers. The correct mechanism is [tool.uv.override-dependencies] or [tool.uv.constraint-dependencies].

---

Test Fixes

The three regex changes from "Input validation error" → "validation error" are correct and appropriate. The broader match is more resilient to minor FastMCP message formatting changes. No issues here.

Minor: consider anchoring the match slightly (e.g., "validation error for") to avoid accidentally matching unrelated error paths, though the current change is acceptable.

---

Lock File

The uv.lock changes are mechanically correct given the dependency bumps. No issues, but the addition of the full ansible + ansible-core tree in the lock is a concrete illustration of the bloat concern above.

---

Summary

Area	Status
Security intent	✅ Correct
Integration test fixes	✅ Correct
ansible dev dependency	❌ Remove; use uv overrides instead
Transient pins in [project.dependencies]	⚠️ Move to [tool.uv.override-dependencies]
FastMCP 2.x→3.x scope	⚠️ Needs explicit confirmation of full test coverage

The security fixes are necessary; the packaging approach needs revision before merge.

[claude]

Code Review: PR #137 — Dependabot Security Fixes and Integration Test Corrections

Overview

This PR addresses 36 Dependabot vulnerabilities (2 critical, 15 high, 17 moderate, 2 low) primarily through a major FastMCP version bump (2.x → 3.x) and bumps to several other dependencies. It also fixes 3 integration tests whose regex patterns no longer matched FastMCP 3.x error messages, and includes a small type annotation improvement and a dict key type fix.

---

Code Quality and Bug Fixes

discovery_server.py — dict key type fix (line 68)

- record.update(row_data.to_dict())
+ record.update({str(k): v for k, v in row_data.to_dict().items()})

This is a correct and necessary fix. Pandas column names are not guaranteed to be strings (integer indices, MultiIndex, etc.), so coercing to str ensures record stays dict[str, CsvCellValue]. Good catch.

secure_evaluator.py — cast() usage (lines 89, 98)

+ result = cast(pd.Series[Any], result)
  # Ensure result is a pandas Series
  if not isinstance(result, pd.Series):
      ...
- return result  # type: ignore[no-any-return]
+ return result

Replacing # type: ignore with an explicit cast() is the right move. Worth noting that cast() is documentation-only at runtime — the real safety guard is the isinstance check on the following line, which was already there. No concerns here.

---

Test Assertions

test_relaxed_integer_validation.py — error message matching (lines 111, 120, 129)

- with pytest.raises(ToolError, match="Input validation error"):
+ with pytest.raises(ToolError, match="validation error"):

This is necessary to track the FastMCP 3.x change, but the replacement string is very broad. The PR description mentions the new format is "1 validation error for call[get_cell_value]". A more specific match would provide better signal:

with pytest.raises(ToolError, match=r"validation error for call\[get_cell_value\]"):

As written, "validation error" would silently pass if FastMCP changes its format again or if a completely different validation error is raised. The stricter form guards against false negatives.

---

Dependency Management — Primary Concern

Transient dependencies pinned in [project.dependencies]

# Transient pins
"python-multipart>=0.0.26",  # fastmcp
"authlib>=1.6.11", # fastmcp
"urllib3>=2.6.3",
"cryptography>=46.0.7",
"starlette>=0.49.1",
"python-dotenv>=1.2.2",
"requests>=2.33.0",
"Pygments>=2.20.0",

Pinning transient dependencies in [project.dependencies] (not dev) makes them explicit installation requirements for everyone who installs databeak, including downstream users. This bloats the install footprint and creates unnecessary coupling. The standard approach for version security floors in uv is [tool.uv.constraint-dependencies], which enforces minimums without adding them to the public dependency list:

[tool.uv]
constraint-dependencies = [
    "urllib3>=2.6.3",
    "cryptography>=46.0.7",
    ...
]

This achieves the same security goal without polluting the package's public interface. The uv.lock already handles reproducible resolution at install time.

ansible>=13.3.0 as a dev dependency

Ansible is a very large package (~55MB wheel, pulls in ansible-core). The PR doesn't explain why it appears as a dev dependency. If Dependabot flagged ansible as a vulnerable transitive dependency of something else in the dev group, adding the full ansible package as an explicit dev dep is a heavy solution. It would be worth clarifying:

1. What originally pulled ansible in transitively?
2. Is ansible actually used in this project's dev workflows (CI, scripts, etc.)?

If ansible is not directly used, the constraint approach above would be more appropriate.

pipdeptree>=2.35.2 — reasonable diagnostic tool to add. No concerns.

---

FastMCP 2.x → 3.x Major Version Bump

This is the most significant change in the PR. A major version bump introduces risk beyond security fixes. A few things to verify:

• The PR confirms integration tests pass (10/10), but the test changes required to accommodate the version suggest API behavior changed. Are there other FastMCP API surfaces (tool registration, error propagation, context handling) that have changed in 3.x that aren't covered by the existing test suite?
• FastMCP 3.x has significantly more dependencies than 2.x (adds griffelib, jsonref, jsonschema-path, opentelemetry-api, watchfiles, websockets, etc.). This substantially expands the attack surface for future vulnerabilities.

---

Security

No new security concerns introduced by the code changes themselves. The intent is correct — bumping vulnerable dependencies. The constraint-dependency approach would be more idiomatic for the transient pins.

---

Summary

Area	Status
Bug fix (dict keys)	✅ Correct
Type annotation improvement	✅ Correct
Test error message matching	⚠️ Too broad — consider tightening
Transient dep pinning strategy	⚠️ Should use constraint-dependencies, not project.dependencies
ansible as dev dep	❓ Needs justification
FastMCP major bump	✅ Necessary, tests pass

The security intent is sound. The main actionable items are the test assertion specificity and the pinning strategy for transient dependencies.

[claude]

Code Review: PR #137 — Dependabot Security Fixes & Integration Test Corrections

Overview

This PR addresses 36 Dependabot-identified security vulnerabilities by bumping dependency versions and fixing 3 integration tests that broke after a FastMCP upgrade. The intent is solid, but there are a few concerns worth addressing before merge.

---

Security Fixes — Dependency Bumps ✅

The core security work is appropriate:

• fastmcp 2.12.4 → 3.2.0 (major version bump — see concern below)
• simpleeval 1.0.3 → 1.0.5
• cryptography 46.0.1 → 47.0.0, authlib 1.6.4 → 1.7.0, urllib3, starlette, requests, Pygments — all reasonable security updates

The transient pin approach (explicitly pinning indirect deps in pyproject.toml) is non-standard but pragmatic for satisfying Dependabot alerts. The inline comments (# fastmcp, # pre-commit) help communicate intent.

---

Issues

🔴 ansible>=13.3.0 added to dev dependencies

This is a 55MB+ package with no apparent relationship to this project. The uv.lock confirms it pulls in ansible-core and a substantial dependency tree. If this was added to satisfy a Dependabot alert about an indirect dependency, the transient pin approach should be used (as done for filelock and virtualenv), not a full ansible package install.

This should be removed or justified before merge.

🟡 fastmcp 2.x → 3.x is a major version bump

The PR description mentions this is a security fix, but fastmcp>=3.2.0 is a major version change. The integration test fixes demonstrate at least one breaking change (error message format). Were all 40+ MCP tools validated against the new API? The PR checklist says "All CI checks passing" but the major-version upgrade risk warrants explicit mention.

🟡 [project.scripts] entry point may be stale

pyproject.toml still references server = "databeak.server:create_server", but PR #134 (open in parallel) renames this function to create_smithery_server. These two PRs need to be coordinated — merging them in either order without reconciling will break the databeak CLI entry point.

---

Code Quality

secure_evaluator.py — cast(pd.Series, result) ✅

Replacing # type: ignore[no-any-return] with a proper cast() is cleaner and more honest about the type contract.

discovery_server.py — {str(k): v for k, v in row_data.to_dict().items()} ✅

Correct defensive fix. Ensures dict keys are always strings regardless of index type, avoiding type errors downstream.

Integration test fixes ✅

Changing "Input validation error" → "validation error" is the right approach — it matches a stable substring that will survive future message wording changes, making the tests less brittle.

---

Summary

Item	Status
Security dep bumps	✅ Appropriate
ansible in dev deps	🔴 Remove or justify
fastmcp major-version validation	🟡 Needs confirmation
Entry point conflict with PR #134	🟡 Coordinate before merge
Code changes (cast, str keys)	✅ Clean
Test fixes	✅ Correct

The ansible dependency is the blocking concern. The rest is low-risk but worth noting.

---

Review generated by Claude Code

[claude]

Code Review — PR #137: Dependabot security fixes and integration test corrections

Overview

This PR addresses 36 Dependabot security vulnerabilities through a combination of direct dependency upgrades and [tool.uv] constraint-dependencies for transient deps. It also fixes 3 integration tests broken by FastMCP's updated error message format. The overall approach is sound, but a few items warrant attention.

---

✅ What's Done Well

• constraint-dependencies strategy is exactly right for transient security floors — it pins vulnerable indirect deps without polluting the public dependency interface or downstream installs. The inline comment explaining the rationale is useful.
• pipdeptree as dev dependency makes sense as a companion tool for auditing the dependency graph, consistent with the purpose of this PR.
• discovery_server.py key coercion ({str(k): v for k, v in row_data.to_dict().items()}) correctly enforces the dict[str, CsvCellValue] type constraint rather than hoping Pandas always returns string keys.
• secure_evaluator.py removes # type: ignore[no-any-return] in favor of an explicit cast, which is directionally correct given the surrounding isinstance guard.

---

⚠️ Concerns

1. FastMCP 2.x → 3.x is a major version bump

This is the most significant risk in the PR. FastMCP 3.x introduces new dependencies (griffelib, opentelemetry-api, py-key-value-aio, watchfiles, websockets) and drops openapi-core. The lock file shows fastmcp 3.2.4 is resolved. The PR description documents the error-message format change that broke tests, but there's no mention of a broader audit for other breaking API changes. With 40+ MCP tools, any silent behavior change in tool registration, context handling, or error propagation could go undetected.

Recommendation: Add a note in the PR description confirming that the full tool suite was smoke-tested against FastMCP 3.x (e.g., running the integration test suite end-to-end), or open a follow-up issue to track.

---

2. Test assertions are more permissive than necessary

The change from "Input validation error" to "validation error" fixes the immediate failure but is overly broad. The PR description says FastMCP now formats errors as "1 validation error for call[get_cell_value]". The new pattern "validation error" would match any error message containing that substring, including unrelated Pydantic errors from different code paths.

# Current (too broad — any validation error matches)
with pytest.raises(ToolError, match="validation error"):

# Better — anchors to the specific tool that should fail
with pytest.raises(ToolError, match=r"validation error for call\[get_cell_value\]"):

This makes the assertions self-documenting and guards against false positives if another validation path happens to raise first.

---

3. cast() placement in secure_evaluator.py

result = self._evaluator.eval(expression)
result = cast(pd.Series, result)          # ← applied here

if not isinstance(result, pd.Series):     # ← then checked here
    ...

cast is a mypy-only annotation with no runtime effect. Applying it before the isinstance guard is harmless but potentially confusing — a future reader might infer the cast means the value is a Series and skip understanding why the isinstance check still follows. The # type: ignore[no-any-return] it replaces was more honest about what was happening. Consider moving the cast to after the guard, or dropping it and using a typed local instead:

result = self._evaluator.eval(expression)
if not isinstance(result, pd.Series):
    result = pd.Series(result)
return result  # now narrowed by isinstance, no cast needed

---

4. authlib 1.6.4 → 1.7.0 introduces joserfc as a new transitive dep

authlib 1.7 replaced its JWT/JWS internals with joserfc. If this project uses OIDC or JWT validation anywhere (the related PR #134 mentions OIDC authentication), the behavior of token parsing could change subtly. Worth confirming the auth-related code paths have test coverage under the new version.

---

5. Minor: discovery_server.py silent integer-column coercion

The key-stringification fix is correct, but it silently converts integer column names (e.g., from headerless CSVs loaded without header=None → columns 0, 1, 2) to "0", "1", "2". This is probably intentional given the dict[str, CsvCellValue] type, but if any callers downstream compare __row_index__ keys by integer equality, they'll silently get mismatches. Low risk, but worth a quick sanity check against the headerless-CSV handling paths.

---

Summary

The dependency-pinning strategy is solid and the test fixes are correct in direction. The main asks before merge:

1. Confirm FastMCP 3.x integration coverage (full test suite pass).
2. Tighten the ToolError match patterns to reference the specific tool name.
3. Optional: revisit the cast placement for clarity.

The authlib / joserfc item is lower priority but worth tracking if OIDC auth is in scope (PR #134).