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

380-399: ⚡ Quick win

Stale doc comment — now describes only half of the function's behavior.

The comment on line 380 says "sets a working directory when the manifest omits one" but the function also marks explicitly-configured directories. Consider updating it.

📝 Suggested doc update

-// applyWorkingDirFallback sets a working directory when the manifest omits one.
+// applyWorkingDirFallback marks any already-configured working directory as
+// explicit and, when none is configured, synthesizes a fallback from filePath
+// or the process working directory.
 func applyWorkingDirFallback(dag *core.DAG, filePath string) error {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/core/spec/loader.go` around lines 380 - 399, Update the stale doc
comment for applyWorkingDirFallback to reflect both behaviors: it marks
explicitly-configured working directories (via
markConfiguredWorkingDirsExplicit) and then sets a default working directory
when none is provided on the DAG (using filePath or getDefaultWorkingDir()).
Mention both steps and the error-return case from getDefaultWorkingDir so the
comment accurately documents applyWorkingDirFallback's full behavior.

internal/runtime/plan_test.go (1)

508-531: 💤 Low value

LGTM — the test correctly captures the core regression scenario: a persisted NodeState.WorkingDir must override the current DAG step's Dir when building the retry plan.

One minor coverage gap: the test only covers the case where node.Step.Dir is empty. Consider adding a sub-case where node.Step.Dir is also non-empty to document the explicit priority order.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/runtime/plan_test.go` around lines 508 - 531, Add a sub-case to
TestCreateRetryPlan_PrefersPersistedNodeWorkingDir that sets both the DAG
Step.Dir and the persisted node.Step.Dir to non-empty values and asserts that
runtime.CreateRetryPlan still prefers the persisted NodeState.WorkingDir (e.g.,
create node via runtime.NodeWithData with NodeData containing
Step:{Name:"target", Dir:"/node-specified"} and
NodeState.WorkingDir:"/original-work", call runtime.CreateRetryPlan, then assert
the returned plan's target.Step().Dir equals "/original-work"); this documents
the explicit priority order and uses the same helpers (NodeWithData, NodeData,
NodeState, core.Step) referenced in the existing test.

internal/service/slack/bot.go (1)

111-121: 💤 Low value

Consider initialising incomingAfterFunc in New to avoid a per-call closure allocation.

In the production path every enqueueIncomingMessage call allocates a fresh func(time.Duration, func()) wrapper just to call time.AfterFunc. Setting the default once in New removes the nil-guard and the allocation entirely, while test code can still override the field directly.

♻️ Proposed refactor

 return &Bot{
     cfg:                   cfg,
     agentAPI:              agentAPI,
     slackClient:           slackClient,
     socketClient:          socketClient,
     allowedChannels:       allowed,
     eventService:          cfg.EventService,
     notificationStateFile: cfg.NotificationStateFile,
     logger:                logger,
     incomingDelay:         defaultIncomingBatchDelay,
+    incomingAfterFunc:     time.AfterFunc,
 }, nil

-    afterFunc := b.incomingAfterFunc
-    if afterFunc == nil {
-        afterFunc = func(delay time.Duration, fn func()) {
-            time.AfterFunc(delay, fn)
-        }
-    }
-    afterFunc(delay, func() {
+    b.incomingAfterFunc(delay, func() {
         b.flushIncomingMessages(ctx, cs, convKey, gen)
     })

Also applies to: 330-338

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/service/slack/bot.go` around lines 111 - 121, The Bot constructor
(New) should initialize the incomingAfterFunc field to time.AfterFunc (and
likewise initialize any similar timers mentioned around enqueueOutgoingMessage)
instead of leaving it nil so enqueueIncomingMessage doesn't allocate a new
closure on every call; update New to set b.incomingAfterFunc = time.AfterFunc,
remove the per-call nil-guard/closure allocation in enqueueIncomingMessage, and
allow tests to override the incomingAfterFunc field directly (same change
applies to the analogous outgoing timer field referenced in the review).

internal/service/slack/bot.go (1)

111-122: 💤 Low value

Consider initialising incomingAfterFunc to its default in New() to avoid a per-call closure allocation.

The nil-check at line 331 creates a fresh wrapper closure on every production enqueueIncomingMessage call. Seeding the field in New() avoids the repeated allocation and removes the branch from the hot path entirely.

♻️ Proposed refactor

 return &Bot{
     cfg:                   cfg,
     agentAPI:              agentAPI,
     slackClient:           slackClient,
     socketClient:          socketClient,
     allowedChannels:       allowed,
     eventService:          cfg.EventService,
     notificationStateFile: cfg.NotificationStateFile,
     logger:                logger,
     incomingDelay:         defaultIncomingBatchDelay,
+    incomingAfterFunc: func(d time.Duration, fn func()) {
+        time.AfterFunc(d, fn)
+    },
 }, nil

Then simplify the dispatch site:

-afterFunc := b.incomingAfterFunc
-if afterFunc == nil {
-    afterFunc = func(delay time.Duration, fn func()) {
-        time.AfterFunc(delay, fn)
-    }
-}
-afterFunc(delay, func() {
+b.incomingAfterFunc(delay, func() {
     b.flushIncomingMessages(ctx, cs, convKey, gen)
 })

Tests override bot.incomingAfterFunc after construction as before; New() always produces a non-nil field, so the nil guard is no longer needed.

Also applies to: 330-338

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/service/slack/bot.go` around lines 111 - 122, The Bot constructor
(New) should initialize the incomingAfterFunc field to the default
implementation to avoid allocating a new closure on every enqueueIncomingMessage
call; update New() to set Bot.incomingAfterFunc to the same default timer/after
behavior used today so enqueueIncomingMessage no longer needs its nil-check and
per-call wrapper allocation, and keep tests able to override
bot.incomingAfterFunc as before; ensure you update references to
incomingAfterFunc, New, and enqueueIncomingMessage accordingly.