Add health_check MCP tool diagnostic handler#73
Merged
jeffgreendesign merged 2 commits intomainfrom Apr 4, 2026
Merged
Conversation
Contributor
WalkthroughThe PR adds a static Sequence Diagram(s)sequenceDiagram
participant Client
participant StatusAPI as Status API
participant HealthTool as Health Tool
participant Postgres as Postgres DB
participant Dashboard
Client->>StatusAPI: GET /status
StatusAPI->>HealthTool: invoke health_check handler
HealthTool->>HealthTool: verify DATABASE_URL configured
alt DB not configured
HealthTool-->>StatusAPI: configError (Database)
else DB configured
HealthTool->>Postgres: SELECT COUNT(*) FROM documents
Postgres-->>HealthTool: count
HealthTool->>Postgres: SELECT COUNT(*) FROM chunks
Postgres-->>HealthTool: count
HealthTool->>Postgres: SELECT COUNT(*) FROM memory_entities
Postgres-->>HealthTool: count
HealthTool->>Postgres: SELECT COUNT(*) FROM conversation_sessions
Postgres-->>HealthTool: count / error
HealthTool->>Postgres: SELECT COUNT(*) FROM proactive_insights
Postgres-->>HealthTool: count
HealthTool->>HealthTool: aggregate component checks -> status (healthy/degraded/unhealthy)
HealthTool-->>StatusAPI: structured health result (timestamp, checks, status)
end
StatusAPI-->>Client: /status response (includes health_check)
StatusAPI->>Dashboard: include tool definition / reported status
Dashboard-->>Client: UI shows MCP Tools including health_check
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/tools/__tests__/health.test.ts (1)
88-92: Consider adding an empty-DB test case.Per project learnings, new tools should include an output schema smoke test covering the empty-DB case. While the current schema validation test passes, adding an explicit test where
countRowsreturns0for all tables would strengthen coverage.📝 Suggested additional test
it('handles empty tables gracefully', async () => { vi.mocked(pgQuery).mockImplementation(async (sql: string) => { if (sql === 'SELECT 1') return { rows: [{ '?column?': 1 }], rowCount: 1 }; return { rows: [{ c: 0 }], rowCount: 1 }; }); const result = (await callHealth()) as { structuredContent?: { status: string; checks: Record<string, { count?: number }> }; }; expect(result.structuredContent?.status).toBe('healthy'); expect(result.structuredContent?.checks.documents.count).toBe(0); });Based on learnings: "New tools must include an output schema smoke test covering the empty-DB case."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/__tests__/health.test.ts` around lines 88 - 92, Add an explicit "empty-DB" unit test to src/tools/__tests__/health.test.ts that mocks pgQuery so countRows returns 0 for tables and the health check still conforms to HealthCheckOutputSchema; specifically, create a test (e.g., it('handles empty tables gracefully')) that vi.mocked(pgQuery).mockImplementation to return the SELECT 1 probe response and zero counts for other queries, call callHealth(), assert result.structuredContent.status is 'healthy' and that result.structuredContent.checks.documents.count === 0, and ensure the output still validates against HealthCheckOutputSchema.src/tools/health.ts (1)
22-25: Consider using a string literal union type for table safety.While currently safe (all callers pass hardcoded literals), adding type-level constraints prevents accidental misuse during future refactoring.
🛡️ Optional defensive typing
+type HealthTable = + | 'documents' + | 'chunks' + | 'memory_entities' + | 'conversation_sessions' + | 'proactive_insights'; + -async function countRows(table: string): Promise<number> { +async function countRows(table: HealthTable): Promise<number> { const result = await pgQuery<{ c: number }>(`SELECT COUNT(*)::int AS c FROM ${table}`); return result.rows[0]?.c ?? 0; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/health.ts` around lines 22 - 25, Replace the untyped string parameter on countRows(table: string) with a string literal union type listing the allowed table names (e.g., type TableName = '...') and change the signature to countRows(table: TableName): Promise<number>; update any callers if they rely on non-literal values and export the TableName type if other modules need it. This constrains usages at the type level while keeping the function body (pgQuery and SQL string) unchanged, preventing accidental future refactor bugs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/tools/__tests__/health.test.ts`:
- Around line 88-92: Add an explicit "empty-DB" unit test to
src/tools/__tests__/health.test.ts that mocks pgQuery so countRows returns 0 for
tables and the health check still conforms to HealthCheckOutputSchema;
specifically, create a test (e.g., it('handles empty tables gracefully')) that
vi.mocked(pgQuery).mockImplementation to return the SELECT 1 probe response and
zero counts for other queries, call callHealth(), assert
result.structuredContent.status is 'healthy' and that
result.structuredContent.checks.documents.count === 0, and ensure the output
still validates against HealthCheckOutputSchema.
In `@src/tools/health.ts`:
- Around line 22-25: Replace the untyped string parameter on countRows(table:
string) with a string literal union type listing the allowed table names (e.g.,
type TableName = '...') and change the signature to countRows(table: TableName):
Promise<number>; update any callers if they rely on non-literal values and
export the TableName type if other modules need it. This constrains usages at
the type level while keeping the function body (pgQuery and SQL string)
unchanged, preventing accidental future refactor bugs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 23e34072-da2f-4231-b2a8-9f33a87c2afb
📒 Files selected for processing (3)
src/api/status.tssrc/tools/__tests__/health.test.tssrc/tools/health.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/tools/__tests__/health.test.ts (1)
39-113: Consider adding a test for theunhealthystatus.The test suite covers
healthyanddegradedstatuses but not theunhealthycase where all checks fail. Since this is a diagnostic tool that will be "the first call when other tools are failing," verifying the all-failed scenario adds confidence.💡 Example test for unhealthy status
it('returns unhealthy when all checks fail', async () => { vi.mocked(pgQuery).mockRejectedValue(new Error('connection refused')); const result = (await callHealth()) as { structuredContent?: { status: string }; }; expect(result.structuredContent?.status).toBe('unhealthy'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/__tests__/health.test.ts` around lines 39 - 113, Add a test that verifies the "unhealthy" status by forcing all subsystem checks to fail (mock pgQuery used by createHandler/callHealth to reject or throw), call createHandler() and assert that the returned structuredContent.status equals 'unhealthy' (and optionally validate the structure with HealthCheckOutputSchema/z.object). Specifically, add a new it('returns unhealthy when all checks fail', ...) that uses vi.mocked(pgQuery).mockRejectedValue(new Error('connection refused')) (or similar), invokes callHealth(), and expects result.structuredContent?.status toBe('unhealthy').src/tools/health.ts (1)
29-32: SQL injection risk via direct table name interpolation.While
TableNameis a string literal union type that restricts valid values at compile time, thetableparameter is interpolated directly into the SQL string. If this function were ever called with a value that bypasses TypeScript checking (e.g., from a type assertion orany), it could enable SQL injection.Consider using a safelist approach for defense-in-depth:
🛡️ Proposed fix with runtime validation
+const ALLOWED_TABLES: ReadonlySet<TableName> = new Set([ + 'documents', + 'chunks', + 'memory_entities', + 'conversation_sessions', + 'proactive_insights', +]); + async function countRows(table: TableName): Promise<number> { + if (!ALLOWED_TABLES.has(table)) { + throw new Error(`Invalid table name: ${table}`); + } const result = await pgQuery<{ c: number }>(`SELECT COUNT(*)::int AS c FROM ${table}`); return result.rows[0]?.c ?? 0; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/health.ts` around lines 29 - 32, The countRows function currently interpolates the table identifier directly into SQL, creating a potential SQL injection vector if TypeScript checks are bypassed; change countRows to perform runtime validation against a safelist of allowed table names (e.g., a const Map or object keyed by TableName -> actual table identifier) and use the mapped, validated identifier in the query, throwing an error for unknown values; reference the countRows function, the TableName type, and pgQuery when implementing this runtime whitelist check to ensure only approved identifiers are used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/tools/__tests__/health.test.ts`:
- Around line 39-113: Add a test that verifies the "unhealthy" status by forcing
all subsystem checks to fail (mock pgQuery used by createHandler/callHealth to
reject or throw), call createHandler() and assert that the returned
structuredContent.status equals 'unhealthy' (and optionally validate the
structure with HealthCheckOutputSchema/z.object). Specifically, add a new
it('returns unhealthy when all checks fail', ...) that uses
vi.mocked(pgQuery).mockRejectedValue(new Error('connection refused')) (or
similar), invokes callHealth(), and expects result.structuredContent?.status
toBe('unhealthy').
In `@src/tools/health.ts`:
- Around line 29-32: The countRows function currently interpolates the table
identifier directly into SQL, creating a potential SQL injection vector if
TypeScript checks are bypassed; change countRows to perform runtime validation
against a safelist of allowed table names (e.g., a const Map or object keyed by
TableName -> actual table identifier) and use the mapped, validated identifier
in the query, throwing an error for unknown values; reference the countRows
function, the TableName type, and pgQuery when implementing this runtime
whitelist check to ensure only approved identifiers are used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 306979c7-5cf6-49e6-9308-44aa3753c0d0
📒 Files selected for processing (2)
src/tools/__tests__/health.test.tssrc/tools/health.ts
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.
Summary
health_checkMCP tool with the requested schema and no input parameters (inputSchema: {})handleHealthCheck()with independent try/catch checks for:SELECT 1with latency)documentschunksmemory_entities)conversation_sessions)proactive_insights)healthy/degraded/unhealthy), per-check details, andtimestamptoolResponse(...)pattern and errors throughtoolError(...)health_checkto the static tool registry list insrc/api/status.tsVerification
pnpm vitest run src/tools/__tests__/health.test.tspnpm buildpnpm verify:fastsuccessfully (quality, tests, security/docs/tool-sync checks)Notes
health_checkregistration/handler behavior and its related status listing/tests.Codex Task