fix(slack): notify sender on denied @-mention instead of dropping silently#4933
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThis PR adds a build-time patch that injects bounded sender-facing feedback (ephemeral in-channel, with DM fallback) when explicit Slack ChangesSlack bounded denial feedback
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Trivy (0.69.3)Trivy execution failed: 2026-06-08T03:37:08Z FATAL Fatal error run error: fs scan error: scan error: scan failed: failed analysis: post analysis error: post analysis error: ansible scan error: fs filter error: fs filter error: walk error open node_modules/cross-spawn/node_modules/.bin/node-which: permission denied: open node_modules/cross-spawn/node_modules/.bin/node-which: permission denied Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/openclaw-slack-deny-feedback-patch.test.ts (1)
122-168: ⚡ Quick winCover the non-
app_mentionexplicit-mention branch.The patch predicate also fires when
explicitlyMentionedBotUserorexplicitlyMentionedBotSubteamis set, but this suite only exercises theopts.source === "app_mention"path. Adding asource: "message"case with one of those flags set would protect the other half of the contract.🤖 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 `@test/openclaw-slack-deny-feedback-patch.test.ts` around lines 122 - 168, Add a new test case that exercises the non-"app_mention" explicit-mention branch by calling runPatchedDenyPath(patched, { opts: { source: "message", explicitlyMentionedBotUser: true } }) (or explicitlyMentionedBotSubteam: true) and assert it behaves like the app_mention explicit-mention case: result is null, exactly one call made with method "chat.postEphemeral" (or fallback to DM when ephemeralFails is true) and the ephemeral call targets the denied user (channel "C1"/DM "DU999DENIED") and does not leak internal IDs or "allowlist" text; place this between the existing mention and silent checks in the same it(...) block.
🤖 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 `@scripts/patch-openclaw-slack-deny-feedback.js`:
- Around line 1-3: The new CLI was added as a .js file but CI requires
TypeScript: rename scripts/patch-openclaw-slack-deny-feedback.js to
scripts/patch-openclaw-slack-deny-feedback.ts (update the file contents as
needed to be valid TypeScript), and then update every call site that references
that script (Dockerfile/entrypoint, package.json scripts, and CI/test job
definitions) to point to the .ts path or invoke it via ts-node/ts-node-esm (or
ensure it is compiled first); specifically search for the literal
"scripts/patch-openclaw-slack-deny-feedback.js" and replace with the new .ts
invocation and make sure execution method (shebang vs node runner) is adjusted
to run TypeScript in your environment.
---
Nitpick comments:
In `@test/openclaw-slack-deny-feedback-patch.test.ts`:
- Around line 122-168: Add a new test case that exercises the non-"app_mention"
explicit-mention branch by calling runPatchedDenyPath(patched, { opts: { source:
"message", explicitlyMentionedBotUser: true } }) (or
explicitlyMentionedBotSubteam: true) and assert it behaves like the app_mention
explicit-mention case: result is null, exactly one call made with method
"chat.postEphemeral" (or fallback to DM when ephemeralFails is true) and the
ephemeral call targets the denied user (channel "C1"/DM "DU999DENIED") and does
not leak internal IDs or "allowlist" text; place this between the existing
mention and silent checks in the same it(...) block.
🪄 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: Enterprise
Run ID: 28c03491-7d07-4c0c-8291-2069660a2444
📒 Files selected for processing (5)
Dockerfilescripts/patch-openclaw-slack-deny-feedback.jstest/e2e/lib/slack-api-proof.shtest/e2e/test-messaging-providers.shtest/openclaw-slack-deny-feedback-patch.test.ts
1bd6d15 to
4e17ade
Compare
|
Thanks @coderabbitai — both addressed:
|
|
Tip For best results, initiate chat on the files or code changes.
🐇 ✨ |
| */ | ||
|
|
||
| import { existsSync, readdirSync, readFileSync, realpathSync, writeFileSync } from "node:fs"; | ||
| import { basename, join, relative, resolve } from "node:path"; |
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 `@Dockerfile`:
- Around line 585-595: The RUN invoking
/usr/local/lib/nemoclaw/patch-openclaw-slack-deny-feedback.mts currently runs
after USER sandbox and the script walks /usr/local/lib/node_modules causing
EACCES if global OpenClaw packages exist; fix by either moving this RUN into the
same root-owned phase as the other OpenClaw shims (so it runs before the USER
sandbox directive) or restrict the script invocation to only writable roots
(remove /usr/local/lib/node_modules from the args and only pass
/sandbox/.openclaw or update the script to avoid scanning
/usr/local/lib/node_modules). Ensure you adjust the Dockerfile around the USER
sandbox declaration and the RUN node ... patch-openclaw-slack-deny-feedback.mts
invocation accordingly.
🪄 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: Enterprise
Run ID: 82ab0719-fccb-4bca-bee6-d7d058719335
📒 Files selected for processing (5)
Dockerfilescripts/patch-openclaw-slack-deny-feedback.mtstest/e2e/lib/slack-api-proof.shtest/e2e/test-messaging-providers.shtest/openclaw-slack-deny-feedback-patch.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- test/e2e/test-messaging-providers.sh
- test/openclaw-slack-deny-feedback-patch.test.ts
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 `@scripts/patch-openclaw-slack-deny-feedback.mts`:
- Around line 145-156: The catch block after client.chat.postEphemeral currently
always falls back to a DM; change it so it only does the DM-fallback when the
caught ephemeralError indicates a definitive non-delivery Slack error.
Implement: define an array of nonDeliveryCodes (e.g.,
"user_not_in_channel","not_in_channel","channel_not_found","cannot_reply_to_message",
etc.), inspect the caught ephemeralError for a Slack error code (check typical
shapes like ephemeralError.data?.error or ephemeralError.error), and only run
the client.conversations.open / client.chat.postMessage fallback when that code
is in nonDeliveryCodes; otherwise just log the ephemeralError (preserve the
existing ctx?.logger?.warn call) and return without sending a DM. Target the
catch (ephemeralError) block around client.chat.postEphemeral and the subsequent
DM fallback code (client.conversations?.open and client.chat.postMessage).
🪄 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: Enterprise
Run ID: 12090071-6f45-4564-a9c1-85aeaad7c11e
📒 Files selected for processing (5)
Dockerfilescripts/patch-openclaw-slack-deny-feedback.mtstest/e2e/lib/slack-api-proof.shtest/e2e/test-messaging-providers.shtest/openclaw-slack-deny-feedback-patch.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- test/e2e/test-messaging-providers.sh
- test/e2e/lib/slack-api-proof.sh
- Dockerfile
- test/openclaw-slack-deny-feedback-patch.test.ts
…ently A non-allowlisted human who explicitly @-mentions the bot in a Slack channel had their command correctly blocked by the channel-users allowlist, but OpenClaw dropped the event with no visible feedback, so the sender could not tell the bot saw them (NemoClaw NVIDIA#4752). Add a NemoClaw-owned compatibility patch to the installed @openclaw/slack prepare deny path that keeps the command blocked (still prepares no command) yet sends exactly one bounded, sender-facing feedback message: an ephemeral reply in the mentioned channel, falling back to a DM. It never reveals the allowlist contents and only fires for explicit @-mentions (app_mention or explicit bot mention), so non-mention denials stay silent as before. The patch classifies the compiled Slack dist by content signature and fails the image build loudly if a @openclaw/slack package is present but the deny-path shape is unrecognized; it is a no-op when the Slack channel is not enabled. Wired into the Dockerfile after the messaging-plugin install. Extend the fake Slack proof to assert a denied app_mention prepares no command but produces exactly one ephemeral feedback action addressed to the sender without leaking the allowlist, and add a unit test for the patch script (injection, idempotency, no-op, and fail-loud shapes). Signed-off-by: Yimo Jiang <yimoj@nvidia.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
4e17ade to
6d875e4
Compare
|
@coderabbitai addressed both in the latest push (
The earlier "use TypeScript" comment is resolved — the shim is |
|
Tip For best results, initiate chat on the files or code changes.
🐇 ✨ |
…ontext (#4970) ## Summary Nightly E2E run [27154074940](https://github.com/NVIDIA/NemoClaw/actions/runs/27154074940) lost **48 of 67 jobs** to a single root cause: every E2E job that exercises onboard/install died at the same Dockerfile step with ``` COPY failed: file not found in build context: stat scripts/patch-openclaw-slack-deny-feedback.mts: file does not exist ``` PR #4933 added `scripts/patch-openclaw-slack-deny-feedback.mts` and the matching `COPY` line in `Dockerfile`, but did **not** register the new file in `stageOptimizedSandboxBuildContext` (`src/lib/sandbox/build-context.ts`). The optimized staging path hand-copies a curated set of files into a temp build context — its two existing siblings (`patch-openclaw-tool-catalog.js`, `patch-openclaw-chat-send.js`) are listed there explicitly. The new `.mts` patch was missed, so `docker build` couldn't see it. PR #4933 itself was green because PR-time builds were satisfied by other paths or cached layers; the gap only showed up when nightly built from main against the staged context. ## Change - `src/lib/sandbox/build-context.ts`: stage `patch-openclaw-slack-deny-feedback.mts` next to its siblings. - `test/sandbox-build-context.test.ts`: extend the fixture + assertion so any future drop of this file is caught locally before it can take down nightly. - `test/sandbox-build-context.test.ts`: parse staged Dockerfile `COPY scripts/...` lines and verify every referenced script exists in the optimized staged context, folding in the broader guard from #4971. ## Validation - `npx vitest run test/sandbox-build-context.test.ts` → 7/7 passing locally. - GitHub PR checks on updated head `7fca86f78` are green, including `build-sandbox-images`, `build-sandbox-images-arm64`, `unit-vitest-linux`, `cli-tests`, and downstream self-hosted smoke jobs. - After merge, rerun `nightly-e2e.yaml` (or `gh run rerun 27154074940 --failed`). A single onboard-stage job clearing Dockerfile step 23 confirms all 48 failures are resolved. ## Follow-up (suggested, not in this PR) The two-place duplication (`Dockerfile` `COPY` + hand-listed staging) is the structural cause of this incident. This PR now adds a contract test for that duplication; a future cleanup could still drive the staged list from a glob (`scripts/patch-openclaw-*.{js,mts}`) or shared manifest. cc @yimoj (PR #4933 author). --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
## Summary - Add the v0.0.61 release notes from the GitHub dev announcement. - Document managed vLLM recovery after host reboot and Slack denied-mention feedback. - Refresh generated `nemoclaw-user-*` skills from the source docs. ## Source summary - #4983 -> `docs/about/release-notes.mdx`: Added the v0.0.61 release summary from the dev announcement and linked behavior groups to deeper docs. - #4904 -> `docs/inference/use-local-inference.mdx`: Documented that managed vLLM restarts the `nemoclaw-vllm` container after host reboot during recovery. - #4933 -> `docs/manage-sandboxes/messaging-channels.mdx`: Documented Slack sender feedback for denied channel `@mention` events. - #4879, #4915, #4935, #4759, #4164, #4888, #4897, #4944, #4959 -> `.agents/skills/`: Refreshed generated user skills from the current source docs for release prep. ## Verification - `python3 scripts/docs-to-skills.py docs/ .agents/skills/ --prefix nemoclaw-user --doc-platform fern-mdx` - `npm run docs` (passed outside the tool sandbox after `tsx` IPC pipe creation was blocked in the sandbox) - `npm run build:cli` (refreshed local `dist/` for the pre-push TypeScript hook) - Commit and pre-push hooks passed, including docs-to-skills verification, markdownlint, gitleaks, skills YAML tests, and CLI TypeScript. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Updated sandbox security documentation with file descriptor limits. * Changed default inference model for DGX Station profile. * Enhanced agent policy and backup/restore documentation. * Improved command reference examples with clearer formatting. * Clarified Slack messaging denial notice behavior. * Added automatic vLLM container recovery during host reboot. * Updated release notes for v0.0.61. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary
A non-allowlisted human who explicitly @-mentions the bot in a Slack channel had their command correctly blocked by the
SLACK_ALLOWED_USERS/ channel-users allowlist, but OpenClaw dropped the event silently — no reply, DM, or reaction — so the sender had no signal the bot saw them. This patches the deployed sandbox's OpenClaw Slack channel to keep the command denied while sending one bounded, sender-facing feedback message.Related Issue
Fixes #4752
Changes
scripts/patch-openclaw-slack-deny-feedback.mts— NemoClaw-owned compatibility patch for the installed@openclaw/slackprepareSlackMessagedeny path. On the channel-users deny gate it keeps returning no prepared command, but for an explicit @-mention (app_mentionor an explicit bot mention) it sends exactly one sender-facing message:chat.postEphemeralin the mentioned channel, falling back to a DM. The denial text never reveals the allowlist contents and the command text is not processed. Non-mention denials stay silent as before.@openclaw/slackpackage is present but the deny-path/mention-state shape is unrecognized, and is a no-op when the Slack channel is not enabled for the image.Dockerfile— copy the patch script and run it after the messaging-plugin install (/sandbox/.openclaw, npm global root), mirroring the existingpatch-openclaw-*shims.test/e2e/lib/slack-api-proof.sh— record sender-facingchat.*calls and assert a deniedapp_mentionprepares no command yet produces exactly one ephemeral feedback action addressed to the sender, without leaking the allowlist; surfacedeniedFeedbackMethod/deniedFeedbackCountin the proof output.test/e2e/test-messaging-providers.sh— newM-S17dassertion on that bounded feedback.test/openclaw-slack-deny-feedback-patch.test.ts— unit coverage for injection + functional behavior (one ephemeral on mention, DM fallback on ephemeral failure, silent on non-mention), idempotency, no-op when no Slack package, and fail-loud on deny-gate / mention-state shape drift.Type of Change
Verification
@openclaw/slack@2026.5.27dist: deniedapp_mentionreturnednullwith zero feedback; after the patch it returnsnullwith exactly onechat.postEphemeralto the sender (DM fallback verified; non-mention denial stays silent).npx @biomejs/biome checkclean on new files;tsc -p tsconfig.cli.jsonclean.clivitest project passes except 3 pre-existing, environment-only failures unrelated to this change (openshell version fetch, rc-file ownership, GPU proof) — confirmed identical on a clean tree.Signed-off-by: Yimo Jiang yimoj@nvidia.com
Summary by CodeRabbit
New Features
@-mentionis denied — delivered as an ephemeral in-channel message (with thread context when available) or a DM fallback, without exposing allowlist details.Tests