Answer top-level channel mentions; re-introduce hardened dedup (v0.1.19)#31
Merged
Conversation
…v0.1.19) Re-introduce the at-least-once dedup + permission-generation hardening (previously rolled back), and fix a real bug in it: a top-level channel mention went unanswered. Root cause: Slack delivers both a `message` and an `app_mention` event for the same top-level mention, sharing one client_msg_id. The `message` handler ran first and consumed the shared seen-id in acceptMessage, even though a top-level channel post is handled by app_mention (handleChannelMessage drops it). The app_mention handler then rejected its own event as a duplicate → no reply. Fix: acceptMessage now returns BEFORE dedup for top-level channel posts, so the id is never consumed and app_mention can claim it as fresh. Extracted acceptMessage + inbound types into src/slack/messages.ts as a pure, injectable function and added regression tests (verified in dev: pre-fix unanswered, post-fix answers correctly). Threaded mentions and DMs are unaffected (message handler processes those). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughAdds Slack event deduplication and filtering, per-process Claude instance tracking, instanceId-scoped permission handling, and entrypoint wiring for duplicate suppression and stale-permission cleanup. The package version is also bumped. ChangesDedup and generation-aware permissions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/permission.ts (1)
40-68: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy liftBind pending permissions to an opaque action key, not just
requestId.
instanceIdis stored, but the registry still overwrites/resolves byreq.requestId. Since the Slack button value is also onlyrequestId, a stale old button can resolve a newer pending entry with the same reused request ID and then pass the newerinstanceIdcheck. Use a unique per-button key, e.g.instanceId + requestIdor a random token, as the registry key and Slack action value while keepingrequestIdonly for the Claude control response.As per path instructions,
src/**/*.tsmust keep an eye on “permission bridging (Block Kit allow/deny)” and “session mapping (Slack thread_ts ⇄ Claude session_id)”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/permission.ts` around lines 40 - 68, The pending-permission registry is keyed only by req.requestId, which allows stale Slack actions to collide with newer entries and bypass the instanceId guard. Update Permission registry methods like register, resolve, and has to use an opaque per-button action key (for example a composite of instanceId plus requestId or a generated token) as the stored lookup key, and pass that same key through the Slack Block Kit button value for allow/deny actions. Keep req.requestId only for the Claude control response, and ensure the permission bridging and session mapping code still resolve the correct pending entry per Slack thread/session.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/index.ts`:
- Around line 143-177: The postPermission flow registers the request before the
Slack calls, but if flushStream or post fails the permission stays registered
and can dead-end. Update postPermission to wrap the awaited Slack section in
error handling and, on rejection, clean up the pending entry via the permissions
API (or mark it failed) before rethrowing or returning. Use the existing
postPermission and permissions.register/permissions.has flow to locate the
cleanup point.
---
Outside diff comments:
In `@src/permission.ts`:
- Around line 40-68: The pending-permission registry is keyed only by
req.requestId, which allows stale Slack actions to collide with newer entries
and bypass the instanceId guard. Update Permission registry methods like
register, resolve, and has to use an opaque per-button action key (for example a
composite of instanceId plus requestId or a generated token) as the stored
lookup key, and pass that same key through the Slack Block Kit button value for
allow/deny actions. Keep req.requestId only for the Claude control response, and
ensure the permission bridging and session mapping code still resolve the
correct pending entry per Slack thread/session.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e2de83ef-cc54-41fc-9e11-1ad9103b66b6
📒 Files selected for processing (10)
package.jsonsrc/claude.tssrc/dedup.test.tssrc/dedup.tssrc/index.tssrc/permission.test.tssrc/permission.tssrc/session.tssrc/slack/messages.test.tssrc/slack/messages.ts
…ailure Address two CodeRabbit findings on the permission bridge: 1) (Critical) The registry was keyed by req.requestId alone. Claude can reuse a request_id across process generations, so a stale button could resolve a newer pending entry and then pass the instanceId guard. Key the registry and the Slack button value by an opaque `instanceId:requestId` action key instead; requestId is kept only for the Claude control response. A stale button now finds no matching entry. Added a regression test. 2) (Major) If posting the buttons to Slack failed, the registration was left dangling. postPermission now drops it (resolve) and logs the error. Verified in dev (manual mode): allow and deny both work end-to-end. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/permission.ts`:
- Around line 10-15: The action key generation in `permission.ts` is still
repeatable across app restarts because `instanceId` is only a module-level
counter combined with `requestId`, so stale Slack buttons can collide with new
pending entries after a restart. Update the `instanceId`/action-key creation
path to include a boot-scoped non-repeating token (for example a random nonce or
UUID initialized once per process) and carry it through the permission flow used
by `requestId`, `registerPendingAction`, and the Slack button value. Ensure the
resulting key remains unique for the lifetime of the app process and cannot
match a previous restart’s pending action.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 27bf9f94-9a2c-4be2-8e95-806683558725
📒 Files selected for processing (3)
src/index.tssrc/permission.test.tssrc/permission.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/index.ts
… collisions The permission action key embeds instanceId, but nextInstanceId reset to 1 on every Iris restart. A stale Slack button from before a restart could then collide with a new pending entry if Claude reused the same request_id. Seed the counter from a random boot offset so ids do not repeat across restarts, while staying monotonic within a run. (CodeRabbit follow-up.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
概要
トップレベルのチャンネル mention が無応答になるバグを修正し、ロールバックしていた dedup + 権限世代検証のハードニングを 修正版で再導入 します。今後の開発は本 OSS リポジトリをベースに一本化します。
バグ(トップレベル mention 無応答)
症状: チャンネルでスレッド外に
@irisすると無応答。根本原因: Slack はトップレベル mention に対し
messageとapp_mentionの 2 イベントを同一client_msg_idで配信する。messageハンドラが先に走りacceptMessageで共有 seen-id を消費するが、トップレベルの channel post はhandleChannelMessageがthread_ts無しで drop する(本来 app_mention 担当)。その後app_mentionハンドラが自分のイベントを duplicate と誤判定 → 返信されない。実機で再現・確定(dev ログでイベント順序とパスを観測)。
修正
acceptMessageで トップレベル channel post を dedup の前に return(seen-id を消費しない)。これでapp_mentionが自分のイベントを fresh として処理できる。acceptMessageと inbound 型をsrc/slack/messages.tsに純粋関数として切り出し、SeenSet/nowを注入可能化。src/slack/messages.test.ts(9 ケース、トップレベルが id を消費しないことを含む)。スレッド内 mention・DM は影響なし(message ハンドラが処理する経路)。
検証
pnpm verify全通過、117 tests pass。再導入した内容(v0.1.18 → v0.1.19)
at-least-once dedup(
src/dedup.ts)、権限ボタンの世代検証(instanceId)、プロセス exit 時の pending 権限 drain。いずれも以前 CodeRabbit 承認済みの正当なハードニング。今回そのバグを直した上で再導入。🤖 Generated with Claude Code
Summary by CodeRabbit
0.1.19.