feat: CI pipeline for tests regarding HQL security and clickhouse wrapper

[devinat1]

• Added .github/workflows/hql-clickhouse-tests.yml to run ClickHouse Wrapper tests automatically on changes to relevant implementation and test files. This workflow sets up a ClickHouse service, installs dependencies with retry logic, and runs targeted tests.
• Added .github/workflows/hql-security-tests.yml to run HQL Security tests for files related to HQL security logic and tests.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Comments	Updated (UTC)
helicone	Ready	Preview	Comment	Sep 8, 2025 9:41pm
helicone-bifrost	Ready	Preview	Comment	Sep 8, 2025 9:41pm
helicone-eu	Ready	Preview	Comment	Sep 8, 2025 9:41pm

[claude]

Claude finished @devinat1's task —— View job

---

Code Review: CI Pipeline for HQL Security and ClickHouse Wrapper Tests

Confidence Score: 7/10 - Merge this PR with minor cautions

Detailed Analysis

✅ Strengths

1. Good Separation of Concerns

• Disaggregating monolithic test workflow into focused pipelines improves maintainability
• Each workflow targets specific functionality (security vs. database operations)
• Path-based triggering optimizes CI resource usage

2. Robust Infrastructure Setup

• ClickHouse service properly configured with health checks and timeout settings
• Comprehensive retry logic for dependency installation with fallback to npm
• Appropriate environment variables and test isolation

3. Code Quality Improvements

• TestClickhouseWrapper.ts modernized to use 'url' parameter instead of deprecated 'host'
• Proper URL construction handles both HTTP and non-HTTP host formats
• New hqlQueryWithContext method maintains API consistency

⚠️ Issues and Concerns

1. Critical Test Command Issues (Confirmed by Greptile review)

run: yarn test:jawn TestClickhouseWrapper.test.ts  # May not work
run: yarn test:jawn hqlSecurityTests.test.ts       # May not work

The test:jawn script runs npx jest --detectOpenHandles which may not properly handle filename arguments. Consider using pattern matching:

run: yarn test:jawn --testNamePattern='TestClickhouseWrapper'
run: yarn test:jawn --testNamePattern='hqlSecurityTests'

2. Missing Error Handling in ClickHouse Workflow

# Current implementation continues even if ClickHouse fails
for i in {1..30}; do
  # ... wait logic ...
  if [ $i -eq 30 ]; then
    echo "ClickHouse failed to start after 30 attempts"
    exit 1  # ✅ Good - this was actually fixed
  fi
done

3. YAML Syntax Error
In hql-security-tests.yml:

jobs:
  hql-security-tests:  # Duplicate key
  hql-security-tests:  # This will cause issues

4. Hardcoded Values

• Default ClickHouse port (18123 vs 8123) in TestClickhouseWrapper.ts singleton
• Environment-specific configurations could be more flexible

🔒 Security Assessment

Low Risk - The workflows themselves are secure:

• No secrets exposed in workflow files
• Proper environment variable usage
• Test isolation prevents cross-contamination
• ClickHouse container runs with standard security settings

🚀 Performance Considerations

Acceptable Performance:

• Workflows trigger only on relevant file changes (good path filtering)
• Parallel execution where possible
• Reasonable timeout values (30s for ClickHouse startup)
• Proper caching strategy for Node.js dependencies

Minor Concerns:

• 30-iteration wait loop could be optimized with exponential backoff
• Retry logic for yarn install could timeout after extended periods

🔧 Recommendations

High Priority Fixes:

1. Fix duplicate job names in hql-security-tests.yml
2. Verify Jest test command patterns work with current configuration
3. Test workflows in a development environment before merging

Medium Priority Improvements:

1. Consider using --testPathPattern instead of bare filenames
2. Add workflow status badges to track CI health
3. Consider adding test result artifacts for better debugging

Low Priority Enhancements:

1. Implement exponential backoff for ClickHouse health checks
2. Add more granular path filtering
3. Consider matrix builds for different Node.js versions

📋 Summary

This PR successfully disaggregates testing workflows and modernizes ClickHouse client usage. The main concerns are:

🔴 Critical: YAML syntax errors and potentially incorrect test commands
🟡 Medium: Missing comprehensive error handling
🟢 Minor: Hardcoded configurations and optimization opportunities

The changes improve CI organization and follow good practices, but the workflow files need syntax fixes and test command verification before merging.

🎯 Action Items

• Fix duplicate job names in YAML
• Verify Jest test commands work as expected
• Test workflows in development environment
• Consider the Jest pattern matching suggestions

---

[greptile-apps]

Greptile Summary

This PR introduces CI automation for HQL security and ClickHouse wrapper testing by adding two new GitHub Actions workflows. The changes enhance the testing infrastructure to automatically validate critical database and security components when relevant files are modified.

The first workflow (.github/workflows/hql-clickhouse-tests.yml) sets up a complete ClickHouse service environment and runs wrapper tests with sophisticated retry logic for dependency installation. It includes proper health checks and environment configuration to ensure realistic testing conditions. The second workflow (.github/workflows/hql-security-tests.yml) focuses on HQL security validation, running security-critical tests in isolation when HQL-related files are modified.

Additionally, the PR includes important maintenance updates: the TestClickhouseWrapper.ts has been modernized to use the newer 'url' parameter instead of the deprecated 'host' parameter for ClickHouse client creation, and adds a new hqlQueryWithContext method for API compatibility. The HeliconeSqlManager.ts receives a type safety improvement by explicitly converting error objects to strings before processing, preventing potential runtime errors in error handling code.

These workflows follow established CI patterns in the codebase and include path-based triggering to optimize resource usage by only running when relevant files change. The changes align with the broader initiative (ENG-3012) to improve code quality through automated testing of critical infrastructure components.

Confidence score: 3/5

• This PR introduces useful CI automation but has several technical issues that could cause workflow failures
• Score reflects the value of automated testing offset by implementation concerns around test execution and environment consistency
• Pay close attention to the workflow YAML files and the ClickHouse wrapper configuration discrepancy

Context used:

Context - When naming jobs in GitHub Actions, prefer descriptive names that clearly indicate the job's purpose, such as 'Build Docker image' instead of generic terms like 'Precheck'. (link)
Context - Use 'bash' at the start of code blocks that contain shell commands for clarity. (link)

_{4 files reviewed, no comments}

_{Edit Code Review Bot Settings | Greptile}

[greptile-apps]

logic: The test command yarn test:jawn hqlSecurityTests.test.ts may not work as expected. Jest typically requires the full path or pattern. Consider using yarn test:jawn --testNamePattern='hqlSecurityTests' or the full file path.

[greptile-apps]

logic: The test command may not work as expected. Based on the package.json, test:jawn uses npx jest --detectOpenHandles, but passing a filename directly might not match the Jest configuration. Consider using pattern matching or verifying the Jest setup.