test(mcp): full tool-surface e2e suite against fake API - #446
Conversation
This comment has been minimized.
This comment has been minimized.
![kody-ai[bot]](https://avatars.githubusercontent.com/in/413034?s=60&v=4){kind=small}
| auth := r.Header.Get("Authorization") | ||
| const prefix = "Bearer " | ||
| token := strings.TrimPrefix(auth, prefix) | ||
| writeJSON(w, http.StatusOK, PongResponse{Ping: "pong", Token: token}) |
There was a problem hiding this comment.
Sensitive token exposure: the token variable derives from the Authorization header, which may contain secrets, and storing or exposing the raw token in the response violates security rules. Return only a boolean or a hashed/redacted value, and ensure any token handling uses secure methods for configuration rather than request-derived secrets.
Kody rule violation: Ban hard-coded secrets in Go source
Prompt for LLM
File internal/mcptest/account/server.go:
Line 133:
Sensitive token exposure: the `token` variable derives from the Authorization header, which may contain secrets, and storing or exposing the raw token in the response violates security rules. Return only a boolean or a hashed/redacted value, and ensure any token handling uses secure methods for configuration rather than request-derived secrets.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
There was a problem hiding this comment.
Declining: this is a test-only fake (internal/mcptest). The echoed token is the deterministic non-secret fixture value "token-e2e@example.com" (not a real credential), and the PongResponse schema's Token field is part of the generated API contract this fake simulates. No production code path is involved.
There was a problem hiding this comment.
Found critical issues please review the requested changes
- Destructive auth_logout de-authenticates the shared config.yaml before afterAll restores it, racing with parallel test files that depend on the seeded token.
- auth.test.ts races the shared config.yaml across the two concurrently-running host projects, making the authenticated/logged-out assertions flaky even on clean runs.
Code Coverage ReportTotal Coverage: 51.6% Generated from commit: 7a2d9ca |
PR Summary (Comment created by Kody 🤖)Code Review Started! 🚀✋ Hi, team! I'm already looking at the changed files and starting the review to ensure everything is in order. If you need more details, I'm here! Kody 📂 Changed Files
📊 Summary of Changes
|
This comment has been minimized.
This comment has been minimized.
| // account_update_email / account_update_password tools against the seeded | ||
| // account. Accounts registered via the register endpoint store the password | ||
| // supplied in the request body instead. | ||
| const DefaultPassword = "password" |
There was a problem hiding this comment.
The constant DefaultPassword is assigned a hard-coded password string, violating the rule against hard-coded secrets in Go source. Replace the literal with a value read from the environment, e.g., const DefaultPassword = os.Getenv("DEFAULT_PASSWORD").
Kody rule violation: Ban hard-coded secrets in Go source
Prompt for LLM
File internal/mcptest/account/server.go:
Line 41:
The constant DefaultPassword is assigned a hard-coded password string, violating the rule against hard-coded secrets in Go source. Replace the literal with a value read from the environment, e.g., `const DefaultPassword = os.Getenv("DEFAULT_PASSWORD")`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
There was a problem hiding this comment.
Declining: DefaultPassword is the deterministic seed password for the test fake's account ("password", not a real secret). It must be a known constant so the e2e harness can drive account_update_email/password against the seeded account. Reading it from the environment would make the fake's seed non-deterministic and break the suite.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
The added test setups invoke Kody rule violation: Disallow GORM queries without timeout |
This comment has been minimized.
This comment has been minimized.
| const SEED_EMAIL = 'e2e@example.com'; | ||
| const SEED_PASSWORD = 'password'; | ||
|
|
||
| test('account_subscription reports the free, not-subscribed status', async ({ mcp }) => { | ||
| // The seeded account is free: is_subscribed=false, no plan period/gateway. | ||
| const result = await invoke(mcp, 'account_subscription', {}); |
There was a problem hiding this comment.
Removing the beforeAll/afterAll hooks drops the safety net that unconditionally restored the SHARED seeded account (e2e@example.com/password) after each run, leaving the shared account re-keyed if any mutating test aborts between its mutation and restore. Restore an afterAll hook that re-establishes SEED_EMAIL/SEED_PASSWORD unless you prefer to keep the per-test restores as the only protection (acceptable only if you accept cascading failures on mid-test abort).
test.afterAll(async ({ mcp }) => {
try {
await invoke(mcp, 'account_update_email', { email: SEED_EMAIL, password: SEED_PASSWORD });
await invoke(mcp, 'account_update_password', { current_password: SEED_PASSWORD, new_password: SEED_PASSWORD });
} catch { /* best-effort */ }
});Prompt for LLM
File tests/sunpeak/mcp-e2e/account.test.ts:
Line 29 to 34:
Removing the beforeAll/afterAll hooks drops the safety net that unconditionally restored the SHARED seeded account (e2e@example.com/password) after each run, leaving the shared account re-keyed if any mutating test aborts between its mutation and restore. Restore an afterAll hook that re-establishes SEED_EMAIL/SEED_PASSWORD unless you prefer to keep the per-test restores as the only protection (acceptable only if you accept cascading failures on mid-test abort).
Suggested Code:
test.afterAll(async ({ mcp }) => {
try {
await invoke(mcp, 'account_update_email', { email: SEED_EMAIL, password: SEED_PASSWORD });
await invoke(mcp, 'account_update_password', { current_password: SEED_PASSWORD, new_password: SEED_PASSWORD });
} catch { /* best-effort */ }
});
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
There was a problem hiding this comment.
Not applied as written: Playwright does not support the per-test fixture in beforeAll/afterAll ("context/page fixtures are not supported in beforeAll/afterAll"), which is why those hooks were removed. The shared account is guarded instead by: the per-test restores inside each mutation test, auth.test.ts immediate-restoring the shared config after both auth_login and auth_logout, and the suite passing green in CI (132 passed).
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…rom adversarial audit
This comment has been minimized.
This comment has been minimized.
| writeJSON(w, http.StatusUnauthorized, map[string]string{"error": "not authenticated"}) | ||
| return | ||
| } | ||
| s.mu.Lock() |
There was a problem hiding this comment.
GetApiOperations ignores all bound query params (status/operation/protocol/cid/search/sort/pagination) and returns every seeded operation unfiltered. The e2e test operations_list filters by status (tests/sunpeak/mcp-e2e/operations.test.ts:26-32) passes only by accident because the fake drops the StatusFilter that the SDK forwards as a server-side query param. Apply the status/operation/protocol/cid filters and pagination from params to the returned rows, mirroring the real API's behavior.
data := make([]OperationListItem, 0, len(s.operations))
for _, op := range s.operations {
if params.Status != nil && string(op.Status) != *params.Status {
continue
}
item := OperationListItem{ ... }
data = append(data, item)
}
total := len(data)Prompt for LLM
File internal/mcptest/account/server.go:
Line 350:
GetApiOperations ignores all bound query params (status/operation/protocol/cid/search/sort/pagination) and returns every seeded operation unfiltered. The e2e test operations_list filters by status (tests/sunpeak/mcp-e2e/operations.test.ts:26-32) passes only by accident because the fake drops the StatusFilter that the SDK forwards as a server-side query param. Apply the status/operation/protocol/cid filters and pagination from params to the returned rows, mirroring the real API's behavior.
Suggested Code:
data := make([]OperationListItem, 0, len(s.operations))
for _, op := range s.operations {
if params.Status != nil && string(op.Status) != *params.Status {
continue
}
item := OperationListItem{ ... }
data = append(data, item)
}
total := len(data)
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| } | ||
| } | ||
|
|
||
| // GetApiOperations lists account operations (GET /api/operations). |
There was a problem hiding this comment.
GetApiOperations receives GetApiOperationsParams but never reads FiltersStatusEq, FiltersIdEq, or _start/_end, so server-side filtering and pagination silently no-op. The SDK's OperationsServiceDefault.List forwards these filters as query params, causing the e2e test (tests/sunpeak/mcp-e2e/operations.test.ts:26-32) to fail its assertion. Honor the bound params by skipping operations that mismatch FiltersStatusEq/FiltersIdEq and applying _start/_end pagination before building the response.
func (s *Server) GetApiOperations(w http.ResponseWriter, r *http.Request, params GetApiOperationsParams) {
if s.authorize(r) == nil { writeJSON(w, http.StatusUnauthorized, ...); return }
s.mu.Lock()
data := make([]OperationListItem, 0, len(s.operations))
for _, op := range s.operations {
if params.FiltersStatusEq != nil && op.Status != *(*params.FiltersStatusEq)["eq"] {
continue
}
if params.FiltersIdEq != nil && op.Id != *params.FiltersIdEq {
continue
}
data = append(data, itemFromOp(op))
}
total := len(data)
s.mu.Unlock()
writeJSON(w, http.StatusOK, OperationListItemResponse{Data: data, Total: total})
}Prompt for LLM
File internal/mcptest/account/server.go:
Line 344:
GetApiOperations receives GetApiOperationsParams but never reads FiltersStatusEq, FiltersIdEq, or _start/_end, so server-side filtering and pagination silently no-op. The SDK's OperationsServiceDefault.List forwards these filters as query params, causing the e2e test (tests/sunpeak/mcp-e2e/operations.test.ts:26-32) to fail its assertion. Honor the bound params by skipping operations that mismatch FiltersStatusEq/FiltersIdEq and applying _start/_end pagination before building the response.
Suggested Code:
func (s *Server) GetApiOperations(w http.ResponseWriter, r *http.Request, params GetApiOperationsParams) {
if s.authorize(r) == nil { writeJSON(w, http.StatusUnauthorized, ...); return }
s.mu.Lock()
data := make([]OperationListItem, 0, len(s.operations))
for _, op := range s.operations {
if params.FiltersStatusEq != nil && op.Status != *(*params.FiltersStatusEq)["eq"] {
continue
}
if params.FiltersIdEq != nil && op.Id != *params.FiltersIdEq {
continue
}
data = append(data, itemFromOp(op))
}
total := len(data)
s.mu.Unlock()
writeJSON(w, http.StatusOK, OperationListItemResponse{Data: data, Total: total})
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| } | ||
| } | ||
|
|
||
| func TestOperationsListAndGet(t *testing.T) { |
There was a problem hiding this comment.
No secret detected in this line. No action required.
Kody rule violation: Ban hard-coded secrets in Go source
Prompt for LLM
File internal/mcptest/account/server_test.go:
Line 234:
No secret detected in this line. No action required.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| s.mu.Lock() | ||
| // A concurrent DeleteApiDnsZonesId may have removed this zone (and its | ||
| // record map) between the zoneByID check above and the lock here; guard | ||
| // against the nil-map panic by (re)creating the map under the lock. | ||
| if s.records[zid] == nil { | ||
| s.records[zid] = map[string]*dnsRecord{} | ||
| } | ||
| s.records[zid][recordKey(name, recordType, body.Content)] = rec | ||
| s.mu.Unlock() |
There was a problem hiding this comment.
Recreating the record map under a concurrent DeleteApiDnsZonesId stores data under a zone id that no longer exists in s.zones, leaving orphaned records unreachable by zone listing or GET requests. Under the lock, verify the zone still exists in s.zones before (re)creating the map; if absent, return 404.
s.mu.Lock()
if _, ok := s.zones[zid]; !ok {
s.mu.Unlock()
writeNotFound(w)
return
}
if s.records[zid] == nil {
s.records[zid] = map[string]*dnsRecord{}
}
s.records[zid][recordKey(name, recordType, body.Content)] = rec
s.mu.Unlock()Prompt for LLM
File internal/mcptest/ipfs/dns.go:
Line 301 to 309:
Recreating the record map under a concurrent DeleteApiDnsZonesId stores data under a zone id that no longer exists in s.zones, leaving orphaned records unreachable by zone listing or GET requests. Under the lock, verify the zone still exists in s.zones before (re)creating the map; if absent, return 404.
Suggested Code:
s.mu.Lock()
if _, ok := s.zones[zid]; !ok {
s.mu.Unlock()
writeNotFound(w)
return
}
if s.records[zid] == nil {
s.records[zid] = map[string]*dnsRecord{}
}
s.records[zid][recordKey(name, recordType, body.Content)] = rec
s.mu.Unlock()
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
…r/OAS binding ambiguity)
Kody Review CompleteGreat news! 🎉 Keep up the excellent work! 🚀 Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
1 participant
Full Sunpeak MCP e2e suite covering the tool surface (auth, account, pins, dns, ipns, websites, operations, meta/wizard/resources), driven through invoke_tool against the swagger-generated fake API. Fake extended as a contract-accurate in-memory implementation wherever a domain tool needed endpoints, plus race-clean Go unit tests for those handlers.
Key constraints kept in this PR:
go test -race(shared-state access locked).CI green (mcp-test + coverage).
Summary
This PR adds a comprehensive end-to-end test suite for the MCP tool surface, covering authentication, pins management, meta-tools (progressive disclosure), and the tool catalog contract.
Key Changes
Fake API Server Enhancement
PostApiAuthPingendpoint to the fake account server that verifies authentication and echoes the provided token, enabling theauth_statustool to validate stored credentials.New Test Suite (
tests/sunpeak/mcp-e2e/)Shared Test Helpers (
helpers.ts)invoke,textOf,isCleanSuccess,describeTool,searchTool) that route all domain tool calls through theinvoke_toolmeta-tool, mirroring how ChatGPT/Claude hosts surface tools.Tool Surface Contract Test (
tool-surface.test.ts)tools/list(49 curated tools + 3 meta-tools), ensuring no hidden catalog tools leak through and no curated ones disappear.account_info,auth_login, and any tool withdns_,ipns_,operations_, orapi_keys_prefixes.Meta-Tools Test (
meta-tools.test.ts)pins_*tools), and that empty/help queries return the curated start-here orientation set.pins_addrequirescids) and tools with no required args (account_info).capabilities(transport, source modes) andagent_guide(onboarding flow chains).Authentication Test (
auth.test.ts)auth_status: Verifies the seeded fixture credential reports as authenticated with the correct account details.auth_login: Confirms the agent-safe token variant validates JWT shape and returns thelogged_instatus contract.auth_logout: Validates the local credential clearing contract, transitioning tonot_authenticatedstate.afterAllto avoid de-authenticating the shared fixture used by other tests/host projects.Pins Domain Test (
pins.test.ts)pins_list(empty) →pins_add(captures request_id) →pins_list(contains pin) →pins_status(round-trip resolution) →pins_rm(destructive gate).pins_rmis blocked by theneeds_human/confirmationhandoff, and that the pin survives after the gate.Existing Test Refactor (
real-tools.test.ts)mcp.callToolboilerplate to the sharedinvoke/isCleanSuccesshelpers.Test Architecture
invoke_tool, matching how a real host would call them from the catalog.