internal/cmn/config/definition.go (4)

136-139: Consider using Go-idiomatic acronym casing.

Go convention prefers ClientID, ClientURL over ClientId, ClientUrl for acronyms in exported identifiers. The mapstructure tags can remain lowercase for config file compatibility.

♻️ Suggested naming alignment

 type AuthOIDCDef struct {
-	ClientId       string              `mapstructure:"clientId"`
-	ClientSecret   string              `mapstructure:"clientSecret"`
-	ClientUrl      string              `mapstructure:"clientUrl"`
+	ClientID       string              `mapstructure:"clientId"`
+	ClientSecret   string              `mapstructure:"clientSecret"`
+	ClientURL      string              `mapstructure:"clientUrl"`
 	Issuer         string              `mapstructure:"issuer"`

---

215-215: Inconsistent mapstructure tag casing.

The field SkipTLSVerify uses the tag skipTlsVerify (lowercase 'l' in 'Tls'). While mapstructure is typically case-insensitive, aligning the tag casing improves consistency, especially since RemoteNodeDef.SkipTLSVerify on line 230 uses skipTLSVerify (uppercase 'TLS').

♻️ Suggested fix

-	SkipTLSVerify bool   `mapstructure:"skipTlsVerify"`
+	SkipTLSVerify bool   `mapstructure:"skipTLSVerify"`

---

248-249: Consider documenting the expected types for interface{} fields.

Using interface{} for Labels and Coordinators provides parsing flexibility, but the brief comments ("String or map", "String or list") could be expanded. Consider adding more explicit documentation about the expected formats and validation that occurs during config loading to help maintainers understand the contract.

---

254-259: Inconsistent duration field representation.

PostgresPoolDef uses int with "Seconds" suffix in comments (lines 257-258), while SchedulerDef (lines 264-266) uses string for durations. Consider standardizing on one approach across all definition types for consistency.

internal/cmn/config/loader.go (1)

1045-1058: Clarify SectionNone semantics in legacy env mappings.

Using SectionNone (value 0) as requires means these path mappings are always loaded regardless of service, which is correct since paths are fundamental. However, this relies on the condition mapping.requires != SectionNone being false for these entries.

Consider adding a brief comment to clarify this intentional behavior for future maintainers.

📝 Suggested comment for clarity

 "DAGU__DATA": {
 	newKey:   "DAGU_DATA_DIR",
 	setter:   func(c *Config, v string) { c.Paths.DataDir = fileutil.ResolvePathOrBlank(v) },
-	requires: SectionNone,
+	requires: SectionNone, // SectionNone means always loaded (paths are universal)
 },

internal/cmd/status_test.go (2)

42-47: Add a timeout when waiting on done to prevent hangs.

Line 46 blocks indefinitely if Start never returns. Consider failing fast with a timeout.

💡 Suggested change

 func stopDAGAndWait(t *testing.T, th test.Command, dagLocation string, done <-chan struct{}) {
 	t.Helper()
 	th.RunCommand(t, cmd.Stop(), test.CmdTest{Args: []string{"stop", dagLocation}})
-	<-done
+	select {
+	case <-done:
+	case <-time.After(5 * time.Second):
+		t.Fatalf("timed out waiting for DAG to stop")
+	}
 }

---

409-482: Replace fixed sleeps with eventual waits for sub-run availability.

Line 430 and Line 473 rely on fixed sleeps, which can flake under slow CI. Prefer require.Eventually to wait for the parent run/sub-run to appear before assertions and error-message checks.

💡 Example approach for StatusSubDAGRun

-	time.Sleep(500 * time.Millisecond)
-
-	parentRef := exec.NewDAGRunRef(dagFile.Location, parentRunID)
-	parentAttempt, err := th.DAGRunStore.FindAttempt(th.Context, parentRef)
-	require.NoError(t, err)
-
-	parentStatus, err := parentAttempt.ReadStatus(th.Context)
-	require.NoError(t, err)
-	require.Len(t, parentStatus.Nodes, 1)
-	require.NotEmpty(t, parentStatus.Nodes[0].SubRuns)
+	parentRef := exec.NewDAGRunRef(dagFile.Location, parentRunID)
+	var parentStatus exec.DAGRunStatus
+	require.Eventually(t, func() bool {
+		parentAttempt, err := th.DAGRunStore.FindAttempt(th.Context, parentRef)
+		if err != nil {
+			return false
+		}
+		status, err := parentAttempt.ReadStatus(th.Context)
+		if err != nil {
+			return false
+		}
+		if len(status.Nodes) != 1 || len(status.Nodes[0].SubRuns) == 0 {
+			return false
+		}
+		parentStatus = status
+		return true
+	}, 5*time.Second, 50*time.Millisecond)

internal/cmn/fileutil/atomic_test.go (1)

55-66: Permission test may behave differently on Windows.

File permission checks with info.Mode().Perm() may not work as expected on Windows, where the permission model differs from Unix. Consider adding a build constraint or skip condition.

💡 Optional: Skip on Windows

 	t.Run("sets correct permissions", func(t *testing.T) {
 		t.Parallel()
+		if runtime.GOOS == "windows" {
+			t.Skip("file permissions work differently on Windows")
+		}
 		dir := t.TempDir()
 		filePath := filepath.Join(dir, "test.txt")

Add "runtime" to imports.

internal/agent/think.go (1)

33-36: The "thought" parameter is not actually recorded.

The function discards the input entirely. While the response says "Thought recorded," the thought content is never logged, stored, or included in the response. Consider either:

1. Including the thought in the response so it appears in conversation history
2. Logging it for debugging purposes
3. Updating the response message to be more accurate

💡 Option: Include thought in response for conversation history

+type thinkInput struct {
+	Thought string `json:"thought"`
+}
+
 // thinkRun acknowledges the thought without performing any action.
-func thinkRun(_ ToolContext, _ json.RawMessage) ToolOut {
-	return ToolOut{Content: "Thought recorded. Continue with your plan."}
+func thinkRun(_ ToolContext, input json.RawMessage) ToolOut {
+	var args thinkInput
+	if err := json.Unmarshal(input, &args); err != nil {
+		return ToolOut{Content: "Thought recorded. Continue with your plan."}
+	}
+	return ToolOut{Content: fmt.Sprintf("Thought: %s\n\nContinue with your plan.", args.Thought)}
 }

internal/agent/subpub_test.go (1)

31-34: Prefer require for test assertions
Switch these (and the other assert.* calls in this file) to require.* so failures stop the test and align with repo conventions.

♻️ Example change

-	msg, ok := next()
-	assert.True(t, ok)
-	assert.Equal(t, "hello", msg)
+	msg, ok := next()
+	require.True(t, ok)
+	require.Equal(t, "hello", msg)

Based on learnings: Use stretchr/testify/require and shared fixtures from internal/test instead of duplicating mocks.

internal/agent/patch_test.go (1)

299-305: Consider adding a test case for the path prefix bypass scenario.

Add a test case to verify that paths like /dags-malicious/file.yaml are correctly rejected when dagsDir is /dags:

🧪 Suggested additional test case

{
	name:     "yaml file with similar prefix but outside dags directory",
	path:     "/dags-malicious/workflow.yaml",
	dagsDir:  "/dags",
	expected: false,
},

internal/cmn/config/config.go (1)

385-391: Consider validating MaxDashboardPageLimit upper bound.

The validation ensures the limit is at least 1, but an extremely large value could cause performance issues. Consider adding a reasonable upper bound:

🔧 Optional: Add upper bound validation

 func (c *Config) validateUI() error {
 	if c.UI.MaxDashboardPageLimit < 1 {
 		return fmt.Errorf("invalid max dashboard page limit: %d", c.UI.MaxDashboardPageLimit)
 	}
+	if c.UI.MaxDashboardPageLimit > 1000 {
+		return fmt.Errorf("max dashboard page limit too high: %d (max: 1000)", c.UI.MaxDashboardPageLimit)
+	}
 	return nil
 }

Makefile (1)

123-123: Consider consistency with run-server-https target.

The run target now sets DAGU_DEBUG=1, but the run-server-https target (line 148) does not. If debug mode is useful during local development, you may want to apply it consistently across both targets.

🔧 Optional: Apply DAGU_DEBUG to run-server-https

 .PHONY: run-server-https
 run-server-https: ${SERVER_CERT_FILE} ${SERVER_KEY_FILE}
 	`@echo` "${COLOR_GREEN}Starting the server with HTTPS...${COLOR_RESET}"
-	`@DAGU_CERT_FILE`=${SERVER_CERT_FILE} \
+	`@DAGU_DEBUG`=1 DAGU_CERT_FILE=${SERVER_CERT_FILE} \
 		DAGU_KEY_FILE=${SERVER_KEY_FILE} \
 		go run ./cmd start-all

internal/agent/store_test.go (1)

38-61: Consider using require instead of assert per coding guidelines.

The coding guidelines recommend using stretchr/testify/require for test assertions. While assert works here, require provides fail-fast behavior which is generally preferred.

♻️ Optional: Switch to require for fail-fast assertions

 import (
 	"testing"
 
-	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestDefaultConfig(t *testing.T) {
 	t.Parallel()
 
 	t.Run("returns correct defaults", func(t *testing.T) {
 		t.Parallel()
 
 		cfg := DefaultConfig()
 
-		assert.True(t, cfg.Enabled)
-		assert.Equal(t, DefaultProvider, cfg.LLM.Provider)
-		assert.Equal(t, DefaultModel, cfg.LLM.Model)
+		require.True(t, cfg.Enabled)
+		require.Equal(t, DefaultProvider, cfg.LLM.Provider)
+		require.Equal(t, DefaultModel, cfg.LLM.Model)
 	})
 // ... apply similar changes throughout

As per coding guidelines: "Use stretchr/testify/require and shared fixtures from internal/test instead of duplicating mocks"

internal/agent/system_prompt.txt (1)

591-603: Consider varying the sentence structure in the final rules.

The static analysis flagged three successive "NEVER" sentences at the end. While the emphasis is intentional for security, varying the structure slightly could improve readability without reducing impact.

📝 Optional: Vary sentence structure

 - Use `navigate` for UI pages, not CLI status commands.
 - ALWAYS navigate after creating/modifying DAGs.
 - ALWAYS validate with `dagu validate` before confirming.
 - Never run (`dagu start`) unless the user explicitly requests it.
 - NEVER read environment variables directly (security risk).
-- NEVER assume uncertain values; ask when unclear.
-- NEVER create DAGs with placeholder/dummy values unless the user explicitly requests dummy data.
+- When uncertain about values, ask—do not assume.
+- Avoid placeholder/dummy values in DAGs unless the user explicitly requests them for testing.

internal/agent/schema/output_test.go (1)

53-68: Consider adding t.Parallel() and using testify for consistency.

Other tests in this PR (e.g., store_test.go) use t.Parallel() and testify assertions. Adding these would improve consistency across the test suite.

♻️ Optional: Add parallel execution and testify assertions

+import (
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
 func TestNavigateDeepPaths(t *testing.T) {
+	t.Parallel()
+
 	tests := []struct {
 		// ...
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
 			result, err := DefaultRegistry.Navigate("dag", tt.path)
-			if (err != nil) != tt.wantErr {
-				t.Errorf("Navigate() error = %v, wantErr %v", err, tt.wantErr)
-				return
-			}
-			if err != nil {
+			if tt.wantErr {
+				require.Error(t, err)
 				return
 			}
+			require.NoError(t, err)
 			for _, want := range tt.want {
-				if !strings.Contains(result, want) {
-					t.Errorf("Navigate() result missing %q\nGot:\n%s", want, result)
-				}
+				require.Contains(t, result, want, "Navigate() result missing expected content")
 			}
 		})
 	}
 }

As per coding guidelines: "Use stretchr/testify/require and shared fixtures from internal/test instead of duplicating mocks"

internal/agent/read_test.go (1)

22-24: Consider escaping special characters in path.

The readInput helper directly interpolates the path into JSON. If a path contains characters like " or \, this could produce invalid JSON.

🛠️ Suggested improvement

 func readInput(path string) json.RawMessage {
-	return json.RawMessage(`{"path": "` + path + `"}`)
+	data, _ := json.Marshal(map[string]string{"path": path})
+	return data
 }

internal/agent/api_test.go (2)

401-436: SSE streaming test is reasonable but timing-dependent.

The test uses a 50ms sleep which could be flaky on slow CI systems. Consider using a synchronization mechanism or increasing the timeout if this becomes flaky.

---

438-499: Loop variable capture in Go 1.22+.

Lines 489 and 604 have tc := tc which was necessary before Go 1.22 to avoid loop variable capture issues. If this project uses Go 1.22+, these lines are no longer needed.

#!/bin/bash
# Check Go version in go.mod to determine if tc := tc is still needed
cat go.mod | head -5

internal/agent/read_schema.go (1)

22-24: Consider: Schema enum is captured at tool creation time.

AvailableSchemas() is called once when NewReadSchemaTool() is invoked. If schemas are registered after tool creation, they won't appear in the enum. This is likely acceptable if tools are created after all schemas are registered (e.g., at startup), but worth noting for future maintainers.

internal/agent/mocks_test.go (1)

13-115: Consider promoting these mocks into shared test fixtures.

This reduces duplication and keeps mock behavior consistent across agent tests.
Based on learnings: Use stretchr/testify/require and shared fixtures from internal/test instead of duplicating mocks.

internal/agent/schema/registry.go (1)

55-63: Sort schema names for deterministic output.

Map iteration order is randomized, so error messages and AvailableSchemas() output can fluctuate. Sorting improves determinism and test stability.

🧭 Suggested change

 import (
 	"encoding/json"
 	"fmt"
+	"sort"
 	"strings"
 	"sync"
 )
@@
 func (r *Registry) AvailableSchemas() []string {
 	r.mu.RLock()
 	defer r.mu.RUnlock()
 	names := make([]string, 0, len(r.schemas))
 	for name := range r.schemas {
 		names = append(names, name)
 	}
+	sort.Strings(names)
 	return names
 }

internal/agent/api.go (1)

319-336: Type assertions from sync.Map are safe but could be more defensive.

The type assertions at lines 320 and 325 assume the map only contains *ConversationManager. While this is true given the current implementation, a defensive check would prevent panics if the invariant is ever violated.

♻️ Optional: Add defensive type assertion check

 a.conversations.Range(func(key, value any) bool {
-	mgr := value.(*ConversationManager)
+	mgr, ok := value.(*ConversationManager)
+	if !ok {
+		return true
+	}
 	if mgr.UserID() != userID {
 		return true
 	}

-	id := key.(string)
+	id, ok := key.(string)
+	if !ok {
+		return true
+	}

internal/agent/web_search.go (1)

107-135: Retry condition may retry non-retryable errors.

The retry condition at lines 112-118 returns true for any error, which could cause retries for non-retryable errors like DNS resolution failures or certificate errors. Consider being more selective.

♻️ Suggested improvement for retry logic

 AddRetryCondition(func(r *resty.Response, err error) bool {
-	if err != nil {
-		return true
-	}
+	// Only retry on transient network errors, not permanent failures
+	if err != nil {
+		// Could check for specific error types if needed
+		return false
+	}
 	code := r.StatusCode()
 	return code == 429 || code >= 500
 })

Alternatively, you could use resty.Backoff with more specific conditions, or check for net.Error temporary errors.

internal/agent/loop.go (1)

264-271: Recursive processLLMRequest call could cause deep call stacks.

The handleToolCalls method recursively calls processLLMRequest after processing tool calls. If the LLM repeatedly returns tool calls, this could lead to deep recursion. Consider converting to an iterative approach.

♻️ Suggested iterative approach

-// handleToolCalls processes tool calls from the LLM response.
-func (l *Loop) handleToolCalls(ctx context.Context, toolCalls []llm.ToolCall) error {
-	for _, tc := range toolCalls {
-		l.logger.Debug("executing tool", "name", tc.Function.Name, "id", tc.ID)
-		l.recordToolResult(ctx, tc, l.executeTool(ctx, tc))
-	}
-	return l.processLLMRequest(ctx)
-}
+// handleToolCalls processes tool calls from the LLM response.
+// Returns true if another LLM request should be made.
+func (l *Loop) handleToolCalls(ctx context.Context, toolCalls []llm.ToolCall) bool {
+	for _, tc := range toolCalls {
+		l.logger.Debug("executing tool", "name", tc.Function.Name, "id", tc.ID)
+		l.recordToolResult(ctx, tc, l.executeTool(ctx, tc))
+	}
+	return true // Signal to continue with another LLM request
+}

Then modify processLLMRequest to return a flag and loop in the caller, or restructure as an internal loop within processLLMRequest.

internal/agent/conversation.go (1)

458-502: Channel send in SubmitUserResponse may silently drop response.

At lines 496-500, the select with default will return false if the channel already has a buffered response. Since the channel is created with buffer size 1 (line 461), a second response to the same prompt would be silently dropped.

This is likely intentional (first response wins), but consider logging when a response is dropped to aid debugging.

♻️ Optional: Add logging for dropped responses

 select {
 case ch <- response:
 	return true
 default:
+	// Response channel is full - prompt already answered
 	return false
 }

Or add debug logging in the ConversationManager's logger if available in this scope.

internal/cmd/flags.go (1)

340-356: Consider using baseFlags for consistency with initFlags.

initFlags uses baseFlags (4 flags), but bindFlags starts with only configFlag. While currently safe (only configFlag has bindViper: true among base flags), this is a maintenance hazard—if daguHomeFlag, quietFlag, or cpuProfileFlag later gains bindViper: true, a developer must remember to update this function separately.

Since the loop already filters on bindViper, using baseFlags here would be both consistent and safe.

♻️ Proposed fix for consistency

 func bindFlags(v *viper.Viper, cmd *cobra.Command, additionalFlags ...commandLineFlag) {
-	allFlags := append([]commandLineFlag{configFlag}, additionalFlags...)
+	allFlags := append(baseFlags, additionalFlags...)

 	for _, flag := range allFlags {
 		if !flag.bindViper {
 			continue
 		}

internal/output/tree.go (1)

252-270: Consider caching log content to avoid repeated file reads.

hasOutput() reads log files to check for content, and then renderOutputs() -> renderOutput() reads the same files again. For nodes with both stdout and stderr, ReadLogFileTail may be called multiple times for the same file paths.

Consider restructuring to read the log content once and pass it through the rendering pipeline, or cache the results to avoid repeated I/O operations—especially beneficial when rendering many nodes.

internal/cmd/status_test.go (1)

430-430: Avoid fixed sleep before reading sub‑run metadata.

A fixed 500ms sleep can be flaky on slower CI; consider waiting until the sub‑run appears.

♻️ Suggested approach

-		time.Sleep(500 * time.Millisecond)
+		require.Eventually(t, func() bool {
+			parentRef := exec.NewDAGRunRef(dagFile.Location, parentRunID)
+			parentAttempt, err := th.DAGRunStore.FindAttempt(th.Context, parentRef)
+			if err != nil {
+				return false
+			}
+			parentStatus, err := parentAttempt.ReadStatus(th.Context)
+			if err != nil || len(parentStatus.Nodes) == 0 {
+				return false
+			}
+			return len(parentStatus.Nodes[0].SubRuns) > 0
+		}, time.Second*3, time.Millisecond*50)

internal/agent/read.go (1)

104-131: Minor: Clarify offset=0 behavior.

When offset=0 is provided, max(0, 0-1) evaluates to 0, which starts from line 1. This is implicitly treating 0 as "start from beginning," but the description says offset is 1-based. Consider either:

1. Documenting that 0 means "start from beginning"
2. Treating 0 as invalid input

This is a minor edge case that doesn't break functionality.

internal/agent/subpub_test.go (1)

26-33: Replace fixed sleeps with explicit synchronization to reduce flakiness.

Several tests rely on time-based waits; on slow CI these can be brittle. Consider using channels/WaitGroups or polling with timeouts instead of fixed sleeps.

Also applies to: 74-81, 136-139, 162-170, 253-266

internal/agent/schema/output_test.go (1)

53-66: Prefer require for fatal assertions in the navigation loop.
This removes manual early returns and aligns the test style with the shared testing guidance.

Proposed refactor

 import (
 	"strings"
 	"testing"

-	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
@@
 		t.Run(tt.name, func(t *testing.T) {
 			result, err := DefaultRegistry.Navigate("dag", tt.path)
-			if (err != nil) != tt.wantErr {
-				t.Errorf("Navigate() error = %v, wantErr %v", err, tt.wantErr)
-				return
-			}
-			if err != nil {
-				return
-			}
+			if tt.wantErr {
+				require.Error(t, err)
+				return
+			}
+			require.NoError(t, err)

 			for _, want := range tt.want {
 				if !strings.Contains(result, want) {
 					t.Errorf("Navigate() result missing %q\nGot:\n%s", want, result)
 				}
 			}
 		})
 	}
 }

As per coding guidelines: Use stretchr/testify/require and shared fixtures from internal/test instead of duplicating mocks.

internal/agent/store_test.go (1)

9-60: Use require assertions for fail-fast test checks.
These assertions are prerequisites; switching to require avoids cascading failures and matches the testing guidance.

Proposed refactor (apply pattern throughout the file)

 import (
 	"testing"

-	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
@@
 	cfg := DefaultConfig()

-	assert.True(t, cfg.Enabled)
-	assert.Equal(t, DefaultProvider, cfg.LLM.Provider)
-	assert.Equal(t, DefaultModel, cfg.LLM.Model)
+	require.True(t, cfg.Enabled)
+	require.Equal(t, DefaultProvider, cfg.LLM.Provider)
+	require.Equal(t, DefaultModel, cfg.LLM.Model)
@@
-	assert.NotNil(t, ErrConversationNotFound)
-	assert.Contains(t, ErrConversationNotFound.Error(), "not found")
+	require.NotNil(t, ErrConversationNotFound)
+	require.Contains(t, ErrConversationNotFound.Error(), "not found")

As per coding guidelines: Use stretchr/testify/require and shared fixtures from internal/test instead of duplicating mocks.

internal/agent/system_prompt.txt (1)

93-97: Hyphenate “Root-level” for clarity.

Proposed edit

- - `path: ""` - Root level fields
+ - `path: ""` - Root-level fields

internal/agent/bash_test.go (1)

20-121: Prefer require for core Bash tool expectations.
These assertions gate the rest of each case; require makes failures clearer and aligns with test conventions.

Proposed refactor (apply pattern throughout the file)

 import (
 	"context"
 	"encoding/json"
 	"strings"
 	"testing"
 	"time"

-	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
@@
-	assert.False(t, result.IsError)
-	assert.Contains(t, result.Content, "hello")
+	require.False(t, result.IsError)
+	require.Contains(t, result.Content, "hello")

As per coding guidelines: Use stretchr/testify/require and shared fixtures from internal/test instead of duplicating mocks.

internal/agent/think_test.go (1)

10-45: Switch to require for the Think tool assertions.
This keeps the tests fail-fast and consistent with the team’s test guidance.

Proposed refactor

 import (
 	"encoding/json"
 	"testing"

-	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
@@
-		assert.False(t, result.IsError)
+		require.False(t, result.IsError)
 		if tc.wantContains != "" {
-			assert.Contains(t, result.Content, tc.wantContains)
+			require.Contains(t, result.Content, tc.wantContains)
 		}

As per coding guidelines: Use stretchr/testify/require and shared fixtures from internal/test instead of duplicating mocks.

internal/agent/read_test.go (1)

22-24: Consider using json.Marshal for safer JSON construction.

The readInput helper uses string concatenation to build JSON, which could produce invalid JSON if the path contains special characters like quotes or backslashes.

♻️ Suggested safer implementation

-func readInput(path string) json.RawMessage {
-	return json.RawMessage(`{"path": "` + path + `"}`)
+func readInput(path string) json.RawMessage {
+	data, _ := json.Marshal(map[string]string{"path": path})
+	return data
 }

internal/agent/think.go (1)

33-36: Consider validating the thought parameter.

The thinkRun function ignores the input entirely. While this is intentional for a no-op tool, consider adding minimal validation to ensure the thought parameter is provided and non-empty. This would give the LLM better feedback if it misuses the tool.

♻️ Optional: Add input validation

+type thinkInput struct {
+	Thought string `json:"thought"`
+}
+
 // thinkRun acknowledges the thought without performing any action.
-func thinkRun(_ ToolContext, _ json.RawMessage) ToolOut {
+func thinkRun(_ ToolContext, input json.RawMessage) ToolOut {
+	var in thinkInput
+	if err := json.Unmarshal(input, &in); err != nil {
+		return toolError("Failed to parse input: %v", err)
+	}
+	if in.Thought == "" {
+		return toolError("thought is required")
+	}
 	return ToolOut{Content: "Thought recorded. Continue with your plan."}
 }

internal/agent/api_test.go (2)

419-435: Stream test may be flaky due to time.Sleep synchronization.

The 50ms sleep is used to ensure the SSE handler has time to set headers before cancellation. This timing-based approach can be flaky under CI load or slow environments.

♻️ Consider a more robust synchronization approach

One option is to check for headers in a loop with a timeout instead of a fixed sleep:

 		done := make(chan struct{})
 		go func() {
 			defer close(done)
 			setup.router.ServeHTTP(rec, req)
 		}()

-		time.Sleep(50 * time.Millisecond)
+		// Wait for SSE headers to be set
+		deadline := time.Now().Add(time.Second)
+		for time.Now().Before(deadline) {
+			if rec.Header().Get("Content-Type") != "" {
+				break
+			}
+			time.Sleep(5 * time.Millisecond)
+		}
 		cancel()
 		<-done

---

488-498: The tc := tc pattern is now unnecessary in Go 1.22+.

Go 1.22 changed loop variable semantics so that each iteration gets its own variable. The tc := tc shadowing is no longer needed for closure captures in parallel subtests. However, keeping it doesn't cause any issues and maintains backward compatibility.

Also applies to: 603-611

internal/agent/patch_test.go (1)

26-32: Consider using json.Marshal for safer JSON construction.

Similar to readInput in read_test.go, this helper uses string formatting which could produce invalid JSON if path or values contain special characters like quotes.

♻️ Suggested safer implementation

 func patchInput(path, operation string, extra ...string) json.RawMessage {
-	base := fmt.Sprintf(`{"path": %q, "operation": %q`, path, operation)
-	for i := 0; i < len(extra)-1; i += 2 {
-		base += fmt.Sprintf(`, %q: %q`, extra[i], extra[i+1])
-	}
-	return json.RawMessage(base + "}")
+	m := map[string]string{"path": path, "operation": operation}
+	for i := 0; i < len(extra)-1; i += 2 {
+		m[extra[i]] = extra[i+1]
+	}
+	data, _ := json.Marshal(m)
+	return data
 }

internal/agent/loop_test.go (1)

406-413: Hardcoded tool count assertion is brittle.

The assertion assert.Len(t, tools, 8) will fail silently when tools are added or removed. Consider checking for expected tools by name or making this more flexible.

♻️ Suggested improvement

 func TestLoop_BuildToolDefinitions(t *testing.T) {
 	t.Parallel()

 	t.Run("converts agent tools to LLM tools", func(t *testing.T) {
 		t.Parallel()

 		loop := NewLoop(LoopConfig{
 			Provider: &mockLLMProvider{},
 			Tools:    CreateTools(""),
 		})

 		tools := loop.buildToolDefinitions()

-		assert.Len(t, tools, 8)
+		assert.NotEmpty(t, tools, "should have at least one tool")
 		for _, tool := range tools {
 			assert.Equal(t, "function", tool.Type)
 			assert.NotEmpty(t, tool.Function.Name)
 		}
 	})
 }

internal/agent/web_search.go (1)

107-118: Consider refining retry conditions for transient errors only.

The retry condition at line 113-114 retries on any error, which includes non-transient errors like DNS resolution failures or invalid URLs. Consider being more selective.

♻️ Suggested refinement

 func performSearch(ctx context.Context, query string, maxResults int) ([]SearchResult, error) {
 	client := resty.New().
 		SetTimeout(defaultWebSearchTimeout).
 		SetRetryCount(maxRetries).
 		SetRetryWaitTime(retryWaitTime).
 		AddRetryCondition(func(r *resty.Response, err error) bool {
-			if err != nil {
-				return true
-			}
 			code := r.StatusCode()
-			return code == 429 || code >= 500
+			// Retry on rate limiting or server errors
+			return code == 429 || code >= 500
 		})

Note: Removing the err != nil check means only HTTP status codes trigger retries, not connection errors. If retrying connection errors is desired, consider checking for specific error types (e.g., net.Error with Temporary() or Timeout()).

internal/cmn/config/loader_test.go (1)

724-757: Avoid double viper.Reset in loadWithEnv.

loadFromYAML already resets viper, so the extra reset is redundant.

♻️ Suggested simplification

 func loadWithEnv(t *testing.T, yamlContent string, env map[string]string) *Config {
 	t.Helper()
-	viper.Reset()
-
 	for k, v := range env {
 		t.Setenv(k, v)
 	}
 
 	return loadFromYAML(t, yamlContent)
 }

internal/agent/mocks_test.go (1)

13-273: Consider moving these mocks to internal/test fixtures for reuse.

If these mocks are shared across multiple test files, centralizing them in internal/test keeps fixtures consistent and avoids future duplication.

Based on learnings: Use stretchr/testify/require and shared fixtures from internal/test instead of duplicating mocks.

internal/cmn/config/loader.go (1)

1155-1308: Consider env bindings for new PathsConfig fields.

API keys, webhooks, and conversations dirs are configurable via YAML, but lack env bindings—adding them keeps parity for env-based overrides.

➕ Suggested env bindings

 	// Paths
 	{key: "paths.dagsDir", env: "DAGS", isPath: true},
 	{key: "paths.dagsDir", env: "DAGS_DIR", isPath: true},
 	{key: "paths.executable", env: "EXECUTABLE", isPath: true},
 	{key: "paths.logDir", env: "LOG_DIR", isPath: true},
 	{key: "paths.dataDir", env: "DATA_DIR", isPath: true},
 	{key: "paths.suspendFlagsDir", env: "SUSPEND_FLAGS_DIR", isPath: true},
 	{key: "paths.adminLogsDir", env: "ADMIN_LOG_DIR", isPath: true},
 	{key: "paths.baseConfig", env: "BASE_CONFIG", isPath: true},
 	{key: "paths.dagRunsDir", env: "DAG_RUNS_DIR", isPath: true},
 	{key: "paths.procDir", env: "PROC_DIR", isPath: true},
 	{key: "paths.queueDir", env: "QUEUE_DIR", isPath: true},
 	{key: "paths.serviceRegistryDir", env: "SERVICE_REGISTRY_DIR", isPath: true},
 	{key: "paths.usersDir", env: "USERS_DIR", isPath: true},
+	{key: "paths.apiKeysDir", env: "API_KEYS_DIR", isPath: true},
+	{key: "paths.webhooksDir", env: "WEBHOOKS_DIR", isPath: true},
+	{key: "paths.conversationsDir", env: "CONVERSATIONS_DIR", isPath: true},

internal/cmd/dry_test.go (1)

36-40: Minor inconsistency: missing closing bracket in expected output.

Line 39 expects [1=p5 2=p6 while line 34 expects [1=p3 2=p4] with the closing bracket. While both work for substring matching, the inconsistency reduces readability.

Suggested fix

 		{
 			Name:        "DryRunDAGWithParamsAfterDash",
 			Args:        []string{"dry", dagWithParams.Location, "--", "p5", "p6"},
-			ExpectedOut: []string{`[1=p5 2=p6`},
+			ExpectedOut: []string{`[1=p5 2=p6]`},
 		},

internal/cmn/fileutil/fileutil.go (1)

255-264: Consider fsync for crash-safe durability.

The current implementation does not call fsync on the temp file before renaming. On some filesystems (especially ext3/ext4 without barrier=1), the data may not be persisted to disk before the rename completes. If the system crashes, the file could be empty or contain partial data.

For truly atomic and durable writes, call Sync() on the file handle before closing/renaming.

♻️ Add fsync for durability (if crash safety is required)

 func WriteFileAtomic(filePath string, data []byte, perm os.FileMode) error {
-	tempPath := filePath + ".tmp"
-	if err := os.WriteFile(tempPath, data, perm); err != nil {
-		return fmt.Errorf("failed to write temp file %s: %w", tempPath, err)
+	dir := filepath.Dir(filePath)
+	f, err := os.CreateTemp(dir, ".tmp-*")
+	if err != nil {
+		return fmt.Errorf("failed to create temp file: %w", err)
+	}
+	tempPath := f.Name()
+	defer func() { _ = os.Remove(tempPath) }() // cleanup on any failure path
+	
+	if _, err := f.Write(data); err != nil {
+		_ = f.Close()
+		return fmt.Errorf("failed to write temp file %s: %w", tempPath, err)
+	}
+	if err := f.Sync(); err != nil {
+		_ = f.Close()
+		return fmt.Errorf("failed to sync temp file %s: %w", tempPath, err)
+	}
+	if err := f.Close(); err != nil {
+		return fmt.Errorf("failed to close temp file %s: %w", tempPath, err)
+	}
+	if err := os.Chmod(tempPath, perm); err != nil {
+		return fmt.Errorf("failed to set permissions on %s: %w", tempPath, err)
 	}
 	if err := os.Rename(tempPath, filePath); err != nil {
-		_ = os.Remove(tempPath)
 		return fmt.Errorf("failed to rename %s to %s: %w", tempPath, filePath, err)
 	}
+	// Remove the deferred cleanup since rename succeeded
 	return nil
 }

Note: This is a nice-to-have improvement. If the use case doesn't require crash safety (e.g., configuration files that can be regenerated), the current implementation is acceptable.

internal/cmn/fileutil/atomic_test.go (1)

55-66: Permission test may be flaky on Windows or with umask interference.

On Unix systems, the actual file permissions depend on the process umask. On Windows, file permissions work differently. This test may pass locally but fail in certain CI environments.

The codebase already applies this pattern in internal/runtime/builtin/docker/keepalive_test.go (lines 156–157), where permission assertions are guarded with if runtime.GOOS != "windows". Consider adopting the same approach here to improve test reliability across platforms.

♻️ Consider adding Windows skip

 	t.Run("sets correct permissions", func(t *testing.T) {
 		t.Parallel()
+		if runtime.GOOS == "windows" {
+			t.Skip("file permissions work differently on Windows")
+		}
 		dir := t.TempDir()
 		filePath := filepath.Join(dir, "test.txt")

 		err := WriteFileAtomic(filePath, []byte("data"), 0644)
 		require.NoError(t, err)

 		info, err := os.Stat(filePath)
 		require.NoError(t, err)
 		assert.Equal(t, os.FileMode(0644), info.Mode().Perm())
 	})

Note: You will also need to add "runtime" to the imports at the top of the file.

internal/core/dag.go (1)

398-401: Non-deterministic iteration order when appending env vars.

The for k, v := range vars loop at line 398 iterates over a map, which has non-deterministic order in Go. While this doesn't affect functionality (environment variables are key-value pairs without ordering requirements), it may cause non-deterministic d.Env slice order across runs.

This is unlikely to cause issues in practice since env vars are typically accessed by key, not position.

Makefile (1)

119-123: Allow DAGU_DEBUG to be overridden.

Hard-coding debug can prevent quiet runs; consider defaulting to 1 but allowing callers to override.

🔧 Suggested tweak

-	`@DAGU_DEBUG`=1 go run ./cmd start-all
+	`@DAGU_DEBUG`=$${DAGU_DEBUG:-1} go run ./cmd start-all

internal/cmd/flags.go (2)

319-325: Potential slice mutation when appending to baseFlags.

Using append(baseFlags, additionalFlags...) directly may mutate the backing array of baseFlags if its capacity exceeds length. This could cause subtle bugs if initFlags is called concurrently or multiple times with different flags.

♻️ Proposed fix to prevent slice mutation

 func initFlags(cmd *cobra.Command, additionalFlags ...commandLineFlag) {
-	allFlags := append(baseFlags, additionalFlags...)
+	allFlags := make([]commandLineFlag, 0, len(baseFlags)+len(additionalFlags))
+	allFlags = append(allFlags, baseFlags...)
+	allFlags = append(allFlags, additionalFlags...)

 	for _, flag := range allFlags {
 		registerFlag(cmd, flag)
 	}
 }

---

340-355: Same slice append pattern in bindFlags.

Similar to initFlags, this function should also avoid potential mutation of shared slices.

♻️ Proposed fix

 func bindFlags(v *viper.Viper, cmd *cobra.Command, additionalFlags ...commandLineFlag) {
-	allFlags := append([]commandLineFlag{configFlag}, additionalFlags...)
+	allFlags := make([]commandLineFlag, 0, 1+len(additionalFlags))
+	allFlags = append(allFlags, configFlag)
+	allFlags = append(allFlags, additionalFlags...)

 	for _, flag := range allFlags {

internal/agent/subpub_test.go (1)

339-356: Unused cancel parameter in collectMessages helper.

The cancel parameter is passed but never used within the function. It can be removed to simplify the signature.

🔧 Proposed fix

 // collectMessages starts a goroutine that collects all messages from the next function
 // until the context is canceled. Returns a channel that receives the collected slice.
-func collectMessages[K any](ctx context.Context, cancel context.CancelFunc, next func() (K, bool)) <-chan []K {
+func collectMessages[K any](next func() (K, bool)) <-chan []K {
 	result := make(chan []K, 1)
 	go func() {
 		var msgs []K
 		for {
 			msg, ok := next()
 			if !ok {
 				result <- msgs
 				close(result)
 				return
 			}
 			msgs = append(msgs, msg)
 		}
 	}()
 	return result
 }

Update call sites accordingly (Lines 72, 92).

internal/cmn/config/config.go (1)

417-436: Consider extracting role names as constants.

The valid role names ("admin", "manager", "operator", "viewer") are hardcoded in the switch statement. Extracting these as package-level constants would improve maintainability and allow reuse in other validation contexts.

♻️ Proposed refactor

// At package level:
const (
    RoleAdmin    = "admin"
    RoleManager  = "manager"
    RoleOperator = "operator"
    RoleViewer   = "viewer"
)

var ValidRoles = []string{RoleAdmin, RoleManager, RoleOperator, RoleViewer}

Then use slices.Contains(ValidRoles, oidc.RoleMapping.DefaultRole) in validation.

internal/agent/bash_test.go (1)

17-122: Prefer require for gating assertions in tests.

Several checks are prerequisites for later assertions; using require avoids follow‑on noise when a precondition fails and aligns with test guidelines.

internal/agent/schema/output_test.go (1)

9-68: Consider require for error gating.

Using require.NoError / require.Error will align with test guidelines and reduce manual error branching.

internal/agent/navigate_test.go (1)

10-80: Prefer require for precondition checks.

For example, require.False(t, result.IsError) before content checks keeps failures clean and matches test guidelines.

internal/agent/schema/registry_test.go (2)

543-559: Capture loop vars in parallel subtests (Go <1.22).
Line 543 runs t.Parallel() inside a range loop; on Go <1.22 this can capture the last value. Same pattern appears in other table-driven tests in this file.

♻️ Suggested fix

-	for _, tt := range tests {
+	for _, tt := range tests {
+		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()

---

1002-1039: Avoid potential deadlock if many errors are emitted.
Line 1003 sizes errCh to numGoroutines*numOps, but Register + Navigate can emit up to 2× that. If failures happen, goroutines can block.

♻️ Suggested fix

-	errCh := make(chan error, numGoroutines*numOps)
+	errCh := make(chan error, numGoroutines*numOps*2)

internal/agent/navigate.go (1)

47-65: Consider validating the navigation path.
Line 53 accepts any string while the description enumerates supported routes. A minimal guard (absolute UI path, no scheme) avoids unexpected navigation.

♻️ Suggested guard

-	if args.Path == "" {
+	path := strings.TrimSpace(args.Path)
+	if path == "" {
 		return toolError("Path is required")
 	}
+
+	if !strings.HasPrefix(path, "/") || strings.Contains(path, "://") {
+		return toolError("Path must be an absolute UI path starting with '/'")
+	}
 ...
-			Path: args.Path,
+			Path: path,
 		})
 	}
 
-	return ToolOut{Content: fmt.Sprintf("Navigating user to %s", args.Path)}
+	return ToolOut{Content: fmt.Sprintf("Navigating user to %s", path)}

📦 Import update

 import (
 	"encoding/json"
 	"fmt"
+	"strings"
 
 	"github.com/dagu-org/dagu/internal/llm"
 )

internal/agent/schema/registry.go (1)

35-53: Consider holding the read lock during navigation.

The Navigate method releases the read lock immediately after retrieving the schema from the map. If the schema map is modified concurrently (e.g., another goroutine calls Register with the same name), the navigator could be working with stale or inconsistent data, though the current map semantics make this unlikely to cause corruption.

For stronger consistency guarantees, consider holding the lock for the duration of navigation or documenting that schemas should not be modified after initial registration.

internal/agent/loop_test.go (2)

395-413: Avoid hardcoding the expected tool count.

The assertion assert.Len(t, tools, 8) will break whenever tools are added or removed. Consider asserting on minimum expected tools or checking for specific tool names instead.

♻️ Suggested fix

 func TestLoop_BuildToolDefinitions(t *testing.T) {
 	t.Parallel()

 	t.Run("converts agent tools to LLM tools", func(t *testing.T) {
 		t.Parallel()

 		loop := NewLoop(LoopConfig{
 			Provider: &mockLLMProvider{},
 			Tools:    CreateTools(""),
 		})

 		tools := loop.buildToolDefinitions()

-		assert.Len(t, tools, 8)
+		assert.NotEmpty(t, tools, "expected at least one tool")
 		for _, tool := range tools {
 			assert.Equal(t, "function", tool.Type)
 			assert.NotEmpty(t, tool.Function.Name)
 		}
 	})
 }

---

41-56: Testing internal state via direct mutex access.

Accessing loop.mu and loop.messageQueue directly couples tests to internal implementation details. If the internal structure changes, these tests will break even if the public API behavior is correct.

Consider exposing a method like PendingMessageCount() for testing, or accept this as a pragmatic trade-off for test coverage.

internal/agent/read.go (2)

104-117: Clarify offset=0 behavior in documentation or validation.

The conversion start := max(0, requestedOffset-1) means both offset=0 and offset=1 result in reading from line 1. This may confuse users expecting 0-based indexing. The description says "1-based" but doesn't clarify what happens with offset=0.

Consider either:

1. Treating offset=0 as invalid (since it's documented as 1-based)
2. Explicitly documenting that offset ≤ 1 starts from the beginning

📝 Option: Validate that offset is 1-based

 func formatFileContent(content string, requestedOffset, requestedLimit int) ToolOut {
 	lines := strings.Split(content, "\n")
 	total := len(lines)

+	if requestedOffset < 0 {
+		return toolError("Offset must be positive (1-based line number)")
+	}
+
 	// Convert 1-based offset to 0-based index
 	start := max(0, requestedOffset-1)

---

72-77: Entire file is read into memory before applying offset/limit.

For files approaching maxReadSize (1MB), the entire content is loaded into memory even if only a small portion is requested via offset/limit. For the current 1MB limit, this is acceptable, but be aware this could become a concern if the limit increases.

internal/agent/api_test.go (2)

52-60: Handle potential JSON marshal errors in test helper.

postJSON ignores the error from json.Marshal. While unlikely to fail with test data, errors would cause confusing test failures.

♻️ Suggested fix

 // postJSON sends a POST request with JSON body and returns the recorder.
-func (s *apiTestSetup) postJSON(path string, body any) *httptest.ResponseRecorder {
-	data, _ := json.Marshal(body)
+func (s *apiTestSetup) postJSON(t *testing.T, path string, body any) *httptest.ResponseRecorder {
+	t.Helper()
+	data, err := json.Marshal(body)
+	require.NoError(t, err, "failed to marshal request body")
 	req := httptest.NewRequest(http.MethodPost, path, bytes.NewReader(data))
 	req.Header.Set("Content-Type", "application/json")
 	rec := httptest.NewRecorder()
 	s.router.ServeHTTP(rec, req)
 	return rec
 }

Note: This would require updating all call sites to pass t.

---

413-436: SSE streaming test uses time.Sleep which can be flaky.

The test relies on time.Sleep(50 * time.Millisecond) to allow the SSE handler to set headers before cancellation. This could be flaky under load. Consider using a synchronization mechanism if this becomes unreliable.

internal/agent/mocks_test.go (1)

117-122: Private helper requireConversation assumes caller holds the lock.

requireConversation accesses m.conversations without acquiring the mutex. This is correct because all callers acquire the lock first, but it's fragile. Consider adding a comment documenting this precondition.

📝 Suggested documentation

+// requireConversation checks if a conversation exists.
+// REQUIRES: m.mu must be held by the caller.
 func (m *mockConversationStore) requireConversation(id string) error {
 	if _, exists := m.conversations[id]; !exists {
 		return ErrConversationNotFound
 	}
 	return nil
 }

internal/agent/patch.go (1)

108-121: patchCreate overwrites existing files without warning.

The function creates or overwrites files silently. Consider checking if the file exists and either warning the user or requiring explicit confirmation for overwrites, which could prevent accidental data loss.

internal/agent/web_search.go (2)

107-134: Scraping DuckDuckGo HTML is fragile and may break without warning.

This implementation relies on DuckDuckGo's HTML structure (class names like result, result__a, result__snippet). If DuckDuckGo changes their markup, the tool will silently return no results.

Consider adding observability (logging when zero results are parsed from non-empty HTML) to detect breakage, or document this fragility for maintainers.

---

112-118: Retry condition may cause excessive retries on non-transient errors.

The retry condition returns true for any error, including non-transient errors like DNS failures or invalid URLs. Consider being more selective:

♻️ Suggested improvement

 		AddRetryCondition(func(r *resty.Response, err error) bool {
 			if err != nil {
-				return true
+				// Only retry on timeout-related errors
+				return r == nil // Connection failed, worth retrying
 			}
 			code := r.StatusCode()
 			return code == 429 || code >= 500
 		})

internal/agent/bash.go (1)

71-74: Consider using absolute path for bash executable.

Using "bash" relies on PATH resolution. For consistency and to avoid potential PATH manipulation in edge cases, consider using /bin/bash or /usr/bin/bash.

♻️ Suggested change

-	cmd := exec.CommandContext(ctx, "bash", "-c", args.Command)
+	cmd := exec.CommandContext(ctx, "/bin/bash", "-c", args.Command)

internal/agent/conversation.go (2)

459-483: createWaitUserResponseFunc has proper cleanup but could leak if context is never cancelled.

The defer cleanup ensures the pending prompt is removed, but if the context is never cancelled and no response arrives, the goroutine will block indefinitely. Consider adding a timeout or documenting this behavior.

---

496-501: SubmitUserResponse silently fails if channel is full.

The non-blocking send with default case returns false if the channel already has a pending message. Since the channel has buffer size 1, this could happen if a response is submitted twice for the same prompt.

This is likely acceptable behavior (idempotent rejection of duplicate responses), but consider logging a warning for debuggability:

♻️ Suggested improvement

 	select {
 	case ch <- response:
 		return true
 	default:
+		// Channel already has a response or was closed
 		return false
 	}

internal/cmn/config/loader.go (2)

135-146: parseDuration silently defaults to zero on invalid input.

Returning 0 and adding a warning may mask configuration errors. Depending on the field, a zero duration could cause unexpected behavior (e.g., no retry interval). Consider whether some duration fields should fail-fast instead.

---

1047-1058: Legacy environment mappings for path sections use SectionNone.

Lines 1047, 1052, 1057 use requires: SectionNone which means these will be skipped when the section check runs (since SectionNone = 0 and req & 0 = 0). This appears intentional to always apply these path mappings, but the logic at lines 1062-1064 checks mapping.requires != SectionNone to skip the requires check entirely for SectionNone entries.

This is correct but subtle - a brief comment might help future maintainers.