Skip to content

fix: include role-based entry points in flow --list#313

Merged
carlos-alm merged 1 commit intomainfrom
fix/flow-list-role-entries
Mar 4, 2026
Merged

fix: include role-based entry points in flow --list#313
carlos-alm merged 1 commit intomainfrom
fix/flow-list-role-entries

Conversation

@carlos-alm
Copy link
Contributor

Summary

  • flow --list returned "No entry points found" on real codebases because listEntryPointsData() only queried framework-prefixed names (route:%, event:%, command:%)
  • The builder correctly classifies exported functions with zero fan-in as role='entry', but the SQL never checked for that column
  • Added OR n.role = 'entry' to the WHERE clause and fall back to type: 'exported' for non-prefixed role-based entries
  • Framework prefix conditions are kept as a safe fallback for older DBs without populated role columns

Test plan

  • Added exportedInit node with role='entry' (no framework prefix) to flow test fixture
  • New test: role-based entries appear in listEntryPointsData() with type: 'exported'
  • Updated existing tests to expect 4 entries (3 framework + 1 role-based)
  • npx vitest run tests/integration/flow.test.js — 19/19 pass
  • npm run lint — clean on modified files (pre-existing warning in queries.test.js unrelated)

`listEntryPointsData()` only queried framework-prefixed names
(route:%, event:%, command:%) but the docstring and builder both
recognize role='entry' nodes. Add `OR n.role = 'entry'` to the
SQL WHERE clause and fall back to type 'exported' for non-prefixed
role-based entries.

Impact: 1 functions changed, 3 affected
@claude
Copy link

claude bot commented Mar 4, 2026

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


PR Review: fix: include role-based entry points in flow --list

Review Progress

  • Read and analyze changed files
  • Phase 1: Root cause analysis
  • Phase 2: Configuration change detection
  • Phase 3: Test integrity assessment
  • Phase 4: Code quality review
  • Final assessment and recommendation

Root Cause Analysis

Problem Being Solved: The flow --list command returned "No entry points found" on real codebases because listEntryPointsData() only searched for framework-prefixed nodes (route:%, event:%, command:%) but ignored functions classified as role='entry' by the builder's role classification system.

Why This Approach: This is a legitimate bug fix that addresses a gap between the role classification logic (in structure.js:classifyNodeRoles()) and the entry point discovery logic (in flow.js:listEntryPointsData()). The builder correctly classifies exported functions with zero fan-in as role='entry' (lines 327-328 in structure.js), but the flow module never checked this column.

Risk Assessment: ✅ LOW RISK - This is a proper fix that aligns two parts of the system rather than a workaround.

Configuration Change Detection

No concerning configuration changes detected

  • No linting rules modified
  • No test timeouts or coverage thresholds altered
  • No build configuration changes

Test Integrity Assessment

Test changes are appropriate and strengthen coverage

Added test coverage for the fix:

  • Lines 83-84: Added exportedInit function as a role-based entry point
  • Line 111: Manually classified it as role='entry' to simulate builder behavior
  • Lines 223-234: Updated test to expect 4 entries instead of 3
  • Lines 248-255: Added explicit test for role-based entry point behavior

Test expectations were properly updated, not weakened:

  • The change from expecting 3 to 4 entries is justified by the legitimate addition of role-based entry points
  • All assertions remain strict and comprehensive

Code Quality Review

✅ SQL Query Enhancement (lines 48-51):

WHERE (
  (${prefixConditions})
  OR n.role = 'entry'  -- NEW: Include role-based entries
)
  • Clean, safe addition that broadens the search scope appropriately
  • No risk of SQL injection (uses prepared statements)
  • Framework prefix conditions are preserved as fallback

✅ Type Assignment Logic (line 65):

type: entryPointType(r.name) || (r.role === 'entry' ? 'exported' : null)
  • Logical fallback: framework prefix → framework type, role-based → 'exported'
  • Maintains backward compatibility with older DBs without role columns

✅ Implementation Quality:

  • Changes are minimal and surgical (only +5/-2 lines in core logic)
  • No breaking changes to existing API
  • Proper handling of edge cases (framework entries still get their specific types)

Minor Observations:

  • The fallback logic assumes role-based entries should be typed as 'exported', which is reasonable but could be documented
  • No performance impact (role column is already indexed in the schema)

Final Assessment

This is a well-executed bug fix that addresses a real gap in functionality. The implementation:

  • Correctly identifies the root cause (missing role column check)
  • Applies the minimal necessary fix without over-engineering
  • Includes comprehensive test coverage for the new behavior
  • Maintains full backward compatibility
  • Has no negative side effects

Strengths:

  1. Surgical precision: Only changes what's necessary
  2. Comprehensive testing: Adds both integration tests and edge case validation
  3. Backward compatibility: Framework prefix logic is preserved
  4. Clear intent: Code changes directly match the problem description

No significant concerns identified.


Final Recommendation

  • Rating: ⭐⭐⭐⭐☆ (4/5)
  • Action: APPROVE
  • Reasoning: This is a solid bug fix with appropriate test coverage. The implementation is clean, safe, and addresses the exact problem described. The slight deduction from 5 stars is only due to minor documentation that could clarify the 'exported' type assignment choice, but this doesn't warrant blocking the PR.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 4, 2026

Greptile Summary

Fixed flow --list to detect role-based entry points (exported functions with zero fan-in) alongside framework-prefixed entry points. The SQL query in listEntryPointsData() now checks both framework prefixes (route:, event:, command:) and the role='entry' column, with a fallback type assignment of 'exported' for non-prefixed entries.

  • Extended WHERE clause to include OR n.role = 'entry'
  • Added conditional type assignment: framework-prefixed entries retain their type, role-based entries get type: 'exported'
  • Maintains backwards compatibility with older databases that lack populated role columns
  • Comprehensive test coverage with new fixture and assertions verifying the behavior

Confidence Score: 5/5

  • This PR is safe to merge with no identified issues
  • The fix is minimal, well-contained, and directly addresses the reported issue. The SQL query extension is correct and maintains backwards compatibility. Type assignment logic properly handles both framework-prefixed and role-based entry points. Test coverage is thorough with clear assertions validating the new behavior.
  • No files require special attention

Important Files Changed

Filename Overview
src/flow.js Extended SQL query to include role-based entry points and added fallback type assignment for non-prefixed entries
tests/integration/flow.test.js Added comprehensive test coverage for role-based entry point detection with new fixture and assertions

Last reviewed commit: 6681597

@carlos-alm carlos-alm merged commit e8b4fc7 into main Mar 4, 2026
16 checks passed
@carlos-alm carlos-alm deleted the fix/flow-list-role-entries branch March 4, 2026 02:52
@github-actions github-actions bot locked and limited conversation to collaborators Mar 4, 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