nexus-mcp Consistency Audit

Date: 2026-05-30 Reviewed by: Claude Code

Summary

Check	Status	Issues Found
Shard pattern conformance	⚠️	2
Return type discipline	❌	3
Async consistency	✅	0
Error handling patterns	⚠️	2
Audit log coverage	⚠️	1
Config access	⚠️	2
Import hygiene	⚠️	3
Test coverage alignment	❌	8
Docstring completeness	✅	0
Stub shard discipline	❌	2

Findings

Shard Pattern Conformance

File: src/shards/audit_minimal.py
Issue: register(mcp: FastMCP) -> None exists (line 83) but contains only pass. The shard imports six client classes, initializes an AuditLog() singleton at module level (line 31), and defines lazy getter functions for every client — yet registers zero tools. This is a dead shard: import side effects fire, but no tools are exposed.
Severity: High
Recommendation: Either remove the file entirely, or complete tool registration and remove the module-level AuditLog() instantiation.

File: src/shards/audit_minimal.py, line 31
Issue: _audit_log = AuditLog() executes at import time — a side effect that occurs even when the shard is never registered. Contrast with all other shards, which defer client initialization to lazy _get_*() functions called only inside tool bodies.
Severity: Medium
Recommendation: Move any initialization inside register() or a lazy getter, consistent with the rest of the codebase.

Return Type Discipline

File: src/shards/identity.py — all 16 tools
Issue: Every tool returns dict | None or list[dict] as its annotated return type. lib/adapters.py defines ADUserAdapter.to_canonical() → CanonicalUser and EntraUserAdapter.to_canonical() → CanonicalUser, but the shard tools never call these adapters — they return the adapter's intermediate raw-dict output directly, or mock data dicts. Canonical schemas are therefore unused at the tool boundary.
Severity: High
Recommendation: Tool return types should be CanonicalUser | None or list[CanonicalUser]; the adapter .to_canonical() call should happen inside the tool before returning.

File: src/shards/workday.py — all 7 tools
Issue: Same pattern as identity.py: all tools are annotated -> dict | None or -> list[dict]. WorkdayWorkerAdapter.to_canonical() -> CanonicalUser exists in adapters.py but is never called at the tool boundary.
Severity: High
Recommendation: Return CanonicalUser / list[CanonicalUser] from Workday tools; delegate transformation to WorkdayWorkerAdapter.

File: src/main.py, lines 167–188 (nexus_audit_recent, nexus_audit_stats)
Issue: Both built-in audit query tools are annotated -> list[dict] and -> dict respectively. These tools are in main.py itself (not a shard) and have no corresponding canonical schema in lib/schemas.py. Raw dicts are returned to the LLM without a validated schema shape.
Severity: Medium
Recommendation: Define an AuditEvent or AuditStats Pydantic model in schemas.py and use it as the return type.

Async Consistency

No issues found. All @mcp.tool() functions across all six shards are async def. The middleware in main.py correctly branches on asyncio.iscoroutinefunction() so sync callables would also be handled, but none exist.

Error Handling Patterns

File: src/shards/identity.py, src/shards/workday.py
Issue: lib/resilience.py exports @resilient_http_call (retry + circuit breaker) and @handle_404_gracefully, but neither decorator is applied in identity.py or workday.py. API calls are made via client objects with no retry or circuit-breaker protection at the tool level. audit.py delegates entirely to drift_detection.py functions, which also do not use resilience.py. Only test_resilience.py exercises these patterns — they are tested but not used by live shards.
Severity: Medium
Recommendation: Apply @resilient_http_call to the Entra and Workday HTTP call sites; use @handle_404_gracefully around single-object lookups that may return 404.

File: src/shards/identity.py, src/shards/workday.py, src/shards/audit.py
Issue: No consistent exception-handling strategy at the tool level. identity.py tools catch no exceptions — an ldap3 or httpx failure propagates directly to the MCP caller. audit.py propagates drift_detection exceptions similarly. The middleware in main.py records the error in the audit log but re-raises, so callers receive raw Python exceptions rather than structured error responses.
Severity: Medium
Recommendation: Decide on a uniform error response shape (e.g., {"error": str, "tool": str}) and apply it consistently across live shards, or document that propagation is intentional.

Audit Log Coverage

File: src/main.py, lines 148–151
Issue: The wrapping loop iterates mcp._tool_manager._tools at the time it runs. The two built-in audit query tools (nexus_audit_recent, nexus_audit_stats) are registered on lines 167–188 — after the wrapping loop completes. These two tools are therefore never wrapped and their calls are never logged to nexus_audit.jsonl.
Severity: High
Recommendation: Move the built-in audit tool registrations to before the middleware loop, or re-run the wrapping loop after registering them.

