Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 74b0622 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 ｜ 10 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 |
1 participant
No description provided.