internal/core/spec/step_types.go (1)

86-98: Thread-safe registration looks correct.

Read/write locking around builtinStepTypeNames is properly paired across RegisterExecutorTypeName and isBuiltinStepTypeName. One optional consideration: RegisterExecutorTypeName does not validate the name against customStepTypeNameRegexp (the same validator applied to custom step types). Registering an arbitrary string here means any garbage value will silently be accepted by DAG loading. Consider rejecting names that don't match customStepTypeNameRegexp (or returning an error) to prevent malformed executor names from poisoning the registry.

Also applies to: 287-292

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/spec/step_types.go` around lines 86 - 98,
RegisterExecutorTypeName currently accepts any non-empty string and inserts it
into builtinStepTypeNames; change it to validate the trimmed name against
customStepTypeNameRegexp and refuse to register invalid names (i.e., return
early without inserting) to avoid poisoning the registry with garbage. Update
the body of RegisterExecutorTypeName to: trim the input, if empty return, if
!customStepTypeNameRegexp.MatchString(name) return (or optionally log/debug),
otherwise acquire stepTypeNamesMu and insert into builtinStepTypeNames; apply
the same validation logic to the other registration site that manipulates
builtinStepTypeNames (the one around the other RegisterExecutorTypeName-like
block).

executor.go (1)

14-51: GoDoc comments are a best practice for public APIs, but not currently enforced by your linter configuration.

This file defines the public API surface (Step, Executor, ExecutorFactory, StepValidator, ExecutorCapabilities, ExecutorOption, RegisterExecutor, WithStepValidator, WithExecutorCapabilities). While adding one-line doc comments to these exported identifiers would improve documentation, the revive linter's exported rule is not enabled in .golangci.yml (only specific rules are listed: unused-parameter, import-alias-naming, if-return, error-return, dot-imports, struct-tag, function-result-limit, atomic, use-any, redundant-import-alias, enforce-map-style, enforce-slice-style). Adding GoDoc is optional unless your team adopts a style guide that requires it.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@executor.go` around lines 14 - 51, Add concise GoDoc one-line comments for
all exported identifiers in this file to improve public API documentation:
document type aliases Step, Executor, ExecutorFactory, StepValidator,
ExecutorCapabilities, ExecutorOption, the executorRegistration struct, and
exported functions RegisterExecutor, WithStepValidator, and
WithExecutorCapabilities by adding a brief comment above each declaration
describing its purpose and usage (e.g., what RegisterExecutor does and what
arguments it expects, and what the option helpers set). Ensure comments follow
GoDoc style (start with the identifier name) and are placed immediately above
the corresponding declarations.

internal/engine/logger.go (1)

41-58: Pass slog.Attr values directly to With() for efficiency, but the LogValuer preservation claim is inaccurate.

slog.Logger.Log (which With uses) explicitly handles slog.Attr arguments: "If an argument is an Attr, it is used as is." The suggested change is valid and more efficient—it avoids unpacking and reconstructing the Attr.

However, the reasoning about preserving "lazy evaluation semantics" is incorrect. Both approaches preserve the LogValuer instance: calling attr.Value.Any() on a LogValuer-type Value returns the LogValuer instance itself, not a resolved value. Neither approach "flattens" the structure; both maintain the typed LogValuer for handler processing.

♻️ Suggested change

 func (l *slogAdapter) With(attrs ...slog.Attr) logger.Logger {
-	args := make([]any, 0, len(attrs)*2)
-	for _, attr := range attrs {
-		args = append(args, attr.Key, attr.Value.Any())
-	}
-	return &slogAdapter{logger: l.logger.With(args...)}
+	args := make([]any, 0, len(attrs))
+	for _, attr := range attrs {
+		args = append(args, attr)
+	}
+	return &slogAdapter{logger: l.logger.With(args...)}
 }

The same applies to log() on line 53.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/engine/logger.go` around lines 41 - 58, The current slogAdapter.With
and slogAdapter.log unpack slog.Attr into key/value pairs by calling
attr.Value.Any(), which is unnecessary and less efficient; instead pass the
slog.Attr values directly to the underlying logger (use l.logger.With(attrs...)
in With and l.logger.Log(..., tags...) in log) so you stop constructing args
slices and preserve Attrs unchanged; update the With method to forward attrs
as-is and update log to forward tags as-is (remove attr.Value.Any() usage and
the key/value unpacking).

engine_test.go (1)

30-56: Use testify assertions in the new tests.

These tests use manual if/t.Fatalf checks instead of require/assert. Per the coding guidelines for **/*_test.go: Use stretchr/testify assertions for testing in Go. Testify (v1.11.1) is already available as a dependency.

♻️ Example conversion pattern

 import (
 	"context"
 	"os"
 	"path/filepath"
 	"strings"
 	"testing"
 	"time"
 
 	"github.com/dagucloud/dagu"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
@@
 	engine, err := dagu.New(ctx, dagu.Options{HomeDir: home})
-	if err != nil {
-		t.Fatalf("New() error = %v", err)
-	}
+	require.NoError(t, err, "New()")
@@
 	run, err := engine.RunFile(ctx, dagFile, dagu.WithParams(map[string]string{"FOO": "bar"}))
-	if err != nil {
-		t.Fatalf("RunFile() error = %v", err)
-	}
+	require.NoError(t, err, "RunFile()")
 	status, err := run.Wait(ctx)
-	if err != nil {
-		t.Fatalf("Wait() error = %v", err)
-	}
-	if status.Status != "succeeded" {
-		t.Fatalf("status = %q, want succeeded", status.Status)
-	}
-	if status.Name != "embedded-file" {
-		t.Fatalf("name = %q, want embedded-file", status.Name)
-	}
-	if status.RunID == "" {
-		t.Fatal("run ID is empty")
-	}
+	require.NoError(t, err, "Wait()")
+	assert.Equal(t, "succeeded", status.Status)
+	assert.Equal(t, "embedded-file", status.Name)
+	assert.NotEmpty(t, status.RunID)

Also applies to: 63-90, 98-118, 122-126

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@engine_test.go` around lines 30 - 56, Replace the manual if/t.Fatalf style
checks in engine_test.go with testify/require assertions: import
"github.com/stretchr/testify/require" and use require.NoError(t, err) after New,
RunFile and Wait calls, use require.Equal(t, "succeeded", status.Status) and
require.Equal(t, "embedded-file", status.Name) for the expected values, and use
require.NotEmpty(t, status.RunID) for the run ID; also replace the deferred
Close() error check with require.NoError(t, engine.Close(context.Background()))
inside the deferred function to keep failure reporting consistent (update the
other similar test blocks at lines referenced in the comment as well).