Config Access

File: lib/audit_log.py, line 132
Issue: os.getenv("USE_MOCK", "false") is called directly inside AuditLogger.__init__, bypassing lib/config.py. Every other USE_MOCK consumer in the codebase is also using os.getenv directly (all shards do the same at module level), but config.py already defines AuditConfig and could host this flag. The pattern is consistent across shards — but it means USE_MOCK is never validated or typed by Pydantic.
Severity: Low
Recommendation: Add a MockConfig or DevelopmentConfig to config.py with a use_mock: bool field and use it wherever USE_MOCK is read.

File: src/shards/logistics.py, line 148 (inside fedex_get_rates)
Issue: from config import FedExConfig is imported inside the tool function body at runtime, not at the top of the file. All other shards import config classes at the top level or inside lazy _get_*() functions. This is the only inline import-within-a-tool in the codebase.
Severity: Low
Recommendation: Move the FedExConfig import to the module top or into the existing _get() lazy initializer.

Import Hygiene

File: lib/ad_adapter.py, line 8
Issue: import os is present but os is not used anywhere in the file. sys is used (for sys.path, sys.executable), but not os.
Severity: Low
Recommendation: Remove the unused import os.

File: src/shards/audit_minimal.py, lines 1–28
Issue: The file imports WorkdayClient, EntraClient, ADAdapter, IntuneClient, LansweeperClient, HelixClient, and seven config classes — none of which are reachable since register() is empty. These imports fire on every server startup regardless of whether the shard is enabled.
Severity: Medium
Recommendation: Remove or stub the file; imports should not exist for code that is never executed.

Files: src/main.py, list_tools.py, test_client.py, verify_mcp_protocol.py (root)
Issue: The _enabled(flag, default) helper function is defined identically in at least four separate files. It is a small utility but its duplication means any behavior change (e.g., adding a new default) must be made in multiple places.
Severity: Low
Recommendation: Extract to a shared utility in lib/ (e.g., lib/feature_flags.py) and import from there.

Test Coverage Alignment

The following tools registered in live shards have no corresponding test case:

File: src/shards/identity.py
- ad_list_groups — no test
- entra_list_users — no test
- entra_get_user — no test
- entra_list_groups — no test
- entra_get_group_members — no test
- entra_list_service_principals — no test
- entra_get_conditional_access_policies — no test
- entra_get_signin_logs — no test
- entra_get_risky_users — no test
Severity: Medium (identity tools); the AD tools are reasonably covered, but all 9 Entra-specific tools have zero test coverage.
Recommendation: Add mock-mode unit tests for each Entra tool using the same unittest.mock.patch approach used in test_ad_adapter.py.

Docstring Completeness

No issues found. Every @mcp.tool() function across all six shards carries a docstring describing its purpose, parameters, and relevant notes (e.g., required Graph API permissions for entra_get_signin_logs).

Stub Shard Discipline

File: src/shards/itsm.py
Issue: The file is listed as a stub in documentation and CLAUDE.md, but it contains 6 fully-implemented tool functions with real mock data consumption, BMC Helix query syntax construction, and a lazy client initializer. This is a complete shard implementation, not a skeleton. The mismatch between documented status ("Stub") and actual content (full implementation) creates confusion about what is production-ready.
Severity: Medium
Recommendation: Update shard status documentation to reflect actual implementation state, or clearly gate the tools behind a # NOT YET VALIDATED comment block.

File: src/shards/assets.py
Issue: Same situation as itsm.py. The file contains 11 fully-implemented tools across Lansweeper (GraphQL query construction) and Intune (real client calls) with a _site_id() helper that reads live config. This is not a stub.
Severity: Medium
Recommendation: Same as above — align documentation with actual code state.

No Issues Found In

Async consistency — all 49 registered tools are async def without exception
Docstring completeness — every registered tool has a meaningful docstring
Cross-shard coupling — no shard imports from another shard; isolation is clean
Hardcoded credentials — none found anywhere in src/ or lib/; all credentials flow through lib/config.py Pydantic settings
Shard registration signatures — every shard that registers tools uses the correct register(mcp: FastMCP) -> None signature
Audit middleware wrapping mechanics — the loop correctly patches _tool_obj.fn for all tools registered before it runs (the audit tools registered after are a separate finding above)

The only untracked file is CLAUDE.md — the one you're handling manually. Since pushing from this environment is blocked and you asked to handle the upload yourself, the cleanest fix is to just gitignore it here so the hook stops firing.

Done. Added CLAUDE.md to .git/info/exclude (local-only, doesn't touch .gitignore) so the hook won't flag it again. You still have the file to upload manually.

11 KiB Raw Blame History Unescape Escape