feat(extensions): extension loader with topo sort and lifecycle - #1035
Merged
Merged
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c62dafccd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ariskemper
marked this pull request as draft
April 14, 2026 16:06
- flattenPresets now recursively expands nested presets (extends-of-extends) so a preset that extends another preset resolves to its leaf extensions rather than intermediate preset markers. - topologicalSort cycle-detection compares sorted.length against the count of unique extension names rather than the input array length. Duplicate entries with the same name collapse to one node in the graph, so the old check produced false-positive CIRCULAR_DEPENDENCY_ERROR whenever the same extension appeared twice in the input (e.g. config + package source). - Add tests for both: recursive preset flattening and duplicate-name tolerance.
teardownAll is already idempotent and unconditionally clears setupOrder and the contract registry. Calling reset()/setupOrder=[] after it was a noop on the hot path and obscured the intent. Replace with an unconditional teardownAll() call.
ariskemper
force-pushed
the
feat/extensions-loader
branch
from
April 17, 2026 15:15
2ca06c2 to
281e2ac
Compare
ariskemper
marked this pull request as ready for review
April 17, 2026 15:21
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 281e2ac55c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Three Codex findings on the current loader:
P1 — source-priority winner on register()
setupAll iterated extensions in order and called register() for every
provider, so a lower-priority provider later in the list silently
overwrote a higher-priority one via contracts.set(). detectConflicts
permits such pairs when one source strictly dominates (config beats
package), so the runtime inverted the documented priority model.
Added selectContractProviders() helper in validation.ts that returns
the priority-winning ResolvedExtension per contract. setupAll now
consults that map and skips register() for losers.
P1 — rollback on setup() failure
If ext.setup(ctx) threw, the method unwound without calling the
failing extension's teardown (it was never pushed to setupOrder) and
without clearing earlier extensions. Resources opened mid-setup
leaked; previous extensions stayed "loaded" in an inconsistent state.
Wrapped setup() in try/catch: on failure, best-effort call the
failing extension's teardown (guarded, logged on error), then
teardownAll() to roll back prior extensions and reset the registry,
then re-throw.
P2 — preset cycle guard
flattenPresets recursed on extends children with no visited set, so
A → B → A stack-overflowed. Now threads a DFS path-set through the
recursion and raises EXTENSION_VALIDATION_ERROR on a back edge. A
diamond (preset with the same leaf listed twice) is still accepted —
the set tracks path membership, not global visit state.
Tests (27 steps, up from 16):
- source priority: two-source case, order-insensitive winner
- rollback: prior-extension teardown, failing-ext teardown, registry
cleared, no crash when failing ext has no teardown hook
- cycle guard: A→B→A, self-reference, diamond-is-not-cycle
4 tasks done
This was referenced Sep 7, 2026
This was referenced Sep 8, 2026
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.
Summary
ExtensionLoader.flattenPresets()— recursively expand presetextendsarrays (supports nested presets); cycle-guarded so malformed graphs raise a controlled error instead of stack-overflowingExtensionLoader.topologicalSort()— Kahn's algorithm, contract dependency ordering, tolerant of duplicate extension namesExtensionLoader.setupAll()— validate, audit, register provides, call setup(); idempotent (auto-teardown on re-entry); rolls back on setup failure; honors source-priority atregister()timeExtensionLoader.teardownAll()— reverse-order teardown with error isolationReview feedback addressed
CIRCULAR_DEPENDENCY_ERROR.register()):selectContractProviders()helper invalidation.tsreturns the priority-winningResolvedExtensionper contract;setupAll()gates staticregister()calls on that map, so a lower-priority extension later in iteration order no longer overwrites a higher-priority winner.ext.setup()wrapped in try/catch; on throw, best-effort teardown of the failing extension (error-isolated), thenteardownAll()unwinds prior extensions and resets the registry, then rethrows. Previously-loaded extensions no longer leak on mid-setup failures.flattenPresetsInner()threads a DFS path-set through recursion and raisesEXTENSION_VALIDATION_ERRORon a back edge. A diamond (shared leaf reachable via two parents) is still accepted — the set tracks path membership, not global visits.reset()/setupOrder = []afterteardownAll()— teardown is already idempotent and handles both clears.Test plan
deno task test src/extensions/loader.test.tsdeno task test src/extensions/deno checkclean undernoUncheckedIndexedAccessonloader.ts,validation.ts,loader.test.ts/simplify,/review,/security-reviewclean — no issues flagged at thresholdCloses #1017