Repoint handler tests at the WithSecurity constructors - #396
Open
dpage wants to merge 1 commit into
Open
Conversation
The follow-up to the dead-code removal. NewConnectionHandler and NewNotificationChannelHandler are unreachable from the server binary, because cmd/mcp-server wires both handlers through the WithSecurity variants instead. They survived the mechanical pass because 49 test call sites still used the shorter forms, and deleting them there would have taken out tests covering live handler code. The two constructor pairs are identical apart from one line: the short form sets hostValidator to DefaultHostValidator(), and the WithSecurity form sets it to NewHostValidator(allowInternal, allowedHosts, blockedHosts). Since DefaultHostValidator() is defined as exactly NewHostValidator(false, nil, nil), rewriting a three-argument call to pass an additional (false, nil, nil) is precisely equivalent, and no test changes behaviour as a result. With the short forms gone, DefaultHostValidator had no production caller either, so it goes too. Its two incidental uses in host_validation_test.go now call NewHostValidator(false, nil, nil) directly, and TestDefaultHostValidator is removed along with the function it existed to test. Server coverage is 55.5%, matching main exactly. The only failing tests are the two pre-existing vector(3) fixture failures from #337, which reproduce identically on unmodified main. This branch is cut from main rather than stacked on the dead-code branch, so CI runs against it. The two changesets touch entirely disjoint files and can merge in either order.
WalkthroughThe change removes superseded handler constructors and ChangesAPI constructor migration
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | -6 |
| Duplication | 2 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/src/internal/api/connection_handlers_test.go (1)
29-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the migrated constructor test.
Line 29 now tests
NewConnectionHandlerWithSecurity, but the test name and failure message still refer to the removedNewConnectionHandler. Use a unique name becauseTestNewConnectionHandlerWithSecurityalready exists.Proposed update
-func TestNewConnectionHandler(t *testing.T) { +func TestNewConnectionHandlerWithSecurity_DefaultConfig(t *testing.T) { handler := NewConnectionHandlerWithSecurity(nil, nil, nil, false, nil, nil) if handler == nil { - t.Fatal("NewConnectionHandler returned nil") + t.Fatal("NewConnectionHandlerWithSecurity returned nil")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/internal/api/connection_handlers_test.go` around lines 29 - 31, Rename the migrated constructor test to a unique name that accurately identifies NewConnectionHandlerWithSecurity, and update its nil-check failure message to reference NewConnectionHandlerWithSecurity instead of the removed NewConnectionHandler. Ensure it does not conflict with the existing TestNewConnectionHandlerWithSecurity.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@server/src/internal/api/connection_handlers_test.go`:
- Around line 29-31: Rename the migrated constructor test to a unique name that
accurately identifies NewConnectionHandlerWithSecurity, and update its nil-check
failure message to reference NewConnectionHandlerWithSecurity instead of the
removed NewConnectionHandler. Ensure it does not conflict with the existing
TestNewConnectionHandlerWithSecurity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f7c50f55-c8cf-43f7-83dc-2bb3b6a425dc
📒 Files selected for processing (12)
docs/changelog.mdserver/src/internal/api/connection_handlers.goserver/src/internal/api/connection_handlers_test.goserver/src/internal/api/host_validation.goserver/src/internal/api/host_validation_test.goserver/src/internal/api/issue269_connection_name_test.goserver/src/internal/api/notification_channel_handlers.goserver/src/internal/api/notification_channel_handlers_test.goserver/src/internal/api/query_handlers_test.goserver/src/internal/api/rbac_integration_test.goserver/src/internal/api/rbac_issue233_connections_test.goserver/src/internal/api/rbac_issue35_test.go
💤 Files with no reviewable changes (3)
- server/src/internal/api/host_validation.go
- server/src/internal/api/notification_channel_handlers.go
- server/src/internal/api/connection_handlers.go
This was referenced Aug 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The follow-up to #395, covering the one piece I deliberately left out of
the mechanical pass.
Why this needed its own PR
NewConnectionHandlerandNewNotificationChannelHandlerareunreachable from the server binary:
cmd/mcp-server/handlers.gowiresboth handlers through the
...WithSecurityvariants. They survived #395because 49 test call sites still used the shorter forms, and deleting
the constructors there would have taken out test suites covering live
handler code, which is exactly the failure mode #395 was tightened to
avoid.
Why the rewrite is safe
The two constructor pairs are identical apart from a single line:
hostValidator: DefaultHostValidator()WithSecurity:hostValidator: NewHostValidator(allowInternal, allowedHosts, blockedHosts)and
DefaultHostValidator()is defined as exactly:So rewriting
NewConnectionHandler(a, b, c)toNewConnectionHandlerWithSecurity(a, b, c, false, nil, nil)isprecisely equivalent. No test changes behaviour, and nothing needed
reinterpreting: every field the short constructor set is set identically
by the longer one. That is what makes this mechanical rather than a
judgement call, which is the thing I wasn't willing to assume without
checking.
With the short forms gone,
DefaultHostValidatorhad no productioncaller either, so it goes too. Its two incidental uses in
host_validation_test.go(inTestHostValidator_ValidatePortandTestHostValidator_InternalNetworksList, which use it only as aconvenient default) now call
NewHostValidator(false, nil, nil)directly.
TestDefaultHostValidatoris removed along with the functionit existed to test.
Scope
49 call sites rewritten across 7 test files. No test was deleted except
TestDefaultHostValidator.Verification
gofmtclean,golangci-lintreports 0 issues,go vetclean.-race -p=1. The only failures are thetwo pre-existing
vector(3)fixture failures from Gemini provider: (1) knowledge base search silently falls back to OpenAI due to missing gemini_embedding column in search_knowledgebase.go; (2) session startup fails with 400 "empty Part" error #337, whichreproduce identically on unmodified
main.mainexactly (Remove dead code identified by reachability analysis #395 leaves it at55.3%). Nothing to top up; no new code is added, so the 90% floor for
new and modified code does not apply.
deadcodeconfirms no cascade: the count from the server binary dropsfrom 65 to 62, exactly the three functions removed here, with nothing
newly stranded in the touched area.
Relationship to #395
Cut from
main, not stacked onremove-dead-code, because a branchbased on a non-
mainbranch triggers no CI workflows at all. I verifiedthe two changesets touch entirely disjoint files (52 files in #395,
11 here, zero overlap), so they can merge in either order without
conflict.
Still outstanding after this
The 42 remaining dead functions whose tests need editing rather than
deleting, and the three test helpers (
NewTestClient,NewTestDatastoreWithSecret,NewTestDatastore) that compile into theshipped binaries but are used across package boundaries by tests, so
they want extracting into a test-only package rather than deleting.
Summary by CodeRabbit
Removed
Documentation
Tests