Refactor Pipeline Architecture & Fix Diagnostic Handling#393
Refactor Pipeline Architecture & Fix Diagnostic Handling#393pipewrk merged 6 commits intowpkernel:mainfrom
Conversation
…ine` directories and fix registration diagnostic handling.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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".
|
Important Review skippedMore than 25% of the files skipped due to max files limit. The review is being skipped to prevent a low-quality review. 193 files out of 300 files are above the max files limit of 100. Please upgrade to Pro plan to get higher limits. You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaces fragment/builder pipeline with an agnostic core: introduces makePipeline/initAgnosticRunner, a runner/program/diagnostics/rollback subsystem, moves core exports under core/, and adds a Standard Pipeline adapter that maps legacy fragment/builder lifecycles onto the new agnostic core. Tests and docs updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Pipeline as Agnostic Pipeline
participant Runner as initAgnosticRunner
participant Context as prepareContext
participant Diag as DiagnosticManager
participant Program as createAgnosticProgram
participant Helpers as Helpers/Extensions
participant Rollback as RollbackCoordinator
Client->>Pipeline: register extensions/helpers (use)
Client->>Pipeline: run(options)
Pipeline->>Runner: prepareContext(options)
Runner->>Context: create context, resolve helper orders
Context->>Diag: prepareRun, setReporter
Runner->>Program: build program (stages)
Program->>Helpers: execute helpers (staged)
alt helper failure
Program->>Rollback: run rollback plan (LIFO)
Rollback->>Helpers: invoke helper/extension rollbacks
Rollback->>Diag: flag/emit rollback diagnostics
Rollback-->>Pipeline: halt with error
else success
Program->>Diag: endRun()
Program-->>Pipeline: return run result
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Areas needing careful review:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
- introduce standard pipeline types, - and update API documentation across packages.
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/pipeline/src/core/extensions/index.ts (1)
1-8:export * from './runner': ensure no export name collisions + confirm intended public surface.If
./runnerexports any of the same symbols as./official(now or in the future), this can break builds or create confusing API. Consider explicitly re-exporting the intended runner symbols (or add a lint/test that checks for duplicates).
🧹 Nitpick comments (23)
packages/core/src/resource/define.ts (1)
168-182: Redundant/unreachable post-validateConfigname checks; prefer one validation path + consistent error shape.
validateConfig(config)already validatesconfig.name(presence/type/format). The subsequentif (!config.name || typeof config.name !== 'string')block is now effectively dead for invalid inputs, and it emits a different error payload than the structuredvalidationErrorsfromvalidateConfig.Suggested cleanup:
export function defineResource< T = unknown, TQuery = unknown, const TRoutes extends ResourceRoutes = ResourceRoutes, >( config: ResourceConfig<T, TQuery, TRoutes> ): ResourceObject<T, TQuery, TRoutes> { if (!config || typeof config !== 'object') { throw new WPKernelError('DeveloperError', { message: 'defineResource requires a configuration object with "name" and "routes".', }); } validateConfig(config); - if (!config.name || typeof config.name !== 'string') { - throw new WPKernelError('DeveloperError', { - message: - 'defineResource requires a non-empty string "name" property.', - }); - } - const { namespace, resourceName } = resolveNamespaceAndName(config); // ... }(If you want to preserve that exact error message, move the
config.namecheck beforevalidateConfigand consider aligningvalidateConfigmessaging/shape instead.)Also applies to: 175-182
packages/pipeline/src/core/__tests__/helper-rollback.test.ts (1)
5-12: Avoid leaving commented-out imports in the long termThe “
// Pipeline, // Moved to standard-pipeline/types” is helpful during migration, but once the move is complete, consider deleting the commented line and keeping the note (if needed) in a short file comment or changelog to reduce noise.packages/pipeline/src/standard-pipeline/__tests__/createPipeline.test.ts (1)
250-256: Consider usingexpect().rejectsfor cleaner async rejection testing.The try-catch pattern works but Jest's built-in matcher is more idiomatic and provides better error messages on failure.
- try { - await pipeline.run({}); - throw new Error('expected pipeline.run() to reject'); - } catch (error) { - expect(error).toBeInstanceOf(Error); - expect((error as Error).message).toBe('builder exploded'); - } + await expect(pipeline.run({})).rejects.toThrow('builder exploded');packages/pipeline/src/core/__tests__/rollback.test.ts (1)
139-157: Unusederrorvariable - consider assertingonErrorreceived it.The test validates continuation after async failure but doesn't verify the error was properly reported. For consistency with the sync version test ('calls onError callback if a rollback fails'), consider asserting the error details.
+ const onError = jest.fn(); + await runRollbackStack(rollbacks, { source: 'helper', - onError: jest.fn(), + onError, }); // Should have executed rollback1 and rollback3 despite rollback2 failing asynchronously expect(order).toEqual([3, 1]); + expect(onError).toHaveBeenCalledWith( + expect.objectContaining({ + error, + metadata: expect.objectContaining({ message: 'async fail' }), + }) + );packages/pipeline/src/core/__tests__/ignored-lifecycle.test.ts (1)
4-6: Test name/description says “silently”, but expectation requires a warning
Either rename the test/describe (recommended) or drop the warning assertion if “silent ignore” is the intended behavior.Also applies to: 35-46
packages/pipeline/src/core/__tests__/makePipeline.coverage.test.ts (1)
60-78: Consider awaiting extension registration in async-aware tests
Ifpipeline.extensions.use(...)returns a promise, this test is currently relying onrun()to “eventually” observe the registration.awaitmakes failures surface at the right spot.packages/pipeline/src/core/__tests__/multi-lifecycle-rollback.test.ts (1)
77-80: If LIFO rollback order matters, assert call order explicitly
Right now the test only checks both were called. If “phase-two then phase-one” is required, assert it (e.g., viamock.invocationCallOrder).packages/pipeline/src/core/runner/context.ts (1)
13-13: Tightenanyon rollback error metadata where possible
Even a local alias to the real metadata type would reduce accidental shape drift between coordinator → handler → user callback.Also applies to: 96-106
packages/pipeline/src/core/runner/types.ts (1)
29-55: AvoidcreateErrorbeing configurable in two places (options vs dependencies).
AgnosticRunnerOptions.createError(Line 40) andAgnosticRunnerDependencies.createError(Line 314) can diverge. Prefer a single source of truth (or document which one wins) to prevent subtle inconsistencies.Also applies to: 291-365
packages/pipeline/src/core/__tests__/registration-diagnostics.repro.test.ts (1)
21-52: Clean up extensive inline reasoning comments.Lines 21-42 contain detailed reasoning about registration behavior that should be removed or moved to documentation. These debugging thoughts clutter the test file.
- // 1. Trigger a registration conflict (which throws, but also logs a diagnostic) const helper = { kind: 'fragment', key: 'conflict', mode: 'override', dependsOn: [], - // Wait, override removes previous. Conflict is when we have duplicate WITHOUT override? - // ... (remove lines 27-42) apply: () => {}, };packages/pipeline/src/core/__tests__/makePipeline.test.ts (3)
22-24: Remove or replace commented-out assertions.The commented-out expectations suggest uncertainty about the expected result shape. Either remove these dead comments or replace them with assertions that validate the new default state-based result structure.
expect(result).toEqual( expect.objectContaining({ - // artifact: 'artifact', // Default result is just state - // diagnostics: [], + artifact: {}, }) );
39-47: Remove uncertain comment and clarify intent.The comment
// Use createStages instead of stages (renamed?)indicates uncertainty. Since this is a refactored API, the comment should be removed or replaced with a clear explanation of the newcreateStagescontract.createStages: (deps: any) => { - // Use createStages instead of stages (renamed?) const { makeHelperStage, finalizeResult } = deps; - // Custom stack: helper -> custom -> finalize + // Compose custom stage between helper and finalize stages return [ makeHelperStage('testHelper'), customStage, finalizeResult, ]; },
76-84: Remove stale code comments referencing removed dependencies.Lines 78 and 80 contain comments about
finalizeFragmentsbeing removed from agnostic deps. These comments add noise without value now that the refactor is complete.createStages: (deps: any) => { const { makeLifecycleStage, finalizeResult } = deps; - // We don't have finalizeFragments anymore return [ - // finalizeFragments, // Removed from agnostic deps makeLifecycleStage('custom-lifecycle'), finalizeResult, ]; },packages/pipeline/src/core/runner/program.ts (1)
193-204: Consider documenting the type cast rationale.The
as unknown as HelperStageSpec<...>cast bypasses type checking. While this may be necessary due to complex generic constraints, a brief comment explaining why the cast is safe would improve maintainability.+ // Type assertion required because stageSpec is built dynamically and + // TypeScript cannot infer the full generic relationship at compile time. return makeStage( kind, stageSpec as unknown as HelperStageSpec<packages/pipeline/src/standard-pipeline/runner/index.ts (1)
367-381: Unusedstateparameter in destructuring causes confusion.The
createRunResultcallback destructures bothartifact: stateandstate: agnosticState, which is confusing. The parameterartifactreceives the value namedstate, then there's a separatestateparameter renamed toagnosticState. This naming is misleading sinceartifacthere isn't actually the artifact but the state.Consider renaming for clarity:
createRunResult: ({ - artifact: state, + artifact: userStateFromCore, context, steps, diagnostics, options: runOpts, state: agnosticState, }: {packages/pipeline/src/core/makePipeline.ts (2)
271-291: Overly defensive promise error suppression may hide issues.The
trackPendingExtensionRegistrationfunction has multiple.catch(() => {})calls that silently swallow errors. While this prevents unhandled rejection warnings, it may hide legitimate registration failures.Consider logging suppressed errors for debugging:
const trackPendingExtensionRegistration = <T>( maybePending: MaybePromise<T> ): MaybePromise<T> => { if (maybePending && isPromiseLike(maybePending)) { - void Promise.resolve(maybePending).catch(() => { }); + void Promise.resolve(maybePending).catch((e) => { + // Intentionally suppressed - error will be handled by caller + }); const pending = Promise.resolve(maybePending).then(() => undefined); - void pending.catch(() => { }); + void pending.catch(() => { /* suppressed */ }); pendingExtensionRegistrations.push(pending); pending .finally(() => { const index = pendingExtensionRegistrations.indexOf(pending); if (index !== -1) { pendingExtensionRegistrations.splice(index, 1); } }) - .catch(() => { }); + .catch(() => { /* suppressed - finally cleanup only */ }); } return maybePending; };
166-191: Extension rollback error handler duplicates logging.The
onExtensionRollbackErrorcallback first invokes the user-provided handler (if any), then always logs viareporter.warn. This means if the user's handler also logs, the error is logged twice.Consider making the default logging conditional:
onExtensionRollbackError: (opts: {...}) => { if (options.onExtensionRollbackError) { options.onExtensionRollbackError(opts); - } - // Adapt to legacy logging format expected by core tests - const logContext = {...}; - opts.context.reporter.warn?.( - 'Pipeline extension rollback failed.', - logContext - ); + } else { + // Default: log to reporter if no custom handler + const logContext = {...}; + opts.context.reporter.warn?.( + 'Pipeline extension rollback failed.', + logContext + ); + } },packages/pipeline/src/core/__tests__/kinds.test.ts (1)
36-67: Excessive use ofanytype reduces test type safety.While
anyis sometimes acceptable in tests for brevity, using it forstate,_entry, anddepsparameters (lines 36, 39, 49, 59) loses the benefit of type checking that could catch integration issues early.Consider importing and using the actual types:
-createStages: (deps: any) => { +createStages: (deps: AgnosticStageDeps<...>) => { const extractStage = deps.makeHelperStage('extract', { - makeArgs: (state: any) => (_entry: any) => ({ + makeArgs: (state) => (_entry) => ({This would provide better IDE support and catch type mismatches during development.
packages/pipeline/src/core/runner/diagnostics.ts (1)
151-171: Unsafe type assertion may mask type mismatches at runtime.The
as unknown as TDiagnosticcast bypasses TypeScript's type checking. IfTDiagnostichas required fields beyond those in the default object literal, this will fail at runtime without compile-time warning. Consider requiring factory functions when custom diagnostic types are used, or narrowingTDiagnosticconstraints.The same pattern appears in
flagMissingDependency(lines 177-191) andflagUnusedHelper(lines 200-212). If custom diagnostic types are expected, the factory methods should be mandatory rather than optional.packages/pipeline/src/core/runner/stage-factories.ts (1)
337-342: Clarify the TODO-style comment aboutregisteredpopulation.The comment on line 329 asks a question: "Registered populated by getOrder result?" This should be clarified or addressed rather than left as an open question in production code.
packages/pipeline/src/core/runner/rollback.ts (1)
14-28: Type naming inconsistency: TArtifact vs TUserState.This file uses
TArtifactwhileextension-coordinator.types.tsandstage-factories.tshave been updated to useTUserState. For consistency across the codebase, consider renamingTArtifacttoTUserStatehere as well.export type RollbackToHaltPlan< TRunResult, TContext, TOptions, - TArtifact, + TUserState, THelper extends { key: string }, > = { readonly rollbackPlan: HelperRollbackPlan< TContext, TOptions, - TArtifact, + TUserState, THelper >; readonly halt: (error?: unknown) => Halt<TRunResult>; };Apply the same rename throughout the file (lines 36-43, 130-146, 149-165, 184-198, 212-226).
packages/pipeline/src/core/types.ts (2)
245-245: Consider renaming to avoid confusion with standard-pipeline's specific definition.The type is correctly generalized to
stringfor the agnostic core. However,standard-pipeline/types.ts(line 26) defines aPipelineExtensionLifecyclewith specific lifecycle values ('prepare', 'before-fragments', etc.). This name collision could cause confusion when both types are in scope.Consider renaming the core version to something like
AgnosticExtensionLifecycleorCoreExtensionLifecycleto clarify the distinction:-export type PipelineExtensionLifecycle = string; +export type AgnosticExtensionLifecycle = string;Then update references to use the new name and let the standard-pipeline export its specific
PipelineExtensionLifecycle.
363-365: Consider alternatives tounknowntype for better type safety.The use of
unknownto avoid circular imports is acknowledged in the comment, but it forces consumers to cast types, reducing type safety. Consider these alternatives:
- Use a generic type parameter that consumers can specify
- Move type definitions to a separate file to break the circular dependency
- Use a branded type to provide some compile-time checking
If refactoring isn't feasible now, at minimum expand the comment to guide consumers on the expected type:
/** * Factory for pipeline stages. * If provided, this overrides the default agnostic stage composition. * Use this to reinstate standard pipeline behaviors or implement custom flows. + * + * @remarks + * The deps parameter should be cast to AgnosticStageDeps<TContext, TReporter, TUserState>. + * The return value should be an array of stage functions. */ readonly createStages?: ( - deps: unknown // We use unknown here to avoid circular imports. Consumers should cast to AgnosticStageDeps. + deps: unknown // Cast to AgnosticStageDeps<TContext, TReporter, TUserState> ) => unknown[];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (58)
docs/packages/pipeline/architecture.md(3 hunks)docs/packages/pipeline/framework-contributors.md(2 hunks)packages/core/src/pipeline/__tests__/createPipeline.run.test.ts(1 hunks)packages/core/src/resource/define.ts(3 hunks)packages/core/src/resource/validation.ts(1 hunks)packages/pipeline/README.md(3 hunks)packages/pipeline/package.json(1 hunks)packages/pipeline/src/core/__tests__/extensions.test.ts(1 hunks)packages/pipeline/src/core/__tests__/helper-rollback.test.ts(1 hunks)packages/pipeline/src/core/__tests__/ignored-lifecycle.test.ts(1 hunks)packages/pipeline/src/core/__tests__/kinds.test.ts(1 hunks)packages/pipeline/src/core/__tests__/makePipeline.coverage.test.ts(4 hunks)packages/pipeline/src/core/__tests__/makePipeline.test.ts(7 hunks)packages/pipeline/src/core/__tests__/multi-lifecycle-rollback.test.ts(2 hunks)packages/pipeline/src/core/__tests__/multi-lifecycle.test.ts(2 hunks)packages/pipeline/src/core/__tests__/pipeline.test.ts(1 hunks)packages/pipeline/src/core/__tests__/registration-diagnostics.repro.test.ts(1 hunks)packages/pipeline/src/core/__tests__/rollback.test.ts(1 hunks)packages/pipeline/src/core/__tests__/runner.test.ts(1 hunks)packages/pipeline/src/core/__tests__/stale-diagnostics.test.ts(1 hunks)packages/pipeline/src/core/__tests__/sync-execution.test.ts(2 hunks)packages/pipeline/src/core/async-utils.ts(1 hunks)packages/pipeline/src/core/executor.ts(1 hunks)packages/pipeline/src/core/extensions/index.ts(1 hunks)packages/pipeline/src/core/extensions/runner.ts(2 hunks)packages/pipeline/src/core/index.ts(1 hunks)packages/pipeline/src/core/internal/extension-coordinator.ts(4 hunks)packages/pipeline/src/core/internal/extension-coordinator.types.ts(2 hunks)packages/pipeline/src/core/makePipeline.ts(1 hunks)packages/pipeline/src/core/registration.ts(2 hunks)packages/pipeline/src/core/runner/context.ts(1 hunks)packages/pipeline/src/core/runner/diagnostics.ts(1 hunks)packages/pipeline/src/core/runner/execution.ts(1 hunks)packages/pipeline/src/core/runner/index.ts(1 hunks)packages/pipeline/src/core/runner/program.ts(1 hunks)packages/pipeline/src/core/runner/rollback.ts(1 hunks)packages/pipeline/src/core/runner/stage-factories.ts(11 hunks)packages/pipeline/src/core/runner/types.ts(3 hunks)packages/pipeline/src/core/types.ts(3 hunks)packages/pipeline/src/createPipeline.ts(0 hunks)packages/pipeline/src/index.ts(2 hunks)packages/pipeline/src/internal/__tests__/diagnostic-manager.test.ts(0 hunks)packages/pipeline/src/internal/__tests__/helper-execution.test.ts(0 hunks)packages/pipeline/src/internal/__tests__/pipeline-runner.test.ts(0 hunks)packages/pipeline/src/internal/diagnostic-manager.ts(0 hunks)packages/pipeline/src/internal/diagnostic-manager.types.ts(0 hunks)packages/pipeline/src/internal/helper-execution.ts(0 hunks)packages/pipeline/src/internal/runner/context.ts(0 hunks)packages/pipeline/src/internal/runner/execution.ts(0 hunks)packages/pipeline/src/internal/runner/index.ts(0 hunks)packages/pipeline/src/internal/runner/program.ts(0 hunks)packages/pipeline/src/makePipeline.ts(0 hunks)packages/pipeline/src/standard-pipeline/__tests__/createExtension.test.ts(1 hunks)packages/pipeline/src/standard-pipeline/__tests__/createPipeline.test.ts(1 hunks)packages/pipeline/src/standard-pipeline/createPipeline.ts(1 hunks)packages/pipeline/src/standard-pipeline/runner/index.ts(1 hunks)packages/pipeline/src/standard-pipeline/types.ts(1 hunks)packages/pipeline/vite.config.ts(1 hunks)
💤 Files with no reviewable changes (12)
- packages/pipeline/src/internal/tests/pipeline-runner.test.ts
- packages/pipeline/src/internal/runner/index.ts
- packages/pipeline/src/createPipeline.ts
- packages/pipeline/src/internal/tests/diagnostic-manager.test.ts
- packages/pipeline/src/internal/helper-execution.ts
- packages/pipeline/src/internal/diagnostic-manager.ts
- packages/pipeline/src/internal/runner/context.ts
- packages/pipeline/src/internal/tests/helper-execution.test.ts
- packages/pipeline/src/internal/runner/execution.ts
- packages/pipeline/src/internal/diagnostic-manager.types.ts
- packages/pipeline/src/internal/runner/program.ts
- packages/pipeline/src/makePipeline.ts
🧰 Additional context used
🧬 Code graph analysis (10)
packages/pipeline/src/core/runner/execution.ts (3)
packages/pipeline/src/core/runner/types.ts (6)
HelperExecutionSnapshot(18-18)AgnosticRunContext(63-101)AgnosticState(109-163)Halt(178-182)ExtensionCoordinator(25-25)ExtensionLifecycleState(25-25)packages/pipeline/src/core/types.ts (4)
HelperExecutionSnapshot(232-239)PipelineReporter(41-43)PipelineDiagnostic(210-213)MaybePromise(7-7)packages/pipeline/src/core/internal/extension-coordinator.types.ts (2)
ExtensionCoordinator(52-81)ExtensionLifecycleState(18-26)
packages/pipeline/src/core/__tests__/runner.test.ts (3)
packages/pipeline/src/core/types.ts (2)
PipelineReporter(41-43)PipelineDiagnostic(210-213)packages/pipeline/src/core/runner/types.ts (4)
PipelineStage(234-234)Halt(178-182)AgnosticState(109-163)AgnosticRunnerDependencies(291-365)packages/pipeline/src/core/runner/index.ts (1)
initAgnosticRunner(14-43)
packages/pipeline/src/core/__tests__/pipeline.test.ts (2)
packages/pipeline/src/core/types.ts (4)
PipelineReporter(41-43)PipelineDiagnostic(210-213)HelperApplyOptions(69-79)Helper(125-133)packages/pipeline/src/core/makePipeline.ts (1)
makePipeline(31-387)
packages/pipeline/src/core/runner/index.ts (6)
packages/pipeline/src/core/types.ts (2)
PipelineReporter(41-43)PipelineDiagnostic(210-213)packages/pipeline/src/index.ts (2)
PipelineReporter(27-27)PipelineDiagnostic(35-35)packages/pipeline/src/standard-pipeline/types.ts (1)
PipelineReporter(17-17)packages/pipeline/src/core/runner/types.ts (2)
AgnosticRunnerDependencies(291-365)AgnosticRunner(367-393)packages/pipeline/src/core/runner/context.ts (1)
prepareContext(21-166)packages/pipeline/src/core/runner/execution.ts (1)
executeRun(29-168)
packages/pipeline/src/standard-pipeline/createPipeline.ts (3)
packages/pipeline/src/core/types.ts (5)
PipelineReporter(41-43)PipelineDiagnostic(210-213)PipelineRunState(219-226)HelperKind(13-13)Helper(125-133)packages/pipeline/src/standard-pipeline/types.ts (4)
PipelineReporter(17-17)PipelineRunState(18-18)CreatePipelineOptions(60-211)Pipeline(217-299)packages/pipeline/src/standard-pipeline/runner/index.ts (1)
createStandardPipeline(59-634)
packages/core/src/resource/define.ts (1)
packages/core/src/resource/validation.ts (1)
validateConfig(40-51)
packages/pipeline/src/core/runner/rollback.ts (2)
packages/pipeline/src/core/runner/types.ts (3)
HelperRollbackPlan(451-466)RollbackEntry(173-176)RollbackContext(184-204)packages/pipeline/src/core/types.ts (2)
MaybePromise(7-7)PipelineExtensionRollbackErrorMetadata(268-273)
packages/pipeline/src/core/internal/extension-coordinator.ts (1)
packages/pipeline/src/core/internal/extension-coordinator.types.ts (3)
ExtensionRollbackEvent(42-50)ExtensionCoordinator(52-81)ExtensionLifecycleState(18-26)
packages/pipeline/src/standard-pipeline/types.ts (1)
packages/pipeline/src/core/types.ts (12)
PipelineExtensionLifecycle(245-245)HelperKind(13-13)PipelineReporter(41-43)PipelineDiagnostic(210-213)PipelineRunState(219-226)Helper(125-133)HelperApplyOptions(69-79)PipelineStep(159-163)PipelineExtensionHookOptions(247-252)PipelineExtensionRollbackErrorMetadata(268-273)PipelineExtension(301-308)MaybePromise(7-7)
packages/pipeline/src/core/types.ts (1)
packages/pipeline/src/standard-pipeline/types.ts (1)
PipelineExtensionLifecycle(26-33)
🪛 markdownlint-cli2 (0.18.1)
packages/pipeline/README.md
51-51: Hard tabs
Column: 1
(MD010, no-hard-tabs)
52-52: Hard tabs
Column: 1
(MD010, no-hard-tabs)
53-53: Hard tabs
Column: 1
(MD010, no-hard-tabs)
54-54: Hard tabs
Column: 1
(MD010, no-hard-tabs)
56-56: Hard tabs
Column: 1
(MD010, no-hard-tabs)
57-57: Hard tabs
Column: 1
(MD010, no-hard-tabs)
58-58: Hard tabs
Column: 1
(MD010, no-hard-tabs)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (68)
packages/core/src/resource/define.ts (1)
26-27: Good centralization of config validation viavalidateConfig.packages/core/src/resource/validation.ts (1)
79-95: Regex update correctly enablesnamespace:resourcewhile keeping kebab-case constraints.Optional: consider making the message a bit more precise since the regex also allows digits (e.g., “lowercase letters/numbers with hyphens…”).
Also applies to: 87-91
packages/core/src/pipeline/__tests__/createPipeline.run.test.ts (2)
2-2: LGTM: Removal aligns with centralized diagnostic architecture.The removal of the
reportPipelineDiagnosticimport is consistent with the broader architectural refactor that introduces a centralized, agnostic diagnostic system in the core runner. Tests at lines 359-471 confirm that diagnostic reporting continues to function correctly through the reporter.
359-471: Diagnostic persistence correctly aligns with the new static/run diagnostics separation.The tests validate that diagnostics are reported to the current reporter and properly replayed when the pipeline is reused with different reporters. The second test (lines 401-471) confirms that static diagnostics persist across runs—once reported to the first reporter, they are not re-reported on the second run, but new reporters receive the complete diagnostic history. This behavior correctly implements the architecture where
staticDiagnosticsare preserved across runs whilerunDiagnosticsare cleared per run via theprepareRun()lifecycle method.packages/pipeline/src/core/executor.ts (1)
1-1: LGTM!Clean barrel re-export that consolidates execution utilities under the core public API surface.
packages/pipeline/src/core/async-utils.ts (1)
38-40: LGTM!Defensive runtime validation for
onFulfilledis a reasonable addition for an internal utility that may be called from JavaScript or through type casts. The check is correctly placed before any other logic.packages/pipeline/src/standard-pipeline/__tests__/createPipeline.test.ts (1)
1-12: LGTM!Import reorganization correctly reflects the new module boundaries:
- Core utilities (
createHelper) and shared types moved to../../core/- Standard-pipeline specific types (
Pipeline,PipelineExtensionRollbackErrorMetadata) imported from../types.jsThis aligns well with the architecture split introduced in this PR.
packages/pipeline/package.json (1)
21-25: LGTM!New
./coresubpath export correctly follows the existing pattern and aligns with the architecture split. This enables consumers to import agnostic core primitives without the standard-pipeline layer. The build process will generate the expected output atdist/core/index.js.packages/pipeline/src/standard-pipeline/__tests__/createExtension.test.ts (1)
1-10: Import/path migration looks consistent with the new core/standard split
No concerns with the updated module boundaries; tests remain behaviorally identical.packages/pipeline/src/core/runner/execution.ts (1)
105-110:Haltpath bypasses extension commit—verify intended semantics
If__haltcan occur after extension lifecycles have produced pending state, skipping commit/rollback may leak side effects. If__haltis guaranteed to occur before any lifecycle effects, consider documenting that invariant in-code.packages/pipeline/src/core/__tests__/multi-lifecycle-rollback.test.ts (1)
7-10: No action needed—this test uses the correct option namecreateInitialState.The type definition in
AgnosticPipelineOptionsonly supportscreateInitialState. Other test files (makePipeline.test.ts, makePipeline.coverage.test.ts) incorrectly usecreateState, which is not a valid option. If compilation succeeds despite this, it may indicate loose typing elsewhere, but the multi-lifecycle-rollback.test.ts is not part of that problem.Likely an incorrect or invalid review comment.
packages/pipeline/src/core/runner/types.ts (1)
234-365:PipelineStage<TState, TResult> = Program<TState | TResult>: confirm this “halt union” is consistently handled by all stages.This pattern is fine if every stage either short-circuits on
haltor the runner guarantees no stage receives aTResultunexpectedly. Worth double-checking downstream stage implementations for consistentisHalthandling.packages/pipeline/vite.config.ts (1)
3-7: Alias updates look consistent with the new core/extension barrels.Adding
core/indexand repointingextensions/indexmatches the new module layout.packages/pipeline/src/core/__tests__/extensions.test.ts (1)
1-8: Import relocation aligns with the new rollback module split.packages/pipeline/src/core/__tests__/multi-lifecycle.test.ts (2)
7-10: LGTM! Clean API surface migration.The shift from lifecycle hook factories to
createInitialStatealigns with the agnostic core architecture introduced in this PR.
22-31: LGTM! Well-structured multi-lifecycle test configuration.The empty
helperKindscorrectly signals this test focuses solely on extension hook execution across multiple lifecycles. The stage composition (makeLifecycleStage→finalizeResult) properly exercises the new agnostic runner architecture.packages/pipeline/src/core/__tests__/stale-diagnostics.test.ts (1)
1-24: LGTM! Proper test scaffolding.The pipeline configuration correctly wires
onDiagnosticto capture diagnostics and includescreateRunResultto surface them in the result.packages/pipeline/src/core/__tests__/pipeline.test.ts (2)
16-33: LGTM! Clear demonstration of agnostic helper kinds.The ETL (extract/transform/load) example effectively showcases the agnostic core's ability to support arbitrary helper kinds beyond the standard fragment/builder pattern.
36-93: LGTM! Comprehensive test validates execution order and state mutations.The test correctly verifies:
- Helpers execute in the order defined by
helperKinds- Context mutations accumulate as expected
- The agnostic pipeline properly orchestrates arbitrary kinds
The
as unknown as Helper<...>type assertions (lines 46-52, 64-70, 82-88) are necessary for test flexibility with the generic helper interface.packages/pipeline/src/core/registration.ts (2)
64-69: LGTM! Clear documentation of diagnostic-then-throw pattern.The explanatory comment (lines 64-66) helps developers understand why both
onConflict(diagnostic recording) andthrow(fatal error) are invoked. This aligns with the PR's improved diagnostic observability.
81-93: LGTM! Correct fix for index stability after splicing.The monotonic index calculation (lines 83-87) ensures stable, creation-order indices even when override mode removes previous entries. This prevents index collisions that would occur if using
entries.lengthafter splicing.docs/packages/pipeline/framework-contributors.md (2)
1-6: LGTM! Clear scope definition for the guide.The updated title and note (line 5) effectively communicate that this guide targets Standard Pipeline users, with a helpful reference to the Architecture Guide for Core Runner scenarios.
81-85: LGTM! Documentation accurately reflects architectural layering.The updated descriptions (lines 81-85) clearly communicate that Standard Pipeline helper registration functions wrap the agnostic core
registerHelper, aligning with the PR's architecture split.packages/pipeline/src/core/__tests__/registration-diagnostics.repro.test.ts (1)
1-19: LGTM! Proper test setup for diagnostic propagation.The configuration correctly wires
onDiagnosticto capture diagnostics emitted during registration and execution.packages/pipeline/src/core/__tests__/runner.test.ts (2)
11-73: LGTM! Well-structured agnostic runner test setup.The test demonstrates the agnostic core's flexibility by using a custom
adderhelper kind. The manual stage implementation (lines 57-72) clearly shows how helpers are orchestrated throughhelperOrders.
74-113: LGTM! Assertions correctly validate execution order and state.The test verifies:
- State mutation:
countreaches 13 (10 + 1 + 2)- Priority-based ordering:
add2(priority 20) executes beforeadd1(priority 10)The inline comments (lines 109-112) appropriately acknowledge the dependency graph's role in priority sorting.
packages/pipeline/src/core/index.ts (1)
1-10: LGTM! Clean barrel export consolidating the core API.The re-export pattern properly consolidates the agnostic core's public surface. The
.jsextensions correctly support ESM module resolution.packages/pipeline/src/core/extensions/runner.ts (3)
1-14: LGTM!Import paths correctly updated to reflect the module restructuring. The
runRollbackStackimport from../rollbackaligns with the PR's goal of centralizing rollback logic.
59-59: Good documentation of code relocation.The comment clearly indicates where
createRollbackErrorMetadatawas moved, which aids future maintainability.
186-217: LGTM!The
rollbackExtensionResultsfunction cleanly adapts extension hook results into the unified rollback interface and delegates to the centralizedrunRollbackStack. The LIFO execution and error reporting behavior align with the PR's "Robust Best-Effort" rollback strategy.packages/pipeline/src/core/runner/index.ts (1)
14-43: LGTM!The
initAgnosticRunnerfactory cleanly wires dependencies toprepareContextandexecuteRun, providing a clear separation between context preparation and execution phases. The type signature correctly mirrors theAgnosticRunnerinterface.packages/pipeline/src/core/runner/program.ts (4)
64-67: LGTM!The
halthelper correctly constructs the discriminated union type with__halt: truefor early termination signaling.
162-174: Good defensive handling of helper invocation.The dual-path invocation logic correctly handles both function-style helpers and object-style helpers with an
applymethod. The error message provides useful debugging context when an invalid helper type is encountered.
280-299: LGTM on ignored lifecycle validation.This implements the PR's stated goal of warning when extensions register hooks for lifecycles not present in the execution plan. The warning is appropriately surfaced via the reporter rather than throwing.
333-340: LGTM!Good defensive check requiring the
stagesfactory. The stage reversal beforecomposeKis correct for right-to-left Kleisli composition, ensuring stages execute in the declared order.packages/pipeline/src/core/__tests__/sync-execution.test.ts (2)
12-29: LGTM!The test correctly exercises the synchronous execution path with the updated pipeline options. The assertions at lines 46-47 validate both the artifact state mutation and the empty diagnostics array.
50-68: LGTM!Good coverage of the async execution path. The test verifies that introducing an async helper causes
run()to return a promise, which is the expected behavior for the synchronous-when-possible execution model.packages/pipeline/src/standard-pipeline/runner/index.ts (3)
232-258: Well-structuredonVisitedcallback for flagging unused helpers.The implementation correctly iterates through registered helpers and flags those not in the visited set. The pattern is clean and reusable across fragment and builder kinds.
283-330: Fragment finalization stage correctly snapshots and applies artifact.The
finalizeFragmentStageproperly captures helper execution metadata and transforms the draft into the final artifact viafinalizeFragmentState. The immutable state update pattern (spreading state) is appropriate.
522-527: The'prepare'lifecycle is referenced but not defined in the stage list.The
usesDraftarray includes'prepare'lifecycle, butcreateStages(lines 332-364) defines'init'as the first lifecycle stage, not'prepare'. This mismatch means extensions registered for'prepare'won't be handled correctly.Verify if the lifecycle should be
'init'instead of'prepare':const usesDraft = [ - 'prepare', + 'init', 'before-fragments', 'after-fragments', ].includes(lifecycle);Likely an incorrect or invalid review comment.
packages/pipeline/src/standard-pipeline/createPipeline.ts (1)
1-89: Clean facade pattern delegating to the runner implementation.This file provides a public API entry point (
createPipeline) that cleanly delegates tocreateStandardPipeline. The type parameters are correctly mirrored, maintaining type safety through the delegation.packages/pipeline/src/core/makePipeline.ts (2)
356-383: Proper diagnostic lifecycle management withendRun()in all paths.The
runmethod correctly callsdiagnosticManager.endRun()in both success and error paths for async runs (lines 371, 375), and for sync runs (line 381). This ensures run diagnostics are properly cleared as mentioned in the PR objectives.
309-320: Extension registration correctly tracks async registrations.The
extensions.usemethod properly handles both sync and async registration results, tracking pending promises and usingmaybeThenfor conditional async handling.packages/pipeline/src/core/internal/extension-coordinator.ts (2)
6-6: Import relocation aligns with module reorganization.Moving
createRollbackErrorMetadataimport from../extensionsto../rollbackimproves module cohesion by keeping rollback-related utilities together.
32-114: Consistent type parameter rename fromTArtifacttoTUserState.The generic rename is applied consistently across the function signature, all method types (
runLifecycle,createRollbackHandler,commit), and the finalsatisfiesclause. This aligns with the broader PR refactoring toward user-state-centric abstractions.packages/pipeline/src/core/__tests__/kinds.test.ts (1)
130-145: Test correctly validates ETL pipeline execution order and data transformation.The test verifies that:
- Helpers execute in the correct stage order (extract → transform → load)
- Data transforms correctly (
['init'] → ['INIT', 'A'])- The custom
createStageswiring works withuserState.draftpackages/pipeline/src/core/runner/diagnostics.ts (4)
1-5: LGTM!Clean import of the required types from the parent module.
10-36: LGTM!Well-designed options interface with proper readonly modifiers and optional factory methods for custom diagnostic creation. The generic constraints properly extend the base types.
42-71: LGTM!The interface clearly separates core operations (
record,setReporter,readDiagnostics) from helper utilities and lifecycle methods. TheprepareRun/endRunlifecycle methods address the PR objective of separating static vs run diagnostics.
83-93: WeakMap for reporter-scoped deduplication is a good pattern.Using
WeakMapprevents memory leaks when reporters are garbage collected. The per-reporterSet<TDiagnostic>correctly ensures diagnostics are only logged once per reporter instance.packages/pipeline/src/core/internal/extension-coordinator.types.ts (3)
18-26: LGTM!The
TArtifacttoTUserStaterename is consistently applied to theExtensionLifecycleStateinterface, improving semantic clarity about what the type parameter represents.
42-50: LGTM!Consistent type parameter rename in
ExtensionRollbackEvent.
52-81: LGTM!The
ExtensionCoordinatorinterface consistently usesTUserStatethroughout all method signatures. The coordinator's API surface remains unchanged apart from the naming improvement.packages/pipeline/src/index.ts (3)
2-9: LGTM!Clear separation of exports with
createPipelinefrom standard-pipeline (user-facing factory) andmakePipelinefrom core (advanced/custom use), aligning with the PR's architecture split objective.
71-76: Good addition of advanced pipeline types for custom architectures.Exposing
PipelineStage,Halt, andAgnosticRunContext(aliased asPipelineRunContext) enables advanced users to build custom pipeline implementations while maintaining type safety.
57-62: The exports forPipelineandCreatePipelineOptionsare correctly defined and exported fromstandard-pipeline/types. No backward compatibility concerns exist—these types were not previously exported fromcore/typesand no codebase references indicate a breaking change. The current module structure is appropriate and intact.packages/pipeline/src/core/runner/stage-factories.ts (3)
1-27: LGTM!Imports are well-organized and aligned with the new module structure.
28-42: Internal types are appropriately scoped.
StateWithExecutionandHelperWithKeyare correctly kept as internal (non-exported) types, encapsulating implementation details of execution tracking.
367-391: LGTM!The rollback plan construction correctly uses
TUserStateand integrates withrunHelperStageWithRollback. The helper stage execution flow properly handles both synchronous and asynchronous paths.packages/pipeline/src/core/runner/rollback.ts (5)
1-12: LGTM!Imports are clean and properly reference both core utilities and type definitions.
66-87: LIFO execution correctly implements "Robust Best-Effort" rollback semantics.The
runRemainingrecursive function correctly:
- Processes handlers in reverse order (LIFO)
- Uses
maybeTryto continue to the next handler regardless of success/failure- Aligns with the PR's stated rollback strategy
89-128: Verify that swallowing rollback failures is intentional.Line 125-127 swallows all rollback failures silently. While this aligns with "best-effort" semantics, it means the original error might be the only thing surfaced to callers. Ensure this is documented behavior, as debugging rollback failures could be challenging.
The
onHelperRollbackErrorcallback (lines 98-118) does provide a hook for observing individual rollback errors, which is good for observability. Confirm that consumers are expected to use this callback for logging/monitoring rollback failures.
149-182: LGTM!
runHelperStageWithRollbackcleanly orchestrates program execution with rollback-on-error semantics. The use ofmaybeTryproperly handles both sync and async code paths.
205-237: LGTM!
makeRollbackHandleris a well-designed factory that creates a reusable rollback handler bound to the given context and helper rollbacks. Good encapsulation of rollback setup.packages/pipeline/src/core/types.ts (2)
9-13: LGTM - Type generalization aligns with agnostic architecture.The change from a constrained union to a generic
stringis appropriate for the agnostic core layer. The standard pipeline layer (instandard-pipeline/types.ts) will provide specific fragment/builder semantics on top of this foundation.
416-450: LGTM - Well-designed agnostic pipeline interface.The interface correctly abstracts away artifact-specific details while providing:
- Generic extension registration with appropriate use of
unknownfor artifact type- Transparent access to
providedKeysfor debugging/introspection- Flexible helper registration accepting any helper kind
- Clean run interface
The design appropriately balances type safety with the flexibility needed for an agnostic core.
packages/pipeline/src/standard-pipeline/types.ts (2)
1-20: LGTM - Clean public API surface.The imports and re-exports appropriately expose core types that consumers will need while maintaining a clean module boundary between core and standard-pipeline layers.
22-54: LGTM - Well-structured lifecycle and metadata types.The lifecycle definition appropriately constrains to known values while allowing extension via
(string & {}). The metadata interfaces cleanly capture fragment and builder execution information with proper type parameters.
| export interface CreatePipelineOptions< | ||
| TRunOptions, | ||
| TBuildOptions, | ||
| TContext extends { reporter: TReporter }, | ||
| TReporter extends PipelineReporter = PipelineReporter, | ||
| TDraft = unknown, | ||
| TArtifact = unknown, | ||
| TDiagnostic extends PipelineDiagnostic = PipelineDiagnostic, | ||
| TRunResult = PipelineRunState<TArtifact, TDiagnostic>, | ||
| TFragmentInput = unknown, | ||
| TFragmentOutput = unknown, | ||
| TBuilderInput = unknown, | ||
| TBuilderOutput = unknown, | ||
| TFragmentKind extends HelperKind = 'fragment', | ||
| TBuilderKind extends HelperKind = 'builder', | ||
| TFragmentHelper extends Helper< | ||
| TContext, | ||
| TFragmentInput, | ||
| TFragmentOutput, | ||
| TReporter, | ||
| TFragmentKind | ||
| > = Helper< | ||
| TContext, | ||
| TFragmentInput, | ||
| TFragmentOutput, | ||
| TReporter, | ||
| TFragmentKind | ||
| >, | ||
| TBuilderHelper extends Helper< | ||
| TContext, | ||
| TBuilderInput, | ||
| TBuilderOutput, | ||
| TReporter, | ||
| TBuilderKind | ||
| > = Helper< | ||
| TContext, | ||
| TBuilderInput, | ||
| TBuilderOutput, | ||
| TReporter, | ||
| TBuilderKind | ||
| >, | ||
| > { | ||
| readonly fragmentKind?: TFragmentKind; | ||
| readonly builderKind?: TBuilderKind; | ||
| readonly createError?: (code: string, message: string) => Error; | ||
| readonly createBuildOptions: (options: TRunOptions) => TBuildOptions; | ||
| readonly createContext: (options: TRunOptions) => TContext; | ||
| readonly createFragmentState: (options: { | ||
| readonly options: TRunOptions; | ||
| readonly context: TContext; | ||
| readonly buildOptions: TBuildOptions; | ||
| }) => TDraft; | ||
| readonly createFragmentArgs: (options: { | ||
| readonly helper: TFragmentHelper; | ||
| readonly options: TRunOptions; | ||
| readonly context: TContext; | ||
| readonly buildOptions: TBuildOptions; | ||
| readonly draft: TDraft; | ||
| }) => HelperApplyOptions< | ||
| TContext, | ||
| TFragmentInput, | ||
| TFragmentOutput, | ||
| TReporter | ||
| >; | ||
| readonly finalizeFragmentState: (options: { | ||
| readonly draft: TDraft; | ||
| readonly options: TRunOptions; | ||
| readonly context: TContext; | ||
| readonly buildOptions: TBuildOptions; | ||
| readonly helpers: FragmentFinalizationMetadata<TFragmentKind>; | ||
| }) => TArtifact; | ||
| readonly createBuilderArgs: (options: { | ||
| readonly helper: TBuilderHelper; | ||
| readonly options: TRunOptions; | ||
| readonly context: TContext; | ||
| readonly buildOptions: TBuildOptions; | ||
| readonly artifact: TArtifact; | ||
| }) => HelperApplyOptions< | ||
| TContext, | ||
| TBuilderInput, | ||
| TBuilderOutput, | ||
| TReporter | ||
| >; | ||
| readonly createRunResult?: (options: { | ||
| readonly artifact: TArtifact; | ||
| readonly diagnostics: readonly TDiagnostic[]; | ||
| readonly steps: readonly PipelineStep[]; | ||
| readonly context: TContext; | ||
| readonly buildOptions: TBuildOptions; | ||
| readonly options: TRunOptions; | ||
| readonly helpers: PipelineExecutionMetadata< | ||
| TFragmentKind, | ||
| TBuilderKind | ||
| >; | ||
| }) => TRunResult; | ||
| /** | ||
| * Optional hook invoked whenever a diagnostic is emitted during a run. | ||
| * | ||
| * Consumers can stream diagnostics to logs or UI shells while the | ||
| * pipeline executes instead of waiting for the final run result. | ||
| */ | ||
| readonly onDiagnostic?: (options: { | ||
| readonly reporter: TReporter; | ||
| readonly diagnostic: TDiagnostic; | ||
| }) => void; | ||
| readonly createExtensionHookOptions?: (options: { | ||
| readonly context: TContext; | ||
| readonly options: TRunOptions; | ||
| readonly buildOptions: TBuildOptions; | ||
| readonly artifact: TArtifact; | ||
| readonly lifecycle: PipelineExtensionLifecycle; | ||
| }) => PipelineExtensionHookOptions<TContext, TRunOptions, TArtifact>; | ||
| readonly onExtensionRollbackError?: (options: { | ||
| readonly error: unknown; | ||
| readonly extensionKeys: readonly string[]; | ||
| readonly hookSequence: readonly string[]; | ||
| readonly errorMetadata: PipelineExtensionRollbackErrorMetadata; | ||
| readonly context: TContext; | ||
| }) => void; | ||
| readonly onHelperRollbackError?: (options: { | ||
| readonly error: unknown; | ||
| readonly helper: TFragmentHelper | TBuilderHelper; | ||
| readonly errorMetadata: PipelineExtensionRollbackErrorMetadata; | ||
| readonly context: TContext; | ||
| }) => void; | ||
| /** | ||
| * Helper keys that should be treated as "already satisfied" for fragment | ||
| * dependency resolution (useful when a run intentionally omits certain | ||
| * fragments). | ||
| */ | ||
| readonly fragmentProvidedKeys?: readonly string[]; | ||
| /** | ||
| * Helper keys that should be treated as “already satisfied” for builder | ||
| * dependency resolution (e.g. builders depending on IR helpers that are | ||
| * executed in a different pipeline stage). | ||
| */ | ||
| readonly builderProvidedKeys?: readonly string[]; | ||
| readonly createConflictDiagnostic?: (options: { | ||
| readonly helper: TFragmentHelper | TBuilderHelper; | ||
| readonly existing: TFragmentHelper | TBuilderHelper; | ||
| readonly message: string; | ||
| }) => TDiagnostic; | ||
| readonly createMissingDependencyDiagnostic?: (options: { | ||
| readonly helper: TFragmentHelper | TBuilderHelper; | ||
| readonly dependency: string; | ||
| readonly message: string; | ||
| }) => TDiagnostic; | ||
| readonly createUnusedHelperDiagnostic?: (options: { | ||
| readonly helper: TFragmentHelper | TBuilderHelper; | ||
| readonly message: string; | ||
| }) => TDiagnostic; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consider reducing type parameter complexity for better DX.
This interface has 12 type parameters, which creates significant complexity:
- Hard to read type errors
- Difficult to understand relationships between parameters
- Challenging to use without extensive documentation
- Type inference may struggle with this many parameters
Suggestions to improve:
- Group related types into objects:
interface FragmentConfig<TContext, TInput, TOutput, TReporter, TKind> {
kind: TKind;
input: TInput;
output: TOutput;
helper: Helper<TContext, TInput, TOutput, TReporter, TKind>;
}
interface BuilderConfig<...> { /* similar */ }
interface CreatePipelineOptions<
TRunOptions,
TFragment extends FragmentConfig<...>,
TBuilder extends BuilderConfig<...>,
...
> { ... }- Use builder pattern for construction:
const pipeline = createPipelineBuilder()
.withFragments<TFragmentKind>()
.withBuilders<TBuilderKind>()
.build(options);- Provide preset configurations for common cases to reduce boilerplate.
While this can be deferred if there's time pressure, the current complexity will significantly impact maintainability and developer adoption.
There was a problem hiding this comment.
Actionable comments posted: 14
♻️ Duplicate comments (2)
packages/pipeline/src/core/runner/execution.ts (1)
74-86: Inconsistent shape between no-stages and with-stages paths.When
stagesis undefined, this returnsdiagnostics: []and onlybuildersin helpers. However, the normal path (lines 151-162) returnsresult.diagnosticsand bothfragmentsandbuilders. This inconsistency could cause runtime issues for consumers expecting a consistent shape.Consider aligning the shapes:
if (!dependencies.stages) { return dependencies.resolveRunResult({ - diagnostics: [], + diagnostics: initialState.diagnostics as TDiagnostic[], steps: [], context, userState: initialState.userState, options: runOptions, helpers: { - builders: createEmptySnapshot('dummy-builder'), + fragments: createEmptySnapshot('fragment'), + builders: createEmptySnapshot('builder'), }, state: initialState, }); }packages/pipeline/src/core/runner/types.ts (1)
1-26: Good: type-only re-export forHelperExecutionSnapshotis now correct.
🧹 Nitpick comments (12)
packages/pipeline/src/core/__tests__/coverage.test.ts (3)
7-58: Add an assertion for the “multiple next() calls” behavior (currently only smoke-tests).
Right now the test will pass even ifexecuteHelpersadvances twice; it only checks “doesn’t throw”. Add a second step + assertions (or a counter) to prove only one advance occurs.it('safeguards against multiple next() calls', async () => { + const hit = jest.fn(); const step = { id: 'step1', helper: { apply: (_: any, next: any) => { next(); next(); // Second call should be ignored/return same result }, }, } as any; - const steps = [step]; + const step2 = { id: 'step2', helper: { apply: () => hit() } } as any; + const steps = [step, step2]; await executeHelpers( steps, () => ({}) as any, (fn: any, args: any, next: any) => fn.apply(args, next) as void, () => {} ); + + expect(hit).toHaveBeenCalledTimes(1); });
115-152: Trim speculative comments inside the test (they read like debugging notes).
The inline commentary aboutcreateError/ dependency-graph behavior is likely to go stale and distracts from the test intent.
219-265: Consider future-proofingexecuteRuntests if it ever becomes async.
IfexecuteRunreturns a Promise in the future, these tests will start failing in confusing ways—wrapping inawait(and making the testsasync) would make intent explicit and resilient.packages/pipeline/src/core/runner/execution.ts (1)
123-148: Consider simplifying the commit chain logic.The reduce with
undefined as MaybePromise<void>initial value works but is slightly awkward. A cleaner approach might be:- const commitPromise = fallbackCommit - ? fallbackCommit() - : extensionStack.reduce( - ( - previous: MaybePromise<void>, - // ... - ) => - maybeThen(previous, () => - coordinator.commit(loopState) - ), - undefined as MaybePromise<void> - ); + const commitAll = (): MaybePromise<void> => { + if (fallbackCommit) return fallbackCommit(); + let chain: MaybePromise<void>; + for (const { coordinator, state: loopState } of extensionStack) { + chain = chain === undefined + ? coordinator.commit(loopState) + : maybeThen(chain, () => coordinator.commit(loopState)); + } + return chain; + }; + const commitPromise = commitAll();packages/pipeline/src/core/__tests__/multi-lifecycle.test.ts (1)
23-24: Avoidanytype for deps parameter.Using
anyloses type safety. Consider using the proper type or at minimum a more specific inline type:- createStages: (deps: any) => { - const { makeLifecycleStage, finalizeResult } = deps; + createStages: (deps: { makeLifecycleStage: (name: string) => any; finalizeResult: any }) => { + const { makeLifecycleStage, finalizeResult } = deps;Or import and use
AgnosticStageDepsif available.packages/pipeline/src/core/__tests__/runner.test.ts (2)
109-113: Remove or resolve the trailing uncertainty comments.These comments suggest uncertainty about whether priority-based sorting is actually being tested. If the dependency graph is expected to sort by priority, the test should either:
- Verify the sorting is happening (and remove the uncertain comments), or
- Document clearly that this test does NOT cover priority sorting
The dangling comments reduce test clarity and suggest incomplete verification.
expect(result.count).toBe(13); // 10 + 1 + 2 expect(result.log).toEqual(['ran add2', 'ran add1']); // Priority sort: add2(20) > add1(10) - // Wait, default dependency graph sort is topological + priority. - // Assuming no dependsOn, generic sort needs to handle priority. - // Standard createDependencyGraph handles priority. - // Let's see if priority works. });
30-54: Consider reducingas anycasts for better type safety.The helper registries and test setup use multiple
as anyassertions. While acceptable in tests, this could mask type mismatches between the test harness and actual API contracts. Consider defining a proper test helper type that satisfiesRegisteredHelper<unknown>to catch type drift early.packages/pipeline/src/core/runner/program.ts (1)
230-233: Unsafe double cast bypasses lifecycle validation.The cast
lifecycle as unknown as PipelineExtensionLifecycleallows any string to be passed without compile-time or runtime validation. Iflifecycledoesn't match a configured lifecycle independencies.extensionLifecycles, this could lead to silent failures or undefined behavior.Consider adding runtime validation:
const makeLifecycleStage = (lifecycle: string): RunnerProgram => makeAfterFragmentsStage<RunnerState, Halt<TRunResult>>({ isHalt, execute: (state) => { + if ( + dependencies.extensionLifecycles && + !dependencies.extensionLifecycles.includes(lifecycle as PipelineExtensionLifecycle) + ) { + state.reporter.warn?.( + `Lifecycle "${lifecycle}" is not configured in extension lifecycles` + ); + } state.executedLifecycles.add(lifecycle);packages/pipeline/src/core/runner/context.ts (1)
55-90: Consider tighteninganyusage forentries/stepsto avoid hiding real mismatches.
Not blocking, butentries as anyandconst steps: any[]make it easy to regress type contracts between runner, stages, and result shaping.Also applies to: 97-110
packages/pipeline/src/standard-pipeline/runner/index.ts (1)
504-518: Proxyregistershould accept (and ignore) the pipeline argument for compatibility.
Some callers/types expectregister(pipeline); a zero-arg function is usually OK in JS, but typing/usage is safer if it preserves the signature.packages/pipeline/src/core/types.ts (1)
363-365: Document the expected type for thedepsparameter.The comment mentions using
unknownto avoid circular imports and suggests consumers cast toAgnosticStageDeps, but this type doesn't appear to be exported or documented in the provided code.Consider adding a JSDoc
@paramtag with the expected shape or a reference to where consumers can find the type definition:/** * Factory for pipeline stages. * If provided, this overrides the default agnostic stage composition. * Use this to reinstate standard pipeline behaviors or implement custom flows. + * @param deps - Cast to AgnosticStageDeps from core/runner module to access runner internals. */ readonly createStages?: ( - deps: unknown // We use unknown here to avoid circular imports. Consumers should cast to AgnosticStageDeps. + deps: unknown ) => unknown[];packages/pipeline/src/standard-pipeline/types.ts (1)
60-211: Type parameter complexity remains high.This interface has 16 type parameters, which exceeds the 12-14 mentioned in previous reviews. While the structure is functional, the high parameter count continues to impact:
- Type error readability
- Documentation requirements
- Usage complexity without extensive type inference
The past review suggestions (type parameter grouping, builder patterns, or preset configurations) remain valid for improving maintainability, though these can be deferred as recommended refactors.
One lightweight improvement: consider extracting common parameter groups into type aliases to reduce repetition:
type HelperConfig<TContext, TInput, TOutput, TReporter, TKind> = { helper: Helper<TContext, TInput, TOutput, TReporter, TKind>; context: TContext; }; type FragmentConfig<...> = { kind: TFragmentKind; ... };This would make the interface signature shorter while maintaining full type safety.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
docs/packages/pipeline/architecture.md(3 hunks)packages/cli/src/dx/readiness/helpers/releasePack.ts(1 hunks)packages/core/src/resource/define.ts(3 hunks)packages/pipeline/README.md(3 hunks)packages/pipeline/jest.config.js(1 hunks)packages/pipeline/package.json(1 hunks)packages/pipeline/src/core/__tests__/coverage.test.ts(1 hunks)packages/pipeline/src/core/__tests__/helper-rollback.test.ts(16 hunks)packages/pipeline/src/core/__tests__/ignored-lifecycle.test.ts(1 hunks)packages/pipeline/src/core/__tests__/kinds.test.ts(1 hunks)packages/pipeline/src/core/__tests__/makePipeline.test.ts(5 hunks)packages/pipeline/src/core/__tests__/multi-lifecycle-rollback.test.ts(3 hunks)packages/pipeline/src/core/__tests__/multi-lifecycle.test.ts(2 hunks)packages/pipeline/src/core/__tests__/pipeline.test.ts(1 hunks)packages/pipeline/src/core/__tests__/registration-diagnostics.repro.test.ts(1 hunks)packages/pipeline/src/core/__tests__/rollback.test.ts(1 hunks)packages/pipeline/src/core/__tests__/runner.test.ts(1 hunks)packages/pipeline/src/core/__tests__/stale-diagnostics.test.ts(1 hunks)packages/pipeline/src/core/__tests__/sync-execution.test.ts(2 hunks)packages/pipeline/src/core/async-utils.ts(2 hunks)packages/pipeline/src/core/makePipeline.ts(1 hunks)packages/pipeline/src/core/runner/context.ts(1 hunks)packages/pipeline/src/core/runner/diagnostics.ts(1 hunks)packages/pipeline/src/core/runner/execution.ts(1 hunks)packages/pipeline/src/core/runner/program.ts(1 hunks)packages/pipeline/src/core/runner/stage-factories.ts(11 hunks)packages/pipeline/src/core/runner/types.ts(3 hunks)packages/pipeline/src/core/types.ts(3 hunks)packages/pipeline/src/standard-pipeline/__tests__/createExtension.test.ts(4 hunks)packages/pipeline/src/standard-pipeline/runner/index.ts(1 hunks)packages/pipeline/src/standard-pipeline/types.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/pipeline/jest.config.js
🚧 Files skipped from review as they are similar to previous changes (6)
- docs/packages/pipeline/architecture.md
- packages/pipeline/src/core/tests/registration-diagnostics.repro.test.ts
- packages/pipeline/src/core/tests/pipeline.test.ts
- packages/pipeline/src/core/tests/ignored-lifecycle.test.ts
- packages/core/src/resource/define.ts
- packages/pipeline/src/core/runner/diagnostics.ts
🧰 Additional context used
🧬 Code graph analysis (6)
packages/pipeline/src/core/async-utils.ts (2)
packages/pipeline/src/core/types.ts (1)
MaybePromise(7-7)packages/pipeline/src/index.ts (2)
MaybePromise(50-50)isPromiseLike(80-80)
packages/pipeline/src/core/__tests__/stale-diagnostics.test.ts (1)
packages/pipeline/src/core/makePipeline.ts (1)
makePipeline(31-395)
packages/pipeline/src/core/runner/context.ts (3)
packages/pipeline/src/core/types.ts (3)
PipelineReporter(41-43)PipelineDiagnostic(210-213)PipelineExtensionRollbackErrorMetadata(268-273)packages/pipeline/src/core/dependency-graph.ts (1)
createDependencyGraph(358-381)packages/pipeline/src/core/internal/extension-coordinator.ts (1)
initExtensionCoordinator(32-115)
packages/pipeline/src/standard-pipeline/runner/index.ts (8)
packages/pipeline/src/core/types.ts (8)
PipelineReporter(41-43)PipelineDiagnostic(210-213)PipelineRunState(219-226)HelperKind(13-13)Helper(125-133)AgnosticPipelineOptions(323-409)HelperExecutionSnapshot(232-239)PipelineStep(159-163)packages/pipeline/src/standard-pipeline/types.ts (5)
PipelineReporter(17-17)PipelineRunState(18-18)CreatePipelineOptions(60-211)Pipeline(280-357)FragmentFinalizationMetadata(39-43)packages/pipeline/src/index.ts (14)
PipelineReporter(27-27)PipelineDiagnostic(35-35)PipelineRunState(52-52)HelperKind(45-45)Helper(41-41)CreatePipelineOptions(59-59)Pipeline(58-58)Halt(74-74)FragmentFinalizationMetadata(61-61)HelperExecutionSnapshot(53-53)PipelineStep(51-51)RegisteredHelper(66-66)makePipeline(4-4)maybeThen(81-81)scripts/precommit-utils.mjs (1)
deps(539-539)packages/pipeline/src/core/runner/types.ts (4)
AgnosticStageDeps(236-283)AgnosticState(109-163)Halt(178-182)HelperExecutionSnapshot(18-18)packages/pipeline/src/core/runner/stage-factories.ts (1)
makeFinalizeFragmentsStage(147-162)packages/pipeline/src/core/makePipeline.ts (1)
makePipeline(31-395)packages/pipeline/src/core/async-utils.ts (1)
maybeThen(34-47)
packages/pipeline/src/core/makePipeline.ts (4)
packages/pipeline/src/core/types.ts (6)
PipelineReporter(41-43)PipelineDiagnostic(210-213)MaybePromise(7-7)HelperKind(13-13)Helper(125-133)HelperDescriptor(49-63)packages/pipeline/src/core/runner/diagnostics.ts (1)
createAgnosticDiagnosticManager(78-219)packages/pipeline/src/core/runner/index.ts (1)
initAgnosticRunner(14-43)packages/pipeline/src/core/async-utils.ts (3)
isPromiseLike(11-20)maybeThen(34-47)maybeAll(160-167)
packages/pipeline/src/core/types.ts (2)
packages/pipeline/src/index.ts (9)
HelperKind(45-45)PipelineExtensionLifecycle(32-32)PipelineReporter(27-27)PipelineDiagnostic(35-35)PipelineRunState(52-52)PipelineExtensionRollbackErrorMetadata(54-54)HelperDescriptor(44-44)PipelineExtension(28-28)Helper(41-41)packages/pipeline/src/standard-pipeline/types.ts (4)
PipelineExtensionLifecycle(26-33)PipelineReporter(17-17)PipelineRunState(18-18)PipelineExtensionRollbackErrorMetadata(16-16)
🪛 markdownlint-cli2 (0.18.1)
packages/pipeline/README.md
51-51: Hard tabs
Column: 1
(MD010, no-hard-tabs)
52-52: Hard tabs
Column: 1
(MD010, no-hard-tabs)
53-53: Hard tabs
Column: 1
(MD010, no-hard-tabs)
54-54: Hard tabs
Column: 1
(MD010, no-hard-tabs)
56-56: Hard tabs
Column: 1
(MD010, no-hard-tabs)
57-57: Hard tabs
Column: 1
(MD010, no-hard-tabs)
58-58: Hard tabs
Column: 1
(MD010, no-hard-tabs)
70-70: Hard tabs
Column: 1
(MD010, no-hard-tabs)
71-71: Hard tabs
Column: 1
(MD010, no-hard-tabs)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (27)
packages/pipeline/src/core/__tests__/coverage.test.ts (1)
154-216: Good targeted coverage forhandleRollbackError(+ no-handler case).
These two tests are focused and validate the callback contract without over-coupling to internal details.packages/cli/src/dx/readiness/helpers/releasePack.ts (1)
62-65: No issues found. The artifact paths are correct and intentionally match the package.json exports configuration for @wpkernel/pipeline. The "./extensions" export explicitly specifiesdist/extensions/index.jsfor the implementation anddist/core/extensions/index.d.tsfor type definitions—this is the intended structure and the readiness check correctly verifies all six artifacts.packages/pipeline/src/standard-pipeline/__tests__/createExtension.test.ts (2)
1-10: LGTM: Import paths correctly reflect the new architecture.The import reorganization properly separates core utilities (helper, createExtension, core types) from standard-pipeline-specific types, aligning with the architectural split described in the PR.
206-342: LGTM: Test coverage for async extensions and lifecycle metadata is comprehensive.The remaining test cases correctly validate:
- Async extension setup with explicit await (line 211)
- Custom
registerimplementations- Lifecycle metadata handling
The test assertions and expected behaviors are accurate.
packages/pipeline/src/core/async-utils.ts (2)
38-40: LGTM! Good defensive runtime validation.The runtime check for
onFulfilledbeing a function is a solid defensive measure that will provide clear error messages during development.
160-167: LGTM! Sync-friendly Promise.all implementation is correct.The logic correctly handles the mixed sync/async case. The
readonlyinput type and returning the original array when all values are synchronous is an appropriate optimization.packages/pipeline/src/core/__tests__/stale-diagnostics.test.ts (1)
36-53: LGTM! Test properly validates stale diagnostics fix.The test now correctly:
- Runs the pipeline twice
- Clears the mock between runs
- Asserts that the second run produces the same number of diagnostics (not accumulated from first run)
This addresses the PR objective of preventing stale diagnostics from persisting between runs.
packages/pipeline/src/core/runner/execution.ts (1)
103-108: LGTM! Halt handling is correct.The halt detection properly checks for the
__haltsentinel and either throws the error or returns the result. The non-null assertion onresult.result!is safe here because iferroris falsy,resultmust be defined per the Halt type contract.packages/pipeline/src/core/runner/stage-factories.ts (2)
321-340: LGTM! Properly initializes helperExecution map.The code now correctly initializes the
helperExecutionmap if it doesn't exist, addressing the previous null reference concern. The snapshot initialization per kind is also correct.
342-347: LGTM! Step recording now includes execution tracking.The
recordStepfunction correctly updates both the global step list viapushStepand the per-kind execution snapshot. This provides good visibility into helper execution order.packages/pipeline/src/core/__tests__/multi-lifecycle.test.ts (1)
58-60: Stale comment: verify if the bug is now fixed.The comment "This is expected to fail currently" suggests this test was written to document a known bug. If the PR fixes this issue, the comment should be removed. If the test is now passing, update the comment or remove it entirely.
// Both should be called - expect(commitSpy1).toHaveBeenCalled(); // This is expected to fail currently + expect(commitSpy1).toHaveBeenCalled(); expect(commitSpy2).toHaveBeenCalled();packages/pipeline/src/core/__tests__/rollback.test.ts (1)
139-165: LGTM! Good coverage for async rollback failure handling.This test correctly validates the "Robust Best-Effort" rollback behavior for async failures, complementing the existing sync failure test. The LIFO order assertion
[3, 1]and error metadata verification are appropriate.packages/pipeline/src/core/__tests__/multi-lifecycle-rollback.test.ts (1)
80-94: LGTM! Clear LIFO rollback order verification.The added verification properly asserts that phase-two's rollback executes before phase-one's (LIFO order). The explanatory comments help document the expected behavior.
One minor improvement: the error message on lines 89-91 could be more descriptive to aid debugging if this ever fails.
if (order2 === undefined || order1 === undefined) { - throw new Error('Rollback spies were not called'); + throw new Error( + `Rollback spies were not called: order2=${order2}, order1=${order1}` + ); }packages/pipeline/src/core/runner/program.ts (3)
292-312: Good implementation of extension lifecycle validation.This correctly implements the PR objective of warning when extensions register hooks for lifecycles not in the execution plan. The iteration and warning logic is clear.
345-352: LGTM! Clean stages composition.The validation and reverse-compose pattern correctly assembles the pipeline program. The error message clearly indicates the required configuration.
132-188: The spec parameter type explicitly restricts spec to onlymakeArgsandonVisitedproperties. Theinvokemethod cannot be overridden via spec because TypeScript's type system preventsinvokefrom being passed in the spec argument. The current design is safe by design constraint, not by spread order.packages/pipeline/src/core/runner/context.ts (1)
51-54: Good fix:state.diagnosticsnow tracks manager diagnostics (live reference), preventing “missing diags on early exit”.
This addresses the earlier concern where diagnostics could be lost if execution aborted before a later sync.Also applies to: 141-145
packages/pipeline/src/core/__tests__/sync-execution.test.ts (1)
12-29: Test migration tocreateStages/createRunResultlooks correct and keeps the sync-vs-async signal.Also applies to: 50-68
packages/pipeline/src/standard-pipeline/runner/index.ts (1)
455-487: Nice:createErroris now null-safe inir.use()/builders.use()(prevents runtime crash).
This matches the earlier review concern and resolves it cleanly.packages/pipeline/src/core/__tests__/makePipeline.test.ts (1)
10-14: Test updates look consistent with the new stage-driven API and cover the right extension points.Also applies to: 16-35, 57-73
packages/pipeline/src/core/types.ts (3)
10-13: LGTM! Type broadened to support agnostic architecture.The change from a specific union to
stringaligns with the PR's goal of creating a generic core that doesn't prescribe concrete helper kinds. The standard pipeline (instandard-pipeline/types.ts) re-establishes concrete 'fragment' and 'builder' kinds as needed.
245-245: LGTM! Lifecycle type generalized for extensibility.Broadening to
stringallows the core to support custom lifecycle names while the standard pipeline (lines 26-33 instandard-pipeline/types.ts) defines the concrete set of lifecycles for fragment/builder workflows.
416-450: LGTM! Clean, simplified agnostic pipeline interface.The interface is well-designed with only 4 type parameters (compared to 16 in the standard pipeline), clear separation between generic helper registration and extension points, and self-referential extension typing that maintains type safety.
packages/pipeline/src/standard-pipeline/types.ts (4)
1-20: LGTM! Clean imports and re-exports.The imports are well-organized and the re-exports provide a convenient public surface for standard pipeline consumers without requiring them to import from core directly.
22-33: LGTM! Well-defined standard lifecycle phases.The lifecycle type properly defines the six standard phases for fragment/builder pipelines while maintaining extensibility through the
(string & {})escape hatch pattern.
213-274: LGTM! Extension type alias addresses previous feedback.This type alias successfully extracts the complex nested
PipelineExtensionsignature that was previously inlined in thePipelineinterface, directly addressing the maintainability concern raised in past reviews. While it still has 16 type parameters inherited fromPipeline, the extraction improves readability and reusability.
280-357: Extension type properly uses the extracted alias.The interface correctly uses
StandardPipelineExtension(lines 331-348) instead of inlining the nested type, implementing the suggestion from previous reviews. The overall 16-parameter complexity remains a longer-term maintainability consideration but doesn't block the current refactor.
| it('handles unresolved helpers during context preparation', () => { | ||
| // Mock dependencies | ||
| const createError = jest.fn((_, msg) => new Error(msg)); | ||
| const diagnosticManager = { | ||
| prepareRun: jest.fn(), | ||
| setReporter: jest.fn(), | ||
| flagMissingDependency: jest.fn(), | ||
| flagUnusedHelper: jest.fn(), | ||
| }; | ||
|
|
||
| const circularHelperA = { | ||
| key: 'A', | ||
| id: 'kind:A#0', | ||
| index: 0, | ||
| helper: { key: 'A', dependsOn: ['B'] }, | ||
| }; | ||
| const circularHelperB = { | ||
| key: 'B', | ||
| id: 'kind:B#1', | ||
| index: 1, | ||
| helper: { key: 'B', dependsOn: ['A'] }, | ||
| }; | ||
|
|
||
| const context = { reporter: {} }; | ||
| const dependencies = { | ||
| diagnosticManager, | ||
| helperRegistries: new Map([ | ||
| ['kind', [circularHelperA, circularHelperB]], | ||
| ]), | ||
| options: { | ||
| createContext: () => context, | ||
| createState: () => ({}), | ||
| createError, | ||
| }, | ||
| }; | ||
|
|
||
| const runOptions = {}; | ||
|
|
||
| expect(() => { | ||
| prepareContext(dependencies as any, runOptions as any); | ||
| }).toThrow('Detected unresolved pipeline helpers: A, B.'); | ||
|
|
||
| // Verify that onUnresolvedHelpers callback logic was executed | ||
| // We check if flagUnusedHelper was called for the circular refs | ||
| expect(diagnosticManager.flagUnusedHelper).toHaveBeenCalledTimes(2); | ||
| expect(diagnosticManager.flagUnusedHelper).toHaveBeenCalledWith( | ||
| circularHelperA.helper, | ||
| 'kind', | ||
| 'has unresolved dependencies (possible cycle)', | ||
| ['B'] | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Make the unresolved-helpers assertion order-insensitive to avoid flakes.
If helper ordering changes (Map iteration, graph traversal), "A, B" vs "B, A" will fail. Prefer a regex that asserts both keys are present.
expect(() => {
prepareContext(dependencies as any, runOptions as any);
-}).toThrow('Detected unresolved pipeline helpers: A, B.');
+}).toThrow(/Detected unresolved pipeline helpers: .*A.*B.*\./);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/pipeline/src/core/__tests__/coverage.test.ts around lines 62 to 113,
the test asserts the thrown message using a hard-coded ordered string "Detected
unresolved pipeline helpers: A, B." which flakes if helper order changes;
replace that exact-string assertion with an order-insensitive check — for
example, assert the thrown error message matches a regex that requires both "A"
and "B" to appear in any order or parse the message into keys and compare sets —
ensure the new assertion still verifies both helper keys are present without
depending on their order.
packages/pipeline/src/standard-pipeline/__tests__/createExtension.test.ts
Show resolved
Hide resolved
… stage, and refine error handling in tests.
This PR refactors
@wpkernel/pipelineto structurally separate the generic core (agnostic workflow engine) from the standard implementation (WPKernel's Fragment/Builder pattern). It also addresses critical bugs regarding diagnostic preservation and extension lifecycle validation.Key Changes
1. Architecture Split
src/core): Contains the pure DAG runner, dependency resolution, and extension orchestration. It is now completely agnostic of "fragments" or "builders".src/standard-pipeline): Implements the specific WPKernel pattern (createPipeline).2. Diagnostic Manager Fixes
clear()was wiping registration-time diagnostics (e.g., conflicts found duringpipeline.use()) before run() started.staticDiagnostics(preserved) andrunDiagnostics(cleared per run).finallyblock, preventing state leaks on failure.3. Extension Validation
4. Robust Rollbacks
Bug Fixes
Verification
ignored-lifecycle.test.tspnpm test).pnpm buildproduces correct exports for makePipeline and types.Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.