feat(pipeline-loader): add loader functionality#487
Conversation
|
📝 WalkthroughWalkthroughThis PR implements a comprehensive pipeline loader system for the pipeline-loader package, adding support for discovering and loading pipelines from local files, arbitrary content, and remote GitHub/GitLab repositories, along with test utilities for pipeline module generation. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Loader
participant Bundler
participant RemoteAPI as GitHub/GitLab API
participant ContentParser
rect rgba(100, 200, 255, 0.5)
Note over Client,ContentParser: Local File Pipeline Loading
Client->>Loader: loadPipelinesFromPaths([paths])
Loader->>Loader: Dynamic import each file
Loader->>ContentParser: Extract PipelineDefinition exports
ContentParser-->>Loader: pipelines, exportNames
Loader-->>Client: LoadPipelinesResult
end
rect rgba(100, 255, 200, 0.5)
Note over Client,ContentParser: Remote Pipeline Loading
Client->>Loader: loadRemotePipelines(source, filePaths)
Loader->>RemoteAPI: listFiles / fetchFile
RemoteAPI-->>Loader: File list / Content
Loader->>Bundler: bundleRemoteModule(content, identifier)
Bundler->>Bundler: Resolve imports via plugin
Bundler->>Bundler: Compile to ES module
Bundler-->>Loader: bundled code
Loader->>ContentParser: Dynamic import & extract pipelines
ContentParser-->>Loader: pipelines, exportNames
Loader-->>Client: LoadPipelinesResult with errors (if any)
end
rect rgba(255, 200, 100, 0.5)
Note over Client,ContentParser: Content-Based Pipeline Loading
Client->>Loader: loadPipelineFromContent(content, filename)
Loader->>Bundler: bundleRemoteModule(content, identifier)
Bundler-->>Loader: bundled code
Loader->>ContentParser: Dynamic import & extract pipelines
ContentParser-->>Loader: pipelines, exportNames
Loader-->>Client: LoadedPipelineFile
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
🌏 Preview Deployments
Built from commit: 🤖 This comment will be updated automatically when you push new commits to this PR. |
There was a problem hiding this comment.
Pull request overview
Introduces a new test-utils helper for generating pipeline module sources and expands @ucdjs/pipelines-loader to support local and remote pipeline discovery/loading (GitHub/GitLab), along with a new test suite validating loader behavior.
Changes:
- Add
#test-utils/pipelinesexport withcreatePipelineModuleSourceto generate.ucd-pipeline.tsmodule content for tests. - Implement local pipeline discovery/loading plus remote (GitHub/GitLab) listing and loading in
@ucdjs/pipelines-loader. - Replace the placeholder pipeline-loader test with targeted tests for local/remote/insecure loading paths.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/test-utils/tsdown.config.ts | Adds a new build entry for the pipelines subpath. |
| packages/test-utils/src/pipelines/source.ts | Adds helper to generate pipeline module source text with configurable exports. |
| packages/test-utils/src/pipelines/index.ts | Re-exports the pipeline source helper. |
| packages/test-utils/package.json | Exposes ./pipelines subpath export. |
| packages/pipelines/pipeline-loader/tsdown.config.ts | Adds build entries for remote and insecure entrypoints. |
| packages/pipelines/pipeline-loader/src/types.ts | Defines loader/remote source and result types. |
| packages/pipelines/pipeline-loader/src/loader.ts | Implements local file globbing and module loading. |
| packages/pipelines/pipeline-loader/src/remote/types.ts | Adds shared remote request/file listing types. |
| packages/pipelines/pipeline-loader/src/remote/github.ts | Implements GitHub tree listing and content fetch. |
| packages/pipelines/pipeline-loader/src/remote/gitlab.ts | Implements GitLab tree listing and raw file fetch. |
| packages/pipelines/pipeline-loader/src/remote/bundler.ts | Adds bundling + relative import resolution for remote/local module content. |
| packages/pipelines/pipeline-loader/src/insecure.ts | Loads pipeline definitions from arbitrary content via bundling + data URL import. |
| packages/pipelines/pipeline-loader/src/remote.ts | Adds remote pipeline discovery + loading orchestration. |
| packages/pipelines/pipeline-loader/src/index.ts | Exports public loader/remote APIs and types. |
| packages/pipelines/pipeline-loader/package.json | Exposes ./remote and ./insecure subpath exports. |
| packages/pipelines/pipeline-loader/test/loader.test.ts | Adds tests for local discovery/loading APIs. |
| packages/pipelines/pipeline-loader/test/remote.test.ts | Adds tests for GitHub/GitLab listing and remote loading behavior. |
| packages/pipelines/pipeline-loader/test/insecure.test.ts | Adds tests for bundling/import restrictions and parsing behavior. |
| packages/pipelines/pipeline-loader/test/index.test.ts | Removes placeholder test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
luxass
force-pushed
the
luxass/pipeline-loader
branch
from
ee2b29a to
6fa7ab2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@packages/pipelines/pipeline-loader/src/remote.ts`:
- Around line 36-43: The glob-to-regex conversion in the matcher creation is
broken: the chained .replace calls cause the '**' -> '.*' replacement to be
reprocessed by the subsequent '*' replacement and the pattern's regex
metacharacters (like '.') are not escaped, causing false matches; update the
logic in remote.ts where matcher is built (the RegExp creation and the pattern
variable usage) to either use the repo's existing picomatch utility to test
fileList.files against the glob (preferred) or implement a safe conversion that
first escapes regex metacharacters, then replaces a temporary placeholder for
'**' before replacing single '*' and '?' and finally swapping the placeholder
for '.*' so '**' isn't double-replaced; ensure you then use picomatch or the
corrected RegExp to filter fileList.files.
In `@packages/pipelines/pipeline-loader/src/remote/bundler.ts`:
- Around line 223-231: The current regex-driven loop in bundleRemoteModule is
vulnerable and incomplete; replace it with es-module-lexer parsing: import and
await the es-module-lexer init, call parse(input.content) to get the array of
import specifiers, for each specifier extract the string via
input.content.slice(spec.s, spec.e), skip empty/null specifiers, and call
assertRelativeSpecifier(specifier) as before; remove the regex matchAll logic
and rely on the lexer which correctly handles multi-line imports, dynamic
import() forms, and avoids ReDoS. Also ensure the es-module-lexer import/init is
added near the top of the module and that bundleRemoteModule awaits the init
before parsing.
- Around line 166-175: The compileModuleSource function currently passes the
full remote identifier into transform, which prevents TypeScript detection;
update compileModuleSource to parse the identifier (e.g., via new
URL(identifier)) and extract the logical filename from the query "path"
parameter (falling back to url.pathname or the original identifier if absent),
then pass that extracted filename into transform (use a variable like filename
instead of identifier) while keeping the same source and options; retain the
existing error handling and return result.code as before.
In `@packages/pipelines/pipeline-loader/src/remote/gitlab.ts`:
- Around line 30-48: The GitLab listFiles implementation currently fetches only
the first page (per_page=100) and hardcodes truncated:false; update the logic in
the GitLab listFiles flow (where encodeProjectPath, customFetch and the current
URL are used) to handle pagination or at minimum detect truncation via GitLab
response headers: after calling customFetch, read headers like x-next-page or
x-total-pages/x-total to determine if more pages exist and set truncated
accordingly, and if implementing pagination loop, repeatedly call customFetch
with the page query param (page=...) to accumulate all blob paths before
returning the final files array and correct truncated flag.
🧹 Nitpick comments (6)
packages/test-utils/src/pipelines/source.ts (1)
46-64: Consider exporting types alongsidecreatePipelineModuleSourcefor consumer ergonomics.
PipelineModuleSourceOptions,PipelineDefinition, and related types are exported from this file but onlycreatePipelineModuleSourceis re-exported from the barrel (index.ts). Test consumers may benefit from importing the option types directly (e.g., for typed helper wrappers). This is optional since inline objects work fine.Extend the barrel re-export in
packages/test-utils/src/pipelines/index.ts:-export { createPipelineModuleSource } from "./source"; +export { createPipelineModuleSource } from "./source"; +export type { PipelineModuleSourceOptions, PipelineDefinition, NamedExportConfig } from "./source";packages/pipelines/pipeline-loader/src/remote/types.ts (1)
1-8: Minor naming inconsistency:customFetchvsfetchFn.This interface uses
customFetchfor the optional fetch override, butLoadPipelineFromContentOptionsininsecure.ts(line 9) usesfetchFnfor the same concept. Consider aligning the naming across both interfaces for consistency.packages/pipelines/pipeline-loader/test/loader.test.ts (2)
68-71: Redundant assertion on line 70.
toEqual(["alpha"])on line 69 already implies length 1, making thetoHaveLength(1)check on line 70 redundant.
128-136: Consider usingrejects.toThrow()for consistency with other test files.The other test files in this PR (insecure.test.ts, remote.test.ts) use
expect(...).rejects.toThrow()for thethrowOnErrorscenario. Using the same pattern here would be more consistent. The current try/catch approach works but risks silently catching the manual fail-safe throw if the.messageassertion ever changes.♻️ Suggested refactor
- try { - await loadPipelinesFromPaths([missingPath], { throwOnError: true }); - throw new Error("Expected loadPipelinesFromPaths to throw"); - } catch (error) { - expect(error).toBeInstanceOf(Error); - expect((error as Error).message).toContain(`Failed to load pipeline file: ${missingPath}`); - expect((error as Error).cause).toBeInstanceOf(Error); - } + await expect( + loadPipelinesFromPaths([missingPath], { throwOnError: true }), + ).rejects.toThrow(`Failed to load pipeline file: ${missingPath}`);Note: if the
error.causeassertion is important, you can keep the try/catch but addexpect.assertions(3)at the top of the test to guard against the fail-safe being silently caught.packages/pipelines/pipeline-loader/src/remote/bundler.ts (1)
196-204: Fragile string-based error matching.Matching on
err.message.includes("Module not found")anderr.message.includes("404")is brittle — any change to the error messages in the GitHub/GitLab modules would silently break candidate resolution. Consider using custom error classes or error codes for structured matching.packages/pipelines/pipeline-loader/src/remote.ts (1)
56-67: Duplicated identifier formatting logic.
buildRemoteIdentifierduplicatesformatRemoteIdentifierinbundler.ts— both produce the sameprovider://owner/repo?ref=X&path=Yformat. Consider extracting a shared utility to avoid divergence.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
…unctionality - Introduced `remote.ts` and `insecure.ts` for handling remote pipeline files and loading from content. - Updated `index.ts` to export new functions and types from the added modules. - Enhanced `package.json` to include new exports for `./insecure` and `./remote`.
…urces - Implement tests for `findRemotePipelineFiles` and `loadRemotePipelines` functions. - Validate behavior for GitHub and GitLab file loading, including error handling. - Ensure proper filtering of pipeline files based on specified patterns.
- Introduced `oxc-parser` for parsing capabilities and `picomatch` for improved glob matching. - Updated `loadPipelineFromContent` to use `customFetch` instead of `fetchFn`. - Enhanced error handling for remote file fetching in GitHub and GitLab. - Added utility functions for remote identifier parsing and formatting. - Improved test cases to reflect changes in function signatures and error handling.
…unctionality Added support for loading and bundling remote modules with a new `bundleRemoteModule` function. Introduced error handling for remote not found scenarios and created utility functions for parsing and formatting remote identifiers. Refactored existing code to improve structure and maintainability.
luxass
force-pushed
the
luxass/pipeline-loader
branch
from
5b5165e to
7a56094
Compare
luxass
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 25 changed files in this pull request and generated 5 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (isUrlLike(identifier)) { | ||
| const url = new URL(identifier); | ||
| url.pathname = `${url.pathname}${suffix}`; | ||
| return url.toString(); | ||
| } |
There was a problem hiding this comment.
appendSuffix calls new URL(identifier) when isUrlLike(identifier) is true, but new URL(...) will throw for Windows-style paths if they are misclassified as URL-like (e.g., C:\...). Even with an isUrlLike fix, adding a small guard/try-catch here would prevent hard-to-debug crashes from any future misclassification.
| function buildNamedExports( | ||
| named: PipelineModuleSourceNamed, | ||
| definitions: Record<string, Partial<PipelineDefinition>>, | ||
| ): string { | ||
| if (isStringArray(named)) { | ||
| return named | ||
| .map((name) => `export const ${name} = ${buildDefinition(name, definitions[name])};`) | ||
| .join("\n\n"); | ||
| } | ||
|
|
||
| return Object.entries(named) | ||
| .map(([name, value]) => { |
There was a problem hiding this comment.
createPipelineModuleSource interpolates named values directly into export const ${name} = .... If a caller passes a non-identifier (e.g., contains -, starts with a number, or is a reserved word in older runtimes), the generated module source becomes invalid and fails later during parsing/bundling. Consider validating/sanitizing export names early and throwing a clear error when an invalid identifier is provided.
| function buildNamedExports( | |
| named: PipelineModuleSourceNamed, | |
| definitions: Record<string, Partial<PipelineDefinition>>, | |
| ): string { | |
| if (isStringArray(named)) { | |
| return named | |
| .map((name) => `export const ${name} = ${buildDefinition(name, definitions[name])};`) | |
| .join("\n\n"); | |
| } | |
| return Object.entries(named) | |
| .map(([name, value]) => { | |
| const RESERVED_IDENTIFIERS = new Set<string>([ | |
| "break", | |
| "case", | |
| "catch", | |
| "class", | |
| "const", | |
| "continue", | |
| "debugger", | |
| "default", | |
| "delete", | |
| "do", | |
| "else", | |
| "enum", | |
| "export", | |
| "extends", | |
| "false", | |
| "finally", | |
| "for", | |
| "function", | |
| "if", | |
| "import", | |
| "in", | |
| "instanceof", | |
| "new", | |
| "null", | |
| "return", | |
| "super", | |
| "switch", | |
| "this", | |
| "throw", | |
| "true", | |
| "try", | |
| "typeof", | |
| "var", | |
| "void", | |
| "while", | |
| "with", | |
| "yield", | |
| "let", | |
| "static", | |
| "implements", | |
| "interface", | |
| "package", | |
| "private", | |
| "protected", | |
| "public", | |
| ]); | |
| function isValidIdentifier(name: string): boolean { | |
| if (!/^[A-Za-z_$][A-Za-z0-9_$]*$/.test(name)) { | |
| return false; | |
| } | |
| return !RESERVED_IDENTIFIERS.has(name); | |
| } | |
| function assertValidIdentifier(name: string): void { | |
| if (!isValidIdentifier(name)) { | |
| throw new Error( | |
| `Invalid export name "${name}". Exported names must be valid JavaScript identifiers.`, | |
| ); | |
| } | |
| } | |
| function buildNamedExports( | |
| named: PipelineModuleSourceNamed, | |
| definitions: Record<string, Partial<PipelineDefinition>>, | |
| ): string { | |
| if (isStringArray(named)) { | |
| return named | |
| .map((name) => { | |
| assertValidIdentifier(name); | |
| return `export const ${name} = ${buildDefinition(name, definitions[name])};`; | |
| }) | |
| .join("\n\n"); | |
| } | |
| return Object.entries(named) | |
| .map(([name, value]) => { | |
| assertValidIdentifier(name); |
| const { owner, repo, ref = "HEAD" } = repoRef; | ||
| const { customFetch = fetch } = options; | ||
|
|
||
| const encodedPath = encodeURIComponent(filePath); |
There was a problem hiding this comment.
fetchFile URL-encodes the entire filePath with encodeURIComponent, which also encodes / into %2F. The GitHub Contents API expects path segments separated by / (i.e., /contents/pipelines/alpha...), so this will request the wrong resource and likely 404. Preserve / when encoding (e.g., encode each segment and join with /, or use an encoding approach that leaves slashes intact).
| const encodedPath = encodeURIComponent(filePath); | |
| const encodedPath = filePath | |
| .split("/") | |
| .map((segment) => encodeURIComponent(segment)) | |
| .join("/"); |
| "GET", | ||
| "https://api.github.com/repos/ucdjs/demo-pipelines/contents/pipelines%2Falpha.ucd-pipeline.ts", | ||
| () => HttpResponse.json({ content: encodeBase64(alpha), encoding: "base64" }), | ||
| ], | ||
| [ | ||
| "GET", | ||
| "https://api.github.com/repos/ucdjs/demo-pipelines/contents/pipelines%2Fbeta.ucd-pipeline.ts", | ||
| () => HttpResponse.json({ content: encodeBase64(beta), encoding: "base64" }), | ||
| ], |
There was a problem hiding this comment.
These GitHub mock URLs use contents/pipelines%2F..., which matches the current implementation but not the real GitHub Contents API path handling (slashes should remain path separators). Once fetchFile is fixed to preserve /, the tests should be updated to mock .../contents/pipelines/alpha.ucd-pipeline.ts?... (and similar for other files).
| path: string; | ||
| } | ||
|
|
||
| export function isUrlLike(value: string): boolean { |
There was a problem hiding this comment.
isUrlLike will treat Windows absolute paths like C:\path\file.ts or C:/path/file.ts as “URL-like” because they match the ^[a-z][a-z+.-]*: scheme regex. This breaks the bundler on Windows (e.g., appendSuffix/new URL(...) and loadRemoteSource will throw instead of reading local files). Please special-case Windows drive-letter paths so they are not classified as URLs.
| export function isUrlLike(value: string): boolean { | |
| export function isUrlLike(value: string): boolean { | |
| // Treat Windows drive-letter absolute paths like "C:\path\file.ts" or "C:/path/file.ts" | |
| // as non-URL values, even though they match the generic "scheme:" regex below. | |
| if (/^[a-zA-Z]:[\\/]/.test(value)) { | |
| return false; | |
| } |
🔗 Linked issue
📚 Description
Summary by CodeRabbit
New Features
Tests