mathias/hyperguild
1
0
Fork 0
Code Issues 30 Pull Requests 3 Actions Packages Projects Releases Wiki Activity

Test that /mcp authenticates through the production router; close the re-wrap gap in main.go #154

New Issue
Closed
opened 2026-10-03 14:35:26 +00:00 by mathias · 3 comments
mathias commented 2026-10-03 14:35:26 +00:00
Owner
Copy Link
Copy Source

Follow-up from the PR #152 round-2 review, 2026-10-03.

  1. /mcp auth is untested in the production wiring. publicRoutes says each public route "authenticates itself", but the bearer check on /mcp is built in main.go and the buildMux tests use stubs. This predates #152. Move the /mcp handler construction into a tested function, or build /mcp's bearer inside buildMux the same way the REST auth is, and assert that /mcp with no token gets 401 through that path.
  2. Re-wrap gap. buildMux returns an http.Handler, so nothing can add routes to its result. But main.go could still wrap that result in a new mux with a public POST /query between buildMux and ListenAndServe, and CI would stay green. One untested line of wiring remains. Either accept it and make the comments in routes_test.go/main.go claim only that, or move the ListenAndServe handler construction into a tested function.

Done when both are tested or the comments are narrowed, with a flip test showing the gap is covered.

Created via git-mcp on behalf of @mathias

Follow-up from the PR #152 round-2 review, 2026-10-03. 1. **`/mcp` auth is untested in the production wiring.** `publicRoutes` says each public route "authenticates itself", but the bearer check on `/mcp` is built in `main.go` and the buildMux tests use stubs. This predates #152. Move the `/mcp` handler construction into a tested function, or build `/mcp`'s bearer inside buildMux the same way the REST auth is, and assert that `/mcp` with no token gets 401 through that path. 2. **Re-wrap gap.** `buildMux` returns an `http.Handler`, so nothing can add routes to *its result*. But main.go could still wrap that result in a new mux with a public `POST /query` between `buildMux` and `ListenAndServe`, and CI would stay green. One untested line of wiring remains. Either accept it and make the comments in `routes_test.go`/`main.go` claim only that, or move the `ListenAndServe` handler construction into a tested function. Done when both are tested or the comments are narrowed, with a flip test showing the gap is covered. --- _Created via git-mcp on behalf of @mathias_
mathias added the risk/lowsize/stype/test labels 2026-10-03 14:35:26 +00:00
mathias commented 2026-10-03 15:15:00 +00:00
Author
Owner
Copy Link
Copy Source

Same class, added from the PR #159 review: the symlink tripwire's wiring in ingestion/cmd/server/main.go (startupSymlinkCheck(brainDir) before any reader starts, and go symlinkguard.Watch(...)) isn't test-covered. Removing both calls keeps the suite green. Covering it belongs with this issue's fix: move server assembly into a tested function, or assert the startup order.

Created via git-mcp on behalf of @mathias

Same class, added from the PR #159 review: the symlink tripwire's wiring in `ingestion/cmd/server/main.go` (`startupSymlinkCheck(brainDir)` before any reader starts, and `go symlinkguard.Watch(...)`) isn't test-covered. Removing both calls keeps the suite green. Covering it belongs with this issue's fix: move server assembly into a tested function, or assert the startup order. --- _Created via git-mcp on behalf of @mathias_
mathias commented 2026-10-05 14:20:59 +00:00
Author
Owner
Copy Link
Copy Source

