Add debug endpoint for agent inspection - #7
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughRecord plugin activation time and expose a new Changes
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: unknown linters: 'modernize', run 'golangci-lint help linters' to see the list of supported linters Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@server/api.go`:
- Line 86: The call to json.NewEncoder(w).Encode(result) is unchecked — capture
its returned error (e.g., err := json.NewEncoder(w).Encode(result)) and handle
it by logging the error and returning an appropriate HTTP error response; update
the code path that writes the response (the json.NewEncoder(w).Encode(result)
call) to check err and call your request logger or log.Printf with context plus
send http.Error(w, "internal server error", http.StatusInternalServerError) (or
use your existing logger/response helper) so encoding failures are recorded and
the client receives a proper 5xx response.
- Line 67: The call to p.kvstore.GetAgentsByUser is ignoring its error return
(agents, _ := p.kvstore.GetAgentsByUser(userID)); update the handler to capture
the error (agents, err := p.kvstore.GetAgentsByUser(userID)), check if err !=
nil, and return an appropriate error response to the caller (e.g., HTTP 500 or
propagated error) instead of proceeding with a nil/partial agents value; ensure
the error is logged or included in the response for visibility.
- Around line 57-87: The handleDebugAgents handler currently uses a hardcoded
password, accepts auth via query params, and returns sensitive secrets; remove
or disable this debug endpoint (handleDebugAgents) from production code or gate
it behind proper admin authentication instead of a URL query param, eliminate
returning any secrets from p.getConfiguration() (do not include CursorAPIKey or
GitHubWebhookSecret in the response), and add rate-limiting/throttling on the
endpoint (or restrict it to localhost/dev-only builds) while replacing the
simple password check with a secure auth mechanism (e.g., middleware that
verifies an admin session/token); also ensure all references to
kvstore.GetAgentsByUser remain but that returned agent objects are sanitized
(omit API keys/tokens) before encoding.
- Around line 51-53: The debug endpoint is registered on the root router and
thus bypasses auth; move the route registration for p.handleDebugAgents off the
root router and register it on the admin-protected router using the existing
middleware (e.g., chain MattermostAuthorizationRequired and RequireSystemAdmin)
so the endpoint enforces at least authorization and ideally admin-only access;
locate the existing router.HandleFunc("/api/v1/debug/agents",
p.handleDebugAgents) call and change it to use the admin router registration
flow that applies MattermostAuthorizationRequired and RequireSystemAdmin
middleware.
🧹 Nitpick comments (1)
server/api.go (1)
70-73: Useanyinstead ofinterface{}and fix formatting.Static analysis flags formatting issues and suggests using the modern
anyalias.Proposed fix
- var result []map[string]interface{} + var result []map[string]any for _, a := range agents { - record := map[string]interface{}{ + record := map[string]any{
| // Debug endpoint -- dump agent data. | ||
| router.HandleFunc("/api/v1/debug/agents", p.handleDebugAgents).Methods(http.MethodGet) | ||
|
|
There was a problem hiding this comment.
Critical: Debug endpoint bypasses all authentication middleware.
This route is registered directly on the root router, circumventing the established three-tier authentication model (unauthenticated with HMAC, authenticated, admin-only). As per coding guidelines, sensitive endpoints should use appropriate middleware—at minimum MattermostAuthorizationRequired, and ideally RequireSystemAdmin for debug/admin functionality.
🔒 Proposed fix: Register under admin router
- // Debug endpoint -- dump agent data.
- router.HandleFunc("/api/v1/debug/agents", p.handleDebugAgents).Methods(http.MethodGet)
-
+ // Debug endpoint -- dump agent data (admin-only).
+ adminRouter.HandleFunc("/debug/agents", p.handleDebugAgents).Methods(http.MethodGet)🤖 Prompt for AI Agents
In `@server/api.go` around lines 51 - 53, The debug endpoint is registered on the
root router and thus bypasses auth; move the route registration for
p.handleDebugAgents off the root router and register it on the admin-protected
router using the existing middleware (e.g., chain
MattermostAuthorizationRequired and RequireSystemAdmin) so the endpoint enforces
at least authorization and ideally admin-only access; locate the existing
router.HandleFunc("/api/v1/debug/agents", p.handleDebugAgents) call and change
it to use the admin router registration flow that applies
MattermostAuthorizationRequired and RequireSystemAdmin middleware.
Address all review feedback: - Remove unauthenticated debug endpoint that bypassed auth middleware - Remove hardcoded password authentication - Remove API key/secret exposure in response - Remove silently ignored error from GetAgentsByUser - Remove unchecked json.Encode error Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Tracks plugin activation time and reports uptime duration in the admin health check endpoint. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@server/plugin.go`:
- Around line 54-55: The activatedAt field is accessed without locking and is
set too early; add synchronized accessor methods getActivatedAt() and
setActivatedAt(time.Time) that use the existing configurationLock to read/write
activatedAt, move the assignment of activatedAt in OnActivate to after
successful initialization and after the background job is scheduled, and replace
direct reads/writes of Plugin.activatedAt (e.g., the health-check read in api.go
and the write in OnActivate) to use getActivatedAt()/setActivatedAt() so all
accesses respect the lock and the timestamp reflects successful activation.
Address CodeRabbit review: use configurationLock for activatedAt access, set timestamp after initialization completes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Closing - restarting spike with Request Changes workflow enabled |
Summary
Test plan
Summary by CodeRabbit