feat(pipeline-core): add pipeline hooks for lifecycle management#608
Merged
feat(pipeline-core): add pipeline hooks for lifecycle management#608
Conversation
Enhance the documentation for pipeline definitions by introducing sections on lifecycle hooks, including examples and usage scenarios. This addition clarifies how hooks can be utilized for side effects during execution and debugging.
Contributor
📝 WalkthroughWalkthroughAdds pipeline lifecycle hooks: new types and public exports, documents hook usage, wires hook invocation across executor stages (pipeline, version, route, parse, resolve, output), propagates hooks through execution context and output materialization, and adds end-to-end tests for sequencing and error semantics. Changes
Sequence DiagramsequenceDiagram
participant User as "User"
participant Def as "Pipeline Definition"
participant Exec as "Executor"
participant Parser as "Parse/Resolve"
participant Output as "Output Materializer"
participant Hooks as "Hook Callbacks"
User->>Def: definePipeline(config with hooks)
Def->>Exec: execute(pipeline)
Exec->>Hooks: pipeline: start (phase)
Exec->>Hooks: version: start (phase)
Exec->>Hooks: route: start (routeId)
Exec->>Parser: run parse
Parser->>Hooks: parse: start
Parser->>Parser: filter/transform
Parser->>Hooks: parse: end (row counts, outputs)
Parser->>Hooks: resolve: start
Parser->>Parser: resolver -> outputs
Parser->>Hooks: resolve: end (outputs or error)
Exec->>Output: materializeOutputs(outputs)
Output->>Hooks: output: start (output metadata)
Output->>Output: write to sink
Output->>Hooks: output: end (status, locator) or output: end (failed, error)
Exec->>Hooks: route: end (outputs or error)
Exec->>Hooks: version: end (aggregated result)
Exec->>Hooks: pipeline: end (aggregated errors)
Exec-->>User: execution complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
Contributor
🌏 Preview Deployments
Built from commit: 🤖 This comment will be updated automatically when you push new commits to this PR. |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds lifecycle hooks to pipeline definitions so callers can observe pipeline execution phases (pipeline/version/route/parse/resolve/output) and use them for side effects like debugging or metrics.
Changes:
- Introduces
PipelineHooksand hook context types in@ucdjs/pipeline-coreand exposes them via the public index. - Threads hook callbacks through the pipeline executor and emits hook events at key lifecycle points.
- Adds docs and a Vitest test validating hook invocation order and phase-aware contexts.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/pipelines/pipeline-executor/test/run.test.ts | Adds a test asserting hook call order and phase contexts. |
| packages/pipelines/pipeline-executor/src/run/route.ts | Emits parse/resolve hook events during parse+resolve execution. |
| packages/pipelines/pipeline-executor/src/run/outputs.ts | Emits output hook events around output materialization and writing. |
| packages/pipelines/pipeline-executor/src/run/context.ts | Extends executor context to carry optional hooks. |
| packages/pipelines/pipeline-executor/src/run.ts | Emits pipeline/version/route hook events and passes hooks into route/output execution. |
| packages/pipelines/pipeline-core/src/pipeline.ts | Adds hooks?: PipelineHooks to pipeline definition spec and definePipeline result. |
| packages/pipelines/pipeline-core/src/index.ts | Re-exports hook types and hasPipelineHooks. |
| packages/pipelines/pipeline-core/src/hooks.ts | Defines hook phases, contexts, hook interface, and hasPipelineHooks. |
| apps/docs/content/pipelines/running.mdx | Documents defining hooks and when to use them. |
| apps/docs/content/pipelines/guides/debug-pipelines.mdx | Adds guidance for lifecycle-level debugging via hooks. |
| apps/docs/content/pipelines/definitions.mdx | Documents the hooks option and available hook points. |
| apps/docs/content/pipelines/api.mdx | Adds API reference section for pipeline hooks and context fields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+191
to
+195
| await hooks?.output?.({ | ||
| phase: "end", | ||
| pipelineId, | ||
| version, | ||
| file, |
There was a problem hiding this comment.
If the output end hook throws after a successful write, the surrounding try/catch will treat it like a write failure: it will enter the catch block, record the output as failed, and emit a second end notification (and a second manifest entry) even though the sink write succeeded. Consider separating hook failures from write failures and ensuring each output produces exactly one manifest entry and one output end event.
Comment on lines
+27
to
+29
| export interface PipelineResolveHookContext extends PipelineRouteHookContext { | ||
| outputs?: readonly unknown[]; | ||
| } |
There was a problem hiding this comment.
PipelineResolveHookContext re-declares outputs even though it already inherits outputs from PipelineRouteHookContext. This duplication increases the chance of the types drifting (e.g., different readonly-ness) and doesn’t add any extra specificity. Consider removing the redundant outputs property from PipelineResolveHookContext (or, if it’s meant to narrow/strengthen the type, make that intention explicit).
Suggested change
| export interface PipelineResolveHookContext extends PipelineRouteHookContext { | |
| outputs?: readonly unknown[]; | |
| } | |
| export interface PipelineResolveHookContext extends PipelineRouteHookContext {} |
Comment on lines
+119
to
+123
| await ctx.hooks?.parse?.({ | ||
| phase: "end", | ||
| pipelineId: ctx.pipelineId, | ||
| version: ctx.version, | ||
| file: ctx.file, |
There was a problem hiding this comment.
If parser(...), transforms, or resolver(...) throw, this parse end hook is never reached, so hook consumers won’t see an end-phase (or the error). Consider restructuring executeParseResolve to always emit parse with phase: "end" from a finally block, including error when the route fails.
Comment on lines
+88
to
+92
| await ctx.hooks?.resolve?.({ | ||
| phase: "end", | ||
| pipelineId: ctx.pipelineId, | ||
| version: ctx.version, | ||
| file: ctx.file, |
There was a problem hiding this comment.
An exception thrown by the resolve end hook here will be caught by the surrounding try and then the catch block will call the resolve end hook again with error, resulting in duplicate hook invocations/side effects. Refactor so the resolve end hook is called exactly once (e.g., collect outputs/error and invoke the hook in a single finally).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/pipelines/pipeline-core/src/hooks.ts (1)
27-29: Redundantoutputsfield inPipelineResolveHookContext.
PipelineResolveHookContextextendsPipelineRouteHookContext, which already declaresoutputs?: readonly unknown[]on line 19. The redeclaration on line 28 is redundant.♻️ Remove redundant field
-export interface PipelineResolveHookContext extends PipelineRouteHookContext { - outputs?: readonly unknown[]; -} +export interface PipelineResolveHookContext extends PipelineRouteHookContext {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pipelines/pipeline-core/src/hooks.ts` around lines 27 - 29, Remove the redundant outputs declaration from the PipelineResolveHookContext interface: since PipelineResolveHookContext extends PipelineRouteHookContext (which already defines outputs?: readonly unknown[]), delete the outputs?: readonly unknown[] line from the PipelineResolveHookContext declaration so the child interface relies on the inherited field (ensure the file still exports PipelineResolveHookContext and type-checks).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/pipelines/pipeline-executor/src/run/outputs.ts`:
- Around line 191-205: The success "end" hook invocation (the hooks?.output?.
call that passes phase: "end" and status: "written" in outputs.ts) must not
allow its errors to propagate and trigger the failure hook; wrap that success
hook call in its own try-catch and log any hook error (do not rethrow) so a
thrown error from the success hook does not fall through to the outer catch
which invokes the failure "end" hook (the block that sends phase: "end" with
status: "failed"); apply the same pattern symmetrically wherever you call the
success and failure end hooks so hook failures are isolated from pipeline
outcome handling.
---
Nitpick comments:
In `@packages/pipelines/pipeline-core/src/hooks.ts`:
- Around line 27-29: Remove the redundant outputs declaration from the
PipelineResolveHookContext interface: since PipelineResolveHookContext extends
PipelineRouteHookContext (which already defines outputs?: readonly unknown[]),
delete the outputs?: readonly unknown[] line from the PipelineResolveHookContext
declaration so the child interface relies on the inherited field (ensure the
file still exports PipelineResolveHookContext and type-checks).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 75e7c96f-0744-4c09-a8db-ea4174a174a4
📒 Files selected for processing (12)
apps/docs/content/pipelines/api.mdxapps/docs/content/pipelines/definitions.mdxapps/docs/content/pipelines/guides/debug-pipelines.mdxapps/docs/content/pipelines/running.mdxpackages/pipelines/pipeline-core/src/hooks.tspackages/pipelines/pipeline-core/src/index.tspackages/pipelines/pipeline-core/src/pipeline.tspackages/pipelines/pipeline-executor/src/run.tspackages/pipelines/pipeline-executor/src/run/context.tspackages/pipelines/pipeline-executor/src/run/outputs.tspackages/pipelines/pipeline-executor/src/run/route.tspackages/pipelines/pipeline-executor/test/run.test.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/pipelines/pipeline-executor/src/run/hooks.ts (1)
19-22: ⚡ Quick winPreserve stack context in hook-failure logs.
Line 21 only stores a flattened message string; adding stack metadata would make production hook failures much easier to diagnose.
Proposed change
} catch (error) { options.logger.error("Pipeline hook failed", { hook: name, error: error instanceof Error ? error.message : String(error), + ...(error instanceof Error && error.stack ? { stack: error.stack } : {}), }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pipelines/pipeline-executor/src/run/hooks.ts` around lines 19 - 22, The log call in hooks.ts currently only records error.message; update the options.logger.error invocation in the hook failure path to include the error's stack and/or full error object so stack traces are preserved for debugging: when building the metadata for options.logger.error (the hook: name, error: ... fields), add an errorStack or stack field populated from error.stack when error is an instance of Error, and also consider passing the original error object (or String(error)) as an additional field so the logger receives both message and stack; modify the code around the options.logger.error call that uses the variables error and name to include these extra fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/pipelines/pipeline-executor/src/run/hooks.ts`:
- Around line 19-22: The log call in hooks.ts currently only records
error.message; update the options.logger.error invocation in the hook failure
path to include the error's stack and/or full error object so stack traces are
preserved for debugging: when building the metadata for options.logger.error
(the hook: name, error: ... fields), add an errorStack or stack field populated
from error.stack when error is an instance of Error, and also consider passing
the original error object (or String(error)) as an additional field so the
logger receives both message and stack; modify the code around the
options.logger.error call that uses the variables error and name to include
these extra fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dcca02fa-1f23-40ad-b22b-bab9f8713aae
📒 Files selected for processing (8)
apps/docs/content/pipelines/api.mdxapps/docs/content/pipelines/definitions.mdxpackages/pipelines/pipeline-core/src/hooks.tspackages/pipelines/pipeline-executor/src/run.tspackages/pipelines/pipeline-executor/src/run/hooks.tspackages/pipelines/pipeline-executor/src/run/outputs.tspackages/pipelines/pipeline-executor/src/run/route.tspackages/pipelines/pipeline-executor/test/run.test.ts
✅ Files skipped from review due to trivial changes (3)
- apps/docs/content/pipelines/api.mdx
- packages/pipelines/pipeline-core/src/hooks.ts
- packages/pipelines/pipeline-executor/src/run/route.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/docs/content/pipelines/definitions.mdx
- packages/pipelines/pipeline-executor/test/run.test.ts
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
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.
🔗 Linked issue
📚 Description
Summary by CodeRabbit
New Features
Documentation
Tests