Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
🦋 Changeset detectedLatest commit: 336f5f2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
TL;DR — Migrates all SigNoz integration from the v4 API ( Key changes
Summary ｜ 9 files ｜ 5 commits ｜ base:
|
| v4 | v5 |
|---|---|
contextResolution + contextHandle |
contextResolutionAndHandle |
aiGenerations + aiStreamingText |
aiLlmCalls |
toolApprovalRequested/Approved/Denied |
toolApprovals |
signoz.ts — Proxy route refactor and v5 endpoint
Before: Inline auth checks, config reads, and error handling duplicated across
/query,/query-batch, and/healthhandlers; endpoint pointed at/api/v4/query_range.
After: SharedgetSignozConfig(),authorizeProject(),handleSignozError(), andextractResults()helpers; endpoint is/api/v5/query_range;query-batchparses columnar v5 responses and narrows step-2 time windows using min/max timestamps from step-1 results.
The injectConversationIdFilter function (which built structured filter objects) is replaced by inline expression string concatenation. The query-batch step-2 optimization reads columns + data arrays instead of series[].labels to extract conversation IDs, and computes a tighter time range from aggregation column values.
How does the step-2 time range narrowing work?
After step 1 returns paginated conversation IDs with timestamps, the code scans the aggregation column to find the min and max timestamps, then sets `detailPayloadTemplate.start = minTs - 1h` and `end = maxTs + 1h`. This avoids querying the full original time range for the detail step.
conversations/[conversationId]/route.ts — Parallel batched queries and v5 payloads
Before: A single monolithic SigNoz payload with 15+
builderQueriesentries, each using verbose filter objects; sequential execution; no timing instrumentation.
After: 3 parallel payloads (core,context,events) usingbuildQueryEnvelope()with expression-based filters;parseListByName()to split merged query results bynamefield; memoizedfindAncestorActivity/findAncestorAgentGenerationwith depth guards; per-batch and total timing logs.
This is the largest change in the PR. The buildConversationListPayload function (returning a single payload) is replaced by buildConversationPayloads (returning 3 payloads executed via Promise.all). Field selection uses a compact sf(name, type, context) helper instead of 5-line objects. Post-query filtering via parseListByName handles the merged queries (e.g., splitting contextResolutionAndHandle rows by span name). The ancestor-lookup functions now use Map-based caches and depth limits to prevent stack overflows on deep span trees.
[conversationId]/route.ts · traces/route.ts
spans/[spanId]/route.ts — v5 ClickHouse SQL response format
Before: Response parsed as
json.data.result[0].series[0].labels(v4 series-based format); payload usedstep,chQueriesmap,queryType: 'clickhouse_sql',panelType: 'table'.
After: Response parsed asjson.data.data.results[0].columns+data(v5 columnar format); payload usesrequestType: 'scalar',queriesarray with{type, spec}envelopes, typed variables with{type: 'custom', value}.
|
TL;DR — Migrates the entire SigNoz observability integration from v4 to v5 API. The v5 format replaces structured JSON filter objects with string-based filter expressions, swaps Key changes
Summary ｜ 9 files ｜ 12 commits ｜ base: SigNoz v4 → v5 query format migration
The v5 format is more concise — the
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid migration from SigNoz v4 to v5 — good refactoring (consolidated queries, parallel batching, memoized ancestor lookups). Two high-severity items need attention: an expression injection vector in signoz.ts:174 and a null-safety gap in enforceSecurityFilters. Lower-severity items noted inline.
| const securityExpr = buildSecurityExpression(tenantId, projectId); | ||
| for (const { type, spec } of payload.compositeQuery.queries) { | ||
| if (type !== QUERY_TYPES.BUILDER_QUERY) continue; | ||
| spec.filter = { expression: `(${spec.filter.expression}) AND ${securityExpr}` }; |
There was a problem hiding this comment.
High: Null-safety gap. If any query arrives without spec.filter or spec.filter.expression (e.g. a new query type or a client-side bug), this line will throw Cannot read properties of undefined (reading 'expression'). The old v4 code was resilient to missing filters — it would create them from scratch.
Suggestion: default to an empty expression:
const existing = spec.filter?.expression ?? 'true';
spec.filter = { expression: `(${existing}) AND ${securityExpr}` };|
|
||
| const convIdExpr = `${SPAN_KEYS.CONVERSATION_ID} IN (${conversationIds.map((id) => `'${id}'`).join(', ')})`; | ||
| for (const { spec } of detailPayloadTemplate.compositeQuery.queries) { | ||
| spec.filter = { expression: `(${spec.filter.expression}) AND ${convIdExpr}` }; |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| logger.debug({ responseData }, 'SigNoz response (truncated)'); | ||
| return json; | ||
| const json = response.data; | ||
| const results = json?.data?.data?.results ?? json?.data?.results ?? []; |
There was a problem hiding this comment.
Same concern as extractResults. The dual fallback json?.data?.data?.results ?? json?.data?.results suggests uncertainty about the response shape. If the v5 shape is known, pin to one path and drop the fallback — the silent fallback makes debugging harder when the response shape changes.
| const float64 = FIELD_DATA_TYPES.FLOAT64; | ||
| const bool = FIELD_DATA_TYPES.BOOL; | ||
|
|
||
| function buildBaseExpression(conversationId: string, projectId?: string): string { |
There was a problem hiding this comment.
Same expression injection pattern as the server-side route. conversationId comes from URL path params (line 461) and projectId from query params (line 469). Both are user-controlled and interpolated into filter expressions without escaping. A value like test' OR 1=1 -- would inject arbitrary filter logic.
This is lower risk since the payload goes through enforceSecurityFilters on the server side (which scopes to the correct tenant), but it still allows a user to broaden their own query scope within their tenant — e.g. fetch data for a different conversation.
| const payloads = buildConversationPayloads(conversationId, start, end, projectId); | ||
| const batchLabels = ['core', 'context', 'events'] as const; | ||
|
|
||
| const batchResults = await Promise.all( |
There was a problem hiding this comment.
Good improvement. Splitting the single massive query into three parallel batches (core, context, events) with Promise.all is a meaningful performance win — the SigNoz backend can process them concurrently. The timing logs are also helpful for diagnosing production latency.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(8) Total Issues | Risk: High
🔴❗ Critical (2) ❗🔴
Inline Comments:
- 🔴 Critical:
signozHelpers.ts:6SQL injection via unescaped tenantId/projectId interpolation - 🔴 Critical:
signoz.ts:174SQL injection via unescaped conversation IDs from query results
🟠⚠️ Major (3) 🟠⚠️
🟠 1) signozHelpers.ts:19 Security filters not applied to non-builder query types
Issue: The enforceSecurityFilters function only applies tenant/project filters to queries where type === QUERY_TYPES.BUILDER_QUERY. If a ClickHouse SQL query or other query type is passed through, it will bypass tenant isolation entirely.
Why: The spans/[spanId]/route.ts uses clickhouse_sql queries which would not be filtered. This creates a potential tenant isolation bypass if the proxy endpoint accepts non-builder queries.
Fix: Either:
- Reject non-builder query types in the proxy endpoint with a 400 error
- Implement security filtering for
clickhouse_sqlqueries by injecting WHERE clauses - Document that only builder queries are supported and validate the query type before proxying
Refs:
Inline Comments:
- 🟠 Major:
signozHelpers.ts:20Null pointer whenspec.filteris undefined - 🟠 Major:
signoz-queries.ts:52Incorrect SQL escaping (backslash vs standard SQL'')
🟡 Minor (3) 🟡
🟡 1) signoz.ts:57-80 Missing 429 rate limit handling in SigNoz proxy
Issue: The handleSignozError function does not check for HTTP 429 responses from SigNoz. When SigNoz is rate-limiting requests, the proxy returns a generic 500 error instead of propagating the 429 status and Retry-After header.
Why: This masks the actual issue from clients and prevents them from implementing proper backoff. The conversation route already handles 429 (lines 116-117), but the agents-api proxy does not.
Fix: Add 429 handling in handleSignozError:
if (error.response?.status === 429) {
logger.warn({ status: 429 }, 'SigNoz rate limit exceeded');
return {
body: { error: 'Too Many Requests', message: 'SigNoz rate limit exceeded', retryAfter: error.response.headers['retry-after'] },
status: 429 as const
};
}Refs:
🟡 2) signoz-queries.ts:76-86 OPERATORS constant missing NOT_IN
Issue: The internal OP_MAP at line 48 includes 'nin' -> 'NOT IN' mapping, but the exported OPERATORS constant does not include NOT_IN. The old v4 OPERATORS had NOT_IN: 'nin'.
Why: Consumer code that previously used OPERATORS.NOT_IN will break after this migration.
Fix: Add NOT_IN: 'nin' to the OPERATORS constant for parity with OP_MAP.
Refs:
Inline Comments:
- 🟡 Minor:
conversations/[conversationId]/route.ts:164Hardcoded'builder_query','desc',60,falseinstead of constants
💭 Consider (1) 💭
💭 1) signoz-stats.ts (multiple methods) Silent fallback to empty data on API errors
Issue: Multiple methods in SigNozStatsAPI catch all errors and return empty arrays/default values without surfacing the error condition to callers. Users see empty dashboards without knowing whether it's due to no data or a service failure.
Why: This is a pre-existing pattern, but worth addressing for better user experience. The pattern appears in 15+ methods throughout the file.
Fix: Consider returning a discriminated union: { status: 'success' | 'error', data?: T, error?: Error } to distinguish between "no data" and "service error".
Refs:
- Pattern appears throughout signoz-stats.ts
🧹 While You're Here (1) 🧹
🧹 1) signoz-links.ts Still uses v4 SigNoz query format
Issue: This PR migrates SigNoz queries from v4 to v5 format, but signoz-links.ts was not touched and still uses the old v4 compositeQuery structure with queryType/builder/dataSource pattern.
Why: This creates split-world inconsistency where future maintainers may not know which pattern to follow.
Fix: Either migrate signoz-links.ts to v5 format in this PR, or create a tracking issue documenting this as intentional legacy code with a migration plan.
Refs:
🚫 REQUEST CHANGES
Summary: The v4→v5 SigNoz migration is well-structured with significant code simplification (-1600 lines net). However, there are critical SQL injection vulnerabilities in the new string-interpolation-based filter building that must be addressed before merge. The tenantId, projectId, and conversationIds values are interpolated directly into SQL filter expressions without escaping single quotes, creating a potential security and data integrity risk. Additionally, the security filter enforcement skips non-builder query types, which could allow tenant isolation bypass.
Discarded (5)
| Location | Issue | Reason Discarded |
|---|---|---|
signoz-stats.ts:160 |
Global axios retry configuration may interfere with other HTTP clients | Pre-existing pattern, out of scope for this PR |
conversations/[conversationId]/route.ts:116 |
Rate limit handling does not respect Retry-After header | Already handles 429 correctly, forwarding the header is a nice-to-have not a bug |
spans/[spanId]/route.ts:127 |
Generic error message loses diagnostic context | Pre-existing pattern, not introduced by this PR |
signoz-stats.ts:127 |
Filter value parsing error uses console.warn but continues | Defensive pattern, appropriate behavior |
conversations/[conversationId]/route.ts:807 |
Empty catch blocks for JSON parsing | Appropriate defensive handling for optional data |
Reviewers (4)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
4 | 1 | 0 | 0 | 3 | 0 | 0 |
pr-review-sre |
7 | 1 | 0 | 0 | 1 | 0 | 3 |
pr-review-errors |
4 | 0 | 1 | 0 | 0 | 0 | 2 |
pr-review-consistency |
5 | 1 | 0 | 1 | 1 | 0 | 0 |
| Total | 20 | 3 | 1 | 1 | 5 | 0 | 5 |
| }; | ||
|
|
||
| function quoteValue(value: unknown): string { | ||
| if (typeof value === 'string') return `'${value.replace(/'/g, "\\'")}'`; |
There was a problem hiding this comment.
🟠 MAJOR Incorrect SQL escaping: backslash instead of standard SQL
Issue: The quoteValue function escapes single quotes by replacing ' with \' (backslash-escape). Standard SQL uses '' (doubled single quotes). ClickHouse supports both syntaxes, but backslash escaping can behave unexpectedly depending on settings.
Why: Using non-standard escaping may cause issues in edge cases and is inconsistent with the escaping recommended for ClickHouse.
Fix:
| if (typeof value === 'string') return `'${value.replace(/'/g, "\\'")}'`; | |
| if (typeof value === 'string') return `'${value.replace(/'/g, "''")}'`; |
Refs:
| limit = 10000 | ||
| ): any { | ||
| return { | ||
| type: 'builder_query', |
There was a problem hiding this comment.
🟡 Minor Hardcoded string instead of QUERY_TYPES.BUILDER_QUERY constant
Issue: This file uses the hardcoded string 'builder_query' while signoz-stats.ts consistently uses QUERY_TYPES.BUILDER_QUERY. The same applies to 'desc' (line 170), 60 (line 172), and false (line 173) which have corresponding constants.
Why: Using magic strings instead of shared constants creates maintenance burden and risks typos. The peer file signoz-stats.ts consistently uses QUERY_TYPES.BUILDER_QUERY, ORDER_DIRECTIONS.DESC, QUERY_DEFAULTS.STEP_INTERVAL, and QUERY_DEFAULTS.DISABLED.
Fix: Import and use the constants:
import { QUERY_TYPES, ORDER_DIRECTIONS, QUERY_DEFAULTS } from '@/constants/signoz';
// ...
return {
type: QUERY_TYPES.BUILDER_QUERY,
spec: {
// ...
order: [{ key: { name: SPAN_KEYS.TIMESTAMP }, direction: ORDER_DIRECTIONS.DESC }],
stepInterval: QUERY_DEFAULTS.STEP_INTERVAL,
disabled: QUERY_DEFAULTS.DISABLED,
},
};Refs:
- signoz-stats.ts:1060 — example of consistent constant usage
Ito Test Report ✅6 test cases ran. 6 passed. ✅ The included trace workflow checks passed: filter mode switching behaved as expected, timestamp badge suppression was validated, date clamping worked, invalid conversation handling remained controlled, and rapid interaction flows converged to stable UI states. ✅ Passed (6)
📋 View Recording |
There was a problem hiding this comment.
PR Review Summary
(4) Total Issues | Risk: Medium
Re-review scope: This review covers the delta since the last automated review (3 commits, 4 files). Prior findings are referenced in Pending Recommendations.
✅ Issues Addressed in This Delta
Two critical issues from the prior review have been fixed:
- SQL escaping fixed —
quoteValue()now uses standard SQL''escaping instead of backslash\' - Non-builder query bypass fixed —
enforceSecurityFilters()now throws on non-builder query types, preventing tenant isolation bypass via raw SQL
🟠⚠️ Major (1) 🟠⚠️
Inline Comments:
- 🟠 Major:
signoz.ts:72-80queryReferencesKeysdoesn't checkorderByfields
🟡 Minor (2) 🟡
Inline Comments:
- 🟡 Minor:
signoz.ts:108Silent empty response when all queries stripped - 🟡 Minor:
signoz.ts:291Magic number for 180-day lookback period
🕐 Pending Recommendations (3)
These issues were raised in the prior review and remain unaddressed:
- 🔴
signozHelpers.ts:8SQL injection via unescaped tenantId/projectId interpolation inbuildSecurityExpression - 🔴
signoz.ts:262SQL injection via unescaped conversation IDs from query results - 🟠
signozHelpers.ts:28-30Null pointer exception whenspec.filteris undefined
🚫 REQUEST CHANGES
Summary: Good progress on this delta — the SQL escaping and non-builder query bypass issues are now fixed. However, the critical SQL injection vulnerabilities flagged in the prior review (unescaped tenantId/projectId and conversationIds interpolation) remain unaddressed. These should be fixed before merging to ensure tenant data isolation.
Quick wins:
- For
buildSecurityExpression: escapetenantIdandprojectIdwith the samequoteValue()helper that was just fixed, or use parameterized queries - For conversation IDs at line 262:
conversationIds.map((id) => quoteValue(id)).join(', ')(importquoteValuefromsignoz-queries.ts) - For
spec.filternull check:if (!spec?.filter?.expression) throw new Error('Query must have filter.expression');
Discarded (4)
| Location | Issue | Reason Discarded |
|---|---|---|
signoz.ts:85-117 |
Retry logic lacks backoff delay | Since the retry modifies the payload (strips queries), immediate retry is reasonable — it's not retrying the same request |
signoz.ts:275-310 |
Span-lookup doesn't use queryWithRetry | ClickHouse SQL queries have different semantics; the retry logic is designed for builder queries with multiple sub-queries |
signoz.ts:103-106 |
retried flag not used at call sites |
The logging at line 103-106 already provides the key information; adding more logging is marginal value |
signoz.ts:111-115 |
Retry failure logging missing | The outer handleSignozError provides adequate error context |
Reviewers (4)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-security-iam |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-sre |
6 | 0 | 0 | 0 | 2 | 0 | 2 |
pr-review-errors |
3 | 0 | 0 | 0 | 1 | 0 | 2 |
pr-review-standards |
3 | 0 | 0 | 0 | 1 | 0 | 0 |
| Total | 12 | 0 | 0 | 0 | 3 | 0 | 4 |
Note: Security reviewer returned 0 findings for delta — prior critical issues are pre-existing and tracked in Pending Recommendations.
| function queryReferencesKeys(query: any, keys: string[]): boolean { | ||
| const spec = query?.spec; | ||
| const searchable = [ | ||
| spec?.filter?.expression ?? '', | ||
| ...(spec?.selectFields ?? []).map((f: any) => f.name), | ||
| ...(spec?.groupBy ?? []).map((g: any) => g.name), | ||
| ]; | ||
| return keys.some((k) => searchable.some((t: string) => t.includes(k))); | ||
| } |
There was a problem hiding this comment.
🟠 MAJOR queryReferencesKeys doesn't check orderBy fields
Issue: The function only checks filter.expression, selectFields, and groupBy for missing key references. If a missing attribute is referenced only in orderBy, the query won't be filtered out and the retry will fail with the same error.
Why: SigNoz builder queries can reference attributes in order clauses. If the missing key error originates from an orderBy reference, the retry logic will fail to identify and remove the problematic query, causing an unnecessary second failure.
Fix: Add orderBy to the searchable fields:
const searchable = [
spec?.filter?.expression ?? '',
...(spec?.selectFields ?? []).map((f: any) => f.name),
...(spec?.groupBy ?? []).map((g: any) => g.name),
...(spec?.order ?? []).map((o: any) => o.key?.name ?? o.columnName ?? ''),
];Refs:
| 'Retrying SigNoz query without queries referencing missing keys' | ||
| ); | ||
|
|
||
| if (kept.length === 0) return { data: EMPTY_RESPONSE, retried: true }; |
There was a problem hiding this comment.
🟡 Minor Silent empty response when all queries stripped
Issue: When all queries reference missing keys, the function returns EMPTY_RESPONSE silently. The retried: true flag is returned but never surfaced to users. Clients receive empty results with no indication their query attributes aren't available.
Why: Users may assume "no data exists" rather than "query attributes not ingested yet." This makes debugging observability issues difficult during SigNoz schema migrations.
Fix: Log at a higher visibility level when returning empty due to stripped queries:
if (kept.length === 0) {
logger.warn(
{ missingKeys: missing, originalQueryCount: queries.length },
'All queries stripped due to missing attributes - returning empty response'
);
return { data: EMPTY_RESPONSE, retried: true };
}Refs:
- Line 103-106 already logs successful partial strips; this extends to full strip case
| return c.json({ error: 'Service Unavailable', message: 'SigNoz is not configured' }, 500); | ||
|
|
||
| const now = Date.now(); | ||
| const lookbackMs = 180 * 24 * 60 * 60 * 1000; |
There was a problem hiding this comment.
🟡 Minor Magic number for lookback period
Issue: The 180-day lookback period is hardcoded without explanation. This wide time range may affect query performance on large ClickHouse tables.
Why: Without documentation, operators can't tune this value during performance issues. The 180-day window seems generous for a span lookup.
Fix: Extract to a named constant:
| const lookbackMs = 180 * 24 * 60 * 60 * 1000; | |
| const SPAN_LOOKUP_LOOKBACK_MS = 180 * 24 * 60 * 60 * 1000; // 180 days: covers full trace retention period |
There was a problem hiding this comment.
PR Review Summary
(0) New Issues | (3) Pending from Prior Review | Risk: High
Re-review scope: This review covers the delta since the last automated review (1 commit, 3 files, +11/-3 lines).
✅ Delta Assessment
The latest commit 74b0622d6 ("comments and fixes") contains no new issues. All changes are improvements:
-
signoz-queries.ts— Added missing operators toOPERATORSconstant (NOT_IN,REGEX,NOT_REGEX,LESS_THAN,GREATER_THAN,CONTAINS,NOT_CONTAINS). This restores parity with the internalOP_MAPand fixes a breaking change flagged in the prior review. -
conversations/[conversationId]/route.ts:1285— Fixed null-safety in cache lookup:- if (agentGenCache.has(activityId)) return agentGenCache.get(activityId)!; + if (agentGenCache.has(activityId)) return agentGenCache.get(activityId) ?? null;
The non-null assertion (
!) was unsafe since the cache storesnullvalues. Now correctly returnsnullwhen the cached value isnull. -
spans/[spanId]/route.ts:51— Simplified response path:- const results = json?.data?.data?.results ?? json?.data?.results; + const results = json?.data?.results ?? [];
Removes uncertain dual-path fallback in favor of the correct v5 response shape.
🕐 Pending Recommendations (3)
These critical and major issues were raised in the prior review and remain unaddressed:
-
🔴
signozHelpers.ts:8— SQL injection via unescapedtenantId/projectIdinterpolation inbuildSecurityExpression. Values are interpolated directly into filter expressions without escaping single quotes. -
🔴
signoz.ts:262— SQL injection via unescaped conversation IDs. TheconversationIdsarray from step-1 results is interpolated directly:conversationIds.map((id) => \'${id}'`).join(', ')` without escaping. -
🟠
signozHelpers.ts:29— Null pointer exception whenspec.filteris undefined. Line 29 accessesspec.filter.expressionwithout null check.
Quick fixes for the SQL injection issues:
// signozHelpers.ts:8 — use quoteValue from signoz-queries.ts
import { quoteValue } from '@inkeep/agents-core';
let expr = `${SERVICE_NAME_FILTER} AND ${SPAN_KEYS.TENANT_ID} = ${quoteValue(tenantId)}`;
if (projectId) expr += ` AND ${SPAN_KEYS.PROJECT_ID} = ${quoteValue(projectId)}`;
// signoz.ts:262 — escape conversation IDs
const convIdExpr = `${SPAN_KEYS.CONVERSATION_ID} IN (${conversationIds.map((id) => quoteValue(id)).join(', ')})`;
// signozHelpers.ts:29 — add null check
const existing = spec.filter?.expression ?? 'true';
spec.filter = { expression: `(${existing}) AND ${securityExpr}` };🚫 REQUEST CHANGES
Summary: The delta itself is clean — all 3 changes are improvements with no new issues introduced. However, the critical SQL injection vulnerabilities flagged in the prior review remain unaddressed in the unchanged files. These must be fixed before merge to ensure tenant data isolation. The fix is straightforward: import and use the quoteValue() helper (which now correctly uses standard SQL '' escaping) for all user-controlled string interpolation.
Discarded (0)
No findings discarded in this delta review.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator |
3 | 0 | 0 | 0 | 0 | 3 | 0 |
| Total | 3 | 0 | 0 | 0 | 0 | 3 | 0 |
Note: Delta-scoped review — sub-reviewers not dispatched for minimal changes. Prior critical issues tracked in Pending Recommendations.
Ito Test Report ❌20 test cases ran. 1 failed, 19 passed. Across 8 report shards covering 20 test cases, 19 passed and the traces platform was largely stable: core overview/detail routes, pagination/search fallback, span lookup side pane, analytics pages (AI/Tool Calls), auth and tenant/project scoping protections, rapid-interaction stress behavior, mobile usability, and multiple API validation guardrails all behaved as expected without crashes or data leakage. The single significant finding was a Medium-severity defect in GET /api/traces/conversations/[conversationId], where a whitespace-only conversationId (%20) is not trimmed/rejected and is incorrectly forwarded into downstream time-range/query logic (likely introduced by the PR), weakening endpoint contract reliability and adding avoidable operational noise. ❌ Failed (1)
🟠 Conversation details endpoint accepts whitespace-only conversation IDs
Relevant code:
const { conversationId } = await context.params;
if (!conversationId) {
return NextResponse.json({ error: 'Conversation ID is required' }, { status: 400 });
}
function buildBaseExpression(conversationId: string, projectId?: string): string {
const parts = [`${SPAN_KEYS.CONVERSATION_ID} = '${conversationId}'`];
if (projectId) parts.push(`${SPAN_KEYS.PROJECT_ID} = '${projectId}'`);
return parts.join(' AND ');
}
const bounds = await makeManagementApiRequest<{
data: { createdAt: string; updatedAt: string };
}>(`tenants/${tenantId}/projects/${projectId}/conversations/${conversationId}/bounds`);✅ Passed (19)
Commit: Tell us how we did: Give Ito Feedback |
There was a problem hiding this comment.
PR Review Summary
(8) Total Issues | Risk: High
🔴❗ Critical (2) ❗🔴
Inline Comments:
- 🔴 Critical:
signozHelpers.ts:8SQL injection via unescaped tenantId/projectId inbuildSecurityExpression() - 🔴 Critical:
signoz.ts:262SQL injection via unescaped conversation IDs from step-1 results
🟠⚠️ Major (2) 🟠⚠️
Inline Comments:
- 🟠 Major:
signozHelpers.ts:29Null pointer whenspec.filteris undefined - 🟠 Major:
signoz.ts:264Same null pointer issue when building conversation ID filter
🟡 Minor (3) 🟡
Inline Comments:
- 🟡 Minor:
signoz.ts:80queryReferencesKeysdoesn't checkorderByfields - 🟡 Minor:
signoz.ts:291Magic number for 180-day lookback period - 🟡 Minor:
conversations/[conversationId]/route.ts:152Unescaped conversationId in client-side filter (lower risk)
💭 Consider (1) 💭
Inline Comments:
- 💭 Consider:
conversations/[conversationId]/route.ts:458Whitespace-only conversationId validation (flagged by Ito tests)
🕐 Pending Recommendations (0)
All prior review issues have been incorporated into this review's findings.
🚫 REQUEST CHANGES
Summary: The v4→v5 SigNoz migration is architecturally sound with good improvements (parallel batching, query consolidation, -1458 net lines). However, critical SQL injection vulnerabilities must be addressed before merge:
signozHelpers.ts:8—tenantId/projectIdinterpolated directly into filter expressions without escaping single quotessignoz.ts:262—conversationIdsfrom step-1 results interpolated without escaping
Quick fix: Import and use the existing quoteValue() helper from signoz-queries.ts (which correctly uses standard SQL '' escaping) for all string interpolation:
import { quoteValue } from '@inkeep/agents-core';
// signozHelpers.ts:8
let expr = `${SERVICE_NAME_FILTER} AND ${SPAN_KEYS.TENANT_ID} = ${quoteValue(tenantId)}`;
// signoz.ts:262
const convIdExpr = `${SPAN_KEYS.CONVERSATION_ID} IN (${conversationIds.map((id) => quoteValue(String(id))).join(', ')})`;The null pointer issues at lines 29 and 264 also need addressing to prevent runtime crashes on queries without pre-existing filters.
Discarded (0)
No findings discarded in this review.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator |
8 | 0 | 0 | 0 | 8 | 0 | 0 |
| Total | 8 | 0 | 0 | 0 | 8 | 0 | 0 |
Note: Sub-reviewers unavailable — review performed directly by orchestrator.
| const SPAN_LOOKUP_TABLE = 'distributed_signoz_index_v3'; | ||
|
|
||
| function buildSecurityExpression(tenantId: string, projectId?: string): string { | ||
| let expr = `${SERVICE_NAME_FILTER} AND ${SPAN_KEYS.TENANT_ID} = '${tenantId}'`; |
There was a problem hiding this comment.
🔴 CRITICAL SQL injection via unescaped tenantId/projectId
Issue: The tenantId and projectId values are directly interpolated into the filter expression without escaping single quotes. While these values come from authenticated middleware context, a compromised or malformed value could inject arbitrary filter logic.
Why: A tenantId like test' OR 1=1 -- would break out of the string literal and inject arbitrary SQL/filter expressions. This could potentially allow cross-tenant data access.
Fix: Use the quoteValue() helper from signoz-queries.ts which correctly escapes single quotes:
import { quoteValue } from '@inkeep/agents-core';
function buildSecurityExpression(tenantId: string, projectId?: string): string {
let expr = `${SERVICE_NAME_FILTER} AND ${SPAN_KEYS.TENANT_ID} = ${quoteValue(tenantId)}`;
if (projectId) expr += ` AND ${SPAN_KEYS.PROJECT_ID} = ${quoteValue(projectId)}`;
return expr;
}Refs:
| ); | ||
| const securityExpr = buildSecurityExpression(tenantId, projectId); | ||
| for (const { spec } of queries) { | ||
| spec.filter = { expression: `(${spec.filter.expression}) AND ${securityExpr}` }; |
There was a problem hiding this comment.
🟠 MAJOR Null pointer when spec.filter is undefined
Issue: This line accesses spec.filter.expression without checking if spec.filter exists. If a query arrives without a pre-existing filter (which is valid in v5), this will throw Cannot read properties of undefined (reading 'expression').
Why: The v4 code was resilient to missing filters — it would create them from scratch. This regression breaks queries that don't have pre-existing filters.
Fix: Default to a truthy base expression:
| spec.filter = { expression: `(${spec.filter.expression}) AND ${securityExpr}` }; | |
| const existing = spec.filter?.expression ?? 'true'; | |
| spec.filter = { expression: `(${existing}) AND ${securityExpr}` }; |
Refs:
| ); | ||
| return c.json({ error: 'Internal Server Error', message: 'Failed to query SigNoz' }, 500); | ||
|
|
||
| const convIdExpr = `${SPAN_KEYS.CONVERSATION_ID} IN (${conversationIds.map((id) => `'${id}'`).join(', ')})`; |
There was a problem hiding this comment.
🔴 CRITICAL SQL injection via unescaped conversation IDs
Issue: The conversationIds array from step-1 SigNoz results is directly interpolated into the filter expression without escaping single quotes. While this data comes from SigNoz (not user input), a conversation ID containing a single quote would break the expression.
Why: Although conversation IDs are typically UUIDs, the defensive principle is to always escape user-controllable or external data. A malformed ID like conv' OR 1=1 -- could inject arbitrary filter logic.
Fix: Use the quoteValue() helper:
| const convIdExpr = `${SPAN_KEYS.CONVERSATION_ID} IN (${conversationIds.map((id) => `'${id}'`).join(', ')})`; | |
| const convIdExpr = `${SPAN_KEYS.CONVERSATION_ID} IN (${conversationIds.map((id) => quoteValue(String(id))).join(', ')})`; |
You'll need to import quoteValue from @inkeep/agents-core or extract it as a shared utility.
Refs:
|
|
||
| const convIdExpr = `${SPAN_KEYS.CONVERSATION_ID} IN (${conversationIds.map((id) => `'${id}'`).join(', ')})`; | ||
| for (const { spec } of detailPayloadTemplate.compositeQuery.queries) { | ||
| spec.filter = { expression: `(${spec.filter.expression}) AND ${convIdExpr}` }; |
There was a problem hiding this comment.
🟠 MAJOR Same null pointer issue as enforceSecurityFilters
Issue: This line accesses spec.filter.expression without checking if spec.filter exists. Same issue as in signozHelpers.ts:29.
Fix: Use optional chaining with a default:
| spec.filter = { expression: `(${spec.filter.expression}) AND ${convIdExpr}` }; | |
| const existing = spec.filter?.expression ?? 'true'; | |
| spec.filter = { expression: `(${existing}) AND ${convIdExpr}` }; |
| ...(spec?.groupBy ?? []).map((g: any) => g.name), | ||
| ]; | ||
| return keys.some((k) => searchable.some((t: string) => t.includes(k))); | ||
| } |
There was a problem hiding this comment.
🟡 Minor queryReferencesKeys doesn't check orderBy fields
Issue: The function only checks filter.expression, selectFields, and groupBy for missing key references. If a missing attribute is referenced only in order, the query won't be filtered out and the retry will fail with the same error.
Why: SigNoz builder queries can reference attributes in order clauses. If the missing key error originates from an orderBy reference, the retry logic will fail to identify and remove the problematic query.
Fix: Add order fields to the searchable array:
const searchable = [
spec?.filter?.expression ?? '',
...(spec?.selectFields ?? []).map((f: any) => f.name),
...(spec?.groupBy ?? []).map((g: any) => g.name),
...(spec?.order ?? []).map((o: any) => o.key?.name ?? o.columnName ?? ''),
];| return c.json({ error: 'Service Unavailable', message: 'SigNoz is not configured' }, 500); | ||
|
|
||
| const now = Date.now(); | ||
| const lookbackMs = 180 * 24 * 60 * 60 * 1000; |
There was a problem hiding this comment.
🟡 Minor Magic number for 180-day lookback period
Issue: The 180-day lookback period is hardcoded without explanation. This wide time range may affect query performance on large ClickHouse tables.
Why: Without documentation, operators can't tune this value during performance issues. The 180-day window seems generous for a span lookup.
Fix: Extract to a named constant with documentation:
/** Maximum lookback for span lookup queries. Covers typical trace retention period. */
const SPAN_LOOKUP_LOOKBACK_MS = 180 * 24 * 60 * 60 * 1000; // 180 days| const bool = FIELD_DATA_TYPES.BOOL; | ||
|
|
||
| function buildBaseExpression(conversationId: string, projectId?: string): string { | ||
| const parts = [`${SPAN_KEYS.CONVERSATION_ID} = '${conversationId}'`]; |
There was a problem hiding this comment.
🟡 Minor Unescaped conversationId in filter expression (lower risk)
Issue: The conversationId from URL path params is interpolated directly into the filter expression without escaping. A value like test' OR 1=1 -- would inject arbitrary filter logic.
Why: This is lower risk than the server-side issues because:
- The payload goes through
enforceSecurityFilters()on the server which scopes to the correct tenant - A user can only broaden their query scope within their own tenant
However, it still allows a user to potentially fetch data for a different conversation within their tenant by manipulating the filter.
Fix: Consider using the buildFilterExpression() helper from @/constants/signoz or escaping manually:
function buildBaseExpression(conversationId: string, projectId?: string): string {
const escapedConvId = conversationId.replace(/'/g, "''");
const parts = [`${SPAN_KEYS.CONVERSATION_ID} = '${escapedConvId}'`];
if (projectId) {
const escapedProjId = projectId.replace(/'/g, "''");
parts.push(`${SPAN_KEYS.PROJECT_ID} = '${escapedProjId}'`);
}
return parts.join(' AND ');
}
There was a problem hiding this comment.
PR Review Summary
(7) Total Issues | Risk: High
🔴❗ Critical (2) ❗🔴
Inline Comments:
- 🔴 Critical:
signozHelpers.ts:8SQL injection via unescaped tenantId/projectId inbuildSecurityExpression() - 🔴 Critical:
signoz.ts:262SQL injection via unescaped conversation IDs from step-1 results
🟠⚠️ Major (2) 🟠⚠️
Inline Comments:
- 🟠 Major:
signozHelpers.ts:29Null pointer whenspec.filteris undefined - 🟠 Major:
signoz.ts:264Same null pointer issue when building conversation ID filter
🟡 Minor (2) 🟡
Inline Comments:
- 🟡 Minor:
signoz.ts:72-80queryReferencesKeysdoesn't checkorderByfields - 🟡 Minor:
signoz.ts:291Magic number for 180-day lookback period
💭 Consider (1) 💭
Inline Comments:
- 💭 Consider:
conversations/[conversationId]/route.ts:460-463Whitespace-only conversationId validation (flagged by Ito tests)
🚫 REQUEST CHANGES
Summary: The v4→v5 SigNoz migration is architecturally sound with excellent improvements:
- Parallel batching (3 batches via
Promise.all) - Query consolidation (reduced from ~16 individual queries)
- Net -1458 lines of code
- Good defensive retry logic for missing attributes
However, critical SQL injection vulnerabilities must be addressed before merge:
Quick Fixes Required
-
Export
quoteValuefrom@inkeep/agents-core— The function atsignoz-queries.ts:53-57correctly escapes single quotes with''but is currently private. Export it:// In signoz-queries.ts export function quoteValue(value: unknown): string { if (typeof value === 'string') return `'${value.replace(/'/g, "''")}'`; if (typeof value === 'boolean') return String(value); return String(value); }
-
Use
quoteValueinsignozHelpers.ts:8:import { quoteValue, SPAN_KEYS, QUERY_TYPES } from '@inkeep/agents-core'; let expr = `${SERVICE_NAME_FILTER} AND ${SPAN_KEYS.TENANT_ID} = ${quoteValue(tenantId)}`; if (projectId) expr += ` AND ${SPAN_KEYS.PROJECT_ID} = ${quoteValue(projectId)}`;
-
Use
quoteValueinsignoz.ts:262:const convIdExpr = `${SPAN_KEYS.CONVERSATION_ID} IN (${conversationIds.map((id) => quoteValue(String(id))).join(', ')})`;
-
Add null checks for
spec.filterat lines 29 and 264:const existing = spec.filter?.expression ?? 'true'; spec.filter = { expression: `(${existing}) AND ${securityExpr}` };
Risk Assessment
While the SQL injection vectors have reduced exploitability (values come from authenticated middleware or SigNoz responses, not direct user input), defense-in-depth requires escaping all interpolated values. The null pointer issues are more immediately impactful as they can cause runtime crashes.
Discarded (0)
No findings discarded in this review.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator |
7 | 0 | 0 | 0 | 7 | 0 | 0 |
| Total | 7 | 0 | 0 | 0 | 7 | 0 | 0 |
Note: This is a re-review consolidating findings from prior automated reviews. Sub-reviewers were not dispatched as the critical issues have been consistently identified across multiple prior review runs.
| const SPAN_LOOKUP_TABLE = 'distributed_signoz_index_v3'; | ||
|
|
||
| function buildSecurityExpression(tenantId: string, projectId?: string): string { | ||
| let expr = `${SERVICE_NAME_FILTER} AND ${SPAN_KEYS.TENANT_ID} = '${tenantId}'`; |
There was a problem hiding this comment.
🔴 CRITICAL SQL injection via unescaped tenantId/projectId
Issue: The tenantId and projectId values are directly interpolated into the filter expression without escaping single quotes. A value containing ' would break out of the string literal.
Why: While these values come from authenticated middleware context (reducing exploitability), a compromised or malformed value could inject arbitrary filter logic, potentially enabling cross-tenant data access. Defense-in-depth requires escaping all interpolated values.
Fix: Use the quoteValue() helper from signoz-queries.ts which correctly escapes single quotes with standard SQL '':
| let expr = `${SERVICE_NAME_FILTER} AND ${SPAN_KEYS.TENANT_ID} = '${tenantId}'`; | |
| let expr = `${SERVICE_NAME_FILTER} AND ${SPAN_KEYS.TENANT_ID} = ${quoteValue(tenantId)}`; | |
| if (projectId) expr += ` AND ${SPAN_KEYS.PROJECT_ID} = ${quoteValue(projectId)}`; |
You'll need to export quoteValue from @inkeep/agents-core and import it here.
Refs:
| ); | ||
| const securityExpr = buildSecurityExpression(tenantId, projectId); | ||
| for (const { spec } of queries) { | ||
| spec.filter = { expression: `(${spec.filter.expression}) AND ${securityExpr}` }; |
There was a problem hiding this comment.
🟠 MAJOR Null pointer when spec.filter is undefined
Issue: This line accesses spec.filter.expression without checking if spec.filter exists. If a query arrives without a pre-existing filter (which is valid in v5), this will throw Cannot read properties of undefined (reading 'expression').
Why: The v4 code was resilient to missing filters. This regression breaks queries that don't have pre-existing filters, causing runtime crashes.
Fix: Default to a truthy base expression:
| spec.filter = { expression: `(${spec.filter.expression}) AND ${securityExpr}` }; | |
| const existing = spec.filter?.expression ?? 'true'; | |
| spec.filter = { expression: `(${existing}) AND ${securityExpr}` }; |
Refs:
| ); | ||
| return c.json({ error: 'Internal Server Error', message: 'Failed to query SigNoz' }, 500); | ||
|
|
||
| const convIdExpr = `${SPAN_KEYS.CONVERSATION_ID} IN (${conversationIds.map((id) => `'${id}'`).join(', ')})`; |
There was a problem hiding this comment.
🔴 CRITICAL SQL injection via unescaped conversation IDs
Issue: The conversationIds array from step-1 SigNoz results is directly interpolated into the filter expression without escaping single quotes. A conversation ID containing ' would break out of the string literal.
Why: Although conversation IDs are typically UUIDs from SigNoz responses, the defensive principle is to always escape external data. A malformed ID like conv' OR 1=1 -- could inject arbitrary filter logic.
Fix: Use the quoteValue() helper (you'll need to export it from @inkeep/agents-core):
| const convIdExpr = `${SPAN_KEYS.CONVERSATION_ID} IN (${conversationIds.map((id) => `'${id}'`).join(', ')})`; | |
| const convIdExpr = `${SPAN_KEYS.CONVERSATION_ID} IN (${conversationIds.map((id) => quoteValue(String(id))).join(', ')})`; |
Refs:
|
|
||
| const convIdExpr = `${SPAN_KEYS.CONVERSATION_ID} IN (${conversationIds.map((id) => `'${id}'`).join(', ')})`; | ||
| for (const { spec } of detailPayloadTemplate.compositeQuery.queries) { | ||
| spec.filter = { expression: `(${spec.filter.expression}) AND ${convIdExpr}` }; |
There was a problem hiding this comment.
🟠 MAJOR Same null pointer issue as enforceSecurityFilters
Issue: This line accesses spec.filter.expression without checking if spec.filter exists. Same issue as in signozHelpers.ts:29.
Why: If a query in detailPayloadTemplate lacks a pre-existing filter, this will throw a runtime error.
Fix: Use optional chaining with a default:
| spec.filter = { expression: `(${spec.filter.expression}) AND ${convIdExpr}` }; | |
| const existing = spec.filter?.expression ?? 'true'; | |
| spec.filter = { expression: `(${existing}) AND ${convIdExpr}` }; |
| function queryReferencesKeys(query: any, keys: string[]): boolean { | ||
| const spec = query?.spec; | ||
| const searchable = [ | ||
| spec?.filter?.expression ?? '', | ||
| ...(spec?.selectFields ?? []).map((f: any) => f.name), | ||
| ...(spec?.groupBy ?? []).map((g: any) => g.name), | ||
| ]; | ||
| return keys.some((k) => searchable.some((t: string) => t.includes(k))); | ||
| } |
There was a problem hiding this comment.
🟡 Minor queryReferencesKeys doesn't check orderBy fields
Issue: The function only checks filter.expression, selectFields, and groupBy for missing key references. If a missing attribute is referenced only in order, the query won't be filtered out and the retry will fail with the same error.
Why: SigNoz builder queries can reference attributes in order clauses. If the missing key error originates from an orderBy reference, the retry logic will fail to identify and remove the problematic query.
Fix: Add order fields to the searchable array:
const searchable = [
spec?.filter?.expression ?? '',
...(spec?.selectFields ?? []).map((f: any) => f.name),
...(spec?.groupBy ?? []).map((g: any) => g.name),
...(spec?.order ?? []).map((o: any) => o.key?.name ?? o.columnName ?? ''),
];| return c.json({ error: 'Service Unavailable', message: 'SigNoz is not configured' }, 500); | ||
|
|
||
| const now = Date.now(); | ||
| const lookbackMs = 180 * 24 * 60 * 60 * 1000; |
There was a problem hiding this comment.
🟡 Minor Magic number for 180-day lookback period
Issue: The 180-day lookback period is hardcoded without explanation. This wide time range may affect query performance on large ClickHouse tables.
Why: Without documentation, operators can't tune this value during performance issues.
Fix: Extract to a named constant with documentation:
| const lookbackMs = 180 * 24 * 60 * 60 * 1000; | |
| /** Maximum lookback for span lookup queries. Covers typical trace retention period. */ | |
| const SPAN_LOOKUP_LOOKBACK_MS = 180 * 24 * 60 * 60 * 1000; // 180 days | |
| const lookbackMs = SPAN_LOOKUP_LOOKBACK_MS; |
| function buildBaseExpression(conversationId: string, projectId?: string): string { | ||
| const parts = [`${SPAN_KEYS.CONVERSATION_ID} = '${conversationId}'`]; | ||
| if (projectId) parts.push(`${SPAN_KEYS.PROJECT_ID} = '${projectId}'`); | ||
| return parts.join(' AND '); | ||
| } |
There was a problem hiding this comment.
🟡 Minor Unescaped conversationId in filter expression
Issue: The conversationId from URL path params is interpolated directly into the filter expression without escaping single quotes. A value like test' OR 1=1 -- would inject arbitrary filter logic.
Why: This is lower risk than the server-side issues because:
- The payload goes through
enforceSecurityFilters()on the server which scopes to the correct tenant - A user can only broaden their query scope within their own tenant
However, it still allows a user to potentially fetch data for a different conversation within their tenant by manipulating the filter.
Fix: Consider escaping manually:
function buildBaseExpression(conversationId: string, projectId?: string): string {
const escapedConvId = conversationId.replace(/'/g, "''");
const parts = [`${SPAN_KEYS.CONVERSATION_ID} = '${escapedConvId}'`];
if (projectId) {
const escapedProjId = projectId.replace(/'/g, "''");
parts.push(`${SPAN_KEYS.PROJECT_ID} = '${escapedProjId}'`);
}
return parts.join(' AND ');
}
Ito Test Report ❌20 test cases ran. 1 failed, 19 passed. Overall, 19 of 20 test cases passed, confirming strong traces functionality across authenticated and unauthenticated routing, overview/detail rendering, span drill-down, deep-link/back-forward stability, multi-tab isolation, mobile flow, proxy error recovery, and API guardrails (including deterministic 400/404 validation and forbidden access behavior). The single notable issue was a Medium-severity, pre-existing client-side race in the traces overview where rapid filter/search/pagination interactions can let out-of-order responses overwrite newer state and show stale rows/pagination, which risks misleading debugging decisions. ❌ Failed (1)
🟠 Rapid interactions can overwrite newer traces state with stale responses
Relevant code:
const fetchData = useCallback(
async (page: number) => {
try {
setLoading(true);
setError(null);
const client = getSigNozStatsClient(options?.tenantId);
const result = await client.getConversationStats(
currentStartTime,
currentEndTime,
options?.filters,
options?.projectId,
{ page, limit: pageSize },
options?.searchQuery,
options?.agentId,
options?.hasErrors
);
setStats(result.data);
setPaginationInfo(result.pagination);
const nextPage = useCallback(() => {
if (paginationInfo?.hasNextPage) {
const nextPageNum = currentPage + 1;
setCurrentPage(nextPageNum);
fetchData(nextPageNum);
}
}, [currentPage, paginationInfo?.hasNextPage, fetchData]);
useEffect(() => {
setCurrentPage(1);
fetchData(1);
}, [
options?.startTime,
options?.endTime,
options?.filters,
options?.searchQuery,
options?.hasErrors,
pageSize,
]);✅ Passed (19)
Commit: Tell us how we did: Give Ito Feedback |
1 participant
No description provided.