feat(core): add subflow resolution module and UnresolvedSubflow rule#292
feat(core): add subflow resolution module and UnresolvedSubflow rule#292RubenHalman wants to merge 2 commits intomainfrom
Conversation
RubenHalman
force-pushed
the
subflow-resolution
branch
from
473c45f to
52af66f
Compare
- Add SubflowResolver to detect DML operations in subflows called within loops - Add UnresolvedSubflow rule to warn when subflows cannot be resolved - Add FileSystemResolver for CLI and VSX packages - Add example flows and comprehensive tests Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
RubenHalman
force-pushed
the
subflow-resolution
branch
from
52af66f to
b254ffd
Compare
Changes SummaryThis PR introduces a comprehensive subflow resolution system that enables the Lightning Flow Scanner to analyze flows that call other flows (subflows). The implementation includes a pluggable resolver architecture with filesystem-based and preloaded resolvers, recursive subflow chain detection for DML-in-loop violations, and a new 'unresolved-subflow' system rule to detect missing flow references. Type: feature Components Affected: core-scanning-engine, subflow-resolution-system, rule-registry, loop-violation-detection, cli-package, vsx-package, documentation Files Changed
Architecture Impact
Risk Areas: Recursive subflow resolution with cycle detection - needs thorough testing for deeply nested and circular flow references, FileSystemResolver glob pattern matching and file indexing - potential performance impact on large codebases, Memory usage with PreloadedResolver when loading many flows in browser/MCP contexts, Synchronous getSync() API on resolvers - FileSystemResolver requires pre-loading via eager mode for this to work, Error handling when subflows fail to parse or are inaccessible (managed packages), Cache invalidation in FileSystemResolver when flows are modified during long-running sessions Suggestions
Full review in progress... | Powered by diffray |
![diffray[bot]](https://avatars.githubusercontent.com/in/2250818?s=60&v=4){kind=small}
| async resolve(flowName: string, context?: SubflowResolutionContext): Promise<ResolvedSubflow> { | ||
| // If we have additional search paths from context, rebuild index | ||
| if (context?.searchPaths && context.searchPaths.length > 0) { | ||
| const newPaths = context.searchPaths.filter( | ||
| (p) => !this.options.searchPaths.includes(p) | ||
| ); | ||
| if (newPaths.length > 0) { | ||
| this.options.searchPaths.push(...newPaths); | ||
| await this.buildIndex(); | ||
| } | ||
| } | ||
|
|
||
| const isManaged = this.managedFlows.has(flowName); | ||
|
|
||
| // Skip managed flows if configured | ||
| if (isManaged && this.options.skipManaged) { | ||
| return { | ||
| flowName, | ||
| flow: undefined, | ||
| isManaged: true, | ||
| error: "Managed package flow - skipped", | ||
| }; | ||
| } | ||
|
|
||
| const flow = await this.loadFlow(flowName); | ||
|
|
||
| return { | ||
| flowName, | ||
| flow, | ||
| isManaged, | ||
| error: flow ? undefined : `Flow "${flowName}" not found in search paths`, | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Resolve multiple subflows at once. | ||
| */ | ||
| async resolveMany( | ||
| flowNames: string[], | ||
| context?: SubflowResolutionContext | ||
| ): Promise<Map<string, ResolvedSubflow>> { | ||
| const results = new Map<string, ResolvedSubflow>(); | ||
|
|
||
| // Rebuild index once if needed | ||
| if (context?.searchPaths && context.searchPaths.length > 0) { | ||
| const newPaths = context.searchPaths.filter( | ||
| (p) => !this.options.searchPaths.includes(p) | ||
| ); | ||
| if (newPaths.length > 0) { | ||
| this.options.searchPaths.push(...newPaths); | ||
| await this.buildIndex(); | ||
| } | ||
| } | ||
|
|
||
| // Load all in parallel | ||
| await Promise.all( | ||
| flowNames.map(async (name) => { | ||
| results.set(name, await this.resolve(name)); | ||
| }) | ||
| ); | ||
|
|
||
| return results; | ||
| } |
There was a problem hiding this comment.
🟡 MEDIUM - Duplicated search path mutation logic across resolve methods
Agent: architecture
Category: quality
Description:
The logic for handling context.searchPaths and rebuilding the index is duplicated in both resolve() (lines 190-197) and resolveMany() (lines 232-239). This code duplication violates DRY principles.
Suggestion:
Extract the common search path handling logic into a private helper method like ensureSearchPaths(context?: SubflowResolutionContext): Promise. Both resolve() and resolveMany() should call this helper at the beginning.
Confidence: 85%
Rule: arch_extract_helper_functions
Review ID: 3f6113ae-1ddc-4e81-98a7-5ec058053b15
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
✅ Fixed in e340695
| flowNames.map(async (name) => { | ||
| results.set(name, await this.resolve(name)); | ||
| }) |
There was a problem hiding this comment.
🟡 MEDIUM - resolveMany calls resolve without passing context
Agent: architecture
Category: quality
Description:
The resolveMany method rebuilds the index once (lines 232-239), but then calls this.resolve(name) without passing context. Since resolve() checks context.searchPaths again at the start, this causes the rebuild check to run again (though it won't rebuild since newPaths would be empty). More importantly, if there are any edge cases with context usage, passing it would be more defensive.
Suggestion:
Pass context to each resolve call: results.set(name, await this.resolve(name, context)); to ensure consistent behavior and avoid redundant checks.
Confidence: 70%
Rule: arch_inappropriate_algorithm_choice
Review ID: 3f6113ae-1ddc-4e81-98a7-5ec058053b15
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
✅ Fixed in e340695
| const newPaths = context.searchPaths.filter( | ||
| (p) => !this.options.searchPaths.includes(p) | ||
| ); |
There was a problem hiding this comment.
🟡 MEDIUM - Linear search in array for path deduplication
Agent: performance
Category: performance
Description:
The code uses Array.includes() to check if paths exist in this.options.searchPaths. With many search paths, this becomes O(n) per check. During resolution with many context paths, this could compound.
Suggestion:
Convert searchPaths to a Set for O(1) lookup when filtering new paths: const existingPaths = new Set(this.options.searchPaths); const newPaths = context.searchPaths.filter(p => !existingPaths.has(p));
Confidence: 65%
Rule: perf_linear_search_to_set
Review ID: 3f6113ae-1ddc-4e81-98a7-5ec058053b15
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
✅ Fixed in e340695
| if (context?.searchPaths && context.searchPaths.length > 0) { | ||
| const newPaths = context.searchPaths.filter( | ||
| (p) => !this.options.searchPaths.includes(p) | ||
| ); | ||
| if (newPaths.length > 0) { | ||
| this.options.searchPaths.push(...newPaths); | ||
| await this.buildIndex(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 MEDIUM - Potential path traversal via context.searchPaths
Agent: security
Category: security
Description:
The resolve() method accepts searchPaths from SubflowResolutionContext and adds them to the resolver's search paths without validation. If an attacker can control the context parameter, they could inject paths outside intended directories.
Suggestion:
Validate and sanitize context.searchPaths before adding them. Use path.resolve() to normalize paths and verify they are within allowed directories.
Confidence: 60%
Rule: sec_semgrep_scan
Review ID: 3f6113ae-1ddc-4e81-98a7-5ec058053b15
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
✅ Fixed in e340695
| if (context?.searchPaths && context.searchPaths.length > 0) { | ||
| const newPaths = context.searchPaths.filter( | ||
| (p) => !this.options.searchPaths.includes(p) | ||
| ); | ||
| if (newPaths.length > 0) { | ||
| this.options.searchPaths.push(...newPaths); | ||
| await this.buildIndex(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 MEDIUM - Potential path traversal via context.searchPaths
Agent: security
Category: security
Description:
The resolve() method accepts searchPaths from SubflowResolutionContext and adds them to the resolver's search paths without validation. If an attacker can control the context parameter, they could inject paths outside intended directories.
Suggestion:
Validate and sanitize context.searchPaths before adding them. Use path.resolve() to normalize paths and verify they are within allowed directories.
Confidence: 60%
Rule: sec_semgrep_scan
Review ID: 3f6113ae-1ddc-4e81-98a7-5ec058053b15
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
✅ Fixed in e340695
| } | ||
| return statementsInLoops; | ||
| } | ||
|
|
||
| /** | ||
| * Find subflows called from within loops and check if they contain violating statements. | ||
| * Recursively checks nested subflow chains. | ||
| * Uses synchronous resolution via getSync(). | ||
| */ | ||
| private findStatementsInSubflowsSync( | ||
| flow: Flow, | ||
| loopElements: FlowNode[], | ||
| resolver: SubflowResolver |
There was a problem hiding this comment.
🟠 HIGH - Quadratic loops - O(n²) complexity
Agent: performance
Category: performance
Description:
The method findStatementsInLoops contains nested loops iterating over loopElements and then loopElems for each loop element. With many loops and elements inside each loop, this creates O(n²) complexity.
Suggestion:
Consider caching loop elements or restructuring to collect all loop elements first, then filter by statement type in a single pass.
Confidence: 85%
Rule: perf_quadratic_loops
Review ID: 3f6113ae-1ddc-4e81-98a7-5ec058053b15
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| private findStatementsInSubflowsSync( | ||
| flow: Flow, | ||
| loopElements: FlowNode[], | ||
| resolver: SubflowResolver | ||
| ): SubflowViolation[] { | ||
| const violations: SubflowViolation[] = []; | ||
|
|
||
| // Check if resolver supports synchronous access | ||
| if (!resolver.getSync) { | ||
| return violations; | ||
| } | ||
|
|
||
| for (const loopElement of loopElements) { | ||
| const loopElems = flow.graph.getLoopElements(loopElement.name); | ||
|
|
||
| for (const elemName of loopElems) { | ||
| const node = flow.graph.getNode(elemName); | ||
| if (!node || node.subtype !== "subflows") continue; | ||
|
|
||
| const subflowName = node.flowName; | ||
| if (!subflowName) continue; | ||
|
|
||
| // Recursively find violations in this subflow and its children | ||
| const subflowViolations = this.findViolationsInSubflowRecursive( | ||
| subflowName, | ||
| node, | ||
| resolver, | ||
| new Set<string>() // Track visited flows to prevent infinite loops | ||
| ); | ||
|
|
||
| violations.push(...subflowViolations); | ||
| } | ||
| } | ||
|
|
||
| return violations; | ||
| } |
There was a problem hiding this comment.
🟡 MEDIUM - Complex recursive subflow traversal should be extracted
Agent: architecture
Category: quality
Description:
The findStatementsInSubflowsSync and findViolationsInSubflowRecursive methods implement complex graph traversal logic with cycle detection, caching, and nested recursion. This logic is embedded in LoopRuleCommon, making it responsible for both loop detection AND graph traversal.
Suggestion:
Extract the subflow graph traversal logic into a separate SubflowTraverser class. LoopRuleCommon should use this traverser as a dependency.
Confidence: 75%
Rule: arch_srp_violation
Review ID: 3f6113ae-1ddc-4e81-98a7-5ec058053b15
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
Review SummaryValidated 95 issues: 28 kept, 67 filtered Issues Found: 28💬 See 27 individual line comment(s) for details. 📊 21 unique issue type(s) across 28 location(s) 📋 Full issue list (click to expand)🟠 HIGH - Quadratic loops - O(n²) complexityAgent: performance Category: performance File: Description: The method findStatementsInLoops contains nested loops iterating over loopElements and then loopElems for each loop element. With many loops and elements inside each loop, this creates O(n²) complexity. Suggestion: Consider caching loop elements or restructuring to collect all loop elements first, then filter by statement type in a single pass. Confidence: 85% Rule: 🟠 HIGH - NEW FileSystemResolver completely missing test coverage (3 occurrences)Agent: testing Category: quality 📍 View all locations
Rule: 🟡 MEDIUM - Confusing test name contradicts assertion (2 occurrences)Agent: testing Category: quality Why this matters: Misleading test names cause confusion during debugging and can lead to tests being trusted when they don't validate the intended behavior. 📍 View all locations
Rule: 🟡 MEDIUM - Missing type annotations on method parametersAgent: typescript Category: quality Why this matters: Missing type annotations bypass TypeScript's type checking and can lead to runtime errors that could have been caught at compile time. File: Description: The getConnectors method has parameters 'subtype' and 'element' without type annotations, resulting in implicit 'any' types. Suggestion: Add explicit type annotations: private getConnectors(subtype: string, element: any): FlowElementConnector[] Confidence: 90% Rule: 🟡 MEDIUM - Circular reference test only validates one scenarioAgent: testing Category: quality Why this matters: Different cycle patterns could have edge cases in the detection algorithm. Testing multiple scenarios ensures robustness. File: Description: The circular reference test only checks Flow_A -> Flow_B -> Flow_A (2-node cycle). It doesn't test self-reference (A -> A) or 3-node cycles (A -> B -> C -> A). Suggestion: Add test cases for self-referencing flow and 3-node circular chain to ensure cycle detection handles all scenarios Confidence: 75% Rule: 🟡 MEDIUM - Duplicated search path mutation logic across resolve methods (2 occurrences)Agent: architecture Category: quality 📍 View all locations
Rule: 🟡 MEDIUM - resolveMany calls resolve without passing contextAgent: architecture Category: quality File: Description: The resolveMany method rebuilds the index once (lines 232-239), but then calls this.resolve(name) without passing context. Since resolve() checks context.searchPaths again at the start, this causes the rebuild check to run again (though it won't rebuild since newPaths would be empty). More importantly, if there are any edge cases with context usage, passing it would be more defensive. Suggestion: Pass context to each resolve call: Confidence: 70% Rule: 🟡 MEDIUM - Use of 'as any' type assertion bypasses type safetyAgent: react Category: quality File: Description: The code uses 'as any' to access nested properties noMoreValuesConnector.targetReference, which bypasses TypeScript's type checking. This could hide potential runtime errors if the element structure changes. Suggestion: Define a proper interface for the element type or use type guards instead of casting to any. Confidence: 75% Rule: 🟡 MEDIUM - Live file I/O in test - should use mocks or be marked as integration testAgent: microservices Category: quality File: Description: This test file performs live file system operations using the parse() function to read actual XML flow files from disk in the beforeAll() hook. While this is valid for integration testing, it's not clearly marked as such. Suggestion: Either rename to SubflowDMLInLoop.integration.test.ts to mark it as an integration test, or add a comment explaining why live I/O is needed here. Confidence: 65% Rule: 🟡 MEDIUM - Linear search in array for path deduplicationAgent: performance Category: performance File: Description: The code uses Array.includes() to check if paths exist in this.options.searchPaths. With many search paths, this becomes O(n) per check. During resolution with many context paths, this could compound. Suggestion: Convert searchPaths to a Set for O(1) lookup when filtering new paths: Confidence: 65% Rule: 🟡 MEDIUM - Potential path traversal via context.searchPaths (2 occurrences)Agent: security Category: security 📍 View all locations
Rule: 🟡 MEDIUM - Complex method with deep nestingAgent: quality Category: quality File: Description: The findViolationsInSubflowRecursive method is complex with 4+ levels of nesting and spans 62 lines with multiple responsibilities: cycle detection, flow resolution, caching, violation detection, and recursive traversal. Suggestion: Break this method into smaller focused methods: shouldProcessSubflow(), resolveAndCacheSubflow(), findDirectViolations(), findNestedViolations(). Confidence: 70% Rule: 🟡 MEDIUM - Complex recursive subflow traversal should be extractedAgent: architecture Category: quality File: Description: The findStatementsInSubflowsSync and findViolationsInSubflowRecursive methods implement complex graph traversal logic with cycle detection, caching, and nested recursion. This logic is embedded in LoopRuleCommon, making it responsible for both loop detection AND graph traversal. Suggestion: Extract the subflow graph traversal logic into a separate SubflowTraverser class. LoopRuleCommon should use this traverser as a dependency. Confidence: 75% Rule: 🟡 MEDIUM - Large function with multiple responsibilitiesAgent: architecture Category: quality File: Description: The main loop in ScanFlows (lines 74-132) handles multiple responsibilities: flow conversion, rule iteration, configuration merging, execution, message customization, URL generation, line number enrichment, error handling, and caching. Suggestion: Decompose into smaller functions: processFlow(), executeRule(), enrichRuleResult(). Confidence: 65% Rule: 🟡 MEDIUM - Nested loops with subflow scanning could lead to O(n²) complexityAgent: performance Category: performance File: Description: The findStatementsInSubflowsSync method has nested loops iterating over loopElements and loopElems, then recursively traversing subflows. However, this is mitigated by: 1) early exit at line 120-122 if resolver lacks getSync, 2) caching at line 179-183, 3) visited set preventing cycles. Suggestion: The concern is valid but overstated. The caching strategy (resolvedSubflows Map) already mitigates repeated resolutions. Consider documenting the O(loops × elementsPerLoop × uniqueSubflows) complexity. Confidence: 62% Rule: 🟡 MEDIUM - Missing edge case: PreloadedResolver should test duplicate flow names (3 occurrences)Agent: testing Category: quality 📍 View all locations
Rule: 🟡 MEDIUM - Missing test coverage for SubflowResolutionContext parameterAgent: testing Category: quality File: Description: The SubflowResolutionContext interface defines parentFlow, parentFlowPath, and searchPaths fields, but the core tests never pass a context parameter to resolve() or resolveMany(). The context parameter behavior is completely untested. Suggestion: Add tests that pass SubflowResolutionContext to verify: 1) PreloadedResolver ignores context (expected), 2) NoOpResolver ignores context (expected), 3) Document which resolvers use which context fields Confidence: 80% Rule: 🔵 LOW - Trivial test checks instanceof rather than behaviorAgent: testing Category: quality Why this matters: Type checks pass even if behavior is broken. Testing behavior ensures the resolver works correctly. File: Description: The test only checks instanceof NoOpResolver, not that defaultResolver actually behaves correctly (returns undefined for all resolutions). Suggestion: Add behavior verification: const result = await defaultResolver.resolve('AnyFlow'); expect(result.flow).toBeUndefined(); Confidence: 65% Rule: 🔵 LOW - Loop-invariant computation inside recursive functionAgent: performance Category: performance File: Description: getStatementTypes() is called on every recursive invocation of findViolationsInSubflowRecursive. Since the result doesn't change, it should be computed once and passed as a parameter. Suggestion: Pass statementTypes as a parameter to avoid repeated calls across recursive invocations. Confidence: 80% Rule: 🔵 LOW - Multiple array operations on same dataAgent: performance Category: performance File: Description: The getSubflowNames() method performs multiple array operations in sequence: filter(), map(), filter(), then creates a Set and converts back to array. This creates multiple intermediate arrays. Suggestion: Combine operations into a single pass using a for loop that populates a Set directly. Confidence: 70% Rule: 🔵 LOW - Unused private methodAgent: react Category: quality File: Description: The private method 'findLoopElements' is defined but never called. The code at line 39 directly calls flow.graph.getLoopNodes() instead. Suggestion: Remove this unused method to reduce code complexity. Confidence: 90% Rule: ℹ️ 1 issue(s) outside PR diff (click to expand)
🟡 MEDIUM - Missing type annotations on method parametersAgent: typescript Category: quality Why this matters: Missing type annotations bypass TypeScript's type checking and can lead to runtime errors that could have been caught at compile time. File: Description: The getConnectors method has parameters 'subtype' and 'element' without type annotations, resulting in implicit 'any' types. Suggestion: Add explicit type annotations: private getConnectors(subtype: string, element: any): FlowElementConnector[] Confidence: 90% Rule: 🔇 6 low-severity issue(s) not posted (min: medium)
📝 11 additional issue(s) shown in summary only (max: 10 inline)
Review ID: |
diffray
bot
added
diffray-review-completed
diffray
bot
added
diffray-review-started
Changes SummarySecurity hardening of FileSystemResolver implementations in both CLI and VSX packages by removing dynamic search path expansion feature and adding XML validation to prevent parsing of arbitrary files. Also includes documentation updates moving SUBFLOW_RESOLUTION.md to the docs directory and updating the SubflowResolutionContext interface. Type: mixed Components Affected: FileSystemResolver (CLI), FileSystemResolver (VSX), SubflowResolver Core Interface, Documentation Files Changed
Architecture Impact
Risk Areas: XML validation pattern might be too strict or cause false negatives if Salesforce Flow files have unusual formatting, Removing dynamic search path expansion could break workflows that relied on context-based path resolution, Reading first 1KB of files could have performance implications when resolving many subflows, Buffer allocation for every file validation adds memory overhead Suggestions
Full review in progress... | Powered by diffray |
| private async validateFlowContent(filePath: string): Promise<boolean> { | ||
| try { | ||
| // Read only the first 1KB to check for Flow root element | ||
| const fd = await fs.open(filePath, "r"); | ||
| const buffer = Buffer.alloc(1024); | ||
| await fd.read(buffer, 0, 1024, 0); | ||
| await fd.close(); | ||
| const header = buffer.toString("utf8"); | ||
| return FLOW_ROOT_PATTERN.test(header); | ||
| } catch { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟠 HIGH - File handle not closed on error path
Agent: performance
Category: performance
Description:
File descriptor opened with fs.open() is not closed if fd.read() throws an error. The catch block at line 170 prevents fd.close() from executing, causing file handle leak in long-running processes.
Suggestion:
Wrap fd.close() in a finally block or use try-finally pattern to ensure cleanup happens on all code paths:
try {
const fd = await fs.open(filePath, "r");
try {
const buffer = Buffer.alloc(1024);
await fd.read(buffer, 0, 1024, 0);
const header = buffer.toString("utf8");
return FLOW_ROOT_PATTERN.test(header);
} finally {
await fd.close();
}
} catch {
return false;
}
Confidence: 95%
Rule: perf_unclosed_resources
Review ID: 501cb6a3-241d-4a88-8a7f-1d126fe58eb8
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| private async validateFlowContent(filePath: string): Promise<boolean> { | ||
| try { | ||
| // Read only the first 1KB to check for Flow root element | ||
| const fd = await fs.open(filePath, "r"); | ||
| const buffer = Buffer.alloc(1024); | ||
| await fd.read(buffer, 0, 1024, 0); | ||
| await fd.close(); | ||
| const header = buffer.toString("utf8"); | ||
| return FLOW_ROOT_PATTERN.test(header); | ||
| } catch { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟠 HIGH - File handle not closed on error path
Agent: performance
Category: performance
Description:
File descriptor opened with fs.open() is not closed if fd.read() throws an error. The catch block at line 146 prevents fd.close() from executing, causing file handle leak in long-running VS Code extension.
Suggestion:
Wrap fd.close() in a finally block or use try-finally pattern to ensure cleanup happens on all code paths:
try {
const fd = await fs.open(filePath, "r");
try {
const buffer = Buffer.alloc(1024);
await fd.read(buffer, 0, 1024, 0);
const header = buffer.toString("utf8");
return FLOW_ROOT_PATTERN.test(header);
} finally {
await fd.close();
}
} catch {
return false;
}
Confidence: 95%
Rule: perf_unclosed_resources
Review ID: 501cb6a3-241d-4a88-8a7f-1d126fe58eb8
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| private async validateFlowContent(filePath: string): Promise<boolean> { | ||
| try { | ||
| // Read only the first 1KB to check for Flow root element | ||
| const fd = await fs.open(filePath, "r"); | ||
| const buffer = Buffer.alloc(1024); | ||
| await fd.read(buffer, 0, 1024, 0); | ||
| await fd.close(); | ||
| const header = buffer.toString("utf8"); | ||
| return FLOW_ROOT_PATTERN.test(header); | ||
| } catch { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 MEDIUM - Low-level file I/O could be extracted into helper
Agent: architecture
Category: quality
Description:
The validateFlowContent() method contains low-level file descriptor operations (open, read, close) mixed with validation logic. This violates single responsibility principle.
Suggestion:
Extract the file reading logic into a separate helper function like readFileHeader(filePath: string, bytes: number): Promise<string>. This would make validateFlowContent() focused solely on validation and improve testability.
Confidence: 65%
Rule: arch_extract_helper_functions
Review ID: 501cb6a3-241d-4a88-8a7f-1d126fe58eb8
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| private async validateFlowContent(filePath: string): Promise<boolean> { | ||
| try { | ||
| // Read only the first 1KB to check for Flow root element | ||
| const fd = await fs.open(filePath, "r"); | ||
| const buffer = Buffer.alloc(1024); | ||
| await fd.read(buffer, 0, 1024, 0); | ||
| await fd.close(); | ||
| const header = buffer.toString("utf8"); | ||
| return FLOW_ROOT_PATTERN.test(header); | ||
| } catch { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 MEDIUM - Low-level file I/O could be extracted into helper
Agent: architecture
Category: quality
Description:
The validateFlowContent() method contains low-level file descriptor operations mixed with validation logic. This is duplicated code from the CLI package.
Suggestion:
Extract the file reading logic into a shared helper function. Since both CLI and VSX packages have identical implementations, consider moving this to a shared utility module to avoid duplication.
Confidence: 65%
Rule: arch_extract_helper_functions
Review ID: 501cb6a3-241d-4a88-8a7f-1d126fe58eb8
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| async resolve(flowName: string, _context?: SubflowResolutionContext): Promise<ResolvedSubflow> { | ||
| const isManaged = this.managedFlows.has(flowName); | ||
|
|
||
| // Skip managed flows if configured | ||
| if (isManaged && this.options.skipManaged) { | ||
| return { | ||
| flowName, | ||
| flow: undefined, | ||
| isManaged: true, | ||
| error: "Managed package flow - skipped", | ||
| }; | ||
| } | ||
|
|
||
| const flow = await this.loadFlow(flowName); | ||
|
|
||
| return { | ||
| flowName, | ||
| flow, | ||
| isManaged, | ||
| error: flow ? undefined : `Flow "${flowName}" not found in search paths`, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🟠 HIGH - Removed parameter behavior lacks test coverage for regression
Agent: testing
Category: quality
Description:
The 'context.searchPaths' parameter functionality was removed from the resolve() and resolveMany() methods. The previous implementation allowed dynamic addition of search paths via the context parameter (lines 221-232 in commit b254ffd), which would rebuild the index with new paths. This behavior is now completely removed, but there are no tests to verify: (1) the old behavior is intentionally removed, (2) existing code that might depend on this feature won't break, (3) the security concern mentioned in comments ('Search paths are fixed at construction time for security') is actually enforced.
Suggestion:
Add tests to verify the security constraint that search paths cannot be modified after construction. Create test cases in packages/cli/test/libs/FileSystemResolver.test.ts that verify: (1) passing context with searchPaths to resolve() does not add new search paths, (2) passing context with searchPaths to resolveMany() does not add new search paths, (3) the flowIndex remains unchanged when context.searchPaths is provided, (4) only flows from the initial searchPaths in constructor options are resolved. Also consider adding a test that documents why this was removed (security fix).
Confidence: 75%
Rule: test_new_parameter_coverage
Review ID: 501cb6a3-241d-4a88-8a7f-1d126fe58eb8
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| /** | ||
| * Validate that file content appears to be a Salesforce Flow XML. | ||
| * Checks for <Flow xmlns= pattern to prevent parsing arbitrary XML files. | ||
| */ | ||
| private async validateFlowContent(filePath: string): Promise<boolean> { | ||
| try { | ||
| // Read only the first 1KB to check for Flow root element | ||
| const fd = await fs.open(filePath, "r"); | ||
| const buffer = Buffer.alloc(1024); | ||
| await fd.read(buffer, 0, 1024, 0); | ||
| await fd.close(); | ||
| const header = buffer.toString("utf8"); | ||
| return FLOW_ROOT_PATTERN.test(header); | ||
| } catch { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟠 HIGH - Duplicated validation logic across packages
Agent: react
Category: quality
Description:
The validateFlowContent() method is duplicated identically in both packages/cli and packages/vsx FileSystemResolver implementations. This violates DRY principle and creates maintenance burden - bug fixes or improvements must be applied in both locations.
Suggestion:
Extract the validateFlowContent() method to a shared utility module in the core package (e.g., @flow-scanner/lightning-flow-scanner-core/src/utils/validateFlowContent.ts) and import it in both FileSystemResolver implementations.
Confidence: 85%
Rule: ts_consolidate_duplicated_schemas
Review ID: 501cb6a3-241d-4a88-8a7f-1d126fe58eb8
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| const fd = await fs.open(filePath, "r"); | ||
| const buffer = Buffer.alloc(1024); | ||
| await fd.read(buffer, 0, 1024, 0); | ||
| await fd.close(); |
There was a problem hiding this comment.
🟡 MEDIUM - File read result not validated before use
Agent: bugs
Category: bug
Description:
The fd.read() method returns { bytesRead, buffer }, but bytesRead is not checked. If the file is empty, buffer may contain uninitialized zeros, though in this case the regex test would still return false correctly.
Suggestion:
Store the read result and validate bytesRead: const { bytesRead } = await fd.read(buffer, 0, 1024, 0); then check if (bytesRead === 0) return false; before testing the pattern.
Confidence: 65%
Rule: ts_validate_nullable_before_use
Review ID: 501cb6a3-241d-4a88-8a7f-1d126fe58eb8
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| static async fromFlow(flow: Flow, additionalPaths?: string[]): Promise<FileSystemResolver> { | ||
| const searchPaths: string[] = []; | ||
|
|
||
| // Add directory containing the parent flow | ||
| if (flow.fsPath) { | ||
| searchPaths.push(path.dirname(flow.fsPath)); | ||
| } else if (flow.uri) { | ||
| searchPaths.push(path.dirname(flow.uri)); | ||
| } | ||
|
|
||
| // Add additional search paths | ||
| if (additionalPaths) { | ||
| searchPaths.push(...additionalPaths); | ||
| } | ||
|
|
||
| return FileSystemResolver.create({ searchPaths }); |
There was a problem hiding this comment.
🟡 MEDIUM - Empty searchPaths array when flow has no path
Agent: bugs
Category: bug
Description:
In fromFlow(), if both flow.fsPath and flow.uri are undefined/null/falsy, and no additionalPaths are provided, searchPaths will be empty. This creates a resolver that cannot find any flows.
Suggestion:
Add validation after building searchPaths: if (searchPaths.length === 0) { throw new Error('Cannot create resolver: flow has no path and no additional paths provided'); } or document that this is acceptable behavior.
Confidence: 60%
Rule: bug_array_bounds
Review ID: 501cb6a3-241d-4a88-8a7f-1d126fe58eb8
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| export interface SubflowResolutionContext { | ||
| /** The parent flow containing the subflow reference */ | ||
| parentFlow: Flow; | ||
| /** File path of the parent flow (if available) */ | ||
| parentFlowPath?: string; |
There was a problem hiding this comment.
🟠 HIGH - Breaking API change: removed optional property from public interface
Agent: architecture
Category: quality
Description:
The searchPaths optional property was removed from SubflowResolutionContext interface. This interface is exported publicly in packages/core/src/index.ts line 82. Consumers passing searchPaths will get TypeScript errors after upgrading.
Suggestion:
For backward compatibility, either: (1) Keep the searchPaths?: string[] property in the interface and ignore it in implementations, or (2) Mark this as a major version change.
Confidence: 85%
Rule: api_backward_compatibility
Review ID: 501cb6a3-241d-4a88-8a7f-1d126fe58eb8
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| return parsed[0].flow; | ||
| } | ||
| } catch (error) { | ||
| console.warn(`Failed to load subflow "${flowName}" from ${filePath}:`, error); |
There was a problem hiding this comment.
🟡 MEDIUM - Console.warn statement in production code for error handling
Agent: accessibility
Category: quality
Description:
console.warn is used in catch blocks (lines 166, 176) to log failures. In a VS Code extension, console.warn output goes to the extension host log which users rarely monitor. This provides no user feedback for errors.
Suggestion:
For VS Code extension code, consider using vscode.window.showWarningMessage() for user-facing errors or an OutputChannel for detailed logs that users can optionally access.
Confidence: 75%
Rule: fe_console_in_production
Review ID: 501cb6a3-241d-4a88-8a7f-1d126fe58eb8
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
Review Summary✅ Fixed Issues: 5View fixed issues
Validated 33 issues: 12 kept, 21 filtered Issues Found: 12💬 See 12 individual line comment(s) for details. 📊 10 unique issue type(s) across 12 location(s) 📋 Full issue list (click to expand)🟠 HIGH - File handle not closed on error path (2 occurrences)Agent: performance Category: performance 📍 View all locations
Rule: 🟠 HIGH - Removed parameter behavior lacks test coverage for regressionAgent: testing Category: quality File: Description: The 'context.searchPaths' parameter functionality was removed from the resolve() and resolveMany() methods. The previous implementation allowed dynamic addition of search paths via the context parameter (lines 221-232 in commit b254ffd), which would rebuild the index with new paths. This behavior is now completely removed, but there are no tests to verify: (1) the old behavior is intentionally removed, (2) existing code that might depend on this feature won't break, (3) the security concern mentioned in comments ('Search paths are fixed at construction time for security') is actually enforced. Suggestion: Add tests to verify the security constraint that search paths cannot be modified after construction. Create test cases in packages/cli/test/libs/FileSystemResolver.test.ts that verify: (1) passing context with searchPaths to resolve() does not add new search paths, (2) passing context with searchPaths to resolveMany() does not add new search paths, (3) the flowIndex remains unchanged when context.searchPaths is provided, (4) only flows from the initial searchPaths in constructor options are resolved. Also consider adding a test that documents why this was removed (security fix). Confidence: 75% Rule: 🟠 HIGH - Duplicated validation logic across packagesAgent: react Category: quality File: Description: The validateFlowContent() method is duplicated identically in both packages/cli and packages/vsx FileSystemResolver implementations. This violates DRY principle and creates maintenance burden - bug fixes or improvements must be applied in both locations. Suggestion: Extract the validateFlowContent() method to a shared utility module in the core package (e.g., @flow-scanner/lightning-flow-scanner-core/src/utils/validateFlowContent.ts) and import it in both FileSystemResolver implementations. Confidence: 85% Rule: 🟠 HIGH - Breaking API change: removed optional property from public interfaceAgent: architecture Category: quality File: Description: The Suggestion: For backward compatibility, either: (1) Keep the Confidence: 85% Rule: 🟠 HIGH - Blocking synchronous glob operation in async contextAgent: performance Category: performance File: Description: glob.sync() blocks the event loop during filesystem scanning. This is called from buildIndex() inside async factory methods (create, fromDirectory, fromFlow). For large codebases, this blocks other async operations. Suggestion: Replace glob.sync() with the async glob() from the glob library. Example: Confidence: 85% Rule: 🟡 MEDIUM - Low-level file I/O could be extracted into helper (2 occurrences)Agent: architecture Category: quality 📍 View all locations
Rule: 🟡 MEDIUM - File read result not validated before useAgent: bugs Category: bug File: Description: The fd.read() method returns { bytesRead, buffer }, but bytesRead is not checked. If the file is empty, buffer may contain uninitialized zeros, though in this case the regex test would still return false correctly. Suggestion: Store the read result and validate bytesRead: Confidence: 65% Rule: 🟡 MEDIUM - Empty searchPaths array when flow has no pathAgent: bugs Category: bug File: Description: In fromFlow(), if both flow.fsPath and flow.uri are undefined/null/falsy, and no additionalPaths are provided, searchPaths will be empty. This creates a resolver that cannot find any flows. Suggestion: Add validation after building searchPaths: Confidence: 60% Rule: 🟡 MEDIUM - Console.warn statement in production code for error handlingAgent: accessibility Category: quality File: Description: console.warn is used in catch blocks (lines 166, 176) to log failures. In a VS Code extension, console.warn output goes to the extension host log which users rarely monitor. This provides no user feedback for errors. Suggestion: For VS Code extension code, consider using vscode.window.showWarningMessage() for user-facing errors or an OutputChannel for detailed logs that users can optionally access. Confidence: 75% Rule: 🟡 MEDIUM - Code duplication between CLI and VSX FileSystemResolverAgent: dry Category: quality File: Description: CLI (317 lines) and VSX (265 lines) FileSystemResolver implementations share significant structure but aren't 99% identical. The main difference is buildIndex() implementation. Both also have duplicated FindFlows.ts utilities that are nearly identical. Suggestion: Consider extracting shared logic into packages/core as a base class with abstract file discovery method. The FindFlows.ts files in both packages are also nearly identical and could be consolidated. Confidence: 70% Rule: 📝 2 additional issue(s) shown in summary only (max: 10 inline)
Review ID: |
diffray
bot
added
diffray-review-completed
1 participant
#272