engine.go (2)

236-257: Run.Wait (and Run.Status partly) can be simplified.

The if err != nil branch returns the same tuple as the success branch in Wait. Collapsing avoids the visual implication that the error case is special-cased.

♻️ Suggested simplification

-	status, err := r.inner.Wait(ctx)
-	if err != nil {
-		return publicStatus(status), err
-	}
-	return publicStatus(status), nil
+	status, err := r.inner.Wait(ctx)
+	return publicStatus(status), err

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@engine.go` around lines 236 - 257, The error branch in Run.Wait duplicates
the success return; simplify both Wait (and mirror for Status) by removing the
redundant if err != nil block and directly return publicStatus(status), err from
the call to r.inner.Wait(ctx) (and similarly return publicStatus(status), err
for r.inner.Status(ctx)), keeping the initial nil-check for r and r.inner and
using publicStatus to convert the inner status.

---

307-318: Inconsistent empty-vs-nil normalization between maps and slices.

WithParams / WithWorkerSelector / WithTags go through cloneMap (which returns nil for an empty input), while WithParamsList / WithTags use append([]string{}, …) which returns a non-nil empty slice. Then internalRunOptions re-clones again, doubling the work and re-introducing the same nil/empty mismatch. Consider:

• Normalizing to one helper (e.g., a cloneSlice that returns nil for empty input) for symmetry, and
• Skipping the second clone in internalRunOptions since applyRunOptions already produced a defensive copy.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@engine.go` around lines 307 - 318, WithParams/WithWorkerSelector/WithTags
currently use cloneMap which returns nil for empty maps while
WithParamsList/WithTags create non-nil empty slices via append, causing
inconsistent empty-vs-nil behavior and redundant cloning in internalRunOptions;
add a cloneSlice helper that mirrors cloneMap semantics (returns nil for empty
input) and update WithParamsList (and any slice-using WithTags overloads) to use
cloneSlice, and then remove the second cloning step inside internalRunOptions
(since applyRunOptions already makes defensive copies) so the code normalizes
empties consistently and avoids double work; reference functions: WithParams,
WithParamsList, WithWorkerSelector, WithTags, cloneMap, cloneSlice (new),
applyRunOptions, internalRunOptions.

internal/engine/run.go (2)

260-267: applyRunOverrides doesn't dedupe tags and only handles two fields.

dag.Tags = append(dag.Tags, core.NewTags(opts.Tags)...) will accumulate duplicates if the embedder calls WithTags with values already present in the YAML. Also, given that other run-level overrides like Name/DefaultWorkingDir/params live in loadOptions, it's worth a comment here noting the split so future maintainers don't re-add overlapping logic.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/engine/run.go` around lines 260 - 267, applyRunOverrides currently
appends opts.Tags to dag.Tags without deduping and only handles
WorkerSelector/Tags; change applyRunOverrides to merge tags idempotently by
building a set of existing dag.Tags (from dag.Tags and core.NewTags(opts.Tags))
and only append new tags that aren't present (use the tag string/identifier
comparisons consistent with core.NewTags), and add a short comment in
applyRunOverrides explaining that other run-level overrides (Name,
DefaultWorkingDir, params) are handled in loadOptions so logic is intentionally
limited here; keep the existing WorkerSelector override behavior.

---

151-216: waitLocal/waitDistributed ignore caller context for the final status fetch.

In both branches the post-completion (or post-cancellation) Status lookup uses context.Background(), even when the caller's ctx is the one carrying deadlines, tracing, or cancellation reasons. For the timeout case (line 167, 190) this is arguably required so we can still return the latest snapshot, but it also means the call can hang indefinitely on a slow file/coord backend after the user's context already expired. Consider using a short bounded context derived from context.Background() (e.g. context.WithTimeout(context.Background(), 5*time.Second)) so the embedder isn't stuck.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/engine/run.go` around lines 151 - 216, Both waitLocal and
waitDistributed call r.Status(context.Background()) after the caller ctx is
done, which can hang indefinitely; replace those background calls with a short
bounded background context (e.g. ctxStatus, cancel :=
context.WithTimeout(context.Background(), 5*time.Second); defer cancel()) and
pass ctxStatus into r.Status. Update all occurrences inside waitLocal (the
status/statusErr fetch after <-r.done) and both places in
waitLocal/waitDistributed where you currently call
r.Status(context.Background()) on ctx.Done() paths so the status fetch times out
quickly; ensure you call cancel() to avoid leaks.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes