fix: route MCP time-range validation errors to isError response, not ERROR log #SUPERLOG

[superlog-app]

Summary

MCP telemetry tools (query_logs, query_traces, query_metrics, list_services) were firing ERROR: MCP telemetry query failed permanently log entries whenever a caller supplied an invalid since/until time bound — including SQL injection probe strings like now()) UNION ALL SELECT hostName(),1,1 FROM system.one --. This created noisy incidents and alert fatigue.

The validation itself was correct and working: timeBoundExpr validates the time bound before any SQL is assembled and throws for anything other than an ISO-8601 timestamp or a recognised now() - INTERVAL N UNIT expression. No injection ever reached ClickHouse. The bug was that this input-validation error was then routed through the same permanent-failure path as genuine backend errors, triggering logger.error and the incident.

Root cause

In executeRecoverableTelemetryQuery, all errors that fail isRetryableTelemetryTimeout unconditionally call onPermanentFailure, which in the MCP server logs at ERROR level. There was no distinction between "ClickHouse returned an unknown table error" and "the caller passed a garbage time string".

Fix

1. packages/telemetry-query/src/index.ts — timeBoundExpr now throws a named TimeRangeValidationError subclass instead of a plain Error. This lets callers distinguish input-validation failures from backend failures without importing the error class (duck-typing on error.name).

2. apps/api/src/mcp/telemetry-recovery.ts — executeRecoverableTelemetryQuery skips the onPermanentFailure callback when the thrown error is a TimeRangeValidationError (checked by error.name). The error is still re-thrown so the MCP layer can handle it explicitly.

3. apps/api/src/mcp/server.ts — executeTelemetryQuery now wraps executeRecoverableTelemetryQuery in a try/catch. TimeRangeValidationError is caught, logged at WARN, and converted to a structured { content, isError: true } MCP tool result with a human-readable message pointing the AI to valid syntax. All four tool handlers (query_logs, query_traces, query_metrics, list_services) check for the _mcpError sentinel and surface it correctly.

4. apps/api/src/mcp/telemetry-recovery.test.ts — two new tests verify that onPermanentFailure is not called for validation errors, and that the error is still re-thrown.

Incident: https://app.superlog.sh/incidents/cda37b62-423a-4a89-a330-6f0463135a2c

---

Was this PR helpful? Leave feedback — goes straight to the Superlog team.

---

Summary by cubic

Invalid MCP telemetry time ranges now return a structured isError response with guidance instead of logging a permanent ERROR and triggering incidents. This adds a dedicated validation error to separate bad input from backend failures.

• Bug Fixes
  • Introduced TimeRangeValidationError in @superlog/telemetry-query and throw it from timeBoundExpr.
  • executeRecoverableTelemetryQuery skips onPermanentFailure for validation errors but re-throws.
  • MCP server catches validation errors, logs WARN, and returns { content, isError: true } for query_logs, query_traces, query_metrics, list_services.
  • Added tests to ensure permanent-failure callback isn’t called and errors are re-thrown.

^{Written for commit 4b59131. Summary will update on new commits.}

[superlog-app]

Observability review

• 1 blocking

[superlog-app]

metrics · blocking — Record validation errors under a distinct outcome label, not "permanent_failure"

Move the recordTelemetryQueryOutcome call (or add a branch) so that TimeRangeValidationError is recorded with outcome "validation_error" instead of "permanent_failure"; otherwise every bad caller time-range inflates the permanent-failure counter and breaks alert thresholds that operators rely on to detect real backend failures.

Suggested change

	// Input-validation errors (bad caller-supplied time range) are not backend
	if (isInputValidationError(error)) {
	recordTelemetryQueryOutcome(
	tool,
	"validation_error",
	performance.now() - startedAt,
	);
	} else {
	recordTelemetryQueryOutcome(
	tool,
	"permanent_failure",
	performance.now() - startedAt,
	);
	onPermanentFailure?.(error);
	}
	throw error;

Useful? React with 👍 / 👎.