internal/service/scheduler/scheduler.go (1)

705-705: Please add a minute-boundary characterization test for NextTick.

Current coverage (in internal/service/scheduler/scheduler_test.go Line 124-133) checks 01:00:50 -> 01:01:00. Add 01:00:00 -> 01:01:00 (and ideally a DST transition case) to lock in intended semantics.

🤖 Prompt for AI Agents

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

In `@internal/service/scheduler/scheduler.go` at line 705, Add a unit test for
NextTick that asserts a time already on a minute boundary advances to the next
minute (e.g., input 01:00:00 -> expected 01:01:00) and include an additional
test that covers a DST boundary if possible; locate the scheduler.NextTick call
(function that returns now.Truncate(time.Minute).Add(time.Minute)) and in
internal/service/scheduler/scheduler_test.go add a test case constructing the
source time (use time.Date with the appropriate location/zone for DST) and
assert equality with the expected next-minute value.

internal/core/step.go (1)

191-196: Copy legacy args slice when constructing CommandEntry.

At Line 194, Args: s.Args reuses the same backing array. A later mutation of deprecated Step.Args can unintentionally affect Commands[0].Args.

Suggested patch

 func (s *Step) legacyCommandEntry() CommandEntry {
 	return CommandEntry{
 		Command:     s.Command,
-		Args:        s.Args,
+		Args:        append([]string(nil), s.Args...),
 		CmdWithArgs: s.CmdWithArgs,
 	}
 }

🤖 Prompt for AI Agents

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

In `@internal/core/step.go` around lines 191 - 196, The legacyCommandEntry method
is returning CommandEntry with Args pointing to the same backing slice as
Step.Args which allows later mutations to Step.Args to affect the stored
Commands; update legacyCommandEntry (the function named legacyCommandEntry that
returns CommandEntry) to copy the slice contents when setting Args (e.g.,
allocate a new []string and copy or use append to copy) so CommandEntry.Args is
independent of Step.Args.

Fix all unresolved CodeRabbit comments on this PR:

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

internal/runtime/agent/reporter_test.go (1)

456-460: Consider expanding to table-driven coverage for statusBadgeClass.

The new test only exercises the "partially_succeeded" branch. Since statusBadgeClass has 9 distinct branches (including the default empty-string case), a table-driven test would provide stronger regression protection at minimal cost.

♻️ Proposed table-driven version

-func TestStatusBadgeClassPartiallySucceeded(t *testing.T) {
-	t.Parallel()
-
-	require.Equal(t, "success", statusBadgeClass("partially_succeeded"))
-}
+func TestStatusBadgeClass(t *testing.T) {
+	t.Parallel()
+
+	cases := map[string]string{
+		"finished":            "success",
+		"succeeded":           "success",
+		"partially_succeeded": "success",
+		"failed":              "failed",
+		"running":             "running",
+		"skipped":             "skipped",
+		"aborted":             "aborted",
+		"waiting":             "wait",
+		"unknown":             "",
+	}
+	for status, want := range cases {
+		status, want := status, want
+		t.Run(status, func(t *testing.T) {
+			t.Parallel()
+			require.Equal(t, want, statusBadgeClass(status))
+		})
+	}
+}

🤖 Prompt for AI Agents

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

In `@internal/runtime/agent/reporter_test.go` around lines 456 - 460, The test
TestStatusBadgeClassPartiallySucceeded only checks one branch; change it to a
table-driven test that enumerates all known status inputs and expected outputs
for statusBadgeClass (including all 9 distinct branches and the
default/empty-string case), iterate the table with t.Run subtests (calling
t.Parallel() per subtest if desired) and assert each mapping with require.Equal,
so the behavior of statusBadgeClass is fully covered; keep the test named to
reflect table-driven style (e.g., TestStatusBadgeClass) and reference
statusBadgeClass in each subtest.

internal/core/step.go (1)

191-197: Optional: use slices.Clone for the defensive Args copy.

append([]string(nil), s.Args...) is correct (and yields nil when s.Args is nil, which is fine here), but slices.Clone(s.Args) from the standard library is more idiomatic and reads more directly as "defensive copy". Purely a readability preference — feel free to skip.

