Skip to content

fix: correct reexport query direction and add exports integration tests#276

Merged
carlos-alm merged 1 commit intomainfrom
fix/274-greptile-feedback
Mar 3, 2026
Merged

fix: correct reexport query direction and add exports integration tests#276
carlos-alm merged 1 commit intomainfrom
fix/274-greptile-feedback

Conversation

@carlos-alm
Copy link
Contributor

@carlos-alm carlos-alm commented Mar 3, 2026

Summary

  • Fix reexport detection query in exportsData — the source_id/target_id join was swapped, causing barrel files to be missed. Add DISTINCT to prevent duplicate entries.
  • Add comprehensive integration test suite for the exports command (tests/integration/exports.test.js) — 6 tests covering exported symbols with consumers, counts, reexport detection, noTests filtering, empty results, and pagination.

Context

This PR was originally bundled with AST, CFG, and expanded node type features (16 files, 2000+ lines). Those features have been extracted into focused PRs and merged separately:

Feature PR Status
normalizeSymbol #267 Merged
Exports command #269 Merged
Expanded node types #270 Merged
CFG construction #274 Merged

This PR now contains only the 2 changes that were missing from the split PRs:

  1. The reexport query bug fix (6 lines in src/queries.js)
  2. The exports integration test file (157 lines)

Test plan

  • New test suite passes: npx vitest run tests/integration/exports.test.js (6/6)
  • Full test suite passes: 1306 passed, 0 failed
  • git diff --stat origin/main confirms only 2 files changed

@claude
Copy link

claude bot commented Mar 3, 2026

Claude finished @carlos-alm's task —— View job


PR Review Complete ⭐⭐☆☆☆ (2/5 stars)

Todo List:

  • Read CLAUDE.md for repo context
  • Phase 1: Root Cause Analysis - understand the problem being solved
  • Phase 2: Configuration Change Detection - check for high-risk changes
  • Phase 3: Test Integrity - verify test quality and coverage
  • Phase 4: Code Quality - review implementation details
  • Final Assessment with rating and recommendation

Root Cause Analysis

Problem Being Solved: Add a command to show exported symbols with their consumers.

Why This Approach: ⚠️ MAJOR SCOPE CREEP - This PR contains 3 separate features:

  1. exports command (legitimate, ~50 lines)
  2. AST node storage system (ast.js +392 lines)
  3. Control Flow Graph implementation (cfg.js +1035 lines)

Risk Assessment: HIGH - Multiple unrelated major features bundled together.

Critical Concerns

⚠️ Massive Scope Creep

This PR title claims "exports command" but includes:

  • 3 new database tables (ast_nodes, cfg_blocks, cfg_edges)
  • Complete AST extraction system (calls, new, string, regex, throw, await)
  • Full CFG construction with basic blocks and edges
  • Over 2000 lines of unrelated infrastructure

The actual exports functionality only touches 6 files with minimal changes.

⚠️ PR Size Violation

16 files changed with massive additions violates the review guidelines for large PRs mixing unrelated concerns.

⚠️ Missing Problem Justification

  • No explanation why AST/CFG features are needed
  • AST and CFG systems are not used by the exports command
  • Appears to bundle multiple features to avoid separate review

Configuration Changes

No risky configuration changes detected - biome.json unchanged, no linting rules relaxed.

Test Integrity

Tests appear solid:

  • Comprehensive test suite for exports functionality
  • AST and CFG tests look thorough
  • No weakened assertions or skipped tests
  • Proper isolation using temp directories

Code Quality

Exports Command: ✅ Good quality

  • Clean SQL with proper parameterization
  • Follows existing patterns in queries.js
  • Good error handling and pagination
  • Consistent with other command implementations

AST/CFG Systems: ❓ Cannot properly review due to scope

  • Large systems require dedicated review
  • Complex tree-sitter integrations need careful analysis
  • Database schema changes need migration review

Final Recommendation

