Consolidate binding-actor orchestration into shared channel engines by Aaronontheweb · Pull Request #2005 · netclaw-dev/netclaw

Aaronontheweb · 2026-08-19T00:43:28Z

What

Phase 4 of the code-reduction stack (on top of #2004), implementing OpenSpec change consolidate-binding-actor-engines (proposal, design, parity spec, and tasks are in the PR). The three channel binding actors — previously ~78% line-identical between Discord and Mattermost — now delegate their four big duplicated regions to shared engines in Netclaw.Channels:

Commit	Engine	Actor lines removed
Discord cursor stringization	`SnowflakeCursorComparer` (length-then-ordinal; proof test vs `ulong` ordering incl. cross-digit-length pairs)	prerequisite
Gap hydration	`ThreadGapHydrationEngine` — fetch → cursor-filter → injection-classify → adopted-context merge → enqueue	−859
Approval response	`ApprovalResponseFlow` — text parsing, cold-spawn path, prompt resolution; requester check has one home	−737
Output + transport	`ChannelOutputEngine` (turn-completion state machine) + `SafeTransportCall`	−401

Actor sizes: Slack 1,919→1,263, Discord 1,733→1,057, Mattermost 1,684→989. This PR net across src: −346 (engines carry ~150 lines of hook XML docs); across the whole stack the src net is −1,004 with functionality unchanged and coverage strengthened.

Zero behavior change, with three disclosed exceptions

Slack hydration log lines gain session=/allowed= fields the other channels already log (no test asserts these).
Slack's approval text path previously swallowed journal-persist/redraw failures inside a wide try; it now wraps only the feedback send, matching the other channels and the no-silent-fallbacks rule.
Prompt-post failure now removes the pending entry before the auto-deny on all three channels (previously order varied; same feedback and list state, removes a Mattermost double-deny risk).

Real differences found and preserved (the stop rule working)

Approval match order: Slack resolves the earliest matching pending approval, Discord/Mattermost the most recent. Preserved per channel via a required ApprovalMatchOrder on the shared lookup; documented in the parity spec. Which is correct is an open product question.
Reinitialize cursor discipline: Slack discards the pending cursor on pipeline reinitialize, Discord/Mattermost keep it — meaning a later TurnCompleted can commit an abandoned turn's cursor. Possible latent defect in the Discord/Mattermost behavior; preserved as-is and documented in the parity spec.
Slack keeps its own transport wrappers (three-way exception classification + RecordReplyRejected don't fit the shared skeleton), delivery-failure report timing (deferred vs inline), reminder observed-at source, text trimming, and error formatting — all per-channel hooks.

Verification

Full Netclaw.Actors.Tests: 3,447 passed / 0 failed / 1 pre-existing Windows-only skip — run after every channel's delegation in every group, plus a 5× stress run at the end. One intermediate run showed 3 unreproduced failures (names not captured; 5 consecutive clean runs after); flagged here for CI scrutiny given the repo's known racy-test history.
Netclaw.Daemon.Tests: 1,023 passed / 0 failed. FileFlow suites green.
Persisted formats untouched: CursorAdvanced values byte-identical (same ulong.ToString() output), journal event types unchanged, no config/schema change.
dotnet slopwatch analyze: 0 issues; header verification passes; zero build warnings.

Stack

PR 4 of 4 in the current stack. Base: refactor/delivery-failure-drift (#2004).

-                    if (_pendingCursorTs is not { } pending || ts.CompareTo(pending) > 0)
-                        _pendingCursorTs = ts;
-                }
+                _outputEngine.AdvancePendingCursorForEnqueuedTurn(currentTs?.Value);


+            isAuthorizedSender: senderId =>
+                SlackAclPolicy.IsAllowedUser(new SlackUserId(senderId), _dependencies.Options),
+            log: _log,
+            readCursor: () => _cursorTs?.Value,


+        Assert.Equal(0, SnowflakeCursorComparer.Instance.Compare(
+            "1234567890123456789", "1234567890123456789"));


+    {
+        Assert.Equal(0, SnowflakeCursorComparer.Instance.Compare(
+            "1234567890123456789", "1234567890123456789"));
+        Assert.Equal(0, SnowflakeCursorComparer.Instance.Compare(null, null));


+        catch (Exception ex)
+        {
+            _log.Error(ex, "Failed to route {Channel} text approval response for call {CallId}", _channelName, pending.CallId);
+            return true;
+        }


+        catch (Exception ex)
+        {
+            _log.Error(ex, "Failed to route {Channel} approval response for call {CallId}", _channelName, callId);
+            respondSynchronously?.Invoke(CommandNack.For(_sessionId, ApprovalNackReasons.PersistFailed));
+            return;
+        }


+        catch (Exception ex)
+        {
+            _log.Error(
+                ex,
+                "Failed to route cold {Channel} text approval response from sender {SenderId}",
+                _channelName,
+                senderId);
+            return false;
+        }


+        catch (Exception ex)
+        {
+            var duration = ElapsedMs(startedAt);
+            logFailure(ex);
+            ChannelTelemetry.For(_channelType).RecordReplyFailed(duration);
+            await _notifyDeliveryFailedAsync(DeliveryFailureKind.TransportFailure, ex.Message);
+            return false;
+        }


+        catch (Exception ex)
+        {
+            _log.Warning(ex, "Thread history fetch failed for session {SessionId}", _sessionId.Value);
+            return;
+        }


+        catch (Exception ex)
+        {
+            // Non-fatal: execute the turn without an adopted window and keep
+            // hydration re-armed so a later authorized inbound retries.
+            _log.Warning(ex, "Re-armed thread history fetch failed for session {SessionId}", _sessionId.Value);
+            return baseInput;
+        }


Plan the extraction of the four duplicated orchestration regions from the Slack, Discord, and Mattermost binding actors into shared engines. The design records the key decision set: plain engine classes over a base actor class, an injected cursor comparator with a length-then- ordinal Discord comparator, hook-limited channel differences, and a stop rule for any real semantic difference found during transplant.

The persisted CursorAdvanced event stores a string cursor for every channel. Discord alone converted that string to ulong for in-memory comparisons, which kept its hydration code textually different from Mattermost's and blocked a shared gap-hydration engine. Hold the cursor as a canonical string and compare with SnowflakeCursorComparer (length first, then ordinal). Plain ordinal is wrong across a digit-length boundary; a unit test proves the comparator matches ulong ordering, with an explicit case that shows the plain ordinal failure. The ulong parse remains as a normalization step, so corrupt IDs are rejected exactly as before and persisted values are byte-identical.

Move the fetch, cursor-filter, injection-classify, adopted-context merge, and turn-enqueue algorithm from the three binding actors into one ThreadGapHydrationEngine in Netclaw.Channels. All constructor dependencies are required. The engine holds no actor state: cursor reads, queue access, warnings, and enqueue bookkeeping are callbacks the actor supplies. Channel differences that remain are genuine and stay per channel: - cursor ordering (Slack decimal event ts, Discord snowflake comparator, Mattermost ordinal) via an injected comparer - authorization basis (Slack ACL policy vs allowed-user options) via a required callback - fetcher availability: Discord and Mattermost construct the engine only when the gateway supplies a history fetcher Slack log lines gain the session and allowed-count fields the other channels already log. No behavior change otherwise; the PR #733 cursor-advance invariants move into the engine verbatim. Net: -859 lines across the three actors, +442 shared.

Move text-approval parsing, the cold-spawn approval path, and prompt resolution from the three binding actors into ApprovalResponseFlow in Netclaw.Channels. All dependencies are required. Persistence stays in the actor via a persist callback. The requester identity check keeps one home in PendingApprovalLookup. The transplant surfaced one real semantic difference: Slack resolved the earliest matching pending approval, Discord and Mattermost the most recent. The shared lookup takes a required ApprovalMatchOrder, so each channel keeps its exact selection. The parity spec documents the difference; which order is correct is a separate product question. Mattermost keeps its synchronous webhook reply through an optional per-call hook; Discord and Slack pass none. One deliberate Slack alignment: its old try block also swallowed journal persist and redraw failures on the text path. The shared flow wraps only the feedback send, so a persist failure now faults the Slack actor exactly as it does the other two. Net: -737 lines across the three actors, +437 shared.

ChannelOutputEngine owns the per-turn delivery state machine: pending cursor, turn-in-flight, reminder observer settlement, empty-turn fallback suppression, and prompt clearing. The engine returns the PendingApprovalPromptCleared events; the actor persists them. A required channel hook handles outputs only some channels support. SafeTransportCall owns the timing, telemetry, failure-notify skeleton for Discord and Mattermost posts and uploads. Slack keeps its own transport wrappers: its three-way exception classification and RecordReplyRejected category do not fit the shared skeleton, per the stop rule. Genuine differences stay per-channel behind hooks: delivery-failure report timing (Slack defers to turn completion), reinitialize cursor discard (Slack only), reminder observed-at source, text trimming, and error formatting. One behavior-preserving unification: prompt-post failure now removes the pending entry before the auto-deny on all three channels, which also removes a Mattermost double-deny risk. Net: -401 lines across the actors, +489 shared.

* Sync delta specs for completed OpenSpec changes Apply the delta specs of 16 completed changes to the main specs. Create four new capability specs: daemon-shell-path, shell-policy-evaluator-architecture, skillserver-native-sidecar-sync, and named-model-definitions. Correct three reminder requirements against the merged code: - One-shot success removes the definition and its history. Only a poisoned one-shot is soft-deleted (PR #1821). - No execution capacity cap exists (PR #1839). The nack and ack-skip policy now covers a duplicate active occurrence and a short acknowledgement lease. - Every delivery kind now holds its envelope. ReminderDeliveryResult replaces ReminderDeliveryObserved. * Archive 16 completed OpenSpec changes Move each completed change to openspec/changes/archive/2026-08-19-<name>/. Their code is merged on dev. Also tick task 5.3 of consolidate-binding-actor-engines. PR #2005 merged that work. Leave eval-run checkboxes unticked. An evals-only gap does not block the archive.

Aaronontheweb changed the title ~~refactor/binding engine~~ Consolidate binding-actor orchestration into shared channel engines Aug 19, 2026

github-code-quality Bot found potential problems Aug 19, 2026

View reviewed changes

Aaronontheweb mentioned this pull request Aug 19, 2026

Simplify three duplication hotspots #2006

Merged

Aaronontheweb force-pushed the refactor/binding-engine branch from cd51d35 to 4ba9688 Compare August 19, 2026 02:08

Aaronontheweb mentioned this pull request Aug 19, 2026

Resolve text approvals against the oldest pending approval #2008

Merged

Aaronontheweb marked this pull request as ready for review August 19, 2026 02:09

Aaronontheweb added cleanup Code quality improvements and tech debt reduction channels Discord, Slack, and other channels. labels Aug 19, 2026

Aaronontheweb force-pushed the refactor/binding-engine branch from 4ba9688 to 7108661 Compare August 19, 2026 02:51

Aaronontheweb force-pushed the refactor/binding-engine branch from 7108661 to 4ba9688 Compare August 19, 2026 03:39

Aaronontheweb mentioned this pull request Aug 19, 2026

Discord and Mattermost keep the pending cursor across pipeline reinitialize #2013

Closed

Aaronontheweb added 7 commits August 19, 2026 12:26

Record the reinitialize cursor divergence in the parity spec

c4dd3f9

Sync the channel-binding-parity spec to main specs

8c34a93

Aaronontheweb force-pushed the refactor/binding-engine branch from 4ba9688 to 8c34a93 Compare August 19, 2026 12:26

Aaronontheweb merged commit ecb9b3b into dev Aug 19, 2026
23 checks passed

Aaronontheweb deleted the refactor/binding-engine branch August 19, 2026 16:41

Aaronontheweb added refactoring security Security-related changes labels Aug 19, 2026

Aaronontheweb mentioned this pull request Aug 20, 2026

Archive completed OpenSpec changes and sync their specs #2024

Merged

Navigation Menu

Uh oh!

Consolidate binding-actor orchestration into shared channel engines - #2005

Consolidate binding-actor orchestration into shared channel engines#2005
Aaronontheweb merged 7 commits into
refactor/delivery-failure-driftfrom
refactor/binding-engine

Aaronontheweb commented Aug 19, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

		Assert.Equal(0, SnowflakeCursorComparer.Instance.Compare(
		"1234567890123456789", "1234567890123456789"));

Uh oh!

Conversation

Aaronontheweb commented Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Zero behavior change, with three disclosed exceptions

Real differences found and preserved (the stop rule working)

Verification

Stack

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Aaronontheweb commented Aug 19, 2026 •

edited

Loading