♻️ Proposed refactor

 func (s *Step) legacyCommandEntry() CommandEntry {
 	return CommandEntry{
 		Command:     s.Command,
-		Args:        append([]string(nil), s.Args...),
+		Args:        slices.Clone(s.Args),
 		CmdWithArgs: s.CmdWithArgs,
 	}
 }

Requires adding "slices" to the import block.

🤖 Prompt for AI Agents

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

In `@internal/core/step.go` around lines 191 - 197, Replace the manual defensive
copy of s.Args in Step.legacyCommandEntry with the idiomatic slices.Clone call:
use slices.Clone(s.Args) for the Args field, and add "slices" to the imports;
keep the rest of the returned CommandEntry (Command and CmdWithArgs) unchanged
to preserve behavior when s.Args is nil.

internal/cmn/mailer/mailer.go (6)

347-364: Pre-existing duplicate Content-Transfer-Encoding header in attachments (out of scope, flagging for awareness).

Unrelated to your refactor, but each attachment block writes Content-Transfer-Encoding: base64 twice — once at Line 354 and again at Line 359. Some strict MIME parsers reject duplicate Content-Transfer-Encoding headers per RFC 2045 §6.4. The base64 switch on Line 360 itself is a clean simplification; consider removing one of the duplicate header lines in a follow-up.

♻️ Follow-up cleanup

 			_, _ = fmt.Fprintf(&buf, "\r\n\n--%s\r\n", boundary)
 			_, _ = buf.WriteString("Content-Type: text/plain;" + "\r\n")
-			_, _ = buf.WriteString("Content-Transfer-Encoding: base64" + "\r\n")
 			_, _ = buf.WriteString(
 				"Content-Disposition: attachment; filename=" +
 					filepath.Base(fileName) + "\r\n",
 			)
 			_, _ = buf.WriteString("Content-Transfer-Encoding: base64\r\n\n")

🤖 Prompt for AI Agents

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

In `@internal/cmn/mailer/mailer.go` around lines 347 - 364, The attachment builder
in addAttachments writes the "Content-Transfer-Encoding: base64" header twice
for each attachment which can break strict MIME parsers; update addAttachments
to emit that header only once per attachment (remove the duplicate write call —
the second occurrence just before writing the base64 payload) while keeping the
existing Content-Type, Content-Disposition, boundary usage, and base64 encoding
of data intact so each attachment block contains a single
Content-Transfer-Encoding header.

---

183-196: Helpers are fine; consider making sanitizeHeaderField a touch stricter.

replacer currently strips \r, \n, and their percent-encoded forms, which is sufficient for the CRLF-injection scenario the new test covers. If you want defense-in-depth against more exotic injection vectors, you could also strip other C0 controls (\x00–\x1f except tab, plus \x7f) and truncate absurdly long values. Not required — the current behavior matches the test contract.

🤖 Prompt for AI Agents

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

In `@internal/cmn/mailer/mailer.go` around lines 183 - 196, Update
sanitizeHeaderField to be stricter: after applying replacer.Replace(value) also
remove remaining C0 control characters (bytes 0x00–0x1F and 0x7F) except
horizontal tab, and truncate the result to a reasonable max length (e.g., 256
chars) to avoid absurdly long header values; reference the sanitizeHeaderField
function and the replacer variable so you locate the code, and keep
sanitizeAddresses behavior unchanged.

---

135-178: Inconsistent error wrapping between useAuth and no-auth paths.

Every SMTP step (MAIL FROM, RCPT TO, DATA, body write, wc.Close) wraps errors with a descriptive message only when useAuth is true, and returns the bare error otherwise. That asymmetry was inherited from the previous split functions, but now that both paths share one implementation there's no reason the no-auth path should produce worse diagnostics — operators will see read tcp ... i/o timeout instead of MAIL FROM failed: ..., making field triage of unauthenticated relay failures noticeably harder.

Also note prepareSession at Line 135 always wraps errors (HELO failed, STARTTLS failed, authentication failed) regardless of useAuth, so the wrapping policy within this single function is already internally inconsistent.

Consider wrapping unconditionally and dropping the useAuth branches:

♻️ Proposed simplification

-	if err := c.Mail(safeFrom); err != nil {
-		if useAuth {
-			return fmt.Errorf("MAIL FROM failed: %w", err)
-		}
-		return err
-	}
+	if err := c.Mail(safeFrom); err != nil {
+		return fmt.Errorf("MAIL FROM failed: %w", err)
+	}
 	for _, recipient := range recipients {
-		if err := c.Rcpt(recipient); err != nil {
-			if useAuth {
-				return fmt.Errorf("RCPT TO failed: %w", err)
-			}
-			return err
-		}
+		if err := c.Rcpt(recipient); err != nil {
+			return fmt.Errorf("RCPT TO failed: %w", err)
+		}
 	}

 	wc, err := c.Data()
 	if err != nil {
-		if useAuth {
-			return fmt.Errorf("DATA command failed: %w", err)
-		}
-		return err
+		return fmt.Errorf("DATA command failed: %w", err)
 	}

🤖 Prompt for AI Agents

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

In `@internal/cmn/mailer/mailer.go` around lines 135 - 178, The code currently
only wraps SMTP errors (MAIL FROM, RCPT TO, DATA, write, Close) when useAuth is
true, causing inconsistent/poorer diagnostics for non-auth paths; update the
send flow (the block after prepareSession including calls to c.Mail, c.Rcpt,
c.Data, wc.Write, wc.Close and the payload construction via
composeMail/processEmailBody) to always wrap returned errors with descriptive
context (e.g., "MAIL FROM failed", "RCPT TO failed", "DATA command failed",
"failed to write email body", "failed to close data writer") and remove the
conditional branches that return the bare err based on useAuth so error wrapping
is consistent with prepareSession.

---

309-324: composeMail switch to bytes.Buffer is cleaner; minor redundant CRLF sequence.

Moving to bytes.Buffer + base64.StdEncoding.EncodeToString is a nice simplification over the prior byte-slice juggling. One small thing: Lines 319–322 emit "\r\n\r\n--", boundary, "--\r\n\r\n", then another "\r\n\r\n". The trailing extra CRLF pair is probably leftover from the old formatting — not harmful, but redundant for a multipart epilogue. You can drop Line 322 if you want a tighter payload.

♻️ Optional tightening

 	buf.WriteString("\r\n\r\n--")
 	buf.WriteString(boundary)
 	buf.WriteString("--\r\n\r\n")
-	buf.WriteString("\r\n\r\n")
 	return buf.Bytes()

🤖 Prompt for AI Agents

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

In `@internal/cmn/mailer/mailer.go` around lines 309 - 324, composeMail assembles
the multipart payload but writes an extra trailing CRLF pair after closing the
final boundary; remove the final buf.WriteString("\r\n\r\n") so the epilogue
ends with "\r\n\r\n--" + boundary + "--\r\n\r\n" (i.e., keep composeHeader,
base64 encoding of body, addAttachments and the boundary close but drop the
redundant trailing CRLF write) to produce a tighter multipart payload.

---

292-307: Double sanitization is harmless but makes the contract ambiguous.

composeHeader now sanitizes to, from, and subject internally (Lines 295–297), but callers in send (Line 165) already pass pre-sanitized values (safeFrom, safeSubject, and recipients from sanitizeAddresses). replacer.Replace is idempotent, so this is correct; however, having sanitization happen in two places makes it unclear which layer owns the invariant.

Preference: keep sanitization in composeHeader (defense-in-depth at the lowest layer closest to the header string) and simplify the call site in send to pass the raw values — or vice versa, remove it from composeHeader and rely on the caller. Either is fine; picking one avoids the "which layer is authoritative?" question for future readers.

🤖 Prompt for AI Agents

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

In `@internal/cmn/mailer/mailer.go` around lines 292 - 307, composeHeader
currently re-sanitizes to/from/subject even though send already sanitizes them;
keep sanitization in composeHeader (defense-in-depth) and simplify send by
passing raw values instead of pre-sanitized ones: in the send function stop
calling sanitizeAddresses for the recipients passed into composeHeader (remove
creation/use of recipients variable derived from sanitizeAddresses) and pass the
original to/from/subject variables (remove safeFrom/safeSubject usage), leaving
composeHeader to call sanitizeAddresses and sanitizeHeaderField as it currently
does; update any related variable names in send to reflect raw inputs and ensure
tests/other callers still supply unsanitized strings if they relied on previous
behavior.

