Skip to content

Implement executor mesh support with Inbox, OnEvent, and ShouldStop#4

Merged
intel352 merged 7 commits intomasterfrom
feat/executor-mesh-support
Apr 5, 2026
Merged

Implement executor mesh support with Inbox, OnEvent, and ShouldStop#4
intel352 merged 7 commits intomasterfrom
feat/executor-mesh-support

Conversation

@intel352
Copy link
Copy Markdown
Contributor

@intel352 intel352 commented Apr 5, 2026

  • Understand the existing code and review comments
  • Fix emit to recover from panics in external OnEvent handlers (with logging)
  • Add Thinking: finalThinking to ShouldStop early-return
  • Emit terminal events (EventFailed) on all early-exit paths (approval_timeout, request_expired, loop_detected)
  • Update docs/plans/2026-04-04-executor-mesh-support-design.md to fix inbox ordering description
  • Update docs/plans/2026-04-04-executor-mesh-support-plan.md to fix inbox ordering description
  • Add tests: panic recovery in OnEvent, ShouldStop Thinking population
  • Fix TestAssetNameForPlatform_AllPlatforms: update expected suffixes to .tar.gz for non-Windows platforms
  • Update copyArgs comment to reflect shallow-copy limitation
  • Bound drainInbox to drainInboxMaxMessages (100) per iteration; add ctx.Err() pre-check and ctx.Done() case to prevent starvation
  • Normalize inbox messages to RoleUser (clearing ToolCallID and ToolCalls) to prevent prompt injection
  • Add TestExecute_InboxNormalizesRole test covering role normalization and all tool field clearing
  • All executor and provider tests pass

intel352 and others added 2 commits April 4, 2026 22:43
Extends executor.Config with Inbox (message injection), OnEvent
(real-time streaming), and ShouldStop (custom termination) to support
ratchet-cli's agent mesh. 6-step plan, all backward-compatible.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* Implement executor mesh support: Inbox, OnEvent, ShouldStop

Add three optional fields to executor.Config for mesh support:
- Inbox: channel for injecting external messages into the conversation
- OnEvent: callback for real-time event streaming
- ShouldStop: custom termination hook

All features are backward-compatible - nil values preserve existing behavior.
Includes comprehensive unit and integration tests.

Agent-Logs-Url: https://github.com/GoCodeAlone/workflow-plugin-agent/sessions/443ef504-27cc-40a0-88f3-b61e67d82a9f

Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>

* Address code review feedback: fix comment and optimize closed-channel handling

Agent-Logs-Url: https://github.com/GoCodeAlone/workflow-plugin-agent/sessions/443ef504-27cc-40a0-88f3-b61e67d82a9f

Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>

* Address review: move inbox drain before compaction, copy tool args, fix drainInbox

- Move drainInbox call before NeedsCompaction so injected messages are
  accounted for in compaction decisions
- Copy tc.Arguments via copyArgs() before placing on EventToolCallStart
  to prevent OnEvent handlers from mutating the map used by tool execution
- Fix drainInbox: remove dead `closed` variable (default branch now returns
  false directly), update doc comment to match actual behavior
- Add TestExecute_ToolArgsEventIsCopy to verify arg isolation

Agent-Logs-Url: https://github.com/GoCodeAlone/workflow-plugin-agent/sessions/79a383c2-cc77-4f96-8c82-963af9a41e07

Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
Copilot AI review requested due to automatic review settings April 5, 2026 03:25
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds “executor mesh” features to enable external message injection, real-time event streaming, and custom termination conditions for agents running under a multi-agent mesh.

Changes:

  • Extended executor.Config with Inbox, OnEvent, and ShouldStop, and integrated these into the main Execute() loop.
  • Introduced an executor.Event model (executor/events.go) and emitted events for iteration/thinking/text/tool execution/completion/failure.
  • Added unit + integration-style tests covering inbox draining, event sequencing, termination behavior, and transcript recording.

Reviewed changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
executor/executor.go Implements inbox draining, event emission, and ShouldStop termination inside the executor loop.
executor/events.go Defines event types and the Event payload used by OnEvent.
executor/executor_test.go Adds focused unit tests for inbox/events/ShouldStop/backward compatibility.
executor/mesh_support_test.go Adds end-to-end style mesh scenario tests (inbox + events + termination).
docs/plans/2026-04-04-executor-mesh-support-design.md Documents the design; currently has an ordering mismatch vs implementation for inbox draining vs compaction.
docs/plans/2026-04-04-executor-mesh-support-plan.md Implementation plan; also has the same ordering mismatch.
go.mod / go.sum Updates dependencies required by the broader mesh-related changes/tests.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread executor/executor.go Outdated
Comment thread executor/executor.go
return &Result{
Content: reason,
Status: "completed",
Iterations: iterCount,
Copy link

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ShouldStop early-return sets Status: "completed" but drops Result.Thinking, unlike the normal completion path (which returns finalThinking). Consider populating Thinking: finalThinking here as well so callers get consistent debugging information for completed runs.

Suggested change
Iterations: iterCount,
Iterations: iterCount,
Thinking: finalThinking,

Copilot uses AI. Check for mistakes.
Comment thread docs/plans/2026-04-04-executor-mesh-support-design.md Outdated
Comment thread docs/plans/2026-04-04-executor-mesh-support-plan.md Outdated
Comment thread executor/executor.go
Comment on lines +439 to +445
// Emit completed event
emit(cfg, Event{
Type: EventCompleted,
AgentID: agentID,
Iteration: iterCount,
Content: finalContent,
})
Copy link

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Terminal events are only emitted on the normal completion path (EventCompleted at the end) and on Provider.Chat errors (EventFailed). Other early-exit statuses returned from this function (e.g. approval timeout, human request expiry, loop detection) still return without emitting a terminal event, which can leave mesh/TUI consumers waiting for a final signal. Consider emitting a terminal event in every return path (or centralizing returns via a helper that always emits).

Copilot uses AI. Check for mistakes.
@intel352
Copy link
Copy Markdown
Contributor Author

intel352 commented Apr 5, 2026

@copilot apply changes based on the comments in this thread

Copilot AI and others added 2 commits April 5, 2026 03:34
…rminal events on all exits, fix docs ordering

Agent-Logs-Url: https://github.com/GoCodeAlone/workflow-plugin-agent/sessions/568b084c-b021-4953-b847-6856a7d13916

Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
Copilot AI review requested due to automatic review settings April 5, 2026 03:38
@intel352 intel352 review requested due to automatic review settings April 5, 2026 03:38
Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 5, 2026

@copilot apply changes based on the comments in this thread

All five review comments have been addressed in commits 2db7d2f and e23d510:

  1. emit panic safetyemit now wraps the OnEvent callback in a defer/recover block. Any panic from an external handler is caught and logged via log.Printf so observers can't crash the executor.

  2. ShouldStop missing Thinking — Added Thinking: finalThinking to the early-return Result so callers get consistent debugging info on all completed exits.

  3. Terminal events on all early-exit pathsEventFailed is now emitted before every non-normal return: approval_timeout, request_expired, and loop_detected.

  4. Doc ordering fix (design.md) — Updated the description to say inbox draining happens before the compaction check (matching the implementation).

  5. Plan ordering fix (plan.md) — Aligned step 3 text to match the actual ordering (drain inbox → compaction check → provider.Chat()).

New tests added: TestExecute_OnEventPanicDoesNotCrashExecutor and TestExecute_ShouldStopThinkingPopulated.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@intel352
Copy link
Copy Markdown
Contributor Author

intel352 commented Apr 5, 2026

@copilot Fix the test failures

--- FAIL: TestAssetNameForPlatform_AllPlatforms (0.00s)
    llama_cpp_download_test.go:50: assetNameForPlatform("darwin", "arm64"): want "-bin-macos-arm64.zip", got "-bin-macos-arm64.tar.gz"
    llama_cpp_download_test.go:50: assetNameForPlatform("darwin", "amd64"): want "-bin-macos-x64.zip", got "-bin-macos-x64.tar.gz"
    llama_cpp_download_test.go:50: assetNameForPlatform("linux", "amd64"): want "-bin-linux-amd64.zip", got "-bin-linux-amd64.tar.gz"
    llama_cpp_download_test.go:50: assetNameForPlatform("linux", "arm64"): want "-bin-linux-arm64.zip", got "-bin-linux-arm64.tar.gz"

…tching assetSuffixesForPlatform priority

Agent-Logs-Url: https://github.com/GoCodeAlone/workflow-plugin-agent/sessions/f77f67f5-0445-4eea-a346-636e4fdd9eef

Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 5, 2026

@copilot Fix the test failures

...

Fixed in commit a92a3e7. The test expectations in TestAssetNameForPlatform_AllPlatforms were out of sync with assetSuffixesForPlatform, which prioritizes .tar.gz over .zip for newer llama.cpp releases on non-Windows platforms. Updated the expected values for darwin and linux to .tar.gz.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread executor/executor.go Outdated
Comment on lines +274 to +283
// Emit tool call start event with a copy of the arguments to prevent
// any OnEvent handler from mutating the map used by the tool execution.
emit(cfg, Event{
Type: EventToolCallStart,
AgentID: agentID,
Iteration: iterCount,
ToolName: tc.Name,
ToolCallID: tc.ID,
ToolArgs: copyArgs(tc.Arguments),
})
Copy link

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment says the ToolArgs copy prevents OnEvent handlers from mutating the args used by tool execution, but copyArgs is only a shallow copy. If tc.Arguments contains nested maps/slices, an OnEvent handler can still mutate nested values and affect tool execution. Consider either updating the comment to reflect the shallow-copy limitation, or performing a deep copy for ToolArgs (with an eye on performance).

Copilot uses AI. Check for mistakes.
Comment thread executor/executor.go Outdated
Comment on lines +590 to +610
for {
select {
case msg, ok := <-cfg.Inbox:
if !ok {
// Channel closed — stop draining.
return messages, true
}
messages = append(messages, msg)
_ = cfg.Transcript.Record(ctx, TranscriptEntry{
ID: uuid.New().String(),
AgentID: agentID,
TaskID: cfg.TaskID,
ProjectID: cfg.ProjectID,
Iteration: iteration,
Role: msg.Role,
Content: msg.Content,
})
default:
// No more pending messages.
return messages, false
}
Copy link

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

drainInbox can starve the executor loop: if another goroutine is continuously sending to cfg.Inbox, the receive case will keep being ready and this for { select { ... } } loop may never hit default, preventing progress to compaction/LLM calls. Consider bounding the number of messages drained per iteration (e.g., max N) and/or adding a case <-ctx.Done(): to break out early.

Copilot uses AI. Check for mistakes.
Comment thread executor/executor.go
Comment on lines +592 to +606
case msg, ok := <-cfg.Inbox:
if !ok {
// Channel closed — stop draining.
return messages, true
}
messages = append(messages, msg)
_ = cfg.Transcript.Record(ctx, TranscriptEntry{
ID: uuid.New().String(),
AgentID: agentID,
TaskID: cfg.TaskID,
ProjectID: cfg.ProjectID,
Iteration: iteration,
Role: msg.Role,
Content: msg.Content,
})
Copy link

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inbox messages are appended to the conversation verbatim, including their Role, ToolCallID, and ToolCalls. Since the design/docs describe inbox injection as user-role mesh messages, allowing arbitrary roles here enables accidental or malicious injection of system/assistant/tool messages (prompt injection / transcript corruption). Consider normalizing inbox messages to RoleUser (and clearing tool-related fields) or validating/allowlisting permitted roles before appending/recording.

Copilot uses AI. Check for mistakes.
@intel352
Copy link
Copy Markdown
Contributor Author

intel352 commented Apr 5, 2026

@copilot apply changes based on the comments in this thread

Copilot AI and others added 2 commits April 5, 2026 04:04
…fix, inbox role normalization

Agent-Logs-Url: https://github.com/GoCodeAlone/workflow-plugin-agent/sessions/94d3766c-9e42-4886-8dd9-9cc92d2cc3e4

Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
…t to cover ToolCalls clearing

Agent-Logs-Url: https://github.com/GoCodeAlone/workflow-plugin-agent/sessions/94d3766c-9e42-4886-8dd9-9cc92d2cc3e4

Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
Copilot AI review requested due to automatic review settings April 5, 2026 04:08
@intel352 intel352 review requested due to automatic review settings April 5, 2026 04:08
Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 5, 2026

@copilot apply changes based on the comments in this thread

All three review points addressed in commit 891af7f:

  1. copyArgs comment — Updated to explicitly state this is a shallow copy and that nested values are still shared, with rationale for the trade-off.

  2. drainInbox starvation — Added a drainInboxMaxMessages constant (100) to bound the loop per iteration, added a ctx.Err() pre-check before the loop, and kept the case <-ctx.Done(): inside the select.

  3. Inbox role normalization — Inbox messages are now normalized to RoleUser with ToolCallID and ToolCalls cleared before being appended to the conversation. TestExecute_InboxNormalizesRole covers all four cases (system, assistant, tool, and assistant-with-tool-calls messages).

@intel352 intel352 merged commit 6b88ebb into master Apr 5, 2026
4 checks passed
@intel352 intel352 deleted the feat/executor-mesh-support branch April 5, 2026 04:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants