🔧(qa-agent): Limit test case generation to 10 for debugging LangSmith tracing issue #3635
Conversation
Added temporary limitation to process only the first 10 requirements in parallel to investigate LangSmith tracing issues where 1-2 parallel processes don't complete properly. This debug change will help isolate whether the issue is related to the number of concurrent operations. TODO: Remove this limitation after the tracing issue is resolved. Issue: route06/liam-internal#5747 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
|
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
Updates to Preview Branch (trace-10-5747) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
🤖 Agent Deep Modeling ExecutionProcessing time: 17m 4s Command Output
RUN v3.2.4 /home/runner/work/liam/liam/frontend/internal-packages/agent (node:7555) ExperimentalWarning: WASI is an experimental feature and might change at any time ✅ [INFO] 2025-09-30T08:31:29.116Z ⏺ (pm): Plan: Produce a concise checklist, analyze and structure the Business Requirements Document with clear, testable functional requirements and data managed, then save the requirements artifact for downstream DB and QA work. 🔧 saveRequirementsToArtifactTool(businessRequirement: "Design a simple u...", functionalRequirements: {11 keys}) ⎿ unknown: Requirements saved successfully to artifact 🔧 schemaDesignTool(operations: [15 items]) 🔧 schemaDesignTool(operations: [12 items]) 🔧 schemaDesignTool(operations: [12 items]) 🔧 schemaDesignTool(operations: [3 items]) 🔧 schemaDesignTool(operations: [12 items]) 🔧 schemaDesignTool(operations: [12 items]) 🔧 schemaDesignTool(operations: [11 items])stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow 🔧 schemaDesignTool(operations: [11 items])stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow 🔧 schemaDesignTool(operations: [11 items])stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow 🔧 saveTestcase(testcaseWithDml: {6 keys}) 🔧 saveTestcase(testcaseWithDml: {6 keys}) 🔧 saveTestcase(testcaseWithDml: {6 keys}) 🔧 saveTestcase(testcaseWithDml: {6 keys}) ⏺ (qa): functions.saveTestcase to=functions.saveTestcase ️ 🔧 saveTestcase(testcaseWithDml: {6 keys}) 🔧 saveTestcase(testcaseWithDml: {6 keys}) 🔧 saveTestcase(testcaseWithDml: {6 keys}) 🔧 saveTestcase(testcaseWithDml: {6 keys}) 🔧 saveTestcase(testcaseWithDml: {6 keys}) 🔧 saveTestcase(testcaseWithDml: {6 keys}) 🔧 saveTestcase(testcaseWithDml: {6 keys}) ⏺ (qa): { 🔧 saveTestcase(testcaseWithDml: {6 keys}) 🔧 saveTestcase(testcaseWithDml: {6 keys}) 🔧 saveTestcase(testcaseWithDml: {6 keys}) 🔧 saveTestcase(testcaseWithDml: {6 keys}) 🔧 saveTestcase(testcaseWithDml: {6 keys}) 🔧 saveTestcase(testcaseWithDml: {6 keys}) 🔧 saveTestcase(testcaseWithDml: {6 keys}) 🔧 saveTestcase(testcaseWithDml: {6 keys}) ⏺ (qa): { 🔧 saveTestcase(testcaseWithDml: {6 keys}) 🔧 saveTestcase(testcaseWithDml: {6 keys}) 🔧 saveTestcase(testcaseWithDml: {6 keys}) 🔧 saveTestcase(testcaseWithDml: {6 keys}) 🔧 saveTestcase(testcaseWithDml: {6 keys}) 🔧 saveTestcase(testcaseWithDml: {6 keys}) ⏺ (qa): { 🔧 saveTestcase(testcaseWithDml: {6 keys}) 🔧 saveTestcase(testcaseWithDml: {6 keys}) 🔧 saveTestcase(testcaseWithDml: {6 keys}) 🔧 saveTestcase(testcaseWithDml: {6 keys}) 🔧 saveTestcase(testcaseWithDml: {6 keys}) ⏺ (qa): { 🔧 saveTestcase(testcaseWithDml: {6 keys}) 🔧 saveTestcase(testcaseWithDml: {6 keys}) 🔧 saveTestcase(testcaseWithDml: {6 keys}) ⏺ (qa): { 🔧 saveTestcase(testcaseWithDml: {6 keys}) 🔧 saveTestcase(testcaseWithDml: {6 keys}) 🔧 saveTestcase(testcaseWithDml: {6 keys}) 🔧 saveTestcase(testcaseWithDml: {6 keys}) 🔧 saveTestcase(testcaseWithDml: {6 keys}) ⏺ (qa): { ⏺ (qa): { 🔧 saveTestcase(testcaseWithDml: {6 keys}) 🔧 saveTestcase(testcaseWithDml: {6 keys}) 🔧 saveTestcase(testcaseWithDml: {6 keys}) 🔧 saveTestcase(testcaseWithDml: {6 keys}) 🔧 saveTestcase(testcaseWithDml: {6 keys}) 🔧 saveTestcase(testcaseWithDml: {6 keys})stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stderr | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stderr | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stderr | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stderr | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stderr | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stderr | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stderr | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stderr | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stderr | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stderr | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stderr | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stderr | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stderr | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stderr | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stderr | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stderr | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stderr | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stderr | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stderr | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stderr | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stderr | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stderr | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stderr | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stderr | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stderr | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stderr | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow stdout | src/createGraph.integration.test.ts > createGraph Integration > should execute complete workflow ⏺ (lead): Here’s a concise recap of what was accomplished in this database design session:
If you’d like, I can distill this into:
Test Files 1 passed (1) ✅ Status: Completed successfully Finished at: 2025-09-30 08:48:30 UTC |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/internal-packages/agent/src/qa-agent/distributeRequirements/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Name utility files in camelCase (e.g., mergeSchema.ts)
Files:
frontend/internal-packages/agent/src/qa-agent/distributeRequirements/index.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript/TSX across the codebase
**/*.{ts,tsx}: Prefer early returns for readability
Use named exports only (no default exports)
Prefer const arrow functions over function declarations for simple utilities (e.g., const toggle = () => {})
Files:
frontend/internal-packages/agent/src/qa-agent/distributeRequirements/index.ts
frontend/internal-packages/**
📄 CodeRabbit inference engine (AGENTS.md)
Infra and tooling (e2e, configs, storybook, agent) live under frontend/internal-packages
Files:
frontend/internal-packages/agent/src/qa-agent/distributeRequirements/index.ts
frontend/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Follow existing import patterns and tsconfig path aliases
Files:
frontend/internal-packages/agent/src/qa-agent/distributeRequirements/index.ts
🧠 Learnings (1)
📚 Learning: 2025-09-12T11:37:37.587Z
Learnt from: NoritakaIkeda
PR: liam-hq/liam#3463
File: frontend/internal-packages/agent/src/qa-agent/testcaseGeneration/saveToolNode.ts:12-22
Timestamp: 2025-09-12T11:37:37.587Z
Learning: In the frontend/internal-packages/agent codebase, state.currentRequirement.requirementId is guaranteed to always be present, so guard checks for missing requirementId are unnecessary in saveToolNode.ts.
Applied to files:
frontend/internal-packages/agent/src/qa-agent/distributeRequirements/index.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Supabase Preview
- GitHub Check: codeql / languages (javascript) / Perform CodeQL for javascript
- GitHub Check: frontend-ci
- GitHub Check: frontend-lint
- GitHub Check: agent-deep-modeling
- GitHub Check: Supabase Preview
frontend/internal-packages/agent/src/qa-agent/distributeRequirements/index.ts
Outdated
Show resolved
Hide resolved
This reverts commit 1b08122.
Replace simple array slicing with p-limit semaphore to control concurrent executions while processing all requirements. This approach: - Adds p-limit dependency for semaphore implementation - Wraps generateTestcaseNode with a semaphore limiting to 10 concurrent executions - Removes the temporary slice(0, 10) limitation from distributeRequirements - Allows processing all requirements while preventing LangSmith tracing issues The Send API still creates parallel tasks for all requirements, but the actual LLM calls are now throttled to max 10 concurrent executions. Issue: route06/liam-internal#5747 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
frontend/internal-packages/agent/package.json (1)
27-27: Adding p-limit is fine for this debug throttle. Consider a clean removal path.Ship it for now. To ease rollback/tuning without another deploy, couple this with an env-driven limit (and add a TODO reference to route06/liam-internal#5747 in code). Also ensure the lockfile includes this new dep.
frontend/internal-packages/agent/src/qa-agent/testcaseGeneration/generateTestcaseNode.ts (2)
30-33: Make the limit configurable and add an explicit TODO reference.Hard-coding 10 works, but an env override lets you tune quickly while debugging and revert without code changes. Also add a TODO linked to route06/liam-internal#5747.
-// Create a semaphore to limit concurrent executions to 10 -// This helps prevent LangSmith tracing issues with too many parallel processes -const limit = pLimit(10) +// Create a semaphore to limit concurrent executions (default 10). +// TODO(route06/liam-internal#5747): Remove once LangSmith tracing issue is resolved. +// You can override via QA_AGENT_TESTCASE_CONCURRENCY for debugging. +const TESTCASE_CONCURRENCY = + Number.parseInt(process.env.QA_AGENT_TESTCASE_CONCURRENCY ?? '10', 10) +const limit = pLimit(Number.isFinite(TESTCASE_CONCURRENCY) && TESTCASE_CONCURRENCY > 0 + ? TESTCASE_CONCURRENCY + : 10)
42-93: Good placement of the throttle; add lightweight debug counters for observability.Wrapping at the node boundary is the right granularity. For the current investigation, brief DEBUG logs of
activeCount/pendingCountwill help correlate with LangSmith traces.- // Use semaphore to limit concurrent executions - return limit(async () => { - const { currentRequirement, schemaData, messages } = state + // Use semaphore to limit concurrent executions + return limit(async () => { + const { currentRequirement, schemaData, messages } = state + if (process.env.LOG_LEVEL === 'DEBUG') { + // p-limit exposes live gauges + console.debug( + `[qa-agent] generateTestcaseNode start req=${currentRequirement?.id ?? 'unknown'} ` + + `active=${limit.activeCount} pending=${limit.pendingCount}`, + ) + } const schemaContext = convertSchemaToText(schemaData) @@ - const streamResult = await streamModel() + const streamResult = await streamModel() @@ - return { - messages: [response], - } + const result = { messages: [response] } + if (process.env.LOG_LEVEL === 'DEBUG') { + console.debug( + `[qa-agent] generateTestcaseNode end req=${currentRequirement?.id ?? 'unknown'} ` + + `active=${limit.activeCount} pending=${limit.pendingCount}`, + ) + } + return result })
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
frontend/internal-packages/agent/package.json(1 hunks)frontend/internal-packages/agent/src/qa-agent/distributeRequirements/index.ts(1 hunks)frontend/internal-packages/agent/src/qa-agent/testcaseGeneration/generateTestcaseNode.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/internal-packages/agent/src/qa-agent/distributeRequirements/index.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Name utility files in camelCase (e.g., mergeSchema.ts)
Files:
frontend/internal-packages/agent/src/qa-agent/testcaseGeneration/generateTestcaseNode.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript/TSX across the codebase
**/*.{ts,tsx}: Prefer early returns for readability
Use named exports only (no default exports)
Prefer const arrow functions over function declarations for simple utilities (e.g., const toggle = () => {})
Files:
frontend/internal-packages/agent/src/qa-agent/testcaseGeneration/generateTestcaseNode.ts
frontend/internal-packages/**
📄 CodeRabbit inference engine (AGENTS.md)
Infra and tooling (e2e, configs, storybook, agent) live under frontend/internal-packages
Files:
frontend/internal-packages/agent/src/qa-agent/testcaseGeneration/generateTestcaseNode.tsfrontend/internal-packages/agent/package.json
frontend/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Follow existing import patterns and tsconfig path aliases
Files:
frontend/internal-packages/agent/src/qa-agent/testcaseGeneration/generateTestcaseNode.ts
🧬 Code graph analysis (1)
frontend/internal-packages/agent/src/qa-agent/testcaseGeneration/generateTestcaseNode.ts (4)
frontend/internal-packages/agent/src/utils/convertSchemaToText.ts (1)
convertSchemaToText(31-43)frontend/internal-packages/agent/src/qa-agent/testcaseGeneration/prompts.ts (2)
humanPromptTemplateForTestcaseGeneration(57-71)SYSTEM_PROMPT_FOR_TESTCASE_GENERATION(44-52)frontend/internal-packages/agent/src/utils/messageCleanup.ts (1)
removeReasoningFromMessages(55-59)frontend/internal-packages/agent/src/utils/streamingLlmUtils.ts (1)
streamLLMResponse(79-119)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Supabase Preview
- GitHub Check: codeql / languages (javascript) / Perform CodeQL for javascript
- GitHub Check: frontend-ci
- GitHub Check: frontend-lint
- GitHub Check: agent-deep-modeling
- GitHub Check: Supabase Preview
frontend/internal-packages/agent/src/qa-agent/testcaseGeneration/generateTestcaseNode.ts
Outdated
Show resolved
Hide resolved
Use namespace import pattern instead of default import since p-limit v3 is ESM-only and the tsconfig doesn't have esModuleInterop enabled. Changed: - import pLimit from 'p-limit' → import * as pLimit from 'p-limit' - pLimit(10) → pLimit.default(10) 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
Set max_concurrency: 10 in the graph invocation config to limit parallel execution at the LangGraph framework level. This approach: - Uses LangGraph's official concurrent execution limit feature - Applies to Send API parallel task distribution - Works alongside the p-limit semaphore for double protection - Should help prevent LangSmith tracing issues Applied in: - createGraph.ts: callQaAgent function - createQaAgentGraph.integration.test.ts: test invocation Issue: route06/liam-internal#5747 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
frontend/internal-packages/agent/src/tools/runTestTool.ts (1)
48-61: Remove unreachable undefined testcase guard.Based on the type definition in the relevant code snippets,
testcasesis declared asTestcase[](non-nullable array). The guard at lines 48-61 checking for!testcaseand returning a dummy result is unreachable dead code, as confirmed by a past review comment.Remove the unreachable branch:
const results = await Promise.all( testcases.map(async (testcase, i) => { - if (!testcase) { - // Return empty result for undefined testcases - const dummyResult: TestcaseDmlExecutionResult = { - testCaseId: '', - testCaseTitle: 'Undefined', - success: false, - executedAt: new Date(), - failedOperation: { - sql: '', - error: 'Testcase is undefined', - }, - } - return dummyResult - } - console.info( `[runTestTool] Starting testcase ${i + 1}/${testcases.length}: ${testcase.id}`, )Also remove the filter at line 119 that depends on this dummy result:
- // Filter out dummy results from undefined testcases - return results.filter((r) => r.testCaseId !== '') + return resultsfrontend/internal-packages/pglite-server/src/PGliteInstanceManager.ts (5)
16-19: PR objective mismatch: Default pool size is 24, not 10.The PR title and description specify limiting to 10 for debugging LangSmith tracing (route06/liam-internal#5747), but the default pool size is 24. Additionally, there are no DEBUG or TODO markers indicating this is a temporary debugging change that should be reverted.
Update to match the PR objective:
- // Pool size configurable via environment variable (default: 24 for maximum parallelism) + // DEBUG: Temporary pool size for LangSmith tracing debugging (route06/liam-internal#5747) + // TODO: Revert to original value after tracing issue is resolved + // Pool size configurable via environment variable (default: 10 for debugging) private static readonly POOL_SIZE = Number( - process.env['PGLITE_POOL_SIZE'] || '24', + process.env['PGLITE_POOL_SIZE'] || '10', )
80-98: Add error handling to prevent partially initialized pool.As flagged in a past review, if
createInstancethrows during initialization, the pool arrays will be left partially populated with mismatched lengths. Subsequent calls togetOrCreateInstancecould returnundefinedor throw at line 157.Wrap the initialization loop in try/catch with cleanup:
private async initializePool(requiredExtensions: string[]): Promise<void> { console.info( `[PGlite] Initializing pool with ${PGliteInstanceManager.POOL_SIZE} instances`, ) + try { for (let i = 0; i < PGliteInstanceManager.POOL_SIZE; i++) { console.info( `[PGlite] Creating instance ${i + 1}/${PGliteInstanceManager.POOL_SIZE}`, ) const { db, supportedExtensions } = await this.createInstance(requiredExtensions) PGliteInstanceManager.instancePool.push(db) PGliteInstanceManager.supportedExtensionsPool.push(supportedExtensions) PGliteInstanceManager.loadedExtensionsPool.push(requiredExtensions) } + } catch (error) { + // Clean up any instances created before the error + for (const instance of PGliteInstanceManager.instancePool) { + await instance.close().catch(() => {}) // Ignore close errors + } + PGliteInstanceManager.instancePool = [] + PGliteInstanceManager.supportedExtensionsPool = [] + PGliteInstanceManager.loadedExtensionsPool = [] + throw error + } console.info('[PGlite] Pool initialization complete') }
103-119: Handle errors during pool recreation.As flagged in a past review, if
instance.close()orinitializePoolfails during recreation, the pool could be left empty or partially populated, breaking all subsequent queries.Add error handling for close operations:
private async recreatePool(requiredExtensions: string[]): Promise<void> { console.info('[PGlite] Extensions changed, recreating pool') // Close all existing instances for (const instance of PGliteInstanceManager.instancePool) { - await instance.close() + await instance.close().catch((error) => { + console.error('[PGlite] Error closing instance during pool recreation:', error) + }) } // Clear the pools PGliteInstanceManager.instancePool = [] PGliteInstanceManager.supportedExtensionsPool = [] PGliteInstanceManager.loadedExtensionsPool = [] PGliteInstanceManager.currentIndex = 0 // Reinitialize with new extensions await this.initializePool(requiredExtensions) }
169-173: DDL detection is incomplete and may misclassify statements.As flagged in a past review, the current string prefix matching approach has several limitations:
- Missing DDL keywords:
COMMENT,GRANT,REVOKE,CREATE INDEX,CREATE VIEW, etc.TRUNCATEcan be transactional in PostgreSQL- Mixed statements like
CREATE TABLE AS SELECTcontain DML but are detected as DDL- Could match keywords in comments or string literals
- Only checks first statement if
filteredSqlcontains multiple statementsUse AST-based detection for accuracy:
private isDDLStatement(stmt: RawStmt): boolean { // Use the parsed statement type from the AST const stmtType = Object.keys(stmt.stmt)[0] const ddlTypes = [ 'CreateStmt', 'AlterTableStmt', 'DropStmt', 'CreateExtensionStmt', 'IndexStmt', 'ViewStmt', 'GrantStmt', 'RevokeStmt', 'CommentStmt' ] return ddlTypes.includes(stmtType) }Then update
executeQueryto parse and check each statement individually rather than checking the entire SQL string.
207-235: Transaction logic assumes uniform DDL or DML, but SQL may contain mixed statements.As flagged in a past review, the code checks
isDDLStatement(filteredSql)once and applies the same transaction strategy to all statements. However, a single SQL string can contain multiple statements—some DDL, some DML. If mixed:
- Classifying as DDL → DML statements execute outside a transaction
- Classifying as DML → DDL statements execute inside
BEGIN/ROLLBACK, which may failParse and handle each statement individually:
async executeQuery( sql: string, requiredExtensions: string[], ): Promise<SqlResult[]> { // ... memory logging ... const { db, supportedExtensions } = await this.getOrCreateInstance(requiredExtensions) const filteredSql = filterExtensionDDL(sql, supportedExtensions) try { // Parse to get individual statements const parseResult = await pgParse(filteredSql) if (parseResult.error) { return [this.createParseErrorResult(filteredSql, parseResult.error.message)] } const statements = this.extractStatements(filteredSql, parseResult.parse_tree.stmts) const results: SqlResult[] = [] for (let i = 0; i < statements.length; i++) { const stmt = statements[i] const rawStmt = parseResult.parse_tree.stmts[i] const isDDL = rawStmt ? this.isDDLStatement(rawStmt) : false if (isDDL) { console.info('[PGlite] Executing DDL without transaction') const result = await this.executeSingleStatement(stmt, db) results.push(result) } else { console.info('[PGlite] Executing DML with transaction isolation') await db.query('BEGIN') try { const result = await this.executeSingleStatement(stmt, db) results.push(result) await db.query('ROLLBACK') } catch (error) { await db.query('ROLLBACK') throw error } } } return results } finally { // ... memory logging ... } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/internal-packages/agent/src/tools/runTestTool.ts(1 hunks)frontend/internal-packages/pglite-server/README.md(1 hunks)frontend/internal-packages/pglite-server/src/PGliteInstanceManager.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/internal-packages/pglite-server/README.md
🧰 Additional context used
📓 Path-based instructions (4)
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Name utility files in camelCase (e.g., mergeSchema.ts)
Files:
frontend/internal-packages/pglite-server/src/PGliteInstanceManager.tsfrontend/internal-packages/agent/src/tools/runTestTool.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript/TSX across the codebase
**/*.{ts,tsx}: Prefer early returns for readability
Use named exports only (no default exports)
Prefer const arrow functions over function declarations for simple utilities (e.g., const toggle = () => {})
Files:
frontend/internal-packages/pglite-server/src/PGliteInstanceManager.tsfrontend/internal-packages/agent/src/tools/runTestTool.ts
frontend/internal-packages/**
📄 CodeRabbit inference engine (AGENTS.md)
Infra and tooling (e2e, configs, storybook, agent) live under frontend/internal-packages
Files:
frontend/internal-packages/pglite-server/src/PGliteInstanceManager.tsfrontend/internal-packages/agent/src/tools/runTestTool.ts
frontend/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Follow existing import patterns and tsconfig path aliases
Files:
frontend/internal-packages/pglite-server/src/PGliteInstanceManager.tsfrontend/internal-packages/agent/src/tools/runTestTool.ts
🧬 Code graph analysis (2)
frontend/internal-packages/pglite-server/src/PGliteInstanceManager.ts (2)
frontend/internal-packages/pglite-server/src/extensionUtils.ts (2)
loadExtensions(147-168)filterExtensionDDL(173-208)frontend/internal-packages/pglite-server/src/types.ts (1)
SqlResult(1-11)
frontend/internal-packages/agent/src/tools/runTestTool.ts (3)
frontend/internal-packages/agent/src/qa-agent/types.ts (1)
Testcase(15-15)frontend/internal-packages/agent/src/qa-agent/validateSchema/types.ts (1)
TestcaseDmlExecutionResult(6-18)frontend/internal-packages/agent/src/utils/executeTestcase.ts (1)
executeTestcase(55-87)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Supabase Preview
- GitHub Check: frontend-ci
- GitHub Check: frontend-lint
- GitHub Check: agent-deep-modeling
- GitHub Check: Supabase Preview
🔇 Additional comments (5)
frontend/internal-packages/agent/src/tools/runTestTool.ts (2)
29-37: Memory logging is thorough and helpful for debugging.The detailed memory instrumentation with initial, periodic, and final logging provides valuable insights for debugging the LangSmith tracing issue. The delta calculations make it easy to track memory growth.
Also applies to: 86-98, 109-116
63-84: Per-testcase timing instrumentation looks good.The per-testcase start/completion logging and slow execution warnings (>10s) provide clear visibility into execution patterns, which aligns well with the debugging objectives.
frontend/internal-packages/pglite-server/src/PGliteInstanceManager.ts (3)
11-19: Pool infrastructure with round-robin selection is well-designed.The pooling implementation with round-robin selection, extension validation, and lazy initialization provides a solid foundation for parallel execution. The
extensionsMatchcomparison and pool recreation logic correctly handle extension changes.Also applies to: 62-75, 80-164
28-57: Instance creation with reduced memory footprint is appropriate.Reducing
initialMemoryfrom 2GB to 256MB per instance is sensible for a pooled architecture with 24 instances (or 10 as per PR objective). The timing logs help track instance creation overhead.
184-202: Memory instrumentation around instance lifecycle is comprehensive.The detailed memory logging before/after getting an instance and after execution provides valuable debugging data for tracking memory growth and identifying leaks.
Also applies to: 236-246
| /** | ||
| * Execute DML operations by testcase with DDL statements | ||
| * Combines DDL and testcase-specific DML into single execution units | ||
| * Executes in parallel using configurable instance pool (default: 3, production: 8) |
There was a problem hiding this comment.
PR objective mismatch: Implementation uses default pool size of 24, not 10.
The PR title and description state the goal is to limit test case generation to 10 for debugging LangSmith tracing issues (route06/liam-internal#5747), but the implementation defaults to 24 (PGLITE_POOL_SIZE with fallback '24'). This inconsistency makes it unclear whether the debugging constraint is being applied.
Update the default pool size to match the PR objective and add DEBUG/TODO markers:
- const poolSize = Number(process.env['PGLITE_POOL_SIZE'] || '24')
+ // DEBUG: Temporary limit for LangSmith tracing debugging (route06/liam-internal#5747)
+ // TODO: Remove this limit after tracing issue is resolved
+ const poolSize = Number(process.env['PGLITE_POOL_SIZE'] || '10')
console.info(
- `[runTestTool] Starting parallel execution with ${poolSize}-instance pool`,
+ `[runTestTool] Starting parallel execution with ${poolSize}-instance pool (debugging LangSmith tracing)`,
)Also update the comment at line 22:
- * Executes in parallel using configurable instance pool (default: 3, production: 8)
+ * Executes in parallel using configurable instance pool (temporary default: 10 for debugging)Also applies to: 39-42
🤖 Prompt for AI Agents
In frontend/internal-packages/agent/src/tools/runTestTool.ts around lines 22 and
39-42, the file comment and the PGLITE_POOL_SIZE fallback are inconsistent with
the PR objective of limiting test case generation to 10; update the header
comment at line 22 to state the default instance pool is 10 (debugging mode),
change the PGLITE_POOL_SIZE fallback from '24' to '10', and add clear DEBUG/TODO
markers near that configuration (lines ~39-42) noting this is a temporary debug
setting for LangSmith tracing investigation and should be reverted once
resolved.
| const poolSize = Number(process.env['PGLITE_POOL_SIZE'] || '24') | ||
| console.info( | ||
| `[runTestTool] Starting parallel execution with ${poolSize}-instance pool`, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
No explicit concurrency limit for debugging purposes.
The current implementation relies on the implicit pool size to limit concurrency. However, the PR objective is to add an explicit temporary debugging limit. Without an explicit concurrency control mechanism like pLimit, the actual parallelism depends entirely on how PGliteInstanceManager manages its pool, making the debugging intent unclear.
Add an explicit concurrency limit using p-limit:
import pLimit from 'p-limit'
// Inside executeDmlOperationsByTestcase function:
const poolSize = Number(process.env['PGLITE_POOL_SIZE'] || '10')
// DEBUG: Temporary limit for LangSmith tracing debugging (route06/liam-internal#5747)
// TODO: Remove this explicit limit after tracing issue is resolved
const limit = pLimit(poolSize)
console.info(
`[runTestTool] Starting parallel execution with explicit concurrency limit of ${poolSize}`,
)
const startTime = Date.now()
// Execute testcases with explicit concurrency limit
const results = await Promise.all(
testcases.map((testcase, i) => {
return limit(async () => {
if (!testcase) {
// ... existing dummy result logic ...
}
// ... existing execution logic ...
})
}),
)🤖 Prompt for AI Agents
In frontend/internal-packages/agent/src/tools/runTestTool.ts around lines 39–42,
add an explicit temporary concurrency limiter for debugging: import p-limit, set
poolSize to Number(process.env['PGLITE_POOL_SIZE'] || '10'), create a limit =
pLimit(poolSize) and wrap each testcase execution call with limit(...) when
building the Promise.all so concurrency is enforced regardless of
PGliteInstanceManager internals; include a console.info message that states the
explicit concurrency limit and add a short DEBUG/TODO comment indicating this is
temporary for LangSmith tracing debugging and should be removed once tracing is
fixed.
- Add test case ID to completion log - Include pass/fail status with visual indicators (✓/✗) - Show progress (e.g., 1/10) for better tracking - Match the format of the starting log for consistency 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
frontend/internal-packages/agent/src/tools/runTestTool.ts (4)
39-42: No explicit concurrency limit—relies on implicit pool behavior.The PR aims to add a temporary debugging limit on concurrency, but the current implementation uses unbounded
Promise.all(line 46) and relies entirely onPGliteInstanceManager's internal pool to naturally limit concurrency. This makes the debugging intent implicit rather than explicit.For clearer debugging semantics, add an explicit concurrency limit using
p-limit:import pLimit from 'p-limit' // Inside executeDmlOperationsByTestcase: const poolSize = Number(process.env['PGLITE_POOL_SIZE'] || '10') // DEBUG: Explicit concurrency limit for LangSmith tracing debugging // TODO: Remove after tracing issue is resolved const limit = pLimit(poolSize) console.info( `[runTestTool] Starting parallel execution with explicit concurrency limit of ${poolSize}`, ) const results = await Promise.all( testcases.map((testcase, i) => limit(async () => { // ... existing testcase execution logic ... }) ), )This ensures concurrency is explicitly bounded at the application level, independent of pool implementation details.
48-61: Verify necessity of undefined testcase handling.The code defensively handles
undefinedtestcases with dummy results, then filters them out at line 119. This suggests thetestcasesarray might containundefinedentries, which would indicate a data integrity issue upstream.Verify whether
testcasescan actually containundefined:#!/bin/bash # Search for how testcases array is constructed and whether it can contain undefined rg -nP --type=ts -C5 'testcases\s*[:=].*\[' frontend/internal-packages/agent/src/ # Check the Testcase type definition rg -nP --type=ts -C3 'type\s+Testcase\s*=' frontend/internal-packages/agent/src/If
undefinedentries should never occur, replace the defensive handling with an assertion:if (!testcase) { throw new Error(`Testcase at index ${i} is undefined - data integrity issue`) }
22-22: Comment claims "default: 3, production: 8" but code uses 24.The comment states the pool defaults to 3 with production at 8, but line 39 uses
'24'as the fallback. Additionally, none of these values match the PR objective of limiting to 10 for debugging.Update the comment to reflect the actual implementation:
- * Executes in parallel using configurable instance pool (default: 3, production: 8) + * Executes in parallel using configurable instance pool (default: 24, override via PGLITE_POOL_SIZE)Note: This discrepancy was flagged in previous reviews. Consider whether the default should be 10 to match the PR objective.
39-42: Missing DEBUG/TODO markers for temporary debugging change.The PR description explicitly states this is a "temporary debugging change to help determine whether 1–2 parallel processes failing to complete in LangSmith traces are related to concurrency" and should be reverted after the issue is resolved. However, the code lacks any markers indicating this is temporary.
Add clear markers for the temporary nature of this change:
const poolSize = Number(process.env['PGLITE_POOL_SIZE'] || '24') + // DEBUG: Temporary pool size for LangSmith tracing debugging (route06/liam-internal#5747) + // TODO: Remove explicit limit and revert to production defaults after tracing issue is resolved console.info( - `[runTestTool] Starting parallel execution with ${poolSize}-instance pool`, + `[runTestTool] Starting parallel execution with ${poolSize}-instance pool (debugging mode)`, )Also consider: The default value of 24 doesn't match the PR objective of 10. If the intent is to debug with 10 concurrent executions, change
'24'to'10'.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/frontend-ci.yml(1 hunks)frontend/internal-packages/agent/src/tools/runTestTool.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Name utility files in camelCase (e.g., mergeSchema.ts)
Files:
frontend/internal-packages/agent/src/tools/runTestTool.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript/TSX across the codebase
**/*.{ts,tsx}: Prefer early returns for readability
Use named exports only (no default exports)
Prefer const arrow functions over function declarations for simple utilities (e.g., const toggle = () => {})
Files:
frontend/internal-packages/agent/src/tools/runTestTool.ts
frontend/internal-packages/**
📄 CodeRabbit inference engine (AGENTS.md)
Infra and tooling (e2e, configs, storybook, agent) live under frontend/internal-packages
Files:
frontend/internal-packages/agent/src/tools/runTestTool.ts
frontend/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Follow existing import patterns and tsconfig path aliases
Files:
frontend/internal-packages/agent/src/tools/runTestTool.ts
🧬 Code graph analysis (1)
frontend/internal-packages/agent/src/tools/runTestTool.ts (3)
frontend/internal-packages/agent/src/qa-agent/types.ts (1)
Testcase(15-15)frontend/internal-packages/agent/src/qa-agent/validateSchema/types.ts (1)
TestcaseDmlExecutionResult(6-18)frontend/internal-packages/agent/src/utils/executeTestcase.ts (1)
executeTestcase(55-87)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Supabase Preview
- GitHub Check: frontend-lint
- GitHub Check: frontend-ci
- GitHub Check: agent-deep-modeling
- GitHub Check: Supabase Preview
🔇 Additional comments (1)
frontend/internal-packages/agent/src/tools/runTestTool.ts (1)
29-119: Extensive instrumentation lacks cleanup plan despite temporary debugging objective.The PR is described as a temporary debugging change for LangSmith tracing (route06/liam-internal#5747), yet the implementation adds extensive permanent-looking instrumentation: initial/final memory logging, per-testcase timing, periodic memory snapshots, and slow-execution warnings. While valuable for debugging, there's no indication which parts are temporary vs. permanent additions.
Clarify the scope of temporary changes:
If all this instrumentation is temporary for debugging, add a single comprehensive comment block at the function start:
// DEBUG: Temporary instrumentation for LangSmith tracing debugging (route06/liam-internal#5747) // TODO: After issue is resolved, remove or reduce logging: // - Memory usage logging (lines 29-37, 86-98, 109-116) // - Per-testcase timing details (lines 63-84) // - Pool size debugging messages (lines 40-42)If some instrumentation should remain (e.g., basic timing, error logging), mark only the temporary sections with
// DEBUG:comments.This will ensure the debugging code is properly cleaned up after the LangSmith issue is resolved.
| env: | ||
| PGLITE_POOL_SIZE: 3 |
There was a problem hiding this comment.
CI pool size (3) diverges from PR objective (10) without explanation.
The PR title and description state the goal is to limit to 10 for debugging LangSmith tracing issues, but the CI environment sets PGLITE_POOL_SIZE: 3. While a lower limit in CI for resource management is reasonable, the mismatch with the stated debugging objective should be documented.
Add a comment explaining the CI-specific value:
- run: pnpm test
env:
+ # CI uses smaller pool size (3) for resource constraints
+ # Production debugging uses 10 (see route06/liam-internal#5747)
PGLITE_POOL_SIZE: 3📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| env: | |
| PGLITE_POOL_SIZE: 3 | |
| - run: pnpm test | |
| env: | |
| # CI uses smaller pool size (3) for resource constraints | |
| # Production debugging uses 10 (see route06/liam-internal#5747) | |
| PGLITE_POOL_SIZE: 3 |
🤖 Prompt for AI Agents
.github/workflows/frontend-ci.yml around lines 74 to 75: CI sets
PGLITE_POOL_SIZE: 3 which conflicts with the PR objective of limiting to 10 for
debugging; add a concise inline comment above this env line explaining that the
CI value is intentionally set to 3 due to shared runner/resource constraints
(and that developers should use 10 locally or in dedicated debug runs), and
optionally note where to change it if someone needs to run CI with the
troubleshooting value; keep the comment short and factual.
- Skip PGlite initialization in CI when pool size > 3 - Increase beforeAll timeout from 30s to 60s - Add logging when skipping initialization - Prevents memory exhaustion in CI environment 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Limit qa-agent test case generation to first 10 requirements - Same as commit fb1290c which was previously reverted - Helps investigate parallel processing and tracing issues - TODO: Remove this limitation after debugging is complete Issue: route06/liam-internal#5747 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Implement savepoint pattern for efficient DDL/DML execution - Execute DDL once per instance and cache with hash tracking - Use ROLLBACK TO SAVEPOINT for fast state reset between tests - Fix DDL/DML splitting logic to use marker-based parsing - Handle regular SQL queries without transaction overhead - Track DDL state and transaction status per instance This optimization reduces test execution overhead by avoiding redundant DDL execution and using efficient savepoint rollback instead of full transaction rollback. 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
This reverts commit d164e5f.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
frontend/internal-packages/pglite-server/src/PGliteInstanceManager.ts (3)
16-19: PR objective mismatch: POOL_SIZE should be 10, not 24.The PR title and description state the goal is to limit test case generation to 10 for debugging LangSmith tracing issues. However,
POOL_SIZEdefaults to 24, and there are no DEBUG or TODO comments marking this as a temporary debugging change that should be reverted.Apply this diff to match the PR objective and add appropriate markers:
- // Pool size configurable via environment variable (default: 24 for maximum parallelism) - private static readonly POOL_SIZE = Number( - process.env['PGLITE_POOL_SIZE'] || '24', - ) + // DEBUG: Temporary limit for LangSmith tracing debugging (route06/liam-internal#5747) + // TODO: Revert to 24 after tracing issue is resolved + private static readonly POOL_SIZE = Number( + process.env['PGLITE_POOL_SIZE'] || '10', + )
84-102: Add error handling to prevent partially initialized pool.If
createInstancethrows during initialization, the pool arrays will be left in a partially populated state with mismatched lengths. Subsequent calls togetOrCreateInstancecould returnundefinedor throw.Wrap the loop in try/catch and clean up on failure:
private async initializePool(requiredExtensions: string[]): Promise<void> { console.info( `[PGlite] Initializing pool with ${PGliteInstanceManager.POOL_SIZE} instances`, ) + try { for (let i = 0; i < PGliteInstanceManager.POOL_SIZE; i++) { console.info( `[PGlite] Creating instance ${i + 1}/${PGliteInstanceManager.POOL_SIZE}`, ) const { db, supportedExtensions } = await this.createInstance(requiredExtensions) PGliteInstanceManager.instancePool.push(db) PGliteInstanceManager.supportedExtensionsPool.push(supportedExtensions) PGliteInstanceManager.loadedExtensionsPool.push(requiredExtensions) } + } catch (error) { + // Clean up any instances created before the error + for (const instance of PGliteInstanceManager.instancePool) { + await instance.close().catch(() => {}) // Ignore close errors + } + PGliteInstanceManager.instancePool = [] + PGliteInstanceManager.supportedExtensionsPool = [] + PGliteInstanceManager.loadedExtensionsPool = [] + throw error + } console.info('[PGlite] Pool initialization complete') }
107-126: Handle errors during instance cleanup in pool recreation.If
instance.close()orinitializePoolfails during pool recreation, the pool could be left in an inconsistent state (empty or partially populated). This would break all subsequent queries.Wrap close operations and ensure reinitialization errors are propagated:
private async recreatePool(requiredExtensions: string[]): Promise<void> { console.info('[PGlite] Extensions changed, recreating pool') // Close all existing instances for (const instance of PGliteInstanceManager.instancePool) { - await instance.close() + await instance.close().catch((error) => { + console.error('[PGlite] Error closing instance during pool recreation:', error) + }) } // Clear the pools PGliteInstanceManager.instancePool = [] PGliteInstanceManager.supportedExtensionsPool = [] PGliteInstanceManager.loadedExtensionsPool = [] PGliteInstanceManager.currentIndex = 0 // Clear state tracking maps PGliteInstanceManager.instanceDDLHash.clear() PGliteInstanceManager.instanceHasTransaction.clear() // Reinitialize with new extensions await this.initializePool(requiredExtensions) }
🧹 Nitpick comments (1)
frontend/internal-packages/pglite-server/src/PGliteInstanceManager.ts (1)
225-251: Consider reducing memory logging verbosity for production.The detailed memory logging before/after operations is useful for debugging but may be too verbose for production. Consider wrapping these logs in a debug flag check or reducing log level to debug/trace.
Example:
- console.info('[PGlite] Before getting instance:', { + if (process.env['PGLITE_DEBUG']) { + console.debug('[PGlite] Before getting instance:', { - rss: `${Math.round(memoryBefore.rss / 1024 / 1024)} MB`, - heapUsed: `${Math.round(memoryBefore.heapUsed / 1024 / 1024)} MB`, - external: `${Math.round(memoryBefore.external / 1024 / 1024)} MB`, - }) + rss: `${Math.round(memoryBefore.rss / 1024 / 1024)} MB`, + heapUsed: `${Math.round(memoryBefore.heapUsed / 1024 / 1024)} MB`, + external: `${Math.round(memoryBefore.external / 1024 / 1024)} MB`, + }) + }Apply similar changes to lines 243-250 and 358-365.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/internal-packages/pglite-server/package.json(1 hunks)frontend/internal-packages/pglite-server/src/PGliteInstanceManager.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
frontend/internal-packages/**
📄 CodeRabbit inference engine (AGENTS.md)
Infra and tooling (e2e, configs, storybook, agent) live under frontend/internal-packages
Files:
frontend/internal-packages/pglite-server/package.jsonfrontend/internal-packages/pglite-server/src/PGliteInstanceManager.ts
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Name utility files in camelCase (e.g., mergeSchema.ts)
Files:
frontend/internal-packages/pglite-server/src/PGliteInstanceManager.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript/TSX across the codebase
**/*.{ts,tsx}: Prefer early returns for readability
Use named exports only (no default exports)
Prefer const arrow functions over function declarations for simple utilities (e.g., const toggle = () => {})
Files:
frontend/internal-packages/pglite-server/src/PGliteInstanceManager.ts
frontend/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Follow existing import patterns and tsconfig path aliases
Files:
frontend/internal-packages/pglite-server/src/PGliteInstanceManager.ts
🧬 Code graph analysis (1)
frontend/internal-packages/pglite-server/src/PGliteInstanceManager.ts (2)
frontend/internal-packages/pglite-server/src/extensionUtils.ts (2)
loadExtensions(147-168)filterExtensionDDL(173-208)frontend/internal-packages/pglite-server/src/types.ts (1)
SqlResult(1-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Supabase Preview
- GitHub Check: frontend-ci
- GitHub Check: frontend-lint
- GitHub Check: codeql / languages (javascript) / Perform CodeQL for javascript
- GitHub Check: agent-deep-modeling
- GitHub Check: Supabase Preview
🔇 Additional comments (4)
frontend/internal-packages/pglite-server/src/PGliteInstanceManager.ts (4)
176-199: LGTM: DDL/DML splitting is appropriate for test SQL.The comment-marker-based approach is straightforward and suitable for the test SQL context where the structure is controlled and known. The method correctly handles both DDL-only and DDL+DML scenarios.
201-213: LGTM: Simple hash is sufficient for DDL change detection.The hash implementation is appropriate for detecting whether DDL has changed. Since it's only used for equality comparison (not cryptographic purposes), the simple bitwise operations are adequate.
261-333: LGTM: Savepoint-based transaction management is well-designed.The implementation correctly:
- Detects DDL changes using hashing and executes DDL only when necessary
- Starts transactions with savepoints after DDL execution
- Rolls back to savepoints after DML execution to maintain clean state
- Properly tracks per-instance transaction state
This design efficiently reuses instances while maintaining transactional isolation for test execution.
339-356: LGTM: Error handling with fallback rollback is robust.The error handling correctly attempts savepoint rollback first, and falls back to full transaction rollback if the savepoint operation fails. This ensures the instance remains in a consistent state even when errors occur.
| "lint:eslint": "eslint .", | ||
| "lint:tsc": "tsc --noEmit", | ||
| "test": "vitest" | ||
| "test": "vitest --watch=false" |
There was a problem hiding this comment.
Inconsistent with PR objective: this change doesn't limit test case generation to 10.
The PR description states the goal is to limit test case generation to 10 for debugging LangSmith tracing issues. However, adding --watch=false to the test script only ensures tests run once and exit (non-interactive mode), which is standard for CI but doesn't control parallelism or test case limits.
If the goal is to limit test parallelism, consider adding Vitest's --pool or --maxConcurrency flags:
- "test": "vitest --watch=false"
+ "test": "vitest --watch=false --maxConcurrency=10"Otherwise, clarify the PR objective if the intent is simply to ensure non-interactive test execution.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "test": "vitest --watch=false" | |
| // frontend/internal-packages/pglite-server/package.json | |
| { | |
| "scripts": { | |
| // … | |
| "test": "vitest --watch=false --maxConcurrency=10" | |
| // … | |
| } | |
| } |
🤖 Prompt for AI Agents
In frontend/internal-packages/pglite-server/package.json around line 29, the
updated test script only adds --watch=false which does not limit test case
generation to 10 as the PR claims; to actually limit parallelism add Vitest's
concurrency flag (e.g., append --maxConcurrency=10 or use the --pool option) to
the script so tests run non-interactively with at most 10 concurrent workers, or
if the PR's intent was merely to ensure non-interactive runs, update the PR
description to reflect that instead.
- Only attempt savepoint rollback for test SQL with active transactions - Regular SQL (without test markers) now executes without savepoint management - Prevents "ROLLBACK TO SAVEPOINT can only be used in transaction blocks" error - Fixes issue when SQL contains explicit BEGIN/COMMIT statements The error occurred when regular SQL with explicit transaction control failed and the catch block tried to rollback to a non-existent savepoint. 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
frontend/internal-packages/pglite-server/src/PGliteInstanceManager.ts (3)
107-126: Handle errors during instance cleanup in pool recreation.If
instance.close()fails during pool recreation, the pool could be left in an inconsistent state. Errors during cleanup should be logged but not prevent the recreation from completing.Wrap close operations to handle errors gracefully:
private async recreatePool(requiredExtensions: string[]): Promise<void> { console.info('[PGlite] Extensions changed, recreating pool') // Close all existing instances for (const instance of PGliteInstanceManager.instancePool) { - await instance.close() + await instance.close().catch((error) => { + console.error('[PGlite] Error closing instance during pool recreation:', error) + }) } // Clear the pools PGliteInstanceManager.instancePool = [] PGliteInstanceManager.supportedExtensionsPool = [] PGliteInstanceManager.loadedExtensionsPool = [] PGliteInstanceManager.currentIndex = 0 // Clear state tracking maps PGliteInstanceManager.instanceDDLHash.clear() PGliteInstanceManager.instanceHasTransaction.clear() // Reinitialize with new extensions await this.initializePool(requiredExtensions) }
16-19: PR objective mismatch: POOL_SIZE default is 24, not 10.The PR objectives state the goal is to limit test case generation to 10 for debugging LangSmith tracing, but
POOL_SIZEdefaults to 24. Additionally, there are no DEBUG or TODO comments marking this as a temporary debugging change that should be reverted.Update to match the PR objective and add appropriate markers:
- // Pool size configurable via environment variable (default: 24 for maximum parallelism) - private static readonly POOL_SIZE = Number( - process.env['PGLITE_POOL_SIZE'] || '24', - ) + // DEBUG: Temporary limit for LangSmith tracing debugging (route06/liam-internal#5747) + // TODO: Revert to original value after tracing issue is resolved + private static readonly POOL_SIZE = Number( + process.env['PGLITE_POOL_SIZE'] || '10', + )
84-102: Add error handling to prevent partially initialized pool.If
createInstancethrows during initialization, the pool arrays will be left in a partially populated state with mismatched lengths. Subsequent calls togetOrCreateInstancecould returnundefinedor throw.Wrap the loop in try/catch and clean up on failure:
private async initializePool(requiredExtensions: string[]): Promise<void> { console.info( `[PGlite] Initializing pool with ${PGliteInstanceManager.POOL_SIZE} instances`, ) + try { for (let i = 0; i < PGliteInstanceManager.POOL_SIZE; i++) { console.info( `[PGlite] Creating instance ${i + 1}/${PGliteInstanceManager.POOL_SIZE}`, ) const { db, supportedExtensions } = await this.createInstance(requiredExtensions) PGliteInstanceManager.instancePool.push(db) PGliteInstanceManager.supportedExtensionsPool.push(supportedExtensions) PGliteInstanceManager.loadedExtensionsPool.push(requiredExtensions) } + } catch (error) { + // Clean up any instances created before the error + for (const instance of PGliteInstanceManager.instancePool) { + await instance.close().catch(() => {}) // Ignore close errors + } + PGliteInstanceManager.instancePool = [] + PGliteInstanceManager.supportedExtensionsPool = [] + PGliteInstanceManager.loadedExtensionsPool = [] + throw error + } console.info('[PGlite] Pool initialization complete') }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/internal-packages/pglite-server/src/PGliteInstanceManager.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Name utility files in camelCase (e.g., mergeSchema.ts)
Files:
frontend/internal-packages/pglite-server/src/PGliteInstanceManager.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript/TSX across the codebase
**/*.{ts,tsx}: Prefer early returns for readability
Use named exports only (no default exports)
Prefer const arrow functions over function declarations for simple utilities (e.g., const toggle = () => {})
Files:
frontend/internal-packages/pglite-server/src/PGliteInstanceManager.ts
frontend/internal-packages/**
📄 CodeRabbit inference engine (AGENTS.md)
Infra and tooling (e2e, configs, storybook, agent) live under frontend/internal-packages
Files:
frontend/internal-packages/pglite-server/src/PGliteInstanceManager.ts
frontend/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Follow existing import patterns and tsconfig path aliases
Files:
frontend/internal-packages/pglite-server/src/PGliteInstanceManager.ts
🧬 Code graph analysis (1)
frontend/internal-packages/pglite-server/src/PGliteInstanceManager.ts (2)
frontend/internal-packages/pglite-server/src/extensionUtils.ts (2)
loadExtensions(147-168)filterExtensionDDL(173-208)frontend/internal-packages/pglite-server/src/types.ts (1)
SqlResult(1-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Supabase Preview
- GitHub Check: frontend-lint
- GitHub Check: frontend-ci
- GitHub Check: agent-deep-modeling
- GitHub Check: Supabase Preview
🔇 Additional comments (2)
frontend/internal-packages/pglite-server/src/PGliteInstanceManager.ts (2)
35-38: LGTM: Memory optimization aligns with pooling strategy.The reduction of
initialMemoryfrom 2GB to 256MB per instance is appropriate for a pooled architecture. With the default pool size of 24, this reduces total memory allocation from 48GB to 6GB, which is more sustainable for concurrent operations.Also applies to: 51-54
226-250: LGTM: Comprehensive memory instrumentation.The memory logging provides valuable diagnostics for tracking memory usage at key points (before/after instance retrieval, after execution). The delta calculations help identify memory growth patterns, which is useful for debugging the LangSmith tracing issue mentioned in the PR.
Also applies to: 362-368
| // Get instance index for state tracking | ||
| const instanceIndex = | ||
| (PGliteInstanceManager.currentIndex - | ||
| 1 + | ||
| PGliteInstanceManager.POOL_SIZE) % | ||
| PGliteInstanceManager.POOL_SIZE |
There was a problem hiding this comment.
Critical: Instance index calculation is off by one.
The instanceIndex calculation is performed after getOrCreateInstance() has already incremented currentIndex (lines 150-151). This means instanceIndex points to the previous instance in the pool, not the instance that was just returned. All subsequent state tracking (DDL hash, transaction state) will be associated with the wrong instance.
Move the instance index retrieval before the call to getOrCreateInstance(), or better yet, have getOrCreateInstance() return the index along with the instance:
Option 1: Modify getOrCreateInstance to return index
private async getOrCreateInstance(
requiredExtensions: string[],
-): Promise<{ db: PGlite; supportedExtensions: string[] }> {
+): Promise<{ db: PGlite; supportedExtensions: string[]; index: number }> {
// Initialize pool if empty
if (PGliteInstanceManager.instancePool.length === 0) {
await this.initializePool(requiredExtensions)
}
// Check if extensions have changed (compare with first instance's extensions)
const firstLoadedExtensions = PGliteInstanceManager.loadedExtensionsPool[0]
if (
firstLoadedExtensions &&
!this.extensionsMatch(requiredExtensions, firstLoadedExtensions)
) {
await this.recreatePool(requiredExtensions)
}
// Get the next instance in round-robin fashion
const index = PGliteInstanceManager.currentIndex
PGliteInstanceManager.currentIndex =
(PGliteInstanceManager.currentIndex + 1) % PGliteInstanceManager.POOL_SIZE
console.info(
`[PGlite] Using instance ${index + 1}/${PGliteInstanceManager.POOL_SIZE} from pool`,
)
const instance = PGliteInstanceManager.instancePool[index]
const supportedExtensions =
PGliteInstanceManager.supportedExtensionsPool[index]
if (!instance || !supportedExtensions) {
// This should never happen if pool is properly initialized
// eslint-disable-next-line no-throw-error/no-throw-error
throw new Error(`Instance ${index} not found in pool`)
}
return {
db: instance,
supportedExtensions,
+ index,
}
}Then update the caller:
- const { db, supportedExtensions } =
+ const { db, supportedExtensions, index: instanceIndex } =
await this.getOrCreateInstance(requiredExtensions)
- // Get instance index for state tracking
- const instanceIndex =
- (PGliteInstanceManager.currentIndex -
- 1 +
- PGliteInstanceManager.POOL_SIZE) %
- PGliteInstanceManager.POOL_SIZE
Issue
Why is this change needed?
We're experiencing an issue where 1-2 parallel processes in qa-agent's generateTestcase don't complete properly in LangSmith traces. This temporary change limits the number of test cases generated in parallel to 10 to help debug whether the issue is related to the number of concurrent operations.
Changes
Note
This is a temporary debugging change that should be reverted once the LangSmith tracing issue is resolved.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores