Skip to content

fix: intercept GET /mcp in session registry + request logger (#178)#233

Merged
cmeans-claude-dev[bot] merged 3 commits into
mainfrom
fix/session-get-intercept
Apr 9, 2026
Merged

fix: intercept GET /mcp in session registry + request logger (#178)#233
cmeans-claude-dev[bot] merged 3 commits into
mainfrom
fix/session-get-intercept

Conversation

@cmeans-claude-dev
Copy link
Copy Markdown
Contributor

@cmeans-claude-dev cmeans-claude-dev Bot commented Apr 9, 2026

Summary

  • Session registry now intercepts GET /mcp (SSE reconnect) with session IDs — previously only POST/DELETE were handled, causing stale GET requests to return 409 directly from FastMCP
  • New McpRequestLogger middleware logs method, truncated session ID, client IP, and response status for every /mcp request
  • 8 new tests: 4 for GET intercept (known session, no session, unknown session, owner mismatch) + 4 for request logger (POST with/without session, GET, non-MCP path skip)

Closes #178

QA

Prerequisites

  • pip install -e ".[dev]"
  • Deploy to test instance on alternate port (AWARENESS_PORT=8421)

Manual tests (via MCP tools)

    • Request logger output visible — MCP POST /mcp lines with session ID and status in logs
    • GET with stale session intercepted — middleware routes through _handle_subsequent (confirmed via log)
    • GET without session passes through — returns 400 from FastMCP (no init handshake)
    • Non-MCP paths not logged — /health produces zero MCP log lines

🤖 Generated with Claude Code

Session registry now handles GET /mcp with session IDs — previously
only POST/DELETE were intercepted, causing stale SSE reconnects to
bypass re-initialization and get 409 from FastMCP directly.

New McpRequestLogger middleware logs method, truncated session ID,
client IP, and response status for every /mcp request. Placed outside
the session registry for full visibility.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@cmeans-claude-dev cmeans-claude-dev Bot added bug Something isn't working enhancement New feature or request Dev Active Developer is actively working on this PR; QA should not start labels Apr 9, 2026
@github-actions github-actions Bot added the Awaiting CI Dev complete, waiting for CI/Codecov to pass before QA label Apr 9, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 9, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@cmeans-claude-dev cmeans-claude-dev Bot added Ready for QA Dev work complete — QA can begin review and removed Dev Active Developer is actively working on this PR; QA should not start labels Apr 9, 2026
@github-actions github-actions Bot removed Awaiting CI Dev complete, waiting for CI/Codecov to pass before QA labels Apr 9, 2026
The exact count goes stale on every PR and adds maintenance burden.
CI badge and coverage reports are the authoritative source.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added Awaiting CI Dev complete, waiting for CI/Codecov to pass before QA and removed Ready for QA Dev work complete — QA can begin review labels Apr 9, 2026
@cmeans-claude-dev
Copy link
Copy Markdown
Contributor Author

Additional commit (f9a60eb): Removed exact test count from README. The count went stale on every PR and QA flagged it multiple times this session. CI badge and Codecov reports are the authoritative source — no more manual maintenance.

@github-actions github-actions Bot added Ready for QA Dev work complete — QA can begin review and removed Awaiting CI Dev complete, waiting for CI/Codecov to pass before QA labels Apr 9, 2026
@cmeans cmeans added QA Active QA is actively reviewing; Dev should not push changes and removed Ready for QA Dev work complete — QA can begin review labels Apr 9, 2026
@cmeans
Copy link
Copy Markdown
Owner

cmeans commented Apr 9, 2026

Adding QA Active — reviewing GET /mcp intercept fix.

Copy link
Copy Markdown
Owner

@cmeans cmeans left a comment

Choose a reason for hiding this comment

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

QA Review — Round 1

Code review

Session registry GET intercept — minimal, targeted fix. Two lines changed in session_registry.py: added "GET" to the method filter and to the _handle_subsequent routing. GET requests with session IDs now go through the same lookup → validate owner → pass through / re-init path as POST requests.

McpRequestLogger — clean ASGI middleware. Logs method, truncated session ID (12 chars + ellipsis), client IP, and response status. Placed outside the session registry (between session registry and SecretPathMiddleware) so it captures both intercepted and pass-through requests. Only logs /mcp requests — health checks and other paths skip.

README — replaced specific test count with "Comprehensive test suite". Prevents recurring test count drift.

Check Result
GET now routed through _handle_subsequent
GET without session passes through (no interception)
Logger truncates session IDs > 12 chars
Logger skips non-MCP paths
Logger placed outside session registry in both MOUNT_PATH and plain transport paths
CHANGELOG: both entries (Added + Fixed)
README: "Comprehensive test suite" replaces count
763/763 full suite pass
CI all green

Manual test results (Docker QA instance)

# Test Result
1 Request logger output MCP POST /mcp session=none/... visible in logs
2 GET with stale session intercepted ✅ Middleware intercepts, routes through _handle_subsequent
3 GET without session passes through ✅ Returns 400 from FastMCP (no handshake)
4 Non-MCP paths not logged ✅ Zero MCP log lines for /health

Test sufficiency

The 4 GET intercept tests cover the key cases: known session (pass through), no session (pass through), unknown session (404 from FastMCP), and owner mismatch (403). The 4 logger tests cover POST with/without session, GET, non-MCP skip, and non-HTTP scope. Solid coverage for both features.

Zero findings. Verdict: Pass — ready for signoff.

@cmeans cmeans added Ready for QA Signoff QA passed — ready for maintainer final review and merge and removed QA Active QA is actively reviewing; Dev should not push changes labels Apr 9, 2026
@cmeans
Copy link
Copy Markdown
Owner

cmeans commented Apr 9, 2026

Applying Ready for QA Signoff — GET intercept and request logger both verified on Docker QA instance, 763/763 tests pass, CI green, zero findings.

Copy link
Copy Markdown
Owner

@cmeans cmeans left a comment

Choose a reason for hiding this comment

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

LGTM

@cmeans cmeans added QA Approved Manual QA testing completed and passed and removed Ready for QA Signoff QA passed — ready for maintainer final review and merge labels Apr 9, 2026
@cmeans-claude-dev cmeans-claude-dev Bot merged commit 4ccf925 into main Apr 9, 2026
35 checks passed
@cmeans-claude-dev cmeans-claude-dev Bot deleted the fix/session-get-intercept branch April 9, 2026 22:47
cmeans pushed a commit that referenced this pull request Apr 10, 2026
6 bug-fix PRs since v0.16.1:
- #226 LazyStore thread safety (#164)
- #227 SQL template injection hardening (#165)
- #232 Stateless HTTP mode (#180)
- #233 GET /mcp intercept + request logger (#178)
- #234 delete_entry IDOR fix (#193)
- #236 RLS-safe opt-in cleanup (#179, #183)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@cmeans-claude-dev cmeans-claude-dev Bot mentioned this pull request Apr 10, 2026
3 tasks
cmeans-claude-dev Bot added a commit that referenced this pull request Apr 10, 2026
6 bug-fix PRs since v0.16.1:
- #226 LazyStore thread safety (#164)
- #227 SQL template injection hardening (#165)
- #232 Stateless HTTP mode (#180)
- #233 GET /mcp intercept + request logger (#178)
- #234 delete_entry IDOR fix (#193)
- #236 RLS-safe opt-in cleanup (#179, #183)

Co-authored-by: cmeans-claude-dev[bot] <3223881+cmeans-claude-dev[bot]@users.noreply.github.com>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request QA Approved Manual QA testing completed and passed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test: end-to-end rolling deploy session recovery

1 participant