feat: Docker connector, artifact system, and site improvements#28
Merged
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Registers custom string member overloads via cel.Lib and wires them into NewEvaluator through a central customFunctions() registration point. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Covers three patterns — CEL-only structural transforms, AI-powered semantic transforms, and hybrid workflows that combine both. Includes a decision guide and a quick-reference table of all available CEL extension functions. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ests (#14) - default(): fall through to rhs when lhs is null (types.NullValue) - parseTimestamp(): try RFC3339Nano, bare ISO date, US date, and named-month formats before erroring - expressions.md: remove undeclared `now` variable from two examples - data-transformations.md: add missing exists_one row to list-macros table - data-transform-api-to-db.yaml: simplify to single-user fetch+insert (fixes broken batch param shape) - functions_test.go: add null/fallback tests for default(), edge-case tests for flatten(), and new format tests for parseTimestamp() Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…, docs accuracy Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…cs accuracy Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…leanup
- jsonDecode: replace dec.More() with dec.Decode(&trailing) EOF check
to catch all trailing data (e.g., "{}]", "1}")
- Add trailing_bracket and trailing_brace regression tests
- Update parseTimestamp description in data-transformations.md to list
all accepted formats
- Plan: rename TestFunc_Timestamp to TestFunc_ParseTimestamp, add
golangci-lint to final validation checklist
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- normalizeJSONNumbers: only use float64 for decimals/exponents, preserve overflow integers as strings to avoid silent precision loss - flatten: initialize result as make([]any, 0) so empty lists return non-nil empty slice - Add large_integer_preserved test for jsonDecode - Plan: remove dead split() variable, replace variadic obj() with fixed-arity overloads, add jsonDecode EOF check, fix timestamp refs - data-transformations.md: note obj() 5-pair limit Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
These internal process docs (specs, plans) are already in .gitignore but were force-added during development. Files remain on disk but are no longer tracked by git. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add Docker connector card to connectors grid, update count to 8/10 - Update hero section connector count - Add Docker to built-in connector list in plugins guide - Fix bottom nav visible on tablets where top nav already shows (lg:hidden → md:hidden) - Fix nav link overlap at tablet sizes (gap-8 → gap-4 lg:gap-8) - Delay Get Started button to md breakpoint with tighter padding - Sync bottom padding and sidebar offset to md breakpoint Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add ArtifactDecl type to declare files produced by a step - Add ArtifactRef type for runtime representation in CEL expressions - Add registry_credential field to Step for container registry auth - Add artifacts field to Step for artifact declarations - Add test case for parsing artifacts and registry credentials Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ontext helpers Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add support for accessing artifact metadata in CEL expressions:
- Add Artifacts field to Context struct for {name, url, size}
- Register artifacts variable in CEL environment as a map
- Initialize artifacts to empty map if nil during evaluation
- Add test for accessing artifact URL and size properties
Workflow authors can now reference artifacts via artifacts['name'].url
in step parameters, enabling dynamic artifact references.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Implements Store with Create, GetByName, ListByExecution, DeleteByExecution, and ListExpired operations backed by the execution_artifacts table (migration 013). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Handle filepath.Abs errors in FilesystemTmpStorage.DeleteByPrefix - Fix artifact reaper over-counting on partial file deletion failure - Extract loadArtifactsIntoCELContext helper, log errors instead of silencing - Return error on invalid memory limit in docker/run - Log stdcopy.StdCopy errors in docker/run - Dynamic credential type list in GetType error message - Add missing error checks in tests - Fix idiomatic boolean check in docker_test.go Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Fix Hero.astro connector count to match Connectors.astro (8 not 10) - Make volume backup notification conditions mutually exclusive Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a Docker connector and a complete artifact subsystem: tmp storage (filesystem impl), DB-backed artifact store and reaper, engine integration to mount/persist artifacts and expose them to CEL, workflow schema/validation for artifacts, migrations, examples, tests, and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant Workflow
participant Engine
participant Docker as "docker/run Connector"
participant Tmp as "TmpStorage"
participant Store as "ArtifactStore"
participant CEL as "CEL Evaluator"
Workflow->>Engine: Start step (may declare artifacts, registry_credential)
Engine->>Engine: Create scratch dir\nartifact.WithArtifactsDir(ctx)
Engine->>Docker: Execute step (context includes artifacts dir)
Docker->>Docker: Pull image, create & run container (may mount /mantle/artifacts)
Docker-->>Engine: Return {exit_code, stdout, stderr}
alt success and artifacts declared
Engine->>Engine: Verify declared files exist in scratch dir
Engine->>Tmp: Put artifact files -> upload (returns URL + size)
Tmp-->>Engine: URL + size
Engine->>Store: Create artifact metadata rows
Store-->>Engine: Persisted
end
Engine->>Store: ListByExecution to load artifacts
Store-->>Engine: Return artifact refs
Engine->>CEL: Populate `artifacts` variable with refs
CEL-->>Workflow: Downstream steps read artifacts['name'].url/size
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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)
Comment |
Deploying mantle with
|
| Latest commit: |
25edd31
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ec693faf.mantle-cxy.pages.dev |
| Branch Preview URL: | https://feat-data-transformation-cel.mantle-cxy.pages.dev |
…credential docs - Add getting-started/docker-workflows.md with examples for stdin, env, resource limits, exit code branching, private images, volume mounts - Add concepts/artifacts.md explaining the artifact system, tmp storage, CEL access, retention, and scope - Add docker and aws credential types to secrets-guide.md - Add tmp storage config fields to configuration.md (config table + env vars) - Add Data Transformations, Docker Workflows, and Artifacts to docs nav Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Keep artifacts CEL variable addition - Keep responsive nav gap fix (gap-4 lg:gap-8) with updated comment from main Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Replace deprecated ImageInspectWithRaw with ImageInspect - Add #nosec G118 for intentional context.Background() in stop goroutine Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add missing doc comments to Store.Create, Store.GetByName, Store.ListByExecution, FilesystemTmpStorage.Put, FilesystemTmpStorage.DeleteByPrefix, and DockerRunConnector.Execute to reach 80% docstring coverage on PR-modified files. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![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: 20
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/docker-data-transform.yaml`:
- Around line 22-24: The example uses steps['fetch-records'].output.json but the
postgres/query connector returns rows and row_count for SELECTs, so stdin should
reference steps['fetch-records'].output.rows (or encode rows to JSON via a CEL
function like json_encode) and/or adjust the jq invocation to consume the rows
array directly; update the stdin field and/or the jq cmd to use
steps['fetch-records'].output.rows (or
json_encode(steps['fetch-records'].output.rows)) so the pipeline reads the
actual SELECT output from the postgres/query step.
In `@examples/docker-volume-backup.yaml`:
- Around line 47-53: The failure notifier never runs because the engine halts on
the first failed step; update the workflow so notify-failure can execute after
errors by (1) making the upstream steps ("upload-to-s3" and/or "backup-volume")
continue on error (e.g., set their continue_on_error / allow_failure flag) so
the pipeline does not stop on failure, and (2) change the notify-failure
condition to check both steps' error fields (for example, if:
"steps['upload-to-s3'].error != null || steps['backup-volume'].error != null")
or mark notify-failure to always run/on_failure depending on the engine,
ensuring notify-failure executes when either step fails.
In `@internal/artifact/reaper_test.go`:
- Around line 63-67: The test currently discards the error from
store.ListByExecution; change the call to capture the error (e.g., arts, err :=
store.ListByExecution(ctx, execID)) and assert it is nil before asserting
len(arts) == 0 (use t.Fatalf or t.Fatalf-like check on err != nil), then proceed
to validate the zero-length result; reference ListByExecution and the local
variables arts/err in reaper_test.go.
In `@internal/artifact/reaper.go`:
- Around line 39-70: The code currently calls r.Store.DeleteByExecution(ctx,
execID) which removes metadata for the entire execution; instead, collect the
specific artifact IDs whose files were successfully removed and delete only
those rows from the DB (e.g., call r.Store.DeleteByIDs(ctx, ids) or
r.Store.Delete(ctx, artifactID) per-artifact), skipping DB deletion for
artifacts whose file delete failed; update cleaned counting to use the number of
successfully deleted artifacts and log failures per-artifact (use symbols:
expired, byExecution, r.TmpStorage.DeleteByPrefix, fileDeleteFailed,
r.Store.DeleteByExecution — replace the latter with DeleteByIDs/Delete or
Delete(ctx, artifactID) and only delete the IDs that succeeded).
In `@internal/artifact/store_test.go`:
- Around line 131-133: The current test only checks for "artifact create" in
err.Error(), which can match unrelated failures; update the assertion in the
test (the err variable in the failing case inside the artifact creation test) to
assert duplicate-name semantics as well—either by checking for a
database-specific uniqueness message (e.g., strings.Contains(err.Error(),
"duplicate") or "duplicate key" or "UNIQUE constraint failed") or by
comparing/using a sentinel error (e.g., ErrAlreadyExists) if the
store.CreateArtifact or Create method returns one; ensure the assertion requires
both the artifact-create context and the uniqueness indicator so the test only
passes for true duplicate-name failures.
In `@internal/artifact/store.go`:
- Around line 89-95: The code uses time.Now() to compute cutoff which can
introduce clock skew; remove the cutoff := time.Now().Add(-olderThan) and
instead move cutoff calculation into the SQL query by using the database clock
(e.g. WHERE created_at < (NOW() - $1::interval)). Update the s.DB.QueryContext
call in the function that queries execution_artifacts (remove the cutoff
variable and its usage) and pass olderThan as a string (olderThan.String()) or
otherwise convert to an interval-acceptable value so the DB can subtract it from
NOW() (ensure parameter binding matches $1::interval).
In `@internal/artifact/test_helpers_test.go`:
- Around line 31-33: The test currently unconditionally calls t.Skipf when the
Postgres container fails to start, which can hide CI failures; update the error
handling in the test helper so that it only skips locally and fails in CI:
detect CI via an environment flag (e.g. os.Getenv("CI") or a similar project
convention) and replace the unconditional t.Skipf with conditional logic that
calls t.Fatalf (or t.Fatalf-equivalent) when running in CI and t.Skipf when not
in CI, keeping the existing error message and referencing the existing err check
and t.Skipf/t.Fatalf call sites in internal/artifact/test_helpers_test.go.
In `@internal/artifact/tmp_test.go`:
- Line 54: The call to fs.Put in tmp_test.go discards its error (url, _ :=
fs.Put(...)); capture the returned error (e.g., url, err := fs.Put(ctx, key,
srcPath)) and fail the test immediately if err != nil (t.Fatalf or t.Fatal) so
the subsequent DeleteByPrefix assertion doesn't pass spuriously when Put failed;
update the test that exercises DeleteByPrefix to check the Put error before
proceeding.
In `@internal/artifact/tmp.go`:
- Around line 27-49: In FilesystemTmpStorage.Put, after computing destPath :=
filepath.Join(fs.BasePath, key) resolve both fs.BasePath and destPath to
absolute, cleaned paths (using filepath.Abs and filepath.Clean or filepath.Join
followed by Abs), then verify containment (e.g., use filepath.Rel(baseAbs,
destAbs) and ensure it does not start with ".." and is not equal to ".."); if
the check fails return an error before calling os.MkdirAll or os.Create to
prevent writes outside fs.BasePath. Ensure this validation happens prior to the
existing MkdirAll/open/create/copy operations in Put.
- Around line 52-64: The containment check in
FilesystemTmpStorage.DeleteByPrefix is vulnerable because strings.HasPrefix on
absolute paths can be bypassed; replace the prefix check with a
path-segment-aware validation using filepath.Rel: compute rel, ensure no error
and that rel does not start with ".." (or equal ".."), and only proceed with
deletion if the target is contained within fs.BasePath; update error messages
accordingly in DeleteByPrefix to reflect a containment validation failure.
In `@internal/connector/docker.go`:
- Around line 179-189: The credential handling in the Docker connector only
applies the "host" field and omits TLS fields and registry creds; update the
client option construction in the block that builds clientOpts (the clientOpts
slice and the params["_credential"] handling) to consume ca_cert, client_cert
and client_key and pass them into the Docker client (e.g., construct a
tls.Config or appropriate client.WithTLSClientConfig/WithCredentials options) so
TCP+TLS daemons are supported, and also locate where image pulls occur and wire
params["_registry_credential"] (and its username/password/token) into the image
pull/auth configuration so private registries are authenticated rather than
anonymous (ensure you still delete params["_credential"]/_registry_credential
after consuming them).
- Around line 350-366: The io.Copy drain error after ImagePull is currently
ignored; update the ImagePull handling in the docker.run logic so that after
obtaining reader and deferring reader.Close() you check the result of
io.Copy(io.Discard, reader) and return a wrapped error (e.g.,
fmt.Errorf("docker/run: draining pull output: %w", err)) if it fails, and
otherwise return nil; locate the ImagePull block around the pull policy switch
(references: cfg.Pull, cli.ImagePull, reader) and replace the ignored io.Copy
with a checked-and-returned error.
In `@internal/db/migrations/013_execution_artifacts.sql`:
- Around line 8-10: The size column currently allows negative values; add a
DB-level check to enforce non-negative sizes by updating the column definition
or adding a table constraint for size (e.g., a CHECK (size >= 0) constraint)
near the existing "size BIGINT NOT NULL" and "UNIQUE (execution_id, name)"
entries; give the constraint a clear name like
execution_artifacts_size_nonnegative_chk and, if this migration may already be
applied in some environments, create a follow-up migration that runs ALTER TABLE
to add the CHECK constraint instead of modifying already-applied SQL.
- Line 13: The migration is creating a redundant index
idx_execution_artifacts_execution_id on execution_artifacts(execution_id) even
though the UNIQUE (execution_id, name) constraint already provides an index for
WHERE execution_id = ... lookups; remove the CREATE INDEX
idx_execution_artifacts_execution_id statement from the migration (or replace it
with a safe DROP INDEX IF EXISTS idx_execution_artifacts_execution_id if this
migration has already been applied in some environments) so only the UNIQUE
(execution_id, name) index remains and write overhead is reduced.
In `@internal/engine/engine.go`:
- Around line 146-147: The CEL artifacts are only loaded once via
e.loadArtifactsIntoCELContext(ctx, execID, celCtx) before the step loop, causing
celCtx.Artifacts to be stale for downstream steps; update the
Execute/resumeExecution flow to refresh artifacts before resolving each step's
params by calling e.loadArtifactsIntoCELContext(ctx, execID, celCtx) (or an
equivalent refresh helper) inside the per-step loop (i.e., before using
celCtx.Artifacts to evaluate params/expressions and before invoking the step
executor), and ensure the same fix is applied to both Execute and
resumeExecution code paths that resolve params so downstream artifact references
see newly produced artifacts.
- Around line 516-528: The code uploads an artifact via e.TmpStorage.Put and
then calls e.ArtifactStore.Create; if Create fails the blob remains orphaned.
Modify the error path after calling e.ArtifactStore.Create in the function
containing TmpStorage.Put and ArtifactStore.Create so that when createErr != nil
you attempt to roll back/delete the uploaded tmp object (using
e.TmpStorage.Delete or the appropriate removal method with ctx and the key or
url returned by TmpStorage.Put) before returning the fmt.Errorf for recording
the artifact; ensure any error from the delete is logged or wrapped but does not
mask the original createErr.
- Around line 505-529: The artifact persistence path (calls to e.TmpStorage.Put
and e.ArtifactStore.Create for each artDecl in step.Artifacts) must emit an
audit event via the engine's AuditEmitter after the artifact is stored and
recorded; add a nil-safe check for e.AuditEmitter and call its emit method with
a concise audit payload (ExecutionID/execID, StepName/step.Name,
ArtifactName/artDecl.Name, URL returned from Put, Size from info.Size(), and an
operation like "artifact.persisted"); ensure the emit error is handled
non-fatally (log a warning rather than aborting the whole operation) so the
artifact flow succeeds even if audit emission fails.
In `@internal/secret/types.go`:
- Around line 80-88: The docker credential schema declares TLS fields (ca_cert,
client_cert, client_key) but the Docker connector ignores them; update the
connector code that currently calls client.WithHost(...) to read those fields
from the credentials and configure TLS when any of
ca_cert/client_cert/client_key are present: build a tls.Config (load CA into a
CertPool from ca_cert, load client cert+key into a tls.Certificate from
client_cert and client_key), apply it to the docker client transport or use the
appropriate TLS option on the client initialization (the function that
constructs the Docker client where client.WithHost is used), and handle
parsing/validation errors; alternatively, if you don’t want TLS now, remove
ca_cert/client_cert/client_key from the docker credential type definition to
avoid silently dropped fields.
In `@site/src/content/docs/getting-started/docker-workflows.md`:
- Around line 136-158: The docs claim registry_credential works but the
connector never uses it; update the connector to extract the registry credential
passed from the engine and send it to Docker when pulling images: modify Execute
in internal/connector/docker.go to accept/forward the registry credential
parameter to pullImage, and change pullImage to build an image.AuthConfig
(username/password or token), JSON-encode and base64-encode it, then pass it in
types.ImagePullOptions{RegistryAuth: ...} to cli.ImagePull so private registry
pulls authenticate correctly; alternatively, if you prefer not to implement,
remove the registry_credential mention from the docs.
In `@site/src/content/docs/workflow-reference/connectors.md`:
- Line 473: The docs claim a registry_credential for private image pulls but
internal/connector/docker.go's pullImage does not pass auth to Docker's
ImagePull; update pullImage to accept and translate the
registry_credential/basic credential into the Docker API auth configuration
(e.g., encode username/password into the auth payload) and supply it to
ImagePull, ensure the connector reads registry_credential from the credential
structure, and add tests exercising private registry pulls to validate the auth
is sent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b036ba4f-40c6-442d-9004-ae8f957c9073
📒 Files selected for processing (39)
examples/docker-data-transform.yamlexamples/docker-volume-backup.yamlgo.modinternal/artifact/context.gointernal/artifact/reaper.gointernal/artifact/reaper_test.gointernal/artifact/store.gointernal/artifact/store_test.gointernal/artifact/test_helpers_test.gointernal/artifact/tmp.gointernal/artifact/tmp_test.gointernal/cel/cel.gointernal/cel/cel_test.gointernal/config/config.gointernal/config/config_test.gointernal/connector/connector.gointernal/connector/docker.gointernal/connector/docker_test.gointernal/db/migrations/013_execution_artifacts.sqlinternal/engine/engine.gointernal/secret/types.gointernal/secret/types_test.gointernal/workflow/parse_test.gointernal/workflow/validate.gointernal/workflow/validate_test.gointernal/workflow/workflow.gosite/src/components/Connectors.astrosite/src/components/Hero.astrosite/src/components/Nav.astrosite/src/content/docs/concepts/artifacts.mdsite/src/content/docs/configuration.mdsite/src/content/docs/getting-started/docker-workflows.mdsite/src/content/docs/plugins-guide.mdsite/src/content/docs/secrets-guide.mdsite/src/content/docs/workflow-reference/connectors.mdsite/src/data/docs-nav.tssite/src/layouts/Docs.astrosite/src/pages/index.astrosite/src/styles/global.css
Comment on lines
+47
to
+53
| - name: notify-failure | ||
| action: slack/send | ||
| credential: slack-token | ||
| if: "steps['upload-to-s3'].error != null" | ||
| params: | ||
| channel: "#ops-alerts" | ||
| text: "Volume backup FAILED — {{ steps['upload-to-s3'].error }}" |
Contributor
There was a problem hiding this comment.
Failure notifications are unreachable in this example.
The engine stops on the first failed step, so notify-failure never runs after an upload-to-s3 or backup-volume error. As written, this example only demonstrates success alerting and misses the failure path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/docker-volume-backup.yaml` around lines 47 - 53, The failure
notifier never runs because the engine halts on the first failed step; update
the workflow so notify-failure can execute after errors by (1) making the
upstream steps ("upload-to-s3" and/or "backup-volume") continue on error (e.g.,
set their continue_on_error / allow_failure flag) so the pipeline does not stop
on failure, and (2) change the notify-failure condition to check both steps'
error fields (for example, if: "steps['upload-to-s3'].error != null ||
steps['backup-volume'].error != null") or mark notify-failure to always
run/on_failure depending on the engine, ensuring notify-failure executes when
either step fails.
Comment on lines
+505
to
+529
| // Persist declared artifacts to tmp storage. | ||
| if lastErr == nil && len(step.Artifacts) > 0 && e.TmpStorage != nil && e.ArtifactStore != nil && artifactsDir != "" { | ||
| for _, artDecl := range step.Artifacts { | ||
| relPath := filepath.Base(artDecl.Path) | ||
| localPath := filepath.Join(artifactsDir, relPath) | ||
| info, statErr := os.Stat(localPath) | ||
| if statErr != nil { | ||
| return nil, fmt.Errorf("declared artifact %q not found at %q: %v", artDecl.Name, localPath, statErr) | ||
| } | ||
|
|
||
| key := fmt.Sprintf("%s/%s/%s/%s", workflowName, execID, artDecl.Name, relPath) | ||
| url, putErr := e.TmpStorage.Put(ctx, key, localPath) | ||
| if putErr != nil { | ||
| return nil, fmt.Errorf("persisting artifact %q: %v", artDecl.Name, putErr) | ||
| } | ||
|
|
||
| if createErr := e.ArtifactStore.Create(ctx, &artifact.Artifact{ | ||
| ExecutionID: execID, | ||
| StepName: step.Name, | ||
| Name: artDecl.Name, | ||
| URL: url, | ||
| Size: info.Size(), | ||
| }); createErr != nil { | ||
| return nil, fmt.Errorf("recording artifact %q: %v", artDecl.Name, createErr) | ||
| } |
Contributor
There was a problem hiding this comment.
Artifact persistence needs an audit event.
This block writes to tmp storage and inserts execution_artifacts rows without emitting through AuditEmitter.
As per coding guidelines, internal/engine/**/*.go: "Emit audit events for every state-changing operation via the AuditEmitter interface".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/engine/engine.go` around lines 505 - 529, The artifact persistence
path (calls to e.TmpStorage.Put and e.ArtifactStore.Create for each artDecl in
step.Artifacts) must emit an audit event via the engine's AuditEmitter after the
artifact is stored and recorded; add a nil-safe check for e.AuditEmitter and
call its emit method with a concise audit payload (ExecutionID/execID,
StepName/step.Name, ArtifactName/artDecl.Name, URL returned from Put, Size from
info.Size(), and an operation like "artifact.persisted"); ensure the emit error
is handled non-fatally (log a warning rather than aborting the whole operation)
so the artifact flow succeeds even if audit emission fails.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
internal/artifact/tmp.go (2)
54-67:⚠️ Potential issue | 🔴 CriticalString prefix check in
DeleteByPrefixis bypassable via sibling path names.The check
strings.HasPrefix(absTarget, absBase)at line 64 uses string-level comparison rather than path-segment-aware validation. A path like/var/tmp-base-evilpasses the check against base/var/tmp-basebecause it's a string prefix match, allowing deletion of unintended sibling directories.Use
filepath.Rel()for proper containment validation:🔒 Proposed fix using filepath.Rel
func (fs *FilesystemTmpStorage) DeleteByPrefix(ctx context.Context, prefix string) error { target := filepath.Join(fs.BasePath, prefix) absBase, err := filepath.Abs(fs.BasePath) if err != nil { return fmt.Errorf("resolving base path: %w", err) } absTarget, err := filepath.Abs(target) if err != nil { return fmt.Errorf("resolving target path: %w", err) } - if !strings.HasPrefix(absTarget, absBase) { + rel, err := filepath.Rel(absBase, absTarget) + if err != nil || strings.HasPrefix(rel, "..") || rel == ".." { return fmt.Errorf("prefix escapes base path") } - return os.RemoveAll(target) + return os.RemoveAll(absTarget) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/artifact/tmp.go` around lines 54 - 67, The containment check in DeleteByPrefix uses strings.HasPrefix which can be bypassed by sibling names; replace it with a filepath.Rel-based containment check: compute rel, err := filepath.Rel(fs.BasePath, absTarget), ensure err == nil and that rel does not start with ".." (or equal "..") and is not an absolute path, and only then proceed to os.RemoveAll(target); continue to resolve absBase and absTarget as already done and return appropriate wrapped errors if containment fails.
27-51:⚠️ Potential issue | 🔴 CriticalPath traversal vulnerability in
Putallows writes outside BasePath.The
filepath.Join(fs.BasePath, key)at line 29 resolves..segments without validating that the resulting path stays withinBasePath. Sincekeyis constructed from workflow inputs (workflowName, execID, artDecl.Name, relPath perengine.go:506-519), a malicious key like../../../etc/passwdwould allow writing to arbitrary filesystem locations.Add path containment validation before any filesystem operations:
🔒 Proposed fix with path containment check
func (fs *FilesystemTmpStorage) Put(ctx context.Context, key string, localPath string) (string, error) { destPath := filepath.Join(fs.BasePath, key) + absBase, err := filepath.Abs(fs.BasePath) + if err != nil { + return "", fmt.Errorf("resolving base path: %w", err) + } + absDest, err := filepath.Abs(destPath) + if err != nil { + return "", fmt.Errorf("resolving dest path: %w", err) + } + rel, err := filepath.Rel(absBase, absDest) + if err != nil || strings.HasPrefix(rel, "..") { + return "", fmt.Errorf("key escapes base path") + } if err := os.MkdirAll(filepath.Dir(destPath), 0755); err != nil { return "", fmt.Errorf("creating artifact dir: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/artifact/tmp.go` around lines 27 - 51, The Put method (FilesystemTmpStorage.Put) currently joins fs.BasePath and key into destPath and proceeds to create directories and files, which allows path traversal via keys like ../../..; fix this by sanitizing and validating the resolved destination before any filesystem calls: compute the cleaned/absolute base (fs.BasePath) and the cleaned/absolute destination (filepath.Join(fs.BasePath, key) then filepath.Clean/Abs), then verify containment (e.g., use filepath.Rel(baseAbs, destAbs) and ensure it does not start with ".." or an error) and only then call os.MkdirAll, os.Create and io.Copy; apply the check to abort with an error if the destination is outside fs.BasePath.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/artifact/store.go`:
- Around line 27-35: The Create method on Store currently executes an INSERT
without capturing DB-generated fields; modify Store.Create to use a RETURNING
clause and QueryRowContext (or QueryRow) to scan the generated ID and CreatedAt
back into the provided *Artifact (e.g., set a.ID and a.CreatedAt after
insertion). Locate the Create function and the Artifact type fields (ID,
CreatedAt) and update the DB call to return and populate those fields and still
wrap/return any error as before.
In `@internal/connector/docker.go`:
- Around line 120-144: In parseMemoryString, after parsing the numeric portion
into n in function parseMemoryString, validate that n is >= 0 and return a
descriptive error (e.g., "negative memory value %q") if it's negative before
multiplying by multiplier; this ensures inputs like "-256m" are rejected rather
than silently accepted and avoids passing negative byte values to Docker.
- Around line 74-87: The loop that builds docker mounts from params["mounts"]
currently appends dockerMount entries even when dm.Source or dm.Target are
empty, causing Docker API errors; update the logic in the block that iterates
params["mounts"] (the code creating dockerMount and appending to cfg.Mounts) to
validate that both dm.Source and dm.Target are non-empty (e.g., non-zero length
after trimming) before appending, and skip (or log/return an error) any mount
entries missing either field; keep existing ReadOnly handling and only append
valid dockerMount structs to cfg.Mounts.
- Around line 280-284: The goroutine that pipes stdin to the container ignores
errors from io.Copy; change it to capture the returned error (err :=
io.Copy(attach.Conn, strings.NewReader(cfg.Stdin))) and if err != nil, log the
error with context (e.g., "failed writing stdin to container" and include err)
before proceeding to call attach.CloseWrite() and attach.Close(); ensure you
still defer attach.Close() and perform attach.CloseWrite() in a safe order so
the connection is always closed even on error.
- Around line 306-316: The select reading from ContainerWait's channels
(statusCh, errCh) can miss an error delivered after reading the other channel;
update the handling around ContainerWait (statusCh, errCh) so that after you
receive a status you also perform a non-blocking read on errCh (or check errCh
in a separate select with default) and return any error instead of ignoring it;
ensure the code still sets exitCode from status.StatusCode and returns the
composite error (wrapping with the existing "docker/run: waiting for container"
message) if errCh yields an error after status is observed.
---
Duplicate comments:
In `@internal/artifact/tmp.go`:
- Around line 54-67: The containment check in DeleteByPrefix uses
strings.HasPrefix which can be bypassed by sibling names; replace it with a
filepath.Rel-based containment check: compute rel, err :=
filepath.Rel(fs.BasePath, absTarget), ensure err == nil and that rel does not
start with ".." (or equal "..") and is not an absolute path, and only then
proceed to os.RemoveAll(target); continue to resolve absBase and absTarget as
already done and return appropriate wrapped errors if containment fails.
- Around line 27-51: The Put method (FilesystemTmpStorage.Put) currently joins
fs.BasePath and key into destPath and proceeds to create directories and files,
which allows path traversal via keys like ../../..; fix this by sanitizing and
validating the resolved destination before any filesystem calls: compute the
cleaned/absolute base (fs.BasePath) and the cleaned/absolute destination
(filepath.Join(fs.BasePath, key) then filepath.Clean/Abs), then verify
containment (e.g., use filepath.Rel(baseAbs, destAbs) and ensure it does not
start with ".." or an error) and only then call os.MkdirAll, os.Create and
io.Copy; apply the check to abort with an error if the destination is outside
fs.BasePath.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4ff9440a-87dc-45c5-96dd-81aa1f247776
📒 Files selected for processing (3)
internal/artifact/store.gointernal/artifact/tmp.gointernal/connector/docker.go
Collaborator
Author
|
Re: Filed as #29. The example now includes a comment noting this limitation and will work correctly once #29 is implemented. |
Artifact system: - Use DB clock (NOW() - interval) in ListExpired instead of app clock - Return generated ID/CreatedAt from Create via RETURNING clause - Add DeleteByID for per-artifact reaper cleanup - Reaper now deletes per-artifact instead of per-execution batch - Add path traversal protection to FilesystemTmpStorage.Put - Use filepath.Rel for stronger containment check in DeleteByPrefix - Add CHECK (size >= 0) to migration, remove redundant index - Fix missing error checks in tests - Fail in CI instead of skip when Postgres container fails Docker connector: - Wire TLS credentials (ca_cert, client_cert, client_key) into client - Wire registry_credential into pullImage for private registry auth - Check io.Copy drain error after ImagePull - Reject negative memory values in parseMemoryString - Validate mount source/target are non-empty - Log stdin pipe errors - Check errCh after status in ContainerWait select Engine: - Refresh artifacts in CEL context after each step completes - Clean up orphaned blob when ArtifactStore.Create fails - Emit audit event (artifact.persisted) after artifact storage Examples: - Fix postgres output field reference (output.json → output.rows) - Note continue_on_error limitation in volume backup (see #29) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/artifact/reaper_test.go`:
- Around line 73-90: The test TestReaper_SkipsWhenRetentionZero currently
constructs Store as &Store{} which may cause future nil-pointer panics if
Sweep’s early-return logic changes; update the test to provide a safe, real or
mocked Store instance (e.g., an in-memory or test helper Store) instead of a raw
empty struct so Reaper.Sweep has a valid Store object to call if retention logic
changes; locate the test and replace the Store creation used in the Reaper
struct (reference: TestReaper_SkipsWhenRetentionZero, Reaper.Sweep, Store) with
a lightweight safe implementation or explicit comment that a real Store is
intended.
In `@internal/artifact/reaper.go`:
- Around line 39-57: The reaper currently calls r.TmpStorage.DeleteByPrefix(ctx,
a.URL) using the stored full filesystem path (a.URL); instead reconstruct the
relative storage key prefix from the artifact metadata (e.g. workflow name,
a.ExecutionID and a.Name) and pass that relative key to
r.TmpStorage.DeleteByPrefix so filepath.Join(fs.BasePath, prefix) behaves
correctly. Locate the loop that calls r.TmpStorage.DeleteByPrefix and replace
the a.URL argument with the reconstructed key string (matching how orphan
cleanup builds the key in engine.go), then keep the subsequent
r.Store.DeleteByID call and logging unchanged.
In `@internal/artifact/tmp.go`:
- Around line 54-62: When io.Copy(dst, src) fails in the Put flow (the snippet
using dst, src and destPath), ensure you remove the partially written
destination file before returning: call os.Remove(destPath) on copy-path failure
(handle/remove any error from Remove appropriately or log it) prior to returning
the wrapped fmt.Errorf; keep the existing dst.Close defer. This avoids leaving
orphaned blobs under BasePath when metadata insertion never happens.
In `@internal/connector/docker.go`:
- Around line 192-240: The code is mutating the caller-owned params map by
calling delete(params, "_credential") and delete(params,
"_registry_credential"), causing lost credentials across retries in
executeStepLogic/resolvedParams; instead, avoid mutating params — either work on
a shallow copy (e.g., copy the map into a local variable) or read cred :=
params["_credential"].(map[string]string) and regCred :=
params["_registry_credential"].(map[string]string) without deleting keys; update
the docker client setup code around the params handling (the blocks that build
clientOpts, call client.NewClientWithOpts, and construct registryAuth) to use
the local copy or non-mutating reads so retries keep original credentials.
- Around line 145-153: The current parsing of memory strings in the function
that uses fmt.Sscanf(s, "%d", &n) allows trailing garbage (e.g., "256mb" ->
"256b") because Sscanf doesn't require full consumption; replace that parsing
with strconv.ParseInt on the remaining string (use base 10, 64-bit) so the parse
will fail if any non-digit characters remain, keep the existing negative check
(n < 0) and then return n * multiplier; update error messages to include the
original input s on parse failures and ensure you reference the same variables
(s, n, multiplier) and the same return signature so callers remain unchanged.
- Around line 281-287: Artifact bind mounts fail against remote Docker daemons
because the daemon resolves the host path; in the artifact mount block using
artifact.ArtifactsDirFromContext(ctx) and appending to hostCfg.Mounts with
mount.Mount you must detect a remote daemon and avoid bind mounts there: update
the code to check the Docker host (e.g., whether the client is using a non-unix
socket / DOCKER_HOST != unix path) and if remote either (A) reject/return an
explicit error/log explaining artifact mounts are unsupported for remote
daemons, or (B) remove the mount logic and implement copy-in/copy-out fallback
using the Docker client’s CopyToContainer/CopyFromContainer around container
lifecycle (copy local dir created by os.MkdirTemp in engine.go into
/mantle/artifacts before start and copy results out after), ensuring
hostCfg.Mounts is only mutated for local/unix-socket daemons.
In `@internal/engine/engine.go`:
- Around line 509-510: Replace the use of filepath.Base on artDecl.Path so
nested paths are preserved: compute a path relative to the artifacts root (use
filepath.Rel or equivalent on artDecl.Path vs the artifacts root) validate that
the resulting relative path is not escaping the artifactsDir (not absolute and
does not start with ".."), then join artifactsDir with that validated relative
path to form localPath; update the code locations referencing relPath/localPath
(including the second occurrence at line ~516) to use these validated relative
paths instead of collapsing to a basename.
- Around line 507-533: Track artifacts successfully persisted during the loop
(after e.TmpStorage.Put and e.ArtifactStore.Create) in a slice; if any
subsequent Stat/Put/Create fails, iterate that slice to rollback both metadata
and storage by calling the artifact store deletion API for each recorded
artifact (the inverse of e.ArtifactStore.Create) and calling
e.TmpStorage.DeleteByPrefix(ctx, key) (or the appropriate TmpStorage delete) for
each stored blob, logging any cleanup errors, then return the original error;
update the loop around step.Artifacts to push successful artifact records/keys
into this slice and perform the rollback on any failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e4255478-bf8d-45ac-b4a0-874688f293cf
📒 Files selected for processing (13)
examples/docker-data-transform.yamlexamples/docker-volume-backup.yamlinternal/artifact/reaper.gointernal/artifact/reaper_test.gointernal/artifact/store.gointernal/artifact/store_test.gointernal/artifact/test_helpers_test.gointernal/artifact/tmp.gointernal/artifact/tmp_test.gointernal/audit/audit.gointernal/connector/docker.gointernal/db/migrations/013_execution_artifacts.sqlinternal/engine/engine.go
Comment on lines
+73
to
+90
| func TestReaper_SkipsWhenRetentionZero(t *testing.T) { | ||
| dir := t.TempDir() | ||
| fs := &FilesystemTmpStorage{BasePath: dir} | ||
|
|
||
| reaper := &Reaper{ | ||
| Store: &Store{}, // won't be called | ||
| TmpStorage: fs, | ||
| Retention: 0, | ||
| } | ||
|
|
||
| cleaned, err := reaper.Sweep(context.Background()) | ||
| if err != nil { | ||
| t.Fatalf("Sweep: %v", err) | ||
| } | ||
| if cleaned != 0 { | ||
| t.Errorf("cleaned = %d, want 0 when retention is 0", cleaned) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider making the zero-retention test more robust.
The test uses &Store{} with a nil DB, which works because Sweep returns early when Retention <= 0. However, this could lead to a confusing nil-pointer panic if the implementation changes.
♻️ Optional: use a real store or document the intent
func TestReaper_SkipsWhenRetentionZero(t *testing.T) {
dir := t.TempDir()
fs := &FilesystemTmpStorage{BasePath: dir}
reaper := &Reaper{
- Store: &Store{}, // won't be called
+ Store: nil, // explicitly nil; Sweep must return early before Store access
TmpStorage: fs,
Retention: 0,
}📝 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
| func TestReaper_SkipsWhenRetentionZero(t *testing.T) { | |
| dir := t.TempDir() | |
| fs := &FilesystemTmpStorage{BasePath: dir} | |
| reaper := &Reaper{ | |
| Store: &Store{}, // won't be called | |
| TmpStorage: fs, | |
| Retention: 0, | |
| } | |
| cleaned, err := reaper.Sweep(context.Background()) | |
| if err != nil { | |
| t.Fatalf("Sweep: %v", err) | |
| } | |
| if cleaned != 0 { | |
| t.Errorf("cleaned = %d, want 0 when retention is 0", cleaned) | |
| } | |
| } | |
| func TestReaper_SkipsWhenRetentionZero(t *testing.T) { | |
| dir := t.TempDir() | |
| fs := &FilesystemTmpStorage{BasePath: dir} | |
| reaper := &Reaper{ | |
| Store: nil, // explicitly nil; Sweep must return early before Store access | |
| TmpStorage: fs, | |
| Retention: 0, | |
| } | |
| cleaned, err := reaper.Sweep(context.Background()) | |
| if err != nil { | |
| t.Fatalf("Sweep: %v", err) | |
| } | |
| if cleaned != 0 { | |
| t.Errorf("cleaned = %d, want 0 when retention is 0", cleaned) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/artifact/reaper_test.go` around lines 73 - 90, The test
TestReaper_SkipsWhenRetentionZero currently constructs Store as &Store{} which
may cause future nil-pointer panics if Sweep’s early-return logic changes;
update the test to provide a safe, real or mocked Store instance (e.g., an
in-memory or test helper Store) instead of a raw empty struct so Reaper.Sweep
has a valid Store object to call if retention logic changes; locate the test and
replace the Store creation used in the Reaper struct (reference:
TestReaper_SkipsWhenRetentionZero, Reaper.Sweep, Store) with a lightweight safe
implementation or explicit comment that a real Store is intended.
Comment on lines
+39
to
+57
| cleaned := 0 | ||
| for _, a := range expired { | ||
| // Delete file from tmp storage. | ||
| if delErr := r.TmpStorage.DeleteByPrefix(ctx, a.URL); delErr != nil { | ||
| logger.Error("failed to delete artifact file", | ||
| "artifact", a.Name, "url", a.URL, "error", delErr) | ||
| continue // skip metadata deletion if file delete failed | ||
| } | ||
|
|
||
| // Delete metadata from database. | ||
| if err := r.Store.DeleteByID(ctx, a.ID); err != nil { | ||
| logger.Error("failed to delete artifact metadata", | ||
| "artifact", a.Name, "id", a.ID, "error", err) | ||
| continue | ||
| } | ||
| cleaned++ | ||
| logger.Info("cleaned expired artifact", | ||
| "artifact", a.Name, "execution_id", a.ExecutionID) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what Put returns and how URL is used
# Find what Put returns
echo "=== FilesystemTmpStorage.Put return value ==="
ast-grep --pattern $'func (fs *FilesystemTmpStorage) Put($_, $_, $_) (string, error) {
$$$
}'
# Check if URL is stored as-is from Put
echo ""
echo "=== Store.Create usage of URL ==="
rg -n -A5 'store\.Create' --type=go | head -40
# Check how engine stores artifacts
echo ""
echo "=== Engine artifact persistence ==="
rg -n 'URL.*=' internal/engine/engine.go | head -20Repository: dvflw/mantle
Length of output: 4659
🏁 Script executed:
# Read DeleteByPrefix implementation
echo "=== DeleteByPrefix implementation (tmp.go lines 67-83) ==="
sed -n '67,83p' internal/artifact/tmp.go
echo ""
echo "=== Full DeleteByPrefix and surrounding context ==="
sed -n '65,95p' internal/artifact/tmp.go
echo ""
echo "=== Artifact struct definition ==="
rg -n -B2 -A8 'type Artifact struct' internal/artifact/
echo ""
echo "=== How artifacts are created in engine ==="
rg -n -B3 -A3 'Store\.Create' internal/engine/ --type=goRepository: dvflw/mantle
Length of output: 2480
🏁 Script executed:
# See the full artifact creation context
sed -n '510,535p' internal/engine/engine.go
echo ""
echo "=== Look for where putResp is assigned ==="
rg -n -B5 'putResp.*:=' internal/engine/engine.go | head -30Repository: dvflw/mantle
Length of output: 1100
Fix DeleteByPrefix call to use a reconstructed key instead of the full a.URL path.
The issue is valid. In engine.go line 519, Put returns the full filesystem path (e.g., /tmp/storage/workflow/exec/artifact/file), which is stored in a.URL. However, DeleteByPrefix at line 45 is called with this full path, while it expects a relative key prefix.
In DeleteByPrefix, the prefix argument is joined with BasePath via filepath.Join(fs.BasePath, prefix). When a full path is passed as prefix, filepath.Join returns that absolute path (absolute paths override the base), which works by accident but is semantically incorrect and fragile.
Compare this to engine.go lines 530–532, where orphan cleanup correctly uses the relative key string, not the full path. The reaper should follow the same pattern—reconstruct the key prefix from the artifact's metadata (workflow name, execution ID, artifact name) and pass that to DeleteByPrefix, not the stored URL.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/artifact/reaper.go` around lines 39 - 57, The reaper currently calls
r.TmpStorage.DeleteByPrefix(ctx, a.URL) using the stored full filesystem path
(a.URL); instead reconstruct the relative storage key prefix from the artifact
metadata (e.g. workflow name, a.ExecutionID and a.Name) and pass that relative
key to r.TmpStorage.DeleteByPrefix so filepath.Join(fs.BasePath, prefix) behaves
correctly. Locate the loop that calls r.TmpStorage.DeleteByPrefix and replace
the a.URL argument with the reconstructed key string (matching how orphan
cleanup builds the key in engine.go), then keep the subsequent
r.Store.DeleteByID call and logging unchanged.
Comment on lines
+509
to
+510
| relPath := filepath.Base(artDecl.Path) | ||
| localPath := filepath.Join(artifactsDir, relPath) |
Contributor
There was a problem hiding this comment.
Preserve the relative artifact path instead of collapsing it to a basename.
filepath.Base(artDecl.Path) makes a declaration like /mantle/artifacts/reports/report.json resolve as ${artifactsDir}/report.json. Nested artifact paths and same-basename files from different subdirectories will either fail lookup or persist the wrong file. Keep the path relative to the artifacts root and validate containment before joining it.
Also applies to: 516-516
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/engine/engine.go` around lines 509 - 510, Replace the use of
filepath.Base on artDecl.Path so nested paths are preserved: compute a path
relative to the artifacts root (use filepath.Rel or equivalent on artDecl.Path
vs the artifacts root) validate that the resulting relative path is not escaping
the artifactsDir (not absolute and does not start with ".."), then join
artifactsDir with that validated relative path to form localPath; update the
code locations referencing relPath/localPath (including the second occurrence at
line ~516) to use these validated relative paths instead of collapsing to a
basename.
- Add TmpStorage.Delete for single-file removal by URL - Clean up partial file in Put on io.Copy failure - Reaper uses Delete(url) instead of DeleteByPrefix - Stop mutating caller's params map in docker/run Execute - Use strconv.ParseInt for stricter memory value parsing - Reject artifact bind mounts for remote Docker daemons - Full rollback of persisted artifacts on partial failure in engine Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
internal/connector/docker.go (2)
193-218:⚠️ Potential issue | 🔴 CriticalNormalize resolver credentials before type asserting them.
internal/engine/engine.gowritesResolver.Resolve()output straight into_credentialand_registry_credential, so thesemap[string]stringassertions miss the normalmap[string]anypayload. In that case the connector silently falls back toFromEnvand anonymous pulls, which breaks remote-daemon/TLS config and private registry auth.Also applies to: 229-238
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/connector/docker.go` around lines 193 - 218, The connector currently type-asserts params["_credential"] (and similarly params["_registry_credential"]) directly to map[string]string which breaks when Resolver returns map[string]any; update the logic in internal/connector/docker.go to normalize/convert credentials before asserting: detect if params["_credential"] (and "_registry_credential") is map[string]any or map[string]interface{} and build a map[string]string by iterating keys and stringifying values (or casting when appropriate), then use that normalized cred map for dockerHost, TLS file writes and client.WithTLSClientConfig and registry auth; ensure the same normalization is applied at the other referenced block (around lines handling registry creds) so anonymous fallback no longer occurs.
219-221:⚠️ Potential issue | 🟠 MajorThe remote-daemon artifact-mount guard is bypassed when the host comes from
FromEnv.This check only looks at
dockerHostcopied from_credential. Whenclient.FromEnvpicks upDOCKER_HOST=tcp://...,dockerHoststays empty and the bind mount is still attempted against a remote daemon. Derive the effective host from the constructed client before allowing/mantle/artifacts.Also applies to: 279-283
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/connector/docker.go` around lines 219 - 221, The guard that prevents bind-mounting /mantle/artifacts uses the copied dockerHost from _credential, which is empty when client.FromEnv sets DOCKER_HOST, so derive the effective daemon host from the constructed Docker client (call the client's DaemonHost()/DaemonHost method or equivalent on the returned *client.Client) and use that value when deciding whether to allow the /mantle/artifacts mount; update the check that currently reads dockerHost to instead query the built client, and apply the same change to the other occurrence around the 279-283 block to ensure FromEnv-picked hosts are handled correctly.internal/engine/engine.go (1)
525-527:⚠️ Potential issue | 🟠 MajorPreserve the declared artifact subpath instead of collapsing it to a basename.
filepath.Base(artDecl.Path)makes/mantle/artifacts/reports/report.jsonresolve as${artifactsDir}/report.json. Nested artifact paths will be missed, and same-basename files from different directories collapse onto the same storage key. Compute and validate the path relative to the artifact root instead.Also applies to: 534-535
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/engine/engine.go` around lines 525 - 527, Replace the use of filepath.Base(artDecl.Path) (used when creating localPath from artDecl.Path in the loop over step.Artifacts) with a preserved subpath computed relative to the artifact root: use filepath.Clean on artDecl.Path, then compute rel, err := filepath.Rel(artifactRoot, cleanedPath) (or if artDecl.Path is already intended to be relative, use filepath.Clean and ensure it's not absolute), then validate that rel does not start with ".." and is not absolute to prevent path traversal, and finally set localPath := filepath.Join(artifactsDir, rel); apply the same change to the other occurrence around lines 534-535 so nested paths are preserved and same-basename collisions avoided.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/artifact/tmp.go`:
- Around line 91-104: In FilesystemTmpStorage.Delete, make deletion idempotent:
after resolving absBase and absURL and validating rel, call os.Remove on absURL
(not the original url) and treat a returned error equal to os.ErrNotExist as a
successful deletion (i.e., return nil), while still returning other errors;
update the behavior in the Delete method accordingly.
In `@internal/engine/engine.go`:
- Around line 413-421: The code currently allows steps that declare artifacts
(step.Artifacts) to run when either e.TmpStorage or e.ArtifactStore is nil,
causing silent ignores and later hard-to-debug failures; update the pre-step
artifact handling (the block creating artifactsDir and calling
artifact.WithArtifactsDir) to check that BOTH e.TmpStorage and e.ArtifactStore
are non-nil and, if not, return an explicit error indicating artifact subsystem
is disabled for steps that declare artifacts (include step identifier/context in
the message). Apply the same guard where artifacts are expected elsewhere in
this file (the other artifact-related block around ArtifactStore usage) so any
step with step.Artifacts fails fast instead of proceeding without mounts.
- Around line 514-523: The rollback function currently deletes persisted
artifacts (using e.ArtifactStore.DeleteByID and e.TmpStorage.Delete) but does
not emit audit events; update rollback to emit audit events via the AuditEmitter
interface after each successful metadata/blob deletion (mirror the same pattern
used when emitting ActionArtifactPersisted), including artifact identifiers
(persisted p.id and p.url) and contextual info; also make the same change in the
other artifact-cleanup paths that call e.ArtifactStore.DeleteByID /
e.TmpStorage.Delete so every state-changing delete emits the corresponding audit
action (e.g., ActionArtifactDeleted) through e.AuditEmitter.Emit with the same
context and payload shape as the persisted-success path.
---
Duplicate comments:
In `@internal/connector/docker.go`:
- Around line 193-218: The connector currently type-asserts
params["_credential"] (and similarly params["_registry_credential"]) directly to
map[string]string which breaks when Resolver returns map[string]any; update the
logic in internal/connector/docker.go to normalize/convert credentials before
asserting: detect if params["_credential"] (and "_registry_credential") is
map[string]any or map[string]interface{} and build a map[string]string by
iterating keys and stringifying values (or casting when appropriate), then use
that normalized cred map for dockerHost, TLS file writes and
client.WithTLSClientConfig and registry auth; ensure the same normalization is
applied at the other referenced block (around lines handling registry creds) so
anonymous fallback no longer occurs.
- Around line 219-221: The guard that prevents bind-mounting /mantle/artifacts
uses the copied dockerHost from _credential, which is empty when client.FromEnv
sets DOCKER_HOST, so derive the effective daemon host from the constructed
Docker client (call the client's DaemonHost()/DaemonHost method or equivalent on
the returned *client.Client) and use that value when deciding whether to allow
the /mantle/artifacts mount; update the check that currently reads dockerHost to
instead query the built client, and apply the same change to the other
occurrence around the 279-283 block to ensure FromEnv-picked hosts are handled
correctly.
In `@internal/engine/engine.go`:
- Around line 525-527: Replace the use of filepath.Base(artDecl.Path) (used when
creating localPath from artDecl.Path in the loop over step.Artifacts) with a
preserved subpath computed relative to the artifact root: use filepath.Clean on
artDecl.Path, then compute rel, err := filepath.Rel(artifactRoot, cleanedPath)
(or if artDecl.Path is already intended to be relative, use filepath.Clean and
ensure it's not absolute), then validate that rel does not start with ".." and
is not absolute to prevent path traversal, and finally set localPath :=
filepath.Join(artifactsDir, rel); apply the same change to the other occurrence
around lines 534-535 so nested paths are preserved and same-basename collisions
avoided.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 53586baa-c217-42c4-8128-7a9ed40b8ae1
📒 Files selected for processing (5)
internal/artifact/reaper.gointernal/artifact/reaper_test.gointernal/artifact/tmp.gointernal/connector/docker.gointernal/engine/engine.go
Comment on lines
+514
to
+523
| rollback := func() { | ||
| for _, p := range persisted { | ||
| if delErr := e.ArtifactStore.DeleteByID(ctx, p.id); delErr != nil { | ||
| log.Printf("warning: rollback: failed to delete artifact metadata %q: %v", p.id, delErr) | ||
| } | ||
| if delErr := e.TmpStorage.Delete(ctx, p.url); delErr != nil { | ||
| log.Printf("warning: rollback: failed to delete artifact blob %q: %v", p.url, delErr) | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Emit audit events for rollback deletes too.
rollback() and the immediate orphan-cleanup path remove artifact blobs/metadata, but only the success path emits ActionArtifactPersisted. A later artifact failure can therefore mutate artifact state without any matching audit record.
As per coding guidelines, internal/engine/**/*.go: Emit audit events for every state-changing operation via the AuditEmitter interface.
Also applies to: 548-553, 558-573
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/engine/engine.go` around lines 514 - 523, The rollback function
currently deletes persisted artifacts (using e.ArtifactStore.DeleteByID and
e.TmpStorage.Delete) but does not emit audit events; update rollback to emit
audit events via the AuditEmitter interface after each successful metadata/blob
deletion (mirror the same pattern used when emitting ActionArtifactPersisted),
including artifact identifiers (persisted p.id and p.url) and contextual info;
also make the same change in the other artifact-cleanup paths that call
e.ArtifactStore.DeleteByID / e.TmpStorage.Delete so every state-changing delete
emits the corresponding audit action (e.g., ActionArtifactDeleted) through
e.AuditEmitter.Emit with the same context and payload shape as the
persisted-success path.
- Make TmpStorage.Delete idempotent (treat ErrNotExist as success) - Fail fast when step declares artifacts but tmp storage not configured - Use cli.DaemonHost() for remote daemon detection (handles DOCKER_HOST env) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@coderabbitai Re: remaining review findings from round 4: Credential type assertion ( Rollback audit events — The audit emitter is currently a
|
Contributor
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/artifact/tmp.go`:
- Around line 51-67: The code currently opens localPath with os.Open which will
follow symlinks; before opening, call os.Lstat(localPath) and verify the
FileInfo.Mode().IsRegular() is true, returning an error if not regular (reject
symlinks, dirs, devices); only then proceed to os.Open and the existing copy
flow. Update the function that contains this snippet (the Put/tmp artifact
writer) to perform the os.Lstat + IsRegular check and return a clear error for
non-regular files to prevent symlink-based exfiltration.
In `@internal/connector/docker.go`:
- Around line 111-118: The code currently accepts negative params["cpus"] values
and stores them in cfg.CPUs, which later causes negative inputs to be treated as
"no limit"; update the parsing branch that handles params["cpus"] (the switch
handling float64/int/int64) to validate that the parsed numeric value is >= 0
and return a descriptive error if it's negative, and ensure downstream logic
that sets NanoCPUs only applies when cfg.CPUs > 0 (leave the existing NanoCPUs
check as-is). Use the same validation for all three cases so cfg.CPUs is never
set to a negative number and callers receive an error early instead of silently
widening resources.
- Around line 309-313: Replace the unbounded context.Background() calls used in
the defer cleanup and stop logic with a bounded timeout context (e.g.,
context.WithTimeout(context.Background(), 30*time.Second)) and ensure you call
the cancel function to avoid leaks; specifically update the ContainerRemove call
inside the defer and the ContainerStop call to use the timeout context. Also
capture and explicitly log any returned errors from cli.ContainerRemove and
cli.ContainerStop (include containerID and cfg.Remove context in the log) so
cleanup failures are visible; reference the existing defer func and the
ContainerStop invocation when making these changes.
In `@internal/engine/engine.go`:
- Around line 411-425: The artifacts scratch directory (artifactsDir) must be
created/cleared per retry attempt instead of once for all attempts; move the
os.MkdirTemp/create or a cleanup+recreate operation into the per-attempt
execution block (the retry loop that runs the step) so that each attempt gets a
fresh artifactsDir, and ensure artifact.WithArtifactsDir(ctx, artifactsDir) is
called with the fresh path and the previous temp is removed after that attempt;
update logic around e.TmpStorage/e.ArtifactStore checks (same symbols) so
creation happens per attempt and persisting (the code that uploads from
artifactsDir) always uses the current attempt’s directory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 39c10fbd-a65e-44b4-80bc-5a3cffb78cbe
📒 Files selected for processing (3)
internal/artifact/tmp.gointernal/connector/docker.gointernal/engine/engine.go
- Reject symlinks and non-regular files in TmpStorage.Put - Validate cpus param is non-negative in docker/run - Add timeout context and error logging to container cleanup/stop - Create fresh artifacts scratch dir per retry attempt Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
This was referenced Mar 25, 2026
4 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
docker/run) — Run containers to completion with full lifecycle management (pull, create, start, wait, capture, remove). Supports env vars, stdin piping, volume mounts, resource limits (memory/CPU), configurable pull policy and network mode. Non-zero exit codes are informational, not failures.artifacts['name'].urlin CEL expressions. Includes metadata store, TTL-based reaper, andexecution_artifactsPostgres table.tmpconfig inmantle.yamlsupporting S3 and filesystem backends with configurable retention.dockercredential with optionalhost,ca_cert,client_cert,client_keyfields for socket and TCP+TLS daemon access.docker/runreference docs.Out-of-scope changes included in this branch
This branch (
feat/data-transformation-cel) was originally created for CEL data transformation functions (PR #21). The Docker connector and artifact system work was built on top of it. The following changes pre-date the Docker work and were part of the earlier CEL feature:docs/superpowers/plans/2026-03-24-init-connection-recovery.md,docs/superpowers/specs/2026-03-24-init-connection-recovery-design.md) — These were gitignored superpowers planning docs that were accidentally force-added and then removed. Not related to this feature.toLower,toUpper,trim,replace,split,parseInt,parseFloat,toString,obj,default,flatten,jsonEncode,jsonDecode,timestamp,formatTimestamp) — These were implemented as part of issue [Feature] Data Transformation Step #14 (data transformation) in prior commits on this branch.New files
internal/connector/docker.go— Docker connector implementationinternal/artifact/— Store, TmpStorage, Reaper, context helpersinternal/db/migrations/013_execution_artifacts.sqlexamples/docker-volume-backup.yaml— Closes [Feature] Docker Volume Backup Example #12examples/docker-data-transform.yaml— Closes [Feature] Run Docker Container for Step #13Test plan
go test ./...)npm run build)Closes #12, closes #13
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Examples
Documentation
Website