Add Lumen support and integration tests

[kargnas]

Summary

• add Lumen-aware configuration and routing logic to the MCP server service provider
• introduce dedicated Lumen test harness and route registration tests
• document Lumen setup steps and adjust dev dependencies for Lumen support

Testing

• vendor/bin/pest

Issue #69

---

https://chatgpt.com/codex/tasks/task_e_68eab737ce7483219b8167f43ece8b89

[claude]

Pull Request Review: Add Lumen Support and Integration Tests

Thank you for adding Lumen support to the package! This is a valuable addition that expands the framework compatibility. Here's my detailed review:

---

✅ Strengths

1. Well-structured implementation: The Lumen support is cleanly separated with dedicated test infrastructure
2. Comprehensive testing: Good test coverage for route registration with different configurations
3. Framework detection logic: Smart use of instanceof checks to detect Lumen vs Laravel
4. Documentation: Clear Lumen setup instructions added to README.md
5. Backward compatibility: No breaking changes to existing Laravel functionality

---

🔍 Code Quality Issues

1. Service Provider - Missing Facade Import (src/LaravelMcpServerServiceProvider.php:5)

The Route facade is still imported but not used for Lumen routing:

use Illuminate\Support\Facades\Route; // Line 5 - Only used for Laravel

Issue: This could cause confusion. When Lumen routing is detected, the facade isn't needed.

Recommendation: Consider conditionally using the facade or document why it's needed:

// If you need it for Laravel only, add a comment explaining the dual usage
use Illuminate\Support\Facades\Route; // Used for Laravel routing (not Lumen)

2. Configuration Method - Potential Issues (src/LaravelMcpServerServiceProvider.php:155-162)

The registerConfiguration() method has a subtle issue:

protected function registerConfiguration(): void
{
    if ($this->isLumenApplication() && \! $this->app['config']->has('mcp-server')) {
        $this->app->configure('mcp-server');
    }

    $this->mergeConfigFrom(__DIR__.'/../config/mcp-server.php', 'mcp-server');
}

Issue: The Lumen configure() call happens before mergeConfigFrom(), but configure() expects the file to exist at config/mcp-server.php. If the user hasn't copied the config file (as mentioned in README step 3), this will silently fail or use defaults.

Recommendation: Either:

• Check if the config file exists before calling configure()
• Document that configure() is optional for Lumen (if defaults are acceptable)
• Reverse the order (merge first, then configure if needed)

protected function registerConfiguration(): void
{
    $this->mergeConfigFrom(__DIR__.'/../config/mcp-server.php', 'mcp-server');
    
    // For Lumen: If user has a custom config file, load it
    if ($this->isLumenApplication() && file_exists($this->app->basePath('config/mcp-server.php'))) {
        $this->app->configure('mcp-server');
    }
}

3. Type Hints Missing (src/LaravelMcpServerServiceProvider.php:164, 183)

Methods lack return type hints:

protected function getConfig(string $key, $default = null) // Line 164
protected function registerLumenRoutes($router, ?string $domain, string $path, array $middlewares, string $provider): void // Line 183

Recommendation:

protected function getConfig(string $key, mixed $default = null): mixed

protected function registerLumenRoutes(
    \Laravel\Lumen\Routing\Router $router,
    ?string $domain,
    string $path,
    array $middlewares,
    string $provider
): void

4. PHPStan Concern

Given the project uses PHPStan level 5, the Lumen class checks might trigger warnings:

protected function isLumenApplication(): bool
{
    return class_exists(\Laravel\Lumen\Application::class) && $this->app instanceof \Laravel\Lumen\Application;
}

Recommendation: Add PHPStan ignore comments if needed, or verify these don't cause issues:

/** @phpstan-ignore-next-line Lumen support is optional */
protected function isLumenApplication(): bool

---

🐛 Potential Bugs

1. Default Case in Switch (src/LaravelMcpServerServiceProvider.php:202-206)

In registerLumenRoutes(), the default case includes 'streamable_http':

case 'streamable_http':
default:
    $router->get($path, [StreamableHttpController::class, 'getHandle']);
    $router->post($path, [StreamableHttpController::class, 'postHandle']);
    break;

But in registerRoutesForDomain() (line 148-151), there's no default case, so invalid provider values silently fail.

Recommendation: Add consistent default handling:

