Repository navigation
NEW @W-24176028@ Add Flow Analyzer engine package - #511
antoinesfdc wants to merge 8 commits into
Conversation
Adds the code-analyzer-flowanalyzer-engine package. The engine collects Flow metadata files from the workspace, bundles them into a metadata archive, authenticates and sends them to the Flow Analyzer API, and maps the returned findings back to locations in the flow files. Includes config, auth service, API client, file collector, archive builder, location mapper, plugin/engine wiring, and unit tests.
Replaces the sample flow fixture with a real flow metadata file and updates the file collector and engine tests to match (new size limits, get_Account element, recordLookups type). Removes the unused sample-flow/notes.txt fixture and updates package-lock.json to register the new workspace package.
|
Git2Gus App is installed but the |
|
Thanks for the contribution! It looks like @antoinesfdc is an internal user so signing the CLA is not required. However, we need to confirm this. |
| } | ||
|
|
||
| private async requestJson<T>(method: 'GET' | 'POST', resourcePath: string, body?: unknown): Promise<T> { | ||
| const version = this.authService.getApiVersion() || this.apiVersion; |
There was a problem hiding this comment.
[P1] Honor the configured API version
FlowAnalyzerAuthService.getApiVersion() always returns a truthy connection version (and even falls back to 64.0), so this.apiVersion is never used here. An org authenticated with an older cached connection version will therefore call that older /services/data/vXX.X/... endpoint even though this engine defaults/configures 68.0; the Flow Analyzer endpoint can then 404 and the engine becomes unusable. Please make the engine's configured api_version take precedence (or set that version on the connection during initialization).
| const detail = err instanceof Error ? err.message : String(err); | ||
| this.emitLogEvent(LogLevel.Error, getMessage('CatalogUnavailable', detail)); | ||
| this.emitDescribeRulesProgressEvent(100); | ||
| return []; |
There was a problem hiding this comment.
[P1] Propagate catalog failures instead of returning no rules
When authentication or the catalog request fails, this converts the failure into an empty catalog. Core then selects zero flowAnalyzer rules, so runRules() is never invoked and its structured skipped insight is never produced; even an explicit --rule-selector flowAnalyzer invocation can finish successfully with no findings. Throwing here lets Core's existing getAllRulesFor() error path record an uninstantiable engine and surface a critical engine error instead of a false-success scan.
- Always use the configured api_version for Connect requests instead of the CLI connection's version, which may predate the Flow Analyzer endpoints. - Propagate rule catalog failures from describeRules so Core reports the engine error instead of silently selecting zero rules. - Make target_org optional: without it the engine is inactive and offers no rules, so scans without --target-org are unaffected. Auth still never falls back to the default org. - Make the package publishable: add it to the publish-package-to-npm workflow and to the not-yet-published list in validate-changed-package-versions.js.
An explicitly configured api_version always wins. Otherwise requests use the target org's API version, but never one older than 68.0 (the first version exposing the Flow Analyzer endpoints), so older CLI connections still work and newer orgs (e.g. 69.0) use their own version. Malformed api_version values are now rejected.
| if (!elementApiName) { | ||
| return undefined; | ||
| } | ||
| const searchTerm = affectedField |
There was a problem hiding this comment.
[P2] Scope location matching to the affected Flow element
When affectedField is absent, this selects the first XML occurrence of elementApiName anywhere in the file. A connector such as get_Account can precede the actual get_Account, causing the diagnostic to point at the referencing connector rather than the element reported by Flow Analyzer. elementType is available but currently ignored during matching. Please scope the lookup to the corresponding element block and its field—preferably with an XML parser—and use the file-level anchor if that identity cannot be established reliably.
| severityLevel: this.toSeverityLevel( | ||
| rule.effectiveSeverity ?? rule.severityLevel ?? rule.severity ?? rule.defaultSeverity | ||
| ), | ||
| tags: tags.length > 0 ? tags : ['Recommended'], |
There was a problem hiding this comment.
[P1] Keep the non-GA Flow Analyzer engine strictly opt-in
Flow Analyzer is a new, non-GA remote engine, but every enabled catalog rule receives the Recommended tag here. When a target org is configured, the default Recommended scan therefore selects this engine and uploads targeted Flow metadata even though the user did not explicitly request Flow Analyzer.
Please follow the established ApexGuru/UIBundle preview pattern: give every Flow Analyzer rule an engine-specific opt-in tag such as flow-analyzer, never add Recommended, and update Core’s rule-selection logic so these rules are excluded from all and severity-based selectors. They should remain selectable explicitly by engine name, rule name, or the opt-in tag. Add selection tests covering Recommended, all, numeric/named severity, engine name, rule name, and the opt-in tag.
|
Review summary I found two issues and posted them inline:
Manual validation before merge Required:
Recommended:
Evidence checked Reviewed the complete dev...6a33e01 diff, production code, tests, package metadata, lockfile, publishing workflow, existing PR discussion, and the ApexGuru/UIBundle opt-in history. The earlier API-version precedence and swallowed catalog-failure findings are fixed in the current head. I did not independently run the test suite or make requests to a Flow Analyzer org. The visible checks currently cover credential scanning, SAST, and CLA rather than the package unit/integration tests. |
| return [ | ||
| "packages/ENGINE-TEMPLATE", | ||
| // TODO: remove once @salesforce/code-analyzer-flowanalyzer-engine is published to npm | ||
| "packages/code-analyzer-flowanalyzer-engine" |
There was a problem hiding this comment.
make sure to revert this post merge
|
Before deciding whether to merge this new engine, I recommend completing and reviewing a comprehensive testing document: https://docs.google.com/document/d/1amoe4NyOCJZVK3RAjZy23jJX9W1TLVhJ/edit At minimum, it should capture evidence for:
Because this introduces a remote, org-backed engine and a new metadata-upload path, I would treat this documented end-to-end evidence as an input to the merge decision rather than relying only on unit tests. |
979a348 to
e85e364
Compare
Adds the code-analyzer-flowanalyzer-engine package. The engine collects Flow metadata files from the workspace, bundles them into a metadata archive, authenticates and sends them to the Flow Analyzer API, and maps the returned findings back to locations in the flow files.
Includes config, auth service, API client, file collector, archive builder, location mapper, plugin/engine wiring, and unit tests.