Consolidate binding-actor orchestration into shared channel engines

openspec/changes/consolidate-binding-actor-engines/.openspec.yaml

@@ -0,0 +1,2 @@
+schema: spec-driven
+created: 2026-08-18

openspec/changes/consolidate-binding-actor-engines/design.md

@@ -0,0 +1,79 @@
+# Design: consolidate-binding-actor-engines
+
+## Context
+
+The three channel binding actors each own a private copy of the same orchestration algorithms. A structural diff shows Discord and Mattermost are ~78% line-identical; Slack shares the same algorithms with different file organization. PR #2002 already extracted the small shared helpers (`PendingApprovalRequest<TPromptId>`, `PendingApprovalLookup`, `PendingApprovalRecovery`, `MessageChunker`). PR #2004 fixed one drift bug this duplication caused. This change extracts the four remaining large duplicated regions.
+
+Constraints from the constitution: actor boundaries stay transport-agnostic, security dependencies are required (non-nullable), no silent fallbacks, persisted types stay framework-owned and unchanged.
+
+## Goals / Non-Goals
+
+**Goals:**
+
+- One implementation each for gap hydration, approval-response flow, output-completion bookkeeping, and safe transport calls.
+- Zero behavior change, proven by the existing cross-channel contract suite plus new parity tests.
+- Per-channel hooks exist only for genuine transport differences.
+
+**Non-Goals:**
+
+- No shared binding-actor base class. The actors keep their own FSM, persistence handlers, and receive wiring.
+- No change to persisted events (`CursorAdvanced`, `PendingApprovalPromptTracked/Cleared`).
+- No new channel features (Mattermost processing indicator stays a separate product decision).
+- No change to the generic approval API (issue #1944).
+
+## Decisions
+
+### D1: Engines are plain classes, not actors and not a base class
+
+Each engine is a non-actor class in `Netclaw.Channels`, constructed by the binding actor with required dependencies (history fetcher, injection classifier, turn-enqueue callback, logger adapter). Actors call engine methods from inside their existing `CommandAsync` handlers.
+
+Rationale: a base actor class couples lifecycle, persistence, and supervision across transports and violates the transport-agnostic boundary rule. Plain classes keep the actors' Akka semantics untouched and make the algorithms unit-testable without TestKit. Alternative considered: template-method base actor — rejected for the coupling above and because Akka.NET receive registration in a base class hides message wiring from the concrete actor.
+
+### D2: Cursor comparison is an injected comparator; Discord uses length-then-ordinal
+
+The persisted `CursorAdvanced.Cursor` is already a `string` for every channel. Discord's in-memory `ulong` round-trip is the main textual difference blocking hydration consolidation. The engine stores cursors as strings and compares with an injected `IComparer<string>`:
+
+- Mattermost and Slack: ordinal comparison (current behavior).
+- Discord: length-then-ordinal comparison. Plain ordinal is WRONG for snowflakes across digit-length boundaries (`"999..."` 18 digits vs `"1000..."` 19 digits), so Discord SHALL NOT use plain ordinal. Length-then-ordinal equals numeric order for all non-negative integer strings without leading zeros.
+
+A unit test SHALL prove the Discord comparator matches `ulong` comparison, and MUST include cross-digit-length pairs and boundary values (`ulong.MaxValue`, adjacent powers of ten). Alternative considered: keep `ulong` inside Discord and make the engine generic over the cursor type — rejected because it preserves the asymmetry the consolidation exists to remove.
+
+### D3: Approval flow is a shared class with two hooks
+
+`ApprovalResponseFlow` owns text-approval parsing, cold-spawn forwarding, and prompt resolution (via `PendingApprovalLookup` from PR #2002). Hooks:
+
+- `RenderResolvedPromptAsync(...)` — required; each channel redraws its own prompt message.
+- `RespondSynchronously(replyTo, ack)` — optional hook used only by Mattermost, whose interactive-message webhook requires a synchronous HTTP reply. Discord (gateway events) and Slack do not register it.
+
+The requester identity check stays inside the shared flow so it cannot drift per channel.
+
+### D4: Output handling is a shared engine with a channel-output hook
+
+The shared engine owns the `TurnCompleted` bookkeeping (cursor advance, turn-in-flight flag, reminder observer settlement, empty-turn fallback, prompt clearing) and delegates unrecognized or channel-specific outputs (`SessionTitleOutput`, `ProcessingStateOutput`) to a `HandleChannelSpecificOutput` hook. A channel that does not support an output type ignores it in its hook; this is a capability difference, not a silent fallback.
+
+Persistence stays in the actor: the engine returns the events to persist (e.g., cleared prompts); the actor calls `Persist`/`PersistAll` and applies via the PR #2002 recovery helpers. The engine never touches Akka persistence.
+
+### D5: Failure modes keep PR #2004 semantics
+
+Engines do not catch-and-swallow. An exception from an engine method escapes the actor's `CommandAsync` handler, supervision restarts the actor, and recovery re-creates the pipeline. The `Feedback_send_failure_faults_the_actor` contract test pins this. The safe transport-call skeleton records telemetry and calls the delivery-failure notifier exactly as each channel does today.
+
+### D6: Extraction lands in four reviewable steps
+
+Order: (1) Discord cursor stringization + comparator test, (2) gap-hydration engine, (3) approval-response flow, (4) output template + safe-call skeleton. Each step compiles, passes the full `Netclaw.Actors.Tests` suite, and is a separate commit. If a step uncovers a real behavioral difference between channels, the step stops and the difference is surfaced for a decision instead of being silently normalized.
+
+## Risks / Trade-offs
+
+- [Discord cursor ordering breaks on short synthetic IDs in tests] → the comparator test covers cross-digit-length pairs; test fixtures with short numeric IDs order correctly under length-then-ordinal.
+- [Hydration consolidation changes turn-enqueue timing] → the engine is a mechanical transplant; the contract suite's hydration tests (fetch-once, stash-during-hydration, restart-re-runs) run per channel and must stay green at every step.
+- [Approval flow consolidation weakens a per-channel security check] → the requester check already has one shared implementation (`PendingApprovalLookup`, PR #2002); this change only moves its callers. New parity tests assert wrong-requester rejection per channel.
+- [Hidden per-channel differences get normalized away] → D6 stop rule: any discovered difference halts that step and is reported, mirroring how the PR #2004 drift was handled.
+- [Larger blast radius for a single engine bug] → trade-off accepted: one visible bug beats three drifting copies; the cross-channel contract suite runs every scenario against all three channels.
+
+## Migration Plan
+
+No deployment migration: no config, persistence, or wire change. Rollback is a revert of the stacked PR. The four commits in D6 allow partial revert per engine.
+
+## Open Questions
+
+- Mattermost lacks the processing-indicator output handling Slack and Discord have. Feature gap or intentional? Needs a product decision; out of scope here (hook default: ignore).
+- Slack's pending-approval lookup shape differs slightly (`FindIndex`, no call-id-first branch). If step 3 confirms a real semantic difference, Slack keeps its lookup and only shares the outer flow; the difference gets documented in the parity spec.

openspec/changes/consolidate-binding-actor-engines/proposal.md

@@ -0,0 +1,36 @@
+# Proposal: consolidate-binding-actor-engines
+
+## Why
+
+`SlackThreadBindingActor`, `DiscordSessionBindingActor`, and `MattermostSessionBindingActor` copy the same orchestration logic. Discord and Mattermost are ~78% line-identical. This duplication already produced one confirmed drift bug: Discord and Mattermost swallowed feedback-pipe failures that Slack propagated (fixed in PR #2004). Every future fix to hydration, approval flow, or delivery handling must land three times by hand. Each hand copy is a chance to miss a security-relevant path.
+
+Source PRDs: PRD-001 (Netclaw MVP, channel bindings), PRD-002 (gateway security envelope, approval checks), PRD-009 (input adapters and unified input).
+
+## What Changes
+
+- Extract a shared **gap-hydration engine** into `Netclaw.Channels`. It owns the fetch → cursor-filter → injection-classify → adopted-context-merge → turn-enqueue algorithm. The three actors delegate to it. (~600-700 duplicated lines removed.)
+- Extract a shared **approval-response flow**. It owns text approval parsing, cold-spawn approval forwarding, and prompt resolution. Mattermost keeps a synchronous HTTP-reply hook; Discord and Slack do not use it. (~450-550 lines removed.)
+- Extract a shared **output-handling template** for `TurnCompleted`/approval-prompt/reminder-observer bookkeeping, with a channel-specific hook for outputs only some channels support (Discord thread rename, processing indicators). (~70-90 lines removed.)
+- Extract a shared **safe transport-call skeleton** (timing → call → telemetry → failure notify). (~60-80 lines removed.)
+- **Prerequisite**: Discord's internal cursor changes from `ulong` snowflake to `string`, which the persisted `CursorAdvanced` event already stores for every channel. A unit test SHALL prove ordinal string comparison orders real Discord snowflake ranges the same as numeric comparison before the numeric path is removed.
+- Zero behavior change. No persisted-type change. Builds on the `PendingApproval*` helpers from PR #2002.
+
+In scope: the four engine extractions above, the Discord cursor change, and parity tests.
+Out of scope: a shared binding-actor base class that owns the actor FSM; Mattermost processing-indicator support (flagged as a possible feature gap, needs a product decision); the generic approval API rework (issue #1944); SignalR channel extraction (issue #691).
+
+## Capabilities
+
+### New Capabilities
+
+- `channel-binding-parity`: cross-channel guarantee that gap hydration, approval response handling, output-completion bookkeeping, and transport-failure escalation run through single shared implementations, with per-channel hooks limited to genuine transport differences.
+
+### Modified Capabilities
+
+<!-- none: thread-history-backfill and tool-approval-gates requirements are unchanged; this change consolidates their implementations without behavior change -->
+
+## Impact
+
+- Code: `src/Netclaw.Channels` (new engine types), `src/Netclaw.Channels.Slack`, `src/Netclaw.Channels.Discord`, `src/Netclaw.Channels.Mattermost` (delegation), `src/Netclaw.Actors.Tests` (parity contract tests).
+- Security impact: the approval requester check and the prompt-injection gap classification move from three copies to one. A fix in one place reaches all channels. No ACL or policy semantics change. The engines take required (non-nullable) security dependencies per the constitution.
+- Operational impact: none at runtime. Log messages keep their per-channel adapter fields. No config change, no migration, no persisted-format change.
+- Rollout: lands as PR 4 of the refactor stack, stacked on #2004. Revert is a single PR revert; no data migration to unwind.

openspec/changes/consolidate-binding-actor-engines/specs/channel-binding-parity/spec.md

@@ -0,0 +1,100 @@
+# channel-binding-parity Specification (delta)
+
+## ADDED Requirements
+
+### Requirement: Shared gap-hydration engine
+
+The system SHALL implement thread gap hydration (fetch history, filter by cursor, classify for prompt-injection risk, merge adopted context, enqueue the turn) in a single engine in the channel abstraction layer. Channel binding actors SHALL delegate hydration to this engine. The engine SHALL take its security-relevant dependencies (injection classifier, sender authorization callback) as required constructor inputs.
+
+#### Scenario: All channels hydrate through one implementation
+
+- **GIVEN** the Slack, Discord, and Mattermost binding actors
+- **WHEN** each performs one-shot hydration at actor start
+- **THEN** each delegates to the shared engine
+- **AND** the per-channel code supplies only transport lookups and the cursor comparator
+
+#### Scenario: Hydration contract behavior is unchanged
+
+- **GIVEN** the existing cross-channel contract suite
+- **WHEN** the hydration tests run (fetch at most once per lifetime, stash during hydration, re-run after supervised restart, adopted-context backfill)
+- **THEN** every test passes for every channel without per-channel test changes
+
+### Requirement: Cursor ordering by injected comparator
+
+The shared engine SHALL store cursors as strings, which matches the persisted `CursorAdvanced` format for every channel. Cursor comparison SHALL use a comparator the channel supplies. The Discord comparator SHALL order numeric snowflake strings identically to unsigned 64-bit numeric comparison. Discord SHALL NOT use plain ordinal string comparison.
+
+#### Scenario: Discord snowflake ordering across digit lengths
+
+- **GIVEN** two snowflake strings with different digit counts, such as an 18-digit and a 19-digit value
+- **WHEN** the Discord comparator orders them
+- **THEN** the result equals the numeric `ulong` ordering
+- **AND** a unit test proves the equivalence for cross-digit-length pairs and for boundary values
+
+### Requirement: Shared approval-response flow
+
+The system SHALL implement text-approval parsing, cold-spawn approval forwarding, and pending-prompt resolution in a single shared flow. The requester identity check SHALL execute inside the shared flow. Per-channel hooks SHALL be limited to prompt rendering, the pending-approval match order, and, for Mattermost only, the synchronous webhook reply.
+
+#### Scenario: Wrong requester is rejected on every channel
+
+- **GIVEN** a pending approval requested by user A
+- **WHEN** user B attempts to approve it on any channel
+- **THEN** the shared flow rejects the response
+- **AND** the channel posts its wrong-requester warning
+
+#### Scenario: Mattermost synchronous reply hook
+
+- **GIVEN** a Mattermost interactive-message approval
+- **WHEN** the shared flow resolves it
+- **THEN** the Mattermost hook sends the synchronous HTTP reply
+- **AND** Discord and Slack register no such hook
+
+#### Scenario: Channel match order picks the same candidate as before
+
+- **GIVEN** two pending approvals that the same sender may approve
+- **WHEN** that sender sends a text approval reply
+- **THEN** Slack resolves the earliest pending approval
+- **AND** Discord and Mattermost resolve the most recent pending approval
+
+> Note: this match order is the one real difference the step-3 stop rule found
+> between the three copies. Slack selected its candidate with `FindIndex`
+> (earliest match); Discord and Mattermost selected it with `LastOrDefault`
+> (most recent match). The shared lookup keeps one requester check and takes the
+> order as a required `ApprovalMatchOrder` input, so each channel keeps the
+> selection it had. Which order is correct is a separate product question,
+> tracked outside this change.
+
+### Requirement: Shared output-completion bookkeeping
+
+The system SHALL implement turn-completion bookkeeping (cursor advance, turn-in-flight state, reminder delivery settlement, empty-turn fallback, pending-prompt clearing) in a single engine. Persistence calls SHALL remain in the actor: the engine SHALL return the events to persist and SHALL NOT invoke Akka persistence. A channel-specific output hook SHALL handle output types that only some channels support.
+
+#### Scenario: Channel-specific outputs go through the hook
+
+- **GIVEN** a `SessionTitleOutput` for a Discord session
+- **WHEN** the shared engine processes outputs
+- **THEN** the Discord hook renames the thread
+- **AND** a channel without that capability ignores the output in its hook
+
+#### Scenario: Pipeline reinitialize keeps each channel's cursor discipline
+
+- **GIVEN** a pipeline reinitialize while a turn is in flight
+- **WHEN** the binding actor resets the engine
+- **THEN** Slack discards the pending cursor
+- **AND** Discord and Mattermost keep it, which preserves their current behavior
+
+> Note: the step-4 transplant surfaced this divergence. Slack clears the
+> pending cursor on reinitialize; Discord and Mattermost keep it, so a later
+> `TurnCompleted` can commit the cursor of a turn that the reinitialize
+> abandoned. That is a possible latent defect in the Discord and Mattermost
+> behavior, tracked as a product question outside this change. This change
+> preserves each channel's current behavior via `DiscardPendingCursor()`.
+
+### Requirement: Transport-failure escalation parity
+
+The safe transport-call skeleton SHALL record telemetry, notify delivery failure, and preserve the fail-loud contract: when the session feedback pipe fails, the error SHALL propagate so supervision restarts the actor and re-creates the pipeline. No channel SHALL swallow a feedback-pipe failure.
+
+#### Scenario: Feedback-pipe failure faults every channel actor
+
+- **GIVEN** a transport post failure whose delivery-failure feedback also fails
+- **WHEN** the binding actor handles it on any channel
+- **THEN** the actor restarts under supervision
+- **AND** the pipeline is re-created, observable as a second pipeline creation