fix(slack): split Text/TextStreaming output filters, fix dropped and duplicated messages#407
Merged
Conversation
…nges Three OpenSpec changes to harden skills as a first-class platform capability: 1. compressed-skill-index: Replace verbose GenerateDescriptionMenu() with pipe-delimited compressed format, add LLM sidecar for trigger phrase generation, and filter skills by session audience and available tools. 2. skill-tools-and-slash-commands: Add skill_load, skill_read_resource, and skill_manage tools. Implement slash-command dispatch adopting Claude Code invocation model (name = command, disable-model-invocation, user-invocable). 3. trust-tiers-security-stub: Add SkillTrustTier enum with directory-based inference, ISkillContentScanner stub, restore skill-authoring system skill, and update existing system skill frontmatter with invocation control fields.
…ring skill Implement the trust-tiers-security-stub OpenSpec change: - Add SkillTrustTier enum (System/Operator/Community/External/Agent) - Infer trust tier from directory location in SkillScanner - Expand hidden-directory scanning to .community, .external, .agent - Exclude .quarantine from scanning - Add ISkillContentScanner interface with NoOpSkillContentScanner stub - Register scanner in DI via SecurityServiceExtensions - Restore skill-authoring system skill with complete frontmatter spec including invocation control fields and trust tier documentation - Add disable-model-invocation to netclaw-operations skill - Add 17 new tests covering trust tier inference and directory scanning
- Merge Operator into User tier (self-hosted model doesn't distinguish) - Add DefaultMinimumAudience() extension mapping tiers to TrustAudience - Default all tiers to Team minimum; Public requires explicit opt-in - Consolidate AllowedHiddenDirectories + InferTrustTier into single HiddenDirectoryTiers dictionary (single source of truth) - Update skill-authoring skill docs with new tier table
Replace verbose skill index (~2000 tokens) with pipe-delimited compressed format (~200-400 tokens). Research from dotnet-skills evals shows compressed indexes achieve 56.5% TPR vs 21.7% for verbose formats. - Rewrite SkillRegistry.GenerateDescriptionMenu() to pipe-delimited format grouped by category, referencing skill_load instead of file_read - Add per-audience pre-built menus (Public/Team/Personal) with trust-tier visibility filtering via DefaultMinimumAudience() - Add DisableModelInvocation/UserInvocable/ArgumentHint frontmatter fields to SkillEntry and SkillFrontmatter (Claude Code invocation model) - Create SkillIndexEnrichmentService (IHostedService) that generates trigger phrases via LLM sidecar, cached to disk by name+version - Fallback to truncated description when sidecar unavailable - Update SystemSkillSyncService to rebuild audience menus after re-scan - 910 tests passing, zero slopwatch violations
…spatch Implement the skill-tools-and-slash-commands OpenSpec change: - Add skill_load tool (Grant=builtin): loads skill by name, returns body with frontmatter stripped + resource manifest - Add skill_read_resource tool (Grant=builtin): reads files from skill's references/scripts/assets dirs with path traversal prevention - Add skill_manage tool (Grant=builtin): 6-action CRUD (create/edit/patch/delete/write_file/remove_file) with frontmatter validation, atomic writes, content scanner integration, and registry re-scan after mutations - Add slash-command dispatch to SkillRegistry: /name syntax resolves to skill, extracts remainder as user content - Add slash-command interception in LlmSessionActor: matched commands inject skill body as transient system message before LLM call; unmatched commands return deterministic error listing available commands - Register skill tools via WithSkillTools() extension method - Add 7 new slash-command dispatch tests - 917 tests passing, zero slopwatch violations
Add the critical IncludeNativeLibrariesForSelfExtract and EnableCompressionInSingleFile flags that match the production CI pipeline. Missing these causes SQLite native library failures at runtime. Also adds skill copy step, platform RID table, and common mistakes reference.
ConfigSchemaDoctorCheck now reports the actual validation errors and which schema file was loaded when validation fails. Previously it only said "Config does not match schema" with no diagnostic detail, making root cause analysis impossible. Also updates local-binary-swap skill to include Schemas directory copy step — stale schema at ~/.netclaw/bin/Schemas/ from previous installs was the root cause of false schema failures after binary swap.
OpenSpec change for security-tui-onboarding: - Reorder wizard: security posture moves to step 3 (after ChatServices) - New SecurityPosture step: explicit Personal/Team/Public selection with explanatory text, derives shell mode and audience defaults - New Channels step: per-channel audience assignment with left/right arrow cycling, dynamic add/remove via conversations.list API - Rework ACL step: type-to-filter Slack user search via users.list API instead of raw user ID copy-paste - Remove Exposure step (posture selection replaces it) - Includes wireframes for all new TUI screens
…narios - Channels step: skip when Slack disabled, fall back to manual channel name entry when conversations.list fails - ACL step: skip when Slack disabled, fall back to manual ID entry on both missing users:read scope and API failure - Onboarding: explicit scenarios for Slack-disabled skip logic
Update specs and tasks to reuse existing LookupSlackUserTool pagination and caching logic via extraction into a shared service, rather than reimplementing users.list in ISlackProbe with raw HTTP. Both the tool and init wizard consume the same code path.
…inal Use existing VirtualTerminal + VirtualInputSource pattern from InitWizardPageTests for headless integration tests of new security onboarding steps. Covers posture selection, audience cycling, channel add/remove, navigation, and Slack-disabled fallback paths.
…ble errors No silent degradation to manual entry when Slack APIs fail. If conversations.list or users.list fails, show the error reason and block until the user fixes it (retry with Enter, go back with Esc). Aligns with the no-silent-fallbacks rule in CLAUDE.md.
Reorder init wizard: Provider → ChatServices → SecurityPosture → ACL → Channels → Search → BrowserAutomation → Identity → HealthCheck. - Remove Exposure step (posture selection replaces it) - Add SecurityPosture step with Personal/Team/Public selection list - Add Channels step with per-channel audience cycling via left/right arrow keys, cursor navigation, d to remove channels - DeriveSecurityDefaults() pre-populates channel entries from posture - ChannelEntry class tracks display name, ID, audience, and DM flag - SyncChannelAudiencesFromEntries() replaces PopulateChannelAudiences() - Update all navigation tests for new step order - 239/242 CLI tests passing (3 unrelated health check timeouts) - Zero slopwatch violations
…ests - Move webhook URL collection to Identity step (sub-step 4, after timezone) - Add ViewModel tests: posture derivation, audience cycling, DM row guard - Add headless TUI tests: SecurityPosture renders options, Channels renders entries - Descope Slack API search (channel names and user IDs stay as text inputs) - Update OpenSpec tasks — all complete - Zero slopwatch violations, all tests passing
… restriction, tool tests 1. Slash-command IO failure now emits deterministic error to user instead of silently falling through to LLM (no-silent-fallbacks rule) 2. skill_manage restricted to Personal audience via IsProfileManagedTool — Public and Team sessions cannot create/edit/delete skills 3. Add 13 unit tests for SkillLoadTool, SkillReadResourceTool, and SkillManageTool covering path traversal, system skill protection, name validation, frontmatter validation, patch, delete, and write_file
… ChatServices Reorder: Provider → SecurityPosture → ChatServices → Channels → Search → BrowserAutomation → Identity → HealthCheck. Security posture is now step 2 (right after LLM provider) so it informs all downstream decisions. ACL (owner identity) folded into ChatServices as sub-step 6 since it's Slack-specific. Channels step populated via DeriveSecurityDefaults() when entering the step (after channel names have been collected in ChatServices). Removes standalone Acl wizard step entirely. Total steps reduced from 9 to 8.
…g, smart DM audience - Channels step now populated from raw channel names entered in ChatServices (no longer depends on conversations.list resolution happening first) - 'a' key opens text input to add a channel by name with deduplication - 'd' key removes focused channel (DM row protected) - DM audience defaults to Personal when only one allowed user (owner-only), otherwise follows posture default - Removed raw ID column from channel display (unnecessary before resolution) - Fixed GoBack_ReturnsToPreviousStep test for new step order
…rtup SkillIndexEnrichmentService.StartAsync was awaiting LLM sidecar calls synchronously, causing 10-second timeouts per skill when the LLM endpoint is slow or unreachable. This blocked the hosted service pipeline and caused the init wizard HealthCheck to time out waiting for daemon readiness. Now fires enrichment as a background task — daemon starts immediately with fallback descriptions, enrichment updates the index asynchronously.
Catch OperationCanceledException for clean shutdown and all other exceptions to prevent unobserved task exceptions from the detached background enrichment task.
Replace N sequential LLM sidecar calls (10s timeout each) with a single batch call (30s timeout total). Sends all uncached skill names and descriptions in one prompt, gets back a JSON object mapping names to trigger phrases. Parses with markdown fence stripping and case-insensitive key matching for robustness. With 4 skills, this reduces worst-case enrichment time from 40s to 30s and best-case from 4 round-trips to 1.
…ty_text as filtered - All slack_event_dropped and slack_event_filtered now logged at Info level instead of Debug so they appear in default daemon logs - empty_text events reclassified from Dropped to Filtered — these are expected noise (bot echoes, link unfurls), not lost user messages - Dropped counter should now only reflect genuinely unexpected drops (acl_denied, thread_not_initialized)
…dy posted The _postedThisTurn guard on TextOutput silently discarded all text after the first post in a turn. This caused the final LLM response to be lost when the model emitted intermediate "thinking aloud" text during tool loops. The user saw "Let me check..." but never the actual result. Remove the guard. Every TextOutput gets posted. If duplicates become a problem, deduplicate explicitly rather than silently dropping content.
Archived completed changes: - trust-tiers-security-stub (36/36 tasks) - compressed-skill-index (46/47 tasks, 1 deferred follow-up) - skill-tools-and-slash-commands (53/53 tasks) - security-tui-onboarding (49/49 tasks) Synced 6 new capability specs: skill-trust-tiers, skill-index-compression, skill-tools, slash-command-dispatch, security-posture-tui, channel-audience-tui Merged delta specs into 4 existing specs: netclaw-session (2 deltas), netclaw-tools, netclaw-onboarding, netclaw-cli
…delivery The _sawTextDelta flag prevented TextOutput from posting when streaming deltas had been seen in the same turn. During multi-step tool loops, intermediate text responses set _sawTextDelta via the streaming path, causing the final LLM response (sent as TextOutput) to be silently discarded. This is the second guard (after _postedThisTurn) that was dropping completed responses. Remove the guard entirely. TextOutput always posts regardless of streaming state. The streaming path (TextDeltaOutput → buffer → flush) handles its own posting independently.
…ping Restore the _sawTextDelta check on TextOutput to prevent duplicate posting when content was already delivered via streaming deltas. The guard now works correctly because BufferFlush resets _sawTextDelta after each LLM response — so the guard only suppresses the duplicate TextOutput for the current response, not future responses after subsequent tool calls in the same turn. This matches the TUI (ChatPage.cs) behavior which checks per-segment whether deltas were already rendered before displaying TextOutput.
…plicate delivery Add OutputFilter.TextStreaming flag for streaming deltas (TextDeltaOutput, BufferFlush). TextOutput uses the existing Text flag. Adapters subscribe to one or the other: - Slack subscribes to Text (final assembled text) — posts once per response - TUI subscribes to Full (gets both, handles dedup via segment tracking) This eliminates the root cause of both duplicate posting AND dropped final responses in Slack. The _sawTextDelta and _postedThisTurn guards are removed from SlackThreadBindingActor since it no longer receives streaming events. The streaming buffer, delta tracking, and BufferFlush handler are removed from the Slack adapter as dead code.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
OutputFilter.TextStreamingflag to separate streaming deltas from final assembled textTextonly (final responses), TUI getsTextStreamingfor live renderingSlackThreadBindingActor(buffer, delta tracking, flush handler)empty_textevents from Dropped to Filtered counterRoot Cause
The session actor emitted both
TextDeltaOutput(streaming) andTextOutput(final) under the sameOutputFilter.Textflag. Slack received both, leading to:Fix
TextDeltaOutputandBufferFlushnow emit underOutputFilter.TextStreaming. Adapters subscribe to one path:Text | Files— gets final assembled text, posts onceFull— gets both, handles dedup via segment tracking (existing behavior)Test plan
netclaw chatstill renders streaming tokens