pr-gatekeeper verdict: BLOCK (PR #183, commit a1e0361 only, closes #154)

Mode: PR mode. Scope: commit a1e0361, diffed against 474473c. #181 is reviewed separately and is still unmerged.

Blocking findings

The code change is correct. The block is on the comments. Part 2 of #154 is closed by narrowing the comments (the "or the comments are narrowed" clause), and the narrowed comments are still wrong, so that done-when clause is not met.

  1. ingestion/cmd/server/main.go:588-591 says ListenAndServe "must serve its result unchanged: that one line is the untested part". routes.go:44 says the same thing ("What stays untested is main's own line"), and so does #182. That is not the only untested line. All of main's buildMux(restAuth{...}, ..., public) call is untested. Checked by running it: I changed main.go's validator: jwtValidator to validator: nil, and go vet and go test ./cmd/server/ both exit 0. That change drops JWT acceptance on both /mcp and REST, which breaks the claude.ai OAuth connector. Narrow the comment, or say what is actually untested: the restAuth fields and public map that main passes in, plus the ListenAndServe line. Update #182 to match.
  2. ingestion/cmd/server/routes_test.go:25-27: the testMux comment says it builds the router "exactly as main does ... and real bearer config". It passes no JWT validator and no resourceMetadataURL. Because of that, these mutations of /mcp's middleware inside buildMux leave the whole suite green:
    • validator set to nil
    • resourceMetadataURL set to ""
    • a different realm
  3. ingestion/cmd/server/routes_test.go:88-91: the TestBuildMuxDoesNotWrapPublicRoutes comment still lists "bearer on /mcp" and "/metrics" as public routes it covers. Neither is in publicRoutes after this commit, so the test no longer iterates them. The comment is stale.

Recommended (not in #154's done-when, but it is the PR's own claim)

  1. The PR says /mcp gets "the same BearerMiddleware and credential as REST". The code supports that: one shared bearer closure. The tests do not prove it. Only the static token is asserted on /mcp. Two cheap assertions would make the claim tested:

    • a JWT-shaped token on /mcp while the issuer is unresolved gets 503, as the REST test already checks for /pending
    • a 401 on /mcp carries resource_metadata in WWW-Authenticate

    Both failures I found go the fail-closed way (a broken connector, not an open door), so this is a recommendation, not a block.

What I verified by running it

  • Old vs new router, side by side. I rebuilt 474473c's wiring in a scratch test (/mcp wrapped the old way, GET /metrics as a public route, the same REST mount, reg.Middleware outside) and compared it with the new buildMux. The cases were 5 methods, 21 paths, 7 Authorization shapes, a resolved and an unresolved JWT issuer, and resourceMetadataURL set and unset: 2940 cases. Status, WWW-Authenticate, Location and body were identical in every case. The only exception was the numeric values in /metrics bodies, and its label set was identical too. With the issuer unresolved, a JWT on /mcp gets 503, and a 401 on /mcp carries resource_metadata.
  • Routes. No route gained or lost auth. /metrics is still open.
  • /metrics observing itself. It was already observed by its own middleware before this commit: it sat on the mux that main wrapped. The old comment claiming it was excluded was false. This commit doesn't change that behaviour, and it adds one bounded series.
  • WithCapture. With a real mcp.Server wrapped by buildMux, calling WithCapture afterwards made capture appear in tools/list through the router: false before the call, true after. In main, buildMux now runs after WithCapture in any case.

Flip tests (routes.go mutations, go test ./cmd/server/ exit code)

Mutation Result Tests that went red
/mcp registered without the bearer red MCPWithoutAValidToken
/mcp registered as "/mcp/" red MCPWithoutAValidToken
/mcp bearer built with an empty token red MCPWithoutAValidToken
metrics wrap dropped red 4 tests
GET /metrics not registered red 1 test
/metrics put behind the bearer red 1 test
nil check removed red 1 test
"/mcp" put back on the allowlist red panic on duplicate pattern
REST mounted without the bearer red 6 tests
/mcp validator set to nil survived none
/mcp resourceMetadataURL set to "" survived none
/mcp realm changed survived none
main.go: validator set to nil survived none

CI

Run 9400, "CI / Lint / Test / Vet (pull_request)", passed on a1e0361. CI was green, and none of the findings above are things it checks. Locally, go vet exits 0, golangci-lint run ./cmd/server/... exits 0, and the tests exit 0 with 25 passing.

Not checked

  • #181's commit 474473c.
  • Packages outside ingestion/cmd/server.
  • Live deployment behaviour.
  • ListenAndServe and the symlink tripwire wiring (by design, tracked in #182).
## pr-gatekeeper verdict: BLOCK (PR #183, commit a1e0361 only, closes #154) Mode: PR mode. Scope: commit a1e0361, diffed against 474473c. #181 is reviewed separately and is still unmerged. ### Blocking findings The code change is correct. The block is on the comments. Part 2 of #154 is closed by narrowing the comments (the "or the comments are narrowed" clause), and the narrowed comments are still wrong, so that done-when clause is not met. 1. **`ingestion/cmd/server/main.go:588-591`** says ListenAndServe "must serve its result unchanged: that one line is the untested part". **`routes.go:44`** says the same thing ("What stays untested is main's own line"), and so does #182. That is not the only untested line. All of main's `buildMux(restAuth{...}, ..., public)` call is untested. Checked by running it: I changed main.go's `validator: jwtValidator` to `validator: nil`, and `go vet` and `go test ./cmd/server/` both exit 0. That change drops JWT acceptance on both /mcp and REST, which breaks the claude.ai OAuth connector. Narrow the comment, or say what is actually untested: the restAuth fields and public map that main passes in, plus the ListenAndServe line. Update #182 to match. 2. **`ingestion/cmd/server/routes_test.go:25-27`**: the testMux comment says it builds the router "exactly as main does ... and real bearer config". It passes no JWT validator and no resourceMetadataURL. Because of that, these mutations of /mcp's middleware inside buildMux leave the whole suite green: - validator set to nil - resourceMetadataURL set to "" - a different realm 3. **`ingestion/cmd/server/routes_test.go:88-91`**: the TestBuildMuxDoesNotWrapPublicRoutes comment still lists "bearer on /mcp" and "/metrics" as public routes it covers. Neither is in publicRoutes after this commit, so the test no longer iterates them. The comment is stale. ### Recommended (not in #154's done-when, but it is the PR's own claim) 4. The PR says /mcp gets "the same BearerMiddleware and credential as REST". The code supports that: one shared `bearer` closure. The tests do not prove it. Only the static token is asserted on /mcp. Two cheap assertions would make the claim tested: - a JWT-shaped token on /mcp while the issuer is unresolved gets 503, as the REST test already checks for /pending - a 401 on /mcp carries `resource_metadata` in WWW-Authenticate Both failures I found go the fail-closed way (a broken connector, not an open door), so this is a recommendation, not a block. ### What I verified by running it - **Old vs new router, side by side.** I rebuilt 474473c's wiring in a scratch test (/mcp wrapped the old way, GET /metrics as a public route, the same REST mount, reg.Middleware outside) and compared it with the new buildMux. The cases were 5 methods, 21 paths, 7 Authorization shapes, a resolved and an unresolved JWT issuer, and resourceMetadataURL set and unset: 2940 cases. Status, WWW-Authenticate, Location and body were identical in every case. The only exception was the numeric values in /metrics bodies, and its label set was identical too. With the issuer unresolved, a JWT on /mcp gets 503, and a 401 on /mcp carries resource_metadata. - **Routes.** No route gained or lost auth. /metrics is still open. - **/metrics observing itself.** It was already observed by its own middleware before this commit: it sat on the mux that main wrapped. The old comment claiming it was excluded was false. This commit doesn't change that behaviour, and it adds one bounded series. - **WithCapture.** With a real mcp.Server wrapped by buildMux, calling WithCapture afterwards made `capture` appear in tools/list through the router: false before the call, true after. In main, buildMux now runs after WithCapture in any case. ### Flip tests (routes.go mutations, `go test ./cmd/server/` exit code) | Mutation | Result | Tests that went red | |---|---|---| | /mcp registered without the bearer | red | MCPWithoutAValidToken | | /mcp registered as "/mcp/" | red | MCPWithoutAValidToken | | /mcp bearer built with an empty token | red | MCPWithoutAValidToken | | metrics wrap dropped | red | 4 tests | | GET /metrics not registered | red | 1 test | | /metrics put behind the bearer | red | 1 test | | nil check removed | red | 1 test | | "/mcp" put back on the allowlist | red | panic on duplicate pattern | | REST mounted without the bearer | red | 6 tests | | /mcp validator set to nil | **survived** | none | | /mcp resourceMetadataURL set to "" | **survived** | none | | /mcp realm changed | **survived** | none | | main.go: validator set to nil | **survived** | none | ### CI Run 9400, "CI / Lint / Test / Vet (pull_request)", passed on a1e0361. CI was green, and none of the findings above are things it checks. Locally, `go vet` exits 0, `golangci-lint run ./cmd/server/...` exits 0, and the tests exit 0 with 25 passing. ### Not checked - #181's commit 474473c. - Packages outside ingestion/cmd/server. - Live deployment behaviour. - ListenAndServe and the symlink tripwire wiring (by design, tracked in #182).
mathias commented 2026-10-05 14:39:48 +00:00
Author
Owner
Copy Link
Copy Source

pr-gatekeeper verdict: APPROVE (PR #183 re-review, head b04a348; closes #154)

Mode: PR mode. Scope: 6d44e9d and b04a348, diffed against e05a13a (#181's head). #181 is reviewed separately.

  • Rebase. git range-diff shows 6d44e9d is the same patch as a1e0361. Only context lines from #181 changed.
  • What b04a348 changes in routes.go. Comments only. So the router comparison from the last round still stands: the old and new wiring gave identical status, WWW-Authenticate, Location and body across 2940 cases. The check that capture appears in tools/list after WithCapture on an already-wrapped server also still stands.
  • Blocking findings 1-3 from the last round are resolved.
    • main.go, routes.go and the testMux comment now say what is actually untested: the restAuth fields and public map that main passes in, plus ListenAndServe.
    • #182 is widened to match.
    • The stale public-route list in the TestBuildMuxDoesNotWrapPublicRoutes comment is corrected.
  • Recommendation 4 is done. It is covered by two tests, and both names are honest:
    • TestBuildMuxAnswers503ToAJWTOnMCPWhileTheIssuerIsUnresolved
    • TestBuildMuxMCP401CarriesResourceMetadataInWWWAuthenticate

Flip tests on b04a348 (go test ./cmd/server/ exit code)

Mutation Result Tests that went red
/mcp registered without the bearer red 3 tests
/mcp registered as "/mcp/" red 3 tests
/mcp validator set to nil red 503-on-MCP test
/mcp resourceMetadataURL set to "" red resource_metadata test
/mcp bearer built with an empty token red MCPWithoutAValidToken
metrics wrap dropped red 4 tests
GET /metrics not registered red 1 test
/metrics put behind the bearer red 1 test
nil check removed red 1 test
"/mcp" put back on the allowlist red panic
REST mounted without the bearer red 7 tests
/mcp realm changed survived none
main.go: validator set to nil survived none

The realm survivor doesn't block: realm is cosmetic in the challenge, and the credential is unaffected. The main.go survivor is now stated in the comments and tracked in #182.

Gate

  • Local: go vet exits 0, golangci-lint run ./cmd/server/... exits 0, and go test exits 0 with 28 passing and 0 failing.
  • CI: run 9410, "CI / Lint / Test / Vet (pull_request)", passed on b04a348. It is green, but it is not what proved the points above.

Not checked

  • #181's commits.
  • Packages outside ingestion/cmd/server.
  • Live deployment.

Merge

Not merged. #181 is not yet on main. This should merge only after #181 lands, #183 is rebased onto main so the stacked diff is gone, and CI is green on the resulting head.

## pr-gatekeeper verdict: APPROVE (PR #183 re-review, head b04a348; closes #154) Mode: PR mode. Scope: 6d44e9d and b04a348, diffed against e05a13a (#181's head). #181 is reviewed separately. - **Rebase.** `git range-diff` shows 6d44e9d is the same patch as a1e0361. Only context lines from #181 changed. - **What b04a348 changes in routes.go.** Comments only. So the router comparison from the last round still stands: the old and new wiring gave identical status, WWW-Authenticate, Location and body across 2940 cases. The check that `capture` appears in tools/list after WithCapture on an already-wrapped server also still stands. - **Blocking findings 1-3 from the last round are resolved.** - main.go, routes.go and the testMux comment now say what is actually untested: the restAuth fields and public map that main passes in, plus ListenAndServe. - #182 is widened to match. - The stale public-route list in the TestBuildMuxDoesNotWrapPublicRoutes comment is corrected. - **Recommendation 4 is done.** It is covered by two tests, and both names are honest: - TestBuildMuxAnswers503ToAJWTOnMCPWhileTheIssuerIsUnresolved - TestBuildMuxMCP401CarriesResourceMetadataInWWWAuthenticate ### Flip tests on b04a348 (`go test ./cmd/server/` exit code) | Mutation | Result | Tests that went red | |---|---|---| | /mcp registered without the bearer | red | 3 tests | | /mcp registered as "/mcp/" | red | 3 tests | | /mcp validator set to nil | red | 503-on-MCP test | | /mcp resourceMetadataURL set to "" | red | resource_metadata test | | /mcp bearer built with an empty token | red | MCPWithoutAValidToken | | metrics wrap dropped | red | 4 tests | | GET /metrics not registered | red | 1 test | | /metrics put behind the bearer | red | 1 test | | nil check removed | red | 1 test | | "/mcp" put back on the allowlist | red | panic | | REST mounted without the bearer | red | 7 tests | | /mcp realm changed | **survived** | none | | main.go: validator set to nil | **survived** | none | The realm survivor doesn't block: realm is cosmetic in the challenge, and the credential is unaffected. The main.go survivor is now stated in the comments and tracked in #182. ### Gate - Local: `go vet` exits 0, `golangci-lint run ./cmd/server/...` exits 0, and `go test` exits 0 with 28 passing and 0 failing. - CI: run 9410, "CI / Lint / Test / Vet (pull_request)", passed on b04a348. It is green, but it is not what proved the points above. ### Not checked - #181's commits. - Packages outside ingestion/cmd/server. - Live deployment. ### Merge Not merged. #181 is not yet on main. This should merge only after #181 lands, #183 is rebased onto main so the stacked diff is gone, and CI is green on the resulting head.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
agent/ready
Meets the agent-ready contract; authorizes unattended execution
 blocked/client
Waiting on the client
 blocked/supplier
Waiting on a supplier
 blocked/upstream
Waiting on an upstream project or vendor
 risk/high
Irreversible, security-relevant, or control plane
 risk/low
Reversible, local, test-covered
 risk/medium
Touches shared state, deploys or data
 size/l
Too big - split it
 size/m
Under two days
 size/s
Under half a day
 size/xs
Under an hour
 status:dispatch
status:dispatched
status:review
status:reviewing
type/chore
Maintenance, no behaviour change
 type/decision
Human decision required; never agent/ready
 type/docs
Documentation only
 type/feat
New capability
 type/fix
Corrects broken behaviour
 type/ops
Infrastructure, deploy, runtime
 type/research
Investigate; output is a finding, not code
 type/test
Test coverage only
No labels risk/low size/s type/test
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
renovate-bot mathias
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: mathias/hyperguild#154