case 'streamable_http':
default:
    $routeRegistrar->get($path, [StreamableHttpController::class, 'getHandle']);
    $routeRegistrar->post($path, [StreamableHttpController::class, 'postHandle']);
    break;

2. Router Instance Check (src/LaravelMcpServerServiceProvider.php:125)

The code makes the router directly:

$router = $this->app->make('router');

Concern: In some edge cases, the 'router' binding might not exist or return an unexpected type.

Recommendation: Add defensive check:

if (\!$this->app->bound('router')) {
    return;
}
$router = $this->app->make('router');

---

🧪 Test Coverage Issues

1. Missing Test Scenarios

The Lumen tests cover basic cases but miss:

• ✗ Multiple domain support for Lumen (similar to Laravel test line 68-88)
• ✗ Middleware application verification (similar to Laravel test line 147-164)
• ✗ Invalid domain configuration handling

Recommendation: Add these test cases:

it('registers routes with multiple domains in lumen', function () {
    $this->app['config']->set('mcp-server.domain', ['api.example.com', 'admin.example.com']);
    // ... test multiple domains
});

it('applies middlewares to lumen routes', function () {
    $this->app['config']->set('mcp-server.middlewares', ['auth:api', 'throttle:60,1']);
    // ... verify middleware attachment
});

2. Test Helper Function Duplication (tests/Lumen/LumenRouteRegistrationTest.php:16-43)

The lumenRegisteredRoutes() function has similar logic to the Laravel version but handles route structures differently.

Recommendation: Consider extracting to a shared trait/helper class to reduce duplication and maintain consistency.

---

⚡ Performance Considerations

1. Router Resolution (src/LaravelMcpServerServiceProvider.php:125)

The router is resolved from the container on every registerRoutesForDomain() call. For multiple domains, this is inefficient.

Recommendation: Resolve once before the loop:

protected function registerRoutes(): void
{
    // ... existing checks ...
    
    $router = $this->app->make('router');
    $domains = $this->normalizeDomains($domain);
    
    foreach ($domains as $domainName) {
        $this->registerRoutesForDomain($router, $domainName, $path, $middlewares, $provider);
    }
}

protected function registerRoutesForDomain($router, ?string $domain, ...): void

---

🔐 Security Concerns

No significant security issues identified. The implementation maintains the same security posture as the Laravel version.

---

📚 Documentation Issues

1. README.md Ordering (README.md:288-313)

The Lumen setup section appears right after requirements but before "Basic Usage".

Recommendation: Consider adding a table of contents or separating into "Laravel Setup" and "Lumen Setup" sections more clearly.

2. Incomplete Instructions (README.md:313)

The note states:

"If you skip steps 3-4 the package will still run with the default configuration"

Issue: This isn't entirely clear because of the configure() call issue mentioned earlier.

Recommendation: Clarify:

(Steps 3-4 are optional. If you skip them, the package will use the default configuration. To customize the configuration, you must complete both steps.)

---

📦 Composer Changes

1. Orchestra Testbench Version Constraint (composer.json:20)

Changed from:

"orchestra/testbench": "^10.0.0||^9.0.0||^8.22.0"

To:

"orchestra/testbench": "^9.0.0||^8.22.0"

Question: Why was ^10.0.0 removed? Is there an incompatibility with Lumen?

Recommendation: Add a comment in the commit message or PR description explaining this change.

---

✨ Recommendations Summary

High Priority:

1. Fix the configuration loading order/file check issue
2. Add consistent default case handling in both routing methods
3. Add missing test coverage (multiple domains, middleware verification)
4. Clarify README documentation about optional config steps

Medium Priority:
5. Add return type hints for better type safety
6. Optimize router resolution for multiple domains
7. Add defensive check for router binding
8. Extract common test helper logic

Low Priority:
9. Consider adding PHPStan ignore comments if needed
10. Improve README structure and navigation

---

🎯 Overall Assessment

Quality Score: 7.5/10

This is a solid implementation that adds valuable Lumen support without breaking existing functionality. The main concerns are around:

• Configuration loading edge cases
• Missing test coverage for some scenarios
• Some minor type safety and performance optimizations

The code follows the repository's conventions and CLAUDE.md guidelines well. With the recommended fixes, this would be an excellent addition to the package.

Recommendation: Approve with requested changes ✅

---

Great work on this feature! Let me know if you'd like me to clarify any of these points or help implement the suggested changes.