refactor(analyzer): single provider registry — collapse 3 hand-synced dispatch tables#649
Merged
Conversation
… dispatch tables The same extension→language/provider mapping existed in three places kept in sync only by comments: AnalyzerPipeline::provider_name_for_path (ecp-core, canonical), detect_needed_providers (ecp-cli admin index), and make_provider/ALL_PROVIDER_NAMES (ecp-cli reanalyze). Drift between them has twice shipped silent bugs (PR #141/#142 missed dispatch site, PR #595 never scanned .yaml). - AnalyzerPipeline::EXTENSION_TABLE (ecp-core) is now the single data-driven source for the plain extension→name mapping; provider_name_for_path derives from it and keeps ownership of the path-based special cases (Dockerfile, docker-compose, GitHub Actions, YAML content-sniff routing). - provider_registry::PROVIDER_CONSTRUCTORS (ecp-cli) is the single name→constructor table; reanalyze::make_provider and admin/index's provider-factory construction both resolve against it instead of each re-listing their own match arms. - detect_needed_providers now resolves paths through the canonical provider_name_for_path instead of re-deriving its own extension match. - Regression tests pin the sync invariants: every EXTENSION_TABLE name is constructible, every path special-case sets its NeededProviders flag, and NeededProviders::is_set agrees with set_needed_flag for every registry name (the yaml/kubernetes dual-flag case is a documented, tested exception).
coseto6125
enabled auto-merge (squash)
July 22, 2026 17:32
coseto6125
disabled auto-merge
July 22, 2026 17:35
…DER_NAMES Since PROVIDER_CONSTRUCTORS now backs make_provider with the full registry (~36 names) instead of reanalyze.rs's old locally-scoped match arms, reanalyze_files's unfiltered provider_name_for_path lookup started resolving names outside ALL_PROVIDER_NAMES (e.g. "bash" for .sh) that the full pipeline deliberately excludes. That gave the incremental path more coverage than make_pipeline(), breaking incremental/full parity — a .sh file yielded 0 graphs via pipeline().analyze() but 1 via reanalyze_files(). Filter the needed-provider set through ALL_PROVIDER_NAMES before building the scoped pipeline, restoring the pre-refactor exclusion. Added a regression test pinning parity for a name outside the subset.
coseto6125
enabled auto-merge (squash)
July 22, 2026 17:46
…for_names The reanalyze_files-only filter added in the prior commit missed a second caller: impact_with_baseline (crates/ecp-cli/src/commands/impact.rs:1312) builds its scoped diff pipeline through the same make_pipeline_for_names helper, unfiltered. Since make_provider now resolves the full provider_registry instead of the old locally-scoped match arms, a name like "bash" (outside ALL_PROVIDER_NAMES, not yet wired into these scoped paths) would register there too — giving impact --baseline more provider coverage than the full pipeline for diffs touching e.g. .sh files. Move the ALL_PROVIDER_NAMES constraint into make_pipeline_for_names itself so every current and future caller inherits it, and drop the now-redundant filter at the reanalyze_files call site. Added test_make_pipeline_for_names_nonsubset_name_skipped, a direct unit test of the helper (verified to fail without the fix).
make_pipeline_for_names's `if let Some(Ok(provider)) = make_provider(name)` silently dropped the Err case, so a broken tree-sitter query would silently skip that language in incremental reanalyze / impact --baseline instead of surfacing the failure. Pre-refactor, every constructor call was `.unwrap()`'d inline and panicked immediately on the same failure. Panic via `.unwrap_or_else` at the point make_provider returns Some(Err(_)), restoring the original fail-fast behavior. Chose this over threading Result through make_pipeline_for_names's signature because its three callers (make_pipeline() via OnceLock::get_or_init, reanalyze_files, impact_with_ baseline) mix Result-free and Result-returning shapes — panicking in the helper matches every one of them without touching any call site. A provider-constructor failure isn't feasible to fixture without either corrupting a real language's compiled-in .scm query (destructive to every other test using that provider) or reworking PROVIDER_CONSTRUCTORS into injectable closures per test — out of scope for this fix. No test added for this specific change; the existing full suite staying green confirms no call site needed updating.
Contributor
ecp impact cache (0 symbols) — internal, used by
|
github-actions
Bot
added
ecp:risk-low
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What / why
The same extension→language/provider mapping existed in three places, kept in sync only by comments — this repo has shipped two real bugs from that drift: one dispatch site missed in PR #141 (fixed by #142), and
.yamlnever scanned by the cold-index path until PR #595.AnalyzerPipeline::provider_name_for_path(crates/ecp-core/src/analyzer/pipeline.rs) — path-based special cases (Dockerfile, docker-compose, GitHub Actions, YAML content-sniff routing) plus a plain extension table.detect_needed_providers(crates/ecp-cli/src/commands/admin/index.rs) — re-listed the same extension table into aNeededProvidersbool-flags struct.make_provider/ALL_PROVIDER_NAMES(crates/ecp-cli/src/reanalyze.rs) — name string → provider constructor, a deliberate subset.Design
AnalyzerPipeline::EXTENSION_TABLE(new,ecp-core) is now the single data-driven source for the plain extension→provider-name mapping.provider_name_for_pathderives from it via a linear scan and keeps ownership of the path-based special cases that aren't extension-derivable.provider_registry::PROVIDER_CONSTRUCTORS(new,crates/ecp-cli/src/provider_registry.rs) is the single name→constructor table (fn() -> anyhow::Result<Box<dyn LanguageProvider>>).reanalyze::make_providerandadmin/index's cold-index provider construction both resolve against it instead of re-listing their own match arms.reanalyze::ALL_PROVIDER_NAMESstays as an explicit filter over registry names (a deliberate subset — crystal/zig/sql aren't wired into the incremental path yet), unchanged in intent, now sourced from the shared constructor table.detect_needed_providersnow resolves each path through the canonicalprovider_name_for_pathand a smallset_needed_flagname→flag table, instead of re-deriving its own extension match.md/txt/rst(markdown, which has no path-dispatch entry) stay as a documented pre-check.ecp-cli(all*Providertypes are defined inecp-analyzer, whichecp-clialready depends on) keyed by name; the extension/name half of the mapping stays inecp-coresinceprovider_name_for_pathisecp-core's existing public API. A test pins that the two halves agree.Zero behavior change
EXTENSION_TABLEis a byte-for-byte transcription of the old match arms.ALL_PROVIDER_NAMES) is unchanged;make_pipeline's markdown/yaml direct registration is unchanged (same.unwrap()semantics, same registration order).NeededProviders's bool-flag struct and its downstreamif needed.Xergonomics are unchanged; construction is now table-driven.Tests
provider_registry::tests— everyEXTENSION_TABLEname (plus the 3 path-special names) resolves to a constructible provider; registry names are unique; every constructor succeeds.admin::index::tests— everyEXTENSION_TABLEentry sets itsNeededProvidersflag; every path special case (Dockerfile/docker-compose/GitHub Actions/generic yaml→kubernetes) sets its flag; markdown extensions set their flag;NeededProviders::is_setagrees withset_needed_flagfor every registry name (with theyaml/kubernetesdual-flag case as a documented, tested exception — no path ever resolves to the bare name"yaml").reanalyze::tests(pre-existing, still pass unmodified) —ALL_PROVIDER_NAMESentries are constructible;make_pipelineholds the named subset plus markdown + yaml.Verify
cargo build -p egent-code-plexus --bin ecp— clean.cargo test -p egent-code-plexus --tests— 205/205 test-result blocks green, 0 failures.cargo test -p ecp-analyzer— green (unaffected by this change; run per the 14-language coverage rule).cargo clippy -p egent-code-plexus --tests— clean, no warnings.rustfmt --edition 2021on all touched files.Deviations from the brief
anyhow = "1.0.82"as a direct dependency ofecp-cli(previously only reached transitively viaecp-analyzer/ecp-core) — needed becauseprovider_registry::PROVIDER_CONSTRUCTORS's constructor signature isanyhow::Result<...>, matching every*Provider::new()inecp-analyzer(allanyhow::Result<Self>exceptMarkdownProvider::new() -> Result<Self, String>, mapped with.map_err(|e| anyhow::anyhow!(e))).