Skip to content

Conversation

@therealnb
Copy link
Collaborator

Summary

This PR introduces three improvements to the mcp-optimizer project:

1. Better logging for validation errors

  • Added comprehensive debug and info logging throughout the MCP client
  • Logs now include workload names for better traceability
  • Added logging for session establishment, initialization, and tool operations
  • Helps with debugging connection issues and monitoring tool calls

2. Build multi-arch locally

  • Added support for building multi-architecture container images (linux/amd64, linux/arm64)
  • Created separate build task for multi-arch builds using docker buildx
  • Created build-local task for faster single-arch development builds
  • Updated run-container task to use the faster local build for development

3. Add labels and provenance to builds

  • Added OCI container image labels (title, description, vendor, source, version, revision, created)
  • Enabled provenance attestation for supply chain security
  • Enabled SBOM (Software Bill of Materials) generation
  • Improves container image metadata and security posture

Changes

  • src/mcp_optimizer/mcp_client.py: Enhanced logging throughout MCP client operations
  • Taskfile.yml: Added multi-arch build support with docker buildx
  • .github/workflows/release.yml: Added OCI labels and enabled provenance/SBOM

@claude
Copy link

claude bot commented Oct 23, 2025

PR Review

Summary

Overall solid improvements to logging, multi-arch builds, and container metadata. The changes are well-structured and align with project goals.

Code Quality & Best Practices ✅

Logging improvements (mcp_client.py):

  • Good: Structured logging with workload context
  • Good: Consistent use of f-strings with workload names for traceability
  • Minor: Some redundancy in logging - both structured fields AND f-string interpolation used (e.g., lines 221-222, 243-246). Consider using only structured fields:
    logger.debug("Listing tools for workload", workload=self.workload.name)
    Instead of:
    logger.debug(f"Listing tools for workload '{self.workload.name}'", workload=self.workload.name)

Taskfile changes:

  • Good: Separation of build (multi-arch) and build-local (fast dev builds)
  • Issue: build task uses --load flag which cannot load multi-platform images. Use --push or remove --load for multi-arch builds
  • Good: Cleanup of old containers before running new ones

Release workflow:

  • Good: OCI labels follow standard conventions
  • Good: Provenance and SBOM enabled for supply chain security

Potential Issues ⚠️

  1. Taskfile.yml line 81: --load flag doesn't work with multi-platform builds. You'll get error: "multi-platform images cannot be loaded". Either:

    • Remove --load (images stay in build cache)
    • Use --push with a registry
    • Or use --platform linux/amd64 --load for single-arch
  2. Logging verbosity: 49 new lines of logging may be excessive. Consider:

    • Moving session establishment logs from INFO→DEBUG (lines 183, 203)
    • Removing duplicate "initialized successfully" logs (already covered by INFO level)

Performance Considerations 🚀

  • Multi-arch builds will be slower (expected), good that build-local exists for dev
  • Logging overhead minimal for async operations
  • No performance regressions expected

Security 🔒

  • ✅ Provenance attestation enabled
  • ✅ SBOM generation enabled
  • ✅ OCI labels include source and revision tracking
  • No security concerns

Breaking Changes

None. All changes are additive or internal improvements.

Recommendations

  1. Fix --load issue in Taskfile.yml:81
  2. Reduce logging verbosity by ~20 lines (use structured fields only, reduce duplicate messages)
  3. Consider making session initialization logs DEBUG-level instead of INFO

@therealnb
Copy link
Collaborator Author

The everything server was giving lots of validation errors, but it was hard to see where they came from.

Copy link
Member

@aponcedeleonch aponcedeleonch left a comment

Choose a reason for hiding this comment

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

looks good and I think there's a bug in the action that is preventing the integration tests to run

@therealnb therealnb merged commit 64f73fd into main Oct 23, 2025
6 of 8 checks passed
@therealnb therealnb deleted the testing-minor-fixes branch October 23, 2025 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants