slack: enforce reaction notification policy [AI]

[pgondhi987]

Summary

• Problem: Slack reaction webhook handling could queue reaction events after base authorization without applying the configured reaction notification policy.
• Why it matters: Operators expect reactionNotifications and reactionAllowlist to control which reaction events can reach the agent event queue.
• What changed: Added a Slack reaction notification gate for off, own, all, and allowlist modes before queueing system events.
• What did NOT change (scope boundary): Slack signature validation, base sender/channel authorization, config schema, manifests, and non-reaction Slack handling were not changed.

AI-assisted: Yes.

Change Type

• Bug fix
• Feature
• Refactor required for the fix
• Docs
• Security hardening
• Chore/infra

Scope

• Gateway / orchestration
• Skills / tool execution
• Auth / tokens
• Memory / storage
• Integrations
• API / contracts
• UI / DX
• CI/CD / infra

Linked Issue/PR

• None included in public PR text.
• This PR addresses a bug or regression

Real Behavior Proof

• Behavior or issue addressed: Slack reaction events now apply the configured reaction notification mode before entering the agent event queue.
• Real environment tested: Not run in this constrained drafting step.
• Exact steps or command run after this patch: Not run; the supervisor requested no package or shell verification commands for this metadata step.
• Evidence after change: Regression coverage was added in extensions/slack/src/monitor/events/reactions.test.ts; command output was not collected.
• Observed result after change: Static review confirms the handler checks off, own, and allowlist before enqueueSystemEvent(...).
• What was not tested: Live Slack webhook delivery and local test execution.
• Before evidence: Not collected in this step.

Root Cause

• Root cause: The Slack reaction handler reused the generic system-event authorization path but did not apply the dedicated reaction notification policy before queueing.
• Missing detection / guardrail: Existing reaction tests covered sender/channel authorization, but not the reaction notification modes.
• Contributing context (if known): The monitor context already carried reactionMode and reactionAllowlist, but the handler did not consume them.

Regression Test Plan

• Coverage level that should have caught this:
  • Unit test
  • Seam / integration test
  • End-to-end test
  • Existing coverage already sufficient
• Target test or file: extensions/slack/src/monitor/events/reactions.test.ts
• Scenario the test should lock in: off drops reaction events, own only allows reactions on bot-authored messages, and allowlist only allows configured reaction senders.
• Why this is the smallest reliable guardrail: The regression is isolated to the Slack reaction event handler before system-event enqueueing.
• Existing test that already covers this (if any): Existing tests covered base DM/channel authorization only.
• If no new test is added, why not: N/A

User-visible / Behavior Changes

Slack reaction webhook events now honor reactionNotifications and reactionAllowlist before reaching the agent system-event queue.

Diagram

Before:
Slack reaction event -> base sender/channel auth -> queue system event

After:
Slack reaction event -> base sender/channel auth -> reaction notification policy -> queue or drop

Security Impact

• New permissions/capabilities? (Yes/No): No
• Secrets/tokens handling changed? (Yes/No): No
• New/changed network calls? (Yes/No): No
• Command/tool execution surface changed? (Yes/No): No
• Data access scope changed? (Yes/No): Yes, reduced
• If any Yes, explain risk + mitigation: Reaction event text is now suppressed before queueing when the configured Slack reaction policy denies it. This narrows event flow and does not expand access.

Repro + Verification

Environment

• OS: Not run in this step
• Runtime/container: Not run in this step
• Model/provider: N/A
• Integration/channel (if any): Slack
• Relevant config (redacted): reactionNotifications modes off, own, all, and allowlist

Steps

1. Configure Slack reaction notification mode.
2. Send a Slack reaction_added or reaction_removed event that passes base sender/channel authorization.
3. Observe whether a system event is queued according to the configured reaction policy.

Expected

• Denied reaction modes do not queue system events.
• Allowed reaction modes continue to queue system events after base authorization.

Actual

• Not run in this step; covered by added unit test scenarios.

Evidence

• Failing test/log before + passing after
• Trace/log snippets
• Screenshot/recording
• Perf numbers (if relevant)

No command evidence collected in this constrained step.

Human Verification

• Verified scenarios: Reviewed the handler path and added regression cases for off, own, and allowlist.
• Edge cases checked: Empty reaction allowlist fails closed; own mode requires the reacted message author to be the Slack bot user.
• What you did not verify: Live Slack delivery and package test execution.

Review Conversations

• I replied to or resolved every bot review conversation I addressed in this PR.
• I left unresolved only the conversations that still need reviewer or maintainer judgment.

Compatibility / Migration

• Backward compatible? (Yes/No): Yes
• Config/env changes? (Yes/No): No
• Migration needed? (Yes/No): No
• If yes, exact upgrade steps: N/A

Risks and Mitigations

• Risk: own mode depends on Slack reaction events carrying the original message author.
  • Mitigation: The handler fails closed for own mode when the bot-authored message condition is not met, and existing all mode remains available for deployments that intentionally want broader reaction notifications.

[clawsweeper]

Codex review: needs real behavior proof before merge.

Summary
Adds a Slack reaction notification gate and tests so reaction events honor reactionNotifications and reactionAllowlist before queueing system events.

Reproducibility: yes. From source inspection, current main reads the Slack reaction policy into context but the reaction handler queues authorized reaction system events without checking it; a focused harness can set off, own, or allowlist and observe queue calls.

Real behavior proof
Needs real behavior proof before merge: The PR body says the changed behavior was not run in a real environment and provides no terminal output, logs, live Slack proof, screenshot, recording, or linked artifact. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, ask a maintainer to comment @clawsweeper re-review.

Next step before merge
The remaining blockers are contributor real behavior proof and explicit maintainer handling for the protected label, not an automation-repairable code defect.

Security
Cleared: The diff narrows Slack reaction event flow and does not add dependencies, workflow changes, secret handling, network calls, or execution surface.

Review details

Best possible solution:

Merge the narrow Slack handler fix after redacted real behavior proof is added and a maintainer approves the protected Slack policy change.

Do we have a high-confidence way to reproduce the issue?

Yes. From source inspection, current main reads the Slack reaction policy into context but the reaction handler queues authorized reaction system events without checking it; a focused harness can set off, own, or allowlist and observe queue calls.

Is this the best way to solve the issue?

Yes, subject to proof. Gating in the Slack reaction handler with the existing allowlist helper before enqueueSystemEvent(...) is the narrow maintainable fix without changing schema or manifests.

What I checked:

• Current main behavior: Current main validates the Slack reaction item, runs base sender/channel authorization, resolves labels, and calls enqueueSystemEvent(...) without checking ctx.reactionMode or ctx.reactionAllowlist. (extensions/slack/src/monitor/events/reactions.ts:21, f6d787cc5c1f)
• Existing config contract: Slack already documents and types reactionNotifications as off | own | all | allowlist, with reactionAllowlist for allowlist mode. (src/config/types.slack.ts:178, f6d787cc5c1f)
• Runtime context already carries policy: The Slack provider reads slackCfg.reactionNotifications ?? "own" and slackCfg.reactionAllowlist ?? [] into the monitor context. (extensions/slack/src/monitor/provider.ts:232, f6d787cc5c1f)
• PR implementation: The PR adds shouldEmitSlackReactionNotification(...), early drops off and non-bot own events, applies allowlist matching, and marks queued reaction system events untrusted. (extensions/slack/src/monitor/events/reactions.ts:10, 89b4ea1881b3)
• Regression coverage: The PR adds focused reaction handler cases for off, own, empty allowlist, denied allowlist, and allowed allowlist behavior. (extensions/slack/src/monitor/events/reactions.test.ts:136, 89b4ea1881b3)
• Real behavior proof gate: The PR body explicitly says no real environment, command verification, live Slack delivery, or command output was collected; only static review and tests were added. (89b4ea1881b3)

Likely related people:

• steipete: Recent commit history for Slack reaction, provider, and test paths shows repeated Slack monitor/runtime changes, including moving Slack system events onto the channel runtime and adjacent Slack handler/test refactors. (role: recent area contributor; confidence: high; commits: d83e3afc5642, f0000ab72d01, a0fb7fb04547; files: extensions/slack/src/monitor/events/reactions.ts, extensions/slack/src/monitor/events/reactions.test.ts, extensions/slack/src/monitor/provider.ts)
• pgondhi987: Beyond authoring this PR, prior merged Slack provider work by this account touched adjacent Slack allowlist/runtime behavior. (role: recent adjacent contributor; confidence: medium; commits: b895c6d939fe; files: extensions/slack/src/monitor/provider.ts)
• scoootscooob: Moved Slack channel implementation files under extensions/slack/src/, which explains the current file location but is less specific to reaction policy behavior. (role: migration contributor; confidence: low; commits: 8746362f5ebf; files: extensions/slack/src/monitor/events/reactions.ts)

Remaining risk / open question:

• No contributor-supplied real Slack, terminal, live-output, or redacted log proof shows the after-fix behavior.
• own mode depends on Slack reaction payloads carrying item_user for the original message author, so live proof should include that path.

Codex review notes: model gpt-5.5, reasoning high; reviewed against f6d787cc5c1f.

[pgondhi987]

Not applicable to this automation stage; changelog/release-note and external real behavior proof requirements are handled outside auto-pr stages.

Quoted comment from @clawsweeper:

Codex review: needs real behavior proof before merge.

Summary
The PR adds a Slack reaction policy gate in the reaction event handler and extends its harness/tests for off, own, and allowlist modes.

Reproducibility: yes. From source inspection, current main reads reactionNotifications and reactionAllowlist into the Slack monitor context but the reaction handler queues authorized reactions without checking them; a focused harness can set off, own, or allowlist and observe enqueueSystemEvent(...).

Real behavior proof
Needs real behavior proof before merge: The PR body says no real environment, command, live Slack delivery, or after-fix output was run, so contributor-supplied redacted terminal/log/live Slack proof is still required before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, ask a maintainer to comment @clawsweeper re-review.

Next step before merge
Contributor real behavior proof and explicit maintainer handling are required for this protected Slack PR; there is no narrow ClawSweeper repair task right now.

Security
Cleared: The diff narrows Slack reaction event flow and does not add dependencies, workflow changes, secret handling changes, new network calls, or broader execution surface.

Review details

Best possible solution:

Merge a narrow Slack handler fix after the contributor adds redacted real behavior proof and a maintainer approves the protected Slack policy change.

Do we have a high-confidence way to reproduce the issue?

Yes. From source inspection, current main reads reactionNotifications and reactionAllowlist into the Slack monitor context but the reaction handler queues authorized reactions without checking them; a focused harness can set off, own, or allowlist and observe enqueueSystemEvent(...).

Is this the best way to solve the issue?

Yes, subject to proof. Gating in the Slack reaction handler with the existing allowlist helper before enqueueSystemEvent(...) is the narrow maintainable fix; the remaining blockers are contributor proof and maintainer review, not a different code direction.

What I checked:

• Current main behavior: Current main carries Slack reaction events through item validation, base sender/channel authorization, user-label resolution, and enqueueSystemEvent(...) without consulting ctx.reactionMode or ctx.reactionAllowlist. (extensions/slack/src/monitor/events/reactions.ts:21, 042a8f106ed1)
• Existing config contract: Slack config already exposes reactionNotifications with default own and reactionAllowlist; provider code reads both into monitor context, docs list the modes, and the zod schema accepts off, own, all, and allowlist. (extensions/slack/src/monitor/provider.ts:232, 042a8f106ed1)
• PR implementation: PR head 911703f90e adds shouldEmitSlackReactionNotification(...), fast-drops off and non-bot own events before context resolution, applies allowlist policy before enqueueing, and adds focused regression cases. (extensions/slack/src/monitor/events/reactions.ts:10, 911703f90eb5)
• Real behavior proof: The PR body explicitly says no real environment, command verification, live Slack delivery, or command output was collected; the only after-change evidence is added regression coverage and static review. (911703f90eb5)
• Protected label: Live PR metadata includes the maintainer label, so this cleanup workflow must keep the PR open for explicit maintainer handling. (911703f90eb5)
• Related search: GitHub issue search found prior Slack reaction-notification PRs fix(slack): add reactionNotifications config check to reactions handler #6089 and fix(slack): add reactionNotifications config check to reactions handler #2638, but both are closed unmerged, so there is no merged canonical fix superseding this PR.

Likely related people:

• steipete: GitHub commit history for extensions/slack/src/monitor/events/reactions.ts, extensions/slack/src/monitor/provider.ts, and adjacent tests shows repeated recent Slack monitor/provider changes, including the system-event runtime move and later Slack handler refactors. (role: recent area contributor; confidence: high; commits: d83e3afc5642, f0000ab72d01, a0fb7fb04547; files: extensions/slack/src/monitor/events/reactions.ts, extensions/slack/src/monitor/events/reactions.test.ts, extensions/slack/src/monitor/provider.ts)
• pgondhi987: Beyond authoring this PR, prior merged Slack provider work by this account touched the same provider/allowlist runtime area, so they are an adjacent Slack policy contributor. (role: recent adjacent contributor; confidence: medium; commits: b895c6d939fe; files: extensions/slack/src/monitor/provider.ts)
• scoootscooob: The Slack channel code was moved under extensions/slack/src/ in a large extension migration, which is relevant historical ownership context for the current file location but less specific to reaction policy behavior. (role: migration contributor; confidence: low; commits: 8746362f5ebf; files: extensions/slack/src/monitor/events/reactions.ts)

Remaining risk / open question:

• No after-fix real Slack, terminal, log, or live-output proof is present; the PR body says the changed behavior was not run.
• The own mode path depends on Slack reaction events carrying item_user for bot-authored messages, so live proof should include that path.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 042a8f106ed1.