---

80-100: These are test-only helpers — consider moving to *_test.go for clarity.

Both sendWithNoAuth and sendWithAuth are called exclusively from tests (mailer_test.go). Since they're internal test seams with no production callers, moving them to the test file would make their intent clearer and avoid confusion about whether they're part of the public API.

🤖 Prompt for AI Agents

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

In `@internal/cmn/mailer/mailer.go` around lines 80 - 100, sendWithNoAuth and
sendWithAuth are test-only helpers and should be moved into the test file to
clarify intent: remove these two functions from mailer.go and paste them into
mailer_test.go (keeping them unexported and in the same package so they can
access Client and mailTimeout), update any test imports or references if
necessary, and run tests to ensure visibility of Client, mailTimeout, and the
methods remain correct when moved to the _test.go file.

internal/cmn/mailer/mailer_test.go (1)

203-221: LGTM — good coverage for header injection prevention.

The test correctly validates that CRLF sequences in to/from/subject are stripped so they cannot split out new header lines, and it pins the resulting "flattened" form. Parallel execution is appropriate since this path has no shared state.

One small observation: this only exercises composeMail directly. Consider also adding a characterization around Send's recipients/safeFrom/safeSubject wiring (e.g., via the existing mock SMTP server) so future refactors of the send pipeline can't reintroduce an injection path upstream of composeMail. Non-blocking.

🤖 Prompt for AI Agents

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

In `@internal/cmn/mailer/mailer_test.go` around lines 203 - 221, The current test
only exercises composeMail directly; add an integration-style test that
exercises Send to ensure its recipients, safeFrom, and safeSubject plumbing
can't reintroduce header injection upstream of composeMail: create a test (e.g.,
TestSendSanitizesHeaders) that initializes the mailer via New(Config{}), starts
the existing mock SMTP server used in other tests, calls client.Send with
malicious values for to/from/subject containing CRLF and injected headers, and
then inspect the SMTP-received payload to assert it does not contain
CRLF-injected header lines and instead contains the flattened forms (reference
composeMail, Send, New, Config to locate code paths to validate).

internal/cmn/sock/server.go (1)

78-109: Consider tracking in-flight serveConn goroutines for graceful shutdown.

Shutdown closes the listener and returns immediately; any goroutines spawned at line 86 keep running with no way for callers to wait for them to drain. A panic in the handler also tears down the whole process since serveConn has no recover. Both are pre-existing traits carried over by the refactor, but this PR is a natural point to tighten them up if desired.

♻️ Sketch: WaitGroup + panic guard

 type Server struct {
 	addr        string
 	handlerFunc HTTPHandlerFunc
 	listener    net.Listener
 	quit        atomic.Bool
 	mu          sync.Mutex
+	wg          sync.WaitGroup
 }
@@
-		go srv.serveConn(ctx, conn)
+		srv.wg.Add(1)
+		go srv.serveConn(ctx, conn)
 	}
 }