## Root Cause Analysis
**Problem Being Solved**: Add exports command for per-symbol consumer analysis
**Why This Approach**: SCOPE CREEP - bundles 3 unrelated major features
**Risk Assessment**: HIGH - Multiple database schema changes, 2000+ lines unrelated code

## Critical Concerns
1. **Massive scope violation** - AST/CFG systems unrelated to exports
2. **Review impossible** - Cannot properly assess 3 major features in one PR  
3. **Breaking change risk** - New database tables without clear migration strategy

## Final Recommendation
- Rating: ⭐⭐☆☆☆ (2/5)
- Action: **REQUEST CHANGES**
- Reasoning: Split into 3 separate PRs: (1) exports command, (2) AST system, (3) CFG system

Required Changes:

  1. Split this PR into 3 focused PRs for proper review
  2. Exports-only PR should include only: CLI command, query function, MCP tool, tests
  3. Separate PRs for AST and CFG with proper justification and use cases

The exports functionality itself is well-implemented, but this PR violates fundamental review principles by bundling unrelated major features.


@greptile-apps
Copy link

greptile-apps bot commented Mar 3, 2026

Greptile Summary

Fixes critical reexport detection bug in exportsData query and adds comprehensive test coverage.

  • Query fix in src/queries.js: The reexport query had source_id/target_id swapped in the JOIN and WHERE clauses, causing barrel files to be missed when querying which files re-export a target file. The corrected query now properly joins source_id with the nodes table to retrieve barrel files that target the queried file. Added DISTINCT to prevent duplicate entries.
  • New test suite: tests/integration/exports.test.js adds 6 integration tests validating exported symbols, consumer detection, reexport detection, noTests filtering, empty results, and pagination. Test fixture includes proper barrel → lib.js reexport edge to validate the fix.

Confidence Score: 5/5

  • This PR is safe to merge with no risk
  • Focused bug fix with correct logic reversal, proper test coverage validates the fix, no breaking changes or side effects
  • No files require special attention

Important Files Changed

Filename Overview
src/queries.js Corrects swapped source_id/target_id join in reexport query to properly find barrel files; adds DISTINCT to prevent duplicates
tests/integration/exports.test.js New comprehensive test suite validates exports command with 6 tests covering consumers, reexport detection, filtering, and pagination

Last reviewed commit: e223d47

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

33 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

Swap source_id/target_id join in reexport detection so barrel files are
correctly identified as re-exporters. Add DISTINCT to prevent duplicates.
Add comprehensive integration tests for the exports command covering
consumer detection, noTests filtering, pagination, and reexport edges.

Impact: 1 functions changed, 4 affected
@carlos-alm carlos-alm force-pushed the fix/274-greptile-feedback branch from d3cd2e6 to e223d47 Compare March 3, 2026 05:26
@claude
Copy link

claude bot commented Mar 3, 2026

Claude finished @carlos-alm's task —— View job


PR Review Complete ⭐⭐⭐⭐☆ (4/5 stars)

Todo List:

  • Read CLAUDE.md for repo context and development guidelines
  • Phase 1: Root Cause Analysis - understand the problem being solved
  • Phase 2: Configuration Change Detection - check for high-risk changes
  • Phase 3: Test Integrity - verify test quality and coverage
  • Phase 4: Code Quality - review implementation details
  • Final Assessment with rating and recommendation

Phase 1: Root Cause Analysis ✅

Problem Being Solved: Fix incorrect reexport query direction in the exports command and add comprehensive test coverage.

Why This Approach: This is a targeted bug fix. The reexport detection query was incorrectly joining source_id/target_id, causing barrel files to not be properly identified as re-exporters. The fix swaps the join direction and adds DISTINCT to prevent duplicates.

Risk Assessment: LOW - This is a focused bug fix to existing functionality with only 6 line changes and comprehensive test coverage added.

Phase 2: Configuration Change Detection ✅

No risky configuration changes detected - biome.json, package.json, tsconfig.json, and .codegraphrc.json are unchanged. No linting rules relaxed or test thresholds lowered.

