Repository navigation
feat: add DAG-level aqua tools - #2150
Merged
Merged
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 implements DAG-level tool installation via aqua, allowing workflows to declare and install external CLI dependencies before execution. It introduces tool configuration schemas, spec loading with validation, aqua-backed installer with file locking, environment variable generation, and runtime command resolution to expose installed tools to step processes. ChangesTool Installation Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 5
🧹 Nitpick comments (3)
internal/tools/tools.go (1)
137-137: ⚡ Quick winHardcoded manifest filename creates inconsistency risk.
Line 137 constructs the manifest path by joining
manifest.EnvDirwith the hardcoded constantmanifestFileName, but theManifeststruct doesn't validate that its fields are consistent with this convention. If the manifest was constructed with a different path structure, this could produce an incorrect path.♻️ Proposed fix to use the manifest's existing path
Since
CacheLayoutalready includesManifestFile, and the manifest is written to that path by the installer, consider storing the manifest file path directly in theManifeststruct or using it from there:return []string{ "AQUA_ROOT_DIR=" + manifest.RootDir, "AQUA_CONFIG=" + manifest.Config, "AQUA_DISABLE_LAZY_INSTALL=true", "AQUA_CHECKSUM=true", "AQUA_REQUIRE_CHECKSUM=true", "AQUA_ENFORCE_CHECKSUM=true", "AQUA_ENFORCE_REQUIRE_CHECKSUM=true", - EnvManifest + "=" + filepath.Join(manifest.EnvDir, manifestFileName), + EnvManifest + "=" + filepath.Join(manifest.EnvDir, "manifest.json"), "PATH=" + pathValue, }Or better, add a
ManifestFilefield to theManifeststruct to track the actual path.🤖 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/tools/tools.go` at line 137, The code builds the env manifest path using a hardcoded manifestFileName joined to manifest.EnvDir (EnvManifest + "=" + filepath.Join(manifest.EnvDir, manifestFileName)), which can mismatch how the manifest was originally written; update the code to derive the file path from the manifest's recorded path instead of hardcoding—either add a ManifestFile field to the Manifest struct or use the existing CacheLayout.ManifestFile (or other canonical ManifestFile property) when available, and replace uses of filepath.Join(manifest.EnvDir, manifestFileName) with that manifest.ManifestFile (or cacheLayout.ManifestFile); ensure Manifest creation/installer sets and validates this field so the constructed EnvManifest is always consistent with where the manifest was actually written.internal/tools/aqua/config.go (1)
23-23: 💤 Low valueShallow copy of ToolPackage structs may cause mutation bugs.
Line 23 creates a new slice and copies
ToolPackagestructs by value, which is correct for avoiding slice aliasing. However, if any future fields inToolPackageare pointers or slices (e.g.,Commands []string), mutations to the effective config could affect the original. CurrentlyCommandsis a slice, so modifications toeffective.Packages[i].Commandswould affect the original.♻️ Defensive deep copy for future safety
effective := *cfg - effective.Packages = append([]core.ToolPackage{}, cfg.Packages...) + effective.Packages = make([]core.ToolPackage, len(cfg.Packages)) + for i, pkg := range cfg.Packages { + pkgCopy := pkg + pkgCopy.Commands = append([]string{}, pkg.Commands...) + effective.Packages[i] = pkgCopy + }Note: This is defensive coding for future changes; the current code doesn't mutate these fields.
🤖 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/tools/aqua/config.go` at line 23, The current append into effective.Packages copies ToolPackage structs shallowly which leaves nested slices (e.g., ToolPackage.Commands) shared and susceptible to mutation; update the code that sets effective.Packages (replace the append of cfg.Packages) to allocate a new slice and deep-copy each ToolPackage: for each pkg in cfg.Packages create a new ToolPackage value, copy primitive fields, and clone any slice or pointer fields (at minimum duplicate Commands with a new []string and copy its elements) before appending to effective.Packages so mutations to effective do not affect cfg.Packages.internal/core/spec/dag_test.go (1)
173-192: ⚡ Quick winAdd a regression test for shell-metacharacter command names.
Current new tests reject path-like commands, but they still miss shell-fragment tokens (e.g.
jq;echo). Adding one negative case here will lock in the intended contract.Suggested test addition
+func TestLoadDAGToolsRejectsShellFragmentCommand(t *testing.T) { + t.Parallel() + + _, err := LoadYAML(context.Background(), []byte(` +tools: + provider: aqua + packages: + - package: jqlang/jq + version: jq-1.7.1 + commands: ["jq;echo"] +steps: + - name: check + command: jq +`)) + + require.Error(t, err) + assert.Contains(t, err.Error(), `must be an executable name`) +}🤖 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/core/spec/dag_test.go` around lines 173 - 192, Extend TestLoadDAGToolsRejectsCommandPath to also assert that shell-metacharacter fragments are rejected: call LoadYAML with a tools/packages entry where commands contains a token like "jq;echo" (or "jq && echo") and require an error, then assert the error message contains the same validation text (e.g. `must be an executable name` or mention of invalid characters). Keep the test structure (t.Parallel(), use LoadYAML, require.Error and assert.Contains) and place the new negative case alongside the existing path-like command assertion in TestLoadDAGToolsRejectsCommandPath so both rejects are covered.
🤖 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/core/spec/dag.go`:
- Around line 2445-2447: The validator isToolCommandName currently only blocks
path separators and whitespace but still accepts shell fragments like "jq;rm" or
"jq|cat"; update isToolCommandName to enforce a strict allowed-character set
(e.g. letters, digits and a small safe subset of punctuation such as . _ - +)
using a full-match check (anchor the pattern) or by iterating rune-by-rune and
rejecting any shell/operator characters such as ; | & $ ` > < ( ) ' " * ? [ ] {
} ~ # and control characters; modify the isToolCommandName function to perform
this full-match validation and return false on any disallowed rune so command
names cannot contain shell metacharacters or operators.
In `@internal/tools/aqua/installer.go`:
- Around line 95-100: The directory creation in installer.go uses os.MkdirAll
for paths.EnvDir and paths.RootDir with mode 0o750 which may be too restrictive
for shared CI/multi-user setups; update the mode to 0o755 for both os.MkdirAll
calls (or, if intentional single-user behavior is required, add a clear comment
above the os.MkdirAll calls documenting the single-user assumption and
rationale) to avoid permission denied errors when multiple users/processes need
access.
- Around line 138-163: The loop over cfg.Packages/pkg.Commands currently
resolves and writes every command into manifest.Commands, allowing later
packages to overwrite earlier ones; update the logic in the loop (around where
whichController.Which is called and before createCommandShim/manifest.Commands
assignment) to detect existing entries in manifest.Commands for the same command
name and fail or surface an error instead of silently overwriting (include the
conflicting package info: pkg.Package and pkg.Version and the already-registered
entry), so duplicate declarations are rejected and the caller can handle the
conflict; ensure you reference and check manifest.Commands[command] before
creating the shim or assigning tools.Command.
In `@internal/tools/aqua/manifest.go`:
- Around line 19-21: Replace the direct os.WriteFile(path, data, 0o600) call
with an atomic replace: create a temp file in the same directory (os.CreateTemp
/ ioutil.TempFile), write the manifest bytes to that temp file, fsync the file
(via file.Sync and optionally syscall.Fsync on Unix), close it, set the desired
permissions (0600) and then atomically rename it into place with
os.Rename(pathTemp, path); ensure you remove the temp file on error. Update the
write logic in internal/tools/aqua/manifest.go where os.WriteFile is used so
readers never observe a partially written/truncated manifest.
In `@internal/tools/tools.go`:
- Line 79: The current replacement only handles "/" and leaves other path-unsafe
characters in the platform string; add a helper function named
sanitizePlatform(platform string) that uses strings.NewReplacer to replace "/",
"\\", ":", " " (and any other path-unsafe characters you deem necessary) with
"-" and call sanitizePlatform(platform) instead of
strings.ReplaceAll(strings.TrimSpace(platform), "/", "-") so the platform
variable is fully sanitized before being used in filesystem paths.
---
Nitpick comments:
In `@internal/core/spec/dag_test.go`:
- Around line 173-192: Extend TestLoadDAGToolsRejectsCommandPath to also assert
that shell-metacharacter fragments are rejected: call LoadYAML with a
tools/packages entry where commands contains a token like "jq;echo" (or "jq &&
echo") and require an error, then assert the error message contains the same
validation text (e.g. `must be an executable name` or mention of invalid
characters). Keep the test structure (t.Parallel(), use LoadYAML, require.Error
and assert.Contains) and place the new negative case alongside the existing
path-like command assertion in TestLoadDAGToolsRejectsCommandPath so both
rejects are covered.
In `@internal/tools/aqua/config.go`:
- Line 23: The current append into effective.Packages copies ToolPackage structs
shallowly which leaves nested slices (e.g., ToolPackage.Commands) shared and
susceptible to mutation; update the code that sets effective.Packages (replace
the append of cfg.Packages) to allocate a new slice and deep-copy each
ToolPackage: for each pkg in cfg.Packages create a new ToolPackage value, copy
primitive fields, and clone any slice or pointer fields (at minimum duplicate
Commands with a new []string and copy its elements) before appending to
effective.Packages so mutations to effective do not affect cfg.Packages.
In `@internal/tools/tools.go`:
- Line 137: The code builds the env manifest path using a hardcoded
manifestFileName joined to manifest.EnvDir (EnvManifest + "=" +
filepath.Join(manifest.EnvDir, manifestFileName)), which can mismatch how the
manifest was originally written; update the code to derive the file path from
the manifest's recorded path instead of hardcoding—either add a ManifestFile
field to the Manifest struct or use the existing CacheLayout.ManifestFile (or
other canonical ManifestFile property) when available, and replace uses of
filepath.Join(manifest.EnvDir, manifestFileName) with that manifest.ManifestFile
(or cacheLayout.ManifestFile); ensure Manifest creation/installer sets and
validates this field so the constructed EnvManifest is always consistent with
where the manifest was actually written.
🪄 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: 3474b667-4f59-4de5-ae8e-f7b90fab841a
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (23)
go.modinternal/cmd/restart.gointernal/cmd/retry.gointernal/cmd/start.gointernal/cmd/start_tools.gointernal/cmd/start_tools_test.gointernal/cmn/schema/dag.schema.jsoninternal/core/dag.gointernal/core/spec/dag.gointernal/core/spec/dag_test.gointernal/core/tools.gointernal/runtime/builtin/command/command.gointernal/runtime/builtin/command/tools.gointernal/runtime/builtin/command/tools_test.gointernal/tools/aqua/config.gointernal/tools/aqua/config_test.gointernal/tools/aqua/installer.gointernal/tools/aqua/installer_integration_test.gointernal/tools/aqua/manifest.gointernal/tools/aqua/shim.gointernal/tools/aqua/shim_test.gointernal/tools/tools.gointernal/tools/tools_test.go
7 of 8 tasks
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 DAG-level
toolssupport backed by embedded aqua APIs so workflows can declare reproducible CLI dependencies without requiring a separate aqua binary.Changes
toolsparsing, validation, and JSON schema support for aqua packages.tools: ["jqlang/jq@jq-1.7.1"]while keeping the full object syntax for overrides.commandsoptional by inferring executable names from aqua registry metadata, while keeping explicitcommandsas an override.providerandregistry, pin the default aqua registry to an immutable commit SHA, and accept package commit refs where aqua supports them.1.26.3to resolve stdlib vulnerabilities reported bygovulncheck.Related Issues
N/A
Validation
go test -count=1 ./internal/core/spec ./internal/tools/aqua ./internal/cmn/schema ./internal/tools ./internal/runtime/builtin/command ./internal/cmdGOOS=windows GOARCH=amd64 go test -exec=true ./internal/core/spec ./internal/tools/aqua ./internal/tools ./internal/runtime/builtin/command ./internal/cmdDAGU_AQUA_INTEGRATION=1 go test ./internal/tools/aqua -run TestInstallerInstallIntegration -count=1jqlang/jq@jq-1.7.1with omittedprovider,registry, andcommands.Checklist