@@
 func (srv *Server) serveConn(ctx context.Context, conn net.Conn) {
+	defer srv.wg.Done()
 	defer func() {
+		if r := recover(); r != nil {
+			logger.Error(ctx, "panic in serveConn", tag.Any("panic", r))
+		}
 		_ = conn.Close()
 	}()

And have Shutdown (or Serve's deferred cleanup) call srv.wg.Wait() before returning.

🤖 Prompt for AI Agents

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

In `@internal/cmn/sock/server.go` around lines 78 - 109, The server currently
spawns serveConn goroutines that aren't tracked and lack panic recovery, so
Shutdown/Serve can't wait for them and a handler panic can crash the process;
add a sync.WaitGroup field (e.g., srv.wg) to increment in Serve before launching
each goroutine and defer wg.Done() inside serveConn, add a defer recover()
wrapper at the top of serveConn to log and swallow panics, and make Shutdown (or
Serve's deferred cleanup) call srv.wg.Wait() before returning to ensure graceful
drain.

internal/runtime/node.go (1)

541-557: Guard against empty subRuns in the non-parallel branch.

runParams(subRuns[0]) at Line 547 will panic if subRuns is empty. Today BuildSubDAGRuns always returns at least one element for the non-parallel path and validates len(items) > 0 for parallel, so this is safe in practice — but the helper now silently relies on that invariant. Consider an explicit guard so a future change to BuildSubDAGRuns can't turn this into a hard crash inside setupExecutor.

🛡️ Proposed defensive guard

 func (n *Node) configureSubDAGExecutor(cmd executor.Executor, subRuns []SubDAGRun) error {
+	if len(subRuns) == 0 {
+		return fmt.Errorf("no sub DAG runs to configure for step %q", n.Name())
+	}
 	if n.Step().Parallel == nil {
 		dagExecutor, ok := cmd.(executor.DAGExecutor)
 		if !ok {
 			return fmt.Errorf("executor %T does not support sub DAG execution", cmd)
 		}
 		dagExecutor.SetParams(runParams(subRuns[0]))
 		return nil
 	}

🤖 Prompt for AI Agents

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

In `@internal/runtime/node.go` around lines 541 - 557, In configureSubDAGExecutor,
guard the non-parallel branch against an empty subRuns slice before calling
runParams(subRuns[0]) to avoid a panic: check len(subRuns) == 0 and return a
clear error (e.g., fmt.Errorf("no subRuns provided for non-parallel executor"))
if empty; otherwise proceed to cast cmd to executor.DAGExecutor and call
SetParams(runParams(subRuns[0])); keep the parallel branch unchanged (still uses
executor.ParallelExecutor and runParamsList).

internal/runtime/node_test.go (1)

171-177: Optional: assert Cancel is a no-op for non-running, non-waiting states.

This subtest covers the new NodeWaiting branch nicely, but the past review thread on this method also called out that Cancel should be precise about what it transitions. Consider one or two extra cases (e.g. NodeSucceeded, NodeNotStarted) asserting the status is left untouched, so any future broadening/narrowing of the switch is caught here.

♻️ Proposed table-style expansion

-	t.Run("CancelWaiting", func(t *testing.T) {
-		t.Parallel()
-
-		node := runtime.NewNode(core.Step{Name: core.NodeWaiting.String()}, runtime.NodeState{Status: core.NodeWaiting})
-		node.Cancel()
-		require.Equal(t, core.NodeAborted, node.State().Status)
-	})
+	t.Run("Cancel", func(t *testing.T) {
+		t.Parallel()
+		cases := []struct {
+			name     string
+			initial  core.NodeStatus
+			expected core.NodeStatus
+		}{
+			{"Running", core.NodeRunning, core.NodeAborted},
+			{"Waiting", core.NodeWaiting, core.NodeAborted},
+			{"Succeeded", core.NodeSucceeded, core.NodeSucceeded},
+			{"NotStarted", core.NodeNotStarted, core.NodeNotStarted},
+		}
+		for _, tc := range cases {
+			t.Run(tc.name, func(t *testing.T) {
+				node := runtime.NewNode(core.Step{Name: "cancel"}, runtime.NodeState{Status: tc.initial})
+				node.Cancel()
+				require.Equal(t, tc.expected, node.State().Status)
+			})
+		}
+	})

As per coding guidelines: "Use stretchr/testify assertions for testing in Go".

🤖 Prompt for AI Agents

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

In `@internal/runtime/node_test.go` around lines 171 - 177, Add assertions to the
CancelWaiting subtest ensuring Node.Cancel is a no-op for states other than
running/waiting: instantiate nodes via runtime.NewNode with statuses
core.NodeSucceeded and core.NodeNotStarted (or another non-running/non-waiting
status), call node.Cancel() for each, and assert using require.Equal that
node.State().Status remains unchanged; reference the existing runtime.NewNode,
node.Cancel, and node.State() helpers to locate where to add these checks so
future changes to the switch are caught.

Fix all unresolved CodeRabbit comments on this PR:

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