Repository navigation
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces a new "action" executor type for DAG steps that can execute Deno-based actions from remote sources and registries. The implementation spans schema updates, step normalization, bundle resolution with git caching, manifest validation, and a full Deno-based executor with permission-driven CLI invocation. ChangesAction Executor Framework
Sequence Diagram(s)sequenceDiagram
participant Client as DAG Step
participant Resolver as resolveBundle
participant Git as Git Ops
participant Cache as Cache Dir
participant Manifest as loadManifest
participant Deno as resolveDeno
participant Executor as Deno Process
participant Output as output.json
Client->>Resolver: source:github.com/...@v1.0
alt Cached
Resolver->>Cache: Check SHA-keyed cache
Cache-->>Resolver: Return cached RootDir
else Not Cached
Resolver->>Git: Clone repo depth-1
Git->>Git: Checkout version/branch
Git-->>Resolver: Get commit SHA
Resolver->>Cache: Store under SHA key
end
Resolver-->>Client: actionBundle{RootDir, Ref, Version}
Client->>Manifest: Load dagu-action.yaml
Manifest-->>Client: {name, runtime, entrypoint, permissions}
Client->>Deno: Resolve deno `@version`
alt Existing
Deno->>Deno: Check manifest
Deno-->>Client: /tools/deno/bin/deno
else Install
Deno->>Deno: aqua install denoland/deno
Deno-->>Client: /tools/deno/bin/deno
end
Client->>Executor: deno run --allow-read --allow-write --allow-net --allow-env
Executor->>Output: Action writes JSON
Output-->>Client: {key: value}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/runtime/builtin/action/action.go`:
- Around line 225-231: The write permissions are being rebased under the run
output directory instead of the action root; update calls that pass outputDir
into writePermissions (e.g., in denoRunArgs) to pass the action root (rootDir)
so writePermissions resolves entries relative to the action root as validated by
loadManifest; apply the same change to the other call site that currently uses
outputDir (the analogous block around where writePermissions is invoked for
tests/other runs) and ensure you keep using the same permissionManifest
(perms.Write) and helper function names (writePermissions, readPermissions,
loadManifest) so resolution and validation remain consistent.
In `@internal/runtime/builtin/action/resolver.go`:
- Around line 181-183: The temporary clone path currently built as tmp :=
fmt.Sprintf("%s.%d.tmp", root, os.Getpid()) can collide for concurrent
resolution attempts; change creation of the temp directory used by runGit in
resolver.go to use a truly unique per-attempt directory (e.g., os.MkdirTemp /
os.CreateTemp or append a monotonic timestamp/UUID to root+pid) and use that
returned path as tmp, keeping the existing cleanup (os.RemoveAll(tmp)) and
subsequent atomic rename logic intact; update the code that references tmp and
the runGit call to use the new unique tmp variable to avoid races.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c610026c-760c-4c88-945b-b7014f10ff95
📒 Files selected for processing (19)
internal/cmn/schema/dag.schema.jsoninternal/cmn/schema/dag_schema_test.gointernal/core/spec/step_types.gointernal/core/spec/step_v2.gointernal/core/spec/step_v2_test.gointernal/core/step.gointernal/engine/run.gointernal/runtime/builtin/action/action.gointernal/runtime/builtin/action/action_integration_test.gointernal/runtime/builtin/action/action_test.gointernal/runtime/builtin/action/deno.gointernal/runtime/builtin/action/manifest.gointernal/runtime/builtin/action/resolver.gointernal/runtime/builtin/builtin.gointernal/tools/aqua/installer.gointernal/tools/aqua/installer_test.gointernal/tools/prepare.gointernal/tools/tools.gointernal/tools/tools_test.go
Comment on lines
+225
to
+231
| func denoRunArgs(rootDir, entrypoint, inputPath, outputDir string, perms permissionManifest) []string { | ||
| args := []string{ | ||
| "run", | ||
| "--cached-only", | ||
| "--no-prompt", | ||
| "--allow-read=" + strings.Join(readPermissions(rootDir, inputPath, perms.Read), ","), | ||
| "--allow-write=" + strings.Join(writePermissions(outputDir, perms.Write), ","), |
There was a problem hiding this comment.
Resolve permissions.write against the action root, not outputDir.
loadManifest validates write paths as action-root-relative paths, but writePermissions rebases the same entries under the run output directory. A manifest like permissions.write: ["cache/state.json"] will validate, then Deno gets access to <runDir>/cache/state.json instead of <actionDir>/cache/state.json.
Suggested fix
- "--allow-write=" + strings.Join(writePermissions(outputDir, perms.Write), ","),
+ "--allow-write=" + strings.Join(writePermissions(rootDir, outputDir, perms.Write), ","),-func writePermissions(outputDir string, extra []string) []string {
+func writePermissions(rootDir, outputDir string, extra []string) []string {
paths := []string{filepath.Clean(outputDir)}
for _, item := range extra {
item = strings.TrimSpace(item)
if item == "" || filepath.IsAbs(item) {
continue
}
- path := filepath.Join(outputDir, item)
- if isPathWithin(outputDir, path) {
- paths = append(paths, filepath.Clean(path))
+ if path, err := safeRelativePath(rootDir, item); err == nil {
+ paths = append(paths, path)
}
}
return dedupeStrings(paths)
}Also applies to: 277-289
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/runtime/builtin/action/action.go` around lines 225 - 231, The write
permissions are being rebased under the run output directory instead of the
action root; update calls that pass outputDir into writePermissions (e.g., in
denoRunArgs) to pass the action root (rootDir) so writePermissions resolves
entries relative to the action root as validated by loadManifest; apply the same
change to the other call site that currently uses outputDir (the analogous block
around where writePermissions is invoked for tests/other runs) and ensure you
keep using the same permissionManifest (perms.Write) and helper function names
(writePermissions, readPermissions, loadManifest) so resolution and validation
remain consistent.
Comment on lines
+181
to
+183
| tmp := fmt.Sprintf("%s.%d.tmp", root, os.Getpid()) | ||
| _ = os.RemoveAll(tmp) | ||
| if err := runGit(ctx, "", "clone", "--depth", "1", "--branch", version, repoURL, tmp); err != nil { |
There was a problem hiding this comment.
Make the clone temp directory unique per resolution attempt.
This path is only keyed by root and os.Getpid(), so parallel first-time installs of the same action inside one worker process will race on the same *.tmp directory. One clone can remove or rename the other and make resolution fail nondeterministically.
Suggested fix
- tmp := fmt.Sprintf("%s.%d.tmp", root, os.Getpid())
- _ = os.RemoveAll(tmp)
+ tmp, err := os.MkdirTemp(filepath.Dir(root), filepath.Base(root)+".*.tmp")
+ if err != nil {
+ return "", "", fmt.Errorf("create action source temp dir: %w", err)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| tmp := fmt.Sprintf("%s.%d.tmp", root, os.Getpid()) | |
| _ = os.RemoveAll(tmp) | |
| if err := runGit(ctx, "", "clone", "--depth", "1", "--branch", version, repoURL, tmp); err != nil { | |
| tmp, err := os.MkdirTemp(filepath.Dir(root), filepath.Base(root)+".*.tmp") | |
| if err != nil { | |
| return "", "", fmt.Errorf("create action source temp dir: %w", err) | |
| } | |
| if err := runGit(ctx, "", "clone", "--depth", "1", "--branch", version, repoURL, tmp); err != nil { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/runtime/builtin/action/resolver.go` around lines 181 - 183, The
temporary clone path currently built as tmp := fmt.Sprintf("%s.%d.tmp", root,
os.Getpid()) can collide for concurrent resolution attempts; change creation of
the temp directory used by runGit in resolver.go to use a truly unique
per-attempt directory (e.g., os.MkdirTemp / os.CreateTemp or append a monotonic
timestamp/UUID to root+pid) and use that returned path as tmp, keeping the
existing cleanup (os.RemoveAll(tmp)) and subsequent atomic rename logic intact;
update the code that references tmp and the runGit call to use the new unique
tmp variable to avoid races.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add a generic external
actionexecutor backed by Deno so Dagu can run third-party actions without expanding core builtins for every integration.Changes
actionexecutor support for Deno-based external actions.action: source:...@...andaction: pkg:...@...into the executor config.type: actionwithrefandinputconfig.file://package dirs.dagu-action.yaml, validate inputs and permissions, and run Deno with constrained read/write/net/env permissions.DAGU_TOOLS_DIRfor runs without declared DAG tools.manifest.jsondoes not exist yet.Related Issues
None linked.
Validation
go test ./internal/core ./internal/runtime ./internal/runtime/builtin/... ./internal/engine ./internal/cmd ./internal/service/worker ./internal/tools ./internal/tools/aqua ./internal/cmn/schemaDAGU_ACTION_DENO_INTEGRATION=1 go test -count=1 -run TestExecutorRunsLocalSourceActionWithRealDenoIntegration -v ./internal/runtime/builtin/actionDAGU_AQUA_INTEGRATION=1 go test -count=1 -run TestInstallerInstallIntegration -v ./internal/tools/aquago fix -diff ./internal/runtime/builtin/actionChecklist