feat(execbroker): propagate middleware errors

[MeteorsLiu]

No description provided.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[fennoai]

Review summary

The middleware signature migration to func(Request) (Request, error) is clean and applied consistently across Command, CommandContext, and Run. All call sites are updated and the module compiles. TestMiddlewareErrorStopsCommand is a strong test — it covers all three entry points and asserts both error propagation (errors.Is) and that no output is produced, directly verifying non-execution. Lock release on the middleware error path is correct, and there is no path where a rejected command can still execute (cmd.Err is honored by Start(), and Run returns before mutating/launching the caller's cmd).

A few non-blocking items are noted inline. Additionally, the doc comments on Command/CommandContext (execbroker.go:80-81, :91) and Run (:102-103) still describe only the pass-through behavior and don't mention that a middleware rejection is now surfaced via cmd.Err (fails at Start()) or returned directly by Run before execution — worth a sentence each so the non-obvious error path is discoverable.

[fennoai]

On the error path, Command/CommandContext still build the cmd from the middleware-mutated req.Name (running a LookPath) and then run apply(), copying the mutated env/dir/streams onto a command that was rejected. This is functionally safe because cmd.Err short-circuits Start()/Run(), but it's asymmetric with Run (which returns early at line 121 without mutating the caller's cmd) and exposes rejected-command mutations to any caller that inspects cmd before running it. Consider short-circuiting on error to keep the three paths symmetric, e.g.

if err != nil {
    cmd := exec.Command(name, args...)
    cmd.Err = err
    return cmd
}

[fennoai]

The user-supplied middleware now runs while scopeMu.RLock() is held (acquired at line 135, released at 159/164). Since this change lets middleware run arbitrary logic (and return errors), the re-entrancy risk is worth flagging: if a middleware calls back into Do (which takes scopeMu.Lock()) on the same goroutine it will deadlock, and holding the shared read lock for the full duration of arbitrary user code lengthens lock-hold time on the hot Command/CommandContext/Run path. Consider snapshotting the needed Scope fields (including the Middleware value) under the lock, RUnlock, then invoking middleware outside the critical section.

[luoliwoshang]

LGTM