GLOOK-26: Glooker MCP server (read-only data + analysis over MCP) - #59
Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…d JSON-RPC, JS-side dedup) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
queryCommits unconditionally called reportOrg, wasting a DB query in report-scoped mode where org is never used. Moved the reportOrg call to only execute in the cross-report branch, matching queryJiraIssues behavior. Also updated the report-scoped test case to remove the mock for the org lookup that no longer happens. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The clampLimit helper now validates that the limit is a positive integer, falling back to the default for negative, zero, NaN, or non-finite inputs. Fractional limits are floored to integers. This prevents LIMIT -1 (which means "no limit" on SQLite and is a syntax error on MySQL) and other edge cases from bypassing the MAX_ROWS cap. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…c_timeseries (GLOOK-26)
- narrow query-result unions in mcp-queries.test.ts via type assertions (ts-jest skips full type-check due to isolatedModules, so tsc --noEmit was the first gate to catch these) - give getHandler an unused Request param so GET's exported type matches the (req: Request) => Promise<Response> shape used in mcp-route.test.ts
…GLOOK-26) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…per-report totals (GLOOK-26) Row-metric timeseries deduped raw rows in JS after a pre-dedup LIMIT, so on production-sized data it capped at 20k rows (ORDER BY committed_at ASC) and silently dropped the most recent buckets. Move dedup into SQL (GROUP BY the entity key with MIN(timestamp)) so Node receives one row per distinct commit/PR/issue — accurate counts, bounded by real activity. group_by=report now returns true per-report totals (no cross-report dedup, its natural meaning). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…objects) (GLOOK-26) String(date).slice(0,10) yielded 'Mon Mar 16' for mysql2 Date objects instead of an ISO date, producing malformed/colliding bucket labels in group_by=report and the impact_score/ai_percentage branch. Add isoDate() normalizing Date|string. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
msogin
left a comment
msogin
left a comment
There was a problem hiding this comment.
Combined Review Summary — 3 reviews (Sr. Architect persona, Security Engineer persona, standard Smartling fullstack review prompt)
Findings: 13 inline comments — 1 critical, 6 warnings, 3 suggestions, 3 questions (overlapping findings from different reviewers collapsed into one comment where they landed on the same line).
Highest priority
- 🔴 No in-app auth (
route.ts) — every MCP tool, including per-developer cost/performance data and cross-org commit/Jira data, is reachable with zero authentication enforced in this codebase. All enforcement is delegated to a separatemcp-okta-proxysidecar that is explicitly out of scope for this PR and not verifiable from this diff. Recommend an explicit sign-off (not just documentation) that the proxy lands in the same deploy window and that the app port cannot be reached except through it, in every environment this deploys to. - 🟡 Dedup-after-limit bug in
queryCommits/queryJiraIssues(found independently by two of the three reviews) — cross-report queries can silently return fewer thanlimitdistinct results. The same bug class was already found and fixed forgetMetricTimeseriesin this same PR; the fix just needs to be back-ported to its two siblings.
Also worth a look (not tied to one line)
- No rate limiting on
/api/mcpbeyond per-call row caps — an automated client can loop across all reports/orgs; a natural consequence of exposing a general-purpose query interface to agents. docs/superpowers/plans/2026-07-21-glook-26-mcp-server.md(the committed plan doc) describes the old JS-side dedup approach forgetMetricTimeseriesthat the shipped code already improved on (moved to SQL-side dedup) — the doc is now stale relative to the code it describes.- No top-level README section documenting the new
/api/mcpcapability for engineers who don't read the internal planning docs.
Per-reviewer verdict
- Architecture (Sr. Architect + Gaps): Safe to merge — layering is sound, the
project-insights/release-notesextraction is a faithful mechanical refactor, no findings rose to "Rethink" level. - Security (Security Engineer + Security): Does not clearly meet "safe to merge" as currently scoped — wants explicit confirmation the proxy-only auth model is airtight before this deploys anywhere reachable outside localhost.
- Standard Smartling review: Ready to merge with fixes — the dedup bug should be fixed, and the proxy-only auth model needs an explicit sign-off before deploying outside localhost.
Posted from local review tooling (review-loop personas + the standard Smartling claude-pr-review fullstack prompt, run locally) — no code changes were made as part of this review.
| import { handleJsonRpc } from '@/lib/mcp/protocol'; | ||
| import { extractUser } from '@/lib/auth'; | ||
|
|
||
| async function postHandler(req: Request) { |
There was a problem hiding this comment.
🔴 critical
No authentication or authorization is enforced in this handler (or anywhere in the MCP request path). Every one of the 14 MCP tools — including per-developer cost/performance data (impact_score, cc_total_cost, ai_percentage) and cross-org commit/Jira/epic data — is reachable by anyone who can POST to /api/mcp. Enforcement is entirely delegated to a separate mcp-okta-proxy sidecar that is explicitly "tracked separately" per the design doc and is not part of this PR.
Fix: add an in-app fail-closed check (e.g. require a shared-secret header from the proxy, or reject if no identity header is present) so this doesn't silently become a fully open read API if the proxy is missing, misconfigured, or the app port becomes reachable another way before/without it.
|
|
||
| async function postHandler(req: Request) { | ||
| // Identity is read for request-log attribution only (read-only server, no gating). | ||
| try { extractUser(req.headers); } catch { /* no-op */ } |
There was a problem hiding this comment.
🔵 suggestion
extractUser(req.headers) here has its return value discarded and is wrapped in a swallowing catch — it has no observable effect. withRequestLog (from @/lib/logger) already calls extractUser independently to populate userEmail in the log entry, so this duplicate call does nothing.
Fix: remove this line, or if it's meant as future-proofing for per-user gating, add a comment clarifying that.
| } | ||
|
|
||
| async function optionsHandler() { | ||
| return new Response(null, { status: 204 }); |
There was a problem hiding this comment.
🟣 question
optionsHandler returns a bare 204 with no Access-Control-Allow-* headers. Is this endpoint intentionally server-to-server only (no browser CORS preflight expected)? If any client ever calls this from a browser context, the preflight will fail silently.
| return rows[0]?.org ?? null; | ||
| } | ||
|
|
||
| export async function listReports(args: { org?: string; status?: string; limit?: number }) { |
There was a problem hiding this comment.
🟣 question
listReports with no org filter returns reports across all orgs, and none of the query tools in this file scope results by the caller's identity/tenant — access is effectively "any authenticated user sees everything." Is this the intended trust model (single company, all engineers see all org analytics), or should this be scoped per-caller the way the existing requireAdmin-gated report-creation route restricts writes?
| FROM commit_analyses ca | ||
| WHERE ${conditions.join(' AND ')} | ||
| ORDER BY ca.committed_at DESC | ||
| LIMIT ?`, |
There was a problem hiding this comment.
🟡 warning
queryCommits's cross-report path applies LIMIT ? to the raw, pre-dedup rows, then dedups by commit_sha afterward in JS (line 120). If the same commit appears in multiple overlapping report windows, a caller requesting limit: 100 can silently get back fewer than 100 distinct commits, with no truncated flag to signal it happened.
This exact bug class was found and fixed in getMetricTimeseries further down this file (moved dedup into SQL via GROUP BY before LIMIT — see the comment above the row-based branch) — worth applying the same pattern here.
Fix: GROUP BY commit_sha with MIN(committed_at) (or similar) before the LIMIT, instead of limiting raw rows and deduping after.
| const params: any[] = [org]; | ||
| if (args.since) { conditions.push(`${alias}.${tsCol} >= ?`); params.push(args.since); } | ||
| if (args.until) { conditions.push(`${alias}.${tsCol} <= ?`); params.push(args.until); } | ||
| if (args.since === undefined && args.until === undefined) { |
There was a problem hiding this comment.
🟡 warning
The default 180-day window only applies when args.since === undefined && args.until === undefined. A caller passing since: '' (empty string) skips both the explicit filter above (if (args.since) is falsy) and this default-window fallback (since '' is not undefined), resulting in an unbounded query up to TIMESERIES_ROW_CAP (20000 rows).
Fix: normalize/trim since/until before this check, or treat falsy strings the same as missing values.
|
|
||
| // get_project_details: filter the insights payload down to one project by name. | ||
| async function getProjectDetails(args: { project_name: string; report_id?: string }) { | ||
| const insights: any = await getProjectInsights(args.report_id); |
There was a problem hiding this comment.
🔵 suggestion
getProjectDetails (here), getTeamPulseTool (line 34), and getDeveloperSummaryTool (line 47) are net-new, non-trivial handlers (case-insensitive project matching + "not found" list; team-membership lookup + "team not found" branch), but mcp-tools.test.ts only exercises list_reports end-to-end and the registry shape — none of these three branches, including their error paths, have dedicated tests, unlike every pure helper/query function elsewhere in this PR.
Worth adding unit tests for these three handlers, especially the not-found branches.
| return await tool.handler(args ?? {}); | ||
| } catch (err) { | ||
| return { error: err instanceof Error ? err.message : String(err) }; | ||
| } |
There was a problem hiding this comment.
🟡 warning
On handler failure, the raw err.message (or String(err)) is returned to the MCP caller as tool content. This can leak internal implementation detail (DB error text, occasionally table/column names) to whatever client/agent is talking to the MCP server — a new, broader surface purpose-built for machine consumption vs. the existing HTTP routes with the same pattern.
Consider returning a generic error message to the caller and logging the real err server-side instead.
| case 'tools/list': | ||
| return ok(id, { tools: MCP_TOOLS.map(t => ({ name: t.name, description: t.description, inputSchema: t.inputSchema })) }); | ||
| case 'tools/call': { | ||
| const name = params?.name; |
There was a problem hiding this comment.
🔵 suggestion
Several tools declare required fields in their JSON-Schema inputSchema (e.g. get_project_details.project_name, get_team_pulse.team/org, get_developer_summary.login, get_metric_timeseries.metric — see tools.ts), but neither this dispatch nor callTool validates presence before invoking the handler. A tools/call missing a required arg flows straight into query builders with undefined, which can produce a confusing downstream error (or wrong result, e.g. "project not found") instead of a clean -32602 Invalid params.
Consider a small required-field check here (or in callTool) driven by each tool's inputSchema.required.
|
|
||
| return { available: true as const, summary, commitCount: commits.length, generatedAt: new Date().toISOString(), latestSha, cached: false }; | ||
| } catch (err) { | ||
| console.error('[release-notes]', err); |
There was a problem hiding this comment.
🟡 warning
Every failure mode here (missing token, GitHub API error, LLM error, DB error) collapses into the same { available: false } as "no new notes yet." This was pre-existing behavior in the original route, but it's now also reachable via the get_release_notes MCP tool, where an autonomous caller has no way to distinguish "nothing to report" from "this is broken."
Consider including an error field (distinct from the normal empty case) when the failure is unexpected (API/DB error) vs. expected (no commits in window).
…r masking, validation (GLOOK-26) - queryCommits/queryJiraIssues: dedup cross-report results in SQL (GROUP BY key) so LIMIT bounds distinct entities, not raw rows (could under-return before) - getEpicSummaries: add limit + LIMIT clause (was unbounded) - get_metric_timeseries: treat empty/whitespace since/until as absent so the default 180-day window isn't bypassed by since:'' - callTool: mask raw error messages to the MCP caller, log real error server-side - protocol: validate declared required fields → clean -32602 before dispatch - release-notes service: distinguish unexpected failures (error field) from the normal empty case - route: drop redundant extractUser call (withRequestLog already attributes logs) - tests for the not-found handler branches, required-field validation, error masking Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…K-26) - /api/mcp returns 401 when AUTH_ENABLED=true and the proxy identity header is absent — defense-in-depth so a missing/misconfigured proxy can't expose an open read API. Off when AUTH_ENABLED unset (local dev/mock). - Document the single-company read-only trust model (any authenticated employee sees org-wide analytics incl. per-dev cost/impact; how to restrict later). - README: new MCP Server section (tools, connect commands, auth). - .env.example: MCP auth backstop + trust model. - Plan doc: note shipped code moved dedup to SQL (supersedes Task 4-5 blocks). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review findings addressedPushed two commits ( 🔴 Critical
🟡 Warnings
🔵 Suggestions
🟣 Questions
Also-worth-a-look items
Separately, local testing against real data surfaced a data issue (not code): |
1 participant
Summary
Adds a read-only MCP server exposing Glooker's data and analysis so managers, execs, and engineers can query it from Claude Code / Claude.ai — including cross-report analysis over time, not just a single report snapshot.
/api/mcpendpoint — stateless Streamable-HTTP, hand-rolled JSON-RPC (no@modelcontextprotocol/sdk, no new dependencies)chat/tools.tspattern):list_reports,get_org_summaryquery_commits,query_jira_issues,query_developer_stats,query_unmerged_workget_project_insights,get_project_details,get_highlights,get_team_pulse,get_developer_summary,get_release_notes,get_epic_summariesget_metric_timeseriesproject-insightsandrelease-notesroutes into shared services (getProjectInsights,getReleaseNotes) so the route and MCP tool share one implementation.mcp.jsonfor local connection +.env.exampleproxy docsArchitecture
route.ts(transport) →protocol.ts(JSON-RPC) →tools.ts(registry) →queries.ts+ existing servicesAuth is handled out-of-band by the Smartling
mcp-okta-proxysidecar in AWS (setOKTA_MCP_PROXY_USERINFO_HEADER_NAME=x-amzn-oidc-data); the route reads identity only for request logging. Deployment (Terraform/ECS/Okta) is tracked separately.Testing
tsc --noEmitcleanlist_reports/get_org_summary/get_metric_timeseriesreturn accurate results;commits/weekreturns the full 23-week range with no truncationNotes
get_metric_timeseriesdedup is done in SQL (GROUP BY+MIN(timestamp)) so it stays bounded on production-sized data;group_by=reportreturns per-report totalsjira_issues.resolved_atare zero-dates in the current DB, so thejira_resolvedtime-series is empty until ingestion is fixed — tracked in GLOOK-33Design spec:
docs/superpowers/specs/2026-07-21-glook-26-mcp-server-design.md🤖 Generated with Claude Code