Phase 3: Test Integrity ✅

Strong test quality:

  • 6 comprehensive test cases covering all major functionality
  • 28 assertions ensuring thorough validation
  • No test weakening detected - no .only, .skip, or eslint-disable workarounds
  • Proper isolation using temp directories and beforeAll/afterAll cleanup
  • Tests cover: exported symbols detection, consumer analysis, reexport detection, noTests filtering, pagination, and empty results

Phase 4: Code Quality ✅

Strong implementation quality:

Security ✅

  • SQL injection protected - All queries use prepared statements with parameterized inputs
  • Path traversal protected - safePath() function in src/queries.js:18-22 prevents directory traversal
  • Proper file handling - File reading uses try-catch with graceful fallbacks

SQL Query Analysis ✅

  • Correct join logic in src/queries.js:3191-3192: JOIN nodes n ON e.source_id = n.id WHERE e.target_id = ? properly identifies barrel files that re-export the target file
  • Proper filtering - Cross-file call detection correctly identifies exports vs internal symbols
  • DISTINCT prevents duplicates - Added to prevent duplicate reexport entries

Error Handling ✅

  • Graceful file read failures - File cache handles missing files with null fallback
  • Database connection management - Proper db.close() after queries
  • Pagination safety - Handles empty results appropriately

Minor Improvements (Non-blocking)

  • Line src/queries.js:3195 - .map((r) => ({ file: r.file })) could be simplified, but follows existing pattern consistency

Final Recommendation

## Root Cause Analysis
**Problem Being Solved**: Fix incorrect reexport query direction and add test coverage
**Why This Approach**: Targeted bug fix with proper SQL join correction and comprehensive testing
**Risk Assessment**: LOW - Focused fix with thorough validation

## Critical Concerns
None - this is a clean, well-tested bug fix.

## Final Recommendation
- Rating: ⭐⭐⭐⭐☆ (4/5)
- Action: **APPROVE**
- Reasoning: High-quality bug fix with excellent test coverage. SQL joins corrected properly, security considerations addressed, and comprehensive test suite added. Only loses one star due to the modest scope - this is exactly what a good bug fix PR should look like.

Strengths:

  • ✅ Focused, targeted fix addressing the root cause
  • ✅ Comprehensive test coverage (6 tests, 28 assertions)
  • ✅ Proper SQL parameterization preventing injection
  • ✅ Path traversal protection maintained
  • ✅ Clear commit message explaining the change

Minor Areas for Future Consideration:

  • The query mapping could be slightly simplified, but consistency with existing patterns is more important

This PR represents excellent engineering practices: identify the problem, fix it precisely, test it thoroughly.


@carlos-alm carlos-alm changed the title feat: add exports <file> command for per-symbol consumer analysis fix: correct reexport query direction and add exports integration tests Mar 3, 2026
@carlos-alm
Copy link
Contributor Author

All concerns from the review have been addressed by splitting this PR.

Scope creep resolved: The AST, CFG, expanded node types, and exports command features were extracted into separate focused PRs and merged independently:

This PR now contains only 2 changes (previously missing from the split):

  1. Reexport query bug fix (src/queries.js, 6 lines) — the source_id/target_id join in exportsFileImpl was swapped, so barrel files weren't detected as re-exporters. Added DISTINCT to prevent duplicates.
  2. Exports integration tests (tests/integration/exports.test.js, 157 lines) — 6 tests covering consumer detection, counts, reexport edges, noTests filtering, empty results, and pagination.

All regressions (builder.js CFG purge removal, stale docs references) were dropped during the rebase.

Full test suite passes: 1306 passed, 0 failed.

@greptileai

@carlos-alm carlos-alm merged commit a97e34d into main Mar 3, 2026
16 checks passed
@carlos-alm carlos-alm deleted the fix/274-greptile-feedback branch March 3, 2026 06:01
@github-actions github-actions bot locked and limited conversation to collaborators Mar 3, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant