fix(slack): respond to thread replies without requiring @mention

[synner88]

Summary

• Fix host-side bug where on_respond callback never commits WASM workspace writes or injects workspace reader
• Add thread participation tracking to Slack WASM channel so the bot responds to follow-up messages in threads where it previously participated

Closes #1404

Changes

Host: src/channels/wasm/wrapper.rs

• Add inject_workspace_reader() to on_respond capabilities (was missing, unlike all other callbacks)
• Clone workspace_store into the async closure
• Call take_pending_writes() + commit_writes() after on_respond WASM execution completes

Slack WASM channel: channels-src/slack/src/lib.rs

• Add ACTIVE_THREADS_PREFIX constant for workspace-persisted thread tracking
• In on_respond(): record thread key (channel/thread_ts) when the bot replies in a thread
• In handle_slack_event() "message" handler: check if an incoming channel message is a reply in a tracked thread, and process it alongside DMs

Test plan

• @mention bot in a channel → bot replies in thread
• Send follow-up in the same thread without @mention → bot replies
• Send message in a different/unrelated thread without @mention → bot ignores (no false positives)
• DM behavior unchanged (still works without @mention)
• Restart IronClaw → thread tracking persists via workspace storage
• cargo clippy --all --benches --tests --examples --all-features passes with zero warnings

🤖 Generated with Claude Code

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly improves the user experience with the Slack bot by enabling it to maintain context within threads. Users no longer need to repeatedly @mention the bot for follow-up questions or comments in an ongoing thread. This is achieved by introducing thread participation tracking and fixing a host-side issue that prevented the proper persistence of WASM workspace data, ensuring that the bot's memory of active threads is correctly managed and saved.

Highlights

• Enhanced Slack Bot Interaction: The bot can now automatically respond to follow-up messages within Slack threads where it has previously participated, removing the need for users to explicitly @mention it for continued conversation.
• WASM Host-Side Fixes: A critical bug in the host-side on_respond callback for WASM channels was resolved, ensuring that workspace writes are correctly committed and the workspace reader is properly injected.
• Thread Participation Tracking: Implemented a mechanism within the Slack WASM channel to track threads where the bot has replied, enabling intelligent filtering of incoming messages.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

The pull request introduces a valuable feature allowing the Slack bot to respond to thread replies without requiring an explicit @mention. The changes correctly implement thread tracking in the Slack WASM channel and ensure that workspace writes are committed by the host. However, there are a couple of places where error handling for workspace operations could be improved to prevent silent failures, which are detailed in the review comments.

[serrrfirat]

Medium Severity: Variable Shadowing Confusion

In src/channels/wasm/wrapper.rs around line 1646, the PR changes host_state from immutable to mutable:

Before:

let host_state = Self::extract_host_state(&mut store, &prepared.name, &capabilities);

After (this PR):

let mut host_state = Self::extract_host_state(&mut store, &prepared.name, &capabilities);
let pending_writes = host_state.take_pending_writes();
workspace_store.commit_writes(&pending_writes);

The change from immutable to mutable is correct (needed for take_pending_writes()), but this suggests the original code had a latent bug where host_state was extracted but never used to commit writes (notice the underscore prefix _host_state at line 1664, indicating it was unused).

Fix: This is actually fixed correctly in the PR. Just noting it confirms the original bug: workspace writes were never committed from on_respond.

[serrrfirat]

Low Severity: Thread Key Format Validation Missing

In channels-src/slack/src/lib.rs at line 271, the thread key format is:

let thread_key = format!("{}{}/{}", ACTIVE_THREADS_PREFIX, metadata.channel, thread_ts);
// Evaluates to: "state/threads/{channel}/{thread_ts}"

There's no validation that channel or thread_ts don't contain forward slashes. While Slack channel IDs are typically alphanumeric (e.g., "C1234567890") and thread_ts is a timestamp (e.g., "1234567890.123456"), so this is likely safe in practice:

Risks:

1. No explicit validation - if Slack's format changes or malicious payload is injected, path traversal issues could occur
2. Undocumented assumption - code assumes these fields are path-safe
3. Potential collisions - channel "C12" with thread "34/56" collides with channel "C12/34" with thread "56"

Fix:

1. Add validation: assert!(!channel.contains('/') && !thread_ts.contains('/'));
2. Or use URL-safe encoding: let thread_key = format!("{}{}", ACTIVE_THREADS_PREFIX, urlencoding::encode(&format!("{}/{}", channel, thread_ts)));
3. Document the key format in the ACTIVE_THREADS_PREFIX constant comment

[serrrfirat]

Low Severity: Missing Cleanup Mechanism

The PR adds thread tracking writes but no cleanup mechanism. Over time, unbounded state accumulates:

• Every thread the bot participates in creates: state/threads/{channel}/{thread_ts}
• No TTL, no expiration, no periodic cleanup
• In a busy workspace: hundreds or thousands of threads per month
• Workspace storage grows indefinitely

Example: 10 channels × 100 threads/month = 1000 new keys/month. After a year: 12,000+ keys.

Fix options:

1. TTL-based expiration (simplest): Store timestamp instead of "1", check age on read (see finding #3)
2. Periodic cleanup: Background task removes markers older than N days
3. Max-threads limit: LRU eviction when count exceeds threshold
4. Accept unbounded growth: Document as known limitation in CLAUDE.md

Recommend option 1 (TTL) as it requires minimal changes: store now_millis().to_string() instead of "1", and check now_millis() - parse(value) < 24*3600*1000 on read.

[ilblackdragon]

Review: fix(slack): respond to thread replies without requiring @mention

Must-fix

1. Thread tracking happens before Slack API success — workspace_write at line 269 runs before chat.postMessage. If the API call fails, the thread is marked as "participated" even though no message was sent. Move tracking to after the slack_response.ok check (after line 308).

2. Fix call_on_broadcast and call_on_status — these callbacks have the identical missing inject_workspace_reader + commit_writes bug. Per review discipline: "Fix the pattern, not just the instance."

Should-fix

3. Store timestamp instead of "1" — enables future TTL-based expiration without migration. Change to channel_host::time_now().to_string().

4. Add regression test for the host-side workspace commit fix, per project testing rules.

Nice-to-have

5. Add TTL-based expiration (e.g., ignore threads older than 7 days) to prevent unbounded growth.

[serrrfirat]

Thanks @synner88 for the original fix and the earlier follow-up on the initial review comments. I pushed the remaining hardening changes onto the PR branch, reran the review locally, and the refreshed CI is green on 43cc54d9.

[serrrfirat]

Addressed the review comments one by one on 43cc54d9:

1. Ignored workspace_write result
   Fixed by routing thread tracking through track_active_thread(...) and propagating persistence failures instead of silently swallowing them.

2. workspace_read error handling
   This one remained a false positive. The WIT signature for workspace-read returns option<string>, not a result, so there is no hidden error case to unwrap.

3. Race / false-positive thread activation
   Fixed by moving thread activation to after chat.postMessage succeeds with HTTP 200 and ok=true. Failed send attempts no longer mark the thread as active.

4. Missing error propagation for tracking state
   Fixed by making the post-send thread-state persistence path return an error if it cannot write the updated state.

5. No expiration / unbounded thread tracking
   Fixed by replacing immortal per-thread markers with timestamped state in state/active_threads.json, pruning expired entries on read/write, and capping the tracked set to the 256 most-recent threads.

6. Missing automated coverage
   Added Slack tests for thread keying, TTL boundary behavior, expiry pruning, and bounded pruning, plus wrapper tests covering workspace-reader injection behavior.

7. Incomplete wrapper audit
   Audited the remaining callback surface and fixed on_broadcast, on_status, and the background status repeater so they all inject workspace reads and commit pending workspace writes, instead of limiting that behavior to on_respond.

8. Extra hardening beyond the review text
   Moved Slack attachment download/storage behind the actual message-eligibility checks so ignored channel traffic does not trigger unnecessary file downloads.

CI is green on this revision.

[ilblackdragon]

Code Review

The PR is in good shape after addressing all prior review feedback. The core bugs (missing inject_workspace_reader + commit_writes in callbacks, thread tracking before API success) are properly fixed, and the thread participation tracking design is solid — TTL expiry, bounded storage, LRU eviction, and proper error propagation.

Minor suggestions

1. Double prune in track_active_thread (nit)

The second prune_active_threads call is unnecessary — the just-inserted entry has last_seen_millis == now_millis so it can never be TTL-pruned, and we go from at most 256 to 257 entries. A single prune before insert is sufficient:

fn track_active_thread(channel: &str, thread_ts: &str) -> Result<(), String> {
    let now_millis = channel_host::now_millis();
    let mut active_threads = load_active_threads();
    prune_active_threads(&mut active_threads, now_millis);
    active_threads.insert(active_thread_key(channel, thread_ts), now_millis);
    // second prune_active_threads call removed — at most 257 entries is fine
    persist_active_threads(&active_threads)
}

2. is_active_thread writes back on read path

Every threaded channel message deserializes the full thread map and potentially writes it back (if pruning removed stale entries). Consider moving the stale-entry cleanup to track_active_thread only (write path) so is_active_thread stays a pure read — avoids unnecessary I/O in busy workspaces.

3. call_on_shutdown not updated

on_shutdown still uses self.capabilities.clone() without inject_workspace_reader. If any WASM channel ever does workspace reads during shutdown, it'll hit the same latent bug this PR fixes. Worth fixing for completeness or documenting that shutdown intentionally doesn't support workspace writes.

Otherwise LGTM — nice work addressing all the earlier feedback.